diff --git a/api/client.go b/api/client.go index 6880609bb..74a5a0417 100644 --- a/api/client.go +++ b/api/client.go @@ -7,6 +7,7 @@ import ( "io" "io/ioutil" "net/http" + "net/url" "regexp" "strings" @@ -154,18 +155,21 @@ func (gr GraphQLErrorResponse) Error() string { for _, e := range gr.Errors { errorMessages = append(errorMessages, e.Message) } - return fmt.Sprintf("graphql error: '%s'", strings.Join(errorMessages, ", ")) + return fmt.Sprintf("GraphQL error: %s", strings.Join(errorMessages, "\n")) } // HTTPError is an error returned by a failed API call type HTTPError struct { - Code int - URL string - Message string + StatusCode int + RequestURL *url.URL + Message string } -func (e HTTPError) Error() string { - return fmt.Sprintf("http error, '%s' failed (%d): '%s'", e.URL, e.Code, e.Message) +func (err HTTPError) Error() string { + if err.Message != "" { + return fmt.Sprintf("HTTP %d: %s (%s)", err.StatusCode, err.Message, err.RequestURL) + } + return fmt.Sprintf("HTTP %d (%s)", err.StatusCode, err.RequestURL) } // Returns whether or not scopes are present, appID, and error @@ -294,26 +298,25 @@ func handleResponse(resp *http.Response, data interface{}) error { } func handleHTTPError(resp *http.Response) error { - var message string + httpError := HTTPError{ + StatusCode: resp.StatusCode, + RequestURL: resp.Request.URL, + } + + body, err := ioutil.ReadAll(resp.Body) + if err != nil { + httpError.Message = err.Error() + return httpError + } + var parsedBody struct { Message string `json:"message"` } - body, err := ioutil.ReadAll(resp.Body) - if err != nil { - return err - } - err = json.Unmarshal(body, &parsedBody) - if err != nil { - message = string(body) - } else { - message = parsedBody.Message + if err := json.Unmarshal(body, &parsedBody); err == nil { + httpError.Message = parsedBody.Message } - return HTTPError{ - Code: resp.StatusCode, - URL: resp.Request.URL.String(), - Message: message, - } + return httpError } var jsonTypeRE = regexp.MustCompile(`[/+]json($|;)`) diff --git a/api/client_test.go b/api/client_test.go index 9c908be12..b7c226c8f 100644 --- a/api/client_test.go +++ b/api/client_test.go @@ -47,9 +47,15 @@ func TestGraphQLError(t *testing.T) { client := NewClient(ReplaceTripper(http)) response := struct{}{} - http.StubResponse(200, bytes.NewBufferString(`{"errors":[{"message":"OH NO"}]}`)) + http.StubResponse(200, bytes.NewBufferString(` + { "errors": [ + {"message":"OH NO"}, + {"message":"this is fine"} + ] + }`)) + err := client.GraphQL("", nil, &response) - if err == nil || err.Error() != "graphql error: 'OH NO'" { + if err == nil || err.Error() != "GraphQL error: OH NO\nthis is fine" { t.Fatalf("got %q", err.Error()) } } @@ -75,8 +81,15 @@ func TestRESTError(t *testing.T) { http.StubResponse(422, bytes.NewBufferString(`{"message": "OH NO"}`)) var httpErr HTTPError - err := client.REST("DELETE", "/repos/branch", nil, nil) - if err == nil || !errors.As(err, &httpErr) || httpErr.Code != 422 { - t.Fatalf("got %q", err.Error()) + err := client.REST("DELETE", "repos/branch", nil, nil) + if err == nil || !errors.As(err, &httpErr) { + t.Fatalf("got %v", err) + } + + if httpErr.StatusCode != 422 { + t.Errorf("expected status code 422, got %d", httpErr.StatusCode) + } + if httpErr.Error() != "HTTP 422: OH NO (https://api.github.com/repos/branch)" { + t.Errorf("got %q", httpErr.Error()) } } diff --git a/api/queries_pr.go b/api/queries_pr.go index b42a94eec..56582d0e3 100644 --- a/api/queries_pr.go +++ b/api/queries_pr.go @@ -1011,20 +1011,8 @@ func PullRequestReady(client *Client, repo ghrepo.Interface, pr *PullRequest) er } func BranchDeleteRemote(client *Client, repo ghrepo.Interface, branch string) error { - var response struct { - NodeID string `json:"node_id"` - } path := fmt.Sprintf("repos/%s/%s/git/refs/heads/%s", repo.RepoOwner(), repo.RepoName(), branch) - err := client.REST("DELETE", path, nil, &response) - if err != nil { - var httpErr HTTPError - // The ref might have already been deleted by GitHub - if !errors.As(err, &httpErr) || httpErr.Code != 422 { - return err - } - } - - return nil + return client.REST("DELETE", path, nil, nil) } func min(a, b int) int { diff --git a/api/queries_pr_test.go b/api/queries_pr_test.go index 3b8bf0a65..07370023b 100644 --- a/api/queries_pr_test.go +++ b/api/queries_pr_test.go @@ -1,7 +1,6 @@ package api import ( - "bytes" "testing" "github.com/cli/cli/internal/ghrepo" @@ -10,31 +9,35 @@ import ( func TestBranchDeleteRemote(t *testing.T) { var tests = []struct { - name string - code int - body string - expectError bool + name string + responseStatus int + responseBody string + expectError bool }{ - {name: "success", code: 204, body: "", expectError: false}, - {name: "error", code: 500, body: `{"message": "oh no"}`, expectError: true}, { - name: "already_deleted", - code: 422, - body: `{"message": "Reference does not exist"}`, - expectError: false, + name: "success", + responseStatus: 204, + responseBody: "", + expectError: false, + }, + { + name: "error", + responseStatus: 500, + responseBody: `{"message": "oh no"}`, + expectError: true, }, } - for _, tc := range tests { - tc := tc - t.Run(tc.name, func(t *testing.T) { + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { http := &httpmock.Registry{} - client := NewClient(ReplaceTripper(http)) + http.Register(httpmock.MatchAny, httpmock.StatusStringResponse(tt.responseStatus, tt.responseBody)) - http.StubResponse(tc.code, bytes.NewBufferString(tc.body)) + client := NewClient(ReplaceTripper(http)) repo, _ := ghrepo.FromFullName("OWNER/REPO") + err := BranchDeleteRemote(client, repo, "branch") - if isError := err != nil; isError != tc.expectError { + if (err != nil) != tt.expectError { t.Fatalf("unexpected result: %v", err) } }) diff --git a/command/issue_test.go b/command/issue_test.go index 418de25d3..b7db7a7e2 100644 --- a/command/issue_test.go +++ b/command/issue_test.go @@ -402,7 +402,7 @@ func TestIssueView_web_notFound(t *testing.T) { defer restoreCmd() _, err := RunCommand("issue view -w 9999") - if err == nil || err.Error() != "graphql error: 'Could not resolve to an Issue with the number of 9999.'" { + if err == nil || err.Error() != "GraphQL error: Could not resolve to an Issue with the number of 9999." { t.Errorf("error running command `issue view`: %v", err) } diff --git a/command/pr.go b/command/pr.go index 2068a10e8..d1720ca42 100644 --- a/command/pr.go +++ b/command/pr.go @@ -523,7 +523,9 @@ func prMerge(cmd *cobra.Command, args []string) error { if !crossRepoPR { err = api.BranchDeleteRemote(apiClient, baseRepo, pr.HeadRefName) - if err != nil { + var httpErr api.HTTPError + // The ref might have already been deleted by GitHub + if err != nil && (!errors.As(err, &httpErr) || httpErr.StatusCode != 422) { err = fmt.Errorf("failed to delete remote branch %s: %w", utils.Cyan(pr.HeadRefName), err) return err } diff --git a/pkg/httpmock/stub.go b/pkg/httpmock/stub.go index cc42dfd6c..c57b7a1ad 100644 --- a/pkg/httpmock/stub.go +++ b/pkg/httpmock/stub.go @@ -64,6 +64,12 @@ func StringResponse(body string) Responder { } } +func StatusStringResponse(status int, body string) Responder { + return func(req *http.Request) (*http.Response, error) { + return httpResponse(status, req, bytes.NewBufferString(body)), nil + } +} + func JSONResponse(body interface{}) Responder { return func(req *http.Request) (*http.Response, error) { b, _ := json.Marshal(body)