Merge pull request #213 from SirTediousOfFoo/patch-1

Optimize slow DB query

2 files changed, 115 insertions, 71 deletions

diff --git a/lib/BackgroundJob/BackgroundScanner.php b/lib/BackgroundJob/BackgroundScanner.php
index 5c686b3..1e41e50 100644
--- a/lib/BackgroundJob/BackgroundScanner.php
+++ b/lib/BackgroundJob/BackgroundScanner.php
@@ -30,7 +30,7 @@ class BackgroundScanner extends TimedJob {
 	/** @var ScannerFactory */
 	private $scannerFactory;
 
-	/** @var  AppConfig  */
+	/** @var  AppConfig */
 	private $appConfig;
 
 	/** @var ILogger */
@@ -50,15 +50,16 @@ class BackgroundScanner extends TimedJob {
 	/** @var bool */
 	private $isCLI;
 
-	public function __construct(ScannerFactory $scannerFactory,
-								AppConfig $appConfig,
-								IRootFolder $rootFolder,
-								ILogger $logger,
-								IUserManager $userManager,
-								IDBConnection $db,
-								IMimeTypeLoader $mimeTypeLoader,
-								ItemFactory $itemFactory,
-								bool $isCLI
+	public function __construct(
+		ScannerFactory $scannerFactory,
+		AppConfig $appConfig,
+		IRootFolder $rootFolder,
+		ILogger $logger,
+		IUserManager $userManager,
+		IDBConnection $db,
+		IMimeTypeLoader $mimeTypeLoader,
+		ItemFactory $itemFactory,
+		bool $isCLI
 	) {
 		$this->rootFolder = $rootFolder;
 		$this->scannerFactory = $scannerFactory;
@@ -270,23 +271,20 @@ class BackgroundScanner extends TimedJob {
 		return $data;
 	}
 
-	protected function getUnscannedFiles() {
+	public function getUnscannedFiles() {
 		$dirMimeTypeId = $this->mimeTypeLoader->getId('httpd/unix-directory');
 
-		$qb1 = $this->db->getQueryBuilder();
-		$qb1->select('fileid')
-			->from('files_antivirus');
-
-		$qb2 = $this->db->getQueryBuilder();
-		$qb2->select('fileid', 'storage')
+		$query = $this->db->getQueryBuilder();
+		$query->select('fc.fileid', 'storage')
 			->from('filecache', 'fc')
-			->where($qb2->expr()->notIn('fileid', $qb2->createFunction($qb1->getSQL())))
-			->andWhere($qb2->expr()->neq('mimetype', $qb2->expr()->literal($dirMimeTypeId)))
-			->andWhere($qb2->expr()->like('path', $qb2->expr()->literal('files/%')))
-			->andWhere($this->getSizeLimitExpression($qb2))
+			->leftJoin('fc', 'files_antivirus', 'fa', $query->expr()->eq('fc.fileid', 'fa.fileid'))
+			->where($query->expr()->isNull('fa.fileid'))
+			->andWhere($query->expr()->neq('mimetype', $query->expr()->literal($dirMimeTypeId)))
+			->andWhere($query->expr()->like('path', $query->expr()->literal('files/%')))
+			->andWhere($this->getSizeLimitExpression($query))
 			->setMaxResults($this->getBatchSize() * 10);
 
-		return $qb2->execute();
+		return $query->execute();
 	}
 
 	protected function getToRescanFiles() {
@@ -301,31 +299,23 @@ class BackgroundScanner extends TimedJob {
 		return $qb->execute();
 	}
 
-	protected function getOutdatedFiles() {
+	public function getOutdatedFiles() {
 		$dirMimeTypeId = $this->mimeTypeLoader->getId('httpd/unix-directory');
 
 		// We do not want to keep scanning the same files. So only scan them once per 28 days at most.
 		$yesterday = time() - (28 * 24 * 60 * 60);
 
-		$qb1 = $this->db->getQueryBuilder();
-		$qb2 = $this->db->getQueryBuilder();
-
-		$qb1->select('fileid')
-			->from('files_antivirus')
-			->andWhere($qb2->expr()->lt('check_time', $qb2->createNamedParameter($yesterday)))
-			->orderBy('check_time', 'ASC');
-
-		$qb2->select('fileid', 'storage')
+		$query = $this->db->getQueryBuilder();
+		$query->select('fc.fileid', 'fc.storage')
 			->from('filecache', 'fc')
-			->where($qb2->expr()->in('fileid', $qb2->createFunction($qb1->getSQL())))
-			->andWhere($qb2->expr()->neq('mimetype', $qb2->expr()->literal($dirMimeTypeId)))
-			->andWhere($qb2->expr()->like('path', $qb2->expr()->literal('files/%')))
-			->andWhere($this->getSizeLimitExpression($qb2))
+			->innerJoin('fc', 'files_antivirus', 'fa', $query->expr()->eq('fc.fileid', 'fa.fileid'))
+			->andWhere($query->expr()->neq('mimetype', $query->createNamedParameter($dirMimeTypeId)))
+			->andWhere($query->expr()->like('path', $query->expr()->literal('files/%')))
+			->andWhere($query->expr()->lt('check_time', $query->createNamedParameter($yesterday)))
+			->andWhere($this->getSizeLimitExpression($query))
 			->setMaxResults($this->getBatchSize() * 10);
 
-		$x = $qb2->getSQL();
-
-		return $qb2->execute();
+		return $query->execute();
 	}
 
 	protected function scanOneFile(File $file): void {
diff --git a/tests/BackgroundScannerTest.php b/tests/BackgroundScannerTest.php
index b399ccc..fad6498 100644
--- a/tests/BackgroundScannerTest.php
+++ b/tests/BackgroundScannerTest.php
@@ -10,43 +10,97 @@
 
 namespace OCA\Files_Antivirus\Tests;
 
+use OC\Files\Storage\Temporary;
 use OCA\Files_Antivirus\BackgroundJob\BackgroundScanner;
-use Doctrine\DBAL\Driver\PDOStatement;
+use OCP\Files\Folder;
+use OCP\Files\IRootFolder;
+use OCP\IDBConnection;
+use Test\Traits\MountProviderTrait;
+use Test\Traits\UserTrait;
 
 /**
- * Class BackgroundScannerTest
- *
- * @package OCA\Files_Antivirus\Tests
  * @group DB
  */
 class BackgroundScannerTest extends TestBase {
-	public function testGetFilesForScan() {
-		$this->assertTrue(true);
-		return;
-
-		$scannerFactory = new Mock\ScannerFactory(
-			new Mock\Config($this->container->query('CoreConfig')),
-			$this->container->query('Logger')
-		);
-
-		$scannerMock = $this->getMockBuilder(BackgroundScanner::class)
-			->setConstructorArgs([
-				$scannerFactory,
-				$this->l10n,
-				$this->config,
-				\OC::$server->getRootFolder(),
-				\OC::$server->getUserSession(),
-				\OC::$server->getLogger(),
-				\OC::$server->getUserManager(),
-				\OC::$server->getDatabaseConnection(),
-				\OC::$server->getMimeTypeLoader()
-			])
-			->getMock();
-
-		$class = new \ReflectionClass($scannerMock);
-		$method = $class->getMethod('getFilesForScan');
-		$method->setAccessible(true);
-		$result = $method->invokeArgs($scannerMock, []);
-		$this->assertEquals(PDOStatement::class, get_class($result));
+	use UserTrait;
+	use MountProviderTrait;
+
+	/** @var Folder */
+	private $homeDirectory;
+
+	protected function setUp(): void {
+		parent::setUp();
+
+		$this->createUser("av", "av");
+		$storage = new Temporary();
+		$storage->mkdir('files');
+		$storage->getScanner()->scan('');
+		$this->registerMount("av", $storage, "av");
+
+		$this->loginAsUser("av");
+		/** @var IRootFolder $root */
+		$root = \OC::$server->get(IRootFolder::class);
+		$this->homeDirectory = $root->getUserFolder("av");
+	}
+
+	private function markAllScanned() {
+		$now = time();
+		/** @var IDBConnection $db */
+		$db = \OC::$server->get(IDBConnection::class);
+
+		$db->getQueryBuilder()->delete('files_antivirus')->execute();
+
+		$query = $db->getQueryBuilder();
+		$query->select('fileid')
+			->from('filecache');
+		$fileIds = $query->execute()->fetchAll(\PDO::FETCH_COLUMN);
+
+		$query = $db->getQueryBuilder();
+		$query->insert('files_antivirus')
+			->values([
+				'fileid' => $query->createParameter('fileid'),
+				'check_time' => $now,
+			]);
+		foreach ($fileIds as $fileId) {
+			$query->setParameter('fileid', $fileId);
+			$query->execute();
+		}
+	}
+
+	private function updateScannedTime(int $fileId, int $time) {
+		/** @var IDBConnection $db */
+		$db = \OC::$server->get(IDBConnection::class);
+
+		$query = $db->getQueryBuilder();
+		$query->update('files_antivirus')
+			->set('check_time', $query->createNamedParameter($time))
+			->where($query->expr()->eq('fileid', $query->createNamedParameter($fileId)));
+		$query->execute();
+	}
+
+	public function testGetUnscannedFiles() {
+		$this->markAllScanned();
+
+		/** @var BackgroundScanner $scanner */
+		$scanner = \OC::$server->get(BackgroundScanner::class);
+		$newFileId = $this->homeDirectory->newFile("foo", "bar")->getId();
+
+		$outdated = $scanner->getUnscannedFiles()->fetchAll(\PDO::FETCH_COLUMN);
+		$this->assertEquals([$newFileId], $outdated);
+	}
+
+	public function testGetOutdatedFiles() {
+		$newFileId = $this->homeDirectory->newFile("foo", "bar")->getId();
+		$this->markAllScanned();
+
+		/** @var BackgroundScanner $scanner */
+		$scanner = \OC::$server->get(BackgroundScanner::class);
+
+		$outdated = $scanner->getOutdatedFiles()->fetchAll(\PDO::FETCH_COLUMN);
+		$this->assertEquals([], $outdated);
+
+		$this->updateScannedTime($newFileId, time() - (30 * 24 * 60 * 60));
+		$outdated = $scanner->getOutdatedFiles()->fetchAll(\PDO::FETCH_COLUMN);
+		$this->assertEquals([$newFileId], $outdated);
 	}
 }