diff options
| -rw-r--r-- | activerecord/lib/active_record/associations.rb | 174 | ||||
| -rw-r--r-- | activerecord/lib/active_record/associations/through_association_scope.rb | 4 | ||||
| -rw-r--r-- | activerecord/test/cases/associations/nested_has_many_through_associations_test.rb | 59 | ||||
| -rw-r--r-- | activerecord/test/fixtures/ratings.yml | 14 | ||||
| -rw-r--r-- | activerecord/test/models/comment.rb | 1 | ||||
| -rw-r--r-- | activerecord/test/models/job.rb | 2 | ||||
| -rw-r--r-- | activerecord/test/models/person.rb | 3 | ||||
| -rw-r--r-- | activerecord/test/models/post.rb | 2 | ||||
| -rw-r--r-- | activerecord/test/models/rating.rb | 3 | ||||
| -rw-r--r-- | activerecord/test/models/reference.rb | 2 | ||||
| -rw-r--r-- | activerecord/test/schema/schema.rb | 5 |
11 files changed, 175 insertions, 94 deletions
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 = {} diff --git a/activerecord/test/cases/associations/nested_has_many_through_associations_test.rb b/activerecord/test/cases/associations/nested_has_many_through_associations_test.rb index a5d3f27702..ba75b70941 100644 --- a/activerecord/test/cases/associations/nested_has_many_through_associations_test.rb +++ b/activerecord/test/cases/associations/nested_has_many_through_associations_test.rb @@ -17,18 +17,37 @@ require 'models/developer' require 'models/subscriber' require 'models/book' require 'models/subscription' +require 'models/rating' + +# NOTE: Some of these tests might not really test "nested" HMT associations, as opposed to ones which +# are just one level deep. But it's all the same thing really, as the "nested" code is being +# written in a generic way which applies to "non-nested" HMT associations too. So let's just shove +# all useful tests in here for now and then work out where they ought to live properly later. class NestedHasManyThroughAssociationsTest < ActiveRecord::TestCase - fixtures :authors, :books, :posts, :subscriptions, :subscribers, :tags, :taggings + fixtures :authors, :books, :posts, :subscriptions, :subscribers, :tags, :taggings, + :people, :readers, :references, :jobs, :ratings, :comments def test_has_many_through_a_has_many_through_association_on_source_reflection author = authors(:david) assert_equal [tags(:general), tags(:general)], author.tags + + # Only David has a Post tagged with General + authors = Author.joins(:tags).where('tags.id' => tags(:general).id) + assert_equal [authors(:david)], authors.uniq + + # This ensures that the polymorphism of taggings is being observed correctly + authors = Author.joins(:tags).where('taggings.taggable_type' => 'FakeModel') + assert authors.empty? end def test_has_many_through_a_has_many_through_association_on_through_reflection author = authors(:david) assert_equal [subscribers(:first), subscribers(:second), subscribers(:second)], author.subscribers + + # All authors with subscribers where one of the subscribers' nick is 'alterself' + authors = Author.joins(:subscribers).where('subscribers.nick' => 'alterself') + assert_equal [authors(:david)], authors end def test_distinct_has_many_through_a_has_many_through_association_on_source_reflection @@ -44,11 +63,41 @@ class NestedHasManyThroughAssociationsTest < ActiveRecord::TestCase def test_nested_has_many_through_with_a_table_referenced_multiple_times author = authors(:bob) assert_equal [posts(:misc_by_bob), posts(:misc_by_mary)], author.similar_posts.sort_by(&:id) + + # Mary and Bob both have posts in misc, but they are the only ones. + authors = Author.joins(:similar_posts).where('posts.id' => posts(:misc_by_bob).id) + assert_equal [authors(:mary), authors(:bob)], authors.uniq.sort_by(&:id) + + # Check the polymorphism of taggings is being observed correctly (in both joins) + authors = Author.joins(:similar_posts).where('taggings.taggable_type' => 'FakeModel') + assert authors.empty? + authors = Author.joins(:similar_posts).where('taggings_authors_join.taggable_type' => 'FakeModel') + assert authors.empty? end - def test_nested_has_many_through_as_a_join - # All authors with subscribers where one of the subscribers' nick is 'alterself' - authors = Author.joins(:subscribers).where('subscribers.nick' => 'alterself') - assert_equal [authors(:david)], authors + def test_has_many_through_with_foreign_key_option_on_through_reflection + assert_equal [posts(:welcome), posts(:authorless)], people(:david).agents_posts + assert_equal [authors(:david)], references(:david_unicyclist).agents_posts_authors + + references = Reference.joins(:agents_posts_authors).where('authors.id' => authors(:david).id) + assert_equal [references(:david_unicyclist)], references + end + + def test_has_many_through_with_foreign_key_option_on_source_reflection + assert_equal [people(:michael), people(:susan)], jobs(:unicyclist).agents + + jobs = Job.joins(:agents) + assert_equal [jobs(:unicyclist), jobs(:unicyclist)], jobs + end + + def test_has_many_through_with_sti_on_through_reflection + ratings = posts(:sti_comments).special_comments_ratings.sort_by(&:id) + assert_equal [ratings(:special_comment_rating), ratings(:sub_special_comment_rating)], ratings + + # Ensure STI is respected in the join + scope = Post.joins(:special_comments_ratings).where(:id => posts(:sti_comments).id) + assert scope.where("comments.type" => "Comment").empty? + assert !scope.where("comments.type" => "SpecialComment").empty? + assert !scope.where("comments.type" => "SubSpecialComment").empty? end end diff --git a/activerecord/test/fixtures/ratings.yml b/activerecord/test/fixtures/ratings.yml new file mode 100644 index 0000000000..34e208efa3 --- /dev/null +++ b/activerecord/test/fixtures/ratings.yml @@ -0,0 +1,14 @@ +normal_comment_rating: + id: 1 + comment_id: 8 + value: 1 + +special_comment_rating: + id: 2 + comment_id: 6 + value: 1 + +sub_special_comment_rating: + id: 3 + comment_id: 12 + value: 1 diff --git a/activerecord/test/models/comment.rb b/activerecord/test/models/comment.rb index 88061b2145..1a3fb42b66 100644 --- a/activerecord/test/models/comment.rb +++ b/activerecord/test/models/comment.rb @@ -7,6 +7,7 @@ class Comment < ActiveRecord::Base :conditions => { "posts.author_id" => 1 } belongs_to :post, :counter_cache => true + has_many :ratings def self.what_are_you 'a comment...' diff --git a/activerecord/test/models/job.rb b/activerecord/test/models/job.rb index 3333a02e27..46b1d87aa1 100644 --- a/activerecord/test/models/job.rb +++ b/activerecord/test/models/job.rb @@ -2,4 +2,6 @@ class Job < ActiveRecord::Base has_many :references has_many :people, :through => :references belongs_to :ideal_reference, :class_name => 'Reference' + + has_many :agents, :through => :people end diff --git a/activerecord/test/models/person.rb b/activerecord/test/models/person.rb index 951ec93c53..d35c51b660 100644 --- a/activerecord/test/models/person.rb +++ b/activerecord/test/models/person.rb @@ -13,6 +13,9 @@ class Person < ActiveRecord::Base belongs_to :primary_contact, :class_name => 'Person' has_many :agents, :class_name => 'Person', :foreign_key => 'primary_contact_id' belongs_to :number1_fan, :class_name => 'Person' + + has_many :agents_posts, :through => :agents, :source => :posts + has_many :agents_posts_authors, :through => :agents_posts, :source => :author scope :males, :conditions => { :gender => 'M' } scope :females, :conditions => { :gender => 'F' } diff --git a/activerecord/test/models/post.rb b/activerecord/test/models/post.rb index a3cb9c724a..f3b78c3647 100644 --- a/activerecord/test/models/post.rb +++ b/activerecord/test/models/post.rb @@ -46,6 +46,8 @@ class Post < ActiveRecord::Base has_one :very_special_comment_with_post, :class_name => "VerySpecialComment", :include => :post has_many :special_comments has_many :nonexistant_comments, :class_name => 'Comment', :conditions => 'comments.id < 0' + + has_many :special_comments_ratings, :through => :special_comments, :source => :ratings has_and_belongs_to_many :categories has_and_belongs_to_many :special_categories, :join_table => "categories_posts", :association_foreign_key => 'category_id' diff --git a/activerecord/test/models/rating.rb b/activerecord/test/models/rating.rb new file mode 100644 index 0000000000..12c4b5affa --- /dev/null +++ b/activerecord/test/models/rating.rb @@ -0,0 +1,3 @@ +class Rating < ActiveRecord::Base + belongs_to :comment +end diff --git a/activerecord/test/models/reference.rb b/activerecord/test/models/reference.rb index 4a17c936f5..2feb15d706 100644 --- a/activerecord/test/models/reference.rb +++ b/activerecord/test/models/reference.rb @@ -1,6 +1,8 @@ class Reference < ActiveRecord::Base belongs_to :person belongs_to :job + + has_many :agents_posts_authors, :through => :person end class BadReference < ActiveRecord::Base diff --git a/activerecord/test/schema/schema.rb b/activerecord/test/schema/schema.rb index dbd5da45eb..2fa9a4521e 100644 --- a/activerecord/test/schema/schema.rb +++ b/activerecord/test/schema/schema.rb @@ -449,6 +449,11 @@ ActiveRecord::Schema.define do t.string :type end + create_table :ratings, :force => true do |t| + t.integer :comment_id + t.integer :value + end + create_table :readers, :force => true do |t| t.integer :post_id, :null => false t.integer :person_id, :null => false |