From 836b5f89c39bc50aa64d79bc32a2b90393eb35de Mon Sep 17 00:00:00 2001 From: Sergei Morozov Date: Sun, 10 Nov 2024 09:01:34 -0800 Subject: [PATCH 1/2] Fix renaming to quoted names on SQL Server sp_rename accepts the old name as a qualified name but the new name as a literal value. The caller of the stored procedure should be able to qualify the old name in order to reference a column in some table or a table in some schema, etc. But it's not necessary for the new name because the reference to the object being renamed is already established in the old name. --- src/Platforms/SQLServerPlatform.php | 20 +++++++------ .../Functional/Platform/RenameColumnTest.php | 28 +++++++++++++++++++ tests/Platforms/SQLServerPlatformTest.php | 8 +++--- 3 files changed, 43 insertions(+), 13 deletions(-) diff --git a/src/Platforms/SQLServerPlatform.php b/src/Platforms/SQLServerPlatform.php index 1be232a6bbc..7dad3796009 100644 --- a/src/Platforms/SQLServerPlatform.php +++ b/src/Platforms/SQLServerPlatform.php @@ -406,18 +406,20 @@ public function getAlterTableSQL(TableDiff $diff): array $tableNameSQL = $table->getQuotedName($this); foreach ($diff->getChangedColumns() as $columnDiff) { - $newColumn = $columnDiff->getNewColumn(); - $newColumnName = $newColumn->getQuotedName($this); + $newColumn = $columnDiff->getNewColumn(); + $oldColumn = $columnDiff->getOldColumn(); + $nameChanged = $columnDiff->hasNameChanged(); - $oldColumn = $columnDiff->getOldColumn(); - $oldColumnName = $oldColumn->getQuotedName($this); - $nameChanged = $columnDiff->hasNameChanged(); - - // Column names in SQL server are case insensitive and automatically uppercased on the server. if ($nameChanged) { + // sp_rename accepts the old name as a qualified name, so it should be quoted. + $oldColumnNameSQL = $oldColumn->getQuotedName($this); + + // sp_rename accepts the new name as a literal value, so it cannot be quoted. + $newColumnName = $newColumn->getName(); + $sql = array_merge( $sql, - $this->getRenameColumnSQL($tableNameSQL, $oldColumnName, $newColumnName), + $this->getRenameColumnSQL($tableNameSQL, $oldColumnNameSQL, $newColumnName), ); } @@ -634,7 +636,7 @@ protected function getRenameIndexSQL(string $oldIndexName, Index $index, string "EXEC sp_rename N'%s.%s', N'%s', N'INDEX'", $tableName, $oldIndexName, - $index->getQuotedName($this), + $index->getName(), ), ]; } diff --git a/tests/Functional/Platform/RenameColumnTest.php b/tests/Functional/Platform/RenameColumnTest.php index 3234e9df6e4..c32e9971dd0 100644 --- a/tests/Functional/Platform/RenameColumnTest.php +++ b/tests/Functional/Platform/RenameColumnTest.php @@ -4,6 +4,7 @@ namespace Doctrine\DBAL\Tests\Functional\Platform; +use Doctrine\DBAL\Exception; use Doctrine\DBAL\Schema\Column; use Doctrine\DBAL\Schema\Table; use Doctrine\DBAL\Schema\TableDiff; @@ -94,4 +95,31 @@ public static function columnNameProvider(): iterable yield ['C1', 'c1_x']; yield ['importantColumn', 'veryImportantColumn']; } + + /** @throws Exception */ + public function testRenameColumToQuoted(): void + { + $table = new Table('test_rename'); + $table->addColumn('c1', Types::INTEGER); + + $this->dropAndCreateTable($table); + + $table->dropColumn('c1') + ->addColumn('"c2"', Types::INTEGER); + + $schemaManager = $this->connection->createSchemaManager(); + $comparator = $schemaManager->createComparator(); + + $diff = $comparator->compareTables($schemaManager->introspectTable('test_rename'), $table); + self::assertFalse($diff->isEmpty()); + + $schemaManager->alterTable($diff); + + $platform = $this->connection->getDatabasePlatform(); + + self::assertEquals(1, $this->connection->insert( + 'test_rename', + [$platform->quoteSingleIdentifier('c2') => 1], + )); + } } diff --git a/tests/Platforms/SQLServerPlatformTest.php b/tests/Platforms/SQLServerPlatformTest.php index ac8337344df..285f2bbb9bb 100644 --- a/tests/Platforms/SQLServerPlatformTest.php +++ b/tests/Platforms/SQLServerPlatformTest.php @@ -848,8 +848,8 @@ protected function getAlterTableRenameIndexSQL(): array protected function getQuotedAlterTableRenameIndexSQL(): array { return [ - "EXEC sp_rename N'[table].[create]', N'[select]', N'INDEX'", - "EXEC sp_rename N'[table].[foo]', N'[bar]', N'INDEX'", + "EXEC sp_rename N'[table].[create]', N'select', N'INDEX'", + "EXEC sp_rename N'[table].[foo]', N'bar', N'INDEX'", ]; } @@ -867,8 +867,8 @@ protected function getAlterTableRenameIndexInSchemaSQL(): array protected function getQuotedAlterTableRenameIndexInSchemaSQL(): array { return [ - "EXEC sp_rename N'[schema].[table].[create]', N'[select]', N'INDEX'", - "EXEC sp_rename N'[schema].[table].[foo]', N'[bar]', N'INDEX'", + "EXEC sp_rename N'[schema].[table].[create]', N'select', N'INDEX'", + "EXEC sp_rename N'[schema].[table].[foo]', N'bar', N'INDEX'", ]; } From 13005427ba24eff0a018de18aa2669a08360d2fc Mon Sep 17 00:00:00 2001 From: Sergei Morozov Date: Thu, 14 Nov 2024 23:06:12 -0800 Subject: [PATCH 2/2] Generalize the logic of sp_rename execution --- src/Platforms/SQLServerPlatform.php | 33 +++++++++++++---------------- 1 file changed, 15 insertions(+), 18 deletions(-) diff --git a/src/Platforms/SQLServerPlatform.php b/src/Platforms/SQLServerPlatform.php index 7dad3796009..18324eb11e4 100644 --- a/src/Platforms/SQLServerPlatform.php +++ b/src/Platforms/SQLServerPlatform.php @@ -22,6 +22,7 @@ use Doctrine\DBAL\Types\Types; use InvalidArgumentException; +use function array_map; use function array_merge; use function array_unique; use function array_values; @@ -494,11 +495,7 @@ public function getAlterTableSQL(TableDiff $diff): array public function getRenameTableSQL(string $oldName, string $newName): string { - return sprintf( - 'sp_rename %s, %s', - $this->quoteStringLiteral($oldName), - $this->quoteStringLiteral($newName), - ); + return $this->getRenameSQL($oldName, $newName); } /** @@ -632,13 +629,7 @@ protected function getDropColumnCommentSQL(string $tableName, string $columnName */ protected function getRenameIndexSQL(string $oldIndexName, Index $index, string $tableName): array { - return [sprintf( - "EXEC sp_rename N'%s.%s', N'%s', N'INDEX'", - $tableName, - $oldIndexName, - $index->getName(), - ), - ]; + return [$this->getRenameSQL($tableName . '.' . $oldIndexName, $index->getName(), 'INDEX')]; } /** @@ -652,12 +643,18 @@ protected function getRenameIndexSQL(string $oldIndexName, Index $index, string */ protected function getRenameColumnSQL(string $tableName, string $oldColumnName, string $newColumnName): array { - return [sprintf( - "EXEC sp_rename %s, %s, 'COLUMN'", - $this->quoteStringLiteral($tableName . '.' . $oldColumnName), - $this->quoteStringLiteral($newColumnName), - ), - ]; + return [$this->getRenameSQL($tableName . '.' . $oldColumnName, $newColumnName)]; + } + + /** + * Returns the SQL statement that will execute sp_rename with the given arguments. + */ + private function getRenameSQL(string ...$arguments): string + { + return 'EXEC sp_rename ' + . implode(', ', array_map(function (string $argument): string { + return 'N' . $this->quoteStringLiteral($argument); + }, $arguments)); } /**