Skip to content

Commit 5b81ada

Browse files
icewind1991icewind1991
committed
perf: only load a single mount at a time when checking for share conflicts
Signed-off-by: Robin Appelman <robin@icewind.nl>
1 parent bfff87a commit 5b81ada

File tree

7 files changed

+67
-27
lines changed

7 files changed

+67
-27
lines changed

‎apps/files_sharing/lib/Repair/CleanupShareTarget.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -107,7 +107,7 @@ public function run(IOutput $output) {
107107
(int)$shareInfo['file_source'],
108108
$absoluteNewTarget,
109109
$targetParentNode->getMountPoint(),
110-
$userMounts,
110+
fn ($path) => $userMounts[$path] ?? null,
111111
);
112112
$newTarget = $userFolder->getRelativePath($absoluteNewTarget);
113113

‎apps/files_sharing/lib/ShareRecipientUpdater.php‎

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,7 @@ public function updateForUser(IUser $user): void {
5151
$mountKey = $parentShare->getNodeId() . '::' . $mountPoint;
5252
if (!isset($cachedMounts[$mountKey])) {
5353
$mountsChanged = true;
54-
$this->shareTargetValidator->verifyMountPoint($user, $parentShare, $mountsByPath, $groupedShares);
54+
$this->shareTargetValidator->verifyMountPoint($user, $parentShare, fn ($path) => $mountsByPath[$path] ?? null, $groupedShares);
5555
}
5656
}
5757

