From 47e3bbeb9057b37c244330cc4e745c8a8090e8c5 Mon Sep 17 00:00:00 2001 From: Ryuta Kamizono Date: Wed, 13 Feb 2019 22:32:40 +0900 Subject: [PATCH] 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] --- .../associations/collection_association.rb | 3 +- .../associations/has_many_association.rb | 14 ++---- .../has_many_associations_test.rb | 50 ++++++------------- 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