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

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

10 files changed, 63 insertions, 48 deletions

diff --git a/activerecord/CHANGELOG b/activerecord/CHANGELOG
index b7c805e2a2..082fdd620a 100644
--- a/activerecord/CHANGELOG
+++ b/activerecord/CHANGELOG
@@ -1,5 +1,7 @@
 *SVN*
 
+* Allow :uniq => true with has_many :through associations. [Jeremy Kemper]
+
 * Ensure that StringIO is always available for the Schema dumper. [Marcel Molina Jr.]
 
 * Allow AR::Base#to_xml to include methods too. Closes #4921. [johan@textdrive.com] 
diff --git a/activerecord/lib/active_record/associations.rb b/activerecord/lib/active_record/associations.rb
index 98f4e43883..31546e9c29 100755
--- a/activerecord/lib/active_record/associations.rb
+++ b/activerecord/lib/active_record/associations.rb
@@ -519,6 +519,7 @@ module ActiveRecord
       # * <tt>:source</tt>: Specifies the source association name used by <tt>has_many :through</tt> queries.  Only use it if the name cannot be 
       #   inferred from the association.  <tt>has_many :subscribers, :through => :subscriptions</tt> will look for either +:subscribers+ or
       #   +:subscriber+ on +Subscription+, unless a +:source+ is given.
+      # * <tt>:uniq</tt> - if set to true, duplicates will be omitted from the collection. Useful in conjunction with :through.
       #
       # Option examples:
       #   has_many :comments, :order => "posted_on"
@@ -1048,6 +1049,7 @@ module ActiveRecord
             :exclusively_dependent, :dependent,
             :select, :conditions, :include, :order, :group, :limit, :offset,
             :as, :through, :source,
+            :uniq,
             :finder_sql, :counter_sql, 
             :before_add, :after_add, :before_remove, :after_remove, 
             :extend
@@ -1085,7 +1087,8 @@ module ActiveRecord
           options.assert_valid_keys(
             :class_name, :table_name, :join_table, :foreign_key, :association_foreign_key, 
             :select, :conditions, :include, :order, :group, :limit, :offset,
-            :finder_sql, :delete_sql, :insert_sql, :uniq, 
+            :uniq, 
+            :finder_sql, :delete_sql, :insert_sql,
             :before_add, :after_add, :before_remove, :after_remove, 
             :extend
           )
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
diff --git a/activerecord/test/associations_cascaded_eager_loading_test.rb b/activerecord/test/associations_cascaded_eager_loading_test.rb
index dd12d529a2..642e4fa433 100644
--- a/activerecord/test/associations_cascaded_eager_loading_test.rb
+++ b/activerecord/test/associations_cascaded_eager_loading_test.rb
@@ -28,7 +28,7 @@ class CascadedEagerLoadingTest < Test::Unit::TestCase
     assert_equal 1, authors[1].posts.size
     assert_equal 9, authors[0].posts.collect{|post| post.comments.size }.inject(0){|sum,i| sum+i}
     assert_equal 1, authors[0].categorizations.size
-    assert_equal 1, authors[1].categorizations.size
+    assert_equal 2, authors[1].categorizations.size
   end
 
   def test_eager_association_loading_with_cascaded_two_levels_with_two_has_many_associations
diff --git a/activerecord/test/associations_join_model_test.rb b/activerecord/test/associations_join_model_test.rb
index 9bacbfc717..be04df9c00 100644
--- a/activerecord/test/associations_join_model_test.rb
+++ b/activerecord/test/associations_join_model_test.rb
@@ -12,21 +12,26 @@ class AssociationsJoinModelTest < Test::Unit::TestCase
   fixtures :posts, :authors, :categories, :categorizations, :comments, :tags, :taggings, :author_favorites
 
   def test_has_many
-    assert_equal categories(:general), authors(:david).categories.first
+    assert authors(:david).categories.include?(categories(:general))
   end
-  
+
   def test_has_many_inherited
-    assert_equal categories(:sti_test), authors(:mary).categories.first
+    assert authors(:mary).categories.include?(categories(:sti_test))
   end
 
   def test_inherited_has_many
-    assert_equal authors(:mary), categories(:sti_test).authors.first
+    assert categories(:sti_test).authors.include?(authors(:mary))
   end
-  
+
+  def test_has_many_uniq_through_join_model
+    assert_equal 2, authors(:mary).categorized_posts.size
+    assert_equal 1, authors(:mary).unique_categorized_posts.size
+  end
+
   def test_polymorphic_has_many
-    assert_equal taggings(:welcome_general), posts(:welcome).taggings.first
+    assert posts(:welcome).taggings.include?(taggings(:welcome_general))
   end
-  
+
   def test_polymorphic_has_one
     assert_equal taggings(:welcome_general), posts(:welcome).tagging
   end
diff --git a/activerecord/test/fixtures/author.rb b/activerecord/test/fixtures/author.rb
index 99142f6621..ec844f6238 100644
--- a/activerecord/test/fixtures/author.rb
+++ b/activerecord/test/fixtures/author.rb
@@ -26,6 +26,9 @@ class Author < ActiveRecord::Base
   has_many :categorizations
   has_many :categories, :through => :categorizations
 
+  has_many :categorized_posts, :through => :categorizations, :source => :post
+  has_many :unique_categorized_posts, :through => :categorizations, :source => :post, :uniq => true
+
   has_many :nothings, :through => :kateggorisatons, :class_name => 'Category'
 
   has_many :author_favorites
@@ -73,4 +76,4 @@ end
 class AuthorFavorite < ActiveRecord::Base
   belongs_to :author
   belongs_to :favorite_author, :class_name => "Author", :foreign_key => 'favorite_author_id'
-end
\ No newline at end of file
+end
diff --git a/activerecord/test/fixtures/categorizations.yml b/activerecord/test/fixtures/categorizations.yml
index f8701fbde0..c5b6fc9a51 100644
--- a/activerecord/test/fixtures/categorizations.yml
+++ b/activerecord/test/fixtures/categorizations.yml
@@ -3,9 +3,15 @@ david_welcome_general:
   author_id: 1
   post_id: 1
   category_id: 1
-  
+
 mary_thinking_sti:
   id: 2
   author_id: 2
   post_id: 2
-  category_id: 3
\ No newline at end of file
+  category_id: 3
+
+mary_thinking_general:
+  id: 3
+  author_id: 2
+  post_id: 2
+  category_id: 1