Fix error parsing for valid JSON responses (#247)

errors.go

@@ -38,17 +38,30 @@ type ResponseError struct {
 	// Errors are the underlying error messages returned in the response body
 	Errors []string
 
+	// RawResponseBytes is only popualted when error messages can't be parsed
+	RawResponseBytes []byte
+
 	// OriginalRequest is a pointer to the request that caused this error
 	OriginalRequest *http.Request
 }
 
 func (e *ResponseError) Error() string {
-	return fmt.Sprintf("%d: %s", e.StatusCode, strings.Join(e.Errors, ", "))
+	switch {
+	case len(e.Errors) > 0:
+		return fmt.Sprintf("%d %s: %s", e.StatusCode, http.StatusText(e.StatusCode), strings.Join(e.Errors, ", "))
+
+	case len(e.RawResponseBytes) > 0:
+		return fmt.Sprintf("%d %s: %s", e.StatusCode, http.StatusText(e.StatusCode), e.RawResponseBytes)
+
+	default:
+		return fmt.Sprintf("%d %s", e.StatusCode, http.StatusText(e.StatusCode))
+	}
 }
 
-// isResponseError determines if this is a response error based on the response
-// status code. If it is determined to be an error, the function consumes the
-// response body without closing it and parses the underlying error messages.
+// isResponseError determines if this is an error response based on the
+// response status code. If it is determined to be an error, the function
+// consumes the response body without closing it and parses the underlying
+// error messages (or populates the RawResponseBody).
 func isResponseError(req *http.Request, resp *http.Response) *ResponseError {
 	// 200 to 399 are non-error status codes
 	if resp.StatusCode >= 200 && resp.StatusCode <= 399 {
@@ -69,8 +82,8 @@ func isResponseError(req *http.Request, resp *http.Response) *ResponseError {
 		OriginalRequest: req,
 	}
 
-	// read the entire response first so that we can return it as a raw error
-	// in case in cannot be parsed
+	// Read the entire response first so that we can return it as a raw
+	// response body in case it cannot be parsed.
 	responseBody, err := io.ReadAll(resp.Body)
 	if err != nil {
 		responseError.Errors = []string{
@@ -83,7 +96,7 @@ func isResponseError(req *http.Request, resp *http.Response) *ResponseError {
 	var jsonResponseErrors struct {
 		Errors []string `json:"errors"`
 	}
-	if err := json.Unmarshal(responseBody, &jsonResponseErrors); err == nil {
+	if err := json.Unmarshal(responseBody, &jsonResponseErrors); err == nil && len(jsonResponseErrors.Errors) > 0 {
 		responseError.Errors = jsonResponseErrors.Errors
 		return responseError // early return
 	}
@@ -92,17 +105,15 @@ func isResponseError(req *http.Request, resp *http.Response) *ResponseError {
 	var jsonResponseError struct {
 		Error string `json:"error"`
 	}
-	if err := json.Unmarshal(responseBody, &jsonResponseError); err == nil {
+	if err := json.Unmarshal(responseBody, &jsonResponseError); err == nil && len(jsonResponseError.Error) > 0 {
 		responseError.Errors = []string{
 			jsonResponseError.Error,
 		}
 		return responseError // early return
 	}
 
 	// else, return the raw response body
-	responseError.Errors = []string{
-		string(responseBody),
-	}
+	responseError.RawResponseBytes = responseBody
 
 	return responseError
 }

errors_test.go

@@ -0,0 +1,81 @@
+// Copyright (c) HashiCorp, Inc.
+// SPDX-License-Identifier: MPL-2.0
+
+package vault
+
+import (
+	"io"
+	"net/http"
+	"net/http/httptest"
+	"strings"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func Test_isResponseError(t *testing.T) {
+	cases := map[string]struct {
+		statusCode               int
+		body                     string
+		expectedError            bool
+		expectedErrors           []string
+		expectedRawResponseBytes []byte
+	}{
+		"non-error": {
+			statusCode:    http.StatusOK,
+			body:          "",
+			expectedError: false,
+		},
+		"response-with-errors": {
+			statusCode:     http.StatusInternalServerError,
+			body:           `{"errors":["error1", "error2"]}`,
+			expectedError:  true,
+			expectedErrors: []string{"error1", "error2"},
+		},
+		"response-with-error": {
+			statusCode:     http.StatusGone,
+			body:           `{"error":"single error"}`,
+			expectedError:  true,
+			expectedErrors: []string{"single error"},
+		},
+		"json-response-without-errors": {
+			statusCode:               http.StatusNotFound,
+			body:                     `{"data":{"key1":"value1","key2":"value2"}}`,
+			expectedError:            true,
+			expectedErrors:           nil,
+			expectedRawResponseBytes: []byte(`{"data":{"key1":"value1","key2":"value2"}}`),
+		},
+		"non-json-response": {
+			statusCode:               http.StatusTeapot,
+			body:                     `this is just a string`,
+			expectedError:            true,
+			expectedErrors:           nil,
+			expectedRawResponseBytes: []byte(`this is just a string`),
+		},
+	}
+
+	for name, tc := range cases {
+		t.Run(name, func(t *testing.T) {
+			err := isResponseError(
+				httptest.NewRequest(http.MethodGet, "http://localhost:8200/v1/foo", nil),
+				&http.Response{
+					StatusCode: tc.statusCode,
+					Body:       io.NopCloser(strings.NewReader(tc.body)),
+				},
+			)
+
+			if !tc.expectedError {
+				require.Nil(t, err)
+				return
+			}
+
+			var responseError *ResponseError
+			require.ErrorAs(t, err, &responseError)
+
+			assert.Equal(t, tc.statusCode, responseError.StatusCode)
+			assert.Equal(t, tc.expectedErrors, responseError.Errors)
+			assert.Equal(t, tc.expectedRawResponseBytes, responseError.RawResponseBytes)
+		})
+	}
+}