Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse files

remove table quoting in primary_key method

* add/cleanup tests
  • Loading branch information...
commit 1d7c751bf703c729887e2d8a9ae104a8e6aef010 1 parent 019c263
Paul Gallagher tardate authored
2  activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb
View
@@ -864,7 +864,7 @@ def pk_and_sequence_for(table) #:nodoc:
# Returns just a table's primary key
def primary_key(table)
- row = exec_query(<<-end_sql, 'SCHEMA', [[nil, quote_table_name(table)]]).rows.first
Ivan Schneider
isc added a note

Why has quoting been removed here ? This causes the method to fail for tables with a name containing one or more capital letters.

Paul Gallagher
tardate added a note

it's been a while since I looked at this (and I'm actually unsure whether this code made it into a release - need to check), but I remember the main reason was that by applying quoting within the pk method, it was breaking the ability to supply schema-qualified table names. i.e. :primary_key is assuming quoting has been applied if necessary on entry .. and this is what is done when the method is used internally.

Have you found some rails core that now breaks because of this? When I last looked at the code it was all green.

Ivan Schneider
isc added a note

This code is present in the 3.1 and 3.2 branches.
It breaks when primary_key is called to fill the @primary_keys cache hash.
It's in connection_pool.rb for 3.1 (https://github.com/rails/rails/blob/3-1-stable/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb#L119) and schema_cache.rb for 3.2 (https://github.com/rails/rails/blob/3-2-stable/activerecord/lib/active_record/connection_adapters/schema_cache.rb#L22).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ row = exec_query(<<-end_sql, 'SCHEMA', [[nil, table]]).rows.first
SELECT DISTINCT(attr.attname)
FROM pg_attribute attr
INNER JOIN pg_depend dep ON attr.attrelid = dep.refobjid AND attr.attnum = dep.refobjsubid
30 activerecord/test/cases/adapters/postgresql/postgresql_adapter_test.rb
View
@@ -74,6 +74,36 @@ def test_default_sequence_name_bad_table
@connection.default_sequence_name('zomg')
end
+ def test_pk_and_sequence_for
+ pk, seq = @connection.pk_and_sequence_for('ex')
+ assert_equal 'id', pk
+ assert_equal @connection.default_sequence_name('ex', 'id'), seq
+ end
+
+ def test_pk_and_sequence_for_with_non_standard_primary_key
+ @connection.exec_query('drop table if exists ex')
+ @connection.exec_query('create table ex(code serial primary key)')
+ pk, seq = @connection.pk_and_sequence_for('ex')
+ assert_equal 'code', pk
+ assert_equal @connection.default_sequence_name('ex', 'code'), seq
+ end
+
+ def test_pk_and_sequence_for_returns_nil_if_no_seq
+ @connection.exec_query('drop table if exists ex')
+ @connection.exec_query('create table ex(id integer primary key)')
+ assert_nil @connection.pk_and_sequence_for('ex')
+ end
+
+ def test_pk_and_sequence_for_returns_nil_if_no_pk
+ @connection.exec_query('drop table if exists ex')
+ @connection.exec_query('create table ex(id integer)')
+ assert_nil @connection.pk_and_sequence_for('ex')
+ end
+
+ def test_pk_and_sequence_for_returns_nil_if_table_not_found
+ assert_nil @connection.pk_and_sequence_for('unobtainium')
+ end
+
def test_exec_insert_number
insert(@connection, 'number' => 10)
22 activerecord/test/cases/adapters/postgresql/schema_test.rb
View
@@ -38,10 +38,6 @@ class Thing4 < ActiveRecord::Base
set_table_name 'test_schema."Things"'
end
- class PrimaryKeyTestHarness < ActiveRecord::Base
- set_table_name 'test_schema.pktest'
- end
-
def setup
@connection = ActiveRecord::Base.connection
@connection.execute "CREATE SCHEMA #{SCHEMA_NAME} CREATE TABLE #{TABLE_NAME} (#{COLUMNS.join(',')})"
@@ -189,7 +185,11 @@ def test_with_uppercase_index_name
end
def test_primary_key_with_schema_specified
- [ %("#{SCHEMA_NAME}"."#{PK_TABLE_NAME}"), %(#{SCHEMA_NAME}."#{PK_TABLE_NAME}"), %(#{SCHEMA_NAME}."#{PK_TABLE_NAME}")].each do |given|
+ [
+ %("#{SCHEMA_NAME}"."#{PK_TABLE_NAME}"),
+ %(#{SCHEMA_NAME}."#{PK_TABLE_NAME}"),
+ %(#{SCHEMA_NAME}.#{PK_TABLE_NAME})
+ ].each do |given|
assert_equal 'id', @connection.primary_key(given), "primary key should be found when table referenced as #{given}"
end
end
@@ -208,6 +208,18 @@ def test_primary_key_raises_error_if_table_not_found_on_schema_search_path
end
end
+ def test_pk_and_sequence_for_with_schema_specified
+ [
+ %("#{SCHEMA_NAME}"."#{PK_TABLE_NAME}"),
+ %(#{SCHEMA_NAME}."#{PK_TABLE_NAME}"),
+ %(#{SCHEMA_NAME}.#{PK_TABLE_NAME})
+ ].each do |given|
+ pk, seq = @connection.pk_and_sequence_for(given)
+ assert_equal 'id', pk, "primary key should be found when table referenced as #{given}"
+ assert_equal "#{SCHEMA_NAME}.#{PK_TABLE_NAME}_id_seq", seq, "sequence name should be found when table referenced as #{given}"
+ end
+ end
+
def test_extract_schema_and_table
{
%(table_name) => [nil,'table_name'],
Ivan Schneider

Why has quoting been removed here ? This causes the method to fail for tables with a name containing one or more capital letters.

Paul Gallagher

it's been a while since I looked at this (and I'm actually unsure whether this code made it into a release - need to check), but I remember the main reason was that by applying quoting within the pk method, it was breaking the ability to supply schema-qualified table names. i.e. :primary_key is assuming quoting has been applied if necessary on entry .. and this is what is done when the method is used internally.

Have you found some rails core that now breaks because of this? When I last looked at the code it was all green.

Please sign in to comment.
Something went wrong with that request. Please try again.