Added check for sub-paths before collections removal in CollectionPer…

…sister::deleteAll. Fixed style of code.
doctrine · Oct 19, 2018 · fdb0202 · fdb0202
1 parent d9e4d28
commit fdb0202
Show file tree

Hide file tree

Showing 4 changed files with 75 additions and 23 deletions.
diff --git 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
@@ -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
@@ -28,7 +28,7 @@
     </groups>
 
     <php>
-        <const name="DOCTRINE_MONGODB_SERVER" value="mongodb://localhost:27017" />
+        <const name="DOCTRINE_MONGODB_SERVER" value="mongodb://172.100.0.171:27017" />
         <const name="DOCTRINE_MONGODB_DATABASE" value="doctrine_odm_tests" />
     </php>
 </phpunit>
diff --git 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()
     {