feat: add error handler with more configuration #35
Conversation
As it simplifies the readability of the checks.
jamietanna
force-pushed
the
feat/err-ctx
branch
from
075c5ac to
8519112
Compare
jamietanna
force-pushed
the
feat/err-ctx
branch
2 times, most recently
from
d39f6e6 to
1907cbd
Compare
Historically, handling errors with the OpenAPI request validation middleware has been fairly frustrating, as we don't provide information around the `error` that occurred (only the message), the `context.Context` that this request is for, what HTTP method + path it's for, or anything else about the request. As a means to much more greatly improve the experience for our users, we can introduce a new `ErrorHandlerWithOpts` function which provides all this information, and the ability to extend this in the future. This is introduced in a backwards-compatible way, with a new function, to avoid a breaking change, and to allow consumers to migrate over to the new function. We'll mark the existing function as deprecated, to indicate folks should migrate over, but also note there is no hard requirement or deadline associated. This introduces a new helper method `performRequestValidationForErrorHandler` that will be used alongside the new `performRequestValidationForErrorHandlerWithOpts` for the new function, as a means to deduplicate processing the error itself. We want to make sure that our testable examples add validation for this functionality, as well as indicating how a `MultiError` could be handled (with added complexity). With thanks to Per, Mike and MattiasMartens who have made tangible efforts towards this in this repository, as well as many others in the past who have worked on suggestions and improvements towards this. Closes #11, #27. Co-authored-by: Per Bockman <[email protected]> Co-authored-by: Mike Schinkel <[email protected]> Co-authored-by: MattiasMartens <[email protected]>
jamietanna
force-pushed
the
feat/err-ctx
branch
from
1907cbd to
9dd933f
Compare
|
@jamietanna — Thanks for tackling this. One thing I notice is your |
| // If both an `ErrorHandlerWithOpts` and `ErrorHandler` are set, the `ErrorHandlerWithOpts` takes precedence. | ||
| // | ||
| // NOTE that this should ideally be used instead of ErrorHandler | ||
| type ErrorHandlerWithOpts func(ctx context.Context, w http.ResponseWriter, r *http.Request, opts ErrorHandlerOpts) |
There was a problem hiding this comment.
There's a context.Context here - I thought it'd make more sense as a function parameter (and the first) than in the opts - thoughts?
There was a problem hiding this comment.
@jamietanna — As it has been a while since I was working on the project whose use-case caused me to write a PR the specifics faded from memory, and I commented because I was vaguely thinking there would two different contexts. However, in reviewing your reply and your code I seems I was just not thinking it through clearly enough and now see that your PR is probably the better and more idiomatic approach.
So, all good.
There was a problem hiding this comment.
Ah, no worries! No, I very much appreciate your review - I think if used with i.e. Gin, you may want the gin.Context and the context.Context, but in this case for pure net/http, we won't have that exposed
There was a problem hiding this comment.
@jamietanna — Hey, though. Super excited to see you address this as I am sure I will need it in the future.
github-project-automation
bot
moved this to Done
in Middleware with better error handling
Historically, handling errors with the OpenAPI request validation
middleware has been fairly frustrating, as we don't provide information
around the
errorthat occurred (only the message), thecontext.Contextthat this request is for, what HTTP method + path it'sfor, or anything else about the request.
As a means to much more greatly improve the experience for our users, we
can introduce a new
ErrorHandlerWithOptsfunction which provides all thisinformation, and the ability to extend this in the future.
This is introduced in a backwards-compatible way, with a new function,
to avoid a breaking change, and to allow consumers to migrate over to
the new function.
We'll mark the existing function as deprecated, to indicate folks should
migrate over, but also note there is no hard requirement or deadline
associated.
This introduces a new helper method
performRequestValidationForErrorHandlerthat will be used alongsidethe new
performRequestValidationForErrorHandlerWithOptsfor the newfunction, as a means to deduplicate processing the error itself.
We want to make sure that our testable examples add validation for this
functionality, as well as indicating how a
MultiErrorcould be handled(with added complexity).
With thanks to Per, Mike and MattiasMartens who have made tangible
efforts towards this in this repository, as well as many others in the
past who have worked on suggestions and improvements towards this.
Closes #11, #27.