Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[query] Allow injecting function for rewriting error response #3008

Merged
merged 12 commits into from
Dec 22, 2020
Merged

[query] Allow injecting function for rewriting error response #3008

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
9a51ac1
set error rewrite function
vpranckaitis Dec 14, 2020
648ec1d
error rewriting in Prom's RespondError
vpranckaitis Dec 14, 2020
867aea8
Merge branch 'master' into vilius/error_rewrite
vpranckaitis Dec 18, 2020
6fb565d
stop serializing custom response body in RespondError()
vpranckaitis Dec 18, 2020
c5a937a
add a test
vpranckaitis Dec 18, 2020
9f900ae
add dots to comments
vpranckaitis Dec 18, 2020
184fb8b
Merge branch 'master' into vilius/error_rewrite
vpranckaitis Dec 21, 2020
a050331
remove RespondError()
vpranckaitis Dec 21, 2020
bb301d7
rename package-local variables
vpranckaitis Dec 21, 2020
f05f3d4
use reader locks when applying rewrite function
vpranckaitis Dec 21, 2020
2631711
update test
vpranckaitis Dec 22, 2020
4bcc525
Merge branch 'master' into vilius/error_rewrite
vpranckaitis Dec 22, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/query/api/v1/handler/prom/common.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Collaborator

@robskillington robskillington Dec 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, can't we just rewrite in xhttp.WriteError which the err is passed into on line 108 here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alas, xhttp.WithErrorResponse(b) passes already serialized error message, which is then directly written to HTTP response. Hence the rewrite inside WriteError() would only affect status code, the message would stay as in original error.

I was considering changing xhttp.WithErrorResponse() to accept a func(error) []byte instead of already prepared []byte. What do you think about that?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After #2933, both RespondError() and WriteError() emits status field. Thus, the latter can be used directly

6fb565d#diff-dcc137769b5c2ad03409d297cf05413b71396fc7df31a9a2ee6ecb9a9b20e22dL95-R95


json := jsoniter.ConfigCompatibleWithStandardLibrary
b, marshalErr := json.Marshal(&response{
Status: statusError,
Expand Down
26 changes: 26 additions & 0 deletions src/x/net/http/errors.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We tend not to use underscores for package local variables. Just use a non-exported (lowercase) letter to begin with.

e.g.

errorRewriteFn
errorRewriteFnLock

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bb301d7

)

// Error is an HTTP JSON error that also sets a return status code.
type Error interface {
// Fulfill error interface.
Expand Down Expand Up @@ -92,6 +101,8 @@ func WriteError(w http.ResponseWriter, err error, opts ...WriteErrorOption) {
fn(&o)
}

err = _errorRewriteFn(err)
Copy link
Collaborator

@robskillington robskillington Dec 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to hold the read lock to get the error function without causing a race.

e.g.

_errorRewriteFnLock.RLock()
err = _errorRewriteFn(err)
_errorRewriteFnLock.RUnlock()

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

f05f3d4


statusCode := getStatusCode(err)
if o.response == nil {
w.Header().Set(HeaderContentType, ContentTypeJSON)
Expand All @@ -103,6 +114,21 @@ 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()
defer _errorRewriteFnLock.Unlock()

res := _errorRewriteFn
_errorRewriteFn = f
return res
}

func getStatusCode(err error) int {
switch v := err.(type) {
case Error:
Expand Down