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 <[email protected]>
nextcloud · Jun 13, 2022 · e05dfe3 · e05dfe3
1 parent 8809de1
commit e05dfe3
Show file tree

Hide file tree

Showing 9 changed files with 84 additions and 49 deletions.
diff --git a/apps/user_ldap/lib/Group_LDAP.php b/apps/user_ldap/lib/Group_LDAP.php
@@ -47,12 +47,13 @@
 use OC;
 use OC\Cache\CappedMemoryCache;
 use OC\ServerNotAvailableException;
+use OCP\Group\Backend\ABackend;
 use OCP\Group\Backend\IGetDisplayNameBackend;
 use OCP\Group\Backend\IDeleteGroupBackend;
 use OCP\GroupInterface;
 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 $enabled = false;
 
 	/** @var CappedMemoryCache<string[]> $cachedGroupMembers array of users with gid as key */
@@ -61,18 +62,17 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I
 	protected CappedMemoryCache $cachedGroupsByMember;
 	/** @var CappedMemoryCache<string[]> $cachedNestedGroups array of groups with gid (DN) as key */
 	protected CappedMemoryCache $cachedNestedGroups;
-	/** @var GroupPluginManager */
-	protected $groupPluginManager;
-	/** @var LoggerInterface */
-	protected $logger;
+	protected GroupPluginManager $groupPluginManager;
+	protected LoggerInterface $logger;
+	protected Access $access;
 
 	/**
 	 * @var string $ldapGroupMemberAssocAttr contains the LDAP setting (in lower case) with the same name
 	 */
 	protected $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)) {
@@ -1370,4 +1370,11 @@ public function getDisplayName(string $gid): string {
 
 		return '';
 	}
+
+	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);
+	}
 }
diff --git a/apps/user_ldap/lib/Group_Proxy.php b/apps/user_ldap/lib/Group_Proxy.php
@@ -306,4 +306,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]);
+	}
 }
diff --git a/lib/private/Group/Group.php b/lib/private/Group/Group.php
@@ -238,14 +238,10 @@ 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);

diff --git a/lib/private/User/Manager.php b/lib/private/User/Manager.php
@@ -284,29 +284,27 @@ public function checkPasswordNoLogging($loginName, $password) {
 	}
 
 	/**
-	 * 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\Sserver::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;
@@ -322,20 +320,17 @@ public function search($pattern, $limit = null, $offset = null) {
 	 */
 	public function searchDisplayName($pattern, $limit = null, $offset = null) {
 		$users = [];
+		$displayNameCache = \OCP\Sserver::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;

diff --git a/lib/public/GroupInterface.php b/lib/public/GroupInterface.php
@@ -107,13 +107,35 @@ public function getGroups($search = '', $limit = -1, $offset = 0);
 	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
+	 * @depreacted 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;
 }
diff --git a/lib/public/IGroup.php b/lib/public/IGroup.php
@@ -99,7 +99,7 @@ public function removeUser($user);
 	 * @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

diff --git a/tests/lib/Group/GroupTest.php b/tests/lib/Group/GroupTest.php
@@ -304,9 +304,9 @@ public function testSearchUsers() {
 		$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');
 
@@ -326,13 +326,13 @@ public function testSearchUsersMultipleBackends() {
 		$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');
 
@@ -349,9 +349,9 @@ public function testSearchUsersLimitAndOffset() {
 		$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);
 
@@ -371,13 +371,13 @@ public function testSearchUsersMultipleBackendsLimitAndOffset() {
 		$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);
 

diff --git a/tests/lib/Group/ManagerTest.php b/tests/lib/Group/ManagerTest.php
@@ -87,6 +87,7 @@ private function getTestBackend($implementedActions = null) {
 				'createGroup',
 				'addToGroup',
 				'removeFromGroup',
+				'searchInGroup',
 			])
 			->getMock();
 		$backend->expects($this->any())

diff --git a/tests/lib/Util/Group/Dummy.php b/tests/lib/Util/Group/Dummy.php
@@ -29,12 +29,17 @@
 
 namespace Test\Util\Group;
 
-use OC\Group\Backend;
+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 +49,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 +65,7 @@ public function createGroup($gid) {
 	 *
 	 * 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 +98,7 @@ public function inGroup($uid, $gid) {
 	 *
 	 * 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 +119,7 @@ public function addToGroup($uid, $gid) {
 	 *
 	 * 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]);
@@ -200,7 +205,7 @@ public function usersInGroup($gid, $search = '', $limit = -1, $offset = 0) {
 	 * @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 +218,10 @@ public function countUsersInGroup($gid, $search = '', $limit = -1, $offset = 0)
 			}
 			return $count;
 		}
+		return 0;
+	}
+
+	public function groupExists($gid) {
+		return isset($this->groups[$gid]);
 	}
 }