From d52328660eadac6fcd2183ee523c51e49f656914 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Tue, 22 Sep 2020 15:03:17 +0200 Subject: [PATCH] only run the background scanner for users that have unscanned files instead of looping trough all users and seeing if they have unscanned files, we do a single query to find all storages that need scanning and run trough the users for them Signed-off-by: Robin Appelman --- apps/files/lib/BackgroundJob/ScanFiles.php | 67 +++++--- .../tests/BackgroundJob/ScanFilesTest.php | 157 +++++++----------- 2 files changed, 102 insertions(+), 122 deletions(-) diff --git a/apps/files/lib/BackgroundJob/ScanFiles.php b/apps/files/lib/BackgroundJob/ScanFiles.php index b9f37ca811..127ca758a1 100644 --- a/apps/files/lib/BackgroundJob/ScanFiles.php +++ b/apps/files/lib/BackgroundJob/ScanFiles.php @@ -25,10 +25,11 @@ namespace OCA\Files\BackgroundJob; use OC\Files\Utils\Scanner; +use OCP\DB\QueryBuilder\IQueryBuilder; use OCP\EventDispatcher\IEventDispatcher; use OCP\IConfig; +use OCP\IDBConnection; use OCP\ILogger; -use OCP\IUser; use OCP\IUserManager; /** @@ -46,36 +47,42 @@ class ScanFiles extends \OC\BackgroundJob\TimedJob { private $dispatcher; /** @var ILogger */ private $logger; + private $connection; /** Amount of users that should get scanned per execution */ public const USERS_PER_SESSION = 500; /** - * @param IConfig|null $config - * @param IUserManager|null $userManager - * @param IEventDispatcher|null $dispatcher - * @param ILogger|null $logger + * @param IConfig $config + * @param IUserManager $userManager + * @param IEventDispatcher $dispatcher + * @param ILogger $logger + * @param IDBConnection $connection */ - public function __construct(IConfig $config = null, - IUserManager $userManager = null, - IEventDispatcher $dispatcher = null, - ILogger $logger = null) { + public function __construct( + IConfig $config, + IUserManager $userManager, + IEventDispatcher $dispatcher, + ILogger $logger, + IDBConnection $connection + ) { // Run once per 10 minutes $this->setInterval(60 * 10); - $this->config = $config ?? \OC::$server->getConfig(); - $this->userManager = $userManager ?? \OC::$server->getUserManager(); - $this->dispatcher = $dispatcher ?? \OC::$server->query(IEventDispatcher::class); - $this->logger = $logger ?? \OC::$server->getLogger(); + $this->config = $config; + $this->userManager = $userManager; + $this->dispatcher = $dispatcher; + $this->logger = $logger; + $this->connection = $connection; } /** - * @param IUser $user + * @param string $user */ - protected function runScanner(IUser $user) { + protected function runScanner(string $user) { try { $scanner = new Scanner( - $user->getUID(), + $user, null, $this->dispatcher, $this->logger @@ -87,6 +94,23 @@ class ScanFiles extends \OC\BackgroundJob\TimedJob { \OC_Util::tearDownFS(); } + /** + * Find all storages which have unindexed files and return a user for each + * + * @return string[] + */ + private function getUsersToScan(): array { + $query = $this->connection->getQueryBuilder(); + $query->select($query->func()->max('user_id')) + ->from('filecache', 'f') + ->innerJoin('f', 'mounts', 'm', $query->expr()->eq('storage_id', 'storage')) + ->where($query->expr()->lt('size', $query->createNamedParameter(0, IQueryBuilder::PARAM_INT))) + ->groupBy('storage_id') + ->setMaxResults(self::USERS_PER_SESSION); + + return $query->execute()->fetchAll(\PDO::FETCH_COLUMN); + } + /** * @param $argument * @throws \Exception @@ -96,16 +120,7 @@ class ScanFiles extends \OC\BackgroundJob\TimedJob { return; } - $offset = $this->config->getAppValue('files', 'cronjob_scan_files', 0); - $users = $this->userManager->search('', self::USERS_PER_SESSION, $offset); - if (!count($users)) { - // No users found, reset offset and retry - $offset = 0; - $users = $this->userManager->search('', self::USERS_PER_SESSION); - } - - $offset += self::USERS_PER_SESSION; - $this->config->setAppValue('files', 'cronjob_scan_files', $offset); + $users = $this->getUsersToScan(); foreach ($users as $user) { $this->runScanner($user); diff --git a/apps/files/tests/BackgroundJob/ScanFilesTest.php b/apps/files/tests/BackgroundJob/ScanFilesTest.php index 75557955d6..1bc449c6c6 100644 --- a/apps/files/tests/BackgroundJob/ScanFilesTest.php +++ b/apps/files/tests/BackgroundJob/ScanFilesTest.php @@ -25,129 +25,94 @@ namespace OCA\Files\Tests\BackgroundJob; +use OC\Files\Mount\MountPoint; +use OC\Files\Storage\Temporary; use OCA\Files\BackgroundJob\ScanFiles; +use OCP\EventDispatcher\IEventDispatcher; use OCP\IConfig; +use OCP\ILogger; use OCP\IUser; use OCP\IUserManager; use Test\TestCase; +use Test\Traits\MountProviderTrait; +use Test\Traits\UserTrait; /** * Class ScanFilesTest * * @package OCA\Files\Tests\BackgroundJob + * @group DB */ class ScanFilesTest extends TestCase { - /** @var IConfig */ - private $config; - /** @var IUserManager */ - private $userManager; + use UserTrait; + use MountProviderTrait; + /** @var ScanFiles */ private $scanFiles; + /** @var \OCP\Files\Config\IUserMountCache */ + private $mountCache; protected function setUp(): void { parent::setUp(); - $this->config = $this->createMock(IConfig::class); - $this->userManager = $this->createMock(IUserManager::class); + $config = $this->createMock(IConfig::class); + $userManager = $this->createMock(IUserManager::class); + $dispatcher = $this->createMock(IEventDispatcher::class); + $logger = $this->createMock(ILogger::class); + $connection = \OC::$server->getDatabaseConnection(); + $this->mountCache = \OC::$server->getUserMountCache(); $this->scanFiles = $this->getMockBuilder('\OCA\Files\BackgroundJob\ScanFiles') - ->setConstructorArgs([ - $this->config, - $this->userManager, - ]) - ->setMethods(['runScanner']) - ->getMock(); + ->setConstructorArgs([ + $config, + $userManager, + $dispatcher, + $logger, + $connection, + ]) + ->setMethods(['runScanner']) + ->getMock(); } - public function testRunWithoutUsers() { - $this->config - ->expects($this->at(0)) - ->method('getSystemValueBool') - ->with('files_no_background_scan', false) - ->willReturn(false); - $this->config - ->expects($this->at(1)) - ->method('getAppValue') - ->with('files', 'cronjob_scan_files', 0) - ->willReturn(50); - $this->userManager - ->expects($this->at(0)) - ->method('search') - ->with('', 500, 50) - ->willReturn([]); - $this->userManager - ->expects($this->at(1)) - ->method('search') - ->with('', 500) - ->willReturn([]); - $this->config - ->expects($this->at(2)) - ->method('setAppValue') - ->with('files', 'cronjob_scan_files', 500); - + private function runJob() { $this->invokePrivate($this->scanFiles, 'run', [[]]); } - public function testRunWithUsers() { - $fakeUser = $this->createMock(IUser::class); - $this->config - ->expects($this->at(0)) - ->method('getSystemValueBool') - ->with('files_no_background_scan', false) - ->willReturn(false); - $this->config - ->expects($this->at(1)) - ->method('getAppValue') - ->with('files', 'cronjob_scan_files', 0) - ->willReturn(50); - $this->userManager - ->expects($this->at(0)) - ->method('search') - ->with('', 500, 50) - ->willReturn([ - $fakeUser - ]); - $this->config - ->expects($this->at(2)) - ->method('setAppValue') - ->with('files', 'cronjob_scan_files', 550); - $this->scanFiles - ->expects($this->once()) - ->method('runScanner') - ->with($fakeUser); - - $this->invokePrivate($this->scanFiles, 'run', [[]]); + private function getUser(string $userId): IUser { + $user = $this->createMock(IUser::class); + $user->method('getUID') + ->willReturn($userId); + return $user; } - public function testRunWithUsersAndOffsetAtEndOfUserList() { - $this->config - ->expects($this->at(0)) - ->method('getSystemValueBool') - ->with('files_no_background_scan', false) - ->willReturn(false); - $this->config - ->expects($this->at(1)) - ->method('getAppValue') - ->with('files', 'cronjob_scan_files', 0) - ->willReturn(50); - $this->userManager - ->expects($this->at(0)) - ->method('search') - ->with('', 500, 50) - ->willReturn([]); - $this->userManager - ->expects($this->at(1)) - ->method('search') - ->with('', 500) - ->willReturn([]); - $this->config - ->expects($this->at(2)) - ->method('setAppValue') - ->with('files', 'cronjob_scan_files', 500); - $this->scanFiles - ->expects($this->never()) - ->method('runScanner'); + private function setupStorage(string $user, string $mountPoint) { + $storage = new Temporary([]); + $storage->mkdir('foo'); + $storage->getScanner()->scan(''); - $this->invokePrivate($this->scanFiles, 'run', [[]]); + $this->createUser($user, ''); + $this->mountCache->registerMounts($this->getUser($user), [ + new MountPoint($storage, $mountPoint) + ]); + + return $storage; + } + + public function testAllScanned() { + $this->setupStorage('foouser', '/foousers/files/foo'); + + $this->scanFiles->expects($this->never()) + ->method('runScanner'); + $this->runJob(); + } + + public function testUnscanned() { + $storage = $this->setupStorage('foouser', '/foousers/files/foo'); + $storage->getCache()->put('foo', ['size' => -1]); + + $this->scanFiles->expects($this->once()) + ->method('runScanner') + ->with('foouser'); + $this->runJob(); } }