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

Components of URL pattern are rewritten(overridden) by subpattern if different names are used #71

Open
gorjan5sk opened this issue Jul 2, 2014 · 4 comments

Comments

@gorjan5sk
Copy link

Adding handlers for subpatterns overrides the parent pattern if the components are named differently. Ex:

apiMux = tigertonic.NewTrieServeMux()
apiMux.Handle("GET", "/templates/{name}", tigertonic.Marshaled(handlers.ReadTemplate))
apiMux.Handle("GET", "/templates/{n1}/{n2}", tigertonic.Marshaled(handlers.ReadSubTemplate))

The first handler can never be executed in this scenario. This is a useful feature in my humble opinion, however, it needs to be documented.

The proper way to use it would be:

apiMux = tigertonic.NewTrieServeMux()
apiMux.Handle("GET", "/templates/{n1}", tigertonic.Marshaled(handlers.ReadTemplate))
apiMux.Handle("GET", "/templates/{n1}/{n2}", tigertonic.Marshaled(handlers.ReadSubTemplate))

Just my 2 cents.

@randallsquared
Copy link
Contributor

If you have a componentized architecture, the requirement that all (perhaps reusable) components know what variable names other components used seems like a problem, to me. So, I think this is more of a bug than a feature.

@gorjan5sk
Copy link
Author

@randallsquared Would it be a security problem or do you think it's prone to abuse?

The reason why I think it's a feature is because it enables us to write common Handlers which do validation, authorization, custom logging etc in special cases not covered by the current API.
Ex (Authorization of resources):

apiMux.Handle("GET", "/user/{name}/photos/{photo_id}", authorize( getPhoto))
apiMux.Handle("GET", "/user/{name}/videos/{video_id}", authorize( getVideo))

The authorize handler must access the same URL component in this case, and the framework is forcing us to think in the context of reusable components.

@randallsquared
Copy link
Contributor

To be clear, I think it's a bug that using a different name invalidates the first route, if and only if the first parameter name is different (I'm assuming you're correct about this behavior; I haven't tried it).

If you have more than one route with a prefix including a parameter, there are two ways (I think) to handle it: use HandleNamespace, or specify the route more than once. If you can't use HandleNamespace, then it seems very likely that the two handlers don't know anything about each other. If two handlers are allowed to specify a parameter placeholder in the same location without namespacing, the name of the parameter shouldn't be required to match: that name is an implementation detail of the handler.

So, in my opinion, either tigertonic should overwrite handlers using the same route portion as a parameter every time, or never, but it shouldn't depend on what those handlers need as the parameter name.

@willfaught
Copy link
Contributor

It would be helpful for people not familiar with this nuance if it just panicked when there are conflicting variables. (I just spent a few hours figuring this out. Yay.)

Assuming this behaves as intended, then there is a related bug: Tigertonic prints that it handles the first route, when in fact it doesn't, which caused me a lot of confusion and frustration:

trie_serve_mux.go:35: handling POST /promotions
trie_serve_mux.go:35: handling GET /promotions/{id}
trie_serve_mux.go:35: handling POST /promotions/eligible
trie_serve_mux.go:35: handling POST /promotions/{code}/eligible
trie_serve_mux.go:35: handling POST /promotions/{code}/sign
server.go:127: Listening on: 127.0.0.1:8270
16:10:50.195954 U3E4QhNpSKIAhhBb > GET /promotions/foo HTTP/1.1
16:10:50.196673 U3E4QhNpSKIAhhBb > Accept: */*
16:10:50.196684 U3E4QhNpSKIAhhBb > User-Agent: curl/7.37.1
16:10:50.196689 U3E4QhNpSKIAhhBb >
16:10:50.196743 U3E4QhNpSKIAhhBb < HTTP/1.1 405 Method Not Allowed
16:10:50.196756 U3E4QhNpSKIAhhBb < Allow: OPTIONS
16:10:50.196761 U3E4QhNpSKIAhhBb < Content-Type: application/json
16:10:50.196765 U3E4QhNpSKIAhhBb <
16:10:50.196811 U3E4QhNpSKIAhhBb < {"description":"only OPTIONS are allowed","error":"method_not_allowed"}

Note the conflict in handling GET /promotions/{id}, GET /promotions/foo HTTP/1.1, and then HTTP/1.1 405 Method Not Allowed.

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

No branches or pull requests

3 participants