Skip to content

Commit 97e3347

Browse files
lunnylunny
committed
Add missed transaction on setmerged
1 parent 9882917 commit 97e3347

File tree

4 files changed

+51
-66
lines changed

4 files changed

+51
-66
lines changed

‎models/issues/issue_update.go‎

Lines changed: 31 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -28,38 +28,13 @@ import (
2828

2929
// UpdateIssueCols updates cols of issue
3030
func UpdateIssueCols(ctx context.Context, issue *Issue, cols ...string) error {
31-
if _, err := db.GetEngine(ctx).ID(issue.ID).Cols(cols...).Update(issue); err != nil {
32-
return err
33-
}
34-
return nil
35-
}
36-
37-
func ChangeIssueStatus(ctx context.Context, issue *Issue, doer *user_model.User, isClosed, isMergePull bool) (*Comment, error) {
38-
// Reload the issue
39-
currentIssue, err := GetIssueByID(ctx, issue.ID)
40-
if err != nil {
41-
return nil, err
42-
}
43-
44-
// Nothing should be performed if current status is same as target status
45-
if currentIssue.IsClosed == isClosed {
46-
if !issue.IsPull {
47-
return nil, ErrIssueWasClosed{
48-
ID: issue.ID,
49-
}
50-
}
51-
return nil, ErrPullWasClosed{
52-
ID: issue.ID,
53-
}
54-
}
55-
56-
issue.IsClosed = isClosed
57-
return doChangeIssueStatus(ctx, issue, doer, isMergePull)
31+
_, err := db.GetEngine(ctx).ID(issue.ID).Cols(cols...).Update(issue)
32+
return err
5833
}
5934

60-
func doChangeIssueStatus(ctx context.Context, issue *Issue, doer *user_model.User, isMergePull bool) (*Comment, error) {
35+
func closeIssue(ctx context.Context, issue *Issue, doer *user_model.User, isMergePull bool) (*Comment, error) {
6136
// Check for open dependencies
62-
if issue.IsClosed && issue.Repo.IsDependenciesEnabled(ctx) {
37+
if issue.Repo.IsDependenciesEnabled(ctx) {
6338
// only check if dependencies are enabled and we're about to close an issue, otherwise reopening an issue would fail when there are unsatisfied dependencies
6439
noDeps, err := IssueNoDependenciesLeft(ctx, issue)
6540
if err != nil {
@@ -71,16 +46,36 @@ func doChangeIssueStatus(ctx context.Context, issue *Issue, doer *user_model.Use
7146
}
7247
}
7348

74-
if issue.IsClosed {
75-
issue.ClosedUnix = timeutil.TimeStampNow()
76-
} else {
77-
issue.ClosedUnix = 0
49+
issue.IsClosed = true
50+
issue.ClosedUnix = timeutil.TimeStampNow()
51+
52+
if cnt, err := db.GetEngine(ctx).ID(issue.ID).Cols("is_closed", "closed_unix").
53+
Where("is_closed == ?", false).
54+
Update(issue); err != nil {
55+
return nil, err
56+
} else if cnt != 1 {
57+
return nil, ErrIssueAlreadyChanged
7858
}
7959

80-
if err := UpdateIssueCols(ctx, issue, "is_closed", "closed_unix"); err != nil {
60+
return updateIssueNumbers(ctx, issue, doer, util.Iif(isMergePull, CommentTypeMergePull, CommentTypeClose))
61+
}
62+
63+
func SetIssueAsReopen(ctx context.Context, issue *Issue, doer *user_model.User, isMergePull bool) (*Comment, error) {
64+
issue.IsClosed = false
65+
issue.ClosedUnix = 0
66+
67+
if cnt, err := db.GetEngine(ctx).ID(issue.ID).Cols("is_closed", "closed_unix").
68+
Where("is_closed == ?", true).
69+
Update(issue); err != nil {
8170
return nil, err
71+
} else if cnt != 1 {
72+
return nil, ErrIssueAlreadyChanged
8273
}
8374

75+
return updateIssueNumbers(ctx, issue, doer, CommentTypeReopen)
76+
}
77+
78+
func updateIssueNumbers(ctx context.Context, issue *Issue, doer *user_model.User, cmtType CommentType) (*Comment, error) {
8479
// Update issue count of labels
8580
if err := issue.LoadLabels(ctx); err != nil {
8681
return nil, err
@@ -103,14 +98,6 @@ func doChangeIssueStatus(ctx context.Context, issue *Issue, doer *user_model.Use
10398
return nil, err
10499
}
105100

106-
// New action comment
107-
cmtType := CommentTypeClose
108-
if !issue.IsClosed {
109-
cmtType = CommentTypeReopen
110-
} else if isMergePull {
111-
cmtType = CommentTypeMergePull
112-
}
113-
114101
return CreateComment(ctx, &CreateCommentOptions{
115102
Type: cmtType,
116103
Doer: doer,
@@ -134,7 +121,7 @@ func CloseIssue(ctx context.Context, issue *Issue, doer *user_model.User) (*Comm
134121
}
135122
defer committer.Close()
136123

137-
comment, err := ChangeIssueStatus(ctx, issue, doer, true, false)
124+
comment, err := closeIssue(ctx, issue, doer, false)
138125
if err != nil {
139126
return nil, err
140127
}
@@ -159,7 +146,7 @@ func ReopenIssue(ctx context.Context, issue *Issue, doer *user_model.User) (*Com
159146
}
160147
defer committer.Close()
161148

162-
comment, err := ChangeIssueStatus(ctx, issue, doer, false, false)
149+
comment, err := SetIssueAsReopen(ctx, issue, doer, false)
163150
if err != nil {
164151
return nil, err
165152
}

‎routers/private/hook_post_receive.go‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -363,6 +363,8 @@ func handlePullRequestMerging(ctx *gitea_context.PrivateContext, opts *private.H
363363
pr.MergedUnix = timeutil.TimeStampNow()
364364
pr.Merger = pusher
365365
pr.MergerID = pusher.ID
366+
// reset the conflicted files as there cannot be any if we're merged
367+
pr.ConflictedFiles = []string{}
366368
err = db.WithTx(ctx, func(ctx context.Context) error {
367369
// Removing an auto merge pull and ignore if not exist
368370
if err := pull_model.DeleteScheduledAutoMerge(ctx, pr.ID); err != nil && !db.IsErrNotExist(err) {

‎services/pull/check.go‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -299,6 +299,8 @@ func manuallyMerged(ctx context.Context, pr *issues_model.PullRequest) bool {
299299
}
300300
pr.Merger = merger
301301
pr.MergerID = merger.ID
302+
// reset the conflicted files as there cannot be any if we're merged
303+
pr.ConflictedFiles = []string{}
302304

303305
if merged, err := SetMerged(ctx, pr); err != nil {
304306
log.Error("%-v setMerged : %v", pr, err)

‎services/pull/merge.go‎

Lines changed: 16 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -637,6 +637,8 @@ func MergedManually(ctx context.Context, pr *issues_model.PullRequest, doer *use
637637
pr.Status = issues_model.PullRequestStatusManuallyMerged
638638
pr.Merger = doer
639639
pr.MergerID = doer.ID
640+
// reset the conflicted files as there cannot be any if we're merged
641+
pr.ConflictedFiles = []string{}
640642

641643
var merged bool
642644
if merged, err = SetMerged(ctx, pr); err != nil {
@@ -667,32 +669,18 @@ func SetMerged(ctx context.Context, pr *issues_model.PullRequest) (bool, error)
667669
}
668670

669671
pr.HasMerged = true
670-
sess := db.GetEngine(ctx)
671672

672-
if _, err := sess.Exec("UPDATE `issue` SET `repo_id` = `repo_id` WHERE `id` = ?", pr.IssueID); err != nil {
673-
return false, err
674-
}
675-
676-
if _, err := sess.Exec("UPDATE `pull_request` SET `issue_id` = `issue_id` WHERE `id` = ?", pr.ID); err != nil {
673+
ctx, committer, err := db.TxContext(ctx)
674+
if err != nil {
677675
return false, err
678676
}
677+
defer committer.Close()
679678

680679
pr.Issue = nil
681680
if err := pr.LoadIssue(ctx); err != nil {
682681
return false, err
683682
}
684683

685-
if tmpPr, err := issues_model.GetPullRequestByID(ctx, pr.ID); err != nil {
686-
return false, err
687-
} else if tmpPr.HasMerged {
688-
if pr.Issue.IsClosed {
689-
return false, nil
690-
}
691-
return false, fmt.Errorf("PullRequest[%d] already merged but it's associated issue [%d] is not closed", pr.Index, pr.IssueID)
692-
} else if pr.Issue.IsClosed {
693-
return false, fmt.Errorf("PullRequest[%d] already closed", pr.Index)
694-
}
695-
696684
if err := pr.Issue.LoadRepo(ctx); err != nil {
697685
return false, err
698686
}
@@ -701,16 +689,22 @@ func SetMerged(ctx context.Context, pr *issues_model.PullRequest) (bool, error)
701689
return false, err
702690
}
703691

704-
if _, err := issues_model.ChangeIssueStatus(ctx, pr.Issue, pr.Merger, true, true); err != nil {
692+
if _, err := issues_model.SetIssueAsReopen(ctx, pr.Issue, pr.Merger, true); err != nil {
705693
return false, fmt.Errorf("ChangeIssueStatus: %w", err)
706694
}
707695

708-
// reset the conflicted files as there cannot be any if we're merged
709-
pr.ConflictedFiles = []string{}
710-
711696
// We need to save all of the data used to compute this merge as it may have already been changed by TestPatch. FIXME: need to set some state to prevent TestPatch from running whilst we are merging.
712-
if _, err := sess.Where("id = ?", pr.ID).Cols("has_merged, status, merge_base, merged_commit_id, merger_id, merged_unix, conflicted_files").Update(pr); err != nil {
697+
if cnt, err := db.GetEngine(ctx).Where("id = ?", pr.ID).
698+
And("has_merged = ?", false).
699+
Cols("has_merged, status, merge_base, merged_commit_id, merger_id, merged_unix, conflicted_files").
700+
Update(pr); err != nil {
713701
return false, fmt.Errorf("failed to update pr[%d]: %w", pr.ID, err)
702+
} else if cnt != 1 {
703+
return false, issues_model.ErrIssueAlreadyChanged
704+
}
705+
706+
if err := committer.Commit(); err != nil {
707+
return false, err
714708
}
715709

716710
return true, nil

0 commit comments

Comments
 (0)