Merge pull request #89 from netlify/handleErrorFix

 HealthCheck middleware only handles non-nil errors

Author: smoya

router/errors.go

@@ -85,6 +85,10 @@ func httpError(code int, fmtString string, args ...interface{}) *HTTPError {
 
 // HandleError will handle an error
 func HandleError(err error, w http.ResponseWriter, r *http.Request) {
+	if err == nil {
+		return
+	}
+
 	log := tracing.GetLogger(r)
 	errorID := tracing.RequestID(r)
 	switch e := err.(type) {

router/errors_test.go

@@ -0,0 +1,76 @@
+package router
+
+import (
+	"errors"
+	"fmt"
+	"github.com/netlify/netlify-commons/tracing"
+	"github.com/sirupsen/logrus/hooks/test"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"io/ioutil"
+	"net/http"
+	"net/http/httptest"
+	"testing"
+)
+
+func TestHandleError_ErrorIsNil(t *testing.T) {
+	logger, loggerOutput := test.NewNullLogger()
+	w, r, _ := tracing.NewTracer(
+		httptest.NewRecorder(),
+		httptest.NewRequest(http.MethodGet, "/", nil),
+		logger,
+		"test",
+	)
+
+	HandleError(nil, w, r)
+
+	assert.Empty(t, loggerOutput.AllEntries())
+	assert.Empty(t, w.Header())
+}
+
+func TestHandleError_StandardError(t *testing.T) {
+	logger, loggerOutput := test.NewNullLogger()
+	w, r, _ := tracing.NewTracer(
+		httptest.NewRecorder(),
+		httptest.NewRequest(http.MethodGet, "/", nil),
+		logger,
+		"test",
+	)
+
+	HandleError(errors.New("random error"), w, r)
+
+	require.Len(t,  loggerOutput.AllEntries(), 1)
+	assert.Equal(t, "Unhandled server error: random error", loggerOutput.AllEntries()[0].Message)
+	assert.Empty(t, w.Header())
+}
+
+func TestHandleError_HTTPError(t *testing.T) {
+	logger, loggerOutput := test.NewNullLogger()
+	recorder := httptest.NewRecorder()
+	w, r, _ := tracing.NewTracer(
+		recorder,
+		httptest.NewRequest(http.MethodGet, "/", nil),
+		logger,
+		"test",
+	)
+
+	httpErr := &HTTPError{
+		Code: http.StatusInternalServerError,
+		Message: http.StatusText(http.StatusInternalServerError),
+		InternalError: errors.New("random error"),
+		InternalMessage: "Something unexpected happened",
+	}
+
+	HandleError(httpErr, w, r)
+
+	resp := recorder.Result()
+	b, err := ioutil.ReadAll(resp.Body)
+	require.NoError(t, err)
+
+	expectedBody := fmt.Sprintf(`{"code":500,"msg":"Internal Server Error","json":null,"error_id":"%s"}`, tracing.RequestID(r))
+	assert.Equal(t, expectedBody, string(b))
+
+	require.Len(t,  loggerOutput.AllEntries(), 1)
+	assert.Equal(t, httpErr.InternalMessage, loggerOutput.AllEntries()[0].Message)
+}
+

router/middleware.go

@@ -87,7 +87,9 @@ func HealthCheck(route string, f APIHandler) Middleware {
 				w.WriteHeader(http.StatusOK)
 				return
 			}
-			HandleError(f(w, r), w, r)
+			if err := f(w, r); err != nil {
+				HandleError(f(w, r), w, r)
+			}
 			return
 		}
 		next.ServeHTTP(w, r)