diff --git a/apps/user_ldap/lib/Group_LDAP.php b/apps/user_ldap/lib/Group_LDAP.php index 3a17ba70bb1a8..b3ff63d3b5c0f 100644 --- a/apps/user_ldap/lib/Group_LDAP.php +++ b/apps/user_ldap/lib/Group_LDAP.php @@ -48,11 +48,12 @@ use OC\ServerNotAvailableException; use OCP\Cache\CappedMemoryCache; use OCP\GroupInterface; +use OCP\Group\Backend\ABackend; use OCP\Group\Backend\IDeleteGroupBackend; use OCP\Group\Backend\IGetDisplayNameBackend; 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 bool $enabled = false; /** @var CappedMemoryCache $cachedGroupMembers array of users with gid as key */ @@ -63,6 +64,7 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I protected CappedMemoryCache $cachedNestedGroups; protected GroupPluginManager $groupPluginManager; protected LoggerInterface $logger; + protected Access $access; /** * @var string $ldapGroupMemberAssocAttr contains the LDAP setting (in lower case) with the same name @@ -70,7 +72,7 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I protected string $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)) { diff --git a/apps/user_ldap/lib/Group_Proxy.php b/apps/user_ldap/lib/Group_Proxy.php index 9d996b45f4330..114902ff9bad7 100644 --- a/apps/user_ldap/lib/Group_Proxy.php +++ b/apps/user_ldap/lib/Group_Proxy.php @@ -29,11 +29,14 @@ namespace OCA\User_LDAP; use OC\ServerNotAvailableException; +use OCP\Group\Backend\IBatchMethodsBackend; 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 { +class Group_Proxy extends Proxy implements \OCP\GroupInterface, IGroupLDAP, IGetDisplayNameBackend, INamedBackend, IDeleteGroupBackend, IBatchMethodsBackend { private $backends = []; private ?Group_LDAP $refBackend = null; private Helper $helper; @@ -256,6 +259,21 @@ public function getGroupDetails($gid) { $gid, 'getGroupDetails', [$gid]); } + /** + * {@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 * @@ -304,6 +322,16 @@ public function dn2GroupName(string $dn): string|false { return $this->handleRequest($id, 'dn2GroupName', [$dn]); } + /** + * {@inheritdoc} + */ + public function groupsExists(array $gids): array { + return array_values(array_filter( + $gids, + fn (string $gid): bool => $this->handleRequest($gid, 'groupExists', [$gid]), + )); + } + /** * Check if backend implements actions * diff --git a/apps/user_ldap/lib/Proxy.php b/apps/user_ldap/lib/Proxy.php index 8cdb1b1da6aaf..e2e33f3007bbf 100644 --- a/apps/user_ldap/lib/Proxy.php +++ b/apps/user_ldap/lib/Proxy.php @@ -130,7 +130,7 @@ protected function isSingleBackend(): bool { * @param string $method string, the method of the user backend that shall be called * @param array $parameters an array of parameters to be passed * @param bool $passOnWhen - * @return mixed, the result of the specified method + * @return mixed the result of the specified method */ protected function handleRequest($id, $method, $parameters, $passOnWhen = false) { if (!$this->isSingleBackend()) { diff --git a/build/psalm-baseline.xml b/build/psalm-baseline.xml index b97a2962c7690..0982147feff1d 100644 --- a/build/psalm-baseline.xml +++ b/build/psalm-baseline.xml @@ -2905,16 +2905,6 @@ getContent())]]> - - - groupCache[$gid]['displayname']]]> - - - groupCache]]> - groupCache]]> - groupCache]]> - - IEventListener diff --git a/lib/composer/composer/autoload_classmap.php b/lib/composer/composer/autoload_classmap.php index 9bdd0bdc55ff6..8fc2438866f22 100644 --- a/lib/composer/composer/autoload_classmap.php +++ b/lib/composer/composer/autoload_classmap.php @@ -421,6 +421,7 @@ 'OCP\\GroupInterface' => $baseDir . '/lib/public/GroupInterface.php', 'OCP\\Group\\Backend\\ABackend' => $baseDir . '/lib/public/Group/Backend/ABackend.php', 'OCP\\Group\\Backend\\IAddToGroupBackend' => $baseDir . '/lib/public/Group/Backend/IAddToGroupBackend.php', + 'OCP\\Group\\Backend\\IBatchMethodsBackend' => $baseDir . '/lib/public/Group/Backend/IBatchMethodsBackend.php', 'OCP\\Group\\Backend\\ICountDisabledInGroup' => $baseDir . '/lib/public/Group/Backend/ICountDisabledInGroup.php', 'OCP\\Group\\Backend\\ICountUsersBackend' => $baseDir . '/lib/public/Group/Backend/ICountUsersBackend.php', 'OCP\\Group\\Backend\\ICreateGroupBackend' => $baseDir . '/lib/public/Group/Backend/ICreateGroupBackend.php', diff --git a/lib/composer/composer/autoload_static.php b/lib/composer/composer/autoload_static.php index 2cdd9c8d3ca9c..fe306545fe322 100644 --- a/lib/composer/composer/autoload_static.php +++ b/lib/composer/composer/autoload_static.php @@ -454,6 +454,7 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2 'OCP\\GroupInterface' => __DIR__ . '/../../..' . '/lib/public/GroupInterface.php', 'OCP\\Group\\Backend\\ABackend' => __DIR__ . '/../../..' . '/lib/public/Group/Backend/ABackend.php', 'OCP\\Group\\Backend\\IAddToGroupBackend' => __DIR__ . '/../../..' . '/lib/public/Group/Backend/IAddToGroupBackend.php', + 'OCP\\Group\\Backend\\IBatchMethodsBackend' => __DIR__ . '/../../..' . '/lib/public/Group/Backend/IBatchMethodsBackend.php', 'OCP\\Group\\Backend\\ICountDisabledInGroup' => __DIR__ . '/../../..' . '/lib/public/Group/Backend/ICountDisabledInGroup.php', 'OCP\\Group\\Backend\\ICountUsersBackend' => __DIR__ . '/../../..' . '/lib/public/Group/Backend/ICountUsersBackend.php', 'OCP\\Group\\Backend\\ICreateGroupBackend' => __DIR__ . '/../../..' . '/lib/public/Group/Backend/ICreateGroupBackend.php', diff --git a/lib/private/Group/Database.php b/lib/private/Group/Database.php index ef5641d81370d..55792ce1dff19 100644 --- a/lib/private/Group/Database.php +++ b/lib/private/Group/Database.php @@ -33,6 +33,7 @@ use OCP\DB\QueryBuilder\IQueryBuilder; use OCP\Group\Backend\ABackend; use OCP\Group\Backend\IAddToGroupBackend; +use OCP\Group\Backend\IBatchMethodsBackend; use OCP\Group\Backend\ICountDisabledInGroup; use OCP\Group\Backend\ICountUsersBackend; use OCP\Group\Backend\ICreateGroupBackend; @@ -61,12 +62,11 @@ class Database extends ABackend implements IRemoveFromGroupBackend, ISetDisplayNameBackend, ISearchableGroupBackend, + IBatchMethodsBackend, INamedBackend { - /** @var string[] */ + /** @var array */ private $groupCache = []; - - /** @var IDBConnection */ - private $dbConn; + private ?IDBConnection $dbConn; /** * \OC\Group\Database constructor. @@ -270,7 +270,7 @@ public function getGroups(string $search = '', int $limit = -1, int $offset = 0) $this->fixDI(); $query = $this->dbConn->getQueryBuilder(); - $query->select('gid') + $query->select('gid', 'displayname') ->from('groups') ->orderBy('gid', 'ASC'); @@ -293,6 +293,10 @@ public function getGroups(string $search = '', int $limit = -1, int $offset = 0) $groups = []; while ($row = $result->fetch()) { + $this->groupCache[$row['gid']] = [ + 'displayname' => $row['displayname'], + 'gid' => $row['gid'], + ]; $groups[] = $row['gid']; } $result->closeCursor(); @@ -331,6 +335,43 @@ public function groupExists($gid) { return false; } + /** + * {@inheritdoc} + */ + public function groupsExists(array $gids): array { + $notFoundGids = []; + $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 { + $notFoundGids[] = $gid; + } + } + + $qb = $this->dbConn->getQueryBuilder(); + $qb->select('gid', 'displayname') + ->from('groups') + ->where($qb->expr()->in('gid', $qb->createParameter('ids'))); + foreach (array_chunk($notFoundGids, 1000) as $chunk) { + $qb->setParameter('ids', $chunk, IQueryBuilder::PARAM_STR_ARRAY); + $result = $qb->executeQuery(); + while ($row = $result->fetch()) { + $this->groupCache[(string)$row['gid']] = [ + 'displayname' => (string)$row['displayname'], + 'gid' => (string)$row['gid'], + ]; + $existingGroups[] = (string)$row['gid']; + } + $result->closeCursor(); + } + + return $existingGroups; + } + /** * Get a list of all users in a group * @param string $gid @@ -488,6 +529,43 @@ public function getGroupDetails(string $gid): array { return []; } + /** + * {@inheritdoc} + */ + public function getGroupsDetails(array $gids): array { + $notFoundGids = []; + $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 { + $notFoundGids[] = $gid; + } + } + + foreach (array_chunk($notFoundGids, 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[(string)$row['gid']] = ['displayName' => (string)$row['displayname']]; + $this->groupCache[(string)$row['gid']] = [ + 'displayname' => (string)$row['displayname'], + 'gid' => (string)$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 c43b5165a791a..47475121ea0cf 100644 --- a/lib/private/Group/Manager.php +++ b/lib/private/Group/Manager.php @@ -21,6 +21,7 @@ * @author Vincent Petry * @author Vinicius Cubas Brand * @author voxsim "Simon Vocella" + * @author Carl Schwan * * @license AGPL-3.0 * @@ -41,6 +42,8 @@ use OC\Hooks\PublicEmitter; use OCP\EventDispatcher\IEventDispatcher; +use OCP\Group\Backend\IBatchMethodsBackend; +use OCP\Group\Backend\IGroupDetailsBackend; use OCP\Group\Events\BeforeGroupCreatedEvent; use OCP\Group\Events\GroupCreatedEvent; use OCP\GroupInterface; @@ -74,10 +77,10 @@ class Manager extends PublicEmitter implements IGroupManager { private IEventDispatcher $dispatcher; private LoggerInterface $logger; - /** @var \OC\Group\Group[] */ + /** @var array */ private $cachedGroups = []; - /** @var (string[])[] */ + /** @var array> */ private $cachedUserGroups = []; /** @var \OC\SubAdmin */ @@ -185,7 +188,7 @@ protected function getGroupObject($gid, $displayName = null) { 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']; } @@ -198,10 +201,68 @@ protected function getGroupObject($gid, $displayName = null) { 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 $gids List of groupIds for which we want to create a IGroup object + * @param array $displayNames Array containing already know display name for a groupId + * @return array + */ + protected function getGroupsObjects(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 */ + if ($backend instanceof IBatchMethodsBackend) { + $groupDatas = $backend->getGroupsDetails($gids); + } else { + $groupDatas = []; + foreach ($gids as $gid) { + $groupDatas[$gid] = $backend->getGroupDetails($gid); + } + } + 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 { + if ($backend instanceof IBatchMethodsBackend) { + $existingGroups = $backend->groupsExists($gids); + } else { + $existingGroups = array_filter($gids, fn (string $gid): bool => $backend->groupExists($gid)); + } + 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 @@ -246,13 +307,9 @@ public function search(string $search, ?int $limit = null, ?int $offset = 0) { $groups = []; foreach ($this->backends as $backend) { $groupIds = $backend->getGroups($search, $limit ?? -1, $offset ?? 0); - 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->getGroupsObjects($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 7f5cf732335f3..274b98655e465 100644 --- a/lib/public/Group/Backend/ABackend.php +++ b/lib/public/Group/Backend/ABackend.php @@ -6,6 +6,7 @@ * @copyright Copyright (c) 2018 Roeland Jago Douma * * @author Roeland Jago Douma + * @author Carl Schwan * * @license GNU AGPL version 3 or any later version * @@ -30,7 +31,7 @@ /** * @since 14.0.0 */ -abstract class ABackend implements GroupInterface { +abstract class ABackend implements GroupInterface, IBatchMethodsBackend { /** * @deprecated 14.0.0 * @since 14.0.0 @@ -65,4 +66,29 @@ public function implementsActions($actions): bool { return (bool)($actions & $implements); } + + /** + * @since 28.0.0 + */ + public function groupsExists(array $gids): array { + return array_values(array_filter( + $gids, + fn (string $gid): bool => $this->groupExists($gid), + )); + } + + /** + * @since 28.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/IBatchMethodsBackend.php b/lib/public/Group/Backend/IBatchMethodsBackend.php new file mode 100644 index 0000000000000..2af00e4282518 --- /dev/null +++ b/lib/public/Group/Backend/IBatchMethodsBackend.php @@ -0,0 +1,61 @@ + + * + * @author Roeland Jago Douma + * @author Carl Schwan + * @author Côme Chilliet + * + * @license GNU AGPL version 3 or any later version + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as + * published by the Free Software Foundation, either version 3 of the + * License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + * + */ +namespace OCP\Group\Backend; + +/** + * @brief Optional interface for group backends + * @since 28.0.0 + */ +interface IBatchMethodsBackend { + /** + * @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 $gids + * @return list the list of group that exists + * @since 28.0.0 + */ + public function groupsExists(array $gids): array; + + /** + * @brief Batch method to get the group details of a list of groups + * + * The default implementation in ABackend will just call getGroupDetails in + * a loop. But a GroupBackend implementation should 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 + * @since 28.0.0 + */ + public function getGroupsDetails(array $gids): array; +} diff --git a/lib/public/Group/Backend/IGroupDetailsBackend.php b/lib/public/Group/Backend/IGroupDetailsBackend.php index 4852f97819592..851c10388e040 100644 --- a/lib/public/Group/Backend/IGroupDetailsBackend.php +++ b/lib/public/Group/Backend/IGroupDetailsBackend.php @@ -6,6 +6,7 @@ * @copyright Copyright (c) 2018 Roeland Jago Douma * * @author Roeland Jago Douma + * @author Carl Schwan * * @license GNU AGPL version 3 or any later version * @@ -26,10 +27,17 @@ 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; diff --git a/lib/public/GroupInterface.php b/lib/public/GroupInterface.php index a18d38df0022e..599a0eb2ff077 100644 --- a/lib/public/GroupInterface.php +++ b/lib/public/GroupInterface.php @@ -86,7 +86,8 @@ public function inGroup($uid, $gid); 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 @@ -98,7 +99,8 @@ public function getUserGroups($uid); public function getGroups(string $search = '', int $limit = -1, int $offset = 0); /** - * check if a group exists + * @brief Check if a group exists + * * @param string $gid * @return bool * @since 4.5.0 diff --git a/tests/lib/Group/ManagerTest.php b/tests/lib/Group/ManagerTest.php index 2887d14acaa17..06875f8653691 100644 --- a/tests/lib/Group/ManagerTest.php +++ b/tests/lib/Group/ManagerTest.php @@ -92,6 +92,7 @@ private function getTestBackend($implementedActions = null) { 'inGroup', 'getGroups', 'groupExists', + 'groupsExists', 'usersInGroup', 'createGroup', 'addToGroup', @@ -361,10 +362,12 @@ public function testSearchResultExistsButGroupDoesNot() { ->method('getGroups') ->with('1') ->willReturn(['group1']); + $backend->expects($this->never()) + ->method('groupExists'); $backend->expects($this->once()) - ->method('groupExists') - ->with('group1') - ->willReturn(false); + ->method('getGroupsDetails') + ->with(['group1']) + ->willReturn([]); /** @var \OC\User\Manager $userManager */ $userManager = $this->createMock(Manager::class);