From 0b0092f24ab515b1d0e890b8f6b1372441ba4b65 Mon Sep 17 00:00:00 2001 From: Matt Butcher Date: Wed, 16 Mar 2016 16:39:59 -0600 Subject: [PATCH] fix(httputil): improve error handling Errors are now returned in the best matching format for the HTTP Accept header. --- cmd/manager/router/router.go | 3 +++ pkg/httputil/encoder.go | 10 +++++++-- pkg/httputil/encoder_test.go | 2 +- pkg/httputil/httperrors.go | 41 ++++++++++++++++++++++++++---------- 4 files changed, 42 insertions(+), 14 deletions(-) diff --git a/cmd/manager/router/router.go b/cmd/manager/router/router.go index 3ed685061..65d1e86fe 100644 --- a/cmd/manager/router/router.go +++ b/cmd/manager/router/router.go @@ -31,6 +31,7 @@ package router import ( "log" "net/http" + "reflect" "github.com/Masterminds/httputil" helmhttp "github.com/kubernetes/helm/pkg/httputil" @@ -68,6 +69,7 @@ func NewHandler(c *Context) *Handler { // // The route name is "VERB /ENPOINT/PATH", e.g. "GET /foo". func (h *Handler) Add(route string, fn HandlerFunc) { + log.Printf("Map %q to %s", route, reflect.ValueOf(fn).Type().Name()) h.routes[route] = fn h.paths = append(h.paths, route) h.resolver = httputil.NewResolver(h.paths) @@ -84,6 +86,7 @@ func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) { fn, ok := h.routes[route] if !ok { + // This is a 500 because the route was registered, but not here. helmhttp.Fatal(w, r, "route %s missing", route) } diff --git a/pkg/httputil/encoder.go b/pkg/httputil/encoder.go index beefd20c0..df5cfb428 100644 --- a/pkg/httputil/encoder.go +++ b/pkg/httputil/encoder.go @@ -28,6 +28,9 @@ import ( "github.com/ghodss/yaml" ) +// DefaultEncoder is an *AcceptEncoder with the default application/json encoding. +var DefaultEncoder = &AcceptEncoder{DefaultEncoding: "application/json"} + // Encoder takes input and translate it to an expected encoded output. // // Implementations of encoders may use details of the HTTP request and response @@ -40,7 +43,9 @@ type Encoder interface { // // When an encoder fails, it logs any necessary data and then responds to // the client. - Encode(http.ResponseWriter, *http.Request, interface{}) + // + // The integer must be a valid http.Status* status code. + Encode(http.ResponseWriter, *http.Request, interface{}, int) } // AcceptEncoder uses the accept headers on a request to determine the response type. @@ -56,7 +61,7 @@ type AcceptEncoder struct { } // Encode encodeds the given interface to the first available type in the Accept header. -func (e *AcceptEncoder) Encode(w http.ResponseWriter, r *http.Request, out interface{}) { +func (e *AcceptEncoder) Encode(w http.ResponseWriter, r *http.Request, out interface{}, statusCode int) { a := r.Header.Get("accept") fn := encoders[e.DefaultEncoding] mt := e.DefaultEncoding @@ -70,6 +75,7 @@ func (e *AcceptEncoder) Encode(w http.ResponseWriter, r *http.Request, out inter return } w.Header().Add("content-type", mt) + w.WriteHeader(statusCode) w.Write(data) } diff --git a/pkg/httputil/encoder_test.go b/pkg/httputil/encoder_test.go index 115c961d5..71ddc19eb 100644 --- a/pkg/httputil/encoder_test.go +++ b/pkg/httputil/encoder_test.go @@ -77,7 +77,7 @@ func TestAcceptEncoder(t *testing.T) { DefaultEncoding: "application/json", } fn := func(w http.ResponseWriter, r *http.Request) { - enc.Encode(w, r, []string{"hello", "world"}) + enc.Encode(w, r, []string{"hello", "world"}, http.StatusOK) } s := httptest.NewServer(http.HandlerFunc(fn)) defer s.Close() diff --git a/pkg/httputil/httperrors.go b/pkg/httputil/httperrors.go index c13a2a6bf..2a2577b39 100644 --- a/pkg/httputil/httperrors.go +++ b/pkg/httputil/httperrors.go @@ -30,27 +30,46 @@ const ( // LogFatal is for logging 500 errors. Form: Internal Server Error r.Method r.URL message LogFatal = "Internal Server Error: %s %s %s" // LogBadRequest logs 400 errors. - LogBadRequest = "Bad Request: %s %s" + LogBadRequest = "Bad Request: %s %s %s" ) +// Error represents an HTTP error that can be converted to structured types. +// +// For example, and error can be serialized to JSON or YAML. Likewise, the +// string marshal can convert it to a string. +type Error struct { + Msg string `json:"message, omitempty"` + Status string `json:"status"` +} + +// Error implements the error interface. +func (e *Error) Error() string { + return fmt.Sprintf("%s: %s", e.Status, e.Msg) +} + // NotFound writes a 404 error to the client and logs an error. func NotFound(w http.ResponseWriter, r *http.Request) { - log.Printf(LogNotFound, r.Method, r.URL) - w.WriteHeader(http.StatusNotFound) - fmt.Fprintln(w, "File Not Found") + msg := fmt.Sprintf(LogNotFound, r.Method, r.URL) + log.Println(msg) + writeErr(w, r, msg, http.StatusNotFound) +} + +// BadRequest writes an HTTP 400. +func BadRequest(w http.ResponseWriter, r *http.Request, err error) { + log.Printf(LogBadRequest, r.Method, r.URL, err) + writeErr(w, r, err.Error(), http.StatusBadRequest) } -func BadRequest(w http.ResponseWriter, r *http.Request) { - log.Printf(LogNotFound, r.Method, r.URL) - w.WriteHeader(http.StatusBadRequest) - fmt.Fprintln(w, "Bad Request") +// writeErr formats and writes the error using the default encoder. +func writeErr(w http.ResponseWriter, r *http.Request, msg string, status int) { + DefaultEncoder.Encode(w, r, &Error{Status: http.StatusText(status), Msg: msg}, status) } // Fatal writes a 500 response to the client and logs the message. // // Additional arguments are past into the the formatter as params to msg. func Fatal(w http.ResponseWriter, r *http.Request, msg string, v ...interface{}) { - log.Printf(LogFatal, r.Method, r.URL, fmt.Sprintf(msg, v...)) - w.WriteHeader(http.StatusInternalServerError) - fmt.Fprintln(w, "Internal Server Error") + m := fmt.Sprintf(msg, v...) + log.Printf(LogFatal, r.Method, r.URL, m) + writeErr(w, r, m, http.StatusInternalServerError) }