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 <teabot@gitea.io>
This commit is contained in:
Lunny Xiao 2023-08-10 10:39:21 +08:00 committed by GitHub
parent a85a862804
commit 36eb3c433a
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
8 changed files with 123 additions and 92 deletions

View file

@ -533,13 +533,12 @@ func (pr *PullRequest) SetMerged(ctx context.Context) (bool, error) {
} }
// NewPullRequest creates new pull request with labels for repository. // 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) { func NewPullRequest(ctx context.Context, repo *repo_model.Repository, issue *Issue, labelIDs []int64, uuids []string, pr *PullRequest) (err error) {
ctx, committer, err := db.TxContext(outerCtx) ctx, committer, err := db.TxContext(ctx)
if err != nil { if err != nil {
return err return err
} }
defer committer.Close() defer committer.Close()
ctx.WithContext(outerCtx)
idx, err := db.GetNextResourceIndex(ctx, "issue_index", repo.ID) idx, err := db.GetNextResourceIndex(ctx, "issue_index", repo.ID)
if err != nil { if err != nil {
@ -948,14 +947,14 @@ func PullRequestCodeOwnersReview(ctx context.Context, pull *Issue, pr *PullReque
for _, u := range uniqUsers { for _, u := range uniqUsers {
if u.ID != pull.Poster.ID { 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) log.Warn("Failed add assignee user: %s to PR review: %s#%d, error: %s", u.Name, pr.BaseRepo.Name, pr.ID, err)
return err return err
} }
} }
} }
for _, t := range uniqTeams { 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) log.Warn("Failed add assignee team: %s to PR review: %s#%d, error: %s", t.Name, pr.BaseRepo.Name, pr.ID, err)
return err return err
} }

View file

