From 4a9644a0d94a88896e1ebd3329b8f796fbd053d1 Mon Sep 17 00:00:00 2001 From: Dmitriy Kiriyenko Date: Wed, 15 Aug 2012 15:43:04 +0300 Subject: [PATCH] Prevent callback from being set twice. When you add one callack in two separate `set_callback` calls - it is only called once. When you do it in one `set_callback` call - it is called twice. This violates the principle of least astonishment for me. Duplicating callback is usually an error. There is a correct and obvious way to do anything without this "feature". If you want to do before_save :clear_balance, :calculate_tax, :clear_balance or whatever, you should better do before_save :carefully_calculate_tax def carefully_calculate_tax clear_balance calculate_tax clear_balance end And this even opens gates for some advanced refactorings, unlike the first approach. My assumptions are: - Principle of least astonishment is violated, when callbacks are either prevented from duplication, or not. - Duplicating callbacks is usually an error. When it is intentional - it's a smell of a bad design and can be approached without abusing this "feature". My suggestion is: do not allow duplicating callbacks in one callback call, like it is not allowed in separate callbacks call. --- activesupport/CHANGELOG.md | 9 +++ activesupport/lib/active_support/callbacks.rb | 34 ++++++++-- activesupport/test/callbacks_test.rb | 62 +++++++++++++++++-- activesupport/test/test_test.rb | 10 +-- 4 files changed, 100 insertions(+), 15 deletions(-) diff --git a/activesupport/CHANGELOG.md b/activesupport/CHANGELOG.md index 7b93731917..2fd5e0318f 100644 --- a/activesupport/CHANGELOG.md +++ b/activesupport/CHANGELOG.md @@ -1,5 +1,14 @@ ## Rails 4.0.0 (unreleased) ## +* Prevent `Callbacks#set_callback` from setting the same callback twice. + + before_save :foo, :bar, :foo + + will at first call `bar`, then `foo`. `foo` will no more be called + twice. + + *Dmitriy Kiriyenko* + * Deprecate `Time.time_with_date_fallback`, `Time.utc_time` and `Time.local_time`. These methods were added to handle the limited range of Ruby's native Time implementation. Those limitations no longer apply so we are deprecating them in 4.0 diff --git a/activesupport/lib/active_support/callbacks.rb b/activesupport/lib/active_support/callbacks.rb index 3a8353857e..7fdb101d24 100644 --- a/activesupport/lib/active_support/callbacks.rb +++ b/activesupport/lib/active_support/callbacks.rb @@ -133,6 +133,10 @@ def matches?(_kind, _filter) @kind == _kind && @filter == _filter end + def duplicates?(other) + matches?(other.kind, other.filter) + end + def _update_filter(filter_options, new_options) filter_options[:if].concat(Array(new_options[:unless])) if new_options.key?(:unless) filter_options[:unless].concat(Array(new_options[:if])) if new_options.key?(:if) @@ -327,6 +331,30 @@ def compile method.join("\n") end + def append(*callbacks) + callbacks.each { |c| append_one(c) } + end + + def prepend(*callbacks) + callbacks.each { |c| prepend_one(c) } + end + + private + + def append_one(callback) + remove_duplicates(callback) + push(callback) + end + + def prepend_one(callback) + remove_duplicates(callback) + unshift(callback) + end + + def remove_duplicates(callback) + delete_if { |c| callback.duplicates?(c) } + end + end module ClassMethods @@ -412,11 +440,7 @@ def set_callback(name, *filter_list, &block) Callback.new(chain, filter, type, options.dup, self) end - filters.each do |filter| - chain.delete_if {|c| c.matches?(type, filter) } - end - - options[:prepend] ? chain.unshift(*(mapped.reverse)) : chain.push(*mapped) + options[:prepend] ? chain.prepend(*mapped) : chain.append(*mapped) target.send("_#{name}_callbacks=", chain) end diff --git a/activesupport/test/callbacks_test.rb b/activesupport/test/callbacks_test.rb index 8810302f40..13f2e3cdaf 100644 --- a/activesupport/test/callbacks_test.rb +++ b/activesupport/test/callbacks_test.rb @@ -297,7 +297,7 @@ def save end end end - + class AroundPersonResult < MySuper attr_reader :result @@ -308,7 +308,7 @@ class AroundPersonResult < MySuper def tweedle_dum @result = yield end - + def tweedle_1 :tweedle_1 end @@ -316,7 +316,7 @@ def tweedle_1 def tweedle_2 :tweedle_2 end - + def save run_callbacks :save do :running @@ -410,7 +410,7 @@ def test_save_around ], around.history end end - + class AroundCallbackResultTest < ActiveSupport::TestCase def test_save_around around = AroundPersonResult.new @@ -607,6 +607,45 @@ def save end end + class OneTwoThreeSave + include ActiveSupport::Callbacks + + define_callbacks :save + + attr_accessor :record + + def initialize + @record = [] + end + + def save + run_callbacks :save do + @record << "yielded" + end + end + + def first + @record << "one" + end + + def second + @record << "two" + end + + def third + @record << "three" + end + end + + class DuplicatingCallbacks < OneTwoThreeSave + set_callback :save, :before, :first, :second + set_callback :save, :before, :first, :third + end + + class DuplicatingCallbacksInSameCall < OneTwoThreeSave + set_callback :save, :before, :first, :second, :first, :third + end + class UsingObjectTest < ActiveSupport::TestCase def test_before_object u = UsingObjectBefore.new @@ -709,5 +748,18 @@ def test_per_key_option_deprecaton end end end - + + class ExcludingDuplicatesCallbackTest < ActiveSupport::TestCase + def test_excludes_duplicates_in_separate_calls + model = DuplicatingCallbacks.new + model.save + assert_equal ["two", "one", "three", "yielded"], model.record + end + + def test_excludes_duplicates_in_one_call + model = DuplicatingCallbacksInSameCall.new + model.save + assert_equal ["two", "one", "three", "yielded"], model.record + end + end end diff --git a/activesupport/test/test_test.rb b/activesupport/test/test_test.rb index 9516556844..d6821135ea 100644 --- a/activesupport/test/test_test.rb +++ b/activesupport/test/test_test.rb @@ -122,12 +122,12 @@ class AlsoDoingNothingTest < ActiveSupport::TestCase # Setup and teardown callbacks. class SetupAndTeardownTest < ActiveSupport::TestCase setup :reset_callback_record, :foo - teardown :foo, :sentinel, :foo + teardown :foo, :sentinel def test_inherited_setup_callbacks assert_equal [:reset_callback_record, :foo], self.class._setup_callbacks.map(&:raw_filter) assert_equal [:foo], @called_back - assert_equal [:foo, :sentinel, :foo], self.class._teardown_callbacks.map(&:raw_filter) + assert_equal [:foo, :sentinel], self.class._teardown_callbacks.map(&:raw_filter) end def setup @@ -147,7 +147,7 @@ def foo end def sentinel - assert_equal [:foo, :foo], @called_back + assert_equal [:foo], @called_back end end @@ -159,7 +159,7 @@ class SubclassSetupAndTeardownTest < SetupAndTeardownTest def test_inherited_setup_callbacks assert_equal [:reset_callback_record, :foo, :bar], self.class._setup_callbacks.map(&:raw_filter) assert_equal [:foo, :bar], @called_back - assert_equal [:foo, :sentinel, :foo, :bar], self.class._teardown_callbacks.map(&:raw_filter) + assert_equal [:foo, :sentinel, :bar], self.class._teardown_callbacks.map(&:raw_filter) end protected @@ -168,7 +168,7 @@ def bar end def sentinel - assert_equal [:foo, :bar, :bar, :foo], @called_back + assert_equal [:foo, :bar, :bar], @called_back end end -- GitLab