Add a safeguard against multiple objects competing for the same ident…

…ity map entry

While trying to understand #3037, I found that it may happen that we have more entries in `\Doctrine\ORM\UnitOfWork::$entityIdentifiers` than in `\Doctrine\ORM\UnitOfWork::$identityMap`.

The former is a mapping from `spl_object_id` values to ID hashes, the latter an array first of entity class names and then from ID hash to entity object instances.

(Basically, "ID hash" is a concatenation of all field values making up the `@Id` for a given entity.)

This means that at some point, we must have _different_ objects representing the same entity, or at least over time different objects are used for the same entity without the UoW properly updating its `::$entityIdentifiers` structure.

I don't think it makes sense to overwrite an entity in the identity map, since that means a currently `MANAGED` entity is replaced with something else.

If it makes sense at all to _replace_ an entity, that should happen through dedicated management methods to first detach the old entity before persisting, merging or otherwise adding the new one. This way we could make sure the internal structures remain consistent.

lib/Doctrine/ORM/UnitOfWork.php

@@ -1591,6 +1591,30 @@ public function addToIdentityMap($entity)
         $className     = $classMetadata->rootEntityName;
 
         if (isset($this->identityMap[$className][$idHash])) {
+            if ($this->identityMap[$className][$idHash] !== $entity) {
+                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
+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. 
+EXCEPTION
+                    ,
+                    get_class($entity),
+                    $idHash,
+                    get_class($this->identityMap[$className][$idHash])
+                ));
+            }
+
             return false;
         }
 
@@ -2811,25 +2835,20 @@ public function createEntity($className, array $data, &$hints = [])
             }
 
             $this->originalEntityData[$oid] = $data;
+
+            if ($entity instanceof NotifyPropertyChanged) {
+                $entity->addPropertyChangedListener($this);
+            }
         } else {
             $entity = $this->newInstance($class);
             $oid    = spl_object_id($entity);
-
-            $this->entityIdentifiers[$oid]  = $id;
-            $this->entityStates[$oid]       = self::STATE_MANAGED;
-            $this->originalEntityData[$oid] = $data;
-
-            $this->identityMap[$class->rootEntityName][$idHash] = $entity;
+            $this->registerManaged($entity, $id, $data);
 
             if (isset($hints[Query::HINT_READ_ONLY])) {
                 $this->readOnlyObjects[$oid] = true;
             }
         }
 
-        if ($entity instanceof NotifyPropertyChanged) {
-            $entity->addPropertyChangedListener($this);
-        }
-
         foreach ($data as $field => $value) {
             if (isset($class->fieldMappings[$field])) {
                 $class->reflFields[$field]->setValue($entity, $value);
@@ -2987,20 +3006,7 @@ public function createEntity($className, array $data, &$hints = [])
                                 break;
                             }
 
-                            // PERF: Inlined & optimized code from UnitOfWork#registerManaged()
-                            $newValueOid                                                     = spl_object_id($newValue);
-                            $this->entityIdentifiers[$newValueOid]                           = $associatedId;
-                            $this->identityMap[$targetClass->rootEntityName][$relatedIdHash] = $newValue;
-
-                            if (
-                                $newValue instanceof NotifyPropertyChanged &&
-                                ( ! $newValue instanceof Proxy || $newValue->__isInitialized())
-                            ) {
-                                $newValue->addPropertyChangedListener($this);
-                            }
-
-                            $this->entityStates[$newValueOid] = self::STATE_MANAGED;
-                            // make sure that when an proxy is then finally loaded, $this->originalEntityData is set also!
+                            $this->registerManaged($newValue, $associatedId, []);
                             break;
                     }
 

tests/Doctrine/Tests/ORM/Functional/BasicFunctionalTest.php

@@ -19,6 +19,7 @@
 use Doctrine\Tests\Models\CMS\CmsUser;
 use Doctrine\Tests\OrmFunctionalTestCase;
 use InvalidArgumentException;
+use RuntimeException;
 
 use function get_class;
 
