Skip to content

Commit 123c254

Browse files
65436543
authored
Move checks for pulls before merge into own function (#19271) (#19277)
Backport #19271 Fix: * The API does ignore issue dependencies where Web does not * The API checks if "IsSignedIfRequired" where Web does not - UI probably do but nothing will some to craft custom requests * Default merge message is crafted a bit different between API and Web if not set on specific cases ...
1 parent db43f63 commit 123c254

File tree

7 files changed

+216
-214
lines changed

7 files changed

+216
-214
lines changed

‎models/pull.go

Lines changed: 11 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -222,22 +222,19 @@ func (pr *PullRequest) loadProtectedBranch(ctx context.Context) (err error) {
222222
}
223223

224224
// GetDefaultMergeMessage returns default message used when merging pull request
225-
func (pr *PullRequest) GetDefaultMergeMessage() string {
225+
func (pr *PullRequest) GetDefaultMergeMessage() (string, error) {
226226
if pr.HeadRepo == nil {
227227
var err error
228228
pr.HeadRepo, err = repo_model.GetRepositoryByID(pr.HeadRepoID)
229229
if err != nil {
230-
log.Error("GetRepositoryById[%d]: %v", pr.HeadRepoID, err)
231-
return ""
230+
return "", fmt.Errorf("GetRepositoryById[%d]: %v", pr.HeadRepoID, err)
232231
}
233232
}
234233
if err := pr.LoadIssue(); err != nil {
235-
log.Error("Cannot load issue %d for PR id %d: Error: %v", pr.IssueID, pr.ID, err)
236-
return ""
234+
return "", fmt.Errorf("Cannot load issue %d for PR id %d: Error: %v", pr.IssueID, pr.ID, err)
237235
}
238236
if err := pr.LoadBaseRepo(); err != nil {
239-
log.Error("LoadBaseRepo: %v", err)
240-
return ""
237+
return "", fmt.Errorf("LoadBaseRepo: %v", err)
241238
}
242239

243240
issueReference := "#"
@@ -246,10 +243,10 @@ func (pr *PullRequest) GetDefaultMergeMessage() string {
246243
}
247244

248245
if pr.BaseRepoID == pr.HeadRepoID {
249-
return fmt.Sprintf("Merge pull request '%s' (%s%d) from %s into %s", pr.Issue.Title, issueReference, pr.Issue.Index, pr.HeadBranch, pr.BaseBranch)
246+
return fmt.Sprintf("Merge pull request '%s' (%s%d) from %s into %s", pr.Issue.Title, issueReference, pr.Issue.Index, pr.HeadBranch, pr.BaseBranch), nil
250247
}
251248

252-
return fmt.Sprintf("Merge pull request '%s' (%s%d) from %s:%s into %s", pr.Issue.Title, issueReference, pr.Issue.Index, pr.HeadRepo.FullName(), pr.HeadBranch, pr.BaseBranch)
249+
return fmt.Sprintf("Merge pull request '%s' (%s%d) from %s:%s into %s", pr.Issue.Title, issueReference, pr.Issue.Index, pr.HeadRepo.FullName(), pr.HeadBranch, pr.BaseBranch), nil
253250
}
254251

255252
// ReviewCount represents a count of Reviews
@@ -335,19 +332,17 @@ func (pr *PullRequest) getReviewedByLines(writer io.Writer) error {
335332
}
336333

337334
// GetDefaultSquashMessage returns default message used when squash and merging pull request
338-
func (pr *PullRequest) GetDefaultSquashMessage() string {
335+
func (pr *PullRequest) GetDefaultSquashMessage() (string, error) {
339336
if err := pr.LoadIssue(); err != nil {
340-
log.Error("LoadIssue: %v", err)
341-
return ""
337+
return "", fmt.Errorf("LoadIssue: %v", err)
342338
}
343339
if err := pr.LoadBaseRepo(); err != nil {
344-
log.Error("LoadBaseRepo: %v", err)
345-
return ""
340+
return "", fmt.Errorf("LoadBaseRepo: %v", err)
346341
}
347342
if pr.BaseRepo.UnitEnabled(unit.TypeExternalTracker) {
348-
return fmt.Sprintf("%s (!%d)", pr.Issue.Title, pr.Issue.Index)
343+
return fmt.Sprintf("%s (!%d)", pr.Issue.Title, pr.Issue.Index), nil
349344
}
350-
return fmt.Sprintf("%s (#%d)", pr.Issue.Title, pr.Issue.Index)
345+
return fmt.Sprintf("%s (#%d)", pr.Issue.Title, pr.Issue.Index), nil
351346
}
352347

353348
// GetGitRefName returns git ref for hidden pull request branch

‎models/pull_test.go

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -261,11 +261,15 @@ func TestPullRequest_GetDefaultMergeMessage_InternalTracker(t *testing.T) {
261261
assert.NoError(t, unittest.PrepareTestDatabase())
262262
pr := unittest.AssertExistsAndLoadBean(t, &PullRequest{ID: 2}).(*PullRequest)
263263

264-
assert.Equal(t, "Merge pull request 'issue3' (#3) from branch2 into master", pr.GetDefaultMergeMessage())
264+
msg, err := pr.GetDefaultMergeMessage()
265+
assert.NoError(t, err)
266+
assert.Equal(t, "Merge pull request 'issue3' (#3) from branch2 into master", msg)
265267

266268
pr.BaseRepoID = 1
267269
pr.HeadRepoID = 2
268-
assert.Equal(t, "Merge pull request 'issue3' (#3) from user2/repo1:branch2 into master", pr.GetDefaultMergeMessage())
270+
msg, err = pr.GetDefaultMergeMessage()
271+
assert.NoError(t, err)
272+
assert.Equal(t, "Merge pull request 'issue3' (#3) from user2/repo1:branch2 into master", msg)
269273
}
270274

271275
func TestPullRequest_GetDefaultMergeMessage_ExternalTracker(t *testing.T) {
@@ -283,9 +287,13 @@ func TestPullRequest_GetDefaultMergeMessage_ExternalTracker(t *testing.T) {
283287

284288
pr := unittest.AssertExistsAndLoadBean(t, &PullRequest{ID: 2, BaseRepo: baseRepo}).(*PullRequest)
285289

286-
assert.Equal(t, "Merge pull request 'issue3' (!3) from branch2 into master", pr.GetDefaultMergeMessage())
290+
msg, err := pr.GetDefaultMergeMessage()
291+
assert.NoError(t, err)
292+
assert.Equal(t, "Merge pull request 'issue3' (!3) from branch2 into master", msg)
287293

288294
pr.BaseRepoID = 1
289295
pr.HeadRepoID = 2
290-
assert.Equal(t, "Merge pull request 'issue3' (!3) from user2/repo1:branch2 into master", pr.GetDefaultMergeMessage())
296+
msg, err = pr.GetDefaultMergeMessage()
297+
assert.NoError(t, err)
298+
assert.Equal(t, "Merge pull request 'issue3' (!3) from user2/repo1:branch2 into master", msg)
291299
}

‎routers/api/v1/repo/pull.go

Lines changed: 28 additions & 76 deletions
Original file line numberDiff line numberDiff line change
@@ -95,7 +95,6 @@ func ListPullRequests(ctx *context.APIContext) {
9595
Labels: ctx.FormStrings("labels"),
9696
MilestoneID: ctx.FormInt64("milestone"),
9797
})
98-
9998
if err != nil {
10099
ctx.Error(http.StatusInternalServerError, "PullRequests", err)
101100
return
@@ -724,13 +723,12 @@ func MergePullRequest(ctx *context.APIContext) {
724723
return
725724
}
726725

727-
if err = pr.LoadHeadRepo(); err != nil {
726+
if err := pr.LoadHeadRepo(); err != nil {
728727
ctx.Error(http.StatusInternalServerError, "LoadHeadRepo", err)
729728
return
730729
}
731730

732-
err = pr.LoadIssue()
733-
if err != nil {
731+
if err := pr.LoadIssue(); err != nil {
734732
ctx.Error(http.StatusInternalServerError, "LoadIssue", err)
735733
return
736734
}
@@ -744,29 +742,33 @@ func MergePullRequest(ctx *context.APIContext) {
744742
}
745743
}
746744

747-
if pr.Issue.IsClosed {
748-
ctx.NotFound()
749-
return
750-
}
745+
manuallMerge := repo_model.MergeStyle(form.Do) == repo_model.MergeStyleManuallyMerged
746+
force := form.ForceMerge != nil && *form.ForceMerge
751747

752-
allowedMerge, err := pull_service.IsUserAllowedToMerge(pr, ctx.Repo.Permission, ctx.User)
753-
if err != nil {
754-
ctx.Error(http.StatusInternalServerError, "IsUSerAllowedToMerge", err)
755-
return
756-
}
757-
if !allowedMerge {
758-
ctx.Error(http.StatusMethodNotAllowed, "Merge", "User not allowed to merge PR")
759-
return
760-
}
761-
762-
if pr.HasMerged {
763-
ctx.Error(http.StatusMethodNotAllowed, "PR already merged", "")
748+
if err := pull_service.CheckPullMergable(ctx, ctx.User, &ctx.Repo.Permission, pr, manuallMerge, force); err != nil {
749+
if errors.Is(err, pull_service.ErrIsClosed) {
750+
ctx.NotFound()
751+
} else if errors.Is(err, pull_service.ErrUserNotAllowedToMerge) {
752+
ctx.Error(http.StatusMethodNotAllowed, "Merge", "User not allowed to merge PR")
753+
} else if errors.Is(err, pull_service.ErrHasMerged) {
754+
ctx.Error(http.StatusMethodNotAllowed, "PR already merged", "")
755+
} else if errors.Is(err, pull_service.ErrIsWorkInProgress) {
756+
ctx.Error(http.StatusMethodNotAllowed, "PR is a work in progress", "Work in progress PRs cannot be merged")
757+
} else if errors.Is(err, pull_service.ErrNotMergableState) {
758+
ctx.Error(http.StatusMethodNotAllowed, "PR not in mergeable state", "Please try again later")
759+
} else if models.IsErrNotAllowedToMerge(err) {
760+
ctx.Error(http.StatusMethodNotAllowed, "PR is not ready to be merged", err)
761+
} else if asymkey_service.IsErrWontSign(err) {
762+
ctx.Error(http.StatusMethodNotAllowed, fmt.Sprintf("Protected branch %s requires signed commits but this merge would not be signed", pr.BaseBranch), err)
763+
} else {
764+
ctx.InternalServerError(err)
765+
}
764766
return
765767
}
766768

767769
// handle manually-merged mark
768-
if repo_model.MergeStyle(form.Do) == repo_model.MergeStyleManuallyMerged {
769-
if err = pull_service.MergedManually(pr, ctx.User, ctx.Repo.GitRepo, form.MergeCommitID); err != nil {
770+
if manuallMerge {
771+
if err := pull_service.MergedManually(pr, ctx.User, ctx.Repo.GitRepo, form.MergeCommitID); err != nil {
770772
if models.IsErrInvalidMergeStyle(err) {
771773
ctx.Error(http.StatusMethodNotAllowed, "Invalid merge style", fmt.Errorf("%s is not allowed an allowed merge style for this repository", repo_model.MergeStyle(form.Do)))
772774
return
@@ -782,63 +784,13 @@ func MergePullRequest(ctx *context.APIContext) {
782784
return
783785
}
784786

785-
if !pr.CanAutoMerge() {
786-
ctx.Error(http.StatusMethodNotAllowed, "PR not in mergeable state", "Please try again later")
787-
return
788-
}
789-
790-
if pr.IsWorkInProgress() {
791-
ctx.Error(http.StatusMethodNotAllowed, "PR is a work in progress", "Work in progress PRs cannot be merged")
787+
// set defaults to propagate needed fields
788+
if err := form.SetDefaults(pr); err != nil {
789+
ctx.ServerError("SetDefaults", fmt.Errorf("SetDefaults: %v", err))
792790
return
793791
}
794792

795-
if err := pull_service.CheckPRReadyToMerge(pr, false); err != nil {
796-
if !models.IsErrNotAllowedToMerge(err) {
797-
ctx.Error(http.StatusInternalServerError, "CheckPRReadyToMerge", err)
798-
return
799-
}
800-
if form.ForceMerge != nil && *form.ForceMerge {
801-
if isRepoAdmin, err := models.IsUserRepoAdmin(pr.BaseRepo, ctx.User); err != nil {
802-
ctx.Error(http.StatusInternalServerError, "IsUserRepoAdmin", err)
803-
return
804-
} else if !isRepoAdmin {
805-
ctx.Error(http.StatusMethodNotAllowed, "Merge", "Only repository admin can merge if not all checks are ok (force merge)")
806-
}
807-
} else {
808-
ctx.Error(http.StatusMethodNotAllowed, "PR is not ready to be merged", err)
809-
return
810-
}
811-
}
812-
813-
if _, err := pull_service.IsSignedIfRequired(pr, ctx.User); err != nil {
814-
if !asymkey_service.IsErrWontSign(err) {
815-
ctx.Error(http.StatusInternalServerError, "IsSignedIfRequired", err)
816-
return
817-
}
818-
ctx.Error(http.StatusMethodNotAllowed, fmt.Sprintf("Protected branch %s requires signed commits but this merge would not be signed", pr.BaseBranch), err)
819-
return
820-
}
821-
822-
if len(form.Do) == 0 {
823-
form.Do = string(repo_model.MergeStyleMerge)
824-
}
825-
826-
message := strings.TrimSpace(form.MergeTitleField)
827-
if len(message) == 0 {
828-
if repo_model.MergeStyle(form.Do) == repo_model.MergeStyleMerge {
829-
message = pr.GetDefaultMergeMessage()
830-
}
831-
if repo_model.MergeStyle(form.Do) == repo_model.MergeStyleSquash {
832-
message = pr.GetDefaultSquashMessage()
833-
}
834-
}
835-
836-
form.MergeMessageField = strings.TrimSpace(form.MergeMessageField)
837-
if len(form.MergeMessageField) > 0 {
838-
message += "\n\n" + form.MergeMessageField
839-
}
840-
841-
if err := pull_service.Merge(pr, ctx.User, ctx.Repo.GitRepo, repo_model.MergeStyle(form.Do), form.HeadCommitID, message); err != nil {
793+
if err := pull_service.Merge(pr, ctx.User, ctx.Repo.GitRepo, repo_model.MergeStyle(form.Do), form.HeadCommitID, form.MergeTitleField); err != nil {
842794
if models.IsErrInvalidMergeStyle(err) {
843795
ctx.Error(http.StatusMethodNotAllowed, "Invalid merge style", fmt.Errorf("%s is not allowed an allowed merge style for this repository", repo_model.MergeStyle(form.Do)))
844796
return

0 commit comments

Comments
 (0)