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 <teabot@gitea.io>
(cherry picked from commit 62f8174aa2fae1481c7e17a6afcb731a5b178cd0)

Conflicts:
	models/activities/notification_list.go
	moved to models/activities/notification.go
This commit is contained in:
Lunny Xiao 2024-03-21 21:13:08 +08:00 committed by Earl Warren
parent 0f4614004e
commit d92c2048b3
No known key found for this signature in database
GPG key ID: 0579CB2928A78A00
14 changed files with 86 additions and 50 deletions

View file

@ -20,6 +20,7 @@ import (
"code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/log"
"code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/setting"
"code.gitea.io/gitea/modules/timeutil" "code.gitea.io/gitea/modules/timeutil"
"code.gitea.io/gitea/modules/util"
"xorm.io/builder" "xorm.io/builder"
) )
@ -841,3 +842,31 @@ func UpdateNotificationStatuses(ctx context.Context, user *user_model.User, curr
Update(n) Update(n)
return err return err
} }
// 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
}

View file

@ -194,20 +194,6 @@ func (issue *Issue) IsTimetrackerEnabled(ctx context.Context) bool {
return issue.Repo.IsTimetrackerEnabled(ctx) 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 // LoadPoster loads poster
func (issue *Issue) LoadPoster(ctx context.Context) (err error) { func (issue *Issue) LoadPoster(ctx context.Context) (err error) {
if issue.Poster == nil && issue.PosterID != 0 { if issue.Poster == nil && issue.PosterID != 0 {

View file

@ -370,6 +370,9 @@ func (issues IssueList) LoadPullRequests(ctx context.Context) error {
for _, issue := range issues { for _, issue := range issues {
issue.PullRequest = pullRequestMaps[issue.ID] issue.PullRequest = pullRequestMaps[issue.ID]
if issue.PullRequest != nil {
issue.PullRequest.Issue = issue
}
} }
return nil return nil
} }

View file

@ -220,3 +220,12 @@ func HasMergedPullRequestInRepo(ctx context.Context, repoID, posterID int64) (bo
Limit(1). Limit(1).
Get(new(Issue)) 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)
}

View file

@ -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) // 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) { func IsOfficialReviewer(ctx context.Context, issue *Issue, reviewer *user_model.User) (bool, error) {
pr, err := GetPullRequestByIssueID(ctx, issue.ID) if err := issue.LoadPullRequest(ctx); err != nil {
if err != nil {
return false, err return false, err
} }
pr := issue.PullRequest
rule, err := git_model.GetFirstMatchProtectedBranchRule(ctx, pr.BaseRepoID, pr.BaseBranch) rule, err := git_model.GetFirstMatchProtectedBranchRule(ctx, pr.BaseRepoID, pr.BaseBranch)
if err != nil { if err != nil {
return false, err 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) // 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) { func IsOfficialReviewerTeam(ctx context.Context, issue *Issue, team *organization.Team) (bool, error) {
pr, err := GetPullRequestByIssueID(ctx, issue.ID) if err := issue.LoadPullRequest(ctx); err != nil {
if err != nil {
return false, err 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 { if err != nil {
return false, err return false, err
} }

View file

@ -54,7 +54,7 @@ func Sorted[S ~[]E, E cmp.Ordered](values S) S {
return values 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 { func ValuesOfMap[K comparable, V any](m map[K]V) []V {
values := make([]V, 0, len(m)) values := make([]V, 0, len(m))
for _, v := range m { for _, v := range m {
@ -62,3 +62,12 @@ func ValuesOfMap[K comparable, V any](m map[K]V) []V {
} }
return values 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
}

View file

@ -874,10 +874,11 @@ func EditIssue(ctx *context.APIContext) {
} }
if form.State != nil { if form.State != nil {
if issue.IsPull { if issue.IsPull {
if pr, err := issue.GetPullRequest(ctx); err != nil { if err := issue.LoadPullRequest(ctx); err != nil {
ctx.Error(http.StatusInternalServerError, "GetPullRequest", err) ctx.Error(http.StatusInternalServerError, "GetPullRequest", err)
return 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") ctx.Error(http.StatusPreconditionFailed, "MergedPRState", "cannot change state of this pull request, it was already merged")
return return
} }

View file

@ -240,18 +240,12 @@ func ListPinnedPullRequests(ctx *context.APIContext) {
} }
apiPrs := make([]*api.PullRequest, len(issues)) 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 { for i, currentIssue := range issues {
pr, err := currentIssue.GetPullRequest(ctx) pr := currentIssue.PullRequest
if err != nil {
ctx.Error(http.StatusInternalServerError, "GetPullRequest", err)
return
}
if err = pr.LoadIssue(ctx); err != nil {
ctx.Error(http.StatusInternalServerError, "LoadIssue", err)
return
}
if err = pr.LoadAttributes(ctx); err != nil { if err = pr.LoadAttributes(ctx); err != nil {
ctx.Error(http.StatusInternalServerError, "LoadAttributes", err) ctx.Error(http.StatusInternalServerError, "LoadAttributes", err)
return return

View file

@ -144,6 +144,12 @@ func getNotifications(ctx *context.Context) {
ctx.ServerError("LoadIssues", err) ctx.ServerError("LoadIssues", err)
return return
} }
if err = notifications.LoadIssuePullRequests(ctx); err != nil {
ctx.ServerError("LoadIssuePullRequests", err)
return
}
notifications = notifications.Without(failures) notifications = notifications.Without(failures)
failCount += len(failures) failCount += len(failures)

View file

@ -61,8 +61,9 @@ func ToNotificationThread(ctx context.Context, n *activities_model.Notification)
result.Subject.LatestCommentHTMLURL = comment.HTMLURL(ctx) result.Subject.LatestCommentHTMLURL = comment.HTMLURL(ctx)
} }
pr, _ := n.Issue.GetPullRequest(ctx) if err := n.Issue.LoadPullRequest(ctx); err == nil &&
if pr != nil && pr.HasMerged { n.Issue.PullRequest != nil &&
n.Issue.PullRequest.HasMerged {
result.Subject.State = "merged" result.Subject.State = "merged"
} }
} }

View file

@ -268,11 +268,11 @@ func CreateCodeCommentKnownReviewID(ctx context.Context, doer *user_model.User,
// SubmitReview creates a review out of the existing pending review or creates a new one if no pending review exist // 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) { 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 := issue.LoadPullRequest(ctx); err != nil {
if err != nil {
return nil, nil, err return nil, nil, err
} }
pr := issue.PullRequest
var stale bool var stale bool
if reviewType != issues_model.ReviewTypeApprove && reviewType != issues_model.ReviewTypeReject { if reviewType != issues_model.ReviewTypeApprove && reviewType != issues_model.ReviewTypeReject {
stale = false stale = false

View file

@ -1,15 +1,15 @@
{{if .IsPull}} {{if .IsPull}}
{{if and .PullRequest .PullRequest.HasMerged}} {{if not .PullRequest}}
{{svg "octicon-git-merge" 16 "text purple"}} No PullRequest
{{else if and (.GetPullRequest ctx) (.GetPullRequest ctx).HasMerged}}
{{svg "octicon-git-merge" 16 "text purple"}}
{{else}} {{else}}
{{if .IsClosed}} {{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}} {{else}}
{{if and .PullRequest (.PullRequest.IsWorkInProgress ctx)}} {{if .PullRequest.IsWorkInProgress ctx}}
{{svg "octicon-git-pull-request-draft" 16 "text grey"}}
{{else if and (.GetPullRequest ctx) ((.GetPullRequest ctx).IsWorkInProgress ctx)}}
{{svg "octicon-git-pull-request-draft" 16 "text grey"}} {{svg "octicon-git-pull-request-draft" 16 "text grey"}}
{{else}} {{else}}
{{svg "octicon-git-pull-request" 16 "text green"}} {{svg "octicon-git-pull-request" 16 "text green"}}

View file

@ -513,8 +513,8 @@ func TestConflictChecking(t *testing.T) {
assert.NoError(t, err) assert.NoError(t, err)
issue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{Title: "PR with conflict!"}) issue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{Title: "PR with conflict!"})
conflictingPR, err := issues_model.GetPullRequestByIssueID(db.DefaultContext, issue.ID) assert.NoError(t, issue.LoadPullRequest(db.DefaultContext))
assert.NoError(t, err) conflictingPR := issue.PullRequest
// Ensure conflictedFiles is populated. // Ensure conflictedFiles is populated.
assert.Len(t, conflictingPR.ConflictedFiles, 1) assert.Len(t, conflictingPR.ConflictedFiles, 1)

View file

@ -167,8 +167,7 @@ func createOutdatedPR(t *testing.T, actor, forkOrg *user_model.User) *issues_mod
assert.NoError(t, err) assert.NoError(t, err)
issue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{Title: "Test Pull -to-update-"}) issue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{Title: "Test Pull -to-update-"})
pr, err := issues_model.GetPullRequestByIssueID(db.DefaultContext, issue.ID) assert.NoError(t, issue.LoadPullRequest(db.DefaultContext))
assert.NoError(t, err)
return pr return issue.PullRequest
} }