Add missed transaction on setmerged

models/issues/issue.go

@@ -78,22 +78,6 @@ func (err ErrNewIssueInsert) Error() string {
 	return err.OriginalError.Error()
 }
 
-// ErrIssueWasClosed is used when close a closed issue
-type ErrIssueWasClosed struct {
-	ID    int64
-	Index int64
-}
-
-// IsErrIssueWasClosed checks if an error is a ErrIssueWasClosed.
-func IsErrIssueWasClosed(err error) bool {
-	_, ok := err.(ErrIssueWasClosed)
-	return ok
-}
-
-func (err ErrIssueWasClosed) Error() string {
-	return fmt.Sprintf("Issue [%d] %d was already closed", err.ID, err.Index)
-}
-
 var ErrIssueAlreadyChanged = util.NewInvalidArgumentErrorf("the issue is already changed")
 
 // Issue represents an issue or pull request of repository.

models/issues/issue_update.go

@@ -28,21 +28,44 @@ import (
 
 // UpdateIssueCols updates cols of issue
 func UpdateIssueCols(ctx context.Context, issue *Issue, cols ...string) error {
-	if _, err := db.GetEngine(ctx).ID(issue.ID).Cols(cols...).Update(issue); err != nil {
-		return err
-	}
-	return nil
+	_, err := db.GetEngine(ctx).ID(issue.ID).Cols(cols...).Update(issue)
+	return err
 }
 
-func ChangeIssueStatus(ctx context.Context, issue *Issue, doer *user_model.User, isClosed, isMergePull bool) (*Comment, error) {
-	// Reload the issue
-	currentIssue, err := GetIssueByID(ctx, issue.ID)
-	if err != nil {
-		return nil, err
-	}
+// ErrIssueWasClosed is used when close a closed issue
+type ErrIssueWasClosed struct {
+	ID    int64
+	Index int64
+}
+
+// IsErrIssueWasClosed checks if an error is a ErrIssueWasClosed.
+func IsErrIssueWasClosed(err error) bool {
+	_, ok := err.(ErrIssueWasClosed)
+	return ok
+}
+
+func (err ErrIssueWasClosed) Error() string {
+	return fmt.Sprintf("Issue [%d] %d was already closed", err.ID, err.Index)
+}
+
+// ErrPullWasClosed is used close a closed pull request
+type ErrPullWasClosed struct {
+	ID    int64
+	Index int64
+}
 
-	// Nothing should be performed if current status is same as target status
-	if currentIssue.IsClosed == isClosed {
+// IsErrPullWasClosed checks if an error is a ErrErrPullWasClosed.
+func IsErrPullWasClosed(err error) bool {
+	_, ok := err.(ErrPullWasClosed)
+	return ok
+}
+
+func (err ErrPullWasClosed) Error() string {
+	return fmt.Sprintf("Pull request [%d] %d was already closed", err.ID, err.Index)
+}
+
+func closeIssue(ctx context.Context, issue *Issue, doer *user_model.User, isMergePull bool) (*Comment, error) {
+	if issue.IsClosed {
 		if !issue.IsPull {
 			return nil, ErrIssueWasClosed{
 				ID: issue.ID,
@@ -53,13 +76,8 @@ func ChangeIssueStatus(ctx context.Context, issue *Issue, doer *user_model.User,
 		}
 	}
 
-	issue.IsClosed = isClosed
-	return doChangeIssueStatus(ctx, issue, doer, isMergePull)
-}
-
-func doChangeIssueStatus(ctx context.Context, issue *Issue, doer *user_model.User, isMergePull bool) (*Comment, error) {
 	// Check for open dependencies
-	if issue.IsClosed && issue.Repo.IsDependenciesEnabled(ctx) {
+	if issue.Repo.IsDependenciesEnabled(ctx) {
 		// 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
 		noDeps, err := IssueNoDependenciesLeft(ctx, issue)
 		if err != nil {
@@ -71,16 +89,79 @@ func doChangeIssueStatus(ctx context.Context, issue *Issue, doer *user_model.Use
 		}
 	}
 
-	if issue.IsClosed {
-		issue.ClosedUnix = timeutil.TimeStampNow()
-	} else {
-		issue.ClosedUnix = 0
+	issue.IsClosed = true
+	issue.ClosedUnix = timeutil.TimeStampNow()
+
+	if cnt, err := db.GetEngine(ctx).ID(issue.ID).Cols("is_closed", "closed_unix").
+		Where("is_closed == ?", false).
+		Update(issue); err != nil {
+		return nil, err
+	} else if cnt != 1 {
+		return nil, ErrIssueAlreadyChanged
 	}
 
-	if err := UpdateIssueCols(ctx, issue, "is_closed", "closed_unix"); err != nil {
+	return updateIssueNumbers(ctx, issue, doer, util.Iif(isMergePull, CommentTypeMergePull, CommentTypeClose))
+}
+
+// ErrIssueWasOpened is used when reopen an opened issue
+type ErrIssueWasOpened struct {
+	ID    int64
+	Index int64
+}
+
+// IsErrIssueWasOpened checks if an error is a ErrIssueWasOpened.
+func IsErrIssueWasOpened(err error) bool {
+	_, ok := err.(ErrIssueWasOpened)
+	return ok
+}
+
+func (err ErrIssueWasOpened) Error() string {
+	return fmt.Sprintf("Issue [%d] %d was already opened", err.ID, err.Index)
+}
+
+// ErrPullWasOpened is used reopen an opened pull request
+type ErrPullWasOpened struct {
+	ID    int64
+	Index int64
+}
+
+// ErrPullWasOpened checks if an error is a ErrPullWasOpened.
+func IsErrPullWasOpened(err error) bool {
+	_, ok := err.(ErrPullWasOpened)
+	return ok
+}
+
+func (err ErrPullWasOpened) Error() string {
+	return fmt.Sprintf("Pull request [%d] %d was already opened", err.ID, err.Index)
+}
+
+func SetIssueAsReopen(ctx context.Context, issue *Issue, doer *user_model.User, isMergePull bool) (*Comment, error) {
+	if !issue.IsClosed {
+		if !issue.IsPull {
+			return nil, ErrIssueWasOpened{
+				ID: issue.ID,
+			}
+		}
+		return nil, ErrPullWasOpened{
+			ID: issue.ID,
+		}
+	}
+
+	issue.IsClosed = false
+	issue.ClosedUnix = 0
+
+	if cnt, err := db.GetEngine(ctx).ID(issue.ID).Cols("is_closed", "closed_unix").
+		Where("is_closed == ?", true).
+		Update(issue); err != nil {
 		return nil, err
+	} else if cnt != 1 {
+		return nil, ErrIssueAlreadyChanged
 	}
 
+	return updateIssueNumbers(ctx, issue, doer, CommentTypeReopen)
+}
+
+func updateIssueNumbers(ctx context.Context, issue *Issue, doer *user_model.User, cmtType CommentType) (*Comment, error) {
 	// Update issue count of labels
 	if err := issue.LoadLabels(ctx); err != nil {
 		return nil, err
@@ -103,14 +184,6 @@ func doChangeIssueStatus(ctx context.Context, issue *Issue, doer *user_model.Use
 		return nil, err
 	}
 
-	// New action comment
-	cmtType := CommentTypeClose
-	if !issue.IsClosed {
-		cmtType = CommentTypeReopen
-	} else if isMergePull {
-		cmtType = CommentTypeMergePull
-	}
-
 	return CreateComment(ctx, &CreateCommentOptions{
 		Type:  cmtType,
 		Doer:  doer,
@@ -134,7 +207,7 @@ func CloseIssue(ctx context.Context, issue *Issue, doer *user_model.User) (*Comm
 	}
 	defer committer.Close()
 
-	comment, err := ChangeIssueStatus(ctx, issue, doer, true, false)
+	comment, err := closeIssue(ctx, issue, doer, false)
 	if err != nil {
 		return nil, err
 	}
@@ -159,7 +232,7 @@ func ReopenIssue(ctx context.Context, issue *Issue, doer *user_model.User) (*Com
 	}
 	defer committer.Close()
 
-	comment, err := ChangeIssueStatus(ctx, issue, doer, false, false)
+	comment, err := SetIssueAsReopen(ctx, issue, doer, false)
 	if err != nil {
 		return nil, err
 	}

models/issues/pull.go

@@ -80,22 +80,6 @@ func (err ErrPullRequestAlreadyExists) Unwrap() error {
 	return util.ErrAlreadyExist
 }
 
-// ErrPullWasClosed is used close a closed pull request
-type ErrPullWasClosed struct {
-	ID    int64
-	Index int64
-}
-
-// IsErrPullWasClosed checks if an error is a ErrErrPullWasClosed.
-func IsErrPullWasClosed(err error) bool {
-	_, ok := err.(ErrPullWasClosed)
-	return ok
-}
-
-func (err ErrPullWasClosed) Error() string {
-	return fmt.Sprintf("Pull request [%d] %d was already closed", err.ID, err.Index)
-}
-
 // PullRequestType defines pull request type
 type PullRequestType int
 

routers/private/hook_post_receive.go

@@ -363,6 +363,8 @@ func handlePullRequestMerging(ctx *gitea_context.PrivateContext, opts *private.H
 	pr.MergedUnix = timeutil.TimeStampNow()
 	pr.Merger = pusher
 	pr.MergerID = pusher.ID
+	// reset the conflicted files as there cannot be any if we're merged
+	pr.ConflictedFiles = []string{}
 	err = db.WithTx(ctx, func(ctx context.Context) error {
 		// Removing an auto merge pull and ignore if not exist
 		if err := pull_model.DeleteScheduledAutoMerge(ctx, pr.ID); err != nil && !db.IsErrNotExist(err) {

services/pull/check.go

@@ -299,6 +299,8 @@ func manuallyMerged(ctx context.Context, pr *issues_model.PullRequest) bool {
 	}
 	pr.Merger = merger
 	pr.MergerID = merger.ID
+	// reset the conflicted files as there cannot be any if we're merged
+	pr.ConflictedFiles = []string{}
 
 	if merged, err := SetMerged(ctx, pr); err != nil {
 		log.Error("%-v setMerged : %v", pr, err)

services/pull/merge.go

@@ -637,6 +637,8 @@ func MergedManually(ctx context.Context, pr *issues_model.PullRequest, doer *use
 		pr.Status = issues_model.PullRequestStatusManuallyMerged
 		pr.Merger = doer
 		pr.MergerID = doer.ID
+		// reset the conflicted files as there cannot be any if we're merged
+		pr.ConflictedFiles = []string{}
 
 		var merged bool
 		if merged, err = SetMerged(ctx, pr); err != nil {
@@ -667,32 +669,18 @@ func SetMerged(ctx context.Context, pr *issues_model.PullRequest) (bool, error)
 	}
 
 	pr.HasMerged = true
-	sess := db.GetEngine(ctx)
 
-	if _, err := sess.Exec("UPDATE `issue` SET `repo_id` = `repo_id` WHERE `id` = ?", pr.IssueID); err != nil {
-		return false, err
-	}
-
-	if _, err := sess.Exec("UPDATE `pull_request` SET `issue_id` = `issue_id` WHERE `id` = ?", pr.ID); err != nil {
+	ctx, committer, err := db.TxContext(ctx)
+	if err != nil {
 		return false, err
 	}
+	defer committer.Close()
 
 	pr.Issue = nil
 	if err := pr.LoadIssue(ctx); err != nil {
 		return false, err
 	}
 
-	if tmpPr, err := issues_model.GetPullRequestByID(ctx, pr.ID); err != nil {
-		return false, err
-	} else if tmpPr.HasMerged {
-		if pr.Issue.IsClosed {
-			return false, nil
-		}
-		return false, fmt.Errorf("PullRequest[%d] already merged but it's associated issue [%d] is not closed", pr.Index, pr.IssueID)
-	} else if pr.Issue.IsClosed {
-		return false, fmt.Errorf("PullRequest[%d] already closed", pr.Index)
-	}
-
 	if err := pr.Issue.LoadRepo(ctx); err != nil {
 		return false, err
 	}
@@ -701,16 +689,22 @@ func SetMerged(ctx context.Context, pr *issues_model.PullRequest) (bool, error)
 		return false, err
 	}
 
-	if _, err := issues_model.ChangeIssueStatus(ctx, pr.Issue, pr.Merger, true, true); err != nil {
+	if _, err := issues_model.SetIssueAsReopen(ctx, pr.Issue, pr.Merger, true); err != nil {
 		return false, fmt.Errorf("ChangeIssueStatus: %w", err)
 	}
 
-	// reset the conflicted files as there cannot be any if we're merged
-	pr.ConflictedFiles = []string{}
-
 	// 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.
-	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 {
+	if cnt, err := db.GetEngine(ctx).Where("id = ?", pr.ID).
+		And("has_merged = ?", false).
+		Cols("has_merged, status, merge_base, merged_commit_id, merger_id, merged_unix, conflicted_files").
+		Update(pr); err != nil {
 		return false, fmt.Errorf("failed to update pr[%d]: %w", pr.ID, err)
+	} else if cnt != 1 {
+		return false, issues_model.ErrIssueAlreadyChanged
+	}
+
+	if err := committer.Commit(); err != nil {
+		return false, err
 	}
 
 	return true, nil