From 09589b76c8e2d4a4a4c7181a14468e1d673ee291 Mon Sep 17 00:00:00 2001 From: Ferdinand Thiessen Date: Sat, 17 May 2025 00:55:41 +0200 Subject: [PATCH 1/5] fix(Router): correctly add routes to separate collections - Add routes to individual collections for each app and then merge them for the root collection holding all routes for the URL generator. - Short cut route loading with already loaded apps - Simplify active collection handling - Remove deprecated `OC_App` usage - Fully type Router methods Signed-off-by: Ferdinand Thiessen --- lib/private/Route/CachingRouter.php | 8 +- lib/private/Route/Router.php | 167 +++++++++++----------------- tests/lib/Route/RouterTest.php | 6 + 3 files changed, 78 insertions(+), 103 deletions(-) diff --git a/lib/private/Route/CachingRouter.php b/lib/private/Route/CachingRouter.php index dbd5ef02603eb..3d05f7c9146e6 100644 --- a/lib/private/Route/CachingRouter.php +++ b/lib/private/Route/CachingRouter.php @@ -43,10 +43,12 @@ public function __construct( * * @param string $name Name of the route to use. * @param array $parameters Parameters for the route - * @param bool $absolute - * @return string */ - public function generate($name, $parameters = [], $absolute = false) { + public function generate( + string $name, + array $parameters = [], + bool $absolute = false, + ): string { asort($parameters); $key = $this->context->getHost() . '#' . $this->context->getBaseUrl() . $name . sha1(json_encode($parameters)) . (int)$absolute; $cachedKey = $this->cache->get($key); diff --git a/lib/private/Route/Router.php b/lib/private/Route/Router.php index 02f371e808a44..455f2102323b5 100644 --- a/lib/private/Route/Router.php +++ b/lib/private/Route/Router.php @@ -31,24 +31,19 @@ use Symfony\Component\Routing\RouteCollection; class Router implements IRouter { + /** @var string[] */ + protected ?array $routingFiles = null; /** @var RouteCollection[] */ - protected $collections = []; - /** @var null|RouteCollection */ - protected $collection = null; - /** @var null|string */ - protected $collectionName = null; - /** @var null|RouteCollection */ - protected $root = null; - /** @var null|UrlGenerator */ - protected $generator = null; - /** @var string[]|null */ - protected $routingFiles; - /** @var bool */ - protected $loaded = false; - /** @var array */ - protected $loadedApps = []; - /** @var RequestContext */ - protected $context; + protected array $collections = []; + protected RouteCollection $root; + protected ?UrlGenerator $generator = null; + protected bool $loaded = false; + /** @var Array */ + protected array $loadedApps = []; + protected RequestContext $context; + + // for legacy reasons - todo: drop + protected string $collectionName = 'root'; public function __construct( protected LoggerInterface $logger, @@ -98,33 +93,35 @@ public function getRoutingFiles() { } /** - * Loads the routes - * - * @param null|string $app + * Loads the routes. */ - public function loadRoutes($app = null) { + public function loadRoutes(?string $app = null): void { if (is_string($app)) { $app = $this->appManager->cleanAppId($app); + // skip loading if not enabled for user + if (!$this->appManager->isEnabledForUser($app)) { + return; + } } - $requestedApp = $app; - if ($this->loaded) { + // already loaded so we skip + if ($this->loaded || ($app !== null && in_array($app, $this->loadedApps))) { return; } + + $requestedApp = $app ?? 'all'; $this->eventLogger->start('route:load:' . $requestedApp, 'Loading Routes for ' . $requestedApp); + if (is_null($app)) { $this->loaded = true; $routingFiles = $this->getRoutingFiles(); - $this->eventLogger->start('route:load:attributes', 'Loading Routes from attributes'); foreach ($this->appManager->getEnabledApps() as $enabledApp) { $this->loadAttributeRoutes($enabledApp); } $this->eventLogger->end('route:load:attributes'); } else { - if (isset($this->loadedApps[$app])) { - return; - } + $this->loadAttributeRoutes($app); try { $appPath = $this->appManager->getAppPath($app); $file = $appPath . '/appinfo/routes.php'; @@ -136,59 +133,38 @@ public function loadRoutes($app = null) { } catch (AppPathNotFoundException) { $routingFiles = []; } - - if ($this->appManager->isEnabledForUser($app)) { - $this->loadAttributeRoutes($app); - } } $this->eventLogger->start('route:load:files', 'Loading Routes from files'); foreach ($routingFiles as $app => $file) { - if (!isset($this->loadedApps[$app])) { - if (!$this->appManager->isAppLoaded($app)) { - // app MUST be loaded before app routes - // try again next time loadRoutes() is called - $this->loaded = false; - continue; - } - $this->loadedApps[$app] = true; - $this->useCollection($app); - $this->requireRouteFile($file, $app); - $collection = $this->getCollection($app); - $this->root->addCollection($collection); - - // Also add the OCS collection - $collection = $this->getCollection($app . '.ocs'); - $collection->addPrefix('/ocsapp'); - $this->root->addCollection($collection); + if (isset($this->loadedApps[$app])) { + continue; } + if (!$this->appManager->isAppLoaded($app)) { + // app MUST be loaded before app routes + // try again next time loadRoutes() is called + $this->loaded = false; + continue; + } + + $this->loadedApps[$app] = true; + $this->requireRouteFile($file, $app); } $this->eventLogger->end('route:load:files'); if (!isset($this->loadedApps['core'])) { $this->loadedApps['core'] = true; - $this->useCollection('root'); $this->setupRoutes($this->getAttributeRoutes('core'), 'core'); $this->requireRouteFile(__DIR__ . '/../../../core/routes.php', 'core'); - - // Also add the OCS collection - $collection = $this->getCollection('root.ocs'); - $collection->addPrefix('/ocsapp'); - $this->root->addCollection($collection); - } - if ($this->loaded) { - $collection = $this->getCollection('ocs'); - $collection->addPrefix('/ocs'); - $this->root->addCollection($collection); } + $this->eventLogger->end('route:load:' . $requestedApp); } /** - * @param string $name - * @return \Symfony\Component\Routing\RouteCollection + * Get the collection for the specified app */ - protected function getCollection($name) { + public function getCollection(string $name): RouteCollection { if (!isset($this->collections[$name])) { $this->collections[$name] = new RouteCollection(); } @@ -202,7 +178,6 @@ protected function getCollection($name) { * @return void */ public function useCollection($name) { - $this->collection = $this->getCollection($name); $this->collectionName = $name; } @@ -230,7 +205,9 @@ public function create($name, array $defaults = [], array $requirements = []) { $route = new Route($pattern, $defaults, $requirements); - $this->collection->add($name, $route); + + $collection = $this->getCollection($this->collectionName); + $collection->add($name, $route); return $route; } @@ -351,11 +328,8 @@ protected function callLegacyActionRoute(array $parameters): void { /** * Get the url generator - * - * @return \Symfony\Component\Routing\Generator\UrlGenerator - * */ - public function getGenerator() { + public function getGenerator(): UrlGenerator { if ($this->generator !== null) { return $this->generator; } @@ -368,12 +342,13 @@ public function getGenerator() { * * @param string $name Name of the route to use. * @param array $parameters Parameters for the route - * @param bool $absolute * @return string */ - public function generate($name, - $parameters = [], - $absolute = false) { + public function generate( + string $name, + array $parameters = [], + bool $absolute = false, + ): string { $referenceType = UrlGenerator::ABSOLUTE_URL; if ($absolute === false) { $referenceType = UrlGenerator::ABSOLUTE_PATH; @@ -445,16 +420,7 @@ private function loadAttributeRoutes(string $app): void { if (count($routes) === 0) { return; } - - $this->useCollection($app); $this->setupRoutes($routes, $app); - $collection = $this->getCollection($app); - $this->root->addCollection($collection); - - // Also add the OCS collection - $collection = $this->getCollection($app . '.ocs'); - $collection->addPrefix('/ocsapp'); - $this->root->addCollection($collection); } /** @@ -512,33 +478,34 @@ private function getAttributeRoutes(string $app): array { * @param string $file the route file location to include * @param string $appName */ - protected function requireRouteFile(string $file, string $appName): void { - $this->setupRoutes(include $file, $appName); + private function requireRouteFile(string $file, string $appName): void { + try { + $this->setupRoutes(include $file, $appName); + } catch (\TypeError) { + $this->logger->debug('Routes should only be registered by returning an array of routes from the routes.php'); + } } /** - * If a routes.php file returns an array, try to set up the application and - * register the routes for the app. The application class will be chosen by - * camelcasing the appname, e.g.: my_app will be turned into - * \OCA\MyApp\AppInfo\Application. If that class does not exist, a default - * App will be initialized. This makes it optional to ship an - * appinfo/application.php by using the built in query resolver + * Parse our custom route format and add the routes to the collection. * * @param array $routes the application routes * @param string $appName the name of the app. */ - private function setupRoutes($routes, $appName) { - if (is_array($routes)) { - $routeParser = new RouteParser(); - - $defaultRoutes = $routeParser->parseDefaultRoutes($routes, $appName); - $ocsRoutes = $routeParser->parseOCSRoutes($routes, $appName); - - $this->root->addCollection($defaultRoutes); - $ocsRoutes->addPrefix('/ocsapp'); - $this->root->addCollection($ocsRoutes); - } + private function setupRoutes(array $routes, string $appName): void { + $routeParser = new RouteParser(); + + $defaultCollection = $this->getCollection($appName); + $defaultRoutes = $routeParser->parseDefaultRoutes($routes, $appName); + $defaultCollection->addCollection($defaultRoutes); + $this->root->addCollection($defaultCollection); + + $ocsCollection = $this->getCollection("$appName.ocs"); + $ocsRoutes = $routeParser->parseOCSRoutes($routes, $appName); + $ocsCollection->addCollection($ocsRoutes); + $ocsCollection->addPrefix('/ocsapp'); + $this->root->addCollection($ocsCollection); } private function getApplicationClass(string $appName) { diff --git a/tests/lib/Route/RouterTest.php b/tests/lib/Route/RouterTest.php index f99ebe4767ffe..21ae2e9dfac20 100644 --- a/tests/lib/Route/RouterTest.php +++ b/tests/lib/Route/RouterTest.php @@ -66,6 +66,12 @@ public function testGenerateConsecutively(): void { $this->appManager->expects(self::atLeastOnce()) ->method('isAppLoaded') ->willReturn(true); + $this->appManager->expects(self::atLeastOnce()) + ->method('isEnabledForUser') + ->willReturn(true); + $this->appManager->expects(self::exactly(2)) + ->method('getEnabledApps') + ->willReturn(['files', 'dav']); $this->assertEquals('/index.php/apps/files/', $this->router->generate('files.view.index')); From 4c749113f74dcb0eaba1c699abb8136078dfd631 Mon Sep 17 00:00:00 2001 From: Ferdinand Thiessen Date: Sat, 17 May 2025 00:58:01 +0200 Subject: [PATCH 2/5] feat: add command to list all routes registered Signed-off-by: Ferdinand Thiessen --- build/psalm-baseline.xml | 8 -- core/Command/Router/ListRoutes.php | 99 +++++++++++++++++++++ core/register_command.php | 3 + lib/composer/composer/autoload_classmap.php | 1 + lib/composer/composer/autoload_static.php | 1 + lib/private/Route/CachingRouter.php | 2 +- lib/private/Route/Router.php | 2 +- 7 files changed, 106 insertions(+), 10 deletions(-) create mode 100644 core/Command/Router/ListRoutes.php diff --git a/build/psalm-baseline.xml b/build/psalm-baseline.xml index 39eeb1e91d110..6a311383810b1 100644 --- a/build/psalm-baseline.xml +++ b/build/psalm-baseline.xml @@ -4335,14 +4335,6 @@ - - - - - - collectionName]]> - - request->server]]> diff --git a/core/Command/Router/ListRoutes.php b/core/Command/Router/ListRoutes.php new file mode 100644 index 0000000000000..458579ec81873 --- /dev/null +++ b/core/Command/Router/ListRoutes.php @@ -0,0 +1,99 @@ +setName('router:list') + ->setDescription('List registered routes') + ->addArgument('appId', InputArgument::OPTIONAL, 'Limit routes to specific app', '') + ->addOption('grouped', 'g', InputOption::VALUE_NONE, 'Group routes by app'); + } + + protected function execute(InputInterface $input, OutputInterface $output): int { + $this->input = $input; + + $app = (string)$input->getArgument('appId'); + $apps = $app === '' ? $this->appManager->getEnabledApps() : [$app]; + + $this->router->loadRoutes(); + $allRoutes = []; + foreach ($apps as $appId) { + $routes = $this->router->getCollection($appId)->all(); + $routes = array_merge($routes, $this->router->getCollection("$appId.ocs")->all()); + $allRoutes = array_merge($allRoutes, $routes); + + if (!empty($routes) && $input->getOption('grouped')) { + $output->writeln("\nRoutes of $appId:"); + $rows = $this->formatRoutes($routes); + $this->writeTableInOutputFormat($input, $output, $rows); + } + } + if (!$input->getOption('grouped')) { + $rows = $this->formatRoutes($allRoutes); + $this->writeTableInOutputFormat($input, $output, $rows); + } + return 0; + } + + /** + * @param Route[] $routes + */ + private function formatRoutes(array $routes): array { + $rows = []; + foreach ($routes as $route) { + [$realApp, $controller, $function] = $route->getDefault('caller'); + //print_r($route->__serialize()); + $rows[] = [ + 'app' => $realApp, + 'controller' => $controller, + 'function' => $function, + 'method' => $route->getMethods()[0], + 'path' => str_replace('/ocsapp', '/ocs/v2.php', $route->getPath()), + 'defaults' => $this->formatDefaults($route->getDefaults()), + 'requirements' => $this->formatArray($route->getRequirements()), + ]; + } + return $rows; + } + + private function formatDefaults(array $defaults): array|string { + $defaults = array_filter($defaults, fn (string $name) => !in_array($name, ['action', 'caller']), ARRAY_FILTER_USE_KEY); + return $this->formatArray($defaults); + } + + private function formatArray(array $value): array|string { + if (str_starts_with($this->input->getOption('output'), self::OUTPUT_FORMAT_JSON)) { + return $value; + } + return empty($value) ? '' : json_encode($value); + } +} diff --git a/core/register_command.php b/core/register_command.php index 72a4b70f059ca..60b6214d94c89 100644 --- a/core/register_command.php +++ b/core/register_command.php @@ -71,6 +71,7 @@ use OC\Core\Command\Memcache\RedisCommand; use OC\Core\Command\Preview\Generate; use OC\Core\Command\Preview\ResetRenderedTexts; +use OC\Core\Command\Router\ListRoutes; use OC\Core\Command\Security\BruteforceAttempts; use OC\Core\Command\Security\BruteforceResetAttempts; use OC\Core\Command\Security\ExportCertificates; @@ -243,6 +244,8 @@ $application->add(Server::get(Statistics::class)); $application->add(Server::get(RedisCommand::class)); + + $application->add(Server::get(ListRoutes::class)); } else { $application->add(Server::get(Command\Maintenance\Install::class)); } diff --git a/lib/composer/composer/autoload_classmap.php b/lib/composer/composer/autoload_classmap.php index 36f64d970c3ab..b5d8abdf32725 100644 --- a/lib/composer/composer/autoload_classmap.php +++ b/lib/composer/composer/autoload_classmap.php @@ -1295,6 +1295,7 @@ 'OC\\Core\\Command\\Preview\\Generate' => $baseDir . '/core/Command/Preview/Generate.php', 'OC\\Core\\Command\\Preview\\Repair' => $baseDir . '/core/Command/Preview/Repair.php', 'OC\\Core\\Command\\Preview\\ResetRenderedTexts' => $baseDir . '/core/Command/Preview/ResetRenderedTexts.php', + 'OC\\Core\\Command\\Router\\ListRoutes' => $baseDir . '/core/Command/Router/ListRoutes.php', 'OC\\Core\\Command\\Security\\BruteforceAttempts' => $baseDir . '/core/Command/Security/BruteforceAttempts.php', 'OC\\Core\\Command\\Security\\BruteforceResetAttempts' => $baseDir . '/core/Command/Security/BruteforceResetAttempts.php', 'OC\\Core\\Command\\Security\\ExportCertificates' => $baseDir . '/core/Command/Security/ExportCertificates.php', diff --git a/lib/composer/composer/autoload_static.php b/lib/composer/composer/autoload_static.php index 327366ca889a0..8af5836003531 100644 --- a/lib/composer/composer/autoload_static.php +++ b/lib/composer/composer/autoload_static.php @@ -1336,6 +1336,7 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2 'OC\\Core\\Command\\Preview\\Generate' => __DIR__ . '/../../..' . '/core/Command/Preview/Generate.php', 'OC\\Core\\Command\\Preview\\Repair' => __DIR__ . '/../../..' . '/core/Command/Preview/Repair.php', 'OC\\Core\\Command\\Preview\\ResetRenderedTexts' => __DIR__ . '/../../..' . '/core/Command/Preview/ResetRenderedTexts.php', + 'OC\\Core\\Command\\Router\\ListRoutes' => __DIR__ . '/../../..' . '/core/Command/Router/ListRoutes.php', 'OC\\Core\\Command\\Security\\BruteforceAttempts' => __DIR__ . '/../../..' . '/core/Command/Security/BruteforceAttempts.php', 'OC\\Core\\Command\\Security\\BruteforceResetAttempts' => __DIR__ . '/../../..' . '/core/Command/Security/BruteforceResetAttempts.php', 'OC\\Core\\Command\\Security\\ExportCertificates' => __DIR__ . '/../../..' . '/core/Command/Security/ExportCertificates.php', diff --git a/lib/private/Route/CachingRouter.php b/lib/private/Route/CachingRouter.php index 3d05f7c9146e6..d88811f729ec9 100644 --- a/lib/private/Route/CachingRouter.php +++ b/lib/private/Route/CachingRouter.php @@ -145,7 +145,7 @@ protected function requireRouteFile(string $file, string $appName): void { $this->legacyCreatedRoutes = []; parent::requireRouteFile($file, $appName); foreach ($this->legacyCreatedRoutes as $routeName) { - $route = $this->collection?->get($routeName); + $route = $this->getCollection($appName)->get($routeName); if ($route === null) { /* Should never happen */ throw new \Exception("Could not find route $routeName"); diff --git a/lib/private/Route/Router.php b/lib/private/Route/Router.php index 455f2102323b5..566d22991e8b7 100644 --- a/lib/private/Route/Router.php +++ b/lib/private/Route/Router.php @@ -478,7 +478,7 @@ private function getAttributeRoutes(string $app): array { * @param string $file the route file location to include * @param string $appName */ - private function requireRouteFile(string $file, string $appName): void { + protected function requireRouteFile(string $file, string $appName): void { try { $this->setupRoutes(include $file, $appName); } catch (\TypeError) { From 14653ef24671dc842fbeab8a1f63560ac539ba28 Mon Sep 17 00:00:00 2001 From: Ferdinand Thiessen Date: Sat, 17 May 2025 13:10:40 +0200 Subject: [PATCH 3/5] fixup! fix(Router): correctly add routes to separate collections --- lib/private/Route/Router.php | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/lib/private/Route/Router.php b/lib/private/Route/Router.php index 566d22991e8b7..901b56310e91c 100644 --- a/lib/private/Route/Router.php +++ b/lib/private/Route/Router.php @@ -136,19 +136,21 @@ public function loadRoutes(?string $app = null): void { } $this->eventLogger->start('route:load:files', 'Loading Routes from files'); - foreach ($routingFiles as $app => $file) { - if (isset($this->loadedApps[$app])) { + foreach ($routingFiles as $appId => $file) { + if (isset($this->loadedApps[$appId])) { continue; } - if (!$this->appManager->isAppLoaded($app)) { + if (!$this->appManager->isAppLoaded($appId)) { // app MUST be loaded before app routes // try again next time loadRoutes() is called $this->loaded = false; continue; } - - $this->loadedApps[$app] = true; - $this->requireRouteFile($file, $app); + $this->requireRouteFile($file, $appId); + // mark fully loaded app as loaded (route file + annotation) + if ($app === null || $app === $appId) { + $this->loadedApps[$appId] = true; + } } $this->eventLogger->end('route:load:files'); From 129983fc12e07166a7fc8f014742817d0fa397e9 Mon Sep 17 00:00:00 2001 From: Ferdinand Thiessen Date: Sat, 17 May 2025 13:13:34 +0200 Subject: [PATCH 4/5] fixup! fixup! fix(Router): correctly add routes to separate collections --- lib/private/Route/Router.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/private/Route/Router.php b/lib/private/Route/Router.php index 901b56310e91c..c26346bf998fc 100644 --- a/lib/private/Route/Router.php +++ b/lib/private/Route/Router.php @@ -105,7 +105,7 @@ public function loadRoutes(?string $app = null): void { } // already loaded so we skip - if ($this->loaded || ($app !== null && in_array($app, $this->loadedApps))) { + if ($this->loaded || ($app !== null && isset($this->loadedApps[$app]))) { return; } @@ -155,9 +155,9 @@ public function loadRoutes(?string $app = null): void { $this->eventLogger->end('route:load:files'); if (!isset($this->loadedApps['core'])) { - $this->loadedApps['core'] = true; $this->setupRoutes($this->getAttributeRoutes('core'), 'core'); $this->requireRouteFile(__DIR__ . '/../../../core/routes.php', 'core'); + $this->loadedApps['core'] = true; } $this->eventLogger->end('route:load:' . $requestedApp); From 0eabd9fff88048895017b4c8409db1350f44de5a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=B4me=20Chilliet?= Date: Tue, 10 Jun 2025 10:37:36 +0200 Subject: [PATCH 5/5] fix(router): Fix loading legacy routes in the correct collection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Côme Chilliet --- lib/private/Route/CachingRouter.php | 1 - lib/private/Route/Router.php | 1 + 2 files changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/private/Route/CachingRouter.php b/lib/private/Route/CachingRouter.php index d88811f729ec9..314d4c22812b3 100644 --- a/lib/private/Route/CachingRouter.php +++ b/lib/private/Route/CachingRouter.php @@ -117,7 +117,6 @@ protected function callLegacyActionRoute(array $parameters): void { * Closures cannot be serialized to cache, so for legacy routes calling an action we have to include the routes.php file again */ $app = $parameters['app']; - $this->useCollection($app); parent::requireRouteFile($parameters['route-file'], $app); $collection = $this->getCollection($app); $parameters['action'] = $collection->get($parameters['_route'])?->getDefault('action'); diff --git a/lib/private/Route/Router.php b/lib/private/Route/Router.php index c26346bf998fc..1f6cb438cb205 100644 --- a/lib/private/Route/Router.php +++ b/lib/private/Route/Router.php @@ -482,6 +482,7 @@ private function getAttributeRoutes(string $app): array { */ protected function requireRouteFile(string $file, string $appName): void { try { + $this->useCollection($appName); $this->setupRoutes(include $file, $appName); } catch (\TypeError) { $this->logger->debug('Routes should only be registered by returning an array of routes from the routes.php');