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

Rescue Child Errors In Error Middleware #544

Closed
wants to merge 4 commits into from

Conversation

xevix
Copy link
Contributor

@xevix xevix commented Dec 30, 2013

Summary

This enables the behavior desired in #542

Assuming this exception hierarchy:

module APIErrors
  class ParentError < StandardError; end
  class Child < ParentError; end
end

And the following rescue_from calls:

rescue_from Grape::Exceptions::ValidationErrors do |e|
    Rack::Response.new(
      status: e.status,
      message: e.message,
      errors: e.errors
    }.to_json, e.status)
end
rescue_from APIErrors::ParentError, rescue_children: true do |e|
  Rack::Response.new({
    error: "#{e.class} error",
    message: e.message
  }.to_json, e.status)
end

Then the following will be handled by the first handler above.

raise Grape::Exceptions::ValidationErrors, "bad data"

And the following will all be handled by the second handler.

raise APIErrors::ParentError, "I'm going to be rescued!"
raise APIErrors::Child, "I'm going to be rescued too!"

Notes

  • Existing behavior of only handling parent errors is maintained as the default code path, so performance differences should be negligible (but please confirm)
  • List of handled errors was removed, since it's redundant (an error is handled if rescue_all is set or if it has a handler)
  • Two sets of handlers, the existing rescue_handlers and a new rescue_children_handlers were used, as a tradeoff for complexity of embedding one in the other
    • The imbue() function does not recursively parse hashes/arrays to merge/concat, so options get overridden on multiple calls
    • Marking every single handler with a children? bool is a waste of time/memory

Alejandro Wainzinger added 2 commits December 31, 2013 03:54
@dblock
Copy link
Member

dblock commented Dec 30, 2013

I think we should find a better name than rescue_children, we are definitely not doing that much for society :-) Maybe invert the option to rescue and provide a list such as :base. I may be open to changing to default too. Open to suggestions.

Also please update CHANGELOG.

@xevix
Copy link
Contributor Author

xevix commented Dec 31, 2013

Yeah, I didn't want to spend a ton of time coming up with a name as I figured someone would come up with a better suggestion anyway, but it does sound hilarious on second look, haha.

I would be fine with making it the default, except it might break existing behavior. Someone out there might be rescuing their own error derived from RuntimeError, and RuntimeError too or something like that. This does sound a bit esoteric though. Anyway, there's a unit test to that effect, so I didn't want to break it, but open to what people think.
https://github.com/intridea/grape/blob/e23069adcc0dd25c2717b86c8f285a8954a2a499/spec/grape/api_spec.rb#L1238

Didn't quite understand the suggestion about inverting the option to rescue and providing a :base list. Are you saying to invert the :rescue_children option so that it doesn't rescue children (switching default) and providing the list of exceptions to the now-default? Sounds like a good idea if so. If I get the greenlight to change the existing behavior, will go this route.

Should I also update the README for rescue_from?

@lloydmeta
Copy link

👍 to this.

@dblock
Copy link
Member

dblock commented Jan 1, 2014

I am OK with breaking the default, I think it's a much better default, we just need to bump the major version of Grape and make sure to document this clearly in CHANGELOG as an API change.

Lets figure out what a better name for this is. Since the default is going to become rescuing the exception and any child classes of the exceptions, we need a name for the opposite behavior. How about rescue_subclasses: false?

I also like happy_new_year: true, to anyone who is contributing! :)

@xevix
Copy link
Contributor Author

xevix commented Jan 1, 2014

Yes, happy new year! It's awesome to even get a chance to contribute to this awesome library in some way, thanks for the quick replies.

I'll go ahead then and switch the default, update the CHANGELOG and README. Will also update the above test case since it'll be obsolete.

@dblock
Copy link
Member

dblock commented Jan 1, 2014

Nice job on the README, too. Merged via 268a58d.

@dblock dblock closed this Jan 1, 2014
@xevix
Copy link
Contributor Author

xevix commented Jan 1, 2014

Thanks! One last thing, I made a typo on a test, but should be the last thing on #546 . Sorry about that, and thanks for the fast work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants