Repository: labstack/echo
Pull request #2072 (2022-01-17 15:14:22)

Add LogLevelSetter to recover middleware
I've come across the situation when I need to distinguish log levels for different recovered values. 

For example, I want to handle all panics as ERRORs except situations when I've got `panic(ErrAbortHandler)`. It may happen in the `ProxyWithConfig` middleware when the client aborts the request.

For now, we have the `LogLevel` and `LogLevelSetter` both in the `RecoverConfig` to not break compatibility and introduce a new feature to the recover middleware.

**Usage**:

```go
config := DefaultRecoverConfig
config.LogLevelSetter = func(value interface{}) log.Lvl {
  if err, ok := value.(error); ok {
    if errors.Is(err, http.ErrAbortHandler) {
      return log.WARN
    }
  }
  return log.ERROR
}
```

aldas: maybe adding something like that to config `LogErrorFunc func(err error, stack []byte) error`

and allowing completely custom handling with:
```go
			defer func() {
				if r := recover(); r != nil {
					err, ok := r.(error)
					if !ok {
						err = fmt.Errorf("%v", r)
					}
					var stack []byte
					var length int

					if !config.DisablePrintStack {
						stack = make([]byte, config.StackSize)
						length = runtime.Stack(stack, !config.DisableStackAll)
						stack = stack[:length]
					}

					if config.LogErrorFunc != nil {
						err = config.LogErrorFunc(err, stack)
					} else if !config.DisablePrintStack {
						msg := fmt.Sprintf("[PANIC RECOVER] %v %s\n", err, stack)
						switch config.LogLevel {
						case log.DEBUG:
...
...
```

it could maybe even made to return an error instead of resulting to `c.Error(err)` which stops handler chain returning errors upstream middlewares.
aldas: Although recover middleware is extremely simple middleware it has uses when you want to log something with stack. If you are not interested in stack there absolutely no need to use Echo provided middleware as it your custom one is vert easy to create.  If you want to log stack I think it would be better to give users means to choose how logging is done. This is more flexible as to give them ability to choose only log level.
aldas: This is example for handling recovered errors - lets say more correctly

```go
func main() {
	e := echo.New()

	e.Use(middleware.Logger())

	customRecovery := func(next echo.HandlerFunc) echo.HandlerFunc {
		return func(c echo.Context) (err error) {
			defer func() {
				if r := recover(); r != nil {
					rErr, ok := r.(error)
					if !ok {
						rErr = fmt.Errorf("%v", r)
					}
					// c.Logger().Error(rErr)  // you do not need to log here. Logger middleware is up in chain.
					err = rErr // allows upstream middlewares to receive recovered error
				}
			}()
			return next(c)
		}
	}
	e.Use(customRecovery)

	e.GET("/", func(c echo.Context) error {
		panic(errors.New("test"))
	})

	log.Fatal(e.Start(":8080"))
}
```
ant1k9: Thanks for your suggestions!

>  This is more flexible as to give them ability to choose only log level.

I like your idea of a more general function in the `RecoverConfig`. It solves our problem!

> Although recover middleware is extremely simple middleware it has uses when you want to log something with stack. If you are not interested in stack there absolutely no need to use Echo provided middleware as it your custom one is vert easy to create.

We use stack traces because they provide more information for identifying the core of the problem, so why I propose to extend the config. For now, we create a copy of the current middleware for our purposes.

> This is example for handling recovered errors - lets say more correctly

You're right, it's not too elaborate to create a custom middleware, but I search the compromise between adding some customization for the service and finding a way to customize an existing solution in the `echo` repo.

