middleware.GetHead

[pkieltyka]

This PR is an alternate approach/solution for #238 - which is to route undefined HEAD requests to GET handlers. In the previous implementation, it was set as default behaviour during routing - but I've felt that its better to be explicit about routes, so we use a middleware to search the routing tree for a matching handler and when missing route to the GET handler.

I've added Find(rctx *Context, method, path string) http.Handler to the chi.Routes.

[VojtechVitek]

I see you pass chi.NewRouteContext() as rctx into the FindHandler() method.

---

Do we ever need to pass real Context into it? And why?

I'm thinking if we could remove the rctx parameter from this method signature and pass the Context down to tree.FindRoute() from the FindHandler() method itself.

ie.

h, _ := rctx.FindHandler("HEAD", rctx.RoutePath)

// the FindHandler() method could pass the rctx itself
// or chi.NewRouteContext()
// down to the mx.tree.FindRoute() method

[pkieltyka]

@VojtechVitek I agree that would be nice, except the FindHandler method under the hood is calling mx.tree.FindRoute(rctx, m, path) which is the call to search the routing tree for the matching handler. The tree routing method requires a *Context (route context) where it will store the url params it find during traversal. It's tuned carefully for low gc pressure. In the case that the url params are throw-away (like in this case), then we could add a chi.NoContext, make an interface, and allow tree.FindRoute to skip setting url params during traversal. I dont really want to do this right now. Rather, people can make a pool of route context's in their middleware or whereever instead right now.

I just realized a different problem though, which is that the FindHandler method does not traverse the subrouter trees, therefore the current implementation is actually incomplete. I will push a breaking test soon.

[pkieltyka]

I've renamed FindHandler to just Find now based on @VojtechVitek suggestion - this feels better

As well, I realized Mux#Find does not search the subrouters, I've added a test case that proves it doesnt work, and will have to fix the implementation later when I have some time, or if someone wants to help thats cool too.

[pkieltyka]

I pushed an updated to Mux#Find that searches the subroutes as well. Give it another review please. I'm getting close to merge.

[VojtechVitek]

Can we return middlewares as a second argument? Would that be possible?

func (mx *Mux) Find(ctx, method, path) (http.Handler, ...func(http.Handler) http.Handler)

// ie.
handler, middlewares := r.Find(ctx, method, path)

[pkieltyka]

It's possible to offer the middlewares from the Mux#middlewares (the middleware stack), but, it wouldn't be very useful because if you stack middlewares from multiple subrouters, it wont represent the clear execution path to the endpoint handler.

What would be the use case to return the execution middleware/handler execution path here?

[VojtechVitek]

Initially, I was thinking about different use cases:

1. Being able to change the routes dynamically. (ie. Call a different handler with some middleware stack applied, based on request/context). But this would much more complex and probably not a good idea.

2. Being able to query Router/Mux for specific route and get the Handler and full Middleware stack. (ie. for documentation purposes: handler, middlewares := r.Find(method, path) - with this, I could easily verify that a specific route has some middleware applied). But this is something than can be done in Walk() anyway, so probably not a good idea either.

[VojtechVitek]

Just thinking out loud -- if this method finds a corresponding handler, how can we invoke it?

And how about its middleware chain?

[pkieltyka]

Currently the handler found is the endpoint handler, not the computed mux handler which contains the middlewares. The online middlewares part of the endpoint handler are the inline handlers which come from With() or Group() middlewares.

[DmitriyMV]

So basically we verify that the combination of RoutePath and RoutheMethod does have an endpoint, but we cannot call it because we are not sure that this route will go through all setted middlewares.

I have to ask - can we use the returned http.Handler in any meaningful way? If not - maybe we should return bool and rename it to IsExist or something like that.

[pkieltyka]

yea, I had a similar thought. Initially the handler was useful, but after writing the first implementation and realizing the need to traverse subrouters and consider explicit HEAD handlers in the tree for some subrouters and not others, the handler return value isn't very useful. It's possible to reconstruct the discrete path, or a computed path, but Id rather find a real need for that with this method, because that is what ServeHTTP() is for. A bool is better for checking if the path can find a matching route in the tree.

