From eb792d9f8a4c6972f5a4cfea6e9cb643b1d6a7ce Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Tue, 7 May 2024 15:36:48 +0800 Subject: [PATCH] Move database operations of merging a pull request to post receive hook and add a transaction (#30805) 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: wxiaoguang (cherry picked from commit ebf0c969403d91ed80745ff5bd7dfbdb08174fc7) Conflicts: modules/private/hook.go routers/private/hook_post_receive.go trivial conflicts because 263a716cb5 * Performance optimization for git push (#30104) was not cherry-picked and because of 998a431747a15cc95f7056a2029b736551eb037b Do not update PRs based on events that happened before they existed --- cmd/hook.go | 3 ++ modules/private/hook.go | 2 + modules/repository/env.go | 8 +++ routers/private/hook_post_receive.go | 63 +++++++++++++++++++++++ routers/private/hook_post_receive_test.go | 49 ++++++++++++++++++ services/contexttest/context_tests.go | 13 +++++ services/pull/merge.go | 27 ++++------ services/pull/update.go | 3 +- 8 files changed, 150 insertions(+), 18 deletions(-) create mode 100644 routers/private/hook_post_receive_test.go diff --git a/cmd/hook.go b/cmd/hook.go index ff3059f9df..4f73f8e2bc 100644 --- a/cmd/hook.go +++ b/cmd/hook.go @@ -366,6 +366,7 @@ Forgejo 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{ @@ -375,6 +376,8 @@ Forgejo 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) diff --git a/modules/private/hook.go b/modules/private/hook.go index cab8c81224..1d0ef4e3a9 100644 --- a/modules/private/hook.go +++ b/modules/private/hook.go @@ -11,6 +11,7 @@ import ( "time" "code.gitea.io/gitea/modules/git" + "code.gitea.io/gitea/modules/repository" "code.gitea.io/gitea/modules/setting" ) @@ -53,6 +54,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 diff --git a/modules/repository/env.go b/modules/repository/env.go index 30edd1c9e3..e4f32092fc 100644 --- a/modules/repository/env.go +++ b/modules/repository/env.go @@ -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 diff --git a/routers/private/hook_post_receive.go b/routers/private/hook_post_receive.go index 381e3c6c77..10b300f3df 100644 --- a/routers/private/hook_post_receive.go +++ b/routers/private/hook_post_receive.go @@ -4,20 +4,26 @@ package private import ( + "context" "fmt" "net/http" "strconv" "time" + "code.gitea.io/gitea/models/db" git_model "code.gitea.io/gitea/models/git" issues_model "code.gitea.io/gitea/models/issues" + 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" @@ -155,6 +161,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 + } + } + // Handle Push Options if len(opts.GitPushOptions) > 0 { // load the repository @@ -302,3 +316,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 index 0000000000..658557d3cf --- /dev/null +++ b/routers/private/hook_post_receive_test.go @@ -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}) +} diff --git a/services/contexttest/context_tests.go b/services/contexttest/context_tests.go index 7bfab2ed16..073af213a2 100644 --- a/services/contexttest/context_tests.go +++ b/services/contexttest/context_tests.go @@ -86,6 +86,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 diff --git a/services/pull/merge.go b/services/pull/merge.go index 1d6431ab66..525146833e 100644 --- a/services/pull/merge.go +++ b/services/pull/merge.go @@ -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" @@ -168,12 +167,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) @@ -190,17 +183,15 @@ func Merge(ctx context.Context, pr *issues_model.PullRequest, doer *user_model.U AddTestPullRequestTask(ctx, doer, pr.BaseRepo.ID, pr.BaseBranch, false, "", "", 0) }() - 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 { @@ -251,7 +242,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 { @@ -324,11 +315,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{ diff --git a/services/pull/update.go b/services/pull/update.go index 1bba396880..dbc1b711e2 100644 --- a/services/pull/update.go +++ b/services/pull/update.go @@ -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() { AddTestPullRequestTask(ctx, doer, reversePR.HeadRepo.ID, reversePR.HeadBranch, false, "", "", 0)