From 093e0a3c71d3a8fba3d0d16daf30a786f650145d Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Mon, 15 Apr 2024 14:47:54 +0800 Subject: [PATCH 01/11] performance improvements for pull request list API --- models/issues/assignees.go | 5 ++ models/issues/issue.go | 89 +++++++++++++++++++-------------- models/issues/issue_label.go | 3 +- models/issues/issue_list.go | 32 ++++++------ models/issues/pull.go | 12 +++-- models/issues/pull_list.go | 95 ++++++++++++++++++++++++++---------- routers/api/v1/repo/pull.go | 48 ++++++++++++------ services/convert/issue.go | 3 ++ services/convert/pull.go | 12 ++++- 9 files changed, 200 insertions(+), 99 deletions(-) diff --git a/models/issues/assignees.go b/models/issues/assignees.go index 30234be07a60d..5f4c14fe6b864 100644 --- a/models/issues/assignees.go +++ b/models/issues/assignees.go @@ -27,6 +27,10 @@ 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 @@ -35,6 +39,7 @@ func (issue *Issue) LoadAssignees(ctx context.Context) (err error) { Join("INNER", "issue_assignees", "assignee_id = `user`.id"). Where("issue_assignees.issue_id = ?", issue.ID). Find(&issue.Assignees) + issue.isAssigneeLoaded = true if err != nil { return err } diff --git a/models/issues/issue.go b/models/issues/issue.go index aad855522d164..b88f1c5be7908 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) + issue.isAttachmentsLoaded = true + if err != nil { + return fmt.Errorf("getAttachmentsByIssueID [%d]: %w", issue.ID, err) + } + 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,8 +304,9 @@ 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) + issue.isMilestoneLoaded = true if err != nil && !IsErrMilestoneNotExist(err) { return fmt.Errorf("getMilestoneByRepoID [repo_id: %d, milestone_id: %d]: %w", issue.RepoID, issue.MilestoneID, err) } @@ -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 { diff --git a/models/issues/issue_label.go b/models/issues/issue_label.go index 733f1043b0fb7..2fb1dc3a45701 100644 --- a/models/issues/issue_label.go +++ b/models/issues/issue_label.go @@ -325,8 +325,9 @@ 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) + issue.isLabelsLoaded = true if err != nil { return fmt.Errorf("getLabelsByIssueID [%d]: %w", issue.ID, err) } diff --git a/models/issues/issue_list.go b/models/issues/issue_list.go index f8ee271a6bbf5..1419bd123a468 100644 --- a/models/issues/issue_list.go +++ b/models/issues/issue_list.go @@ -74,11 +74,11 @@ func (issues IssueList) LoadRepositories(ctx context.Context) (repo_model.Reposi func (issues IssueList) getPosterIDs() []int64 { return container.FilterSlice(issues, func(issue *Issue) (int64, bool) { - return issue.PosterID, true + return issue.PosterID, issue.Poster == nil }) } -func (issues IssueList) loadPosters(ctx context.Context) error { +func (issues IssueList) LoadPosters(ctx context.Context) error { if len(issues) == 0 { return nil } @@ -136,7 +136,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 +168,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 +177,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 +185,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 +196,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 +221,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 +265,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 +312,7 @@ func (issues IssueList) loadAssignees(ctx context.Context) error { for _, issue := range issues { issue.Assignees = assignees[issue.ID] + issue.isAssigneeLoaded = true } return nil } @@ -413,6 +416,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 +542,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 014fcd9fd022e..8eb33809bd9c9 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,10 +290,11 @@ 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 } + pr.isRequestedReviewersLoaded = true reviews, err := GetReviewsByIssueID(ctx, pr.Issue.ID) if err != nil { return err diff --git a/models/issues/pull_list.go b/models/issues/pull_list.go index b5557cad060ac..b768965313dc3 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) 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() +} + +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). + In("id", repoIDs). + Find(&reposMap); err != nil { + return fmt.Errorf("find issues: %w", err) + } + for _, pr := range prs { + if pr.BaseRepo == nil { + pr.BaseRepo = reposMap[pr.BaseRepoID] + } + if pr.HeadRepo == nil { + pr.HeadRepo = reposMap[pr.HeadRepoID] + pr.isHeadRepoLoaded = true + } + } + 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 + return nil, nil } // Load issues. issueIDs := prs.GetIssueIDs() - issues := make([]*Issue, 0, len(issueIDs)) + issues := make(map[int64]*Issue, len(issueIDs)) if err := db.GetEngine(ctx). - Where("id > 0"). In("id", issueIDs). Find(&issues); err != nil { - return fmt.Errorf("find issues: %w", err) + return nil, fmt.Errorf("find issues: %w", err) } - set := make(map[int64]*Issue) - for i := range issues { - set[issues[i].ID] = issues[i] - } + issueList := make(IssueList, 0, len(prs)) 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) + 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 nil + 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/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index a9aa5c4d8efde..1c88193c6dcc6 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, "LoadAttributes", err) + return + } + issueList, err := prs.LoadIssues(ctx) + if err != nil { + ctx.Error(http.StatusInternalServerError, "LoadAttributes", 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 4fe7ef44fe377..69f83c7c15e55 100644 --- a/services/convert/issue.go +++ b/services/convert/issue.go @@ -40,6 +40,9 @@ func toIssue(ctx context.Context, doer *user_model.User, issue *issues_model.Iss 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, diff --git a/services/convert/pull.go b/services/convert/pull.go index 6d95804b38d0e..c214805ed55d3 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 From 7b16daefd71ff64ad8d26dcd50b49e944f4745ae Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Mon, 15 Apr 2024 14:50:52 +0800 Subject: [PATCH 02/11] Small improvements --- models/user/user.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/models/user/user.go b/models/user/user.go index 6848d1be95be0..c6568e4d352d6 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"). From 94e04f068a9e93d9c95c90a2245de12762a73328 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Mon, 15 Apr 2024 15:25:44 +0800 Subject: [PATCH 03/11] Fix lint --- models/user/user.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/models/user/user.go b/models/user/user.go index c6568e4d352d6..23637f4616288 100644 --- a/models/user/user.go +++ b/models/user/user.go @@ -856,7 +856,7 @@ 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 { + if len(ids) == 0 { return nil, nil } From 57abdef07bf142af02a7a27b59b2ea1642e1699c Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Mon, 15 Apr 2024 17:18:25 +0800 Subject: [PATCH 04/11] Fix reload labels --- models/issues/issue_label.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/models/issues/issue_label.go b/models/issues/issue_label.go index 2fb1dc3a45701..4bd06e982a745 100644 --- a/models/issues/issue_label.go +++ b/models/issues/issue_label.go @@ -160,6 +160,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 From ec8c1702a1f3b8452ef35bc471e0081d9668bfd9 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Mon, 15 Apr 2024 21:37:42 +0800 Subject: [PATCH 05/11] Fix test --- models/issues/issue_label.go | 1 + 1 file changed, 1 insertion(+) diff --git a/models/issues/issue_label.go b/models/issues/issue_label.go index 4bd06e982a745..bf7e697afa9ed 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 From 4b85ef7e7d2718027a0bc2e4cb3c07e4b94e6a56 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Tue, 16 Apr 2024 10:35:11 +0800 Subject: [PATCH 06/11] Reset reload flags --- models/issues/issue.go | 7 +++++++ services/issue/assignee_test.go | 3 ++- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/models/issues/issue.go b/models/issues/issue.go index b88f1c5be7908..5d4acaba82767 100644 --- a/models/issues/issue.go +++ b/models/issues/issue.go @@ -365,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/services/issue/assignee_test.go b/services/issue/assignee_test.go index da25da60ee176..38d56f9d9dc89 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) From e50140d0e5dddb700602b709a92e0f9d3a9c88fa Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Tue, 16 Apr 2024 10:00:48 +0800 Subject: [PATCH 07/11] Update routers/api/v1/repo/pull.go Co-authored-by: yp05327 <576951401@qq.com> --- routers/api/v1/repo/pull.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index 1c88193c6dcc6..9f9f9c782cec4 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -123,7 +123,7 @@ func ListPullRequests(ctx *context.APIContext) { } issueList, err := prs.LoadIssues(ctx) if err != nil { - ctx.Error(http.StatusInternalServerError, "LoadAttributes", err) + ctx.Error(http.StatusInternalServerError, "LoadIssues", err) return } From 0766b944e834b61ffa30633ccad75b7fc08b2534 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Tue, 30 Apr 2024 14:25:55 +0800 Subject: [PATCH 08/11] Fix bug --- models/issues/assignees.go | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/models/issues/assignees.go b/models/issues/assignees.go index 5f4c14fe6b864..efd992cda2b5f 100644 --- a/models/issues/assignees.go +++ b/models/issues/assignees.go @@ -35,20 +35,19 @@ func (issue *Issue) LoadAssignees(ctx context.Context) (err error) { 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) - issue.isAssigneeLoaded = true - 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 From 0a45d666ebb34a14a10630125c412ac69d8dc8c1 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Tue, 30 Apr 2024 16:08:12 +0800 Subject: [PATCH 09/11] Follow yp05327's suggestion --- models/issues/issue.go | 4 ++-- models/issues/issue_label.go | 2 +- models/issues/pull.go | 3 +-- 3 files changed, 4 insertions(+), 5 deletions(-) diff --git a/models/issues/issue.go b/models/issues/issue.go index 5d4acaba82767..40462ed09dfd6 100644 --- a/models/issues/issue.go +++ b/models/issues/issue.go @@ -197,10 +197,10 @@ func (issue *Issue) LoadAttachments(ctx context.Context) (err error) { } issue.Attachments, err = repo_model.GetAttachmentsByIssueID(ctx, issue.ID) - issue.isAttachmentsLoaded = true if err != nil { return fmt.Errorf("getAttachmentsByIssueID [%d]: %w", issue.ID, err) } + issue.isAttachmentsLoaded = true return nil } @@ -306,10 +306,10 @@ func (issue *Issue) loadReactions(ctx context.Context) (err error) { func (issue *Issue) LoadMilestone(ctx context.Context) (err error) { if !issue.isMilestoneLoaded && (issue.Milestone == nil || issue.Milestone.ID != issue.MilestoneID) && issue.MilestoneID > 0 { issue.Milestone, err = GetMilestoneByRepoID(ctx, issue.RepoID, issue.MilestoneID) - issue.isMilestoneLoaded = true 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 } diff --git a/models/issues/issue_label.go b/models/issues/issue_label.go index bf7e697afa9ed..10fc821454275 100644 --- a/models/issues/issue_label.go +++ b/models/issues/issue_label.go @@ -330,10 +330,10 @@ func FixIssueLabelWithOutsideLabels(ctx context.Context) (int64, error) { func (issue *Issue) LoadLabels(ctx context.Context) (err error) { if !issue.isLabelsLoaded && issue.Labels == nil && issue.ID != 0 { issue.Labels, err = GetLabelsByIssueID(ctx, issue.ID) - issue.isLabelsLoaded = true if err != nil { return fmt.Errorf("getLabelsByIssueID [%d]: %w", issue.ID, err) } + issue.isLabelsLoaded = true } return nil } diff --git a/models/issues/pull.go b/models/issues/pull.go index 8eb33809bd9c9..ef49a510458b1 100644 --- a/models/issues/pull.go +++ b/models/issues/pull.go @@ -294,15 +294,14 @@ func (pr *PullRequest) LoadRequestedReviewers(ctx context.Context) error { return nil } - pr.isRequestedReviewersLoaded = true reviews, err := GetReviewsByIssueID(ctx, pr.Issue.ID) 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) } From 29ae51d54b39eacacab948e4a5dd5607f350e862 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Wed, 8 May 2024 12:32:59 +0800 Subject: [PATCH 10/11] Follow yp05327's suggestion --- models/issues/issue_list.go | 3 +++ models/issues/pull_list.go | 2 +- routers/api/v1/repo/pull.go | 2 +- 3 files changed, 5 insertions(+), 2 deletions(-) diff --git a/models/issues/issue_list.go b/models/issues/issue_list.go index 1419bd123a468..60ef2169b311e 100644 --- a/models/issues/issue_list.go +++ b/models/issues/issue_list.go @@ -312,6 +312,9 @@ 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 diff --git a/models/issues/pull_list.go b/models/issues/pull_list.go index b768965313dc3..e8011a916fb4c 100644 --- a/models/issues/pull_list.go +++ b/models/issues/pull_list.go @@ -174,7 +174,7 @@ func (prs PullRequestList) LoadRepositories(ctx context.Context) error { if err := db.GetEngine(ctx). In("id", repoIDs). Find(&reposMap); err != nil { - return fmt.Errorf("find issues: %w", err) + return fmt.Errorf("find repos: %w", err) } for _, pr := range prs { if pr.BaseRepo == nil { diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index 9f9f9c782cec4..4014fe80f3272 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -118,7 +118,7 @@ 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, "LoadAttributes", err) + ctx.Error(http.StatusInternalServerError, "LoadRepositories", err) return } issueList, err := prs.LoadIssues(ctx) From f4ad036e598b8e7d22dd7bea287cc2b04c574483 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Thu, 9 May 2024 16:16:22 +0800 Subject: [PATCH 11/11] Follow yp05327's suggestion --- models/issues/comment_list.go | 12 +++++------- models/issues/issue_list.go | 14 ++++++-------- services/convert/issue.go | 6 +++--- 3 files changed, 14 insertions(+), 18 deletions(-) diff --git a/models/issues/comment_list.go b/models/issues/comment_list.go index 370b5396e0904..6b4ad80eed59c 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_list.go b/models/issues/issue_list.go index 60ef2169b311e..0dd37a04df230 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, issue.Poster == nil - }) -} - 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 { diff --git a/services/convert/issue.go b/services/convert/issue.go index 69f83c7c15e55..f514dc431351e 100644 --- a/services/convert/issue.go +++ b/services/convert/issue.go @@ -31,9 +31,6 @@ 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{} } @@ -66,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,