Use 415 status for request bodies of unsupported media type #1765
Conversation
|
That is probably correct, but is a backwards incompatible change. At the very least it needs to have a section in https://github.com/ruby-grape/grape/blob/master/UPGRADING.md, please. |
spec/grape/endpoint_spec.rb
Outdated
| @@ -948,7 +948,7 @@ def app | |||
| params[:user] | |||
| end | |||
| put '/request_body', '<user>Bobby T.</user>', 'CONTENT_TYPE' => 'application/xml' | |||
| expect(last_response.status).to eq(406) | |||
| expect(last_response.status).to eq(415) | |||
There was a problem hiding this comment.
The description of this test is now incorrect.
|
Does this need more test scenarios? With or without body for example? |
|
@dblock, I agree it could probably use more tests. Do you have a preference/idea of where you want them. The tests that failed are actually just in a Also, I can update the UPGRADING document, but where should I put it (in terms of heading)? Should I assume a 2.x release? Will this need to sit in a 2.x feature branch until any other planned or unplanned non-passive changes are in? |
|
No strong opinions on test locations, but definitely in specs :) Maybe something new. It doesn't need to sit in a branch, just keep updating the PR. I think we should do 1.1, Grape hasn't been using the major version, we've spent years getting to 1.0. Appreciate any thoughts on that too, maybe this is where we just use semantic versioning properly? |
|
This looks good. Maybe squash it if you don't mind? I want to leave this open for a bit, maybe @namusyaka or @dm1try could pitch in that we want to do this? |
|
@dblock, squashed all the things. |
|
as for versioning
I agree, up to 1.1.0 and add "incompatibility" note in UPGRADING.md Grape does not claim that it uses SemVer, so for such incompatible fix is fine :) (moreover, this way is common in Ruby/Rails community itself, at least for now) |
lib/grape/middleware/formatter.rb
Outdated
| @@ -95,7 +95,7 @@ def read_rack_input(body) | |||
| fmt = request.media_type ? mime_types[request.media_type] : options[:default_format] | |||
|
|
|||
| unless content_type_for(fmt) | |||
| throw :error, status: 406, message: "The requested content-type '#{request.media_type}' is not supported." | |||
| throw :error, status: 415, message: "The requested content-type '#{request.media_type}' is not supported." | |||
There was a problem hiding this comment.
nitpicking, but I would change the message (ex. from "requested" to "provided") to clarify the difference between 406/415
There was a problem hiding this comment.
Updated, rebased and pushed as a single commit
|
@jdmurphy can you please do the version bump and the upgrading part of this? thx |
|
@dblock when you say do the version bump, what did you mean? Like update the version file to be 1.1.0 and update the changelog and upgrading doc to reflect the new version there as well? |
|
I updated the version file, README, UPGRADING and CHANGELOG, and then rebased and pushed. |
|
Merged. Great work @jdmurphy and thanks! |
Noticed that for requests where bodies are present (i.e. PUT, POST, PATCH, etc.), that if the content type header was something unsupported that a 406 would be returned. Reading over the original RFC 2616, a 415 should be used when a request entity is not in a supported format, but currently a 406 is returned. It seems like it should be as simple as changing this logic.