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.

Sign up for GitHub

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

Jump to bottom

Fix rescue_from subclasses default #651

Closed
wants to merge 6 commits into from
Closed

Fix rescue_from subclasses default #651

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
196b0e1
Fix rescue_from default value of rescue_subclasses to true.
xevix May 23, 2014
14be585
Add test to ensure that rescue_from rescue_subclasses defaults to true.
xevix May 23, 2014
ba410fa
Create a separate option for the handler to rescue_from :all exceptions.
xevix May 26, 2014
acff701
Update CHANGELOG.
xevix May 26, 2014
462bd7c
Remove check to see if element is a class since no longer necessary.
xevix May 26, 2014
bfb7b83
Remove unnecessary variable.
xevix May 26, 2014
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
8 changes: 7 additions & 1 deletion lib/grape/api.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,7 +214,13 @@ def rescue_from(*args, &block)
options = args.last.is_a?(Hash) ? args.pop : {}
handler ||= proc { options[:with] } if options.key?(:with)

handler_type = !!options[:rescue_subclasses] ? :rescue_handlers : :base_only_rescue_handlers
handler_type =
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dblock The !! trick works when you want nil, false but since we want nil, true I ended up with a case statement. Do you know of a prettier more idiomatic way of doing this?

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 this is totally fine.

Copy link

Choose a reason for hiding this comment

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

You could just swap it around (nil != false):

handler_type = options[:rescue_subclasses] == false ? :base_only_rescue_handlers : :rescue_handlers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@adrianmacneil Yeah that would do the trick, thanks for the suggestion. In this case I'll stick with the case as it reads a bit clearer.

case options[:rescue_subclasses]
when nil, true
:rescue_handlers
else
:base_only_rescue_handlers
end
imbue handler_type, Hash[args.map { |arg| [arg, handler] }]

imbue(:rescue_options, options)
Expand Down
2 changes: 1 addition & 1 deletion lib/grape/middleware/error.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ def call!(env)
end

def find_handler(klass)
handler = options[:rescue_handlers].find(-> { [] }) { |error, _| klass <= error }[1]
handler = options[:rescue_handlers].find(-> { [] }) { |error, _| error.is_a?(Class) && klass <= error }[1] || options[:rescue_handlers][:all]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dblock Added the check for Class because :all can show up here, and the <= operator doesn't like non-classes. An alternative is to remove the :all member from :rescue_handlers beforehand, but I thought this was overkill. Does this seem ok?

Copy link
Member

Choose a reason for hiding this comment

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

Seems OK, however obscure if you don't have the context when reading this code. I think removing :all would have been clearer. Up to you if you want to try doing that or add a comment that error can be :all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dblock Yeah, I was thinking of doing that. I don't know a method that can find only the first element matching and delete it for any Enumerable (args in api.rb can be a Hash or Array I believe). The find_index and delete_at will work for Array but not sure of a DRY way for it to also work on Hash. Also, the :all member's value (handler) can be a block so it would need to be available here somehow anyway, perhaps as a separate :all_rescue_handler or something.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe you can select first. But TBH I am ok with whatever works at this point here :)

handler = options[:base_only_rescue_handlers][klass] || options[:base_only_rescue_handlers][:all] unless handler
handler
end
Expand Down
12 changes: 12 additions & 0 deletions spec/grape/api_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1343,6 +1343,18 @@ class ChildError < ParentError; end
expect { get '/uncaught_parent' }.to raise_error(StandardError)
end

it 'sets rescue_subclasses to true by default' do
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dblock I'm testing the behavior to see if it rescues subclasses, rather than testing if internally, it interprets rescue_subclasses as true. Is there a way we can set this option to default to true and then check the options hash in a test somehow, or is this ok?

Copy link
Member

Choose a reason for hiding this comment

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

This is OK IMO.

subject.rescue_from APIErrors::ParentError do |e|
rack_response("rescued from #{e.class.name}", 500)
end
subject.get '/caught_child' do
raise APIErrors::ChildError
end

get '/caught_child'
expect(last_response.status).to eql 500
end

it 'does not rescue child errors if rescue_subclasses is false' do
subject.rescue_from APIErrors::ParentError, rescue_subclasses: false do |e|
rack_response("rescued from #{e.class.name}", 500)
Expand Down