More thoroughly quote table names. Exposes some issues with sqlite2 adapter. Closes #10698.

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

activerecord/CHANGELOG

@@ -1,5 +1,7 @@
 *SVN*
 
+* More thoroughly quote table names.  #10698 [dimdenis, lotswholetime, Jeremy Kemper]
+
 * update_all ignores scoped :order and :limit, so post.comments.update_all doesn't try to include the comment order in the update statement.  #10686 [Brendan Ribera]
 
 * Added ActiveRecord::Base.cache_key to make it easier to cache Active Records in combination with the new ActiveSupport::Cache::* libraries [DHH]

activerecord/lib/active_record/associations/has_and_belongs_to_many_association.rb

@@ -100,7 +100,7 @@ module ActiveRecord
             end
 
             sql =
-              "INSERT INTO #{@reflection.options[:join_table]} (#{@owner.send(:quoted_column_names, attributes).join(', ')}) " +
+              "INSERT INTO #{@owner.connection.quote_table_name @reflection.options[:join_table]} (#{@owner.send(:quoted_column_names, attributes).join(', ')}) " +
               "VALUES (#{attributes.values.join(', ')})"
 
             @owner.connection.execute(sql)
@@ -114,7 +114,7 @@ module ActiveRecord
             records.each { |record| @owner.connection.execute(interpolate_sql(sql, record)) }
           else
             ids = quoted_record_ids(records)
-            sql = "DELETE FROM #{@reflection.options[:join_table]} WHERE #{@reflection.primary_key_name} = #{@owner.quoted_id} AND #{@reflection.association_foreign_key} IN (#{ids})"
+            sql = "DELETE FROM #{@owner.connection.quote_table_name @reflection.options[:join_table]} WHERE #{@reflection.primary_key_name} = #{@owner.quoted_id} AND #{@reflection.association_foreign_key} IN (#{ids})"
             @owner.connection.execute(sql)
           end
         end
@@ -125,11 +125,11 @@ module ActiveRecord
           if @reflection.options[:finder_sql]
             @finder_sql = @reflection.options[:finder_sql]
           else
-            @finder_sql = "#{@reflection.options[:join_table]}.#{@reflection.primary_key_name} = #{@owner.quoted_id} "
+            @finder_sql = "#{@owner.connection.quote_table_name @reflection.options[:join_table]}.#{@reflection.primary_key_name} = #{@owner.quoted_id} "
             @finder_sql << " AND (#{conditions})" if conditions
           end
 
-          @join_sql = "INNER JOIN #{@reflection.options[:join_table]} ON #{@reflection.klass.table_name}.#{@reflection.klass.primary_key} = #{@reflection.options[:join_table]}.#{@reflection.association_foreign_key}"
+          @join_sql = "INNER JOIN #{@owner.connection.quote_table_name @reflection.options[:join_table]} ON #{@reflection.quoted_table_name}.#{@reflection.klass.primary_key} = #{@owner.connection.quote_table_name @reflection.options[:join_table]}.#{@reflection.association_foreign_key}"
         end
 
         def construct_scope

activerecord/lib/active_record/associations/has_many_association.rb

@@ -144,12 +144,12 @@ module ActiveRecord
 
             when @reflection.options[:as]
               @finder_sql = 
-                "#{@reflection.klass.table_name}.#{@reflection.options[:as]}_id = #{@owner.quoted_id} AND " + 
+                "#{@reflection.quoted_table_name}.#{@reflection.options[:as]}_id = #{@owner.quoted_id} AND " +
-                "#{@reflection.klass.table_name}.#{@reflection.options[:as]}_type = #{@owner.class.quote_value(@owner.class.base_class.name.to_s)}"
+                "#{@reflection.quoted_table_name}.#{@reflection.options[:as]}_type = #{@owner.class.quote_value(@owner.class.base_class.name.to_s)}"
               @finder_sql << " AND (#{conditions})" if conditions
             
             else
-              @finder_sql = "#{@reflection.klass.table_name}.#{@reflection.primary_key_name} = #{@owner.quoted_id}"
+              @finder_sql = "#{@reflection.quoted_table_name}.#{@reflection.primary_key_name} = #{@owner.quoted_id}"
               @finder_sql << " AND (#{conditions})" if conditions
           end
 

