From 41e6124dcc816f1bf495b2edf68e40615708eedc Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Tue, 14 Apr 2026 16:43:08 +0200 Subject: [PATCH] perf: only load a single mount at a time when checking for share conflicts Signed-off-by: Robin Appelman --- .../lib/Repair/CleanupShareTarget.php | 2 +- .../lib/ShareRecipientUpdater.php | 8 +--- .../lib/ShareTargetValidator.php | 20 +++++----- .../tests/ShareRecipientUpdaterTest.php | 4 +- .../tests/ShareTargetValidatorTest.php | 11 +++--- lib/private/Files/Config/UserMountCache.php | 37 ++++++++++++++++++- lib/public/Files/Config/IUserMountCache.php | 12 +++++- 7 files changed, 67 insertions(+), 27 deletions(-) diff --git a/apps/files_sharing/lib/Repair/CleanupShareTarget.php b/apps/files_sharing/lib/Repair/CleanupShareTarget.php index 60a62c7add1..880c2a15af6 100644 --- a/apps/files_sharing/lib/Repair/CleanupShareTarget.php +++ b/apps/files_sharing/lib/Repair/CleanupShareTarget.php @@ -107,7 +107,7 @@ class CleanupShareTarget implements IRepairStep { (int)$shareInfo['file_source'], $absoluteNewTarget, $targetParentNode->getMountPoint(), - $userMounts, + fn ($path) => $userMounts[$path] ?? null, ); $newTarget = $userFolder->getRelativePath($absoluteNewTarget); diff --git a/apps/files_sharing/lib/ShareRecipientUpdater.php b/apps/files_sharing/lib/ShareRecipientUpdater.php index 4b410f97cbd..cdb7a7c1b75 100644 --- a/apps/files_sharing/lib/ShareRecipientUpdater.php +++ b/apps/files_sharing/lib/ShareRecipientUpdater.php @@ -51,7 +51,7 @@ class ShareRecipientUpdater { $mountKey = $parentShare->getNodeId() . '::' . $mountPoint; if (!isset($cachedMounts[$mountKey])) { $mountsChanged = true; - $this->shareTargetValidator->verifyMountPoint($user, $parentShare, $mountsByPath, $groupedShares); + $this->shareTargetValidator->verifyMountPoint($user, $parentShare, fn ($path) => $mountsByPath[$path] ?? null, $groupedShares); } } @@ -67,11 +67,7 @@ class ShareRecipientUpdater { * Validate a single received share for a user */ public function updateForAddedShare(IUser $user, IShare $share): void { - $cachedMounts = $this->userMountCache->getMountsForUser($user); - $mountPoints = array_map(fn (ICachedMountInfo $mount) => $mount->getMountPoint(), $cachedMounts); - $mountsByPath = array_combine($mountPoints, $cachedMounts); - - $target = $this->shareTargetValidator->verifyMountPoint($user, $share, $mountsByPath, [$share]); + $target = $this->shareTargetValidator->verifyMountPoint($user, $share, fn ($path) => $this->userMountCache->getMountAtPath($user, $path), [$share]); $mountPoint = $this->getMountPointFromTarget($user, $target); $this->userMountCache->addMount($user, $mountPoint, $share->getNode()->getData(), MountProvider::class); diff --git a/apps/files_sharing/lib/ShareTargetValidator.php b/apps/files_sharing/lib/ShareTargetValidator.php index 40aa5e7b7fc..e77dc5ddad0 100644 --- a/apps/files_sharing/lib/ShareTargetValidator.php +++ b/apps/files_sharing/lib/ShareTargetValidator.php @@ -45,14 +45,14 @@ class ShareTargetValidator { /** * check if the parent folder exists otherwise move the mount point up * - * @param array $allCachedMounts Other mounts for the user, indexed by path + * @param callable(string):?ICachedMountInfo $getMountByPath * @param IShare[] $childShares * @return string */ public function verifyMountPoint( IUser $user, IShare &$share, - array $allCachedMounts, + callable $getMountByPath, array $childShares, ): string { $mountPoint = basename($share->getTarget()); @@ -94,7 +94,7 @@ class ShareTargetValidator { $share->getNodeId(), Filesystem::normalizePath($absoluteParent . '/' . $mountPoint), $parentMount, - $allCachedMounts, + $getMountByPath, ); /** @psalm-suppress InternalMethod */ @@ -112,13 +112,13 @@ class ShareTargetValidator { /** - * @param ICachedMountInfo[] $allCachedMounts + * @param callable(string):?ICachedMountInfo $getMountByPath */ public function generateUniqueTarget( int $shareNodeId, string $absolutePath, IMountPoint $parentMount, - array $allCachedMounts, + callable $getMountByPath, ): string { $pathInfo = pathinfo($absolutePath); $ext = isset($pathInfo['extension']) ? '.' . $pathInfo['extension'] : ''; @@ -128,7 +128,7 @@ class ShareTargetValidator { $i = 2; $parentCache = $parentMount->getStorage()->getCache(); $internalPath = $parentMount->getInternalPath($absolutePath); - while ($parentCache->inCache($internalPath) || $this->hasConflictingMount($shareNodeId, $allCachedMounts, $absolutePath)) { + while ($parentCache->inCache($internalPath) || $this->hasConflictingMount($shareNodeId, $getMountByPath, $absolutePath)) { $absolutePath = Filesystem::normalizePath($dir . '/' . $name . ' (' . $i . ')' . $ext); $internalPath = $parentMount->getInternalPath($absolutePath); $i++; @@ -138,14 +138,14 @@ class ShareTargetValidator { } /** - * @param ICachedMountInfo[] $allCachedMounts + * @param callable(string):?ICachedMountInfo $getMountByPath */ - private function hasConflictingMount(int $shareNodeId, array $allCachedMounts, string $absolutePath): bool { - if (!isset($allCachedMounts[$absolutePath . '/'])) { + private function hasConflictingMount(int $shareNodeId, callable $getMountByPath, string $absolutePath): bool { + $mount = $getMountByPath($absolutePath . '/'); + if ($mount === null) { return false; } - $mount = $allCachedMounts[$absolutePath . '/']; if ($mount->getMountProvider() === MountProvider::class && $mount->getRootId() === $shareNodeId) { // "conflicting" mount is a mount for the current share return false; diff --git a/apps/files_sharing/tests/ShareRecipientUpdaterTest.php b/apps/files_sharing/tests/ShareRecipientUpdaterTest.php index 2316e6b8b7e..91c449f5843 100644 --- a/apps/files_sharing/tests/ShareRecipientUpdaterTest.php +++ b/apps/files_sharing/tests/ShareRecipientUpdaterTest.php @@ -61,7 +61,7 @@ class ShareRecipientUpdaterTest extends \Test\TestCase { ->willReturn([]); $this->shareTargetValidator->method('verifyMountPoint') - ->with($user1, $share, [], [$share]) + ->with($user1, $share, fn ($path) => null, [$share]) ->willReturn('/new-target'); $this->userMountCache->expects($this->exactly(1)) @@ -122,7 +122,7 @@ class ShareRecipientUpdaterTest extends \Test\TestCase { $this->setCachedMounts($user1, []); $this->shareTargetValidator->method('verifyMountPoint') - ->with($user1, $share, [], [$share]) + ->with($user1, $share, fn ($path) => null, [$share]) ->willReturn('/new-target'); $this->userMountCache->expects($this->exactly(1)) diff --git a/apps/files_sharing/tests/ShareTargetValidatorTest.php b/apps/files_sharing/tests/ShareTargetValidatorTest.php index df7766d8ea2..1c5b8b619c2 100644 --- a/apps/files_sharing/tests/ShareTargetValidatorTest.php +++ b/apps/files_sharing/tests/ShareTargetValidatorTest.php @@ -83,7 +83,7 @@ class ShareTargetValidatorTest extends TestCase { $share = $this->shareManager->getShareById($share->getFullId()); $this->assertSame('/foo/bar' . $this->folder, $share->getTarget()); - $this->targetValidator->verifyMountPoint($this->user2, $share, [], [$share]); + $this->targetValidator->verifyMountPoint($this->user2, $share, fn ($path) => null, [$share]); $share = $this->shareManager->getShareById($share->getFullId()); $this->assertSame($this->folder, $share->getTarget()); @@ -117,7 +117,7 @@ class ShareTargetValidatorTest extends TestCase { $share = $this->shareManager->getShareById($share->getFullId()); - $this->targetValidator->verifyMountPoint($this->user2, $share, [], [$share]); + $this->targetValidator->verifyMountPoint($this->user2, $share, fn ($path) => null, [$share]); $share = $this->shareManager->getShareById($share->getFullId()); $this->assertSame('/bar (2)', $share->getTarget()); @@ -142,9 +142,10 @@ class ShareTargetValidatorTest extends TestCase { $this->shareManager->acceptShare($share2, self::TEST_FILES_SHARING_API_USER2); $conflictingMount = $this->createMock(ICachedMountInfo::class); - $this->targetValidator->verifyMountPoint($this->user2, $share2, [ + $conflictingMounts = [ '/' . $this->user2->getUID() . '/files' . $this->folder2 . '/' => $conflictingMount - ], [$share2]); + ]; + $this->targetValidator->verifyMountPoint($this->user2, $share2, fn ($path) => $conflictingMounts[$path] ?? null, [$share2]); $share2 = $this->shareManager->getShareById($share2->getFullId()); @@ -179,7 +180,7 @@ class ShareTargetValidatorTest extends TestCase { $this->eventDispatcher->addListener(VerifyMountPointEvent::class, function (VerifyMountPointEvent $event): void { $event->setCreateParent(true); }); - $this->targetValidator->verifyMountPoint($this->user2, $share, [], [$share]); + $this->targetValidator->verifyMountPoint($this->user2, $share, fn ($path) => null, [$share]); $share = $this->shareManager->getShareById($share->getFullId()); $this->assertSame('/foo/bar' . $this->folder, $share->getTarget()); diff --git a/lib/private/Files/Config/UserMountCache.php b/lib/private/Files/Config/UserMountCache.php index 2d20b821c51..8bc04c62b33 100644 --- a/lib/private/Files/Config/UserMountCache.php +++ b/lib/private/Files/Config/UserMountCache.php @@ -5,6 +5,7 @@ * SPDX-FileCopyrightText: 2016 ownCloud, Inc. * SPDX-License-Identifier: AGPL-3.0-only */ + namespace OC\Files\Config; use OC\DB\Exceptions\DbalException; @@ -33,11 +34,13 @@ class UserMountCache implements IUserMountCache { /** * Cached mount info. + * * @var CappedMemoryCache **/ private CappedMemoryCache $mountsForUsers; /** * fileid => internal path mapping for cached mount info. + * * @var CappedMemoryCache **/ private CappedMemoryCache $internalPathCache; @@ -73,7 +76,9 @@ class UserMountCache implements IUserMountCache { $cachedMounts = $this->getMountsForUser($user); if (is_array($mountProviderClasses)) { - $cachedMounts = array_filter($cachedMounts, function (ICachedMountInfo $mountInfo) use ($mountProviderClasses, $newMounts) { + $cachedMounts = array_filter($cachedMounts, function ( + ICachedMountInfo $mountInfo, + ) use ($mountProviderClasses, $newMounts) { // for existing mounts that didn't have a mount provider set // we still want the ones that map to new mounts if ($mountInfo->getMountProvider() === '' && isset($newMounts[$mountInfo->getKey()])) { @@ -536,7 +541,13 @@ class UserMountCache implements IUserMountCache { } } - public function addMount(IUser $user, string $mountPoint, ICacheEntry $rootCacheEntry, string $mountProvider, ?int $mountId = null): void { + public function addMount( + IUser $user, + string $mountPoint, + ICacheEntry $rootCacheEntry, + string $mountProvider, + ?int $mountId = null, + ): void { $query = $this->connection->getQueryBuilder(); $query->insert('mounts') ->values([ @@ -567,4 +578,26 @@ class UserMountCache implements IUserMountCache { $this->internalPathCache = new CappedMemoryCache(); $this->mountsForUsers = new CappedMemoryCache(); } + + public function getMountAtPath(IUser $user, string $mountPoint): ?ICachedMountInfo { + if (isset($this->mountsForUsers[$user->getUID()])) { + foreach ($this->mountsForUsers[$user->getUID()] as $mount) { + if ($mount->getMountPoint() === $mountPoint) { + return $mount; + } + } + return null; + } + + $builder = $this->connection->getQueryBuilder(); + $query = $builder->select('storage_id', 'root_id', 'user_id', 'mount_point', 'mount_id', 'f.path', 'mount_provider_class') + ->from('mounts', 'm') + ->innerJoin('m', 'filecache', 'f', $builder->expr()->eq('m.root_id', 'f.fileid')) + ->where($builder->expr()->eq('user_id', $builder->createNamedParameter($user->getUID()))) + ->andWhere($builder->expr()->eq('mount_point_hash', $builder->createNamedParameter(hash('xxh128', $mountPoint)))) + ->setMaxResults(1); + + $row = $query->executeQuery()->fetch(); + return $row ? $this->dbRowToMountInfo($row) : null; + } } diff --git a/lib/public/Files/Config/IUserMountCache.php b/lib/public/Files/Config/IUserMountCache.php index d1541c7c4f5..fc0ec3f1f92 100644 --- a/lib/public/Files/Config/IUserMountCache.php +++ b/lib/public/Files/Config/IUserMountCache.php @@ -113,7 +113,10 @@ interface IUserMountCache { public function clear(): void; /** - * Get all cached mounts for a user + * Get the cached mount for a path + * + * This walks up the directly tree until a mount is found, if you only want + * to get the mount at the specific path, use `getMountAtPath` instead. * * @param IUser $user * @param string $path @@ -147,4 +150,11 @@ interface IUserMountCache { * @since 33.0.0 */ public function addMount(IUser $user, string $mountPoint, ICacheEntry $rootCacheEntry, string $mountProvider, ?int $mountId = null): void; + + /** + * Get the mount at the specified path, if any + * + * @since 33.0.2 + */ + public function getMountAtPath(IUser $user, string $mountPoint): ?ICachedMountInfo; }