Merge remote branch 'jonleighton/deprecate_string_interpolation-3-0-s…

…table' into 3-0-stable

* jonleighton/deprecate_string_interpolation-3-0-stable:
  Deprecated support for interpolated association conditions with the :conditions => 'foo = #{bar}' syntax, and added the new interpolation syntax which is :conditions => proc { "foo = #{bar}" }.

activerecord/CHANGELOG

@@ -1,4 +1,31 @@
-*Rails 3.0.4 (unreleased)*
+*Rails 3.0.5 (unreleased)*
+
+* Deprecated support for interpolated association conditions in the form of :conditions => 'foo = #{bar}'.
+
+  Instead, you should use a proc, like so:
+
+  Before:
+
+    has_many :things, :conditions => 'foo = #{bar}'
+
+  After:
+
+    has_many :things, :conditions => proc { "foo = #{bar}" }
+
+  Inside the proc, 'self' is the object which is the owner of the association, unless you are
+  eager loading the association, in which case 'self' is the class which the association is within.
+
+  You can have any "normal" conditions inside the proc, so the following will work too:
+
+    has_many :things, :conditions => proc { ["foo = ?", bar] }
+
+  Previously :insert_sql and :delete_sql on has_and_belongs_to_many association allowed you to call
+  'record' to get the record being inserted or deleted. This is now passed as an argument to
+  the proc.
+
+  [Jon Leighton]
+
+*Rails 3.0.4*
 
 * Added deprecation warning for has_and_belongs_to_many associations where the join table has
   additional attributes other than the keys. Access to these attributes is removed in 3.1.

activerecord/lib/active_record/association_preload.rb

@@ -387,15 +387,27 @@ def find_associated_records(ids, reflection, preload_options)
         end
       end
 
-
-      def interpolate_sql_for_preload(sql)
-        instance_eval("%@#{sql.gsub('@', '\@')}@", __FILE__, __LINE__)
+      def process_conditions_for_preload(conditions, klass = self)
+        sanitized = klass.send(:sanitize_sql, conditions)
+
+        if sanitized =~ /\#\{.*\}/
+          ActiveSupport::Deprecation.warn(
+            'String-based interpolation of association conditions is deprecated. Please use a ' \
+            'proc instead. So, for example, has_many :older_friends, :conditions => \'age > #{age}\' ' \
+            'should be changed to has_many :older_friends, :conditions => proc { "age > #{age}" }.'
+          )
+          instance_eval("%@#{sanitized.gsub('@', '\@')}@", __FILE__, __LINE__)
+        elsif conditions.respond_to?(:to_proc)
+          klass.send(:sanitize_sql, instance_eval(&conditions))
+        else
+          sanitized
+        end
       end
 
       def append_conditions(reflection, preload_options)
         sql = ""
-        sql << " AND (#{interpolate_sql_for_preload(reflection.sanitized_conditions)})" if reflection.sanitized_conditions
-        sql << " AND (#{sanitize_sql preload_options[:conditions]})" if preload_options[:conditions]
+        sql << " AND (#{process_conditions_for_preload(reflection.options[:conditions], reflection.klass)})" if reflection.options[:conditions]
+        sql << " AND (#{process_conditions_for_preload(preload_options[:conditions])})" if preload_options[:conditions]
         sql
       end
 

activerecord/lib/active_record/associations.rb

@@ -2221,7 +2221,7 @@ def association_join
 
               [through_reflection, reflection].each do |ref|
                 if ref && ref.options[:conditions]
-                  @join << interpolate_sql(sanitize_sql(ref.options[:conditions], aliased_table_name))
+                  @join << process_conditions(ref.options[:conditions], aliased_table_name)
                 end
               end
 
@@ -2282,8 +2282,21 @@ def table_name_and_alias
                 table_alias_for table_name, @aliased_table_name
               end
 
-              def interpolate_sql(sql)
-                instance_eval("%@#{sql.gsub('@', '\@')}@", __FILE__, __LINE__)
+              def process_conditions(conditions, table_name)
+                sanitized = sanitize_sql(conditions, table_name)
+
+                if sanitized =~ /\#\{.*\}/
+                  ActiveSupport::Deprecation.warn(
+                    'String-based interpolation of association conditions is deprecated. Please use a ' \
+                    'proc instead. So, for example, has_many :older_friends, :conditions => \'age > #{age}\' ' \
+                    'should be changed to has_many :older_friends, :conditions => proc { "age > #{age}" }.'
+                  )
+                  instance_eval("%@#{sanitized.gsub('@', '\@')}@", __FILE__, __LINE__)
+                elsif conditions.respond_to?(:to_proc)
+                  conditions = sanitize_sql(instance_eval(&conditions), table_name)
+                else
+                  sanitized
+                end
               end
           end
         end

