]> source.dussan.org Git - gitea.git/commitdiff
Move database operations of merging a pull request to post receive hook and add a...
authorGiteabot <teabot@gitea.io>
Wed, 8 May 2024 14:17:18 +0000 (22:17 +0800)
committerGitHub <noreply@github.com>
Wed, 8 May 2024 14:17:18 +0000 (14:17 +0000)
Backport #30805 by @lunny

Merging PR may fail because of various problems. The pull request may
have a dirty state because there is no transaction when merging a pull
request. ref
https://github.com/go-gitea/gitea/pull/25741#issuecomment-2074126393

This PR moves all database update operations to post-receive handler for
merging a pull request and having a database transaction. That means if
database operations fail, then the git merging will fail, the git client
will get a fail result.

There are already many tests for pull request merging, so we don't need
to add a new one.

Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>
Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
cmd/hook.go
modules/private/hook.go
modules/repository/env.go
routers/private/hook_post_receive.go
routers/private/hook_post_receive_test.go [new file with mode: 0644]
services/contexttest/context_tests.go
services/pull/merge.go
services/pull/update.go

index 9c1cb66f2a4dd895b86efc222c1fe25411605b34..6e31710caff2180e5409ebaf42b7546e3cc146ef 100644 (file)
@@ -338,6 +338,7 @@ Gitea or set your environment appropriately.`, "")
        isWiki, _ := strconv.ParseBool(os.Getenv(repo_module.EnvRepoIsWiki))
        repoName := os.Getenv(repo_module.EnvRepoName)
        pusherID, _ := strconv.ParseInt(os.Getenv(repo_module.EnvPusherID), 10, 64)
+       prID, _ := strconv.ParseInt(os.Getenv(repo_module.EnvPRID), 10, 64)
        pusherName := os.Getenv(repo_module.EnvPusherName)
 
        hookOptions := private.HookOptions{
@@ -347,6 +348,8 @@ Gitea or set your environment appropriately.`, "")
                GitObjectDirectory:              os.Getenv(private.GitObjectDirectory),
                GitQuarantinePath:               os.Getenv(private.GitQuarantinePath),
                GitPushOptions:                  pushOptions(),
+               PullRequestID:                   prID,
+               PushTrigger:                     repo_module.PushTrigger(os.Getenv(repo_module.EnvPushTrigger)),
        }
        oldCommitIDs := make([]string, hookBatchSize)
        newCommitIDs := make([]string, hookBatchSize)
index 79c3d482298ccf9235d77500decef0799afd7cc2..49d92987441c64e85b9b62c58672def12d4739a1 100644 (file)
@@ -12,6 +12,7 @@ import (
 
        "code.gitea.io/gitea/modules/git"
        "code.gitea.io/gitea/modules/optional"
+       "code.gitea.io/gitea/modules/repository"
        "code.gitea.io/gitea/modules/setting"
 )
 
