IBX-8418: Fixed removing orphaned drafts when trashing or deleting its ancestors (#439)

barw4 · web-flow · commit 02200e652f7e · 2024-11-08T16:26:12.000+01:00
For more details see https://issues.ibexa.co/browse/IBX-8418 and #439 Key changes: * Fixed removing orphaned draft when trashing or deleting its parent or ancestor location
diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon
@@ -19010,11 +19010,6 @@ parameters:
 			count: 1
 			path: src/lib/Persistence/Legacy/User/Mapper.php
 
-		-
-			message: "#^array\\|string does not accept array\\<int, mixed\\>\\.$#"
-			count: 1
-			path: src/lib/Persistence/Legacy/User/Mapper.php
-
 		-
 			message: "#^Method Ibexa\\\\Core\\\\Persistence\\\\Legacy\\\\User\\\\Role\\\\Gateway\\:\\:addPolicyLimitations\\(\\) has parameter \\$limitations with no value type specified in iterable type array\\.$#"
 			count: 1
@@ -27090,23 +27085,18 @@ parameters:
 			count: 5
 			path: tests/integration/Core/Repository/ContentServiceTest.php
 
-		-
-			message: "#^Cannot access offset 0 on iterable\\<int, Ibexa\\\\Contracts\\\\Core\\\\Repository\\\\Values\\\\Content\\\\Relation\\>\\.$#"
-			count: 12
-			path: tests/integration/Core/Repository/ContentServiceTest.php
-
 		-
 			message: "#^Cannot access offset 0 on iterable\\<Ibexa\\\\Contracts\\\\Core\\\\Repository\\\\Values\\\\Content\\\\VersionInfo\\>\\.$#"
 			count: 7
 			path: tests/integration/Core/Repository/ContentServiceTest.php
 
 		-
-			message: "#^Cannot access offset 0 on iterable\\<int, Ibexa\\\\Contracts\\\\Core\\\\Repository\\\\Values\\\\Content\\\\VersionInfo\\>\\.$#"
-			count: 4
+			message: "#^Cannot access offset 0 on iterable\\<int, Ibexa\\\\Contracts\\\\Core\\\\Repository\\\\Values\\\\Content\\\\Relation\\>\\.$#"
+			count: 12
 			path: tests/integration/Core/Repository/ContentServiceTest.php
 
 		-
-			message: "#^Cannot access offset 1 on iterable\\<int, Ibexa\\\\Contracts\\\\Core\\\\Repository\\\\Values\\\\Content\\\\Relation\\>\\.$#"
+			message: "#^Cannot access offset 0 on iterable\\<int, Ibexa\\\\Contracts\\\\Core\\\\Repository\\\\Values\\\\Content\\\\VersionInfo\\>\\.$#"
 			count: 4
 			path: tests/integration/Core/Repository/ContentServiceTest.php
 
@@ -27115,6 +27105,11 @@ parameters:
 			count: 2
 			path: tests/integration/Core/Repository/ContentServiceTest.php
 
+		-
+			message: "#^Cannot access offset 1 on iterable\\<int, Ibexa\\\\Contracts\\\\Core\\\\Repository\\\\Values\\\\Content\\\\Relation\\>\\.$#"
+			count: 4
+			path: tests/integration/Core/Repository/ContentServiceTest.php
+
 		-
 			message: "#^Cannot access offset 1 on iterable\\<int, Ibexa\\\\Contracts\\\\Core\\\\Repository\\\\Values\\\\Content\\\\VersionInfo\\>\\.$#"
 			count: 1
@@ -27141,12 +27136,12 @@ parameters:
 			path: tests/integration/Core/Repository/ContentServiceTest.php
 
 		-
-			message: "#^Cannot access offset mixed on iterable\\<int, Ibexa\\\\Contracts\\\\Core\\\\Repository\\\\Values\\\\Content\\\\ContentInfo\\>\\.$#"
+			message: "#^Cannot access offset int on iterable\\<int, Ibexa\\\\Contracts\\\\Core\\\\Repository\\\\Values\\\\Content\\\\VersionInfo\\>\\.$#"
 			count: 1
 			path: tests/integration/Core/Repository/ContentServiceTest.php
 
 		-
-			message: "#^Cannot access offset int on iterable\\<int, Ibexa\\\\Contracts\\\\Core\\\\Repository\\\\Values\\\\Content\\\\VersionInfo\\>\\.$#"
+			message: "#^Cannot access offset mixed on iterable\\<int, Ibexa\\\\Contracts\\\\Core\\\\Repository\\\\Values\\\\Content\\\\ContentInfo\\>\\.$#"
 			count: 1
 			path: tests/integration/Core/Repository/ContentServiceTest.php
 
diff --git a/src/contracts/Persistence/Content/Location/Handler.php b/src/contracts/Persistence/Content/Location/Handler.php
@@ -215,6 +215,11 @@ public function create(CreateStruct $location);
      */
     public function removeSubtree($locationId);
 
+    /**
+     * Removes all draft contents that have no location assigned to them under the given parent location.
+     */
+    public function deleteChildrenDrafts(int $locationId): void;
+
     /**
      * Set section on all content objects in the subtree.
      * Only main locations will be updated.
diff --git a/src/contracts/Test/IbexaKernelTestTrait.php b/src/contracts/Test/IbexaKernelTestTrait.php
@@ -19,6 +19,7 @@
 use Ibexa\Contracts\Core\Repository\RoleService;
 use Ibexa\Contracts\Core\Repository\SearchService;
 use Ibexa\Contracts\Core\Repository\SectionService;
+use Ibexa\Contracts\Core\Repository\TrashService;
 use Ibexa\Contracts\Core\Repository\URLAliasService;
 use Ibexa\Contracts\Core\Repository\UserService;
 use Ibexa\Contracts\Core\Test\Persistence\Fixture\FixtureImporter;
@@ -169,6 +170,11 @@ protected static function getUrlAliasService(): URLAliasService
         return self::getServiceByClassName(URLAliasService::class);
     }
 
+    protected static function getTrashService(): TrashService
+    {
+        return self::getServiceByClassName(TrashService::class);
+    }
+
     protected static function setAnonymousUser(): void
     {
         $anonymousUserId = 10;
diff --git a/src/contracts/Test/IbexaTestKernel.php b/src/contracts/Test/IbexaTestKernel.php
@@ -92,6 +92,7 @@ class IbexaTestKernel extends Kernel implements IbexaTestKernelInterface
         Repository\TokenService::class,
         Repository\URLAliasService::class,
         Repository\BookmarkService::class,
+        Repository\TrashService::class,
         Handler::class,
     ];
 
diff --git a/src/lib/Persistence/Cache/LocationHandler.php b/src/lib/Persistence/Cache/LocationHandler.php
@@ -415,6 +415,20 @@ public function removeSubtree($locationId)
         return $return;
     }
 
+    public function deleteChildrenDrafts(int $locationId): void
+    {
+        $this->logger->logCall(__METHOD__, ['location' => $locationId]);
+
+        $this->persistenceHandler->locationHandler()->deleteChildrenDrafts($locationId);
+
+        $this->cache->invalidateTags([
+            $this->cacheIdentifierGenerator->generateTag(
+                self::LOCATION_PATH_IDENTIFIER,
+                [$locationId],
+            ),
+        ]);
+    }
+
     /**
      * {@inheritdoc}
      */
diff --git a/src/lib/Persistence/Legacy/Content/Handler.php b/src/lib/Persistence/Legacy/Content/Handler.php
@@ -650,6 +650,7 @@ public function deleteContent($contentId)
             $this->removeRawContent($contentId);
         } else {
             foreach ($contentLocations as $locationId) {
+                $this->treeHandler->deleteChildrenDrafts($locationId);
                 $this->treeHandler->removeSubtree($locationId);
             }
         }
