Fix: #6146 - utf8/utf8mb3 alias causes redundant ALTER TABLE changes #6370

fisharebest · 2024-04-29T22:07:09Z

Q	A
Type	bug
Fixed issues	#6146

Summary

In MySQL (and MariaDB), utf8 and utf8mb3 are aliases. Depending on the database server version, the information schema may contain one or the other. Similarly, column definitions may contain one or the other.

When performing schema-comparisons, a mismatch between utf8 and utf8mb3 causes an ALTER TABLE statement to be generated - even when columns are identical.

The code already contains a function to normalize the charset/collation options:

dbal/src/Platforms/MySQL/Comparator.php

Line 83 in 61d79c6

private function normalizeOptions(array $options): array

My solution is to extend this function so that it also replaces utf8 with utf8mb3 (or vice-versa), to match the current database connection.

To detect the prefered character set for the connection, I've added:
AbstractMySQLPlatform::informationSchemaUsesUtf8mb3()
MariaDBPlatform::informationSchemaUsesUtf8mb3()

The obvious place for the new normalization logic is the CharsetMetadataProvider and CollationMetadataProvider classes. These are interfaces, so this is technically a BC break, however, they are marked @internal.

I've added tests which should cover all the new/modified code. I've run it against MySQL. I don't have MariaDB to hand.

fisharebest · 2024-04-30T08:20:43Z

Just pushed a change to fix the psalm issues

greg0ire · 2024-04-30T08:26:42Z

It seems there are CI jobs failing. Please take a look at this guide for more on how to handle those.

greg0ire · 2024-04-30T08:57:01Z

ಠ_ಠ

greg0ire · 2024-05-01T08:29:02Z

I've added tests which should cover all the new/modified code. I've run it against MySQL. I don't have MariaDB to hand.

Codecov says that some of the new code is untested, most notably the parts using version_compare. Please add functional tests for this change. As you can see, our test suite features, among other platforms MariaDB 10.5 and MariaDB 10.6, so you should be able to test that schema comparisons on either version of MariaDB indeed doesn't cause ALTER TABLE statements to be generated. Same goes for MySQL.

You can learn more about such tests at https://www.doctrine-project.org/projects/doctrine-dbal/en/stable/reference/testing.html

fisharebest · 2024-05-01T13:40:46Z

Codecov says that some of the new code is untested, most notably the parts using version_compare. Please add functional tests for this change.

Running the tests locally says the code is covered. (Ad hoc testing confirms this).

XDEBUG_MODE=coverage vendor/bin/phpunit -c ci/github/phpunit/pdo_mysql.xml --coverage-html=coverage

However, Codecov says it isn't.

greg0ire · 2024-05-01T14:29:57Z

