Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix performance problem when searching for groups in the groupfolder app #32201

Closed
wants to merge 12 commits into from
Prev Previous commit
Revert search modification and implement batch groupExists and
groupDetails method

Signed-off-by: Carl Schwan <carl@carlschwan.eu>
CarlSchwan committed Jun 13, 2022
commit e1ce8ee3c1b629cc767cf321bf5ef6d4c6ab674e
25 changes: 24 additions & 1 deletion apps/user_ldap/lib/Group_Proxy.php
Original file line number Diff line number Diff line change
@@ -31,10 +31,10 @@
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 @@ public function getGroupDetails(string $gid): array {
$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 @@ public function groupExists($gid) {
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
*
10 changes: 9 additions & 1 deletion apps/user_ldap/lib/LDAP.php
Original file line number Diff line number Diff line change
@@ -320,7 +320,15 @@ private function preFunctionCall(string $functionName, array $args): void {
$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);
}
51 changes: 2 additions & 49 deletions build/psalm-baseline.xml
Original file line number Diff line number Diff line change
@@ -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>
36 changes: 30 additions & 6 deletions lib/private/Group/Database.php
Original file line number Diff line number Diff line change
@@ -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 @@ public function getGroups($search = '', $limit = null, $offset = null) {
$this->fixDI();

$query = $this->dbConn->getQueryBuilder();
$query->select('gid')
$query->select('gid', 'displayname')
->from('groups')
->orderBy('gid', 'ASC');

@@ -289,6 +287,10 @@ public function getGroups($search = '', $limit = null, $offset = null) {

$groups = [];
while ($row = $result->fetch()) {
$this->groupCache[$row['gid']] = [
'displayname' => $row['displayname'],
'gid' => $row['gid'],
];
$groups[] = $row['gid'];
}
$result->closeCursor();
@@ -529,6 +531,28 @@ public function getGroupDetails(string $gid): array {
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;
2 changes: 1 addition & 1 deletion lib/private/Group/Group.php
Original file line number Diff line number Diff line change
@@ -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
96 changes: 79 additions & 17 deletions lib/private/Group/Manager.php
Original file line number Diff line number Diff line change
@@ -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 @@ public function get($gid) {
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)) {
@@ -202,6 +197,49 @@ protected function getGroupObject($gid, $displayName = null) {
return $this->cachedGroups[$gid];
}

/**
* @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);
Fixed Show fixed Hide fixed
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]);
Fixed Show fixed Hide fixed
$groups[$gid] = $this->cachedGroups[$gid];
}
return $groups;
}

/**
* @param string $gid
* @return bool
@@ -245,13 +283,9 @@ public function search($search, $limit = null, $offset = null) {
$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 @@ public function displayNamesInGroup($gid, $search = '', $limit = -1, $offset = 0
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) {
Loading