Add middleware support

[vmihailenco]

Updates #69

[kravemir]

With latest golang's version, there is some traction to define middlewares in router-agnostic way, ie: func(http.Handler) http.Handler

Maybe, helper function to wrap middleware(s) using standard http package would be useful. For example echo#WrapMiddleware.

Chi router even uses middleware definition based on standard http package directly chi#Mux.Use.

Existing middlewares:

• https://godoc.org/github.com/rs/cors#Cors.Handler (it's method, but can be used as func(http.Handler) http.Handler),
• https://godoc.org/github.com/rs/formjson#Handler,
• https://www.godoc.org/github.com/philippgille/ln-paywall/wall#NewHandlerMiddleware

[vmihailenco]

I am all for it, but passing params in a fast way without allocation is the problem, e.g. you could

ctx = ctx.WithValue("params", params) // wastes some CPU (and allocates?)
req = req.WithContext(ctx) // allocates
req.Context().Value("params").(map[string]string) // and again

It probably does not matter in practice (except in benchmarks tournament) but it is also trivial to use standard middlewares with current design too:

router.Use(func(next HandlerFunc) HandlerFunc {
	h := cors.New(...)
	return func(w http.ResponseWriter, r *http.Request, params map[string]string) {
		h.HandlerFunc(w, r)
		next(w, r, params)
	}
})

httptreemux can even provide an adapter so the code above becomes

h := cors.New(...)
router.Use(httptreemux.Adapter(h.HandlerFunc))

[vmihailenco]

Or even

h := cors.New(...)
router.UseHandlerFunc(h.HandlerFunc)

I will add it - good idea :)

[kravemir]

I am all for it, but passing params in a fast way without allocation is the problem, e.g. you could...

Makes sense. Maybe for httptreemux, WrapMiddleware(m func(http.Handler) http.Handler) Middleware makes more sense.

Or even

I meant to stick to http.Handler, which is more widely used, as http.HandlerFunc is just an adapter for functions:

h := cors.New(...)
router.Use(httptreemux.WrapMiddleware(h.Handler))

[vmihailenco]

For now I've added only UseHandlerFunc since it is trivial and without "side effects".

It is possible to support Handler(h http.Handler) http.Handler too but there are 2 possibilities:

1. don't pass params further in chain which I guess is not a good option unless params are in req.Context already
2. allocate a closure for params on each request to preserve params
3. save params in req.Context if they are not there already, but then we pay the cost of using context and request.WIthContext without being compatible with Go ecosystem

My preference would be 2.) (and drop ContextGroup support - only half-joking) :) Or drop Group and use ContextGroup as the only option to have max compatibility with Go ecosystem (which makes much more sense). Then we don't need these adapters.

[kravemir]

I would go for maximum compatibility with Go ecosystem. However, @dimfeld should call shots.

Also, although function definition is convenient, interface based types allow to provide much more optional functionality, and not demand these extra functionalities to be implemented.

For example it is possible to list/print registered middlewares in user-friendly way, using fmt#Stringer: if asStringer, ok := middleware.(fmt.Stringer); ok { ... } else { ... }. Or, via type MiddlewareDescriber interface { DescribeMiddleware() string } This could allow to produce debug output after initialization of router, and list all registered routes and used middlewares in user-friendly way.

[vmihailenco]

Hard to tell how good this will work in practice if middleware does some heavy initialization on middleware(nextHandler) call.

[kravemir]

IMHO, middleware initialization should be lightweight, ie:

package middleware

type Middleware struct {...}

func New(...) *Middleware { /* initialize here */ }

type handler {
    m    *Middleware
    next http.Handler
}

// implement handler as http.Handler

func (m *Middleware) Handler (next func(http.Handler) http.Handler {
    return handler{m: m, next: next}
}

[kravemir]

One big reason for it to be lightweight is, that middleware will be used with many routes (different http.Handlers), and therefore it doesn't make sense to do heavy initialization for each route separately.

[kravemir]

Though,... this is internal implementation. The primary focus should be on public/exported API, which should stay same, once it's released.

[vmihailenco]

Not sure I follow. I mean that some existing 3rd party middlewares may do some initialization on middleware(nextHandler) call since most routers call middleware(nextHandler) once - not for every route call. We can't do that because we need to capture params.

[kravemir]

... call since most routers call middleware(nextHandler) once - not for every route call...

Not once per handled request, but once per route handler registration, as each registered route has got different handler.

With tens of registered routes, it doesn't make sense to have heavy weight initialization there, as it would mean for middleware to have heavy-weight handler wrapper,... therefore probably lots of data duplication (unnecessary memory consumption).

[kravemir]

@vmihailenco is it still work-in-progress, or ready for merge / review by @dimfeld ?

Actually, with this feature,... I might even start considering to switch from chi router to httptreemux. It was probably the most important reason, why I chose chi.

[vmihailenco]

Yes, it should be ready for review.

[dimfeld]

Thanks! I think this looks pretty good. I added a couple tests for params that I'll commit separately.

I agree that it would be nice to have a solution for http.Handler middleware that didn't involve calling the middleware function again for each request. I'm not entirely sure how to do that though without using Context. Perhaps that's ok, and we can put UseHandler in another file that only compiles under go 1.7+ where Context is available. Presumably anyone still on an older version of Go won't be using these new features anyway. Any thoughts on this?