Fix Comparator column diff for blob length #4551

ausi · 2021-03-23T10:45:02Z

Q	A
Type	bug
BC Break	no
Fixed issues	#2663

Summary

In MySQL the blob type depends on the length to use one of TINYBLOB, BLOB, MEDIUMBLOB or LONGBLOB. When diffing two blob columns the length is currently ignored which results in missing changes of the underlying database column type.

UPDATE: It is the same with the text type which uses one of TINYTEXT, TEXT, MEDIUMTEXT or LONGTEXT.

greg0ire

The patch looks good, and the tests fail as they should when reverting changes in lib:

There were 4 failures:

1) Doctrine\Tests\DBAL\Platforms\MariaDb1027PlatformTest::testAlterTableChangeBlobColumnLength
Failed asserting that false is not false.

/home/gregoire/dev/doctrine-dbal/tests/Doctrine/Tests/DBAL/Platforms/AbstractMySQLPlatformTestCase.php:840

2) Doctrine\Tests\DBAL\Platforms\MySQL57PlatformTest::testAlterTableChangeBlobColumnLength
Failed asserting that false is not false.

/home/gregoire/dev/doctrine-dbal/tests/Doctrine/Tests/DBAL/Platforms/AbstractMySQLPlatformTestCase.php:840

3) Doctrine\Tests\DBAL\Platforms\MySqlPlatformTest::testAlterTableChangeBlobColumnLength
Failed asserting that false is not false.

/home/gregoire/dev/doctrine-dbal/tests/Doctrine/Tests/DBAL/Platforms/AbstractMySQLPlatformTestCase.php:840

4) Doctrine\Tests\DBAL\Schema\ComparatorTest::testCompareChangedBlobColumn
Failed asserting that two objects are equal.
--- Expected
+++ Actual
@@ @@
     'removedNamespaces' => Array ()
     'newTables' => Array ()
     'changedTables' => Array (
-        'foo' => Doctrine\DBAL\Schema\TableDiff Object (...)
     )
     'removedTables' => Array ()
     'newSequences' => Array ()

/home/gregoire/dev/doctrine-dbal/tests/Doctrine/Tests/DBAL/Schema/ComparatorTest.php:1126

About the target branch, I'm unsure if there will be more releases of 2.12.x in the future, but it appears this branch has been contributed recently though. @morozov will know more about this.

morozov · 2021-03-23T16:10:30Z

Yes, all bugfixes currently go to 2.12.x. It will be released right before 2.13.0.

morozov

Does the same logic apply to the TEXT/CLOB columns?

morozov · 2021-03-23T16:14:01Z

lib/Doctrine/DBAL/Schema/Comparator.php

@@ -482,6 +482,10 @@ public function diffColumn(Column $column1, Column $column2)
            if ($properties1['fixed'] !== $properties2['fixed']) {
                $changedProperties[] = 'fixed';
            }
