Update to use util.JSONResponse (#18)

src/github.com/matrix-org/dendrite/clientapi/readers/sync.go

@@ -7,11 +7,8 @@ import (
 )
 
 // Sync implements /sync
-func Sync(req *http.Request) (interface{}, *util.HTTPError) {
+func Sync(req *http.Request) util.JSONResponse {
 	logger := util.GetLogger(req.Context())
 	logger.Info("Doing stuff...")
-	return nil, &util.HTTPError{
-		Code:    404,
-		Message: "Not implemented yet",
-	}
+	return util.MessageResponse(404, "Not implemented yet")
 }

src/github.com/matrix-org/dendrite/clientapi/routing/routing.go

@@ -17,11 +17,11 @@ const pathPrefixR0 = "/_matrix/client/r0"
 func Setup(servMux *http.ServeMux, httpClient *http.Client) {
 	apiMux := mux.NewRouter()
 	r0mux := apiMux.PathPrefix(pathPrefixR0).Subrouter()
-	r0mux.Handle("/sync", make("sync", wrap(func(req *http.Request) (interface{}, *util.HTTPError) {
+	r0mux.Handle("/sync", make("sync", wrap(func(req *http.Request) util.JSONResponse {
 		return readers.Sync(req)
 	})))
 	r0mux.Handle("/rooms/{roomID}/send/{eventType}",
-		make("send_message", wrap(func(req *http.Request) (interface{}, *util.HTTPError) {
+		make("send_message", wrap(func(req *http.Request) util.JSONResponse {
 			vars := mux.Vars(req)
 			return writers.SendMessage(req, vars["roomID"], vars["eventType"])
 		})),
@@ -38,12 +38,12 @@ func make(metricsName string, h util.JSONRequestHandler) http.Handler {
 
 // jsonRequestHandlerWrapper is a wrapper to allow in-line functions to conform to util.JSONRequestHandler
 type jsonRequestHandlerWrapper struct {
-	function func(req *http.Request) (interface{}, *util.HTTPError)
+	function func(req *http.Request) util.JSONResponse
 }
 
-func (r *jsonRequestHandlerWrapper) OnIncomingRequest(req *http.Request) (interface{}, *util.HTTPError) {
+func (r *jsonRequestHandlerWrapper) OnIncomingRequest(req *http.Request) util.JSONResponse {
 	return r.function(req)
 }
-func wrap(f func(req *http.Request) (interface{}, *util.HTTPError)) *jsonRequestHandlerWrapper {
+func wrap(f func(req *http.Request) util.JSONResponse) *jsonRequestHandlerWrapper {
 	return &jsonRequestHandlerWrapper{f}
 }

src/github.com/matrix-org/dendrite/clientapi/writers/sendmessage.go

@@ -7,11 +7,8 @@ import (
 )
 
 // SendMessage implements /rooms/{roomID}/send/{eventType}
-func SendMessage(req *http.Request, roomID, eventType string) (interface{}, *util.HTTPError) {
+func SendMessage(req *http.Request, roomID, eventType string) util.JSONResponse {
 	logger := util.GetLogger(req.Context())
 	logger.WithField("roomID", roomID).WithField("eventType", eventType).Info("Doing stuff...")
-	return nil, &util.HTTPError{
-		Code:    404,
-		Message: "Not implemented yet",
-	}
+	return util.MessageResponse(404, "Not implemented yet")
 }

vendor/manifest

@@ -98,7 +98,7 @@
 		{
 			"importpath": "github.com/matrix-org/util",
 			"repository": "https://github.com/matrix-org/util",
-			"revision": "4de125c773716ad380f2f80cc6c04789ef4c906a",
+			"revision": "ccef6dc7c24a7c896d96b433a9107b7c47ecf828",
 			"branch": "master"
 		},
 		{
@@ -206,4 +206,4 @@
 			"branch": "master"
 		}
 	]
-}
+}

vendor/src/github.com/matrix-org/util/context.go

@@ -0,0 +1,35 @@
+package util
+
+import (
+	"context"
+
+	log "github.com/Sirupsen/logrus"
+)
+
+// contextKeys is a type alias for string to namespace Context keys per-package.
+type contextKeys string
+
+// ctxValueRequestID is the key to extract the request ID for an HTTP request
+const ctxValueRequestID = contextKeys("requestid")
+
+// GetRequestID returns the request ID associated with this context, or the empty string
+// if one is not associated with this context.
+func GetRequestID(ctx context.Context) string {
+	id := ctx.Value(ctxValueRequestID)
+	if id == nil {
+		return ""
+	}
+	return id.(string)
+}
+
+// ctxValueLogger is the key to extract the logrus Logger.
+const ctxValueLogger = contextKeys("logger")
+
+// GetLogger retrieves the logrus logger from the supplied context. Returns nil if there is no logger.
+func GetLogger(ctx context.Context) *log.Entry {
+	l := ctx.Value(ctxValueLogger)
+	if l == nil {
+		return nil
+	}
+	return l.(*log.Entry)
+}

vendor/src/github.com/matrix-org/util/error.go

@@ -1,24 +0,0 @@
-package util
-
-import "fmt"
-
-// HTTPError An HTTP Error response, which may wrap an underlying native Go Error.
-type HTTPError struct {
-	WrappedError error
-	// A human-readable message to return to the client in a JSON response. This
-	// is ignored if JSON is supplied.
-	Message string
-	// HTTP status code.
-	Code int
-	// JSON represents the JSON that should be serialized and sent to the client
-	// instead of the given Message.
-	JSON interface{}
-}
-
-func (e HTTPError) Error() string {
-	var wrappedErrMsg string
-	if e.WrappedError != nil {
-		wrappedErrMsg = e.WrappedError.Error()
-	}
-	return fmt.Sprintf("%s: %d: %s", e.Message, e.Code, wrappedErrMsg)
-}

vendor/src/github.com/matrix-org/util/json.go

@@ -3,7 +3,6 @@ package util
 import (
 	"context"
 	"encoding/json"
-	"fmt"
 	"math/rand"
 	"net/http"
 	"runtime/debug"
@@ -12,46 +11,51 @@ import (
 	log "github.com/Sirupsen/logrus"
 )
 
-// contextKeys is a type alias for string to namespace Context keys per-package.
-type contextKeys string
-
-// ctxValueRequestID is the key to extract the request ID for an HTTP request
-const ctxValueRequestID = contextKeys("requestid")
-
-// GetRequestID returns the request ID associated with this context, or the empty string
-// if one is not associated with this context.
-func GetRequestID(ctx context.Context) string {
-	id := ctx.Value(ctxValueRequestID)
-	if id == nil {
-		return ""
-	}
-	return id.(string)
+// JSONResponse represents an HTTP response which contains a JSON body.
+type JSONResponse struct {
+	// HTTP status code.
+	Code int
+	// JSON represents the JSON that should be serialized and sent to the client
+	JSON interface{}
+	// Headers represent any headers that should be sent to the client
+	Headers map[string]string
 }
 
-// ctxValueLogger is the key to extract the logrus Logger.
-const ctxValueLogger = contextKeys("logger")
+// Is2xx returns true if the Code is between 200 and 299.
+func (r JSONResponse) Is2xx() bool {
+	return r.Code/100 == 2
+}
 
-// GetLogger retrieves the logrus logger from the supplied context. Returns nil if there is no logger.
-func GetLogger(ctx context.Context) *log.Entry {
-	l := ctx.Value(ctxValueLogger)
-	if l == nil {
-		return nil
+// RedirectResponse returns a JSONResponse which 302s the client to the given location.
+func RedirectResponse(location string) JSONResponse {
+	headers := make(map[string]string)
+	headers["Location"] = location
+	return JSONResponse{
+		Code:    302,
+		JSON:    struct{}{},
+		Headers: headers,
 	}
-	return l.(*log.Entry)
+}
+
+// MessageResponse returns a JSONResponse with a 'message' key containing the given text.
+func MessageResponse(code int, msg string) JSONResponse {
+	return JSONResponse{
+		Code: code,
+		JSON: struct {
+			Message string `json:"message"`
+		}{msg},
+	}
+}
+
+// ErrorResponse returns an HTTP 500 JSONResponse with the stringified form of the given error.
+func ErrorResponse(err error) JSONResponse {
+	return MessageResponse(500, err.Error())
 }
 
 // JSONRequestHandler represents an interface that must be satisfied in order to respond to incoming
-// HTTP requests with JSON. The interface returned will be marshalled into JSON to be sent to the client,
-// unless the interface is []byte in which case the bytes are sent to the client unchanged.
-// If an error is returned, a JSON error response will also be returned, unless the error code
-// is a 302 REDIRECT in which case a redirect is sent based on the Message field.
+// HTTP requests with JSON.
 type JSONRequestHandler interface {
-	OnIncomingRequest(req *http.Request) (interface{}, *HTTPError)
-}
-
-// JSONError represents a JSON API error response
-type JSONError struct {
-	Message string `json:"message"`
+	OnIncomingRequest(req *http.Request) JSONResponse
 }
 
 // Protect panicking HTTP requests from taking down the entire process, and log them using
@@ -67,12 +71,7 @@ func Protect(handler http.HandlerFunc) http.HandlerFunc {
 				}).Errorf(
 					"Request panicked!\n%s", debug.Stack(),
 				)
-				jsonErrorResponse(
-					w, req, &HTTPError{
-						Message: "Internal Server Error",
-						Code:    500,
-					},
-				)
+				respond(w, req, MessageResponse(500, "Internal Server Error"))
 			}
 		}()
 		handler(w, req)
@@ -81,11 +80,11 @@ func Protect(handler http.HandlerFunc) http.HandlerFunc {
 
 // MakeJSONAPI creates an HTTP handler which always responds to incoming requests with JSON responses.
 // Incoming http.Requests will have a logger (with a request ID/method/path logged) attached to the Context.
-// This can be accessed via GetLogger(Context). The type of the logger is *log.Entry from github.com/Sirupsen/logrus
+// This can be accessed via GetLogger(Context).
 func MakeJSONAPI(handler JSONRequestHandler) http.HandlerFunc {
 	return Protect(func(w http.ResponseWriter, req *http.Request) {
 		reqID := RandomString(12)
-		// Set a Logger on the context
+		// Set a Logger and request ID on the context
 		ctx := context.WithValue(req.Context(), ctxValueLogger, log.WithFields(log.Fields{
 			"req.method": req.Method,
 			"req.path":   req.URL.Path,
@@ -97,75 +96,39 @@ func MakeJSONAPI(handler JSONRequestHandler) http.HandlerFunc {
 		logger := req.Context().Value(ctxValueLogger).(*log.Entry)
 		logger.Print("Incoming request")
 
-		res, httpErr := handler.OnIncomingRequest(req)
+		res := handler.OnIncomingRequest(req)
 
 		// Set common headers returned regardless of the outcome of the request
 		w.Header().Set("Content-Type", "application/json")
 		SetCORSHeaders(w)
 
-		if httpErr != nil {
-			jsonErrorResponse(w, req, httpErr)
-			return
-		}
-
-		// if they've returned bytes as the response, then just return them rather than marshalling as JSON.
-		// This gives handlers an escape hatch if they want to return cached bytes.
-		var resBytes []byte
-		resBytes, ok := res.([]byte)
-		if !ok {
-			r, err := json.Marshal(res)
-			if err != nil {
-				jsonErrorResponse(w, req, &HTTPError{
-					Message: "Failed to serialise response as JSON",
-					Code:    500,
-				})
-				return
-			}
-			resBytes = r
-		}
-		logger.Print(fmt.Sprintf("Responding (%d bytes)", len(resBytes)))
-		w.Write(resBytes)
+		respond(w, req, res)
 	})
 }
 
-func jsonErrorResponse(w http.ResponseWriter, req *http.Request, httpErr *HTTPError) {
+func respond(w http.ResponseWriter, req *http.Request, res JSONResponse) {
 	logger := req.Context().Value(ctxValueLogger).(*log.Entry)
-	if httpErr.Code == 302 {
-		logger.WithField("err", httpErr.Error()).Print("Redirecting")
-		http.Redirect(w, req, httpErr.Message, 302)
-		return
-	}
-	logger.WithFields(log.Fields{
-		log.ErrorKey: httpErr,
-	}).Print("Responding with error")
 
-	w.WriteHeader(httpErr.Code) // Set response code
-
-	var err error
-	var r []byte
-	if httpErr.JSON != nil {
-		r, err = json.Marshal(httpErr.JSON)
-		if err != nil {
-			// failed to marshal the supplied interface. Whine and fallback to the HTTP message.
-			logger.WithError(err).Error("Failed to marshal HTTPError.JSON")
+	// Set custom headers
+	if res.Headers != nil {
+		for h, val := range res.Headers {
+			w.Header().Set(h, val)
 		}
 	}
 
-	// failed to marshal or no custom JSON was supplied, send message JSON.
-	if err != nil || httpErr.JSON == nil {
-		r, err = json.Marshal(&JSONError{
-			Message: httpErr.Message,
-		})
+	// Marshal JSON response into raw bytes to send as the HTTP body
+	resBytes, err := json.Marshal(res.JSON)
+	if err != nil {
+		logger.WithError(err).Error("Failed to marshal JSONResponse")
+		// this should never fail to be marshalled so drop err to the floor
+		res = MessageResponse(500, "Internal Server Error")
+		resBytes, _ = json.Marshal(res.JSON)
 	}
 
-	if err != nil {
-		// We should never fail to marshal the JSON error response, but in this event just skip
-		// marshalling altogether
-		logger.Warn("Failed to marshal error response")
-		w.Write([]byte(`{}`))
-		return
-	}
-	w.Write(r)
+	// Set status code and write the body
+	w.WriteHeader(res.Code)
+	logger.WithField("code", res.Code).Infof("Responding (%d bytes)", len(resBytes))
+	w.Write(resBytes)
 }
 
 // WithCORSOptions intercepts all OPTIONS requests and responds with CORS headers. The request handler

vendor/src/github.com/matrix-org/util/json_test.go

@@ -2,6 +2,7 @@ package util
 
 import (
 	"context"
+	"errors"
 	"net/http"
 	"net/http/httptest"
 	"testing"
@@ -10,10 +11,10 @@ import (
 )
 
 type MockJSONRequestHandler struct {
-	handler func(req *http.Request) (interface{}, *HTTPError)
+	handler func(req *http.Request) JSONResponse
 }
 
-func (h *MockJSONRequestHandler) OnIncomingRequest(req *http.Request) (interface{}, *HTTPError) {
+func (h *MockJSONRequestHandler) OnIncomingRequest(req *http.Request) JSONResponse {
 	return h.handler(req)
 }
 
@@ -24,36 +25,27 @@ type MockResponse struct {
 func TestMakeJSONAPI(t *testing.T) {
 	log.SetLevel(log.PanicLevel) // suppress logs in test output
 	tests := []struct {
-		Return     interface{}
-		Err        *HTTPError
+		Return     JSONResponse
 		ExpectCode int
 		ExpectJSON string
 	}{
-		// Error message return values
-		{nil, &HTTPError{nil, "Everything is broken", 500, nil}, 500, `{"message":"Everything is broken"}`},
-		// Error JSON return values
-		{nil, &HTTPError{nil, "Everything is broken", 500, struct {
-			Foo string `json:"foo"`
-		}{"yep"}}, 500, `{"foo":"yep"}`},
+		// MessageResponse return values
+		{MessageResponse(500, "Everything is broken"), 500, `{"message":"Everything is broken"}`},
+		// interface return values
+		{JSONResponse{500, MockResponse{"yep"}, nil}, 500, `{"foo":"yep"}`},
 		// Error JSON return values which fail to be marshalled should fallback to text
-		{nil, &HTTPError{nil, "Everything is broken", 500, struct {
+		{JSONResponse{500, struct {
 			Foo interface{} `json:"foo"`
-		}{func(cannotBe, marshalled string) {}}}, 500, `{"message":"Everything is broken"}`},
+		}{func(cannotBe, marshalled string) {}}, nil}, 500, `{"message":"Internal Server Error"}`},
 		// With different status codes
-		{nil, &HTTPError{nil, "Not here", 404, nil}, 404, `{"message":"Not here"}`},
-		// Success return values
-		{&MockResponse{"yep"}, nil, 200, `{"foo":"yep"}`},
+		{JSONResponse{201, MockResponse{"narp"}, nil}, 201, `{"foo":"narp"}`},
 		// Top-level array success values
-		{[]MockResponse{{"yep"}, {"narp"}}, nil, 200, `[{"foo":"yep"},{"foo":"narp"}]`},
-		// raw []byte escape hatch
-		{[]byte(`actually bytes`), nil, 200, `actually bytes`},
-		// impossible marshal
-		{func(cannotBe, marshalled string) {}, nil, 500, `{"message":"Failed to serialise response as JSON"}`},
+		{JSONResponse{200, []MockResponse{{"yep"}, {"narp"}}, nil}, 200, `[{"foo":"yep"},{"foo":"narp"}]`},
 	}
 
 	for _, tst := range tests {
-		mock := MockJSONRequestHandler{func(req *http.Request) (interface{}, *HTTPError) {
-			return tst.Return, tst.Err
+		mock := MockJSONRequestHandler{func(req *http.Request) JSONResponse {
+			return tst.Return
 		}}
 		mockReq, _ := http.NewRequest("GET", "http://example.com/foo", nil)
 		mockWriter := httptest.NewRecorder()
@@ -69,10 +61,38 @@ func TestMakeJSONAPI(t *testing.T) {
 	}
 }
 
+func TestMakeJSONAPICustomHeaders(t *testing.T) {
+	mock := MockJSONRequestHandler{func(req *http.Request) JSONResponse {
+		headers := make(map[string]string)
+		headers["Custom"] = "Thing"
+		headers["X-Custom"] = "Things"
+		return JSONResponse{
+			Code:    200,
+			JSON:    MockResponse{"yep"},
+			Headers: headers,
+		}
+	}}
+	mockReq, _ := http.NewRequest("GET", "http://example.com/foo", nil)
+	mockWriter := httptest.NewRecorder()
+	handlerFunc := MakeJSONAPI(&mock)
+	handlerFunc(mockWriter, mockReq)
+	if mockWriter.Code != 200 {
+		t.Errorf("TestMakeJSONAPICustomHeaders wanted HTTP status 200, got %d", mockWriter.Code)
+	}
+	h := mockWriter.Header().Get("Custom")
+	if h != "Thing" {
+		t.Errorf("TestMakeJSONAPICustomHeaders wanted header 'Custom: Thing' , got 'Custom: %s'", h)
+	}
+	h = mockWriter.Header().Get("X-Custom")
+	if h != "Things" {
+		t.Errorf("TestMakeJSONAPICustomHeaders wanted header 'X-Custom: Things' , got 'X-Custom: %s'", h)
+	}
+}
+
 func TestMakeJSONAPIRedirect(t *testing.T) {
 	log.SetLevel(log.PanicLevel) // suppress logs in test output
-	mock := MockJSONRequestHandler{func(req *http.Request) (interface{}, *HTTPError) {
-		return nil, &HTTPError{nil, "https://matrix.org", 302, nil}
+	mock := MockJSONRequestHandler{func(req *http.Request) JSONResponse {
+		return RedirectResponse("https://matrix.org")
 	}}
 	mockReq, _ := http.NewRequest("GET", "http://example.com/foo", nil)
 	mockWriter := httptest.NewRecorder()
@@ -87,6 +107,50 @@ func TestMakeJSONAPIRedirect(t *testing.T) {
 	}
 }
 
+func TestMakeJSONAPIError(t *testing.T) {
+	log.SetLevel(log.PanicLevel) // suppress logs in test output
+	mock := MockJSONRequestHandler{func(req *http.Request) JSONResponse {
+		err := errors.New("oops")
+		return ErrorResponse(err)
+	}}
+	mockReq, _ := http.NewRequest("GET", "http://example.com/foo", nil)
+	mockWriter := httptest.NewRecorder()
+	handlerFunc := MakeJSONAPI(&mock)
+	handlerFunc(mockWriter, mockReq)
+	if mockWriter.Code != 500 {
+		t.Errorf("TestMakeJSONAPIError wanted HTTP status 500, got %d", mockWriter.Code)
+	}
+	actualBody := mockWriter.Body.String()
+	expect := `{"message":"oops"}`
+	if actualBody != expect {
+		t.Errorf("TestMakeJSONAPIError wanted body '%s', got '%s'", expect, actualBody)
+	}
+}
+
+func TestIs2xx(t *testing.T) {
+	tests := []struct {
+		Code   int
+		Expect bool
+	}{
+		{200, true},
+		{201, true},
+		{299, true},
+		{300, false},
+		{199, false},
+		{0, false},
+		{500, false},
+	}
+	for _, test := range tests {
+		j := JSONResponse{
+			Code: test.Code,
+		}
+		actual := j.Is2xx()
+		if actual != test.Expect {
+			t.Errorf("TestIs2xx wanted %t, got %t", test.Expect, actual)
+		}
+	}
+}
+
 func TestGetLogger(t *testing.T) {
 	log.SetLevel(log.PanicLevel) // suppress logs in test output
 	entry := log.WithField("test", "yep")
@@ -130,6 +194,32 @@ func TestProtect(t *testing.T) {
 	}
 }
 
+func TestWithCORSOptions(t *testing.T) {
+	log.SetLevel(log.PanicLevel) // suppress logs in test output
+	mockWriter := httptest.NewRecorder()
+	mockReq, _ := http.NewRequest("OPTIONS", "http://example.com/foo", nil)
+	h := WithCORSOptions(func(w http.ResponseWriter, req *http.Request) {
+		w.WriteHeader(200)
+		w.Write([]byte("yep"))
+	})
+	h(mockWriter, mockReq)
+	if mockWriter.Code != 200 {
+		t.Errorf("TestWithCORSOptions wanted HTTP status 200, got %d", mockWriter.Code)
+	}
+
+	origin := mockWriter.Header().Get("Access-Control-Allow-Origin")
+	if origin != "*" {
+		t.Errorf("TestWithCORSOptions wanted Access-Control-Allow-Origin header '*', got '%s'", origin)
+	}
+
+	// OPTIONS request shouldn't hit the handler func
+	expectBody := ""
+	actualBody := mockWriter.Body.String()
+	if actualBody != expectBody {
+		t.Errorf("TestWithCORSOptions wanted body %s, got %s", expectBody, actualBody)
+	}
+}
+
 func TestGetRequestID(t *testing.T) {
 	log.SetLevel(log.PanicLevel) // suppress logs in test output
 	reqID := "alphabetsoup"