Fix: Incorrectly preloading through association records when middle association has been loaded #46579
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
joshuay03
changed the title
joshuay03
force-pushed
the
fix-through-association-incorrect-preloading
branch
from
54d8dd8 to
1fd3021
Compare
joshuay03
force-pushed
the
fix-through-association-incorrect-preloading
branch
from
1fd3021 to
3758d99
Compare
joshuay03
changed the title
joshuay03
force-pushed
the
fix-through-association-incorrect-preloading
branch
19 times, most recently
from
b657cbe to
c540b36
Compare
joshuay03
force-pushed
the
fix-through-association-incorrect-preloading
branch
3 times, most recently
from
498cbc2 to
fc76bf3
Compare
joshuay03
commented
Dec 17, 2022
| end | ||
|
|
||
| assert_no_queries do | ||
| assert_equal expected_member_detail_ids.sort, member.organization_member_details_2.map(&:id).sort |
There was a problem hiding this comment.
This fails without the fix above
joshuay03
commented
Dec 17, 2022
| member.reload.organization # load through record | ||
|
|
||
| assert_queries(1) do | ||
| ActiveRecord::Associations::Preloader.new(records: [member], associations: :organization_member_details_2).call |
There was a problem hiding this comment.
Down from 3 queries to 1 🎉
joshuay03
force-pushed
the
fix-through-association-incorrect-preloading
branch
from
fc76bf3 to
5eb9443
Compare
joshuay03
changed the title
joshuay03
changed the title
joshuay03
changed the title
joshuay03
force-pushed
the
fix-through-association-incorrect-preloading
branch
from
5eb9443 to
85e404e
Compare
joshuay03
changed the title
joshuay03
force-pushed
the
fix-through-association-incorrect-preloading
branch
from
85e404e to
18f8335
Compare
|
FWIW, I've confirmed this fixes the issue described in #45542, but I'm not authoritative enough to say whether or not it's a real bug. |
|
This looks great! Thanks! |
joshuay03
force-pushed
the
fix-through-association-incorrect-preloading
branch
2 times, most recently
from
d472cb9 to
4ab9e84
Compare
… when middle association has been loaded
joshuay03
force-pushed
the
fix-through-association-incorrect-preloading
branch
from
4ab9e84 to
0178e1d
Compare
|
Cheers! @byroot |
|
Hi, Thanks for the fix, this issue was blocking application I work on from upgrading to Rails 7.0. If I understand correctly how rails releases work, this fix will be included in Rails 7.1 release as it was merged in |
byroot
added a commit
that referenced
this pull request
Mar 8, 2023
…rrect-preloading Fix: Incorrectly preloading through association records when middle association has been loaded
|
I just backported it in 7-0-stable as 99ee8bd. You can point your Gemfile at this branch. |
|
Thanks a lot! |
4 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix: #45822
This Pull Request has been created to address #45822.
Update: Looks like this also resolves #45542.
Detail
This Pull Request fixes
ActiveRecord::Associations::Preloader::ThroughAssociation#records_by_ownerto:RE: 1
Using a failing test and some logging, I found that the memoized keys in
source_records_by_ownerhad differentobject_ids to the values inthrough_records_by_ownerat the point at which the value was fetched usingsource_records_by_owner[record]and was therefore returningnil.This was being caused by
def middle_recordsloading new objects into@preloaded_recordswhich would get passed in as the records for the source preloader, rather than using the already loaded through objects inthrough_records_by_owner.This resulted in the
source_records_by_ownerhash containing keys which were different objects to the values in thethrough_records_by_ownerhash.RE: 2
This also happens to fix a circular dependency of sorts, since
def middle_recordswould eventually calldef source_preloaderswhile already in a previous call to it originating fromdef source_records_by_owner, causing the through record/s to reload and the source record/s to load, before finally loading the source records once again. These extra queries are visible in my comment below.Additional information
Checklist
Before submitting the PR make sure the following are checked:
[Fix #issue-number]