Added descriptive error messages for invalid has_many :through associations: going through :has_one or :has_and_belongs_to_many [Rick]

Added support for going through a polymorphic has_many association: (closes #4401) [Rick]


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

6 files changed, 64 insertions, 10 deletions

diff --git a/activerecord/CHANGELOG b/activerecord/CHANGELOG
index 4ffef99db6..dc61ab3c31 100644
--- a/activerecord/CHANGELOG
+++ b/activerecord/CHANGELOG
@@ -1,5 +1,19 @@
 *SVN*
 
+* Added descriptive error messages for invalid has_many :through associations: going through :has_one or :has_and_belongs_to_many [Rick]
+
+* Added support for going through a polymorphic has_many association: (closes #4401) [Rick]
+
+    class PhotoCollection < ActiveRecord::Base
+      has_many :photos, :as => :photographic
+      belongs_to :firm
+    end
+     
+    class Firm < ActiveRecord::Base
+      has_many :photo_collections
+      has_many :photos, :through => :photo_collections
+    end
+
 * Multiple fixes and optimizations in PostgreSQL adapter, allowing ruby-postgres gem to work properly. [ruben.nine@gmail.com]
 
 * Fixed that AssociationCollection#delete_all should work even if the records of the association are not loaded yet. [Florian Weber]
diff --git a/activerecord/lib/active_record/associations.rb b/activerecord/lib/active_record/associations.rb
index 3ad0eeecc7..5319fa39e8 100755
--- a/activerecord/lib/active_record/associations.rb
+++ b/activerecord/lib/active_record/associations.rb
@@ -44,6 +44,18 @@ module ActiveRecord
     end
   end
 
+  class HasManyThroughSourceAssociationMacroError < ActiveRecordError #:nodoc
+    def initialize(reflection)
+      @reflection         = reflection
+      @through_reflection = reflection.through_reflection
+      @source_reflection  = reflection.source_reflection
+    end
+    
+    def message
+      "Invalid source reflection macro :#{@source_reflection.macro}#{" :through" if @source_reflection.options[:through]} for has_many #{@reflection.name.inspect}, :through => #{@through_reflection.name.inspect}.  Use :source to specify the source reflection."
+    end
+  end
+
   class EagerLoadPolymorphicError < ActiveRecordError #:nodoc:
     def initialize(reflection)
       @reflection = reflection
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 dafae89861..44054b42e1 100644
--- a/activerecord/lib/active_record/associations/has_many_through_association.rb
+++ b/activerecord/lib/active_record/associations/has_many_through_association.rb
@@ -63,17 +63,12 @@ module ActiveRecord
           )
         end
 
-        def construct_conditions          
+        def construct_conditions
           conditions = if @reflection.through_reflection.options[:as]
               "#{@reflection.through_reflection.table_name}.#{@reflection.through_reflection.options[:as]}_id = #{@owner.quoted_id} " + 
               "AND #{@reflection.through_reflection.table_name}.#{@reflection.through_reflection.options[:as]}_type = #{@owner.class.quote @owner.class.base_class.name.to_s}"
           else
-            case @reflection.source_reflection.macro
-              when :belongs_to, :has_many
-                "#{@reflection.through_reflection.table_name}.#{@reflection.through_reflection.primary_key_name} = #{@owner.quoted_id}"
-              else
-                raise ActiveRecordError, "Invalid source reflection macro :#{@reflection.source_reflection.macro} for has_many #{@reflection.name}, :through => #{@reflection.through_reflection.name}.  Use :source to specify the source reflection."
-            end
+            "#{@reflection.through_reflection.table_name}.#{@reflection.through_reflection.primary_key_name} = #{@owner.quoted_id}"
           end
           conditions << " AND (#{sql_conditions})" if sql_conditions
           
@@ -88,19 +83,27 @@ module ActiveRecord
           selected = custom_select || @reflection.options[:select] || "#{@reflection.table_name}.*"          
         end
         
-        def construct_joins(custom_joins = nil)
+        def construct_joins(custom_joins = nil)          
+          polymorphic_join = nil
           if @reflection.through_reflection.options[:as] || @reflection.source_reflection.macro == :belongs_to
             reflection_primary_key = @reflection.klass.primary_key
             source_primary_key     = @reflection.source_reflection.primary_key_name
           else
             reflection_primary_key = @reflection.source_reflection.primary_key_name
             source_primary_key     = @reflection.klass.primary_key
+            if @reflection.source_reflection.options[:as]
+              polymorphic_join = "AND %s.%s = %s" % [
+                @reflection.table_name, "#{@reflection.source_reflection.options[:as]}_type",
+                @owner.class.quote(@reflection.through_reflection.klass.name)
+              ]
+            end
           end
 
-          "INNER JOIN %s ON %s.%s = %s.%s #{@reflection.options[:joins]} #{custom_joins}" % [
+          "INNER JOIN %s ON %s.%s = %s.%s %s #{@reflection.options[:joins]} #{custom_joins}" % [
             @reflection.through_reflection.table_name,
             @reflection.table_name, reflection_primary_key,
-            @reflection.through_reflection.table_name, source_primary_key
+            @reflection.through_reflection.table_name, source_primary_key,
+            polymorphic_join
           ]
         end
         
diff --git a/activerecord/lib/active_record/reflection.rb b/activerecord/lib/active_record/reflection.rb
index 40a5e5f2d1..d2c5393b59 100644
--- a/activerecord/lib/active_record/reflection.rb
+++ b/activerecord/lib/active_record/reflection.rb
@@ -176,6 +176,10 @@ module ActiveRecord
           if source_reflection.options[:polymorphic]
             raise HasManyThroughAssociationPolymorphicError.new(class_name, self, source_reflection)
           end
+          
+          unless [:belongs_to, :has_many].include?(source_reflection.macro) && source_reflection.options[:through].nil?
+            raise HasManyThroughSourceAssociationMacroError.new(self)
+          end
         end
       end
 
diff --git a/activerecord/test/associations_join_model_test.rb b/activerecord/test/associations_join_model_test.rb
index f0ee2eb9e7..b7f0af173b 100644
--- a/activerecord/test/associations_join_model_test.rb
+++ b/activerecord/test/associations_join_model_test.rb
@@ -295,6 +295,22 @@ class AssociationsJoinModelTest < Test::Unit::TestCase
     assert_equal comments(:more_greetings), authors(:david).comments.find(2)
   end
 
+  def test_has_many_through_polymorphic_has_one
+    assert_raise(ActiveRecord::HasManyThroughSourceAssociationMacroError) { authors(:david).tagging }
+  end
+
+  def test_has_many_through_polymorphic_has_many
+    assert_equal [taggings(:welcome_general), taggings(:thinking_general)], authors(:david).taggings.uniq.sort_by { |t| t.id }
+  end
+
+  def test_has_many_through_has_many_through
+    assert_raise(ActiveRecord::HasManyThroughSourceAssociationMacroError) { authors(:david).tags }
+  end
+
+  def test_has_many_through_habtm
+    assert_raise(ActiveRecord::HasManyThroughSourceAssociationMacroError) { authors(:david).post_categories }
+  end
+
   def test_eager_load_has_many_through_has_many
     author = Author.find :first, :conditions => ['name = ?', 'David'], :include => :comments, :order => 'comments.id'
     SpecialComment.new; VerySpecialComment.new
diff --git a/activerecord/test/fixtures/author.rb b/activerecord/test/fixtures/author.rb
index 1a9b6d788a..99142f6621 100644
--- a/activerecord/test/fixtures/author.rb
+++ b/activerecord/test/fixtures/author.rb
@@ -31,6 +31,11 @@ class Author < ActiveRecord::Base
   has_many :author_favorites
   has_many :favorite_authors, :through => :author_favorites, :order => 'name'
 
+  has_many :tagging,  :through => :posts # through polymorphic has_one
+  has_many :taggings, :through => :posts, :source => :taggings # through polymorphic has_many
+  has_many :tags,     :through => :posts # through has_many :through
+  has_many :post_categories, :through => :posts, :source => :categories
+
   belongs_to :author_address
 
   attr_accessor :post_log