diff --git a/src/lib/Persistence/Legacy/Content/Location/Gateway.php b/src/lib/Persistence/Legacy/Content/Location/Gateway.php
@@ -111,6 +111,13 @@ abstract public function loadParentLocationsDataForDraftContent(int $contentId):
      */
     abstract public function getSubtreeContent(int $sourceId, bool $onlyIds = false): array;
 
+    /**
+     * Finds draft contents created under the given parent location.
+     *
+     * @return array<int>
+     */
+    abstract public function getSubtreeChildrenDraftContentIds(int $sourceId): array;
+
     abstract public function getSubtreeSize(string $path): int;
 
     /**
diff --git a/src/lib/Persistence/Legacy/Content/Location/Gateway/DoctrineDatabase.php b/src/lib/Persistence/Legacy/Content/Location/Gateway/DoctrineDatabase.php
@@ -237,6 +237,29 @@ public function getSubtreeContent(int $sourceId, bool $onlyIds = false): array
             : $results;
     }
 
+    /**
+     * @return array<int>
+     *
+     * @throws \Doctrine\DBAL\Exception
+     * @throws \Doctrine\DBAL\Driver\Exception
+     */
+    public function getSubtreeChildrenDraftContentIds(int $sourceId): array
+    {
+        $query = $this->connection->createQueryBuilder();
+        $query
+            ->select('contentobject_id')
+            ->from('eznode_assignment', 'n')
+            ->innerJoin('n', 'ezcontentobject', 'c', 'n.contentobject_id = c.id')
+            ->andWhere('n.parent_node = :parentNode')
+            ->andWhere('c.status = :status')
+            ->setParameter(':parentNode', $sourceId, ParameterType::INTEGER)
+            ->setParameter(':status', ContentInfo::STATUS_DRAFT, ParameterType::INTEGER);
+
+        $statement = $query->execute();
+
+        return $statement->fetchFirstColumn();
+    }
+
     public function getSubtreeSize(string $path): int
     {
         $query = $this->createNodeQueryBuilder([$this->dbPlatform->getCountExpression('node_id')]);
diff --git a/src/lib/Persistence/Legacy/Content/Location/Gateway/ExceptionConversion.php b/src/lib/Persistence/Legacy/Content/Location/Gateway/ExceptionConversion.php
@@ -106,6 +106,18 @@ public function getSubtreeContent(int $sourceId, bool $onlyIds = false): array
         }
     }
 
