From 352a2cae247afa254241f113c5c22b9351f116b9 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Fri, 31 May 2024 20:10:11 +0800 Subject: [PATCH] Performance improvements for pull request list API (#30490) Fix #30483 --------- Co-authored-by: yp05327 <576951401@qq.com> Co-authored-by: Giteabot --- models/issues/assignees.go | 12 ++-- models/issues/comment_list.go | 12 ++-- models/issues/issue.go | 96 ++++++++++++++++----------- models/issues/issue_label.go | 6 +- models/issues/issue_list.go | 47 ++++++++------ models/issues/pull.go | 13 ++-- models/issues/pull_list.go | 111 ++++++++++++++++++++++---------- models/user/user.go | 4 ++ routers/api/v1/repo/pull.go | 48 +++++++++----- services/convert/issue.go | 9 ++- services/convert/pull.go | 12 +++- services/issue/assignee_test.go | 3 +- 12 files changed, 243 insertions(+), 130 deletions(-) diff --git a/models/issues/assignees.go b/models/issues/assignees.go index 30234be07..efd992cda 100644 --- a/models/issues/assignees.go +++ b/models/issues/assignees.go @@ -27,23 +27,27 @@ func init() { // LoadAssignees load assignees of this issue. func (issue *Issue) LoadAssignees(ctx context.Context) (err error) { + if issue.isAssigneeLoaded || len(issue.Assignees) > 0 { + return nil + } + // Reset maybe preexisting assignees issue.Assignees = []*user_model.User{} issue.Assignee = nil - err = db.GetEngine(ctx).Table("`user`"). + if err = db.GetEngine(ctx).Table("`user`"). Join("INNER", "issue_assignees", "assignee_id = `user`.id"). Where("issue_assignees.issue_id = ?", issue.ID). - Find(&issue.Assignees) - if err != nil { + Find(&issue.Assignees); err != nil { return err } + issue.isAssigneeLoaded = true // Check if we have at least one assignee and if yes put it in as `Assignee` if len(issue.Assignees) > 0 { issue.Assignee = issue.Assignees[0] } - return err + return nil } // GetAssigneeIDsByIssue returns the IDs of users assigned to an issue diff --git a/models/issues/comment_list.go b/models/issues/comment_list.go index 370b5396e..6b4ad80ee 100644 --- a/models/issues/comment_list.go +++ b/models/issues/comment_list.go @@ -16,19 +16,17 @@ import ( // CommentList defines a list of comments type CommentList []*Comment -func (comments CommentList) getPosterIDs() []int64 { - return container.FilterSlice(comments, func(c *Comment) (int64, bool) { - return c.PosterID, c.PosterID > 0 - }) -} - // LoadPosters loads posters func (comments CommentList) LoadPosters(ctx context.Context) error { if len(comments) == 0 { return nil } - posterMaps, err := getPosters(ctx, comments.getPosterIDs()) + posterIDs := container.FilterSlice(comments, func(c *Comment) (int64, bool) { + return c.PosterID, c.Poster == nil && c.PosterID > 0 + }) + + posterMaps, err := getPostersByIDs(ctx, posterIDs) if err != nil { return err } diff --git a/models/issues/issue.go b/models/issues/issue.go index aad855522..40462ed09 100644 --- a/models/issues/issue.go +++ b/models/issues/issue.go @@ -98,32 +98,35 @@ var ErrIssueAlreadyChanged = util.NewInvalidArgumentErrorf("the issue is already // Issue represents an issue or pull request of repository. type Issue struct { - ID int64 `xorm:"pk autoincr"` - RepoID int64 `xorm:"INDEX UNIQUE(repo_index)"` - Repo *repo_model.Repository `xorm:"-"` - Index int64 `xorm:"UNIQUE(repo_index)"` // Index in one repository. - PosterID int64 `xorm:"INDEX"` - Poster *user_model.User `xorm:"-"` - OriginalAuthor string - OriginalAuthorID int64 `xorm:"index"` - Title string `xorm:"name"` - Content string `xorm:"LONGTEXT"` - RenderedContent template.HTML `xorm:"-"` - ContentVersion int `xorm:"NOT NULL DEFAULT 0"` - Labels []*Label `xorm:"-"` - MilestoneID int64 `xorm:"INDEX"` - Milestone *Milestone `xorm:"-"` - Project *project_model.Project `xorm:"-"` - Priority int - AssigneeID int64 `xorm:"-"` - Assignee *user_model.User `xorm:"-"` - IsClosed bool `xorm:"INDEX"` - IsRead bool `xorm:"-"` - IsPull bool `xorm:"INDEX"` // Indicates whether is a pull request or not. - PullRequest *PullRequest `xorm:"-"` - NumComments int - Ref string - PinOrder int `xorm:"DEFAULT 0"` + ID int64 `xorm:"pk autoincr"` + RepoID int64 `xorm:"INDEX UNIQUE(repo_index)"` + Repo *repo_model.Repository `xorm:"-"` + Index int64 `xorm:"UNIQUE(repo_index)"` // Index in one repository. + PosterID int64 `xorm:"INDEX"` + Poster *user_model.User `xorm:"-"` + OriginalAuthor string + OriginalAuthorID int64 `xorm:"index"` + Title string `xorm:"name"` + Content string `xorm:"LONGTEXT"` + RenderedContent template.HTML `xorm:"-"` + ContentVersion int `xorm:"NOT NULL DEFAULT 0"` + Labels []*Label `xorm:"-"` + isLabelsLoaded bool `xorm:"-"` + MilestoneID int64 `xorm:"INDEX"` + Milestone *Milestone `xorm:"-"` + isMilestoneLoaded bool `xorm:"-"` + Project *project_model.Project `xorm:"-"` + Priority int + AssigneeID int64 `xorm:"-"` + Assignee *user_model.User `xorm:"-"` + isAssigneeLoaded bool `xorm:"-"` + IsClosed bool `xorm:"INDEX"` + IsRead bool `xorm:"-"` + IsPull bool `xorm:"INDEX"` // Indicates whether is a pull request or not. + PullRequest *PullRequest `xorm:"-"` + NumComments int + Ref string + PinOrder int `xorm:"DEFAULT 0"` DeadlineUnix timeutil.TimeStamp `xorm:"INDEX"` @@ -131,11 +134,12 @@ type Issue struct { UpdatedUnix timeutil.TimeStamp `xorm:"INDEX updated"` ClosedUnix timeutil.TimeStamp `xorm:"INDEX"` - Attachments []*repo_model.Attachment `xorm:"-"` - Comments CommentList `xorm:"-"` - Reactions ReactionList `xorm:"-"` - TotalTrackedTime int64 `xorm:"-"` - Assignees []*user_model.User `xorm:"-"` + Attachments []*repo_model.Attachment `xorm:"-"` + isAttachmentsLoaded bool `xorm:"-"` + Comments CommentList `xorm:"-"` + Reactions ReactionList `xorm:"-"` + TotalTrackedTime int64 `xorm:"-"` + Assignees []*user_model.User `xorm:"-"` // IsLocked limits commenting abilities to users on an issue // with write access @@ -187,6 +191,19 @@ func (issue *Issue) LoadRepo(ctx context.Context) (err error) { return nil } +func (issue *Issue) LoadAttachments(ctx context.Context) (err error) { + if issue.isAttachmentsLoaded || issue.Attachments != nil { + return nil + } + + issue.Attachments, err = repo_model.GetAttachmentsByIssueID(ctx, issue.ID) + if err != nil { + return fmt.Errorf("getAttachmentsByIssueID [%d]: %w", issue.ID, err) + } + issue.isAttachmentsLoaded = true + return nil +} + // IsTimetrackerEnabled returns true if the repo enables timetracking func (issue *Issue) IsTimetrackerEnabled(ctx context.Context) bool { if err := issue.LoadRepo(ctx); err != nil { @@ -287,11 +304,12 @@ func (issue *Issue) loadReactions(ctx context.Context) (err error) { // LoadMilestone load milestone of this issue. func (issue *Issue) LoadMilestone(ctx context.Context) (err error) { - if (issue.Milestone == nil || issue.Milestone.ID != issue.MilestoneID) && issue.MilestoneID > 0 { + if !issue.isMilestoneLoaded && (issue.Milestone == nil || issue.Milestone.ID != issue.MilestoneID) && issue.MilestoneID > 0 { issue.Milestone, err = GetMilestoneByRepoID(ctx, issue.RepoID, issue.MilestoneID) if err != nil && !IsErrMilestoneNotExist(err) { return fmt.Errorf("getMilestoneByRepoID [repo_id: %d, milestone_id: %d]: %w", issue.RepoID, issue.MilestoneID, err) } + issue.isMilestoneLoaded = true } return nil } @@ -327,11 +345,8 @@ func (issue *Issue) LoadAttributes(ctx context.Context) (err error) { return err } - if issue.Attachments == nil { - issue.Attachments, err = repo_model.GetAttachmentsByIssueID(ctx, issue.ID) - if err != nil { - return fmt.Errorf("getAttachmentsByIssueID [%d]: %w", issue.ID, err) - } + if err = issue.LoadAttachments(ctx); err != nil { + return err } if err = issue.loadComments(ctx); err != nil { @@ -350,6 +365,13 @@ func (issue *Issue) LoadAttributes(ctx context.Context) (err error) { return issue.loadReactions(ctx) } +func (issue *Issue) ResetAttributesLoaded() { + issue.isLabelsLoaded = false + issue.isMilestoneLoaded = false + issue.isAttachmentsLoaded = false + issue.isAssigneeLoaded = false +} + // GetIsRead load the `IsRead` field of the issue func (issue *Issue) GetIsRead(ctx context.Context, userID int64) error { issueUser := &IssueUser{IssueID: issue.ID, UID: userID} diff --git a/models/issues/issue_label.go b/models/issues/issue_label.go index 733f1043b..10fc82145 100644 --- a/models/issues/issue_label.go +++ b/models/issues/issue_label.go @@ -111,6 +111,7 @@ func NewIssueLabel(ctx context.Context, issue *Issue, label *Label, doer *user_m return err } + issue.isLabelsLoaded = false issue.Labels = nil if err = issue.LoadLabels(ctx); err != nil { return err @@ -160,6 +161,8 @@ func NewIssueLabels(ctx context.Context, issue *Issue, labels []*Label, doer *us return err } + // reload all labels + issue.isLabelsLoaded = false issue.Labels = nil if err = issue.LoadLabels(ctx); err != nil { return err @@ -325,11 +328,12 @@ func FixIssueLabelWithOutsideLabels(ctx context.Context) (int64, error) { // LoadLabels loads labels func (issue *Issue) LoadLabels(ctx context.Context) (err error) { - if issue.Labels == nil && issue.ID != 0 { + if !issue.isLabelsLoaded && issue.Labels == nil && issue.ID != 0 { issue.Labels, err = GetLabelsByIssueID(ctx, issue.ID) if err != nil { return fmt.Errorf("getLabelsByIssueID [%d]: %w", issue.ID, err) } + issue.isLabelsLoaded = true } return nil } diff --git a/models/issues/issue_list.go b/models/issues/issue_list.go index f8ee271a6..0dd37a04d 100644 --- a/models/issues/issue_list.go +++ b/models/issues/issue_list.go @@ -72,18 +72,16 @@ func (issues IssueList) LoadRepositories(ctx context.Context) (repo_model.Reposi return repo_model.ValuesRepository(repoMaps), nil } -func (issues IssueList) getPosterIDs() []int64 { - return container.FilterSlice(issues, func(issue *Issue) (int64, bool) { - return issue.PosterID, true - }) -} - -func (issues IssueList) loadPosters(ctx context.Context) error { +func (issues IssueList) LoadPosters(ctx context.Context) error { if len(issues) == 0 { return nil } - posterMaps, err := getPosters(ctx, issues.getPosterIDs()) + posterIDs := container.FilterSlice(issues, func(issue *Issue) (int64, bool) { + return issue.PosterID, issue.Poster == nil && issue.PosterID > 0 + }) + + posterMaps, err := getPostersByIDs(ctx, posterIDs) if err != nil { return err } @@ -94,7 +92,7 @@ func (issues IssueList) loadPosters(ctx context.Context) error { return nil } -func getPosters(ctx context.Context, posterIDs []int64) (map[int64]*user_model.User, error) { +func getPostersByIDs(ctx context.Context, posterIDs []int64) (map[int64]*user_model.User, error) { posterMaps := make(map[int64]*user_model.User, len(posterIDs)) left := len(posterIDs) for left > 0 { @@ -136,7 +134,7 @@ func (issues IssueList) getIssueIDs() []int64 { return ids } -func (issues IssueList) loadLabels(ctx context.Context) error { +func (issues IssueList) LoadLabels(ctx context.Context) error { if len(issues) == 0 { return nil } @@ -168,7 +166,7 @@ func (issues IssueList) loadLabels(ctx context.Context) error { err = rows.Scan(&labelIssue) if err != nil { if err1 := rows.Close(); err1 != nil { - return fmt.Errorf("IssueList.loadLabels: Close: %w", err1) + return fmt.Errorf("IssueList.LoadLabels: Close: %w", err1) } return err } @@ -177,7 +175,7 @@ func (issues IssueList) loadLabels(ctx context.Context) error { // When there are no rows left and we try to close it. // Since that is not relevant for us, we can safely ignore it. if err1 := rows.Close(); err1 != nil { - return fmt.Errorf("IssueList.loadLabels: Close: %w", err1) + return fmt.Errorf("IssueList.LoadLabels: Close: %w", err1) } left -= limit issueIDs = issueIDs[limit:] @@ -185,6 +183,7 @@ func (issues IssueList) loadLabels(ctx context.Context) error { for _, issue := range issues { issue.Labels = issueLabels[issue.ID] + issue.isLabelsLoaded = true } return nil } @@ -195,7 +194,7 @@ func (issues IssueList) getMilestoneIDs() []int64 { }) } -func (issues IssueList) loadMilestones(ctx context.Context) error { +func (issues IssueList) LoadMilestones(ctx context.Context) error { milestoneIDs := issues.getMilestoneIDs() if len(milestoneIDs) == 0 { return nil @@ -220,6 +219,7 @@ func (issues IssueList) loadMilestones(ctx context.Context) error { for _, issue := range issues { issue.Milestone = milestoneMaps[issue.MilestoneID] + issue.isMilestoneLoaded = true } return nil } @@ -263,7 +263,7 @@ func (issues IssueList) LoadProjects(ctx context.Context) error { return nil } -func (issues IssueList) loadAssignees(ctx context.Context) error { +func (issues IssueList) LoadAssignees(ctx context.Context) error { if len(issues) == 0 { return nil } @@ -310,6 +310,10 @@ func (issues IssueList) loadAssignees(ctx context.Context) error { for _, issue := range issues { issue.Assignees = assignees[issue.ID] + if len(issue.Assignees) > 0 { + issue.Assignee = issue.Assignees[0] + } + issue.isAssigneeLoaded = true } return nil } @@ -413,6 +417,7 @@ func (issues IssueList) LoadAttachments(ctx context.Context) (err error) { for _, issue := range issues { issue.Attachments = attachments[issue.ID] + issue.isAttachmentsLoaded = true } return nil } @@ -538,23 +543,23 @@ func (issues IssueList) LoadAttributes(ctx context.Context) error { return fmt.Errorf("issue.loadAttributes: LoadRepositories: %w", err) } - if err := issues.loadPosters(ctx); err != nil { - return fmt.Errorf("issue.loadAttributes: loadPosters: %w", err) + if err := issues.LoadPosters(ctx); err != nil { + return fmt.Errorf("issue.loadAttributes: LoadPosters: %w", err) } - if err := issues.loadLabels(ctx); err != nil { - return fmt.Errorf("issue.loadAttributes: loadLabels: %w", err) + if err := issues.LoadLabels(ctx); err != nil { + return fmt.Errorf("issue.loadAttributes: LoadLabels: %w", err) } - if err := issues.loadMilestones(ctx); err != nil { - return fmt.Errorf("issue.loadAttributes: loadMilestones: %w", err) + if err := issues.LoadMilestones(ctx); err != nil { + return fmt.Errorf("issue.loadAttributes: LoadMilestones: %w", err) } if err := issues.LoadProjects(ctx); err != nil { return fmt.Errorf("issue.loadAttributes: loadProjects: %w", err) } - if err := issues.loadAssignees(ctx); err != nil { + if err := issues.LoadAssignees(ctx); err != nil { return fmt.Errorf("issue.loadAttributes: loadAssignees: %w", err) } diff --git a/models/issues/pull.go b/models/issues/pull.go index 014fcd9fd..ef49a5104 100644 --- a/models/issues/pull.go +++ b/models/issues/pull.go @@ -159,10 +159,11 @@ type PullRequest struct { ChangedProtectedFiles []string `xorm:"TEXT JSON"` - IssueID int64 `xorm:"INDEX"` - Issue *Issue `xorm:"-"` - Index int64 - RequestedReviewers []*user_model.User `xorm:"-"` + IssueID int64 `xorm:"INDEX"` + Issue *Issue `xorm:"-"` + Index int64 + RequestedReviewers []*user_model.User `xorm:"-"` + isRequestedReviewersLoaded bool `xorm:"-"` HeadRepoID int64 `xorm:"INDEX"` HeadRepo *repo_model.Repository `xorm:"-"` @@ -289,7 +290,7 @@ func (pr *PullRequest) LoadHeadRepo(ctx context.Context) (err error) { // LoadRequestedReviewers loads the requested reviewers. func (pr *PullRequest) LoadRequestedReviewers(ctx context.Context) error { - if len(pr.RequestedReviewers) > 0 { + if pr.isRequestedReviewersLoaded || len(pr.RequestedReviewers) > 0 { return nil } @@ -297,10 +298,10 @@ func (pr *PullRequest) LoadRequestedReviewers(ctx context.Context) error { if err != nil { return err } - if err = reviews.LoadReviewers(ctx); err != nil { return err } + pr.isRequestedReviewersLoaded = true for _, review := range reviews { pr.RequestedReviewers = append(pr.RequestedReviewers, review.Reviewer) } diff --git a/models/issues/pull_list.go b/models/issues/pull_list.go index b5557cad0..e8011a916 100644 --- a/models/issues/pull_list.go +++ b/models/issues/pull_list.go @@ -9,8 +9,10 @@ import ( "code.gitea.io/gitea/models/db" access_model "code.gitea.io/gitea/models/perm/access" + repo_model "code.gitea.io/gitea/models/repo" "code.gitea.io/gitea/models/unit" 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" @@ -123,7 +125,7 @@ func GetPullRequestIDsByCheckStatus(ctx context.Context, status PullRequestStatu } // PullRequests returns all pull requests for a base Repo by the given conditions -func PullRequests(ctx context.Context, baseRepoID int64, opts *PullRequestsOptions) ([]*PullRequest, int64, error) { +func PullRequests(ctx context.Context, baseRepoID int64, opts *PullRequestsOptions) (PullRequestList, int64, error) { if opts.Page <= 0 { opts.Page = 1 } @@ -153,50 +155,93 @@ func PullRequests(ctx context.Context, baseRepoID int64, opts *PullRequestsOptio // PullRequestList defines a list of pull requests type PullRequestList []*PullRequest -func (prs PullRequestList) LoadAttributes(ctx context.Context) error { - if len(prs) == 0 { - return nil +func (prs PullRequestList) getRepositoryIDs() []int64 { + repoIDs := make(container.Set[int64]) + for _, pr := range prs { + if pr.BaseRepo == nil && pr.BaseRepoID > 0 { + repoIDs.Add(pr.BaseRepoID) + } + if pr.HeadRepo == nil && pr.HeadRepoID > 0 { + repoIDs.Add(pr.HeadRepoID) + } } + return repoIDs.Values() +} - // Load issues. - issueIDs := prs.GetIssueIDs() - issues := make([]*Issue, 0, len(issueIDs)) +func (prs PullRequestList) LoadRepositories(ctx context.Context) error { + repoIDs := prs.getRepositoryIDs() + reposMap := make(map[int64]*repo_model.Repository, len(repoIDs)) if err := db.GetEngine(ctx). - Where("id > 0"). - In("id", issueIDs). - Find(&issues); err != nil { - return fmt.Errorf("find issues: %w", err) - } - - set := make(map[int64]*Issue) - for i := range issues { - set[issues[i].ID] = issues[i] + In("id", repoIDs). + Find(&reposMap); err != nil { + return fmt.Errorf("find repos: %w", err) } for _, pr := range prs { - pr.Issue = set[pr.IssueID] - /* - Old code: - pr.Issue.PullRequest = pr // panic here means issueIDs and prs are not in sync - - It's worth panic because it's almost impossible to happen under normal use. - But in integration testing, an asynchronous task could read a database that has been reset. - So returning an error would make more sense, let the caller has a choice to ignore it. - */ - if pr.Issue == nil { - return fmt.Errorf("issues and prs may be not in sync: cannot find issue %v for pr %v: %w", pr.IssueID, pr.ID, util.ErrNotExist) + if pr.BaseRepo == nil { + pr.BaseRepo = reposMap[pr.BaseRepoID] + } + if pr.HeadRepo == nil { + pr.HeadRepo = reposMap[pr.HeadRepoID] + pr.isHeadRepoLoaded = true } - pr.Issue.PullRequest = pr } return nil } +func (prs PullRequestList) LoadAttributes(ctx context.Context) error { + if _, err := prs.LoadIssues(ctx); err != nil { + return err + } + return nil +} + +func (prs PullRequestList) LoadIssues(ctx context.Context) (IssueList, error) { + if len(prs) == 0 { + return nil, nil + } + + // Load issues. + issueIDs := prs.GetIssueIDs() + issues := make(map[int64]*Issue, len(issueIDs)) + if err := db.GetEngine(ctx). + In("id", issueIDs). + Find(&issues); err != nil { + return nil, fmt.Errorf("find issues: %w", err) + } + + issueList := make(IssueList, 0, len(prs)) + for _, pr := range prs { + if pr.Issue == nil { + pr.Issue = issues[pr.IssueID] + /* + Old code: + pr.Issue.PullRequest = pr // panic here means issueIDs and prs are not in sync + + It's worth panic because it's almost impossible to happen under normal use. + But in integration testing, an asynchronous task could read a database that has been reset. + So returning an error would make more sense, let the caller has a choice to ignore it. + */ + if pr.Issue == nil { + return nil, fmt.Errorf("issues and prs may be not in sync: cannot find issue %v for pr %v: %w", pr.IssueID, pr.ID, util.ErrNotExist) + } + } + pr.Issue.PullRequest = pr + if pr.Issue.Repo == nil { + pr.Issue.Repo = pr.BaseRepo + } + issueList = append(issueList, pr.Issue) + } + return issueList, nil +} + // GetIssueIDs returns all issue ids func (prs PullRequestList) GetIssueIDs() []int64 { - issueIDs := make([]int64, 0, len(prs)) - for i := range prs { - issueIDs = append(issueIDs, prs[i].IssueID) - } - return issueIDs + return container.FilterSlice(prs, func(pr *PullRequest) (int64, bool) { + if pr.Issue == nil { + return pr.IssueID, pr.IssueID > 0 + } + return 0, false + }) } // HasMergedPullRequestInRepo returns whether the user(poster) has merged pull-request in the repo diff --git a/models/user/user.go b/models/user/user.go index 6848d1be9..23637f461 100644 --- a/models/user/user.go +++ b/models/user/user.go @@ -856,6 +856,10 @@ func GetUserByID(ctx context.Context, id int64) (*User, error) { // GetUserByIDs returns the user objects by given IDs if exists. func GetUserByIDs(ctx context.Context, ids []int64) ([]*User, error) { + if len(ids) == 0 { + return nil, nil + } + users := make([]*User, 0, len(ids)) err := db.GetEngine(ctx).In("id", ids). Table("user"). diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index a9aa5c4d8..4014fe80f 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -116,23 +116,39 @@ func ListPullRequests(ctx *context.APIContext) { } apiPrs := make([]*api.PullRequest, len(prs)) + // NOTE: load repository first, so that issue.Repo will be filled with pr.BaseRepo + if err := prs.LoadRepositories(ctx); err != nil { + ctx.Error(http.StatusInternalServerError, "LoadRepositories", err) + return + } + issueList, err := prs.LoadIssues(ctx) + if err != nil { + ctx.Error(http.StatusInternalServerError, "LoadIssues", err) + return + } + + if err := issueList.LoadLabels(ctx); err != nil { + ctx.Error(http.StatusInternalServerError, "LoadLabels", err) + return + } + if err := issueList.LoadPosters(ctx); err != nil { + ctx.Error(http.StatusInternalServerError, "LoadPoster", err) + return + } + if err := issueList.LoadAttachments(ctx); err != nil { + ctx.Error(http.StatusInternalServerError, "LoadAttachments", err) + return + } + if err := issueList.LoadMilestones(ctx); err != nil { + ctx.Error(http.StatusInternalServerError, "LoadMilestones", err) + return + } + if err := issueList.LoadAssignees(ctx); err != nil { + ctx.Error(http.StatusInternalServerError, "LoadAssignees", err) + return + } + for i := range prs { - if err = prs[i].LoadIssue(ctx); err != nil { - ctx.Error(http.StatusInternalServerError, "LoadIssue", err) - return - } - if err = prs[i].LoadAttributes(ctx); err != nil { - ctx.Error(http.StatusInternalServerError, "LoadAttributes", err) - return - } - if err = prs[i].LoadBaseRepo(ctx); err != nil { - ctx.Error(http.StatusInternalServerError, "LoadBaseRepo", err) - return - } - if err = prs[i].LoadHeadRepo(ctx); err != nil { - ctx.Error(http.StatusInternalServerError, "LoadHeadRepo", err) - return - } apiPrs[i] = convert.ToAPIPullRequest(ctx, prs[i], ctx.Doer) } diff --git a/services/convert/issue.go b/services/convert/issue.go index 4fe7ef44f..f514dc431 100644 --- a/services/convert/issue.go +++ b/services/convert/issue.go @@ -31,15 +31,15 @@ func ToAPIIssue(ctx context.Context, doer *user_model.User, issue *issues_model. } func toIssue(ctx context.Context, doer *user_model.User, issue *issues_model.Issue, getDownloadURL func(repo *repo_model.Repository, attach *repo_model.Attachment) string) *api.Issue { - if err := issue.LoadLabels(ctx); err != nil { - return &api.Issue{} - } if err := issue.LoadPoster(ctx); err != nil { return &api.Issue{} } if err := issue.LoadRepo(ctx); err != nil { return &api.Issue{} } + if err := issue.LoadAttachments(ctx); err != nil { + return &api.Issue{} + } apiIssue := &api.Issue{ ID: issue.ID, @@ -63,6 +63,9 @@ func toIssue(ctx context.Context, doer *user_model.User, issue *issues_model.Iss } apiIssue.URL = issue.APIURL(ctx) apiIssue.HTMLURL = issue.HTMLURL() + if err := issue.LoadLabels(ctx); err != nil { + return &api.Issue{} + } apiIssue.Labels = ToLabelList(issue.Labels, issue.Repo, issue.Repo.Owner) apiIssue.Repo = &api.RepositoryMeta{ ID: issue.Repo.ID, diff --git a/services/convert/pull.go b/services/convert/pull.go index 6d95804b3..c214805ed 100644 --- a/services/convert/pull.go +++ b/services/convert/pull.go @@ -11,6 +11,7 @@ import ( "code.gitea.io/gitea/models/perm" access_model "code.gitea.io/gitea/models/perm/access" user_model "code.gitea.io/gitea/models/user" + "code.gitea.io/gitea/modules/cache" "code.gitea.io/gitea/modules/git" "code.gitea.io/gitea/modules/gitrepo" "code.gitea.io/gitea/modules/log" @@ -44,7 +45,16 @@ func ToAPIPullRequest(ctx context.Context, pr *issues_model.PullRequest, doer *u return nil } - p, err := access_model.GetUserRepoPermission(ctx, pr.BaseRepo, doer) + var doerID int64 + if doer != nil { + doerID = doer.ID + } + + const repoDoerPermCacheKey = "repo_doer_perm_cache" + p, err := cache.GetWithContextCache(ctx, repoDoerPermCacheKey, fmt.Sprintf("%d_%d", pr.BaseRepoID, doerID), + func() (access_model.Permission, error) { + return access_model.GetUserRepoPermission(ctx, pr.BaseRepo, doer) + }) if err != nil { log.Error("GetUserRepoPermission[%d]: %v", pr.BaseRepoID, err) p.AccessMode = perm.AccessModeNone diff --git a/services/issue/assignee_test.go b/services/issue/assignee_test.go index da25da60e..38d56f9d9 100644 --- a/services/issue/assignee_test.go +++ b/services/issue/assignee_test.go @@ -39,7 +39,8 @@ func TestDeleteNotPassedAssignee(t *testing.T) { assert.NoError(t, err) assert.Empty(t, issue.Assignees) - // Check they're gone + // Reload to check they're gone + issue.ResetAttributesLoaded() assert.NoError(t, issue.LoadAssignees(db.DefaultContext)) assert.Empty(t, issue.Assignees) assert.Empty(t, issue.Assignee)