Skip to content

perf(sharing): Avoid loading all shares from all users when unsharing #58057

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.

Sign up for GitHub

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

Open
CarlSchwan wants to merge 1 commit into
base: master
Choose a base branch
from
Open

perf(sharing): Avoid loading all shares from all users when unsharing #58057

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions apps/files_sharing/tests/CapabilitiesTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
use OCP\IAppConfig;
use OCP\IConfig;
use OCP\IDateTimeZone;
use OCP\IDBConnection;
use OCP\IGroupManager;
use OCP\IUserManager;
use OCP\IUserSession;
Expand Down Expand Up @@ -92,6 +93,7 @@ private function getResults(array $map, array $typedMap = [], bool $federationEn
$this->createMock(ShareDisableChecker::class),
$this->createMock(IDateTimeZone::class),
$appConfig,
$this->createMock(IDBConnection::class),
);

$cap = new Capabilities($config, $appConfig, $shareManager, $appManager);
Expand Down
2 changes: 2 additions & 0 deletions apps/settings/tests/Settings/Admin/SharingTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,11 @@
use OCP\IL10N;
use OCP\IURLGenerator;
use OCP\Share\IManager;
use PHPUnit\Framework\Attributes\Group;
use PHPUnit\Framework\MockObject\MockObject;
use Test\TestCase;

