Skip to content

Commit 62f8174

Browse files
lunnyGiteaBot
andauthored
Performance improvements for pull request list page (#29900)
This PR will avoid load pullrequest.Issue twice in pull request list page. It will reduce x times database queries for those WIP pull requests. Partially fix #29585 --------- Co-authored-by: Giteabot <[email protected]>
1 parent 0150095 commit 62f8174

File tree

14 files changed

+86
-50
lines changed

14 files changed

+86
-50
lines changed

models/activities/notification_list.go

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import (
1414
user_model "code.gitea.io/gitea/models/user"
1515
"code.gitea.io/gitea/modules/container"
1616
"code.gitea.io/gitea/modules/log"
17+
"code.gitea.io/gitea/modules/util"
1718

1819
"xorm.io/builder"
1920
)
@@ -470,3 +471,31 @@ func (nl NotificationList) LoadComments(ctx context.Context) ([]int, error) {
470471
}
471472
return failures, nil
472473
}
474+
475+
// LoadIssuePullRequests loads all issues' pull requests if possible
476+
func (nl NotificationList) LoadIssuePullRequests(ctx context.Context) error {
477+
issues := make(map[int64]*issues_model.Issue, len(nl))
478+
for _, notification := range nl {
479+
if notification.Issue != nil && notification.Issue.IsPull && notification.Issue.PullRequest == nil {
480+
issues[notification.Issue.ID] = notification.Issue
481+
}
482+
}
483+
484+
if len(issues) == 0 {
485+
return nil
486+
}
487+
488+
pulls, err := issues_model.GetPullRequestByIssueIDs(ctx, util.KeysOfMap(issues))
489+
if err != nil {
490+
return err
491+
}
492+
493+
for _, pull := range pulls {
494+
if issue := issues[pull.IssueID]; issue != nil {
495+
issue.PullRequest = pull
496+
issue.PullRequest.Issue = issue
497+
}
498+
}
499+
500+
return nil
501+
}

models/issues/issue.go

Lines changed: 0 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -193,20 +193,6 @@ func (issue *Issue) IsTimetrackerEnabled(ctx context.Context) bool {
193193
return issue.Repo.IsTimetrackerEnabled(ctx)
194194
}
195195

