Merge pull request rails#44607 from matthewd/mark-written-v2

Re-deprecate return/throw exits from transactions

Author: matthewd

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

@@ -326,6 +326,13 @@ def transaction(requires_new: nil, isolation: nil, joinable: true, &block)
                :disable_lazy_transactions!, :enable_lazy_transactions!, :dirty_current_transaction,
                to: :transaction_manager
 
+      def mark_transaction_written_if_write(sql) # :nodoc:
+        transaction = current_transaction
+        if transaction.open?
+          transaction.written ||= write_query?(sql)
+        end
+      end
+
       def transaction_open?
         current_transaction.open?
       end

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

@@ -87,6 +87,7 @@ def dirty!; end
 
     class Transaction # :nodoc:
       attr_reader :connection, :state, :savepoint_name, :isolation_level
+      attr_accessor :written, :written_indirectly
 
       def initialize(connection, isolation: nil, joinable: true, run_commit_callbacks: false)
         @connection = connection
@@ -383,6 +384,10 @@ def commit_transaction
 
           dirty_current_transaction if transaction.dirty?
 
+          if current_transaction.open?
+            current_transaction.written_indirectly ||= transaction.written || transaction.written_indirectly
+          end
+
           transaction.commit
           transaction.commit_records
         end
@@ -419,19 +424,37 @@ def within_new_transaction(isolation: nil, joinable: true)
               # @connection still holds an open or invalid transaction, so we must not
               # put it back in the pool for reuse.
               @connection.throw_away! unless transaction.state.rolledback?
-            elsif Thread.current.status == "aborting" || !completed
-              # The transaction is still open but the block returned earlier.
-              #
-              # The block could return early because of a timeout or because the thread is aborting,
-              # so we are rolling back to make sure the timeout didn't caused the transaction to be
-              # committed incompletely.
-              rollback_transaction
             else
-              begin
-                commit_transaction
-              rescue Exception
-                rollback_transaction(transaction) unless transaction.state.completed?
-                raise
+              if Thread.current.status == "aborting"
+                rollback_transaction
+              elsif !completed && transaction.written
+                # This was deprecated in 6.1, and has now changed to a rollback
+                rollback_transaction
+              elsif !completed && !transaction.written_indirectly
+                # This was a silent commit in 6.1, but now becomes a rollback; we skipped
+                # the warning because (having not been written) the change generally won't
+                # have any effect
+                rollback_transaction
+              else
+                if !completed && transaction.written_indirectly
+                  # This is the case that was missed in the 6.1 deprecation, so we have to
+                  # do it now
+                  ActiveSupport::Deprecation.warn(<<~EOW)
+                    Using `return`, `break` or `throw` to exit a transaction block is
+                    deprecated without replacement. If the `throw` came from
+                    `Timeout.timeout(duration)`, pass an exception class as a second
+                    argument so it doesn't use `throw` to abort its block. This results
+                    in the transaction being committed, but in the next release of Rails
+                    it will rollback.
+                  EOW
+                end
+
+                begin
+                  commit_transaction
+                rescue Exception
+                  rollback_transaction(transaction) unless transaction.state.completed?
+                  raise
+                end
               end
             end
           end

activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb

@@ -644,6 +644,7 @@ def extended_type_map_key
 
         def raw_execute(sql, name, async: false)
           materialize_transactions
+          mark_transaction_written_if_write(sql)
 
           log(sql, name, async: async) do
             ActiveSupport::Dependencies.interlock.permit_concurrent_loads do

activerecord/lib/active_record/connection_adapters/mysql/database_statements.rb

@@ -168,6 +168,7 @@ def exec_stmt_and_free(sql, name, binds, cache_stmt: false, async: false)
             check_if_write_query(sql)
 
             materialize_transactions
+            mark_transaction_written_if_write(sql)
 
             # make sure we carry over any changes to ActiveRecord.default_timezone that have been
             # made since we established the connection

activerecord/lib/active_record/connection_adapters/postgresql/database_statements.rb

