From d125f61e94f6975102d6d9f1c28d359623260c82 Mon Sep 17 00:00:00 2001 From: Carl Schwan Date: Fri, 5 Jun 2026 12:06:48 +0200 Subject: [PATCH 1/4] fix(preview): First cleanup from filecache and then from preview table Signed-off-by: Carl Schwan --- core/Command/Preview/Cleanup.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/core/Command/Preview/Cleanup.php b/core/Command/Preview/Cleanup.php index 56508c3f0f024..ffed7a65ccd4a 100644 --- a/core/Command/Preview/Cleanup.php +++ b/core/Command/Preview/Cleanup.php @@ -39,11 +39,11 @@ protected function configure(): void { #[\Override] protected function execute(InputInterface $input, OutputInterface $output): int { - if ($this->deletePreviewFromPreviewTable($output) !== 0) { + if ($this->deletePreviewFromFileCacheTable($output) !== 0) { return 1; } - return $this->deletePreviewFromFileCacheTable($output); + return $this->deletePreviewFromPreviewTable($output); } /** From c00f82aa826f6c35ba5642ac70dbd5946c9d7c7a Mon Sep 17 00:00:00 2001 From: Carl Schwan Date: Mon, 8 Jun 2026 10:58:05 +0200 Subject: [PATCH 2/4] fix(preview): Don't reuse same query builder for delete query Signed-off-by: Carl Schwan --- lib/private/Preview/PreviewMigrationService.php | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/private/Preview/PreviewMigrationService.php b/lib/private/Preview/PreviewMigrationService.php index 6b67b8334772d..220a52baddb91 100644 --- a/lib/private/Preview/PreviewMigrationService.php +++ b/lib/private/Preview/PreviewMigrationService.php @@ -109,9 +109,10 @@ public function migrateFileId(int $fileId, bool $flatPath): array { try { $preview = $this->previewMapper->insert($preview); } catch (Exception) { + $delete = $this->connection->getQueryBuilder(); // We already have this preview in the preview table, skip - $qb->delete('filecache') - ->where($qb->expr()->eq('fileid', $qb->createNamedParameter($file->getId()))) + $delete->delete('filecache') + ->where($delete->expr()->eq('fileid', $delete->createNamedParameter($file->getId()))) ->hintShardKey('storage', $this->rootFolder->getMountPoint()->getNumericStorageId()) ->executeStatement(); continue; From fc4938c3ed482411ebebdcef26f26a1a4e7f3dac Mon Sep 17 00:00:00 2001 From: Carl Schwan Date: Mon, 8 Jun 2026 11:08:02 +0200 Subject: [PATCH 3/4] fix(preview): Better handle errors while migrating previews Signed-off-by: Carl Schwan --- core/BackgroundJobs/PreviewMigrationJob.php | 18 ++++++++++++++++-- .../Preview/PreviewMigrationService.php | 17 +++++++++++++---- tests/lib/Preview/PreviewMigrationJobTest.php | 9 ++++++--- 3 files changed, 35 insertions(+), 9 deletions(-) diff --git a/core/BackgroundJobs/PreviewMigrationJob.php b/core/BackgroundJobs/PreviewMigrationJob.php index a9a5c9f773c29..5a8f9fffbccac 100644 --- a/core/BackgroundJobs/PreviewMigrationJob.php +++ b/core/BackgroundJobs/PreviewMigrationJob.php @@ -19,6 +19,7 @@ use OCP\IConfig; use OCP\IDBConnection; use Override; +use Psr\Log\LoggerInterface; class PreviewMigrationJob extends TimedJob { private string $previewRootPath; @@ -30,6 +31,7 @@ public function __construct( private readonly IDBConnection $connection, private readonly IRootFolder $rootFolder, private readonly PreviewMigrationService $migrationService, + private readonly LoggerInterface $logger, ) { parent::__construct($time); @@ -95,11 +97,23 @@ private function processQueryResult(IResult $result): bool { } foreach ($fileIds as $fileId) { - $this->migrationService->migrateFileId($fileId, flatPath: false); + try { + $this->migrationService->migrateFileId($fileId, flatPath: false); + } catch (\Exception $e) { + $this->logger->error('Failed to migrate preview with fileId: ' . $fileId . ' (hierarchical file structure)', [ + 'exception' => $e, + ]); + } } foreach ($flatFileIds as $fileId) { - $this->migrationService->migrateFileId($fileId, flatPath: true); + try { + $this->migrationService->migrateFileId($fileId, flatPath: true); + } catch (\Exception $e) { + $this->logger->error('Failed to migrate preview with fileId: ' . $fileId . ' (legacy file structure)', [ + 'exception' => $e, + ]); + } } return $foundPreview; } diff --git a/lib/private/Preview/PreviewMigrationService.php b/lib/private/Preview/PreviewMigrationService.php index 220a52baddb91..555bd3884474b 100644 --- a/lib/private/Preview/PreviewMigrationService.php +++ b/lib/private/Preview/PreviewMigrationService.php @@ -93,8 +93,9 @@ public function migrateFileId(int $fileId, bool $flatPath): array { ->where($qb->expr()->eq('fileid', $qb->createNamedParameter($fileId))) ->setMaxResults(1); - $result = $qb->executeQuery(); - $result = $result->fetchAssociative(); + $cursor = $qb->executeQuery(); + $result = $cursor->fetchAssociative(); + $cursor->closeCursor(); if ($result !== false) { foreach ($previewFiles as $previewFile) { @@ -108,7 +109,11 @@ public function migrateFileId(int $fileId, bool $flatPath): array { $preview->generateId(); try { $preview = $this->previewMapper->insert($preview); - } catch (Exception) { + } catch (Exception $e) { + if ($e->getReason() !== Exception::REASON_UNIQUE_CONSTRAINT_VIOLATION) { + throw $e; + } + $delete = $this->connection->getQueryBuilder(); // We already have this preview in the preview table, skip $delete->delete('filecache') @@ -180,7 +185,11 @@ private function deleteFolder(string $path): void { break; } - $folder = $this->appData->getFolder($current); + try { + $folder = $this->appData->getFolder($current); + } catch (NotFoundException) { + break; + } if (count($folder->getDirectoryListing()) !== 0) { break; } diff --git a/tests/lib/Preview/PreviewMigrationJobTest.php b/tests/lib/Preview/PreviewMigrationJobTest.php index 2f769d79d24ed..fe9cd96879959 100644 --- a/tests/lib/Preview/PreviewMigrationJobTest.php +++ b/tests/lib/Preview/PreviewMigrationJobTest.php @@ -131,7 +131,8 @@ public function testMigrationLegacyPath(): void { $this->previewMapper, $this->storageFactory, Server::get(IAppDataFactory::class), - ) + ), + $this->logger, ); $this->invokePrivate($job, 'run', [[]]); $this->assertEquals(0, count($this->previewAppData->getDirectoryListing())); @@ -168,7 +169,8 @@ public function testMigrationPath(): void { $this->previewMapper, $this->storageFactory, Server::get(IAppDataFactory::class), - ) + ), + $this->logger, ); $this->invokePrivate($job, 'run', [[]]); $this->assertEquals(0, count($this->previewAppData->getDirectoryListing())); @@ -213,7 +215,8 @@ public function testMigrationPathWithVersion(): void { $this->previewMapper, $this->storageFactory, Server::get(IAppDataFactory::class), - ) + ), + $this->logger, ); $this->invokePrivate($job, 'run', [[]]); $previews = iterator_to_array($this->previewMapper->getAvailablePreviewsForFile(5)); From acfe107fc950c714f8dee806b181354c58507b98 Mon Sep 17 00:00:00 2001 From: Carl Schwan Date: Mon, 8 Jun 2026 11:22:49 +0200 Subject: [PATCH 4/4] fix(preview): Adapt cleanup tests Now the logic is inverted so the tests need to be changed Signed-off-by: Carl Schwan --- tests/Core/Command/Preview/CleanupTest.php | 39 +++++++++++++++++----- 1 file changed, 30 insertions(+), 9 deletions(-) diff --git a/tests/Core/Command/Preview/CleanupTest.php b/tests/Core/Command/Preview/CleanupTest.php index 84b0068842f6d..f321ac719c2cb 100644 --- a/tests/Core/Command/Preview/CleanupTest.php +++ b/tests/Core/Command/Preview/CleanupTest.php @@ -81,8 +81,6 @@ public function testCleanup(): void { } public function testCleanupWhenNotDeletable(): void { - $this->previewService->expects($this->once())->method('deleteAll'); - $previewFolder = $this->createMock(Folder::class); $previewFolder->expects($this->once()) ->method('isDeletable') @@ -112,8 +110,6 @@ public function testCleanupWhenNotDeletable(): void { #[\PHPUnit\Framework\Attributes\DataProvider('dataForTestCleanupWithDeleteException')] public function testCleanupWithDeleteException(string $exceptionClass, string $errorMessage): void { - $this->previewService->expects($this->once())->method('deleteAll'); - $previewFolder = $this->createMock(Folder::class); $previewFolder->expects($this->once()) ->method('isDeletable') @@ -150,8 +146,6 @@ public static function dataForTestCleanupWithDeleteException(): array { } public function testCleanupWithCreateException(): void { - $this->previewService->expects($this->once())->method('deleteAll'); - $previewFolder = $this->createMock(Folder::class); $previewFolder->expects($this->once()) ->method('isDeletable') @@ -188,14 +182,41 @@ public function testCleanupWithCreateException(): void { } public function testCleanupWithPreviewServiceException(): void { + $previewFolder = $this->createMock(Folder::class); + $previewFolder->expects($this->once()) + ->method('isDeletable') + ->willReturn(true); + + $previewFolder->expects($this->once()) + ->method('delete'); + + $appDataFolder = $this->createMock(Folder::class); + $appDataFolder->expects($this->once())->method('get')->with('preview')->willReturn($previewFolder); + $appDataFolder->expects($this->once())->method('newFolder')->with('preview'); + + $this->rootFolder->method('getAppDataDirectoryName') + ->willReturn('appdata_some_id'); + + $this->rootFolder->method('get') + ->with('appdata_some_id') + ->willReturn($appDataFolder); + + $this->output->expects($this->exactly(3))->method('writeln') + ->with(self::callback(function (string $message): bool { + static $i = 0; + return match (++$i) { + 1 => $message === 'Preview folder deleted', + 2 => $message === 'Preview folder recreated', + 3 => $message === 'Previews removed' + }; + })); + + $this->assertEquals(0, $this->repair->run($this->input, $this->output)); $this->previewService->expects($this->once())->method('deleteAll') ->willThrowException(new NotPermittedException('abc')); $this->logger->expects($this->once())->method('error')->with("Previews can't be removed: exception occurred: abc"); - $this->rootFolder->expects($this->never()) - ->method('get'); - $this->assertEquals(1, $this->repair->run($this->input, $this->output)); } }