From bf5168fa6cbfab9b10890eb91ef8a2e0ac35fe7a Mon Sep 17 00:00:00 2001 From: Sergei Morozov Date: Sat, 1 Feb 2025 13:58:43 -0800 Subject: [PATCH 1/2] Fix introspection of table options in non-default schemas --- src/Platforms/SQLServerPlatform.php | 9 +++- src/Schema/PostgreSQLSchemaManager.php | 10 ++++- src/Schema/SQLServerSchemaManager.php | 26 ++++++----- .../Schema/PostgreSQLSchemaManagerTest.php | 39 ----------------- .../SchemaManagerFunctionalTestCase.php | 43 +++++++++++++++++++ 5 files changed, 74 insertions(+), 53 deletions(-) diff --git a/src/Platforms/SQLServerPlatform.php b/src/Platforms/SQLServerPlatform.php index 16d7bd4e73d..eed1cbab4d8 100644 --- a/src/Platforms/SQLServerPlatform.php +++ b/src/Platforms/SQLServerPlatform.php @@ -38,6 +38,7 @@ use function preg_match; use function preg_match_all; use function sprintf; +use function str_contains; use function str_ends_with; use function str_replace; use function str_starts_with; @@ -1777,11 +1778,17 @@ private function generateIdentifierName($identifier): string protected function getCommentOnTableSQL(string $tableName, ?string $comment): string { + if (str_contains($tableName, '.')) { + [$schemaName, $tableName] = explode('.', $tableName); + } else { + $schemaName = 'dbo'; + } + return $this->getAddExtendedPropertySQL( 'MS_Description', $comment, 'SCHEMA', - $this->quoteStringLiteral('dbo'), + $this->quoteStringLiteral($schemaName), 'TABLE', $this->quoteStringLiteral($this->unquoteSingleIdentifier($tableName)), ); diff --git a/src/Schema/PostgreSQLSchemaManager.php b/src/Schema/PostgreSQLSchemaManager.php index 1716249fc14..8346d175c5d 100644 --- a/src/Schema/PostgreSQLSchemaManager.php +++ b/src/Schema/PostgreSQLSchemaManager.php @@ -726,7 +726,8 @@ protected function selectForeignKeyColumns(string $databaseName, ?string $tableN protected function fetchTableOptionsByTable(string $databaseName, ?string $tableName = null): array { $sql = <<<'SQL' -SELECT c.relname, +SELECT n.nspname AS schema_name, + c.relname AS table_name, CASE c.relpersistence WHEN 'u' THEN true ELSE false END as unlogged, obj_description(c.oid, 'pg_class') AS comment FROM pg_class c @@ -738,7 +739,12 @@ protected function fetchTableOptionsByTable(string $databaseName, ?string $table $sql .= ' WHERE ' . implode(' AND ', $conditions); - return $this->_conn->fetchAllAssociativeIndexed($sql); + $tableOptions = []; + foreach ($this->_conn->iterateAssociative($sql) as $row) { + $tableOptions[$this->_getPortableTableDefinition($row)] = $row; + } + + return $tableOptions; } /** diff --git a/src/Schema/SQLServerSchemaManager.php b/src/Schema/SQLServerSchemaManager.php index acef511abea..a46e552147d 100644 --- a/src/Schema/SQLServerSchemaManager.php +++ b/src/Schema/SQLServerSchemaManager.php @@ -227,9 +227,15 @@ protected function _getPortableTableForeignKeysList($tableForeignKeys) $name = $tableForeignKey['ForeignKey']; if (! isset($foreignKeys[$name])) { + $referencedTableName = $tableForeignKey['ReferenceTableName']; + + if ($tableForeignKey['ReferenceSchemaName'] !== 'dbo') { + $referencedTableName = $tableForeignKey['ReferenceSchemaName'] . '.' . $referencedTableName; + } + $foreignKeys[$name] = [ 'local_columns' => [$tableForeignKey['ColumnName']], - 'foreign_table' => $tableForeignKey['ReferenceTableName'], + 'foreign_table' => $referencedTableName, 'foreign_columns' => [$tableForeignKey['ReferenceColumnName']], 'name' => $name, 'options' => [ @@ -556,31 +562,29 @@ protected function fetchTableOptionsByTable(string $databaseName, ?string $table { $sql = <<<'SQL' SELECT - tbl.name, + scm.name AS schema_name, + tbl.name AS table_name, p.value AS [table_comment] FROM sys.tables AS tbl + JOIN sys.schemas AS scm + ON tbl.schema_id = scm.schema_id INNER JOIN sys.extended_properties AS p ON p.major_id=tbl.object_id AND p.minor_id=0 AND p.class=1 SQL; - $conditions = ["SCHEMA_NAME(tbl.schema_id) = N'dbo'", "p.name = N'MS_Description'"]; - $params = []; + $conditions = ["p.name = N'MS_Description'"]; if ($tableName !== null) { - $conditions[] = "tbl.name = N'" . $tableName . "'"; + $conditions[] = $this->getTableWhereClause($tableName, 'scm.name', 'tbl.name'); } $sql .= ' WHERE ' . implode(' AND ', $conditions); - /** @var array> $metadata */ - $metadata = $this->_conn->executeQuery($sql, $params) - ->fetchAllAssociativeIndexed(); - $tableOptions = []; - foreach ($metadata as $table => $data) { + foreach ($this->_conn->iterateAssociative($sql) as $data) { $data = array_change_key_case($data, CASE_LOWER); - $tableOptions[$table] = [ + $tableOptions[$this->_getPortableTableDefinition($data)] = [ 'comment' => $data['table_comment'], ]; } diff --git a/tests/Functional/Schema/PostgreSQLSchemaManagerTest.php b/tests/Functional/Schema/PostgreSQLSchemaManagerTest.php index a98bf4d08b3..2695b0a31c6 100644 --- a/tests/Functional/Schema/PostgreSQLSchemaManagerTest.php +++ b/tests/Functional/Schema/PostgreSQLSchemaManagerTest.php @@ -25,7 +25,6 @@ use function array_map; use function array_merge; -use function array_pop; use function array_unshift; use function assert; use function count; @@ -163,44 +162,6 @@ public function testAlterTableAutoIncrementDrop(callable $comparatorFactory): vo self::assertFalse($tableFinal->getColumn('id')->getAutoincrement()); } - public function testTableWithSchema(): void - { - $this->connection->executeStatement('CREATE SCHEMA nested'); - - $nestedRelatedTable = new Table('nested.schemarelated'); - $column = $nestedRelatedTable->addColumn('id', Types::INTEGER); - $column->setAutoincrement(true); - $nestedRelatedTable->setPrimaryKey(['id']); - - $nestedSchemaTable = new Table('nested.schematable'); - $column = $nestedSchemaTable->addColumn('id', Types::INTEGER); - $column->setAutoincrement(true); - $nestedSchemaTable->setPrimaryKey(['id']); - $nestedSchemaTable->addForeignKeyConstraint($nestedRelatedTable, ['id'], ['id']); - - $this->schemaManager->createTable($nestedRelatedTable); - $this->schemaManager->createTable($nestedSchemaTable); - - $tableNames = $this->schemaManager->listTableNames(); - self::assertContains('nested.schematable', $tableNames); - - $tables = $this->schemaManager->listTables(); - self::assertNotNull($this->findTableByName($tables, 'nested.schematable')); - - $nestedSchemaTable = $this->schemaManager->introspectTable('nested.schematable'); - self::assertTrue($nestedSchemaTable->hasColumn('id')); - - $primaryKey = $nestedSchemaTable->getPrimaryKey(); - - self::assertNotNull($primaryKey); - self::assertEquals(['id'], $primaryKey->getColumns()); - - $relatedFks = $nestedSchemaTable->getForeignKeys(); - self::assertCount(1, $relatedFks); - $relatedFk = array_pop($relatedFks); - self::assertEquals('nested.schemarelated', $relatedFk->getForeignTableName()); - } - public function testListSameTableNameColumnsWithDifferentSchema(): void { $this->connection->executeStatement('CREATE SCHEMA another'); diff --git a/tests/Functional/Schema/SchemaManagerFunctionalTestCase.php b/tests/Functional/Schema/SchemaManagerFunctionalTestCase.php index 65b2d2fb6cf..d67a932efd7 100644 --- a/tests/Functional/Schema/SchemaManagerFunctionalTestCase.php +++ b/tests/Functional/Schema/SchemaManagerFunctionalTestCase.php @@ -1813,6 +1813,49 @@ protected function findTableByName(array $tables, string $name): ?Table return null; } + + public function testTableWithSchema(): void + { + if (! $this->connection->getDatabasePlatform()->supportsSchemas()) { + self::markTestSkipped('The currently used database platform does not support schemas.'); + } + + $this->connection->executeStatement('CREATE SCHEMA nested'); + + $nestedRelatedTable = new Table('nested.schemarelated'); + $column = $nestedRelatedTable->addColumn('id', Types::INTEGER); + $column->setAutoincrement(true); + $nestedRelatedTable->setPrimaryKey(['id']); + + $nestedSchemaTable = new Table('nested.schematable'); + $column = $nestedSchemaTable->addColumn('id', Types::INTEGER); + $column->setAutoincrement(true); + $nestedSchemaTable->setPrimaryKey(['id']); + $nestedSchemaTable->addForeignKeyConstraint($nestedRelatedTable->getName(), ['id'], ['id']); + $nestedSchemaTable->setComment('This is a comment'); + + $this->schemaManager->createTable($nestedRelatedTable); + $this->schemaManager->createTable($nestedSchemaTable); + + $tableNames = $this->schemaManager->listTableNames(); + self::assertContains('nested.schematable', $tableNames); + + $tables = $this->schemaManager->listTables(); + self::assertNotNull($this->findTableByName($tables, 'nested.schematable')); + + $nestedSchemaTable = $this->schemaManager->introspectTable('nested.schematable'); + self::assertTrue($nestedSchemaTable->hasColumn('id')); + + $primaryKey = $nestedSchemaTable->getPrimaryKey(); + self::assertNotNull($primaryKey); + self::assertEquals(['id'], $primaryKey->getColumns()); + + $relatedFks = array_values($nestedSchemaTable->getForeignKeys()); + self::assertCount(1, $relatedFks); + $relatedFk = $relatedFks[0]; + self::assertEquals('nested.schemarelated', $relatedFk->getForeignTableName()); + self::assertEquals('This is a comment', $nestedSchemaTable->getComment()); + } } interface ListTableColumnsDispatchEventListener From 62ae320d77ea01c26f05be7f5dbb9c16b4944dce Mon Sep 17 00:00:00 2001 From: Sergei Morozov Date: Sat, 1 Feb 2025 17:58:41 -0800 Subject: [PATCH 2/2] Auto-quote introspected PostgreSQL identifiers --- src/Schema/PostgreSQLSchemaManager.php | 17 ++- .../Schema/OracleSchemaManagerTest.php | 116 ------------------ .../SchemaManagerFunctionalTestCase.php | 92 ++++++++++++++ 3 files changed, 104 insertions(+), 121 deletions(-) diff --git a/src/Schema/PostgreSQLSchemaManager.php b/src/Schema/PostgreSQLSchemaManager.php index 1716249fc14..bfa9a182db4 100644 --- a/src/Schema/PostgreSQLSchemaManager.php +++ b/src/Schema/PostgreSQLSchemaManager.php @@ -294,9 +294,16 @@ protected function _getPortableTableIndexesList($tableIndexes, $tableName = null foreach ($tableIndexes as $row) { $colNumbers = array_map('intval', explode(' ', $row['indkey'])); $columnNameSql = sprintf( - 'SELECT attnum, attname FROM pg_attribute WHERE attrelid=%d AND attnum IN (%s) ORDER BY attnum ASC', + <<<'SQL' + SELECT attnum, + quote_ident(attname) AS attname + FROM pg_attribute + WHERE attrelid = %d + AND attnum IN (%s) + ORDER BY attnum + SQL, $row['indrelid'], - implode(' ,', $colNumbers), + implode(', ', $colNumbers), ); $indexColumns = $this->_conn->fetchAllAssociative($columnNameSql); @@ -612,7 +619,7 @@ protected function selectTableColumns(string $databaseName, ?string $tableName = $sql = 'SELECT'; if ($tableName === null) { - $sql .= ' c.relname AS table_name, n.nspname AS schema_name,'; + $sql .= ' quote_ident(c.relname) AS table_name, quote_ident(n.nspname) AS schema_name,'; } $sql .= sprintf(<<<'SQL' @@ -664,7 +671,7 @@ protected function selectIndexColumns(string $databaseName, ?string $tableName = $sql = 'SELECT'; if ($tableName === null) { - $sql .= ' tc.relname AS table_name, tn.nspname AS schema_name,'; + $sql .= ' quote_ident(tc.relname) AS table_name, quote_ident(tn.nspname) AS schema_name,'; } $sql .= <<<'SQL' @@ -698,7 +705,7 @@ protected function selectForeignKeyColumns(string $databaseName, ?string $tableN $sql = 'SELECT'; if ($tableName === null) { - $sql .= ' tc.relname AS table_name, tn.nspname AS schema_name,'; + $sql .= ' quote_ident(tc.relname) AS table_name, quote_ident(tn.nspname) AS schema_name,'; } $sql .= <<<'SQL' diff --git a/tests/Functional/Schema/OracleSchemaManagerTest.php b/tests/Functional/Schema/OracleSchemaManagerTest.php index 7305fa91e03..e5cdd346135 100644 --- a/tests/Functional/Schema/OracleSchemaManagerTest.php +++ b/tests/Functional/Schema/OracleSchemaManagerTest.php @@ -71,122 +71,6 @@ public function testAlterTableColumnNotNull(callable $comparatorFactory): void self::assertTrue($columns['bar']->getNotnull()); } - public function testListTableDetailsWithDifferentIdentifierQuotingRequirements(): void - { - $primaryTableName = '"Primary_Table"'; - $offlinePrimaryTable = new Table($primaryTableName); - $offlinePrimaryTable->addColumn( - '"Id"', - Types::INTEGER, - ['autoincrement' => true, 'comment' => 'Explicit casing.'], - ); - $offlinePrimaryTable->addColumn('select', Types::INTEGER, ['comment' => 'Reserved keyword.']); - $offlinePrimaryTable->addColumn('foo', Types::INTEGER, ['comment' => 'Implicit uppercasing.']); - $offlinePrimaryTable->addColumn('BAR', Types::INTEGER); - $offlinePrimaryTable->addColumn('"BAZ"', Types::INTEGER); - $offlinePrimaryTable->addIndex(['select'], 'from'); - $offlinePrimaryTable->addIndex(['foo'], 'foo_index'); - $offlinePrimaryTable->addIndex(['BAR'], 'BAR_INDEX'); - $offlinePrimaryTable->addIndex(['"BAZ"'], 'BAZ_INDEX'); - $offlinePrimaryTable->setPrimaryKey(['"Id"']); - - $foreignTableName = 'foreign'; - $offlineForeignTable = new Table($foreignTableName); - $offlineForeignTable->addColumn('id', Types::INTEGER, ['autoincrement' => true]); - $offlineForeignTable->addColumn('"Fk"', Types::INTEGER); - $offlineForeignTable->addIndex(['"Fk"'], '"Fk_index"'); - $offlineForeignTable->addForeignKeyConstraint( - $primaryTableName, - ['"Fk"'], - ['"Id"'], - [], - '"Primary_Table_Fk"', - ); - $offlineForeignTable->setPrimaryKey(['id']); - - $this->dropTableIfExists($foreignTableName); - $this->dropTableIfExists($primaryTableName); - - $this->schemaManager->createTable($offlinePrimaryTable); - $this->schemaManager->createTable($offlineForeignTable); - - $onlinePrimaryTable = $this->schemaManager->introspectTable($primaryTableName); - $onlineForeignTable = $this->schemaManager->introspectTable($foreignTableName); - - $platform = $this->connection->getDatabasePlatform(); - - // Primary table assertions - self::assertSame($primaryTableName, $onlinePrimaryTable->getQuotedName($platform)); - - self::assertTrue($onlinePrimaryTable->hasColumn('"Id"')); - self::assertSame('"Id"', $onlinePrimaryTable->getColumn('"Id"')->getQuotedName($platform)); - self::assertTrue($onlinePrimaryTable->hasPrimaryKey()); - - $primaryKey = $onlinePrimaryTable->getPrimaryKey(); - - self::assertNotNull($primaryKey); - self::assertSame(['"Id"'], $primaryKey->getQuotedColumns($platform)); - - self::assertTrue($onlinePrimaryTable->hasColumn('select')); - self::assertSame('"select"', $onlinePrimaryTable->getColumn('select')->getQuotedName($platform)); - - self::assertTrue($onlinePrimaryTable->hasColumn('foo')); - self::assertSame('FOO', $onlinePrimaryTable->getColumn('foo')->getQuotedName($platform)); - - self::assertTrue($onlinePrimaryTable->hasColumn('BAR')); - self::assertSame('BAR', $onlinePrimaryTable->getColumn('BAR')->getQuotedName($platform)); - - self::assertTrue($onlinePrimaryTable->hasColumn('"BAZ"')); - self::assertSame('BAZ', $onlinePrimaryTable->getColumn('"BAZ"')->getQuotedName($platform)); - - self::assertTrue($onlinePrimaryTable->hasIndex('from')); - self::assertTrue($onlinePrimaryTable->getIndex('from')->hasColumnAtPosition('"select"')); - self::assertSame(['"select"'], $onlinePrimaryTable->getIndex('from')->getQuotedColumns($platform)); - - self::assertTrue($onlinePrimaryTable->hasIndex('foo_index')); - self::assertTrue($onlinePrimaryTable->getIndex('foo_index')->hasColumnAtPosition('foo')); - self::assertSame(['FOO'], $onlinePrimaryTable->getIndex('foo_index')->getQuotedColumns($platform)); - - self::assertTrue($onlinePrimaryTable->hasIndex('BAR_INDEX')); - self::assertTrue($onlinePrimaryTable->getIndex('BAR_INDEX')->hasColumnAtPosition('BAR')); - self::assertSame(['BAR'], $onlinePrimaryTable->getIndex('BAR_INDEX')->getQuotedColumns($platform)); - - self::assertTrue($onlinePrimaryTable->hasIndex('BAZ_INDEX')); - self::assertTrue($onlinePrimaryTable->getIndex('BAZ_INDEX')->hasColumnAtPosition('"BAZ"')); - self::assertSame(['BAZ'], $onlinePrimaryTable->getIndex('BAZ_INDEX')->getQuotedColumns($platform)); - - // Foreign table assertions - self::assertTrue($onlineForeignTable->hasColumn('id')); - self::assertSame('ID', $onlineForeignTable->getColumn('id')->getQuotedName($platform)); - self::assertTrue($onlineForeignTable->hasPrimaryKey()); - - $primaryKey = $onlineForeignTable->getPrimaryKey(); - - self::assertNotNull($primaryKey); - self::assertSame(['ID'], $primaryKey->getQuotedColumns($platform)); - - self::assertTrue($onlineForeignTable->hasColumn('"Fk"')); - self::assertSame('"Fk"', $onlineForeignTable->getColumn('"Fk"')->getQuotedName($platform)); - - self::assertTrue($onlineForeignTable->hasIndex('"Fk_index"')); - self::assertTrue($onlineForeignTable->getIndex('"Fk_index"')->hasColumnAtPosition('"Fk"')); - self::assertSame(['"Fk"'], $onlineForeignTable->getIndex('"Fk_index"')->getQuotedColumns($platform)); - - self::assertTrue($onlineForeignTable->hasForeignKey('"Primary_Table_Fk"')); - self::assertSame( - $primaryTableName, - $onlineForeignTable->getForeignKey('"Primary_Table_Fk"')->getQuotedForeignTableName($platform), - ); - self::assertSame( - ['"Fk"'], - $onlineForeignTable->getForeignKey('"Primary_Table_Fk"')->getQuotedLocalColumns($platform), - ); - self::assertSame( - ['"Id"'], - $onlineForeignTable->getForeignKey('"Primary_Table_Fk"')->getQuotedForeignColumns($platform), - ); - } - public function testListTableColumnsSameTableNamesInDifferentSchemas(): void { $table = $this->createListTableColumns(); diff --git a/tests/Functional/Schema/SchemaManagerFunctionalTestCase.php b/tests/Functional/Schema/SchemaManagerFunctionalTestCase.php index 65b2d2fb6cf..1e4ec5ab767 100644 --- a/tests/Functional/Schema/SchemaManagerFunctionalTestCase.php +++ b/tests/Functional/Schema/SchemaManagerFunctionalTestCase.php @@ -8,7 +8,9 @@ use Doctrine\DBAL\Exception; use Doctrine\DBAL\Exception\DatabaseObjectNotFoundException; use Doctrine\DBAL\Platforms\AbstractPlatform; +use Doctrine\DBAL\Platforms\DB2Platform; use Doctrine\DBAL\Platforms\OraclePlatform; +use Doctrine\DBAL\Platforms\PostgreSQLPlatform; use Doctrine\DBAL\Platforms\SqlitePlatform; use Doctrine\DBAL\Platforms\SQLServerPlatform; use Doctrine\DBAL\Schema\AbstractAsset; @@ -1716,6 +1718,96 @@ private function createReservedKeywordTables(): void $schemaManager->createSchemaObjects($schema); } + /** @throws Exception */ + public function testQuotedIdentifiers(): void + { + $platform = $this->connection->getDatabasePlatform(); + + if ($platform instanceof DB2Platform) { + self::markTestIncomplete( + 'Introspection of lower-case identifiers as quoted is currently not implemented on IBM DB2.', + ); + } + + if (! $platform instanceof OraclePlatform && ! $platform instanceof PostgreSQLPlatform) { + self::markTestSkipped('The current platform does not auto-quote introspected identifiers.'); + } + + $artists = new Table('"Artists"'); + $artists->addColumn('"Id"', Types::INTEGER); + $artists->addColumn('"Name"', Types::INTEGER); + $artists->addIndex(['"Name"'], '"Idx_Name"'); + $artists->setPrimaryKey(['"Id"']); + + $tracks = new Table('"Tracks"'); + $tracks->addColumn('"Id"', Types::INTEGER); + $tracks->addColumn('"Artist_Id"', Types::INTEGER); + $tracks->addIndex(['"Artist_Id"'], '"Idx_Artist_Id"'); + $tracks->addForeignKeyConstraint( + '"Artists"', + ['"Artist_Id"'], + ['"Id"'], + [], + '"Artists_Fk"', + ); + $tracks->setPrimaryKey(['"Id"']); + + $this->dropTableIfExists('"Tracks"'); + $this->dropTableIfExists('"Artists"'); + + $this->schemaManager->createTable($artists); + $this->schemaManager->createTable($tracks); + + $artists = $this->schemaManager->introspectTable('"Artists"'); + $tracks = $this->schemaManager->introspectTable('"Tracks"'); + + $platform = $this->connection->getDatabasePlatform(); + + // Primary table assertions + self::assertSame('"Artists"', $artists->getQuotedName($platform)); + self::assertSame('"Id"', $artists->getColumn('"Id"')->getQuotedName($platform)); + self::assertSame('"Name"', $artists->getColumn('"Name"')->getQuotedName($platform)); + self::assertSame(['"Name"'], $artists->getIndex('"Idx_Name"')->getQuotedColumns($platform)); + + $primaryKey = $artists->getPrimaryKey(); + self::assertNotNull($primaryKey); + self::assertSame(['"Id"'], $primaryKey->getQuotedColumns($platform)); + + // Foreign table assertions + self::assertTrue($tracks->hasColumn('"Id"')); + self::assertSame('"Id"', $tracks->getColumn('"Id"')->getQuotedName($platform)); + + $primaryKey = $tracks->getPrimaryKey(); + self::assertNotNull($primaryKey); + self::assertSame(['"Id"'], $primaryKey->getQuotedColumns($platform)); + + self::assertTrue($tracks->hasColumn('"Artist_Id"')); + self::assertSame( + '"Artist_Id"', + $tracks->getColumn('"Artist_Id"')->getQuotedName($platform), + ); + + self::assertTrue($tracks->hasIndex('"Idx_Artist_Id"')); + self::assertSame( + ['"Artist_Id"'], + $tracks->getIndex('"Idx_Artist_Id"')->getQuotedColumns($platform), + ); + + self::assertTrue($tracks->hasForeignKey('"Artists_Fk"')); + self::assertSame( + '"Artists"', + $tracks->getForeignKey('"Artists_Fk"')->getQuotedForeignTableName($platform), + ); + self::assertSame( + ['"Artist_Id"'], + $tracks->getForeignKey('"Artists_Fk"')->getQuotedLocalColumns($platform), + ); + self::assertSame( + ['"Id"'], + $tracks->getForeignKey('"Artists_Fk"')->getQuotedForeignColumns($platform), + ); + } + public function testChangeIndexWithForeignKeys(): void { $this->dropTableIfExists('child');