Allow :uniq => true with has_many :through associations.

git-svn-id: http://svn-commit.rubyonrails.org/rails/trunk@4325 5ecf4fe2-1ee6-0310-87b1-e25e094e27de

4 files changed, 32 insertions, 36 deletions

diff --git a/activerecord/lib/active_record/associations/association_collection.rb b/activerecord/lib/active_record/associations/association_collection.rb
index 268452f422..e705576120 100644
--- a/activerecord/lib/active_record/associations/association_collection.rb
+++ b/activerecord/lib/active_record/associations/association_collection.rb
@@ -9,7 +9,7 @@ module ActiveRecord
       end
   
       def reset
-        @target = []
+        reset_target!
         @loaded = false
       end
 
@@ -28,7 +28,7 @@ module ActiveRecord
             callback(:after_add, record)
           end
         end
-				
+
         result && self
       end
 
@@ -39,7 +39,7 @@ module ActiveRecord
       def delete_all
         load_target
         delete(@target)
-        @target = []
+        reset_target!
       end
 
       # Remove +records+ from this association.  Does not destroy +records+.
@@ -77,9 +77,9 @@ module ActiveRecord
           each { |record| record.destroy }
         end
 
-        @target = []
+        reset_target!
       end
-      
+
       def create(attributes = {})
         # Can't use Base.create since the foreign key may be a protected attribute.
         if attributes.is_a?(Array)
@@ -95,21 +95,21 @@ module ActiveRecord
       # calling collection.size if it has. If it's more likely than not that the collection does have a size larger than zero
       # and you need to fetch that collection afterwards, it'll take one less SELECT query if you use length.
       def size
-        if loaded? then @target.size else count_records end
+        if loaded? && !@reflection.options[:uniq] then @target.size else count_records end
       end
-      
+
       # Returns the size of the collection by loading it and calling size on the array. If you want to use this method to check
       # whether the collection is empty, use collection.length.zero? instead of collection.empty?
       def length
         load_target.size
       end
-      
+
       def empty?
         size.zero?
       end
-      
+
       def uniq(collection = self)
-        collection.inject([]) { |uniq_records, record| uniq_records << record unless uniq_records.include?(record); uniq_records }
+        collection.to_set.to_a
       end
 
       # Replace this collection with +other_array+
@@ -127,6 +127,22 @@ module ActiveRecord
         end
       end
 
+      protected
+        def reset_target!
+          @target = Array.new
+        end
+
+        def find_target
+          records =
+            if @reflection.options[:finder_sql]
+              @reflection.klass.find_by_sql(@finder_sql)
+            else
+              find(:all)
+            end
+
+          @reflection.options[:uniq] ? uniq(records) : records
+        end
+
       private
         # Array#flatten has problems with recursive arrays. Going one level deeper solves the majority of the problems.
         def flatten_deeper(array)
diff --git a/activerecord/lib/active_record/associations/has_and_belongs_to_many_association.rb b/activerecord/lib/active_record/associations/has_and_belongs_to_many_association.rb
index cd866d2cdd..cc6549f3ae 100644
--- a/activerecord/lib/active_record/associations/has_and_belongs_to_many_association.rb
+++ b/activerecord/lib/active_record/associations/has_and_belongs_to_many_association.rb
@@ -68,13 +68,9 @@ module ActiveRecord
 
         self
       end
-      
+
       alias :concat_with_attributes :push_with_attributes
 
-      def size
-        @reflection.options[:uniq] ? count_records : super
-      end
-      
       protected
         def method_missing(method, *args, &block)
           if @target.respond_to?(method) || (!@reflection.klass.respond_to?(method) && Class.respond_to?(method))
@@ -85,17 +81,7 @@ module ActiveRecord
             end
           end
         end
-            
-        def find_target
-          if @reflection.options[:finder_sql]
-            records = @reflection.klass.find_by_sql(@finder_sql)
-          else
-            records = find(:all)
-          end
-          
-          @reflection.options[:uniq] ? uniq(records) : records
-        end
-        
+
         def count_records
           load_target.size
         end
diff --git a/activerecord/lib/active_record/associations/has_many_association.rb b/activerecord/lib/active_record/associations/has_many_association.rb
index 56368f4198..912e0aa390 100644
--- a/activerecord/lib/active_record/associations/has_many_association.rb
+++ b/activerecord/lib/active_record/associations/has_many_association.rb
@@ -106,14 +106,6 @@ module ActiveRecord
             end
           end
         end
-            
-        def find_target
-          if @reflection.options[:finder_sql]
-            @reflection.klass.find_by_sql(@finder_sql)
-          else
-            find(:all)
-          end
-        end
 
         def count_records
           count = if has_cached_counter?
diff --git a/activerecord/lib/active_record/associations/has_many_through_association.rb b/activerecord/lib/active_record/associations/has_many_through_association.rb
index 8cafb26d44..037c375cde 100644
--- a/activerecord/lib/active_record/associations/has_many_through_association.rb
+++ b/activerecord/lib/active_record/associations/has_many_through_association.rb
@@ -56,9 +56,9 @@ module ActiveRecord
             @reflection.klass.with_scope(construct_scope) { @reflection.klass.send(method, *args, &block) }
           end
         end
-            
+
         def find_target
-          @reflection.klass.find(:all, 
+          records = @reflection.klass.find(:all, 
             :select     => construct_select,
             :conditions => construct_conditions,
             :from       => construct_from,
@@ -68,6 +68,8 @@ module ActiveRecord
             :group      => @reflection.options[:group],
             :include    => @reflection.options[:include] || @reflection.source_reflection.options[:include]
           )
+
+          @reflection.options[:uniq] ? records.to_set.to_a : records
         end
 
         def construct_conditions