Set inverse asoociation for cache_has_many without :embed => true by roccoblues · Pull Request #360 · Shopify/identity_cache

roccoblues · 2017-11-15T14:40:55Z

Fixes #357

…ixes #357)

shwetabhgarg · 2017-11-19T19:02:14Z


 #### 1.0.0 Unreleased

+- Set inverse asoociation for cache_has_many without `:embed => true`


asoociation -> association :P

I would say it is for an "id embedded cach_has_many associations" rather than saying what it isn't for.

dylanahsmith

Looks like prefetch_one_association also needs to be changed so that the inverse cached association is also set when the association is batch loaded (e.g. through using the :includes option on fetch).

dylanahsmith · 2017-11-29T16:00:15Z

        options[:records_variable_name]   = "cached_#{association}"
        options[:prepopulate_method_name] = "prepopulate_fetched_#{association}"

+        inverse_name = options[:association_reflection].inverse_of.try(:name)


Prefer try! over try, since the latter also does a respond_to? check that can hide bugs.

dylanahsmith · 2017-11-29T16:17:41Z

-              @#{options[:records_variable_name]} ||= #{options[:association_reflection].class_name}.fetch_multi(#{options[:cached_ids_name]})
+              @#{options[:records_variable_name]} ||= begin
+                #{options[:association_reflection].class_name}.fetch_multi(#{options[:cached_ids_name]}).map do |record|
+                  record.send('#{inverse_name}=', self) if #{inverse_name.present?}


We don't want to set the association itself, since putting records loaded from the cache into an active record association would make it easier to unintentionally persist that data to the database, which could persist cache corruption. Instead, we want to make sure they use the cached accessor to access it. Can you re-use set_inverse_of_cached_association to do it the same way as embedded cached associations?

dylanahsmith · 2017-11-29T16:21:47Z

+  end
+
+  def test_fetch_should_setup_association_for_cache_has_many
+    Item.cache_has_many :associated_records


Explicitly use the :embed => :ids option for clarity

dylanahsmith · 2017-11-29T16:23:52Z

+
+    records = item.fetch_associated_records
+    records.each do |r|
+      assert_equal item.object_id, r.item.object_id


You should need to use r.fetch_item.

dylanahsmith · 2017-11-29T16:25:16Z

+
+    item = Item.find_by_id(@item.id)
+
+    records = item.fetch_associated_records


This will always just delegate to the active record association, since item wasn't fetched from the cache.

dylanahsmith · 2017-11-29T16:28:34Z

@@ -0,0 +1,53 @@
+require "test_helper"
+
+class InverseOfTest < IdentityCache::TestCase


Aren't there already tests for setting the inverse cached association? If there are already tests in denormalized_has_many_test.rb, denormalized_has_one_test.rb and normalized_belongs_to_test.rb then we just need tests for this behaviour in normalized_has_many_test.rb

No really, only this: https://github.com/Shopify/identity_cache/blob/master/test/denormalized_has_many_test.rb#L105 and I'm not sure what it tests.

dylanahsmith · 2017-11-29T16:30:40Z

+    item = Item.find_by_id(@item.id)
+
+    record = item.fetch_associated
+    assert_equal item.object_id, record.item.object_id


This will always just delegate to the active record association, since item wasn't fetched from the cache and this is an embedded cached association.

dylanahsmith · 2017-11-29T16:39:36Z

+  end
+
+  def test_fetch_should_setup_association_for_cache_belongs_to
+    Item.cache_belongs_to :associated


This seems like it should raise, since the model doesn't even have a belongs_to :associated association, nor does the table have an associated_id column on the items table. I think this must be actually using the has_one association.

dylanahsmith · 2017-11-29T16:42:33Z


 #### 1.0.0 Unreleased

+- Set inverse asoociation for cache_has_many without `:embed => true`


I would say it is for an "id embedded cach_has_many associations" rather than saying what it isn't for.

dylanahsmith · 2017-11-29T16:47:37Z

