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.

models/issues/issue.go

@@ -46,23 +46,6 @@ func (err ErrIssueNotExist) Unwrap() error {
 	return util.ErrNotExist
 }
 
-// ErrIssueIsClosed represents a "IssueIsClosed" kind of error.
-type ErrIssueIsClosed struct {
-	ID     int64
-	RepoID int64
-	Index  int64
-}
-
-// IsErrIssueIsClosed checks if an error is a ErrIssueNotExist.
-func IsErrIssueIsClosed(err error) bool {
-	_, ok := err.(ErrIssueIsClosed)
-	return ok
-}
-
-func (err ErrIssueIsClosed) Error() string {
-	return fmt.Sprintf("issue is closed [id: %d, repo_id: %d, index: %d]", err.ID, err.RepoID, err.Index)
-}
-
 // ErrNewIssueInsert is used when the INSERT statement in newIssue fails
 type ErrNewIssueInsert struct {
 	OriginalError error
@@ -78,22 +61,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,38 +28,40 @@ 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 {
+	_, err := db.GetEngine(ctx).ID(issue.ID).Cols(cols...).Update(issue)
 	return err
-	}
-	return nil
 }
 
-func ChangeIssueStatus(ctx context.Context, issue *Issue, doer *user_model.User, isClosed, isMergePull bool) (*Comment, error) {
+// ErrIssueIsClosed is used when close a closed issue
-	// Reload the issue
+type ErrIssueIsClosed struct {
-	currentIssue, err := GetIssueByID(ctx, issue.ID)
+	ID     int64
-	if err != nil {
+	RepoID int64
-		return nil, err
+	Index  int64
-	}
+	IsPull bool
+}
 
-	// Nothing should be performed if current status is same as target status
+// IsErrIssueIsClosed checks if an error is a ErrIssueIsClosed.
-	if currentIssue.IsClosed == isClosed {
+func IsErrIssueIsClosed(err error) bool {
-		if !issue.IsPull {
+	_, ok := err.(ErrIssueIsClosed)
-			return nil, ErrIssueWasClosed{
+	return ok
-				ID: issue.ID,
+}
-			}
+
-		}
+func (err ErrIssueIsClosed) Error() string {
-		return nil, ErrPullWasClosed{
+	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)
+}
+
+func SetIssueAsClosed(ctx context.Context, issue *Issue, doer *user_model.User, isMergePull bool) (*Comment, error) {
+	if issue.IsClosed {
+		return nil, ErrIssueIsClosed{
 			ID:     issue.ID,
+			RepoID: issue.RepoID,
+			Index:  issue.Index,
+			IsPull: issue.IsPull,
 		}
 	}
 
-	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 +73,63 @@ func doChangeIssueStatus(ctx context.Context, issue *Issue, doer *user_model.Use
 		}
 	}
 
-	if issue.IsClosed {
+	issue.IsClosed = true
 	issue.ClosedUnix = timeutil.TimeStampNow()
-	} else {
+
-		issue.ClosedUnix = 0
+	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))
+}
+
+// ErrIssueIsOpen is used when reopen an opened issue
+type ErrIssueIsOpen struct {
+	ID     int64
+	RepoID int64
+	IsPull bool
+	Index  int64
+}
+
+// IsErrIssueIsOpen checks if an error is a ErrIssueIsOpen.
+func IsErrIssueIsOpen(err error) bool {
+	_, ok := err.(ErrIssueIsOpen)
+	return ok
+}
+
+func (err ErrIssueIsOpen) Error() string {
+	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)
+}
+
+func setIssueAsReopen(ctx context.Context, issue *Issue, doer *user_model.User) (*Comment, error) {
+	if !issue.IsClosed {
+		return nil, ErrIssueIsOpen{
+			ID:     issue.ID,
+			RepoID: issue.RepoID,
+			Index:  issue.Index,
+			IsPull: issue.IsPull,
+		}
+	}
+
+	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 +152,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 +175,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 := SetIssueAsClosed(ctx, issue, doer, false)
 	if err != nil {
 		return nil, err
 	}
@@ -159,7 +200,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)
 	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

