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.

Sign up for GitHub

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

Jump to bottom

e4e1b62 broke to_param handling: #33341

Merged
merged 1 commit into from Jul 12, 2018
Merged

e4e1b62 broke to_param handling: #33341

merged 1 commit into from Jul 12, 2018

Conversation

Edouard-chin
Copy link
Member

@Edouard-chin Edouard-chin commented Jul 11, 2018 •
edited

  • There was an issue inside controller tests where order params were not respected, the reason
    was because we were calling Hash#to_query which sorts the results lexicographically.
    1e4e1b62 fixed that issue by not using to_query but instead a utility function provided by rack.

  • However with the fix came another issue where it's now no longer possible to do this

     post :foo, params: { user: User.first }

 # Prior to the patch the controller will receive { "user" => "1" }
 # Whereas now you get { "user": "#<User: ...>" }

    The fix in this PR is to modify Hash#to_query to sort only when it
    doesn't contain an array structure that looks something like "bar[]"

    Ref e4e1b62 broke to_param handling: #33341 (comment)

@rails-bot
Copy link

r? @kaspth

(@rails-bot has picked a reviewer for you, use r? to override)

@Edouard-chin
Copy link
Member Author

cc/ @rafaelfranca

@matthewd
Copy link
Member

I think the best solution is to change Hash#to_query so that it sorts in a structure-aware way. The sorting is to ensure that querystrings with identical semantic meanings (a=b&c=d vs c=d&a=b) don't both get generated (thereby missing out on cache hits, for example)... but it currently achieves that without considering the fact that sometimes order does make a difference.

Edouard-chin added a commit to Edouard-chin/rails that referenced this pull request Jul 11, 2018
@Edouard-chin
Modify Hash#to_query to not sort lexicographically for nested struc…
11fc857 
…ture:

- As discussed in rails#33341 (comment)
matthewd
matthewd reviewed Jul 12, 2018
View reviewed changes
Copy link
Member

@matthewd matthewd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

I think this skips some sorting that would still be permissible (and that a deeper fix could incidentally fix round-tripping on e.g. [{a:1,c:3},{b:2,c:4}]), but I like the brevity of this change, and it's a good balance of keeping useful sorting without allowing it to break things. 👍

activesupport/lib/active_support/core_ext/object/to_query.rb Outdated
end.compact.sort! * "&"
end.compact

query.sort! unless namespace =~ /\[\w+\]\[\]/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is namespace.include?("[]") sufficient here?

@Edouard-chin
Copy link
Member Author

Thanks for the quick review friends 🤗 . Addressed your comment Matthew and squashed commits

@Edouard-chin
e4e1b62 broke to_param handling:
48b6bac 
- There was an issue inside controller tests where order params were not respected, the reason
  was because we were calling `Hash#to_query` which sorts the results lexicographically.
  1e4e1b62 fixed that issue by not using `to_query` but instead a utility function provided by rack.
- However with the fix came another issue where it's now no longer possible to do this

  ```
   post :foo, params: { user: User.first }

   # Prior to the patch the controller will receive { "user" => "1" }
   # Whereas now you get { "user": "#<User: ...>" }
  ```

  The fix in this PR is to modify `Hash#to_query` to sort only when it
  doesn't contain an array structure that looks something like "bar[]"

  Ref rails#33341 (comment)
@Edouard-chin
Copy link
Member Author

CI seems broken but unrelated. Try re-running without success (it fails fetching some ubuntu package)

@rafaelfranca rafaelfranca merged commit fd132d0 into rails:master Jul 12, 2018
rafaelfranca added a commit that referenced this pull request Jul 12, 2018
@rafaelfranca
Merge pull request #33341 from Edouard-chin/ec-fix-to-param
73df2b3 
e4e1b62 broke `to_param` handling:
rafaelfranca added a commit that referenced this pull request Jul 12, 2018
@rafaelfranca
Merge pull request #33341 from Edouard-chin/ec-fix-to-param
c6424c8 
e4e1b62 broke `to_param` handling:
rafaelfranca added a commit that referenced this pull request Jul 12, 2018
@rafaelfranca
Merge pull request #33341 from Edouard-chin/ec-fix-to-param
2e824af 
e4e1b62 broke `to_param` handling:
@Edouard-chin Edouard-chin deleted the ec-fix-to-param branch July 12, 2018 18:17
@rafaelfranca rafaelfranca mentioned this pull request Nov 19, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matthewd matthewd matthewd approved these changes

@rafaelfranca rafaelfranca rafaelfranca approved these changes

@tenderlove tenderlove Awaiting requested review from tenderlove

Assignees

@kaspth kaspth

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@Edouard-chin @rails-bot @matthewd @rafaelfranca @kaspth