Skip to content

Commit 75d9aaa

Browse files
salmart-devsalmart-dev
authored
Merge pull request #54318 from nextcloud/feat/54115/emitPreloadCollectionEvent
Emits a `preloadCollection` event in the DAV server, so that plugins can listen to it and preload DAV properties for files inside a collection, to avoid the N+1 issue that would follow if loading properties on a per-file basis.
2 parents 89fa14f + 4a0a00a commit 75d9aaa

18 files changed

+412
-169
lines changed

‎apps/dav/composer/composer/autoload_classmap.php‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -217,6 +217,7 @@
217217
'OCA\\DAV\\Connector\\Sabre\\ObjectTree' => $baseDir . '/../lib/Connector/Sabre/ObjectTree.php',
218218
'OCA\\DAV\\Connector\\Sabre\\Principal' => $baseDir . '/../lib/Connector/Sabre/Principal.php',
219219
'OCA\\DAV\\Connector\\Sabre\\PropFindMonitorPlugin' => $baseDir . '/../lib/Connector/Sabre/PropFindMonitorPlugin.php',
220+
'OCA\\DAV\\Connector\\Sabre\\PropFindPreloadNotifyPlugin' => $baseDir . '/../lib/Connector/Sabre/PropFindPreloadNotifyPlugin.php',
220221
'OCA\\DAV\\Connector\\Sabre\\PropfindCompressionPlugin' => $baseDir . '/../lib/Connector/Sabre/PropfindCompressionPlugin.php',
221222
'OCA\\DAV\\Connector\\Sabre\\PublicAuth' => $baseDir . '/../lib/Connector/Sabre/PublicAuth.php',
222223
'OCA\\DAV\\Connector\\Sabre\\QuotaPlugin' => $baseDir . '/../lib/Connector/Sabre/QuotaPlugin.php',

‎apps/dav/composer/composer/autoload_static.php‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -232,6 +232,7 @@ class ComposerStaticInitDAV
232232
'OCA\\DAV\\Connector\\Sabre\\ObjectTree' => __DIR__ . '/..' . '/../lib/Connector/Sabre/ObjectTree.php',
233233
'OCA\\DAV\\Connector\\Sabre\\Principal' => __DIR__ . '/..' . '/../lib/Connector/Sabre/Principal.php',
234234
'OCA\\DAV\\Connector\\Sabre\\PropFindMonitorPlugin' => __DIR__ . '/..' . '/../lib/Connector/Sabre/PropFindMonitorPlugin.php',
235+
'OCA\\DAV\\Connector\\Sabre\\PropFindPreloadNotifyPlugin' => __DIR__ . '/..' . '/../lib/Connector/Sabre/PropFindPreloadNotifyPlugin.php',
235236
'OCA\\DAV\\Connector\\Sabre\\PropfindCompressionPlugin' => __DIR__ . '/..' . '/../lib/Connector/Sabre/PropfindCompressionPlugin.php',
236237
'OCA\\DAV\\Connector\\Sabre\\PublicAuth' => __DIR__ . '/..' . '/../lib/Connector/Sabre/PublicAuth.php',
237238
'OCA\\DAV\\Connector\\Sabre\\QuotaPlugin' => __DIR__ . '/..' . '/../lib/Connector/Sabre/QuotaPlugin.php',

‎apps/dav/lib/CalDAV/EmbeddedCalDavServer.php‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
use OCA\DAV\Connector\Sabre\ExceptionLoggerPlugin;
2121
use OCA\DAV\Connector\Sabre\LockPlugin;
2222
use OCA\DAV\Connector\Sabre\MaintenancePlugin;
23+
use OCA\DAV\Connector\Sabre\PropFindPreloadNotifyPlugin;
2324
use OCA\DAV\Events\SabrePluginAuthInitEvent;
2425
use OCA\DAV\RootCollection;
2526
use OCA\Theming\ThemingDefaults;
@@ -96,6 +97,9 @@ public function __construct(bool $public = true) {
9697
$this->server->addPlugin(Server::get(\OCA\DAV\CalDAV\Schedule\IMipPlugin::class));
9798
}
9899

