From 022a487f696b4203d8b214f39085dc88e8945c52 Mon Sep 17 00:00:00 2001 From: Justin Fortunato Date: Thu, 15 May 2025 15:50:18 -0400 Subject: [PATCH 1/4] Fix updates to embedded documents after removal After a parent document and its embeds are removed and then subsequently persisted, the embedded documents didn't get updated. This happened because the parent document goes through an upsert operation, but `PersistenceBuilder->prepareUpsertData()` did not handle embedded associations in the same way that `PersistenceBuilder->prepareUpdateData()` does. This fixes that issue so that an upserted parent document handles embed-many's the same was as an updated parent document. Fixes doctrine#2767 --- .../MongoDB/Persisters/PersistenceBuilder.php | 23 ++++-- lib/Doctrine/ODM/MongoDB/UnitOfWork.php | 7 +- .../Tests/Functional/Ticket/GH2767Test.php | 75 +++++++++++++++++++ 3 files changed, 98 insertions(+), 7 deletions(-) create mode 100644 tests/Doctrine/ODM/MongoDB/Tests/Functional/Ticket/GH2767Test.php diff --git a/lib/Doctrine/ODM/MongoDB/Persisters/PersistenceBuilder.php b/lib/Doctrine/ODM/MongoDB/Persisters/PersistenceBuilder.php index 80c257d66c..39cf84877e 100644 --- a/lib/Doctrine/ODM/MongoDB/Persisters/PersistenceBuilder.php +++ b/lib/Doctrine/ODM/MongoDB/Persisters/PersistenceBuilder.php @@ -287,12 +287,23 @@ public function prepareUpsertData($document) $updateData['$set'][$mapping['name']] = $this->prepareReferencedDocumentValue($mapping, $new); // @ReferenceMany, @EmbedMany - } elseif ( - $mapping['type'] === ClassMetadata::MANY && ! $mapping['isInverseSide'] - && $new instanceof PersistentCollectionInterface && $new->isDirty() - && CollectionHelper::isAtomic($mapping['strategy']) - ) { - $updateData['$set'][$mapping['name']] = $this->prepareAssociatedCollectionValue($new, true); + } elseif ($mapping['type'] === ClassMetadata::MANY) { + if (! $mapping['isInverseSide'] && $new instanceof PersistentCollectionInterface && $new->isDirty() && CollectionHelper::isAtomic($mapping['strategy'])) { + $updateData['$set'][$mapping['name']] = $this->prepareAssociatedCollectionValue($new, true); + } elseif ($mapping['association'] === ClassMetadata::EMBED_MANY) { + foreach ($new as $key => $embeddedDoc) { + if ($this->uow->isScheduledForInsert($embeddedDoc)) { + continue; + } + + $update = $this->prepareUpsertData($embeddedDoc); + foreach ($update as $cmd => $values) { + foreach ($values as $name => $value) { + $updateData[$cmd][$mapping['name'] . '.' . $key . '.' . $name] = $value; + } + } + } + } } // @EmbedMany and @ReferenceMany are handled by CollectionPersister } diff --git a/lib/Doctrine/ODM/MongoDB/UnitOfWork.php b/lib/Doctrine/ODM/MongoDB/UnitOfWork.php index 82d4195aa6..69e5ed68d9 100644 --- a/lib/Doctrine/ODM/MongoDB/UnitOfWork.php +++ b/lib/Doctrine/ODM/MongoDB/UnitOfWork.php @@ -1790,7 +1790,12 @@ private function doPersist(object $document, array &$visited): void // Document becomes managed again unset($this->scheduledDocumentDeletions[$oid]); - $this->persistNew($class, $document); + if ($class->isEmbeddedDocument) { + $this->documentStates[$oid] = self::STATE_MANAGED; + } else { + $this->persistNew($class, $document); + } + break; case self::STATE_DETACHED: diff --git a/tests/Doctrine/ODM/MongoDB/Tests/Functional/Ticket/GH2767Test.php b/tests/Doctrine/ODM/MongoDB/Tests/Functional/Ticket/GH2767Test.php new file mode 100644 index 0000000000..15ab254b42 --- /dev/null +++ b/tests/Doctrine/ODM/MongoDB/Tests/Functional/Ticket/GH2767Test.php @@ -0,0 +1,75 @@ +dm->persist($document); + $this->dm->flush(); + $id = $document->id; + + // Remove the document + $this->dm->remove($document); + + // Increase the removedCount of the parent document and the embedded document by 1 + $document->incrementRemovedCount(); + + // Re-persist the same document + $this->dm->persist($document); + + $this->dm->flush(); + $this->dm->clear(); + + $repository = $this->dm->getRepository(TestDocument::class); + $result = $repository->find($id); + + self::assertEquals(1, $result->removedCount); + self::assertEquals(1, $result->embeddedDocuments[0]->removedCount); + } +} + +#[ODM\Document] +class TestDocument +{ + #[ODM\Id] + public ?string $id = null; + + #[ODM\Field(type: 'int')] + public int $removedCount = 0; + + /** @var Collection */ + #[ODM\EmbedMany(targetDocument: TestEmbeddedDocument::class)] + public $embeddedDocuments; + + /** @param TestEmbeddedDocument[] $embeddedDocuments */ + public function __construct(array $embeddedDocuments) + { + $this->embeddedDocuments = new ArrayCollection($embeddedDocuments); + } + + public function incrementRemovedCount(): void + { + $this->removedCount++; + foreach ($this->embeddedDocuments as $embeddedDocument) { + $embeddedDocument->removedCount++; + } + } +} + +#[ODM\EmbeddedDocument] +class TestEmbeddedDocument +{ + #[ODM\Field(type: 'int')] + public int $removedCount = 0; +} From 881c6f8f2ff7b77385871dbbd4eacb9fcfa7844e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Tamarelle?= Date: Wed, 21 May 2025 11:40:26 +0200 Subject: [PATCH 2/4] Rename test classes to prevent name conflict --- .../MongoDB/Tests/Functional/Ticket/GH2767Test.php | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/tests/Doctrine/ODM/MongoDB/Tests/Functional/Ticket/GH2767Test.php b/tests/Doctrine/ODM/MongoDB/Tests/Functional/Ticket/GH2767Test.php index 15ab254b42..18bcefef66 100644 --- a/tests/Doctrine/ODM/MongoDB/Tests/Functional/Ticket/GH2767Test.php +++ b/tests/Doctrine/ODM/MongoDB/Tests/Functional/Ticket/GH2767Test.php @@ -14,7 +14,7 @@ class GH2767Test extends BaseTestCase public function testRemovePersistEmbedded(): void { // Start with a parent document and 1 embedded document, both with removedCount = 0 - $document = new TestDocument([new TestEmbeddedDocument()]); + $document = new GH2767TestDocument([new GH2767TestEmbeddedDocument()]); $this->dm->persist($document); $this->dm->flush(); $id = $document->id; @@ -31,7 +31,7 @@ public function testRemovePersistEmbedded(): void $this->dm->flush(); $this->dm->clear(); - $repository = $this->dm->getRepository(TestDocument::class); + $repository = $this->dm->getRepository(GH2767TestDocument::class); $result = $repository->find($id); self::assertEquals(1, $result->removedCount); @@ -40,7 +40,7 @@ public function testRemovePersistEmbedded(): void } #[ODM\Document] -class TestDocument +class GH2767TestDocument { #[ODM\Id] public ?string $id = null; @@ -48,11 +48,11 @@ class TestDocument #[ODM\Field(type: 'int')] public int $removedCount = 0; - /** @var Collection */ - #[ODM\EmbedMany(targetDocument: TestEmbeddedDocument::class)] + /** @var Collection */ + #[ODM\EmbedMany(targetDocument: GH2767TestEmbeddedDocument::class)] public $embeddedDocuments; - /** @param TestEmbeddedDocument[] $embeddedDocuments */ + /** @param GH2767TestEmbeddedDocument[] $embeddedDocuments */ public function __construct(array $embeddedDocuments) { $this->embeddedDocuments = new ArrayCollection($embeddedDocuments); @@ -68,7 +68,7 @@ public function incrementRemovedCount(): void } #[ODM\EmbeddedDocument] -class TestEmbeddedDocument +class GH2767TestEmbeddedDocument { #[ODM\Field(type: 'int')] public int $removedCount = 0; From 7a4d664678f474857df62cf638c31e6468280507 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Tamarelle?= Date: Wed, 21 May 2025 11:44:46 +0200 Subject: [PATCH 3/4] Split handling of ReferenceMany and EmbedMany --- .../MongoDB/Persisters/PersistenceBuilder.php | 43 +++++++++++-------- 1 file changed, 24 insertions(+), 19 deletions(-) diff --git a/lib/Doctrine/ODM/MongoDB/Persisters/PersistenceBuilder.php b/lib/Doctrine/ODM/MongoDB/Persisters/PersistenceBuilder.php index 39cf84877e..96fac94161 100644 --- a/lib/Doctrine/ODM/MongoDB/Persisters/PersistenceBuilder.php +++ b/lib/Doctrine/ODM/MongoDB/Persisters/PersistenceBuilder.php @@ -282,30 +282,35 @@ public function prepareUpsertData($document) } } - // @ReferenceOne - } elseif ($mapping['association'] === ClassMetadata::REFERENCE_ONE) { - $updateData['$set'][$mapping['name']] = $this->prepareReferencedDocumentValue($mapping, $new); - - // @ReferenceMany, @EmbedMany - } elseif ($mapping['type'] === ClassMetadata::MANY) { - if (! $mapping['isInverseSide'] && $new instanceof PersistentCollectionInterface && $new->isDirty() && CollectionHelper::isAtomic($mapping['strategy'])) { - $updateData['$set'][$mapping['name']] = $this->prepareAssociatedCollectionValue($new, true); - } elseif ($mapping['association'] === ClassMetadata::EMBED_MANY) { - foreach ($new as $key => $embeddedDoc) { - if ($this->uow->isScheduledForInsert($embeddedDoc)) { - continue; - } + // @EmbedMany + } elseif ($mapping['association'] === ClassMetadata::EMBED_MANY) { + foreach ($new as $key => $embeddedDoc) { + if ($this->uow->isScheduledForInsert($embeddedDoc)) { + continue; + } - $update = $this->prepareUpsertData($embeddedDoc); - foreach ($update as $cmd => $values) { - foreach ($values as $name => $value) { - $updateData[$cmd][$mapping['name'] . '.' . $key . '.' . $name] = $value; - } + $update = $this->prepareUpsertData($embeddedDoc); + foreach ($update as $cmd => $values) { + foreach ($values as $name => $value) { + $updateData[$cmd][$mapping['name'] . '.' . $key . '.' . $name] = $value; } } } + + // @ReferenceOne + } elseif ($mapping['association'] === ClassMetadata::REFERENCE_ONE) { + $updateData['$set'][$mapping['name']] = $this->prepareReferencedDocumentValue($mapping, $new); + + // @ReferenceMany + } elseif ( + $mapping['type'] === ClassMetadata::MANY && ! $mapping['isInverseSide'] + && $new instanceof PersistentCollectionInterface && $new->isDirty() + && CollectionHelper::isAtomic($mapping['strategy']) + ) { + $updateData['$set'][$mapping['name']] = $this->prepareAssociatedCollectionValue($new, true); } - // @EmbedMany and @ReferenceMany are handled by CollectionPersister + + // @ReferenceMany is handled by CollectionPersister } // add discriminator if the class has one From 0fc3237947c99512a652721e61358167acf78f09 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Tamarelle?= Date: Wed, 21 May 2025 12:22:44 +0200 Subject: [PATCH 4/4] Specific EmbedMany handling doesn't apply with atomic strategy --- lib/Doctrine/ODM/MongoDB/Persisters/PersistenceBuilder.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/Doctrine/ODM/MongoDB/Persisters/PersistenceBuilder.php b/lib/Doctrine/ODM/MongoDB/Persisters/PersistenceBuilder.php index 96fac94161..65931a1eec 100644 --- a/lib/Doctrine/ODM/MongoDB/Persisters/PersistenceBuilder.php +++ b/lib/Doctrine/ODM/MongoDB/Persisters/PersistenceBuilder.php @@ -283,7 +283,7 @@ public function prepareUpsertData($document) } // @EmbedMany - } elseif ($mapping['association'] === ClassMetadata::EMBED_MANY) { + } elseif ($mapping['association'] === ClassMetadata::EMBED_MANY && ! CollectionHelper::isAtomic($mapping['strategy'])) { foreach ($new as $key => $embeddedDoc) { if ($this->uow->isScheduledForInsert($embeddedDoc)) { continue;