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

adding option to allow grape to handle Grape::Exception when rescue all is set #1398

Merged
merged 1 commit into from
Jun 30, 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
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@

* Your contribution here.
Copy link
Member

Choose a reason for hiding this comment

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

Put this back please.


* [#1398](https://github.com/ruby-grape/grape/pull/1398): Added rescue_from :grape_exceptions option to allow Grape to use the built in Grape::Exception handing and use rescue :all behavior for everything else - [@mmclead](https://github.com/mmclead).

#### Features

* [#1393](https://github.com/ruby-grape/grape/pull/1393): Middleware can be inserted before or after default Grape middleware - [@ridiculous](https://github.com/ridiculous).
Expand Down
11 changes: 11 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -1853,6 +1853,17 @@ class Twitter::API < Grape::API
end
```

Grape can also rescue from all exceptions and still use the built-in exception handing.
This will give the same behavior as `rescue_from :all` with the addition that Grape will use the exception handling defined by all Exception classes that inherit Grape::Exceptions::Base.

The intent of this setting is to provide a simple way to cover the most common exceptions and return any unexpected exceptions in the API format.

```ruby
class Twitter::API < Grape::API
rescue_from :grape_exceptions
end
```

You can also rescue specific exceptions.

```ruby
Expand Down
6 changes: 5 additions & 1 deletion lib/grape/dsl/request_response.rb
Original file line number Diff line number Diff line change
Expand Up @@ -110,9 +110,13 @@ def rescue_from(*args, &block)
end
handler ||= extract_with(options)

if args.include?(:all)
case
when args.include?(:all)
namespace_inheritable(:rescue_all, true)
namespace_inheritable :all_rescue_handler, handler
when args.include?(:grape_exceptions)
namespace_inheritable(:rescue_all, true)
namespace_inheritable(:rescue_grape_exceptions, true)
else
handler_type =
case options[:rescue_subclasses]
Expand Down
1 change: 1 addition & 0 deletions lib/grape/endpoint.rb
Original file line number Diff line number Diff line change
Expand Up @@ -254,6 +254,7 @@ def build_stack(helpers)
content_types: namespace_stackable_with_hash(:content_types),
default_status: namespace_inheritable(:default_error_status),
rescue_all: namespace_inheritable(:rescue_all),
rescue_grape_exceptions: namespace_inheritable(:rescue_grape_exceptions),
default_error_formatter: namespace_inheritable(:default_error_formatter),
error_formatters: namespace_stackable_with_hash(:error_formatters),
rescue_options: namespace_stackable_with_hash(:rescue_options) || {},
Expand Down
10 changes: 8 additions & 2 deletions lib/grape/middleware/error.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,9 @@ def default_options
formatters: {},
error_formatters: {},
rescue_all: false, # true to rescue all exceptions
rescue_grape_exceptions: false,
rescue_subclasses: true, # rescue subclasses of exceptions listed
rescue_options: { backtrace: false }, # true to display backtrace
rescue_options: { backtrace: false }, # true to display backtrace, true to let Grape handle Grape::Exceptions
rescue_handlers: {}, # rescue handler blocks
base_only_rescue_handlers: {}, # rescue handler blocks rescuing only the base class
all_rescue_handler: nil # rescue handler block to rescue from all exceptions
Expand All @@ -34,7 +35,7 @@ def call!(env)
end)
rescue StandardError => e
is_rescuable = rescuable?(e.class)
if e.is_a?(Grape::Exceptions::Base) && !is_rescuable
if e.is_a?(Grape::Exceptions::Base) && (!is_rescuable || rescuable_by_grape?(e.class))
handler = ->(arg) { error_response(arg) }
else
raise unless is_rescuable
Expand Down Expand Up @@ -63,6 +64,11 @@ def rescuable?(klass)
rescue_all? || rescue_class_or_its_ancestor?(klass) || rescue_with_base_only_handler?(klass)
end

def rescuable_by_grape?(klass)
return false if klass == Grape::Exceptions::InvalidVersionHeader
options[:rescue_grape_exceptions]
end

def exec_handler(e, &handler)
if handler.lambda? && handler.arity == 0
instance_exec(&handler)
Expand Down
14 changes: 14 additions & 0 deletions spec/grape/dsl/request_response_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,20 @@ def self.imbue(key, value)
end
end

describe ':grape_exceptions' do
it 'sets rescue all to true' do
expect(subject).to receive(:namespace_inheritable).with(:rescue_all, true)
expect(subject).to receive(:namespace_inheritable).with(:rescue_grape_exceptions, true)
subject.rescue_from :grape_exceptions
end

it 'sets rescue_grape_exceptions to true' do
expect(subject).to receive(:namespace_inheritable).with(:rescue_all, true)
expect(subject).to receive(:namespace_inheritable).with(:rescue_grape_exceptions, true)
subject.rescue_from :grape_exceptions
end
end

describe 'list of exceptions is passed' do
it 'sets hash of exceptions as rescue handlers' do
expect(subject).to receive(:namespace_reverse_stackable).with(:rescue_handlers, StandardError => nil)
Expand Down
37 changes: 37 additions & 0 deletions spec/grape/exceptions/body_parse_errors_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,43 @@ def app
end
end

context 'api with rescue_from :grape_exceptions handler' do
subject { Class.new(Grape::API) }
before do
subject.rescue_from :all do |_e|
rack_response 'message was processed', 400
end
subject.rescue_from :grape_exceptions

subject.params do
requires :beer
end
subject.post '/beer' do
'beer received'
end
end

def app
subject
end

context 'with content_type json' do
it 'returns body parsing error message' do
post '/beer', 'test', 'CONTENT_TYPE' => 'application/json'
expect(last_response.status).to eq 400
expect(last_response.body).to include 'message body does not match declared format'
Copy link
Member

Choose a reason for hiding this comment

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

What's the full body here? Maybe we can make an exact match?

Copy link
Author

Choose a reason for hiding this comment

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

The full body is

Problem:
  message body does not match declared format
Resolution:
  when specifying application/json as content-type, you must pass valid application/json in the request's 'body'

Kind of a pain to match the full thing. Totally could do. Was following the precedent set on the tests below:
https://github.com/mmclead/grape/blob/68717c623ef5c3c0ad4f9180c5553c8ce758fa92/spec/grape/exceptions/body_parse_errors_spec.rb#L111

Copy link
Member

Choose a reason for hiding this comment

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

That's fine, thanks.

end
end

context 'with content_type xml' do
it 'returns body parsing error message' do
post '/beer', 'test', 'CONTENT_TYPE' => 'application/xml'
expect(last_response.status).to eq 400
expect(last_response.body).to include 'message body does not match declared format'
end
end
end

context 'api without a rescue handler' do
subject { Class.new(Grape::API) }
before do
Expand Down