Introduce error handler middleware.

Replaces the service and controller error handlers.
This has several benefits, mainly it fixes a bug where error returned by
middlewares would not be caught because the error handler was at the
bottom of the middleware chain. Putting the error handler at the top of
the chain does not work with middlewares that rely on the HTTP response
being initialized like LogRequest.

Using a middleware allows the client code to control where the mapping
from error to HTTP response is done. The bootstrap code generated by
gen_main builds the middleware stack in the "right order" (i.e. the
error handler middleware is closer to the action handler then the
LogRequest middleware).

Author: raphael

error_test.go

@@ -273,7 +273,6 @@ var _ = Describe("Merge", func() {
 
 	Context("with two nil errors", func() {
 		It("returns a nil error", func() {
-			Ω(mErr).Should(BeAssignableToTypeOf(&goa.Error{}))
 			Ω(mErr).Should(BeNil())
 		})
 	})

goagen/gen_main/generator.go

@@ -239,6 +239,7 @@ func main() {
 	// Setup middleware
 	service.Use(middleware.RequestID())
 	service.Use(middleware.LogRequest(true))
+	service.Use(middleware.ErrorHandler(false))
 	service.Use(middleware.Recover())
 {{$api := .API}}
 {{range $name, $res := $api.Resources}}{{$name := goify $res.Name true}}	// Mount "{{$res.Name}}" controller

middleware/error_handler.go

@@ -0,0 +1,44 @@
+package middleware
+
+import (
+	"net/http"
+
+	"github.com/goadesign/goa"
+	"golang.org/x/net/context"
+)
+
+// ErrorHandler turns a Go error into an HTTP response. It should be placed in the middleware chain
+// below the logger middleware so the logger properly logs the HTTP response. ErrorHandler
+// understands instances of goa.Error and returns the status and response body embodied in them,
+// it turns other Go error types into a 500 internal error response.
+// If suppressInternal is true the details of internal errors is not included in HTTP responses.
+func ErrorHandler(suppressInternal bool) goa.Middleware {
+	return func(h goa.Handler) goa.Handler {
+		return func(ctx context.Context, rw http.ResponseWriter, req *http.Request) error {
+			e := h(ctx, rw, req)
+			if e == nil {
+				return nil
+			}
+
+			goa.LogInfo(ctx, "Default error handler", "err", e)
+			status := 500
+			var respBody interface{}
+			if err, ok := e.(*goa.Error); ok {
+				status = err.Status
+				respBody = err
+				rw.Header().Set("Content-Type", goa.ErrorMediaIdentifier)
+			} else {
+				respBody = e.Error()
+				rw.Header().Set("Content-Type", "text/plain")
+			}
+			if status >= 500 && status < 600 {
+				goa.LogError(ctx, e.Error())
+				if suppressInternal {
+					rw.Header().Set("Content-Type", goa.ErrorMediaIdentifier)
+					respBody = goa.ErrInternal("internal error, detail suppressed")
+				}
+			}
+			return goa.ContextResponse(ctx).Send(ctx, status, respBody)
+		}
+	}
+}

middleware/error_handler_test.go

@@ -0,0 +1,97 @@
+package middleware_test
+
+import (
+	"bytes"
+	"errors"
+	"fmt"
+	"net/http"
+
+	"golang.org/x/net/context"
+
+	"github.com/goadesign/goa"
+	"github.com/goadesign/goa/middleware"
+	. "github.com/onsi/ginkgo"
+	. "github.com/onsi/gomega"
+)
+
+var _ = Describe("ErrorHandler", func() {
+	var service *goa.Service
+	var h goa.Handler
+	var suppressInternal bool
+
+	var rw *testResponseWriter
+
+	BeforeEach(func() {
+		service = nil
+		h = nil
+		suppressInternal = false
+		rw = nil
+	})
+
+	JustBeforeEach(func() {
+		rw = newTestResponseWriter()
+		eh := middleware.ErrorHandler(suppressInternal)(h)
+		req, err := http.NewRequest("GET", "/foo", nil)
+		Ω(err).ShouldNot(HaveOccurred())
+		ctx := newContext(service, rw, req, nil)
+		err = eh(ctx, rw, req)
+		Ω(err).ShouldNot(HaveOccurred())
+	})
+
+	Context("with a handler returning a Go error", func() {
+		BeforeEach(func() {
+			service = newService(nil)
+			h = func(ctx context.Context, rw http.ResponseWriter, req *http.Request) error {
+				return errors.New("boom")
+			}
+		})
+
+		It("turns Go errors into HTTP 500 responses", func() {
+			Ω(rw.Status).Should(Equal(500))
+			Ω(rw.ParentHeader["Content-Type"]).Should(Equal([]string{"text/plain"}))
+			Ω(string(rw.Body)).Should(Equal(`"boom"` + "\n"))
+		})
+
+		Context("suppressing internal errors", func() {
+			BeforeEach(func() {
+				suppressInternal = true
+			})
+
+			It("suppresses the error details", func() {
+				var decoded goa.Error
+				Ω(rw.Status).Should(Equal(500))
+				Ω(rw.ParentHeader["Content-Type"]).Should(Equal([]string{goa.ErrorMediaIdentifier}))
+				err := service.Decode(&decoded, bytes.NewBuffer(rw.Body), "application/json")
+				Ω(err).ShouldNot(HaveOccurred())
+				Ω(fmt.Sprintf("%v", decoded)).Should(Equal(fmt.Sprintf("%v", *goa.ErrInternal("internal error, detail suppressed"))))
+			})
+
+		})
+	})
+
+	Context("with a handler returning a goa error", func() {
+		var gerr *goa.Error
+
+		BeforeEach(func() {
+			service = newService(nil)
+			gerr = &goa.Error{
+				Status:     418,
+				Detail:     "teapot",
+				Code:       "code",
+				MetaValues: map[string]interface{}{"foobar": 42},
+			}
+			h = func(ctx context.Context, rw http.ResponseWriter, req *http.Request) error {
+				return gerr
+			}
+		})
+
+		It("maps goa errors to HTTP responses", func() {
+			var decoded goa.Error
+			Ω(rw.Status).Should(Equal(gerr.Status))
+			Ω(rw.ParentHeader["Content-Type"]).Should(Equal([]string{goa.ErrorMediaIdentifier}))
+			err := service.Decode(&decoded, bytes.NewBuffer(rw.Body), "application/json")
+			Ω(err).ShouldNot(HaveOccurred())
+			Ω(fmt.Sprintf("%v", decoded)).Should(Equal(fmt.Sprintf("%v", *gerr)))
+		})
+	})
+})

middleware/gzip/middleware_test.go

@@ -45,9 +45,7 @@ var _ = Describe("Gzip", func() {
 		req, err = http.NewRequest("POST", "/foo/bar", strings.NewReader(`{"payload":42}`))
 		req.Header.Set("Accept-Encoding", "gzip")
 		Ω(err).ShouldNot(HaveOccurred())
-		rw = &TestResponseWriter{
-			ParentHeader: http.Header{},
-		}
+		rw = &TestResponseWriter{ParentHeader: make(http.Header)}
 
 		ctx = goa.NewContext(nil, rw, req, nil)
 		goa.ContextRequest(ctx).Payload = payload

middleware/middleware_test.go

@@ -12,6 +12,7 @@ import (
 func newService(logger goa.Logger) *goa.Service {
 	service := goa.New("test")
 	service.Encoder(goa.NewJSONEncoder, "*/*")
+	service.Decoder(goa.NewJSONDecoder, "*/*")
 	service.UseLogger(logger)
 	return service
 }
@@ -48,6 +49,11 @@ type testResponseWriter struct {
 	Status       int
 }
 
+func newTestResponseWriter() *testResponseWriter {
+	h := make(http.Header)
+	return &testResponseWriter{ParentHeader: h}
+}
+
 func (t *testResponseWriter) Header() http.Header {
 	return t.ParentHeader
 }

service.go

@@ -8,12 +8,16 @@ import (
 	"net/url"
 	"os"
 	"path/filepath"
-	"strconv"
 	"strings"
 
 	"golang.org/x/net/context"
 )
 
+const (
+	// ErrorMediaIdentifier is the media type identifier used for error responses.
+	ErrorMediaIdentifier = "application/vnd.api.error+json"
+)
+
 type (
 	// Service is the data structure supporting goa services.
 	// It provides methods for configuring a service and running it.
@@ -46,21 +50,18 @@ type (
 		Context context.Context
 		// Middleware chain
 		Middleware []Middleware
-		// Service-wide error handler
-		ErrorHandler ErrorHandler
 
-		cancel                context.CancelFunc
+		cancel                context.CancelFunc      // Service context cancel signal trigger
 		decoderPools          map[string]*decoderPool // Registered decoders for the service
 		encoderPools          map[string]*encoderPool // Registered encoders for the service
 		encodableContentTypes []string                // List of contentTypes for response negotiation
 	}
 
 	// Controller provides the common state and behavior for generated controllers.
 	Controller struct {
-		Name         string          // Controller resource name
-		Context      context.Context // Controller root context
-		ErrorHandler ErrorHandler    // Controller specific error handler if any
-		Middleware   []Middleware    // Controller specific middleware if any
+		Name       string          // Controller resource name
+		Context    context.Context // Controller root context
+		Middleware []Middleware    // Controller specific middleware if any
 	}
 
 	// Handler defines the controller handler signatures.
@@ -85,10 +86,9 @@ func New(name string) *Service {
 	ctx := UseLogger(context.Background(), NewStdLogger(stdlog))
 	ctx, cancel := context.WithCancel(ctx)
 	return &Service{
-		Name:         name,
-		ErrorHandler: DefaultErrorHandler,
-		Context:      ctx,
-		Mux:          NewMux(),
+		Name:    name,
+		Context: ctx,
+		Mux:     NewMux(),
 
 		cancel:                cancel,
 		decoderPools:          map[string]*decoderPool{},
@@ -143,10 +143,9 @@ func (service *Service) ListenAndServeTLS(addr, certFile, keyFile string) error
 func (service *Service) NewController(resName string) *Controller {
 	ctx := context.WithValue(service.Context, serviceKey, service)
 	return &Controller{
-		Name:         resName,
-		Middleware:   service.Middleware,
-		ErrorHandler: service.ErrorHandler,
-		Context:      context.WithValue(ctx, "ctrl", resName),
+		Name:       resName,
+		Middleware: service.Middleware,
+		Context:    context.WithValue(ctx, "ctrl", resName),
 	}
 }
 
@@ -205,23 +204,6 @@ func (ctrl *Controller) Use(m Middleware) {
 	ctrl.Middleware = append(ctrl.Middleware, m)
 }
 
-// HandleError invokes the controller error handler or - if there isn't one - the service error
-// handler.
-func (ctrl *Controller) HandleError(ctx context.Context, rw http.ResponseWriter, req *http.Request, err error) {
-	status := 500
-	if e, ok := err.(*Error); ok {
-		status = e.Status
-	}
-	go IncrCounter([]string{"goa", "handler", "error", strconv.Itoa(status)}, 1.0)
-	if ctrl.ErrorHandler != nil {
-		ctrl.ErrorHandler(ctx, rw, req, err)
-		return
-	}
-	if h := ContextService(ctx).ErrorHandler; h != nil {
-		h(ctx, rw, req, err)
-	}
-}
-
 // MuxHandler wraps a request handler into a MuxHandler. The MuxHandler initializes the
 // request context by loading the request state, invokes the handler and in case of error invokes
 // the controller (if there is one) or Service error handler.
@@ -231,10 +213,7 @@ func (ctrl *Controller) MuxHandler(name string, hdlr Handler, unm Unmarshaler) M
 	// Setup middleware outside of closure
 	middleware := func(ctx context.Context, rw http.ResponseWriter, req *http.Request) error {
 		if !ContextResponse(ctx).Written() {
-			if err := hdlr(ctx, rw, req); err != nil {
-				ContextService(ctx).LogInfo("ERROR", "err", err)
-				ctrl.HandleError(ctx, rw, req, err)
-			}
+			return hdlr(ctx, rw, req)
 		}
 		return nil
 	}
@@ -258,55 +237,27 @@ func (ctrl *Controller) MuxHandler(name string, hdlr Handler, unm Unmarshaler) M
 		handler := middleware
 		if err != nil {
 			handler = func(ctx context.Context, rw http.ResponseWriter, req *http.Request) error {
-				ctrl.ErrorHandler(ctx, rw, req, ErrInvalidEncoding(err))
-				return nil
+				rw.Header().Set("Content-Type", ErrorMediaIdentifier)
+				return ContextResponse(ctx).Send(ctx, 400, ErrInvalidEncoding(err))
 			}
 			for i := range chain {
 				handler = chain[ml-i-1](handler)
 			}
 		}
 
 		// Invoke middleware chain, wrap writer to capture response status and length
-		handler(ctx, ContextResponse(ctx), req)
-	}
-}
-
-// DefaultErrorHandler returns a response with status 500 or the status specified in the error if
-// an instance of HTTPStatusError.
-// It writes the error message to the response body in both cases.
-func DefaultErrorHandler(ctx context.Context, rw http.ResponseWriter, req *http.Request, e error) {
-	status := 500
-	var respBody interface{}
-	switch err := e.(type) {
-	case *Error:
-		status = err.Status
-		respBody = err
-	default:
-		respBody = e.Error()
-	}
-	if status == 500 {
-		LogError(ctx, e.Error())
+		if err := handler(ctx, ContextResponse(ctx), req); err != nil {
+			LastResortErrorHandler(ctx, rw, req, err)
+		}
 	}
-	ContextResponse(ctx).Send(ctx, status, respBody)
 }
 
-// TerseErrorHandler behaves like DefaultErrorHandler except that it does not write to the response
-// body for internal errors.
-func TerseErrorHandler(ctx context.Context, rw http.ResponseWriter, req *http.Request, e error) {
-	status := 500
-	var respBody interface{}
-	switch err := e.(type) {
-	case *Error:
-		status = err.Status
-		if status != 500 {
-			respBody = err
-		}
-	}
-	if respBody == nil {
-		respBody = "internal error"
-	}
-	if status == 500 {
-		LogError(ctx, e.Error())
-	}
-	ContextResponse(ctx).Send(ctx, status, respBody)
+// LastResortErrorHandler is the last thing that can handle an error propagating up the middleware
+// chain and turns all errors into a response indicating an internal error.
+// Ideally all errors are handled at a lower level in the middleware chain so they
+// can be logged properly.
+func LastResortErrorHandler(ctx context.Context, rw http.ResponseWriter, req *http.Request, e error) {
+	LogError(ctx, "Last resort error handler", "err", e)
+	respBody := fmt.Sprintf("Internal error: %s", e) // Sprintf catches panics
+	ContextResponse(ctx).Send(ctx, 500, respBody)
 }

service_test.go

@@ -151,7 +151,7 @@ var _ = Describe("Service", func() {
 				errorHandlerCalled := false
 
 				BeforeEach(func() {
-					s.ErrorHandler = TErrorHandler(&errorHandlerCalled)
+					s.Use(TErrorHandler(&errorHandlerCalled))
 				})
 
 				Context("by returning an error", func() {
@@ -219,10 +219,12 @@ var _ = Describe("Service", func() {
 	})
 })
 
-func TErrorHandler(witness *bool) goa.ErrorHandler {
-	return func(ctx context.Context, rw http.ResponseWriter, req *http.Request, err error) {
+func TErrorHandler(witness *bool) goa.Middleware {
+	m, _ := goa.NewMiddleware(func(ctx context.Context, rw http.ResponseWriter, req *http.Request) error {
 		*witness = true
-	}
+		return nil
+	})
+	return m
 }
 
 func TMiddleware(witness *bool) goa.Middleware {