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

API to return matched router path #139

Closed
wants to merge 1 commit into from

Conversation

jehiah
Copy link

@jehiah jehiah commented Apr 21, 2016

I would like to propose an API that returns the exact router path that's been matched. There are multiple situations where having the router path available would be beneficial like logging, rate limiting, etc because it will have less cardinality than the full URL with dynamic parameters.

Given the following example

r = httprouter.New()
r.GET("/user/:name", func(w http.ResponseWriter, r *http.Request, _ httprouter.Params) {
    fmt.Println("no way to get '/user/:name' here")
})

I'd like to store the state of the full route being matched in each node of the match tree and add a router.LookupRoute() (to avoid a backwards incompatible change to Lookup()) so that you can re-parse the request path and find which route matched. I'm open to other naming suggestions, or if you'd rather make a breaking change to Lookup and add the return value there.

r = httprouter.New()
r.GET("/user/:name", func(w http.ResponseWriter, r *http.Request, _ httprouter.Params) {
    _, matchedRoute, _, _ := r.LookupRoute(r.Method, r.URL.Path)
    fmt.Printf("matched %q for %q", matchedRoute, r.URL.Path)
})

@jehiah jehiah force-pushed the return_matched_path branch from d31a9b1 to c4d72b3 Compare April 21, 2016 14:57
@jharlap
Copy link

jharlap commented Apr 21, 2016

My $0.02: Love the idea, and have a totally hack logger in place that this would make a million times better, but maybe it could just be a new method on Params that just returns a string?

@jehiah
Copy link
Author

jehiah commented Apr 21, 2016

@jharlap because Params is an array I don't see how we can add this context there in a backwards compatible fashion.

tree_test.go Outdated
t.Errorf("handle mismatch for route %q: Wrong handle (%q != %q)", request.path, matchPath, request.route)
}
if matchPath != fakeHandlerValue {
t.Errorf("handler missmatc for route %q: (%q != %q)", request.path, matchPath, fakeHandlerValue)
Copy link

Choose a reason for hiding this comment

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

missmatc > mismatch

Copy link
Author

Choose a reason for hiding this comment

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

you know me. always trying to sneak in some typos.

@jehiah jehiah force-pushed the return_matched_path branch from c4d72b3 to f17f139 Compare April 21, 2016 20:29
@kpurdon
Copy link

kpurdon commented May 6, 2016

@julienschmidt hoping to get some feedback on this! Thanks!

@apriendeau
Copy link

@julienschmidt This is really a much needed feature. Can you please provide some sort of feedback here?

@laplaceon
Copy link

I have a fork with this feature here: https://github.com/laplaceon/httprouter. You can see it in action here: https://github.com/laplaceon/polyhedron.

@alexanderheldt
Copy link

@julienschmidt Could you please take a look at this again? Having this would be really great when measuring endpoints with prometheus. 🙏

@GeorgeMac
Copy link
Contributor

I only saw this PR after I started to open mine, but I took a slightly different stab at this as well. Mine doesn't effect the Lookup method signature and exposes the route to your handlers through the request context.
If anyone is interested: #286
My usecase is enriching my trace data with router path matches in middleware.

@jehiah
Copy link
Author

jehiah commented Jan 6, 2020

@julienschmidt do you have any feedback on this or #286?

I'd be happy to take any design or implementation feedback and update this PR. This has been very useful for me since i've opened this PR a few years ago and i'd love to contribute this upstream.

@julienschmidt
Copy link
Owner

I gave some feedback over here: #286 (comment)

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.

8 participants