Merge pull request #3762 from doctrine/GH-1204-CleanupSqliteForeignKe…

…ySupport

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

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);
     }
 

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;
+    }
+}

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;
+}

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 = [];
+}

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.
      *

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,15 +794,15 @@ 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.');
     }
 
     /**
      * {@inheritdoc}
      */
     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.');
     }
 
     /**

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();
+    }
 }

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);
         }

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>.~'

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();

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))
+            );
+        }
+    }
 }