From ddf83d14f1c7ddae07a285a8ad7c45f652edc843 Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Sat, 5 Mar 2011 20:10:24 +0000 Subject: Add a test for STI on the through where the through is nested, and change the code which support this --- .../associations/through_association.rb | 35 ++++++++++++++-------- 1 file changed, 22 insertions(+), 13 deletions(-) (limited to 'activerecord/lib/active_record/associations/through_association.rb') diff --git a/activerecord/lib/active_record/associations/through_association.rb b/activerecord/lib/active_record/associations/through_association.rb index ed24373cba..8e9259a28d 100644 --- a/activerecord/lib/active_record/associations/through_association.rb +++ b/activerecord/lib/active_record/associations/through_association.rb @@ -10,8 +10,20 @@ module ActiveRecord protected + # We merge in these scopes for two reasons: + # + # 1. To get the scope_for_create on through reflection when building associated objects + # 2. To get the type conditions for any STI classes in the chain + # + # TODO: Don't actually do this. Getting the creation attributes for a non-nested through + # is a special case. The rest (STI conditions) should be handled by the reflection + # itself. def target_scope - super.merge(through_reflection.klass.scoped) + scope = super + through_reflection_chain[1..-1].each do |reflection| + scope = scope.merge(reflection.klass.scoped) + end + scope end def association_scope @@ -227,21 +239,18 @@ module ActiveRecord def reflection_conditions(index) reflection = through_reflection_chain[index] - conditions = through_conditions[index].dup - - # TODO: maybe this should go in Reflection#through_conditions directly? - unless reflection.klass.descends_from_active_record? - conditions << reflection.klass.send(:type_condition) - end + conditions = through_conditions[index] unless conditions.empty? - 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 + Arel::Nodes::And.new(process_conditions(conditions, reflection)) + end + end - Arel::Nodes::And.new(conditions) + 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 -- cgit v1.2.3 From 7fddb942624478a23173dfa379f7ade6a0fc9218 Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Sat, 5 Mar 2011 22:07:30 +0000 Subject: Push source_type and polymorphic conditions out of ThroughAssociation and JoinDependency::JoinAssociation and into the reflection instead. --- .../active_record/associations/through_association.rb | 17 ----------------- 1 file changed, 17 deletions(-) (limited to 'activerecord/lib/active_record/associations/through_association.rb') diff --git a/activerecord/lib/active_record/associations/through_association.rb b/activerecord/lib/active_record/associations/through_association.rb index 8e9259a28d..11263d5def 100644 --- a/activerecord/lib/active_record/associations/through_association.rb +++ b/activerecord/lib/active_record/associations/through_association.rb @@ -89,7 +89,6 @@ module ActiveRecord right_table, left_table[left.foreign_key], right_table[right.association_primary_key], - polymorphic_conditions(left, left), reflection_conditions(right_index) ) when :has_and_belongs_to_many @@ -107,7 +106,6 @@ module ActiveRecord right_table, left_table[left.association_primary_key], right_table[left.foreign_key], - source_type_conditions(left), reflection_conditions(right_index) ) when :has_many, :has_one @@ -119,7 +117,6 @@ module ActiveRecord right_table, left_table[left.foreign_key], right_table[left.source_reflection.active_record_primary_key], - polymorphic_conditions(left, left.source_reflection), reflection_conditions(right_index) ) @@ -254,20 +251,6 @@ module ActiveRecord end end - def polymorphic_conditions(reflection, polymorphic_reflection) - if polymorphic_reflection.options[:as] - tables[reflection][polymorphic_reflection.type]. - eq(polymorphic_reflection.active_record.base_class.name) - end - end - - def source_type_conditions(reflection) - if reflection.options[:source_type] - tables[reflection.through_reflection][reflection.foreign_type]. - eq(reflection.options[:source_type]) - end - end - # TODO: Think about this in the context of nested associations def stale_state if through_reflection.macro == :belongs_to -- cgit v1.2.3 From d02c326a8b5fdcfdb28ba91ee4dbc9c9edf6bf18 Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Sun, 6 Mar 2011 18:14:39 +0000 Subject: Refactor ThroughAssociation#tables to just be a flat array of tables in the order that they should be joined together. --- .../associations/through_association.rb | 145 ++++++++++----------- 1 file changed, 70 insertions(+), 75 deletions(-) (limited to 'activerecord/lib/active_record/associations/through_association.rb') diff --git a/activerecord/lib/active_record/associations/through_association.rb b/activerecord/lib/active_record/associations/through_association.rb index 11263d5def..0857d8e253 100644 --- a/activerecord/lib/active_record/associations/through_association.rb +++ b/activerecord/lib/active_record/associations/through_association.rb @@ -27,7 +27,7 @@ module ActiveRecord end def association_scope - scope = super.joins(construct_joins) + scope = join_to(super) scope = scope.where(reflection_conditions(0)) unless options[:include] @@ -57,98 +57,98 @@ module ActiveRecord end def construct_owner_conditions - reflection = through_reflection_chain.last - - if reflection.macro == :has_and_belongs_to_many - table = tables[reflection].first - else - table = Array.wrap(tables[reflection]).first - end - - super(table, reflection) + super(tables.last, through_reflection_chain.last) end - def construct_joins - joins, right_index = [], 1 + def join_to(scope) + joins = [] + tables = tables().dup # FIXME: Ugly - # Iterate over each pair in the through reflection chain, joining them together - through_reflection_chain.each_cons(2) do |left, right| - left_table, right_table = tables[left], tables[right] + foreign_reflection = through_reflection_chain.first + foreign_table = tables.shift - if left.source_reflection.nil? - case left.macro + through_reflection_chain[1..-1].each_with_index do |reflection, i| + i += 1 + table = tables.shift + + if foreign_reflection.source_reflection.nil? + case foreign_reflection.macro when :belongs_to joins << inner_join( - right_table, - left_table[left.association_primary_key], - right_table[left.foreign_key], - reflection_conditions(right_index) + table, + foreign_table[foreign_reflection.association_primary_key], + table[foreign_reflection.foreign_key], + reflection_conditions(i) ) when :has_many, :has_one joins << inner_join( - right_table, - left_table[left.foreign_key], - right_table[right.association_primary_key], - reflection_conditions(right_index) + table, + foreign_table[foreign_reflection.foreign_key], + table[reflection.association_primary_key], + reflection_conditions(i) ) when :has_and_belongs_to_many + join_table = foreign_table + joins << inner_join( - right_table, - left_table.first[left.foreign_key], - right_table[right.klass.primary_key], - reflection_conditions(right_index) + table, + join_table[foreign_reflection.foreign_key], + table[reflection.klass.primary_key], + reflection_conditions(i) ) end else - case left.source_reflection.macro + case foreign_reflection.source_reflection.macro when :belongs_to joins << inner_join( - right_table, - left_table[left.association_primary_key], - right_table[left.foreign_key], - reflection_conditions(right_index) + table, + foreign_table[foreign_reflection.association_primary_key], + table[foreign_reflection.foreign_key], + reflection_conditions(i) ) when :has_many, :has_one - if right.macro == :has_and_belongs_to_many - join_table, right_table = tables[right] - end - joins << inner_join( - right_table, - left_table[left.foreign_key], - right_table[left.source_reflection.active_record_primary_key], - reflection_conditions(right_index) + table, + foreign_table[foreign_reflection.foreign_key], + table[foreign_reflection.source_reflection.active_record_primary_key], + reflection_conditions(i) ) - if right.macro == :has_and_belongs_to_many + if reflection.macro == :has_and_belongs_to_many + join_table = tables.shift + joins << inner_join( join_table, - right_table[right.klass.primary_key], - join_table[right.association_foreign_key] + table[reflection.klass.primary_key], + join_table[reflection.association_foreign_key] ) + + # hack to make it become the foreign_table + table = join_table end when :has_and_belongs_to_many - join_table, left_table = tables[left] + join_table, table = table, tables.shift joins << inner_join( join_table, - left_table[left.klass.primary_key], - join_table[left.association_foreign_key] + foreign_table[foreign_reflection.klass.primary_key], + join_table[foreign_reflection.association_foreign_key] ) joins << inner_join( - right_table, - join_table[left.foreign_key], - right_table[right.klass.primary_key], - reflection_conditions(right_index) + table, + join_table[foreign_reflection.foreign_key], + table[reflection.klass.primary_key], + reflection_conditions(i) ) end end - right_index += 1 + foreign_reflection = reflection + foreign_table = table end - joins + scope.joins(joins) end # Construct attributes for :through pointing to owner and associate. This is used by the @@ -191,31 +191,26 @@ module ActiveRecord @alias_tracker ||= AliasTracker.new end - # TODO: It is decidedly icky to have an array for habtm entries, and no array for others def tables @tables ||= begin - Hash[ - through_reflection_chain.map do |reflection| - table = alias_tracker.aliased_table_for( - reflection.table_name, - table_alias_for(reflection, reflection != self.reflection) + 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) ) - - if reflection.macro == :has_and_belongs_to_many || - (reflection.source_reflection && - reflection.source_reflection.macro == :has_and_belongs_to_many) - - join_table = alias_tracker.aliased_table_for( - (reflection.source_reflection || reflection).options[:join_table], - table_alias_for(reflection, true) - ) - - [reflection, [join_table, table]] - else - [reflection, table] - end end - ] + end + tables end end -- cgit v1.2.3 From 5dc1fb39dde569c2633e067cdc895ddc56dd3482 Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Sun, 6 Mar 2011 23:38:10 +0000 Subject: Refactor ThroughAssociation#join_to to be much smaller, and independent of construct_owner_conditions. --- .../associations/through_association.rb | 112 ++++++--------------- 1 file changed, 28 insertions(+), 84 deletions(-) (limited to 'activerecord/lib/active_record/associations/through_association.rb') diff --git a/activerecord/lib/active_record/associations/through_association.rb b/activerecord/lib/active_record/associations/through_association.rb index 0857d8e253..5768915eaf 100644 --- a/activerecord/lib/active_record/associations/through_association.rb +++ b/activerecord/lib/active_record/associations/through_association.rb @@ -28,7 +28,6 @@ module ActiveRecord def association_scope scope = join_to(super) - scope = scope.where(reflection_conditions(0)) unless options[:include] scope = scope.includes(source_options[:include]) @@ -57,95 +56,42 @@ module ActiveRecord end def construct_owner_conditions - super(tables.last, through_reflection_chain.last) end def join_to(scope) joins = [] tables = tables().dup # FIXME: Ugly - foreign_reflection = through_reflection_chain.first - foreign_table = tables.shift - - through_reflection_chain[1..-1].each_with_index do |reflection, i| - i += 1 - table = tables.shift - - if foreign_reflection.source_reflection.nil? - case foreign_reflection.macro - when :belongs_to - joins << inner_join( - table, - foreign_table[foreign_reflection.association_primary_key], - table[foreign_reflection.foreign_key], - reflection_conditions(i) - ) - when :has_many, :has_one - joins << inner_join( - table, - foreign_table[foreign_reflection.foreign_key], - table[reflection.association_primary_key], - reflection_conditions(i) - ) - when :has_and_belongs_to_many - join_table = foreign_table - - joins << inner_join( - table, - join_table[foreign_reflection.foreign_key], - table[reflection.klass.primary_key], - reflection_conditions(i) - ) - end + 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 - case foreign_reflection.source_reflection.macro - when :belongs_to - joins << inner_join( - table, - foreign_table[foreign_reflection.association_primary_key], - table[foreign_reflection.foreign_key], - reflection_conditions(i) - ) - when :has_many, :has_one - joins << inner_join( - table, - foreign_table[foreign_reflection.foreign_key], - table[foreign_reflection.source_reflection.active_record_primary_key], - reflection_conditions(i) - ) - - if reflection.macro == :has_and_belongs_to_many - join_table = tables.shift - - joins << inner_join( - join_table, - table[reflection.klass.primary_key], - join_table[reflection.association_foreign_key] - ) - - # hack to make it become the foreign_table - table = join_table - end - when :has_and_belongs_to_many - join_table, table = table, tables.shift - - joins << inner_join( - join_table, - foreign_table[foreign_reflection.klass.primary_key], - join_table[foreign_reflection.association_foreign_key] - ) - - joins << inner_join( - table, - join_table[foreign_reflection.foreign_key], - table[reflection.klass.primary_key], - reflection_conditions(i) - ) - end + key = reflection.foreign_key + foreign_key = reflection.association_primary_key end - foreign_reflection = reflection - foreign_table = table + 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) @@ -221,9 +167,7 @@ module ActiveRecord name end - def inner_join(table, left_column, right_column, *conditions) - conditions << left_column.eq(right_column) - + def inner_join(table, *conditions) table.create_join( table, table.create_on(table.create_and(conditions.flatten.compact))) -- cgit v1.2.3 From bb063b2f1b3e5b6fb2a4732cb696929f1652c555 Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Mon, 7 Mar 2011 20:58:32 +0000 Subject: Fix test_has_many_association_through_a_has_many_association_with_nonstandard_primary_keys --- activerecord/lib/active_record/associations/through_association.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'activerecord/lib/active_record/associations/through_association.rb') diff --git a/activerecord/lib/active_record/associations/through_association.rb b/activerecord/lib/active_record/associations/through_association.rb index 5768915eaf..ae2c8b65ed 100644 --- a/activerecord/lib/active_record/associations/through_association.rb +++ b/activerecord/lib/active_record/associations/through_association.rb @@ -82,7 +82,7 @@ module ActiveRecord foreign_key = reflection.foreign_key else key = reflection.foreign_key - foreign_key = reflection.association_primary_key + foreign_key = reflection.active_record_primary_key end if reflection == through_reflection_chain.last -- cgit v1.2.3 From 6490d65234b89d4d28308b72b13d4834fd44bbb3 Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Thu, 10 Mar 2011 19:04:00 +0000 Subject: 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. --- .../associations/through_association.rb | 128 --------------------- 1 file changed, 128 deletions(-) (limited to 'activerecord/lib/active_record/associations/through_association.rb') diff --git a/activerecord/lib/active_record/associations/through_association.rb b/activerecord/lib/active_record/associations/through_association.rb index ae2c8b65ed..408237c689 100644 --- a/activerecord/lib/active_record/associations/through_association.rb +++ b/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 -- cgit v1.2.3 From 2d3d9e3531d0d49a94ded10b993640053bd76c03 Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Thu, 10 Mar 2011 19:28:26 +0000 Subject: Rename Reflection#through_reflection_chain and #through_options to Reflection#chain and Reflection#options as they now no longer relate solely to through associations. --- activerecord/lib/active_record/associations/through_association.rb | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) (limited to 'activerecord/lib/active_record/associations/through_association.rb') diff --git a/activerecord/lib/active_record/associations/through_association.rb b/activerecord/lib/active_record/associations/through_association.rb index 408237c689..c0c92e8d72 100644 --- a/activerecord/lib/active_record/associations/through_association.rb +++ b/activerecord/lib/active_record/associations/through_association.rb @@ -3,8 +3,7 @@ module ActiveRecord module Associations module ThroughAssociation #:nodoc: - delegate :source_options, :through_options, :source_reflection, :through_reflection, - :through_reflection_chain, :through_conditions, :to => :reflection + delegate :source_reflection, :through_reflection, :chain, :to => :reflection protected @@ -18,7 +17,7 @@ module ActiveRecord # itself. def target_scope scope = super - through_reflection_chain[1..-1].each do |reflection| + chain[1..-1].each do |reflection| scope = scope.merge(reflection.klass.scoped) end scope -- cgit v1.2.3 From 02a43f9f4585d3c932e12b60ef23543f9c534a2e Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Sat, 12 Mar 2011 08:42:57 +0000 Subject: Resolve some TODO comments which I decided did not need anything done --- .../lib/active_record/associations/through_association.rb | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) (limited to 'activerecord/lib/active_record/associations/through_association.rb') diff --git a/activerecord/lib/active_record/associations/through_association.rb b/activerecord/lib/active_record/associations/through_association.rb index c0c92e8d72..e6ab628719 100644 --- a/activerecord/lib/active_record/associations/through_association.rb +++ b/activerecord/lib/active_record/associations/through_association.rb @@ -9,12 +9,8 @@ module ActiveRecord # We merge in these scopes for two reasons: # - # 1. To get the scope_for_create on through reflection when building associated objects - # 2. To get the type conditions for any STI classes in the chain - # - # TODO: Don't actually do this. Getting the creation attributes for a non-nested through - # is a special case. The rest (STI conditions) should be handled by the reflection - # itself. + # 1. To get the default_scope conditions for any of the other reflections in the chain + # 2. To get the type conditions for any STI models in the chain def target_scope scope = super chain[1..-1].each do |reflection| @@ -61,7 +57,8 @@ module ActiveRecord end end - # TODO: Think about this in the context of nested associations + # Note: this does not capture all cases, for example it would be crazy to try to + # properly support stale-checking for nested associations. def stale_state if through_reflection.macro == :belongs_to owner[through_reflection.foreign_key].to_s -- cgit v1.2.3