Skip to content
This repository

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Fix dropping foreign key multiple times with test #212

Merged
merged 3 commits into from over 1 year ago

5 participants

Sergi de Pablos doctrinebot Coveralls Christophe Coevoet Benjamin Eberlei
Sergi de Pablos

In some cases the Comparator class returns multiple drops for the same foreign key.
Specifically, in case you have two tables, A & B, with A having a foreign key FK
referencing B, if you drop table B, the resulting diff shows this FK twice,
once on the diff->orphanedForeignKeys array as we're deleting B, and another on
the diff->changedTables array as table A is also being modified. As a result of this you
get the DROP FOREIGN KEY instruction twice in the final SQL.

Sergi de Pablos Fix dropping foreign key multiple times with test
In some cases the Comparator class returns multiple drops for the same foreign key.
Specifically, in case you have two tables, A & B, with A having a foreign key FK
referencing B, if you drop table B, the resulting diff shows this FK twice,
once on the diff->orphanedForeignKeys array as we're deleting B, and another on
the diff->changedTables array as table A is also being modified. As a result of this you
get the DROP FOREIGN KEY instruction twice in the final SQL.
04208c6
doctrinebot
Collaborator

Hello,

thank you for positing this Pull Request. I have automatically opened an issue on our Jira Bug Tracker for you with the details of this Pull-Request. See the Link:

http://doctrine-project.org/jira/browse/DBAL-360

lib/Doctrine/DBAL/Schema/Comparator.php
@@ -46,7 +46,7 @@ static public function compareSchemas( Schema $fromSchema, Schema $toSchema )
46 46
     /**
47 47
      * Returns a SchemaDiff object containing the differences between the schemas $fromSchema and $toSchema.
48 48
      *
49  
-     * The returned differences are returned in such a way that they contain the
  49
+     * The returned diferences are returned in such a way that they contain the
1
Christophe Coevoet
stof added a note October 04, 2012

Please revert this typo

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
lib/Doctrine/DBAL/Schema/Comparator.php
@@ -262,7 +272,7 @@ public function diffTable(Table $table1, Table $table2)
262 272
 
263 273
     /**
264 274
      * Try to find columns that only changed their name, rename operations maybe cheaper than add/drop
265  
-     * however ambiguities between different possibilities should not lead to renaming at all.
  275
+     * however ambiguouties between different possibilites should not lead to renaming at all.
1
Christophe Coevoet
stof added a note October 04, 2012

please revert this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Christophe Coevoet stof commented on the diff October 04, 2012
tests/Doctrine/Tests/DBAL/Schema/ComparatorTest.php
@@ -781,6 +781,32 @@ public function testAutoIncremenetSequences()
781 781
         $this->assertCount(0, $diff->removedSequences);
782 782
     }
783 783
 
  784
+
  785
+    /**
  786
+     * You can get multiple drops for a FK when 
1
Christophe Coevoet
stof added a note October 04, 2012

the end is missing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Sergi de Pablos

I provide an example of the bug. Having the following Foo entity

namespace Test\HelloBundle\Entity;

use Doctrine\ORM\Mapping as ORM;

/**
 * Test\HelloBundle\Entity\foo
 *
 * @ORM\Table()
 * @ORM\Entity
 */
class Foo
{
    /**
     * @var integer $id
     *
     * @ORM\Column(name="id", type="integer")
     * @ORM\Id
     * @ORM\GeneratedValue(strategy="AUTO")
     */
    private $id;
}

we add a new table ForeignTable

namespace Test\HelloBundle\Entity;

use Doctrine\ORM\Mapping as ORM;

/**
 * Test\HelloBundle\Entity\ForeignTable
 *
 * @ORM\Table()
 * @ORM\Entity
 */
class ForeignTable
{
    /**
     * @var integer $id
     *
     * @ORM\Column(name="id", type="integer")
     * @ORM\Id
     * @ORM\GeneratedValue(strategy="AUTO")
     */
    private $id;
}

referenced by Foo

namespace Test\HelloBundle\Entity;

use Doctrine\ORM\Mapping as ORM;

/**
 * Test\HelloBundle\Entity\foo
 *
 * @ORM\Table()
 * @ORM\Entity
 */
class Foo
{
    /**
     * @var integer $id
     *
     * @ORM\Column(name="id", type="integer")
     * @ORM\Id
     * @ORM\GeneratedValue(strategy="AUTO")
     */
    private $id;

    /**
     * @ORM\ManyToOne(targetEntity="ForeignTable")
     */
    private $fk;
}

When executing the diff, before the changes we got

public function down(Schema $schema)
{
    // this down() migration is autogenerated, please modify it to your needs
    $this->abortIf($this->connection->getDatabasePlatform()->getName() != "mysql");

    $this->addSql("ALTER TABLE Foo DROP FOREIGN KEY FK_B43E23C1A57719D0");
    $this->addSql("DROP TABLE ForeignTable");
    $this->addSql("ALTER TABLE Foo DROP FOREIGN KEY FK_B43E23C1A57719D0");
    $this->addSql("DROP INDEX IDX_B43E23C1A57719D0 ON Foo");
    $this->addSql("ALTER TABLE Foo DROP fk_id");
}

