Support symbols in the query_values hash

[jasonkarns]

Given:

uri = Addressable::URI.parse("http://www.google.com/?test=original")
uri.query_values = uri.query_values.merge :test => "new"

Instead of getting => http://www.google.com/?test=new, I get:

ArgumentError: comparison of Array with Array failed
        from /Users/user/.rbenv/versions/2.0.0-p0/lib/ruby/gems/2.0.0/gems/addressable-2.3.4/lib/addressable/uri.rb:1546:in `sort!'
        from /Users/user/.rbenv/versions/2.0.0-p0/lib/ruby/gems/2.0.0/gems/addressable-2.3.4/lib/addressable/uri.rb:1546:in `query_values='
        from (irb):16
        from /Users/user/.rbenv/versions/2.0.0-p0/bin/irb:12:in `<main>'

Should be a simple matter of converting all the hash keys (and values too?) to string before doing anything else?

[sporkmonger]

This no longer gives an error in the 3.0 release branch, however, it also does not do what you'd probably expect. Since query_values does not (and will not) provide indifferent access, merge produces {"test"=>"original", :test=>"new"}. I could be persuaded that query_values= should be more intelligent about how it handles an assignment with mixed-type keys where two distinct keys resolve to the same string. And that would give you the behavior you're looking for.

[sporkmonger]

See also #94.

[Mifrill]

more intelligent

Sounds nice, it looks like for now this is something like wrong behavior:

[1] pry(main)> require "addressable"
=> true
[2] pry(main)> uri = Addressable::URI.parse("http://www.google.com/?test=original")
=> #<Addressable::URI:0x2b173ec18b28 URI:http://www.google.com/?test=original>

[3] pry(main)> uri.query_values
=> {"test"=>"original"}

[4] pry(main)> old_uri = uri
=> #<Addressable::URI:0x2b173ec18b28 URI:http://www.google.com/?test=original>

[5] pry(main)> uri.query_values = uri.query_values.merge test: "new"
=> {"test"=>"original", :test=>"new"}

[6] pry(main)> uri.to_s
=> "http://www.google.com/?test=new&test=original"

With regard to Ruby, looks correctly:

[7] pry(main)> :new != "new"
=> true

But, with regard to external API that will gather this string and will continue to work with this, it seems nope, because:

[8] pry(main)> Addressable::URI.parse(uri.to_s).query_values
=> {"test"=>"original"}

There is other way:

[9] pry(main)> old_uri.query_values = old_uri.query_values.merge "test" => "new"
=> {"test"=>"new"}

[10] pry(main)> old_uri.to_s
=> "http://www.google.com/?test=new"

I mean, i think, it should be same for both cases.

Besides, what about make shortly, something like this:

uri.query_values.merge(test: => "new")

without uri.query_values= Expect

uri.to_s => "http://www.google.com/?test=new"

I think, it would be more convenient

[vfonic]

Thanks for the detailed writeup @Mifrill!

I agree with what @Mifrill wrote. (I'd only change merge to merge! in the last example: uri.query_values.merge!(test: => "new") # bang!)

I could be persuaded that query_values= should be more intelligent about how it handles an assignment with mixed-type keys where two distinct keys resolve to the same string. And that would give you the behavior you're looking for.

I think this is Addressable's main functionality: "everything URI" That's schema, host, port, parameters, hash...

:test and "test", from URI's point of view, are the same thing. We should convert any parameter to string and decide, for consistent behavior to either:

1. Have them join so there are two parameters: ?test=asd&test=dsa
2. Have the latest overwrite all/any before: ?test=dsa

I'd vote for option 2.

This is also weird issue:

uri.query_values['test'] = 'dsa'
# => 'dsa'
uri.query_values
# => no "test" key in sight
uri.query_values.key? 'test'
# => false

This should at least raise an error.

It's come up before and as before, the answer is essentially no, won't fix. If Ruby adds an indifferent access hash structure to the standard library, I'll revisit that answer.

I'm not sure why ruby adding indifferent access would make the bad pattern any better. 🤔 It would just be more "standard" as opposed to "de-facto standard" that it became with rails.

until then, I prefer for this kind of usage to be explicitly controlled by the user of the library

uri = Addressable::URI.parse('https://a.com/?')
uri.query_values
# => {}
uri.query_values.class
# => Hash < Object
uri.method(:query_values=)
# => Addressable::URI#query_values=(new_query_values)
# now that we know it's a hash, we can assign it a hash, and it will...keep that hash?
# `uri.query_values` will "obviously" return the same hash, right?
 uri.query_values = {asd: :dsa}
# => { asd: :dsa }
# so we got back the hash we assigned?
uri.query_values
# => { "asd" => "dsa" }
# no, we were shown the hash we assigned, only to find out it changed when we tried to read it back

Both key and value got converted to strings, but instead of knowing that immediately, we find out when stuff breaks down the road. Reminds me of nil issue: https://www.destroyallsoftware.com/screencasts/catalog/how-and-why-to-avoid-nil

uri = Addressable::URI.parse('https://a.com/?')
uri.query_values = uri.query_values.merge(asd: :dsa)
# => { asd: :dsa }
uri.query_values[:asd]
# => nil

Here's some more odd behavior:

uri = Addressable::URI.parse('https://a.com/?')
# it's odd to have to add '?' to get empty hash (otherwise nil) for query_values
# while at it, I'd also rename `query_values` for v3
# as it contains both query keys _and_ values /smartass 😇 

Not necessarily odd, but I'd prefer to always get back empty hash:

uri = Addressable::URI.parse('https://a.com/')
uri.query_values
# => nil
uri = Addressable::URI.parse('https://a.com/?')
uri.query_values
# => {}

Because:

uri = Addressable::URI.parse(arbitrary_uri_from_client)
uri.query_values = uri.query_values.merge(asd: :dsa) # => NoMethodError: undefined method `merge' for nil:NilClass

