Merge pull request goadesign#372 from goadesign/error_handler_middleware

Introduce error handler middleware.

Author: Raphaël Simon

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 {