Construct an actual ActiveRecord::Relation object for the association scope, rather than a hash which is passed to apply_finder_options. This allows more flexibility in how the scope is created, for example because scope.where(a, b) and scope.where(a).where(b) mean different things.

8 files changed, 72 insertions, 113 deletions

diff --git a/activerecord/lib/active_record/associations/association_collection.rb b/activerecord/lib/active_record/associations/association_collection.rb
index bd27365b08..f33a9ce732 100644
--- a/activerecord/lib/active_record/associations/association_collection.rb
+++ b/activerecord/lib/active_record/associations/association_collection.rb
@@ -333,23 +333,16 @@ module ActiveRecord
 
       protected
 
-        def finder_options
-          {
-            :conditions => construct_conditions,
-            :select     => construct_select,
-            :readonly   => @reflection.options[:readonly],
-            :order      => @reflection.options[:order],
-            :limit      => @reflection.options[:limit],
-            :include    => @reflection.options[:include],
-            :joins      => @reflection.options[:joins],
-            :group      => @reflection.options[:group],
-            :having     => @reflection.options[:having],
-            :offset     => @reflection.options[:offset]
-          }
-        end
-
-        def construct_select
-          @reflection.options[:select] ||
+        def association_scope
+          options = @reflection.options.slice(:order, :limit, :joins, :group, :having, :offset)
+          super.apply_finder_options(options)
+        end
+
+        def select_value
+          super || uniq_select_value
+        end
+
+        def uniq_select_value
           @reflection.options[:uniq] && "DISTINCT #{@reflection.quoted_table_name}.*"
         end
 
diff --git a/activerecord/lib/active_record/associations/association_proxy.rb b/activerecord/lib/active_record/associations/association_proxy.rb
index 8e64056a06..405a0307c1 100644
--- a/activerecord/lib/active_record/associations/association_proxy.rb
+++ b/activerecord/lib/active_record/associations/association_proxy.rb
@@ -165,9 +165,7 @@ module ActiveRecord
       end
 
       def scoped
-        target_scope.
-          apply_finder_options(@finder_options).
-          create_with(@creation_attributes)
+        target_scope & @association_scope
       end
 
       protected
@@ -180,27 +178,32 @@ module ActiveRecord
           @reflection.klass.send(:sanitize_sql, sql, table_name)
         end
 
-        # Construct the data used for the scope for this association
+        # Construct the scope for this association.
         #
-        # Note that we don't actually build the scope here, we just construct the options and
-        # attributes. We must only build the scope when it's actually needed, because at that
-        # point the call may be surrounded by scope.scoping { ... } or with_scope { ... } etc,
-        # which affects the scope which actually gets built.
+        # Note that the association_scope is merged into the targed_scope only when the
+        # scoped method is called. This is because at that point the call may be surrounded
+        # by scope.scoping { ... } or with_scope { ... } etc, which affects the scope which
+        # actually gets built.
         def construct_scope
-          if target_klass
-            @finder_options      = finder_options
-            @creation_attributes = creation_attributes
-          end
+          @association_scope = association_scope if target_klass
+        end
+
+        def association_scope
+          scope = target_klass.unscoped
+          scope = scope.create_with(creation_attributes)
+          scope = scope.apply_finder_options(@reflection.options.slice(:conditions, :readonly, :include))
+          scope = scope.where(construct_owner_conditions)
+          scope = scope.select(select_value) if select_value = self.select_value
+          scope
         end
 
-        # Implemented by subclasses
-        def finder_options
-          raise NotImplementedError
+        def select_value
+          @reflection.options[:select]
         end
 
         # Implemented by (some) subclasses
         def creation_attributes
-          {}
+          { }
         end
 
         def aliased_table
@@ -226,6 +229,29 @@ module ActiveRecord
           target_klass.scoped
         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]
+          else
+            attributes[reflection.foreign_key] = @owner[reflection.active_record_primary_key]
+
+            if reflection.options[:as]
+              attributes["#{reflection.options[:as]}_type"] = @owner.class.base_class.name
+            end
+          end
+          attributes
+        end
+
+        # Builds an array of arel nodes from the owner attributes hash
+        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
+
       private
         # Forwards any missing method call to the \target.
         def method_missing(method, *args)
diff --git a/activerecord/lib/active_record/associations/belongs_to_association.rb b/activerecord/lib/active_record/associations/belongs_to_association.rb
index b50ee689cd..391471849c 100644
--- a/activerecord/lib/active_record/associations/belongs_to_association.rb
+++ b/activerecord/lib/active_record/associations/belongs_to_association.rb
@@ -58,23 +58,6 @@ module ActiveRecord
           scoped.first.tap { |record| set_inverse_instance(record) }
         end
 
-        def finder_options
-          {
-            :conditions => construct_conditions,
-            :select     => @reflection.options[:select],
-            :include    => @reflection.options[:include],
-            :readonly   => @reflection.options[:readonly]
-          }
-        end
-
-        def construct_conditions
-          conditions = aliased_table[@reflection.association_primary_key].
-                       eq(@owner[@reflection.foreign_key])
-
-          conditions = conditions.and(Arel.sql(sql_conditions)) if sql_conditions
-          conditions
-        end
-
         def foreign_key_present?
           !@owner[@reflection.foreign_key].nil?
         end
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 b18ec23037..bc7894173d 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
@@ -88,14 +88,14 @@ module ActiveRecord
           super(join_table)
         end
 
