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

This is a 4th attempt to make counter cache transactional completely.

Past attempts: #9236, #14849, #23357.

All existing counter cache issues (increment/decrement twice, lost
increment) are caused due to updating counter cache on the outside of
the record saving transaction by assigning belongs_to record, even
though assigning that doesn't cause the record saving.

We have the `@_after_replace_counter_called` guard condition to mitigate
double increment/decrement issues, but we can't completely prevent that
inconsistency as long as updating counter cache on the outside of the
transaction, since saving the record is not always happened after that.

We already have handling counter cache after create/update/destroy,

https://github.com/rails/rails/blob/1b90f614b1b3d06b7f02a8b9ea6cd84f15d58643/activerecord/lib/active_record/counter_cache.rb#L162-L189
https://github.com/rails/rails/blob/1b90f614b1b3d06b7f02a8b9ea6cd84f15d58643/activerecord/lib/active_record/associations/builder/belongs_to.rb#L33-L59

so just removing assigning logic on the belongs_to association makes
counter cache transactional completely.

Closes #14849.
Closes #23357.
Closes #31493.
Closes #31494.
Closes #32372.
Closes #33113.
Closes #33117
Closes #33129.
Closes #33458.

5 files changed, 43 insertions, 29 deletions

diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
index 477bc1b54a..f79ed01122 100644
--- a/activerecord/CHANGELOG.md
+++ b/activerecord/CHANGELOG.md
@@ -1,3 +1,9 @@
+*   Don't update counter cache unless the record is actually saved.
+
+    Fixes #31493, #33113, #33117.
+
+    *Ryuta Kamizono*
+
 *   SQLite3 adapter supports expression indexes.
 
     ```
diff --git a/activerecord/lib/active_record/associations/belongs_to_association.rb b/activerecord/lib/active_record/associations/belongs_to_association.rb
index 3d4ad1dd5b..544aec5e8b 100644
--- a/activerecord/lib/active_record/associations/belongs_to_association.rb
+++ b/activerecord/lib/active_record/associations/belongs_to_association.rb
@@ -50,11 +50,8 @@ module ActiveRecord
         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 @@ module ActiveRecord
           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
diff --git a/activerecord/lib/active_record/associations/belongs_to_polymorphic_association.rb b/activerecord/lib/active_record/associations/belongs_to_polymorphic_association.rb
index 3fd2fb5f67..9ae452e7a1 100644
--- a/activerecord/lib/active_record/associations/belongs_to_polymorphic_association.rb
+++ b/activerecord/lib/active_record/associations/belongs_to_polymorphic_association.rb
@@ -19,10 +19,6 @@ module ActiveRecord
           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
diff --git a/activerecord/lib/active_record/associations/builder/belongs_to.rb b/activerecord/lib/active_record/associations/builder/belongs_to.rb
index 0166ed98ca..da4cc343eb 100644
--- a/activerecord/lib/active_record/associations/builder/belongs_to.rb
+++ b/activerecord/lib/active_record/associations/builder/belongs_to.rb
@@ -34,9 +34,7 @@ module ActiveRecord::Associations::Builder # :nodoc:
           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)
diff --git a/activerecord/test/cases/associations/belongs_to_associations_test.rb b/activerecord/test/cases/associations/belongs_to_associations_test.rb
index 169ff96b94..43763dc715 100644
--- a/activerecord/test/cases/associations/belongs_to_associations_test.rb
+++ b/activerecord/test/cases/associations/belongs_to_associations_test.rb
@@ -452,15 +452,39 @@ class BelongsToAssociationsTest < ActiveRecord::TestCase
   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 @@ class BelongsToAssociationsTest < ActiveRecord::TestCase
     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 @@ class BelongsToAssociationsTest < ActiveRecord::TestCase
     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 @@ class BelongsToAssociationsTest < ActiveRecord::TestCase
     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 @@ class BelongsToAssociationsTest < ActiveRecord::TestCase
 
     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