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.

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

override 404 and 405 handlers #19

Open
dvic opened this issue Jul 1, 2017 · 8 comments
Open

override 404 and 405 handlers #19

dvic opened this issue Jul 1, 2017 · 8 comments
Assignees
Labels

Comments

@dvic
Copy link

dvic commented Jul 1, 2017

Is it possible to override the 404 and 405 handlers of a route/group? Is it something you would like to have? I could submit a PR, making a NewFromOpts where you can pass the following:

  • 404 handlersChain HandlersChain
  • 405 handlersChain HandlersChain
  • redirectTrailingSlash bool
  • handleMethodNotAllowed bool
  • automaticallyHandleOPTIONS bool

By the way: just wanted to say that I really like this router, AWESOME work! Lately I have been looking extensively into the 'router ecosystem' of Go 😉 and this is exactly what I was looking for! 👍

@deankarn
Copy link

deankarn commented Jul 1, 2017

Hey @dvic glad you like the router, there are too many out there re-inventing the wheel, but this one is the only one that you can use a customer Context object that I'm aware of; but I dirges.

I'm just curious , I can't see a reason to have differing values per group for redirectTrailingSlash

What's the use case for having this differ per route group? So I understand the use case.

P.S. I would make them options on the RouteGroup object themselves rather than a new constructor.

@deankarn deankarn self-assigned this Jul 1, 2017
@dvic
Copy link
Author

dvic commented Jul 1, 2017

Ah yes it makes sense to have SetRedirectTrailingSlash only available on LARS (which is already the case). How about adding the following methods to the IRouteGroup interface (which LARS should continue to implement):

  • SetHttp404HandlersChain (currently also missing on LARS)
  • SetHttp405HandlersChain (currently also missing on LARS)
  • SetHandle405MethodNotAllowed
  • SetAutomaticallyHandleOPTIONS

@deankarn
Copy link

deankarn commented Jul 1, 2017

I'm just trying to understand the use case for doing so I always try and understand the roadblock before proceeding as sometimes there are better ways...

Why would you need a separate 404 or 405 handler per group?

@dvic
Copy link
Author

dvic commented Jul 1, 2017

Sure, no problem. Let's say I have /assets/* (css, images, etc.), /uploads/* (files uploaded by users) and /api/*. For the /assets/* group I don't care so much, because requests to this endpoint are done by browsers. However, for /uploads/* I would like to show a nicely formatted error pages (probably specific to the current web app) as these requests will be made by users directly in the browser. For /api/* the same applies, there I would like to show a JSON/XML/etc. 404 response. Is this something I can achieve with the current API? Thanks :)

@deankarn
Copy link

deankarn commented Jul 3, 2017

I see @dvic how that could be useful, I'll have to think about it for a bit on how to do it. RouteGroups aren't really used passed the registration of handlers.... don't yet know how I would link the 404 to a particular group the way the router works.

I mean there is a workaround with the current implementation that may be just as good though, why not add a middleware to each of the route groups you wish to handle differently and if a 404 status code is present when request is returning do your custom page rendering or json 404.

Sorry for the late replies, you just caught me in the middle of moving...likely be busy for the next couple weeks at least.

@dvic
Copy link
Author

dvic commented Jul 3, 2017

Ah just to make sure I understand: I would use a 404 handler that only sets status code to 404 and then use (as last) a middleware on each group to render the actual response?

@deankarn
Copy link

deankarn commented Jul 3, 2017

Yep that should work

@dvic
Copy link
Author

dvic commented Jul 3, 2017

Cool, thanks!

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

No branches or pull requests

2 participants