From 85a983c3f8d6447c4441832477def1c28d57bee9 Mon Sep 17 00:00:00 2001
From: Benjamin Eberlei <kontakt@beberlei.de>
Date: Sun, 1 Dec 2019 13:54:49 +0100
Subject: [PATCH] [GH-1204] Add full support for foreign key constraints in
 SQLite Platform and Schema.

---
 .../DBAL/Driver/AbstractSQLiteDriver.php      |   4 +
 .../Internal/DependencyOrderCalculator.php    | 131 ++++++++++++++++++
 .../DBAL/Internal/DependencyOrderEdge.php     |  12 ++
 .../DBAL/Internal/DependencyOrderNode.php     |  18 +++
 .../DBAL/Platforms/AbstractPlatform.php       |  10 ++
 .../DBAL/Platforms/SqlitePlatform.php         |  12 +-
 lib/Doctrine/DBAL/Schema/SchemaDiff.php       |  49 ++++++-
 .../Schema/Visitor/DropSchemaSqlCollector.php |   4 +
 phpstan.neon.dist                             |   6 +
 .../Tests/DBAL/Functional/ExceptionTest.php   |   4 +-
 .../SchemaManagerFunctionalTestCase.php       |  47 +++++++
 .../DependencyOrderCalculatorTest.php         |  56 ++++++++
 .../Platforms/AbstractPlatformTestCase.php    |  20 +--
 .../Tests/DBAL/Schema/SchemaDiffTest.php      |   6 +-
 14 files changed, 354 insertions(+), 25 deletions(-)
 create mode 100644 lib/Doctrine/DBAL/Internal/DependencyOrderCalculator.php
 create mode 100644 lib/Doctrine/DBAL/Internal/DependencyOrderEdge.php
 create mode 100644 lib/Doctrine/DBAL/Internal/DependencyOrderNode.php
 create mode 100644 tests/Doctrine/Tests/DBAL/Internal/DependencyOrderCalculatorTest.php

diff --git a/lib/Doctrine/DBAL/Driver/AbstractSQLiteDriver.php b/lib/Doctrine/DBAL/Driver/AbstractSQLiteDriver.php
index 582f7cae20a..13c4a83e909 100644
--- a/lib/Doctrine/DBAL/Driver/AbstractSQLiteDriver.php
+++ b/lib/Doctrine/DBAL/Driver/AbstractSQLiteDriver.php
@@ -67,6 +67,10 @@ public function convertException($message, DriverException $exception)
             return new Exception\ConnectionException($message, $exception);
         }
 
+        if (strpos($exception->getMessage(), 'FOREIGN KEY constraint failed') !== false) {
+            return new Exception\ForeignKeyConstraintViolationException($message, $exception);
+        }
+
         return new Exception\DriverException($message, $exception);
     }
 
