Skip to content

Commit

Permalink
Improve error response handling
Browse files Browse the repository at this point in the history
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:
#367 (comment)
  • Loading branch information
awilliams-fastly committed Nov 15, 2022
1 parent d3473c7 commit 15fe634
Show file tree
Hide file tree
Showing 2 changed files with 65 additions and 12 deletions.
39 changes: 27 additions & 12 deletions fastly/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"encoding/json"
"errors"
"fmt"
"io"
"net/http"
"strconv"

Expand Down Expand Up @@ -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":
Expand All @@ -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,
Expand Down
38 changes: 38 additions & 0 deletions fastly/errors_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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")
}
}
})
}

0 comments on commit 15fe634

Please sign in to comment.