Skip to content

Commit ffb56ef

Browse files
nickvergessennickvergessen
authored
Merge pull request #62223 from nextcloud/backport/61904/stable34
[stable34] perf: Replace more calls from getUserGroups to getUserGroupIds
2 parents b18b6f8 + 303b368 commit ffb56ef

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
@@ -105,11 +105,7 @@ protected function getUserData(string $userId, bool $includeScopes = false): ?ar
105105

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

114110
if ($isAdmin || $isDelegatedAdmin) {
115111
try {
@@ -278,10 +274,11 @@ protected function findGroupsWithDisplayname(array $userDetails): array {
278274
$groupIds = array_unique($groupIds);
279275
sort($groupIds);
280276

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

287284
/**

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

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

10841084
if ($groups !== null) {
1085-
$currentGroups = $this->groupManager->getUserGroups($targetUser);
1086-
$currentGroupIds = array_map(fn (IGroup $g) => $g->getGID(), $currentGroups);
1085+
$currentGroupIds = $this->groupManager->getUserGroupIds($targetUser);
10871086
foreach (array_diff($currentGroupIds, $groups) as $gid) {
10881087
$this->groupManager->get($gid)?->removeUser($targetUser);
10891088
}

‎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
@@ -1206,8 +1206,8 @@ public function testGetUserDataAsAdmin(): void {
12061206
->willReturn(true);
12071207
$this->groupManager
12081208
->expects($this->any())
1209-
->method('getUserGroups')
1210-
->willReturn([$group0, $group1, $group2]);
1209+
->method('getUserGroupIds')
1210+
->willReturn(['group0', 'group1', 'group2']);
12111211
$this->groupManager
12121212
->expects($this->once())
12131213
->method('getSubAdmin')
@@ -1216,15 +1216,6 @@ public function testGetUserDataAsAdmin(): void {
12161216
->expects($this->once())
12171217
->method('getSubAdminsGroups')
12181218
->willReturn([$group3]);
1219-
$group0->expects($this->exactly(1))
1220-
->method('getGID')
1221-
->willReturn('group0');
1222-
$group1->expects($this->exactly(1))
1223-
->method('getGID')
1224-
->willReturn('group1');
1225-
$group2->expects($this->exactly(1))
1226-
->method('getGID')
1227-
->willReturn('group2');
12281219
$group3->expects($this->once())
12291220
->method('getGID')
12301221
->willReturn('group3');
@@ -2778,7 +2769,7 @@ public function testUpdateUserGroupDiff(): void {
27782769
$newGroup = $this->createMock(IGroup::class);
27792770
$newGroup->method('getGID')->willReturn('newgroup');
27802771

2781-
$this->groupManager->method('getUserGroups')->willReturn([$oldGroup]);
2772+
$this->groupManager->method('getUserGroupIds')->willReturn(['oldgroup']);
27822773
$this->groupManager->method('groupExists')->willReturn(true);
27832774
$this->groupManager->method('get')->willReturnMap([
27842775
['newgroup', $newGroup],

‎lib/private/Group/DisplayNameCache.php‎

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

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

‎lib/private/Group/Manager.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -173,7 +173,7 @@ protected function getGroupObject($gid, $displayName = null) {
173173
* @param array<string, string> $displayNames Array containing already know display name for a groupId
174174
* @return array<string, IGroup>
175175
*/
176-
protected function getGroupsObjects(array $gids, array $displayNames = []): array {
176+
public function getGroupsObjects(array $gids, array $displayNames = []): array {
177177
$backends = [];
178178
$groups = [];
179179
foreach ($gids as $gid) {

0 commit comments

Comments
 (0)