Skip to content

Commit 67aeb1f

Browse files
lunnylunny
authored
Add missed transaction on setmerged (#33079)
Follow #33045. There are two updates on `Set Merged`, which should be in one transaction. This also introduced some refactors for changeissuestatus to make it more clear.
1 parent a8e7cae commit 67aeb1f

File tree

7 files changed

+121
-146
lines changed

7 files changed

+121
-146
lines changed

‎models/issues/issue.go

-33
Original file line numberDiff line numberDiff line change
@@ -46,23 +46,6 @@ func (err ErrIssueNotExist) Unwrap() error {
4646
return util.ErrNotExist
4747
}
4848

49-
// ErrIssueIsClosed represents a "IssueIsClosed" kind of error.
50-
type ErrIssueIsClosed struct {
51-
ID int64
52-
RepoID int64
53-
Index int64
54-
}
55-
56-
// IsErrIssueIsClosed checks if an error is a ErrIssueNotExist.
57-
func IsErrIssueIsClosed(err error) bool {
58-
_, ok := err.(ErrIssueIsClosed)
59-
return ok
60-
}
61-
62-
func (err ErrIssueIsClosed) Error() string {
63-
return fmt.Sprintf("issue is closed [id: %d, repo_id: %d, index: %d]", err.ID, err.RepoID, err.Index)
64-
}
65-
6649
// ErrNewIssueInsert is used when the INSERT statement in newIssue fails
6750
type ErrNewIssueInsert struct {
6851
OriginalError error
@@ -78,22 +61,6 @@ func (err ErrNewIssueInsert) Error() string {
7861
return err.OriginalError.Error()
7962
}
8063

81-
// ErrIssueWasClosed is used when close a closed issue
82-
type ErrIssueWasClosed struct {
83-
ID int64
84-
Index int64
85-
}
86-
87-
// IsErrIssueWasClosed checks if an error is a ErrIssueWasClosed.
88-
func IsErrIssueWasClosed(err error) bool {
89-
_, ok := err.(ErrIssueWasClosed)
90-
return ok
91-
}
92-
93-
func (err ErrIssueWasClosed) Error() string {
94-
return fmt.Sprintf("Issue [%d] %d was already closed", err.ID, err.Index)
95-
}
96-
9764
var ErrIssueAlreadyChanged = util.NewInvalidArgumentErrorf("the issue is already changed")
9865

9966
// Issue represents an issue or pull request of repository.

‎models/issues/issue_update.go

+81-40
Original file line numberDiff line numberDiff line change
@@ -28,38 +28,40 @@ 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
31+
_, err := db.GetEngine(ctx).ID(issue.ID).Cols(cols...).Update(issue)
32+
return err
3533
}
3634

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-
}
35+
// ErrIssueIsClosed is used when close a closed issue
36+
type ErrIssueIsClosed struct {
37+
ID int64
38+
RepoID int64
39+
Index int64
40+
IsPull bool
41+
}
4342

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-
}
43+
// IsErrIssueIsClosed checks if an error is a ErrIssueIsClosed.
44+
func IsErrIssueIsClosed(err error) bool {
45+
_, ok := err.(ErrIssueIsClosed)
46+
return ok
47+
}
5548

56-
issue.IsClosed = isClosed
57-
return doChangeIssueStatus(ctx, issue, doer, isMergePull)
49+
func (err ErrIssueIsClosed) Error() string {
50+
return fmt.Sprintf("%s [id: %d, repo_id: %d, index: %d] is already closed", util.Iif(err.IsPull, "Pull Request", "Issue"), err.ID, err.RepoID, err.Index)
5851
}
5952

