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.

12 files changed, 76 insertions, 125 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
 
diff --git a/activerecord/test/cases/associations/has_one_associations_test.rb b/activerecord/test/cases/associations/has_one_associations_test.rb
index fa36b527a2..8203534a37 100644
--- a/activerecord/test/cases/associations/has_one_associations_test.rb
+++ b/activerecord/test/cases/associations/has_one_associations_test.rb
@@ -227,14 +227,6 @@ class HasOneAssociationsTest < ActiveRecord::TestCase
     firm.destroy
   end
 
-  def test_finding_with_interpolated_condition
-    firm = Firm.find(:first)
-    superior = firm.clients.create(:name => 'SuperiorCo')
-    superior.rating = 10
-    superior.save
-    assert_equal 10, firm.clients_with_interpolated_conditions.first.rating
-  end
-
   def test_assignment_before_child_saved
     firm = Firm.find(1)
     firm.account = a = Account.new("credit_limit" => 1000)
diff --git a/activerecord/test/cases/reflection_test.rb b/activerecord/test/cases/reflection_test.rb
index d75bc3982e..eb580928ba 100644
--- a/activerecord/test/cases/reflection_test.rb
+++ b/activerecord/test/cases/reflection_test.rb
@@ -181,8 +181,8 @@ class ReflectionTest < ActiveRecord::TestCase
 
   def test_reflection_of_all_associations
     # FIXME these assertions bust a lot
-    assert_equal 37, Firm.reflect_on_all_associations.size
-    assert_equal 27, Firm.reflect_on_all_associations(:has_many).size
+    assert_equal 36, Firm.reflect_on_all_associations.size
+    assert_equal 26, Firm.reflect_on_all_associations(:has_many).size
     assert_equal 10, Firm.reflect_on_all_associations(:has_one).size
     assert_equal 0, Firm.reflect_on_all_associations(:belongs_to).size
   end
diff --git a/activerecord/test/cases/relation_scoping_test.rb b/activerecord/test/cases/relation_scoping_test.rb
index bff0151f1a..1bdf3136d4 100644
--- a/activerecord/test/cases/relation_scoping_test.rb
+++ b/activerecord/test/cases/relation_scoping_test.rb
@@ -259,7 +259,8 @@ class HasManyScopingTest< ActiveRecord::TestCase
   end
 
   def test_should_default_scope_on_associations_is_overriden_by_association_conditions
-    assert_equal [], people(:michael).fixed_bad_references
+    reference = references(:michael_unicyclist).becomes(BadReference)
+    assert_equal [reference], people(:michael).fixed_bad_references
   end
 
   def test_should_maintain_default_scope_on_eager_loaded_associations
diff --git a/activerecord/test/models/company.rb b/activerecord/test/models/company.rb
index d08e593db1..f6e7a5ccf7 100644
--- a/activerecord/test/models/company.rb
+++ b/activerecord/test/models/company.rb
@@ -49,7 +49,6 @@ class Firm < Company
   has_many :exclusively_dependent_clients_of_firm, :foreign_key => "client_of", :class_name => "Client", :order => "id", :dependent => :delete_all
   has_many :limited_clients, :class_name => "Client", :limit => 1
   has_many :clients_like_ms, :conditions => "name = 'Microsoft'", :class_name => "Client", :order => "id"
-  has_many :clients_with_interpolated_conditions, :class_name => "Client", :conditions => 'rating > #{rating}'
   has_many :clients_like_ms_with_hash_conditions, :conditions => { :name => 'Microsoft' }, :class_name => "Client", :order => "id"
   has_many :clients_using_sql, :class_name => "Client", :finder_sql => 'SELECT * FROM companies WHERE client_of = #{id}'
   has_many :clients_using_multiline_sql, :class_name => "Client", :finder_sql => '