+    /**
+     * @return array<int>
+     */
+    public function getSubtreeChildrenDraftContentIds(int $sourceId): array
+    {
+        try {
+            return $this->innerGateway->getSubtreeChildrenDraftContentIds($sourceId);
+        } catch (PDOException $e) {
+            throw DatabaseException::wrap($e);
+        }
+    }
+
     public function getSubtreeSize(string $path): int
     {
         try {
diff --git a/src/lib/Persistence/Legacy/Content/Location/Handler.php b/src/lib/Persistence/Legacy/Content/Location/Handler.php
@@ -546,6 +546,11 @@ public function removeSubtree($locationId)
         $this->treeHandler->removeSubtree($locationId);
     }
 
+    public function deleteChildrenDrafts(int $locationId): void
+    {
+        $this->treeHandler->deleteChildrenDrafts($locationId);
+    }
+
     /**
      * Set section on all content objects in the subtree.
      *
diff --git a/src/lib/Persistence/Legacy/Content/TreeHandler.php b/src/lib/Persistence/Legacy/Content/TreeHandler.php
@@ -215,6 +215,26 @@ public function removeSubtree($locationId)
         $this->locationGateway->deleteNodeAssignment($contentId);
     }
 
+    /**
+     * Removes draft contents assigned to the given parent location and its descendant locations.
+     *
+     * @throws \Ibexa\Contracts\Core\Repository\Exceptions\NotFoundException
+     */
+    public function deleteChildrenDrafts(int $locationId): void
+    {
+        $subLocations = $this->locationGateway->getChildren($locationId);
+        foreach ($subLocations as $subLocation) {
+            $this->deleteChildrenDrafts($subLocation['node_id']);
+        }
+
+        // Fetch child draft content ids
+        $subtreeChildrenDraftIds = $this->locationGateway->getSubtreeChildrenDraftContentIds($locationId);
+
+        foreach ($subtreeChildrenDraftIds as $contentId) {
+            $this->removeRawContent($contentId);
+        }
+    }
+
     /**
      * Set section on all content objects in the subtree.
      *
diff --git a/src/lib/Repository/TrashService.php b/src/lib/Repository/TrashService.php
@@ -146,6 +146,7 @@ public function trash(Location $location): ?APITrashItem
 
         $this->repository->beginTransaction();
         try {
+            $this->persistenceHandler->locationHandler()->deleteChildrenDrafts($location->id);
             $spiTrashItem = $this->persistenceHandler->trashHandler()->trashSubtree($location->id);
             $this->persistenceHandler->urlAliasHandler()->locationDeleted($location->id);
             $this->repository->commit();
diff --git a/tests/integration/Core/Repository/ContentService/DeleteContentTest.php b/tests/integration/Core/Repository/ContentService/DeleteContentTest.php
@@ -0,0 +1,111 @@
+<?php
+
+/**
+ * @copyright Copyright (C) Ibexa AS. All rights reserved.
+ * @license For full copyright and license information view LICENSE file distributed with this source code.
+ */
+declare(strict_types=1);
+
+namespace Ibexa\Tests\Integration\Core\Repository\ContentService;
+
+use Ibexa\Contracts\Core\Repository\Values\Content\Content;
+use Ibexa\Tests\Integration\Core\RepositoryTestCase;
+use PHPUnit\Framework\Assert;
+
+/**
+ * @covers \Ibexa\Contracts\Core\Repository\ContentService
+ */
+final class DeleteContentTest extends RepositoryTestCase
+{
+    /**
+     * @throws \Ibexa\Contracts\Core\Repository\Exceptions\Exception
+     */
+    public function testDeleteContentDeletesChildrenDrafts(): void
+    {
+        $contentService = self::getContentService();
+
+        [$folder, $draft1, $draft2, $draft3, $draftSecondDepth] = $this->prepareContentStructure();
+
+        $contentService->deleteContent($folder->getContentInfo());
+
+        $contentInfos = $contentService->loadContentInfoList([
+            $draft1->getId(),
+            $draft2->getId(),
+            $draft3->getId(),
+            $draftSecondDepth->getId(),
+        ]);
+
+        self::assertEmpty($contentInfos);
+    }
+
+    /**
+     * @throws \Ibexa\Contracts\Core\Repository\Exceptions\Exception
+     */
+    public function testTrashLocationDeletesChildrenDrafts(): void
+    {
+        $trashService = self::getTrashService();
+        $contentService = self::getContentService();
+
+        [$folder, $draft1, $draft2, $draft3, $draftSecondDepth] = $this->prepareContentStructure();
+
+        $folderMainLocationId = $folder->getVersionInfo()->getContentInfo()->getMainLocationId();
+        Assert::assertIsNumeric($folderMainLocationId);
+
+        $locationToTrash = self::getLocationService()->loadLocation($folderMainLocationId);
+
+        $trashService->trash($locationToTrash);
+
+        $contentInfos = $contentService->loadContentInfoList([
+            $draft1->getId(),
+            $draft2->getId(),
+            $draft3->getId(),
+            $draftSecondDepth->getId(),
+        ]);
+
+        self::assertEmpty($contentInfos);
+    }
+
+    /**
+     * @return array<Content>
+     */
+    private function prepareContentStructure(): array
+    {
+        $folder = $this->createFolder(['eng-GB' => 'Folder'], 2);
+        $folderMainLocationId = $folder->getVersionInfo()->getContentInfo()->getMainLocationId();
+        Assert::assertIsNumeric($folderMainLocationId);
+
+        $childFolder = $this->createFolder(
+            ['eng-GB' => 'Child folder'],
+            $folderMainLocationId,
+        );
+        $childFolderMainLocationId = $childFolder->getVersionInfo()->getContentInfo()->getMainLocationId();
+        Assert::assertIsNumeric($childFolderMainLocationId);
+
+        $secondDepthChildFolder = $this->createFolder(
+            ['eng-GB' => 'Second depth folder'],
+            $childFolderMainLocationId,
+        );
+        $secondDepthChildFolderLocationId = $secondDepthChildFolder
+            ->getVersionInfo()
+            ->getContentInfo()
+            ->getMainLocationId()
+        ;
+        Assert::assertIsNumeric($secondDepthChildFolderLocationId);
+
+        $draft1 = $this->createFolderDraft(['eng-GB' => 'Folder draft 1'], $folderMainLocationId);
+        $draft2 = $this->createFolderDraft(['eng-GB' => 'Folder draft 2'], $childFolderMainLocationId);
+        $draft3 = $this->createFolderDraft(['eng-GB' => 'Folder draft 3'], $childFolderMainLocationId);
+        $draftSecondDepth = $this->createFolderDraft(
+            ['eng-GB' => 'Folder draft 4'],
+            $secondDepthChildFolderLocationId,
+        );
+
+        return [
+            $folder,
+            $draft1,
+            $draft2,
+            $draft3,
+            $draftSecondDepth,
+        ];
+    }
+}
diff --git a/tests/lib/Persistence/Cache/LocationHandlerTest.php b/tests/lib/Persistence/Cache/LocationHandlerTest.php
@@ -67,6 +67,7 @@ public function providerForUnCachedMethods(): array
                 ['c-4', 'ragl-4'],
             ],
             ['removeSubtree', [12], [['location_path', [12], false]], null, ['lp-12']],
