From 430d9df43789fcff102e41ccfcdc11ce2b5787f1 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Fri, 3 May 2024 22:22:55 +0800 Subject: [PATCH] fix --- cmd/hook.go | 2 +- modules/private/hook.go | 3 +- modules/repository/env.go | 3 +- routers/private/hook_post_receive.go | 58 +++++++++++------------ routers/private/hook_post_receive_test.go | 37 +++++++++++++++ services/contexttest/context_tests.go | 13 +++++ services/pull/update.go | 3 +- 7 files changed, 86 insertions(+), 33 deletions(-) create mode 100644 routers/private/hook_post_receive_test.go diff --git a/cmd/hook.go b/cmd/hook.go index 95491c2e46..c2fb910235 100644 --- a/cmd/hook.go +++ b/cmd/hook.go @@ -352,7 +352,7 @@ Gitea or set your environment appropriately.`, "") GitQuarantinePath: os.Getenv(private.GitQuarantinePath), GitPushOptions: pushOptions(), PullRequestID: prID, - PushTrigger: os.Getenv(repo_module.EnvPushTrigger), + 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 8f4724c5b0..49d9298744 100644 --- a/modules/private/hook.go +++ b/modules/private/hook.go @@ -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,7 +55,7 @@ type HookOptions struct { GitQuarantinePath string GitPushOptions GitPushOptions PullRequestID int64 - PushTrigger string + 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 4560ea81e7..e4f32092fc 100644 --- a/modules/repository/env.go +++ b/modules/repository/env.go @@ -34,7 +34,8 @@ const ( type PushTrigger string const ( - PushTriggerPRMergeToBase PushTrigger = "pr-merge-to-base" + PushTriggerPRMergeToBase PushTrigger = "pr-merge-to-base" + PushTriggerPRUpdateWithBase PushTrigger = "pr-update-with-base" ) // InternalPushingEnvironment returns an os environment to switch off hooks on push diff --git a/routers/private/hook_post_receive.go b/routers/private/hook_post_receive.go index 58e5c7b142..d69d342f4e 100644 --- a/routers/private/hook_post_receive.go +++ b/routers/private/hook_post_receive.go @@ -164,9 +164,11 @@ func HookPostReceive(ctx *gitea_context.PrivateContext) { } // handle pull request merging, a pull request action should push at least 1 commit - handlePullRequestMerging(ctx, opts, ownerName, repoName, updates) - if ctx.Written() { - return + if opts.PushTrigger == repo_module.PushTriggerPRMergeToBase { + handlePullRequestMerging(ctx, opts, ownerName, repoName, updates) + if ctx.Written() { + return + } } isPrivate := opts.GitPushOptions.Bool(private.GitPushOptionRepoPrivate) @@ -183,9 +185,7 @@ func HookPostReceive(ctx *gitea_context.PrivateContext) { wasEmpty = repo.IsEmpty } - pusher, err := cache.GetWithContextCache(ctx, contextCachePusherKey, opts.UserID, func() (*user_model.User, error) { - return 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{ @@ -321,55 +321,55 @@ func HookPostReceive(ctx *gitea_context.PrivateContext) { }) } -const contextCachePusherKey = "hook_post_receive_pusher" +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 only push 1 commit func handlePullRequestMerging(ctx *gitea_context.PrivateContext, opts *private.HookOptions, ownerName, repoName string, updates []*repo_module.PushUpdateOptions) { - if opts.PushTrigger != string(repo_module.PushTriggerPRMergeToBase) || len(updates) < 1 { + 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 } + if len(updates) != 1 && !setting.IsProd { + // it shouldn't happen + panic(fmt.Sprintf("Pushing a merged PR (pr:%d) should only push 1 commit, but got %d", opts.PullRequestID, len(updates))) + } - // Get the pull request 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: fmt.Sprintf("GetPullRequestByID[%d]: %v", opts.PullRequestID, err), - }) + ctx.JSON(http.StatusInternalServerError, private.HookPostReceiveResult{Err: "GetPullRequestByID failed"}) return } - pusher, err := cache.GetWithContextCache(ctx, contextCachePusherKey, opts.UserID, func() (*user_model.User, error) { - return 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{ - Err: fmt.Sprintf("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 = opts.UserID - - if err := db.WithTx(ctx, func(ctx context.Context) error { + 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("Failed to SetMerged: %s/%s Error: %v", ownerName, repoName, err) + return fmt.Errorf("SetMerged failed: %s/%s Error: %v", ownerName, repoName, err) } return nil - }); err != nil { - log.Error("%v", err) - ctx.JSON(http.StatusInternalServerError, private.HookPostReceiveResult{ - Err: err.Error(), - }) - return + }) + 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..d4401bc699 --- /dev/null +++ b/routers/private/hook_post_receive_test.go @@ -0,0 +1,37 @@ +// 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" + "code.gitea.io/gitea/models/unittest" + "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)) + 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) + // TODO: test the removal of auto merge +} diff --git a/services/contexttest/context_tests.go b/services/contexttest/context_tests.go index 0c1e5ee54f..5624d24058 100644 --- a/services/contexttest/context_tests.go +++ b/services/contexttest/context_tests.go @@ -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 diff --git a/services/pull/update.go b/services/pull/update.go index d912d5a8d5..d2c0c2df80 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() { go AddTestPullRequestTask(doer, reversePR.HeadRepo.ID, reversePR.HeadBranch, false, "", "")