Call #after of middleware on error #1240
Conversation
| after_response = after | ||
| end | ||
|
|
||
| after_response || (error_class ? (fail error_class) : @app_response) | ||
| end |
There was a problem hiding this comment.
Either all the paths aren't covered by specs or this can be:
begin
@app_response = @app.call(@env)
ensure
after || @app_response
endCauses the after middleware to execute on error, and raises the exception as before.
What am I missing?
There was a problem hiding this comment.
Well, If after method returns rack response format on error, the response should be used forcibly, I think.
If not reasonable, I'm going to fix it.
There was a problem hiding this comment.
Also, the ensure section isn't used as retured value. Thoughts?
There was a problem hiding this comment.
I think we shouldn't be expecting the after block to return a response. Otherwise we need to change the behavior of before as well and this will definitely have impact on API implementations out there. These are callbacks, so I think they should be called, but their results discarded, at least as the next increment.
There was a problem hiding this comment.
Makes sense.
So should I remove Response from this line?
There was a problem hiding this comment.
Hmmm... wait. Looks like the old behavior was to use the response. So let me take this back, we should preserve the response if it was there. I guess it should be closer to this:
begin
@app_response = @app.call(@env)
ensure
@after_response = after
end
@after_response || @app_response|
I pushed new changes into this branch. If there is no problem, I'm going to squash a few commits and force push. |
|
This needs a spec for the behavior of |
|
Added. |
|
Forgot one more thing. The behavior is different from previous versions (even if it was undocumented), so this needs an entry in UPGRADING, please. Squash everything and I'll merge. |
|
Thanks for hanging in with me! |
45966fb to
06cdf05
Compare
|
Yeah, squashed and force pushed. |
06cdf05 to
bc6be4b
Compare
| @@ -3,6 +3,30 @@ Upgrading Grape | |||
|
|
|||
| ### Upgrading to >= 0.14.0 | |||
|
|
|||
| #### Changes to behavior of `after` method of middleware on error | |||
There was a problem hiding this comment.
This should be under >= 0.15.0 and increment the version in version.rb to 0.15.0 as well, now that we have a major change.
Add a test for the bevhavior of returning Rack response by after callback
bc6be4b to
cc60281
Compare
|
Force pushed. Also I added a little Grape middleware section. |
Call #after of middleware on error
|
Merged. Thanks! |
|
Thank you for your polite response. |
|
continue discussion from #1255: this change breaks https://github.com/aserafin/grape_logging (and all other middlewares which assumes While updates on the middleware are pending, I'd propose to at least rescue any begin
@app_response = @app.call(@env)
ensure
after_response = after rescue nil
end... or would setting |
|
@rngtng Could you open a new issue please? I really don't like hiding errors like this. Should we just take it all in and make grape_logging more resilient? |
|
jepp, updating grape_logging is the right way to go, I only guess there are more middleware out there which probably break. Instead of hiding, we should communicate the error at least. (a warning?) - I'll open a issue for that... |
This might be able to close #1147.
Could you review this PR?