From 945f121262ef5a393d9530b62a82b2c141bbe21c Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Fri, 18 Oct 2019 19:13:31 +0800 Subject: [PATCH] Fix bug on pull requests when transfer head repository (#8564) * fix bug on pull requests when transfer head repository * add migration and fix lint * fix tests and add a cache check on LoadBaseRepo --- models/fixtures/pull_request.yml | 3 --- models/migrations/migrations.go | 2 ++ models/migrations/v102.go | 15 +++++++++++ models/pull.go | 46 ++++++++++++++++++++++---------- models/pull_test.go | 14 ---------- models/user.go | 4 --- modules/migrations/gitea.go | 15 +++++------ routers/api/v1/repo/pull.go | 19 +++++++------ routers/repo/pull.go | 31 +++++++++++---------- services/pull/merge.go | 11 +++++--- 10 files changed, 87 insertions(+), 73 deletions(-) create mode 100644 models/migrations/v102.go diff --git a/models/fixtures/pull_request.yml b/models/fixtures/pull_request.yml index baaaf6bb8a..505584ea18 100644 --- a/models/fixtures/pull_request.yml +++ b/models/fixtures/pull_request.yml @@ -6,7 +6,6 @@ index: 2 head_repo_id: 1 base_repo_id: 1 - head_user_name: user1 head_branch: branch1 base_branch: master merge_base: 1234567890abcdef @@ -21,7 +20,6 @@ index: 3 head_repo_id: 1 base_repo_id: 1 - head_user_name: user1 head_branch: branch2 base_branch: master merge_base: fedcba9876543210 @@ -35,7 +33,6 @@ index: 1 head_repo_id: 11 base_repo_id: 10 - head_user_name: user13 head_branch: branch2 base_branch: master merge_base: 0abcb056019adb83 diff --git a/models/migrations/migrations.go b/models/migrations/migrations.go index 19b1095cd3..8064eccfc1 100644 --- a/models/migrations/migrations.go +++ b/models/migrations/migrations.go @@ -258,6 +258,8 @@ var migrations = []Migration{ NewMigration("update migration repositories' service type", updateMigrationServiceTypes), // v101 -> v102 NewMigration("change length of some external login users columns", changeSomeColumnsLengthOfExternalLoginUser), + // v102 -> v103 + NewMigration("update migration repositories' service type", dropColumnHeadUserNameOnPullRequest), } // Migrate database to current version diff --git a/models/migrations/v102.go b/models/migrations/v102.go new file mode 100644 index 0000000000..74e8574ec3 --- /dev/null +++ b/models/migrations/v102.go @@ -0,0 +1,15 @@ +// Copyright 2019 The Gitea Authors. All rights reserved. +// Use of this source code is governed by a MIT-style +// license that can be found in the LICENSE file. + +package migrations + +import ( + "xorm.io/xorm" +) + +func dropColumnHeadUserNameOnPullRequest(x *xorm.Engine) error { + sess := x.NewSession() + defer sess.Close() + return dropTableColumns(sess, "pull_request", "head_user_name") +} diff --git a/models/pull.go b/models/pull.go index 817ea09cca..a3554a1b3d 100644 --- a/models/pull.go +++ b/models/pull.go @@ -66,7 +66,6 @@ type PullRequest struct { HeadRepo *Repository `xorm:"-"` BaseRepoID int64 `xorm:"INDEX"` BaseRepo *Repository `xorm:"-"` - HeadUserName string HeadBranch string BaseBranch string ProtectedBranch *ProtectedBranch `xorm:"-"` @@ -79,6 +78,15 @@ type PullRequest struct { MergedUnix timeutil.TimeStamp `xorm:"updated INDEX"` } +// MustHeadUserName returns the HeadRepo's username if failed return blank +func (pr *PullRequest) MustHeadUserName() string { + if err := pr.LoadHeadRepo(); err != nil { + log.Error("LoadHeadRepo: %v", err) + return "" + } + return pr.HeadRepo.MustOwnerName() +} + // Note: don't try to get Issue because will end up recursive querying. func (pr *PullRequest) loadAttributes(e Engine) (err error) { if pr.HasMerged && pr.Merger == nil { @@ -102,6 +110,10 @@ func (pr *PullRequest) LoadAttributes() error { // LoadBaseRepo loads pull request base repository from database func (pr *PullRequest) LoadBaseRepo() error { if pr.BaseRepo == nil { + if pr.HeadRepoID == pr.BaseRepoID && pr.HeadRepo != nil { + pr.BaseRepo = pr.HeadRepo + return nil + } var repo Repository if has, err := x.ID(pr.BaseRepoID).Get(&repo); err != nil { return err @@ -113,6 +125,24 @@ func (pr *PullRequest) LoadBaseRepo() error { return nil } +// LoadHeadRepo loads pull request head repository from database +func (pr *PullRequest) LoadHeadRepo() error { + if pr.HeadRepo == nil { + if pr.HeadRepoID == pr.BaseRepoID && pr.BaseRepo != nil { + pr.HeadRepo = pr.BaseRepo + return nil + } + var repo Repository + if has, err := x.ID(pr.HeadRepoID).Get(&repo); err != nil { + return err + } else if !has { + return ErrRepoNotExist{ID: pr.BaseRepoID} + } + pr.HeadRepo = &repo + } + return nil +} + // LoadIssue loads issue information from database func (pr *PullRequest) LoadIssue() (err error) { return pr.loadIssue(x) @@ -152,7 +182,7 @@ func (pr *PullRequest) GetDefaultMergeMessage() string { return "" } } - return fmt.Sprintf("Merge branch '%s' of %s/%s into %s", pr.HeadBranch, pr.HeadUserName, pr.HeadRepo.Name, pr.BaseBranch) + return fmt.Sprintf("Merge branch '%s' of %s/%s into %s", pr.HeadBranch, pr.MustHeadUserName(), pr.HeadRepo.Name, pr.BaseBranch) } // GetDefaultSquashMessage returns default message used when squash and merging pull request @@ -1072,18 +1102,6 @@ func (prs PullRequestList) InvalidateCodeComments(doer *User, repo *git.Reposito return prs.invalidateCodeComments(x, doer, repo, branch) } -// ChangeUsernameInPullRequests changes the name of head_user_name -func ChangeUsernameInPullRequests(oldUserName, newUserName string) error { - pr := PullRequest{ - HeadUserName: strings.ToLower(newUserName), - } - _, err := x. - Cols("head_user_name"). - Where("head_user_name = ?", strings.ToLower(oldUserName)). - Update(pr) - return err -} - // checkAndUpdateStatus checks if pull request is possible to leaving checking status, // and set to be either conflict or mergeable. func (pr *PullRequest) checkAndUpdateStatus() { diff --git a/models/pull_test.go b/models/pull_test.go index df051d51bc..8e2436b1a2 100644 --- a/models/pull_test.go +++ b/models/pull_test.go @@ -232,20 +232,6 @@ func TestPullRequestList_LoadAttributes(t *testing.T) { // TODO TestAddTestPullRequestTask -func TestChangeUsernameInPullRequests(t *testing.T) { - assert.NoError(t, PrepareTestDatabase()) - const newUsername = "newusername" - assert.NoError(t, ChangeUsernameInPullRequests("user1", newUsername)) - - prs := make([]*PullRequest, 0, 10) - assert.NoError(t, x.Where("head_user_name = ?", newUsername).Find(&prs)) - assert.Len(t, prs, 2) - for _, pr := range prs { - assert.Equal(t, newUsername, pr.HeadUserName) - } - CheckConsistencyFor(t, &PullRequest{}) -} - func TestPullRequest_IsWorkInProgress(t *testing.T) { assert.NoError(t, PrepareTestDatabase()) diff --git a/models/user.go b/models/user.go index c393d8dce5..a3679c9a5b 100644 --- a/models/user.go +++ b/models/user.go @@ -994,10 +994,6 @@ func ChangeUserName(u *User, newUserName string) (err error) { return ErrUserAlreadyExist{newUserName} } - if err = ChangeUsernameInPullRequests(u.Name, newUserName); err != nil { - return fmt.Errorf("ChangeUsernameInPullRequests: %v", err) - } - // Do not fail if directory does not exist if err = os.Rename(UserPath(u.Name), UserPath(newUserName)); err != nil && !os.IsNotExist(err) { return fmt.Errorf("Rename user directory: %v", err) diff --git a/modules/migrations/gitea.go b/modules/migrations/gitea.go index 2452a7a883..a095751c6b 100644 --- a/modules/migrations/gitea.go +++ b/modules/migrations/gitea.go @@ -579,14 +579,13 @@ func (g *GiteaLocalUploader) newPullRequest(pr *base.PullRequest) (*models.PullR } var pullRequest = models.PullRequest{ - HeadRepoID: g.repo.ID, - HeadBranch: head, - HeadUserName: g.repoOwner, - BaseRepoID: g.repo.ID, - BaseBranch: pr.Base.Ref, - MergeBase: pr.Base.SHA, - Index: pr.Number, - HasMerged: pr.Merged, + HeadRepoID: g.repo.ID, + HeadBranch: head, + BaseRepoID: g.repo.ID, + BaseBranch: pr.Base.Ref, + MergeBase: pr.Base.SHA, + Index: pr.Number, + HasMerged: pr.Merged, Issue: &issue, } diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index 978c8a3f1f..16ddd10c60 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -190,7 +190,7 @@ func CreatePullRequest(ctx *context.APIContext, form api.CreatePullRequestOption ) // Get repo/branch information - headUser, headRepo, headGitRepo, compareInfo, baseBranch, headBranch := parseCompareInfo(ctx, form) + _, headRepo, headGitRepo, compareInfo, baseBranch, headBranch := parseCompareInfo(ctx, form) if ctx.Written() { return } @@ -265,15 +265,14 @@ func CreatePullRequest(ctx *context.APIContext, form api.CreatePullRequestOption DeadlineUnix: deadlineUnix, } pr := &models.PullRequest{ - HeadRepoID: headRepo.ID, - BaseRepoID: repo.ID, - HeadUserName: headUser.Name, - HeadBranch: headBranch, - BaseBranch: baseBranch, - HeadRepo: headRepo, - BaseRepo: repo, - MergeBase: compareInfo.MergeBase, - Type: models.PullRequestGitea, + HeadRepoID: headRepo.ID, + BaseRepoID: repo.ID, + HeadBranch: headBranch, + BaseBranch: baseBranch, + HeadRepo: headRepo, + BaseRepo: repo, + MergeBase: compareInfo.MergeBase, + Type: models.PullRequestGitea, } // Get all assignee IDs diff --git a/routers/repo/pull.go b/routers/repo/pull.go index 8b97e55670..4001e3a4f8 100644 --- a/routers/repo/pull.go +++ b/routers/repo/pull.go @@ -272,12 +272,12 @@ func checkPullInfo(ctx *context.Context) *models.Issue { } func setMergeTarget(ctx *context.Context, pull *models.PullRequest) { - if ctx.Repo.Owner.Name == pull.HeadUserName { + if ctx.Repo.Owner.Name == pull.MustHeadUserName() { ctx.Data["HeadTarget"] = pull.HeadBranch } else if pull.HeadRepo == nil { - ctx.Data["HeadTarget"] = pull.HeadUserName + ":" + pull.HeadBranch + ctx.Data["HeadTarget"] = pull.MustHeadUserName() + ":" + pull.HeadBranch } else { - ctx.Data["HeadTarget"] = pull.HeadUserName + "/" + pull.HeadRepo.Name + ":" + pull.HeadBranch + ctx.Data["HeadTarget"] = pull.MustHeadUserName() + "/" + pull.HeadRepo.Name + ":" + pull.HeadBranch } ctx.Data["BaseTarget"] = pull.BaseBranch } @@ -440,7 +440,7 @@ func ViewPullCommits(ctx *context.Context) { ctx.NotFound("ViewPullCommits", nil) return } - ctx.Data["Username"] = pull.HeadUserName + ctx.Data["Username"] = pull.MustHeadUserName() ctx.Data["Reponame"] = pull.HeadRepo.Name commits = prInfo.Commits } @@ -512,7 +512,7 @@ func ViewPullFiles(ctx *context.Context) { return } - headRepoPath := models.RepoPath(pull.HeadUserName, pull.HeadRepo.Name) + headRepoPath := pull.HeadRepo.RepoPath() headGitRepo, err := git.OpenRepository(headRepoPath) if err != nil { @@ -531,8 +531,8 @@ func ViewPullFiles(ctx *context.Context) { endCommitID = headCommitID gitRepo = headGitRepo - headTarget = path.Join(pull.HeadUserName, pull.HeadRepo.Name) - ctx.Data["Username"] = pull.HeadUserName + headTarget = path.Join(pull.MustHeadUserName(), pull.HeadRepo.Name) + ctx.Data["Username"] = pull.MustHeadUserName() ctx.Data["Reponame"] = pull.HeadRepo.Name } @@ -754,15 +754,14 @@ func CompareAndPullRequestPost(ctx *context.Context, form auth.CreateIssueForm) Content: form.Content, } pullRequest := &models.PullRequest{ - HeadRepoID: headRepo.ID, - BaseRepoID: repo.ID, - HeadUserName: headUser.Name, - HeadBranch: headBranch, - BaseBranch: baseBranch, - HeadRepo: headRepo, - BaseRepo: repo, - MergeBase: prInfo.MergeBase, - Type: models.PullRequestGitea, + HeadRepoID: headRepo.ID, + BaseRepoID: repo.ID, + HeadBranch: headBranch, + BaseBranch: baseBranch, + HeadRepo: headRepo, + BaseRepo: repo, + MergeBase: prInfo.MergeBase, + Type: models.PullRequestGitea, } // FIXME: check error in the case two people send pull request at almost same time, give nice error prompt // instead of 500. diff --git a/services/pull/merge.go b/services/pull/merge.go index 0d762dbc30..5eb8eaa4d4 100644 --- a/services/pull/merge.go +++ b/services/pull/merge.go @@ -71,7 +71,7 @@ func Merge(pr *models.PullRequest, doer *models.User, baseGitRepo *git.Repositor } }() - headRepoPath := models.RepoPath(pr.HeadUserName, pr.HeadRepo.Name) + headRepoPath := pr.HeadRepo.RepoPath() if err := git.InitRepository(tmpBasePath, false); err != nil { return fmt.Errorf("git init: %v", err) @@ -306,14 +306,17 @@ func Merge(pr *models.PullRequest, doer *models.User, baseGitRepo *git.Repositor } } - headUser, err := models.GetUserByName(pr.HeadUserName) + var headUser *models.User + err = pr.HeadRepo.GetOwner() if err != nil { if !models.IsErrUserNotExist(err) { - log.Error("Can't find user: %s for head repository - %v", pr.HeadUserName, err) + log.Error("Can't find user: %d for head repository - %v", pr.HeadRepo.OwnerID, err) return err } - log.Error("Can't find user: %s for head repository - defaulting to doer: %s - %v", pr.HeadUserName, doer.Name, err) + log.Error("Can't find user: %d for head repository - defaulting to doer: %s - %v", pr.HeadRepo.OwnerID, doer.Name, err) headUser = doer + } else { + headUser = pr.HeadRepo.Owner } env = models.FullPushingEnvironment(