@ -88,14 +88,14 @@ func TestLoadRequestedReviewers(t *testing.T) {
user1, err := user_model.GetUserByID(db.DefaultContext, 1) user1, err := user_model.GetUserByID(db.DefaultContext, 1)
assert.NoError(t, err) 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.NoError(t, err)
assert.NotNil(t, comment) assert.NotNil(t, comment)
assert.NoError(t, pull.LoadRequestedReviewers(db.DefaultContext)) assert.NoError(t, pull.LoadRequestedReviewers(db.DefaultContext))
assert.Len(t, pull.RequestedReviewers, 1) 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.NoError(t, err)
assert.NotNil(t, comment) assert.NotNil(t, comment)

View file

@ -560,8 +560,8 @@ func InsertReviews(reviews []*Review) error {
} }
// AddReviewRequest add a review request from one reviewer // AddReviewRequest add a review request from one reviewer
func AddReviewRequest(issue *Issue, reviewer, doer *user_model.User) (*Comment, error) { func AddReviewRequest(ctx context.Context, issue *Issue, reviewer, doer *user_model.User) (*Comment, error) {
ctx, committer, err := db.TxContext(db.DefaultContext) ctx, committer, err := db.TxContext(ctx)
if err != nil { if err != nil {
return nil, err 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 // RemoveReviewRequest remove a review request from one reviewer
func RemoveReviewRequest(issue *Issue, reviewer, doer *user_model.User) (*Comment, error) { func RemoveReviewRequest(ctx context.Context, issue *Issue, reviewer, doer *user_model.User) (*Comment, error) {
ctx, committer, err := db.TxContext(db.DefaultContext) ctx, committer, err := db.TxContext(ctx)
if err != nil { if err != nil {
return nil, err return nil, err
} }
@ -676,8 +676,8 @@ func restoreLatestOfficialReview(ctx context.Context, issueID, reviewerID int64)
} }
// AddTeamReviewRequest add a review request from one team // AddTeamReviewRequest add a review request from one team
func AddTeamReviewRequest(issue *Issue, reviewer *organization.Team, doer *user_model.User) (*Comment, error) { func AddTeamReviewRequest(ctx context.Context, issue *Issue, reviewer *organization.Team, doer *user_model.User) (*Comment, error) {
ctx, committer, err := db.TxContext(db.DefaultContext) ctx, committer, err := db.TxContext(ctx)
if err != nil { if err != nil {
return nil, err return nil, err
} }
@ -735,8 +735,8 @@ func AddTeamReviewRequest(issue *Issue, reviewer *organization.Team, doer *user_
} }
// RemoveTeamReviewRequest remove a review request from one team // RemoveTeamReviewRequest remove a review request from one team
func RemoveTeamReviewRequest(issue *Issue, reviewer *organization.Team, doer *user_model.User) (*Comment, error) { func RemoveTeamReviewRequest(ctx context.Context, issue *Issue, reviewer *organization.Team, doer *user_model.User) (*Comment, error) {
ctx, committer, err := db.TxContext(db.DefaultContext) ctx, committer, err := db.TxContext(ctx)
if err != nil { if err != nil {
return nil, err return nil, err
} }

View file

@ -2290,7 +2290,7 @@ func UpdateIssueAssignee(ctx *context.Context) {
return return
} }
_, _, err = issue_service.ToggleAssignee(ctx, issue, ctx.Doer, assigneeID) _, _, err = issue_service.ToggleAssigneeWithNotify(ctx, issue, ctx.Doer, assigneeID)
if err != nil { if err != nil {
ctx.ServerError("ToggleAssignee", err) ctx.ServerError("ToggleAssignee", err)
return return

View file

@ -33,7 +33,7 @@ func DeleteNotPassedAssignee(ctx context.Context, issue *issues_model.Issue, doe
if !found { if !found {
// This function also does comments and hooks, which is why we call it separately instead of directly removing the assignees here // 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 return err
} }
} }
@ -42,8 +42,8 @@ func DeleteNotPassedAssignee(ctx context.Context, issue *issues_model.Issue, doe
return nil return nil
} }
// ToggleAssignee changes a user between assigned and not assigned for this issue, and make issue comment for it. // ToggleAssigneeWithNoNotify 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) { 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) removed, comment, err = issues_model.ToggleIssueAssignee(ctx, issue, doer, assigneeID)
if err != nil { if err != nil {
return false, nil, err 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. // 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) { func ReviewRequest(ctx context.Context, issue *issues_model.Issue, doer, reviewer *user_model.User, isAdd bool) (comment *issues_model.Comment, err error) {
if isAdd { if isAdd {
comment, err = issues_model.AddReviewRequest(issue, reviewer, doer) comment, err = issues_model.AddReviewRequest(ctx, issue, reviewer, doer)
} else { } else {
comment, err = issues_model.RemoveReviewRequest(issue, reviewer, doer) comment, err = issues_model.RemoveReviewRequest(ctx, issue, reviewer, doer)
} }
if err != nil { 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. // 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) { 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 { if isAdd {
comment, err = issues_model.AddTeamReviewRequest(issue, reviewer, doer) comment, err = issues_model.AddTeamReviewRequest(ctx, issue, reviewer, doer)
} else { } else {
comment, err = issues_model.RemoveTeamReviewRequest(issue, reviewer, doer) comment, err = issues_model.RemoveTeamReviewRequest(ctx, issue, reviewer, doer)
} }
if err != nil { if err != nil {

View file

@ -27,7 +27,7 @@ func NewIssue(ctx context.Context, repo *repo_model.Repository, issue *issues_mo
} }
for _, assigneeID := range assigneeIDs { 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 return err
} }
} }
@ -128,7 +128,7 @@ func UpdateAssignees(ctx context.Context, issue *issues_model.Issue, oneAssignee
// has access to the repo. // has access to the repo.
for _, assignee := range allNewAssignees { for _, assignee := range allNewAssignees {
// Extra method to prevent double adding (which would result in removing) // 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 { if err != nil {
return err 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. // AddAssigneeIfNotAssigned adds an assignee only if he isn't already assigned to the issue.
// Also checks for access of assigned user // 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) assignee, err := user_model.GetUserByID(ctx, assigneeID)
if err != nil { if err != nil {
return err return nil, err
} }
// Check if the user is already assigned // Check if the user is already assigned
isAssigned, err := issues_model.IsUserAssignedToIssue(ctx, issue, assignee) isAssigned, err := issues_model.IsUserAssignedToIssue(ctx, issue, assignee)
if err != nil { if err != nil {
return err return nil, err
} }
if isAssigned { if isAssigned {
// nothing to to // nothing to to
return nil return nil, nil
} }
valid, err := access_model.CanBeAssigned(ctx, assignee, issue.Repo, issue.IsPull) valid, err := access_model.CanBeAssigned(ctx, assignee, issue.Repo, issue.IsPull)
if err != nil { if err != nil {
return err return nil, err
} }
if !valid { 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 notify {
if err != nil { _, comment, err = ToggleAssigneeWithNotify(ctx, issue, doer, assigneeID)
return err return comment, err
} }
_, comment, err = issues_model.ToggleIssueAssignee(ctx, issue, doer, assigneeID)
return nil return comment, err
} }
// GetRefEndNamesAndURLs retrieves the ref end names (e.g. refs/heads/branch-name -> branch-name) // GetRefEndNamesAndURLs retrieves the ref end names (e.g. refs/heads/branch-name -> branch-name)

View file

@ -62,14 +62,19 @@ func TestPatch(pr *issues_model.PullRequest) error {
ctx, _, finished := process.GetManager().AddContext(graceful.GetManager().HammerContext(), fmt.Sprintf("TestPatch: %s", pr)) ctx, _, finished := process.GetManager().AddContext(graceful.GetManager().HammerContext(), fmt.Sprintf("TestPatch: %s", pr))
defer finished() defer finished()
// Clone base repo.
prCtx, cancel, err := createTemporaryRepoForPR(ctx, pr) prCtx, cancel, err := createTemporaryRepoForPR(ctx, pr)
if err != nil { if err != nil {
log.Error("createTemporaryRepoForPR %-v: %v", pr, err) if !git_model.IsErrBranchNotExist(err) {
log.Error("CreateTemporaryRepoForPR %-v: %v", pr, err)
}
return err return err
} }
defer cancel() 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) gitRepo, err := git.OpenRepository(ctx, prCtx.tmpBasePath)
if err != nil { if err != nil {
return fmt.Errorf("OpenRepository: %w", err) return fmt.Errorf("OpenRepository: %w", err)

View file

@ -26,7 +26,6 @@ import (
"code.gitea.io/gitea/modules/json" "code.gitea.io/gitea/modules/json"
"code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/log"
"code.gitea.io/gitea/modules/notification" "code.gitea.io/gitea/modules/notification"
"code.gitea.io/gitea/modules/process"
repo_module "code.gitea.io/gitea/modules/repository" repo_module "code.gitea.io/gitea/modules/repository"
"code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/setting"
"code.gitea.io/gitea/modules/sync" "code.gitea.io/gitea/modules/sync"
@ -38,73 +37,70 @@ import (
var pullWorkingPool = sync.NewExclusivePool() var pullWorkingPool = sync.NewExclusivePool()
// NewPullRequest creates new pull request with labels for repository. // 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 { func NewPullRequest(ctx context.Context, repo *repo_model.Repository, issue *issues_model.Issue, labelIDs []int64, uuids []string, pr *issues_model.PullRequest, assigneeIDs []int64) error {
if err := TestPatch(pr); err != nil { 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 return err
} }
divergence, err := GetDiverging(ctx, pr) divergence, err := git.GetDivergingCommits(ctx, prCtx.tmpBasePath, baseBranch, trackingBranch)
if err != nil { if err != nil {
return err return err
} }
pr.CommitsAhead = divergence.Ahead pr.CommitsAhead = divergence.Ahead
pr.CommitsBehind = divergence.Behind pr.CommitsBehind = divergence.Behind
if err := issues_model.NewPullRequest(ctx, repo, pull, labelIDs, uuids, pr); err != nil { assigneeCommentMap := make(map[int64]*issues_model.Comment)
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)
}
// add first push codes 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 { if err != nil {
return err return err
} }
defer baseGitRepo.Close() defer baseGitRepo.Close()
compareInfo, err := baseGitRepo.GetCompareInfo(pr.BaseRepo.RepoPath(), if err := db.WithTx(ctx, func(ctx context.Context) error {
git.BranchPrefix+pr.BaseBranch, pr.GetGitRefName(), false, false) if err := issues_model.NewPullRequest(ctx, repo, issue, labelIDs, uuids, pr); err != nil {
if err != nil { return err
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 := issues_model.PushActionContent{IsForcePush: false}
data.CommitIDs = make([]string, 0, len(compareInfo.Commits)) data.CommitIDs = make([]string, 0, len(compareInfo.Commits))
for i := len(compareInfo.Commits) - 1; i >= 0; i-- { 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{ ops := &issues_model.CreateCommentOptions{
Type: issues_model.CommentTypePullRequestPush, Type: issues_model.CommentTypePullRequestPush,
Doer: pull.Poster, Doer: issue.Poster,
Repo: repo, Repo: repo,
Issue: pr.Issue, Issue: pr.Issue,
IsForcePush: false, IsForcePush: false,
Content: string(dataJSON), Content: string(dataJSON),
} }
_, _ = issues_model.CreateComment(ctx, ops) if _, err = issues_model.CreateComment(ctx, ops); err != nil {
return err
}
if !pr.IsWorkInProgress() { 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 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 return nil