Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
May 2, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions lib/Doctrine/DBAL/Driver/AbstractSQLiteDriver.php
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down
131 changes: 131 additions & 0 deletions lib/Doctrine/DBAL/Internal/DependencyOrderCalculator.php
Original file line number Diff line number Diff line change
@@ -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;
}
}
12 changes: 12 additions & 0 deletions lib/Doctrine/DBAL/Internal/DependencyOrderEdge.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
<?php

namespace Doctrine\DBAL\Internal;

class DependencyOrderEdge
{
/** @var string */
public $from;

/** @var string */
public $to;
}
18 changes: 18 additions & 0 deletions lib/Doctrine/DBAL/Internal/DependencyOrderNode.php
Original file line number Diff line number Diff line change
@@ -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 = [];
}
10 changes: 10 additions & 0 deletions lib/Doctrine/DBAL/Platforms/AbstractPlatform.php
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*
Expand Down
12 changes: 10 additions & 2 deletions lib/Doctrine/DBAL/Platforms/SqlitePlatform.php
Original file line number Diff line number Diff line change
Expand Up @@ -769,6 +769,14 @@ public function canEmulateSchemas()
* {@inheritDoc}
*/
public function supportsForeignKeyConstraints()
{
return true;
}

/**
* {@inheritDoc}
*/
public function supportsCreateDropForeignKeyConstraints() : bool
{
return false;
}
Expand All @@ -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.');
}

/**
Expand Down
49 changes: 43 additions & 6 deletions lib/Doctrine/DBAL/Schema/SchemaDiff.php
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

namespace Doctrine\DBAL\Schema;

use Doctrine\DBAL\Internal\DependencyOrderCalculator;
use Doctrine\DBAL\Platforms\AbstractPlatform;
use function array_merge;

Expand Down Expand Up @@ -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;
}

Expand All @@ -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();
}
}
4 changes: 4 additions & 0 deletions lib/Doctrine/DBAL/Schema/Visitor/DropSchemaSqlCollector.php
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
6 changes: 6 additions & 0 deletions phpstan.neon.dist
Original file line number Diff line number Diff line change
Expand Up @@ -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~'
Copy link
Member

@morozov morozov Jun 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@beberlei how is this a false positive?

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


# Needs Generics
- '~Method Doctrine\\DBAL\\Schema\\SchemaDiff::getNewTablesSortedByDependencies\(\) should return array<Doctrine\\DBAL\\Schema\\Table> but returns array<object>.~'
4 changes: 2 additions & 2 deletions tests/Doctrine/Tests/DBAL/Functional/ExceptionTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

continue;
}

$this->fail(
'No changes on foreigh keys should be detected, but we have: ' .
implode(', ', $diff->toSql($platform))
);
}
}
}
Loading