Skip to content

Commit 368e9e0

Browse files
lunnyGiteaBot
lunny
and
GiteaBot
authored
Add transaction when creating pull request created dirty data (#26259) (#26437)
Backport #26259 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 <[email protected]>
1 parent d6cf261 commit 368e9e0

File tree

8 files changed

+120
-87
lines changed

8 files changed

+120
-87
lines changed

‎models/issues/pull.go

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -531,13 +531,12 @@ func (pr *PullRequest) SetMerged(ctx context.Context) (bool, error) {
531531
}
532532

533533
// NewPullRequest creates new pull request with labels for repository.
534-
func NewPullRequest(outerCtx context.Context, repo *repo_model.Repository, issue *Issue, labelIDs []int64, uuids []string, pr *PullRequest) (err error) {
535-
ctx, committer, err := db.TxContext(outerCtx)
534+
func NewPullRequest(ctx context.Context, repo *repo_model.Repository, issue *Issue, labelIDs []int64, uuids []string, pr *PullRequest) (err error) {
535+
ctx, committer, err := db.TxContext(ctx)
536536
if err != nil {
537537
return err
538538
}
539539
defer committer.Close()
540-
ctx.WithContext(outerCtx)
541540

542541
idx, err := db.GetNextResourceIndex(ctx, "issue_index", repo.ID)
543542
if err != nil {

‎models/issues/pull_test.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -87,14 +87,14 @@ func TestLoadRequestedReviewers(t *testing.T) {
8787
user1, err := user_model.GetUserByID(db.DefaultContext, 1)
8888
assert.NoError(t, err)
8989

90-
comment, err := issues_model.AddReviewRequest(issue, user1, &user_model.User{})
90+
comment, err := issues_model.AddReviewRequest(db.DefaultContext, issue, user1, &user_model.User{})
9191
assert.NoError(t, err)
9292
assert.NotNil(t, comment)
9393

9494
assert.NoError(t, pull.LoadRequestedReviewers(db.DefaultContext))
9595
assert.Len(t, pull.RequestedReviewers, 1)
9696

97-
comment, err = issues_model.RemoveReviewRequest(issue, user1, &user_model.User{})
97+
comment, err = issues_model.RemoveReviewRequest(db.DefaultContext, issue, user1, &user_model.User{})
9898
assert.NoError(t, err)
9999
assert.NotNil(t, comment)
100100

‎models/issues/review.go

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -557,8 +557,8 @@ func InsertReviews(reviews []*Review) error {
557557
}
558558

559559
// AddReviewRequest add a review request from one reviewer
560-
func AddReviewRequest(issue *Issue, reviewer, doer *user_model.User) (*Comment, error) {
561-
ctx, committer, err := db.TxContext(db.DefaultContext)
560+
func AddReviewRequest(ctx context.Context, issue *Issue, reviewer, doer *user_model.User) (*Comment, error) {
561+
ctx, committer, err := db.TxContext(ctx)
562562
if err != nil {
563563
return nil, err
564564
}
@@ -612,8 +612,8 @@ func AddReviewRequest(issue *Issue, reviewer, doer *user_model.User) (*Comment,
612612
}
613613

614614
// RemoveReviewRequest remove a review request from one reviewer
615-
func RemoveReviewRequest(issue *Issue, reviewer, doer *user_model.User) (*Comment, error) {
616-
ctx, committer, err := db.TxContext(db.DefaultContext)
615+
func RemoveReviewRequest(ctx context.Context, issue *Issue, reviewer, doer *user_model.User) (*Comment, error) {
616+
ctx, committer, err := db.TxContext(ctx)
617617
if err != nil {
618618
return nil, err
619619
}
@@ -673,8 +673,8 @@ func restoreLatestOfficialReview(ctx context.Context, issueID, reviewerID int64)
673673
}
674674

675675
// AddTeamReviewRequest add a review request from one team
676-
func AddTeamReviewRequest(issue *Issue, reviewer *organization.Team, doer *user_model.User) (*Comment, error) {
677-
ctx, committer, err := db.TxContext(db.DefaultContext)
676+
func AddTeamReviewRequest(ctx context.Context, issue *Issue, reviewer *organization.Team, doer *user_model.User) (*Comment, error) {
677+
ctx, committer, err := db.TxContext(ctx)
678678
if err != nil {
679679
return nil, err
680680
}
@@ -732,8 +732,8 @@ func AddTeamReviewRequest(issue *Issue, reviewer *organization.Team, doer *user_
732732
}
733733

734734
// RemoveTeamReviewRequest remove a review request from one team
735-
func RemoveTeamReviewRequest(issue *Issue, reviewer *organization.Team, doer *user_model.User) (*Comment, error) {
736-
ctx, committer, err := db.TxContext(db.DefaultContext)
735+
func RemoveTeamReviewRequest(ctx context.Context, issue *Issue, reviewer *organization.Team, doer *user_model.User) (*Comment, error) {
736+
ctx, committer, err := db.TxContext(ctx)
737737
if err != nil {
738738
return nil, err
739739
}

‎routers/web/repo/issue.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2226,7 +2226,7 @@ func UpdateIssueAssignee(ctx *context.Context) {
22262226
return
22272227
}
22282228

2229-
_, _, err = issue_service.ToggleAssignee(ctx, issue, ctx.Doer, assigneeID)
2229+
_, _, err = issue_service.ToggleAssigneeWithNotify(ctx, issue, ctx.Doer, assigneeID)
22302230
if err != nil {
22312231
ctx.ServerError("ToggleAssignee", err)
22322232
return

‎services/issue/assignee.go

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ func DeleteNotPassedAssignee(ctx context.Context, issue *issues_model.Issue, doe
3333

3434
if !found {
3535
// This function also does comments and hooks, which is why we call it separately instead of directly removing the assignees here
36-
if _, _, err := ToggleAssignee(ctx, issue, doer, assignee.ID); err != nil {
36+
if _, _, err := ToggleAssigneeWithNotify(ctx, issue, doer, assignee.ID); err != nil {
3737
return err
3838
}
3939
}
@@ -42,8 +42,8 @@ func DeleteNotPassedAssignee(ctx context.Context, issue *issues_model.Issue, doe
4242
return nil
4343
}
4444

45-
// ToggleAssignee changes a user between assigned and not assigned for this issue, and make issue comment for it.
46-
func ToggleAssignee(ctx context.Context, issue *issues_model.Issue, doer *user_model.User, assigneeID int64) (removed bool, comment *issues_model.Comment, err error) {
45+
// ToggleAssigneeWithNoNotify changes a user between assigned and not assigned for this issue, and make issue comment for it.
46+
func ToggleAssigneeWithNotify(ctx context.Context, issue *issues_model.Issue, doer *user_model.User, assigneeID int64) (removed bool, comment *issues_model.Comment, err error) {
4747
removed, comment, err = issues_model.ToggleIssueAssignee(ctx, issue, doer, assigneeID)
4848
if err != nil {
4949
return
@@ -63,9 +63,9 @@ func ToggleAssignee(ctx context.Context, issue *issues_model.Issue, doer *user_m
6363
// ReviewRequest add or remove a review request from a user for this PR, and make comment for it.
6464
func ReviewRequest(ctx context.Context, issue *issues_model.Issue, doer, reviewer *user_model.User, isAdd bool) (comment *issues_model.Comment, err error) {
6565
if isAdd {
66-
comment, err = issues_model.AddReviewRequest(issue, reviewer, doer)
66+
comment, err = issues_model.AddReviewRequest(ctx, issue, reviewer, doer)
6767
} else {
68-
comment, err = issues_model.RemoveReviewRequest(issue, reviewer, doer)
68+
comment, err = issues_model.RemoveReviewRequest(ctx, issue, reviewer, doer)
6969
}
7070

7171
if err != nil {
@@ -230,9 +230,9 @@ func IsValidTeamReviewRequest(ctx context.Context, reviewer *organization.Team,
230230
// TeamReviewRequest add or remove a review request from a team for this PR, and make comment for it.
231231
func TeamReviewRequest(ctx context.Context, issue *issues_model.Issue, doer *user_model.User, reviewer *organization.Team, isAdd bool) (comment *issues_model.Comment, err error) {
232232
if isAdd {
233-
comment, err = issues_model.AddTeamReviewRequest(issue, reviewer, doer)
233+
comment, err = issues_model.AddTeamReviewRequest(ctx, issue, reviewer, doer)
234234
} else {
235-
comment, err = issues_model.RemoveTeamReviewRequest(issue, reviewer, doer)
235+
comment, err = issues_model.RemoveTeamReviewRequest(ctx, issue, reviewer, doer)
236236
}
237237

238238
if err != nil {

‎services/issue/issue.go

Lines changed: 13 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@ func NewIssue(ctx context.Context, repo *repo_model.Repository, issue *issues_mo
2727
}
2828

2929
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 {
3131
return err
3232
}
3333
}
@@ -122,7 +122,7 @@ func UpdateAssignees(ctx context.Context, issue *issues_model.Issue, oneAssignee
122122
// has access to the repo.
123123
for _, assignee := range allNewAssignees {
124124
// Extra method to prevent double adding (which would result in removing)
125-
err = AddAssigneeIfNotAssigned(ctx, issue, doer, assignee.ID)
125+
_, err = AddAssigneeIfNotAssigned(ctx, issue, doer, assignee.ID, true)
126126
if err != nil {
127127
return err
128128
}
@@ -167,36 +167,36 @@ func DeleteIssue(ctx context.Context, doer *user_model.User, gitRepo *git.Reposi
167167

168168
// AddAssigneeIfNotAssigned adds an assignee only if he isn't already assigned to the issue.
169169
// Also checks for access of assigned user
170-
func AddAssigneeIfNotAssigned(ctx context.Context, issue *issues_model.Issue, doer *user_model.User, assigneeID int64) (err error) {
170+
func AddAssigneeIfNotAssigned(ctx context.Context, issue *issues_model.Issue, doer *user_model.User, assigneeID int64, notify bool) (comment *issues_model.Comment, err error) {
171171
assignee, err := user_model.GetUserByID(ctx, assigneeID)
172172
if err != nil {
173-
return err
173+
return nil, err
174174
}
175175

176176
// Check if the user is already assigned
177177
isAssigned, err := issues_model.IsUserAssignedToIssue(ctx, issue, assignee)
178178
if err != nil {
179-
return err
179+
return nil, err
180180
}
181181
if isAssigned {
182182
// nothing to to
183-
return nil
183+
return nil, nil
184184
}
185185

186186
valid, err := access_model.CanBeAssigned(ctx, assignee, issue.Repo, issue.IsPull)
187187
if err != nil {
188-
return err
188+
return nil, err
189189
}
190190
if !valid {
191-
return repo_model.ErrUserDoesNotHaveAccessToRepo{UserID: assigneeID, RepoName: issue.Repo.Name}
191+
return nil, repo_model.ErrUserDoesNotHaveAccessToRepo{UserID: assigneeID, RepoName: issue.Repo.Name}
192192
}
193193

194-
_, _, err = ToggleAssignee(ctx, issue, doer, assigneeID)
195-
if err != nil {
196-
return err
194+
if notify {
195+
_, comment, err = ToggleAssigneeWithNotify(ctx, issue, doer, assigneeID)
196+
return comment, err
197197
}
198-
199-
return nil
198+
_, comment, err = issues_model.ToggleIssueAssignee(ctx, issue, doer, assigneeID)
199+
return comment, err
200200
}
201201

202202
// GetRefEndNamesAndURLs retrieves the ref end names (e.g. refs/heads/branch-name -> branch-name)

‎services/pull/patch.go

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -62,14 +62,19 @@ func TestPatch(pr *issues_model.PullRequest) error {
6262
ctx, _, finished := process.GetManager().AddContext(graceful.GetManager().HammerContext(), fmt.Sprintf("TestPatch: %s", pr))
6363
defer finished()
6464

65-
// Clone base repo.
6665
prCtx, cancel, err := createTemporaryRepoForPR(ctx, pr)
6766
if err != nil {
68-
log.Error("createTemporaryRepoForPR %-v: %v", pr, err)
67+
if !models.IsErrBranchDoesNotExist(err) {
68+
log.Error("CreateTemporaryRepoForPR %-v: %v", pr, err)
69+
}
6970
return err
7071
}
7172
defer cancel()
7273

74+
return testPatch(ctx, prCtx, pr)
75+
}
76+
77+
func testPatch(ctx context.Context, prCtx *prContext, pr *issues_model.PullRequest) error {
7378
gitRepo, err := git.OpenRepository(ctx, prCtx.tmpBasePath)
7479
if err != nil {
7580
return fmt.Errorf("OpenRepository: %w", err)

‎services/pull/pull.go

Lines changed: 80 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,6 @@ import (
2323
"code.gitea.io/gitea/modules/json"
2424
"code.gitea.io/gitea/modules/log"
2525
"code.gitea.io/gitea/modules/notification"
26-
"code.gitea.io/gitea/modules/process"
2726
repo_module "code.gitea.io/gitea/modules/repository"
2827
"code.gitea.io/gitea/modules/setting"
2928
"code.gitea.io/gitea/modules/sync"
@@ -35,73 +34,70 @@ import (
3534
var pullWorkingPool = sync.NewExclusivePool()
3635

3736
// NewPullRequest creates new pull request with labels for repository.
38-
func NewPullRequest(ctx context.Context, repo *repo_model.Repository, pull *issues_model.Issue, labelIDs []int64, uuids []string, pr *issues_model.PullRequest, assigneeIDs []int64) error {
39-
if err := TestPatch(pr); err != nil {
40-
return err
41-
}
42-
43-
divergence, err := GetDiverging(ctx, pr)
37+
func NewPullRequest(ctx context.Context, repo *repo_model.Repository, issue *issues_model.Issue, labelIDs []int64, uuids []string, pr *issues_model.PullRequest, assigneeIDs []int64) error {
38+
prCtx, cancel, err := createTemporaryRepoForPR(ctx, pr)
4439
if err != nil {
45-
return err
46-
}
47-
pr.CommitsAhead = divergence.Ahead
48-
pr.CommitsBehind = divergence.Behind
49-
50-
if err := issues_model.NewPullRequest(ctx, repo, pull, labelIDs, uuids, pr); err != nil {
51-
return err
52-
}
53-
54-
for _, assigneeID := range assigneeIDs {
55-
if err := issue_service.AddAssigneeIfNotAssigned(ctx, pull, pull.Poster, assigneeID); err != nil {
56-
return err
40+
if !models.IsErrBranchDoesNotExist(err) {
41+
log.Error("CreateTemporaryRepoForPR %-v: %v", pr, err)
5742
}
43+
return err
5844
}
45+
defer cancel()
5946

60-
pr.Issue = pull
61-
pull.PullRequest = pr
62-
63-
// Now - even if the request context has been cancelled as the PR has been created
64-
// in the db and there is no way to cancel that transaction we have to proceed - therefore
65-
// create new context and work from there
66-
prCtx, _, finished := process.GetManager().AddContext(graceful.GetManager().HammerContext(), fmt.Sprintf("NewPullRequest: %s:%d", repo.FullName(), pr.Index))
67-
defer finished()
68-
69-
if pr.Flow == issues_model.PullRequestFlowGithub {
70-
err = PushToBaseRepo(prCtx, pr)
71-
} else {
72-
err = UpdateRef(prCtx, pr)
73-
}
74-
if err != nil {
47+
if err := testPatch(ctx, prCtx, pr); err != nil {
7548
return err
7649
}
7750

78-
mentions, err := issues_model.FindAndUpdateIssueMentions(ctx, pull, pull.Poster, pull.Content)
51+
divergence, err := git.GetDivergingCommits(ctx, prCtx.tmpBasePath, baseBranch, trackingBranch)
7952
if err != nil {
8053
return err
8154
}
55+
pr.CommitsAhead = divergence.Ahead
56+
pr.CommitsBehind = divergence.Behind
8257

83-
notification.NotifyNewPullRequest(prCtx, pr, mentions)
84-
if len(pull.Labels) > 0 {
85-
notification.NotifyIssueChangeLabels(prCtx, pull.Poster, pull, pull.Labels, nil)
86-
}
87-
if pull.Milestone != nil {
88-
notification.NotifyIssueChangeMilestone(prCtx, pull.Poster, pull, 0)
89-
}
58+
assigneeCommentMap := make(map[int64]*issues_model.Comment)
9059

9160
// add first push codes comment
92-
baseGitRepo, err := git.OpenRepository(prCtx, pr.BaseRepo.RepoPath())
61+
baseGitRepo, err := git.OpenRepository(ctx, pr.BaseRepo.RepoPath())
9362
if err != nil {
9463
return err
9564
}
9665
defer baseGitRepo.Close()
9766

98-
compareInfo, err := baseGitRepo.GetCompareInfo(pr.BaseRepo.RepoPath(),
99-
git.BranchPrefix+pr.BaseBranch, pr.GetGitRefName(), false, false)
100-
if err != nil {
101-
return err
102-
}
67+
if err := db.WithTx(ctx, func(ctx context.Context) error {
68+
if err := issues_model.NewPullRequest(ctx, repo, issue, labelIDs, uuids, pr); err != nil {
69+
return err
70+
}
71+
72+
for _, assigneeID := range assigneeIDs {
73+
comment, err := issue_service.AddAssigneeIfNotAssigned(ctx, issue, issue.Poster, assigneeID, false)
74+
if err != nil {
75+
return err
76+
}
77+
assigneeCommentMap[assigneeID] = comment
78+
}
79+
80+
pr.Issue = issue
81+
issue.PullRequest = pr
82+
83+
if pr.Flow == issues_model.PullRequestFlowGithub {
84+
err = PushToBaseRepo(ctx, pr)
85+
} else {
86+
err = UpdateRef(ctx, pr)
87+
}
88+
if err != nil {
89+
return err
90+
}
91+
92+
compareInfo, err := baseGitRepo.GetCompareInfo(pr.BaseRepo.RepoPath(),
93+
git.BranchPrefix+pr.BaseBranch, pr.GetGitRefName(), false, false)
94+
if err != nil {
95+
return err
96+
}
97+
if len(compareInfo.Commits) == 0 {
98+
return nil
99+
}
103100

104-
if len(compareInfo.Commits) > 0 {
105101
data := issues_model.PushActionContent{IsForcePush: false}
106102
data.CommitIDs = make([]string, 0, len(compareInfo.Commits))
107103
for i := len(compareInfo.Commits) - 1; i >= 0; i-- {
@@ -115,14 +111,47 @@ func NewPullRequest(ctx context.Context, repo *repo_model.Repository, pull *issu
115111

116112
ops := &issues_model.CreateCommentOptions{
117113
Type: issues_model.CommentTypePullRequestPush,
118-
Doer: pull.Poster,
114+
Doer: issue.Poster,
119115
Repo: repo,
120116
Issue: pr.Issue,
121117
IsForcePush: false,
122118
Content: string(dataJSON),
123119
}
124120

125-
_, _ = issue_service.CreateComment(ctx, ops)
121+
if _, err = issues_model.CreateComment(ctx, ops); err != nil {
122+
return err
123+
}
124+
125+
return nil
126+
}); err != nil {
127+
// cleanup: this will only remove the reference, the real commit will be clean up when next GC
128+
if err1 := baseGitRepo.RemoveReference(pr.GetGitRefName()); err1 != nil {
129+
log.Error("RemoveReference: %v", err1)
130+
}
131+
return err
132+
}
133+
baseGitRepo.Close() // close immediately to avoid notifications will open the repository again
134+
135+
mentions, err := issues_model.FindAndUpdateIssueMentions(ctx, issue, issue.Poster, issue.Content)
136+
if err != nil {
137+
return err
138+
}
139+
140+
notification.NotifyNewPullRequest(ctx, pr, mentions)
141+
if len(issue.Labels) > 0 {
142+
notification.NotifyIssueChangeLabels(ctx, issue.Poster, issue, issue.Labels, nil)
143+
}
144+
if issue.Milestone != nil {
145+
notification.NotifyIssueChangeMilestone(ctx, issue.Poster, issue, 0)
146+
}
147+
if len(assigneeIDs) > 0 {
148+
for _, assigneeID := range assigneeIDs {
149+
assignee, err := user_model.GetUserByID(ctx, assigneeID)
150+
if err != nil {
151+
return ErrDependenciesLeft
152+
}
153+
notification.NotifyIssueChangeAssignee(ctx, issue.Poster, issue, assignee, false, assigneeCommentMap[assigneeID])
154+
}
126155
}
127156

128157
return nil

0 commit comments

Comments
 (0)