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

@@ -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]
 

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

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)

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

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}' " +
+            "#{@association_class_primary_key_name} = #{@owner.quoted_id}" +
-            "#{@conditions ? " AND " + @conditions : ""} #{runtime_conditions ? " AND " + @association_class.send(:sanitize_conditions, runtime_conditions) : ""}",
+            "#{@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

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 <tt>new(attributes)</tt> 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 <tt>connection.quote</tt>.
       def sanitize(object) # :nodoc:
-        return object if Fixnum === object
+        connection.quote(object)
-        object.to_s.gsub(/([;:])/, "").gsub('##', '\#\#').gsub(/'/, "''") # ' (for ruby-mode)
       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

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 String                then "'#{quote_string(value)}'" # ' (for ruby-mode)
-          when NilClass                    then "NULL"
+          when NilClass              then "NULL"
-          when TrueClass                   then (column && column.type == :boolean ? "'t'" : "1")
+          when TrueClass             then (column && column.type == :boolean ? "'t'" : "1")
-          when FalseClass                  then (column && column.type == :boolean ? "'f'" : "0")
+          when FalseClass            then (column && column.type == :boolean ? "'f'" : "0")
-          when Float, Fixnum, Bignum, Date then "'#{value.to_s}'" 
+          when Float, Fixnum, Bignum then value.to_s
-          when Time, DateTime              then "'#{value.strftime("%Y-%m-%d %H:%M:%S")}'"
+          when Date                  then "'#{value.to_s}'" 
-          else                                  "'#{quote_string(value.to_yaml)}'"
+          when Time, DateTime        then "'#{value.strftime("%Y-%m-%d %H:%M:%S")}'"
+          else                            "'#{quote_string(value.to_yaml)}'"
         end
       end
 

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

activerecord/lib/active_record/support/inflector.rb

@@ -62,18 +62,18 @@ module Inflector
 
     def singular_rules #:doc:
       [
-          [/(x|ch|ss)es$/, '\1'],
+        [/(x|ch|ss)es$/, '\1'],
-          [/movies$/, 'movie'],
+        [/movies$/, 'movie'],
-          [/([^aeiouy]|qu)ies$/, '\1y'],
+        [/([^aeiouy]|qu)ies$/, '\1y'],
-          [/([lr])ves$/, '\1f'],
+        [/([lr])ves$/, '\1f'],
-          [/([^f])ves$/, '\1fe'],
+        [/([^f])ves$/, '\1fe'],
-          [/(analy|ba|diagno|parenthe|progno|synop|the)ses$/, '\1sis'],
+        [/(analy|ba|diagno|parenthe|progno|synop|the)ses$/, '\1sis'],
-          [/([ti])a$/, '\1um'],
+        [/([ti])a$/, '\1um'],
-          [/people$/, 'person'],
+        [/people$/, 'person'],
-          [/men$/, 'man'],
+        [/men$/, 'man'],
-          [/status$/, 'status'],
+        [/status$/, 'status'],
-          [/children$/, 'child'],
+        [/children$/, 'child'],
-          [/s$/, '']
+        [/s$/, '']
-        ]
+      ]
     end
 end

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_not_equal "'something ' 1=1'", ActiveRecord::Base.sanitize("something ' 1=1")
-    assert_equal "something select table", ActiveRecord::Base.sanitize("something; select table")
+    assert_equal "'something; select table'", ActiveRecord::Base.sanitize("something; select table")
   end
 end