From f30f428035633da15d00d3dfefb0128c5e569ef4 Mon Sep 17 00:00:00 2001 From: Marcin Owsiany Date: Thu, 25 Oct 2018 19:44:21 +0200 Subject: [PATCH] Add a way to return the body of a 5xx response. (#484) expose body when handling HTTP errors Signed-off-by: Marcin Owsiany --- api/prometheus/v1/api.go | 23 +++- api/prometheus/v1/api_test.go | 194 ++++++++++++++++++++++------------ 2 files changed, 147 insertions(+), 70 deletions(-) diff --git a/api/prometheus/v1/api.go b/api/prometheus/v1/api.go index 6a19fac..255f3ba 100644 --- a/api/prometheus/v1/api.go +++ b/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), } } diff --git a/api/prometheus/v1/api_test.go b/api/prometheus/v1/api_test.go index 1f7fda7..8492a5c 100644 --- a/api/prometheus/v1/api_test.go +++ b/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 + for i, test := range tests { + t.Run(fmt.Sprintf("%d", i), func(t *testing.T) { + client.curTest = test - res, err := test.do() + res, err := test.do() - if test.err != nil { - if err == nil { - t.Errorf("expected error %q but got none", test.err) - continue + 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.StatusInternalServerError, }, { + code: http.StatusNotFound, + response: "404 error details", + expectedErr: &Error{ + Type: ErrClient, + Msg: "client error: 404", + Detail: "404 error details", + }, + }, + { + 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 { + for i, test := range tests { + t.Run(fmt.Sprintf("%d", i), func(t *testing.T) { - tc.ch <- test + tc.ch <- test - _, body, err := client.Do(context.Background(), tc.req) + _, 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 + 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) + } + }) + } }