diff --git a/apps/dav/composer/composer/autoload_classmap.php b/apps/dav/composer/composer/autoload_classmap.php index b9708ea5589..b0c4385ae04 100644 --- a/apps/dav/composer/composer/autoload_classmap.php +++ b/apps/dav/composer/composer/autoload_classmap.php @@ -217,6 +217,7 @@ return array( 'OCA\\DAV\\Connector\\Sabre\\ObjectTree' => $baseDir . '/../lib/Connector/Sabre/ObjectTree.php', 'OCA\\DAV\\Connector\\Sabre\\Principal' => $baseDir . '/../lib/Connector/Sabre/Principal.php', 'OCA\\DAV\\Connector\\Sabre\\PropFindMonitorPlugin' => $baseDir . '/../lib/Connector/Sabre/PropFindMonitorPlugin.php', + 'OCA\\DAV\\Connector\\Sabre\\PropFindPreloadNotifyPlugin' => $baseDir . '/../lib/Connector/Sabre/PropFindPreloadNotifyPlugin.php', 'OCA\\DAV\\Connector\\Sabre\\PropfindCompressionPlugin' => $baseDir . '/../lib/Connector/Sabre/PropfindCompressionPlugin.php', 'OCA\\DAV\\Connector\\Sabre\\PublicAuth' => $baseDir . '/../lib/Connector/Sabre/PublicAuth.php', 'OCA\\DAV\\Connector\\Sabre\\QuotaPlugin' => $baseDir . '/../lib/Connector/Sabre/QuotaPlugin.php', diff --git a/apps/dav/composer/composer/autoload_static.php b/apps/dav/composer/composer/autoload_static.php index 75ac3350160..26efddd3df5 100644 --- a/apps/dav/composer/composer/autoload_static.php +++ b/apps/dav/composer/composer/autoload_static.php @@ -232,6 +232,7 @@ class ComposerStaticInitDAV 'OCA\\DAV\\Connector\\Sabre\\ObjectTree' => __DIR__ . '/..' . '/../lib/Connector/Sabre/ObjectTree.php', 'OCA\\DAV\\Connector\\Sabre\\Principal' => __DIR__ . '/..' . '/../lib/Connector/Sabre/Principal.php', 'OCA\\DAV\\Connector\\Sabre\\PropFindMonitorPlugin' => __DIR__ . '/..' . '/../lib/Connector/Sabre/PropFindMonitorPlugin.php', + 'OCA\\DAV\\Connector\\Sabre\\PropFindPreloadNotifyPlugin' => __DIR__ . '/..' . '/../lib/Connector/Sabre/PropFindPreloadNotifyPlugin.php', 'OCA\\DAV\\Connector\\Sabre\\PropfindCompressionPlugin' => __DIR__ . '/..' . '/../lib/Connector/Sabre/PropfindCompressionPlugin.php', 'OCA\\DAV\\Connector\\Sabre\\PublicAuth' => __DIR__ . '/..' . '/../lib/Connector/Sabre/PublicAuth.php', 'OCA\\DAV\\Connector\\Sabre\\QuotaPlugin' => __DIR__ . '/..' . '/../lib/Connector/Sabre/QuotaPlugin.php', diff --git a/apps/dav/lib/CalDAV/EmbeddedCalDavServer.php b/apps/dav/lib/CalDAV/EmbeddedCalDavServer.php index 21d8c06fa99..d9d6d840c5e 100644 --- a/apps/dav/lib/CalDAV/EmbeddedCalDavServer.php +++ b/apps/dav/lib/CalDAV/EmbeddedCalDavServer.php @@ -20,6 +20,7 @@ use OCA\DAV\Connector\Sabre\DavAclPlugin; use OCA\DAV\Connector\Sabre\ExceptionLoggerPlugin; use OCA\DAV\Connector\Sabre\LockPlugin; use OCA\DAV\Connector\Sabre\MaintenancePlugin; +use OCA\DAV\Connector\Sabre\PropFindPreloadNotifyPlugin; use OCA\DAV\Events\SabrePluginAuthInitEvent; use OCA\DAV\RootCollection; use OCA\Theming\ThemingDefaults; @@ -96,6 +97,9 @@ class EmbeddedCalDavServer { $this->server->addPlugin(Server::get(\OCA\DAV\CalDAV\Schedule\IMipPlugin::class)); } + // collection preload plugin + $this->server->addPlugin(new PropFindPreloadNotifyPlugin()); + // wait with registering these until auth is handled and the filesystem is setup $this->server->on('beforeMethod:*', function () use ($root): void { // register plugins from apps diff --git a/apps/dav/lib/Connector/Sabre/PropFindMonitorPlugin.php b/apps/dav/lib/Connector/Sabre/PropFindMonitorPlugin.php index 130d4562146..38538fdcff0 100644 --- a/apps/dav/lib/Connector/Sabre/PropFindMonitorPlugin.php +++ b/apps/dav/lib/Connector/Sabre/PropFindMonitorPlugin.php @@ -48,30 +48,34 @@ class PropFindMonitorPlugin extends ServerPlugin { if (empty($pluginQueries)) { return; } - $maxDepth = max(0, ...array_keys($pluginQueries)); - // entries at the top are usually not interesting - unset($pluginQueries[$maxDepth]); $logger = $this->server->getLogger(); - foreach ($pluginQueries as $depth => $propFinds) { - foreach ($propFinds as $pluginName => $propFind) { - [ - 'queries' => $queries, - 'nodes' => $nodes - ] = $propFind; - if ($queries === 0 || $nodes > $queries || $nodes < self::THRESHOLD_NODES - || $queries < $nodes * self::THRESHOLD_QUERY_FACTOR) { - continue; + foreach ($pluginQueries as $eventName => $eventQueries) { + $maxDepth = max(0, ...array_keys($eventQueries)); + // entries at the top are usually not interesting + unset($eventQueries[$maxDepth]); + foreach ($eventQueries as $depth => $propFinds) { + foreach ($propFinds as $pluginName => $propFind) { + [ + 'queries' => $queries, + 'nodes' => $nodes + ] = $propFind; + if ($queries === 0 || $nodes > $queries || $nodes < self::THRESHOLD_NODES + || $queries < $nodes * self::THRESHOLD_QUERY_FACTOR) { + continue; + } + $logger->error( + '{name}:{event} scanned {scans} nodes with {count} queries in depth {depth}/{maxDepth}. This is bad for performance, please report to the plugin developer!', + [ + 'name' => $pluginName, + 'scans' => $nodes, + 'count' => $queries, + 'depth' => $depth, + 'maxDepth' => $maxDepth, + 'event' => $eventName, + ] + ); } - $logger->error( - '{name} scanned {scans} nodes with {count} queries in depth {depth}/{maxDepth}. This is bad for performance, please report to the plugin developer!', [ - 'name' => $pluginName, - 'scans' => $nodes, - 'count' => $queries, - 'depth' => $depth, - 'maxDepth' => $maxDepth, - ] - ); } } } diff --git a/apps/dav/lib/Connector/Sabre/PropFindPreloadNotifyPlugin.php b/apps/dav/lib/Connector/Sabre/PropFindPreloadNotifyPlugin.php new file mode 100644 index 00000000000..c7b0c64132c --- /dev/null +++ b/apps/dav/lib/Connector/Sabre/PropFindPreloadNotifyPlugin.php @@ -0,0 +1,55 @@ +server = $server; + $this->server->on('propFind', [$this, 'collectionPreloadNotifier' ], 1); + } + + /** + * Uses the server instance to emit a `preloadCollection` event to signal + * to interested plugins that a collection can be preloaded. + * + * NOTE: this can be emitted several times, so ideally every plugin + * should cache what they need and check if a cache exists before + * re-fetching. + */ + public function collectionPreloadNotifier(PropFind $propFind, INode $node): bool { + if (!$this->shouldPreload($propFind, $node)) { + return true; + } + + return $this->server->emit('preloadCollection', [$propFind, $node]); + } + + private function shouldPreload( + PropFind $propFind, + INode $node, + ): bool { + $depth = $propFind->getDepth(); + return $node instanceof ICollection + && ($depth === Server::DEPTH_INFINITY || $depth > 0); + } +} diff --git a/apps/dav/lib/Connector/Sabre/Server.php b/apps/dav/lib/Connector/Sabre/Server.php index dda9c29b763..eef65000131 100644 --- a/apps/dav/lib/Connector/Sabre/Server.php +++ b/apps/dav/lib/Connector/Sabre/Server.php @@ -27,7 +27,8 @@ class Server extends \Sabre\DAV\Server { /** * Tracks queries done by plugins. - * @var array> + * @var array>> The keys represent: event name, depth and plugin name */ private array $pluginQueries = []; @@ -50,8 +51,8 @@ class Server extends \Sabre\DAV\Server { ): void { $this->debugEnabled ? $this->monitorPropfindQueries( parent::once(...), - ...func_get_args(), - ) : parent::once(...func_get_args()); + ...\func_get_args(), + ) : parent::once(...\func_get_args()); } #[Override] @@ -62,8 +63,8 @@ class Server extends \Sabre\DAV\Server { ): void { $this->debugEnabled ? $this->monitorPropfindQueries( parent::on(...), - ...func_get_args(), - ) : parent::on(...func_get_args()); + ...\func_get_args(), + ) : parent::on(...\func_get_args()); } /** @@ -76,13 +77,17 @@ class Server extends \Sabre\DAV\Server { callable $callBack, int $priority = 100, ): void { - if ($eventName !== 'propFind') { + $pluginName = debug_backtrace(DEBUG_BACKTRACE_IGNORE_ARGS, 3)[2]['class'] ?? 'unknown'; + // The NotifyPlugin needs to be excluded as it emits the + // `preloadCollection` event, which causes many plugins run queries. + /** @psalm-suppress TypeDoesNotContainType */ + if ($pluginName === PropFindPreloadNotifyPlugin::class || ($eventName !== 'propFind' + && $eventName !== 'preloadCollection')) { $parentFn($eventName, $callBack, $priority); return; } - $pluginName = debug_backtrace(DEBUG_BACKTRACE_IGNORE_ARGS, 3)[2]['class'] ?? 'unknown'; - $callback = $this->getMonitoredCallback($callBack, $pluginName); + $callback = $this->getMonitoredCallback($callBack, $pluginName, $eventName); $parentFn($eventName, $callback, $priority); } @@ -94,22 +99,26 @@ class Server extends \Sabre\DAV\Server { private function getMonitoredCallback( callable $callBack, string $pluginName, + string $eventName, ): callable { return function (PropFind $propFind, INode $node) use ( $callBack, $pluginName, - ) { + $eventName, + ): bool { $connection = \OCP\Server::get(Connection::class); $queriesBefore = $connection->getStats()['executed']; $result = $callBack($propFind, $node); $queriesAfter = $connection->getStats()['executed']; $this->trackPluginQueries( $pluginName, + $eventName, $queriesAfter - $queriesBefore, $propFind->getDepth() ); - return $result; + // many callbacks don't care about returning a bool + return $result ?? true; }; } @@ -118,6 +127,7 @@ class Server extends \Sabre\DAV\Server { */ private function trackPluginQueries( string $pluginName, + string $eventName, int $queriesExecuted, int $depth, ): void { @@ -126,11 +136,11 @@ class Server extends \Sabre\DAV\Server { return; } - $this->pluginQueries[$depth][$pluginName]['nodes'] - = ($this->pluginQueries[$depth][$pluginName]['nodes'] ?? 0) + 1; + $this->pluginQueries[$eventName][$depth][$pluginName]['nodes'] + = ($this->pluginQueries[$eventName][$depth][$pluginName]['nodes'] ?? 0) + 1; - $this->pluginQueries[$depth][$pluginName]['queries'] - = ($this->pluginQueries[$depth][$pluginName]['queries'] ?? 0) + $queriesExecuted; + $this->pluginQueries[$eventName][$depth][$pluginName]['queries'] + = ($this->pluginQueries[$eventName][$depth][$pluginName]['queries'] ?? 0) + $queriesExecuted; } /** @@ -221,8 +231,8 @@ class Server extends \Sabre\DAV\Server { /** * Returns queries executed by registered plugins. - * - * @return array> + * @return array>> The keys represent: event name, depth and plugin name */ public function getPluginQueries(): array { return $this->pluginQueries; diff --git a/apps/dav/lib/Connector/Sabre/ServerFactory.php b/apps/dav/lib/Connector/Sabre/ServerFactory.php index a6a27057177..a8d80dd8429 100644 --- a/apps/dav/lib/Connector/Sabre/ServerFactory.php +++ b/apps/dav/lib/Connector/Sabre/ServerFactory.php @@ -94,6 +94,8 @@ class ServerFactory { $server->debugEnabled = $debugEnabled; $server->addPlugin(new PropFindMonitorPlugin()); } + + $server->addPlugin(new PropFindPreloadNotifyPlugin()); // FIXME: The following line is a workaround for legacy components relying on being able to send a GET to / $server->addPlugin(new DummyGetResponsePlugin()); $server->addPlugin(new ExceptionLoggerPlugin('webdav', $this->logger)); diff --git a/apps/dav/lib/Server.php b/apps/dav/lib/Server.php index a92e162f1b0..ec3294d94ce 100644 --- a/apps/dav/lib/Server.php +++ b/apps/dav/lib/Server.php @@ -46,6 +46,7 @@ use OCA\DAV\Connector\Sabre\LockPlugin; use OCA\DAV\Connector\Sabre\MaintenancePlugin; use OCA\DAV\Connector\Sabre\PropfindCompressionPlugin; use OCA\DAV\Connector\Sabre\PropFindMonitorPlugin; +use OCA\DAV\Connector\Sabre\PropFindPreloadNotifyPlugin; use OCA\DAV\Connector\Sabre\QuotaPlugin; use OCA\DAV\Connector\Sabre\RequestIdHeaderPlugin; use OCA\DAV\Connector\Sabre\SharesPlugin; @@ -237,6 +238,7 @@ class Server { \OCP\Server::get(IUserSession::class) )); + // performance improvement plugins $this->server->addPlugin(new CopyEtagHeaderPlugin()); $this->server->addPlugin(new RequestIdHeaderPlugin(\OCP\Server::get(IRequest::class))); $this->server->addPlugin(new UploadAutoMkcolPlugin()); @@ -248,6 +250,7 @@ class Server { $eventDispatcher, )); $this->server->addPlugin(\OCP\Server::get(PaginatePlugin::class)); + $this->server->addPlugin(new PropFindPreloadNotifyPlugin()); // allow setup of additional plugins $eventDispatcher->dispatch('OCA\DAV\Connector\Sabre::addPlugin', $event); diff --git a/apps/dav/tests/unit/Connector/Sabre/PropFindMonitorPluginTest.php b/apps/dav/tests/unit/Connector/Sabre/PropFindMonitorPluginTest.php index b528c3d731c..9d22befa201 100644 --- a/apps/dav/tests/unit/Connector/Sabre/PropFindMonitorPluginTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/PropFindMonitorPluginTest.php @@ -29,66 +29,76 @@ class PropFindMonitorPluginTest extends TestCase { 'No queries logged' => [[], 0], 'Plugins with queries in less than threshold nodes should not be logged' => [ [ - [ - 'PluginName' => ['queries' => 100, 'nodes' - => PropFindMonitorPlugin::THRESHOLD_NODES - 1] - ], - [], + 'propFind' => [ + [ + 'PluginName' => [ + 'queries' => 100, + 'nodes' => PropFindMonitorPlugin::THRESHOLD_NODES - 1] + ], + [], + ] ], 0 ], 'Plugins with query-to-node ratio less than threshold should not be logged' => [ [ - [ - 'PluginName' => [ - 'queries' => $minQueriesTrigger - 1, - 'nodes' => PropFindMonitorPlugin::THRESHOLD_NODES ], - ], - [], + 'propFind' => [ + [ + 'PluginName' => [ + 'queries' => $minQueriesTrigger - 1, + 'nodes' => PropFindMonitorPlugin::THRESHOLD_NODES ], + ], + [], + ] ], 0 ], 'Plugins with more nodes scanned than queries executed should not be logged' => [ [ - [ - 'PluginName' => [ - 'queries' => $minQueriesTrigger, - 'nodes' => PropFindMonitorPlugin::THRESHOLD_NODES * 2], - ], - [], + 'propFind' => [ + [ + 'PluginName' => [ + 'queries' => $minQueriesTrigger, + 'nodes' => PropFindMonitorPlugin::THRESHOLD_NODES * 2], + ], + [],] ], 0 ], 'Plugins with queries only in highest depth level should not be logged' => [ [ - [ - 'PluginName' => [ - 'queries' => $minQueriesTrigger, - 'nodes' => PropFindMonitorPlugin::THRESHOLD_NODES - 1 - ] - ], - [ - 'PluginName' => [ - 'queries' => $minQueriesTrigger * 2, - 'nodes' => PropFindMonitorPlugin::THRESHOLD_NODES - ] + 'propFind' => [ + [ + 'PluginName' => [ + 'queries' => $minQueriesTrigger, + 'nodes' => PropFindMonitorPlugin::THRESHOLD_NODES - 1 + ] + ], + [ + 'PluginName' => [ + 'queries' => $minQueriesTrigger * 2, + 'nodes' => PropFindMonitorPlugin::THRESHOLD_NODES + ] + ], ] ], 0 ], 'Plugins with too many queries should be logged' => [ [ - [ - 'FirstPlugin' => [ - 'queries' => $minQueriesTrigger, - 'nodes' => PropFindMonitorPlugin::THRESHOLD_NODES, + 'propFind' => [ + [ + 'FirstPlugin' => [ + 'queries' => $minQueriesTrigger, + 'nodes' => PropFindMonitorPlugin::THRESHOLD_NODES, + ], + 'SecondPlugin' => [ + 'queries' => $minQueriesTrigger, + 'nodes' => PropFindMonitorPlugin::THRESHOLD_NODES, + ] ], - 'SecondPlugin' => [ - 'queries' => $minQueriesTrigger, - 'nodes' => PropFindMonitorPlugin::THRESHOLD_NODES, - ] - ], - [] + [], + ] ], 2 ] diff --git a/apps/dav/tests/unit/Connector/Sabre/PropFindPreloadNotifyPluginTest.php b/apps/dav/tests/unit/Connector/Sabre/PropFindPreloadNotifyPluginTest.php new file mode 100644 index 00000000000..52fe3eba5bf --- /dev/null +++ b/apps/dav/tests/unit/Connector/Sabre/PropFindPreloadNotifyPluginTest.php @@ -0,0 +1,92 @@ +server = $this->createMock(Server::class); + $this->plugin = new PropFindPreloadNotifyPlugin(); + } + + public function testInitialize(): void { + $this->server + ->expects(self::once()) + ->method('on') + ->with('propFind', + $this->anything(), 1); + $this->plugin->initialize($this->server); + } + + public static function dataTestCollectionPreloadNotifier(): array { + return [ + 'When node is not a collection, should not emit' => [ + IFile::class, + 1, + false, + true + ], + 'When node is a collection but depth is zero, should not emit' => [ + ICollection::class, + 0, + false, + true + ], + 'When node is a collection, and depth > 0, should emit' => [ + ICollection::class, + 1, + true, + true + ], + 'When node is a collection, and depth is infinite, should emit' + => [ + ICollection::class, + Server::DEPTH_INFINITY, + true, + true + ], + 'When called called handler returns false, it should be returned' + => [ + ICollection::class, + 1, + true, + false + ] + ]; + } + + #[DataProvider(methodName: 'dataTestCollectionPreloadNotifier')] + public function testCollectionPreloadNotifier(string $nodeType, int $depth, bool $shouldEmit, bool $emitReturns): + void { + $this->plugin->initialize($this->server); + $propFind = $this->createMock(PropFind::class); + $propFind->expects(self::any())->method('getDepth')->willReturn($depth); + $node = $this->createMock($nodeType); + + $expectation = $shouldEmit ? self::once() : self::never(); + $this->server->expects($expectation)->method('emit')->with('preloadCollection', + [$propFind, $node])->willReturn($emitReturns); + $return = $this->plugin->collectionPreloadNotifier($propFind, $node); + $this->assertEquals($emitReturns, $return); + } +}