diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index 0b59668100..5a693b5bf6 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,3 +1,34 @@ +* Run transactional callbacks on the freshest instance to save a given + record within a transaction. + + When multiple Active Record instances change the same record within a + transaction, Rails runs `after_commit` or `after_rollback` callbacks for + only one of them. `config.active_record.run_commit_callbacks_on_first_saved_instances_in_transaction` + was added to specify how Rails chooses which instance receives the + callbacks. The framework defaults were changed to use the new logic. + + When `config.active_record.run_commit_callbacks_on_first_saved_instances_in_transaction` + is `true`, transactional callbacks are run on the first instance to save, + even though its instance state may be stale. + + When it is `false`, which is the new framework default starting with version + 7.1, transactional callbacks are run on the instances with the freshest + instance state. Those instances are chosen as follows: + + - In general, run transactional callbacks on the last instance to save a + given record within the transaction. + - There are two exceptions: + - If the record is created within the transaction, then updated by + another instance, `after_create_commit` callbacks will be run on the + second instance. This is instead of the `after_update_commit` + callbacks that would naively be run based on that instance’s state. + - If the record is destroyed within the transaction, then + `after_destroy_commit` callbacks will be fired on the last destroyed + instance, even if a stale instance subsequently performed an update + (which will have affected 0 rows). + + *Cameron Bothner and Mitch Vollebregt* + * Resolve issue where a relation cache_version could be left stale. Previously, when `reset` was called on a relation object it did not reset the cache_versions diff --git a/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb b/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb index d703a14c54..834dad7d4d 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb @@ -147,12 +147,12 @@ module ActiveRecord def rollback_records return unless records + ite = records.uniq(&:__id__) - already_run_callbacks = {} + instances_to_run_callbacks_on = prepare_instances_to_run_callbacks_on(ite) + while record = ite.shift - trigger_callbacks = record.trigger_transactional_callbacks? - should_run_callbacks = !already_run_callbacks[record] && trigger_callbacks - already_run_callbacks[record] ||= trigger_callbacks + should_run_callbacks = record.__id__ == instances_to_run_callbacks_on[record].__id__ record.rolledback!(force_restore_state: full_rollback?, should_run_callbacks: should_run_callbacks) end ensure @@ -167,15 +167,18 @@ module ActiveRecord def commit_records return unless records + ite = records.uniq(&:__id__) - already_run_callbacks = {} - while record = ite.shift - if @run_commit_callbacks - trigger_callbacks = record.trigger_transactional_callbacks? - should_run_callbacks = !already_run_callbacks[record] && trigger_callbacks - already_run_callbacks[record] ||= trigger_callbacks + + if @run_commit_callbacks + instances_to_run_callbacks_on = prepare_instances_to_run_callbacks_on(ite) + + while record = ite.shift + should_run_callbacks = record.__id__ == instances_to_run_callbacks_on[record].__id__ record.committed!(should_run_callbacks: should_run_callbacks) - else + end + else + while record = ite.shift # if not running callbacks, only adds the record to the parent transaction connection.add_transaction_record(record) end @@ -188,6 +191,33 @@ module ActiveRecord def joinable?; @joinable; end def closed?; false; end def open?; !closed?; end + + private + def prepare_instances_to_run_callbacks_on(records) + records.each_with_object({}) do |record, candidates| + next unless record.trigger_transactional_callbacks? + + earlier_saved_candidate = candidates[record] + + next if earlier_saved_candidate && record.class.run_commit_callbacks_on_first_saved_instances_in_transaction + + # If the candidate instance destroyed itself in the database, then + # instances which were added to the transaction afterwards, and which + # think they updated themselves, are wrong. They should not replace + # our candidate as an instance to run callbacks on + next if earlier_saved_candidate&.destroyed? && !record.destroyed? + + # If the candidate instance was created inside of this transaction, + # then instances which were subsequently loaded from the database + # and updated need that state transferred to them so that + # the after_create_commit callbacks are run + record._new_record_before_last_commit = true if earlier_saved_candidate&._new_record_before_last_commit + + # The last instance to save itself is likeliest to have internal + # state that matches what's committed to the database + candidates[record] = record + end + end end class RestartParentTransaction < Transaction diff --git a/activerecord/lib/active_record/core.rb b/activerecord/lib/active_record/core.rb index 28ea49b914..0a1a6af63a 100644 --- a/activerecord/lib/active_record/core.rb +++ b/activerecord/lib/active_record/core.rb @@ -81,6 +81,8 @@ module ActiveRecord class_attribute :has_many_inversing, instance_accessor: false, default: false + class_attribute :run_commit_callbacks_on_first_saved_instances_in_transaction, instance_accessor: false, default: true + class_attribute :default_connection_handler, instance_writer: false class_attribute :default_role, instance_writer: false diff --git a/activerecord/lib/active_record/transactions.rb b/activerecord/lib/active_record/transactions.rb index aa68d1e7ac..6281485fea 100644 --- a/activerecord/lib/active_record/transactions.rb +++ b/activerecord/lib/active_record/transactions.rb @@ -13,6 +13,8 @@ module ActiveRecord scope: [:kind, :name] end + attr_accessor :_new_record_before_last_commit # :nodoc: + # = Active Record Transactions # # \Transactions are protective blocks where SQL statements are only permanent diff --git a/activerecord/test/cases/transaction_callbacks_test.rb b/activerecord/test/cases/transaction_callbacks_test.rb index a1dfa6f3f3..31687a9174 100644 --- a/activerecord/test/cases/transaction_callbacks_test.rb +++ b/activerecord/test/cases/transaction_callbacks_test.rb @@ -770,3 +770,143 @@ class CallbacksOnActionAndConditionTest < ActiveRecord::TestCase assert_equal [], topic.history end end + +class CallbacksOnMultipleInstancesInATransactionTest < ActiveRecord::TestCase + class TopicWithTitleHistory < ActiveRecord::Base + self.table_name = :topics + + def self.clear_history + @@history = [] + end + + def self.history + @@history ||= [] + end + + after_create_commit { |record| record.class.history << "Created (title = #{record.title.inspect})" } + after_update_commit { |record| record.class.history << "Updated (title = #{record.title.inspect})" } + after_destroy_commit { |record| record.class.history << "Destroyed (title = #{record.title.inspect})" } + end + + def test_created_callback_called_on_last_to_save_of_separate_instances_in_a_transaction + with_run_commit_callbacks_on_first_saved_instances_in_transaction(false) do + TopicWithTitleHistory.clear_history + + TopicWithTitleHistory.transaction do + topic = TopicWithTitleHistory.create!(title: "A") + TopicWithTitleHistory.find(topic.id).update!(title: "B") + end + + assert_equal ['Created (title = "B")'], TopicWithTitleHistory.history + end + end + + def test_created_callback_called_on_first_to_save_in_transaction_with_old_configuration + with_run_commit_callbacks_on_first_saved_instances_in_transaction(true) do + TopicWithTitleHistory.clear_history + + TopicWithTitleHistory.transaction do + topic = TopicWithTitleHistory.create!(title: "A") + TopicWithTitleHistory.find(topic.id).update!(title: "B") + end + + assert_equal ['Created (title = "A")'], TopicWithTitleHistory.history + end + end + + def test_updated_callback_called_on_last_to_save_of_separate_instances_in_a_transaction + with_run_commit_callbacks_on_first_saved_instances_in_transaction(false) do + topic = TopicWithTitleHistory.create!(title: "one") + TopicWithTitleHistory.clear_history + + TopicWithTitleHistory.transaction do + TopicWithTitleHistory.find(topic.id).update!(title: "two") + TopicWithTitleHistory.find(topic.id).update!(title: "three") + end + + assert_equal ['Updated (title = "three")'], TopicWithTitleHistory.history + end + end + + def test_updated_callback_called_on_first_to_save_in_transaction_with_old_configuration + with_run_commit_callbacks_on_first_saved_instances_in_transaction(true) do + topic = TopicWithTitleHistory.create!(title: "one") + TopicWithTitleHistory.clear_history + + TopicWithTitleHistory.transaction do + TopicWithTitleHistory.find(topic.id).update!(title: "two") + TopicWithTitleHistory.find(topic.id).update!(title: "three") + end + + assert_equal ['Updated (title = "two")'], TopicWithTitleHistory.history + end + end + + def test_destroyed_callback_called_on_destroyed_instance_when_preceded_in_transaction_by_save_from_separate_instance + with_run_commit_callbacks_on_first_saved_instances_in_transaction(false) do + topic = TopicWithTitleHistory.create!(title: "one") + TopicWithTitleHistory.clear_history + + TopicWithTitleHistory.transaction do + TopicWithTitleHistory.find(topic.id).update!(title: "two") + TopicWithTitleHistory.find(topic.id).destroy! + end + + assert_equal ['Destroyed (title = "two")'], TopicWithTitleHistory.history + end + end + + def test_updated_callback_called_on_first_to_save_when_followed_in_transaction_by_destroy_from_separate_instance_with_old_configuration + with_run_commit_callbacks_on_first_saved_instances_in_transaction(true) do + topic = TopicWithTitleHistory.create!(title: "one") + TopicWithTitleHistory.clear_history + + TopicWithTitleHistory.transaction do + TopicWithTitleHistory.find(topic.id).update!(title: "two") + TopicWithTitleHistory.find(topic.id).destroy! + end + + assert_equal ['Updated (title = "two")'], TopicWithTitleHistory.history + end + end + + def test_destroyed_callbacks_called_on_destroyed_instance_even_when_followed_by_update_from_separate_instances_in_a_transaction + with_run_commit_callbacks_on_first_saved_instances_in_transaction(false) do + topic = TopicWithTitleHistory.create!(title: "one") + TopicWithTitleHistory.clear_history + + TopicWithTitleHistory.transaction do + TopicWithTitleHistory.find(topic.id).destroy! + topic.update!(title: "two") + end + + assert_equal ['Destroyed (title = "one")'], TopicWithTitleHistory.history + end + end + + def test_destroyed_callbacks_called_on_first_saved_instance_in_transaction_with_old_configuration + with_run_commit_callbacks_on_first_saved_instances_in_transaction(true) do + topic = TopicWithTitleHistory.create!(title: "one") + TopicWithTitleHistory.clear_history + + TopicWithTitleHistory.transaction do + TopicWithTitleHistory.find(topic.id).destroy! + topic.update!(title: "two") + end + + assert_equal ['Destroyed (title = "one")'], TopicWithTitleHistory.history + end + end + + def with_run_commit_callbacks_on_first_saved_instances_in_transaction(value, model: ActiveRecord::Base) + old = model.run_commit_callbacks_on_first_saved_instances_in_transaction + model.run_commit_callbacks_on_first_saved_instances_in_transaction = value + yield + ensure + model.run_commit_callbacks_on_first_saved_instances_in_transaction = old + if model != ActiveRecord::Base && !old + # reset the class_attribute + model.singleton_class.remove_method(:run_commit_callbacks_on_first_saved_instances_in_transaction) + end + end +end diff --git a/guides/source/configuring.md b/guides/source/configuring.md index d099d15219..ae74f0b8c8 100644 --- a/guides/source/configuring.md +++ b/guides/source/configuring.md @@ -937,6 +937,26 @@ The default value depends on the `config.load_defaults` target version: | (original) | `false` | | 7.0 | `true` | +#### `config.active_record.run_commit_callbacks_on_first_saved_instances_in_transaction` + +When multiple Active Record instances change the same record within a transaction, Rails runs `after_commit` or `after_rollback` callbacks for only one of them. This option specifies how Rails chooses which instance receives the callbacks. + +When `true`, transactional callbacks are run on the first instance to save, even though its instance state may be stale. + +When `false`, transactional callbacks are run on the instances with the freshest instance state. Those instances are chosen as follows: + +- In general, run transactional callbacks on the last instance to save a given record within the transaction. +- There are two exceptions: + - If the record is created within the transaction, then updated by another instance, `after_create_commit` callbacks will be run on the second instance. This is instead of the `after_update_commit` callbacks that would naively be run based on that instance’s state. + - If the record is destroyed within the transaction, then `after_destroy_commit` callbacks will be fired on the last destroyed instance, even if a stale instance subsequently performed an update (which will have affected 0 rows). + +The default value depends on the `config.load_defaults` target version: + +| Starting with version | The default value is | +| --------------------- | -------------------- | +| (original) | `true` | +| 7.1 | `false` | + #### `config.active_record.query_log_tags_enabled` Specifies whether or not to enable adapter-level query comments. Defaults to diff --git a/railties/lib/rails/application/configuration.rb b/railties/lib/rails/application/configuration.rb index 3a3f1f73fe..6df81c1b35 100644 --- a/railties/lib/rails/application/configuration.rb +++ b/railties/lib/rails/application/configuration.rb @@ -262,6 +262,10 @@ module Rails self.log_file_size = (100 * 1024 * 1024) end + if respond_to?(:active_record) + active_record.run_commit_callbacks_on_first_saved_instances_in_transaction = false + end + if respond_to?(:action_dispatch) action_dispatch.default_headers = { "X-Frame-Options" => "SAMEORIGIN", diff --git a/railties/lib/rails/generators/rails/app/templates/config/initializers/new_framework_defaults_7_1.rb.tt b/railties/lib/rails/generators/rails/app/templates/config/initializers/new_framework_defaults_7_1.rb.tt index 92a02cc1b7..d033659b55 100644 --- a/railties/lib/rails/generators/rails/app/templates/config/initializers/new_framework_defaults_7_1.rb.tt +++ b/railties/lib/rails/generators/rails/app/templates/config/initializers/new_framework_defaults_7_1.rb.tt @@ -28,3 +28,10 @@ # Do not treat an `ActionController::Parameters` instance # as equal to an equivalent `Hash` by default. # Rails.application.config.action_controller.allow_deprecated_parameters_hash_equality = false + +# No longer run after_commit callbacks on the first of multiple Active Record +# instances to save changes to the same database row within a transaction. +# Instead, run these callbacks on the instance most likely to have internal +# state which matches what was committed to the database, typically the last +# instance to save. +# Rails.application.config.active_record.run_commit_callbacks_on_first_saved_instances_in_transaction = false diff --git a/railties/test/application/configuration_test.rb b/railties/test/application/configuration_test.rb index 008dc93e5e..5861ac19ee 100644 --- a/railties/test/application/configuration_test.rb +++ b/railties/test/application/configuration_test.rb @@ -2382,6 +2382,32 @@ module ApplicationTests assert_equal true, ActiveRecord.verify_foreign_keys_for_fixtures end + test "ActiveRecord::Base.run_commit_callbacks_on_first_saved_instances_in_transaction is false by default for new apps" do + app "development" + + assert_equal false, ActiveRecord::Base.run_commit_callbacks_on_first_saved_instances_in_transaction + end + + test "ActiveRecord::Base.run_commit_callbacks_on_first_saved_instances_in_transaction is true by default for upgraded apps" do + remove_from_config '.*config\.load_defaults.*\n' + + app "development" + + assert_equal true, ActiveRecord::Base.run_commit_callbacks_on_first_saved_instances_in_transaction + end + + test "ActiveRecord::Base.run_commit_callbacks_on_first_saved_instances_in_transaction can be configured via config.active_record.run_commit_callbacks_on_first_saved_instances_in_transaction" do + remove_from_config '.*config\.load_defaults.*\n' + + app_file "config/initializers/new_framework_defaults_7_0.rb", <<-RUBY + Rails.application.config.active_record.run_commit_callbacks_on_first_saved_instances_in_transaction = false + RUBY + + app "development" + + assert_equal false, ActiveRecord::Base.run_commit_callbacks_on_first_saved_instances_in_transaction + end + test "ActiveSupport::MessageEncryptor.use_authenticated_message_encryption is true by default for new apps" do app "development"