Skip to content

Commit

Permalink
Add batch methods in user backends
Browse files Browse the repository at this point in the history 
This allows for faster group search with significantly less DB traffic

Signed-off-by: Carl Schwan <carl@carlschwan.eu>
  • Loading branch information
@CarlSchwan
CarlSchwan committed Jun 23, 2022
1 parent 596ead7 commit 49c2ffb
Show file tree
Hide file tree
Showing 8 changed files with 244 additions and 81 deletions.
14 changes: 7 additions & 7 deletions apps/user_ldap/lib/Group_LDAP.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 */
Expand All @@ -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)) {
Expand Down Expand Up @@ -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);
Expand Down
31 changes: 31 additions & 0 deletions apps/user_ldap/lib/Group_Proxy.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,9 @@

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 = [];
Expand Down Expand Up @@ -229,6 +231,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
*
Expand Down Expand Up @@ -259,6 +276,20 @@ public function groupExists($gid) {
return $this->handleRequest($gid, 'groupExists', [$gid]);
}

/**
* {@inheritdoc}
*/
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
*
Expand Down
59 changes: 2 additions & 57 deletions build/psalm-baseline.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand All @@ -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>
Expand Down Expand Up @@ -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>
Expand All @@ -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>
Expand Down Expand Up @@ -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">
Expand Down Expand Up @@ -1732,14 +1698,6 @@
<code>0</code>
</InvalidScalarArgument>
</file>
<file src="apps/updatenotification/lib/ResetTokenBackgroundJob.php">
<InvalidOperand occurrences="1">
<code>$this-&gt;config-&gt;getAppValue('core', 'updater.secret.created', $this-&gt;timeFactory-&gt;getTime())</code>
</InvalidOperand>
<InvalidScalarArgument occurrences="1">
<code>$this-&gt;timeFactory-&gt;getTime()</code>
</InvalidScalarArgument>
</file>
<file src="apps/updatenotification/lib/Settings/Admin.php">
<InvalidScalarArgument occurrences="1">
<code>$lastUpdateCheckTimestamp</code>
Expand Down Expand Up @@ -3366,16 +3324,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>
Expand Down Expand Up @@ -3476,16 +3431,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>
Expand Down
85 changes: 80 additions & 5 deletions lib/private/Group/Database.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -267,7 +265,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');

Expand All @@ -286,6 +284,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();
Expand Down Expand Up @@ -324,6 +326,42 @@ public function groupExists($gid) {
return false;
}

/**
* {@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($gids, 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
Expand Down Expand Up @@ -474,6 +512,43 @@ public function getGroupDetails(string $gid): array {
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;
Expand Down
Loading

0 comments on commit 49c2ffb

Please sign in to comment.