WrapMiddleware possible unexpected behavior

[jmcnevin]

Issue Description

Hey all. Thank you for this project and all the great work on it.

I was recently surprised by some behavior in my app when using WrapMiddleware where my wrapped middleware was not able to use httpsnoop to retrieve the actual response code for an error response for reporting some metrics.

I suspect this has something to do with how echo may set response codes for errors late, based on where echo.HttpError errors are handled.

My question is: Is there a reliable way for wrapped middleware functions to retrieve response codes from error responses, or should WrapMiddleware be documented with some sort of disclaimer about this behavior?

Checklist

• [x] Dependencies installed
• [x] No typos
• [x] Searched existing issues and docs

Expected behaviour

Wrapped middleware functions should be able to add hooks to a custom http.ResponseWriter that will capture the setting of error response codes.

Actual behaviour

It seems that error codes may not be set by the time wrapped middlewares are executing.

Steps to reproduce

Working code to debug

// Wrapped middleware never seems to get proper error response codes for m.Code
func (mw *Middleware) MiddlewareFunc(next http.Handler) http.Handler {
	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		m = httpsnoop.CaptureMetrics(next, w, r)
		metrics.RecordRequest(r, r.URL.Path, startTime, m.Code, int(m.Written))
	})
}

Version/commit

4.3.0

[aldas]

Why do you need to use httpsnoop? You can get same data without using it.

See how logging middleware is able to get same/similar data with Echo https://github.com/labstack/echo/blob/f20820c0030a0d8c8aa20f63996092faa329fe03/middleware/logger.go#L114

but you could do it like that:

	e.Use(func(next echo.HandlerFunc) echo.HandlerFunc {
		return func(c echo.Context) error {
			req := c.Request()
			res := c.Response()

			start := time.Now()
			err := next(c)
			duration := time.Since(start)

			code := res.Status
			if err != nil { 
				// in case error is returned by middleware/handler return value chain. Usually at this point is res.status = http.StatusOK
				// so we need to get status code from error
				code = http.StatusInternalServerError
				var errHTTP *echo.HTTPError
				if errors.As(err, &errHTTP) {
					code = errHTTP.Code
				}
			}

			//metrics.RecordRequest(req, req.URL.Path, start, code, res.Size)
			return err
		}
	})

update: nevermind. I skipped the part where WrapMiddleware was mentioned.

[aldas]

you probably should mention what metrics library are you trying to wrap. It would make probably more sense to convert that part to Echo middleware (assuming it is not huge amount of code). I mean https://github.com/labstack/echo-contrib has middleware for prometheus.

[jmcnevin]

Our team is using our own metrics library that we share across a number of applications, some of which are not echo-based servers.

That said, yeah, not a big deal to write custom echo middleware to get it done, and in fact we've done just that. I'm just trying to determine if, in general, there are some scenarios where usage of WrapMiddleware should be avoided.

[aldas]

This is probably what you search but it really feels like pushing square peg through round hole.

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

	var sizeFromMW int64
	var statusFromMW int
	e.Use(echo.WrapMiddleware(func(h http.Handler) http.Handler {
		return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
			h.ServeHTTP(w, r)

			res, ok := w.(*echo.Response)
			if !ok {
				return
			}

			statusFromMW = res.Status
			sizeFromMW = res.Size
		})
	}))

	e.GET("/", func(c echo.Context) error {
		return c.String(http.StatusTeapot, "OK")
	})

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

	e.ServeHTTP(rec, req)

	assert.Equal(t, "OK", rec.Body.String())
	assert.Equal(t, http.StatusTeapot, rec.Code)

	assert.Equal(t, http.StatusTeapot, statusFromMW)
	assert.Equal(t, int64(2), sizeFromMW)
}