activerecord/lib/active_record/associations/association_collection.rb

@@ -185,9 +185,11 @@ def sum(*args)
       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]
+        if @reflection.options[:finder_sql] || @reflection.options[:counter_sql]
+          unless options.blank?
+            raise ArgumentError, "If finder_sql/counter_sql is used then options cannot be passed"
+          end
+
           @reflection.klass.count_by_sql(@counter_sql)
         else
 
@@ -379,11 +381,10 @@ def construct_find_options!(options)
 
         def construct_counter_sql
           if @reflection.options[:counter_sql]
-            @counter_sql = interpolate_sql(@reflection.options[:counter_sql])
+            @counter_sql = interpolate_and_sanitize_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])
+            @counter_sql = interpolate_and_sanitize_sql(@reflection.options[:finder_sql]).sub(/SELECT\b(\/\*.*?\*\/ )?(.*)\bFROM\b/im) { "SELECT #{$1}COUNT(*) FROM" }
           else
             @counter_sql = @finder_sql
           end

activerecord/lib/active_record/associations/association_proxy.rb

@@ -102,7 +102,7 @@ def aliased_table_name
       # Returns the SQL string that corresponds to the <tt>:conditions</tt>
       # option of the macro, if given, or +nil+ otherwise.
       def conditions
-        @conditions ||= interpolate_sql(@reflection.sanitized_conditions) if @reflection.sanitized_conditions
+        @conditions ||= @reflection.options[:conditions] && interpolate_and_sanitize_sql(@reflection.options[:conditions])
       end
       alias :sql_conditions :conditions
 
@@ -161,8 +161,8 @@ def dependent?
           @reflection.options[:dependent]
         end
 
-        def interpolate_sql(sql, record = nil)
-          @owner.send(:interpolate_sql, sql, record)
+        def interpolate_and_sanitize_sql(sql, record = nil, sanitize_klass = @reflection.klass)
+          @owner.send(:interpolate_and_sanitize_sql, sql, record, sanitize_klass)
         end
 
         # Forwards the call to the reflection class.

activerecord/lib/active_record/associations/belongs_to_polymorphic_association.rb

@@ -24,7 +24,7 @@ def updated?
       end
 
       def conditions
-       	@conditions ||= interpolate_sql(association_class.send(:sanitize_sql, @reflection.options[:conditions])) if @reflection.options[:conditions]
+        @conditions ||= @reflection.options[:conditions] && interpolate_and_sanitize_sql(@reflection.options[:conditions], nil, association_class)
       end
 
       private

activerecord/lib/active_record/associations/has_and_belongs_to_many_association.rb

@@ -52,7 +52,7 @@ def insert_record(record, force = true, validate = true)
           end
 
           if @reflection.options[:insert_sql]
-            @owner.connection.insert(interpolate_sql(@reflection.options[:insert_sql], record))
+            @owner.connection.insert(interpolate_and_sanitize_sql(@reflection.options[:insert_sql], record))
           else
             relation   = Arel::Table.new(@reflection.options[:join_table])
             timestamps = record_timestamp_columns(record)
@@ -81,7 +81,7 @@ def insert_record(record, force = true, validate = true)
 
         def delete_records(records)
           if sql = @reflection.options[:delete_sql]
