From 56e8eb0661e4cf6a0185e54e89d5a5deb85a622f Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sat, 4 Jan 2025 21:29:46 -0800 Subject: [PATCH 1/9] Filter reviews of one pull request in memory instead of database to reduce slow response because of lacking database index --- models/issues/pull.go | 4 +- models/issues/review_list.go | 68 +++++++++++++++++------------ routers/web/repo/issue_page_meta.go | 10 +---- 3 files changed, 45 insertions(+), 37 deletions(-) diff --git a/models/issues/pull.go b/models/issues/pull.go index 8c43eb19ddfda..786b2aa130d01 100644 --- a/models/issues/pull.go +++ b/models/issues/pull.go @@ -301,7 +301,7 @@ func (pr *PullRequest) LoadRequestedReviewers(ctx context.Context) error { return nil } - reviews, err := GetReviewsByIssueID(ctx, pr.Issue.ID) + reviews, _, err := GetReviewsByIssueID(ctx, pr.Issue.ID) if err != nil { return err } @@ -320,7 +320,7 @@ func (pr *PullRequest) LoadRequestedReviewers(ctx context.Context) error { // LoadRequestedReviewersTeams loads the requested reviewers teams. func (pr *PullRequest) LoadRequestedReviewersTeams(ctx context.Context) error { - reviews, err := GetReviewsByIssueID(ctx, pr.Issue.ID) + reviews, _, err := GetReviewsByIssueID(ctx, pr.Issue.ID) if err != nil { return err } diff --git a/models/issues/review_list.go b/models/issues/review_list.go index bc7d7ec0f0168..cccf20e8fa45c 100644 --- a/models/issues/review_list.go +++ b/models/issues/review_list.go @@ -5,6 +5,8 @@ package issues import ( "context" + "slices" + "sort" "code.gitea.io/gitea/models/db" organization_model "code.gitea.io/gitea/models/organization" @@ -153,43 +155,55 @@ func CountReviews(ctx context.Context, opts FindReviewOptions) (int64, error) { return db.GetEngine(ctx).Where(opts.toCond()).Count(&Review{}) } -// GetReviewersFromOriginalAuthorsByIssueID gets the latest review of each original authors for a pull request -func GetReviewersFromOriginalAuthorsByIssueID(ctx context.Context, issueID int64) (ReviewList, error) { +var allowedReivewTypes = []ReviewType{ReviewTypeApprove, ReviewTypeReject, ReviewTypeRequest} + +// GetReviewsByIssueID gets the latest review of each reviewer for a pull request +func GetReviewsByIssueID(ctx context.Context, issueID int64) (ReviewList, ReviewList, error) { reviews := make([]*Review, 0, 10) - // Get latest review of each reviewer, sorted in order they were made - if err := db.GetEngine(ctx).SQL("SELECT * FROM review WHERE id IN (SELECT max(id) as id FROM review WHERE issue_id = ? AND reviewer_team_id = 0 AND type in (?, ?, ?) AND original_author_id <> 0 GROUP BY issue_id, original_author_id) ORDER BY review.updated_unix ASC", - issueID, ReviewTypeApprove, ReviewTypeReject, ReviewTypeRequest). - Find(&reviews); err != nil { - return nil, err + // Get all reviews for the issue id + if err := db.GetEngine(ctx).Where("issue_id=?", issueID).OrderBy("updated_unix ASC").Find(&reviews); err != nil { + return nil, nil, err } - return reviews, nil -} - -// GetReviewsByIssueID gets the latest review of each reviewer for a pull request -func GetReviewsByIssueID(ctx context.Context, issueID int64) (ReviewList, error) { - reviews := make([]*Review, 0, 10) + // filter them in memory to get the latest review of each reviewer + // Since the reviewes should not be too many for one issue, less than 100 commonly, it's acceptable to do this in memory + reviewersMap := make(map[int64][]*Review) // key is reviewer id + originalReviewersMap := make(map[int64][]*Review) + reviewTeamsMap := make(map[int64][]*Review) + for _, review := range reviews { + if review.ReviewerTeamID == 0 && slices.Contains(allowedReivewTypes, review.Type) && !review.Dismissed { + if review.OriginalAuthorID != 0 { + originalReviewersMap[review.OriginalAuthorID] = append(originalReviewersMap[review.OriginalAuthorID], review) + } else { + reviewersMap[review.ReviewerID] = append(reviewersMap[review.ReviewerID], review) + } + } else if review.ReviewerTeamID != 0 && review.OriginalAuthorID == 0 { + reviewTeamsMap[review.ReviewerTeamID] = append(reviewTeamsMap[review.ReviewerTeamID], review) + } + } - sess := db.GetEngine(ctx) + mergedReviews := make([]*Review, 0, 10) + for _, reviews := range reviewersMap { + mergedReviews = append(mergedReviews, reviews[len(reviews)-1]) + } + sort.Slice(mergedReviews, func(i, j int) bool { + return mergedReviews[i].UpdatedUnix < mergedReviews[j].UpdatedUnix + }) - // Get latest review of each reviewer, sorted in order they were made - if err := sess.SQL("SELECT * FROM review WHERE id IN (SELECT max(id) as id FROM review WHERE issue_id = ? AND reviewer_team_id = 0 AND type in (?, ?, ?) AND dismissed = ? AND original_author_id = 0 GROUP BY issue_id, reviewer_id) ORDER BY review.updated_unix ASC", - issueID, ReviewTypeApprove, ReviewTypeReject, ReviewTypeRequest, false). - Find(&reviews); err != nil { - return nil, err + originalReviewers := make([]*Review, 0, 10) + for _, reviews := range originalReviewersMap { + originalReviewers = append(originalReviewers, reviews[len(reviews)-1]) } teamReviewRequests := make([]*Review, 0, 5) - if err := sess.SQL("SELECT * FROM review WHERE id IN (SELECT max(id) as id FROM review WHERE issue_id = ? AND reviewer_team_id <> 0 AND original_author_id = 0 GROUP BY issue_id, reviewer_team_id) ORDER BY review.updated_unix ASC", - issueID). - Find(&teamReviewRequests); err != nil { - return nil, err + for _, reviews := range reviewTeamsMap { + teamReviewRequests = append(teamReviewRequests, reviews[len(reviews)-1]) } - if len(teamReviewRequests) > 0 { - reviews = append(reviews, teamReviewRequests...) - } + sort.Slice(teamReviewRequests, func(i, j int) bool { + return teamReviewRequests[i].UpdatedUnix < teamReviewRequests[j].UpdatedUnix + }) - return reviews, nil + return append(teamReviewRequests, teamReviewRequests...), originalReviewers, nil } diff --git a/routers/web/repo/issue_page_meta.go b/routers/web/repo/issue_page_meta.go index b536b04d7c603..272343f460ff3 100644 --- a/routers/web/repo/issue_page_meta.go +++ b/routers/web/repo/issue_page_meta.go @@ -193,6 +193,7 @@ func (d *IssuePageMetaData) retrieveReviewersData(ctx *context.Context) { var posterID int64 var isClosed bool var reviews issues_model.ReviewList + var err error if d.Issue == nil { if ctx.Doer != nil { @@ -206,14 +207,7 @@ func (d *IssuePageMetaData) retrieveReviewersData(ctx *context.Context) { isClosed = d.Issue.IsClosed || d.Issue.PullRequest.HasMerged - originalAuthorReviews, err := issues_model.GetReviewersFromOriginalAuthorsByIssueID(ctx, d.Issue.ID) - if err != nil { - ctx.ServerError("GetReviewersFromOriginalAuthorsByIssueID", err) - return - } - data.OriginalReviews = originalAuthorReviews - - reviews, err = issues_model.GetReviewsByIssueID(ctx, d.Issue.ID) + reviews, data.OriginalReviews, err = issues_model.GetReviewsByIssueID(ctx, d.Issue.ID) if err != nil { ctx.ServerError("GetReviewersByIssueID", err) return From af777ec16e850b17bb107828c0c0a342c97373bf Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sat, 4 Jan 2025 21:40:46 -0800 Subject: [PATCH 2/9] Add comment and fix test --- models/issues/review_list.go | 1 + models/issues/review_test.go | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/models/issues/review_list.go b/models/issues/review_list.go index cccf20e8fa45c..1fb4a27d436bb 100644 --- a/models/issues/review_list.go +++ b/models/issues/review_list.go @@ -168,6 +168,7 @@ func GetReviewsByIssueID(ctx context.Context, issueID int64) (ReviewList, Review // filter them in memory to get the latest review of each reviewer // Since the reviewes should not be too many for one issue, less than 100 commonly, it's acceptable to do this in memory + // And since there are too less indexes in review table, it will be very slow to filter in the database reviewersMap := make(map[int64][]*Review) // key is reviewer id originalReviewersMap := make(map[int64][]*Review) reviewTeamsMap := make(map[int64][]*Review) diff --git a/models/issues/review_test.go b/models/issues/review_test.go index 50330e3ff2c60..c1bb8a69a6008 100644 --- a/models/issues/review_test.go +++ b/models/issues/review_test.go @@ -162,7 +162,7 @@ func TestGetReviewersByIssueID(t *testing.T) { }, ) - allReviews, err := issues_model.GetReviewsByIssueID(db.DefaultContext, issue.ID) + allReviews, _, err := issues_model.GetReviewsByIssueID(db.DefaultContext, issue.ID) assert.NoError(t, err) for _, review := range allReviews { assert.NoError(t, review.LoadReviewer(db.DefaultContext)) From 0f4ca0960b4d56a572cc2113bcf796e0bfb326bf Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sat, 4 Jan 2025 21:41:20 -0800 Subject: [PATCH 3/9] Fix spell --- models/issues/review_list.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/models/issues/review_list.go b/models/issues/review_list.go index 1fb4a27d436bb..409de31bdc0ec 100644 --- a/models/issues/review_list.go +++ b/models/issues/review_list.go @@ -167,7 +167,7 @@ func GetReviewsByIssueID(ctx context.Context, issueID int64) (ReviewList, Review } // filter them in memory to get the latest review of each reviewer - // Since the reviewes should not be too many for one issue, less than 100 commonly, it's acceptable to do this in memory + // Since the reviews should not be too many for one issue, less than 100 commonly, it's acceptable to do this in memory // And since there are too less indexes in review table, it will be very slow to filter in the database reviewersMap := make(map[int64][]*Review) // key is reviewer id originalReviewersMap := make(map[int64][]*Review) From 1235dbc6d91bb1a32fbb2bfefc8a4980f9c38ce7 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sun, 5 Jan 2025 10:16:12 -0800 Subject: [PATCH 4/9] Fix bug --- models/issues/review_list.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/models/issues/review_list.go b/models/issues/review_list.go index 409de31bdc0ec..743d26ee49b20 100644 --- a/models/issues/review_list.go +++ b/models/issues/review_list.go @@ -206,5 +206,5 @@ func GetReviewsByIssueID(ctx context.Context, issueID int64) (ReviewList, Review return teamReviewRequests[i].UpdatedUnix < teamReviewRequests[j].UpdatedUnix }) - return append(teamReviewRequests, teamReviewRequests...), originalReviewers, nil + return append(mergedReviews, teamReviewRequests...), originalReviewers, nil } From 394b86b219a8b38be5258c42ea698b5fc4a8f1da Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sun, 5 Jan 2025 11:07:58 -0800 Subject: [PATCH 5/9] Add some comments --- models/issues/review_list.go | 28 +++++++++++++++------------- 1 file changed, 15 insertions(+), 13 deletions(-) diff --git a/models/issues/review_list.go b/models/issues/review_list.go index 743d26ee49b20..f618409ed0c4d 100644 --- a/models/issues/review_list.go +++ b/models/issues/review_list.go @@ -155,7 +155,7 @@ func CountReviews(ctx context.Context, opts FindReviewOptions) (int64, error) { return db.GetEngine(ctx).Where(opts.toCond()).Count(&Review{}) } -var allowedReivewTypes = []ReviewType{ReviewTypeApprove, ReviewTypeReject, ReviewTypeRequest} +var countedReivewTypes = []ReviewType{ReviewTypeApprove, ReviewTypeReject, ReviewTypeRequest} // GetReviewsByIssueID gets the latest review of each reviewer for a pull request func GetReviewsByIssueID(ctx context.Context, issueID int64) (ReviewList, ReviewList, error) { @@ -169,11 +169,11 @@ func GetReviewsByIssueID(ctx context.Context, issueID int64) (ReviewList, Review // filter them in memory to get the latest review of each reviewer // Since the reviews should not be too many for one issue, less than 100 commonly, it's acceptable to do this in memory // And since there are too less indexes in review table, it will be very slow to filter in the database - reviewersMap := make(map[int64][]*Review) // key is reviewer id - originalReviewersMap := make(map[int64][]*Review) - reviewTeamsMap := make(map[int64][]*Review) + reviewersMap := make(map[int64][]*Review) // key is reviewer id + originalReviewersMap := make(map[int64][]*Review) // key is original author id + reviewTeamsMap := make(map[int64][]*Review) // key is reviewer team id for _, review := range reviews { - if review.ReviewerTeamID == 0 && slices.Contains(allowedReivewTypes, review.Type) && !review.Dismissed { + if review.ReviewerTeamID == 0 && slices.Contains(countedReivewTypes, review.Type) && !review.Dismissed { if review.OriginalAuthorID != 0 { originalReviewersMap[review.OriginalAuthorID] = append(originalReviewersMap[review.OriginalAuthorID], review) } else { @@ -184,27 +184,29 @@ func GetReviewsByIssueID(ctx context.Context, issueID int64) (ReviewList, Review } } - mergedReviews := make([]*Review, 0, 10) + individualReviews := make([]*Review, 0, 10) for _, reviews := range reviewersMap { - mergedReviews = append(mergedReviews, reviews[len(reviews)-1]) + individualReviews = append(individualReviews, reviews[len(reviews)-1]) } - sort.Slice(mergedReviews, func(i, j int) bool { - return mergedReviews[i].UpdatedUnix < mergedReviews[j].UpdatedUnix + sort.Slice(individualReviews, func(i, j int) bool { + return individualReviews[i].UpdatedUnix < individualReviews[j].UpdatedUnix }) - originalReviewers := make([]*Review, 0, 10) + originalReviews := make([]*Review, 0, 10) for _, reviews := range originalReviewersMap { - originalReviewers = append(originalReviewers, reviews[len(reviews)-1]) + originalReviews = append(originalReviews, reviews[len(reviews)-1]) } + sort.Slice(originalReviews, func(i, j int) bool { + return originalReviews[i].UpdatedUnix < originalReviews[j].UpdatedUnix + }) teamReviewRequests := make([]*Review, 0, 5) for _, reviews := range reviewTeamsMap { teamReviewRequests = append(teamReviewRequests, reviews[len(reviews)-1]) } - sort.Slice(teamReviewRequests, func(i, j int) bool { return teamReviewRequests[i].UpdatedUnix < teamReviewRequests[j].UpdatedUnix }) - return append(mergedReviews, teamReviewRequests...), originalReviewers, nil + return append(individualReviews, teamReviewRequests...), originalReviews, nil } From a893d27ec7b86bf25d8822a85aa0cf5b0e7a4d94 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Mon, 6 Jan 2025 16:38:59 -0800 Subject: [PATCH 6/9] Some improvements --- models/issues/review_list.go | 7 ++++--- models/issues/review_test.go | 3 ++- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/models/issues/review_list.go b/models/issues/review_list.go index f618409ed0c4d..a85b2f4381f56 100644 --- a/models/issues/review_list.go +++ b/models/issues/review_list.go @@ -155,9 +155,10 @@ func CountReviews(ctx context.Context, opts FindReviewOptions) (int64, error) { return db.GetEngine(ctx).Where(opts.toCond()).Count(&Review{}) } -var countedReivewTypes = []ReviewType{ReviewTypeApprove, ReviewTypeReject, ReviewTypeRequest} - // GetReviewsByIssueID gets the latest review of each reviewer for a pull request +// The first returned parameter is the latest review of each individual reviewer or team +// The second returned parameter is the latest review of each original author which is migrated from other systems +// The reviews are sorted by updated time func GetReviewsByIssueID(ctx context.Context, issueID int64) (ReviewList, ReviewList, error) { reviews := make([]*Review, 0, 10) @@ -173,7 +174,7 @@ func GetReviewsByIssueID(ctx context.Context, issueID int64) (ReviewList, Review originalReviewersMap := make(map[int64][]*Review) // key is original author id reviewTeamsMap := make(map[int64][]*Review) // key is reviewer team id for _, review := range reviews { - if review.ReviewerTeamID == 0 && slices.Contains(countedReivewTypes, review.Type) && !review.Dismissed { + if review.ReviewerTeamID == 0 && slices.Contains([]ReviewType{ReviewTypeApprove, ReviewTypeReject, ReviewTypeRequest}, review.Type) && !review.Dismissed { if review.OriginalAuthorID != 0 { originalReviewersMap[review.OriginalAuthorID] = append(originalReviewersMap[review.OriginalAuthorID], review) } else { diff --git a/models/issues/review_test.go b/models/issues/review_test.go index c1bb8a69a6008..2588b8ba41b05 100644 --- a/models/issues/review_test.go +++ b/models/issues/review_test.go @@ -162,8 +162,9 @@ func TestGetReviewersByIssueID(t *testing.T) { }, ) - allReviews, _, err := issues_model.GetReviewsByIssueID(db.DefaultContext, issue.ID) + allReviews, migratedReviews, err := issues_model.GetReviewsByIssueID(db.DefaultContext, issue.ID) assert.NoError(t, err) + assert.Empty(t, migratedReviews) for _, review := range allReviews { assert.NoError(t, review.LoadReviewer(db.DefaultContext)) } From d4a7d01a6d43650e4977c57f08219476d24fed93 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Mon, 6 Jan 2025 16:49:36 -0800 Subject: [PATCH 7/9] with named return parameter --- models/issues/review_list.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/models/issues/review_list.go b/models/issues/review_list.go index a85b2f4381f56..3ed49d9442f8e 100644 --- a/models/issues/review_list.go +++ b/models/issues/review_list.go @@ -159,7 +159,7 @@ func CountReviews(ctx context.Context, opts FindReviewOptions) (int64, error) { // The first returned parameter is the latest review of each individual reviewer or team // The second returned parameter is the latest review of each original author which is migrated from other systems // The reviews are sorted by updated time -func GetReviewsByIssueID(ctx context.Context, issueID int64) (ReviewList, ReviewList, error) { +func GetReviewsByIssueID(ctx context.Context, issueID int64) (latestReviews ReviewList, migratedOriginalReviews ReviewList, err error) { //nolint reviews := make([]*Review, 0, 10) // Get all reviews for the issue id From b8d697593f7eb2cd941f8afacd6ce51c881861d1 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Mon, 6 Jan 2025 16:51:05 -0800 Subject: [PATCH 8/9] adjustment --- models/issues/review_list.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/models/issues/review_list.go b/models/issues/review_list.go index 3ed49d9442f8e..b0c697a040dc5 100644 --- a/models/issues/review_list.go +++ b/models/issues/review_list.go @@ -173,8 +173,9 @@ func GetReviewsByIssueID(ctx context.Context, issueID int64) (latestReviews Revi reviewersMap := make(map[int64][]*Review) // key is reviewer id originalReviewersMap := make(map[int64][]*Review) // key is original author id reviewTeamsMap := make(map[int64][]*Review) // key is reviewer team id + countedReivewTypes := []ReviewType{ReviewTypeApprove, ReviewTypeReject, ReviewTypeRequest} for _, review := range reviews { - if review.ReviewerTeamID == 0 && slices.Contains([]ReviewType{ReviewTypeApprove, ReviewTypeReject, ReviewTypeRequest}, review.Type) && !review.Dismissed { + if review.ReviewerTeamID == 0 && slices.Contains(countedReivewTypes, review.Type) && !review.Dismissed { if review.OriginalAuthorID != 0 { originalReviewersMap[review.OriginalAuthorID] = append(originalReviewersMap[review.OriginalAuthorID], review) } else { From b96b71f0757635b7706c55fbab7d402d28ff3ec8 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Tue, 7 Jan 2025 08:55:20 +0800 Subject: [PATCH 9/9] Update models/issues/review_list.go --- models/issues/review_list.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/models/issues/review_list.go b/models/issues/review_list.go index b0c697a040dc5..928f24fb2dcd7 100644 --- a/models/issues/review_list.go +++ b/models/issues/review_list.go @@ -159,7 +159,7 @@ func CountReviews(ctx context.Context, opts FindReviewOptions) (int64, error) { // The first returned parameter is the latest review of each individual reviewer or team // The second returned parameter is the latest review of each original author which is migrated from other systems // The reviews are sorted by updated time -func GetReviewsByIssueID(ctx context.Context, issueID int64) (latestReviews ReviewList, migratedOriginalReviews ReviewList, err error) { //nolint +func GetReviewsByIssueID(ctx context.Context, issueID int64) (latestReviews, migratedOriginalReviews ReviewList, err error) { reviews := make([]*Review, 0, 10) // Get all reviews for the issue id