Standardize API error return values#183
Conversation
Codecov Report
|
5db043f to
fb1cafa
Compare
gerardsn
changed the title
|
Increase max-width of read the docs 800 -> 1000px |
reinkrul
left a comment
reinkrul
left a comment
There was a problem hiding this comment.
I like the setup. Some small things.
| func httpErrorHandler(err error, ctx echo.Context) { | ||
| if prb, ok := err.(*problem.Problem); ok { | ||
| // todo: connect to module logger | ||
| if !ctx.Response().Committed { |
There was a problem hiding this comment.
what if the response already is committed?
There was a problem hiding this comment.
there is nothing we can do if the response is already committed (besides logging). The current approach replicates echo's DefaultHTTPErrorHandler()
| // todo: connect to module logger | ||
| if !ctx.Response().Committed { | ||
| if _, err := prb.WriteTo(ctx.Response()); err != nil { | ||
| //ctx.Echo().Logger.Error(err) |
There was a problem hiding this comment.
Yes, if the write fails you want to see it in the logs.
There was a problem hiding this comment.
Failing to write a response is unlikely, so we can log this in the core logger
| if err != nil { | ||
| log.Logger().Error(err.Error()) | ||
| return echo.NewHTTPError(http.StatusBadRequest, err.Error()) | ||
| return createProblem("SignJWT failed", http.StatusBadRequest, err.Error()) |
stevenvegt
left a comment
stevenvegt
left a comment
There was a problem hiding this comment.
Nice first PR. Some minor points to fix 👍
| return "" | ||
| } | ||
|
|
||
| func creatorFn() EchoServer { |
There was a problem hiding this comment.
changed to createEchoServer
|
|
||
| // ErrorToProblem returns a Problem generated from a problem.Problem | ||
| // problem.Problem doesn't expose its fields | ||
| func ErrorToProblem(err error) Problem { |
There was a problem hiding this comment.
I don't like these poluting the global namespace. The problem can perhaps be made private?
Or a few helper methods:
TestAssertProblemErrStatus(err error, status int) boolTestAssertProblemErrTitle(err error, title string) boolTestAssertProblemErrMessage(err error, message string) bool
There was a problem hiding this comment.
These have been moved to nuts-node/test. Since the conversion from from error to problem.Probelm now takes place within the test method, I've added a check to make sure the error actually is a problem before verifying the field's specified value.
| // todo: connect to module logger | ||
| if !ctx.Response().Committed { | ||
| if _, err := prb.WriteTo(ctx.Response()); err != nil { | ||
| //ctx.Echo().Logger.Error(err) |
There was a problem hiding this comment.
Yes, if the write fails you want to see it in the logs.
| resp, err := client.Do(req) | ||
|
|
||
| assert.NoError(t, err) | ||
| assert.Equal(t, http.StatusInternalServerError, resp.StatusCode) |
There was a problem hiding this comment.
Test some more. We expect a json struct on the correct content type right? There should be a title and description in here too?
There was a problem hiding this comment.
added tests to check content type, and response body.
c993a5f to
08e1ab1
Compare
stevenvegt
left a comment
stevenvegt
left a comment
There was a problem hiding this comment.
Nice improvements. 2 minor things / questions.
| assert.Equal(t, problem.ContentTypeJSON, resp.Header.Get("Content-Type")) | ||
|
|
||
| // Validate response body with expected problem | ||
| prb := NewProblem("problem title", http.StatusInternalServerError, "problem detail") |
There was a problem hiding this comment.
you already created this problem on line 120 right?
| } | ||
|
|
||
| // AssertErrProblemTitle asserts err is a *problem.Problem with the specified title | ||
| func AssertErrProblemTitle(t assert.TestingT, title string, err error) bool { |
There was a problem hiding this comment.
Why the use of asssert.TestingT instead of golang's testing.T?
addressed as requested
4 participants
Adds general error handeling middleware.
Proposes standard for 400+ return values. Currently only for crypto, the rest is tracked in #112.
Closes #3 #9