diff --git a/lib/Doctrine/DBAL/Internal/DependencyOrderCalculator.php b/lib/Doctrine/DBAL/Internal/DependencyOrderCalculator.php
new file mode 100644
index 00000000000..c1026993be8
--- /dev/null
+++ b/lib/Doctrine/DBAL/Internal/DependencyOrderCalculator.php
@@ -0,0 +1,131 @@
+<?php
+
+namespace Doctrine\DBAL\Internal;
+
+use function array_reverse;
+
+/**
+ * DependencyOrderCalculator implements topological sorting, which is an ordering
+ * algorithm for directed graphs (DG) and/or directed acyclic graphs (DAG) by
+ * using a depth-first searching (DFS) to traverse the graph built in memory.
+ * 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).
+ */
+final class DependencyOrderCalculator
+{
+    public const NOT_VISITED = 0;
+    public const IN_PROGRESS = 1;
+    public const VISITED     = 2;
+
+    /**
+     * Matrix of nodes (aka. vertex).
+     * Keys are provided hashes and values are the node definition objects.
+     *
+     * @var array<string,DependencyOrderNode>
+     */
+    private $nodeList = [];
+
+    /**
+     * Volatile variable holding calculated nodes during sorting process.
+     *
+     * @var array<object>
+     */
+    private $sortedNodeList = [];
+
+    /**
+     * Checks for node (vertex) existence in graph.
+     */
+    public function hasNode(string $hash) : bool
+    {
+        return isset($this->nodeList[$hash]);
+    }
+
+    /**
+     * Adds a new node (vertex) to the graph, assigning its hash and value.
+     *
+     * @param object $node
+     */
+    public function addNode(string $hash, $node) : void
+    {
+        $vertex = new DependencyOrderNode();
+
+        $vertex->hash  = $hash;
+        $vertex->state = self::NOT_VISITED;
+        $vertex->value = $node;
+
+        $this->nodeList[$hash] = $vertex;
+    }
+
+    /**
+     * Adds a new dependency (edge) to the graph using their hashes.
+     */
+    public function addDependency(string $fromHash, string $toHash) : void
+    {
+        $vertex = $this->nodeList[$fromHash];
+        $edge   = new DependencyOrderEdge();
+
+        $edge->from = $fromHash;
+        $edge->to   = $toHash;
+
+        $vertex->dependencyList[$toHash] = $edge;
+    }
+
+    /**
+     * Return a valid order list of all current nodes.
+     * The desired topological sorting is the reverse post order of these searches.
+     *
+     * {@internal Highly performance-sensitive method.}
+     *
+     * @return array<object>
+     */
+    public function sort() : array
+    {
+        foreach ($this->nodeList as $vertex) {
+            if ($vertex->state !== self::NOT_VISITED) {
+                continue;
+            }
+
+            $this->visit($vertex);
+        }
+
+        $sortedList = $this->sortedNodeList;
+
+        $this->nodeList       = [];
+        $this->sortedNodeList = [];
+
+        return array_reverse($sortedList);
+    }
+
+    /**
+     * Visit a given node definition for reordering.
+     *
+     * {@internal Highly performance-sensitive method.}
+     */
+    private function visit(DependencyOrderNode $vertex) : void
+    {
+        $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) {
+            return;
+        }
+
+        $vertex->state = self::VISITED;
+
+        $this->sortedNodeList[] = $vertex->value;
+    }
+}
diff --git a/lib/Doctrine/DBAL/Internal/DependencyOrderEdge.php b/lib/Doctrine/DBAL/Internal/DependencyOrderEdge.php
new file mode 100644
index 00000000000..fa1a0453c46
--- /dev/null
+++ b/lib/Doctrine/DBAL/Internal/DependencyOrderEdge.php
@@ -0,0 +1,12 @@
+<?php
+
+namespace Doctrine\DBAL\Internal;
+
+class DependencyOrderEdge
+{
+    /** @var string */
+    public $from;
+
+    /** @var string */
+    public $to;
+}
diff --git a/lib/Doctrine/DBAL/Internal/DependencyOrderNode.php b/lib/Doctrine/DBAL/Internal/DependencyOrderNode.php
new file mode 100644
index 00000000000..6d190318b67
--- /dev/null
+++ b/lib/Doctrine/DBAL/Internal/DependencyOrderNode.php
@@ -0,0 +1,18 @@
+<?php
+
+namespace Doctrine\DBAL\Internal;
+
+class DependencyOrderNode
+{
+    /** @var string */
+    public $hash;
+
+    /** @var int */
+    public $state;
+
+    /** @var object */
+    public $value;
+
+    /** @var DependencyOrderEdge[] */
+    public $dependencyList = [];
+}
diff --git a/lib/Doctrine/DBAL/Platforms/AbstractPlatform.php b/lib/Doctrine/DBAL/Platforms/AbstractPlatform.php
index f805b3d8c52..e76bc003da7 100644
--- a/lib/Doctrine/DBAL/Platforms/AbstractPlatform.php
+++ b/lib/Doctrine/DBAL/Platforms/AbstractPlatform.php
@@ -3184,6 +3184,16 @@ public function supportsForeignKeyConstraints()
         return true;
     }
 
+    /**
+     * Whether foreign key constraints can be dropped.
+     *
+     * If false, then getDropForeignKeySQL() throws exception.
+     */
+    public function supportsCreateDropForeignKeyConstraints() : bool
+    {
+        return true;
+    }
+
     /**
      * Whether this platform supports onUpdate in foreign key constraints.
      *
diff --git a/lib/Doctrine/DBAL/Platforms/SqlitePlatform.php b/lib/Doctrine/DBAL/Platforms/SqlitePlatform.php
index 4eff83e8dcf..e46965e3b32 100644
--- a/lib/Doctrine/DBAL/Platforms/SqlitePlatform.php
+++ b/lib/Doctrine/DBAL/Platforms/SqlitePlatform.php
@@ -769,6 +769,14 @@ public function canEmulateSchemas()
      * {@inheritDoc}
      */
     public function supportsForeignKeyConstraints()
+    {
+        return true;
+    }
+
+    /**
+     * {@inheritDoc}
+     */
+    public function supportsCreateDropForeignKeyConstraints() : bool
     {
         return false;
     }
@@ -786,7 +794,7 @@ public function getCreatePrimaryKeySQL(Index $index, $table)
      */
     public function getCreateForeignKeySQL(ForeignKeyConstraint $foreignKey, $table)
     {
-        throw new DBALException('Sqlite platform does not support alter foreign key.');
+        throw new DBALException('Sqlite platform does not support alter foreign key, the table must be fully recreated using getAlterTableSQL.');
     }
 
     /**
@@ -794,7 +802,7 @@ public function getCreateForeignKeySQL(ForeignKeyConstraint $foreignKey, $table)
      */
     public function getDropForeignKeySQL($foreignKey, $table)
     {
-        throw new DBALException('Sqlite platform does not support alter foreign key.');
+        throw new DBALException('Sqlite platform does not support alter foreign key, the table must be fully recreated using getAlterTableSQL.');
     }
 
     /**
diff --git a/lib/Doctrine/DBAL/Schema/SchemaDiff.php b/lib/Doctrine/DBAL/Schema/SchemaDiff.php
index 69bf125a844..29c699c0610 100644
--- a/lib/Doctrine/DBAL/Schema/SchemaDiff.php
+++ b/lib/Doctrine/DBAL/Schema/SchemaDiff.php
@@ -2,6 +2,7 @@
 
 namespace Doctrine\DBAL\Schema;
 
+use Doctrine\DBAL\Internal\DependencyOrderCalculator;
 use Doctrine\DBAL\Platforms\AbstractPlatform;
 use function array_merge;
 
@@ -137,13 +138,16 @@ protected function _toSql(AbstractPlatform $platform, $saveMode = false)
         }
 
         $foreignKeySql = [];
-        foreach ($this->newTables as $table) {
-            $sql = array_merge(
-                $sql,
-                $platform->getCreateTableSQL($table, AbstractPlatform::CREATE_INDEXES)
-            );
+        $createFlags   = AbstractPlatform::CREATE_INDEXES;
+
+        if (! $platform->supportsCreateDropForeignKeyConstraints()) {
+            $createFlags |= AbstractPlatform::CREATE_FOREIGNKEYS;
+        }
 
-            if (! $platform->supportsForeignKeyConstraints()) {
+        foreach ($this->getNewTablesSortedByDependencies() as $table) {
+            $sql = array_merge($sql, $platform->getCreateTableSQL($table, $createFlags));
+
+            if (! $platform->supportsCreateDropForeignKeyConstraints()) {
                 continue;
             }
 
@@ -165,4 +169,37 @@ protected function _toSql(AbstractPlatform $platform, $saveMode = false)
 
         return $sql;
     }
+
+    /**
+     * Sorts tables by dependencies so that they are created in the right order.
+     *
+     * This is necessary when one table depends on another while creating foreign key
+     * constraints directly during CREATE TABLE.
+     *
+     * @return array<Table>
+     */
+    private function getNewTablesSortedByDependencies()
+    {
+        $calculator = new DependencyOrderCalculator();
+        $newTables  = [];
+
+        foreach ($this->newTables as $table) {
+            $newTables[$table->getName()] = true;
+            $calculator->addNode($table->getName(), $table);
+        }
+
+        foreach ($this->newTables as $table) {
+            foreach ($table->getForeignKeys() as $foreignKey) {
+                $foreignTableName = $foreignKey->getForeignTableName();
+
+                if (! isset($newTables[$foreignTableName])) {
+                    continue;
+                }
+
+                $calculator->addDependency($foreignTableName, $table->getName());
+            }
+        }
+
+        return $calculator->sort();
+    }
 }
