diff options
author | Richard Steinmetz <richard@steinmetz.cloud> | 2025-05-02 13:08:23 +0200 |
---|---|---|
committer | Richard Steinmetz <richard@steinmetz.cloud> | 2025-05-07 13:28:53 +0200 |
commit | 9a41fe993e9f2310242b9d5d572e660068dda24e (patch) | |
tree | fd2dac6e22167d878ee7b3c23b49b8e81c84dbff | |
parent | 8f6fe594860770c5bcbcbf99e49313d9137af671 (diff) | |
download | nextcloud-server-backport/52589/stable30.tar.gz nextcloud-server-backport/52589/stable30.zip |
fix(dav): move orphan cleaning logic to a chunked background jobbackport/52589/stable30
Signed-off-by: Richard Steinmetz <richard@steinmetz.cloud>
5 files changed, 289 insertions, 66 deletions
diff --git a/apps/dav/composer/composer/autoload_classmap.php b/apps/dav/composer/composer/autoload_classmap.php index 324738bc45a..a8677c08b5e 100644 --- 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', diff --git a/apps/dav/composer/composer/autoload_static.php b/apps/dav/composer/composer/autoload_static.php index cd3fff1eaee..85d574165d2 100644 --- 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', diff --git a/apps/dav/lib/BackgroundJob/CleanupOrphanedChildrenJob.php b/apps/dav/lib/BackgroundJob/CleanupOrphanedChildrenJob.php new file mode 100644 index 00000000000..8a5e34381a7 --- /dev/null +++ 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); + } +} diff --git a/apps/dav/lib/Migration/RemoveOrphanEventsAndContacts.php b/apps/dav/lib/Migration/RemoveOrphanEventsAndContacts.php index 789cd70afc1..143dc3cd1e6 100644 --- a/apps/dav/lib/Migration/RemoveOrphanEventsAndContacts.php +++ b/apps/dav/lib/Migration/RemoveOrphanEventsAndContacts.php @@ -8,84 +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 { - - /** @var IDBConnection */ - private $connection; - - public function __construct(IDBConnection $connection) { - $this->connection = $connection; + public function __construct( + 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->execute(); - - $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->execute(); - } - } - - 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, + ]); } } diff --git a/apps/dav/tests/unit/BackgroundJob/CleanupOrphanedChildrenJobTest.php b/apps/dav/tests/unit/BackgroundJob/CleanupOrphanedChildrenJobTest.php new file mode 100644 index 00000000000..fe05616d609 --- /dev/null +++ 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]); + } +} |