@@ -8,11 +8,9 @@ import (
 	"fmt"
 	"net/http"
 
-	"code.gitea.io/gitea/models/db"
 	git_model "code.gitea.io/gitea/models/git"
 	issues_model "code.gitea.io/gitea/models/issues"
 	access_model "code.gitea.io/gitea/models/perm/access"
-	pull_model "code.gitea.io/gitea/models/pull"
 	repo_model "code.gitea.io/gitea/models/repo"
 	user_model "code.gitea.io/gitea/models/user"
 	"code.gitea.io/gitea/modules/cache"
@@ -22,7 +20,7 @@ import (
 	"code.gitea.io/gitea/modules/private"
 	repo_module "code.gitea.io/gitea/modules/repository"
 	"code.gitea.io/gitea/modules/setting"
-	timeutil "code.gitea.io/gitea/modules/timeutil"
+	"code.gitea.io/gitea/modules/timeutil"
 	"code.gitea.io/gitea/modules/util"
 	"code.gitea.io/gitea/modules/web"
 	gitea_context "code.gitea.io/gitea/services/context"
@@ -359,21 +357,9 @@ func handlePullRequestMerging(ctx *gitea_context.PrivateContext, opts *private.H
 		return
 	}
 
-	pr.MergedCommitID = updates[len(updates)-1].NewCommitID
+	// FIXME: Maybe we need a `PullRequestStatusMerged` status for PRs that are merged, currently we use the previous status
-	pr.MergedUnix = timeutil.TimeStampNow()
+	// here to keep it as before, that maybe PullRequestStatusMergeable
-	pr.Merger = pusher
+	if _, err := pull_service.SetMerged(ctx, pr, updates[len(updates)-1].NewCommitID, timeutil.TimeStampNow(), pusher, pr.Status); err != nil {
-	pr.MergerID = pusher.ID
-	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) {
-			return fmt.Errorf("DeleteScheduledAutoMerge[%d]: %v", opts.PullRequestID, err)
-		}
-		if _, err := pull_service.SetMerged(ctx, pr); err != nil {
-			return fmt.Errorf("SetMerged failed: %s/%s Error: %v", ownerName, repoName, err)
-		}
-		return nil
-	})
-	if err != nil {
 		log.Error("Failed to update PR to merged: %v", err)
 		ctx.JSON(http.StatusInternalServerError, private.HookPostReceiveResult{Err: "Failed to update PR to merged"})
 	}

services/pull/check.go

@@ -282,9 +282,6 @@ func manuallyMerged(ctx context.Context, pr *issues_model.PullRequest) bool {
 		return false
 	}
 
-	pr.MergedCommitID = commit.ID.String()
-	pr.MergedUnix = timeutil.TimeStamp(commit.Author.When.Unix())
-	pr.Status = issues_model.PullRequestStatusManuallyMerged
 	merger, _ := user_model.GetUserByEmail(ctx, commit.Author.Email)
 
 	// 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 {
 		}
 		merger = pr.BaseRepo.Owner
 	}
-	pr.Merger = merger
-	pr.MergerID = merger.ID
 