196-
// GetPullRequest returns the issue pull request
197-
func (issue *Issue) GetPullRequest(ctx context.Context) (pr *PullRequest, err error) {
198-
if !issue.IsPull {
199-
return nil, fmt.Errorf("Issue is not a pull request")
200-
}
201-
202-
pr, err = GetPullRequestByIssueID(ctx, issue.ID)
203-
if err != nil {
204-
return nil, err
205-
}
206-
pr.Issue = issue
207-
return pr, err
208-
}
209-
210196
// LoadPoster loads poster
211197
func (issue *Issue) LoadPoster(ctx context.Context) (err error) {
212198
if issue.Poster == nil && issue.PosterID != 0 {

models/issues/issue_list.go

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -370,6 +370,9 @@ func (issues IssueList) LoadPullRequests(ctx context.Context) error {
370370

371371
for _, issue := range issues {
372372
issue.PullRequest = pullRequestMaps[issue.ID]
373+
if issue.PullRequest != nil {
374+
issue.PullRequest.Issue = issue
375+
}
373376
}
374377
return nil
375378
}

models/issues/pull_list.go

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -212,3 +212,12 @@ func HasMergedPullRequestInRepo(ctx context.Context, repoID, posterID int64) (bo
212212
Limit(1).
213213
Get(new(Issue))
214214
}
215+
216+
// GetPullRequestByIssueIDs returns all pull requests by issue ids
217+
func GetPullRequestByIssueIDs(ctx context.Context, issueIDs []int64) (PullRequestList, error) {
218+
prs := make([]*PullRequest, 0, len(issueIDs))
219+
return prs, db.GetEngine(ctx).
220+
Where("issue_id > 0").
221+
In("issue_id", issueIDs).
222+
Find(&prs)
223+
}

models/issues/review.go

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -239,11 +239,11 @@ type CreateReviewOptions struct {
239239

240240
// IsOfficialReviewer check if at least one of the provided reviewers can make official reviews in issue (counts towards required approvals)
241241
func IsOfficialReviewer(ctx context.Context, issue *Issue, reviewer *user_model.User) (bool, error) {
242-
pr, err := GetPullRequestByIssueID(ctx, issue.ID)
243-
if err != nil {
242+
if err := issue.LoadPullRequest(ctx); err != nil {
244243
return false, err
245244
}
246245

246+
pr := issue.PullRequest
247247
rule, err := git_model.GetFirstMatchProtectedBranchRule(ctx, pr.BaseRepoID, pr.BaseBranch)
248248
if err != nil {
249249
return false, err
@@ -271,11 +271,10 @@ func IsOfficialReviewer(ctx context.Context, issue *Issue, reviewer *user_model.
271271

272272
// IsOfficialReviewerTeam check if reviewer in this team can make official reviews in issue (counts towards required approvals)
273273
func IsOfficialReviewerTeam(ctx context.Context, issue *Issue, team *organization.Team) (bool, error) {
274-
pr, err := GetPullRequestByIssueID(ctx, issue.ID)
275-
if err != nil {
274+
if err := issue.LoadPullRequest(ctx); err != nil {
276275
return false, err
277276
}
278-
pb, err := git_model.GetFirstMatchProtectedBranchRule(ctx, pr.BaseRepoID, pr.BaseBranch)
277+
pb, err := git_model.GetFirstMatchProtectedBranchRule(ctx, issue.PullRequest.BaseRepoID, issue.PullRequest.BaseBranch)
279278
if err != nil {
280279
return false, err
281280
}

modules/util/slice.go

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -54,11 +54,20 @@ func Sorted[S ~[]E, E cmp.Ordered](values S) S {
5454
return values
5555
}
5656

57-
// TODO: Replace with "maps.Values" once available
57+
// TODO: Replace with "maps.Values" once available, current it only in golang.org/x/exp/maps but not in standard library
5858
func ValuesOfMap[K comparable, V any](m map[K]V) []V {
5959
values := make([]V, 0, len(m))
6060
for _, v := range m {
6161
values = append(values, v)
6262
}
6363
return values
6464
}
65+
66+
// TODO: Replace with "maps.Keys" once available, current it only in golang.org/x/exp/maps but not in standard library
67+
func KeysOfMap[K comparable, V any](m map[K]V) []K {
68+
keys := make([]K, 0, len(m))
69+
for k := range m {
70+
keys = append(keys, k)
71+
}
72+
return keys
73+
}

routers/api/v1/repo/issue.go

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -872,10 +872,11 @@ func EditIssue(ctx *context.APIContext) {
872872
}
873873
if form.State != nil {
874874
if issue.IsPull {
875-
if pr, err := issue.GetPullRequest(ctx); err != nil {
875+
if err := issue.LoadPullRequest(ctx); err != nil {
876876
ctx.Error(http.StatusInternalServerError, "GetPullRequest", err)
877877
return
878-
} else if pr.HasMerged {
878+
}
879+
if issue.PullRequest.HasMerged {
879880
ctx.Error(http.StatusPreconditionFailed, "MergedPRState", "cannot change state of this pull request, it was already merged")
880881
return
881882
}

routers/api/v1/repo/issue_pin.go

Lines changed: 5 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -240,18 +240,12 @@ func ListPinnedPullRequests(ctx *context.APIContext) {
240240
}
241241

242242
apiPrs := make([]*api.PullRequest, len(issues))
243+
if err := issues.LoadPullRequests(ctx); err != nil {
244+
ctx.Error(http.StatusInternalServerError, "LoadPullRequests", err)
245+
return
246+
}
243247
for i, currentIssue := range issues {
244-
pr, err := currentIssue.GetPullRequest(ctx)
245-
if err != nil {
246-
ctx.Error(http.StatusInternalServerError, "GetPullRequest", err)
247-
return
248-
}
249-
250-
if err = pr.LoadIssue(ctx); err != nil {
251-
ctx.Error(http.StatusInternalServerError, "LoadIssue", err)
252-
return
253-
}
254-
248+
pr := currentIssue.PullRequest
255249
if err = pr.LoadAttributes(ctx); err != nil {
256250
ctx.Error(http.StatusInternalServerError, "LoadAttributes", err)
257251
return

routers/web/user/notification.go

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -144,6 +144,12 @@ func getNotifications(ctx *context.Context) {
144144
ctx.ServerError("LoadIssues", err)
145145
return
146146
}
147+
148+
if err = notifications.LoadIssuePullRequests(ctx); err != nil {
149+
ctx.ServerError("LoadIssuePullRequests", err)
150+
return
151+
}
152+
147153
notifications = notifications.Without(failures)
148154
failCount += len(failures)
149155

services/convert/notification.go

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -61,8 +61,9 @@ func ToNotificationThread(ctx context.Context, n *activities_model.Notification)
6161
result.Subject.LatestCommentHTMLURL = comment.HTMLURL(ctx)
6262
}
6363

64-
pr, _ := n.Issue.GetPullRequest(ctx)
65-
if pr != nil && pr.HasMerged {
64+
if err := n.Issue.LoadPullRequest(ctx); err == nil &&
65+
n.Issue.PullRequest != nil &&
66+
n.Issue.PullRequest.HasMerged {
6667
result.Subject.State = "merged"
6768
}
6869
}

services/pull/review.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -268,11 +268,11 @@ func createCodeComment(ctx context.Context, doer *user_model.User, repo *repo_mo
268268

269269
// SubmitReview creates a review out of the existing pending review or creates a new one if no pending review exist
270270
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) {
271-
pr, err := issue.GetPullRequest(ctx)
272-
if err != nil {
271+
if err := issue.LoadPullRequest(ctx); err != nil {
273272
return nil, nil, err
274273
}
275274

275+
pr := issue.PullRequest
276276
var stale bool
277277
if reviewType != issues_model.ReviewTypeApprove && reviewType != issues_model.ReviewTypeReject {
278278
stale = false

templates/shared/issueicon.tmpl

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,15 @@
11
{{if .IsPull}}
2-
{{if and .PullRequest .PullRequest.HasMerged}}
3-
{{svg "octicon-git-merge" 16 "text purple"}}
4-
{{else if and (.GetPullRequest ctx) (.GetPullRequest ctx).HasMerged}}
5-
{{svg "octicon-git-merge" 16 "text purple"}}
2+
{{if not .PullRequest}}
3+
No PullRequest
64
{{else}}
75
{{if .IsClosed}}
8-
{{svg "octicon-git-pull-request" 16 "text red"}}
6+
{{if .PullRequest.HasMerged}}
7+
{{svg "octicon-git-merge" 16 "text purple"}}
8+
{{else}}
9+
{{svg "octicon-git-pull-request" 16 "text red"}}
10+
{{end}}
911
{{else}}
10-
{{if and .PullRequest (.PullRequest.IsWorkInProgress ctx)}}
11-
{{svg "octicon-git-pull-request-draft" 16 "text grey"}}
12-
{{else if and (.GetPullRequest ctx) ((.GetPullRequest ctx).IsWorkInProgress ctx)}}
12+
{{if .PullRequest.IsWorkInProgress ctx}}
1313
{{svg "octicon-git-pull-request-draft" 16 "text grey"}}
1414
{{else}}
1515
{{svg "octicon-git-pull-request" 16 "text green"}}

tests/integration/pull_merge_test.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -516,8 +516,8 @@ func TestConflictChecking(t *testing.T) {
516516
assert.NoError(t, err)
517517

518518
issue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{Title: "PR with conflict!"})
519-
conflictingPR, err := issues_model.GetPullRequestByIssueID(db.DefaultContext, issue.ID)
520-
assert.NoError(t, err)
519+
assert.NoError(t, issue.LoadPullRequest(db.DefaultContext))
520+
conflictingPR := issue.PullRequest
521521

522522
// Ensure conflictedFiles is populated.
523523
assert.Len(t, conflictingPR.ConflictedFiles, 1)

tests/integration/pull_update_test.go

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -177,8 +177,7 @@ func createOutdatedPR(t *testing.T, actor, forkOrg *user_model.User) *issues_mod
177177
assert.NoError(t, err)
178178

179179
issue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{Title: "Test Pull -to-update-"})
180-
pr, err := issues_model.GetPullRequestByIssueID(db.DefaultContext, issue.ID)
181-
assert.NoError(t, err)
180+
assert.NoError(t, issue.LoadPullRequest(db.DefaultContext))
182181

183-
return pr
182+
return issue.PullRequest
184183
}

0 commit comments

Comments
 (0)