From 9ea3f4ab8f79ca7e85376e25540b77d3ca56f97c Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Wed, 7 Feb 2024 14:44:24 +0100 Subject: perf: use lazy user in UserMountCache for getting user for cached mount Signed-off-by: Robin Appelman --- lib/private/Files/Config/UserMountCache.php | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/lib/private/Files/Config/UserMountCache.php b/lib/private/Files/Config/UserMountCache.php index 27d84e93838..19fc1a2b573 100644 --- a/lib/private/Files/Config/UserMountCache.php +++ b/lib/private/Files/Config/UserMountCache.php @@ -28,6 +28,7 @@ */ namespace OC\Files\Config; +use OC\User\LazyUser; use OCP\Cache\CappedMemoryCache; use OCP\DB\QueryBuilder\IQueryBuilder; use OCP\Diagnostics\IEventLogger; @@ -213,13 +214,10 @@ class UserMountCache implements IUserMountCache { /** * @param array $row * @param (callable(CachedMountInfo): string)|null $pathCallback - * @return CachedMountInfo|null + * @return CachedMountInfo */ - private function dbRowToMountInfo(array $row, ?callable $pathCallback = null): ?ICachedMountInfo { - $user = $this->userManager->get($row['user_id']); - if (is_null($user)) { - return null; - } + private function dbRowToMountInfo(array $row, ?callable $pathCallback = null): ICachedMountInfo { + $user = new LazyUser($row['user_id'], $this->userManager); $mount_id = $row['mount_id']; if (!is_null($mount_id)) { $mount_id = (int)$mount_id; -- cgit v1.2.3 From e33b32095ea32177219eb863fe91f62b601f4324 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Wed, 7 Feb 2024 14:45:47 +0100 Subject: Revert "Filter mounts for file id before trying to get user information" This reverts commit 1e2cf820c89b774d0a8d6f85bfd8d2fd1b4ab2d6. Signed-off-by: Robin Appelman --- lib/private/Files/Config/UserMountCache.php | 20 ++++---------------- 1 file changed, 4 insertions(+), 16 deletions(-) diff --git a/lib/private/Files/Config/UserMountCache.php b/lib/private/Files/Config/UserMountCache.php index 19fc1a2b573..6c64dc4973f 100644 --- a/lib/private/Files/Config/UserMountCache.php +++ b/lib/private/Files/Config/UserMountCache.php @@ -368,30 +368,18 @@ class UserMountCache implements IUserMountCache { } catch (NotFoundException $e) { return []; } - $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('storage_id', $builder->createPositionalParameter($storageId, IQueryBuilder::PARAM_INT))); + $mountsForStorage = $this->getMountsForStorageId($storageId, $user); - if ($user) { - $query->andWhere($builder->expr()->eq('user_id', $builder->createPositionalParameter($user))); - } - - $result = $query->execute(); - $rows = $result->fetchAll(); - $result->closeCursor(); // filter mounts that are from the same storage but a different directory - $filteredMounts = array_filter($rows, function (array $row) use ($internalPath, $fileId) { - if ($fileId === (int)$row['root_id']) { + $filteredMounts = array_filter($mountsForStorage, function (ICachedMountInfo $mount) use ($internalPath, $fileId) { + if ($fileId === $mount->getRootId()) { return true; } - $internalMountPath = $row['path'] ?? ''; + $internalMountPath = $mount->getRootInternalPath(); return $internalMountPath === '' || substr($internalPath, 0, strlen($internalMountPath) + 1) === $internalMountPath . '/'; }); - $filteredMounts = array_filter(array_map([$this, 'dbRowToMountInfo'], $filteredMounts)); return array_map(function (ICachedMountInfo $mount) use ($internalPath) { return new CachedMountFileInfo( $mount->getUser(), -- cgit v1.2.3 From 51019fda7a74816ca38962d899e7b641a4d5f88e Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Wed, 7 Feb 2024 14:49:31 +0100 Subject: fix: clearify logic around getMountsForFileId filtering Signed-off-by: Robin Appelman --- lib/private/Files/Config/UserMountCache.php | 11 +++++++++-- tests/lib/Files/Config/UserMountCacheTest.php | 22 +++++++++++----------- 2 files changed, 20 insertions(+), 13 deletions(-) diff --git a/lib/private/Files/Config/UserMountCache.php b/lib/private/Files/Config/UserMountCache.php index 6c64dc4973f..8275eee7b9f 100644 --- a/lib/private/Files/Config/UserMountCache.php +++ b/lib/private/Files/Config/UserMountCache.php @@ -251,6 +251,9 @@ class UserMountCache implements IUserMountCache { */ public function getMountsForUser(IUser $user) { $userUID = $user->getUID(); + if (!$this->userManager->userExists($userUID)) { + return []; + } if (!isset($this->mountsForUsers[$userUID])) { $builder = $this->connection->getQueryBuilder(); $query = $builder->select('storage_id', 'root_id', 'user_id', 'mount_point', 'mount_id', 'mount_provider_class') @@ -370,14 +373,18 @@ class UserMountCache implements IUserMountCache { } $mountsForStorage = $this->getMountsForStorageId($storageId, $user); - // filter mounts that are from the same storage but a different directory + // filter mounts that are from the same storage but not a parent of the file we care about $filteredMounts = array_filter($mountsForStorage, function (ICachedMountInfo $mount) use ($internalPath, $fileId) { if ($fileId === $mount->getRootId()) { return true; } $internalMountPath = $mount->getRootInternalPath(); - return $internalMountPath === '' || substr($internalPath, 0, strlen($internalMountPath) + 1) === $internalMountPath . '/'; + return $internalMountPath === '' || str_starts_with($internalPath, $internalMountPath . '/'); + }); + + $filteredMounts = array_filter($filteredMounts, function (ICachedMountInfo $mount) { + return $this->userManager->userExists($mount->getUser()->getUID()); }); return array_map(function (ICachedMountInfo $mount) use ($internalPath) { diff --git a/tests/lib/Files/Config/UserMountCacheTest.php b/tests/lib/Files/Config/UserMountCacheTest.php index 10984e1d578..9e910f4f47f 100644 --- a/tests/lib/Files/Config/UserMountCacheTest.php +++ b/tests/lib/Files/Config/UserMountCacheTest.php @@ -137,7 +137,7 @@ class UserMountCacheTest extends TestCase { $this->assertCount(1, $cachedMounts); $cachedMount = $cachedMounts[$this->keyForMount($mount)]; $this->assertEquals('/asd/', $cachedMount->getMountPoint()); - $this->assertEquals($user, $cachedMount->getUser()); + $this->assertEquals($user->getUID(), $cachedMount->getUser()->getUID()); $this->assertEquals($storage->getCache()->getId(''), $cachedMount->getRootId()); $this->assertEquals($storage->getStorageCache()->getNumericId(), $cachedMount->getStorageId()); } @@ -161,7 +161,7 @@ class UserMountCacheTest extends TestCase { $this->assertCount(1, $cachedMounts); $cachedMount = $cachedMounts[$this->keyForMount($mount)]; $this->assertEquals('/asd/', $cachedMount->getMountPoint()); - $this->assertEquals($user, $cachedMount->getUser()); + $this->assertEquals($user->getUID(), $cachedMount->getUser()->getUID()); $this->assertEquals($storage->getCache()->getId(''), $cachedMount->getRootId()); $this->assertEquals($storage->getStorageCache()->getNumericId(), $cachedMount->getStorageId()); } @@ -253,13 +253,13 @@ class UserMountCacheTest extends TestCase { $this->assertCount(2, $cachedMounts); $this->assertEquals('/foo/', $cachedMounts[$this->keyForMount($mount1)]->getMountPoint()); - $this->assertEquals($user1, $cachedMounts[$this->keyForMount($mount1)]->getUser()); + $this->assertEquals($user1->getUID(), $cachedMounts[$this->keyForMount($mount1)]->getUser()->getUID()); $this->assertEquals($id1, $cachedMounts[$this->keyForMount($mount1)]->getRootId()); $this->assertEquals(1, $cachedMounts[$this->keyForMount($mount1)]->getStorageId()); $this->assertEquals('', $cachedMounts[$this->keyForMount($mount1)]->getRootInternalPath()); $this->assertEquals('/bar/', $cachedMounts[$this->keyForMount($mount2)]->getMountPoint()); - $this->assertEquals($user1, $cachedMounts[$this->keyForMount($mount2)]->getUser()); + $this->assertEquals($user1->getUID(), $cachedMounts[$this->keyForMount($mount2)]->getUser()->getUID()); $this->assertEquals($id2, $cachedMounts[$this->keyForMount($mount2)]->getRootId()); $this->assertEquals(2, $cachedMounts[$this->keyForMount($mount2)]->getStorageId()); $this->assertEquals('foo/bar', $cachedMounts[$this->keyForMount($mount2)]->getRootInternalPath()); @@ -288,12 +288,12 @@ class UserMountCacheTest extends TestCase { $this->assertCount(2, $cachedMounts); $this->assertEquals('/bar/', $cachedMounts[0]->getMountPoint()); - $this->assertEquals($user1, $cachedMounts[0]->getUser()); + $this->assertEquals($user1->getUID(), $cachedMounts[0]->getUser()->getUID()); $this->assertEquals($id2, $cachedMounts[0]->getRootId()); $this->assertEquals(2, $cachedMounts[0]->getStorageId()); $this->assertEquals('/bar/', $cachedMounts[1]->getMountPoint()); - $this->assertEquals($user2, $cachedMounts[1]->getUser()); + $this->assertEquals($user2->getUID(), $cachedMounts[1]->getUser()->getUID()); $this->assertEquals($id2, $cachedMounts[1]->getRootId()); $this->assertEquals(2, $cachedMounts[1]->getStorageId()); } @@ -318,12 +318,12 @@ class UserMountCacheTest extends TestCase { $this->assertCount(2, $cachedMounts); $this->assertEquals('/bar/', $cachedMounts[0]->getMountPoint()); - $this->assertEquals($user1, $cachedMounts[0]->getUser()); + $this->assertEquals($user1->getUID(), $cachedMounts[0]->getUser()->getUID()); $this->assertEquals($id2, $cachedMounts[0]->getRootId()); $this->assertEquals(2, $cachedMounts[0]->getStorageId()); $this->assertEquals('/bar/', $cachedMounts[1]->getMountPoint()); - $this->assertEquals($user2, $cachedMounts[1]->getUser()); + $this->assertEquals($user2->getUID(), $cachedMounts[1]->getUser()->getUID()); $this->assertEquals($id2, $cachedMounts[1]->getRootId()); $this->assertEquals(2, $cachedMounts[1]->getStorageId()); } @@ -378,7 +378,7 @@ class UserMountCacheTest extends TestCase { $this->assertCount(1, $cachedMounts); $this->assertEquals('/foo/', $cachedMounts[0]->getMountPoint()); - $this->assertEquals($user1, $cachedMounts[0]->getUser()); + $this->assertEquals($user1->getUID(), $cachedMounts[0]->getUser()->getUID()); $this->assertEquals($rootId, $cachedMounts[0]->getRootId()); $this->assertEquals(2, $cachedMounts[0]->getStorageId()); } @@ -400,7 +400,7 @@ class UserMountCacheTest extends TestCase { $this->assertCount(1, $cachedMounts); $this->assertEquals('/foo/', $cachedMounts[0]->getMountPoint()); - $this->assertEquals($user1, $cachedMounts[0]->getUser()); + $this->assertEquals($user1->getUID(), $cachedMounts[0]->getUser()->getUID()); $this->assertEquals($rootId, $cachedMounts[0]->getRootId()); $this->assertEquals(2, $cachedMounts[0]->getStorageId()); $this->assertEquals('foo/bar', $cachedMounts[0]->getInternalPath()); @@ -433,7 +433,7 @@ class UserMountCacheTest extends TestCase { $this->assertCount(1, $cachedMounts); $this->assertEquals('/', $cachedMounts[0]->getMountPoint()); - $this->assertEquals($user1, $cachedMounts[0]->getUser()); + $this->assertEquals($user1->getUID(), $cachedMounts[0]->getUser()->getUID()); $this->assertEquals($folderId, $cachedMounts[0]->getRootId()); $this->assertEquals(2, $cachedMounts[0]->getStorageId()); $this->assertEquals('foo', $cachedMounts[0]->getRootInternalPath()); -- cgit v1.2.3