[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 <mattermod@users.noreply.github.com>

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

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;

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;

db/migrations/postgres/000089_add-channelid-to-reaction.down.sql

@@ -0,0 +1 @@
+ALTER TABLE reactions DROP COLUMN IF EXISTS channelid;

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);

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 {

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

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 Channels.Id = Reactions.ChannelId
-					INNER JOIN Reactions ON Posts.Id = Reactions.PostId
 				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 Reactions.ChannelId = PublicChannels.Id
-					INNER JOIN PublicChannels ON Posts.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
 		}
 	}

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)