Revert search modification and implement batch groupExists andfix/performance-search-groupfolder

groupDetails method

Signed-off-by: Carl Schwan <carl@carlschwan.eu>

8 files changed, 179 insertions, 76 deletions

diff --git a/apps/user_ldap/lib/Group_Proxy.php b/apps/user_ldap/lib/Group_Proxy.php
index 461cf3c3829..b4cf50a2f40 100644
--- a/apps/user_ldap/lib/Group_Proxy.php
+++ b/apps/user_ldap/lib/Group_Proxy.php
@@ -31,10 +31,10 @@ namespace OCA\User_LDAP;
 use OCP\Group\Backend\ABackend;
 use OCP\Group\Backend\IDeleteGroupBackend;
 use OCP\Group\Backend\IGetDisplayNameBackend;
+use OCP\Group\Backend\IGroupDetailsBackend;
 use OCP\Group\Backend\INamedBackend;
 use OCP\Group\Backend\IAddToGroupBackend;
 use OCP\Group\Backend\ICountUsersBackend;
-use OCP\Group\Backend\IGroupDetailsBackend;
 use OCP\Group\Backend\IRemoveFromGroupBackend;
 use OCP\GroupInterface;
 
@@ -233,6 +233,18 @@ class Group_Proxy extends Proxy implements GroupInterface, IGroupLDAP,
 			$gid, 'getGroupDetails', [$gid]);
 	}
 
+	public function getGroupsDetails(array $gids): array {
+		if (!($this instanceof IGroupDetailsBackend || $this->implementsActions(GroupInterface::GROUP_DETAILS))) {
+			throw new \Exception("Should not have been called");
+		}
+
+		$groupData = [];
+		foreach ($gids as $gid) {
+			$groupData[$gid] = $this->handleRequest($gid, 'getGroupDetails', [$gid]);
+		}
+		return $groupData;
+	}
+
 	/**
 	 * get a list of all groups
 	 *
@@ -263,6 +275,17 @@ class Group_Proxy extends Proxy implements GroupInterface, IGroupLDAP,
 		return $this->handleRequest($gid, 'groupExists', [$gid]);
 	}
 
+	public function groupsExists(array $gids): array {
+		$existingGroups = [];
+		foreach ($gids as $gid) {
+			$exits = $this->handleRequest($gid, 'groupExists', [$gid]);
+			if ($exits) {
+				$existingGroups[] = $gid;
+			}
+		}
+		return $existingGroups;
+	}
+
 	/**
 	 * Check if backend implements actions
 	 *
diff --git a/apps/user_ldap/lib/LDAP.php b/apps/user_ldap/lib/LDAP.php
index 3c579596941..35df0b17225 100644
--- a/apps/user_ldap/lib/LDAP.php
+++ b/apps/user_ldap/lib/LDAP.php
@@ -320,7 +320,15 @@ class LDAP implements ILDAPWrapper {
 		$this->curArgs = $args;
 
 		if ($this->dataCollector !== null) {
-			$args = array_map(fn ($item) => (!$this->isResource($item) ? $item : '(resource)'), $this->curArgs);
+			$args = array_map(function ($item) {
+				if ($this->isResource($item)) {
+					return '(resource)';
+				}
+				if (isset($item[0]['value']['cookie'])) {
+					$item[0]['value']['cookie'] = "";
+				}
+				return $item;
+			}, $this->curArgs);
 
 			$this->dataCollector->startLdapRequest($this->curFunc, $args);
 		}
diff --git a/build/psalm-baseline.xml b/build/psalm-baseline.xml
index bbb51189825..dba096a699a 100644
--- 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">
@@ -3361,16 +3327,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>
@@ -3471,16 +3434,6 @@
       <code>is_null($this-&gt;getContent())</code>
     </TypeDoesNotContainNull>
   </file>
-  <file src="lib/private/Group/Database.php">
-    <InvalidArrayOffset occurrences="1">
-      <code>$this-&gt;groupCache[$gid]['displayname']</code>
-    </InvalidArrayOffset>
-    <InvalidPropertyAssignmentValue occurrences="3">
-      <code>$this-&gt;groupCache</code>
-      <code>$this-&gt;groupCache</code>
-      <code>$this-&gt;groupCache</code>
-    </InvalidPropertyAssignmentValue>
-  </file>
   <file src="lib/private/Group/Group.php">
     <InvalidArgument occurrences="6">
       <code>IGroup::class . '::postAddUser'</code>
diff --git a/lib/private/Group/Database.php b/lib/private/Group/Database.php
index 903d1d154a1..bab6d27d9fb 100644
--- a/lib/private/Group/Database.php
+++ b/lib/private/Group/Database.php
@@ -62,11 +62,9 @@ class Database extends ABackend implements
 			   ISetDisplayNameBackend,
 			   INamedBackend {
 
-	/** @var string[] */
-	private $groupCache = [];
-
-	/** @var IDBConnection */
-	private $dbConn;
+	/** @var array<string, array{gid: string, displayname: string}> */
+	private array $groupCache = [];
+	private ?IDBConnection $dbConn;
 
 	/**
 	 * \OC\Group\Database constructor.
@@ -270,7 +268,7 @@ class Database extends ABackend implements
 		$this->fixDI();
 
 		$query = $this->dbConn->getQueryBuilder();
-		$query->select('gid')
+		$query->select('gid', 'displayname')
 			->from('groups')
 			->orderBy('gid', 'ASC');
 
@@ -289,6 +287,10 @@ class Database extends ABackend implements
 
 		$groups = [];
 		while ($row = $result->fetch()) {
+			$this->groupCache[$row['gid']] = [
+				'displayname' => $row['displayname'],
+				'gid' => $row['gid'],
+			];
 			$groups[] = $row['gid'];
 		}
 		$result->closeCursor();
@@ -529,6 +531,28 @@ class Database extends ABackend implements
 		return [];
 	}
 
+	public function getGroupsDetails(array $gids): array {
+		$details = [];
+		foreach (array_chunk($gids, 1000) as $chunk) {
+			$query = $this->dbConn->getQueryBuilder();
+			$query->select('gid', 'displayname')
+				->from('groups')
+				->where($query->expr()->in('gid', $query->createNamedParameter($chunk, IQueryBuilder::PARAM_STR_ARRAY)));
+
+			$result = $query->executeQuery();
+			while ($row = $result->fetch()) {
+				$details[$row['gid']] = $row['displayname'];
+				$this->groupCache[$row['gid']] = [
+					'displayname' => $row['displayname'],
+					'gid' => $row['gid'],
+				];
+			}
+			$result->closeCursor();
+		}
+
+		return $details;
+	}
+
 	public function setDisplayName(string $gid, string $displayName): bool {
 		if (!$this->groupExists($gid)) {
 			return false;
diff --git a/lib/private/Group/Group.php b/lib/private/Group/Group.php
index e535a09733b..6a2936f33fc 100644
--- a/lib/private/Group/Group.php
+++ b/lib/private/Group/Group.php
@@ -67,7 +67,7 @@ class Group implements IGroup {
 
 	/**
 	 * @param string $gid
-	 * @param GroupInterface[] $backends
+	 * @param list<GroupInterface> $backends
 	 * @param EventDispatcherInterface $dispatcher
 	 * @param IUserManager $userManager
 	 * @param PublicEmitter $emitter
diff --git a/lib/private/Group/Manager.php b/lib/private/Group/Manager.php
index 6a97f7fb520..4dd5a2be749 100644
--- a/lib/private/Group/Manager.php
+++ b/lib/private/Group/Manager.php
@@ -76,10 +76,10 @@ class Manager extends PublicEmitter implements IGroupManager {
 	private $dispatcher;
 	private LoggerInterface $logger;
 
-	/** @var \OC\Group\Group[] */
-	private $cachedGroups = [];
+	/** @var array<string, IGroup> */
+	private array $cachedGroups = [];
 
