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

Quality arguments to Accept header should default to 1.0 #2366

Merged
merged 8 commits into from
Nov 7, 2023
Merged

Quality arguments to Accept header should default to 1.0 #2366

merged 8 commits into from
Nov 7, 2023

Conversation

hiddewie
Copy link
Contributor

@hiddewie hiddewie commented Nov 2, 2023

This PR ensures that Accept headers with mime types without a given quality value, are parsed as having a quality value of 1.0.

The current value for mime types without a quality value is 0.0 (nil.to_f).

From the HTTP specification (RFC 9110), https://www.rfc-editor.org/rfc/rfc9110.html#name-accept and https://www.rfc-editor.org/rfc/rfc9110.html#name-quality-values:

The weight is normalized to a real number in the range 0 through 1, where 0.001 is the least preferred and 1 is the most preferred; a value of 0 means "not acceptable". If no "q" parameter is present, the default weight is 1.

@hiddewie hiddewie marked this pull request as ready for review November 2, 2023 15:31
@hiddewie hiddewie changed the title Quality arguments to Accept header should default to 1.0 Quality arguments to Accept header should default to 1.0 Nov 2, 2023
lib/grape/middleware/formatter.rb Outdated Show resolved Hide resolved
Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

Add a test that with nil, 0.0, and invalid, too?

@hiddewie
Copy link
Contributor Author

hiddewie commented Nov 3, 2023

Thanks for the review. I changed the parsing to include all q=... values. This will try to parse every value to a floating point number. Invalid (textual) values will be parsed as 0.0. The empty q value will become a default quality of 1.0 which looks like a fine behaviour for me.

@hiddewie hiddewie requested a review from dblock November 3, 2023 20:34
@dblock
Copy link
Member

dblock commented Nov 7, 2023

Add to CHANGELOG and this is good to go!

@hiddewie
Copy link
Contributor Author

hiddewie commented Nov 7, 2023

Thanks, done!

Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

One more, should we also document this in README? We don't need to explain the entire purpose of q, but maybe call it out in https://github.com/ruby-grape/grape#header with an example that chooses JSON over XML? Maybe something like this?

"Grape will evaluate the relative quality preference included in Accept headers and default to a quality of 1.0 when omitted. In the following example a Grape API that supports XML and JSON in that order will return JSON."

curl ... -H "Accept: text/xml;q=0.8, application/json;q=0.9"

CHANGELOG.md Outdated Show resolved Hide resolved
@hiddewie
Copy link
Contributor Author

hiddewie commented Nov 7, 2023

Sounds good, done in 7a6cefc!

@hiddewie hiddewie requested a review from dblock November 7, 2023 15:55
@dblock dblock merged commit 32b8d22 into ruby-grape:master Nov 7, 2023
31 checks passed
@dblock
Copy link
Member

dblock commented Nov 7, 2023

Merged, nice work, thanks for hanging in here with me!

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