From 2f562d6880d647a0b5d430a1f44ab1c19ab3d547 Mon Sep 17 00:00:00 2001 From: Sergei Morozov Date: Sat, 10 Sep 2022 11:26:56 -0700 Subject: [PATCH 1/2] Rework functional tests for default values on SQL Server --- .../Schema/SQLServerSchemaManagerTest.php | 140 ++++++------------ .../SchemaManagerFunctionalTestCase.php | 87 ++++++----- 2 files changed, 90 insertions(+), 137 deletions(-) diff --git a/tests/Functional/Schema/SQLServerSchemaManagerTest.php b/tests/Functional/Schema/SQLServerSchemaManagerTest.php index 015d2360034..e541d79a487 100644 --- a/tests/Functional/Schema/SQLServerSchemaManagerTest.php +++ b/tests/Functional/Schema/SQLServerSchemaManagerTest.php @@ -6,11 +6,7 @@ use Doctrine\DBAL\Platforms\AbstractPlatform; use Doctrine\DBAL\Platforms\SQLServerPlatform; -use Doctrine\DBAL\Schema\Column; -use Doctrine\DBAL\Schema\ColumnDiff; use Doctrine\DBAL\Schema\Table; -use Doctrine\DBAL\Schema\TableDiff; -use Doctrine\DBAL\Types\Type; use function array_shift; @@ -23,18 +19,21 @@ protected function supportsPlatform(AbstractPlatform $platform): bool public function testDropColumnConstraints(): void { - $table = new Table('sqlsrv_drop_column'); - $table->addColumn('id', 'integer'); - $table->addColumn('todrop', 'integer', ['default' => 10]); + $oldTable = new Table('sqlsrv_drop_column'); + $oldTable->addColumn('id', 'integer'); + $oldTable->addColumn('todrop', 'integer', ['default' => 10]); - $this->schemaManager->createTable($table); + $newTable = clone $oldTable; + $newTable->dropColumn('todrop'); + + $this->schemaManager->createTable($oldTable); - $diff = new TableDiff( - 'sqlsrv_drop_column', - [], - [], - ['todrop' => new Column('todrop', Type::getType('integer'))], - ); + $diff = $this->schemaManager->createComparator() + ->diffTable( + $this->schemaManager->introspectTable('sqlsrv_drop_column'), + $newTable + ); + self::assertNotNull($diff); $this->schemaManager->alterTable($diff); @@ -63,28 +62,30 @@ public function testColumnCollation(): void public function testDefaultConstraints(): void { - $table = new Table('sqlsrv_default_constraints'); - $table->addColumn('no_default', 'string', ['length' => 32]); - $table->addColumn('df_integer', 'integer', ['default' => 666]); - $table->addColumn('df_string_1', 'string', [ + $oldTable = new Table('sqlsrv_default_constraints'); + $oldTable->addColumn('no_default', 'string', ['length' => 32]); + $oldTable->addColumn('df_integer', 'integer', ['default' => 666]); + $oldTable->addColumn('df_string_1', 'string', [ 'length' => 32, 'default' => 'foobar', ]); - $table->addColumn('df_string_2', 'string', [ + $oldTable->addColumn('df_string_2', 'string', [ 'length' => 32, 'default' => 'Doctrine rocks!!!', ]); - $table->addColumn('df_string_3', 'string', [ + $oldTable->addColumn('df_string_3', 'string', [ 'length' => 32, 'default' => 'another default value', ]); - $table->addColumn('df_string_4', 'string', [ + $oldTable->addColumn('df_string_4', 'string', [ 'length' => 32, 'default' => 'column to rename', ]); - $table->addColumn('df_boolean', 'boolean', ['default' => true]); + $oldTable->addColumn('df_boolean', 'boolean', ['default' => true]); - $this->schemaManager->createTable($table); + $newTable = clone $oldTable; + + $this->schemaManager->createTable($oldTable); $columns = $this->schemaManager->listTableColumns('sqlsrv_default_constraints'); self::assertNull($columns['no_default']->getDefault()); @@ -94,50 +95,29 @@ public function testDefaultConstraints(): void self::assertEquals('another default value', $columns['df_string_3']->getDefault()); self::assertEquals(1, $columns['df_boolean']->getDefault()); - $diff = new TableDiff( - 'sqlsrv_default_constraints', - [], - [ - 'df_integer' => new ColumnDiff( - new Column('df_integer', Type::getType('integer'), ['default' => 0]), - new Column('df_integer', Type::getType('integer'), ['default' => 666]), - ), - 'df_string_2' => new ColumnDiff( - new Column('df_string_2', Type::getType('string'), ['length' => 32]), - new Column('df_string_2', Type::getType('string'), [ - 'length' => 32, - 'default' => 'Doctrine rocks!!!', - ]), - ), - 'df_string_3' => new ColumnDiff( - new Column('df_string_3', Type::getType('string'), [ - 'length' => 50, - 'default' => 'another default value', - ]), - new Column('df_string_3', Type::getType('string'), [ - 'length' => 50, - 'default' => 'another default value', - ]), - ), - 'df_boolean' => new ColumnDiff( - new Column('df_boolean', Type::getType('boolean'), ['default' => false]), - new Column('df_boolean', Type::getType('boolean'), ['default' => true]), - ), - ], - [ - 'df_string_1' => new Column('df_string_1', Type::getType('string'), ['length' => 32]), - ], - [], - [], - [], - $table, - ); - $diff->newName = 'sqlsrv_default_constraints'; - $diff->renamedColumns['df_string_4'] = new Column( - 'df_string_renamed', - Type::getType('string'), - ['default' => 'column to rename'], - ); + $newTable->getColumn('df_integer') + ->setDefault(0); + + $newTable->dropColumn('df_string_1'); + + $newTable->getColumn('df_string_2') + ->setDefault(null); + + $newTable->getColumn('df_boolean') + ->setDefault(false); + + $newTable->dropColumn('df_string_4'); + $newTable->addColumn('df_string_4_renamed', 'string', [ + 'length' => 32, + 'default' => 'column to rename', + ]); + + $diff = $this->schemaManager->createComparator() + ->diffTable( + $this->schemaManager->introspectTable('sqlsrv_default_constraints'), + $newTable + ); + self::assertNotNull($diff); $this->schemaManager->alterTable($diff); $columns = $this->schemaManager->listTableColumns('sqlsrv_default_constraints'); @@ -147,31 +127,7 @@ public function testDefaultConstraints(): void self::assertNull($columns['df_string_2']->getDefault()); self::assertEquals('another default value', $columns['df_string_3']->getDefault()); self::assertEquals(0, $columns['df_boolean']->getDefault()); - self::assertEquals('column to rename', $columns['df_string_renamed']->getDefault()); - - /** - * Test that column default constraints can still be referenced after table rename - */ - $diff = new TableDiff( - 'sqlsrv_default_constraints', - [], - [ - 'df_integer' => new ColumnDiff( - new Column('df_integer', Type::getType('integer'), ['default' => 666]), - new Column('df_integer', Type::getType('integer'), ['default' => 0]), - ), - ], - [], - [], - [], - [], - $table, - ); - - $this->schemaManager->alterTable($diff); - $columns = $this->schemaManager->listTableColumns('sqlsrv_default_constraints'); - - self::assertEquals(666, $columns['df_integer']->getDefault()); + self::assertEquals('column to rename', $columns['df_string_4_renamed']->getDefault()); } public function testPkOrdering(): void diff --git a/tests/Functional/Schema/SchemaManagerFunctionalTestCase.php b/tests/Functional/Schema/SchemaManagerFunctionalTestCase.php index 579666949fd..854162a1115 100644 --- a/tests/Functional/Schema/SchemaManagerFunctionalTestCase.php +++ b/tests/Functional/Schema/SchemaManagerFunctionalTestCase.php @@ -16,7 +16,6 @@ use Doctrine\DBAL\Schema\AbstractAsset; use Doctrine\DBAL\Schema\AbstractSchemaManager; use Doctrine\DBAL\Schema\Column; -use Doctrine\DBAL\Schema\ColumnDiff; use Doctrine\DBAL\Schema\ForeignKeyConstraint; use Doctrine\DBAL\Schema\Index; use Doctrine\DBAL\Schema\Schema; @@ -838,39 +837,34 @@ public function testRenameIndexUsedInForeignKeyConstraint(): void public function testChangeColumnsTypeWithDefaultValue(): void { - $tableName = 'column_def_change_type'; - $table = new Table($tableName); + $oldTable = new Table('column_def_change_type'); - $table->addColumn('col_int', 'smallint', ['default' => 666]); - $table->addColumn('col_string', 'string', [ + $oldTable->addColumn('col_int', 'smallint', ['default' => 666]); + $oldTable->addColumn('col_string', 'string', [ 'length' => 3, 'default' => 'foo', ]); - $this->dropAndCreateTable($table); + $this->dropAndCreateTable($oldTable); - $tableDiff = new TableDiff($tableName); - $tableDiff->fromTable = $table; - $tableDiff->changedColumns['col_int'] = new ColumnDiff( - new Column('col_int', Type::getType('integer'), ['default' => 666]), - new Column('col_int', Type::getType('smallint'), ['default' => 666]), - ); + $newTable = clone $oldTable; - $tableDiff->changedColumns['col_string'] = new ColumnDiff( - new Column('col_string', Type::getType('string'), [ - 'length' => 3, - 'fixed' => true, - 'default' => 'foo', - ]), - new Column('col_string', Type::getType('string'), [ - 'length' => 3, - 'default' => 'foo', - ]), - ); + $newTable->getColumn('col_int') + ->setType(Type::getType('integer')); - $this->schemaManager->alterTable($tableDiff); + $newTable->getColumn('col_string') + ->setFixed(true); - $columns = $this->schemaManager->listTableColumns($tableName); + $diff = $this->schemaManager->createComparator() + ->diffTable( + $this->schemaManager->introspectTable('column_def_change_type'), + $newTable + ); + self::assertNotNull($diff); + + $this->schemaManager->alterTable($diff); + + $columns = $this->schemaManager->listTableColumns('column_def_change_type'); self::assertInstanceOf(IntegerType::class, $columns['col_int']->getType()); self::assertEquals(666, $columns['col_int']->getDefault()); @@ -969,45 +963,48 @@ public function testListForeignKeysComposite(): void public function testColumnDefaultLifecycle(): void { - $table = new Table('col_def_lifecycle'); - $table->addColumn('id', 'integer', ['autoincrement' => true]); - $table->addColumn('column1', 'string', [ + $oldTable = new Table('col_def_lifecycle'); + $oldTable->addColumn('id', 'integer', ['autoincrement' => true]); + $oldTable->addColumn('column1', 'string', [ 'length' => 1, 'default' => null, ]); - $table->addColumn('column2', 'string', [ + $oldTable->addColumn('column2', 'string', [ 'length' => 1, 'default' => '', ]); - $table->addColumn('column3', 'string', [ + $oldTable->addColumn('column3', 'string', [ 'length' => 8, 'default' => 'default1', ]); - $table->addColumn('column4', 'integer', [ + $oldTable->addColumn('column4', 'integer', [ 'length' => 1, 'default' => 0, ]); - $table->setPrimaryKey(['id']); + $oldTable->setPrimaryKey(['id']); - $this->dropAndCreateTable($table); + $this->dropAndCreateTable($oldTable); - $columns = $this->schemaManager->listTableColumns('col_def_lifecycle'); + $oldTable = $this->schemaManager->introspectTable('col_def_lifecycle'); - self::assertNull($columns['id']->getDefault()); - self::assertNull($columns['column1']->getDefault()); - self::assertSame('', $columns['column2']->getDefault()); - self::assertSame('default1', $columns['column3']->getDefault()); - self::assertSame('0', $columns['column4']->getDefault()); + self::assertNull($oldTable->getColumn('id')->getDefault()); + self::assertNull($oldTable->getColumn('column1')->getDefault()); + self::assertSame('', $oldTable->getColumn('column2')->getDefault()); + self::assertSame('default1', $oldTable->getColumn('column3')->getDefault()); + self::assertSame('0', $oldTable->getColumn('column4')->getDefault()); - $diffTable = clone $table; + $newTable = clone $oldTable; - $diffTable->changeColumn('column1', ['default' => '']); - $diffTable->changeColumn('column2', ['default' => null]); - $diffTable->changeColumn('column3', ['default' => 'default2']); - $diffTable->changeColumn('column4', ['default' => null]); + $newTable->changeColumn('column1', ['default' => '']); + $newTable->changeColumn('column2', ['default' => null]); + $newTable->changeColumn('column3', ['default' => 'default2']); + $newTable->changeColumn('column4', ['default' => null]); $diff = $this->schemaManager->createComparator() - ->diffTable($table, $diffTable); + ->diffTable( + $this->schemaManager->introspectTable('col_def_lifecycle'), + $newTable + ); self::assertNotNull($diff); $this->schemaManager->alterTable($diff); From c8d2c3defdba3b2bf4421c92b649598fa2a24164 Mon Sep 17 00:00:00 2001 From: Sergei Morozov Date: Fri, 9 Sep 2022 22:20:24 -0700 Subject: [PATCH 2/2] Introspect default constraint name --- UPGRADE.md | 5 ++ src/Platforms/SQLServerPlatform.php | 88 +++++++++------------------ src/Schema/SQLServerSchemaManager.php | 68 ++++----------------- 3 files changed, 45 insertions(+), 116 deletions(-) diff --git a/UPGRADE.md b/UPGRADE.md index 94124f40bfd..ba590bf0e38 100644 --- a/UPGRADE.md +++ b/UPGRADE.md @@ -46,6 +46,11 @@ The following classes have been converted to enums: The corresponding class constants are now instances of their enum type. +## BC BREAK: dropped naming convention for default constraints on SQL Server + +The DBAL no longer generates default constraint names using the table name and column name. The name is now generated +by the database. + ## BC BREAK: renamed SQLite platform classes 1. `SqlitePlatform` => `SQLitePlatform` diff --git a/src/Platforms/SQLServerPlatform.php b/src/Platforms/SQLServerPlatform.php index ae974995164..f099accc610 100644 --- a/src/Platforms/SQLServerPlatform.php +++ b/src/Platforms/SQLServerPlatform.php @@ -45,6 +45,9 @@ */ class SQLServerPlatform extends AbstractPlatform { + /** @internal Should be used only from within the {@see AbstractSchemaManager} class hierarchy. */ + public const OPTION_DEFAULT_CONSTRAINT_NAME = 'default_constraint_name'; + public function getCurrentDateSQL(): string { return $this->getConvertExpression('date', 'GETDATE()'); @@ -182,7 +185,7 @@ protected function _getCreateTableSQL(string $name, array $columns, array $optio // Build default constraints SQL statements. if (isset($column['default'])) { $defaultConstraintsSql[] = 'ALTER TABLE ' . $name . - ' ADD' . $this->getDefaultConstraintDeclarationSQL($name, $column); + ' ADD' . $this->getDefaultConstraintDeclarationSQL($column); } if (empty($column['comment']) && ! is_numeric($column['comment'])) { @@ -288,12 +291,11 @@ protected function getCreateColumnCommentSQL(string $tableName, string $columnNa /** * Returns the SQL snippet for declaring a default constraint. * - * @param string $table Name of the table to return the default constraint declaration for. * @param mixed[] $column Column definition. * * @throws InvalidArgumentException */ - protected function getDefaultConstraintDeclarationSQL(string $table, array $column): string + protected function getDefaultConstraintDeclarationSQL(array $column): string { if (! isset($column['default'])) { throw new InvalidArgumentException('Incomplete column definition. "default" required.'); @@ -301,10 +303,7 @@ protected function getDefaultConstraintDeclarationSQL(string $table, array $colu $columnName = new Identifier($column['name']); - return ' CONSTRAINT ' . - $this->generateDefaultConstraintName($table, $column['name']) . - $this->getDefaultValueDeclarationSQL($column) . - ' FOR ' . $columnName->getQuotedName($this); + return $this->getDefaultValueDeclarationSQL($column) . ' FOR ' . $columnName->getQuotedName($this); } public function getCreateIndexSQL(Index $index, string $table): string @@ -368,9 +367,7 @@ public function getAlterTableSQL(TableDiff $diff): array $addColumnSql = 'ADD ' . $this->getColumnDeclarationSQL($column->getQuotedName($this), $columnProperties); if (isset($columnProperties['default'])) { - $addColumnSql .= ' CONSTRAINT ' . - $this->generateDefaultConstraintName($diff->name, $column->getQuotedName($this)) . - $this->getDefaultValueDeclarationSQL($columnProperties); + $addColumnSql .= $this->getDefaultValueDeclarationSQL($columnProperties); } $queryParts[] = $addColumnSql; @@ -393,6 +390,10 @@ public function getAlterTableSQL(TableDiff $diff): array continue; } + if ($column->getDefault() !== null) { + $queryParts[] = $this->getAlterTableDropDefaultConstraintClause($column); + } + $queryParts[] = 'DROP COLUMN ' . $column->getQuotedName($this); } @@ -440,10 +441,7 @@ public function getAlterTableSQL(TableDiff $diff): array $requireDropDefaultConstraint = $this->alterColumnRequiresDropDefaultConstraint($columnDiff); if ($requireDropDefaultConstraint) { - $queryParts[] = $this->getAlterTableDropDefaultConstraintClause( - $diff->name, - $oldColumn->getName(), - ); + $queryParts[] = $this->getAlterTableDropDefaultConstraintClause($oldColumn); } if ($declarationSQLChanged) { @@ -467,20 +465,12 @@ public function getAlterTableSQL(TableDiff $diff): array $oldColumnName = new Identifier($oldColumnName); - $sql[] = "sp_rename '" . - $diff->getName($this)->getQuotedName($this) . '.' . $oldColumnName->getQuotedName($this) . - "', '" . $newColumn->getQuotedName($this) . "', 'COLUMN'"; - - // Recreate default constraint with new column name if necessary (for future reference). - if ($newColumn->getDefault() === null) { - continue; - } - - $queryParts[] = $this->getAlterTableDropDefaultConstraintClause( - $diff->name, + $sql[] = sprintf( + "sp_rename '%s.%s', '%s', 'COLUMN'", + $diff->getName($this)->getQuotedName($this), $oldColumnName->getQuotedName($this), + $newColumn->getQuotedName($this) ); - $queryParts[] = $this->getAlterTableAddDefaultConstraintClause($diff->name, $newColumn); } $tableSql = []; @@ -499,23 +489,6 @@ public function getAlterTableSQL(TableDiff $diff): array if ($newName !== null) { $sql[] = "sp_rename '" . $diff->getName($this)->getQuotedName($this) . "', '" . $newName->getName() . "'"; - - /** - * Rename table's default constraints names - * to match the new table name. - * This is necessary to ensure that the default - * constraints can be referenced in future table - * alterations as the table name is encoded in - * default constraints' names. - */ - $sql[] = "DECLARE @sql NVARCHAR(MAX) = N''; " . - "SELECT @sql += N'EXEC sp_rename N''' + dc.name + ''', N''' " . - "+ REPLACE(dc.name, '" . $this->generateIdentifierName($diff->name) . "', " . - "'" . $this->generateIdentifierName($newName->getName()) . "') + ''', ''OBJECT'';' " . - 'FROM sys.default_constraints dc ' . - 'JOIN sys.tables tbl ON dc.parent_object_id = tbl.object_id ' . - "WHERE tbl.name = '" . $newName->getName() . "';" . - 'EXEC sp_executesql @sql'; } $sql = array_merge( @@ -538,18 +511,24 @@ private function getAlterTableAddDefaultConstraintClause(string $tableName, Colu $columnDef = $column->toArray(); $columnDef['name'] = $column->getQuotedName($this); - return 'ADD' . $this->getDefaultConstraintDeclarationSQL($tableName, $columnDef); + return 'ADD' . $this->getDefaultConstraintDeclarationSQL($columnDef); } /** * Returns the SQL clause for dropping an existing default constraint in an ALTER TABLE statement. - * - * @param string $tableName The name of the table to generate the clause for. - * @param string $columnName The name of the column to generate the clause for. */ - private function getAlterTableDropDefaultConstraintClause(string $tableName, string $columnName): string + private function getAlterTableDropDefaultConstraintClause(Column $column): string { - return 'DROP CONSTRAINT ' . $this->generateDefaultConstraintName($tableName, $columnName); + if (! $column->hasPlatformOption(self::OPTION_DEFAULT_CONSTRAINT_NAME)) { + throw new InvalidArgumentException( + 'Column ' . $column->getName() . ' was not properly introspected as it has a default value' + . ' but does not have the default constraint name.', + ); + } + + return 'DROP CONSTRAINT ' . $this->quoteIdentifier( + $column->getPlatformOption(self::OPTION_DEFAULT_CONSTRAINT_NAME), + ); } /** @@ -1226,17 +1205,6 @@ protected function getLikeWildcardCharacters(): string return parent::getLikeWildcardCharacters() . '[]^'; } - /** - * Returns a unique default constraint name for a table and column. - * - * @param string $table Name of the table to generate the unique default constraint name for. - * @param string $column Name of the column in the table to generate the unique default constraint name for. - */ - private function generateDefaultConstraintName(string $table, string $column): string - { - return 'DF_' . $this->generateIdentifierName($table) . '_' . $this->generateIdentifierName($column); - } - /** * Returns a hash value for a given identifier. * diff --git a/src/Schema/SQLServerSchemaManager.php b/src/Schema/SQLServerSchemaManager.php index c1493521497..40e7fb77e35 100644 --- a/src/Schema/SQLServerSchemaManager.php +++ b/src/Schema/SQLServerSchemaManager.php @@ -12,7 +12,6 @@ use function array_change_key_case; use function assert; -use function count; use function explode; use function implode; use function is_string; @@ -65,7 +64,7 @@ protected function _getPortableTableColumnDefinition(array $tableColumn): Column $length = (int) $tableColumn['length']; - $precision = $default = null; + $precision = null; $scale = 0; $fixed = false; @@ -82,10 +81,6 @@ protected function _getPortableTableColumnDefinition(array $tableColumn): Column $precision = (int) $tableColumn['precision']; } - if ($tableColumn['default'] !== null) { - $default = $this->parseDefaultExpression($tableColumn['default']); - } - switch ($dbType) { case 'nchar': case 'nvarchar': @@ -118,7 +113,6 @@ protected function _getPortableTableColumnDefinition(array $tableColumn): Column $options = [ 'fixed' => $fixed, - 'default' => $default, 'notnull' => (bool) $tableColumn['notnull'], 'scale' => $scale, 'precision' => $precision, @@ -135,6 +129,16 @@ protected function _getPortableTableColumnDefinition(array $tableColumn): Column $column = new Column($tableColumn['name'], Type::getType($type), $options); + if ($tableColumn['default'] !== null) { + $default = $this->parseDefaultExpression($tableColumn['default']); + + $column->setDefault($default); + $column->setPlatformOption( + SQLServerPlatform::OPTION_DEFAULT_CONSTRAINT_NAME, + $tableColumn['df_name'], + ); + } + if (isset($tableColumn['collation']) && $tableColumn['collation'] !== 'NULL') { $column->setPlatformOption('collation', $tableColumn['collation']); } @@ -246,58 +250,9 @@ protected function _getPortableDatabaseDefinition(array $database): string */ protected function _getPortableViewDefinition(array $view): View { - // @todo return new View($view['name'], $view['definition']); } - public function alterTable(TableDiff $tableDiff): void - { - if (count($tableDiff->removedColumns) > 0) { - foreach ($tableDiff->removedColumns as $col) { - foreach ($this->getColumnConstraints($tableDiff->name, $col->getName()) as $constraint) { - $this->connection->executeStatement( - sprintf( - 'ALTER TABLE %s DROP CONSTRAINT %s', - $tableDiff->name, - $constraint, - ), - ); - } - } - } - - parent::alterTable($tableDiff); - } - - /** - * Returns the names of the constraints for a given column. - * - * @return iterable - * - * @throws Exception - */ - private function getColumnConstraints(string $table, string $column): iterable - { - return $this->connection->iterateColumn( - <<<'SQL' -SELECT o.name -FROM sys.objects o - INNER JOIN sys.objects t - ON t.object_id = o.parent_object_id - AND t.type = 'U' - INNER JOIN sys.default_constraints dc - ON dc.object_id = o.object_id - INNER JOIN sys.columns c - ON c.column_id = dc.parent_column_id - AND c.object_id = t.object_id -WHERE t.name = ? - AND c.name = ? -SQL - , - [$table, $column], - ); - } - /** @throws Exception */ public function createComparator(): Comparator { @@ -351,6 +306,7 @@ protected function selectTableColumns(string $databaseName, ?string $tableName = col.max_length AS length, ~col.is_nullable AS notnull, def.definition AS [default], + def.name AS df_name, col.scale, col.precision, col.is_identity AS autoincrement,