From 15fe6349f22423d288700d6c221c00c4218810e2 Mon Sep 17 00:00:00 2001 From: Adam Williams <83312471+awilliams-fastly@users.noreply.github.com> Date: Tue, 15 Nov 2022 13:05:19 -0700 Subject: [PATCH 1/2] Improve error response handling Instead of panic'ing when an error response cannot be decoded from JSON, return a generic error which includes the read portion of the response body. Followup to @dgryski's comment: https://github.com/fastly/go-fastly/pull/367#discussion_r980318242 --- fastly/errors.go | 39 +++++++++++++++++++++++++++------------ fastly/errors_test.go | 38 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 65 insertions(+), 12 deletions(-) diff --git a/fastly/errors.go b/fastly/errors.go index 792fe855b..cd2be6570 100644 --- a/fastly/errors.go +++ b/fastly/errors.go @@ -5,6 +5,7 @@ import ( "encoding/json" "errors" "fmt" + "io" "net/http" "strconv" @@ -327,11 +328,23 @@ func NewHTTPError(resp *http.Response) *HTTPError { return &e } + // Save a copy of the body as it's read/decoded. + // If decoding fails, it can then be used (via addDecodeErr) + // to create a generic error containing the body's read contents. + var bodyCp bytes.Buffer + body := io.TeeReader(resp.Body, &bodyCp) + addDecodeErr := func() { + e.Errors = append(e.Errors, &ErrorObject{ + Title: "Undefined error", + Detail: bodyCp.String(), + }) + } + switch resp.Header.Get("Content-Type") { case jsonapi.MediaType: - // If this is a jsonapi response, decode it accordingly - if err := decodeBodyMap(resp.Body, &e); err != nil { - panic(err) + // If this is a jsonapi response, decode it accordingly. + if err := decodeBodyMap(body, &e); err != nil { + addDecodeErr() } case "application/problem+json": @@ -342,19 +355,21 @@ func NewHTTPError(resp *http.Response) *HTTPError { Title string `json:"title,omitempty"` // A short name for the error type, which remains constant from occurrence to occurrence URL string `json:"type,omitempty"` // URL to a human-readable document describing this specific error condition } - if err := json.NewDecoder(resp.Body).Decode(&problemDetail); err != nil { - panic(err) + if err := json.NewDecoder(body).Decode(&problemDetail); err != nil { // Ignore json decode errors. + addDecodeErr() + } else { + e.Errors = append(e.Errors, &ErrorObject{ + Title: problemDetail.Title, + Detail: problemDetail.Detail, + Status: strconv.Itoa(problemDetail.Status), + }) } - e.Errors = append(e.Errors, &ErrorObject{ - Title: problemDetail.Title, - Detail: problemDetail.Detail, - Status: strconv.Itoa(problemDetail.Status), - }) default: var lerr *legacyError - _ = decodeBodyMap(resp.Body, &lerr) - if lerr != nil { + if err := decodeBodyMap(body, &lerr); err != nil { + addDecodeErr() + } else if lerr != nil { e.Errors = append(e.Errors, &ErrorObject{ Title: lerr.Message, Detail: lerr.Detail, diff --git a/fastly/errors_test.go b/fastly/errors_test.go index e60b13446..617d88cf0 100644 --- a/fastly/errors_test.go +++ b/fastly/errors_test.go @@ -106,4 +106,42 @@ func TestNewHTTPError(t *testing.T) { t.Error("not not found") } }) + + t.Run("invalid JSON", func(t *testing.T) { + contentTypes := []string{ + jsonapi.MediaType, + "application/problem+json", + "default", + } + + for _, ct := range contentTypes { + resp := &http.Response{ + StatusCode: 404, + Header: http.Header(map[string][]string{"Content-Type": {ct}}), + Body: io.NopCloser(bytes.NewBufferString(`THIS IS NOT JSON`)), + } + e := NewHTTPError(resp) + + if e.StatusCode != 404 { + t.Errorf("expected %d to be %d", e.StatusCode, 404) + } + + expected := strings.TrimSpace(` +404 - Not Found: + + Title: Undefined error + Detail: THIS IS NOT JSON +`) + if e.Error() != expected { + t.Errorf("Content-Type: %q: expected \n\n%q\n\n to be \n\n%q\n\n", ct, e.Error(), expected) + } + if e.String() != expected { + t.Errorf("Content-Type: %q: expected \n\n%q\n\n to be \n\n%q\n\n", ct, e.String(), expected) + } + + if !e.IsNotFound() { + t.Error("not not found") + } + } + }) } From f57e64580bbef4f81107c4ba62a454f33697d1dc Mon Sep 17 00:00:00 2001 From: Adam Williams <83312471+awilliams-fastly@users.noreply.github.com> Date: Wed, 16 Nov 2022 10:52:29 -0700 Subject: [PATCH 2/2] Additional comments Provide context for why the response's body is included in the error message and the decode error is not. --- fastly/errors.go | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/fastly/errors.go b/fastly/errors.go index cd2be6570..5c89173fb 100644 --- a/fastly/errors.go +++ b/fastly/errors.go @@ -334,6 +334,13 @@ func NewHTTPError(resp *http.Response) *HTTPError { var bodyCp bytes.Buffer body := io.TeeReader(resp.Body, &bodyCp) addDecodeErr := func() { + // There are 2 errors at this point: + // 1. The response error. + // 2. The error decoding the response. + // The response error is still most relevant to users (just unable to be decoded). + // Provide the response's body verbatim as the error 'Detail' with the assumption + // that it may contain useful information, e.g. 'Bad Gateway'. + // The decode error could be conflated with the response error, so it is omitted. e.Errors = append(e.Errors, &ErrorObject{ Title: "Undefined error", Detail: bodyCp.String(), @@ -355,7 +362,7 @@ func NewHTTPError(resp *http.Response) *HTTPError { Title string `json:"title,omitempty"` // A short name for the error type, which remains constant from occurrence to occurrence URL string `json:"type,omitempty"` // URL to a human-readable document describing this specific error condition } - if err := json.NewDecoder(body).Decode(&problemDetail); err != nil { // Ignore json decode errors. + if err := json.NewDecoder(body).Decode(&problemDetail); err != nil { addDecodeErr() } else { e.Errors = append(e.Errors, &ErrorObject{