From 2b7c149f895e297b2ab2154c22755eb65e164d63 Mon Sep 17 00:00:00 2001 From: Carl Schwan Date: Tue, 28 Jul 2026 10:36:26 +0200 Subject: [PATCH 1/6] perf(preview): Optimize retriving all previews from oc_filecache Previoulsy we used a PATH LIKE expression which is fine for small instances but doesn't scale for big instance (timeout). Manually moving accross the tree with getFolderContentsById is significantly faster as we can use the index and this also reuse common APIs from OCP/Files instead of directly manipulating the filecache with the query builder. Assisted-by: ClaudeCode:claude-opus-4-8 Signed-off-by: Carl Schwan --- core/BackgroundJobs/PreviewMigrationJob.php | 100 +++++++++--------- tests/lib/Preview/PreviewMigrationJobTest.php | 15 +-- 2 files changed, 57 insertions(+), 58 deletions(-) diff --git a/core/BackgroundJobs/PreviewMigrationJob.php b/core/BackgroundJobs/PreviewMigrationJob.php index a9a5c9f773c29..9a9e64b9703b3 100644 --- a/core/BackgroundJobs/PreviewMigrationJob.php +++ b/core/BackgroundJobs/PreviewMigrationJob.php @@ -9,16 +9,15 @@ namespace OC\Core\BackgroundJobs; -use OC\Preview\Db\Preview; use OC\Preview\PreviewMigrationService; use OCP\AppFramework\Utility\ITimeFactory; use OCP\BackgroundJob\TimedJob; -use OCP\DB\IResult; +use OCP\Files\FileInfo; use OCP\Files\IRootFolder; use OCP\IAppConfig; use OCP\IConfig; -use OCP\IDBConnection; use Override; +use Psr\Log\LoggerInterface; class PreviewMigrationJob extends TimedJob { private string $previewRootPath; @@ -27,9 +26,9 @@ public function __construct( ITimeFactory $time, private readonly IAppConfig $appConfig, private readonly IConfig $config, - private readonly IDBConnection $connection, private readonly IRootFolder $rootFolder, private readonly PreviewMigrationService $migrationService, + private readonly LoggerInterface $logger, ) { parent::__construct($time); @@ -44,23 +43,52 @@ protected function run(mixed $argument): void { return; } + $storage = $this->rootFolder->getMountPoint()->getStorage(); + if ($storage === null) { + $this->appConfig->setValueBool('core', 'previewMovedDone', true); + return; + } + + $cache = $storage->getCache(); + $previewRootId = $cache->getId(rtrim($this->previewRootPath, '/')); + if ($previewRootId === -1) { + // No previews have ever been generated on this instance. + $this->appConfig->setValueBool('core', 'previewMovedDone', true); + return; + } + $startTime = time(); - while (true) { - $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 . '%/%.%'))) - ->hintShardKey('storage', $this->rootFolder->getMountPoint()->getNumericStorageId()) - ->setMaxResults(100); - - $result = $qb->executeQuery(); - $foundPreviews = $this->processQueryResult($result); - - if (!$foundPreviews) { - break; + + // Walk the preview folder tree via the `parent` column, which is indexed on + // every supported database platform. + // + // Depth from the preview root tells us which structure a leaf folder holds: + // - depth 1: legacy flat structure, e.g. preview//.png + // - depth 8: hierarchical structure, e.g. preview/a/b/c/d/e/f/g//.png + $foldersToVisit = [[$previewRootId, '', 0]]; + + while ($foldersToVisit !== []) { + [$folderId, $folderName, $depth] = array_pop($foldersToVisit); + + $hasPreviewFiles = false; + foreach ($cache->getFolderContentsById($folderId) as $entry) { + if ($entry->getMimeType() === FileInfo::MIMETYPE_FOLDER) { + $foldersToVisit[] = [$entry->getId(), $entry->getName(), $depth + 1]; + } else { + $hasPreviewFiles = true; + } + } + + if (!$hasPreviewFiles || !ctype_digit($folderName)) { + continue; + } + + try { + $this->migrationService->migrateFileId((int)$folderName, flatPath: $depth === 1); + } catch (\Exception $e) { + $this->logger->error('Failed to migrate preview with fileId: ' . $folderName, [ + 'exception' => $e, + ]); } // Stop if execution time is more than one hour. @@ -71,36 +99,4 @@ protected function run(mixed $argument): void { $this->appConfig->setValueBool('core', 'previewMovedDone', true); } - - private function processQueryResult(IResult $result): bool { - $foundPreview = false; - $fileIds = []; - $flatFileIds = []; - while ($row = $result->fetch()) { - $pathSplit = explode('/', $row['path']); - assert(count($pathSplit) >= 2); - $fileId = (int)$pathSplit[count($pathSplit) - 2]; - if (count($pathSplit) === 11) { - // Hierarchical structure - if (!in_array($fileId, $fileIds)) { - $fileIds[] = $fileId; - } - } else { - // Flat structure - if (!in_array($fileId, $flatFileIds)) { - $flatFileIds[] = $fileId; - } - } - $foundPreview = true; - } - - foreach ($fileIds as $fileId) { - $this->migrationService->migrateFileId($fileId, flatPath: false); - } - - foreach ($flatFileIds as $fileId) { - $this->migrationService->migrateFileId($fileId, flatPath: true); - } - return $foundPreview; - } } diff --git a/tests/lib/Preview/PreviewMigrationJobTest.php b/tests/lib/Preview/PreviewMigrationJobTest.php index 2fd4fc1c511db..06108c954fb6c 100644 --- a/tests/lib/Preview/PreviewMigrationJobTest.php +++ b/tests/lib/Preview/PreviewMigrationJobTest.php @@ -42,6 +42,7 @@ class PreviewMigrationJobTest extends TestCase { private IMimeTypeDetector&MockObject $mimeTypeDetector; private LoggerInterface&MockObject $logger; + #[\Override] public function setUp(): void { parent::setUp(); $this->previewAppData = Server::get(IAppDataFactory::class)->get('preview'); @@ -91,6 +92,7 @@ public function setUp(): void { $this->logger = $this->createMock(LoggerInterface::class); } + #[\Override] public function tearDown(): void { foreach ($this->previewAppData->getDirectoryListing() as $folder) { $folder->delete(); @@ -101,6 +103,7 @@ public function tearDown(): void { $qb->delete('filecache') ->where($qb->expr()->eq('fileid', $qb->createNamedParameter(5))) ->executeStatement(); + parent::tearDown(); } #[TestDox('Test the migration from the legacy flat hierarchy to the new database format')] @@ -116,7 +119,6 @@ public function testMigrationLegacyPath(): void { Server::get(ITimeFactory::class), $this->appConfig, $this->config, - Server::get(IDBConnection::class), Server::get(IRootFolder::class), new PreviewMigrationService( $this->config, @@ -128,7 +130,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())); @@ -153,7 +156,6 @@ public function testMigrationPath(): void { Server::get(ITimeFactory::class), $this->appConfig, $this->config, - Server::get(IDBConnection::class), Server::get(IRootFolder::class), new PreviewMigrationService( $this->config, @@ -165,7 +167,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())); @@ -198,7 +201,6 @@ public function testMigrationPathWithVersion(): void { Server::get(ITimeFactory::class), $this->appConfig, $this->config, - Server::get(IDBConnection::class), Server::get(IRootFolder::class), new PreviewMigrationService( $this->config, @@ -210,7 +212,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 0ff58534fda2191d542cc1cc57d323bb5f7db6c8 Mon Sep 17 00:00:00 2001 From: Carl Schwan Date: Tue, 28 Jul 2026 12:34:17 +0200 Subject: [PATCH 2/6] perf: Batch deletion of old file entries Faster than doing a commit after each file Assisted-by: ClaudeCode:claude-opus-4-8 Signed-off-by: Carl Schwan --- .../Preview/PreviewMigrationService.php | 117 ++++++++++++------ 1 file changed, 82 insertions(+), 35 deletions(-) diff --git a/lib/private/Preview/PreviewMigrationService.php b/lib/private/Preview/PreviewMigrationService.php index 90cb917c8ce94..4d452904f1873 100644 --- a/lib/private/Preview/PreviewMigrationService.php +++ b/lib/private/Preview/PreviewMigrationService.php @@ -14,6 +14,7 @@ use OC\Preview\Db\PreviewMapper; use OC\Preview\Storage\StorageFactory; use OCP\DB\Exception; +use OCP\DB\QueryBuilder\IQueryBuilder; use OCP\Files\AppData\IAppDataFactory; use OCP\Files\IAppData; use OCP\Files\IMimeTypeDetector; @@ -95,40 +96,42 @@ public function migrateFileId(int $fileId, bool $flatPath): array { $result = $result->fetchAssociative(); if ($result !== false) { - foreach ($previewFiles as $previewFile) { - /** @var Preview $preview */ - $preview = $previewFile['preview']; - /** @var SimpleFile $file */ - $file = $previewFile['file']; - $preview->setStorageId($result['storage']); - $preview->setEtag($result['etag']); - $preview->setSourceMimeType($this->mimeTypeLoader->getMimetypeById((int)$result['mimetype'])); - $preview->generateId(); - try { - $preview = $this->previewMapper->insert($preview); - } catch (Exception) { - // We already have this preview in the preview table, skip - $qb->delete('filecache') - ->where($qb->expr()->eq('fileid', $qb->createNamedParameter($file->getId()))) - ->hintShardKey('storage', $this->rootFolder->getMountPoint()->getNumericStorageId()) - ->executeStatement(); - continue; - } - - try { - $this->storageFactory->migratePreview($preview, $file); - $qb = $this->connection->getQueryBuilder(); - $qb->delete('filecache') - ->where($qb->expr()->eq('fileid', $qb->createNamedParameter($file->getId()))) - ->hintShardKey('storage', $this->rootFolder->getMountPoint()->getNumericStorageId()) - ->executeStatement(); - // Do not call $file->delete() as this will also delete the file from the file system - } catch (\Exception $e) { - $this->previewMapper->delete($preview); - throw $e; + $oldFileIdsToDelete = []; + try { + foreach ($previewFiles as $previewFile) { + /** @var Preview $preview */ + $preview = $previewFile['preview']; + /** @var SimpleFile $file */ + $file = $previewFile['file']; + $preview->setStorageId($result['storage']); + $preview->setEtag($result['etag']); + $preview->setSourceMimeType($this->mimeTypeLoader->getMimetypeById((int)$result['mimetype'])); + $preview->generateId(); + try { + $preview = $this->previewMapper->insert($preview); + } catch (Exception $e) { + if ($e->getReason() !== Exception::REASON_UNIQUE_CONSTRAINT_VIOLATION) { + throw $e; + } + + // We already have this preview in the preview table, skip + $oldFileIdsToDelete[] = $file->getId(); + continue; + } + + try { + $this->storageFactory->migratePreview($preview, $file); + // Do not call $file->delete() as this will also delete the file from the file system + } catch (\Exception $e) { + $this->previewMapper->delete($preview); + throw $e; + } + + $oldFileIdsToDelete[] = $file->getId(); + $previews[] = $preview; } - - $previews[] = $preview; + } finally { + $this->deleteOldFileCacheEntries($oldFileIdsToDelete); } } else { // No matching fileId, delete preview @@ -157,6 +160,23 @@ private static function getInternalFolder(string $name, bool $flatPath): string return implode('/', str_split(substr(md5($name), 0, 7))) . '/' . $name; } + /** + * @param list $fileIds + */ + private function deleteOldFileCacheEntries(array $fileIds): void { + if ($fileIds === []) { + return; + } + + foreach (array_chunk($fileIds, 1000) as $chunk) { + $qb = $this->connection->getQueryBuilder(); + $qb->delete('filecache') + ->where($qb->expr()->in('fileid', $qb->createNamedParameter($chunk, IQueryBuilder::PARAM_INT_ARRAY))) + ->hintShardKey('storage', $this->rootFolder->getMountPoint()->getNumericStorageId()) + ->executeStatement(); + } + } + private function deleteFolder(string $path): void { $current = $path; @@ -177,10 +197,37 @@ private function deleteFolder(string $path): void { break; } - $folder = $this->appData->getFolder($current); - if (count($folder->getDirectoryListing()) !== 0) { + if ($this->folderHasChildren($rootFolderId, $this->previewRootPath . $current)) { break; } } } + + private function folderHasChildren(int $storageId, string $path): bool { + $qb = $this->connection->getQueryBuilder(); + $qb->select('fileid') + ->from('filecache') + ->where($qb->expr()->eq('path_hash', $qb->createNamedParameter(md5($path)))) + ->andWhere($qb->expr()->eq('storage', $qb->createNamedParameter($storageId))) + ->setMaxResults(1); + $cursor = $qb->executeQuery(); + $folderId = $cursor->fetchOne(); + $cursor->closeCursor(); + + if ($folderId === false) { + // The folder itself is already gone, nothing to check. + return false; + } + + $qb = $this->connection->getQueryBuilder(); + $qb->select('fileid') + ->from('filecache') + ->where($qb->expr()->eq('parent', $qb->createNamedParameter((int)$folderId))) + ->setMaxResults(1); + $cursor = $qb->executeQuery(); + $hasChild = $cursor->fetchOne() !== false; + $cursor->closeCursor(); + + return $hasChild; + } } From 0d6f55e12bf12efcfb734bccd6e2da0a31e87ba2 Mon Sep 17 00:00:00 2001 From: Carl Schwan Date: Tue, 28 Jul 2026 12:56:54 +0200 Subject: [PATCH 3/6] perf: Don't list items twice in the preview folder Assisted-by: ClaudeCode:claude-opus-4-8 Signed-off-by: Carl Schwan --- core/BackgroundJobs/PreviewMigrationJob.php | 10 +-- .../Preview/PreviewMigrationService.php | 64 +++++++++---------- .../Preview/Storage/IPreviewStorage.php | 3 +- .../Preview/Storage/LocalPreviewStorage.php | 3 +- .../Storage/ObjectStorePreviewStorage.php | 3 +- .../Preview/Storage/StorageFactory.php | 5 +- 6 files changed, 43 insertions(+), 45 deletions(-) diff --git a/core/BackgroundJobs/PreviewMigrationJob.php b/core/BackgroundJobs/PreviewMigrationJob.php index 9a9e64b9703b3..e5bd2b4a49450 100644 --- a/core/BackgroundJobs/PreviewMigrationJob.php +++ b/core/BackgroundJobs/PreviewMigrationJob.php @@ -70,21 +70,23 @@ protected function run(mixed $argument): void { while ($foldersToVisit !== []) { [$folderId, $folderName, $depth] = array_pop($foldersToVisit); - $hasPreviewFiles = false; + // Collect the actual preview files here so migrateFileId() doesn't need to + // list this folder's contents a second time. + $previewEntries = []; foreach ($cache->getFolderContentsById($folderId) as $entry) { if ($entry->getMimeType() === FileInfo::MIMETYPE_FOLDER) { $foldersToVisit[] = [$entry->getId(), $entry->getName(), $depth + 1]; } else { - $hasPreviewFiles = true; + $previewEntries[] = $entry; } } - if (!$hasPreviewFiles || !ctype_digit($folderName)) { + if ($previewEntries === [] || !ctype_digit($folderName)) { continue; } try { - $this->migrationService->migrateFileId((int)$folderName, flatPath: $depth === 1); + $this->migrationService->migrateFileId((int)$folderName, flatPath: $depth === 1, entries: $previewEntries); } catch (\Exception $e) { $this->logger->error('Failed to migrate preview with fileId: ' . $folderName, [ 'exception' => $e, diff --git a/lib/private/Preview/PreviewMigrationService.php b/lib/private/Preview/PreviewMigrationService.php index 4d452904f1873..2d978830b2114 100644 --- a/lib/private/Preview/PreviewMigrationService.php +++ b/lib/private/Preview/PreviewMigrationService.php @@ -16,6 +16,7 @@ use OCP\DB\Exception; use OCP\DB\QueryBuilder\IQueryBuilder; use OCP\Files\AppData\IAppDataFactory; +use OCP\Files\Cache\ICacheEntry; use OCP\Files\IAppData; use OCP\Files\IMimeTypeDetector; use OCP\Files\IMimeTypeLoader; @@ -45,44 +46,45 @@ public function __construct( } /** - * @param array $previewFolders + * @param list|null $entries Preview file entries already fetched by the caller. * @return Preview[] */ - public function migrateFileId(int $fileId, bool $flatPath): array { + public function migrateFileId(int $fileId, bool $flatPath, ?array $entries = null): array { $previews = []; $internalPath = $this->getInternalFolder((string)$fileId, $flatPath); - try { - $folder = $this->appData->getFolder($internalPath); - } catch (NotFoundException) { - return []; + + if ($entries === null) { + try { + $entries = $this->appData->getFolder($internalPath)->getDirectoryListing(); + } catch (NotFoundException) { + return []; + } } /** - * @var list $previewFiles + * @var list $previewsToInsert */ - $previewFiles = []; + $previewsToInsert = []; - foreach ($folder->getDirectoryListing() as $previewFile) { - $path = $fileId . '/' . $previewFile->getName(); - /** @var SimpleFile $previewFile */ + foreach ($entries as $entry) { + $path = $fileId . '/' . $entry->getName(); $preview = Preview::fromPath($path, $this->mimeTypeDetector); if ($preview === false) { $this->logger->error('Unable to import old preview at path.'); continue; } $preview->generateId(); - $preview->setSize($previewFile->getSize()); - $preview->setMtime($previewFile->getMtime()); - $preview->setOldFileId($previewFile->getId()); + $preview->setSize($entry->getSize()); + $preview->setMtime($entry->getMTime()); + $preview->setOldFileId($entry->getId()); $preview->setEncrypted(false); - $previewFiles[] = [ - 'file' => $previewFile, - 'preview' => $preview, - ]; + $previewsToInsert[] = $preview; } - if (empty($previewFiles)) { + if (empty($previewsToInsert)) { + $this->deleteFolder($internalPath); + return $previews; } @@ -98,11 +100,7 @@ public function migrateFileId(int $fileId, bool $flatPath): array { if ($result !== false) { $oldFileIdsToDelete = []; try { - foreach ($previewFiles as $previewFile) { - /** @var Preview $preview */ - $preview = $previewFile['preview']; - /** @var SimpleFile $file */ - $file = $previewFile['file']; + foreach ($previewsToInsert as $preview) { $preview->setStorageId($result['storage']); $preview->setEtag($result['etag']); $preview->setSourceMimeType($this->mimeTypeLoader->getMimetypeById((int)$result['mimetype'])); @@ -115,34 +113,36 @@ public function migrateFileId(int $fileId, bool $flatPath): array { } // We already have this preview in the preview table, skip - $oldFileIdsToDelete[] = $file->getId(); + $oldFileIdsToDelete[] = $preview->getOldFileId(); continue; } try { - $this->storageFactory->migratePreview($preview, $file); - // Do not call $file->delete() as this will also delete the file from the file system + $this->storageFactory->migratePreview($preview); + // Do not delete the old file via a Node here, as that would also + // delete it from the file system; only its filecache row is stale. } catch (\Exception $e) { $this->previewMapper->delete($preview); throw $e; } - $oldFileIdsToDelete[] = $file->getId(); + $oldFileIdsToDelete[] = $preview->getOldFileId(); $previews[] = $preview; } } finally { $this->deleteOldFileCacheEntries($oldFileIdsToDelete); } } else { - // No matching fileId, delete preview + // No matching fileId, delete the orphaned preview files themselves. try { + $folder = $this->appData->getFolder($internalPath); $this->connection->beginTransaction(); - foreach ($previewFiles as $previewFile) { - /** @var SimpleFile $file */ - $file = $previewFile['file']; + foreach ($folder->getDirectoryListing() as $file) { $file->delete(); } $this->connection->commit(); + } catch (NotFoundException) { + // Folder already gone, nothing to clean up. } catch (Exception) { $this->connection->rollback(); } diff --git a/lib/private/Preview/Storage/IPreviewStorage.php b/lib/private/Preview/Storage/IPreviewStorage.php index f7b60b9cc9be3..28d2dc53ef577 100644 --- a/lib/private/Preview/Storage/IPreviewStorage.php +++ b/lib/private/Preview/Storage/IPreviewStorage.php @@ -10,7 +10,6 @@ namespace OC\Preview\Storage; use Exception; -use OC\Files\SimpleFS\SimpleFile; use OC\Preview\Db\Preview; use OCP\Files\NotFoundException; use OCP\Files\NotPermittedException; @@ -42,7 +41,7 @@ public function deletePreview(Preview $preview): void; * To remove at some point * @throws Exception */ - public function migratePreview(Preview $preview, SimpleFile $file): void; + public function migratePreview(Preview $preview): void; /** * @throws NotPermittedException diff --git a/lib/private/Preview/Storage/LocalPreviewStorage.php b/lib/private/Preview/Storage/LocalPreviewStorage.php index 9ada6be2c58d7..f704e0d9313dd 100644 --- a/lib/private/Preview/Storage/LocalPreviewStorage.php +++ b/lib/private/Preview/Storage/LocalPreviewStorage.php @@ -12,7 +12,6 @@ use LogicException; use OC; -use OC\Files\SimpleFS\SimpleFile; use OC\Preview\Db\Preview; use OC\Preview\Db\PreviewMapper; use OCP\DB\Exception; @@ -93,7 +92,7 @@ private function createParentFiles(string $path): void { } #[Override] - public function migratePreview(Preview $preview, SimpleFile $file): void { + public function migratePreview(Preview $preview): void { // legacy flat directory $sourcePath = $this->getPreviewRootFolder() . $preview->getFileId() . '/' . $preview->getName(); if (!file_exists($sourcePath)) { diff --git a/lib/private/Preview/Storage/ObjectStorePreviewStorage.php b/lib/private/Preview/Storage/ObjectStorePreviewStorage.php index 713d39f9b0b38..f77da26eaea95 100644 --- a/lib/private/Preview/Storage/ObjectStorePreviewStorage.php +++ b/lib/private/Preview/Storage/ObjectStorePreviewStorage.php @@ -12,7 +12,6 @@ use Icewind\Streams\CountWrapper; use OC\Files\ObjectStore\PrimaryObjectStoreConfig; -use OC\Files\SimpleFS\SimpleFile; use OC\Preview\Db\Preview; use OC\Preview\Db\PreviewMapper; use OCP\Files\NotPermittedException; @@ -95,7 +94,7 @@ public function deletePreview(Preview $preview): void { } #[Override] - public function migratePreview(Preview $preview, SimpleFile $file): void { + public function migratePreview(Preview $preview): void { // Just set the Preview::bucket and Preview::objectStore $this->getObjectStoreInfoForNewPreview($preview, migration: true); $this->previewMapper->update($preview); diff --git a/lib/private/Preview/Storage/StorageFactory.php b/lib/private/Preview/Storage/StorageFactory.php index 69c7776c42e51..39cd11e09cfbd 100644 --- a/lib/private/Preview/Storage/StorageFactory.php +++ b/lib/private/Preview/Storage/StorageFactory.php @@ -10,7 +10,6 @@ namespace OC\Preview\Storage; use OC\Files\ObjectStore\PrimaryObjectStoreConfig; -use OC\Files\SimpleFS\SimpleFile; use OC\Preview\Db\Preview; use OCP\Server; use Override; @@ -53,8 +52,8 @@ private function getBackend(): IPreviewStorage { } #[Override] - public function migratePreview(Preview $preview, SimpleFile $file): void { - $this->getBackend()->migratePreview($preview, $file); + public function migratePreview(Preview $preview): void { + $this->getBackend()->migratePreview($preview); } #[Override] From 85c6bfc4a4102f4da967516ea235984465527af3 Mon Sep 17 00:00:00 2001 From: Carl Schwan Date: Tue, 28 Jul 2026 13:07:15 +0200 Subject: [PATCH 4/6] chore(preview): Add more tests for the preview migration job Assisted-by: ClaudeCode:claude-opus-4-8 Signed-off-by: Carl Schwan --- tests/lib/Preview/PreviewMigrationJobTest.php | 123 ++++++++++++++++++ 1 file changed, 123 insertions(+) diff --git a/tests/lib/Preview/PreviewMigrationJobTest.php b/tests/lib/Preview/PreviewMigrationJobTest.php index 06108c954fb6c..c579ce092443c 100644 --- a/tests/lib/Preview/PreviewMigrationJobTest.php +++ b/tests/lib/Preview/PreviewMigrationJobTest.php @@ -10,6 +10,7 @@ namespace lib\Preview; use OC\Core\BackgroundJobs\PreviewMigrationJob; +use OC\Preview\Db\Preview; use OC\Preview\Db\PreviewMapper; use OC\Preview\PreviewMigrationService; use OC\Preview\PreviewService; @@ -142,6 +143,128 @@ private static function getInternalFolder(string $name): string { return implode('/', str_split(substr(md5($name), 0, 7))) . '/' . $name; } + private function createJob(): PreviewMigrationJob { + return new PreviewMigrationJob( + Server::get(ITimeFactory::class), + $this->appConfig, + $this->config, + Server::get(IRootFolder::class), + new PreviewMigrationService( + $this->config, + Server::get(IRootFolder::class), + $this->logger, + $this->mimeTypeDetector, + $this->mimeTypeLoader, + Server::get(IDBConnection::class), + $this->previewMapper, + $this->storageFactory, + Server::get(IAppDataFactory::class), + ), + $this->logger, + ); + } + + private function insertFilecacheRow(string $path, string $etag): int { + $qb = $this->db->getQueryBuilder(); + $qb->insert('filecache') + ->values([ + 'storage' => $qb->createNamedParameter(1), + 'path' => $qb->createNamedParameter($path), + 'path_hash' => $qb->createNamedParameter(md5($path)), + 'parent' => $qb->createNamedParameter(0), + 'name' => $qb->createNamedParameter(basename($path)), + 'mimetype' => $qb->createNamedParameter(42), + 'size' => $qb->createNamedParameter(1000), + 'mtime' => $qb->createNamedParameter(1000), + 'storage_mtime' => $qb->createNamedParameter(1000), + 'encrypted' => $qb->createNamedParameter(0), + 'unencrypted_size' => $qb->createNamedParameter(0), + 'etag' => $qb->createNamedParameter($etag), + 'permissions' => $qb->createNamedParameter(0), + 'checksum' => $qb->createNamedParameter($etag), + ])->executeStatement(); + return $qb->getLastInsertId(); + } + + private function deleteFilecacheRow(int $fileId): void { + $qb = $this->db->getQueryBuilder(); + $qb->delete('filecache') + ->where($qb->expr()->eq('fileid', $qb->createNamedParameter($fileId))) + ->executeStatement(); + } + + #[TestDox('A single run must migrate multiple different fileids in one pass, mixing both folder structures')] + public function testMigrationMultipleFileIds(): void { + $otherFileId = $this->insertFilecacheRow('test/def', 'xyz123'); + + try { + $flatFolder = $this->previewAppData->newFolder('5'); + $flatFolder->newFile('64-64-crop.jpg', 'abcdefg'); + + $hierFolder = $this->previewAppData->newFolder(self::getInternalFolder((string)$otherFileId)); + $hierFolder->newFile('128-128.png', 'abcdefg'); + + $this->invokePrivate($this->createJob(), 'run', [[]]); + + $this->assertEquals(0, count($this->previewAppData->getDirectoryListing())); + $this->assertEquals(1, count(iterator_to_array($this->previewMapper->getAvailablePreviewsForFile(5)))); + $this->assertEquals(1, count(iterator_to_array($this->previewMapper->getAvailablePreviewsForFile($otherFileId)))); + } finally { + $this->deleteFilecacheRow($otherFileId); + } + } + + #[TestDox('Re-migrating a preview that already exists must skip the insert and still clean up the stale filecache row')] + public function testMigrationSkipsDuplicatePreview(): void { + $folder = $this->previewAppData->newFolder('5'); + $folder->newFile('64-64-crop.jpg', 'abcdefg'); + + // Simulate a preview that was already migrated by an earlier, interrupted run: + // same fileid/width/height/mimetype/cropped/version as what this migration + // would produce for the file created above, so the insert below hits the + // `previews_file_uniq_idx` unique constraint. + $existing = Preview::fromPath('5/64-64-crop.jpg', $this->mimeTypeDetector); + $this->assertNotFalse($existing); + $existing->setFileId(5); + $existing->setStorageId(1); + $existing->setSourceMimeType('image/png'); + $existing->setEtag('abcdefg'); + $existing->setSize(7); + $existing->setMtime(1000); + $existing->setEncrypted(false); + $existing->generateId(); + $this->previewMapper->insert($existing); + + $this->invokePrivate($this->createJob(), 'run', [[]]); + + // No duplicate preview row was inserted, but the legacy folder and its stale + // filecache row were still cleaned up. + $this->assertEquals(0, count($this->previewAppData->getDirectoryListing())); + $this->assertEquals(1, count(iterator_to_array($this->previewMapper->getAvailablePreviewsForFile(5)))); + } + + #[TestDox('An orphaned preview folder whose source file no longer exists must be deleted, not migrated')] + public function testMigrationDeletesOrphanedPreview(): void { + $orphanFileId = 9999998; + $folder = $this->previewAppData->newFolder((string)$orphanFileId); + $folder->newFile('64-64-crop.jpg', 'abcdefg'); + + $this->invokePrivate($this->createJob(), 'run', [[]]); + + $this->assertEquals(0, count($this->previewAppData->getDirectoryListing())); + $this->assertEquals(0, count(iterator_to_array($this->previewMapper->getAvailablePreviewsForFile($orphanFileId)))); + } + + #[TestDox('run() must complete without error when there is nothing to migrate')] + public function testMigrationWithoutAnyPreviews(): void { + $this->assertEquals(0, count($this->previewAppData->getDirectoryListing())); + + $this->invokePrivate($this->createJob(), 'run', [[]]); + + $this->assertEquals(0, count($this->previewAppData->getDirectoryListing())); + $this->assertEquals(0, count(iterator_to_array($this->previewMapper->getAvailablePreviewsForFile(5)))); + } + #[TestDox("Test the migration from the 'new' nested hierarchy to the database format")] public function testMigrationPath(): void { $folder = $this->previewAppData->newFolder(self::getInternalFolder((string)5)); From 500f73ef780b8f2a943048f967dbdb70b2dd01a1 Mon Sep 17 00:00:00 2001 From: Carl Schwan Date: Tue, 28 Jul 2026 13:12:56 +0200 Subject: [PATCH 5/6] refactor(Preview): Refactor tests to reduce code duplication Signed-off-by: Carl Schwan --- tests/lib/Preview/PreviewMigrationJobTest.php | 19 +------------------ 1 file changed, 1 insertion(+), 18 deletions(-) diff --git a/tests/lib/Preview/PreviewMigrationJobTest.php b/tests/lib/Preview/PreviewMigrationJobTest.php index c579ce092443c..dcd57464f0d1e 100644 --- a/tests/lib/Preview/PreviewMigrationJobTest.php +++ b/tests/lib/Preview/PreviewMigrationJobTest.php @@ -116,24 +116,7 @@ public function testMigrationLegacyPath(): void { $this->assertEquals(2, count($folder->getDirectoryListing())); $this->assertEquals(0, count(iterator_to_array($this->previewMapper->getAvailablePreviewsForFile(5)))); - $job = new PreviewMigrationJob( - Server::get(ITimeFactory::class), - $this->appConfig, - $this->config, - Server::get(IRootFolder::class), - new PreviewMigrationService( - $this->config, - Server::get(IRootFolder::class), - $this->logger, - $this->mimeTypeDetector, - $this->mimeTypeLoader, - Server::get(IDBConnection::class), - $this->previewMapper, - $this->storageFactory, - Server::get(IAppDataFactory::class), - ), - $this->logger, - ); + $job = $this->createJob(); $this->invokePrivate($job, 'run', [[]]); $this->assertEquals(0, count($this->previewAppData->getDirectoryListing())); $this->assertEquals(2, count(iterator_to_array($this->previewMapper->getAvailablePreviewsForFile(5)))); From d19492749e30c936018a7c4992c1b1a0a365b3fd Mon Sep 17 00:00:00 2001 From: Carl Schwan Date: Tue, 28 Jul 2026 13:43:07 +0200 Subject: [PATCH 6/6] fix(Preview): Fix sharding of the PreviewMigrationService Signed-off-by: Carl Schwan --- lib/private/Preview/PreviewMigrationService.php | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/private/Preview/PreviewMigrationService.php b/lib/private/Preview/PreviewMigrationService.php index 2d978830b2114..29893363b38f3 100644 --- a/lib/private/Preview/PreviewMigrationService.php +++ b/lib/private/Preview/PreviewMigrationService.php @@ -223,6 +223,7 @@ private function folderHasChildren(int $storageId, string $path): bool { $qb->select('fileid') ->from('filecache') ->where($qb->expr()->eq('parent', $qb->createNamedParameter((int)$folderId))) + ->andWhere($qb->expr()->eq('storage', $qb->createNamedParameter($storageId))) ->setMaxResults(1); $cursor = $qb->executeQuery(); $hasChild = $cursor->fetchOne() !== false;