[GH-1204] Add full support for foreign key constraints for SQLite #3762

beberlei · 2019-12-01T13:07:47Z

Add full support for foreign key in SQLite.

Q	A
Type	bug
BC Break	no
Fixed issues	#1204 doctrine/orm#7841 doctrine/orm#7930

Summary

SQLite has partial foreign key support, which leads to bugs in ORM Schema Tool, because it cannot reliable make use of AbstractPlatform::supportsForeignKeyConstraints() and hence always re-creates the same table structure, because the DBAL Comparator it finds a diff in the schema that doesn't exist.

This PR does a few things:

Sets SqlitePlatform::supportsForeignKeyConstraints() to true
Introduces new flag supportsCreateDropForeignKeyConstraints() which is necessary to control in SchemaDiff::toSql() how to create the SQL.
Copies the CommitOrderCalculator from ORM to DBAL because its needed in Schema\Comparator to calculate the order of new tables to create. The tests are also copied over, the idea is probably that ORM removes the commit order calculator in 2.8 and uses the one from DBAL instead.
Throw ForeignKeyConstraintViolationException when PRAGMA foreign_keys=On set and violation occurs. Tests failed after switching supportsForeignKeyConstraints to true.

Even though its big, I would argue this is still considered a bugfix, because returning false from supportsForeignKeyConstraints() is actually not wrong, since the code supports foreig key constraints.

lib/Doctrine/DBAL/Internal/CommitOrderCalculator.php

morozov

The code and tests look fine. Just a few questions.

lib/Doctrine/DBAL/Internal/CommitOrderCalculator.php

morozov · 2019-12-14T00:09:48Z

lib/Doctrine/DBAL/Internal/CommitOrderCalculator.php

+ * This algorithm have a linear running time based on nodes (V) and dependency
+ * between the nodes (E), resulting in a computational complexity of O(V + E).
+ */
+class CommitOrderCalculator


Although this is used for commit order calculation, it's not specialized for that. Would it make sense to rename this class and the related ones in a way that doesn't imply that it's only for commits? E.g. Graph, Node, Edge?

We should make it final too

Perhaps we could bring this to doctrine/persistence to be reuse by the ORM too?

I made it final now, we can move it to persistence in another step, but ORM depends on DBAL, so it can already use it?

If it's needed in ORM together with the methods like hasNode() that are not used by DBAL and probably weights as well, why not move it to a shared library instead?

It feels weird that the code is ported from one library to another as is just because it's needed as is for the library it's currently located in.

@morozov i don't see how its weird, DBAL is the common denominator of where this code is needed in terms of dependencies. Introducing yet another library is just adding overhead. In addition it would prevent this fix from being merged into 2.10 per the dependencies are locked rule.

IMO, the "common denominator" approach is wrong and counterproductive. This way, the consumers of the shared logic end up storing and maintaining the logic they need in a place where it doesn't belong.

Eventually, the changes in this logic are driven by the consumers while the owners of the library end up accepting everything "just because it's needed for project X".

In this case, I see two proper solutions:

Have @internal implementations that solve the problems of each of the libraries independently and evolve independently (looks the most reasonable in this case).

Have a shared library that solves this problem in general (look like overkill in this case).

We can keep this code as a 100% copy of the ORM code. This is not an attempt to force the ORM to use the class from here in the future. I just happened to need the exact same logic, because its the same problem, hence suggesting it can be re-used by the ORM in the future.

I disagree. The commit order resolution is an internal (as the namespace suggests) implementation detail of the SQLite driver which is not part of the database abstraction layer. It cannot be planned for reuse by another component. And this argument cannot be used to keep the pieces of the code that this component doesn't need for its internal purposes.

lib/Doctrine/DBAL/Schema/SchemaDiff.php

tests/Doctrine/Tests/DBAL/Internal/CommitOrderCalculatorTest.php

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

lib/Doctrine/DBAL/Platforms/AbstractPlatform.php

greg0ire · 2019-12-29T22:03:50Z

lib/Doctrine/DBAL/Schema/SchemaDiff.php

