Skip to content

Commit

Permalink
allow defining duplicate indexes based on columns and properties on a…
Browse files Browse the repository at this point in the history 
… table
  • Loading branch information
@deeky666
deeky666 committed Jan 3, 2015
1 parent 89d4f06 commit fc8beca
Show file tree
Hide file tree
Showing 5 changed files with 249 additions and 63 deletions.
27 changes: 23 additions & 4 deletions UPGRADE.md
View file
Original file line number Diff line number Diff line change
@@ -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
issues in the ORM schema tool / DBAL schema manager which pretends users from updating
their schemas and migrate to DBAL 2.5.*.
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
Expand Down
89 changes: 65 additions & 24 deletions lib/Doctrine/DBAL/Schema/Comparator.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();

Expand Down Expand Up @@ -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
Expand Down
28 changes: 14 additions & 14 deletions lib/Doctrine/DBAL/Schema/Table.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down Expand Up @@ -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());
}

Expand Down
53 changes: 53 additions & 0 deletions tests/Doctrine/Tests/DBAL/Schema/ComparatorTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.');
Expand Down
Loading

6 comments on commit fc8beca

@raziel057
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,

Since this commit I see a lot of indexes are duplicated in my application when using the doctrine:schema:update command. For example if I have an entity defined like this:

class Participant
{
    ...

    /**
     * @var File
     *
     * @ORM\OneToOne(targetEntity="PTC\CoreBundle\Entity\File", cascade={"all"})
     * @ORM\JoinColumn(name="photo_id", referencedColumnName="id")
     */
    private $photo;

    ...
}

When I run php app/console doctrine:schema:update --dump-sql I get:

ALTER TABLE participant ADD CONSTRAINT FK_D79F6B117E9E4C8C FOREIGN KEY (photo_id) REFERENCES file (id);
CREATE INDEX IDX_D79F6B117E9E4C8C ON participant (photo_id);
CREATE UNIQUE INDEX UNIQ_D79F6B117E9E4C8C ON participant (photo_id);

Before this commit, just the unique index was created (it seems to be more appropriated):

ALTER TABLE participant ADD CONSTRAINT FK_D79F6B117E9E4C8C FOREIGN KEY (photo_id) REFERENCES file (id);
CREATE UNIQUE INDEX UNIQ_D79F6B117E9E4C8C ON participant (photo_id);

@Ocramius
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a known issue that needs fixing in the ORM. Duplicate index is less dangerous than the previous behavior.

@deeky666
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might have a look into this this evening.

@raziel057
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Ocramius Ok thanks you for your quick answer and the explanations.

@deeky666 Thank you very much for your hard work!

@deeky666
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@raziel057 @Ocramius patch provided in doctrine/orm#1239

@michalbundyra
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This commit caused one more problem with duplicated unique keys as described here: http://www.doctrine-project.org/jira/browse/DDC-3671

Please sign in to comment.