Skip to content

Call #after of middleware on error #1240

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

Merged
merged 2 commits into from
Jan 11, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
* [#1227](https://github.com/ruby-grape/grape/pull/1227): Store `message_key` on Grape::Exceptions::Validation - [@stjhimy](https://github.com/sthimy).
* [#1232](https://github.com/ruby-grape/grape/pull/1232): Helpers are now available inside `rescue_from` - [@namusyaka](https://github.com/namusyaka).
* [#1237](https://github.com/ruby-grape/grape/pull/1237): Allow multiple parameters in `given`, which behaves as if the scopes were nested in the inputted order - [@ochagata](https://github.com/ochagata).
* [#1238](https://github.com/ruby-grape/grape/pull/1238): Call `after` of middleware on error - [@namusyaka](https://github.com/namusyaka).
* Your contribution here.

#### Fixes
Expand Down
28 changes: 28 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@
- [Before and After](#before-and-after)
- [Anchoring](#anchoring)
- [Using Custom Middleware](#using-custom-middleware)
- [Grape Middleware](#grape-middleware)
- [Rails Middleware](#rails-middleware)
- [Remote IP](#remote-ip)
- [Writing Tests](#writing-tests)
Expand Down Expand Up @@ -2557,6 +2558,33 @@ part.

## Using Custom Middleware

### Grape Middleware

You can make a custom middleware by using `Grape::Middleware::Base`.
It's inherited from some grape official middlewares in fact.

For example, you can write a middleware to log application exception.

```ruby
class LoggingError < Grape::Middleware::Base
def after
if @api_response[0] == 500
env['rack.logger'].error("Raised error on #{env['PATH_INFO']}")
end
end
end
```

Your middleware can overwrite application response as follows, except error case.

```ruby
class Overwriter < Grape::Middleware::Base
def after
[200, { 'Content-Type' => 'text/plain' }, ['Overwrited.']]
end
end
```

### Rails Middleware

Note that when you're using Grape mounted on Rails you don't have to use Rails middleware because it's already included into your middleware stack.
Expand Down
19 changes: 19 additions & 0 deletions UPGRADING.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,25 @@
Upgrading Grape
===============

### Upgrading to >= 0.15.0

#### Changes to behavior of `after` method of middleware on error
Copy link
Member

Choose a reason for hiding this comment

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

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.


The `after` method of the middleware is now called on error.
The following code would work correctly.

```ruby
class ErrorMiddleware < Grape::Middleware::Base
def after
if @api_response[0] == 500
env['rack.logger'].debug("Raised error on #{env['PATH_INFO']}")
end
end
end
```

See [#1147](https://github.com/ruby-grape/grape/issues/1147) and [#1240](https://github.com/ruby-grape/grape/issues/1240) for discussion of the issues.

### Upgrading to >= 0.14.0

#### Changes to availability of DSL methods in filters
Expand Down
8 changes: 6 additions & 2 deletions lib/grape/middleware/base.rb
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,12 @@ def call(env)
def call!(env)
@env = env
before
@app_response = @app.call(@env)
after || @app_response
begin
@app_response = @app.call(@env)
ensure
after_response = after
end
after_response || @app_response
end
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, thanks.


# @abstract
Expand Down
1 change: 1 addition & 0 deletions lib/grape/middleware/formatter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ def before
end

def after
return unless @app_response
Copy link
Member

Choose a reason for hiding this comment

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

I think the semantics of after is to always execute, with or without @app_response.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Raised an exception if your suggestion is reflect.
Should the formatter's after method fully be executed in this case?

Copy link
Member

Choose a reason for hiding this comment

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

Oh I see, this is right, leave it here.

status, headers, bodies = *@app_response

if Rack::Utils::STATUS_WITH_NO_ENTITY_BODY.include?(status)
Expand Down
19 changes: 19 additions & 0 deletions spec/grape/middleware/base_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,25 @@
after { subject.call!({}) }
end

context 'callbacks on error' do
let(:blank_app) { ->(_) { fail StandardError } }

it 'calls #after' do
expect(subject).to receive(:after)
expect { subject.call({}) }.to raise_error(StandardError)
end
end

context 'after callback' do
before do
allow(subject).to receive(:after).and_return([200, {}, 'Hello from after callback'])
end

it 'overwrites application response' do
expect(subject.call!({}).last).to eq('Hello from after callback')
end
end

it 'is able to access the response' do
subject.call({})
expect(subject.response).to be_kind_of(Rack::Response)
Expand Down