@@ -1291,4 +1292,33 @@ public function testWrongAssociationInstance(): void
 
         $this->_em->flush();
     }
+
+    public function testItThrowsWhenReferenceUsesIdAssignedByDatabase(): void
+    {
+        $user           = new CmsUser();
+        $user->name     = 'test';
+        $user->username = 'test';
+        $this->_em->persist($user);
+        $this->_em->flush();
+
+        // Obtain a reference object for the next ID. This is a user error - references
+        // should be fetched only for existing IDs
+        $ref = $this->_em->getReference(CmsUser::class, $user->id + 1);
+
+        $user2           = new CmsUser();
+        $user2->name     = 'test2';
+        $user2->username = 'test2';
+
+        // Now the database will assign an ID to the $user2 entity, but that place
+        // in the identity map is already taken by user error.
+        $this->expectException(RuntimeException::class);
+        $this->expectExceptionMessageMatches('/another object .* was already present for the same ID/');
+
+        // depending on ID generation strategy, the ID may be asssigned already here
+        // and the entity be put in the identity map
+        $this->_em->persist($user2);
+
+        // post insert IDs will be assigned during flush
+        $this->_em->flush();
+    }
 }

tests/Doctrine/Tests/ORM/Functional/ProxiesLikeEntitiesTest.php

@@ -95,7 +95,7 @@ public function testEntityWithIdentifier(): void
      */
     public function testProxyAsDqlParameterPersist(): void
     {
-        $proxy     = $this->_em->getProxyFactory()->getProxy(CmsUser::class, ['id' => $this->user->getId()]);
+        $proxy     = $this->_em->getReference(CmsUser::class, ['id' => $this->user->getId()]);
         $proxy->id = $this->user->getId();
         $result    = $this
             ->_em

tests/Doctrine/Tests/ORM/Functional/Ticket/DDC1238Test.php

@@ -56,7 +56,7 @@ public function testIssueProxyClear(): void
 
         $user2 = $this->_em->getReference(DDC1238User::class, $userId);
 
-        $user->__load();
+        //$user->__load();
 
         self::assertIsInt($user->getId(), 'Even if a proxy is detached, it should still have an identifier');
 

tests/Doctrine/Tests/ORM/Functional/Ticket/GH7869Test.php

@@ -55,7 +55,7 @@ public function getClassMetadata($className): ClassMetadata
         $uow->clear();
         $uow->triggerEagerLoads();
 
-        self::assertSame(2, $em->getClassMetadataCalls);
+        self::assertSame(4, $em->getClassMetadataCalls);
     }
 }
 

tests/Doctrine/Tests/ORM/UnitOfWorkTest.php

@@ -41,6 +41,7 @@
 use Doctrine\Tests\OrmTestCase;
 use Doctrine\Tests\PHPUnitCompatibility\MockBuilderCompatibilityTools;
 use PHPUnit\Framework\MockObject\MockObject;
+use RuntimeException;
 use stdClass;
 
 use function assert;
@@ -108,6 +109,8 @@ protected function setUp(): void
         $driverConnection = $this->createMock(Driver\Connection::class);
         $driverConnection->method('prepare')
             ->willReturn($driverStatement);
+        $driverConnection->method('lastInsertId')
+            ->willReturnOnConsecutiveCalls(1, 2, 3, 4, 5, 6);
 
         $driver = $this->createMock(Driver::class);
         $driver->method('getDatabasePlatform')
@@ -923,6 +926,25 @@ public function testRemovedEntityIsRemovedFromOneToManyCollection(): void
         self::assertFalse($user->phonenumbers->isDirty());
         self::assertEmpty($user->phonenumbers->getSnapshot());
     }
+
+    public function testItThrowsWhenApplicationProvidedIdsCollide(): 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->expectException(RuntimeException::class);
+        $this->expectExceptionMessageMatches('/another object .* was already present for the same ID/');
+
+        $this->_unitOfWork->persist($phone2);
+    }
 }
 
 /** @Entity */