Skip to content

Commit

Permalink
Fix SQL Server "extended property" SQL generation (#6353)
Browse files Browse the repository at this point in the history 
|      Q       |   A
|------------- | -----------
| Type         | bug
| Fixed issues | #4283

#### Summary


https://learn.microsoft.com/en-us/sql/relational-databases/system-stored-procedures/sp-addextendedproperty-transact-sql?view=sql-server-ver15
shows the `sp_addextendedproperty` accepts `sysname` datatype which is
subtype of string datatype and therefore the values must be escaped as
string literal instead of escaped identifier. This is evident also in
the
[examples](https://learn.microsoft.com/en-us/sql/relational-databases/system-stored-procedures/sp-addextendedproperty-transact-sql?view=sql-server-ver15#examples).

As the DBAL method arguments accepts possibly escaped identifier, we
unescape it before we escape the argument as string literal.
  • Loading branch information
@mvorisek
mvorisek authored May 3, 2024
1 parent bc75742 commit 40e7b7c
Show file tree
Hide file tree
Showing 3 changed files with 105 additions and 84 deletions.
96 changes: 52 additions & 44 deletions src/Platforms/SQLServerPlatform.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,12 @@
use function preg_match;
use function preg_match_all;
use function sprintf;
use function str_ends_with;
use function str_replace;
use function str_starts_with;
use function strpos;
use function strtoupper;
use function substr;
use function substr_count;

use const PREG_OFFSET_CAPTURE;
Expand Down Expand Up @@ -399,6 +402,13 @@ public function getCreatePrimaryKeySQL(Index $index, $table)
return $sql . ' (' . $this->getIndexFieldDeclarationListSQL($index) . ')';
}

private function unquoteSingleIdentifier(string $possiblyQuotedName): string
{
return str_starts_with($possiblyQuotedName, '[') && str_ends_with($possiblyQuotedName, ']')
? substr($possiblyQuotedName, 1, -1)
: $possiblyQuotedName;
}

/**
* Returns the SQL statement for creating a column comment.
*
Expand All @@ -419,23 +429,20 @@ public function getCreatePrimaryKeySQL(Index $index, $table)
protected function getCreateColumnCommentSQL($tableName, $columnName, $comment)
{
if (strpos($tableName, '.') !== false) {
[$schemaSQL, $tableSQL] = explode('.', $tableName);
$schemaSQL = $this->quoteStringLiteral($schemaSQL);
$tableSQL = $this->quoteStringLiteral($tableSQL);
[$schemaName, $tableName] = explode('.', $tableName);
} else {
$schemaSQL = "'dbo'";
$tableSQL = $this->quoteStringLiteral($tableName);
$schemaName = 'dbo';
}

return $this->getAddExtendedPropertySQL(
'MS_Description',
$comment,
'SCHEMA',
$schemaSQL,
$this->quoteStringLiteral($this->unquoteSingleIdentifier($schemaName)),
'TABLE',
$tableSQL,
$this->quoteStringLiteral($this->unquoteSingleIdentifier($tableName)),
'COLUMN',
$columnName,
$this->quoteStringLiteral($this->unquoteSingleIdentifier($columnName)),
);
}

Expand Down Expand Up @@ -806,23 +813,20 @@ private function alterColumnRequiresDropDefaultConstraint(ColumnDiff $columnDiff
protected function getAlterColumnCommentSQL($tableName, $columnName, $comment)
{
if (strpos($tableName, '.') !== false) {
[$schemaSQL, $tableSQL] = explode('.', $tableName);
$schemaSQL = $this->quoteStringLiteral($schemaSQL);
$tableSQL = $this->quoteStringLiteral($tableSQL);
[$schemaName, $tableName] = explode('.', $tableName);
} else {
$schemaSQL = "'dbo'";
$tableSQL = $this->quoteStringLiteral($tableName);
$schemaName = 'dbo';
}

return $this->getUpdateExtendedPropertySQL(
'MS_Description',
$comment,
'SCHEMA',
$schemaSQL,
$this->quoteStringLiteral($this->unquoteSingleIdentifier($schemaName)),
'TABLE',
$tableSQL,
$this->quoteStringLiteral($this->unquoteSingleIdentifier($tableName)),
'COLUMN',
$columnName,
$this->quoteStringLiteral($this->unquoteSingleIdentifier($columnName)),
);
}

Expand All @@ -845,22 +849,19 @@ protected function getAlterColumnCommentSQL($tableName, $columnName, $comment)
protected function getDropColumnCommentSQL($tableName, $columnName)
{
if (strpos($tableName, '.') !== false) {
[$schemaSQL, $tableSQL] = explode('.', $tableName);
$schemaSQL = $this->quoteStringLiteral($schemaSQL);
$tableSQL = $this->quoteStringLiteral($tableSQL);
[$schemaName, $tableName] = explode('.', $tableName);
} else {
$schemaSQL = "'dbo'";
$tableSQL = $this->quoteStringLiteral($tableName);
$schemaName = 'dbo';
}

return $this->getDropExtendedPropertySQL(
'MS_Description',
'SCHEMA',
$schemaSQL,
$this->quoteStringLiteral($this->unquoteSingleIdentifier($schemaName)),
'TABLE',
$tableSQL,
$this->quoteStringLiteral($this->unquoteSingleIdentifier($tableName)),
'COLUMN',
$columnName,
$this->quoteStringLiteral($this->unquoteSingleIdentifier($columnName)),
);
}

Expand Down Expand Up @@ -907,10 +908,13 @@ public function getAddExtendedPropertySQL(
$level2Name = null
) {
return 'EXEC sp_addextendedproperty ' .
'N' . $this->quoteStringLiteral($name) . ', N' . $this->quoteStringLiteral((string) $value) . ', ' .
'N' . $this->quoteStringLiteral((string) $level0Type) . ', ' . $level0Name . ', ' .
'N' . $this->quoteStringLiteral((string) $level1Type) . ', ' . $level1Name . ', ' .
'N' . $this->quoteStringLiteral((string) $level2Type) . ', ' . $level2Name;
'N' . $this->quoteStringLiteral($name) . ', N' . $this->quoteStringLiteral($value ?? '') . ', ' .
'N' . $this->quoteStringLiteral($level0Type ?? '') . ', ' . $level0Name . ', ' .
'N' . $this->quoteStringLiteral($level1Type ?? '') . ', ' . $level1Name .
($level2Type !== null || $level2Name !== null
? ', N' . $this->quoteStringLiteral($level2Type ?? '') . ', ' . $level2Name
: ''
);
}

/**
Expand Down Expand Up @@ -941,9 +945,12 @@ public function getDropExtendedPropertySQL(
) {
return 'EXEC sp_dropextendedproperty ' .
'N' . $this->quoteStringLiteral($name) . ', ' .
'N' . $this->quoteStringLiteral((string) $level0Type) . ', ' . $level0Name . ', ' .
'N' . $this->quoteStringLiteral((string) $level1Type) . ', ' . $level1Name . ', ' .
'N' . $this->quoteStringLiteral((string) $level2Type) . ', ' . $level2Name;
'N' . $this->quoteStringLiteral($level0Type ?? '') . ', ' . $level0Name . ', ' .
'N' . $this->quoteStringLiteral($level1Type ?? '') . ', ' . $level1Name .
($level2Type !== null || $level2Name !== null
? ', N' . $this->quoteStringLiteral($level2Type ?? '') . ', ' . $level2Name
: ''
);
}

/**
Expand Down Expand Up @@ -975,10 +982,13 @@ public function getUpdateExtendedPropertySQL(
$level2Name = null
) {
return 'EXEC sp_updateextendedproperty ' .
'N' . $this->quoteStringLiteral($name) . ', N' . $this->quoteStringLiteral((string) $value) . ', ' .
'N' . $this->quoteStringLiteral((string) $level0Type) . ', ' . $level0Name . ', ' .
'N' . $this->quoteStringLiteral((string) $level1Type) . ', ' . $level1Name . ', ' .
'N' . $this->quoteStringLiteral((string) $level2Type) . ', ' . $level2Name;
'N' . $this->quoteStringLiteral($name) . ', N' . $this->quoteStringLiteral($value ?? '') . ', ' .
'N' . $this->quoteStringLiteral($level0Type ?? '') . ', ' . $level0Name . ', ' .
'N' . $this->quoteStringLiteral($level1Type ?? '') . ', ' . $level1Name .
($level2Type !== null || $level2Name !== null
? ', N' . $this->quoteStringLiteral($level2Type ?? '') . ', ' . $level2Name
: ''
);
}

/**
Expand Down Expand Up @@ -1765,15 +1775,13 @@ private function generateIdentifierName($identifier): string

protected function getCommentOnTableSQL(string $tableName, ?string $comment): string
{
return sprintf(
<<<'SQL'
EXEC sys.sp_addextendedproperty @name=N'MS_Description',
@value=N%s, @level0type=N'SCHEMA', @level0name=N'dbo',
@level1type=N'TABLE', @level1name=N%s
SQL
,
$this->quoteStringLiteral((string) $comment),
$this->quoteStringLiteral($tableName),
return $this->getAddExtendedPropertySQL(
'MS_Description',
$comment,
'SCHEMA',
$this->quoteStringLiteral('dbo'),
'TABLE',
$this->quoteStringLiteral($this->unquoteSingleIdentifier($tableName)),
);
}

Expand Down
23 changes: 18 additions & 5 deletions tests/Functional/Schema/SQLServerSchemaManagerTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -97,10 +97,13 @@ public function testDefaultConstraints(): void
self::assertEquals(666, $columns['df_integer']->getDefault());
}

/** @psalm-suppress DeprecatedConstant */
public function testColumnComments(): void
/**
* @dataProvider columnCommentsProvider
* @psalm-suppress DeprecatedConstant
*/
public function testColumnComments(string $tableName): void
{
$table = new Table('sqlsrv_column_comment');
$table = new Table($tableName);
$table->addColumn('id', Types::INTEGER, ['autoincrement' => true]);
$table->addColumn('comment_null', Types::INTEGER, ['comment' => null]);
$table->addColumn('comment_false', Types::INTEGER, ['comment' => false]);
Expand Down Expand Up @@ -130,7 +133,7 @@ public function testColumnComments(): void

$this->schemaManager->createTable($table);

$columns = $this->schemaManager->listTableColumns('sqlsrv_column_comment');
$columns = $this->schemaManager->listTableColumns($tableName);
self::assertCount(13, $columns);
self::assertNull($columns['id']->getComment());
self::assertNull($columns['comment_null']->getComment());
Expand Down Expand Up @@ -209,7 +212,7 @@ public function testColumnComments(): void

$this->schemaManager->alterTable($diff);

$columns = $this->schemaManager->listTableColumns('sqlsrv_column_comment');
$columns = $this->schemaManager->listTableColumns($tableName);
self::assertCount(24, $columns);
self::assertEquals('primary', $columns['id']->getComment());
self::assertNull($columns['comment_null']->getComment());
Expand Down Expand Up @@ -237,6 +240,16 @@ public function testColumnComments(): void
self::assertEquals('Some comment', $columns['commented_req_change_column']->getComment());
}

/** @return mixed[][] */
public static function columnCommentsProvider(): iterable
{
return [
'Simple table name' => ['sqlsrv_column_comment'],
'Quoted table name' => ['[sqlsrv_column_comment quoted]'],
'Quoted table name with schema' => ['[dbo].[sqlsrv_column_comment " with_schema]'],
];
}

public function testPkOrdering(): void
{
// SQL Server stores index column information in a system table with two
Expand Down
Loading

0 comments on commit 40e7b7c

Please sign in to comment.