Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Add Rack::Utils.q_value #443

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
4 participants

What

This pull request adds Rack::Utils.q_value, which accepts a q-value-style header and returns an ordered Array of [value, quality] pairs.

Rationale

The q-value-style header is common in HTTP requests, so many types of Rack middleware will be interested in parsing it. Additionally, it's easy to get wrong, as rack-contrib/locale.rb does. Having one central method will help many middleware authors do the right thing.

Caveat

This pull request also changes Rack::Request#accept_encoding to use this new method. Doing so changes its behavior in that it sorts the entries by quality, descending. This doesn't change the meaning of #accept_encoding (since anyone who was using it should have been sorting on quality themselves), but it is possible that it would break clients. If there is concern that this is a breaking change, I will gladly revert this second commit.

@gioele gioele and 1 other commented on an outdated diff Oct 18, 2012

lib/rack/utils.rb
@@ -166,6 +166,20 @@ def build_nested_query(value, prefix = nil)
end
module_function :build_nested_query
+ def q_values(q_value_header)
+ q_value_header.to_s.split(/\s*,\s*/).map do |part|
+ value, parameters = part.split(/\s*;\s*/, 2)
+ quality = 1.0
+ if parameters and /\Aq=([\d.]+)/ =~ parameters
+ quality = $1.to_f
@gioele

gioele Oct 18, 2012

Contributor

Could you avoid the use of the variable $1? Some Ruby VM have problems with it (killed performance or wrong values in threaded environments). See https://news.ycombinator.com/item?id=4658933.

@jamesarosen

jamesarosen Oct 18, 2012

Good idea. I'll replace with a MatchData.

Contributor

gioele commented Oct 18, 2012

This method would be really useful. With it I could simplify some code in my rack-18n_best_langs middleware component and I think many other gems could get rid of their half-baked code.

@rack @chneukirchen any thoughts?

Owner

raggi commented Nov 2, 2012

The ordering change likely has more relevance when dealing with q values that are equal, where it will change the left to right order. It's not ideal for folks to rely on that, and it only matters when the precedence of the selectors is also equal.

Talking of precedence, this talk also does not take that into account.

Are you saying that the sort algorithm isn't stable? I'm fairly sure sort_by is stable.

Member

rkh commented Nov 2, 2012

The content negotiation implementation is incomplete, btw, both in the current version and this patch. q is not the only value used to determine order.

@rkh are you referring to level? Is that used anywhere?

Member

rkh commented Nov 2, 2012

It's not just level, the spec is using level as example. "Media ranges can be overridden by more specific media ranges or specific media types.", more parameters = more specific, more asterisks = less specific.

@rkh can you give some examples? I'm happy to improve the implementation, but I don't really know what I'm coding for. Also: is this only true for the Accept header? I had meant this utility to be useful for all q-value headers. Is that generalization not true to the spec?

Owner

raggi commented Nov 2, 2012

@jamesarosen if the desire is for the user to be able to rely on order such that they can select the first appropriate option using #find, then we'll need to order by specificity too.

Additionally, if #find were to be particularly convenient, a matcher would be required. We could add one to Rack::Mime or Rack::Utils.

The resultant use case might look something like this:

Rack::Utils.q_value(env['header']).find do |(mime, quality)|
  Rack::Mime.match?(mime, 'text/thing')
end

The expectation would be that #match? would return true if mime was '' or '/' or 'text/' or 'text/thing'

Owner

raggi commented Nov 2, 2012

If we go the route of adding a match helper, it may also make sense to have it accept a variable argument list of acceptable values match?(value, option1, option2, option3)

In an early draft of this work, I had considered creating a value object to represent the list. Something like QValues.new( header_content ), which would be enumerable and could have the best_match(options) functionality you're talking about. It seemed like overkill at the time, but I'm happy to bring it back.

@raggi raggi added a commit that referenced this pull request Jan 11, 2013

@raggi raggi Introduce Rack::Mime.match?, references #443 b69d3e7

@raggi raggi closed this in 29d5d6f Jan 11, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment