From ccf3b981afd969f6864d847f3a02cd1db0a8b2fb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20M=C3=BCller?= <1005065+DeepDiver1975@users.noreply.github.com> Date: Mon, 20 Nov 2023 12:48:30 +0100 Subject: [PATCH] fix: reduce sql queries in Share::getUsersSharingFile() --- lib/private/Diagnostics/Query.php | 17 ++ lib/private/Share/Share.php | 224 +++++++++----------- lib/public/IDBConnection.php | 2 +- tests/lib/Share/GetUsersSharingFileTest.php | 129 ++++++++++- tests/lib/Traits/AssertQueryCountTrait.php | 16 +- 5 files changed, 251 insertions(+), 137 deletions(-) diff --git a/lib/private/Diagnostics/Query.php b/lib/private/Diagnostics/Query.php index 7c6d2ce6a1ed..c523b651ab53 100644 --- a/lib/private/Diagnostics/Query.php +++ b/lib/private/Diagnostics/Query.php @@ -76,6 +76,23 @@ public function getDuration() { return $this->end - $this->start; } + public function getFullSql(): string { + $s = preg_replace('/\s\s+/', ' ', $this->sql); + foreach ($this->params as $i => $param) { + $param = json_encode($param); + # NOTE: this might not result in a perfect sql query because ? must not be a placeholder + # nevertheless for analysis purpose this is a good enough approach + $pos = strpos($s, '?'); + if ($pos !== false) { + $s = substr_replace($s, $param, $pos, \strlen('?')); + } else { + $s = str_replace(":$i", $param, $s); + } + } + + return $s; + } + public function jsonSerialize() { return [ 'query' => $this->sql, diff --git a/lib/private/Share/Share.php b/lib/private/Share/Share.php index 108b70a05eb1..a1019dca58b2 100644 --- a/lib/private/Share/Share.php +++ b/lib/private/Share/Share.php @@ -44,6 +44,7 @@ use Doctrine\DBAL\Connection; use OC\Files\Filesystem; use OC\Group\Group; +use OC\User\NoUserException; use OCA\FederatedFileSharing\DiscoveryManager; use OCP\DB\QueryBuilder\IQueryBuilder; use OCP\IUser; @@ -151,7 +152,7 @@ public static function getUsersSharingFile($path, $ownerUser, $includeOwner = fa if ($userObject === null) { $msg = "Backends provided no user object for $ownerUser"; \OC::$server->getLogger()->error($msg, ['app' => __CLASS__]); - throw new \OC\User\NoUserException($msg); + throw new NoUserException($msg); } $ownerUser = $userObject->getUID(); @@ -160,7 +161,7 @@ public static function getUsersSharingFile($path, $ownerUser, $includeOwner = fa $shares = $sharePaths = $fileTargets = []; $publicShare = false; $remoteShare = false; - $source = -1; + $sources = []; $cache = $mountPath = false; $view = new \OC\Files\View('/' . $ownerUser . '/files'); @@ -172,8 +173,10 @@ public static function getUsersSharingFile($path, $ownerUser, $includeOwner = fa $meta = $view->getFileInfo(\dirname($path)); } + $source = -1; if ($meta !== false) { $source = $meta['fileid']; + $sources[]= $source; $cache = new \OC\Files\Cache\Cache($meta['storage']); $mountPath = $meta->getMountPoint()->getMountPoint(); @@ -182,127 +185,96 @@ public static function getUsersSharingFile($path, $ownerUser, $includeOwner = fa } } - $connection = \OC::$server->getDatabaseConnection(); - + // let's get the parent for the next round $paths = []; - $rejected = []; - # TODO: performance consideration - build list of all parent folders up front - don't loop - while ($source !== -1) { - // Fetch all shares with another user - if (!$returnUserPaths) { - $query = \OC_DB::prepare( - 'SELECT `parent`, `share_with`, `file_source`, `file_target`, `accepted` - FROM - `*PREFIX*share` - WHERE - `item_source` = ? AND `share_type` = ? AND `item_type` IN (\'file\', \'folder\')' - ); - $result = $query->execute([$source, self::SHARE_TYPE_USER]); - } else { - $query = \OC_DB::prepare( - 'SELECT `parent`, `share_with`, `file_source`, `file_target`, `accepted` - FROM - `*PREFIX*share` - WHERE - `item_source` = ? AND `share_type` IN (?, ?) AND `item_type` IN (\'file\', \'folder\')' - ); - $result = $query->execute([$source, self::SHARE_TYPE_USER, self::$shareTypeGroupUserUnique]); - } - - if (\OCP\DB::isError($result)) { - \OCP\Util::writeLog('OCP\Share', \OC_DB::getErrorMessage(), \OCP\Util::ERROR); - } else { - while ($row = $result->fetchRow()) { - if ((int)($row['accepted']) === Constants::STATE_REJECTED) { - $rejected[]= $row['parent']; - continue; - } - $shares[] = $row['share_with']; - if ($returnUserPaths) { - $fileTargets[(int) $row['file_source']][$row['share_with']] = $row; - } - } - } - - // We also need to take group shares into account - $qb = $connection->getQueryBuilder(); - $qb->select('share_with', 'file_source', 'file_target') - ->from('share') - ->where($qb->expr()->eq('item_source', $qb->createNamedParameter($source))) - ->andWhere($qb->expr()->eq('share_type', $qb->createNamedParameter(self::SHARE_TYPE_GROUP))) - ->andWhere($qb->expr()->in('item_type', $qb->createNamedParameter(['file', 'folder'], Connection::PARAM_STR_ARRAY))); - - if (!empty($rejected)) { - $qb->andWhere($qb->expr()->notIn('id', $qb->createNamedParameter($rejected, Connection::PARAM_INT_ARRAY))); - } - $result = $qb->execute(); - - while ($row = $result->fetch()) { - $usersInGroup = self::usersInGroup($row['share_with']); - $shares = \array_merge($shares, $usersInGroup); - if ($returnUserPaths) { - foreach ($usersInGroup as $user) { - if (!isset($fileTargets[(int) $row['file_source']][$user])) { - // When the user already has an entry for this file source - // the file is either shared directly with him as well, or - // he has an exception entry (because of naming conflict). - $fileTargets[(int) $row['file_source']][$user] = $row; - } - } + if ($recursive === true) { + while ($source !== -1) { + $meta = $cache->get((int)$source); + if ($meta !== false) { + $paths[$source] = $meta['path']; + $source = (int)$meta['parent']; + $sources[]= $source; + } else { + \OC::$server->getLogger()->debug("Share::getUsersSharingFile: could not find $source in file cache."); + break; } } + } - //check for public link shares - if (!$publicShare) { - $query = \OC_DB::prepare( - ' - SELECT `share_with` - FROM `*PREFIX*share` - WHERE `item_source` = ? AND `share_type` = ? AND `item_type` IN (\'file\', \'folder\')', - 1 - ); + $connection = \OC::$server->getDatabaseConnection(); - $result = $query->execute([$source, self::SHARE_TYPE_LINK]); + $rejected = []; + // Fetch all shares with another user + $qb = $connection->getQueryBuilder(); + $qb->select('parent', 'share_with', 'file_source', 'file_target', 'accepted') + ->from('share') + ->where($qb->expr()->in('item_source', $qb->createNamedParameter($sources, Connection::PARAM_INT_ARRAY))) + ->andWhere($qb->expr()->in('item_type', $qb->createNamedParameter(['file', 'folder'], Connection::PARAM_STR_ARRAY))); - if (\OCP\DB::isError($result)) { - \OCP\Util::writeLog('OCP\Share', \OC_DB::getErrorMessage(), \OCP\Util::ERROR); - } else { - if ($result->fetchRow()) { - $publicShare = true; - } - } + if ($returnUserPaths) { + $qb->andWhere($qb->expr()->in('share_type', $qb->createNamedParameter([self::SHARE_TYPE_USER, self::$shareTypeGroupUserUnique], Connection::PARAM_INT_ARRAY))); + } else { + $qb->andWhere($qb->expr()->eq('share_type', $qb->createNamedParameter(self::SHARE_TYPE_USER))); + } + $result = $qb->execute(); + while ($row = $result->fetch()) { + if ((int)($row['accepted']) === Constants::STATE_REJECTED) { + $rejected[]= $row['parent']; + continue; + } + $shares[] = $row['share_with']; + if ($returnUserPaths) { + $fileTargets[(int) $row['file_source']][$row['share_with']] = $row; } + } + $result->free(); - //check for remote share - if (!$remoteShare) { - $query = \OC_DB::prepare( - ' - SELECT `share_with` - FROM `*PREFIX*share` - WHERE `item_source` = ? AND `share_type` = ? AND `item_type` IN (\'file\', \'folder\')', - 1 - ); + // We also need to take group shares into account + $qb = $connection->getQueryBuilder(); + $qb->select('share_with', 'file_source', 'file_target') + ->from('share') + ->where($qb->expr()->in('item_source', $qb->createNamedParameter($sources, Connection::PARAM_INT_ARRAY))) + ->andWhere($qb->expr()->eq('share_type', $qb->createNamedParameter(self::SHARE_TYPE_GROUP))) + ->andWhere($qb->expr()->in('item_type', $qb->createNamedParameter(['file', 'folder'], Connection::PARAM_STR_ARRAY))); - $result = $query->execute([$source, self::SHARE_TYPE_REMOTE]); + if (!empty($rejected)) { + $qb->andWhere($qb->expr()->notIn('id', $qb->createNamedParameter($rejected, Connection::PARAM_INT_ARRAY))); + } + $result = $qb->execute(); - if (\OCP\DB::isError($result)) { - \OCP\Util::writeLog('OCP\Share', \OC_DB::getErrorMessage(), \OCP\Util::ERROR); - } else { - if ($result->fetchRow()) { - $remoteShare = true; + while ($row = $result->fetch()) { + $usersInGroup = self::usersInGroup($row['share_with']); + $shares = \array_merge($shares, $usersInGroup); + if ($returnUserPaths) { + foreach ($usersInGroup as $user) { + if (!isset($fileTargets[(int) $row['file_source']][$user])) { + // When the user already has an entry for this file source + // the file is either shared directly with him as well, or + // he has an exception entry (because of naming conflict). + $fileTargets[(int) $row['file_source']][$user] = $row; } } } + } + $result->free(); - // let's get the parent for the next round - $meta = $cache->get((int)$source); - if ($recursive === true && $meta !== false) { - $paths[$source] = $meta['path']; - $source = (int)$meta['parent']; - } else { - $source = -1; + //check for public link shares + $qb = $connection->getQueryBuilder(); + $qb->select('share_with', 'share_type') + ->from('share') + ->where($qb->expr()->in('item_source', $qb->createNamedParameter($sources, Connection::PARAM_INT_ARRAY))) + ->andWhere($qb->expr()->in('share_type', $qb->createNamedParameter([3, 6], Connection::PARAM_INT_ARRAY))) + ->andWhere($qb->expr()->in('item_type', $qb->createNamedParameter(['file', 'folder'], Connection::PARAM_STR_ARRAY))); + $result = $qb->execute(); + while ($row = $result->fetch()) { + if ((int)($row['share_type']) === self::SHARE_TYPE_LINK) { + $publicShare = true; + } + if ((int)($row['share_type']) === self::SHARE_TYPE_REMOTE) { + $remoteShare = true; } } + $result->free(); // Include owner in list of users, if requested if ($includeOwner) { @@ -314,30 +286,28 @@ public static function getUsersSharingFile($path, $ownerUser, $includeOwner = fa $fileTargetIDs = \array_unique($fileTargetIDs); if (!empty($fileTargetIDs)) { - $query = \OC_DB::prepare( - 'SELECT `fileid`, `path` - FROM `*PREFIX*filecache` - WHERE `fileid` IN (' . \implode(',', $fileTargetIDs) . ')' - ); - $result = $query->execute(); + $qb = $connection->getQueryBuilder(); + $qb->select('fileid', 'path') + ->from('filecache') + ->where($qb->expr()->in('fileid', $qb->createNamedParameter($fileTargetIDs, Connection::PARAM_INT_ARRAY))); - if (\OCP\DB::isError($result)) { - \OCP\Util::writeLog('OCP\Share', \OC_DB::getErrorMessage(), \OCP\Util::ERROR); - } else { - while ($row = $result->fetchRow()) { - foreach ($fileTargets[$row['fileid']] as $uid => $shareData) { - if ($mountPath !== false) { - $sharedPath = $shareData['file_target']; - $sharedPath .= \substr($path, \strlen($mountPath) + \strlen($paths[$row['fileid']])); - $sharePaths[$uid] = $sharedPath; - } else { - $sharedPath = $shareData['file_target']; - $sharedPath .= \substr($path, \strlen($row['path']) -5); - $sharePaths[$uid] = $sharedPath; - } + $result = $qb->execute(); + + while ($row = $result->fetch()) { + foreach ($fileTargets[$row['fileid']] as $uid => $shareData) { + if ($mountPath !== false) { + $sharedPath = $shareData['file_target']; + # TODO: this produces spooky results + $sharedPath .= \substr($path, \strlen($mountPath) + \strlen($paths[$row['fileid']])); + $sharePaths[$uid] = $sharedPath; + } else { + $sharedPath = $shareData['file_target']; + $sharedPath .= \substr($path, \strlen($row['path']) -5); + $sharePaths[$uid] = $sharedPath; } } } + $result->free(); } if ($includeOwner) { diff --git a/lib/public/IDBConnection.php b/lib/public/IDBConnection.php index 0b444cb6df8a..acd87582ebd2 100644 --- a/lib/public/IDBConnection.php +++ b/lib/public/IDBConnection.php @@ -57,7 +57,7 @@ public function getQueryBuilder(); * @param string $sql the sql query with ? placeholder for params * @param int $limit the maximum number of rows * @param int $offset from which row we want to start - * @return \Doctrine\DBAL\Driver\Statement The prepared statement. + * @return \Doctrine\DBAL\Statement The prepared statement. * @since 6.0.0 */ public function prepare($sql, $limit=null, $offset=null); diff --git a/tests/lib/Share/GetUsersSharingFileTest.php b/tests/lib/Share/GetUsersSharingFileTest.php index 86e7be483bb9..54ba08f2aaff 100644 --- a/tests/lib/Share/GetUsersSharingFileTest.php +++ b/tests/lib/Share/GetUsersSharingFileTest.php @@ -1,8 +1,9 @@ getShareManager(); $share = $sm->newShare(); $share->setSharedBy($owner->getUID()); @@ -54,6 +56,20 @@ private static function shareWithUser(File $file, IUser $user, IUser $owner): IS return $sm->createShare($share); } + /** + * @throws GenericShareException + */ + private static function shareByLink(File $file, IUser $owner): IShare { + $sm = OC::$server->getShareManager(); + $share = $sm->newShare(); + $share->setSharedBy($owner->getUID()); + $share->setNode($file); + $share->setShareType(Constants::SHARE_TYPE_LINK); + $share->setPermissions(OCConstants::PERMISSION_READ); + + return $sm->createShare($share); + } + /** * @see https://github.com/owncloud/activity/issues/1143 * @throws NotPermittedException @@ -69,15 +85,17 @@ public function testGroupShare() : void { self::loginAsUser($alice->getUID()); $aliceFolder = OC::$server->getUserFolder($alice->getUID()); $file = self::createFileWithContent($aliceFolder, 'welcome.txt', 'loram ipsum'); - $share = self::shareWithGroup($file, $group, $alice); + self::shareWithGroup($file, $group, $alice); # test + self::trackQueries(); $result = Share::getUsersSharingFile('welcome.txt', $alice->getUID()); assertEquals([ 'users' => ['alice', 'bob'], 'public' => false, 'remote' => false ], $result); + $this->assertQueryCountLessThan(9); } /** @@ -96,16 +114,18 @@ public function testRejected() : void { $share = self::shareWithUser($file, $bob, $alice); # test accepted + self::trackQueries(); $result = Share::getUsersSharingFile('welcome.txt', $alice->getUID()); assertEquals([ 'users' => ['bob'], 'public' => false, 'remote' => false ], $result); + $this->assertQueryCountMatches(7); - # tst rejected + # test rejected $share->setState(Constants::STATE_REJECTED); - \OC::$server->getShareManager()->updateShare($share); + OC::$server->getShareManager()->updateShare($share); $result = Share::getUsersSharingFile('welcome.txt', $alice->getUID()); assertEquals([ @@ -114,4 +134,101 @@ public function testRejected() : void { 'remote' => false ], $result); } + + /** + * @throws NotPermittedException + * @throws NoUserException + * @throws GenericShareException + */ + public function testPublicLink(): void { + # setup + $alice = $this->createUser('alice'); + + self::loginAsUser($alice->getUID()); + $aliceFolder = OC::$server->getUserFolder($alice->getUID()); + $file = self::createFileWithContent($aliceFolder, 'welcome.txt', 'loram ipsum'); + self::shareByLink($file, $alice); + + # test + self::trackQueries(); + $result = Share::getUsersSharingFile('welcome.txt', $alice->getUID()); + assertEquals([ + 'users' => [], + 'public' => true, + 'remote' => false + ], $result); + $this->assertQueryCountMatches(7); + } + + /** + * @throws NotPermittedException + * @throws NoUserException + * @throws GenericShareException + */ + public function testRecursive() : void { + # setup + $alice = $this->createUser('alice'); + $bob = $this->createUser('bob'); + + self::loginAsUser($alice->getUID()); + $aliceFolder = OC::$server->getUserFolder($alice->getUID()); + $fooFolder = $aliceFolder->newFolder('foo'); + self::createFileWithContent($fooFolder, 'welcome.txt', 'loram ipsum'); + self::shareWithUser($fooFolder, $bob, $alice); + + # test recursive + $result = Share::getUsersSharingFile('foo/welcome.txt', $alice->getUID()); + assertEquals([ + 'users' => ['bob'], + 'public' => false, + 'remote' => false + ], $result); + + # test recursive + self::trackQueries(); + $result = Share::getUsersSharingFile('foo/welcome.txt', $alice->getUID(), false, false, false); + assertEquals([ + 'users' => [], + 'public' => false, + 'remote' => false + ], $result); + $this->assertQueryCountMatches(4); + } + + /** + * @throws NotPermittedException + * @throws NoUserException + * @throws GenericShareException + */ + public function testIncludeOwner() : void { + self::assertTrue(Share::isEnabled()); + # setup + $alice = $this->createUser('alice'); + $bob = $this->createUser('bob'); + + self::loginAsUser($alice->getUID()); + $aliceFolder = OC::$server->getUserFolder($alice->getUID()); + $fooFolder = $aliceFolder->newFolder('foo'); + self::createFileWithContent($fooFolder, 'welcome.txt', 'loram ipsum'); + self::shareWithUser($fooFolder, $bob, $alice); + + self::trackQueries(); + $result = Share::getUsersSharingFile('foo/welcome.txt', $alice->getUID(), true, true); + assertEquals([ + 'bob' => '/foo/welcome.txt', + 'alice' => '/foo/welcome.txt', + ], $result); + # in some cases there is an additional query required because Cache::$path_cache is not holding the path in question + $this->assertQueryCountLessThan(11); + + # let's see how bob is doing .... + self::loginAsUser($bob->getUID()); + self::trackQueries(); + $result = Share::getUsersSharingFile('foo/welcome.txt', $bob->getUID(), true, true); + assertEquals([ + 'bob' => '/foo/welcome.txt', + ], $result); + # in some cases there is an additional query required because Cache::$path_cache is not holding the path in question + $this->assertQueryCountLessThan(11); + } } diff --git a/tests/lib/Traits/AssertQueryCountTrait.php b/tests/lib/Traits/AssertQueryCountTrait.php index 135b8f3f99d9..2568292b3d72 100644 --- a/tests/lib/Traits/AssertQueryCountTrait.php +++ b/tests/lib/Traits/AssertQueryCountTrait.php @@ -3,6 +3,7 @@ namespace Test\Traits; use Closure; +use OC\Diagnostics\Query; use function PHPUnit\Framework\assertEquals; use function PHPUnit\Framework\assertGreaterThan; use function PHPUnit\Framework\assertLessThan; @@ -33,7 +34,7 @@ public function assertQueryCountMatches(int $count, Closure $closure = null): vo } self::ensureTacking(); - assertEquals($count, self::getQueryCount()); + assertEquals($count, self::getQueryCount(), self::buildAssertMessage()); if ($closure) { self::flushQueryLog(); @@ -48,7 +49,7 @@ public function assertQueryCountLessThan(int $count, Closure $closure = null): v } self::ensureTacking(); - assertLessThan($count, self::getQueryCount()); + assertLessThan($count, self::getQueryCount(), self::buildAssertMessage()); if ($closure) { self::flushQueryLog(); @@ -63,7 +64,7 @@ public function assertQueryCountGreaterThan(int $count, Closure $closure = null) } self::ensureTacking(); - assertGreaterThan($count, self::getQueryCount()); + assertGreaterThan($count, self::getQueryCount(), self::buildAssertMessage()); if ($closure) { self::flushQueryLog(); @@ -80,6 +81,11 @@ public static function getQueriesExecuted(): array { return \OC::$server->getQueryLogger()->getQueries(); } + public static function getQueriesExecutedHumanReadable(): array { + $queries = self::getQueriesExecuted(); + return array_map(static fn (Query $q) => $q->getFullSql(), $queries); + } + public static function getQueryCount(): int { return \count(self::getQueriesExecuted()); } @@ -91,4 +97,8 @@ private static function flushQueryLog(): void { private static function ensureTacking(): void { assertTrue(self::$trackingStarted, "SQl query tracking not enabled."); } + + private static function buildAssertMessage(): string { + return "Recorded queries: " . PHP_EOL . implode(PHP_EOL, self::getQueriesExecutedHumanReadable()); + } }