From ff5e7254cf2ceca1e96595c9db5469a21113d1f2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Steve=20M=C3=BCller?= Date: Sat, 3 Jan 2015 19:00:53 +0100 Subject: [PATCH] allow defining duplicate indexes based on columns and properties on a table --- UPGRADE.md | 27 +++- lib/Doctrine/DBAL/Schema/Comparator.php | 89 ++++++++++---- lib/Doctrine/DBAL/Schema/Table.php | 28 ++--- .../Tests/DBAL/Schema/ComparatorTest.php | 53 ++++++++ .../Doctrine/Tests/DBAL/Schema/TableTest.php | 115 ++++++++++++++---- 5 files changed, 249 insertions(+), 63 deletions(-) diff --git a/UPGRADE.md b/UPGRADE.md index aad5f102065..f1448b9232e 100644 --- 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 index 91c8c28f2da..4f11e6c8dfe 100644 --- 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 index e5d98e1ab77..69109d21d5c 100644 --- a/lib/Doctrine/DBAL/Schema/Table.php +++ b/lib/Doctrine/DBAL/Schema/Table.php @@ -489,13 +489,6 @@ 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); @@ -503,13 +496,6 @@ protected function _addIndex(Index $indexCandidate) 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 index c028bb10d58..8cd3205a59a 100644 --- 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.'); diff --git a/tests/Doctrine/Tests/DBAL/Schema/TableTest.php b/tests/Doctrine/Tests/DBAL/Schema/TableTest.php index 2a97ab5799f..66b2cef96c0 100644 --- a/tests/Doctrine/Tests/DBAL/Schema/TableTest.php +++ b/tests/Doctrine/Tests/DBAL/Schema/TableTest.php @@ -351,19 +351,6 @@ public function testAllowImplicitSchemaTableInAutogeneratedIndexNames() $this->assertEquals(1, count($table->getIndexes())); } - /** - * @group DBAL-50 - */ - public function testAddIndexTwice_IgnoreSecond() - { - $table = new Table("foo.bar"); - $table->addColumn('baz', 'integer', array()); - $table->addIndex(array('baz')); - $table->addIndex(array('baz')); - - $this->assertEquals(1, count($table->getIndexes())); - } - /** * @group DBAL-50 */ @@ -385,10 +372,57 @@ public function testAddForeignKeyIndexImplicitly() $this->assertEquals(array('id'), $index->getColumns()); } + /** + * @group DBAL-1063 + */ + public function testAddForeignKeyDoesNotCreateDuplicateIndex() + { + $table = new Table('foo'); + $table->addColumn('bar', 'integer'); + $table->addIndex(array('bar'), 'bar_idx'); + + $foreignTable = new Table('bar'); + $foreignTable->addColumn('foo', 'integer'); + + $table->addForeignKeyConstraint($foreignTable, array('bar'), array('foo')); + + $this->assertCount(1, $table->getIndexes()); + $this->assertTrue($table->hasIndex('bar_idx')); + $this->assertSame(array('bar'), $table->getIndex('bar_idx')->getColumns()); + } + + /** + * @group DBAL-1063 + */ + public function testAddForeignKeyAddsImplicitIndexIfIndexColumnsDoNotSpan() + { + $table = new Table('foo'); + $table->addColumn('bar', 'integer'); + $table->addColumn('baz', 'string'); + $table->addColumn('bloo', 'string'); + $table->addIndex(array('baz', 'bar'), 'composite_idx'); + $table->addIndex(array('bar', 'baz', 'bloo'), 'full_idx'); + + $foreignTable = new Table('bar'); + $foreignTable->addColumn('foo', 'integer'); + $foreignTable->addColumn('baz', 'string'); + + $table->addForeignKeyConstraint($foreignTable, array('bar', 'baz'), array('foo', 'baz')); + + $this->assertCount(3, $table->getIndexes()); + $this->assertTrue($table->hasIndex('composite_idx')); + $this->assertTrue($table->hasIndex('full_idx')); + $this->assertTrue($table->hasIndex('idx_8c73652176ff8caa78240498')); + $this->assertSame(array('baz', 'bar'), $table->getIndex('composite_idx')->getColumns()); + $this->assertSame(array('bar', 'baz', 'bloo'), $table->getIndex('full_idx')->getColumns()); + $this->assertSame(array('bar', 'baz'), $table->getIndex('idx_8c73652176ff8caa78240498')->getColumns()); + } + /** * @group DBAL-50 + * @group DBAL-1063 */ - public function testOverruleIndex() + public function testOverrulingIndexDoesNotDropOverruledIndex() { $table = new Table("bar"); $table->addColumn('baz', 'integer', array()); @@ -399,23 +433,62 @@ public function testOverruleIndex() $index = current($indexes); $table->addUniqueIndex(array('baz')); - $this->assertEquals(1, count($table->getIndexes())); - $this->assertFalse($table->hasIndex($index->getName())); + $this->assertEquals(2, count($table->getIndexes())); + $this->assertTrue($table->hasIndex($index->getName())); + } + + /** + * @group DBAL-1063 + */ + public function testAllowsAddingDuplicateIndexesBasedOnColumns() + { + $table = new Table('foo'); + $table->addColumn('bar', 'integer'); + $table->addIndex(array('bar'), 'bar_idx'); + $table->addIndex(array('bar'), 'duplicate_idx'); + + $this->assertCount(2, $table->getIndexes()); + $this->assertTrue($table->hasIndex('bar_idx')); + $this->assertTrue($table->hasIndex('duplicate_idx')); + $this->assertSame(array('bar'), $table->getIndex('bar_idx')->getColumns()); + $this->assertSame(array('bar'), $table->getIndex('duplicate_idx')->getColumns()); } - public function testPrimaryKeyOverrulesUniqueIndex() + /** + * @group DBAL-1063 + */ + public function testAllowsAddingFulfillingIndexesBasedOnColumns() + { + $table = new Table('foo'); + $table->addColumn('bar', 'integer'); + $table->addColumn('baz', 'string'); + $table->addIndex(array('bar'), 'bar_idx'); + $table->addIndex(array('bar', 'baz'), 'fulfilling_idx'); + + $this->assertCount(2, $table->getIndexes()); + $this->assertTrue($table->hasIndex('bar_idx')); + $this->assertTrue($table->hasIndex('fulfilling_idx')); + $this->assertSame(array('bar'), $table->getIndex('bar_idx')->getColumns()); + $this->assertSame(array('bar', 'baz'), $table->getIndex('fulfilling_idx')->getColumns()); + } + + /** + * @group DBAL-50 + * @group DBAL-1063 + */ + public function testPrimaryKeyOverrulingUniqueIndexDoesNotDropUniqueIndex() { $table = new Table("bar"); $table->addColumn('baz', 'integer', array()); - $table->addUniqueIndex(array('baz')); + $table->addUniqueIndex(array('baz'), 'idx_unique'); $table->setPrimaryKey(array('baz')); $indexes = $table->getIndexes(); - $this->assertEquals(1, count($indexes), "Table should only contain the primary key table index, not the unique one anymore, because it was overruled."); + $this->assertEquals(2, count($indexes), "Table should only contain both the primary key table index and the unique one, even though it was overruled."); - $index = current($indexes); - $this->assertTrue($index->isPrimary()); + $this->assertTrue($table->hasPrimaryKey()); + $this->assertTrue($table->hasIndex('idx_unique')); } /**