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 <villa061004@gmail.com>
Co-authored-by: Mattermod <mattermod@users.noreply.github.com>

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)
+	reactions, nErr := a.Srv().Store.Reaction().GetForPost(postId, true)
-	if err != nil {
+	if nErr != nil {
-		return nil, err
+		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))

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 {
+	if _, nErr := a.Srv().Store.Reaction().Save(reaction); nErr != nil {
-		return err
+		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

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)
+	reactions, nErr := th.App.Srv().Store.Reaction().GetForPost(post.Id, false)
-	require.Nil(t, err, "Can't get reaction")
+	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)
+		reactions, nErr := th.App.Srv().Store.Reaction().GetForPost(post.Id, false)
-		require.Nil(t, err, "Can't get reaction")
+		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)
+		reactions, nErr := th.App.Srv().Store.Reaction().GetForPost(post.Id, false)
-		require.Nil(t, err, "Can't get reaction")
+		require.Nil(t, nErr, "Can't get reaction")
 
 		require.Len(t, reactions, 1, "Invalid number of reactions")
 	})

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)
+	reaction, nErr := a.Srv().Store.Reaction().Save(reaction)
-	if err != nil {
+	if nErr != nil {
-		return nil, err
+		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

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."

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)

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)

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)
+	err = saveReactionAndUpdatePost(transaction, reaction)
-	if appErr != nil {
+	if err != nil {
 		// We don't consider duplicated save calls as an error
-		if !IsUniqueConstraintError(appErr, []string{"reactions_pkey", "PRIMARY"}) {
+		if !IsUniqueConstraintError(err, []string{"reactions_pkey", "PRIMARY"}) {
-			return nil, model.NewAppError("SqlPreferenceStore.Save", "store.sql_reaction.save.save.app_error", nil, appErr.Error(), http.StatusBadRequest)
+			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",
+		return errors.Wrapf(err, "failed to get Reactions with emojiName=%s", emojiName)
-			"store.sql_reaction.delete_all_with_emoji_name.get_reactions.app_error", nil,
-			"emoji_name="+emojiName+", error="+err.Error(), http.StatusInternalServerError)
 	}
 
 	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",
+		return errors.Wrapf(err, "failed to delete Reactions with emojiName=%s", emojiName)
-			"store.sql_reaction.delete_all_with_emoji_name.delete_reactions.app_error", nil,
-			"emoji_name="+emojiName+", error="+err.Error(), http.StatusInternalServerError)
 	}
 
 	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
 }

store/store.go