[VojtechVitek]

I agree. rctx.Routes.Exists(tctx, method, type) bool sounds good to me, since we can't use the handler anyway.

[DmitriyMV]

But, from the API viewpoint - maybe we could add a new private interface type and use it to cleanup API - this way we can achieve the similar performance without mental overhead for the end user.

[DmitriyMV]

Something like

// Find searches the routing tree for a handler that matches the method/path.
// It's similar to routing a http request, but without executing the handler
// thereafter.
func (mx *Mux) Find(method, path string) http.Handler {
	tctx := NewRouteContext()
	tctx.RouteMethod = method
	tctx.RoutePath = path
	return mx.find(tctx, method, path)
}

func (mx *Mux) find(rctx *Context, method, path string) http.Handler {
	m, ok := methodMap[method]
	if !ok {
		return nil
	}

	node, _, h := mx.tree.FindRoute(rctx, m, path)

	if node != nil && node.subroutes != nil {
		rctx.RoutePath = mx.nextRoutePath(rctx)
		if subroute, ok := node.subroutes.(extendedRoutes); ok {
			// Our type is implementing allocation free find. Use it.
			return subroute.find(rctx, rctx.RouteMethod, rctx.RoutePath)
		}
		// Fallback to the public method
		return node.subroutes.Find(rctx.RouteMethod, rctx.RoutePath)
	}

	return h
}

Where

// Routes interface adds two methods for router traversal, which is also
// used by the `docgen` subpackage to generation documentation for Routers.
type Routes interface {
	// Routes returns the routing tree in an easily traversable structure.
	Routes() []Route

	// Middlewares returns the list of middlewares in use by the router.
	Middlewares() Middlewares

	// Find searches the routing tree for a handler that matches
	// the method/path - similar to routing a http request, but without
	// executing the handler thereafter.
	Find(method, path string) http.Handler
}

type extendedRoutes interface {
	Routes
	find(rctx *Context, method, path string) http.Handler
}

[pkieltyka]

There are advantages to passing the chi.Context yourself, since you can look at the URI parameters that were parsed from the route. But also, you could pool them if you really wanted to.

[DmitriyMV]

But also, you could pool them if you really wanted to.

Sorry - can you elaborate?

[pkieltyka]

In the case you're creating a lot of NewRouteContext() objects and finding it to cause memory pressure for whatever reason (which I do not expect to be an issue), you could do something similar as here: https://github.com/go-chi/chi/blob/master/mux.go#L51-L53 - that is, make a bunch of temporary routing contexts and grab them from the pool. But, its not really necessary for just matching

[DmitriyMV]

I think the addition for (*Mux).Find tests would be a good idea - that would explain how it works with middlewares and handlers. As I understand it now - it works like this:

func TestMuxFind(t *testing.T) {
	result := []string{}

	r := NewRouter()
	r.Use(func(handler http.Handler) http.Handler {
		return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
			result = append(result, "middleware")
			handler.ServeHTTP(w, r)
		})
	})
	r.Get("/handler", func(w http.ResponseWriter, r *http.Request) {
		result = append(result, "handler")
	})

	tctx := NewRouteContext()
	tctx.RouteMethod = "GET"
	tctx.RoutePath = "/handler"

	h := r.Find(tctx, tctx.RouteMethod, tctx.RoutePath)
	h.ServeHTTP(nil, nil)
	t.Log(result)
}

Which results in this

mux_test.go:38: [handler]

So basically - no, no middlewares using Find method.

[pkieltyka]

@DmitriyMV yea, for sure will add a test for it too

[pkieltyka]

I've updated the PR again based on feedback - renamed the method to Match(*Context, method, path) bool and added a test case in mux_test.go

[VojtechVitek]

Does Match() need three parameters? Seems like RouteMethod and RoutePath are already stored in tctx and thus are available in the method.

Is there any use case where the tctx values would be different from method arguments?

[pkieltyka]

I believe the API is just clearer this way, as the routing context is intended to be read from, but can be manipulated to change logic, but shouldnt be an input in most common uses.