From f8b74cf0a5c62f062d6f05add8e69874e7edc8aa Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Mon, 21 Jan 2019 12:02:36 +0100 Subject: [PATCH 1/2] Allow resources via OCS as well Signed-off-by: Joas Schilling --- .../AppFramework/Routing/RouteConfig.php | 56 +++++++++++- .../lib/AppFramework/Routing/RoutingTest.php | 85 +++++++++++++++++++ 2 files changed, 140 insertions(+), 1 deletion(-) diff --git a/lib/private/AppFramework/Routing/RouteConfig.php b/lib/private/AppFramework/Routing/RouteConfig.php index 70208725f4..30cd4cea16 100644 --- a/lib/private/AppFramework/Routing/RouteConfig.php +++ b/lib/private/AppFramework/Routing/RouteConfig.php @@ -84,6 +84,9 @@ class RouteConfig { // parse ocs simple routes $this->processOCS($this->routes); + // parse ocs simple routes + $this->processOCSResources($this->routes); + $this->router->useCollection($oldCollection); } @@ -190,7 +193,58 @@ class RouteConfig { * For a given name and url restful routes are created: * - index * - show - * - new + * - create + * - update + * - destroy + * + * @param array $routes + */ + private function processOCSResources($routes) + { + // declaration of all restful actions + $actions = array( + array('name' => 'index', 'verb' => 'GET', 'on-collection' => true), + array('name' => 'show', 'verb' => 'GET'), + array('name' => 'create', 'verb' => 'POST', 'on-collection' => true), + array('name' => 'update', 'verb' => 'PUT'), + array('name' => 'destroy', 'verb' => 'DELETE'), + ); + + $resources = $routes['ocs-resources'] ?? []; + foreach ($resources as $resource => $config) { + $root = $config['root'] ?? '/apps/' . $this->appName; + + // the url parameter used as id to the resource + foreach($actions as $action) { + $url = $root . $config['url']; + $method = $action['name']; + $verb = strtoupper($action['verb'] ?? 'GET'); + $collectionAction = $action['on-collection'] ?? false; + if (!$collectionAction) { + $url .= '/{id}'; + } + if (isset($action['url-postfix'])) { + $url .= '/' . $action['url-postfix']; + } + + $controller = $resource; + + $controllerName = $this->buildControllerName($controller); + $actionName = $this->buildActionName($method); + + $routeName = 'ocs.' . $this->appName . '.' . strtolower($resource) . '.' . strtolower($method); + + $this->router->create($routeName, $url)->method($verb)->action( + new RouteActionHandler($this->container, $controllerName, $actionName) + ); + } + } + } + + /** + * For a given name and url restful routes are created: + * - index + * - show * - create * - update * - destroy diff --git a/tests/lib/AppFramework/Routing/RoutingTest.php b/tests/lib/AppFramework/Routing/RoutingTest.php index 76533fff01..fccece481c 100644 --- a/tests/lib/AppFramework/Routing/RoutingTest.php +++ b/tests/lib/AppFramework/Routing/RoutingTest.php @@ -6,6 +6,9 @@ use OC\AppFramework\DependencyInjection\DIContainer; use OC\AppFramework\Routing\RouteActionHandler; use OC\AppFramework\Routing\RouteConfig; use OCP\ILogger; +use OCP\Route\IRouter; +use PHPUnit\Framework\MockObject\MockObject; +use OC\Route\Router; class RoutingTest extends \Test\TestCase { @@ -179,6 +182,27 @@ class RoutingTest extends \Test\TestCase $this->assertSimpleOCSRoute($routes, 'admin_folders.open_current', 'DELETE', '/apps/app1/folders/{folderId}/open', 'AdminFoldersController', 'openCurrent'); } + public function testOCSResource() + { + $routes = ['ocs-resources' => ['account' => ['url' => '/accounts']]]; + + $this->assertOCSResource($routes, 'account', '/apps/app1/accounts', 'AccountController', 'id'); + } + + public function testOCSResourceWithUnderScoreName() + { + $routes = ['ocs-resources' => ['admin_accounts' => ['url' => '/admin/accounts']]]; + + $this->assertOCSResource($routes, 'admin_accounts', '/apps/app1/admin/accounts', 'AdminAccountsController', 'id'); + } + + public function testOCSResourceWithRoot() + { + $routes = ['ocs-resources' => ['admin_accounts' => ['url' => '/admin/accounts', 'root' => '/core/endpoint']]]; + + $this->assertOCSResource($routes, 'admin_accounts', '/core/endpoint/admin/accounts', 'AdminAccountsController', 'id'); + } + public function testResource() { $routes = array('resources' => array('account' => array('url' => '/accounts'))); @@ -277,6 +301,67 @@ class RoutingTest extends \Test\TestCase $config->register(); } + /** + * @param array $yaml + * @param string $resourceName + * @param string $url + * @param string $controllerName + * @param string $paramName + */ + private function assertOCSResource($yaml, $resourceName, $url, $controllerName, $paramName): void { + /** @var IRouter|MockObject $router */ + $router = $this->getMockBuilder(Router::class) + ->setMethods(['create']) + ->setConstructorArgs([$this->getMockBuilder(ILogger::class)->getMock()]) + ->getMock(); + + // route mocks + $container = new DIContainer('app1'); + $indexRoute = $this->mockRoute($container, 'GET', $controllerName, 'index'); + $showRoute = $this->mockRoute($container, 'GET', $controllerName, 'show'); + $createRoute = $this->mockRoute($container, 'POST', $controllerName, 'create'); + $updateRoute = $this->mockRoute($container, 'PUT', $controllerName, 'update'); + $destroyRoute = $this->mockRoute($container, 'DELETE', $controllerName, 'destroy'); + + $urlWithParam = $url . '/{' . $paramName . '}'; + + // we expect create to be called once: + $router + ->expects($this->at(0)) + ->method('create') + ->with($this->equalTo('ocs.app1.' . $resourceName . '.index'), $this->equalTo($url)) + ->willReturn($indexRoute); + + $router + ->expects($this->at(1)) + ->method('create') + ->with($this->equalTo('ocs.app1.' . $resourceName . '.show'), $this->equalTo($urlWithParam)) + ->willReturn($showRoute); + + $router + ->expects($this->at(2)) + ->method('create') + ->with($this->equalTo('ocs.app1.' . $resourceName . '.create'), $this->equalTo($url)) + ->willReturn($createRoute); + + $router + ->expects($this->at(3)) + ->method('create') + ->with($this->equalTo('ocs.app1.' . $resourceName . '.update'), $this->equalTo($urlWithParam)) + ->willReturn($updateRoute); + + $router + ->expects($this->at(4)) + ->method('create') + ->with($this->equalTo('ocs.app1.' . $resourceName . '.destroy'), $this->equalTo($urlWithParam)) + ->willReturn($destroyRoute); + + // load route configuration + $config = new RouteConfig($container, $router, $yaml); + + $config->register(); + } + /** * @param string $resourceName * @param string $url From 92edd40e51a03b99243bf38559863da36b6ad3c1 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Mon, 21 Jan 2019 12:14:01 +0100 Subject: [PATCH 2/2] Make RouteConfig strict Signed-off-by: Joas Schilling --- .../AppFramework/Routing/RouteConfig.php | 110 ++++++++---------- 1 file changed, 48 insertions(+), 62 deletions(-) diff --git a/lib/private/AppFramework/Routing/RouteConfig.php b/lib/private/AppFramework/Routing/RouteConfig.php index 30cd4cea16..0477f4dc20 100644 --- a/lib/private/AppFramework/Routing/RouteConfig.php +++ b/lib/private/AppFramework/Routing/RouteConfig.php @@ -1,4 +1,5 @@ router->getCurrentCollection(); - $this->router->useCollection($oldCollection.'.ocs'); + $this->router->useCollection($oldCollection . '.ocs'); // parse ocs simple routes $this->processOCS($this->routes); @@ -90,39 +91,31 @@ class RouteConfig { $this->router->useCollection($oldCollection); } - private function processOCS(array $routes) { - $ocsRoutes = isset($routes['ocs']) ? $routes['ocs'] : []; + private function processOCS(array $routes): void { + $ocsRoutes = $routes['ocs'] ?? []; foreach ($ocsRoutes as $ocsRoute) { $name = $ocsRoute['name']; - $postfix = ''; - - if (isset($ocsRoute['postfix'])) { - $postfix = $ocsRoute['postfix']; - } - - if (isset($ocsRoute['root'])) { - $root = $ocsRoute['root']; - } else { - $root = '/apps/'.$this->appName; - } + $postfix = $ocsRoute['postfix'] ?? ''; + $root = $ocsRoute['root'] ?? '/apps/' . $this->appName; $url = $root . $ocsRoute['url']; - $verb = isset($ocsRoute['verb']) ? strtoupper($ocsRoute['verb']) : 'GET'; + $verb = strtoupper($ocsRoute['verb'] ?? 'GET'); $split = explode('#', $name, 2); - if (count($split) != 2) { + if (count($split) !== 2) { throw new \UnexpectedValueException('Invalid route name'); } - $controller = $split[0]; - $action = $split[1]; + list($controller, $action) = $split; $controllerName = $this->buildControllerName($controller); $actionName = $this->buildActionName($action); + $routeName = 'ocs.' . $this->appName . '.' . $controller . '.' . $action . $postfix; + // register the route $handler = new RouteActionHandler($this->container, $controllerName, $actionName); - $router = $this->router->create('ocs.'.$this->appName.'.'.$controller.'.'.$action . $postfix, $url) + $router = $this->router->create($routeName, $url) ->method($verb) ->action($handler); @@ -145,33 +138,29 @@ class RouteConfig { * @param array $routes * @throws \UnexpectedValueException */ - private function processSimpleRoutes($routes) - { - $simpleRoutes = isset($routes['routes']) ? $routes['routes'] : array(); + private function processSimpleRoutes(array $routes): void { + $simpleRoutes = $routes['routes'] ?? []; foreach ($simpleRoutes as $simpleRoute) { $name = $simpleRoute['name']; - $postfix = ''; - - if (isset($simpleRoute['postfix'])) { - $postfix = $simpleRoute['postfix']; - } + $postfix = $simpleRoute['postfix'] ?? ''; $url = $simpleRoute['url']; - $verb = isset($simpleRoute['verb']) ? strtoupper($simpleRoute['verb']) : 'GET'; + $verb = strtoupper($simpleRoute['verb'] ?? 'GET'); $split = explode('#', $name, 2); - if (count($split) != 2) { + if (count($split) !== 2) { throw new \UnexpectedValueException('Invalid route name'); } - $controller = $split[0]; - $action = $split[1]; + list($controller, $action) = $split; $controllerName = $this->buildControllerName($controller); $actionName = $this->buildActionName($action); + $routeName = $this->appName . '.' . $controller . '.' . $action . $postfix; + // register the route $handler = new RouteActionHandler($this->container, $controllerName, $actionName); - $router = $this->router->create($this->appName.'.'.$controller.'.'.$action . $postfix, $url) + $router = $this->router->create($routeName, $url) ->method($verb) ->action($handler); @@ -190,7 +179,7 @@ class RouteConfig { } /** - * For a given name and url restful routes are created: + * For a given name and url restful OCS routes are created: * - index * - show * - create @@ -199,16 +188,15 @@ class RouteConfig { * * @param array $routes */ - private function processOCSResources($routes) - { + private function processOCSResources(array $routes): void { // declaration of all restful actions - $actions = array( - array('name' => 'index', 'verb' => 'GET', 'on-collection' => true), - array('name' => 'show', 'verb' => 'GET'), - array('name' => 'create', 'verb' => 'POST', 'on-collection' => true), - array('name' => 'update', 'verb' => 'PUT'), - array('name' => 'destroy', 'verb' => 'DELETE'), - ); + $actions = [ + ['name' => 'index', 'verb' => 'GET', 'on-collection' => true], + ['name' => 'show', 'verb' => 'GET'], + ['name' => 'create', 'verb' => 'POST', 'on-collection' => true], + ['name' => 'update', 'verb' => 'PUT'], + ['name' => 'destroy', 'verb' => 'DELETE'], + ]; $resources = $routes['ocs-resources'] ?? []; foreach ($resources as $resource => $config) { @@ -251,31 +239,30 @@ class RouteConfig { * * @param array $routes */ - private function processResources($routes) - { + private function processResources(array $routes): void { // declaration of all restful actions - $actions = array( - array('name' => 'index', 'verb' => 'GET', 'on-collection' => true), - array('name' => 'show', 'verb' => 'GET'), - array('name' => 'create', 'verb' => 'POST', 'on-collection' => true), - array('name' => 'update', 'verb' => 'PUT'), - array('name' => 'destroy', 'verb' => 'DELETE'), - ); + $actions = [ + ['name' => 'index', 'verb' => 'GET', 'on-collection' => true], + ['name' => 'show', 'verb' => 'GET'], + ['name' => 'create', 'verb' => 'POST', 'on-collection' => true], + ['name' => 'update', 'verb' => 'PUT'], + ['name' => 'destroy', 'verb' => 'DELETE'], + ]; - $resources = isset($routes['resources']) ? $routes['resources'] : array(); + $resources = $routes['resources'] ?? []; foreach ($resources as $resource => $config) { // the url parameter used as id to the resource foreach($actions as $action) { $url = $config['url']; $method = $action['name']; - $verb = isset($action['verb']) ? strtoupper($action['verb']) : 'GET'; - $collectionAction = isset($action['on-collection']) ? $action['on-collection'] : false; + $verb = strtoupper($action['verb'] ?? 'GET'); + $collectionAction = $action['on-collection'] ?? false; if (!$collectionAction) { - $url = $url . '/{id}'; + $url .= '/{id}'; } if (isset($action['url-postfix'])) { - $url = $url . '/' . $action['url-postfix']; + $url .= '/' . $action['url-postfix']; } $controller = $resource; @@ -297,8 +284,7 @@ class RouteConfig { * @param string $controller * @return string */ - private function buildControllerName($controller) - { + private function buildControllerName(string $controller): string { if (!isset($this->controllerNameCache[$controller])) { $this->controllerNameCache[$controller] = $this->underScoreToCamelCase(ucfirst($controller)) . 'Controller'; } @@ -310,7 +296,7 @@ class RouteConfig { * @param string $action * @return string */ - private function buildActionName($action) { + private function buildActionName(string $action): string { return $this->underScoreToCamelCase($action); } @@ -319,12 +305,12 @@ class RouteConfig { * @param string $str * @return string */ - private function underScoreToCamelCase($str) { - $pattern = "/_[a-z]?/"; + private function underScoreToCamelCase(string $str): string { + $pattern = '/_[a-z]?/'; return preg_replace_callback( $pattern, function ($matches) { - return strtoupper(ltrim($matches[0], "_")); + return strtoupper(ltrim($matches[0], '_')); }, $str); }