From 0a79884abfb37983fe2d9874d46b6a6aab6f3a16 Mon Sep 17 00:00:00 2001 From: Maxence Lange Date: Wed, 31 Jan 2024 10:06:52 -0100 Subject: [PATCH 1/2] feat(appconfig): switching integrity check to lazy config values Signed-off-by: Maxence Lange --- lib/private/IntegrityCheck/Checker.php | 68 ++++++------------------ lib/private/Server.php | 5 +- tests/lib/IntegrityCheck/CheckerTest.php | 10 +++- 3 files changed, 27 insertions(+), 56 deletions(-) diff --git a/lib/private/IntegrityCheck/Checker.php b/lib/private/IntegrityCheck/Checker.php index a5dec637bdba8..e8fd087ebc2f9 100644 --- a/lib/private/IntegrityCheck/Checker.php +++ b/lib/private/IntegrityCheck/Checker.php @@ -40,6 +40,7 @@ use OC\IntegrityCheck\Iterator\ExcludeFoldersByPathFilterIterator; use OCP\App\IAppManager; use OCP\Files\IMimeTypeDetector; +use OCP\IAppConfig; use OCP\ICache; use OCP\ICacheFactory; use OCP\IConfig; @@ -58,44 +59,20 @@ */ class Checker { public const CACHE_KEY = 'oc.integritycheck.checker'; - /** @var EnvironmentHelper */ - private $environmentHelper; - /** @var AppLocator */ - private $appLocator; - /** @var FileAccessHelper */ - private $fileAccessHelper; - /** @var IConfig|null */ - private $config; - /** @var ICache */ - private $cache; - /** @var IAppManager|null */ - private $appManager; - /** @var IMimeTypeDetector */ - private $mimeTypeDetector; - /** - * @param EnvironmentHelper $environmentHelper - * @param FileAccessHelper $fileAccessHelper - * @param AppLocator $appLocator - * @param IConfig|null $config - * @param ICacheFactory $cacheFactory - * @param IAppManager|null $appManager - * @param IMimeTypeDetector $mimeTypeDetector - */ - public function __construct(EnvironmentHelper $environmentHelper, - FileAccessHelper $fileAccessHelper, - AppLocator $appLocator, - ?IConfig $config, + private ICache $cache; + + public function __construct( + private EnvironmentHelper $environmentHelper, + private FileAccessHelper $fileAccessHelper, + private AppLocator $appLocator, + private ?IConfig $config, + private ?IAppConfig $appConfig, ICacheFactory $cacheFactory, - ?IAppManager $appManager, - IMimeTypeDetector $mimeTypeDetector) { - $this->environmentHelper = $environmentHelper; - $this->fileAccessHelper = $fileAccessHelper; - $this->appLocator = $appLocator; - $this->config = $config; + private ?IAppManager $appManager, + private IMimeTypeDetector $mimeTypeDetector, + ) { $this->cache = $cacheFactory->createDistributed(self::CACHE_KEY); - $this->appManager = $appManager; - $this->mimeTypeDetector = $mimeTypeDetector; } /** @@ -114,15 +91,7 @@ public function isCodeCheckEnforced(): bool { * applicable for very specific scenarios and we should not advertise it * too prominent. So please do not add it to config.sample.php. */ - $isIntegrityCheckDisabled = false; - if ($this->config !== null) { - $isIntegrityCheckDisabled = $this->config->getSystemValueBool('integrity.check.disabled', false); - } - if ($isIntegrityCheckDisabled) { - return false; - } - - return true; + return !($this->config?->getSystemValueBool('integrity.check.disabled', false) ?? false); } /** @@ -443,10 +412,7 @@ public function getResults(): array { return json_decode($cachedResults, true); } - if ($this->config !== null) { - return json_decode($this->config->getAppValue('core', self::CACHE_KEY, '{}'), true); - } - return []; + return $this->appConfig?->getValueArray('core', self::CACHE_KEY, lazy: true) ?? []; } /** @@ -461,9 +427,7 @@ private function storeResults(string $scope, array $result) { if (!empty($result)) { $resultArray[$scope] = $result; } - if ($this->config !== null) { - $this->config->setAppValue('core', self::CACHE_KEY, json_encode($resultArray)); - } + $this->appConfig?->setValueArray('core', self::CACHE_KEY, $resultArray, lazy: true); $this->cache->set(self::CACHE_KEY, json_encode($resultArray)); } @@ -472,7 +436,7 @@ private function storeResults(string $scope, array $result) { * Clean previous results for a proper rescanning. Otherwise */ private function cleanResults() { - $this->config->deleteAppValue('core', self::CACHE_KEY); + $this->appConfig->deleteKey('core', self::CACHE_KEY); $this->cache->remove(self::CACHE_KEY); } diff --git a/lib/private/Server.php b/lib/private/Server.php index a29280a09eab1..cb5086acfc643 100644 --- a/lib/private/Server.php +++ b/lib/private/Server.php @@ -989,10 +989,10 @@ public function __construct($webRoot, \OC\Config $config) { // might however be called when ownCloud is not yet setup. if (\OC::$server->get(SystemConfig::class)->getValue('installed', false)) { $config = $c->get(\OCP\IConfig::class); + $appConfig = $c->get(\OCP\IAppConfig::class); $appManager = $c->get(IAppManager::class); } else { - $config = null; - $appManager = null; + $config = $appConfig = $appManager = null; } return new Checker( @@ -1000,6 +1000,7 @@ public function __construct($webRoot, \OC\Config $config) { new FileAccessHelper(), new AppLocator(), $config, + $appConfig, $c->get(ICacheFactory::class), $appManager, $c->get(IMimeTypeDetector::class) diff --git a/tests/lib/IntegrityCheck/CheckerTest.php b/tests/lib/IntegrityCheck/CheckerTest.php index 203e7e9722715..1cfbe9907a980 100644 --- a/tests/lib/IntegrityCheck/CheckerTest.php +++ b/tests/lib/IntegrityCheck/CheckerTest.php @@ -28,6 +28,7 @@ use OC\IntegrityCheck\Helpers\FileAccessHelper; use OC\Memcache\NullCache; use OCP\App\IAppManager; +use OCP\IAppConfig; use OCP\ICacheFactory; use OCP\IConfig; use phpseclib\Crypt\RSA; @@ -45,6 +46,8 @@ class CheckerTest extends TestCase { private $fileAccessHelper; /** @var IConfig|\PHPUnit\Framework\MockObject\MockObject */ private $config; + /** @var IAppConfig|\PHPUnit\Framework\MockObject\MockObject */ + private $appConfig; /** @var ICacheFactory|\PHPUnit\Framework\MockObject\MockObject */ private $cacheFactory; /** @var IAppManager|\PHPUnit\Framework\MockObject\MockObject */ @@ -58,6 +61,7 @@ protected function setUp(): void { $this->fileAccessHelper = $this->createMock(FileAccessHelper::class); $this->appLocator = $this->createMock(AppLocator::class); $this->config = $this->createMock(IConfig::class); + $this->appConfig = $this->createMock(IAppConfig::class); $this->cacheFactory = $this->createMock(ICacheFactory::class); $this->appManager = $this->createMock(IAppManager::class); $this->mimeTypeDetector = $this->createMock(\OC\Files\Type\Detection::class); @@ -76,6 +80,7 @@ protected function setUp(): void { $this->fileAccessHelper, $this->appLocator, $this->config, + $this->appConfig, $this->cacheFactory, $this->appManager, $this->mimeTypeDetector @@ -1025,6 +1030,7 @@ public function testRunInstanceVerification() { $this->fileAccessHelper, $this->appLocator, $this->config, + $this->appConfig, $this->cacheFactory, $this->appManager, $this->mimeTypeDetector, @@ -1089,9 +1095,9 @@ public function testRunInstanceVerification() { true, false, ); - $this->config + $this->appConfig ->expects($this->once()) - ->method('deleteAppValue') + ->method('deleteKey') ->with('core', 'oc.integritycheck.checker'); $this->checker->runInstanceVerification(); From b0bfe3e2cd7b6ef393e74000ba083cd57145df2c Mon Sep 17 00:00:00 2001 From: Maxence Lange Date: Thu, 29 Feb 2024 12:43:08 -0100 Subject: [PATCH 2/2] feat(appconfig): repair step to migrate lazy config values Signed-off-by: Maxence Lange f Signed-off-by: Maxence Lange --- lib/composer/composer/autoload_classmap.php | 1 + lib/composer/composer/autoload_static.php | 1 + lib/private/Repair.php | 2 + .../Repair/AddAppConfigLazyMigration.php | 62 +++++++++++++++++++ 4 files changed, 66 insertions(+) create mode 100644 lib/private/Repair/AddAppConfigLazyMigration.php diff --git a/lib/composer/composer/autoload_classmap.php b/lib/composer/composer/autoload_classmap.php index 936d8c7cce2af..8cedc1a5c0b82 100644 --- a/lib/composer/composer/autoload_classmap.php +++ b/lib/composer/composer/autoload_classmap.php @@ -1640,6 +1640,7 @@ 'OC\\Remote\\User' => $baseDir . '/lib/private/Remote/User.php', 'OC\\Repair' => $baseDir . '/lib/private/Repair.php', 'OC\\RepairException' => $baseDir . '/lib/private/RepairException.php', + 'OC\\Repair\\AddAppConfigLazyMigration' => $baseDir . '/lib/private/Repair/AddAppConfigLazyMigration.php', 'OC\\Repair\\AddBruteForceCleanupJob' => $baseDir . '/lib/private/Repair/AddBruteForceCleanupJob.php', 'OC\\Repair\\AddCleanupUpdaterBackupsJob' => $baseDir . '/lib/private/Repair/AddCleanupUpdaterBackupsJob.php', 'OC\\Repair\\AddMetadataGenerationJob' => $baseDir . '/lib/private/Repair/AddMetadataGenerationJob.php', diff --git a/lib/composer/composer/autoload_static.php b/lib/composer/composer/autoload_static.php index 9294cce9a0d6f..4c3394b23d271 100644 --- a/lib/composer/composer/autoload_static.php +++ b/lib/composer/composer/autoload_static.php @@ -1673,6 +1673,7 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2 'OC\\Remote\\User' => __DIR__ . '/../../..' . '/lib/private/Remote/User.php', 'OC\\Repair' => __DIR__ . '/../../..' . '/lib/private/Repair.php', 'OC\\RepairException' => __DIR__ . '/../../..' . '/lib/private/RepairException.php', + 'OC\\Repair\\AddAppConfigLazyMigration' => __DIR__ . '/../../..' . '/lib/private/Repair/AddAppConfigLazyMigration.php', 'OC\\Repair\\AddBruteForceCleanupJob' => __DIR__ . '/../../..' . '/lib/private/Repair/AddBruteForceCleanupJob.php', 'OC\\Repair\\AddCleanupUpdaterBackupsJob' => __DIR__ . '/../../..' . '/lib/private/Repair/AddCleanupUpdaterBackupsJob.php', 'OC\\Repair\\AddMetadataGenerationJob' => __DIR__ . '/../../..' . '/lib/private/Repair/AddMetadataGenerationJob.php', diff --git a/lib/private/Repair.php b/lib/private/Repair.php index 1af9b0d1b22c7..b1800d6087d39 100644 --- a/lib/private/Repair.php +++ b/lib/private/Repair.php @@ -36,6 +36,7 @@ use OC\DB\Connection; use OC\DB\ConnectionAdapter; +use OC\Repair\AddAppConfigLazyMigration; use OC\Repair\AddBruteForceCleanupJob; use OC\Repair\AddCleanupUpdaterBackupsJob; use OC\Repair\AddMetadataGenerationJob; @@ -209,6 +210,7 @@ public static function getRepairSteps(): array { \OCP\Server::get(AddMissingSecretJob::class), \OCP\Server::get(AddRemoveOldTasksBackgroundJob::class), \OCP\Server::get(AddMetadataGenerationJob::class), + \OCP\Server::get(AddAppConfigLazyMigration::class), ]; } diff --git a/lib/private/Repair/AddAppConfigLazyMigration.php b/lib/private/Repair/AddAppConfigLazyMigration.php new file mode 100644 index 0000000000000..1f466b98b28eb --- /dev/null +++ b/lib/private/Repair/AddAppConfigLazyMigration.php @@ -0,0 +1,62 @@ + + * + * @author Maxence Lange + * + * @license GNU AGPL version 3 or any later version + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as + * published by the Free Software Foundation, either version 3 of the + * License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + * + */ +namespace OC\Repair; + +use OCP\IAppConfig; +use OCP\Migration\IOutput; +use OCP\Migration\IRepairStep; +use Psr\Log\LoggerInterface; + +class AddAppConfigLazyMigration implements IRepairStep { + /** + * Just add config values that needs to be migrated to lazy loading + */ + private static array $lazyAppConfig = [ + 'core' => [ + 'oc.integritycheck.checker', + ], + ]; + + public function __construct( + private IAppConfig $appConfig, + private LoggerInterface $logger, + ) { + } + + public function getName() { + return 'migrate lazy config values'; + } + + public function run(IOutput $output) { + $c = 0; + foreach (self::$lazyAppConfig as $appId => $configKeys) { + foreach ($configKeys as $configKey) { + $c += (int)$this->appConfig->updateLazy($appId, $configKey, true); + } + } + + $this->logger->notice('core/BackgroundJobs/AppConfigLazyMigration: ' . $c . ' config values updated'); + } +}