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

Accept header strict #144

Merged
merged 3 commits into from
Feb 19, 2012
Merged

Conversation

rodzyn
Copy link
Contributor

@rodzyn rodzyn commented Feb 14, 2012

Hey,

I was a bit confused about header versioning (for example #133) and I thought maybe it should be more strict.
When no Accept is passed like

curl http://localhost:9292/hello

I had default value */* of accept header, so condition:

(accept.nil? || accept.empty?)

was never true.

BTW.
Test case covered only this scenario:

curl -H "Accept: " http://localhost:9292/hello

I've added few more tests.

@@ -48,6 +48,11 @@ def before
end
end
end

protected
Copy link
Member

Choose a reason for hiding this comment

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

Small note, if ... && options[:versions] is redundant with the if above it. This block is executed within the same if already.

@dblock
Copy link
Member

dblock commented Feb 17, 2012

This is definitely on the right path, but I think we shouldn't be eating an invalid accept header and issuing a 404. Shouldn't there be a 400 failure in this case? If so then I would split the condition and rewrite incorrect_header? as is_accept_header_valid? (adding the accept part cause incorrect_header? is too generic and misleading for a name).

@dblock
Copy link
Member

dblock commented Feb 17, 2012

The RFC says this:

If no Accept header field is present, then it is assumed that the client accepts all media types. If an Accept header field is present, and if the server cannot send a response which is acceptable according to the combined Accept field value, then the server SHOULD send a 406 (not acceptable) response.

So I think the 404 is wrong. It should be a 406 both ways.

@rodzyn
Copy link
Contributor Author

rodzyn commented Feb 19, 2012

Hi, thanks for comments I definitely agree with your thoughts. I've added some changes. What do you think?

@dblock
Copy link
Member

dblock commented Feb 19, 2012

Very good. I'm merging this.

dblock pushed a commit that referenced this pull request Feb 19, 2012
@dblock dblock merged commit d24bd2f into ruby-grape:frontier Feb 19, 2012
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