From cd6482ecb8c420a1179541a1e28c2db4d6dda744 Mon Sep 17 00:00:00 2001 From: Aaron Lehmann Date: Mon, 14 Mar 2016 10:06:30 -0700 Subject: [PATCH 1/2] Don't return empty errcode.Errors slices If this slice ends up empty after parsing the HTTP response body, it means the body is not well-formed. We've probably encountered an error message produced by something that uses a different JSON schema, or an error that just happens to validate as JSON. An empty errcode.Errors slice is not a very useful thing to return, since its Error() output is just ``. Detect this case, and instend return an UnexpectedHTTPResponseError. Signed-off-by: Aaron Lehmann --- registry/client/errors.go | 19 +++++++++++++++++-- registry/client/errors_test.go | 19 +++++++++++++++++-- 2 files changed, 34 insertions(+), 4 deletions(-) diff --git a/registry/client/errors.go b/registry/client/errors.go index a528a8657..043782bfb 100644 --- a/registry/client/errors.go +++ b/registry/client/errors.go @@ -2,6 +2,7 @@ package client import ( "encoding/json" + "errors" "fmt" "io" "io/ioutil" @@ -10,6 +11,10 @@ import ( "github.com/docker/distribution/registry/api/errcode" ) +// ErrNoErrorsInBody is returned when a HTTP response body parses to an empty +// errcode.Errors slice. +var ErrNoErrorsInBody = errors.New("no error details found in HTTP response body") + // UnexpectedHTTPStatusError is returned when an unexpected HTTP status is // returned when making a registry api call. type UnexpectedHTTPStatusError struct { @@ -17,7 +22,7 @@ type UnexpectedHTTPStatusError struct { } func (e *UnexpectedHTTPStatusError) Error() string { - return fmt.Sprintf("Received unexpected HTTP status: %s", e.Status) + return fmt.Sprintf("received unexpected HTTP status: %s", e.Status) } // UnexpectedHTTPResponseError is returned when an expected HTTP status code @@ -28,7 +33,7 @@ type UnexpectedHTTPResponseError struct { } func (e *UnexpectedHTTPResponseError) Error() string { - return fmt.Sprintf("Error parsing HTTP response: %s: %q", e.ParseErr.Error(), string(e.Response)) + return fmt.Sprintf("error parsing HTTP response: %s: %q", e.ParseErr.Error(), string(e.Response)) } func parseHTTPErrorResponse(statusCode int, r io.Reader) error { @@ -57,6 +62,16 @@ func parseHTTPErrorResponse(statusCode int, r io.Reader) error { Response: body, } } + + if len(errors) == 0 { + // If there was no error specified in the body, return + // UnexpectedHTTPResponseError. + return &UnexpectedHTTPResponseError{ + ParseErr: ErrNoErrorsInBody, + Response: body, + } + } + return errors } diff --git a/registry/client/errors_test.go b/registry/client/errors_test.go index 80241a5a4..1d60cd2da 100644 --- a/registry/client/errors_test.go +++ b/registry/client/errors_test.go @@ -59,6 +59,21 @@ func TestHandleErrorResponseExpectedStatusCode400ValidBody(t *testing.T) { } } +func TestHandleErrorResponseExpectedStatusCode404EmptyErrorSlice(t *testing.T) { + json := `{"randomkey": "randomvalue"}` + response := &http.Response{ + Status: "404 Not Found", + StatusCode: 404, + Body: nopCloser{bytes.NewBufferString(json)}, + } + err := HandleErrorResponse(response) + + expectedMsg := `error parsing HTTP response: no error details found in HTTP response body: "{\"randomkey\": \"randomvalue\"}"` + if !strings.Contains(err.Error(), expectedMsg) { + t.Errorf("Expected \"%s\", got: \"%s\"", expectedMsg, err.Error()) + } +} + func TestHandleErrorResponseExpectedStatusCode404InvalidBody(t *testing.T) { json := "{invalid json}" response := &http.Response{ @@ -68,7 +83,7 @@ func TestHandleErrorResponseExpectedStatusCode404InvalidBody(t *testing.T) { } err := HandleErrorResponse(response) - expectedMsg := "Error parsing HTTP response: invalid character 'i' looking for beginning of object key string: \"{invalid json}\"" + expectedMsg := "error parsing HTTP response: invalid character 'i' looking for beginning of object key string: \"{invalid json}\"" if !strings.Contains(err.Error(), expectedMsg) { t.Errorf("Expected \"%s\", got: \"%s\"", expectedMsg, err.Error()) } @@ -82,7 +97,7 @@ func TestHandleErrorResponseUnexpectedStatusCode501(t *testing.T) { } err := HandleErrorResponse(response) - expectedMsg := "Received unexpected HTTP status: 501 Not Implemented" + expectedMsg := "received unexpected HTTP status: 501 Not Implemented" if !strings.Contains(err.Error(), expectedMsg) { t.Errorf("Expected \"%s\", got: \"%s\"", expectedMsg, err.Error()) } From 3a2231fe392c7a1212eee3bb89c9cb9a255057bc Mon Sep 17 00:00:00 2001 From: Aaron Lehmann Date: Tue, 15 Mar 2016 09:03:56 -0700 Subject: [PATCH 2/2] Include status code in UnexpectedHTTPResponseError Signed-off-by: Aaron Lehmann --- registry/client/errors.go | 17 ++++++++++------- registry/client/errors_test.go | 4 ++-- 2 files changed, 12 insertions(+), 9 deletions(-) diff --git a/registry/client/errors.go b/registry/client/errors.go index 043782bfb..00fafe117 100644 --- a/registry/client/errors.go +++ b/registry/client/errors.go @@ -28,12 +28,13 @@ func (e *UnexpectedHTTPStatusError) Error() string { // UnexpectedHTTPResponseError is returned when an expected HTTP status code // is returned, but the content was unexpected and failed to be parsed. type UnexpectedHTTPResponseError struct { - ParseErr error - Response []byte + ParseErr error + StatusCode int + Response []byte } func (e *UnexpectedHTTPResponseError) Error() string { - return fmt.Sprintf("error parsing HTTP response: %s: %q", e.ParseErr.Error(), string(e.Response)) + return fmt.Sprintf("error parsing HTTP %d response body: %s: %q", e.StatusCode, e.ParseErr.Error(), string(e.Response)) } func parseHTTPErrorResponse(statusCode int, r io.Reader) error { @@ -58,8 +59,9 @@ func parseHTTPErrorResponse(statusCode int, r io.Reader) error { if err := json.Unmarshal(body, &errors); err != nil { return &UnexpectedHTTPResponseError{ - ParseErr: err, - Response: body, + ParseErr: err, + StatusCode: statusCode, + Response: body, } } @@ -67,8 +69,9 @@ func parseHTTPErrorResponse(statusCode int, r io.Reader) error { // If there was no error specified in the body, return // UnexpectedHTTPResponseError. return &UnexpectedHTTPResponseError{ - ParseErr: ErrNoErrorsInBody, - Response: body, + ParseErr: ErrNoErrorsInBody, + StatusCode: statusCode, + Response: body, } } diff --git a/registry/client/errors_test.go b/registry/client/errors_test.go index 1d60cd2da..ca9dddd10 100644 --- a/registry/client/errors_test.go +++ b/registry/client/errors_test.go @@ -68,7 +68,7 @@ func TestHandleErrorResponseExpectedStatusCode404EmptyErrorSlice(t *testing.T) { } err := HandleErrorResponse(response) - expectedMsg := `error parsing HTTP response: no error details found in HTTP response body: "{\"randomkey\": \"randomvalue\"}"` + expectedMsg := `error parsing HTTP 404 response body: no error details found in HTTP response body: "{\"randomkey\": \"randomvalue\"}"` if !strings.Contains(err.Error(), expectedMsg) { t.Errorf("Expected \"%s\", got: \"%s\"", expectedMsg, err.Error()) } @@ -83,7 +83,7 @@ func TestHandleErrorResponseExpectedStatusCode404InvalidBody(t *testing.T) { } err := HandleErrorResponse(response) - expectedMsg := "error parsing HTTP response: invalid character 'i' looking for beginning of object key string: \"{invalid json}\"" + expectedMsg := "error parsing HTTP 404 response body: invalid character 'i' looking for beginning of object key string: \"{invalid json}\"" if !strings.Contains(err.Error(), expectedMsg) { t.Errorf("Expected \"%s\", got: \"%s\"", expectedMsg, err.Error()) }