60-
func doChangeIssueStatus(ctx context.Context, issue *Issue, doer *user_model.User, isMergePull bool) (*Comment, error) {
53+
func SetIssueAsClosed(ctx context.Context, issue *Issue, doer *user_model.User, isMergePull bool) (*Comment, error) {
54+
if issue.IsClosed {
55+
return nil, ErrIssueIsClosed{
56+
ID: issue.ID,
57+
RepoID: issue.RepoID,
58+
Index: issue.Index,
59+
IsPull: issue.IsPull,
60+
}
61+
}
62+
6163
// Check for open dependencies
62-
if issue.IsClosed && issue.Repo.IsDependenciesEnabled(ctx) {
64+
if issue.Repo.IsDependenciesEnabled(ctx) {
6365
// 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
6466
noDeps, err := IssueNoDependenciesLeft(ctx, issue)
6567
if err != nil {
@@ -71,16 +73,63 @@ func doChangeIssueStatus(ctx context.Context, issue *Issue, doer *user_model.Use
7173
}
7274
}
7375

74-
if issue.IsClosed {
75-
issue.ClosedUnix = timeutil.TimeStampNow()
76-
} else {
77-
issue.ClosedUnix = 0
76+
issue.IsClosed = true
77+
issue.ClosedUnix = timeutil.TimeStampNow()
78+
79+
if cnt, err := db.GetEngine(ctx).ID(issue.ID).Cols("is_closed", "closed_unix").
80+
Where("is_closed = ?", false).
81+
Update(issue); err != nil {
82+
return nil, err
83+
} else if cnt != 1 {
84+
return nil, ErrIssueAlreadyChanged
7885
}
7986

80-
if err := UpdateIssueCols(ctx, issue, "is_closed", "closed_unix"); err != nil {
87+
return updateIssueNumbers(ctx, issue, doer, util.Iif(isMergePull, CommentTypeMergePull, CommentTypeClose))
88+
}
89+
90+
// ErrIssueIsOpen is used when reopen an opened issue
91+
type ErrIssueIsOpen struct {
92+
ID int64
93+
RepoID int64
94+
IsPull bool
95+
Index int64
96+
}
97+
98+
// IsErrIssueIsOpen checks if an error is a ErrIssueIsOpen.
99+
func IsErrIssueIsOpen(err error) bool {
100+
_, ok := err.(ErrIssueIsOpen)
101+
return ok
102+
}
103+
104+
func (err ErrIssueIsOpen) Error() string {
105+
return fmt.Sprintf("%s [id: %d, repo_id: %d, index: %d] is already open", util.Iif(err.IsPull, "Pull Request", "Issue"), err.ID, err.RepoID, err.Index)
106+
}
107+
108+
func setIssueAsReopen(ctx context.Context, issue *Issue, doer *user_model.User) (*Comment, error) {
109+
if !issue.IsClosed {
110+
return nil, ErrIssueIsOpen{
111+
ID: issue.ID,
112+
RepoID: issue.RepoID,
113+
Index: issue.Index,
114+
IsPull: issue.IsPull,
115+
}
116+
}
117+
118+
issue.IsClosed = false
119+
issue.ClosedUnix = 0
120+
121+
if cnt, err := db.GetEngine(ctx).ID(issue.ID).Cols("is_closed", "closed_unix").
122+
Where("is_closed = ?", true).
123+
Update(issue); err != nil {
81124
return nil, err
125+
} else if cnt != 1 {
126+
return nil, ErrIssueAlreadyChanged
82127
}
83128

129+
return updateIssueNumbers(ctx, issue, doer, CommentTypeReopen)
130+
}
131+
132+
func updateIssueNumbers(ctx context.Context, issue *Issue, doer *user_model.User, cmtType CommentType) (*Comment, error) {
84133
// Update issue count of labels
85134
if err := issue.LoadLabels(ctx); err != nil {
86135
return nil, err
@@ -103,14 +152,6 @@ func doChangeIssueStatus(ctx context.Context, issue *Issue, doer *user_model.Use
103152
return nil, err
104153
}
105154

106-
// New action comment
107-
cmtType := CommentTypeClose
108-
if !issue.IsClosed {
109-
cmtType = CommentTypeReopen
110-
} else if isMergePull {
111-
cmtType = CommentTypeMergePull
112-
}
113-
114155
return CreateComment(ctx, &CreateCommentOptions{
115156
Type: cmtType,
116157
Doer: doer,
@@ -134,7 +175,7 @@ func CloseIssue(ctx context.Context, issue *Issue, doer *user_model.User) (*Comm
134175
}
135176
defer committer.Close()
136177

137-
comment, err := ChangeIssueStatus(ctx, issue, doer, true, false)
178+
comment, err := SetIssueAsClosed(ctx, issue, doer, false)
138179
if err != nil {
139180
return nil, err
140181
}
@@ -159,7 +200,7 @@ func ReopenIssue(ctx context.Context, issue *Issue, doer *user_model.User) (*Com
159200
}
160201
defer committer.Close()
161202

162-
comment, err := ChangeIssueStatus(ctx, issue, doer, false, false)
203+
comment, err := setIssueAsReopen(ctx, issue, doer)
163204
if err != nil {
164205
return nil, err
165206
}

‎models/issues/pull.go

-16
Original file line numberDiff line numberDiff line change
@@ -80,22 +80,6 @@ func (err ErrPullRequestAlreadyExists) Unwrap() error {
8080
return util.ErrAlreadyExist
8181
}
8282

83-
// ErrPullWasClosed is used close a closed pull request
84-
type ErrPullWasClosed struct {
85-
ID int64
86-
Index int64
87-
}
88-
89-
// IsErrPullWasClosed checks if an error is a ErrErrPullWasClosed.
90-
func IsErrPullWasClosed(err error) bool {
91-
_, ok := err.(ErrPullWasClosed)
92-
return ok
93-
}
94-
95-
func (err ErrPullWasClosed) Error() string {
96-
return fmt.Sprintf("Pull request [%d] %d was already closed", err.ID, err.Index)
97-
}
98-
9983
// PullRequestType defines pull request type
10084
type PullRequestType int
10185

‎routers/private/hook_post_receive.go

+4-18
Original file line numberDiff line numberDiff line change
@@ -8,11 +8,9 @@ import (
88
"fmt"
99
"net/http"
1010

11-
"code.gitea.io/gitea/models/db"
1211
git_model "code.gitea.io/gitea/models/git"
1312
issues_model "code.gitea.io/gitea/models/issues"
1413
access_model "code.gitea.io/gitea/models/perm/access"
15-
pull_model "code.gitea.io/gitea/models/pull"
1614
repo_model "code.gitea.io/gitea/models/repo"
1715
user_model "code.gitea.io/gitea/models/user"
1816
"code.gitea.io/gitea/modules/cache"
@@ -22,7 +20,7 @@ import (
2220
"code.gitea.io/gitea/modules/private"
2321
repo_module "code.gitea.io/gitea/modules/repository"
2422
"code.gitea.io/gitea/modules/setting"
25-
timeutil "code.gitea.io/gitea/modules/timeutil"
23+
"code.gitea.io/gitea/modules/timeutil"
2624
"code.gitea.io/gitea/modules/util"
2725
"code.gitea.io/gitea/modules/web"
2826
gitea_context "code.gitea.io/gitea/services/context"
@@ -359,21 +357,9 @@ func handlePullRequestMerging(ctx *gitea_context.PrivateContext, opts *private.H
359357
return
360358
}
361359

362-
pr.MergedCommitID = updates[len(updates)-1].NewCommitID
363-
pr.MergedUnix = timeutil.TimeStampNow()
364-
pr.Merger = pusher
365-
pr.MergerID = pusher.ID
366-
err = db.WithTx(ctx, func(ctx context.Context) error {
367-
// Removing an auto merge pull and ignore if not exist
368-
if err := pull_model.DeleteScheduledAutoMerge(ctx, pr.ID); err != nil && !db.IsErrNotExist(err) {
369-
return fmt.Errorf("DeleteScheduledAutoMerge[%d]: %v", opts.PullRequestID, err)
370-
}
371-
if _, err := pull_service.SetMerged(ctx, pr); err != nil {
372-
return fmt.Errorf("SetMerged failed: %s/%s Error: %v", ownerName, repoName, err)
373-
}
374-
return nil
375-
})
376-
if err != nil {
360+
// FIXME: Maybe we need a `PullRequestStatusMerged` status for PRs that are merged, currently we use the previous status
361+
// here to keep it as before, that maybe PullRequestStatusMergeable
362+
if _, err := pull_service.SetMerged(ctx, pr, updates[len(updates)-1].NewCommitID, timeutil.TimeStampNow(), pusher, pr.Status); err != nil {
377363
log.Error("Failed to update PR to merged: %v", err)
378364
ctx.JSON(http.StatusInternalServerError, private.HookPostReceiveResult{Err: "Failed to update PR to merged"})
379365
}

‎services/pull/check.go

+1-6
Original file line numberDiff line numberDiff line change
@@ -282,9 +282,6 @@ func manuallyMerged(ctx context.Context, pr *issues_model.PullRequest) bool {
282282
return false
283283
}
284284

285-
pr.MergedCommitID = commit.ID.String()
286-
pr.MergedUnix = timeutil.TimeStamp(commit.Author.When.Unix())
287-
pr.Status = issues_model.PullRequestStatusManuallyMerged
288285
merger, _ := user_model.GetUserByEmail(ctx, commit.Author.Email)
289286

290287
// When the commit author is unknown set the BaseRepo owner as merger
@@ -297,10 +294,8 @@ func manuallyMerged(ctx context.Context, pr *issues_model.PullRequest) bool {
297294
}
298295
merger = pr.BaseRepo.Owner
299296
}
300-
pr.Merger = merger
301-
pr.MergerID = merger.ID
302297

303-
if merged, err := SetMerged(ctx, pr); err != nil {
298+
if merged, err := SetMerged(ctx, pr, commit.ID.String(), timeutil.TimeStamp(commit.Author.When.Unix()), merger, issues_model.PullRequestStatusManuallyMerged); err != nil {
304299
log.Error("%-v setMerged : %v", pr, err)
305300
return false
306301
} else if !merged {

0 commit comments

Comments
 (0)