From bd101e0cdebc2211d0534fe32a6ed267fb1bc307 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Fri, 9 Feb 2024 13:13:01 +0100 Subject: [PATCH 1/5] perf(sharing): Remove the item_type from the query so other indices can be used Signed-off-by: Joas Schilling --- lib/private/Share20/DefaultShareProvider.php | 48 +++++++++----------- 1 file changed, 21 insertions(+), 27 deletions(-) diff --git a/lib/private/Share20/DefaultShareProvider.php b/lib/private/Share20/DefaultShareProvider.php index 55ac3eda644e1..2d62fe7ea631e 100644 --- a/lib/private/Share20/DefaultShareProvider.php +++ b/lib/private/Share20/DefaultShareProvider.php @@ -656,11 +656,7 @@ public function getSharesInFolder($userId, Folder $node, $reshares, $shallow = t 'f.fileid', 'f.path', 'f.permissions AS f_permissions', 'f.storage', 'f.path_hash', 'f.parent AS f_parent', 'f.name', 'f.mimetype', 'f.mimepart', 'f.size', 'f.mtime', 'f.storage_mtime', 'f.encrypted', 'f.unencrypted_size', 'f.etag', 'f.checksum') - ->from('share', 's') - ->andWhere($qb->expr()->orX( - $qb->expr()->eq('item_type', $qb->createNamedParameter('file')), - $qb->expr()->eq('item_type', $qb->createNamedParameter('folder')) - )); + ->from('share', 's'); $qb->andWhere($qb->expr()->orX( $qb->expr()->eq('share_type', $qb->createNamedParameter(IShare::TYPE_USER)), @@ -722,6 +718,9 @@ public function getSharesInFolder($userId, Folder $node, $reshares, $shallow = t $qb->setParameter('chunk', $chunk, IQueryBuilder::PARAM_INT_ARRAY); $cursor = $qb->executeQuery(); while ($data = $cursor->fetch()) { + if ($data['item_type'] !== 'file' && $data['item_type'] !== 'folder') { + continue; + } $shares[$data['fileid']][] = $this->createShare($data); } $cursor->closeCursor(); @@ -737,12 +736,7 @@ public function getSharesBy($userId, $shareType, $node, $reshares, $limit, $offs $qb = $this->dbConn->getQueryBuilder(); $qb->select('*') ->from('share') - ->andWhere($qb->expr()->orX( - $qb->expr()->eq('item_type', $qb->createNamedParameter('file')), - $qb->expr()->eq('item_type', $qb->createNamedParameter('folder')) - )); - - $qb->andWhere($qb->expr()->eq('share_type', $qb->createNamedParameter($shareType))); + ->andWhere($qb->expr()->eq('share_type', $qb->createNamedParameter($shareType))); /** * Reshares for this user are shares where they are the owner. @@ -774,6 +768,9 @@ public function getSharesBy($userId, $shareType, $node, $reshares, $limit, $offs $cursor = $qb->execute(); $shares = []; while ($data = $cursor->fetch()) { + if ($data['item_type'] !== 'file' && $data['item_type'] !== 'folder') { + continue; + } $shares[] = $this->createShare($data); } $cursor->closeCursor(); @@ -1030,10 +1027,6 @@ public function getShareByToken($token) { ->from('share') ->where($qb->expr()->eq('share_type', $qb->createNamedParameter(IShare::TYPE_LINK))) ->andWhere($qb->expr()->eq('token', $qb->createNamedParameter($token))) - ->andWhere($qb->expr()->orX( - $qb->expr()->eq('item_type', $qb->createNamedParameter('file')), - $qb->expr()->eq('item_type', $qb->createNamedParameter('folder')) - )) ->execute(); $data = $cursor->fetch(); @@ -1043,6 +1036,9 @@ public function getShareByToken($token) { } try { + if ($data['item_type'] !== 'file' && $data['item_type'] !== 'folder') { + throw new ShareNotFound(); + } $share = $this->createShare($data); } catch (InvalidShare $e) { throw new ShareNotFound(); @@ -1151,15 +1147,14 @@ private function resolveGroupShares($shares, $userId) { $query = $qb->select('*') ->from('share') ->where($qb->expr()->in('parent', $qb->createNamedParameter($ids, IQueryBuilder::PARAM_INT_ARRAY))) - ->andWhere($qb->expr()->eq('share_with', $qb->createNamedParameter($userId))) - ->andWhere($qb->expr()->orX( - $qb->expr()->eq('item_type', $qb->createNamedParameter('file')), - $qb->expr()->eq('item_type', $qb->createNamedParameter('folder')) - )); + ->andWhere($qb->expr()->eq('share_with', $qb->createNamedParameter($userId))); $stmt = $query->execute(); while ($data = $stmt->fetch()) { + if ($data['item_type'] !== 'file' && $data['item_type'] !== 'folder') { + continue; + } $shareMap[$data['parent']]->setPermissions((int)$data['permissions']); $shareMap[$data['parent']]->setStatus((int)$data['accepted']); $shareMap[$data['parent']]->setTarget($data['file_target']); @@ -1346,21 +1341,20 @@ public function getAccessList($nodes, $currentAccess) { $or->add($qb->expr()->eq('share_type', $qb->createNamedParameter(IShare::TYPE_USERGROUP))); } - $qb->select('id', 'parent', 'share_type', 'share_with', 'file_source', 'file_target', 'permissions') + $qb->select('id', 'parent', 'share_type', 'share_with', 'file_source', 'file_target', 'permissions', 'item_type') ->from('share') ->where( $or ) - ->andWhere($qb->expr()->in('file_source', $qb->createNamedParameter($ids, IQueryBuilder::PARAM_INT_ARRAY))) - ->andWhere($qb->expr()->orX( - $qb->expr()->eq('item_type', $qb->createNamedParameter('file')), - $qb->expr()->eq('item_type', $qb->createNamedParameter('folder')) - )); - $cursor = $qb->execute(); + ->andWhere($qb->expr()->in('file_source', $qb->createNamedParameter($ids, IQueryBuilder::PARAM_INT_ARRAY))); + $cursor = $qb->executeQuery(); $users = []; $link = false; while ($row = $cursor->fetch()) { + if ($row['item_type'] !== 'file' && $row['item_type'] !== 'folder') { + continue; + } $type = (int)$row['share_type']; if ($type === IShare::TYPE_USER) { $uid = $row['share_with']; From 4ef47f029760484d497cd8be68ed31e59dd1a660 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Fri, 9 Feb 2024 13:13:54 +0100 Subject: [PATCH 2/5] fix(sharing): Fix the type handling to the public interface Signed-off-by: Joas Schilling --- lib/private/Share20/DefaultShareProvider.php | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/private/Share20/DefaultShareProvider.php b/lib/private/Share20/DefaultShareProvider.php index 2d62fe7ea631e..fb511f9dcbd27 100644 --- a/lib/private/Share20/DefaultShareProvider.php +++ b/lib/private/Share20/DefaultShareProvider.php @@ -1115,16 +1115,16 @@ private function createShare($data) { } /** - * @param Share[] $shares + * @param IShare[] $shares * @param $userId - * @return Share[] The updates shares if no update is found for a share return the original + * @return IShare[] The updates shares if no update is found for a share return the original */ private function resolveGroupShares($shares, $userId) { $result = []; $start = 0; while (true) { - /** @var Share[] $shareSlice */ + /** @var IShare[] $shareSlice */ $shareSlice = array_slice($shares, $start, 100); $start += 100; @@ -1134,7 +1134,7 @@ private function resolveGroupShares($shares, $userId) { /** @var int[] $ids */ $ids = []; - /** @var Share[] $shareMap */ + /** @var IShare[] $shareMap */ $shareMap = []; foreach ($shareSlice as $share) { From 17b8f15ea807f476fb64784b9a21ce30193448f8 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Fri, 9 Feb 2024 13:21:56 +0100 Subject: [PATCH 3/5] perf(sharing): Split the getSharedWith() method First of all the method was huge and was handling multiple usecases: - User and group shares - "get all" (mount logic) and "get some" To improve the performance of the "get all" it was extracted into yet another method, to allow to further optimize it. E.g. the query was split into reading the shares and reading the filecache, which will be required for future work with sharding anyway. Without limits we also don't need to sort the entries. Signed-off-by: Joas Schilling --- lib/private/Share20/DefaultShareProvider.php | 357 ++++++++++++++----- 1 file changed, 274 insertions(+), 83 deletions(-) diff --git a/lib/private/Share20/DefaultShareProvider.php b/lib/private/Share20/DefaultShareProvider.php index fb511f9dcbd27..a82aba056ea29 100644 --- a/lib/private/Share20/DefaultShareProvider.php +++ b/lib/private/Share20/DefaultShareProvider.php @@ -886,11 +886,113 @@ private function isAccessibleResult($data) { * @inheritdoc */ public function getSharedWith($userId, $shareType, $node, $limit, $offset) { + if ($shareType === IShare::TYPE_USER) { + if ($limit === -1 && $node === null && $offset === 0) { + return $this->getAllSharedWithUser((string) $userId); + } + return $this->getSharedWithUser($userId, $node, $limit, $offset); + } + if ($shareType === IShare::TYPE_GROUP) { + if ($limit === -1 && $node === null && $offset === 0) { + return $this->getAllSharedWithGroups((string) $userId); + } + return $this->getSharedWithGroups($userId, $node, $limit, $offset); + } + throw new BackendError('Invalid backend'); + } + + /** + * Get limited user-shares shared with the given user + * + * @param string $userId get shares where this user is the recipient + * @param Node|null $node + * @param int $limit The max number of entries returned, -1 for all + * @param int $offset + * @return IShare[] + */ + public function getSharedWithUser($userId, $node, $limit, $offset): array { /** @var Share[] $shares */ $shares = []; - if ($shareType === IShare::TYPE_USER) { - //Get shares directly with this user + //Get shares directly with this user + $qb = $this->dbConn->getQueryBuilder(); + $qb->select('s.*', + 'f.fileid', 'f.path', 'f.permissions AS f_permissions', 'f.storage', 'f.path_hash', + 'f.parent AS f_parent', 'f.name', 'f.mimetype', 'f.mimepart', 'f.size', 'f.mtime', 'f.storage_mtime', + 'f.encrypted', 'f.unencrypted_size', 'f.etag', 'f.checksum' + ) + ->selectAlias('st.id', 'storage_string_id') + ->from('share', 's') + ->leftJoin('s', 'filecache', 'f', $qb->expr()->eq('s.file_source', 'f.fileid')) + ->leftJoin('f', 'storages', 'st', $qb->expr()->eq('f.storage', 'st.numeric_id')); + + // Order by id + $qb->orderBy('s.id'); + + // Set limit and offset + if ($limit !== -1) { + $qb->setMaxResults($limit); + } + $qb->setFirstResult($offset); + + $qb->where($qb->expr()->eq('share_type', $qb->createNamedParameter(IShare::TYPE_USER))) + ->andWhere($qb->expr()->eq('share_with', $qb->createNamedParameter($userId))) + ->andWhere($qb->expr()->orX( + $qb->expr()->eq('item_type', $qb->createNamedParameter('file')), + $qb->expr()->eq('item_type', $qb->createNamedParameter('folder')) + )); + + // Filter by node if provided + if ($node !== null) { + $qb->andWhere($qb->expr()->eq('file_source', $qb->createNamedParameter($node->getId()))); + } + + $cursor = $qb->execute(); + + while ($data = $cursor->fetch()) { + if ($data['fileid'] && $data['path'] === null) { + $data['path'] = (string) $data['path']; + $data['name'] = (string) $data['name']; + $data['checksum'] = (string) $data['checksum']; + } + if ($this->isAccessibleResult($data)) { + $shares[] = $this->createShare($data); + } + } + $cursor->closeCursor(); + + return $shares; + } + + + /** + * Get limited group-shares shared with the groups of a given user + * + * @param string $userId get shares where this user is the recipient + * @param Node|null $node + * @param int $limit The max number of entries returned, -1 for all + * @param int $offset + * @return IShare[] + */ + public function getSharedWithGroups($userId, $node, $limit, $offset): array { + /** @var Share[] $shares */ + $shares = []; + + $user = $this->userManager->get($userId); + $allGroups = ($user instanceof IUser) ? $this->groupManager->getUserGroupIds($user) : []; + + /** @var Share[] $shares2 */ + $shares2 = []; + + $start = 0; + while (true) { + $groups = array_slice($allGroups, $start, 1000); + $start += 1000; + + if ($groups === []) { + break; + } + $qb = $this->dbConn->getQueryBuilder(); $qb->select('s.*', 'f.fileid', 'f.path', 'f.permissions AS f_permissions', 'f.storage', 'f.path_hash', @@ -900,117 +1002,206 @@ public function getSharedWith($userId, $shareType, $node, $limit, $offset) { ->selectAlias('st.id', 'storage_string_id') ->from('share', 's') ->leftJoin('s', 'filecache', 'f', $qb->expr()->eq('s.file_source', 'f.fileid')) - ->leftJoin('f', 'storages', 'st', $qb->expr()->eq('f.storage', 'st.numeric_id')); - - // Order by id - $qb->orderBy('s.id'); + ->leftJoin('f', 'storages', 'st', $qb->expr()->eq('f.storage', 'st.numeric_id')) + ->orderBy('s.id') + ->setFirstResult(0); - // Set limit and offset if ($limit !== -1) { - $qb->setMaxResults($limit); + $qb->setMaxResults($limit - count($shares)); } - $qb->setFirstResult($offset); - - $qb->where($qb->expr()->eq('share_type', $qb->createNamedParameter(IShare::TYPE_USER))) - ->andWhere($qb->expr()->eq('share_with', $qb->createNamedParameter($userId))) - ->andWhere($qb->expr()->orX( - $qb->expr()->eq('item_type', $qb->createNamedParameter('file')), - $qb->expr()->eq('item_type', $qb->createNamedParameter('folder')) - )); // Filter by node if provided if ($node !== null) { $qb->andWhere($qb->expr()->eq('file_source', $qb->createNamedParameter($node->getId()))); } - $cursor = $qb->execute(); + $groups = array_filter($groups); + + $qb->andWhere($qb->expr()->eq('share_type', $qb->createNamedParameter(IShare::TYPE_GROUP))) + ->andWhere($qb->expr()->in('share_with', $qb->createNamedParameter( + $groups, + IQueryBuilder::PARAM_STR_ARRAY + ))) + ->andWhere($qb->expr()->orX( + $qb->expr()->eq('item_type', $qb->createNamedParameter('file')), + $qb->expr()->eq('item_type', $qb->createNamedParameter('folder')) + )); + + $cursor = $qb->execute(); while ($data = $cursor->fetch()) { - if ($data['fileid'] && $data['path'] === null) { - $data['path'] = (string) $data['path']; - $data['name'] = (string) $data['name']; - $data['checksum'] = (string) $data['checksum']; + if ($offset > 0) { + $offset--; + continue; } + if ($this->isAccessibleResult($data)) { - $shares[] = $this->createShare($data); + $shares2[] = $this->createShare($data); } } $cursor->closeCursor(); - } elseif ($shareType === IShare::TYPE_GROUP) { - $user = $this->userManager->get($userId); - $allGroups = ($user instanceof IUser) ? $this->groupManager->getUserGroupIds($user) : []; + } - /** @var Share[] $shares2 */ - $shares2 = []; + /* + * Resolve all group shares to user specific shares + */ + $shares = $this->resolveGroupShares($shares2, $userId); + + return $shares; + } - $start = 0; - while (true) { - $groups = array_slice($allGroups, $start, 1000); - $start += 1000; + /** + * Get all user-shares shared with the given user + * + * @param string $userId get shares where this user is the recipient + * @return IShare[] + */ + public function getAllSharedWithUser(string $userId): array { + //Get shares directly with this user + $query = $this->dbConn->getQueryBuilder(); + $query->select('*') + ->from('share') + ->where($query->expr()->eq('share_type', $query->createNamedParameter(IShare::TYPE_USER))) + ->andWhere($query->expr()->eq('share_with', $query->createNamedParameter($userId))); - if ($groups === []) { - break; - } + /** @var array $shareRows */ + $shareRows = []; - $qb = $this->dbConn->getQueryBuilder(); - $qb->select('s.*', - 'f.fileid', 'f.path', 'f.permissions AS f_permissions', 'f.storage', 'f.path_hash', - 'f.parent AS f_parent', 'f.name', 'f.mimetype', 'f.mimepart', 'f.size', 'f.mtime', 'f.storage_mtime', - 'f.encrypted', 'f.unencrypted_size', 'f.etag', 'f.checksum' - ) - ->selectAlias('st.id', 'storage_string_id') - ->from('share', 's') - ->leftJoin('s', 'filecache', 'f', $qb->expr()->eq('s.file_source', 'f.fileid')) - ->leftJoin('f', 'storages', 'st', $qb->expr()->eq('f.storage', 'st.numeric_id')) - ->orderBy('s.id') - ->setFirstResult(0); - - if ($limit !== -1) { - $qb->setMaxResults($limit - count($shares)); - } + /** @var array $fileData */ + $fileData = []; - // Filter by node if provided - if ($node !== null) { - $qb->andWhere($qb->expr()->eq('file_source', $qb->createNamedParameter($node->getId()))); + $result = $query->executeQuery(); + while ($row = $result->fetch()) { + if ($row['item_type'] !== 'file' && $row['item_type'] !== 'folder') { + continue; + } + + $shareRows[(int)$row['id']] = $row; + $fileData[(int)$row['file_source']] = null; + } + $result->closeCursor(); + + if (empty($fileData)) { + return []; + } + + $queryFileCache = $this->dbConn->getQueryBuilder(); + $queryFileCache->select('f.fileid', 'f.path', 'f.permissions AS f_permissions', 'f.storage', 'f.path_hash', + 'f.parent AS f_parent', 'f.name', 'f.mimetype', 'f.mimepart', 'f.size', 'f.mtime', 'f.storage_mtime', + 'f.encrypted', 'f.unencrypted_size', 'f.etag', 'f.checksum') + ->selectAlias('st.id', 'storage_string_id') + ->from('filecache', 'f') + ->leftJoin('f', 'storages', 'st', $queryFileCache->expr()->eq('f.storage', 'st.numeric_id')) + ->where($queryFileCache->expr()->in('f.fileid', $queryFileCache->createParameter('fileIds'))); + + $allFileIds = array_keys($fileData); + foreach (array_chunk($allFileIds, 1000) as $fileIds) { + // Filecache and storage info + $queryFileCache->setParameter('fileIds', $fileIds, IQueryBuilder::PARAM_INT_ARRAY); + + $result = $queryFileCache->executeQuery(); + while ($row = $result->fetch()) { + if (!$this->isAccessibleResult($row)) { + continue; } + $fileData[(int) $row['fileid']] = $row; + } + $result->closeCursor(); + } - $groups = array_filter($groups); - - $qb->andWhere($qb->expr()->eq('share_type', $qb->createNamedParameter(IShare::TYPE_GROUP))) - ->andWhere($qb->expr()->in('share_with', $qb->createNamedParameter( - $groups, - IQueryBuilder::PARAM_STR_ARRAY - ))) - ->andWhere($qb->expr()->orX( - $qb->expr()->eq('item_type', $qb->createNamedParameter('file')), - $qb->expr()->eq('item_type', $qb->createNamedParameter('folder')) - )); - - $cursor = $qb->execute(); - while ($data = $cursor->fetch()) { - if ($offset > 0) { - $offset--; - continue; - } - - if ($this->isAccessibleResult($data)) { - $shares2[] = $this->createShare($data); - } + /** @var IShare[] $shares */ + $shares = []; + foreach ($shareRows as $row) { + if (empty($fileData[(int)$row['file_source']])) { + continue; + } + $shares[] = $this->createShare(array_merge($row, $fileData[(int)$row['file_source']])); + } + + return $shares; + } + + + /** + * Get all group-shares shared with the groups of a given user + * + * @param string $userId get shares where this user is the recipient + * @return IShare[] + */ + public function getAllSharedWithGroups($userId): array { + $user = $this->userManager->get($userId); + $allGroups = ($user instanceof IUser) ? $this->groupManager->getUserGroupIds($user) : []; + + $query = $this->dbConn->getQueryBuilder(); + $query->select('s.*') + ->from('share', 's') + ->where($query->expr()->eq('s.share_type', $query->createNamedParameter(IShare::TYPE_GROUP))) + ->andWhere($query->expr()->in('s.share_with', $query->createParameter('groups'))); + + /** @var array $shareRows */ + $shareRows = []; + + /** @var array $fileData */ + $fileData = []; + + foreach (array_chunk($allGroups, 1000) as $groups) { + $query->setParameter('groups', $groups, IQueryBuilder::PARAM_STR_ARRAY); + + $result = $query->executeQuery(); + while ($row = $result->fetch()) { + if ($row['item_type'] !== 'file' && $row['item_type'] !== 'folder') { + continue; } - $cursor->closeCursor(); + + $shareRows[(int)$row['id']] = $row; + $fileData[(int)$row['file_source']] = null; } + $result->closeCursor(); + } - /* - * Resolve all group shares to user specific shares - */ - $shares = $this->resolveGroupShares($shares2, $userId); - } else { - throw new BackendError('Invalid backend'); + if (empty($fileData)) { + return []; } + $queryFileCache = $this->dbConn->getQueryBuilder(); + $queryFileCache->select('f.fileid', 'f.path', 'f.permissions AS f_permissions', 'f.storage', 'f.path_hash', + 'f.parent AS f_parent', 'f.name', 'f.mimetype', 'f.mimepart', 'f.size', 'f.mtime', 'f.storage_mtime', + 'f.encrypted', 'f.unencrypted_size', 'f.etag', 'f.checksum') + ->selectAlias('st.id', 'storage_string_id') + ->from('filecache', 'f') + ->leftJoin('f', 'storages', 'st', $queryFileCache->expr()->eq('f.storage', 'st.numeric_id')) + ->where($queryFileCache->expr()->in('f.fileid', $queryFileCache->createParameter('fileIds'))); + + $allFileIds = array_keys($fileData); + foreach (array_chunk($allFileIds, 1000) as $fileIds) { + // Filecache and storage info + $queryFileCache->setParameter('fileIds', $fileIds, IQueryBuilder::PARAM_INT_ARRAY); + + $result = $queryFileCache->executeQuery(); + while ($row = $result->fetch()) { + if (!$this->isAccessibleResult($row)) { + continue; + } - return $shares; + $fileData[(int) $row['fileid']] = $row; + } + $result->closeCursor(); + } + + /** @var IShare[] $shares */ + $shares = []; + foreach ($shareRows as $row) { + if (empty($fileData[(int)$row['file_source']])) { + continue; + } + $shares[] = $this->createShare(array_merge($row, $fileData[(int)$row['file_source']])); + } + + /** + * Resolve all group shares to user specific shares + */ + return $this->resolveGroupShares($shares, $userId); } /** From 792560b02b0df997513e5d8a1d8bb316355c61fc Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Fri, 9 Feb 2024 13:38:54 +0100 Subject: [PATCH 4/5] perf(sharing): Add a better index to select shares as they always have type+with Signed-off-by: Joas Schilling --- core/Application.php | 12 +++- .../Version13000Date20170718121200.php | 4 +- .../Version29000Date20240209123412.php | 56 +++++++++++++++++++ 3 files changed, 69 insertions(+), 3 deletions(-) create mode 100644 core/Migrations/Version29000Date20240209123412.php diff --git a/core/Application.php b/core/Application.php index 700fb3297afce..f9a16ca1cdeaa 100644 --- a/core/Application.php +++ b/core/Application.php @@ -79,10 +79,18 @@ public function __construct() { $notificationManager->registerNotifierService(AuthenticationNotifier::class); $eventDispatcher->addListener(AddMissingIndicesEvent::class, function (AddMissingIndicesEvent $event) { + /** + * Replaced by the share_type_with index below + * $event->addMissingIndex( + * 'share', + * 'share_with_index', + * ['share_with'] + * ); + */ $event->addMissingIndex( 'share', - 'share_with_index', - ['share_with'] + 'share_type_with', + ['share_type', 'share_with'] ); $event->addMissingIndex( 'share', diff --git a/core/Migrations/Version13000Date20170718121200.php b/core/Migrations/Version13000Date20170718121200.php index da83b0732d8a5..63cdd1d6a34a2 100644 --- a/core/Migrations/Version13000Date20170718121200.php +++ b/core/Migrations/Version13000Date20170718121200.php @@ -475,7 +475,9 @@ public function changeSchema(IOutput $output, \Closure $schemaClosure, array $op $table->addIndex(['item_type', 'share_type'], 'item_share_type_index'); $table->addIndex(['file_source'], 'file_source_index'); $table->addIndex(['token'], 'token_index'); - $table->addIndex(['share_with'], 'share_with_index'); + // Replaced by the share_type_with index below + // $table->addIndex(['share_with'], 'share_with_index'); + $table->addIndex(['share_type', 'share_with'], 'share_type_with'); $table->addIndex(['parent'], 'parent_index'); $table->addIndex(['uid_owner'], 'owner_index'); $table->addIndex(['uid_initiator'], 'initiator_index'); diff --git a/core/Migrations/Version29000Date20240209123412.php b/core/Migrations/Version29000Date20240209123412.php new file mode 100644 index 0000000000000..fa35210f5979d --- /dev/null +++ b/core/Migrations/Version29000Date20240209123412.php @@ -0,0 +1,56 @@ + + * + * @author Your name + * + * @license GNU AGPL version 3 or any later version + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as + * published by the Free Software Foundation, either version 3 of the + * License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + * + */ + +namespace OC\Core\Migrations; + +use Closure; +use OCP\DB\ISchemaWrapper; +use OCP\Migration\IOutput; +use OCP\Migration\SimpleMigrationStep; + +/** + * Auto-generated migration step: Please modify to your needs! + */ +class Version29000Date20240209123412 extends SimpleMigrationStep { + /** + * @param IOutput $output + * @param Closure(): ISchemaWrapper $schemaClosure + * @param array $options + * @return null|ISchemaWrapper + */ + public function changeSchema(IOutput $output, Closure $schemaClosure, array $options): ?ISchemaWrapper { + /** @var ISchemaWrapper $schema */ + $schema = $schemaClosure(); + + $table = $schema->getTable('share'); + if ($table->hasIndex('share_with_index')) { + $table->dropIndex('share_with_index'); + return $schema; + } + return null; + } + +} From d844f2243f432ff91059940cfa4a9032cb55a84b Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Fri, 9 Feb 2024 14:38:42 +0100 Subject: [PATCH 5/5] test(sharing): Add unit tests proofing the chunking Signed-off-by: Joas Schilling --- lib/private/Share20/DefaultShareProvider.php | 9 +- .../lib/Share20/DefaultShareProviderTest.php | 124 ++++++++++++++++++ 2 files changed, 130 insertions(+), 3 deletions(-) diff --git a/lib/private/Share20/DefaultShareProvider.php b/lib/private/Share20/DefaultShareProvider.php index a82aba056ea29..03880b4bba7b4 100644 --- a/lib/private/Share20/DefaultShareProvider.php +++ b/lib/private/Share20/DefaultShareProvider.php @@ -63,6 +63,8 @@ * @package OC\Share20 */ class DefaultShareProvider implements IShareProvider { + protected int $chunkSize = 1000; + // Special share type for user modified group shares public const SHARE_TYPE_USERGROUP = 2; @@ -1095,7 +1097,7 @@ public function getAllSharedWithUser(string $userId): array { ->where($queryFileCache->expr()->in('f.fileid', $queryFileCache->createParameter('fileIds'))); $allFileIds = array_keys($fileData); - foreach (array_chunk($allFileIds, 1000) as $fileIds) { + foreach (array_chunk($allFileIds, $this->chunkSize) as $fileIds) { // Filecache and storage info $queryFileCache->setParameter('fileIds', $fileIds, IQueryBuilder::PARAM_INT_ARRAY); @@ -1145,7 +1147,7 @@ public function getAllSharedWithGroups($userId): array { /** @var array $fileData */ $fileData = []; - foreach (array_chunk($allGroups, 1000) as $groups) { + foreach (array_chunk($allGroups, $this->chunkSize) as $groups) { $query->setParameter('groups', $groups, IQueryBuilder::PARAM_STR_ARRAY); $result = $query->executeQuery(); @@ -1158,6 +1160,7 @@ public function getAllSharedWithGroups($userId): array { $fileData[(int)$row['file_source']] = null; } $result->closeCursor(); + } if (empty($fileData)) { @@ -1174,7 +1177,7 @@ public function getAllSharedWithGroups($userId): array { ->where($queryFileCache->expr()->in('f.fileid', $queryFileCache->createParameter('fileIds'))); $allFileIds = array_keys($fileData); - foreach (array_chunk($allFileIds, 1000) as $fileIds) { + foreach (array_chunk($allFileIds, $this->chunkSize) as $fileIds) { // Filecache and storage info $queryFileCache->setParameter('fileIds', $fileIds, IQueryBuilder::PARAM_INT_ARRAY); diff --git a/tests/lib/Share20/DefaultShareProviderTest.php b/tests/lib/Share20/DefaultShareProviderTest.php index f977619b7b246..0dca507eae46d 100644 --- a/tests/lib/Share20/DefaultShareProviderTest.php +++ b/tests/lib/Share20/DefaultShareProviderTest.php @@ -994,6 +994,130 @@ public function testGetSharedWithUser($storageStringId, $fileName1, $fileName2) $this->assertEquals(IShare::TYPE_USER, $share->getShareType()); } + public function testGetAllSharedWithUser(): void { + $storageId = $this->createTestStorageEntry('home::shareOwner'); + $fileIds = []; + for ($i = 0; $i < 50; $i++) { + $fileIds[] = $this->createTestFileEntry('files/test-' . $i . '.txt', $storageId); + } + + $shareIds = []; + foreach ($fileIds as $fileId) { + $qb = $this->dbConn->getQueryBuilder(); + $qb->insert('share') + ->values([ + 'share_type' => $qb->expr()->literal(IShare::TYPE_USER), + 'share_with' => $qb->expr()->literal('sharedWith'), + 'uid_owner' => $qb->expr()->literal('shareOwner'), + 'uid_initiator' => $qb->expr()->literal('sharedBy'), + 'item_type' => $qb->expr()->literal('file'), + 'file_source' => $qb->expr()->literal($fileId), + 'file_target' => $qb->expr()->literal('myTarget'), + 'permissions' => $qb->expr()->literal(13), + ]); + $qb->executeStatement(); + $shareIds[$fileId] = $qb->getLastInsertId(); + } + + $file = $this->createMock(File::class); + $this->rootFolder->method('getUserFolder')->with('shareOwner')->willReturnSelf(); + $this->rootFolder->method('getById')->willReturn([$file]); + + self::invokePrivate($this->provider, 'chunkSize', [5]); + $shares = $this->provider->getSharedWith('sharedWith', IShare::TYPE_USER, null, -1, 0); + $this->assertCount(50, $shares); + + foreach ($shares as $share) { + $this->assertEquals($shareIds[$share->getNodeId()], $share->getId()); + $this->assertEquals('sharedWith', $share->getSharedWith()); + $this->assertEquals('shareOwner', $share->getShareOwner()); + $this->assertEquals('sharedBy', $share->getSharedBy()); + $this->assertEquals(IShare::TYPE_USER, $share->getShareType()); + } + } + public function testGetAllSharedWithGroup() { + $storageId = $this->createTestStorageEntry('home::shareOwner'); + $fileIds = []; + for ($i = 0; $i < 50; $i++) { + $fileIds[] = $this->createTestFileEntry('files/test-' . $i . '.txt', $storageId); + } + + $shareIdToFileIds = []; + $shareIdToGroupId = []; + for ($i = 0; $i < 50; $i++) { + $qb = $this->dbConn->getQueryBuilder(); + $qb->insert('share') + ->values([ + 'share_type' => $qb->expr()->literal(IShare::TYPE_GROUP), + 'share_with' => $qb->expr()->literal('sharedWith'), + 'uid_owner' => $qb->expr()->literal('shareOwner'), + 'uid_initiator' => $qb->expr()->literal('sharedBy'), + 'item_type' => $qb->expr()->literal('file'), + 'file_source' => $qb->expr()->literal($fileIds[$i]), + 'file_target' => $qb->expr()->literal('myTarget' . $i . 'shareWith'), + 'permissions' => $qb->expr()->literal(13), + ]); + $qb->executeStatement(); + $shareId = $qb->getLastInsertId(); + $shareIdToFileIds[$shareId] = $fileIds[$i]; + $shareIdToGroupId[$shareId] = 'sharedWith'; + + $qb = $this->dbConn->getQueryBuilder(); + $qb->insert('share') + ->values([ + 'share_type' => $qb->expr()->literal(IShare::TYPE_GROUP), + 'share_with' => $qb->expr()->literal('group' . $i), + 'uid_owner' => $qb->expr()->literal('shareOwner'), + 'uid_initiator' => $qb->expr()->literal('sharedBy'), + 'item_type' => $qb->expr()->literal('file'), + 'file_source' => $qb->expr()->literal($fileIds[$i]), + 'file_target' => $qb->expr()->literal('myTarget' . $i . 'group'), + 'permissions' => $qb->expr()->literal(13), + ]); + $qb->executeStatement(); + $shareId = $qb->getLastInsertId(); + $shareIdToFileIds[$shareId] = $fileIds[$i]; + $shareIdToGroupId[$shareId] = 'group' . $i; + } + + $groups = []; + foreach (range(0, 100) as $i) { + $groups[] = 'group'.$i; + } + + $groups[] = 'sharedWith'; + + $user = $this->createMock(IUser::class); + $user->method('getUID')->willReturn('sharedWith'); + $owner = $this->createMock(IUser::class); + $owner->method('getUID')->willReturn('shareOwner'); + $initiator = $this->createMock(IUser::class); + $initiator->method('getUID')->willReturn('sharedBy'); + + $this->userManager->method('get')->willReturnMap([ + ['sharedWith', $user], + ['shareOwner', $owner], + ['sharedBy', $initiator], + ]); + $this->groupManager->method('getUserGroupIds')->with($user)->willReturn($groups); + + $file = $this->createMock(File::class); + $this->rootFolder->method('getUserFolder')->with('shareOwner')->willReturnSelf(); + $this->rootFolder->method('getById')->willReturn([$file]); + + self::invokePrivate($this->provider, 'chunkSize', [5]); + $shares = $this->provider->getSharedWith('sharedWith', IShare::TYPE_GROUP, null, -1, 0); + $this->assertCount(100, $shares); + + foreach ($shares as $share) { + $this->assertEquals($shareIdToFileIds[$share->getId()], $share->getNodeId()); + $this->assertEquals($shareIdToGroupId[$share->getId()], $share->getSharedWith()); + $this->assertEquals('shareOwner', $share->getShareOwner()); + $this->assertEquals('sharedBy', $share->getSharedBy()); + $this->assertEquals(IShare::TYPE_GROUP, $share->getShareType()); + } + } + /** * @dataProvider storageAndFileNameProvider */