[stable34] fix(preview): First cleanup from filecache and then from preview table

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);
 
@@ -49,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);
 
@@ -95,11 +101,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;
 	}

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);
 	}
 
 	/**
@@ -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()) {
@@ -102,16 +101,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;
 	}

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,10 +109,15 @@ 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
-					$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;
@@ -179,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;
 			}

lib/public/Migration/Attributes/IndexMigrationAttribute.php

@@ -11,15 +11,15 @@
 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 {
 	/**
 	 * @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(

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,22 +65,19 @@ 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'
 				};
 			}));
 
 		$this->assertEquals(0, $this->repair->run($this->input, $this->output));
 	}
 
 	public function testCleanupWhenNotDeletable(): void {
-		$this->previewService->expects($this->once())->method('deleteAll');
-
 		$previewFolder = $this->createMock(Folder::class);
 		$previewFolder->expects($this->once())
 			->method('isDeletable')
@@ -92,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')
@@ -111,8 +106,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')
@@ -124,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')
@@ -148,53 +140,16 @@ 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')
-			->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 {
+		$this->rootFolder->method('getAppDataDirectoryName')
+			->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->rootFolder->expects($this->never())
-			->method('get');
-
 		$this->assertEquals(1, $this->repair->run($this->input, $this->output));
 	}
 }

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));