Browse Source

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>
tags/v27.0.0beta1
Carl Schwan 2 years ago
parent
commit
35dc223500
No account linked to committer's email address

+ 13
- 4
apps/user_ldap/lib/Group_LDAP.php View File

@@ -45,14 +45,15 @@
namespace OCA\User_LDAP;

use Exception;
use OC\ServerNotAvailableException;
use OCP\Cache\CappedMemoryCache;
use OCP\GroupInterface;
use OCP\Group\Backend\ABackend;
use OCP\Group\Backend\IDeleteGroupBackend;
use OCP\Group\Backend\IGetDisplayNameBackend;
use OC\ServerNotAvailableException;
use Psr\Log\LoggerInterface;

class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, IGetDisplayNameBackend, IDeleteGroupBackend {
class Group_LDAP extends ABackend implements GroupInterface, IGroupLDAP, IGetDisplayNameBackend, IDeleteGroupBackend {
protected bool $enabled = false;

/** @var CappedMemoryCache<string[]> $cachedGroupMembers array of users with gid as key */
@@ -63,6 +64,7 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I
protected CappedMemoryCache $cachedNestedGroups;
protected GroupPluginManager $groupPluginManager;
protected LoggerInterface $logger;
protected Access $access;

/**
* @var string $ldapGroupMemberAssocAttr contains the LDAP setting (in lower case) with the same name
@@ -70,7 +72,7 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I
protected string $ldapGroupMemberAssocAttr;

public function __construct(Access $access, GroupPluginManager $groupPluginManager) {
parent::__construct($access);
$this->access = $access;
$filter = $this->access->connection->ldapGroupFilter;
$gAssoc = $this->access->connection->ldapGroupMemberAssocAttr;
if (!empty($filter) && !empty($gAssoc)) {
@@ -1163,7 +1165,7 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I
* Returns the supported actions as int to be
* compared with GroupInterface::CREATE_GROUP etc.
*/
public function implementsActions($actions) {
public function implementsActions($actions): bool {
return (bool)((GroupInterface::COUNT_USERS |
GroupInterface::DELETE_GROUP |
$this->groupPluginManager->getImplementedActions()) & $actions);
@@ -1331,4 +1333,11 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I
$this->access->connection->writeToCache($cacheKey, $displayName);
return $displayName;
}

public function searchInGroup(string $gid, string $search = '', int $limit = -1, int $offset = 0): array {
if (!$this->enabled) {
return [];
}
return parent::searchInGroup($gid, $search, $limit, $offset);
}
}

+ 4
- 0
apps/user_ldap/lib/Group_Proxy.php View File

@@ -334,4 +334,8 @@ class Group_Proxy extends Proxy implements \OCP\GroupInterface, IGroupLDAP, IGet
public function getBackendName(): string {
return 'LDAP';
}

public function searchInGroup(string $gid, string $search = '', int $limit = -1, int $offset = 0): array {
return $this->handleRequest($gid, 'searchInGroup', [$gid, $search, $limit, $offset]);
}
}

+ 5
- 0
build/psalm-baseline-ocp.xml View File

@@ -5,6 +5,11 @@
<code>OC</code>
</UndefinedClass>
</file>
<file src="lib/public/Group/Backend/ABackend.php">
<UndefinedClass occurrences="2">
<code>OC</code>
</UndefinedClass>
</file>
<file src="lib/public/AppFramework/ApiController.php">
<NoInterfaceProperties occurrences="1">
<code>$this-&gt;request-&gt;server</code>

+ 52
- 0
lib/private/Group/Database.php View File

@@ -43,6 +43,9 @@ use OCP\Group\Backend\IRemoveFromGroupBackend;
use OCP\Group\Backend\ISetDisplayNameBackend;
use OCP\Group\Backend\INamedBackend;
use OCP\IDBConnection;
use OCP\IUserManager;
use OC\User\LazyUser;
use OC\User\DisplayNameCache;

/**
* Class for group management in a SQL Database (e.g. MySQL, SQLite)
@@ -376,6 +379,55 @@ class Database extends ABackend implements
return $users;
}

public function searchInGroup(string $gid, string $search = '', int $limit = -1, int $offset = 0): array {
$this->fixDI();

$query = $this->dbConn->getQueryBuilder();
$query->select('g.uid', 'u.displayname');

$query->from('group_user', 'g')
->where($query->expr()->eq('gid', $query->createNamedParameter($gid)))
->orderBy('g.uid', 'ASC');

$query->leftJoin('g', 'users', 'u', $query->expr()->eq('g.uid', 'u.uid'))

if ($search !== '') {
$query->leftJoin('u', 'preferences', 'p', $query->expr()->andX(
$query->expr()->eq('p.userid', 'u.uid'),
$query->expr()->eq('p.appid', $query->expr()->literal('settings')),
$query->expr()->eq('p.configkey', $query->expr()->literal('email'))
))
// sqlite doesn't like re-using a single named parameter here
->andWhere(
$query->expr()->orX(
$query->expr()->ilike('g.uid', $query->createNamedParameter('%' . $this->dbConn->escapeLikeParameter($search) . '%')),
$query->expr()->ilike('u.displayname', $query->createNamedParameter('%' . $this->dbConn->escapeLikeParameter($search) . '%')),
$query->expr()->ilike('p.configvalue', $query->createNamedParameter('%' . $this->dbConn->escapeLikeParameter($search) . '%'))
)
)
->orderBy('u.uid_lower', 'ASC');
}

if ($limit !== -1) {
$query->setMaxResults($limit);
}
if ($offset !== 0) {
$query->setFirstResult($offset);
}

$result = $query->executeQuery();

$users = [];
$userManager = \OCP\Server::get(IUserManager::class);
$displayNameCache = \OCP\Server::get(DisplayNameCache::class);
while ($row = $result->fetch()) {
$users[$row['uid']] = new LazyUser($row['uid'], $displayNameCache, $userManager, $row['displayname'] ?? null);
}
$result->closeCursor();

return $users;
}

/**
* get the number of all users matching the search string in a group
* @param string $gid

+ 6
- 11
lib/private/Group/Group.php View File

@@ -242,18 +242,13 @@ class Group implements IGroup {
}

/**
* search for users in the group by userid
*
* @param string $search
* @param int $limit
* @param int $offset
* @return \OC\User\User[]
* Search for users in the group by userid or display name
* @return IUser[]
*/
public function searchUsers($search, $limit = null, $offset = null) {
public function searchUsers(string $search, ?int $limit = null, ?int $offset = null): array {
$users = [];
foreach ($this->backends as $backend) {
$userIds = $backend->usersInGroup($this->gid, $search, $limit, $offset);
$users += $this->getVerifiedUsers($userIds);
$users = array_merge($users, $backend->searchInGroup($this->gid, $search, $limit ?? -1, $offset ?? 0));
if (!is_null($limit) and $limit <= 0) {
return $users;
}
@@ -309,12 +304,12 @@ class Group implements IGroup {
* @param int $limit
* @param int $offset
* @return \OC\User\User[]
* @deprecated 25.0.0 Use searchUsers instead (same implementation)
*/
public function searchDisplayName($search, $limit = null, $offset = null) {
$users = [];
foreach ($this->backends as $backend) {
$userIds = $backend->usersInGroup($this->gid, $search, $limit, $offset);
$users = $this->getVerifiedUsers($userIds);
$users = array_merge($users, $backend->searchInGroup($this->gid, $search, $limit ?? -1, $offset ?? 0));
if (!is_null($limit) and $limit <= 0) {
return array_values($users);
}

+ 17
- 3
lib/private/User/LazyUser.php View File

@@ -30,16 +30,26 @@ use OCP\UserInterface;
class LazyUser implements IUser {
private ?IUser $user = null;
private string $uid;
private ?string $displayName;
private IUserManager $userManager;
private ?UserInterface $backend;

public function __construct(string $uid, IUserManager $userManager) {
public function __construct(string $uid, IUserManager $userManager, ?string $displayName = null, ?UserInterface $backend = null) {
$this->uid = $uid;
$this->userManager = $userManager;
$this->displayName = $displayName;
$this->backend = $backend;
}

private function getUser(): IUser {
if ($this->user === null) {
$this->user = $this->userManager->get($this->uid);
if ($this->backend) {
/** @var \OC\User\Manager $manager */
$manager = $this->userManager;
$this->user = $manager->getUserObject($this->uid, $this->backend);
} else {
$this->user = $this->userManager->get($this->uid);
}
}
/** @var IUser */
$user = $this->user;
@@ -51,7 +61,11 @@ class LazyUser implements IUser {
}

public function getDisplayName() {
return $this->userManager->getDisplayName($this->uid) ?? $this->uid;
if ($this->displayName) {
return $this->displayName;
}

return $this->userManager->getDisplayName($this->uid);
}

public function setDisplayName($displayName) {

+ 12
- 17
lib/private/User/Manager.php View File

@@ -202,7 +202,7 @@ class Manager extends PublicEmitter implements IUserManager {
* @param bool $cacheUser If false the newly created user object will not be cached
* @return \OC\User\User
*/
protected function getUserObject($uid, $backend, $cacheUser = true) {
public function getUserObject($uid, $backend, $cacheUser = true) {
if ($backend instanceof IGetRealUIDBackend) {
$uid = $backend->getRealUID($uid);
}
@@ -293,58 +293,53 @@ class Manager extends PublicEmitter implements IUserManager {
}

/**
* search by user id
* Search by user id
*
* @param string $pattern
* @param int $limit
* @param int $offset
* @return \OC\User\User[]
* @return IUser[]
* @deprecated since 25.0.0, use searchDisplayName instead
*/
public function search($pattern, $limit = null, $offset = null) {
$users = [];
$displayNameCache = \OCP\Server::get(DisplayNameCache::class);
foreach ($this->backends as $backend) {
$backendUsers = $backend->getUsers($pattern, $limit, $offset);
if (is_array($backendUsers)) {
foreach ($backendUsers as $uid) {
$users[$uid] = $this->getUserObject($uid, $backend);
$users[$uid] = new LazyUser($uid, $displayNameCache, $this, null, $backend);
}
}
}

uasort($users, function ($a, $b) {
/**
* @var \OC\User\User $a
* @var \OC\User\User $b
*/
uasort($users, function (IUser $a, IUser $b) {
return strcasecmp($a->getUID(), $b->getUID());
});
return $users;
}

/**
* search by displayName
* Search by displayName
*
* @param string $pattern
* @param int $limit
* @param int $offset
* @return \OC\User\User[]
* @return IUser[]
*/
public function searchDisplayName($pattern, $limit = null, $offset = null) {
$users = [];
$displayNameCache = \OCP\Server::get(DisplayNameCache::class);
foreach ($this->backends as $backend) {
$backendUsers = $backend->getDisplayNames($pattern, $limit, $offset);
if (is_array($backendUsers)) {
foreach ($backendUsers as $uid => $displayName) {
$users[] = $this->getUserObject($uid, $backend);
$users[] = new LazyUser($uid, $displayNameCache, $this, $displayName, $backend);
}
}
}

usort($users, function ($a, $b) {
/**
* @var \OC\User\User $a
* @var \OC\User\User $b
*/
usort($users, function (IUser $a, IUser $b) {
return strcasecmp($a->getDisplayName(), $b->getDisplayName());
});
return $users;

+ 15
- 0
lib/public/Group/Backend/ABackend.php View File

@@ -26,6 +26,10 @@ declare(strict_types=1);
namespace OCP\Group\Backend;

use OCP\GroupInterface;
use OCP\IUserManager;
use OCP\Server;
use OC\User\LazyUser;
use OC\User\DisplayNameCache;

/**
* @since 14.0.0
@@ -65,4 +69,15 @@ abstract class ABackend implements GroupInterface {

return (bool)($actions & $implements);
}

public function searchInGroup(string $gid, string $search = '', int $limit = -1, int $offset = 0): array {
// Default implementation for compatibility reasons
$displayNameCache = Server::get(DisplayNameCache::class);
$userManager = Server::get(IUserManager::class);
$users = [];
foreach ($this->usersInGroup($gid, $search, $limit, $offset) as $userId) {
$users[$userId] = new LazyUser($userId, $displayNameCache, $userManager);
}
return $users;
}
}

+ 23
- 1
lib/public/GroupInterface.php View File

@@ -106,13 +106,35 @@ interface GroupInterface {
public function groupExists($gid);

/**
* get a list of all users in a group
* @brief Get a list of user ids in a group matching the given search parameters.
*
* @param string $gid
* @param string $search
* @param int $limit
* @param int $offset
* @return array an array of user ids
* @since 4.5.0
* @deprecated 25.0.0 Use searchInGroup instead, for performance reasons
*/
public function usersInGroup($gid, $search = '', $limit = -1, $offset = 0);

/**
* @brief Get a list of users matching the given search parameters.
*
* Implementations of this method should return lazy evaluated user objects and
* preload if possible the display name.
*
* <code>
* $users = $groupBackend->searchInGroup('admin', 'John', 10, 0);
* </code>
*
* @param string $gid The group id of the user we want to search
* @param string $search The part of the display name or user id of the users we
* want to search. This can be empty to get all the users.
* @param int $limit The limit of results
* @param int $offset The offset of the results
* @return IUser[]
* @since 25.0.0
*/
public function searchInGroup(string $gid, string $search = '', int $limit = -1, int $offset = 0): array;
}

+ 1
- 1
lib/public/IGroup.php View File

@@ -99,7 +99,7 @@ interface IGroup {
* @return \OCP\IUser[]
* @since 8.0.0
*/
public function searchUsers($search, $limit = null, $offset = null);
public function searchUsers(string $search, ?int $limit = null, ?int $offset = null): array;

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

+ 12
- 12
tests/lib/Group/GroupTest.php View File

@@ -303,9 +303,9 @@ class GroupTest extends \Test\TestCase {
$group = new \OC\Group\Group('group1', [$backend], $this->dispatcher, $userManager);

$backend->expects($this->once())
->method('usersInGroup')
->method('searchInGroup')
->with('group1', '2')
->willReturn(['user2']);
->willReturn(['user2' => new \OC\User\User('user2', null, $this->dispatcher)]);

$users = $group->searchUsers('2');

@@ -325,13 +325,13 @@ class GroupTest extends \Test\TestCase {
$group = new \OC\Group\Group('group1', [$backend1, $backend2], $this->dispatcher, $userManager);

$backend1->expects($this->once())
->method('usersInGroup')
->method('searchInGroup')
->with('group1', '2')
->willReturn(['user2']);
->willReturn(['user2' => new \OC\User\User('user2', null, $this->dispatcher)]);
$backend2->expects($this->once())
->method('usersInGroup')
->method('searchInGroup')
->with('group1', '2')
->willReturn(['user2']);
->willReturn(['user2' => new \OC\User\User('user2', null, $this->dispatcher)]);

$users = $group->searchUsers('2');

@@ -348,9 +348,9 @@ class GroupTest extends \Test\TestCase {
$group = new \OC\Group\Group('group1', [$backend], $this->dispatcher, $userManager);

$backend->expects($this->once())
->method('usersInGroup')
->method('searchInGroup')
->with('group1', 'user', 1, 1)
->willReturn(['user2']);
->willReturn(['user2' => new \OC\User\User('user2', null, $this->dispatcher)]);

$users = $group->searchUsers('user', 1, 1);

@@ -370,13 +370,13 @@ class GroupTest extends \Test\TestCase {
$group = new \OC\Group\Group('group1', [$backend1, $backend2], $this->dispatcher, $userManager);

$backend1->expects($this->once())
->method('usersInGroup')
->method('searchInGroup')
->with('group1', 'user', 2, 1)
->willReturn(['user2']);
->willReturn(['user2' => new \OC\User\User('user2', null, $this->dispatcher)]);
$backend2->expects($this->once())
->method('usersInGroup')
->method('searchInGroup')
->with('group1', 'user', 2, 1)
->willReturn(['user1']);
->willReturn(['user1' => new \OC\User\User('user1', null, $this->dispatcher)]);

$users = $group->searchUsers('user', 2, 1);


+ 12
- 43
tests/lib/Group/ManagerTest.php View File

@@ -24,6 +24,7 @@
namespace Test\Group;

use OC\Group\Database;
use OC\User\User;
use OC\User\Manager;
use OCP\GroupInterface;
use OCP\ICacheFactory;
@@ -91,6 +92,7 @@ class ManagerTest extends TestCase {
'createGroup',
'addToGroup',
'removeFromGroup',
'searchInGroup',
])
->getMock();
$backend->expects($this->any())
@@ -724,7 +726,7 @@ class ManagerTest extends TestCase {

public function testDisplayNamesInGroupWithOneUserBackendAndSearchEmpty() {
/**
* @var \PHPUnit\Framework\MockObject\MockObject | \OC\Group\Backend $backend
* @var \PHPUnit\Framework\MockObject\MockObject|\OC\Group\Backend $backend
*/
$backend = $this->getTestBackend();
$backend->expects($this->exactly(1))
@@ -733,22 +735,11 @@ class ManagerTest extends TestCase {
->willReturn(true);

$backend->expects($this->once())
->method('usersInGroup')
->method('searchInGroup')
->with('testgroup', '', -1, 0)
->willReturn(['user2', 'user33']);
->willReturn([$this->getTestUser('user2'), $this->getTestUser('user33')]);

$this->userManager->expects($this->any())
->method('get')
->willReturnCallback(function ($uid) {
switch ($uid) {
case 'user1': return $this->getTestUser('user1');
case 'user2': return $this->getTestUser('user2');
case 'user3': return $this->getTestUser('user3');
case 'user33': return $this->getTestUser('user33');
default:
return null;
}
});
$this->userManager->expects($this->never())->method('get');

$manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache);
$manager->addBackend($backend);
@@ -772,22 +763,11 @@ class ManagerTest extends TestCase {
->willReturn(true);

$backend->expects($this->once())
->method('usersInGroup')
->method('searchInGroup')
->with('testgroup', '', 1, 0)
->willReturn(['user2']);
->willReturn([new User('user2', null, $this->dispatcher)]);

$this->userManager->expects($this->any())
->method('get')
->willReturnCallback(function ($uid) {
switch ($uid) {
case 'user1': return $this->getTestUser('user1');
case 'user2': return $this->getTestUser('user2');
case 'user3': return $this->getTestUser('user3');
case 'user33': return $this->getTestUser('user33');
default:
return null;
}
});
$this->userManager->expects($this->never())->method('get');

$manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache);
$manager->addBackend($backend);
@@ -811,22 +791,11 @@ class ManagerTest extends TestCase {
->willReturn(true);

$backend->expects($this->once())
->method('usersInGroup')
->method('searchInGroup')
->with('testgroup', '', 1, 1)
->willReturn(['user33']);
->willReturn([$this->getTestUser('user33')]);

$this->userManager->expects($this->any())
->method('get')
->willReturnCallback(function ($uid) {
switch ($uid) {
case 'user1': return $this->getTestUser('user1');
case 'user2': return $this->getTestUser('user2');
case 'user3': return $this->getTestUser('user3');
case 'user33': return $this->getTestUser('user33');
default:
return null;
}
});
$this->userManager->expects($this->never())->method('get');

$manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache);
$manager->addBackend($backend);

+ 38
- 8
tests/lib/Util/Group/Dummy.php View File

@@ -29,12 +29,18 @@

namespace Test\Util\Group;

use OC\Group\Backend;
use Test\Util\User\Dummy as DummyUser;
use OCP\Group\Backend\ABackend;
use OCP\Group\Backend\IDeleteGroupBackend;
use OCP\Group\Backend\IAddToGroupBackend;
use OCP\Group\Backend\IRemoveFromGroupBackend;
use OCP\Group\Backend\ICreateGroupBackend;
use OCP\Group\Backend\ICountUsersBackend;

/**
* dummy group backend, does not keep state, only for testing use
* Dummy group backend, does not keep state, only for testing use
*/
class Dummy extends Backend {
class Dummy extends ABackend implements ICreateGroupBackend, IDeleteGroupBackend, IAddToGroupBackend, IRemoveFromGroupBackend, ICountUsersBackend {
private $groups = [];
/**
* Try to create a new group
@@ -44,7 +50,7 @@ class Dummy extends Backend {
* Tries to create a new group. If the group name already exists, false will
* be returned.
*/
public function createGroup($gid) {
public function createGroup(string $gid): bool {
if (!isset($this->groups[$gid])) {
$this->groups[$gid] = [];
return true;
@@ -60,7 +66,7 @@ class Dummy extends Backend {
*
* Deletes a group and removes it from the group_user-table
*/
public function deleteGroup($gid) {
public function deleteGroup(string $gid): bool {
if (isset($this->groups[$gid])) {
unset($this->groups[$gid]);
return true;
@@ -93,7 +99,7 @@ class Dummy extends Backend {
*
* Adds a user to a group.
*/
public function addToGroup($uid, $gid) {
public function addToGroup(string $uid, string $gid): bool {
if (isset($this->groups[$gid])) {
if (array_search($uid, $this->groups[$gid]) === false) {
$this->groups[$gid][] = $uid;
@@ -114,7 +120,7 @@ class Dummy extends Backend {
*
* removes the user from a group.
*/
public function removeFromGroup($uid, $gid) {
public function removeFromGroup(string $uid, string $gid): bool {
if (isset($this->groups[$gid])) {
if (($index = array_search($uid, $this->groups[$gid])) !== false) {
unset($this->groups[$gid][$index]);
@@ -192,6 +198,25 @@ class Dummy extends Backend {
}
}

public function searchInGroup(string $gid, string $search = '', int $limit = -1, int $offset = 0): array {
if (isset($this->groups[$gid])) {
if (empty($search)) {
$length = $limit < 0 ? null : $limit;
$users = array_slice($this->groups[$gid], $offset, $length);
return array_map(fn ($user) => new DummyUser($user, ''));
}
$result = [];
foreach ($this->groups[$gid] as $user) {
if (stripos($user, $search) !== false) {
$result[] = new DummyUser($user, '');
}
}
return $result;
} else {
return [];
}
}

/**
* get the number of all users in a group
* @param string $gid
@@ -200,7 +225,7 @@ class Dummy extends Backend {
* @param int $offset
* @return int
*/
public function countUsersInGroup($gid, $search = '', $limit = -1, $offset = 0) {
public function countUsersInGroup(string $gid, string $search = ''): int {
if (isset($this->groups[$gid])) {
if (empty($search)) {
return count($this->groups[$gid]);
@@ -213,5 +238,10 @@ class Dummy extends Backend {
}
return $count;
}
return 0;
}

public function groupExists($gid) {
return isset($this->groups[$gid]);
}
}

Loading…
Cancel
Save