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

return 400 bad request when contradiction between body and type #134

Merged
merged 3 commits into from Aug 29, 2017

Conversation

sawanoboly
Copy link
Contributor

PostBodyContentTypeParser will stop at 500 error if body and content-type are inconsistent.

e.g. POST as content-type=application/json but passed body foobar.

With this pull request, set the response to 400 and tell the client that the cause of the error is in the request.

@sawanoboly
Copy link
Contributor Author

Note:
This PR doesn't seems easier to use than #76. If #76 will be adopted, you can close this PR.

@mpalmer
Copy link
Contributor

mpalmer commented Aug 11, 2017

Thanks for the PR. I don't think you have to worry too much about #76 superseding this one; it doesn't look like anyone's going to be cleaning that up for merge any time soon.

I'm fine with the intent of your change, but I think your framing of the problem in the tests (thanks for providing them) could be improved. To me, the core of the issue is that the provided content doesn't conform to the rules required of the specified Content-Type. Also, I'm not a fan of putting exception classes in error messages, because they don't mean anything to someone who isn't a Ruby programmer (the heathens!); it would be better to describe the error as something like "failed to parse body as JSON".

If you could tweak the change to incorporate those changes, I'll merge.

@sawanoboly
Copy link
Contributor Author

@mpalmer Thank you for your review 😃 I've updated response message.

@@ -34,11 +34,11 @@
end

describe "contradiction between body and type" do
def assert_bad_request(response, err_class)
def assert_failed_to_parse_as_json(response, err_class)
Copy link
Contributor

Choose a reason for hiding this comment

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

So, err_class is no longer actually used in the assertion... can probably take that out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I had overlooked it 😓 I removed it and pushed.

@mpalmer
Copy link
Contributor

mpalmer commented Aug 17, 2017

Teeny tiny little nit picked...

@mpalmer mpalmer merged commit 624bf9b into rack:master Aug 29, 2017
@mpalmer
Copy link
Contributor

mpalmer commented Aug 29, 2017

OK, I've merged this in. It'll be in the next release, slated for the first of next month. Thanks for your contribution!

@sawanoboly sawanoboly deleted the return_400_for_json_parseerror branch August 29, 2017 06:18
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.

None yet

2 participants