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.

Sign up for GitHub

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

Remove handling orphaned foreign keys #5763

Merged
merged 1 commit into from
Oct 16, 2022
Merged

Remove handling orphaned foreign keys #5763

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
7 changes: 4 additions & 3 deletions UPGRADE.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,10 @@ awareness about deprecated code.

# Upgrade to 4.0

## Removed `SchemaDiff::$orphanedForeignKeys`

The functionality of automatically dropping the foreign keys referencing the tables being dropped has been removed.

## BC Break: Removed registration of user defined functions for SQLite

DBAL does not register functions for SQLite anymore. The following functions
Expand Down Expand Up @@ -571,9 +575,6 @@ Reference from `ForeignKeyConstraint` to its local (referencing) `Table` is remo
- `getLocalTable()`,
- `getLocalTableName()`.

The type of `SchemaDiff::$orphanedForeignKeys` has changed from list of foreign keys (`list<ForeignKeyConstraint>`)
to map of referencing tables to their list of foreign keys (`array<string,list<ForeignKeyConstraint>>`).

## BC BREAK: Removed redundant `AbstractPlatform` methods.

The following redundant `AbstractPlatform` methods have been removed:
Expand Down
55 changes: 3 additions & 52 deletions src/Schema/Comparator.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,14 +36,6 @@ public function compareSchemas(Schema $fromSchema, Schema $toSchema): SchemaDiff
$alteredSequences = [];
$droppedSequences = [];

$orphanedForeignKeys = [];

/** @var array<string,list<array{ForeignKeyConstraint,string}>> $foreignKeysToTable */
$foreignKeysToTable = [];

/** @var array<string,array<string,null>> $localTablesByForeignTable */
$localTablesByForeignTable = [];

foreach ($toSchema->getNamespaces() as $namespace) {
if ($fromSchema->hasNamespace($namespace)) {
continue;
Expand Down Expand Up @@ -81,48 +73,11 @@ public function compareSchemas(Schema $fromSchema, Schema $toSchema): SchemaDiff
$tableName = $table->getShortestName($fromSchema->getName());

$table = $fromSchema->getTable($tableName);
if (! $toSchema->hasTable($tableName)) {
$droppedTables[$tableName] = $table;
}

// also remember all foreign keys that point to a specific table
foreach ($table->getForeignKeys() as $foreignKey) {
$foreignTable = strtolower($foreignKey->getForeignTableName());

$foreignKeysToTable[$foreignTable][] = [$foreignKey, $table->getName()];
$localTablesByForeignTable[$foreignTable][$tableName] = null;
}
}

foreach ($droppedTables as $tableName => $table) {
if (! isset($foreignKeysToTable[$tableName])) {
if ($toSchema->hasTable($tableName)) {
continue;
}

foreach ($foreignKeysToTable[$tableName] as [$foreignKey, $localTableName]) {
if (isset($droppedTables[strtolower($localTableName)])) {
continue;
}

$orphanedForeignKeys[$tableName][] = $foreignKey;
}

// deleting duplicated foreign keys present on both on the orphanedForeignKey
// and the removedForeignKeys from changedTables
foreach ($localTablesByForeignTable[$tableName] as $localTableName => $_) {
if (! isset($alteredTables[$localTableName])) {
continue;
}

foreach ($alteredTables[$localTableName]->getDroppedForeignKeys() as $droppedForeignKey) {
// We check if the key is from the removed table if not we skip.
if ($tableName !== strtolower($droppedForeignKey->getForeignTableName())) {
continue;
}

$alteredTables[$localTableName]->unsetDroppedForeignKey($droppedForeignKey);
}
}
$droppedTables[$tableName] = $table;
}

foreach ($toSchema->getSequences() as $sequence) {
Expand Down Expand Up @@ -152,7 +107,7 @@ public function compareSchemas(Schema $fromSchema, Schema $toSchema): SchemaDiff
$droppedSequences[] = $sequence;
}

$diff = new SchemaDiff(
return new SchemaDiff(
$createdTables,
$alteredTables,
$droppedTables,
Expand All @@ -162,10 +117,6 @@ public function compareSchemas(Schema $fromSchema, Schema $toSchema): SchemaDiff
$alteredSequences,
$droppedSequences,
);

$diff->orphanedForeignKeys = $orphanedForeignKeys;

return $diff;
}

private function isAutoIncrementSequenceInSchema(Schema $schema, Sequence $sequence): bool
Expand Down
18 changes: 0 additions & 18 deletions src/Schema/SchemaDiff.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,13 +53,6 @@ class SchemaDiff
*/
public array $removedSequences = [];

/**
* @deprecated
*
* @var array<string,list<ForeignKeyConstraint>>
*/
public array $orphanedForeignKeys = [];

/**
* Constructs an SchemaDiff object.
*
Expand Down Expand Up @@ -175,17 +168,6 @@ protected function _toSql(AbstractPlatform $platform, bool $saveMode = false): a
}
}

if ($saveMode === false) {
foreach ($this->orphanedForeignKeys as $localTableName => $tableOrphanedForeignKey) {
foreach ($tableOrphanedForeignKey as $orphanedForeignKey) {
$sql[] = $platform->getDropForeignKeySQL(
$orphanedForeignKey->getQuotedName($platform),
$localTableName,
);
}
}
}

if ($platform->supportsSequences()) {
foreach ($this->getAlteredSequences() as $sequence) {
$sql[] = $platform->getAlterSequenceSQL($sequence);
Expand Down
13 changes: 1 addition & 12 deletions src/Schema/TableDiff.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ public function __construct(
private readonly array $renamedIndexes,
private readonly array $addedForeignKeys,
private readonly array $modifiedForeignKeys,
private array $droppedForeignKeys,
private readonly array $droppedForeignKeys,
) {
}

Expand Down Expand Up @@ -142,15 +142,4 @@ public function getDroppedForeignKeys(): array
{
return $this->droppedForeignKeys;
}

/** @internal This method exists only for compatibility with the current implementation of the schema comparator. */
public function unsetDroppedForeignKey(ForeignKeyConstraint $foreignKey): void
{
$this->droppedForeignKeys = array_filter(
$this->droppedForeignKeys,
static function (ForeignKeyConstraint $droppedForeignKey) use ($foreignKey): bool {
return $droppedForeignKey !== $foreignKey;
},
);
}
}
59 changes: 0 additions & 59 deletions tests/Schema/ComparatorTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -619,44 +619,6 @@ public function testAutoIncrementNoSequences(): void
self::assertCount(0, $diff->newSequences);
}

