Sorting query parameters undesireable #245

Merged
merged 2 commits into from Jan 4, 2014

2 participants

@Billiam

Some services return results based on the query parameter order (ex: mapquest's batch query api), and sorting may break input/output mapping unexpectedly.

Patch removes the sort call from the NON_RAILS_QUERY_STRING_NORMALIZER proc, and updates spec expectations accordingly.

[Edit]
Patch sorts query parameters by key in the NON_RAILS_QUERY_STRING_NORMALIZER proc, prior to processing. This adds some consistency across ruby versions during the actual processing, and removes the value sorting which affected query array values

Updates the array value test to check for maintained query values, and sorting of keys.

@jnunemaker
Owner

Query string order should never matter, right? So this should not affect anyone else? The one way it will is stubbing requests in specs, which is probably why it was added. Anyone have any thoughts on this either way?

@Billiam

Query string order should never matter, right?

Unfortunately, whether it matters will depend on the receiving end. I ran into this issue working with an api (mapquest batch geocoding), where the incoming argument order determines the output result order, and the parameter sort caused mismatching.

It looks like the PR build failed for ree 2012.02 specifically, which I've been unable to build locally. I'm not sure whether there's some implicit argument sorting going on there...

@Billiam

After some testing, the sorted/unsorted behavior of Array() in different 1.8.7 versions is pretty varied.

A better fix, here, might just be to sort before flattening rather than after, which would retain the order of array hash values (while still sorting other params). The original tests would pass, and a test could be added to ensure array values are not sorted. Thoughts?

@jnunemaker
Owner

That makes sense I think.

@Billiam Billiam Sorting query parameters by key before processing
 - Query parameter order in specs to use sorted values again
 - Adding test for maintained array value order
 - Updating test to include mix of array and non-array values for sort.
ac468e0
@jnunemaker jnunemaker merged commit 7c1a19e into jnunemaker:master Jan 4, 2014

1 check passed

Details default The Travis CI build passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment