Skip to content

Commit

Permalink
Turn identity map collisions from exception to deprecation notice
Browse files Browse the repository at this point in the history
In #10785, a check was added that prevents entity instances from getting into the identity map when another object for the same ID is already being tracked.

This caused regressions for users that work with application-provided IDs and expect this condition to fail with `UniqueConstraintViolationExceptions` when flushing to the database.

Thus, this PR turns the exception into a deprecation notice. Users can opt-in to the new behavior. In 3.0, the exception will be used.

Implements #10871.
  • Loading branch information
mpdude committed Aug 4, 2023
1 parent fd0bdc6 commit 588a200
Show file tree
Hide file tree
Showing 5 changed files with 86 additions and 7 deletions.
14 changes: 14 additions & 0 deletions UPGRADE.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,19 @@
# Upgrade to 2.16

## Deprecated accepting duplicate IDs in the identity map

For any given entity class and ID value, there should be only one object instance
representing the entity.

In https://github.com/doctrine/orm/pull/10785, a check was added that will guard this
in the identity map. The most probable cause for violations of this rule are collisions
of application-provided IDs.

In ORM 2.16.0, the check was added by throwing an exception. In ORM 2.16.1, this will be
changed to a deprecation notice. ORM 3.0 will make it an exception again. Use
`\Doctrine\ORM\Configuration::setRejectIdCollisionInIdentityMap()` if you want to opt-in
to the new mode.

## Potential changes to the order in which `INSERT`s are executed

In https://github.com/doctrine/orm/pull/10547, the commit order computation was improved
Expand Down
10 changes: 10 additions & 0 deletions lib/Doctrine/ORM/Configuration.php
Original file line number Diff line number Diff line change
Expand Up @@ -1117,4 +1117,14 @@ public function setLazyGhostObjectEnabled(bool $flag): void

$this->_attributes['isLazyGhostObjectEnabled'] = $flag;
}

public function setRejectIdCollisionInIdentityMap(bool $flag): void
{
$this->_attributes['rejectIdCollisionInIdentityMap'] = $flag;
}

public function isRejectIdCollisionInIdentityMapEnabled(): bool
{
return $this->_attributes['rejectIdCollisionInIdentityMap'] ?? false;
}
}
47 changes: 40 additions & 7 deletions lib/Doctrine/ORM/UnitOfWork.php
Original file line number Diff line number Diff line change
Expand Up @@ -1635,8 +1635,10 @@ public function addToIdentityMap($entity)

if (isset($this->identityMap[$className][$idHash])) {
if ($this->identityMap[$className][$idHash] !== $entity) {
throw new RuntimeException(sprintf(
<<<'EXCEPTION'
if ($this->em->getConfiguration()->isRejectIdCollisionInIdentityMapEnabled()) {
throw new RuntimeException(
sprintf(
<<<'EXCEPTION'
While adding an entity of class %s with an ID hash of "%s" to the identity map,
another object of class %s was already present for the same ID. This exception
is a safeguard against an internal inconsistency - IDs should uniquely map to
Expand All @@ -1651,11 +1653,42 @@ public function addToIdentityMap($entity)
Otherwise, it might be an ORM-internal inconsistency, please report it.
EXCEPTION
,
get_class($entity),
$idHash,
get_class($this->identityMap[$className][$idHash])
));
,
get_class($entity),
$idHash,
get_class($this->identityMap[$className][$idHash])
)
);
} else {
Deprecation::trigger(
'doctrine/orm',
'https://github.com/doctrine/orm/pull/10785',
<<<'EXCEPTION'
While adding an entity of class %s with an ID hash of "%s" to the identity map,
another object of class %s was already present for the same ID. This will trigger
an exception in ORM 3.0.
IDs should uniquely map to entity object instances. This problem may occur if:
- you use application-provided IDs and reuse ID values;
- database-provided IDs are reassigned after truncating the database without
clearing the EntityManager;
- you might have been using EntityManager#getReference() to create a reference
for a nonexistent ID that was subsequently (by the RDBMS) assigned to another
entity.
Otherwise, it might be an ORM-internal inconsistency, please report it.
To opt-in to the new exception, call
\Doctrine\ORM\Configuration::setRejectIdCollisionInIdentityMap on the entity
manager's configuration.
EXCEPTION
,
get_class($entity),
$idHash,
get_class($this->identityMap[$className][$idHash])
);
}
}

return false;
Expand Down
2 changes: 2 additions & 0 deletions tests/Doctrine/Tests/ORM/Functional/BasicFunctionalTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -1329,6 +1329,8 @@ public function testWrongAssociationInstance(): void

public function testItThrowsWhenReferenceUsesIdAssignedByDatabase(): void
{
$this->_em->getConfiguration()->setRejectIdCollisionInIdentityMap(true);

$user = new CmsUser();
$user->name = 'test';
$user->username = 'test';
Expand Down
20 changes: 20 additions & 0 deletions tests/Doctrine/Tests/ORM/UnitOfWorkTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -927,8 +927,28 @@ public function testRemovedEntityIsRemovedFromOneToManyCollection(): void
self::assertEmpty($user->phonenumbers->getSnapshot());
}

public function testItTriggersADeprecationNoticeWhenApplicationProvidedIdsCollide(): void
{
// We're using application-provided IDs and assign the same ID twice
// Note this is about colliding IDs in the identity map in memory.
// Duplicate database-level IDs would be spotted when the EM is flushed.

$phone1 = new CmsPhonenumber();
$phone1->phonenumber = '1234';
$this->_unitOfWork->persist($phone1);

$phone2 = new CmsPhonenumber();
$phone2->phonenumber = '1234';

$this->expectDeprecationWithIdentifier('https://github.com/doctrine/orm/pull/10785');

$this->_unitOfWork->persist($phone2);
}

public function testItThrowsWhenApplicationProvidedIdsCollide(): void
{
$this->_emMock->getConfiguration()->setRejectIdCollisionInIdentityMap(true);

// We're using application-provided IDs and assign the same ID twice
// Note this is about colliding IDs in the identity map in memory.
// Duplicate database-level IDs would be spotted when the EM is flushed.
Expand Down

0 comments on commit 588a200

Please sign in to comment.