Merge pull request rails#52007 from rails/fxn/blank-txn

Pass transaction: nil in sql.active_record events if no transaction i…

Author: fxn

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

@@ -277,7 +277,7 @@ def cache_notification_info(sql, name, binds)
             type_casted_binds: -> { type_casted_binds(binds) },
             name: name,
             connection: self,
-            transaction: current_transaction,
+            transaction: current_transaction.presence,
             cached: true
           }
         end

activerecord/lib/active_record/connection_adapters/abstract_adapter.rb

@@ -1118,7 +1118,7 @@ def log(sql, name = "SQL", binds = [], type_casted_binds = [], statement_name =
             statement_name:    statement_name,
             async:             async,
             connection:        self,
-            transaction:       current_transaction,
+            transaction:       current_transaction.presence,
             row_count:         0,
             &block
           )

activerecord/test/cases/instrumentation_test.rb

@@ -170,36 +170,48 @@ def test_payload_connection_with_query_cache_enabled
     ensure
       ActiveSupport::Notifications.unsubscribe(subscriber) if subscriber
     end
+  end
+end
 
-    def test_payload_with_implicit_transaction
-      expected_transaction = Book.current_transaction
+class ActiveRecord::TransactionInSqlActiveRecordPayloadTest < ActiveRecord::TestCase
+  # We need current_transaction to return the null transaction.
+  self.use_transactional_tests = false
 
-      subscriber = ActiveSupport::Notifications.subscribe("sql.active_record") do |event|
-        if event.payload[:name] == "Book Count"
-          assert_same expected_transaction, event.payload[:transaction]
-        end
-      end
+  def test_payload_without_an_open_transaction
+    asserted = false
 
-      Book.count
-    ensure
-      ActiveSupport::Notifications.unsubscribe(subscriber)
+    subscriber = ActiveSupport::Notifications.subscribe("sql.active_record") do |event|
+      if event.payload.fetch(:name) == "Book Count"
+        assert_nil event.payload.fetch(:transaction)
+        asserted = true
+      end
     end
 
-    def test_payload_with_explicit_transaction
-      expected_transaction = nil
+    Book.count
 
-      subscriber = ActiveSupport::Notifications.subscribe("sql.active_record") do |event|
-        if event.payload[:name] == "Book Load"
-          assert_same expected_transaction, event.payload[:transaction]
-        end
-      end
+    assert asserted
+  ensure
+    ActiveSupport::Notifications.unsubscribe(subscriber)
+  end
 
-      Book.transaction do |transaction|
-        expected_transaction = transaction
-        Book.first
+  def test_payload_with_an_open_transaction
+    asserted = false
+    expected_transaction = nil
+
+    subscriber = ActiveSupport::Notifications.subscribe("sql.active_record") do |event|
+      if event.payload.fetch(:name) == "Book Count"
+        assert_same expected_transaction, event.payload.fetch(:transaction)
+        asserted = true
       end
-    ensure
-      ActiveSupport::Notifications.unsubscribe(subscriber)
     end
+
+    Book.transaction do |transaction|
+      expected_transaction = transaction
+      Book.count
+    end
+
+    assert asserted
+  ensure
+    ActiveSupport::Notifications.unsubscribe(subscriber)
   end
 end

activerecord/test/cases/query_cache_test.rb

@@ -1067,29 +1067,38 @@ def test_query_cache_lru_eviction
 
     assert_equal @connection_1, @connection_2
   end
+end
+
+class TransactionInCachedSqlActiveRecordPayloadTest < ActiveRecord::TestCase
+  # We need current_transaction to return the null transaction.
+  self.use_transactional_tests = false
 
-  test "payload with implicit transaction" do
-    expected_transaction = Task.current_transaction
+  def test_payload_without_open_transaction
+    asserted = false
 
     subscriber = ActiveSupport::Notifications.subscribe("sql.active_record") do |event|
       if event.payload[:cached]
-        assert_same expected_transaction, event.payload[:transaction]
+        assert_nil event.payload.fetch(:transaction)
+        asserted = true
       end
     end
-
     Task.cache do
       2.times { Task.count }
     end
+
+    assert asserted
   ensure
     ActiveSupport::Notifications.unsubscribe(subscriber)
   end
 
-  test "payload with explicit transaction" do
+  def test_payload_with_open_transaction
+    asserted = false
     expected_transaction = nil
 
     subscriber = ActiveSupport::Notifications.subscribe("sql.active_record") do |event|
       if event.payload[:cached]
         assert_same expected_transaction, event.payload[:transaction]
+        asserted = true
       end
     end
 
@@ -1100,6 +1109,8 @@ def test_query_cache_lru_eviction
         2.times { Task.count }
       end
     end
+
+    assert asserted
   ensure
     ActiveSupport::Notifications.unsubscribe(subscriber)
   end

guides/source/active_support_instrumentation.md

@@ -360,7 +360,7 @@ The `:cache_hits` key is only included if the collection is rendered with `cache
 | `:sql`               | SQL statement                            |
 | `:name`              | Name of the operation                    |
 | `:connection`        | Connection object                        |
-| `:transaction`       | Current transaction                      |
+| `:transaction`       | Current transaction, if any              |
 | `:binds`             | Bind parameters                          |
 | `:type_casted_binds` | Typecasted bind parameters               |
 | `:statement_name`    | SQL Statement name                       |
@@ -383,9 +383,7 @@ Adapters may add their own data as well.
 }
 ```
 
-If there is no transaction started at the moment, `:transaction` has a null
-object with UUID `00000000-0000-0000-0000-000000000000` (the nil UUID). This may
-happen, for example, issuing a `SELECT` not wrapped in a transaction.
+If the query is not executed in the context of a transaction, `:transaction` is `nil`.
 
 #### `strict_loading_violation.active_record`