From 5a3dc8092d19c816b0b1203945639cb91d065847 Mon Sep 17 00:00:00 2001
From: Pascal Friederich <paukul@gmail.com>
Date: Tue, 25 Dec 2012 19:35:52 +0100
Subject: validate :on option on after_commit and after_rollback callbacks

---
 activerecord/CHANGELOG.md                          |  5 +++++
 activerecord/lib/active_record/transactions.rb     | 22 ++++++++++++++++------
 .../test/cases/transaction_callbacks_test.rb       |  8 ++++++++
 3 files changed, 29 insertions(+), 6 deletions(-)

(limited to 'activerecord')

diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
index 272cef6fcf..e080da9c06 100644
--- a/activerecord/CHANGELOG.md
+++ b/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.
 
diff --git a/activerecord/lib/active_record/transactions.rb b/activerecord/lib/active_record/transactions.rb
index ce6998530f..4a608e4f7b 100644
--- a/activerecord/lib/active_record/transactions.rb
+++ b/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 @@ module ActiveRecord
       # 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 @@ module ActiveRecord
       #
       # 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
 
diff --git a/activerecord/test/cases/transaction_callbacks_test.rb b/activerecord/test/cases/transaction_callbacks_test.rb
index 2ddc449c12..869892e33f 100644
--- a/activerecord/test/cases/transaction_callbacks_test.rb
+++ b/activerecord/test/cases/transaction_callbacks_test.rb
@@ -244,6 +244,14 @@ class TransactionCallbacksTest < ActiveRecord::TestCase
     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
 
 
-- 
cgit v1.2.3