From 2386daabe7f8c979b453010dc0de3e1f6bbf859d Mon Sep 17 00:00:00 2001 From: claudiob Date: Thu, 16 Oct 2014 16:21:24 -0700 Subject: [PATCH] Throw :abort halts default CallbackChains This commit changes arguments and default value of CallbackChain's :terminator option. After this commit, Chains of callbacks defined **without** an explicit `:terminator` option will be halted as soon as a `before_` callback throws `:abort`. Chains of callbacks defined **with** a `:terminator` option will maintain their existing behavior of halting as soon as a `before_` callback matches the terminator's expectation. For instance, ActiveModel's callbacks will still halt the chain when a `before_` callback returns `false`. --- .../lib/abstract_controller/callbacks.rb | 2 +- activemodel/lib/active_model/callbacks.rb | 2 +- .../lib/active_model/validations/callbacks.rb | 2 +- .../lib/active_record/transactions.rb | 2 +- activesupport/CHANGELOG.md | 12 ++++++ activesupport/lib/active_support/callbacks.rb | 23 ++++++++--- activesupport/test/callbacks_test.rb | 39 ++++++++++++++++--- 7 files changed, 68 insertions(+), 14 deletions(-) diff --git a/actionpack/lib/abstract_controller/callbacks.rb b/actionpack/lib/abstract_controller/callbacks.rb index ca5c80cd71..60604db447 100644 --- a/actionpack/lib/abstract_controller/callbacks.rb +++ b/actionpack/lib/abstract_controller/callbacks.rb @@ -9,7 +9,7 @@ module Callbacks included do define_callbacks :process_action, - terminator: ->(controller,_) { controller.response_body }, + terminator: ->(controller, result_lambda) { result_lambda.call if result_lambda.is_a?(Proc); controller.response_body }, skip_after_callbacks_if_terminated: true end diff --git a/activemodel/lib/active_model/callbacks.rb b/activemodel/lib/active_model/callbacks.rb index b3d70dc515..061d96a80b 100644 --- a/activemodel/lib/active_model/callbacks.rb +++ b/activemodel/lib/active_model/callbacks.rb @@ -103,7 +103,7 @@ def self.extended(base) #:nodoc: def define_model_callbacks(*callbacks) options = callbacks.extract_options! options = { - terminator: ->(_,result) { result == false }, + terminator: ->(_,result_lambda) { result_lambda.call == false }, skip_after_callbacks_if_terminated: true, scope: [:kind, :name], only: [:before, :around, :after] diff --git a/activemodel/lib/active_model/validations/callbacks.rb b/activemodel/lib/active_model/validations/callbacks.rb index 25ccabd66b..449f1a0299 100644 --- a/activemodel/lib/active_model/validations/callbacks.rb +++ b/activemodel/lib/active_model/validations/callbacks.rb @@ -23,7 +23,7 @@ module Callbacks included do include ActiveSupport::Callbacks define_callbacks :validation, - terminator: ->(_,result) { result == false }, + terminator: ->(_,result_lambda) { result_lambda.call == false }, skip_after_callbacks_if_terminated: true, scope: [:kind, :name] end diff --git a/activerecord/lib/active_record/transactions.rb b/activerecord/lib/active_record/transactions.rb index de701edca0..95e9a8646e 100644 --- a/activerecord/lib/active_record/transactions.rb +++ b/activerecord/lib/active_record/transactions.rb @@ -17,7 +17,7 @@ module Transactions included do define_callbacks :commit, :rollback, - terminator: ->(_, result) { result == false }, + terminator: ->(_, result_lambda) { result_lambda.call == false }, scope: [:kind, :name] mattr_accessor :raise_in_transactional_callbacks, instance_writer: false diff --git a/activesupport/CHANGELOG.md b/activesupport/CHANGELOG.md index 3606d7e572..01bb816923 100644 --- a/activesupport/CHANGELOG.md +++ b/activesupport/CHANGELOG.md @@ -1,3 +1,15 @@ +* Changes arguments and default value of CallbackChain's :terminator option + + Chains of callbacks defined without an explicit `:terminator` option will + now be halted as soon as a `before_` callback throws `:abort`. + + Chains of callbacks defined with a `:terminator` option will maintain their + existing behavior of halting as soon as a `before_` callback matches the + terminator's expectation. For instance, ActiveModel's callbacks will still + halt the chain when a `before_` callback returns `false`. + + *claudiob* + * Deprecate `MissingSourceFile` in favor of `LoadError`. `MissingSourceFile` was just an alias to `LoadError` and was not being diff --git a/activesupport/lib/active_support/callbacks.rb b/activesupport/lib/active_support/callbacks.rb index d2911a254c..3a3061e536 100644 --- a/activesupport/lib/active_support/callbacks.rb +++ b/activesupport/lib/active_support/callbacks.rb @@ -142,8 +142,8 @@ def self.halting_and_conditional(callback_sequence, user_callback, user_conditio halted = env.halted if !halted && user_conditions.all? { |c| c.call(target, value) } - result = user_callback.call target, value - env.halted = halted_lambda.call(target, result) + result_lambda = -> { user_callback.call target, value } + env.halted = halted_lambda.call(target, result_lambda) if env.halted target.send :halted_callback_hook, filter end @@ -161,8 +161,9 @@ def self.halting(callback_sequence, user_callback, halted_lambda, filter) halted = env.halted unless halted - result = user_callback.call target, value - env.halted = halted_lambda.call(target, result) + result_lambda = -> { user_callback.call target, value } + env.halted = halted_lambda.call(target, result_lambda) + if env.halted target.send :halted_callback_hook, filter end @@ -517,7 +518,8 @@ class CallbackChain #:nodoc:# def initialize(name, config) @name = name @config = { - :scope => [ :kind ] + scope: [:kind], + terminator: default_terminator }.merge!(config) @chain = [] @callbacks = nil @@ -588,6 +590,17 @@ def remove_duplicates(callback) @callbacks = nil @chain.delete_if { |c| callback.duplicates?(c) } end + + def default_terminator + Proc.new do |target, result_lambda| + terminate = true + catch(:abort) do + result_lambda.call if result_lambda.is_a?(Proc) + terminate = false + end + terminate + end + end end module ClassMethods diff --git a/activesupport/test/callbacks_test.rb b/activesupport/test/callbacks_test.rb index d19e5fd6e7..73b00e80f6 100644 --- a/activesupport/test/callbacks_test.rb +++ b/activesupport/test/callbacks_test.rb @@ -511,8 +511,6 @@ def self.set_save_callbacks set_callback :save, :before, :third set_callback :save, :after, :first set_callback :save, :around, :around_it - set_callback :save, :after, :second - set_callback :save, :around, :around_it set_callback :save, :after, :third end @@ -552,16 +550,27 @@ def halted_callback_hook(filter) end class CallbackTerminator < AbstractCallbackTerminator - define_callbacks :save, terminator: ->(_,result) { result == :halt } + define_callbacks :save, terminator: ->(_, result_lambda) { result_lambda.call == :halt } set_save_callbacks end class CallbackTerminatorSkippingAfterCallbacks < AbstractCallbackTerminator - define_callbacks :save, terminator: ->(_,result) { result == :halt }, + define_callbacks :save, terminator: ->(_, result_lambda) { result_lambda.call == :halt }, skip_after_callbacks_if_terminated: true set_save_callbacks end + class CallbackDefaultTerminator < AbstractCallbackTerminator + define_callbacks :save + + def second + @history << "second" + throw(:abort) + end + + set_save_callbacks + end + class CallbackObject def before(caller) caller.record << "before" @@ -701,7 +710,7 @@ class CallbackTerminatorTest < ActiveSupport::TestCase def test_termination_skips_following_before_and_around_callbacks terminator = CallbackTerminator.new terminator.save - assert_equal ["first", "second", "third", "second", "first"], terminator.history + assert_equal ["first", "second", "third", "first"], terminator.history end def test_termination_invokes_hook @@ -725,6 +734,26 @@ def test_termination_skips_after_callbacks end end + class CallbackDefaultTerminatorTest < ActiveSupport::TestCase + def test_default_termination + terminator = CallbackDefaultTerminator.new + terminator.save + assert_equal ["first", "second", "third", "first"], terminator.history + end + + def test_default_termination_invokes_hook + terminator = CallbackDefaultTerminator.new + terminator.save + assert_equal :second, terminator.halted + end + + def test_block_never_called_if_abort_is_thrown + obj = CallbackDefaultTerminator.new + obj.save + assert !obj.saved + end + end + class HyphenatedKeyTest < ActiveSupport::TestCase def test_save obj = HyphenatedCallbacks.new -- GitLab