1 files changed, 32 insertions, 42 deletions

diff --git a/activerecord/lib/active_record/transactions.rb b/activerecord/lib/active_record/transactions.rb
index 08417aaa0f..c5d5fca672 100644
--- a/activerecord/lib/active_record/transactions.rb
+++ b/activerecord/lib/active_record/transactions.rb
@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 module ActiveRecord
   # See ActiveRecord::Transactions::ClassMethods for documentation.
   module Transactions
@@ -123,7 +125,7 @@ module ActiveRecord
     #     # statement will cause a PostgreSQL error, even though the unique
     #     # constraint is no longer violated:
     #     Number.create(i: 1)
-    #     # => "PGError: ERROR:  current transaction is aborted, commands
+    #     # => "PG::Error: ERROR:  current transaction is aborted, commands
     #     #     ignored until end of transaction block"
     #   end
     #
@@ -168,7 +170,7 @@ module ActiveRecord
     # writing, the only database that we're aware of that supports true nested
     # transactions, is MS-SQL. Because of this, Active Record emulates nested
     # transactions by using savepoints on MySQL and PostgreSQL. See
-    # http://dev.mysql.com/doc/refman/5.7/en/savepoint.html
+    # https://dev.mysql.com/doc/refman/5.7/en/savepoint.html
     # for more information about savepoints.
     #
     # === \Callbacks
@@ -188,7 +190,7 @@ module ActiveRecord
     #
     # === Caveats
     #
-    # If you're on MySQL, then do not use Data Definition Language(DDL) operations in nested
+    # If you're on MySQL, then do not use Data Definition Language (DDL) operations in nested
     # transactions blocks that are emulated with savepoints. That is, do not execute statements
     # like 'CREATE TABLE' inside such blocks. This is because MySQL automatically
     # releases all savepoints upon executing a DDL operation. When +transaction+
@@ -283,7 +285,7 @@ module ActiveRecord
             fire_on = Array(options[:on])
             assert_valid_transaction_action(fire_on)
             options[:if] = Array(options[:if])
-            options[:if] << "transaction_include_any_action?(#{fire_on})"
+            options[:if].unshift(-> { transaction_include_any_action?(fire_on) })
           end
         end
 
@@ -304,9 +306,7 @@ module ActiveRecord
     end
 
     def save(*) #:nodoc:
-      rollback_active_record_state! do
-        with_transaction_returning_status { super }
-      end
+      with_transaction_returning_status { super }
     end
 
     def save!(*) #:nodoc:
@@ -317,17 +317,6 @@ module ActiveRecord
       with_transaction_returning_status { super }
     end
 
-    # Reset id and @new_record if the transaction rolls back.
-    def rollback_active_record_state!
-      remember_transaction_record_state
-      yield
-    rescue Exception
-      restore_transaction_record_state
-      raise
-    ensure
-      clear_transaction_record_state
-    end
-
     def before_committed! # :nodoc:
       _run_before_commit_without_transaction_enrollment_callbacks
       _run_before_commit_callbacks
@@ -338,11 +327,13 @@ module ActiveRecord
     # Ensure that it is not called if the object was never persisted (failed create),
     # but call it after the commit of a destroyed object.
     def committed!(should_run_callbacks: true) #:nodoc:
-      if should_run_callbacks && destroyed? || persisted?
+      if should_run_callbacks && (destroyed? || persisted?)
+        @_committed_already_called = true
         _run_commit_without_transaction_enrollment_callbacks
         _run_commit_callbacks
       end
     ensure
+      @_committed_already_called = false
       force_clear_transaction_record_state
     end
 
@@ -380,13 +371,7 @@ module ActiveRecord
       status = nil
       self.class.transaction do
         add_to_transaction
-        begin
-          status = yield
-        rescue ActiveRecord::Rollback
-          clear_transaction_record_state
-          status = nil
-        end
-
+        status = yield
         raise ActiveRecord::Rollback unless status
       end
       status
@@ -397,16 +382,26 @@ module ActiveRecord
     end
 
     private
+      attr_reader :_committed_already_called, :_trigger_update_callback, :_trigger_destroy_callback
 
       # Save the new record state and id of a record so it can be restored later if a transaction fails.
       def remember_transaction_record_state
-        @_start_transaction_state[:id] = id
         @_start_transaction_state.reverse_merge!(
+          id: id,
           new_record: @new_record,
           destroyed: @destroyed,
           frozen?: frozen?,
         )
         @_start_transaction_state[:level] = (@_start_transaction_state[:level] || 0) + 1
+        remember_new_record_before_last_commit
+      end
+
+      def remember_new_record_before_last_commit
+        if _committed_already_called
+          @_new_record_before_last_commit = false
+        else
+          @_new_record_before_last_commit = @_start_transaction_state[:new_record]
+        end
       end
 
       # Clear the new record state and id of a record.
@@ -430,30 +425,24 @@ module ActiveRecord
             @new_record = restore_state[:new_record]
             @destroyed  = restore_state[:destroyed]
             pk = self.class.primary_key
-            if pk && read_attribute(pk) != restore_state[:id]
-              write_attribute(pk, restore_state[:id])
+            if pk && _read_attribute(pk) != restore_state[:id]
+              _write_attribute(pk, restore_state[:id])
             end
             freeze if restore_state[:frozen?]
           end
         end
       end
 
-      # Determine if a record was created or destroyed in a transaction. State should be one of :new_record or :destroyed.
-      def transaction_record_state(state)
-        @_start_transaction_state[state]
-      end
-
       # Determine if a transaction included an action for :create, :update, or :destroy. Used in filtering callbacks.
       def transaction_include_any_action?(actions)
         actions.any? do |action|
           case action
           when :create
-            transaction_record_state(:new_record)
-          when :destroy
-            defined?(@_trigger_destroy_callback) && @_trigger_destroy_callback
+            persisted? && @_new_record_before_last_commit
           when :update
-            !(transaction_record_state(:new_record) || destroyed?) &&
-              (defined?(@_trigger_update_callback) && @_trigger_update_callback)
+            !(@_new_record_before_last_commit || destroyed?) && _trigger_update_callback
+          when :destroy
+            _trigger_destroy_callback
           end
         end
       end
@@ -470,7 +459,7 @@ module ActiveRecord
       # if it's associated with a transaction, then the state of the Active Record
       # object will be updated to reflect the current state of the transaction.
       #
-      # The +@transaction_state+ variable stores the states of the associated
+      # The <tt>@transaction_state</tt> variable stores the states of the associated
       # transaction. This relies on the fact that a transaction can only be in
       # one rollback or commit (otherwise a list of states would be required).
       # Each Active Record object inside of a transaction carries that transaction's
@@ -489,8 +478,9 @@ module ActiveRecord
 
       def update_attributes_from_transaction_state(transaction_state)
         if transaction_state && transaction_state.finalized?
-          restore_transaction_record_state if transaction_state.rolledback?
-          clear_transaction_record_state
+          restore_transaction_record_state(transaction_state.fully_rolledback?) if transaction_state.rolledback?
+          force_clear_transaction_record_state if transaction_state.fully_committed?
+          clear_transaction_record_state if transaction_state.fully_completed?
         end
       end
   end