From 397c8755f2165effbf9696934e30fbcdf5d3ba57 Mon Sep 17 00:00:00 2001 From: forgejo-backport-action Date: Sun, 5 Apr 2026 17:30:39 +0200 Subject: [PATCH] [v15.0/forgejo] perf: bulk load resolvers & reactions on pull request comments (#11995) **Backport:** https://codeberg.org/forgejo/forgejo/pulls/11988 Optimize loading pull request review comments, which currently perform separate database queries for each comment in order to load the resolver of the comment, and the reactions on that comment, and the users on each reaction of the comments. I stumbled across this ugly code, which enticed me to look into this: https://codeberg.org/forgejo/forgejo/src/commit/80d840c1284e4f44b9efac208811b9ed26455ade/routers/web/repo/pull.go#L1107-L1120 It appeared to load the attachments from each comment on the pull request review page in separate database queries. It turned out to be a noop, as the attachments are already loaded in bulk: https://codeberg.org/forgejo/forgejo/src/commit/80d840c1284e4f44b9efac208811b9ed26455ade/models/issues/comment_code.go#L120-L122 but the `findCodeComments` method loads the "resolver doer" and the reactions one-by-one for each comment. So I fixed that instead, and removed the ineffective deeply nested for loop. ## Checklist The [contributor guide](https://forgejo.org/docs/next/contributor/) contains information that will be helpful to first time contributors. All work and communication must conform to Forgejo's [AI Agreement](https://codeberg.org/forgejo/governance/src/branch/main/AIAgreement.md). There also are a few [conditions for merging Pull Requests in Forgejo repositories](https://codeberg.org/forgejo/governance/src/branch/main/PullRequestsAgreement.md). You are also welcome to join the [Forgejo development chatroom](https://matrix.to/#/#forgejo-development:matrix.org). ### Tests for Go changes - I added test coverage for Go changes... - [x] in their respective `*_test.go` for unit tests. - [ ] in the `tests/integration` directory if it involves interactions with a live Forgejo server. - I ran... - [x] `make pr-go` before pushing ### Documentation - [ ] I created a pull request [to the documentation](https://codeberg.org/forgejo/docs) to explain to Forgejo users how to use this change. - [x] I did not document these changes and I do not expect someone else to do it. ### Release notes - [ ] This change will be noticed by a Forgejo user or admin (feature, bug fix, performance, etc.). I suggest to include a release note for this change. - [x] This change is not visible to a Forgejo user or admin (refactor, dependency upgrade, etc.). I think there is no need to add a release note for this change. Co-authored-by: Mathieu Fenniak Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/11995 Reviewed-by: Mathieu Fenniak Co-authored-by: forgejo-backport-action Co-committed-by: forgejo-backport-action --- models/issues/comment.go | 15 ---- models/issues/comment_code.go | 22 +++--- models/issues/comment_list.go | 79 ++++++++++++++++++++ models/issues/comment_list_test.go | 108 +++++++++++++++++++++++++++ models/issues/comment_test.go | 25 ++++++- models/issues/reaction.go | 31 ++++++++ routers/api/v1/repo/issue_comment.go | 5 ++ routers/web/repo/issue.go | 9 ++- routers/web/repo/pull.go | 15 ---- services/convert/issue_comment.go | 6 -- 10 files changed, 261 insertions(+), 54 deletions(-) diff --git a/models/issues/comment.go b/models/issues/comment.go index fd0f595945..bfad3935fb 100644 --- a/models/issues/comment.go +++ b/models/issues/comment.go @@ -663,21 +663,6 @@ func (c *Comment) LoadAssigneeUserAndTeam(ctx context.Context) error { return nil } -// LoadResolveDoer if comment.Type is CommentTypeCode and ResolveDoerID not zero, then load resolveDoer -func (c *Comment) LoadResolveDoer(ctx context.Context) (err error) { - if c.ResolveDoerID == 0 || c.Type != CommentTypeCode { - return nil - } - c.ResolveDoer, err = user_model.GetUserByID(ctx, c.ResolveDoerID) - if err != nil { - if user_model.IsErrUserNotExist(err) { - c.ResolveDoer = user_model.NewGhostUser() - err = nil - } - } - return err -} - // IsResolved check if an code comment is resolved func (c *Comment) IsResolved() bool { return c.ResolveDoerID != 0 && c.Type == CommentTypeCode diff --git a/models/issues/comment_code.go b/models/issues/comment_code.go index 3c87a1b41a..800d1e830e 100644 --- a/models/issues/comment_code.go +++ b/models/issues/comment_code.go @@ -133,7 +133,7 @@ func findCodeComments(ctx context.Context, opts FindCommentsOptions, issue *Issu return nil, err } - n := 0 + readyComments := make(CommentList, 0, len(comments)) for _, comment := range comments { if re, ok := reviews[comment.ReviewID]; ok && re != nil { // If the review is pending only the author can see the comments (except if the review is set) @@ -143,17 +143,18 @@ func findCodeComments(ctx context.Context, opts FindCommentsOptions, issue *Issu } comment.Review = re } - comments[n] = comment - n++ + readyComments = append(readyComments, comment) + } - if err := comment.LoadResolveDoer(ctx); err != nil { - return nil, err - } + if err := readyComments.LoadResolveDoers(ctx); err != nil { + return nil, err + } - if err := comment.LoadReactions(ctx, issue.Repo); err != nil { - return nil, err - } + if err := readyComments.LoadReactions(ctx, issue.Repo); err != nil { + return nil, err + } + for _, comment := range readyComments { var err error if comment.RenderedContent, err = markdown.RenderString(&markup.RenderContext{ Ctx: ctx, @@ -165,7 +166,8 @@ func findCodeComments(ctx context.Context, opts FindCommentsOptions, issue *Issu return nil, err } } - return comments[:n], nil + + return readyComments, nil } // FetchCodeConversation fetches the code conversation of a given comment (same review, treePath and line number) diff --git a/models/issues/comment_list.go b/models/issues/comment_list.go index 53903bea31..9a5c22244b 100644 --- a/models/issues/comment_list.go +++ b/models/issues/comment_list.go @@ -5,6 +5,7 @@ package issues import ( "context" + "errors" "forgejo.org/models/db" repo_model "forgejo.org/models/repo" @@ -390,6 +391,84 @@ func (comments CommentList) LoadAttachments(ctx context.Context) (err error) { return nil } +func (comments CommentList) LoadResolveDoers(ctx context.Context) (err error) { + relevant := func(c *Comment) bool { + return c.ResolveDoerID != 0 && c.Type == CommentTypeCode + } + userIDs := make(container.Set[int64]) + for _, comment := range comments { + if relevant(comment) { + userIDs.Add(comment.ResolveDoerID) + } + } + + if len(userIDs) == 0 { + return nil + } + + userMap := make(map[int64]*user_model.User) + users, err := user_model.GetUsersByIDs(ctx, userIDs.Slice()) + if err != nil { + return err + } + for _, user := range users { + userMap[user.ID] = user + } + + for _, comment := range comments { + if !relevant(comment) { + continue + } + resolveDoer, ok := userMap[comment.ResolveDoerID] + if !ok { + comment.ResolveDoer = user_model.NewGhostUser() + } else { + comment.ResolveDoer = resolveDoer + } + } + + return nil +} + +func (comments CommentList) LoadReactions(ctx context.Context, repo *repo_model.Repository) (err error) { + loadIssueID := int64(0) + loadCommentIDs := make([]int64, 0, len(comments)) + + for _, comment := range comments { + if loadIssueID == 0 { + loadIssueID = comment.IssueID + } else if loadIssueID != comment.IssueID { + return errors.New("unable to load reactions from comments on different issues than each other") + } + if comment.Reactions == nil { + loadCommentIDs = append(loadCommentIDs, comment.ID) + } + } + + if loadIssueID == 0 { + return nil + } + + reactions, err := getReactionsForComments(ctx, loadIssueID, loadCommentIDs) + if err != nil { + return err + } + + allReactions := make(ReactionList, 0, len(reactions)) + for _, comment := range comments { + if comment.Reactions == nil { + comment.Reactions = reactions[comment.ID] + allReactions = append(allReactions, comment.Reactions...) + } + } + + if _, err := allReactions.LoadUsers(ctx, repo); err != nil { + return err + } + + return nil +} + func (comments CommentList) getReviewIDs() []int64 { return container.FilterSlice(comments, func(comment *Comment) (int64, bool) { return comment.ReviewID, comment.ReviewID > 0 diff --git a/models/issues/comment_list_test.go b/models/issues/comment_list_test.go index 062a710b84..12a9144722 100644 --- a/models/issues/comment_list_test.go +++ b/models/issues/comment_list_test.go @@ -84,3 +84,111 @@ func TestCommentListLoadUser(t *testing.T) { }) } } + +func TestCommentListLoadResolveDoers(t *testing.T) { + require.NoError(t, unittest.PrepareTestDatabase()) + + issue := unittest.AssertExistsAndLoadBean(t, &Issue{}) + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: issue.RepoID}) + doer := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: repo.OwnerID}) + + empty := CommentList{} + require.NoError(t, empty.LoadResolveDoers(t.Context())) + + comment1, err := CreateComment(db.DefaultContext, &CreateCommentOptions{ + Type: CommentTypeCode, + Doer: doer, + Repo: repo, + Issue: issue, + Content: "Hello", + }) + require.NoError(t, err) + require.NoError(t, MarkConversation(t.Context(), comment1, doer, true)) + comment1 = unittest.AssertExistsAndLoadBean(t, &Comment{ID: comment1.ID}) // reload after change + comment1List := CommentList{comment1} + require.NoError(t, comment1List.LoadResolveDoers(t.Context())) + require.NotNil(t, comment1.ResolveDoer) + assert.Equal(t, doer.ID, comment1.ResolveDoer.ID) + + comment2, err := CreateComment(db.DefaultContext, &CreateCommentOptions{ + Type: CommentTypeCode, + Doer: doer, + Repo: repo, + Issue: issue, + Content: "Hello again", + }) + require.NoError(t, err) + require.NoError(t, MarkConversation(t.Context(), comment2, user_model.NewGhostUser(), true)) + + // Reload for fresh objects + comment1 = unittest.AssertExistsAndLoadBean(t, &Comment{ID: comment1.ID}) + comment2 = unittest.AssertExistsAndLoadBean(t, &Comment{ID: comment2.ID}) + + comment2List := CommentList{comment1, comment2} + require.NoError(t, comment2List.LoadResolveDoers(t.Context())) + require.NotNil(t, comment1.ResolveDoer) + assert.Equal(t, doer.ID, comment1.ResolveDoer.ID) + require.NotNil(t, comment2.ResolveDoer) + assert.EqualValues(t, -1, comment2.ResolveDoer.ID) +} + +func TestCommentListLoadReactions(t *testing.T) { + require.NoError(t, unittest.PrepareTestDatabase()) + + issue := unittest.AssertExistsAndLoadBean(t, &Issue{}) + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: issue.RepoID}) + doer := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: repo.OwnerID}) + + empty := CommentList{} + require.NoError(t, empty.LoadReactions(t.Context(), repo)) + + comment1, err := CreateComment(db.DefaultContext, &CreateCommentOptions{ + Type: CommentTypeCode, + Doer: doer, + Repo: repo, + Issue: issue, + Content: "Hello", + }) + require.NoError(t, err) + _, err = CreateReaction(t.Context(), &ReactionOptions{ + Type: "eyes", + DoerID: doer.ID, + IssueID: issue.ID, + CommentID: comment1.ID, + }) + require.NoError(t, err) + + comment1 = unittest.AssertExistsAndLoadBean(t, &Comment{ID: comment1.ID}) // reload after change + comment1List := CommentList{comment1} + require.NoError(t, comment1List.LoadReactions(t.Context(), repo)) + require.Len(t, comment1.Reactions, 1) + assert.Equal(t, "eyes", comment1.Reactions[0].Type) + assert.NotNil(t, comment1.Reactions[0].User) + + comment2, err := CreateComment(db.DefaultContext, &CreateCommentOptions{ + Type: CommentTypeCode, + Doer: doer, + Repo: repo, + Issue: issue, + Content: "Hello again", + }) + require.NoError(t, err) + _, err = CreateReaction(t.Context(), &ReactionOptions{ + Type: "rocket", + DoerID: doer.ID, + IssueID: issue.ID, + CommentID: comment2.ID, + }) + require.NoError(t, err) + + // Reload for fresh objects + comment1 = unittest.AssertExistsAndLoadBean(t, &Comment{ID: comment1.ID}) + comment2 = unittest.AssertExistsAndLoadBean(t, &Comment{ID: comment2.ID}) + + comment2List := CommentList{comment1, comment2} + require.NoError(t, comment2List.LoadReactions(t.Context(), repo)) + require.Len(t, comment1.Reactions, 1) + require.Len(t, comment2.Reactions, 1) + assert.Equal(t, "rocket", comment2.Reactions[0].Type) + assert.NotNil(t, comment2.Reactions[0].User) +} diff --git a/models/issues/comment_test.go b/models/issues/comment_test.go index c7adf6f62e..da87b8ec2f 100644 --- a/models/issues/comment_test.go +++ b/models/issues/comment_test.go @@ -52,12 +52,29 @@ func TestFetchCodeConversations(t *testing.T) { issue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: 2}) user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1}) + _, err := issues_model.CreateReaction(t.Context(), &issues_model.ReactionOptions{ + Type: "eyes", + DoerID: 2, + IssueID: issue.ID, + CommentID: 4, + }) + require.NoError(t, err) + require.NoError(t, issues_model.MarkConversation(t.Context(), + unittest.AssertExistsAndLoadBean(t, &issues_model.Comment{ID: 4}), + user, true)) + res, err := issues_model.FetchCodeConversations(db.DefaultContext, issue, user, false) require.NoError(t, err) - assert.Contains(t, res, "README.md") - assert.Contains(t, res["README.md"], int64(4)) - assert.Len(t, res["README.md"][4], 1) - assert.Equal(t, int64(4), res["README.md"][4][0][0].ID) + require.Contains(t, res, "README.md") + require.Contains(t, res["README.md"], int64(4)) + require.Len(t, res["README.md"][4], 1) + require.Len(t, res["README.md"][4][0], 1) + comment := res["README.md"][4][0][0] + assert.Equal(t, int64(4), comment.ID) + assert.NotNil(t, comment.ResolveDoer) + require.Len(t, comment.Reactions, 1) + r := comment.Reactions[0] + assert.NotNil(t, r.User) user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) res, err = issues_model.FetchCodeConversations(db.DefaultContext, issue, user2, false) diff --git a/models/issues/reaction.go b/models/issues/reaction.go index 522040c022..9a277a8c12 100644 --- a/models/issues/reaction.go +++ b/models/issues/reaction.go @@ -176,6 +176,37 @@ func FindReactions(ctx context.Context, opts FindReactionsOptions) (ReactionList return reactions, count, err } +func getReactionsForComments(ctx context.Context, issueID int64, commentIDs []int64) (map[int64]ReactionList, error) { + reactions := make(map[int64]ReactionList, len(commentIDs)) + left := len(commentIDs) + for left > 0 { + limit := min(left, db.DefaultMaxInSize) + rows, err := db.GetEngine(ctx). + Where(builder.Eq{"issue_id": issueID}). + In("reaction.`type`", setting.UI.Reactions). + In("comment_id", commentIDs[:limit]). + Rows(&Reaction{}) + if err != nil { + return nil, err + } + + for rows.Next() { + var reaction Reaction + err = rows.Scan(&reaction) + if err != nil { + _ = rows.Close() + return nil, err + } + reactions[reaction.CommentID] = append(reactions[reaction.CommentID], &reaction) + } + + _ = rows.Close() + left -= limit + commentIDs = commentIDs[limit:] + } + return reactions, nil +} + func createReaction(ctx context.Context, opts *ReactionOptions) (*Reaction, error) { reaction := &Reaction{ Type: opts.Type, diff --git a/routers/api/v1/repo/issue_comment.go b/routers/api/v1/repo/issue_comment.go index ceebb41f9e..3f58e4e271 100644 --- a/routers/api/v1/repo/issue_comment.go +++ b/routers/api/v1/repo/issue_comment.go @@ -215,6 +215,11 @@ func ListIssueCommentsAndTimeline(ctx *context.APIContext) { return } + if err := comments.LoadResolveDoers(ctx); err != nil { + ctx.Error(http.StatusInternalServerError, "LoadResolveDoers", err) + return + } + var apiComments []*api.TimelineComment for _, comment := range comments { if comment.Type != issues_model.CommentTypeCode && isXRefCommentAccessible(ctx, ctx.Doer, comment, issue.RepoID, ctx.Reducer) { diff --git a/routers/web/repo/issue.go b/routers/web/repo/issue.go index 3852c4c18f..bb4185f785 100644 --- a/routers/web/repo/issue.go +++ b/routers/web/repo/issue.go @@ -1663,6 +1663,11 @@ func ViewIssue(ctx *context.Context) { return } + if err := issue.Comments.LoadResolveDoers(ctx); err != nil { + ctx.ServerError("LoadResolveDoers", err) + return + } + for commentIdx, comment = range issue.Comments { comment.Issue = issue metas := ctx.Repo.Repository.ComposeMetas(ctx) @@ -1801,10 +1806,6 @@ func ViewIssue(ctx *context.Context) { } } } - if err = comment.LoadResolveDoer(ctx); err != nil { - ctx.ServerError("LoadResolveDoer", err) - return - } } else if comment.Type == issues_model.CommentTypePullRequestPush { participants = addParticipant(comment.Poster, participants) if err = comment.LoadPushCommits(ctx); err != nil { diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index c4b8583e2d..e9cdfc5ac7 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -1104,21 +1104,6 @@ func viewPullFiles(ctx *context.Context, specifiedStartCommit, specifiedEndCommi return } - for _, file := range diff.Files { - for _, section := range file.Sections { - for _, line := range section.Lines { - for _, comments := range line.Conversations { - for _, comment := range comments { - if err := comment.LoadAttachments(ctx); err != nil { - ctx.ServerError("LoadAttachments", err) - return - } - } - } - } - } - } - pb, err := git_model.GetFirstMatchProtectedBranchRule(ctx, pull.BaseRepoID, pull.BaseBranch) if err != nil { ctx.ServerError("LoadProtectedBranch", err) diff --git a/services/convert/issue_comment.go b/services/convert/issue_comment.go index 9ea315aee6..2f9af9be7c 100644 --- a/services/convert/issue_comment.go +++ b/services/convert/issue_comment.go @@ -43,12 +43,6 @@ func ToTimelineComment(ctx context.Context, repo *repo_model.Repository, c *issu return nil } - err = c.LoadResolveDoer(ctx) - if err != nil { - log.Error("LoadResolveDoer: %v", err) - return nil - } - err = c.LoadDepIssueDetails(ctx) if err != nil { log.Error("LoadDepIssueDetails: %v", err)