Make HTTP middleware easier to use

server/router/router.go

+// Copyright 2016 The LUCI Authors. All rights reserved.
+// Use of this source code is governed under the Apache License, Version 2.0
+// that can be found in the LICENSE file.
+
+package router
+
+import (
+        "net/http"
+
+        "github.com/julienschmidt/httprouter"
+        "golang.org/x/net/context"
+)
+
+// TODO(nishanths): Implement adapter methods to convert http style handlers to Handlers.
+
+// Router represents the main router type.
+type (
+        // Router is the main type for the package. It contains the base path for
+        // the router, a list of handlers, and configuration.

[dnj (Google), 2016/06/13 18:50:42]

nit: Note that Router shouldn't be instantiated by users, and that they should
use New() instead.

+        Router struct {
+                hrouter  *httprouter.Router
+                handlers []Handler
+                BasePath string
+                parent   *Router
+        }
+
+        // Context is the argument to a Handler method. It is used to carry the
+        // HTTP request and response writer between handlers, share context,
+        // and manage flow using the Next and Abort methods.
+        Context struct {
+                Context context.Context
+                Writer  http.ResponseWriter
+                Request *http.Request
+                Params  httprouter.Params
+
+                handlers []Handler
+                index    int
+                aborted  bool
+        }
+)
+
+var (
+        // To ensure that Router conforms to the http.Handler interface.
+        _ http.Handler = New()

[dnj (Google), 2016/06/13 18:50:42]

Use:
var _ http.Handler = (*Router)(nil)

rather than call New()

[iannucci, 2016/06/13 19:23:29]

Do:

  _ http.Handler = (*Router)(nil)

As it will avoid the function call at runtime and just be statically checked by
the compiler.

+
+        // VoidHandler is a handler that has no effect.
+        VoidHandler Handler = func(_ *Context) {}
+)
+
+// New creates a Router with specified initial context.
+func New() *Router {
+        return &Router{
+                hrouter:  httprouter.New(),
+                BasePath: "/",
+        }
+}
+
+// Use adds middleware chains to the group. The added middleware applies to
+// all routes in the current group and in groups derived from the current group.
+func (r *Router) Use(handlers ...Handler) {
+        r.handlers = append(r.handlers, handlers...)
+}
+
+// Group creates a new router with an updated base path.
+// The new router carries over configuration from the router it derives
+// from.
+func (r *Router) Group(relativePath string) *Router {

[dnj (Google), 2016/06/13 18:50:42]

We should probably clean leading/trailing "/" from relativePath, and explicitly
add a "/" when generating the next BasePath.

[nodir, 2016/06/15 20:14:43]

I think other routers use method names Path and Subrouter. Consider one of these
names

[nishanths, 2016/06/16 00:18:09]

Chose 'Subrouter'. Open to changing the name if anyone has thoughts.

+        h := make([]Handler, len(r.handlers))
+        copy(h, r.handlers)
+        return &Router{
+                hrouter:  r.hrouter,
+                handlers: h,
+                BasePath: r.BasePath + relativePath,
+                parent:   r,
+        }
+}
+
+// GET is a shortcut for router.Handle("GET", path, handlers)
+func (r *Router) GET(path string, handlers ...Handler) {
+        r.Handle("GET", path, handlers...)
+}
+
+// HEAD is a shortcut for router.Handle("HEAD", path, handlers)
+func (r *Router) HEAD(path string, handlers ...Handler) {
+        r.Handle("HEAD", path, handlers...)
+}
+
+// OPTIONS is a shortcut for router.Handle("OPTIONS", path, handlers)
+func (r *Router) OPTIONS(path string, handlers ...Handler) {
+        r.Handle("OPTIONS", path, handlers...)
+}
+
+// POST is a shortcut for router.Handle("POST", path, handlers)
+func (r *Router) POST(path string, handlers ...Handler) {
+        r.Handle("POST", path, handlers...)
+}
+
+// PUT is a shortcut for router.Handle("PUT", path, handlers)
+func (r *Router) PUT(path string, handlers ...Handler) {
+        r.Handle("PUT", path, handlers...)
+}
+
+// PATCH is a shortcut for router.Handle("PATCH", path, handlers)
+func (r *Router) PATCH(path string, handlers ...Handler) {
+        r.Handle("PATCH", path, handlers...)
+}
+
+// DELETE is a shortcut for router.Handle("DELETE", path, handlers)
+func (r *Router) DELETE(path string, handlers ...Handler) {
+        r.Handle("DELETE", path, handlers...)
+}
+
+// Handle registers handlers for the given method and path. Handle panics if
+// no handlers are provided.
+func (r *Router) Handle(method, path string, handlers ...Handler) {
+        if len(handlers) == 0 {
+                panic("router: at least one handler should be specified")
+        }
+        h := r.adapt(handlers)
+        r.hrouter.Handle(method, path, h)
+}
+
+// ServeHTTP makes Router implement the http.Handler interface.
+func (r *Router) ServeHTTP(rw http.ResponseWriter, req *http.Request) {
+        r.hrouter.ServeHTTP(rw, req)
+}
+
+// copyHandlers allocates a new array and copies the
+// the handlers from the two handler lists into it.
+func copyHandlers(m, n []Handler) []Handler {
+        totalLen := len(m) + len(n)
+        merged := make([]Handler, totalLen)

[dnj (Google), 2016/06/13 18:50:42]

nit: probably cleaner to just allocate size 0, capacity "totalLen" and have two
normal "append(merged, m...)". Also just use "len(m) + len(n)" directly in the
allocation, no need to store it in its own variable.

[nishanths, 2016/06/15 18:25:08]

Acknowledged.

+        copy(merged, m)
+        copy(merged[len(m):], n)
+        return merged
+}
+
+// adapt adapts a list of handlers into a httprouter-style handler.
+func (r *Router) adapt(handlers []Handler) httprouter.Handle {
+        return httprouter.Handle(func(rw http.ResponseWriter, req *http.Request, p httprouter.Params) {
+                // TODO(maybe): Use a free list for making Contexts.
+                c := &Context{
+                        Context:  context.Background(),
+                        Writer:   rw,
+                        Request:  req,
+                        Params:   p,
+                        handlers: copyHandlers(r.handlers, handlers),

[dnj (Google), 2016/06/13 18:50:42]

Do we really need to allocate/copy handlers each time we handle an HTTP call? It
would be better to just store "adaptHandlers", and resolve them to an index
during execution (e.g.,

handlers := r.handlers
if i >= len(handlers) {
  handlers, i = r.adaptHandlers, (i - len(handlers))
}

... in Next.

[nishanths, 2016/06/15 18:25:08]

Thanks for catching this. Done.

+                        index:    -1,
+                }
+                c.Next()
+        })
+}
+
+// Next executes the next handler in the chain. If Next is called from inside a
+// handler, the next registered handler begins execution  immdediately.

[dnj (Google), 2016/06/13 18:50:42]

nit: two spaces.

+func (c *Context) Next() {
+        c.index++
+        for ; c.index < len(c.handlers) && !c.aborted; c.index++ {
+                c.handlers[c.index](c)
+        }
+}

[iannucci, 2016/06/13 19:23:29]

Is the intended usecase of this to allow a middleware that 'wraps' the entire
rest of the chain? If so, I would add a comment in the docstring to that effect:

"This is useful if you need to execute some code before+after the rest of the
chain. Most middleware implementations will not call this."

or something.

+
+// Abort prevents upcoming handlers from being executed. Calling Abort inside a
+// handler however does not stop the execution of the current handler.
+func (c *Context) Abort() {
+        c.aborted = true

[dnj (Google), 2016/06/13 18:50:42]

Just thinking here, but maybe we want "Abort()" to abort the current handler? We
could do that via panic(), then catch at middleware root and handle.

[nodir, 2016/06/13 20:20:52]

perhaps it may be simple to remove Abort and make Handler return bool. If
handler returns false, then no need to continue

[nodir, 2016/06/13 20:26:48]

and if you do that, replace "index" field in Context with "i" variable inside
"Context.Next". And then rename Context.Next to Run.
Unless it breaks some use case that I am not aware of

[nodir, 2016/06/13 20:29:36]

or rather remove handlers from Context (because really, they are not contextual
information) and change "Context.Run" to global "Run" function that accepts
handlers and the Context. I would be simpler

+}

[iannucci, 2016/06/13 19:23:29]

Yeah, I'm not convinced this is a good/safe api :/. I think capturing + exposing
the status code would be much better, as it allows existing handler code to be
used unmodified. For example, right now if we had a monitoring middleware that
came after the main handler, any calls to Abort would just point-blank skip that
processing.

If we capture the status code and expose it as c.StatusCode (or something), then
middlewares could make a choice about whether or not they want to execute anyway
(e.g. if c.StatusCode != 0 { return }).