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.

1 files changed, 29 insertions, 5 deletions

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 @@ module ActiveSupport
         @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 @@ module ActiveSupport
         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 @@ module ActiveSupport
             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