Improve consistency of counter caches updating in memory

When we made sure that the counter gets updated in memory, we only did
it on the has many side. The has many side only does the update if the
belongs to cannot. The belongs to side was updated to update the counter
cache (if it is able). This means that we need to check if the
belongs_to is able to update in memory on the has_many side.

We also found an inconsistency where the reflection names were used to
grab the association which should update the counter cache. Since
reflection names are now strings, this means it was using a different
instance than the one which would have the inverse instance set.

Fixes #18689

[Sean Griffin & anthonynavarre]

5 files changed, 29 insertions, 7 deletions

diff --git a/activerecord/lib/active_record/associations/belongs_to_association.rb b/activerecord/lib/active_record/associations/belongs_to_association.rb
index c63b42e2a0..265a65c4c1 100644
--- a/activerecord/lib/active_record/associations/belongs_to_association.rb
+++ b/activerecord/lib/active_record/associations/belongs_to_association.rb
@@ -68,6 +68,9 @@ module ActiveRecord
         def increment_counter(counter_cache_name)
           if foreign_key_present?
             klass.increment_counter(counter_cache_name, target_id)
+            if target && !stale_target?
+              target.increment(counter_cache_name)
+            end
           end
         end
 
diff --git a/activerecord/lib/active_record/associations/has_many_association.rb b/activerecord/lib/active_record/associations/has_many_association.rb
index 2a782c06d0..b574185561 100644
--- a/activerecord/lib/active_record/associations/has_many_association.rb
+++ b/activerecord/lib/active_record/associations/has_many_association.rb
@@ -101,7 +101,7 @@ module ActiveRecord
         end
 
         def update_counter_in_memory(difference, reflection = reflection())
-          if has_cached_counter?(reflection)
+          if counter_must_be_updated_by_has_many?(reflection)
             counter = cached_counter_attribute_name(reflection)
             owner[counter] += difference
             owner.send(:clear_attribute_changes, counter) # eww
@@ -118,18 +118,28 @@ module ActiveRecord
         #     it will be decremented twice.
         #
         # Hence this method.
-        def inverse_updates_counter_cache?(reflection = reflection())
+        def inverse_which_updates_counter_cache(reflection = reflection())
           counter_name = cached_counter_attribute_name(reflection)
-          inverse_updates_counter_named?(counter_name, reflection)
+          inverse_which_updates_counter_named(counter_name, reflection)
         end
+        alias inverse_updates_counter_cache? inverse_which_updates_counter_cache
 
-        def inverse_updates_counter_named?(counter_name, reflection = reflection())
-          reflection.klass._reflections.values.any? { |inverse_reflection|
+        def inverse_which_updates_counter_named(counter_name, reflection)
+          reflection.klass._reflections.values.find { |inverse_reflection|
             inverse_reflection.belongs_to? &&
             inverse_reflection.counter_cache_column == counter_name
           }
         end
 
+        def inverse_updates_counter_in_memory?(reflection)
+          inverse = inverse_which_updates_counter_cache(reflection)
+          inverse && inverse == reflection.inverse_of
+        end
+
+        def counter_must_be_updated_by_has_many?(reflection)
+          !inverse_updates_counter_in_memory?(reflection) && has_cached_counter?(reflection)
+        end
+
         def delete_count(method, scope)
           if method == :delete_all
             scope.delete_all
diff --git a/activerecord/lib/active_record/counter_cache.rb b/activerecord/lib/active_record/counter_cache.rb
index 7d8e0a2063..82596b63df 100644
--- a/activerecord/lib/active_record/counter_cache.rb
+++ b/activerecord/lib/active_record/counter_cache.rb
@@ -156,7 +156,7 @@ module ActiveRecord
 
       def each_counter_cached_associations
         _reflections.each do |name, reflection|
-          yield association(name) if reflection.belongs_to? && reflection.counter_cache_column
+          yield association(name.to_sym) if reflection.belongs_to? && reflection.counter_cache_column
         end
       end
 
diff --git a/activerecord/test/cases/counter_cache_test.rb b/activerecord/test/cases/counter_cache_test.rb
index 96a0edea34..1f5055b2a2 100644
--- a/activerecord/test/cases/counter_cache_test.rb
+++ b/activerecord/test/cases/counter_cache_test.rb
@@ -189,4 +189,13 @@ class CounterCacheTest < ActiveRecord::TestCase
     assert_equal 2, car.engines_count
     assert_equal 2, car.reload.engines_count
   end
+
+  test "counter caches are updated in memory when the default value is nil" do
+    car = Car.new(engines_count: nil)
+    car.engines = [Engine.new, Engine.new]
+    car.save!
+
+    assert_equal 2, car.engines_count
+    assert_equal 2, car.reload.engines_count
+  end
 end
diff --git a/activerecord/test/models/car.rb b/activerecord/test/models/car.rb
index db0f93f63b..81263b79d1 100644
--- a/activerecord/test/models/car.rb
+++ b/activerecord/test/models/car.rb
@@ -8,7 +8,7 @@ class Car < ActiveRecord::Base
   has_one :bulb
 
   has_many :tyres
-  has_many :engines, :dependent => :destroy
+  has_many :engines, :dependent => :destroy, inverse_of: :my_car
   has_many :wheels, :as => :wheelable, :dependent => :destroy
 
   scope :incl_tyres, -> { includes(:tyres) }