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 duplicate record assignment #31

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

apauly
Copy link

@apauly apauly commented Jul 3, 2020

Hi,

I found the following issue when using jit_preloader:

If multiple records in a collection have the same record set for a belongs_to, which itself has a has_many association, the has_many association will end up having duplicate records assigned. When using ARs preload, there are no duplicate records.

I could reproduce this behaviour with a spec and was also able to fix it. However, I'm not 100% sure if my fix will add other side effects if there are actually duplicate records via has_many through: or similar setups.

Maybe somebody else with better understanding of the gem itself can simply tell where to apply a fix without side effects.

@arthurchui arthurchui self-requested a review July 6, 2020 16:44
@fimmtiu
Copy link
Contributor

fimmtiu commented Jul 7, 2020

@apauly Thank you for the bug report and the pull request! We quite appreciate the feedback.

I can duplicate the issue under Rails 6 now. (Seems to be okay on Rails 5.) I'm not sure that this is the best approach for fixing it, however, as it still makes twice as many queries as it needs to, then throws away half the results. (By the time we get to the uniq call, we've already made a duplicate phone_numbers query.)

I'll open an issue for it (#32) and we'll try to get it to it soonish.

@arthurchui arthurchui removed their request for review July 8, 2020 17:36
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