diff options
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 f6fdbcde54..4be2edbf30 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 5921193374..23a6dc04ad 100644 --- a/activerecord/test/cases/associations/has_many_associations_test.rb +++ b/activerecord/test/cases/associations/has_many_associations_test.rb @@ -1223,12 +1223,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") @@ -1349,6 +1352,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 @@ -1969,9 +1986,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 |