@@ -67,11 +67,7 @@ public function updateForUser(IUser $user): void {
6767
* Validate a single received share for a user
6868
*/
6969
public function updateForAddedShare(IUser $user, IShare $share): void {
70-
$cachedMounts = $this->userMountCache->getMountsForUser($user);
71-
$mountPoints = array_map(fn (ICachedMountInfo $mount) => $mount->getMountPoint(), $cachedMounts);
72-
$mountsByPath = array_combine($mountPoints, $cachedMounts);
73-
74-
$target = $this->shareTargetValidator->verifyMountPoint($user, $share, $mountsByPath, [$share]);
70+
$target = $this->shareTargetValidator->verifyMountPoint($user, $share, fn ($path) => $this->userMountCache->getMountAtPath($user, $path), [$share]);
7571
$mountPoint = $this->getMountPointFromTarget($user, $target);
7672

7773
$this->userMountCache->addMount($user, $mountPoint, $share->getNode()->getData(), MountProvider::class);

‎apps/files_sharing/lib/ShareTargetValidator.php‎

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -45,14 +45,14 @@ private function getViewForUser(IUser $user): View {
4545
/**
4646
* check if the parent folder exists otherwise move the mount point up
4747
*
48-
* @param array<string, ICachedMountInfo> $allCachedMounts Other mounts for the user, indexed by path
48+
* @param callable(string):?ICachedMountInfo $getMountByPath
4949
* @param IShare[] $childShares
5050
* @return string
5151
*/
5252
public function verifyMountPoint(
5353
IUser $user,
5454
IShare &$share,
55-
array $allCachedMounts,
55+
callable $getMountByPath,
5656
array $childShares,
5757
): string {
5858
$mountPoint = basename($share->getTarget());
@@ -94,7 +94,7 @@ public function verifyMountPoint(
9494
$share->getNodeId(),
9595
Filesystem::normalizePath($absoluteParent . '/' . $mountPoint),
9696
$parentMount,
97-
$allCachedMounts,
97+
$getMountByPath,
9898
);
9999

100100
/** @psalm-suppress InternalMethod */
@@ -112,13 +112,13 @@ public function verifyMountPoint(
112112

113113

114114
/**
115-
* @param ICachedMountInfo[] $allCachedMounts
115+
* @param callable(string):?ICachedMountInfo $getMountByPath
116116
*/
117117
public function generateUniqueTarget(
118118
int $shareNodeId,
119119
string $absolutePath,
120120
IMountPoint $parentMount,
121-
array $allCachedMounts,
121+
callable $getMountByPath,
122122
): string {
123123
$pathInfo = pathinfo($absolutePath);
124124
$ext = isset($pathInfo['extension']) ? '.' . $pathInfo['extension'] : '';
@@ -128,7 +128,7 @@ public function generateUniqueTarget(
128128
$i = 2;
129129
$parentCache = $parentMount->getStorage()->getCache();
130130
$internalPath = $parentMount->getInternalPath($absolutePath);
131-
while ($parentCache->inCache($internalPath) || $this->hasConflictingMount($shareNodeId, $allCachedMounts, $absolutePath)) {
131+
while ($parentCache->inCache($internalPath) || $this->hasConflictingMount($shareNodeId, $getMountByPath, $absolutePath)) {
132132
$absolutePath = Filesystem::normalizePath($dir . '/' . $name . ' (' . $i . ')' . $ext);
133133
$internalPath = $parentMount->getInternalPath($absolutePath);
134134
$i++;
@@ -138,14 +138,14 @@ public function generateUniqueTarget(
138138
}
139139

140140
/**
141-
* @param ICachedMountInfo[] $allCachedMounts
141+
* @param callable(string):?ICachedMountInfo $getMountByPath
142142
*/
143-
private function hasConflictingMount(int $shareNodeId, array $allCachedMounts, string $absolutePath): bool {
144-
if (!isset($allCachedMounts[$absolutePath . '/'])) {
143+
private function hasConflictingMount(int $shareNodeId, callable $getMountByPath, string $absolutePath): bool {
144+
$mount = $getMountByPath($absolutePath . '/');
145+
if ($mount === null) {
145146
return false;
146147
}
147148

148-
$mount = $allCachedMounts[$absolutePath . '/'];
149149
if ($mount->getMountProvider() === MountProvider::class && $mount->getRootId() === $shareNodeId) {
150150
// "conflicting" mount is a mount for the current share
151151
return false;

‎apps/files_sharing/tests/ShareRecipientUpdaterTest.php‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,7 @@ public function testUpdateForShare() {
6161
->willReturn([]);
6262

6363
$this->shareTargetValidator->method('verifyMountPoint')
64-
->with($user1, $share, [], [$share])
64+
->with($user1, $share, fn ($path) => null, [$share])
6565
->willReturn('/new-target');
6666

6767
$this->userMountCache->expects($this->exactly(1))
@@ -122,7 +122,7 @@ public function testUpdateForUserAddedNoExisting() {
122122
$this->setCachedMounts($user1, []);
123123

124124
$this->shareTargetValidator->method('verifyMountPoint')
125-
->with($user1, $share, [], [$share])
125+
->with($user1, $share, fn ($path) => null, [$share])
126126
->willReturn('/new-target');
127127

128128
$this->userMountCache->expects($this->exactly(1))

‎apps/files_sharing/tests/ShareTargetValidatorTest.php‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -83,7 +83,7 @@ public function testShareMountLoseParentFolder(): void {
8383
$share = $this->shareManager->getShareById($share->getFullId());
8484
$this->assertSame('/foo/bar' . $this->folder, $share->getTarget());
8585

86-
$this->targetValidator->verifyMountPoint($this->user2, $share, [], [$share]);
86+
$this->targetValidator->verifyMountPoint($this->user2, $share, fn ($path) => null, [$share]);
8787

8888
$share = $this->shareManager->getShareById($share->getFullId());
8989
$this->assertSame($this->folder, $share->getTarget());
@@ -117,7 +117,7 @@ public function testShareMountOverFolder(): void {
117117

118118
$share = $this->shareManager->getShareById($share->getFullId());
119119

120-
$this->targetValidator->verifyMountPoint($this->user2, $share, [], [$share]);
120+
$this->targetValidator->verifyMountPoint($this->user2, $share, fn ($path) => null, [$share]);
121121

122122
$share = $this->shareManager->getShareById($share->getFullId());
123123
$this->assertSame('/bar (2)', $share->getTarget());
@@ -142,9 +142,10 @@ public function testShareMountOverShare(): void {
142142
$this->shareManager->acceptShare($share2, self::TEST_FILES_SHARING_API_USER2);
143143

144144
$conflictingMount = $this->createMock(ICachedMountInfo::class);
145-
$this->targetValidator->verifyMountPoint($this->user2, $share2, [
145+
$conflictingMounts = [
146146
'/' . $this->user2->getUID() . '/files' . $this->folder2 . '/' => $conflictingMount
147-
], [$share2]);
147+
];
148+
$this->targetValidator->verifyMountPoint($this->user2, $share2, fn ($path) => $conflictingMounts[$path] ?? null, [$share2]);
148149

149150
$share2 = $this->shareManager->getShareById($share2->getFullId());
150151

@@ -179,7 +180,7 @@ public function testShareMountCreateParentFolder(): void {
179180
$this->eventDispatcher->addListener(VerifyMountPointEvent::class, function (VerifyMountPointEvent $event): void {
180181
$event->setCreateParent(true);
181182
});
182-
$this->targetValidator->verifyMountPoint($this->user2, $share, [], [$share]);
183+
$this->targetValidator->verifyMountPoint($this->user2, $share, fn ($path) => null, [$share]);
183184

184185
$share = $this->shareManager->getShareById($share->getFullId());
185186
$this->assertSame('/foo/bar' . $this->folder, $share->getTarget());

‎lib/private/Files/Config/UserMountCache.php‎

Lines changed: 35 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
* SPDX-FileCopyrightText: 2016 ownCloud, Inc.
66
* SPDX-License-Identifier: AGPL-3.0-only
77
*/
8+
89
namespace OC\Files\Config;
910

1011
use OC\DB\Exceptions\DbalException;
@@ -33,11 +34,13 @@ class UserMountCache implements IUserMountCache {
3334

3435
/**
3536
* Cached mount info.
37+
*
3638
* @var CappedMemoryCache<ICachedMountInfo[]>
3739
**/
3840
private CappedMemoryCache $mountsForUsers;
3941
/**
4042
* fileid => internal path mapping for cached mount info.
43+
*
4144
* @var CappedMemoryCache<string>
4245
**/
4346
private CappedMemoryCache $internalPathCache;
@@ -73,7 +76,9 @@ public function registerMounts(IUser $user, array $mounts, ?array $mountProvider
7376

7477
$cachedMounts = $this->getMountsForUser($user);
7578
if (is_array($mountProviderClasses)) {
76-
$cachedMounts = array_filter($cachedMounts, function (ICachedMountInfo $mountInfo) use ($mountProviderClasses, $newMounts) {
79+
$cachedMounts = array_filter($cachedMounts, function (
80+
ICachedMountInfo $mountInfo,
81+
) use ($mountProviderClasses, $newMounts) {
7782
// for existing mounts that didn't have a mount provider set
7883
// we still want the ones that map to new mounts
7984
if ($mountInfo->getMountProvider() === '' && isset($newMounts[$mountInfo->getKey()])) {
@@ -536,7 +541,13 @@ public function removeMount(string $mountPoint, ?IUser $user = null): void {
536541
}
537542
}
538543

539-
public function addMount(IUser $user, string $mountPoint, ICacheEntry $rootCacheEntry, string $mountProvider, ?int $mountId = null): void {
544+
public function addMount(
545+
IUser $user,
546+
string $mountPoint,
547+
ICacheEntry $rootCacheEntry,
548+
string $mountProvider,
549+
?int $mountId = null,
550+
): void {
540551
$query = $this->connection->getQueryBuilder();
541552
$query->insert('mounts')
542553
->values([
@@ -567,4 +578,26 @@ public function flush(): void {
567578
$this->internalPathCache = new CappedMemoryCache();
568579
$this->mountsForUsers = new CappedMemoryCache();
569580
}
581+
582+
public function getMountAtPath(IUser $user, string $mountPoint): ?ICachedMountInfo {
583+
if (isset($this->mountsForUsers[$user->getUID()])) {
584+
foreach ($this->mountsForUsers[$user->getUID()] as $mount) {
585+
if ($mount->getMountPoint() === $mountPoint) {
586+
return $mount;
587+
}
588+
}
589+
return null;
590+
}
591+
592+
$builder = $this->connection->getQueryBuilder();
593+
$query = $builder->select('storage_id', 'root_id', 'user_id', 'mount_point', 'mount_id', 'f.path', 'mount_provider_class')
594+
->from('mounts', 'm')
595+
->innerJoin('m', 'filecache', 'f', $builder->expr()->eq('m.root_id', 'f.fileid'))
596+
->where($builder->expr()->eq('user_id', $builder->createNamedParameter($user->getUID())))
597+
->andWhere($builder->expr()->eq('mount_point', $builder->createNamedParameter($mountPoint)))
598+
->setMaxResults(1);
599+
600+
$row = $query->executeQuery()->fetch();
601+
return $row ? $this->dbRowToMountInfo($row) : null;
602+
}
570603
}

‎lib/public/Files/Config/IUserMountCache.php‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -113,7 +113,10 @@ public function getUsedSpaceForUsers(array $users);
113113
public function clear(): void;
114114

115115
/**
116-
* Get all cached mounts for a user
116+
* Get the cached mount for a path
117+
*
118+
* This walks up the directly tree until a mount is found, if you only want
119+
* to get the mount at the specific path, use `getMountAtPath` instead.
117120
*
118121
* @param IUser $user
119122
* @param string $path
@@ -147,4 +150,11 @@ public function removeMount(string $mountPoint, ?IUser $user = null): void;
147150
* @since 33.0.0
148151
*/
149152
public function addMount(IUser $user, string $mountPoint, ICacheEntry $rootCacheEntry, string $mountProvider, ?int $mountId = null): void;
153+
154+
/**
155+
* Get the mount at the specified path, if any
156+
*
157+
* @since 33.0.2
158+
*/
159+
public function getMountAtPath(IUser $user, string $mountPoint): ?ICachedMountInfo;
150160
}

0 commit comments

Comments
 (0)