diff options
Diffstat (limited to 'apps/dav/lib/Connector')
| -rw-r--r-- | apps/dav/lib/Connector/Sabre/CommentPropertiesPlugin.php | 27 | ||||
| -rw-r--r-- | apps/dav/lib/Connector/Sabre/PropFindMonitorPlugin.php | 46 | ||||
| -rw-r--r-- | apps/dav/lib/Connector/Sabre/PropFindPreloadNotifyPlugin.php | 55 | ||||
| -rw-r--r-- | apps/dav/lib/Connector/Sabre/Server.php | 42 | ||||
| -rw-r--r-- | apps/dav/lib/Connector/Sabre/ServerFactory.php | 4 | ||||
| -rw-r--r-- | apps/dav/lib/Connector/Sabre/SharesPlugin.php | 62 | ||||
| -rw-r--r-- | apps/dav/lib/Connector/Sabre/TagsPlugin.php | 41 |
7 files changed, 192 insertions, 85 deletions
diff --git a/apps/dav/lib/Connector/Sabre/CommentPropertiesPlugin.php b/apps/dav/lib/Connector/Sabre/CommentPropertiesPlugin.php index e4b6c2636da..ef9bd1ae472 100644 --- a/apps/dav/lib/Connector/Sabre/CommentPropertiesPlugin.php +++ b/apps/dav/lib/Connector/Sabre/CommentPropertiesPlugin.php @@ -10,6 +10,7 @@ namespace OCA\DAV\Connector\Sabre; use OCP\Comments\ICommentsManager; use OCP\IUserSession; +use Sabre\DAV\ICollection; use Sabre\DAV\PropFind; use Sabre\DAV\Server; use Sabre\DAV\ServerPlugin; @@ -21,6 +22,7 @@ class CommentPropertiesPlugin extends ServerPlugin { protected ?Server $server = null; private array $cachedUnreadCount = []; + private array $cachedDirectories = []; public function __construct( private ICommentsManager $commentsManager, @@ -41,6 +43,8 @@ class CommentPropertiesPlugin extends ServerPlugin { */ public function initialize(\Sabre\DAV\Server $server) { $this->server = $server; + + $this->server->on('preloadCollection', $this->preloadCollection(...)); $this->server->on('propFind', [$this, 'handleGetProperties']); } @@ -69,6 +73,21 @@ class CommentPropertiesPlugin extends ServerPlugin { } } + private function preloadCollection(PropFind $propFind, ICollection $collection): + void { + if (!($collection instanceof Directory)) { + return; + } + + $collectionPath = $collection->getPath(); + if (!isset($this->cachedDirectories[$collectionPath]) && $propFind->getStatus( + self::PROPERTY_NAME_UNREAD + ) !== null) { + $this->cacheDirectory($collection); + $this->cachedDirectories[$collectionPath] = true; + } + } + /** * Adds tags and favorites properties to the response, * if requested. @@ -85,14 +104,6 @@ class CommentPropertiesPlugin extends ServerPlugin { return; } - // need prefetch ? - if ($node instanceof Directory - && $propFind->getDepth() !== 0 - && !is_null($propFind->getStatus(self::PROPERTY_NAME_UNREAD)) - ) { - $this->cacheDirectory($node); - } - $propFind->handle(self::PROPERTY_NAME_COUNT, function () use ($node): int { return $this->commentsManager->getNumberOfCommentsForObject('files', (string)$node->getId()); }); 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 @@ +<?php + +declare(strict_types = 1); + +/** + * SPDX-FileCopyrightText: 2025 Nextcloud GmbH and Nextcloud contributors + * SPDX-License-Identifier: AGPL-3.0-or-later + */ +namespace OCA\DAV\Connector\Sabre; + +use Sabre\DAV\ICollection; +use Sabre\DAV\INode; +use Sabre\DAV\PropFind; +use Sabre\DAV\Server; +use Sabre\DAV\ServerPlugin; + +/** + * This plugin asks other plugins to preload data for a collection, so that + * subsequent PROPFIND handlers for children do not query the DB on a per-node + * basis. + */ +class PropFindPreloadNotifyPlugin extends ServerPlugin { + + private Server $server; + + public function initialize(Server $server): void { + $this->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<int, array<string, array{nodes:int, queries:int}>> + * @var array<string, array<int, array<string, array{nodes:int, + * queries:int}>>> 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<int, array<string, array{nodes:int, queries:int}>> + * @return array<string, array<int, array<string, array{nodes:int, + * queries:int}>>> 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..1b4de841ec6 100644 --- a/apps/dav/lib/Connector/Sabre/ServerFactory.php +++ b/apps/dav/lib/Connector/Sabre/ServerFactory.php @@ -14,6 +14,7 @@ use OCA\DAV\CalDAV\DefaultCalendarValidator; use OCA\DAV\CalDAV\Proxy\ProxyMapper; use OCA\DAV\DAV\CustomPropertiesBackend; use OCA\DAV\DAV\ViewOnlyPlugin; +use OCA\DAV\Db\PropertyMapper; use OCA\DAV\Files\BrowserErrorPagePlugin; use OCA\DAV\Files\Sharing\RootCollection; use OCA\DAV\Upload\CleanupService; @@ -94,6 +95,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)); @@ -226,6 +229,7 @@ class ServerFactory { $tree, $this->databaseConnection, $this->userSession->getUser(), + \OCP\Server::get(PropertyMapper::class), \OCP\Server::get(DefaultCalendarValidator::class), ) ) diff --git a/apps/dav/lib/Connector/Sabre/SharesPlugin.php b/apps/dav/lib/Connector/Sabre/SharesPlugin.php index f49e85333f3..11e50362dc2 100644 --- a/apps/dav/lib/Connector/Sabre/SharesPlugin.php +++ b/apps/dav/lib/Connector/Sabre/SharesPlugin.php @@ -15,6 +15,7 @@ use OCP\Files\NotFoundException; use OCP\IUserSession; use OCP\Share\IManager; use OCP\Share\IShare; +use Sabre\DAV\ICollection; use Sabre\DAV\PropFind; use Sabre\DAV\Server; use Sabre\DAV\Tree; @@ -38,7 +39,14 @@ class SharesPlugin extends \Sabre\DAV\ServerPlugin { /** @var IShare[][] */ private array $cachedShares = []; - /** @var string[] */ + + /** + * Tracks which folders have been cached. + * When a folder is cached, it will appear with its path as key and true + * as value. + * + * @var bool[] + */ private array $cachedFolders = []; public function __construct( @@ -67,6 +75,7 @@ class SharesPlugin extends \Sabre\DAV\ServerPlugin { $server->protectedProperties[] = self::SHAREES_PROPERTYNAME; $this->server = $server; + $this->server->on('preloadCollection', $this->preloadCollection(...)); $this->server->on('propFind', [$this, 'handleGetProperties']); } @@ -89,28 +98,28 @@ class SharesPlugin extends \Sabre\DAV\ServerPlugin { ]; foreach ($requestedShareTypes as $requestedShareType) { - $result = array_merge($result, $this->shareManager->getSharesBy( + $result[] = $this->shareManager->getSharesBy( $this->userId, $requestedShareType, $node, false, -1 - )); + ); // Also check for shares where the user is the recipient try { - $result = array_merge($result, $this->shareManager->getSharedWith( + $result[] = $this->shareManager->getSharedWith( $this->userId, $requestedShareType, $node, -1 - )); + ); } catch (BackendError $e) { // ignore } } - return $result; + return array_merge(...$result); } /** @@ -141,7 +150,7 @@ class SharesPlugin extends \Sabre\DAV\ServerPlugin { // if we already cached the folder containing this file // then we already know there are no shares here. - if (array_search($parentPath, $this->cachedFolders) === false) { + if (!isset($this->cachedFolders[$parentPath])) { try { $node = $sabreNode->getNode(); } catch (NotFoundException $e) { @@ -156,6 +165,27 @@ class SharesPlugin extends \Sabre\DAV\ServerPlugin { return []; } + private function preloadCollection(PropFind $propFind, ICollection $collection): void { + if (!$collection instanceof Directory + || isset($this->cachedFolders[$collection->getPath()]) + || ( + $propFind->getStatus(self::SHARETYPES_PROPERTYNAME) === null + && $propFind->getStatus(self::SHAREES_PROPERTYNAME) === null + ) + ) { + return; + } + + // If the node is a directory and we are requesting share types or sharees + // then we get all the shares in the folder and cache them. + // This is more performant than iterating each files afterwards. + $folderNode = $collection->getNode(); + $this->cachedFolders[$collection->getPath()] = true; + foreach ($this->getSharesFolder($folderNode) as $id => $shares) { + $this->cachedShares[$id] = $shares; + } + } + /** * Adds shares to propfind response * @@ -170,24 +200,6 @@ class SharesPlugin extends \Sabre\DAV\ServerPlugin { return; } - // If the node is a directory and we are requesting share types or sharees - // then we get all the shares in the folder and cache them. - // This is more performant than iterating each files afterwards. - if ($sabreNode instanceof Directory - && $propFind->getDepth() !== 0 - && ( - !is_null($propFind->getStatus(self::SHARETYPES_PROPERTYNAME)) - || !is_null($propFind->getStatus(self::SHAREES_PROPERTYNAME)) - ) - ) { - $folderNode = $sabreNode->getNode(); - $this->cachedFolders[] = $sabreNode->getPath(); - $childShares = $this->getSharesFolder($folderNode); - foreach ($childShares as $id => $shares) { - $this->cachedShares[$id] = $shares; - } - } - $propFind->handle(self::SHARETYPES_PROPERTYNAME, function () use ($sabreNode): ShareTypeList { $shares = $this->getShares($sabreNode); diff --git a/apps/dav/lib/Connector/Sabre/TagsPlugin.php b/apps/dav/lib/Connector/Sabre/TagsPlugin.php index 25c1633df36..ec3e6fc5320 100644 --- a/apps/dav/lib/Connector/Sabre/TagsPlugin.php +++ b/apps/dav/lib/Connector/Sabre/TagsPlugin.php @@ -31,6 +31,7 @@ use OCP\EventDispatcher\IEventDispatcher; use OCP\ITagManager; use OCP\ITags; use OCP\IUserSession; +use Sabre\DAV\ICollection; use Sabre\DAV\PropFind; use Sabre\DAV\PropPatch; @@ -61,6 +62,7 @@ class TagsPlugin extends \Sabre\DAV\ServerPlugin { * @var array */ private $cachedTags; + private array $cachedDirectories; /** * @param \Sabre\DAV\Tree $tree tree @@ -92,6 +94,7 @@ class TagsPlugin extends \Sabre\DAV\ServerPlugin { $server->xml->elementMap[self::TAGS_PROPERTYNAME] = TagList::class; $this->server = $server; + $this->server->on('preloadCollection', $this->preloadCollection(...)); $this->server->on('propFind', [$this, 'handleGetProperties']); $this->server->on('propPatch', [$this, 'handleUpdateProperties']); $this->server->on('preloadProperties', [$this, 'handlePreloadProperties']); @@ -194,6 +197,29 @@ class TagsPlugin extends \Sabre\DAV\ServerPlugin { } } + private function preloadCollection(PropFind $propFind, ICollection $collection): + void { + if (!($collection instanceof Node)) { + return; + } + + // need prefetch ? + if ($collection instanceof Directory + && !isset($this->cachedDirectories[$collection->getPath()]) + && (!is_null($propFind->getStatus(self::TAGS_PROPERTYNAME)) + || !is_null($propFind->getStatus(self::FAVORITE_PROPERTYNAME)) + )) { + // note: pre-fetching only supported for depth <= 1 + $folderContent = $collection->getChildren(); + $fileIds = [(int)$collection->getId()]; + foreach ($folderContent as $info) { + $fileIds[] = (int)$info->getId(); + } + $this->prefetchTagsForFileIds($fileIds); + $this->cachedDirectories[$collection->getPath()] = true; + } + } + /** * Adds tags and favorites properties to the response, * if requested. @@ -210,21 +236,6 @@ class TagsPlugin extends \Sabre\DAV\ServerPlugin { return; } - // need prefetch ? - if ($node instanceof Directory - && $propFind->getDepth() !== 0 - && (!is_null($propFind->getStatus(self::TAGS_PROPERTYNAME)) - || !is_null($propFind->getStatus(self::FAVORITE_PROPERTYNAME)) - )) { - // note: pre-fetching only supported for depth <= 1 - $folderContent = $node->getChildren(); - $fileIds = [(int)$node->getId()]; - foreach ($folderContent as $info) { - $fileIds[] = (int)$info->getId(); - } - $this->prefetchTagsForFileIds($fileIds); - } - $isFav = null; $propFind->handle(self::TAGS_PROPERTYNAME, function () use (&$isFav, $node) { |