From 9731350acef75931ccbeafa054b40afe8189653f Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Thu, 24 Aug 2017 16:06:37 +0200 Subject: [PATCH 1/3] split async test bus for easier subclassing --- lib/private/Command/AsyncBus.php | 55 ++++------------------ lib/private/Command/CronBus.php | 75 ++++++++++++++++++++++++++++++ lib/private/Server.php | 4 +- tests/lib/Command/AsyncBusTest.php | 56 +++++++++++----------- tests/lib/Command/CronBusTest.php | 50 ++++++++++++++++++++ tests/lib/TestCase.php | 4 +- 6 files changed, 168 insertions(+), 76 deletions(-) create mode 100644 lib/private/Command/CronBus.php create mode 100644 tests/lib/Command/CronBusTest.php diff --git a/lib/private/Command/AsyncBus.php b/lib/private/Command/AsyncBus.php index fb3cbee724..2dffc9c784 100644 --- a/lib/private/Command/AsyncBus.php +++ b/lib/private/Command/AsyncBus.php @@ -24,17 +24,11 @@ namespace OC\Command; use OCP\Command\IBus; use OCP\Command\ICommand; -use SuperClosure\Serializer; /** * Asynchronous command bus that uses the background job system as backend */ -class AsyncBus implements IBus { - /** - * @var \OCP\BackgroundJob\IJobList - */ - private $jobList; - +abstract class AsyncBus implements IBus { /** * List of traits for command which require sync execution * @@ -42,13 +36,6 @@ class AsyncBus implements IBus { */ private $syncTraits = []; - /** - * @param \OCP\BackgroundJob\IJobList $jobList - */ - public function __construct($jobList) { - $this->jobList = $jobList; - } - /** * Schedule a command to be fired * @@ -56,12 +43,19 @@ class AsyncBus implements IBus { */ public function push($command) { if ($this->canRunAsync($command)) { - $this->jobList->add($this->getJobClass($command), $this->serializeCommand($command)); + $this->queueCommand($command); } else { $this->runCommand($command); } } + /** + * Queue a command in the bus + * + * @param \OCP\Command\ICommand | callable $command + */ + abstract protected function queueCommand($command); + /** * Require all commands using a trait to be run synchronous * @@ -82,37 +76,6 @@ class AsyncBus implements IBus { } } - /** - * @param \OCP\Command\ICommand | callable $command - * @return string - */ - private function getJobClass($command) { - if ($command instanceof \Closure) { - return 'OC\Command\ClosureJob'; - } else if (is_callable($command)) { - return 'OC\Command\CallableJob'; - } else if ($command instanceof ICommand) { - return 'OC\Command\CommandJob'; - } else { - throw new \InvalidArgumentException('Invalid command'); - } - } - - /** - * @param \OCP\Command\ICommand | callable $command - * @return string - */ - private function serializeCommand($command) { - if ($command instanceof \Closure) { - $serializer = new Serializer(); - return $serializer->serialize($command); - } else if (is_callable($command) or $command instanceof ICommand) { - return serialize($command); - } else { - throw new \InvalidArgumentException('Invalid command'); - } - } - /** * @param \OCP\Command\ICommand | callable $command * @return bool diff --git a/lib/private/Command/CronBus.php b/lib/private/Command/CronBus.php new file mode 100644 index 0000000000..a0ad9b3c86 --- /dev/null +++ b/lib/private/Command/CronBus.php @@ -0,0 +1,75 @@ + + * + * @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\Command; + +use OCP\Command\ICommand; +use SuperClosure\Serializer; + +class CronBus extends AsyncBus { + /** + * @var \OCP\BackgroundJob\IJobList + */ + private $jobList; + + + /** + * @param \OCP\BackgroundJob\IJobList $jobList + */ + public function __construct($jobList) { + $this->jobList = $jobList; + } + + protected function queueCommand($command) { + $this->jobList->add($this->getJobClass($command), $this->serializeCommand($command)); + } + + /** + * @param \OCP\Command\ICommand | callable $command + * @return string + */ + private function getJobClass($command) { + if ($command instanceof \Closure) { + return 'OC\Command\ClosureJob'; + } else if (is_callable($command)) { + return 'OC\Command\CallableJob'; + } else if ($command instanceof ICommand) { + return 'OC\Command\CommandJob'; + } else { + throw new \InvalidArgumentException('Invalid command'); + } + } + + /** + * @param \OCP\Command\ICommand | callable $command + * @return string + */ + private function serializeCommand($command) { + if ($command instanceof \Closure) { + $serializer = new Serializer(); + return $serializer->serialize($command); + } else if (is_callable($command) or $command instanceof ICommand) { + return serialize($command); + } else { + throw new \InvalidArgumentException('Invalid command'); + } + } +} \ No newline at end of file diff --git a/lib/private/Server.php b/lib/private/Server.php index 60a5de97bb..d49e782ea2 100644 --- a/lib/private/Server.php +++ b/lib/private/Server.php @@ -51,7 +51,7 @@ use OC\AppFramework\Http\Request; use OC\AppFramework\Utility\SimpleContainer; use OC\AppFramework\Utility\TimeFactory; use OC\Authentication\LoginCredentials\Store; -use OC\Command\AsyncBus; +use OC\Command\CronBus; use OC\Contacts\ContactsMenu\ActionFactory; use OC\Diagnostics\EventLogger; use OC\Diagnostics\NullEventLogger; @@ -695,7 +695,7 @@ class Server extends ServerContainer implements IServerContainer { }); $this->registerService('AsyncCommandBus', function (Server $c) { $jobList = $c->getJobList(); - return new AsyncBus($jobList); + return new CronBus($jobList); }); $this->registerService('TrustedDomainHelper', function ($c) { return new TrustedDomainHelper($this->getConfig()); diff --git a/tests/lib/Command/AsyncBusTest.php b/tests/lib/Command/AsyncBusTest.php index 8c1713f126..da168d66e6 100644 --- a/tests/lib/Command/AsyncBusTest.php +++ b/tests/lib/Command/AsyncBusTest.php @@ -12,7 +12,6 @@ namespace Test\Command; use OC\Command\FileAccess; use OCP\Command\IBus; use OCP\Command\ICommand; -use Test\BackgroundJob\DummyJobList; use Test\TestCase; class SimpleCommand implements ICommand { @@ -58,7 +57,7 @@ class ThisClosureTest { } } -class AsyncBusTest extends TestCase { +abstract class AsyncBusTest extends TestCase { /** * Basic way to check output from a command * @@ -66,11 +65,6 @@ class AsyncBusTest extends TestCase { */ public static $lastCommand; - /** - * @var \OCP\BackgroundJob\IJobList - */ - private $jobList; - /** * @var \OCP\Command\IBus */ @@ -80,47 +74,60 @@ class AsyncBusTest extends TestCase { self::$lastCommand = 'static'; } + /** + * @return IBus + */ + protected function getBus() { + if (!$this->bus instanceof IBus) { + $this->bus = $this->createBus(); + } + return $this->bus; + } + + /** + * @return IBus + */ + abstract protected function createBus(); + public function setUp() { - $this->jobList = new DummyJobList(); - $this->bus = new \OC\Command\AsyncBus($this->jobList); self::$lastCommand = ''; } public function testSimpleCommand() { $command = new SimpleCommand(); - $this->bus->push($command); + $this->getBus()->push($command); $this->runJobs(); $this->assertEquals('SimpleCommand', self::$lastCommand); } public function testStateFullCommand() { $command = new StateFullCommand('foo'); - $this->bus->push($command); + $this->getBus()->push($command); $this->runJobs(); $this->assertEquals('foo', self::$lastCommand); } public function testStaticCallable() { - $this->bus->push(['\Test\Command\AsyncBusTest', 'DummyCommand']); + $this->getBus()->push(['\Test\Command\AsyncBusTest', 'DummyCommand']); $this->runJobs(); $this->assertEquals('static', self::$lastCommand); } public function testMemberCallable() { $command = new StateFullCommand('bar'); - $this->bus->push([$command, 'handle']); + $this->getBus()->push([$command, 'handle']); $this->runJobs(); $this->assertEquals('bar', self::$lastCommand); } public function testFunctionCallable() { - $this->bus->push('\Test\Command\BasicFunction'); + $this->getBus()->push('\Test\Command\BasicFunction'); $this->runJobs(); $this->assertEquals('function', self::$lastCommand); } public function testClosure() { - $this->bus->push(function () { + $this->getBus()->push(function () { AsyncBusTest::$lastCommand = 'closure'; }); $this->runJobs(); @@ -128,7 +135,7 @@ class AsyncBusTest extends TestCase { } public function testClosureSelf() { - $this->bus->push(function () { + $this->getBus()->push(function () { self::$lastCommand = 'closure-self'; }); $this->runJobs(); @@ -139,14 +146,14 @@ class AsyncBusTest extends TestCase { public function testClosureThis() { // clean class to prevent phpunit putting closure in $this $test = new ThisClosureTest(); - $test->test($this->bus); + $test->test($this->getBus()); $this->runJobs(); $this->assertEquals('closure-this', self::$lastCommand); } public function testClosureBind() { $state = 'bar'; - $this->bus->push(function () use ($state) { + $this->getBus()->push(function () use ($state) { self::$lastCommand = 'closure-' . $state; }); $this->runJobs(); @@ -154,15 +161,15 @@ class AsyncBusTest extends TestCase { } public function testFileFileAccessCommand() { - $this->bus->push(new FilesystemCommand()); + $this->getBus()->push(new FilesystemCommand()); $this->assertEquals('', self::$lastCommand); $this->runJobs(); $this->assertEquals('FileAccess', self::$lastCommand); } public function testFileFileAccessCommandSync() { - $this->bus->requireSync('\OC\Command\FileAccess'); - $this->bus->push(new FilesystemCommand()); + $this->getBus()->requireSync('\OC\Command\FileAccess'); + $this->getBus()->push(new FilesystemCommand()); $this->assertEquals('FileAccess', self::$lastCommand); self::$lastCommand = ''; $this->runJobs(); @@ -170,10 +177,5 @@ class AsyncBusTest extends TestCase { } - private function runJobs() { - $jobs = $this->jobList->getAll(); - foreach ($jobs as $job) { - $job->execute($this->jobList); - } - } + abstract protected function runJobs(); } diff --git a/tests/lib/Command/CronBusTest.php b/tests/lib/Command/CronBusTest.php new file mode 100644 index 0000000000..79fd13a99a --- /dev/null +++ b/tests/lib/Command/CronBusTest.php @@ -0,0 +1,50 @@ + + * + * @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 Test\Command; + +use OC\Command\CronBus; +use Test\BackgroundJob\DummyJobList; + +class CronBusTest extends AsyncBusTest { + /** + * @var \OCP\BackgroundJob\IJobList + */ + private $jobList; + + + public function setUp() { + parent::setUp(); + + $this->jobList = new DummyJobList(); + } + + protected function createBus() { + return new CronBus($this->jobList); + } + + protected function runJobs() { + $jobs = $this->jobList->getAll(); + foreach ($jobs as $job) { + $job->execute($this->jobList); + } + } +} \ No newline at end of file diff --git a/tests/lib/TestCase.php b/tests/lib/TestCase.php index c3aa2c73c9..818b3454c3 100644 --- a/tests/lib/TestCase.php +++ b/tests/lib/TestCase.php @@ -190,7 +190,9 @@ abstract class TestCase extends \PHPUnit_Framework_TestCase { self::assertEquals([], $errors, "There have been xml parsing errors"); } - \OC\Files\Cache\Storage::getGlobalCache()->clearCache(); + if ($this->IsDatabaseAccessAllowed()) { + \OC\Files\Cache\Storage::getGlobalCache()->clearCache(); + } // tearDown the traits $traits = $this->getTestTraits(); From 5431b8bfc203815dfbe1d041295896a54312b1f9 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Thu, 24 Aug 2017 16:21:50 +0200 Subject: [PATCH 2/3] allow configuring the command bus --- lib/private/Server.php | 71 ++++++++++++++++++++++++------------------ 1 file changed, 41 insertions(+), 30 deletions(-) diff --git a/lib/private/Server.php b/lib/private/Server.php index d49e782ea2..7df1c91d67 100644 --- a/lib/private/Server.php +++ b/lib/private/Server.php @@ -39,6 +39,7 @@ * along with this program. If not, see * */ + namespace OC; use bantu\IniGetWrapper\IniGetWrapper; @@ -139,7 +140,7 @@ class Server extends ServerContainer implements IServerContainer { parent::__construct(); $this->webRoot = $webRoot; - $this->registerService(\OCP\IServerContainer::class, function(IServerContainer $c) { + $this->registerService(\OCP\IServerContainer::class, function (IServerContainer $c) { return $c; }); @@ -149,7 +150,6 @@ class Server extends ServerContainer implements IServerContainer { $this->registerAlias(IActionFactory::class, ActionFactory::class); - $this->registerService(\OCP\IPreview::class, function (Server $c) { return new PreviewManager( $c->getConfig(), @@ -256,8 +256,8 @@ class Server extends ServerContainer implements IServerContainer { }); $this->registerAlias('SystemTagObjectMapper', \OCP\SystemTag\ISystemTagObjectMapper::class); - $this->registerService(\OCP\Files\IRootFolder::class, function(Server $c) { - return new LazyRoot(function() use ($c) { + $this->registerService(\OCP\Files\IRootFolder::class, function (Server $c) { + return new LazyRoot(function () use ($c) { return $c->query('RootFolder'); }); }); @@ -295,7 +295,7 @@ class Server extends ServerContainer implements IServerContainer { }); $this->registerAlias('GroupManager', \OCP\IGroupManager::class); - $this->registerService(Store::class, function(Server $c) { + $this->registerService(Store::class, function (Server $c) { $session = $c->getSession(); if (\OC::$server->getSystemConfig()->getValue('installed', false)) { $tokenProvider = $c->query('OC\Authentication\Token\IProvider'); @@ -535,7 +535,7 @@ class Server extends ServerContainer implements IServerContainer { }); $this->registerAlias('Search', \OCP\ISearch::class); - $this->registerService(\OC\Security\RateLimiting\Limiter::class, function($c) { + $this->registerService(\OC\Security\RateLimiting\Limiter::class, function ($c) { return new \OC\Security\RateLimiting\Limiter( $this->getUserSession(), $this->getRequest(), @@ -543,7 +543,7 @@ class Server extends ServerContainer implements IServerContainer { $c->query(\OC\Security\RateLimiting\Backend\IBackend::class) ); }); - $this->registerService(\OC\Security\RateLimiting\Backend\IBackend::class, function($c) { + $this->registerService(\OC\Security\RateLimiting\Backend\IBackend::class, function ($c) { return new \OC\Security\RateLimiting\Backend\MemoryCache( $this->getMemCacheFactory(), new \OC\AppFramework\Utility\TimeFactory() @@ -677,7 +677,7 @@ class Server extends ServerContainer implements IServerContainer { $this->registerService(\OCP\Files\Config\IMountProviderCollection::class, function (Server $c) { $loader = \OC\Files\Filesystem::getLoader(); $mountCache = $c->query('UserMountCache'); - $manager = new \OC\Files\Config\MountProviderCollection($loader, $mountCache); + $manager = new \OC\Files\Config\MountProviderCollection($loader, $mountCache); // builtin providers @@ -694,13 +694,24 @@ class Server extends ServerContainer implements IServerContainer { return new IniGetWrapper(); }); $this->registerService('AsyncCommandBus', function (Server $c) { - $jobList = $c->getJobList(); - return new CronBus($jobList); + $busClass = $c->getConfig()->getSystemValue('commandbus'); + if ($busClass) { + list($app, $class) = explode('::', $busClass, 2); + if ($c->getAppManager()->isInstalled($app)) { + \OC_App::loadApp($app); + return $c->query($class); + } else { + throw new ServiceUnavailableException("The app providing the command bus ($app) is not enabled"); + } + } else { + $jobList = $c->getJobList(); + return new CronBus($jobList); + } }); $this->registerService('TrustedDomainHelper', function ($c) { return new TrustedDomainHelper($this->getConfig()); }); - $this->registerService('Throttler', function(Server $c) { + $this->registerService('Throttler', function (Server $c) { return new Throttler( $c->getDatabaseConnection(), new TimeFactory(), @@ -711,7 +722,7 @@ class Server extends ServerContainer implements IServerContainer { $this->registerService('IntegrityCodeChecker', function (Server $c) { // IConfig and IAppManager requires a working database. This code // might however be called when ownCloud is not yet setup. - if(\OC::$server->getSystemConfig()->getValue('installed', false)) { + if (\OC::$server->getSystemConfig()->getValue('installed', false)) { $config = $c->getConfig(); $appManager = $c->getAppManager(); } else { @@ -720,13 +731,13 @@ class Server extends ServerContainer implements IServerContainer { } return new Checker( - new EnvironmentHelper(), - new FileAccessHelper(), - new AppLocator(), - $config, - $c->getMemCacheFactory(), - $appManager, - $c->getTempManager() + new EnvironmentHelper(), + new FileAccessHelper(), + new AppLocator(), + $config, + $c->getMemCacheFactory(), + $appManager, + $c->getTempManager() ); }); $this->registerService(\OCP\IRequest::class, function ($c) { @@ -776,10 +787,10 @@ class Server extends ServerContainer implements IServerContainer { }); $this->registerAlias('Mailer', \OCP\Mail\IMailer::class); - $this->registerService('LDAPProvider', function(Server $c) { + $this->registerService('LDAPProvider', function (Server $c) { $config = $c->getConfig(); $factoryClass = $config->getSystemValue('ldapProviderFactory', null); - if(is_null($factoryClass)) { + if (is_null($factoryClass)) { throw new \Exception('ldapProviderFactory not set'); } /** @var \OCP\LDAP\ILDAPProviderFactory $factory */ @@ -845,7 +856,7 @@ class Server extends ServerContainer implements IServerContainer { }); $this->registerAlias('CapabilitiesManager', \OC\CapabilitiesManager::class); - $this->registerService(\OCP\Comments\ICommentsManager::class, function(Server $c) { + $this->registerService(\OCP\Comments\ICommentsManager::class, function (Server $c) { $config = $c->getConfig(); $factoryClass = $config->getSystemValue('comments.managerFactory', '\OC\Comments\ManagerFactory'); /** @var \OCP\Comments\ICommentsManagerFactory $factory */ @@ -854,7 +865,7 @@ class Server extends ServerContainer implements IServerContainer { }); $this->registerAlias('CommentsManager', \OCP\Comments\ICommentsManager::class); - $this->registerService('ThemingDefaults', function(Server $c) { + $this->registerService('ThemingDefaults', function (Server $c) { /* * Dark magic for autoloader. * If we do a class_exists it will try to load the class which will @@ -880,7 +891,7 @@ class Server extends ServerContainer implements IServerContainer { } return new \OC_Defaults(); }); - $this->registerService(SCSSCacher::class, function(Server $c) { + $this->registerService(SCSSCacher::class, function (Server $c) { /** @var Factory $cacheFactory */ $cacheFactory = $c->query(Factory::class); return new SCSSCacher( @@ -940,14 +951,14 @@ class Server extends ServerContainer implements IServerContainer { }); $this->registerAlias('ContentSecurityPolicyManager', \OCP\Security\IContentSecurityPolicyManager::class); - $this->registerService('ContentSecurityPolicyNonceManager', function(Server $c) { + $this->registerService('ContentSecurityPolicyNonceManager', function (Server $c) { return new ContentSecurityPolicyNonceManager( $c->getCsrfTokenManager(), $c->getRequest() ); }); - $this->registerService(\OCP\Share\IManager::class, function(Server $c) { + $this->registerService(\OCP\Share\IManager::class, function (Server $c) { $config = $c->getConfig(); $factoryClass = $config->getSystemValue('sharing.managerFactory', '\OC\Share20\ProviderFactory'); /** @var \OCP\Share\IProviderFactory $factory */ @@ -974,7 +985,7 @@ class Server extends ServerContainer implements IServerContainer { }); $this->registerAlias('ShareManager', \OCP\Share\IManager::class); - $this->registerService('SettingsManager', function(Server $c) { + $this->registerService('SettingsManager', function (Server $c) { $manager = new \OC\Settings\Manager( $c->getLogger(), $c->getDatabaseConnection(), @@ -1002,7 +1013,7 @@ class Server extends ServerContainer implements IServerContainer { }); $this->registerService('LockdownManager', function (Server $c) { - return new LockdownManager(function() use ($c) { + return new LockdownManager(function () use ($c) { return $c->getSession(); }); }); @@ -1038,11 +1049,11 @@ class Server extends ServerContainer implements IServerContainer { }); $this->registerAlias('Defaults', \OCP\Defaults::class); - $this->registerService(\OCP\ISession::class, function(SimpleContainer $c) { + $this->registerService(\OCP\ISession::class, function (SimpleContainer $c) { return $c->query(\OCP\IUserSession::class)->getSession(); }); - $this->registerService(IShareHelper::class, function(Server $c) { + $this->registerService(IShareHelper::class, function (Server $c) { return new ShareHelper( $c->query(\OCP\Share\IManager::class) ); From 1ebf91ec16c00ea0696defe70d29884e2819237c Mon Sep 17 00:00:00 2001 From: Morris Jobke Date: Mon, 4 Sep 2017 14:21:44 +0200 Subject: [PATCH 3/3] Add proper EOL Signed-off-by: Morris Jobke --- lib/private/Command/CronBus.php | 2 +- tests/lib/Command/CronBusTest.php | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/private/Command/CronBus.php b/lib/private/Command/CronBus.php index a0ad9b3c86..9bde4d8824 100644 --- a/lib/private/Command/CronBus.php +++ b/lib/private/Command/CronBus.php @@ -72,4 +72,4 @@ class CronBus extends AsyncBus { throw new \InvalidArgumentException('Invalid command'); } } -} \ No newline at end of file +} diff --git a/tests/lib/Command/CronBusTest.php b/tests/lib/Command/CronBusTest.php index 79fd13a99a..9bf53ba92d 100644 --- a/tests/lib/Command/CronBusTest.php +++ b/tests/lib/Command/CronBusTest.php @@ -47,4 +47,4 @@ class CronBusTest extends AsyncBusTest { $job->execute($this->jobList); } } -} \ No newline at end of file +}