Removes the ability for eager loaded conditions to be interpolated, since there is no model instance to use as a context for interpolation. #5553 [turnip@turnipspatch.com]

git-svn-id: http://svn-commit.rubyonrails.org/rails/trunk@5264 5ecf4fe2-1ee6-0310-87b1-e25e094e27de

9 files changed, 28 insertions, 29 deletions

diff --git a/activerecord/CHANGELOG b/activerecord/CHANGELOG
index a16e2bef02..0ccf3ccc3f 100644
--- a/activerecord/CHANGELOG
+++ b/activerecord/CHANGELOG
@@ -1,5 +1,7 @@
 *SVN*
 
+* Removes the ability for eager loaded conditions to be interpolated, since there is no model instance to use as a context for interpolation. #5553 [turnip@turnipspatch.com]
+
 * Added timeout option to SQLite3 configurations to deal more gracefully with SQLite3::BusyException, now the connection can instead retry for x seconds to see if the db clears up before throwing that exception #6126 [wreese@gmail.com]
 
 * Added update_attributes! which uses save! to raise an exception if a validation error prevents saving #6192 [jonathan]
diff --git a/activerecord/lib/active_record/associations.rb b/activerecord/lib/active_record/associations.rb
index 93f10c666c..459484f8c2 100755
--- a/activerecord/lib/active_record/associations.rb
+++ b/activerecord/lib/active_record/associations.rb
@@ -349,23 +349,17 @@ module ActiveRecord
     # But that shouldn't fool you to think that you can pull out huge amounts of data with no performance penalty just because you've reduced
     # the number of queries. The database still needs to send all the data to Active Record and it still needs to be processed. So it's no
     # catch-all for performance problems, but it's a great way to cut down on the number of queries in a situation as the one described above.
+    # 
+    # Since the eager loading pulls from multiple tables, you'll have to disambiguate any column references in both conditions and orders. So
+    # :order => "posts.id DESC" will work while :order => "id DESC" will not. Because eager loading generates the SELECT statement too, the
+    # :select option is ignored.
     #
-    # Please note that limited eager loading with has_many and has_and_belongs_to_many associations is not compatible with describing conditions
-    # on these eager tables. This will work:
-    #
-    #   Post.find(:all, :include => :comments, :conditions => "posts.title = 'magic forest'", :limit => 2)
-    #
-    # ...but this will not (and an ArgumentError will be raised):
-    #
-    #   Post.find(:all, :include => :comments, :conditions => "comments.body like 'Normal%'", :limit => 2)
-    #
-    # Also have in mind that since the eager loading is pulling from multiple tables, you'll have to disambiguate any column references
-    # in both conditions and orders. So :order => "posts.id DESC" will work while :order => "id DESC" will not. This may require that
-    # you alter the :order and :conditions on the association definitions themselves.  Because eager loading generates the SELECT statement too,
-    # the :select option is ignored.
-    #
-    # It's currently not possible to use eager loading on multiple associations from the same table. Eager loading will not pull
-    # additional attributes on join tables, so "rich associations" with has_and_belongs_to_many is not a good fit for eager loading.
+    # You can use eager loading on multiple associations from the same table, but you cannot use those associations in orders and conditions
+    # as there is currently not any way to disambiguate them. Eager loading will not pull additional attributes on join tables, so "rich
+    # associations" with has_and_belongs_to_many are not a good fit for eager loading.
+    # 
+    # When eager loaded, conditions are not interpolated. This is because the interpolation of lazy loaded conditions happens within the context
+    # of the object; when eager loading the object does not exist yet.
     # 
     # == Table Aliasing
     #
@@ -1567,7 +1561,7 @@ module ActiveRecord
                 aliased_table_name, 
                 reflection.active_record.connection.quote_column_name(reflection.active_record.inheritance_column), 
                 klass.quote_value(klass.name.demodulize)] unless klass.descends_from_active_record?
-              join << "AND #{interpolate_sql(sanitize_sql(reflection.options[:conditions]))} " if reflection.options[:conditions]
+              join << "AND #{sanitize_sql(reflection.options[:conditions])} " if reflection.options[:conditions]
               join
             end
             
@@ -1583,10 +1577,6 @@ module ActiveRecord
               def table_name_and_alias
                 table_alias_for table_name, @aliased_table_name
               end
-
-              def interpolate_sql(sql)
-                instance_eval("%@#{sql.gsub('@', '\@')}@")
-              end
           end
         end
     end
diff --git a/activerecord/lib/active_record/associations/association_proxy.rb b/activerecord/lib/active_record/associations/association_proxy.rb
index 4cdbc52ccf..1b929f28bd 100644
--- a/activerecord/lib/active_record/associations/association_proxy.rb
+++ b/activerecord/lib/active_record/associations/association_proxy.rb
@@ -41,7 +41,7 @@ module ActiveRecord
       end
       
       def conditions
-        @conditions ||= eval("%(#{@reflection.klass.send :sanitize_sql, @reflection.options[:conditions]})") if @reflection.options[:conditions]
+        @conditions ||= interpolate_sql(sanitize_sql(@reflection.options[:conditions])) if @reflection.options[:conditions]
       end
       alias :sql_conditions :conditions
       
