From 9a51ac19c750f007a65ef17e341d5efca834c953 Mon Sep 17 00:00:00 2001 From: Vilius Pranckaitis Date: Mon, 14 Dec 2020 13:31:01 +0200 Subject: [PATCH 1/9] set error rewrite function --- src/x/net/http/errors.go | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/src/x/net/http/errors.go b/src/x/net/http/errors.go index 331e23e4d3..4224e23ef1 100644 --- a/src/x/net/http/errors.go +++ b/src/x/net/http/errors.go @@ -25,10 +25,19 @@ import ( "encoding/json" "errors" "net/http" + "sync" xerrors "github.com/m3db/m3/src/x/errors" ) +// ErrorRewriteFn is a function for rewriting response error +type ErrorRewriteFn func(error) error + +var ( + _errorRewriteFn ErrorRewriteFn = func(err error) error { return err } + _errorRewriteFnLock sync.Mutex +) + // Error is an HTTP JSON error that also sets a return status code. type Error interface { // Fulfill error interface. @@ -92,6 +101,8 @@ func WriteError(w http.ResponseWriter, err error, opts ...WriteErrorOption) { fn(&o) } + err = _errorRewriteFn(err) + statusCode := getStatusCode(err) if o.response == nil { w.Header().Set(HeaderContentType, ContentTypeJSON) @@ -103,6 +114,16 @@ func WriteError(w http.ResponseWriter, err error, opts ...WriteErrorOption) { } } +// SetErrorRewriteFn sets error rewrite function +func SetErrorRewriteFn(f ErrorRewriteFn) ErrorRewriteFn { + _errorRewriteFnLock.Lock() + defer _errorRewriteFnLock.Unlock() + + res := _errorRewriteFn + _errorRewriteFn = f + return res +} + func getStatusCode(err error) int { switch v := err.(type) { case Error: From 648ec1d70cc303257cf4dc5987c68c8274fbaa6c Mon Sep 17 00:00:00 2001 From: Vilius Pranckaitis Date: Mon, 14 Dec 2020 13:44:29 +0200 Subject: [PATCH 2/9] error rewriting in Prom's RespondError --- src/query/api/v1/handler/prom/common.go | 2 ++ src/x/net/http/errors.go | 5 +++++ 2 files changed, 7 insertions(+) diff --git a/src/query/api/v1/handler/prom/common.go b/src/query/api/v1/handler/prom/common.go index 7c4a66a0d6..55f93a8f5f 100644 --- a/src/query/api/v1/handler/prom/common.go +++ b/src/query/api/v1/handler/prom/common.go @@ -92,6 +92,8 @@ func Respond(w http.ResponseWriter, data interface{}, warnings promstorage.Warni // Responds with error status code and writes error JSON to response body. func RespondError(w http.ResponseWriter, err error) { + err = xhttp.GetErrorRewriteFn()(err) + json := jsoniter.ConfigCompatibleWithStandardLibrary b, marshalErr := json.Marshal(&response{ Status: statusError, diff --git a/src/x/net/http/errors.go b/src/x/net/http/errors.go index 4224e23ef1..07731da61c 100644 --- a/src/x/net/http/errors.go +++ b/src/x/net/http/errors.go @@ -114,6 +114,11 @@ func WriteError(w http.ResponseWriter, err error, opts ...WriteErrorOption) { } } +// GetErrorRewriteFn returns the error rewrite function +func GetErrorRewriteFn() ErrorRewriteFn { + return _errorRewriteFn +} + // SetErrorRewriteFn sets error rewrite function func SetErrorRewriteFn(f ErrorRewriteFn) ErrorRewriteFn { _errorRewriteFnLock.Lock() From 6fb565d9c777d9475f21de8401b34f4da5b2a34f Mon Sep 17 00:00:00 2001 From: Vilius Pranckaitis Date: Fri, 18 Dec 2020 17:41:36 +0200 Subject: [PATCH 3/9] stop serializing custom response body in RespondError() --- src/query/api/v1/handler/prom/common.go | 15 +-------------- src/x/net/http/errors.go | 5 ----- 2 files changed, 1 insertion(+), 19 deletions(-) diff --git a/src/query/api/v1/handler/prom/common.go b/src/query/api/v1/handler/prom/common.go index 55f93a8f5f..3cc3ea74ee 100644 --- a/src/query/api/v1/handler/prom/common.go +++ b/src/query/api/v1/handler/prom/common.go @@ -92,18 +92,5 @@ func Respond(w http.ResponseWriter, data interface{}, warnings promstorage.Warni // Responds with error status code and writes error JSON to response body. func RespondError(w http.ResponseWriter, err error) { - err = xhttp.GetErrorRewriteFn()(err) - - json := jsoniter.ConfigCompatibleWithStandardLibrary - b, marshalErr := json.Marshal(&response{ - Status: statusError, - Error: err.Error(), - }) - if marshalErr != nil { - xhttp.WriteError(w, marshalErr) - return - } - - w.Header().Set(xhttp.HeaderContentType, xhttp.ContentTypeJSON) - xhttp.WriteError(w, err, xhttp.WithErrorResponse(b)) + xhttp.WriteError(w, err) } diff --git a/src/x/net/http/errors.go b/src/x/net/http/errors.go index 3aa51a7bbd..5329093133 100644 --- a/src/x/net/http/errors.go +++ b/src/x/net/http/errors.go @@ -115,11 +115,6 @@ func WriteError(w http.ResponseWriter, err error, opts ...WriteErrorOption) { } } -// GetErrorRewriteFn returns the error rewrite function -func GetErrorRewriteFn() ErrorRewriteFn { - return _errorRewriteFn -} - // SetErrorRewriteFn sets error rewrite function func SetErrorRewriteFn(f ErrorRewriteFn) ErrorRewriteFn { _errorRewriteFnLock.Lock() From c5a937a2efde53d5f05ad7f27253af58d3b57df3 Mon Sep 17 00:00:00 2001 From: Vilius Pranckaitis Date: Fri, 18 Dec 2020 17:58:00 +0200 Subject: [PATCH 4/9] add a test --- src/x/net/http/errors_test.go | 41 +++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/src/x/net/http/errors_test.go b/src/x/net/http/errors_test.go index 4bd5ecf46d..6d3414e363 100644 --- a/src/x/net/http/errors_test.go +++ b/src/x/net/http/errors_test.go @@ -21,14 +21,55 @@ package xhttp import ( + "errors" "fmt" + "net/http/httptest" "testing" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" xerrors "github.com/m3db/m3/src/x/errors" ) +func TestErrorRewrite(t *testing.T) { + tests := []struct { + name string + err error + expectedStatus int + expectedBody string + }{ + { + name: "non rewritten error", + err: errors.New("random error"), + expectedStatus: 500, + expectedBody: `{"status":"error","error":"random error"}`, + }, + { + name: "rewritten error", + err: xerrors.NewInvalidParamsError(errors.New("to be rewritten")), + expectedStatus: 500, + expectedBody: `{"status":"error","error":"rewritten error"}`, + }, + } + + SetErrorRewriteFn(func(err error) error { + if xerrors.IsInvalidParams(err) { + return errors.New("rewritten error") + } + return err + }) + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + recorder := httptest.NewRecorder() + WriteError(recorder, tt.err) + assert.Equal(t, tt.expectedStatus, recorder.Code) + assert.JSONEq(t, tt.expectedBody, recorder.Body.String()) + }) + } +} + func TestIsClientError(t *testing.T) { tests := []struct { err error From 9f900ae38b46277ffb4188d707e12e41137e789b Mon Sep 17 00:00:00 2001 From: Vilius Pranckaitis Date: Fri, 18 Dec 2020 18:03:16 +0200 Subject: [PATCH 5/9] add dots to comments --- src/x/net/http/errors.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/x/net/http/errors.go b/src/x/net/http/errors.go index 5329093133..6d3afc0dc2 100644 --- a/src/x/net/http/errors.go +++ b/src/x/net/http/errors.go @@ -30,7 +30,7 @@ import ( xerrors "github.com/m3db/m3/src/x/errors" ) -// ErrorRewriteFn is a function for rewriting response error +// ErrorRewriteFn is a function for rewriting response error. type ErrorRewriteFn func(error) error var ( @@ -115,7 +115,7 @@ func WriteError(w http.ResponseWriter, err error, opts ...WriteErrorOption) { } } -// SetErrorRewriteFn sets error rewrite function +// SetErrorRewriteFn sets error rewrite function. func SetErrorRewriteFn(f ErrorRewriteFn) ErrorRewriteFn { _errorRewriteFnLock.Lock() defer _errorRewriteFnLock.Unlock() @@ -139,7 +139,7 @@ func getStatusCode(err error) int { return http.StatusInternalServerError } -// IsClientError returns true if this error would result in 4xx status code +// IsClientError returns true if this error would result in 4xx status code. func IsClientError(err error) bool { code := getStatusCode(err) return code >= 400 && code < 500 From a0503313f67b4ea81e01714a2940eeeba8f61e4b Mon Sep 17 00:00:00 2001 From: Vilius Pranckaitis Date: Mon, 21 Dec 2020 17:00:43 +0200 Subject: [PATCH 6/9] remove RespondError() --- src/query/api/v1/handler/prom/common.go | 5 ----- src/query/api/v1/handler/prom/read.go | 9 +++++---- 2 files changed, 5 insertions(+), 9 deletions(-) diff --git a/src/query/api/v1/handler/prom/common.go b/src/query/api/v1/handler/prom/common.go index 3cc3ea74ee..7ef1d039a1 100644 --- a/src/query/api/v1/handler/prom/common.go +++ b/src/query/api/v1/handler/prom/common.go @@ -89,8 +89,3 @@ func Respond(w http.ResponseWriter, data interface{}, warnings promstorage.Warni w.WriteHeader(http.StatusOK) w.Write(b) } - -// Responds with error status code and writes error JSON to response body. -func RespondError(w http.ResponseWriter, err error) { - xhttp.WriteError(w, err) -} diff --git a/src/query/api/v1/handler/prom/read.go b/src/query/api/v1/handler/prom/read.go index b69cdecc39..7b89fcbf67 100644 --- a/src/query/api/v1/handler/prom/read.go +++ b/src/query/api/v1/handler/prom/read.go @@ -34,6 +34,7 @@ import ( "github.com/m3db/m3/src/query/models" "github.com/m3db/m3/src/query/storage/prometheus" xerrors "github.com/m3db/m3/src/x/errors" + xhttp "github.com/m3db/m3/src/x/net/http" "github.com/prometheus/prometheus/promql" promstorage "github.com/prometheus/prometheus/storage" @@ -99,13 +100,13 @@ func (h *readHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { fetchOptions, err := h.hOpts.FetchOptionsBuilder().NewFetchOptions(r) if err != nil { - RespondError(w, err) + xhttp.WriteError(w, err) return } request, err := native.ParseRequest(ctx, r, h.opts.instant, h.hOpts) if err != nil { - RespondError(w, err) + xhttp.WriteError(w, err) return } @@ -129,7 +130,7 @@ func (h *readHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { h.logger.Error("error creating query", zap.Error(err), zap.String("query", params.Query), zap.Bool("instant", h.opts.instant)) - RespondError(w, xerrors.NewInvalidParamsError(err)) + xhttp.WriteError(w, xerrors.NewInvalidParamsError(err)) return } defer qry.Close() @@ -139,7 +140,7 @@ func (h *readHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { h.logger.Error("error executing query", zap.Error(res.Err), zap.String("query", params.Query), zap.Bool("instant", h.opts.instant)) - RespondError(w, res.Err) + xhttp.WriteError(w, res.Err) return } From bb301d72c514a5fe80bb912a8aa06d963836d230 Mon Sep 17 00:00:00 2001 From: Vilius Pranckaitis Date: Mon, 21 Dec 2020 17:01:45 +0200 Subject: [PATCH 7/9] rename package-local variables --- src/x/net/http/errors.go | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/x/net/http/errors.go b/src/x/net/http/errors.go index 6d3afc0dc2..5fe6580250 100644 --- a/src/x/net/http/errors.go +++ b/src/x/net/http/errors.go @@ -34,8 +34,8 @@ import ( type ErrorRewriteFn func(error) error var ( - _errorRewriteFn ErrorRewriteFn = func(err error) error { return err } - _errorRewriteFnLock sync.Mutex + errorRewriteFn ErrorRewriteFn = func(err error) error { return err } + errorRewriteFnLock sync.Mutex ) // Error is an HTTP JSON error that also sets a return status code. @@ -102,7 +102,7 @@ func WriteError(w http.ResponseWriter, err error, opts ...WriteErrorOption) { fn(&o) } - err = _errorRewriteFn(err) + err = errorRewriteFn(err) statusCode := getStatusCode(err) if o.response == nil { @@ -117,11 +117,11 @@ func WriteError(w http.ResponseWriter, err error, opts ...WriteErrorOption) { // SetErrorRewriteFn sets error rewrite function. func SetErrorRewriteFn(f ErrorRewriteFn) ErrorRewriteFn { - _errorRewriteFnLock.Lock() - defer _errorRewriteFnLock.Unlock() + errorRewriteFnLock.Lock() + defer errorRewriteFnLock.Unlock() - res := _errorRewriteFn - _errorRewriteFn = f + res := errorRewriteFn + errorRewriteFn = f return res } From f05f3d4cd8556f522d72258b3331e3c727c98a35 Mon Sep 17 00:00:00 2001 From: Vilius Pranckaitis Date: Mon, 21 Dec 2020 17:07:50 +0200 Subject: [PATCH 8/9] use reader locks when applying rewrite function --- src/x/net/http/errors.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/x/net/http/errors.go b/src/x/net/http/errors.go index 5fe6580250..5e10e54e5f 100644 --- a/src/x/net/http/errors.go +++ b/src/x/net/http/errors.go @@ -35,7 +35,7 @@ type ErrorRewriteFn func(error) error var ( errorRewriteFn ErrorRewriteFn = func(err error) error { return err } - errorRewriteFnLock sync.Mutex + errorRewriteFnLock sync.RWMutex ) // Error is an HTTP JSON error that also sets a return status code. @@ -102,7 +102,9 @@ func WriteError(w http.ResponseWriter, err error, opts ...WriteErrorOption) { fn(&o) } + errorRewriteFnLock.RLock() err = errorRewriteFn(err) + errorRewriteFnLock.RUnlock() statusCode := getStatusCode(err) if o.response == nil { From 26317117c0b228472caa3ada6d9d4b61318a25bd Mon Sep 17 00:00:00 2001 From: Vilius Pranckaitis Date: Tue, 22 Dec 2020 09:32:00 +0200 Subject: [PATCH 9/9] update test --- src/x/net/http/errors_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/x/net/http/errors_test.go b/src/x/net/http/errors_test.go index 6d3414e363..f5c0e6fabe 100644 --- a/src/x/net/http/errors_test.go +++ b/src/x/net/http/errors_test.go @@ -40,13 +40,13 @@ func TestErrorRewrite(t *testing.T) { expectedBody string }{ { - name: "non rewritten error", + name: "error that should not be rewritten", err: errors.New("random error"), expectedStatus: 500, expectedBody: `{"status":"error","error":"random error"}`, }, { - name: "rewritten error", + name: "error that should be rewritten", err: xerrors.NewInvalidParamsError(errors.New("to be rewritten")), expectedStatus: 500, expectedBody: `{"status":"error","error":"rewritten error"}`,