Call #after of middleware on error

[namusyaka]

This might be able to close #1147.
Could you review this PR?

[dblock]

Either all the paths aren't covered by specs or this can be:

begin
  @app_response = @app.call(@env)
ensure
  after || @app_response
end

Causes the after middleware to execute on error, and raises the exception as before.

What am I missing?

[namusyaka]

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.

[namusyaka]

Also, the ensure section isn't used as retured value. Thoughts?

[dblock]

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.

[namusyaka]

Makes sense.
So should I remove Response from this line?

[dblock]

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

[namusyaka]

Okay, thanks.

[namusyaka]

I pushed new changes into this branch.
Currently, we raise the exception even if an exception is raised on @app.call.

If there is no problem, I'm going to squash a few commits and force push.

[dblock]

This needs a spec for the behavior of after returning a Rack response and overwriting the app response, so we don't break this in the future. Otherwise it's good to go.

[namusyaka]

Added.

[dblock]

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.

[dblock]

Thanks for hanging in with me!

[namusyaka]

Yeah, squashed and force pushed.
Thank you, too!

[dblock]

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.

[namusyaka]

Force pushed. Also I added a little Grape middleware section.

[dblock]

Merged. Thanks!

[namusyaka]

Thank you for your polite response.

[rngtng]

continue discussion from #1255: this change breaks https://github.com/aserafin/grape_logging (and all other middlewares which assumes @app_response isn't nil) on application error.

While updates on the middleware are pending, I'd propose to at least rescue any after call errors silently:

begin
  @app_response = @app.call(@env)
ensure
    after_response = after rescue nil
end

... or would setting @app_response to a default value a better solution?

[dblock]

@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?

[rngtng]

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...