HTTPErrorHandler is ignored if a WrapMiddleware writes the response (red test and fix included)

[pnmcosta]

Issue Description

HTTPErrorHandler is ignored if a WrapMiddleware writes the response (red test and fix included)

Checklist

• Dependencies installed
• No typos
• Searched existing issues and docs

Expected behaviour

HTTPErrorHandler should be respected if a WrapMiddleware writes the response.

Actual behaviour

Custom error handling is not respected if a WrapMiddleware writes the response instead, the final response is always 200 with an empty body instead of whatever might have been set with HTTPErrorHandler.

Steps to reproduce

The below is a simplified red test

package main

import (
	"errors"
	"net/http"
	"net/http/httptest"
	"strings"
	"testing"

	"github.com/labstack/echo/v5"
)

type ApiError struct {
	Code    int    `json:"code"`
	Message string `json:"message"`
}

// Error makes it compatible with the `error` interface.
func (e *ApiError) Error() string {
	return e.Message
}

func TestWrapMiddleware(t *testing.T) {
	e := echo.New()

	e.HTTPErrorHandler = func(c echo.Context, err error) {
		if err == nil {
			return // no error
		}

		if c.Response().Committed {
			return
		}

		var apiErr *ApiError
		if errors.As(err, &apiErr) {
			c.JSON(apiErr.Code, apiErr)
		}
	}

	e.Use(
		// simulate a middleware that would cache the response, but still write it
		echo.WrapMiddleware(func(next http.Handler) http.Handler {
			return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
				rec := httptest.NewRecorder()
				next.ServeHTTP(rec, r)
				result := rec.Result()

				statusCode := result.StatusCode
				value := rec.Body.Bytes()
				// TODO: cache statusCode/value

				for k, v := range result.Header {
					w.Header().Set(k, strings.Join(v, ","))
				}
				w.WriteHeader(statusCode)
				w.Write(value)
			})
		}),
	)

	e.GET("/", func(c echo.Context) error {
		return &ApiError{
			Code:    http.StatusBadRequest,
			Message: "invalid request",
		}
	})

	req := httptest.NewRequest(http.MethodGet, "/", nil)
	rec := httptest.NewRecorder()
	e.ServeHTTP(rec, req)

	if rec.Code != http.StatusBadRequest {
		t.Errorf("Expected code to be %v, got %v", http.StatusBadRequest, rec.Code)
	}

	body := rec.Body.String()
	if !strings.Contains(body, `"message":"invalid request"`) {
		t.Errorf("Expected body to contain %v, got %v", `"message":"invalid request"`, body)
	}
}

Working code to debug

Replace echo.WrapMiddleware on the example above with the overload below:

func WrapMiddleware(m func(http.Handler) http.Handler) echo.MiddlewareFunc {
	return func(next echo.HandlerFunc) echo.HandlerFunc {
		return func(c echo.Context) (err error) {
			m(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
				c.SetRequest(r)
				c.SetResponse(echo.NewResponse(w, c.Echo()))
				err = next(c)
				if err != nil {
					c.Echo().HTTPErrorHandler(c, err)
				}
			})).ServeHTTP(c.Response(), c.Request())
			return
		}
	}
}

Version/commit

github.com/labstack/echo/v5 v5.0.0-20230722203903-ec5b858dab61

[aldas]

You really should not use v5. Anyway, WrapMiddleware behaves the same in v4.

I think this is misunderstanding how Echo middlewares are executed and how errors are "bubbled up" in middleware chain
.
So currently you are forcing errors to be commited with this line

				if err != nil {
					c.Echo().HTTPErrorHandler(c, err)
				}

which means any middleware before WrapMiddleware can no longer decide to "swallow" the error and allow global error handler to send their error as response.

and your wrapped middleware is doing error handling as w.WriteHeader(statusCode) which for Echo is not error handling. This is OK answer with http status that happens to be error code.

It is not WrapMiddleware responsibility, and should not be, to commit errors . By commit I mean forcefully send them to the client.

[aldas]

Similarly

	e.GET("/", func(c Context) error {
		return c.JSON(http.StatusBadRequest, "nope")
	})

will not trigger error handling as it not an error.

[pnmcosta]

Hi @aldas

Thanks for getting back to me, you're probably right, I don't fully understand Echo and how it works.

However I think the use case is valid, I'm simplified it further to explain to you:

func TestWrapMiddleware(t *testing.T) {
	e := echo.New()

	e.Use(
		// simulate a middleware that reads the response
		echo.WrapMiddleware(func(next http.Handler) http.Handler {
			return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
				rec := httptest.NewRecorder()
				next.ServeHTTP(rec, r)
				rec.Result()
			})
		}),
	)

	e.GET("/", func(c echo.Context) error {
		return echo.ErrBadRequest
	})

	req := httptest.NewRequest(http.MethodGet, "/", nil)
	rec := httptest.NewRecorder()
	e.ServeHTTP(rec, req)

	if rec.Code != http.StatusBadRequest {
		t.Errorf("Expected code to be %v, got %v", http.StatusBadRequest, rec.Code)
	}

	body := rec.Body.String()
	if !strings.Contains(body, `"message":"Bad Request"`) {
		t.Errorf("Expected body to contain %v, got %v", `"message":"Bad Request"`, body)
	}
}

Any suggestion how best to handle this user case? Preferably without having to refactor the http middleware.

[pnmcosta]

Also could you ellaborate on:

You really should not use v5.

I'm using Echo as dependency of a framework and it was not my decision to use v5 or Echo, so any additional info it would be great.