Move the code which builds a scope for through associations into a generic AssociationScope class which is capable of building a scope for any association.

activerecord/lib/active_record/associations.rb

@@ -140,9 +140,10 @@ module ActiveRecord
       autoload :HasAndBelongsToMany, 'active_record/associations/builder/has_and_belongs_to_many'
     end
 
-    autoload :Preloader,      'active_record/associations/preloader'
+    autoload :Preloader,        'active_record/associations/preloader'
-    autoload :JoinDependency, 'active_record/associations/join_dependency'
+    autoload :JoinDependency,   'active_record/associations/join_dependency'
-    autoload :AliasTracker,   'active_record/associations/alias_tracker'
+    autoload :AssociationScope, 'active_record/associations/association_scope'
+    autoload :AliasTracker,     'active_record/associations/alias_tracker'
 
     # Clears out the association cache.
     def clear_association_cache #:nodoc:

activerecord/lib/active_record/associations/association.rb

@@ -93,23 +93,9 @@ module ActiveRecord
       # by scope.scoping { ... } or with_scope { ... } etc, which affects the scope which
       # actually gets built.
       def construct_scope
-        @association_scope = association_scope if klass
+        if klass
-      end
+          @association_scope = AssociationScope.new(self).scope
-
-      def association_scope
-        scope = klass.unscoped
-        scope = scope.create_with(creation_attributes)
-        scope = scope.apply_finder_options(options.slice(:readonly, :include))
-        scope = scope.where(interpolate(options[:conditions]))
-        if select = select_value
-          scope = scope.select(select)
         end
-        scope = scope.extending(*Array.wrap(options[:extend]))
-        scope.where(construct_owner_conditions)
-      end
-
-      def aliased_table
-        klass.arel_table
       end
 
       # Set the inverse association, if possible
@@ -174,42 +160,24 @@ module ActiveRecord
           end
         end
 
-        def select_value
-          options[:select]
-        end
-
-        # Implemented by (some) subclasses
         def creation_attributes
-          { }
-        end
-
-        # Returns a hash linking the owner to the association represented by the reflection
-        def construct_owner_attributes(reflection = reflection)
           attributes = {}
-          if reflection.macro == :belongs_to
+
-            attributes[reflection.association_primary_key] = owner[reflection.foreign_key]
+          if [:has_one, :has_many].include?(reflection.macro) && !options[:through]
-          else
             attributes[reflection.foreign_key] = owner[reflection.active_record_primary_key]
 
             if reflection.options[:as]
               attributes[reflection.type] = owner.class.base_class.name
             end
           end
-          attributes
-        end
 
-        # Builds an array of arel nodes from the owner attributes hash
+          attributes
-        def construct_owner_conditions(table = aliased_table, reflection = reflection)
-          conditions = construct_owner_attributes(reflection).map do |attr, value|
-            table[attr].eq(value)
-          end
-          table.create_and(conditions)
         end
 
         # Sets the owner attributes on the given record
         def set_owner_attributes(record)
           if owner.persisted?
-            construct_owner_attributes.each { |key, value| record[key] = value }
+            creation_attributes.each { |key, value| record[key] = value }
           end
         end
 

activerecord/lib/active_record/associations/association_scope.rb

