Browse files

More thoroughly quote table names. Exposes some issues with sqlite2 a…

…dapter. Closes #10698.

git-svn-id: http://svn-commit.rubyonrails.org/rails/trunk@8571 5ecf4fe2-1ee6-0310-87b1-e25e094e27de
  • Loading branch information...
1 parent 139b924 commit ebe3a0d532152d47f270ffaa0e11b994f4a8b177 @jeremy jeremy committed Jan 5, 2008
View
2 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]
View
8 activerecord/lib/active_record/associations/has_and_belongs_to_many_association.rb
@@ -100,7 +100,7 @@ def insert_record(record, force=true)
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 @@ def delete_records(records)
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 @@ def construct_sql
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
View
6 activerecord/lib/active_record/associations/has_many_association.rb
@@ -144,12 +144,12 @@ def construct_sql
when @reflection.options[:as]
@finder_sql =
- "#{@reflection.klass.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]}_id = #{@owner.quoted_id} AND " +
+ "#{@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
View
16 activerecord/lib/active_record/associations/has_many_through_association.rb
@@ -121,7 +121,7 @@ def count(*args)
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 @@ def construct_quoted_owner_attributes(reflection)
# 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 @@ def construct_conditions
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 @@ def construct_joins(custom_joins = nil)
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 @@ def construct_joins(custom_joins = nil)
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 @@ def construct_sql
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 @@ def build_conditions
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
View
6 activerecord/lib/active_record/associations/has_one_association.rb
@@ -61,10 +61,10 @@ def construct_sql
case
when @reflection.options[:as]
@finder_sql =
- "#{@reflection.klass.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]}_id = #{@owner.quoted_id} AND " +
+ "#{@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
View
2 activerecord/lib/active_record/base.rb
@@ -677,7 +677,7 @@ def destroy(id)
# 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)
View
7 activerecord/lib/active_record/connection_adapters/abstract_adapter.rb
@@ -66,11 +66,16 @@ def reset_runtime #:nodoc:
# 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+
View
20 activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb
@@ -529,8 +529,10 @@ def reset_pk_sequence!(table, pk = nil, sequence = nil) #:nodoc:
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,22 +593,24 @@ def add_column(table_name, column_name, type, options = {})
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
end
# 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 @@ def change_column(table_name, column_name, type, options = {})
# 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.
View
2 activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb
@@ -25,7 +25,7 @@ def self.sqlite3_connection(config) # :nodoc:
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
View
9 activerecord/lib/active_record/connection_adapters/sqlite_adapter.rb
@@ -192,7 +192,7 @@ def columns(table_name, name = nil) #:nodoc:
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 @@ def select(sql, name = nil) #:nodoc:
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 @@ def copy_table_contents(from, to, columns, rename = {}) #:nodoc:
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|
- sql = "INSERT INTO #{to} (#{quoted_columns}) VALUES ("
+ quoted_to = quote_table_name(to)
+ @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
View
4 activerecord/lib/active_record/reflection.rb
@@ -129,6 +129,10 @@ def table_name
@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
View
8 activerecord/lib/active_record/validations.rb
@@ -653,23 +653,23 @@ def validates_uniqueness_of(*attr_names)
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
View
14 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 `?comments`? ON `?comments`?.post_id = posts.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
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 `?comments`? ON `?comments`?.post_id = `?posts`?.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
end
def test_construct_finder_sql_ignores_empty_joins_hash
View
8 activerecord/test/base_test.rb
@@ -12,6 +12,7 @@
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 @@ def test_update_all_with_hash
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')
View
4 activerecord/test/fixtures/db_definitions/schema.rb
@@ -351,4 +351,8 @@ def create_table(*args, &block)
t.datetime :updated_at
t.datetime :updated_on
end
+
+ create_table 'warehouse-things', :force => true do |t|
+ t.integer :value
+ end
end
View
3 activerecord/test/fixtures/warehouse-things.yml
@@ -0,0 +1,3 @@
+one:
+ id: 1
+ value: 1000
View
5 activerecord/test/fixtures/warehouse_thing.rb
@@ -0,0 +1,5 @@
+class WarehouseThing < ActiveRecord::Base
+ set_table_name "warehouse-things"
+
+ validates_uniqueness_of :value
+end
View
10 activerecord/test/validations_test.rb
@@ -3,6 +3,7 @@
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 @@ def test_validate_case_insensitive_uniqueness
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"

0 comments on commit ebe3a0d

Please sign in to comment.