Fix schema comparison on tables with multiple foreign keys referencing the same tables and columns

[mickverm]

Q	A
Type	bug
Version	4.3.2

Summary

When attempting to run a single migration, I encountered the following MySQL error:

php bin/console doctrine:migrations:execute 'DoctrineMigrations\Version00000008000000'

 WARNING! You are about to execute a migration in database "development" that could result in schema changes and data loss. Are you sure you wish to continue? (yes/no) [yes]:
 > 

[notice] Executing DoctrineMigrations\Version00000008000000 up
[error] Migration DoctrineMigrations\Version00000008000000 failed during Execution. Error: "An exception occurred while executing a query: SQLSTATE[42000]: Syntax error or access violation: 1064 You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near '-table DROP FOREIGN KEY `FK_redacted`' at line 1"

In ExceptionConverter.php line 67:
                                                                                                                                                                                                                                                                                                                              
  An exception occurred while executing a query: SQLSTATE[42000]: Syntax error or access violation: 1064 You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near '-table DROP FOREIGN KEY `FK_redacted`' at line 1    
                                                                                                                                                                                                                                                                                                                              

In Exception.php line 24:

  SQLSTATE[42000]: Syntax error or access violation: 1064 You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near '-table DROP FOREIGN KEY `FK_redacted`' at line 1  


In Connection.php line 57:

  SQLSTATE[42000]: Syntax error or access violation: 1064 You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near '-table DROP FOREIGN KEY `FK_redacted`' at line 1  


doctrine:migrations:execute [--write-sql [WRITE-SQL]] [--dry-run] [--up] [--down] [--query-time] [--configuration CONFIGURATION] [--em EM] [--conn CONN] [--] <versions>...

This error is not caused by the migration itself, but by a table name that was not properly escaped.

-- Failing query:
ALTER TABLE users-table DROP FOREIGN KEY `FK_redacted`

-- Correct query:
ALTER TABLE `users-table` DROP FOREIGN KEY `FK_redacted`

This query wasn't even defined in the migration, it was introduced during execution of the migration via the following code: DbalExecutor.php#L144

In this case, the users-table table had two foreign keys (FK_redacted and FK_redacted_2) pointing to the same referenced table and columns.

I implemented a small fix in the foreign key change detection and added a test case to cover this scenario.

Note: I acknowledge that double foreign keys and table names with - symbols are not best practice, but I was working with a legacy database.

[derrabus]

Thank you.

I've merged a workaround for the broken DB2 CI. I'd like to see your changes tested against DB2 as well which is why I've rebased your branch. I hope that was okay for you. 🙂

[derrabus]

Thank you for your PR. That is an interesting edge case indeed.

What about the following scenario: I have a database with two identical foreign keys and compare it to a configured schema with only one foreign key. Would the comparator detect this and as a consequence drop one of the foreign keys? Can we cover this with a test maybe?

[mickverm]

I've added the test case, removed the DB2 skip.

[mickverm]

I'm guessing the DB2 skip is necessary? https://github.com/doctrine/dbal/actions/runs/17487261825/job/49669823130

[morozov]

The fix itself looks fine.

What I didn't know is that the comparator apparently doesn't detect renamed foreign key constraints. It compares them only by value, and if they match, it will unset the corresponding array keys in the old and new arrays.

Here's a slightly modified original test which reproduces this issue:

$articles = Table::editor()
    ->setUnquotedName('articles')
    ->setColumns(
        Column::editor()
            ->setUnquotedName('id')
            ->setTypeName(Types::INTEGER)
            ->create(),
    )
    ->setPrimaryKeyConstraint(
        PrimaryKeyConstraint::editor()
            ->setUnquotedColumnNames('id')
            ->create(),
    )
    ->create();

$orders = Table::editor()
    ->setUnquotedName('orders')
    ->setColumns(
        Column::editor()
            ->setUnquotedName('id')
            ->setTypeName(Types::INTEGER)
            ->create(),
        Column::editor()
            ->setUnquotedName('article_id')
            ->setTypeName(Types::INTEGER)
            ->create(),
    )
    ->setForeignKeyConstraints(
        ForeignKeyConstraint::editor()
            ->setUnquotedName('articles_fk_1')
            ->setUnquotedReferencingColumnNames('article_id')
            ->setUnquotedReferencedTableName('articles')
            ->setUnquotedReferencedColumnNames('id')
            ->create(),
    )
    ->create();

$this->dropTableIfExists('orders');
$this->dropTableIfExists('articles');

$this->connection->createSchemaManager()
    ->createTable($articles);
$this->connection->createSchemaManager()
    ->createTable($orders);

$ordersActual = $this->schemaManager->introspectTable('orders');

$orders = $orders->edit()
    ->setForeignKeyConstraints(
        ForeignKeyConstraint::editor()
            ->setUnquotedName('articles_fk_2')
            ->setUnquotedReferencingColumnNames('article_id')
            ->setUnquotedReferencedTableName('articles')
            ->setUnquotedReferencedColumnNames('id')
            ->create(),
    )
    ->create();

self::assertFalse(
    $this->schemaManager->createComparator()
        ->compareTables($ordersActual, $orders)
        ->isEmpty(),
);

What it does is:

1. Creates a table with articles_fk_1.
2. Replaces articles_fk_1 with an identical articles_fk_2.
3. Compares the tables.

It is expected that there is a diff, but there is none.

I'm fine with merging the fix that only addresses the original issue, but I'm curious if we should fix the root cause: the constraints should be compared by value and name (unless either of the old/new is unnamed), not just by value – that's why the original issue exists.

[morozov]

As I understand, this is the fix. The rest of the lines have been mostly affected to satisfy the coding standard.

The essence of the fix is that once we have found a matching pair of new key / old key, we not only continue the inner loop, we continue the outer one as well. Otherwise, the other equal key will also match.

Is that correct?

[mickverm]

Yes, the continue 2; is the actual change. It stops comparing the new key when a matching old key has been found.

[morozov]

It looks like this test passes even without the fix.

[mickverm]

This is the regression test @derrabus asked for.

[morozov]

Should this be named "articles_fk_1" for consistency?