@@ -0,0 +1,149 @@
+module ActiveRecord
+  module Associations
+    class AssociationScope #:nodoc:
+      attr_reader :association, :alias_tracker
+
+      delegate :klass, :owner, :reflection, :interpolate, :to => :association
+      delegate :through_reflection_chain, :through_conditions, :options, :source_options, :to => :reflection
+
+      def initialize(association)
+        @association   = association
+        @alias_tracker = AliasTracker.new
+      end
+
+      def scope
+        scope = klass.unscoped
+        scope = scope.extending(*Array.wrap(options[:extend]))
+
+        # It's okay to just apply all these like this. The options will only be present if the
+        # association supports that option; this is enforced by the association builder.
+        scope = scope.apply_finder_options(options.slice(
+          :readonly, :include, :order, :limit, :joins, :group, :having, :offset))
+
+        if options[:through] && !options[:include]
+          scope = scope.includes(source_options[:include])
+        end
+
+        if select = select_value
+          scope = scope.select(select)
+        end
+
+        add_constraints(scope)
+      end
+
+      private
+
+      def select_value
+        select_value = options[:select]
+
+        if reflection.collection?
+          select_value ||= options[:uniq] && "DISTINCT #{reflection.quoted_table_name}.*"
+        end
+
+        if reflection.macro == :has_and_belongs_to_many
+          select_value ||= reflection.klass.arel_table[Arel.star]
+        end
+
+        select_value
+      end
+
+      def add_constraints(scope)
+        tables = construct_tables
+
+        through_reflection_chain.each_with_index do |reflection, i|
+          table, foreign_table = tables.shift, tables.first
+
+          if reflection.source_macro == :has_and_belongs_to_many
+            join_table = tables.shift
+
+            scope = scope.joins(inner_join(
+              join_table, reflection,
+              table[reflection.active_record_primary_key].
+                eq(join_table[reflection.association_foreign_key])
+            ))
+
+            table, foreign_table = join_table, tables.first
+          end
+
+          if reflection.source_macro == :belongs_to
+            key         = reflection.association_primary_key
+            foreign_key = reflection.foreign_key
+          else
+            key         = reflection.foreign_key
+            foreign_key = reflection.active_record_primary_key
+          end
+
+          if reflection == through_reflection_chain.last
+            scope = scope.where(table[key].eq(owner[foreign_key]))
+
+            through_conditions[i].each do |condition|
+              if options[:through] && condition.is_a?(Hash)
+                condition = { table.name => condition }
+              end
+
+              scope = scope.where(interpolate(condition))
+            end
+          else
+            constraint = table[key].eq foreign_table[foreign_key]
+
+            join  = inner_join(foreign_table, reflection, constraint, *through_conditions[i])
+            scope = scope.joins(join)
+          end
+        end
+
+        scope
+      end
+
+      def construct_tables
+        tables = []
+        through_reflection_chain.each do |reflection|
+          tables << alias_tracker.aliased_table_for(
+            table_name_for(reflection),
+            table_alias_for(reflection, reflection != self.reflection)
+          )
+
+          if reflection.source_macro == :has_and_belongs_to_many
+            tables << alias_tracker.aliased_table_for(
+              (reflection.source_reflection || reflection).options[:join_table],
+              table_alias_for(reflection, true)
+            )
+          end
+        end
+        tables
+      end
+
+      def table_name_for(reflection)
+        if reflection == self.reflection
+          # If this is a polymorphic belongs_to, we want to get the klass from the
+          # association because it depends on the polymorphic_type attribute of
+          # the owner
+          klass.table_name
+        else
+          reflection.table_name
+        end
+      end
+
+      def table_alias_for(reflection, join = false)
+        name = alias_tracker.pluralize(reflection.name)
+        name << "_#{self.reflection.name}"
+        name << "_join" if join
+        name
+      end
+
+      def inner_join(table, reflection, *conditions)
+        conditions = sanitize_conditions(reflection, conditions)
+        table.create_join(table, table.create_on(conditions))
+      end
+
+      def sanitize_conditions(reflection, conditions)
+        conditions = conditions.map do |condition|
+          condition = reflection.klass.send(:sanitize_sql, interpolate(condition), reflection.table_name)
+          condition = Arel.sql(condition) unless condition.is_a?(Arel::Node)
+          condition
+        end
+
+        conditions.length == 1 ? conditions.first : Arel::Nodes::And.new(conditions)
+      end
+    end
+  end
+end

activerecord/lib/active_record/associations/collection_association.rb

@@ -331,11 +331,6 @@ module ActiveRecord
         @scopes_cache[method][args] ||= scoped.readonly(nil).send(method, *args)
       end
 
-      def association_scope
-        options = reflection.options.slice(:order, :limit, :joins, :group, :having, :offset)
-        super.apply_finder_options(options)
-      end
-
       def load_target
         if find_target?
           targets = []
@@ -373,14 +368,6 @@ module ActiveRecord
 
       private
 
-        def select_value
-          super || uniq_select_value
-        end
-
-        def uniq_select_value
-          options[:uniq] && "DISTINCT #{reflection.quoted_table_name}.*"
-        end
-
         def custom_counter_sql
           if options[:counter_sql]
             interpolate(options[:counter_sql])

activerecord/lib/active_record/associations/has_and_belongs_to_many_association.rb

@@ -26,10 +26,6 @@ module ActiveRecord
         record
       end
 
-      def association_scope
-        super.joins(construct_joins)
-      end
-
       private
 
         def count_records
@@ -48,24 +44,6 @@ module ActiveRecord
           end
         end
 
-        def construct_joins
-          right = join_table
-          left  = reflection.klass.arel_table
-
-          condition = left[reflection.klass.primary_key].eq(
-            right[reflection.association_foreign_key])
-
-          right.create_join(right, right.create_on(condition))
-        end
-
-        def construct_owner_conditions
-          super(join_table)
-        end
-
-        def select_value
-          super || reflection.klass.arel_table[Arel.star]
-        end
-
         def invertible_for?(record)
           false
         end

activerecord/lib/active_record/associations/has_many_association.rb

@@ -94,8 +94,6 @@ module ActiveRecord
             end
           end
         end
-
-        alias creation_attributes construct_owner_attributes
     end
   end
 end

activerecord/lib/active_record/associations/has_one_association.rb

@@ -39,14 +39,8 @@ module ActiveRecord
         end
       end
 
-      def association_scope
-        super.order(options[:order])
-      end
-
       private
 
-        alias creation_attributes construct_owner_attributes
-
         # The reason that the save param for replace is false, if for create (not just build),
         # is because the setting of the foreign keys is actually handled by the scoping when
         # the record is instantiated, and so they are set straight away and do not need to be

activerecord/lib/active_record/associations/through_association.rb

@@ -1,5 +1,3 @@
-require 'enumerator'
-
 module ActiveRecord
   # = Active Record Through Association
   module Associations
@@ -26,77 +24,8 @@ module ActiveRecord
           scope
         end
 
-        def association_scope
-          scope = join_to(super)
-
-          unless options[:include]
-            scope = scope.includes(source_options[:include])
-          end
-
-          scope
-        end
-
       private
 
-        # This scope affects the creation of the associated records (not the join records). At the
-        # moment we only support creating on a :through association when the source reflection is a
-        # belongs_to. Thus it's not necessary to set a foreign key on the associated record(s), so
-        # this scope has can legitimately be empty.
-        def creation_attributes
-          { }
-        end
-
-        # TODO: Needed?
-        def aliased_through_table
-          name = through_reflection.table_name
-
-          reflection.table_name == name ?
-            through_reflection.klass.arel_table.alias(name + "_join") :
-            through_reflection.klass.arel_table
-        end
-
-        def construct_owner_conditions
-        end
-
-        def join_to(scope)
-          joins  = []
-          tables = tables().dup # FIXME: Ugly
-
-          through_reflection_chain.each_with_index do |reflection, i|
-            table, foreign_table = tables.shift, tables.first
-
-            if reflection.source_macro == :has_and_belongs_to_many
-              join_table = tables.shift
-
-              joins << inner_join(
-                join_table,
-                table[reflection.active_record_primary_key].
-                  eq(join_table[reflection.association_foreign_key])
-              )
-
-              table, foreign_table = join_table, tables.first
-            end
-
-            if reflection.source_macro == :belongs_to
-              key         = reflection.association_primary_key
-              foreign_key = reflection.foreign_key
-            else
-              key         = reflection.foreign_key
-              foreign_key = reflection.active_record_primary_key
-            end
-
-            if reflection == through_reflection_chain.last
-              constraint = table[key].eq owner[foreign_key]
-              scope = scope.where(constraint).where(reflection_conditions(i))
-            else
-              constraint = table[key].eq foreign_table[foreign_key]
-              joins << inner_join(foreign_table, constraint, reflection_conditions(i))
-            end
-          end
-
-          scope.joins(joins)
-        end
-
         # Construct attributes for :through pointing to owner and associate. This is used by the
         # methods which create and delete records on the association.
         #
@@ -133,63 +62,6 @@ module ActiveRecord
           end
         end
 
-        def alias_tracker
-          @alias_tracker ||= AliasTracker.new
-        end
-
-        def tables
-          @tables ||= begin
-            tables = []
-            through_reflection_chain.each do |reflection|
-              tables << alias_tracker.aliased_table_for(
-                reflection.table_name,
-                table_alias_for(reflection, reflection != self.reflection)
-              )
-
-              if reflection.macro == :has_and_belongs_to_many ||
-                   (reflection.source_reflection &&
-                    reflection.source_reflection.macro == :has_and_belongs_to_many)
-
-                tables << alias_tracker.aliased_table_for(
-                  (reflection.source_reflection || reflection).options[:join_table],
-                  table_alias_for(reflection, true)
-                )
-              end
-            end
-            tables
-          end
-        end
-
-        def table_alias_for(reflection, join = false)
-          name = alias_tracker.pluralize(reflection.name)
-          name << "_#{self.reflection.name}"
-          name << "_join" if join
-          name
-        end
-
-        def inner_join(table, *conditions)
-          table.create_join(
-            table,
-            table.create_on(table.create_and(conditions.flatten.compact)))
-        end
-
-        def reflection_conditions(index)
-          reflection = through_reflection_chain[index]
-          conditions = through_conditions[index]
-
-          unless conditions.empty?
-            Arel::Nodes::And.new(process_conditions(conditions, reflection))
-          end
-        end
-
-        def process_conditions(conditions, reflection)
-          conditions.map do |condition|
-            condition = reflection.klass.send(:sanitize_sql, interpolate(condition), reflection.table_name)
-            condition = Arel.sql(condition) unless condition.is_a?(Arel::Node)
-            condition
-          end
-        end
-
         # TODO: Think about this in the context of nested associations
         def stale_state
           if through_reflection.macro == :belongs_to

activerecord/lib/active_record/reflection.rb

@@ -270,9 +270,9 @@ module ActiveRecord
       end
 
       def through_conditions
-        through_conditions = [Array.wrap(options[:conditions])]
+        conditions = [options[:conditions]].compact
-        through_conditions.first << { type => active_record.base_class.name } if options[:as]
+        conditions << { type => active_record.base_class.name } if options[:as]
-        through_conditions
+        [conditions]
       end
 
       def source_reflection