Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Should return an array for multiple values #91

Closed
wants to merge 1 commit into from

2 participants

@cfabianski

No description provided.

@sporkmonger
Owner

I think the bulk of this commit is good. I could be easily convinced that the right behavior is to return a hash containing a list when you have repeated values. I need to spend some more time with RFC 6570 to be really sure about this, but I think it's very reasonable. However, what I can't accept is the special treatment of square brackets. I've taken a hardline stance on this convention and I'm strongly against the use of this particular syntax. It is not advocated in any standards, and the fact that square brackets must be encoded in normal form leads to inelegant, error-prone code when trying to process them. It's a terrible way to solve something that has a much more obvious and vastly superior solution in the form of simple repeated parameters and flatter structures.

If you're going to use square bracket syntax, you need to include the brackets in the key value. I'm going to be exceedingly stubborn on this point.

@cfabianski

I did restore the behavior until 2.2.8 which was more respectful of this RFC.
Why do I need to include the brackets in the key value? If I want an array, the key should be without.

If I let the brackets in the key value, then it will screw up a lot of framework including Rails

@cfabianski

A lot of library rely onto addressable (oauth-provider2 for instance) I kinda agree with your point (square bracket) but it does exist, you can't just drop it

@sporkmonger
Owner

Nah, that kind of logic is what results in legacy software. I'm a firm believer in throwing away the stuff that sucks, compatibility be damned. And then following it up with lots of outreach to try to convince the rest of the world to stop doing things that suck.

@sporkmonger sporkmonger referenced this pull request from a commit
@sporkmonger Added pending test for #91. fab8ab2
@sporkmonger sporkmonger referenced this pull request from a commit
@sporkmonger Added pending test for #91. 77ec678
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Nov 8, 2012
  1. @cfabianski
This page is out of date. Refresh to see the latest.
Showing with 24 additions and 4 deletions.
  1. +9 −2 lib/addressable/uri.rb
  2. +15 −2 spec/addressable/uri_spec.rb
View
11 lib/addressable/uri.rb
@@ -1428,6 +1428,8 @@ def query=(new_query)
# #=> [["one", "two"], ["one", "three"]]
# Addressable::URI.parse("?one=two&one=three").query_values(Hash)
# #=> {"one" => "three"}
+ # Addressable::URI.parse("?one[]=two&one[]=three").query_values(Hash)
+ # #=> {"one" => ["two", "three"]}
def query_values(return_type=Hash)
empty_accumulator = Array == return_type ? [] : {}
if return_type != Hash && return_type != Array
@@ -1435,7 +1437,7 @@ def query_values(return_type=Hash)
end
return nil if self.query == nil
split_query = (self.query.split("&").map do |pair|
- pair.split("=", 2) if pair && !pair.empty?
+ pair.split(%r{\[\]=|=}, 2) if pair && !pair.empty?
end).compact
return split_query.inject(empty_accumulator.dup) do |accu, pair|
# I'd rather use key/value identifiers instead of array lookups,
@@ -1449,8 +1451,13 @@ def query_values(return_type=Hash)
# If it ain't broke, don't fix it!
pair[1] = URI.unencode_component(pair[1].to_str.gsub(/\+/, " "))
end
+
if return_type == Hash
- accu[pair[0]] = pair[1]
+ accu[pair[0]] = if accu[pair[0]]
+ [ accu[pair[0]] ].push(pair[1]).flatten
+ else
+ pair[1]
+ end
else
accu << pair
end
View
17 spec/addressable/uri_spec.rb
@@ -3790,17 +3790,30 @@ def to_s
end
it "should have correct query values" do
- @uri.query_values(Hash).should == {"one[two][three][]" => "five"}
+ @uri.query_values(Hash).should == {"one[two][three]" => ["four", "five"]}
end
it "should have correct array query values" do
@uri.query_values(Array).should == [
- ["one[two][three][]", "four"], ["one[two][three][]", "five"]
+ ["one[two][three]", "four"], ["one[two][three]", "five"]
]
end
end
describe Addressable::URI, "when parsed from " +
+ "'?one[]=two&one[]=three&one[]=four'" do
+ before do
+ @uri = Addressable::URI.parse(
+ "?one[]=two&one[]=three&one[]=four"
+ )
+ end
+
+ it "should have correct hash query values" do
+ @uri.query_values.should == {"one" => ["two", "three", "four"]}
+ end
+end
+
+describe Addressable::URI, "when parsed from " +
"'?one[two][three][0]=four&one[two][three][1]=five'" do
before do
@uri = Addressable::URI.parse(
Something went wrong with that request. Please try again.