@@ -54,6 +55,7 @@ type HookOptions struct {
        GitQuarantinePath               string
        GitPushOptions                  GitPushOptions
        PullRequestID                   int64
+       PushTrigger                     repository.PushTrigger
        DeployKeyID                     int64 // if the pusher is a DeployKey, then UserID is the repo's org user.
        IsWiki                          bool
        ActionPerm                      int
index 30edd1c9e326c60758482dc15a9dce2912243994..e4f32092fc792c9c535e4d6596c38c7055c98c36 100644 (file)
@@ -25,11 +25,19 @@ const (
        EnvKeyID        = "GITEA_KEY_ID" // public key ID
        EnvDeployKeyID  = "GITEA_DEPLOY_KEY_ID"
        EnvPRID         = "GITEA_PR_ID"
+       EnvPushTrigger  = "GITEA_PUSH_TRIGGER"
        EnvIsInternal   = "GITEA_INTERNAL_PUSH"
        EnvAppURL       = "GITEA_ROOT_URL"
        EnvActionPerm   = "GITEA_ACTION_PERM"
 )
 
+type PushTrigger string
+
+const (
+       PushTriggerPRMergeToBase    PushTrigger = "pr-merge-to-base"
+       PushTriggerPRUpdateWithBase PushTrigger = "pr-update-with-base"
+)
+
 // InternalPushingEnvironment returns an os environment to switch off hooks on push
 // It is recommended to avoid using this unless you are pushing within a transaction
 // or if you absolutely are sure that post-receive and pre-receive will do nothing
index 769a68970d210f5e2c5a7f9512937d55076e4256..b6699e465fcb04f4a16d64797b42b4e6441af388 100644 (file)
@@ -4,20 +4,25 @@
 package private
 
 import (
+       "context"
        "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"
        "code.gitea.io/gitea/modules/git"
        "code.gitea.io/gitea/modules/gitrepo"
        "code.gitea.io/gitea/modules/log"
        "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/util"
        "code.gitea.io/gitea/modules/web"
        gitea_context "code.gitea.io/gitea/services/context"
@@ -160,6 +165,14 @@ func HookPostReceive(ctx *gitea_context.PrivateContext) {
                }
        }
 
+       // handle pull request merging, a pull request action should push at least 1 commit
+       if opts.PushTrigger == repo_module.PushTriggerPRMergeToBase {
+               handlePullRequestMerging(ctx, opts, ownerName, repoName, updates)
+               if ctx.Written() {
+                       return
+               }
+       }
+
        isPrivate := opts.GitPushOptions.Bool(private.GitPushOptionRepoPrivate)
        isTemplate := opts.GitPushOptions.Bool(private.GitPushOptionRepoTemplate)
        // Handle Push Options
@@ -174,7 +187,7 @@ func HookPostReceive(ctx *gitea_context.PrivateContext) {
                        wasEmpty = repo.IsEmpty
                }
 
-               pusher, err := user_model.GetUserByID(ctx, opts.UserID)
+               pusher, err := loadContextCacheUser(ctx, opts.UserID)
                if err != nil {
                        log.Error("Failed to Update: %s/%s Error: %v", ownerName, repoName, err)
                        ctx.JSON(http.StatusInternalServerError, private.HookPostReceiveResult{
@@ -309,3 +322,52 @@ func HookPostReceive(ctx *gitea_context.PrivateContext) {
                RepoWasEmpty: wasEmpty,
        })
 }
+
+func loadContextCacheUser(ctx context.Context, id int64) (*user_model.User, error) {
+       return cache.GetWithContextCache(ctx, "hook_post_receive_user", id, func() (*user_model.User, error) {
+               return user_model.GetUserByID(ctx, id)
+       })
+}
+
+// handlePullRequestMerging handle pull request merging, a pull request action should push at least 1 commit
+func handlePullRequestMerging(ctx *gitea_context.PrivateContext, opts *private.HookOptions, ownerName, repoName string, updates []*repo_module.PushUpdateOptions) {
+       if len(updates) == 0 {
+               ctx.JSON(http.StatusInternalServerError, private.HookPostReceiveResult{
+                       Err: fmt.Sprintf("Pushing a merged PR (pr:%d) no commits pushed ", opts.PullRequestID),
+               })
+               return
+       }
+
+       pr, err := issues_model.GetPullRequestByID(ctx, opts.PullRequestID)
+       if err != nil {
+               log.Error("GetPullRequestByID[%d]: %v", opts.PullRequestID, err)
+               ctx.JSON(http.StatusInternalServerError, private.HookPostReceiveResult{Err: "GetPullRequestByID failed"})
+               return
+       }
+
+       pusher, err := loadContextCacheUser(ctx, opts.UserID)
+       if err != nil {
+               log.Error("Failed to Update: %s/%s Error: %v", ownerName, repoName, err)
+               ctx.JSON(http.StatusInternalServerError, private.HookPostReceiveResult{Err: "Load pusher user failed"})
+               return
+       }
+
+       pr.MergedCommitID = updates[len(updates)-1].NewCommitID
+       pr.MergedUnix = timeutil.TimeStampNow()
+       pr.Merger = pusher
+       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 := pr.SetMerged(ctx); 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"})
+       }
+}
diff --git a/routers/private/hook_post_receive_test.go b/routers/private/hook_post_receive_test.go
new file mode 100644 (file)
index 0000000..658557d
--- /dev/null
@@ -0,0 +1,49 @@
+// Copyright 2024 The Gitea Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+package private
+
+import (
+       "testing"
+
+       "code.gitea.io/gitea/models/db"
+       issues_model "code.gitea.io/gitea/models/issues"
+       pull_model "code.gitea.io/gitea/models/pull"
+       repo_model "code.gitea.io/gitea/models/repo"
+       "code.gitea.io/gitea/models/unittest"
+       user_model "code.gitea.io/gitea/models/user"
+       "code.gitea.io/gitea/modules/private"
+       repo_module "code.gitea.io/gitea/modules/repository"
+       "code.gitea.io/gitea/services/contexttest"
+
+       "github.com/stretchr/testify/assert"
+)
+
+func TestHandlePullRequestMerging(t *testing.T) {
+       assert.NoError(t, unittest.PrepareTestDatabase())
+       pr, err := issues_model.GetUnmergedPullRequest(db.DefaultContext, 1, 1, "branch2", "master", issues_model.PullRequestFlowGithub)
+       assert.NoError(t, err)
+       assert.NoError(t, pr.LoadBaseRepo(db.DefaultContext))
+
+       user1 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1})
+
+       err = pull_model.ScheduleAutoMerge(db.DefaultContext, user1, pr.ID, repo_model.MergeStyleSquash, "squash merge a pr")
+       assert.NoError(t, err)
+
+       autoMerge := unittest.AssertExistsAndLoadBean(t, &pull_model.AutoMerge{PullID: pr.ID})
+
+       ctx, resp := contexttest.MockPrivateContext(t, "/")
+       handlePullRequestMerging(ctx, &private.HookOptions{
+               PullRequestID: pr.ID,
+               UserID:        2,
+       }, pr.BaseRepo.OwnerName, pr.BaseRepo.Name, []*repo_module.PushUpdateOptions{
+               {NewCommitID: "01234567"},
+       })
+       assert.Equal(t, 0, len(resp.Body.String()))
+       pr, err = issues_model.GetPullRequestByID(db.DefaultContext, pr.ID)
+       assert.NoError(t, err)
+       assert.True(t, pr.HasMerged)
+       assert.EqualValues(t, "01234567", pr.MergedCommitID)
+
+       unittest.AssertNotExistsBean(t, &pull_model.AutoMerge{ID: autoMerge.ID})
+}
index 0c1e5ee54fe2fc10ca223be8bd8c77208e24330c..5624d24058727891c601ce5256a3337a0ae97f54 100644 (file)
@@ -94,6 +94,19 @@ func MockAPIContext(t *testing.T, reqPath string) (*context.APIContext, *httptes
        return ctx, resp
 }
 
