Make HTTP middleware easier to use

server/router/router.go

Index: server/router/router.go
diff --git a/server/router/router.go b/server/router/router.go
new file mode 100644
index 0000000000000000000000000000000000000000..53e3cac15e7f297df90f9959d7b43fc9cf7be188
--- /dev/null
+++ b/server/router/router.go
@@ -0,0 +1,167 @@
+// 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 }).