From b689834bcf2730353d066277f43047f10abb8d30 Mon Sep 17 00:00:00 2001 From: Bodaniel Jeanes Date: Sun, 26 Sep 2010 22:17:18 +1000 Subject: Initial nested_has_many_through support [#1152] --- activerecord/lib/active_record/associations.rb | 1 + .../associations/has_many_through_association.rb | 2 + .../associations/nested_has_many_through.rb | 156 +++++++++++++++++++++ activerecord/lib/active_record/reflection.rb | 6 +- 4 files changed, 162 insertions(+), 3 deletions(-) create mode 100644 activerecord/lib/active_record/associations/nested_has_many_through.rb (limited to 'activerecord/lib/active_record') diff --git a/activerecord/lib/active_record/associations.rb b/activerecord/lib/active_record/associations.rb index 565ebf8197..812abf5a55 100644 --- a/activerecord/lib/active_record/associations.rb +++ b/activerecord/lib/active_record/associations.rb @@ -111,6 +111,7 @@ module ActiveRecord autoload :HasAndBelongsToManyAssociation, 'active_record/associations/has_and_belongs_to_many_association' autoload :HasManyAssociation, 'active_record/associations/has_many_association' autoload :HasManyThroughAssociation, 'active_record/associations/has_many_through_association' + autoload :NestedHasManyThroughAssociation, 'active_record/associations/nested_has_many_through_association' autoload :HasOneAssociation, 'active_record/associations/has_one_association' autoload :HasOneThroughAssociation, 'active_record/associations/has_one_through_association' diff --git a/activerecord/lib/active_record/associations/has_many_through_association.rb b/activerecord/lib/active_record/associations/has_many_through_association.rb index 97883d8393..964c381c0d 100644 --- a/activerecord/lib/active_record/associations/has_many_through_association.rb +++ b/activerecord/lib/active_record/associations/has_many_through_association.rb @@ -1,4 +1,5 @@ require "active_record/associations/through_association_scope" +require "active_record/associations/nested_has_many_through" require 'active_support/core_ext/object/blank' module ActiveRecord @@ -6,6 +7,7 @@ module ActiveRecord module Associations class HasManyThroughAssociation < HasManyAssociation #:nodoc: include ThroughAssociationScope + include NestedHasManyThrough alias_method :new, :build diff --git a/activerecord/lib/active_record/associations/nested_has_many_through.rb b/activerecord/lib/active_record/associations/nested_has_many_through.rb new file mode 100644 index 0000000000..2d03b81128 --- /dev/null +++ b/activerecord/lib/active_record/associations/nested_has_many_through.rb @@ -0,0 +1,156 @@ +module ActiveRecord + module Associations + module NestedHasManyThrough + def self.included(klass) + klass.alias_method_chain :construct_conditions, :nesting + klass.alias_method_chain :construct_joins, :nesting + end + + def construct_joins_with_nesting(custom_joins = nil) + if nested? + @nested_join_attributes ||= construct_nested_join_attributes + "#{construct_nested_join_attributes[:joins]} #{@reflection.options[:joins]} #{custom_joins}" + else + construct_joins_without_nesting(custom_joins) + end + end + + def construct_conditions_with_nesting + if nested? + @nested_join_attributes ||= construct_nested_join_attributes + if @reflection.through_reflection && @reflection.through_reflection.macro == :belongs_to + "#{@nested_join_attributes[:remote_key]} = #{belongs_to_quoted_key} #{@nested_join_attributes[:conditions]}" + else + "#{@nested_join_attributes[:remote_key]} = #{@owner.quoted_id} #{@nested_join_attributes[:conditions]}" + end + else + construct_conditions_without_nesting + end + end + + protected + + # Given any belongs_to or has_many (including has_many :through) association, + # return the essential components of a join corresponding to that association, namely: + # + # * :joins: any additional joins required to get from the association's table + # (reflection.table_name) to the table that's actually joining to the active record's table + # * :remote_key: the name of the key in the join table (qualified by table name) which will join + # to a field of the active record's table + # * :local_key: the name of the key in the local table (not qualified by table name) which will + # take part in the join + # * :conditions: any additional conditions (e.g. filtering by type for a polymorphic association, + # or a :conditions clause explicitly given in the association), including a leading AND + def construct_nested_join_attributes(reflection = @reflection, association_class = reflection.klass, table_ids = {association_class.table_name => 1}) + if (reflection.macro == :has_many || reflection.macro == :has_one) && reflection.through_reflection + construct_has_many_through_attributes(reflection, table_ids) + else + construct_has_many_or_belongs_to_attributes(reflection, association_class, table_ids) + end + end + + def construct_has_many_through_attributes(reflection, table_ids) + # Construct the join components of the source association, so that we have a path from + # the eventual target table of the association up to the table named in :through, and + # all tables involved are allocated table IDs. + source_attrs = construct_nested_join_attributes(reflection.source_reflection, reflection.klass, table_ids) + + # Determine the alias of the :through table; this will be the last table assigned + # when constructing the source join components above. + through_table_alias = through_table_name = reflection.through_reflection.table_name + through_table_alias += "_#{table_ids[through_table_name]}" unless table_ids[through_table_name] == 1 + + # Construct the join components of the through association, so that we have a path to + # the active record's table. + through_attrs = construct_nested_join_attributes(reflection.through_reflection, reflection.through_reflection.klass, table_ids) + + # Any subsequent joins / filters on owner attributes will act on the through association, + # so that's what we return for the conditions/keys of the overall association. + conditions = through_attrs[:conditions] + conditions += " AND #{interpolate_sql(reflection.klass.send(:sanitize_sql, reflection.options[:conditions]))}" if reflection.options[:conditions] + + { + :joins => "%s INNER JOIN %s ON ( %s = %s.%s %s) %s %s" % [ + source_attrs[:joins], + through_table_name == through_table_alias ? through_table_name : "#{through_table_name} #{through_table_alias}", + source_attrs[:remote_key], + through_table_alias, source_attrs[:local_key], + source_attrs[:conditions], + through_attrs[:joins], + reflection.options[:joins] + ], + :remote_key => through_attrs[:remote_key], + :local_key => through_attrs[:local_key], + :conditions => conditions + } + end + + # reflection is not has_many :through; it's a standard has_many / belongs_to instead + # TODO: see if we can defer to rails code here a bit more + def construct_has_many_or_belongs_to_attributes(reflection, association_class, table_ids) + # Determine the alias used for remote_table_name, if any. In all cases this will already + # have been assigned an ID in table_ids (either through being involved in a previous join, + # or - if it's the first table in the query - as the default value of table_ids) + remote_table_alias = remote_table_name = association_class.table_name + remote_table_alias += "_#{table_ids[remote_table_name]}" unless table_ids[remote_table_name] == 1 + + # Assign a new alias for the local table. + local_table_alias = local_table_name = reflection.active_record.table_name + if table_ids[local_table_name] + table_id = table_ids[local_table_name] += 1 + local_table_alias += "_#{table_id}" + else + table_ids[local_table_name] = 1 + end + + conditions = '' + # Add type_condition, if applicable + conditions += " AND #{association_class.send(:type_condition).to_sql}" if association_class.finder_needs_type_condition? + # Add custom conditions + conditions += " AND (#{interpolate_sql(association_class.send(:sanitize_sql, reflection.options[:conditions]))})" if reflection.options[:conditions] + + if reflection.macro == :belongs_to + if reflection.options[:polymorphic] + conditions += " AND #{local_table_alias}.#{reflection.options[:foreign_type]} = #{reflection.active_record.quote_value(association_class.base_class.name.to_s)}" + end + { + :joins => reflection.options[:joins], + :remote_key => "#{remote_table_alias}.#{association_class.primary_key}", + :local_key => reflection.primary_key_name, + :conditions => conditions + } + else + # Association is has_many (without :through) + if reflection.options[:as] + conditions += " AND #{remote_table_alias}.#{reflection.options[:as]}_type = #{reflection.active_record.quote_value(reflection.active_record.base_class.name.to_s)}" + end + { + :joins => "#{reflection.options[:joins]}", + :remote_key => "#{remote_table_alias}.#{reflection.primary_key_name}", + :local_key => reflection.klass.primary_key, + :conditions => conditions + } + end + end + + def belongs_to_quoted_key + attribute = @reflection.through_reflection.primary_key_name + column = @owner.column_for_attribute attribute + + @owner.send(:quote_value, @owner.send(attribute), column) + end + + def nested? + through_source_reflection? || through_through_reflection? + end + + def through_source_reflection? + @reflection.source_reflection && @reflection.source_reflection.options[:through] + end + + def through_through_reflection? + @reflection.through_reflection && @reflection.through_reflection.options[:through] + end + end + end +end diff --git a/activerecord/lib/active_record/reflection.rb b/activerecord/lib/active_record/reflection.rb index db18fb7c0f..ae90d30b42 100644 --- a/activerecord/lib/active_record/reflection.rb +++ b/activerecord/lib/active_record/reflection.rb @@ -378,9 +378,9 @@ module ActiveRecord raise HasManyThroughAssociationPolymorphicError.new(active_record.name, self, source_reflection) end - unless [:belongs_to, :has_many, :has_one].include?(source_reflection.macro) && source_reflection.options[:through].nil? - raise HasManyThroughSourceAssociationMacroError.new(self) - end + # unless [:belongs_to, :has_many, :has_one].include?(source_reflection.macro) && source_reflection.options[:through].nil? + # raise HasManyThroughSourceAssociationMacroError.new(self) + # end check_validity_of_inverse! end -- cgit v1.2.3 From 4f69a61107d9d59f96bf249ef077483e90babe72 Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Fri, 1 Oct 2010 13:10:41 +0100 Subject: Started implementing nested :through associations by using the existing structure of ThroughAssociationScope rather than layering a module over the top --- .../associations/has_many_through_association.rb | 2 +- .../associations/through_association_scope.rb | 49 ++++++++++++++-------- activerecord/lib/active_record/reflection.rb | 15 +++++++ 3 files changed, 47 insertions(+), 19 deletions(-) (limited to 'activerecord/lib/active_record') diff --git a/activerecord/lib/active_record/associations/has_many_through_association.rb b/activerecord/lib/active_record/associations/has_many_through_association.rb index 964c381c0d..ee892d373c 100644 --- a/activerecord/lib/active_record/associations/has_many_through_association.rb +++ b/activerecord/lib/active_record/associations/has_many_through_association.rb @@ -7,7 +7,7 @@ module ActiveRecord module Associations class HasManyThroughAssociation < HasManyAssociation #:nodoc: include ThroughAssociationScope - include NestedHasManyThrough + # include NestedHasManyThrough alias_method :new, :build diff --git a/activerecord/lib/active_record/associations/through_association_scope.rb b/activerecord/lib/active_record/associations/through_association_scope.rb index cabb33c4a8..c433c9e66e 100644 --- a/activerecord/lib/active_record/associations/through_association_scope.rb +++ b/activerecord/lib/active_record/associations/through_association_scope.rb @@ -19,8 +19,8 @@ module ActiveRecord # Build SQL conditions from attributes, qualified by table name. def construct_conditions - table_name = @reflection.through_reflection.quoted_table_name - conditions = construct_quoted_owner_attributes(@reflection.through_reflection).map do |attr, value| + table_name = @reflection.final_through_reflection.quoted_table_name + conditions = construct_quoted_owner_attributes(@reflection.final_through_reflection).map do |attr, value| "#{table_name}.#{attr} = #{value}" end conditions << sql_conditions if sql_conditions @@ -49,35 +49,48 @@ module ActiveRecord distinct = "DISTINCT " if @reflection.options[:uniq] selected = custom_select || @reflection.options[:select] || "#{distinct}#{@reflection.quoted_table_name}.*" end - + def construct_joins(custom_joins = nil) + "#{construct_through_joins(@reflection)} #{@reflection.options[:joins]} #{custom_joins}" + end + + def construct_through_joins(reflection) polymorphic_join = nil - if @reflection.source_reflection.macro == :belongs_to - reflection_primary_key = @reflection.klass.primary_key - source_primary_key = @reflection.source_reflection.primary_key_name - if @reflection.options[:source_type] + if reflection.source_reflection.macro == :belongs_to + reflection_primary_key = reflection.klass.primary_key + source_primary_key = reflection.source_reflection.primary_key_name + if reflection.options[:source_type] polymorphic_join = "AND %s.%s = %s" % [ - @reflection.through_reflection.quoted_table_name, "#{@reflection.source_reflection.options[:foreign_type]}", - @owner.class.quote_value(@reflection.options[:source_type]) + reflection.through_reflection.quoted_table_name, "#{@reflection.source_reflection.options[:foreign_type]}", + @owner.class.quote_value(reflection.options[:source_type]) ] end else - reflection_primary_key = @reflection.source_reflection.primary_key_name - source_primary_key = @reflection.through_reflection.klass.primary_key - if @reflection.source_reflection.options[:as] + reflection_primary_key = reflection.source_reflection.primary_key_name + source_primary_key = reflection.through_reflection.klass.primary_key + if reflection.source_reflection.options[:as] polymorphic_join = "AND %s.%s = %s" % [ - @reflection.quoted_table_name, "#{@reflection.source_reflection.options[:as]}_type", - @owner.class.quote_value(@reflection.through_reflection.klass.name) + reflection.quoted_table_name, "#{@reflection.source_reflection.options[:as]}_type", + @owner.class.quote_value(reflection.through_reflection.klass.name) ] end end - "INNER JOIN %s ON %s.%s = %s.%s %s #{@reflection.options[:joins]} #{custom_joins}" % [ - @reflection.through_reflection.quoted_table_name, - @reflection.quoted_table_name, reflection_primary_key, - @reflection.through_reflection.quoted_table_name, source_primary_key, + joins = "INNER JOIN %s ON %s.%s = %s.%s %s" % [ + reflection.through_reflection.quoted_table_name, + reflection.quoted_table_name, reflection_primary_key, + reflection.through_reflection.quoted_table_name, source_primary_key, polymorphic_join ] + + # If the reflection we are going :through goes itself :through another reflection, then + # we must recursively get the joins to make that happen too. + if reflection.through_reflection.through_reflection + joins << " " + joins << construct_through_joins(reflection.through_reflection) + end + + joins end # Construct attributes for associate pointing to owner. diff --git a/activerecord/lib/active_record/reflection.rb b/activerecord/lib/active_record/reflection.rb index ae90d30b42..888ddcdd5b 100644 --- a/activerecord/lib/active_record/reflection.rb +++ b/activerecord/lib/active_record/reflection.rb @@ -352,6 +352,21 @@ module ActiveRecord def through_reflection @through_reflection ||= active_record.reflect_on_association(options[:through]) end + + # A :through reflection may have a :through reflection itself. This method returns the through + # reflection which is furthest away, i.e. the last in the chain, so the first which does not + # have its own :through reflection. + def final_through_reflection + @final_through_reflection ||= begin + reflection = through_reflection + + while reflection.through_reflection + reflection = reflection.through_reflection + end + + reflection + end + end # Gets an array of possible :through source reflection names: # -- cgit v1.2.3 From 34ee586e993ad9e466b81f376fa92feb5d312b4c Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Sat, 2 Oct 2010 18:50:17 +0100 Subject: Integrate nested support into ThroughAssociationScope, using my concept of generating a 'chain' of reflections to be joined. It seems to work at the moment, all existing tests are passing. There may be further complications as we add more test cases for nested associations, though. --- .../associations/has_many_through_association.rb | 1 - .../associations/nested_has_many_through.rb | 2 + .../associations/through_association_scope.rb | 89 +++++++++++++--------- activerecord/lib/active_record/reflection.rb | 50 +++++++++--- 4 files changed, 95 insertions(+), 47 deletions(-) (limited to 'activerecord/lib/active_record') diff --git a/activerecord/lib/active_record/associations/has_many_through_association.rb b/activerecord/lib/active_record/associations/has_many_through_association.rb index ee892d373c..313d9da621 100644 --- a/activerecord/lib/active_record/associations/has_many_through_association.rb +++ b/activerecord/lib/active_record/associations/has_many_through_association.rb @@ -7,7 +7,6 @@ module ActiveRecord module Associations class HasManyThroughAssociation < HasManyAssociation #:nodoc: include ThroughAssociationScope - # include NestedHasManyThrough alias_method :new, :build diff --git a/activerecord/lib/active_record/associations/nested_has_many_through.rb b/activerecord/lib/active_record/associations/nested_has_many_through.rb index 2d03b81128..d699a60edb 100644 --- a/activerecord/lib/active_record/associations/nested_has_many_through.rb +++ b/activerecord/lib/active_record/associations/nested_has_many_through.rb @@ -1,3 +1,5 @@ +# TODO: Remove in the end, when its functionality is fully integrated in ThroughAssociationScope. + module ActiveRecord module Associations module NestedHasManyThrough diff --git a/activerecord/lib/active_record/associations/through_association_scope.rb b/activerecord/lib/active_record/associations/through_association_scope.rb index c433c9e66e..c5453fa79f 100644 --- a/activerecord/lib/active_record/associations/through_association_scope.rb +++ b/activerecord/lib/active_record/associations/through_association_scope.rb @@ -1,3 +1,5 @@ +require 'enumerator' + module ActiveRecord # = Active Record Through Association Scope module Associations @@ -19,8 +21,9 @@ module ActiveRecord # Build SQL conditions from attributes, qualified by table name. def construct_conditions - table_name = @reflection.final_through_reflection.quoted_table_name - conditions = construct_quoted_owner_attributes(@reflection.final_through_reflection).map do |attr, value| + reflection = @reflection.through_reflection_chain.last + table_name = reflection.quoted_table_name + conditions = construct_quoted_owner_attributes(reflection).map do |attr, value| "#{table_name}.#{attr} = #{value}" end conditions << sql_conditions if sql_conditions @@ -51,43 +54,57 @@ module ActiveRecord end def construct_joins(custom_joins = nil) - "#{construct_through_joins(@reflection)} #{@reflection.options[:joins]} #{custom_joins}" + # puts @reflection.through_reflection_chain.map(&:inspect) + + "#{construct_through_joins} #{@reflection.options[:joins]} #{custom_joins}" end - def construct_through_joins(reflection) - polymorphic_join = nil - if reflection.source_reflection.macro == :belongs_to - reflection_primary_key = reflection.klass.primary_key - source_primary_key = reflection.source_reflection.primary_key_name - if reflection.options[:source_type] - polymorphic_join = "AND %s.%s = %s" % [ - reflection.through_reflection.quoted_table_name, "#{@reflection.source_reflection.options[:foreign_type]}", - @owner.class.quote_value(reflection.options[:source_type]) - ] - end - else - reflection_primary_key = reflection.source_reflection.primary_key_name - source_primary_key = reflection.through_reflection.klass.primary_key - if reflection.source_reflection.options[:as] - polymorphic_join = "AND %s.%s = %s" % [ - reflection.quoted_table_name, "#{@reflection.source_reflection.options[:as]}_type", - @owner.class.quote_value(reflection.through_reflection.klass.name) - ] - end - end - - joins = "INNER JOIN %s ON %s.%s = %s.%s %s" % [ - reflection.through_reflection.quoted_table_name, - reflection.quoted_table_name, reflection_primary_key, - reflection.through_reflection.quoted_table_name, source_primary_key, - polymorphic_join - ] + def construct_through_joins + joins = [] - # If the reflection we are going :through goes itself :through another reflection, then - # we must recursively get the joins to make that happen too. - if reflection.through_reflection.through_reflection - joins << " " - joins << construct_through_joins(reflection.through_reflection) + # Iterate over each pair in the through reflection chain, joining them together + @reflection.through_reflection_chain.each_cons(2) do |left, right| + polymorphic_join = nil + + case + when left.options[:as] + left_primary_key = left.primary_key_name + right_primary_key = right.klass.primary_key + + polymorphic_join = "AND %s.%s = %s" % [ + left.quoted_table_name, "#{left.options[:as]}_type", + @owner.class.quote_value(right.klass.name) + ] + when left.source_reflection.macro == :belongs_to + left_primary_key = left.klass.primary_key + right_primary_key = left.source_reflection.primary_key_name + + if left.options[:source_type] + polymorphic_join = "AND %s.%s = %s" % [ + right.quoted_table_name, + left.source_reflection.options[:foreign_type].to_s, + @owner.class.quote_value(left.options[:source_type]) + ] + end + else + left_primary_key = left.source_reflection.primary_key_name + right_primary_key = right.klass.primary_key + + if left.source_reflection.options[:as] + polymorphic_join = "AND %s.%s = %s" % [ + left.quoted_table_name, + "#{left.source_reflection.options[:as]}_type", + @owner.class.quote_value(right.klass.name) + ] + end + end + + joins << "INNER JOIN %s ON %s.%s = %s.%s %s" % [ + right.quoted_table_name, + left.quoted_table_name, left_primary_key, + right.quoted_table_name, right_primary_key, + polymorphic_join + ] end joins diff --git a/activerecord/lib/active_record/reflection.rb b/activerecord/lib/active_record/reflection.rb index 888ddcdd5b..b7cd466e13 100644 --- a/activerecord/lib/active_record/reflection.rb +++ b/activerecord/lib/active_record/reflection.rb @@ -131,6 +131,14 @@ module ActiveRecord @sanitized_conditions ||= klass.send(:sanitize_sql, options[:conditions]) if options[:conditions] end + # TODO: Remove these in the final patch. I am just using them for debugging etc. + def inspect + "#<#{code_name}>" + end + def code_name + "#{active_record.name}.#{macro} :#{name}" + end + private def derive_class_name name.to_s.camelize @@ -241,6 +249,10 @@ module ActiveRecord def through_reflection false end + + def through_reflection_chain + [self] + end def through_reflection_primary_key_name end @@ -304,6 +316,16 @@ module ActiveRecord def belongs_to? macro == :belongs_to end + + # TODO: Remove for final patch. Just here for debugging. + def inspect + str = "#<#{code_name}, @source_reflection=" + str << (source_reflection.respond_to?(:code_name) ? source_reflection.code_name : source_reflection.inspect) + str << ", @through_reflection=" + str << (through_reflection.respond_to?(:code_name) ? through_reflection.code_name : through_reflection.inspect) + str << ">" + str + end private def derive_class_name @@ -353,18 +375,24 @@ module ActiveRecord @through_reflection ||= active_record.reflect_on_association(options[:through]) end - # A :through reflection may have a :through reflection itself. This method returns the through - # reflection which is furthest away, i.e. the last in the chain, so the first which does not - # have its own :through reflection. - def final_through_reflection - @final_through_reflection ||= begin - reflection = through_reflection - - while reflection.through_reflection - reflection = reflection.through_reflection + # TODO: Documentation + def through_reflection_chain + @through_reflection_chain ||= begin + if source_reflection.through_reflection + # If the source reflection goes through another reflection, then the chain must start + # by getting us to the source reflection. + chain = source_reflection.through_reflection_chain + else + # If the source reflection does not go through another reflection, then we can get + # to this reflection directly, and so start the chain here + chain = [self] end - reflection + # Recursively build the rest of the chain + chain += through_reflection.through_reflection_chain + + # Finally return the completed chain + chain end end @@ -393,6 +421,8 @@ module ActiveRecord raise HasManyThroughAssociationPolymorphicError.new(active_record.name, self, source_reflection) end + # TODO: Presumably remove the HasManyThroughSourceAssociationMacroError class and delete these lines. + # Think about whether there are any cases which should still be disallowed. # unless [:belongs_to, :has_many, :has_one].include?(source_reflection.macro) && source_reflection.options[:through].nil? # raise HasManyThroughSourceAssociationMacroError.new(self) # end -- cgit v1.2.3 From a34391c3b495bad268204bdf4f6b3483a61abcd5 Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Sat, 2 Oct 2010 21:45:46 +0100 Subject: Add support for table aliasing, with a test that needs aliasing in order to work correctly. This test incidentally provides a more complicated test case (4 inner joins, 2 using polymorphism). --- .../associations/through_association_scope.rb | 50 ++++++++++++++++------ 1 file changed, 37 insertions(+), 13 deletions(-) (limited to 'activerecord/lib/active_record') diff --git a/activerecord/lib/active_record/associations/through_association_scope.rb b/activerecord/lib/active_record/associations/through_association_scope.rb index c5453fa79f..90ebadda89 100644 --- a/activerecord/lib/active_record/associations/through_association_scope.rb +++ b/activerecord/lib/active_record/associations/through_association_scope.rb @@ -22,9 +22,8 @@ module ActiveRecord # Build SQL conditions from attributes, qualified by table name. def construct_conditions reflection = @reflection.through_reflection_chain.last - table_name = reflection.quoted_table_name conditions = construct_quoted_owner_attributes(reflection).map do |attr, value| - "#{table_name}.#{attr} = #{value}" + "#{table_aliases[reflection]}.#{attr} = #{value}" end conditions << sql_conditions if sql_conditions "(" + conditions.join(') AND (') + ")" @@ -67,21 +66,23 @@ module ActiveRecord polymorphic_join = nil case - when left.options[:as] + when left.source_reflection.nil? left_primary_key = left.primary_key_name right_primary_key = right.klass.primary_key - polymorphic_join = "AND %s.%s = %s" % [ - left.quoted_table_name, "#{left.options[:as]}_type", - @owner.class.quote_value(right.klass.name) - ] + if left.options[:as] + polymorphic_join = "AND %s.%s = %s" % [ + table_aliases[left], "#{left.options[:as]}_type", + @owner.class.quote_value(right.klass.name) + ] + end when left.source_reflection.macro == :belongs_to left_primary_key = left.klass.primary_key right_primary_key = left.source_reflection.primary_key_name if left.options[:source_type] polymorphic_join = "AND %s.%s = %s" % [ - right.quoted_table_name, + table_aliases[right], left.source_reflection.options[:foreign_type].to_s, @owner.class.quote_value(left.options[:source_type]) ] @@ -92,22 +93,45 @@ module ActiveRecord if left.source_reflection.options[:as] polymorphic_join = "AND %s.%s = %s" % [ - left.quoted_table_name, + table_aliases[left], "#{left.source_reflection.options[:as]}_type", @owner.class.quote_value(right.klass.name) ] end end + if right.quoted_table_name == table_aliases[right] + table = right.quoted_table_name + else + table = "#{right.quoted_table_name} #{table_aliases[right]}" + end + joins << "INNER JOIN %s ON %s.%s = %s.%s %s" % [ - right.quoted_table_name, - left.quoted_table_name, left_primary_key, - right.quoted_table_name, right_primary_key, + table, + table_aliases[left], left_primary_key, + table_aliases[right], right_primary_key, polymorphic_join ] end - joins + joins.join(" ") + end + + def table_aliases + @table_aliases ||= begin + tally = {} + @reflection.through_reflection_chain.inject({}) do |aliases, reflection| + if tally[reflection.table_name].nil? + tally[reflection.table_name] = 1 + aliases[reflection] = reflection.quoted_table_name + else + tally[reflection.table_name] += 1 + aliased_table_name = reflection.table_name + "_#{tally[reflection.table_name]}" + aliases[reflection] = reflection.klass.connection.quote_table_name(aliased_table_name) + end + aliases + end + end end # Construct attributes for associate pointing to owner. -- cgit v1.2.3 From f2b41914d6be935182d37e0c0d491352ac3de043 Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Wed, 6 Oct 2010 12:06:51 +0100 Subject: Refactoring JoinDependency and friends. This improves the code (IMO) including adding some explanatory comments, but more importantly structures it in such a way as to allow a JoinAssociation to produce an arbitrary number of actual joins, which will be necessary for nested has many through support. Also added 3 tests covering functionality which existed but was not previously covered. --- activerecord/lib/active_record/associations.rb | 490 +++++++++++++-------- .../lib/active_record/relation/finder_methods.rb | 7 +- .../lib/active_record/relation/query_methods.rb | 13 +- 3 files changed, 313 insertions(+), 197 deletions(-) (limited to 'activerecord/lib/active_record') diff --git a/activerecord/lib/active_record/associations.rb b/activerecord/lib/active_record/associations.rb index 812abf5a55..67c204f154 100644 --- a/activerecord/lib/active_record/associations.rb +++ b/activerecord/lib/active_record/associations.rb @@ -1834,10 +1834,10 @@ module ActiveRecord end class JoinDependency # :nodoc: - attr_reader :joins, :reflections, :table_aliases + attr_reader :join_parts, :reflections, :table_aliases def initialize(base, associations, joins) - @joins = [JoinBase.new(base, joins)] + @join_parts = [JoinBase.new(base, joins)] @associations = associations @reflections = [] @base_records_hash = {} @@ -1850,17 +1850,17 @@ module ActiveRecord def graft(*associations) associations.each do |association| join_associations.detect {|a| association == a} || - build(association.reflection.name, association.find_parent_in(self) || join_base, association.join_class) + build(association.reflection.name, association.find_parent_in(self) || join_base, association.join_type) end self end def join_associations - @joins.last(@joins.length - 1) + join_parts.last(join_parts.length - 1) end def join_base - @joins[0] + join_parts.first end def count_aliases_from_table_joins(name) @@ -1918,22 +1918,24 @@ module ActiveRecord protected - def build(associations, parent = nil, join_class = Arel::InnerJoin) - parent ||= @joins.last + def build(associations, parent = nil, join_type = Arel::InnerJoin) + parent ||= join_parts.last case associations when Symbol, String reflection = parent.reflections[associations.to_s.intern] or raise ConfigurationError, "Association named '#{ associations }' was not found; perhaps you misspelled it?" @reflections << reflection - @joins << build_join_association(reflection, parent).with_join_class(join_class) + join_association = build_join_association(reflection, parent) + join_association.join_type = join_type + @join_parts << join_association when Array associations.each do |association| - build(association, parent, join_class) + build(association, parent, join_type) end when Hash associations.keys.sort{|a,b|a.to_s<=>b.to_s}.each do |name| - build(name, parent, join_class) - build(associations[name], nil, join_class) + build(name, parent, join_type) + build(associations[name], nil, join_type) end else raise ConfigurationError, associations.inspect @@ -1950,91 +1952,111 @@ module ActiveRecord JoinAssociation.new(reflection, self, parent) end - def construct(parent, associations, joins, row) + def construct(parent, associations, join_parts, row) case associations when Symbol, String - join = joins.detect{|j| j.reflection.name.to_s == associations.to_s && j.parent_table_name == parent.class.table_name } - raise(ConfigurationError, "No such association") if join.nil? + join_part = join_parts.detect { |j| + j.reflection.name.to_s == associations.to_s && + j.parent_table_name == parent.class.table_name } + raise(ConfigurationError, "No such association") if join_part.nil? - joins.delete(join) - construct_association(parent, join, row) + join_parts.delete(join_part) + construct_association(parent, join_part, row) when Array associations.each do |association| - construct(parent, association, joins, row) + construct(parent, association, join_parts, row) end when Hash associations.sort_by { |k,_| k.to_s }.each do |name, assoc| - join = joins.detect{|j| j.reflection.name.to_s == name.to_s && j.parent_table_name == parent.class.table_name } - raise(ConfigurationError, "No such association") if join.nil? - - association = construct_association(parent, join, row) - joins.delete(join) - construct(association, assoc, joins, row) if association + join_part = join_parts.detect{ |j| + j.reflection.name.to_s == name.to_s && + j.parent_table_name == parent.class.table_name } + raise(ConfigurationError, "No such association") if join_part.nil? + + association = construct_association(parent, join_part, row) + join_parts.delete(join_part) + construct(association, assoc, join_parts, row) if association end else raise ConfigurationError, associations.inspect end end - def construct_association(record, join, row) - return if record.id.to_s != join.parent.record_id(row).to_s + def construct_association(record, join_part, row) + return if record.id.to_s != join_part.parent.record_id(row).to_s - macro = join.reflection.macro + macro = join_part.reflection.macro if macro == :has_one - return if record.instance_variable_defined?("@#{join.reflection.name}") - association = join.instantiate(row) unless row[join.aliased_primary_key].nil? - set_target_and_inverse(join, association, record) + return if record.instance_variable_defined?("@#{join_part.reflection.name}") + association = join_part.instantiate(row) unless row[join_part.aliased_primary_key].nil? + set_target_and_inverse(join_part, association, record) else - return if row[join.aliased_primary_key].nil? - association = join.instantiate(row) + return if row[join_part.aliased_primary_key].nil? + association = join_part.instantiate(row) case macro when :has_many, :has_and_belongs_to_many - collection = record.send(join.reflection.name) + collection = record.send(join_part.reflection.name) collection.loaded collection.target.push(association) collection.__send__(:set_inverse_instance, association, record) when :belongs_to - set_target_and_inverse(join, association, record) + set_target_and_inverse(join_part, association, record) else - raise ConfigurationError, "unknown macro: #{join.reflection.macro}" + raise ConfigurationError, "unknown macro: #{join_part.reflection.macro}" end end association end - def set_target_and_inverse(join, association, record) - association_proxy = record.send("set_#{join.reflection.name}_target", association) + def set_target_and_inverse(join_part, association, record) + association_proxy = record.send("set_#{join_part.reflection.name}_target", association) association_proxy.__send__(:set_inverse_instance, association, record) end - - class JoinBase # :nodoc: - attr_reader :active_record, :table_joins - delegate :table_name, :column_names, :primary_key, :reflections, :sanitize_sql, :arel_engine, :to => :active_record - - def initialize(active_record, joins = nil) + + # A JoinPart represents a part of a JoinDependency. It is an abstract class, inherited + # by JoinBase and JoinAssociation. A JoinBase represents the Active Record which + # everything else is being joined onto. A JoinAssociation represents an association which + # is joining to the base. A JoinAssociation may result in more than one actual join + # operations (for example a has_and_belongs_to_many JoinAssociation would result in + # two; one for the join table and one for the target table). + class JoinPart # :nodoc: + # The Active Record class which this join part is associated 'about'; for a JoinBase + # this is the actual base model, for a JoinAssociation this is the target model of the + # association. + attr_reader :active_record + + delegate :table_name, :column_names, :primary_key, :reflections, :sanitize_sql, :arel_engine, :to => :active_record + + def initialize(active_record) @active_record = active_record @cached_record = {} - @table_joins = joins end - + def ==(other) - other.class == self.class && - other.active_record == active_record && - other.table_joins == table_joins + raise NotImplementedError end - + + # An Arel::Table for the active_record + def table + raise NotImplementedError + end + + # The prefix to be used when aliasing columns in the active_record's table def aliased_prefix - "t0" + raise NotImplementedError end - + + # The alias for the active_record's table + def aliased_table_name + raise NotImplementedError + end + + # The alias for the primary key of the active_record's table def aliased_primary_key "#{aliased_prefix}_r0" end - - def aliased_table_name - active_record.table_name - end - + + # An array of [column_name, alias] pairs for the table def column_names_with_alias unless defined?(@column_names_with_alias) @column_names_with_alias = [] @@ -2060,33 +2082,74 @@ module ActiveRecord end end - class JoinAssociation < JoinBase # :nodoc: - attr_reader :reflection, :parent, :aliased_table_name, :aliased_prefix, :aliased_join_table_name, :parent_table_name, :join_class - delegate :options, :klass, :through_reflection, :source_reflection, :to => :reflection + class JoinBase < JoinPart # :nodoc: + # Extra joins provided when the JoinDependency was created + attr_reader :table_joins + + def initialize(active_record, joins = nil) + super(active_record) + @table_joins = joins + end + + def ==(other) + other.class == self.class && + other.active_record == active_record && + other.table_joins == table_joins + end + + def aliased_prefix + "t0" + end + + def table + Arel::Table.new(table_name, :engine => arel_engine, :columns => active_record.columns) + end + + def aliased_table_name + active_record.table_name + end + end + + class JoinAssociation < JoinPart # :nodoc: + # The reflection of the association represented + attr_reader :reflection + + # The JoinDependency object which this JoinAssociation exists within. This is mainly + # relevant for generating aliases which do not conflict with other joins which are + # part of the query. + attr_reader :join_dependency + + # A JoinBase instance representing the active record we are joining onto. + # (So in Author.has_many :posts, the Author would be that base record.) + attr_reader :parent + + # What type of join will be generated, either Arel::InnerJoin (default) or Arel::OuterJoin + attr_accessor :join_type + + # These implement abstract methods from the superclass + attr_reader :aliased_prefix, :aliased_table_name + + delegate :options, :through_reflection, :source_reflection, :to => :reflection + delegate :table, :table_name, :to => :parent, :prefix => true def initialize(reflection, join_dependency, parent = nil) reflection.check_validity! + if reflection.options[:polymorphic] raise EagerLoadPolymorphicError.new(reflection) end super(reflection.klass) - @join_dependency = join_dependency - @parent = parent - @reflection = reflection - @aliased_prefix = "t#{ join_dependency.joins.size }" - @parent_table_name = parent.active_record.table_name - @aliased_table_name = aliased_table_name_for(table_name) - @join = nil - @join_class = Arel::InnerJoin - - if reflection.macro == :has_and_belongs_to_many - @aliased_join_table_name = aliased_table_name_for(reflection.options[:join_table], "_join") - end - - if [:has_many, :has_one].include?(reflection.macro) && reflection.options[:through] - @aliased_join_table_name = aliased_table_name_for(reflection.through_reflection.klass.table_name, "_join") - end + + @reflection = reflection + @join_dependency = join_dependency + @parent = parent + @join_type = Arel::InnerJoin + + # This must be done eagerly upon initialisation because the alias which is produced + # depends on the state of the join dependency, but we want it to work the same way + # every time. + allocate_aliases end def ==(other) @@ -2096,63 +2159,29 @@ module ActiveRecord end def find_parent_in(other_join_dependency) - other_join_dependency.joins.detect do |join| - self.parent == join + other_join_dependency.join_parts.detect do |join_part| + self.parent == join_part end end - - def with_join_class(join_class) - @join_class = join_class - self - end - - def association_join - return @join if @join - - aliased_table = Arel::Table.new(table_name, :as => @aliased_table_name, - :engine => arel_engine, - :columns => klass.columns) - - parent_table = Arel::Table.new(parent.table_name, :as => parent.aliased_table_name, - :engine => arel_engine, - :columns => parent.active_record.columns) - - @join = send("build_#{reflection.macro}", aliased_table, parent_table) - - unless klass.descends_from_active_record? - sti_column = aliased_table[klass.inheritance_column] - sti_condition = sti_column.eq(klass.sti_name) - klass.descendants.each {|subclass| sti_condition = sti_condition.or(sti_column.eq(subclass.sti_name)) } - - @join << sti_condition - end - - [through_reflection, reflection].each do |ref| - if ref && ref.options[:conditions] - @join << interpolate_sql(sanitize_sql(ref.options[:conditions], aliased_table_name)) - end - end - - @join + + def join_to(relation) + send("join_#{reflection.macro}_to", relation) end - def relation - aliased = Arel::Table.new(table_name, :as => @aliased_table_name, - :engine => arel_engine, - :columns => klass.columns) - - if reflection.macro == :has_and_belongs_to_many - [Arel::Table.new(options[:join_table], :as => aliased_join_table_name, :engine => arel_engine), aliased] - elsif reflection.options[:through] - [Arel::Table.new(through_reflection.klass.table_name, :as => aliased_join_table_name, :engine => arel_engine), aliased] - else - aliased - end + def join_relation(joining_relation) + self.join_type = Arel::OuterJoin + joining_relation.joins(self) end - - def join_relation(joining_relation, join = nil) - joining_relation.joins(self.with_join_class(Arel::OuterJoin)) + + def table + @table ||= Arel::Table.new( + table_name, :as => aliased_table_name, + :engine => arel_engine, :columns => active_record.columns + ) end + + # More semantic name given we are talking about associations + alias_method :target_table, :table protected @@ -2186,7 +2215,7 @@ module ActiveRecord end def table_name_and_alias - table_alias_for table_name, @aliased_table_name + table_alias_for table_name, aliased_table_name end def interpolate_sql(sql) @@ -2194,74 +2223,169 @@ module ActiveRecord end private + + def allocate_aliases + @aliased_prefix = "t#{ join_dependency.join_parts.size }" + @aliased_table_name = aliased_table_name_for(table_name) + + if reflection.macro == :has_and_belongs_to_many + @aliased_join_table_name = aliased_table_name_for(reflection.options[:join_table], "_join") + elsif [:has_many, :has_one].include?(reflection.macro) && reflection.options[:through] + @aliased_join_table_name = aliased_table_name_for(reflection.through_reflection.klass.table_name, "_join") + end + end + + def process_conditions(conditions, table_name) + Arel.sql(interpolate_sql(sanitize_sql(conditions, table_name))) + end + + def join_target_table(relation, *conditions) + relation = relation.join(target_table, join_type) + + # If the target table is an STI model then we must be sure to only include records of + # its type and its sub-types. + unless active_record.descends_from_active_record? + sti_column = target_table[active_record.inheritance_column] + + sti_condition = sti_column.eq(active_record.sti_name) + active_record.descendants.each do |subclass| + sti_condition = sti_condition.or(sti_column.eq(subclass.sti_name)) + end + + conditions << sti_condition + end + + # If the reflection has conditions, add them + if options[:conditions] + conditions << process_conditions(options[:conditions], aliased_table_name) + end + + relation = relation.on(*conditions) + end - def build_has_and_belongs_to_many(aliased_table, parent_table) - join_table = Arel::Table.new(options[:join_table], :as => aliased_join_table_name, :engine => arel_engine) - fk = options[:foreign_key] || reflection.active_record.to_s.foreign_key - klass_fk = options[:association_foreign_key] || klass.to_s.foreign_key - - [ - join_table[fk].eq(parent_table[reflection.active_record.primary_key]), - aliased_table[klass.primary_key].eq(join_table[klass_fk]) - ] + def join_has_and_belongs_to_many_to(relation) + join_table = Arel::Table.new( + options[:join_table], :engine => arel_engine, + :as => @aliased_join_table_name + ) + + fk = options[:foreign_key] || reflection.active_record.to_s.foreign_key + klass_fk = options[:association_foreign_key] || reflection.klass.to_s.foreign_key + + relation = relation.join(join_table, join_type) + relation = relation.on( + join_table[fk]. + eq(parent_table[reflection.active_record.primary_key]) + ) + + join_target_table( + relation, + target_table[reflection.klass.primary_key]. + eq(join_table[klass_fk]) + ) end - def build_has_many(aliased_table, parent_table) + def join_has_many_to(relation) if reflection.options[:through] - join_table = Arel::Table.new(through_reflection.klass.table_name, - :as => aliased_join_table_name, - :engine => arel_engine) - jt_foreign_key = jt_as_extra = jt_source_extra = jt_sti_extra = nil - first_key = second_key = nil - - if through_reflection.options[:as] # has_many :through against a polymorphic join - as_key = through_reflection.options[:as].to_s - jt_foreign_key = as_key + '_id' - jt_as_extra = join_table[as_key + '_type'].eq(parent.active_record.base_class.name) - else - jt_foreign_key = through_reflection.primary_key_name - end - - case source_reflection.macro - when :has_many - second_key = options[:foreign_key] || primary_key + join_has_many_through_to(relation) + elsif reflection.options[:as] + join_has_many_polymorphic_to(relation) + else + foreign_key = options[:foreign_key] || reflection.active_record.name.foreign_key + primary_key = options[:primary_key] || parent.primary_key + + join_target_table( + relation, + target_table[foreign_key]. + eq(parent_table[primary_key]) + ) + end + end + alias :join_has_one_to :join_has_many_to + + def join_has_many_through_to(relation) + join_table = Arel::Table.new( + through_reflection.klass.table_name, :engine => arel_engine, + :as => @aliased_join_table_name + ) + + jt_conditions = [] + jt_foreign_key = first_key = second_key = nil + + if through_reflection.options[:as] # has_many :through against a polymorphic join + as_key = through_reflection.options[:as].to_s + jt_foreign_key = as_key + '_id' + + jt_conditions << + join_table[as_key + '_type']. + eq(parent.active_record.base_class.name) + else + jt_foreign_key = through_reflection.primary_key_name + end - if source_reflection.options[:as] - first_key = "#{source_reflection.options[:as]}_id" - else - first_key = through_reflection.klass.base_class.to_s.foreign_key - end + case source_reflection.macro + when :has_many + second_key = options[:foreign_key] || primary_key - unless through_reflection.klass.descends_from_active_record? - jt_sti_extra = join_table[through_reflection.active_record.inheritance_column].eq(through_reflection.klass.sti_name) - end - when :belongs_to - first_key = primary_key - if reflection.options[:source_type] - second_key = source_reflection.association_foreign_key - jt_source_extra = join_table[reflection.source_reflection.options[:foreign_type]].eq(reflection.options[:source_type]) - else - second_key = source_reflection.primary_key_name - end + if source_reflection.options[:as] + first_key = "#{source_reflection.options[:as]}_id" + else + first_key = through_reflection.klass.base_class.to_s.foreign_key end - [ - [parent_table[parent.primary_key].eq(join_table[jt_foreign_key]), jt_as_extra, jt_source_extra, jt_sti_extra].compact, - aliased_table[first_key].eq(join_table[second_key]) - ] - elsif reflection.options[:as] - id_rel = aliased_table["#{reflection.options[:as]}_id"].eq(parent_table[parent.primary_key]) - type_rel = aliased_table["#{reflection.options[:as]}_type"].eq(parent.active_record.base_class.name) - [id_rel, type_rel] - else - foreign_key = options[:foreign_key] || reflection.active_record.name.foreign_key - [aliased_table[foreign_key].eq(parent_table[reflection.options[:primary_key] || parent.primary_key])] + unless through_reflection.klass.descends_from_active_record? + jt_conditions << + join_table[through_reflection.active_record.inheritance_column]. + eq(through_reflection.klass.sti_name) + end + when :belongs_to + first_key = primary_key + + if reflection.options[:source_type] + second_key = source_reflection.association_foreign_key + + jt_conditions << + join_table[reflection.source_reflection.options[:foreign_type]]. + eq(reflection.options[:source_type]) + else + second_key = source_reflection.primary_key_name + end + end + + jt_conditions << + parent_table[parent.primary_key]. + eq(join_table[jt_foreign_key]) + + if through_reflection.options[:conditions] + jt_conditions << process_conditions(through_reflection.options[:conditions], aliased_table_name) end + + relation = relation.join(join_table, join_type).on(*jt_conditions) + + join_target_table( + relation, + target_table[first_key].eq(join_table[second_key]) + ) + end + + def join_has_many_polymorphic_to(relation) + join_target_table( + relation, + target_table["#{reflection.options[:as]}_id"]. + eq(parent_table[parent.primary_key]), + target_table["#{reflection.options[:as]}_type"]. + eq(parent.active_record.base_class.name) + ) end - alias :build_has_one :build_has_many - def build_belongs_to(aliased_table, parent_table) - [aliased_table[options[:primary_key] || reflection.klass.primary_key].eq(parent_table[options[:foreign_key] || reflection.primary_key_name])] + def join_belongs_to_to(relation) + foreign_key = options[:foreign_key] || reflection.primary_key_name + primary_key = options[:primary_key] || reflection.klass.primary_key + + join_target_table( + relation, + target_table[primary_key].eq(parent_table[foreign_key]) + ) end end end diff --git a/activerecord/lib/active_record/relation/finder_methods.rb b/activerecord/lib/active_record/relation/finder_methods.rb index ede1c8821e..5034caf084 100644 --- a/activerecord/lib/active_record/relation/finder_methods.rb +++ b/activerecord/lib/active_record/relation/finder_methods.rb @@ -343,8 +343,11 @@ module ActiveRecord end def column_aliases(join_dependency) - join_dependency.joins.collect{|join| join.column_names_with_alias.collect{|column_name, aliased_name| - "#{connection.quote_table_name join.aliased_table_name}.#{connection.quote_column_name column_name} AS #{aliased_name}"}}.flatten.join(", ") + join_dependency.join_parts.collect { |join_part| + join_part.column_names_with_alias.collect{ |column_name, aliased_name| + "#{connection.quote_table_name join_part.aliased_table_name}.#{connection.quote_column_name column_name} AS #{aliased_name}" + } + }.flatten.join(", ") end def using_limitable_reflections?(reflections) diff --git a/activerecord/lib/active_record/relation/query_methods.rb b/activerecord/lib/active_record/relation/query_methods.rb index 2e0a2effc2..acc42faf7d 100644 --- a/activerecord/lib/active_record/relation/query_methods.rb +++ b/activerecord/lib/active_record/relation/query_methods.rb @@ -230,19 +230,8 @@ module ActiveRecord @implicit_readonly = true unless association_joins.empty? && stashed_association_joins.empty? - to_join = [] - join_dependency.join_associations.each do |association| - if (association_relation = association.relation).is_a?(Array) - to_join << [association_relation.first, association.join_class, association.association_join.first] - to_join << [association_relation.last, association.join_class, association.association_join.last] - else - to_join << [association_relation, association.join_class, association.association_join] - end - end - - to_join.uniq.each do |left, join_class, right| - relation = relation.join(left, join_class).on(*right) + relation = association.join_to(relation) end relation.join(custom_joins) -- cgit v1.2.3 From ab5a9335020eff0da35b62b86a62ed8587a4d598 Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Sat, 9 Oct 2010 22:00:33 +0100 Subject: Add support for nested through associations in JoinAssociation. Hence Foo.joins(:bar) will work for through associations. There is some duplicated code now, which will be refactored. --- activerecord/lib/active_record/associations.rb | 174 ++++++++++----------- .../associations/through_association_scope.rb | 4 + 2 files changed, 89 insertions(+), 89 deletions(-) (limited to 'activerecord/lib/active_record') diff --git a/activerecord/lib/active_record/associations.rb b/activerecord/lib/active_record/associations.rb index 67c204f154..688c05c545 100644 --- a/activerecord/lib/active_record/associations.rb +++ b/activerecord/lib/active_record/associations.rb @@ -2129,7 +2129,7 @@ module ActiveRecord # These implement abstract methods from the superclass attr_reader :aliased_prefix, :aliased_table_name - delegate :options, :through_reflection, :source_reflection, :to => :reflection + delegate :options, :through_reflection, :source_reflection, :through_reflection_chain, :to => :reflection delegate :table, :table_name, :to => :parent, :prefix => true def initialize(reflection, join_dependency, parent = nil) @@ -2185,13 +2185,13 @@ module ActiveRecord protected - def aliased_table_name_for(name, suffix = nil) + def aliased_table_name_for(name, aliased_name, suffix = nil) if @join_dependency.table_aliases[name].zero? @join_dependency.table_aliases[name] = @join_dependency.count_aliases_from_table_joins(name) end if !@join_dependency.table_aliases[name].zero? # We need an alias - name = active_record.connection.table_alias_for "#{pluralize(reflection.name)}_#{parent_table_name}#{suffix}" + name = active_record.connection.table_alias_for "#{aliased_name}_#{parent_table_name}#{suffix}" @join_dependency.table_aliases[name] += 1 if @join_dependency.table_aliases[name] == 1 # First time we've seen this name # Also need to count the aliases from the table_aliases to avoid incorrect count @@ -2226,12 +2226,26 @@ module ActiveRecord def allocate_aliases @aliased_prefix = "t#{ join_dependency.join_parts.size }" - @aliased_table_name = aliased_table_name_for(table_name) + @aliased_table_name = aliased_table_name_for(table_name, pluralize(reflection.name)) - if reflection.macro == :has_and_belongs_to_many - @aliased_join_table_name = aliased_table_name_for(reflection.options[:join_table], "_join") - elsif [:has_many, :has_one].include?(reflection.macro) && reflection.options[:through] - @aliased_join_table_name = aliased_table_name_for(reflection.through_reflection.klass.table_name, "_join") + case reflection.macro + when :has_and_belongs_to_many + @aliased_join_table_name = aliased_table_name_for( + reflection.options[:join_table], + pluralize(reflection.name), "_join" + ) + when :has_many, :has_one + # Add the target table name which was already generated. We don't want to generate + # it again as that would lead to an unnecessary alias. + @aliased_through_table_names = [@aliased_table_name] + + # Generate the rest in the original order + @aliased_through_table_names += through_reflection_chain[1..-1].map do |reflection| + aliased_table_name_for(reflection.table_name, pluralize(reflection.name), "_join") + end + + # Now reverse the list, as we will use it in that order + @aliased_through_table_names.reverse! end end @@ -2284,99 +2298,81 @@ module ActiveRecord eq(join_table[klass_fk]) ) end - - def join_has_many_to(relation) - if reflection.options[:through] - join_has_many_through_to(relation) - elsif reflection.options[:as] - join_has_many_polymorphic_to(relation) - else - foreign_key = options[:foreign_key] || reflection.active_record.name.foreign_key - primary_key = options[:primary_key] || parent.primary_key - - join_target_table( - relation, - target_table[foreign_key]. - eq(parent_table[primary_key]) - ) - end - end - alias :join_has_one_to :join_has_many_to - def join_has_many_through_to(relation) - join_table = Arel::Table.new( - through_reflection.klass.table_name, :engine => arel_engine, - :as => @aliased_join_table_name - ) + def join_has_many_to(relation) + # Chain usually starts with target, but we want to end with it here (just makes it + # easier to understand the joins that are generated) + chain = through_reflection_chain.reverse - jt_conditions = [] - jt_foreign_key = first_key = second_key = nil - - if through_reflection.options[:as] # has_many :through against a polymorphic join - as_key = through_reflection.options[:as].to_s - jt_foreign_key = as_key + '_id' + foreign_table = parent_table + + chain.zip(@aliased_through_table_names).each do |reflection, aliased_table_name| + table = Arel::Table.new( + reflection.table_name, :engine => arel_engine, + :as => aliased_table_name, :columns => reflection.klass.columns + ) - jt_conditions << - join_table[as_key + '_type']. - eq(parent.active_record.base_class.name) - else - jt_foreign_key = through_reflection.primary_key_name - end - - case source_reflection.macro - when :has_many - second_key = options[:foreign_key] || primary_key - - if source_reflection.options[:as] - first_key = "#{source_reflection.options[:as]}_id" + conditions = [] + + if reflection.source_reflection.nil? + case reflection.macro + when :belongs_to + key = reflection.options[:primary_key] || + reflection.klass.primary_key + foreign_key = reflection.primary_key_name + when :has_many, :has_one + key = reflection.primary_key_name + foreign_key = reflection.options[:primary_key] || + reflection.active_record.primary_key + + if reflection.options[:as] + conditions << + table["#{reflection.options[:as]}_type"]. + eq(reflection.active_record.base_class.name) + end + end + elsif reflection.source_reflection.macro == :belongs_to + key = reflection.klass.primary_key + foreign_key = reflection.source_reflection.primary_key_name + + if reflection.options[:source_type] + conditions << + foreign_table[reflection.source_reflection.options[:foreign_type]]. + eq(reflection.options[:source_type]) + end else - first_key = through_reflection.klass.base_class.to_s.foreign_key + key = reflection.source_reflection.primary_key_name + foreign_key = reflection.source_reflection.klass.primary_key end - - unless through_reflection.klass.descends_from_active_record? - jt_conditions << - join_table[through_reflection.active_record.inheritance_column]. - eq(through_reflection.klass.sti_name) + + conditions << table[key].eq(foreign_table[foreign_key]) + + if reflection.options[:conditions] + conditions << process_conditions(reflection.options[:conditions], aliased_table_name) end - when :belongs_to - first_key = primary_key - if reflection.options[:source_type] - second_key = source_reflection.association_foreign_key + # If the target table is an STI model then we must be sure to only include records of + # its type and its sub-types. + unless reflection.klass.descends_from_active_record? + sti_column = table[reflection.klass.inheritance_column] - jt_conditions << - join_table[reflection.source_reflection.options[:foreign_type]]. - eq(reflection.options[:source_type]) - else - second_key = source_reflection.primary_key_name + sti_condition = sti_column.eq(reflection.klass.sti_name) + reflection.klass.descendants.each do |subclass| + sti_condition = sti_condition.or(sti_column.eq(subclass.sti_name)) + end + + conditions << sti_condition end + + relation = relation.join(table, join_type).on(*conditions) + + # The current table in this iteration becomes the foreign table in the next + foreign_table = table end - jt_conditions << - parent_table[parent.primary_key]. - eq(join_table[jt_foreign_key]) - - if through_reflection.options[:conditions] - jt_conditions << process_conditions(through_reflection.options[:conditions], aliased_table_name) - end - - relation = relation.join(join_table, join_type).on(*jt_conditions) - - join_target_table( - relation, - target_table[first_key].eq(join_table[second_key]) - ) - end - - def join_has_many_polymorphic_to(relation) - join_target_table( - relation, - target_table["#{reflection.options[:as]}_id"]. - eq(parent_table[parent.primary_key]), - target_table["#{reflection.options[:as]}_type"]. - eq(parent.active_record.base_class.name) - ) + relation end + alias :join_has_one_to :join_has_many_to def join_belongs_to_to(relation) foreign_key = options[:foreign_key] || reflection.primary_key_name diff --git a/activerecord/lib/active_record/associations/through_association_scope.rb b/activerecord/lib/active_record/associations/through_association_scope.rb index 90ebadda89..8406f5fd20 100644 --- a/activerecord/lib/active_record/associations/through_association_scope.rb +++ b/activerecord/lib/active_record/associations/through_association_scope.rb @@ -73,6 +73,8 @@ module ActiveRecord if left.options[:as] polymorphic_join = "AND %s.%s = %s" % [ table_aliases[left], "#{left.options[:as]}_type", + # TODO: Why right.klass.name? Rather than left.active_record.name? + # TODO: Also should maybe use the base_class (see related code in JoinAssociation) @owner.class.quote_value(right.klass.name) ] end @@ -117,6 +119,8 @@ module ActiveRecord joins.join(" ") end + # TODO: Use the same aliasing strategy (and code?) as JoinAssociation (as this is the + # documented behaviour) def table_aliases @table_aliases ||= begin tally = {} -- cgit v1.2.3 From 3aba73fed1afc93cf64f9da90d5ad2d51c99df9a Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Sun, 10 Oct 2010 00:53:04 +0100 Subject: Refactoring to remove duplication introduced by the last commit --- activerecord/lib/active_record/associations.rb | 305 ++++++++++++------------- 1 file changed, 145 insertions(+), 160 deletions(-) (limited to 'activerecord/lib/active_record') diff --git a/activerecord/lib/active_record/associations.rb b/activerecord/lib/active_record/associations.rb index 688c05c545..397159d35e 100644 --- a/activerecord/lib/active_record/associations.rb +++ b/activerecord/lib/active_record/associations.rb @@ -2126,8 +2126,7 @@ module ActiveRecord # What type of join will be generated, either Arel::InnerJoin (default) or Arel::OuterJoin attr_accessor :join_type - # These implement abstract methods from the superclass - attr_reader :aliased_prefix, :aliased_table_name + attr_reader :aliased_prefix delegate :options, :through_reflection, :source_reflection, :through_reflection_chain, :to => :reflection delegate :table, :table_name, :to => :parent, :prefix => true @@ -2141,15 +2140,13 @@ module ActiveRecord super(reflection.klass) - @reflection = reflection - @join_dependency = join_dependency - @parent = parent - @join_type = Arel::InnerJoin + @reflection = reflection + @join_dependency = join_dependency + @parent = parent + @join_type = Arel::InnerJoin + @aliased_prefix = "t#{ join_dependency.join_parts.size }" - # This must be done eagerly upon initialisation because the alias which is produced - # depends on the state of the join dependency, but we want it to work the same way - # every time. - allocate_aliases + setup_tables end def ==(other) @@ -2165,7 +2162,71 @@ module ActiveRecord end def join_to(relation) - send("join_#{reflection.macro}_to", relation) + # The chain starts with the target table, but we want to end with it here (makes + # more sense in this context) + chain = through_reflection_chain.reverse + + foreign_table = parent_table + + chain.zip(@tables).each do |reflection, table| + conditions = [] + + if reflection.source_reflection.nil? + case reflection.macro + when :belongs_to + key = reflection.options[:primary_key] || + reflection.klass.primary_key + foreign_key = reflection.primary_key_name + when :has_many, :has_one + key = reflection.primary_key_name + foreign_key = reflection.options[:primary_key] || + reflection.active_record.primary_key + + conditions << polymorphic_conditions(reflection, table) + when :has_and_belongs_to_many + # For habtm, we need to deal with the join table at the same time as the + # target table (because unlike a :through association, there is no reflection + # to represent the join table) + table, join_table = table + + join_key = reflection.options[:foreign_key] || + reflection.active_record.to_s.foreign_key + join_foreign_key = reflection.active_record.primary_key + + relation = relation.join(join_table, join_type).on( + join_table[join_key]. + eq(foreign_table[join_foreign_key]) + ) + + # We've done the first join now, so update the foreign_table for the second + foreign_table = join_table + + key = reflection.klass.primary_key + foreign_key = reflection.options[:association_foreign_key] || + reflection.klass.to_s.foreign_key + end + elsif reflection.source_reflection.macro == :belongs_to + key = reflection.klass.primary_key + foreign_key = reflection.source_reflection.primary_key_name + + conditions << source_type_conditions(reflection, foreign_table) + else + key = reflection.source_reflection.primary_key_name + foreign_key = reflection.source_reflection.klass.primary_key + end + + conditions << table[key].eq(foreign_table[foreign_key]) + + conditions << reflection_conditions(reflection, table) + conditions << sti_conditions(reflection, table) + + relation = relation.join(table, join_type).on(*conditions.compact) + + # The current table in this iteration becomes the foreign table in the next + foreign_table = table + end + + relation end def join_relation(joining_relation) @@ -2174,15 +2235,17 @@ module ActiveRecord end def table - @table ||= Arel::Table.new( - table_name, :as => aliased_table_name, - :engine => arel_engine, :columns => active_record.columns - ) + if reflection.macro == :has_and_belongs_to_many + @tables.last.first + else + @tables.last + end + end + + def aliased_table_name + table.table_alias || table.name end - # More semantic name given we are talking about associations - alias_method :target_table, :table - protected def aliased_table_name_for(name, aliased_name, suffix = nil) @@ -2224,167 +2287,89 @@ module ActiveRecord private - def allocate_aliases - @aliased_prefix = "t#{ join_dependency.join_parts.size }" - @aliased_table_name = aliased_table_name_for(table_name, pluralize(reflection.name)) - - case reflection.macro - when :has_and_belongs_to_many - @aliased_join_table_name = aliased_table_name_for( + # Generate aliases and Arel::Table instances for each of the tables which we will + # later generate joins for. We must do this in advance in order to correctly allocate + # the proper alias. + def setup_tables + @tables = through_reflection_chain.map do |reflection| + suffix = reflection == self.reflection ? nil : '_join' + + aliased_table_name = aliased_table_name_for( + reflection.table_name, + pluralize(reflection.name), + suffix + ) + + table = Arel::Table.new( + reflection.table_name, :engine => arel_engine, + :as => aliased_table_name, :columns => reflection.klass.columns + ) + + # For habtm, we have two Arel::Table instances related to a single reflection, so + # we just store them as a pair in the array. + if reflection.macro == :has_and_belongs_to_many + aliased_join_table_name = aliased_table_name_for( reflection.options[:join_table], pluralize(reflection.name), "_join" ) - when :has_many, :has_one - # Add the target table name which was already generated. We don't want to generate - # it again as that would lead to an unnecessary alias. - @aliased_through_table_names = [@aliased_table_name] - # Generate the rest in the original order - @aliased_through_table_names += through_reflection_chain[1..-1].map do |reflection| - aliased_table_name_for(reflection.table_name, pluralize(reflection.name), "_join") - end + join_table = Arel::Table.new( + reflection.options[:join_table], :engine => arel_engine, + :as => aliased_join_table_name + ) - # Now reverse the list, as we will use it in that order - @aliased_through_table_names.reverse! - end - end - - def process_conditions(conditions, table_name) - Arel.sql(interpolate_sql(sanitize_sql(conditions, table_name))) - end - - def join_target_table(relation, *conditions) - relation = relation.join(target_table, join_type) - - # If the target table is an STI model then we must be sure to only include records of - # its type and its sub-types. - unless active_record.descends_from_active_record? - sti_column = target_table[active_record.inheritance_column] - - sti_condition = sti_column.eq(active_record.sti_name) - active_record.descendants.each do |subclass| - sti_condition = sti_condition.or(sti_column.eq(subclass.sti_name)) + [table, join_table] + else + table end - - conditions << sti_condition end - # If the reflection has conditions, add them - if options[:conditions] - conditions << process_conditions(options[:conditions], aliased_table_name) - end + # The joins are generated from the through_reflection_chain in reverse order, so + # reverse the tables too (but it's important to generate the aliases in the 'forward' + # order, which is why we only do the reversal now. + @tables.reverse! - relation = relation.on(*conditions) + @tables end - - def join_has_and_belongs_to_many_to(relation) - join_table = Arel::Table.new( - options[:join_table], :engine => arel_engine, - :as => @aliased_join_table_name - ) - - fk = options[:foreign_key] || reflection.active_record.to_s.foreign_key - klass_fk = options[:association_foreign_key] || reflection.klass.to_s.foreign_key - - relation = relation.join(join_table, join_type) - relation = relation.on( - join_table[fk]. - eq(parent_table[reflection.active_record.primary_key]) - ) - - join_target_table( - relation, - target_table[reflection.klass.primary_key]. - eq(join_table[klass_fk]) - ) + + def reflection_conditions(reflection, table) + if reflection.options[:conditions] + Arel.sql(interpolate_sql(sanitize_sql( + reflection.options[:conditions], + table.table_alias || table.name + ))) + end end - def join_has_many_to(relation) - # Chain usually starts with target, but we want to end with it here (just makes it - # easier to understand the joins that are generated) - chain = through_reflection_chain.reverse - - foreign_table = parent_table - - chain.zip(@aliased_through_table_names).each do |reflection, aliased_table_name| - table = Arel::Table.new( - reflection.table_name, :engine => arel_engine, - :as => aliased_table_name, :columns => reflection.klass.columns - ) + def sti_conditions(reflection, table) + unless reflection.klass.descends_from_active_record? + sti_column = table[reflection.klass.inheritance_column] - conditions = [] + condition = sti_column.eq(reflection.klass.sti_name) - if reflection.source_reflection.nil? - case reflection.macro - when :belongs_to - key = reflection.options[:primary_key] || - reflection.klass.primary_key - foreign_key = reflection.primary_key_name - when :has_many, :has_one - key = reflection.primary_key_name - foreign_key = reflection.options[:primary_key] || - reflection.active_record.primary_key - - if reflection.options[:as] - conditions << - table["#{reflection.options[:as]}_type"]. - eq(reflection.active_record.base_class.name) - end - end - elsif reflection.source_reflection.macro == :belongs_to - key = reflection.klass.primary_key - foreign_key = reflection.source_reflection.primary_key_name - - if reflection.options[:source_type] - conditions << - foreign_table[reflection.source_reflection.options[:foreign_type]]. - eq(reflection.options[:source_type]) - end - else - key = reflection.source_reflection.primary_key_name - foreign_key = reflection.source_reflection.klass.primary_key - end - - conditions << table[key].eq(foreign_table[foreign_key]) - - if reflection.options[:conditions] - conditions << process_conditions(reflection.options[:conditions], aliased_table_name) + reflection.klass.descendants.each do |subclass| + condition = condition.or(sti_column.eq(subclass.sti_name)) end - # If the target table is an STI model then we must be sure to only include records of - # its type and its sub-types. - unless reflection.klass.descends_from_active_record? - sti_column = table[reflection.klass.inheritance_column] - - sti_condition = sti_column.eq(reflection.klass.sti_name) - reflection.klass.descendants.each do |subclass| - sti_condition = sti_condition.or(sti_column.eq(subclass.sti_name)) - end - - conditions << sti_condition - end - - relation = relation.join(table, join_type).on(*conditions) - - # The current table in this iteration becomes the foreign table in the next - foreign_table = table + condition end - - relation end - alias :join_has_one_to :join_has_many_to - - def join_belongs_to_to(relation) - foreign_key = options[:foreign_key] || reflection.primary_key_name - primary_key = options[:primary_key] || reflection.klass.primary_key - - join_target_table( - relation, - target_table[primary_key].eq(parent_table[foreign_key]) - ) - end - end + + def source_type_conditions(reflection, foreign_table) + if reflection.options[:source_type] + foreign_table[reflection.source_reflection.options[:foreign_type]]. + eq(reflection.options[:source_type]) + end + end + + def polymorphic_conditions(reflection, table) + if reflection.options[:as] + table["#{reflection.options[:as]}_type"]. + eq(reflection.active_record.base_class.name) + end + end end + end end end end -- cgit v1.2.3 From 1777600e6e11e553ad97b7bc89e4b19e992eb3d3 Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Tue, 12 Oct 2010 16:40:24 +0100 Subject: Support has_one through assocs as the source association --- .../associations/through_association_scope.rb | 31 +++++++++++++++------- 1 file changed, 21 insertions(+), 10 deletions(-) (limited to 'activerecord/lib/active_record') diff --git a/activerecord/lib/active_record/associations/through_association_scope.rb b/activerecord/lib/active_record/associations/through_association_scope.rb index 8406f5fd20..81e29f047b 100644 --- a/activerecord/lib/active_record/associations/through_association_scope.rb +++ b/activerecord/lib/active_record/associations/through_association_scope.rb @@ -53,7 +53,7 @@ module ActiveRecord end def construct_joins(custom_joins = nil) - # puts @reflection.through_reflection_chain.map(&:inspect) + # p @reflection.through_reflection_chain "#{construct_through_joins} #{@reflection.options[:joins]} #{custom_joins}" end @@ -67,16 +67,27 @@ module ActiveRecord case when left.source_reflection.nil? - left_primary_key = left.primary_key_name - right_primary_key = right.klass.primary_key + # TODO: Perhaps need to pay attention to left.options[:primary_key] and + # left.options[:foreign_key] in places here - if left.options[:as] - polymorphic_join = "AND %s.%s = %s" % [ - table_aliases[left], "#{left.options[:as]}_type", - # TODO: Why right.klass.name? Rather than left.active_record.name? - # TODO: Also should maybe use the base_class (see related code in JoinAssociation) - @owner.class.quote_value(right.klass.name) - ] + case left.macro + when :belongs_to + left_primary_key = left.klass.primary_key + right_primary_key = right.primary_key_name + when :has_many, :has_one + left_primary_key = left.primary_key_name + right_primary_key = right.klass.primary_key + + if left.options[:as] + polymorphic_join = "AND %s.%s = %s" % [ + table_aliases[left], "#{left.options[:as]}_type", + # TODO: Why right.klass.name? Rather than left.active_record.name? + # TODO: Also should maybe use the base_class (see related code in JoinAssociation) + @owner.class.quote_value(right.klass.name) + ] + end + when :has_and_belongs_to_many + raise NotImplementedError end when left.source_reflection.macro == :belongs_to left_primary_key = left.klass.primary_key -- cgit v1.2.3 From dc39aceb94fa810f8d7e263c0293f325fbf9a109 Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Tue, 12 Oct 2010 17:27:10 +0100 Subject: Adding test_has_many_through_has_one_with_has_many_through_source_reflection and modifying ThroughAssociationScope to make it work correctly. --- .../lib/active_record/associations/through_association_scope.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'activerecord/lib/active_record') diff --git a/activerecord/lib/active_record/associations/through_association_scope.rb b/activerecord/lib/active_record/associations/through_association_scope.rb index 81e29f047b..09f92332cf 100644 --- a/activerecord/lib/active_record/associations/through_association_scope.rb +++ b/activerecord/lib/active_record/associations/through_association_scope.rb @@ -73,7 +73,7 @@ module ActiveRecord case left.macro when :belongs_to left_primary_key = left.klass.primary_key - right_primary_key = right.primary_key_name + right_primary_key = left.primary_key_name when :has_many, :has_one left_primary_key = left.primary_key_name right_primary_key = right.klass.primary_key -- cgit v1.2.3 From c37a5e7acde436b359043a67b7daace8be6f08c6 Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Tue, 12 Oct 2010 18:16:31 +0100 Subject: Add a commented, failing test for using a habtm in a has many through association. I want to refactor how aliasing works first. --- .../associations/through_association_scope.rb | 96 +++++++++++----------- 1 file changed, 50 insertions(+), 46 deletions(-) (limited to 'activerecord/lib/active_record') diff --git a/activerecord/lib/active_record/associations/through_association_scope.rb b/activerecord/lib/active_record/associations/through_association_scope.rb index 09f92332cf..8c5b95439e 100644 --- a/activerecord/lib/active_record/associations/through_association_scope.rb +++ b/activerecord/lib/active_record/associations/through_association_scope.rb @@ -65,52 +65,56 @@ module ActiveRecord @reflection.through_reflection_chain.each_cons(2) do |left, right| polymorphic_join = nil - case - when left.source_reflection.nil? - # TODO: Perhaps need to pay attention to left.options[:primary_key] and - # left.options[:foreign_key] in places here - - case left.macro - when :belongs_to - left_primary_key = left.klass.primary_key - right_primary_key = left.primary_key_name - when :has_many, :has_one - left_primary_key = left.primary_key_name - right_primary_key = right.klass.primary_key - - if left.options[:as] - polymorphic_join = "AND %s.%s = %s" % [ - table_aliases[left], "#{left.options[:as]}_type", - # TODO: Why right.klass.name? Rather than left.active_record.name? - # TODO: Also should maybe use the base_class (see related code in JoinAssociation) - @owner.class.quote_value(right.klass.name) - ] - end - when :has_and_belongs_to_many - raise NotImplementedError - end - when left.source_reflection.macro == :belongs_to - left_primary_key = left.klass.primary_key - right_primary_key = left.source_reflection.primary_key_name - - if left.options[:source_type] - polymorphic_join = "AND %s.%s = %s" % [ - table_aliases[right], - left.source_reflection.options[:foreign_type].to_s, - @owner.class.quote_value(left.options[:source_type]) - ] - end - else - left_primary_key = left.source_reflection.primary_key_name - right_primary_key = right.klass.primary_key - - if left.source_reflection.options[:as] - polymorphic_join = "AND %s.%s = %s" % [ - table_aliases[left], - "#{left.source_reflection.options[:as]}_type", - @owner.class.quote_value(right.klass.name) - ] - end + if left.source_reflection.nil? + # TODO: Perhaps need to pay attention to left.options[:primary_key] and + # left.options[:foreign_key] in places here + + case left.macro + when :belongs_to + left_primary_key = left.klass.primary_key + right_primary_key = left.primary_key_name + when :has_many, :has_one + left_primary_key = left.primary_key_name + right_primary_key = right.klass.primary_key + + if left.options[:as] + polymorphic_join = "AND %s.%s = %s" % [ + table_aliases[left], "#{left.options[:as]}_type", + # TODO: Why right.klass.name? Rather than left.active_record.name? + # TODO: Also should maybe use the base_class (see related code in JoinAssociation) + @owner.class.quote_value(right.klass.name) + ] + end + when :has_and_belongs_to_many + raise NotImplementedError + end + else + case left.source_reflection.macro + when :belongs_to + left_primary_key = left.klass.primary_key + right_primary_key = left.source_reflection.primary_key_name + + if left.options[:source_type] + polymorphic_join = "AND %s.%s = %s" % [ + table_aliases[right], + left.source_reflection.options[:foreign_type].to_s, + @owner.class.quote_value(left.options[:source_type]) + ] + end + when :has_many, :has_one + left_primary_key = left.source_reflection.primary_key_name + right_primary_key = right.klass.primary_key + + if left.source_reflection.options[:as] + polymorphic_join = "AND %s.%s = %s" % [ + table_aliases[left], + "#{left.source_reflection.options[:as]}_type", + @owner.class.quote_value(right.klass.name) + ] + end + when :has_and_belongs_to_many + raise NotImplementedError + end end if right.quoted_table_name == table_aliases[right] -- cgit v1.2.3 From e8874318b7a025ffd30df1a53c403eb9d8912c9f Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Tue, 12 Oct 2010 19:49:32 +0100 Subject: Extract aliasing code from JoinDependency and JoinAssociation into a separate AliasTracker class. This can then be used by ThroughAssociationScope as well. --- activerecord/lib/active_record/associations.rb | 56 +++++------------- .../active_record/associations/alias_tracker.rb | 68 ++++++++++++++++++++++ 2 files changed, 83 insertions(+), 41 deletions(-) create mode 100644 activerecord/lib/active_record/associations/alias_tracker.rb (limited to 'activerecord/lib/active_record') diff --git a/activerecord/lib/active_record/associations.rb b/activerecord/lib/active_record/associations.rb index 397159d35e..d0d1eeec45 100644 --- a/activerecord/lib/active_record/associations.rb +++ b/activerecord/lib/active_record/associations.rb @@ -114,6 +114,7 @@ module ActiveRecord autoload :NestedHasManyThroughAssociation, 'active_record/associations/nested_has_many_through_association' autoload :HasOneAssociation, 'active_record/associations/has_one_association' autoload :HasOneThroughAssociation, 'active_record/associations/has_one_through_association' + autoload :AliasTracker, 'active_record/associations/alias_tracker' # Clears out the association cache. def clear_association_cache #:nodoc: @@ -1834,7 +1835,7 @@ module ActiveRecord end class JoinDependency # :nodoc: - attr_reader :join_parts, :reflections, :table_aliases + attr_reader :join_parts, :reflections, :alias_tracker def initialize(base, associations, joins) @join_parts = [JoinBase.new(base, joins)] @@ -1842,8 +1843,8 @@ module ActiveRecord @reflections = [] @base_records_hash = {} @base_records_in_order = [] - @table_aliases = Hash.new(0) - @table_aliases[base.table_name] = 1 + @alias_tracker = AliasTracker.new(joins) + @alias_tracker.aliased_name_for(base.table_name) # Updates the count for base.table_name to 1 build(associations) end @@ -1863,17 +1864,6 @@ module ActiveRecord join_parts.first end - def count_aliases_from_table_joins(name) - # quoted_name should be downcased as some database adapters (Oracle) return quoted name in uppercase - quoted_name = join_base.active_record.connection.quote_table_name(name.downcase).downcase - join_sql = join_base.table_joins.to_s.downcase - join_sql.blank? ? 0 : - # Table names - join_sql.scan(/join(?:\s+\w+)?\s+#{quoted_name}\son/).size + - # Table aliases - join_sql.scan(/join(?:\s+\w+)?\s+\S+\s+#{quoted_name}\son/).size - end - def instantiate(rows) rows.each_with_index do |row, i| primary_id = join_base.record_id(row) @@ -2130,6 +2120,7 @@ module ActiveRecord delegate :options, :through_reflection, :source_reflection, :through_reflection_chain, :to => :reflection delegate :table, :table_name, :to => :parent, :prefix => true + delegate :alias_tracker, :to => :join_dependency def initialize(reflection, join_dependency, parent = nil) reflection.check_validity! @@ -2248,24 +2239,10 @@ module ActiveRecord protected - def aliased_table_name_for(name, aliased_name, suffix = nil) - if @join_dependency.table_aliases[name].zero? - @join_dependency.table_aliases[name] = @join_dependency.count_aliases_from_table_joins(name) - end - - if !@join_dependency.table_aliases[name].zero? # We need an alias - name = active_record.connection.table_alias_for "#{aliased_name}_#{parent_table_name}#{suffix}" - @join_dependency.table_aliases[name] += 1 - if @join_dependency.table_aliases[name] == 1 # First time we've seen this name - # Also need to count the aliases from the table_aliases to avoid incorrect count - @join_dependency.table_aliases[name] += @join_dependency.count_aliases_from_table_joins(name) - end - table_index = @join_dependency.table_aliases[name] - name = name[0..active_record.connection.table_alias_length-3] + "_#{table_index}" if table_index > 1 - else - @join_dependency.table_aliases[name] += 1 - end - + def table_alias_for(reflection) + name = pluralize(reflection.name) + name << "_#{parent_table_name}" + name << "_join" if reflection != self.reflection name end @@ -2273,12 +2250,12 @@ module ActiveRecord ActiveRecord::Base.pluralize_table_names ? table_name.to_s.pluralize : table_name end - def table_alias_for(table_name, table_alias) + def table_name_and_alias_for(table_name, table_alias) "#{table_name} #{table_alias if table_name != table_alias}".strip end def table_name_and_alias - table_alias_for table_name, aliased_table_name + table_name_and_alias_for(table_name, aliased_table_name) end def interpolate_sql(sql) @@ -2292,12 +2269,9 @@ module ActiveRecord # the proper alias. def setup_tables @tables = through_reflection_chain.map do |reflection| - suffix = reflection == self.reflection ? nil : '_join' - - aliased_table_name = aliased_table_name_for( + aliased_table_name = alias_tracker.aliased_name_for( reflection.table_name, - pluralize(reflection.name), - suffix + table_alias_for(reflection) ) table = Arel::Table.new( @@ -2308,9 +2282,9 @@ module ActiveRecord # For habtm, we have two Arel::Table instances related to a single reflection, so # we just store them as a pair in the array. if reflection.macro == :has_and_belongs_to_many - aliased_join_table_name = aliased_table_name_for( + aliased_join_table_name = alias_tracker.aliased_name_for( reflection.options[:join_table], - pluralize(reflection.name), "_join" + table_alias_for(reflection) ) join_table = Arel::Table.new( diff --git a/activerecord/lib/active_record/associations/alias_tracker.rb b/activerecord/lib/active_record/associations/alias_tracker.rb new file mode 100644 index 0000000000..f48efabec2 --- /dev/null +++ b/activerecord/lib/active_record/associations/alias_tracker.rb @@ -0,0 +1,68 @@ +require 'active_support/core_ext/string/conversions' + +module ActiveRecord + module Associations + # Keeps track of table aliases for ActiveRecord::Associations::ClassMethods::JoinDependency + class AliasTracker # :nodoc: + # other_sql is some other sql which might conflict with the aliases we assign here. Therefore + # we store other_sql so that we can scan it before assigning a specific name. + def initialize(other_sql) + @aliases = Hash.new + @other_sql = other_sql.to_s.downcase + end + + def aliased_name_for(table_name, aliased_name = nil) + aliased_name ||= table_name + + initialize_count_for(table_name) if @aliases[table_name].nil? + + if @aliases[table_name].zero? + # If it's zero, we can have our table_name + @aliases[table_name] = 1 + table_name + else + # Otherwise, we need to use an alias + aliased_name = connection.table_alias_for(aliased_name) + + initialize_count_for(aliased_name) if @aliases[aliased_name].nil? + + # Update the count + @aliases[aliased_name] += 1 + + if @aliases[aliased_name] > 1 + "#{truncate(aliased_name)}_#{@aliases[aliased_name]}" + else + aliased_name + end + end + end + + private + + def initialize_count_for(name) + @aliases[name] = 0 + + unless @other_sql.blank? + # quoted_name should be downcased as some database adapters (Oracle) return quoted name in uppercase + quoted_name = connection.quote_table_name(name.downcase).downcase + + # Table names + @aliases[name] += @other_sql.scan(/join(?:\s+\w+)?\s+#{quoted_name}\son/).size + + # Table aliases + @aliases[name] += @other_sql.scan(/join(?:\s+\w+)?\s+\S+\s+#{quoted_name}\son/).size + end + + @aliases[name] + end + + def truncate(name) + name[0..connection.table_alias_length-3] + end + + def connection + ActiveRecord::Base.connection + end + end + end +end -- cgit v1.2.3 From 3f2e25805d56440a4ef2a7a9ae6b99be04e6357b Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Tue, 12 Oct 2010 23:42:30 +0100 Subject: Some small tweaks on the last commit --- activerecord/lib/active_record/associations.rb | 22 +++++----------------- .../active_record/associations/alias_tracker.rb | 9 +++++++-- 2 files changed, 12 insertions(+), 19 deletions(-) (limited to 'activerecord/lib/active_record') diff --git a/activerecord/lib/active_record/associations.rb b/activerecord/lib/active_record/associations.rb index d0d1eeec45..41f882743c 100644 --- a/activerecord/lib/active_record/associations.rb +++ b/activerecord/lib/active_record/associations.rb @@ -2239,25 +2239,13 @@ module ActiveRecord protected - def table_alias_for(reflection) - name = pluralize(reflection.name) + def table_alias_for(reflection, join = false) + name = alias_tracker.pluralize(reflection.name) name << "_#{parent_table_name}" - name << "_join" if reflection != self.reflection + name << "_join" if join name end - def pluralize(table_name) - ActiveRecord::Base.pluralize_table_names ? table_name.to_s.pluralize : table_name - end - - def table_name_and_alias_for(table_name, table_alias) - "#{table_name} #{table_alias if table_name != table_alias}".strip - end - - def table_name_and_alias - table_name_and_alias_for(table_name, aliased_table_name) - end - def interpolate_sql(sql) instance_eval("%@#{sql.gsub('@', '\@')}@", __FILE__, __LINE__) end @@ -2271,7 +2259,7 @@ module ActiveRecord @tables = through_reflection_chain.map do |reflection| aliased_table_name = alias_tracker.aliased_name_for( reflection.table_name, - table_alias_for(reflection) + table_alias_for(reflection, reflection != self.reflection) ) table = Arel::Table.new( @@ -2284,7 +2272,7 @@ module ActiveRecord if reflection.macro == :has_and_belongs_to_many aliased_join_table_name = alias_tracker.aliased_name_for( reflection.options[:join_table], - table_alias_for(reflection) + table_alias_for(reflection, true) ) join_table = Arel::Table.new( diff --git a/activerecord/lib/active_record/associations/alias_tracker.rb b/activerecord/lib/active_record/associations/alias_tracker.rb index f48efabec2..10e90ec117 100644 --- a/activerecord/lib/active_record/associations/alias_tracker.rb +++ b/activerecord/lib/active_record/associations/alias_tracker.rb @@ -2,11 +2,12 @@ require 'active_support/core_ext/string/conversions' module ActiveRecord module Associations - # Keeps track of table aliases for ActiveRecord::Associations::ClassMethods::JoinDependency + # Keeps track of table aliases for ActiveRecord::Associations::ClassMethods::JoinDependency and + # ActiveRecord::Associations::ThroughAssociationScope class AliasTracker # :nodoc: # other_sql is some other sql which might conflict with the aliases we assign here. Therefore # we store other_sql so that we can scan it before assigning a specific name. - def initialize(other_sql) + def initialize(other_sql = nil) @aliases = Hash.new @other_sql = other_sql.to_s.downcase end @@ -36,6 +37,10 @@ module ActiveRecord end end end + + def pluralize(table_name) + ActiveRecord::Base.pluralize_table_names ? table_name.to_s.pluralize : table_name + end private -- cgit v1.2.3 From 199db8c8c006a5f3bcbbe2a32d39444a741c5843 Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Wed, 13 Oct 2010 00:05:04 +0100 Subject: Hook ThroughAssociationScope up to use the AliasTracker class --- .../associations/through_association_scope.rb | 39 +++++++++++++++++----- 1 file changed, 30 insertions(+), 9 deletions(-) (limited to 'activerecord/lib/active_record') diff --git a/activerecord/lib/active_record/associations/through_association_scope.rb b/activerecord/lib/active_record/associations/through_association_scope.rb index 8c5b95439e..d73f35c2db 100644 --- a/activerecord/lib/active_record/associations/through_association_scope.rb +++ b/activerecord/lib/active_record/associations/through_association_scope.rb @@ -20,6 +20,7 @@ module ActiveRecord end # Build SQL conditions from attributes, qualified by table name. + # TODO: Conditions on joins def construct_conditions reflection = @reflection.through_reflection_chain.last conditions = construct_quoted_owner_attributes(reflection).map do |attr, value| @@ -134,24 +135,44 @@ module ActiveRecord joins.join(" ") end - # TODO: Use the same aliasing strategy (and code?) as JoinAssociation (as this is the - # documented behaviour) + def alias_tracker + @alias_tracker ||= AliasTracker.new + end + def table_aliases @table_aliases ||= begin - tally = {} @reflection.through_reflection_chain.inject({}) do |aliases, reflection| - if tally[reflection.table_name].nil? - tally[reflection.table_name] = 1 - aliases[reflection] = reflection.quoted_table_name + table_alias = quote_table_name(alias_tracker.aliased_name_for( + reflection.table_name, + table_alias_for(reflection, reflection != @reflection) + )) + + if reflection.macro == :has_and_belongs_to_many + join_table_alias = quote_table_name(alias_tracker.aliased_name_for( + reflection.options[:join_table], + table_alias_for(reflection, true) + )) + + aliases[reflection] = [table_alias, join_table_alias] else - tally[reflection.table_name] += 1 - aliased_table_name = reflection.table_name + "_#{tally[reflection.table_name]}" - aliases[reflection] = reflection.klass.connection.quote_table_name(aliased_table_name) + aliases[reflection] = table_alias end + aliases end end end + + def table_alias_for(reflection, join = false) + name = alias_tracker.pluralize(reflection.name) + name << "_#{@reflection.name}" + name << "_join" if join + name + end + + def quote_table_name(table_name) + @reflection.klass.connection.quote_table_name(table_name) + end # Construct attributes for associate pointing to owner. def construct_owner_attributes(reflection) -- cgit v1.2.3 From 781ad0f8fee209bcf10c5e52daae246477d49ea7 Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Wed, 13 Oct 2010 01:29:09 +0100 Subject: First bit of support for habtm in through assocs - test_has_many_through_has_many_with_has_and_belongs_to_many_source_reflection now passes --- activerecord/lib/active_record/associations.rb | 45 +++++++++++++++++----- .../associations/through_association_scope.rb | 45 +++++++++++++++------- 2 files changed, 67 insertions(+), 23 deletions(-) (limited to 'activerecord/lib/active_record') diff --git a/activerecord/lib/active_record/associations.rb b/activerecord/lib/active_record/associations.rb index 41f882743c..2a72fa95c9 100644 --- a/activerecord/lib/active_record/associations.rb +++ b/activerecord/lib/active_record/associations.rb @@ -2180,6 +2180,7 @@ module ActiveRecord # to represent the join table) table, join_table = table + # TODO: Can join_key just be reflection.primary_key_name ? join_key = reflection.options[:foreign_key] || reflection.active_record.to_s.foreign_key join_foreign_key = reflection.active_record.primary_key @@ -2192,18 +2193,37 @@ module ActiveRecord # We've done the first join now, so update the foreign_table for the second foreign_table = join_table + # TODO: Can foreign_key be reflection.association_foreign_key? key = reflection.klass.primary_key foreign_key = reflection.options[:association_foreign_key] || reflection.klass.to_s.foreign_key end - elsif reflection.source_reflection.macro == :belongs_to - key = reflection.klass.primary_key - foreign_key = reflection.source_reflection.primary_key_name - - conditions << source_type_conditions(reflection, foreign_table) else - key = reflection.source_reflection.primary_key_name - foreign_key = reflection.source_reflection.klass.primary_key + case reflection.source_reflection.macro + when :belongs_to + key = reflection.klass.primary_key + foreign_key = reflection.source_reflection.primary_key_name + + conditions << source_type_conditions(reflection, foreign_table) + when :has_many, :has_one + key = reflection.source_reflection.primary_key_name + foreign_key = reflection.source_reflection.klass.primary_key + when :has_and_belongs_to_many + table, join_table = table + + join_key = reflection.source_reflection.primary_key_name + join_foreign_key = reflection.source_reflection.klass.primary_key + + relation = relation.join(join_table, join_type).on( + join_table[join_key]. + eq(foreign_table[join_foreign_key]) + ) + + foreign_table = join_table + + key = reflection.klass.primary_key + foreign_key = reflection.source_reflection.association_foreign_key + end end conditions << table[key].eq(foreign_table[foreign_key]) @@ -2269,14 +2289,19 @@ module ActiveRecord # For habtm, we have two Arel::Table instances related to a single reflection, so # we just store them as a pair in the array. - if reflection.macro == :has_and_belongs_to_many + if reflection.macro == :has_and_belongs_to_many || + (reflection.source_reflection && + reflection.source_reflection.macro == :has_and_belongs_to_many) + + join_table_name = (reflection.source_reflection || reflection).options[:join_table] + aliased_join_table_name = alias_tracker.aliased_name_for( - reflection.options[:join_table], + join_table_name, table_alias_for(reflection, true) ) join_table = Arel::Table.new( - reflection.options[:join_table], :engine => arel_engine, + join_table_name, :engine => arel_engine, :as => aliased_join_table_name ) diff --git a/activerecord/lib/active_record/associations/through_association_scope.rb b/activerecord/lib/active_record/associations/through_association_scope.rb index d73f35c2db..6cc2fe2559 100644 --- a/activerecord/lib/active_record/associations/through_association_scope.rb +++ b/activerecord/lib/active_record/associations/through_association_scope.rb @@ -65,6 +65,7 @@ module ActiveRecord # Iterate over each pair in the through reflection chain, joining them together @reflection.through_reflection_chain.each_cons(2) do |left, right| polymorphic_join = nil + left_table, right_table = table_aliases[left], table_aliases[right] if left.source_reflection.nil? # TODO: Perhaps need to pay attention to left.options[:primary_key] and @@ -114,20 +115,31 @@ module ActiveRecord ] end when :has_and_belongs_to_many - raise NotImplementedError + join_table, left_table = left_table + + left_primary_key = left.klass.primary_key + join_primary_key = left.source_reflection.association_foreign_key + + joins << "INNER JOIN %s ON %s.%s = %s.%s" % [ + table_name_and_alias( + quote_table_name(left.source_reflection.options[:join_table]), + join_table + ), + left_table, left_primary_key, + join_table, join_primary_key + ] + + left_table = join_table + + left_primary_key = left.source_reflection.primary_key_name + right_primary_key = right.klass.primary_key end end - if right.quoted_table_name == table_aliases[right] - table = right.quoted_table_name - else - table = "#{right.quoted_table_name} #{table_aliases[right]}" - end - joins << "INNER JOIN %s ON %s.%s = %s.%s %s" % [ - table, - table_aliases[left], left_primary_key, - table_aliases[right], right_primary_key, + table_name_and_alias(right.quoted_table_name, right_table), + left_table, left_primary_key, + right_table, right_primary_key, polymorphic_join ] end @@ -147,13 +159,16 @@ module ActiveRecord table_alias_for(reflection, reflection != @reflection) )) - if reflection.macro == :has_and_belongs_to_many + if reflection.macro == :has_and_belongs_to_many || + (reflection.source_reflection && + reflection.source_reflection.macro == :has_and_belongs_to_many) + join_table_alias = quote_table_name(alias_tracker.aliased_name_for( - reflection.options[:join_table], + (reflection.source_reflection || reflection).options[:join_table], table_alias_for(reflection, true) )) - aliases[reflection] = [table_alias, join_table_alias] + aliases[reflection] = [join_table_alias, table_alias] else aliases[reflection] = table_alias end @@ -173,6 +188,10 @@ module ActiveRecord def quote_table_name(table_name) @reflection.klass.connection.quote_table_name(table_name) end + + def table_name_and_alias(table_name, table_alias) + "#{table_name} #{table_alias if table_alias != table_name}".strip + end # Construct attributes for associate pointing to owner. def construct_owner_attributes(reflection) -- cgit v1.2.3 From 5d8bb060909339d858151ca24bf764c642bf2b12 Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Wed, 13 Oct 2010 17:55:41 +0100 Subject: Refactoring ThroughAssociationScope#construct_through_joins --- .../associations/through_association_scope.rb | 116 +++++++++++---------- 1 file changed, 62 insertions(+), 54 deletions(-) (limited to 'activerecord/lib/active_record') diff --git a/activerecord/lib/active_record/associations/through_association_scope.rb b/activerecord/lib/active_record/associations/through_association_scope.rb index 6cc2fe2559..25fde49650 100644 --- a/activerecord/lib/active_record/associations/through_association_scope.rb +++ b/activerecord/lib/active_record/associations/through_association_scope.rb @@ -64,8 +64,7 @@ module ActiveRecord # Iterate over each pair in the through reflection chain, joining them together @reflection.through_reflection_chain.each_cons(2) do |left, right| - polymorphic_join = nil - left_table, right_table = table_aliases[left], table_aliases[right] + right_table_and_alias = table_name_and_alias(right.quoted_table_name, table_aliases[right]) if left.source_reflection.nil? # TODO: Perhaps need to pay attention to left.options[:primary_key] and @@ -73,75 +72,56 @@ module ActiveRecord case left.macro when :belongs_to - left_primary_key = left.klass.primary_key - right_primary_key = left.primary_key_name + joins << inner_join_sql( + right_table_and_alias, + table_aliases[left], left.klass.primary_key, + table_aliases[right], left.primary_key_name + ) when :has_many, :has_one - left_primary_key = left.primary_key_name - right_primary_key = right.klass.primary_key - - if left.options[:as] - polymorphic_join = "AND %s.%s = %s" % [ - table_aliases[left], "#{left.options[:as]}_type", - # TODO: Why right.klass.name? Rather than left.active_record.name? - # TODO: Also should maybe use the base_class (see related code in JoinAssociation) - @owner.class.quote_value(right.klass.name) - ] - end + joins << inner_join_sql( + right_table_and_alias, + table_aliases[left], left.primary_key_name, + table_aliases[right], right.klass.primary_key, + polymorphic_conditions(left, left.options[:as]) + ) when :has_and_belongs_to_many raise NotImplementedError end else case left.source_reflection.macro when :belongs_to - left_primary_key = left.klass.primary_key - right_primary_key = left.source_reflection.primary_key_name - - if left.options[:source_type] - polymorphic_join = "AND %s.%s = %s" % [ - table_aliases[right], - left.source_reflection.options[:foreign_type].to_s, - @owner.class.quote_value(left.options[:source_type]) - ] - end + joins << inner_join_sql( + right_table_and_alias, + table_aliases[left], left.klass.primary_key, + table_aliases[right], left.source_reflection.primary_key_name, + source_type_conditions(left) + ) when :has_many, :has_one - left_primary_key = left.source_reflection.primary_key_name - right_primary_key = right.klass.primary_key - - if left.source_reflection.options[:as] - polymorphic_join = "AND %s.%s = %s" % [ - table_aliases[left], - "#{left.source_reflection.options[:as]}_type", - @owner.class.quote_value(right.klass.name) - ] - end + joins << inner_join_sql( + right_table_and_alias, + table_aliases[left], left.source_reflection.primary_key_name, + table_aliases[right], right.klass.primary_key, + polymorphic_conditions(left, left.source_reflection.options[:as]) + ) when :has_and_belongs_to_many - join_table, left_table = left_table + join_table, left_table = table_aliases[left] - left_primary_key = left.klass.primary_key - join_primary_key = left.source_reflection.association_foreign_key - - joins << "INNER JOIN %s ON %s.%s = %s.%s" % [ + joins << inner_join_sql( table_name_and_alias( quote_table_name(left.source_reflection.options[:join_table]), join_table ), - left_table, left_primary_key, - join_table, join_primary_key - ] - - left_table = join_table + left_table, left.klass.primary_key, + join_table, left.source_reflection.association_foreign_key + ) - left_primary_key = left.source_reflection.primary_key_name - right_primary_key = right.klass.primary_key + joins << inner_join_sql( + right_table_and_alias, + join_table, left.source_reflection.primary_key_name, + table_aliases[right], right.klass.primary_key + ) end end - - joins << "INNER JOIN %s ON %s.%s = %s.%s %s" % [ - table_name_and_alias(right.quoted_table_name, right_table), - left_table, left_primary_key, - right_table, right_primary_key, - polymorphic_join - ] end joins.join(" ") @@ -192,6 +172,34 @@ module ActiveRecord def table_name_and_alias(table_name, table_alias) "#{table_name} #{table_alias if table_alias != table_name}".strip end + + def inner_join_sql(table, on_left_table, on_left_key, on_right_table, on_right_key, conds = nil) + "INNER JOIN %s ON %s.%s = %s.%s %s" % [ + table, + on_left_table, on_left_key, + on_right_table, on_right_key, + conds + ] + end + + def polymorphic_conditions(reflection, interface_name) + if interface_name + "AND %s.%s = %s" % [ + table_aliases[reflection], "#{interface_name}_type", + @owner.class.quote_value(reflection.active_record.base_class.name) + ] + end + end + + def source_type_conditions(reflection) + if reflection.options[:source_type] + "AND %s.%s = %s" % [ + table_aliases[reflection.through_reflection], + reflection.source_reflection.options[:foreign_type].to_s, + @owner.class.quote_value(reflection.options[:source_type]) + ] + end + end # Construct attributes for associate pointing to owner. def construct_owner_attributes(reflection) -- cgit v1.2.3 From 212fdd8ba9624f61421a7a950283537a3d39ac18 Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Wed, 13 Oct 2010 18:36:51 +0100 Subject: Add test_has_many_through_has_and_belongs_to_many_with_has_many_source_reflection and make it work --- .../associations/through_association_scope.rb | 31 +++++++++++++++++++--- 1 file changed, 28 insertions(+), 3 deletions(-) (limited to 'activerecord/lib/active_record') diff --git a/activerecord/lib/active_record/associations/through_association_scope.rb b/activerecord/lib/active_record/associations/through_association_scope.rb index 25fde49650..582474355e 100644 --- a/activerecord/lib/active_record/associations/through_association_scope.rb +++ b/activerecord/lib/active_record/associations/through_association_scope.rb @@ -23,8 +23,15 @@ module ActiveRecord # TODO: Conditions on joins def construct_conditions reflection = @reflection.through_reflection_chain.last + + if reflection.macro == :has_and_belongs_to_many + table_alias = table_aliases[reflection].first + else + table_alias = table_aliases[reflection] + end + conditions = construct_quoted_owner_attributes(reflection).map do |attr, value| - "#{table_aliases[reflection]}.#{attr} = #{value}" + "#{table_alias}.#{attr} = #{value}" end conditions << sql_conditions if sql_conditions "(" + conditions.join(') AND (') + ")" @@ -97,12 +104,30 @@ module ActiveRecord source_type_conditions(left) ) when :has_many, :has_one + if right.macro == :has_and_belongs_to_many + join_table, right_table = table_aliases[right] + right_table_and_alias = table_name_and_alias(right.quoted_table_name, right_table) + else + right_table = table_aliases[right] + end + joins << inner_join_sql( right_table_and_alias, - table_aliases[left], left.source_reflection.primary_key_name, - table_aliases[right], right.klass.primary_key, + table_aliases[left], left.source_reflection.primary_key_name, + right_table, right.klass.primary_key, polymorphic_conditions(left, left.source_reflection.options[:as]) ) + + if right.macro == :has_and_belongs_to_many + joins << inner_join_sql( + table_name_and_alias( + quote_table_name(right.options[:join_table]), + join_table + ), + right_table, right.klass.primary_key, + join_table, right.association_foreign_key + ) + end when :has_and_belongs_to_many join_table, left_table = table_aliases[left] -- cgit v1.2.3 From 22782e2cc131863b72e457636f9a995a6ae50136 Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Thu, 14 Oct 2010 12:31:35 +0100 Subject: Fix bug in previous refactoring --- .../active_record/associations/through_association_scope.rb | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) (limited to 'activerecord/lib/active_record') diff --git a/activerecord/lib/active_record/associations/through_association_scope.rb b/activerecord/lib/active_record/associations/through_association_scope.rb index 582474355e..c3f12fee2b 100644 --- a/activerecord/lib/active_record/associations/through_association_scope.rb +++ b/activerecord/lib/active_record/associations/through_association_scope.rb @@ -89,7 +89,7 @@ module ActiveRecord right_table_and_alias, table_aliases[left], left.primary_key_name, table_aliases[right], right.klass.primary_key, - polymorphic_conditions(left, left.options[:as]) + polymorphic_conditions(left, left) ) when :has_and_belongs_to_many raise NotImplementedError @@ -115,7 +115,7 @@ module ActiveRecord right_table_and_alias, table_aliases[left], left.source_reflection.primary_key_name, right_table, right.klass.primary_key, - polymorphic_conditions(left, left.source_reflection.options[:as]) + polymorphic_conditions(left, left.source_reflection) ) if right.macro == :has_and_belongs_to_many @@ -207,11 +207,11 @@ module ActiveRecord ] end - def polymorphic_conditions(reflection, interface_name) - if interface_name + def polymorphic_conditions(reflection, polymorphic_reflection) + if polymorphic_reflection.options[:as] "AND %s.%s = %s" % [ - table_aliases[reflection], "#{interface_name}_type", - @owner.class.quote_value(reflection.active_record.base_class.name) + table_aliases[reflection], "#{polymorphic_reflection.options[:as]}_type", + @owner.class.quote_value(polymorphic_reflection.active_record.base_class.name) ] end end -- cgit v1.2.3 From bc821a56114ae6f6d0b595475ad9e71f01f46f35 Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Thu, 14 Oct 2010 12:59:16 +0100 Subject: Added test_has_many_through_has_many_with_has_many_through_habtm_source_reflection and make it pass --- .../lib/active_record/associations/through_association_scope.rb | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) (limited to 'activerecord/lib/active_record') diff --git a/activerecord/lib/active_record/associations/through_association_scope.rb b/activerecord/lib/active_record/associations/through_association_scope.rb index c3f12fee2b..a52672eecd 100644 --- a/activerecord/lib/active_record/associations/through_association_scope.rb +++ b/activerecord/lib/active_record/associations/through_association_scope.rb @@ -92,7 +92,11 @@ module ActiveRecord polymorphic_conditions(left, left) ) when :has_and_belongs_to_many - raise NotImplementedError + joins << inner_join_sql( + right_table_and_alias, + table_aliases[left].first, left.primary_key_name, + table_aliases[right], right.klass.primary_key + ) end else case left.source_reflection.macro @@ -106,7 +110,7 @@ module ActiveRecord when :has_many, :has_one if right.macro == :has_and_belongs_to_many join_table, right_table = table_aliases[right] - right_table_and_alias = table_name_and_alias(right.quoted_table_name, right_table) + right_table_and_alias = table_name_and_alias(right.quoted_table_name, right_table) else right_table = table_aliases[right] end -- cgit v1.2.3 From 06c64eb60611bdeeb55e35a4819ba65d74dbadc3 Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Fri, 15 Oct 2010 15:46:19 +0100 Subject: Support preloading nested through associations (using the default multi-query strategy) --- .../lib/active_record/association_preload.rb | 137 ++++++++++++--------- 1 file changed, 76 insertions(+), 61 deletions(-) (limited to 'activerecord/lib/active_record') diff --git a/activerecord/lib/active_record/association_preload.rb b/activerecord/lib/active_record/association_preload.rb index e6b367790b..664b0a7d59 100644 --- a/activerecord/lib/active_record/association_preload.rb +++ b/activerecord/lib/active_record/association_preload.rb @@ -202,93 +202,108 @@ module ActiveRecord set_association_collection_records(id_to_record_map, reflection.name, associated_records, 'the_parent_record_id') end - def preload_has_one_association(records, reflection, preload_options={}) - return if records.first.send("loaded_#{reflection.name}?") - id_to_record_map, ids = construct_id_map(records, reflection.options[:primary_key]) - options = reflection.options - records.each {|record| record.send("set_#{reflection.name}_target", nil)} - if options[:through] - through_records = preload_through_records(records, reflection, options[:through]) - - unless through_records.empty? - through_reflection = reflections[options[:through]] - through_primary_key = through_reflection.primary_key_name - source = reflection.source_reflection.name - through_records.first.class.preload_associations(through_records, source) - if through_reflection.macro == :belongs_to - id_to_record_map = construct_id_map(records, through_primary_key).first - through_primary_key = through_reflection.klass.primary_key - end - - through_records.each do |through_record| - add_preloaded_record_to_collection(id_to_record_map[through_record[through_primary_key].to_s], - reflection.name, through_record.send(source)) - end - end + def preload_has_one_or_has_many_association(records, reflection, preload_options={}) + if reflection.macro == :has_many + return if records.first.send(reflection.name).loaded? + records.each { |record| record.send(reflection.name).loaded } else - set_association_single_records(id_to_record_map, reflection.name, find_associated_records(ids, reflection, preload_options), reflection.primary_key_name) + return if records.first.send("loaded_#{reflection.name}?") + records.each {|record| record.send("set_#{reflection.name}_target", nil)} end - end - - def preload_has_many_association(records, reflection, preload_options={}) - return if records.first.send(reflection.name).loaded? + options = reflection.options - - primary_key_name = reflection.through_reflection_primary_key_name - id_to_record_map, ids = construct_id_map(records, primary_key_name || reflection.options[:primary_key]) - records.each {|record| record.send(reflection.name).loaded} - + if options[:through] - through_records = preload_through_records(records, reflection, options[:through]) - unless through_records.empty? + records_with_through_records = preload_through_records(records, reflection, options[:through]) + all_through_records = records_with_through_records.map(&:last).flatten + + unless all_through_records.empty? source = reflection.source_reflection.name - through_records.first.class.preload_associations(through_records, source, options) - through_records.each do |through_record| - through_record_id = through_record[reflection.through_reflection_primary_key].to_s - add_preloaded_records_to_collection(id_to_record_map[through_record_id], reflection.name, through_record.send(source)) + all_through_records.first.class.preload_associations(all_through_records, source, options) + + records_with_through_records.each do |record, through_records| + source_records = through_records.map(&source).flatten.compact + + case reflection.macro + when :has_many, :has_and_belongs_to_many + add_preloaded_records_to_collection([record], reflection.name, source_records) + when :has_one, :belongs_to + add_preloaded_record_to_collection([record], reflection.name, source_records.first) + end end end - else - set_association_collection_records(id_to_record_map, reflection.name, find_associated_records(ids, reflection, preload_options), - reflection.primary_key_name) + id_to_record_map, ids = construct_id_map(records, reflection.options[:primary_key]) + associated_records = find_associated_records(ids, reflection, preload_options) + + if reflection.macro == :has_many + set_association_collection_records( + id_to_record_map, reflection.name, + associated_records, reflection.primary_key_name + ) + else + set_association_single_records( + id_to_record_map, reflection.name, + associated_records, reflection.primary_key_name + ) + end end end + + alias_method :preload_has_one_association, :preload_has_one_or_has_many_association + alias_method :preload_has_many_association, :preload_has_one_or_has_many_association def preload_through_records(records, reflection, through_association) through_reflection = reflections[through_association] - through_records = [] + # If the same through record is loaded twice, we want to return exactly the same + # object in the result, rather than two separate instances representing the same + # record. This is so that we can preload the source association for each record, + # and always be able to access the preloaded association regardless of where we + # refer to the record. + # + # Suffices to say, if AR had an identity map built in then this would be unnecessary. + identity_map = {} + + options = {} + if reflection.options[:source_type] interface = reflection.source_reflection.options[:foreign_type] - preload_options = {:conditions => ["#{connection.quote_column_name interface} = ?", reflection.options[:source_type]]} - + options[:conditions] = ["#{connection.quote_column_name interface} = ?", reflection.options[:source_type]] records.compact! - records.first.class.preload_associations(records, through_association, preload_options) + else + if reflection.options[:conditions] + options[:include] = reflection.options[:include] || + reflection.options[:source] + options[:conditions] = reflection.options[:conditions] + end + + options[:order] = reflection.options[:order] + end + + records.first.class.preload_associations(records, through_association, options) - # Dont cache the association - we would only be caching a subset - records.each do |record| + records.map do |record| + if reflection.options[:source_type] + # Dont cache the association - we would only be caching a subset proxy = record.send(through_association) - + if proxy.respond_to?(:target) - through_records.concat Array.wrap(proxy.target) + through_records = proxy.target proxy.reset else # this is a has_one :through reflection - through_records << proxy if proxy + through_records = proxy end + else + through_records = record.send(through_association) end - else - options = {} - options[:include] = reflection.options[:include] || reflection.options[:source] if reflection.options[:conditions] - options[:order] = reflection.options[:order] - options[:conditions] = reflection.options[:conditions] - records.first.class.preload_associations(records, through_association, options) - - records.each do |record| - through_records.concat Array.wrap(record.send(through_association)) + + through_records = Array.wrap(through_records).map do |through_record| + identity_map[through_record] ||= through_record end + + [record, through_records] end - through_records end def preload_belongs_to_association(records, reflection, preload_options={}) -- cgit v1.2.3 From d619e399380cd840f9f5ec88bb3d823fbb1f4d08 Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Fri, 15 Oct 2010 16:27:13 +0100 Subject: Fix small bug which was shown by the last commit --- activerecord/lib/active_record/associations.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'activerecord/lib/active_record') diff --git a/activerecord/lib/active_record/associations.rb b/activerecord/lib/active_record/associations.rb index 2a72fa95c9..22a693540e 100644 --- a/activerecord/lib/active_record/associations.rb +++ b/activerecord/lib/active_record/associations.rb @@ -2246,7 +2246,7 @@ module ActiveRecord end def table - if reflection.macro == :has_and_belongs_to_many + if @tables.last.is_a?(Array) @tables.last.first else @tables.last -- cgit v1.2.3 From edc176d33be9499f4c096779c5b4711b5daf0c06 Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Fri, 15 Oct 2010 17:46:09 +0100 Subject: Make sure nested through associations are read only --- activerecord/lib/active_record/associations.rb | 6 +++++ .../associations/has_many_through_association.rb | 10 ++++++++ .../associations/has_one_through_association.rb | 2 ++ .../associations/through_association_scope.rb | 27 ++++++++++++++-------- activerecord/lib/active_record/reflection.rb | 4 ++++ 5 files changed, 40 insertions(+), 9 deletions(-) (limited to 'activerecord/lib/active_record') diff --git a/activerecord/lib/active_record/associations.rb b/activerecord/lib/active_record/associations.rb index 22a693540e..1111033435 100644 --- a/activerecord/lib/active_record/associations.rb +++ b/activerecord/lib/active_record/associations.rb @@ -64,6 +64,12 @@ module ActiveRecord super("Cannot dissociate new records through '#{owner.class.name}##{reflection.name}' on '#{reflection.source_reflection.class_name rescue nil}##{reflection.source_reflection.name rescue nil}'. Both records must have an id in order to delete the has_many :through record associating them.") end end + + class HasManyThroughNestedAssociationsAreReadonly < ActiveRecordError #:nodoc + def initialize(owner, reflection) + super("Cannot modify association '#{owner.class.name}##{reflection.name}' because it goes through more than one other association.") + end + end class HasAndBelongsToManyAssociationWithPrimaryKeyError < ActiveRecordError #:nodoc: def initialize(reflection) diff --git a/activerecord/lib/active_record/associations/has_many_through_association.rb b/activerecord/lib/active_record/associations/has_many_through_association.rb index 313d9da621..f0ad166802 100644 --- a/activerecord/lib/active_record/associations/has_many_through_association.rb +++ b/activerecord/lib/active_record/associations/has_many_through_association.rb @@ -8,6 +8,11 @@ module ActiveRecord class HasManyThroughAssociation < HasManyAssociation #:nodoc: include ThroughAssociationScope + def build(attributes = {}, &block) + ensure_not_nested + super + end + alias_method :new, :build def create!(attrs = nil) @@ -37,6 +42,7 @@ module ActiveRecord protected def create_record(attrs, force = true) + ensure_not_nested ensure_owner_is_not_new transaction do @@ -60,6 +66,8 @@ module ActiveRecord end def insert_record(record, force = true, validate = true) + ensure_not_nested + if record.new_record? if force record.save! @@ -75,6 +83,8 @@ module ActiveRecord # TODO - add dependent option support def delete_records(records) + ensure_not_nested + klass = @reflection.through_reflection.klass records.each do |associate| klass.delete_all(construct_join_attributes(associate)) diff --git a/activerecord/lib/active_record/associations/has_one_through_association.rb b/activerecord/lib/active_record/associations/has_one_through_association.rb index fba0a2bfcc..8153eb7c57 100644 --- a/activerecord/lib/active_record/associations/has_one_through_association.rb +++ b/activerecord/lib/active_record/associations/has_one_through_association.rb @@ -14,6 +14,8 @@ module ActiveRecord private def create_through_record(new_value) #nodoc: + ensure_not_nested + klass = @reflection.through_reflection.klass current_object = @owner.send(@reflection.through_reflection.name) diff --git a/activerecord/lib/active_record/associations/through_association_scope.rb b/activerecord/lib/active_record/associations/through_association_scope.rb index a52672eecd..51ab8869ed 100644 --- a/activerecord/lib/active_record/associations/through_association_scope.rb +++ b/activerecord/lib/active_record/associations/through_association_scope.rb @@ -8,15 +8,18 @@ module ActiveRecord protected def construct_scope - { :create => construct_owner_attributes(@reflection), - :find => { :conditions => construct_conditions, - :joins => construct_joins, - :include => @reflection.options[:include] || @reflection.source_reflection.options[:include], - :select => construct_select, - :order => @reflection.options[:order], - :limit => @reflection.options[:limit], - :readonly => @reflection.options[:readonly], - } } + scope = {} + scope[:find] = { + :conditions => construct_conditions, + :joins => construct_joins, + :include => @reflection.options[:include] || @reflection.source_reflection.options[:include], + :select => construct_select, + :order => @reflection.options[:order], + :limit => @reflection.options[:limit], + :readonly => @reflection.options[:readonly] + } + scope[:create] = construct_owner_attributes(@reflection) unless @reflection.nested? + scope end # Build SQL conditions from attributes, qualified by table name. @@ -299,6 +302,12 @@ module ActiveRecord end alias_method :sql_conditions, :conditions + + def ensure_not_nested + if @reflection.nested? + raise HasManyThroughNestedAssociationsAreReadonly.new(@owner, @reflection) + end + end end end end diff --git a/activerecord/lib/active_record/reflection.rb b/activerecord/lib/active_record/reflection.rb index b7cd466e13..ee63fcfce2 100644 --- a/activerecord/lib/active_record/reflection.rb +++ b/activerecord/lib/active_record/reflection.rb @@ -395,6 +395,10 @@ module ActiveRecord chain end end + + def nested? + through_reflection_chain.length > 2 + end # Gets an array of possible :through source reflection names: # -- cgit v1.2.3 From 78b8c51cb3b0c629152f3bbaf6d8bcf988cc936e Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Sun, 17 Oct 2010 23:29:56 +0100 Subject: Refactoring: replace the mix of variables like @finder_sql, @counter_sql, etc with just a single scope hash (created on initialization of the proxy). This is now used consistently across all associations. Therefore, all you have to do to ensure finding/counting etc is done correctly is implement the scope correctly. --- .../associations/association_collection.rb | 66 ++++++++++------------ .../associations/association_proxy.rb | 19 +++++++ .../associations/belongs_to_association.rb | 20 ++++--- .../belongs_to_polymorphic_association.rb | 22 ++++---- .../has_and_belongs_to_many_association.rb | 37 ++++++------ .../associations/has_many_association.rb | 56 ++++++++---------- .../associations/has_many_through_association.rb | 16 +----- .../associations/has_one_association.rb | 39 ++++++------- .../associations/has_one_through_association.rb | 2 +- .../associations/through_association_scope.rb | 11 ++-- .../lib/active_record/autosave_association.rb | 4 +- 11 files changed, 138 insertions(+), 154 deletions(-) (limited to 'activerecord/lib/active_record') diff --git a/activerecord/lib/active_record/associations/association_collection.rb b/activerecord/lib/active_record/associations/association_collection.rb index cb2d9e0a79..896e18af01 100644 --- a/activerecord/lib/active_record/associations/association_collection.rb +++ b/activerecord/lib/active_record/associations/association_collection.rb @@ -19,11 +19,6 @@ module ActiveRecord # If you need to work on all current children, new and existing records, # +load_target+ and the +loaded+ flag are your friends. class AssociationCollection < AssociationProxy #:nodoc: - def initialize(owner, reflection) - super - construct_sql - end - delegate :group, :order, :limit, :joins, :where, :preload, :eager_load, :includes, :from, :lock, :readonly, :having, :to => :scoped def select(select = nil) @@ -36,7 +31,7 @@ module ActiveRecord end def scoped - with_scope(construct_scope) { @reflection.klass.scoped } + with_scope(@scope) { @reflection.klass.scoped } end def find(*args) @@ -58,9 +53,7 @@ module ActiveRecord merge_options_from_reflection!(options) construct_find_options!(options) - find_scope = construct_scope[:find].slice(:conditions, :order) - - with_scope(:find => find_scope) do + with_scope(:find => @scope[:find].slice(:conditions, :order)) do relation = @reflection.klass.send(:construct_finder_arel, options, @reflection.klass.send(:current_scoped_methods)) case args.first @@ -178,17 +171,18 @@ module ActiveRecord end end - # Count all records using SQL. If the +:counter_sql+ option is set for the association, it will - # be used for the query. If no +:counter_sql+ was supplied, but +:finder_sql+ was set, the - # descendant's +construct_sql+ method will have set :counter_sql automatically. - # Otherwise, construct options and pass them with scope to the target class's +count+. + # Count all records using SQL. If the +:counter_sql+ or +:finder_sql+ option is set for the + # association, it will be used for the query. Otherwise, construct options and pass them with + # scope to the target class's +count+. def count(column_name = nil, options = {}) column_name, options = nil, column_name if column_name.is_a?(Hash) - if @reflection.options[:counter_sql] && !options.blank? - raise ArgumentError, "If finder_sql/counter_sql is used then options cannot be passed" - elsif @reflection.options[:counter_sql] - @reflection.klass.count_by_sql(@counter_sql) + if @reflection.options[:counter_sql] || @reflection.options[:finder_sql] + unless options.blank? + raise ArgumentError, "If finder_sql/counter_sql is used then options cannot be passed" + end + + @reflection.klass.count_by_sql(custom_counter_sql) else if @reflection.options[:uniq] @@ -197,7 +191,7 @@ module ActiveRecord options.merge!(:distinct => true) end - value = @reflection.klass.send(:with_scope, construct_scope) { @reflection.klass.count(column_name, options) } + value = @reflection.klass.send(:with_scope, @scope) { @reflection.klass.count(column_name, options) } limit = @reflection.options[:limit] offset = @reflection.options[:offset] @@ -377,18 +371,6 @@ module ActiveRecord def construct_find_options!(options) end - def construct_counter_sql - if @reflection.options[:counter_sql] - @counter_sql = interpolate_sql(@reflection.options[:counter_sql]) - elsif @reflection.options[:finder_sql] - # replace the SELECT clause with COUNT(*), preserving any hints within /* ... */ - @reflection.options[:counter_sql] = @reflection.options[:finder_sql].sub(/SELECT\b(\/\*.*?\*\/ )?(.*)\bFROM\b/im) { "SELECT #{$1}COUNT(*) FROM" } - @counter_sql = interpolate_sql(@reflection.options[:counter_sql]) - else - @counter_sql = @finder_sql - end - end - def load_target if !@owner.new_record? || foreign_key_present begin @@ -434,9 +416,9 @@ module ActiveRecord elsif @reflection.klass.scopes[method] @_named_scopes_cache ||= {} @_named_scopes_cache[method] ||= {} - @_named_scopes_cache[method][args] ||= with_scope(construct_scope) { @reflection.klass.send(method, *args) } + @_named_scopes_cache[method][args] ||= with_scope(@scope) { @reflection.klass.send(method, *args) } else - with_scope(construct_scope) do + with_scope(@scope) do if block_given? @reflection.klass.send(method, *args) { |*block_args| yield(*block_args) } else @@ -446,9 +428,19 @@ module ActiveRecord end end - # overloaded in derived Association classes to provide useful scoping depending on association type. - def construct_scope - {} + def custom_counter_sql + if @reflection.options[:counter_sql] + counter_sql = @reflection.options[:counter_sql] + else + # replace the SELECT clause with COUNT(*), preserving any hints within /* ... */ + counter_sql = @reflection.options[:finder_sql].sub(/SELECT\b(\/\*.*?\*\/ )?(.*)\bFROM\b/im) { "SELECT #{$1}COUNT(*) FROM" } + end + + interpolate_sql(counter_sql) + end + + def custom_finder_sql + interpolate_sql(@reflection.options[:finder_sql]) end def reset_target! @@ -462,7 +454,7 @@ module ActiveRecord def find_target records = if @reflection.options[:finder_sql] - @reflection.klass.find_by_sql(@finder_sql) + @reflection.klass.find_by_sql(custom_finder_sql) else find(:all) end @@ -494,7 +486,7 @@ module ActiveRecord ensure_owner_is_not_new scoped_where = scoped.where_values_hash - create_scope = scoped_where ? construct_scope[:create].merge(scoped_where) : construct_scope[:create] + create_scope = scoped_where ? @scope[:create].merge(scoped_where) : @scope[:create] record = @reflection.klass.send(:with_scope, :create => create_scope) do @reflection.build_association(attrs) end diff --git a/activerecord/lib/active_record/associations/association_proxy.rb b/activerecord/lib/active_record/associations/association_proxy.rb index f333f4d603..0c12c3737d 100644 --- a/activerecord/lib/active_record/associations/association_proxy.rb +++ b/activerecord/lib/active_record/associations/association_proxy.rb @@ -61,6 +61,7 @@ module ActiveRecord reflection.check_validity! Array.wrap(reflection.options[:extend]).each { |ext| proxy_extend(ext) } reset + construct_scope end # Returns the owner of the proxy. @@ -203,6 +204,24 @@ module ActiveRecord @reflection.klass.send :with_scope, *args, &block end + # Construct the scope used for find/create queries on the target + def construct_scope + @scope = { + :find => construct_find_scope, + :create => construct_create_scope + } + end + + # Implemented by subclasses + def construct_find_scope + raise NotImplementedError + end + + # Implemented by (some) subclasses + def construct_create_scope + {} + 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 2eb56e5cd3..34b6cd5576 100644 --- a/activerecord/lib/active_record/associations/belongs_to_association.rb +++ b/activerecord/lib/active_record/associations/belongs_to_association.rb @@ -50,19 +50,21 @@ module ActiveRecord "find" end - options = @reflection.options.dup - (options.keys - [:select, :include, :readonly]).each do |key| - options.delete key - end - options[:conditions] = conditions + options = @reflection.options.dup.slice(:select, :include, :readonly) - the_target = @reflection.klass.send(find_method, - @owner[@reflection.primary_key_name], - options - ) if @owner[@reflection.primary_key_name] + the_target = with_scope(:find => @scope[:find]) do + @reflection.klass.send(find_method, + @owner[@reflection.primary_key_name], + options + ) if @owner[@reflection.primary_key_name] + end set_inverse_instance(the_target, @owner) the_target end + + def construct_find_scope + { :conditions => conditions } + end def foreign_key_present !@owner[@reflection.primary_key_name].nil? diff --git a/activerecord/lib/active_record/associations/belongs_to_polymorphic_association.rb b/activerecord/lib/active_record/associations/belongs_to_polymorphic_association.rb index e429806b0c..a0df860623 100644 --- a/activerecord/lib/active_record/associations/belongs_to_polymorphic_association.rb +++ b/activerecord/lib/active_record/associations/belongs_to_polymorphic_association.rb @@ -44,20 +44,20 @@ module ActiveRecord end end + def construct_find_scope + { :conditions => conditions } + end + def find_target return nil if association_class.nil? - target = - if @reflection.options[:conditions] - association_class.find( - @owner[@reflection.primary_key_name], - :select => @reflection.options[:select], - :conditions => conditions, - :include => @reflection.options[:include] - ) - else - association_class.find(@owner[@reflection.primary_key_name], :select => @reflection.options[:select], :include => @reflection.options[:include]) - end + target = association_class.send(:with_scope, :find => @scope[:find]) do + association_class.find( + @owner[@reflection.primary_key_name], + :select => @reflection.options[:select], + :include => @reflection.options[:include] + ) + end set_inverse_instance(target, @owner) target 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 eb65234dfb..1fc9aba5cf 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 @@ -24,7 +24,7 @@ module ActiveRecord protected def construct_find_options!(options) - options[:joins] = Arel::SqlLiteral.new @join_sql + options[:joins] = Arel::SqlLiteral.new(@scope[:find][:joins]) options[:readonly] = finding_with_ambiguous_select?(options[:select] || @reflection.options[:select]) options[:select] ||= (@reflection.options[:select] || Arel::SqlLiteral.new('*')) end @@ -80,27 +80,26 @@ module ActiveRecord ).delete end end + + def construct_joins + "INNER JOIN #{@owner.connection.quote_table_name @reflection.options[:join_table]} ON #{@reflection.quoted_table_name}.#{@reflection.klass.primary_key} = #{@owner.connection.quote_table_name @reflection.options[:join_table]}.#{@reflection.association_foreign_key}" + end - def construct_sql - if @reflection.options[:finder_sql] - @finder_sql = interpolate_sql(@reflection.options[:finder_sql]) - else - @finder_sql = "#{@owner.connection.quote_table_name @reflection.options[:join_table]}.#{@reflection.primary_key_name} = #{owner_quoted_id} " - @finder_sql << " AND (#{conditions})" if conditions - end - - @join_sql = "INNER JOIN #{@owner.connection.quote_table_name @reflection.options[:join_table]} ON #{@reflection.quoted_table_name}.#{@reflection.klass.primary_key} = #{@owner.connection.quote_table_name @reflection.options[:join_table]}.#{@reflection.association_foreign_key}" - - construct_counter_sql + def construct_conditions + sql = "#{@owner.connection.quote_table_name @reflection.options[:join_table]}.#{@reflection.primary_key_name} = #{owner_quoted_id} " + sql << " AND (#{conditions})" if conditions + sql end - def construct_scope - { :find => { :conditions => @finder_sql, - :joins => @join_sql, - :readonly => false, - :order => @reflection.options[:order], - :include => @reflection.options[:include], - :limit => @reflection.options[:limit] } } + def construct_find_scope + { + :conditions => construct_conditions, + :joins => construct_joins, + :readonly => false, + :order => @reflection.options[:order], + :include => @reflection.options[:include], + :limit => @reflection.options[:limit] + } 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_many_association.rb b/activerecord/lib/active_record/associations/has_many_association.rb index 978fc74560..830a82980d 100644 --- a/activerecord/lib/active_record/associations/has_many_association.rb +++ b/activerecord/lib/active_record/associations/has_many_association.rb @@ -6,14 +6,10 @@ module ActiveRecord # If the association has a :through option further specialization # is provided by its child HasManyThroughAssociation. class HasManyAssociation < AssociationCollection #:nodoc: - def initialize(owner, reflection) - @finder_sql = nil - super - end protected def owner_quoted_id if @reflection.options[:primary_key] - quote_value(@owner.send(@reflection.options[:primary_key])) + @owner.class.quote_value(@owner.send(@reflection.options[:primary_key])) else @owner.quoted_id end @@ -35,10 +31,10 @@ module ActiveRecord def count_records count = if has_cached_counter? @owner.send(:read_attribute, cached_counter_attribute_name) - elsif @reflection.options[:counter_sql] - @reflection.klass.count_by_sql(@counter_sql) + elsif @reflection.options[:counter_sql] || @reflection.options[:finder_sql] + @reflection.klass.count_by_sql(custom_counter_sql) else - @reflection.klass.count(:conditions => @counter_sql, :include => @reflection.options[:include]) + @reflection.klass.count(@scope[:find].slice(:conditions, :joins, :include)) end # If there's nothing in the database and @target has no new records @@ -87,36 +83,32 @@ module ActiveRecord false end - def construct_sql - case - when @reflection.options[:finder_sql] - @finder_sql = interpolate_sql(@reflection.options[:finder_sql]) - - when @reflection.options[:as] - @finder_sql = - "#{@reflection.quoted_table_name}.#{@reflection.options[:as]}_id = #{owner_quoted_id} AND " + - "#{@reflection.quoted_table_name}.#{@reflection.options[:as]}_type = #{@owner.class.quote_value(@owner.class.base_class.name.to_s)}" - @finder_sql << " AND (#{conditions})" if conditions - - else - @finder_sql = "#{@reflection.quoted_table_name}.#{@reflection.primary_key_name} = #{owner_quoted_id}" - @finder_sql << " AND (#{conditions})" if conditions + def construct_conditions + if @reflection.options[:as] + sql = + "#{@reflection.quoted_table_name}.#{@reflection.options[:as]}_id = #{owner_quoted_id} AND " + + "#{@reflection.quoted_table_name}.#{@reflection.options[:as]}_type = #{@owner.class.quote_value(@owner.class.base_class.name.to_s)}" + else + sql = "#{@reflection.quoted_table_name}.#{@reflection.primary_key_name} = #{owner_quoted_id}" end + sql << " AND (#{conditions})" if conditions + sql + end - construct_counter_sql + def construct_find_scope + { + :conditions => construct_conditions, + :readonly => false, + :order => @reflection.options[:order], + :limit => @reflection.options[:limit], + :include => @reflection.options[:include] + } end - def construct_scope + def construct_create_scope create_scoping = {} set_belongs_to_association_for(create_scoping) - { - :find => { :conditions => @finder_sql, - :readonly => false, - :order => @reflection.options[:order], - :limit => @reflection.options[:limit], - :include => @reflection.options[:include]}, - :create => create_scoping - } + create_scoping end def we_can_set_the_inverse_on_this?(record) diff --git a/activerecord/lib/active_record/associations/has_many_through_association.rb b/activerecord/lib/active_record/associations/has_many_through_association.rb index f0ad166802..419a3d385e 100644 --- a/activerecord/lib/active_record/associations/has_many_through_association.rb +++ b/activerecord/lib/active_record/associations/has_many_through_association.rb @@ -93,21 +93,7 @@ module ActiveRecord def find_target return [] unless target_reflection_has_associated_record? - with_scope(construct_scope) { @reflection.klass.find(:all) } - end - - def construct_sql - case - when @reflection.options[:finder_sql] - @finder_sql = interpolate_sql(@reflection.options[:finder_sql]) - - @finder_sql = "#{@reflection.quoted_table_name}.#{@reflection.primary_key_name} = #{owner_quoted_id}" - @finder_sql << " AND (#{conditions})" if conditions - else - @finder_sql = construct_conditions - end - - construct_counter_sql + with_scope(@scope) { @reflection.klass.find(:all) } end def has_cached_counter? diff --git a/activerecord/lib/active_record/associations/has_one_association.rb b/activerecord/lib/active_record/associations/has_one_association.rb index a6e6bfa356..17901387e9 100644 --- a/activerecord/lib/active_record/associations/has_one_association.rb +++ b/activerecord/lib/active_record/associations/has_one_association.rb @@ -2,11 +2,6 @@ module ActiveRecord # = Active Record Belongs To Has One Association module Associations class HasOneAssociation < AssociationProxy #:nodoc: - def initialize(owner, reflection) - super - construct_sql - end - def create(attrs = {}, replace_existing = true) new_record(replace_existing) do |reflection| attrs = merge_with_conditions(attrs) @@ -79,33 +74,31 @@ module ActiveRecord private def find_target - options = @reflection.options.dup - (options.keys - [:select, :order, :include, :readonly]).each do |key| - options.delete key - end - options[:conditions] = @finder_sql + options = @reflection.options.dup.slice(:select, :order, :include, :readonly) - the_target = @reflection.klass.find(:first, options) + the_target = with_scope(:find => @scope[:find]) do + @reflection.klass.find(:first, options) + end set_inverse_instance(the_target, @owner) the_target end - def construct_sql - case - when @reflection.options[:as] - @finder_sql = - "#{@reflection.quoted_table_name}.#{@reflection.options[:as]}_id = #{owner_quoted_id} AND " + - "#{@reflection.quoted_table_name}.#{@reflection.options[:as]}_type = #{@owner.class.quote_value(@owner.class.base_class.name.to_s)}" - else - @finder_sql = "#{@reflection.quoted_table_name}.#{@reflection.primary_key_name} = #{owner_quoted_id}" + def construct_find_scope + if @reflection.options[:as] + sql = + "#{@reflection.quoted_table_name}.#{@reflection.options[:as]}_id = #{owner_quoted_id} AND " + + "#{@reflection.quoted_table_name}.#{@reflection.options[:as]}_type = #{@owner.class.quote_value(@owner.class.base_class.name.to_s)}" + else + sql = "#{@reflection.quoted_table_name}.#{@reflection.primary_key_name} = #{owner_quoted_id}" end - @finder_sql << " AND (#{conditions})" if conditions + sql << " AND (#{conditions})" if conditions + { :conditions => sql } end - def construct_scope + def construct_create_scope create_scoping = {} set_belongs_to_association_for(create_scoping) - { :create => create_scoping } + create_scoping end def new_record(replace_existing) @@ -113,7 +106,7 @@ module ActiveRecord # instance. Otherwise, if the target has not previously been loaded # elsewhere, the instance we create will get orphaned. load_target if replace_existing - record = @reflection.klass.send(:with_scope, :create => construct_scope[:create]) do + record = @reflection.klass.send(:with_scope, :create => @scope[:create]) do yield @reflection end diff --git a/activerecord/lib/active_record/associations/has_one_through_association.rb b/activerecord/lib/active_record/associations/has_one_through_association.rb index 8153eb7c57..de962e01b6 100644 --- a/activerecord/lib/active_record/associations/has_one_through_association.rb +++ b/activerecord/lib/active_record/associations/has_one_through_association.rb @@ -35,7 +35,7 @@ module ActiveRecord private def find_target - with_scope(construct_scope) { @reflection.klass.find(:first) } + with_scope(@scope) { @reflection.klass.find(:first) } end end end diff --git a/activerecord/lib/active_record/associations/through_association_scope.rb b/activerecord/lib/active_record/associations/through_association_scope.rb index 51ab8869ed..261b4037a3 100644 --- a/activerecord/lib/active_record/associations/through_association_scope.rb +++ b/activerecord/lib/active_record/associations/through_association_scope.rb @@ -7,9 +7,8 @@ module ActiveRecord protected - def construct_scope - scope = {} - scope[:find] = { + def construct_find_scope + { :conditions => construct_conditions, :joins => construct_joins, :include => @reflection.options[:include] || @reflection.source_reflection.options[:include], @@ -18,8 +17,10 @@ module ActiveRecord :limit => @reflection.options[:limit], :readonly => @reflection.options[:readonly] } - scope[:create] = construct_owner_attributes(@reflection) unless @reflection.nested? - scope + end + + def construct_create_scope + @reflection.nested? ? {} : construct_owner_attributes(@reflection) end # Build SQL conditions from attributes, qualified by table name. diff --git a/activerecord/lib/active_record/autosave_association.rb b/activerecord/lib/active_record/autosave_association.rb index 21a9a1f2cb..f3f89fe7c3 100644 --- a/activerecord/lib/active_record/autosave_association.rb +++ b/activerecord/lib/active_record/autosave_association.rb @@ -313,8 +313,8 @@ module ActiveRecord end end - # reconstruct the SQL queries now that we know the owner's id - association.send(:construct_sql) if association.respond_to?(:construct_sql) + # reconstruct the scope now that we know the owner's id + association.send(:construct_scope) if association.respond_to?(:construct_scope) end end -- cgit v1.2.3 From 9ec07348749675110843c44f680da79223218db2 Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Tue, 19 Oct 2010 00:27:40 +0100 Subject: Properly support conditions on any of the reflections involved in a nested through association --- activerecord/lib/active_record/associations.rb | 15 ++- .../associations/through_association_scope.rb | 127 ++++++++++----------- activerecord/lib/active_record/reflection.rb | 53 ++++++++- 3 files changed, 119 insertions(+), 76 deletions(-) (limited to 'activerecord/lib/active_record') diff --git a/activerecord/lib/active_record/associations.rb b/activerecord/lib/active_record/associations.rb index 1111033435..75e5eb8ee4 100644 --- a/activerecord/lib/active_record/associations.rb +++ b/activerecord/lib/active_record/associations.rb @@ -2164,8 +2164,10 @@ module ActiveRecord chain = through_reflection_chain.reverse foreign_table = parent_table + index = 0 - chain.zip(@tables).each do |reflection, table| + chain.each do |reflection| + table = @tables[index] conditions = [] if reflection.source_reflection.nil? @@ -2234,13 +2236,14 @@ module ActiveRecord conditions << table[key].eq(foreign_table[foreign_key]) - conditions << reflection_conditions(reflection, table) + conditions << reflection_conditions(index, table) conditions << sti_conditions(reflection, table) - relation = relation.join(table, join_type).on(*conditions.compact) + relation = relation.join(table, join_type).on(*conditions.flatten.compact) # The current table in this iteration becomes the foreign table in the next foreign_table = table + index += 1 end relation @@ -2325,10 +2328,10 @@ module ActiveRecord @tables end - def reflection_conditions(reflection, table) - if reflection.options[:conditions] + def reflection_conditions(index, table) + @reflection.through_conditions.reverse[index].map do |condition| Arel.sql(interpolate_sql(sanitize_sql( - reflection.options[:conditions], + condition, table.table_alias || table.name ))) end diff --git a/activerecord/lib/active_record/associations/through_association_scope.rb b/activerecord/lib/active_record/associations/through_association_scope.rb index 261b4037a3..feb0a93360 100644 --- a/activerecord/lib/active_record/associations/through_association_scope.rb +++ b/activerecord/lib/active_record/associations/through_association_scope.rb @@ -24,7 +24,6 @@ module ActiveRecord end # Build SQL conditions from attributes, qualified by table name. - # TODO: Conditions on joins def construct_conditions reflection = @reflection.through_reflection_chain.last @@ -34,11 +33,12 @@ module ActiveRecord table_alias = table_aliases[reflection] end - conditions = construct_quoted_owner_attributes(reflection).map do |attr, value| + parts = construct_quoted_owner_attributes(reflection).map do |attr, value| "#{table_alias}.#{attr} = #{value}" end - conditions << sql_conditions if sql_conditions - "(" + conditions.join(') AND (') + ")" + parts += reflection_conditions(0) + + "(" + parts.join(') AND (') + ")" end # Associate attributes pointing to owner, quoted. @@ -55,23 +55,21 @@ module ActiveRecord end end - def construct_from - @reflection.table_name - end - def construct_select(custom_select = nil) distinct = "DISTINCT " if @reflection.options[:uniq] selected = custom_select || @reflection.options[:select] || "#{distinct}#{@reflection.quoted_table_name}.*" end def construct_joins(custom_joins = nil) - # p @reflection.through_reflection_chain + # TODO: Remove this at the end + #p @reflection.through_reflection_chain + #p @reflection.through_conditions "#{construct_through_joins} #{@reflection.options[:joins]} #{custom_joins}" end def construct_through_joins - joins = [] + joins, right_index = [], 1 # Iterate over each pair in the through reflection chain, joining them together @reflection.through_reflection_chain.each_cons(2) do |left, right| @@ -86,20 +84,23 @@ module ActiveRecord joins << inner_join_sql( right_table_and_alias, table_aliases[left], left.klass.primary_key, - table_aliases[right], left.primary_key_name + table_aliases[right], left.primary_key_name, + reflection_conditions(right_index) ) when :has_many, :has_one joins << inner_join_sql( right_table_and_alias, table_aliases[left], left.primary_key_name, table_aliases[right], right.klass.primary_key, - polymorphic_conditions(left, left) + polymorphic_conditions(left, left), + reflection_conditions(right_index) ) when :has_and_belongs_to_many joins << inner_join_sql( right_table_and_alias, table_aliases[left].first, left.primary_key_name, - table_aliases[right], right.klass.primary_key + table_aliases[right], right.klass.primary_key, + reflection_conditions(right_index) ) end else @@ -109,7 +110,8 @@ module ActiveRecord right_table_and_alias, table_aliases[left], left.klass.primary_key, table_aliases[right], left.source_reflection.primary_key_name, - source_type_conditions(left) + source_type_conditions(left), + reflection_conditions(right_index) ) when :has_many, :has_one if right.macro == :has_and_belongs_to_many @@ -123,7 +125,8 @@ module ActiveRecord right_table_and_alias, table_aliases[left], left.source_reflection.primary_key_name, right_table, right.klass.primary_key, - polymorphic_conditions(left, left.source_reflection) + polymorphic_conditions(left, left.source_reflection), + reflection_conditions(right_index) ) if right.macro == :has_and_belongs_to_many @@ -151,10 +154,13 @@ module ActiveRecord joins << inner_join_sql( right_table_and_alias, join_table, left.source_reflection.primary_key_name, - table_aliases[right], right.klass.primary_key + table_aliases[right], right.klass.primary_key, + reflection_conditions(right_index) ) end end + + right_index += 1 end joins.join(" ") @@ -206,18 +212,45 @@ module ActiveRecord "#{table_name} #{table_alias if table_alias != table_name}".strip end - def inner_join_sql(table, on_left_table, on_left_key, on_right_table, on_right_key, conds = nil) - "INNER JOIN %s ON %s.%s = %s.%s %s" % [ - table, - on_left_table, on_left_key, - on_right_table, on_right_key, - conds - ] + def inner_join_sql(table, on_left_table, on_left_key, on_right_table, on_right_key, *conditions) + conditions << "#{on_left_table}.#{on_left_key} = #{on_right_table}.#{on_right_key}" + conditions = conditions.flatten.compact + conditions = conditions.map { |sql| "(#{sql})" } * ' AND ' + + "INNER JOIN #{table} ON #{conditions}" + end + + def reflection_conditions(index) + reflection = @reflection.through_reflection_chain[index] + reflection_conditions = @reflection.through_conditions[index] + + conditions = [] + + if reflection.options[:as].nil? && # reflection.klass is a Module if :as is used + reflection.klass.finder_needs_type_condition? + conditions << reflection.klass.send(:type_condition).to_sql + end + + reflection_conditions.each do |condition| + sanitized_condition = reflection.klass.send(:sanitize_sql, condition) + interpolated_condition = interpolate_sql(sanitized_condition) + + if condition.is_a?(Hash) + interpolated_condition.gsub!( + @reflection.quoted_table_name, + reflection.quoted_table_name + ) + end + + conditions << interpolated_condition + end + + conditions end def polymorphic_conditions(reflection, polymorphic_reflection) if polymorphic_reflection.options[:as] - "AND %s.%s = %s" % [ + "%s.%s = %s" % [ table_aliases[reflection], "#{polymorphic_reflection.options[:as]}_type", @owner.class.quote_value(polymorphic_reflection.active_record.base_class.name) ] @@ -226,7 +259,7 @@ module ActiveRecord def source_type_conditions(reflection) if reflection.options[:source_type] - "AND %s.%s = %s" % [ + "%s.%s = %s" % [ table_aliases[reflection.through_reflection], reflection.source_reflection.options[:foreign_type].to_s, @owner.class.quote_value(reflection.options[:source_type]) @@ -245,6 +278,8 @@ module ActiveRecord end # Construct attributes for :through pointing to owner and associate. + # This method is used when adding records to the association. Since this only makes sense for + # non-nested through associations, that's the only case we have to worry about here. def construct_join_attributes(associate) # TODO: revisit this to allow it for deletion, supposing dependent option is supported raise ActiveRecord::HasManyThroughCantAssociateThroughHasOneOrManyReflection.new(@owner, @reflection) if [:has_one, :has_many].include?(@reflection.source_reflection.macro) @@ -261,48 +296,6 @@ module ActiveRecord join_attributes end - - def conditions - @conditions = build_conditions unless defined?(@conditions) - @conditions - 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 - all = [] - - [association_conditions, source_conditions].each do |conditions| - all << interpolate_sql(sanitize_sql(conditions)) if conditions - end - - all << through_conditions if through_conditions - all << build_sti_condition if uses_sti - - all.map { |sql| "(#{sql})" } * ' AND ' - end - end - - def build_through_conditions - conditions = @reflection.through_reflection.options[:conditions] - if conditions.is_a?(Hash) - interpolate_sql(@reflection.through_reflection.klass.send(:sanitize_sql, conditions)).gsub( - @reflection.quoted_table_name, - @reflection.through_reflection.quoted_table_name) - elsif conditions - interpolate_sql(sanitize_sql(conditions)) - end - end - - def build_sti_condition - @reflection.through_reflection.klass.send(:type_condition).to_sql - end - - alias_method :sql_conditions, :conditions def ensure_not_nested if @reflection.nested? diff --git a/activerecord/lib/active_record/reflection.rb b/activerecord/lib/active_record/reflection.rb index ee63fcfce2..d8bd6c9873 100644 --- a/activerecord/lib/active_record/reflection.rb +++ b/activerecord/lib/active_record/reflection.rb @@ -253,6 +253,10 @@ module ActiveRecord def through_reflection_chain [self] end + + def through_conditions + [Array.wrap(options[:conditions])] + end def through_reflection_primary_key_name end @@ -378,9 +382,9 @@ module ActiveRecord # TODO: Documentation def through_reflection_chain @through_reflection_chain ||= begin - if source_reflection.through_reflection - # If the source reflection goes through another reflection, then the chain must start - # by getting us to the source reflection. + if source_reflection.source_reflection + # If the source reflection has its own source reflection, then the chain must start + # by getting us to that source reflection. chain = source_reflection.through_reflection_chain else # If the source reflection does not go through another reflection, then we can get @@ -396,6 +400,49 @@ module ActiveRecord end end + # Consider the following example: + # + # class Person + # has_many :articles + # has_many :comment_tags, :through => :articles + # end + # + # class Article + # has_many :comments + # has_many :comment_tags, :through => :comments, :source => :tags + # end + # + # class Comment + # has_many :tags + # end + # + # There may be conditions on Person.comment_tags, Article.comment_tags and/or Comment.tags, + # but only Comment.tags will be represented in the through_reflection_chain. So this method + # creates an array of conditions corresponding to the through_reflection_chain. Each item in + # the through_conditions array corresponds to an item in the through_reflection_chain, and is + # itself an array of conditions from an arbitrary number of relevant reflections. + def through_conditions + @through_conditions ||= begin + # Initialize the first item - which corresponds to this reflection - either by recursing + # into the souce reflection (if it is itself a through reflection), or by grabbing the + # source reflection conditions. + if source_reflection.source_reflection + conditions = source_reflection.through_conditions + else + conditions = [Array.wrap(source_reflection.options[:conditions])] + end + + # Add to it the conditions from this reflection if necessary. + conditions.first << options[:conditions] if options[:conditions] + + # Recursively fill out the rest of the array from the through reflection + conditions += through_reflection.through_conditions + + # And return + conditions + end + end + def nested? through_reflection_chain.length > 2 end -- cgit v1.2.3 From 596cc3b2329a9cc4a30c95c157ce36b2d08975df Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Tue, 19 Oct 2010 12:47:19 +0100 Subject: Respect the :primary_key option on the through_reflection of (non-nested) through associations --- activerecord/lib/active_record/associations/has_many_association.rb | 6 +++--- activerecord/lib/active_record/associations/has_one_association.rb | 6 +++--- .../lib/active_record/associations/through_association_scope.rb | 4 ++-- 3 files changed, 8 insertions(+), 8 deletions(-) (limited to 'activerecord/lib/active_record') diff --git a/activerecord/lib/active_record/associations/has_many_association.rb b/activerecord/lib/active_record/associations/has_many_association.rb index 830a82980d..7eaa05ee36 100644 --- a/activerecord/lib/active_record/associations/has_many_association.rb +++ b/activerecord/lib/active_record/associations/has_many_association.rb @@ -7,9 +7,9 @@ module ActiveRecord # is provided by its child HasManyThroughAssociation. class HasManyAssociation < AssociationCollection #:nodoc: protected - def owner_quoted_id - if @reflection.options[:primary_key] - @owner.class.quote_value(@owner.send(@reflection.options[:primary_key])) + def owner_quoted_id(reflection = @reflection) + if reflection.options[:primary_key] + @owner.class.quote_value(@owner.send(reflection.options[:primary_key])) else @owner.quoted_id end diff --git a/activerecord/lib/active_record/associations/has_one_association.rb b/activerecord/lib/active_record/associations/has_one_association.rb index 17901387e9..c6bcfec275 100644 --- a/activerecord/lib/active_record/associations/has_one_association.rb +++ b/activerecord/lib/active_record/associations/has_one_association.rb @@ -64,9 +64,9 @@ module ActiveRecord end protected - def owner_quoted_id - if @reflection.options[:primary_key] - @owner.class.quote_value(@owner.send(@reflection.options[:primary_key])) + def owner_quoted_id(reflection = @reflection) + if reflection.options[:primary_key] + @owner.class.quote_value(@owner.send(reflection.options[:primary_key])) else @owner.quoted_id end diff --git a/activerecord/lib/active_record/associations/through_association_scope.rb b/activerecord/lib/active_record/associations/through_association_scope.rb index feb0a93360..86ceb1a204 100644 --- a/activerecord/lib/active_record/associations/through_association_scope.rb +++ b/activerecord/lib/active_record/associations/through_association_scope.rb @@ -44,14 +44,14 @@ module ActiveRecord # Associate attributes pointing to owner, quoted. def construct_quoted_owner_attributes(reflection) if as = reflection.options[:as] - { "#{as}_id" => owner_quoted_id, + { "#{as}_id" => owner_quoted_id(reflection), "#{as}_type" => reflection.klass.quote_value( @owner.class.base_class.name.to_s, reflection.klass.columns_hash["#{as}_type"]) } elsif reflection.macro == :belongs_to { reflection.klass.primary_key => @owner.class.quote_value(@owner[reflection.primary_key_name]) } else - { reflection.primary_key_name => owner_quoted_id } + { reflection.primary_key_name => owner_quoted_id(reflection) } end end -- cgit v1.2.3 From 01838636c6136d9a649ace71db61bb7990f9bd82 Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Tue, 19 Oct 2010 14:14:06 +0100 Subject: Support for :primary_key option on the source reflection of a through association, where the source is a has_one or has_many --- activerecord/lib/active_record/associations.rb | 10 ++++------ .../active_record/associations/through_association_scope.rb | 4 ++-- activerecord/lib/active_record/reflection.rb | 8 ++++++++ 3 files changed, 14 insertions(+), 8 deletions(-) (limited to 'activerecord/lib/active_record') diff --git a/activerecord/lib/active_record/associations.rb b/activerecord/lib/active_record/associations.rb index 75e5eb8ee4..29f1c7b81d 100644 --- a/activerecord/lib/active_record/associations.rb +++ b/activerecord/lib/active_record/associations.rb @@ -2173,13 +2173,11 @@ module ActiveRecord if reflection.source_reflection.nil? case reflection.macro when :belongs_to - key = reflection.options[:primary_key] || - reflection.klass.primary_key + key = reflection.association_primary_key foreign_key = reflection.primary_key_name when :has_many, :has_one key = reflection.primary_key_name - foreign_key = reflection.options[:primary_key] || - reflection.active_record.primary_key + foreign_key = reflection.active_record_primary_key conditions << polymorphic_conditions(reflection, table) when :has_and_belongs_to_many @@ -2209,13 +2207,13 @@ module ActiveRecord else case reflection.source_reflection.macro when :belongs_to - key = reflection.klass.primary_key + key = reflection.source_reflection.association_primary_key foreign_key = reflection.source_reflection.primary_key_name conditions << source_type_conditions(reflection, foreign_table) when :has_many, :has_one key = reflection.source_reflection.primary_key_name - foreign_key = reflection.source_reflection.klass.primary_key + foreign_key = reflection.source_reflection.active_record_primary_key when :has_and_belongs_to_many table, join_table = table diff --git a/activerecord/lib/active_record/associations/through_association_scope.rb b/activerecord/lib/active_record/associations/through_association_scope.rb index 86ceb1a204..2b2229f01f 100644 --- a/activerecord/lib/active_record/associations/through_association_scope.rb +++ b/activerecord/lib/active_record/associations/through_association_scope.rb @@ -108,7 +108,7 @@ module ActiveRecord when :belongs_to joins << inner_join_sql( right_table_and_alias, - table_aliases[left], left.klass.primary_key, + table_aliases[left], left.source_reflection.association_primary_key, table_aliases[right], left.source_reflection.primary_key_name, source_type_conditions(left), reflection_conditions(right_index) @@ -124,7 +124,7 @@ module ActiveRecord joins << inner_join_sql( right_table_and_alias, table_aliases[left], left.source_reflection.primary_key_name, - right_table, right.klass.primary_key, + right_table, left.source_reflection.active_record_primary_key, polymorphic_conditions(left, left.source_reflection), reflection_conditions(right_index) ) diff --git a/activerecord/lib/active_record/reflection.rb b/activerecord/lib/active_record/reflection.rb index d8bd6c9873..a46597e497 100644 --- a/activerecord/lib/active_record/reflection.rb +++ b/activerecord/lib/active_record/reflection.rb @@ -217,6 +217,14 @@ module ActiveRecord def association_foreign_key @association_foreign_key ||= @options[:association_foreign_key] || class_name.foreign_key end + + def association_primary_key + @association_primary_key ||= @options[:primary_key] || klass.primary_key + end + + def active_record_primary_key + @active_record_primary_key ||= @options[:primary_key] || active_record.primary_key + end def counter_cache_column if options[:counter_cache] == true -- cgit v1.2.3 From 9ff5fdeda99b3d8c5148d4c40956842518d1c788 Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Tue, 19 Oct 2010 14:56:59 +0100 Subject: Remove unused methods --- activerecord/lib/active_record/reflection.rb | 11 ----------- 1 file changed, 11 deletions(-) (limited to 'activerecord/lib/active_record') diff --git a/activerecord/lib/active_record/reflection.rb b/activerecord/lib/active_record/reflection.rb index a46597e497..6078191773 100644 --- a/activerecord/lib/active_record/reflection.rb +++ b/activerecord/lib/active_record/reflection.rb @@ -266,9 +266,6 @@ module ActiveRecord [Array.wrap(options[:conditions])] end - def through_reflection_primary_key_name - end - def source_reflection nil end @@ -489,14 +486,6 @@ module ActiveRecord check_validity_of_inverse! end - def through_reflection_primary_key - through_reflection.belongs_to? ? through_reflection.klass.primary_key : through_reflection.primary_key_name - end - - def through_reflection_primary_key_name - through_reflection.primary_key_name if through_reflection.belongs_to? - end - private def derive_class_name # get the class_name of the belongs_to association of the through reflection -- cgit v1.2.3 From 0ceb34295501a797c9e549c581ecee17f837f01c Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Tue, 19 Oct 2010 15:24:30 +0100 Subject: Bugfix/refactoring --- activerecord/lib/active_record/associations.rb | 12 ++++++------ .../active_record/associations/through_association_scope.rb | 10 +++++----- activerecord/lib/active_record/reflection.rb | 9 +++++++++ 3 files changed, 20 insertions(+), 11 deletions(-) (limited to 'activerecord/lib/active_record') diff --git a/activerecord/lib/active_record/associations.rb b/activerecord/lib/active_record/associations.rb index 29f1c7b81d..9e000f2aae 100644 --- a/activerecord/lib/active_record/associations.rb +++ b/activerecord/lib/active_record/associations.rb @@ -2207,18 +2207,18 @@ module ActiveRecord else case reflection.source_reflection.macro when :belongs_to - key = reflection.source_reflection.association_primary_key - foreign_key = reflection.source_reflection.primary_key_name + key = reflection.association_primary_key + foreign_key = reflection.primary_key_name conditions << source_type_conditions(reflection, foreign_table) when :has_many, :has_one - key = reflection.source_reflection.primary_key_name + key = reflection.primary_key_name foreign_key = reflection.source_reflection.active_record_primary_key when :has_and_belongs_to_many table, join_table = table - join_key = reflection.source_reflection.primary_key_name - join_foreign_key = reflection.source_reflection.klass.primary_key + join_key = reflection.primary_key_name + join_foreign_key = reflection.klass.primary_key relation = relation.join(join_table, join_type).on( join_table[join_key]. @@ -2228,7 +2228,7 @@ module ActiveRecord foreign_table = join_table key = reflection.klass.primary_key - foreign_key = reflection.source_reflection.association_foreign_key + foreign_key = reflection.association_foreign_key end end diff --git a/activerecord/lib/active_record/associations/through_association_scope.rb b/activerecord/lib/active_record/associations/through_association_scope.rb index 2b2229f01f..1365851337 100644 --- a/activerecord/lib/active_record/associations/through_association_scope.rb +++ b/activerecord/lib/active_record/associations/through_association_scope.rb @@ -108,8 +108,8 @@ module ActiveRecord when :belongs_to joins << inner_join_sql( right_table_and_alias, - table_aliases[left], left.source_reflection.association_primary_key, - table_aliases[right], left.source_reflection.primary_key_name, + table_aliases[left], left.association_primary_key, + table_aliases[right], left.primary_key_name, source_type_conditions(left), reflection_conditions(right_index) ) @@ -123,7 +123,7 @@ module ActiveRecord joins << inner_join_sql( right_table_and_alias, - table_aliases[left], left.source_reflection.primary_key_name, + table_aliases[left], left.primary_key_name, right_table, left.source_reflection.active_record_primary_key, polymorphic_conditions(left, left.source_reflection), reflection_conditions(right_index) @@ -148,12 +148,12 @@ module ActiveRecord join_table ), left_table, left.klass.primary_key, - join_table, left.source_reflection.association_foreign_key + join_table, left.association_foreign_key ) joins << inner_join_sql( right_table_and_alias, - join_table, left.source_reflection.primary_key_name, + join_table, left.primary_key_name, table_aliases[right], right.klass.primary_key, reflection_conditions(right_index) ) diff --git a/activerecord/lib/active_record/reflection.rb b/activerecord/lib/active_record/reflection.rb index 6078191773..7ce2bbb8ae 100644 --- a/activerecord/lib/active_record/reflection.rb +++ b/activerecord/lib/active_record/reflection.rb @@ -357,6 +357,8 @@ module ActiveRecord # Holds all the meta-data about a :through association as it was specified # in the Active Record class. class ThroughReflection < AssociationReflection #:nodoc: + delegate :primary_key_name, :association_foreign_key, :to => :source_reflection + # Gets the source of the through reflection. It checks both a singularized # and pluralized form for :belongs_to or :has_many. # @@ -451,6 +453,13 @@ module ActiveRecord def nested? through_reflection_chain.length > 2 end + + # We want to use the klass from this reflection, rather than just delegate straight to + # the source_reflection, because the source_reflection may be polymorphic. We still + # need to respect the source_reflection's :primary_key option, though. + def association_primary_key + @association_primary_key ||= source_reflection.options[:primary_key] || klass.primary_key + end # Gets an array of possible :through source reflection names: # -- cgit v1.2.3 From 915ea5ea826d48107e4c1953c7a32cf26727d10e Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Tue, 19 Oct 2010 16:13:06 +0100 Subject: Support the :primary_key option on a through reflection in a nested through association --- .../lib/active_record/associations/through_association_scope.rb | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) (limited to 'activerecord/lib/active_record') diff --git a/activerecord/lib/active_record/associations/through_association_scope.rb b/activerecord/lib/active_record/associations/through_association_scope.rb index 1365851337..649bbd206a 100644 --- a/activerecord/lib/active_record/associations/through_association_scope.rb +++ b/activerecord/lib/active_record/associations/through_association_scope.rb @@ -76,14 +76,11 @@ module ActiveRecord right_table_and_alias = table_name_and_alias(right.quoted_table_name, table_aliases[right]) if left.source_reflection.nil? - # TODO: Perhaps need to pay attention to left.options[:primary_key] and - # left.options[:foreign_key] in places here - case left.macro when :belongs_to joins << inner_join_sql( right_table_and_alias, - table_aliases[left], left.klass.primary_key, + table_aliases[left], left.association_primary_key, table_aliases[right], left.primary_key_name, reflection_conditions(right_index) ) @@ -91,7 +88,7 @@ module ActiveRecord joins << inner_join_sql( right_table_and_alias, table_aliases[left], left.primary_key_name, - table_aliases[right], right.klass.primary_key, + table_aliases[right], right.association_primary_key, polymorphic_conditions(left, left), reflection_conditions(right_index) ) -- cgit v1.2.3 From b00db54e746ab0f1664d7f160b46beb49587b370 Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Tue, 19 Oct 2010 16:23:53 +0100 Subject: Small refactoring --- activerecord/lib/active_record/associations.rb | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) (limited to 'activerecord/lib/active_record') diff --git a/activerecord/lib/active_record/associations.rb b/activerecord/lib/active_record/associations.rb index 9e000f2aae..028157d7e9 100644 --- a/activerecord/lib/active_record/associations.rb +++ b/activerecord/lib/active_record/associations.rb @@ -2186,9 +2186,7 @@ module ActiveRecord # to represent the join table) table, join_table = table - # TODO: Can join_key just be reflection.primary_key_name ? - join_key = reflection.options[:foreign_key] || - reflection.active_record.to_s.foreign_key + join_key = reflection.primary_key_name join_foreign_key = reflection.active_record.primary_key relation = relation.join(join_table, join_type).on( @@ -2199,10 +2197,8 @@ module ActiveRecord # We've done the first join now, so update the foreign_table for the second foreign_table = join_table - # TODO: Can foreign_key be reflection.association_foreign_key? key = reflection.klass.primary_key - foreign_key = reflection.options[:association_foreign_key] || - reflection.klass.to_s.foreign_key + foreign_key = reflection.association_foreign_key end else case reflection.source_reflection.macro -- cgit v1.2.3 From 82b889f7d37249adaa606558d4c05356b3e84d9a Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Tue, 19 Oct 2010 17:22:42 +0100 Subject: Add explicit tests for the nested through association changes in reflection.rb --- activerecord/lib/active_record/reflection.rb | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) (limited to 'activerecord/lib/active_record') diff --git a/activerecord/lib/active_record/reflection.rb b/activerecord/lib/active_record/reflection.rb index 7ce2bbb8ae..3448cc506c 100644 --- a/activerecord/lib/active_record/reflection.rb +++ b/activerecord/lib/active_record/reflection.rb @@ -450,15 +450,26 @@ module ActiveRecord end end + # A through association is nested iff there would be more than one join table def nested? - through_reflection_chain.length > 2 + through_reflection_chain.length > 2 || + through_reflection.macro == :has_and_belongs_to_many end # We want to use the klass from this reflection, rather than just delegate straight to # the source_reflection, because the source_reflection may be polymorphic. We still # need to respect the source_reflection's :primary_key option, though. def association_primary_key - @association_primary_key ||= source_reflection.options[:primary_key] || klass.primary_key + @association_primary_key ||= begin + # Get the "actual" source reflection if the immediate source reflection has a + # source reflection itself + source_reflection = self.source_reflection + while source_reflection.source_reflection + source_reflection = source_reflection.source_reflection + end + + source_reflection.options[:primary_key] || klass.primary_key + end end # Gets an array of possible :through source reflection names: -- cgit v1.2.3 From 7ee33b80a2048ec3801f02018b0ea81d2abe0011 Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Tue, 19 Oct 2010 17:29:19 +0100 Subject: Remove various comments and code which were just being used during the development of nested through association support (OMFGZ, I might just have nearly finished this\! --- activerecord/lib/active_record/associations.rb | 8 -- .../associations/nested_has_many_through.rb | 158 --------------------- .../associations/through_association_scope.rb | 4 - activerecord/lib/active_record/reflection.rb | 24 ---- 4 files changed, 194 deletions(-) delete mode 100644 activerecord/lib/active_record/associations/nested_has_many_through.rb (limited to 'activerecord/lib/active_record') diff --git a/activerecord/lib/active_record/associations.rb b/activerecord/lib/active_record/associations.rb index 028157d7e9..44d3258c40 100644 --- a/activerecord/lib/active_record/associations.rb +++ b/activerecord/lib/active_record/associations.rb @@ -39,14 +39,6 @@ module ActiveRecord end end - class HasManyThroughSourceAssociationMacroError < ActiveRecordError #:nodoc: - def initialize(reflection) - through_reflection = reflection.through_reflection - source_reflection = reflection.source_reflection - super("Invalid source reflection macro :#{source_reflection.macro}#{" :through" if source_reflection.options[:through]} for has_many #{reflection.name.inspect}, :through => #{through_reflection.name.inspect}. Use :source to specify the source reflection.") - end - end - class HasManyThroughCantAssociateThroughHasOneOrManyReflection < ActiveRecordError #:nodoc: def initialize(owner, reflection) super("Cannot modify association '#{owner.class.name}##{reflection.name}' because the source reflection class '#{reflection.source_reflection.class_name}' is associated to '#{reflection.through_reflection.class_name}' via :#{reflection.source_reflection.macro}.") diff --git a/activerecord/lib/active_record/associations/nested_has_many_through.rb b/activerecord/lib/active_record/associations/nested_has_many_through.rb deleted file mode 100644 index d699a60edb..0000000000 --- a/activerecord/lib/active_record/associations/nested_has_many_through.rb +++ /dev/null @@ -1,158 +0,0 @@ -# TODO: Remove in the end, when its functionality is fully integrated in ThroughAssociationScope. - -module ActiveRecord - module Associations - module NestedHasManyThrough - def self.included(klass) - klass.alias_method_chain :construct_conditions, :nesting - klass.alias_method_chain :construct_joins, :nesting - end - - def construct_joins_with_nesting(custom_joins = nil) - if nested? - @nested_join_attributes ||= construct_nested_join_attributes - "#{construct_nested_join_attributes[:joins]} #{@reflection.options[:joins]} #{custom_joins}" - else - construct_joins_without_nesting(custom_joins) - end - end - - def construct_conditions_with_nesting - if nested? - @nested_join_attributes ||= construct_nested_join_attributes - if @reflection.through_reflection && @reflection.through_reflection.macro == :belongs_to - "#{@nested_join_attributes[:remote_key]} = #{belongs_to_quoted_key} #{@nested_join_attributes[:conditions]}" - else - "#{@nested_join_attributes[:remote_key]} = #{@owner.quoted_id} #{@nested_join_attributes[:conditions]}" - end - else - construct_conditions_without_nesting - end - end - - protected - - # Given any belongs_to or has_many (including has_many :through) association, - # return the essential components of a join corresponding to that association, namely: - # - # * :joins: any additional joins required to get from the association's table - # (reflection.table_name) to the table that's actually joining to the active record's table - # * :remote_key: the name of the key in the join table (qualified by table name) which will join - # to a field of the active record's table - # * :local_key: the name of the key in the local table (not qualified by table name) which will - # take part in the join - # * :conditions: any additional conditions (e.g. filtering by type for a polymorphic association, - # or a :conditions clause explicitly given in the association), including a leading AND - def construct_nested_join_attributes(reflection = @reflection, association_class = reflection.klass, table_ids = {association_class.table_name => 1}) - if (reflection.macro == :has_many || reflection.macro == :has_one) && reflection.through_reflection - construct_has_many_through_attributes(reflection, table_ids) - else - construct_has_many_or_belongs_to_attributes(reflection, association_class, table_ids) - end - end - - def construct_has_many_through_attributes(reflection, table_ids) - # Construct the join components of the source association, so that we have a path from - # the eventual target table of the association up to the table named in :through, and - # all tables involved are allocated table IDs. - source_attrs = construct_nested_join_attributes(reflection.source_reflection, reflection.klass, table_ids) - - # Determine the alias of the :through table; this will be the last table assigned - # when constructing the source join components above. - through_table_alias = through_table_name = reflection.through_reflection.table_name - through_table_alias += "_#{table_ids[through_table_name]}" unless table_ids[through_table_name] == 1 - - # Construct the join components of the through association, so that we have a path to - # the active record's table. - through_attrs = construct_nested_join_attributes(reflection.through_reflection, reflection.through_reflection.klass, table_ids) - - # Any subsequent joins / filters on owner attributes will act on the through association, - # so that's what we return for the conditions/keys of the overall association. - conditions = through_attrs[:conditions] - conditions += " AND #{interpolate_sql(reflection.klass.send(:sanitize_sql, reflection.options[:conditions]))}" if reflection.options[:conditions] - - { - :joins => "%s INNER JOIN %s ON ( %s = %s.%s %s) %s %s" % [ - source_attrs[:joins], - through_table_name == through_table_alias ? through_table_name : "#{through_table_name} #{through_table_alias}", - source_attrs[:remote_key], - through_table_alias, source_attrs[:local_key], - source_attrs[:conditions], - through_attrs[:joins], - reflection.options[:joins] - ], - :remote_key => through_attrs[:remote_key], - :local_key => through_attrs[:local_key], - :conditions => conditions - } - end - - # reflection is not has_many :through; it's a standard has_many / belongs_to instead - # TODO: see if we can defer to rails code here a bit more - def construct_has_many_or_belongs_to_attributes(reflection, association_class, table_ids) - # Determine the alias used for remote_table_name, if any. In all cases this will already - # have been assigned an ID in table_ids (either through being involved in a previous join, - # or - if it's the first table in the query - as the default value of table_ids) - remote_table_alias = remote_table_name = association_class.table_name - remote_table_alias += "_#{table_ids[remote_table_name]}" unless table_ids[remote_table_name] == 1 - - # Assign a new alias for the local table. - local_table_alias = local_table_name = reflection.active_record.table_name - if table_ids[local_table_name] - table_id = table_ids[local_table_name] += 1 - local_table_alias += "_#{table_id}" - else - table_ids[local_table_name] = 1 - end - - conditions = '' - # Add type_condition, if applicable - conditions += " AND #{association_class.send(:type_condition).to_sql}" if association_class.finder_needs_type_condition? - # Add custom conditions - conditions += " AND (#{interpolate_sql(association_class.send(:sanitize_sql, reflection.options[:conditions]))})" if reflection.options[:conditions] - - if reflection.macro == :belongs_to - if reflection.options[:polymorphic] - conditions += " AND #{local_table_alias}.#{reflection.options[:foreign_type]} = #{reflection.active_record.quote_value(association_class.base_class.name.to_s)}" - end - { - :joins => reflection.options[:joins], - :remote_key => "#{remote_table_alias}.#{association_class.primary_key}", - :local_key => reflection.primary_key_name, - :conditions => conditions - } - else - # Association is has_many (without :through) - if reflection.options[:as] - conditions += " AND #{remote_table_alias}.#{reflection.options[:as]}_type = #{reflection.active_record.quote_value(reflection.active_record.base_class.name.to_s)}" - end - { - :joins => "#{reflection.options[:joins]}", - :remote_key => "#{remote_table_alias}.#{reflection.primary_key_name}", - :local_key => reflection.klass.primary_key, - :conditions => conditions - } - end - end - - def belongs_to_quoted_key - attribute = @reflection.through_reflection.primary_key_name - column = @owner.column_for_attribute attribute - - @owner.send(:quote_value, @owner.send(attribute), column) - end - - def nested? - through_source_reflection? || through_through_reflection? - end - - def through_source_reflection? - @reflection.source_reflection && @reflection.source_reflection.options[:through] - end - - def through_through_reflection? - @reflection.through_reflection && @reflection.through_reflection.options[:through] - end - end - end -end diff --git a/activerecord/lib/active_record/associations/through_association_scope.rb b/activerecord/lib/active_record/associations/through_association_scope.rb index 649bbd206a..abe7af418d 100644 --- a/activerecord/lib/active_record/associations/through_association_scope.rb +++ b/activerecord/lib/active_record/associations/through_association_scope.rb @@ -61,10 +61,6 @@ module ActiveRecord end def construct_joins(custom_joins = nil) - # TODO: Remove this at the end - #p @reflection.through_reflection_chain - #p @reflection.through_conditions - "#{construct_through_joins} #{@reflection.options[:joins]} #{custom_joins}" end diff --git a/activerecord/lib/active_record/reflection.rb b/activerecord/lib/active_record/reflection.rb index 3448cc506c..1ea892895f 100644 --- a/activerecord/lib/active_record/reflection.rb +++ b/activerecord/lib/active_record/reflection.rb @@ -131,14 +131,6 @@ module ActiveRecord @sanitized_conditions ||= klass.send(:sanitize_sql, options[:conditions]) if options[:conditions] end - # TODO: Remove these in the final patch. I am just using them for debugging etc. - def inspect - "#<#{code_name}>" - end - def code_name - "#{active_record.name}.#{macro} :#{name}" - end - private def derive_class_name name.to_s.camelize @@ -325,16 +317,6 @@ module ActiveRecord def belongs_to? macro == :belongs_to end - - # TODO: Remove for final patch. Just here for debugging. - def inspect - str = "#<#{code_name}, @source_reflection=" - str << (source_reflection.respond_to?(:code_name) ? source_reflection.code_name : source_reflection.inspect) - str << ", @through_reflection=" - str << (through_reflection.respond_to?(:code_name) ? through_reflection.code_name : through_reflection.inspect) - str << ">" - str - end private def derive_class_name @@ -497,12 +479,6 @@ module ActiveRecord raise HasManyThroughAssociationPolymorphicError.new(active_record.name, self, source_reflection) end - # TODO: Presumably remove the HasManyThroughSourceAssociationMacroError class and delete these lines. - # Think about whether there are any cases which should still be disallowed. - # unless [:belongs_to, :has_many, :has_one].include?(source_reflection.macro) && source_reflection.options[:through].nil? - # raise HasManyThroughSourceAssociationMacroError.new(self) - # end - check_validity_of_inverse! end -- cgit v1.2.3 From 2aa9388746412bc88be6a1728ecfbcc8ceacbb30 Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Tue, 19 Oct 2010 17:40:14 +0100 Subject: Add some comments for ThroughReflection#through_reflection_chain --- activerecord/lib/active_record/reflection.rb | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) (limited to 'activerecord/lib/active_record') diff --git a/activerecord/lib/active_record/reflection.rb b/activerecord/lib/active_record/reflection.rb index 1ea892895f..824674ee1d 100644 --- a/activerecord/lib/active_record/reflection.rb +++ b/activerecord/lib/active_record/reflection.rb @@ -368,7 +368,16 @@ module ActiveRecord @through_reflection ||= active_record.reflect_on_association(options[:through]) end - # TODO: Documentation + # Returns an array of AssociationReflection objects which are involved in this through + # association. Each item in the array corresponds to a table which will be part of the + # query for this association. + # + # If the source reflection is itself a ThroughReflection, then we don't include self in + # the chain, but just defer to the source reflection. + # + # The chain is built by recursively calling through_reflection_chain on the source + # reflection and the through reflection. The base case for the recursion is a normal + # association, which just returns [self] for its through_reflection_chain. def through_reflection_chain @through_reflection_chain ||= begin if source_reflection.source_reflection -- cgit v1.2.3 From 2c7183c0260ca105c6440b31f60ac010891b69a9 Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Tue, 19 Oct 2010 18:16:19 +0100 Subject: Add some API documentation about nested through associations --- activerecord/lib/active_record/associations.rb | 60 ++++++++++++++++++++++---- 1 file changed, 52 insertions(+), 8 deletions(-) (limited to 'activerecord/lib/active_record') diff --git a/activerecord/lib/active_record/associations.rb b/activerecord/lib/active_record/associations.rb index 44d3258c40..379a4eb1ef 100644 --- a/activerecord/lib/active_record/associations.rb +++ b/activerecord/lib/active_record/associations.rb @@ -487,6 +487,49 @@ module ActiveRecord # @group.avatars << Avatar.new # this would work if User belonged_to Avatar rather than the other way around # @group.avatars.delete(@group.avatars.last) # so would this # + # === Nested Associations + # + # You can actually specify *any* association with the :through option, including an + # association which has a :through option itself. For example: + # + # class Author < ActiveRecord::Base + # has_many :posts + # has_many :comments, :through => :posts + # has_many :commenters, :through => :comments + # end + # + # class Post < ActiveRecord::Base + # has_many :comments + # end + # + # class Comment < ActiveRecord::Base + # belongs_to :commenter + # end + # + # @author = Author.first + # @author.commenters # => People who commented on posts written by the author + # + # An equivalent way of setting up this association this would be: + # + # class Author < ActiveRecord::Base + # has_many :posts + # has_many :commenters, :through => :posts + # end + # + # class Post < ActiveRecord::Base + # has_many :comments + # has_many :commenters, :through => :comments + # end + # + # class Comment < ActiveRecord::Base + # belongs_to :commenter + # end + # + # When using nested association, you will not be able to modify the association because there + # is not enough information to know what modification to make. For example, if you tries to + # add a Commenter in the example above, there would be no way to tell how to set up the + # intermediate Post and Comment objects. + # # === Polymorphic Associations # # Polymorphic associations on models are not restricted on what types of models they @@ -934,10 +977,11 @@ module ActiveRecord # [:as] # Specifies a polymorphic interface (See belongs_to). # [:through] - # Specifies a join model through which to perform the query. Options for :class_name - # and :foreign_key are ignored, as the association uses the source reflection. You - # can only use a :through query through a belongs_to, has_one - # or has_many association on the join model. The collection of join models + # Specifies a join model through which to perform the query. Options for :class_name, + # :primary_key and :foreign_key are ignored, as the association uses the + # source reflection. You can use a :through association through any other, + # association, but if other :through associations are involved then the resulting + # association will be read-only. Otherwise, the collection of join models # can be managed via the collection API. For example, new join models are created for # newly associated objects, and if some are gone their rows are deleted (directly, # no destroy callbacks are triggered). @@ -1061,10 +1105,10 @@ module ActiveRecord # you want to do a join but not include the joined columns. Do not forget to include the # primary and foreign keys, otherwise it will raise an error. # [:through] - # Specifies a Join Model through which to perform the query. Options for :class_name - # and :foreign_key are ignored, as the association uses the source reflection. You - # can only use a :through query through a has_one or belongs_to - # association on the join model. + # Specifies a Join Model through which to perform the query. Options for :class_name, + # :primary_key, and :foreign_key are ignored, as the association uses the + # source reflection. You can only use a :through query through a has_one + # or belongs_to association on the join model. # [:source] # Specifies the source association name used by has_one :through queries. # Only use it if the name cannot be inferred from the association. -- cgit v1.2.3 From cf7c475ef187e88044cba139cc2e1dbf5f180b15 Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Tue, 19 Oct 2010 18:29:10 +0100 Subject: Remove obsolete autoload --- activerecord/lib/active_record/associations.rb | 1 - 1 file changed, 1 deletion(-) (limited to 'activerecord/lib/active_record') diff --git a/activerecord/lib/active_record/associations.rb b/activerecord/lib/active_record/associations.rb index 379a4eb1ef..98a101fac4 100644 --- a/activerecord/lib/active_record/associations.rb +++ b/activerecord/lib/active_record/associations.rb @@ -109,7 +109,6 @@ module ActiveRecord autoload :HasAndBelongsToManyAssociation, 'active_record/associations/has_and_belongs_to_many_association' autoload :HasManyAssociation, 'active_record/associations/has_many_association' autoload :HasManyThroughAssociation, 'active_record/associations/has_many_through_association' - autoload :NestedHasManyThroughAssociation, 'active_record/associations/nested_has_many_through_association' autoload :HasOneAssociation, 'active_record/associations/has_one_association' autoload :HasOneThroughAssociation, 'active_record/associations/has_one_through_association' autoload :AliasTracker, 'active_record/associations/alias_tracker' -- cgit v1.2.3 From 7b84477598137c6261bf2aeb5ce0d1b17e4b2b3c Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Tue, 19 Oct 2010 18:32:02 +0100 Subject: Fix typo --- activerecord/lib/active_record/associations.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'activerecord/lib/active_record') diff --git a/activerecord/lib/active_record/associations.rb b/activerecord/lib/active_record/associations.rb index 98a101fac4..25be9a52ff 100644 --- a/activerecord/lib/active_record/associations.rb +++ b/activerecord/lib/active_record/associations.rb @@ -525,7 +525,7 @@ module ActiveRecord # end # # When using nested association, you will not be able to modify the association because there - # is not enough information to know what modification to make. For example, if you tries to + # is not enough information to know what modification to make. For example, if you tried to # add a Commenter in the example above, there would be no way to tell how to set up the # intermediate Post and Comment objects. # -- cgit v1.2.3 From fcabfa428e57af115aca56f5c9aba99afae2cf7c Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Tue, 19 Oct 2010 18:43:09 +0100 Subject: Remove obsolete require to active_record/associations/nested_has_many_through --- .../lib/active_record/associations/has_many_through_association.rb | 1 - 1 file changed, 1 deletion(-) (limited to 'activerecord/lib/active_record') diff --git a/activerecord/lib/active_record/associations/has_many_through_association.rb b/activerecord/lib/active_record/associations/has_many_through_association.rb index 419a3d385e..2c9fa3b447 100644 --- a/activerecord/lib/active_record/associations/has_many_through_association.rb +++ b/activerecord/lib/active_record/associations/has_many_through_association.rb @@ -1,5 +1,4 @@ require "active_record/associations/through_association_scope" -require "active_record/associations/nested_has_many_through" require 'active_support/core_ext/object/blank' module ActiveRecord -- cgit v1.2.3 From 9a1a32ac2b8a526f543367bc7e8258bbd7e6a164 Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Sun, 31 Oct 2010 11:21:28 +0000 Subject: Fix naughty trailing whitespace --- .../lib/active_record/association_preload.rb | 30 +++---- activerecord/lib/active_record/associations.rb | 98 +++++++++++----------- .../active_record/associations/alias_tracker.rb | 26 +++--- .../associations/has_many_through_association.rb | 4 +- .../associations/has_one_through_association.rb | 2 +- .../associations/through_association_scope.rb | 64 +++++++------- activerecord/lib/active_record/reflection.rb | 42 +++++----- 7 files changed, 133 insertions(+), 133 deletions(-) (limited to 'activerecord/lib/active_record') diff --git a/activerecord/lib/active_record/association_preload.rb b/activerecord/lib/active_record/association_preload.rb index c3ccb93ffd..8e7416472f 100644 --- a/activerecord/lib/active_record/association_preload.rb +++ b/activerecord/lib/active_record/association_preload.rb @@ -210,9 +210,9 @@ module ActiveRecord return if records.first.send("loaded_#{reflection.name}?") records.each {|record| record.send("set_#{reflection.name}_target", nil)} end - + options = reflection.options - + if options[:through] records_with_through_records = preload_through_records(records, reflection, options[:through]) all_through_records = records_with_through_records.map(&:last).flatten @@ -220,10 +220,10 @@ module ActiveRecord unless all_through_records.empty? source = reflection.source_reflection.name all_through_records.first.class.preload_associations(all_through_records, source, options) - + records_with_through_records.each do |record, through_records| source_records = through_records.map(&source).flatten.compact - + case reflection.macro when :has_many, :has_and_belongs_to_many add_preloaded_records_to_collection([record], reflection.name, source_records) @@ -235,7 +235,7 @@ module ActiveRecord else id_to_record_map, ids = construct_id_map(records, reflection.options[:primary_key]) associated_records = find_associated_records(ids, reflection, preload_options) - + if reflection.macro == :has_many set_association_collection_records( id_to_record_map, reflection.name, @@ -249,7 +249,7 @@ module ActiveRecord end end end - + alias_method :preload_has_one_association, :preload_has_one_or_has_many_association alias_method :preload_has_many_association, :preload_has_one_or_has_many_association @@ -259,12 +259,12 @@ module ActiveRecord # record. This is so that we can preload the source association for each record, # and always be able to access the preloaded association regardless of where we # refer to the record. - # + # # Suffices to say, if AR had an identity map built in then this would be unnecessary. identity_map = {} - + options = {} - + if reflection.options[:source_type] interface = reflection.source_reflection.options[:foreign_type] options[:conditions] = ["#{connection.quote_column_name interface} = ?", reflection.options[:source_type]] @@ -272,20 +272,20 @@ module ActiveRecord else if reflection.options[:conditions] options[:include] = reflection.options[:include] || - reflection.options[:source] + reflection.options[:source] options[:conditions] = reflection.options[:conditions] end - + options[:order] = reflection.options[:order] end - + records.first.class.preload_associations(records, through_association, options) records.map do |record| if reflection.options[:source_type] # Dont cache the association - we would only be caching a subset proxy = record.send(through_association) - + if proxy.respond_to?(:target) through_records = proxy.target proxy.reset @@ -295,11 +295,11 @@ module ActiveRecord else through_records = record.send(through_association) end - + through_records = Array.wrap(through_records).map do |through_record| identity_map[through_record] ||= through_record end - + [record, through_records] end end diff --git a/activerecord/lib/active_record/associations.rb b/activerecord/lib/active_record/associations.rb index 37dbff4061..f061c8da0f 100644 --- a/activerecord/lib/active_record/associations.rb +++ b/activerecord/lib/active_record/associations.rb @@ -56,7 +56,7 @@ module ActiveRecord super("Cannot dissociate new records through '#{owner.class.name}##{reflection.name}' on '#{reflection.source_reflection.class_name rescue nil}##{reflection.source_reflection.name rescue nil}'. Both records must have an id in order to delete the has_many :through record associating them.") end end - + class HasManyThroughNestedAssociationsAreReadonly < ActiveRecordError #:nodoc def initialize(owner, reflection) super("Cannot modify association '#{owner.class.name}##{reflection.name}' because it goes through more than one other association.") @@ -487,7 +487,7 @@ module ActiveRecord # @group.avatars.delete(@group.avatars.last) # so would this # # === Nested Associations - # + # # You can actually specify *any* association with the :through option, including an # association which has a :through option itself. For example: # @@ -496,15 +496,15 @@ module ActiveRecord # has_many :comments, :through => :posts # has_many :commenters, :through => :comments # end - # + # # class Post < ActiveRecord::Base # has_many :comments # end - # + # # class Comment < ActiveRecord::Base # belongs_to :commenter # end - # + # # @author = Author.first # @author.commenters # => People who commented on posts written by the author # @@ -514,19 +514,19 @@ module ActiveRecord # has_many :posts # has_many :commenters, :through => :posts # end - # + # # class Post < ActiveRecord::Base # has_many :comments # has_many :commenters, :through => :comments # end - # + # # class Comment < ActiveRecord::Base # belongs_to :commenter # end # # When using nested association, you will not be able to modify the association because there # is not enough information to know what modification to make. For example, if you tried to - # add a Commenter in the example above, there would be no way to tell how to set up the + # add a Commenter in the example above, there would be no way to tell how to set up the # intermediate Post and Comment objects. # # === Polymorphic Associations @@ -2183,9 +2183,9 @@ module ActiveRecord # What type of join will be generated, either Arel::InnerJoin (default) or Arel::OuterJoin attr_accessor :join_type - + attr_reader :aliased_prefix - + delegate :options, :through_reflection, :source_reflection, :through_reflection_chain, :to => :reflection delegate :table, :table_name, :to => :parent, :prefix => true delegate :alias_tracker, :to => :join_dependency @@ -2198,13 +2198,13 @@ module ActiveRecord end super(reflection.klass) - + @reflection = reflection @join_dependency = join_dependency @parent = parent @join_type = Arel::InnerJoin @aliased_prefix = "t#{ join_dependency.join_parts.size }" - + setup_tables end @@ -2221,17 +2221,17 @@ module ActiveRecord end def join_to(relation) - # The chain starts with the target table, but we want to end with it here (makes + # The chain starts with the target table, but we want to end with it here (makes # more sense in this context) chain = through_reflection_chain.reverse - + foreign_table = parent_table index = 0 - + chain.each do |reflection| table = @tables[index] conditions = [] - + if reflection.source_reflection.nil? case reflection.macro when :belongs_to @@ -2240,25 +2240,25 @@ module ActiveRecord when :has_many, :has_one key = reflection.primary_key_name foreign_key = reflection.active_record_primary_key - + conditions << polymorphic_conditions(reflection, table) when :has_and_belongs_to_many # For habtm, we need to deal with the join table at the same time as the # target table (because unlike a :through association, there is no reflection # to represent the join table) table, join_table = table - + join_key = reflection.primary_key_name join_foreign_key = reflection.active_record.primary_key - + relation = relation.join(join_table, join_type).on( join_table[join_key]. eq(foreign_table[join_foreign_key]) ) - + # We've done the first join now, so update the foreign_table for the second foreign_table = join_table - + key = reflection.klass.primary_key foreign_key = reflection.association_foreign_key end @@ -2267,41 +2267,41 @@ module ActiveRecord when :belongs_to key = reflection.association_primary_key foreign_key = reflection.primary_key_name - + conditions << source_type_conditions(reflection, foreign_table) when :has_many, :has_one key = reflection.primary_key_name foreign_key = reflection.source_reflection.active_record_primary_key when :has_and_belongs_to_many table, join_table = table - + join_key = reflection.primary_key_name join_foreign_key = reflection.klass.primary_key - + relation = relation.join(join_table, join_type).on( join_table[join_key]. eq(foreign_table[join_foreign_key]) ) - + foreign_table = join_table - + key = reflection.klass.primary_key foreign_key = reflection.association_foreign_key end end - + conditions << table[key].eq(foreign_table[foreign_key]) - + conditions << reflection_conditions(index, table) conditions << sti_conditions(reflection, table) - + relation = relation.join(table, join_type).on(*conditions.flatten.compact) - + # The current table in this iteration becomes the foreign table in the next foreign_table = table index += 1 end - + relation end @@ -2317,11 +2317,11 @@ module ActiveRecord @tables.last end end - + def aliased_table_name table.table_alias || table.name end - + protected def table_alias_for(reflection, join = false) @@ -2336,7 +2336,7 @@ module ActiveRecord end private - + # Generate aliases and Arel::Table instances for each of the tables which we will # later generate joins for. We must do this in advance in order to correctly allocate # the proper alias. @@ -2346,44 +2346,44 @@ module ActiveRecord reflection.table_name, table_alias_for(reflection, reflection != self.reflection) ) - + table = Arel::Table.new( reflection.table_name, :engine => arel_engine, :as => aliased_table_name, :columns => reflection.klass.columns ) - + # For habtm, we have two Arel::Table instances related to a single reflection, so # we just store them as a pair in the array. if reflection.macro == :has_and_belongs_to_many || (reflection.source_reflection && reflection.source_reflection.macro == :has_and_belongs_to_many) - + join_table_name = (reflection.source_reflection || reflection).options[:join_table] - + aliased_join_table_name = alias_tracker.aliased_name_for( join_table_name, table_alias_for(reflection, true) ) - + join_table = Arel::Table.new( join_table_name, :engine => arel_engine, :as => aliased_join_table_name ) - + [table, join_table] else table end end - + # The joins are generated from the through_reflection_chain in reverse order, so # reverse the tables too (but it's important to generate the aliases in the 'forward' # order, which is why we only do the reversal now. @tables.reverse! - + @tables end - + def reflection_conditions(index, table) @reflection.through_conditions.reverse[index].map do |condition| Arel.sql(interpolate_sql(sanitize_sql( @@ -2392,28 +2392,28 @@ module ActiveRecord ))) end end - + def sti_conditions(reflection, table) unless reflection.klass.descends_from_active_record? sti_column = table[reflection.klass.inheritance_column] - + condition = sti_column.eq(reflection.klass.sti_name) - + reflection.klass.descendants.each do |subclass| condition = condition.or(sti_column.eq(subclass.sti_name)) end - + condition end end - + def source_type_conditions(reflection, foreign_table) if reflection.options[:source_type] foreign_table[reflection.source_reflection.options[:foreign_type]]. eq(reflection.options[:source_type]) end end - + def polymorphic_conditions(reflection, table) if reflection.options[:as] table["#{reflection.options[:as]}_type"]. diff --git a/activerecord/lib/active_record/associations/alias_tracker.rb b/activerecord/lib/active_record/associations/alias_tracker.rb index 10e90ec117..64582188b6 100644 --- a/activerecord/lib/active_record/associations/alias_tracker.rb +++ b/activerecord/lib/active_record/associations/alias_tracker.rb @@ -11,10 +11,10 @@ module ActiveRecord @aliases = Hash.new @other_sql = other_sql.to_s.downcase end - + def aliased_name_for(table_name, aliased_name = nil) aliased_name ||= table_name - + initialize_count_for(table_name) if @aliases[table_name].nil? if @aliases[table_name].zero? @@ -24,12 +24,12 @@ module ActiveRecord else # Otherwise, we need to use an alias aliased_name = connection.table_alias_for(aliased_name) - + initialize_count_for(aliased_name) if @aliases[aliased_name].nil? - + # Update the count @aliases[aliased_name] += 1 - + if @aliases[aliased_name] > 1 "#{truncate(aliased_name)}_#{@aliases[aliased_name]}" else @@ -41,30 +41,30 @@ module ActiveRecord def pluralize(table_name) ActiveRecord::Base.pluralize_table_names ? table_name.to_s.pluralize : table_name end - + private - + def initialize_count_for(name) @aliases[name] = 0 - + unless @other_sql.blank? # quoted_name should be downcased as some database adapters (Oracle) return quoted name in uppercase quoted_name = connection.quote_table_name(name.downcase).downcase - + # Table names @aliases[name] += @other_sql.scan(/join(?:\s+\w+)?\s+#{quoted_name}\son/).size - + # Table aliases @aliases[name] += @other_sql.scan(/join(?:\s+\w+)?\s+\S+\s+#{quoted_name}\son/).size end - + @aliases[name] end - + def truncate(name) name[0..connection.table_alias_length-3] end - + def connection ActiveRecord::Base.connection end diff --git a/activerecord/lib/active_record/associations/has_many_through_association.rb b/activerecord/lib/active_record/associations/has_many_through_association.rb index 2c9fa3b447..c45f2ee224 100644 --- a/activerecord/lib/active_record/associations/has_many_through_association.rb +++ b/activerecord/lib/active_record/associations/has_many_through_association.rb @@ -66,7 +66,7 @@ module ActiveRecord def insert_record(record, force = true, validate = true) ensure_not_nested - + if record.new_record? if force record.save! @@ -83,7 +83,7 @@ module ActiveRecord # TODO - add dependent option support def delete_records(records) ensure_not_nested - + klass = @reflection.through_reflection.klass records.each do |associate| klass.delete_all(construct_join_attributes(associate)) diff --git a/activerecord/lib/active_record/associations/has_one_through_association.rb b/activerecord/lib/active_record/associations/has_one_through_association.rb index de962e01b6..e9dc32efd3 100644 --- a/activerecord/lib/active_record/associations/has_one_through_association.rb +++ b/activerecord/lib/active_record/associations/has_one_through_association.rb @@ -15,7 +15,7 @@ module ActiveRecord def create_through_record(new_value) #nodoc: ensure_not_nested - + klass = @reflection.through_reflection.klass current_object = @owner.send(@reflection.through_reflection.name) diff --git a/activerecord/lib/active_record/associations/through_association_scope.rb b/activerecord/lib/active_record/associations/through_association_scope.rb index abe7af418d..07ce6f1597 100644 --- a/activerecord/lib/active_record/associations/through_association_scope.rb +++ b/activerecord/lib/active_record/associations/through_association_scope.rb @@ -18,7 +18,7 @@ module ActiveRecord :readonly => @reflection.options[:readonly] } end - + def construct_create_scope @reflection.nested? ? {} : construct_owner_attributes(@reflection) end @@ -26,18 +26,18 @@ module ActiveRecord # Build SQL conditions from attributes, qualified by table name. def construct_conditions reflection = @reflection.through_reflection_chain.last - + if reflection.macro == :has_and_belongs_to_many table_alias = table_aliases[reflection].first else table_alias = table_aliases[reflection] end - + parts = construct_quoted_owner_attributes(reflection).map do |attr, value| "#{table_alias}.#{attr} = #{value}" end parts += reflection_conditions(0) - + "(" + parts.join(') AND (') + ")" end @@ -59,18 +59,18 @@ module ActiveRecord distinct = "DISTINCT " if @reflection.options[:uniq] selected = custom_select || @reflection.options[:select] || "#{distinct}#{@reflection.quoted_table_name}.*" end - + def construct_joins(custom_joins = nil) "#{construct_through_joins} #{@reflection.options[:joins]} #{custom_joins}" end def construct_through_joins joins, right_index = [], 1 - + # Iterate over each pair in the through reflection chain, joining them together @reflection.through_reflection_chain.each_cons(2) do |left, right| right_table_and_alias = table_name_and_alias(right.quoted_table_name, table_aliases[right]) - + if left.source_reflection.nil? case left.macro when :belongs_to @@ -113,7 +113,7 @@ module ActiveRecord else right_table = table_aliases[right] end - + joins << inner_join_sql( right_table_and_alias, table_aliases[left], left.primary_key_name, @@ -121,7 +121,7 @@ module ActiveRecord polymorphic_conditions(left, left.source_reflection), reflection_conditions(right_index) ) - + if right.macro == :has_and_belongs_to_many joins << inner_join_sql( table_name_and_alias( @@ -134,7 +134,7 @@ module ActiveRecord end when :has_and_belongs_to_many join_table, left_table = table_aliases[left] - + joins << inner_join_sql( table_name_and_alias( quote_table_name(left.source_reflection.options[:join_table]), @@ -143,7 +143,7 @@ module ActiveRecord left_table, left.klass.primary_key, join_table, left.association_foreign_key ) - + joins << inner_join_sql( right_table_and_alias, join_table, left.primary_key_name, @@ -152,10 +152,10 @@ module ActiveRecord ) end end - + right_index += 1 end - + joins.join(" ") end @@ -170,77 +170,77 @@ module ActiveRecord reflection.table_name, table_alias_for(reflection, reflection != @reflection) )) - + if reflection.macro == :has_and_belongs_to_many || (reflection.source_reflection && reflection.source_reflection.macro == :has_and_belongs_to_many) - + join_table_alias = quote_table_name(alias_tracker.aliased_name_for( (reflection.source_reflection || reflection).options[:join_table], table_alias_for(reflection, true) )) - + aliases[reflection] = [join_table_alias, table_alias] else aliases[reflection] = table_alias end - + aliases end end end - + def table_alias_for(reflection, join = false) name = alias_tracker.pluralize(reflection.name) name << "_#{@reflection.name}" name << "_join" if join name end - + def quote_table_name(table_name) @reflection.klass.connection.quote_table_name(table_name) end - + def table_name_and_alias(table_name, table_alias) "#{table_name} #{table_alias if table_alias != table_name}".strip end - + def inner_join_sql(table, on_left_table, on_left_key, on_right_table, on_right_key, *conditions) conditions << "#{on_left_table}.#{on_left_key} = #{on_right_table}.#{on_right_key}" conditions = conditions.flatten.compact conditions = conditions.map { |sql| "(#{sql})" } * ' AND ' - + "INNER JOIN #{table} ON #{conditions}" end - + def reflection_conditions(index) reflection = @reflection.through_reflection_chain[index] reflection_conditions = @reflection.through_conditions[index] - + conditions = [] - + if reflection.options[:as].nil? && # reflection.klass is a Module if :as is used reflection.klass.finder_needs_type_condition? conditions << reflection.klass.send(:type_condition).to_sql end - + reflection_conditions.each do |condition| sanitized_condition = reflection.klass.send(:sanitize_sql, condition) interpolated_condition = interpolate_sql(sanitized_condition) - + if condition.is_a?(Hash) interpolated_condition.gsub!( @reflection.quoted_table_name, reflection.quoted_table_name ) end - + conditions << interpolated_condition end - + conditions end - + def polymorphic_conditions(reflection, polymorphic_reflection) if polymorphic_reflection.options[:as] "%s.%s = %s" % [ @@ -249,7 +249,7 @@ module ActiveRecord ] end end - + def source_type_conditions(reflection) if reflection.options[:source_type] "%s.%s = %s" % [ @@ -289,7 +289,7 @@ module ActiveRecord join_attributes end - + def ensure_not_nested if @reflection.nested? raise HasManyThroughNestedAssociationsAreReadonly.new(@owner, @reflection) diff --git a/activerecord/lib/active_record/reflection.rb b/activerecord/lib/active_record/reflection.rb index 6eb2057f66..ba37fed3c7 100644 --- a/activerecord/lib/active_record/reflection.rb +++ b/activerecord/lib/active_record/reflection.rb @@ -209,11 +209,11 @@ module ActiveRecord def association_foreign_key @association_foreign_key ||= @options[:association_foreign_key] || class_name.foreign_key end - + def association_primary_key @association_primary_key ||= @options[:primary_key] || klass.primary_key end - + def active_record_primary_key @active_record_primary_key ||= @options[:primary_key] || active_record.primary_key end @@ -249,11 +249,11 @@ module ActiveRecord def through_reflection false end - + def through_reflection_chain [self] end - + def through_conditions [Array.wrap(options[:conditions])] end @@ -340,7 +340,7 @@ module ActiveRecord # in the Active Record class. class ThroughReflection < AssociationReflection #:nodoc: delegate :primary_key_name, :association_foreign_key, :to => :source_reflection - + # Gets the source of the through reflection. It checks both a singularized # and pluralized form for :belongs_to or :has_many. # @@ -367,14 +367,14 @@ module ActiveRecord def through_reflection @through_reflection ||= active_record.reflect_on_association(options[:through]) end - + # Returns an array of AssociationReflection objects which are involved in this through # association. Each item in the array corresponds to a table which will be part of the # query for this association. - # + # # If the source reflection is itself a ThroughReflection, then we don't include self in # the chain, but just defer to the source reflection. - # + # # The chain is built by recursively calling through_reflection_chain on the source # reflection and the through reflection. The base case for the recursion is a normal # association, which just returns [self] for its through_reflection_chain. @@ -389,31 +389,31 @@ module ActiveRecord # to this reflection directly, and so start the chain here chain = [self] end - + # Recursively build the rest of the chain chain += through_reflection.through_reflection_chain - + # Finally return the completed chain chain end end - + # Consider the following example: - # + # # class Person # has_many :articles # has_many :comment_tags, :through => :articles # end - # + # # class Article # has_many :comments # has_many :comment_tags, :through => :comments, :source => :tags # end - # + # # class Comment # has_many :tags # end - # + # # There may be conditions on Person.comment_tags, Article.comment_tags and/or Comment.tags, # but only Comment.tags will be represented in the through_reflection_chain. So this method # creates an array of conditions corresponding to the through_reflection_chain. Each item in @@ -429,24 +429,24 @@ module ActiveRecord else conditions = [Array.wrap(source_reflection.options[:conditions])] end - + # Add to it the conditions from this reflection if necessary. conditions.first << options[:conditions] if options[:conditions] - + # Recursively fill out the rest of the array from the through reflection conditions += through_reflection.through_conditions - + # And return conditions end end - + # A through association is nested iff there would be more than one join table def nested? through_reflection_chain.length > 2 || through_reflection.macro == :has_and_belongs_to_many end - + # We want to use the klass from this reflection, rather than just delegate straight to # the source_reflection, because the source_reflection may be polymorphic. We still # need to respect the source_reflection's :primary_key option, though. @@ -458,7 +458,7 @@ module ActiveRecord while source_reflection.source_reflection source_reflection = source_reflection.source_reflection end - + source_reflection.options[:primary_key] || klass.primary_key end end -- cgit v1.2.3 From 73c0b390b3a1ea9487c3f667352463a90af6dd71 Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Fri, 4 Mar 2011 22:29:40 +0000 Subject: When preloading has_and_belongs_to_many associations, we should only instantiate one AR object per actual record in the database. (Even when IM is off.) --- .../active_record/associations/preloader/has_and_belongs_to_many.rb | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) (limited to 'activerecord/lib/active_record') diff --git a/activerecord/lib/active_record/associations/preloader/has_and_belongs_to_many.rb b/activerecord/lib/active_record/associations/preloader/has_and_belongs_to_many.rb index e794f05340..24be279449 100644 --- a/activerecord/lib/active_record/associations/preloader/has_and_belongs_to_many.rb +++ b/activerecord/lib/active_record/associations/preloader/has_and_belongs_to_many.rb @@ -31,10 +31,12 @@ module ActiveRecord private # Once we have used the join table column (in super), we manually instantiate the - # actual records + # actual records, ensuring that we don't create more than one instances of the same + # record def associated_records_by_owner + records = {} super.each do |owner_key, rows| - rows.map! { |row| klass.instantiate(row) } + rows.map! { |row| records[row[klass.primary_key]] ||= klass.instantiate(row) } end end -- cgit v1.2.3 From b5b5558d2f5347707862b8eeb1816da7c02a1d90 Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Fri, 4 Mar 2011 21:21:34 +0000 Subject: Fix a couple of tests in join_model_test.rb which were failing when the identity map is turned off --- activerecord/lib/active_record/associations/association.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'activerecord/lib/active_record') diff --git a/activerecord/lib/active_record/associations/association.rb b/activerecord/lib/active_record/associations/association.rb index 86904ea2bc..67752da2a5 100644 --- a/activerecord/lib/active_record/associations/association.rb +++ b/activerecord/lib/active_record/associations/association.rb @@ -191,8 +191,8 @@ module ActiveRecord else attributes[reflection.foreign_key] = owner[reflection.active_record_primary_key] - if options[:as] - attributes["#{options[:as]}_type"] = owner.class.base_class.name + if reflection.options[:as] + attributes[reflection.type] = owner.class.base_class.name end end attributes -- cgit v1.2.3 From 4206eff1895dccadcbec471798bfbd129404cc94 Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Fri, 4 Mar 2011 22:36:44 +0000 Subject: Stop identity-mapping the through records in the preloader since I fixed the underlying problem in the habtm preloader. --- .../lib/active_record/associations/preloader/through_association.rb | 5 ----- 1 file changed, 5 deletions(-) (limited to 'activerecord/lib/active_record') diff --git a/activerecord/lib/active_record/associations/preloader/through_association.rb b/activerecord/lib/active_record/associations/preloader/through_association.rb index 30558ae29c..ad6374d09a 100644 --- a/activerecord/lib/active_record/associations/preloader/through_association.rb +++ b/activerecord/lib/active_record/associations/preloader/through_association.rb @@ -33,13 +33,8 @@ module ActiveRecord through_options ).run - # TODO: Verify that this is actually necessary and not just a symptom of an - # underlying inefficiency - identity_map = {} - Hash[owners.map do |owner| through_records = Array.wrap(owner.send(through_reflection.name)) - through_records.map! { |record| identity_map[record] ||= record } # Dont cache the association - we would only be caching a subset if reflection.options[:source_type] && through_reflection.collection? -- cgit v1.2.3 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 ++++++++++++++-------- activerecord/lib/active_record/reflection.rb | 3 ++ 2 files changed, 25 insertions(+), 13 deletions(-) (limited to 'activerecord/lib/active_record') 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 diff --git a/activerecord/lib/active_record/reflection.rb b/activerecord/lib/active_record/reflection.rb index e3e2cac042..7ae9bfc928 100644 --- a/activerecord/lib/active_record/reflection.rb +++ b/activerecord/lib/active_record/reflection.rb @@ -416,6 +416,9 @@ module ActiveRecord else # If the source reflection does not go through another reflection, then we can get # to this reflection directly, and so start the chain here + # + # It is important to use self, rather than the source_reflection, because self + # may has a :source_type option which needs to be used. chain = [self] 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. --- .../join_dependency/join_association.rb | 18 ------------------ .../associations/through_association.rb | 17 ----------------- activerecord/lib/active_record/reflection.rb | 21 +++++++++++---------- 3 files changed, 11 insertions(+), 45 deletions(-) (limited to 'activerecord/lib/active_record') diff --git a/activerecord/lib/active_record/associations/join_dependency/join_association.rb b/activerecord/lib/active_record/associations/join_dependency/join_association.rb index 890e77fca9..5da3416023 100644 --- a/activerecord/lib/active_record/associations/join_dependency/join_association.rb +++ b/activerecord/lib/active_record/associations/join_dependency/join_association.rb @@ -76,8 +76,6 @@ module ActiveRecord when :has_many, :has_one key = reflection.foreign_key foreign_key = reflection.active_record_primary_key - - conditions << polymorphic_conditions(reflection, table) when :has_and_belongs_to_many # For habtm, we need to deal with the join table at the same time as the # target table (because unlike a :through association, there is no reflection @@ -103,8 +101,6 @@ module ActiveRecord when :belongs_to key = reflection.association_primary_key foreign_key = reflection.foreign_key - - conditions << source_type_conditions(reflection, foreign_table) when :has_many, :has_one key = reflection.foreign_key foreign_key = reflection.source_reflection.active_record_primary_key @@ -239,20 +235,6 @@ module ActiveRecord end end - def source_type_conditions(reflection, foreign_table) - if reflection.options[:source_type] - foreign_table[reflection.source_reflection.foreign_type]. - eq(reflection.options[:source_type]) - end - end - - def polymorphic_conditions(reflection, table) - if reflection.options[:as] - table[reflection.type]. - eq(reflection.active_record.base_class.name) - end - end - end end end 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 diff --git a/activerecord/lib/active_record/reflection.rb b/activerecord/lib/active_record/reflection.rb index 7ae9bfc928..82f648b873 100644 --- a/activerecord/lib/active_record/reflection.rb +++ b/activerecord/lib/active_record/reflection.rb @@ -270,7 +270,9 @@ module ActiveRecord end def through_conditions - [Array.wrap(options[:conditions])] + through_conditions = [Array.wrap(options[:conditions])] + through_conditions.first << { type => active_record.base_class.name } if options[:as] + through_conditions end def source_reflection @@ -453,20 +455,19 @@ module ActiveRecord # itself an array of conditions from an arbitrary number of relevant reflections. def through_conditions @through_conditions ||= begin - # Initialize the first item - which corresponds to this reflection - either by recursing - # into the souce reflection (if it is itself a through reflection), or by grabbing the - # source reflection conditions. - if source_reflection.source_reflection - conditions = source_reflection.through_conditions - else - conditions = [Array.wrap(source_reflection.options[:conditions])] - end + conditions = source_reflection.through_conditions # Add to it the conditions from this reflection if necessary. conditions.first << options[:conditions] if options[:conditions] + through_conditions = through_reflection.through_conditions + + if options[:source_type] + through_conditions.first << { foreign_type => options[:source_type] } + end + # Recursively fill out the rest of the array from the through reflection - conditions += through_reflection.through_conditions + conditions += through_conditions # And return conditions -- cgit v1.2.3 From b7f1b3641afe0ff4f3cd344815c6f7bb58821e9e Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Sat, 5 Mar 2011 22:32:49 +0000 Subject: Use Base#type_condition in JoinAssociation --- .../associations/join_dependency/join_association.rb | 19 ++++--------------- activerecord/lib/active_record/base.rb | 4 ++-- 2 files changed, 6 insertions(+), 17 deletions(-) (limited to 'activerecord/lib/active_record') diff --git a/activerecord/lib/active_record/associations/join_dependency/join_association.rb b/activerecord/lib/active_record/associations/join_dependency/join_association.rb index 5da3416023..fc8e75b10d 100644 --- a/activerecord/lib/active_record/associations/join_dependency/join_association.rb +++ b/activerecord/lib/active_record/associations/join_dependency/join_association.rb @@ -123,9 +123,11 @@ module ActiveRecord end conditions << table[key].eq(foreign_table[foreign_key]) - conditions << reflection_conditions(index, table) - conditions << sti_conditions(reflection, table) + + if reflection.klass.finder_needs_type_condition? + conditions << reflection.klass.send(:type_condition, table) + end ands = relation.create_and(conditions.flatten.compact) @@ -222,19 +224,6 @@ module ActiveRecord end end - def sti_conditions(reflection, table) - unless reflection.klass.descends_from_active_record? - sti_column = table[reflection.klass.inheritance_column] - sti_condition = sti_column.eq(reflection.klass.sti_name) - subclasses = reflection.klass.descendants - - # TODO: use IN (...), or possibly AR::Base#type_condition - subclasses.inject(sti_condition) { |attr,subclass| - attr.or(sti_column.eq(subclass.sti_name)) - } - end - end - end end end diff --git a/activerecord/lib/active_record/base.rb b/activerecord/lib/active_record/base.rb index b3204b2bda..baf82bedd3 100644 --- a/activerecord/lib/active_record/base.rb +++ b/activerecord/lib/active_record/base.rb @@ -973,8 +973,8 @@ module ActiveRecord #:nodoc: relation end - def type_condition - sti_column = arel_table[inheritance_column.to_sym] + def type_condition(table = arel_table) + sti_column = table[inheritance_column.to_sym] sti_names = ([self] + descendants).map { |model| model.sti_name } sti_column.in(sti_names) -- 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') 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 ++++++--------------- activerecord/lib/active_record/reflection.rb | 6 ++ 2 files changed, 34 insertions(+), 84 deletions(-) (limited to 'activerecord/lib/active_record') 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))) diff --git a/activerecord/lib/active_record/reflection.rb b/activerecord/lib/active_record/reflection.rb index 82f648b873..5199886f79 100644 --- a/activerecord/lib/active_record/reflection.rb +++ b/activerecord/lib/active_record/reflection.rb @@ -279,6 +279,8 @@ module ActiveRecord nil end + alias :source_macro :macro + def has_inverse? @options[:inverse_of] end @@ -474,6 +476,10 @@ module ActiveRecord end end + def source_macro + source_reflection.source_macro + end + # A through association is nested iff there would be more than one join table def nested? through_reflection_chain.length > 2 || -- cgit v1.2.3 From cee3f9b36d01e6d54e0bd4c2fd06bee369bfff12 Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Mon, 7 Mar 2011 00:04:06 +0000 Subject: Referencing a table via the ON condition in a join should force that table to be eager-loaded via a JOIN rather than via subsequent queries. --- activerecord/lib/active_record/relation.rb | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) (limited to 'activerecord/lib/active_record') diff --git a/activerecord/lib/active_record/relation.rb b/activerecord/lib/active_record/relation.rb index f939bedc81..5af20bf38b 100644 --- a/activerecord/lib/active_record/relation.rb +++ b/activerecord/lib/active_record/relation.rb @@ -407,8 +407,19 @@ module ActiveRecord private def references_eager_loaded_tables? + joined_tables = arel.join_sources.map do |join| + if join.is_a?(Arel::Nodes::StringJoin) + tables_in_string(join.left) + else + [join.left.table_name, join.left.table_alias] + end + end + + joined_tables += [table.name, table.table_alias] + # always convert table names to downcase as in Oracle quoted table names are in uppercase - joined_tables = (tables_in_string(arel.join_sql) + [table.name, table.table_alias]).compact.map{ |t| t.downcase }.uniq + joined_tables = joined_tables.flatten.compact.map { |t| t.downcase }.uniq + (tables_in_string(to_sql) - joined_tables).any? end -- 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 +- activerecord/lib/active_record/reflection.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) (limited to 'activerecord/lib/active_record') 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 diff --git a/activerecord/lib/active_record/reflection.rb b/activerecord/lib/active_record/reflection.rb index 5199886f79..0a9855ec25 100644 --- a/activerecord/lib/active_record/reflection.rb +++ b/activerecord/lib/active_record/reflection.rb @@ -372,7 +372,7 @@ module ActiveRecord # Holds all the meta-data about a :through association as it was specified # in the Active Record class. class ThroughReflection < AssociationReflection #:nodoc: - delegate :foreign_key, :foreign_type, :association_foreign_key, :to => :source_reflection + delegate :foreign_key, :foreign_type, :association_foreign_key, :active_record_primary_key, :to => :source_reflection # Gets the source of the through reflection. It checks both a singularized # and pluralized form for :belongs_to or :has_many. -- 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. --- activerecord/lib/active_record/associations.rb | 7 +- .../lib/active_record/associations/association.rb | 44 +----- .../associations/association_scope.rb | 149 +++++++++++++++++++++ .../associations/collection_association.rb | 13 -- .../has_and_belongs_to_many_association.rb | 22 --- .../associations/has_many_association.rb | 2 - .../associations/has_one_association.rb | 6 - .../associations/through_association.rb | 128 ------------------ activerecord/lib/active_record/reflection.rb | 6 +- 9 files changed, 162 insertions(+), 215 deletions(-) create mode 100644 activerecord/lib/active_record/associations/association_scope.rb (limited to 'activerecord/lib/active_record') diff --git a/activerecord/lib/active_record/associations.rb b/activerecord/lib/active_record/associations.rb index ec5b41a3e7..90745112b1 100644 --- a/activerecord/lib/active_record/associations.rb +++ b/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 :JoinDependency, 'active_record/associations/join_dependency' - autoload :AliasTracker, 'active_record/associations/alias_tracker' + autoload :Preloader, 'active_record/associations/preloader' + autoload :JoinDependency, 'active_record/associations/join_dependency' + autoload :AssociationScope, 'active_record/associations/association_scope' + autoload :AliasTracker, 'active_record/associations/alias_tracker' # Clears out the association cache. def clear_association_cache #:nodoc: diff --git a/activerecord/lib/active_record/associations/association.rb b/activerecord/lib/active_record/associations/association.rb index 25b4b9d90d..27c446b12c 100644 --- a/activerecord/lib/active_record/associations/association.rb +++ b/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 - end - - 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) + if klass + @association_scope = AssociationScope.new(self).scope 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] - else + + if [:has_one, :has_many].include?(reflection.macro) && !options[:through] 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 - 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) + attributes 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 diff --git a/activerecord/lib/active_record/associations/association_scope.rb b/activerecord/lib/active_record/associations/association_scope.rb new file mode 100644 index 0000000000..5df2e964be --- /dev/null +++ b/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 diff --git a/activerecord/lib/active_record/associations/collection_association.rb b/activerecord/lib/active_record/associations/collection_association.rb index f3761bd2c7..9f4fc44cc6 100644 --- a/activerecord/lib/active_record/associations/collection_association.rb +++ b/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]) 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 028630977d..217213808b 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 @@ -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 diff --git a/activerecord/lib/active_record/associations/has_many_association.rb b/activerecord/lib/active_record/associations/has_many_association.rb index cebf3e477a..78c5c4b870 100644 --- a/activerecord/lib/active_record/associations/has_many_association.rb +++ b/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 diff --git a/activerecord/lib/active_record/associations/has_one_association.rb b/activerecord/lib/active_record/associations/has_one_association.rb index e13f97125f..1d2e8667e4 100644 --- a/activerecord/lib/active_record/associations/has_one_association.rb +++ b/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 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 diff --git a/activerecord/lib/active_record/reflection.rb b/activerecord/lib/active_record/reflection.rb index 0a9855ec25..8c73e49e74 100644 --- a/activerecord/lib/active_record/reflection.rb +++ b/activerecord/lib/active_record/reflection.rb @@ -270,9 +270,9 @@ module ActiveRecord end def through_conditions - through_conditions = [Array.wrap(options[:conditions])] - through_conditions.first << { type => active_record.base_class.name } if options[:as] - through_conditions + conditions = [options[:conditions]].compact + conditions << { type => active_record.base_class.name } if options[:as] + [conditions] end def source_reflection -- 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. --- .../associations/association_scope.rb | 12 ++--- .../join_dependency/join_association.rb | 16 +++--- .../associations/through_association.rb | 5 +- activerecord/lib/active_record/reflection.rb | 60 ++++++++++++---------- 4 files changed, 48 insertions(+), 45 deletions(-) (limited to 'activerecord/lib/active_record') diff --git a/activerecord/lib/active_record/associations/association_scope.rb b/activerecord/lib/active_record/associations/association_scope.rb index 5df2e964be..cf859bbdee 100644 --- a/activerecord/lib/active_record/associations/association_scope.rb +++ b/activerecord/lib/active_record/associations/association_scope.rb @@ -4,7 +4,7 @@ module ActiveRecord attr_reader :association, :alias_tracker delegate :klass, :owner, :reflection, :interpolate, :to => :association - delegate :through_reflection_chain, :through_conditions, :options, :source_options, :to => :reflection + delegate :chain, :conditions, :options, :source_options, :to => :reflection def initialize(association) @association = association @@ -50,7 +50,7 @@ module ActiveRecord def add_constraints(scope) tables = construct_tables - through_reflection_chain.each_with_index do |reflection, i| + chain.each_with_index do |reflection, i| table, foreign_table = tables.shift, tables.first if reflection.source_macro == :has_and_belongs_to_many @@ -73,10 +73,10 @@ module ActiveRecord foreign_key = reflection.active_record_primary_key end - if reflection == through_reflection_chain.last + if reflection == chain.last scope = scope.where(table[key].eq(owner[foreign_key])) - through_conditions[i].each do |condition| + conditions[i].each do |condition| if options[:through] && condition.is_a?(Hash) condition = { table.name => condition } end @@ -86,7 +86,7 @@ module ActiveRecord else constraint = table[key].eq foreign_table[foreign_key] - join = inner_join(foreign_table, reflection, constraint, *through_conditions[i]) + join = inner_join(foreign_table, reflection, constraint, *conditions[i]) scope = scope.joins(join) end end @@ -96,7 +96,7 @@ module ActiveRecord def construct_tables tables = [] - through_reflection_chain.each do |reflection| + chain.each do |reflection| tables << alias_tracker.aliased_table_for( table_name_for(reflection), table_alias_for(reflection, reflection != self.reflection) diff --git a/activerecord/lib/active_record/associations/join_dependency/join_association.rb b/activerecord/lib/active_record/associations/join_dependency/join_association.rb index fc8e75b10d..f26881a6c6 100644 --- a/activerecord/lib/active_record/associations/join_dependency/join_association.rb +++ b/activerecord/lib/active_record/associations/join_dependency/join_association.rb @@ -22,7 +22,7 @@ module ActiveRecord attr_reader :tables - delegate :options, :through_reflection, :source_reflection, :through_reflection_chain, :to => :reflection + delegate :options, :through_reflection, :source_reflection, :chain, :to => :reflection delegate :table, :table_name, :to => :parent, :prefix => :parent delegate :alias_tracker, :to => :join_dependency @@ -57,14 +57,12 @@ module ActiveRecord end def join_to(relation) - # The chain starts with the target table, but we want to end with it here (makes - # more sense in this context) - chain = through_reflection_chain.reverse - foreign_table = parent_table index = 0 - chain.each do |reflection| + # The chain starts with the target table, but we want to end with it here (makes + # more sense in this context), so we reverse + chain.reverse.each do |reflection| table = tables[index] conditions = [] @@ -178,7 +176,7 @@ module ActiveRecord # later generate joins for. We must do this in advance in order to correctly allocate # the proper alias. def setup_tables - @tables = through_reflection_chain.map do |reflection| + @tables = chain.map do |reflection| table = alias_tracker.aliased_table_for( reflection.table_name, table_alias_for(reflection, reflection != self.reflection) @@ -200,7 +198,7 @@ module ActiveRecord end end - # The joins are generated from the through_reflection_chain in reverse order, so + # The joins are generated from the chain in reverse order, so # reverse the tables too (but it's important to generate the aliases in the 'forward' # order, which is why we only do the reversal now. @tables.reverse! @@ -219,7 +217,7 @@ module ActiveRecord end def reflection_conditions(index, table) - reflection.through_conditions.reverse[index].map do |condition| + reflection.conditions.reverse[index].map do |condition| process_conditions(condition, table.table_alias || table.name) end end 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 diff --git a/activerecord/lib/active_record/reflection.rb b/activerecord/lib/active_record/reflection.rb index 8c73e49e74..1f3ace93a9 100644 --- a/activerecord/lib/active_record/reflection.rb +++ b/activerecord/lib/active_record/reflection.rb @@ -262,23 +262,28 @@ module ActiveRecord end def through_reflection - false + nil + end + + def source_reflection + nil end - def through_reflection_chain + # A chain of reflections from this one back to the owner. For more see the explanation in + # ThroughReflection. + def chain [self] end - def through_conditions + # An array of arrays of conditions. Each item in the outside array corresponds to a reflection + # in the #chain. The inside arrays are simply conditions (and each condition may itself be + # a hash, array, arel predicate, etc...) + def conditions conditions = [options[:conditions]].compact conditions << { type => active_record.base_class.name } if options[:as] [conditions] end - def source_reflection - nil - end - alias :source_macro :macro def has_inverse? @@ -401,33 +406,34 @@ module ActiveRecord @through_reflection ||= active_record.reflect_on_association(options[:through]) end - # Returns an array of AssociationReflection objects which are involved in this through - # association. Each item in the array corresponds to a table which will be part of the - # query for this association. + # Returns an array of reflections which are involved in this association. Each item in the + # array corresponds to a table which will be part of the query for this association. # # If the source reflection is itself a ThroughReflection, then we don't include self in # the chain, but just defer to the source reflection. # - # The chain is built by recursively calling through_reflection_chain on the source - # reflection and the through reflection. The base case for the recursion is a normal - # association, which just returns [self] for its through_reflection_chain. - def through_reflection_chain - @through_reflection_chain ||= begin + # The chain is built by recursively calling #chain on the source reflection and the through + # reflection. The base case for the recursion is a normal association, which just returns + # [self] as its #chain. + def chain + @chain ||= begin if source_reflection.source_reflection # If the source reflection has its own source reflection, then the chain must start # by getting us to that source reflection. - chain = source_reflection.through_reflection_chain + chain = source_reflection.chain else # If the source reflection does not go through another reflection, then we can get # to this reflection directly, and so start the chain here # # It is important to use self, rather than the source_reflection, because self # may has a :source_type option which needs to be used. + # + # FIXME: Not sure this is correct justification now that we have #conditions chain = [self] end # Recursively build the rest of the chain - chain += through_reflection.through_reflection_chain + chain += through_reflection.chain # Finally return the completed chain chain @@ -451,18 +457,18 @@ module ActiveRecord # end # # There may be conditions on Person.comment_tags, Article.comment_tags and/or Comment.tags, - # but only Comment.tags will be represented in the through_reflection_chain. So this method - # creates an array of conditions corresponding to the through_reflection_chain. Each item in - # the through_conditions array corresponds to an item in the through_reflection_chain, and is - # itself an array of conditions from an arbitrary number of relevant reflections. - def through_conditions - @through_conditions ||= begin - conditions = source_reflection.through_conditions + # but only Comment.tags will be represented in the #chain. So this method creates an array + # of conditions corresponding to the chain. Each item in the #conditions array corresponds + # to an item in the #chain, and is itself an array of conditions from an arbitrary number + # of relevant reflections, plus any :source_type or polymorphic :as constraints. + def conditions + @conditions ||= begin + conditions = source_reflection.conditions # Add to it the conditions from this reflection if necessary. conditions.first << options[:conditions] if options[:conditions] - through_conditions = through_reflection.through_conditions + through_conditions = through_reflection.conditions if options[:source_type] through_conditions.first << { foreign_type => options[:source_type] } @@ -476,14 +482,14 @@ module ActiveRecord end end + # The macro used by the source association def source_macro source_reflection.source_macro end # A through association is nested iff there would be more than one join table def nested? - through_reflection_chain.length > 2 || - through_reflection.macro == :has_and_belongs_to_many + chain.length > 2 || through_reflection.macro == :has_and_belongs_to_many end # We want to use the klass from this reflection, rather than just delegate straight to -- cgit v1.2.3 From aef3629c6ebae013333e11911934dfff28de875a Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Thu, 10 Mar 2011 23:55:29 +0000 Subject: Refactor JoinAssociation --- .../join_dependency/join_association.rb | 154 ++++++++------------- 1 file changed, 54 insertions(+), 100 deletions(-) (limited to 'activerecord/lib/active_record') diff --git a/activerecord/lib/active_record/associations/join_dependency/join_association.rb b/activerecord/lib/active_record/associations/join_dependency/join_association.rb index f26881a6c6..750f03a120 100644 --- a/activerecord/lib/active_record/associations/join_dependency/join_association.rb +++ b/activerecord/lib/active_record/associations/join_dependency/join_association.rb @@ -57,88 +57,46 @@ module ActiveRecord end def join_to(relation) + tables = @tables.dup foreign_table = parent_table - index = 0 # The chain starts with the target table, but we want to end with it here (makes # more sense in this context), so we reverse - chain.reverse.each do |reflection| - table = tables[index] - conditions = [] - - if reflection.source_reflection.nil? - case reflection.macro - when :belongs_to - key = reflection.association_primary_key - foreign_key = reflection.foreign_key - when :has_many, :has_one - key = reflection.foreign_key - foreign_key = reflection.active_record_primary_key - when :has_and_belongs_to_many - # For habtm, we need to deal with the join table at the same time as the - # target table (because unlike a :through association, there is no reflection - # to represent the join table) - table, join_table = table - - join_key = reflection.foreign_key - join_foreign_key = reflection.active_record.primary_key - - relation = relation.join(join_table, join_type).on( - join_table[join_key]. - eq(foreign_table[join_foreign_key]) - ) - - # We've done the first join now, so update the foreign_table for the second - foreign_table = join_table - - key = reflection.klass.primary_key - foreign_key = reflection.association_foreign_key - end + chain.reverse.each_with_index do |reflection, i| + table = tables.shift + + case reflection.source_macro + when :belongs_to + key = reflection.association_primary_key + foreign_key = reflection.foreign_key + when :has_and_belongs_to_many + # Join the join table first... + relation = relation.from(join( + table, + table[reflection.foreign_key]. + eq(foreign_table[reflection.active_record_primary_key]) + )) + + foreign_table, table = table, tables.shift + + key = reflection.association_primary_key + foreign_key = reflection.association_foreign_key else - case reflection.source_reflection.macro - when :belongs_to - key = reflection.association_primary_key - foreign_key = reflection.foreign_key - when :has_many, :has_one - key = reflection.foreign_key - foreign_key = reflection.source_reflection.active_record_primary_key - when :has_and_belongs_to_many - table, join_table = table - - join_key = reflection.foreign_key - join_foreign_key = reflection.klass.primary_key - - relation = relation.join(join_table, join_type).on( - join_table[join_key]. - eq(foreign_table[join_foreign_key]) - ) - - foreign_table = join_table - - key = reflection.klass.primary_key - foreign_key = reflection.association_foreign_key - end + key = reflection.foreign_key + foreign_key = reflection.active_record_primary_key end + conditions = self.conditions[i].dup conditions << table[key].eq(foreign_table[foreign_key]) - conditions << reflection_conditions(index, table) if reflection.klass.finder_needs_type_condition? conditions << reflection.klass.send(:type_condition, table) end - ands = relation.create_and(conditions.flatten.compact) - - join = relation.create_join( - table, - relation.create_on(ands), - join_type) - - relation = relation.from(join) + relation = relation.from(join(table, *conditions)) # The current table in this iteration becomes the foreign table in the next foreign_table = table - index += 1 end relation @@ -150,18 +108,18 @@ module ActiveRecord end def table - if tables.last.is_a?(Array) - tables.last.first - else - tables.last - end + tables.last end def aliased_table_name table.table_alias || table.name end - protected + def conditions + @conditions ||= reflection.conditions.reverse + end + + private def table_alias_for(reflection, join = false) name = alias_tracker.pluralize(reflection.name) @@ -170,55 +128,51 @@ module ActiveRecord name end - private - # Generate aliases and Arel::Table instances for each of the tables which we will # later generate joins for. We must do this in advance in order to correctly allocate # the proper alias. def setup_tables - @tables = chain.map do |reflection| - table = alias_tracker.aliased_table_for( + @tables = [] + chain.each do |reflection| + @tables << alias_tracker.aliased_table_for( reflection.table_name, table_alias_for(reflection, reflection != self.reflection) ) - # For habtm, we have two Arel::Table instances related to a single reflection, so - # we just store them as a pair in the array. - 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( + 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) ) - - [table, join_table] - else - table end end - # The joins are generated from the chain in reverse order, so - # reverse the tables too (but it's important to generate the aliases in the 'forward' - # order, which is why we only do the reversal now. + # We construct the tables in the forward order so that the aliases are generated + # correctly, but then reverse the array because that is the order in which we will + # iterate the chain. @tables.reverse! end - def process_conditions(conditions, table_name) - if conditions.respond_to?(:to_proc) - conditions = instance_eval(&conditions) - end - - Arel.sql(sanitize_sql(conditions, table_name)) + def join(table, *conditions) + conditions = sanitize_conditions(table, conditions) + table.create_join(table, table.create_on(conditions), join_type) end - def sanitize_sql(condition, table_name) - active_record.send(:sanitize_sql, condition, table_name) + def sanitize_conditions(table, conditions) + conditions = conditions.map do |condition| + condition = active_record.send(:sanitize_sql, interpolate(condition), table.table_alias || 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 - def reflection_conditions(index, table) - reflection.conditions.reverse[index].map do |condition| - process_conditions(condition, table.table_alias || table.name) + def interpolate(conditions) + if conditions.respond_to?(:to_proc) + instance_eval(&conditions) + else + conditions end end -- cgit v1.2.3 From e18679ab0428797369027fc549ef964c8c2038ba Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Fri, 11 Mar 2011 00:47:18 +0000 Subject: Abstract some common code from AssociationScope and JoinDependency::JoinAssociation into a JoinHelper module --- activerecord/lib/active_record/associations.rb | 1 + .../associations/association_scope.rb | 56 ++++--------------- .../join_dependency/join_association.rb | 65 ++++------------------ .../lib/active_record/associations/join_helper.rb | 58 +++++++++++++++++++ 4 files changed, 83 insertions(+), 97 deletions(-) create mode 100644 activerecord/lib/active_record/associations/join_helper.rb (limited to 'activerecord/lib/active_record') diff --git a/activerecord/lib/active_record/associations.rb b/activerecord/lib/active_record/associations.rb index 90745112b1..08fb6bf3c4 100644 --- a/activerecord/lib/active_record/associations.rb +++ b/activerecord/lib/active_record/associations.rb @@ -144,6 +144,7 @@ module ActiveRecord autoload :JoinDependency, 'active_record/associations/join_dependency' autoload :AssociationScope, 'active_record/associations/association_scope' autoload :AliasTracker, 'active_record/associations/alias_tracker' + autoload :JoinHelper, 'active_record/associations/join_helper' # Clears out the association cache. def clear_association_cache #:nodoc: diff --git a/activerecord/lib/active_record/associations/association_scope.rb b/activerecord/lib/active_record/associations/association_scope.rb index cf859bbdee..b9bbeed4d5 100644 --- a/activerecord/lib/active_record/associations/association_scope.rb +++ b/activerecord/lib/active_record/associations/association_scope.rb @@ -1,10 +1,12 @@ module ActiveRecord module Associations class AssociationScope #:nodoc: + include JoinHelper + attr_reader :association, :alias_tracker delegate :klass, :owner, :reflection, :interpolate, :to => :association - delegate :chain, :conditions, :options, :source_options, :to => :reflection + delegate :chain, :conditions, :options, :source_options, :active_record, :to => :reflection def initialize(association) @association = association @@ -56,8 +58,8 @@ module ActiveRecord if reflection.source_macro == :has_and_belongs_to_many join_table = tables.shift - scope = scope.joins(inner_join( - join_table, reflection, + scope = scope.joins(join( + join_table, table[reflection.active_record_primary_key]. eq(join_table[reflection.association_foreign_key]) )) @@ -84,32 +86,19 @@ module ActiveRecord scope = scope.where(interpolate(condition)) end else - constraint = table[key].eq foreign_table[foreign_key] - - join = inner_join(foreign_table, reflection, constraint, *conditions[i]) - scope = scope.joins(join) + scope = scope.joins(join( + foreign_table, + table[key].eq(foreign_table[foreign_key]), + *conditions[i] + )) end end scope end - def construct_tables - tables = [] - 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 + def alias_suffix + reflection.name end def table_name_for(reflection) @@ -123,27 +112,6 @@ module ActiveRecord 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 diff --git a/activerecord/lib/active_record/associations/join_dependency/join_association.rb b/activerecord/lib/active_record/associations/join_dependency/join_association.rb index 750f03a120..7dc6beeede 100644 --- a/activerecord/lib/active_record/associations/join_dependency/join_association.rb +++ b/activerecord/lib/active_record/associations/join_dependency/join_association.rb @@ -2,6 +2,8 @@ module ActiveRecord module Associations class JoinDependency # :nodoc: class JoinAssociation < JoinPart # :nodoc: + include JoinHelper + # The reflection of the association represented attr_reader :reflection @@ -26,6 +28,8 @@ module ActiveRecord delegate :table, :table_name, :to => :parent, :prefix => :parent delegate :alias_tracker, :to => :join_dependency + alias :alias_suffix :parent_table_name + def initialize(reflection, join_dependency, parent = nil) reflection.check_validity! @@ -40,8 +44,7 @@ module ActiveRecord @parent = parent @join_type = Arel::InnerJoin @aliased_prefix = "t#{ join_dependency.join_parts.size }" - - setup_tables + @tables = construct_tables.reverse end def ==(other) @@ -86,14 +89,17 @@ module ActiveRecord foreign_key = reflection.active_record_primary_key end - conditions = self.conditions[i].dup - conditions << table[key].eq(foreign_table[foreign_key]) + conditions = self.conditions[i] if reflection.klass.finder_needs_type_condition? - conditions << reflection.klass.send(:type_condition, table) + conditions += [reflection.klass.send(:type_condition, table)] end - relation = relation.from(join(table, *conditions)) + relation = relation.from(join( + table, + table[key].eq(foreign_table[foreign_key]), + *conditions + )) # The current table in this iteration becomes the foreign table in the next foreign_table = table @@ -121,53 +127,6 @@ module ActiveRecord private - def table_alias_for(reflection, join = false) - name = alias_tracker.pluralize(reflection.name) - name << "_#{parent_table_name}" - name << "_join" if join - name - end - - # Generate aliases and Arel::Table instances for each of the tables which we will - # later generate joins for. We must do this in advance in order to correctly allocate - # the proper alias. - def setup_tables - @tables = [] - chain.each do |reflection| - @tables << alias_tracker.aliased_table_for( - reflection.table_name, - 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 - - # We construct the tables in the forward order so that the aliases are generated - # correctly, but then reverse the array because that is the order in which we will - # iterate the chain. - @tables.reverse! - end - - def join(table, *conditions) - conditions = sanitize_conditions(table, conditions) - table.create_join(table, table.create_on(conditions), join_type) - end - - def sanitize_conditions(table, conditions) - conditions = conditions.map do |condition| - condition = active_record.send(:sanitize_sql, interpolate(condition), table.table_alias || 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 - def interpolate(conditions) if conditions.respond_to?(:to_proc) instance_eval(&conditions) diff --git a/activerecord/lib/active_record/associations/join_helper.rb b/activerecord/lib/active_record/associations/join_helper.rb new file mode 100644 index 0000000000..6474f39503 --- /dev/null +++ b/activerecord/lib/active_record/associations/join_helper.rb @@ -0,0 +1,58 @@ +module ActiveRecord + module Associations + # Helper class module which gets mixed into JoinDependency::JoinAssociation and AssociationScope + module JoinHelper #:nodoc: + + def join_type + Arel::InnerJoin + end + + private + + def construct_tables + tables = [] + 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) + reflection.table_name + end + + def table_alias_for(reflection, join = false) + name = alias_tracker.pluralize(reflection.name) + name << "_#{alias_suffix}" + name << "_join" if join + name + end + + def join(table, *conditions) + table.create_join(table, table.create_on(sanitize(conditions)), join_type) + end + + def sanitize(conditions) + table = conditions.first.left.relation + + conditions = conditions.map do |condition| + condition = active_record.send(:sanitize_sql, interpolate(condition), table.table_alias || 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 -- cgit v1.2.3 From 39a6f4f25d958783c73377ac52886c9edc19632e Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Fri, 11 Mar 2011 00:51:57 +0000 Subject: Simplify implementation of ThroughReflection#chain --- activerecord/lib/active_record/reflection.rb | 24 ++---------------------- 1 file changed, 2 insertions(+), 22 deletions(-) (limited to 'activerecord/lib/active_record') diff --git a/activerecord/lib/active_record/reflection.rb b/activerecord/lib/active_record/reflection.rb index 1f3ace93a9..e801bc4afa 100644 --- a/activerecord/lib/active_record/reflection.rb +++ b/activerecord/lib/active_record/reflection.rb @@ -409,33 +409,13 @@ module ActiveRecord # Returns an array of reflections which are involved in this association. Each item in the # array corresponds to a table which will be part of the query for this association. # - # If the source reflection is itself a ThroughReflection, then we don't include self in - # the chain, but just defer to the source reflection. - # # The chain is built by recursively calling #chain on the source reflection and the through # reflection. The base case for the recursion is a normal association, which just returns # [self] as its #chain. def chain @chain ||= begin - if source_reflection.source_reflection - # If the source reflection has its own source reflection, then the chain must start - # by getting us to that source reflection. - chain = source_reflection.chain - else - # If the source reflection does not go through another reflection, then we can get - # to this reflection directly, and so start the chain here - # - # It is important to use self, rather than the source_reflection, because self - # may has a :source_type option which needs to be used. - # - # FIXME: Not sure this is correct justification now that we have #conditions - chain = [self] - end - - # Recursively build the rest of the chain - chain += through_reflection.chain - - # Finally return the completed chain + chain = source_reflection.chain + through_reflection.chain + chain[0] = self # Use self so we don't lose the information from :source_type chain end end -- 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 --- activerecord/lib/active_record/associations/alias_tracker.rb | 2 +- .../lib/active_record/associations/through_association.rb | 11 ++++------- activerecord/lib/active_record/relation/query_methods.rb | 1 - 3 files changed, 5 insertions(+), 9 deletions(-) (limited to 'activerecord/lib/active_record') diff --git a/activerecord/lib/active_record/associations/alias_tracker.rb b/activerecord/lib/active_record/associations/alias_tracker.rb index 6fc2bfdb31..634dee2289 100644 --- a/activerecord/lib/active_record/associations/alias_tracker.rb +++ b/activerecord/lib/active_record/associations/alias_tracker.rb @@ -16,7 +16,7 @@ module ActiveRecord def aliased_table_for(table_name, aliased_name = nil) table_alias = aliased_name_for(table_name, aliased_name) - if table_alias == table_name # TODO: Is this conditional necessary? + if table_alias == table_name Arel::Table.new(table_name) else Arel::Table.new(table_name).alias(table_alias) 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 diff --git a/activerecord/lib/active_record/relation/query_methods.rb b/activerecord/lib/active_record/relation/query_methods.rb index 0c7a9ec56d..9470e7c6c5 100644 --- a/activerecord/lib/active_record/relation/query_methods.rb +++ b/activerecord/lib/active_record/relation/query_methods.rb @@ -260,7 +260,6 @@ module ActiveRecord join_list ) - # TODO: Necessary? join_nodes.each do |join| join_dependency.alias_tracker.aliased_name_for(join.left.name.downcase) end -- cgit v1.2.3 From 37d93ea16046add35fecd8c279e868869ee744a5 Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Sat, 12 Mar 2011 09:32:20 +0000 Subject: Fix tests under postgres - we should always put conditions in the WHERE part not in ON constraints because postgres requires that the table has been joined before the condition references it. --- .../active_record/associations/association_scope.rb | 13 ++++++++----- .../associations/join_dependency/join_association.rb | 19 +++++++++++-------- .../lib/active_record/associations/join_helper.rb | 8 +++----- 3 files changed, 22 insertions(+), 18 deletions(-) (limited to 'activerecord/lib/active_record') diff --git a/activerecord/lib/active_record/associations/association_scope.rb b/activerecord/lib/active_record/associations/association_scope.rb index b9bbeed4d5..ab102b2b8f 100644 --- a/activerecord/lib/active_record/associations/association_scope.rb +++ b/activerecord/lib/active_record/associations/association_scope.rb @@ -86,11 +86,14 @@ module ActiveRecord scope = scope.where(interpolate(condition)) end else - scope = scope.joins(join( - foreign_table, - table[key].eq(foreign_table[foreign_key]), - *conditions[i] - )) + constraint = table[key].eq(foreign_table[foreign_key]) + join = join(foreign_table, constraint) + + scope = scope.joins(join) + + unless conditions[i].empty? + scope = scope.where(sanitize(conditions[i], table)) + end end end diff --git a/activerecord/lib/active_record/associations/join_dependency/join_association.rb b/activerecord/lib/active_record/associations/join_dependency/join_association.rb index 7dc6beeede..4121a5b378 100644 --- a/activerecord/lib/active_record/associations/join_dependency/join_association.rb +++ b/activerecord/lib/active_record/associations/join_dependency/join_association.rb @@ -74,7 +74,7 @@ module ActiveRecord foreign_key = reflection.foreign_key when :has_and_belongs_to_many # Join the join table first... - relation = relation.from(join( + relation.from(join( table, table[reflection.foreign_key]. eq(foreign_table[reflection.active_record_primary_key]) @@ -89,17 +89,20 @@ module ActiveRecord foreign_key = reflection.active_record_primary_key end - conditions = self.conditions[i] + constraint = table[key].eq(foreign_table[foreign_key]) if reflection.klass.finder_needs_type_condition? - conditions += [reflection.klass.send(:type_condition, table)] + constraint = table.create_and([ + constraint, + reflection.klass.send(:type_condition, table) + ]) end - relation = relation.from(join( - table, - table[key].eq(foreign_table[foreign_key]), - *conditions - )) + relation.from(join(table, constraint)) + + unless conditions[i].empty? + relation.where(sanitize(conditions[i], table)) + end # The current table in this iteration becomes the foreign table in the next foreign_table = table diff --git a/activerecord/lib/active_record/associations/join_helper.rb b/activerecord/lib/active_record/associations/join_helper.rb index 6474f39503..eae546e76e 100644 --- a/activerecord/lib/active_record/associations/join_helper.rb +++ b/activerecord/lib/active_record/associations/join_helper.rb @@ -38,13 +38,11 @@ module ActiveRecord name end - def join(table, *conditions) - table.create_join(table, table.create_on(sanitize(conditions)), join_type) + def join(table, constraint) + table.create_join(table, table.create_on(constraint), join_type) end - def sanitize(conditions) - table = conditions.first.left.relation - + def sanitize(conditions, table) conditions = conditions.map do |condition| condition = active_record.send(:sanitize_sql, interpolate(condition), table.table_alias || table.name) condition = Arel.sql(condition) unless condition.is_a?(Arel::Node) -- cgit v1.2.3 From 64fe0d4cba1cbe3bb6c78cad129aa9be43fdcc29 Mon Sep 17 00:00:00 2001 From: Manuel Meurer Date: Wed, 16 Mar 2011 11:01:43 +0700 Subject: Remove incorrect comment that a default value of NULL cannot be set with change_column_default. --- .../active_record/connection_adapters/abstract/schema_statements.rb | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) (limited to 'activerecord/lib/active_record') diff --git a/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb b/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb index 3ec7dd02a4..8bae50885f 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb @@ -279,12 +279,11 @@ module ActiveRecord raise NotImplementedError, "change_column is not implemented" end - # Sets a new default value for a column. If you want to set the default - # value to +NULL+, you are out of luck. You need to - # DatabaseStatements#execute the appropriate SQL statement yourself. + # Sets a new default value for a column. # ===== Examples # change_column_default(:suppliers, :qualification, 'new') # change_column_default(:accounts, :authorized, 1) + # change_column_default(:users, :email, nil) def change_column_default(table_name, column_name, default) raise NotImplementedError, "change_column_default is not implemented" end -- cgit v1.2.3 From f3666040a0a883ab75dd53012cc4ace66f90606b Mon Sep 17 00:00:00 2001 From: Paco Guzman Date: Fri, 18 Mar 2011 11:29:52 +0100 Subject: remove bank line --- .../active_record/connection_adapters/abstract/database_statements.rb | 1 - 1 file changed, 1 deletion(-) (limited to 'activerecord/lib/active_record') diff --git a/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb b/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb index 5c1ce173c8..d54a643d7b 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb @@ -237,7 +237,6 @@ module ActiveRecord # add_limit_offset!('SELECT * FROM suppliers', {:limit => 10, :offset => 50}) # generates # SELECT * FROM suppliers LIMIT 10 OFFSET 50 - def add_limit_offset!(sql, options) if limit = options[:limit] sql << " LIMIT #{sanitize_limit(limit)}" -- cgit v1.2.3 From 3378d77b044306d6e92f188932d435483af569e3 Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Mon, 21 Mar 2011 16:32:13 -0700 Subject: use prepared statements to fetch the last insert id --- .../lib/active_record/connection_adapters/postgresql_adapter.rb | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) (limited to 'activerecord/lib/active_record') diff --git a/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb b/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb index 576450bc3a..5a830a50fb 100644 --- a/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb @@ -453,7 +453,7 @@ module ActiveRecord # If a pk is given, fallback to default sequence name. # Don't fetch last insert id for a table without a pk. if pk && sequence_name ||= default_sequence_name(table, pk) - last_insert_id(table, sequence_name) + last_insert_id(sequence_name) end end end @@ -1038,8 +1038,9 @@ module ActiveRecord end # Returns the current ID of a table's sequence. - def last_insert_id(table, sequence_name) #:nodoc: - Integer(select_value("SELECT currval('#{sequence_name}')")) + def last_insert_id(sequence_name) #:nodoc: + r = exec_query("SELECT currval($1)", 'SQL', [[nil, sequence_name]]) + Integer(r.rows.first.first) end # Executes a SELECT query and returns the results, performing any data type -- cgit v1.2.3 From 15d3cc21f41af4b3caaae4f3586481effa77058f Mon Sep 17 00:00:00 2001 From: Aaron Patterson Date: Tue, 22 Mar 2011 09:18:01 -0700 Subject: pushing id insertion and prefetch primary keys down to Relation#insert --- .../connection_adapters/abstract/database_statements.rb | 4 ++++ .../active_record/connection_adapters/sqlite_adapter.rb | 4 ++++ activerecord/lib/active_record/persistence.rb | 10 +--------- activerecord/lib/active_record/relation.rb | 17 ++++++++++++++--- 4 files changed, 23 insertions(+), 12 deletions(-) (limited to 'activerecord/lib/active_record') diff --git a/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb b/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb index 5c1ce173c8..756c221fad 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb @@ -272,6 +272,10 @@ module ActiveRecord execute "INSERT INTO #{quote_table_name(table_name)} (#{key_list.join(', ')}) VALUES (#{value_list.join(', ')})", 'Fixture Insert' end + def null_insert_value + Arel.sql 'DEFAULT' + end + def empty_insert_statement_value "VALUES(DEFAULT)" end diff --git a/activerecord/lib/active_record/connection_adapters/sqlite_adapter.rb b/activerecord/lib/active_record/connection_adapters/sqlite_adapter.rb index 9ee6b88ab6..ae61d6ce94 100644 --- a/activerecord/lib/active_record/connection_adapters/sqlite_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/sqlite_adapter.rb @@ -336,6 +336,10 @@ module ActiveRecord alter_table(table_name, :rename => {column_name.to_s => new_column_name.to_s}) end + def null_insert_value + Arel.sql 'NULL' + end + def empty_insert_statement_value "VALUES(NULL)" end diff --git a/activerecord/lib/active_record/persistence.rb b/activerecord/lib/active_record/persistence.rb index df7b22080c..17a64b6e86 100644 --- a/activerecord/lib/active_record/persistence.rb +++ b/activerecord/lib/active_record/persistence.rb @@ -270,17 +270,9 @@ module ActiveRecord # Creates a record with values matching those of the instance attributes # and returns its id. def create - if id.nil? && connection.prefetch_primary_key?(self.class.table_name) - self.id = connection.next_sequence_value(self.class.sequence_name) - end - attributes_values = arel_attributes_values(!id.nil?) - new_id = if attributes_values.empty? - self.class.unscoped.insert connection.empty_insert_statement_value - else - self.class.unscoped.insert attributes_values - end + new_id = self.class.unscoped.insert attributes_values self.id ||= new_id diff --git a/activerecord/lib/active_record/relation.rb b/activerecord/lib/active_record/relation.rb index 371403f510..8e545f9cad 100644 --- a/activerecord/lib/active_record/relation.rb +++ b/activerecord/lib/active_record/relation.rb @@ -30,15 +30,26 @@ module ActiveRecord end def insert(values) - im = arel.compile_insert values - im.into @table - primary_key_value = nil if primary_key && Hash === values primary_key_value = values[values.keys.find { |k| k.name == primary_key }] + + if !primary_key_value && connection.prefetch_primary_key?(klass.table_name) + primary_key_value = connection.next_sequence_value(klass.sequence_name) + values[klass.arel_table[klass.primary_key]] = primary_key_value + end + end + + im = arel.create_insert + im.into @table + + if values.empty? # empty insert + im.values = im.create_values [connection.null_insert_value], [] + else + im.insert values end @klass.connection.insert( -- cgit v1.2.3 From da6c7bd4b411105e7556ff5015e3c9f6ab1d26fe Mon Sep 17 00:00:00 2001 From: Santiago Pastorino Date: Fri, 18 Mar 2011 13:35:57 -0300 Subject: Do not in place modify what table_name returns --- activerecord/lib/active_record/base.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'activerecord/lib/active_record') diff --git a/activerecord/lib/active_record/base.rb b/activerecord/lib/active_record/base.rb index 7f506faeee..b778b0c0f0 100644 --- a/activerecord/lib/active_record/base.rb +++ b/activerecord/lib/active_record/base.rb @@ -995,7 +995,7 @@ module ActiveRecord #:nodoc: if parent < ActiveRecord::Base && !parent.abstract_class? contained = parent.table_name contained = contained.singularize if parent.pluralize_table_names - contained << '_' + contained += '_' end "#{full_table_name_prefix}#{contained}#{undecorated_table_name(name)}#{table_name_suffix}" else -- cgit v1.2.3 From baa237c974fee8023dd704a4efb418ff0e963de0 Mon Sep 17 00:00:00 2001 From: Santiago Pastorino Date: Mon, 21 Mar 2011 21:36:05 -0300 Subject: Allow to read and write AR attributes with non valid identifiers --- activerecord/lib/active_record/attribute_methods/read.rb | 5 ++++- activerecord/lib/active_record/attribute_methods/write.rb | 4 +++- 2 files changed, 7 insertions(+), 2 deletions(-) (limited to 'activerecord/lib/active_record') diff --git a/activerecord/lib/active_record/attribute_methods/read.rb b/activerecord/lib/active_record/attribute_methods/read.rb index ab86d8bad1..affe2d7f53 100644 --- a/activerecord/lib/active_record/attribute_methods/read.rb +++ b/activerecord/lib/active_record/attribute_methods/read.rb @@ -70,7 +70,10 @@ module ActiveRecord if cache_attribute?(attr_name) access_code = "@attributes_cache['#{attr_name}'] ||= (#{access_code})" end - generated_attribute_methods.module_eval("def _#{symbol}; #{access_code}; end; alias #{symbol} _#{symbol}", __FILE__, __LINE__) + generated_attribute_methods.module_eval do + define_method("_#{symbol}") { eval(access_code) } + alias_method(symbol, "_#{symbol}") + end end end diff --git a/activerecord/lib/active_record/attribute_methods/write.rb b/activerecord/lib/active_record/attribute_methods/write.rb index 6a593a7e0e..832f2ed408 100644 --- a/activerecord/lib/active_record/attribute_methods/write.rb +++ b/activerecord/lib/active_record/attribute_methods/write.rb @@ -10,7 +10,9 @@ module ActiveRecord module ClassMethods protected def define_method_attribute=(attr_name) - generated_attribute_methods.module_eval("def #{attr_name}=(new_value); write_attribute('#{attr_name}', new_value); end", __FILE__, __LINE__) + generated_attribute_methods.send(:define_method, "#{attr_name}=") do |new_value| + write_attribute(attr_name, new_value) + end end end -- cgit v1.2.3 From 450f7cf01b855b536416fc048a92c4309da2492e Mon Sep 17 00:00:00 2001 From: Santiago Pastorino Date: Tue, 22 Mar 2011 20:11:36 -0300 Subject: use class_eval with a string when it's possible --- activerecord/lib/active_record/attribute_methods/read.rb | 10 +++++++--- activerecord/lib/active_record/attribute_methods/write.rb | 8 ++++++-- 2 files changed, 13 insertions(+), 5 deletions(-) (limited to 'activerecord/lib/active_record') diff --git a/activerecord/lib/active_record/attribute_methods/read.rb b/activerecord/lib/active_record/attribute_methods/read.rb index affe2d7f53..69d5cd83f1 100644 --- a/activerecord/lib/active_record/attribute_methods/read.rb +++ b/activerecord/lib/active_record/attribute_methods/read.rb @@ -70,9 +70,13 @@ module ActiveRecord if cache_attribute?(attr_name) access_code = "@attributes_cache['#{attr_name}'] ||= (#{access_code})" end - generated_attribute_methods.module_eval do - define_method("_#{symbol}") { eval(access_code) } - alias_method(symbol, "_#{symbol}") + if symbol =~ /^[a-zA-Z_]\w*[!?=]?$/ + generated_attribute_methods.module_eval("def _#{symbol}; #{access_code}; end; alias #{symbol} _#{symbol}", __FILE__, __LINE__) + else + generated_attribute_methods.module_eval do + define_method("_#{symbol}") { eval(access_code) } + alias_method(symbol, "_#{symbol}") + end end end end diff --git a/activerecord/lib/active_record/attribute_methods/write.rb b/activerecord/lib/active_record/attribute_methods/write.rb index 832f2ed408..3c4dab304e 100644 --- a/activerecord/lib/active_record/attribute_methods/write.rb +++ b/activerecord/lib/active_record/attribute_methods/write.rb @@ -10,8 +10,12 @@ module ActiveRecord module ClassMethods protected def define_method_attribute=(attr_name) - generated_attribute_methods.send(:define_method, "#{attr_name}=") do |new_value| - write_attribute(attr_name, new_value) + if attr_name =~ /^[a-zA-Z_]\w*[!?=]?$/ + generated_attribute_methods.module_eval("def #{attr_name}=(new_value); write_attribute('#{attr_name}', new_value); end", __FILE__, __LINE__) + else + generated_attribute_methods.send(:define_method, "#{attr_name}=") do |new_value| + write_attribute(attr_name, new_value) + end end end end -- cgit v1.2.3 From 54c963c89b81cfc4fd7dcad6779e41c85d1180ce Mon Sep 17 00:00:00 2001 From: Murray Steele Date: Fri, 11 Mar 2011 12:02:49 +0000 Subject: Make clearing of HABTM join table contents happen in an after_destory callback. The old method of redefining destroy meant that clearing the HABTM join table would happen as long as the call to destroy succeeded. Which meant if there was a before_destroy that stopped the instance being destroyed using normal means (returning false, raising ActiveRecord::Rollback) rather than exceptional means the join table would be cleared even though the instance wasn't destroyed. Doing it in an after_destroy hook avoids this and has the advantage of happening inside the DB transaction too. --- .../builder/has_and_belongs_to_many.rb | 24 +++++++++++----------- 1 file changed, 12 insertions(+), 12 deletions(-) (limited to 'activerecord/lib/active_record') diff --git a/activerecord/lib/active_record/associations/builder/has_and_belongs_to_many.rb b/activerecord/lib/active_record/associations/builder/has_and_belongs_to_many.rb index e40b32826a..4b48757da7 100644 --- a/activerecord/lib/active_record/associations/builder/has_and_belongs_to_many.rb +++ b/activerecord/lib/active_record/associations/builder/has_and_belongs_to_many.rb @@ -7,24 +7,24 @@ module ActiveRecord::Associations::Builder def build reflection = super check_validity(reflection) - redefine_destroy + define_after_destroy_method reflection end private - def redefine_destroy - # Don't use a before_destroy callback since users' before_destroy - # callbacks will be executed after the association is wiped out. + def define_after_destroy_method name = self.name - model.send(:include, Module.new { - class_eval <<-RUBY, __FILE__, __LINE__ + 1 - def destroy # def destroy - super # super - #{name}.clear # posts.clear - end # end - RUBY - }) + model.send(:class_eval, <<-eoruby, __FILE__, __LINE__ + 1) + def #{after_destroy_method_name} + association(#{name.to_sym.inspect}).delete_all + end + eoruby + model.after_destroy after_destroy_method_name + end + + def after_destroy_method_name + "has_and_belongs_to_many_after_destroy_for_#{name}" end # TODO: These checks should probably be moved into the Reflection, and we should not be -- cgit v1.2.3 From c5908a86492271ca55a6f54ccfd62b521cdc47c9 Mon Sep 17 00:00:00 2001 From: Adam Meehan Date: Tue, 1 Mar 2011 22:18:46 +1100 Subject: Fix before_type_cast for timezone aware attributes by caching converted value on write. Also remove read method reload arg on timezone attributes. --- .../lib/active_record/attribute_methods/time_zone_conversion.rb | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) (limited to 'activerecord/lib/active_record') diff --git a/activerecord/lib/active_record/attribute_methods/time_zone_conversion.rb b/activerecord/lib/active_record/attribute_methods/time_zone_conversion.rb index 76218d2a73..6aac96df6f 100644 --- a/activerecord/lib/active_record/attribute_methods/time_zone_conversion.rb +++ b/activerecord/lib/active_record/attribute_methods/time_zone_conversion.rb @@ -21,9 +21,9 @@ module ActiveRecord def define_method_attribute(attr_name) if create_time_zone_conversion_attribute?(attr_name, columns_hash[attr_name]) method_body, line = <<-EOV, __LINE__ + 1 - def _#{attr_name}(reload = false) + def _#{attr_name} cached = @attributes_cache['#{attr_name}'] - return cached if cached && !reload + return cached if cached time = _read_attribute('#{attr_name}') @attributes_cache['#{attr_name}'] = time.acts_like?(:time) ? time.in_time_zone : time end @@ -41,12 +41,13 @@ module ActiveRecord if create_time_zone_conversion_attribute?(attr_name, columns_hash[attr_name]) method_body, line = <<-EOV, __LINE__ + 1 def #{attr_name}=(original_time) - time = original_time.dup unless original_time.nil? + time = original_time unless time.acts_like?(:time) time = time.is_a?(String) ? Time.zone.parse(time) : time.to_time rescue time end time = time.in_time_zone rescue nil if time - write_attribute(:#{attr_name}, (time || original_time)) + write_attribute(:#{attr_name}, original_time) + @attributes_cache["#{attr_name}"] = time end EOV generated_attribute_methods.module_eval(method_body, __FILE__, line) -- cgit v1.2.3