DDC-3671 prevent duplicate unique index #1375
Conversation
|
Hello, thank you for creating this pull request. I have automatically opened an issue http://www.doctrine-project.org/jira/browse/DDC-3677 We use Jira to track the state of pull requests and the versions they got |
| foreach ($table->getIndexes() as $tableIndexName => $tableIndex) { | ||
| if ($tableIndex->isFullfilledBy($uniqIndex)) { | ||
| $table->dropIndex($tableIndexName); | ||
| break; |
There was a problem hiding this comment.
I assumed if one fulfilled index will be found it has no sense to look for another one. Can be removed, but is it possible to have more than one fulfilled index in table before? Can you provide an example?
I was thinking to add condition to check if found index is primary, than we shouldn't add unique index, but it will be only possible when annotation is invalid - so when we have uniqueConstraint at primary column.
There was a problem hiding this comment.
@webimpress nothing denies adding more than one unique constraint for the same column, so the break is probably not needed
There was a problem hiding this comment.
@Ocramius, yes, nothing denies adding more than one unique constraint for the same column in (DBAL) Table class (actually the commit that made it possible caused that issue), but that method creates table from metadata and adding a unique index for a table is possible when one of the conditions is met i.e.:
- column has annotation
unique=true; - column has relation
OneToOne; - entity has annotation
uniqueConstraint.
Therefore in modified method getSchemaFromMetadata table can have at most one fulfilled index in this loop, then I can't see any reason to continue search for another one.
I can remove it, but I'm worried that it could partially cover another bug, which should be resolved in another part. If you can provide an example to write one more unit test to prove that this break causes an issue I'll be glad and all will be clear when we have one more unit test for that case.
There was a problem hiding this comment.
IMO the preference should be uniqueConstraint (explicit) > unique=true (implicit) if and only if uniqueConstraint spans ONLY the unique=true column (same behaviour as for uniqueConstraint (explicit) > foreignKey index (implicit).
Not sure about OneToOne but it should be similar I guess.
So what about simply leaving this block as it was and move it to the top of the classes loop to line 162. Then it will first add explicit unique constraints before adding implicit unique indexes from the columns gathering. gatherColumn() should then ask the table if a fullfilling unique index is already available and skip addition eventually.
There was a problem hiding this comment.
@deeky666 It makes sense, but gatherColumn()1 is not the only one place where unique index can be added.
It could be also added in method gatherRelationJoinColumns()2 (for relation OneToOne).
About foreign key... It creates index key (not unique index) implicit ONLY when is needed, so when another fulfilling index doesn't exist3. Therefore when relation is OneToOne unique index is created2 and then on creating foreign key normal index is not added3.
My solution fits better, because indexes from uniqueConstraint are the most important (explicit) so always replace other (fulfilled) implicit indexes. (Even we have relation ManyToOne for a column and uniqueConstraint is defined on the column, index key created with foreign key will be replaced by unique index from uniqueConstraint - of course, in that case relation should be OneToOne and can be defined uniqueConstraint to set name for the unique index).
Finally, your proposition will be also good, but IMO more complicated, because calls 1 or 2 should be replaced by new method which will decide if unique index should be added or not.
After all, break; can be taken out but, in my loop table can have at most one fulfilled index (implicit) (added due column annotation 1 or 2).
1 Add unique index for column with annotation unique=true (method gatherColumn())
2 Add unique index for relation OneToOne (method gatherRelationJoinColumns())
3 Add index key with foreign key only when needed
|
@deeky666 could you give me your opinion for this? I don't see anything else required :) |
DDC-3671 prevent duplicate unique index
Backported #1375 to 2.5
|
Backported by @guilhermeblanco into |
|
Thanks for the PR! =) |
No description provided.