From 6d2fa0f3d27ca6e4e2ea573cde50f46d031b6b8e Mon Sep 17 00:00:00 2001 From: PR <45575072+therealpuneeth20@users.noreply.github.com> Date: Fri, 17 May 2019 01:46:18 -0700 Subject: [PATCH] Migrate Overwrite and GetMaxPostSize() in PostStore to sync by default (#10829) * Migrate Overwrite and GetMaxPostSize() in PostStore to sync by default * GH-10762: fix return type for mock func * GH-10762: fix tests for MaxPostSize() mocks * fix imports --- app/import_functions.go | 12 +++---- app/post.go | 13 ++++--- app/post_test.go | 14 ++------ store/sqlstore/post_store.go | 55 +++++++++--------------------- store/store.go | 4 +-- store/storetest/mocks/PostStore.go | 29 ++++++++++------ store/storetest/post_store.go | 20 +++++------ 7 files changed, 60 insertions(+), 87 deletions(-) diff --git a/app/import_functions.go b/app/import_functions.go index c98f995611..823feb7cbd 100644 --- a/app/import_functions.go +++ b/app/import_functions.go @@ -898,8 +898,8 @@ func (a *App) ImportReply(data *ReplyImportData, post *model.Post, teamId string return result.Err } } else { - if result := <-a.Srv.Store.Post().Overwrite(reply); result.Err != nil { - return result.Err + if _, err := a.Srv.Store.Post().Overwrite(reply); err != nil { + return err } } @@ -1000,8 +1000,8 @@ func (a *App) ImportPost(data *PostImportData, dryRun bool) *model.AppError { return result.Err } } else { - if result := <-a.Srv.Store.Post().Overwrite(post); result.Err != nil { - return result.Err + if _, err := a.Srv.Store.Post().Overwrite(post); err != nil { + return err } } @@ -1224,8 +1224,8 @@ func (a *App) ImportDirectPost(data *DirectPostImportData, dryRun bool) *model.A return result.Err } } else { - if result := <-a.Srv.Store.Post().Overwrite(post); result.Err != nil { - return result.Err + if _, err := a.Srv.Store.Post().Overwrite(post); err != nil { + return err } } diff --git a/app/post.go b/app/post.go index 9a3bf7366b..9412c021b1 100644 --- a/app/post.go +++ b/app/post.go @@ -315,9 +315,8 @@ func (a *App) attachFilesToPost(post *model.Post) *model.AppError { // We couldn't attach all files to the post, so ensure that post.FileIds reflects what was actually attached post.FileIds = attachedIds - result := <-a.Srv.Store.Post().Overwrite(post) - if result.Err != nil { - return result.Err + if _, err := a.Srv.Store.Post().Overwrite(post); err != nil { + return err } } @@ -1027,10 +1026,10 @@ func (a *App) ImageProxyRemover() (f func(string) string) { } func (a *App) MaxPostSize() int { - result := <-a.Srv.Store.Post().GetMaxPostSize() - if result.Err != nil { - mlog.Error(fmt.Sprint(result.Err)) + maxPostSize := a.Srv.Store.Post().GetMaxPostSize() + if maxPostSize == 0 { return model.POST_MESSAGE_MAX_RUNES_V1 } - return result.Data.(int) + + return maxPostSize } diff --git a/app/post_test.go b/app/post_test.go index aa9f317c55..ebe5822ab5 100644 --- a/app/post_test.go +++ b/app/post_test.go @@ -14,7 +14,6 @@ import ( "github.com/stretchr/testify/require" "github.com/mattermost/mattermost-server/model" - "github.com/mattermost/mattermost-server/store" "github.com/mattermost/mattermost-server/store/storetest" ) @@ -565,25 +564,21 @@ func TestMaxPostSize(t *testing.T) { Description string StoreMaxPostSize int ExpectedMaxPostSize int - ExpectedError *model.AppError }{ { - "error fetching max post size", + "Max post size less than model.model.POST_MESSAGE_MAX_RUNES_V1 ", 0, model.POST_MESSAGE_MAX_RUNES_V1, - model.NewAppError("TestMaxPostSize", "this is an error", nil, "", http.StatusBadRequest), }, { "4000 rune limit", 4000, 4000, - nil, }, { "16383 rune limit", 16383, 16383, - nil, }, } @@ -595,12 +590,7 @@ func TestMaxPostSize(t *testing.T) { mockStore := &storetest.Store{} defer mockStore.AssertExpectations(t) - mockStore.PostStore.On("GetMaxPostSize").Return( - storetest.NewStoreChannel(store.StoreResult{ - Data: testCase.StoreMaxPostSize, - Err: testCase.ExpectedError, - }), - ) + mockStore.PostStore.On("GetMaxPostSize").Return(testCase.StoreMaxPostSize) app := App{ Srv: &Server{ diff --git a/store/sqlstore/post_store.go b/store/sqlstore/post_store.go index d1828cea9f..e56061ecf5 100644 --- a/store/sqlstore/post_store.go +++ b/store/sqlstore/post_store.go @@ -97,13 +97,7 @@ func (s *SqlPostStore) Save(post *model.Post) store.StoreChannel { return } - var maxPostSize int - if result := <-s.GetMaxPostSize(); result.Err != nil { - result.Err = model.NewAppError("SqlPostStore.Save", "store.sql_post.save.app_error", nil, "id="+post.Id+", "+result.Err.Error(), http.StatusInternalServerError) - return - } else { - maxPostSize = result.Data.(int) - } + maxPostSize := s.GetMaxPostSize() post.PreSave() if result.Err = post.IsValid(maxPostSize); result.Err != nil { @@ -146,13 +140,7 @@ func (s *SqlPostStore) Update(newPost *model.Post, oldPost *model.Post) store.St oldPost.Id = model.NewId() oldPost.PreCommit() - var maxPostSize int - if result := <-s.GetMaxPostSize(); result.Err != nil { - result.Err = model.NewAppError("SqlPostStore.Save", "store.sql_post.update.app_error", nil, "id="+newPost.Id+", "+result.Err.Error(), http.StatusInternalServerError) - return - } else { - maxPostSize = result.Data.(int) - } + maxPostSize := s.GetMaxPostSize() if result.Err = newPost.IsValid(maxPostSize); result.Err != nil { return @@ -176,28 +164,19 @@ func (s *SqlPostStore) Update(newPost *model.Post, oldPost *model.Post) store.St }) } -func (s *SqlPostStore) Overwrite(post *model.Post) store.StoreChannel { - return store.Do(func(result *store.StoreResult) { - post.UpdateAt = model.GetMillis() +func (s *SqlPostStore) Overwrite(post *model.Post) (*model.Post, *model.AppError) { + post.UpdateAt = model.GetMillis() - var maxPostSize int - if result := <-s.GetMaxPostSize(); result.Err != nil { - result.Err = model.NewAppError("SqlPostStore.Save", "store.sql_post.overwrite.app_error", nil, "id="+post.Id+", "+result.Err.Error(), http.StatusInternalServerError) - return - } else { - maxPostSize = result.Data.(int) - } + maxPostSize := s.GetMaxPostSize() + if appErr := post.IsValid(maxPostSize); appErr != nil { + return nil, appErr + } - if result.Err = post.IsValid(maxPostSize); result.Err != nil { - return - } + if _, err := s.GetMaster().Update(post); err != nil { + return nil, model.NewAppError("SqlPostStore.Overwrite", "store.sql_post.overwrite.app_error", nil, "id="+post.Id+", "+err.Error(), http.StatusInternalServerError) + } - if _, err := s.GetMaster().Update(post); err != nil { - result.Err = model.NewAppError("SqlPostStore.Overwrite", "store.sql_post.overwrite.app_error", nil, "id="+post.Id+", "+err.Error(), http.StatusInternalServerError) - } else { - result.Data = post - } - }) + return post, nil } func (s *SqlPostStore) GetFlaggedPosts(userId string, offset int, limit int) store.StoreChannel { @@ -1296,13 +1275,11 @@ func (s *SqlPostStore) determineMaxPostSize() int { } // GetMaxPostSize returns the maximum number of runes that may be stored in a post. -func (s *SqlPostStore) GetMaxPostSize() store.StoreChannel { - return store.Do(func(result *store.StoreResult) { - s.maxPostSizeOnce.Do(func() { - s.maxPostSizeCached = s.determineMaxPostSize() - }) - result.Data = s.maxPostSizeCached +func (s *SqlPostStore) GetMaxPostSize() int { + s.maxPostSizeOnce.Do(func() { + s.maxPostSizeCached = s.determineMaxPostSize() }) + return s.maxPostSizeCached } func (s *SqlPostStore) GetParentsForExportAfter(limit int, afterId string) store.StoreChannel { diff --git a/store/store.go b/store/store.go index f43c0e2c76..36cee04ef9 100644 --- a/store/store.go +++ b/store/store.go @@ -232,12 +232,12 @@ type PostStore interface { ClearCaches() InvalidateLastPostTimeCache(channelId string) GetPostsCreatedAt(channelId string, time int64) StoreChannel - Overwrite(post *model.Post) StoreChannel + Overwrite(post *model.Post) (*model.Post, *model.AppError) GetPostsByIds(postIds []string) StoreChannel GetPostsBatchForIndexing(startTime int64, endTime int64, limit int) StoreChannel PermanentDeleteBatch(endTime int64, limit int64) StoreChannel GetOldest() StoreChannel - GetMaxPostSize() StoreChannel + GetMaxPostSize() int GetParentsForExportAfter(limit int, afterId string) StoreChannel GetRepliesForExport(parentId string) StoreChannel GetDirectPostParentsForExportAfter(limit int, afterId string) StoreChannel diff --git a/store/storetest/mocks/PostStore.go b/store/storetest/mocks/PostStore.go index 6f8742f870..8e7a133bfc 100644 --- a/store/storetest/mocks/PostStore.go +++ b/store/storetest/mocks/PostStore.go @@ -179,16 +179,14 @@ func (_m *PostStore) GetFlaggedPostsForTeam(userId string, teamId string, offset } // GetMaxPostSize provides a mock function with given fields: -func (_m *PostStore) GetMaxPostSize() store.StoreChannel { +func (_m *PostStore) GetMaxPostSize() int { ret := _m.Called() - var r0 store.StoreChannel - if rf, ok := ret.Get(0).(func() store.StoreChannel); ok { + var r0 int + if rf, ok := ret.Get(0).(func() int); ok { r0 = rf() } else { - if ret.Get(0) != nil { - r0 = ret.Get(0).(store.StoreChannel) - } + r0 = ret.Get(0).(int) } return r0 @@ -376,19 +374,28 @@ func (_m *PostStore) InvalidateLastPostTimeCache(channelId string) { } // Overwrite provides a mock function with given fields: post -func (_m *PostStore) Overwrite(post *model.Post) store.StoreChannel { +func (_m *PostStore) Overwrite(post *model.Post) (*model.Post, *model.AppError) { ret := _m.Called(post) - var r0 store.StoreChannel - if rf, ok := ret.Get(0).(func(*model.Post) store.StoreChannel); ok { + var r0 *model.Post + if rf, ok := ret.Get(0).(func(*model.Post) *model.Post); ok { r0 = rf(post) } else { if ret.Get(0) != nil { - r0 = ret.Get(0).(store.StoreChannel) + r0 = ret.Get(0).(*model.Post) } } - return r0 + var r1 *model.AppError + if rf, ok := ret.Get(1).(func(*model.Post) *model.AppError); ok { + r1 = rf(post) + } else { + if ret.Get(1) != nil { + r1 = ret.Get(1).(*model.AppError) + } + } + + return r0, r1 } // PermanentDeleteBatch provides a mock function with given fields: endTime, limit diff --git a/store/storetest/post_store.go b/store/storetest/post_store.go index 1149e3501d..5b970440ce 100644 --- a/store/storetest/post_store.go +++ b/store/storetest/post_store.go @@ -1638,8 +1638,8 @@ func testPostStoreOverwrite(t *testing.T, ss store.Store) { o1a := &model.Post{} *o1a = *ro1 o1a.Message = ro1.Message + "BBBBBBBBBB" - if result := <-ss.Post().Overwrite(o1a); result.Err != nil { - t.Fatal(result.Err) + if _, err := ss.Post().Overwrite(o1a); err != nil { + t.Fatal(err) } ro1a := (<-ss.Post().Get(o1.Id)).Data.(*model.PostList).Posts[o1.Id] @@ -1651,8 +1651,8 @@ func testPostStoreOverwrite(t *testing.T, ss store.Store) { o2a := &model.Post{} *o2a = *ro2 o2a.Message = ro2.Message + "DDDDDDD" - if result := <-ss.Post().Overwrite(o2a); result.Err != nil { - t.Fatal(result.Err) + if _, err := ss.Post().Overwrite(o2a); err != nil { + t.Fatal(err) } ro2a := (<-ss.Post().Get(o1.Id)).Data.(*model.PostList).Posts[o2.Id] @@ -1664,8 +1664,8 @@ func testPostStoreOverwrite(t *testing.T, ss store.Store) { o3a := &model.Post{} *o3a = *ro3 o3a.Message = ro3.Message + "WWWWWWW" - if result := <-ss.Post().Overwrite(o3a); result.Err != nil { - t.Fatal(result.Err) + if _, err := ss.Post().Overwrite(o3a); err != nil { + t.Fatal(err) } ro3a := (<-ss.Post().Get(o3.Id)).Data.(*model.PostList).Posts[o3.Id] @@ -1687,8 +1687,8 @@ func testPostStoreOverwrite(t *testing.T, ss store.Store) { *o4a = *ro4 o4a.Filenames = []string{} o4a.FileIds = []string{model.NewId()} - if result := <-ss.Post().Overwrite(o4a); result.Err != nil { - t.Fatal(result.Err) + if _, err := ss.Post().Overwrite(o4a); err != nil { + t.Fatal(err) } if ro4a := store.Must(ss.Post().Get(o4.Id)).(*model.PostList).Posts[o4.Id]; len(ro4a.Filenames) != 0 { @@ -1872,8 +1872,8 @@ func testPostStoreGetOldest(t *testing.T, ss store.Store) { } func testGetMaxPostSize(t *testing.T, ss store.Store) { - assert.Equal(t, model.POST_MESSAGE_MAX_RUNES_V2, (<-ss.Post().GetMaxPostSize()).Data.(int)) - assert.Equal(t, model.POST_MESSAGE_MAX_RUNES_V2, (<-ss.Post().GetMaxPostSize()).Data.(int)) + assert.Equal(t, model.POST_MESSAGE_MAX_RUNES_V2, ss.Post().GetMaxPostSize()) + assert.Equal(t, model.POST_MESSAGE_MAX_RUNES_V2, ss.Post().GetMaxPostSize()) } func testPostStoreGetParentsForExportAfter(t *testing.T, ss store.Store) {