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

Make prefetch_associations work as expected on associations that have been particually prefetched #558

Merged
merged 1 commit into from
Feb 16, 2024

Conversation

kirs
Copy link
Contributor

@kirs kirs commented Feb 15, 2024

Consider the following code:

def work(product_variant_ids)
  product_variants = ProductVariant.fetch_multi(product_variant_ids, includes: { product: [] })
  safe = perform_safety_checks(product_variants)
  return if !safe
  
  # Now that it's safe, we want to access product variants
  # with their products and images and some other items.
  # For that, we'd want them to be prefetched to avoid N+1
  ProductVariant.prefetch_associations({ product: [:images], some_other_items: [] }, product_variants)
end

The following uses prefetch_associations API from IDC. It works great, apart from when the same association (in our case, :product) has already been part of prefetching.

In the snippet above, some_other_items would be loaded but product: [:images] would not be, because product is already considered being loaded.

This PR fixes that bug.

@kirs kirs force-pushed the prefetch_associations-repeating branch from 9deb4d7 to 50d1d7c Compare February 15, 2024 02:00
Copy link
Contributor

@drinkbeer drinkbeer left a comment

Choose a reason for hiding this comment

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

Great find about the bug!

Comment on lines +80 to +81
if ids_to_owner_record.any?
load_strategy.load_multi(
Copy link
Contributor

@drinkbeer drinkbeer Feb 15, 2024

Choose a reason for hiding this comment

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

Just try to understand the code and unit tests. When adding a binding.pry here, I saw the records were like:

[3] pry(#<IdentityCache::Cached::BelongsTo>)> records
=> [#<AssociatedRecord:0x000000010ac24880
  id: 1,
  name: "bob child",
  item_id: 1,
  item_two_id: nil,
  created_at: 2024-02-15 21:32:33.350638 UTC,
  updated_at: 2024-02-15 21:32:33.350638 UTC>,
 #<AssociatedRecord:0x000000010ac24740
  id: 2,
  name: "fred child",
  item_id: 3,
  item_two_id: nil,
  created_at: 2024-02-15 21:32:33.361986 UTC,
  updated_at: 2024-02-15 21:32:33.361986 UTC>]

So basically, in the tests, the bob has id 1, fred has id 3, bob_child has id 1, fred_child has id 2.

So in the test:

      assert_memcache_operations(1) do
        AssociatedRecord.prefetch_associations({ item: :item }, [@cached_bob_child, @cached_fred_child])
      end

There is only 1 memcached operation because only fred is loaded as an item of fred_child, and bob and bob_child has the same id, so only one associated object loaded from memcached?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for spotting. I'll rework the tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reworked the tests 🙏

@kirs kirs force-pushed the prefetch_associations-repeating branch from 50d1d7c to ccb2fd7 Compare February 16, 2024 21:34
@kirs kirs merged commit ad26a21 into main Feb 16, 2024
5 checks passed
@kirs kirs deleted the prefetch_associations-repeating branch February 16, 2024 22:31
@kirs kirs changed the title Allow prefetch_associations on association that was prefetched before Make prefetch_associations work as expected on associations that have been particually prefetched Feb 16, 2024
@kirs kirs mentioned this pull request Feb 16, 2024
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.

3 participants