/**
* You can get multiple drops for a FK when a table referenced by a foreign
* key is deleted, as this FK is referenced twice, once on the orphanedForeignKeys
* array because of the dropped table, and once on changedTables array. We
* now check that the key is present once.
*/
public function testAvoidMultipleDropForeignKey(): void
{
$oldSchema = new Schema();

$tableA = $oldSchema->createTable('table_a');
$tableA->addColumn('id', 'integer');

$tableB = $oldSchema->createTable('table_b');
$tableB->addColumn('id', 'integer');

$tableC = $oldSchema->createTable('table_c');
$tableC->addColumn('id', 'integer');
$tableC->addColumn('table_a_id', 'integer');
$tableC->addColumn('table_b_id', 'integer');

$tableC->addForeignKeyConstraint($tableA->getName(), ['table_a_id'], ['id']);
$tableC->addForeignKeyConstraint($tableB->getName(), ['table_b_id'], ['id']);

$newSchema = new Schema();

$tableB = $newSchema->createTable('table_b');
$tableB->addColumn('id', 'integer');

$tableC = $newSchema->createTable('table_c');
$tableC->addColumn('id', 'integer');

$schemaDiff = $this->comparator->compareSchemas($oldSchema, $newSchema);

self::assertCount(1, $schemaDiff->changedTables['table_c']->getDroppedForeignKeys());
Copy link
Member Author

@morozov morozov Oct 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This test is being removed as invalid. Consider instrumenting it with the following code:

$platform = new MySQLPlatform();

echo '-- Schema', PHP_EOL;
foreach ($oldSchema->toSql($platform) as $statement) {
    echo $statement, ';', PHP_EOL;
}

echo '-- Diff', PHP_EOL;
foreach ($schemaDiff->toSql($platform) as $statement) {
    echo $statement, ';', PHP_EOL;
}

If run on 4.0.x, it produces the following statements (the PRIMARY KEY clauses are not defined by the test schema and were added manually for the sake of correctness):

-- Schema
CREATE TABLE table_a (id INT NOT NULL PRIMARY KEY);
CREATE TABLE table_b (id INT NOT NULL PRIMARY KEY);
CREATE TABLE table_c (id INT NOT NULL, table_a_id INT NOT NULL, table_b_id INT NOT NULL, INDEX IDX_7365E39D71081393 (table_a_id), INDEX IDX_7365E39D63BDBC7D (table_b_id));
ALTER TABLE table_c ADD CONSTRAINT FK_7365E39D71081393 FOREIGN KEY (table_a_id) REFERENCES table_a (id);
ALTER TABLE table_c ADD CONSTRAINT FK_7365E39D63BDBC7D FOREIGN KEY (table_b_id) REFERENCES table_b (id);

-- Diff
ALTER TABLE table_a DROP FOREIGN KEY FK_7365E39D71081393;
DROP TABLE table_a;
ALTER TABLE table_c DROP FOREIGN KEY FK_7365E39D63BDBC7D;
DROP INDEX IDX_7365E39D71081393 ON table_c;
DROP INDEX IDX_7365E39D63BDBC7D ON table_c;
ALTER TABLE table_c DROP table_a_id, DROP table_b_id;

The assertion in question passes only because the DROP FOREIGN KEY FK_7365E39D71081393 statement is mistakenly executed on table_a although it should be executed on table_c.

With the functionality of handling orphaned foreign keys removed, the test starts failing because now the SQL looks like this:

-- Schema
CREATE TABLE table_a (id INT NOT NULL PRIMARY KEY);
CREATE TABLE table_b (id INT NOT NULL PRIMARY KEY);
CREATE TABLE table_c (id INT NOT NULL, table_a_id INT NOT NULL, table_b_id INT NOT NULL, INDEX IDX_7365E39D71081393 (table_a_id), INDEX IDX_7365E39D63BDBC7D (table_b_id));
ALTER TABLE table_c ADD CONSTRAINT FK_7365E39D71081393 FOREIGN KEY (table_a_id) REFERENCES table_a (id);
ALTER TABLE table_c ADD CONSTRAINT FK_7365E39D63BDBC7D FOREIGN KEY (table_b_id) REFERENCES table_b (id);

-- Diff
DROP TABLE table_a;
ALTER TABLE table_c DROP FOREIGN KEY FK_7365E39D71081393;
ALTER TABLE table_c DROP FOREIGN KEY FK_7365E39D63BDBC7D;
DROP INDEX IDX_7365E39D71081393 ON table_c;
DROP INDEX IDX_7365E39D63BDBC7D ON table_c;
ALTER TABLE table_c DROP table_a_id, DROP table_b_id;

Both constraints are dropped on table_c (which is correct) but this is not what the assertion expects.

Note that the resulting SQL is still invalid because table_a cannot be dropped while it's still referenced by a foreign key. The functionality of dropping multiple tables should be reworked accordingly (see the API introduced in #5416) but it seems to be out of the scope of this change since it didn't work properly even before this change.

Copy link
Member

Choose a reason for hiding this comment

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

A test that was passing by accident… must have been quite confusing, thanks for the detailed explanation!

self::assertCount(1, $schemaDiff->orphanedForeignKeys);
}

