Skip to content

Add context to ErrorHandler #11

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

Jump to bottom
Closed
pebo opened this issue Feb 9, 2024 · 7 comments · Fixed by #35
Closed

Add context to ErrorHandler #11

pebo opened this issue Feb 9, 2024 · 7 comments · Fixed by #35

Comments

@pebo
Copy link
Contributor

pebo commented Feb 9, 2024

In order to correlate custom ErrorHandler logging with requests if would be very helpful if the ErrorHandler signature was expanded to include context.Context e.g.

type ErrorHandler func(ctx context.Context, w http.ResponseWriter, message string, statusCode int)

This however is a breaking change so another option could be to expand Options with a ErrorHandlerWithContext field.
This was referenced Feb 13, 2024
Closed
Closed
@pebo
Copy link
Contributor Author

pebo commented Feb 14, 2024

Added two example PRs for this.

Ran into the missing context when migration to chi from gin which has a context in the validation middleware:

type ErrorHandler func(c *gin.Context, message string, statusCode int)

@mikeschinkel
Copy link
Contributor

There is definitely a need for more info in the error handler.

Context is one such need, but the current matched Route, the Request, and the Request's Headers — like Accept — are others.

I created a PR (#15) that (I think) addresses @pebo's use-case in this issue but also addresses others, and in a manner that will allow adding other info if such info is found to be needed by others in the future. Hopefully PR #15 can supersede PRs #13 and #14 since they change error handling but don't address the use-cases that #15 address.

@MattiasMartens
Copy link
Contributor

Would love to see this merged! Seems it's waiting on a review from the maintainer.

@tmzane
Copy link

tmzane commented Dec 17, 2024

+1 for this feature.

I agree with @mikeschinkel that it should be not just the context.Context but the whole http.Request. It'd be also really useful to have access to the original error instead of just message string, to be able to match it with errors.Is/As. My use case is to ignore ErrPathNotFound/ErrMethodNotAllowed returned by the FindRounte method. Currently, we have to match the string, which is not perfect.

So I propose the new signature to be:

type ErrorHandler func(r *http.Request, w http.ResponseWriter, err error, statusCode int)

What do you guys think?

@mikeschinkel
Copy link
Contributor

@tmzane — Definitely agree that having the original error could be useful. But changing the ErrorHandler signature would break existing code so I am not a fan of that.

Have you looked at PR #15? I could see it added to ErrorHandlerOpts if #15 is accepted to resolve this PR, #14, my issue (#15) and your issue with error.

@MattiasMartens MattiasMartens mentioned this issue Dec 31, 2024
Closed
@jamietanna
Copy link
Member

Thanks all for the patience and suggestions - I'm looking at getting this tackled via #35 - please take a look at the signature and let me know thoughts

(This will be merged in the next couple of days, and anyone who's contributed suggestions will get a Co-Authored-By in this change, so even though it won't show as a "merged PR" from you, it'll definitely show as you having contributed to the work!)

@pebo
Copy link
Contributor Author

pebo commented Apr 24, 2025

Thanks all for the patience and suggestions - I'm looking at getting this tackled via #35 - please take a look at the signature and let me know thoughts

The signature looks great! This would allow us to remove some workaround middleware.

jamietanna added a commit that referenced this issue Apr 24, 2025
@jamietanna @pebo
@mikeschinkel @MattiasMartens
feat: add error handler with more configuration
d39f6e6 
As a means to **??**o

This has been a long-standing issue **??**

With thanks to Per, Mike and MattiasMartens who have **??**, as well as
many others in the past who have **??**

Closes #11, #27.

Co-authored-by: Per Bockman <[email protected]>
Co-authored-by: Mike Schinkel <[email protected]>
Co-authored-by: MattiasMartens <[email protected]>
jamietanna added a commit that referenced this issue Apr 24, 2025
@jamietanna @pebo
@mikeschinkel @MattiasMartens
rew! feat: add error handler with more configuration
1907cbd 
When handling errros **??**, historically we have only been given the
error message and a suggested HTTP status code to return, which provides
**??**

To improve this experience, and provide much more control over **??**,
we will introduce the `ErrorHandlerWithOpts` function and its
corresponding `ErrorHandlerOpts` struct to handle the **??**

This will provide:

- Direct access to the `error` that occurred **??**
-

As a means to **??**o

This has been a long-standing issue **??**

With thanks to Per, Mike and MattiasMartens who have **??**, as well as
many others in the past who have **??**

Closes #11, #27.

Co-authored-by: Per Bockman <[email protected]>
Co-authored-by: Mike Schinkel <[email protected]>
Co-authored-by: MattiasMartens <[email protected]>
jamietanna added a commit that referenced this issue Apr 24, 2025
@jamietanna @pebo
@mikeschinkel @MattiasMartens
feat: add error handler with more configuration
9dd933f 
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 jamietanna mentioned this issue Apr 24, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
Middleware with better error handling
Status: Done
Milestone
No milestone
5 participants
@mikeschinkel @pebo @jamietanna @MattiasMartens @tmzane