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

Changed error handling logic to stop after error has been handled #497

Merged
merged 2 commits into from
Apr 14, 2020
Merged

Changed error handling logic to stop after error has been handled #497

merged 2 commits into from
Apr 14, 2020

Conversation

philprime
Copy link
Contributor

@philprime philprime commented Mar 28, 2020

Changed run error middlewares so that it conforms to the behaviour stated in the documentation

middleware1 = {
  onError: (handler) => {
    handler.response = { error: handler.error };
    return Promise.resolve();
    // Resolves to a falsy value
  }
}
middleware2 = {
  onError: (handler) => {
    return Promise.resolve(handler.error)
  }
}
handler.use(middleware1).use(middleware2);

middleware2 should not be called.

Relevant issues:

@codecov
Copy link

codecov bot commented Apr 9, 2020

Codecov Report

Merging #497 into master will not change coverage by %.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #497   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           20        20           
  Lines          547       550    +3     
  Branches       112       113    +1     
=========================================
+ Hits           547       550    +3     
Impacted Files Coverage Δ
src/middy.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2931123...f55511f. Read the comment docs.

@lmammino
Copy link
Member

Hello @philprime!
Thanks a lot for taking the time to address this and apologies again for the delay with my review.

At first glance, this looks good and more consistent with what is documented here.

I am ok for merging this, but I prefer to do it after #504 so that I can release it together with few other pending PRs.

Meanwhile, I'd love to get @willfarrell or @dkatavic review as well here :)

@lmammino lmammino merged commit c6c80a9 into middyjs:master Apr 14, 2020
@@ -276,7 +276,7 @@ describe('🛵 Middy test suite', () => {
})
})

test('If theres an error and one error middleware handles the error, the next error middlewares is executed', (endTest) => {
test('If theres is an error and one error middleware handles the error, the next error middlewares should not be executed', (endTest) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should there be there instead of theres?

Copy link
Contributor

Choose a reason for hiding this comment

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

This was merged while inspecting the code, ignore it

Copy link
Member

Choose a reason for hiding this comment

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

I did mess up with this one a little. I will do another commit to properly release this on 0.x and also fix this typo. So totally valuable comment :) Thanks

Copy link
Member

Choose a reason for hiding this comment

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

(i will also fix in the port to 1.0)

@philprime philprime deleted the hotfix/error-handled-middleware branch April 14, 2020 12:52
@lmammino
Copy link
Member

We should port this one to 1.0.0-beta, realized only now this was targeting master :(

@lmammino
Copy link
Member

This fix will be released shortly in 0.35.0 and in 1.0.0-beta.11

@lmammino
Copy link
Member

I am revisiting this in the context of v1 (which includes a http-response-serializer that also works with the default HTTP error handler).

This PR breaks the original intended design (and therefore compatibility with some middlewares).

As per the docs:

When a middleware handles the error and creates a response, the execution is still propagated to all the other error middlewares and they have a chance to update or replace the response as needed. At the end of the error middlewares sequence, the response is returned to the user.

This is needed for instance in the case of the HTTP error handler which will format the error in a correct response for the lambda HTTP integration. If this is combined with the response serializer it must be possible to convert this response to XML or other formats.

I will have to revert this on master

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants