From 49403831fc90a9d0d6955bab2ae6f7833be3c0ba Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Tue, 7 Dec 2004 14:48:53 +0000 Subject: Fixed value quoting in all generated SQL statements, so that integers are not surrounded in quotes and that all sanitation are happening through the database's own quoting routine. This should hopefully make it lots easier for new adapters that doesn't accept '1' for integer columns. git-svn-id: http://svn-commit.rubyonrails.org/rails/trunk@70 5ecf4fe2-1ee6-0310-87b1-e25e094e27de --- activerecord/CHANGELOG | 4 +++ activerecord/lib/active_record/associations.rb | 6 ++--- .../associations/association_collection.rb | 2 +- .../has_and_belongs_to_many_association.rb | 13 ++++----- .../associations/has_many_association.rb | 17 +++++++----- activerecord/lib/active_record/base.rb | 31 +++++++++++++--------- .../connection_adapters/abstract_adapter.rb | 15 ++++++----- .../connection_adapters/mysql_adapter.rb | 4 +++ .../lib/active_record/support/inflector.rb | 26 +++++++++--------- activerecord/test/finder_test.rb | 4 +-- 10 files changed, 71 insertions(+), 51 deletions(-) diff --git a/activerecord/CHANGELOG b/activerecord/CHANGELOG index 80218b9070..eb8a906338 100644 --- a/activerecord/CHANGELOG +++ b/activerecord/CHANGELOG @@ -1,5 +1,9 @@ *CVS* +* Fixed value quoting in all generated SQL statements, so that integers are not surrounded in quotes and that all sanitation are happening + through the database's own quoting routine. This should hopefully make it lots easier for new adapters that doesn't accept '1' for integer + columns. + * Fixed has_and_belongs_to_many guessing of foreign key so that keys are generated correctly for models like SomeVerySpecialClient [Florian Weber] diff --git a/activerecord/lib/active_record/associations.rb b/activerecord/lib/active_record/associations.rb index b83ce831c7..6f6da10a01 100755 --- a/activerecord/lib/active_record/associations.rb +++ b/activerecord/lib/active_record/associations.rb @@ -190,7 +190,7 @@ module ActiveRecord elsif options[:dependent] module_eval "before_destroy '#{association_name}.each { |o| o.destroy }'" elsif options[:exclusively_dependent] - module_eval "before_destroy { |record| #{association_class_name}.delete_all(%(#{association_class_primary_key_name} = '\#{record.id}')) }" + module_eval "before_destroy { |record| #{association_class_name}.delete_all(%(#{association_class_primary_key_name} = \#{record.quoted_id})) }" end define_method(association_name) do |*params| @@ -323,7 +323,7 @@ module ActiveRecord if options[:remote] association_finder = <<-"end_eval" #{association_class_name}.find_first( - "#{class_primary_key_name} = '\#{id}'#{options[:conditions] ? " AND " + options[:conditions] : ""}", + "#{class_primary_key_name} = \#{quoted_id}#{options[:conditions] ? " AND " + options[:conditions] : ""}", #{options[:order] ? "\"" + options[:order] + "\"" : "nil" } ) end_eval @@ -437,7 +437,7 @@ module ActiveRecord association end - before_destroy_sql = "DELETE FROM #{join_table} WHERE #{association_class_primary_key_name} = '\\\#{self.id}'" + before_destroy_sql = "DELETE FROM #{join_table} WHERE #{association_class_primary_key_name} = \\\#{self.quoted_id}" module_eval(%{before_destroy "self.connection.delete(%{#{before_destroy_sql}})"}) # " # deprecated api diff --git a/activerecord/lib/active_record/associations/association_collection.rb b/activerecord/lib/active_record/associations/association_collection.rb index a60b9ddab5..00758aa66c 100644 --- a/activerecord/lib/active_record/associations/association_collection.rb +++ b/activerecord/lib/active_record/associations/association_collection.rb @@ -81,7 +81,7 @@ module ActiveRecord end def quoted_record_ids(records) - records.map { |record| "'#{@association_class.send(:sanitize, record.id)}'" }.join(',') + records.map { |record| record.quoted_id }.join(',') end def interpolate_sql_options!(options, *keys) diff --git a/activerecord/lib/active_record/associations/has_and_belongs_to_many_association.rb b/activerecord/lib/active_record/associations/has_and_belongs_to_many_association.rb index 8dec71403c..d53650fbd8 100644 --- a/activerecord/lib/active_record/associations/has_and_belongs_to_many_association.rb +++ b/activerecord/lib/active_record/associations/has_and_belongs_to_many_association.rb @@ -13,7 +13,7 @@ module ActiveRecord @finder_sql = options[:finder_sql] || "SELECT t.*, j.* FROM #{association_table_name} t, #{@join_table} j " + "WHERE t.#{@owner.class.primary_key} = j.#{@association_foreign_key} AND " + - "j.#{association_class_primary_key_name} = '#{@owner.id}' " + + "j.#{association_class_primary_key_name} = #{@owner.quoted_id} " + (options[:conditions] ? " AND " + options[:conditions] : "") + " " + "ORDER BY #{@order}" end @@ -26,11 +26,11 @@ module ActiveRecord each { |record| @owner.connection.execute(sql) } elsif @options[:conditions] sql = - "DELETE FROM #{@join_table} WHERE #{@association_class_primary_key_name} = '#{@owner.id}' " + + "DELETE FROM #{@join_table} WHERE #{@association_class_primary_key_name} = #{@owner.quoted_id} " + "AND #{@association_foreign_key} IN (#{collect { |record| record.id }.join(", ")})" @owner.connection.execute(sql) else - sql = "DELETE FROM #{@join_table} WHERE #{@association_class_primary_key_name} = '#{@owner.id}'" + sql = "DELETE FROM #{@join_table} WHERE #{@association_class_primary_key_name} = #{@owner.quoted_id}" @owner.connection.execute(sql) end @@ -46,7 +46,7 @@ module ActiveRecord if loaded? find_all { |record| record.id == association_id.to_i }.first else - find_all_records(@finder_sql.sub(/ORDER BY/, "AND j.#{@association_foreign_key} = '#{association_id}' ORDER BY")).first + find_all_records(@finder_sql.sub(/ORDER BY/, "AND j.#{@association_foreign_key} = #{@owner.send(:quote, association_id)} ORDER BY")).first end end end @@ -80,7 +80,8 @@ module ActiveRecord if @options[:insert_sql] @owner.connection.execute(interpolate_sql(@options[:insert_sql], record)) else - sql = "INSERT INTO #{@join_table} (#{@association_class_primary_key_name}, #{@association_foreign_key}) VALUES ('#{@owner.id}','#{record.id}')" + sql = "INSERT INTO #{@join_table} (#{@association_class_primary_key_name}, #{@association_foreign_key}) " + + "VALUES (#{@owner.quoted_id},#{record.quoted_id})" @owner.connection.execute(sql) end end @@ -98,7 +99,7 @@ module ActiveRecord records.each { |record| @owner.connection.execute(sql) } else ids = quoted_record_ids(records) - sql = "DELETE FROM #{@join_table} WHERE #{@association_class_primary_key_name} = '#{@owner.id}' AND #{@association_foreign_key} IN (#{ids})" + sql = "DELETE FROM #{@join_table} WHERE #{@association_class_primary_key_name} = #{@owner.quoted_id} AND #{@association_foreign_key} IN (#{ids})" @owner.connection.execute(sql) end end diff --git a/activerecord/lib/active_record/associations/has_many_association.rb b/activerecord/lib/active_record/associations/has_many_association.rb index 0f2d20d240..1d8441e6f8 100644 --- a/activerecord/lib/active_record/associations/has_many_association.rb +++ b/activerecord/lib/active_record/associations/has_many_association.rb @@ -8,7 +8,7 @@ module ActiveRecord if options[:finder_sql] @finder_sql = interpolate_sql(options[:finder_sql]) else - @finder_sql = "#{@association_class_primary_key_name} = '#{@owner.id}' #{@conditions ? " AND " + interpolate_sql(@conditions) : ""}" + @finder_sql = "#{@association_class_primary_key_name} = #{@owner.quoted_id} #{@conditions ? " AND " + interpolate_sql(@conditions) : ""}" end if options[:counter_sql] @@ -16,7 +16,7 @@ module ActiveRecord elsif options[:finder_sql] @counter_sql = options[:counter_sql] = @finder_sql.gsub(/SELECT (.*) FROM/i, "SELECT COUNT(*) FROM") else - @counter_sql = "#{@association_class_primary_key_name} = '#{@owner.id}'#{@conditions ? " AND " + interpolate_sql(@conditions) : ""}" + @counter_sql = "#{@association_class_primary_key_name} = #{@owner.quoted_id}#{@conditions ? " AND " + interpolate_sql(@conditions) : ""}" end end @@ -40,8 +40,8 @@ module ActiveRecord @collection.find_all(&block) else @association_class.find_all( - "#{@association_class_primary_key_name} = '#{@owner.id}' " + - "#{@conditions ? " AND " + @conditions : ""} #{runtime_conditions ? " AND " + @association_class.send(:sanitize_conditions, runtime_conditions) : ""}", + "#{@association_class_primary_key_name} = #{@owner.quoted_id}" + + "#{@conditions ? " AND " + @conditions : ""}#{runtime_conditions ? " AND " + @association_class.send(:sanitize_conditions, runtime_conditions) : ""}", orderings, limit, joins @@ -55,7 +55,7 @@ module ActiveRecord @collection.find(&block) else @association_class.find_on_conditions(association_id, - "#{@association_class_primary_key_name} = '#{@owner.id}' #{@conditions ? " AND " + @conditions : ""}" + "#{@association_class_primary_key_name} = #{@owner.quoted_id}#{@conditions ? " AND " + @conditions : ""}" ) end end @@ -63,7 +63,7 @@ module ActiveRecord # Removes all records from this association. Returns +self+ so # method calls may be chained. def clear - @association_class.update_all("#{@association_class_primary_key_name} = NULL", "#{@association_class_primary_key_name} = '#{@owner.id}'") + @association_class.update_all("#{@association_class_primary_key_name} = NULL", "#{@association_class_primary_key_name} = #{@owner.quoted_id}") @collection = [] self end @@ -101,7 +101,10 @@ module ActiveRecord def delete_records(records) ids = quoted_record_ids(records) - @association_class.update_all("#{@association_class_primary_key_name} = NULL", "#{@association_class_primary_key_name} = '#{@owner.id}' AND #{@association_class.primary_key} IN (#{ids})") + @association_class.update_all( + "#{@association_class_primary_key_name} = NULL", + "#{@association_class_primary_key_name} = #{@owner.quoted_id} AND #{@association_class.primary_key} IN (#{ids})" + ) end end end diff --git a/activerecord/lib/active_record/base.rb b/activerecord/lib/active_record/base.rb index a45480945e..f52a1524d2 100755 --- a/activerecord/lib/active_record/base.rb +++ b/activerecord/lib/active_record/base.rb @@ -239,7 +239,7 @@ module ActiveRecord #:nodoc: ids = ids.flatten.compact.uniq if ids.length > 1 - ids_list = ids.map{ |id| "'#{sanitize(id)}'" }.join(", ") + ids_list = ids.map{ |id| "#{sanitize(id)}" }.join(", ") objects = find_all("#{primary_key} IN (#{ids_list})", primary_key) if objects.length == ids.length @@ -249,7 +249,7 @@ module ActiveRecord #:nodoc: end elsif ids.length == 1 id = ids.first - sql = "SELECT * FROM #{table_name} WHERE #{primary_key} = '#{sanitize(id)}'" + sql = "SELECT * FROM #{table_name} WHERE #{primary_key} = #{sanitize(id)}" sql << " AND #{type_condition}" unless descends_from_active_record? if record = connection.select_one(sql, "#{name} Find") @@ -267,7 +267,7 @@ module ActiveRecord #:nodoc: # Example: # Person.find_on_conditions 5, "first_name LIKE '%dav%' AND last_name = 'heinemeier'" def find_on_conditions(id, conditions) - find_first("#{primary_key} = '#{sanitize(id)}' AND #{sanitize_conditions(conditions)}") || + find_first("#{primary_key} = #{sanitize(id)} AND #{sanitize_conditions(conditions)}") || raise(RecordNotFound, "Couldn't find #{name} with #{primary_key} = #{id} on the condition of #{conditions}") end @@ -370,12 +370,12 @@ module ActiveRecord #:nodoc: # for looping over a collection where each element require a number of aggregate values. Like the DiscussionBoard # that needs to list both the number of posts and comments. def increment_counter(counter_name, id) - update_all "#{counter_name} = #{counter_name} + 1", "#{primary_key} = #{id}" + update_all "#{counter_name} = #{counter_name} + 1", "#{primary_key} = #{quote(id)}" end # Works like increment_counter, but decrements instead. def decrement_counter(counter_name, id) - update_all "#{counter_name} = #{counter_name} - 1", "#{primary_key} = #{id}" + update_all "#{counter_name} = #{counter_name} - 1", "#{primary_key} = #{quote(id)}" end # Attributes named in this macro are protected from mass-assignment, such as new(attributes) and @@ -526,10 +526,13 @@ module ActiveRecord #:nodoc: superclass == Base end - # Used to sanitize objects before they're used in an SELECT SQL-statement. + def quote(object) + connection.quote(object) + end + + # Used to sanitize objects before they're used in an SELECT SQL-statement. Delegates to connection.quote. def sanitize(object) # :nodoc: - return object if Fixnum === object - object.to_s.gsub(/([;:])/, "").gsub('##', '\#\#').gsub(/'/, "''") # ' (for ruby-mode) + connection.quote(object) end # Used to aggregate logging and benchmark, so you can measure and represent multiple statements in a single block. @@ -592,7 +595,7 @@ module ActiveRecord #:nodoc: def type_condition " (" + subclasses.inject("#{inheritance_column} = '#{Inflector.demodulize(name)}' ") do |condition, subclass| - condition << "OR #{inheritance_column} = '#{Inflector.demodulize(subclass.name)}'" + condition << "OR #{inheritance_column} = '#{Inflector.demodulize(subclass.name)}' " end + ") " end @@ -638,7 +641,7 @@ module ActiveRecord #:nodoc: statement =~ /\?/ ? replace_bind_variables(statement, values) : - statement % values.collect { |value| sanitize(value) } + statement % values.collect { |value| connection.quote_string(value.to_s) } end def replace_bind_variables(statement, values) @@ -669,6 +672,10 @@ module ActiveRecord #:nodoc: read_attribute(self.class.primary_key) end + def quoted_id + quote(id, self.class.columns_hash[self.class.primary_key]) + end + # Sets the primary ID. def id=(value) write_attribute(self.class.primary_key, value) @@ -692,7 +699,7 @@ module ActiveRecord #:nodoc: unless new_record? connection.delete( "DELETE FROM #{self.class.table_name} " + - "WHERE #{self.class.primary_key} = '#{id}'", + "WHERE #{self.class.primary_key} = #{quote(id)}", "#{self.class.name} Destroy" ) end @@ -814,7 +821,7 @@ module ActiveRecord #:nodoc: connection.update( "UPDATE #{self.class.table_name} " + "SET #{quoted_comma_pair_list(connection, attributes_with_quotes(false))} " + - "WHERE #{self.class.primary_key} = '#{id}'", + "WHERE #{self.class.primary_key} = #{quote(id)}", "#{self.class.name} Update" ) end diff --git a/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb b/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb index 301ee208a3..6f4dca1b7b 100755 --- a/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb @@ -320,13 +320,14 @@ module ActiveRecord def quote(value, column = nil) case value - when String then "'#{quote_string(value)}'" # ' (for ruby-mode) - when NilClass then "NULL" - when TrueClass then (column && column.type == :boolean ? "'t'" : "1") - when FalseClass then (column && column.type == :boolean ? "'f'" : "0") - when Float, Fixnum, Bignum, Date then "'#{value.to_s}'" - when Time, DateTime then "'#{value.strftime("%Y-%m-%d %H:%M:%S")}'" - else "'#{quote_string(value.to_yaml)}'" + when String then "'#{quote_string(value)}'" # ' (for ruby-mode) + when NilClass then "NULL" + when TrueClass then (column && column.type == :boolean ? "'t'" : "1") + when FalseClass then (column && column.type == :boolean ? "'f'" : "0") + when Float, Fixnum, Bignum then value.to_s + when Date then "'#{value.to_s}'" + when Time, DateTime then "'#{value.strftime("%Y-%m-%d %H:%M:%S")}'" + else "'#{quote_string(value.to_yaml)}'" end end diff --git a/activerecord/lib/active_record/connection_adapters/mysql_adapter.rb b/activerecord/lib/active_record/connection_adapters/mysql_adapter.rb index 5dcdded5bc..55c15c6823 100755 --- a/activerecord/lib/active_record/connection_adapters/mysql_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/mysql_adapter.rb @@ -117,6 +117,10 @@ module ActiveRecord execute "CREATE DATABASE #{name}" end + def quote_string(s) + Mysql::quote(s) + end + private def select(sql, name = nil) result = nil diff --git a/activerecord/lib/active_record/support/inflector.rb b/activerecord/lib/active_record/support/inflector.rb index b41f85188b..bc5724e742 100644 --- a/activerecord/lib/active_record/support/inflector.rb +++ b/activerecord/lib/active_record/support/inflector.rb @@ -62,18 +62,18 @@ module Inflector def singular_rules #:doc: [ - [/(x|ch|ss)es$/, '\1'], - [/movies$/, 'movie'], - [/([^aeiouy]|qu)ies$/, '\1y'], - [/([lr])ves$/, '\1f'], - [/([^f])ves$/, '\1fe'], - [/(analy|ba|diagno|parenthe|progno|synop|the)ses$/, '\1sis'], - [/([ti])a$/, '\1um'], - [/people$/, 'person'], - [/men$/, 'man'], - [/status$/, 'status'], - [/children$/, 'child'], - [/s$/, ''] - ] + [/(x|ch|ss)es$/, '\1'], + [/movies$/, 'movie'], + [/([^aeiouy]|qu)ies$/, '\1y'], + [/([lr])ves$/, '\1f'], + [/([^f])ves$/, '\1fe'], + [/(analy|ba|diagno|parenthe|progno|synop|the)ses$/, '\1sis'], + [/([ti])a$/, '\1um'], + [/people$/, 'person'], + [/men$/, 'man'], + [/status$/, 'status'], + [/children$/, 'child'], + [/s$/, ''] + ] end end \ No newline at end of file diff --git a/activerecord/test/finder_test.rb b/activerecord/test/finder_test.rb index b7b4ab589a..cc240c8acc 100755 --- a/activerecord/test/finder_test.rb +++ b/activerecord/test/finder_test.rb @@ -68,7 +68,7 @@ class FinderTest < Test::Unit::TestCase end def test_string_sanitation - assert_equal "something '' 1=1", ActiveRecord::Base.sanitize("something ' 1=1") - assert_equal "something select table", ActiveRecord::Base.sanitize("something; select table") + assert_not_equal "'something ' 1=1'", ActiveRecord::Base.sanitize("something ' 1=1") + assert_equal "'something; select table'", ActiveRecord::Base.sanitize("something; select table") end end \ No newline at end of file -- cgit v1.2.3