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

better handling of method not allowed #11

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

skaes
Copy link
Contributor

@skaes skaes commented Jun 19, 2020

No description provided.

Copy link
Contributor

@roccoblues roccoblues left a comment

Choose a reason for hiding this comment

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

I'm not sure if this is correct. If I setup a route like this:

gorilla.ActionName(r.HandleFunc("/image/{folder}/{file}", app.publicHandler).Methods("GET", "HEAD"), "Image#public")

Will the methodNotAllowed handlers added also have the action name Image#public?
What if I call request.ChangeAction in my handler? Then the methodNotAllowed handler will have a different action name for sure.

Definitely needs some test. ;-)

Also, since this is gorilla/mux specific we should still add something like #10 to support other routers.

gorilla/gorilla.go Show resolved Hide resolved
@skaes
Copy link
Contributor Author

skaes commented Jun 20, 2020

The idea of this code was to make the system behave as if the handler for a given route would accept all HTTP methods initially and then report 405 for the methods it does not really support. This works fine when template paths are unique, but yields confusing results when there are multiple routes with the same template path, but different handlers and action names. In that case, the current implementation will pick the name of the first route with the given template path.

Example:

gorilla.ActionName(router.Path("/users").Methods("GET").HandlerFunc(...), "Users#index")
gorilla.ActionName(router.Path("/users").Methods("POST").HandlerFunc(...), "Users#create")

This will lead to one additional route being installed that one could have written manually as

gorilla.ActionName(router.Path("/users").Methods("PUT", "DELETE", "HEAD", "OPTIONS", "PATCH").HandlerFunc(methodNotAllowed), "Users#index")

Maybe a better approach would be to do the equivalent of

gorilla.ActionName(router.Path("/users").Methods("PUT", "DELETE", "HEAD", "OPTIONS", "PATH").HandlerFunc(methodNotAllowed), "Users#methodNotAllowed")

The goal was to preserve some information on which part of the application was the intended receiver of the request and make this visible in the logjam UI. In this case: the part that deals with users.

However, I'm no longer sure the extra code complexity is worth pursuing this further, especially as the number of method not allowed requests for a given application is usually very low and one can easily debug the few requests under a single action name (like System#methodNotAllowed).

@skaes skaes force-pushed the add-method-not-allow-handlers branch from 58c983f to 02fffb1 Compare June 20, 2020 03:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants