Skip to content

Commit 473c0b2

Browse files
CarlSchwanCarlSchwan
committed
Optimize retrieving display name when searching for users in a group
This is recurrent scenario that we are searching for users and then for each users we fetch the displayName. This is inefficient, so instead try to do one query to fetch everything (e.g. Database backend) or use the already existing DisplayNameCache helper. Signed-off-by: Carl Schwan <carl@carlschwan.eu>
1 parent 8809de1 commit 473c0b2

File tree

7 files changed

+71
-37
lines changed

7 files changed

+71
-37
lines changed

‎apps/user_ldap/lib/Group_LDAP.php‎

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -47,12 +47,13 @@
4747
use OC;
4848
use OC\Cache\CappedMemoryCache;
4949
use OC\ServerNotAvailableException;
50+
use OCP\Group\Backend\ABackend;
5051
use OCP\Group\Backend\IGetDisplayNameBackend;
5152
use OCP\Group\Backend\IDeleteGroupBackend;
5253
use OCP\GroupInterface;
5354
use Psr\Log\LoggerInterface;
5455

55-
class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, IGetDisplayNameBackend, IDeleteGroupBackend {
56+
class Group_LDAP extends ABackend implements GroupInterface, IGroupLDAP, IGetDisplayNameBackend, IDeleteGroupBackend {
5657
protected $enabled = false;
5758

5859
/** @var CappedMemoryCache<string[]> $cachedGroupMembers array of users with gid as key */
@@ -61,18 +62,17 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I
6162
protected CappedMemoryCache $cachedGroupsByMember;
6263
/** @var CappedMemoryCache<string[]> $cachedNestedGroups array of groups with gid (DN) as key */
6364
protected CappedMemoryCache $cachedNestedGroups;
64-
/** @var GroupPluginManager */
65-
protected $groupPluginManager;
66-
/** @var LoggerInterface */
67-
protected $logger;
65+
protected GroupPluginManager $groupPluginManager;
66+
protected LoggerInterface $logger;
67+
protected Access $access;
6868

6969
/**
7070
* @var string $ldapGroupMemberAssocAttr contains the LDAP setting (in lower case) with the same name
7171
*/
7272
protected $ldapGroupMemberAssocAttr;
7373

7474
public function __construct(Access $access, GroupPluginManager $groupPluginManager) {
75-
parent::__construct($access);
75+
$this->access = $access;
7676
$filter = $this->access->connection->ldapGroupFilter;
7777
$gAssoc = $this->access->connection->ldapGroupMemberAssocAttr;
7878
if (!empty($filter) && !empty($gAssoc)) {
@@ -1370,4 +1370,11 @@ public function getDisplayName(string $gid): string {
13701370

13711371
return '';
13721372
}
1373+
1374+
public function searchInGroup(string $gid, string $search = '', int $limit = -1, int $offset = 0): array {
1375+
if (!$this->enabled) {
1376+
return [];
1377+
}
1378+
return parent::searchInGroup($gid, $search, $limit, $offset);
1379+
}
13731380
}

‎apps/user_ldap/lib/Group_Proxy.php‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -306,4 +306,8 @@ public function getDisplayName(string $gid): string {
306306
public function getBackendName(): string {
307307
return 'LDAP';
308308
}
309+
310+
public function searchInGroup(string $gid, string $search = '', int $limit = -1, int $offset = 0): array {
311+
return $this->handleRequest($gid, 'searchInGroup', [$gid, $search, $limit, $offset]);
312+
}
309313
}

‎lib/private/Group/Group.php‎

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -238,14 +238,10 @@ public function removeUser($user) {
238238
}
239239

240240
/**
241-
* search for users in the group by userid
242-
*
243-
* @param string $search
244-
* @param int $limit
245-
* @param int $offset
246-
* @return \OC\User\User[]
241+
* Search for users in the group by userid or display name
242+
* @return IUser[]
247243
*/
248-
public function searchUsers($search, $limit = null, $offset = null) {
244+
public function searchUsers(string $search, ?int $limit = null, ?int $offset = null): array {
249245
$users = [];
250246
foreach ($this->backends as $backend) {
251247
$userIds = $backend->usersInGroup($this->gid, $search, $limit, $offset);

‎lib/private/User/Manager.php‎

Lines changed: 9 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -284,29 +284,27 @@ public function checkPasswordNoLogging($loginName, $password) {
284284
}
285285

286286
/**
287-
* search by user id
287+
* Search by user id
288288
*
289289
* @param string $pattern
290290
* @param int $limit
291291
* @param int $offset
292-
* @return \OC\User\User[]
292+
* @return IUser[]
293+
* @deprecated since 25.0.0, use searchDisplayName instead
293294
*/
294295
public function search($pattern, $limit = null, $offset = null) {
295296
$users = [];
297+
$displayNameCache = \OCP\Sserver::get(DisplayNameCache::class);
296298
foreach ($this->backends as $backend) {
297299
$backendUsers = $backend->getUsers($pattern, $limit, $offset);
298300
if (is_array($backendUsers)) {
299301
foreach ($backendUsers as $uid) {
300-
$users[$uid] = $this->getUserObject($uid, $backend);
302+
$users[$uid] = new LazyUser($uid, $displayNameCache, $this, null, $backend);
301303
}
302304
}
303305
}
304306

305-
uasort($users, function ($a, $b) {
306-
/**
307-
* @var \OC\User\User $a
308-
* @var \OC\User\User $b
309-
*/
307+
uasort($users, function (IUser $a, IUser $b) {
310308
return strcasecmp($a->getUID(), $b->getUID());
311309
});
312310
return $users;
@@ -322,20 +320,17 @@ public function search($pattern, $limit = null, $offset = null) {
322320
*/
323321
public function searchDisplayName($pattern, $limit = null, $offset = null) {
324322
$users = [];
323+
$displayNameCache = \OCP\Sserver::get(DisplayNameCache::class);
325324
foreach ($this->backends as $backend) {
326325
$backendUsers = $backend->getDisplayNames($pattern, $limit, $offset);
327326
if (is_array($backendUsers)) {
328327
foreach ($backendUsers as $uid => $displayName) {
329-
$users[] = $this->getUserObject($uid, $backend);
328+
$users[] = new LazyUser($uid, $displayNameCache, $this, $displayName, $backend);
330329
}
331330
}
332331
}
333332

334-
usort($users, function ($a, $b) {
335-
/**
336-
* @var \OC\User\User $a
337-
* @var \OC\User\User $b
338-
*/
333+
usort($users, function (IUser $a, IUser $b) {
339334
return strcasecmp($a->getDisplayName(), $b->getDisplayName());
340335
});
341336
return $users;

‎lib/public/GroupInterface.php‎

Lines changed: 23 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -107,13 +107,35 @@ public function getGroups($search = '', $limit = -1, $offset = 0);
107107
public function groupExists($gid);
108108

109109
/**
110-
* get a list of all users in a group
110+
* @brief Get a list of user ids in a group matching the given search parameters.
111+
*
111112
* @param string $gid
112113
* @param string $search
113114
* @param int $limit
114115
* @param int $offset
115116
* @return array an array of user ids
116117
* @since 4.5.0
118+
* @depreacted 25.0.0 Use searchInGroup instead, for performance reasons
117119
*/
118120
public function usersInGroup($gid, $search = '', $limit = -1, $offset = 0);
121+
122+
/**
123+
* @brief Get a list of users matching the given search parameters.
124+
*
125+
* Implementations of this method should return lazy evaluated user objects and
126+
* preload if possible the display name.
127+
*
128+
* <code>
129+
* $users = $groupBackend->searchInGroup('admin', 'John', 10, 0);
130+
* </code>
131+
*
132+
* @param string $gid The group id of the user we want to search
133+
* @param string $search The part of the display name or user id of the users we
134+
* want to search. This can be empty to get all the users.
135+
* @param int $limit The limit of results
136+
* @param int $offset The offset of the results
137+
* @return IUser[]
138+
* @since 25.0.0
139+
*/
140+
public function searchInGroup(string $gid, string $search = '', int $limit = -1, int $offset = 0): array;
119141
}

‎lib/public/IGroup.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -99,7 +99,7 @@ public function removeUser($user);
9999
* @return \OCP\IUser[]
100100
* @since 8.0.0
101101
*/
102-
public function searchUsers($search, $limit = null, $offset = null);
102+
public function searchUsers(string $search, ?int $limit = null, ?int $offset = null): array;
103103

104104
/**
105105
* returns the number of users matching the search string

‎tests/lib/Util/Group/Dummy.php‎

Lines changed: 18 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -29,12 +29,17 @@
2929

3030
namespace Test\Util\Group;
3131

32-
use OC\Group\Backend;
32+
use OCP\Group\Backend\ABackend;
33+
use OCP\Group\Backend\IDeleteGroupBackend;
34+
use OCP\Group\Backend\IAddToGroupBackend;
35+
use OCP\Group\Backend\IRemoveFromGroupBackend;
36+
use OCP\Group\Backend\ICreateGroupBackend;
37+
use OCP\Group\Backend\ICountUsersBackend;
3338

3439
/**
35-
* dummy group backend, does not keep state, only for testing use
40+
* Dummy group backend, does not keep state, only for testing use
3641
*/
37-
class Dummy extends Backend {
42+
class Dummy extends ABackend implements ICreateGroupBackend, IDeleteGroupBackend, IAddToGroupBackend, IRemoveFromGroupBackend, ICountUsersBackend {
3843
private $groups = [];
3944
/**
4045
* Try to create a new group
@@ -44,7 +49,7 @@ class Dummy extends Backend {
4449
* Tries to create a new group. If the group name already exists, false will
4550
* be returned.
4651
*/
47-
public function createGroup($gid) {
52+
public function createGroup(string $gid): bool {
4853
if (!isset($this->groups[$gid])) {
4954
$this->groups[$gid] = [];
5055
return true;
@@ -60,7 +65,7 @@ public function createGroup($gid) {
6065
*
6166
* Deletes a group and removes it from the group_user-table
6267
*/
63-
public function deleteGroup($gid) {
68+
public function deleteGroup(string $gid): bool {
6469
if (isset($this->groups[$gid])) {
6570
unset($this->groups[$gid]);
6671
return true;
@@ -93,7 +98,7 @@ public function inGroup($uid, $gid) {
9398
*
9499
* Adds a user to a group.
95100
*/
96-
public function addToGroup($uid, $gid) {
101+
public function addToGroup(string $uid, string $gid): bool {
97102
if (isset($this->groups[$gid])) {
98103
if (array_search($uid, $this->groups[$gid]) === false) {
99104
$this->groups[$gid][] = $uid;
@@ -114,7 +119,7 @@ public function addToGroup($uid, $gid) {
114119
*
115120
* removes the user from a group.
116121
*/
117-
public function removeFromGroup($uid, $gid) {
122+
public function removeFromGroup(string $uid, string $gid): bool {
118123
if (isset($this->groups[$gid])) {
119124
if (($index = array_search($uid, $this->groups[$gid])) !== false) {
120125
unset($this->groups[$gid][$index]);
@@ -200,7 +205,7 @@ public function usersInGroup($gid, $search = '', $limit = -1, $offset = 0) {
200205
* @param int $offset
201206
* @return int
202207
*/
203-
public function countUsersInGroup($gid, $search = '', $limit = -1, $offset = 0) {
208+
public function countUsersInGroup(string $gid, string $search = ''): int {
204209
if (isset($this->groups[$gid])) {
205210
if (empty($search)) {
206211
return count($this->groups[$gid]);
@@ -213,5 +218,10 @@ public function countUsersInGroup($gid, $search = '', $limit = -1, $offset = 0)
213218
}
214219
return $count;
215220
}
221+
return 0;
222+
}
223+
224+
public function groupExists($gid) {
225+
return isset($this->groups[$gid]);
216226
}
217227
}

0 commit comments

Comments
 (0)