Add batch methods in user backendsgroup-backend-batch-method

This allows for faster group search with significantly less DB traffic

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

10 files changed, 284 insertions, 43 deletions

diff --git a/apps/user_ldap/lib/Group_LDAP.php b/apps/user_ldap/lib/Group_LDAP.php
index 5072f9d3f74..f0926e6e562 100644
--- a/apps/user_ldap/lib/Group_LDAP.php
+++ b/apps/user_ldap/lib/Group_LDAP.php
@@ -47,12 +47,13 @@ use Exception;
 use OC;
 use OCP\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,10 +62,9 @@ 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
@@ -72,7 +72,7 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I
 	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 @@ 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);
diff --git a/apps/user_ldap/lib/Group_Proxy.php b/apps/user_ldap/lib/Group_Proxy.php
index ea2fcce679c..096d0e76a5e 100644
--- a/apps/user_ldap/lib/Group_Proxy.php
+++ b/apps/user_ldap/lib/Group_Proxy.php
@@ -30,7 +30,9 @@ namespace OCA\User_LDAP;
 
 use OCP\Group\Backend\IDeleteGroupBackend;
 use OCP\Group\Backend\IGetDisplayNameBackend;
+use OCP\Group\Backend\IGroupDetailsBackend;
 use OCP\Group\Backend\INamedBackend;
+use OCP\GroupInterface;
 
 class Group_Proxy extends Proxy implements \OCP\GroupInterface, IGroupLDAP, IGetDisplayNameBackend, INamedBackend, IDeleteGroupBackend {
 	private $backends = [];
@@ -230,6 +232,21 @@ class Group_Proxy extends Proxy implements \OCP\GroupInterface, IGroupLDAP, IGet
 	}
 
 	/**
+	 * {@inheritdoc}
+	 */
+	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
 	 *
 	 * @return string[] with group names
@@ -260,6 +277,20 @@ class Group_Proxy extends Proxy implements \OCP\GroupInterface, IGroupLDAP, IGet
 	}
 
 	/**
+	 * {@inheritdoc}
+	 */
+	public function groupsExists(array $gids): array {
+		$existingGroups = [];
+		foreach ($gids as $gid) {
+			$exists = $this->handleRequest($gid, 'groupExists', [$gid]);
+			if ($exists) {
+				$existingGroups[$gid] = $gid;
+			}
+		}
+		return $existingGroups;
+	}
+
+	/**
 	 * Check if backend implements actions
 	 *
 	 * @param int $actions bitwise-or'ed actions
diff --git a/build/psalm-baseline.xml b/build/psalm-baseline.xml
index dc2b8a8f10a..8601b90e19d 100644
--- a/build/psalm-baseline.xml
+++ b/build/psalm-baseline.xml
@@ -3163,16 +3163,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="7">
       <code>IGroup::class . '::postAddUser'</code>
diff --git a/lib/private/Group/Database.php b/lib/private/Group/Database.php
index 7b7ee41def9..fc32277b7a8 100644
--- a/lib/private/Group/Database.php
+++ b/lib/private/Group/Database.php
@@ -59,11 +59,9 @@ class Database extends ABackend implements
 			   ISetDisplayNameBackend,
 			   INamedBackend {
 
-	/** @var string[] */
+	/** @var array<string, array{gid: string, displayname: string}> */
 	private $groupCache = [];
