Merge pull request #33913 from kamipo/counter_cache

Don't update counter cache unless the record is actually saved

activerecord/CHANGELOG.md

+*   Don't update counter cache unless the record is actually saved.
+
+    Fixes #31493, #33113, #33117.
+
+    *Ryuta Kamizono*
+
 *   Deprecate `ActiveRecord::Result#to_hash` in favor of `ActiveRecord::Result#to_a`.
 
     *Gannon McGibbon*, *Kevin Cheng*

activerecord/lib/active_record/associations/belongs_to_association.rb

@@ -50,11 +50,8 @@ def target_changed?
         def replace(record)
           if record
             raise_on_type_mismatch!(record)
-            update_counters_on_replace(record)
             set_inverse_instance(record)
             @updated = true
-          else
-            decrement_counters
           end
 
           replace_keys(record)
@@ -80,19 +77,6 @@ def require_counter_update?
           reflection.counter_cache_column && owner.persisted?
         end
 
-        def update_counters_on_replace(record)
-          if require_counter_update? && different_target?(record)
-            owner.instance_variable_set :@_after_replace_counter_called, true
-            record.increment!(reflection.counter_cache_column, touch: reflection.options[:touch])
-            decrement_counters
-          end
-        end
-
-        # Checks whether record is different to the current target, without loading it
-        def different_target?(record)
-          record._read_attribute(primary_key(record)) != owner._read_attribute(reflection.foreign_key)
-        end
-
         def replace_keys(record)
           owner[reflection.foreign_key] = record ? record._read_attribute(primary_key(record)) : nil
         end

activerecord/lib/active_record/associations/belongs_to_polymorphic_association.rb

@@ -19,10 +19,6 @@ def replace_keys(record)
           owner[reflection.foreign_type] = record ? record.class.polymorphic_name : nil
         end
 
-        def different_target?(record)
-          super || record.class != klass
-        end
-
         def inverse_reflection_for(record)
           reflection.polymorphic_inverse_of(record.class)
         end

activerecord/lib/active_record/associations/builder/belongs_to.rb

@@ -34,9 +34,7 @@ def belongs_to_counter_cache_after_update(reflection)
           foreign_key  = reflection.foreign_key
           cache_column = reflection.counter_cache_column
 
-          if @_after_replace_counter_called ||= false
-            @_after_replace_counter_called = false
-          elsif association(reflection.name).target_changed?
+          if association(reflection.name).target_changed?
             if reflection.polymorphic?
               model     = attribute_in_database(reflection.foreign_type).try(:constantize)
               model_was = attribute_before_last_save(reflection.foreign_type).try(:constantize)

activerecord/test/cases/associations/belongs_to_associations_test.rb

@@ -452,15 +452,39 @@ def test_belongs_to_counter
   end
 
   def test_belongs_to_counter_with_assigning_nil
-    post = Post.find(1)
-    comment = Comment.find(1)
+    topic = Topic.create!(title: "debate")
+    reply = Reply.create!(title: "blah!", content: "world around!", topic: topic)
 
-    assert_equal post.id, comment.post_id
-    assert_equal 2, Post.find(post.id).comments.size
+    assert_equal topic.id, reply.parent_id
+    assert_equal 1, topic.reload.replies.size
 
-    comment.post = nil
+    reply.topic = nil
+    reply.reload
 
-    assert_equal 1, Post.find(post.id).comments.size
+    assert_equal topic.id, reply.parent_id
+    assert_equal 1, topic.reload.replies.size
+
+    reply.topic = nil
+    reply.save!
+
+    assert_equal 0, topic.reload.replies.size
+  end
+
+  def test_belongs_to_counter_with_assigning_new_object
+    topic = Topic.create!(title: "debate")
+    reply = Reply.create!(title: "blah!", content: "world around!", topic: topic)
+
+    assert_equal topic.id, reply.parent_id
+    assert_equal 1, topic.reload.replies_count
+
+    topic2 = reply.build_topic(title: "debate2")
+    reply.save!
+
+    assert_not_equal topic.id, reply.parent_id
+    assert_equal topic2.id, reply.parent_id
+
+    assert_equal 0, topic.reload.replies_count
+    assert_equal 1, topic2.reload.replies_count
   end
 
   def test_belongs_to_with_primary_key_counter
@@ -485,11 +509,13 @@ def test_belongs_to_with_primary_key_counter
     assert_equal 0, debate2.reload.replies_count
 
     reply.topic_with_primary_key = debate2
+    reply.save!
 
     assert_equal 0, debate.reload.replies_count
     assert_equal 1, debate2.reload.replies_count
 
     reply.topic_with_primary_key = nil
+    reply.save!
 
     assert_equal 0, debate.reload.replies_count
     assert_equal 0, debate2.reload.replies_count
@@ -516,11 +542,13 @@ def test_belongs_to_counter_with_reassigning
     assert_equal 1, Topic.find(topic2.id).replies.size
 
     reply1.topic = nil
+    reply1.save!
 
     assert_equal 0, Topic.find(topic1.id).replies.size
     assert_equal 0, Topic.find(topic2.id).replies.size
 
     reply1.topic = topic1
+    reply1.save!
 
     assert_equal 1, Topic.find(topic1.id).replies.size
     assert_equal 0, Topic.find(topic2.id).replies.size
@@ -594,6 +622,7 @@ def test_belongs_to_touch_with_reassigning
     debate2.touch(time: time)
 
     reply.topic_with_primary_key = debate2
+    reply.save!
 
     assert_operator debate.reload.updated_at, :>, time
     assert_operator debate2.reload.updated_at, :>, time
@@ -772,6 +801,7 @@ def test_counter_cache
 
     reply = Reply.create(title: "re: zoom", content: "speedy quick!")
     reply.topic = topic
+    reply.save!
 
     assert_equal 1, topic.reload[:replies_count]
     assert_equal 1, topic.replies.size