Skip to content

Commit a705132

Browse files
juliusknorrjuliusknorr
authored
Merge pull request #36656 from nextcloud/route-instrumentation
2 parents 39b13e0 + b911da3 commit a705132

6 files changed

Lines changed: 123 additions & 35 deletions

File tree

‎lib/private/AppFramework/App.php‎

Lines changed: 17 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,6 @@
3434
use OC\AppFramework\DependencyInjection\DIContainer;
3535
use OC\AppFramework\Http\Dispatcher;
3636
use OC\AppFramework\Http\Request;
37-
use OC\Diagnostics\EventLogger;
3837
use OCP\Profiler\IProfiler;
3938
use OC\Profiler\RoutingDataCollector;
4039
use OCP\AppFramework\QueryException;
@@ -43,7 +42,6 @@
4342
use OCP\AppFramework\Http\IOutput;
4443
use OCP\Diagnostics\IEventLogger;
4544
use OCP\HintException;
46-
use OCP\IConfig;
4745
use OCP\IRequest;
4846

4947
/**
@@ -120,14 +118,16 @@ public static function getAppIdForClass(string $className, string $topNamespace
120118
public static function main(string $controllerName, string $methodName, DIContainer $container, array $urlParams = null) {
121119
/** @var IProfiler $profiler */
122120
$profiler = $container->get(IProfiler::class);
123-
$config = $container->get(IConfig::class);
121+
$eventLogger = $container->get(IEventLogger::class);
124122
// Disable profiler on the profiler UI
125123
$profiler->setEnabled($profiler->isEnabled() && !is_null($urlParams) && isset($urlParams['_route']) && !str_starts_with($urlParams['_route'], 'profiler.'));
126124
if ($profiler->isEnabled()) {
127125
\OC::$server->get(IEventLogger::class)->activate();
128126
$profiler->add(new RoutingDataCollector($container['AppName'], $controllerName, $methodName));
129127
}
130128

