Skip to content

Commit d53a36b

Browse files
solracsfsolracsf
committed
perf(acl): cut per-folder/path DB queries during PROPFIND
Signed-off-by: Git'Fellow <12234510+solracsf@users.noreply.github.com>
1 parent d41e698 commit d53a36b

4 files changed

Lines changed: 219 additions & 3 deletions

File tree

‎lib/Folder/FolderManager.php‎

Lines changed: 63 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,21 @@
5959
class FolderManager {
6060
public const SPACE_DEFAULT = -4;
6161

62+
/**
63+
* Per-request cache of `acl_default_no_permission`; the value only changes on
64+
* folder creation, so it is safe to reuse for the whole request.
65+
*
66+
* @var array<int, bool>
67+
*/
68+
private array $aclDefaultNoPermissionCache = [];
69+
70+
/**
71+
* Per-request cache of `canManageACL`, keyed by "folderId::uid::excludeAdmins".
72+
*
73+
* @var array<string, bool>
74+
*/
75+
private array $canManageACLCache = [];
76+
6277
public function __construct(
6378
private readonly IDBConnection $connection,
6479
private readonly IGroupManager $groupManager,
@@ -480,6 +495,15 @@ private function getCircles(int $id): array {
480495
* @throws Exception
481496
*/
482497
public function canManageACL(int $folderId, IUser $user, bool $excludeAdmins = false): bool {
498+
$cacheKey = $folderId . '::' . $user->getUID() . '::' . ($excludeAdmins ? '1' : '0');
499+
if (isset($this->canManageACLCache[$cacheKey])) {
500+
return $this->canManageACLCache[$cacheKey];
501+
}
502+
503+
return $this->canManageACLCache[$cacheKey] = $this->computeCanManageACL($folderId, $user, $excludeAdmins);
504+
}
505+
506+
private function computeCanManageACL(int $folderId, IUser $user, bool $excludeAdmins): bool {
483507
$userId = $user->getUId();
484508
if (!$excludeAdmins && $this->groupManager->isAdmin($userId)) {
485509
return true;
@@ -921,6 +945,8 @@ public function setManageACL(int $folderId, string $type, string $id, bool $mana
921945

922946
$query->executeStatement();
923947

948+
$this->invalidateFolderAclCache($folderId);
949+
924950
$action = $manageAcl ? 'given' : 'revoked';
925951
$this->eventDispatcher->dispatchTyped(new CriticalActionPerformedEvent('The %s "%s" was %s acl management rights to the groupfolder with id %d', [$type, $id, $action, $folderId]));
926952
}
@@ -956,6 +982,8 @@ public function removeFolder(int $folderId): void {
956982

957983
$this->connection->commit();
958984

985+
$this->invalidateFolderAclCache($folderId);
986+
959987
$this->eventDispatcher->dispatchTyped(new CriticalActionPerformedEvent('The groupfolder with id %d was removed', [$folderId]));
960988

961989
$this->updateOverwriteHomeFolders();
@@ -1016,6 +1044,9 @@ public function deleteGroup(string $groupId): void {
10161044
->where($query->expr()->eq('mapping_id', $query->createNamedParameter($groupId)))
10171045
->andWhere($query->expr()->eq('mapping_type', $query->createNamedParameter('group')));
10181046
$query->executeStatement();
1047+
1048+
// group_folders_manage rows were removed, so canManageACL results may change
1049+
$this->canManageACLCache = [];
10191050
}
10201051

10211052
/**
@@ -1033,6 +1064,9 @@ public function deleteUser(string $userId): void {
10331064
->where($query->expr()->eq('mapping_id', $query->createNamedParameter($userId)))
10341065
->andWhere($query->expr()->eq('mapping_type', $query->createNamedParameter('user')));
10351066
$query->executeStatement();
1067+
1068+
// group_folders_manage rows were removed, so canManageACL results may change
1069+
$this->canManageACLCache = [];
10361070
}
10371071

10381072
/**
@@ -1071,6 +1105,8 @@ public function setFolderACL(int $folderId, bool $acl): void {
10711105
$query->executeStatement();
10721106
}
10731107

1108+
$this->invalidateFolderAclCache($folderId);
1109+
10741110
$action = $acl ? 'enabled' : 'disabled';
10751111
$this->eventDispatcher->dispatchTyped(new CriticalActionPerformedEvent('Advanced permissions for the groupfolder with id %d was %s', [$folderId, $action]));
10761112
}
@@ -1209,6 +1245,10 @@ public function updateOverwriteHomeFolders(): void {
12091245
}
12101246

12111247
public function hasFolderACLDefaultNoPermission(int $folderId): bool {
1248+
if (isset($this->aclDefaultNoPermissionCache[$folderId])) {
1249+
return $this->aclDefaultNoPermissionCache[$folderId];
1250+
}
1251+
12121252
$qb = $this->connection->getQueryBuilder();
12131253

12141254
$query = $qb
@@ -1220,7 +1260,29 @@ public function hasFolderACLDefaultNoPermission(int $folderId): bool {
12201260
$hasDefaultNoPermission = (bool)$result->fetchOne();
12211261
$result->closeCursor();
12221262

1223-
return $hasDefaultNoPermission;
1263+
return $this->aclDefaultNoPermissionCache[$folderId] = $hasDefaultNoPermission;
1264+
}
1265+
1266+
/**
1267+
* Seed the cache from an already-loaded folder so bulk callers (the mount
1268+
* provider) skip one `getBasePermission()` lookup per folder.
1269+
*/
1270+
public function primeAclDefaultNoPermission(FolderDefinition $folder): void {
1271+
$this->aclDefaultNoPermissionCache[$folder->id] = $folder->aclDefaultNoPermission;
1272+
}
1273+
1274+
/**
1275+
* Drop request-scoped memoized ACL state for a folder.
1276+
*
1277+
* Call after any mutation that can change `acl_default_no_permission` or the
1278+
* set of users/groups allowed to manage a folder's ACL, so later reads within
1279+
* the same request observe the new value.
1280+
*/
1281+
private function invalidateFolderAclCache(int $folderId): void {
1282+
unset($this->aclDefaultNoPermissionCache[$folderId]);
1283+
// canManageACL keys embed the folder id together with user/excludeAdmins;
1284+
// clearing the whole (small, request-scoped) map is simplest and safe.
1285+
$this->canManageACLCache = [];
12241286
}
12251287

12261288
public function countAllFolders(): int {

‎lib/Mount/FolderStorageManager.php‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -116,7 +116,8 @@ private function getBaseStorageForFolderSeparate(
116116
'acl_manager' => $aclManager,
117117
'in_share' => $inShare,
118118
'folder_id' => $folderId,
119-
'storage_id' => $storage->getCache()->getNumericStorageId(),
119+
// already loaded with the folder; avoids a storages lookup per folder
120+
'storage_id' => $folder->storageId,
120121
]);
121122
}
122123

@@ -250,7 +251,8 @@ private function getBaseStorageForFolderRootJail(
250251
'acl_manager' => $aclManager,
251252
'in_share' => $inShare,
252253
'folder_id' => $folderId,
253-
'storage_id' => $rootStorage->getCache()->getNumericStorageId(),
254+
// already loaded; the same id MountProvider uses for rule lookups (incl. legacy root-jail)
255+
'storage_id' => $folder->storageId,
254256
]);
255257
}
256258

‎lib/Mount/MountProvider.php‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,12 @@ public function __construct(
5454
public function getMountsForUser(IUser $user, IStorageFactory $loader): array {
5555
$folders = $this->folderManager->getFoldersForUser($user);
5656

57+
// The folder definitions already carry `aclDefaultNoPermission`; prime the
58+
// cache so the per-folder `getBasePermission()` below does not re-query it.
59+
foreach ($folders as $folder) {
60+
$this->folderManager->primeAclDefaultNoPermission($folder);
61+
}
62+
5763
$mountPoints = array_map(fn (FolderDefinitionWithPermissions $folder): string => 'files/' . $folder->mountPoint, $folders);
5864
$conflicts = $this->findConflictsForUser($user, $mountPoints);
5965

@@ -307,6 +313,8 @@ public function getMountsForPath(string $setupPathHint, bool $forChildren, array
307313
$folders = $this->folderManager->getFoldersForUser($user, null, $relativePaths);
308314

309315
foreach ($folders as $folder) {
316+
$this->folderManager->primeAclDefaultNoPermission($folder);
317+
310318
$mountPoint = '/' . $user->getUID() . '/files/' . $folder->mountPoint;
311319

312320
$mounts[$mountPoint] = $this->getMount(

‎tests/Folder/FolderManagerTest.php‎

Lines changed: 144 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
use OCA\GroupFolders\Mount\FolderStorageManager;
1717
use OCA\GroupFolders\ResponseDefinitions;
1818
use OCP\Constants;
19+
use OCP\DB\QueryBuilder\IQueryBuilder;
1920
use OCP\EventDispatcher\IEventDispatcher;
2021
use OCP\Files\FileInfo;
2122
use OCP\Files\IMimeTypeLoader;
@@ -517,4 +518,147 @@ public function testQuotaDefaultValue(): void {
517518
}
518519
$this->assertEquals(1024 ** 4, $folder->quota);
519520
}
521+
522+
/**
523+
* Regression test for the PROPFIND/ACL per-path query.
524+
*
525+
* `hasFolderACLDefaultNoPermission()` is called once per path during ACL
526+
* permission calculation (`ACLManager::getBasePermission`). It must be served
527+
* from a request-scoped cache instead of querying `group_folders` every time.
528+
*/
529+
public function testHasFolderACLDefaultNoPermissionIsMemoizedUntilInvalidated(): void {
530+
$folderId = $this->manager->createFolder('acl-default-test', [], true);
531+
532+
// First read populates the cache.
533+
$this->assertTrue($this->manager->hasFolderACLDefaultNoPermission($folderId));
534+
535+
// Flip the stored value behind the manager's back.
536+
$db = Server::get(IDBConnection::class);
537+
$update = $db->getQueryBuilder();
538+
$update->update('group_folders')
539+
->set('acl_default_no_permission', $update->createNamedParameter(0, IQueryBuilder::PARAM_INT))
540+
->where($update->expr()->eq('folder_id', $update->createNamedParameter($folderId, IQueryBuilder::PARAM_INT)));
541+
$update->executeStatement();
542+
543+
// Still served from cache: proves the per-call query was eliminated.
544+
$this->assertTrue($this->manager->hasFolderACLDefaultNoPermission($folderId));
545+
546+
// A mutation through the manager invalidates the cache and forces a fresh read.
547+
$this->manager->setManageACL($folderId, 'group', 'somegroup', true);
548+
$this->assertFalse($this->manager->hasFolderACLDefaultNoPermission($folderId));
549+
}
550+
551+
/**
552+
* Regression test for the repeated manager-mapping lookups.
553+
*
554+
* `canManageACL()` is consulted once per path for folders that grant no
555+
* permission by default. The result is stable within a request, so the
556+
* underlying lookups must only run once.
557+
*/
558+
public function testCanManageACLIsMemoized(): void {
559+
$folderId = $this->manager->createFolder('manage-cache-test');
560+
561+
$user = $this->createMock(IUser::class);
562+
$user->method('getUID')->willReturn('alice');
563+
564+
$this->groupManager->method('isAdmin')->willReturn(false);
565+
// The DB-backed mapping check must run exactly once across both calls.
566+
$this->userMappingManager->expects($this->once())
567+
->method('userInMappings')
568+
->willReturn(false);
569+
570+
$this->assertFalse($this->manager->canManageACL($folderId, $user));
571+
$this->assertFalse($this->manager->canManageACL($folderId, $user));
572+
}
573+
574+
/**
575+
* The `canManageACL` cache must be dropped when the manager mappings change,
576+
* so a later check within the same request observes the new state.
577+
*/
578+
public function testCanManageACLCacheInvalidatedOnManageChange(): void {
579+
$folderId = $this->manager->createFolder('manage-invalidate-test');
580+
581+
$user = $this->createMock(IUser::class);
582+
$user->method('getUID')->willReturn('alice');
583+
584+
$this->groupManager->method('isAdmin')->willReturn(false);
585+
// Recomputed once before and once after the invalidating mutation.
586+
$this->userMappingManager->expects($this->exactly(2))
587+
->method('userInMappings')
588+
->willReturn(false);
589+
590+
$this->assertFalse($this->manager->canManageACL($folderId, $user));
591+
$this->manager->setManageACL($folderId, 'group', 'somegroup', true);
592+
$this->assertFalse($this->manager->canManageACL($folderId, $user));
593+
}
594+
595+
/**
596+
* The ACL storage wrapper uses the folder's stored `storage_id` instead of
597+
* resolving the numeric id per folder; this guards that the stored value
598+
* stays equal to the live numeric storage id of the folder's files storage.
599+
*/
600+
public function testStoredStorageIdMatchesLiveNumericStorageId(): void {
601+
$this->config->method('getSystemValueInt')
602+
->with('groupfolders.quota.default', FileInfo::SPACE_UNLIMITED)
603+
->willReturn(FileInfo::SPACE_UNLIMITED);
604+
605+
$folderId = $this->manager->createFolder('storage-id-test');
606+
$folder = $this->manager->getFolder($folderId);
607+
if (!$folder) {
608+
throw new \Exception('Folder not found');
609+
}
610+
611+
$storage = $this->folderStorageManager->getBaseStorageForFolder(
612+
$folderId,
613+
$folder->useSeparateStorage(),
614+
$folder,
615+
null,
616+
false,
617+
'files',
618+
);
619+
620+
$this->assertSame($folder->storageId, $storage->getCache()->getNumericStorageId());
621+
}
622+
623+
/**
624+
* Deleting a group removes its manager mappings, so the `canManageACL` cache
625+
* must be dropped for a later check in the same request to stay correct.
626+
*/
627+
public function testCanManageACLCacheInvalidatedOnGroupDeletion(): void {
628+
$folderId = $this->manager->createFolder('group-delete-test');
629+
630+
$user = $this->createMock(IUser::class);
631+
$user->method('getUID')->willReturn('alice');
632+
633+
$this->groupManager->method('isAdmin')->willReturn(false);
634+
// Recomputed once before and once after the group deletion.
635+
$this->userMappingManager->expects($this->exactly(2))
636+
->method('userInMappings')
637+
->willReturn(false);
638+
639+
$this->assertFalse($this->manager->canManageACL($folderId, $user));
640+
$this->manager->deleteGroup('somegroup');
641+
$this->assertFalse($this->manager->canManageACL($folderId, $user));
642+
}
643+
644+
/**
645+
* Deleting a user removes its manager mappings, so the `canManageACL` cache
646+
* must be dropped as well.
647+
*/
648+
public function testCanManageACLCacheInvalidatedOnUserDeletion(): void {
649+
$folderId = $this->manager->createFolder('user-delete-test');
650+
651+
$user = $this->createMock(IUser::class);
652+
$user->method('getUID')->willReturn('alice');
653+
654+
$this->groupManager->method('isAdmin')->willReturn(false);
655+
// Recomputed once before and once after the user deletion.
656+
$this->userMappingManager->expects($this->exactly(2))
657+
->method('userInMappings')
658+
->willReturn(false);
659+
660+
$this->assertFalse($this->manager->canManageACL($folderId, $user));
661+
$this->manager->deleteUser('bob');
662+
$this->assertFalse($this->manager->canManageACL($folderId, $user));
663+
}
520664
}

0 commit comments

Comments
 (0)