Shopify / Shopify/identity_cache

prefetch_associations assumes passed in records have loaded the same associations

Open
#453 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Ruby
Stars
2k
Forks
174
Avg merge
13m
Merged PRs (30d)
3

Description

This method was initially just used internally to support the includes option to model.fetch_by_id, where the records had just been loaded, so it seemed like a safe assumption. Rails made a similar assumption with its association preloader for similar reasons, so it also seemed like this assumption made sense by being consistent with rails' association preloader. However, rails 5.2 changed the association preloader in https://github.com/rails/rails/pull/37747 to check the whole collection when checking if the association has already been loaded.

prefetch_associations has since been made public in identity_cache, which also makes it such that an application can pass it an array of records where the association might only be loaded on the first record. The bug fixed by https://github.com/Shopify/identity_cache/pull/449 also shows an additional reason why we might need to check all records, because an association could be modified on a subset of the records, which can cause it to fallback to using the active record association instead of the cached association.

We should get rid of this assumption for when prefetch_associations is called directly. However, we may want to leverage some assumptions we can make for the fetch_by_id includes option, such as that the association hasn't been modified and that embedded associations have been loaded, to avoid adding significant overhead for that common case.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Trace the public prefetch_associations method and the fetch_by_id includes path to understand where loaded-association assumptions are made. Compare the direct-call behavior with the optimized includes case, then verify that direct calls inspect all records while preserving the stated assumptions for fetch_by_id. The payload names no files or tests to run.

Written by the indexing model from the issue text.

Assessment

Tech stack
ruby
Domain
backend, databases
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
38/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.