Skip to content

Commit 1ca3d17

Browse files
mislavmislav
committed
Tweak HTTP 422 handling when deleting branches
1 parent 3afec6f commit 1ca3d17

File tree

7 files changed

+72
-57
lines changed

7 files changed

+72
-57
lines changed

‎api/client.go‎

Lines changed: 23 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import (
77
"io"
88
"io/ioutil"
99
"net/http"
10+
"net/url"
1011
"regexp"
1112
"strings"
1213

@@ -154,18 +155,21 @@ func (gr GraphQLErrorResponse) Error() string {
154155
for _, e := range gr.Errors {
155156
errorMessages = append(errorMessages, e.Message)
156157
}
157-
return fmt.Sprintf("graphql error: '%s'", strings.Join(errorMessages, ", "))
158+
return fmt.Sprintf("GraphQL error: %s", strings.Join(errorMessages, "\n"))
158159
}
159160

160161
// HTTPError is an error returned by a failed API call
161162
type HTTPError struct {
162-
Code int
163-
URL string
164-
Message string
163+
StatusCode int
164+
RequestURL *url.URL
165+
Message string
165166
}
166167

167-
func (e HTTPError) Error() string {
168-
return fmt.Sprintf("http error, '%s' failed (%d): '%s'", e.URL, e.Code, e.Message)
168+
func (err HTTPError) Error() string {
169+
if err.Message != "" {
170+
return fmt.Sprintf("HTTP %d: %s (%s)", err.StatusCode, err.Message, err.RequestURL)
171+
}
172+
return fmt.Sprintf("HTTP %d (%s)", err.StatusCode, err.RequestURL)
169173
}
170174

171175
// Returns whether or not scopes are present, appID, and error
@@ -294,26 +298,25 @@ func handleResponse(resp *http.Response, data interface{}) error {
294298
}
295299

296300
func handleHTTPError(resp *http.Response) error {
297-
var message string
298-
var parsedBody struct {
299-
Message string `json:"message"`
301+
httpError := HTTPError{
302+
StatusCode: resp.StatusCode,
303+
RequestURL: resp.Request.URL,
300304
}
305+
301306
body, err := ioutil.ReadAll(resp.Body)
302307
if err != nil {
303-
return err
304-
}
305-
err = json.Unmarshal(body, &parsedBody)
306-
if err != nil {
307-
message = string(body)
308-
} else {
309-
message = parsedBody.Message
308+
httpError.Message = err.Error()
309+
return httpError
310310
}
311311

312-
return HTTPError{
313-
Code: resp.StatusCode,
314-
URL: resp.Request.URL.String(),
315-
Message: message,
312+
var parsedBody struct {
313+
Message string `json:"message"`
314+
}
315+
if err := json.Unmarshal(body, &parsedBody); err == nil {
316+
httpError.Message = parsedBody.Message
316317
}
318+
319+
return httpError
317320
}
318321

319322
var jsonTypeRE = regexp.MustCompile(`[/+]json($|;)`)

‎api/client_test.go‎

Lines changed: 18 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -47,9 +47,15 @@ func TestGraphQLError(t *testing.T) {
4747
client := NewClient(ReplaceTripper(http))
4848

4949
response := struct{}{}
50-
http.StubResponse(200, bytes.NewBufferString(`{"errors":[{"message":"OH NO"}]}`))
50+
http.StubResponse(200, bytes.NewBufferString(`
51+
{ "errors": [
52+
{"message":"OH NO"},
53+
{"message":"this is fine"}
54+
]
55+
}`))
56+
5157
err := client.GraphQL("", nil, &response)
52-
if err == nil || err.Error() != "graphql error: 'OH NO'" {
58+
if err == nil || err.Error() != "GraphQL error: OH NO\nthis is fine" {
5359
t.Fatalf("got %q", err.Error())
5460
}
5561
}
@@ -75,8 +81,15 @@ func TestRESTError(t *testing.T) {
7581
http.StubResponse(422, bytes.NewBufferString(`{"message": "OH NO"}`))
7682

7783
var httpErr HTTPError
78-
err := client.REST("DELETE", "/repos/branch", nil, nil)
79-
if err == nil || !errors.As(err, &httpErr) || httpErr.Code != 422 {
80-
t.Fatalf("got %q", err.Error())
84+
err := client.REST("DELETE", "repos/branch", nil, nil)
85+
if err == nil || !errors.As(err, &httpErr) {
86+
t.Fatalf("got %v", err)
87+
}
88+
89+
if httpErr.StatusCode != 422 {
90+
t.Errorf("expected status code 422, got %d", httpErr.StatusCode)
91+
}
92+
if httpErr.Error() != "HTTP 422: OH NO (https://api.github.com/repos/branch)" {
93+
t.Errorf("got %q", httpErr.Error())
8194
}
8295
}

‎api/queries_pr.go‎

Lines changed: 1 addition & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1011,20 +1011,8 @@ func PullRequestReady(client *Client, repo ghrepo.Interface, pr *PullRequest) er
10111011
}
10121012

10131013
func BranchDeleteRemote(client *Client, repo ghrepo.Interface, branch string) error {
1014-
var response struct {
1015-
NodeID string `json:"node_id"`
1016-
}
10171014
path := fmt.Sprintf("repos/%s/%s/git/refs/heads/%s", repo.RepoOwner(), repo.RepoName(), branch)
1018-
err := client.REST("DELETE", path, nil, &response)
1019-
if err != nil {
1020-
var httpErr HTTPError
1021-
// The ref might have already been deleted by GitHub
1022-
if !errors.As(err, &httpErr) || httpErr.Code != 422 {
1023-
return err
1024-
}
1025-
}
1026-
1027-
return nil
1015+
return client.REST("DELETE", path, nil, nil)
10281016
}
10291017

10301018
func min(a, b int) int {

‎api/queries_pr_test.go‎

Lines changed: 20 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,6 @@
11
package api
22

33
import (
4-
"bytes"
54
"testing"
65

76
"github.com/cli/cli/internal/ghrepo"
@@ -10,31 +9,35 @@ import (
109

1110
func TestBranchDeleteRemote(t *testing.T) {
1211
var tests = []struct {
13-
name string
14-
code int
15-
body string
16-
expectError bool
12+
name string
13+
responseStatus int
14+
responseBody string
15+
expectError bool
1716
}{
18-
{name: "success", code: 204, body: "", expectError: false},
19-
{name: "error", code: 500, body: `{"message": "oh no"}`, expectError: true},
2017
{
21-
name: "already_deleted",
22-
code: 422,
23-
body: `{"message": "Reference does not exist"}`,
24-
expectError: false,
18+
name: "success",
19+
responseStatus: 204,
20+
responseBody: "",
21+
expectError: false,
22+
},
23+
{
24+
name: "error",
25+
responseStatus: 500,
26+
responseBody: `{"message": "oh no"}`,
27+
expectError: true,
2528
},
2629
}
2730

28-
for _, tc := range tests {
29-
tc := tc
30-
t.Run(tc.name, func(t *testing.T) {
31+
for _, tt := range tests {
32+
t.Run(tt.name, func(t *testing.T) {
3133
http := &httpmock.Registry{}
32-
client := NewClient(ReplaceTripper(http))
34+
http.Register(httpmock.MatchAny, httpmock.StatusStringResponse(tt.responseStatus, tt.responseBody))
3335

34-
http.StubResponse(tc.code, bytes.NewBufferString(tc.body))
36+
client := NewClient(ReplaceTripper(http))
3537
repo, _ := ghrepo.FromFullName("OWNER/REPO")
38+
3639
err := BranchDeleteRemote(client, repo, "branch")
37-
if isError := err != nil; isError != tc.expectError {
40+
if (err != nil) != tt.expectError {
3841
t.Fatalf("unexpected result: %v", err)
3942
}
4043
})

‎command/issue_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -402,7 +402,7 @@ func TestIssueView_web_notFound(t *testing.T) {
402402
defer restoreCmd()
403403

404404
_, err := RunCommand("issue view -w 9999")
405-
if err == nil || err.Error() != "graphql error: 'Could not resolve to an Issue with the number of 9999.'" {
405+
if err == nil || err.Error() != "GraphQL error: Could not resolve to an Issue with the number of 9999." {
406406
t.Errorf("error running command `issue view`: %v", err)
407407
}
408408

‎command/pr.go‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -523,7 +523,9 @@ func prMerge(cmd *cobra.Command, args []string) error {
523523

524524
if !crossRepoPR {
525525
err = api.BranchDeleteRemote(apiClient, baseRepo, pr.HeadRefName)
526-
if err != nil {
526+
var httpErr api.HTTPError
527+
// The ref might have already been deleted by GitHub
528+
if err != nil && (!errors.As(err, &httpErr) || httpErr.StatusCode != 422) {
527529
err = fmt.Errorf("failed to delete remote branch %s: %w", utils.Cyan(pr.HeadRefName), err)
528530
return err
529531
}

‎pkg/httpmock/stub.go‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,12 @@ func StringResponse(body string) Responder {
6464
}
6565
}
6666

67+
func StatusStringResponse(status int, body string) Responder {
68+
return func(req *http.Request) (*http.Response, error) {
69+
return httpResponse(status, req, bytes.NewBufferString(body)), nil
70+
}
71+
}
72+
6773
func JSONResponse(body interface{}) Responder {
6874
return func(req *http.Request) (*http.Response, error) {
6975
b, _ := json.Marshal(body)

0 commit comments

Comments
 (0)