activerecord/lib/active_record/associations/has_many_through_association.rb

@@ -121,7 +121,7 @@ module ActiveRecord
         column_name, options = @reflection.klass.send(:construct_count_options_from_args, *args)
         if @reflection.options[:uniq]
           # This is needed because 'SELECT count(DISTINCT *)..' is not valid sql statement.
-          column_name = "#{@reflection.klass.table_name}.#{@reflection.klass.primary_key}" if column_name == :all
+          column_name = "#{@reflection.quoted_table_name}.#{@reflection.klass.primary_key}" if column_name == :all
           options.merge!(:distinct => true) 
         end
         @reflection.klass.send(:with_scope, construct_scope) { @reflection.klass.count(column_name, options) } 
@@ -185,7 +185,7 @@ module ActiveRecord
 
         # Build SQL conditions from attributes, qualified by table name.
         def construct_conditions
-          table_name = @reflection.through_reflection.table_name
+          table_name = @reflection.through_reflection.quoted_table_name
           conditions = construct_quoted_owner_attributes(@reflection.through_reflection).map do |attr, value|
             "#{table_name}.#{attr} = #{value}"
           end
@@ -194,11 +194,11 @@ module ActiveRecord
         end
 
         def construct_from
-          @reflection.table_name
+          @reflection.quoted_table_name
         end
 
         def construct_select(custom_select = nil)
-          selected = custom_select || @reflection.options[:select] || "#{@reflection.table_name}.*"
+          selected = custom_select || @reflection.options[:select] || "#{@reflection.quoted_table_name}.*"
         end
 
         def construct_joins(custom_joins = nil)
@@ -208,7 +208,7 @@ module ActiveRecord
             source_primary_key     = @reflection.source_reflection.primary_key_name
             if @reflection.options[:source_type]
               polymorphic_join = "AND %s.%s = %s" % [
-                @reflection.through_reflection.table_name, "#{@reflection.source_reflection.options[:foreign_type]}",
+                @reflection.through_reflection.quoted_table_name, "#{@reflection.source_reflection.options[:foreign_type]}",
                 @owner.class.quote_value(@reflection.options[:source_type])
               ]
             end
@@ -217,7 +217,7 @@ module ActiveRecord
             source_primary_key     = @reflection.klass.primary_key
             if @reflection.source_reflection.options[:as]
               polymorphic_join = "AND %s.%s = %s" % [
-                @reflection.table_name, "#{@reflection.source_reflection.options[:as]}_type",
+                @reflection.quoted_table_name, "#{@reflection.source_reflection.options[:as]}_type",
                 @owner.class.quote_value(@reflection.through_reflection.klass.name)
               ]
             end
@@ -246,7 +246,7 @@ module ActiveRecord
             when @reflection.options[:finder_sql]
               @finder_sql = interpolate_sql(@reflection.options[:finder_sql])
 
-              @finder_sql = "#{@reflection.klass.table_name}.#{@reflection.primary_key_name} = #{@owner.quoted_id}"
+              @finder_sql = "#{@reflection.quoted_table_name}.#{@reflection.primary_key_name} = #{@owner.quoted_id}"
               @finder_sql << " AND (#{conditions})" if conditions
           end
 
@@ -286,7 +286,7 @@ module ActiveRecord
         end
 
         def build_sti_condition
-          "#{@reflection.through_reflection.table_name}.#{@reflection.through_reflection.klass.inheritance_column} = #{@reflection.klass.quote_value(@reflection.through_reflection.klass.name.demodulize)}"
+          "#{@reflection.through_reflection.quoted_table_name}.#{@reflection.through_reflection.klass.inheritance_column} = #{@reflection.klass.quote_value(@reflection.through_reflection.klass.name.demodulize)}"
         end
 
         alias_method :sql_conditions, :conditions

activerecord/lib/active_record/associations/has_one_association.rb

@@ -61,10 +61,10 @@ module ActiveRecord
           case
             when @reflection.options[:as]
               @finder_sql = 
