Skip to content

Commit 1cf8f69

Browse files
GiteaBotwolfogre
andauthored
Avoid returning without written ctx when posting PR (#31843) (#31848)
Backport #31843 by @wolfogre 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. Co-authored-by: Jason Song <[email protected]>
1 parent 771fb45 commit 1cf8f69

File tree

1 file changed

+10
-4
lines changed

1 file changed

+10
-4
lines changed

routers/web/repo/pull.go

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1296,9 +1296,10 @@ func CompareAndPullRequestPost(ctx *context.Context) {
12961296
// instead of 500.
12971297

12981298
if err := pull_service.NewPullRequest(ctx, repo, pullIssue, labelIDs, attachments, pullRequest, assigneeIDs); err != nil {
1299-
if repo_model.IsErrUserDoesNotHaveAccessToRepo(err) {
1299+
switch {
1300+
case repo_model.IsErrUserDoesNotHaveAccessToRepo(err):
13001301
ctx.Error(http.StatusBadRequest, "UserDoesNotHaveAccessToRepo", err.Error())
1301-
} else if git.IsErrPushRejected(err) {
1302+
case git.IsErrPushRejected(err):
13021303
pushrejErr := err.(*git.ErrPushRejected)
13031304
message := pushrejErr.Message
13041305
if len(message) == 0 {
@@ -1315,7 +1316,7 @@ func CompareAndPullRequestPost(ctx *context.Context) {
13151316
return
13161317
}
13171318
ctx.JSONError(flashError)
1318-
} else if errors.Is(err, user_model.ErrBlockedUser) {
1319+
case errors.Is(err, user_model.ErrBlockedUser):
13191320
flashError, err := ctx.RenderToHTML(tplAlertDetails, map[string]any{
13201321
"Message": ctx.Tr("repo.pulls.push_rejected"),
13211322
"Summary": ctx.Tr("repo.pulls.new.blocked_user"),
@@ -1325,7 +1326,7 @@ func CompareAndPullRequestPost(ctx *context.Context) {
13251326
return
13261327
}
13271328
ctx.JSONError(flashError)
1328-
} else if errors.Is(err, issues_model.ErrMustCollaborator) {
1329+
case errors.Is(err, issues_model.ErrMustCollaborator):
13291330
flashError, err := ctx.RenderToHTML(tplAlertDetails, map[string]any{
13301331
"Message": ctx.Tr("repo.pulls.push_rejected"),
13311332
"Summary": ctx.Tr("repo.pulls.new.must_collaborator"),
@@ -1335,6 +1336,11 @@ func CompareAndPullRequestPost(ctx *context.Context) {
13351336
return
13361337
}
13371338
ctx.JSONError(flashError)
1339+
default:
1340+
// It's an unexpected error.
1341+
// If it happens, we should add another case to handle it.
1342+
log.Error("Unexpected error of NewPullRequest: %T %s", err, err)
1343+
ctx.ServerError("CompareAndPullRequest", err)
13381344
}
13391345
return
13401346
}

0 commit comments

Comments
 (0)