From a6b0c0a7bae98ee9e4899383337cd88f03601f5a Mon Sep 17 00:00:00 2001 From: Carl Schwan Date: Wed, 4 Feb 2026 12:14:51 +0100 Subject: [PATCH] perf(sharing): Avoid loading all shares from all users when unsharing First check which users have a shares and for which providers and then only load these shares. Avoid doing at most 5 SQL queries for each users a share was shared with if there are no shares. Signed-off-by: Carl Schwan --- apps/files_sharing/tests/CapabilitiesTest.php | 2 + .../tests/Settings/Admin/SharingTest.php | 2 + lib/private/Share20/DefaultShareProvider.php | 5 +- lib/private/Share20/Manager.php | 43 ++++++++- tests/lib/Share20/LegacyHooksTest.php | 14 +-- tests/lib/Share20/ManagerTest.php | 92 ++++++++++++++++++- 6 files changed, 143 insertions(+), 15 deletions(-) diff --git a/apps/files_sharing/tests/CapabilitiesTest.php b/apps/files_sharing/tests/CapabilitiesTest.php index 252459eaaae75..787a31c0626f2 100644 --- a/apps/files_sharing/tests/CapabilitiesTest.php +++ b/apps/files_sharing/tests/CapabilitiesTest.php @@ -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; @@ -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); diff --git a/apps/settings/tests/Settings/Admin/SharingTest.php b/apps/settings/tests/Settings/Admin/SharingTest.php index 4b9745ea5998d..0ab11e40ce687 100644 --- a/apps/settings/tests/Settings/Admin/SharingTest.php +++ b/apps/settings/tests/Settings/Admin/SharingTest.php @@ -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; diff --git a/lib/private/Share20/DefaultShareProvider.php b/lib/private/Share20/DefaultShareProvider.php index 46c2b94116217..fa2445524d2dd 100644 --- a/lib/private/Share20/DefaultShareProvider.php +++ b/lib/private/Share20/DefaultShareProvider.php @@ -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 $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']) diff --git a/lib/private/Share20/Manager.php b/lib/private/Share20/Manager.php index 7a1b7045b3643..61778f12d6aac 100644 --- a/lib/private/Share20/Manager.php +++ b/lib/private/Share20/Manager.php @@ -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; @@ -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; @@ -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 @@ -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') + ->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); diff --git a/tests/lib/Share20/LegacyHooksTest.php b/tests/lib/Share20/LegacyHooksTest.php index 2ce72b3fc1ccf..091bd5fb8dca7 100644 --- a/tests/lib/Share20/LegacyHooksTest.php +++ b/tests/lib/Share20/LegacyHooksTest.php @@ -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; @@ -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; @@ -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(); diff --git a/tests/lib/Share20/ManagerTest.php b/tests/lib/Share20/ManagerTest.php index 2afb5e6c3b3b7..f5c77fd17fa3d 100644 --- a/tests/lib/Share20/ManagerTest.php +++ b/tests/lib/Share20/ManagerTest.php @@ -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; @@ -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; @@ -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; @@ -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; @@ -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); @@ -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); @@ -157,6 +164,7 @@ private function createManager(IProviderFactory $factory): Manager { $this->shareDisabledChecker, $this->dateTimeZone, $this->appConfig, + $this->connection, ); } @@ -182,6 +190,7 @@ private function createManagerMock(): MockBuilder { $this->shareDisabledChecker, $this->dateTimeZone, $this->appConfig, + $this->connection, ]); } @@ -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]); } @@ -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]); } @@ -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]); } @@ -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]); }