From 36eb3c433ae384f21beec63eb648141fb9dba676 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Thu, 10 Aug 2023 10:39:21 +0800 Subject: [PATCH] Add transaction when creating pull request created dirty data (#26259) Fix #26129 Replace #26258 This PR will introduce a transaction on creating pull request so that if some step failed, it will rollback totally. And there will be no dirty pull request exist. --------- Co-authored-by: Giteabot --- models/issues/pull.go | 9 ++- models/issues/pull_test.go | 4 +- models/issues/review.go | 16 ++--- routers/web/repo/issue.go | 2 +- services/issue/assignee.go | 14 ++-- services/issue/issue.go | 26 +++---- services/pull/patch.go | 9 ++- services/pull/pull.go | 135 ++++++++++++++++++++++--------------- 8 files changed, 123 insertions(+), 92 deletions(-) diff --git a/models/issues/pull.go b/models/issues/pull.go index cedbb62c3..676224a3d 100644 --- a/models/issues/pull.go +++ b/models/issues/pull.go @@ -533,13 +533,12 @@ func (pr *PullRequest) SetMerged(ctx context.Context) (bool, error) { } // NewPullRequest creates new pull request with labels for repository. -func NewPullRequest(outerCtx context.Context, repo *repo_model.Repository, issue *Issue, labelIDs []int64, uuids []string, pr *PullRequest) (err error) { - ctx, committer, err := db.TxContext(outerCtx) +func NewPullRequest(ctx context.Context, repo *repo_model.Repository, issue *Issue, labelIDs []int64, uuids []string, pr *PullRequest) (err error) { + ctx, committer, err := db.TxContext(ctx) if err != nil { return err } defer committer.Close() - ctx.WithContext(outerCtx) idx, err := db.GetNextResourceIndex(ctx, "issue_index", repo.ID) if err != nil { @@ -948,14 +947,14 @@ func PullRequestCodeOwnersReview(ctx context.Context, pull *Issue, pr *PullReque for _, u := range uniqUsers { if u.ID != pull.Poster.ID { - if _, err := AddReviewRequest(pull, u, pull.Poster); err != nil { + if _, err := AddReviewRequest(ctx, pull, u, pull.Poster); err != nil { log.Warn("Failed add assignee user: %s to PR review: %s#%d, error: %s", u.Name, pr.BaseRepo.Name, pr.ID, err) return err } } } for _, t := range uniqTeams { - if _, err := AddTeamReviewRequest(pull, t, pull.Poster); err != nil { + if _, err := AddTeamReviewRequest(ctx, pull, t, pull.Poster); err != nil { log.Warn("Failed add assignee team: %s to PR review: %s#%d, error: %s", t.Name, pr.BaseRepo.Name, pr.ID, err) return err } diff --git a/models/issues/pull_test.go b/models/issues/pull_test.go index 0990a3b87..fa1f551ad 100644 --- a/models/issues/pull_test.go +++ b/models/issues/pull_test.go @@ -88,14 +88,14 @@ func TestLoadRequestedReviewers(t *testing.T) { user1, err := user_model.GetUserByID(db.DefaultContext, 1) assert.NoError(t, err) - comment, err := issues_model.AddReviewRequest(issue, user1, &user_model.User{}) + comment, err := issues_model.AddReviewRequest(db.DefaultContext, issue, user1, &user_model.User{}) assert.NoError(t, err) assert.NotNil(t, comment) assert.NoError(t, pull.LoadRequestedReviewers(db.DefaultContext)) assert.Len(t, pull.RequestedReviewers, 1) - comment, err = issues_model.RemoveReviewRequest(issue, user1, &user_model.User{}) + comment, err = issues_model.RemoveReviewRequest(db.DefaultContext, issue, user1, &user_model.User{}) assert.NoError(t, err) assert.NotNil(t, comment) diff --git a/models/issues/review.go b/models/issues/review.go index cae3ef1d3..931f1a2ba 100644 --- a/models/issues/review.go +++ b/models/issues/review.go @@ -560,8 +560,8 @@ func InsertReviews(reviews []*Review) error { } // AddReviewRequest add a review request from one reviewer -func AddReviewRequest(issue *Issue, reviewer, doer *user_model.User) (*Comment, error) { - ctx, committer, err := db.TxContext(db.DefaultContext) +func AddReviewRequest(ctx context.Context, issue *Issue, reviewer, doer *user_model.User) (*Comment, error) { + ctx, committer, err := db.TxContext(ctx) if err != nil { return nil, err } @@ -615,8 +615,8 @@ func AddReviewRequest(issue *Issue, reviewer, doer *user_model.User) (*Comment, } // RemoveReviewRequest remove a review request from one reviewer -func RemoveReviewRequest(issue *Issue, reviewer, doer *user_model.User) (*Comment, error) { - ctx, committer, err := db.TxContext(db.DefaultContext) +func RemoveReviewRequest(ctx context.Context, issue *Issue, reviewer, doer *user_model.User) (*Comment, error) { + ctx, committer, err := db.TxContext(ctx) if err != nil { return nil, err } @@ -676,8 +676,8 @@ func restoreLatestOfficialReview(ctx context.Context, issueID, reviewerID int64) } // AddTeamReviewRequest add a review request from one team -func AddTeamReviewRequest(issue *Issue, reviewer *organization.Team, doer *user_model.User) (*Comment, error) { - ctx, committer, err := db.TxContext(db.DefaultContext) +func AddTeamReviewRequest(ctx context.Context, issue *Issue, reviewer *organization.Team, doer *user_model.User) (*Comment, error) { + ctx, committer, err := db.TxContext(ctx) if err != nil { return nil, err } @@ -735,8 +735,8 @@ func AddTeamReviewRequest(issue *Issue, reviewer *organization.Team, doer *user_ } // RemoveTeamReviewRequest remove a review request from one team -func RemoveTeamReviewRequest(issue *Issue, reviewer *organization.Team, doer *user_model.User) (*Comment, error) { - ctx, committer, err := db.TxContext(db.DefaultContext) +func RemoveTeamReviewRequest(ctx context.Context, issue *Issue, reviewer *organization.Team, doer *user_model.User) (*Comment, error) { + ctx, committer, err := db.TxContext(ctx) if err != nil { return nil, err } diff --git a/routers/web/repo/issue.go b/routers/web/repo/issue.go index 7bddabd10..488c97b0e 100644 --- a/routers/web/repo/issue.go +++ b/routers/web/repo/issue.go @@ -2290,7 +2290,7 @@ func UpdateIssueAssignee(ctx *context.Context) { return } - _, _, err = issue_service.ToggleAssignee(ctx, issue, ctx.Doer, assigneeID) + _, _, err = issue_service.ToggleAssigneeWithNotify(ctx, issue, ctx.Doer, assigneeID) if err != nil { ctx.ServerError("ToggleAssignee", err) return diff --git a/services/issue/assignee.go b/services/issue/assignee.go index 8fe35b560..9b0445d29 100644 --- a/services/issue/assignee.go +++ b/services/issue/assignee.go @@ -33,7 +33,7 @@ func DeleteNotPassedAssignee(ctx context.Context, issue *issues_model.Issue, doe if !found { // This function also does comments and hooks, which is why we call it separately instead of directly removing the assignees here - if _, _, err := ToggleAssignee(ctx, issue, doer, assignee.ID); err != nil { + if _, _, err := ToggleAssigneeWithNotify(ctx, issue, doer, assignee.ID); err != nil { return err } } @@ -42,8 +42,8 @@ func DeleteNotPassedAssignee(ctx context.Context, issue *issues_model.Issue, doe return nil } -// ToggleAssignee changes a user between assigned and not assigned for this issue, and make issue comment for it. -func ToggleAssignee(ctx context.Context, issue *issues_model.Issue, doer *user_model.User, assigneeID int64) (removed bool, comment *issues_model.Comment, err error) { +// ToggleAssigneeWithNoNotify changes a user between assigned and not assigned for this issue, and make issue comment for it. +func ToggleAssigneeWithNotify(ctx context.Context, issue *issues_model.Issue, doer *user_model.User, assigneeID int64) (removed bool, comment *issues_model.Comment, err error) { removed, comment, err = issues_model.ToggleIssueAssignee(ctx, issue, doer, assigneeID) if err != nil { return false, nil, err @@ -62,9 +62,9 @@ func ToggleAssignee(ctx context.Context, issue *issues_model.Issue, doer *user_m // ReviewRequest add or remove a review request from a user for this PR, and make comment for it. func ReviewRequest(ctx context.Context, issue *issues_model.Issue, doer, reviewer *user_model.User, isAdd bool) (comment *issues_model.Comment, err error) { if isAdd { - comment, err = issues_model.AddReviewRequest(issue, reviewer, doer) + comment, err = issues_model.AddReviewRequest(ctx, issue, reviewer, doer) } else { - comment, err = issues_model.RemoveReviewRequest(issue, reviewer, doer) + comment, err = issues_model.RemoveReviewRequest(ctx, issue, reviewer, doer) } if err != nil { @@ -229,9 +229,9 @@ func IsValidTeamReviewRequest(ctx context.Context, reviewer *organization.Team, // TeamReviewRequest add or remove a review request from a team for this PR, and make comment for it. func TeamReviewRequest(ctx context.Context, issue *issues_model.Issue, doer *user_model.User, reviewer *organization.Team, isAdd bool) (comment *issues_model.Comment, err error) { if isAdd { - comment, err = issues_model.AddTeamReviewRequest(issue, reviewer, doer) + comment, err = issues_model.AddTeamReviewRequest(ctx, issue, reviewer, doer) } else { - comment, err = issues_model.RemoveTeamReviewRequest(issue, reviewer, doer) + comment, err = issues_model.RemoveTeamReviewRequest(ctx, issue, reviewer, doer) } if err != nil { diff --git a/services/issue/issue.go b/services/issue/issue.go index 9ca4e21b1..35409589e 100644 --- a/services/issue/issue.go +++ b/services/issue/issue.go @@ -27,7 +27,7 @@ func NewIssue(ctx context.Context, repo *repo_model.Repository, issue *issues_mo } for _, assigneeID := range assigneeIDs { - if err := AddAssigneeIfNotAssigned(ctx, issue, issue.Poster, assigneeID); err != nil { + if _, err := AddAssigneeIfNotAssigned(ctx, issue, issue.Poster, assigneeID, true); err != nil { return err } } @@ -128,7 +128,7 @@ func UpdateAssignees(ctx context.Context, issue *issues_model.Issue, oneAssignee // has access to the repo. for _, assignee := range allNewAssignees { // Extra method to prevent double adding (which would result in removing) - err = AddAssigneeIfNotAssigned(ctx, issue, doer, assignee.ID) + _, err = AddAssigneeIfNotAssigned(ctx, issue, doer, assignee.ID, true) if err != nil { return err } @@ -173,36 +173,36 @@ func DeleteIssue(ctx context.Context, doer *user_model.User, gitRepo *git.Reposi // AddAssigneeIfNotAssigned adds an assignee only if he isn't already assigned to the issue. // Also checks for access of assigned user -func AddAssigneeIfNotAssigned(ctx context.Context, issue *issues_model.Issue, doer *user_model.User, assigneeID int64) (err error) { +func AddAssigneeIfNotAssigned(ctx context.Context, issue *issues_model.Issue, doer *user_model.User, assigneeID int64, notify bool) (comment *issues_model.Comment, err error) { assignee, err := user_model.GetUserByID(ctx, assigneeID) if err != nil { - return err + return nil, err } // Check if the user is already assigned isAssigned, err := issues_model.IsUserAssignedToIssue(ctx, issue, assignee) if err != nil { - return err + return nil, err } if isAssigned { // nothing to to - return nil + return nil, nil } valid, err := access_model.CanBeAssigned(ctx, assignee, issue.Repo, issue.IsPull) if err != nil { - return err + return nil, err } if !valid { - return repo_model.ErrUserDoesNotHaveAccessToRepo{UserID: assigneeID, RepoName: issue.Repo.Name} + return nil, repo_model.ErrUserDoesNotHaveAccessToRepo{UserID: assigneeID, RepoName: issue.Repo.Name} } - _, _, err = ToggleAssignee(ctx, issue, doer, assigneeID) - if err != nil { - return err + if notify { + _, comment, err = ToggleAssigneeWithNotify(ctx, issue, doer, assigneeID) + return comment, err } - - return nil + _, comment, err = issues_model.ToggleIssueAssignee(ctx, issue, doer, assigneeID) + return comment, err } // GetRefEndNamesAndURLs retrieves the ref end names (e.g. refs/heads/branch-name -> branch-name) diff --git a/services/pull/patch.go b/services/pull/patch.go index 927735572..688cbcc02 100644 --- a/services/pull/patch.go +++ b/services/pull/patch.go @@ -62,14 +62,19 @@ func TestPatch(pr *issues_model.PullRequest) error { ctx, _, finished := process.GetManager().AddContext(graceful.GetManager().HammerContext(), fmt.Sprintf("TestPatch: %s", pr)) defer finished() - // Clone base repo. prCtx, cancel, err := createTemporaryRepoForPR(ctx, pr) if err != nil { - log.Error("createTemporaryRepoForPR %-v: %v", pr, err) + if !git_model.IsErrBranchNotExist(err) { + log.Error("CreateTemporaryRepoForPR %-v: %v", pr, err) + } return err } defer cancel() + return testPatch(ctx, prCtx, pr) +} + +func testPatch(ctx context.Context, prCtx *prContext, pr *issues_model.PullRequest) error { gitRepo, err := git.OpenRepository(ctx, prCtx.tmpBasePath) if err != nil { return fmt.Errorf("OpenRepository: %w", err) diff --git a/services/pull/pull.go b/services/pull/pull.go index 8730b9684..0b6194b14 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -26,7 +26,6 @@ import ( "code.gitea.io/gitea/modules/json" "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/notification" - "code.gitea.io/gitea/modules/process" repo_module "code.gitea.io/gitea/modules/repository" "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/sync" @@ -38,73 +37,70 @@ import ( var pullWorkingPool = sync.NewExclusivePool() // NewPullRequest creates new pull request with labels for repository. -func NewPullRequest(ctx context.Context, repo *repo_model.Repository, pull *issues_model.Issue, labelIDs []int64, uuids []string, pr *issues_model.PullRequest, assigneeIDs []int64) error { - if err := TestPatch(pr); err != nil { +func NewPullRequest(ctx context.Context, repo *repo_model.Repository, issue *issues_model.Issue, labelIDs []int64, uuids []string, pr *issues_model.PullRequest, assigneeIDs []int64) error { + prCtx, cancel, err := createTemporaryRepoForPR(ctx, pr) + if err != nil { + if !git_model.IsErrBranchNotExist(err) { + log.Error("CreateTemporaryRepoForPR %-v: %v", pr, err) + } + return err + } + defer cancel() + + if err := testPatch(ctx, prCtx, pr); err != nil { return err } - divergence, err := GetDiverging(ctx, pr) + divergence, err := git.GetDivergingCommits(ctx, prCtx.tmpBasePath, baseBranch, trackingBranch) if err != nil { return err } pr.CommitsAhead = divergence.Ahead pr.CommitsBehind = divergence.Behind - if err := issues_model.NewPullRequest(ctx, repo, pull, labelIDs, uuids, pr); err != nil { - return err - } - - for _, assigneeID := range assigneeIDs { - if err := issue_service.AddAssigneeIfNotAssigned(ctx, pull, pull.Poster, assigneeID); err != nil { - return err - } - } - - pr.Issue = pull - pull.PullRequest = pr - - // Now - even if the request context has been cancelled as the PR has been created - // in the db and there is no way to cancel that transaction we have to proceed - therefore - // create new context and work from there - prCtx, _, finished := process.GetManager().AddContext(graceful.GetManager().HammerContext(), fmt.Sprintf("NewPullRequest: %s:%d", repo.FullName(), pr.Index)) - defer finished() - - if pr.Flow == issues_model.PullRequestFlowGithub { - err = PushToBaseRepo(prCtx, pr) - } else { - err = UpdateRef(prCtx, pr) - } - if err != nil { - return err - } - - mentions, err := issues_model.FindAndUpdateIssueMentions(ctx, pull, pull.Poster, pull.Content) - if err != nil { - return err - } - - notification.NotifyNewPullRequest(prCtx, pr, mentions) - if len(pull.Labels) > 0 { - notification.NotifyIssueChangeLabels(prCtx, pull.Poster, pull, pull.Labels, nil) - } - if pull.Milestone != nil { - notification.NotifyIssueChangeMilestone(prCtx, pull.Poster, pull, 0) - } + assigneeCommentMap := make(map[int64]*issues_model.Comment) // add first push codes comment - baseGitRepo, err := git.OpenRepository(prCtx, pr.BaseRepo.RepoPath()) + baseGitRepo, err := git.OpenRepository(ctx, pr.BaseRepo.RepoPath()) if err != nil { return err } defer baseGitRepo.Close() - compareInfo, err := baseGitRepo.GetCompareInfo(pr.BaseRepo.RepoPath(), - git.BranchPrefix+pr.BaseBranch, pr.GetGitRefName(), false, false) - if err != nil { - return err - } + if err := db.WithTx(ctx, func(ctx context.Context) error { + if err := issues_model.NewPullRequest(ctx, repo, issue, labelIDs, uuids, pr); err != nil { + return err + } + + for _, assigneeID := range assigneeIDs { + comment, err := issue_service.AddAssigneeIfNotAssigned(ctx, issue, issue.Poster, assigneeID, false) + if err != nil { + return err + } + assigneeCommentMap[assigneeID] = comment + } + + pr.Issue = issue + issue.PullRequest = pr + + if pr.Flow == issues_model.PullRequestFlowGithub { + err = PushToBaseRepo(ctx, pr) + } else { + err = UpdateRef(ctx, pr) + } + if err != nil { + return err + } + + compareInfo, err := baseGitRepo.GetCompareInfo(pr.BaseRepo.RepoPath(), + git.BranchPrefix+pr.BaseBranch, pr.GetGitRefName(), false, false) + if err != nil { + return err + } + if len(compareInfo.Commits) == 0 { + return nil + } - if len(compareInfo.Commits) > 0 { data := issues_model.PushActionContent{IsForcePush: false} data.CommitIDs = make([]string, 0, len(compareInfo.Commits)) for i := len(compareInfo.Commits) - 1; i >= 0; i-- { @@ -118,21 +114,52 @@ func NewPullRequest(ctx context.Context, repo *repo_model.Repository, pull *issu ops := &issues_model.CreateCommentOptions{ Type: issues_model.CommentTypePullRequestPush, - Doer: pull.Poster, + Doer: issue.Poster, Repo: repo, Issue: pr.Issue, IsForcePush: false, Content: string(dataJSON), } - _, _ = issues_model.CreateComment(ctx, ops) + if _, err = issues_model.CreateComment(ctx, ops); err != nil { + return err + } if !pr.IsWorkInProgress() { - if err := issues_model.PullRequestCodeOwnersReview(ctx, pull, pr); err != nil { + if err := issues_model.PullRequestCodeOwnersReview(ctx, issue, pr); err != nil { return err } } + return nil + }); err != nil { + // cleanup: this will only remove the reference, the real commit will be clean up when next GC + if err1 := baseGitRepo.RemoveReference(pr.GetGitRefName()); err1 != nil { + log.Error("RemoveReference: %v", err1) + } + return err + } + baseGitRepo.Close() // close immediately to avoid notifications will open the repository again + mentions, err := issues_model.FindAndUpdateIssueMentions(ctx, issue, issue.Poster, issue.Content) + if err != nil { + return err + } + + notification.NotifyNewPullRequest(ctx, pr, mentions) + if len(issue.Labels) > 0 { + notification.NotifyIssueChangeLabels(ctx, issue.Poster, issue, issue.Labels, nil) + } + if issue.Milestone != nil { + notification.NotifyIssueChangeMilestone(ctx, issue.Poster, issue, 0) + } + if len(assigneeIDs) > 0 { + for _, assigneeID := range assigneeIDs { + assignee, err := user_model.GetUserByID(ctx, assigneeID) + if err != nil { + return ErrDependenciesLeft + } + notification.NotifyIssueChangeAssignee(ctx, issue.Poster, issue, assignee, false, assigneeCommentMap[assigneeID]) + } } return nil