@@ -12,6 +12,7 @@ def explain(arel, binds = [])
         # Queries the database and returns the results in an Array-like object
         def query(sql, name = nil) # :nodoc:
           materialize_transactions
+          mark_transaction_written_if_write(sql)
 
           log(sql, name) do
             ActiveSupport::Dependencies.interlock.permit_concurrent_loads do
@@ -40,6 +41,7 @@ def execute(sql, name = nil)
           check_if_write_query(sql)
 
           materialize_transactions
+          mark_transaction_written_if_write(sql)
 
           log(sql, name) do
             ActiveSupport::Dependencies.interlock.permit_concurrent_loads do

activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb

@@ -760,6 +760,7 @@ def execute_and_clear(sql, name, binds, prepare: false, async: false)
 
         def exec_no_cache(sql, name, binds, async: false)
           materialize_transactions
+          mark_transaction_written_if_write(sql)
 
           # make sure we carry over any changes to ActiveRecord.default_timezone that have been
           # made since we established the connection
@@ -775,6 +776,7 @@ def exec_no_cache(sql, name, binds, async: false)
 
         def exec_cache(sql, name, binds, async: false)
           materialize_transactions
+          mark_transaction_written_if_write(sql)
           update_typemap_for_default_timezone
 
           stmt_key = prepare_statement(sql, binds)

activerecord/lib/active_record/connection_adapters/sqlite3/database_statements.rb

@@ -25,6 +25,7 @@ def execute(sql, name = nil) # :nodoc:
           check_if_write_query(sql)
 
           materialize_transactions
+          mark_transaction_written_if_write(sql)
 
           log(sql, name) do
             ActiveSupport::Dependencies.interlock.permit_concurrent_loads do
@@ -38,6 +39,7 @@ def exec_query(sql, name = nil, binds = [], prepare: false, async: false) # :nod
           check_if_write_query(sql)
 
           materialize_transactions
+          mark_transaction_written_if_write(sql)
 
           type_casted_binds = type_casted_binds(binds)
 
@@ -121,6 +123,7 @@ def execute_batch(statements, name = nil)
             check_if_write_query(sql)
 
             materialize_transactions
+            mark_transaction_written_if_write(sql)
 
             log(sql, name) do
               ActiveSupport::Dependencies.interlock.permit_concurrent_loads do

activerecord/test/cases/transactions_test.rb

@@ -183,11 +183,65 @@ def transaction_with_return
     end
   end
 
+  def transaction_with_shallow_return
+    Topic.transaction do
+      Topic.transaction(requires_new: true) do
+        @first.approved  = true
+        @second.approved = false
+        @first.save
+        @second.save
+      end
+      return
+    end
+  end
+
   def test_add_to_null_transaction
     topic = Topic.new
     topic.send(:add_to_transaction)
   end
 
+  def test_successful_with_return_outside_inner_transaction
+    committed = false
+
+    Topic.connection.class_eval do
+      alias :real_commit_db_transaction :commit_db_transaction
+      define_method(:commit_db_transaction) do
+        committed = true
+        real_commit_db_transaction
+      end
+    end
+
+    assert_deprecated do
+      transaction_with_shallow_return
+    end
+    assert committed
+
+    assert_predicate Topic.find(1), :approved?, "First should have been approved"
+    assert_not_predicate Topic.find(2), :approved?, "Second should have been unapproved"
+  ensure
+    Topic.connection.class_eval do
+      remove_method :commit_db_transaction
+      alias :commit_db_transaction :real_commit_db_transaction rescue nil
+    end
+  end
+
+  def test_deprecation_on_ruby_timeout_outside_inner_transaction
+    assert_deprecated do
+      catch do |timeout|
+        Topic.transaction do
+          Topic.transaction(requires_new: true) do
+            @first.approved = true
+            @first.save!
+          end
+
+          throw timeout
+        end
+      end
+    end
+
+    assert Topic.find(1).approved?, "First should have been approved"
+  end
+
   def test_rollback_with_return
     committed = false