Merge pull request '[GITEA PORT] Do some performance optimize for issues list and view issue/pull (gitea#29515)' (#3116) from oliverpool/forgejo:port_29515 into forgejo

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/3116
Reviewed-by: Earl Warren <earl-warren@noreply.codeberg.org>

models/issues/comment.go

@@ -673,7 +673,8 @@ func (c *Comment) LoadTime(ctx context.Context) error {
 	return err
 }
 
-func (c *Comment) loadReactions(ctx context.Context, repo *repo_model.Repository) (err error) {
+// LoadReactions loads comment reactions
+func (c *Comment) LoadReactions(ctx context.Context, repo *repo_model.Repository) (err error) {
 	if c.Reactions != nil {
 		return nil
 	}
@@ -691,11 +692,6 @@ func (c *Comment) loadReactions(ctx context.Context, repo *repo_model.Repository
 	return nil
 }
 
-// LoadReactions loads comment reactions
-func (c *Comment) LoadReactions(ctx context.Context, repo *repo_model.Repository) error {
-	return c.loadReactions(ctx, repo)
-}
-
 func (c *Comment) loadReview(ctx context.Context) (err error) {
 	if c.ReviewID == 0 {
 		return nil

models/issues/comment_code.go

@@ -90,7 +90,7 @@ func fetchCodeCommentsByReview(ctx context.Context, issue *Issue, doer *user_mod
 	return pathToLineToComment, nil
 }
 
-func findCodeComments(ctx context.Context, opts FindCommentsOptions, issue *Issue, doer *user_model.User, review *Review, showOutdatedComments bool) ([]*Comment, error) {
+func findCodeComments(ctx context.Context, opts FindCommentsOptions, issue *Issue, doer *user_model.User, review *Review, showOutdatedComments bool) (CommentList, error) {
 	var comments CommentList
 	if review == nil {
 		review = &Review{ID: 0}
@@ -169,7 +169,7 @@ func findCodeComments(ctx context.Context, opts FindCommentsOptions, issue *Issu
 }
 
 // FetchCodeConversation fetches the code conversation of a given comment (same review, treePath and line number)
-func FetchCodeConversation(ctx context.Context, comment *Comment, doer *user_model.User) ([]*Comment, error) {
+func FetchCodeConversation(ctx context.Context, comment *Comment, doer *user_model.User) (CommentList, error) {
 	opts := FindCommentsOptions{
 		Type:     CommentTypeCode,
 		IssueID:  comment.IssueID,

models/issues/comment_list.go

@@ -19,7 +19,9 @@ type CommentList []*Comment
 func (comments CommentList) getPosterIDs() []int64 {
 	posterIDs := make(container.Set[int64], len(comments))
 	for _, comment := range comments {
-		posterIDs.Add(comment.PosterID)
+		if comment.PosterID > 0 {
+			posterIDs.Add(comment.PosterID)
+		}
 	}
 	return posterIDs.Values()
 }
@@ -41,18 +43,12 @@ func (comments CommentList) LoadPosters(ctx context.Context) error {
 	return nil
 }
 
-func (comments CommentList) getCommentIDs() []int64 {
-	ids := make([]int64, 0, len(comments))
-	for _, comment := range comments {
-		ids = append(ids, comment.ID)
-	}
-	return ids
-}
-
 func (comments CommentList) getLabelIDs() []int64 {
 	ids := make(container.Set[int64], len(comments))
 	for _, comment := range comments {
-		ids.Add(comment.LabelID)
+		if comment.LabelID > 0 {
+			ids.Add(comment.LabelID)
+		}
 	}
 	return ids.Values()
 }
@@ -100,7 +96,9 @@ func (comments CommentList) loadLabels(ctx context.Context) error {
 func (comments CommentList) getMilestoneIDs() []int64 {
 	ids := make(container.Set[int64], len(comments))
 	for _, comment := range comments {
-		ids.Add(comment.MilestoneID)
+		if comment.MilestoneID > 0 {
+			ids.Add(comment.MilestoneID)
+		}
 	}
 	return ids.Values()
 }
@@ -141,7 +139,9 @@ func (comments CommentList) loadMilestones(ctx context.Context) error {
 func (comments CommentList) getOldMilestoneIDs() []int64 {
 	ids := make(container.Set[int64], len(comments))
 	for _, comment := range comments {
-		ids.Add(comment.OldMilestoneID)
+		if comment.OldMilestoneID > 0 {
+			ids.Add(comment.OldMilestoneID)
+		}
 	}
 	return ids.Values()
 }
@@ -182,7 +182,9 @@ func (comments CommentList) loadOldMilestones(ctx context.Context) error {
 func (comments CommentList) getAssigneeIDs() []int64 {
 	ids := make(container.Set[int64], len(comments))
 	for _, comment := range comments {
-		ids.Add(comment.AssigneeID)
+		if comment.AssigneeID > 0 {
+			ids.Add(comment.AssigneeID)
+		}
 	}
 	return ids.Values()
 }
@@ -314,7 +316,9 @@ func (comments CommentList) getDependentIssueIDs() []int64 {
 		if comment.DependentIssue != nil {
 			continue
 		}
-		ids.Add(comment.DependentIssueID)
+		if comment.DependentIssueID > 0 {
+			ids.Add(comment.DependentIssueID)
+		}
 	}
 	return ids.Values()
 }
@@ -369,6 +373,39 @@ func (comments CommentList) loadDependentIssues(ctx context.Context) error {
 	return nil
 }
 
+// getAttachmentCommentIDs only return the comment ids which possibly has attachments
+func (comments CommentList) getAttachmentCommentIDs() []int64 {
+	ids := make(container.Set[int64], len(comments))
+	for _, comment := range comments {
+		if comment.Type.HasAttachmentSupport() {
+			ids.Add(comment.ID)
+		}
+	}
+	return ids.Values()
+}
+
+// LoadAttachmentsByIssue loads attachments by issue id
+func (comments CommentList) LoadAttachmentsByIssue(ctx context.Context) error {
+	if len(comments) == 0 {
+		return nil
+	}
+
+	attachments := make([]*repo_model.Attachment, 0, len(comments)/2)
+	if err := db.GetEngine(ctx).Where("issue_id=? AND comment_id>0", comments[0].IssueID).Find(&attachments); err != nil {
+		return err
+	}
+
+	commentAttachmentsMap := make(map[int64][]*repo_model.Attachment, len(comments))
+	for _, attach := range attachments {
+		commentAttachmentsMap[attach.CommentID] = append(commentAttachmentsMap[attach.CommentID], attach)
+	}
+
+	for _, comment := range comments {
+		comment.Attachments = commentAttachmentsMap[comment.ID]
+	}
+	return nil
+}
+
 // LoadAttachments loads attachments
 func (comments CommentList) LoadAttachments(ctx context.Context) (err error) {
 	if len(comments) == 0 {
@@ -376,16 +413,15 @@ func (comments CommentList) LoadAttachments(ctx context.Context) (err error) {
 	}
 
 	attachments := make(map[int64][]*repo_model.Attachment, len(comments))
-	commentsIDs := comments.getCommentIDs()
+	commentsIDs := comments.getAttachmentCommentIDs()
 	left := len(commentsIDs)
 	for left > 0 {
 		limit := db.DefaultMaxInSize
 		if left < limit {
 			limit = left
 		}
-		rows, err := db.GetEngine(ctx).Table("attachment").
+		rows, err := db.GetEngine(ctx).
-			Join("INNER", "comment", "comment.id = attachment.comment_id").
+			In("comment_id", commentsIDs[:limit]).
-			In("comment.id", commentsIDs[:limit]).
 			Rows(new(repo_model.Attachment))
 		if err != nil {
 			return err
@@ -415,7 +451,9 @@ func (comments CommentList) LoadAttachments(ctx context.Context) (err error) {
 func (comments CommentList) getReviewIDs() []int64 {
 	ids := make(container.Set[int64], len(comments))
 	for _, comment := range comments {
-		ids.Add(comment.ReviewID)
+		if comment.ReviewID > 0 {
+			ids.Add(comment.ReviewID)
+		}
 	}
 	return ids.Values()
 }

models/issues/issue_list.go

@@ -391,9 +391,8 @@ func (issues IssueList) LoadAttachments(ctx context.Context) (err error) {
 		if left < limit {
 			limit = left
 		}
-		rows, err := db.GetEngine(ctx).Table("attachment").
+		rows, err := db.GetEngine(ctx).
-			Join("INNER", "issue", "issue.id = attachment.issue_id").
+			In("issue_id", issuesIDs[:limit]).
-			In("issue.id", issuesIDs[:limit]).
 			Rows(new(repo_model.Attachment))
 		if err != nil {
 			return err
@@ -612,3 +611,23 @@ func (issues IssueList) GetApprovalCounts(ctx context.Context) (map[int64][]*Rev
 
 	return approvalCountMap, nil
 }
+
+func (issues IssueList) LoadIsRead(ctx context.Context, userID int64) error {
+	issueIDs := issues.getIssueIDs()
+	issueUsers := make([]*IssueUser, 0, len(issueIDs))
+	if err := db.GetEngine(ctx).Where("uid =?", userID).
+		In("issue_id", issueIDs).
+		Find(&issueUsers); err != nil {
+		return err
+	}
+
+	for _, issueUser := range issueUsers {
+		for _, issue := range issues {
+			if issue.ID == issueUser.IssueID {
+				issue.IsRead = issueUser.IsRead
+			}
+		}
+	}
+
+	return nil
+}

models/issues/issue_list_test.go

@@ -72,4 +72,9 @@ func TestIssueList_LoadAttributes(t *testing.T) {
 			assert.Nil(t, issue.Project)
 		}
 	}
+
+	assert.NoError(t, issueList.LoadIsRead(db.DefaultContext, 1))
+	for _, issue := range issueList {
+		assert.Equal(t, issue.ID == 1, issue.IsRead, "unexpected is_read value for issue[%d]", issue.ID)
+	}
 }

models/repo/attachment.go

@@ -24,7 +24,7 @@ type Attachment struct {
 	IssueID           int64  `xorm:"INDEX"`           // maybe zero when creating
 	ReleaseID         int64  `xorm:"INDEX"`           // maybe zero when creating
 	UploaderID        int64  `xorm:"INDEX DEFAULT 0"` // Notice: will be zero before this column added
-	CommentID         int64
+	CommentID         int64  `xorm:"INDEX"`
 	Name              string
 	DownloadCount     int64              `xorm:"DEFAULT 0"`
 	Size              int64              `xorm:"DEFAULT 0"`

routers/api/v1/repo/issue_comment.go

@@ -323,10 +323,6 @@ func ListRepoIssueComments(ctx *context.APIContext) {
 		ctx.Error(http.StatusInternalServerError, "LoadIssues", err)
 		return
 	}
-	if err := comments.LoadPosters(ctx); err != nil {
-		ctx.Error(http.StatusInternalServerError, "LoadPosters", err)
-		return
-	}
 	if err := comments.LoadAttachments(ctx); err != nil {
 		ctx.Error(http.StatusInternalServerError, "LoadAttachments", err)
 		return

routers/web/repo/issue.go

@@ -328,15 +328,15 @@ func issues(ctx *context.Context, milestoneID, projectID int64, isPullOption opt
 		return
 	}
 
-	// Get posters.
+	if ctx.IsSigned {
-	for i := range issues {
+		if err := issues.LoadIsRead(ctx, ctx.Doer.ID); err != nil {
-		// Check read status
+			ctx.ServerError("LoadIsRead", err)
-		if !ctx.IsSigned {
-			issues[i].IsRead = true
-		} else if err = issues[i].GetIsRead(ctx, ctx.Doer.ID); err != nil {
-			ctx.ServerError("GetIsRead", err)
 			return
 		}
+	} else {
+		for i := range issues {
+			issues[i].IsRead = true
+		}
 	}
 
 	commitStatuses, lastStatus, err := pull_service.GetIssuesAllCommitStatus(ctx, issues)
@@ -1602,20 +1602,20 @@ func ViewIssue(ctx *context.Context) {
 
 	// Render comments and fetch participants.
 	participants[0] = issue.Poster
+
+	if err := issue.Comments.LoadAttachmentsByIssue(ctx); err != nil {
+		ctx.ServerError("LoadAttachmentsByIssue", err)
+		return
+	}
+	if err := issue.Comments.LoadPosters(ctx); err != nil {
+		ctx.ServerError("LoadPosters", err)
+		return
+	}
+
 	for _, comment = range issue.Comments {
 		comment.Issue = issue
 
-		if err := comment.LoadPoster(ctx); err != nil {
-			ctx.ServerError("LoadPoster", err)
-			return
-		}
-
 		if comment.Type == issues_model.CommentTypeComment || comment.Type == issues_model.CommentTypeReview {
-			if err := comment.LoadAttachments(ctx); err != nil {
-				ctx.ServerError("LoadAttachments", err)
-				return
-			}
-
 			comment.RenderedContent, err = markdown.RenderString(&markup.RenderContext{
 				Links: markup.Links{
 					Base: ctx.Repo.RepoLink,
@@ -1663,7 +1663,6 @@ func ViewIssue(ctx *context.Context) {
 				comment.Milestone = ghostMilestone
 			}
 		} else if comment.Type == issues_model.CommentTypeProject {
-
 			if err = comment.LoadProject(ctx); err != nil {
 				ctx.ServerError("LoadProject", err)
 				return
@@ -1729,10 +1728,6 @@ func ViewIssue(ctx *context.Context) {
 			for _, codeComments := range comment.Review.CodeComments {
 				for _, lineComments := range codeComments {
 					for _, c := range lineComments {
-						if err := c.LoadAttachments(ctx); err != nil {
-							ctx.ServerError("LoadAttachments", err)
-							return
-						}
 						// Check tag.
 						role, ok = marked[c.PosterID]
 						if ok {

routers/web/repo/pull_review.go

@@ -169,11 +169,9 @@ func renderConversation(ctx *context.Context, comment *issues_model.Comment, ori
 	}
 	ctx.Data["PageIsPullFiles"] = (origin == "diff")
 
-	for _, c := range comments {
+	if err := comments.LoadAttachments(ctx); err != nil {
-		if err := c.LoadAttachments(ctx); err != nil {
+		ctx.ServerError("LoadAttachments", err)
-			ctx.ServerError("LoadAttachments", err)
+		return
-			return
-		}
 	}
 
 	ctx.Data["IsAttachmentEnabled"] = setting.Attachment.Enabled