Merge pull request #3762 from doctrine/GH-1204-CleanupSqliteForeignKeySupport

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

lib/Doctrine/DBAL/Driver/AbstractSQLiteDriver.php

@@ -67,6 +67,10 @@ abstract class AbstractSQLiteDriver implements Driver, ExceptionConverterDriver
             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

+<?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

+<?php
+
+namespace Doctrine\DBAL\Internal;
+
+class DependencyOrderEdge
+{
+    /** @var string */
+    public $from;
+
+    /** @var string */
+    public $to;
+}

lib/Doctrine/DBAL/Internal/DependencyOrderNode.php

+<?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 @@ abstract class AbstractPlatform
         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 @@ class SqlitePlatform extends AbstractPlatform
      * {@inheritDoc}
      */
     public function supportsForeignKeyConstraints()
+    {
+        return true;
+    }
+
+    /**
+     * {@inheritDoc}
+     */
+    public function supportsCreateDropForeignKeyConstraints() : bool
     {
         return false;
     }
@@ -786,7 +794,7 @@ class SqlitePlatform extends AbstractPlatform
      */
     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 @@ class SqlitePlatform extends AbstractPlatform
      */
     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 @@ class SchemaDiff
         }
 
         $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 @@ class SchemaDiff
 
         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 @@ class DropSchemaSqlCollector extends AbstractVisitor
      */
     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 @@ class ExceptionTest extends DbalFunctionalTestCase
 
     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\ColumnDiff;
 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 count;
 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 @@ abstract class SchemaManagerFunctionalTestCase extends DbalFunctionalTestCase
         $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))
+            );
+        }
+    }
 }

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

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

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

@@ -254,6 +254,10 @@ abstract class AbstractPlatformTestCase extends DbalTestCase
 
     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 @@ abstract class AbstractPlatformTestCase extends DbalTestCase
         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 @@ abstract class AbstractPlatformTestCase extends DbalTestCase
      */
     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))
             );
         }
 

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

@@ -48,6 +48,10 @@ class SchemaDiffTest extends TestCase
             ->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 @@ class SchemaDiffTest extends TestCase
         $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));