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.19k stars 178 forks source link

Bugfix: Ensure that using `id: [..]` as condition returns unique records #266

Closed pfeiffer closed 1 year ago

pfeiffer commented 1 year ago

This PR fixes an issue where only unique records will be returned when using where(id: ..). Previously, if the same ID was provided multiple times in the id condition, more records than expected would be returned.

This PR fixes the issue and now mimics the way ActiveRecord work. It also streamlines so that query_hash always has symbols as keys. We can use deep_symbolize_keys for this as there's already a dependency on ActiveSupport.

Before

Country.where(id: [1, 2, 1])
# => 3 records; the Country with ID=1 is returned twice

After

Country.where(id: [1, 2, 1])
# => 2 records as expected
pfeiffer commented 1 year ago

See also the #268 as an alternative which addresses the root of the issue.

kbrock commented 1 year ago

At this moment, I am leaning towards #268 Thanks again for providing this options (more surgical) and the other one (more like an atomic bomb ;) )

kbrock commented 1 year ago

so that query_hash always has symbols as keys

I had just read #196 and the thoughts around converting all keys to strings. I'm guessing you don't care which camp this code is in, as long as it is consistent.

nvr mind. This is moot if we go with #268

pfeiffer commented 1 year ago

I had just read #196 and the thoughts around converting all keys to strings. I'm guessing you don't care which camp this code is in, as long as it is consistent.

It's actually two totally different cases; for query_hash where we store the conditions of the query, the type of keys in the hash is not that important, but I do think it's important to be consistent and not allow both strings and symbols as keys, but one of them.

For #196 I'm mentioning that attributes should be stored with strings as keys, as this is what ActiveModel concerns expect and use for instance when serializing with as_json

Please note that in #268 also fixes the issue in this PR and supersedes that. This PR is the "surgical" one fixing the bug, while #268 addresses the overall issue (and a lot more) giving a better structure going forward IMO.

kbrock commented 1 year ago

Going with #268