Skip to content

Fix cache_belongs_to prefetch with owners sharing a foreign key - #635

Open
VolodyaAll wants to merge 1 commit into
Shopify:mainfrom
VolodyaAll:fix-belongs-to-prefetch-shared-foreign-key
Open

VolodyaAll wants to merge 1 commit into
Shopify:mainfrom
VolodyaAll:fix-belongs-to-prefetch-shared-foreign-key

Conversation

@VolodyaAll

Copy link
Copy Markdown

Problem

In the non-polymorphic branch of Cached::BelongsTo#fetch_async, the owners to load are indexed by foreign key with a plain hash:

hash[associated_id] = owner_record

When several owners share the same foreign key, each one overwrites the previous entry, so the prefetched record is written only into the last owner. The other owners stay unloaded, and their fetch_<association> falls back to fetch_by_id, which costs one extra cache round trip per owner. Prefetching quietly degrades into N+1 cache reads. Includes nested under the association are lost for those owners too, because the fallback returns a separate copy of the record that doesn't have them.

This is easy to hit with nested includes, where many children belong to the same record:

Item.cache_has_many :associated_records, embed: :ids
AssociatedRecord.cache_belongs_to :item_two

# three associated_records of two items, all pointing at the same ItemTwo
bob, joe = Item.fetch_multi(bob.id, joe.id, includes: { associated_records: :item_two })
(bob.fetch_associated_records + joe.fetch_associated_records).map(&:fetch_item_two)
# only the last child got the ItemTwo assigned; the other two each do a separate `cas`

Fix

Collect all owners per foreign key and write the associated record into each of them. The polymorphic branch of the same method already iterates over every owner and assigns the record to each one, so the two branches now behave the same. The collection yielded to nested includes stays unchanged: unique associated records with nils compacted.

Tests

  • test_prefetch_associations_cached_belongs_to_with_shared_foreign_key covers two owners with the same foreign key, prefetched with a symbol include (LoadStrategy::Eager). Without the fix, fetch_item on the first owner does an extra cache operation (1 instead of 0).
  • test_fetch_multi_batch_fetches_non_embedded_second_level_belongs_to_associations_with_shared_foreign_key covers the nested includes case above (LoadStrategy::Lazy). Without the fix it does 5 cache operations instead of 3.

I ran rake test locally against MySQL 5.7 with dalli and against PostgreSQL with memcached_store, then rake test_all and rubocop with the latest-release gemfile. Everything passes except two tests on the Rails edge gemfile (FetchTest#test_fetch_by_title_hit and IndexCacheTest#test_fetch_with_unique_adds_limit_clause). Those also fail on main and are unrelated to this change.

I also checked the other cached associations for the same pattern. Reference::HasMany and Reference::HasOne key their hash by child id, and a child belongs to a single parent, so they are not affected.

🤖 Generated with Claude Code

Cached::BelongsTo#fetch_async indexed the owners to load by foreign
key with a plain hash, so when several owners pointed at the same
record only the last one kept its entry. The prefetched record was
written into that owner only, and every other owner fell back to
fetch_by_id, one cache round trip each.

Collect every owner per foreign key and write the record into all of
them, as the polymorphic branch already does. The collection yielded
to nested includes is unchanged.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@VolodyaAll

Copy link
Copy Markdown
Author

I have signed the CLA!

This branch has not been deployed

No deployments
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.

1 participant