Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: quote namespace names #6652

Open
wants to merge 1 commit into
base: 4.2.x
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions src/Platforms/AbstractPlatform.php
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@
use function key;
use function max;
use function mb_strlen;
use function preg_match;
use function preg_quote;
use function preg_replace;
use function sprintf;
Expand Down Expand Up @@ -1038,6 +1039,10 @@ public function getAlterSchemaSQL(SchemaDiff $diff): array
foreach ($diff->getCreatedSchemas() as $schema) {
$sql[] = $this->getCreateSchemaSQL($schema);
}

foreach ($diff->getDroppedSchemas() as $schema) {
$sql[] = $this->getDropSchemaSQL($schema);
}
}

if ($this->supportsSequences()) {
Expand Down Expand Up @@ -1162,6 +1167,10 @@ public function getCreateSchemaSQL(string $schemaName): string
throw NotSupported::new(__METHOD__);
}

if (preg_match('/^\d/', $schemaName) > 0) {
Copy link
Member

@morozov morozov Dec 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even though it's not consistently documented, all get[...]SQL() methods of this class accept SQL fragments as string parameters and just concatenate them with other fragments of SQL. Treating the schema name as a literal is against this implied contract. It's the caller of the method who should quote the names that need to be quoted.

Depending on how you manage your schema via DBAL, you may try explicitly quoting names in the schema definition similar to this:

public function testQuotedTableNameRemainsQuotedInSchema(): void
{
$table = new Table('"tester"');
$table->addColumn('"id"', Types::INTEGER);
$table->addColumn('"name"', Types::STRING, ['length' => 32]);

This won't be necessary in DBAL 5.0 (see #6589).

Copy link
Member

@morozov morozov Dec 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Within the current API, a proper solution would be to assume all object names obtained by introspecting the database quoted, but schema names are returned as strings, so there's no way to indicate that besides wrapping all names in quotes. Doing that will fix this bug but will be a breaking change, so we can do that only in a major release.

return 'CREATE SCHEMA ' . $this->quoteIdentifier($schemaName);
}

return 'CREATE SCHEMA ' . $schemaName;
}

Expand All @@ -1183,6 +1192,10 @@ public function getDropSchemaSQL(string $schemaName): string
throw NotSupported::new(__METHOD__);
}

if (preg_match('/^\d/', $schemaName) > 0) {
return 'DROP SCHEMA ' . $this->quoteIdentifier($schemaName);
}

return 'DROP SCHEMA ' . $schemaName;
}

Expand Down
5 changes: 4 additions & 1 deletion src/Schema/AbstractAsset.php
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
use function dechex;
use function explode;
use function implode;
use function preg_match;
use function str_contains;
use function str_replace;
use function strtolower;
Expand Down Expand Up @@ -131,7 +132,9 @@ public function getQuotedName(AbstractPlatform $platform): string
$keywords = $platform->getReservedKeywordsList();
$parts = explode('.', $this->getName());
foreach ($parts as $k => $v) {
$parts[$k] = $this->_quoted || $keywords->isKeyword($v) ? $platform->quoteIdentifier($v) : $v;
$shouldBeQuoted = $this->_quoted || $keywords->isKeyword($v) || preg_match('/^\d/', $v) !== 0;

$parts[$k] = $shouldBeQuoted ? $platform->quoteIdentifier($v) : $v;
}

return implode('.', $parts);
Expand Down
19 changes: 19 additions & 0 deletions src/Schema/Exception/NamespaceDoesNotExist.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
<?php

declare(strict_types=1);

namespace Doctrine\DBAL\Schema\Exception;

use Doctrine\DBAL\Schema\SchemaException;
use LogicException;

use function sprintf;

/** @psalm-immutable */
final class NamespaceDoesNotExist extends LogicException implements SchemaException
{
public static function new(string $namespaceName): self

Check warning on line 15 in src/Schema/Exception/NamespaceDoesNotExist.php

View check run for this annotation

Codecov / codecov/patch

src/Schema/Exception/NamespaceDoesNotExist.php#L15

Added line #L15 was not covered by tests
{
return new self(sprintf('There is no namespace with name "%s" in the schema.', $namespaceName));

Check warning on line 17 in src/Schema/Exception/NamespaceDoesNotExist.php

View check run for this annotation

Codecov / codecov/patch

src/Schema/Exception/NamespaceDoesNotExist.php#L17

Added line #L17 was not covered by tests
}
}
19 changes: 19 additions & 0 deletions src/Schema/Schema.php
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
use Doctrine\DBAL\Exception;
use Doctrine\DBAL\Platforms\AbstractPlatform;
use Doctrine\DBAL\Schema\Exception\NamespaceAlreadyExists;
use Doctrine\DBAL\Schema\Exception\NamespaceDoesNotExist;
use Doctrine\DBAL\Schema\Exception\SequenceAlreadyExists;
use Doctrine\DBAL\Schema\Exception\SequenceDoesNotExist;
use Doctrine\DBAL\Schema\Exception\TableAlreadyExists;
Expand Down Expand Up @@ -267,6 +268,24 @@
return $this;
}

