From d69167fe47d7a5dc6be6e914d655fd725e1b7f49 Mon Sep 17 00:00:00 2001
From: Mike Mason <mimason@equinix.com>
Date: Fri, 13 Sep 2024 10:14:25 -0500
Subject: [PATCH] properly handle echo http wrapped errors (#288)

Errors may be wrapped by a specific echo http error by a handler.
In some cases (such as canceled context) this might report as a 500
error, however this error might've been triggered by the requestor.

To simplify handling this scenario, we will unwrap the http error,
preserving the original message only if the error is a context canceled
error and the request context has been canceled.

Signed-off-by: Mike Mason <mimason@equinix.com>
---
 internal/api/router.go      | 25 ++++++++++-----
 internal/api/router_test.go | 64 ++++++++++++++++++++++++++++++++++++-
 2 files changed, 80 insertions(+), 9 deletions(-)

diff --git a/internal/api/router.go b/internal/api/router.go
index 63ed681e..c2242800 100644
--- a/internal/api/router.go
+++ b/internal/api/router.go
@@ -98,22 +98,31 @@ func (r *Router) Routes(rg *echo.Group) {
 
 func errorMiddleware(next echo.HandlerFunc) echo.HandlerFunc {
 	return func(c echo.Context) error {
-		err := next(c)
+		origErr := next(c)
 
-		if err == nil {
+		if origErr == nil {
 			return nil
 		}
 
-		// If error is an echo.HTTPError simply return it
-		if _, ok := err.(*echo.HTTPError); ok {
-			return err
+		var (
+			checkErr = origErr
+			echoMsg  []any
+		)
+
+		// If error is an echo.HTTPError, extract it's message to be reused if status is rewritten.
+		// Additionally we unwrap the internal error which is then checked instead of the echo error.
+		if eerr, ok := origErr.(*echo.HTTPError); ok {
+			echoMsg = []any{eerr.Message}
+			checkErr = eerr.Internal
 		}
 
 		switch {
-		case errors.Is(err, context.Canceled):
-			return echo.ErrUnprocessableEntity.WithInternal(err)
+		// Only if the error is a context canceled error and the request context has been canceled.
+		// If the request was not canceled, then the context canceled error probably came from the service.
+		case errors.Is(checkErr, context.Canceled) && errors.Is(c.Request().Context().Err(), context.Canceled):
+			return echo.NewHTTPError(http.StatusUnprocessableEntity, echoMsg...).WithInternal(checkErr)
 		default:
-			return err
+			return origErr
 		}
 	}
 }
diff --git a/internal/api/router_test.go b/internal/api/router_test.go
index cd8366e2..2a296b03 100644
--- a/internal/api/router_test.go
+++ b/internal/api/router_test.go
@@ -2,6 +2,7 @@ package api
 
 import (
 	"context"
+	"encoding/json"
 	"io"
 	"net/http"
 	"net/http/httptest"
@@ -19,6 +20,8 @@ func TestErrorMiddleware(t *testing.T) {
 	ctx := context.Background()
 
 	e := echo.New()
+	e.Debug = true
+
 	e.Use(echoTestLogger(t, e))
 	e.Use(errorMiddleware)
 
@@ -27,7 +30,15 @@ func TestErrorMiddleware(t *testing.T) {
 
 		select {
 		case <-c.Request().Context().Done():
-			return c.Request().Context().Err()
+			err := c.Request().Context().Err()
+
+			switch errType {
+			case "":
+			case "echo":
+				return echo.NewHTTPError(http.StatusInternalServerError, "some message").WithInternal(err)
+			}
+
+			return err
 		case <-time.After(time.Second):
 		}
 
@@ -37,6 +48,8 @@ func TestErrorMiddleware(t *testing.T) {
 			return echo.ErrTeapot
 		case "other":
 			return io.ErrUnexpectedEOF
+		case "internalCancel":
+			return echo.NewHTTPError(http.StatusInternalServerError, "service error").WithInternal(context.Canceled)
 		}
 
 		return nil
@@ -97,6 +110,55 @@ func TestErrorMiddleware(t *testing.T) {
 				assert.Equal(t, http.StatusUnprocessableEntity, res.Success.Code)
 			},
 		},
+		{
+			Name: "Canceled echo",
+			Input: testinput{
+				path:  "/test?error=echo",
+				delay: time.Second / 2,
+			},
+			CheckFn: func(_ context.Context, t *testing.T, res testingx.TestResult[*httptest.ResponseRecorder]) {
+				require.NoError(t, res.Err)
+				require.NotNil(t, res.Success)
+
+				require.Equal(t, http.StatusUnprocessableEntity, res.Success.Code)
+
+				resp := map[string]string{}
+
+				err := json.Unmarshal(res.Success.Body.Bytes(), &resp)
+				require.NoError(t, err, "no error expected decoding response body")
+
+				expect := map[string]string{
+					"error":   "code=422, message=some message, internal=context canceled",
+					"message": "some message",
+				}
+
+				assert.Equal(t, expect, resp, "unexpected response")
+			},
+		},
+		{
+			Name: "Canceled echo internal",
+			Input: testinput{
+				path: "/test?error=internalCancel",
+			},
+			CheckFn: func(_ context.Context, t *testing.T, res testingx.TestResult[*httptest.ResponseRecorder]) {
+				require.NoError(t, res.Err)
+				require.NotNil(t, res.Success)
+
+				require.Equal(t, http.StatusInternalServerError, res.Success.Code)
+
+				resp := map[string]string{}
+
+				err := json.Unmarshal(res.Success.Body.Bytes(), &resp)
+				require.NoError(t, err, "no error expected decoding response body")
+
+				expect := map[string]string{
+					"error":   "code=500, message=service error, internal=context canceled",
+					"message": "service error",
+				}
+
+				assert.Equal(t, expect, resp, "unexpected response")
+			},
+		},
 	}
 
 	testFn := func(ctx context.Context, input testinput) testingx.TestResult[*httptest.ResponseRecorder] {