fix: Don't call c.Error(...) inside echotrace middlewares
#2609
Conversation
|
This PR is stale because it has been open 20 days with no activity. Remove stale label or comment or this will be closed in 10 days. |
There was a problem hiding this comment.
My only concern with these changes is the possibility of breaking existing applications that rely in the existing behavior (even though they probably shouldn't...). Any thoughts? @darccio
| return err | ||
| } | ||
| return nil | ||
| return next(c) |
There was a problem hiding this comment.
I think we should add some test to verify this behavior and ensure we don't introduce a regression in the future.
There was a problem hiding this comment.
@rarguelloF Actually, if somebody was relying in the existing behaviour, they probably won't feel a difference. The existing behaviour blocked middlewares running before our own to manipulate the response, as c.Error already commited it to the client.
I think it's safe to go ahead as it is right now.
| @@ -163,7 +167,6 @@ func TestError(t *testing.T) { | |||
| called = true | |||
|
|
|||
| err := wantErr | |||
| c.Error(err) | |||
There was a problem hiding this comment.
@rarguelloF I've updated the test to ensure that the error handler is called once, and I've removed the c.Error call here as well, since this is a test handler and from the echo docs we don't want to explicitly invoke the c.Error inside of handlers.
// Avoid using this method in handlers as no middleware will be able to effectively handle errors after that.
There was a problem hiding this comment.
The fact that this change has removed this call and not broken anything in the rest of test should hopefully further highlight the fact that the expected behavior is already happening without this call, and explicitly calling it is causing as a second, unnecessary call.
|
I approve this but I'll wait to @rarguelloF final approval. |
|
Hey, thanks for approving @darccio @rarguelloF I'm not sure that the failing unit tests are related to my changes, they are in a separate contrib directory, seems to be a flaky test. |
|
@nakkamarra Yes, it is a flaky test. We opened an issue to work on it: #2641 |
What does this PR do?
This PR removes the calls to
c.Error(...)inside echotrace middlewares to remove duplicate calls to the registered echo.HTTPErrorHandlerMotivation
As described in the issue filed, we don't need to both invoke Error func and return the error up the chain of middlewares (as this will cause the registered error handler to execute twice), and it seems best to me to simply return the error so that the default error handler can handle this error, and as well as any custom error handle can without any hacks needed.
Reviewer's Checklist
For Datadog employees:
@DataDog/security-design-and-guidance.Unsure? Have a question? Request a review!