Skip to content

Commit

Permalink
prevent duplicate unique index
Browse files Browse the repository at this point in the history
  • Loading branch information
Michał Bundyra committed Apr 8, 2015
1 parent 2c90930 commit 699a6e1
Show file tree
Hide file tree
Showing 2 changed files with 49 additions and 0 deletions.
10 changes: 10 additions & 0 deletions lib/Doctrine/ORM/Tools/SchemaTool.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@

use Doctrine\ORM\ORMException;
use Doctrine\DBAL\Schema\Comparator;
use Doctrine\DBAL\Schema\Index;
use Doctrine\DBAL\Schema\Schema;
use Doctrine\DBAL\Schema\Table;
use Doctrine\DBAL\Schema\Visitor\DropSchemaSqlCollector;
Expand Down Expand Up @@ -274,6 +275,15 @@ public function getSchemaFromMetadata(array $classes)

if (isset($class->table['uniqueConstraints'])) {
foreach ($class->table['uniqueConstraints'] as $indexName => $indexData) {
$uniqIndex = new Index($indexName, $indexData['columns'], true, false, [], isset($indexData['options']) ? $indexData['options'] : []);

foreach ($table->getIndexes() as $tableIndexName => $tableIndex) {
if ($tableIndex->isFullfilledBy($uniqIndex)) {
$table->dropIndex($tableIndexName);
break;
}
}

$table->addUniqueIndex($indexData['columns'], is_numeric($indexName) ? null : $indexName, isset($indexData['options']) ? $indexData['options'] : array());
}
}
Expand Down
39 changes: 39 additions & 0 deletions tests/Doctrine/Tests/ORM/Tools/SchemaToolTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,28 @@ public function testNullDefaultNotAddedToCustomSchemaOptions()

$this->assertSame(array(), $customSchemaOptions);
}

/**
* @group DDC-3671
*/
public function testSchemaHasProperIndexesFromUniqueConstraintAnnotation()
{
$em = $this->_getTestEntityManager();
$schemaTool = new SchemaTool($em);

$classes = [
$em->getClassMetadata(__NAMESPACE__ . '\\UniqueConstraintAnnotationModel'),
];

$schema = $schemaTool->getSchemaFromMetadata($classes);

$this->assertTrue($schema->hasTable('unique_constraint_annotation_table'));
$table = $schema->getTable('unique_constraint_annotation_table');

$this->assertEquals(2, count($table->getIndexes()));
$this->assertTrue($table->hasIndex('primary'));
$this->assertTrue($table->hasIndex('uniq_hash'));
}
}

/**
Expand Down Expand Up @@ -148,3 +170,20 @@ public function postGenerateSchema(GenerateSchemaEventArgs $eventArgs)
$this->schemaCalled = true;
}
}

/**
* @Entity
* @Table(name="unique_constraint_annotation_table", uniqueConstraints={
* @UniqueConstraint(name="uniq_hash", columns={"hash"})
* })
*/
class UniqueConstraintAnnotationModel
{
/** @Id @Column */
private $id;

/**
* @Column(name="hash", type="string", length=8, nullable=false, unique=true)

This comment has been minimized.

Sign in to view

Copy link
@deeky666

deeky666 Jan 6, 2016

Member

This mapping really makes no sense to me. You are explicitly defining 2 unique indexes here. One via table's UniqueConstraint annotation and one via column's unique attribute.
IMO one should define one of them but not both. Silently dropping the duplicate index then has evil side effects. See doctrine/dbal#2248
I am still of the opinion that Doctrine should be as transparent as possible about index creation and only do "magic" where completely necessary. In the end it's the user's responsibility to take care of definitions and without Doctrine putting a lot of magic in there, it's also much better comprehensible what the mappings are actually doing.
IMO this commit has to be reverted. /cc @Ocramius @zeroedin-bill

This comment has been minimized.

Sign in to view

Copy link
@Ocramius

Ocramius Jan 6, 2016

Member

@deeky666 do you have a case where a duplicate unique index being de-duplicated may cause an issue? That said, I agree: we need some sort of clear path on how to deal with index duplication (exception being a perfectly valid solution as well)

This comment has been minimized.

Sign in to view

Copy link
@deeky666

deeky666 Jan 6, 2016

Member

@Ocramius yes have a look at the example in the referenced issue doctrine/dbal#2248. The fulltext non-unique index is dropped in favour of the unique constraint defined (as the constraints are fullfilled according to DBAL: Index::isFullfilledBy()). However IMO this method should not be used in such scenarios anymore, as it's purpose is solely to tell whether two indexes COULD be "the same" and one of them COULD be dropped silently. The example in the issue shows it perfectly. If you evaluate the physical constraints, the regular fulltext index can easily be discarded in favour of the unique index. If you evaluate other semantics like fulltext searchability in this case, the regular fulltext index clearly has the right to live on its own. The point really is, that Doctrine cannot and should not do those kinds of evaluations for the user as Doctrine cannot know the reasoning behind multiple similar index definitions... Just 2 my 0.02$

This comment has been minimized.

Sign in to view

Copy link
@Ocramius

Ocramius Jan 6, 2016

Member

The fulltext non-unique index is dropped

Ah yes, that one I was aware of.

This comment has been minimized.

Sign in to view

Copy link
@Ocramius

Ocramius Jan 6, 2016

Member

cannot

Exceptions then. We should think about how to do that, but can't do it in the 2.x series.
*/
private $hash;
}

0 comments on commit 699a6e1

Please sign in to comment.