with the ALTER TABLE Foo DROP FOREIGN KEY FK_B43E23C1A57719D0 twice, but applying this changes we now correctly get

public function down(Schema $schema)
{
    // this down() migration is autogenerated, please modify it to your needs
    $this->abortIf($this->connection->getDatabasePlatform()->getName() != "mysql");

    $this->addSql("ALTER TABLE Foo DROP FOREIGN KEY FK_B43E23C1A57719D0");
    $this->addSql("DROP TABLE ForeignTable");
    $this->addSql("DROP INDEX IDX_B43E23C1A57719D0 ON Foo");
    $this->addSql("ALTER TABLE Foo DROP fk_id");
}
Benjamin Eberlei beberlei merged commit 9a467f1 into from October 05, 2012
Benjamin Eberlei beberlei closed this October 05, 2012
Coveralls

Coverage Status

Changes Unknown when pulling 283ff9b on sdepablos:master into ** on doctrine:master**.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Showing 3 unique commits by 1 author.

Oct 04, 2012
Sergi de Pablos Fix dropping foreign key multiple times with test
In some cases the Comparator class returns multiple drops for the same foreign key.
Specifically, in case you have two tables, A & B, with A having a foreign key FK
referencing B, if you drop table B, the resulting diff shows this FK twice,
once on the diff->orphanedForeignKeys array as we're deleting B, and another on
the diff->changedTables array as table A is also being modified. As a result of this you
get the DROP FOREIGN KEY instruction twice in the final SQL.
04208c6
Oct 05, 2012
Sergi de Pablos Fixed typos, finished comment on test and added new assert. 3a1c84e
Sergi de Pablos Fix bug with CamelCase table names 283ff9b
This page is out of date. Refresh to see the latest.
14  lib/Doctrine/DBAL/Schema/Comparator.php
@@ -24,7 +24,7 @@
24 24
  *
25 25
  * @copyright Copyright (C) 2005-2009 eZ Systems AS. All rights reserved.
26 26
  * @license http://ez.no/licenses/new_bsd New BSD License
27  
- * 
  27
+ *
28 28
  * @link    www.doctrine-project.org
29 29
  * @since   2.0
30 30
  * @version $Revision$
@@ -95,6 +95,18 @@ public function compare(Schema $fromSchema, Schema $toSchema)
95 95
         foreach ($diff->removedTables as $tableName => $table) {
96 96
             if (isset($foreignKeysToTable[$tableName])) {
97 97
                 $diff->orphanedForeignKeys = array_merge($diff->orphanedForeignKeys, $foreignKeysToTable[$tableName]);
  98
+
  99
+                // deleting duplicated foreign keys present on both on the orphanedForeignKey
  100
+                // and the removedForeignKeys from changedTables
  101
+                foreach ($foreignKeysToTable[$tableName] as $foreignKey) {
  102
+                    // strtolower the table name to make if compatible with getShortestName
  103
+                    $localTableName = strtolower($foreignKey->getLocalTableName());
  104
+                    if (isset($diff->changedTables[$localTableName])) {
  105
+                        foreach ($diff->changedTables[$localTableName]->removedForeignKeys as $key => $removedForeignKey) {
  106
+                            unset($diff->changedTables[$localTableName]->removedForeignKeys[$key]);
  107
+                        }
  108
+                    }
  109
+                }
98 110
             }
99 111
         }
100 112
 
30  tests/Doctrine/Tests/DBAL/Schema/ComparatorTest.php
@@ -781,6 +781,36 @@ public function testAutoIncremenetSequences()
781 781
         $this->assertCount(0, $diff->removedSequences);
782 782
     }
783 783
 
  784
+
  785
+    /**
  786
+     * You can get multiple drops for a FK when a table referenced by a foreign
  787
+     * key is deleted, as this FK is referenced twice, once on the orphanedForeignKeys
  788
+     * array because of the dropped table, and once on changedTables array. We
  789
+     * now check that the key is present once.
  790
+     */
  791
+    public function testAvoidMultipleDropForeignKey()
  792
+    {
  793
+        $oldSchema = new Schema();
  794
+
  795
+        $tableForeign = $oldSchema->createTable('foreign');
  796
+        $tableForeign->addColumn('id', 'integer');
  797
+
  798
+        $table = $oldSchema->createTable('foo');
  799
+        $table->addColumn('fk', 'integer');
  800
+        $table->addForeignKeyConstraint($tableForeign, array('fk'), array('id'));
  801
+
  802
+
  803
+        $newSchema = new Schema();
  804
+        $table = $newSchema->createTable('foo');
  805
+
  806
+        $c = new Comparator();
  807
+        $diff = $c->compare($oldSchema, $newSchema);
  808
+
  809
+        $this->assertCount(0, $diff->changedTables['foo']->removedForeignKeys);
  810
+        $this->assertCount(1, $diff->orphanedForeignKeys);
  811
+    }
  812
+
  813
+
784 814
     /**
785 815
      * @param SchemaDiff $diff
786 816
      * @param int $newTableCount
Commit_comment_tip

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.