From 5f4d90a2834d0972e24564f94daef86627bd064f Mon Sep 17 00:00:00 2001 From: Omkar Chorghe Date: Thu, 23 Feb 2023 21:07:59 +0530 Subject: [PATCH 1/5] Added a config variable to disable centralized error handler in recovery middleware --- middleware/recover.go | 10 +++++++++- middleware/recover_test.go | 29 +++++++++++++++++++++++++++++ 2 files changed, 38 insertions(+), 1 deletion(-) diff --git a/middleware/recover.go b/middleware/recover.go index 7b6128533..827b59a0b 100644 --- a/middleware/recover.go +++ b/middleware/recover.go @@ -38,6 +38,10 @@ type ( // 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 + + // DisableErrorHandler disables the centralized HTTPErrorHandler. + // Optional. Default value false. + DisableErrorHandler bool `yaml:"disable_error_handler"` } ) @@ -50,6 +54,7 @@ var ( DisablePrintStack: false, LogLevel: 0, LogErrorFunc: nil, + DisableErrorHandler: false, } ) @@ -113,7 +118,10 @@ func RecoverWithConfig(config RecoverConfig) echo.MiddlewareFunc { c.Logger().Print(msg) } } - c.Error(err) + + if(!config.DisableErrorHandler) { + c.Error(err) + } } }() return next(c) diff --git a/middleware/recover_test.go b/middleware/recover_test.go index b27f3b41c..29b6f26f6 100644 --- a/middleware/recover_test.go +++ b/middleware/recover_test.go @@ -163,3 +163,32 @@ func TestRecoverWithConfig_LogErrorFunc(t *testing.T) { assert.Contains(t, output, `"level":"ERROR"`) }) } + +func TestRecoverWithDisabled_ErrorHandler(t *testing.T) { + e := echo.New() + buf := new(bytes.Buffer) + e.Logger.SetOutput(buf) + req := httptest.NewRequest(http.MethodGet, "/", nil) + rec := httptest.NewRecorder() + c := e.NewContext(req, rec) + + config := DefaultRecoverConfig + config.DisableErrorHandler = false + h := RecoverWithConfig(config)(echo.HandlerFunc(func(c echo.Context) error { + panic(http.ErrAbortHandler) + })) + + defer func() { + if r := recover(); r == nil { + t.Errorf("expecting nil, got `%v`", r) + } + }() + h(c) + + h = Recover()(echo.HandlerFunc(func(c echo.Context) error { + panic(http.ErrAbortHandler) + })) + h(c) + assert.Equal(t, http.StatusInternalServerError, rec.Code) + assert.Contains(t, buf.String(), "PANIC RECOVER") +} From c8f05aa37f1d037f0f4f047765c9c7f78aa9a82f Mon Sep 17 00:00:00 2001 From: Omkar Chorghe Date: Thu, 23 Feb 2023 21:27:29 +0530 Subject: [PATCH 2/5] removed faulty tests --- middleware/recover_test.go | 29 ----------------------------- 1 file changed, 29 deletions(-) diff --git a/middleware/recover_test.go b/middleware/recover_test.go index 29b6f26f6..b27f3b41c 100644 --- a/middleware/recover_test.go +++ b/middleware/recover_test.go @@ -163,32 +163,3 @@ func TestRecoverWithConfig_LogErrorFunc(t *testing.T) { assert.Contains(t, output, `"level":"ERROR"`) }) } - -func TestRecoverWithDisabled_ErrorHandler(t *testing.T) { - e := echo.New() - buf := new(bytes.Buffer) - e.Logger.SetOutput(buf) - req := httptest.NewRequest(http.MethodGet, "/", nil) - rec := httptest.NewRecorder() - c := e.NewContext(req, rec) - - config := DefaultRecoverConfig - config.DisableErrorHandler = false - h := RecoverWithConfig(config)(echo.HandlerFunc(func(c echo.Context) error { - panic(http.ErrAbortHandler) - })) - - defer func() { - if r := recover(); r == nil { - t.Errorf("expecting nil, got `%v`", r) - } - }() - h(c) - - h = Recover()(echo.HandlerFunc(func(c echo.Context) error { - panic(http.ErrAbortHandler) - })) - h(c) - assert.Equal(t, http.StatusInternalServerError, rec.Code) - assert.Contains(t, buf.String(), "PANIC RECOVER") -} From 63bd8da94a37e4910bde51a5f09593ab24b86a50 Mon Sep 17 00:00:00 2001 From: Omkar Chorghe Date: Thu, 23 Feb 2023 22:07:09 +0530 Subject: [PATCH 3/5] sent back error, tests for recovering with error handler disabled --- middleware/recover.go | 3 ++- middleware/recover_test.go | 20 ++++++++++++++++++++ 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/middleware/recover.go b/middleware/recover.go index 827b59a0b..691eaff8c 100644 --- a/middleware/recover.go +++ b/middleware/recover.go @@ -76,7 +76,7 @@ func RecoverWithConfig(config RecoverConfig) echo.MiddlewareFunc { } return func(next echo.HandlerFunc) echo.HandlerFunc { - return func(c echo.Context) error { + return func(c echo.Context) (returnErr error) { if config.Skipper(c) { return next(c) } @@ -122,6 +122,7 @@ func RecoverWithConfig(config RecoverConfig) echo.MiddlewareFunc { if(!config.DisableErrorHandler) { c.Error(err) } + returnErr = err } }() return next(c) diff --git a/middleware/recover_test.go b/middleware/recover_test.go index b27f3b41c..ec624f32e 100644 --- a/middleware/recover_test.go +++ b/middleware/recover_test.go @@ -163,3 +163,23 @@ func TestRecoverWithConfig_LogErrorFunc(t *testing.T) { assert.Contains(t, output, `"level":"ERROR"`) }) } + +func TestRecoverWithDisabled_ErrorHandler(t *testing.T) { + e := echo.New() + buf := new(bytes.Buffer) + e.Logger.SetOutput(buf) + req := httptest.NewRequest(http.MethodGet, "/", nil) + rec := httptest.NewRecorder() + c := e.NewContext(req, rec) + + config := DefaultRecoverConfig + config.DisableErrorHandler = true + h := RecoverWithConfig(config)(echo.HandlerFunc(func(c echo.Context) error { + panic("test") + })) + err := h(c) + + assert.Equal(t, http.StatusOK, rec.Code) + assert.Contains(t, buf.String(), "PANIC RECOVER") + assert.NotNil(t, err) +} From f584058ab0cedb5a7c7ea24055fd60f076d662a4 Mon Sep 17 00:00:00 2001 From: Omkar Chorghe Date: Thu, 23 Feb 2023 22:27:14 +0530 Subject: [PATCH 4/5] Improved message for recovery middleware config variable DisableErrorHandler --- middleware/recover.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/middleware/recover.go b/middleware/recover.go index 691eaff8c..1dcb06c32 100644 --- a/middleware/recover.go +++ b/middleware/recover.go @@ -39,7 +39,7 @@ type ( // If it's set you don't need to provide LogLevel for config. LogErrorFunc LogErrorFunc - // DisableErrorHandler disables the centralized HTTPErrorHandler. + // DisableErrorHandler disables the call to centralized HTTPErrorHandler. // Optional. Default value false. DisableErrorHandler bool `yaml:"disable_error_handler"` } From e368ae06eb28003d5f4d675559c944da589dd86f Mon Sep 17 00:00:00 2001 From: Omkar Chorghe Date: Fri, 24 Feb 2023 08:47:52 +0530 Subject: [PATCH 5/5] Improved recovery config message, now mentions the behavior that error is only returned back if centralized error handler is disabled, improved test cases --- middleware/recover.go | 4 +++- middleware/recover_test.go | 5 +++-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/middleware/recover.go b/middleware/recover.go index 1dcb06c32..36d41aa64 100644 --- a/middleware/recover.go +++ b/middleware/recover.go @@ -40,6 +40,7 @@ type ( LogErrorFunc LogErrorFunc // DisableErrorHandler disables the call to centralized HTTPErrorHandler. + // The recovered error is then passed back to upstream middleware, instead of swallowing the error. // Optional. Default value false. DisableErrorHandler bool `yaml:"disable_error_handler"` } @@ -121,8 +122,9 @@ func RecoverWithConfig(config RecoverConfig) echo.MiddlewareFunc { if(!config.DisableErrorHandler) { c.Error(err) + } else { + returnErr = err } - returnErr = err } }() return next(c) diff --git a/middleware/recover_test.go b/middleware/recover_test.go index ec624f32e..3e0d35d79 100644 --- a/middleware/recover_test.go +++ b/middleware/recover_test.go @@ -23,7 +23,8 @@ func TestRecover(t *testing.T) { h := Recover()(echo.HandlerFunc(func(c echo.Context) error { panic("test") })) - h(c) + err := h(c) + assert.NoError(t, err) assert.Equal(t, http.StatusInternalServerError, rec.Code) assert.Contains(t, buf.String(), "PANIC RECOVER") } @@ -181,5 +182,5 @@ func TestRecoverWithDisabled_ErrorHandler(t *testing.T) { assert.Equal(t, http.StatusOK, rec.Code) assert.Contains(t, buf.String(), "PANIC RECOVER") - assert.NotNil(t, err) + assert.EqualError(t, err, "test") }