From d57b5171cfa0a528652945d0a40bef257938e282 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Fri, 5 Jun 2020 18:24:24 +0200 Subject: [PATCH 1/2] Print HTTP errors on stderr in `api` command Most API errors are present in the response body itself, which will be sent to stdout normally, but if stdout is redirected somewhere (as it's common with scripts), failed HTTP requests will likely sabotage the rest of the script, but no useful info will be shown on stderr. This makes it so all REST and GraphQL errors are always shown on stderr. Additionally, this makes sure that the command exits with a nonzero status on any GraphQL errors. --- pkg/cmd/api/api.go | 43 +++++++++++++++++++++++++++++++++++++---- pkg/cmd/api/api_test.go | 27 +++++++++++++++++++++++++- 2 files changed, 65 insertions(+), 5 deletions(-) diff --git a/pkg/cmd/api/api.go b/pkg/cmd/api/api.go index 59db80332..052652aa0 100644 --- a/pkg/cmd/api/api.go +++ b/pkg/cmd/api/api.go @@ -1,6 +1,8 @@ package api import ( + "bytes" + "encoding/json" "fmt" "io" "io/ioutil" @@ -109,24 +111,57 @@ func apiRun(opts *ApiOptions) error { if resp.StatusCode == 204 { return nil } + var responseBody io.Reader = resp.Body defer resp.Body.Close() isJSON, _ := regexp.MatchString(`[/+]json(;|$)`, resp.Header.Get("Content-Type")) + var serverError string + if isJSON && (opts.RequestPath == "graphql" || resp.StatusCode >= 400) { + bodyCopy := &bytes.Buffer{} + b, err := ioutil.ReadAll(io.TeeReader(responseBody, bodyCopy)) + if err != nil { + return err + } + var respData struct { + Message string + Errors []struct { + Message string + } + } + err = json.Unmarshal(b, &respData) + if err != nil { + return err + } + if respData.Message != "" { + serverError = fmt.Sprintf("%s (HTTP %d)", respData.Message, resp.StatusCode) + } else if len(respData.Errors) > 0 { + msgs := make([]string, len(respData.Errors)) + for i, e := range respData.Errors { + msgs[i] = e.Message + } + serverError = strings.Join(msgs, "\n") + } + responseBody = bodyCopy + } + if isJSON && opts.IO.ColorEnabled() { - err = jsoncolor.Write(opts.IO.Out, resp.Body, " ") + err = jsoncolor.Write(opts.IO.Out, responseBody, " ") if err != nil { return err } } else { - _, err = io.Copy(opts.IO.Out, resp.Body) + _, err = io.Copy(opts.IO.Out, responseBody) if err != nil { return err } } - // TODO: detect GraphQL errors - if resp.StatusCode > 299 { + if serverError != "" { + fmt.Fprintf(opts.IO.ErrOut, "gh: %s\n", serverError) + return cmdutil.SilentError + } else if resp.StatusCode > 299 { + fmt.Fprintf(opts.IO.ErrOut, "gh: HTTP %d\n", resp.StatusCode) return cmdutil.SilentError } diff --git a/pkg/cmd/api/api_test.go b/pkg/cmd/api/api_test.go index 8149f5906..f01bab3e5 100644 --- a/pkg/cmd/api/api_test.go +++ b/pkg/cmd/api/api_test.go @@ -158,6 +158,31 @@ func Test_apiRun(t *testing.T) { stdout: ``, stderr: ``, }, + { + name: "REST error", + httpResponse: &http.Response{ + StatusCode: 400, + Body: ioutil.NopCloser(bytes.NewBufferString(`{"message": "THIS IS FINE"}`)), + Header: http.Header{"Content-Type": []string{"application/json; charset=utf-8"}}, + }, + err: cmdutil.SilentError, + stdout: `{"message": "THIS IS FINE"}`, + stderr: "gh: THIS IS FINE (HTTP 400)\n", + }, + { + name: "GraphQL error", + options: ApiOptions{ + RequestPath: "graphql", + }, + httpResponse: &http.Response{ + StatusCode: 200, + Body: ioutil.NopCloser(bytes.NewBufferString(`{"errors": [{"message":"AGAIN"}, {"message":"FINE"}]}`)), + Header: http.Header{"Content-Type": []string{"application/json; charset=utf-8"}}, + }, + err: cmdutil.SilentError, + stdout: `{"errors": [{"message":"AGAIN"}, {"message":"FINE"}]}`, + stderr: "gh: AGAIN\nFINE\n", + }, { name: "failure", httpResponse: &http.Response{ @@ -166,7 +191,7 @@ func Test_apiRun(t *testing.T) { }, err: cmdutil.SilentError, stdout: `gateway timeout`, - stderr: ``, + stderr: "gh: HTTP 502\n", }, } From 5d5bd04102f0d92e1ae5ae59c3be7d943f7d2cef Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Mon, 8 Jun 2020 15:11:36 +0200 Subject: [PATCH 2/2] :nail_care: Extract parsing error response into its own function --- pkg/cmd/api/api.go | 54 +++++++++++++++++++++++++++------------------- 1 file changed, 32 insertions(+), 22 deletions(-) diff --git a/pkg/cmd/api/api.go b/pkg/cmd/api/api.go index 052652aa0..a3f02107a 100644 --- a/pkg/cmd/api/api.go +++ b/pkg/cmd/api/api.go @@ -118,31 +118,10 @@ func apiRun(opts *ApiOptions) error { var serverError string if isJSON && (opts.RequestPath == "graphql" || resp.StatusCode >= 400) { - bodyCopy := &bytes.Buffer{} - b, err := ioutil.ReadAll(io.TeeReader(responseBody, bodyCopy)) + responseBody, serverError, err = parseErrorResponse(responseBody, resp.StatusCode) if err != nil { return err } - var respData struct { - Message string - Errors []struct { - Message string - } - } - err = json.Unmarshal(b, &respData) - if err != nil { - return err - } - if respData.Message != "" { - serverError = fmt.Sprintf("%s (HTTP %d)", respData.Message, resp.StatusCode) - } else if len(respData.Errors) > 0 { - msgs := make([]string, len(respData.Errors)) - for i, e := range respData.Errors { - msgs[i] = e.Message - } - serverError = strings.Join(msgs, "\n") - } - responseBody = bodyCopy } if isJSON && opts.IO.ColorEnabled() { @@ -234,3 +213,34 @@ func readUserFile(fn string, stdin io.ReadCloser) ([]byte, error) { defer r.Close() return ioutil.ReadAll(r) } + +func parseErrorResponse(r io.Reader, statusCode int) (io.Reader, string, error) { + bodyCopy := &bytes.Buffer{} + b, err := ioutil.ReadAll(io.TeeReader(r, bodyCopy)) + if err != nil { + return r, "", err + } + + var parsedBody struct { + Message string + Errors []struct { + Message string + } + } + err = json.Unmarshal(b, &parsedBody) + if err != nil { + return r, "", err + } + + if parsedBody.Message != "" { + return bodyCopy, fmt.Sprintf("%s (HTTP %d)", parsedBody.Message, statusCode), nil + } else if len(parsedBody.Errors) > 0 { + msgs := make([]string, len(parsedBody.Errors)) + for i, e := range parsedBody.Errors { + msgs[i] = e.Message + } + return bodyCopy, strings.Join(msgs, "\n"), nil + } + + return bodyCopy, "", nil +}