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>
nextcloud · Jun 23, 2022 · b4f3295 · b4f3295
1 parent 8809de1
commit b4f3295
Show file tree

Hide file tree

Showing 14 changed files with 212 additions and 141 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)) {
@@ -1203,7 +1203,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);
@@ -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/build/psalm-baseline-ocp.xml b/build/psalm-baseline-ocp.xml
@@ -10,6 +10,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/App.php">
     <UndefinedClass occurrences="2">
       <code>\OC</code>

diff --git a/build/psalm-baseline.xml b/build/psalm-baseline.xml
@@ -136,16 +136,6 @@
     </ParamNameMismatch>
   </file>
   <file src="apps/dav/lib/CalDAV/CalDavBackend.php">
-    <InvalidArgument occurrences="8">
-      <code>'\OCA\DAV\CalDAV\CalDavBackend::createCachedCalendarObject'</code>
-      <code>'\OCA\DAV\CalDAV\CalDavBackend::createSubscription'</code>
-      <code>'\OCA\DAV\CalDAV\CalDavBackend::deleteCachedCalendarObject'</code>
-      <code>'\OCA\DAV\CalDAV\CalDavBackend::deleteSubscription'</code>
-      <code>'\OCA\DAV\CalDAV\CalDavBackend::publishCalendar'</code>
-      <code>'\OCA\DAV\CalDAV\CalDavBackend::updateCachedCalendarObject'</code>
-      <code>'\OCA\DAV\CalDAV\CalDavBackend::updateShares'</code>
-      <code>'\OCA\DAV\CalDAV\CalDavBackend::updateSubscription'</code>
-    </InvalidArgument>
     <InvalidNullableReturnType occurrences="2">
       <code>array</code>
       <code>array</code>
@@ -161,16 +151,6 @@
     <RedundantCast occurrences="1">
       <code>(int)$calendarId</code>
     </RedundantCast>
-    <TooManyArguments occurrences="8">
-      <code>dispatch</code>
-      <code>dispatch</code>
-      <code>dispatch</code>
-      <code>dispatch</code>
-      <code>dispatch</code>
-      <code>dispatch</code>
-      <code>dispatch</code>
-      <code>dispatch</code>
-    </TooManyArguments>
     <UndefinedFunction occurrences="4">
       <code>Uri\split($principalUri)</code>
       <code>Uri\split($row['principaluri'])</code>
@@ -341,9 +321,6 @@
     </InvalidArgument>
   </file>
   <file src="apps/dav/lib/CardDAV/AddressBookImpl.php">
-    <InvalidArgument occurrences="1">
-      <code>$id</code>
-    </InvalidArgument>
     <InvalidScalarArgument occurrences="2">
       <code>$this-&gt;getKey()</code>
       <code>$this-&gt;getKey()</code>
@@ -358,16 +335,6 @@
     <FalsableReturnStatement occurrences="1">
       <code>false</code>
     </FalsableReturnStatement>
-    <InvalidArgument occurrences="3">
-      <code>'\OCA\DAV\CardDAV\CardDavBackend::createCard'</code>
-      <code>'\OCA\DAV\CardDAV\CardDavBackend::deleteCard'</code>
-      <code>'\OCA\DAV\CardDAV\CardDavBackend::updateCard'</code>
-    </InvalidArgument>
-    <TooManyArguments occurrences="3">
-      <code>dispatch</code>
-      <code>dispatch</code>
-      <code>dispatch</code>
-    </TooManyArguments>
     <TypeDoesNotContainType occurrences="1">
       <code>$addressBooks[$row['id']][$readOnlyPropertyName] === 0</code>
     </TypeDoesNotContainType>
@@ -889,7 +856,6 @@
     </RedundantCondition>
     <TypeDoesNotContainType occurrences="2">
       <code>get_class($res) === 'OpenSSLAsymmetricKey'</code>
-      <code>is_object($res)</code>
     </TypeDoesNotContainType>
   </file>
   <file src="apps/encryption/lib/Crypto/EncryptAll.php">
@@ -3366,16 +3332,13 @@
       <code>$result</code>
       <code>$this-&gt;copyFromStorage($sourceStorage, $sourceInternalPath . '/' . $file, $targetInternalPath . '/' . $file, false, $isRename)</code>
     </InvalidOperand>
-    <InvalidReturnStatement occurrences="6">
+    <InvalidReturnStatement occurrences="4">
       <code>$newUnencryptedSize</code>
       <code>$result</code>
-      <code>$stat</code>
       <code>$this-&gt;storage-&gt;file_get_contents($path)</code>
       <code>$this-&gt;storage-&gt;filesize($path)</code>
-      <code>$this-&gt;storage-&gt;getLocalFile($path)</code>
     </InvalidReturnStatement>
-    <InvalidReturnType occurrences="5">
-      <code>array</code>
+    <InvalidReturnType occurrences="3">
       <code>bool</code>
       <code>int</code>
       <code>string</code>

diff --git a/lib/private/Group/Database.php b/lib/private/Group/Database.php
@@ -43,6 +43,9 @@
 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)
@@ -377,6 +380,55 @@ public function usersInGroup($gid, $search = '', $limit = -1, $offset = 0) {
 		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

diff --git a/lib/private/Group/Group.php b/lib/private/Group/Group.php
@@ -238,18 +238,13 @@ 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);
+			$users = array_merge($users, $backend->searchInGroup($this->gid, $search, $limit ?? -1, $offset ?? 0));
 			if (!is_null($limit) and $limit <= 0) {
 				return $users;
 			}
@@ -305,12 +300,12 @@ public function countDisabled() {
 	 * @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);
 			}

diff --git a/lib/private/User/LazyUser.php b/lib/private/User/LazyUser.php
@@ -31,17 +31,27 @@ class LazyUser implements IUser {
 	private ?IUser $user = null;
 	private DisplayNameCache $displayNameCache;
 	private string $uid;
+	private ?string $displayName;
 	private IUserManager $userManager;
+	private ?UserInterface $backend;
 
-	public function __construct(string $uid, DisplayNameCache $displayNameCache, IUserManager $userManager) {
+	public function __construct(string $uid, DisplayNameCache $displayNameCache, IUserManager $userManager, ?string $displayName = null, ?UserInterface $backend = null) {
 		$this->displayNameCache = $displayNameCache;
 		$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;
@@ -53,6 +63,10 @@ public function getUID() {
 	}
 
 	public function getDisplayName() {
+		if ($this->displayName) {
+			return $this->displayName;
+		}
+
 		return $this->displayNameCache->getDisplayName($this->uid);
 	}
 

diff --git a/lib/private/User/Manager.php b/lib/private/User/Manager.php
@@ -193,7 +193,7 @@ public function get($uid) {
 	 * @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);
 		}
@@ -284,58 +284,53 @@ 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\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;