From d738ecfcfef047e429a2d2b7fac943c695e81d5f Mon Sep 17 00:00:00 2001 From: Matthias Pigulla Date: Thu, 22 Jun 2023 16:52:36 +0200 Subject: [PATCH 1/2] Avoid creating unmanaged proxy instances for referred-to entities during merge() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This PR tries to improve the situation/problem explained in #3037: Under certain conditions – there may be multiple and not all are known/well-understood – we may get inconsistencies between the `\Doctrine\ORM\UnitOfWork::$entityIdentifiers` and `\Doctrine\ORM\UnitOfWork::$identityMap` arrays. Since the `::$identityMap` is a plain array holding object references, objects contained in it cannot be garbage-collected. `::$entityIdentifiers`, however, is indexed by `spl_object_id` values. When those objects are destructed and/or garbage-collected, the OID may be reused and reassigned to other objects later on. When the OID re-assignment happens to be for another entity, the UoW may assume incorrect entity states and, for example, miss INSERT or UPDATE operations. One cause for such inconsistencies is _replacing_ identity map entries with other object instances: This makes it possible that the old object becomes GC'd, while its OID is not cleaned up. Since that is not a use case we need to support (IMHO), #10785 is about adding a safeguard against it. In this test shown here, the `merge()` operation is currently too eager in creating a proxy object for another referred-to entity. This proxy represents an entity already present in the identity map at that time, potentially leading to this problem later on. --- lib/Doctrine/ORM/UnitOfWork.php | 20 +++-- .../ORM/Functional/Ticket/GH7407Test.php | 87 +++++++++++++++++++ 2 files changed, 99 insertions(+), 8 deletions(-) create mode 100644 tests/Doctrine/Tests/ORM/Functional/Ticket/GH7407Test.php diff --git a/lib/Doctrine/ORM/UnitOfWork.php b/lib/Doctrine/ORM/UnitOfWork.php index ebb5f12a01b..cc1c5ab22bc 100644 --- a/lib/Doctrine/ORM/UnitOfWork.php +++ b/lib/Doctrine/ORM/UnitOfWork.php @@ -3669,14 +3669,18 @@ private function mergeEntityStateIntoManagedCopy($entity, $managedCopy): void $targetClass = $this->em->getClassMetadata($assoc2['targetEntity']); $relatedId = $targetClass->getIdentifierValues($other); - if ($targetClass->subClasses) { - $other = $this->em->find($targetClass->name, $relatedId); - } else { - $other = $this->em->getProxyFactory()->getProxy( - $assoc2['targetEntity'], - $relatedId - ); - $this->registerManaged($other, $relatedId, []); + $other = $this->tryGetById($relatedId, $targetClass->name); + + if (! $other) { + if ($targetClass->subClasses) { + $other = $this->em->find($targetClass->name, $relatedId); + } else { + $other = $this->em->getProxyFactory()->getProxy( + $assoc2['targetEntity'], + $relatedId + ); + $this->registerManaged($other, $relatedId, []); + } } } diff --git a/tests/Doctrine/Tests/ORM/Functional/Ticket/GH7407Test.php b/tests/Doctrine/Tests/ORM/Functional/Ticket/GH7407Test.php new file mode 100644 index 00000000000..3e010e177cb --- /dev/null +++ b/tests/Doctrine/Tests/ORM/Functional/Ticket/GH7407Test.php @@ -0,0 +1,87 @@ +useModelSet('cms'); + + parent::setUp(); + } + + public function testMergingEntitiesDoesNotCreateUnmanagedProxyReferences(): void + { + // 1. Create an article with a user; persist, flush and clear the entity manager + $user = new CmsUser(); + $user->username = 'Test'; + $user->name = 'Test'; + $this->_em->persist($user); + + $article = new CmsArticle(); + $article->topic = 'Test'; + $article->text = 'Test'; + $article->setAuthor($user); + $this->_em->persist($article); + + $this->_em->flush(); + $this->_em->clear(); + + // 2. Merge the user object back in: + // We get a new (different) entity object that represents the user instance + // which is now (through this object instance) managed by the EM/UoW + $mergedUser = $this->_em->merge($user); + $mergedUserOid = spl_object_id($mergedUser); + + // 3. Merge the article object back in, + // the returned entity object is the article instance as it is managed by the EM/UoW + $mergedArticle = $this->_em->merge($article); + $mergedArticleOid = spl_object_id($mergedArticle); + + self::assertSame($mergedUser, $mergedArticle->user, 'The $mergedArticle\'s #user property should hold the $mergedUser we obtained previously, since that\'s the only legitimate object instance representing the user from the UoW\'s point of view.'); + + // Inspect internal UoW state + $uow = $this->_em->getUnitOfWork(); + $entityIdentifiers = $this->grabProperty('entityIdentifiers', $uow); + $identityMap = $this->grabProperty('identityMap', $uow); + $entityStates = $this->grabProperty('entityStates', $uow); + + self::assertCount(2, $entityIdentifiers, 'UoW#entityIdentifiers contains exactly two OID -> ID value mapping entries one for the article, one for the user object'); + self::assertArrayHasKey($mergedArticleOid, $entityIdentifiers); + self::assertArrayHasKey($mergedUserOid, $entityIdentifiers); + + self::assertSame([ + $mergedUserOid => UnitOfWork::STATE_MANAGED, + $mergedArticleOid => UnitOfWork::STATE_MANAGED, + ], $entityStates, 'UoW#entityStates contains two OID -> state entries, one for the article, one for the user object'); + + self::assertCount(2, $entityIdentifiers); + self::assertArrayHasKey($mergedArticleOid, $entityIdentifiers); + self::assertArrayHasKey($mergedUserOid, $entityIdentifiers); + + self::assertSame([ + CmsUser::class => [$user->id => $mergedUser], + CmsArticle::class => [$article->id => $mergedArticle], + ], $identityMap, 'The identity map contains exactly two objects, the article and the user.'); + } + + private function grabProperty(string $name, UnitOfWork $uow) + { + $reflection = new ReflectionClass($uow); + $property = $reflection->getProperty($name); + $property->setAccessible(true); + + return $property->getValue($uow); + } +} From 1989531d4ff67a19832a8219a8ef69c84bd52d9b Mon Sep 17 00:00:00 2001 From: Matthias Pigulla Date: Fri, 23 Jun 2023 22:47:34 +0200 Subject: [PATCH 2/2] Update tests/Doctrine/Tests/ORM/Functional/Ticket/GH7407Test.php MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Grégoire Paris --- tests/Doctrine/Tests/ORM/Functional/Ticket/GH7407Test.php | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/Doctrine/Tests/ORM/Functional/Ticket/GH7407Test.php b/tests/Doctrine/Tests/ORM/Functional/Ticket/GH7407Test.php index 3e010e177cb..c12d3da3279 100644 --- a/tests/Doctrine/Tests/ORM/Functional/Ticket/GH7407Test.php +++ b/tests/Doctrine/Tests/ORM/Functional/Ticket/GH7407Test.php @@ -76,6 +76,7 @@ public function testMergingEntitiesDoesNotCreateUnmanagedProxyReferences(): void ], $identityMap, 'The identity map contains exactly two objects, the article and the user.'); } + /** @return mixed */ private function grabProperty(string $name, UnitOfWork $uow) { $reflection = new ReflectionClass($uow);