From fdb0202f9a4e829d62c0e2da407eecba8942b414 Mon Sep 17 00:00:00 2001 From: watari Date: Fri, 19 Oct 2018 17:48:46 +0300 Subject: [PATCH] Added check for sub-paths before collections removal in CollectionPersister::deleteAll. Fixed style of code. --- .../Persisters/CollectionPersister.php | 47 +++++++++++++++---- .../MongoDB/Persisters/DocumentPersister.php | 8 ++-- phpunit.xml.dist | 2 +- .../Functional/CollectionPersisterTest.php | 41 +++++++++++----- 4 files changed, 75 insertions(+), 23 deletions(-) diff --git a/lib/Doctrine/ODM/MongoDB/Persisters/CollectionPersister.php b/lib/Doctrine/ODM/MongoDB/Persisters/CollectionPersister.php index a61b5af2e8..d12696562a 100644 --- a/lib/Doctrine/ODM/MongoDB/Persisters/CollectionPersister.php +++ b/lib/Doctrine/ODM/MongoDB/Persisters/CollectionPersister.php @@ -12,12 +12,18 @@ use Doctrine\ODM\MongoDB\Utility\CollectionHelper; use LogicException; use UnexpectedValueException; +use function array_fill_keys; +use function array_keys; use function array_map; use function array_reverse; +use function array_unique; use function array_values; +use function count; use function get_class; use function implode; +use function spl_object_hash; use function sprintf; +use function strpos; /** * The CollectionPersister is responsible for persisting collections of embedded @@ -51,11 +57,11 @@ public function __construct(DocumentManager $dm, PersistenceBuilder $pb, UnitOfW * Deletes a PersistentCollection instances completely from a document using $unset. If collections belong to the different * * @param PersistentCollectionInterface[] $collections - * @param array $options + * @param array $options */ public function deleteAll(array $collections, array $options) { - $parents = []; + $parents = []; $unsetPathsMap = []; foreach ($collections as $coll) { @@ -64,16 +70,17 @@ public function deleteAll(array $collections, array $options) continue; // ignore inverse side } if (CollectionHelper::isAtomic($mapping['strategy'])) { - throw new \UnexpectedValueException($mapping['strategy'] . ' delete collection strategy should have been handled by DocumentPersister. Please report a bug in issue tracker'); + throw new UnexpectedValueException($mapping['strategy'] . ' delete collection strategy should have been handled by DocumentPersister. Please report a bug in issue tracker'); } - list($propertyPath, $parent) = $this->getPathAndParent($coll); - $oid = \spl_object_hash($parent); - $parents[$oid] = $parent; + [$propertyPath, $parent] = $this->getPathAndParent($coll); + $oid = spl_object_hash($parent); + $parents[$oid] = $parent; $unsetPathsMap[$oid][$propertyPath] = true; } - foreach ($unsetPathsMap as $oid => $unsetPaths) { - $query = array('$unset' => $unsetPaths); + foreach ($unsetPathsMap as $oid => $paths) { + $unsetPaths = array_fill_keys($this->excludeSubPaths(array_keys($paths)), true); + $query = ['$unset' => $unsetPaths]; $this->executeQuery($parents[$oid], $query, $options); } } @@ -276,4 +283,28 @@ private function executeQuery(object $document, array $newObj, array $options) : throw LockException::lockFailed($document); } } + + private function excludeSubPaths(array $paths) + { + $checkedPaths = []; + $pathsAmount = count($paths); + $paths = array_unique($paths); + for ($i = 0; $i < $pathsAmount; $i++) { + $isSubPath = false; + $j = $i; + for (; $j < $pathsAmount; $j++) { + if ($i !== $j && strpos($paths[$i], $paths[$j]) === 0) { + $isSubPath = true; + break; + } + } + if ($isSubPath) { + continue; + } + + $checkedPaths[] = $paths[$i]; + } + + return $checkedPaths; + } } diff --git a/lib/Doctrine/ODM/MongoDB/Persisters/DocumentPersister.php b/lib/Doctrine/ODM/MongoDB/Persisters/DocumentPersister.php index d878d18bc6..e9ed4f3761 100644 --- a/lib/Doctrine/ODM/MongoDB/Persisters/DocumentPersister.php +++ b/lib/Doctrine/ODM/MongoDB/Persisters/DocumentPersister.php @@ -1272,11 +1272,13 @@ private function handleCollections(object $document, array $options) : void $collections = []; // Collection deletions (deletions of complete collections) foreach ($this->uow->getScheduledCollections($document) as $coll) { - if ($this->uow->isCollectionScheduledForDeletion($coll)) { - $collections[] = $coll; + if (! $this->uow->isCollectionScheduledForDeletion($coll)) { + continue; } + + $collections[] = $coll; } - if (!empty($collections)) { + if (! empty($collections)) { $this->cp->deleteAll($collections, $options); } // Collection updates (deleteRows, updateRows, insertRows) diff --git a/phpunit.xml.dist b/phpunit.xml.dist index 1bf79ca26d..d3c1c2696f 100644 --- a/phpunit.xml.dist +++ b/phpunit.xml.dist @@ -28,7 +28,7 @@ - + diff --git a/tests/Doctrine/ODM/MongoDB/Tests/Functional/CollectionPersisterTest.php b/tests/Doctrine/ODM/MongoDB/Tests/Functional/CollectionPersisterTest.php index 4c8e70ab0a..a4f8a8d2bf 100644 --- a/tests/Doctrine/ODM/MongoDB/Tests/Functional/CollectionPersisterTest.php +++ b/tests/Doctrine/ODM/MongoDB/Tests/Functional/CollectionPersisterTest.php @@ -61,43 +61,62 @@ public function testDeleteNestedEmbedMany() public function testDeleteAllEmbedMany() { $persister = $this->getCollectionPersister(); - $user = $this->getTestUser('jwage'); - $persister->deleteAll([$user->categories], array()); - $user = $this->dm->getDocumentCollection(__NAMESPACE__ . '\CollectionPersisterUser')->findOne(array('username' => 'jwage')); + $user = $this->getTestUser('jwage'); + $persister->deleteAll([$user->categories], []); + $user = $this->dm->getDocumentCollection(__NAMESPACE__ . '\CollectionPersisterUser')->findOne(['username' => 'jwage']); $this->assertArrayNotHasKey('categories', $user, 'Test that the categories field was deleted'); } public function testDeleteAllReferenceMany() { $persister = $this->getCollectionPersister(); - $user = $this->getTestUser('jwage'); - $persister->deleteAll([$user->phonenumbers], array()); - $user = $this->dm->getDocumentCollection(__NAMESPACE__ . '\CollectionPersisterUser')->findOne(array('username' => 'jwage')); + $user = $this->getTestUser('jwage'); + $persister->deleteAll([$user->phonenumbers], []); + $user = $this->dm->getDocumentCollection(__NAMESPACE__ . '\CollectionPersisterUser')->findOne(['username' => 'jwage']); $this->assertArrayNotHasKey('phonenumbers', $user, 'Test that the phonenumbers field was deleted'); } public function testDeleteAllNestedEmbedMany() { $persister = $this->getCollectionPersister(); - $user = $this->getTestUser('jwage'); + $user = $this->getTestUser('jwage'); $persister->deleteAll( [$user->categories[0]->children[0]->children, $user->categories[0]->children[1]->children], - array() + [] ); - $check = $this->dm->getDocumentCollection(__NAMESPACE__ . '\CollectionPersisterUser')->findOne(array('username' => 'jwage')); + $check = $this->dm->getDocumentCollection(__NAMESPACE__ . '\CollectionPersisterUser')->findOne(['username' => 'jwage']); $this->assertFalse(isset($check['categories']['0']['children'][0]['children'])); $this->assertFalse(isset($check['categories']['0']['children'][1]['children'])); $persister->deleteAll( [$user->categories[0]->children, $user->categories[1]->children], - array() + [] ); - $check = $this->dm->getDocumentCollection(__NAMESPACE__ . '\CollectionPersisterUser')->findOne(array('username' => 'jwage')); + $check = $this->dm->getDocumentCollection(__NAMESPACE__ . '\CollectionPersisterUser')->findOne(['username' => 'jwage']); $this->assertFalse(isset($check['categories'][0]['children']), 'Test that the nested children categories field was deleted'); $this->assertTrue(isset($check['categories'][0]), 'Test that the category with the children still exists'); $this->assertFalse(isset($check['categories'][1]['children']), 'Test that the nested children categories field was deleted'); $this->assertTrue(isset($check['categories'][1]), 'Test that the category with the children still exists'); } + public function testDeleteAllNestedEmbedManyAndNestedParent() + { + $persister = $this->getCollectionPersister(); + $user = $this->getTestUser('jwage'); + $persister->deleteAll( + [$user->categories[0]->children[0]->children, $user->categories[0]->children[1]->children], + [] + ); + $check = $this->dm->getDocumentCollection(__NAMESPACE__ . '\CollectionPersisterUser')->findOne(['username' => 'jwage']); + $this->assertFalse(isset($check['categories']['0']['children'][0]['children'])); + $this->assertFalse(isset($check['categories']['0']['children'][1]['children'])); + $persister->deleteAll( + [$user->categories[0]->children, $user->categories[0]->children[1]->children, $user->categories], + [] + ); + $check = $this->dm->getDocumentCollection(__NAMESPACE__ . '\CollectionPersisterUser')->findOne(['username' => 'jwage']); + $this->assertFalse(isset($check['categories']), 'Test that the nested categories field was deleted'); + } + public function testDeleteRows() {