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

Type mismatch causes Addressable::URI#query_values= to fail on sort! with "comparison of Array with Array failed" #94

Open
rehevkor5 opened this issue Dec 13, 2012 · 12 comments
Assignees
Labels
Milestone

Comments

@rehevkor5
Copy link

If the data passed to query_values= results in a type mismatch in the value of new_query_values on line 1504 when .sort! is called, such as:

[['a', '1'], ['a', 2]]

the method will fail with ``sort!': comparison of Array with Array failed`

I'd submit a PR fix myself, but I'm not sure what the desired solution is.

@sporkmonger
Copy link
Owner

What version of Addressable are you using? I can't duplicate.

@rehevkor5
Copy link
Author

2.3.2

Maybe need to make sure you're passing a hash into query_value=

@sporkmonger
Copy link
Owner

Nope, still can't duplicate. Provide a minimal working example of the bug please.

@rehevkor5
Copy link
Author

Try this:

1.9.3p194 :002 > require 'addressable/uri'
 => true 
...
1.9.3p194 :006 > uri = Addressable::URI.parse('http://example.com/?page=1')
 => #<Addressable::URI:0x3fd3f85b8b30 URI:http://example.com/?page=1> 
1.9.3p194 :007 > uri.query_values = uri.query_values.merge(:page => 2)
ArgumentError: comparison of Array with Array failed
    from /Users/shannoncarey/.rvm/gems/ruby-1.9.3-p194@hermes-frontend/gems/addressable-2.3.2/lib/addressable/uri.rb:1504:in `sort!'
    from /Users/shannoncarey/.rvm/gems/ruby-1.9.3-p194@hermes-frontend/gems/addressable-2.3.2/lib/addressable/uri.rb:1504:in `query_values='
    from (irb):7
    from /Users/shannoncarey/.rvm/gems/ruby-1.9.3-p194@hermes-frontend/gems/railties-3.2.9/lib/rails/commands/console.rb:47:in `start'
    from /Users/shannoncarey/.rvm/gems/ruby-1.9.3-p194@hermes-frontend/gems/railties-3.2.9/lib/rails/commands/console.rb:8:in `start'
    from /Users/shannoncarey/.rvm/gems/ruby-1.9.3-p194@hermes-frontend/gems/railties-3.2.9/lib/rails/commands.rb:41:in `<top (required)>'
    from script/rails:6:in `require'
    from script/rails:6:in `<main>'

@csaura
Copy link

csaura commented Feb 1, 2013

The problem here is query_values always returns a hash keys and values as strings after the parse, and then {"page" => "1"}.merge({:page => 2}) --> {"page" => "1", :page => 2}

that code in lib/addressable/uri.rb:197

new_query_values = new_query_values.to_hash
new_query_values = new_query_values.map do |key, value|
  key = key.to_s if key.kind_of?(Symbol)
  [key, value]
 end

The new_query_values has [["page", "1"], ["page", 2]] and when use new_query_values.sort! first sort by first value (the keys) and if are equal, sort by the second values (the hash values), but you can't compare String with Integer.

@sporkmonger
Copy link
Owner

Could you write a test that reproduces this problem?

@rehevkor5
Copy link
Author

I'll have one for you shortly.

rehevkor5 pushed a commit to rehevkor5/addressable that referenced this issue Feb 4, 2013
@rehevkor5
Copy link
Author

See associated PR.

sporkmonger added a commit that referenced this issue Feb 4, 2013
@sporkmonger
Copy link
Owner

Thank you, that helps a lot.

@dmitry
Copy link

dmitry commented Jun 13, 2016

Had the same issue, solved by stringify_keys; but I think this one should be solved inside addressable it self.

@sporkmonger
Copy link
Owner

@dmitry 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, but until then, I prefer for this kind of usage to be explicitly controlled by the user of the library, exactly as you're doing. I strongly argue that principle of least surprise was violated when Rails introduced this pattern in the first place and I reject the idea that once they made bad behavior commonplace that suddenly good behavior became "surprising".

@dmitry
Copy link

dmitry commented Jun 16, 2016

@sporkmonger I agree, but in that case the small note should be added to the documentation, as keys should be strings, and not symbols because of the ambiguous while sorting and forming the query. Somewhere here:

https://github.com/sporkmonger/addressable/blob/master/lib/addressable/uri.rb#L1607

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants