Platform-aware schema comparison #4746

morozov · 2021-08-15T02:23:28Z

State

The existing property-based column definition comparison has quite some flaws (see all the associated issues):

Certain properties, even if different, may yield the same column definition (e.g. the length of a blob column within the type limits).
The above is platform-specific, so if a certain type's property is ignored on one platform, it can cause a false-positive match on another platform.

Proposal

The idea is to replace property-based comparison with the comparison of the SQL that the given column definition generates on the target platform. This way, the diff will be only generated if the column SQL representations on a given platform are different.

API changes

This is quite an invasive change but it's meant to be safe for a minor release. The changes in the test logic identify the existing bugs rather than breaking changes.

The Comparator class now takes an optional AbstractPlatform argument. If it's passed, column comparison will be performed in the context of the platform. Not passing a platform is now deprecated.
AbstractPlatform::columnsEqual() and Comparator::columnsEqual() have been added to compare column definitions in the context of the given platform. Depending on the platform, the comparison logic may be more complex than comparing the immediate column SQL. E.g. the default value on SQL Server is represented as a separate constraint.
AbstractSchemaManager::createComparator() has been added to create the comparator. Besides the schemas being compared and the target platform, the comparison logic may depend on the data available only at runtime (current database collation on SQL Server).
Constructors of the Comparator class and the subclasses are marked as internal to their schema managers. This will allow potentially improving the comparison logic without introducing breaking changes. The same is already implemented in driver-level connections and statements.

Platform-specific comparators clone the tables, if necessary, and normalize them before passing them to the base implementation. This implementation is far from perfect but it allows to avoid the introduction of a which may be not extensible enough and require a breaking change in the future.

Testing

Most of the changes in the tests are temporary and are needed to guarantee the parity between the old and the new implementation. The data providers will be removed in the next major release when the dependency of Comparator on AbstractPlatform becomes mandatory.