+            ['deleteChildrenDrafts', [12], [['location_path', [12], false]], null, ['lp-12']],
             ['setSectionForSubtree', [12, 2], [['location_path', [12], false]], null, ['lp-12']],
             ['changeMainLocation', [4, 12], [['content', [4], false]], null, ['c-4']],
             ['countLocationsByContent', [4]],
diff --git a/tests/lib/Persistence/Legacy/Content/LocationHandlerTest.php b/tests/lib/Persistence/Legacy/Content/LocationHandlerTest.php
@@ -453,6 +453,18 @@ public function testRemoveSubtree()
         $handler->removeSubtree(42);
     }
 
+    public function testDeleteChildrenDrafts(): void
+    {
+        $handler = $this->getLocationHandler();
+
+        $this->treeHandler
+            ->expects(self::once())
+            ->method('deleteChildrenDrafts')
+            ->with(42);
+
+        $handler->deleteChildrenDrafts(42);
+    }
+
     /**
      * Test for the copySubtree() method.
      */
diff --git a/tests/lib/Persistence/Legacy/Content/TreeHandlerTest.php b/tests/lib/Persistence/Legacy/Content/TreeHandlerTest.php

Original file line number	Diff line number	Diff line change
`@@ -650,6 +650,7 @@ public function deleteContent($contentId)`
`650`	`650`	`$this->removeRawContent($contentId);`
`651`	`651`	`} else {`
`652`	`652`	`foreach ($contentLocations as $locationId) {`
	`653`	`+ $this->treeHandler->deleteChildrenDrafts($locationId);`
