From 03852b6bcf5d26bd22f7422a8fdd4763c0c11e07 Mon Sep 17 00:00:00 2001 From: Patrick Bacon Date: Mon, 28 Mar 2011 06:59:09 -0400 Subject: [PATCH] Added back the use of the Reflection module's cached sanitized_conditions in an AssociationProxy. This was recently removed and when a has_one association with conditions is eager loaded the conditions would be sanitized once for every result row, causing a database hit to fetch the columns. --- .../associations/association_proxy.rb | 6 +++++- activerecord/lib/active_record/base.rb | 7 +++++-- .../test/cases/associations/eager_test.rb | 17 +++++++++++++++++ activerecord/test/cases/helper.rb | 5 ++++- activerecord/test/models/post.rb | 1 + 5 files changed, 32 insertions(+), 4 deletions(-) diff --git a/activerecord/lib/active_record/associations/association_proxy.rb b/activerecord/lib/active_record/associations/association_proxy.rb index 943006042d748..5226edfd92491 100644 --- a/activerecord/lib/active_record/associations/association_proxy.rb +++ b/activerecord/lib/active_record/associations/association_proxy.rb @@ -102,7 +102,7 @@ def aliased_table_name # Returns the SQL string that corresponds to the :conditions # option of the macro, if given, or +nil+ otherwise. def conditions - @conditions ||= @reflection.options[:conditions] && interpolate_and_sanitize_sql(@reflection.options[:conditions]) + @conditions ||= interpolate_sanitized_sql(@reflection.sanitized_conditions) if @reflection.sanitized_conditions end alias :sql_conditions :conditions @@ -161,6 +161,10 @@ def dependent? @reflection.options[:dependent] end + def interpolate_sanitized_sql(sql, record = nil, sanitize_klass = @reflection.klass) + @owner.send(:interpolate_sanitized_sql, sql, record, sanitize_klass) + end + def interpolate_and_sanitize_sql(sql, record = nil, sanitize_klass = @reflection.klass) @owner.send(:interpolate_and_sanitize_sql, sql, record, sanitize_klass) end diff --git a/activerecord/lib/active_record/base.rb b/activerecord/lib/active_record/base.rb index a2a6e03a44ce2..a18b681a610e7 100644 --- a/activerecord/lib/active_record/base.rb +++ b/activerecord/lib/active_record/base.rb @@ -1733,7 +1733,10 @@ def quote_value(value, column = nil) def interpolate_and_sanitize_sql(sql, record = nil, sanitize_klass = self.class) sanitized = sanitize_klass.send(:sanitize_sql, sql) + interpolate_sanitized_sql(sanitized, record, sanitize_klass) + end + def interpolate_sanitized_sql(sanitized, record = nil, sanitize_klass = self.class) if sanitized =~ /\#\{.*\}/ ActiveSupport::Deprecation.warn( 'String-based interpolation of association conditions is deprecated. Please use a ' \ @@ -1741,8 +1744,8 @@ def interpolate_and_sanitize_sql(sql, record = nil, sanitize_klass = self.class) 'should be changed to has_many :older_friends, :conditions => proc { "age > #{age}" }.' ) instance_eval("%@#{sanitized.gsub('@', '\@')}@", __FILE__, __LINE__) - elsif sql.respond_to?(:to_proc) - sanitize_klass.send(:sanitize_sql, instance_exec(record, &sql)) + elsif sanitized.respond_to?(:to_proc) + sanitize_klass.send(:sanitize_sql, instance_exec(record, &sanitized)) else sanitized end diff --git a/activerecord/test/cases/associations/eager_test.rb b/activerecord/test/cases/associations/eager_test.rb index bcf7b298dd1ca..6ebea60d0794e 100644 --- a/activerecord/test/cases/associations/eager_test.rb +++ b/activerecord/test/cases/associations/eager_test.rb @@ -670,6 +670,23 @@ def test_preload_with_deprecated_interpolation assert_equal [comments(:greetings)], post.comments_with_interpolated_conditions end + def test_preload_has_one_with_conditions + # pre-heat our cache + Post.arel_table.columns + Comment.columns + + Post.connection.column_calls_by_table['comments'] = 0 + Post.includes(:very_special_comment).all.to_a + assert_equal 0, Post.connection.column_calls_by_table['comments'] + + Post.connection.column_calls_by_table['comments'] = 0 + Post.includes(:first_post_comment).all.to_a + + # Don't care exactly how many column lookup are done, + # as long as the number is small + assert(Post.connection.column_calls_by_table['comments'] < 3) + end + def test_polymorphic_type_condition post = Post.find(posts(:thinking).id, :include => :taggings) assert post.taggings.include?(taggings(:thinking_general)) diff --git a/activerecord/test/cases/helper.rb b/activerecord/test/cases/helper.rb index 226600a48acd7..1b12f81feca80 100644 --- a/activerecord/test/cases/helper.rb +++ b/activerecord/test/cases/helper.rb @@ -39,11 +39,14 @@ def execute_with_query_record(sql, name = nil, &block) end ActiveRecord::Base.connection.class.class_eval { - attr_accessor :column_calls + attr_accessor :column_calls, :column_calls_by_table def columns_with_calls(*args) @column_calls ||= 0 + @column_calls_by_table ||= Hash.new {|h,table| h[table] = 0} + @column_calls += 1 + @column_calls_by_table[args.first.to_s] += 1 columns_without_calls(*args) end diff --git a/activerecord/test/models/post.rb b/activerecord/test/models/post.rb index 8fe716739d110..e6359c19e53ab 100644 --- a/activerecord/test/models/post.rb +++ b/activerecord/test/models/post.rb @@ -44,6 +44,7 @@ def find_most_recent :conditions => proc { ["#{"#{aliased_table_name}." rescue ""}body = ?", 'Thank you for the welcome'] } has_many :comments_with_deprecated_interpolated_conditions, :class_name => 'Comment', :conditions => ['#{"#{aliased_table_name}." rescue ""}body = ?', 'Thank you for the welcome'] + has_one :first_post_comment, :class_name => 'Comment', :conditions => {:body => 'First Post!'} has_one :very_special_comment has_one :very_special_comment_with_post, :class_name => "VerySpecialComment", :include => :post