Patch:
diff --git a/README.md b/README.md
index 20d662c..8b2321f 100644
--- a/README.md
+++ b/README.md
@@ -101,6 +101,7 @@ func hello(c echo.Context) error {
 | [github.com/ziflex/lecho](https://github.com/ziflex/lecho) | [Zerolog](https://github.com/rs/zerolog) logging library wrapper for Echo logger interface.                                                                                                                                                                                                                                                                    |
 | [github.com/brpaz/echozap](https://github.com/brpaz/echozap) | Uber´s [Zap](https://github.com/uber-go/zap) logging library wrapper for Echo logger interface.                                                                                                                                                                                                                                                              |
 | [github.com/darkweak/souin/plugins/echo](https://github.com/darkweak/souin/tree/master/plugins/echo) | HTTP cache system based on [Souin](https://github.com/darkweak/souin) to automatically get your endpoints cached. It supports some distributed and non-distributed storage systems depending your needs.                                                                                                             |
+| [github.com/mikestefanello/pagoda](https://github.com/mikestefanello/pagoda) | Rapid, easy full-stack web development starter kit built with Echo.
 
 Please send a PR to add your own library here.
 
diff --git a/bind.go b/bind.go
index fdf0524..c841ca0 100644
--- a/bind.go
+++ b/bind.go
@@ -111,11 +111,11 @@ func (b *DefaultBinder) Bind(i interface{}, c Context) (err error) {
 	if err := b.BindPathParams(c, i); err != nil {
 		return err
 	}
-	// Issue #1670 - Query params are binded only for GET/DELETE and NOT for usual request with body (POST/PUT/PATCH)
-	// Reasoning here is that parameters in query and bind destination struct could have UNEXPECTED matches and results due that.
-	// i.e. is `&id=1&lang=en` from URL same as `{"id":100,"lang":"de"}` request body and which one should have priority when binding.
-	// This HTTP method check restores pre v4.1.11 behavior and avoids different problems when query is mixed with body
-	if c.Request().Method == http.MethodGet || c.Request().Method == http.MethodDelete {
+	// Only bind query parameters for GET/DELETE/HEAD to avoid unexpected behavior with destination struct binding from body.
+	// For example a request URL `&id=1&lang=en` with body `{"id":100,"lang":"de"}` would lead to precedence issues.
+	// The HTTP method check restores pre-v4.1.11 behavior to avoid these problems (see issue #1670)
+  method := c.Request().Method
+	if method == http.MethodGet || method == http.MethodDelete || method == http.MethodHead {
 		if err = b.BindQueryParams(c, i); err != nil {
 			return err
 		}
diff --git a/echo.go b/echo.go
index 9ae2ed4..d067b89 100644
--- a/echo.go
+++ b/echo.go
@@ -207,6 +207,7 @@ const (
 	HeaderIfModifiedSince     = "If-Modified-Since"
 	HeaderLastModified        = "Last-Modified"
 	HeaderLocation            = "Location"
+	HeaderRetryAfter          = "Retry-After"
 	HeaderUpgrade             = "Upgrade"
 	HeaderVary                = "Vary"
 	HeaderWWWAuthenticate     = "WWW-Authenticate"
@@ -653,7 +654,7 @@ func (e *Echo) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 	// Acquire context
 	c := e.pool.Get().(*context)
 	c.Reset(r, w)
-	h := NotFoundHandler
+	var h func(Context) error
 
 	if e.premiddleware == nil {
 		e.findRouter(r.Host).Find(r.Method, GetPath(r), c)
diff --git a/middleware/recover.go b/middleware/recover.go
index 0dbe740..a621a9e 100644
--- a/middleware/recover.go
+++ b/middleware/recover.go
@@ -9,6 +9,9 @@ import (
 )
 
 type (
+	// LogErrorFunc defines a function for custom logging in the middleware.
+	LogErrorFunc func(c echo.Context, err error, stack []byte) error
+
 	// RecoverConfig defines the config for Recover middleware.
 	RecoverConfig struct {
 		// Skipper defines a function to skip middleware.
@@ -30,6 +33,10 @@ type (
 		// LogLevel is log level to printing stack trace.
 		// Optional. Default value 0 (Print).
 		LogLevel log.Lvl
+
+		// LogErrorFunc defines a function for custom logging in the middleware.
+		// If it's set you don't need to provide LogLevel for config.
+		LogErrorFunc LogErrorFunc
 	}
 )
 
@@ -41,6 +48,7 @@ var (
 		DisableStackAll:   false,
 		DisablePrintStack: false,
 		LogLevel:          0,
+		LogErrorFunc:      nil,
 	}
 )
 
@@ -73,9 +81,18 @@ func RecoverWithConfig(config RecoverConfig) echo.MiddlewareFunc {
 					if !ok {
 						err = fmt.Errorf("%v", r)
 					}
-					stack := make([]byte, config.StackSize)
-					length := runtime.Stack(stack, !config.DisableStackAll)
+					var stack []byte
+					var length int
+
 					if !config.DisablePrintStack {
+						stack = make([]byte, config.StackSize)
+						length = runtime.Stack(stack, !config.DisableStackAll)
+						stack = stack[:length]
+					}
+
+					if config.LogErrorFunc != nil {
+						err = config.LogErrorFunc(c, err, stack)
+					} else if !config.DisablePrintStack {
 						msg := fmt.Sprintf("[PANIC RECOVER] %v %s\n", err, stack[:length])
 						switch config.LogLevel {
 						case log.DEBUG:
diff --git a/router.go b/router.go
index 1a2ce56..a1de2d6 100644
--- a/router.go
+++ b/router.go
@@ -624,6 +624,4 @@ func (r *Router) Find(method, path string, c Context) {
 	}
 	ctx.path = currentNode.ppath
 	ctx.pnames = currentNode.pnames
-
-	return
 }


Test patch:
diff --git a/middleware/recover_test.go b/middleware/recover_test.go
index 6443329..9ac4fee 100644
--- a/middleware/recover_test.go
+++ b/middleware/recover_test.go
@@ -2,6 +2,7 @@ package middleware
 
 import (
 	"bytes"
+	"errors"
 	"fmt"
 	"net/http"
 	"net/http/httptest"
@@ -81,3 +82,55 @@ func TestRecoverWithConfig_LogLevel(t *testing.T) {
 		})
 	}
 }
+
+func TestRecoverWithConfig_LogErrorFunc(t *testing.T) {
+	e := echo.New()
+	e.Logger.SetLevel(log.DEBUG)
+
+	buf := new(bytes.Buffer)
+	e.Logger.SetOutput(buf)
+
+	req := httptest.NewRequest(http.MethodGet, "/", nil)
+	rec := httptest.NewRecorder()
+	c := e.NewContext(req, rec)
+
+	testError := errors.New("test")
+	config := DefaultRecoverConfig
+	config.LogErrorFunc = func(c echo.Context, err error, stack []byte) error {
+		msg := fmt.Sprintf("[PANIC RECOVER] %v %s\n", err, stack)
+		if errors.Is(err, testError) {
+			c.Logger().Debug(msg)
+		} else {
+			c.Logger().Error(msg)
+		}
+		return err
+	}
+
+	t.Run("first branch case for LogErrorFunc", func(t *testing.T) {
+		buf.Reset()
+		h := RecoverWithConfig(config)(echo.HandlerFunc(func(c echo.Context) error {
+			panic(testError)
+		}))
+
+		h(c)
+		assert.Equal(t, http.StatusInternalServerError, rec.Code)
+
+		output := buf.String()
+		assert.Contains(t, output, "PANIC RECOVER")
+		assert.Contains(t, output, `"level":"DEBUG"`)
+	})
+
+	t.Run("else branch case for LogErrorFunc", func(t *testing.T) {
+		buf.Reset()
+		h := RecoverWithConfig(config)(echo.HandlerFunc(func(c echo.Context) error {
+			panic("other")
+		}))
+
+		h(c)
+		assert.Equal(t, http.StatusInternalServerError, rec.Code)
+
+		output := buf.String()
+		assert.Contains(t, output, "PANIC RECOVER")
+		assert.Contains(t, output, `"level":"ERROR"`)
+	})
+}