From 21a55559127527a6ce9a7cd75fb31a69903684c3 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Tue, 19 Mar 2024 12:25:06 +0800 Subject: [PATCH 1/6] performance improvements for pull request list page --- models/issues/issue_list.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/models/issues/issue_list.go b/models/issues/issue_list.go index 41a90d133d1c8..218891ad35771 100644 --- a/models/issues/issue_list.go +++ b/models/issues/issue_list.go @@ -370,6 +370,9 @@ func (issues IssueList) LoadPullRequests(ctx context.Context) error { for _, issue := range issues { issue.PullRequest = pullRequestMaps[issue.ID] + if issue.PullRequest != nil { + issue.PullRequest.Issue = issue + } } return nil } From 9182b643a75fe1086247b898ad90c6029706b662 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Tue, 19 Mar 2024 16:43:57 +0800 Subject: [PATCH 2/6] More performance improvements --- models/issues/issue.go | 14 -------------- routers/api/v1/repo/issue.go | 5 +++-- routers/api/v1/repo/issue_pin.go | 16 +++++----------- services/convert/notification.go | 7 ++++--- services/pull/review.go | 4 ++-- templates/shared/issueicon.tmpl | 4 ++-- 6 files changed, 16 insertions(+), 34 deletions(-) diff --git a/models/issues/issue.go b/models/issues/issue.go index 563a780dcb134..87c1c86eb15be 100644 --- a/models/issues/issue.go +++ b/models/issues/issue.go @@ -193,20 +193,6 @@ func (issue *Issue) IsTimetrackerEnabled(ctx context.Context) bool { return issue.Repo.IsTimetrackerEnabled(ctx) } -// GetPullRequest returns the issue pull request -func (issue *Issue) GetPullRequest(ctx context.Context) (pr *PullRequest, err error) { - if !issue.IsPull { - return nil, fmt.Errorf("Issue is not a pull request") - } - - pr, err = GetPullRequestByIssueID(ctx, issue.ID) - if err != nil { - return nil, err - } - pr.Issue = issue - return pr, err -} - // LoadPoster loads poster func (issue *Issue) LoadPoster(ctx context.Context) (err error) { if issue.Poster == nil && issue.PosterID != 0 { diff --git a/routers/api/v1/repo/issue.go b/routers/api/v1/repo/issue.go index 61a318baababd..6934b34b24bcb 100644 --- a/routers/api/v1/repo/issue.go +++ b/routers/api/v1/repo/issue.go @@ -872,10 +872,11 @@ func EditIssue(ctx *context.APIContext) { } if form.State != nil { if issue.IsPull { - if pr, err := issue.GetPullRequest(ctx); err != nil { + if err := issue.LoadPullRequest(ctx); err != nil { ctx.Error(http.StatusInternalServerError, "GetPullRequest", err) return - } else if pr.HasMerged { + } + if issue.PullRequest.HasMerged { ctx.Error(http.StatusPreconditionFailed, "MergedPRState", "cannot change state of this pull request, it was already merged") return } diff --git a/routers/api/v1/repo/issue_pin.go b/routers/api/v1/repo/issue_pin.go index ff1135862b3f1..8fcf670fd0cbe 100644 --- a/routers/api/v1/repo/issue_pin.go +++ b/routers/api/v1/repo/issue_pin.go @@ -240,18 +240,12 @@ func ListPinnedPullRequests(ctx *context.APIContext) { } apiPrs := make([]*api.PullRequest, len(issues)) + if err := issues.LoadPullRequests(ctx); err != nil { + ctx.Error(http.StatusInternalServerError, "LoadPullRequests", err) + return + } for i, currentIssue := range issues { - pr, err := currentIssue.GetPullRequest(ctx) - if err != nil { - ctx.Error(http.StatusInternalServerError, "GetPullRequest", err) - return - } - - if err = pr.LoadIssue(ctx); err != nil { - ctx.Error(http.StatusInternalServerError, "LoadIssue", err) - return - } - + pr := currentIssue.PullRequest if err = pr.LoadAttributes(ctx); err != nil { ctx.Error(http.StatusInternalServerError, "LoadAttributes", err) return diff --git a/services/convert/notification.go b/services/convert/notification.go index 0b97530d8b583..f0989a30576a9 100644 --- a/services/convert/notification.go +++ b/services/convert/notification.go @@ -61,9 +61,10 @@ func ToNotificationThread(ctx context.Context, n *activities_model.Notification) result.Subject.LatestCommentHTMLURL = comment.HTMLURL(ctx) } - pr, _ := n.Issue.GetPullRequest(ctx) - if pr != nil && pr.HasMerged { - result.Subject.State = "merged" + if err := n.Issue.LoadPullRequest(ctx); err == nil && n.Issue.PullRequest != nil { + if n.Issue.PullRequest != nil && n.Issue.PullRequest.HasMerged { + result.Subject.State = "merged" + } } } case activities_model.NotificationSourceCommit: diff --git a/services/pull/review.go b/services/pull/review.go index 90d07c8358067..8900ae2ab14a9 100644 --- a/services/pull/review.go +++ b/services/pull/review.go @@ -268,11 +268,11 @@ func createCodeComment(ctx context.Context, doer *user_model.User, repo *repo_mo // SubmitReview creates a review out of the existing pending review or creates a new one if no pending review exist func SubmitReview(ctx context.Context, doer *user_model.User, gitRepo *git.Repository, issue *issues_model.Issue, reviewType issues_model.ReviewType, content, commitID string, attachmentUUIDs []string) (*issues_model.Review, *issues_model.Comment, error) { - pr, err := issue.GetPullRequest(ctx) - if err != nil { + if err := issue.LoadPullRequest(ctx); err != nil { return nil, nil, err } + pr := issue.PullRequest var stale bool if reviewType != issues_model.ReviewTypeApprove && reviewType != issues_model.ReviewTypeReject { stale = false diff --git a/templates/shared/issueicon.tmpl b/templates/shared/issueicon.tmpl index 089e80bd8b39c..026d6f90c3a45 100644 --- a/templates/shared/issueicon.tmpl +++ b/templates/shared/issueicon.tmpl @@ -1,7 +1,7 @@ {{if .IsPull}} {{if and .PullRequest .PullRequest.HasMerged}} {{svg "octicon-git-merge" 16 "text purple"}} - {{else if and (.GetPullRequest ctx) (.GetPullRequest ctx).HasMerged}} + {{else if and (.PullRequest) (.PullRequest.HasMerged)}} {{svg "octicon-git-merge" 16 "text purple"}} {{else}} {{if .IsClosed}} @@ -9,7 +9,7 @@ {{else}} {{if and .PullRequest (.PullRequest.IsWorkInProgress ctx)}} {{svg "octicon-git-pull-request-draft" 16 "text grey"}} - {{else if and (.GetPullRequest ctx) ((.GetPullRequest ctx).IsWorkInProgress ctx)}} + {{else if and (.PullRequest) (.PullRequest.IsWorkInProgress ctx)}} {{svg "octicon-git-pull-request-draft" 16 "text grey"}} {{else}} {{svg "octicon-git-pull-request" 16 "text green"}} From ae258b2f620b169fd6d64b2c52925a9c49b31fdc Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Tue, 19 Mar 2024 16:52:31 +0800 Subject: [PATCH 3/6] make the code simpler --- services/convert/notification.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/services/convert/notification.go b/services/convert/notification.go index f0989a30576a9..41063cf399f76 100644 --- a/services/convert/notification.go +++ b/services/convert/notification.go @@ -61,10 +61,10 @@ func ToNotificationThread(ctx context.Context, n *activities_model.Notification) result.Subject.LatestCommentHTMLURL = comment.HTMLURL(ctx) } - if err := n.Issue.LoadPullRequest(ctx); err == nil && n.Issue.PullRequest != nil { - if n.Issue.PullRequest != nil && n.Issue.PullRequest.HasMerged { - result.Subject.State = "merged" - } + if err := n.Issue.LoadPullRequest(ctx); err == nil && + n.Issue.PullRequest != nil && + n.Issue.PullRequest.HasMerged { + result.Subject.State = "merged" } } case activities_model.NotificationSourceCommit: From cb62e7ff0a32ef5cefbad03eae213e6579fd31b4 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Wed, 20 Mar 2024 11:25:12 +0800 Subject: [PATCH 4/6] Remove duplicated template code --- templates/shared/issueicon.tmpl | 4 ---- 1 file changed, 4 deletions(-) diff --git a/templates/shared/issueicon.tmpl b/templates/shared/issueicon.tmpl index 026d6f90c3a45..dfeaf01f30400 100644 --- a/templates/shared/issueicon.tmpl +++ b/templates/shared/issueicon.tmpl @@ -1,16 +1,12 @@ {{if .IsPull}} {{if and .PullRequest .PullRequest.HasMerged}} {{svg "octicon-git-merge" 16 "text purple"}} - {{else if and (.PullRequest) (.PullRequest.HasMerged)}} - {{svg "octicon-git-merge" 16 "text purple"}} {{else}} {{if .IsClosed}} {{svg "octicon-git-pull-request" 16 "text red"}} {{else}} {{if and .PullRequest (.PullRequest.IsWorkInProgress ctx)}} {{svg "octicon-git-pull-request-draft" 16 "text grey"}} - {{else if and (.PullRequest) (.PullRequest.IsWorkInProgress ctx)}} - {{svg "octicon-git-pull-request-draft" 16 "text grey"}} {{else}} {{svg "octicon-git-pull-request" 16 "text green"}} {{end}} From 0d421aad0ce40652c614f89b35eb024dfc6462c9 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Wed, 20 Mar 2024 14:43:09 +0800 Subject: [PATCH 5/6] Use LoadPullRequest in more places instead of GetPullRequestByIssueID --- models/issues/review.go | 9 ++++----- tests/integration/pull_merge_test.go | 4 ++-- tests/integration/pull_update_test.go | 5 ++--- 3 files changed, 8 insertions(+), 10 deletions(-) diff --git a/models/issues/review.go b/models/issues/review.go index fc110630e0fa8..70aba0f94d8e8 100644 --- a/models/issues/review.go +++ b/models/issues/review.go @@ -239,11 +239,11 @@ type CreateReviewOptions struct { // IsOfficialReviewer check if at least one of the provided reviewers can make official reviews in issue (counts towards required approvals) func IsOfficialReviewer(ctx context.Context, issue *Issue, reviewer *user_model.User) (bool, error) { - pr, err := GetPullRequestByIssueID(ctx, issue.ID) - if err != nil { + if err := issue.LoadPullRequest(ctx); err != nil { return false, err } + pr := issue.PullRequest rule, err := git_model.GetFirstMatchProtectedBranchRule(ctx, pr.BaseRepoID, pr.BaseBranch) if err != nil { return false, err @@ -271,11 +271,10 @@ func IsOfficialReviewer(ctx context.Context, issue *Issue, reviewer *user_model. // IsOfficialReviewerTeam check if reviewer in this team can make official reviews in issue (counts towards required approvals) func IsOfficialReviewerTeam(ctx context.Context, issue *Issue, team *organization.Team) (bool, error) { - pr, err := GetPullRequestByIssueID(ctx, issue.ID) - if err != nil { + if err := issue.LoadPullRequest(ctx); err != nil { return false, err } - pb, err := git_model.GetFirstMatchProtectedBranchRule(ctx, pr.BaseRepoID, pr.BaseBranch) + pb, err := git_model.GetFirstMatchProtectedBranchRule(ctx, issue.PullRequest.BaseRepoID, issue.PullRequest.BaseBranch) if err != nil { return false, err } diff --git a/tests/integration/pull_merge_test.go b/tests/integration/pull_merge_test.go index a04b4c98cd558..daf411f452a44 100644 --- a/tests/integration/pull_merge_test.go +++ b/tests/integration/pull_merge_test.go @@ -516,8 +516,8 @@ func TestConflictChecking(t *testing.T) { assert.NoError(t, err) issue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{Title: "PR with conflict!"}) - conflictingPR, err := issues_model.GetPullRequestByIssueID(db.DefaultContext, issue.ID) - assert.NoError(t, err) + assert.NoError(t, issue.LoadPullRequest(db.DefaultContext)) + conflictingPR := issue.PullRequest // Ensure conflictedFiles is populated. assert.Len(t, conflictingPR.ConflictedFiles, 1) diff --git a/tests/integration/pull_update_test.go b/tests/integration/pull_update_test.go index 078253ffb0cb8..5ae241f3af720 100644 --- a/tests/integration/pull_update_test.go +++ b/tests/integration/pull_update_test.go @@ -177,8 +177,7 @@ func createOutdatedPR(t *testing.T, actor, forkOrg *user_model.User) *issues_mod assert.NoError(t, err) issue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{Title: "Test Pull -to-update-"}) - pr, err := issues_model.GetPullRequestByIssueID(db.DefaultContext, issue.ID) - assert.NoError(t, err) + assert.NoError(t, issue.LoadPullRequest(db.DefaultContext)) - return pr + return issue.PullRequest } From 5ece2403dfa1be6be6be25a108508e3a65e19d3a Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Thu, 21 Mar 2024 14:40:25 +0800 Subject: [PATCH 6/6] Load pull request on notifications --- models/activities/notification.go | 29 +++++++++++++++++++++++++++++ models/issues/pull_list.go | 9 +++++++++ modules/util/slice.go | 11 ++++++++++- routers/web/user/notification.go | 6 ++++++ templates/shared/issueicon.tmpl | 12 ++++++++---- 5 files changed, 62 insertions(+), 5 deletions(-) diff --git a/models/activities/notification.go b/models/activities/notification.go index 230bcdd6e8053..5d554296b32d6 100644 --- a/models/activities/notification.go +++ b/models/activities/notification.go @@ -20,6 +20,7 @@ import ( "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/timeutil" + "code.gitea.io/gitea/modules/util" "xorm.io/builder" ) @@ -609,6 +610,34 @@ func (nl NotificationList) LoadIssues(ctx context.Context) ([]int, error) { return failures, nil } +// LoadIssuePullRequests loads all issues' pull requests if possible +func (nl NotificationList) LoadIssuePullRequests(ctx context.Context) error { + issues := make(map[int64]*issues_model.Issue, len(nl)) + for _, notification := range nl { + if notification.Issue != nil && notification.Issue.IsPull && notification.Issue.PullRequest == nil { + issues[notification.Issue.ID] = notification.Issue + } + } + + if len(issues) == 0 { + return nil + } + + pulls, err := issues_model.GetPullRequestByIssueIDs(ctx, util.KeysOfMap(issues)) + if err != nil { + return err + } + + for _, pull := range pulls { + if issue := issues[pull.IssueID]; issue != nil { + issue.PullRequest = pull + issue.PullRequest.Issue = issue + } + } + + return nil +} + // Without returns the notification list without the failures func (nl NotificationList) Without(failures []int) NotificationList { if len(failures) == 0 { diff --git a/models/issues/pull_list.go b/models/issues/pull_list.go index c209386e2e07d..2ee69cd323a5e 100644 --- a/models/issues/pull_list.go +++ b/models/issues/pull_list.go @@ -212,3 +212,12 @@ func HasMergedPullRequestInRepo(ctx context.Context, repoID, posterID int64) (bo Limit(1). Get(new(Issue)) } + +// GetPullRequestByIssueIDs returns all pull requests by issue ids +func GetPullRequestByIssueIDs(ctx context.Context, issueIDs []int64) (PullRequestList, error) { + prs := make([]*PullRequest, 0, len(issueIDs)) + return prs, db.GetEngine(ctx). + Where("issue_id > 0"). + In("issue_id", issueIDs). + Find(&prs) +} diff --git a/modules/util/slice.go b/modules/util/slice.go index f00e84bf06e9b..9c878c24bea7c 100644 --- a/modules/util/slice.go +++ b/modules/util/slice.go @@ -54,7 +54,7 @@ func Sorted[S ~[]E, E cmp.Ordered](values S) S { return values } -// TODO: Replace with "maps.Values" once available +// TODO: Replace with "maps.Values" once available, current it only in golang.org/x/exp/maps but not in standard library func ValuesOfMap[K comparable, V any](m map[K]V) []V { values := make([]V, 0, len(m)) for _, v := range m { @@ -62,3 +62,12 @@ func ValuesOfMap[K comparable, V any](m map[K]V) []V { } return values } + +// TODO: Replace with "maps.Keys" once available, current it only in golang.org/x/exp/maps but not in standard library +func KeysOfMap[K comparable, V any](m map[K]V) []K { + keys := make([]K, 0, len(m)) + for k := range m { + keys = append(keys, k) + } + return keys +} diff --git a/routers/web/user/notification.go b/routers/web/user/notification.go index 438462371bca0..28f9846d6bdf7 100644 --- a/routers/web/user/notification.go +++ b/routers/web/user/notification.go @@ -144,6 +144,12 @@ func getNotifications(ctx *context.Context) { ctx.ServerError("LoadIssues", err) return } + + if err = notifications.LoadIssuePullRequests(ctx); err != nil { + ctx.ServerError("LoadIssuePullRequests", err) + return + } + notifications = notifications.Without(failures) failCount += len(failures) diff --git a/templates/shared/issueicon.tmpl b/templates/shared/issueicon.tmpl index dfeaf01f30400..a62714e988ca3 100644 --- a/templates/shared/issueicon.tmpl +++ b/templates/shared/issueicon.tmpl @@ -1,11 +1,15 @@ {{if .IsPull}} - {{if and .PullRequest .PullRequest.HasMerged}} - {{svg "octicon-git-merge" 16 "text purple"}} + {{if not .PullRequest}} + No PullRequest {{else}} {{if .IsClosed}} - {{svg "octicon-git-pull-request" 16 "text red"}} + {{if .PullRequest.HasMerged}} + {{svg "octicon-git-merge" 16 "text purple"}} + {{else}} + {{svg "octicon-git-pull-request" 16 "text red"}} + {{end}} {{else}} - {{if and .PullRequest (.PullRequest.IsWorkInProgress ctx)}} + {{if .PullRequest.IsWorkInProgress ctx}} {{svg "octicon-git-pull-request-draft" 16 "text grey"}} {{else}} {{svg "octicon-git-pull-request" 16 "text green"}}