-	if merged, err := SetMerged(ctx, pr); err != nil {
+	if merged, err := SetMerged(ctx, pr, commit.ID.String(), timeutil.TimeStamp(commit.Author.When.Unix()), merger, issues_model.PullRequestStatusManuallyMerged); err != nil {
 		log.Error("%-v setMerged : %v", pr, err)
 		return false
 	} else if !merged {

services/pull/merge.go

@@ -17,6 +17,7 @@ import (
 	git_model "code.gitea.io/gitea/models/git"
 	issues_model "code.gitea.io/gitea/models/issues"
 	access_model "code.gitea.io/gitea/models/perm/access"
+	pull_model "code.gitea.io/gitea/models/pull"
 	repo_model "code.gitea.io/gitea/models/repo"
 	"code.gitea.io/gitea/models/unit"
 	user_model "code.gitea.io/gitea/models/user"
@@ -632,14 +633,8 @@ func MergedManually(ctx context.Context, pr *issues_model.PullRequest, doer *use
 			return fmt.Errorf("Wrong commit ID")
 		}
 
-		pr.MergedCommitID = commitID
-		pr.MergedUnix = timeutil.TimeStamp(commit.Author.When.Unix())
-		pr.Status = issues_model.PullRequestStatusManuallyMerged
-		pr.Merger = doer
-		pr.MergerID = doer.ID
-
 		var merged bool
-		if merged, err = SetMerged(ctx, pr); err != nil {
+		if merged, err = SetMerged(ctx, pr, commitID, timeutil.TimeStamp(commit.Author.When.Unix()), doer, issues_model.PullRequestStatusManuallyMerged); err != nil {
 			return err
 		} else if !merged {
 			return fmt.Errorf("SetMerged failed")
@@ -658,41 +653,35 @@ func MergedManually(ctx context.Context, pr *issues_model.PullRequest, doer *use
 }
 
 // SetMerged sets a pull request to merged and closes the corresponding issue
-func SetMerged(ctx context.Context, pr *issues_model.PullRequest) (bool, error) {
+func SetMerged(ctx context.Context, pr *issues_model.PullRequest, mergedCommitID string, mergedTimeStamp timeutil.TimeStamp, merger *user_model.User, mergeStatus issues_model.PullRequestStatus) (bool, error) {
 	if pr.HasMerged {
 		return false, fmt.Errorf("PullRequest[%d] already merged", pr.Index)
 	}
-	if pr.MergedCommitID == "" || pr.MergedUnix == 0 || pr.Merger == nil {
-		return false, fmt.Errorf("unable to merge PullRequest[%d], some required fields are empty", pr.Index)
-	}
 
 	pr.HasMerged = true
-	sess := db.GetEngine(ctx)
+	pr.MergedCommitID = mergedCommitID
+	pr.MergedUnix = mergedTimeStamp
+	pr.Merger = merger
+	pr.MergerID = merger.ID
+	pr.Status = mergeStatus
+	// reset the conflicted files as there cannot be any if we're merged
+	pr.ConflictedFiles = []string{}
 
-	if _, err := sess.Exec("UPDATE `issue` SET `repo_id` = `repo_id` WHERE `id` = ?", pr.IssueID); err != nil {
+	if pr.MergedCommitID == "" || pr.MergedUnix == 0 || pr.Merger == nil {
-		return false, err
+		return false, fmt.Errorf("unable to merge PullRequest[%d], some required fields are empty", pr.Index)
 	}
 
-	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 +690,28 @@ 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 {
+	// Removing an auto merge pull and ignore if not exist
-		return false, fmt.Errorf("ChangeIssueStatus: %w", err)
+	if err := pull_model.DeleteScheduledAutoMerge(ctx, pr.ID); err != nil && !db.IsErrNotExist(err) {
+		return false, fmt.Errorf("DeleteScheduledAutoMerge[%d]: %v", pr.ID, err)
 	}
 
-	// reset the conflicted files as there cannot be any if we're merged
+	// Set issue as closed
-	pr.ConflictedFiles = []string{}
+	if _, err := issues_model.SetIssueAsClosed(ctx, pr.Issue, pr.Merger, true); err != nil {
+		return false, fmt.Errorf("ChangeIssueStatus: %w", err)
+	}
 
 	// 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

services/pull/pull.go

@@ -265,6 +265,7 @@ func ChangeTargetBranch(ctx context.Context, pr *issues_model.PullRequest, doer
 			ID:     pr.Issue.ID,
 			RepoID: pr.Issue.RepoID,
 			Index:  pr.Issue.Index,
+			IsPull: true,
 		}
 	}
 
@@ -707,7 +708,7 @@ func CloseBranchPulls(ctx context.Context, doer *user_model.User, repoID int64,
 
 	var errs errlist
 	for _, pr := range prs {
-		if err = issue_service.CloseIssue(ctx, pr.Issue, doer, ""); err != nil && !issues_model.IsErrPullWasClosed(err) && !issues_model.IsErrDependenciesLeft(err) {
+		if err = issue_service.CloseIssue(ctx, pr.Issue, doer, ""); err != nil && !issues_model.IsErrIssueIsClosed(err) && !issues_model.IsErrDependenciesLeft(err) {
 			errs = append(errs, err)
 		}
 	}
@@ -741,7 +742,7 @@ func CloseRepoBranchesPulls(ctx context.Context, doer *user_model.User, repo *re
 			if pr.BaseRepoID == repo.ID {
 				continue
 			}
-			if err = issue_service.CloseIssue(ctx, pr.Issue, doer, ""); err != nil && !issues_model.IsErrPullWasClosed(err) {
+			if err = issue_service.CloseIssue(ctx, pr.Issue, doer, ""); err != nil && !issues_model.IsErrIssueIsClosed(err) {
 				errs = append(errs, err)
 			}
 		}