1 files changed, 49 insertions, 42 deletions

diff --git a/activerecord/lib/active_record/associations/collection_association.rb b/activerecord/lib/active_record/associations/collection_association.rb
index 7aed64d48c..2447914640 100644
--- a/activerecord/lib/active_record/associations/collection_association.rb
+++ b/activerecord/lib/active_record/associations/collection_association.rb
@@ -6,6 +6,15 @@ module ActiveRecord
     # ease the implementation of association proxies that represent
     # collections. See the class hierarchy in AssociationProxy.
     #
+    #   CollectionAssociation:
+    #     HasAndBelongsToManyAssociation => has_and_belongs_to_many
+    #     HasManyAssociation => has_many
+    #       HasManyThroughAssociation + ThroughAssociation => has_many :through
+    #
+    # CollectionAssociation class provides common methods to the collections
+    # defined by +has_and_belongs_to_many+, +has_many+ or +has_many+ with
+    # +:through association+ option.
+    #
     # You need to be careful with assumptions regarding the target: The proxy
     # does not fetch records from the database until it needs them, but new
     # ones created with +build+ are added to the target. So, the target may be
@@ -16,12 +25,6 @@ module ActiveRecord
     # If you need to work on all current children, new and existing records,
     # +load_target+ and the +loaded+ flag are your friends.
     class CollectionAssociation < Association #:nodoc:
-      attr_reader :proxy
-
-      def initialize(owner, reflection)
-        super
-        @proxy = CollectionProxy.new(self)
-      end
 
       # Implements the reader method, e.g. foo.items for Foo.has_many :items
       def reader(force_reload = false)
@@ -31,7 +34,7 @@ module ActiveRecord
           reload
         end
 
-        proxy
+        CollectionProxy.new(self)
       end
 
       # Implements the writer method, e.g. foo.items= for Foo.has_many :items
@@ -47,18 +50,7 @@ module ActiveRecord
           end
         else
           column  = "#{reflection.quoted_table_name}.#{reflection.association_primary_key}"
-          relation = scoped
-
-          including = (relation.eager_load_values + relation.includes_values).uniq
-
-          if including.any?
-            join_dependency = ActiveRecord::Associations::JoinDependency.new(reflection.klass, including, [])
-            relation = join_dependency.join_associations.inject(relation) do |r, association|
-              association.join_relation(r)
-            end
-          end
-
-          relation.uniq.pluck(column)
+          scoped.pluck(column)
         end
       end
 
@@ -71,7 +63,7 @@ module ActiveRecord
       end
 
       def reset
-        @loaded = false
+        super
         @target = []
       end
 
@@ -121,8 +113,9 @@ module ActiveRecord
         create_record(attributes, options, true, &block)
       end
 
-      # Add +records+ to this association. Returns +self+ so method calls may be chained.
-      # Since << flattens its argument list and inserts each record, +push+ and +concat+ behave identically.
+      # Add +records+ to this association. Returns +self+ so method calls may
+      # be chained. Since << flattens its argument list and inserts each record,
+      # +push+ and +concat+ behave identically.
       def concat(*records)
         load_target if owner.new_record?
 
@@ -148,23 +141,16 @@ module ActiveRecord
         end
       end
 
-      # Remove all records from this association
+      # Remove all records from this association.
       #
       # See delete for more info.
       def delete_all
-        delete(load_target).tap do
+        delete(:all).tap do
           reset
           loaded!
         end
       end
 
-      # Called when the association is declared as :dependent => :delete_all. This is
-      # an optimised version which avoids loading the records into memory. Not really
-      # for public consumption.
-      def delete_all_on_destroy
-        scoped.delete_all
-      end
-
       # Destroy all the records from this association.
       #
       # See destroy for more info.
@@ -175,7 +161,7 @@ module ActiveRecord
         end
       end
 
-      # Calculate sum using SQL, not Enumerable
+      # Calculate sum using SQL, not Enumerable.
       def sum(*args)
         if block_given?
           scoped.sum(*args) { |*block_args| yield(*block_args) }
@@ -224,7 +210,18 @@ module ActiveRecord
       # are actually removed from the database, that depends precisely on
       # +delete_records+. They are in any case removed from the collection.
       def delete(*records)
-        delete_or_destroy(records, options[:dependent])
+        dependent = options[:dependent]
+
+        if records.first == :all
+          if loaded? || dependent == :destroy
+            delete_or_destroy(load_target, dependent)
+          else
+            delete_records(:all, dependent)
+          end
+        else
+          records = find(records) if records.any? { |record| record.kind_of?(Fixnum) || record.kind_of?(String) }
+          delete_or_destroy(records, dependent)
+        end
       end
 
       # Destroy +records+ and remove them from this association calling
@@ -248,8 +245,12 @@ module ActiveRecord
       # This method is abstract in the sense that it relies on
       # +count_records+, which is a method descendants have to provide.
       def size
-        if !find_target? || (loaded? && !options[:uniq])
-          target.size
+        if !find_target? || loaded?
+          if options[:uniq]
+            target.uniq.size
+          else
+            target.size
+          end
         elsif !loaded? && options[:group]
           load_target.size
         elsif !loaded? && !options[:uniq] && target.is_a?(Array)
@@ -269,13 +270,16 @@ module ActiveRecord
         load_target.size
       end
 
-      # Equivalent to <tt>collection.size.zero?</tt>. If the collection has
-      # not been already loaded and you are going to fetch the records anyway
-      # it is better to check <tt>collection.length.zero?</tt>.
+      # Returns true if the collection is empty. Equivalent to
+      # <tt>collection.size.zero?</tt>. If the collection has not been already
+      # loaded and you are going to fetch the records anyway it is better to
+      # check <tt>collection.length.zero?</tt>.
       def empty?
         size.zero?
       end
 
+      # Returns true if the collections is not empty.
+      # Equivalent to +!collection.empty?+.
       def any?
         if block_given?
           load_target.any? { |*block_args| yield(*block_args) }
@@ -284,7 +288,8 @@ module ActiveRecord
         end
       end
 
-      # Returns true if the collection has more than 1 record. Equivalent to collection.size > 1.
+      # Returns true if the collection has more than 1 record.
+      # Equivalent to +collection.size > 1+.
       def many?
         if block_given?
           load_target.many? { |*block_args| yield(*block_args) }
@@ -300,8 +305,8 @@ module ActiveRecord
         end
       end
 
-      # Replace this collection with +other_array+
-      # This will perform a diff and delete/add only records that have changed.
+      # Replace this collection with +other_array+. This will perform a diff
+      # and delete/add only records that have changed.
       def replace(other_array)
         other_array.each { |val| raise_on_type_mismatch(val) }
         original_target = load_target.dup
@@ -474,6 +479,8 @@ module ActiveRecord
             raise RecordNotSaved, "Failed to replace #{reflection.name} because one or more of the " \
                                   "new records could not be saved."
           end
+
+          target
         end
 
         def concat_records(records)
@@ -544,7 +551,7 @@ module ActiveRecord
         # If using a custom finder_sql, #find scans the entire collection.
         def find_by_scan(*args)
           expects_array = args.first.kind_of?(Array)
-          ids           = args.flatten.compact.uniq.map { |arg| arg.to_i }
+          ids           = args.flatten.compact.map{ |arg| arg.to_i }.uniq
 
           if ids.size == 1
             id = ids.first