Merge pull request #52589 from nextcloud/fix/dav/orphan-cleanup-job

fix(dav): move orphan cleaning logic to a chunked background job
2026-05-28 04:32:30 -04:00 · 2025-05-05 12:32:14 +02:00 · 2025-05-05 12:32:14 +02:00 · 4783459144
commit 4783459144
parent 5644f33b44 d0fcb7e638
5 changed files with 287 additions and 62 deletions
--- a/apps/dav/composer/composer/autoload_classmap.php
+++ b/apps/dav/composer/composer/autoload_classmap.php
@ -16,6 +16,7 @@ return array(
    'OCA\\DAV\\BackgroundJob\\CalendarRetentionJob' => $baseDir . '/../lib/BackgroundJob/CalendarRetentionJob.php',
    'OCA\\DAV\\BackgroundJob\\CleanupDirectLinksJob' => $baseDir . '/../lib/BackgroundJob/CleanupDirectLinksJob.php',
    'OCA\\DAV\\BackgroundJob\\CleanupInvitationTokenJob' => $baseDir . '/../lib/BackgroundJob/CleanupInvitationTokenJob.php',
+    'OCA\\DAV\\BackgroundJob\\CleanupOrphanedChildrenJob' => $baseDir . '/../lib/BackgroundJob/CleanupOrphanedChildrenJob.php',
    'OCA\\DAV\\BackgroundJob\\DeleteOutdatedSchedulingObjects' => $baseDir . '/../lib/BackgroundJob/DeleteOutdatedSchedulingObjects.php',
    'OCA\\DAV\\BackgroundJob\\EventReminderJob' => $baseDir . '/../lib/BackgroundJob/EventReminderJob.php',
    'OCA\\DAV\\BackgroundJob\\GenerateBirthdayCalendarBackgroundJob' => $baseDir . '/../lib/BackgroundJob/GenerateBirthdayCalendarBackgroundJob.php',
--- a/apps/dav/composer/composer/autoload_static.php
+++ b/apps/dav/composer/composer/autoload_static.php
@ -31,6 +31,7 @@ class ComposerStaticInitDAV
        'OCA\\DAV\\BackgroundJob\\CalendarRetentionJob' => __DIR__ . '/..' . '/../lib/BackgroundJob/CalendarRetentionJob.php',
        'OCA\\DAV\\BackgroundJob\\CleanupDirectLinksJob' => __DIR__ . '/..' . '/../lib/BackgroundJob/CleanupDirectLinksJob.php',
        'OCA\\DAV\\BackgroundJob\\CleanupInvitationTokenJob' => __DIR__ . '/..' . '/../lib/BackgroundJob/CleanupInvitationTokenJob.php',
+        'OCA\\DAV\\BackgroundJob\\CleanupOrphanedChildrenJob' => __DIR__ . '/..' . '/../lib/BackgroundJob/CleanupOrphanedChildrenJob.php',
        'OCA\\DAV\\BackgroundJob\\DeleteOutdatedSchedulingObjects' => __DIR__ . '/..' . '/../lib/BackgroundJob/DeleteOutdatedSchedulingObjects.php',
        'OCA\\DAV\\BackgroundJob\\EventReminderJob' => __DIR__ . '/..' . '/../lib/BackgroundJob/EventReminderJob.php',
        'OCA\\DAV\\BackgroundJob\\GenerateBirthdayCalendarBackgroundJob' => __DIR__ . '/..' . '/../lib/BackgroundJob/GenerateBirthdayCalendarBackgroundJob.php',
--- a/apps/dav/lib/BackgroundJob/CleanupOrphanedChildrenJob.php
+++ b/apps/dav/lib/BackgroundJob/CleanupOrphanedChildrenJob.php
@ -0,0 +1,89 @@
+<?php
+
+declare(strict_types=1);
+
+/**
+ * SPDX-FileCopyrightText: 2025 Nextcloud GmbH and Nextcloud contributors
+ * SPDX-License-Identifier: AGPL-3.0-or-later
+ */
+
+namespace OCA\DAV\BackgroundJob;
+
+use OCA\DAV\CalDAV\CalDavBackend;
+use OCP\AppFramework\Utility\ITimeFactory;
+use OCP\BackgroundJob\IJobList;
+use OCP\BackgroundJob\QueuedJob;
+use OCP\DB\QueryBuilder\IQueryBuilder;
+use OCP\IDBConnection;
+use Psr\Log\LoggerInterface;
+
+class CleanupOrphanedChildrenJob extends QueuedJob {
+	public const ARGUMENT_CHILD_TABLE = 'childTable';
+	public const ARGUMENT_PARENT_TABLE = 'parentTable';
+	public const ARGUMENT_PARENT_ID = 'parentId';
+	public const ARGUMENT_LOG_MESSAGE = 'logMessage';
+
+	private const BATCH_SIZE = 1000;
+
+	public function __construct(
+		ITimeFactory $time,
+		private readonly IDBConnection $connection,
+		private readonly LoggerInterface $logger,
+		private readonly IJobList $jobList,
+	) {
+		parent::__construct($time);
+	}
+
+	protected function run($argument): void {
+		$childTable = $argument[self::ARGUMENT_CHILD_TABLE];
+		$parentTable = $argument[self::ARGUMENT_PARENT_TABLE];
+		$parentId = $argument[self::ARGUMENT_PARENT_ID];
+		$logMessage = $argument[self::ARGUMENT_LOG_MESSAGE];
+
+		$orphanCount = $this->cleanUpOrphans($childTable, $parentTable, $parentId);
+		$this->logger->debug(sprintf($logMessage, $orphanCount));
+
+		// Requeue if there might be more orphans
+		if ($orphanCount >= self::BATCH_SIZE) {
+			$this->jobList->add(self::class, $argument);
+		}
+	}
+
+	private function cleanUpOrphans(
+		string $childTable,
+		string $parentTable,
+		string $parentId,
+	): int {
+		// We can't merge both queries into a single one here as DELETEing from a table while
+		// SELECTing it in a sub query is not supported by Oracle DB.
+		// Ref https://docs.oracle.com/cd/E17952_01/mysql-8.0-en/delete.html#idm46006185488144
+
+		$selectQb = $this->connection->getQueryBuilder();
+
+		$selectQb->select('c.id')
+			->from($childTable, 'c')
+			->leftJoin('c', $parentTable, 'p', $selectQb->expr()->eq('c.' . $parentId, 'p.id'))
+			->where($selectQb->expr()->isNull('p.id'))
+			->setMaxResults(self::BATCH_SIZE);
+
+		if (\in_array($parentTable, ['calendars', 'calendarsubscriptions'], true)) {
+			$calendarType = $parentTable === 'calendarsubscriptions' ? CalDavBackend::CALENDAR_TYPE_SUBSCRIPTION : CalDavBackend::CALENDAR_TYPE_CALENDAR;
+			$selectQb->andWhere($selectQb->expr()->eq('c.calendartype', $selectQb->createNamedParameter($calendarType, IQueryBuilder::PARAM_INT), IQueryBuilder::PARAM_INT));
+		}
+
+		$result = $selectQb->executeQuery();
+		$rows = $result->fetchAll();
+		$result->closeCursor();
+		if (empty($rows)) {
+			return 0;
+		}
+
+		$orphanItems = array_map(static fn ($row) => $row['id'], $rows);
+		$deleteQb = $this->connection->getQueryBuilder();
+		$deleteQb->delete($childTable)
+			->where($deleteQb->expr()->in('id', $deleteQb->createNamedParameter($orphanItems, IQueryBuilder::PARAM_INT_ARRAY)));
+		$deleteQb->executeStatement();
+
+		return count($orphanItems);
+	}
+}
--- a/apps/dav/lib/Migration/RemoveOrphanEventsAndContacts.php
+++ b/apps/dav/lib/Migration/RemoveOrphanEventsAndContacts.php
@ -8,82 +8,45 @@ declare(strict_types=1);
 */
 namespace OCA\DAV\Migration;

-use OCA\DAV\CalDAV\CalDavBackend;
-use OCP\DB\QueryBuilder\IQueryBuilder;
-use OCP\IDBConnection;
+use OCA\DAV\BackgroundJob\CleanupOrphanedChildrenJob;
+use OCP\BackgroundJob\IJobList;
 use OCP\Migration\IOutput;
 use OCP\Migration\IRepairStep;

 class RemoveOrphanEventsAndContacts implements IRepairStep {
-
 	public function __construct(
-		private IDBConnection $connection,
+		private readonly IJobList $jobList,
 	) {
 	}

-	/**
-	 * @inheritdoc
-	 */
 	public function getName(): string {
-		return 'Clean up orphan event and contact data';
+		return 'Queue jobs to clean up orphan event and contact data';
 	}

-	/**
-	 * @inheritdoc
-	 */
-	public function run(IOutput $output) {
-		$orphanItems = $this->removeOrphanChildren('calendarobjects', 'calendars', 'calendarid');
-		$output->info(sprintf('%d events without a calendar have been cleaned up', $orphanItems));
-		$orphanItems = $this->removeOrphanChildren('calendarobjects_props', 'calendarobjects', 'objectid');
-		$output->info(sprintf('%d properties without an events have been cleaned up', $orphanItems));
-		$orphanItems = $this->removeOrphanChildren('calendarchanges', 'calendars', 'calendarid');
-		$output->info(sprintf('%d changes without a calendar have been cleaned up', $orphanItems));
+	public function run(IOutput $output): void {
+		$this->queueJob('calendarobjects', 'calendars', 'calendarid', '%d events without a calendar have been cleaned up');
+		$this->queueJob('calendarobjects_props', 'calendarobjects', 'objectid', '%d properties without an events have been cleaned up');
+		$this->queueJob('calendarchanges', 'calendars', 'calendarid', '%d changes without a calendar have been cleaned up');

-		$orphanItems = $this->removeOrphanChildren('calendarobjects', 'calendarsubscriptions', 'calendarid');
-		$output->info(sprintf('%d cached events without a calendar subscription have been cleaned up', $orphanItems));
-		$orphanItems = $this->removeOrphanChildren('calendarchanges', 'calendarsubscriptions', 'calendarid');
-		$output->info(sprintf('%d changes without a calendar subscription have been cleaned up', $orphanItems));
+		$this->queueJob('calendarobjects', 'calendarsubscriptions', 'calendarid', '%d cached events without a calendar subscription have been cleaned up');
+		$this->queueJob('calendarchanges', 'calendarsubscriptions', 'calendarid', '%d changes without a calendar subscription have been cleaned up');

-		$orphanItems = $this->removeOrphanChildren('cards', 'addressbooks', 'addressbookid');
-		$output->info(sprintf('%d contacts without an addressbook have been cleaned up', $orphanItems));
-		$orphanItems = $this->removeOrphanChildren('cards_properties', 'cards', 'cardid');
-		$output->info(sprintf('%d properties without a contact have been cleaned up', $orphanItems));
-		$orphanItems = $this->removeOrphanChildren('addressbookchanges', 'addressbooks', 'addressbookid');
-		$output->info(sprintf('%d changes without an addressbook have been cleaned up', $orphanItems));
+		$this->queueJob('cards', 'addressbooks', 'addressbookid', '%d contacts without an addressbook have been cleaned up');
+		$this->queueJob('cards_properties', 'cards', 'cardid', '%d properties without a contact have been cleaned up');
+		$this->queueJob('addressbookchanges', 'addressbooks', 'addressbookid', '%d changes without an addressbook have been cleaned up');
 	}

-	protected function removeOrphanChildren($childTable, $parentTable, $parentId): int {
-		$qb = $this->connection->getQueryBuilder();
-
-		$qb->select('c.id')
-			->from($childTable, 'c')
-			->leftJoin('c', $parentTable, 'p', $qb->expr()->eq('c.' . $parentId, 'p.id'))
-			->where($qb->expr()->isNull('p.id'));
-
-		if (\in_array($parentTable, ['calendars', 'calendarsubscriptions'], true)) {
-			$calendarType = $parentTable === 'calendarsubscriptions' ? CalDavBackend::CALENDAR_TYPE_SUBSCRIPTION : CalDavBackend::CALENDAR_TYPE_CALENDAR;
-			$qb->andWhere($qb->expr()->eq('c.calendartype', $qb->createNamedParameter($calendarType, IQueryBuilder::PARAM_INT), IQueryBuilder::PARAM_INT));
-		}
-
-		$result = $qb->executeQuery();
-
-		$orphanItems = [];
-		while ($row = $result->fetch()) {
-			$orphanItems[] = (int)$row['id'];
-		}
-		$result->closeCursor();
-
-		if (!empty($orphanItems)) {
-			$qb->delete($childTable)
-				->where($qb->expr()->in('id', $qb->createParameter('ids')));
-
-			$orphanItemsBatch = array_chunk($orphanItems, 1000);
-			foreach ($orphanItemsBatch as $items) {
-				$qb->setParameter('ids', $items, IQueryBuilder::PARAM_INT_ARRAY);
-				$qb->executeStatement();
-			}
-		}
-
-		return count($orphanItems);
+	private function queueJob(
+		string $childTable,
+		string $parentTable,
+		string $parentId,
+		string $logMessage,
+	): void {
+		$this->jobList->add(CleanupOrphanedChildrenJob::class, [
+			CleanupOrphanedChildrenJob::ARGUMENT_CHILD_TABLE => $childTable,
+			CleanupOrphanedChildrenJob::ARGUMENT_PARENT_TABLE => $parentTable,
+			CleanupOrphanedChildrenJob::ARGUMENT_PARENT_ID => $parentId,
+			CleanupOrphanedChildrenJob::ARGUMENT_LOG_MESSAGE => $logMessage,
+		]);
 	}
 }
--- a/apps/dav/tests/unit/BackgroundJob/CleanupOrphanedChildrenJobTest.php
+++ b/apps/dav/tests/unit/BackgroundJob/CleanupOrphanedChildrenJobTest.php
@ -0,0 +1,171 @@
+<?php
+
+declare(strict_types=1);
+
+/**
+ * SPDX-FileCopyrightText: 2025 Nextcloud GmbH and Nextcloud contributors
+ * SPDX-License-Identifier: AGPL-3.0-or-later
+ */
+
+namespace OCA\DAV\Tests\unit\BackgroundJob;
+
+use OCA\DAV\BackgroundJob\CleanupOrphanedChildrenJob;
+use OCP\AppFramework\Utility\ITimeFactory;
+use OCP\BackgroundJob\IJobList;
+use OCP\DB\IResult;
+use OCP\DB\QueryBuilder\IExpressionBuilder;
+use OCP\DB\QueryBuilder\IQueryBuilder;
+use OCP\IDBConnection;
+use PHPUnit\Framework\MockObject\MockObject;
+use Psr\Log\LoggerInterface;
+use Test\TestCase;
+
+class CleanupOrphanedChildrenJobTest extends TestCase {
+	private CleanupOrphanedChildrenJob $job;
+
+	private ITimeFactory&MockObject $timeFactory;
+	private IDBConnection&MockObject $connection;
+	private LoggerInterface&MockObject $logger;
+	private IJobList&MockObject $jobList;
+
+	protected function setUp(): void {
+		parent::setUp();
+
+		$this->timeFactory = $this->createMock(ITimeFactory::class);
+		$this->connection = $this->createMock(IDBConnection::class);
+		$this->logger = $this->createMock(LoggerInterface::class);
+		$this->jobList = $this->createMock(IJobList::class);
+
+		$this->job = new CleanupOrphanedChildrenJob(
+			$this->timeFactory,
+			$this->connection,
+			$this->logger,
+			$this->jobList,
+		);
+	}
+
+	private function getArgument(): array {
+		return [
+			'childTable' => 'childTable',
+			'parentTable' => 'parentTable',
+			'parentId' => 'parentId',
+			'logMessage' => 'logMessage',
+		];
+	}
+
+	private function getMockQueryBuilder(): IQueryBuilder&MockObject {
+		$expr = $this->createMock(IExpressionBuilder::class);
+		$qb = $this->createMock(IQueryBuilder::class);
+		$qb->method('select')
+			->willReturnSelf();
+		$qb->method('from')
+			->willReturnSelf();
+		$qb->method('leftJoin')
+			->willReturnSelf();
+		$qb->method('where')
+			->willReturnSelf();
+		$qb->method('setMaxResults')
+			->willReturnSelf();
+		$qb->method('andWhere')
+			->willReturnSelf();
+		$qb->method('expr')
+			->willReturn($expr);
+		$qb->method('delete')
+			->willReturnSelf();
+		return $qb;
+	}
+
+	public function testRunWithoutOrphans(): void {
+		$argument = $this->getArgument();
+		$selectQb = $this->getMockQueryBuilder();
+		$result = $this->createMock(IResult::class);
+
+		$this->connection->expects(self::once())
+			->method('getQueryBuilder')
+			->willReturn($selectQb);
+		$selectQb->expects(self::once())
+			->method('executeQuery')
+			->willReturn($result);
+		$result->expects(self::once())
+			->method('fetchAll')
+			->willReturn([]);
+		$result->expects(self::once())
+			->method('closeCursor');
+		$this->jobList->expects(self::never())
+			->method('add');
+
+		self::invokePrivate($this->job, 'run', [$argument]);
+	}
+
+	public function testRunWithPartialBatch(): void {
+		$argument = $this->getArgument();
+		$selectQb = $this->getMockQueryBuilder();
+		$deleteQb = $this->getMockQueryBuilder();
+		$result = $this->createMock(IResult::class);
+
+		$qbInvocationCount = self::exactly(2);
+		$this->connection->expects($qbInvocationCount)
+			->method('getQueryBuilder')
+			->willReturnCallback(function () use ($qbInvocationCount, $selectQb, $deleteQb) {
+				return match ($qbInvocationCount->getInvocationCount()) {
+					1 => $selectQb,
+					2 => $deleteQb,
+				};
+			});
+		$selectQb->expects(self::once())
+			->method('executeQuery')
+			->willReturn($result);
+		$result->expects(self::once())
+			->method('fetchAll')
+			->willReturn([
+				['id' => 42],
+				['id' => 43],
+			]);
+		$result->expects(self::once())
+			->method('closeCursor');
+		$deleteQb->expects(self::once())
+			->method('delete')
+			->willReturnSelf();
+		$deleteQb->expects(self::once())
+			->method('executeStatement');
+		$this->jobList->expects(self::never())
+			->method('add');
+
+		self::invokePrivate($this->job, 'run', [$argument]);
+	}
+
+	public function testRunWithFullBatch(): void {
+		$argument = $this->getArgument();
+		$selectQb = $this->getMockQueryBuilder();
+		$deleteQb = $this->getMockQueryBuilder();
+		$result = $this->createMock(IResult::class);
+
+		$qbInvocationCount = self::exactly(2);
+		$this->connection->expects($qbInvocationCount)
+			->method('getQueryBuilder')
+			->willReturnCallback(function () use ($qbInvocationCount, $selectQb, $deleteQb) {
+				return match ($qbInvocationCount->getInvocationCount()) {
+					1 => $selectQb,
+					2 => $deleteQb,
+				};
+			});
+		$selectQb->expects(self::once())
+			->method('executeQuery')
+			->willReturn($result);
+		$result->expects(self::once())
+			->method('fetchAll')
+			->willReturn(array_map(static fn ($i) => ['id' => 42 + $i], range(0, 999)));
+		$result->expects(self::once())
+			->method('closeCursor');
+		$deleteQb->expects(self::once())
+			->method('delete')
+			->willReturnSelf();
+		$deleteQb->expects(self::once())
+			->method('executeStatement');
+		$this->jobList->expects(self::once())
+			->method('add')
+			->with(CleanupOrphanedChildrenJob::class, $argument);
+
+		self::invokePrivate($this->job, 'run', [$argument]);
+	}
+}