Wrong column comment setting command in migrations of SQL Server

[msyfurukawa]

Bug Report

Q	A
BC Break	no
Version	2.8.0

Summary

The condition to use different SQL command to set column comment in SQL Server is not proper.

Current behaviour

When I attempted to alter columns (already had comments) in SQL Server with a migration function of Laravel (it uses doctrine/dbal in alteration of columns), the migration sequence was aborted with an SQL execution error if a same column comment was specified as previous. The error message was as follows:

Illuminate\Database\QueryException  : SQLSTATE[42000]: [Microsoft][ODBC Driver 13 for SQL Server][SQL Server]Property cannot be added. Property 'MS_Description' already exists for 'dbo.table_name.column_name'. (SQL: EXEC sp_addextendedproperty N'MS_Description', N'column_comment', N'SCHEMA', 'dbo', N'TABLE', 'table_name', N'COLUMN', column_name)

  at /project_dir/vendor/laravel/framework/src/Illuminate/Database/Connection.php:664
    660|         // If an exception occurs when attempting to run a query, we'll format the error
    661|         // message to include the bindings with SQL, which will make this exception a
    662|         // lot more helpful to the developer instead of just the database's errors.
    663|         catch (Exception $e) {
  > 664|             throw new QueryException(
    665|                 $query, $this->prepareBindings($bindings), $e
    666|             );
    667|         }
    668| 

  Exception trace:

  1   Doctrine\DBAL\Driver\PDOException::("SQLSTATE[42000]: [Microsoft][ODBC Driver 13 for SQL Server][SQL Server]Property cannot be added. Property 'MS_Description' already exists for 'dbo.table_name.column_name'.")
      /project_dir/vendor/doctrine/dbal/lib/Doctrine/DBAL/Driver/PDOStatement.php:144

  2   PDOException::("SQLSTATE[42000]: [Microsoft][ODBC Driver 13 for SQL Server][SQL Server]Property cannot be added. Property 'MS_Description' already exists for 'dbo.table_name.column_name'.")
      /project_dir/vendor/doctrine/dbal/lib/Doctrine/DBAL/Driver/PDOStatement.php:142

To change (not add) column comment, sp_updateextendedproperty should be used, but instead doctrine/dbal tries to use sp_addextendedproperty when a new comment is same as previous, which results in SQL execution error.

Solution

The causal code is here:

dbal/lib/Doctrine/DBAL/Platforms/SQLServerPlatform.php

Lines 509 to 523 in 2b8513d

	if ($hasFromComment && $hasComment && $fromComment !== $comment) {
	$commentsSql[] = $this->getAlterColumnCommentSQL(
	$diff->name,
	$column->getQuotedName($this),
	$comment
	);
	} elseif ($hasFromComment && ! $hasComment) {
	$commentsSql[] = $this->getDropColumnCommentSQL($diff->name, $column->getQuotedName($this));
	} elseif ($hasComment) {
	$commentsSql[] = $this->getCreateColumnCommentSQL(
	$diff->name,
	$column->getQuotedName($this),
	$comment
	);
	}

The line 517 should be corrected as follows:

-   } elseif ($hasComment) {
+   } elseif (! $hasFromComment && $hasComment) {

In my local environment, this correction solved the problem.

[Ocramius]

@msyfurukawa is it possible to isolate this into a test case?

[msyfurukawa]

@Ocramius
I've tested the schema manager of doctrine/dbal isolatedly.
To mention the result first, doctrine/dbal itself works in the right manner. It seems that Laravel migration uses doctrine/dbal in an improper way.

The test workflow is as follows:

1. Create a test table on SQL Server

Execute the raw sql below:

CREATE TABLE dbo.users
(
  id      int IDENTITY (1,1) Not Null,
  name varchar(5) Not Null,
  CONSTRAINT PK_users PRIMARY KEY (id),
)

-- set column comment `user name` on users.name
EXEC sys.sp_addextendedproperty  @name=N'MS_Description'
,@value=N'user name'
,@level0type=N'SCHEMA'
,@level0name=N'dbo'
,@level1type=N'TABLE'
,@level1name=N'users'
,@level2type=N'COLUMN'
,@level2name=N'name'

2. Test the doctrine/dbal behavior

Execute the php code below:

<?php

$config = new \Doctrine\DBAL\Configuration();
$connectionParams = array(
    'dbname' => '[DB_NAME]',
    'user' => '[USER_NAME]',
    'password' => '[PASSWORD]',
    'host' => '[HOST_NAME]',
    'port' => [PORT],
    'driver' => 'sqlsrv',
);
$conn = \Doctrine\DBAL\DriverManager::getConnection($connectionParams, $config);

$sm = $conn->getSchemaManager();
$fromSchema = $sm->createSchema();

/*
 * set column comment
 * (different from previous)
 */
$toSchema1 = clone $fromSchema;
$toSchema1->getTable('users')
    ->getColumn('name')
    ->setComment('different comment');
$sql1 = $fromSchema->getMigrateToSql($toSchema1, $conn->getDatabasePlatform());
print_r($sql1);

/*
 * set column comment
 * (same as previous)
 */
$toSchema2 = clone $fromSchema;
$toSchema2->getTable('users')
    ->getColumn('name')
    ->setComment('user name');
$sql2 = $fromSchema->getMigrateToSql($toSchema2, $conn->getDatabasePlatform());
print_r($sql2);

and I gained the output below:

Array
(
    [0] => EXEC sp_updateextendedproperty N'MS_Description', N'different comment', N'SCHEMA', 'dbo', N'TABLE', 'users', N'COLUMN', name
)
Array
(
)

[msyfurukawa]

I'm sorry to flip-flop, but I've tested the code below changed subtly from the above.

<?php

$config = new \Doctrine\DBAL\Configuration();
$connectionParams = array(
    'dbname' => '[DB_NAME]',
    'user' => '[USER_NAME]',
    'password' => '[PASSWORD]',
    'host' => '[HOST_NAME]',
    'port' => [PORT],
    'driver' => 'sqlsrv',
);
$conn = \Doctrine\DBAL\DriverManager::getConnection($connectionParams, $config);

$sm = $conn->getSchemaManager();
$fromSchema = $sm->createSchema();

/*
 * set same column comment and change length
 */
$toSchema1 = clone $fromSchema;
$toSchema1->getTable('users')
    ->getColumn('name')
    ->setLength(6)  // changed here!!
    ->setComment('user name');
$sql1 = $fromSchema->getMigrateToSql($toSchema1, $conn->getDatabasePlatform());
print_r($sql1);

and I get the output below this time.

Array
(
    [0] => ALTER TABLE users ALTER COLUMN name NVARCHAR(6) COLLATE SQL_Latin1_General_CP1_CI_AS NOT NULL
    [1] => EXEC sp_addextendedproperty N'MS_Description', N'user name', N'SCHEMA', 'dbo', N'TABLE', 'users', N'COLUMN', name   // this sql is unnecessary, and also causes error
)

So I think the correction of the source as I mentioned in my first comment is necessary.

[msyfurukawa]

Would you review #3416 please?

[morozov]

Fixed by #4315.

[github-actions]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.