Skip to content

Commit acd7053

Browse files
authored
Avoid returning without written ctx when posting PR (#31843)
Fix #31625. If `pull_service.NewPullRequest` return an error which misses each `if` check, `CompareAndPullRequestPost` will return immediately, since it doesn't write the HTTP response, a 200 response with empty body will be sent to clients. ```go if err := pull_service.NewPullRequest(ctx, repo, pullIssue, labelIDs, attachments, pullRequest, assigneeIDs); err != nil { if repo_model.IsErrUserDoesNotHaveAccessToRepo(err) { ctx.Error(http.StatusBadRequest, "UserDoesNotHaveAccessToRepo", err.Error()) } else if git.IsErrPushRejected(err) { // ... ctx.JSONError(flashError) } else if errors.Is(err, user_model.ErrBlockedUser) { // ... ctx.JSONError(flashError) } else if errors.Is(err, issues_model.ErrMustCollaborator) { // ... ctx.JSONError(flashError) } return } ``` Not sure what kind of error can cause it to happen, so this PR just expose it. And we can fix it when users report that creating PRs failed with error responses. It's all my guess since I cannot reproduce the problem, but even if it's not related, the code here needs to be improved.
1 parent 2010fbe commit acd7053

File tree

1 file changed

+10
-4
lines changed

1 file changed

+10
-4
lines changed

routers/web/repo/pull.go

+10-4
Original file line numberDiff line numberDiff line change
@@ -1308,9 +1308,10 @@ func CompareAndPullRequestPost(ctx *context.Context) {
13081308
// instead of 500.
13091309

13101310
if err := pull_service.NewPullRequest(ctx, repo, pullIssue, labelIDs, attachments, pullRequest, assigneeIDs); err != nil {
1311-
if repo_model.IsErrUserDoesNotHaveAccessToRepo(err) {
1311+
switch {
1312+
case repo_model.IsErrUserDoesNotHaveAccessToRepo(err):
13121313
ctx.Error(http.StatusBadRequest, "UserDoesNotHaveAccessToRepo", err.Error())
1313-
} else if git.IsErrPushRejected(err) {
1314+
case git.IsErrPushRejected(err):
13141315
pushrejErr := err.(*git.ErrPushRejected)
13151316
message := pushrejErr.Message
13161317
if len(message) == 0 {
@@ -1327,7 +1328,7 @@ func CompareAndPullRequestPost(ctx *context.Context) {
13271328
return
13281329
}
13291330
ctx.JSONError(flashError)
1330-
} else if errors.Is(err, user_model.ErrBlockedUser) {
1331+
case errors.Is(err, user_model.ErrBlockedUser):
13311332
flashError, err := ctx.RenderToHTML(tplAlertDetails, map[string]any{
13321333
"Message": ctx.Tr("repo.pulls.push_rejected"),
13331334
"Summary": ctx.Tr("repo.pulls.new.blocked_user"),
@@ -1337,7 +1338,7 @@ func CompareAndPullRequestPost(ctx *context.Context) {
13371338
return
13381339
}
13391340
ctx.JSONError(flashError)
1340-
} else if errors.Is(err, issues_model.ErrMustCollaborator) {
1341+
case errors.Is(err, issues_model.ErrMustCollaborator):
13411342
flashError, err := ctx.RenderToHTML(tplAlertDetails, map[string]any{
13421343
"Message": ctx.Tr("repo.pulls.push_rejected"),
13431344
"Summary": ctx.Tr("repo.pulls.new.must_collaborator"),
@@ -1347,6 +1348,11 @@ func CompareAndPullRequestPost(ctx *context.Context) {
13471348
return
13481349
}
13491350
ctx.JSONError(flashError)
1351+
default:
1352+
// It's an unexpected error.
1353+
// If it happens, we should add another case to handle it.
1354+
log.Error("Unexpected error of NewPullRequest: %T %s", err, err)
1355+
ctx.ServerError("CompareAndPullRequest", err)
13501356
}
13511357
return
13521358
}

0 commit comments

Comments
 (0)