-                "#{@reflection.klass.table_name}.#{@reflection.options[:as]}_id = #{@owner.quoted_id} AND " + 
+                "#{@reflection.quoted_table_name}.#{@reflection.options[:as]}_id = #{@owner.quoted_id} AND " +
-                "#{@reflection.klass.table_name}.#{@reflection.options[:as]}_type = #{@owner.class.quote_value(@owner.class.base_class.name.to_s)}"          
+                "#{@reflection.quoted_table_name}.#{@reflection.options[:as]}_type = #{@owner.class.quote_value(@owner.class.base_class.name.to_s)}"
             else
-              @finder_sql = "#{@reflection.table_name}.#{@reflection.primary_key_name} = #{@owner.quoted_id}"
+              @finder_sql = "#{@reflection.quoted_table_name}.#{@reflection.primary_key_name} = #{@owner.quoted_id}"
           end
           @finder_sql << " AND (#{conditions})" if conditions
         end

activerecord/lib/active_record/base.rb

@@ -677,7 +677,7 @@ module ActiveRecord #:nodoc:
       #   Billing.update_all( "author = 'David'", "title LIKE '%Rails%'",
       #                         :order => 'created_at', :limit => 5 )
       def update_all(updates, conditions = nil, options = {})
-        sql  = "UPDATE #{table_name} SET #{sanitize_sql_for_assignment(updates)} "
+        sql  = "UPDATE #{quoted_table_name} SET #{sanitize_sql_for_assignment(updates)} "
         scope = scope(:find)
         add_conditions!(sql, conditions, scope)
         add_order!(sql, options[:order], nil)

activerecord/lib/active_record/connection_adapters/abstract_adapter.rb

@@ -66,11 +66,16 @@ module ActiveRecord
 
       # QUOTING ==================================================
 
-      # Override to return the quoted table name if the database needs it
+      # Override to return the quoted column name. Defaults to no quoting.
       def quote_table_name(name)
         name
       end
 
+      # Override to return the quoted table name. Defaults to column quoting.
+      def quote_table_name(name)
+        quote_column_name(name)
+      end
+
       # REFERENTIAL INTEGRITY ====================================
 
       # Override to turn off referential integrity while executing +&block+

activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb

@@ -529,8 +529,10 @@ module ActiveRecord
         end
         if pk
           if sequence
+            quoted_sequence = quote_column_name(sequence)
+
             select_value <<-end_sql, 'Reset sequence'
