Add Find to Routes interface

[joeriddles]

Background

I am working on adding a middleware for OpenTelemetry to a Chi-based service that needs to send a metric before the request has executed. We need to include a string for the API endpoint, like the route pattern, in the metric. This is currently not possible due to the way request route patterns are resolved using Chi's trie-based router, as noted by the maintainer in this issue #692 (comment).

Description

This PR proposes adding a Find function to the Routes interface. This Find method closely mirrors the Match function that already exists. Unlike Match, which returns a boolean value if a route exists for the path/HTTP method combination, Find returns the pattern that was matched.

Unit tests have been added for Find, as well as new tests added for Match. I refactored Match to use Find under the hood, since they are both doing almost the same thing.

This PR also adds a new middleware, FindPattern, which allows the user to pass a callback function that will receive the fully resolved route pattern from the HTTP request.

Examples

A basic example of using Find in a middleware before the request is handled can be found here: https://github.com/joeriddles/chi-api-middleware, specifically this function https://github.com/joeriddles/chi-api-middleware/blob/main/main.go#L37.

A smaller example has also been added to the _examples folder.

[Pipello]

That's cool feature 💯

[pkieltyka]

hi @joeriddles what will be the official middleware repo for your package?

[joeriddles]

Thanks for the review @pkieltyka! I have renamed the file and variables you requested in this commit: 5cf9bc3

hi @joeriddles what will be the official middleware repo for your package?

Do you mean I should add a new middleware file, like middlewares/find.go? I can add something like this that allows the user to pass a function that should be called with the found route pattern.

[joeriddles]

@pkieltyka I have added a new middleware here: f6c1df8. I also updated the example I added to use the new FindPattern middleware. Thanks again for reviewing!

[casoetan]

👀 🙌🏾

[batazor]

any news?

[joeriddles]

@pkieltyka any chance this gets merged soon?

[dils2k]

I am going to fork the repo and merge this PR in the fork 👀

[VojtechVitek]

Hi,

The .Find() method on Mux is indeed a very nice contribution to chi. LGTM. Thank you!

---

But for now, I removed the middleware.FindRoute() in 1daabdc, as I think it needs some more thoughts / discussion.

r.Use(middleware.FindPattern(r, func(pattern string) {
     fmt.Printf("pattern=%s\n", pattern)
}))

I think it'd make sense to rename this to middleware.RoutePattern() but also make the *http.Request and http.ResponseWriter available to the callback, so users could check r.Method/r.Host/... and for example respond with HTTP 403 if they were to implement ACL inside of this callback.

If you have good ideas how to finish this new middleware API, please submit an issue with a proposal first.

---

Thank you for your contribution!

[dils2k]

@VojtechVitek Can you make a new release please?

[joeriddles]

Thanks for merging @VojtechVitek!

[dils2k]

@joeriddles Is it expected behaviour? It seems to truncate the actual path:

m := chi.NewMux()
m.Route("/yo", func(r chi.Router) {
	r.Get("/sup", func(w http.ResponseWriter, r *http.Request) {
		w.Write([]byte("sup"))
	})
})

fmt.Println(m.Find(chi.NewRouteContext(), http.MethodGet, "/yo/sup")) // "/sup", but expected "/yo/sup"

[VojtechVitek]

@joeriddles @dils2k

btw: I have just found out that we already have this rctx.RoutePattern() method on RouteContext:

func Instrument(next http.Handler) http.Handler {
	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		next.ServeHTTP(w, r)
		routePattern := chi.RouteContext(r.Context()).RoutePattern()
		measure(w, r, routePattern)
	})
}

Isn't it what we were after?

[joeriddles]

@joeriddles Is it expected behaviour? It seems to truncate the actual path:

m := chi.NewMux()
m.Route("/yo", func(r chi.Router) {
	r.Get("/sup", func(w http.ResponseWriter, r *http.Request) {
		w.Write([]byte("sup"))
	})
})

fmt.Println(m.Find(chi.NewRouteContext(), http.MethodGet, "/yo/sup")) // "/sup", but expected "/yo/sup"

No, that is not expected behavior. I will create a fix.

[joeriddles]

@joeriddles @dils2k

btw: I have just found out that we already have this rctx.RoutePattern() method on RouteContext:

Isn't it what we were after?

The goal of Find is to determine the matched route pattern before the request is handled, similar to the existing Match function.

See the issue description:

I am working on adding a middleware for OpenTelemetry to a Chi-based service that needs to send a metric before the request has executed. We need to include a string for the API endpoint, like the route pattern, in the metric. This is currently not possible due to the way request route patterns are resolved using Chi's trie-based router, as noted by the maintainer in this issue #692 (comment).

[dils2k]

@joeriddles @dils2k

btw: I have just found out that we already have this rctx.RoutePattern() method on RouteContext:

func Instrument(next http.Handler) http.Handler {
	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		next.ServeHTTP(w, r)
		routePattern := chi.RouteContext(r.Context()).RoutePattern()
		measure(w, r, routePattern)
	})
}

Isn't it what we were after?

The issue with RoutePattern is that it returns the pattern only if the request successfully reaches the route see here.

However, in some cases (such as collecting metrics), it's necessary to determine the route pattern before the request is fully executed. This is important because requests can fail or be declined by authentication middleware.

[VojtechVitek]

Cool, makes sense. Thanks for the explanation 👍

@joeriddles I'll hold off with releasing new minor version until the fix is ready. Please keep me posted.

[VojtechVitek]

@joeriddles any updates?

This is blocking the release, so I'm tempted to revert this PR until a fix for #872 (comment) is ready. Thoughts?

[joeriddles]

This is blocking the release, so I'm tempted to revert this PR until a fix for #872 (comment) is ready. Thoughts?

@VojtechVitek a follow-up PR is created here: #954. Thanks!