diff --git a/core/BackgroundJobs/PreviewMigrationJob.php b/core/BackgroundJobs/PreviewMigrationJob.php index 5ca3de9286a35..e5bd2b4a49450 100644 --- a/core/BackgroundJobs/PreviewMigrationJob.php +++ b/core/BackgroundJobs/PreviewMigrationJob.php @@ -12,11 +12,10 @@ 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; @@ -27,7 +26,6 @@ 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, @@ -45,79 +43,62 @@ protected function run(mixed $argument): void { return; } - $startTime = time(); - while (true) { - $qb = $this->connection->getQueryBuilder(); - $qb->select('path') - ->from('filecache') - ->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); - - $result = $qb->executeQuery(); - $foundPreviews = $this->processQueryResult($result); - - if (!$foundPreviews) { - break; - } + $storage = $this->rootFolder->getMountPoint()->getStorage(); + if ($storage === null) { + $this->appConfig->setValueBool('core', 'previewMovedDone', true); + return; + } - // Stop if execution time is more than one hour. - if (time() - $startTime > 3600) { - 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; } - $this->appConfig->setValueBool('core', 'previewMovedDone', true); - } + $startTime = time(); - 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; + // 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); + + // 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 { + $previewEntries[] = $entry; } } - $foundPreview = true; - } - foreach ($fileIds as $fileId) { - 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, - ]); + if ($previewEntries === [] || !ctype_digit($folderName)) { + continue; } - } - foreach ($flatFileIds as $fileId) { try { - $this->migrationService->migrateFileId($fileId, flatPath: true); + $this->migrationService->migrateFileId((int)$folderName, flatPath: $depth === 1, entries: $previewEntries); } catch (\Exception $e) { - $this->logger->error('Failed to migrate preview with fileId: ' . $fileId . ' (legacy file structure)', [ + $this->logger->error('Failed to migrate preview with fileId: ' . $folderName, [ 'exception' => $e, ]); } + + // Stop if execution time is more than one hour. + if (time() - $startTime > 3600) { + return; + } } - return $foundPreview; + + $this->appConfig->setValueBool('core', 'previewMovedDone', true); } } diff --git a/lib/private/Preview/PreviewMigrationService.php b/lib/private/Preview/PreviewMigrationService.php index 555bd3884474b..377950da84952 100644 --- a/lib/private/Preview/PreviewMigrationService.php +++ b/lib/private/Preview/PreviewMigrationService.php @@ -14,7 +14,9 @@ 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\Cache\ICacheEntry; use OCP\Files\IAppData; use OCP\Files\IMimeTypeDetector; use OCP\Files\IMimeTypeLoader; @@ -44,44 +46,43 @@ 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,56 +99,51 @@ public function migrateFileId(int $fileId, bool $flatPath): array { $cursor->closeCursor(); 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 $e) { - if ($e->getReason() !== Exception::REASON_UNIQUE_CONSTRAINT_VIOLATION) { - throw $e; + $oldFileIdsToDelete = []; + try { + foreach ($previewsToInsert as $preview) { + $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[] = $preview->getOldFileId(); + continue; } - $delete = $this->connection->getQueryBuilder(); - // We already have this preview in the preview table, skip - $delete->delete('filecache') - ->where($delete->expr()->eq('fileid', $delete->createNamedParameter($file->getId()))) - ->hintShardKey('storage', $this->rootFolder->getMountPoint()->getNumericStorageId()) - ->executeStatement(); - continue; - } + try { + $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; + } - 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[] = $preview->getOldFileId(); + $previews[] = $preview; } - - $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(); } @@ -165,6 +161,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; @@ -185,14 +198,38 @@ private function deleteFolder(string $path): void { break; } - try { - $folder = $this->appData->getFolder($current); - } catch (NotFoundException) { - break; - } - 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))) + ->andWhere($qb->expr()->eq('storage', $qb->createNamedParameter($storageId))) + ->setMaxResults(1); + $cursor = $qb->executeQuery(); + $hasChild = $cursor->fetchOne() !== false; + $cursor->closeCursor(); + + return $hasChild; + } } 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 99427b74b2901..4d9d594089e40 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] diff --git a/tests/lib/Preview/PreviewMigrationJobTest.php b/tests/lib/Preview/PreviewMigrationJobTest.php index fe9cd96879959..dcd57464f0d1e 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; @@ -115,11 +116,21 @@ public function testMigrationLegacyPath(): void { $this->assertEquals(2, count($folder->getDirectoryListing())); $this->assertEquals(0, count(iterator_to_array($this->previewMapper->getAvailablePreviewsForFile(5)))); - $job = new PreviewMigrationJob( + $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)))); + } + + 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(IDBConnection::class), Server::get(IRootFolder::class), new PreviewMigrationService( $this->config, @@ -134,13 +145,107 @@ public function testMigrationLegacyPath(): void { ), $this->logger, ); - $this->invokePrivate($job, 'run', [[]]); + } + + 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(2, count(iterator_to_array($this->previewMapper->getAvailablePreviewsForFile(5)))); + $this->assertEquals(1, count(iterator_to_array($this->previewMapper->getAvailablePreviewsForFile(5)))); } - private static function getInternalFolder(string $name): string { - return implode('/', str_split(substr(md5($name), 0, 7))) . '/' . $name; + #[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")] @@ -157,7 +262,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, @@ -203,7 +307,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,