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 STI in available_records causing new instances of records to be loaded from database #43524

Merged
merged 1 commit into from Oct 27, 2021

Conversation

octatone
Copy link
Contributor

@octatone octatone commented Oct 25, 2021

Summary

Fixes bug in ActiveRecord::Associations::Preloader causing unnecessary refetching of records by removing grouping by available_record class names.

Copy link

@akenneth akenneth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice change!

Left 1 question I am unsure about, other than that lgtm!

activerecord/CHANGELOG.md Outdated Show resolved Hide resolved
activerecord/CHANGELOG.md Outdated Show resolved Hide resolved
activerecord/test/cases/associations_test.rb Show resolved Hide resolved
Copy link
Member

@eileencodes eileencodes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good, can you squash your commits into one and make sure the one commit explains the change?

@octatone octatone force-pushed the octatone/preloadSTIFix branch 2 times, most recently from 3d5183d to 355deb4 Compare October 26, 2021 14:20
…oaded from database

Removes grouping by available_record class names in ActiveRecord::Associations::Preloader::Batch so that records of differing STI classes can be located.
@eileencodes eileencodes merged commit 18922cc into rails:main Oct 27, 2021
@zzak
Copy link
Member

zzak commented Oct 31, 2021

@octatone Was there an associated issue with this PR or just something you picked up on and posted a patch?

@eileencodes
Copy link
Member

There's no public issue, this was found in the GitHub app @zzak.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants