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

This reverts commit eec3e28a1abf75676dcee58308ee5721bb53c325, reversing
changes made to 5588fb4802328a2183f4a55c36d6703ee435f85c.

Reason: Marking as loaded without actual loading is too greedy optimization.

See more context #35239.

Closes #35239.

[Edouard CHIN & Ryuta Kamizono]

3 files changed, 20 insertions, 47 deletions

diff --git a/activerecord/lib/active_record/associations/collection_association.rb b/activerecord/lib/active_record/associations/collection_association.rb
index b0c0beac0e..c3d4eab562 100644
--- a/activerecord/lib/active_record/associations/collection_association.rb
+++ b/activerecord/lib/active_record/associations/collection_association.rb
@@ -209,8 +209,7 @@ 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! unless loaded?
+        if !find_target? || loaded?
           target.size
         elsif @association_ids
           @association_ids.size
diff --git a/activerecord/lib/active_record/associations/has_many_association.rb b/activerecord/lib/active_record/associations/has_many_association.rb
index 6f67934a79..5972846940 100644
--- a/activerecord/lib/active_record/associations/has_many_association.rb
+++ b/activerecord/lib/active_record/associations/has_many_association.rb
@@ -52,7 +52,11 @@ 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 = counter_cache_value || scope.count(:all)
+          count = if reflection.has_cached_counter?
+            owner._read_attribute(reflection.counter_cache_column).to_i
+          else
+            scope.count(:all)
+          end
 
           # 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
@@ -62,14 +66,6 @@ module ActiveRecord
           [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 bf44be1811..7d669198ca 100644
--- a/activerecord/test/cases/associations/has_many_associations_test.rb
+++ b/activerecord/test/cases/associations/has_many_associations_test.rb
@@ -1230,10 +1230,7 @@ class HasManyAssociationsTest < ActiveRecord::TestCase
     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_queries(1) do
-      assert_equal ship.treasures.size, 0
-      assert_predicate ship.treasures, :loaded?
-    end
+    assert_equal ship.treasures.size, 0
 
     assert_no_difference lambda { ship.reload.treasures_count }, "treasures_count should not be changed" do
       ship.treasures.create(name: "Gold")
@@ -1354,20 +1351,6 @@ 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
 
@@ -1755,6 +1738,14 @@ class HasManyAssociationsTest < ActiveRecord::TestCase
     assert_nil posts.first
   end
 
+  def test_destroy_all_on_desynced_counter_cache_association
+    category = categories(:general)
+    assert_operator category.categorizations.count, :>, 0
+
+    category.categorizations.destroy_all
+    assert_equal 0, category.categorizations.count
+  end
+
   def test_destroy_on_association_clears_scope
     author = Author.create!(name: "Gannon")
     posts = author.posts
@@ -2004,6 +1995,11 @@ 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 0, car.engines.size
+  end
+
   def test_ids_reader_cache_not_used_for_size_when_association_is_dirty
     firm = Firm.create!(name: "Startup")
     assert_equal 0, firm.client_ids.size
@@ -2019,24 +2015,6 @@ class HasManyAssociationsTest < ActiveRecord::TestCase
     assert_equal [3, 11], firm.client_ids
   end
 
-  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
     assert_equal [readers(:michael_welcome).id], posts(:welcome).readers_with_person_ids
   end