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

Ambiguities re middlewares after routes #712

Closed
curio77 opened this issue Mar 17, 2022 · 7 comments · May be fixed by #714
Closed

Ambiguities re middlewares after routes #712

curio77 opened this issue Mar 17, 2022 · 7 comments · May be fixed by #714

Comments

@curio77
Copy link

curio77 commented Mar 17, 2022

In the following example,

package main

import (
	"net/http"

	"github.com/go-chi/chi/v5"
	"github.com/go-chi/chi/v5/middleware"
)

func main() {
	router := chi.NewRouter()
	router.Use(middleware.Recoverer)
	router.Group(func(subRouter chi.Router) {
		subRouter.Use(middleware.RequestID)
		subRouter.Get("/foo", func(w http.ResponseWriter, r *http.Request) {
			_, _ = w.Write([]byte("foo"))
		})
	})
	router.Group(func(subRouter chi.Router) {
		subRouter.Use(middleware.RequestID)
		subRouter.Get("/bar", func(w http.ResponseWriter, r *http.Request) {
			_, _ = w.Write([]byte("bar"))
		})
	})
	router.Use(middleware.RequestID)  // XXX
	panic(http.ListenAndServe(":8080", router).Error())
}

Why does the line marked // XXX raise panic: chi: all middlewares must be defined before routes on a mux? (Please ignore the pointlessness of the specific middlewares in this scenario, just adding any for testing purposes.)

Routes have been defined in sub-muxes via Router.Group(), but this apparently still prevents the later application of middlewares at the level of the parent/top router, despite the sub-muxes getting “fresh” middleware stacks as per the docs.

If the interpretation is that the sub-mux's route definitions count as route definitions per se, then why doesn't the second Router.Group() call panic for the same reason? Conversely, does the Router.Use() call marked // XXX apply its middleware to the sub-muxes as well?

@curio77
Copy link
Author

curio77 commented Mar 17, 2022

I need to add, about Router.Group() providing the sub-router with a “fresh” middleware stack… That's not the case at all!

package main

import (
	"fmt"
	"net/http"

	"github.com/go-chi/chi/v5"
)

func main() {
	router := chi.NewRouter()
	router.Use(dummyMiddleware)
	router.Group(func(subRouter chi.Router) {
		fmt.Println("Middlewares of sub-router inside Router.Group():", subRouter.Middlewares())
		subRouter.Get("/foo", func(w http.ResponseWriter, r *http.Request) {
			_, _ = w.Write([]byte("foo"))
		})
	})
	panic(http.ListenAndServe(":8080", router).Error())
}

func dummyMiddleware(next http.Handler) http.Handler {
	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		fmt.Println("dummyMiddleware!")
		next.ServeHTTP(w, r)
	})
}

Run this, query /foo, and you'll see dummyMiddleware! printed, despite the route definition inside Router.Group() supposedly having been served a “fresh” middlware stack and the fmt.Println() call in there printing Middlewares of sub-router inside Router.Group(): [] (i.e., apparently no middlewares in effect from this point of view)!

Is this intentional? If so, does this mean that the sub-router gets the stack of middlewares of the parent router but its additions do not propagate to the latter? If this is the case, this means that you cannot use Router.Group() to define routes that don't get the middleware stack of the parent router and that the slice of middlewares returned by Router.Middlewares() on the sub-router does not include those of the parent router. Then the Router.Group() should really be documented more clearly (WRT the definition of “fresh middleware stack”).

@pkieltyka
Copy link
Member

this is all intentionally yes. nothing we can do about it. sorry for the confusion, but this is how it is. the .Group() is a helper for inline-middleware handlers. Feel free to read the code to see find the answers why the design is constrained this way.

@VojtechVitek
Copy link
Contributor

VojtechVitek commented Mar 18, 2022

router.Group() doesn't provide a sub-router with a “fresh” middleware stack

it provides with a copy of parent middleware stack, on top of which you can apply middleware specific to the group only

@pkieltyka perhaps the godoc might be a bit misleading?

	// Group adds a new inline-Router along the current routing
	// path, with a fresh middleware stack for the inline-Router.
	Group(fn func(r Router)) Router

@curio77
Copy link
Author

curio77 commented Mar 18, 2022

Yep, as I said, that at least could do with some clarification.

@VojtechVitek
Copy link
Contributor

@curio77 would you be able to improve the godoc for the Group() method and submit a PR, please?

@curio77
Copy link
Author

curio77 commented Mar 18, 2022

Yep, will do…

@curio77
Copy link
Author

curio77 commented Mar 18, 2022

I'd like to add that it feels kinda weird to document such intrinsics/caveats on an interface method. Don't know whether it's actually just intrinsics of the implementation in chi.Mux, but if that's the foremost manifestation of the interface users will deal with…

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 a pull request may close this issue.

3 participants