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

Conversation

mmclead
Copy link

@mmclead mmclead commented May 13, 2016

This will allow Grape to use the built in Grape Exceptions defined in /lib/grape/exceptions when rescue :all is set.

@@ -1,7 +1,7 @@
0.16.3 (Next)
=============

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

@dblock
Copy link
Member

dblock commented May 13, 2016

Would you please explain a bit more in detail what this achieves? I understand what it does, but I cannot think of a scenario where this is really useful.

@mmclead mmclead force-pushed the adding_rescue_grape_errors_option branch from 6f0cffe to 2700b54 Compare May 17, 2016 20:41
@mmclead
Copy link
Author

mmclead commented May 17, 2016

I added this so that I could easily respond with JSON errors and not HTML that leaked too much data and still have Grape handle 404's, 406's, and all the other exceptions that are handled out of the box.

So, if you were to add rescue_from :all, rescue_grape_errors: true to your API, you could cover all exception handling in one go, and then add specific error! calls as needed throughout the code.

Does that make sense? I can try to explain better if needed.

@dblock
Copy link
Member

dblock commented May 18, 2016

There's still something odd about this feature. It skips a specific exception, handles the rest, so that seems very "special" to begin with.

I want to talk it over more, I am sure we can come up with something better.

Can't the same thing be accomplished with rescue_from Grape::Exceptions::Base, minus the special handling of Grape::Exceptions::InvalidVersionHeader? If that's the case maybe we could alias that long exception name with rescue_from :grape_exceptions?

@mmclead
Copy link
Author

mmclead commented Jun 6, 2016

The exception Grape::Exceptions::InvalidVersionHeader is skipped for the same reason that it is skipped in rescuable?(klass) which is explained in the README:

Unrescuable Exceptions

Grape::Exceptions::InvalidVersionHeader, which is raised when the version in the request header doesn't match the currently evaluated version for the endpoint, will never be rescued from a rescue_from block (even a rescue_from :all) This is because Grape relies on Rack to catch that error and try the next versioned-route for cases where there exist identical Grape endpoints with different versions.

I just copied the same logic. I wanted to put this repeated logic into one place but couldn't think of a clean way to do it.

I don't think it'll work the same if we do rescue_from Grape::Exceptions::Base because an ActiveRecord::RecordInvalid wouldn't be caught. The purpose of this PR is to allow users to be lazy about their error catching if they don't have specific use cases for certain errors. As soon as the user has need of specific error handling then error! is available.
Until then, this PR gives a 1 line way to catch all errors and use the previously built in error handling that Grape currently bypasses when rescue :all is set.

EDIT: I totally agree that the name could be better though. Any suggestions?

@dblock
Copy link
Member

dblock commented Jun 7, 2016

Ok, but don't you think a DSL like rescue_from :grape_exceptions would be more user-friendly? Same implementation roughly.

@mmclead
Copy link
Author

mmclead commented Jun 7, 2016

yeah. I like that name better. it doesn't explicitly say that its also rescuing :all but a decently descriptive README might make up for it.

I'll make that change and we can take a look at it.

@dblock change completed. let me know what you think.

@mmclead mmclead force-pushed the adding_rescue_grape_errors_option branch 4 times, most recently from fa98bcb to 4b2411d Compare June 10, 2016 17:54
@mmclead
Copy link
Author

mmclead commented Jun 20, 2016

@dblock change completed. let me know what you think.

@@ -3,6 +3,8 @@

* Your contribution here.

* [#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)
Copy link
Member

Choose a reason for hiding this comment

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

Make this look like every other line please, the period goes to the end and there's a dash before your name ;)

@dblock
Copy link
Member

dblock commented Jun 20, 2016

Made some comments, it's mostly ready. Try to shorten the README, get to the point :)

@mmclead mmclead force-pushed the adding_rescue_grape_errors_option branch from 4b2411d to 68717c6 Compare June 30, 2016 05:49
@mmclead
Copy link
Author

mmclead commented Jun 30, 2016

@dblock updated again. Sorry my grammar is terrible. And, I answered comment on test. Your call on whether you want the full match or continue with the way other examples are.

@dblock dblock merged commit 42107da into ruby-grape:master Jun 30, 2016
@dblock
Copy link
Member

dblock commented Jun 30, 2016

Merged, thank you.

dblock added a commit that referenced this pull request Jun 30, 2016
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.

2 participants