public function assertSchemaTableChangeCount(
SchemaDiff $diff,
int $newTableCount = 0,
Expand Down Expand Up @@ -790,8 +752,6 @@ public function testForeignKeyRemovalWithRenamedLocalColumn(): void
$schemaDiff = $this->comparator->compareSchemas($fromSchema, $toSchema);

self::assertArrayHasKey('table2', $schemaDiff->changedTables);
self::assertCount(1, $schemaDiff->orphanedForeignKeys);
self::assertEquals('fk_table2_table1', $schemaDiff->orphanedForeignKeys['table1'][0]->getName());

$tableDiff = $schemaDiff->changedTables['table2'];

Expand Down Expand Up @@ -837,25 +797,6 @@ public function testWillNotProduceSchemaDiffOnTableWithAddedCustomSchemaDefiniti
);
}

public function testNoOrphanedForeignKeyIfReferencingTableIsDropped(): void
{
$schema1 = new Schema();

$parent = $schema1->createTable('parent');
$parent->addColumn('id', 'integer');

$child = $schema1->createTable('child');
$child->addColumn('id', 'integer');
$child->addColumn('parent_id', 'integer');
$child->addForeignKeyConstraint('parent', ['parent_id'], ['id']);

$schema2 = new Schema();

$diff = $this->comparator->compareSchemas($schema1, $schema2);

self::assertEmpty($diff->orphanedForeignKeys);
}

/**
* @param array<AbstractAsset> $assets
*
Expand Down