From 60f36e8995582c76c94e7f68fd5965e4a1c7886e Mon Sep 17 00:00:00 2001 From: Alex Pott Date: Sat, 28 Feb 2015 16:43:46 +0000 Subject: [PATCH] Issue #1600670 by mradcliffe, bendiy, bzrudi71, andypost, daffie, stefan.r, devpreview: Cannot query Postgres database that has column names with capital letters --- .../Core/Database/Driver/pgsql/Connection.php | 58 +++++++++ .../Core/Database/Driver/pgsql/Schema.php | 3 +- .../Core/Database/Driver/pgsql/Select.php | 27 ++++- .../system/src/Tests/Database/SelectTest.php | 8 +- .../src/Tests/Database/UpdateComplexTest.php | 9 +- .../Driver/pgsql/PostgresqlConnectionTest.php | 112 ++++++++++++++++++ 6 files changed, 205 insertions(+), 12 deletions(-) create mode 100644 core/tests/Drupal/Tests/Core/Database/Driver/pgsql/PostgresqlConnectionTest.php diff --git a/core/lib/Drupal/Core/Database/Driver/pgsql/Connection.php b/core/lib/Drupal/Core/Database/Driver/pgsql/Connection.php index cacae24efca..b888f14a676 100644 --- a/core/lib/Drupal/Core/Database/Driver/pgsql/Connection.php +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Connection.php @@ -186,6 +186,64 @@ class Connection extends DatabaseConnection { return $tablename; } + /** + * {@inheritdoc} + */ + public function escapeField($field) { + $escaped = parent::escapeField($field); + + // Remove any invalid start character. + $escaped = preg_replace('/^[^A-Za-z0-9_]/', '', $escaped); + + // The pgsql database driver does not support field names that contain + // periods (supported by PostgreSQL server) because this method may be + // called by a field with a table alias as part of SQL conditions or + // order by statements. This will consider a period as a table alias + // identifier, and split the string at the first period. + if (preg_match('/^([A-Za-z0-9_]+)"?[.]"?([A-Za-z0-9_.]+)/', $escaped, $parts)) { + $table = $parts[1]; + $column = $parts[2]; + + // Use escape alias because escapeField may contain multiple periods that + // need to be escaped. + $escaped = $this->escapeTable($table) . '.' . $this->escapeAlias($column); + } + elseif (preg_match('/[A-Z]/', $escaped)) { + // Quote the field name for case-sensitivity. + $escaped = '"' . $escaped . '"'; + } + + return $escaped; + } + + /** + * {@inheritdoc} + */ + public function escapeAlias($field) { + $escaped = preg_replace('/[^A-Za-z0-9_]+/', '', $field); + + // Escape the alias in quotes for case-sensitivity. + if (preg_match('/[A-Z]/', $escaped)) { + $escaped = '"' . $escaped . '"'; + } + + return $escaped; + } + + /** + * {@inheritdoc} + */ + public function escapeTable($table) { + $escaped = parent::escapeTable($table); + + // Quote identifier to make it case-sensitive. + if (preg_match('/[A-Z]/', $escaped)) { + $escaped = '"' . $escaped . '"'; + } + + return $escaped; + } + public function driver() { return 'pgsql'; } diff --git a/core/lib/Drupal/Core/Database/Driver/pgsql/Schema.php b/core/lib/Drupal/Core/Database/Driver/pgsql/Schema.php index 8fe59d15bcb..d4a96b3a49a 100644 --- a/core/lib/Drupal/Core/Database/Driver/pgsql/Schema.php +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Schema.php @@ -262,7 +262,8 @@ class Schema extends DatabaseSchema { * The field specification, as per the schema data structure format. */ protected function createFieldSql($name, $spec) { - $sql = $name . ' ' . $spec['pgsql_type']; + // The PostgreSQL server converts names into lowercase, unless quoted. + $sql = '"' . $name . '" ' . $spec['pgsql_type']; if (isset($spec['type']) && $spec['type'] == 'serial') { unset($spec['not null']); diff --git a/core/lib/Drupal/Core/Database/Driver/pgsql/Select.php b/core/lib/Drupal/Core/Database/Driver/pgsql/Select.php index de9409f03cf..8c63044dd6f 100644 --- a/core/lib/Drupal/Core/Database/Driver/pgsql/Select.php +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Select.php @@ -82,7 +82,7 @@ class Select extends QuerySelect { // Also check expression aliases. foreach ($this->expressions as $expression) { - if ($expression['alias'] == $field) { + if ($expression['alias'] == $this->connection->escapeAlias($field)) { return $return; } } @@ -109,6 +109,31 @@ class Select extends QuerySelect { return $return; } + /** + * {@inheritdoc} + */ + public function addExpression($expression, $alias = NULL, $arguments = array()) { + if (empty($alias)) { + $alias = 'expression'; + } + + // This implements counting in the same manner as the parent method. + $alias_candidate = $alias; + $count = 2; + while (!empty($this->expressions[$alias_candidate])) { + $alias_candidate = $alias . '_' . $count++; + } + $alias = $alias_candidate; + + $this->expressions[$alias] = array( + 'expression' => $expression, + 'alias' => $this->connection->escapeAlias($alias_candidate), + 'arguments' => $arguments, + ); + + return $alias; + } + /** * {@inheritdoc} */ diff --git a/core/modules/system/src/Tests/Database/SelectTest.php b/core/modules/system/src/Tests/Database/SelectTest.php index 26dd1094559..d92c3a8ed3c 100644 --- a/core/modules/system/src/Tests/Database/SelectTest.php +++ b/core/modules/system/src/Tests/Database/SelectTest.php @@ -39,10 +39,10 @@ class SelectTest extends DatabaseTestBase { $records = $result->fetchAll(); $query = (string) $query; - $expected = "/* Testing query comments */ SELECT test.name AS name, test.age AS age\nFROM \n{test} test"; + $expected = "/* Testing query comments */"; $this->assertEqual(count($records), 4, 'Returned the correct number of rows.'); - $this->assertEqual($query, $expected, 'The flattened query contains the comment string.'); + $this->assertNotIdentical(FALSE, strpos($query, $expected), 'The flattened query contains the comment string.'); } /** @@ -57,10 +57,10 @@ class SelectTest extends DatabaseTestBase { $records = $result->fetchAll(); $query = (string) $query; - $expected = "/* Testing query comments SELECT nid FROM {node}; -- */ SELECT test.name AS name, test.age AS age\nFROM \n{test} test"; + $expected = "/* Testing query comments SELECT nid FROM {node}; -- */"; $this->assertEqual(count($records), 4, 'Returned the correct number of rows.'); - $this->assertEqual($query, $expected, 'The flattened query contains the sanitised comment string.'); + $this->assertNotIdentical(FALSE, strpos($query, $expected), 'The flattened query contains the sanitised comment string.'); } /** diff --git a/core/modules/system/src/Tests/Database/UpdateComplexTest.php b/core/modules/system/src/Tests/Database/UpdateComplexTest.php index 19eff495024..abe27302d7d 100644 --- a/core/modules/system/src/Tests/Database/UpdateComplexTest.php +++ b/core/modules/system/src/Tests/Database/UpdateComplexTest.php @@ -139,15 +139,12 @@ class UpdateComplexTest extends DatabaseTestBase { $query = db_update('test') ->expression('age', $subselect) ->condition('name', 'Ringo'); - // Save the query for a __toString test later. - $string_test = $query; - $query->execute(); + // Save the number of rows that updated for assertion later. + $num_updated = $query->execute(); $after_age = db_query('SELECT age FROM {test} WHERE name = :name', array(':name' => 'Ringo'))->fetchField(); $expected_age = $select->execute()->fetchField(); $this->assertEqual($after_age, $expected_age); - // Replace whitespace with a single space. - $query_string = preg_replace('/\s+/', ' ', $string_test); - $this->assertIdentical('UPDATE {test} SET age= (SELECT MAX(priority) + :increment AS max_priority FROM {test_task} t) WHERE (name = :db_condition_placeholder_0)', trim($query_string)); + $this->assertEqual(1, $num_updated, t('Expected 1 row to be updated in subselect update query.')); } } diff --git a/core/tests/Drupal/Tests/Core/Database/Driver/pgsql/PostgresqlConnectionTest.php b/core/tests/Drupal/Tests/Core/Database/Driver/pgsql/PostgresqlConnectionTest.php new file mode 100644 index 00000000000..57e38d02d2e --- /dev/null +++ b/core/tests/Drupal/Tests/Core/Database/Driver/pgsql/PostgresqlConnectionTest.php @@ -0,0 +1,112 @@ +mock_pdo = $this->getMock('Drupal\Tests\Core\Database\Stub\StubPDO'); + } + + /** + * Data provider for testEscapeTable. + * + * @return [] + * An indexed array of where each value is an array of arguments to pass to + * testEscapeField. The first value is the expected value, and the second + * value is the value to test. + */ + public function providerEscapeTables() { + return array( + array('nocase', 'nocase'), + array('"camelCase"', 'camelCase'), + array('"camelCase"', '"camelCase"'), + array('"camelCase"', 'camel/Case'), + ); + } + + /** + * Data provider for testEscapeAlias. + * + * @return [] + * Array of arrays with the following elements: + * - Expected escaped string. + * - String to escape. + */ + public function providerEscapeAlias() { + return array( + array('nocase', 'nocase'), + array('"camelCase"', '"camelCase"'), + array('"camelCase"', 'camelCase'), + array('"camelCase"', 'camel.Case'), + ); + } + + /** + * Data provider for testEscapeField. + * + * @return [] + * Array of arrays with the following elements: + * - Expected escaped string. + * - String to escape. + */ + public function providerEscapeFields() { + return array( + array('title', 'title'), + array('"isDefaultRevision"', 'isDefaultRevision'), + array('"isDefaultRevision"', '"isDefaultRevision"'), + array('entity_test."isDefaultRevision"', 'entity_test.isDefaultRevision'), + array('entity_test."isDefaultRevision"', '"entity_test"."isDefaultRevision"'), + array('"entityTest"."isDefaultRevision"', '"entityTest"."isDefaultRevision"'), + array('"entityTest"."isDefaultRevision"', 'entityTest.isDefaultRevision'), + array('entity_test."isDefaultRevision"', 'entity_test.is.Default.Revision'), + ); + } + + /** + * @covers ::escapeTable + * @dataProvider providerEscapeTables + */ + public function testEscapeTable($expected, $name) { + $pgsql_connection = new Connection($this->mock_pdo, array()); + + $this->assertEquals($expected, $pgsql_connection->escapeTable($name)); + } + + /** + * @covers ::escapeAlias + * @dataProvider providerEscapeAlias + */ + public function testEscapeAlias($expected, $name) { + $pgsql_connection = new Connection($this->mock_pdo, array()); + + $this->assertEquals($expected, $pgsql_connection->escapeAlias($name)); + } + + /** + * @covers ::escapeField + * @dataProvider providerEscapeFields + */ + public function testEscapeField($expected, $name) { + $pgsql_connection = new Connection($this->mock_pdo, array()); + + $this->assertEquals($expected, $pgsql_connection->escapeField($name)); + } + +}