Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Performance improvements for pull request list page #29900

Merged
merged 18 commits into from
Mar 21, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 29 additions & 0 deletions models/activities/notification_list.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import (
user_model "code.gitea.io/gitea/models/user"
"code.gitea.io/gitea/modules/container"
"code.gitea.io/gitea/modules/log"
"code.gitea.io/gitea/modules/util"

"xorm.io/builder"
)
Expand Down Expand Up @@ -470,3 +471,31 @@ func (nl NotificationList) LoadComments(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
}
14 changes: 0 additions & 14 deletions models/issues/issue.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
3 changes: 3 additions & 0 deletions models/issues/issue_list.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
9 changes: 9 additions & 0 deletions models/issues/pull_list.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
9 changes: 4 additions & 5 deletions models/issues/review.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
}
Expand Down
11 changes: 10 additions & 1 deletion modules/util/slice.go
Original file line number Diff line number Diff line change
Expand Up @@ -54,11 +54,20 @@ 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 {
values = append(values, 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
}
5 changes: 3 additions & 2 deletions routers/api/v1/repo/issue.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
16 changes: 5 additions & 11 deletions routers/api/v1/repo/issue_pin.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
6 changes: 6 additions & 0 deletions routers/web/user/notification.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down
5 changes: 3 additions & 2 deletions services/convert/notification.go
Original file line number Diff line number Diff line change
Expand Up @@ -61,8 +61,9 @@ 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 {
if err := n.Issue.LoadPullRequest(ctx); err == nil &&
n.Issue.PullRequest != nil &&
n.Issue.PullRequest.HasMerged {
result.Subject.State = "merged"
}
}
Expand Down
4 changes: 2 additions & 2 deletions services/pull/review.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
16 changes: 8 additions & 8 deletions templates/shared/issueicon.tmpl
Original file line number Diff line number Diff line change
@@ -1,15 +1,15 @@
{{if .IsPull}}
{{if and .PullRequest .PullRequest.HasMerged}}
{{svg "octicon-git-merge" 16 "text purple"}}
{{else if and (.GetPullRequest ctx) (.GetPullRequest ctx).HasMerged}}
{{svg "octicon-git-merge" 16 "text purple"}}
lunny marked this conversation as resolved.
Show resolved Hide resolved
{{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)}}
{{svg "octicon-git-pull-request-draft" 16 "text grey"}}
{{else if and (.GetPullRequest ctx) ((.GetPullRequest ctx).IsWorkInProgress ctx)}}
{{if .PullRequest.IsWorkInProgress ctx}}
{{svg "octicon-git-pull-request-draft" 16 "text grey"}}
{{else}}
{{svg "octicon-git-pull-request" 16 "text green"}}
Expand Down
4 changes: 2 additions & 2 deletions tests/integration/pull_merge_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
5 changes: 2 additions & 3 deletions tests/integration/pull_update_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}