+    /**
+     * Sorts tables by dependencies so that they are created in the right order.
+     *
+     * This is ncessary when one table depends on another while creating foreign key


Suggested change

* This is ncessary when one table depends on another while creating foreign key

* This is necessary when one table depends on another while creating foreign key

lib/Doctrine/DBAL/Internal/CommitOrderCalculator.php

lib/Doctrine/DBAL/Internal/CommitOrderEdge.php

greg0ire · 2020-02-29T19:08:13Z

The new one true base branch is 2.10.x, switching to that.

morozov · 2020-03-22T15:05:48Z

@beberlei the result looks good. Should we squash at least some commits? The “fix typo”-like are definitely irrelevant for the history.

morozov · 2020-03-22T15:07:14Z

The ContinuousPHP failure is temporary and expected due to the configuration change in #3915.

morozov · 2020-03-22T15:17:06Z

@beberlei to avoid double builds w/o making additional configuration changes across CI platforms, please open future PRs from your fork instead of the upstream repository.

PReimers · 2020-04-28T12:33:50Z

@morozov
Any chance this could make it into the next release?

morozov · 2020-04-28T14:08:04Z

@PReimers yes, but it depends on whether @beberlei finds the time to finish it.

alcaeus · 2020-04-29T13:27:12Z

@morozov if it's just a rebase that involves squashing a bunch of commits, there's nothing to prevent us from doing that ourselves or doing it via the GitHub merge functionality. I've squashed all commits since all follow-up commits were only due to review changes.

morozov · 2020-04-29T16:51:01Z

@alcaeus the newly added test fails on Oracle:

1) Doctrine\Tests\DBAL\Functional\Schema\OracleSchemaManagerTest::testSchemaDiffForeignKeys
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
+    0 => 'ALTER TABLE child MODIFY (ID ... NULL)'
+    1 => 'ALTER TABLE parent MODIFY (ID... NULL)'
 )

tests/Doctrine/Tests/DBAL/Functional/Schema/SchemaManagerFunctionalTestCase.php:1642

…form and Schema.

beberlei · 2020-05-01T14:33:58Z

@morozov Fixed by rewriting the assertion logic, I couldn't find out why Oracle had the column ID vs id names because of the default casing. Its not related to what the test is testing, so the adjusted assertion takes care of that.

morozov · 2020-05-01T14:38:26Z

tests/Doctrine/Tests/DBAL/Functional/Schema/SchemaManagerFunctionalTestCase.php

+        $diff = Comparator::compareSchemas($offlineSchema, $schema);
+
+        foreach ($diff->changedTables as $table) {
+            if (count($table->changedForeignKeys) <= 0 && count($table->addedForeignKeys) <= 0 && count($table->removedForeignKeys) <= 0) {


Not that it needs to be taken care of immediately but aren't these just three assertions like assertCount(0, $table->changedForeignKeys)?

morozov · 2020-05-01T14:42:06Z

[…] Oracle had the column ID vs id names because of the default casing. Its not related to what the test is testing, so the adjusted assertion takes care of that.

Makes sense. Given that on Oracle, DBAL produces unexpected DDL, it looks like a bug (although, unrelated). Right?

morozov · 2020-05-02T15:23:16Z

Thanks, @beberlei and @alcaeus.

morozov · 2020-06-29T23:03:51Z

phpstan.neon.dist

@@ -77,3 +77,9 @@ parameters:
        -
            message: '~^Access to undefined constant PDO::PGSQL_ATTR_DISABLE_PREPARES\.$~'
            path: %currentWorkingDirectory%/lib/Doctrine/DBAL/Driver/PDOPgSql/Driver.php
+
+        # False Positive
+        - '~Strict comparison using === between 1 and 2 will always evaluate to false~'


@beberlei how is this a false positive?

dbal/lib/Doctrine/DBAL/Internal/DependencyOrderCalculator.php

Lines 106 to 123 in 85a983c

$vertex->state = self::IN_PROGRESS;

foreach ($vertex->dependencyList as $edge) {

$adjacentVertex = $this->nodeList[$edge->to];

switch ($adjacentVertex->state) {

case self::VISITED:

case self::IN_PROGRESS:

// Do nothing, since node was already visited or is

// currently visited

break;

case self::NOT_VISITED:

$this->visit($adjacentVertex);

}

}

if ($vertex->state === self::VISITED) {

It looks like a bug in the code. How is $vertex->state expected to change its value from IN_PROGRESS to anything else between the first and last lines of the block above?

Technically, it's possible if the node is adjacent to (depends on) itself but this doesn't look like a valid case.

beberlei requested review from morozov and lcobucci December 1, 2019 13:07

This was referenced Dec 1, 2019

#7841 SchemaTool generates extra diff for platforms without FK support doctrine/orm#7842

Merged

Revert SchemaTool change to check for foreign key support doctrine/orm#7937

Merged

BackEndTea reviewed Dec 2, 2019

View reviewed changes

lib/Doctrine/DBAL/Internal/CommitOrderCalculator.php Outdated Show resolved Hide resolved

beberlei force-pushed the GH-1204-CleanupSqliteForeignKeySupport branch from aaf2410 to 5a6bedc Compare December 7, 2019 00:05

beberlei changed the base branch from 2.9 to 2.10 December 7, 2019 00:07

beberlei added this to the 2.10.1 milestone Dec 7, 2019

morozov reviewed Dec 14, 2019

View reviewed changes

lcobucci reviewed Dec 16, 2019

View reviewed changes

tests/Doctrine/Tests/DBAL/Schema/SchemaDiffTest.php Outdated Show resolved Hide resolved

lcobucci reviewed Dec 16, 2019

View reviewed changes

lib/Doctrine/DBAL/Platforms/AbstractPlatform.php Outdated Show resolved Hide resolved

lcobucci changed the title ~~[GH-1204] Add full support for foreign key constraintts for SQLite~~ [GH-1204] Add full support for foreign key constraints for SQLite Dec 16, 2019

greg0ire reviewed Dec 29, 2019

View reviewed changes

lcobucci mentioned this pull request Dec 30, 2019

Upgrade to PHPStan v0.12 #3799

Merged

BackEndTea reviewed Dec 30, 2019

View reviewed changes

lib/Doctrine/DBAL/Internal/CommitOrderCalculator.php Outdated Show resolved Hide resolved

BackEndTea reviewed Dec 30, 2019

View reviewed changes

lib/Doctrine/DBAL/Internal/CommitOrderCalculator.php Outdated Show resolved Hide resolved

lib/Doctrine/DBAL/Internal/CommitOrderEdge.php Outdated Show resolved Hide resolved

morozov modified the milestones: 2.10.1, 2.10.2 Jan 4, 2020

morozov mentioned this pull request Jan 17, 2020

Fix phpstan errors #3824

Merged

BenMorel mentioned this pull request Jan 18, 2020

Proper classes for QueryBuilder SQL parts #3829

Closed

greg0ire changed the base branch from 2.10 to 2.10.x February 29, 2020 19:07

beberlei force-pushed the GH-1204-CleanupSqliteForeignKeySupport branch from bdc458d to cb21373 Compare March 22, 2020 14:50

morozov previously approved these changes Mar 22, 2020

View reviewed changes

morozov modified the milestones: 2.10.2, 2.10.3 Apr 20, 2020

alcaeus dismissed morozov’s stale review via e7d04a6 April 29, 2020 13:26

alcaeus force-pushed the GH-1204-CleanupSqliteForeignKeySupport branch from cb21373 to e7d04a6 Compare April 29, 2020 13:26

alcaeus requested a review from morozov April 29, 2020 13:27

alcaeus previously approved these changes Apr 29, 2020

View reviewed changes

alcaeus dismissed their stale review via 8073d1a April 29, 2020 13:29

alcaeus force-pushed the GH-1204-CleanupSqliteForeignKeySupport branch from e7d04a6 to 8073d1a Compare April 29, 2020 13:29

[GH-1204] Add full support for foreign key constraints in SQLite Plat…

85a983c

…form and Schema.

beberlei force-pushed the GH-1204-CleanupSqliteForeignKeySupport branch from 950cdac to 85a983c Compare May 1, 2020 14:32

morozov reviewed May 1, 2020

View reviewed changes

morozov approved these changes May 1, 2020

View reviewed changes

morozov added Improvement SQLite Foreign Keys Bug labels May 2, 2020

morozov merged commit fa8b6af into 2.10.x May 2, 2020

morozov deleted the GH-1204-CleanupSqliteForeignKeySupport branch May 2, 2020 15:22

morozov mentioned this pull request May 2, 2020

DBAL-1253: Sqlite: inconsistent (non-)support of foreign keys #1204

Closed

morozov reviewed Jun 29, 2020

View reviewed changes

DarkPrinceWW mentioned this pull request Sep 2, 2020

Problem changing table when run tests using in memory sqlite after upgrade on v2.10.3. #4244

Closed

greg0ire mentioned this pull request Sep 2, 2020

SQLite Foreign Key Behavior BC Break #4243

Closed

This was referenced May 17, 2022

Natural order of SQLite foreign keys #5402

Merged

Improve foreign key introspection on SQLite #5409

Merged

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[GH-1204] Add full support for foreign key constraints for SQLite #3762

[GH-1204] Add full support for foreign key constraints for SQLite #3762

beberlei commented Dec 1, 2019 •

edited

Loading

morozov left a comment

morozov Dec 14, 2019

lcobucci Dec 16, 2019

lcobucci Dec 16, 2019 •

edited

Loading

beberlei Dec 29, 2019

morozov Dec 29, 2019

beberlei Jan 3, 2020

morozov Jan 3, 2020

beberlei Jan 3, 2020

morozov Jan 3, 2020

greg0ire Dec 29, 2019

greg0ire commented Feb 29, 2020

morozov commented Mar 22, 2020

morozov commented Mar 22, 2020

morozov commented Mar 22, 2020

PReimers commented Apr 28, 2020

morozov commented Apr 28, 2020

alcaeus commented Apr 29, 2020 •

edited

Loading

morozov commented Apr 29, 2020

beberlei commented May 1, 2020

morozov May 1, 2020

morozov commented May 1, 2020 •

edited

Loading

morozov commented May 2, 2020

morozov Jun 29, 2020 •

edited

Loading

	* This is ncessary when one table depends on another while creating foreign key
	* This is necessary when one table depends on another while creating foreign key

	$vertex->state = self::IN_PROGRESS;

	foreach ($vertex->dependencyList as $edge) {
	$adjacentVertex = $this->nodeList[$edge->to];

	switch ($adjacentVertex->state) {
	case self::VISITED:
	case self::IN_PROGRESS:
	// Do nothing, since node was already visited or is
	// currently visited
	break;

	case self::NOT_VISITED:
	$this->visit($adjacentVertex);
	}
	}

	if ($vertex->state === self::VISITED) {

[GH-1204] Add full support for foreign key constraints for SQLite #3762

[GH-1204] Add full support for foreign key constraints for SQLite #3762

Conversation

beberlei commented Dec 1, 2019 • edited Loading

Summary

morozov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lcobucci Dec 16, 2019 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greg0ire commented Feb 29, 2020

morozov commented Mar 22, 2020

morozov commented Mar 22, 2020

morozov commented Mar 22, 2020

PReimers commented Apr 28, 2020

morozov commented Apr 28, 2020

alcaeus commented Apr 29, 2020 • edited Loading

morozov commented Apr 29, 2020

beberlei commented May 1, 2020

Choose a reason for hiding this comment

morozov commented May 1, 2020 • edited Loading

morozov commented May 2, 2020

morozov Jun 29, 2020 • edited Loading

Choose a reason for hiding this comment

beberlei commented Dec 1, 2019 •

edited

Loading

lcobucci Dec 16, 2019 •

edited

Loading

alcaeus commented Apr 29, 2020 •

edited

Loading

morozov commented May 1, 2020 •

edited

Loading

morozov Jun 29, 2020 •

edited

Loading