+  def test_fetch_should_setup_association_for_cache_has_many
+    Item.cache_has_many :associated_records
+
+    item = Item.find_by_id(@item.id)


Use find when you always expect it to return a record.

roccoblues · 2017-12-11T10:56:45Z

@dylanahsmith I have the feeling we've been talking about different things. Can you have a look at the code in the related issue again? Here's what I meant in pseudo-code:

item = Item.find(ID) # fetches item from the db
associated_records = item.fetch_associated # fetches from cache

# now I wanted the item accessor on the records fetched from cache
# to *not* load the item again
associated_records.first.item.object_id == item.object_id

I just found that you even have a test that this should explicitly not happen: https://github.com/Shopify/identity_cache/blob/master/test/denormalized_has_many_test.rb#L124 ?

Did you instead think about this?

item = Item.fetch(ID) # fetches item from the cache
associated_records = item.fetch_associated # fetches from cache

# the fetch_ method should return the already fetch item from cache?
associated_records.first.fetch_item.object_id == item.object_id

dylanahsmith · 2017-12-11T15:32:06Z

I just found that you even have a test that this should explicitly not happen: https://github.com/Shopify/identity_cache/blob/master/test/denormalized_has_many_test.rb#L124 ?

In that case it is using the active record association associated_record.item, instead of the cache accessor associated_record.fetch_item, so it shouldn't need modification. However, you could also add an assertion of assert_equal associated_record.fetch_item.object_id, item.object_id which should also pass with your change. The difference is that we don't want to store the record loaded from the cache in the active record association itself, we want to store it in the instance variable that the fetch_#{association} method accesses.

…t.rb

roccoblues · 2017-12-12T09:26:17Z

Ok, I think I got it now. Please check again.

dylanahsmith · 2021-10-26T19:32:45Z

the default branch was renamed from master to main, which seemed to result in this PR being closed by the bot doing the rename. I'm not sure why it wasn't able to update this PRs target to main like in other PRs, but I can't reopen or update this PRs target branch

roccoblues added 2 commits November 13, 2017 13:15

add failing testcases

01219cc

Set inverse asoociation for cache_has_many without ":embed => true" (F…

24534b0

…ixes #357)

shwetabhgarg reviewed Nov 19, 2017

View reviewed changes

dylanahsmith suggested changes Nov 29, 2017

View reviewed changes

dylanahsmith mentioned this pull request Nov 29, 2017

Prevent cache_belongs_to from being used with a has_one association #361

Open

roccoblues added 6 commits December 11, 2017 10:36

Merge branch 'master' into add_inverse_association

c744e2f

adjust changelog message

01459a3

use try! instead of try

d403dde

use find instead of find_by_id

ad2c910

add :embed => :ids option to testcase to make it more explicit

2d2bf9b

adjust belongs_to test which is now broken

25f4682

roccoblues added 6 commits December 11, 2017 21:13

better tests for inverse association caching

7f8cda9

set inverse association for id embedded cache_has_many associations

89c9bc4

remove unused variable

b88c271

remove another unused variable

da6fd96

remove broken inverse association test from normalized_belongs_to_tes…

f11e1da

…t.rb

update changelog

a9cbaae

spy-v2 Bot deleted the branch Shopify:master October 26, 2021 14:14

spy-v2 Bot closed this Oct 26, 2021


		#### 1.0.0 Unreleased

		- Set inverse asoociation for cache_has_many without `:embed => true`


		item = Item.find_by_id(@item.id)

		records = item.fetch_associated_records

		@@ -0,0 +1,53 @@
		require "test_helper"

		class InverseOfTest < IdentityCache::TestCase

Conversation

roccoblues commented Nov 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dylanahsmith left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

roccoblues commented Dec 11, 2017

Uh oh!

dylanahsmith commented Dec 11, 2017

Uh oh!

roccoblues commented Dec 12, 2017

Uh oh!

dylanahsmith commented Oct 26, 2021

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants