Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added a optional config variable to disable centralized error handler in recovery middleware #2410

Merged
merged 5 commits into from
Feb 24, 2023

Conversation

Omkar-C
Copy link
Contributor

@Omkar-C Omkar-C commented Feb 23, 2023

Recovery Middleware calls the centralized Error Handler. This is a change to have a optional config variable to disable centralized error handler in recovery. If the centrailzed Error Handler is disabled, panic error caught, will be returned to upstream middleware.

@Omkar-C
Copy link
Contributor Author

Omkar-C commented Feb 23, 2023

Should I pass back the error with closure, so upstream middlewares can handle ?

@Omkar-C
Copy link
Contributor Author

Omkar-C commented Feb 23, 2023

Should I pass back the error with closure, so upstream middlewares can handle ?

I have returned back the error, seems like that should be the ideal behavior, also added corrected tests

middleware/recover.go Outdated Show resolved Hide resolved
middleware/recover_test.go Outdated Show resolved Hide resolved
middleware/recover.go Show resolved Hide resolved
…r is only returned back if centralized error handler is disabled, improved test cases
@Omkar-C Omkar-C requested a review from aldas February 24, 2023 05:57
@codecov
Copy link

codecov bot commented Feb 24, 2023

Codecov Report

Base: 92.84% // Head: 92.85% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (e368ae0) compared to base (47844c9).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2410   +/-   ##
=======================================
  Coverage   92.84%   92.85%           
=======================================
  Files          39       39           
  Lines        4503     4507    +4     
=======================================
+ Hits         4181     4185    +4     
  Misses        234      234           
  Partials       88       88           
Impacted Files Coverage Δ
middleware/recover.go 83.01% <100.00%> (+1.38%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@Omkar-C Omkar-C changed the title Added a optional config variable to disable centralized error handler in recovery Added a optional config variable to disable centralized error handler in recovery middleware Feb 24, 2023
Copy link
Contributor

@aldas aldas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@aldas
Copy link
Contributor

aldas commented Feb 24, 2023

For history sake - not returning error by default bothers me as it would be more correct behavior. But this would make middleware behave completely different as it has done since 2015. These simple things can be problematic as it changes how already existing application middleware chain work. Probably 98% time this is not an issue - but for some it could be.

life is about making compromises.

p.s. just looked my v5 branch and there I already have made so that c.Errror is not used anymore and we return that recovered "error"

@aldas aldas merged commit 1e575b7 into labstack:master Feb 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants