Merge pull request #542 from deeky666/DBAL-825

[DBAL-825] Drop default constraints before altering column type on SQL Server

lib/Doctrine/DBAL/Platforms/PostgreSqlPlatform.php

@@ -379,7 +379,7 @@ class PostgreSqlPlatform extends AbstractPlatform
                         AND pg_index.indkey[0] = a.attnum
                         AND pg_index.indisprimary = 't'
                     ) AS pri,
-                    (SELECT pg_attrdef.adsrc
+                    (SELECT pg_get_expr(adbin, adrelid)
                      FROM pg_attrdef
                      WHERE c.oid = pg_attrdef.adrelid
                         AND pg_attrdef.adnum=a.attnum
@@ -485,7 +485,7 @@ class PostgreSqlPlatform extends AbstractPlatform
                 $sql[] = 'ALTER TABLE ' . $diff->getName()->getQuotedName($this) . ' ' . $query;
             }
 
-            if ($columnDiff->hasChanged('default')) {
+            if ($columnDiff->hasChanged('default') || $columnDiff->hasChanged('type')) {
                 $defaultClause = null === $column->getDefault()
                     ? ' DROP DEFAULT'
                     : ' SET' . $this->getDefaultValueDeclarationSQL($column->toArray());

lib/Doctrine/DBAL/Platforms/SQLServerPlatform.php

@@ -21,6 +21,7 @@ namespace Doctrine\DBAL\Platforms;
 
 use Doctrine\DBAL\LockMode;
 use Doctrine\DBAL\Schema\Column;
+use Doctrine\DBAL\Schema\ColumnDiff;
 use Doctrine\DBAL\Schema\TableDiff;
 use Doctrine\DBAL\Schema\ForeignKeyConstraint;
 use Doctrine\DBAL\Schema\Index;
@@ -453,8 +454,7 @@ class SQLServerPlatform extends AbstractPlatform
             $queryParts[] = 'ADD ' . $this->getColumnDeclarationSQL($column->getQuotedName($this), $columnDef);
 
             if (isset($columnDef['default'])) {
-                $columnDef['name'] = $column->getQuotedName($this);
-                $queryParts[] = 'ADD' . $this->getDefaultConstraintDeclarationSQL($diff->name, $columnDef);
+                $queryParts[] = $this->getAlterTableAddDefaultConstraintClause($diff->name, $column);
             }
 
             $comment = $this->getColumnComment($column);
@@ -514,26 +514,22 @@ class SQLServerPlatform extends AbstractPlatform
                 continue;
             }
 
-            $fromColumnDefault = isset($columnDiff->fromColumn) ? $columnDiff->fromColumn->getDefault() : null;
-            $columnDef = $column->toArray();
-            $columnDefaultHasChanged = $columnDiff->hasChanged('default');
+            $requireDropDefaultConstraint = $this->alterColumnRequiresDropDefaultConstraint($columnDiff);
 
-            /**
-             * Drop existing column default constraint
-             * if default value has changed and another
-             * default constraint already exists for the column.
-             */
-            if ($columnDefaultHasChanged && null !== $fromColumnDefault) {
-                $queryParts[] = 'DROP CONSTRAINT ' .
-                    $this->generateDefaultConstraintName($diff->name, $columnDiff->oldColumnName);
+            if ($requireDropDefaultConstraint) {
+                $queryParts[] = $this->getAlterTableDropDefaultConstraintClause(
+                    $diff->name,
+                    $columnDiff->oldColumnName
+                );
             }
 
+            $columnDef = $column->toArray();
+
             $queryParts[] = 'ALTER COLUMN ' .
                     $this->getColumnDeclarationSQL($column->getQuotedName($this), $columnDef);
 
-            if ($columnDefaultHasChanged && isset($columnDef['default'])) {
-                $columnDef['name'] = $column->getQuotedName($this);
-                $queryParts[] = 'ADD' . $this->getDefaultConstraintDeclarationSQL($diff->name, $columnDef);
+            if (isset($columnDef['default']) && ($requireDropDefaultConstraint || $columnDiff->hasChanged('default'))) {
+                $queryParts[] = $this->getAlterTableAddDefaultConstraintClause($diff->name, $column);
             }
         }
 
@@ -544,26 +540,10 @@ class SQLServerPlatform extends AbstractPlatform
 
             $sql[] = "sp_RENAME '". $diff->name. ".". $oldColumnName . "', '".$column->getQuotedName($this)."', 'COLUMN'";
 
-            $columnDef = $column->toArray();
-
-            /**
-             * Drop existing default constraint for the old column name
-             * if column has default value.
-             */
-            if (isset($columnDef['default'])) {
-                $queryParts[] = 'DROP CONSTRAINT ' .
-                    $this->generateDefaultConstraintName($diff->name, $oldColumnName);
-            }
-
-            $queryParts[] = 'ALTER COLUMN ' .
-                    $this->getColumnDeclarationSQL($column->getQuotedName($this), $columnDef);
-
-            /**
-             * Readd default constraint for the new column name.
-             */
-            if (isset($columnDef['default'])) {
-                $columnDef['name'] = $column->getQuotedName($this);
-                $queryParts[] = 'ADD' . $this->getDefaultConstraintDeclarationSQL($diff->name, $columnDef);
+            // Recreate default constraint with new column name if necessary (for future reference).
+            if ($column->getDefault() !== null) {
+                $queryParts[] = $this->getAlterTableDropDefaultConstraintClause($diff->name, $oldColumnName);
+                $queryParts[] = $this->getAlterTableAddDefaultConstraintClause($diff->name, $column);
             }
         }
 
@@ -603,6 +583,76 @@ class SQLServerPlatform extends AbstractPlatform
         return array_merge($sql, $tableSql, $columnSql);
     }
 
+    /**
+     * Returns the SQL clause for adding a default constraint in an ALTER TABLE statement.
+     *
+     * @param  string $tableName The name of the table to generate the clause for.
+     * @param  Column $column    The column to generate the clause for.
+     *
+     * @return string
+     */
+    private function getAlterTableAddDefaultConstraintClause($tableName, Column $column)
+    {
+        $columnDef = $column->toArray();
+        $columnDef['name'] = $column->getQuotedName($this);
+
+        return 'ADD' . $this->getDefaultConstraintDeclarationSQL($tableName, $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.
+     *
+     * @return string
+     */
+    private function getAlterTableDropDefaultConstraintClause($tableName, $columnName)
+    {
+        return 'DROP CONSTRAINT ' . $this->generateDefaultConstraintName($tableName, $columnName);
+    }
+
+    /**
+     * Checks whether a column alteration requires dropping its default constraint first.
+     *
+     * Different to other database vendors SQL Server implements column default values
+     * as constraints and therefore changes in a column's default value as well as changes
+     * in a column's type require dropping the default constraint first before being to
+     * alter the particular column to the new definition.
+     *
+     * @param  ColumnDiff $columnDiff The column diff to evaluate.
+     *
+     * @return boolean True if the column alteration requires dropping its default constraint first, false otherwise.
+     */
+    private function alterColumnRequiresDropDefaultConstraint(ColumnDiff $columnDiff)
+    {
+        // We can only decide whether to drop an existing default constraint
+        // if we know the original default value.
+        if ( ! $columnDiff->fromColumn instanceof Column) {
+            return false;
+        }
+
+        // We only need to drop an existing default constraint if we know the
+        // column was defined with a default value before.
+        if ($columnDiff->fromColumn->getDefault() === null) {
+            return false;
+        }
+
+        // We need to drop an existing default constraint if the column was
+        // defined with a default value before and it has changed.
+        if ($columnDiff->hasChanged('default')) {
+            return true;
+        }
+
+        // We need to drop an existing default constraint if the column was
+        // defined with a default value before and the native column type has changed.
+        if ($columnDiff->hasChanged('type') || $columnDiff->hasChanged('fixed')) {
+            return true;
+        }
+
+        return false;
+    }
+
     /**
      * Returns the SQL statement for altering a column comment.
      *

tests/Doctrine/Tests/DBAL/Functional/Schema/SchemaManagerFunctionalTestCase.php

@@ -4,8 +4,11 @@ namespace Doctrine\Tests\DBAL\Functional\Schema;
 
 use Doctrine\Common\EventManager;
 use Doctrine\DBAL\Events;
+use Doctrine\DBAL\Schema\Column;
+use Doctrine\DBAL\Schema\ColumnDiff;
 use Doctrine\DBAL\Schema\Comparator;
 use Doctrine\DBAL\Schema\Table;
+use Doctrine\DBAL\Schema\TableDiff;
 use Doctrine\DBAL\Types\Type;
 
 require_once __DIR__ . '/../../../TestInit.php';
@@ -621,6 +624,46 @@ class SchemaManagerFunctionalTestCase extends \Doctrine\Tests\DbalFunctionalTest
         $this->assertInstanceOf('Doctrine\DBAL\Types\ArrayType', $columns['arr']->getType(), "The Doctrine2 should be detected from comment hint.");
     }
 
+    /**
+     * @group DBAL-825
+     */
+    public function testChangeColumnsTypeWithDefaultValue()
+    {
+        $tableName = 'column_def_change_type';
+        $table     = new Table($tableName);
+
+        $table->addColumn('col_int', 'smallint', array('default' => 666));
+        $table->addColumn('col_string', 'string', array('default' => 'foo'));
+
+        $this->_sm->dropAndCreateTable($table);
+
+        $tableDiff = new TableDiff($tableName);
+        $tableDiff->fromTable = $table;
+        $tableDiff->changedColumns['col_int'] = new ColumnDiff(
+            'col_int',
+            new Column('col_int', Type::getType('integer'), array('default' => 666)),
+            array('type'),
+            new Column('col_int', Type::getType('smallint'), array('default' => 666))
+        );
+
+        $tableDiff->changedColumns['col_string'] = new ColumnDiff(
+            'col_string',
+            new Column('col_string', Type::getType('string'), array('default' => 'foo', 'fixed' => true)),
+            array('fixed'),
+            new Column('col_string', Type::getType('string'), array('default' => 'foo'))
+        );
+
+        $this->_sm->alterTable($tableDiff);
+
+        $columns = $this->_sm->listTableColumns($tableName);
+
+        $this->assertInstanceOf('Doctrine\DBAL\Types\IntegerType', $columns['col_int']->getType());
+        $this->assertEquals(666, $columns['col_int']->getDefault());
+
+        $this->assertInstanceOf('Doctrine\DBAL\Types\StringType', $columns['col_string']->getType());
+        $this->assertEquals('foo', $columns['col_string']->getDefault());
+    }
+
     /**
      * @group DBAL-197
      */

tests/Doctrine/Tests/DBAL/Platforms/AbstractSQLServerPlatformTestCase.php

@@ -677,9 +677,6 @@ abstract class AbstractSQLServerPlatformTestCase extends AbstractPlatformTestCas
                 "ALTER TABLE mytable ALTER COLUMN [create] VARCHAR(MAX) NOT NULL",
                 "ALTER TABLE mytable ALTER COLUMN commented_type INT NOT NULL",
 
-                // Renamed columns.
-                "ALTER TABLE mytable ALTER COLUMN comment_double_0 NUMERIC(10, 0) NOT NULL",
-
                 // Added columns.
                 "EXEC sp_addextendedproperty N'MS_Description', N'0', N'SCHEMA', dbo, N'TABLE', mytable, N'COLUMN', added_comment_integer_0",
                 "EXEC sp_addextendedproperty N'MS_Description', N'0', N'SCHEMA', dbo, N'TABLE', mytable, N'COLUMN', added_comment_float_0",
@@ -824,4 +821,46 @@ abstract class AbstractSQLServerPlatformTestCase extends AbstractPlatformTestCas
             "EXEC sp_RENAME N'[table].[foo]', N'[bar]', N'INDEX'",
         );
     }
+
+    /**
+     * @group DBAL-825
+     */
+    public function testChangeColumnsTypeWithDefaultValue()
+    {
+        $tableName = 'column_def_change_type';
+        $table     = new Table($tableName);
+
+        $table->addColumn('col_int', 'smallint', array('default' => 666));
+        $table->addColumn('col_string', 'string', array('default' => 'foo'));
+
+        $tableDiff = new TableDiff($tableName);
+        $tableDiff->fromTable = $table;
+        $tableDiff->changedColumns['col_int'] = new ColumnDiff(
+            'col_int',
+            new Column('col_int', Type::getType('integer'), array('default' => 666)),
+            array('type'),
+            new Column('col_int', Type::getType('smallint'), array('default' => 666))
+        );
+
+        $tableDiff->changedColumns['col_string'] = new ColumnDiff(
+            'col_string',
+            new Column('col_string', Type::getType('string'), array('default' => 666, 'fixed' => true)),
+            array('fixed'),
+            new Column('col_string', Type::getType('string'), array('default' => 666))
+        );
+
+        $expected = $this->_platform->getAlterTableSQL($tableDiff);
+
+        $this->assertSame(
+            $expected,
+            array(
+                'ALTER TABLE column_def_change_type DROP CONSTRAINT DF_829302E0_FA2CB292',
+                'ALTER TABLE column_def_change_type ALTER COLUMN col_int INT NOT NULL',
+                'ALTER TABLE column_def_change_type ADD CONSTRAINT DF_829302E0_FA2CB292 DEFAULT 666 FOR col_int',
+                'ALTER TABLE column_def_change_type DROP CONSTRAINT DF_829302E0_2725A6D0',
+                'ALTER TABLE column_def_change_type ALTER COLUMN col_string NCHAR(255) NOT NULL',
+                "ALTER TABLE column_def_change_type ADD CONSTRAINT DF_829302E0_2725A6D0 DEFAULT '666' FOR col_string",
+            )
+        );
+    }
 }