-        def finder_options
-          super.merge(
-            :joins    => construct_joins,
-            :readonly => ambiguous_select?(@reflection.options[:select]),
-            :select   => @reflection.options[:select] || [
-                @reflection.klass.arel_table[Arel.star],
-                join_table[Arel.star]]
-          )
+        def association_scope
+          scope = super.joins(construct_joins)
+          scope = scope.readonly if ambiguous_select?(@reflection.options[:select])
+          scope
+        end
+
+        def select_value
+          super || [@reflection.klass.arel_table[Arel.star], join_table[Arel.star]]
         end
 
         # Join tables with additional columns on top of the two foreign keys must be considered
diff --git a/activerecord/lib/active_record/associations/has_association.rb b/activerecord/lib/active_record/associations/has_association.rb
index 190fed77c2..8b180c7301 100644
--- a/activerecord/lib/active_record/associations/has_association.rb
+++ b/activerecord/lib/active_record/associations/has_association.rb
@@ -9,34 +9,6 @@ module ActiveRecord
             construct_owner_attributes.each { |key, value| record[key] = value }
           end
         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.send(reflection.foreign_key)
-          else
-            attributes[reflection.foreign_key] = @owner.send(reflection.active_record_primary_key)
-
-            if reflection.options[:as]
-              attributes["#{reflection.options[:as]}_type"] = @owner.class.base_class.name
-            end
-          end
-          attributes
-        end
-
-        # Builds an array of arel nodes from the owner attributes hash
-        def construct_owner_conditions(table = aliased_table, reflection = @reflection)
-          construct_owner_attributes(reflection).map do |attr, value|
-            table[attr].eq(value)
-          end
-        end
-
-        def construct_conditions
-          conditions = construct_owner_conditions
-          conditions << Arel.sql(sql_conditions) if sql_conditions
-          aliased_table.create_and(conditions)
-        end
     end
   end
 end
diff --git a/activerecord/lib/active_record/associations/has_many_association.rb b/activerecord/lib/active_record/associations/has_many_association.rb
index dba26a78da..b07441f3c6 100644
--- a/activerecord/lib/active_record/associations/has_many_association.rb
+++ b/activerecord/lib/active_record/associations/has_many_association.rb
@@ -69,9 +69,7 @@ module ActiveRecord
           end
         end
 
-        def creation_attributes
-          construct_owner_attributes
-        end
+        alias creation_attributes construct_owner_attributes
     end
   end
 end
diff --git a/activerecord/lib/active_record/associations/has_one_association.rb b/activerecord/lib/active_record/associations/has_one_association.rb
index f1e01197b5..5107be1eaf 100644
--- a/activerecord/lib/active_record/associations/has_one_association.rb
+++ b/activerecord/lib/active_record/associations/has_one_association.rb
@@ -68,19 +68,11 @@ module ActiveRecord
           scoped.first.tap { |record| set_inverse_instance(record) }
         end
 
-        def finder_options
-          {
-            :conditions => construct_conditions,
-            :select     => @reflection.options[:select],
-            :include    => @reflection.options[:include],
-            :readonly   => @reflection.options[:readonly],
-            :order      => @reflection.options[:order]
-          }
+        def association_scope
+          super.order(@reflection.options[:order])
         end
 
-        def creation_attributes
-          construct_owner_attributes
-        end
+        alias creation_attributes construct_owner_attributes
 
         def new_record
           record = scoped.scoping { yield @reflection }
diff --git a/activerecord/lib/active_record/associations/through_association.rb b/activerecord/lib/active_record/associations/through_association.rb
index 65c36419da..d2112fb2b6 100644
--- a/activerecord/lib/active_record/associations/through_association.rb
+++ b/activerecord/lib/active_record/associations/through_association.rb
@@ -9,12 +9,12 @@ module ActiveRecord
         super & @reflection.through_reflection.klass.scoped
       end
 
-      def finder_options
-        super.merge(
-          :joins   => construct_joins,
-          :include => @reflection.options[:include] ||
-                      @reflection.source_reflection.options[:include]
-        )
+      def association_scope
+        scope = super.joins(construct_joins).where(conditions)
+        unless @reflection.options[:include]
+          scope = scope.includes(@reflection.source_reflection.options[:include])
+        end
+        scope
       end
 
       # This scope affects the creation of the associated records (not the join records). At the
@@ -98,18 +98,13 @@ module ActiveRecord
       end
 
       def build_conditions
-        association_conditions = @reflection.options[:conditions]
         through_conditions = build_through_conditions
         source_conditions = @reflection.source_reflection.options[:conditions]
         uses_sti = !@reflection.through_reflection.klass.descends_from_active_record?
 
-        if association_conditions || through_conditions || source_conditions || uses_sti
+        if through_conditions || source_conditions || uses_sti
           all = []
-
-          [association_conditions, source_conditions].each do |conditions|
-            all << interpolate_sql(sanitize_sql(conditions)) if conditions
-          end
-
+          all << interpolate_sql(sanitize_sql(source_conditions)) if source_conditions
           all << through_conditions  if through_conditions
           all << build_sti_condition if uses_sti