I think I found the reason: [2024-05-01T09:48:28.648Z] ['error'] There was an error running the uploader: Error uploading to [https://codecov.io:](https://codecov.io/) Error: There was an error fetching the storage URL during POST: 429 - {'detail': ErrorDetail(string='Rate limit reached. Please upload with the Codecov repository upload token to resolve issue. Expected available in 1935 seconds.', code='throttled')}

Let me retry this.

In a recent PR, we got reports from CodeCov about several lines not being covered. After further inspection, I found an error message in the coverage file upload saying we hit a rate limit. This will make it more likely we spot such issues in the future. The PR: doctrine#6370

greg0ire · 2024-05-01T14:42:05Z

From the Codecov UI:

Coverage data is based on head 9c87c57(2 uploads) compared to base 7fb00fd(5 uploads)

… we currently send 69 uploads, so there is no way we are going to be below the upload limit. I will have to look into this separately.

greg0ire

Looks good overall, my feedback is not a blocker.

greg0ire · 2024-05-01T14:46:50Z

src/Platforms/MySQL/CollationMetadataProvider/ConnectionCollationMetadataProvider.php

+    public function normalizeCollation(string $collation): string
+    {
+        if ($this->useUtf8mb3 && str_starts_with($collation, 'utf8_')) {
+            return 'utf8mb3' . substr($collation, 4);


I'd make things more obvious here:

Suggested change

return 'utf8mb3' . substr($collation, 4);

return 'utf8mb3' . substr($collation, strlen('utf8'));

greg0ire · 2024-05-01T14:47:07Z

src/Platforms/MySQL/CollationMetadataProvider/ConnectionCollationMetadataProvider.php

+        }
+
+        if (! $this->useUtf8mb3 && str_starts_with($collation, 'utf8mb3_')) {
+            return 'utf8' . substr($collation, 7);


Suggested change

return 'utf8' . substr($collation, 7);

return 'utf8' . substr($collation, strlen('utf8mb3'));

greg0ire · 2024-05-01T14:51:26Z

Ah there is one issue though: since this is a fix, you should target 3.8.x

fisharebest · 2024-05-01T15:46:12Z

Ah there is one issue though: since this is a fix, you should target 3.8.x

I can create another PR for this branch.

greg0ire · 2024-05-01T16:35:57Z

Please don't, we usually do merges up. Instead, let me retarget your PR, and please rebase your commits on 3.8.x and force push.

The base branch was changed.

In a recent PR, we got reports from CodeCov about several lines not being covered. After further inspection, I found an error message in the coverage file upload saying we hit a rate limit. This will make it more likely we spot such issues in the future. The PR: doctrine#6370

fisharebest · 2024-05-02T15:35:37Z

Please don't, we usually do merges up. Instead, let me retarget your PR, and please rebase your commits on 3.8.x and force push.

The PR uses Connection::getServerVersion() to select the prefered alias for utf8/utf8mb3.
In 3.8.x, this method isn't available, so I'll need to find another approach.

I'm going to be offline for the next week or so, and will look at this on my return.

greg0ire · 2024-05-02T17:48:33Z

Enjoy your time off 👍

greg0ire · 2024-06-04T11:35:02Z

@fisharebest do you need help with the rebase?

fisharebest · 2024-06-04T20:31:45Z

I have rewritten my patch to use the information_schema instead of server version numbers. This seems more robust, and it should also continue to work if MySQL remove utf8mb3 in version 9.

However, I have been unable to back-port this PR to the 3.8.x branch. (I can, but it doesn't fix the bug...).

The handling of charset/collation is different. I'd probably need to copy lots of other changes from the 4.0.x branch to make it work.

Options are:

apply this patch to 4.0.x only
attempt to backport the changes in charset/collation handling from 4.0.x to 3.8.x and then try again
????

greg0ire · 2024-06-04T21:04:26Z

I'd be surprised if there were lots of changes you could backport that are not breaking changes, so I'd say we should consider applying this to 4.0.x although given the usage statistics of 4.0.x, it won't have much effect at first.

arbor95

All seems ok

SenseException · 2024-07-27T20:01:13Z

I have rewritten my patch to use the information_schema instead of server version numbers. This seems more robust, and it should also continue to work if MySQL remove utf8mb3 in version 9.

As far as I understand this, utf8 would stay utf8 once utf8mb3 isn't supported anymore. I didn't find information about when this alias handling was introduced on a short search though.

fisharebest · 2024-08-14T13:09:03Z

it should also continue to work if MySQL remove utf8mb3 in version 9.

Now that MySQL 9.0 is released, we can see that the logic still works, and the tests pass.

derrabus · 2024-08-14T13:33:46Z

Can you please rebase onto 4.1? That should trigger a CI pipeline that includes MySQL 9.

…changes

fisharebest · 2024-09-04T11:53:30Z

All the tests are passing. Looks like a missing token in the CI tools.

greg0ire · 2024-09-04T12:05:52Z

You can ignore the Codecov failure, that issue is tracked here: codecov/codecov-action#1548

derrabus · 2024-09-04T12:09:30Z

Yes sorry, we lost track of this PR, apparently. I'll do a review of the PR.

derrabus · 2024-09-04T12:17:37Z

src/Platforms/AbstractMySQLPlatform.php

+    /**
+     * INFORMATION_SCHEMA.CHARACTER_SETS will contain either 'utf8' or 'utf8mb3' but not both.
+     */
+    public function informationSchemaUsesUtf8mb3(Connection $connection): bool
+    {
+        $sql = "SELECT character_set_name FROM INFORMATION_SCHEMA.CHARACTER_SETS WHERE character_set_name = 'utf8mb3'";
+
+        return $connection->fetchOne($sql) === 'utf8mb3';
+    }


I'm having my issues with this method.

First of all, it is highly unusual that our platform classes interact with the connection, in this case send a query to the server. Our platform classes are meant to collect and normalize differences between database systems and versions. Since you don't ever override this method, there's no reason why this method should exist on a platform class.

Moreover, you wrote that…

In MySQL (and MariaDB), utf8 and utf8mb3 are aliases. Depending on the database server version, the information schema may contain one or the other. Similarly, column definitions may contain one or the other.

If this behavior really depends on the database version and taking into account that we know the database version: Why do we need to perform feature detection by running a query on the server?

derrabus · 2024-09-04T12:31:26Z

src/Platforms/MySQL/CharsetMetadataProvider/ConnectionCharsetMetadataProvider.php

+    public function normalizeCharset(string $charset): string
+    {
+        if ($this->useUtf8mb3 && $charset === 'utf8') {
+            return 'utf8mb3';
+        }
+
+        if (! $this->useUtf8mb3 && $charset === 'utf8mb3') {
+            return 'utf8';
+        }
+
+        return $charset;
+    }


I don't think that CharsetMetadataProvider is the right abstraction for this logic. The purpose of CharsetMetadataProvider is to query a piece of configuration from the server in a cachable manner. A normalization method is something entirely different.

The more I think about it: This normalization logic belongs into the platform classes. And if you put it there, you don't need that detection logic of yours and you don't that useUtf8mb3 flag. Give it a try. 🙂

derrabus · 2024-09-04T12:33:19Z

tests/Platforms/MySQL/CollationMetadataProvider/ConnectionCharsetMetadataProviderTest.php

+        $utf8mb3Provider = new ConnectionCharsetMetadataProvider($connection, true);
+
+        self::assertSame('utf8mb3', $utf8mb3Provider->normalizeCharset('utf8'));
+        self::assertSame('utf8mb3', $utf8mb3Provider->normalizeCharset('utf8mb3'));
+        self::assertSame('foobar', $utf8mb3Provider->normalizeCharset('foobar'));


This should be a separate test.

derrabus · 2024-09-04T12:33:42Z

tests/Platforms/MySQL/CollationMetadataProvider/ConnectionCollationMetadataProviderTest.php

+        $utf8mb3Provider = new ConnectionCollationMetadataProvider($connection, true);
+
+        self::assertSame('utf8mb3_unicode_ci', $utf8mb3Provider->normalizeCollation('utf8_unicode_ci'));
+        self::assertSame('utf8mb3_unicode_ci', $utf8mb3Provider->normalizeCollation('utf8mb3_unicode_ci'));
+        self::assertSame('foobar_unicode_ci', $utf8mb3Provider->normalizeCollation('foobar_unicode_ci'));


This should be a separate test.

fisharebest force-pushed the fix-6146 branch from 793f09e to fa707e2 Compare April 30, 2024 08:33

fisharebest force-pushed the fix-6146 branch from fa707e2 to 83f4788 Compare April 30, 2024 09:04

fisharebest force-pushed the fix-6146 branch from 61de19b to 9c87c57 Compare May 1, 2024 09:39

greg0ire previously approved these changes May 1, 2024

View reviewed changes

greg0ire requested review from SenseException and derrabus May 1, 2024 14:50

greg0ire mentioned this pull request May 1, 2024

Set fail_ci_if_error flag to true #6372

Merged

greg0ire changed the base branch from 4.0.x to 3.8.x May 1, 2024 16:36

fisharebest added a commit to fisharebest/dbal that referenced this pull request Jun 4, 2024

Backport doctrine#6370 into 3.8.x

9f0e65a

fisharebest changed the base branch from 3.8.x to 4.0.x June 4, 2024 21:13

fisharebest force-pushed the fix-6146 branch from 360b326 to fb2b723 Compare June 4, 2024 21:13

arbor95 approved these changes Jul 26, 2024

View reviewed changes

derrabus changed the base branch from 4.0.x to 4.1.x August 14, 2024 11:03

derrabus added Bug MySQL labels Aug 14, 2024

fisharebest added 2 commits August 14, 2024 14:37

Fix: doctrine#6146 - utf8/utf8mb3 alias causes redundant ALTER TABLE …

6a48c66

…changes

getModifiedColumns() is deprecated - use getChangedColumns() instead

7effa44

fisharebest force-pushed the fix-6146 branch from fb2b723 to 7effa44 Compare August 14, 2024 13:50

derrabus requested changes Sep 4, 2024

View reviewed changes

derrabus changed the base branch from 4.1.x to 4.2.x October 10, 2024 13:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: #6146 - utf8/utf8mb3 alias causes redundant ALTER TABLE changes #6370

Fix: #6146 - utf8/utf8mb3 alias causes redundant ALTER TABLE changes #6370

fisharebest commented Apr 29, 2024 •

edited

Loading

fisharebest commented Apr 30, 2024

greg0ire commented Apr 30, 2024

greg0ire commented Apr 30, 2024

greg0ire commented May 1, 2024 •

edited

Loading

fisharebest commented May 1, 2024

greg0ire commented May 1, 2024 •

edited

Loading

greg0ire commented May 1, 2024

greg0ire left a comment

greg0ire May 1, 2024

greg0ire May 1, 2024

greg0ire commented May 1, 2024

fisharebest commented May 1, 2024

greg0ire commented May 1, 2024 •

edited

Loading

fisharebest commented May 2, 2024

greg0ire commented May 2, 2024

greg0ire commented Jun 4, 2024

fisharebest commented Jun 4, 2024

greg0ire commented Jun 4, 2024

arbor95 left a comment

SenseException commented Jul 27, 2024

fisharebest commented Aug 14, 2024

derrabus commented Aug 14, 2024

fisharebest commented Sep 4, 2024

greg0ire commented Sep 4, 2024

derrabus commented Sep 4, 2024

derrabus Sep 4, 2024

derrabus Sep 4, 2024

derrabus Sep 4, 2024 •

edited

Loading

derrabus Sep 4, 2024

derrabus Sep 4, 2024

	return 'utf8mb3' . substr($collation, 4);
	return 'utf8mb3' . substr($collation, strlen('utf8'));

	return 'utf8' . substr($collation, 7);
	return 'utf8' . substr($collation, strlen('utf8mb3'));

Fix: #6146 - utf8/utf8mb3 alias causes redundant ALTER TABLE changes #6370

Are you sure you want to change the base?

Fix: #6146 - utf8/utf8mb3 alias causes redundant ALTER TABLE changes #6370

Conversation

fisharebest commented Apr 29, 2024 • edited Loading

Summary

fisharebest commented Apr 30, 2024

greg0ire commented Apr 30, 2024

greg0ire commented Apr 30, 2024

greg0ire commented May 1, 2024 • edited Loading

fisharebest commented May 1, 2024

greg0ire commented May 1, 2024 • edited Loading

greg0ire commented May 1, 2024

greg0ire left a comment

Choose a reason for hiding this comment

greg0ire May 1, 2024

Choose a reason for hiding this comment

greg0ire May 1, 2024

Choose a reason for hiding this comment

greg0ire commented May 1, 2024

fisharebest commented May 1, 2024

greg0ire commented May 1, 2024 • edited Loading

fisharebest commented May 2, 2024

greg0ire commented May 2, 2024

greg0ire commented Jun 4, 2024

fisharebest commented Jun 4, 2024

greg0ire commented Jun 4, 2024

arbor95 left a comment

Choose a reason for hiding this comment

SenseException commented Jul 27, 2024

fisharebest commented Aug 14, 2024

derrabus commented Aug 14, 2024

fisharebest commented Sep 4, 2024

greg0ire commented Sep 4, 2024

derrabus commented Sep 4, 2024

derrabus Sep 4, 2024

Choose a reason for hiding this comment

derrabus Sep 4, 2024

Choose a reason for hiding this comment

derrabus Sep 4, 2024 • edited Loading

Choose a reason for hiding this comment

derrabus Sep 4, 2024

Choose a reason for hiding this comment

derrabus Sep 4, 2024

Choose a reason for hiding this comment

fisharebest commented Apr 29, 2024 •

edited

Loading

greg0ire commented May 1, 2024 •

edited

Loading

greg0ire commented May 1, 2024 •

edited

Loading

greg0ire commented May 1, 2024 •

edited

Loading

derrabus Sep 4, 2024 •

edited

Loading