search

active-hash / active_hash

A readonly ActiveRecord-esque base class that lets you use a hash, a Yaml file or a custom file as the datasource
MIT License
1.2k stars 179 forks source link

Fix order bug #261

Closed Yuma-Nagaoka closed 1 year ago

Yuma-Nagaoka commented 2 years ago

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.

pfeiffer commented 1 year ago

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

kbrock commented 1 year ago

@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 commented 1 year ago

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

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

kbrock commented 1 year ago

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