diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index 272cef6fcf4c43fd1d997a39ffc251bf1ec621a6..e080da9c06f8ac08b5da2af7908bcd684315f4c4 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 ce6998530fcabdbcfadff1bbb936012fd0a4551d..4a608e4f7b1bc75245975ff20f928d7643bc44f8 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 @@ 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 diff --git a/activerecord/test/cases/transaction_callbacks_test.rb b/activerecord/test/cases/transaction_callbacks_test.rb index 2ddc449c127ca6c77b254a3102f831a3e9ffa7a3..869892e33fa20bd8575d031f2279023b7db86f62 100644 --- a/activerecord/test/cases/transaction_callbacks_test.rb +++ b/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