Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optimize retrieving display name when searching for users in a group #32866

Merged
merged 8 commits into from
May 2, 2023
5 changes: 3 additions & 2 deletions apps/user_ldap/lib/Access.php
Original file line number Diff line number Diff line change
Expand Up @@ -631,7 +631,7 @@ public function mapAndAnnounceIfApplicable(
* gives back the user names as they are used ownClod internally
*
* @param array $ldapUsers as returned by fetchList()
* @return array an array with the user names to use in Nextcloud
* @return array<int,string> an array with the user names to use in Nextcloud
*
* gives back the user names as they are used ownClod internally
* @throws \Exception
Expand All @@ -644,7 +644,7 @@ public function nextcloudUserNames($ldapUsers) {
* gives back the group names as they are used ownClod internally
*
* @param array $ldapGroups as returned by fetchList()
* @return array an array with the group names to use in Nextcloud
* @return array<int,string> an array with the group names to use in Nextcloud
*
* gives back the group names as they are used ownClod internally
* @throws \Exception
Expand All @@ -655,6 +655,7 @@ public function nextcloudGroupNames($ldapGroups) {

/**
* @param array[] $ldapObjects as returned by fetchList()
* @return array<int,string>
* @throws \Exception
*/
private function ldap2NextcloudNames(array $ldapObjects, bool $isUsers): array {
Expand Down
15 changes: 10 additions & 5 deletions apps/user_ldap/lib/Group_LDAP.php
Original file line number Diff line number Diff line change
Expand Up @@ -45,11 +45,11 @@
namespace OCA\User_LDAP;

use Exception;
use OC\ServerNotAvailableException;
use OCP\Cache\CappedMemoryCache;
use OCP\GroupInterface;
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 {
Expand Down Expand Up @@ -466,7 +466,7 @@ private function prepareFilterForUsersHasGidNumber(string $groupDN, string $sear
}

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

/**
* @throws ServerNotAvailableException
* @return array<int,string>
*/
public function getUsersInPrimaryGroup(
string $groupDN,
Expand Down Expand Up @@ -840,7 +841,7 @@ private function getGroupsByMember(string $dn, array &$seen = []): array {
* @param string $search
* @param int $limit
* @param int $offset
* @return array with user ids
* @return array<int,string> user ids
* @throws Exception
* @throws ServerNotAvailableException
*/
Expand Down Expand Up @@ -909,7 +910,11 @@ public function usersInGroup($gid, $search = '', $limit = -1, $offset = 0) {
if (empty($ldap_users)) {
break;
}
$groupUsers[] = $this->access->dn2username($ldap_users[0]['dn'][0]);
$uid = $this->access->dn2username($ldap_users[0]['dn'][0]);
if (!$uid) {
break;
}
$groupUsers[] = $uid;
break;
default:
//we got DNs, check if we need to filter by search or we can give back all of them
Expand Down Expand Up @@ -1163,7 +1168,7 @@ protected function filterValidGroups(array $listOfGroups): array {
* 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);
Expand Down
6 changes: 5 additions & 1 deletion apps/user_ldap/lib/Group_Proxy.php
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,7 @@ public function getUserGroups($uid) {
/**
* get a list of all users in a group
*
* @return string[] with user ids
* @return array<int,string> user ids
*/
public function usersInGroup($gid, $search = '', $limit = -1, $offset = 0) {
$this->setup();
Expand Down Expand Up @@ -334,4 +334,8 @@ public function getDisplayName(string $gid): string {
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]);
}
}
2 changes: 1 addition & 1 deletion core/Command/User/ListCommand.php
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ protected function configure() {
}

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

$this->writeArrayInOutputFormat($input, $output, $this->formatUsers($users, (bool)$input->getOption('info')));
return 0;
Expand Down
1 change: 1 addition & 0 deletions lib/composer/composer/autoload_classmap.php
Original file line number Diff line number Diff line change
Expand Up @@ -412,6 +412,7 @@
'OCP\\Group\\Backend\\IIsAdminBackend' => $baseDir . '/lib/public/Group/Backend/IIsAdminBackend.php',
'OCP\\Group\\Backend\\INamedBackend' => $baseDir . '/lib/public/Group/Backend/INamedBackend.php',
'OCP\\Group\\Backend\\IRemoveFromGroupBackend' => $baseDir . '/lib/public/Group/Backend/IRemoveFromGroupBackend.php',
'OCP\\Group\\Backend\\ISearchableGroupBackend' => $baseDir . '/lib/public/Group/Backend/ISearchableGroupBackend.php',
'OCP\\Group\\Backend\\ISetDisplayNameBackend' => $baseDir . '/lib/public/Group/Backend/ISetDisplayNameBackend.php',
'OCP\\Group\\Events\\BeforeGroupChangedEvent' => $baseDir . '/lib/public/Group/Events/BeforeGroupChangedEvent.php',
'OCP\\Group\\Events\\BeforeGroupCreatedEvent' => $baseDir . '/lib/public/Group/Events/BeforeGroupCreatedEvent.php',
Expand Down
1 change: 1 addition & 0 deletions lib/composer/composer/autoload_static.php
Original file line number Diff line number Diff line change
Expand Up @@ -445,6 +445,7 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2
'OCP\\Group\\Backend\\IIsAdminBackend' => __DIR__ . '/../../..' . '/lib/public/Group/Backend/IIsAdminBackend.php',
'OCP\\Group\\Backend\\INamedBackend' => __DIR__ . '/../../..' . '/lib/public/Group/Backend/INamedBackend.php',
'OCP\\Group\\Backend\\IRemoveFromGroupBackend' => __DIR__ . '/../../..' . '/lib/public/Group/Backend/IRemoveFromGroupBackend.php',
'OCP\\Group\\Backend\\ISearchableGroupBackend' => __DIR__ . '/../../..' . '/lib/public/Group/Backend/ISearchableGroupBackend.php',
'OCP\\Group\\Backend\\ISetDisplayNameBackend' => __DIR__ . '/../../..' . '/lib/public/Group/Backend/ISetDisplayNameBackend.php',
'OCP\\Group\\Events\\BeforeGroupChangedEvent' => __DIR__ . '/../../..' . '/lib/public/Group/Events/BeforeGroupChangedEvent.php',
'OCP\\Group\\Events\\BeforeGroupCreatedEvent' => __DIR__ . '/../../..' . '/lib/public/Group/Events/BeforeGroupCreatedEvent.php',
Expand Down
2 changes: 1 addition & 1 deletion lib/private/Group/Backend.php
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ public function groupExists($gid) {
* @param string $search
* @param int $limit
* @param int $offset
* @return array an array of user ids
* @return array<int,string> an array of user ids
*/
public function usersInGroup($gid, $search = '', $limit = -1, $offset = 0) {
return [];
Expand Down
37 changes: 24 additions & 13 deletions lib/private/Group/Database.php
Original file line number Diff line number Diff line change
Expand Up @@ -40,9 +40,12 @@
use OCP\Group\Backend\IGetDisplayNameBackend;
use OCP\Group\Backend\IGroupDetailsBackend;
use OCP\Group\Backend\IRemoveFromGroupBackend;
use OCP\Group\Backend\ISearchableGroupBackend;
use OCP\Group\Backend\ISetDisplayNameBackend;
use OCP\Group\Backend\INamedBackend;
use OCP\IDBConnection;
use OCP\IUserManager;
use OC\User\LazyUser;

/**
* Class for group management in a SQL Database (e.g. MySQL, SQLite)
Expand All @@ -57,6 +60,7 @@ class Database extends ABackend implements
IGroupDetailsBackend,
IRemoveFromGroupBackend,
ISetDisplayNameBackend,
ISearchableGroupBackend,
INamedBackend {
/** @var string[] */
private $groupCache = [];
Expand Down Expand Up @@ -324,29 +328,35 @@ public function groupExists($gid) {
}

/**
* get a list of all users in a group
* Get a list of all users in a group
* @param string $gid
* @param string $search
* @param int $limit
* @param int $offset
* @return array an array of user ids
* @return array<int,string> an array of user ids
*/
public function usersInGroup($gid, $search = '', $limit = -1, $offset = 0) {
public function usersInGroup($gid, $search = '', $limit = -1, $offset = 0): array {
return array_values(array_map(fn ($user) => $user->getUid(), $this->searchInGroup($gid, $search, $limit, $offset)));
}

public function searchInGroup(string $gid, string $search = '', int $limit = -1, int $offset = 0): array {
come-nc marked this conversation as resolved.
Show resolved Hide resolved
$this->fixDI();

$query = $this->dbConn->getQueryBuilder();
$query->select('g.uid')
->from('group_user', 'g')
$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 !== '') {
Fixed Show fixed Hide fixed
$query->leftJoin('g', 'users', 'u', $query->expr()->eq('g.uid', 'u.uid'))
->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')))
)
$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(
Expand All @@ -365,11 +375,12 @@ public function usersInGroup($gid, $search = '', $limit = -1, $offset = 0) {
$query->setFirstResult($offset);
}

$result = $query->execute();
$result = $query->executeQuery();

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

Expand Down
45 changes: 21 additions & 24 deletions lib/private/Group/Group.php
Original file line number Diff line number Diff line change
Expand Up @@ -33,14 +33,16 @@
namespace OC\Group;

use OC\Hooks\PublicEmitter;
use OC\User\LazyUser;
use OCP\GroupInterface;
use OCP\Group\Backend\ICountDisabledInGroup;
use OCP\Group\Backend\IGetDisplayNameBackend;
use OCP\Group\Backend\IHideFromCollaborationBackend;
use OCP\Group\Backend\INamedBackend;
use OCP\Group\Backend\ISearchableGroupBackend;
use OCP\Group\Backend\ISetDisplayNameBackend;
use OCP\Group\Events\BeforeGroupChangedEvent;
use OCP\Group\Events\GroupChangedEvent;
use OCP\GroupInterface;
use OCP\IGroup;
use OCP\IUser;
use OCP\IUserManager;
Expand Down Expand Up @@ -242,18 +244,23 @@ public function removeUser($user) {
}

/**
* 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);
if ($backend instanceof ISearchableGroupBackend) {
$users += $backend->searchInGroup($this->gid, $search, $limit ?? -1, $offset ?? 0);
} else {
$userIds = $backend->usersInGroup($this->gid, $search, $limit ?? -1, $offset ?? 0);
$userManager = \OCP\Server::get(IUserManager::class);
foreach ($userIds as $userId) {
if (!isset($users[$userId])) {
$users[$userId] = new LazyUser($userId, $userManager);
}
}
}
if (!is_null($limit) and $limit <= 0) {
return $users;
}
Expand Down Expand Up @@ -308,18 +315,11 @@ public function countDisabled() {
* @param string $search
* @param int $limit
* @param int $offset
* @return \OC\User\User[]
* @return IUser[]
* @deprecated 27.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);
if (!is_null($limit) and $limit <= 0) {
return array_values($users);
}
}
return array_values($users);
return $this->searchUsers($search, $limit, $offset);
}

/**
Expand Down Expand Up @@ -375,10 +375,7 @@ public function delete() {
* @param string[] $userIds an array containing user IDs
* @return \OC\User\User[] an Array with the userId as Key and \OC\User\User as value
*/
private function getVerifiedUsers($userIds) {
if (!is_array($userIds)) {
return [];
}
private function getVerifiedUsers(array $userIds): array {
$users = [];
foreach ($userIds as $userId) {
$user = $this->userManager->get($userId);
Expand Down
18 changes: 16 additions & 2 deletions lib/private/User/LazyUser.php
Original file line number Diff line number Diff line change
Expand Up @@ -30,16 +30,26 @@
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;
Expand All @@ -51,6 +61,10 @@ public function getUID() {
}

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

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

Expand Down
Loading