-              SELECT setval('#{sequence}', (SELECT COALESCE(MAX(#{pk})+(SELECT increment_by FROM #{sequence}), (SELECT min_value FROM #{sequence})) FROM #{table}), false)
+              SELECT setval('#{sequence}', (SELECT COALESCE(MAX(#{pk})+(SELECT increment_by FROM #{quoted_sequence}), (SELECT min_value FROM #{quoted_sequence})) FROM #{quote_table_name(table)}), false)
             end_sql
           else
             @logger.warn "#{table} has primary key #{pk} with no default sequence" if @logger
@@ -591,7 +593,7 @@ module ActiveRecord
         notnull = options[:null] == false
 
         # Add the column.
-        execute("ALTER TABLE #{table_name} ADD COLUMN #{quote_column_name(column_name)} #{type_to_sql(type, options[:limit])}")
+        execute("ALTER TABLE #{quote_table_name(table_name)} ADD COLUMN #{quote_column_name(column_name)} #{type_to_sql(type, options[:limit])}")
 
         change_column_default(table_name, column_name, default) if options_include_default?(options)
         change_column_null(table_name, column_name, false, default) if notnull
@@ -599,14 +601,16 @@ module ActiveRecord
 
       # Changes the column of a table.
       def change_column(table_name, column_name, type, options = {})
+        quoted_table_name = quote_table_name(table_name)
+
         begin
-          execute "ALTER TABLE #{table_name} ALTER COLUMN #{quote_column_name(column_name)} TYPE #{type_to_sql(type, options[:limit], options[:precision], options[:scale])}"
+          execute "ALTER TABLE #{quoted_table_name} ALTER COLUMN #{quote_column_name(column_name)} TYPE #{type_to_sql(type, options[:limit], options[:precision], options[:scale])}"
         rescue ActiveRecord::StatementInvalid
           # This is PostgreSQL 7.x, so we have to use a more arcane way of doing it.
           begin_db_transaction
           tmp_column_name = "#{column_name}_ar_tmp"
           add_column(table_name, tmp_column_name, type, options)
-          execute "UPDATE #{table_name} SET #{quote_column_name(tmp_column_name)} = CAST(#{quote_column_name(column_name)} AS #{type_to_sql(type, options[:limit], options[:precision], options[:scale])})"
+          execute "UPDATE #{quoted_table_name} SET #{quote_column_name(tmp_column_name)} = CAST(#{quote_column_name(column_name)} AS #{type_to_sql(type, options[:limit], options[:precision], options[:scale])})"
           remove_column(table_name, column_name)
           rename_column(table_name, tmp_column_name, column_name)
           commit_db_transaction
@@ -618,19 +622,19 @@ module ActiveRecord
 
       # Changes the default value of a table column.
       def change_column_default(table_name, column_name, default)
-        execute "ALTER TABLE #{table_name} ALTER COLUMN #{quote_column_name(column_name)} SET DEFAULT #{quote(default)}"
+        execute "ALTER TABLE #{quote_table_name(table_name)} ALTER COLUMN #{quote_column_name(column_name)} SET DEFAULT #{quote(default)}"
       end
 
       def change_column_null(table_name, column_name, null, default = nil)
         unless null || default.nil?
-          execute("UPDATE #{table_name} SET #{quote_column_name(column_name)}=#{quote(default)} WHERE #{quote_column_name(column_name)} IS NULL")
+          execute("UPDATE #{quote_table_name(table_name)} SET #{quote_column_name(column_name)}=#{quote(default)} WHERE #{quote_column_name(column_name)} IS NULL")
         end
-        execute("ALTER TABLE #{table_name} ALTER #{quote_column_name(column_name)} #{null ? 'DROP' : 'SET'} NOT NULL")
+        execute("ALTER TABLE #{quote_table_name(table_name)} ALTER #{quote_column_name(column_name)} #{null ? 'DROP' : 'SET'} NOT NULL")
       end
 
       # Renames a column in a table.
       def rename_column(table_name, column_name, new_column_name)
-        execute "ALTER TABLE #{table_name} RENAME COLUMN #{quote_column_name(column_name)} TO #{quote_column_name(new_column_name)}"
+        execute "ALTER TABLE #{quote_table_name(table_name)} RENAME COLUMN #{quote_column_name(column_name)} TO #{quote_column_name(new_column_name)}"
       end
 
       # Drops an index from a table.

activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb

@@ -25,7 +25,7 @@ module ActiveRecord
   module ConnectionAdapters #:nodoc:
     class SQLite3Adapter < SQLiteAdapter # :nodoc:
       def table_structure(table_name)
-        returning structure = @connection.table_info(table_name) do
+        returning structure = @connection.table_info(quote_table_name(table_name)) do
           raise(ActiveRecord::StatementInvalid, "Could not find table '#{table_name}'") if structure.empty?
         end
       end

activerecord/lib/active_record/connection_adapters/sqlite_adapter.rb

@@ -192,7 +192,7 @@ module ActiveRecord
       end
 
       def indexes(table_name, name = nil) #:nodoc:
-        execute("PRAGMA index_list(#{table_name})", name).map do |row|
+        execute("PRAGMA index_list(#{quote_table_name(table_name)})", name).map do |row|
           index = IndexDefinition.new(table_name, row['name'])
           index.unique = row['unique'] != '0'
           index.columns = execute("PRAGMA index_info('#{index.name}')").map { |col| col['name'] }
@@ -265,7 +265,7 @@ module ActiveRecord
         end
 
         def table_structure(table_name)
-          returning structure = execute("PRAGMA table_info(#{table_name})") do
+          returning structure = execute("PRAGMA table_info(#{quote_table_name(table_name)})") do
             raise(ActiveRecord::StatementInvalid, "Could not find table '#{table_name}'") if structure.empty?
           end
         end
@@ -340,8 +340,9 @@ module ActiveRecord
           columns = columns.find_all{|col| from_columns.include?(column_mappings[col])}
           quoted_columns = columns.map { |col| quote_column_name(col) } * ','
 
-          @connection.execute "SELECT * FROM #{from}" do |row|
+          quoted_to = quote_table_name(to)
-            sql = "INSERT INTO #{to} (#{quoted_columns}) VALUES ("
+          @connection.execute "SELECT * FROM #{quote_table_name(from)}" do |row|
+            sql = "INSERT INTO #{quoted_to} (#{quoted_columns}) VALUES ("
             sql << columns.map {|col| quote row[column_mappings[col]]} * ', '
             sql << ')'
             @connection.execute sql

activerecord/lib/active_record/reflection.rb

@@ -129,6 +129,10 @@ module ActiveRecord
         @table_name ||= klass.table_name
       end
 
+      def quoted_table_name
+        @quoted_table_name ||= klass.quoted_table_name
+      end
+
       def primary_key_name
         @primary_key_name ||= options[:foreign_key] || derive_primary_key_name
       end

activerecord/lib/active_record/validations.rb

@@ -653,23 +653,23 @@ module ActiveRecord
 
         validates_each(attr_names,configuration) do |record, attr_name, value|
           if value.nil? || (configuration[:case_sensitive] || !columns_hash[attr_name.to_s].text?)
-            condition_sql = "#{record.class.table_name}.#{attr_name} #{attribute_condition(value)}"
+            condition_sql = "#{record.class.quoted_table_name}.#{attr_name} #{attribute_condition(value)}"
             condition_params = [value]
           else
-            condition_sql = "LOWER(#{record.class.table_name}.#{attr_name}) #{attribute_condition(value)}"
+            condition_sql = "LOWER(#{record.class.quoted_table_name}.#{attr_name}) #{attribute_condition(value)}"
             condition_params = [value.downcase]
           end
 
           if scope = configuration[:scope]
             Array(scope).map do |scope_item|
               scope_value = record.send(scope_item)
-              condition_sql << " AND #{record.class.table_name}.#{scope_item} #{attribute_condition(scope_value)}"
+              condition_sql << " AND #{record.class.quoted_table_name}.#{scope_item} #{attribute_condition(scope_value)}"
               condition_params << scope_value
             end
           end
 
           unless record.new_record?
-            condition_sql << " AND #{record.class.table_name}.#{record.class.primary_key} <> ?"
+            condition_sql << " AND #{record.class.quoted_table_name}.#{record.class.primary_key} <> ?"
             condition_params << record.send(:id)
           end
 

activerecord/test/associations/inner_join_association_test.rb

@@ -10,30 +10,30 @@ class InnerJoinAssociationTest < ActiveSupport::TestCase
 
   def test_construct_finder_sql_creates_inner_joins
     sql = Author.send(:construct_finder_sql, :joins => :posts)
-    assert_match /INNER JOIN `?posts`? ON `?posts`?.author_id = authors.id/, sql
+    assert_match /INNER JOIN .?posts.? ON .?posts.?.author_id = authors.id/, sql
   end
   
   def test_construct_finder_sql_cascades_inner_joins
     sql = Author.send(:construct_finder_sql, :joins => {:posts => :comments})
-    assert_match /INNER JOIN `?posts`? ON `?posts`?.author_id = authors.id/, sql
+    assert_match /INNER JOIN .?posts.? ON .?posts.?.author_id = authors.id/, sql
-    assert_match /INNER JOIN `?comments`? ON `?comments`?.post_id = posts.id/, sql
+    assert_match /INNER JOIN .?comments.? ON .?comments.?.post_id = posts.id/, sql
   end
   
   def test_construct_finder_sql_inner_joins_through_associations
     sql = Author.send(:construct_finder_sql, :joins => :categorized_posts)
-    assert_match /INNER JOIN `?categorizations`?.*INNER JOIN `?posts`?/, sql
+    assert_match /INNER JOIN .?categorizations.?.*INNER JOIN .?posts.?/, sql
   end
   
   def test_construct_finder_sql_applies_association_conditions
     sql = Author.send(:construct_finder_sql, :joins => :categories_like_general, :conditions => "TERMINATING_MARKER")
-    assert_match /INNER JOIN `?categories`? ON.*AND.*`?General`?.*TERMINATING_MARKER/, sql
+    assert_match /INNER JOIN .?categories.? ON.*AND.*.?General.?.*TERMINATING_MARKER/, sql
   end
 
   def test_construct_finder_sql_unpacks_nested_joins
     sql = Author.send(:construct_finder_sql, :joins => {:posts => [[:comments]]})
     assert_no_match /inner join.*inner join.*inner join/i, sql, "only two join clauses should be present"
-    assert_match /INNER JOIN `?posts`? ON `?posts`?.author_id = authors.id/, sql
+    assert_match /INNER JOIN .?posts.? ON .?posts.?.author_id = authors.id/, sql
-    assert_match /INNER JOIN `?comments`? ON `?comments`?.post_id = `?posts`?.id/, sql
+    assert_match /INNER JOIN .?comments.? ON .?comments.?.post_id = .?posts.?.id/, sql
   end
 
   def test_construct_finder_sql_ignores_empty_joins_hash

activerecord/test/base_test.rb

@@ -12,6 +12,7 @@ require 'fixtures/subscriber'
 require 'fixtures/keyboard'
 require 'fixtures/post'
 require 'fixtures/minimalistic'
+require 'fixtures/warehouse_thing'
 require 'rexml/document'
 
 class Category < ActiveRecord::Base; end
@@ -71,7 +72,7 @@ class TopicWithProtectedContentAndAccessibleAuthorName < ActiveRecord::Base
 end
 
 class BasicsTest < ActiveSupport::TestCase
-  fixtures :topics, :companies, :developers, :projects, :computers, :accounts, :minimalistics
+  fixtures :topics, :companies, :developers, :projects, :computers, :accounts, :minimalistics, 'warehouse-things'
 
   def test_table_exists
     assert !NonExistentTable.table_exists?
@@ -590,6 +591,11 @@ class BasicsTest < ActiveSupport::TestCase
     assert_nil Topic.find(2).last_read
   end
 
+  def test_update_all_with_non_standard_table_name
+    assert_equal 1, WarehouseThing.update_all(['value = ?', 0], ['id = ?', 1])
+    assert_equal 0, WarehouseThing.find(1).value
+  end
+
   if current_adapter?(:MysqlAdapter)
     def test_update_all_with_order_and_limit
       assert_equal 1, Topic.update_all("content = 'bulk updated!'", nil, :limit => 1, :order => 'id DESC')

activerecord/test/fixtures/db_definitions/schema.rb

@@ -351,4 +351,8 @@ ActiveRecord::Schema.define do
     t.datetime :updated_at
     t.datetime :updated_on
   end
+
+  create_table 'warehouse-things', :force => true do |t|
+    t.integer :value
+  end
 end

activerecord/test/fixtures/warehouse-things.yml

@@ -0,0 +1,3 @@
+one:
+  id: 1
+  value: 1000

activerecord/test/fixtures/warehouse_thing.rb

@@ -0,0 +1,5 @@
+class WarehouseThing < ActiveRecord::Base
+  set_table_name "warehouse-things"
+
+  validates_uniqueness_of :value
+end

activerecord/test/validations_test.rb

@@ -3,6 +3,7 @@ require 'fixtures/topic'
 require 'fixtures/reply'
 require 'fixtures/person'
 require 'fixtures/developer'
+require 'fixtures/warehouse_thing'
 
 # The following methods in Topic are used in test_conditional_validation_*
 class Topic
@@ -54,7 +55,7 @@ class Thaumaturgist < IneptWizard
 end
 
 class ValidationsTest < ActiveSupport::TestCase
-  fixtures :topics, :developers
+  fixtures :topics, :developers, 'warehouse-things'
 
   def setup
     Topic.write_inheritable_attribute(:validate, nil)
@@ -435,6 +436,13 @@ class ValidationsTest < ActiveSupport::TestCase
     assert t2.save, "should save with nil"
   end
 
+  def test_validate_uniqueness_with_non_standard_table_names
+    i1 = WarehouseThing.create(:value => 1000)
+    assert !i1.valid?, "i1 should not be valid"
+    assert i1.errors.on(:value), "Should not be empty"
+  end
+
+
   def test_validate_straight_inheritance_uniqueness
     w1 = IneptWizard.create(:name => "Rincewind", :city => "Ankh-Morpork")
     assert w1.valid?, "Saving w1"