From 4f00c70580e376691bd1d6c1f9d09efbaa7bf9c9 Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Sun, 5 Mar 2006 18:43:56 +0000 Subject: [PATCH] Fixed eager loading problems with single-table inheritance [Rick Olson] Added smarter table aliasing for eager associations for multiple self joins [Rick Olson] (closes #3580) git-svn-id: http://svn-commit.rubyonrails.org/rails/trunk@3776 5ecf4fe2-1ee6-0310-87b1-e25e094e27de --- activerecord/CHANGELOG | 11 ++ .../lib/active_record/associations.rb | 101 ++++++++++-------- activerecord/test/abstract_unit.rb | 22 ++++ ...ssociations_cascaded_eager_loading_test.rb | 26 +++-- .../test/associations_go_eager_test.rb | 8 +- .../test/associations_join_model_test.rb | 29 ++++- activerecord/test/base_test.rb | 4 +- activerecord/test/finder_test.rb | 3 +- activerecord/test/fixtures/topic.rb | 2 +- activerecord/test/fixtures/topics.yml | 1 + 10 files changed, 144 insertions(+), 63 deletions(-) diff --git a/activerecord/CHANGELOG b/activerecord/CHANGELOG index 95544950e9..9114f70ea1 100644 --- a/activerecord/CHANGELOG +++ b/activerecord/CHANGELOG @@ -1,5 +1,16 @@ *SVN* +* Added smarter table aliasing for eager associations for multiple self joins #3580 [Rick Olson] + + * The first time a table is referenced in a join, no alias is used. + * After that, the parent table name and the reflection name are used. + + Tree.find(:all, :include => :children) # LEFT OUTER JOIN trees AS tress_children ... + + * Any additional join references get a numerical suffix like '_2', '_3', etc. + +* Fixed eager loading problems with single-table inheritance #3580 [Rick Olson]. Post.find(:all, :include => :special_comments) now returns all posts, and any special comments that the posts may have. And made STI work with has_many :through and polymorphic belongs_to. + * Added cascading eager loading that allows for queries like Author.find(:all, :include=> { :posts=> :comments }), which will fetch all authors, their posts, and the comments belonging to those posts in a single query (using LEFT OUTER JOIN) #3913 [anna@wota.jp]. Examples: # cascaded in two levels diff --git a/activerecord/lib/active_record/associations.rb b/activerecord/lib/active_record/associations.rb index 5454631999..d31d43c800 100755 --- a/activerecord/lib/active_record/associations.rb +++ b/activerecord/lib/active_record/associations.rb @@ -932,7 +932,6 @@ module ActiveRecord sql << "#{options[:joins]} " if options[:joins] add_conditions!(sql, options[:conditions]) - add_sti_conditions!(sql, join_dependency) add_limited_ids_condition!(sql, options, join_dependency) if !using_limitable_reflections?(join_dependency.reflections) && options[:limit] add_limit!(sql, options) if using_limitable_reflections?(join_dependency.reflections) @@ -950,7 +949,6 @@ module ActiveRecord sql << "#{options[:joins]} " if options[:joins] add_conditions!(sql, options[:conditions]) - add_sti_conditions!(sql, join_dependency) add_limited_ids_condition!(sql, options, join_dependency) if !using_limitable_reflections?(join_dependency.reflections) && options[:limit] sql << "ORDER BY #{options[:order]} " if options[:order] @@ -1011,41 +1009,6 @@ module ActiveRecord reflections.reject { |r| [ :belongs_to, :has_one ].include?(r.macro) }.length.zero? end - def join_depended_type_condition (klass, join_dependency) - aliased_table_name = join_dependency.aliased_table_names_for(klass.table_name).last || klass.table_name - quoted_inheritance_column = connection.quote_column_name(klass.inheritance_column) - type_condition = klass.subclasses.inject(sti_condition(klass, aliased_table_name, quoted_inheritance_column)) do |condition, subclass| - condition << " OR #{sti_condition subclass, aliased_table_name, quoted_inheritance_column}" - end - - " (#{type_condition}) " - end - - def sti_condition(klass, table_name, inheritance_column) - "(#{table_name}.#{inheritance_column} = '#{klass.name.demodulize}' OR #{table_name}.#{inheritance_column} IS NULL)" - end - - #def join_depended_type_condition (klass, join_dependency) - # aliased_table_name = join_dependency.aliased_table_names_for(klass.table_name).first || klass.table_name - # quoted_inheritance_column = connection.quote_column_name(klass.inheritance_column) - # type_condition = klass.subclasses.inject("#{aliased_table_name}.#{quoted_inheritance_column} = '#{klass.name.demodulize}' ") do |condition, subclass| - # condition << "OR #{aliased_table_name}.#{quoted_inheritance_column} = '#{subclass.name.demodulize}' " - # end - # - # " (#{type_condition}) " - #end - - def add_sti_conditions!(sql, join_dependency) - reflections = join_dependency.reflections - sti_conditions = reflections.collect do |reflection| - join_depended_type_condition(reflection.klass, join_dependency) unless reflection.klass.descends_from_active_record? - end.compact - - unless sti_conditions.empty? - sql << condition_word(sql) + sti_conditions.join(" AND ") - end - end - def column_aliases(join_dependency) join_dependency.joins.collect{|join| join.column_names_with_alias.collect{|column_name, aliased_name| "#{join.aliased_table_name}.#{connection.quote_column_name column_name} AS #{aliased_name}"}}.flatten.join(", ") @@ -1078,7 +1041,7 @@ module ActiveRecord end class JoinDependency - attr_reader :joins, :reflections + attr_reader :joins, :reflections, :table_aliases def initialize(base, associations) @joins = [JoinBase.new(base)] @@ -1086,6 +1049,8 @@ module ActiveRecord @reflections = [] @base_records_hash = {} @base_records_in_order = [] + @table_aliases = Hash.new { |aliases, table| aliases[table] = 0 } + @table_aliases[base.table_name] = 1 build(associations) end @@ -1228,26 +1193,65 @@ module ActiveRecord @parent = parent @reflection = reflection @aliased_prefix = "t#{ join_dependency.joins.size }" - @aliased_table_name = join_dependency.aliased_table_names_for(table_name).empty? ? table_name : @aliased_prefix + @aliased_table_name = table_name # start with the table name + unless join_dependency.table_aliases[aliased_table_name].zero? + # if the table name has been used, then use an alias + # if the alias has been used, add a '_n' suffix to the end. + @aliased_table_name = "#{parent.table_name}_#{reflection.name}_#{join_dependency.table_aliases[aliased_table_name]}".gsub(/_1$/, '') + end + join_dependency.table_aliases[aliased_table_name] += 1 end def association_join - case reflection.macro + join = case reflection.macro when :has_and_belongs_to_many join_table_name = " LEFT OUTER JOIN %s ON %s.%s = %s.%s " % [ options[:join_table], options[:join_table], options[:foreign_key] || reflection.active_record.to_s.classify.foreign_key, reflection.active_record.table_name, reflection.active_record.primary_key] + - " LEFT OUTER JOIN %s ON %s.%s = %s.%s " % [ - aliased_table_name, aliased_table_name, klass.primary_key, + " LEFT OUTER JOIN %s AS %s ON %s.%s = %s.%s " % [ + table_name, aliased_table_name, aliased_table_name, klass.primary_key, options[:join_table], options[:association_foreign_key] || klass.table_name.classify.foreign_key ] when :has_many, :has_one - " LEFT OUTER JOIN %s AS %s ON %s.%s = %s.%s " % [table_name, aliased_table_name, - aliased_table_name, options[:foreign_key] || reflection.active_record.to_s.classify.foreign_key, - parent.aliased_table_name, parent.primary_key - ] + case + when reflection.macro == :has_many && reflection.options[:through] + through_reflection = parent.active_record.reflect_on_association(reflection.options[:through]) + aliased_through_table = "#{parent.table_name}_#{through_reflection.klass.table_name}" + if through_reflection.options[:as] # has_many :through against a polymorphic join + polymorphic_foreign_key = through_reflection.options[:as].to_s + '_id' + polymorphic_foreign_type = through_reflection.options[:as].to_s + '_type' + + " LEFT OUTER JOIN %s AS %s ON (%s.%s = %s.%s AND %s.%s = %s) " % [through_reflection.klass.table_name, aliased_through_table, + aliased_through_table, polymorphic_foreign_key, + parent.aliased_table_name, parent.primary_key, + aliased_through_table, polymorphic_foreign_type, reflection.klass.quote(parent.active_record.base_class.name)] + + " LEFT OUTER JOIN %s AS %s ON %s.%s = %s.%s " % [table_name, aliased_table_name, + aliased_table_name, primary_key, aliased_through_table, options[:foreign_key] || reflection.klass.to_s.classify.foreign_key + ] + else # has_many :through against a normal join + " LEFT OUTER JOIN %s AS %s ON %s.%s = %s.%s " % [through_reflection.klass.table_name, aliased_through_table, + aliased_through_table, through_reflection.options[:foreign_key] || through_reflection.active_record.to_s.classify.foreign_key, + parent.aliased_table_name, parent.primary_key] + + " LEFT OUTER JOIN %s AS %s ON %s.%s = %s.%s " % [table_name, aliased_table_name, + aliased_table_name, primary_key, aliased_through_table, options[:foreign_key] || reflection.klass.to_s.classify.foreign_key + ] + end + + when reflection.macro == :has_many && reflection.options[:as] + " LEFT OUTER JOIN %s AS %s ON %s.%s = %s.%s AND %s.%s = %s" % [table_name, aliased_table_name, + aliased_table_name, "#{reflection.options[:as]}_id", + parent.aliased_table_name, parent.primary_key, + aliased_table_name, "#{reflection.options[:as]}_type", + reflection.klass.quote(parent.active_record.base_class.name) + ] + else + " LEFT OUTER JOIN %s AS %s ON %s.%s = %s.%s " % [table_name, aliased_table_name, + aliased_table_name, options[:foreign_key] || reflection.active_record.to_s.classify.foreign_key, + parent.aliased_table_name, parent.primary_key + ] + end when :belongs_to " LEFT OUTER JOIN %s AS %s ON %s.%s = %s.%s " % [table_name, aliased_table_name, aliased_table_name, reflection.klass.primary_key, @@ -1256,6 +1260,11 @@ module ActiveRecord else "" end + join << %(AND %s."%s" = %s ) % [ + aliased_table_name, + reflection.active_record.inheritance_column, + reflection.klass.quote(klass.name)] unless join.blank? || reflection.klass.descends_from_active_record? + join end end end diff --git a/activerecord/test/abstract_unit.rb b/activerecord/test/abstract_unit.rb index 1bd2f6baaf..70487980c5 100755 --- a/activerecord/test/abstract_unit.rb +++ b/activerecord/test/abstract_unit.rb @@ -29,6 +29,19 @@ class Test::Unit::TestCase #:nodoc: assert_equal expected.to_s, actual.to_s, message end end + + def assert_no_queries + ActiveRecord::Base.connection.class.class_eval do + self.query_count = 0 + alias_method :execute, :execute_with_query_counting + end + yield + ensure + ActiveRecord::Base.connection.class.class_eval do + alias_method :execute, :execute_without_query_counting + end + assert_equal 0, ActiveRecord::Base.connection.query_count, "1 or more queries were executed" + end end def current_adapter?(type) @@ -36,5 +49,14 @@ def current_adapter?(type) ActiveRecord::Base.connection.instance_of?(ActiveRecord::ConnectionAdapters.const_get(type)) end +ActiveRecord::Base.connection.class.class_eval do + cattr_accessor :query_count + alias_method :execute_without_query_counting, :execute + def execute_with_query_counting(sql, name = nil) + self.query_count += 1 + execute_without_query_counting(sql, name) + end +end + #ActiveRecord::Base.logger = Logger.new(STDOUT) #ActiveRecord::Base.colorize_logging = false diff --git a/activerecord/test/associations_cascaded_eager_loading_test.rb b/activerecord/test/associations_cascaded_eager_loading_test.rb index 797bc85bc3..8f0a41f593 100644 --- a/activerecord/test/associations_cascaded_eager_loading_test.rb +++ b/activerecord/test/associations_cascaded_eager_loading_test.rb @@ -56,23 +56,33 @@ class CascadedEagerLoadingTest < Test::Unit::TestCase def test_eager_association_loading_with_acts_as_tree roots = TreeMixin.find(:all, :include=>"children", :conditions=>"mixins.parent_id IS NULL", :order=>"mixins.id") assert_equal [mixins(:tree_1), mixins(:tree2_1), mixins(:tree3_1)], roots - assert_equal 2, roots[0].children.size - assert_equal 0, roots[1].children.size - assert_equal 0, roots[2].children.size + assert_no_queries do + assert_equal 2, roots[0].children.size + assert_equal 0, roots[1].children.size + assert_equal 0, roots[2].children.size + end end def test_eager_association_loading_with_cascaded_three_levels_by_ping_pong firms = Firm.find(:all, :include=>{:account=>{:firm=>:account}}, :order=>"companies.id") assert_equal 2, firms.size assert_equal firms.first.account, firms.first.account.firm.account - assert_equal companies(:first_firm).account, firms.first.account.firm.account - assert_equal companies(:first_firm).account.firm.account, firms.first.account.firm.account + assert_equal companies(:first_firm).account, assert_no_queries { firms.first.account.firm.account } + assert_equal companies(:first_firm).account.firm.account, assert_no_queries { firms.first.account.firm.account } end - def test_eager_association_loading_with_sti + def test_eager_association_loading_with_has_many_sti topics = Topic.find(:all, :include => :replies, :order => 'topics.id') assert_equal [topics(:first), topics(:second)], topics - assert_equal 1, topics[0].replies.size - assert_equal 0, topics[1].replies.size + assert_no_queries do + assert_equal 1, topics[0].replies.size + assert_equal 0, topics[1].replies.size + end + end + + def test_eager_association_loading_with_belongs_to_sti + replies = Reply.find(:all, :include => :topic, :order => 'topics.id') + assert_equal [topics(:second)], replies + assert_equal topics(:first), assert_no_queries { replies.first.topic } end end diff --git a/activerecord/test/associations_go_eager_test.rb b/activerecord/test/associations_go_eager_test.rb index 2212596099..50cb9bec59 100644 --- a/activerecord/test/associations_go_eager_test.rb +++ b/activerecord/test/associations_go_eager_test.rb @@ -94,13 +94,13 @@ class EagerAssociationTest < Test::Unit::TestCase def test_eager_association_loading_with_belongs_to_and_limit_and_multiple_associations posts = Post.find(:all, :include => [:author, :very_special_comment], :limit => 1, :order => 'posts.id') assert_equal 1, posts.length - assert_equal [3], posts.collect { |p| p.id } + assert_equal [1], posts.collect { |p| p.id } end def test_eager_association_loading_with_belongs_to_and_limit_and_offset_and_multiple_associations posts = Post.find(:all, :include => [:author, :very_special_comment], :limit => 1, :offset => 1, :order => 'posts.id') assert_equal 1, posts.length - assert_equal [4], posts.collect { |p| p.id } + assert_equal [2], posts.collect { |p| p.id } end def test_eager_with_has_many_through @@ -108,8 +108,8 @@ class EagerAssociationTest < Test::Unit::TestCase posts_with_author = people(:michael).posts.find(:all, :include => :author ) posts_with_comments_and_author = people(:michael).posts.find(:all, :include => [ :comments, :author ]) assert_equal 2, posts_with_comments.inject(0) { |sum, post| sum += post.comments.size } - assert_equal authors(:david), posts_with_author.first.author - assert_equal authors(:david), posts_with_comments_and_author.first.author + assert_equal authors(:david), assert_no_queries { posts_with_author.first.author } + assert_equal authors(:david), assert_no_queries { posts_with_comments_and_author.first.author } end def test_eager_with_has_many_and_limit diff --git a/activerecord/test/associations_join_model_test.rb b/activerecord/test/associations_join_model_test.rb index a2f42f060e..b14693d7a6 100644 --- a/activerecord/test/associations_join_model_test.rb +++ b/activerecord/test/associations_join_model_test.rb @@ -81,7 +81,34 @@ class AssociationsJoinModelTest < Test::Unit::TestCase def test_has_many_with_piggyback assert_equal "2", categories(:sti_test).authors.first.post_id.to_s end - + + def test_include_has_many_through + posts = Post.find(:all, :order => 'posts.id') + posts_with_authors = Post.find(:all, :include => :authors, :order => 'posts.id') + assert_equal posts.length, posts_with_authors.length + posts.length.times do |i| + assert_equal posts[i].authors.length, assert_no_queries { posts_with_authors[i].authors.length } + end + end + + def test_include_polymorphic_has_many_through + posts = Post.find(:all, :order => 'posts.id') + posts_with_tags = Post.find(:all, :include => :tags, :order => 'posts.id') + assert_equal posts.length, posts_with_tags.length + posts.length.times do |i| + assert_equal posts[i].tags.length, assert_no_queries { posts_with_tags[i].tags.length } + end + end + + def test_include_polymorphic_has_many + posts = Post.find(:all, :order => 'posts.id') + posts_with_taggings = Post.find(:all, :include => :taggings, :order => 'posts.id') + assert_equal posts.length, posts_with_taggings.length + posts.length.times do |i| + assert_equal posts[i].taggings.length, assert_no_queries { posts_with_taggings[i].taggings.length } + end + end + def test_has_many_find_all assert_equal [categories(:general)], authors(:david).categories.find(:all) end diff --git a/activerecord/test/base_test.rb b/activerecord/test/base_test.rb index a84e0ed5e8..c5834f2e5b 100755 --- a/activerecord/test/base_test.rb +++ b/activerecord/test/base_test.rb @@ -567,7 +567,7 @@ class BasicsTest < Test::Unit::TestCase end def test_equality - assert_equal Topic.find(1), Topic.find(2).parent + assert_equal Topic.find(1), Topic.find(2).topic end def test_equality_of_new_records @@ -575,7 +575,7 @@ class BasicsTest < Test::Unit::TestCase end def test_hashing - assert_equal [ Topic.find(1) ], [ Topic.find(2).parent ] & [ Topic.find(1) ] + assert_equal [ Topic.find(1) ], [ Topic.find(2).topic ] & [ Topic.find(1) ] end def test_destroy_new_record diff --git a/activerecord/test/finder_test.rb b/activerecord/test/finder_test.rb index 26a4688108..bf7237b684 100644 --- a/activerecord/test/finder_test.rb +++ b/activerecord/test/finder_test.rb @@ -1,6 +1,7 @@ require 'abstract_unit' require 'fixtures/company' require 'fixtures/topic' +require 'fixtures/reply' require 'fixtures/entrant' require 'fixtures/developer' require 'fixtures/post' @@ -92,7 +93,7 @@ class FinderTest < Test::Unit::TestCase Topic.find(1).parent } - Topic.find(2).parent + Topic.find(2).topic end def test_find_only_some_columns diff --git a/activerecord/test/fixtures/topic.rb b/activerecord/test/fixtures/topic.rb index 4c46122450..1941080ec7 100755 --- a/activerecord/test/fixtures/topic.rb +++ b/activerecord/test/fixtures/topic.rb @@ -6,7 +6,7 @@ class Topic < ActiveRecord::Base before_destroy :destroy_children def parent - self.class.find(parent_id) + Topic.find(parent_id) end protected diff --git a/activerecord/test/fixtures/topics.yml b/activerecord/test/fixtures/topics.yml index 6d4f5d800b..810bbcf4e8 100644 --- a/activerecord/test/fixtures/topics.yml +++ b/activerecord/test/fixtures/topics.yml @@ -19,3 +19,4 @@ second: approved: true replies_count: 2 parent_id: 1 + type: Reply