Flatten HTTP handler config (#2662)

Differentiating middleware and responders has one benefit, namely that
it's clear which module provides the response, but even then it's not
a great advantage. Linear handler config makes a little more sense,
giving greater flexibility and simplifying the core a bit, even though
it's slightly awkward that handlers which are responders may not use
the 'next' handler that is passed in at all.

9 files changed, 99 insertions, 148 deletions

diff --git a/modules/caddyhttp/caddyhttp.go b/modules/caddyhttp/caddyhttp.go
index d0c7540..330e135 100644
--- a/modules/caddyhttp/caddyhttp.go
+++ b/modules/caddyhttp/caddyhttp.go
@@ -322,13 +322,15 @@ func (app *App) automaticHTTPS() error {
 							MatchHost(domains),
 						},
 					},
-					responder: Static{
-						StatusCode: http.StatusTemporaryRedirect, // TODO: use permanent redirect instead
-						Headers: http.Header{
-							"Location":   []string{redirTo},
-							"Connection": []string{"close"},
+					handlers: []MiddlewareHandler{
+						Static{
+							StatusCode: strconv.Itoa(http.StatusTemporaryRedirect), // TODO: use permanent redirect instead
+							Headers: http.Header{
+								"Location":   []string{redirTo},
+								"Connection": []string{"close"},
+							},
+							Close: true,
 						},
-						Close: true,
 					},
 				})
 			}
