Skip to content

Add ErrorHandlerWithOpts #15

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.

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

Closed
wants to merge 3 commits into from
Closed

Conversation

mikeschinkel
Copy link
Contributor

@mikeschinkel mikeschinkel commented Apr 25, 2024

This PR adds an alternate custom error handler with an additional opts parameter that allows a custom error handler access to the incoming *http.Request, then matched routes.Route, and the relevant context.Context.

My use-case for why the Route is needed is to be able to determine the appropriate Content-Type for an error response; should it be application/json, text/html or something else?

I added Request as I originally thought I would need it, but I didn't. Still, I can easily see that others might so it doesn't seem a harm to provide it.

I added Context to explicitly fulfill the use-case for #13 & #14, and hopefully to supercede them.

Lastly, I chose an opts parameter passing in ErrorHandlerOpts so that future needs could be accommodated easily by just adding a property without needing to add yet another signature and yet another type of error func.

P.S. I also had to modify private function validateRequest() to get it to return the matched routers.Route — as well as that that called it — given that access to the Route was the motivation for this PR.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
type ErrorHandlerOpts struct {
*http.Request
*routers.Route
context.Context
Copy link
Contributor

Choose a reason for hiding this comment

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

Adding a field here error to capture the original error would be helpful (cf: #11 (comment))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MattiasMartens — Thank you for that suggested change. Assuming I understood your suggestion then I think it is an excellent idea. Actually, disappointed in myself for not thinking of it initially.

I made that change to the PR. Please review the PR to make sure I correctly understood what you were suggesting.

if statusCode, route, err := validateRequest(r, router, options); err != nil {
if options != nil {
if options.ErrorHandlerWithOpts != nil {
options.ErrorHandlerWithOpts(w, err.Error(), statusCode, ErrorHandlerOpts{
Copy link
Contributor

Choose a reason for hiding this comment

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

If change to ErrorHandlerOpts is implemented then we could add here:

Suggested change
options.ErrorHandlerWithOpts(w, err.Error(), statusCode, ErrorHandlerOpts{
options.ErrorHandlerWithOpts(w, err.Error(), statusCode, ErrorHandlerOpts{
Error: err,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made that change to the PR. Please review the PR to make sure I correctly understood what you were suggesting.

Copy link
Contributor

@MattiasMartens MattiasMartens left a comment

Choose a reason for hiding this comment

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

LGTM

@gk-peytz
Copy link

Thanks guys. Also looking for such functionality.

@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!)

@jamietanna
Copy link
Member

Replaced by #35

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants