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

Allow rescue from parse errors #34341

Merged
merged 1 commit into from Nov 14, 2018
Merged

Conversation

@gmcgibbon
Copy link
Member

@gmcgibbon gmcgibbon commented Oct 29, 2018

Related to #34287, allows parameter parse errors to be caught by rescue_from blocks.

Closes #34287.

r? @rafaelfranca

test "can rescue a ParserError" do
silence_logger
post :arbitrary_action, body: "{", as: :json
assert_response :success
Copy link
Member

@Edouard-chin Edouard-chin Oct 30, 2018

assert_response :ok

Could we make an assertion on the body's content to ensure that the response came from rescuing the exception

@gmcgibbon gmcgibbon force-pushed the parse_error_rescue_2 branch 2 times, most recently from 82ceb6d to 2795339 Oct 30, 2018
post :arbitrary_action, body: "{", as: :json
end
assert_response :ok
assert_equal "arbitrary action", response.body
Copy link
Member

@rafaelfranca rafaelfranca Oct 30, 2018

Should not the response be "parser error"?

Copy link
Member Author

@gmcgibbon gmcgibbon Oct 30, 2018

Whoops, I'll fix that, and look into why its failing.

Copy link
Member Author

@gmcgibbon gmcgibbon Oct 30, 2018

I didn't realize this at first, but it only blows up if you reference params in the controller. Is that acceptable or should we look at rescuing on the request.filtered_parameters call in instrumentation's #process_action?

Copy link
Member

@rafaelfranca rafaelfranca Oct 30, 2018

That is the expected behavior for me.

@gmcgibbon gmcgibbon force-pushed the parse_error_rescue_2 branch from 2795339 to 20a2a11 Oct 30, 2018
Copy link
Member

@rafaelfranca rafaelfranca left a comment

This looks good to me. I'll ask other people to review too to check if I'm not missing something before merging.

cc @JoshCheek

@rafaelfranca rafaelfranca requested review from jeremy and pixeltrix Oct 30, 2018
@JoshCheek
Copy link
Contributor

@JoshCheek JoshCheek commented Oct 31, 2018

Oh, super exciting! I'll try it out this morning and report back with my results.

@JoshCheek
Copy link
Contributor

@JoshCheek JoshCheek commented Oct 31, 2018

👍

This worked for me!! Ty ty ❤️ I was able to remove my parse_action override, which was explicitly rescuing this error. A few modifications to my test suite (b/c it only raises if you reference request.request_parameters, whereas it would previously fail before making it into the action) and my entire test suite passed with this patch instead of my hack. You can see the diff here

image

I think it would be worth changing the example in the docs to show that you can access the original exception by receiving the parse error and asking for its #cause. I use this to allow my custom params parsers to provide helpful explanations about why the body was invalid. I think it's worth mentioning since I feel like people who want to do this are likely to also likely to want access to the original error, but I don't think it's super well known that you can use Exception#cause to do that.

rescue_from ActionDispatch::Http::Parameters::ParseError do |exception|
  render status: 400, json: { errors: [ exception.cause.message ] }
end

@gmcgibbon
Copy link
Member Author

@gmcgibbon gmcgibbon commented Nov 13, 2018

Any further comments on this @jeremy or @pixeltrix?

@@ -253,7 +253,8 @@ def process_action(*args)
# This will display the wrapped hash in the log file.
request.filtered_parameters.merge! wrapped_filtered_hash
end
super
ensure
return super
Copy link
Member

@pixeltrix pixeltrix Nov 13, 2018

Maybe clarify the behaviour here with a comment explaining that it will rescue all exceptions - it's not a construct that you see every day.

Otherwise 👍

[Gannon McGibbon + Josh Cheek]
@gmcgibbon gmcgibbon force-pushed the parse_error_rescue_2 branch from 20a2a11 to 6b3faf8 Nov 13, 2018
@rafaelfranca rafaelfranca merged commit 9ef26ac into rails:master Nov 14, 2018
2 checks passed
@JoshCheek
Copy link
Contributor

@JoshCheek JoshCheek commented Nov 14, 2018

Ty! 🎉

@gmcgibbon gmcgibbon deleted the parse_error_rescue_2 branch Nov 14, 2018
@JoshCheek
Copy link
Contributor

@JoshCheek JoshCheek commented Dec 21, 2018

Hmm, this is on master, but not released in 5.2.2 Looking at the git tag, the released branch diverged from master on 30 Jan (explanatory comments are in pink comments at the end of each line)

image

The long-running separate branches seem like they're part of some intentional process like git-flow or something like that. But I read RELEASING_RAILS.md and skimmed the contribution guide and don't see an explanation, so figured I'd mention it in case it slipped through the cracks.

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

Successfully merging this pull request may close these issues.

None yet

5 participants