Fix error handling of wrapped errors #1
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This exposes a bug in the existing implementation. Wrapped errors are not handled by the middleware.
The parameters used where in the wrong order.
richzw
requested changes
Sep 8, 2023
Owner
richzw
left a comment
There was a problem hiding this comment.
Thank you very much for your PR. I think it could be better for if errors.Is(lastError.Err, e) || errors.Is(e, lastError.Err) { since the only the first parameter of errors.is was Unwrap .
This allows simpler unit tests of the matching logic and reduces complexity of the original function.
These ensure the matching will stay the same for future changes without the testing overhead of performing an http request.
This has been suggested in PR review.
It seems that all tests are fine without the second comparison.
Contributor
Author
|
Thx for the feedback. I think it is useful to move the error matching into it's own function which allowed for far simpler unit testing. After I applied you change, which I don't yet fully get, I wanted to test if we even need two comparisons. Have a look at the changes and tell me what you think. I think only one comparison is actually needed. Tests are green. |
richzw
approved these changes
Sep 8, 2023
Owner
richzw
left a comment
There was a problem hiding this comment.
LGTM. Thank you very much for your PR
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR fixes error handling for wrapped errors.
This bug was caused by wrong parameter order in the usage of
errors.IsThe added test case shows a use case.
@richzw This changes the behaviour of the middleware: Now - errors which may deeply wrap other errors could match in the middleware where they previously didn't. I think this change fixes a bug but maybe you want the behaviour do not change? LMK.