From 770e6893b9f2aaaebe3de10576931dc7194451bc Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Thu, 6 Jan 2011 18:04:32 +0000 Subject: 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. --- .../associations/association_collection.rb | 27 ++++------ .../associations/association_proxy.rb | 58 ++++++++++++++++------ .../associations/belongs_to_association.rb | 17 ------- .../has_and_belongs_to_many_association.rb | 16 +++--- .../active_record/associations/has_association.rb | 28 ----------- .../associations/has_many_association.rb | 4 +- .../associations/has_one_association.rb | 14 ++---- .../associations/through_association.rb | 21 +++----- .../associations/has_one_associations_test.rb | 8 --- activerecord/test/cases/reflection_test.rb | 4 +- activerecord/test/cases/relation_scoping_test.rb | 3 +- activerecord/test/models/company.rb | 1 - 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 => ' -- cgit v1.2.3