Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP
Browse files

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.
  • Loading branch information...
commit 4a9644a0d94a88896e1ebd3329b8f796fbd053d1 1 parent 01d3a36
@dmitriy-kiriyenko dmitriy-kiriyenko authored
View
9 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
View
34 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
View
62 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
View
10 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
Please sign in to comment.
Something went wrong with that request. Please try again.