Merge pull request #39453 from dylanahsmith/transaction-return-no-raise

activerecord: No warning for return out of transaction block without writes

activerecord/CHANGELOG.md

@@ -253,7 +253,7 @@
 
     *Eugene Kenny*
 
-*   Deprecate using `return`, `break` or `throw` to exit a transaction block.
+*   Deprecate using `return`, `break` or `throw` to exit a transaction block after writes.
 
     *Dylan Thacker-Smith*
 

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

@@ -323,6 +323,13 @@ def transaction(requires_new: nil, isolation: nil, joinable: true)
                :commit_transaction, :rollback_transaction, :materialize_transactions,
                :disable_lazy_transactions!, :enable_lazy_transactions!, 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

@@ -75,6 +75,7 @@ def add_record(record, _ = true); end
 
     class Transaction #:nodoc:
       attr_reader :connection, :state, :savepoint_name, :isolation_level
+      attr_accessor :written
 
       def initialize(connection, isolation: nil, joinable: true, run_commit_callbacks: false)
         @connection = connection
@@ -320,14 +321,14 @@ def within_new_transaction(isolation: nil, joinable: true)
             if Thread.current.status == "aborting"
               rollback_transaction
             else
-              unless completed
+              if !completed && transaction.written
                 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 raise and rollback.
+                  it will rollback.
                 EOW
               end
               begin

activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb

@@ -191,6 +191,7 @@ def clear_cache! # :nodoc:
       # Executes the SQL statement in the context of this connection.
       def execute(sql, name = nil)
         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/mysql/database_statements.rb

@@ -154,6 +154,7 @@ def exec_stmt_and_free(sql, name, binds, cache_stmt: false)
             end
 
             materialize_transactions
+            mark_transaction_written_if_write(sql)
 
             # make sure we carry over any changes to ActiveRecord::Base.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
@@ -39,6 +40,7 @@ def execute(sql, name = nil)
           end
 
           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

@@ -651,6 +651,7 @@ def execute_and_clear(sql, name, binds, prepare: false)
 
         def exec_no_cache(sql, name, binds)
           materialize_transactions
+          mark_transaction_written_if_write(sql)
 
           # make sure we carry over any changes to ActiveRecord::Base.default_timezone that have been
           # made since we established the connection
@@ -666,6 +667,7 @@ def exec_no_cache(sql, name, binds)
 
         def exec_cache(sql, name, binds)
           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

@@ -24,6 +24,7 @@ def execute(sql, name = nil) #:nodoc:
           end
 
           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)
           end
 
           materialize_transactions
+          mark_transaction_written_if_write(sql)
 
           type_casted_binds = type_casted_binds(binds)
 
@@ -113,6 +115,7 @@ def execute_batch(statements, name = nil)
             end
 
             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

@@ -177,6 +177,14 @@ def test_deprecation_on_ruby_timeout
     assert Topic.find(1).approved?, "First should have been approved"
   end
 
+  def test_early_return_from_transaction
+    assert_not_deprecated do
+      @first.with_lock do
+        break
+      end
+    end
+  end
+
   def test_number_of_transactions_in_commit
     num = nil