@@ -381,34 +383,18 @@ func (app *App) listenerTaken(network, address string) bool {
 var defaultALPN = []string{"h2", "http/1.1"}
 
 // RequestMatcher is a type that can match to a request.
-// A route matcher MUST NOT modify the request.
+// A route matcher MUST NOT modify the request, with the
+// only exception being its context.
 type RequestMatcher interface {
 	Match(*http.Request) bool
 }
 
-// Middleware chains one Handler to the next by being passed
-// the next Handler in the chain.
-type Middleware func(HandlerFunc) HandlerFunc
-
-// MiddlewareHandler is a Handler that includes a reference
-// to the next middleware handler in the chain. Middleware
-// handlers MUST NOT call Write() or WriteHeader() on the
-// response writer; doing so will panic. See Handler godoc
-// for more information.
-type MiddlewareHandler interface {
-	ServeHTTP(http.ResponseWriter, *http.Request, Handler) error
-}
-
 // Handler is like http.Handler except ServeHTTP may return an error.
 //
-// Middleware and responder handlers both implement this method.
-// Middleware must not call Write or WriteHeader on the ResponseWriter;
-// doing so will cause a panic. Responders should write to the response
-// if there was not an error.
-//
 // If any handler encounters an error, it should be returned for proper
 // handling. Return values should be propagated down the middleware chain
-// by returning it unchanged. Returned errors should not be re-wrapped.
+// by returning it unchanged. Returned errors should not be re-wrapped
+// if they are already HandlerError values.
 type Handler interface {
 	ServeHTTP(http.ResponseWriter, *http.Request) error
 }
@@ -421,9 +407,25 @@ func (f HandlerFunc) ServeHTTP(w http.ResponseWriter, r *http.Request) error {
 	return f(w, r)
 }
 
-// emptyHandler is used as a no-op handler, which is
-// sometimes better than a nil Handler pointer.
-var emptyHandler HandlerFunc = func(w http.ResponseWriter, r *http.Request) error { return nil }
+// Middleware chains one Handler to the next by being passed
+// the next Handler in the chain.
+type Middleware func(HandlerFunc) HandlerFunc
+
+// MiddlewareHandler is like Handler except it takes as a third
+// argument the next handler in the chain. The next handler will
+// never be nil, but may be a no-op handler if this is the last
+// handler in the chain. Handlers which act as middleware should
+// call the next handler's ServeHTTP method so as to propagate
+// the request down the chain properly. Handlers which act as
+// responders (content origins) need not invoke the next handler,
+// since the last handler in the chain should be the first to
+// write the response.
+type MiddlewareHandler interface {
+	ServeHTTP(http.ResponseWriter, *http.Request, Handler) error
+}
+
+// emptyHandler is used as a no-op handler.
+var emptyHandler HandlerFunc = func(http.ResponseWriter, *http.Request) error { return nil }
 
 const (
 	// DefaultHTTPPort is the default port for HTTP.
diff --git a/modules/caddyhttp/encode/encode.go b/modules/caddyhttp/encode/encode.go
index 2b45917..c78ccb9 100644
--- a/modules/caddyhttp/encode/encode.go
+++ b/modules/caddyhttp/encode/encode.go
@@ -36,7 +36,7 @@ import (
 
 func init() {
 	caddy.RegisterModule(caddy.Module{
-		Name: "http.middleware.encode",
+		Name: "http.handlers.encode",
 		New:  func() interface{} { return new(Encode) },
 	})
 }
diff --git a/modules/caddyhttp/fileserver/staticfiles.go b/modules/caddyhttp/fileserver/staticfiles.go
index 0bb910d..a7c72c9 100644
--- a/modules/caddyhttp/fileserver/staticfiles.go
+++ b/modules/caddyhttp/fileserver/staticfiles.go
@@ -35,7 +35,7 @@ func init() {
 	weakrand.Seed(time.Now().UnixNano())
 
 	caddy.RegisterModule(caddy.Module{
-		Name: "http.responders.file_server",
+		Name: "http.handlers.file_server",
 		New:  func() interface{} { return new(FileServer) },
 	})
 }
@@ -108,7 +108,7 @@ func (fsrv *FileServer) Validate() error {
 	return nil
 }
 
-func (fsrv *FileServer) ServeHTTP(w http.ResponseWriter, r *http.Request) error {
+func (fsrv *FileServer) ServeHTTP(w http.ResponseWriter, r *http.Request, _ caddyhttp.Handler) error {
 	repl := r.Context().Value(caddy.ReplacerCtxKey).(caddy.Replacer)
 
 	filesToHide := fsrv.transformHidePaths(repl)
@@ -119,7 +119,7 @@ func (fsrv *FileServer) ServeHTTP(w http.ResponseWriter, r *http.Request) error
 	if filename == "" {
 		// no files worked, so resort to fallback
 		if fsrv.Fallback != nil {
-			fallback, w := fsrv.Fallback.BuildCompositeRoute(w, r)
+			fallback := fsrv.Fallback.BuildCompositeRoute(w, r)
 			return fallback.ServeHTTP(w, r)
 		}
 		return caddyhttp.Error(http.StatusNotFound, nil)
@@ -452,7 +452,7 @@ const minBackoff, maxBackoff = 2, 5
 
 // Interface guards
 var (
-	_ caddy.Provisioner = (*FileServer)(nil)
-	_ caddy.Validator   = (*FileServer)(nil)
-	_ caddyhttp.Handler = (*FileServer)(nil)
+	_ caddy.Provisioner           = (*FileServer)(nil)
+	_ caddy.Validator             = (*FileServer)(nil)
+	_ caddyhttp.MiddlewareHandler = (*FileServer)(nil)
 )
diff --git a/modules/caddyhttp/rewrite/rewrite.go b/modules/caddyhttp/rewrite/rewrite.go
index 9060e22..ac113ff 100644
--- a/modules/caddyhttp/rewrite/rewrite.go
+++ b/modules/caddyhttp/rewrite/rewrite.go
@@ -25,7 +25,7 @@ import (
 
 func init() {
 	caddy.RegisterModule(caddy.Module{
-		Name: "http.middleware.rewrite",
+		Name: "http.handlers.rewrite",
 		New:  func() interface{} { return new(Rewrite) },
 	})
 }
diff --git a/modules/caddyhttp/routes.go b/modules/caddyhttp/routes.go
index 5e61fae..e010bb6 100644
--- a/modules/caddyhttp/routes.go
+++ b/modules/caddyhttp/routes.go
@@ -28,25 +28,19 @@ import (
 type ServerRoute struct {
 	Group       string                       `json:"group,omitempty"`
 	MatcherSets []map[string]json.RawMessage `json:"match,omitempty"`
-	Apply       []json.RawMessage            `json:"apply,omitempty"`
-	Respond     json.RawMessage              `json:"respond,omitempty"`
-
-	Terminal bool `json:"terminal,omitempty"`
+	Handle      []json.RawMessage            `json:"handle,omitempty"`
+	Terminal    bool                         `json:"terminal,omitempty"`
 
 	// decoded values
 	matcherSets []MatcherSet
-	middleware  []MiddlewareHandler
-	responder   Handler
+	handlers    []MiddlewareHandler
 }
 
 // Empty returns true if the route has all zero/default values.
 func (sr ServerRoute) Empty() bool {
 	return len(sr.MatcherSets) == 0 &&
-		len(sr.Apply) == 0 &&
-		len(sr.Respond) == 0 &&
-		len(sr.matcherSets) == 0 &&
-		len(sr.middleware) == 0 &&
-		sr.responder == nil &&
+		len(sr.Handle) == 0 &&
+		len(sr.handlers) == 0 &&
 		!sr.Terminal &&
 		sr.Group == ""
 }
@@ -98,40 +92,27 @@ func (routes RouteList) Provision(ctx caddy.Context) error {
 		}
 		routes[i].MatcherSets = nil // allow GC to deallocate - TODO: Does this help?
 
-		// middleware
-		for j, rawMsg := range route.Apply {
-			mid, err := ctx.LoadModuleInline("middleware", "http.middleware", rawMsg)
-			if err != nil {
-				return fmt.Errorf("loading middleware module in position %d: %v", j, err)
-			}
-			routes[i].middleware = append(routes[i].middleware, mid.(MiddlewareHandler))
-		}
-		routes[i].Apply = nil // allow GC to deallocate - TODO: Does this help?
-
-		// responder
-		if route.Respond != nil {
-			resp, err := ctx.LoadModuleInline("responder", "http.responders", route.Respond)
+		// handlers
+		for j, rawMsg := range route.Handle {
+			mh, err := ctx.LoadModuleInline("handler", "http.handlers", rawMsg)
 			if err != nil {
-				return fmt.Errorf("loading responder module: %v", err)
+				return fmt.Errorf("loading handler module in position %d: %v", j, err)
 			}
-			routes[i].responder = resp.(Handler)
+			routes[i].handlers = append(routes[i].handlers, mh.(MiddlewareHandler))
 		}
-		routes[i].Respond = nil // allow GC to deallocate - TODO: Does this help?
+		routes[i].Handle = nil // allow GC to deallocate - TODO: Does this help?
 	}
 	return nil
 }
 
-// BuildCompositeRoute creates a chain of handlers by applying all the matching
-// routes. The returned ResponseWriter should be used instead of rw.
-func (routes RouteList) BuildCompositeRoute(rw http.ResponseWriter, req *http.Request) (Handler, http.ResponseWriter) {
-	mrw := &middlewareResponseWriter{ResponseWriterWrapper: &ResponseWriterWrapper{rw}}
-
+// BuildCompositeRoute creates a chain of handlers by
+// applying all of the matching routes.
+func (routes RouteList) BuildCompositeRoute(rw http.ResponseWriter, req *http.Request) Handler {
 	if len(routes) == 0 {
-		return emptyHandler, mrw
+		return emptyHandler
 	}
 
 	var mid []Middleware
-	var responder Handler
 	groups := make(map[string]struct{})
 
 	for _, route := range routes {
@@ -140,9 +121,8 @@ func (routes RouteList) BuildCompositeRoute(rw http.ResponseWriter, req *http.Re
 			continue
 		}
 
-		// if route is part of a group, ensure only
-		// the first matching route in the group is
-		// applied
+		// if route is part of a group, ensure only the
+		// first matching route in the group is applied
 		if route.Group != "" {
 			_, ok := groups[route.Group]
 			if ok {
@@ -155,78 +135,48 @@ func (routes RouteList) BuildCompositeRoute(rw http.ResponseWriter, req *http.Re
 		}
 
 		// apply the rest of the route
-		for _, m := range route.middleware {
-			// we have to be sure to wrap m outside
-			// of our current scope so that the
-			// reference to this m isn't overwritten
-			// on the next iteration, leaving only
-			// the last middleware in the chain as
-			// the ONLY middleware in the chain!
-			mid = append(mid, wrapMiddleware(m))
-		}
-		if responder == nil {
-			responder = route.responder
+		for _, mh := range route.handlers {
+			// we have to be sure to wrap mh outside
+			// of our current stack frame so that the
+			// reference to this mh isn't overwritten
+			// on the next iteration, leaving the last
+			// middleware in the chain as the ONLY
+			// middleware in the chain!
+			mid = append(mid, wrapMiddleware(mh))
 		}
+
+		// if this route is supposed to be last, don't
+		// compile any more into the chain
 		if route.Terminal {
 			break
 		}
 	}
 
-	// build the middleware stack, with the responder at the end
-	stack := HandlerFunc(func(w http.ResponseWriter, r *http.Request) error {
-		if responder == nil {
-			return nil
-		}
-		mrw.allowWrites = true
-		return responder.ServeHTTP(w, r)
-	})
+	// build the middleware chain, with the responder at the end
+	stack := emptyHandler
 	for i := len(mid) - 1; i >= 0; i-- {
 		stack = mid[i](stack)
 	}
 
-	return stack, mrw
+	return stack
 }
 
 // wrapMiddleware wraps m such that it can be correctly
-// appended to a list of middleware. This separate closure
-// is necessary so that only the last middleware in a loop
-// does not become the only middleware of the stack,
-// repeatedly executed (i.e. it is necessary to keep a
-// reference to this m outside of the scope of a loop)!
-func wrapMiddleware(m MiddlewareHandler) Middleware {
+// appended to a list of middleware. We can't do this
+// directly in a loop because it relies on a reference
+// to mh not changing until the execution of its handler,
+// which is deferred by multiple func closures. In other
+// words, we need to pull this particular MiddlewareHandler
+// pointer into its own stack frame to preserve it so it
+// won't be overwritten in future loop iterations.
+func wrapMiddleware(mh MiddlewareHandler) Middleware {
 	return func(next HandlerFunc) HandlerFunc {
 		return func(w http.ResponseWriter, r *http.Request) error {
 			// TODO: This is where request tracing could be implemented; also
 			// see below to trace the responder as well
 			// TODO: Trace a diff of the request, would be cool too! see what changed since the last middleware (host, headers, URI...)
 			// TODO: see what the std lib gives us in terms of stack tracing too
-			return m.ServeHTTP(w, r, next)
+			return mh.ServeHTTP(w, r, next)
 		}
 	}
 }
-
-type middlewareResponseWriter struct {
-	*ResponseWriterWrapper
-	allowWrites bool
-}
-
-func (mrw middlewareResponseWriter) WriteHeader(statusCode int) {
-	if !mrw.allowWrites {
-		// technically, this is not true: middleware can write headers,
-		// but only after the responder handler has returned; either the
-		// responder did nothing with the response (sad face), or the
-		// middleware wrapped the response and deferred the write
-		panic("WriteHeader: middleware cannot write response headers")
-	}
-	mrw.ResponseWriterWrapper.WriteHeader(statusCode)
-}
-
-func (mrw middlewareResponseWriter) Write(b []byte) (int, error) {
-	if !mrw.allowWrites {
-		panic("Write: middleware cannot write to the response before responder")
-	}
-	return mrw.ResponseWriterWrapper.Write(b)
-}
-
-// Interface guard
-var _ HTTPInterfaces = (*middlewareResponseWriter)(nil)
diff --git a/modules/caddyhttp/server.go b/modules/caddyhttp/server.go
index d40a01d..8bc3a5a 100644
--- a/modules/caddyhttp/server.go
+++ b/modules/caddyhttp/server.go
@@ -65,9 +65,9 @@ func (s *Server) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 	addHTTPVarsToReplacer(repl, r, w)
 
 	// build and execute the main handler chain
-	stack, wrappedWriter := s.Routes.BuildCompositeRoute(w, r)
+	stack := s.Routes.BuildCompositeRoute(w, r)
 	stack = s.wrapPrimaryRoute(stack)
-	err := s.executeCompositeRoute(wrappedWriter, r, stack)
+	err := s.executeCompositeRoute(w, r, stack)
 	if err != nil {
 		// add the raw error value to the request context
 		// so it can be accessed by error handlers
@@ -85,8 +85,8 @@ func (s *Server) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 		}
 
 		if s.Errors != nil && len(s.Errors.Routes) > 0 {
-			errStack, wrappedWriter := s.Errors.Routes.BuildCompositeRoute(w, r)
-			err := s.executeCompositeRoute(wrappedWriter, r, errStack)
+			errStack := s.Errors.Routes.BuildCompositeRoute(w, r)
+			err := s.executeCompositeRoute(w, r, errStack)
 			if err != nil {
 				// TODO: what should we do if the error handler has an error?
 				log.Printf("[ERROR] [%s %s] handling error: %v", r.Method, r.RequestURI, err)
@@ -154,6 +154,8 @@ func (s *Server) enforcementHandler(w http.ResponseWriter, r *http.Request, next
 	return next.ServeHTTP(w, r)
 }
 
+// listenersUseAnyPortOtherThan returns true if there are any
+// listeners in s that use a port which is not otherPort.
 func (s *Server) listenersUseAnyPortOtherThan(otherPort int) bool {
 	for _, lnAddr := range s.Listen {
 		_, addrs, err := caddy.ParseListenAddr(lnAddr)
diff --git a/modules/caddyhttp/staticresp.go b/modules/caddyhttp/staticresp.go
index 3f43c92..8e4a3df 100644
--- a/modules/caddyhttp/staticresp.go
+++ b/modules/caddyhttp/staticresp.go
@@ -24,21 +24,20 @@ import (
 
 func init() {
 	caddy.RegisterModule(caddy.Module{
-		Name: "http.responders.static",
+		Name: "http.handlers.static",
 		New:  func() interface{} { return new(Static) },
 	})
 }
 
 // Static implements a simple responder for static responses.
 type Static struct {
-	StatusCode    int         `json:"status_code"` // TODO: should we turn this into a string so that only one field is needed? (string allows replacements)
-	StatusCodeStr string      `json:"status_code_str"`
-	Headers       http.Header `json:"headers"`
-	Body          string      `json:"body"`
-	Close         bool        `json:"close"`
+	StatusCode string      `json:"status_code"`
+	Headers    http.Header `json:"headers"`
+	Body       string      `json:"body"`
+	Close      bool        `json:"close"`
 }
 
-func (s Static) ServeHTTP(w http.ResponseWriter, r *http.Request) error {
+func (s Static) ServeHTTP(w http.ResponseWriter, r *http.Request, _ Handler) error {
 	repl := r.Context().Value(caddy.ReplacerCtxKey).(caddy.Replacer)
 
 	// close the connection after responding
@@ -60,16 +59,13 @@ func (s Static) ServeHTTP(w http.ResponseWriter, r *http.Request) error {
 	}
 
 	// get the status code
-	statusCode := s.StatusCode
-	if statusCode == 0 && s.StatusCodeStr != "" {
-		intVal, err := strconv.Atoi(repl.ReplaceAll(s.StatusCodeStr, ""))
+	statusCode := http.StatusOK
+	if s.StatusCode != "" {
+		intVal, err := strconv.Atoi(repl.ReplaceAll(s.StatusCode, ""))
 		if err == nil {
 			statusCode = intVal
 		}
 	}
-	if statusCode == 0 {
-		statusCode = http.StatusOK
-	}
 
 	// write headers
 	w.WriteHeader(statusCode)
@@ -83,4 +79,4 @@ func (s Static) ServeHTTP(w http.ResponseWriter, r *http.Request) error {
 }
 
 // Interface guard
-var _ Handler = (*Static)(nil)
+var _ MiddlewareHandler = (*Static)(nil)
diff --git a/modules/caddyhttp/staticresp_test.go b/modules/caddyhttp/staticresp_test.go
index b518bb0..111e4f3 100644
--- a/modules/caddyhttp/staticresp_test.go
+++ b/modules/caddyhttp/staticresp_test.go
@@ -19,6 +19,7 @@ import (
 	"io/ioutil"
 	"net/http"
 	"net/http/httptest"
+	"strconv"
 	"testing"
 
 	"github.com/caddyserver/caddy/v2"
@@ -29,7 +30,7 @@ func TestStaticResponseHandler(t *testing.T) {
 	w := httptest.NewRecorder()
 
 	s := Static{
-		StatusCode: http.StatusNotFound,
+		StatusCode: strconv.Itoa(http.StatusNotFound),
 		Headers: http.Header{
 			"X-Test": []string{"Testing"},
 		},
@@ -37,7 +38,7 @@ func TestStaticResponseHandler(t *testing.T) {
 		Close: true,
 	}
 
-	err := s.ServeHTTP(w, r)
+	err := s.ServeHTTP(w, r, nil)
 	if err != nil {
 		t.Errorf("did not expect an error, but got: %v", err)
 	}
diff --git a/modules/caddyhttp/templates/templates.go b/modules/caddyhttp/templates/templates.go
index e5bfa35..85b0bc2 100644
--- a/modules/caddyhttp/templates/templates.go
+++ b/modules/caddyhttp/templates/templates.go
@@ -28,7 +28,7 @@ import (
 
 func init() {
 	caddy.RegisterModule(caddy.Module{
-		Name: "http.middleware.templates",
+		Name: "http.handlers.templates",
 		New:  func() interface{} { return new(Templates) },
 	})
 }