From 20d690b41231a8462b02027287acebe350fc79d0 Mon Sep 17 00:00:00 2001 From: Shivashis Padhi Date: Mon, 11 Jul 2022 18:55:03 +0530 Subject: [PATCH] [MM-45444] Denormalize Reactions to add ChannelId for top reactions insights query (#20572) * Denormalize Reactions to add ChannelId for top reactions insights query * Remove hardcoded timestamps * Fix store, api4 tests for reactions * Fix tests * Fix integrity tests, allow reaction to have ChannelId populated before calling store function * Lint fixes * Add ChannelId field to BulkGetForPosts, Delete store handlers * Add index to mysql migration, add not null characteristic without a separate command * Select channelId instead of fetching post via store.GetPost, add if exists to drop column * Make updating of Reactions conditional to support pre-migration Co-authored-by: Mattermod --- db/migrations/migrations.list | 4 + .../000089_add-channelid-to-reaction.down.sql | 14 ++++ .../000089_add-channelid-to-reaction.up.sql | 33 ++++++++ .../000089_add-channelid-to-reaction.down.sql | 1 + .../000089_add-channelid-to-reaction.up.sql | 3 + model/reaction.go | 1 + store/sqlstore/integrity_test.go | 1 + store/sqlstore/reaction_store.go | 36 +++++--- store/storetest/reaction_store.go | 84 +++++++++++++++---- 9 files changed, 148 insertions(+), 29 deletions(-) create mode 100644 db/migrations/mysql/000089_add-channelid-to-reaction.down.sql create mode 100644 db/migrations/mysql/000089_add-channelid-to-reaction.up.sql create mode 100644 db/migrations/postgres/000089_add-channelid-to-reaction.down.sql create mode 100644 db/migrations/postgres/000089_add-channelid-to-reaction.up.sql diff --git a/db/migrations/migrations.list b/db/migrations/migrations.list index 3e871f1225..b5b1349f2f 100644 --- a/db/migrations/migrations.list +++ b/db/migrations/migrations.list @@ -176,6 +176,8 @@ db/migrations/mysql/000087_sidebar_categories_index.down.sql db/migrations/mysql/000087_sidebar_categories_index.up.sql db/migrations/mysql/000088_remaining_migrations.down.sql db/migrations/mysql/000088_remaining_migrations.up.sql +db/migrations/mysql/000089_add-channelid-to-reaction.down.sql +db/migrations/mysql/000089_add-channelid-to-reaction.up.sql db/migrations/postgres/000001_create_teams.down.sql db/migrations/postgres/000001_create_teams.up.sql db/migrations/postgres/000002_create_team_members.down.sql @@ -352,3 +354,5 @@ db/migrations/postgres/000087_sidebar_categories_index.down.sql db/migrations/postgres/000087_sidebar_categories_index.up.sql db/migrations/postgres/000088_remaining_migrations.down.sql db/migrations/postgres/000088_remaining_migrations.up.sql +db/migrations/postgres/000089_add-channelid-to-reaction.down.sql +db/migrations/postgres/000089_add-channelid-to-reaction.up.sql diff --git a/db/migrations/mysql/000089_add-channelid-to-reaction.down.sql b/db/migrations/mysql/000089_add-channelid-to-reaction.down.sql new file mode 100644 index 0000000000..0380b0de94 --- /dev/null +++ b/db/migrations/mysql/000089_add-channelid-to-reaction.down.sql @@ -0,0 +1,14 @@ +SET @preparedStatement = (SELECT IF( + EXISTS( + SELECT 1 FROM INFORMATION_SCHEMA.STATISTICS + WHERE table_name = 'Reactions' + AND table_schema = DATABASE() + AND column_name = 'ChannelId' + ) > 0, + 'ALTER TABLE Reactions DROP COLUMN ChannelId;', + 'SELECT 1;' +)); + +PREPARE removeColumnIfExists FROM @preparedStatement; +EXECUTE removeColumnIfExists; +DEALLOCATE PREPARE removeColumnIfExists; diff --git a/db/migrations/mysql/000089_add-channelid-to-reaction.up.sql b/db/migrations/mysql/000089_add-channelid-to-reaction.up.sql new file mode 100644 index 0000000000..e1864b938b --- /dev/null +++ b/db/migrations/mysql/000089_add-channelid-to-reaction.up.sql @@ -0,0 +1,33 @@ +SET @preparedStatement = (SELECT IF( + NOT EXISTS( + SELECT 1 FROM INFORMATION_SCHEMA.COLUMNS + WHERE table_name = 'Reactions' + AND table_schema = DATABASE() + AND column_name = 'ChannelId' + ), + 'ALTER TABLE Reactions ADD COLUMN ChannelId varchar(26) NOT NULL DEFAULT "";', + 'SELECT 1;' +)); + +PREPARE addColumnIfNotExists FROM @preparedStatement; +EXECUTE addColumnIfNotExists; +DEALLOCATE PREPARE addColumnIfNotExists; + + +UPDATE Reactions SET ChannelId = (select ChannelId from Posts where Posts.Id = Reactions.PostId) WHERE ChannelId=""; + + +SET @preparedStatement = (SELECT IF( + ( + SELECT COUNT(*) FROM INFORMATION_SCHEMA.STATISTICS + WHERE table_name = 'Reactions' + AND table_schema = DATABASE() + AND index_name = 'idx_reactions_channel_id' + ) > 0, + 'SELECT 1', + 'CREATE INDEX idx_reactions_channel_id ON Reactions(ChannelId);' +)); + +PREPARE createIndexIfNotExists FROM @preparedStatement; +EXECUTE createIndexIfNotExists; +DEALLOCATE PREPARE createIndexIfNotExists; diff --git a/db/migrations/postgres/000089_add-channelid-to-reaction.down.sql b/db/migrations/postgres/000089_add-channelid-to-reaction.down.sql new file mode 100644 index 0000000000..a772e1cd5a --- /dev/null +++ b/db/migrations/postgres/000089_add-channelid-to-reaction.down.sql @@ -0,0 +1 @@ +ALTER TABLE reactions DROP COLUMN IF EXISTS channelid; diff --git a/db/migrations/postgres/000089_add-channelid-to-reaction.up.sql b/db/migrations/postgres/000089_add-channelid-to-reaction.up.sql new file mode 100644 index 0000000000..2affd3fcde --- /dev/null +++ b/db/migrations/postgres/000089_add-channelid-to-reaction.up.sql @@ -0,0 +1,3 @@ +ALTER TABLE reactions ADD COLUMN IF NOT EXISTS channelid varchar(26) NOT NULL DEFAULT ''; +UPDATE reactions SET channelid = (select channelid from posts where posts.id = reactions.postid) WHERE channelid=''; +CREATE INDEX IF NOT EXISTS idx_reactions_channel_id on reactions (channelid); diff --git a/model/reaction.go b/model/reaction.go index 335cb904b0..0a0ed36027 100644 --- a/model/reaction.go +++ b/model/reaction.go @@ -16,6 +16,7 @@ type Reaction struct { UpdateAt int64 `json:"update_at"` DeleteAt int64 `json:"delete_at"` RemoteId *string `json:"remote_id"` + ChannelId string `json:"channel_id"` } func (o *Reaction) IsValid() *AppError { diff --git a/store/sqlstore/integrity_test.go b/store/sqlstore/integrity_test.go index 1e331bc151..84096ab998 100644 --- a/store/sqlstore/integrity_test.go +++ b/store/sqlstore/integrity_test.go @@ -212,6 +212,7 @@ func createReaction(ss store.Store, userId, postId string) *model.Reaction { UserId: userId, PostId: postId, EmojiName: model.NewId(), + ChannelId: model.NewId(), } reaction, _ = ss.Reaction().Save(reaction) return reaction diff --git a/store/sqlstore/reaction_store.go b/store/sqlstore/reaction_store.go index e88840568c..7651257ff5 100644 --- a/store/sqlstore/reaction_store.go +++ b/store/sqlstore/reaction_store.go @@ -26,12 +26,23 @@ func (s *SqlReactionStore) Save(reaction *model.Reaction) (*model.Reaction, erro if err := reaction.IsValid(); err != nil { return nil, err } - transaction, err := s.GetMasterX().Beginx() if err != nil { return nil, errors.Wrap(err, "begin_transaction") } defer finalizeTransactionX(transaction) + if reaction.ChannelId == "" { + // get channelId, if not already populated + var channelIds []string + var args []interface{} + query := "SELECT ChannelId from Posts where Id = ?" + args = append(args, reaction.PostId) + err = transaction.Select(&channelIds, query, args...) + if err != nil { + return nil, errors.Wrap(err, "failed while getting channelId from Posts") + } + reaction.ChannelId = channelIds[0] + } err = s.saveReactionAndUpdatePost(transaction, reaction) if err != nil { // We don't consider duplicated save calls as an error @@ -71,7 +82,7 @@ func (s *SqlReactionStore) Delete(reaction *model.Reaction) (*model.Reaction, er func (s *SqlReactionStore) GetForPost(postId string, allowFromCache bool) ([]*model.Reaction, error) { queryString, args, err := s.getQueryBuilder(). Select("UserId", "PostId", "EmojiName", "CreateAt", "COALESCE(UpdateAt, CreateAt) As UpdateAt", - "COALESCE(DeleteAt, 0) As DeleteAt", "RemoteId"). + "COALESCE(DeleteAt, 0) As DeleteAt", "RemoteId", "ChannelId"). From("Reactions"). Where(sq.Eq{"PostId": postId}). Where(sq.Eq{"COALESCE(DeleteAt, 0)": 0}). @@ -132,7 +143,8 @@ func (s *SqlReactionStore) BulkGetForPosts(postIds []string) ([]*model.Reaction, CreateAt, COALESCE(UpdateAt, CreateAt) As UpdateAt, COALESCE(DeleteAt, 0) As DeleteAt, - RemoteId + RemoteId, + ChannelId FROM Reactions WHERE @@ -247,8 +259,7 @@ func (s *SqlReactionStore) GetTopForTeamSince(teamID string, userID string, sinc FROM ChannelMembers INNER JOIN Channels ON ChannelMembers.ChannelId = Channels.Id - INNER JOIN Posts ON Channels.Id = Posts.ChannelId - INNER JOIN Reactions ON Posts.Id = Reactions.PostId + INNER JOIN Reactions ON Channels.Id = Reactions.ChannelId WHERE ChannelMembers.UserId = ? AND Channels.Type = 'P' @@ -265,8 +276,7 @@ func (s *SqlReactionStore) GetTopForTeamSince(teamID string, userID string, sinc Reactions.CreateAt AS CreateAt FROM Reactions - INNER JOIN Posts ON Reactions.PostId = Posts.Id - INNER JOIN PublicChannels ON Posts.ChannelId = PublicChannels.Id + INNER JOIN PublicChannels ON Reactions.ChannelId = PublicChannels.Id WHERE PublicChannels.TeamId = ? GROUP BY @@ -356,22 +366,22 @@ func (s *SqlReactionStore) saveReactionAndUpdatePost(transaction *sqlxTxWrapper, if _, err := transaction.NamedExec( `INSERT INTO Reactions - (UserId, PostId, EmojiName, CreateAt, UpdateAt, DeleteAt, RemoteId) + (UserId, PostId, EmojiName, CreateAt, UpdateAt, DeleteAt, RemoteId, ChannelId) VALUES - (:UserId, :PostId, :EmojiName, :CreateAt, :UpdateAt, :DeleteAt, :RemoteId) + (:UserId, :PostId, :EmojiName, :CreateAt, :UpdateAt, :DeleteAt, :RemoteId, :ChannelId) ON DUPLICATE KEY UPDATE - UpdateAt = :UpdateAt, DeleteAt = :DeleteAt, RemoteId = :RemoteId`, reaction); err != nil { + UpdateAt = :UpdateAt, DeleteAt = :DeleteAt, RemoteId = :RemoteId, ChannelId = :ChannelId`, reaction); err != nil { return err } } else if s.DriverName() == model.DatabaseDriverPostgres { if _, err := transaction.NamedExec( `INSERT INTO Reactions - (UserId, PostId, EmojiName, CreateAt, UpdateAt, DeleteAt, RemoteId) + (UserId, PostId, EmojiName, CreateAt, UpdateAt, DeleteAt, RemoteId, ChannelId) VALUES - (:UserId, :PostId, :EmojiName, :CreateAt, :UpdateAt, :DeleteAt, :RemoteId) + (:UserId, :PostId, :EmojiName, :CreateAt, :UpdateAt, :DeleteAt, :RemoteId, :ChannelId) ON CONFLICT (UserId, PostId, EmojiName) - DO UPDATE SET UpdateAt = :UpdateAt, DeleteAt = :DeleteAt, RemoteId = :RemoteId`, reaction); err != nil { + DO UPDATE SET UpdateAt = :UpdateAt, DeleteAt = :DeleteAt, RemoteId = :RemoteId, ChannelId = :ChannelId`, reaction); err != nil { return err } } diff --git a/store/storetest/reaction_store.go b/store/storetest/reaction_store.go index aa75a872ec..0093f181d5 100644 --- a/store/storetest/reaction_store.go +++ b/store/storetest/reaction_store.go @@ -52,6 +52,7 @@ func testReactionSave(t *testing.T, ss store.Store) { 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.Equal(t, saved.ChannelId, post.ChannelId, "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 @@ -85,9 +86,16 @@ func testReactionSave(t *testing.T, ss store.Store) { assert.NotEqual(t, postList.Posts[post.Id].UpdateAt, secondUpdateAt, "should've marked post as updated even if HasReactions doesn't change") // different post + // create post1 + post1, err := ss.Post().Save(&model.Post{ + ChannelId: model.NewId(), + UserId: model.NewId(), + }) + require.NoError(t, err) + reaction3 := &model.Reaction{ UserId: reaction1.UserId, - PostId: model.NewId(), + PostId: post1.Id, EmojiName: reaction1.EmojiName, } _, nErr = ss.Reaction().Save(reaction3) @@ -183,9 +191,21 @@ func testReactionDelete(t *testing.T, ss store.Store) { } func testReactionGetForPost(t *testing.T, ss store.Store) { - postId := model.NewId() - userId := model.NewId() + // create post + post, err := ss.Post().Save(&model.Post{ + ChannelId: model.NewId(), + UserId: userId, + }) + require.NoError(t, err) + post1, err := ss.Post().Save(&model.Post{ + ChannelId: model.NewId(), + UserId: userId, + }) + require.NoError(t, err) + + postId := post.Id + post1Id := post1.Id reactions := []*model.Reaction{ { @@ -194,7 +214,7 @@ func testReactionGetForPost(t *testing.T, ss store.Store) { EmojiName: "smile", }, { - UserId: model.NewId(), + UserId: post1Id, PostId: postId, EmojiName: "smile", }, @@ -205,13 +225,13 @@ func testReactionGetForPost(t *testing.T, ss store.Store) { }, { UserId: userId, - PostId: model.NewId(), + PostId: post1Id, EmojiName: "angry", }, } for _, reaction := range reactions { - _, err := ss.Reaction().Save(reaction) + _, err = ss.Reaction().Save(reaction) require.NoError(t, err) } @@ -276,9 +296,21 @@ func testReactionGetForPostSince(t *testing.T, ss store.Store, s SqlStore) { now := model.GetMillis() later := now + 1800000 // add 30 minutes remoteId := model.NewId() - - postId := model.NewId() userId := model.NewId() + + // create post + post, _ := ss.Post().Save(&model.Post{ + ChannelId: model.NewId(), + UserId: userId, + }) + post1, _ := ss.Post().Save(&model.Post{ + ChannelId: model.NewId(), + UserId: userId, + }) + + postId := post.Id + post1Id := post1.Id + reactions := []*model.Reaction{ { UserId: userId, @@ -300,7 +332,7 @@ func testReactionGetForPostSince(t *testing.T, ss store.Store, s SqlStore) { }, { UserId: userId, - PostId: model.NewId(), + PostId: post1Id, EmojiName: "angry", }, { @@ -591,12 +623,27 @@ func testReactionStorePermanentDeleteBatch(t *testing.T, ss store.Store) { } func testReactionBulkGetForPosts(t *testing.T, ss store.Store) { - postId := model.NewId() - post2Id := model.NewId() - post3Id := model.NewId() - post4Id := model.NewId() - userId := model.NewId() + post, _ := ss.Post().Save(&model.Post{ + ChannelId: model.NewId(), + UserId: userId, + }) + postId := post.Id + post, _ = ss.Post().Save(&model.Post{ + ChannelId: model.NewId(), + UserId: userId, + }) + post2Id := post.Id + post, _ = ss.Post().Save(&model.Post{ + ChannelId: model.NewId(), + UserId: userId, + }) + post3Id := post.Id + post, _ = ss.Post().Save(&model.Post{ + ChannelId: model.NewId(), + UserId: userId, + }) + post4Id := post.Id reactions := []*model.Reaction{ { @@ -663,7 +710,12 @@ func testReactionDeadlock(t *testing.T, ss store.Store) { UserId: model.NewId(), }) require.NoError(t, err) - + postId := post.Id + post, err = ss.Post().Save(&model.Post{ + ChannelId: model.NewId(), + UserId: model.NewId(), + }) + require.NoError(t, err) reaction1 := &model.Reaction{ UserId: model.NewId(), PostId: post.Id, @@ -684,7 +736,7 @@ func testReactionDeadlock(t *testing.T, ss store.Store) { // different post reaction3 := &model.Reaction{ UserId: reaction1.UserId, - PostId: model.NewId(), + PostId: postId, EmojiName: reaction1.EmojiName, } _, nErr = ss.Reaction().Save(reaction3)