From 71925ea224666ce2f869f2607cde71341c4fa6d5 Mon Sep 17 00:00:00 2001 From: Agniva De Sarker Date: Thu, 2 Jul 2020 09:43:28 +0530 Subject: [PATCH] MM-26031: Migrate reaction store to plain errors (#14931) * ReactionStore migration to return plain errors * Fix translations * FixImports * Rollback fix imports * Fix merge conflict * add ent translation Co-authored-by: Rodrigo Villablanca Co-authored-by: Mattermod --- app/export.go | 9 ++-- app/import_functions.go | 10 ++++- app/import_functions_test.go | 12 ++--- app/reaction.go | 27 +++++++---- i18n/en.json | 56 +++++++++-------------- store/localcachelayer/reaction_layer.go | 8 ++-- store/opentracing_layer.go | 12 ++--- store/sqlstore/supplier_reactions.go | 44 ++++++++---------- store/store.go | 12 ++--- store/storetest/mocks/ReactionStore.go | 60 ++++++++++--------------- store/storetest/reaction_store.go | 55 ++++++++++++----------- store/timer_layer.go | 12 ++--- 12 files changed, 150 insertions(+), 167 deletions(-) diff --git a/app/export.go b/app/export.go index 1c43a35165..348bc77df2 100644 --- a/app/export.go +++ b/app/export.go @@ -403,14 +403,13 @@ func (a *App) buildPostReplies(postId string) (*[]ReplyImportData, *model.AppErr func (a *App) BuildPostReactions(postId string) (*[]ReactionImportData, *model.AppError) { var reactionsOfPost []ReactionImportData - reactions, err := a.Srv().Store.Reaction().GetForPost(postId, true) - if err != nil { - return nil, err + reactions, nErr := a.Srv().Store.Reaction().GetForPost(postId, true) + if nErr != nil { + return nil, model.NewAppError("BuildPostReactions", "app.reaction.get_for_post.app_error", nil, nErr.Error(), http.StatusInternalServerError) } for _, reaction := range reactions { - var user *model.User - user, err = a.Srv().Store.User().Get(reaction.UserId) + user, err := a.Srv().Store.User().Get(reaction.UserId) if err != nil { if err.Id == store.MISSING_ACCOUNT_ERROR { // this is a valid case, the user that reacted might've been deleted by now mlog.Info("Skipping reactions by user since the entity doesn't exist anymore", mlog.String("user_id", reaction.UserId)) diff --git a/app/import_functions.go b/app/import_functions.go index ec28c8d2d4..411b97e483 100644 --- a/app/import_functions.go +++ b/app/import_functions.go @@ -953,8 +953,14 @@ func (a *App) importReaction(data *ReactionImportData, post *model.Post, dryRun EmojiName: *data.EmojiName, CreateAt: *data.CreateAt, } - if _, err = a.Srv().Store.Reaction().Save(reaction); err != nil { - return err + if _, nErr := a.Srv().Store.Reaction().Save(reaction); nErr != nil { + var appErr *model.AppError + switch { + case errors.As(nErr, &appErr): + return appErr + default: + return model.NewAppError("importReaction", "app.reaction.save.save.app_error", nil, nErr.Error(), http.StatusInternalServerError) + } } return nil diff --git a/app/import_functions_test.go b/app/import_functions_test.go index 34f99adcb2..39885a43d8 100644 --- a/app/import_functions_test.go +++ b/app/import_functions_test.go @@ -2278,8 +2278,8 @@ func TestImportimportMultiplePostLines(t *testing.T) { postBool = post.Message != *data.Post.Message || post.CreateAt != *data.Post.CreateAt || post.UserId != user.Id || !post.HasReactions require.False(t, postBool, "Post properties not as expected") - reactions, err := th.App.Srv().Store.Reaction().GetForPost(post.Id, false) - require.Nil(t, err, "Can't get reaction") + reactions, nErr := th.App.Srv().Store.Reaction().GetForPost(post.Id, false) + require.Nil(t, nErr, "Can't get reaction") require.Len(t, reactions, 1, "Invalid number of reactions") @@ -2768,8 +2768,8 @@ func TestImportImportPost(t *testing.T) { postBool := post.Message != *data.Post.Message || post.CreateAt != *data.Post.CreateAt || post.UserId != user.Id || !post.HasReactions require.False(t, postBool, "Post properties not as expected") - reactions, err := th.App.Srv().Store.Reaction().GetForPost(post.Id, false) - require.Nil(t, err, "Can't get reaction") + reactions, nErr := th.App.Srv().Store.Reaction().GetForPost(post.Id, false) + require.Nil(t, nErr, "Can't get reaction") require.Len(t, reactions, 1, "Invalid number of reactions") }) @@ -3635,8 +3635,8 @@ func TestImportImportDirectPost(t *testing.T) { postBool := post.Message != *data.DirectPost.Message || post.CreateAt != *data.DirectPost.CreateAt || post.UserId != th.BasicUser.Id || !post.HasReactions require.False(t, postBool, "Post properties not as expected") - reactions, err := th.App.Srv().Store.Reaction().GetForPost(post.Id, false) - require.Nil(t, err, "Can't get reaction") + reactions, nErr := th.App.Srv().Store.Reaction().GetForPost(post.Id, false) + require.Nil(t, nErr, "Can't get reaction") require.Len(t, reactions, 1, "Invalid number of reactions") }) diff --git a/app/reaction.go b/app/reaction.go index dc3c6cb453..2b2fde21f9 100644 --- a/app/reaction.go +++ b/app/reaction.go @@ -4,6 +4,7 @@ package app import ( + "errors" "net/http" "github.com/mattermost/mattermost-server/v5/model" @@ -36,9 +37,15 @@ func (a *App) SaveReactionForPost(reaction *model.Reaction) (*model.Reaction, *m } } - reaction, err = a.Srv().Store.Reaction().Save(reaction) - if err != nil { - return nil, err + reaction, nErr := a.Srv().Store.Reaction().Save(reaction) + if nErr != nil { + var appErr *model.AppError + switch { + case errors.As(nErr, &appErr): + return nil, appErr + default: + return nil, model.NewAppError("SaveReactionForPost", "app.reaction.save.save.app_error", nil, nErr.Error(), http.StatusInternalServerError) + } } // The post is always modified since the UpdateAt always changes @@ -52,7 +59,11 @@ func (a *App) SaveReactionForPost(reaction *model.Reaction) (*model.Reaction, *m } func (a *App) GetReactionsForPost(postId string) ([]*model.Reaction, *model.AppError) { - return a.Srv().Store.Reaction().GetForPost(postId, true) + reactions, err := a.Srv().Store.Reaction().GetForPost(postId, true) + if err != nil { + return nil, model.NewAppError("GetReactionsForPost", "app.reaction.get_for_post.app_error", nil, err.Error(), http.StatusInternalServerError) + } + return reactions, nil } func (a *App) GetBulkReactionsForPosts(postIds []string) (map[string][]*model.Reaction, *model.AppError) { @@ -60,7 +71,7 @@ func (a *App) GetBulkReactionsForPosts(postIds []string) (map[string][]*model.Re allReactions, err := a.Srv().Store.Reaction().BulkGetForPosts(postIds) if err != nil { - return nil, err + return nil, model.NewAppError("GetBulkReactionsForPosts", "app.reaction.bulk_get_for_post_ids.app_error", nil, err.Error(), http.StatusInternalServerError) } for _, reaction := range allReactions { @@ -95,7 +106,7 @@ func (a *App) DeleteReactionForPost(reaction *model.Reaction) *model.AppError { } if channel.DeleteAt > 0 { - return model.NewAppError("deleteReactionForPost", "api.reaction.delete.archived_channel.app_error", nil, "", http.StatusForbidden) + return model.NewAppError("DeleteReactionForPost", "api.reaction.delete.archived_channel.app_error", nil, "", http.StatusForbidden) } if a.Srv().License() != nil && *a.Config().TeamSettings.ExperimentalTownSquareIsReadOnly && channel.Name == model.DEFAULT_CHANNEL { @@ -105,7 +116,7 @@ func (a *App) DeleteReactionForPost(reaction *model.Reaction) *model.AppError { } if !a.RolesGrantPermission(user.GetRoles(), model.PERMISSION_MANAGE_SYSTEM.Id) { - return model.NewAppError("deleteReactionForPost", "api.reaction.town_square_read_only", nil, "", http.StatusForbidden) + return model.NewAppError("DeleteReactionForPost", "api.reaction.town_square_read_only", nil, "", http.StatusForbidden) } } @@ -115,7 +126,7 @@ func (a *App) DeleteReactionForPost(reaction *model.Reaction) *model.AppError { } if _, err := a.Srv().Store.Reaction().Delete(reaction); err != nil { - return err + return model.NewAppError("DeleteReactionForPost", "app.reaction.delete_all_with_emoji_name.get_reactions.app_error", nil, err.Error(), http.StatusInternalServerError) } // The post is always modified since the UpdateAt always changes diff --git a/i18n/en.json b/i18n/en.json index cc79c14d8f..e0c02f22ef 100644 --- a/i18n/en.json +++ b/i18n/en.json @@ -3954,6 +3954,22 @@ "id": "app.plugin.write_file.saving.app_error", "translation": "An error occurred while saving the file." }, + { + "id": "app.reaction.bulk_get_for_post_ids.app_error", + "translation": "Unable to get reactions for post." + }, + { + "id": "app.reaction.delete_all_with_emoji_name.get_reactions.app_error", + "translation": "Unable to get all reactions with this emoji name." + }, + { + "id": "app.reaction.get_for_post.app_error", + "translation": "Unable to get reactions for post." + }, + { + "id": "app.reaction.save.save.app_error", + "translation": "Unable to save reaction." + }, { "id": "app.role.check_roles_exist.role_not_found", "translation": "The provided role does not exist" @@ -4310,6 +4326,10 @@ "id": "ent.data_retention.generic.license.error", "translation": "Your license does not support Data Retention." }, + { + "id": "ent.data_retention.reactions_batch.internal_error", + "translation": "We encountered an error permanently deleting the batch of reactions." + }, { "id": "ent.elasticsearch.aggregator_worker.create_index_job.error", "translation": "Elasticsearch aggregator worker failed to create the indexing job" @@ -6906,42 +6926,6 @@ "id": "store.sql_preference.update.app_error", "translation": "Unable to update the preference." }, - { - "id": "store.sql_reaction.bulk_get_for_post_ids.app_error", - "translation": "Unable to get reactions for post." - }, - { - "id": "store.sql_reaction.delete.app_error", - "translation": "Unable to delete reaction." - }, - { - "id": "store.sql_reaction.delete_all_with_emoji_name.delete_reactions.app_error", - "translation": "Unable to delete all reactions with this emoji name." - }, - { - "id": "store.sql_reaction.delete_all_with_emoji_name.get_reactions.app_error", - "translation": "Unable to get all reactions with this emoji name." - }, - { - "id": "store.sql_reaction.get_for_post.app_error", - "translation": "Unable to get reactions for post." - }, - { - "id": "store.sql_reaction.permanent_delete_batch.app_error", - "translation": "We encountered an error permanently deleting the batch of reactions." - }, - { - "id": "store.sql_reaction.save.begin.app_error", - "translation": "Unable to open transaction while saving reaction." - }, - { - "id": "store.sql_reaction.save.commit.app_error", - "translation": "Unable to commit transaction while saving reaction." - }, - { - "id": "store.sql_reaction.save.save.app_error", - "translation": "Unable to save reaction." - }, { "id": "store.sql_recover.delete.app_error", "translation": "Unable to delete token." diff --git a/store/localcachelayer/reaction_layer.go b/store/localcachelayer/reaction_layer.go index d53569005e..6a0f914996 100644 --- a/store/localcachelayer/reaction_layer.go +++ b/store/localcachelayer/reaction_layer.go @@ -21,17 +21,17 @@ func (s *LocalCacheReactionStore) handleClusterInvalidateReaction(msg *model.Clu } } -func (s LocalCacheReactionStore) Save(reaction *model.Reaction) (*model.Reaction, *model.AppError) { +func (s LocalCacheReactionStore) Save(reaction *model.Reaction) (*model.Reaction, error) { defer s.rootStore.doInvalidateCacheCluster(s.rootStore.reactionCache, reaction.PostId) return s.ReactionStore.Save(reaction) } -func (s LocalCacheReactionStore) Delete(reaction *model.Reaction) (*model.Reaction, *model.AppError) { +func (s LocalCacheReactionStore) Delete(reaction *model.Reaction) (*model.Reaction, error) { defer s.rootStore.doInvalidateCacheCluster(s.rootStore.reactionCache, reaction.PostId) return s.ReactionStore.Delete(reaction) } -func (s LocalCacheReactionStore) GetForPost(postId string, allowFromCache bool) ([]*model.Reaction, *model.AppError) { +func (s LocalCacheReactionStore) GetForPost(postId string, allowFromCache bool) ([]*model.Reaction, error) { if !allowFromCache { return s.ReactionStore.GetForPost(postId, false) } @@ -51,7 +51,7 @@ func (s LocalCacheReactionStore) GetForPost(postId string, allowFromCache bool) return reaction, nil } -func (s LocalCacheReactionStore) DeleteAllWithEmojiName(emojiName string) *model.AppError { +func (s LocalCacheReactionStore) DeleteAllWithEmojiName(emojiName string) error { // This could be improved. Right now we just clear the whole // cache because we don't have a way find what post Ids have this emoji name. defer s.rootStore.doClearCacheCluster(s.rootStore.reactionCache) diff --git a/store/opentracing_layer.go b/store/opentracing_layer.go index 02d67e2b3f..c70a6179f1 100644 --- a/store/opentracing_layer.go +++ b/store/opentracing_layer.go @@ -5274,7 +5274,7 @@ func (s *OpenTracingLayerPreferenceStore) Save(preferences *model.Preferences) * return resultVar0 } -func (s *OpenTracingLayerReactionStore) BulkGetForPosts(postIds []string) ([]*model.Reaction, *model.AppError) { +func (s *OpenTracingLayerReactionStore) BulkGetForPosts(postIds []string) ([]*model.Reaction, error) { origCtx := s.Root.Store.Context() span, newCtx := tracing.StartSpanWithParentByContext(s.Root.Store.Context(), "ReactionStore.BulkGetForPosts") s.Root.Store.SetContext(newCtx) @@ -5292,7 +5292,7 @@ func (s *OpenTracingLayerReactionStore) BulkGetForPosts(postIds []string) ([]*mo return resultVar0, resultVar1 } -func (s *OpenTracingLayerReactionStore) Delete(reaction *model.Reaction) (*model.Reaction, *model.AppError) { +func (s *OpenTracingLayerReactionStore) Delete(reaction *model.Reaction) (*model.Reaction, error) { origCtx := s.Root.Store.Context() span, newCtx := tracing.StartSpanWithParentByContext(s.Root.Store.Context(), "ReactionStore.Delete") s.Root.Store.SetContext(newCtx) @@ -5310,7 +5310,7 @@ func (s *OpenTracingLayerReactionStore) Delete(reaction *model.Reaction) (*model return resultVar0, resultVar1 } -func (s *OpenTracingLayerReactionStore) DeleteAllWithEmojiName(emojiName string) *model.AppError { +func (s *OpenTracingLayerReactionStore) DeleteAllWithEmojiName(emojiName string) error { origCtx := s.Root.Store.Context() span, newCtx := tracing.StartSpanWithParentByContext(s.Root.Store.Context(), "ReactionStore.DeleteAllWithEmojiName") s.Root.Store.SetContext(newCtx) @@ -5328,7 +5328,7 @@ func (s *OpenTracingLayerReactionStore) DeleteAllWithEmojiName(emojiName string) return resultVar0 } -func (s *OpenTracingLayerReactionStore) GetForPost(postId string, allowFromCache bool) ([]*model.Reaction, *model.AppError) { +func (s *OpenTracingLayerReactionStore) GetForPost(postId string, allowFromCache bool) ([]*model.Reaction, error) { origCtx := s.Root.Store.Context() span, newCtx := tracing.StartSpanWithParentByContext(s.Root.Store.Context(), "ReactionStore.GetForPost") s.Root.Store.SetContext(newCtx) @@ -5346,7 +5346,7 @@ func (s *OpenTracingLayerReactionStore) GetForPost(postId string, allowFromCache return resultVar0, resultVar1 } -func (s *OpenTracingLayerReactionStore) PermanentDeleteBatch(endTime int64, limit int64) (int64, *model.AppError) { +func (s *OpenTracingLayerReactionStore) PermanentDeleteBatch(endTime int64, limit int64) (int64, error) { origCtx := s.Root.Store.Context() span, newCtx := tracing.StartSpanWithParentByContext(s.Root.Store.Context(), "ReactionStore.PermanentDeleteBatch") s.Root.Store.SetContext(newCtx) @@ -5364,7 +5364,7 @@ func (s *OpenTracingLayerReactionStore) PermanentDeleteBatch(endTime int64, limi return resultVar0, resultVar1 } -func (s *OpenTracingLayerReactionStore) Save(reaction *model.Reaction) (*model.Reaction, *model.AppError) { +func (s *OpenTracingLayerReactionStore) Save(reaction *model.Reaction) (*model.Reaction, error) { origCtx := s.Root.Store.Context() span, newCtx := tracing.StartSpanWithParentByContext(s.Root.Store.Context(), "ReactionStore.Save") s.Root.Store.SetContext(newCtx) diff --git a/store/sqlstore/supplier_reactions.go b/store/sqlstore/supplier_reactions.go index f0f76f5bf0..40843a2aae 100644 --- a/store/sqlstore/supplier_reactions.go +++ b/store/sqlstore/supplier_reactions.go @@ -4,8 +4,6 @@ package sqlstore import ( - "net/http" - "github.com/mattermost/mattermost-server/v5/mlog" "github.com/mattermost/mattermost-server/v5/model" "github.com/mattermost/mattermost-server/v5/store" @@ -31,7 +29,7 @@ func newSqlReactionStore(sqlStore SqlStore) store.ReactionStore { return s } -func (s *SqlReactionStore) Save(reaction *model.Reaction) (*model.Reaction, *model.AppError) { +func (s *SqlReactionStore) Save(reaction *model.Reaction) (*model.Reaction, error) { reaction.PreSave() if err := reaction.IsValid(); err != nil { return nil, err @@ -39,25 +37,25 @@ func (s *SqlReactionStore) Save(reaction *model.Reaction) (*model.Reaction, *mod transaction, err := s.GetMaster().Begin() if err != nil { - return nil, model.NewAppError("SqlReactionStore.Save", "store.sql_reaction.save.begin.app_error", nil, err.Error(), http.StatusInternalServerError) + return nil, errors.Wrap(err, "begin_transaction") } defer finalizeTransaction(transaction) - appErr := saveReactionAndUpdatePost(transaction, reaction) - if appErr != nil { + err = saveReactionAndUpdatePost(transaction, reaction) + if err != nil { // We don't consider duplicated save calls as an error - if !IsUniqueConstraintError(appErr, []string{"reactions_pkey", "PRIMARY"}) { - return nil, model.NewAppError("SqlPreferenceStore.Save", "store.sql_reaction.save.save.app_error", nil, appErr.Error(), http.StatusBadRequest) + if !IsUniqueConstraintError(err, []string{"reactions_pkey", "PRIMARY"}) { + return nil, errors.Wrap(err, "failed while saving reaction or updating post") } } else { if err := transaction.Commit(); err != nil { - return nil, model.NewAppError("SqlPreferenceStore.Save", "store.sql_reaction.save.commit.app_error", nil, err.Error(), http.StatusInternalServerError) + return nil, errors.Wrap(err, "commit_transaction") } } return reaction, nil } -func (s *SqlReactionStore) Delete(reaction *model.Reaction) (*model.Reaction, *model.AppError) { +func (s *SqlReactionStore) Delete(reaction *model.Reaction) (*model.Reaction, error) { err := store.WithDeadlockRetry(func() error { transaction, err := s.GetMaster().Begin() if err != nil { @@ -75,13 +73,13 @@ func (s *SqlReactionStore) Delete(reaction *model.Reaction) (*model.Reaction, *m return nil }) if err != nil { - return nil, model.NewAppError("SqlReactionStore.Delete", "store.sql_reaction.delete.app_error", nil, err.Error(), http.StatusInternalServerError) + return nil, errors.Wrap(err, "failed to delete reaction") } return reaction, nil } -func (s *SqlReactionStore) GetForPost(postId string, allowFromCache bool) ([]*model.Reaction, *model.AppError) { +func (s *SqlReactionStore) GetForPost(postId string, allowFromCache bool) ([]*model.Reaction, error) { var reactions []*model.Reaction if _, err := s.GetReplica().Select(&reactions, @@ -93,13 +91,13 @@ func (s *SqlReactionStore) GetForPost(postId string, allowFromCache bool) ([]*mo PostId = :PostId ORDER BY CreateAt`, map[string]interface{}{"PostId": postId}); err != nil { - return nil, model.NewAppError("SqlReactionStore.GetForPost", "store.sql_reaction.get_for_post.app_error", nil, "", http.StatusInternalServerError) + return nil, errors.Wrapf(err, "failed to get Reactions with postId=%s", postId) } return reactions, nil } -func (s *SqlReactionStore) BulkGetForPosts(postIds []string) ([]*model.Reaction, *model.AppError) { +func (s *SqlReactionStore) BulkGetForPosts(postIds []string) ([]*model.Reaction, error) { keys, params := MapStringsToQueryParams(postIds, "postId") var reactions []*model.Reaction @@ -111,12 +109,12 @@ func (s *SqlReactionStore) BulkGetForPosts(postIds []string) ([]*model.Reaction, PostId IN `+keys+` ORDER BY CreateAt`, params); err != nil { - return nil, model.NewAppError("SqlReactionStore.GetForPost", "store.sql_reaction.bulk_get_for_post_ids.app_error", nil, "", http.StatusInternalServerError) + return nil, errors.Wrap(err, "failed to get Reactions") } return reactions, nil } -func (s *SqlReactionStore) DeleteAllWithEmojiName(emojiName string) *model.AppError { +func (s *SqlReactionStore) DeleteAllWithEmojiName(emojiName string) error { var reactions []*model.Reaction if _, err := s.GetReplica().Select(&reactions, @@ -126,9 +124,7 @@ func (s *SqlReactionStore) DeleteAllWithEmojiName(emojiName string) *model.AppEr Reactions WHERE EmojiName = :EmojiName`, map[string]interface{}{"EmojiName": emojiName}); err != nil { - return model.NewAppError("SqlReactionStore.DeleteAllWithEmojiName", - "store.sql_reaction.delete_all_with_emoji_name.get_reactions.app_error", nil, - "emoji_name="+emojiName+", error="+err.Error(), http.StatusInternalServerError) + return errors.Wrapf(err, "failed to get Reactions with emojiName=%s", emojiName) } err := store.WithDeadlockRetry(func() error { @@ -140,9 +136,7 @@ func (s *SqlReactionStore) DeleteAllWithEmojiName(emojiName string) *model.AppEr return err }) if err != nil { - return model.NewAppError("SqlReactionStore.DeleteAllWithEmojiName", - "store.sql_reaction.delete_all_with_emoji_name.delete_reactions.app_error", nil, - "emoji_name="+emojiName+", error="+err.Error(), http.StatusInternalServerError) + return errors.Wrapf(err, "failed to delete Reactions with emojiName=%s", emojiName) } for _, reaction := range reactions { @@ -165,7 +159,7 @@ func (s *SqlReactionStore) DeleteAllWithEmojiName(emojiName string) *model.AppEr return nil } -func (s *SqlReactionStore) PermanentDeleteBatch(endTime int64, limit int64) (int64, *model.AppError) { +func (s *SqlReactionStore) PermanentDeleteBatch(endTime int64, limit int64) (int64, error) { var query string if s.DriverName() == "postgres" { query = "DELETE from Reactions WHERE CreateAt = any (array (SELECT CreateAt FROM Reactions WHERE CreateAt < :EndTime LIMIT :Limit))" @@ -175,12 +169,12 @@ func (s *SqlReactionStore) PermanentDeleteBatch(endTime int64, limit int64) (int sqlResult, err := s.GetMaster().Exec(query, map[string]interface{}{"EndTime": endTime, "Limit": limit}) if err != nil { - return 0, model.NewAppError("SqlReactionStore.PermanentDeleteBatch", "store.sql_reaction.permanent_delete_batch.app_error", nil, ""+err.Error(), http.StatusInternalServerError) + return 0, errors.Wrap(err, "failed to delete Reactions") } rowsAffected, err := sqlResult.RowsAffected() if err != nil { - return 0, model.NewAppError("SqlReactionStore.PermanentDeleteBatch", "store.sql_reaction.permanent_delete_batch.app_error", nil, ""+err.Error(), http.StatusInternalServerError) + return 0, errors.Wrap(err, "unable to get rows affected for deleted Reactions") } return rowsAffected, nil } diff --git a/store/store.go b/store/store.go index a1fd293add..92644d603d 100644 --- a/store/store.go +++ b/store/store.go @@ -541,12 +541,12 @@ type FileInfoStore interface { } type ReactionStore interface { - Save(reaction *model.Reaction) (*model.Reaction, *model.AppError) - Delete(reaction *model.Reaction) (*model.Reaction, *model.AppError) - GetForPost(postId string, allowFromCache bool) ([]*model.Reaction, *model.AppError) - DeleteAllWithEmojiName(emojiName string) *model.AppError - PermanentDeleteBatch(endTime int64, limit int64) (int64, *model.AppError) - BulkGetForPosts(postIds []string) ([]*model.Reaction, *model.AppError) + Save(reaction *model.Reaction) (*model.Reaction, error) + Delete(reaction *model.Reaction) (*model.Reaction, error) + GetForPost(postId string, allowFromCache bool) ([]*model.Reaction, error) + DeleteAllWithEmojiName(emojiName string) error + PermanentDeleteBatch(endTime int64, limit int64) (int64, error) + BulkGetForPosts(postIds []string) ([]*model.Reaction, error) } type JobStore interface { diff --git a/store/storetest/mocks/ReactionStore.go b/store/storetest/mocks/ReactionStore.go index 66c9d1e02b..f27627665d 100644 --- a/store/storetest/mocks/ReactionStore.go +++ b/store/storetest/mocks/ReactionStore.go @@ -15,7 +15,7 @@ type ReactionStore struct { } // BulkGetForPosts provides a mock function with given fields: postIds -func (_m *ReactionStore) BulkGetForPosts(postIds []string) ([]*model.Reaction, *model.AppError) { +func (_m *ReactionStore) BulkGetForPosts(postIds []string) ([]*model.Reaction, error) { ret := _m.Called(postIds) var r0 []*model.Reaction @@ -27,20 +27,18 @@ func (_m *ReactionStore) BulkGetForPosts(postIds []string) ([]*model.Reaction, * } } - var r1 *model.AppError - if rf, ok := ret.Get(1).(func([]string) *model.AppError); ok { + var r1 error + if rf, ok := ret.Get(1).(func([]string) error); ok { r1 = rf(postIds) } else { - if ret.Get(1) != nil { - r1 = ret.Get(1).(*model.AppError) - } + r1 = ret.Error(1) } return r0, r1 } // Delete provides a mock function with given fields: reaction -func (_m *ReactionStore) Delete(reaction *model.Reaction) (*model.Reaction, *model.AppError) { +func (_m *ReactionStore) Delete(reaction *model.Reaction) (*model.Reaction, error) { ret := _m.Called(reaction) var r0 *model.Reaction @@ -52,36 +50,32 @@ func (_m *ReactionStore) Delete(reaction *model.Reaction) (*model.Reaction, *mod } } - var r1 *model.AppError - if rf, ok := ret.Get(1).(func(*model.Reaction) *model.AppError); ok { + var r1 error + if rf, ok := ret.Get(1).(func(*model.Reaction) error); ok { r1 = rf(reaction) } else { - if ret.Get(1) != nil { - r1 = ret.Get(1).(*model.AppError) - } + r1 = ret.Error(1) } return r0, r1 } // DeleteAllWithEmojiName provides a mock function with given fields: emojiName -func (_m *ReactionStore) DeleteAllWithEmojiName(emojiName string) *model.AppError { +func (_m *ReactionStore) DeleteAllWithEmojiName(emojiName string) error { ret := _m.Called(emojiName) - var r0 *model.AppError - if rf, ok := ret.Get(0).(func(string) *model.AppError); ok { + var r0 error + if rf, ok := ret.Get(0).(func(string) error); ok { r0 = rf(emojiName) } else { - if ret.Get(0) != nil { - r0 = ret.Get(0).(*model.AppError) - } + r0 = ret.Error(0) } return r0 } // GetForPost provides a mock function with given fields: postId, allowFromCache -func (_m *ReactionStore) GetForPost(postId string, allowFromCache bool) ([]*model.Reaction, *model.AppError) { +func (_m *ReactionStore) GetForPost(postId string, allowFromCache bool) ([]*model.Reaction, error) { ret := _m.Called(postId, allowFromCache) var r0 []*model.Reaction @@ -93,20 +87,18 @@ func (_m *ReactionStore) GetForPost(postId string, allowFromCache bool) ([]*mode } } - var r1 *model.AppError - if rf, ok := ret.Get(1).(func(string, bool) *model.AppError); ok { + var r1 error + if rf, ok := ret.Get(1).(func(string, bool) error); ok { r1 = rf(postId, allowFromCache) } else { - if ret.Get(1) != nil { - r1 = ret.Get(1).(*model.AppError) - } + r1 = ret.Error(1) } return r0, r1 } // PermanentDeleteBatch provides a mock function with given fields: endTime, limit -func (_m *ReactionStore) PermanentDeleteBatch(endTime int64, limit int64) (int64, *model.AppError) { +func (_m *ReactionStore) PermanentDeleteBatch(endTime int64, limit int64) (int64, error) { ret := _m.Called(endTime, limit) var r0 int64 @@ -116,20 +108,18 @@ func (_m *ReactionStore) PermanentDeleteBatch(endTime int64, limit int64) (int64 r0 = ret.Get(0).(int64) } - var r1 *model.AppError - if rf, ok := ret.Get(1).(func(int64, int64) *model.AppError); ok { + var r1 error + if rf, ok := ret.Get(1).(func(int64, int64) error); ok { r1 = rf(endTime, limit) } else { - if ret.Get(1) != nil { - r1 = ret.Get(1).(*model.AppError) - } + r1 = ret.Error(1) } return r0, r1 } // Save provides a mock function with given fields: reaction -func (_m *ReactionStore) Save(reaction *model.Reaction) (*model.Reaction, *model.AppError) { +func (_m *ReactionStore) Save(reaction *model.Reaction) (*model.Reaction, error) { ret := _m.Called(reaction) var r0 *model.Reaction @@ -141,13 +131,11 @@ func (_m *ReactionStore) Save(reaction *model.Reaction) (*model.Reaction, *model } } - var r1 *model.AppError - if rf, ok := ret.Get(1).(func(*model.Reaction) *model.AppError); ok { + var r1 error + if rf, ok := ret.Get(1).(func(*model.Reaction) error); ok { r1 = rf(reaction) } else { - if ret.Get(1) != nil { - r1 = ret.Get(1).(*model.AppError) - } + r1 = ret.Error(1) } return r0, r1 diff --git a/store/storetest/reaction_store.go b/store/storetest/reaction_store.go index cbc72dc2d6..a3f5e4d681 100644 --- a/store/storetest/reaction_store.go +++ b/store/storetest/reaction_store.go @@ -37,8 +37,8 @@ func testReactionSave(t *testing.T, ss store.Store) { PostId: post.Id, EmojiName: model.NewId(), } - reaction, err := ss.Reaction().Save(reaction1) - require.Nil(t, err) + reaction, nErr := ss.Reaction().Save(reaction1) + require.Nil(t, nErr) saved := reaction assert.Equal(t, saved.UserId, reaction1.UserId, "should've saved reaction user_id and returned it") @@ -56,8 +56,8 @@ func testReactionSave(t *testing.T, ss store.Store) { secondUpdateAt = postList.Posts[post.Id].UpdateAt } - _, err = ss.Reaction().Save(reaction1) - assert.Nil(t, err, "should've allowed saving a duplicate reaction") + _, nErr = ss.Reaction().Save(reaction1) + assert.Nil(t, nErr, "should've allowed saving a duplicate reaction") // different user reaction2 := &model.Reaction{ @@ -65,8 +65,8 @@ func testReactionSave(t *testing.T, ss store.Store) { PostId: reaction1.PostId, EmojiName: reaction1.EmojiName, } - _, err = ss.Reaction().Save(reaction2) - require.Nil(t, err) + _, nErr = ss.Reaction().Save(reaction2) + require.Nil(t, nErr) postList, err = ss.Post().Get(reaction2.PostId, false) require.Nil(t, err) @@ -79,8 +79,8 @@ func testReactionSave(t *testing.T, ss store.Store) { PostId: model.NewId(), EmojiName: reaction1.EmojiName, } - _, err = ss.Reaction().Save(reaction3) - require.Nil(t, err) + _, nErr = ss.Reaction().Save(reaction3) + require.Nil(t, nErr) // different emoji reaction4 := &model.Reaction{ @@ -88,16 +88,17 @@ func testReactionSave(t *testing.T, ss store.Store) { PostId: reaction1.PostId, EmojiName: model.NewId(), } - _, err = ss.Reaction().Save(reaction4) - require.Nil(t, err) + _, nErr = ss.Reaction().Save(reaction4) + require.Nil(t, nErr) // invalid reaction reaction5 := &model.Reaction{ UserId: reaction1.UserId, PostId: reaction1.PostId, } - _, err = ss.Reaction().Save(reaction5) - require.NotNil(t, err, "should've failed for invalid reaction") + _, nErr = ss.Reaction().Save(reaction5) + require.NotNil(t, nErr, "should've failed for invalid reaction") + } func testReactionDelete(t *testing.T, ss store.Store) { @@ -113,16 +114,16 @@ func testReactionDelete(t *testing.T, ss store.Store) { EmojiName: model.NewId(), } - _, err = ss.Reaction().Save(reaction) - require.Nil(t, err) + _, nErr := ss.Reaction().Save(reaction) + require.Nil(t, nErr) result, err := ss.Post().Get(reaction.PostId, false) require.Nil(t, err) firstUpdateAt := result.Posts[post.Id].UpdateAt - _, err = ss.Reaction().Delete(reaction) - require.Nil(t, err) + _, nErr = ss.Reaction().Delete(reaction) + require.Nil(t, nErr) reactions, rErr := ss.Reaction().GetForPost(post.Id, false) require.Nil(t, rErr) @@ -341,9 +342,9 @@ func testReactionStorePermanentDeleteBatch(t *testing.T, ss store.Store) { // Need to hang on to a reaction to delete later in order to clear the cache, as "allowFromCache" isn't honoured any more. var lastReaction *model.Reaction for _, reaction := range reactions { - var err *model.AppError - lastReaction, err = ss.Reaction().Save(reaction) - require.Nil(t, err) + var nErr error + lastReaction, nErr = ss.Reaction().Save(reaction) + require.Nil(t, nErr) } returned, err := ss.Reaction().GetForPost(post.Id, false) @@ -439,8 +440,8 @@ func testReactionDeadlock(t *testing.T, ss store.Store) { PostId: post.Id, EmojiName: model.NewId(), } - _, err = ss.Reaction().Save(reaction1) - require.Nil(t, err) + _, nErr := ss.Reaction().Save(reaction1) + require.Nil(t, nErr) // different user reaction2 := &model.Reaction{ @@ -448,8 +449,8 @@ func testReactionDeadlock(t *testing.T, ss store.Store) { PostId: reaction1.PostId, EmojiName: reaction1.EmojiName, } - _, err = ss.Reaction().Save(reaction2) - require.Nil(t, err) + _, nErr = ss.Reaction().Save(reaction2) + require.Nil(t, nErr) // different post reaction3 := &model.Reaction{ @@ -457,8 +458,8 @@ func testReactionDeadlock(t *testing.T, ss store.Store) { PostId: model.NewId(), EmojiName: reaction1.EmojiName, } - _, err = ss.Reaction().Save(reaction3) - require.Nil(t, err) + _, nErr = ss.Reaction().Save(reaction3) + require.Nil(t, nErr) // different emoji reaction4 := &model.Reaction{ @@ -466,8 +467,8 @@ func testReactionDeadlock(t *testing.T, ss store.Store) { PostId: reaction1.PostId, EmojiName: model.NewId(), } - _, err = ss.Reaction().Save(reaction4) - require.Nil(t, err) + _, nErr = ss.Reaction().Save(reaction4) + require.Nil(t, nErr) var wg sync.WaitGroup wg.Add(2) diff --git a/store/timer_layer.go b/store/timer_layer.go index cca5afd016..ca5814e7b6 100644 --- a/store/timer_layer.go +++ b/store/timer_layer.go @@ -4780,7 +4780,7 @@ func (s *TimerLayerPreferenceStore) Save(preferences *model.Preferences) *model. return resultVar0 } -func (s *TimerLayerReactionStore) BulkGetForPosts(postIds []string) ([]*model.Reaction, *model.AppError) { +func (s *TimerLayerReactionStore) BulkGetForPosts(postIds []string) ([]*model.Reaction, error) { start := timemodule.Now() resultVar0, resultVar1 := s.ReactionStore.BulkGetForPosts(postIds) @@ -4796,7 +4796,7 @@ func (s *TimerLayerReactionStore) BulkGetForPosts(postIds []string) ([]*model.Re return resultVar0, resultVar1 } -func (s *TimerLayerReactionStore) Delete(reaction *model.Reaction) (*model.Reaction, *model.AppError) { +func (s *TimerLayerReactionStore) Delete(reaction *model.Reaction) (*model.Reaction, error) { start := timemodule.Now() resultVar0, resultVar1 := s.ReactionStore.Delete(reaction) @@ -4812,7 +4812,7 @@ func (s *TimerLayerReactionStore) Delete(reaction *model.Reaction) (*model.React return resultVar0, resultVar1 } -func (s *TimerLayerReactionStore) DeleteAllWithEmojiName(emojiName string) *model.AppError { +func (s *TimerLayerReactionStore) DeleteAllWithEmojiName(emojiName string) error { start := timemodule.Now() resultVar0 := s.ReactionStore.DeleteAllWithEmojiName(emojiName) @@ -4828,7 +4828,7 @@ func (s *TimerLayerReactionStore) DeleteAllWithEmojiName(emojiName string) *mode return resultVar0 } -func (s *TimerLayerReactionStore) GetForPost(postId string, allowFromCache bool) ([]*model.Reaction, *model.AppError) { +func (s *TimerLayerReactionStore) GetForPost(postId string, allowFromCache bool) ([]*model.Reaction, error) { start := timemodule.Now() resultVar0, resultVar1 := s.ReactionStore.GetForPost(postId, allowFromCache) @@ -4844,7 +4844,7 @@ func (s *TimerLayerReactionStore) GetForPost(postId string, allowFromCache bool) return resultVar0, resultVar1 } -func (s *TimerLayerReactionStore) PermanentDeleteBatch(endTime int64, limit int64) (int64, *model.AppError) { +func (s *TimerLayerReactionStore) PermanentDeleteBatch(endTime int64, limit int64) (int64, error) { start := timemodule.Now() resultVar0, resultVar1 := s.ReactionStore.PermanentDeleteBatch(endTime, limit) @@ -4860,7 +4860,7 @@ func (s *TimerLayerReactionStore) PermanentDeleteBatch(endTime int64, limit int6 return resultVar0, resultVar1 } -func (s *TimerLayerReactionStore) Save(reaction *model.Reaction) (*model.Reaction, *model.AppError) { +func (s *TimerLayerReactionStore) Save(reaction *model.Reaction) (*model.Reaction, error) { start := timemodule.Now() resultVar0, resultVar1 := s.ReactionStore.Save(reaction)