@@ -541,12 +541,12 @@ type FileInfoStore interface {
 }
 
 type ReactionStore interface {
-	Save(reaction *model.Reaction) (*model.Reaction, *model.AppError)
+	Save(reaction *model.Reaction) (*model.Reaction, error)
-	Delete(reaction *model.Reaction) (*model.Reaction, *model.AppError)
+	Delete(reaction *model.Reaction) (*model.Reaction, error)
-	GetForPost(postId string, allowFromCache bool) ([]*model.Reaction, *model.AppError)
+	GetForPost(postId string, allowFromCache bool) ([]*model.Reaction, error)
-	DeleteAllWithEmojiName(emojiName string) *model.AppError
+	DeleteAllWithEmojiName(emojiName string) error
-	PermanentDeleteBatch(endTime int64, limit int64) (int64, *model.AppError)
+	PermanentDeleteBatch(endTime int64, limit int64) (int64, error)
-	BulkGetForPosts(postIds []string) ([]*model.Reaction, *model.AppError)
+	BulkGetForPosts(postIds []string) ([]*model.Reaction, error)
 }
 
 type JobStore interface {

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
+	var r1 error
-	if rf, ok := ret.Get(1).(func([]string) *model.AppError); ok {
+	if rf, ok := ret.Get(1).(func([]string) error); ok {
 		r1 = rf(postIds)
 	} else {
-		if ret.Get(1) != nil {
+		r1 = ret.Error(1)
-			r1 = ret.Get(1).(*model.AppError)
-		}
 	}
 
 	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
+	var r1 error
-	if rf, ok := ret.Get(1).(func(*model.Reaction) *model.AppError); ok {
+	if rf, ok := ret.Get(1).(func(*model.Reaction) error); ok {
 		r1 = rf(reaction)
 	} else {
-		if ret.Get(1) != nil {
+		r1 = ret.Error(1)
-			r1 = ret.Get(1).(*model.AppError)
-		}
 	}
 
 	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
+	var r0 error
-	if rf, ok := ret.Get(0).(func(string) *model.AppError); ok {
+	if rf, ok := ret.Get(0).(func(string) error); ok {
 		r0 = rf(emojiName)
 	} else {
-		if ret.Get(0) != nil {
+		r0 = ret.Error(0)
-			r0 = ret.Get(0).(*model.AppError)
-		}
 	}
 
 	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
+	var r1 error
-	if rf, ok := ret.Get(1).(func(string, bool) *model.AppError); ok {
+	if rf, ok := ret.Get(1).(func(string, bool) error); ok {
 		r1 = rf(postId, allowFromCache)
 	} else {
-		if ret.Get(1) != nil {
+		r1 = ret.Error(1)
-			r1 = ret.Get(1).(*model.AppError)
-		}
 	}
 
 	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
+	var r1 error
-	if rf, ok := ret.Get(1).(func(int64, int64) *model.AppError); ok {
+	if rf, ok := ret.Get(1).(func(int64, int64) error); ok {
 		r1 = rf(endTime, limit)
 	} else {
-		if ret.Get(1) != nil {
+		r1 = ret.Error(1)
-			r1 = ret.Get(1).(*model.AppError)
-		}
 	}
 
 	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
+	var r1 error
-	if rf, ok := ret.Get(1).(func(*model.Reaction) *model.AppError); ok {
+	if rf, ok := ret.Get(1).(func(*model.Reaction) error); ok {
 		r1 = rf(reaction)
 	} else {
-		if ret.Get(1) != nil {
+		r1 = ret.Error(1)
-			r1 = ret.Get(1).(*model.AppError)
-		}
 	}
 
 	return r0, r1

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)
+	reaction, nErr := ss.Reaction().Save(reaction1)
-	require.Nil(t, err)
+	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)
+	_, nErr = ss.Reaction().Save(reaction1)
-	assert.Nil(t, err, "should've allowed saving a duplicate reaction")
+	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)
+	_, nErr = ss.Reaction().Save(reaction2)
-	require.Nil(t, err)
+	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)
+	_, nErr = ss.Reaction().Save(reaction3)
-	require.Nil(t, err)
+	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)
+	_, nErr = ss.Reaction().Save(reaction4)
-	require.Nil(t, err)
+	require.Nil(t, nErr)
 
 	// invalid reaction
 	reaction5 := &model.Reaction{
 		UserId: reaction1.UserId,
 		PostId: reaction1.PostId,
 	}
-	_, err = ss.Reaction().Save(reaction5)
+	_, nErr = ss.Reaction().Save(reaction5)
-	require.NotNil(t, err, "should've failed for invalid reaction")
+	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)
+	_, nErr := ss.Reaction().Save(reaction)
-	require.Nil(t, err)
+	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)
+	_, nErr = ss.Reaction().Delete(reaction)
-	require.Nil(t, err)
+	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
+		var nErr error
-		lastReaction, err = ss.Reaction().Save(reaction)
+		lastReaction, nErr = ss.Reaction().Save(reaction)
-		require.Nil(t, err)
+		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)
+	_, nErr := ss.Reaction().Save(reaction1)
-	require.Nil(t, err)
+	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)
+	_, nErr = ss.Reaction().Save(reaction2)
-	require.Nil(t, err)
+	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)
+	_, nErr = ss.Reaction().Save(reaction3)
-	require.Nil(t, err)
+	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)
+	_, nErr = ss.Reaction().Save(reaction4)
-	require.Nil(t, err)
+	require.Nil(t, nErr)
 
 	var wg sync.WaitGroup
 	wg.Add(2)

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)