/**
* Drops a new namespace from the schema.
*
* @return $this
*/
public function dropNamespace(string $name): self
{
$unquotedName = strtolower($this->getUnquotedAssetName($name));

if (! isset($this->namespaces[$unquotedName])) {
throw NamespaceDoesNotExist::new($unquotedName);

Check warning on line 281 in src/Schema/Schema.php

View check run for this annotation

Codecov / codecov/patch

src/Schema/Schema.php#L281

Added line #L281 was not covered by tests
}

unset($this->namespaces[$unquotedName]);

return $this;
}

/**
* Creates a new table.
*/
Expand Down
6 changes: 2 additions & 4 deletions tests/Functional/Schema/PostgreSQLSchemaManagerTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -336,12 +336,10 @@ protected function assertVarBinaryColumnIsValid(Table $table, string $columnName
/**
* Although this test would pass in isolation on any platform, we keep it here for the following reasons:
*
* 1. The DBAL currently doesn't properly drop tables in the namespaces that need to be quoted
* (@see testListTableDetailsWhenCurrentSchemaNameQuoted()).
* 2. The schema returned by {@see AbstractSchemaManager::introspectSchema()} doesn't contain views, so
* 1. The schema returned by {@see AbstractSchemaManager::introspectSchema()} doesn't contain views, so
* {@see AbstractSchemaManager::dropSchemaObjects()} cannot drop the tables that have dependent views
* (@see testListTablesExcludesViews()).
* 3. In the case of inheritance, PHPUnit runs the tests declared immediately in the test class
* 2. In the case of inheritance, PHPUnit runs the tests declared immediately in the test class
* and then runs the tests declared in the parent.
*
* This test needs to be executed before the ones it conflicts with, so it has to be declared in the same class.
Expand Down
47 changes: 47 additions & 0 deletions tests/Functional/Schema/SchemaManagerFunctionalTestCase.php
Original file line number Diff line number Diff line change
Expand Up @@ -1419,6 +1419,53 @@ public function testDropColumnWithDefault(): void
self::assertCount(1, $columns);
}

public function testCanCreateAndDropSchemaThatNeedToBeQuoted(): void
{
$platform = $this->connection->getDatabasePlatform();

if (! $platform->supportsSchemas()) {
self::markTestSkipped('The platform does not support schema/namespaces.');
}

$schemaManager = $this->connection->createSchemaManager();
$schema = $schemaManager->introspectSchema();

$schema->createNamespace('001_schema');

$schemaManager = $this->connection->createSchemaManager();
$schemaManager->migrateSchema($schema);
self::assertContains('001_schema', $schemaManager->listSchemaNames());

$schema->dropNamespace('001_schema');
$schemaManager->migrateSchema($schema);
self::assertNotContains('001_schema', $schemaManager->listSchemaNames());
}

public function testCanCreateAndDropTableFromNamespaceThatNeedToBeQuoted(): void
{
$platform = $this->connection->getDatabasePlatform();

if (! $platform->supportsSchemas()) {
self::markTestSkipped('The platform does not support schema/namespaces.');
}

$schemaManager = $this->connection->createSchemaManager();
$schema = $schemaManager->introspectSchema();

$table = $schema->createTable('001_schema.test_quoted_schema');
$table->addColumn('id', Types::INTEGER, ['notnull' => true]);
$table->setPrimaryKey(['id']);

$schemaManager = $this->connection->createSchemaManager();
$schemaManager->migrateSchema($schema);
self::assertContains('001_schema', $schemaManager->listSchemaNames());
self::assertContains('001_schema.test_quoted_schema', $schemaManager->listTableNames());

$schema->dropTable('001_schema.test_quoted_schema');
$schemaManager->migrateSchema($schema);
self::assertNotContains('001_schema.test_quoted_schema', $schemaManager->listTableNames());
}

/** @param list<Table> $tables */
protected function findTableByName(array $tables, string $name): ?Table
{
Expand Down