MM-31875 soft delete reactions (#16654)

- soft delete reaction by setting new field Reactions.DeleteAt to non-zero. - include new field Reactions.UpdateAt
2021-01-20 10:09:23 -05:00
--- a/store/localcachelayer/channel_layer_test.go
+++ b/store/localcachelayer/channel_layer_test.go
@@ -15,7 +15,7 @@ import (
 )

 func TestChannelStore(t *testing.T) {
-	StoreTest(t, storetest.TestReactionStore)
+	StoreTestWithSqlStore(t, storetest.TestReactionStore)
 }

 func TestChannelStoreChannelMemberCountsCache(t *testing.T) {
--- a/store/localcachelayer/reaction_layer_test.go
+++ b/store/localcachelayer/reaction_layer_test.go
@@ -15,7 +15,7 @@ import (
 )

 func TestReactionStore(t *testing.T) {
-	StoreTest(t, storetest.TestReactionStore)
+	StoreTestWithSqlStore(t, storetest.TestReactionStore)
 }

 func TestReactionStoreCache(t *testing.T) {
--- a/store/sqlstore/reaction_store.go
+++ b/store/sqlstore/reaction_store.go
@@ -4,12 +4,12 @@
 package sqlstore

 import (
-	"github.com/mattermost/gorp"
-	"github.com/pkg/errors"
-
 	"github.com/mattermost/mattermost-server/v5/mlog"
 	"github.com/mattermost/mattermost-server/v5/model"
 	"github.com/mattermost/mattermost-server/v5/store"
+
+	"github.com/mattermost/gorp"
+	"github.com/pkg/errors"
 )

 type SqlReactionStore struct {
@@ -40,7 +40,7 @@ func (s *SqlReactionStore) Save(reaction *model.Reaction) (*model.Reaction, erro
 		return nil, errors.Wrap(err, "begin_transaction")
 	}
 	defer finalizeTransaction(transaction)
-	err = saveReactionAndUpdatePost(transaction, reaction)
+	err = s.saveReactionAndUpdatePost(transaction, reaction)
 	if err != nil {
 		// We don't consider duplicated save calls as an error
 		if !IsUniqueConstraintError(err, []string{"reactions_pkey", "PRIMARY"}) {
@@ -56,6 +56,8 @@ func (s *SqlReactionStore) Save(reaction *model.Reaction) (*model.Reaction, erro
 }

 func (s *SqlReactionStore) Delete(reaction *model.Reaction) (*model.Reaction, error) {
+	reaction.PreUpdate()
+
 	transaction, err := s.GetMaster().Begin()
 	if err != nil {
 		return nil, errors.Wrap(err, "begin_transaction")
@@ -78,11 +80,16 @@ func (s *SqlReactionStore) GetForPost(postId string, allowFromCache bool) ([]*mo

 	if _, err := s.GetReplica().Select(&reactions,
 		`SELECT
-				*
+				UserId,
+				PostId,
+				EmojiName,
+				CreateAt,
+				COALESCE(UpdateAt, CreateAt) As UpdateAt,
+				COALESCE(DeleteAt, 0) As DeleteAt
 			FROM
 				Reactions
 			WHERE
-				PostId = :PostId
+				PostId = :PostId AND COALESCE(DeleteAt, 0) = 0
 			ORDER BY
 				CreateAt`, map[string]interface{}{"PostId": postId}); err != nil {
 		return nil, errors.Wrapf(err, "failed to get Reactions with postId=%s", postId)
@@ -95,12 +102,18 @@ func (s *SqlReactionStore) BulkGetForPosts(postIds []string) ([]*model.Reaction,
 	keys, params := MapStringsToQueryParams(postIds, "postId")
 	var reactions []*model.Reaction

-	if _, err := s.GetReplica().Select(&reactions, `SELECT
-				*
+	if _, err := s.GetReplica().Select(&reactions,
+		`SELECT
+				UserId,
+				PostId,
+				EmojiName,
+				CreateAt,
+				COALESCE(UpdateAt, CreateAt) As UpdateAt,
+				COALESCE(DeleteAt, 0) As DeleteAt
 			FROM
 				Reactions
 			WHERE
-				PostId IN `+keys+`
+				PostId IN `+keys+` AND COALESCE(DeleteAt, 0) = 0
 			ORDER BY
 				CreateAt`, params); err != nil {
 		return nil, errors.Wrap(err, "failed to get Reactions")
@@ -110,22 +123,36 @@ func (s *SqlReactionStore) BulkGetForPosts(postIds []string) ([]*model.Reaction,

 func (s *SqlReactionStore) DeleteAllWithEmojiName(emojiName string) error {
 	var reactions []*model.Reaction
+	now := model.GetMillis()
+
+	params := map[string]interface{}{
+		"EmojiName": emojiName,
+		"UpdateAt":  now,
+		"DeleteAt":  now,
+	}

 	if _, err := s.GetReplica().Select(&reactions,
 		`SELECT
-				*
-			FROM
-				Reactions
-			WHERE
-				EmojiName = :EmojiName`, map[string]interface{}{"EmojiName": emojiName}); err != nil {
+					UserId,
+					PostId,
+					EmojiName,
+					CreateAt,
+					COALESCE(UpdateAt, CreateAt) As UpdateAt,
+					COALESCE(DeleteAt, 0) As DeleteAt
+				FROM
+					Reactions
+				WHERE
+					EmojiName = :EmojiName AND COALESCE(DeleteAt, 0) = 0`, params); err != nil {
 		return errors.Wrapf(err, "failed to get Reactions with emojiName=%s", emojiName)
 	}

 	_, err := s.GetMaster().Exec(
-		`DELETE FROM
+		`UPDATE
 			Reactions
+		SET
+			UpdateAt = :UpdateAt, DeleteAt = :DeleteAt
 		WHERE
-			EmojiName = :EmojiName`, map[string]interface{}{"EmojiName": emojiName})
+			EmojiName = :EmojiName AND COALESCE(DeleteAt, 0) = 0`, params)
 	if err != nil {
 		return errors.Wrapf(err, "failed to delete Reactions with emojiName=%s", emojiName)
 	}
@@ -167,23 +194,60 @@ func (s *SqlReactionStore) PermanentDeleteBatch(endTime int64, limit int64) (int
 	return rowsAffected, nil
 }

-func saveReactionAndUpdatePost(transaction *gorp.Transaction, reaction *model.Reaction) error {
-	if err := transaction.Insert(reaction); err != nil {
-		return err
+func (s *SqlReactionStore) saveReactionAndUpdatePost(transaction *gorp.Transaction, reaction *model.Reaction) error {
+	params := map[string]interface{}{
+		"UserId":    reaction.UserId,
+		"PostId":    reaction.PostId,
+		"EmojiName": reaction.EmojiName,
+		"CreateAt":  reaction.CreateAt,
+		"UpdateAt":  reaction.UpdateAt,
 	}

+	if s.DriverName() == model.DATABASE_DRIVER_MYSQL {
+		if _, err := transaction.Exec(
+			`INSERT INTO
+				Reactions
+				(UserId, PostId, EmojiName, CreateAt, UpdateAt, DeleteAt)
+			VALUES
+				(:UserId, :PostId, :EmojiName, :CreateAt, :UpdateAt, 0)
+			ON DUPLICATE KEY UPDATE
+				UpdateAt = :UpdateAt, DeleteAt = 0`, params); err != nil {
+			return err
+		}
+	} else if s.DriverName() == model.DATABASE_DRIVER_POSTGRES {
+		if _, err := transaction.Exec(
+			`INSERT INTO
+				Reactions
+				(UserId, PostId, EmojiName, CreateAt, UpdateAt, DeleteAt)
+			VALUES
+				(:UserId, :PostId, :EmojiName, :CreateAt, :UpdateAt, 0)
+			ON CONFLICT (UserId, PostId, EmojiName) 
+				DO UPDATE SET UpdateAt = :UpdateAt, DeleteAt = 0`, params); err != nil {
+			return err
+		}
+	}
 	return updatePostForReactionsOnInsert(transaction, reaction.PostId)
 }

 func deleteReactionAndUpdatePost(transaction *gorp.Transaction, reaction *model.Reaction) error {
+	params := map[string]interface{}{
+		"UserId":    reaction.UserId,
+		"PostId":    reaction.PostId,
+		"EmojiName": reaction.EmojiName,
+		"CreateAt":  reaction.CreateAt,
+		"UpdateAt":  reaction.UpdateAt,
+		"DeleteAt":  reaction.UpdateAt, // DeleteAt = UpdateAt
+	}
+
 	if _, err := transaction.Exec(
-		`DELETE FROM
+		`UPDATE
 			Reactions
+		SET 
+			UpdateAt = :UpdateAt, DeleteAt = :DeleteAt
 		WHERE
 			PostId = :PostId AND
 			UserId = :UserId AND
-			EmojiName = :EmojiName`,
-		map[string]interface{}{"PostId": reaction.PostId, "UserId": reaction.UserId, "EmojiName": reaction.EmojiName}); err != nil {
+			EmojiName = :EmojiName`, params); err != nil {
 		return err
 	}

@@ -195,7 +259,7 @@ const (
 			Posts
 		SET
 			UpdateAt = :UpdateAt,
-			HasReactions = (SELECT count(0) > 0 FROM Reactions WHERE PostId = :PostId)
+			HasReactions = (SELECT count(0) > 0 FROM Reactions WHERE PostId = :PostId AND COALESCE(DeleteAt, 0) = 0)
 		WHERE
 			Id = :PostId`
 )
--- a/store/sqlstore/reaction_store_test.go
+++ b/store/sqlstore/reaction_store_test.go
@@ -10,5 +10,5 @@ import (
 )

 func TestReactionStore(t *testing.T) {
-	StoreTest(t, storetest.TestReactionStore)
+	StoreTestWithSqlStore(t, storetest.TestReactionStore)
 }
--- a/store/sqlstore/upgrade.go
+++ b/store/sqlstore/upgrade.go
@@ -966,6 +966,9 @@ func upgradeDatabaseToVersion532(sqlStore *SqlStore) {
 	sqlStore.AlterColumnTypeIfExists("Posts", "FileIds", "text", "varchar(300)")
 	sqlStore.CreateColumnIfNotExists("ThreadMemberships", "UnreadMentions", "bigint", "bigint", "0")
 	sqlStore.CreateColumnIfNotExistsNoDefault("Channels", "Shared", "tinyint(1)", "boolean")
+	sqlStore.CreateColumnIfNotExistsNoDefault("Reactions", "UpdateAt", "bigint", "bigint")
+	sqlStore.CreateColumnIfNotExistsNoDefault("Reactions", "DeleteAt", "bigint", "bigint")
+
 	// saveSchemaVersion(sqlStore, Version5320)
 	// }
 }
--- a/store/storetest/reaction_store.go
+++ b/store/storetest/reaction_store.go
@@ -16,11 +16,11 @@ import (
 	"github.com/mattermost/mattermost-server/v5/store/retrylayer"
 )

-func TestReactionStore(t *testing.T, ss store.Store) {
+func TestReactionStore(t *testing.T, ss store.Store, s SqlStore) {
 	t.Run("ReactionSave", func(t *testing.T) { testReactionSave(t, ss) })
 	t.Run("ReactionDelete", func(t *testing.T) { testReactionDelete(t, ss) })
 	t.Run("ReactionGetForPost", func(t *testing.T) { testReactionGetForPost(t, ss) })
-	t.Run("ReactionDeleteAllWithEmojiName", func(t *testing.T) { testReactionDeleteAllWithEmojiName(t, ss) })
+	t.Run("ReactionDeleteAllWithEmojiName", func(t *testing.T) { testReactionDeleteAllWithEmojiName(t, ss, s) })
 	t.Run("PermanentDeleteBatch", func(t *testing.T) { testReactionStorePermanentDeleteBatch(t, ss) })
 	t.Run("ReactionBulkGetForPosts", func(t *testing.T) { testReactionBulkGetForPosts(t, ss) })
 	t.Run("ReactionDeadlock", func(t *testing.T) { testReactionDeadlock(t, ss) })
@@ -48,6 +48,8 @@ func testReactionSave(t *testing.T, ss store.Store) {
 	assert.Equal(t, saved.UserId, reaction1.UserId, "should've saved reaction user_id and returned it")
 	assert.Equal(t, saved.PostId, reaction1.PostId, "should've saved reaction post_id and returned it")
 	assert.Equal(t, saved.EmojiName, reaction1.EmojiName, "should've saved reaction emoji_name and returned it")
+	assert.NotZero(t, saved.UpdateAt, "should've saved reaction update_at and returned it")
+	assert.Zero(t, saved.DeleteAt, "should've saved reaction delete_at with zero value and returned it")

 	var secondUpdateAt int64
 	postList, err := ss.Post().Get(reaction1.PostId, false, false, false)
@@ -108,39 +110,73 @@ func testReactionSave(t *testing.T, ss store.Store) {
 }

 func testReactionDelete(t *testing.T, ss store.Store) {
-	post, err := ss.Post().Save(&model.Post{
-		ChannelId: model.NewId(),
-		UserId:    model.NewId(),
+	t.Run("Delete", func(t *testing.T) {
+		post, err := ss.Post().Save(&model.Post{
+			ChannelId: model.NewId(),
+			UserId:    model.NewId(),
+		})
+		require.Nil(t, err)
+
+		reaction := &model.Reaction{
+			UserId:    model.NewId(),
+			PostId:    post.Id,
+			EmojiName: model.NewId(),
+		}
+
+		_, nErr := ss.Reaction().Save(reaction)
+		require.Nil(t, nErr)
+
+		result, err := ss.Post().Get(reaction.PostId, false, false, false)
+		require.Nil(t, err)
+
+		firstUpdateAt := result.Posts[post.Id].UpdateAt
+
+		_, nErr = ss.Reaction().Delete(reaction)
+		require.Nil(t, nErr)
+
+		reactions, rErr := ss.Reaction().GetForPost(post.Id, false)
+		require.Nil(t, rErr)
+
+		assert.Empty(t, reactions, "should've deleted reaction")
+
+		postList, err := ss.Post().Get(post.Id, false, false, false)
+		require.Nil(t, err)
+
+		assert.False(t, postList.Posts[post.Id].HasReactions, "should've set HasReactions = false on post")
+		assert.NotEqual(t, postList.Posts[post.Id].UpdateAt, firstUpdateAt, "should mark post as updated after deleting reactions")
 	})
-	require.Nil(t, err)

-	reaction := &model.Reaction{
-		UserId:    model.NewId(),
-		PostId:    post.Id,
-		EmojiName: model.NewId(),
-	}
+	t.Run("Undelete", func(t *testing.T) {
+		post, err := ss.Post().Save(&model.Post{
+			ChannelId: model.NewId(),
+			UserId:    model.NewId(),
+		})
+		require.Nil(t, err)

-	_, nErr := ss.Reaction().Save(reaction)
-	require.Nil(t, nErr)
+		reaction := &model.Reaction{
+			UserId:    model.NewId(),
+			PostId:    post.Id,
+			EmojiName: model.NewId(),
+		}

-	result, err := ss.Post().Get(reaction.PostId, false, false, false)
-	require.Nil(t, err)
+		savedReaction, nErr := ss.Reaction().Save(reaction)
+		require.Nil(t, nErr)

-	firstUpdateAt := result.Posts[post.Id].UpdateAt
+		updateAt := savedReaction.UpdateAt

-	_, nErr = ss.Reaction().Delete(reaction)
-	require.Nil(t, nErr)
+		_, nErr = ss.Reaction().Delete(savedReaction)
+		require.Nil(t, nErr)

-	reactions, rErr := ss.Reaction().GetForPost(post.Id, false)
-	require.Nil(t, rErr)
+		// add same reaction back and ensure update_at is set
+		_, nErr = ss.Reaction().Save(savedReaction)
+		require.Nil(t, nErr)

-	assert.Empty(t, reactions, "should've deleted reaction")
+		reactions, err := ss.Reaction().GetForPost(post.Id, false)
+		require.Nil(t, err)

-	postList, err := ss.Post().Get(post.Id, false, false, false)
-	require.Nil(t, err)
-
-	assert.False(t, postList.Posts[post.Id].HasReactions, "should've set HasReactions = false on post")
-	assert.NotEqual(t, postList.Posts[post.Id].UpdateAt, firstUpdateAt, "should mark post as updated after deleting reactions")
+		assert.Len(t, reactions, 1)
+		assert.GreaterOrEqual(t, reactions[0].UpdateAt, updateAt)
+	})
 }

 func testReactionGetForPost(t *testing.T, ss store.Store) {
@@ -176,6 +212,17 @@ func testReactionGetForPost(t *testing.T, ss store.Store) {
 		require.Nil(t, err)
 	}

+	// save and delete an additional reaction to test soft deletion
+	temp := &model.Reaction{
+		UserId:    userId,
+		PostId:    postId,
+		EmojiName: "grin",
+	}
+	savedTmp, err := ss.Reaction().Save(temp)
+	require.Nil(t, err)
+	_, err = ss.Reaction().Delete(savedTmp)
+	require.Nil(t, err)
+
 	returned, err := ss.Reaction().GetForPost(postId, false)
 	require.Nil(t, err)
 	require.Len(t, returned, 3, "should've returned 3 reactions")
@@ -185,7 +232,7 @@ func testReactionGetForPost(t *testing.T, ss store.Store) {

 		for _, returnedReaction := range returned {
 			if returnedReaction.UserId == reaction.UserId && returnedReaction.PostId == reaction.PostId &&
-				returnedReaction.EmojiName == reaction.EmojiName {
+				returnedReaction.EmojiName == reaction.EmojiName && returnedReaction.UpdateAt > 0 {
 				found = true
 				break
 			}
@@ -222,7 +269,7 @@ func testReactionGetForPost(t *testing.T, ss store.Store) {
 	}
 }

-func testReactionDeleteAllWithEmojiName(t *testing.T, ss store.Store) {
+func testReactionDeleteAllWithEmojiName(t *testing.T, ss store.Store, s SqlStore) {
 	emojiToDelete := model.NewId()

 	post, err1 := ss.Post().Save(&model.Post{
@@ -276,7 +323,25 @@ func testReactionDeleteAllWithEmojiName(t *testing.T, ss store.Store) {
 		require.Nil(t, err)
 	}

-	err := ss.Reaction().DeleteAllWithEmojiName(emojiToDelete)
+	// make at least one Reaction record contain NULL for Update and DeleteAt to simulate post schema upgrade case.
+	sqlResult, err := s.GetMaster().Exec(`
+		UPDATE 
+				Reactions 
+			SET 
+				UpdateAt=NULL, DeleteAt=NULL 
+			WHERE 
+				UserId = :UserId AND PostId = :PostId AND EmojiName = :EmojiName`,
+		map[string]interface{}{
+			"UserId":    userId,
+			"PostId":    post.Id,
+			"EmojiName": emojiToDelete,
+		})
+	require.Nil(t, err)
+	rowsAffected, err := sqlResult.RowsAffected()
+	require.Nil(t, err)
+	require.NotZero(t, rowsAffected)
+
+	err = ss.Reaction().DeleteAllWithEmojiName(emojiToDelete)
 	require.Nil(t, err)

 	// check that the reactions were deleted