Merge pull request #5100 from paukul/validate_on_condition_on_transaction_callbacks

Validate :on option on after_commit and after_rollback callbacks

activerecord/CHANGELOG.md

@@ -22,6 +22,11 @@
 
     *Rafael Mendonça França*
 
+*   after_commit and after_rollback now validate the :on option and raise an ArgumentError if
+    it is not one of :create, :destroy or :update
+
+    *Pascal Friederich*
+
 *   Keep index names when using `alter_table` with sqlite3.
     Fix #3489.
 

activerecord/lib/active_record/transactions.rb

@@ -4,6 +4,7 @@ module ActiveRecord
   # See ActiveRecord::Transactions::ClassMethods for documentation.
   module Transactions
     extend ActiveSupport::Concern
+    ACTIONS = [:create, :destroy, :update]
 
     class TransactionError < ActiveRecordError # :nodoc:
     end
@@ -224,11 +225,7 @@ def transaction(options = {}, &block)
       # Note that transactional fixtures do not play well with this feature. Please
       # use the +test_after_commit+ gem to have these hooks fired in tests.
       def after_commit(*args, &block)
-        options = args.last
-        if options.is_a?(Hash) && options[:on]
-          options[:if] = Array(options[:if])
-          options[:if] << "transaction_include_action?(:#{options[:on]})"
-        end
+        set_options_for_callbacks!(args)
         set_callback(:commit, :after, *args, &block)
       end
 
@@ -236,12 +233,25 @@ def after_commit(*args, &block)
       #
       # Please check the documentation of +after_commit+ for options.
       def after_rollback(*args, &block)
+        set_options_for_callbacks!(args)
+        set_callback(:rollback, :after, *args, &block)
+      end
+
+      private
+
+      def set_options_for_callbacks!(args)
         options = args.last
         if options.is_a?(Hash) && options[:on]
+          assert_valid_transaction_action(options[:on])
           options[:if] = Array(options[:if])
           options[:if] << "transaction_include_action?(:#{options[:on]})"
         end
-        set_callback(:rollback, :after, *args, &block)
+      end
+
+      def assert_valid_transaction_action(action)
+        unless ACTIONS.include?(action.to_sym)
+          raise ArgumentError, ":on conditions for after_commit and after_rollback callbacks have to be one of #{ACTIONS.join(",")}"
+        end
       end
     end
 

activerecord/test/cases/transaction_callbacks_test.rb

@@ -244,6 +244,14 @@ def @first.last_after_transaction_error; @last_transaction_error; end
     assert_equal :rollback, @first.last_after_transaction_error
     assert_equal [:after_rollback], @second.history
   end
+
+  def test_after_rollback_callbacks_should_validate_on_condition
+    assert_raise(ArgumentError) { Topic.send(:after_rollback, :on => :save) }
+  end
+
+  def test_after_commit_callbacks_should_validate_on_condition
+    assert_raise(ArgumentError) { Topic.send(:after_commit, :on => :save) }
+  end
 end