doctrine/dbal

Update schema has 2 time the same "drop foreign key"

Nikoms opened this issue · 18 comments

Nikoms commented

Hello!

On mysql, I have this issue with 2.5.5.

When I removed a Foreign key to a simple int, I get the "drop" twice.

ALTER TABLE my_table DROP FOREIGN KEY FK_CE1415A19EB6921;
ALTER TABLE my_table DROP FOREIGN KEY FK_CE1415A19EB6921;

I already checked a bit.
And I see here that we merge some sqls: doctrine/dbal/lib/Doctrine/DBAL/Platforms/MySqlPlatform.php:674

$this->getPreAlterTableAlterIndexForeignKeySQL($diff)
AND
parent::getPreAlterTableIndexForeignKeySQL($diff),

both return the "ALTER TABLE ... DROP FOREIGN FEY"

Maybe one of them should not return the "ALTER TABLE", but which one? or I guess we should add a array_unique?

If someone tell me, i can make a PR :)

Ocramius commented

@Nikoms is this specific to 2.5.5, as in "worked fine on 2.5.4"?

Nikoms commented

We were in v2.4.5. I'll try to test from version to version to know which version introduces this bug.

Nikoms commented

I can say that it's from 2.4.5 to 2.5.0

Like I said, it worked with 2.4.5 which is the last 2.4 version. And I checked with 2.5.0 and it does not work

Ocramius commented
Nikoms commented

Np :)

Ocramius commented

Maybe one of them should not return the "ALTER TABLE", but which one? or I guess we should add a array_unique?

Before looking at a solution, we need to have a test case reproducing the issue.

Could you try creating one?

websmurf commented

Hi @Ocramius

Running into the same issue, have set up a repo for it here: https://github.com/websmurf/doctrine-foreign-key-duplicate

Let me know if you need more.

Ocramius commented

@websmurf needs a test case running in our test suite.

websmurf commented

Hi @Ocramius, took me a while to rebuild the exact TableDiff structure, but I've committed it in a fork here: https://github.com/websmurf/dbal/blob/master/tests/Doctrine/Tests/DBAL/Platforms/AbstractMySQLPlatformTestCase.php#L894

Start it using:
vendor/bin/phpunit -c mysql.xml --group=DBAL-2576

What stands out for me is that it iterates the test twice, the first iteration actually gives a correct result, where the second iteration is giving a false result:

adam@mpb-adam dbal (master) $ vendor/bin/phpunit -c mysql.xml --group=DBAL-2576
PHPUnit 5.7.14 by Sebastian Bergmann and contributors.

.Array
(
    [0] => ALTER TABLE documents DROP FOREIGN KEY jos_documents_ibfk_1
    [1] => ALTER TABLE documents CHANGE document_params document_params LONGTEXT NOT NULL COMMENT '(DC2Type:json_array)', CHANGE document_state document_state TINYINT(1) NOT NULL
    [2] => ALTER TABLE documents ADD CONSTRAINT FK_a788692ffa0c224 FOREIGN KEY (office_id) REFERENCES offices (id) ON DELETE CASCADE
    [3] => ALTER TABLE documents RENAME INDEX office_id TO IDX_A788692FFA0C224
)
F                                                                  2 / 2 (100%)Array
(
    [0] => ALTER TABLE documents DROP FOREIGN KEY jos_documents_ibfk_1
    [1] => ALTER TABLE documents DROP FOREIGN KEY jos_documents_ibfk_1
    [2] => ALTER TABLE documents CHANGE document_params document_params LONGTEXT NOT NULL COMMENT '(DC2Type:json_array)', CHANGE document_state document_state TINYINT(1) NOT NULL
    [3] => ALTER TABLE documents ADD CONSTRAINT FK_a788692ffa0c224 FOREIGN KEY (office_id) REFERENCES offices (id) ON DELETE CASCADE
    [4] => DROP INDEX office_id ON documents
    [5] => CREATE INDEX IDX_A788692FFA0C224 ON documents (office_id)
    [6] => ALTER TABLE documents ADD CONSTRAINT jos_documents_ibfk_1 FOREIGN KEY (office_id) REFERENCES offices (id) ON UPDATE RESTRICT ON DELETE RESTRICT
)


Time: 1.35 seconds, Memory: 26.00MB

There was 1 failure:

1) Doctrine\Tests\DBAL\Platforms\MySqlPlatformTest::testNotDuplicateDropForeignKeySql
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
     6 => 'ALTER TABLE documents ADD CON...STRICT'
+    1 => 'ALTER TABLE documents DROP FO...ibfk_1'
 )

/Users/adam/Sites/github/dbal/tests/Doctrine/Tests/DBAL/Platforms/AbstractMySQLPlatformTestCase.php:973
websmurf commented

Added a pull request for it as well as cleaning up the code a bit.

websmurf commented

It turns out that mysql57 gives the correct result because the getPreAlterTableRenameIndexForeignKeySQL is overwritten in the MySQL57Platform.php file (so that it nevers gives a result back)

websmurf commented

Ok, I'm another step closer to the resolution.

In the getRemainingForeignKeyConstraintsRequiringRenamedIndexes method, there's a piece that compares which foreign keys have already been updated. Problem is, that it searches by array key, and in the diff, that's not set correctly.

This it's what it's set to:

$diff->addedForeignKeys[
  0  => // foreign key details
]

while it should be this to get a valid result:

$diff->addedForeignKeys[
  'foreignKeyName'  => // foreign key details
]

Now to find where the TableDiff is created :-)

websmurf commented

Ok, fix is committed to the pull request :-)

Not too sure what the impact might be on platforms other than mysql though..

websmurf commented

@Ocramius any update on this?

Ocramius commented

@websmurf no, otherwise you would have gotten the notifications.

automatix commented

Just got the issue ina project. @Ocramius What is about @websmurf's pull request with the fix? Can you accept it?

morozov commented

@automatix the pull request has merge conflicts and a build failure. During the last year, we added a few more platforms to the test matrix, so it makes sense to rebase is and fix the build.

websmurf commented

Rebased it and fixed a few small styling issues that the last step complained about.