-            records.each { |record| @owner.connection.delete(interpolate_sql(sql, record)) }
+            records.each { |record| @owner.connection.delete(interpolate_and_sanitize_sql(sql, record)) }
           else
             relation = Arel::Table.new(@reflection.options[:join_table])
             relation.where(relation[@reflection.primary_key_name].eq(@owner.id).
@@ -92,7 +92,7 @@ def delete_records(records)
 
         def construct_sql
           if @reflection.options[:finder_sql]
-            @finder_sql = interpolate_sql(@reflection.options[:finder_sql])
+            @finder_sql = interpolate_and_sanitize_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

activerecord/lib/active_record/associations/has_many_association.rb

@@ -35,7 +35,7 @@ def owner_quoted_id
         def count_records
           count = if has_cached_counter?
             @owner.send(:read_attribute, cached_counter_attribute_name)
-          elsif @reflection.options[:counter_sql]
+          elsif @reflection.options[:finder_sql] || @reflection.options[:counter_sql]
             @reflection.klass.count_by_sql(@counter_sql)
           else
             @reflection.klass.count(:conditions => @counter_sql, :include => @reflection.options[:include])
@@ -90,7 +90,7 @@ def target_obsolete?
         def construct_sql
           case
             when @reflection.options[:finder_sql]
-              @finder_sql = interpolate_sql(@reflection.options[:finder_sql])
+              @finder_sql = interpolate_and_sanitize_sql(@reflection.options[:finder_sql])
 
             when @reflection.options[:as]
               @finder_sql =

activerecord/lib/active_record/associations/has_many_through_association.rb

@@ -87,7 +87,7 @@ def find_target
         def construct_sql
           case
             when @reflection.options[:finder_sql]
-              @finder_sql = interpolate_sql(@reflection.options[:finder_sql])
+              @finder_sql = interpolate_and_sanitize_sql(@reflection.options[:finder_sql])
 
               @finder_sql = "#{@reflection.quoted_table_name}.#{@reflection.primary_key_name} = #{owner_quoted_id}"
               @finder_sql << " AND (#{conditions})" if conditions

activerecord/lib/active_record/associations/through_association_scope.rb

@@ -123,7 +123,7 @@ def build_conditions
           all = []
 
           [association_conditions, source_conditions].each do |conditions|
-            all << interpolate_sql(sanitize_sql(conditions)) if conditions
+            all << interpolate_and_sanitize_sql(conditions) if conditions
           end
 
           all << through_conditions  if through_conditions
@@ -136,11 +136,11 @@ def build_conditions
       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(
+          interpolate_and_sanitize_sql(conditions, nil, @reflection.through_reflection.klass).gsub(
             @reflection.quoted_table_name,
             @reflection.through_reflection.quoted_table_name)
         elsif conditions
-          interpolate_sql(sanitize_sql(conditions))
+          interpolate_and_sanitize_sql(conditions)
         end
       end
 

activerecord/lib/active_record/base.rb

@@ -879,8 +879,8 @@ def arel_engine
       # It is recommended to use block form of unscoped because chaining unscoped with <tt>named_scope</tt>
       # does not work. Assuming that <tt>published</tt> is a <tt>named_scope</tt> following two statements are same.
       #
-      # Post.unscoped.published 
-      # Post.published 
+      # Post.unscoped.published
+      # Post.published
       def unscoped #:nodoc:
         block_given? ? relation.scoping { yield } : relation
       end
@@ -1606,7 +1606,7 @@ def column_for_attribute(name)
         self.class.columns_hash[name.to_s]
       end
 
-      # Returns true if +comparison_object+ is the same exact object, or +comparison_object+ 
+      # Returns true if +comparison_object+ is the same exact object, or +comparison_object+
       # is of the same type and +self+ has an ID and it is equal to +comparison_object.id+.
       #
       # Note that new records are different from any other record by definition, unless the
@@ -1730,10 +1730,21 @@ def quote_value(value, column = nil)
         self.class.connection.quote(value, column)
       end
 
-      # Interpolate custom SQL string in instance context.
-      # Optional record argument is meant for custom insert_sql.
-      def interpolate_sql(sql, record = nil)
-        instance_eval("%@#{sql.gsub('@', '\@')}@", __FILE__, __LINE__)
+      def interpolate_and_sanitize_sql(sql, record = nil, sanitize_klass = self.class)
+        sanitized = sanitize_klass.send(:sanitize_sql, sql)
+
+        if sanitized =~ /\#\{.*\}/
+          ActiveSupport::Deprecation.warn(
+            'String-based interpolation of association conditions is deprecated. Please use a ' \
+            'proc instead. So, for example, has_many :older_friends, :conditions => \'age > #{age}\' ' \
+            '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))
+        else
+          sanitized
+        end
       end
 
       # Instantiates objects for all attribute classes that needs more than one constructor parameter. This is done

activerecord/test/cases/associations/belongs_to_associations_test.rb

@@ -151,7 +151,7 @@ def test_with_condition
     assert_equal Company.find(1).name, Company.find(3).firm_with_condition.name
     assert_not_nil Company.find(3).firm_with_condition, "Microsoft should have a firm"
   end
-  
+
   def test_with_polymorphic_and_condition
     sponsor = Sponsor.create
     member = Member.create :name => "Bert"

activerecord/test/cases/associations/eager_test.rb

@@ -651,7 +651,23 @@ def test_limited_eager_with_numeric_in_association
   end
 
   def test_preload_with_interpolation
-    assert_equal [comments(:greetings)], Post.find(posts(:welcome).id, :include => :comments_with_interpolated_conditions).comments_with_interpolated_conditions
+    post = Post.includes(:comments_with_interpolated_conditions).find(posts(:welcome).id)
+    assert_equal [comments(:greetings)], post.comments_with_interpolated_conditions
+
+    post = Post.joins(:comments_with_interpolated_conditions).find(posts(:welcome).id)
+    assert_equal [comments(:greetings)], post.comments_with_interpolated_conditions
+  end
+
+  def test_preload_with_deprecated_interpolation
+    post = assert_deprecated do
+      Post.includes(:comments_with_deprecated_interpolated_conditions).find(posts(:welcome).id)
+    end
+    assert_equal [comments(:greetings)], post.comments_with_interpolated_conditions
+
+    post = assert_deprecated do
+      Post.joins(:comments_with_deprecated_interpolated_conditions).find(posts(:welcome).id)
+    end
+    assert_equal [comments(:greetings)], post.comments_with_interpolated_conditions
   end
 
   def test_polymorphic_type_condition

activerecord/test/cases/associations/has_and_belongs_to_many_associations_test.rb

@@ -415,6 +415,16 @@ def test_deleting_with_sql
     assert_equal 2, active_record.developers_by_sql(true).size
   end
 
+  def test_deleting_with_sql_with_deprecated_interpolation
+    david = Developer.find(1)
+    active_record = Project.find(1)
+    active_record.developers.reload
+    assert_equal 3, active_record.developers_by_sql_deprecated.size
+
+    active_record.developers_by_sql_deprecated.delete(david)
+    assert_equal 2, active_record.developers_by_sql_deprecated(true).size
+  end
+
   def test_deleting_array_with_sql
     active_record = Project.find(1)
     active_record.developers.reload
@@ -837,7 +847,7 @@ def test_count_with_counter_sql
   unless current_adapter?(:PostgreSQLAdapter)
     def test_count_with_finder_sql
       assert_equal 3, projects(:active_record).developers_with_finder_sql.count
-      assert_equal 3, projects(:active_record).developers_with_multiline_finder_sql.count
+      assert_equal 3, projects(:active_record).developers_with_deprecated_multiline_finder_sql.count
     end
   end
 

activerecord/test/cases/associations/has_many_associations_test.rb

@@ -247,7 +247,9 @@ def test_counting_non_existant_items_using_sql
 
   def test_counting_using_finder_sql
     assert_equal 2, Firm.find(4).clients_using_sql.count
-    assert_equal 2, Firm.find(4).clients_using_multiline_sql.count
+    assert_deprecated do
+      assert_equal 2, Firm.find(4).clients_using_deprecated_multiline_sql.count
+    end
   end
 
   def test_belongs_to_sanity

activerecord/test/cases/associations/has_many_through_associations_test.rb

@@ -21,7 +21,7 @@
 require 'models/categorization'
 
 class HasManyThroughAssociationsTest < ActiveRecord::TestCase
-  fixtures :posts, :readers, :people, :comments, :authors, :categories,
+  fixtures :posts, :readers, :people, :comments, :authors, :categories, :tags, :taggings,
            :owners, :pets, :toys, :jobs, :references, :companies,
            :subscribers, :books, :subscriptions, :developers, :categorizations
 
@@ -477,4 +477,22 @@ def test_joining_has_many_through_belongs_to
 
     assert_equal [posts(:eager_other)], posts
   end
+
+  def test_interpolated_conditions
+    post = posts(:welcome)
+    assert !post.tags.empty?
+    assert_equal post.tags, post.interpolated_tags
+    assert_equal post.tags, post.interpolated_tags_2
+  end
+
+  def test_deprecated_interpolated_conditions
+    post = posts(:welcome)
+    assert !post.tags.empty?
+    assert_deprecated do
+      assert_equal post.tags, post.deprecated_interpolated_tags
+    end
+    assert_deprecated do
+      assert_equal post.tags, post.deprecated_interpolated_tags_2
+    end
+  end
 end

activerecord/test/cases/associations/has_one_associations_test.rb

@@ -265,6 +265,16 @@ def test_finding_with_interpolated_condition
     assert_equal 10, firm.clients_with_interpolated_conditions.first.rating
   end
 
+  def test_finding_with_deprecated_interpolated_condition
+    firm = Firm.find(:first)
+    superior = firm.clients.create(:name => 'SuperiorCo')
+    superior.rating = 10
+    superior.save
+    assert_deprecated do
+      assert_equal 10, firm.clients_with_deprecated_interpolated_conditions.first.rating
+    end
+  end
+
   def test_assignment_before_child_saved
     firm = Firm.find(1)
     firm.account = a = Account.new("credit_limit" => 1000)