Fix that after commit callbacks on update does not triggered when optimistic locking is enabled

This issue is caused by `@_trigger_update_callback` won't be updated due
to `_update_record` in `Locking::Optimistic` doesn't call `super` when
optimistic locking is enabled.

Now optimistic locking concern when updating is supported by
`_update_row` low level API, therefore overriding `_update_record` is no
longer necessary.

Removing the method just fix the issue.

Closes #29096.
Closes #29321.
Closes #30823.

3 files changed, 42 insertions, 53 deletions

diff --git a/activerecord/lib/active_record/locking/optimistic.rb b/activerecord/lib/active_record/locking/optimistic.rb
index af361a8d2d..7f096bb532 100644
--- a/activerecord/lib/active_record/locking/optimistic.rb
+++ b/activerecord/lib/active_record/locking/optimistic.rb
@@ -61,13 +61,6 @@ module ActiveRecord
       end
 
       private
-
-        def increment_lock
-          lock_col = self.class.locking_column
-          previous_lock_value = send(lock_col)
-          send("#{lock_col}=", previous_lock_value + 1)
-        end
-
         def _create_record(attribute_names = self.attribute_names, *)
           if locking_enabled?
             # We always want to persist the locking version, even if we don't detect
@@ -77,41 +70,13 @@ module ActiveRecord
           super
         end
 
-        def _update_record(attribute_names = self.attribute_names)
-          attribute_names &= self.class.column_names
-          return super unless locking_enabled?
-          return 0 if attribute_names.empty?
-
-          begin
-            lock_col = self.class.locking_column
-
-            previous_lock_value = read_attribute_before_type_cast(lock_col)
-
-            increment_lock
-
-            attribute_names.push(lock_col)
-
-            affected_rows = self.class._update_record(
-              attributes_with_values_for_update(attribute_names),
-              self.class.primary_key => id_in_database,
-              lock_col => previous_lock_value
-            )
-
-            unless affected_rows == 1
-              raise ActiveRecord::StaleObjectError.new(self, "update")
-            end
-
-            affected_rows
-
-          # If something went wrong, revert the locking_column value.
-          rescue Exception
-            send("#{lock_col}=", previous_lock_value.to_i)
-
-            raise
-          end
+        def _touch_row(attribute_names, time)
+          super
+        ensure
+          clear_attribute_change(self.class.locking_column) if locking_enabled?
         end
 
-        def _update_row(attribute_names, attempted_action)
+        def _update_row(attribute_names, attempted_action = "update")
           return super unless locking_enabled?
 
           begin
@@ -135,10 +100,8 @@ module ActiveRecord
 
           # If something went wrong, revert the locking_column value.
           rescue Exception
-            self[locking_column] = previous_lock_value
+            self[locking_column] = previous_lock_value.to_i
             raise
-          ensure
-            clear_attribute_change(locking_column)
           end
         end
 
diff --git a/activerecord/lib/active_record/persistence.rb b/activerecord/lib/active_record/persistence.rb
index ce16587326..d4f4a5887a 100644
--- a/activerecord/lib/active_record/persistence.rb
+++ b/activerecord/lib/active_record/persistence.rb
@@ -656,18 +656,11 @@ module ActiveRecord
         MSG
       end
 
-      time ||= current_time_from_proper_timezone
       attribute_names = timestamp_attributes_for_update_in_model
       attribute_names.concat(names)
 
       unless attribute_names.empty?
-        attribute_names.each do |attr_name|
-          write_attribute(attr_name, time)
-          clear_attribute_change(attr_name)
-        end
-
-        affected_rows = _update_row(attribute_names, "touch")
-
+        affected_rows = _touch_row(attribute_names, time)
         @_trigger_update_callback = affected_rows == 1
       else
         true
@@ -688,7 +681,18 @@ module ActiveRecord
       self.class._delete_record(self.class.primary_key => id_in_database)
     end
 
-    def _update_row(attribute_names, attempted_action)
+    def _touch_row(attribute_names, time)
+      time ||= current_time_from_proper_timezone
+
+      attribute_names.each do |attr_name|
+        write_attribute(attr_name, time)
+        clear_attribute_change(attr_name)
+      end
+
+      _update_row(attribute_names, "touch")
+    end
+
+    def _update_row(attribute_names, attempted_action = "update")
       self.class._update_record(
         attributes_with_values(attribute_names),
         self.class.primary_key => id_in_database
@@ -712,7 +716,7 @@ module ActiveRecord
         affected_rows = 0
         @_trigger_update_callback = true
       else
-        affected_rows = _update_row(attribute_names, "update")
+        affected_rows = _update_row(attribute_names)
         @_trigger_update_callback = affected_rows == 1
       end
 
diff --git a/activerecord/test/cases/transaction_callbacks_test.rb b/activerecord/test/cases/transaction_callbacks_test.rb
index 1c7cec4dad..e89ac53732 100644
--- a/activerecord/test/cases/transaction_callbacks_test.rb
+++ b/activerecord/test/cases/transaction_callbacks_test.rb
@@ -394,6 +394,28 @@ class TransactionCallbacksTest < ActiveRecord::TestCase
     end
 end
 
+class TransactionAfterCommitCallbacksWithOptimisticLockingTest < ActiveRecord::TestCase
+  class PersonWithCallbacks < ActiveRecord::Base
+    self.table_name = :people
+
+    after_create_commit { |record| record.history << :commit_on_create }
+    after_update_commit { |record| record.history << :commit_on_update }
+    after_destroy_commit { |record| record.history << :commit_on_destroy }
+
+    def history
+      @history ||= []
+    end
+  end
+
+  def test_after_commit_callbacks_with_optimistic_locking
+    person = PersonWithCallbacks.create!(first_name: "first name")
+    person.update!(first_name: "another name")
+    person.destroy
+
+    assert_equal [:commit_on_create, :commit_on_update, :commit_on_destroy], person.history
+  end
+end
+
 class CallbacksOnMultipleActionsTest < ActiveRecord::TestCase
   self.use_transactional_tests = false