From 5bd223c836a71c1148a3b5990fde6f22f2fdc1ed Mon Sep 17 00:00:00 2001 From: Jesse Hallam Date: Tue, 19 Apr 2022 17:06:31 -0300 Subject: [PATCH] MM-43045: minimize JOIN Posts (#19934) --- app/post_test.go | 3 +- app/user.go | 25 ++- .../mysql/000081_threads_deleteat.down.sql | 14 ++ .../mysql/000081_threads_deleteat.up.sql | 19 ++ .../postgres/000081_threads_deleteat.down.sql | 1 + .../postgres/000081_threads_deleteat.up.sql | 2 + model/thread.go | 4 + store/sqlstore/post_store.go | 181 +++++++++------- store/sqlstore/thread_store.go | 191 +++++++++-------- store/storetest/post_store.go | 194 ++++++++++-------- store/storetest/thread_store.go | 110 +++++++--- store/storetest/user_store.go | 3 + 12 files changed, 465 insertions(+), 282 deletions(-) create mode 100644 db/migrations/mysql/000081_threads_deleteat.down.sql create mode 100644 db/migrations/mysql/000081_threads_deleteat.up.sql create mode 100644 db/migrations/postgres/000081_threads_deleteat.down.sql create mode 100644 db/migrations/postgres/000081_threads_deleteat.up.sql diff --git a/app/post_test.go b/app/post_test.go index a96696e19c..07fd9ecb3a 100644 --- a/app/post_test.go +++ b/app/post_test.go @@ -2491,13 +2491,12 @@ func TestCollapsedThreadFetch(t *testing.T) { }() require.NotPanics(t, func() { - _, err = th.App.CreatePost(th.Context, &model.Post{ + th.App.CreatePost(th.Context, &model.Post{ UserId: user1.Id, ChannelId: channel.Id, RootId: postRoot.Id, Message: fmt.Sprintf("@%s", user2.Username), }, channel, false, true) - require.Nil(t, err) }) wg.Wait() diff --git a/app/user.go b/app/user.go index c88ab9c7de..13313469ba 100644 --- a/app/user.go +++ b/app/user.go @@ -2311,15 +2311,20 @@ func (a *App) GetThreadsForUser(userID, teamID string, options model.GetUserThre return nil }) - eg.Go(func() error { - totalCount, err := a.Srv().Store.Thread().GetTotalThreads(userID, teamID, options) - if err != nil { - return errors.Wrapf(err, "failed to count threads for user id=%s", userID) - } - result.Total = totalCount + // Unread is a legacy flag that caused GetTotalThreads to compute the same value as + // GetTotalUnreadThreads. If unspecified, do this work normally; otherwise, skip, + // and send back duplicate values down below. + if !options.Unread { + eg.Go(func() error { + totalCount, err := a.Srv().Store.Thread().GetTotalThreads(userID, teamID, options) + if err != nil { + return errors.Wrapf(err, "failed to count threads for user id=%s", userID) + } + result.Total = totalCount - return nil - }) + return nil + }) + } eg.Go(func() error { totalUnreadMentions, err := a.Srv().Store.Thread().GetTotalUnreadMentions(userID, teamID, options) @@ -2348,6 +2353,10 @@ func (a *App) GetThreadsForUser(userID, teamID string, options model.GetUserThre return nil, model.NewAppError("GetThreadsForUser", "app.user.get_threads_for_user.app_error", nil, err.Error(), http.StatusInternalServerError) } + if options.Unread { + result.Total = result.TotalUnreadThreads + } + for _, thread := range result.Threads { a.sanitizeProfiles(thread.Participants, false) thread.Post.SanitizeProps() diff --git a/db/migrations/mysql/000081_threads_deleteat.down.sql b/db/migrations/mysql/000081_threads_deleteat.down.sql new file mode 100644 index 0000000000..3bae8762bc --- /dev/null +++ b/db/migrations/mysql/000081_threads_deleteat.down.sql @@ -0,0 +1,14 @@ +SET @preparedStatement = (SELECT IF( + EXISTS( + SELECT 1 FROM INFORMATION_SCHEMA.STATISTICS + WHERE table_name = 'Threads' + AND table_schema = DATABASE() + AND column_name = 'DeleteAt' + ) > 0, + 'ALTER TABLE Threads DROP COLUMN DeleteAt;', + 'SELECT 1;' +)); + +PREPARE removeColumnIfExists FROM @preparedStatement; +EXECUTE removeColumnIfExists; +DEALLOCATE PREPARE removeColumnIfExists; diff --git a/db/migrations/mysql/000081_threads_deleteat.up.sql b/db/migrations/mysql/000081_threads_deleteat.up.sql new file mode 100644 index 0000000000..578294be26 --- /dev/null +++ b/db/migrations/mysql/000081_threads_deleteat.up.sql @@ -0,0 +1,19 @@ +SET @preparedStatement = (SELECT IF( + NOT EXISTS( + SELECT 1 FROM INFORMATION_SCHEMA.COLUMNS + WHERE table_name = 'Threads' + AND table_schema = DATABASE() + AND column_name = 'DeleteAt' + ), + 'ALTER TABLE Threads ADD COLUMN DeleteAt bigint(20);', + 'SELECT 1;' +)); + +PREPARE addColumnIfNotExists FROM @preparedStatement; +EXECUTE addColumnIfNotExists; +DEALLOCATE PREPARE addColumnIfNotExists; + +UPDATE Threads, Posts +SET Threads.DeleteAt = Posts.DeleteAt +WHERE Posts.Id = Threads.PostId +AND Threads.DeleteAt IS NULL; diff --git a/db/migrations/postgres/000081_threads_deleteat.down.sql b/db/migrations/postgres/000081_threads_deleteat.down.sql new file mode 100644 index 0000000000..5d9d16db1f --- /dev/null +++ b/db/migrations/postgres/000081_threads_deleteat.down.sql @@ -0,0 +1 @@ +ALTER TABLE threads DROP COLUMN IF EXISTS deleteat; diff --git a/db/migrations/postgres/000081_threads_deleteat.up.sql b/db/migrations/postgres/000081_threads_deleteat.up.sql new file mode 100644 index 0000000000..5de4aa1905 --- /dev/null +++ b/db/migrations/postgres/000081_threads_deleteat.up.sql @@ -0,0 +1,2 @@ +ALTER TABLE threads ADD COLUMN IF NOT EXISTS deleteat bigint; +UPDATE threads SET deleteat = posts.deleteat FROM posts WHERE threads.deleteat IS NULL AND posts.id = threads.postid; diff --git a/model/thread.go b/model/thread.go index 17fffdddb3..d2ca025a72 100644 --- a/model/thread.go +++ b/model/thread.go @@ -22,6 +22,9 @@ type Thread struct { // Participants is a list of user ids that have replied to the thread, sorted by the oldest // to newest. Note that the root post author is not included in this list until they reply. Participants StringArray `json:"participants"` + + // DeleteAt is a denormalized copy of the root posts's DeleteAt. + DeleteAt int64 `json:"delete_at"` } type ThreadResponse struct { @@ -33,6 +36,7 @@ type ThreadResponse struct { Post *Post `json:"post"` UnreadReplies int64 `json:"unread_replies"` UnreadMentions int64 `json:"unread_mentions"` + DeleteAt int64 `json:"delete_at"` } type Threads struct { diff --git a/store/sqlstore/post_store.go b/store/sqlstore/post_store.go index bca52e9593..0e1e3e3251 100644 --- a/store/sqlstore/post_store.go +++ b/store/sqlstore/post_store.go @@ -215,7 +215,7 @@ func (s *SqlPostStore) SaveMultiple(posts []*model.Post) ([]*model.Post, int, er } if err = s.updateThreadsFromPosts(transaction, posts); err != nil { - mlog.Warn("Error updating posts, thread update failed", mlog.Err(err)) + return nil, -1, errors.Wrap(err, "update thread from posts failed") } if err = transaction.Commit(); err != nil { @@ -290,7 +290,7 @@ func (s *SqlPostStore) populateReplyCount(posts []*model.Post) error { Select("RootId, COUNT(Id) AS Count"). From("Posts"). Where(sq.Eq{"RootId": rootIds}). - Where(sq.Eq{"DeleteAt": 0}). + Where(sq.Eq{"Posts.DeleteAt": 0}). GroupBy("RootId") queryString, args, err := query.ToSql() @@ -487,7 +487,7 @@ func (s *SqlPostStore) getFlaggedPosts(userId, channelId, teamId string, offset AND Category = ? ) CHANNEL_FILTER - AND DeleteAt = 0 + AND Posts.DeleteAt = 0 ) as A INNER JOIN Channels as B ON B.Id = A.ChannelId @@ -569,8 +569,8 @@ func (s *SqlPostStore) getPostWithCollapsedThreads(id, userID string, opts model From("Posts"). LeftJoin("Threads ON Threads.PostId = Id"). LeftJoin("ThreadMemberships ON ThreadMemberships.PostId = Id AND ThreadMemberships.UserId = ?", userID). - Where(sq.Eq{"DeleteAt": 0}). - Where(sq.Eq{"Id": id}).ToSql() + Where(sq.Eq{"Posts.DeleteAt": 0}). + Where(sq.Eq{"Posts.Id": id}).ToSql() err := s.GetReplicaX().Get(&post, postFetchQuery, args...) if err != nil { @@ -586,8 +586,8 @@ func (s *SqlPostStore) getPostWithCollapsedThreads(id, userID string, opts model Select("*"). From("Posts"). Where(sq.Eq{ - "RootId": id, - "DeleteAt": 0, + "Posts.RootId": id, + "Posts.DeleteAt": 0, }) var sort string @@ -873,7 +873,11 @@ func (s *SqlPostStore) Delete(postID string, time int64, deleteByID string) erro return errors.Wrap(err, "failed to update Posts") } - err = s.cleanupThreadComments(transaction, postID, id.RootId, id.UserId) + if id.RootId == "" { + err = s.deleteThread(transaction, postID, time) + } else { + err = s.updateThreadAfterReplyDeletion(transaction, id.RootId, id.UserId) + } if err != nil { return errors.Wrapf(err, "failed to cleanup Thread with postid=%s", id.RootId) @@ -933,13 +937,12 @@ func (s *SqlPostStore) permanentDeleteAllCommentByUser(userId string) error { } _, err = transaction.Exec("DELETE FROM Posts WHERE UserId = ? AND RootId != ''", userId) - if err != nil { return errors.Wrapf(err, "failed to delete Posts with userId=%s", userId) } for _, ids := range results { - if err = s.cleanupThreadComments(transaction, ids.Id, ids.RootId, userId); err != nil { + if err = s.updateThreadAfterReplyDeletion(transaction, ids.RootId, userId); err != nil { return err } } @@ -1103,14 +1106,14 @@ func (s *SqlPostStore) getPostsCollapsedThreads(options model.GetPostsOptions) ( postFetchQuery, args, _ := s.getQueryBuilder(). Select(columns...). From("Posts"). - LeftJoin("Threads ON Threads.PostId = Id"). - LeftJoin("ThreadMemberships ON ThreadMemberships.PostId = Id AND ThreadMemberships.UserId = ?", options.UserId). - Where(sq.Eq{"DeleteAt": 0}). + LeftJoin("Threads ON Threads.PostId = Posts.Id"). + LeftJoin("ThreadMemberships ON ThreadMemberships.PostId = Posts.Id AND ThreadMemberships.UserId = ?", options.UserId). + Where(sq.Eq{"Posts.DeleteAt": 0}). Where(sq.Eq{"Posts.ChannelId": options.ChannelId}). - Where(sq.Eq{"RootId": ""}). + Where(sq.Eq{"Posts.RootId": ""}). Limit(uint64(options.PerPage)). Offset(uint64(offset)). - OrderBy("CreateAt DESC").ToSql() + OrderBy("Posts.CreateAt DESC").ToSql() err := s.GetReplicaX().Select(&posts, postFetchQuery, args...) if err != nil { @@ -1187,13 +1190,13 @@ func (s *SqlPostStore) getPostsSinceCollapsedThreads(options model.GetPostsSince postFetchQuery, args, _ := s.getQueryBuilder(). Select(columns...). From("Posts"). - LeftJoin("Threads ON Threads.PostId = Id"). - LeftJoin("ThreadMemberships ON ThreadMemberships.PostId = Id AND ThreadMemberships.UserId = ?", options.UserId). - Where(sq.Eq{"DeleteAt": 0}). + LeftJoin("Threads ON Threads.PostId = Posts.Id"). + LeftJoin("ThreadMemberships ON ThreadMemberships.PostId = Posts.Id AND ThreadMemberships.UserId = ?", options.UserId). + Where(sq.Eq{"Posts.DeleteAt": 0}). Where(sq.Eq{"Posts.ChannelId": options.ChannelId}). - Where(sq.Gt{"UpdateAt": options.Time}). - Where(sq.Eq{"RootId": ""}). - OrderBy("CreateAt DESC").ToSql() + Where(sq.Gt{"Posts.UpdateAt": options.Time}). + Where(sq.Eq{"Posts.RootId": ""}). + OrderBy("Posts.CreateAt DESC").ToSql() err := s.GetReplicaX().Select(&posts, postFetchQuery, args...) if err != nil { @@ -1316,16 +1319,16 @@ func (s *SqlPostStore) GetPostsSinceForSync(options model.GetPostsSinceForSyncOp query := s.getQueryBuilder(). Select("*"). From("Posts"). - Where(sq.Or{sq.Gt{"UpdateAt": cursor.LastPostUpdateAt}, sq.And{sq.Eq{"UpdateAt": cursor.LastPostUpdateAt}, sq.Gt{"Id": cursor.LastPostId}}}). - OrderBy("UpdateAt", "Id"). + Where(sq.Or{sq.Gt{"Posts.UpdateAt": cursor.LastPostUpdateAt}, sq.And{sq.Eq{"Posts.UpdateAt": cursor.LastPostUpdateAt}, sq.Gt{"Posts.Id": cursor.LastPostId}}}). + OrderBy("Posts.UpdateAt", "Id"). Limit(uint64(limit)) if options.ChannelId != "" { - query = query.Where(sq.Eq{"ChannelId": options.ChannelId}) + query = query.Where(sq.Eq{"Posts.ChannelId": options.ChannelId}) } if !options.IncludeDeleted { - query = query.Where(sq.Eq{"DeleteAt": 0}) + query = query.Where(sq.Eq{"Posts.DeleteAt": 0}) } if options.ExcludeRemoteId != "" { @@ -1402,7 +1405,7 @@ func (s *SqlPostStore) getPostsAround(before bool, options model.GetPostsOptions conditions := sq.And{ sq.Expr(`CreateAt `+direction+` (SELECT CreateAt FROM Posts WHERE Id = ?)`, options.PostId), sq.Eq{"p.ChannelId": options.ChannelId}, - sq.Eq{"DeleteAt": int(0)}, + sq.Eq{"p.DeleteAt": int(0)}, } if options.CollapsedThreads { conditions = append(conditions, sq.Eq{"RootId": ""}) @@ -1415,7 +1418,7 @@ func (s *SqlPostStore) getPostsAround(before bool, options model.GetPostsOptions // Adding ChannelId and DeleteAt order columns // to let mysql choose the "idx_posts_channel_id_delete_at_create_at" index always. // See MM-24170. - OrderBy("p.ChannelId", "DeleteAt", "CreateAt "+sort). + OrderBy("p.ChannelId", "p.DeleteAt", "p.CreateAt "+sort). Limit(uint64(options.PerPage)). Offset(uint64(offset)) @@ -1448,8 +1451,8 @@ func (s *SqlPostStore) getPostsAround(before bool, options model.GetPostsOptions rootQuery = rootQuery.From("Posts p"). Where(sq.And{ idQuery, - sq.Eq{"ChannelId": options.ChannelId}, - sq.Eq{"DeleteAt": 0}, + sq.Eq{"p.ChannelId": options.ChannelId}, + sq.Eq{"p.DeleteAt": 0}, }). OrderBy("CreateAt DESC") @@ -1505,11 +1508,11 @@ func (s *SqlPostStore) getPostIdAroundTime(channelId string, time int64, before conditions := sq.And{ direction, - sq.Eq{"ChannelId": channelId}, - sq.Eq{"DeleteAt": int(0)}, + sq.Eq{"Posts.ChannelId": channelId}, + sq.Eq{"Posts.DeleteAt": int(0)}, } if collapsedThreads { - conditions = sq.And{conditions, sq.Eq{"RootId": ""}} + conditions = sq.And{conditions, sq.Eq{"Posts.RootId": ""}} } query := s.getQueryBuilder(). Select("Id"). @@ -1518,7 +1521,7 @@ func (s *SqlPostStore) getPostIdAroundTime(channelId string, time int64, before // Adding ChannelId and DeleteAt order columns // to let mysql choose the "idx_posts_channel_id_delete_at_create_at" index always. // See MM-23369. - OrderBy("ChannelId", "DeleteAt", "CreateAt "+sort). + OrderBy("Posts.ChannelId", "Posts.DeleteAt", "Posts.CreateAt "+sort). Limit(1) queryString, args, err := query.ToSql() @@ -1545,9 +1548,9 @@ func (s *SqlPostStore) GetPostAfterTime(channelId string, time int64, collapsedT table += " USE INDEX(idx_posts_channel_id_delete_at_create_at)" } conditions := sq.And{ - sq.Gt{"CreateAt": time}, - sq.Eq{"ChannelId": channelId}, - sq.Eq{"DeleteAt": int(0)}, + sq.Gt{"Posts.CreateAt": time}, + sq.Eq{"Posts.ChannelId": channelId}, + sq.Eq{"Posts.DeleteAt": int(0)}, } if collapsedThreads { conditions = sq.And{conditions, sq.Eq{"RootId": ""}} @@ -1559,7 +1562,7 @@ func (s *SqlPostStore) GetPostAfterTime(channelId string, time int64, collapsedT // Adding ChannelId and DeleteAt order columns // to let mysql choose the "idx_posts_channel_id_delete_at_create_at" index always. // See MM-23369. - OrderBy("ChannelId", "DeleteAt", "CreateAt ASC"). + OrderBy("Posts.ChannelId", "Posts.DeleteAt", "Posts.CreateAt ASC"). Limit(1) queryString, args, err := query.ToSql() @@ -1581,9 +1584,9 @@ func (s *SqlPostStore) getRootPosts(channelId string, offset int, limit int, ski posts := []*model.Post{} var fetchQuery string if skipFetchThreads { - fetchQuery = "SELECT p.*, (SELECT COUNT(*) FROM Posts WHERE Posts.RootId = (CASE WHEN p.RootId = '' THEN p.Id ELSE p.RootId END) AND Posts.DeleteAt = 0) as ReplyCount FROM Posts p WHERE ChannelId = ? AND DeleteAt = 0 ORDER BY CreateAt DESC LIMIT ? OFFSET ?" + fetchQuery = "SELECT p.*, (SELECT COUNT(*) FROM Posts WHERE Posts.RootId = (CASE WHEN p.RootId = '' THEN p.Id ELSE p.RootId END) AND Posts.DeleteAt = 0) as ReplyCount FROM Posts p WHERE p.ChannelId = ? AND p.DeleteAt = 0 ORDER BY p.CreateAt DESC LIMIT ? OFFSET ?" } else { - fetchQuery = "SELECT * FROM Posts WHERE ChannelId = ? AND DeleteAt = 0 ORDER BY CreateAt DESC LIMIT ? OFFSET ?" + fetchQuery = "SELECT * FROM Posts WHERE Posts.ChannelId = ? AND Posts.DeleteAt = 0 ORDER BY Posts.CreateAt DESC LIMIT ? OFFSET ?" } err := s.GetReplicaX().Select(&posts, fetchQuery, channelId, limit, offset) if err != nil { @@ -1604,13 +1607,13 @@ func (s *SqlPostStore) getParentsPosts(channelId string, offset int, limit int, q.RootId FROM (SELECT - RootId + Posts.RootId FROM Posts WHERE - ChannelId = ? - AND DeleteAt = 0 - ORDER BY CreateAt DESC + Posts.ChannelId = ? + AND Posts.DeleteAt = 0 + ORDER BY Posts.CreateAt DESC LIMIT ? OFFSET ?) q WHERE q.RootId != ''` @@ -1639,10 +1642,10 @@ func (s *SqlPostStore) getParentsPosts(channelId string, offset int, limit int, From("Posts p"). Where(sq.And{ where, - sq.Eq{"ChannelId": channelId}, - sq.Eq{"DeleteAt": 0}, + sq.Eq{"p.ChannelId": channelId}, + sq.Eq{"p.DeleteAt": 0}, }). - OrderBy("CreateAt") + OrderBy("p.CreateAt") sql, args, err := query.ToSql() if err != nil { @@ -1675,20 +1678,20 @@ func (s *SqlPostStore) getParentsPostsPostgreSQL(channelId string, offset int, l q3.RootId FROM (SELECT - RootId + Posts.RootId FROM Posts WHERE - ChannelId = ? - AND DeleteAt = 0 - ORDER BY CreateAt DESC + Posts.ChannelId = ? + AND Posts.DeleteAt = 0 + ORDER BY Posts.CreateAt DESC LIMIT ? OFFSET ?) q3 WHERE q3.RootId != '') q1 ON `+onStatement+` WHERE - ChannelId = ? - AND DeleteAt = 0 - ORDER BY CreateAt`, channelId, limit, offset, channelId) + q2.ChannelId = ? + AND q2.DeleteAt = 0 + ORDER BY q2.CreateAt`, channelId, limit, offset, channelId) if err != nil { return nil, errors.Wrapf(err, "failed to find Posts with channelId=%s", channelId) } @@ -1835,9 +1838,9 @@ func (s *SqlPostStore) search(teamId string, userId string, params *model.Search "*", "(SELECT COUNT(*) FROM Posts WHERE Posts.RootId = (CASE WHEN q2.RootId = '' THEN q2.Id ELSE q2.RootId END) AND Posts.DeleteAt = 0) as ReplyCount", ).From("Posts q2"). - Where("DeleteAt = 0"). - Where(fmt.Sprintf("Type NOT LIKE '%s%%'", model.PostSystemMessagePrefix)). - OrderByClause("CreateAt DESC"). + Where("q2.DeleteAt = 0"). + Where(fmt.Sprintf("q2.Type NOT LIKE '%s%%'", model.PostSystemMessagePrefix)). + OrderByClause("q2.CreateAt DESC"). Limit(100) var err error @@ -1928,11 +1931,11 @@ func (s *SqlPostStore) search(teamId string, userId string, params *model.Search Where("Id = ChannelId") if !params.IncludeDeletedChannels { - inQuery = inQuery.Where("DeleteAt = 0") + inQuery = inQuery.Where("Channels.DeleteAt = 0") } if !params.SearchWithoutUserId { - inQuery = inQuery.Where("UserId = ?", userId) + inQuery = inQuery.Where("ChannelMembers.UserId = ?", userId) } inQuery = s.buildSearchTeamFilterClause(teamId, inQuery) @@ -2378,10 +2381,10 @@ func (s *SqlPostStore) GetParentsForExportAfter(limit int, afterId string) ([]*m FROM Posts WHERE - Id > ? - AND RootId = '' - AND DeleteAt = 0 - ORDER BY Id + Posts.Id > ? + AND Posts.RootId = '' + AND Posts.DeleteAt = 0 + ORDER BY Posts.Id LIMIT ?`, afterId, limit) if err != nil { @@ -2395,7 +2398,7 @@ func (s *SqlPostStore) GetParentsForExportAfter(limit int, afterId string) ([]*m builder := s.getQueryBuilder(). Select("p1.*, Users.Username as Username, Teams.Name as TeamName, Channels.Name as ChannelName"). - FromSelect(sq.Select("*").From("Posts").Where(sq.Eq{"Id": rootIds}), "p1"). + FromSelect(sq.Select("*").From("Posts").Where(sq.Eq{"Posts.Id": rootIds}), "p1"). InnerJoin("Channels ON p1.ChannelId = Channels.Id"). InnerJoin("Teams ON Channels.TeamId = Teams.Id"). InnerJoin("Users ON p1.UserId = Users.Id"). @@ -2598,19 +2601,36 @@ func (s *SqlPostStore) permanentDeleteThreads(transaction *sqlxTxWrapper, postId return nil } -// Thread cleanup upon post deletion -// if the post is a comment -// reply count is reduced by 1 and, -// the user is removed from participants if the comment deleted is the last reply from said user. -func (s *SqlPostStore) cleanupThreadComments(transaction *sqlxTxWrapper, postId, rootId string, userId string) error { +// deleteThread marks a thread as deleted at the given time. +func (s *SqlPostStore) deleteThread(transaction *sqlxTxWrapper, postId string, deleteAtTime int64) error { + queryString, args, err := s.getQueryBuilder(). + Update("Threads"). + Set("DeleteAt", deleteAtTime). + Where(sq.Eq{"PostId": postId}). + ToSql() + if err != nil { + return errors.Wrapf(err, "failed to create SQL query to mark thread for root post %s as deleted", postId) + } + + _, err = transaction.Exec(queryString, args...) + if err != nil { + return errors.Wrapf(err, "failed to mark thread for root post %s as deleted", postId) + } + + return nil +} + +// updateThreadAfterReplyDeletion decrements the thread reply count and adjusts the participants +// list as necessary. +func (s *SqlPostStore) updateThreadAfterReplyDeletion(transaction *sqlxTxWrapper, rootId string, userId string) error { if rootId != "" { queryString, args, err := s.getQueryBuilder(). - Select("COUNT(Id)"). + Select("COUNT(Posts.Id)"). From("Posts"). Where(sq.And{ - sq.Eq{"RootId": rootId}, - sq.Eq{"UserId": userId}, - sq.Eq{"DeleteAt": 0}, + sq.Eq{"Posts.RootId": rootId}, + sq.Eq{"Posts.UserId": userId}, + sq.Eq{"Posts.DeleteAt": 0}, }). ToSql() @@ -2675,9 +2695,16 @@ func (s *SqlPostStore) updateThreadsFromPosts(transaction *sqlxTxWrapper, posts return nil } threadsByRootsSql, threadsByRootsArgs, _ := s.getQueryBuilder(). - Select("*"). + Select( + "Threads.PostId", + "Threads.ChannelId", + "Threads.ReplyCount", + "Threads.LastReplyAt", + "Threads.Participants", + "COALESCE(Threads.DeleteAt, 0) AS DeleteAt", + ). From("Threads"). - Where(sq.Eq{"PostId": rootIds}). + Where(sq.Eq{"Threads.PostId": rootIds}). ToSql() threadsByRoots := []*model.Thread{} if err := transaction.Select(&threadsByRoots, threadsByRootsSql, threadsByRootsArgs...); err != nil { @@ -2697,7 +2724,7 @@ func (s *SqlPostStore) updateThreadsFromPosts(transaction *sqlxTxWrapper, posts }{} // calculate participants - if err := transaction.Select(&data, "SELECT UserId, MAX(CreateAt) as RepliedAt FROM Posts WHERE RootId=? AND DeleteAt=0 GROUP BY UserId ORDER BY RepliedAt ASC", rootId); err != nil { + if err := transaction.Select(&data, "SELECT Posts.UserId, MAX(Posts.CreateAt) as RepliedAt FROM Posts WHERE Posts.RootId=? AND Posts.DeleteAt=0 GROUP BY Posts.UserId ORDER BY RepliedAt ASC", rootId); err != nil { return err } @@ -2708,13 +2735,13 @@ func (s *SqlPostStore) updateThreadsFromPosts(transaction *sqlxTxWrapper, posts // calculate reply count var count int64 - err := transaction.Get(&count, "SELECT COUNT(Id) FROM Posts WHERE RootId=? And DeleteAt=0", rootId) + err := transaction.Get(&count, "SELECT COUNT(Posts.Id) FROM Posts WHERE Posts.RootId=? And Posts.DeleteAt=0", rootId) if err != nil { return err } // calculate last reply at var lastReplyAt int64 - err = transaction.Get(&lastReplyAt, "SELECT COALESCE(MAX(Posts.CreateAt), 0) FROM Posts WHERE RootID=? and DeleteAt=0", rootId) + err = transaction.Get(&lastReplyAt, "SELECT COALESCE(MAX(Posts.CreateAt), 0) FROM Posts WHERE Posts.RootID=? and Posts.DeleteAt=0", rootId) if err != nil { return err } diff --git a/store/sqlstore/thread_store.go b/store/sqlstore/thread_store.go index bd6511cfea..6b8c417607 100644 --- a/store/sqlstore/thread_store.go +++ b/store/sqlstore/thread_store.go @@ -20,28 +20,63 @@ import ( type SqlThreadStore struct { *SqlStore + + // threadsSelectQuery is for querying directly into model.Thread + threadsSelectQuery sq.SelectBuilder + + // threadsAndPostsSelectQuery is for querying into a struct embedding fields from + // model.Thread and model.Post. + threadsAndPostsSelectQuery sq.SelectBuilder } func (s *SqlThreadStore) ClearCaches() { } func newSqlThreadStore(sqlStore *SqlStore) store.ThreadStore { - return &SqlThreadStore{ + s := SqlThreadStore{ SqlStore: sqlStore, } + + s.initializeQueries() + + return &s +} + +func (s *SqlThreadStore) initializeQueries() { + s.threadsSelectQuery = s.getQueryBuilder(). + Select( + "Threads.PostId", + "Threads.ChannelId", + "Threads.ReplyCount", + "Threads.LastReplyAt", + "Threads.Participants", + "COALESCE(Threads.DeleteAt, 0) AS DeleteAt", + ). + From("Threads") + + s.threadsAndPostsSelectQuery = s.getQueryBuilder(). + Select( + "Threads.PostId", + "Threads.ChannelId", + "Threads.ReplyCount", + "Threads.LastReplyAt", + "Threads.Participants", + "COALESCE(Threads.DeleteAt, 0) AS ThreadDeleteAt", + ). + From("Threads") } func (s *SqlThreadStore) Get(id string) (*model.Thread, error) { var thread model.Thread - query, args, err := s.getQueryBuilder(). - Select("*"). - From("Threads"). + + query, args, err := s.threadsSelectQuery. Where(sq.Eq{"PostId": id}). ToSql() if err != nil { return nil, errors.Wrap(err, "thread_tosql") } - err = s.GetMasterX().Get(&thread, query, args...) + + err = s.GetReplicaX().Get(&thread, query, args...) if err != nil { if err == sql.ErrNoRows { return nil, nil @@ -52,55 +87,7 @@ func (s *SqlThreadStore) Get(id string) (*model.Thread, error) { return &thread, nil } -// GetTotalUnreadThreads counts the number of unread threads for the given user, optionally -// constrained to the given team + DMs/GMs. -func (s *SqlThreadStore) GetTotalUnreadThreads(userId, teamId string, opts model.GetUserThreadsOpts) (int64, error) { - var totalUnreadThreads int64 - - query := s.getQueryBuilder(). - Select("COUNT(DISTINCT(Posts.RootId))"). - From("Posts"). - LeftJoin("ThreadMemberships ON Posts.RootId = ThreadMemberships.PostId"). - Where("Posts.CreateAt > ThreadMemberships.LastViewed"). - Where(sq.Eq{ - "ThreadMemberships.UserId": userId, - "ThreadMemberships.Following": true, - }) - - if teamId != "" { - query = query. - LeftJoin("Channels ON Posts.ChannelId = Channels.Id"). - Where(sq.Or{ - sq.Eq{"Channels.TeamId": teamId}, - sq.Eq{"Channels.TeamId": ""}, - }) - } - - if !opts.Deleted { - query = query.Where(sq.Eq{"COALESCE(Posts.DeleteAt, 0)": 0}) - } - - sql, args, err := query.ToSql() - if err != nil { - return 0, errors.Wrapf(err, "failed to build query to count unread threads for user id=%s", userId) - } - - err = s.GetMasterX().Get(&totalUnreadThreads, sql, args...) - if err != nil { - return 0, errors.Wrapf(err, "failed to count unread threads for user id=%s", userId) - } - - return totalUnreadThreads, nil -} - -// GetTotalUnreadThreads counts the number of threads for the given user, optionally constrained -// to the given team + DMs/GMs. -// -// TODO: Why do we support an Unread flag here? It's basically the same as GetTotalUnreadThreads, -// but with different comparison semantics. -func (s *SqlThreadStore) GetTotalThreads(userId, teamId string, opts model.GetUserThreadsOpts) (int64, error) { - var totalCount int64 - +func (s *SqlThreadStore) getTotalThreadsQuery(userId, teamId string, opts model.GetUserThreadsOpts) sq.SelectBuilder { query := s.getQueryBuilder(). Select("COUNT(ThreadMemberships.PostId)"). From("ThreadMemberships"). @@ -120,27 +107,53 @@ func (s *SqlThreadStore) GetTotalThreads(userId, teamId string, opts model.GetUs } if !opts.Deleted { - query = query. - LeftJoin("Posts ON Posts.Id = ThreadMemberships.PostId"). - Where(sq.Eq{"COALESCE(Posts.DeleteAt, 0)": 0}) + query = query.Where(sq.Eq{"COALESCE(Threads.DeleteAt, 0)": 0}) } - if opts.Unread { - query = query. - Where(sq.Expr("ThreadMemberships.LastViewed < Threads.LastReplyAt")) + return query +} + +// GetTotalUnreadThreads counts the number of unread threads for the given user, optionally +// constrained to the given team + DMs/GMs. +func (s *SqlThreadStore) GetTotalUnreadThreads(userId, teamId string, opts model.GetUserThreadsOpts) (int64, error) { + query := s.getTotalThreadsQuery(userId, teamId, opts). + Where(sq.Expr("ThreadMemberships.LastViewed < Threads.LastReplyAt")) + + sql, args, err := query.ToSql() + if err != nil { + return 0, errors.Wrapf(err, "failed to build query to count unread threads for user id=%s", userId) } + var totalUnreadThreads int64 + err = s.GetReplicaX().Get(&totalUnreadThreads, sql, args...) + if err != nil { + return 0, errors.Wrapf(err, "failed to count unread threads for user id=%s", userId) + } + + return totalUnreadThreads, nil +} + +// GetTotalUnreadThreads counts the number of threads for the given user, optionally constrained +// to the given team + DMs/GMs. +func (s *SqlThreadStore) GetTotalThreads(userId, teamId string, opts model.GetUserThreadsOpts) (int64, error) { + if opts.Unread { + return 0, errors.New("GetTotalThreads does not support the Unread flag; use GetTotalUnreadThreads instead") + } + + query := s.getTotalThreadsQuery(userId, teamId, opts) + sql, args, err := query.ToSql() if err != nil { return 0, errors.Wrapf(err, "failed to build query to count threads for user id=%s", userId) } - err = s.GetMasterX().Get(&totalCount, sql, args...) + var totalThreads int64 + err = s.GetReplicaX().Get(&totalThreads, sql, args...) if err != nil { return 0, errors.Wrapf(err, "failed to count threads for user id=%s", userId) } - return totalCount, nil + return totalThreads, nil } // GetTotalUnreadMentions counts the number of unread mentions for the given user, optionally @@ -151,6 +164,7 @@ func (s *SqlThreadStore) GetTotalUnreadMentions(userId, teamId string, opts mode query := s.getQueryBuilder(). Select("COALESCE(SUM(ThreadMemberships.UnreadMentions),0)"). From("ThreadMemberships"). + LeftJoin("Threads ON Threads.PostId = ThreadMemberships.PostId"). Where(sq.Eq{ "ThreadMemberships.UserId": userId, "ThreadMemberships.Following": true, @@ -158,7 +172,6 @@ func (s *SqlThreadStore) GetTotalUnreadMentions(userId, teamId string, opts mode if teamId != "" { query = query. - LeftJoin("Threads ON Threads.PostId = ThreadMemberships.PostId"). LeftJoin("Channels ON Threads.ChannelId = Channels.Id"). Where(sq.Or{ sq.Eq{"Channels.TeamId": teamId}, @@ -167,9 +180,7 @@ func (s *SqlThreadStore) GetTotalUnreadMentions(userId, teamId string, opts mode } if !opts.Deleted { - query = query. - LeftJoin("Posts ON Posts.Id = ThreadMemberships.PostId"). - Where(sq.Eq{"COALESCE(Posts.DeleteAt, 0)": 0}) + query = query.Where(sq.Eq{"COALESCE(Threads.DeleteAt, 0)": 0}) } sql, args, err := query.ToSql() @@ -177,7 +188,7 @@ func (s *SqlThreadStore) GetTotalUnreadMentions(userId, teamId string, opts mode return 0, errors.Wrapf(err, "failed to build query to count unread mentions for user id=%s", userId) } - err = s.GetMasterX().Get(&totalUnreadMentions, sql, args...) + err = s.GetReplicaX().Get(&totalUnreadMentions, sql, args...) if err != nil { return 0, errors.Wrapf(err, "failed to count unread mentions for user id=%s", userId) } @@ -199,6 +210,7 @@ func (s *SqlThreadStore) GetThreadsForUser(userId, teamId string, opts model.Get UnreadReplies int64 UnreadMentions int64 Participants model.StringArray + ThreadDeleteAt int64 model.Post } @@ -217,12 +229,12 @@ func (s *SqlThreadStore) GetThreadsForUser(userId, teamId string, opts model.Get return nil, errors.Wrapf(err, "failed to build subquery to count unread replies when getting threads for user id=%s", userId) } - query := s.getQueryBuilder(). - Select(`Threads.*, - ` + postSliceCoalesceQuery() + `, - ThreadMemberships.LastViewed as LastViewedAt, - ThreadMemberships.UnreadMentions as UnreadMentions`). - From("Threads"). + query := s.threadsAndPostsSelectQuery. + Column(postSliceCoalesceQuery()). + Columns( + "ThreadMemberships.LastViewed as LastViewedAt", + "ThreadMemberships.UnreadMentions as UnreadMentions", + ). Column(sq.Alias(sq.Expr(unreadRepliesSql, unreadRepliesArgs...), "UnreadReplies")). Join("Posts ON Posts.Id = Threads.PostId"). Join("ThreadMemberships ON ThreadMemberships.PostId = Threads.PostId") @@ -235,7 +247,7 @@ func (s *SqlThreadStore) GetThreadsForUser(userId, teamId string, opts model.Get // a team at all. if teamId != "" { query = query. - Join("Channels ON Posts.ChannelId = Channels.Id"). + Join("Channels ON Threads.ChannelId = Channels.Id"). Where(sq.Or{ sq.Eq{"Channels.TeamId": teamId}, sq.Eq{"Channels.TeamId": ""}, @@ -243,7 +255,10 @@ func (s *SqlThreadStore) GetThreadsForUser(userId, teamId string, opts model.Get } if !opts.Deleted { - query = query.Where(sq.Eq{"Posts.DeleteAt": 0}) + query = query.Where(sq.Or{ + sq.Eq{"Threads.DeleteAt": nil}, + sq.Eq{"Threads.DeleteAt": 0}, + }) } if opts.Since > 0 { @@ -256,11 +271,11 @@ func (s *SqlThreadStore) GetThreadsForUser(userId, teamId string, opts model.Get order := "DESC" if opts.Before != "" { - query = query.Where(sq.Expr(`LastReplyAt < (SELECT LastReplyAt FROM Threads WHERE PostId = ?)`, opts.Before)) + query = query.Where(sq.Expr(`Threads.LastReplyAt < (SELECT LastReplyAt FROM Threads WHERE PostId = ?)`, opts.Before)) } if opts.After != "" { order = "ASC" - query = query.Where(sq.Expr(`LastReplyAt > (SELECT LastReplyAt FROM Threads WHERE PostId = ?)`, opts.After)) + query = query.Where(sq.Expr(`Threads.LastReplyAt > (SELECT LastReplyAt FROM Threads WHERE PostId = ?)`, opts.After)) } query = query. @@ -323,6 +338,7 @@ func (s *SqlThreadStore) GetThreadsForUser(userId, teamId string, opts model.Get UnreadMentions: thread.UnreadMentions, Participants: threadParticipants, Post: thread.Post.ToNilIfInvalid(), + DeleteAt: thread.ThreadDeleteAt, }) } @@ -336,7 +352,7 @@ func (s *SqlThreadStore) GetTeamsUnreadForUser(userID string, teamIDs []string) sq.Eq{"ThreadMemberships.UserId": userID}, sq.Eq{"ThreadMemberships.Following": true}, sq.Eq{"Channels.TeamId": teamIDs}, - sq.Eq{"COALESCE(Posts.DeleteAt, 0)": 0}, + sq.Eq{"COALESCE(Threads.DeleteAt, 0)": 0}, } var wg sync.WaitGroup @@ -362,7 +378,6 @@ func (s *SqlThreadStore) GetTeamsUnreadForUser(userID string, teamIDs []string) From("Threads"). LeftJoin("ThreadMemberships ON Threads.PostId = ThreadMemberships.PostId"). LeftJoin("Channels ON Threads.ChannelId = Channels.Id"). - LeftJoin("Posts ON Posts.Id = Threads.PostId"). Where(fetchConditions). Where("Threads.LastReplyAt > ThreadMemberships.LastViewed"). GroupBy("Channels.TeamId"). @@ -385,7 +400,6 @@ func (s *SqlThreadStore) GetTeamsUnreadForUser(userID string, teamIDs []string) Select("COALESCE(SUM(ThreadMemberships.UnreadMentions),0) AS Count, TeamId"). From("ThreadMemberships"). LeftJoin("Threads ON Threads.PostId = ThreadMemberships.PostId"). - LeftJoin("Posts ON Posts.Id = ThreadMemberships.PostId"). LeftJoin("Channels ON Threads.ChannelId = Channels.Id"). Where(fetchConditions). GroupBy("Channels.TeamId"). @@ -476,6 +490,7 @@ func (s *SqlThreadStore) GetThreadForUser(teamId string, threadMembership *model UnreadReplies int64 UnreadMentions int64 Participants model.StringArray + ThreadDeleteAt int64 model.Post } @@ -496,21 +511,26 @@ func (s *SqlThreadStore) GetThreadForUser(teamId string, threadMembership *model sq.Eq{"Threads.PostId": threadMembership.PostId}, } + query := s.threadsAndPostsSelectQuery + + for _, c := range postSliceColumns() { + query = query.Column("Posts." + c) + } + var thread JoinedThread - query, threadArgs, err := s.getQueryBuilder(). - Select("Threads.*, Posts.*"). - From("Threads"). + querySQL, threadArgs, err := query. Column(sq.Alias(sq.Expr(unreadRepliesQuery), "UnreadReplies")). LeftJoin("Posts ON Posts.Id = Threads.PostId"). LeftJoin("Channels ON Posts.ChannelId = Channels.Id"). - Where(fetchConditions).ToSql() + Where(fetchConditions). + ToSql() if err != nil { return nil, errors.Wrapf(err, "failed to build query to get thread for user id=%s, post id=%s", threadMembership.UserId, threadMembership.PostId) } args := append(unreadRepliesArgs, threadArgs...) - err = s.GetReplicaX().Get(&thread, query, args...) + err = s.GetReplicaX().Get(&thread, querySQL, args...) if err != nil { if err == sql.ErrNoRows { return nil, store.NewErrNotFound("Thread", threadMembership.PostId) @@ -557,6 +577,7 @@ func (s *SqlThreadStore) GetThreadForUser(teamId string, threadMembership *model UnreadMentions: thread.UnreadMentions, Participants: participants, Post: thread.Post.ToNilIfInvalid(), + DeleteAt: thread.ThreadDeleteAt, } return result, nil diff --git a/store/storetest/post_store.go b/store/storetest/post_store.go index 0ea40fc5c0..eb10ac9b91 100644 --- a/store/storetest/post_store.go +++ b/store/storetest/post_store.go @@ -27,8 +27,6 @@ func TestPostStore(t *testing.T, ss store.Store, s SqlStore) { t.Run("GetSingle", func(t *testing.T) { testPostStoreGetSingle(t, ss) }) t.Run("Update", func(t *testing.T) { testPostStoreUpdate(t, ss) }) t.Run("Delete", func(t *testing.T) { testPostStoreDelete(t, ss) }) - t.Run("Delete1Level", func(t *testing.T) { testPostStoreDelete1Level(t, ss) }) - t.Run("Delete2Level", func(t *testing.T) { testPostStoreDelete2Level(t, ss) }) t.Run("PermDelete1Level", func(t *testing.T) { testPostStorePermDelete1Level(t, ss) }) t.Run("PermDelete1Level2", func(t *testing.T) { testPostStorePermDelete1Level2(t, ss) }) t.Run("GetWithChildren", func(t *testing.T) { testPostStoreGetWithChildren(t, ss) }) @@ -317,6 +315,9 @@ func testPostStoreSaveMultiple(t *testing.T, ss store.Store) { replyPost3.Message = NewTestId() replyPost3.RootId = rootPost.Id + // Ensure update does not occur in the same timestamp as creation + time.Sleep(time.Millisecond) + _, _, err = ss.Post().SaveMultiple([]*model.Post{&replyPost2, &replyPost3}) require.NoError(t, err) @@ -819,109 +820,132 @@ func testPostStoreUpdate(t *testing.T, ss store.Store) { } func testPostStoreDelete(t *testing.T, ss store.Store) { - o1 := &model.Post{} - o1.ChannelId = model.NewId() - o1.UserId = model.NewId() - o1.Message = model.NewRandomString(10) - deleteByID := model.NewId() + t.Run("single post, no replies", func(t *testing.T) { + // Create a post + rootPost, err := ss.Post().Save(&model.Post{ + ChannelId: model.NewId(), + UserId: model.NewId(), + Message: model.NewRandomString(10), + }) + require.NoError(t, err) - etag1 := ss.Post().GetEtag(o1.ChannelId, false, false) - require.Equal(t, 0, strings.Index(etag1, model.CurrentVersion+"."), "Invalid Etag") + // Verify etag generation for the channel containing the post. + etag1 := ss.Post().GetEtag(rootPost.ChannelId, false, false) + require.Equal(t, 0, strings.Index(etag1, model.CurrentVersion+"."), "Invalid Etag") - o1, err := ss.Post().Save(o1) - require.NoError(t, err) + // Verify the created post. + r1, err := ss.Post().Get(context.Background(), rootPost.Id, model.GetPostsOptions{}, "") + require.NoError(t, err) + require.NotNil(t, r1.Posts[rootPost.Id]) + require.Equal(t, rootPost, r1.Posts[rootPost.Id]) - r1, err := ss.Post().Get(context.Background(), o1.Id, model.GetPostsOptions{}, "") - require.NoError(t, err) - require.Equal(t, r1.Posts[o1.Id].CreateAt, o1.CreateAt, "invalid returned post") + // Mark the post as deleted by the user identified with deleteByID. + deleteByID := model.NewId() + err = ss.Post().Delete(rootPost.Id, model.GetMillis(), deleteByID) + require.NoError(t, err) - err = ss.Post().Delete(o1.Id, model.GetMillis(), deleteByID) - require.NoError(t, err) + // Ensure the appropriate posts prop reflects the user deleting the post. + posts, err := ss.Post().GetPostsCreatedAt(rootPost.ChannelId, rootPost.CreateAt) + require.NoError(t, err) + require.NotEmpty(t, posts) + assert.Equal(t, deleteByID, posts[0].GetProp(model.PostPropsDeleteBy), "unexpected Props[model.PostPropsDeleteBy]") - posts, _ := ss.Post().GetPostsCreatedAt(o1.ChannelId, o1.CreateAt) - post := posts[0] - actual := post.GetProp(model.PostPropsDeleteBy) + // Verify that the post is no longer fetched by default. + _, err = ss.Post().Get(context.Background(), rootPost.Id, model.GetPostsOptions{}, "") + require.Error(t, err, "fetching deleted post should have failed") + require.IsType(t, &store.ErrNotFound{}, err) - assert.Equal(t, deleteByID, actual, "Expected (*Post).Props[model.PostPropsDeleteBy] to be %v but got %v.", deleteByID, actual) + // Verify etag generation for the channel containing the now deleted post. + etag2 := ss.Post().GetEtag(rootPost.ChannelId, false, false) + require.Equal(t, 0, strings.Index(etag2, model.CurrentVersion+"."), "Invalid Etag") + }) - r3, err := ss.Post().Get(context.Background(), o1.Id, model.GetPostsOptions{}, "") - require.Error(t, err, "Missing id should have failed - PostList %v", r3) + t.Run("thread with one reply", func(t *testing.T) { + // Create a root post + rootPost, err := ss.Post().Save(&model.Post{ + ChannelId: model.NewId(), + UserId: model.NewId(), + Message: NewTestId(), + }) + require.NoError(t, err) - etag2 := ss.Post().GetEtag(o1.ChannelId, false, false) - require.Equal(t, 0, strings.Index(etag2, model.CurrentVersion+"."), "Invalid Etag") -} + // Reply to that root post + replyPost, err := ss.Post().Save(&model.Post{ + ChannelId: rootPost.ChannelId, + UserId: model.NewId(), + Message: NewTestId(), + RootId: rootPost.Id, + }) + require.NoError(t, err) -func testPostStoreDelete1Level(t *testing.T, ss store.Store) { - o1 := &model.Post{} - o1.ChannelId = model.NewId() - o1.UserId = model.NewId() - o1.Message = NewTestId() - o1, err := ss.Post().Save(o1) - require.NoError(t, err) + // Delete the root post + err = ss.Post().Delete(rootPost.Id, model.GetMillis(), "") + require.NoError(t, err) - o2 := &model.Post{} - o2.ChannelId = o1.ChannelId - o2.UserId = model.NewId() - o2.Message = NewTestId() - o2.RootId = o1.Id - o2, err = ss.Post().Save(o2) - require.NoError(t, err) + // Verify the root post deleted + _, err = ss.Post().Get(context.Background(), rootPost.Id, model.GetPostsOptions{}, "") + require.Error(t, err, "Deleted id should have failed") + require.IsType(t, &store.ErrNotFound{}, err) - err = ss.Post().Delete(o1.Id, model.GetMillis(), "") - require.NoError(t, err) + // Verify the reply post deleted + _, err = ss.Post().Get(context.Background(), replyPost.Id, model.GetPostsOptions{}, "") + require.Error(t, err, "Deleted id should have failed") + require.IsType(t, &store.ErrNotFound{}, err) + }) - _, err = ss.Post().Get(context.Background(), o1.Id, model.GetPostsOptions{}, "") - require.Error(t, err, "Deleted id should have failed") + t.Run("thread with multiple replies", func(t *testing.T) { + // Create a root post + rootPost1, err := ss.Post().Save(&model.Post{ + ChannelId: model.NewId(), + UserId: model.NewId(), + Message: NewTestId(), + }) + require.NoError(t, err) - _, err = ss.Post().Get(context.Background(), o2.Id, model.GetPostsOptions{}, "") - require.Error(t, err, "Deleted id should have failed") -} + // Reply to that root post + replyPost1, err := ss.Post().Save(&model.Post{ + ChannelId: rootPost1.ChannelId, + UserId: model.NewId(), + Message: NewTestId(), + RootId: rootPost1.Id, + }) + require.NoError(t, err) -func testPostStoreDelete2Level(t *testing.T, ss store.Store) { - o1 := &model.Post{} - o1.ChannelId = model.NewId() - o1.UserId = model.NewId() - o1.Message = NewTestId() - o1, err := ss.Post().Save(o1) - require.NoError(t, err) + // Reply to that root post a second time + replyPost2, err := ss.Post().Save(&model.Post{ + ChannelId: rootPost1.ChannelId, + UserId: model.NewId(), + Message: NewTestId(), + RootId: rootPost1.Id, + }) + require.NoError(t, err) - o2 := &model.Post{} - o2.ChannelId = o1.ChannelId - o2.UserId = model.NewId() - o2.Message = NewTestId() - o2.RootId = o1.Id - o2, err = ss.Post().Save(o2) - require.NoError(t, err) + // Create another root post in a separate channel + rootPost2, err := ss.Post().Save(&model.Post{ + ChannelId: model.NewId(), + UserId: model.NewId(), + Message: NewTestId(), + }) + require.NoError(t, err) - o3 := &model.Post{} - o3.ChannelId = o1.ChannelId - o3.UserId = model.NewId() - o3.Message = NewTestId() - o3.RootId = o1.Id - o3, err = ss.Post().Save(o3) - require.NoError(t, err) + // Delete the root post + err = ss.Post().Delete(rootPost1.Id, model.GetMillis(), "") + require.NoError(t, err) - o4 := &model.Post{} - o4.ChannelId = model.NewId() - o4.UserId = model.NewId() - o4.Message = NewTestId() - o4, err = ss.Post().Save(o4) - require.NoError(t, err) + // Verify the root post and replies deleted + _, err = ss.Post().Get(context.Background(), rootPost1.Id, model.GetPostsOptions{}, "") + require.Error(t, err, "Deleted id should have failed") - err = ss.Post().Delete(o1.Id, model.GetMillis(), "") - require.NoError(t, err) + _, err = ss.Post().Get(context.Background(), replyPost1.Id, model.GetPostsOptions{}, "") + require.Error(t, err, "Deleted id should have failed") - _, err = ss.Post().Get(context.Background(), o1.Id, model.GetPostsOptions{}, "") - require.Error(t, err, "Deleted id should have failed") + _, err = ss.Post().Get(context.Background(), replyPost2.Id, model.GetPostsOptions{}, "") + require.Error(t, err, "Deleted id should have failed") - _, err = ss.Post().Get(context.Background(), o2.Id, model.GetPostsOptions{}, "") - require.Error(t, err, "Deleted id should have failed") - - _, err = ss.Post().Get(context.Background(), o3.Id, model.GetPostsOptions{}, "") - require.Error(t, err, "Deleted id should have failed") - - _, err = ss.Post().Get(context.Background(), o4.Id, model.GetPostsOptions{}, "") - require.NoError(t, err) + // Verify other root posts remain undeleted. + _, err = ss.Post().Get(context.Background(), rootPost2.Id, model.GetPostsOptions{}, "") + require.NoError(t, err) + }) } func testPostStorePermDelete1Level(t *testing.T, ss store.Store) { diff --git a/store/storetest/thread_store.go b/store/storetest/thread_store.go index d2c32fe3cb..091f7a48d5 100644 --- a/store/storetest/thread_store.go +++ b/store/storetest/thread_store.go @@ -684,6 +684,12 @@ func testGetTeamsUnreadForUser(t *testing.T, ss store.Store) { assert.Equal(t, int64(1), teamsUnread[team2.Id].ThreadMentionCount) } +type byPostId []*model.Post + +func (a byPostId) Len() int { return len(a) } +func (a byPostId) Swap(i, j int) { a[i], a[j] = a[j], a[i] } +func (a byPostId) Less(i, j int) bool { return a[i].Id < a[j].Id } + func testVarious(t *testing.T, ss store.Store) { createThreadMembership := func(userID, postID string, isMention bool) { t.Helper() @@ -818,6 +824,16 @@ func testVarious(t *testing.T, ss store.Store) { }) require.NoError(t, err) + postNames := map[string]string{ + team1channel1post1.Id: "team1channel1post1", + team1channel1post2.Id: "team1channel1post2", + team1channel1post3.Id: "team1channel1post3", + team2channel1post1.Id: "team2channel1post1", + team2channel1post2deleted.Id: "team2channel1post2deleted", + dm1post1.Id: "dm1post1", + gm1post1.Id: "gm1post1", + } + threadStoreCreateReply(t, ss, team1channel1.Id, team1channel1post1.Id, user2ID, model.GetMillis()) threadStoreCreateReply(t, ss, team1channel1.Id, team1channel1post2.Id, user2ID, model.GetMillis()) threadStoreCreateReply(t, ss, team1channel1.Id, team1channel1post3.Id, user2ID, model.GetMillis()) @@ -845,9 +861,37 @@ func testVarious(t *testing.T, ss store.Store) { time.Sleep(1 * time.Millisecond) threadStoreCreateReply(t, ss, team1channel1.Id, team1channel1post2.Id, user2ID, model.GetMillis()) + // Actually make team2channel1post2deleted deleted err = ss.Post().Delete(team2channel1post2deleted.Id, model.GetMillis(), user1ID) require.NoError(t, err) + // Re-fetch posts to ensure metadata up-to-date + allPosts := []*model.Post{ + team1channel1post1, + team1channel1post2, + team1channel1post3, + team2channel1post1, + team2channel1post2deleted, + dm1post1, + gm1post1, + } + for i := range allPosts { + updatedPost, err := ss.Post().GetSingle(allPosts[i].Id, true) + require.NoError(t, err) + + // Fix some inconsistencies with how the post store returns posts vs. how the + // thread store returns it. + if updatedPost.RemoteId == nil { + updatedPost.RemoteId = new(string) + } + + // Also, we don't populate ReplyCount for posts when querying threads, so don't + // assert same. + updatedPost.ReplyCount = 0 + + updatedPost.ShallowCopy(allPosts[i]) + } + t.Run("GetTotalUnreadThreads", func(t *testing.T) { testCases := []struct { Description string @@ -899,27 +943,15 @@ func testVarious(t *testing.T, ss store.Store) { {"team1, user1", user1ID, team1.Id, model.GetUserThreadsOpts{}, []*model.Post{ team1channel1post1, team1channel1post2, team1channel1post3, dm1post1, gm1post1, }}, - {"team1, user1, unread", user1ID, team1.Id, model.GetUserThreadsOpts{Unread: true}, []*model.Post{ - team1channel1post2, team1channel1post3, gm1post1, - }}, {"team1, user1, deleted", user1ID, team1.Id, model.GetUserThreadsOpts{Deleted: true}, []*model.Post{ team1channel1post1, team1channel1post2, team1channel1post3, dm1post1, gm1post1, // (no deleted threads in team1) }}, - {"team1, user1, unread + deleted", user1ID, team1.Id, model.GetUserThreadsOpts{Unread: true, Deleted: true}, []*model.Post{ - team1channel1post2, team1channel1post3, gm1post1, // (no deleted threads in team1) - }}, {"team2, user1", user1ID, team2.Id, model.GetUserThreadsOpts{}, []*model.Post{ team2channel1post1, dm1post1, gm1post1, }}, - {"team2, user1, unread", user1ID, team2.Id, model.GetUserThreadsOpts{Unread: true}, []*model.Post{ - gm1post1, // (no unread in team2) - }}, {"team2, user1, deleted", user1ID, team2.Id, model.GetUserThreadsOpts{Deleted: true}, []*model.Post{ team2channel1post1, team2channel1post2deleted, dm1post1, gm1post1, }}, - {"team2, user1, unread + deleted", user1ID, team2.Id, model.GetUserThreadsOpts{Unread: true, Deleted: true}, []*model.Post{ - team2channel1post2deleted, gm1post1, - }}, } for _, testCase := range testCases { @@ -962,6 +994,46 @@ func testVarious(t *testing.T, ss store.Store) { } }) + assertThreadPosts := func(t *testing.T, threads []*model.ThreadResponse, expectedPosts []*model.Post) { + t.Helper() + + actualPosts := make([]*model.Post, 0, len(threads)) + actualPostNames := make([]string, 0, len(threads)) + for _, thread := range threads { + actualPosts = append(actualPosts, thread.Post) + postName, ok := postNames[thread.PostId] + require.True(t, ok, "failed to find actual %s in post names", thread.PostId) + actualPostNames = append(actualPostNames, postName) + } + sort.Strings(actualPostNames) + + expectedPostNames := make([]string, 0, len(expectedPosts)) + for _, post := range expectedPosts { + postName, ok := postNames[post.Id] + require.True(t, ok, "failed to find expected %s in post names", post.Id) + expectedPostNames = append(expectedPostNames, postName) + } + sort.Strings(expectedPostNames) + + assert.Equal(t, expectedPostNames, actualPostNames) + + // Check posts themselves + sort.Sort(byPostId(expectedPosts)) + sort.Sort(byPostId(actualPosts)) + if assert.Len(t, actualPosts, len(expectedPosts)) { + for i := range actualPosts { + assert.Equal(t, expectedPosts[i], actualPosts[i], "mismatch comparing expected post %s with actual post %s", postNames[expectedPosts[i].Id], postNames[actualPosts[i].Id]) + } + } else { + assert.Equal(t, expectedPosts, actualPosts) + } + + // Check common fields between threads and posts. + for _, thread := range threads { + assert.Equal(t, thread.DeleteAt, thread.Post.DeleteAt, "expected Thread.DeleteAt == Post.DeleteAt") + } + } + t.Run("GetThreadsForUser", func(t *testing.T) { testCases := []struct { Description string @@ -1005,19 +1077,7 @@ func testVarious(t *testing.T, ss store.Store) { threads, err := ss.Thread().GetThreadsForUser(testCase.UserID, testCase.TeamID, testCase.Options) require.NoError(t, err) - postIDs := make([]string, 0, len(threads)) - for _, thread := range threads { - postIDs = append(postIDs, thread.PostId) - } - sort.Strings(postIDs) - - expectedPostIDs := make([]string, 0, len(testCase.ExpectedThreads)) - for _, post := range testCase.ExpectedThreads { - expectedPostIDs = append(expectedPostIDs, post.Id) - } - sort.Strings(expectedPostIDs) - - assert.Equal(t, expectedPostIDs, postIDs) + assertThreadPosts(t, threads, testCase.ExpectedThreads) }) } }) diff --git a/store/storetest/user_store.go b/store/storetest/user_store.go index 8013ec8929..679ca51cd2 100644 --- a/store/storetest/user_store.go +++ b/store/storetest/user_store.go @@ -244,6 +244,9 @@ func testUserStoreUpdateUpdateAt(t *testing.T, ss store.Store) { _, nErr := ss.Team().SaveMember(&model.TeamMember{TeamId: model.NewId(), UserId: u1.Id}, -1) require.NoError(t, nErr) + // Ensure UpdateAt has a change to be different below. + time.Sleep(1 * time.Millisecond) + _, err = ss.User().UpdateUpdateAt(u1.Id) require.NoError(t, err)