Fix before_commit when updating a record on the callback

activerecord/lib/active_record/connection_adapters/abstract/transaction.rb

@@ -44,11 +44,12 @@ class Transaction #:nodoc:
       attr_reader :connection, :state, :records, :savepoint_name
       attr_writer :joinable
 
-      def initialize(connection, options)
+      def initialize(connection, options, run_commit_callbacks: false)
         @connection = connection
         @state = TransactionState.new
         @records = []
         @joinable = options.fetch(:joinable, true)
+        @run_commit_callbacks = run_commit_callbacks
       end
 
       def add_record(record)
@@ -75,18 +76,21 @@ def commit
       end
 
       def before_commit_records
-        records.uniq.each(&:before_committed!)
+        records.uniq.each(&:before_committed!) if @run_commit_callbacks
       end
 
       def commit_records
         ite = records.uniq
         while record = ite.shift
-          record.committed!
+          if @run_commit_callbacks
+            record.committed!
+          else
+            # if not running callbacks, only adds the record to the parent transaction
+            record.add_to_transaction
+          end
         end
       ensure
-        ite.each do |i|
-          i.committed!(should_run_callbacks: false)
-        end
+        ite.each { |i| i.committed!(should_run_callbacks: false) }
       end
 
       def full_rollback?; true; end
@@ -97,8 +101,8 @@ def open?; !closed?; end
 
     class SavepointTransaction < Transaction
 
-      def initialize(connection, savepoint_name, options)
-        super(connection, options)
+      def initialize(connection, savepoint_name, options, *args)
+        super(connection, options, *args)
         if options[:isolation]
           raise ActiveRecord::TransactionIsolationError, "cannot set transaction isolation in a nested transaction"
         end
@@ -120,7 +124,7 @@ def full_rollback?; false; end
 
     class RealTransaction < Transaction
 
-      def initialize(connection, options)
+      def initialize(connection, options, *args)
         super
         if options[:isolation]
           connection.begin_isolated_db_transaction(options[:isolation])
@@ -147,30 +151,25 @@ def initialize(connection)
       end
 
       def begin_transaction(options = {})
+        run_commit_callbacks = !current_transaction.joinable?
         transaction =
           if @stack.empty?
-            RealTransaction.new(@connection, options)
+            RealTransaction.new(@connection, options, run_commit_callbacks: run_commit_callbacks)
           else
-            SavepointTransaction.new(@connection, "active_record_#{@stack.size}", options)
+            SavepointTransaction.new(@connection, "active_record_#{@stack.size}", options,
+                                     run_commit_callbacks: run_commit_callbacks)
           end
 
         @stack.push(transaction)
         transaction
       end
 
       def commit_transaction
-        inner_transaction = @stack.pop
-
-        if current_transaction.joinable?
-          inner_transaction.commit
-          inner_transaction.records.each do |r|
-            r.add_to_transaction
-          end
-        else
-          inner_transaction.before_commit_records
-          inner_transaction.commit
-          inner_transaction.commit_records
-        end
+        transaction = @stack.last
+        transaction.before_commit_records
+        @stack.pop
+        transaction.commit
+        transaction.commit_records
       end
 
       def rollback_transaction(transaction = nil)

activerecord/test/cases/transaction_callbacks_test.rb

@@ -376,13 +376,18 @@ class TopicWithCallbacksOnMultipleActions < ActiveRecord::Base
     after_commit(on: [:create, :update]) { |record| record.history << :create_and_update }
     after_commit(on: [:update, :destroy]) { |record| record.history << :update_and_destroy }
 
+    before_commit(if: :save_before_commit_history) { |record| record.history << :before_commit }
+    before_commit(if: :update_title) { |record| record.update(title: "before commit title") }
+
     def clear_history
       @history = []
     end
 
     def history
       @history ||= []
     end
+
+    attr_accessor :save_before_commit_history, :update_title
   end
 
   def test_after_commit_on_multiple_actions
@@ -399,6 +404,23 @@ def test_after_commit_on_multiple_actions
     topic.destroy
     assert_equal [:update_and_destroy, :create_and_destroy], topic.history
   end
+
+  def test_before_commit_actions
+    topic = TopicWithCallbacksOnMultipleActions.new
+    topic.save_before_commit_history = true
+    topic.save
+
+    assert_equal [:before_commit, :create_and_update, :create_and_destroy], topic.history
+  end
+
+  def test_before_commit_update_in_same_transaction
+    topic = TopicWithCallbacksOnMultipleActions.new
+    topic.update_title = true
+    topic.save
+
+    assert_equal "before commit title", topic.title
+    assert_equal "before commit title", topic.reload.title
+  end
 end