From 57533b80b98d692ecbf3279db3245ba7f23808e0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?John=20Molakvo=C3=A6?= <14975046+skjnldsv@users.noreply.github.com> Date: Tue, 13 Oct 2026 03:35:22 +0200 Subject: [PATCH 1/2] feat(sharing): look up the shares of many files at once MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit IManager::getSharesByNodes() and getSharedWithByNodes() return the shares of a list of nodes, the same as calling getSharesBy() and getSharedWith() for each node. Share providers implementing IShareProviderSupportsFileIds answer with one query for all the files, the default, mail and federated providers do. Other providers are still asked file by file. Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: John Molakvoæ <14975046+skjnldsv@users.noreply.github.com> --- .../lib/FederatedShareProvider.php | 37 +++++++- apps/sharebymail/lib/ShareByMailProvider.php | 36 ++++++- lib/composer/composer/autoload_classmap.php | 1 + lib/composer/composer/autoload_static.php | 1 + lib/private/Share20/DefaultShareProvider.php | 43 ++++++--- lib/private/Share20/Manager.php | 76 +++++++++++++++ lib/public/Share/IManager.php | 22 +++++ .../Share/IShareProviderSupportsFileIds.php | 37 ++++++++ .../lib/Share20/DefaultShareProviderTest.php | 60 ++++++++++++ tests/lib/Share20/ManagerTest.php | 94 +++++++++++++++++++ 10 files changed, 386 insertions(+), 21 deletions(-) create mode 100644 lib/public/Share/IShareProviderSupportsFileIds.php diff --git a/apps/federatedfilesharing/lib/FederatedShareProvider.php b/apps/federatedfilesharing/lib/FederatedShareProvider.php index aedea7c7dd03f..b6c5ff68e6a70 100644 --- a/apps/federatedfilesharing/lib/FederatedShareProvider.php +++ b/apps/federatedfilesharing/lib/FederatedShareProvider.php @@ -28,6 +28,7 @@ use OCP\Share\IShare; use OCP\Share\IShareProvider; use OCP\Share\IShareProviderSupportsAllSharesInFolder; +use OCP\Share\IShareProviderSupportsFileIds; use Override; use Psr\Log\LoggerInterface; @@ -36,7 +37,7 @@ * * @package OCA\FederatedFileSharing */ -class FederatedShareProvider implements IShareProvider, IShareProviderSupportsAllSharesInFolder { +class FederatedShareProvider implements IShareProvider, IShareProviderSupportsAllSharesInFolder, IShareProviderSupportsFileIds { public const SHARE_TYPE_REMOTE = 6; /** @var string */ @@ -559,6 +560,19 @@ private function getSharesInFolderInternal(?string $userId, Folder $node, ?bool */ #[\Override] public function getSharesBy($userId, $shareType, $node, $reshares, $limit, $offset) { + return $this->getSharesByInternal($userId, $shareType, $node === null ? null : [$node->getId()], $reshares, $limit, $offset); + } + + #[\Override] + public function getSharesByFileIds(string $userId, int $shareType, array $fileIds): array { + return $this->getSharesByInternal($userId, $shareType, $fileIds, false, -1, 0); + } + + /** + * @param list|null $fileIds + * @return list + */ + private function getSharesByInternal($userId, $shareType, ?array $fileIds, $reshares, $limit, $offset): array { $qb = $this->dbConnection->getQueryBuilder(); $qb->select('*') ->from('share'); @@ -590,8 +604,8 @@ public function getSharesBy($userId, $shareType, $node, $reshares, $limit, $offs ); } - if ($node !== null) { - $qb->andWhere($qb->expr()->eq('file_source', $qb->createNamedParameter($node->getId()))); + if ($fileIds !== null) { + $qb->andWhere($qb->expr()->in('file_source', $qb->createNamedParameter($fileIds, IQueryBuilder::PARAM_INT_ARRAY))); } if ($limit !== -1) { @@ -673,6 +687,19 @@ public function getSharesByPath(Node $path) { */ #[\Override] public function getSharedWith($userId, $shareType, $node, $limit, $offset) { + return $this->getSharedWithInternal($userId, $shareType, $node === null ? null : [$node->getId()], $limit, $offset); + } + + #[\Override] + public function getSharedWithByFileIds(string $userId, int $shareType, array $fileIds): array { + return array_values($this->getSharedWithInternal($userId, $shareType, $fileIds, -1, 0)); + } + + /** + * @param list|null $fileIds + * @return IShare[] + */ + private function getSharedWithInternal($userId, $shareType, ?array $fileIds, $limit, $offset): array { /** @var IShare[] $shares */ $shares = []; @@ -695,8 +722,8 @@ public function getSharedWith($userId, $shareType, $node, $limit, $offset) { $qb->andWhere($qb->expr()->eq('share_with', $qb->createNamedParameter($userId))); // Filter by node if provided - if ($node !== null) { - $qb->andWhere($qb->expr()->eq('file_source', $qb->createNamedParameter($node->getId()))); + if ($fileIds !== null) { + $qb->andWhere($qb->expr()->in('file_source', $qb->createNamedParameter($fileIds, IQueryBuilder::PARAM_INT_ARRAY))); } $cursor = $qb->executeQuery(); diff --git a/apps/sharebymail/lib/ShareByMailProvider.php b/apps/sharebymail/lib/ShareByMailProvider.php index 71f984444b3ed..7bcff1323b463 100644 --- a/apps/sharebymail/lib/ShareByMailProvider.php +++ b/apps/sharebymail/lib/ShareByMailProvider.php @@ -839,6 +839,19 @@ public function restore(IShare $share, string $recipient): IShare { */ #[\Override] public function getSharesBy($userId, $shareType, $node, $reshares, $limit, $offset): array { + return $this->getSharesByInternal($userId, $shareType, $node === null ? null : [$node->getId()], $reshares, $limit, $offset); + } + + #[\Override] + public function getSharesByFileIds(string $userId, int $shareType, array $fileIds): array { + return $this->getSharesByInternal($userId, $shareType, $fileIds, false, -1, 0); + } + + /** + * @param list|null $fileIds + * @return list + */ + private function getSharesByInternal($userId, $shareType, ?array $fileIds, $reshares, $limit, $offset): array { $qb = $this->dbConnection->getQueryBuilder(); $qb->select('*') ->from('share'); @@ -861,7 +874,7 @@ public function getSharesBy($userId, $shareType, $node, $reshares, $limit, $offs $or1 ) ); - } elseif ($node === null) { + } elseif ($fileIds === null) { $qb->andWhere( $qb->expr()->orX( $qb->expr()->eq('uid_owner', $qb->createNamedParameter($userId)), @@ -870,8 +883,8 @@ public function getSharesBy($userId, $shareType, $node, $reshares, $limit, $offs ); } - if ($node !== null) { - $qb->andWhere($qb->expr()->eq('file_source', $qb->createNamedParameter($node->getId()))); + if ($fileIds !== null) { + $qb->andWhere($qb->expr()->in('file_source', $qb->createNamedParameter($fileIds, IQueryBuilder::PARAM_INT_ARRAY))); } if ($limit !== -1) { @@ -954,6 +967,19 @@ public function getSharesByPath(Node $path): array { */ #[\Override] public function getSharedWith($userId, $shareType, $node, $limit, $offset): array { + return $this->getSharedWithInternal($userId, $shareType, $node === null ? null : [$node->getId()], $limit, $offset); + } + + #[\Override] + public function getSharedWithByFileIds(string $userId, int $shareType, array $fileIds): array { + return array_values($this->getSharedWithInternal($userId, $shareType, $fileIds, -1, 0)); + } + + /** + * @param list|null $fileIds + * @return IShare[] + */ + private function getSharedWithInternal($userId, $shareType, ?array $fileIds, $limit, $offset): array { /** @var IShare[] $shares */ $shares = []; @@ -977,8 +1003,8 @@ public function getSharedWith($userId, $shareType, $node, $limit, $offset): arra $qb->andWhere($qb->expr()->eq('share_with', $qb->createNamedParameter($userId))); // Filter by node if provided - if ($node !== null) { - $qb->andWhere($qb->expr()->eq('file_source', $qb->createNamedParameter($node->getId()))); + if ($fileIds !== null) { + $qb->andWhere($qb->expr()->in('file_source', $qb->createNamedParameter($fileIds, IQueryBuilder::PARAM_INT_ARRAY))); } $cursor = $qb->executeQuery(); diff --git a/lib/composer/composer/autoload_classmap.php b/lib/composer/composer/autoload_classmap.php index 941234b4a8b1b..f7dd900d247aa 100644 --- a/lib/composer/composer/autoload_classmap.php +++ b/lib/composer/composer/autoload_classmap.php @@ -971,6 +971,7 @@ 'OCP\\Share\\IShareProviderGetUsers' => $baseDir . '/lib/public/Share/IShareProviderGetUsers.php', 'OCP\\Share\\IShareProviderSupportsAccept' => $baseDir . '/lib/public/Share/IShareProviderSupportsAccept.php', 'OCP\\Share\\IShareProviderSupportsAllSharesInFolder' => $baseDir . '/lib/public/Share/IShareProviderSupportsAllSharesInFolder.php', + 'OCP\\Share\\IShareProviderSupportsFileIds' => $baseDir . '/lib/public/Share/IShareProviderSupportsFileIds.php', 'OCP\\Share\\IShareProviderWithNotification' => $baseDir . '/lib/public/Share/IShareProviderWithNotification.php', 'OCP\\Share\\ShareReview\\Events\\ShareReviewAccessCheckEvent' => $baseDir . '/lib/public/Share/ShareReview/Events/ShareReviewAccessCheckEvent.php', 'OCP\\Share\\ShareReview\\IShareReviewSource' => $baseDir . '/lib/public/Share/ShareReview/IShareReviewSource.php', diff --git a/lib/composer/composer/autoload_static.php b/lib/composer/composer/autoload_static.php index 65ee51a5eec52..c7330437544b5 100644 --- a/lib/composer/composer/autoload_static.php +++ b/lib/composer/composer/autoload_static.php @@ -1017,6 +1017,7 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2 'OCP\\Share\\IShareProviderGetUsers' => __DIR__ . '/../../..' . '/lib/public/Share/IShareProviderGetUsers.php', 'OCP\\Share\\IShareProviderSupportsAccept' => __DIR__ . '/../../..' . '/lib/public/Share/IShareProviderSupportsAccept.php', 'OCP\\Share\\IShareProviderSupportsAllSharesInFolder' => __DIR__ . '/../../..' . '/lib/public/Share/IShareProviderSupportsAllSharesInFolder.php', + 'OCP\\Share\\IShareProviderSupportsFileIds' => __DIR__ . '/../../..' . '/lib/public/Share/IShareProviderSupportsFileIds.php', 'OCP\\Share\\IShareProviderWithNotification' => __DIR__ . '/../../..' . '/lib/public/Share/IShareProviderWithNotification.php', 'OCP\\Share\\ShareReview\\Events\\ShareReviewAccessCheckEvent' => __DIR__ . '/../../..' . '/lib/public/Share/ShareReview/Events/ShareReviewAccessCheckEvent.php', 'OCP\\Share\\ShareReview\\IShareReviewSource' => __DIR__ . '/../../..' . '/lib/public/Share/ShareReview/IShareReviewSource.php', diff --git a/lib/private/Share20/DefaultShareProvider.php b/lib/private/Share20/DefaultShareProvider.php index 965f22fae4822..52e66eaa91127 100644 --- a/lib/private/Share20/DefaultShareProvider.php +++ b/lib/private/Share20/DefaultShareProvider.php @@ -44,6 +44,7 @@ use OCP\Share\IShareProviderGetUsers; use OCP\Share\IShareProviderSupportsAccept; use OCP\Share\IShareProviderSupportsAllSharesInFolder; +use OCP\Share\IShareProviderSupportsFileIds; use OCP\Share\IShareProviderWithNotification; use OCP\Util; use Psr\Log\LoggerInterface; @@ -57,6 +58,7 @@ * @package OC\Share20 */ class DefaultShareProvider implements + IShareProviderSupportsFileIds, IShareProviderWithNotification, IShareProviderSupportsAccept, IShareProviderSupportsAllSharesInFolder, @@ -726,6 +728,19 @@ private function getSharesInFolderInternal(?string $userId, Folder $node, ?bool */ #[\Override] public function getSharesBy($userId, $shareType, $node, $reshares, $limit, $offset) { + return $this->getSharesByInternal($userId, $shareType, $node === null ? null : [$node->getId()], $reshares, $limit, $offset); + } + + #[\Override] + public function getSharesByFileIds(string $userId, int $shareType, array $fileIds): array { + return $this->getSharesByInternal($userId, $shareType, $fileIds, false, -1, 0); + } + + /** + * @param list|null $fileIds + * @return list + */ + private function getSharesByInternal($userId, $shareType, ?array $fileIds, $reshares, $limit, $offset): array { $qb = $this->dbConn->getQueryBuilder(); $qb->select('*') ->from('share') @@ -739,7 +754,7 @@ public function getSharesBy($userId, $shareType, $node, $reshares, $limit, $offs if ($reshares === false) { $qb->andWhere($qb->expr()->eq('uid_initiator', $qb->createNamedParameter($userId))); } else { - if ($node === null) { + if ($fileIds === null) { $qb->andWhere( $qb->expr()->orX( $qb->expr()->eq('uid_owner', $qb->createNamedParameter($userId)), @@ -749,8 +764,8 @@ public function getSharesBy($userId, $shareType, $node, $reshares, $limit, $offs } } - if ($node !== null) { - $qb->andWhere($qb->expr()->eq('file_source', $qb->createNamedParameter($node->getId()))); + if ($fileIds !== null) { + $qb->andWhere($qb->expr()->in('file_source', $qb->createNamedParameter($fileIds, IQueryBuilder::PARAM_INT_ARRAY))); } if ($limit !== -1) { @@ -869,7 +884,12 @@ private function isAccessibleResult($data) { #[\Override] public function getSharedWith($userId, $shareType, $node, $limit, $offset) { - return $this->_getSharedWith($userId, $shareType, $limit, $offset, $node); + return $this->_getSharedWith($userId, $shareType, $limit, $offset, $node === null ? null : [$node->getId()]); + } + + #[\Override] + public function getSharedWithByFileIds(string $userId, int $shareType, array $fileIds): array { + return array_values($this->_getSharedWith($userId, $shareType, -1, 0, $fileIds)); } #[\Override] @@ -885,6 +905,7 @@ public function getSharedWithByPath( } /** + * @param list|null $fileIds only the shares of these files * @return IShare[] */ private function _getSharedWith( @@ -892,7 +913,7 @@ private function _getSharedWith( int $shareType, int $limit, int $offset, - ?Node $node = null, + ?array $fileIds = null, ?string $path = null, ?bool $forChildren = false, ): iterable { @@ -940,9 +961,9 @@ private function _getSharedWith( ->andWhere($qb->expr()->eq('share_with', $qb->createNamedParameter($userId))) ->andWhere($qb->expr()->in('item_type', $qb->createNamedParameter(['file', 'folder'], IQueryBuilder::PARAM_STR_ARRAY))); - // Filter by node if provided - if ($node !== null) { - $qb->andWhere($qb->expr()->eq('file_source', $qb->createNamedParameter($node->getId()))); + // Filter by files if provided + if ($fileIds !== null) { + $qb->andWhere($qb->expr()->in('file_source', $qb->createNamedParameter($fileIds, IQueryBuilder::PARAM_INT_ARRAY))); } if ($path !== null) { @@ -1007,9 +1028,9 @@ private function _getSharedWith( $qb->setMaxResults($limit - count($shares)); } - // Filter by node if provided - if ($node !== null) { - $qb->andWhere($qb->expr()->eq('file_source', $qb->createNamedParameter($node->getId()))); + // Filter by files if provided + if ($fileIds !== null) { + $qb->andWhere($qb->expr()->in('file_source', $qb->createNamedParameter($fileIds, IQueryBuilder::PARAM_INT_ARRAY))); } if ($path !== null) { diff --git a/lib/private/Share20/Manager.php b/lib/private/Share20/Manager.php index 2d2720dfb8f5b..2ced393890f4f 100644 --- a/lib/private/Share20/Manager.php +++ b/lib/private/Share20/Manager.php @@ -79,6 +79,7 @@ use OCP\Share\IShareProviderGetUsers; use OCP\Share\IShareProviderSupportsAccept; use OCP\Share\IShareProviderSupportsAllSharesInFolder; +use OCP\Share\IShareProviderSupportsFileIds; use OCP\Share\IShareProviderWithNotification; use Override; use Psr\Log\LoggerInterface; @@ -1234,6 +1235,81 @@ public function getSharesBy(string $userId, int $shareType, ?Node $path = null, return $shares; } + #[Override] + public function getSharesByNodes(string $userId, int $shareType, array $nodes): array { + try { + $provider = $this->factory->getProviderForType($shareType); + } catch (ProviderException) { + return []; + } + + if ($this->config->getAppValue('files_sharing', 'hide_disabled_user_shares', 'no') === 'yes' + && $this->userManager->get($userId)?->isEnabled() === false) { + return []; + } + + $shares = []; + $lookup = []; + foreach ($nodes as $node) { + if ($node->getMountPoint() instanceof IShareOwnerlessMount) { + // Every share of the path, not only the ones the user created + $shares[] = array_filter($provider->getSharesByPath($node), static fn (IShare $share): bool => $share->getShareType() === $shareType); + } elseif ($provider instanceof IShareProviderSupportsFileIds) { + $lookup[] = $node->getId(); + } else { + $shares[] = $provider->getSharesBy($userId, $shareType, $node, false, -1, 0); + } + } + foreach (array_chunk(array_values(array_unique($lookup)), 1000) as $fileIds) { + /** @var IShareProviderSupportsFileIds $provider */ + $shares[] = $provider->getSharesByFileIds($userId, $shareType, $fileIds); + } + + return $this->groupValidSharesByFileId(array_merge(...$shares), $userId); + } + + #[Override] + public function getSharedWithByNodes(string $userId, int $shareType, array $nodes): array { + try { + $provider = $this->factory->getProviderForType($shareType); + } catch (ProviderException) { + return []; + } + + $shares = []; + if ($provider instanceof IShareProviderSupportsFileIds) { + $fileIds = array_values(array_unique(array_map(static fn (Node $node): int => $node->getId(), $nodes))); + foreach (array_chunk($fileIds, 1000) as $chunk) { + $shares[] = $provider->getSharedWithByFileIds($userId, $shareType, $chunk); + } + } else { + foreach ($nodes as $node) { + $shares[] = $provider->getSharedWith($userId, $shareType, $node, -1, 0); + } + } + + return $this->groupValidSharesByFileId(array_merge(...$shares), $userId); + } + + /** + * Drop the expired and invalid shares, like `getSharesBy()` and `getSharedWith()` do + * + * @param list $shares + * @return array> + */ + private function groupValidSharesByFileId(array $shares, string $userId): array { + $result = []; + foreach ($shares as $share) { + try { + $this->checkShare($share, viewer: $userId); + } catch (ShareNotFound) { + continue; + } + $result[$share->getNodeId()][] = $share; + } + return $result; + } + #[Override] public function getSharedWith(string $userId, int $shareType, ?Node $node = null, int $limit = 50, int $offset = 0): array { try { diff --git a/lib/public/Share/IManager.php b/lib/public/Share/IManager.php index d6bc7725c60de..b2b1a0ccfea39 100644 --- a/lib/public/Share/IManager.php +++ b/lib/public/Share/IManager.php @@ -134,6 +134,28 @@ public function getSharesBy(string $userId, int $shareType, ?Node $path = null, */ public function getSharedWith(string $userId, int $shareType, ?Node $node = null, int $limit = 50, int $offset = 0): array; + /** + * Get the shares the user created on each of these nodes, + * the same as `getSharesBy($userId, $shareType, $node, false, -1)` for each node + * + * @param IShare::TYPE_* $shareType + * @param list $nodes + * @return array> shares by file id, nodes without shares are left out + * @since 36.0.0 + */ + public function getSharesByNodes(string $userId, int $shareType, array $nodes): array; + + /** + * Get the shares of each of these nodes the user received, + * the same as `getSharedWith($userId, $shareType, $node, -1)` for each node + * + * @param IShare::TYPE_* $shareType + * @param list $nodes + * @return array> shares by file id, nodes without shares are left out + * @since 36.0.0 + */ + public function getSharedWithByNodes(string $userId, int $shareType, array $nodes): array; + /** * Get shares shared with a $user filtering by $path. * diff --git a/lib/public/Share/IShareProviderSupportsFileIds.php b/lib/public/Share/IShareProviderSupportsFileIds.php new file mode 100644 index 0000000000000..788c78e65446a --- /dev/null +++ b/lib/public/Share/IShareProviderSupportsFileIds.php @@ -0,0 +1,37 @@ + $fileIds at most 1000 + * @return list + * @since 36.0.0 + */ + public function getSharesByFileIds(string $userId, int $shareType, array $fileIds): array; + + /** + * Get the shares of these files the user received, like `getSharedWith()` for each file + * + * @param IShare::TYPE_* $shareType + * @param non-empty-list $fileIds at most 1000 + * @return list + * @since 36.0.0 + */ + public function getSharedWithByFileIds(string $userId, int $shareType, array $fileIds): array; +} diff --git a/tests/lib/Share20/DefaultShareProviderTest.php b/tests/lib/Share20/DefaultShareProviderTest.php index 7b64acee33f29..ee82c2e799214 100644 --- a/tests/lib/Share20/DefaultShareProviderTest.php +++ b/tests/lib/Share20/DefaultShareProviderTest.php @@ -3272,4 +3272,64 @@ public function testSendNote(): void { self::invokePrivate($this->provider, 'sendNote', [[$recipient], $share]); } + + public function testLookupByFileIdsMatchesLookupByNode(): void { + $storageId = $this->createTestStorageEntry('home::shareOwner'); + $fileIds = [ + $this->createTestFileEntry('files/a.txt', $storageId), + $this->createTestFileEntry('files/b.txt', $storageId), + $this->createTestFileEntry('files/c.txt', $storageId), + ]; + [$a, $b, $c] = $fileIds; + + // Created by alice + $this->addShareToDB(IShare::TYPE_USER, 'bob', 'alice', 'shareOwner', 'file', $a, '/a.txt', 19, null, null); + $this->addShareToDB(IShare::TYPE_USER, 'carol', 'alice', 'shareOwner', 'file', $b, '/b.txt', 19, null, null); + $this->addShareToDB(IShare::TYPE_LINK, null, 'alice', 'shareOwner', 'file', $b, null, 1, 'token1', null); + // Created by someone else + $this->addShareToDB(IShare::TYPE_USER, 'dave', 'bob', 'shareOwner', 'file', $a, '/a.txt', 19, null, null); + // Received by alice, directly and through her groups + $this->addShareToDB(IShare::TYPE_USER, 'alice', 'bob', 'shareOwner', 'file', $c, '/c.txt', 19, null, null); + $this->addShareToDB(IShare::TYPE_GROUP, 'staff', 'bob', 'shareOwner', 'file', $a, '/a.txt', 19, null, null); + $left = $this->addShareToDB(IShare::TYPE_GROUP, 'staff', 'bob', 'shareOwner', 'file', $c, '/c.txt', 19, null, null); + // alice left that one + $this->addShareToDB(IShare::TYPE_USERGROUP, 'alice', 'bob', 'shareOwner', 'file', $c, '/c.txt', 0, null, null, $left); + $this->addShareToDB(IShare::TYPE_GROUP, 'other', 'bob', 'shareOwner', 'file', $b, '/b.txt', 19, null, null); + + $user = $this->createMock(IUser::class); + $user->method('getUID')->willReturn('alice'); + $this->userManager->method('get')->willReturn($user); + $this->groupManager->method('getUserGroupIds')->willReturn(['staff']); + $this->rootFolder->method('getUserFolder')->willReturnSelf(); + $this->rootFolder->method('getFirstNodeById')->willReturn($this->createMock(File::class)); + + $ids = static fn (array $shares): array => array_map(static fn (IShare $share): string => $share->getId(), $shares); + foreach ([IShare::TYPE_USER, IShare::TYPE_GROUP, IShare::TYPE_LINK] as $shareType) { + $byFileIds = $this->provider->getSharesByFileIds('alice', $shareType, $fileIds); + // Like getSharedWith(), the provider receives no link shares + $withByFileIds = $shareType === IShare::TYPE_LINK ? [] : $this->provider->getSharedWithByFileIds('alice', $shareType, $fileIds); + $byNode = []; + $withByNode = []; + foreach ($fileIds as $fileId) { + $node = $this->createMock(File::class); + $node->method('getId')->willReturn($fileId); + $byNode[] = $this->provider->getSharesBy('alice', $shareType, $node, false, -1, 0); + if ($shareType !== IShare::TYPE_LINK) { + $withByNode[] = $this->provider->getSharedWith('alice', $shareType, $node, -1, 0); + } + } + + $this->assertEqualsCanonicalizing($ids(array_merge(...$byNode)), $ids($byFileIds), 'shares by alice, type ' . $shareType); + if ($shareType !== IShare::TYPE_LINK) { + $this->assertEqualsCanonicalizing($ids(array_merge(...$withByNode)), $ids($withByFileIds), 'shares with alice, type ' . $shareType); + } + } + + // The expected shares are found at all + $this->assertCount(2, $this->provider->getSharesByFileIds('alice', IShare::TYPE_USER, $fileIds)); + $this->assertCount(1, $this->provider->getSharedWithByFileIds('alice', IShare::TYPE_USER, $fileIds)); + // The group share alice left comes back with no permissions, like with getSharedWith() + $groupShares = $this->provider->getSharedWithByFileIds('alice', IShare::TYPE_GROUP, $fileIds); + $this->assertEqualsCanonicalizing([19, 0], array_map(static fn (IShare $share): int => $share->getPermissions(), $groupShares)); + } } diff --git a/tests/lib/Share20/ManagerTest.php b/tests/lib/Share20/ManagerTest.php index 1d62c7301c519..e04de55b61365 100644 --- a/tests/lib/Share20/ManagerTest.php +++ b/tests/lib/Share20/ManagerTest.php @@ -5749,6 +5749,100 @@ public function testCurrentUserCanEnumerateTargetUser(bool $currentUserIsGuest, $this->assertSame($expected, $manager->currentUserCanEnumerateTargetUser($currentUser, $targetUser)); } + + private function nodeWithId(int $id, bool $ownerless = false): File&MockObject { + $node = $this->createMock(File::class); + $node->method('getId')->willReturn($id); + $node->method('getMountPoint')->willReturn($this->createMock($ownerless ? IShareOwnerlessMount::class : IMountPoint::class)); + return $node; + } + + private function shareOn(int $fileId, int $shareType = IShare::TYPE_USER): IShare { + $share = $this->manager->newShare(); + $share->setNodeId($fileId); + $share->setShareType($shareType); + return $share; + } + + public function testGetSharesByNodesInOneLookup(): void { + $nodes = [$this->nodeWithId(1), $this->nodeWithId(2), $this->nodeWithId(3)]; + $first = $this->shareOn(1); + $second = $this->shareOn(1); + $third = $this->shareOn(3); + + $this->defaultProvider->expects($this->once()) + ->method('getSharesByFileIds') + ->with('user', IShare::TYPE_USER, [1, 2, 3]) + ->willReturn([$first, $third, $second]); + $this->defaultProvider->expects($this->never())->method('getSharesBy'); + + $this->assertSame([1 => [$first, $second], 3 => [$third]], $this->manager->getSharesByNodes('user', IShare::TYPE_USER, $nodes)); + } + + public function testGetSharesByNodesOnOwnerlessMounts(): void { + $ownerless = $this->nodeWithId(1, true); + $regular = $this->nodeWithId(2); + $byOthers = $this->shareOn(1); + $otherType = $this->shareOn(1, IShare::TYPE_LINK); + $mine = $this->shareOn(2); + + // Every share of the path, of the requested type + $this->defaultProvider->expects($this->once()) + ->method('getSharesByPath') + ->with($ownerless) + ->willReturn([$byOthers, $otherType]); + $this->defaultProvider->expects($this->once()) + ->method('getSharesByFileIds') + ->with('user', IShare::TYPE_USER, [2]) + ->willReturn([$mine]); + + $this->assertSame([1 => [$byOthers], 2 => [$mine]], $this->manager->getSharesByNodes('user', IShare::TYPE_USER, [$ownerless, $regular])); + } + + public function testGetSharedWithByNodesInOneLookup(): void { + $nodes = [$this->nodeWithId(1), $this->nodeWithId(2)]; + $share = $this->shareOn(2, IShare::TYPE_GROUP); + + $this->defaultProvider->expects($this->once()) + ->method('getSharedWithByFileIds') + ->with('user', IShare::TYPE_GROUP, [1, 2]) + ->willReturn([$share]); + $this->defaultProvider->expects($this->never())->method('getSharedWith'); + + $this->assertSame([2 => [$share]], $this->manager->getSharedWithByNodes('user', IShare::TYPE_GROUP, $nodes)); + } + + public function testLookupByNodesFallsBackToEachNode(): void { + $provider = $this->createMock(IShareProvider::class); + $this->factory->setProvider($provider); + $nodes = [$this->nodeWithId(1), $this->nodeWithId(2)]; + $outgoing = $this->shareOn(2); + $incoming = $this->shareOn(1); + + $provider->expects($this->exactly(2)) + ->method('getSharesBy') + ->willReturnCallback(fn (string $userId, int $shareType, Node $node, bool $reshares, int $limit, int $offset): array => $node->getId() === 2 && !$reshares && $limit === -1 ? [$outgoing] : []); + $provider->expects($this->exactly(2)) + ->method('getSharedWith') + ->willReturnCallback(fn (string $userId, int $shareType, Node $node, int $limit, int $offset): array => $node->getId() === 1 && $limit === -1 ? [$incoming] : []); + + $this->assertSame([2 => [$outgoing]], $this->manager->getSharesByNodes('user', IShare::TYPE_USER, $nodes)); + $this->assertSame([1 => [$incoming]], $this->manager->getSharedWithByNodes('user', IShare::TYPE_USER, $nodes)); + } + + public function testLookupByNodesSkipsExpiredShares(): void { + $expired = $this->shareOn(1); + $expired->setExpirationDate(new \DateTime('-1 day')); + $valid = $this->shareOn(1); + + $manager = $this->createManagerMock() + ->onlyMethods(['deleteShare']) + ->getMock(); + $manager->expects($this->once())->method('deleteShare')->with($expired); + $this->defaultProvider->method('getSharesByFileIds')->willReturn([$expired, $valid]); + + $this->assertSame([1 => [$valid]], $manager->getSharesByNodes('user', IShare::TYPE_USER, [$this->nodeWithId(1)])); + } } class DummyFactory implements IProviderFactory { From d33c06816561cd71d7b4436b61a138162c159d95 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?John=20Molakvo=C3=A6?= <14975046+skjnldsv@users.noreply.github.com> Date: Tue, 13 Oct 2026 03:35:23 +0200 Subject: [PATCH 2/2] perf(dav): load the shares of all requested nodes at once MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: John Molakvoæ <14975046+skjnldsv@users.noreply.github.com> --- .../Files/NodeAttributes/SharesProvider.php | 29 +++++--- .../Files/NodeAttributes/SharesParityTest.php | 4 +- .../NodeAttributes/SharesProviderTest.php | 72 ++++++++++++------- 3 files changed, 66 insertions(+), 39 deletions(-) diff --git a/apps/dav/lib/Files/NodeAttributes/SharesProvider.php b/apps/dav/lib/Files/NodeAttributes/SharesProvider.php index 112dc8b19b31c..4ed7bc405f6f8 100644 --- a/apps/dav/lib/Files/NodeAttributes/SharesProvider.php +++ b/apps/dav/lib/Files/NodeAttributes/SharesProvider.php @@ -52,10 +52,9 @@ public function preload(NodeAttributeContext $context, array $nodes, array $attr return; } - foreach ($nodes as $node) { - if (!isset($this->shares[$userId][$node->getId()])) { - $this->shares[$userId][$node->getId()] = $this->getShares($userId, $node); - } + $nodes = array_values(array_filter($nodes, fn (Node $node): bool => !isset($this->shares[$userId][$node->getId()]))); + if ($nodes !== []) { + $this->shares[$userId] = $this->getShares($userId, $nodes) + ($this->shares[$userId] ?? []); } } @@ -104,20 +103,28 @@ public function getAttributes(NodeAttributeContext $context, Node $node, array $ } /** - * Shares the user created on the node, and the ones the user received for it + * Shares the user created on the nodes, and the ones the user received for them * - * @return list + * @param non-empty-list $nodes + * @return array> by file id */ - private function getShares(string $userId, Node $node): array { - $shares = []; + private function getShares(string $userId, array $nodes): array { + $shares = array_fill_keys(array_map(static fn (Node $node): int => $node->getId(), $nodes), []); foreach (self::SHARE_TYPES as $shareType) { - $shares[] = $this->shareManager->getSharesBy($userId, $shareType, $node, false, -1); + $typeShares = [$this->shareManager->getSharesByNodes($userId, $shareType, $nodes)]; try { - $shares[] = $this->shareManager->getSharedWith($userId, $shareType, $node, -1); + $typeShares[] = $this->shareManager->getSharedWithByNodes($userId, $shareType, $nodes); } catch (BackendError) { // a share provider that is not available has no shares to show } + foreach ($typeShares as $sharesByFileId) { + foreach ($sharesByFileId as $fileId => $fileShares) { + if (isset($shares[$fileId])) { + array_push($shares[$fileId], ...$fileShares); + } + } + } } - return array_values(array_merge(...$shares)); + return $shares; } } diff --git a/apps/dav/tests/unit/Files/NodeAttributes/SharesParityTest.php b/apps/dav/tests/unit/Files/NodeAttributes/SharesParityTest.php index 3bfc18ca46446..74cf7204e9244 100644 --- a/apps/dav/tests/unit/Files/NodeAttributes/SharesParityTest.php +++ b/apps/dav/tests/unit/Files/NodeAttributes/SharesParityTest.php @@ -91,8 +91,8 @@ private function shareManager(array $outgoing, array $incoming): IManager { }, array_filter($shares, static fn (array $data): bool => $data[0] === $type))); $shareManager = $this->createMock(IManager::class); - $shareManager->method('getSharesBy')->willReturnCallback(static fn (string $userId, int $type): array => $toShares($outgoing, $type)); - $shareManager->method('getSharedWith')->willReturnCallback(static fn (string $userId, int $type): array => $toShares($incoming, $type)); + $shareManager->method('getSharesByNodes')->willReturnCallback(static fn (string $userId, int $type): array => array_filter([42 => $toShares($outgoing, $type)])); + $shareManager->method('getSharedWithByNodes')->willReturnCallback(static fn (string $userId, int $type): array => array_filter([42 => $toShares($incoming, $type)])); return $shareManager; } diff --git a/apps/dav/tests/unit/Files/NodeAttributes/SharesProviderTest.php b/apps/dav/tests/unit/Files/NodeAttributes/SharesProviderTest.php index 4532da43904c0..063666431cff9 100644 --- a/apps/dav/tests/unit/Files/NodeAttributes/SharesProviderTest.php +++ b/apps/dav/tests/unit/Files/NodeAttributes/SharesProviderTest.php @@ -95,11 +95,11 @@ public static function dataShareTypes(): array { #[DataProvider('dataShareTypes')] public function testOutgoingSharesOfANode(array $shareTypes): void { $node = $this->node(File::class, 11, '/alice/files/report.md'); - $this->shareManager->method('getSharesBy') - ->willReturnCallback(fn (string $userId, int $type, Node $sharedNode, bool $reshares, int $limit): array => $userId === 'alice' && $sharedNode === $node && !$reshares && $limit === -1 && in_array($type, $shareTypes, true) - ? [$this->share($type)] + $this->shareManager->method('getSharesByNodes') + ->willReturnCallback(fn (string $userId, int $type, array $nodes): array => $userId === 'alice' && $nodes === [$node] && in_array($type, $shareTypes, true) + ? [11 => [$this->share($type)]] : []); - $this->shareManager->method('getSharedWith')->willReturn([]); + $this->shareManager->method('getSharedWithByNodes')->willReturn([]); [$types] = $this->getValues($node); self::assertSame($shareTypes, $types); @@ -107,11 +107,11 @@ public function testOutgoingSharesOfANode(array $shareTypes): void { public function testReceivedSharesAreIncludedForANode(): void { $node = $this->node(Folder::class, 12, '/alice/files/Shared'); - $this->shareManager->method('getSharesBy') - ->willReturnCallback(fn (string $userId, int $type): array => $type === IShare::TYPE_LINK ? [$this->share(IShare::TYPE_LINK)] : []); - $this->shareManager->method('getSharedWith') - ->willReturnCallback(fn (string $userId, int $type, ?Node $sharedNode, int $limit): array => $userId === 'alice' && $sharedNode === $node && $limit === -1 && $type === IShare::TYPE_GROUP - ? [$this->share(IShare::TYPE_GROUP, 'staff', 'Staff')] + $this->shareManager->method('getSharesByNodes') + ->willReturnCallback(fn (string $userId, int $type): array => $type === IShare::TYPE_LINK ? [12 => [$this->share(IShare::TYPE_LINK)]] : []); + $this->shareManager->method('getSharedWithByNodes') + ->willReturnCallback(fn (string $userId, int $type, array $nodes): array => $userId === 'alice' && $nodes === [$node] && $type === IShare::TYPE_GROUP + ? [12 => [$this->share(IShare::TYPE_GROUP, 'staff', 'Staff')]] : []); [$types, $sharees] = $this->getValues($node); @@ -124,9 +124,9 @@ public function testReceivedSharesAreIncludedForANode(): void { public function testUnavailableShareProviderIsSkipped(): void { $node = $this->node(File::class, 11, '/alice/files/report.md'); - $this->shareManager->method('getSharesBy') - ->willReturnCallback(fn (string $userId, int $type): array => $type === IShare::TYPE_USER ? [$this->share(IShare::TYPE_USER, 'bob', 'Bob')] : []); - $this->shareManager->method('getSharedWith') + $this->shareManager->method('getSharesByNodes') + ->willReturnCallback(fn (string $userId, int $type): array => $type === IShare::TYPE_USER ? [11 => [$this->share(IShare::TYPE_USER, 'bob', 'Bob')]] : []); + $this->shareManager->method('getSharedWithByNodes') ->willReturnCallback(static fn (string $userId, int $type): array => $type === IShare::TYPE_ROOM ? throw new BackendError() : []); [$types] = $this->getValues($node); @@ -135,17 +135,37 @@ public function testUnavailableShareProviderIsSkipped(): void { public function testShareTypesAreUnique(): void { $node = $this->node(File::class, 11, '/alice/files/report.md'); - $this->shareManager->method('getSharesBy') + $this->shareManager->method('getSharesByNodes') ->willReturnCallback(fn (string $userId, int $type): array => $type === IShare::TYPE_USER - ? [$this->share(IShare::TYPE_USER, 'bob', 'Bob'), $this->share(IShare::TYPE_USER, 'carol', 'Carol')] + ? [11 => [$this->share(IShare::TYPE_USER, 'bob', 'Bob'), $this->share(IShare::TYPE_USER, 'carol', 'Carol')]] : []); - $this->shareManager->method('getSharedWith')->willReturn([]); + $this->shareManager->method('getSharedWithByNodes')->willReturn([]); [$types, $sharees] = $this->getValues($node); self::assertSame([IShare::TYPE_USER], $types); self::assertSame(['bob', 'carol'], array_column($sharees, 'id')); } + public function testNodesAreLoadedTogether(): void { + $first = $this->node(File::class, 31, '/alice/files/a.md'); + $second = $this->node(File::class, 32, '/alice/files/Other/b.md'); + + // One call per share type and direction for all the nodes + $this->shareManager->expects(self::exactly(8)) + ->method('getSharesByNodes') + ->with('alice', self::anything(), [$first, $second]) + ->willReturnCallback(fn (string $userId, int $type): array => $type === IShare::TYPE_LINK ? [32 => [$this->share(IShare::TYPE_LINK)]] : []); + $this->shareManager->expects(self::exactly(8)) + ->method('getSharedWithByNodes') + ->with('alice', self::anything(), [$first, $second]) + ->willReturn([]); + + $this->provider->preload($this->context, [$first, $second], self::ATTRIBUTES); + + self::assertSame([], $this->getValues($first)[0]); + self::assertSame([IShare::TYPE_LINK], $this->getValues($second)[0]); + } + public function testFolderListingLoadsTheChildrenAtOnce(): void { $folder = $this->node(Folder::class, 20, '/alice/files/Projects'); $shared = $this->node(File::class, 21, '/alice/files/Projects/plan.md'); @@ -157,12 +177,12 @@ public function testFolderListingLoadsTheChildrenAtOnce(): void { ->willReturn([21 => [$this->share(IShare::TYPE_LINK)]]); // The folder itself is looked up on its own, its children are not $this->shareManager->expects(self::exactly(8)) - ->method('getSharesBy') - ->with('alice', self::anything(), $folder, false, -1) + ->method('getSharesByNodes') + ->with('alice', self::anything(), [$folder]) ->willReturn([]); $this->shareManager->expects(self::exactly(8)) - ->method('getSharedWith') - ->with('alice', self::anything(), $folder, -1) + ->method('getSharedWithByNodes') + ->with('alice', self::anything(), [$folder]) ->willReturn([]); $this->provider->preloadListing($this->context, $folder, [$shared, $notShared], self::ATTRIBUTES); @@ -178,10 +198,10 @@ public function testAFolderAndItsChildOutsideOfAListingGetTheirOwnShares(): void $received = $this->node(Folder::class, 41, '/alice/files/X/Y'); $this->shareManager->expects(self::never())->method('getSharesInFolder'); - $this->shareManager->method('getSharesBy')->willReturn([]); - $this->shareManager->method('getSharedWith') - ->willReturnCallback(fn (string $userId, int $type, Node $node): array => $node === $received && $type === IShare::TYPE_USER - ? [$this->share(IShare::TYPE_USER, 'alice', 'Alice')] + $this->shareManager->method('getSharesByNodes')->willReturn([]); + $this->shareManager->method('getSharedWithByNodes') + ->willReturnCallback(fn (string $userId, int $type, array $nodes): array => $nodes === [$folder, $received] && $type === IShare::TYPE_USER + ? [41 => [$this->share(IShare::TYPE_USER, 'alice', 'Alice')]] : []); $this->provider->preload($this->context, [$folder, $received], self::ATTRIBUTES); @@ -192,9 +212,9 @@ public function testAFolderAndItsChildOutsideOfAListingGetTheirOwnShares(): void public function testPreloadingTwiceOnlyLoadsNewNodes(): void { $this->shareManager->expects(self::exactly(16)) - ->method('getSharesBy') + ->method('getSharesByNodes') ->willReturn([]); - $this->shareManager->method('getSharedWith')->willReturn([]); + $this->shareManager->method('getSharedWithByNodes')->willReturn([]); $this->provider->preload($this->context, [$this->node(File::class, 1, '/alice/files/a')], self::ATTRIBUTES); $this->provider->preload($this->context, [$this->node(File::class, 1, '/alice/files/a')], self::ATTRIBUTES); @@ -204,7 +224,7 @@ public function testPreloadingTwiceOnlyLoadsNewNodes(): void { public function testNothingOnPublicShares(): void { $context = NodeAttributeContext::forShare($this->createMock(IShare::class)); $node = $this->node(File::class, 11, '/bob/files/Shared/report.md'); - $this->shareManager->expects(self::never())->method('getSharesBy'); + $this->shareManager->expects(self::never())->method('getSharesByNodes'); $this->provider->preload($context, [$node], self::ATTRIBUTES); self::assertSame([], $this->provider->getAttributes($context, $node, self::ATTRIBUTES));