diff --git a/lib/Doctrine/DBAL/Schema/Visitor/DropSchemaSqlCollector.php b/lib/Doctrine/DBAL/Schema/Visitor/DropSchemaSqlCollector.php
index 81e2023128b..f846206329c 100644
--- a/lib/Doctrine/DBAL/Schema/Visitor/DropSchemaSqlCollector.php
+++ b/lib/Doctrine/DBAL/Schema/Visitor/DropSchemaSqlCollector.php
@@ -46,6 +46,10 @@ public function acceptTable(Table $table)
      */
     public function acceptForeignKey(Table $localTable, ForeignKeyConstraint $fkConstraint)
     {
+        if (! $this->platform->supportsCreateDropForeignKeyConstraints()) {
+            return;
+        }
+
         if (strlen($fkConstraint->getName()) === 0) {
             throw SchemaException::namedForeignKeyRequired($localTable, $fkConstraint);
         }
diff --git a/phpstan.neon.dist b/phpstan.neon.dist
index 7178aa6a906..70544cbf7ec 100644
--- a/phpstan.neon.dist
+++ b/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~'
+
+        # Needs Generics
+        - '~Method Doctrine\\DBAL\\Schema\\SchemaDiff::getNewTablesSortedByDependencies\(\) should return array<Doctrine\\DBAL\\Schema\\Table> but returns array<object>.~'
diff --git a/tests/Doctrine/Tests/DBAL/Functional/ExceptionTest.php b/tests/Doctrine/Tests/DBAL/Functional/ExceptionTest.php
index 3b9649d5595..25824381048 100644
--- a/tests/Doctrine/Tests/DBAL/Functional/ExceptionTest.php
+++ b/tests/Doctrine/Tests/DBAL/Functional/ExceptionTest.php
@@ -77,8 +77,8 @@ public function testTableExistsException() : void
 
     public function testForeignKeyConstraintViolationExceptionOnInsert() : void
     {
-        if (! $this->connection->getDatabasePlatform()->supportsForeignKeyConstraints()) {
-            $this->markTestSkipped('Only fails on platforms with foreign key constraints.');
+        if ($this->connection->getDatabasePlatform()->getName() === 'sqlite') {
+            $this->connection->exec('PRAGMA foreign_keys=ON');
         }
 
         $this->setUpForeignKeyConstraintViolationExceptionTest();
diff --git a/tests/Doctrine/Tests/DBAL/Functional/Schema/SchemaManagerFunctionalTestCase.php b/tests/Doctrine/Tests/DBAL/Functional/Schema/SchemaManagerFunctionalTestCase.php
index 7b76f802612..669d78445fc 100644
--- a/tests/Doctrine/Tests/DBAL/Functional/Schema/SchemaManagerFunctionalTestCase.php
+++ b/tests/Doctrine/Tests/DBAL/Functional/Schema/SchemaManagerFunctionalTestCase.php
@@ -14,6 +14,7 @@
 use Doctrine\DBAL\Schema\Comparator;
 use Doctrine\DBAL\Schema\ForeignKeyConstraint;
 use Doctrine\DBAL\Schema\Index;
+use Doctrine\DBAL\Schema\Schema;
 use Doctrine\DBAL\Schema\SchemaDiff;
 use Doctrine\DBAL\Schema\Sequence;
 use Doctrine\DBAL\Schema\Table;
@@ -39,6 +40,7 @@
 use function current;
 use function end;
 use function explode;
+use function implode;
 use function in_array;
 use function sprintf;
 use function str_replace;
@@ -1604,4 +1606,49 @@ public function testCommentInTable() : void
         $table = $this->schemaManager->listTableDetails('table_with_comment');
         self::assertSame('Foo with control characters \'\\', $table->getComment());
     }
+
+    public function testSchemaDiffForeignKeys() : void
+    {
+        $schemaManager = $this->connection->getSchemaManager();
+        $platform      = $this->connection->getDatabasePlatform();
+
+        $table1 = new Table('child');
+        $table1->addColumn('id', 'integer', ['autoincrement' => true]);
+        $table1->addColumn('parent_id', 'integer');
+        $table1->setPrimaryKey(['id']);
+        $table1->addForeignKeyConstraint('parent', ['parent_id'], ['id']);
+
+        $table2 = new Table('parent');
+        $table2->addColumn('id', 'integer', ['autoincrement' => true]);
+        $table2->setPrimaryKey(['id']);
+
+        $diff = new SchemaDiff([$table1, $table2]);
+        $sqls = $diff->toSql($platform);
+
+        foreach ($sqls as $sql) {
+            $this->connection->exec($sql);
+        }
+
+        $schema = new Schema([
+            $schemaManager->listTableDetails('child'),
+            $schemaManager->listTableDetails('parent'),
+        ]);
+
+        $this->assertCount(1, $schema->getTable('child')->getForeignKeys());
+
+        $offlineSchema = new Schema([$table1, $table2]);
+
+        $diff = Comparator::compareSchemas($offlineSchema, $schema);
+
+        foreach ($diff->changedTables as $table) {
+            if (count($table->changedForeignKeys) <= 0 && count($table->addedForeignKeys) <= 0 && count($table->removedForeignKeys) <= 0) {
+                continue;
+            }
+
+            $this->fail(
+                'No changes on foreigh keys should be detected, but we have: ' .
+                implode(', ', $diff->toSql($platform))
+            );
+        }
+    }
 }
diff --git a/tests/Doctrine/Tests/DBAL/Internal/DependencyOrderCalculatorTest.php b/tests/Doctrine/Tests/DBAL/Internal/DependencyOrderCalculatorTest.php
new file mode 100644
index 00000000000..254145f0d18
--- /dev/null
+++ b/tests/Doctrine/Tests/DBAL/Internal/DependencyOrderCalculatorTest.php
@@ -0,0 +1,56 @@
+<?php
+
+namespace Doctrine\Tests\DBAL\Internal;
+
+use Doctrine\DBAL\Internal\DependencyOrderCalculator;
+use Doctrine\DBAL\Schema\Table;
+use Doctrine\Tests\DbalTestCase;
+
+/**
+ * Tests of the commit order calculation.
+ *
+ * IMPORTANT: When writing tests here consider that a lot of graph constellations
+ * can have many valid orderings, so you may want to build a graph that has only
+ * 1 valid order to simplify your tests.
+ */
+class DependencyOrderCalculatorTest extends DbalTestCase
+{
+    /** @var DependencyOrderCalculator */
+    private $calculator;
+
+    protected function setUp() : void
+    {
+        $this->calculator = new DependencyOrderCalculator();
+    }
+
+    public function testCommitOrdering1() : void
+    {
+        $table1 = new Table('table1');
+        $table2 = new Table('table2');
+        $table3 = new Table('table3');
+        $table4 = new Table('table4');
+        $table5 = new Table('table5');
+
+        $this->assertFalse($this->calculator->hasNode($table1->getName()));
+
+        $this->calculator->addNode($table1->getName(), $table1);
+        $this->calculator->addNode($table2->getName(), $table2);
+        $this->calculator->addNode($table3->getName(), $table3);
+        $this->calculator->addNode($table4->getName(), $table4);
+        $this->calculator->addNode($table5->getName(), $table5);
+
+        $this->assertTrue($this->calculator->hasNode($table1->getName()));
+
+        $this->calculator->addDependency($table1->getName(), $table2->getName());
+        $this->calculator->addDependency($table2->getName(), $table3->getName());
+        $this->calculator->addDependency($table3->getName(), $table4->getName());
+        $this->calculator->addDependency($table5->getName(), $table1->getName());
+
+        $sorted = $this->calculator->sort();
+
+        // There is only 1 valid ordering for this constellation
+        $correctOrder = [$table5, $table1, $table2, $table3, $table4];
+
+        $this->assertSame($correctOrder, $sorted);
+    }
+}
diff --git a/tests/Doctrine/Tests/DBAL/Platforms/AbstractPlatformTestCase.php b/tests/Doctrine/Tests/DBAL/Platforms/AbstractPlatformTestCase.php
index 1ef4e65e322..6ae57373cc3 100644
--- a/tests/Doctrine/Tests/DBAL/Platforms/AbstractPlatformTestCase.php
+++ b/tests/Doctrine/Tests/DBAL/Platforms/AbstractPlatformTestCase.php
@@ -254,6 +254,10 @@ abstract public function getGenerateForeignKeySql() : string;
 
     public function testGeneratesConstraintCreationSql() : void
     {
+        if (! $this->platform->supportsCreateDropForeignKeyConstraints()) {
+            $this->markTestSkipped('Platform does not support creating or dropping foreign key constraints.');
+        }
+
         $idx = new Index('constraint_name', ['test'], true, false);
         $sql = $this->platform->getCreateConstraintSQL($idx, 'test');
         self::assertEquals($this->getGenerateConstraintUniqueIndexSql(), $sql);
@@ -267,18 +271,6 @@ public function testGeneratesConstraintCreationSql() : void
         self::assertEquals($this->getGenerateConstraintForeignKeySql($fk), $sql);
     }
 
-    public function testGeneratesForeignKeySqlOnlyWhenSupportingForeignKeys() : void
-    {
-        $fk = new ForeignKeyConstraint(['fk_name'], 'foreign', ['id'], 'constraint_fk');
-
-        if ($this->platform->supportsForeignKeyConstraints()) {
-            self::assertIsString($this->platform->getCreateForeignKeySQL($fk, 'test'));
-        } else {
-            $this->expectException(DBALException::class);
-            $this->platform->getCreateForeignKeySQL($fk, 'test');
-        }
-    }
-
     protected function getBitAndComparisonExpressionSql(string $value1, string $value2) : string
     {
         return '(' . $value1 . ' & ' . $value2 . ')';
@@ -1123,9 +1115,9 @@ protected function getQuotedAlterTableRenameIndexInSchemaSQL() : array
      */
     public function testQuotesDropForeignKeySQL() : void
     {
-        if (! $this->platform->supportsForeignKeyConstraints()) {
+        if (! $this->platform->supportsCreateDropForeignKeyConstraints()) {
             $this->markTestSkipped(
-                sprintf('%s does not support foreign key constraints.', get_class($this->platform))
+                sprintf('%s does not support modifying foreign key constraints.', get_class($this->platform))
             );
         }
 
diff --git a/tests/Doctrine/Tests/DBAL/Schema/SchemaDiffTest.php b/tests/Doctrine/Tests/DBAL/Schema/SchemaDiffTest.php
index 87146c2f001..5a975662e7e 100644
--- a/tests/Doctrine/Tests/DBAL/Schema/SchemaDiffTest.php
+++ b/tests/Doctrine/Tests/DBAL/Schema/SchemaDiffTest.php
@@ -48,6 +48,10 @@ private function createPlatform(bool $unsafe)
             ->method('getCreateSchemaSQL')
             ->with('foo_ns')
             ->will($this->returnValue('create_schema'));
+
+        $platform->method('supportsCreateDropForeignKeyConstraints')
+            ->will($this->returnValue(true));
+
         if ($unsafe) {
             $platform->expects($this->exactly(1))
                  ->method('getDropSequenceSql')
@@ -95,7 +99,7 @@ private function createPlatform(bool $unsafe)
         $platform->expects($this->exactly(1))
                 ->method('supportsSequences')
                 ->will($this->returnValue(true));
-        $platform->expects($this->exactly(2))
+        $platform->expects($this->any())
                 ->method('supportsForeignKeyConstraints')
                 ->will($this->returnValue(true));