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

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

Closed
wants to merge 2 commits into from

Conversation

pfeiffer
Copy link
Contributor

@pfeiffer pfeiffer commented Oct 4, 2022

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
Copy link
Contributor Author

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

@kbrock
Copy link
Collaborator

kbrock commented Mar 9, 2023

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
Copy link
Collaborator

kbrock commented Mar 10, 2023

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
Copy link
Contributor Author

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
Copy link
Collaborator

kbrock commented Mar 27, 2023

Going with #268

@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.

2 participants