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

Infer omitted column charset from collation #5471

Merged
merged 1 commit into from
Jul 2, 2022
Merged
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/MySQL/CollationMetadataProvider.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
<?php

declare(strict_types=1);

namespace Doctrine\DBAL\Platforms\MySQL;

/**
* @internal
*/
interface CollationMetadataProvider
{
public function getCollationCharset(string $collation): ?string;
Copy link
Member

Choose a reason for hiding this comment

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

What does null mean here? "This collation has no charset"? "We don't know about this collation"? Both? If both, should the second case result in an exception?

Copy link
Member Author

Choose a reason for hiding this comment

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

What does null mean here?

"This collation has no charset" is impossible, so it's "We don't know about this collation".

[…] should the second case result in an exception?

It doesn't have to. Should strpos() throw an exception if the substring is not found? The calling code handles null by not updating options, so if the collation is invalid, the DDL will eventually fail once it hits the database.

If we decide to throw an exception here, how do we unit-test the comparator without the database connection? Right now, we supply it with a provider that always returns null, which is fine because no unit tests rely on this feature.

Copy link
Member

Choose a reason for hiding this comment

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

It's clearer, thanks for the explanation.

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
<?php

declare(strict_types=1);

namespace Doctrine\DBAL\Platforms\MySQL\CollationMetadataProvider;

use Doctrine\DBAL\Platforms\MySQL\CollationMetadataProvider;

use function array_key_exists;

/**
* @internal
*/
final class CachingCollationMetadataProvider implements CollationMetadataProvider
{
/** @var CollationMetadataProvider */
private $collationMetadataProvider;

/** @var array<string,?string> */
private $cache = [];

public function __construct(CollationMetadataProvider $collationMetadataProvider)
{
$this->collationMetadataProvider = $collationMetadataProvider;
}

public function getCollationCharset(string $collation): ?string
{
if (array_key_exists($collation, $this->cache)) {
return $this->cache[$collation];
}

return $this->cache[$collation] = $this->collationMetadataProvider->getCollationCharset($collation);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
<?php

declare(strict_types=1);

namespace Doctrine\DBAL\Platforms\MySQL\CollationMetadataProvider;

use Doctrine\DBAL\Connection;
use Doctrine\DBAL\Exception;
use Doctrine\DBAL\Platforms\MySQL\CollationMetadataProvider;

/**
* @internal
*/
final class ConnectionCollationMetadataProvider implements CollationMetadataProvider
{
/** @var Connection */
private $connection;

public function __construct(Connection $connection)
{
$this->connection = $connection;
}

/**
* @throws Exception
*/
public function getCollationCharset(string $collation): ?string
{
$charset = $this->connection->fetchOne(
<<<'SQL'
SELECT CHARACTER_SET_NAME
FROM information_schema.COLLATIONS
WHERE COLLATION_NAME = ?;
SQL
,
[$collation]
);

if ($charset !== false) {
return $charset;
}

return null;
}
}
41 changes: 31 additions & 10 deletions src/Platforms/MySQL/Comparator.php
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,17 @@
*/
class Comparator extends BaseComparator
{
/** @var CollationMetadataProvider */
private $collationMetadataProvider;

/**
* @internal The comparator can be only instantiated by a schema manager.
*/
public function __construct(AbstractMySQLPlatform $platform)
public function __construct(AbstractMySQLPlatform $platform, CollationMetadataProvider $collationMetadataProvider)
{
parent::__construct($platform);

$this->collationMetadataProvider = $collationMetadataProvider;
}

/**
Expand All @@ -39,28 +44,44 @@ public function diffTable(Table $fromTable, Table $toTable)

private function normalizeColumns(Table $table): Table
{
$defaults = array_intersect_key($table->getOptions(), [
$tableOptions = array_intersect_key($table->getOptions(), [
'charset' => null,
'collation' => null,
]);

if ($defaults === []) {
return $table;
}

$table = clone $table;

foreach ($table->getColumns() as $column) {
$options = $column->getPlatformOptions();
$diff = array_diff_assoc($options, $defaults);
$originalOptions = $column->getPlatformOptions();
$normalizedOptions = $this->normalizeOptions($originalOptions);

if ($diff === $options) {
$overrideOptions = array_diff_assoc($normalizedOptions, $tableOptions);

if ($overrideOptions === $originalOptions) {
continue;
}

$column->setPlatformOptions($diff);
$column->setPlatformOptions($overrideOptions);
}

return $table;
}

/**
* @param array<string,string> $options
*
* @return array<string,string>
*/
private function normalizeOptions(array $options): array
{
if (isset($options['collation']) && ! isset($options['charset'])) {
$charset = $this->collationMetadataProvider->getCollationCharset($options['collation']);

if ($charset !== null) {
$options['charset'] = $charset;
}
}

return $options;
}
}
9 changes: 8 additions & 1 deletion src/Schema/MySQLSchemaManager.php
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@
use Doctrine\DBAL\Platforms\AbstractMySQLPlatform;
use Doctrine\DBAL\Platforms\MariaDb1027Platform;
use Doctrine\DBAL\Platforms\MySQL;
use Doctrine\DBAL\Platforms\MySQL\CollationMetadataProvider\CachingCollationMetadataProvider;
use Doctrine\DBAL\Platforms\MySQL\CollationMetadataProvider\ConnectionCollationMetadataProvider;
use Doctrine\DBAL\Types\Type;

use function array_change_key_case;
Expand Down Expand Up @@ -363,7 +365,12 @@ public function listTableDetails($name)

public function createComparator(): Comparator
{
return new MySQL\Comparator($this->getDatabasePlatform());
return new MySQL\Comparator(
$this->getDatabasePlatform(),
new CachingCollationMetadataProvider(
new ConnectionCollationMetadataProvider($this->_conn)
)
);
}

/**
Expand Down
22 changes: 22 additions & 0 deletions tests/Functional/Schema/MySQL/ComparatorTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,28 @@ public function testChangeColumnCollation(): void
ComparatorTestUtils::assertDiffNotEmpty($this->connection, $this->comparator, $table);
}

public function testImplicitColumnCharset(): void
{
$table = new Table('comparator_test');
$table->addColumn('name', Types::STRING, [
'length' => 32,
'platformOptions' => ['collation' => 'utf8mb4_unicode_ci'],
]);
$this->dropAndCreateTable($table);

self::assertFalse(ComparatorTestUtils::diffFromActualToDesiredTable(
$this->schemaManager,
$this->comparator,
$table
));

self::assertFalse(ComparatorTestUtils::diffFromDesiredToActualTable(
$this->schemaManager,
$this->comparator,
$table
));
}

/**
* @return array{Table,Column}
*
Expand Down
18 changes: 16 additions & 2 deletions tests/Platforms/AbstractMySQLPlatformTestCase.php
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

use Doctrine\DBAL\Platforms\AbstractPlatform;
use Doctrine\DBAL\Platforms\MySQL;
use Doctrine\DBAL\Platforms\MySQL\CollationMetadataProvider;
use Doctrine\DBAL\Platforms\MySQLPlatform;
use Doctrine\DBAL\Schema\Comparator;
use Doctrine\DBAL\Schema\ForeignKeyConstraint;
Expand Down Expand Up @@ -795,7 +796,12 @@ public function testIgnoresDifferenceInDefaultValuesForUnsupportedColumnTypes():
$diffTable->changeColumn('def_blob', ['default' => null]);
$diffTable->changeColumn('def_blob_null', ['default' => null]);

self::assertFalse((new MySQL\Comparator($this->platform))->diffTable($table, $diffTable));
$comparator = new MySQL\Comparator(
$this->platform,
$this->createMock(CollationMetadataProvider::class)
);

self::assertFalse($comparator->diffTable($table, $diffTable));
}

/**
Expand Down Expand Up @@ -1052,7 +1058,15 @@ public static function comparatorProvider(): iterable
];

yield 'MySQL comparator' => [
new MySQL\Comparator(new MySQLPlatform()),
new MySQL\Comparator(
new MySQLPlatform(),
new class implements CollationMetadataProvider {
public function getCollationCharset(string $collation): ?string
{
return null;
}
}
),
];
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
<?php

declare(strict_types=1);

namespace Doctrine\DBAL\Tests\Platforms\MySQL\CollationMetadataProvider;

use Doctrine\DBAL\Platforms\MySQL\CollationMetadataProvider;
use Doctrine\DBAL\Platforms\MySQL\CollationMetadataProvider\CachingCollationMetadataProvider;
use PHPUnit\Framework\TestCase;

class CachingCollationMetadataProviderTest extends TestCase
{
/**
* @dataProvider charsetAndCollationProvider
*/
public function testCharsetCaching(string $collation, ?string $charset): void
{
$underlyingProvider = $this->createMock(CollationMetadataProvider::class);
$underlyingProvider->expects(self::once())
->method('getCollationCharset')
->with($collation)
->willReturn($charset);

$cachingProvider = new CachingCollationMetadataProvider($underlyingProvider);
self::assertSame($charset, $cachingProvider->getCollationCharset($collation));
self::assertSame($charset, $cachingProvider->getCollationCharset($collation));
}

/**
* @return iterable<string,array{string,?string}>
*/
public static function charsetAndCollationProvider(): iterable
{
yield 'found' => ['utf8mb4_unicode_ci', 'utf8mb4'];
yield 'not found' => ['utf8mb5_unicode_ci', null];
}
}
11 changes: 10 additions & 1 deletion tests/Platforms/MySQL/ComparatorTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

namespace Doctrine\DBAL\Tests\Platforms\MySQL;

use Doctrine\DBAL\Platforms\MySQL\CollationMetadataProvider;
use Doctrine\DBAL\Platforms\MySQL\Comparator;
use Doctrine\DBAL\Platforms\MySQLPlatform;
use Doctrine\DBAL\Tests\Schema\ComparatorTest as BaseComparatorTest;
Expand All @@ -10,6 +11,14 @@ class ComparatorTest extends BaseComparatorTest
{
protected function setUp(): void
{
$this->comparator = new Comparator(new MySQLPlatform());
$this->comparator = new Comparator(
new MySQLPlatform(),
new class implements CollationMetadataProvider {
public function getCollationCharset(string $collation): ?string
{
return null;
}
}
);
}
}
11 changes: 10 additions & 1 deletion tests/Schema/Platforms/MySQLSchemaTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

use Doctrine\DBAL\Platforms\AbstractPlatform;
use Doctrine\DBAL\Platforms\MySQL;
use Doctrine\DBAL\Platforms\MySQL\CollationMetadataProvider;
use Doctrine\DBAL\Platforms\MySQLPlatform;
use Doctrine\DBAL\Schema\Comparator;
use Doctrine\DBAL\Schema\Table;
Expand Down Expand Up @@ -99,7 +100,15 @@ public static function comparatorProvider(): iterable
];

yield 'MySQL comparator' => [
new MySQL\Comparator(new MySQLPlatform()),
new MySQL\Comparator(
new MySQLPlatform(),
new class implements CollationMetadataProvider {
public function getCollationCharset(string $collation): ?string
{
return null;
}
}
),
];
}
}