Skip to content

Commit f7632f2

Browse files
come-nccome-nc
authored
Merge pull request #32866 from nextcloud/performance/searchInGroup-displayname-cache
Optimize retrieving display name when searching for users in a group
2 parents ac56be1 + 10296ba commit f7632f2

17 files changed

Lines changed: 221 additions & 138 deletions

File tree

‎apps/user_ldap/lib/Access.php‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -631,7 +631,7 @@ public function mapAndAnnounceIfApplicable(
631631
* gives back the user names as they are used ownClod internally
632632
*
633633
* @param array $ldapUsers as returned by fetchList()
634-
* @return array an array with the user names to use in Nextcloud
634+
* @return array<int,string> an array with the user names to use in Nextcloud
635635
*
636636
* gives back the user names as they are used ownClod internally
637637
* @throws \Exception
@@ -644,7 +644,7 @@ public function nextcloudUserNames($ldapUsers) {
644644
* gives back the group names as they are used ownClod internally
645645
*
646646
* @param array $ldapGroups as returned by fetchList()
647-
* @return array an array with the group names to use in Nextcloud
647+
* @return array<int,string> an array with the group names to use in Nextcloud
648648
*
649649
* gives back the group names as they are used ownClod internally
650650
* @throws \Exception
@@ -655,6 +655,7 @@ public function nextcloudGroupNames($ldapGroups) {
655655

656656
/**
657657
* @param array[] $ldapObjects as returned by fetchList()
658+
* @return array<int,string>
658659
* @throws \Exception
659660
*/
660661
private function ldap2NextcloudNames(array $ldapObjects, bool $isUsers): array {

‎apps/user_ldap/lib/Group_LDAP.php‎

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -45,11 +45,11 @@
4545
namespace OCA\User_LDAP;
4646

4747
use Exception;
48+
use OC\ServerNotAvailableException;
4849
use OCP\Cache\CappedMemoryCache;
4950
use OCP\GroupInterface;
5051
use OCP\Group\Backend\IDeleteGroupBackend;
5152
use OCP\Group\Backend\IGetDisplayNameBackend;
52-
use OC\ServerNotAvailableException;
5353
use Psr\Log\LoggerInterface;
5454

5555
class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, IGetDisplayNameBackend, IDeleteGroupBackend {
@@ -466,7 +466,7 @@ private function prepareFilterForUsersHasGidNumber(string $groupDN, string $sear
466466
}
467467

468468
/**
469-
* @return array A list of users that have the given group as gid number
469+
* @return array<int,string> A list of users that have the given group as gid number
470470
* @throws ServerNotAvailableException
471471
*/
472472
public function getUsersInGidNumber(
@@ -591,6 +591,7 @@ private function prepareFilterForUsersInPrimaryGroup(string $groupDN, string $se
591591

592592
/**
593593
* @throws ServerNotAvailableException
594+
* @return array<int,string>
594595
*/
595596
public function getUsersInPrimaryGroup(
596597
string $groupDN,
@@ -840,7 +841,7 @@ private function getGroupsByMember(string $dn, array &$seen = []): array {
840841
* @param string $search
841842
* @param int $limit
842843
* @param int $offset
843-
* @return array with user ids
844+
* @return array<int,string> user ids
844845
* @throws Exception
845846
* @throws ServerNotAvailableException
846847
*/
@@ -909,7 +910,11 @@ public function usersInGroup($gid, $search = '', $limit = -1, $offset = 0) {
909910
if (empty($ldap_users)) {
910911
break;
911912
}
912-
$groupUsers[] = $this->access->dn2username($ldap_users[0]['dn'][0]);
913+
$uid = $this->access->dn2username($ldap_users[0]['dn'][0]);
914+
if (!$uid) {
915+
break;
916+
}
917+
$groupUsers[] = $uid;
913918
break;
914919
default:
915920
//we got DNs, check if we need to filter by search or we can give back all of them
@@ -1163,7 +1168,7 @@ protected function filterValidGroups(array $listOfGroups): array {
11631168
* Returns the supported actions as int to be
11641169
* compared with GroupInterface::CREATE_GROUP etc.
11651170
*/
1166-
public function implementsActions($actions) {
1171+
public function implementsActions($actions): bool {
11671172
return (bool)((GroupInterface::COUNT_USERS |
11681173
GroupInterface::DELETE_GROUP |
11691174
$this->groupPluginManager->getImplementedActions()) & $actions);

‎apps/user_ldap/lib/Group_Proxy.php‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -171,7 +171,7 @@ public function getUserGroups($uid) {
171171
/**
172172
* get a list of all users in a group
173173
*
174-
* @return string[] with user ids
174+
* @return array<int,string> user ids
175175
*/
176176
public function usersInGroup($gid, $search = '', $limit = -1, $offset = 0) {
177177
$this->setup();
@@ -334,4 +334,8 @@ public function getDisplayName(string $gid): string {
334334
public function getBackendName(): string {
335335
return 'LDAP';
336336
}
337+
338+
public function searchInGroup(string $gid, string $search = '', int $limit = -1, int $offset = 0): array {
339+
return $this->handleRequest($gid, 'searchInGroup', [$gid, $search, $limit, $offset]);
340+
}
337341
}

‎core/Command/User/ListCommand.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -74,7 +74,7 @@ protected function configure() {
7474
}
7575

7676
protected function execute(InputInterface $input, OutputInterface $output): int {
77-
$users = $this->userManager->search('', (int) $input->getOption('limit'), (int) $input->getOption('offset'));
77+
$users = $this->userManager->searchDisplayName('', (int) $input->getOption('limit'), (int) $input->getOption('offset'));
7878

7979
$this->writeArrayInOutputFormat($input, $output, $this->formatUsers($users, (bool)$input->getOption('info')));
8080
return 0;

‎lib/composer/composer/autoload_classmap.php‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -412,6 +412,7 @@
412412
'OCP\\Group\\Backend\\IIsAdminBackend' => $baseDir . '/lib/public/Group/Backend/IIsAdminBackend.php',
413413
'OCP\\Group\\Backend\\INamedBackend' => $baseDir . '/lib/public/Group/Backend/INamedBackend.php',
414414
'OCP\\Group\\Backend\\IRemoveFromGroupBackend' => $baseDir . '/lib/public/Group/Backend/IRemoveFromGroupBackend.php',
415+
'OCP\\Group\\Backend\\ISearchableGroupBackend' => $baseDir . '/lib/public/Group/Backend/ISearchableGroupBackend.php',
415416
'OCP\\Group\\Backend\\ISetDisplayNameBackend' => $baseDir . '/lib/public/Group/Backend/ISetDisplayNameBackend.php',
416417
'OCP\\Group\\Events\\BeforeGroupChangedEvent' => $baseDir . '/lib/public/Group/Events/BeforeGroupChangedEvent.php',
417418
'OCP\\Group\\Events\\BeforeGroupCreatedEvent' => $baseDir . '/lib/public/Group/Events/BeforeGroupCreatedEvent.php',

‎lib/composer/composer/autoload_static.php‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -445,6 +445,7 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2
445445
'OCP\\Group\\Backend\\IIsAdminBackend' => __DIR__ . '/../../..' . '/lib/public/Group/Backend/IIsAdminBackend.php',
446446
'OCP\\Group\\Backend\\INamedBackend' => __DIR__ . '/../../..' . '/lib/public/Group/Backend/INamedBackend.php',
447447
'OCP\\Group\\Backend\\IRemoveFromGroupBackend' => __DIR__ . '/../../..' . '/lib/public/Group/Backend/IRemoveFromGroupBackend.php',
448+
'OCP\\Group\\Backend\\ISearchableGroupBackend' => __DIR__ . '/../../..' . '/lib/public/Group/Backend/ISearchableGroupBackend.php',
448449
'OCP\\Group\\Backend\\ISetDisplayNameBackend' => __DIR__ . '/../../..' . '/lib/public/Group/Backend/ISetDisplayNameBackend.php',
449450
'OCP\\Group\\Events\\BeforeGroupChangedEvent' => __DIR__ . '/../../..' . '/lib/public/Group/Events/BeforeGroupChangedEvent.php',
450451
'OCP\\Group\\Events\\BeforeGroupCreatedEvent' => __DIR__ . '/../../..' . '/lib/public/Group/Events/BeforeGroupCreatedEvent.php',

‎lib/private/Group/Backend.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -126,7 +126,7 @@ public function groupExists($gid) {
126126
* @param string $search
127127
* @param int $limit
128128
* @param int $offset
129-
* @return array an array of user ids
129+
* @return array<int,string> an array of user ids
130130
*/
131131
public function usersInGroup($gid, $search = '', $limit = -1, $offset = 0) {
132132
return [];

‎lib/private/Group/Database.php‎

Lines changed: 24 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -40,9 +40,12 @@
4040
use OCP\Group\Backend\IGetDisplayNameBackend;
4141
use OCP\Group\Backend\IGroupDetailsBackend;
4242
use OCP\Group\Backend\IRemoveFromGroupBackend;
43+
use OCP\Group\Backend\ISearchableGroupBackend;
4344
use OCP\Group\Backend\ISetDisplayNameBackend;
4445
use OCP\Group\Backend\INamedBackend;
4546
use OCP\IDBConnection;
47+
use OCP\IUserManager;
48+
use OC\User\LazyUser;
4649

4750
/**
4851
* Class for group management in a SQL Database (e.g. MySQL, SQLite)
@@ -57,6 +60,7 @@ class Database extends ABackend implements
5760
IGroupDetailsBackend,
5861
IRemoveFromGroupBackend,
5962
ISetDisplayNameBackend,
63+
ISearchableGroupBackend,
6064
INamedBackend {
6165
/** @var string[] */
6266
private $groupCache = [];
@@ -324,29 +328,35 @@ public function groupExists($gid) {
324328
}
325329

326330
/**
327-
* get a list of all users in a group
331+
* Get a list of all users in a group
328332
* @param string $gid
329333
* @param string $search
330334
* @param int $limit
331335
* @param int $offset
332-
* @return array an array of user ids
336+
* @return array<int,string> an array of user ids
333337
*/
334-
public function usersInGroup($gid, $search = '', $limit = -1, $offset = 0) {
338+
public function usersInGroup($gid, $search = '', $limit = -1, $offset = 0): array {
339+
return array_values(array_map(fn ($user) => $user->getUid(), $this->searchInGroup($gid, $search, $limit, $offset)));
340+
}
341+
342+
public function searchInGroup(string $gid, string $search = '', int $limit = -1, int $offset = 0): array {
335343
$this->fixDI();
336344

337345
$query = $this->dbConn->getQueryBuilder();
338-
$query->select('g.uid')
339-
->from('group_user', 'g')
346+
$query->select('g.uid', 'u.displayname');
347+
348+
$query->from('group_user', 'g')
340349
->where($query->expr()->eq('gid', $query->createNamedParameter($gid)))
341350
->orderBy('g.uid', 'ASC');
342351

352+
$query->leftJoin('g', 'users', 'u', $query->expr()->eq('g.uid', 'u.uid'));
353+
343354
if ($search !== '') {
344-
$query->leftJoin('g', 'users', 'u', $query->expr()->eq('g.uid', 'u.uid'))
345-
->leftJoin('u', 'preferences', 'p', $query->expr()->andX(
346-
$query->expr()->eq('p.userid', 'u.uid'),
347-
$query->expr()->eq('p.appid', $query->expr()->literal('settings')),
348-
$query->expr()->eq('p.configkey', $query->expr()->literal('email')))
349-
)
355+
$query->leftJoin('u', 'preferences', 'p', $query->expr()->andX(
356+
$query->expr()->eq('p.userid', 'u.uid'),
357+
$query->expr()->eq('p.appid', $query->expr()->literal('settings')),
358+
$query->expr()->eq('p.configkey', $query->expr()->literal('email'))
359+
))
350360
// sqlite doesn't like re-using a single named parameter here
351361
->andWhere(
352362
$query->expr()->orX(
@@ -365,11 +375,12 @@ public function usersInGroup($gid, $search = '', $limit = -1, $offset = 0) {
365375
$query->setFirstResult($offset);
366376
}
367377

368-
$result = $query->execute();
378+
$result = $query->executeQuery();
369379

370380
$users = [];
381+
$userManager = \OCP\Server::get(IUserManager::class);
371382
while ($row = $result->fetch()) {
372-
$users[] = $row['uid'];
383+
$users[$row['uid']] = new LazyUser($row['uid'], $userManager, $row['displayname'] ?? null);
373384
}
374385
$result->closeCursor();
375386

‎lib/private/Group/Group.php‎

Lines changed: 21 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -33,14 +33,16 @@
3333
namespace OC\Group;
3434

3535
use OC\Hooks\PublicEmitter;
36+
use OC\User\LazyUser;
37+
use OCP\GroupInterface;
3638
use OCP\Group\Backend\ICountDisabledInGroup;
3739
use OCP\Group\Backend\IGetDisplayNameBackend;
3840
use OCP\Group\Backend\IHideFromCollaborationBackend;
3941
use OCP\Group\Backend\INamedBackend;
42+
use OCP\Group\Backend\ISearchableGroupBackend;
4043
use OCP\Group\Backend\ISetDisplayNameBackend;
4144
use OCP\Group\Events\BeforeGroupChangedEvent;
4245
use OCP\Group\Events\GroupChangedEvent;
43-
use OCP\GroupInterface;
4446
use OCP\IGroup;
4547
use OCP\IUser;
4648
use OCP\IUserManager;
@@ -242,18 +244,23 @@ public function removeUser($user) {
242244
}
243245

244246
/**
245-
* search for users in the group by userid
246-
*
247-
* @param string $search
248-
* @param int $limit
249-
* @param int $offset
250-
* @return \OC\User\User[]
247+
* Search for users in the group by userid or display name
248+
* @return IUser[]
251249
*/
252-
public function searchUsers($search, $limit = null, $offset = null) {
250+
public function searchUsers(string $search, ?int $limit = null, ?int $offset = null): array {
253251
$users = [];
254252
foreach ($this->backends as $backend) {
255-
$userIds = $backend->usersInGroup($this->gid, $search, $limit, $offset);
256-
$users += $this->getVerifiedUsers($userIds);
253+
if ($backend instanceof ISearchableGroupBackend) {
254+
$users += $backend->searchInGroup($this->gid, $search, $limit ?? -1, $offset ?? 0);
255+
} else {
256+
$userIds = $backend->usersInGroup($this->gid, $search, $limit ?? -1, $offset ?? 0);
257+
$userManager = \OCP\Server::get(IUserManager::class);
258+
foreach ($userIds as $userId) {
259+
if (!isset($users[$userId])) {
260+
$users[$userId] = new LazyUser($userId, $userManager);
261+
}
262+
}
263+
}
257264
if (!is_null($limit) and $limit <= 0) {
258265
return $users;
259266
}
@@ -308,18 +315,11 @@ public function countDisabled() {
308315
* @param string $search
309316
* @param int $limit
310317
* @param int $offset
311-
* @return \OC\User\User[]
318+
* @return IUser[]
319+
* @deprecated 27.0.0 Use searchUsers instead (same implementation)
312320
*/
313321
public function searchDisplayName($search, $limit = null, $offset = null) {
314-
$users = [];
315-
foreach ($this->backends as $backend) {
316-
$userIds = $backend->usersInGroup($this->gid, $search, $limit, $offset);
317-
$users = $this->getVerifiedUsers($userIds);
318-
if (!is_null($limit) and $limit <= 0) {
319-
return array_values($users);
320-
}
321-
}
322-
return array_values($users);
322+
return $this->searchUsers($search, $limit, $offset);
323323
}
324324

325325
/**
@@ -375,10 +375,7 @@ public function delete() {
375375
* @param string[] $userIds an array containing user IDs
376376
* @return \OC\User\User[] an Array with the userId as Key and \OC\User\User as value
377377
*/
378-
private function getVerifiedUsers($userIds) {
379-
if (!is_array($userIds)) {
380-
return [];
381-
}
378+
private function getVerifiedUsers(array $userIds): array {
382379
$users = [];
383380
foreach ($userIds as $userId) {
384381
$user = $this->userManager->get($userId);

‎lib/private/User/LazyUser.php‎

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -30,16 +30,26 @@
3030
class LazyUser implements IUser {
3131
private ?IUser $user = null;
3232
private string $uid;
33+
private ?string $displayName;
3334
private IUserManager $userManager;
35+
private ?UserInterface $backend;
3436

35-
public function __construct(string $uid, IUserManager $userManager) {
37+
public function __construct(string $uid, IUserManager $userManager, ?string $displayName = null, ?UserInterface $backend = null) {
3638
$this->uid = $uid;
3739
$this->userManager = $userManager;
40+
$this->displayName = $displayName;
41+
$this->backend = $backend;
3842
}
3943

4044
private function getUser(): IUser {
4145
if ($this->user === null) {
42-
$this->user = $this->userManager->get($this->uid);
46+
if ($this->backend) {
47+
/** @var \OC\User\Manager $manager */
48+
$manager = $this->userManager;
49+
$this->user = $manager->getUserObject($this->uid, $this->backend);
50+
} else {
51+
$this->user = $this->userManager->get($this->uid);
52+
}
4353
}
4454
/** @var IUser */
4555
$user = $this->user;
@@ -51,6 +61,10 @@ public function getUID() {
5161
}
5262

5363
public function getDisplayName() {
64+
if ($this->displayName) {
65+
return $this->displayName;
66+
}
67+
5468
return $this->userManager->getDisplayName($this->uid) ?? $this->uid;
5569
}
5670

0 commit comments

Comments
 (0)