All functional tests have been reworked to cover the default comparator and the one returned by $schemaManager->createSchemaManager().
The unit tests that cover schema comparison in the context of a platform have been reworked to cover the default comparator and the one for the given platform (if it's different from the default).
The unit tests covering the Comparator outside of the context of a platform have been extended to cover all sub-classes.

Future scope

Array representation of column definitions. Burn with fire. Some of the existing APIs expect Column $column, others expect array<string,mixed> $column. The latter is an archaism, relies on some magic defaults (see AbstractPlatfrom::columnToArray()) and requires unnecessary type conversion back and forth.
Deprecate and get rid of Comparator::diffColumn() and ColumnDiff::$changedProperties. In light of the above changes, the meaning and the purpose of the changed properties become ambiguous. If a property has changed but it doesn't result in a diff in the context of the current platform, should it be included in the changed properties?
Mark SQLServer2012Platform::getDefaultConstraintDeclarationSQL() and other similar methods internal and then protected. See Inconsistent SchemaManager API across platforms #4503 for reference.
Use parametrized classes instead of the temporarily added assertions.

morozov · 2021-08-23T19:38:14Z

tests/Platforms/AbstractMySQLPlatformTestCase.php

-        $diff = (new Comparator())->diffTable($table, $diffTable);
-        self::assertNotFalse($diff);
-
-        self::assertEmpty($this->platform->getAlterTableSQL($diff));


These two assertions cover the bug in the current implementation rather than the expected behavior. The original test and assertions read as follows:

Make some schema changes that don't change the resulting SQL.

Assert that the diff is not empty. This is the bug being fixed.

Assert that even though the diff is not empty, the resulting diff SQL is. This is a workaround of the bug.

morozov · 2021-08-23T20:04:29Z

tests/Platforms/OraclePlatformTest.php

-        $diff = (new Comparator())->diffTable($table1, $table2);
-        self::assertNotFalse($diff);
-        self::assertEmpty($this->platform->getAlterTableSQL($diff));
+        self::assertFalse((new Comparator($this->platform))->diffTable($table1, $table2));


Same as above, the existing logic covers a bug, not the expected behavior.

src/Driver/AbstractMySQLDriver.php

derrabus · 2021-08-24T01:10:14Z

src/Schema/Comparator.php

+        Schema $toSchema,
+        ?AbstractPlatform $platform = null
+    ) {
+        $comparator       = new self($platform);


Does this still work, now that we have Comparator implementations for the individual drivers? This constructor call will yield a generic Comparator and not the implementation specific to the passed platform.

Good point. The generic comparator supplied with a platform can cause false-positive diffs. It shouldn't be used this way. I'll also adjust the deprecation message in the comparator constructor accordingly.

I rolled this back and deprecated Schema::getMigrateFromSql() and ::getMigrateToSql() that instantiate a comparator internally. Introducing a dependency of Schema on SchemaManager would be incorrect, and they are just convenience methods that can be replaced with their implementation.

derrabus · 2021-08-24T01:17:07Z

tests/Functional/Schema/MySQL/ComparatorTest.php

+    /** @var AbstractPlatform */
+    private $platform;
+
+    /** @var AbstractSchemaManager */


Suggested change

/** @var AbstractSchemaManager */

/** @var AbstractSchemaManager<MySQLPlatform> */

This will require Connection::createSchemaManager() and Driver::createSchemaManager() to be parametrized which will lead to the same issues as in #4746 (comment).

derrabus · 2021-08-24T01:25:04Z

tests/Schema/ComparatorTest.php

+
+    protected function setUp(): void
+    {
+        $this->comparator = new Comparator();


You have deprecated not passing a platform to the constructor.

Yes, but it still has to be covered with a test until the deprecated code is removed.

derrabus · 2021-08-24T01:25:32Z

tests/Schema/ComparatorTest.php

@@ -42,7 +51,7 @@ public function testCompareSame1(): void

        $expected             = new SchemaDiff();
        $expected->fromSchema = $schema1;
-        self::assertEquals($expected, Comparator::compareSchemas($schema1, $schema2));
+        self::assertEquals($expected, $this->comparator->compareSchemas($schema1, $schema2));


Why this change? The method is still static, isn't it?

It is but now the comparator class is extended by platform-specific classes and I want to reuse the same test case to test all implementations. See the classes extending this one.

greg0ire

Wow, looks like this is going to be a huge improvement given the number of linked issues 🎉

morozov · 2021-08-25T22:58:04Z

The remaining four linked issues are indeed fixed by this PR (the details are in the comments to each issue). I unlinked two since they appear to be unrelated and need to be addressed separately. One more is not reproducible even on 3.1.x (closed).

This PR was merged into the 4.4 branch. Discussion ---------- [Messenger] Fix calls to deprecated DBAL methods | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | N/A | License | MIT | Doc PR | N/A Follows doctrine/dbal#4746 and doctrine/dbal#4707. Commits ------- 2f834f2 [Messenger] Fix calls to deprecated DBAL methods

This code was relevant before the improvement in doctrine#4746 was implemented. The new comparator API is enforced in doctrine#4771. Now, the comparator will not produce a false-positive diff which is enforced by the new test.

morozov added Improvement MySQL SQLite Schema Comparison labels Aug 15, 2021

morozov force-pushed the platform-schema-comparison branch from ff2f807 to 5be15a1 Compare August 15, 2021 02:32

morozov mentioned this pull request Aug 20, 2021

Make the comparator API platform aware #4659

Closed

morozov force-pushed the platform-schema-comparison branch 2 times, most recently from 8fe95db to eb000ad Compare August 23, 2021 02:31

morozov commented Aug 23, 2021

View reviewed changes

morozov force-pushed the platform-schema-comparison branch 2 times, most recently from 924f0ab to 489d210 Compare August 23, 2021 20:03

morozov commented Aug 23, 2021

View reviewed changes

morozov force-pushed the platform-schema-comparison branch from 489d210 to badc300 Compare August 23, 2021 20:07

This was linked to issues Aug 23, 2021

Comparator fails for BLOBS #2663

Closed

Column altering in migration from TEXT to LONGTEXT may not apply. #2566

Closed

morozov marked this pull request as ready for review August 23, 2021 20:19

morozov requested review from greg0ire and derrabus August 23, 2021 20:19

morozov added the SQL Server label Aug 23, 2021

derrabus reviewed Aug 24, 2021

View reviewed changes

morozov force-pushed the platform-schema-comparison branch 2 times, most recently from 7ed68e7 to 26f64f4 Compare August 24, 2021 17:42

morozov added this to the 3.2.0 milestone Aug 24, 2021

morozov force-pushed the platform-schema-comparison branch from 26f64f4 to 8374a83 Compare August 25, 2021 15:49

greg0ire previously approved these changes Aug 25, 2021

View reviewed changes

morozov dismissed greg0ire’s stale review via 1809a91 August 25, 2021 22:55

morozov force-pushed the platform-schema-comparison branch from 8374a83 to 1809a91 Compare August 25, 2021 22:55

greg0ire approved these changes Aug 26, 2021

View reviewed changes

morozov merged commit 0453eef into doctrine:3.2.x Aug 26, 2021

morozov deleted the platform-schema-comparison branch August 26, 2021 14:36

morozov mentioned this pull request Aug 29, 2021

Remove platform-unaware schema comparison API #4771

Merged

morozov mentioned this pull request Sep 10, 2021

Deprecate not passing $fromColumn to ColumnDiff #4785

Merged

fritzmg mentioned this pull request Sep 29, 2021

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

Closed

morozov mentioned this pull request Oct 17, 2021

Add getOptions function to Column #4888

Closed

morozov mentioned this pull request Nov 13, 2021

Changing precision/scale value has no effect in schema comparison when not extendend from DecimalType #4981

Open

morozov mentioned this pull request Dec 8, 2021

Use Native JSON type instead of LONGTEXT for MariaDB #5100

Closed

greg0ire mentioned this pull request Dec 11, 2021

Drop type comments entirely #5107

Merged

morozov mentioned this pull request Jan 20, 2022

[Bugfix] Comparator should include the "length" of the text type fiel… #5192

Closed

mvorisek mentioned this pull request Jan 24, 2022

Diff is not working if options are set directly in Type::getSQLDeclaration() #3021

Closed

morozov mentioned this pull request Feb 17, 2022

Removed Unsigned Integer Support in SQL Server (and unsupported platforms) #2283

Closed

surikman mentioned this pull request Mar 16, 2022

Regression on charset/collate from v3.2.2 to v3.3.2 #5321

Closed

morozov mentioned this pull request Jul 3, 2022

Deprecate custom schema options #5476

Merged

This was referenced Jul 14, 2022

[Messenger] Fix calls to deprecated DBAL methods symfony/symfony#46941

Merged

Fix link in deprecation message #5503

Merged

morozov mentioned this pull request Aug 13, 2022

Update documentation on schema comparison #5590

Merged

morozov mentioned this pull request Aug 26, 2022

Remove code compensating for a schema comparison flaw #5620

Merged

github-actions bot locked as resolved and limited conversation to collaborators Aug 27, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Platform-aware schema comparison #4746

Platform-aware schema comparison #4746

morozov commented Aug 15, 2021 •

edited

Loading

morozov Aug 23, 2021

morozov Aug 23, 2021

derrabus Aug 24, 2021

morozov Aug 24, 2021 •

edited

Loading

morozov Aug 24, 2021

derrabus Aug 24, 2021

morozov Aug 24, 2021

derrabus Aug 24, 2021

morozov Aug 24, 2021 •

edited

Loading

derrabus Aug 24, 2021

morozov Aug 24, 2021

greg0ire left a comment

morozov commented Aug 25, 2021

	/** @var AbstractSchemaManager */
	/** @var AbstractSchemaManager<MySQLPlatform> */

Platform-aware schema comparison #4746

Platform-aware schema comparison #4746

Conversation

morozov commented Aug 15, 2021 • edited Loading

State

Proposal

API changes

Testing

Future scope

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

morozov Aug 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

morozov Aug 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greg0ire left a comment

Choose a reason for hiding this comment

morozov commented Aug 25, 2021

morozov commented Aug 15, 2021 •

edited

Loading

morozov Aug 24, 2021 •

edited

Loading

morozov Aug 24, 2021 •

edited

Loading