-	/** @var (string[])[] */
+	/** @var array<string, list<string> */
 	private $cachedUserGroups = [];
 
 	/** @var \OC\SubAdmin */
@@ -172,12 +172,7 @@ class Manager extends PublicEmitter implements IGroupManager {
 		return $this->getGroupObject($gid);
 	}
 
-	/**
-	 * @param string $gid
-	 * @param string $displayName
-	 * @return \OCP\IGroup|null
-	 */
-	protected function getGroupObject($gid, $displayName = null) {
+	protected function getGroupObject(string $gid, ?string $displayName = null): ?IGroup {
 		$backends = [];
 		foreach ($this->backends as $backend) {
 			if ($backend instanceof IGroupDetailsBackend || $backend->implementsActions(GroupInterface::GROUP_DETAILS)) {
@@ -203,6 +198,49 @@ class Manager extends PublicEmitter implements IGroupManager {
 	}
 
 	/**
+	 * @param list<string> $gids List of groupIds for which we want to create a IGroup object
+	 * @param array<string, string> $displayNames Array containing already know display name for a groupId
+	 * @return array<string, IGroup>
+	 */
+	protected function getGroupsObject(array $gids, array $displayNames = []): array {
+		$backends = [];
+		$groups = [];
+		foreach ($gids as $gid) {
+			$backends[$gid] = [];
+			if (!isset($displayNames[$gid])) {
+				$displayNames[$gid] = null;
+			}
+		}
+		foreach ($this->backends as $backend) {
+			if ($backend instanceof IGroupDetailsBackend || $backend->implementsActions(GroupInterface::GROUP_DETAILS)) {
+				$groupDatas = $backend->getGroupsDetails($gids);
+				foreach ($groupDatas as $gid => $groupData) {
+					if (!empty($groupData)) {
+						// take the display name from the first backend that has a non-null one
+						if (isset($groupData['displayName'])) {
+							$displayNames[$gid] = $groupData['displayName'];
+						}
+						$backends[$gid][] = $backend;
+					}
+				}
+			} else {
+				$existingGroups = $backend->groupsExists($gids);
+				foreach ($existingGroups as $group) {
+					$backends[$group][] = $backend;
+				}
+			}
+		}
+		foreach ($gids as $gid) {
+			if (count($backends[$gid]) === 0) {
+				 continue;
+			}
+			$this->cachedGroups[$gid] = new Group($gid, $backends[$gid], $this->dispatcher, $this->userManager, $this, $displayNames[$gid]);
+			$groups[$gid] = $this->cachedGroups[$gid];
+		}
+		return $groups;
+	}
+
+	/**
 	 * @param string $gid
 	 * @return bool
 	 */
@@ -245,13 +283,9 @@ class Manager extends PublicEmitter implements IGroupManager {
 		$groups = [];
 		foreach ($this->backends as $backend) {
 			$groupIds = $backend->getGroups($search, $limit, $offset);
-			foreach ($groupIds as $groupId) {
-				$aGroup = $this->get($groupId);
-				if ($aGroup instanceof IGroup) {
-					$groups[$groupId] = $aGroup;
-				} else {
-					$this->logger->debug('Group "' . $groupId . '" was returned by search but not found through direct access', ['app' => 'core']);
-				}
+			$newGroups = $this->getGroupsObject($groupIds);
+			foreach ($newGroups as $groupId => $group) {
+				$groups[$groupId] = $group;
 			}
 			if (!is_null($limit) and $limit <= 0) {
 				return array_values($groups);
@@ -374,7 +408,35 @@ class Manager extends PublicEmitter implements IGroupManager {
 		if (is_null($group)) {
 			return [];
 		}
-		$groupUsers = $group->searchUsers($search, $limit, $offset);
+		$search = trim($search);
+		$groupUsers = [];
+
+		if (!empty($search)) {
+			// only user backends have the capability to do a complex search for users
+			$searchOffset = 0;
+			$searchLimit = $limit * 100;
+			if ($limit === -1) {
+				$searchLimit = 500;
+			}
+
+			do {
+				$filteredUsers = $this->userManager->searchDisplayName($search, $searchLimit, $searchOffset);
+				foreach ($filteredUsers as $filteredUser) {
+					if ($group->inGroup($filteredUser)) {
+						$groupUsers[] = $filteredUser;
+					}
+				}
+				$searchOffset += $searchLimit;
+			} while (count($groupUsers) < $searchLimit + $offset && count($filteredUsers) >= $searchLimit);
+
+			if ($limit === -1) {
+				$groupUsers = array_slice($groupUsers, $offset);
+			} else {
+				$groupUsers = array_slice($groupUsers, $offset, $limit);
+			}
+		} else {
+			$groupUsers = $group->searchUsers('', $limit, $offset);
+		}
 
 		$matchingUsers = [];
 		foreach ($groupUsers as $groupUser) {
diff --git a/lib/public/Group/Backend/ABackend.php b/lib/public/Group/Backend/ABackend.php
index 222f1722bf3..1c7d62594e1 100644
--- a/lib/public/Group/Backend/ABackend.php
+++ b/lib/public/Group/Backend/ABackend.php
@@ -80,4 +80,27 @@ abstract class ABackend implements GroupInterface {
 		}
 		return $users;
 	}
+
+	public function groupsExists(array $gids): array {
+		$existingGroups = [];
+		foreach ($gids as $gid) {
+			$exists = $this->groupExists($gid);
+			if ($exists) {
+				$existringGroups[] = $gid;
+			}
+		}
+		return $existingGroups;
+	}
+
+	public function getGroupsDetails(array $gids): array {
+		if (!($this instanceof IGroupDetailsBackend || $this->implementsActions(GroupInterface::GROUP_DETAILS))) {
+			throw new \Exception("Should not have been called");
+		}
+		/** @var IGroupDetailsBackend $this */
+		$groupData = [];
+		foreach ($gids as $gid) {
+			$groupData[$gid] = $this->getGroupDetails($gid);
+		}
+		return $groupData;
+	}
 }
diff --git a/lib/public/GroupInterface.php b/lib/public/GroupInterface.php
index ce16807c988..70633aa7006 100644
--- a/lib/public/GroupInterface.php
+++ b/lib/public/GroupInterface.php
@@ -94,7 +94,7 @@ interface GroupInterface {
 	 * @return array an array of group names
 	 * @since 4.5.0
 	 *
-	 * Returns a list with all groups
+	 * @deprecated since 25.0.0 use getGroupObjects as it is more efficient
 	 */
 	public function getGroups($search = '', $limit = -1, $offset = 0);
 
@@ -107,6 +107,14 @@ interface GroupInterface {
 	public function groupExists($gid);
 
 	/**
+	 * Check if a list of groups exists
+	 * @param list<string> $gids
+	 * @return list<string> The list of group that exists
+	 * @since 25.0.0
+	 */
+	public function groupsExists(array $gids): array;
+
+	/**
 	 * @brief Get a list of user ids in a group matching the given search parameters.
 	 *
 	 * @param string $gid
@@ -138,4 +146,6 @@ interface GroupInterface {
 	 * @since 25.0.0
 	 */
 	public function searchInGroup(string $gid, string $search = '', int $limit = -1, int $offset = 0): array;
+
+	public function getGroupsDetails(array $gids): array;
 }