`653`	`654`	`$this->treeHandler->removeSubtree($locationId);`
`654`	`655`	`}`
`655`	`656`	`}`
Original file line number	Diff line number	Diff line change
`@@ -106,6 +106,18 @@ public function getSubtreeContent(int $sourceId, bool $onlyIds = false): array`
`106`	`106`	`}`
`107`	`107`	`}`
`108`	`108`
	`109`	`+ /**`
	`110`	`+ * @return array<int>`
	`111`	`+ */`
	`112`	`+ public function getSubtreeChildrenDraftContentIds(int $sourceId): array`
	`113`	`+ {`
	`114`	`+ try {`
	`115`	`+ return $this->innerGateway->getSubtreeChildrenDraftContentIds($sourceId);`
	`116`	`+ } catch (PDOException $e) {`
	`117`	`+ throw DatabaseException::wrap($e);`
	`118`	`+ }`
	`119`	`+ }`
	`120`	`+`
`109`	`121`	`public function getSubtreeSize(string $path): int`
`110`	`122`	`{`
`111`	`123`	`try {`
Original file line number	Diff line number	Diff line change
`@@ -546,6 +546,11 @@ public function removeSubtree($locationId)`
`546`	`546`	`$this->treeHandler->removeSubtree($locationId);`
`547`	`547`	`}`
`548`	`548`
	`549`	`+ public function deleteChildrenDrafts(int $locationId): void`
	`550`	`+ {`
	`551`	`+ $this->treeHandler->deleteChildrenDrafts($locationId);`
	`552`	`+ }`
	`553`	`+`
`549`	`554`	`/**`
`550`	`555`	`* Set section on all content objects in the subtree.`
`551`	`556`	`*`