Skip to content

Commit 89fa14f

Browse files
authored
Merge pull request #54386 from nextcloud/fix-n+1-caldav
fix(performance): Fix n+1 issue when fetching calendar properties
2 parents 64c5200 + 9df79ba commit 89fa14f

File tree

9 files changed

+156
-15
lines changed

9 files changed

+156
-15
lines changed

apps/dav/lib/CalDAV/Calendar.php

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ class Calendar extends \Sabre\CalDAV\Calendar implements IRestorable, IShareable
3636

3737
public function __construct(
3838
BackendInterface $caldavBackend,
39-
$calendarInfo,
39+
array $calendarInfo,
4040
IL10N $l10n,
4141
private IConfig $config,
4242
private LoggerInterface $logger,
@@ -60,6 +60,10 @@ public function __construct(
6060
$this->l10n = $l10n;
6161
}
6262

63+
public function getUri(): string {
64+
return $this->calendarInfo['uri'];
65+
}
66+
6367
/**
6468
* {@inheritdoc}
6569
* @throws Forbidden

apps/dav/lib/CalDAV/CalendarProvider.php

Lines changed: 29 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -36,9 +36,14 @@ public function getCalendars(string $principalUri, array $calendarUris = []): ar
3636
});
3737
}
3838

39+
$additionalProperties = $this->getAdditionalPropertiesForCalendars($calendarInfos);
3940
$iCalendars = [];
4041
foreach ($calendarInfos as $calendarInfo) {
41-
$calendarInfo = array_merge($calendarInfo, $this->getAdditionalProperties($calendarInfo['principaluri'], $calendarInfo['uri']));
42+
$user = str_replace('principals/users/', '', $calendarInfo['principaluri']);
43+
$path = 'calendars/' . $user . '/' . $calendarInfo['uri'];
44+
45+
$calendarInfo = array_merge($calendarInfo, $additionalProperties[$path] ?? []);
46+
4247
$calendar = new Calendar($this->calDavBackend, $calendarInfo, $this->l10n, $this->config, $this->logger);
4348
$iCalendars[] = new CalendarImpl(
4449
$calendar,
@@ -49,16 +54,34 @@ public function getCalendars(string $principalUri, array $calendarUris = []): ar
4954
return $iCalendars;
5055
}
5156

52-
public function getAdditionalProperties(string $principalUri, string $calendarUri): array {
53-
$user = str_replace('principals/users/', '', $principalUri);
54-
$path = 'calendars/' . $user . '/' . $calendarUri;
57+
/**
58+
* @param array{
59+
* principaluri: string,
60+
* uri: string,
61+
* }[] $uris
62+
* @return array<string, array<string, string|bool>>
63+
*/
64+
private function getAdditionalPropertiesForCalendars(array $uris): array {
65+
$calendars = [];
66+
foreach ($uris as $uri) {
67+
/** @var string $user */
68+
$user = str_replace('principals/users/', '', $uri['principaluri']);
69+
if (!array_key_exists($user, $calendars)) {
70+
$calendars[$user] = [];
71+
}
72+
$calendars[$user][] = 'calendars/' . $user . '/' . $uri['uri'];
73+
}
5574

56-
$properties = $this->propertyMapper->findPropertiesByPath($user, $path);
75+
$properties = $this->propertyMapper->findPropertiesByPathsAndUsers($calendars);
5776

5877
$list = [];
5978
foreach ($properties as $property) {
6079
if ($property instanceof Property) {
61-
$list[$property->getPropertyname()] = match ($property->getPropertyname()) {
80+
if (!isset($list[$property->getPropertypath()])) {
81+
$list[$property->getPropertypath()] = [];
82+
}
83+
84+
$list[$property->getPropertypath()][$property->getPropertyname()] = match ($property->getPropertyname()) {
6285
'{http://owncloud.org/ns}calendar-enabled' => (bool)$property->getPropertyvalue(),
6386
default => $property->getPropertyvalue()
6487
};

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
use OCA\DAV\CalDAV\Proxy\ProxyMapper;
1515
use OCA\DAV\DAV\CustomPropertiesBackend;
1616
use OCA\DAV\DAV\ViewOnlyPlugin;
17+
use OCA\DAV\Db\PropertyMapper;
1718
use OCA\DAV\Files\BrowserErrorPagePlugin;
1819
use OCA\DAV\Files\Sharing\RootCollection;
1920
use OCA\DAV\Upload\CleanupService;
@@ -226,6 +227,7 @@ public function createServer(
226227
$tree,
227228
$this->databaseConnection,
228229
$this->userSession->getUser(),
230+
\OCP\Server::get(PropertyMapper::class),
229231
\OCP\Server::get(DefaultCalendarValidator::class),
230232
)
231233
)

apps/dav/lib/DAV/CustomPropertiesBackend.php

Lines changed: 78 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -9,13 +9,20 @@
99
namespace OCA\DAV\DAV;
1010

1111
use Exception;
12+
use OCA\DAV\CalDAV\CalDavBackend;
1213
use OCA\DAV\CalDAV\Calendar;
14+
use OCA\DAV\CalDAV\CalendarHome;
1315
use OCA\DAV\CalDAV\CalendarObject;
1416
use OCA\DAV\CalDAV\DefaultCalendarValidator;
17+
use OCA\DAV\CalDAV\Integration\ExternalCalendar;
18+
use OCA\DAV\CalDAV\Outbox;
19+
use OCA\DAV\CalDAV\Trashbin\TrashbinHome;
1520
use OCA\DAV\Connector\Sabre\Directory;
21+
use OCA\DAV\Db\PropertyMapper;
1622
use OCP\DB\QueryBuilder\IQueryBuilder;
1723
use OCP\IDBConnection;
1824
use OCP\IUser;
25+
use Sabre\CalDAV\Schedule\Inbox;
1926
use Sabre\DAV\Exception as DavException;
2027
use Sabre\DAV\PropertyStorage\Backend\BackendInterface;
2128
use Sabre\DAV\PropFind;
@@ -98,10 +105,9 @@ class CustomPropertiesBackend implements BackendInterface {
98105

99106
/**
100107
* Properties cache
101-
*
102-
* @var array
103108
*/
104-
private $userCache = [];
109+
private array $userCache = [];
110+
private array $publishedCache = [];
105111
private XmlService $xmlService;
106112

107113
/**
@@ -114,6 +120,7 @@ public function __construct(
114120
private Tree $tree,
115121
private IDBConnection $connection,
116122
private IUser $user,
123+
private PropertyMapper $propertyMapper,
117124
private DefaultCalendarValidator $defaultCalendarValidator,
118125
) {
119126
$this->xmlService = new XmlService();
@@ -197,6 +204,13 @@ public function propFind($path, PropFind $propFind) {
197204
$this->cacheDirectory($path, $node);
198205
}
199206

207+
if ($node instanceof CalendarHome && $propFind->getDepth() !== 0) {
208+
$backend = $node->getCalDAVBackend();
209+
if ($backend instanceof CalDavBackend) {
210+
$this->cacheCalendars($node, $requestedProps);
211+
}
212+
}
213+
200214
if ($node instanceof CalendarObject) {
201215
// No custom properties supported on individual events
202216
return;
@@ -316,6 +330,10 @@ private function getPublishedProperties(string $path, array $requestedProperties
316330
return [];
317331
}
318332

333+
if (isset($this->publishedCache[$path])) {
334+
return $this->publishedCache[$path];
335+
}
336+
319337
$qb = $this->connection->getQueryBuilder();
320338
$qb->select('*')
321339
->from(self::TABLE_NAME)
@@ -326,6 +344,7 @@ private function getPublishedProperties(string $path, array $requestedProperties
326344
$props[$row['propertyname']] = $this->decodeValueFromDatabase($row['propertyvalue'], $row['valuetype']);
327345
}
328346
$result->closeCursor();
347+
$this->publishedCache[$path] = $props;
329348
return $props;
330349
}
331350

@@ -364,6 +383,62 @@ private function cacheDirectory(string $path, Directory $node): void {
364383
$this->userCache = array_merge($this->userCache, $propsByPath);
365384
}
366385

386+
private function cacheCalendars(CalendarHome $node, array $requestedProperties): void {
387+
$calendars = $node->getChildren();
388+
389+
$users = [];
390+
foreach ($calendars as $calendar) {
391+
if ($calendar instanceof Calendar) {
392+
$user = str_replace('principals/users/', '', $calendar->getPrincipalURI());
393+
if (!isset($users[$user])) {
394+
$users[$user] = ['calendars/' . $user];
395+
}
396+
$users[$user][] = 'calendars/' . $user . '/' . $calendar->getUri();
397+
} elseif ($calendar instanceof Inbox || $calendar instanceof Outbox || $calendar instanceof TrashbinHome || $calendar instanceof ExternalCalendar) {
398+
if ($calendar->getOwner()) {
399+
$user = str_replace('principals/users/', '', $calendar->getOwner());
400+
if (!isset($users[$user])) {
401+
$users[$user] = ['calendars/' . $user];
402+
}
403+
$users[$user][] = 'calendars/' . $user . '/' . $calendar->getName();
404+
}
405+
}
406+
}
407+
408+
// user properties
409+
$properties = $this->propertyMapper->findPropertiesByPathsAndUsers($users);
410+
411+
$propsByPath = [];
412+
foreach ($users as $paths) {
413+
foreach ($paths as $path) {
414+
$propsByPath[$path] = [];
415+
}
416+
}
417+
418+
foreach ($properties as $property) {
419+
$propsByPath[$property->getPropertypath()][$property->getPropertyname()] = $this->decodeValueFromDatabase($property->getPropertyvalue(), $property->getValuetype());
420+
}
421+
$this->userCache = array_merge($this->userCache, $propsByPath);
422+
423+
// published properties
424+
$allowedProps = array_intersect(self::PUBLISHED_READ_ONLY_PROPERTIES, $requestedProperties);
425+
if (empty($allowedProps)) {
426+
return;
427+
}
428+
$paths = [];
429+
foreach ($users as $nestedPaths) {
430+
$paths = array_merge($paths, $nestedPaths);
431+
}
432+
$paths = array_unique($paths);
433+
434+
$propsByPath = array_fill_keys(array_values($paths), []);
435+
$properties = $this->propertyMapper->findPropertiesByPaths($paths, $allowedProps);
436+
foreach ($properties as $property) {
437+
$propsByPath[$property->getPropertypath()][$property->getPropertyname()] = $this->decodeValueFromDatabase($property->getPropertyvalue(), $property->getValuetype());
438+
}
439+
$this->publishedCache = array_merge($this->publishedCache, $propsByPath);
440+
}
441+
367442
/**
368443
* Returns a list of properties for the given path and current user
369444
*

apps/dav/lib/Db/Property.php

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
* @method string getPropertypath()
1717
* @method string getPropertyname()
1818
* @method string getPropertyvalue()
19+
* @method int getValuetype()
1920
*/
2021
class Property extends Entity {
2122

apps/dav/lib/Db/PropertyMapper.php

Lines changed: 32 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
namespace OCA\DAV\Db;
1111

1212
use OCP\AppFramework\Db\QBMapper;
13+
use OCP\DB\QueryBuilder\IQueryBuilder;
1314
use OCP\IDBConnection;
1415

1516
/**
@@ -39,17 +40,43 @@ public function findPropertyByPathAndName(string $userId, string $path, string $
3940
}
4041

4142
/**
43+
* @param array<string, string[]> $calendars
4244
* @return Property[]
45+
* @throws \OCP\DB\Exception
4346
*/
44-
public function findPropertiesByPath(string $userId, string $path): array {
47+
public function findPropertiesByPathsAndUsers(array $calendars): array {
4548
$selectQb = $this->db->getQueryBuilder();
4649
$selectQb->select('*')
47-
->from(self::TABLE_NAME)
48-
->where(
49-
$selectQb->expr()->eq('userid', $selectQb->createNamedParameter($userId)),
50-
$selectQb->expr()->eq('propertypath', $selectQb->createNamedParameter($path)),
50+
->from(self::TABLE_NAME);
51+
52+
foreach ($calendars as $user => $paths) {
53+
$selectQb->orWhere(
54+
$selectQb->expr()->andX(
55+
$selectQb->expr()->eq('userid', $selectQb->createNamedParameter($user)),
56+
$selectQb->expr()->in('propertypath', $selectQb->createNamedParameter($paths, IQueryBuilder::PARAM_STR_ARRAY)),
57+
)
5158
);
59+
}
60+
5261
return $this->findEntities($selectQb);
5362
}
5463

64+
/**
65+
* @param string[] $calendars
66+
* @param string[] $allowedProperties
67+
* @return Property[]
68+
* @throws \OCP\DB\Exception
69+
*/
70+
public function findPropertiesByPaths(array $calendars, array $allowedProperties = []): array {
71+
$selectQb = $this->db->getQueryBuilder();
72+
$selectQb->select('*')
73+
->from(self::TABLE_NAME)
74+
->where($selectQb->expr()->in('propertypath', $selectQb->createNamedParameter($calendars, IQueryBuilder::PARAM_STR_ARRAY)));
75+
76+
if ($allowedProperties) {
77+
$selectQb->andWhere($selectQb->expr()->in('propertyname', $selectQb->createNamedParameter($allowedProperties, IQueryBuilder::PARAM_STR_ARRAY)));
78+
}
79+
80+
return $this->findEntities($selectQb);
81+
}
5582
}

apps/dav/lib/Server.php

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,7 @@
5454
use OCA\DAV\DAV\CustomPropertiesBackend;
5555
use OCA\DAV\DAV\PublicAuth;
5656
use OCA\DAV\DAV\ViewOnlyPlugin;
57+
use OCA\DAV\Db\PropertyMapper;
5758
use OCA\DAV\Events\SabrePluginAddEvent;
5859
use OCA\DAV\Events\SabrePluginAuthInitEvent;
5960
use OCA\DAV\Files\BrowserErrorPagePlugin;
@@ -306,6 +307,7 @@ public function __construct(
306307
$this->server->tree,
307308
\OCP\Server::get(IDBConnection::class),
308309
\OCP\Server::get(IUserSession::class)->getUser(),
310+
\OCP\Server::get(PropertyMapper::class),
309311
\OCP\Server::get(DefaultCalendarValidator::class),
310312
)
311313
)

apps/dav/tests/unit/Connector/Sabre/CustomPropertiesBackendTest.php

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
use OCA\DAV\Connector\Sabre\Directory;
1313
use OCA\DAV\Connector\Sabre\File;
1414
use OCA\DAV\DAV\CustomPropertiesBackend;
15+
use OCA\DAV\Db\PropertyMapper;
1516
use OCP\IDBConnection;
1617
use OCP\IUser;
1718
use OCP\Server;
@@ -52,6 +53,7 @@ protected function setUp(): void {
5253
$this->tree,
5354
Server::get(IDBConnection::class),
5455
$this->user,
56+
Server::get(PropertyMapper::class),
5557
$this->defaultCalendarValidator,
5658
);
5759
}

apps/dav/tests/unit/DAV/CustomPropertiesBackendTest.php

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
use OCA\DAV\CalDAV\Calendar;
1111
use OCA\DAV\CalDAV\DefaultCalendarValidator;
1212
use OCA\DAV\DAV\CustomPropertiesBackend;
13+
use OCA\DAV\Db\PropertyMapper;
1314
use OCP\DB\QueryBuilder\IQueryBuilder;
1415
use OCP\IDBConnection;
1516
use OCP\IUser;
@@ -36,6 +37,7 @@ class CustomPropertiesBackendTest extends TestCase {
3637
private IUser&MockObject $user;
3738
private DefaultCalendarValidator&MockObject $defaultCalendarValidator;
3839
private CustomPropertiesBackend $backend;
40+
private PropertyMapper $propertyMapper;
3941

4042
protected function setUp(): void {
4143
parent::setUp();
@@ -49,13 +51,15 @@ protected function setUp(): void {
4951
->with()
5052
->willReturn('dummy_user_42');
5153
$this->dbConnection = \OCP\Server::get(IDBConnection::class);
54+
$this->propertyMapper = \OCP\Server::get(PropertyMapper::class);
5255
$this->defaultCalendarValidator = $this->createMock(DefaultCalendarValidator::class);
5356

5457
$this->backend = new CustomPropertiesBackend(
5558
$this->server,
5659
$this->tree,
5760
$this->dbConnection,
5861
$this->user,
62+
$this->propertyMapper,
5963
$this->defaultCalendarValidator,
6064
);
6165
}
@@ -129,6 +133,7 @@ public function testPropFindNoDbCalls(): void {
129133
$this->tree,
130134
$db,
131135
$this->user,
136+
$this->propertyMapper,
132137
$this->defaultCalendarValidator,
133138
);
134139

0 commit comments

Comments
 (0)