Add a way to return the body of a 5xx response. (#484)

expose body when handling HTTP errors

Signed-off-by: Marcin Owsiany <[email protected]>

Author: porridge

api/prometheus/v1/api.go

@@ -60,6 +60,8 @@ const (
 	ErrCanceled              = "canceled"
 	ErrExec                  = "execution"
 	ErrBadResponse           = "bad_response"
+	ErrServer                = "server_error"
+	ErrClient                = "client_error"
 
 	// Possible values for HealthStatus.
 	HealthGood    HealthStatus = "up"
@@ -69,8 +71,9 @@ const (
 
 // Error is an error returned by the API.
 type Error struct {
-	Type ErrorType
-	Msg  string
+	Type   ErrorType
+	Msg    string
+	Detail string
 }
 
 func (e *Error) Error() string {
@@ -460,6 +463,16 @@ func apiError(code int) bool {
 	return code == statusAPIError || code == http.StatusBadRequest
 }
 
+func errorTypeAndMsgFor(resp *http.Response) (ErrorType, string) {
+	switch resp.StatusCode / 100 {
+	case 4:
+		return ErrClient, fmt.Sprintf("client error: %d", resp.StatusCode)
+	case 5:
+		return ErrServer, fmt.Sprintf("server error: %d", resp.StatusCode)
+	}
+	return ErrBadResponse, fmt.Sprintf("bad response code %d", resp.StatusCode)
+}
+
 func (c apiClient) Do(ctx context.Context, req *http.Request) (*http.Response, []byte, error) {
 	resp, body, err := c.Client.Do(ctx, req)
 	if err != nil {
@@ -469,9 +482,11 @@ func (c apiClient) Do(ctx context.Context, req *http.Request) (*http.Response, [
 	code := resp.StatusCode
 
 	if code/100 != 2 && !apiError(code) {
+		errorType, errorMsg := errorTypeAndMsgFor(resp)
 		return resp, body, &Error{
-			Type: ErrBadResponse,
-			Msg:  fmt.Sprintf("bad response code %d", resp.StatusCode),
+			Type:   errorType,
+			Msg:    errorMsg,
+			Detail: string(body),
 		}
 	}
 

api/prometheus/v1/api_test.go

@@ -18,6 +18,7 @@ package v1
 import (
 	"context"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"net/http"
 	"net/url"
@@ -30,9 +31,10 @@ import (
 )
 
 type apiTest struct {
-	do    func() (interface{}, error)
-	inErr error
-	inRes interface{}
+	do           func() (interface{}, error)
+	inErr        error
+	inStatusCode int
+	inRes        interface{}
 
 	reqPath   string
 	reqParam  url.Values
@@ -75,7 +77,9 @@ func (c *apiTestClient) Do(ctx context.Context, req *http.Request) (*http.Respon
 	}
 
 	resp := &http.Response{}
-	if test.inErr != nil {
+	if test.inStatusCode != 0 {
+		resp.StatusCode = test.inStatusCode
+	} else if test.inErr != nil {
 		resp.StatusCode = statusAPIError
 	} else {
 		resp.StatusCode = http.StatusOK
@@ -194,6 +198,42 @@ func TestAPIs(t *testing.T) {
 			},
 			err: fmt.Errorf("some error"),
 		},
+		{
+			do:           doQuery("2", testTime),
+			inRes:        "some body",
+			inStatusCode: 500,
+			inErr: &Error{
+				Type:   ErrServer,
+				Msg:    "server error: 500",
+				Detail: "some body",
+			},
+
+			reqMethod: "GET",
+			reqPath:   "/api/v1/query",
+			reqParam: url.Values{
+				"query": []string{"2"},
+				"time":  []string{testTime.Format(time.RFC3339Nano)},
+			},
+			err: errors.New("server_error: server error: 500"),
+		},
+		{
+			do:           doQuery("2", testTime),
+			inRes:        "some body",
+			inStatusCode: 404,
+			inErr: &Error{
+				Type:   ErrClient,
+				Msg:    "client error: 404",
+				Detail: "some body",
+			},
+
+			reqMethod: "GET",
+			reqPath:   "/api/v1/query",
+			reqParam: url.Values{
+				"query": []string{"2"},
+				"time":  []string{testTime.Format(time.RFC3339Nano)},
+			},
+			err: errors.New("client_error: client error: 404"),
+		},
 
 		{
 			do: doQueryRange("2", Range{
@@ -498,29 +538,34 @@ func TestAPIs(t *testing.T) {
 	var tests []apiTest
 	tests = append(tests, queryTests...)
 
-	for _, test := range tests {
-		client.curTest = test
-
-		res, err := test.do()
-
-		if test.err != nil {
-			if err == nil {
-				t.Errorf("expected error %q but got none", test.err)
-				continue
+	for i, test := range tests {
+		t.Run(fmt.Sprintf("%d", i), func(t *testing.T) {
+			client.curTest = test
+
+			res, err := test.do()
+
+			if test.err != nil {
+				if err == nil {
+					t.Fatalf("expected error %q but got none", test.err)
+				}
+				if err.Error() != test.err.Error() {
+					t.Errorf("unexpected error: want %s, got %s", test.err, err)
+				}
+				if apiErr, ok := err.(*Error); ok {
+					if apiErr.Detail != test.inRes {
+						t.Errorf("%q should be %q", apiErr.Detail, test.inRes)
+					}
+				}
+				return
 			}
-			if err.Error() != test.err.Error() {
-				t.Errorf("unexpected error: want %s, got %s", test.err, err)
+			if err != nil {
+				t.Fatalf("unexpected error: %s", err)
 			}
-			continue
-		}
-		if err != nil {
-			t.Errorf("unexpected error: %s", err)
-			continue
-		}
 
-		if !reflect.DeepEqual(res, test.res) {
-			t.Errorf("unexpected result: want %v, got %v", test.res, res)
-		}
+			if !reflect.DeepEqual(res, test.res) {
+				t.Errorf("unexpected result: want %v, got %v", test.res, res)
+			}
+		})
 	}
 }
 
@@ -532,10 +577,10 @@ type testClient struct {
 }
 
 type apiClientTest struct {
-	code     int
-	response interface{}
-	expected string
-	err      *Error
+	code         int
+	response     interface{}
+	expectedBody string
+	expectedErr  *Error
 }
 
 func (c *testClient) URL(ep string, args map[string]string) *url.URL {
@@ -575,98 +620,108 @@ func (c *testClient) Do(ctx context.Context, req *http.Request) (*http.Response,
 func TestAPIClientDo(t *testing.T) {
 	tests := []apiClientTest{
 		{
+			code: statusAPIError,
 			response: &apiResponse{
 				Status:    "error",
 				Data:      json.RawMessage(`null`),
 				ErrorType: ErrBadData,
 				Error:     "failed",
 			},
-			err: &Error{
+			expectedErr: &Error{
 				Type: ErrBadData,
 				Msg:  "failed",
 			},
-			code:     statusAPIError,
-			expected: `null`,
+			expectedBody: `null`,
 		},
 		{
+			code: statusAPIError,
 			response: &apiResponse{
 				Status:    "error",
 				Data:      json.RawMessage(`"test"`),
 				ErrorType: ErrTimeout,
 				Error:     "timed out",
 			},
-			err: &Error{
+			expectedErr: &Error{
 				Type: ErrTimeout,
 				Msg:  "timed out",
 			},
-			code:     statusAPIError,
-			expected: `test`,
+			expectedBody: `test`,
 		},
 		{
-			response: "bad json",
-			err: &Error{
-				Type: ErrBadResponse,
-				Msg:  "bad response code 500",
+			code:     http.StatusInternalServerError,
+			response: "500 error details",
+			expectedErr: &Error{
+				Type:   ErrServer,
+				Msg:    "server error: 500",
+				Detail: "500 error details",
+			},
+		},
+		{
+			code:     http.StatusNotFound,
+			response: "404 error details",
+			expectedErr: &Error{
+				Type:   ErrClient,
+				Msg:    "client error: 404",
+				Detail: "404 error details",
 			},
-			code: http.StatusInternalServerError,
 		},
 		{
+			code: http.StatusBadRequest,
 			response: &apiResponse{
 				Status:    "error",
 				Data:      json.RawMessage(`null`),
 				ErrorType: ErrBadData,
 				Error:     "end timestamp must not be before start time",
 			},
-			err: &Error{
+			expectedErr: &Error{
 				Type: ErrBadData,
 				Msg:  "end timestamp must not be before start time",
 			},
-			code: http.StatusBadRequest,
 		},
 		{
+			code:     statusAPIError,
 			response: "bad json",
-			err: &Error{
+			expectedErr: &Error{
 				Type: ErrBadResponse,
 				Msg:  "invalid character 'b' looking for beginning of value",
 			},
-			code: statusAPIError,
 		},
 		{
+			code: statusAPIError,
 			response: &apiResponse{
 				Status: "success",
 				Data:   json.RawMessage(`"test"`),
 			},
-			err: &Error{
+			expectedErr: &Error{
 				Type: ErrBadResponse,
 				Msg:  "inconsistent body for response code",
 			},
-			code: statusAPIError,
 		},
 		{
+			code: statusAPIError,
 			response: &apiResponse{
 				Status:    "success",
 				Data:      json.RawMessage(`"test"`),
 				ErrorType: ErrTimeout,
 				Error:     "timed out",
 			},
-			err: &Error{
+			expectedErr: &Error{
 				Type: ErrBadResponse,
 				Msg:  "inconsistent body for response code",
 			},
-			code: statusAPIError,
 		},
 		{
+			code: http.StatusOK,
 			response: &apiResponse{
 				Status:    "error",
 				Data:      json.RawMessage(`"test"`),
 				ErrorType: ErrTimeout,
 				Error:     "timed out",
 			},
-			err: &Error{
+			expectedErr: &Error{
 				Type: ErrBadResponse,
 				Msg:  "inconsistent body for response code",
 			},
-			code: http.StatusOK,
 		},
 	}
 
@@ -677,30 +732,37 @@ func TestAPIClientDo(t *testing.T) {
 	}
 	client := &apiClient{tc}
 
-	for _, test := range tests {
-
-		tc.ch <- test
-
-		_, body, err := client.Do(context.Background(), tc.req)
-
-		if test.err != nil {
-			if err == nil {
-				t.Errorf("expected error %q but got none", test.err)
-				continue
+	for i, test := range tests {
+		t.Run(fmt.Sprintf("%d", i), func(t *testing.T) {
+
+			tc.ch <- test
+
+			_, body, err := client.Do(context.Background(), tc.req)
+
+			if test.expectedErr != nil {
+				if err == nil {
+					t.Fatalf("expected error %q but got none", test.expectedErr)
+				}
+				if test.expectedErr.Error() != err.Error() {
+					t.Errorf("unexpected error: want %q, got %q", test.expectedErr, err)
+				}
+				if test.expectedErr.Detail != "" {
+					apiErr := err.(*Error)
+					if apiErr.Detail != test.expectedErr.Detail {
+						t.Errorf("unexpected error details: want %q, got %q", test.expectedErr.Detail, apiErr.Detail)
+					}
+				}
+				return
 			}
-			if test.err.Error() != err.Error() {
-				t.Errorf("unexpected error: want %q, got %q", test.err, err)
+			if err != nil {
+				t.Fatalf("unexpeceted error %s", err)
 			}
-			continue
-		}
-		if err != nil {
-			t.Errorf("unexpeceted error %s", err)
-			continue
-		}
 
-		want, got := test.expected, string(body)
-		if want != got {
-			t.Errorf("unexpected body: want %q, got %q", want, got)
-		}
+			want, got := test.expectedBody, string(body)
+			if want != got {
+				t.Errorf("unexpected body: want %q, got %q", want, got)
+			}
+		})
+
 	}
 }