From f1b64dff47fcd0f05bbba1ec88e37a62b9f0b48f Mon Sep 17 00:00:00 2001
From: Bogdan Gusiev <agresso@gmail.com>
Date: Fri, 1 Feb 2019 15:58:09 +0200
Subject: Bugfix association loading behavior when counter cache is zero
---
.../associations/collection_association.rb | 5 +--
.../associations/has_many_association.rb | 24 ++++++-------
.../associations/has_many_associations_test.rb | 40 +++++++++++++++++++---
3 files changed, 48 insertions(+), 21 deletions(-)
(limited to 'activerecord')
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
--
cgit v1.2.3