129+
$eventLogger->start('app:controller:params', 'Gather controller parameters');
130+
131131
if (!is_null($urlParams)) {
132132
/** @var Request $request */
133133
$request = $container->get(IRequest::class);
@@ -139,6 +139,10 @@ public static function main(string $controllerName, string $methodName, DIContai
139139
}
140140
$appName = $container['AppName'];
141141

142+
$eventLogger->end('app:controller:params');
143+
144+
$eventLogger->start('app:controller:load', 'Load app controller');
145+
142146
// first try $controllerName then go for \OCA\AppName\Controller\$controllerName
143147
try {
144148
$controller = $container->get($controllerName);
@@ -158,10 +162,18 @@ public static function main(string $controllerName, string $methodName, DIContai
158162
$controller = $container->query($controllerName);
159163
}
160164

165+
$eventLogger->end('app:controller:load');
166+
167+
$eventLogger->start('app:controller:dispatcher', 'Initialize dispatcher and pre-middleware');
168+
161169
// initialize the dispatcher and run all the middleware before the controller
162170
/** @var Dispatcher $dispatcher */
163171
$dispatcher = $container['Dispatcher'];
164172

173+
$eventLogger->end('app:controller:dispatcher');
174+
175+
$eventLogger->start('app:controller:run', 'Run app controller');
176+
165177
[
166178
$httpHeaders,
167179
$responseHeaders,
@@ -170,11 +182,11 @@ public static function main(string $controllerName, string $methodName, DIContai
170182
$response
171183
] = $dispatcher->dispatch($controller, $methodName);
172184

185+
$eventLogger->end('app:controller:run');
186+
173187
$io = $container[IOutput::class];
174188

175189
if ($profiler->isEnabled()) {
176-
/** @var EventLogger $eventLogger */
177-
$eventLogger = $container->get(IEventLogger::class);
178190
$eventLogger->end('runtime');
179191
$profile = $profiler->collect($container->get(IRequest::class), $response);
180192
$profiler->saveProfile($profile);

‎lib/private/Route/CachingRouter.php‎

Lines changed: 17 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -24,20 +24,27 @@
2424
*/
2525
namespace OC\Route;
2626

27+
use OCP\Diagnostics\IEventLogger;
28+
use OCP\ICache;
29+
use OCP\ICacheFactory;
30+
use OCP\IConfig;
31+
use OCP\IRequest;
32+
use Psr\Container\ContainerInterface;
2733
use Psr\Log\LoggerInterface;
2834

2935
class CachingRouter extends Router {
30-
/**
31-
* @var \OCP\ICache
32-
*/
33-
protected $cache;
36+
protected ICache $cache;
3437

35-
/**
36-
* @param \OCP\ICache $cache
37-
*/
38-
public function __construct($cache, LoggerInterface $logger) {
39-
$this->cache = $cache;
40-
parent::__construct($logger);
38+
public function __construct(
39+
ICacheFactory $cacheFactory,
40+
LoggerInterface $logger,
41+
IRequest $request,
42+
IConfig $config,
43+
IEventLogger $eventLogger,
44+
ContainerInterface $container
45+
) {
46+
$this->cache = $cacheFactory->createLocal('route');
47+
parent::__construct($logger, $request, $config, $eventLogger, $container);
4148
}
4249

4350
/**

‎lib/private/Route/Router.php‎

Lines changed: 29 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -34,8 +34,12 @@
3434

3535
use OC\AppFramework\Routing\RouteParser;
3636
use OCP\AppFramework\App;
37+
use OCP\Diagnostics\IEventLogger;
38+
use OCP\IConfig;
39+
use OCP\IRequest;
3740
use OCP\Route\IRouter;
3841
use OCP\Util;
42+
use Psr\Container\ContainerInterface;
3943
use Psr\Log\LoggerInterface;
4044
use Symfony\Component\Routing\Exception\ResourceNotFoundException;
4145
use Symfony\Component\Routing\Exception\RouteNotFoundException;
@@ -64,24 +68,35 @@ class Router implements IRouter {
6468
protected LoggerInterface $logger;
6569
/** @var RequestContext */
6670
protected $context;
67-
68-
public function __construct(LoggerInterface $logger) {
71+
private IEventLogger $eventLogger;
72+
private IConfig $config;
73+
private ContainerInterface $container;
74+
75+
public function __construct(
76+
LoggerInterface $logger,
77+
IRequest $request,
78+
IConfig $config,
79+
IEventLogger $eventLogger,
80+
ContainerInterface $container
81+
) {
6982
$this->logger = $logger;
83+
$this->config = $config;
7084
$baseUrl = \OC::$WEBROOT;
71-
if (!(\OC::$server->getConfig()->getSystemValue('htaccess.IgnoreFrontController', false) === true || getenv('front_controller_active') === 'true')) {
85+
if (!($config->getSystemValue('htaccess.IgnoreFrontController', false) === true || getenv('front_controller_active') === 'true')) {
7286
$baseUrl .= '/index.php';
7387
}
7488
if (!\OC::$CLI && isset($_SERVER['REQUEST_METHOD'])) {
7589
$method = $_SERVER['REQUEST_METHOD'];
7690
} else {
7791
$method = 'GET';
7892
}
79-
$request = \OC::$server->getRequest();
8093
$host = $request->getServerHost();
8194
$schema = $request->getServerProtocol();
8295
$this->context = new RequestContext($baseUrl, $method, $host, $schema);
8396
// TODO cache
8497
$this->root = $this->getCollection('root');
98+
$this->eventLogger = $eventLogger;
99+
$this->container = $container;
85100
}
86101

87102
/**
@@ -134,7 +149,7 @@ public function loadRoutes($app = null) {
134149
$routingFiles = [];
135150
}
136151
}
137-
\OC::$server->getEventLogger()->start('loadroutes' . $requestedApp, 'Loading Routes');
152+
$this->eventLogger->start('route:load:' . $requestedApp, 'Loading Routes for ' . $requestedApp);
138153
foreach ($routingFiles as $app => $file) {
139154
if (!isset($this->loadedApps[$app])) {
140155
if (!\OC_App::isAppLoaded($app)) {
@@ -170,7 +185,7 @@ public function loadRoutes($app = null) {
170185
$collection->addPrefix('/ocs');
171186
$this->root->addCollection($collection);
172187
}
173-
\OC::$server->getEventLogger()->end('loadroutes' . $requestedApp);
188+
$this->eventLogger->end('route:load:' . $requestedApp);
174189
}
175190

176191
/**
@@ -231,6 +246,7 @@ public function create($name,
231246
* @return array
232247
*/
233248
public function findMatchingRoute(string $url): array {
249+
$this->eventLogger->start('route:match', 'Match route');
234250
if (substr($url, 0, 6) === '/apps/') {
235251
// empty string / 'apps' / $app / rest of the route
236252
[, , $app,] = explode('/', $url, 4);
@@ -249,7 +265,7 @@ public function findMatchingRoute(string $url): array {
249265
$this->loadRoutes('settings');
250266
} elseif (substr($url, 0, 6) === '/core/') {
251267
\OC::$REQUESTEDAPP = $url;
252-
if (!\OC::$server->getConfig()->getSystemValueBool('maintenance') && !Util::needUpgrade()) {
268+
if (!$this->config->getSystemValueBool('maintenance') && !Util::needUpgrade()) {
253269
\OC_App::loadApps();
254270
}
255271
$this->loadRoutes('core');
@@ -276,6 +292,7 @@ public function findMatchingRoute(string $url): array {
276292
}
277293
}
278294

295+
$this->eventLogger->end('route:match');
279296
return $parameters;
280297
}
281298

@@ -289,7 +306,7 @@ public function findMatchingRoute(string $url): array {
289306
public function match($url) {
290307
$parameters = $this->findMatchingRoute($url);
291308

292-
\OC::$server->getEventLogger()->start('run_route', 'Run route');
309+
$this->eventLogger->start('route:run', 'Run route');
293310
if (isset($parameters['caller'])) {
294311
$caller = $parameters['caller'];
295312
unset($parameters['caller']);
@@ -303,13 +320,15 @@ public function match($url) {
303320
}
304321
unset($parameters['action']);
305322
unset($parameters['caller']);
323+
$this->eventLogger->start('route:run:call', 'Run callable route');
306324
call_user_func($action, $parameters);
325+
$this->eventLogger->end('route:run:call');
307326
} elseif (isset($parameters['file'])) {
308327
include $parameters['file'];
309328
} else {
310329
throw new \Exception('no action available');
311330
}
312-
\OC::$server->getEventLogger()->end('run_route');
331+
$this->eventLogger->end('route:run');
313332
}
314333

315334
/**
@@ -434,7 +453,7 @@ private function getApplicationClass(string $appName) {
434453
$applicationClassName = $appNameSpace . '\\AppInfo\\Application';
435454

436455
if (class_exists($applicationClassName)) {
437-
$application = \OC::$server->query($applicationClassName);
456+
$application = $this->container->get($applicationClassName);
438457
} else {
439458
$application = new App($appName);
440459
}

‎lib/private/Server.php‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -130,6 +130,7 @@
130130
use OC\Remote\Api\ApiFactory;
131131
use OC\Remote\InstanceFactory;
132132
use OC\RichObjectStrings\Validator;
133+
use OC\Route\CachingRouter;
133134
use OC\Route\Router;
134135
use OC\Security\Bruteforce\Throttler;
135136
use OC\Security\CertificateManager;
@@ -820,11 +821,10 @@ public function __construct($webRoot, \OC\Config $config) {
820821

821822
$this->registerService(Router::class, function (Server $c) {
822823
$cacheFactory = $c->get(ICacheFactory::class);
823-
$logger = $c->get(LoggerInterface::class);
824824
if ($cacheFactory->isLocalCacheAvailable()) {
825-
$router = new \OC\Route\CachingRouter($cacheFactory->createLocal('route'), $logger);
825+
$router = $c->resolve(CachingRouter::class);
826826
} else {
827-
$router = new \OC\Route\Router($logger);
827+
$router = $c->resolve(Router::class);
828828
}
829829
return $router;
830830
});

‎tests/lib/AppFramework/Routing/RoutingTest.php‎

Lines changed: 46 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -6,8 +6,12 @@
66
use OC\AppFramework\Routing\RouteConfig;
77
use OC\Route\Route;
88
use OC\Route\Router;
9+
use OCP\Diagnostics\IEventLogger;
10+
use OCP\IConfig;
11+
use OCP\IRequest;
912
use OCP\Route\IRouter;
1013
use PHPUnit\Framework\MockObject\MockObject;
14+
use Psr\Container\ContainerInterface;
1115
use Psr\Log\LoggerInterface;
1216

1317
class RoutingTest extends \Test\TestCase {
@@ -133,7 +137,13 @@ public function testSimpleRouteWithBrokenName() {
133137
/** @var IRouter|MockObject $router */
134138
$router = $this->getMockBuilder(Router::class)
135139
->onlyMethods(['create'])
136-
->setConstructorArgs([$this->createMock(LoggerInterface::class)])
140+
->setConstructorArgs([
141+
$this->createMock(LoggerInterface::class),
142+
$this->createMock(IRequest::class),
143+
$this->createMock(IConfig::class),
144+
$this->createMock(IEventLogger::class),
145+
$this->createMock(ContainerInterface::class)
146+
])
137147
->getMock();
138148

139149
// load route configuration
@@ -154,7 +164,13 @@ public function testSimpleOCSRouteWithBrokenName() {
154164
/** @var IRouter|MockObject $router */
155165
$router = $this->getMockBuilder(Router::class)
156166
->onlyMethods(['create'])
157-
->setConstructorArgs([$this->createMock(LoggerInterface::class)])
167+
->setConstructorArgs([
168+
$this->createMock(LoggerInterface::class),
169+
$this->createMock(IRequest::class),
170+
$this->createMock(IConfig::class),
171+
$this->createMock(IEventLogger::class),
172+
$this->createMock(ContainerInterface::class)
173+
])
158174
->getMock();
159175

160176
// load route configuration
@@ -214,7 +230,13 @@ private function assertSimpleRoute($routes, $name, $verb, $url, $controllerName,
214230
/** @var IRouter|MockObject $router */
215231
$router = $this->getMockBuilder(Router::class)
216232
->onlyMethods(['create'])
217-
->setConstructorArgs([$this->createMock(LoggerInterface::class)])
233+
->setConstructorArgs([
234+
$this->createMock(LoggerInterface::class),
235+
$this->createMock(IRequest::class),
236+
$this->createMock(IConfig::class),
237+
$this->createMock(IEventLogger::class),
238+
$this->createMock(ContainerInterface::class)
239+
])
218240
->getMock();
219241

220242
// we expect create to be called once:
@@ -264,7 +286,13 @@ private function assertSimpleOCSRoute($routes,
264286
/** @var IRouter|MockObject $router */
265287
$router = $this->getMockBuilder(Router::class)
266288
->onlyMethods(['create'])
267-
->setConstructorArgs([$this->createMock(LoggerInterface::class)])
289+
->setConstructorArgs([
290+
$this->createMock(LoggerInterface::class),
291+
$this->createMock(IRequest::class),
292+
$this->createMock(IConfig::class),
293+
$this->createMock(IEventLogger::class),
294+
$this->createMock(ContainerInterface::class)
295+
])
268296
->getMock();
269297

270298
// we expect create to be called once:
@@ -291,7 +319,13 @@ private function assertOCSResource($yaml, $resourceName, $url, $controllerName,
291319
/** @var IRouter|MockObject $router */
292320
$router = $this->getMockBuilder(Router::class)
293321
->onlyMethods(['create'])
294-
->setConstructorArgs([$this->createMock(LoggerInterface::class)])
322+
->setConstructorArgs([
323+
$this->createMock(LoggerInterface::class),
324+
$this->createMock(IRequest::class),
325+
$this->createMock(IConfig::class),
326+
$this->createMock(IEventLogger::class),
327+
$this->createMock(ContainerInterface::class)
328+
])
295329
->getMock();
296330

297331
// route mocks
@@ -338,7 +372,13 @@ private function assertResource($yaml, $resourceName, $url, $controllerName, $pa
338372
/** @var IRouter|MockObject $router */
339373
$router = $this->getMockBuilder(Router::class)
340374
->onlyMethods(['create'])
341-
->setConstructorArgs([$this->createMock(LoggerInterface::class)])
375+
->setConstructorArgs([
376+
$this->createMock(LoggerInterface::class),
377+
$this->createMock(IRequest::class),
378+
$this->createMock(IConfig::class),
379+
$this->createMock(IEventLogger::class),
380+
$this->createMock(ContainerInterface::class)
381+
])
342382
->getMock();
343383

344384
// route mocks

‎tests/lib/Route/RouterTest.php‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,10 @@
2424
namespace Test\Route;
2525

2626
use OC\Route\Router;
27+
use OCP\Diagnostics\IEventLogger;
28+
use OCP\IConfig;
29+
use OCP\IRequest;
30+
use Psr\Container\ContainerInterface;
2731
use Psr\Log\LoggerInterface;
2832
use Test\TestCase;
2933

@@ -44,7 +48,13 @@ function (string $message, array $data) {
4448
$this->fail('Unexpected info log: '.(string)($data['exception'] ?? $message));
4549
}
4650
);
47-
$router = new Router($logger);
51+
$router = new Router(
52+
$logger,
53+
$this->createMock(IRequest::class),
54+
$this->createMock(IConfig::class),
55+
$this->createMock(IEventLogger::class),
56+
$this->createMock(ContainerInterface::class),
57+
);
4858

4959
$this->assertEquals('/index.php/apps/files/', $router->generate('files.view.index'));
5060

0 commit comments

Comments
 (0)