Merge pull request rails#46739 from adrianna-chang-shopify/ac-hide-callbacks-behaviour-change-behind-flag

Hide changes to `before_committed!` behaviour behind config

Author: eileencodes

activerecord/CHANGELOG.md

@@ -1,3 +1,12 @@
+*   Hide changes to before_committed! callback behaviour behind flag.
+
+    In #46525, behavior around before_committed! callbacks was changed so that callbacks
+    would run on every enrolled record in a transaction, not just the first copy of a record.
+    This change in behavior is now controlled by a configuration option,
+    `config.active_record.before_committed_on_all_records`. It will be enabled by default on Rails 7.1.
+
+    *Adrianna Chang*
+
 *   The `namespaced_controller` Query Log tag now matches the `controller` format
 
     For example, a request processed by `NameSpaced::UsersController` will now log as:

activerecord/lib/active_record.rb

@@ -274,6 +274,9 @@ def self.global_executor_concurrency # :nodoc:
   singleton_class.attr_accessor :belongs_to_required_validates_foreign_key
   self.belongs_to_required_validates_foreign_key = true
 
+  singleton_class.attr_accessor :before_committed_on_all_records
+  self.before_committed_on_all_records = false
+
   ##
   # :singleton-method:
   # Specify a threshold for the size of query result sets. If the number of

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

@@ -171,14 +171,18 @@ def before_commit_records
         return unless records
 
         if @run_commit_callbacks
-          ite = unique_records
+          if ActiveRecord.before_committed_on_all_records
+            ite = unique_records
 
-          instances_to_run_callbacks_on = records.each_with_object({}) do |record, candidates|
-            candidates[record] = record
-          end
+            instances_to_run_callbacks_on = records.each_with_object({}) do |record, candidates|
+              candidates[record] = record
+            end
 
-          run_action_on_records(ite, instances_to_run_callbacks_on) do |record, should_run_callbacks|
-            record.before_committed! if should_run_callbacks
+            run_action_on_records(ite, instances_to_run_callbacks_on) do |record, should_run_callbacks|
+              record.before_committed! if should_run_callbacks
+            end
+          else
+            records.uniq.each(&:before_committed!)
           end
         end
       end

activerecord/test/cases/touch_later_test.rb

@@ -123,7 +123,31 @@ def test_touching_three_deep
     assert_not_equal trees(:root).reload.updated_at, previous_tree_updated_at
   end
 
-  def test_touching_through_nested_attributes
+  def test_touching_through_nested_attributes_without_before_committed_on_all_records
+    original = ActiveRecord.before_committed_on_all_records
+    ActiveRecord.before_committed_on_all_records = false
+
+    time = Time.now.utc - 25.days
+
+    owner = owners(:blackbeard)
+
+    owner.touch(time: time)
+
+    assert_equal time.to_i, owner.reload.updated_at.to_i
+
+    owner.update pets_attributes: { "0" => { id: "1", name: "Alfred" } }
+
+    # The second copy of the record is not touched, so the owner's updated_at
+    # remains the same.
+    assert_equal time.to_i, owner.reload.updated_at.to_i
+  ensure
+    ActiveRecord.before_committed_on_all_records = original
+  end
+
+  def test_touching_through_nested_attributes_with_before_committed_on_all_records
+    original = ActiveRecord.before_committed_on_all_records
+    ActiveRecord.before_committed_on_all_records = true
+
     time = Time.now.utc - 25.days
 
     owner = owners(:blackbeard)
@@ -135,5 +159,7 @@ def test_touching_through_nested_attributes
     owner.update pets_attributes: { "0" => { id: "1", name: "Alfred" } }
 
     assert_not_equal time.to_i, owner.reload.updated_at.to_i
+  ensure
+    ActiveRecord.before_committed_on_all_records = original
   end
 end

activerecord/test/storage/test.sqlite3

@@ -64,6 +64,7 @@ Below are the default values associated with each target version. In cases of co
 - [`config.action_dispatch.default_headers`](#config-action-dispatch-default-headers): `{ "X-Frame-Options" => "SAMEORIGIN", "X-XSS-Protection" => "0", "X-Content-Type-Options" => "nosniff", "X-Permitted-Cross-Domain-Policies" => "none", "Referrer-Policy" => "strict-origin-when-cross-origin" }`
 - [`config.active_job.use_big_decimal_serializer`](#config-active-job-use-big-decimal-serializer): `true`
 - [`config.active_record.allow_deprecated_singular_associations_name`](#config-active-record-allow-deprecated-singular-associations-name): `false`
+- [`config.active_record.before_committed_on_all_records`](#config-active-record-before-committed-on-all-records): `true`
 - [`config.active_record.belongs_to_required_validates_foreign_key`](#config-active-record-belongs-to-required-validates-foreign-key): `false`
 - [`config.active_record.query_log_tags_format`](#config-active-record-query-log-tags-format): `:sqlcommenter`
 - [`config.active_record.raise_on_assign_to_attr_readonly`](#config-active-record-raise-on-assign-to-attr-readonly): `true`
@@ -1006,6 +1007,17 @@ The options are `:schema_search_path` (the default) which dumps any schemas list
 `:all` which always dumps all schemas regardless of the `schema_search_path`,
 or a string of comma separated schemas.
 
+#### `config.active_record.before_committed_on_all_records`
+
+Enable before_committed! callbacks on all enrolled records in a transaction.
+The previous behavior was to only run the callbacks on the first copy of a record
+if there were multiple copies of the same record enrolled in the transaction.
+
+| Starting with version | The default value is |
+| --------------------- | -------------------- |
+| (original)            | `false`              |
+| 7.1                   | `true`               |
+
 #### `config.active_record.belongs_to_required_by_default`
 
 Is a boolean value and controls whether a record fails validation if

guides/source/configuring.md

@@ -291,6 +291,7 @@ def load_defaults(target_version)
             active_record.query_log_tags_format = :sqlcommenter
             active_record.raise_on_assign_to_attr_readonly = true
             active_record.belongs_to_required_validates_foreign_key = false
+            active_record.before_committed_on_all_records = true
           end
 
           if respond_to?(:action_dispatch)

railties/lib/rails/application/configuration.rb

@@ -121,3 +121,8 @@
 # Enable precompilation of `config.filter_parameters`. Precompilation can
 # improve filtering performance, depending on the quantity and types of filters.
 # Rails.application.config.precompile_filter_parameters = true
+
+# Enable before_committed! callbacks on all enrolled records in a transaction.
+# The previous behavior was to only run the callbacks on the first copy of a record
+# if there were multiple copies of the same record enrolled in the transaction.
+# Rails.application.config.active_record.before_committed_on_all_records = true