Merge pull request #35127 from bogdan/counter-cache-loading

Bugfix association loading behavior when counter cache is zero

3 files changed, 48 insertions, 21 deletions

diff --git a/activerecord/lib/active_record/associations/collection_association.rb b/activerecord/lib/active_record/associations/collection_association.rb
index 4a25567c9d..6f5df807fe 100644
--- a/activerecord/lib/active_record/associations/collection_association.rb
+++ b/activerecord/lib/active_record/associations/collection_association.rb
@@ -210,7 +210,8 @@ module ActiveRecord
       # This method is abstract in the sense that it relies on
       # +count_records+, which is a method descendants have to provide.
       def size
-        if !find_target? || loaded?
+        if !find_target?
+          loaded! unless loaded?
           target.size
         elsif @association_ids
           @association_ids.size
@@ -233,7 +234,7 @@ module ActiveRecord
       # loaded and you are going to fetch the records anyway it is better to
       # check <tt>collection.length.zero?</tt>.
       def empty?
-        if loaded? || @association_ids
+        if loaded? || @association_ids || reflection.has_cached_counter?
           size.zero?
         else
           target.empty? && !scope.exists?
diff --git a/activerecord/lib/active_record/associations/has_many_association.rb b/activerecord/lib/active_record/associations/has_many_association.rb
index eb22db838c..6f67934a79 100644
--- a/activerecord/lib/active_record/associations/has_many_association.rb
+++ b/activerecord/lib/active_record/associations/has_many_association.rb
@@ -36,14 +36,6 @@ module ActiveRecord
         super
       end
 
-      def empty?
-        if reflection.has_cached_counter?
-          size.zero?
-        else
-          super
-        end
-      end
-
       private
 
         # Returns the number of records in this collection.
@@ -60,20 +52,24 @@ module ActiveRecord
         # If the collection is empty the target is set to an empty array and
         # the loaded flag is set to true as well.
         def count_records
-          count = if reflection.has_cached_counter?
-            owner._read_attribute(reflection.counter_cache_column).to_i
-          else
-            scope.count(:all)
-          end
+          count = counter_cache_value || scope.count(:all)
 
           # If there's nothing in the database and @target has no new records
           # we are certain the current target is an empty array. This is a
           # documented side-effect of the method that may avoid an extra SELECT.
-          (@target ||= []) && loaded! if count == 0
+          loaded! if count == 0
 
           [association_scope.limit_value, count].compact.min
         end
 
+        def counter_cache_value
+          reflection.has_cached_counter? ? owner._read_attribute(reflection.counter_cache_column).to_i : nil
+        end
+
+        def find_target?
+          super && !counter_cache_value&.zero?
+        end
+
         def update_counter(difference, reflection = reflection())
           if reflection.has_cached_counter?
             owner.increment!(reflection.counter_cache_column, difference)
diff --git a/activerecord/test/cases/associations/has_many_associations_test.rb b/activerecord/test/cases/associations/has_many_associations_test.rb
index 2f090d9862..4c9e4d0ad2 100644
--- a/activerecord/test/cases/associations/has_many_associations_test.rb
+++ b/activerecord/test/cases/associations/has_many_associations_test.rb
@@ -1224,12 +1224,15 @@ class HasManyAssociationsTest < ActiveRecord::TestCase
   def test_has_many_without_counter_cache_option
     # Ship has a conventionally named `treasures_count` column, but the counter_cache
     # option is not given on the association.
-    ship = Ship.create(name: "Countless", treasures_count: 10)
+    ship = Ship.create!(name: "Countless", treasures_count: 10)
 
     assert_not_predicate Ship.reflect_on_association(:treasures), :has_cached_counter?
 
     # Count should come from sql count() of treasures rather than treasures_count attribute
-    assert_equal ship.treasures.size, 0
+    assert_queries(1) do
+      assert_equal ship.treasures.size, 0
+      assert_predicate ship.treasures, :loaded?
+    end
 
     assert_no_difference lambda { ship.reload.treasures_count }, "treasures_count should not be changed" do
       ship.treasures.create(name: "Gold")
@@ -1350,6 +1353,20 @@ class HasManyAssociationsTest < ActiveRecord::TestCase
     post = posts(:welcome)
     assert_no_queries do
       assert_not_empty post.comments
+      assert_equal 2, post.comments.size
+      assert_not_predicate post.comments, :loaded?
+    end
+    post = posts(:misc_by_bob)
+    assert_no_queries do
+      assert_empty post.comments
+      assert_predicate post.comments, :loaded?
+    end
+  end
+
+  def test_empty_association_loading_with_counter_cache
+    post = posts(:misc_by_bob)
+    assert_no_queries do
+      assert_empty post.comments.to_a
     end
   end
 
@@ -1986,9 +2003,22 @@ class HasManyAssociationsTest < ActiveRecord::TestCase
     assert_not_predicate company.clients, :loaded?
   end
 
-  def test_counter_cache_on_unloaded_association
-    car = Car.create(name: "My AppliCar")
-    assert_equal car.engines.size, 0
+  def test_zero_counter_cache_usage_on_unloaded_association
+    car = Car.create!(name: "My AppliCar")
+    assert_no_queries do
+      assert_equal car.engines.size, 0
+      assert_predicate car.engines, :loaded?
+    end
+  end
+
+  def test_counter_cache_on_new_record_unloaded_association
+    car = Car.new(name: "My AppliCar")
+    # Ensure no schema queries inside assertion
+    Engine.primary_key
+    assert_no_queries do
+      assert_equal car.engines.size, 0
+      assert_predicate car.engines, :loaded?
+    end
   end
 
   def test_get_ids_ignores_include_option