diff --git a/internal/requests/requests.go b/internal/requests/requests.go index 133f598fe..f094b3336 100644 --- a/internal/requests/requests.go +++ b/internal/requests/requests.go @@ -388,10 +388,7 @@ func (c *Client) Do(p *RequestParams) (*HTTPResponse, error) { err := json.Unmarshal([]byte(body), &respBody) if err != nil { c.Logger.Debug("response body from %s (status %d):\n%s", req.URL, resp.StatusCode, string(body)) - // Still carry the status code so callers can distinguish e.g. a 404 - // with an empty or non-JSON body (a proxy/CDN page). Keep the - // (JSON parse) error text as the message. - return nil, &APIError{StatusCode: resp.StatusCode, Message: err.Error()} + return nil, &APIError{StatusCode: resp.StatusCode, Message: nonJSONErrorMessage(resp.StatusCode, body, c.Debug)} } cleanedErrorMessage := "" hasServerMessage := false @@ -550,6 +547,20 @@ func isSuccess(status int) bool { return false } +// nonJSONErrorMessage reports an error response whose body is not JSON, such as +// a proxy's HTML page, by its status: the decoder's complaint about the body +// tells the user nothing. +func nonJSONErrorMessage(status int, body []byte, debug bool) string { + switch { + case len(bytes.TrimSpace(body)) == 0: + return httpStatus(status) + case debug: + return httpStatus(status) + ": the response is not JSON" + default: + return httpStatus(status) + ": the response is not JSON, rerun with --debug to see it" + } +} + // giveUpErrorHandler keeps the last response's status, which retryablehttp's // default handler drops. The *url.Error that http.Client wraps it in names the // method and URL. diff --git a/internal/requests/requests_test.go b/internal/requests/requests_test.go index 158e169c1..dd2bae34b 100644 --- a/internal/requests/requests_test.go +++ b/internal/requests/requests_test.go @@ -79,6 +79,10 @@ func (suite *RequestsTestSuite) SetupSuite() { Get("/fail/"). Reply(500). BodyString("server broken") + suite.fakeService.NewHandler(). + Get("/blank/"). + Reply(401). + BodyString("\r\n") suite.fakeService.NewHandler(). Get("/proxy-block/"). Reply(403). @@ -323,7 +327,7 @@ func (suite *RequestsTestSuite) TestDo() { URL: suite.fakeService.ResolveURL("/no-content/"), }, wantError: true, - expectedErrorMsg: "unexpected end of JSON input", + expectedErrorMsg: "HTTP 204 No Content", }, { name: "206 is not a success: a partial body is not the answer", @@ -332,7 +336,7 @@ func (suite *RequestsTestSuite) TestDo() { URL: suite.fakeService.ResolveURL("/partial/"), }, wantError: true, - expectedErrorMsg: "unexpected end of JSON input", + expectedErrorMsg: "HTTP 206 Partial Content: the response is not JSON, rerun with --debug to see it", }, { name: "GET request to 404 endpoint", @@ -430,13 +434,22 @@ func (suite *RequestsTestSuite) TestDo() { expectedErrorMsg: "Denied", }, { - name: "GET request to a PUT endpoint fails because of invalid response", + name: "GET to an unrouted path reports the status of the empty 404", params: &RequestParams{ Method: http.MethodGet, URL: suite.fakeService.ResolveURL("/html"), }, wantError: true, - expectedErrorMsg: "unexpected end of JSON input", + expectedErrorMsg: "HTTP 404 Not Found", + }, + { + name: "a whitespace-only body reports the status alone", + params: &RequestParams{ + Method: http.MethodGet, + URL: suite.fakeService.ResolveURL("/blank/"), + }, + wantError: true, + expectedErrorMsg: "HTTP 401 Unauthorized", }, { name: "non-JSON error response body is logged with debug enabled", @@ -446,7 +459,7 @@ func (suite *RequestsTestSuite) TestDo() { }, debug: true, wantError: true, - expectedErrorMsg: "invalid character '<' looking for beginning of value", + expectedErrorMsg: "HTTP 403 Forbidden: the response is not JSON", expectedLog: `

403 Forbidden

Blocked by proxy

`, }, } {