From a97a290fd5ed06dfc33d2adeed5d3b7531674827 Mon Sep 17 00:00:00 2001 From: Morris Jobke Date: Thu, 23 Nov 2017 15:04:51 +0100 Subject: Cache fetched apps in update check The code tried to find the apps with updates and thus was called for every available app. This caused to get the full appstore content as often as apps are available. The appstore request itself was cached nevertheless in an appdata dir, but with an object storage this is still a lot of round trips to read this cached result. Thus the instantiated list is now cached in a static variable (because it's a static method call). Signed-off-by: Morris Jobke --- lib/private/Installer.php | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) (limited to 'lib/private/Installer.php') diff --git a/lib/private/Installer.php b/lib/private/Installer.php index e754f28455b..cc6725e9175 100644 --- a/lib/private/Installer.php +++ b/lib/private/Installer.php @@ -395,7 +395,11 @@ class Installer { return false; } - $apps = $appFetcher->get(); + static $apps = null; + if ($apps === null) { + $apps = $appFetcher->get(); + } + foreach($apps as $app) { if($app['id'] === $appId) { $currentVersion = OC_App::getAppVersion($appId); -- cgit v1.2.3 From df61d43529418aace241b99be106ff9a35188dac Mon Sep 17 00:00:00 2001 From: Morris Jobke Date: Fri, 24 Nov 2017 10:41:51 +0100 Subject: Make isUpdateAvailable non-static Signed-off-by: Morris Jobke --- .../lib/Notification/BackgroundJob.php | 9 ++++++-- .../tests/Notification/BackgroundJobTest.php | 8 ++++++- lib/private/Installer.php | 25 +++++++++++----------- lib/private/Updater.php | 2 +- settings/Controller/AppSettingsController.php | 19 ++++++++++------ .../Controller/AppSettingsControllerTest.php | 10 ++++++++- tests/lib/InstallerTest.php | 3 ++- 7 files changed, 50 insertions(+), 26 deletions(-) (limited to 'lib/private/Installer.php') diff --git a/apps/updatenotification/lib/Notification/BackgroundJob.php b/apps/updatenotification/lib/Notification/BackgroundJob.php index 08baa35664f..3c0cac60cde 100644 --- a/apps/updatenotification/lib/Notification/BackgroundJob.php +++ b/apps/updatenotification/lib/Notification/BackgroundJob.php @@ -53,6 +53,9 @@ class BackgroundJob extends TimedJob { /** @var IClientService */ protected $client; + /** @var Installer */ + protected $installer; + /** @var string[] */ protected $users; @@ -64,8 +67,9 @@ class BackgroundJob extends TimedJob { * @param IGroupManager $groupManager * @param IAppManager $appManager * @param IClientService $client + * @param Installer $installer */ - public function __construct(IConfig $config, IManager $notificationManager, IGroupManager $groupManager, IAppManager $appManager, IClientService $client) { + public function __construct(IConfig $config, IManager $notificationManager, IGroupManager $groupManager, IAppManager $appManager, IClientService $client, Installer $installer) { // Run once a day $this->setInterval(60 * 60 * 24); @@ -74,6 +78,7 @@ class BackgroundJob extends TimedJob { $this->groupManager = $groupManager; $this->appManager = $appManager; $this->client = $client; + $this->installer = $installer; } protected function run($argument) { @@ -257,6 +262,6 @@ class BackgroundJob extends TimedJob { * @return string|false */ protected function isUpdateAvailable($app) { - return Installer::isUpdateAvailable($app, \OC::$server->getAppFetcher()); + return $this->installer->isUpdateAvailable($app); } } diff --git a/apps/updatenotification/tests/Notification/BackgroundJobTest.php b/apps/updatenotification/tests/Notification/BackgroundJobTest.php index 92a8a687f5a..0355b10a09c 100644 --- a/apps/updatenotification/tests/Notification/BackgroundJobTest.php +++ b/apps/updatenotification/tests/Notification/BackgroundJobTest.php @@ -23,6 +23,7 @@ namespace OCA\UpdateNotification\Tests\Notification; +use OC\Installer; use OCA\UpdateNotification\Notification\BackgroundJob; use OCP\App\IAppManager; use OCP\Http\Client\IClientService; @@ -47,6 +48,8 @@ class BackgroundJobTest extends TestCase { protected $appManager; /** @var IClientService|\PHPUnit_Framework_MockObject_MockObject */ protected $client; + /** @var Installer|\PHPUnit_Framework_MockObject_MockObject */ + protected $installer; public function setUp() { parent::setUp(); @@ -56,6 +59,7 @@ class BackgroundJobTest extends TestCase { $this->groupManager = $this->createMock(IGroupManager::class); $this->appManager = $this->createMock(IAppManager::class); $this->client = $this->createMock(IClientService::class); + $this->installer = $this->createMock(Installer::class); } /** @@ -69,7 +73,8 @@ class BackgroundJobTest extends TestCase { $this->notificationManager, $this->groupManager, $this->appManager, - $this->client + $this->client, + $this->installer ); } { return $this->getMockBuilder(BackgroundJob::class) @@ -79,6 +84,7 @@ class BackgroundJobTest extends TestCase { $this->groupManager, $this->appManager, $this->client, + $this->installer, ]) ->setMethods($methods) ->getMock(); diff --git a/lib/private/Installer.php b/lib/private/Installer.php index cc6725e9175..70d6c10b335 100644 --- a/lib/private/Installer.php +++ b/lib/private/Installer.php @@ -67,6 +67,10 @@ class Installer { private $logger; /** @var IConfig */ private $config; + /** @var array - for caching the result of app fetcher */ + private $apps = null; + /** @var bool|null - for caching the result of the ready status */ + private $isInstanceReadyForUpdates = null; /** * @param AppFetcher $appFetcher @@ -187,7 +191,7 @@ class Installer { * @return bool */ public function updateAppstoreApp($appId) { - if(self::isUpdateAvailable($appId, $this->appFetcher)) { + if($this->isUpdateAvailable($appId)) { try { $this->downloadApp($appId); } catch (\Exception $e) { @@ -375,29 +379,24 @@ class Installer { * Check if an update for the app is available * * @param string $appId - * @param AppFetcher $appFetcher * @return string|false false or the version number of the update */ - public static function isUpdateAvailable($appId, - AppFetcher $appFetcher) { - static $isInstanceReadyForUpdates = null; - - if ($isInstanceReadyForUpdates === null) { + public function isUpdateAvailable($appId) { + if ($this->isInstanceReadyForUpdates === null) { $installPath = OC_App::getInstallPath(); if ($installPath === false || $installPath === null) { - $isInstanceReadyForUpdates = false; + $this->isInstanceReadyForUpdates = false; } else { - $isInstanceReadyForUpdates = true; + $this->isInstanceReadyForUpdates = true; } } - if ($isInstanceReadyForUpdates === false) { + if ($this->isInstanceReadyForUpdates === false) { return false; } - static $apps = null; - if ($apps === null) { - $apps = $appFetcher->get(); + if ($this->apps === null) { + $apps = $this->appFetcher->get(); } foreach($apps as $app) { diff --git a/lib/private/Updater.php b/lib/private/Updater.php index 5c8d5a2cd4e..996163daacc 100644 --- a/lib/private/Updater.php +++ b/lib/private/Updater.php @@ -468,7 +468,7 @@ class Updater extends BasicEmitter { foreach($disabledApps as $app) { try { $this->emit('\OC\Updater', 'checkAppStoreAppBefore', [$app]); - if (Installer::isUpdateAvailable($app, \OC::$server->getAppFetcher())) { + if ($this->installer->isUpdateAvailable($app)) { $this->emit('\OC\Updater', 'upgradeAppStoreApp', [$app]); $this->installer->updateAppstoreApp($app); } diff --git a/settings/Controller/AppSettingsController.php b/settings/Controller/AppSettingsController.php index 26858eabcf3..f2a92b52f6d 100644 --- a/settings/Controller/AppSettingsController.php +++ b/settings/Controller/AppSettingsController.php @@ -37,6 +37,7 @@ use OC\App\AppStore\Fetcher\CategoryFetcher; use OC\App\AppStore\Version\VersionParser; use OC\App\DependencyAnalyzer; use OC\App\Platform; +use OC\Installer; use OCP\App\IAppManager; use \OCP\AppFramework\Controller; use OCP\AppFramework\Http\ContentSecurityPolicy; @@ -74,6 +75,8 @@ class AppSettingsController extends Controller { private $l10nFactory; /** @var BundleFetcher */ private $bundleFetcher; + /** @var Installer */ + private $installer; /** * @param string $appName @@ -86,6 +89,7 @@ class AppSettingsController extends Controller { * @param AppFetcher $appFetcher * @param IFactory $l10nFactory * @param BundleFetcher $bundleFetcher + * @param Installer $installer */ public function __construct($appName, IRequest $request, @@ -96,7 +100,8 @@ class AppSettingsController extends Controller { CategoryFetcher $categoryFetcher, AppFetcher $appFetcher, IFactory $l10nFactory, - BundleFetcher $bundleFetcher) { + BundleFetcher $bundleFetcher, + Installer $installer) { parent::__construct($appName, $request); $this->l10n = $l10n; $this->config = $config; @@ -106,6 +111,7 @@ class AppSettingsController extends Controller { $this->appFetcher = $appFetcher; $this->l10nFactory = $l10nFactory; $this->bundleFetcher = $bundleFetcher; + $this->installer = $installer; } /** @@ -270,8 +276,7 @@ class AppSettingsController extends Controller { ]; - $appFetcher = \OC::$server->getAppFetcher(); - $newVersion = \OC\Installer::isUpdateAvailable($app['id'], $appFetcher); + $newVersion = $this->installer->isUpdateAvailable($app['id']); if($newVersion && $this->appManager->isInstalled($app['id'])) { $formattedApps[count($formattedApps)-1]['update'] = $newVersion; } @@ -284,7 +289,7 @@ class AppSettingsController extends Controller { $appClass = new \OC_App(); $apps = $appClass->listAllApps(); foreach($apps as $key => $app) { - $newVersion = \OC\Installer::isUpdateAvailable($app['id'], $this->appFetcher); + $newVersion = $this->installer->isUpdateAvailable($app['id']); if($newVersion !== false) { $apps[$key]['update'] = $newVersion; } else { @@ -317,7 +322,7 @@ class AppSettingsController extends Controller { $apps = $appClass->listAllApps(); foreach($apps as $key => $app) { - $newVersion = \OC\Installer::isUpdateAvailable($app['id'], $this->appFetcher); + $newVersion = $this->installer->isUpdateAvailable($app['id']); $apps[$key]['update'] = $newVersion; } @@ -342,7 +347,7 @@ class AppSettingsController extends Controller { }); foreach($apps as $key => $app) { - $newVersion = \OC\Installer::isUpdateAvailable($app['id'], $this->appFetcher); + $newVersion = $this->installer->isUpdateAvailable($app['id']); $apps[$key]['update'] = $newVersion; } @@ -363,7 +368,7 @@ class AppSettingsController extends Controller { }); $apps = array_map(function ($app) { - $newVersion = \OC\Installer::isUpdateAvailable($app['id'], $this->appFetcher); + $newVersion = $this->installer->isUpdateAvailable($app['id']); if ($newVersion !== false) { $app['update'] = $newVersion; } diff --git a/tests/Settings/Controller/AppSettingsControllerTest.php b/tests/Settings/Controller/AppSettingsControllerTest.php index e264d0dfbfe..6631873d8ad 100644 --- a/tests/Settings/Controller/AppSettingsControllerTest.php +++ b/tests/Settings/Controller/AppSettingsControllerTest.php @@ -25,6 +25,7 @@ namespace Tests\Settings\Controller; use OC\App\AppStore\Bundles\BundleFetcher; use OC\App\AppStore\Fetcher\AppFetcher; use OC\App\AppStore\Fetcher\CategoryFetcher; +use OC\Installer; use OC\Settings\Controller\AppSettingsController; use OCP\AppFramework\Http\ContentSecurityPolicy; use OCP\AppFramework\Http\JSONResponse; @@ -63,6 +64,8 @@ class AppSettingsControllerTest extends TestCase { private $l10nFactory; /** @var BundleFetcher|\PHPUnit_Framework_MockObject_MockObject */ private $bundleFetcher; + /** @var Installer|\PHPUnit_Framework_MockObject_MockObject */ + private $installer; public function setUp() { parent::setUp(); @@ -79,6 +82,7 @@ class AppSettingsControllerTest extends TestCase { $this->appFetcher = $this->createMock(AppFetcher::class); $this->l10nFactory = $this->createMock(IFactory::class); $this->bundleFetcher = $this->createMock(BundleFetcher::class); + $this->installer = $this->createMock(Installer::class); $this->appSettingsController = new AppSettingsController( 'settings', @@ -90,11 +94,15 @@ class AppSettingsControllerTest extends TestCase { $this->categoryFetcher, $this->appFetcher, $this->l10nFactory, - $this->bundleFetcher + $this->bundleFetcher, + $this->installer ); } public function testListCategories() { + $this->installer->expects($this->any()) + ->method('isUpdateAvailable') + ->willReturn(false); $expected = new JSONResponse([ [ 'id' => 2, diff --git a/tests/lib/InstallerTest.php b/tests/lib/InstallerTest.php index 68f3676faa8..897bc472103 100644 --- a/tests/lib/InstallerTest.php +++ b/tests/lib/InstallerTest.php @@ -154,7 +154,8 @@ class InstallerTest extends TestCase { ->method('get') ->willReturn($appArray); - $this->assertSame($updateAvailable, Installer::isUpdateAvailable('files', $this->appFetcher)); + $installer = $this->getInstaller(); + $this->assertSame($updateAvailable, $installer->isUpdateAvailable('files')); } /** -- cgit v1.2.3