STACKITGit/cli
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Tweak HTTP 422 handling when deleting branches

Browse source
This commit is contained in:
Mislav Marohnić 2020-06-30 19:13:54 +02:00
parent 3afec6f90a
commit 1ca3d171e6
7 changed files with 73 additions and 58 deletions
Download patch file Download diff file Expand all files Collapse all files

45
api/client.go
View file

@ -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($|;)`)

23
api/client_test.go
View file

@ -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())
}
}

14
api/queries_pr.go
View file

@ -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 {

37
api/queries_pr_test.go
View file

@ -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)
}
})

2
command/issue_test.go
View file

@ -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)
}

4
command/pr.go
View file

@ -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
}

6
pkg/httpmock/stub.go
View file

@ -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)