-
-	/** @var IDBConnection */
-	private $dbConn;
+	private ?IDBConnection $dbConn;
 
 	/**
 	 * \OC\Group\Database constructor.
@@ -267,7 +265,7 @@ class Database extends ABackend implements
 		$this->fixDI();
 
 		$query = $this->dbConn->getQueryBuilder();
-		$query->select('gid')
+		$query->select('gid', 'displayname')
 			->from('groups')
 			->orderBy('gid', 'ASC');
 
@@ -286,6 +284,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();
@@ -325,6 +327,42 @@ class Database extends ABackend implements
 	}
 
 	/**
+	 * {@inheritdoc}
+	 */
+	public function groupsExists(array $gids): array {
+		$notFounsGids = [];
+		$existingGroups = [];
+
+		// In case the data is already locally accessible, not need to do SQL query
+		// or do a SQL query but with a smaller in clause
+		foreach ($gids as $gid) {
+			if (isset($this->groupCache[$gid])) {
+				$existingGroups[] = $gid;
+			} else {
+				$notFounsGids[] = $gid;
+			}
+		}
+
+		foreach (array_chunk($notFounsGids, 1000) as $chunk) {
+			$qb = $this->dbConn->getQueryBuilder();
+			$result = $qb->select('gid', 'displayname')
+				->from('groups')
+				->where($qb->expr()->in('gid', $qb->createNamedParameter($chunk, IQueryBuilder::PARAM_STR_ARRAY)))
+				->executeQuery();
+			while ($row = $result->fetch()) {
+				$this->groupCache[$row['gid']] = [
+					'displayname' => $row['displayname'],
+					'gid' => $row['gid'],
+				];
+				$existingGroups[] = $gid;
+			}
+			$result->closeCursor();
+		}
+
+		return $existingGroups;
+	}
+
+	/**
 	 * get a list of all users in a group
 	 * @param string $gid
 	 * @param string $search
@@ -474,6 +512,43 @@ class Database extends ABackend implements
 		return [];
 	}
 
+	/**
+	 * {@inheritdoc}
+	 */
+	public function getGroupsDetails(array $gids): array {
+		$notFounsGids = [];
+		$details = [];
+
+		// In case the data is already locally accessible, not need to do SQL query
+		// or do a SQL query but with a smaller in clause
+		foreach ($gids as $gid) {
+			if (isset($this->groupCache[$gid])) {
+				$details[$gid] = ['displayName' => $this->groupCache[$gid]['displayname']];
+			} else {
+				$notFounsGids[] = $gid;
+			}
+		}
+
+		foreach (array_chunk($notFounsGids, 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']] = ['displayName' => $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/Manager.php b/lib/private/Group/Manager.php
index 28f7a400b41..b5596ca471b 100644
--- a/lib/private/Group/Manager.php
+++ b/lib/private/Group/Manager.php
@@ -21,6 +21,7 @@
  * @author Vincent Petry <vincent@nextcloud.com>
  * @author Vinicius Cubas Brand <vinicius@eita.org.br>
  * @author voxsim "Simon Vocella"
+ * @author Carl Schwan <carl@carlschwan.eu>
  *
  * @license AGPL-3.0
  *
@@ -41,6 +42,7 @@ namespace OC\Group;
 
 use OC\Hooks\PublicEmitter;
 use OCP\EventDispatcher\IEventDispatcher;
+use OCP\Group\Backend\IGroupDetailsBackend;
 use OCP\GroupInterface;
 use OCP\IGroup;
 use OCP\IGroupManager;
@@ -73,10 +75,10 @@ class Manager extends PublicEmitter implements IGroupManager {
 	private $dispatcher;
 	private LoggerInterface $logger;
 
-	/** @var \OC\Group\Group[] */
+	/** @var array<string, IGroup> */
 	private $cachedGroups = [];
 
-	/** @var (string[])[] */
+	/** @var array<string, list<string>> */
 	private $cachedUserGroups = [];
 
 	/** @var \OC\SubAdmin */
@@ -180,7 +182,7 @@ class Manager extends PublicEmitter implements IGroupManager {
 			if ($backend->implementsActions(Backend::GROUP_DETAILS)) {
 				$groupData = $backend->getGroupDetails($gid);
 				if (is_array($groupData) && !empty($groupData)) {
-					// take the display name from the first backend that has a non-null one
+					// take the display name from the last backend that has a non-null one
 					if (is_null($displayName) && isset($groupData['displayName'])) {
 						$displayName = $groupData['displayName'];
 					}
@@ -193,11 +195,58 @@ class Manager extends PublicEmitter implements IGroupManager {
 		if (count($backends) === 0) {
 			return null;
 		}
+		/** @var GroupInterface[] $backends */
 		$this->cachedGroups[$gid] = new Group($gid, $backends, $this->dispatcher, $this->userManager, $this, $displayName);
 		return $this->cachedGroups[$gid];
 	}
 
 	/**
+	 * @brief Batch method to create group objects
+	 *
+	 * @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)) {
+				/** @var IGroupDetailsBackend $backend */
+				$groupDatas = $backend->getGroupsDetails($gids);
+				foreach ($groupDatas as $gid => $groupData) {
+					if (!empty($groupData)) {
+						// take the display name from the last 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
 	 */
@@ -239,13 +288,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);
diff --git a/lib/public/Group/Backend/ABackend.php b/lib/public/Group/Backend/ABackend.php
index 2d611c27b4f..539c208e971 100644
--- a/lib/public/Group/Backend/ABackend.php
+++ b/lib/public/Group/Backend/ABackend.php
@@ -6,6 +6,7 @@ declare(strict_types=1);
  * @copyright Copyright (c) 2018 Roeland Jago Douma <roeland@famdouma.nl>
  *
  * @author Roeland Jago Douma <roeland@famdouma.nl>
+ * @author Carl Schwan <carl@carlschwan.eu>
  *
  * @license GNU AGPL version 3 or any later version
  *
@@ -65,4 +66,33 @@ abstract class ABackend implements GroupInterface {
 
 		return (bool)($actions & $implements);
 	}
+
+	/**
+	 * @since 26.0.0
+	 */
+	public function groupsExists(array $gids): array {
+		$existingGroups = [];
+		foreach ($gids as $gid) {
+			$exists = $this->groupExists($gid);
+			if ($exists) {
+				$existingGroups[$gid] = $gid;
+			}
+		}
+		return $existingGroups;
+	}
+
+	/**
+	 * @since 26.0.0
+	 */
+	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/Group/Backend/IGroupDetailsBackend.php b/lib/public/Group/Backend/IGroupDetailsBackend.php
index 56241d5538b..c6084ea7984 100644
--- a/lib/public/Group/Backend/IGroupDetailsBackend.php
+++ b/lib/public/Group/Backend/IGroupDetailsBackend.php
@@ -6,6 +6,7 @@ declare(strict_types=1);
  * @copyright Copyright (c) 2018 Roeland Jago Douma <roeland@famdouma.nl>
  *
  * @author Roeland Jago Douma <roeland@famdouma.nl>
+ * @author Carl Schwan <carl@carlschwan.eu>
  *
  * @license GNU AGPL version 3 or any later version
  *
@@ -26,12 +27,34 @@ declare(strict_types=1);
 namespace OCP\Group\Backend;
 
 /**
+ * @brief Optional interface for group backends
  * @since 14.0.0
  */
 interface IGroupDetailsBackend {
 
 	/**
+	 * @brief Get additional details for a group, for example the display name.
+	 *
+	 * The array returned can be empty when no additional information is available
+	 * for the group.
+	 *
+	 * @return array{displayName?: string}
 	 * @since 14.0.0
 	 */
 	public function getGroupDetails(string $gid): array;
+
+
+	/**
+	 * @brief Batch method to get the group details of a list of groups
+	 *
+	 * The default implementation in ABackend will just call getGroupDetail in
+	 * a loop. But a GroupBackend implementation should provides a more optimized
+	 * override this method to provide a more optimized way to execute this operation.
+	 *
+	 * @throw \RuntimeException if called on a backend that doesn't implements IGroupDetailsBackend
+	 *
+	 * @return array<string, array{displayName: string}>
+	 * @since 26.0.0
+	 */
+	public function getGroupsDetails(array $gids): array;
 }
diff --git a/lib/public/GroupInterface.php b/lib/public/GroupInterface.php
index f1765afed96..29f03dd5965 100644
--- a/lib/public/GroupInterface.php
+++ b/lib/public/GroupInterface.php
@@ -87,7 +87,8 @@ interface GroupInterface {
 	public function getUserGroups($uid);
 
 	/**
-	 * get a list of all groups
+	 * @brief Get a list of all groups
+	 *
 	 * @param string $search
 	 * @param int $limit
 	 * @param int $offset
@@ -99,7 +100,8 @@ interface GroupInterface {
 	public function getGroups($search = '', $limit = -1, $offset = 0);
 
 	/**
-	 * check if a group exists
+	 * @brief Check if a group exists
+	 *
 	 * @param string $gid
 	 * @return bool
 	 * @since 4.5.0
@@ -107,7 +109,21 @@ interface GroupInterface {
 	public function groupExists($gid);
 
 	/**
-	 * get a list of all users in a group
+	 * @brief Batch method to check if a list of groups exists
+	 *
+	 * The default implementation in ABackend will just call groupExists in
+	 * a loop. But a GroupBackend implementation should provides a more optimized
+	 * override this method to provide a more optimized way to execute this operation.
+	 *
+	 * @param list<string> $gids
+	 * @return list<string> the list of group that exists
+	 * @since 25.0.0
+	 */
+	public function groupsExists(array $gids): array;
+
+	/**
+	 * Get a list of all users in a group
+	 *
 	 * @param string $gid
 	 * @param string $search
 	 * @param int $limit
diff --git a/tests/lib/Group/ManagerTest.php b/tests/lib/Group/ManagerTest.php
index d689fd4eb7a..c02a837366c 100644
--- a/tests/lib/Group/ManagerTest.php
+++ b/tests/lib/Group/ManagerTest.php
@@ -83,6 +83,7 @@ class ManagerTest extends TestCase {
 				'inGroup',
 				'getGroups',
 				'groupExists',
+				'groupsExists',
 				'usersInGroup',
 				'createGroup',
 				'addToGroup',
diff --git a/tests/lib/Util/Group/Dummy.php b/tests/lib/Util/Group/Dummy.php
index 3735a5e1167..2d86e45cfc1 100644
--- a/tests/lib/Util/Group/Dummy.php
+++ b/tests/lib/Util/Group/Dummy.php
@@ -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]);
 	}
 }