chi icon indicating copy to clipboard operation
chi copied to clipboard

chi.Walk missing middlewares

Open ebabani opened this issue 2 years ago • 6 comments

Hi,

I was trying to use chi.Walk to get a report of all routes and the middlewares used by every route. I noticed that some of the middlewares were missing from some of the nested routes when using groups.

Example routes:

	r := chi.NewRouter()

	r.Use(middleware.RequestID)

	r.Get("/A", func(w http.ResponseWriter, r *http.Request) {})

	r.Group(func(r chi.Router) {
		r.Use(middleware.Timeout(2500 * time.Millisecond))

		r.Get("/B", func(w http.ResponseWriter, r *http.Request) {})

		r.Route("/C", func(r chi.Router) {

			// Walk on the below route does not show the Timeout middleware
			r.Get("/D", func(w http.ResponseWriter, r *http.Request) {})
		})
	})

In the example above, I expected the C/D route to have both RequestId and Timeout middlewares. but chi.Walk only shows RequestID.

It's possible I'm missing something here and this is behaving as expected.

Go playground link: https://go.dev/play/p/HKh7cA35Bgx

ebabani avatar Sep 23 '22 04:09 ebabani

I guess the issue is that r.Route() creates a new sub-router (unlike r.Group() or r.Handle()) and Walk() doesn't reach out to its middleware stack.

https://github.com/go-chi/chi/blob/v0.9.0/mux.go#L169

VojtechVitek avatar Sep 23 '22 08:09 VojtechVitek

It does seem related to the Group to me. I updated the example with some nested routes, and for those it shows the correct middleware chain.

https://go.dev/play/p/fia_kU8Lzqs

ebabani avatar Sep 23 '22 13:09 ebabani

Group() is used for inlining middleware, where as Rotue() mount a subrouter -- it's a subtle difference, but there is one. The middlewares however will be there along the C/D route.

If I recall -- the inline middleware stack is computed and reduced in memory, and we don't keep around in memory the middleware stack, which is why we can't walk it. We could definitely add it, it just means a bit more code. It would be safe to keep, but would only be useful for walking like in this case.

pkieltyka avatar Oct 27 '22 11:10 pkieltyka

In terms of functionality all is good, it was just surprising behaviour when I hit this.

For context, I am using the walk function to write tests agains the router we're using. I was writing checks to ensure certain endpoints were always behind an auth middleware. I'm not super familiars with the chi internals, but if you can point me in the general area I can take a look and create a PR.

ebabani avatar Oct 27 '22 13:10 ebabani

In terms of functionality all is good, it was just surprising behaviour when I hit this.

For context, I am using the walk function to write tests agains the router we're using. I was writing checks to ensure certain endpoints were always behind an auth middleware. I'm not super familiars with the chi internals, but if you can point me in the general area I can take a look and create a PR.

@ebabani what did you end up doing for testing with Route() and Group()?

ganicus avatar Sep 15 '23 22:09 ganicus

Hey @ganicus,

We ended up not writing an explicit test for the middleware checks. We only have a handful of routes that don't have the auth middleware, and keeping that grouping separate from the main routing logic.

The only route tests we have are to make sure our api routes match our documented routes. We use https://github.com/swaggo/swag for our swagger docs, and our route tests confirm that all the chi routes exactly match the swagger declared routes.

ebabani avatar Sep 18 '23 14:09 ebabani