allow defining duplicate indexes based on columns and properties on a…

… table
doctrine · Jan 3, 2015 · ff5e725 · ff5e725
1 parent 89d4f06
commit ff5e725
Show file tree

Hide file tree

Showing 5 changed files with 249 additions and 63 deletions.
diff --git a/UPGRADE.md b/UPGRADE.md
@@ -1,13 +1,32 @@
+# Upgrade to 2.5.1
+
+## MINOR BC BREAK: Doctrine\DBAL\Schema\Table
+
+When adding indexes to ``Doctrine\DBAL\Schema\Table`` via ``addIndex()`` or ``addUniqueIndex()``,
+duplicate indexes are not silently ignored/dropped anymore (based on semantics, not naming!).
+Duplicate indexes are considered indexes that pass ``isFullfilledBy()`` or ``overrules()``
+in ``Doctrine\DBAL\Schema\Index``.
+This is required to make the index renaming feature introduced in 2.5.0 work properly and avoid
+race conditions in the ORM schema tool / DBAL schema manager which pretends users from updating
+their schemas.
+Additionally it offers more flexibility in declaring indexes for the user and potentially fixes
+related issues in the ORM.
+With this change the responsibility to decide which index is a "duplicate" is completely deferred
+to the user.
+Please also note that adding foreign key constraints to a table via ``addForeignKeyConstraint()``,
+``addUnnamedForeignKeyConstraint()`` or ``addNamedForeignKeyConstraint()`` now first checks if an
+appropriate index is already present and avoids adding an additional auto-generated one eventually.
+
 # Upgrade to 2.5
 
 ## BC BREAK: time type resets date fields to UNIX epoch
 
-When mapping `time` type field to PHP's `DateTime` instance all unused date fields are 
-reset to UNIX epoch (i.e. 1970-01-01). This might break any logic which relies on comparing 
-`DateTime` instances with date fields set to the current date. 
+When mapping `time` type field to PHP's `DateTime` instance all unused date fields are
+reset to UNIX epoch (i.e. 1970-01-01). This might break any logic which relies on comparing
+`DateTime` instances with date fields set to the current date.
 
 Use `!` format prefix (see http://php.net/manual/en/datetime.createfromformat.php) for parsing
-time strings to prevent having different date fields when comparing user input and `DateTime` 
+time strings to prevent having different date fields when comparing user input and `DateTime`
 instances as mapped by Doctrine.
 
 ## BC BREAK: Doctrine\DBAL\Schema\Table

diff --git a/lib/Doctrine/DBAL/Schema/Comparator.php b/lib/Doctrine/DBAL/Schema/Comparator.php
@@ -238,37 +238,37 @@ public function diffTable(Table $table1, Table $table2)
         $table1Indexes = $table1->getIndexes();
         $table2Indexes = $table2->getIndexes();
 
-        foreach ($table2Indexes as $index2Name => $index2Definition) {
-            foreach ($table1Indexes as $index1Name => $index1Definition) {
-                if ($this->diffIndex($index1Definition, $index2Definition) === false) {
-                    if ( ! $index1Definition->isPrimary() && $index1Name != $index2Name) {
-                        $tableDifferences->renamedIndexes[$index1Name] = $index2Definition;
-                        $changes++;
-                    }
-
-                    unset($table1Indexes[$index1Name]);
-                    unset($table2Indexes[$index2Name]);
-                } else {
-                    if ($index1Name == $index2Name) {
-                        $tableDifferences->changedIndexes[$index2Name] = $table2Indexes[$index2Name];
-                        unset($table1Indexes[$index1Name]);
-                        unset($table2Indexes[$index2Name]);
-                        $changes++;
-                    }
-                }
+        /* See if all the indexes in table 1 exist in table 2 */
+        foreach ($table2Indexes as $indexName => $index) {
+            if (($index->isPrimary() && $table1->hasPrimaryKey()) || $table1->hasIndex($indexName)) {
+                continue;
             }
-        }
 
-        foreach ($table1Indexes as $index1Name => $index1Definition) {
-            $tableDifferences->removedIndexes[$index1Name] = $index1Definition;
+            $tableDifferences->addedIndexes[$indexName] = $index;
             $changes++;
         }
+        /* See if there are any removed indexes in table 2 */
+        foreach ($table1Indexes as $indexName => $index) {
+            // See if index is removed in table 2.
+            if (($index->isPrimary() && ! $table2->hasPrimaryKey()) ||
+                ! $index->isPrimary() && ! $table2->hasIndex($indexName)
+            ) {
+                $tableDifferences->removedIndexes[$indexName] = $index;
+                $changes++;
+                continue;
+            }
 
-        foreach ($table2Indexes as $index2Name => $index2Definition) {
-            $tableDifferences->addedIndexes[$index2Name] = $index2Definition;
-            $changes++;
+            // See if index has changed in table 2.
+            $table2Index = $index->isPrimary() ? $table2->getPrimaryKey() : $table2->getIndex($indexName);
+
+            if ($this->diffIndex($index, $table2Index)) {
+                $tableDifferences->changedIndexes[$indexName] = $table2Index;
+                $changes++;
+            }
         }
 
+        $this->detectIndexRenamings($tableDifferences);
+
         $fromFkeys = $table1->getForeignKeys();
         $toFkeys = $table2->getForeignKeys();
 
@@ -335,6 +335,47 @@ private function detectColumnRenamings(TableDiff $tableDifferences)
         }
     }
 
+    /**
+     * Try to find indexes that only changed their name, rename operations maybe cheaper than add/drop
+     * however ambiguities between different possibilities should not lead to renaming at all.
+     *
+     * @param \Doctrine\DBAL\Schema\TableDiff $tableDifferences
+     *
+     * @return void
+     */
+    private function detectIndexRenamings(TableDiff $tableDifferences)
+    {
+        $renameCandidates = array();
+
+        // Gather possible rename candidates by comparing each added and removed index based on semantics.
+        foreach ($tableDifferences->addedIndexes as $addedIndexName => $addedIndex) {
+            foreach ($tableDifferences->removedIndexes as $removedIndex) {
+                if (! $this->diffIndex($addedIndex, $removedIndex)) {
+                    $renameCandidates[$addedIndex->getName()][] = array($removedIndex, $addedIndex, $addedIndexName);
+                }
+            }
+        }
+
+        foreach ($renameCandidates as $candidateIndexes) {
+            // If the current rename candidate contains exactly one semantically equal index,
+            // we can safely rename it.
+            // Otherwise it is unclear if a rename action is really intended,
+            // therefore we let those ambiguous indexes be added/dropped.
+            if (count($candidateIndexes) === 1) {
+                list($removedIndex, $addedIndex) = $candidateIndexes[0];
+
+                $removedIndexName = strtolower($removedIndex->getName());
+                $addedIndexName = strtolower($addedIndex->getName());
+
+                if (! isset($tableDifferences->renamedIndexes[$removedIndexName])) {
+                    $tableDifferences->renamedIndexes[$removedIndexName] = $addedIndex;
+                    unset($tableDifferences->addedIndexes[$addedIndexName]);
+                    unset($tableDifferences->removedIndexes[$removedIndexName]);
+                }
+            }
+        }
+    }
+
     /**
      * @param \Doctrine\DBAL\Schema\ForeignKeyConstraint $key1
      * @param \Doctrine\DBAL\Schema\ForeignKeyConstraint $key2

diff --git a/lib/Doctrine/DBAL/Schema/Table.php b/lib/Doctrine/DBAL/Schema/Table.php
@@ -489,27 +489,13 @@ protected function _addColumn(Column $column)
      */
     protected function _addIndex(Index $indexCandidate)
     {
-        // check for duplicates
-        foreach ($this->_indexes as $existingIndex) {
-            if ($indexCandidate->isFullfilledBy($existingIndex)) {
-                return $this;
-            }
-        }
-
         $indexName = $indexCandidate->getName();
         $indexName = $this->normalizeIdentifier($indexName);
 
         if (isset($this->_indexes[$indexName]) || ($this->_primaryKeyName != false && $indexCandidate->isPrimary())) {
             throw SchemaException::indexAlreadyExists($indexName, $this->_name);
         }
 
-        // remove overruled indexes
-        foreach ($this->_indexes as $idxKey => $existingIndex) {
-            if ($indexCandidate->overrules($existingIndex)) {
-                unset($this->_indexes[$idxKey]);
-            }
-        }
-
         if ($indexCandidate->isPrimary()) {
             $this->_primaryKeyName = $indexName;
         }
@@ -538,9 +524,23 @@ protected function _addForeignKeyConstraint(ForeignKeyConstraint $constraint)
         $name = $this->normalizeIdentifier($name);
 
         $this->_fkConstraints[$name] = $constraint;
+
         // add an explicit index on the foreign key columns. If there is already an index that fulfils this requirements drop the request.
         // In the case of __construct calling this method during hydration from schema-details all the explicitly added indexes
         // lead to duplicates. This creates computation overhead in this case, however no duplicate indexes are ever added (based on columns).
+        $indexName = $this->_generateIdentifierName(
+            array_merge(array($this->getName()), $constraint->getColumns()),
+            "idx",
+            $this->_getMaxIdentifierLength()
+        );
+        $indexCandidate = $this->_createIndex($constraint->getColumns(), $indexName, false, false);
+
+        foreach ($this->_indexes as $existingIndex) {
+            if ($indexCandidate->isFullfilledBy($existingIndex)) {
+                return;
+            }
+        }
+
         $this->addIndex($constraint->getColumns());
     }
 

diff --git a/tests/Doctrine/Tests/DBAL/Schema/ComparatorTest.php b/tests/Doctrine/Tests/DBAL/Schema/ComparatorTest.php
@@ -708,6 +708,59 @@ public function testDetectRenameColumnAmbiguous()
         $this->assertEquals(0, count($tableDiff->renamedColumns), "no renamings should take place.");
     }
 
+    /**
+     * @group DBAL-1063
+     */
+    public function testDetectRenameIndex()
+    {
+        $table1 = new Table('foo');
+        $table1->addColumn('foo', 'integer');
+
+        $table2 = clone $table1;
+
+        $table1->addIndex(array('foo'), 'idx_foo');
+
+        $table2->addIndex(array('foo'), 'idx_bar');
+
+        $comparator = new Comparator();
+        $tableDiff = $comparator->diffTable($table1, $table2);
+
+        $this->assertCount(0, $tableDiff->addedIndexes);
+        $this->assertCount(0, $tableDiff->removedIndexes);
+        $this->assertArrayHasKey('idx_foo', $tableDiff->renamedIndexes);
+        $this->assertEquals('idx_bar', $tableDiff->renamedIndexes['idx_foo']->getName());
+    }
+
+    /**
+     * You can easily have ambiguities in the index renaming. If these
+     * are detected no renaming should take place, instead adding and dropping
+     * should be used exclusively.
+     *
+     * @group DBAL-1063
+     */
+    public function testDetectRenameIndexAmbiguous()
+    {
+        $table1 = new Table('foo');
+        $table1->addColumn('foo', 'integer');
+
+        $table2 = clone $table1;
+
+        $table1->addIndex(array('foo'), 'idx_foo');
+        $table1->addIndex(array('foo'), 'idx_bar');
+
+        $table2->addIndex(array('foo'), 'idx_baz');
+
+        $comparator = new Comparator();
+        $tableDiff = $comparator->diffTable($table1, $table2);
+
+        $this->assertCount(1, $tableDiff->addedIndexes);
+        $this->assertArrayHasKey('idx_baz', $tableDiff->addedIndexes);
+        $this->assertCount(2, $tableDiff->removedIndexes);
+        $this->assertArrayHasKey('idx_foo', $tableDiff->removedIndexes);
+        $this->assertArrayHasKey('idx_bar', $tableDiff->removedIndexes);
+        $this->assertCount(0, $tableDiff->renamedIndexes);
+    }
+
     public function testDetectChangeIdentifierType()
     {
         $this->markTestSkipped('DBAL-2 was reopened, this test cannot work anymore.');