Integrate nested support into ThroughAssociationScope, using my conce…

…pt of generating a 'chain' of reflections to be joined. It seems to work at the moment, all existing tests are passing. There may be further complications as we add more test cases for nested associations, though.

activerecord/lib/active_record/associations/has_many_through_association.rb

@@ -7,7 +7,6 @@ module ActiveRecord
   module Associations
     class HasManyThroughAssociation < HasManyAssociation #:nodoc:
       include ThroughAssociationScope
-      # include NestedHasManyThrough
 
       alias_method :new, :build
 

activerecord/lib/active_record/associations/nested_has_many_through.rb

@@ -1,3 +1,5 @@
+# TODO: Remove in the end, when its functionality is fully integrated in ThroughAssociationScope.
+
 module ActiveRecord
   module Associations
     module NestedHasManyThrough

activerecord/lib/active_record/associations/through_association_scope.rb

@@ -1,3 +1,5 @@
+require 'enumerator'
+
 module ActiveRecord
   # = Active Record Through Association Scope
   module Associations
@@ -19,8 +21,9 @@ def construct_scope
 
       # Build SQL conditions from attributes, qualified by table name.
       def construct_conditions
-        table_name = @reflection.final_through_reflection.quoted_table_name
-        conditions = construct_quoted_owner_attributes(@reflection.final_through_reflection).map do |attr, value|
+        reflection = @reflection.through_reflection_chain.last
+        table_name = reflection.quoted_table_name
+        conditions = construct_quoted_owner_attributes(reflection).map do |attr, value|
           "#{table_name}.#{attr} = #{value}"
         end
         conditions << sql_conditions if sql_conditions
@@ -51,43 +54,57 @@ def construct_select(custom_select = nil)
       end
 
       def construct_joins(custom_joins = nil)
-        "#{construct_through_joins(@reflection)} #{@reflection.options[:joins]} #{custom_joins}"
+        # puts @reflection.through_reflection_chain.map(&:inspect)
+
+        "#{construct_through_joins} #{@reflection.options[:joins]} #{custom_joins}"
       end
 
-      def construct_through_joins(reflection)
-        polymorphic_join = nil
-        if reflection.source_reflection.macro == :belongs_to
-          reflection_primary_key = reflection.klass.primary_key
-          source_primary_key     = reflection.source_reflection.primary_key_name
-          if reflection.options[:source_type]
-            polymorphic_join = "AND %s.%s = %s" % [
-              reflection.through_reflection.quoted_table_name, "#{@reflection.source_reflection.options[:foreign_type]}",
-              @owner.class.quote_value(reflection.options[:source_type])
-            ]
-          end
-        else
-          reflection_primary_key = reflection.source_reflection.primary_key_name
-          source_primary_key     = reflection.through_reflection.klass.primary_key
-          if reflection.source_reflection.options[:as]
-            polymorphic_join = "AND %s.%s = %s" % [
-              reflection.quoted_table_name, "#{@reflection.source_reflection.options[:as]}_type",
-              @owner.class.quote_value(reflection.through_reflection.klass.name)
-            ]
-          end
-        end
-
-        joins = "INNER JOIN %s ON %s.%s = %s.%s %s" % [
-          reflection.through_reflection.quoted_table_name,
-          reflection.quoted_table_name, reflection_primary_key,
-          reflection.through_reflection.quoted_table_name, source_primary_key,
-          polymorphic_join
-        ]
+      def construct_through_joins
+        joins = []
 
-        # If the reflection we are going :through goes itself :through another reflection, then
-        # we must recursively get the joins to make that happen too.
-        if reflection.through_reflection.through_reflection
-          joins << " "
-          joins << construct_through_joins(reflection.through_reflection)
+        # Iterate over each pair in the through reflection chain, joining them together
+        @reflection.through_reflection_chain.each_cons(2) do |left, right|
+          polymorphic_join  = nil
+
+          case
+            when left.options[:as]
+              left_primary_key  = left.primary_key_name
+              right_primary_key = right.klass.primary_key
+
+              polymorphic_join = "AND %s.%s = %s" % [
+                left.quoted_table_name, "#{left.options[:as]}_type",
+                @owner.class.quote_value(right.klass.name)
+              ]
+            when left.source_reflection.macro == :belongs_to
+              left_primary_key  = left.klass.primary_key
+              right_primary_key = left.source_reflection.primary_key_name
+
+              if left.options[:source_type]
+                polymorphic_join = "AND %s.%s = %s" % [
+                  right.quoted_table_name,
+                  left.source_reflection.options[:foreign_type].to_s,
+                  @owner.class.quote_value(left.options[:source_type])
+                ]
+              end
+            else
+              left_primary_key  = left.source_reflection.primary_key_name
+              right_primary_key = right.klass.primary_key
+
+              if left.source_reflection.options[:as]
+                polymorphic_join = "AND %s.%s = %s" % [
+                  left.quoted_table_name,
+                  "#{left.source_reflection.options[:as]}_type",
+                  @owner.class.quote_value(right.klass.name)
+                ]
+              end
+          end
+
+          joins << "INNER JOIN %s ON %s.%s = %s.%s %s" % [
+            right.quoted_table_name,
+            left.quoted_table_name,  left_primary_key,
+            right.quoted_table_name, right_primary_key,
+            polymorphic_join
+          ]
         end
 
         joins

