Merge pull request #7376 from dmitriy-kiriyenko/fix-double-callback-in-same-statement

Prevent callback from being set twice.

Conflicts:
	activesupport/CHANGELOG.md

activesupport/CHANGELOG.md

 ## 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*
+
 *   Add ActiveSupport::Logger#silence that works the same as the old Logger#silence extension.
 
     *DHH*

activesupport/lib/active_support/callbacks.rb

@@ -134,6 +134,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)
@@ -328,6 +332,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
@@ -421,11 +449,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

activesupport/test/callbacks_test.rb

@@ -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
@@ -710,4 +749,17 @@ def test_per_key_option_deprecaton
     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

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