MM-49564: Drafts upsert in the Store vs App layer (#22530)

* MM-49564: Upsert in the Store vs App layer Refactor drafts so that Upserting a draft would happen in the DB and not in the app layer. * Fixes mocks * Fixes tests * Fixes translations * Fixes tests * Update tests * Fixes tests * Addresses review comments - renames Save => Upsert - removes Sleep from tests * Fixes flaky test --------- Co-authored-by: Mattermost Build <build@mattermost.com>
2023-04-01 10:32:40 +03:00
--- a/server/channels/store/opentracinglayer/opentracinglayer.go
+++ b/server/channels/store/opentracinglayer/opentracinglayer.go
@@ -3315,34 +3315,16 @@ func (s *OpenTracingLayerDraftStore) GetDraftsForUser(userID string, teamID stri
 	return result, err
 }

-func (s *OpenTracingLayerDraftStore) Save(d *model.Draft) (*model.Draft, error) {
+func (s *OpenTracingLayerDraftStore) Upsert(d *model.Draft) (*model.Draft, error) {
 	origCtx := s.Root.Store.Context()
-	span, newCtx := tracing.StartSpanWithParentByContext(s.Root.Store.Context(), "DraftStore.Save")
+	span, newCtx := tracing.StartSpanWithParentByContext(s.Root.Store.Context(), "DraftStore.Upsert")
 	s.Root.Store.SetContext(newCtx)
 	defer func() {
 		s.Root.Store.SetContext(origCtx)
 	}()

 	defer span.Finish()
-	result, err := s.DraftStore.Save(d)
-	if err != nil {
-		span.LogFields(spanlog.Error(err))
-		ext.Error.Set(span, true)
-	}
-
-	return result, err
-}
-
-func (s *OpenTracingLayerDraftStore) Update(d *model.Draft) (*model.Draft, error) {
-	origCtx := s.Root.Store.Context()
-	span, newCtx := tracing.StartSpanWithParentByContext(s.Root.Store.Context(), "DraftStore.Update")
-	s.Root.Store.SetContext(newCtx)
-	defer func() {
-		s.Root.Store.SetContext(origCtx)
-	}()
-
-	defer span.Finish()
-	result, err := s.DraftStore.Update(d)
+	result, err := s.DraftStore.Upsert(d)
 	if err != nil {
 		span.LogFields(spanlog.Error(err))
 		ext.Error.Set(span, true)
--- a/server/channels/store/retrylayer/retrylayer.go
+++ b/server/channels/store/retrylayer/retrylayer.go
@@ -3703,32 +3703,11 @@ func (s *RetryLayerDraftStore) GetDraftsForUser(userID string, teamID string) ([

 }

-func (s *RetryLayerDraftStore) Save(d *model.Draft) (*model.Draft, error) {
+func (s *RetryLayerDraftStore) Upsert(d *model.Draft) (*model.Draft, error) {

 	tries := 0
 	for {
-		result, err := s.DraftStore.Save(d)
-		if err == nil {
-			return result, nil
-		}
-		if !isRepeatableError(err) {
-			return result, err
-		}
-		tries++
-		if tries >= 3 {
-			err = errors.Wrap(err, "giving up after 3 consecutive repeatable transaction failures")
-			return result, err
-		}
-		timepkg.Sleep(100 * timepkg.Millisecond)
-	}
-
-}
-
-func (s *RetryLayerDraftStore) Update(d *model.Draft) (*model.Draft, error) {
-
-	tries := 0
-	for {
-		result, err := s.DraftStore.Update(d)
+		result, err := s.DraftStore.Upsert(d)
 		if err == nil {
 			return result, nil
 		}
--- a/server/channels/store/sqlstore/draft_store.go
+++ b/server/channels/store/sqlstore/draft_store.go
@@ -88,7 +88,7 @@ func (s *SqlDraftStore) Get(userId, channelId, rootId string, includeDeleted boo
 	return &dt, nil
 }

-func (s *SqlDraftStore) Save(draft *model.Draft) (*model.Draft, error) {
+func (s *SqlDraftStore) Upsert(draft *model.Draft) (*model.Draft, error) {
 	draft.PreSave()
 	maxDraftSize := s.GetMaxDraftSize()
 	if err := draft.IsValid(maxDraftSize); err != nil {
@@ -96,6 +96,13 @@ func (s *SqlDraftStore) Save(draft *model.Draft) (*model.Draft, error) {
 	}

 	builder := s.getQueryBuilder().Insert("Drafts").Columns(draftSliceColumns()...).Values(draftToSlice(draft)...)
+
+	if s.DriverName() == model.DatabaseDriverMysql {
+		builder = builder.SuffixExpr(sq.Expr("ON DUPLICATE KEY UPDATE  UpdateAt = ?, Message = ?, Props = ?, FileIds = ?, Priority = ?, DeleteAt = ?", draft.UpdateAt, draft.Message, draft.Props, draft.FileIds, draft.Priority, 0))
+	} else {
+		builder = builder.SuffixExpr(sq.Expr("ON CONFLICT (UserId, ChannelId, RootId) DO UPDATE SET UpdateAt = ?, Message = ?, Props = ?, FileIds = ?, Priority = ?, DeleteAt = ?", draft.UpdateAt, draft.Message, draft.Props, draft.FileIds, draft.Priority, 0))
+	}
+
 	query, args, err := builder.ToSql()

 	if err != nil {
@@ -103,36 +110,7 @@ func (s *SqlDraftStore) Save(draft *model.Draft) (*model.Draft, error) {
 	}

 	if _, err = s.GetMasterX().Exec(query, args...); err != nil {
-		return nil, errors.Wrap(err, "failed to save Draft")
-	}
-
-	return draft, nil
-}
-
-func (s *SqlDraftStore) Update(draft *model.Draft) (*model.Draft, error) {
-	draft.PreUpdate()
-
-	maxDraftSize := s.GetMaxDraftSize()
-	if err := draft.IsValid(maxDraftSize); err != nil {
-		return nil, err
-	}
-
-	query := s.getQueryBuilder().
-		Update("Drafts").
-		Set("UpdateAt", draft.UpdateAt).
-		Set("Message", draft.Message).
-		Set("Props", draft.Props).
-		Set("FileIds", draft.FileIds).
-		Set("Priority", draft.Priority).
-		Set("DeleteAt", 0).
-		Where(sq.Eq{
-			"UserId":    draft.UserId,
-			"ChannelId": draft.ChannelId,
-			"RootId":    draft.RootId,
-		})
-
-	if _, err := s.GetMasterX().ExecBuilder(query); err != nil {
-		return nil, errors.Wrapf(err, "failed to update Draft with channelid=%s", draft.ChannelId)
+		return nil, errors.Wrap(err, "failed to upsert Draft")
 	}

 	return draft, nil
--- a/server/channels/store/store.go
+++ b/server/channels/store/store.go
@@ -985,11 +985,10 @@ type PostPriorityStore interface {
 }

 type DraftStore interface {
-	Save(d *model.Draft) (*model.Draft, error)
+	Upsert(d *model.Draft) (*model.Draft, error)
 	Get(userID, channelID, rootID string, includeDeleted bool) (*model.Draft, error)
 	Delete(userID, channelID, rootID string) error
 	GetDraftsForUser(userID, teamID string) ([]*model.Draft, error)
-	Update(d *model.Draft) (*model.Draft, error)
 }

 type PostAcknowledgementStore interface {
--- a/server/channels/store/storetest/draft_store.go
+++ b/server/channels/store/storetest/draft_store.go
@@ -68,17 +68,21 @@ func testSaveDraft(t *testing.T, ss store.Store) {
 	}

 	t.Run("save drafts", func(t *testing.T) {
-		draftResp, err := ss.Draft().Save(draft1)
+		draftResp, err := ss.Draft().Upsert(draft1)
 		assert.NoError(t, err)

 		assert.Equal(t, draft1.Message, draftResp.Message)
 		assert.Equal(t, draft1.ChannelId, draftResp.ChannelId)

-		draftResp, err = ss.Draft().Save(draft2)
+		draftResp, err = ss.Draft().Upsert(draft2)
 		assert.NoError(t, err)

 		assert.Equal(t, draft2.Message, draftResp.Message)
 		assert.Equal(t, draft2.ChannelId, draftResp.ChannelId)
+
+		drafts, err := ss.Draft().GetDraftsForUser(user.Id, "")
+		assert.NoError(t, err)
+		assert.Len(t, drafts, 2)
 	})
 }

@@ -90,56 +94,52 @@ func testUpdateDraft(t *testing.T, ss store.Store) {
 	channel := &model.Channel{
 		Id: model.NewId(),
 	}
-	channel2 := &model.Channel{
-		Id: model.NewId(),
-	}

-	member1 := &model.ChannelMember{
+	member := &model.ChannelMember{
 		ChannelId:   channel.Id,
 		UserId:      user.Id,
 		NotifyProps: model.GetDefaultChannelNotifyProps(),
 	}

-	member2 := &model.ChannelMember{
-		ChannelId:   channel2.Id,
-		UserId:      user.Id,
-		NotifyProps: model.GetDefaultChannelNotifyProps(),
-	}
-
-	_, err := ss.Channel().SaveMember(member1)
+	_, err := ss.Channel().SaveMember(member)
 	require.NoError(t, err)

-	_, err = ss.Channel().SaveMember(member2)
-	require.NoError(t, err)
-
-	draft1 := &model.Draft{
-		CreateAt:  00001,
-		UpdateAt:  00001,
-		UserId:    user.Id,
-		ChannelId: channel.Id,
-		Message:   "draft1",
-	}
-
-	draft2 := &model.Draft{
-		CreateAt:  00005,
-		UpdateAt:  00005,
-		UserId:    user.Id,
-		ChannelId: channel2.Id,
-		Message:   "draft2",
-	}
-
 	t.Run("update drafts", func(t *testing.T) {
-		draftResp, err := ss.Draft().Update(draft1)
+		draft := &model.Draft{
+			UserId:    user.Id,
+			ChannelId: channel.Id,
+			Message:   "draft",
+		}
+		_, err := ss.Draft().Upsert(draft)
 		assert.NoError(t, err)

-		assert.Equal(t, draft1.Message, draftResp.Message)
-		assert.Equal(t, draft1.ChannelId, draftResp.ChannelId)
+		drafts, err := ss.Draft().GetDraftsForUser(user.Id, "")
+		assert.NoError(t, err)
+		assert.Len(t, drafts, 1)
+		draft1 := drafts[0]

-		draftResp, err = ss.Draft().Update(draft2)
+		assert.Greater(t, draft1.CreateAt, int64(0))
+		assert.Equal(t, draft1.UpdateAt, draft1.CreateAt)
+		assert.Equal(t, channel.Id, draft1.ChannelId)
+		assert.Equal(t, "draft", draft1.Message)
+
+		updatedDraft := &model.Draft{
+			UserId:    user.Id,
+			ChannelId: channel.Id,
+			Message:   "updatedDraft",
+		}
+		_, err = ss.Draft().Upsert(updatedDraft)
 		assert.NoError(t, err)

-		assert.Equal(t, draft2.Message, draftResp.Message)
-		assert.Equal(t, draft2.ChannelId, draftResp.ChannelId)
+		drafts, err = ss.Draft().GetDraftsForUser(user.Id, "")
+		assert.NoError(t, err)
+		assert.Len(t, drafts, 1)
+		draft2 := drafts[0]
+
+		assert.Greater(t, draft2.CreateAt, int64(0))
+		assert.Equal(t, "updatedDraft", draft2.Message)
+		assert.Equal(t, channel.Id, draft2.ChannelId)
+		assert.Equal(t, draft1.CreateAt, draft2.CreateAt)
 	})
 }

@@ -189,10 +189,10 @@ func testDeleteDraft(t *testing.T, ss store.Store) {
 		Message:   "draft2",
 	}

-	_, err = ss.Draft().Save(draft1)
+	_, err = ss.Draft().Upsert(draft1)
 	require.NoError(t, err)

-	_, err = ss.Draft().Save(draft2)
+	_, err = ss.Draft().Upsert(draft2)
 	require.NoError(t, err)

 	t.Run("delete drafts", func(t *testing.T) {
@@ -258,10 +258,10 @@ func testGetDraft(t *testing.T, ss store.Store) {
 		Message:   "draft2",
 	}

-	_, err = ss.Draft().Save(draft1)
+	_, err = ss.Draft().Upsert(draft1)
 	require.NoError(t, err)

-	_, err = ss.Draft().Save(draft2)
+	_, err = ss.Draft().Upsert(draft2)
 	require.NoError(t, err)

 	t.Run("get drafts", func(t *testing.T) {
@@ -326,35 +326,28 @@ func testGetDraftsForUser(t *testing.T, ss store.Store) {
 	require.NoError(t, err)

 	draft1 := &model.Draft{
-		CreateAt:  00001,
-		UpdateAt:  00001,
 		UserId:    user.Id,
 		ChannelId: channel.Id,
 		Message:   "draft1",
 	}

 	draft2 := &model.Draft{
-		CreateAt:  00005,
-		UpdateAt:  00005,
 		UserId:    user.Id,
 		ChannelId: channel2.Id,
 		Message:   "draft2",
 	}

-	_, err = ss.Draft().Save(draft1)
+	_, err = ss.Draft().Upsert(draft1)
 	require.NoError(t, err)

-	_, err = ss.Draft().Save(draft2)
+	_, err = ss.Draft().Upsert(draft2)
 	require.NoError(t, err)

 	t.Run("get drafts", func(t *testing.T) {
 		draftResp, err := ss.Draft().GetDraftsForUser(user.Id, "")
 		assert.NoError(t, err)
+		assert.Len(t, draftResp, 2)

-		assert.Equal(t, draft2.Message, draftResp[0].Message)
-		assert.Equal(t, draft2.ChannelId, draftResp[0].ChannelId)
-
-		assert.Equal(t, draft1.Message, draftResp[1].Message)
-		assert.Equal(t, draft1.ChannelId, draftResp[1].ChannelId)
+		assert.ElementsMatch(t, []*model.Draft{draft1, draft2}, draftResp)
 	})
 }
--- a/server/channels/store/storetest/mocks/DraftStore.go
+++ b/server/channels/store/storetest/mocks/DraftStore.go
@@ -74,31 +74,8 @@ func (_m *DraftStore) GetDraftsForUser(userID string, teamID string) ([]*model.D
 	return r0, r1
 }

-// Save provides a mock function with given fields: d
-func (_m *DraftStore) Save(d *model.Draft) (*model.Draft, error) {
-	ret := _m.Called(d)
-
-	var r0 *model.Draft
-	if rf, ok := ret.Get(0).(func(*model.Draft) *model.Draft); ok {
-		r0 = rf(d)
-	} else {
-		if ret.Get(0) != nil {
-			r0 = ret.Get(0).(*model.Draft)
-		}
-	}
-
-	var r1 error
-	if rf, ok := ret.Get(1).(func(*model.Draft) error); ok {
-		r1 = rf(d)
-	} else {
-		r1 = ret.Error(1)
-	}
-
-	return r0, r1
-}
-
-// Update provides a mock function with given fields: d
-func (_m *DraftStore) Update(d *model.Draft) (*model.Draft, error) {
+// Upsert provides a mock function with given fields: d
+func (_m *DraftStore) Upsert(d *model.Draft) (*model.Draft, error) {
 	ret := _m.Called(d)

 	var r0 *model.Draft
--- a/server/channels/store/timerlayer/timerlayer.go
+++ b/server/channels/store/timerlayer/timerlayer.go
@@ -3038,10 +3038,10 @@ func (s *TimerLayerDraftStore) GetDraftsForUser(userID string, teamID string) ([
 	return result, err
 }

-func (s *TimerLayerDraftStore) Save(d *model.Draft) (*model.Draft, error) {
+func (s *TimerLayerDraftStore) Upsert(d *model.Draft) (*model.Draft, error) {
 	start := time.Now()

-	result, err := s.DraftStore.Save(d)
+	result, err := s.DraftStore.Upsert(d)

 	elapsed := float64(time.Since(start)) / float64(time.Second)
 	if s.Root.Metrics != nil {
@@ -3049,23 +3049,7 @@ func (s *TimerLayerDraftStore) Save(d *model.Draft) (*model.Draft, error) {
 		if err == nil {
 			success = "true"
 		}
-		s.Root.Metrics.ObserveStoreMethodDuration("DraftStore.Save", success, elapsed)
-	}
-	return result, err
-}
-
-func (s *TimerLayerDraftStore) Update(d *model.Draft) (*model.Draft, error) {
-	start := time.Now()
-
-	result, err := s.DraftStore.Update(d)
-
-	elapsed := float64(time.Since(start)) / float64(time.Second)
-	if s.Root.Metrics != nil {
-		success := "false"
-		if err == nil {
-			success = "true"
-		}
-		s.Root.Metrics.ObserveStoreMethodDuration("DraftStore.Update", success, elapsed)
+		s.Root.Metrics.ObserveStoreMethodDuration("DraftStore.Upsert", success, elapsed)
 	}
 	return result, err
 }