From d2ce8f0244ea21b79da80e297c83ff9cfef336f6 Mon Sep 17 00:00:00 2001 From: Carl Schwan Date: Fri, 5 Jun 2026 12:06:48 +0200 Subject: [PATCH 1/6] 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 e9d0beb57664e2398d722d05a685e1ef1d6986e7 Mon Sep 17 00:00:00 2001 From: Carl Schwan Date: Mon, 8 Jun 2026 10:58:05 +0200 Subject: [PATCH 2/6] 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 cba205b4c2eed5b2fd1a21f5c3ca8da209c0cf7b Mon Sep 17 00:00:00 2001 From: Carl Schwan Date: Mon, 8 Jun 2026 11:08:02 +0200 Subject: [PATCH 3/6] 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 c0277dafb08490774ae32019f86162c3fdeb794e Mon Sep 17 00:00:00 2001 From: Carl Schwan Date: Mon, 8 Jun 2026 11:22:49 +0200 Subject: [PATCH 4/6] 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 5d9d51026a2b6..e0050c5a9057a 100644 --- a/tests/Core/Command/Preview/CleanupTest.php +++ b/tests/Core/Command/Preview/CleanupTest.php @@ -80,8 +80,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') @@ -111,8 +109,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') @@ -149,8 +145,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') @@ -187,14 +181,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)); } } From 58753203f5a5375d00d5adffb0a9d68a543f9af3 Mon Sep 17 00:00:00 2001 From: Carl Schwan Date: Tue, 9 Jun 2026 11:14:39 +0200 Subject: [PATCH 5/6] fix(preview): Do not recreate preview folder in filecache after cleanup Signed-off-by: Carl Schwan --- core/Command/Preview/Cleanup.php | 10 ---- .../Attributes/IndexMigrationAttribute.php | 4 +- tests/Core/Command/Preview/CleanupTest.php | 51 ++----------------- 3 files changed, 6 insertions(+), 59 deletions(-) diff --git a/core/Command/Preview/Cleanup.php b/core/Command/Preview/Cleanup.php index ffed7a65ccd4a..80664f7023785 100644 --- a/core/Command/Preview/Cleanup.php +++ b/core/Command/Preview/Cleanup.php @@ -102,16 +102,6 @@ private function deletePreviewFromFileCacheTable(OutputInterface $output): int { return 1; } - try { - $appDataFolder->newFolder('preview'); - $this->logger->debug('Preview folder recreated'); - $output->writeln('Preview folder recreated', OutputInterface::VERBOSITY_VERBOSE); - } catch (NotPermittedException $e) { - $output->writeln("Preview folder was deleted, but you don't have the permission to create preview folder"); - $this->logger->error("Preview folder was deleted, but you don't have the permission to create preview folder", ['exception' => $e]); - return 1; - } - $output->writeln('Previews removed'); return 0; } diff --git a/lib/public/Migration/Attributes/IndexMigrationAttribute.php b/lib/public/Migration/Attributes/IndexMigrationAttribute.php index b640a900f39d6..a67642cc726c1 100644 --- a/lib/public/Migration/Attributes/IndexMigrationAttribute.php +++ b/lib/public/Migration/Attributes/IndexMigrationAttribute.php @@ -11,7 +11,7 @@ use OCP\AppFramework\Attribute\Consumable; /** - * generic class related to migration attribute about index changes + * Generic class related to migration attribute about index changes */ #[Consumable(since: '30.0.0')] class IndexMigrationAttribute extends MigrationAttribute { @@ -19,7 +19,7 @@ class IndexMigrationAttribute extends MigrationAttribute { * @param string $table name of the database table * @param IndexType|null $type type of the index * @param string $description description of the migration - * @param array $notes notes abour the migration/index + * @param array $notes notes about the migration/index * @since 30.0.0 */ public function __construct( diff --git a/tests/Core/Command/Preview/CleanupTest.php b/tests/Core/Command/Preview/CleanupTest.php index e0050c5a9057a..3c62c3ab074eb 100644 --- a/tests/Core/Command/Preview/CleanupTest.php +++ b/tests/Core/Command/Preview/CleanupTest.php @@ -55,7 +55,6 @@ public function testCleanup(): void { $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->expects($this->once()) ->method('getAppDataDirectoryName') @@ -66,13 +65,12 @@ public function testCleanup(): void { ->with('appdata_some_id') ->willReturn($appDataFolder); - $this->output->expects($this->exactly(3))->method('writeln') + $this->output->expects($this->exactly(2))->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' + 2 => $message === 'Previews removed' }; })); @@ -90,7 +88,6 @@ public function testCleanupWhenNotDeletable(): void { $appDataFolder = $this->createMock(Folder::class); $appDataFolder->expects($this->once())->method('get')->with('preview')->willReturn($previewFolder); - $appDataFolder->expects($this->never())->method('newFolder')->with('preview'); $this->rootFolder->expects($this->once()) ->method('getAppDataDirectoryName') @@ -120,7 +117,6 @@ public function testCleanupWithDeleteException(string $exceptionClass, string $e $appDataFolder = $this->createMock(Folder::class); $appDataFolder->expects($this->once())->method('get')->with('preview')->willReturn($previewFolder); - $appDataFolder->expects($this->never())->method('newFolder')->with('preview'); $this->rootFolder->expects($this->once()) ->method('getAppDataDirectoryName') @@ -144,42 +140,6 @@ public static function dataForTestCleanupWithDeleteException(): array { ]; } - public function testCleanupWithCreateException(): 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')->willThrowException(new NotPermittedException()); - - $this->rootFolder->expects($this->once()) - ->method('getAppDataDirectoryName') - ->willReturn('appdata_some_id'); - - $this->rootFolder->expects($this->once()) - ->method('get') - ->with('appdata_some_id') - ->willReturn($appDataFolder); - - $this->output->expects($this->exactly(2))->method('writeln') - ->with(self::callback(function (string $message): bool { - static $i = 0; - return match (++$i) { - 1 => $message === 'Preview folder deleted', - 2 => $message === "Preview folder was deleted, but you don't have the permission to create preview folder", - }; - })); - - $this->logger->expects($this->once())->method('error')->with("Preview folder was deleted, but you don't have the permission to create preview folder"); - - $this->assertEquals(1, $this->repair->run($this->input, $this->output)); - } - public function testCleanupWithPreviewServiceException(): void { $previewFolder = $this->createMock(Folder::class); $previewFolder->expects($this->once()) @@ -191,7 +151,6 @@ public function testCleanupWithPreviewServiceException(): void { $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'); @@ -200,17 +159,15 @@ public function testCleanupWithPreviewServiceException(): void { ->with('appdata_some_id') ->willReturn($appDataFolder); - $this->output->expects($this->exactly(3))->method('writeln') + $this->output->expects($this->exactly(2))->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' + 2 => $message === 'Previews removed' }; })); - $this->assertEquals(0, $this->repair->run($this->input, $this->output)); $this->previewService->expects($this->once())->method('deleteAll') ->willThrowException(new NotPermittedException('abc')); From 357a6eefa3619651100b46126b25021b650ce19d Mon Sep 17 00:00:00 2001 From: Carl Schwan Date: Tue, 9 Jun 2026 11:16:19 +0200 Subject: [PATCH 6/6] fix(preview): Don't abort cleanup of previews too early If we don't find previews in the filecache, this is now normal. Don't abort and instead delete previews from the new preview table instead. Signed-off-by: Carl Schwan --- core/BackgroundJobs/PreviewMigrationJob.php | 12 ++++++--- core/Command/Preview/Cleanup.php | 5 ++-- tests/Core/Command/Preview/CleanupTest.php | 27 ++------------------- 3 files changed, 12 insertions(+), 32 deletions(-) diff --git a/core/BackgroundJobs/PreviewMigrationJob.php b/core/BackgroundJobs/PreviewMigrationJob.php index 5a8f9fffbccac..842aa56b1f217 100644 --- a/core/BackgroundJobs/PreviewMigrationJob.php +++ b/core/BackgroundJobs/PreviewMigrationJob.php @@ -51,10 +51,14 @@ protected function run(mixed $argument): void { $qb = $this->connection->getQueryBuilder(); $qb->select('path') ->from('filecache') - // Hierarchical preview folder structure - ->where($qb->expr()->like('path', $qb->createNamedParameter($this->previewRootPath . '%/%/%/%/%/%/%/%/%'))) - // Legacy flat preview folder structure - ->orWhere($qb->expr()->like('path', $qb->createNamedParameter($this->previewRootPath . '%/%.%'))) + ->where($qb->expr()->orX( + // Hierarchical preview folder structure + $qb->expr()->like('path', $qb->createNamedParameter($this->previewRootPath . '%/%/%/%/%/%/%/%/%')), + // Legacy flat preview folder structure + $qb->expr()->like('path', $qb->createNamedParameter($this->previewRootPath . '%/%.%')) + ))->andWhere( + $qb->expr()->eq('storage', $qb->createNamedParameter($this->rootFolder->getMountPoint()->getNumericStorageId())) + ) ->hintShardKey('storage', $this->rootFolder->getMountPoint()->getNumericStorageId()) ->setMaxResults(100); diff --git a/core/Command/Preview/Cleanup.php b/core/Command/Preview/Cleanup.php index 80664f7023785..0fe6efbcc1e46 100644 --- a/core/Command/Preview/Cleanup.php +++ b/core/Command/Preview/Cleanup.php @@ -77,9 +77,8 @@ private function deletePreviewFromFileCacheTable(OutputInterface $output): int { $previewFolder = $appDataFolder->get('preview'); } catch (NotFoundException $e) { - $this->logger->error("Previews can't be removed: appdata folder can't be found", ['exception' => $e]); - $output->writeln("Previews can't be removed: preview folder isn't deletable"); - return 1; + $this->logger->info("Legacy previews can't be removed: appdata folder can't be found", ['exception' => $e]); + return 0; } if (!$previewFolder->isDeletable()) { diff --git a/tests/Core/Command/Preview/CleanupTest.php b/tests/Core/Command/Preview/CleanupTest.php index 3c62c3ab074eb..579d5abf6d23d 100644 --- a/tests/Core/Command/Preview/CleanupTest.php +++ b/tests/Core/Command/Preview/CleanupTest.php @@ -141,36 +141,13 @@ public static function dataForTestCleanupWithDeleteException(): array { } 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); - $this->rootFolder->method('getAppDataDirectoryName') - ->willReturn('appdata_some_id'); - - $this->rootFolder->method('get') - ->with('appdata_some_id') - ->willReturn($appDataFolder); - - $this->output->expects($this->exactly(2))->method('writeln') - ->with(self::callback(function (string $message): bool { - static $i = 0; - return match (++$i) { - 1 => $message === 'Preview folder deleted', - 2 => $message === 'Previews removed' - }; - })); + ->willThrowException(new NotFoundException()); $this->previewService->expects($this->once())->method('deleteAll') ->willThrowException(new NotPermittedException('abc')); + $this->logger->expects($this->once())->method('info')->with("Legacy previews can't be removed: appdata folder can't be found"); $this->logger->expects($this->once())->method('error')->with("Previews can't be removed: exception occurred: abc"); $this->assertEquals(1, $this->repair->run($this->input, $this->output));