diff --git a/i18n/en.json b/i18n/en.json index 0ba67d7a4e..36182284fd 100644 --- a/i18n/en.json +++ b/i18n/en.json @@ -7946,6 +7946,10 @@ "id": "model.reaction.is_valid.post_id.app_error", "translation": "Invalid post id." }, + { + "id": "model.reaction.is_valid.update_at.app_error", + "translation": "Update at must be a valid time." + }, { "id": "model.reaction.is_valid.user_id.app_error", "translation": "Invalid user id." diff --git a/model/reaction.go b/model/reaction.go index 6f950d74c3..b90a1b2aec 100644 --- a/model/reaction.go +++ b/model/reaction.go @@ -15,6 +15,8 @@ type Reaction struct { PostId string `json:"post_id"` EmojiName string `json:"emoji_name"` CreateAt int64 `json:"create_at"` + UpdateAt int64 `json:"update_at"` + DeleteAt int64 `json:"delete_at"` } func (o *Reaction) ToJson() string { @@ -79,6 +81,10 @@ func (o *Reaction) IsValid() *AppError { return NewAppError("Reaction.IsValid", "model.reaction.is_valid.create_at.app_error", nil, "", http.StatusBadRequest) } + if o.UpdateAt == 0 { + return NewAppError("Reaction.IsValid", "model.reaction.is_valid.update_at.app_error", nil, "", http.StatusBadRequest) + } + return nil } @@ -86,4 +92,10 @@ func (o *Reaction) PreSave() { if o.CreateAt == 0 { o.CreateAt = GetMillis() } + o.UpdateAt = GetMillis() + o.DeleteAt = 0 +} + +func (o *Reaction) PreUpdate() { + o.UpdateAt = GetMillis() } diff --git a/model/reaction_test.go b/model/reaction_test.go index ede7b3ac14..b88726a714 100644 --- a/model/reaction_test.go +++ b/model/reaction_test.go @@ -25,6 +25,7 @@ func TestReactionIsValid(t *testing.T) { PostId: NewId(), EmojiName: "emoji", CreateAt: GetMillis(), + UpdateAt: GetMillis(), }, errMsg: "", shouldErr: false, @@ -35,6 +36,7 @@ func TestReactionIsValid(t *testing.T) { PostId: NewId(), EmojiName: "emoji", CreateAt: GetMillis(), + UpdateAt: GetMillis(), }, errMsg: "user id should be invalid", shouldErr: true, @@ -45,6 +47,7 @@ func TestReactionIsValid(t *testing.T) { PostId: NewId(), EmojiName: "emoji", CreateAt: GetMillis(), + UpdateAt: GetMillis(), }, errMsg: "user id should be invalid", shouldErr: true, @@ -55,6 +58,7 @@ func TestReactionIsValid(t *testing.T) { PostId: "", EmojiName: "emoji", CreateAt: GetMillis(), + UpdateAt: GetMillis(), }, errMsg: "post id should be invalid", shouldErr: true, @@ -65,6 +69,7 @@ func TestReactionIsValid(t *testing.T) { PostId: "1234garbage", EmojiName: "emoji", CreateAt: GetMillis(), + UpdateAt: GetMillis(), }, errMsg: "post id should be invalid", shouldErr: true, @@ -75,6 +80,7 @@ func TestReactionIsValid(t *testing.T) { PostId: NewId(), EmojiName: strings.Repeat("a", 64), CreateAt: GetMillis(), + UpdateAt: GetMillis(), }, errMsg: "", shouldErr: false, @@ -85,6 +91,7 @@ func TestReactionIsValid(t *testing.T) { PostId: NewId(), EmojiName: "emoji-", CreateAt: GetMillis(), + UpdateAt: GetMillis(), }, errMsg: "", shouldErr: false, @@ -95,6 +102,7 @@ func TestReactionIsValid(t *testing.T) { PostId: NewId(), EmojiName: "emoji_", CreateAt: GetMillis(), + UpdateAt: GetMillis(), }, errMsg: "", shouldErr: false, @@ -105,6 +113,7 @@ func TestReactionIsValid(t *testing.T) { PostId: NewId(), EmojiName: "+1", CreateAt: GetMillis(), + UpdateAt: GetMillis(), }, errMsg: "", shouldErr: false, @@ -115,6 +124,7 @@ func TestReactionIsValid(t *testing.T) { PostId: NewId(), EmojiName: "emoji:", CreateAt: GetMillis(), + UpdateAt: GetMillis(), }, errMsg: "", shouldErr: true, @@ -125,6 +135,7 @@ func TestReactionIsValid(t *testing.T) { PostId: NewId(), EmojiName: "", CreateAt: GetMillis(), + UpdateAt: GetMillis(), }, errMsg: "emoji name should be invalid", shouldErr: true, @@ -135,6 +146,7 @@ func TestReactionIsValid(t *testing.T) { PostId: NewId(), EmojiName: strings.Repeat("a", 65), CreateAt: GetMillis(), + UpdateAt: GetMillis(), }, errMsg: "emoji name should be invalid", shouldErr: true, @@ -145,10 +157,22 @@ func TestReactionIsValid(t *testing.T) { PostId: NewId(), EmojiName: "emoji", CreateAt: 0, + UpdateAt: GetMillis(), }, errMsg: "create at should be invalid", shouldErr: true, }, + { + reaction: Reaction{ + UserId: NewId(), + PostId: NewId(), + EmojiName: "emoji", + CreateAt: GetMillis(), + UpdateAt: 0, + }, + errMsg: "update at should be invalid", + shouldErr: true, + }, } for _, test := range tests { diff --git a/store/localcachelayer/channel_layer_test.go b/store/localcachelayer/channel_layer_test.go index c4b131d213..cdd8561934 100644 --- 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) { diff --git a/store/localcachelayer/reaction_layer_test.go b/store/localcachelayer/reaction_layer_test.go index ccabdfdfd7..b2acccca07 100644 --- 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) { diff --git a/store/sqlstore/reaction_store.go b/store/sqlstore/reaction_store.go index 2223226a8b..23b0f48d95 100644 --- 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` ) diff --git a/store/sqlstore/reaction_store_test.go b/store/sqlstore/reaction_store_test.go index a23a20ce77..c793095334 100644 --- 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) } diff --git a/store/sqlstore/upgrade.go b/store/sqlstore/upgrade.go index 5435daf5ff..cf9b2a1fc8 100644 --- 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) // } } diff --git a/store/storetest/reaction_store.go b/store/storetest/reaction_store.go index 1d04e8ce29..33970257c2 100644 --- 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