diff --git a/activerecord/test/associations/eager_test.rb b/activerecord/test/associations/eager_test.rb
index bcdd63ce34..d6e795b1fe 100644
--- a/activerecord/test/associations/eager_test.rb
+++ b/activerecord/test/associations/eager_test.rb
@@ -283,7 +283,7 @@ class EagerAssociationTest < Test::Unit::TestCase
     end
     # Test regular association, association with conditions, association with
     # STI, and association with conditions assured not to be true
-    post_types = [:posts, :hello_posts, :special_posts, :nonexistent_posts]
+    post_types = [:posts, :other_posts, :special_posts]
     # test both has_many and has_and_belongs_to_many
     [Author, Category].each do |className|
       d1 = find_all_ordered(className)
diff --git a/activerecord/test/associations_test.rb b/activerecord/test/associations_test.rb
index a75f708ea7..350799e306 100755
--- a/activerecord/test/associations_test.rb
+++ b/activerecord/test/associations_test.rb
@@ -315,6 +315,14 @@ class HasOneAssociationsTest < Test::Unit::TestCase
     assert_equal a, firm.account
     assert_equal a, firm.account(true)
   end
+  
+  def test_finding_with_interpolated_condition
+    firm = Firm.find(:first)
+    superior = firm.clients.create(:name => 'SuperiorCo')
+    superior.rating = 10
+    superior.save
+    assert_equal 10, firm.clients_with_interpolated_conditions.first.rating
+  end
 
   def test_assignment_before_child_saved
     firm = Firm.find(1)
diff --git a/activerecord/test/fixtures/author.rb b/activerecord/test/fixtures/author.rb
index 1c49b90fe5..7373639383 100644
--- a/activerecord/test/fixtures/author.rb
+++ b/activerecord/test/fixtures/author.rb
@@ -18,8 +18,7 @@ class Author < ActiveRecord::Base
   has_many :funky_comments, :through => :posts, :source => :comments
 
   has_many :special_posts,        :class_name => "Post"
-  has_many :hello_posts,          :class_name => "Post", :conditions=>"\#{aliased_table_name}.body = 'hello'"
-  has_many :nonexistent_posts,    :class_name => "Post", :conditions=>"\#{aliased_table_name}.body = 'nonexistent'"
+  has_many :other_posts,          :class_name => "Post"
   has_many :posts_with_callbacks, :class_name => "Post", :before_add => :log_before_adding,
            :after_add     => :log_after_adding, 
            :before_remove => :log_before_removing,
diff --git a/activerecord/test/fixtures/category.rb b/activerecord/test/fixtures/category.rb
index 6917c51d34..295bd8277f 100644
--- a/activerecord/test/fixtures/category.rb
+++ b/activerecord/test/fixtures/category.rb
@@ -1,8 +1,7 @@
 class Category < ActiveRecord::Base
   has_and_belongs_to_many :posts
   has_and_belongs_to_many :special_posts, :class_name => "Post"
-  has_and_belongs_to_many :hello_posts, :class_name => "Post", :conditions => "\#{aliased_table_name}.body = 'hello'"
-  has_and_belongs_to_many :nonexistent_posts, :class_name => "Post", :conditions=>"\#{aliased_table_name}.body = 'nonexistent'"
+  has_and_belongs_to_many :other_posts, :class_name => "Post"
   
   def self.what_are_you
     'a category...'
diff --git a/activerecord/test/fixtures/company.rb b/activerecord/test/fixtures/company.rb
index 37e4ebda56..c012a978a0 100755
--- a/activerecord/test/fixtures/company.rb
+++ b/activerecord/test/fixtures/company.rb
@@ -22,6 +22,7 @@ class Firm < Company
   has_many :exclusively_dependent_clients_of_firm, :foreign_key => "client_of", :class_name => "Client", :order => "id", :dependent => :delete_all
   has_many :limited_clients, :class_name => "Client", :order => "id", :limit => 1
   has_many :clients_like_ms, :conditions => "name = 'Microsoft'", :class_name => "Client", :order => "id"
+  has_many :clients_with_interpolated_conditions, :class_name => "Client", :conditions => 'rating > #{rating}'
   has_many :clients_like_ms_with_hash_conditions, :conditions => { :name => 'Microsoft' }, :class_name => "Client", :order => "id"
   has_many :clients_using_sql, :class_name => "Client", :finder_sql => 'SELECT * FROM companies WHERE client_of = #{id}'
   has_many :clients_using_counter_sql, :class_name => "Client",
diff --git a/activerecord/test/reflection_test.rb b/activerecord/test/reflection_test.rb
index 76e42860fb..796076bc81 100644
--- a/activerecord/test/reflection_test.rb
+++ b/activerecord/test/reflection_test.rb
@@ -143,8 +143,8 @@ class ReflectionTest < Test::Unit::TestCase
   end
   
   def test_reflection_of_all_associations
-    assert_equal 16, Firm.reflect_on_all_associations.size
-    assert_equal 14, Firm.reflect_on_all_associations(:has_many).size
+    assert_equal 17, Firm.reflect_on_all_associations.size
+    assert_equal 15, Firm.reflect_on_all_associations(:has_many).size
     assert_equal 2, Firm.reflect_on_all_associations(:has_one).size
     assert_equal 0, Firm.reflect_on_all_associations(:belongs_to).size
   end