Skip to content

Commit 4c61d02

Browse files
CarlSchwanCarlSchwan
authored
Merge pull request #61904 from nextcloud/carl/groups-optimization
perf: Replace more calls from getUserGroups to getUserGroupIds
2 parents baeb766 + 2b5395a commit 4c61d02

6 files changed

Lines changed: 60 additions & 34 deletions

File tree

‎apps/provisioning_api/lib/Controller/AUserDataOCSController.php‎

Lines changed: 6 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -106,11 +106,7 @@ protected function getUserData(string $userId, bool $includeScopes = false): ?ar
106106

107107
// Get groups data
108108
$userAccount = $this->accountManager->getAccount($targetUserObject);
109-
$groups = $this->groupManager->getUserGroups($targetUserObject);
110-
$gids = [];
111-
foreach ($groups as $group) {
112-
$gids[] = $group->getGID();
113-
}
109+
$gids = $this->groupManager->getUserGroupIds($targetUserObject);
114110

115111
if ($isAdmin || $isDelegatedAdmin) {
116112
try {
@@ -279,10 +275,11 @@ protected function findGroupsWithDisplayname(array $userDetails): array {
279275
$groupIds = array_unique($groupIds);
280276
sort($groupIds);
281277

282-
return array_map(function ($groupId) {
283-
$displayname = $this->groupDisplayNameCache->getDisplayName($groupId) ?? $groupId;
284-
return ['id' => $groupId, 'displayname' => $displayname];
285-
}, $groupIds);
278+
$info = [];
279+
foreach ($this->groupDisplayNameCache->getDisplayNames($groupIds) as $groupId => $displayName) {
280+
$info[] = ['id' => $groupId, 'displayname' => $displayName ?? $groupId];
281+
}
282+
return $info;
286283
}
287284

288285
/**

‎apps/provisioning_api/lib/Controller/UsersController.php‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1080,8 +1080,7 @@ public function editUserMultiField(
10801080
}
10811081

10821082
if ($groups !== null) {
1083-
$currentGroups = $this->groupManager->getUserGroups($targetUser);
1084-
$currentGroupIds = array_map(fn (IGroup $g) => $g->getGID(), $currentGroups);
1083+
$currentGroupIds = $this->groupManager->getUserGroupIds($targetUser);
10851084
foreach (array_diff($currentGroupIds, $groups) as $gid) {
10861085
$this->groupManager->get($gid)?->removeUser($targetUser);
10871086
}

‎apps/provisioning_api/tests/Controller/GroupsControllerTest.php‎

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -483,8 +483,8 @@ public function testGetGroupUsersDetails(): void {
483483
->with($gid)
484484
->willReturn($group);
485485
$this->groupManager->expects($this->any())
486-
->method('getUserGroups')
487-
->willReturn([$group]);
486+
->method('getUserGroupIds')
487+
->willReturn(['ncg1']);
488488

489489
/** @var MockObject */
490490
$this->subAdminManager->expects($this->any())
@@ -495,9 +495,9 @@ public function testGetGroupUsersDetails(): void {
495495
->willReturn([]);
496496

497497
$this->groupDisplayNameCache
498-
->method('getDisplayName')
499-
->with('ncg1')
500-
->willReturn('Group One');
498+
->method('getDisplayNames')
499+
->with(['ncg1'])
500+
->willReturn(['ncg1' => 'Group One']);
501501

502502
$result = $this->api->getGroupUsersDetails($gid);
503503

@@ -538,8 +538,8 @@ public function testGetGroupUsersDetailsEncoded(): void {
538538
->with($gid)
539539
->willReturn($group);
540540
$this->groupManager->expects($this->any())
541-
->method('getUserGroups')
542-
->willReturn([$group]);
541+
->method('getUserGroupIds')
542+
->willReturn(['Department A/B C/D']);
543543

544544
/** @var MockObject */
545545
$this->subAdminManager->expects($this->any())
@@ -550,9 +550,9 @@ public function testGetGroupUsersDetailsEncoded(): void {
550550
->willReturn([]);
551551

552552
$this->groupDisplayNameCache
553-
->method('getDisplayName')
554-
->with('Department A/B C/D')
555-
->willReturn('Department A/B C/D-name');
553+
->method('getDisplayNames')
554+
->with(['Department A/B C/D'])
555+
->willReturn(['Department A/B C/D' => 'Department A/B C/D-name']);
556556

557557
$result = $this->api->getGroupUsersDetails(urlencode($gid));
558558

‎apps/provisioning_api/tests/Controller/UsersControllerTest.php‎

Lines changed: 3 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1196,8 +1196,8 @@ public function testGetUserDataAsAdmin(): void {
11961196
->willReturn(true);
11971197
$this->groupManager
11981198
->expects($this->any())
1199-
->method('getUserGroups')
1200-
->willReturn([$group0, $group1, $group2]);
1199+
->method('getUserGroupIds')
1200+
->willReturn(['group0', 'group1', 'group2']);
12011201
$this->groupManager
12021202
->expects($this->once())
12031203
->method('getSubAdmin')
@@ -1206,15 +1206,6 @@ public function testGetUserDataAsAdmin(): void {
12061206
->expects($this->once())
12071207
->method('getSubAdminsGroups')
12081208
->willReturn([$group3]);
1209-
$group0->expects($this->exactly(1))
1210-
->method('getGID')
1211-
->willReturn('group0');
1212-
$group1->expects($this->exactly(1))
1213-
->method('getGID')
1214-
->willReturn('group1');
1215-
$group2->expects($this->exactly(1))
1216-
->method('getGID')
1217-
->willReturn('group2');
12181209
$group3->expects($this->once())
12191210
->method('getGID')
12201211
->willReturn('group3');
@@ -2759,7 +2750,7 @@ public function testUpdateUserGroupDiff(): void {
27592750
$newGroup = $this->createMock(IGroup::class);
27602751
$newGroup->method('getGID')->willReturn('newgroup');
27612752

2762-
$this->groupManager->method('getUserGroups')->willReturn([$oldGroup]);
2753+
$this->groupManager->method('getUserGroupIds')->willReturn(['oldgroup']);
27632754
$this->groupManager->method('groupExists')->willReturn(true);
27642755
$this->groupManager->method('get')->willReturnMap([
27652756
['newgroup', $newGroup],

‎lib/private/Group/DisplayNameCache.php‎

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,45 @@ public function getDisplayName(string $groupId): ?string {
5757
return $displayName;
5858
}
5959

60+
/**
61+
* @param list<string> $groupIds
62+
* @return array<string, ?string>
63+
*/
64+
public function getDisplayNames(array $groupIds): array {
65+
$result = [];
66+
$missing = [];
67+
foreach ($groupIds as $groupId) {
68+
if (isset($this->cache[$groupId])) {
69+
$result[$groupId] = $this->cache[$groupId];
70+
} else {
71+
$displayName = $this->memCache->get($groupId);
72+
if ($displayName) {
73+
$this->cache[$groupId] = $displayName;
74+
$result[$groupId] = $displayName;
75+
} else {
76+
$missing[] = $groupId;
77+
}
78+
}
79+
}
80+
81+
/** @var Manager $groupManager */
82+
$groupManager = $this->groupManager;
83+
$groups = $groupManager->getGroupsObjects($missing);
84+
$stillMissingGroups = array_diff($missing, array_keys($groups));
85+
foreach ($groups as $groupId => $group) {
86+
$displayName = $group->getDisplayName();
87+
$this->cache[$groupId] = $displayName;
88+
$this->memCache->set($groupId, $displayName, 60 * 10); // 10 minutes
89+
$result[$groupId] = $displayName;
90+
}
91+
92+
foreach ($stillMissingGroups as $groupId) {
93+
$result[$groupId] = null;
94+
}
95+
96+
return $result;
97+
}
98+
6099
public function clear(): void {
61100
$this->cache = new CappedMemoryCache();
62101
$this->memCache->clear();

‎lib/private/Group/Manager.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -181,7 +181,7 @@ protected function getGroupObject($gid, $displayName = null) {
181181
* @param array<string, string> $displayNames Array containing already know display name for a groupId
182182
* @return array<string, IGroup>
183183
*/
184-
protected function getGroupsObjects(array $gids, array $displayNames = []): array {
184+
public function getGroupsObjects(array $gids, array $displayNames = []): array {
185185
$backends = [];
186186
$groups = [];
187187
foreach ($gids as $gid) {

0 commit comments

Comments
 (0)