Permalink
Browse files

Deprecated support for interpolated association conditions with the :…

…conditions => 'foo = #{bar}' syntax, and added the new interpolation syntax which is :conditions => proc { "foo = #{bar}" }.
  • Loading branch information...
1 parent 66003f5 commit 756e70cb4492ded56b72b1601da7d198eaf7b840 @jonleighton jonleighton committed Feb 13, 2011
Showing with 186 additions and 59 deletions.
  1. +28 −1 activerecord/CHANGELOG
  2. +17 −5 activerecord/lib/active_record/association_preload.rb
  3. +16 −3 activerecord/lib/active_record/associations.rb
  4. +7 −6 activerecord/lib/active_record/associations/association_collection.rb
  5. +3 −3 activerecord/lib/active_record/associations/association_proxy.rb
  6. +1 −1 activerecord/lib/active_record/associations/belongs_to_polymorphic_association.rb
  7. +3 −3 activerecord/lib/active_record/associations/has_and_belongs_to_many_association.rb
  8. +2 −2 activerecord/lib/active_record/associations/has_many_association.rb
  9. +1 −1 activerecord/lib/active_record/associations/has_many_through_association.rb
  10. +3 −3 activerecord/lib/active_record/associations/through_association_scope.rb
  11. +18 −7 activerecord/lib/active_record/base.rb
  12. +1 −1 activerecord/test/cases/associations/belongs_to_associations_test.rb
  13. +17 −1 activerecord/test/cases/associations/eager_test.rb
  14. +11 −1 activerecord/test/cases/associations/has_and_belongs_to_many_associations_test.rb
  15. +3 −1 activerecord/test/cases/associations/has_many_associations_test.rb
  16. +19 −1 activerecord/test/cases/associations/has_many_through_associations_test.rb
  17. +10 −0 activerecord/test/cases/associations/has_one_associations_test.rb
  18. +0 −6 activerecord/test/cases/base_test.rb
  19. +2 −2 activerecord/test/cases/reflection_test.rb
  20. +8 −7 activerecord/test/models/company.rb
  21. +9 −0 activerecord/test/models/post.rb
  22. +4 −3 activerecord/test/models/project.rb
  23. +3 −1 activerecord/test/models/tagging.rb
@@ -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.
@@ -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
@@ -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
@@ -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
@@ -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.
@@ -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
@@ -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
@@ -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 =
@@ -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
@@ -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
@@ -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
@@ -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"
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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)
Oops, something went wrong. Retry.

0 comments on commit 756e70c

Please sign in to comment.