#[Group(name: 'DB')]
class SharingTest extends TestCase {
private Sharing $admin;

Expand Down
5 changes: 2 additions & 3 deletions lib/private/Share20/DefaultShareProvider.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1033,11 +1033,10 @@ public function getShareByToken($token) {
/**
* Create a share object from a database row
*
* @param mixed[] $data
* @return \OCP\Share\IShare
* @param array<string, mixed> $data
* @throws InvalidShare
*/
private function createShare($data) {
private function createShare($data): IShare {
$share = new Share($this->rootFolder, $this->userManager);
$share->setId($data['id'])
->setShareType((int)$data['share_type'])
Expand Down
43 changes: 41 additions & 2 deletions lib/private/Share20/Manager.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
use OCA\Files_Sharing\AppInfo\Application;
use OCA\Files_Sharing\SharedStorage;
use OCA\ShareByMail\ShareByMailProvider;
use OCP\DB\QueryBuilder\IQueryBuilder;
use OCP\EventDispatcher\Event;
use OCP\EventDispatcher\IEventDispatcher;
use OCP\Files\File;
Expand All @@ -29,6 +30,7 @@
use OCP\IAppConfig;
use OCP\IConfig;
use OCP\IDateTimeZone;
use OCP\IDBConnection;
use OCP\IGroupManager;
use OCP\IL10N;
use OCP\IUser;
Expand Down Expand Up @@ -85,6 +87,7 @@ public function __construct(
private ShareDisableChecker $shareDisableChecker,
private IDateTimeZone $dateTimeZone,
private IAppConfig $appConfig,
private IDBConnection $connection,
) {
$this->l = $this->l10nFactory->get('lib');
// The constructor of LegacyHooks registers the listeners of share events
Expand Down Expand Up @@ -1033,14 +1036,50 @@ protected function promoteReshares(IShare $share): void {
IShare::TYPE_EMAIL,
];

foreach ($userIds as $userId) {
// Figure out which users has some shares with which providers
$qb = $this->connection->getQueryBuilder();
$qb->select('uid_initiator', 'share_type')
->from('share')
Copy link
Member

@provokateurin provokateurin Feb 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But not all providers save their data in this table, no?

Copy link
Member Author

@CarlSchwan CarlSchwan Feb 4, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At least talk, deck, circles and all the one in server do. I don't think there are more of them as the list is hardcoded in server

Copy link
Member

@provokateurin provokateurin Feb 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking of federated shares, because it uses two tables, one for incoming and one for outgoing. I'm not entirely sure if this would be covered here.

Copy link
Member Author

@CarlSchwan CarlSchwan Feb 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

federated shares getSharesBy is also just checking the share table only. Ultimately, I think we should make the manager fetch the share data and then let the providers create the IShare with the createShare method.

This would make sharing quite a bit lighter

Copy link
Member

@provokateurin provokateurin Feb 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but all share providers are slightly different and it's hard to unify all the logic.

->andWhere($qb->expr()->in('item_type', $qb->createNamedParameter(['file', 'folder'], IQueryBuilder::PARAM_STR_ARRAY)))
->andWhere($qb->expr()->in('share_type', $qb->createNamedParameter($shareTypes, IQueryBuilder::PARAM_INT_ARRAY)))
->andWhere(
$qb->expr()->orX(
$qb->expr()->in('uid_initiator', $qb->createNamedParameter($userIds, IQueryBuilder::PARAM_STR_ARRAY)),
// Special case for old shares created via the web UI
$qb->expr()->andX(
$qb->expr()->in('uid_owner', $qb->createNamedParameter($userIds, IQueryBuilder::PARAM_STR_ARRAY)),
$qb->expr()->isNull('uid_initiator')
)
)
);

if (!$node instanceof Folder) {
$qb->andWhere($qb->expr()->eq('file_source', $qb->createNamedParameter($node->getId(), IQueryBuilder::PARAM_INT)));
}

$qb->orderBy('id');

$cursor = $qb->executeQuery();
$rawShares = [];
while ($data = $cursor->fetch()) {
if (!isset($rawShares[$data['uid_initiator']])) {
$rawShares[$data['uid_initiator']] = [];
}
if (!in_array($data['share_type'], $rawShares[$data['uid_initiator']], true)) {
$rawShares[$data['uid_initiator']][] = $data['share_type'];
}
}
$cursor->closeCursor();

foreach ($rawShares as $userId => $shareTypes) {
foreach ($shareTypes as $shareType) {
try {
$provider = $this->factory->getProviderForType($shareType);
} catch (ProviderException $e) {
} catch (ProviderException) {
continue;
}


if ($node instanceof Folder) {
/* We need to get all shares by this user to get subshares */
$shares = $provider->getSharesBy($userId, $shareType, null, false, -1, 0);
Expand Down
14 changes: 5 additions & 9 deletions tests/lib/Share20/LegacyHooksTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@

use OC\EventDispatcher\EventDispatcher;
use OC\Share20\LegacyHooks;
use OC\Share20\Manager;
use OCP\Constants;
use OCP\EventDispatcher\IEventDispatcher;
use OCP\Files\Cache\ICacheEntry;
Expand All @@ -24,6 +23,7 @@
use OCP\Share\IManager as IShareManager;
use OCP\Share\IShare;
use OCP\Util;
use PHPUnit\Framework\Attributes\Group;
use Psr\Log\LoggerInterface;
use Test\TestCase;

Expand All @@ -40,15 +40,11 @@ public function pre() {
}
}

#[Group(name: 'DB')]
class LegacyHooksTest extends TestCase {
/** @var LegacyHooks */
private $hooks;

/** @var IEventDispatcher */
private $eventDispatcher;

/** @var Manager */
private $manager;
private LegacyHooks $hooks;
private IEventDispatcher $eventDispatcher;
private IShareManager $manager;

protected function setUp(): void {
parent::setUp();
Expand Down
92 changes: 91 additions & 1 deletion tests/lib/Share20/ManagerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,9 @@
use OC\Share20\Share;
use OC\Share20\ShareDisableChecker;
use OCP\Constants;
use OCP\DB\IResult;
use OCP\DB\QueryBuilder\IExpressionBuilder;
use OCP\DB\QueryBuilder\IQueryBuilder;
use OCP\EventDispatcher\Event;
use OCP\EventDispatcher\IEventDispatcher;
use OCP\Files\File;
Expand All @@ -33,6 +36,7 @@
use OCP\IAppConfig;
use OCP\IConfig;
use OCP\IDateTimeZone;
use OCP\IDBConnection;
use OCP\IGroup;
use OCP\IGroupManager;
use OCP\IL10N;
Expand All @@ -59,6 +63,7 @@
use OCP\Share\IShareProviderSupportsAllSharesInFolder;
use OCP\Util;
use PHPUnit\Framework\Attributes\DataProvider;
use PHPUnit\Framework\Attributes\Group;
use PHPUnit\Framework\MockObject\MockBuilder;
use PHPUnit\Framework\MockObject\MockObject;
use Psr\Log\LoggerInterface;
Expand All @@ -75,7 +80,7 @@ public function listener() {
*
* @package Test\Share20
*/
#[\PHPUnit\Framework\Attributes\Group('DB')]
#[Group(name: 'DB')]
class ManagerTest extends \Test\TestCase {
protected Manager $manager;
protected LoggerInterface&MockObject $logger;
Expand All @@ -97,6 +102,7 @@ class ManagerTest extends \Test\TestCase {
private DateTimeZone $timezone;
protected IDateTimeZone&MockObject $dateTimeZone;
protected IAppConfig&MockObject $appConfig;
protected IDBConnection&MockObject $connection;

protected function setUp(): void {
$this->logger = $this->createMock(LoggerInterface::class);
Expand All @@ -110,6 +116,7 @@ protected function setUp(): void {
$this->dispatcher = $this->createMock(IEventDispatcher::class);
$this->userSession = $this->createMock(IUserSession::class);
$this->knownUserService = $this->createMock(KnownUserService::class);
$this->connection = $this->createMock(IDBConnection::class);

$this->shareDisabledChecker = new ShareDisableChecker($this->config, $this->userManager, $this->groupManager);
$this->dateTimeZone = $this->createMock(IDateTimeZone::class);
Expand Down Expand Up @@ -157,6 +164,7 @@ private function createManager(IProviderFactory $factory): Manager {
$this->shareDisabledChecker,
$this->dateTimeZone,
$this->appConfig,
$this->connection,
);
}

Expand All @@ -182,6 +190,7 @@ private function createManagerMock(): MockBuilder {
$this->shareDisabledChecker,
$this->dateTimeZone,
$this->appConfig,
$this->connection,
]);
}

Expand Down Expand Up @@ -486,6 +495,26 @@ public function testPromoteReshareFile(): void {

$manager->expects($this->exactly(1))->method('updateShare')->with($reShare)->willReturn($reShare);

$qb = $this->createMock(IQueryBuilder::class);
$result = $this->createMock(IResult::class);
$qb->method('select')
->willReturn($qb);
$qb->method('from')
->willReturn($qb);
$qb->method('andWhere')
->willReturn($qb);
$qb->method('expr')
->willReturn($this->createMock(IExpressionBuilder::class));
$qb->method('executeQuery')
->willReturn($result);
$this->connection->method('getQueryBuilder')
->willReturn($qb);
$result->method('fetch')
->willReturnOnConsecutiveCalls(
['uid_initiator' => 'userB', 'share_type' => IShare::TYPE_USER],
false,
);

self::invokePrivate($manager, 'promoteReshares', [$share]);
}

Expand Down Expand Up @@ -546,6 +575,26 @@ public function testPromoteReshare(): void {
return $expected;
});

$qb = $this->createMock(IQueryBuilder::class);
$result = $this->createMock(IResult::class);
$qb->method('select')
->willReturn($qb);
$qb->method('from')
->willReturn($qb);
$qb->method('andWhere')
->willReturn($qb);
$qb->method('expr')
->willReturn($this->createMock(IExpressionBuilder::class));
$qb->method('executeQuery')
->willReturn($result);
$this->connection->method('getQueryBuilder')
->willReturn($qb);
$result->method('fetch')
->willReturnOnConsecutiveCalls(
['uid_initiator' => 'userB', 'share_type' => IShare::TYPE_USER],
false,
);

self::invokePrivate($manager, 'promoteReshares', [$share]);
}

Expand Down Expand Up @@ -574,6 +623,26 @@ public function testPromoteReshareWhenUserHasAnotherShare(): void {
/* No share is promoted because generalCreateChecks does not throw */
$manager->expects($this->never())->method('updateShare');

$qb = $this->createMock(IQueryBuilder::class);
$result = $this->createMock(IResult::class);
$qb->method('select')
->willReturn($qb);
$qb->method('from')
->willReturn($qb);
$qb->method('andWhere')
->willReturn($qb);
$qb->method('expr')
->willReturn($this->createMock(IExpressionBuilder::class));
$qb->method('executeQuery')
->willReturn($result);
$this->connection->method('getQueryBuilder')
->willReturn($qb);
$result->method('fetch')
->willReturnOnConsecutiveCalls(
['uid_initiator' => 'userB', 'share_type' => IShare::TYPE_USER],
false,
);

self::invokePrivate($manager, 'promoteReshares', [$share]);
}

Expand Down Expand Up @@ -641,6 +710,27 @@ public function testPromoteReshareOfUsersInGroupShare(): void {
return $expected;
});

$qb = $this->createMock(IQueryBuilder::class);
$result = $this->createMock(IResult::class);
$qb->method('select')
->willReturn($qb);
$qb->method('from')
->willReturn($qb);
$qb->method('andWhere')
->willReturn($qb);
$qb->method('expr')
->willReturn($this->createMock(IExpressionBuilder::class));
$qb->method('executeQuery')
->willReturn($result);
$this->connection->method('getQueryBuilder')
->willReturn($qb);
$result->method('fetch')
->willReturnOnConsecutiveCalls(
['uid_initiator' => 'userB', 'share_type' => IShare::TYPE_USER],
['uid_initiator' => 'userC', 'share_type' => IShare::TYPE_USER],
false,
);

self::invokePrivate($manager, 'promoteReshares', [$share]);
}

Expand Down
Loading