100+
// collection preload plugin
101+
$this->server->addPlugin(new PropFindPreloadNotifyPlugin());
102+
99103
// wait with registering these until auth is handled and the filesystem is setup
100104
$this->server->on('beforeMethod:*', function () use ($root): void {
101105
// register plugins from apps

‎apps/dav/lib/Connector/Sabre/CommentPropertiesPlugin.php‎

Lines changed: 19 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010

1111
use OCP\Comments\ICommentsManager;
1212
use OCP\IUserSession;
13+
use Sabre\DAV\ICollection;
1314
use Sabre\DAV\PropFind;
1415
use Sabre\DAV\Server;
1516
use Sabre\DAV\ServerPlugin;
@@ -21,6 +22,7 @@ class CommentPropertiesPlugin extends ServerPlugin {
2122

2223
protected ?Server $server = null;
2324
private array $cachedUnreadCount = [];
25+
private array $cachedDirectories = [];
2426

2527
public function __construct(
2628
private ICommentsManager $commentsManager,
@@ -41,6 +43,8 @@ public function __construct(
4143
*/
4244
public function initialize(\Sabre\DAV\Server $server) {
4345
$this->server = $server;
46+
47+
$this->server->on('preloadCollection', $this->preloadCollection(...));
4448
$this->server->on('propFind', [$this, 'handleGetProperties']);
4549
}
4650

@@ -69,6 +73,21 @@ private function cacheDirectory(Directory $directory): void {
6973
}
7074
}
7175

76+
private function preloadCollection(PropFind $propFind, ICollection $collection):
77+
void {
78+
if (!($collection instanceof Directory)) {
79+
return;
80+
}
81+
82+
$collectionPath = $collection->getPath();
83+
if (!isset($this->cachedDirectories[$collectionPath]) && $propFind->getStatus(
84+
self::PROPERTY_NAME_UNREAD
85+
) !== null) {
86+
$this->cacheDirectory($collection);
87+
$this->cachedDirectories[$collectionPath] = true;
88+
}
89+
}
90+
7291
/**
7392
* Adds tags and favorites properties to the response,
7493
* if requested.
@@ -85,14 +104,6 @@ public function handleGetProperties(
85104
return;
86105
}
87106

88-
// need prefetch ?
89-
if ($node instanceof Directory
90-
&& $propFind->getDepth() !== 0
91-
&& !is_null($propFind->getStatus(self::PROPERTY_NAME_UNREAD))
92-
) {
93-
$this->cacheDirectory($node);
94-
}
95-
96107
$propFind->handle(self::PROPERTY_NAME_COUNT, function () use ($node): int {
97108
return $this->commentsManager->getNumberOfCommentsForObject('files', (string)$node->getId());
98109
});

‎apps/dav/lib/Connector/Sabre/PropFindMonitorPlugin.php‎

Lines changed: 25 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -48,30 +48,34 @@ public function afterResponse(
4848
if (empty($pluginQueries)) {
4949
return;
5050
}
51-
$maxDepth = max(0, ...array_keys($pluginQueries));
52-
// entries at the top are usually not interesting
53-
unset($pluginQueries[$maxDepth]);
5451

5552
$logger = $this->server->getLogger();
56-
foreach ($pluginQueries as $depth => $propFinds) {
57-
foreach ($propFinds as $pluginName => $propFind) {
58-
[
59-
'queries' => $queries,
60-
'nodes' => $nodes
61-
] = $propFind;
62-
if ($queries === 0 || $nodes > $queries || $nodes < self::THRESHOLD_NODES
63-
|| $queries < $nodes * self::THRESHOLD_QUERY_FACTOR) {
64-
continue;
53+
foreach ($pluginQueries as $eventName => $eventQueries) {
54+
$maxDepth = max(0, ...array_keys($eventQueries));
55+
// entries at the top are usually not interesting
56+
unset($eventQueries[$maxDepth]);
57+
foreach ($eventQueries as $depth => $propFinds) {
58+
foreach ($propFinds as $pluginName => $propFind) {
59+
[
60+
'queries' => $queries,
61+
'nodes' => $nodes
62+
] = $propFind;
63+
if ($queries === 0 || $nodes > $queries || $nodes < self::THRESHOLD_NODES
64+
|| $queries < $nodes * self::THRESHOLD_QUERY_FACTOR) {
65+
continue;
66+
}
67+
$logger->error(
68+
'{name}:{event} scanned {scans} nodes with {count} queries in depth {depth}/{maxDepth}. This is bad for performance, please report to the plugin developer!',
69+
[
70+
'name' => $pluginName,
71+
'scans' => $nodes,
72+
'count' => $queries,
73+
'depth' => $depth,
74+
'maxDepth' => $maxDepth,
75+
'event' => $eventName,
76+
]
77+
);
6578
}
66-
$logger->error(
67-
'{name} scanned {scans} nodes with {count} queries in depth {depth}/{maxDepth}. This is bad for performance, please report to the plugin developer!', [
68-
'name' => $pluginName,
69-
'scans' => $nodes,
70-
'count' => $queries,
71-
'depth' => $depth,
72-
'maxDepth' => $maxDepth,
73-
]
74-
);
7579
}
7680
}
7781
}

‎apps/dav/lib/Connector/Sabre/PropFindPreloadNotifyPlugin.php‎

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,55 @@
1+
<?php
2+
3+
declare(strict_types = 1);
4+
5+
/**
6+
* SPDX-FileCopyrightText: 2025 Nextcloud GmbH and Nextcloud contributors
7+
* SPDX-License-Identifier: AGPL-3.0-or-later
8+
*/
9+
namespace OCA\DAV\Connector\Sabre;
10+
11+
use Sabre\DAV\ICollection;
12+
use Sabre\DAV\INode;
13+
use Sabre\DAV\PropFind;
14+
use Sabre\DAV\Server;
15+
use Sabre\DAV\ServerPlugin;
16+
17+
/**
18+
* This plugin asks other plugins to preload data for a collection, so that
19+
* subsequent PROPFIND handlers for children do not query the DB on a per-node
20+
* basis.
21+
*/
22+
class PropFindPreloadNotifyPlugin extends ServerPlugin {
23+
24+
private Server $server;
25+
26+
public function initialize(Server $server): void {
27+
$this->server = $server;
28+
$this->server->on('propFind', [$this, 'collectionPreloadNotifier' ], 1);
29+
}
30+
31+
/**
32+
* Uses the server instance to emit a `preloadCollection` event to signal
33+
* to interested plugins that a collection can be preloaded.
34+
*
35+
* NOTE: this can be emitted several times, so ideally every plugin
36+
* should cache what they need and check if a cache exists before
37+
* re-fetching.
38+
*/
39+
public function collectionPreloadNotifier(PropFind $propFind, INode $node): bool {
40+
if (!$this->shouldPreload($propFind, $node)) {
41+
return true;
42+
}
43+
44+
return $this->server->emit('preloadCollection', [$propFind, $node]);
45+
}
46+
47+
private function shouldPreload(
48+
PropFind $propFind,
49+
INode $node,
50+
): bool {
51+
$depth = $propFind->getDepth();
52+
return $node instanceof ICollection
53+
&& ($depth === Server::DEPTH_INFINITY || $depth > 0);
54+
}
55+
}

‎apps/dav/lib/Connector/Sabre/Server.php‎

Lines changed: 26 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,8 @@ class Server extends \Sabre\DAV\Server {
2727

2828
/**
2929
* Tracks queries done by plugins.
30-
* @var array<int, array<string, array{nodes:int, queries:int}>>
30+
* @var array<string, array<int, array<string, array{nodes:int,
31+
* queries:int}>>> The keys represent: event name, depth and plugin name
3132
*/
3233
private array $pluginQueries = [];
3334

@@ -50,8 +51,8 @@ public function once(
5051
): void {
5152
$this->debugEnabled ? $this->monitorPropfindQueries(
5253
parent::once(...),
53-
...func_get_args(),
54-
) : parent::once(...func_get_args());
54+
...\func_get_args(),
55+
) : parent::once(...\func_get_args());
5556
}
5657

5758
#[Override]
@@ -62,8 +63,8 @@ public function on(
6263
): void {
6364
$this->debugEnabled ? $this->monitorPropfindQueries(
6465
parent::on(...),
65-
...func_get_args(),
66-
) : parent::on(...func_get_args());
66+
...\func_get_args(),
67+
) : parent::on(...\func_get_args());
6768
}
6869

6970
/**
@@ -76,13 +77,17 @@ private function monitorPropfindQueries(
7677
callable $callBack,
7778
int $priority = 100,
7879
): void {
79-
if ($eventName !== 'propFind') {
80+
$pluginName = debug_backtrace(DEBUG_BACKTRACE_IGNORE_ARGS, 3)[2]['class'] ?? 'unknown';
81+
// The NotifyPlugin needs to be excluded as it emits the
82+
// `preloadCollection` event, which causes many plugins run queries.
83+
/** @psalm-suppress TypeDoesNotContainType */
84+
if ($pluginName === PropFindPreloadNotifyPlugin::class || ($eventName !== 'propFind'
85+
&& $eventName !== 'preloadCollection')) {
8086
$parentFn($eventName, $callBack, $priority);
8187
return;
8288
}
8389

84-
$pluginName = debug_backtrace(DEBUG_BACKTRACE_IGNORE_ARGS, 3)[2]['class'] ?? 'unknown';
85-
$callback = $this->getMonitoredCallback($callBack, $pluginName);
90+
$callback = $this->getMonitoredCallback($callBack, $pluginName, $eventName);
8691

8792
$parentFn($eventName, $callback, $priority);
8893
}
@@ -94,22 +99,26 @@ private function monitorPropfindQueries(
9499
private function getMonitoredCallback(
95100
callable $callBack,
96101
string $pluginName,
102+
string $eventName,
97103
): callable {
98104
return function (PropFind $propFind, INode $node) use (
99105
$callBack,
100106
$pluginName,
101-
) {
107+
$eventName,
108+
): bool {
102109
$connection = \OCP\Server::get(Connection::class);
103110
$queriesBefore = $connection->getStats()['executed'];
104111
$result = $callBack($propFind, $node);
105112
$queriesAfter = $connection->getStats()['executed'];
106113
$this->trackPluginQueries(
107114
$pluginName,
115+
$eventName,
108116
$queriesAfter - $queriesBefore,
109117
$propFind->getDepth()
110118
);
111119

112-
return $result;
120+
// many callbacks don't care about returning a bool
121+
return $result ?? true;
113122
};
114123
}
115124

@@ -118,6 +127,7 @@ private function getMonitoredCallback(
118127
*/
119128
private function trackPluginQueries(
120129
string $pluginName,
130+
string $eventName,
121131
int $queriesExecuted,
122132
int $depth,
123133
): void {
@@ -126,11 +136,11 @@ private function trackPluginQueries(
126136
return;
127137
}
128138

129-
$this->pluginQueries[$depth][$pluginName]['nodes']
130-
= ($this->pluginQueries[$depth][$pluginName]['nodes'] ?? 0) + 1;
139+
$this->pluginQueries[$eventName][$depth][$pluginName]['nodes']
140+
= ($this->pluginQueries[$eventName][$depth][$pluginName]['nodes'] ?? 0) + 1;
131141

132-
$this->pluginQueries[$depth][$pluginName]['queries']
133-
= ($this->pluginQueries[$depth][$pluginName]['queries'] ?? 0) + $queriesExecuted;
142+
$this->pluginQueries[$eventName][$depth][$pluginName]['queries']
143+
= ($this->pluginQueries[$eventName][$depth][$pluginName]['queries'] ?? 0) + $queriesExecuted;
134144
}
135145

136146
/**
@@ -221,8 +231,8 @@ public function start() {
221231

222232
/**
223233
* Returns queries executed by registered plugins.
224-
*
225-
* @return array<int, array<string, array{nodes:int, queries:int}>>
234+
* @return array<string, array<int, array<string, array{nodes:int,
235+
* queries:int}>>> The keys represent: event name, depth and plugin name
226236
*/
227237
public function getPluginQueries(): array {
228238
return $this->pluginQueries;

‎apps/dav/lib/Connector/Sabre/ServerFactory.php‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -95,6 +95,8 @@ public function createServer(
9595
$server->debugEnabled = $debugEnabled;
9696
$server->addPlugin(new PropFindMonitorPlugin());
9797
}
98+
99+
$server->addPlugin(new PropFindPreloadNotifyPlugin());
98100
// FIXME: The following line is a workaround for legacy components relying on being able to send a GET to /
99101
$server->addPlugin(new DummyGetResponsePlugin());
100102
$server->addPlugin(new ExceptionLoggerPlugin('webdav', $this->logger));

0 commit comments

Comments
 (0)