to_query used on hash's don't keep order #10529

Closed
hughdavenport opened this Issue May 9, 2013 · 19 comments

Comments

Projects
None yet
9 participants

There is an issue in the to_query function (defined in https://github.com/rails/rails/blob/master/activesupport/lib/active_support/core_ext/object/to_query.rb) for Hash objects.

When to_query is used on an Array, the order of the input array is kept, while when used on a Hash, the order is changed to be lexographical, not what is input (the Hash.to_query is aliased to to_param, see https://github.com/rails/rails/blob/master/activesupport/lib/active_support/core_ext/object/to_param.rb)

You can see this by the following:

1.9.3p392 :001 > require 'active_support/core_ext/object/to_query'
 => true 
1.9.3p392 :002 > ['c', 'a', 'b'].to_query('foo')
 => "foo%5B%5D=c&foo%5B%5D=a&foo%5B%5D=b" 
1.9.3p392 :003 > {'c' => 'b', 'a' => 'c', 'b' => 'a'}.to_query('foo')
 => "foo%5Ba%5D=c&foo%5Bb%5D=a&foo%5Bc%5D=b" 

Here, the ['c', 'a', 'b'].to_query('foo') goes to "foo[]=c&foo[]=a&foo[]=b" url escaped. Note that the order of 3 1 2 is kept.
However, when the input is {'c' => 'b, 'a' => 'c', 'b' => 'a'}.to_query('foo'), the output is "foo[a]=c&foo[b]=a&foo[c]=b" url escaped. Note that the order of the original hash keys was c a b, but the output has lexographical of a b c.

I would say this is a bug, as it is inconsistent with the Array to_query, and there is no standard saying that url param queries must be in lexographical order. Ruby hashes keep order, so why break it?

A fix for this is at https://gist.github.com/hughdavenport/5545117

Cheers,

Hugh

Contributor

gaurish commented May 9, 2013

query strings are always fetched via key name, so why does order matter?

Contributor

aditya-kapoor commented May 9, 2013

It has been mentioned in the documentation for Hash#to_query that the keys would be arranged in the lexographical style and I guess it is the way it should work..

@gaurish You may fetch the query strings by a params.each statement
@aditya-kapoor Yup, I noticed that, this bug was merely as there is a difference between the Array and the Hash invocations. If it should be ordered lexographically, then why not both? and why does it need to be lexographical? as that is just extra work.

All this said, I am aware that the way I am using this is an unusual scenario, but yeh... I would say they should either both be sorted, or both not.

Hugh

Owner

rafaelfranca commented May 13, 2013

This seems more a feature request that a bug. So I'll close this issue. You can discuss this on the Rails Core mailing list, or open a pull request changing the behavior explaining why.

Thanks

@rafaelfranca Are you sure this would be a feature? Seems like because Array and Hash both have different invocations the standard behaviour is flawed.

Hi @rafaelfranca,

I would be interested in why you think this is a feature request. The Array and Hash invocations of the same method have different semantics which can lead to inconsistencies. I have put a patch on this request, if you would like I can make a pull request out of this. If you feel that this is still a feature because you are removing the lexographical ordering which seems to be placed there for a reason, then I can make a new patch that adds lexographical ordering to the Array incantation.

I forgot to mention in the original report that Hash's in ruby have a default ordering of the order they are inserted into the Hash (http://ruby-doc.org/core-2.0/Hash.html "Hashes enumerate their values in the order that the corresponding keys were inserted."), so why you want to change this order I don't know.

If you think about it this way, if you have a large hash, and then need to sort it just to be able to generate a query string that reduces performance from that of just using the hash's natural ordering.

Let me know.

Cheers,

Hugh

Owner

rafaelfranca commented May 13, 2013

It is not a bug for me since it is documented that the result is ordered lexographically, and the behavior matches the documentation.

Actually I think that ordering was only to make predictable the result of to_query in ruby 1.8. Since Hashes are ordered in Ruby 1.9 we can safety remove that sort! call.

@rafaelfranca hmm, the ruby 1.9 hash's are ordered by the order you put it in, not lexographically. But yeh, I agree with predictable results. If you aren't happy with changing the Hash to take away the .sort!, then can I suggest that we add the .sort! to the Array version, as that would have inconsistent results compared to a Hash

The order of query parameters IS significant, even if you saw some software that ignores it during parsing. For example:

  • Equivalent URLs sometimes have a canonical/normalized form (e.g. to make visual comparisons easier)
  • When a human sees a URL, a certain order might be natural or more readable
  • Very long URLs often get truncated in certain contexts, so the most important query parameters (e.g. the database key) should come first so they aren't the part that gets truncated

Sorting the keys might be useful, and it might even be a reasonable default, but it's presumptuous for an API to completely remove the ability to specify the order! A bad decision was made here.

+1 to this problem. I cannot find a way without manually looping and building my own query to maintain the order of key value pairs. This is important to me for what I am doing, and this assumption breaks it.

This is just the beginning of your trouble with with Ruby's URI class, which turns out to be a complete turd. I finally solved this problem by switching to the gem from addressable.rubyforge.org, which is better designed, not riddled with bugs, and also gives you support for international URLs.

Here's some discussion about its (much better!) approach to query parameters:

http://addressable.rubyforge.org/api/Addressable/URI.html#query_values-instance_method

sporkmonger/addressable#77

Thanks for the heads up, I'll look at using that instead!

@rafaelfranca Can you please re-open this Bug seems to still be very active with people running into the same problem.

Owner

rafaelfranca commented Feb 10, 2014

My answers remains:

It is not a bug for me since it is documented that the result is ordered lexographically, and the behavior matches the documentation.

Actually I think that ordering was only to make predictable the result of to_query in ruby 1.8. Since Hashes are ordered in Ruby 1.9 we can safety remove that sort! call.

If you want to change this behavior feel free to open a pull request, but I'll not reopen this issue since it is not a bug.

My response which was never answered remains...

Yup, I noticed that, this bug was merely as there is a difference between the Array and the Hash invocations. If it should be ordered lexographically, then why not both? and why does it need to be lexographical? as that is just extra work.

All this said, I am aware that the way I am using this is an unusual scenario, but yeh... I would say they should either both be sorted, or both not.

I would open a pull request, but I find that with this community that when I offer a suggestion to fix what myself and a whole lot of other users think is a bug, it gets shut down. The point of an open source community is to allow friendly collaboration. If you would like to discuss why you don't find what I just wrote above is not a bug, could you? Otherwise why comment at all, just leave the bug tracker stale.

Hugh

Owner

rafaelfranca commented Feb 11, 2014

Sorry but I think you are being a little extreme here.

We all already agree this behavior can be changed. So nobody is shutting down the change. I just can't and will not leave this issue open until someone get time to work on the patch because it is not a bug. It is change of documented behavior.

This issue tracker is not for text suggestion, it is for real bugs and for implemented suggestions. Your text suggestion is welcome in the Rails Core mailing list. This is stated in the contributing guide of this project, linked in the "new issue" page.

I also already answered your question in the comment before you did it:

Actually I think that ordering was only to make predictable the result of to_query in ruby 1.8. Since Hashes are ordered in Ruby 1.9 we can safety remove that sort! call.

So, answering your question again, if you like to change this behavior I'd remove the lexicographic sort.

@rafaelfranca ah sorry xD, I read that as basically closing the bug completely. My bad for skim reading. Differences in using bug trackers for projects I guess (most of the ones I work with have features as well as bugs on trackers) :D

I may actually send a patch to the list. I fixed it somehow in my codebase.

mollat commented Mar 2, 2016

Just for the files, you can solve it like this:

query = params.map{|k, v| CGI::escape(k.to_s) + '=' + CGI::escape(v)}.join('&')

mockdeep commented Sep 9, 2017

Looks like this is the cause of this other issue. Sorting the query params before joining changes the way that Rack::Utils.parse_nested_query processes them and ends up producing different results than if they weren't sorted.

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