+func MockPrivateContext(t *testing.T, reqPath string) (*context.PrivateContext, *httptest.ResponseRecorder) {
+       resp := httptest.NewRecorder()
+       req := mockRequest(t, reqPath)
+       base, baseCleanUp := context.NewBaseContext(resp, req)
+       base.Data = middleware.GetContextData(req.Context())
+       base.Locale = &translation.MockLocale{}
+       ctx := &context.PrivateContext{Base: base}
+       _ = baseCleanUp // during test, it doesn't need to do clean up. TODO: this can be improved later
+       chiCtx := chi.NewRouteContext()
+       ctx.Base.AppendContextValue(chi.RouteCtxKey, chiCtx)
+       return ctx, resp
+}
+
 // LoadRepo load a repo into a test context.
 func LoadRepo(t *testing.T, ctx gocontext.Context, repoID int64) {
        var doer *user_model.User
index 00f23e1e3ae23be9d3502056a1c3e1902cb854b6..20be7c5b5a87797689fd4c284804e582bdd2603c 100644 (file)
@@ -18,7 +18,6 @@ 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"
@@ -162,12 +161,6 @@ func Merge(ctx context.Context, pr *issues_model.PullRequest, doer *user_model.U
        pullWorkingPool.CheckIn(fmt.Sprint(pr.ID))
        defer pullWorkingPool.CheckOut(fmt.Sprint(pr.ID))
 
-       // Removing an auto merge pull and ignore if not exist
-       // FIXME: is this the correct point to do this? Shouldn't this be after IsMergeStyleAllowed?
-       if err := pull_model.DeleteScheduledAutoMerge(ctx, pr.ID); err != nil && !db.IsErrNotExist(err) {
-               return err
-       }
-
        prUnit, err := pr.BaseRepo.GetUnit(ctx, unit.TypePullRequests)
        if err != nil {
                log.Error("pr.BaseRepo.GetUnit(unit.TypePullRequests): %v", err)
@@ -184,17 +177,15 @@ func Merge(ctx context.Context, pr *issues_model.PullRequest, doer *user_model.U
                go AddTestPullRequestTask(doer, pr.BaseRepo.ID, pr.BaseBranch, false, "", "")
        }()
 
-       pr.MergedCommitID, err = doMergeAndPush(ctx, pr, doer, mergeStyle, expectedHeadCommitID, message)
+       _, err = doMergeAndPush(ctx, pr, doer, mergeStyle, expectedHeadCommitID, message, repo_module.PushTriggerPRMergeToBase)
        if err != nil {
                return err
        }
 
-       pr.MergedUnix = timeutil.TimeStampNow()
-       pr.Merger = doer
-       pr.MergerID = doer.ID
-
-       if _, err := pr.SetMerged(ctx); err != nil {
-               log.Error("SetMerged %-v: %v", pr, err)
+       // reload pull request because it has been updated by post receive hook
+       pr, err = issues_model.GetPullRequestByID(ctx, pr.ID)
+       if err != nil {
+               return err
        }
 
        if err := pr.LoadIssue(ctx); err != nil {
@@ -245,7 +236,7 @@ func Merge(ctx context.Context, pr *issues_model.PullRequest, doer *user_model.U
 }
 
 // doMergeAndPush performs the merge operation without changing any pull information in database and pushes it up to the base repository
-func doMergeAndPush(ctx context.Context, pr *issues_model.PullRequest, doer *user_model.User, mergeStyle repo_model.MergeStyle, expectedHeadCommitID, message string) (string, error) {
+func doMergeAndPush(ctx context.Context, pr *issues_model.PullRequest, doer *user_model.User, mergeStyle repo_model.MergeStyle, expectedHeadCommitID, message string, pushTrigger repo_module.PushTrigger) (string, error) {
        // Clone base repo.
        mergeCtx, cancel, err := createTemporaryRepoForMerge(ctx, pr, doer, expectedHeadCommitID)
        if err != nil {
@@ -318,11 +309,13 @@ func doMergeAndPush(ctx context.Context, pr *issues_model.PullRequest, doer *use
                pr.BaseRepo.Name,
                pr.ID,
        )
+
+       mergeCtx.env = append(mergeCtx.env, repo_module.EnvPushTrigger+"="+string(pushTrigger))
        pushCmd := git.NewCommand(ctx, "push", "origin").AddDynamicArguments(baseBranch + ":" + git.BranchPrefix + pr.BaseBranch)
 
        // Push back to upstream.
-       // TODO: this cause an api call to "/api/internal/hook/post-receive/...",
-       //       that prevents us from doint the whole merge in one db transaction
+       // This cause an api call to "/api/internal/hook/post-receive/...",
+       // If it's merge, all db transaction and operations should be there but not here to prevent deadlock.
        if err := pushCmd.Run(mergeCtx.RunOpts()); err != nil {
                if strings.Contains(mergeCtx.errbuf.String(), "non-fast-forward") {
                        return "", &git.ErrPushOutOfDate{
index bc8c4a25e5f61c4e6afaaa51252359b8db333245..939449b7544e5e6afbeac8683e9404ae004a6666 100644 (file)
@@ -15,6 +15,7 @@ import (
        user_model "code.gitea.io/gitea/models/user"
        "code.gitea.io/gitea/modules/git"
        "code.gitea.io/gitea/modules/log"
+       "code.gitea.io/gitea/modules/repository"
 )
 
 // Update updates pull request with base branch.
@@ -72,7 +73,7 @@ func Update(ctx context.Context, pr *issues_model.PullRequest, doer *user_model.
                BaseBranch: pr.HeadBranch,
        }
 
-       _, err = doMergeAndPush(ctx, reversePR, doer, repo_model.MergeStyleMerge, "", message)
+       _, err = doMergeAndPush(ctx, reversePR, doer, repo_model.MergeStyleMerge, "", message, repository.PushTriggerPRUpdateWithBase)
 
        defer func() {
                go AddTestPullRequestTask(doer, reversePR.HeadRepo.ID, reversePR.HeadBranch, false, "", "")