Add transaction when creating pull request created dirty data (go-gitea#26259)

lunny · GiteaBot · web-flow · commit 36eb3c433ae3 · 2023-08-10T02:39:21.000Z
Fix go-gitea#26129 Replace go-gitea#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>
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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

Original file line number	Diff line number	Diff line change
`@@ -533,13 +533,12 @@ func (pr *PullRequest) SetMerged(ctx context.Context) (bool, error) {`
`533`	`533`	`}`
`534`	`534`
`535`	`535`	`// NewPullRequest creates new pull request with labels for repository.`
`536`		`-func NewPullRequest(outerCtx context.Context, repo repo_model.Repository, issue Issue, labelIDs []int64, uuids []string, pr *PullRequest) (err error) {`
`537`		`- ctx, committer, err := db.TxContext(outerCtx)`
	`536`	`+func NewPullRequest(ctx context.Context, repo repo_model.Repository, issue Issue, labelIDs []int64, uuids []string, pr *PullRequest) (err error) {`
	`537`	`+ ctx, committer, err := db.TxContext(ctx)`
`538`	`538`	`if err != nil {`
`539`	`539`	`return err`
`540`	`540`	`}`
`541`	`541`	`defer committer.Close()`
`542`		`- ctx.WithContext(outerCtx)`
`543`	`542`
`544`	`543`	`idx, err := db.GetNextResourceIndex(ctx, "issue_index", repo.ID)`
`545`	`544`	`if err != nil {`
`@@ -948,14 +947,14 @@ func PullRequestCodeOwnersReview(ctx context.Context, pull Issue, pr PullReque`
`948`	`947`
`949`	`948`	`for _, u := range uniqUsers {`
`950`	`949`	`if u.ID != pull.Poster.ID {`
`951`		`- if _, err := AddReviewRequest(pull, u, pull.Poster); err != nil {`
	`950`	`+ if _, err := AddReviewRequest(ctx, pull, u, pull.Poster); err != nil {`
`952`	`951`	`log.Warn("Failed add assignee user: %s to PR review: %s#%d, error: %s", u.Name, pr.BaseRepo.Name, pr.ID, err)`
`953`	`952`	`return err`
`954`	`953`	`}`
`955`	`954`	`}`
`956`	`955`	`}`
`957`	`956`	`for _, t := range uniqTeams {`
`958`		`- if _, err := AddTeamReviewRequest(pull, t, pull.Poster); err != nil {`
	`957`	`+ if _, err := AddTeamReviewRequest(ctx, pull, t, pull.Poster); err != nil {`
`959`	`958`	`log.Warn("Failed add assignee team: %s to PR review: %s#%d, error: %s", t.Name, pr.BaseRepo.Name, pr.ID, err)`
`960`	`959`	`return err`
`961`	`960`	`}`
Original file line number	Diff line number	Diff line change
`@@ -560,8 +560,8 @@ func InsertReviews(reviews []*Review) error {`
`560`	`560`	`}`
`561`	`561`
`562`	`562`	`// AddReviewRequest add a review request from one reviewer`
`563`		`-func AddReviewRequest(issue Issue, reviewer, doer user_model.User) (*Comment, error) {`
`564`		`- ctx, committer, err := db.TxContext(db.DefaultContext)`
	`563`	`+func AddReviewRequest(ctx context.Context, issue Issue, reviewer, doer user_model.User) (*Comment, error) {`
	`564`	`+ ctx, committer, err := db.TxContext(ctx)`
`565`	`565`	`if err != nil {`
`566`	`566`	`return nil, err`
`567`	`567`	`}`
`@@ -615,8 +615,8 @@ func AddReviewRequest(issue Issue, reviewer, doer user_model.User) (*Comment,`
`615`	`615`	`}`
`616`	`616`
`617`	`617`	`// RemoveReviewRequest remove a review request from one reviewer`
`618`		`-func RemoveReviewRequest(issue Issue, reviewer, doer user_model.User) (*Comment, error) {`
`619`		`- ctx, committer, err := db.TxContext(db.DefaultContext)`
	`618`	`+func RemoveReviewRequest(ctx context.Context, issue Issue, reviewer, doer user_model.User) (*Comment, error) {`
	`619`	`+ ctx, committer, err := db.TxContext(ctx)`
`620`	`620`	`if err != nil {`
`621`	`621`	`return nil, err`
`622`	`622`	`}`
`@@ -676,8 +676,8 @@ func restoreLatestOfficialReview(ctx context.Context, issueID, reviewerID int64)`
`676`	`676`	`}`
`677`	`677`
`678`	`678`	`// AddTeamReviewRequest add a review request from one team`
`679`		`-func AddTeamReviewRequest(issue Issue, reviewer organization.Team, doer user_model.User) (Comment, error) {`
`680`		`- ctx, committer, err := db.TxContext(db.DefaultContext)`
	`679`	`+func AddTeamReviewRequest(ctx context.Context, issue Issue, reviewer organization.Team, doer user_model.User) (Comment, error) {`
	`680`	`+ ctx, committer, err := db.TxContext(ctx)`
`681`	`681`	`if err != nil {`
`682`	`682`	`return nil, err`
`683`	`683`	`}`
`@@ -735,8 +735,8 @@ func AddTeamReviewRequest(issue Issue, reviewer organization.Team, doer *user_`
`735`	`735`	`}`
`736`	`736`
`737`	`737`	`// RemoveTeamReviewRequest remove a review request from one team`
`738`		`-func RemoveTeamReviewRequest(issue Issue, reviewer organization.Team, doer user_model.User) (Comment, error) {`
`739`		`- ctx, committer, err := db.TxContext(db.DefaultContext)`
	`738`	`+func RemoveTeamReviewRequest(ctx context.Context, issue Issue, reviewer organization.Team, doer user_model.User) (Comment, error) {`
	`739`	`+ ctx, committer, err := db.TxContext(ctx)`
`740`	`740`	`if err != nil {`
`741`	`741`	`return nil, err`
`742`	`742`	`}`
Original file line number	Diff line number	Diff line change
`@@ -2290,7 +2290,7 @@ func UpdateIssueAssignee(ctx *context.Context) {`
`2290`	`2290`	`return`
`2291`	`2291`	`}`
`2292`	`2292`
`2293`		`- _, _, err = issue_service.ToggleAssignee(ctx, issue, ctx.Doer, assigneeID)`
	`2293`	`+ _, _, err = issue_service.ToggleAssigneeWithNotify(ctx, issue, ctx.Doer, assigneeID)`
`2294`	`2294`	`if err != nil {`
`2295`	`2295`	`ctx.ServerError("ToggleAssignee", err)`
`2296`	`2296`	`return`
Original file line number	Diff line number	Diff line change
`@@ -27,7 +27,7 @@ func NewIssue(ctx context.Context, repo repo_model.Repository, issue issues_mo`
`27`	`27`	`}`
`28`	`28`
`29`	`29`	`for _, assigneeID := range assigneeIDs {`
`30`		`- if err := AddAssigneeIfNotAssigned(ctx, issue, issue.Poster, assigneeID); err != nil {`
	`30`	`+ if _, err := AddAssigneeIfNotAssigned(ctx, issue, issue.Poster, assigneeID, true); err != nil {`
`31`	`31`	`return err`
`32`	`32`	`}`
`33`	`33`	`}`
`@@ -128,7 +128,7 @@ func UpdateAssignees(ctx context.Context, issue *issues_model.Issue, oneAssignee`
`128`	`128`	`// has access to the repo.`
`129`	`129`	`for _, assignee := range allNewAssignees {`
`130`	`130`	`// Extra method to prevent double adding (which would result in removing)`
`131`		`- err = AddAssigneeIfNotAssigned(ctx, issue, doer, assignee.ID)`
	`131`	`+ _, err = AddAssigneeIfNotAssigned(ctx, issue, doer, assignee.ID, true)`
`132`	`132`	`if err != nil {`
`133`	`133`	`return err`
`134`	`134`	`}`
`@@ -173,36 +173,36 @@ func DeleteIssue(ctx context.Context, doer user_model.User, gitRepo git.Reposi`
`173`	`173`
`174`	`174`	`// AddAssigneeIfNotAssigned adds an assignee only if he isn't already assigned to the issue.`
`175`	`175`	`// Also checks for access of assigned user`
`176`		`-func AddAssigneeIfNotAssigned(ctx context.Context, issue issues_model.Issue, doer user_model.User, assigneeID int64) (err error) {`
	`176`	`+func AddAssigneeIfNotAssigned(ctx context.Context, issue issues_model.Issue, doer user_model.User, assigneeID int64, notify bool) (comment *issues_model.Comment, err error) {`
`177`	`177`	`assignee, err := user_model.GetUserByID(ctx, assigneeID)`
`178`	`178`	`if err != nil {`
`179`		`- return err`
	`179`	`+ return nil, err`
`180`	`180`	`}`
`181`	`181`
`182`	`182`	`// Check if the user is already assigned`
`183`	`183`	`isAssigned, err := issues_model.IsUserAssignedToIssue(ctx, issue, assignee)`
`184`	`184`	`if err != nil {`
`185`		`- return err`
	`185`	`+ return nil, err`
`186`	`186`	`}`
`187`	`187`	`if isAssigned {`
`188`	`188`	`// nothing to to`
`189`		`- return nil`
	`189`	`+ return nil, nil`
`190`	`190`	`}`
`191`	`191`
`192`	`192`	`valid, err := access_model.CanBeAssigned(ctx, assignee, issue.Repo, issue.IsPull)`
`193`	`193`	`if err != nil {`
`194`		`- return err`
	`194`	`+ return nil, err`
`195`	`195`	`}`
`196`	`196`	`if !valid {`
`197`		`- return repo_model.ErrUserDoesNotHaveAccessToRepo{UserID: assigneeID, RepoName: issue.Repo.Name}`
	`197`	`+ return nil, repo_model.ErrUserDoesNotHaveAccessToRepo{UserID: assigneeID, RepoName: issue.Repo.Name}`
`198`	`198`	`}`
`199`	`199`
`200`		`- _, _, err = ToggleAssignee(ctx, issue, doer, assigneeID)`
`201`		`- if err != nil {`
`202`		`- return err`
	`200`	`+ if notify {`
	`201`	`+ _, comment, err = ToggleAssigneeWithNotify(ctx, issue, doer, assigneeID)`
	`202`	`+ return comment, err`
`203`	`203`	`}`
`204`		`-`
`205`		`- return nil`
	`204`	`+ _, comment, err = issues_model.ToggleIssueAssignee(ctx, issue, doer, assigneeID)`
	`205`	`+ return comment, err`
`206`	`206`	`}`
`207`	`207`
`208`	`208`	`// GetRefEndNamesAndURLs retrieves the ref end names (e.g. refs/heads/branch-name -> branch-name)`