diff --git a/build/psalm-baseline.xml b/build/psalm-baseline.xml index 621e368f0a3..606cb3b5bb1 100644 --- a/build/psalm-baseline.xml +++ b/build/psalm-baseline.xml @@ -3721,16 +3721,6 @@ - - - - - - - - - - @@ -3991,14 +3981,6 @@ - - - mode]]> - - - - - diff --git a/lib/private/Preview/BackgroundCleanupJob.php b/lib/private/Preview/BackgroundCleanupJob.php index 3138abb1bf9..62e3303dc4e 100644 --- a/lib/private/Preview/BackgroundCleanupJob.php +++ b/lib/private/Preview/BackgroundCleanupJob.php @@ -8,23 +8,25 @@ declare(strict_types=1); */ namespace OC\Preview; -use OC\Preview\Storage\Root; +use OC\Preview\Db\PreviewMapper; +use OC\Preview\Storage\StorageFactory; use OCP\AppFramework\Utility\ITimeFactory; use OCP\BackgroundJob\TimedJob; use OCP\DB\QueryBuilder\IQueryBuilder; -use OCP\Files\IMimeTypeLoader; -use OCP\Files\NotFoundException; -use OCP\Files\NotPermittedException; use OCP\IDBConnection; +/** + * @psalm-type FileId int + * @psalm-type StorageId int + */ class BackgroundCleanupJob extends TimedJob { public function __construct( ITimeFactory $timeFactory, - private IDBConnection $connection, - private Root $previewFolder, - private IMimeTypeLoader $mimeTypeLoader, - private bool $isCLI, + readonly private IDBConnection $connection, + readonly private PreviewMapper $previewMapper, + readonly private StorageFactory $storageFactory, + readonly private bool $isCLI, ) { parent::__construct($timeFactory); // Run at most once an hour @@ -32,88 +34,37 @@ class BackgroundCleanupJob extends TimedJob { $this->setTimeSensitivity(self::TIME_INSENSITIVE); } - public function run($argument) { - foreach ($this->getDeletedFiles() as $fileId) { - try { - $preview = $this->previewFolder->getFolder((string)$fileId); - $preview->delete(); - } catch (NotFoundException $e) { - // continue - } catch (NotPermittedException $e) { - // continue + public function run($argument): void { + foreach ($this->getDeletedFiles() as $chunk) { + foreach ($chunk as $storage => $fileIds) { + foreach ($this->previewMapper->getByFileIds($storage, $fileIds) as $previews) { + $previewIds = []; + foreach ($previews as $preview) { + $previewIds[] = $preview->getId(); + $this->storageFactory->deletePreview($preview); + } + + $this->previewMapper->deleteByIds($storage, $previewIds); + }; } } } + /** + * @return \Iterator> + */ private function getDeletedFiles(): \Iterator { - yield from $this->getOldPreviewLocations(); - yield from $this->getNewPreviewLocations(); - } - - private function getOldPreviewLocations(): \Iterator { - if ($this->connection->getShardDefinition('filecache')) { - // sharding is new enough that we don't need to support this - return; - } - - $qb = $this->connection->getQueryBuilder(); - $qb->select('a.name') - ->from('filecache', 'a') - ->leftJoin('a', 'filecache', 'b', $qb->expr()->eq( - $qb->expr()->castColumn('a.name', IQueryBuilder::PARAM_INT), 'b.fileid' - )) - ->where( - $qb->expr()->andX( - $qb->expr()->isNull('b.fileid'), - $qb->expr()->eq('a.storage', $qb->createNamedParameter($this->previewFolder->getStorageId())), - $qb->expr()->eq('a.parent', $qb->createNamedParameter($this->previewFolder->getId())), - $qb->expr()->like('a.name', $qb->createNamedParameter('__%')), - $qb->expr()->eq('a.mimetype', $qb->createNamedParameter($this->mimeTypeLoader->getId('httpd/unix-directory'))) - ) - ); - - if (!$this->isCLI) { - $qb->setMaxResults(10); - } - - $cursor = $qb->executeQuery(); - - while ($row = $cursor->fetch()) { - yield $row['name']; - } - - $cursor->closeCursor(); - } - - private function getNewPreviewLocations(): \Iterator { - $qb = $this->connection->getQueryBuilder(); - $qb->select('path', 'mimetype') - ->from('filecache') - ->where($qb->expr()->eq('fileid', $qb->createNamedParameter($this->previewFolder->getId()))); - $cursor = $qb->executeQuery(); - $data = $cursor->fetch(); - $cursor->closeCursor(); - - if ($data === null) { - return []; - } - if ($this->connection->getShardDefinition('filecache')) { - $chunks = $this->getAllPreviewIds($data['path'], 1000); + $chunks = $this->getAllPreviewIds(1000); foreach ($chunks as $chunk) { - yield from $this->findMissingSources($chunk); + foreach ($chunk as $storage => $preview) { + yield [$storage => $this->findMissingSources($storage, $preview)]; + } } return; } - /* - * This lovely like is the result of the way the new previews are stored - * We take the md5 of the name (fileid) and split the first 7 chars. That way - * there are not a gazillion files in the root of the preview appdata. - */ - $like = $this->connection->escapeLikeParameter($data['path']) . '/_/_/_/_/_/_/_/%'; - /* * Deleting a file will not delete related previews right away. * @@ -130,19 +81,12 @@ class BackgroundCleanupJob extends TimedJob { * If the related file is deleted, b.fileid will be null and the preview folder can be deleted. */ $qb = $this->connection->getQueryBuilder(); - $qb->select('a.name') - ->from('filecache', 'a') - ->leftJoin('a', 'filecache', 'b', $qb->expr()->eq( - $qb->expr()->castColumn('a.name', IQueryBuilder::PARAM_INT), 'b.fileid' + $qb->select('p.storage_id', 'p.file_id') + ->from('previews', 'p') + ->leftJoin('p', 'filecache', 'f', $qb->expr()->eq( + 'p.file_id', 'f.fileid' )) - ->where( - $qb->expr()->andX( - $qb->expr()->eq('a.storage', $qb->createNamedParameter($this->previewFolder->getStorageId())), - $qb->expr()->isNull('b.fileid'), - $qb->expr()->like('a.path', $qb->createNamedParameter($like)), - $qb->expr()->eq('a.mimetype', $qb->createNamedParameter($this->mimeTypeLoader->getId('httpd/unix-directory'))) - ) - ); + ->where($qb->expr()->isNull('f.fileid')); if (!$this->isCLI) { $qb->setMaxResults(10); @@ -150,29 +94,38 @@ class BackgroundCleanupJob extends TimedJob { $cursor = $qb->executeQuery(); + $lastStorageId = null; + /** @var FileId[] $tmpResult */ + $tmpResult = []; while ($row = $cursor->fetch()) { - yield $row['name']; + if ($lastStorageId === null) { + $lastStorageId = $row['storage_id']; + } else if ($lastStorageId !== $row['storage_id']) { + yield [$lastStorageId => $tmpResult]; + $tmpResult = []; + $lastStorageId = $row['storage_id']; + } + $tmpResult[] = $row['file_id']; + } + + if (!empty($tmpResult)) { + yield [$lastStorageId => $tmpResult]; } $cursor->closeCursor(); } - private function getAllPreviewIds(string $previewRoot, int $chunkSize): \Iterator { - // See `getNewPreviewLocations` for some more info about the logic here - $like = $this->connection->escapeLikeParameter($previewRoot) . '/_/_/_/_/_/_/_/%'; - + /** + * @return \Iterator> + */ + private function getAllPreviewIds(int $chunkSize): \Iterator { $qb = $this->connection->getQueryBuilder(); - $qb->select('name', 'fileid') - ->from('filecache') + $qb->select('id', 'file_id', 'storage_id') + ->from('previews') ->where( - $qb->expr()->andX( - $qb->expr()->eq('storage', $qb->createNamedParameter($this->previewFolder->getStorageId())), - $qb->expr()->like('path', $qb->createNamedParameter($like)), - $qb->expr()->eq('mimetype', $qb->createNamedParameter($this->mimeTypeLoader->getId('httpd/unix-directory'))), - $qb->expr()->gt('fileid', $qb->createParameter('min_id')), - ) + $qb->expr()->gt('id', $qb->createParameter('min_id')), ) - ->orderBy('fileid', 'ASC') + ->orderBy('id', 'ASC') ->setMaxResults($chunkSize); $minId = 0; @@ -180,21 +133,33 @@ class BackgroundCleanupJob extends TimedJob { $qb->setParameter('min_id', $minId); $rows = $qb->executeQuery()->fetchAll(); if (count($rows) > 0) { - $minId = $rows[count($rows) - 1]['fileid']; - yield array_map(function ($row) { - return (int)$row['name']; - }, $rows); + $minId = $rows[count($rows) - 1]['id']; + $result = []; + foreach ($rows as $row) { + if (!isset($result[$row['storage_id']])) { + $result[$row['storage_id']] = []; + } + $result[$row['storage_id']][] = $row['file_id']; + } + yield $result; } else { break; } } } - private function findMissingSources(array $ids): array { + /** + * @param FileId[] $ids + * @return FileId[] + */ + private function findMissingSources(int $storage, array $ids): array { $qb = $this->connection->getQueryBuilder(); $qb->select('fileid') ->from('filecache') - ->where($qb->expr()->in('fileid', $qb->createNamedParameter($ids, IQueryBuilder::PARAM_INT_ARRAY))); + ->where($qb->expr()->andX( + $qb->expr()->in('fileid', $qb->createNamedParameter($ids, IQueryBuilder::PARAM_INT_ARRAY)), + $qb->expr()->eq('storage', $qb->createNamedParameter($storage, IQueryBuilder::PARAM_INT)), + )); $found = $qb->executeQuery()->fetchAll(\PDO::FETCH_COLUMN); return array_diff($ids, $found); } diff --git a/lib/private/Preview/Db/PreviewMapper.php b/lib/private/Preview/Db/PreviewMapper.php index 5faa508c8e0..62023638150 100644 --- a/lib/private/Preview/Db/PreviewMapper.php +++ b/lib/private/Preview/Db/PreviewMapper.php @@ -63,4 +63,34 @@ class PreviewMapper extends QBMapper { return null; } } + + /** + * @param int[] $fileIds + * @return array + */ + public function getByFileIds(int $storageId, array $fileIds): array { + $selectQb = $this->db->getQueryBuilder(); + $selectQb->select('*') + ->from(self::TABLE_NAME) + ->where($selectQb->expr()->andX( + $selectQb->expr()->in('file_id', $selectQb->createNamedParameter($fileIds, IQueryBuilder::PARAM_INT_ARRAY)), + )); + $previews = array_fill_keys($fileIds, []); + foreach ($this->yieldEntities($selectQb) as $preview) { + $previews[$preview->getFileId()][] = $preview; + } + return $previews; + } + + /** + * @param int[] $previewIds + */ + public function deleteByIds(int $storageId, array $previewIds): void { + $qb = $this->db->getQueryBuilder(); + $qb->delete(self::TABLE_NAME) + ->where($qb->expr()->andX( + $qb->expr()->eq('storage_id', $qb->createNamedParameter($storageId, IQueryBuilder::PARAM_INT)), + $qb->expr()->in('id', $qb->createNamedParameter($previewIds, IQueryBuilder::PARAM_INT_ARRAY)) + ))->executeStatement(); + } } diff --git a/lib/private/Preview/Generator.php b/lib/private/Preview/Generator.php index e846a8221ab..2dfbfa6db62 100644 --- a/lib/private/Preview/Generator.php +++ b/lib/private/Preview/Generator.php @@ -174,7 +174,6 @@ class Generator { if ($maxPreviewImage === null) { $maxPreviewImage = $this->helper->getImage(new PreviewFile($maxPreview, $this->storageFactory, $this->previewMapper)); } - assert($maxPreviewImage); $this->logger->debug('Cached preview not found for file {path}, generating a new preview.', ['path' => $file->getPath()]); $previewFile = $this->generatePreview($file, $maxPreviewImage, $width, $height, $crop, $maxWidth, $maxHeight, $previewVersion, $cacheResult); @@ -508,7 +507,6 @@ class Generator { self::unguardWithSemaphore($sem); } - $path = $this->generatePath($width, $height, $crop, false, $preview->dataMimeType(), $version); if ($cacheResult) { $previewEntry = $this->savePreview($file, $width, $height, $crop, false, $preview, $version); @@ -519,11 +517,9 @@ class Generator { } /** - * @param string $mimeType - * @return null|string * @throws \InvalidArgumentException */ - private function getExtension($mimeType) { + private function getExtension(string $mimeType): string { switch ($mimeType) { case 'image/png': return 'png'; diff --git a/lib/public/AppFramework/Db/QBMapper.php b/lib/public/AppFramework/Db/QBMapper.php index 7fb5b2a9afd..d80bb5aec8b 100644 --- a/lib/public/AppFramework/Db/QBMapper.php +++ b/lib/public/AppFramework/Db/QBMapper.php @@ -84,7 +84,6 @@ abstract class QBMapper { return $entity; } - /** * Creates a new entry in the db from an entity * diff --git a/lib/public/Preview/BeforePreviewFetchedEvent.php b/lib/public/Preview/BeforePreviewFetchedEvent.php index 8ab875070d9..69cd281ac02 100644 --- a/lib/public/Preview/BeforePreviewFetchedEvent.php +++ b/lib/public/Preview/BeforePreviewFetchedEvent.php @@ -21,6 +21,7 @@ use OCP\IPreview; */ class BeforePreviewFetchedEvent extends \OCP\EventDispatcher\Event { /** + * @param null|IPreview::MODE_FILL|IPreview::MODE_COVER $mode * @since 25.0.1 */ public function __construct( diff --git a/tests/lib/Preview/BackgroundCleanupJobTest.php b/tests/lib/Preview/BackgroundCleanupJobTest.php index ab904f2b499..89503085fa4 100644 --- a/tests/lib/Preview/BackgroundCleanupJobTest.php +++ b/tests/lib/Preview/BackgroundCleanupJobTest.php @@ -9,7 +9,10 @@ namespace Test\Preview; use OC\Files\Storage\Temporary; use OC\Preview\BackgroundCleanupJob; +use OC\Preview\Db\Preview; +use OC\Preview\Db\PreviewMapper; use OC\Preview\Storage\Root; +use OC\Preview\Storage\StorageFactory; use OC\PreviewManager; use OC\SystemConfig; use OCP\App\IAppManager; @@ -42,6 +45,8 @@ class BackgroundCleanupJobTest extends \Test\TestCase { private IRootFolder $rootFolder; private IMimeTypeLoader $mimeTypeLoader; private ITimeFactory $timeFactory; + private PreviewMapper $previewMapper; + private StorageFactory $previewStorageFactory; protected function setUp(): void { parent::setUp(); @@ -65,6 +70,8 @@ class BackgroundCleanupJobTest extends \Test\TestCase { $this->rootFolder = Server::get(IRootFolder::class); $this->mimeTypeLoader = Server::get(IMimeTypeLoader::class); $this->timeFactory = Server::get(ITimeFactory::class); + $this->previewMapper = Server::get(PreviewMapper::class); + $this->previewStorageFactory = Server::get(StorageFactory::class); } protected function tearDown(): void { @@ -78,13 +85,6 @@ class BackgroundCleanupJobTest extends \Test\TestCase { parent::tearDown(); } - private function getRoot(): Root { - return new Root( - Server::get(IRootFolder::class), - Server::get(SystemConfig::class) - ); - } - private function setup11Previews(): array { $userFolder = $this->rootFolder->getUserFolder($this->userId); @@ -99,130 +99,50 @@ class BackgroundCleanupJobTest extends \Test\TestCase { return $files; } - private function countPreviews(Root $previewRoot, array $fileIds): int { - $i = 0; - - foreach ($fileIds as $fileId) { - try { - $previewRoot->getFolder((string)$fileId); - } catch (NotFoundException $e) { - continue; - } - - $i++; - } - - return $i; + private function countPreviews(PreviewMapper $previewMapper, array $fileIds): int { + $previews = $previewMapper->getAvailablePreviews($fileIds); + return array_reduce($previews, fn (int $result, array $previews) => $result + count($previews), 0); } public function testCleanupSystemCron(): void { $files = $this->setup11Previews(); - $fileIds = array_map(function (File $f) { - return $f->getId(); - }, $files); - - $root = $this->getRoot(); + $fileIds = array_map(fn (File $f): int => $f->getId(), $files); - $this->assertSame(11, $this->countPreviews($root, $fileIds)); - $job = new BackgroundCleanupJob($this->timeFactory, $this->connection, $root, $this->mimeTypeLoader, true); + $this->assertSame(11, $this->countPreviews($this->previewMapper, $fileIds)); + $job = new BackgroundCleanupJob($this->timeFactory, $this->connection, $this->previewMapper, $this->previewStorageFactory, true); $job->run([]); foreach ($files as $file) { $file->delete(); } - $root = $this->getRoot(); - $this->assertSame(11, $this->countPreviews($root, $fileIds)); + $this->assertSame(11, $this->countPreviews($this->previewMapper, $fileIds)); $job->run([]); - $root = $this->getRoot(); - $this->assertSame(0, $this->countPreviews($root, $fileIds)); + $this->assertSame(0, $this->countPreviews($this->previewMapper, $fileIds)); } public function testCleanupAjax(): void { if ($this->connection->getShardDefinition('filecache')) { $this->markTestSkipped('ajax cron is not supported for sharded setups'); - return; } $files = $this->setup11Previews(); - $fileIds = array_map(function (File $f) { - return $f->getId(); - }, $files); - - $root = $this->getRoot(); + $fileIds = array_map(fn (File $f): int => $f->getId(), $files); - $this->assertSame(11, $this->countPreviews($root, $fileIds)); - $job = new BackgroundCleanupJob($this->timeFactory, $this->connection, $root, $this->mimeTypeLoader, false); + $this->assertSame(11, $this->countPreviews($this->previewMapper, $fileIds)); + $job = new BackgroundCleanupJob($this->timeFactory, $this->connection, $this->previewMapper, $this->previewStorageFactory, false); $job->run([]); foreach ($files as $file) { $file->delete(); } - $root = $this->getRoot(); - $this->assertSame(11, $this->countPreviews($root, $fileIds)); - $job->run([]); - - $root = $this->getRoot(); - $this->assertSame(1, $this->countPreviews($root, $fileIds)); + $this->assertSame(11, $this->countPreviews($this->previewMapper, $fileIds)); $job->run([]); - $root = $this->getRoot(); - $this->assertSame(0, $this->countPreviews($root, $fileIds)); - } - - public function testOldPreviews(): void { - if ($this->connection->getShardDefinition('filecache')) { - $this->markTestSkipped('old previews are not supported for sharded setups'); - return; - } - $appdata = Server::get(IAppDataFactory::class)->get('preview'); - - $f1 = $appdata->newFolder('123456781'); - $f1->newFile('foo.jpg', 'foo'); - $f2 = $appdata->newFolder('123456782'); - $f2->newFile('foo.jpg', 'foo'); - $f2 = $appdata->newFolder((string)PHP_INT_MAX - 1); - $f2->newFile('foo.jpg', 'foo'); - - /* - * Cleanup of OldPreviewLocations should only remove numeric folders on AppData level, - * therefore these files should stay untouched. - */ - $appdata->getFolder('/')->newFile('not-a-directory', 'foo'); - $appdata->getFolder('/')->newFile('133742', 'bar'); - - $appdata = Server::get(IAppDataFactory::class)->get('preview'); - // AppData::getDirectoryListing filters all non-folders - $this->assertSame(3, count($appdata->getDirectoryListing())); - try { - $appdata->getFolder('/')->getFile('not-a-directory'); - } catch (NotFoundException) { - $this->fail('Could not find file \'not-a-directory\''); - } - try { - $appdata->getFolder('/')->getFile('133742'); - } catch (NotFoundException) { - $this->fail('Could not find file \'133742\''); - } - - $job = new BackgroundCleanupJob($this->timeFactory, $this->connection, $this->getRoot(), $this->mimeTypeLoader, true); + $this->assertSame(1, $this->countPreviews($this->previewMapper, $fileIds)); $job->run([]); - $appdata = Server::get(IAppDataFactory::class)->get('preview'); - - // Check if the files created above are still present - // Remember: AppData::getDirectoryListing filters all non-folders - $this->assertSame(0, count($appdata->getDirectoryListing())); - try { - $appdata->getFolder('/')->getFile('not-a-directory'); - } catch (NotFoundException) { - $this->fail('Could not find file \'not-a-directory\''); - } - try { - $appdata->getFolder('/')->getFile('133742'); - } catch (NotFoundException) { - $this->fail('Could not find file \'133742\''); - } + $this->assertSame(0, $this->countPreviews($this->previewMapper, $fileIds)); } }