From 8dd6e00222b3e1d7e2bf852a396262505861ae0c Mon Sep 17 00:00:00 2001 From: Louis Chmn Date: Fri, 7 Nov 2025 18:13:14 +0100 Subject: [PATCH 1/2] chore(Scanner): Use modern syntax and APIs Signed-off-by: Louis Chmn --- lib/private/Files/Utils/Scanner.php | 85 ++++++++++++----------------- 1 file changed, 34 insertions(+), 51 deletions(-) diff --git a/lib/private/Files/Utils/Scanner.php b/lib/private/Files/Utils/Scanner.php index 576cb66b3cf9f..d18bd5357dca5 100644 --- a/lib/private/Files/Utils/Scanner.php +++ b/lib/private/Files/Utils/Scanner.php @@ -9,6 +9,8 @@ use OC\Files\Cache\Cache; use OC\Files\Filesystem; +use OC\Files\Mount\MountPoint; +use OC\Files\SetupManager; use OC\Files\Storage\FailedStorage; use OC\Files\Storage\Home; use OC\ForbiddenException; @@ -28,6 +30,7 @@ use OCP\Files\Storage\IStorage; use OCP\Files\StorageNotAvailableException; use OCP\IDBConnection; +use OCP\IUserManager; use OCP\Lock\ILockingProvider; use OCP\Lock\LockedException; use Psr\Log\LoggerInterface; @@ -42,45 +45,27 @@ * @package OC\Files\Utils */ class Scanner extends PublicEmitter { - public const MAX_ENTRIES_TO_COMMIT = 10000; - - /** @var string $user */ - private $user; - - /** @var IDBConnection */ - protected $db; - - /** @var IEventDispatcher */ - private $dispatcher; - - protected LoggerInterface $logger; - /** * Whether to use a DB transaction - * - * @var bool */ - protected $useTransaction; + protected bool $useTransaction; /** * Number of entries scanned to commit - * - * @var int */ - protected $entriesToCommit; + protected int $entriesToCommit; - /** - * @param string $user - * @param IDBConnection|null $db - * @param IEventDispatcher $dispatcher - */ - public function __construct($user, $db, IEventDispatcher $dispatcher, LoggerInterface $logger) { - $this->user = $user; - $this->db = $db; - $this->dispatcher = $dispatcher; - $this->logger = $logger; + public function __construct( + private readonly string $user, + private readonly ?IDBConnection $db, + private readonly IEventDispatcher $dispatcher, + private readonly LoggerInterface $logger, + private readonly SetupManager $setupManager, + private readonly IUserManager $userManager, + private readonly IEventDispatcher $eventDispatcher, + ) { // when DB locking is used, no DB transactions will be used - $this->useTransaction = !(\OC::$server->get(ILockingProvider::class) instanceof DBLockingProvider); + $this->useTransaction = !(\OCP\Server::get(ILockingProvider::class) instanceof DBLockingProvider); } /** @@ -91,8 +76,14 @@ public function __construct($user, $db, IEventDispatcher $dispatcher, LoggerInte */ protected function getMounts($dir) { //TODO: move to the node based fileapi once that's done - \OC_Util::tearDownFS(); - \OC_Util::setupFS($this->user); + $this->setupManager->tearDown(); + + $userObject = $this->userManager->get($this->user); + if ($userObject === null) { + throw new \InvalidArgumentException("User {$this->user} does not exist"); + } + + $this->setupManager->setupForUser($userObject); $mountManager = Filesystem::getMountManager(); $mounts = $mountManager->findIn($dir); @@ -105,37 +96,32 @@ protected function getMounts($dir) { /** * attach listeners to the scanner - * - * @param \OC\Files\Mount\MountPoint $mount */ - protected function attachListener($mount) { + protected function attachListener(MountPoint $mount) { /** @var \OC\Files\Cache\Scanner $scanner */ $scanner = $mount->getStorage()->getScanner(); $scanner->listen('\OC\Files\Cache\Scanner', 'scanFile', function ($path) use ($mount) { $this->emit('\OC\Files\Utils\Scanner', 'scanFile', [$mount->getMountPoint() . $path]); - $this->dispatcher->dispatchTyped(new BeforeFileScannedEvent($mount->getMountPoint() . $path)); + $this->eventDispatcher->dispatchTyped(new BeforeFileScannedEvent($mount->getMountPoint() . $path)); }); $scanner->listen('\OC\Files\Cache\Scanner', 'scanFolder', function ($path) use ($mount) { $this->emit('\OC\Files\Utils\Scanner', 'scanFolder', [$mount->getMountPoint() . $path]); - $this->dispatcher->dispatchTyped(new BeforeFolderScannedEvent($mount->getMountPoint() . $path)); + $this->eventDispatcher->dispatchTyped(new BeforeFolderScannedEvent($mount->getMountPoint() . $path)); }); $scanner->listen('\OC\Files\Cache\Scanner', 'postScanFile', function ($path) use ($mount) { $this->emit('\OC\Files\Utils\Scanner', 'postScanFile', [$mount->getMountPoint() . $path]); - $this->dispatcher->dispatchTyped(new FileScannedEvent($mount->getMountPoint() . $path)); + $this->eventDispatcher->dispatchTyped(new FileScannedEvent($mount->getMountPoint() . $path)); }); $scanner->listen('\OC\Files\Cache\Scanner', 'postScanFolder', function ($path) use ($mount) { $this->emit('\OC\Files\Utils\Scanner', 'postScanFolder', [$mount->getMountPoint() . $path]); - $this->dispatcher->dispatchTyped(new FolderScannedEvent($mount->getMountPoint() . $path)); + $this->eventDispatcher->dispatchTyped(new FolderScannedEvent($mount->getMountPoint() . $path)); }); $scanner->listen('\OC\Files\Cache\Scanner', 'normalizedNameMismatch', function ($path) use ($mount) { $this->emit('\OC\Files\Utils\Scanner', 'normalizedNameMismatch', [$path]); }); } - /** - * @param string $dir - */ - public function backgroundScan($dir) { + public function backgroundScan(string $dir) { $mounts = $this->getMounts($dir); foreach ($mounts as $mount) { try { @@ -174,13 +160,10 @@ public function backgroundScan($dir) { } /** - * @param string $dir - * @param $recursive - * @param callable|null $mountFilter * @throws ForbiddenException * @throws NotFoundException */ - public function scan($dir = '', $recursive = \OC\Files\Cache\Scanner::SCAN_RECURSIVE, ?callable $mountFilter = null) { + public function scan(string $dir = '', $recursive = \OC\Files\Cache\Scanner::SCAN_RECURSIVE, ?callable $mountFilter = null) { if (!Filesystem::isValidPath($dir)) { throw new \InvalidArgumentException('Invalid path to scan'); } @@ -236,18 +219,18 @@ public function scan($dir = '', $recursive = \OC\Files\Cache\Scanner::SCAN_RECUR $scanner->listen('\OC\Files\Cache\Scanner', 'removeFromCache', function ($path) use ($storage) { $this->postProcessEntry($storage, $path); - $this->dispatcher->dispatchTyped(new NodeRemovedFromCache($storage, $path)); + $this->eventDispatcher->dispatchTyped(new NodeRemovedFromCache($storage, $path)); }); $scanner->listen('\OC\Files\Cache\Scanner', 'updateCache', function ($path) use ($storage) { $this->postProcessEntry($storage, $path); - $this->dispatcher->dispatchTyped(new FileCacheUpdated($storage, $path)); + $this->eventDispatcher->dispatchTyped(new FileCacheUpdated($storage, $path)); }); $scanner->listen('\OC\Files\Cache\Scanner', 'addToCache', function ($path, $storageId, $data, $fileId) use ($storage) { $this->postProcessEntry($storage, $path); if ($fileId) { - $this->dispatcher->dispatchTyped(new FileCacheUpdated($storage, $path)); + $this->eventDispatcher->dispatchTyped(new FileCacheUpdated($storage, $path)); } else { - $this->dispatcher->dispatchTyped(new NodeAddedToCache($storage, $path)); + $this->eventDispatcher->dispatchTyped(new NodeAddedToCache($storage, $path)); } }); From d63a2454598c87af7a9d83fe41eedcd27c3422d4 Mon Sep 17 00:00:00 2001 From: Louis Chmn Date: Fri, 7 Nov 2025 16:41:30 +0100 Subject: [PATCH 2/2] fix(Scanner): Use time based limit for the transaction Using the node count does not help if handling a node is slow. Signed-off-by: Louis Chmn --- apps/files/lib/BackgroundJob/ScanFiles.php | 11 ++- apps/files/lib/Command/Scan.php | 8 ++- apps/files/lib/Command/ScanAppData.php | 9 ++- .../tests/BackgroundJob/ScanFilesTest.php | 7 +- build/psalm-baseline.xml | 3 - lib/private/Files/Utils/Scanner.php | 42 ++++++------ tests/lib/Files/EtagTest.php | 13 +++- tests/lib/Files/Utils/ScannerTest.php | 68 +++++++++++++++++-- 8 files changed, 126 insertions(+), 35 deletions(-) diff --git a/apps/files/lib/BackgroundJob/ScanFiles.php b/apps/files/lib/BackgroundJob/ScanFiles.php index f3f9093d64819..45bb66102a60c 100644 --- a/apps/files/lib/BackgroundJob/ScanFiles.php +++ b/apps/files/lib/BackgroundJob/ScanFiles.php @@ -8,6 +8,7 @@ namespace OCA\Files\BackgroundJob; +use OC\Files\SetupManager; use OC\Files\Utils\Scanner; use OCP\AppFramework\Utility\ITimeFactory; use OCP\BackgroundJob\TimedJob; @@ -15,6 +16,7 @@ use OCP\EventDispatcher\IEventDispatcher; use OCP\IConfig; use OCP\IDBConnection; +use OCP\IUserManager; use Psr\Log\LoggerInterface; /** @@ -33,6 +35,9 @@ public function __construct( private LoggerInterface $logger, private IDBConnection $connection, ITimeFactory $time, + private readonly SetupManager $setupManager, + private readonly IUserManager $userManager, + private readonly IEventDispatcher $eventDispatcher, ) { parent::__construct($time); // Run once per 10 minutes @@ -45,7 +50,11 @@ protected function runScanner(string $user): void { $user, null, $this->dispatcher, - $this->logger + $this->logger, + $this->setupManager, + $this->userManager, + $this->eventDispatcher, + $this->time, ); $scanner->backgroundScan(''); } catch (\Exception $e) { diff --git a/apps/files/lib/Command/Scan.php b/apps/files/lib/Command/Scan.php index b9057139b0e16..3f17fa792e319 100644 --- a/apps/files/lib/Command/Scan.php +++ b/apps/files/lib/Command/Scan.php @@ -11,10 +11,12 @@ use OC\Core\Command\InterruptedException; use OC\DB\Connection; use OC\DB\ConnectionAdapter; +use OC\Files\SetupManager; use OC\Files\Storage\Wrapper\Jail; use OC\Files\Utils\Scanner; use OC\FilesMetadata\FilesMetadataManager; use OC\ForbiddenException; +use OCP\AppFramework\Utility\ITimeFactory; use OCP\EventDispatcher\IEventDispatcher; use OCP\Files\Events\FileCacheUpdated; use OCP\Files\Events\NodeAddedToCache; @@ -114,7 +116,11 @@ protected function scanFiles( $user, new ConnectionAdapter($connection), Server::get(IEventDispatcher::class), - Server::get(LoggerInterface::class) + Server::get(LoggerInterface::class), + Server::get(SetupManager::class), + Server::get(IUserManager::class), + Server::get(IEventDispatcher::class), + Server::get(ITimeFactory::class), ); # check on each file/folder if there was a user interrupt (ctrl-c) and throw an exception diff --git a/apps/files/lib/Command/ScanAppData.php b/apps/files/lib/Command/ScanAppData.php index 23604a82df9b5..583110314edde 100644 --- a/apps/files/lib/Command/ScanAppData.php +++ b/apps/files/lib/Command/ScanAppData.php @@ -10,9 +10,11 @@ use OC\Core\Command\InterruptedException; use OC\DB\Connection; use OC\DB\ConnectionAdapter; +use OC\Files\SetupManager; use OC\Files\Utils\Scanner; use OC\ForbiddenException; use OC\Preview\Storage\StorageFactory; +use OCP\AppFramework\Utility\ITimeFactory; use OCP\EventDispatcher\IEventDispatcher; use OCP\Files\Folder; use OCP\Files\IRootFolder; @@ -20,6 +22,7 @@ use OCP\Files\NotFoundException; use OCP\Files\StorageNotAvailableException; use OCP\IConfig; +use OCP\IUserManager; use OCP\Server; use Psr\Log\LoggerInterface; use Symfony\Component\Console\Helper\Table; @@ -84,7 +87,11 @@ protected function scanFiles(OutputInterface $output, string $folder): int { null, new ConnectionAdapter($connection), Server::get(IEventDispatcher::class), - Server::get(LoggerInterface::class) + Server::get(LoggerInterface::class), + Server::get(SetupManager::class), + Server::get(IUserManager::class), + Server::get(IEventDispatcher::class), + Server::get(ITimeFactory::class), ); # check on each file/folder if there was a user interrupt (ctrl-c) and throw an exception diff --git a/apps/files/tests/BackgroundJob/ScanFilesTest.php b/apps/files/tests/BackgroundJob/ScanFilesTest.php index 22ec049741b2b..e6148d9ced83a 100644 --- a/apps/files/tests/BackgroundJob/ScanFilesTest.php +++ b/apps/files/tests/BackgroundJob/ScanFilesTest.php @@ -9,6 +9,7 @@ namespace OCA\Files\Tests\BackgroundJob; use OC\Files\Mount\MountPoint; +use OC\Files\SetupManager; use OC\Files\Storage\Temporary; use OCA\Files\BackgroundJob\ScanFiles; use OCP\AppFramework\Utility\ITimeFactory; @@ -17,6 +18,7 @@ use OCP\IConfig; use OCP\IDBConnection; use OCP\IUser; +use OCP\IUserManager; use OCP\Server; use Psr\Log\LoggerInterface; use Test\TestCase; @@ -51,7 +53,10 @@ protected function setUp(): void { $dispatcher, $logger, $connection, - $this->createMock(ITimeFactory::class) + $this->createMock(ITimeFactory::class), + $this->createMock(SetupManager::class), + $this->createMock(IUserManager::class), + $this->createMock(IEventDispatcher::class), ]) ->onlyMethods(['runScanner']) ->getMock(); diff --git a/build/psalm-baseline.xml b/build/psalm-baseline.xml index 71d9fe016a481..d79045123a0d8 100644 --- a/build/psalm-baseline.xml +++ b/build/psalm-baseline.xml @@ -1366,9 +1366,6 @@ - - - diff --git a/lib/private/Files/Utils/Scanner.php b/lib/private/Files/Utils/Scanner.php index d18bd5357dca5..9521b82381ab5 100644 --- a/lib/private/Files/Utils/Scanner.php +++ b/lib/private/Files/Utils/Scanner.php @@ -17,6 +17,7 @@ use OC\Hooks\PublicEmitter; use OC\Lock\DBLockingProvider; use OCA\Files_Sharing\SharedStorage; +use OCP\AppFramework\Utility\ITimeFactory; use OCP\EventDispatcher\IEventDispatcher; use OCP\Files\Events\BeforeFileScannedEvent; use OCP\Files\Events\BeforeFolderScannedEvent; @@ -45,24 +46,23 @@ * @package OC\Files\Utils */ class Scanner extends PublicEmitter { + private const TRANSACTION_SECOND_TIMEOUT = 10; + /** * Whether to use a DB transaction */ protected bool $useTransaction; - - /** - * Number of entries scanned to commit - */ - protected int $entriesToCommit; + protected int $transactionStartTime; public function __construct( - private readonly string $user, + private readonly ?string $user, private readonly ?IDBConnection $db, private readonly IEventDispatcher $dispatcher, private readonly LoggerInterface $logger, private readonly SetupManager $setupManager, private readonly IUserManager $userManager, private readonly IEventDispatcher $eventDispatcher, + private readonly ITimeFactory $timeFactory, ) { // when DB locking is used, no DB transactions will be used $this->useTransaction = !(\OCP\Server::get(ILockingProvider::class) instanceof DBLockingProvider); @@ -78,13 +78,11 @@ protected function getMounts($dir) { //TODO: move to the node based fileapi once that's done $this->setupManager->tearDown(); - $userObject = $this->userManager->get($this->user); - if ($userObject === null) { - throw new \InvalidArgumentException("User {$this->user} does not exist"); + if ($this->user !== null) { + $userObject = $this->userManager->get($this->user); + $this->setupManager->setupForUser($userObject); } - $this->setupManager->setupForUser($userObject); - $mountManager = Filesystem::getMountManager(); $mounts = $mountManager->findIn($dir); $mounts[] = $mountManager->find($dir); @@ -239,6 +237,7 @@ public function scan(string $dir = '', $recursive = \OC\Files\Cache\Scanner::SCA } if ($this->useTransaction) { + $this->transactionStartTime = $this->timeFactory->getTime(); $this->db->beginTransaction(); } try { @@ -275,16 +274,17 @@ private function triggerPropagator(IStorage $storage, $internalPath) { private function postProcessEntry(IStorage $storage, $internalPath) { $this->triggerPropagator($storage, $internalPath); - if ($this->useTransaction) { - $this->entriesToCommit++; - if ($this->entriesToCommit >= self::MAX_ENTRIES_TO_COMMIT) { - $propagator = $storage->getPropagator(); - $this->entriesToCommit = 0; - $this->db->commit(); - $propagator->commitBatch(); - $this->db->beginTransaction(); - $propagator->beginBatch(); - } + + if ( + $this->useTransaction + && $this->transactionStartTime + self::TRANSACTION_SECOND_TIMEOUT <= $this->timeFactory->getTime() + ) { + $propagator = $storage->getPropagator(); + $this->db->commit(); + $propagator->commitBatch(); + $this->db->beginTransaction(); + $propagator->beginBatch(); + $this->transactionStartTime = $this->timeFactory->getTime(); } } } diff --git a/tests/lib/Files/EtagTest.php b/tests/lib/Files/EtagTest.php index ccf1319a6a87f..36040de98289d 100644 --- a/tests/lib/Files/EtagTest.php +++ b/tests/lib/Files/EtagTest.php @@ -9,9 +9,11 @@ namespace Test\Files; use OC\Files\Filesystem; +use OC\Files\SetupManager; use OC\Files\Utils\Scanner; use OC\Share\Share; use OCA\Files_Sharing\AppInfo\Application; +use OCP\AppFramework\Utility\ITimeFactory; use OCP\EventDispatcher\IEventDispatcher; use OCP\IConfig; use OCP\IDBConnection; @@ -77,7 +79,16 @@ public function testNewUser(): void { $files = ['/foo.txt', '/folder/bar.txt', '/folder/subfolder', '/folder/subfolder/qwerty.txt']; $originalEtags = $this->getEtags($files); - $scanner = new Scanner($user1, Server::get(IDBConnection::class), Server::get(IEventDispatcher::class), Server::get(LoggerInterface::class)); + $scanner = new Scanner( + $user1, + Server::get(IDBConnection::class), + Server::get(IEventDispatcher::class), + Server::get(LoggerInterface::class), + Server::get(SetupManager::class), + Server::get(IUserManager::class), + Server::get(IEventDispatcher::class), + Server::get(ITimeFactory::class) + ); $scanner->backgroundScan('/'); $newEtags = $this->getEtags($files); diff --git a/tests/lib/Files/Utils/ScannerTest.php b/tests/lib/Files/Utils/ScannerTest.php index 3e60c0234cbc8..0e82cb3896a03 100644 --- a/tests/lib/Files/Utils/ScannerTest.php +++ b/tests/lib/Files/Utils/ScannerTest.php @@ -10,8 +10,10 @@ use OC\Files\Filesystem; use OC\Files\Mount\MountPoint; +use OC\Files\SetupManager; use OC\Files\Storage\Temporary; use OC\Files\Utils\Scanner; +use OCP\AppFramework\Utility\ITimeFactory; use OCP\EventDispatcher\IEventDispatcher; use OCP\Files\Config\IMountProvider; use OCP\Files\Config\IMountProviderCollection; @@ -77,7 +79,16 @@ public function testReuseExistingRoot(): void { $storage->file_put_contents('foo.txt', 'qwerty'); $storage->file_put_contents('folder/bar.txt', 'qwerty'); - $scanner = new TestScanner('', Server::get(IDBConnection::class), $this->createMock(IEventDispatcher::class), Server::get(LoggerInterface::class)); + $scanner = new TestScanner( + '', + Server::get(IDBConnection::class), + $this->createMock(IEventDispatcher::class), + Server::get(LoggerInterface::class), + Server::get(SetupManager::class), + Server::get(IUserManager::class), + Server::get(IEventDispatcher::class), + Server::get(ITimeFactory::class), + ); $scanner->addMount($mount); $scanner->scan(''); @@ -99,7 +110,16 @@ public function testReuseExistingFile(): void { $storage->file_put_contents('foo.txt', 'qwerty'); $storage->file_put_contents('folder/bar.txt', 'qwerty'); - $scanner = new TestScanner('', Server::get(IDBConnection::class), $this->createMock(IEventDispatcher::class), Server::get(LoggerInterface::class)); + $scanner = new TestScanner( + '', + Server::get(IDBConnection::class), + $this->createMock(IEventDispatcher::class), + Server::get(LoggerInterface::class), + Server::get(SetupManager::class), + Server::get(IUserManager::class), + Server::get(IEventDispatcher::class), + Server::get(ITimeFactory::class), + ); $scanner->addMount($mount); $scanner->scan(''); @@ -137,7 +157,16 @@ public function testScanSubMount(): void { $storage->file_put_contents('foo.txt', 'qwerty'); $storage->file_put_contents('folder/bar.txt', 'qwerty'); - $scanner = new Scanner($uid, Server::get(IDBConnection::class), Server::get(IEventDispatcher::class), Server::get(LoggerInterface::class)); + $scanner = new Scanner( + $uid, + Server::get(IDBConnection::class), + Server::get(IEventDispatcher::class), + Server::get(LoggerInterface::class), + Server::get(SetupManager::class), + Server::get(IUserManager::class), + Server::get(IEventDispatcher::class), + Server::get(ITimeFactory::class), + ); $this->assertFalse($cache->inCache('folder/bar.txt')); $scanner->scan('/' . $uid . '/files/foo'); @@ -166,7 +195,16 @@ public function testInvalidPathScanning($invalidPath): void { $this->expectException(\InvalidArgumentException::class); $this->expectExceptionMessage('Invalid path to scan'); - $scanner = new TestScanner('', Server::get(IDBConnection::class), $this->createMock(IEventDispatcher::class), Server::get(LoggerInterface::class)); + $scanner = new TestScanner( + '', + Server::get(IDBConnection::class), + $this->createMock(IEventDispatcher::class), + Server::get(LoggerInterface::class), + Server::get(SetupManager::class), + Server::get(IUserManager::class), + Server::get(IEventDispatcher::class), + Server::get(ITimeFactory::class), + ); $scanner->scan($invalidPath); } @@ -180,7 +218,16 @@ public function testPropagateEtag(): void { $storage->file_put_contents('folder/bar.txt', 'qwerty'); $storage->touch('folder/bar.txt', time() - 200); - $scanner = new TestScanner('', Server::get(IDBConnection::class), $this->createMock(IEventDispatcher::class), Server::get(LoggerInterface::class)); + $scanner = new TestScanner( + '', + Server::get(IDBConnection::class), + $this->createMock(IEventDispatcher::class), + Server::get(LoggerInterface::class), + Server::get(SetupManager::class), + Server::get(IUserManager::class), + Server::get(IEventDispatcher::class), + Server::get(ITimeFactory::class), + ); $scanner->addMount($mount); $scanner->scan(''); @@ -206,7 +253,16 @@ public function testShallow(): void { $storage->file_put_contents('folder/bar.txt', 'qwerty'); $storage->file_put_contents('folder/subfolder/foobar.txt', 'qwerty'); - $scanner = new TestScanner('', Server::get(IDBConnection::class), $this->createMock(IEventDispatcher::class), Server::get(LoggerInterface::class)); + $scanner = new TestScanner( + '', + Server::get(IDBConnection::class), + $this->createMock(IEventDispatcher::class), + Server::get(LoggerInterface::class), + Server::get(SetupManager::class), + Server::get(IUserManager::class), + Server::get(IEventDispatcher::class), + Server::get(ITimeFactory::class), + ); $scanner->addMount($mount); $scanner->scan('', $recusive = false);