activerecord/lib/active_record/reflection.rb

@@ -131,6 +131,14 @@ def sanitized_conditions #:nodoc:
         @sanitized_conditions ||= klass.send(:sanitize_sql, options[:conditions]) if options[:conditions]
       end
 
+      # TODO: Remove these in the final patch. I am just using them for debugging etc.
+      def inspect
+        "#<#{code_name}>"
+      end
+      def code_name
+        "#{active_record.name}.#{macro} :#{name}"
+      end
+
       private
         def derive_class_name
           name.to_s.camelize
@@ -241,6 +249,10 @@ def check_validity_of_inverse!
       def through_reflection
         false
       end
+
+      def through_reflection_chain
+        [self]
+      end
 
       def through_reflection_primary_key_name
       end
@@ -304,6 +316,16 @@ def dependent_conditions(record, base_class, extra_conditions)
       def belongs_to?
         macro == :belongs_to
       end
+
+      # TODO: Remove for final patch. Just here for debugging.
+      def inspect
+        str = "#<#{code_name}, @source_reflection="
+        str << (source_reflection.respond_to?(:code_name) ? source_reflection.code_name : source_reflection.inspect)
+        str << ", @through_reflection="
+        str << (through_reflection.respond_to?(:code_name) ? through_reflection.code_name : through_reflection.inspect)
+        str << ">"
+        str
+      end
 
       private
         def derive_class_name
@@ -353,18 +375,24 @@ def through_reflection
         @through_reflection ||= active_record.reflect_on_association(options[:through])
       end
 
-      # A :through reflection may have a :through reflection itself. This method returns the through
-      # reflection which is furthest away, i.e. the last in the chain, so the first which does not
-      # have its own :through reflection.
-      def final_through_reflection
-        @final_through_reflection ||= begin
-          reflection = through_reflection
-
-          while reflection.through_reflection
-            reflection = reflection.through_reflection
+      # TODO: Documentation
+      def through_reflection_chain
+        @through_reflection_chain ||= begin
+          if source_reflection.through_reflection
+            # If the source reflection goes through another reflection, then the chain must start
+            # by getting us to the source reflection.
+            chain = source_reflection.through_reflection_chain
+          else
+            # If the source reflection does not go through another reflection, then we can get
+            # to this reflection directly, and so start the chain here
+            chain = [self]
           end
 
-          reflection
+          # Recursively build the rest of the chain
+          chain += through_reflection.through_reflection_chain
+
+          # Finally return the completed chain
+          chain
         end
       end
 
@@ -393,6 +421,8 @@ def check_validity!
           raise HasManyThroughAssociationPolymorphicError.new(active_record.name, self, source_reflection)
         end
 
+        # TODO: Presumably remove the HasManyThroughSourceAssociationMacroError class and delete these lines.
+        # Think about whether there are any cases which should still be disallowed.
         # unless [:belongs_to, :has_many, :has_one].include?(source_reflection.macro) && source_reflection.options[:through].nil?
         #   raise HasManyThroughSourceAssociationMacroError.new(self)
         # end

activerecord/test/cases/associations/nested_has_many_through_associations_test.rb

@@ -21,23 +21,23 @@
 class NestedHasManyThroughAssociationsTest < ActiveRecord::TestCase
   fixtures :authors, :books, :posts, :subscriptions, :subscribers, :tags, :taggings
 
-#  def test_has_many_through_a_has_many_through_association_on_source_reflection
-#    author = authors(:david)
-#    assert_equal [tags(:general), tags(:general)], author.tags
-#  end
+  def test_has_many_through_a_has_many_through_association_on_source_reflection
+    author = authors(:david)
+    assert_equal [tags(:general), tags(:general)], author.tags
+  end
 
   def test_has_many_through_a_has_many_through_association_on_through_reflection
     author = authors(:david)
     assert_equal [subscribers(:first), subscribers(:second), subscribers(:second)], author.subscribers
   end
 
-#  def test_distinct_has_many_through_a_has_many_through_association_on_source_reflection
-#    author = authors(:david)
-#    assert_equal [tags(:general)], author.distinct_tags
-#  end
+  def test_distinct_has_many_through_a_has_many_through_association_on_source_reflection
+    author = authors(:david)
+    assert_equal [tags(:general)], author.distinct_tags
+  end
 
-#  def test_distinct_has_many_through_a_has_many_through_association_on_through_reflection
-#    author = authors(:david)
-#    assert_equal [subscribers(:first), subscribers(:second)], author.distinct_subscribers
-#  end
+  def test_distinct_has_many_through_a_has_many_through_association_on_through_reflection
+    author = authors(:david)
+    assert_equal [subscribers(:first), subscribers(:second)], author.distinct_subscribers
+  end
 end