Many times this is addressed like this:

u.query_values = (u.query_values || {}).merge(additional)

I find it a bit odd API to do something many people will do with the URI. I guess I'm not the only one: https://exceptionshub.com/append-query-string-to-url.html

I also can't find anything in the RFC that suggests empty and not-specified query couldn't (in ruby or otherwise) be represented in the same way...apart from this little snippet that says if there's not '?', it should be undefined (no mention of what's the value when only the '?' is there): https://tools.ietf.org/html/rfc3986#page-51 In terms of the outputted URI, both query_values == nil and query_values == {} could output the same thing.

Another one:

uri = Addressable::URI.parse('https://a.com/?asd=in_url')
uri.query_values[:asd] = 'assigned'
# => 'assigned' # looks like it worked!
uri.query_values[:asd]
# => nil

uri.query_values['asd']
# => 'in_url' # ah, I know! I should assign to string!
uri.query_values['asd'] = 'assigned'
# => 'assigned'
uri.query_values['asd']
# => 'in_url'

Another one:

uri = Addressable::URI.parse('https://a.com/?asd=in_url')
uri.query_values = uri.query_values.merge('asd' => 'dsa')
# => { "asd" => "dsa" }
# BUT!
uri = Addressable::URI.parse('https://a.com/?asd=in_url')
uri.query_values = uri.query_values.merge(asd: 'dsa')
# ArgumentError: comparison of Array with Array failed
# a what with what did what? 😄

Sorry for the biblically long comment! :D

I suggest two things:

1. Default query_values to empty hash when there's no arguments and keep query_values as hash internally, not as a string query
2. Always immediately convert query params (both key and value) to how they will be represented in the final outputted URI

Thanks!

[vfonic]

One more thing:

uri = Addressable::URI.parse('https://ex.com/?')
uri.query_values = uri.query_values.merge(op: 5)
# => { op: 5 }
uri.query_values = uri.query_values.merge(op: 5)
# => ArgumentError: comparison of Array with Array failed
# ok, so when there's a symbol key and I add the same symbol key, Array-Array error is raised

uri = Addressable::URI.parse('https://ex.com/?')
uri.query_values = uri.query_values.merge(op: '5')
# => { op: "5" }
uri.query_values = uri.query_values.merge(op: '5')
# => { "op" => "5", op: "5" }
# ok, so when there's a symbol key and I add the same symbol key, Array-Array error is raised
uri.query_values = uri.query_values.merge(op: '5')
# => { "op" => "5", op: "5" }
uri.query_values
# => { "op" => "5" }