+        } elseif ($properties1['type'] instanceof Types\BlobType) {
+            if ($properties1['length'] !== $properties2['length']) {


Looks like it will introduce a false-positive diff when comparing say BLOB(1) and BLOB(2) which both would translate to a TINYBLOB in MySQL.

While the fix seems to be within the current comparator design, the design obviously has flaws. A proper approach might be to add platform awareness to the comparator API and compare the SQL generated against a given platform. It will also likely eliminate the need for the hack introduced in #3382.

Agreed, but changing the comparator API cannot be done in a bugfix release I guess?

2.12.2 and 2.13.0 should be released at about the same time, so the release schedule shouldn't be a concern. If it's a non-breaking change (e.g. that adds a new optional parameter to the API), it could be released in 2.13.0. Why does it have to be a bugfix release?

Why does it have to be a bugfix release?

I would consider it a bug because the MySQL diff for blob/text columns doesn’t work currently.

Looks like it will introduce a false-positive diff when comparing say BLOB(1) and BLOB(2)

Wouldn’t such false positives also occur for many other types already? Like binary(fixed=true) vs binary(fixed=false) on PostgreSQL/SQLite or smallint vs integer on SQLite?

morozov · 2021-03-23T16:14:46Z

tests/Doctrine/Tests/DBAL/Platforms/AbstractMySQLPlatformTestCase.php

+    /**
+     * @return string[]
+     */
+    protected function getAlterTableChangeBlobColumnLength(): array


Why does it have to be a separate method?

I wanted to use the same coding style as other similar tests. Should I integrate it into the testAlterTableChangeBlobColumnLength() method instead?

morozov · 2021-03-23T16:16:15Z

tests/Doctrine/Tests/DBAL/Schema/ComparatorTest.php

+        $oldSchema = new Schema();
+
+        $tableFoo = $oldSchema->createTable('foo');
+        $tableFoo->addColumn('id', 'string', ['length' => 127]);


The issue is about BLOB columns. Why does the test compare string column lengths?

There was no test yet that tested the length changes for string columns. I mainly added this test to confirm my expectation about how the API works. I can move this test to its own pull request, or remove it alltogehter, however you prefer ☺️

ausi · 2021-03-23T16:40:42Z

Does the same logic apply to the TEXT/CLOB columns?

Yes, added in a85737f

ausi · 2021-03-24T09:58:59Z

lib/Doctrine/DBAL/Schema/DB2SchemaManager.php

-            case 'clob':
-                $length = $tableColumn['length'];
-                break;
-


This needs to be removed in order for Db2SchemaManagerTest::testDiffListTableColumns() to succeed.

I think this is correct because DB2Platform does currently not handle lengths for blob and text types, see

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

Lines 127 to 130 in b366d93

public function getClobTypeDeclarationSQL(array $column)

{

// todo clob(n) with $column['length'];

return 'CLOB(1M)';

and

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

Lines 63 to 66 in b366d93

public function getBlobTypeDeclarationSQL(array $column)

{

// todo blob(n) with $column['length'];

return 'BLOB(1M)';

and this change normalizes the behavior for blob and text to work the same now.

Toflar · 2021-05-25T10:09:22Z

Any chance this is going to be merged into 3.1 if conflicts are resolved?
I just ran into this bug again :/

…SQL()

morozov · 2021-05-25T22:14:39Z

Any chance this is going to be merged into 3.1 if conflicts are resolved?

I do not intend to merge it in the current state: #4551 (comment).

ausi · 2021-05-26T06:20:12Z

While the fix seems to be within the current comparator design, the design obviously has flaws. A proper approach might be to add platform awareness to the comparator API and compare the SQL generated against a given platform. It will also likely eliminate the need for the hack introduced in #3382.

I will try to work on this. I would add a constructor to the Comparator class with an optional argument for passing an AbstractPlatform object.

Should I mark using the comparator without a platform as deprecated?

Is this considered to be a new feature and based on the 3.2.x branch or a bugfix based on 3.1.x instead?

morozov · 2021-05-27T01:25:30Z

I would add a constructor to the Comparator class with an optional argument for passing an AbstractPlatform object.

I think it'd make more sense to pass the platform to all methods explicitly. This will keep the comparator free of dependencies.

Should I mark using the comparator without a platform as deprecated?

I wouldn't bother about deprecation and backward compatibility for now. I'd start with prototyping a solution with the new API and see if it solves the problem and reduces the number of platform-specific edge cases to be handled in the code. Once this is confirmed, we can think of the rest.

Is this considered to be a new feature and based on the 3.2.x branch or a bugfix based on 3.1.x instead?

It's definitely a new feature.

ausi · 2021-05-29T21:48:59Z

Done: #4659

morozov · 2021-07-13T18:34:00Z

Closing in favor of #4659.

This was referenced Mar 23, 2021

Comparator fails for BLOBS #2663

Closed

Changing a field's sql type from blob to mediumblob isn't recognized by migrate script contao/contao#2750

Closed

greg0ire added the MySQL label Mar 23, 2021

greg0ire previously approved these changes Mar 23, 2021

View reviewed changes

morozov reviewed Mar 23, 2021

View reviewed changes

ausi dismissed greg0ire’s stale review via a85737f March 23, 2021 16:40

ausi force-pushed the fix/schema-diff-blob-size branch from 89cadaf to 0efc0ac Compare March 24, 2021 09:03

ausi commented Mar 24, 2021

View reviewed changes

morozov closed this Apr 21, 2021

morozov deleted the branch doctrine:3.1.x April 21, 2021 16:26

morozov reopened this Apr 21, 2021

morozov changed the base branch from 2.12.x to 3.1.x May 2, 2021 17:39

ausi added 6 commits May 25, 2021 12:44

Fix Comparator column diff for blob length

30dc99e

Fix Comparator column diff for text length

7b93708

Fix default value for text/blob length

5c21228

Normalize length -1 to 0

3c191ed

DB2 not supports clob length, see DB2Platform::getClobTypeDeclaration…

f519ae6

…SQL()

Fix coding style

6fccbbc

ausi force-pushed the fix/schema-diff-blob-size branch from d4916d2 to 6fccbbc Compare May 25, 2021 10:53

ausi mentioned this pull request May 29, 2021

Make the comparator API platform aware #4659

Closed

morozov closed this Jul 13, 2021

github-actions bot locked as resolved and limited conversation to collaborators Jul 20, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Comparator column diff for blob length #4551

Fix Comparator column diff for blob length #4551

ausi commented Mar 23, 2021 •

edited

Loading

greg0ire left a comment

morozov commented Mar 23, 2021

morozov left a comment

morozov Mar 23, 2021 •

edited

Loading

ausi Mar 23, 2021

morozov Mar 23, 2021

ausi Mar 24, 2021 •

edited

Loading

morozov Mar 23, 2021

ausi Mar 23, 2021

morozov Mar 23, 2021

ausi Mar 23, 2021

ausi commented Mar 23, 2021

ausi Mar 24, 2021

Toflar commented May 25, 2021

morozov commented May 25, 2021 •

edited

Loading

ausi commented May 26, 2021 •

edited

Loading

morozov commented May 27, 2021 •

edited

Loading

ausi commented May 29, 2021

morozov commented Jul 13, 2021

	public function getClobTypeDeclarationSQL(array $column)
	{
	// todo clob(n) with $column['length'];
	return 'CLOB(1M)';

	public function getBlobTypeDeclarationSQL(array $column)
	{
	// todo blob(n) with $column['length'];
	return 'BLOB(1M)';

Fix Comparator column diff for blob length #4551

Fix Comparator column diff for blob length #4551

Conversation

ausi commented Mar 23, 2021 • edited Loading

Summary

greg0ire left a comment

Choose a reason for hiding this comment

morozov commented Mar 23, 2021

morozov left a comment

Choose a reason for hiding this comment

morozov Mar 23, 2021 • edited Loading

Choose a reason for hiding this comment

ausi Mar 23, 2021

Choose a reason for hiding this comment

morozov Mar 23, 2021

Choose a reason for hiding this comment

ausi Mar 24, 2021 • edited Loading

Choose a reason for hiding this comment

morozov Mar 23, 2021

Choose a reason for hiding this comment

ausi Mar 23, 2021

Choose a reason for hiding this comment

morozov Mar 23, 2021

Choose a reason for hiding this comment

ausi Mar 23, 2021

Choose a reason for hiding this comment

ausi commented Mar 23, 2021

ausi Mar 24, 2021

Choose a reason for hiding this comment

Toflar commented May 25, 2021

morozov commented May 25, 2021 • edited Loading

ausi commented May 26, 2021 • edited Loading

morozov commented May 27, 2021 • edited Loading

ausi commented May 29, 2021

morozov commented Jul 13, 2021

ausi commented Mar 23, 2021 •

edited

Loading

morozov Mar 23, 2021 •

edited

Loading

ausi Mar 24, 2021 •

edited

Loading

morozov commented May 25, 2021 •

edited

Loading

ausi commented May 26, 2021 •

edited

Loading

morozov commented May 27, 2021 •

edited

Loading