Merge pull request #566 from Shopify/change-order-use-idc-check

Change order in if clause for should_use_cache?

CHANGELOG.md

@@ -2,6 +2,15 @@
 
 ## Unreleased
 
+## 1.5.6
+
+- Minor performance improvements on association read
+
+## 1.5.5
+
+- Minor performance improvements on association read
+
+
 ## 1.5.4
 
 - Make `prefetch_associations` work as expected on associations that have been partially prefetched

Gemfile.lock

@@ -1,7 +1,7 @@
 PATH
   remote: .
   specs:
-    identity_cache (1.5.5)
+    identity_cache (1.5.6)
       activerecord (>= 7.0)
       ar_transaction_changes (~> 1.1)
 

lib/identity_cache/cached/belongs_to.rb

@@ -9,7 +9,7 @@ def build
         reflection.active_record.class_eval(<<-RUBY, __FILE__, __LINE__ + 1)
           def #{cached_accessor_name}
             association_klass = association(:#{name}).klass
-            if (loaded_by_idc? || association_klass.should_use_cache?) && #{reflection.foreign_key}.present? && !association(:#{name}).loaded?
+            if #{reflection.foreign_key}.present? && !association(:#{name}).loaded? && (loaded_by_idc? || association_klass.should_use_cache?)
               if defined?(#{records_variable_name})
                 #{records_variable_name}
               else

lib/identity_cache/cached/recursive/association.rb

@@ -25,7 +25,7 @@ def build
         def read(record)
           assoc = record.association(name)
 
-          if (record.send(:loaded_by_idc?) || assoc.klass.should_use_cache?) && !assoc.loaded? && assoc.target.blank?
+          if !assoc.loaded? && assoc.target.blank? && (record.send(:loaded_by_idc?) || assoc.klass.should_use_cache?)
             if record.instance_variable_defined?(records_variable_name)
               record.instance_variable_get(records_variable_name)
             elsif record.instance_variable_defined?(dehydrated_variable_name)

lib/identity_cache/version.rb

@@ -1,6 +1,6 @@
 # frozen_string_literal: true
 
 module IdentityCache
-  VERSION = "1.5.5"
+  VERSION = "1.5.6"
   CACHE_VERSION = 8
 end

test/fetch_test.rb

@@ -342,6 +342,18 @@ def test_returned_records_are_not_readonly_with_open_transactions
     end
   end
 
+  def test_should_use_cache_not_called_on_preloaded
+    Item.send(:cache_belongs_to, :item)
+
+    bob = Item.create!(title: "bob")
+    john = Item.create!(title: "john")
+    bob.update_column(:item_id, john.id)
+
+    from_db = Item.includes(:item).find(bob.id)
+    IdentityCache.expects(:should_use_cache?).never
+    from_db.fetch_item
+  end
+
   def test_should_use_cache_not_called_on_prefetched
     Item.send(:cache_belongs_to, :item)
 
@@ -366,6 +378,21 @@ def test_should_use_cache_not_called_on_prefetched_multi
     items.first.fetch_item
   end
 
+  def test_should_use_cache_not_called_on_preloaded_has_many
+    Item.send(:cache_has_many, :associated_records, embed: true)
+
+    bob = Item.create!(title: "bob")
+
+    bob.associated_records.create!(name: "foo")
+    bob.associated_records.create!(name: "bar")
+
+    from_db = Item.includes(:associated_records).find(bob.id)
+
+    IdentityCache.expects(:should_use_cache?).never
+    records = from_db.fetch_associated_records
+    assert_equal(2, records.size)
+  end
+
   def test_should_use_cache_not_called_on_embedded_has_many
     Item.send(:cache_has_many, :associated_records, embed: true)