Merge pull request #112 from netlify/fix/routerZeroValueError

fix(router): early return when zero vallue of err in HandleError

Author: smoya

router/errors.go

@@ -3,6 +3,7 @@ package router
 import (
 	"fmt"
 	"net/http"
+	"unsafe"
 
 	"github.com/netlify/netlify-commons/tracing"
 )
@@ -85,7 +86,8 @@ 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 {
+	// if err is nil or nil pointer of type *HTTPError
+	if err == nil || (*[2]uintptr)(unsafe.Pointer(&err))[1] == 0 {
 		return
 	}
 

router/errors_test.go

@@ -3,14 +3,15 @@ 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"
+
+	"github.com/netlify/netlify-commons/tracing"
+	"github.com/sirupsen/logrus/hooks/test"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 func TestHandleError_ErrorIsNil(t *testing.T) {
@@ -28,6 +29,25 @@ func TestHandleError_ErrorIsNil(t *testing.T) {
 	assert.Empty(t, w.Header())
 }
 
+func TestHandleError_ErrorIsNilPointerToTypeHTTPError(t *testing.T) {
+	logger, loggerOutput := test.NewNullLogger()
+	w, r, _ := tracing.NewTracer(
+		httptest.NewRecorder(),
+		httptest.NewRequest(http.MethodGet, "/", nil),
+		logger,
+		"test",
+	)
+
+	h := func(_ http.ResponseWriter, _ *http.Request) *HTTPError {
+		return nil
+	}
+
+	HandleError(h(w, r), 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(
@@ -39,7 +59,7 @@ func TestHandleError_StandardError(t *testing.T) {
 
 	HandleError(errors.New("random error"), w, r)
 
-	require.Len(t,  loggerOutput.AllEntries(), 1)
+	require.Len(t, loggerOutput.AllEntries(), 1)
 	assert.Equal(t, "Unhandled server error: random error", loggerOutput.AllEntries()[0].Message)
 	assert.Empty(t, w.Header())
 }
@@ -55,9 +75,9 @@ func TestHandleError_HTTPError(t *testing.T) {
 	)
 
 	httpErr := &HTTPError{
-		Code: http.StatusInternalServerError,
-		Message: http.StatusText(http.StatusInternalServerError),
-		InternalError: errors.New("random error"),
+		Code:            http.StatusInternalServerError,
+		Message:         http.StatusText(http.StatusInternalServerError),
+		InternalError:   errors.New("random error"),
 		InternalMessage: "Something unexpected happened",
 	}
 
@@ -70,7 +90,6 @@ func TestHandleError_HTTPError(t *testing.T) {
 	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)
+	require.Len(t, loggerOutput.AllEntries(), 1)
 	assert.Equal(t, httpErr.InternalMessage, loggerOutput.AllEntries()[0].Message)
 }
-

router/middleware.go

@@ -90,7 +90,11 @@ 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(err, w, r)
+			}
+
 			return
 		}
 		next.ServeHTTP(w, r)