Move database operations of merging a pull request to post receive hook and add a transaction (#30805) (#30888)

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>
This commit is contained in:
Giteabot 2024-05-08 22:17:18 +08:00 committed by GitHub
parent d4c2db39bf
commit ec3f5f9992
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
8 changed files with 150 additions and 19 deletions

View 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)

View 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

View 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

View 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"})
}
}

View File

@ -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})
}

View 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

View 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{

View 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, "", "")