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

Ordered query_values #24

Closed
igrigorik opened this issue Jan 19, 2011 · 13 comments
Closed

Ordered query_values #24

igrigorik opened this issue Jan 19, 2011 · 13 comments

Comments

@igrigorik
Copy link
Contributor

Under 1.8.x, it would be nice to preserve the order of the query values, such that the query string can be reconstructed without changing the signature of the URL.

Under 1.9, ruby hashes guarantee insertion order, so.. parsing the string will result in behavior above, but this breaks under 1.8. OrderedHash under 1.8? Having this as an option, or transparent would be nice.

Ex: have a use case where we need to parse the QS, delete a few variables, and re-assemble it. After re-assembling it, we need to guarantee that we'll end up with same order of parameters between different processes.

@sporkmonger
Copy link
Owner

Yeah, I strongly agree. A fix will probably come in the next major release though, because my solution probably will be to change the return value to:

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

This format both preserves order as well as allows for handling query strings with multiple occurrences of a single key as above. But because it's a breaking change, it probably won't land until a major version release. I might make it available under a branch earlier though.

@sporkmonger
Copy link
Owner

Actually, bvandenbos just sent me a pull request that resolves this.

@sporkmonger
Copy link
Owner

Note that his patch requires you to explicitly indicate you want a :flat_array. In the future this will probably become the only format the method returns, but for now you can indicate you want it as an alternative notation.

@igrigorik
Copy link
Contributor Author

Awesome - thanks!

@igrigorik
Copy link
Contributor Author

Hmm, almost.. There are still two problems:

hash.sort.inject([]) do |accu, (key, value)|

new_query_values.sort! # Useful default for OAuth and caching

Need to drop the .sort call on 1422, and remove 1503 (or make optional / conditional)

        url = 'http://a.com/?'+('a'..'z').to_a.shuffle.map {|e| "#{e}=#{e}"}.join("&")

Above case should result in same url post query_values parse + query_parse=

@sporkmonger
Copy link
Owner

I was actually going to ask bvandenbos for some test cases to prove he'd gotten the sort order correct. But then I was lazy and didn't. Oh, well, as least my test-completeness instincts are working. I'll fix it right now.

@sporkmonger
Copy link
Owner

Bother. I just pushed a pair of test cases that cover some of the weird edge cases in some of the notation forms that addressable currently parses. Dropping the .sort call makes these tests fail in ruby 1.9.x. The .sort call on line 1422 is necessary. However I moved the sorting code on line 1503 up into the non-Array input block, so it will only sort if you give it a Hash input. Admittedly, this might be a bit aggressive in 1.9.x where order is preserved in Hash values, but you'll always have the Array option available if it matters. I'm not concerned about the occasional instance where someone has to do an extra .to_a call on their 1.9.x Hash to ensure order is preserved.

@igrigorik
Copy link
Contributor Author

Why is 1422 necessary? Without removing that sort, can't make this pass:

https://gist.github.com/787989

@sporkmonger
Copy link
Owner

Hmmm. Looks like the .sort call is having an unintentional side-effect. The intent was to sort Hash values, not Array values. For instance: ?a[1]=one&a[0]=zero should parse to {'a' => ['zero', 'one']}.

@sporkmonger
Copy link
Owner

Actually, I added your test and the unmodified code passed just fine. Please take another look.

@igrigorik
Copy link
Contributor Author

Interesting! You're right, works just fine. Oi! :-)

In which case, what are the odds of a point release to get this out in the wild?

@sporkmonger
Copy link
Owner

Done.

@igrigorik
Copy link
Contributor Author

Sweet, thanks Bob!

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

No branches or pull requests

2 participants