Run after_commit callbacks defined on models in the correct order

Author: ghiculescu

activerecord/CHANGELOG.md

@@ -1,3 +1,22 @@
+*   `after_commit` callbacks defined on models now execute in the correct order.
+
+    ```ruby
+    class User < ActiveRecord::Base
+      after_commit { puts("this gets called first") }
+      after_commit { puts("this gets called second") }
+    end
+    ```
+
+    Previously, the callbacks executed in the reverse order. To opt in to the new behaviour:
+
+    ```ruby
+    config.active_record.run_after_transaction_callbacks_in_order_defined = true
+    ```
+
+    This is the default for new apps.
+
+    *Alex Ghiculescu*
+
 *   Infer `foreign_key` when `inverse_of` is present on `has_one` and `has_many` associations.
 
     ```ruby

activerecord/lib/active_record.rb

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

activerecord/lib/active_record/callbacks.rb

@@ -234,30 +234,16 @@ module ActiveRecord
   #   end
   #
   # In this case the +log_children+ is executed before +do_something_else+.
-  # The same applies to all non-transactional callbacks.
+  # This applies to all non-transactional callbacks, and to +before_commit+.
   #
-  # As seen below, in case there are multiple transactional callbacks the order
-  # is reversed.
+  # For transactional +after_+ callbacks (+after_commit+, +after_rollback+, etc), the order
+  # can be set via configuration.
   #
-  # For example:
-  #
-  #   class Topic < ActiveRecord::Base
-  #     has_many :children
-  #
-  #     after_commit :log_children
-  #     after_commit :do_something_else
-  #
-  #     private
-  #       def log_children
-  #         # Child processing
-  #       end
-  #
-  #       def do_something_else
-  #         # Something else
-  #       end
-  #   end
+  #   config.active_record.run_after_transaction_callbacks_in_order_defined = false
   #
-  # In this case the +do_something_else+ is executed before +log_children+.
+  # If +true+ (the default from Rails 7.1), callbacks are executed in the order they
+  # are defined, just like the example above. If +false+, the order is reversed, so
+  # +do_something_else+ is executed before +log_children+.
   #
   # == \Transactions
   #

activerecord/lib/active_record/transactions.rb

@@ -230,43 +230,51 @@ def before_commit(*args, &block) # :nodoc:
       #   after_commit :do_bar_baz, on: [:update, :destroy]
       #
       def after_commit(*args, &block)
-        set_options_for_callbacks!(args)
+        set_options_for_callbacks!(args, prepend_option)
         set_callback(:commit, :after, *args, &block)
       end
 
       # Shortcut for <tt>after_commit :hook, on: [ :create, :update ]</tt>.
       def after_save_commit(*args, &block)
-        set_options_for_callbacks!(args, on: [ :create, :update ])
+        set_options_for_callbacks!(args, on: [ :create, :update ], **prepend_option)
         set_callback(:commit, :after, *args, &block)
       end
 
       # Shortcut for <tt>after_commit :hook, on: :create</tt>.
       def after_create_commit(*args, &block)
-        set_options_for_callbacks!(args, on: :create)
+        set_options_for_callbacks!(args, on: :create, **prepend_option)
         set_callback(:commit, :after, *args, &block)
       end
 
       # Shortcut for <tt>after_commit :hook, on: :update</tt>.
       def after_update_commit(*args, &block)
-        set_options_for_callbacks!(args, on: :update)
+        set_options_for_callbacks!(args, on: :update, **prepend_option)
         set_callback(:commit, :after, *args, &block)
       end
 
       # Shortcut for <tt>after_commit :hook, on: :destroy</tt>.
       def after_destroy_commit(*args, &block)
-        set_options_for_callbacks!(args, on: :destroy)
+        set_options_for_callbacks!(args, on: :destroy, **prepend_option)
         set_callback(:commit, :after, *args, &block)
       end
 
       # This callback is called after a create, update, or destroy are rolled back.
       #
       # Please check the documentation of #after_commit for options.
       def after_rollback(*args, &block)
-        set_options_for_callbacks!(args)
+        set_options_for_callbacks!(args, prepend_option)
         set_callback(:rollback, :after, *args, &block)
       end
 
       private
+        def prepend_option
+          if ActiveRecord.run_after_transaction_callbacks_in_order_defined
+            { prepend: true }
+          else
+            {}
+          end
+        end
+
         def set_options_for_callbacks!(args, enforced_options = {})
           options = args.extract_options!.merge!(enforced_options)
           args << options

activerecord/test/cases/transaction_callbacks_test.rb

@@ -640,6 +640,99 @@ def test_before_commit_update_in_same_transaction
   end
 end
 
+class CallbackOrderTest < ActiveRecord::TestCase
+  self.use_transactional_tests = false
+
+  module Behaviour
+    extend ActiveSupport::Concern
+
+    included do
+      self.table_name = :topics
+
+      after_commit { |record| record.history << 3 }
+      after_commit { |record| record.history << 4 }
+      after_save_commit { |record| record.history << "save" }
+      after_create_commit { |record| record.history << "create" }
+      after_update_commit { |record| record.history << "update" }
+      after_destroy_commit { |record| record.history << "destroy" }
+
+      after_rollback { |record| record.history << "rollback1" }
+      after_rollback { |record| record.history << "rollback2" }
+
+      before_commit { |record| record.history << 1 }
+      before_commit { |record| record.history << 2 }
+    end
+
+    def clear_history
+      @history = []
+    end
+
+    def history
+      @history ||= []
+    end
+  end
+
+  run_after_transaction_callbacks_in_order_defined_was = ActiveRecord.run_after_transaction_callbacks_in_order_defined
+  ActiveRecord.run_after_transaction_callbacks_in_order_defined = true
+  class TopicWithCallbacksWithSpecificOrderWithSettingTrue < ActiveRecord::Base
+    include Behaviour
+  end
+  ActiveRecord.run_after_transaction_callbacks_in_order_defined = run_after_transaction_callbacks_in_order_defined_was
+
+  run_after_transaction_callbacks_in_order_defined_was = ActiveRecord.run_after_transaction_callbacks_in_order_defined
+  ActiveRecord.run_after_transaction_callbacks_in_order_defined = false
+  class TopicWithCallbacksWithSpecificOrderWithSettingFalse < ActiveRecord::Base
+    include Behaviour
+  end
+  ActiveRecord.run_after_transaction_callbacks_in_order_defined = run_after_transaction_callbacks_in_order_defined_was
+
+  def test_callbacks_run_in_order_defined_in_model_if_using_run_after_transaction_callbacks_in_order_defined
+    topic = TopicWithCallbacksWithSpecificOrderWithSettingTrue.new
+    topic.save
+    assert_equal [1, 2, 3, 4, "save", "create"], topic.history
+
+    topic.clear_history
+    topic.approved = true
+    topic.save
+    assert_equal [1, 2, 3, 4, "save", "update"], topic.history
+
+    topic.clear_history
+    topic.transaction do
+      topic.approved = false
+      topic.save
+      raise ActiveRecord::Rollback
+    end
+    assert_equal ["rollback1", "rollback2"], topic.history
+
+    topic.clear_history
+    topic.destroy
+    assert_equal [1, 2, 3, 4, "destroy"], topic.history
+  end
+
+  def test_callbacks_run_in_order_defined_in_model_if_not_using_run_after_transaction_callbacks_in_order_defined
+    topic = TopicWithCallbacksWithSpecificOrderWithSettingFalse.new
+    topic.save
+    assert_equal [1, 2, "create", "save", 4, 3], topic.history
+
+    topic.clear_history
+    topic.approved = true
+    topic.save
+    assert_equal [1, 2, "update", "save", 4, 3], topic.history
+
+    topic.clear_history
+    topic.transaction do
+      topic.approved = false
+      topic.save
+      raise ActiveRecord::Rollback
+    end
+    assert_equal ["rollback2", "rollback1"], topic.history
+
+    topic.clear_history
+    topic.destroy
+    assert_equal [1, 2, "destroy", 4, 3], topic.history
+  end
+end
+
 class CallbacksOnDestroyUpdateActionRaceTest < ActiveRecord::TestCase
   self.use_transactional_tests = false
 

guides/source/configuring.md

@@ -71,6 +71,7 @@ Below are the default values associated with each target version. In cases of co
 - [`config.active_record.marshalling_format_version`](#config-active-record-marshalling-format-version): `7.1`
 - [`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`
+- [`config.active_record.run_after_transaction_callbacks_in_order_defined`](#config-active-record-run-after-transaction-callbacks-in-order-defined): `true`
 - [`config.active_record.run_commit_callbacks_on_first_saved_instances_in_transaction`](#config-active-record-run-commit-callbacks-on-first-saved-instances-in-transaction): `false`
 - [`config.active_record.sqlite3_adapter_strict_strings_by_default`](#config-active-record-sqlite3-adapter-strict-strings-by-default): `true`
 - [`config.active_support.default_message_encryptor_serializer`](#config-active-support-default-message-encryptor-serializer): `:json`
@@ -1303,6 +1304,19 @@ The default value depends on the `config.load_defaults` target version:
 | (original)            | `YAML`               |
 | 7.1                   | `nil`                |
 
+#### `config.active_record.run_after_transaction_callbacks_in_order_defined`
+
+If true, `after_commit` callbacks are executed in the order they are defined in a model. If false, they are executed in reverse order.
+
+All other callbacks are always executed in the order they are defined in a model (unless you use `prepend: true`).
+
+The default value depends on the `config.load_defaults` target version:
+
+| Starting with version | The default value is |
+| --------------------- | -------------------- |
+| (original)            | `false`              |
+| 7.1                   | `true`               |
+
 #### `config.active_record.query_log_tags_enabled`
 
 Specifies whether or not to enable adapter-level query comments. Defaults to

railties/lib/rails/application/configuration.rb

@@ -284,6 +284,7 @@ def load_defaults(target_version)
             active_record.default_column_serializer = nil
             active_record.encryption.hash_digest_class = OpenSSL::Digest::SHA256
             active_record.marshalling_format_version = 7.1
+            active_record.run_after_transaction_callbacks_in_order_defined = true
           end
 
           if respond_to?(:action_dispatch)

railties/lib/rails/generators/rails/app/templates/config/initializers/new_framework_defaults_7_1.rb.tt

@@ -161,3 +161,8 @@
 # leave this optimization off on the first deploy, then enable it on a
 # subsequent deploy.
 # Rails.application.config.active_record.marshalling_format_version = 7.1
+
+# Run `after_commit` and `after_*_commit` callbacks in the order they are defined in a model.
+# This matches the behaviour of all other callbacks.
+# In previous versions of Rails, they ran in the inverse order.
+# Rails.application.config.active_record.run_after_transaction_callbacks_in_order_defined = true

railties/test/application/configuration_test.rb

@@ -4346,6 +4346,31 @@ def new(app); self; end
       end
     end
 
+    test "run_after_transaction_callbacks_in_order_defined is true in new apps" do
+      app "development"
+
+      assert_equal true, ActiveRecord.run_after_transaction_callbacks_in_order_defined
+    end
+
+    test "run_after_transaction_callbacks_in_order_defined is false in upgrading apps" do
+      remove_from_config '.*config\.load_defaults.*\n'
+      add_to_config 'config.load_defaults "7.0"'
+      app "development"
+
+      assert_equal false, ActiveRecord.run_after_transaction_callbacks_in_order_defined
+    end
+
+    test "run_after_transaction_callbacks_in_order_defined can be set via framework defaults" do
+      remove_from_config '.*config\.load_defaults.*\n'
+      add_to_config 'config.load_defaults "7.0"'
+      app_file "config/initializers/new_framework_defaults_7_1.rb", <<-RUBY
+        Rails.application.config.active_record.run_after_transaction_callbacks_in_order_defined = true
+      RUBY
+      app "development"
+
+      assert_equal true, ActiveRecord.run_after_transaction_callbacks_in_order_defined
+    end
+
     private
       def set_custom_config(contents, config_source = "custom".inspect)
         app_file "config/custom.yml", contents