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

Fix order bug #261

Closed
wants to merge 3 commits into from
Closed

Conversation

Yuma-Nagaoka
Copy link

@Yuma-Nagaoka Yuma-Nagaoka commented Aug 25, 2022

Overview

fix #193

ActiveHash::Relation#find can't return a correct record after executing #order.
The reason is that the #order_by_args! method destructively reorders the 'relation' itself, and #find is based on the 'relation's index' which has the wrong reordered index.

How?

Change #order_by_args! method to be non-destructive.

@Yuma-Nagaoka Yuma-Nagaoka marked this pull request as ready for review August 30, 2022 07:16
@pfeiffer
Copy link
Contributor

pfeiffer commented Oct 4, 2022

This is much needed - mutating the original object is really unexpected. See also #257 and #265.

@kbrock
Copy link
Collaborator

kbrock commented Feb 15, 2023

@Yuma-Nagaoka Please add a test that sorts by order([]). Think this will return an empty array.

@flavorjones I feel like the other implementation is closer to active record, where it does a dup and all methods seem to modify the value in place (i.e.: are bang methods). But you are right. this is simpler.

This looks good. Just want that one test to make sure it is not breaking things if args is empty.

Also, not sure how the original code worked with 2 arguments for sorting order(:id, :name). In other implementations I always have had to map the args and compare those arrays (worrying about nils sneaking in). This existing solution looks like it is too simple.

@kbrock
Copy link
Collaborator

kbrock commented Mar 9, 2023

Yea, ok. so my thoughts were right the order(nil) and order({}) return []

  • spec/active_hash/base_spec.rb:940
  • spec/active_hash/base_spec.rb:944

Also, it looks like it screwed up the original order as well. These 2 tests are returning US instead of Canada:

  • spec/active_hash/base_spec.rb:963
  • spec/active_hash/base_spec.rb:984

@kbrock
Copy link
Collaborator

kbrock commented Mar 27, 2023

Thank you for helping us with this solution. Going with #268 instead

@kbrock kbrock closed this Mar 27, 2023
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

Successfully merging this pull request may close these issues.

The order method breaks finders
3 participants