Skip to content

Commit e4e1cea

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 e4e1cea

File tree

12 files changed

+192
-144
lines changed

12 files changed

+192
-144
lines changed

‎apps/user_ldap/lib/Group_LDAP.php‎

Lines changed: 14 additions & 7 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)) {
@@ -1203,7 +1203,7 @@ protected function filterValidGroups(array $listOfGroups): array {
12031203
* Returns the supported actions as int to be
12041204
* compared with GroupInterface::CREATE_GROUP etc.
12051205
*/
1206-
public function implementsActions($actions) {
1206+
public function implementsActions($actions): bool {
12071207
return (bool)((GroupInterface::COUNT_USERS |
12081208
GroupInterface::DELETE_GROUP |
12091209
$this->groupPluginManager->getImplementedActions()) & $actions);
@@ -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
}

‎build/psalm-baseline.xml‎

Lines changed: 2 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -136,16 +136,6 @@
136136
</ParamNameMismatch>
137137
</file>
138138
<file src="apps/dav/lib/CalDAV/CalDavBackend.php">
139-
<InvalidArgument occurrences="8">
140-
<code>'\OCA\DAV\CalDAV\CalDavBackend::createCachedCalendarObject'</code>
141-
<code>'\OCA\DAV\CalDAV\CalDavBackend::createSubscription'</code>
142-
<code>'\OCA\DAV\CalDAV\CalDavBackend::deleteCachedCalendarObject'</code>
143-
<code>'\OCA\DAV\CalDAV\CalDavBackend::deleteSubscription'</code>
144-
<code>'\OCA\DAV\CalDAV\CalDavBackend::publishCalendar'</code>
145-
<code>'\OCA\DAV\CalDAV\CalDavBackend::updateCachedCalendarObject'</code>
146-
<code>'\OCA\DAV\CalDAV\CalDavBackend::updateShares'</code>
147-
<code>'\OCA\DAV\CalDAV\CalDavBackend::updateSubscription'</code>
148-
</InvalidArgument>
149139
<InvalidNullableReturnType occurrences="2">
150140
<code>array</code>
151141
<code>array</code>
@@ -161,16 +151,6 @@
161151
<RedundantCast occurrences="1">
162152
<code>(int)$calendarId</code>
163153
</RedundantCast>
164-
<TooManyArguments occurrences="8">
165-
<code>dispatch</code>
166-
<code>dispatch</code>
167-
<code>dispatch</code>
168-
<code>dispatch</code>
169-
<code>dispatch</code>
170-
<code>dispatch</code>
171-
<code>dispatch</code>
172-
<code>dispatch</code>
173-
</TooManyArguments>
174154
<UndefinedFunction occurrences="4">
175155
<code>Uri\split($principalUri)</code>
176156
<code>Uri\split($row['principaluri'])</code>
@@ -341,9 +321,6 @@
341321
</InvalidArgument>
342322
</file>
343323
<file src="apps/dav/lib/CardDAV/AddressBookImpl.php">
344-
<InvalidArgument occurrences="1">
345-
<code>$id</code>
346-
</InvalidArgument>
347324
<InvalidScalarArgument occurrences="2">
348325
<code>$this-&gt;getKey()</code>
349326
<code>$this-&gt;getKey()</code>
@@ -358,16 +335,6 @@
358335
<FalsableReturnStatement occurrences="1">
359336
<code>false</code>
360337
</FalsableReturnStatement>
361-
<InvalidArgument occurrences="3">
362-
<code>'\OCA\DAV\CardDAV\CardDavBackend::createCard'</code>
363-
<code>'\OCA\DAV\CardDAV\CardDavBackend::deleteCard'</code>
364-
<code>'\OCA\DAV\CardDAV\CardDavBackend::updateCard'</code>
365-
</InvalidArgument>
366-
<TooManyArguments occurrences="3">
367-
<code>dispatch</code>
368-
<code>dispatch</code>
369-
<code>dispatch</code>
370-
</TooManyArguments>
371338
<TypeDoesNotContainType occurrences="1">
372339
<code>$addressBooks[$row['id']][$readOnlyPropertyName] === 0</code>
373340
</TypeDoesNotContainType>
@@ -889,7 +856,6 @@
889856
</RedundantCondition>
890857
<TypeDoesNotContainType occurrences="2">
891858
<code>get_class($res) === 'OpenSSLAsymmetricKey'</code>
892-
<code>is_object($res)</code>
893859
</TypeDoesNotContainType>
894860
</file>
895861
<file src="apps/encryption/lib/Crypto/EncryptAll.php">
@@ -3366,16 +3332,13 @@
33663332
<code>$result</code>
33673333
<code>$this-&gt;copyFromStorage($sourceStorage, $sourceInternalPath . '/' . $file, $targetInternalPath . '/' . $file, false, $isRename)</code>
33683334
</InvalidOperand>
3369-
<InvalidReturnStatement occurrences="6">
3335+
<InvalidReturnStatement occurrences="4">
33703336
<code>$newUnencryptedSize</code>
33713337
<code>$result</code>
3372-
<code>$stat</code>
33733338
<code>$this-&gt;storage-&gt;file_get_contents($path)</code>
33743339
<code>$this-&gt;storage-&gt;filesize($path)</code>
3375-
<code>$this-&gt;storage-&gt;getLocalFile($path)</code>
33763340
</InvalidReturnStatement>
3377-
<InvalidReturnType occurrences="5">
3378-
<code>array</code>
3341+
<InvalidReturnType occurrences="3">
33793342
<code>bool</code>
33803343
<code>int</code>
33813344
<code>string</code>

‎lib/private/Group/Database.php‎

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,9 @@
4343
use OCP\Group\Backend\ISetDisplayNameBackend;
4444
use OCP\Group\Backend\INamedBackend;
4545
use OCP\IDBConnection;
46+
use OCP\IUserManager;
47+
use OC\User\LazyUser;
48+
use OC\User\DisplayNameCache;
4649

4750
/**
4851
* Class for group management in a SQL Database (e.g. MySQL, SQLite)
@@ -377,6 +380,58 @@ public function usersInGroup($gid, $search = '', $limit = -1, $offset = 0) {
377380
return $users;
378381
}
379382

383+
public function searchInGroup(string $gid, string $search = '', int $limit = -1, int $offset = 0): array {
384+
$this->fixDI();
385+
386+
$query = $this->dbConn->getQueryBuilder();
387+
if ($search !== '') {
388+
$query->select('g.uid', 'u.displayname');
389+
} else {
390+
$query->select('g.uid');
391+
}
392+
393+
$query->from('group_user', 'g')
394+
->where($query->expr()->eq('gid', $query->createNamedParameter($gid)))
395+
->orderBy('g.uid', 'ASC');
396+
397+
if ($search !== '') {
398+
$query->leftJoin('g', 'users', 'u', $query->expr()->eq('g.uid', 'u.uid'))
399+
->leftJoin('u', 'preferences', 'p', $query->expr()->andX(
400+
$query->expr()->eq('p.userid', 'u.uid'),
401+
$query->expr()->eq('p.appid', $query->expr()->literal('settings')),
402+
$query->expr()->eq('p.configkey', $query->expr()->literal('email')))
403+
)
404+
// sqlite doesn't like re-using a single named parameter here
405+
->andWhere(
406+
$query->expr()->orX(
407+
$query->expr()->ilike('g.uid', $query->createNamedParameter('%' . $this->dbConn->escapeLikeParameter($search) . '%')),
408+
$query->expr()->ilike('u.displayname', $query->createNamedParameter('%' . $this->dbConn->escapeLikeParameter($search) . '%')),
409+
$query->expr()->ilike('p.configvalue', $query->createNamedParameter('%' . $this->dbConn->escapeLikeParameter($search) . '%'))
410+
)
411+
)
412+
->orderBy('u.uid_lower', 'ASC');
413+
}
414+
415+
if ($limit !== -1) {
416+
$query->setMaxResults($limit);
417+
}
418+
if ($offset !== 0) {
419+
$query->setFirstResult($offset);
420+
}
421+
422+
$result = $query->executeQuery();
423+
424+
$users = [];
425+
$userManager = \OC::$server->get(IUserManager::class);
426+
$displayNameCache = \OC::$server->get(DisplayNameCache::class);
427+
while ($row = $result->fetch()) {
428+
$users[$row['uid']] = new LazyUser($row['uid'], $displayNameCache, $userManager, $row['displayname'] ?? null);
429+
}
430+
$result->closeCursor();
431+
432+
return $users;
433+
}
434+
380435
/**
381436
* get the number of all users matching the search string in a group
382437
* @param string $gid

‎lib/private/Group/Group.php‎

Lines changed: 6 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -238,18 +238,13 @@ 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) {
251-
$userIds = $backend->usersInGroup($this->gid, $search, $limit, $offset);
252-
$users += $this->getVerifiedUsers($userIds);
247+
$users = array_merge($users, $backend->searchInGroup($this->gid, $search, $limit ?? -1, $offset ?? 0));
253248
if (!is_null($limit) and $limit <= 0) {
254249
return $users;
255250
}
@@ -305,12 +300,12 @@ public function countDisabled() {
305300
* @param int $limit
306301
* @param int $offset
307302
* @return \OC\User\User[]
303+
* @depreacted 25.0.0 Use searchUsers instead (same implementation)
308304
*/
309305
public function searchDisplayName($search, $limit = null, $offset = null) {
310306
$users = [];
311307
foreach ($this->backends as $backend) {
312-
$userIds = $backend->usersInGroup($this->gid, $search, $limit, $offset);
313-
$users = $this->getVerifiedUsers($userIds);
308+
$users = array_merge($users, $backend->searchInGroup($this->gid, $search, $limit ?? -1, $offset ?? 0));
314309
if (!is_null($limit) and $limit <= 0) {
315310
return array_values($users);
316311
}

‎lib/private/User/LazyUser.php‎

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -31,17 +31,27 @@ class LazyUser implements IUser {
3131
private ?IUser $user = null;
3232
private DisplayNameCache $displayNameCache;
3333
private string $uid;
34+
private ?string $displayName;
3435
private IUserManager $userManager;
36+
private ?UserInterface $backend;
3537

36-
public function __construct(string $uid, DisplayNameCache $displayNameCache, IUserManager $userManager) {
38+
public function __construct(string $uid, DisplayNameCache $displayNameCache, IUserManager $userManager, ?string $displayName = null, ?UserInterface $backend = null) {
3739
$this->displayNameCache = $displayNameCache;
3840
$this->uid = $uid;
3941
$this->userManager = $userManager;
42+
$this->displayName = $displayName;
43+
$this->backend = $backend;
4044
}
4145

4246
private function getUser(): IUser {
4347
if ($this->user === null) {
44-
$this->user = $this->userManager->get($this->uid);
48+
if ($this->backend) {
49+
/** @var \OC\User\Manager $manager */
50+
$manager = $this->userManager;
51+
$this->user = $manager->getUserObject($this->uid, $this->backend);
52+
} else {
53+
$this->user = $this->userManager->get($this->uid);
54+
}
4555
}
4656
/** @var IUser */
4757
$user = $this->user;
@@ -53,6 +63,10 @@ public function getUID() {
5363
}
5464

5565
public function getDisplayName() {
66+
if ($this->displayName) {
67+
return $this->displayName;
68+
}
69+
5670
return $this->displayNameCache->getDisplayName($this->uid);
5771
}
5872

‎lib/private/User/Manager.php‎

Lines changed: 12 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -193,7 +193,7 @@ public function get($uid) {
193193
* @param bool $cacheUser If false the newly created user object will not be cached
194194
* @return \OC\User\User
195195
*/
196-
protected function getUserObject($uid, $backend, $cacheUser = true) {
196+
public function getUserObject($uid, $backend, $cacheUser = true) {
197197
if ($backend instanceof IGetRealUIDBackend) {
198198
$uid = $backend->getRealUID($uid);
199199
}
@@ -284,58 +284,53 @@ 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\Server::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;
313311
}
314312

315313
/**
316-
* search by displayName
314+
* Search by displayName
317315
*
318316
* @param string $pattern
319317
* @param int $limit
320318
* @param int $offset
321-
* @return \OC\User\User[]
319+
* @return IUser[]
322320
*/
323321
public function searchDisplayName($pattern, $limit = null, $offset = null) {
324322
$users = [];
323+
$displayNameCache = \OCP\Server::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;

0 commit comments

Comments
 (0)