From 7e2ad7650507ecefbf7d567030944a480a8466c5 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Fri, 28 Apr 2023 14:09:22 +0200 Subject: [PATCH 01/15] use efficient tag retrieval on DAV report request - uses DAV search approach against valid files joined by systemtag selector - reduced table join for tag/systemtag search - supports pagination - no changes to the output formats or similar Example request body: 32 50 0 Signed-off-by: Arthur Schiwon --- .../lib/Connector/Sabre/FilesReportPlugin.php | 78 +++++++++++++------ lib/private/Files/Cache/QuerySearchHelper.php | 37 +++++---- lib/private/Files/Node/Folder.php | 7 +- .../SystemTag/SystemTagObjectMapper.php | 1 + 4 files changed, 84 insertions(+), 39 deletions(-) diff --git a/apps/dav/lib/Connector/Sabre/FilesReportPlugin.php b/apps/dav/lib/Connector/Sabre/FilesReportPlugin.php index 4876e9ad8f3d7..c30c7bc51d009 100644 --- a/apps/dav/lib/Connector/Sabre/FilesReportPlugin.php +++ b/apps/dav/lib/Connector/Sabre/FilesReportPlugin.php @@ -48,6 +48,7 @@ class FilesReportPlugin extends ServerPlugin { // namespace public const NS_OWNCLOUD = 'http://owncloud.org/ns'; + public const NS_NEXTCLOUD = 'http://nextcloud.org/ns'; public const REPORT_NAME = '{http://owncloud.org/ns}filter-files'; public const SYSTEMTAG_PROPERTYNAME = '{http://owncloud.org/ns}systemtag'; public const CIRCLE_PROPERTYNAME = '{http://owncloud.org/ns}circle'; @@ -186,6 +187,7 @@ public function onReport($reportName, $report, $uri) { } $ns = '{' . $this::NS_OWNCLOUD . '}'; + $ncns = '{' . $this::NS_NEXTCLOUD . '}'; $requestedProps = []; $filterRules = []; @@ -199,6 +201,14 @@ public function onReport($reportName, $report, $uri) { foreach ($reportProps['value'] as $propVal) { $requestedProps[] = $propVal['name']; } + } elseif ($name === '{DAV:}limit') { + foreach ($reportProps['value'] as $propVal) { + if ($propVal['name'] === '{DAV:}nresults') { + $limit = (int)$propVal['value']; + } elseif ($propVal['name'] === $ncns . 'firstresult') { + $offset = (int)$propVal['value']; + } + } } } @@ -209,13 +219,24 @@ public function onReport($reportName, $report, $uri) { // gather all file ids matching filter try { - $resultFileIds = $this->processFilterRules($filterRules); + $resultFileIds = $this->processFilterRulesForFileIDs($filterRules); + $resultNodes = $this->processFilterRulesForFileNodes($filterRules, $limit ?? null, $offset ?? null); } catch (TagNotFoundException $e) { throw new PreconditionFailed('Cannot filter by non-existing tag', 0, $e); } + $results = []; + foreach ($resultNodes as $entry) { + if ($entry) { + $results[] = $this->wrapNode($entry); + } + } + // find sabre nodes by file id, restricted to the root node path - $results = $this->findNodesByFileIds($reportTargetNode, $resultFileIds); + $additionalNodes = $this->findNodesByFileIds($reportTargetNode, $resultFileIds); + if (!empty($additionalNodes)) { + $results = array_intersect($results, $additionalNodes); + } $filesUri = $this->getFilesBaseUri($uri, $reportTargetNode->getPath()); $responses = $this->prepareResponses($filesUri, $requestedProps, $results); @@ -264,16 +285,12 @@ private function getFilesBaseUri(string $uri, string $subPath): string { * * @throws TagNotFoundException whenever a tag was not found */ - protected function processFilterRules($filterRules) { + protected function processFilterRulesForFileIDs($filterRules) { $ns = '{' . $this::NS_OWNCLOUD . '}'; $resultFileIds = null; - $systemTagIds = []; $circlesIds = []; $favoriteFilter = null; foreach ($filterRules as $filterRule) { - if ($filterRule['name'] === $ns . 'systemtag') { - $systemTagIds[] = $filterRule['value']; - } if ($filterRule['name'] === self::CIRCLE_PROPERTYNAME) { $circlesIds[] = $filterRule['value']; } @@ -289,15 +306,6 @@ protected function processFilterRules($filterRules) { } } - if (!empty($systemTagIds)) { - $fileIds = $this->getSystemTagFileIds($systemTagIds); - if (empty($resultFileIds)) { - $resultFileIds = $fileIds; - } else { - $resultFileIds = array_intersect($fileIds, $resultFileIds); - } - } - if (!empty($circlesIds)) { $fileIds = $this->getCirclesFileIds($circlesIds); if (empty($resultFileIds)) { @@ -310,6 +318,25 @@ protected function processFilterRules($filterRules) { return $resultFileIds; } + protected function processFilterRulesForFileNodes(array $filterRules, ?int $limit, ?int $offset): array { + $systemTagIds = []; + foreach ($filterRules as $filterRule) { + if ($filterRule['name'] === self::SYSTEMTAG_PROPERTYNAME) { + $systemTagIds[] = $filterRule['value']; + } + } + + $nodes = []; + + if (!empty($systemTagIds)) { + $tags = $this->tagManager->getTagsByIds($systemTagIds); + $tagName = (current($tags))->getName(); + $nodes = $this->userFolder->searchBySystemTag($tagName, $this->userSession->getUser()->getUID(), $limit ?? 0, $offset ?? 0); + } + + return $nodes; + } + private function getSystemTagFileIds($systemTagIds) { $resultFileIds = null; @@ -406,7 +433,10 @@ public function prepareResponses($filesUri, $requestedProps, $nodes) { * @param array $fileIds file ids * @return Node[] array of Sabre nodes */ - public function findNodesByFileIds($rootNode, $fileIds) { + public function findNodesByFileIds($rootNode, $fileIds): array { + if (empty($fileIds)) { + return []; + } $folder = $this->userFolder; if (trim($rootNode->getPath(), '/') !== '') { $folder = $folder->get($rootNode->getPath()); @@ -417,17 +447,21 @@ public function findNodesByFileIds($rootNode, $fileIds) { $entry = $folder->getById($fileId); if ($entry) { $entry = current($entry); - if ($entry instanceof \OCP\Files\File) { - $results[] = new File($this->fileView, $entry); - } elseif ($entry instanceof \OCP\Files\Folder) { - $results[] = new Directory($this->fileView, $entry); - } + $results[] = $this->wrapNode($entry); } } return $results; } + protected function wrapNode(\OCP\Files\File|\OCP\Files\Folder $node): File|Directory { + if ($node instanceof \OCP\Files\File) { + return new File($this->fileView, $node); + } else { + return new Directory($this->fileView, $node); + } + } + /** * Returns whether the currently logged in user is an administrator */ diff --git a/lib/private/Files/Cache/QuerySearchHelper.php b/lib/private/Files/Cache/QuerySearchHelper.php index f3401f7a9b678..ff3ddf240bb73 100644 --- a/lib/private/Files/Cache/QuerySearchHelper.php +++ b/lib/private/Files/Cache/QuerySearchHelper.php @@ -36,6 +36,7 @@ use OCP\Files\IRootFolder; use OCP\Files\Mount\IMountPoint; use OCP\Files\Search\ISearchBinaryOperator; +use OCP\Files\Search\ISearchComparison; use OCP\Files\Search\ISearchQuery; use OCP\IDBConnection; use Psr\Log\LoggerInterface; @@ -152,22 +153,26 @@ public function searchInCaches(ISearchQuery $searchQuery, array $caches): array if ($user === null) { throw new \InvalidArgumentException("Searching by tag requires the user to be set in the query"); } - $query - ->leftJoin('file', 'vcategory_to_object', 'tagmap', $builder->expr()->eq('file.fileid', 'tagmap.objid')) - ->leftJoin('tagmap', 'vcategory', 'tag', $builder->expr()->andX( - $builder->expr()->eq('tagmap.type', 'tag.type'), - $builder->expr()->eq('tagmap.categoryid', 'tag.id'), - $builder->expr()->eq('tag.type', $builder->createNamedParameter('files')), - $builder->expr()->eq('tag.uid', $builder->createNamedParameter($user->getUID())) - )) - ->leftJoin('file', 'systemtag_object_mapping', 'systemtagmap', $builder->expr()->andX( - $builder->expr()->eq('file.fileid', $builder->expr()->castColumn('systemtagmap.objectid', IQueryBuilder::PARAM_INT)), - $builder->expr()->eq('systemtagmap.objecttype', $builder->createNamedParameter('files')) - )) - ->leftJoin('systemtagmap', 'systemtag', 'systemtag', $builder->expr()->andX( - $builder->expr()->eq('systemtag.id', 'systemtagmap.systemtagid'), - $builder->expr()->eq('systemtag.visibility', $builder->createNamedParameter(true)) - )); + if ($searchQuery->getSearchOperation() instanceof ISearchComparison && $searchQuery->getSearchOperation()->getField() === 'systemtag') { + $query + ->leftJoin('file', 'systemtag_object_mapping', 'systemtagmap', $builder->expr()->andX( + $builder->expr()->eq('file.fileid', $builder->expr()->castColumn('systemtagmap.objectid', IQueryBuilder::PARAM_INT)), + $builder->expr()->eq('systemtagmap.objecttype', $builder->createNamedParameter('files')) + )) + ->leftJoin('systemtagmap', 'systemtag', 'systemtag', $builder->expr()->andX( + $builder->expr()->eq('systemtag.id', 'systemtagmap.systemtagid'), + $builder->expr()->eq('systemtag.visibility', $builder->createNamedParameter(true)) + )); + } else { + $query + ->leftJoin('file', 'vcategory_to_object', 'tagmap', $builder->expr()->eq('file.fileid', 'tagmap.objid')) + ->leftJoin('tagmap', 'vcategory', 'tag', $builder->expr()->andX( + $builder->expr()->eq('tagmap.type', 'tag.type'), + $builder->expr()->eq('tagmap.categoryid', 'tag.id'), + $builder->expr()->eq('tag.type', $builder->createNamedParameter('files')), + $builder->expr()->eq('tag.uid', $builder->createNamedParameter($user->getUID())) + )); + } } $this->applySearchConstraints($query, $searchQuery, $caches); diff --git a/lib/private/Files/Node/Folder.php b/lib/private/Files/Node/Folder.php index 5669f2a4b4599..1e370e6b3e945 100644 --- a/lib/private/Files/Node/Folder.php +++ b/lib/private/Files/Node/Folder.php @@ -297,7 +297,12 @@ public function searchByMime($mimetype) { * @return Node[] */ public function searchByTag($tag, $userId) { - $query = $this->queryFromOperator(new SearchComparison(ISearchComparison::COMPARE_EQUAL, 'tagname', $tag), $userId); + $query = $this->queryFromOperator(new SearchComparison(ISearchComparison::COMPARE_EQUAL, 'tag', $tag), $userId); + return $this->search($query); + } + + public function searchBySystemTag(string $tag, string $userId, int $limit = 0, int $offset = 0): array { + $query = $this->queryFromOperator(new SearchComparison(ISearchComparison::COMPARE_EQUAL, 'systemtag', $tag), $userId, $limit, $offset); return $this->search($query); } diff --git a/lib/private/SystemTag/SystemTagObjectMapper.php b/lib/private/SystemTag/SystemTagObjectMapper.php index b61a81a1fa701..864ba3e91a898 100644 --- a/lib/private/SystemTag/SystemTagObjectMapper.php +++ b/lib/private/SystemTag/SystemTagObjectMapper.php @@ -135,6 +135,7 @@ public function getObjectIdsForTags($tagIds, string $objectType, int $limit = 0, while ($row = $result->fetch()) { $objectIds[] = $row['objectid']; } + $result->closeCursor(); return $objectIds; } From 5086dff5635a9a00501ece1056993f591fbf5a45 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Thu, 11 May 2023 11:22:27 +0200 Subject: [PATCH 02/15] fix: change if with conditionless else to switch; and a parameter value Signed-off-by: Arthur Schiwon --- lib/private/Files/Cache/QuerySearchHelper.php | 43 +++++++++++-------- lib/private/Files/Node/Folder.php | 2 +- 2 files changed, 25 insertions(+), 20 deletions(-) diff --git a/lib/private/Files/Cache/QuerySearchHelper.php b/lib/private/Files/Cache/QuerySearchHelper.php index ff3ddf240bb73..ad86f3bd82bcc 100644 --- a/lib/private/Files/Cache/QuerySearchHelper.php +++ b/lib/private/Files/Cache/QuerySearchHelper.php @@ -153,25 +153,30 @@ public function searchInCaches(ISearchQuery $searchQuery, array $caches): array if ($user === null) { throw new \InvalidArgumentException("Searching by tag requires the user to be set in the query"); } - if ($searchQuery->getSearchOperation() instanceof ISearchComparison && $searchQuery->getSearchOperation()->getField() === 'systemtag') { - $query - ->leftJoin('file', 'systemtag_object_mapping', 'systemtagmap', $builder->expr()->andX( - $builder->expr()->eq('file.fileid', $builder->expr()->castColumn('systemtagmap.objectid', IQueryBuilder::PARAM_INT)), - $builder->expr()->eq('systemtagmap.objecttype', $builder->createNamedParameter('files')) - )) - ->leftJoin('systemtagmap', 'systemtag', 'systemtag', $builder->expr()->andX( - $builder->expr()->eq('systemtag.id', 'systemtagmap.systemtagid'), - $builder->expr()->eq('systemtag.visibility', $builder->createNamedParameter(true)) - )); - } else { - $query - ->leftJoin('file', 'vcategory_to_object', 'tagmap', $builder->expr()->eq('file.fileid', 'tagmap.objid')) - ->leftJoin('tagmap', 'vcategory', 'tag', $builder->expr()->andX( - $builder->expr()->eq('tagmap.type', 'tag.type'), - $builder->expr()->eq('tagmap.categoryid', 'tag.id'), - $builder->expr()->eq('tag.type', $builder->createNamedParameter('files')), - $builder->expr()->eq('tag.uid', $builder->createNamedParameter($user->getUID())) - )); + if ($searchQuery->getSearchOperation() instanceof ISearchComparison) { + switch ($searchQuery->getSearchOperation()->getField()) { + case 'systemtag': + $query + ->leftJoin('file', 'systemtag_object_mapping', 'systemtagmap', $builder->expr()->andX( + $builder->expr()->eq('file.fileid', $builder->expr()->castColumn('systemtagmap.objectid', IQueryBuilder::PARAM_INT)), + $builder->expr()->eq('systemtagmap.objecttype', $builder->createNamedParameter('files')) + )) + ->leftJoin('systemtagmap', 'systemtag', 'systemtag', $builder->expr()->andX( + $builder->expr()->eq('systemtag.id', 'systemtagmap.systemtagid'), + $builder->expr()->eq('systemtag.visibility', $builder->createNamedParameter(true)) + )); + break; + case 'tagname': + $query + ->leftJoin('file', 'vcategory_to_object', 'tagmap', $builder->expr()->eq('file.fileid', 'tagmap.objid')) + ->leftJoin('tagmap', 'vcategory', 'tag', $builder->expr()->andX( + $builder->expr()->eq('tagmap.type', 'tag.type'), + $builder->expr()->eq('tagmap.categoryid', 'tag.id'), + $builder->expr()->eq('tag.type', $builder->createNamedParameter('files')), + $builder->expr()->eq('tag.uid', $builder->createNamedParameter($user->getUID())) + )); + break; + } } } diff --git a/lib/private/Files/Node/Folder.php b/lib/private/Files/Node/Folder.php index 1e370e6b3e945..f409f5e63f405 100644 --- a/lib/private/Files/Node/Folder.php +++ b/lib/private/Files/Node/Folder.php @@ -297,7 +297,7 @@ public function searchByMime($mimetype) { * @return Node[] */ public function searchByTag($tag, $userId) { - $query = $this->queryFromOperator(new SearchComparison(ISearchComparison::COMPARE_EQUAL, 'tag', $tag), $userId); + $query = $this->queryFromOperator(new SearchComparison(ISearchComparison::COMPARE_EQUAL, 'tagname', $tag), $userId); return $this->search($query); } From 837fd9f8da08c2c44a4e8af00d52d0622ca7dee2 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Thu, 11 May 2023 13:08:57 +0200 Subject: [PATCH 03/15] fix: favorites view and universal search against tags Signed-off-by: Arthur Schiwon --- .../lib/Connector/Sabre/FilesReportPlugin.php | 4 +- lib/private/Files/Cache/QuerySearchHelper.php | 47 ++++++++++++------- 2 files changed, 33 insertions(+), 18 deletions(-) diff --git a/apps/dav/lib/Connector/Sabre/FilesReportPlugin.php b/apps/dav/lib/Connector/Sabre/FilesReportPlugin.php index c30c7bc51d009..7a53c34744cf5 100644 --- a/apps/dav/lib/Connector/Sabre/FilesReportPlugin.php +++ b/apps/dav/lib/Connector/Sabre/FilesReportPlugin.php @@ -234,8 +234,10 @@ public function onReport($reportName, $report, $uri) { // find sabre nodes by file id, restricted to the root node path $additionalNodes = $this->findNodesByFileIds($reportTargetNode, $resultFileIds); - if (!empty($additionalNodes)) { + if ($additionalNodes && $results) { $results = array_intersect($results, $additionalNodes); + } elseif (!$results && $additionalNodes) { + $results = $additionalNodes; } $filesUri = $this->getFilesBaseUri($uri, $reportTargetNode->getPath()); diff --git a/lib/private/Files/Cache/QuerySearchHelper.php b/lib/private/Files/Cache/QuerySearchHelper.php index ad86f3bd82bcc..d020ec00cea5e 100644 --- a/lib/private/Files/Cache/QuerySearchHelper.php +++ b/lib/private/Files/Cache/QuerySearchHelper.php @@ -39,6 +39,7 @@ use OCP\Files\Search\ISearchComparison; use OCP\Files\Search\ISearchQuery; use OCP\IDBConnection; +use OCP\IUser; use Psr\Log\LoggerInterface; class QuerySearchHelper { @@ -118,6 +119,29 @@ public function findUsedTagsInCaches(ISearchQuery $searchQuery, array $caches): return $tags; } + protected function equipQueryForSystemTags(CacheQueryBuilder $query): void { + $query + ->leftJoin('file', 'systemtag_object_mapping', 'systemtagmap', $query->expr()->andX( + $query->expr()->eq('file.fileid', $query->expr()->castColumn('systemtagmap.objectid', IQueryBuilder::PARAM_INT)), + $query->expr()->eq('systemtagmap.objecttype', $query->createNamedParameter('files')) + )) + ->leftJoin('systemtagmap', 'systemtag', 'systemtag', $query->expr()->andX( + $query->expr()->eq('systemtag.id', 'systemtagmap.systemtagid'), + $query->expr()->eq('systemtag.visibility', $query->createNamedParameter(true)) + )); + } + + protected function equipQueryForDavTags(CacheQueryBuilder $query, IUser $user): void { + $query + ->leftJoin('file', 'vcategory_to_object', 'tagmap', $query->expr()->eq('file.fileid', 'tagmap.objid')) + ->leftJoin('tagmap', 'vcategory', 'tag', $query->expr()->andX( + $query->expr()->eq('tagmap.type', 'tag.type'), + $query->expr()->eq('tagmap.categoryid', 'tag.id'), + $query->expr()->eq('tag.type', $query->createNamedParameter('files')), + $query->expr()->eq('tag.uid', $query->createNamedParameter($user->getUID())) + )); + } + /** * Perform a file system search in multiple caches * @@ -156,27 +180,16 @@ public function searchInCaches(ISearchQuery $searchQuery, array $caches): array if ($searchQuery->getSearchOperation() instanceof ISearchComparison) { switch ($searchQuery->getSearchOperation()->getField()) { case 'systemtag': - $query - ->leftJoin('file', 'systemtag_object_mapping', 'systemtagmap', $builder->expr()->andX( - $builder->expr()->eq('file.fileid', $builder->expr()->castColumn('systemtagmap.objectid', IQueryBuilder::PARAM_INT)), - $builder->expr()->eq('systemtagmap.objecttype', $builder->createNamedParameter('files')) - )) - ->leftJoin('systemtagmap', 'systemtag', 'systemtag', $builder->expr()->andX( - $builder->expr()->eq('systemtag.id', 'systemtagmap.systemtagid'), - $builder->expr()->eq('systemtag.visibility', $builder->createNamedParameter(true)) - )); + $this->equipQueryForSystemTags($query); break; case 'tagname': - $query - ->leftJoin('file', 'vcategory_to_object', 'tagmap', $builder->expr()->eq('file.fileid', 'tagmap.objid')) - ->leftJoin('tagmap', 'vcategory', 'tag', $builder->expr()->andX( - $builder->expr()->eq('tagmap.type', 'tag.type'), - $builder->expr()->eq('tagmap.categoryid', 'tag.id'), - $builder->expr()->eq('tag.type', $builder->createNamedParameter('files')), - $builder->expr()->eq('tag.uid', $builder->createNamedParameter($user->getUID())) - )); + case 'favorite': + $this->equipQueryForDavTags($query, $user); break; } + } elseif ($searchQuery->getSearchOperation() instanceof SearchBinaryOperator) { + $this->equipQueryForSystemTags($query); + $this->equipQueryForDavTags($query, $user); } } From 03590e86e7a02e382d23c4889a811863281434cd Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Thu, 11 May 2023 13:17:49 +0200 Subject: [PATCH 04/15] fix: ensure searchBySystemTag() is available Signed-off-by: Arthur Schiwon --- apps/dav/lib/Connector/Sabre/FilesReportPlugin.php | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/apps/dav/lib/Connector/Sabre/FilesReportPlugin.php b/apps/dav/lib/Connector/Sabre/FilesReportPlugin.php index 7a53c34744cf5..8330934fa7585 100644 --- a/apps/dav/lib/Connector/Sabre/FilesReportPlugin.php +++ b/apps/dav/lib/Connector/Sabre/FilesReportPlugin.php @@ -330,7 +330,9 @@ protected function processFilterRulesForFileNodes(array $filterRules, ?int $limi $nodes = []; - if (!empty($systemTagIds)) { + // type check to ensure searchBySystemTag is available, it is not + // exposed in API yet + if (!empty($systemTagIds) && $this->userFolder instanceof \OC\Files\Node\Folder) { $tags = $this->tagManager->getTagsByIds($systemTagIds); $tagName = (current($tags))->getName(); $nodes = $this->userFolder->searchBySystemTag($tagName, $this->userSession->getUser()->getUID(), $limit ?? 0, $offset ?? 0); From 2b513e9d12f25fea4fa7de067dd1b4d7e82ee3e9 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Thu, 11 May 2023 13:18:45 +0200 Subject: [PATCH 05/15] chore: cleanup unused code Signed-off-by: Arthur Schiwon --- .../lib/Connector/Sabre/FilesReportPlugin.php | 43 ------------------- 1 file changed, 43 deletions(-) diff --git a/apps/dav/lib/Connector/Sabre/FilesReportPlugin.php b/apps/dav/lib/Connector/Sabre/FilesReportPlugin.php index 8330934fa7585..4cccdc842b995 100644 --- a/apps/dav/lib/Connector/Sabre/FilesReportPlugin.php +++ b/apps/dav/lib/Connector/Sabre/FilesReportPlugin.php @@ -341,49 +341,6 @@ protected function processFilterRulesForFileNodes(array $filterRules, ?int $limi return $nodes; } - private function getSystemTagFileIds($systemTagIds) { - $resultFileIds = null; - - // check user permissions, if applicable - if (!$this->isAdmin()) { - // check visibility/permission - $tags = $this->tagManager->getTagsByIds($systemTagIds); - $unknownTagIds = []; - foreach ($tags as $tag) { - if (!$tag->isUserVisible()) { - $unknownTagIds[] = $tag->getId(); - } - } - - if (!empty($unknownTagIds)) { - throw new TagNotFoundException('Tag with ids ' . implode(', ', $unknownTagIds) . ' not found'); - } - } - - // fetch all file ids and intersect them - foreach ($systemTagIds as $systemTagId) { - $fileIds = $this->tagMapper->getObjectIdsForTags($systemTagId, 'files'); - - if (empty($fileIds)) { - // This tag has no files, nothing can ever show up - return []; - } - - // first run ? - if ($resultFileIds === null) { - $resultFileIds = $fileIds; - } else { - $resultFileIds = array_intersect($resultFileIds, $fileIds); - } - - if (empty($resultFileIds)) { - // Empty intersection, nothing can show up anymore - return []; - } - } - return $resultFileIds; - } - /** * @suppress PhanUndeclaredClassMethod * @param array $circlesIds From 8d252731927fa7cb129d099d0b2b80711756db61 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Fri, 12 May 2023 12:10:38 +0200 Subject: [PATCH 06/15] fix: no search when LazyFolder was provided Signed-off-by: Arthur Schiwon --- apps/dav/lib/Connector/Sabre/FilesReportPlugin.php | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/apps/dav/lib/Connector/Sabre/FilesReportPlugin.php b/apps/dav/lib/Connector/Sabre/FilesReportPlugin.php index 4cccdc842b995..2cae9d948b5b7 100644 --- a/apps/dav/lib/Connector/Sabre/FilesReportPlugin.php +++ b/apps/dav/lib/Connector/Sabre/FilesReportPlugin.php @@ -27,6 +27,7 @@ */ namespace OCA\DAV\Connector\Sabre; +use OC\Files\Node\LazyFolder; use OC\Files\View; use OCP\App\IAppManager; use OCP\Files\Folder; @@ -332,7 +333,11 @@ protected function processFilterRulesForFileNodes(array $filterRules, ?int $limi // type check to ensure searchBySystemTag is available, it is not // exposed in API yet - if (!empty($systemTagIds) && $this->userFolder instanceof \OC\Files\Node\Folder) { + if ( + !empty($systemTagIds) + && ($this->userFolder instanceof \OC\Files\Node\Folder + || $this->userFolder instanceof LazyFolder) + ) { $tags = $this->tagManager->getTagsByIds($systemTagIds); $tagName = (current($tags))->getName(); $nodes = $this->userFolder->searchBySystemTag($tagName, $this->userSession->getUser()->getUID(), $limit ?? 0, $offset ?? 0); From 5c0a1a4b096a9feb9f215fcf87acad1456d168e0 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Fri, 2 Jun 2023 23:11:30 +0200 Subject: [PATCH 07/15] fix: search with more than one search tags Signed-off-by: Arthur Schiwon --- .../lib/Connector/Sabre/FilesReportPlugin.php | 26 +- .../Connector/Sabre/FilesReportPluginTest.php | 500 +++++++++++++----- 2 files changed, 374 insertions(+), 152 deletions(-) diff --git a/apps/dav/lib/Connector/Sabre/FilesReportPlugin.php b/apps/dav/lib/Connector/Sabre/FilesReportPlugin.php index 2cae9d948b5b7..c0cd382d994f7 100644 --- a/apps/dav/lib/Connector/Sabre/FilesReportPlugin.php +++ b/apps/dav/lib/Connector/Sabre/FilesReportPlugin.php @@ -27,7 +27,6 @@ */ namespace OCA\DAV\Connector\Sabre; -use OC\Files\Node\LazyFolder; use OC\Files\View; use OCP\App\IAppManager; use OCP\Files\Folder; @@ -46,7 +45,6 @@ use Sabre\DAV\Xml\Response\MultiStatus; class FilesReportPlugin extends ServerPlugin { - // namespace public const NS_OWNCLOUD = 'http://owncloud.org/ns'; public const NS_NEXTCLOUD = 'http://nextcloud.org/ns'; @@ -335,12 +333,28 @@ protected function processFilterRulesForFileNodes(array $filterRules, ?int $limi // exposed in API yet if ( !empty($systemTagIds) - && ($this->userFolder instanceof \OC\Files\Node\Folder - || $this->userFolder instanceof LazyFolder) + && (method_exists($this->userFolder, 'searchBySystemTag')) ) { $tags = $this->tagManager->getTagsByIds($systemTagIds); - $tagName = (current($tags))->getName(); - $nodes = $this->userFolder->searchBySystemTag($tagName, $this->userSession->getUser()->getUID(), $limit ?? 0, $offset ?? 0); + foreach ($tags as $tag) { + if (!$tag->isUserVisible()) { + // searchBySystemTag() also has the criteria to include only user visible tags. They can be skipped early nevertheless. + continue; + } + $tagName = $tag->getName(); + $tmpNodes = $this->userFolder->searchBySystemTag($tagName, $this->userSession->getUser()->getUID(), $limit ?? 0, $offset ?? 0); + if (count($nodes) === 0) { + $nodes = $tmpNodes; + } else { + $nodes = array_uintersect($nodes, $tmpNodes, function (\OCP\Files\Node $a, \OCP\Files\Node $b): int { + return $a->getId() - $b->getId(); + }); + } + if ($nodes === []) { + // there cannot be a common match when nodes are empty early. + return $nodes; + } + } } return $nodes; diff --git a/apps/dav/tests/unit/Connector/Sabre/FilesReportPluginTest.php b/apps/dav/tests/unit/Connector/Sabre/FilesReportPluginTest.php index f73434b33b6b1..5c81757b11ac6 100644 --- a/apps/dav/tests/unit/Connector/Sabre/FilesReportPluginTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/FilesReportPluginTest.php @@ -44,45 +44,48 @@ use OCP\SystemTag\ISystemTag; use OCP\SystemTag\ISystemTagManager; use OCP\SystemTag\ISystemTagObjectMapper; +use PHPUnit\Framework\MockObject\MockObject; use Sabre\DAV\INode; use Sabre\DAV\Tree; use Sabre\HTTP\ResponseInterface; class FilesReportPluginTest extends \Test\TestCase { - /** @var \Sabre\DAV\Server|\PHPUnit\Framework\MockObject\MockObject */ + /** @var \Sabre\DAV\Server|MockObject */ private $server; - /** @var \Sabre\DAV\Tree|\PHPUnit\Framework\MockObject\MockObject */ + /** @var \Sabre\DAV\Tree|MockObject */ private $tree; - /** @var ISystemTagObjectMapper|\PHPUnit\Framework\MockObject\MockObject */ + /** @var ISystemTagObjectMapper|MockObject */ private $tagMapper; - /** @var ISystemTagManager|\PHPUnit\Framework\MockObject\MockObject */ + /** @var ISystemTagManager|MockObject */ private $tagManager; - /** @var ITags|\PHPUnit\Framework\MockObject\MockObject */ + /** @var ITags|MockObject */ private $privateTags; + private ITagManager|MockObject $privateTagManager; + /** @var \OCP\IUserSession */ private $userSession; /** @var FilesReportPluginImplementation */ private $plugin; - /** @var View|\PHPUnit\Framework\MockObject\MockObject **/ + /** @var View|MockObject **/ private $view; - /** @var IGroupManager|\PHPUnit\Framework\MockObject\MockObject **/ + /** @var IGroupManager|MockObject **/ private $groupManager; - /** @var Folder|\PHPUnit\Framework\MockObject\MockObject **/ + /** @var Folder|MockObject **/ private $userFolder; - /** @var IPreview|\PHPUnit\Framework\MockObject\MockObject * */ + /** @var IPreview|MockObject * */ private $previewManager; - /** @var IAppManager|\PHPUnit\Framework\MockObject\MockObject * */ + /** @var IAppManager|MockObject * */ private $appManager; protected function setUp(): void { @@ -124,8 +127,8 @@ protected function setUp(): void { $this->tagMapper = $this->createMock(ISystemTagObjectMapper::class); $this->userSession = $this->createMock(IUserSession::class); $this->privateTags = $this->createMock(ITags::class); - $privateTagManager = $this->createMock(ITagManager::class); - $privateTagManager->expects($this->any()) + $this->privateTagManager = $this->createMock(ITagManager::class); + $this->privateTagManager->expects($this->any()) ->method('load') ->with('files') ->willReturn($this->privateTags); @@ -145,7 +148,7 @@ protected function setUp(): void { $this->view, $this->tagManager, $this->tagMapper, - $privateTagManager, + $this->privateTagManager, $this->userSession, $this->groupManager, $this->userFolder, @@ -153,7 +156,7 @@ protected function setUp(): void { ); } - public function testOnReportInvalidNode() { + public function testOnReportInvalidNode(): void { $path = 'totally/unrelated/13'; $this->tree->expects($this->any()) @@ -173,7 +176,7 @@ public function testOnReportInvalidNode() { $this->assertNull($this->plugin->onReport(FilesReportPluginImplementation::REPORT_NAME, [], '/' . $path)); } - public function testOnReportInvalidReportName() { + public function testOnReportInvalidReportName(): void { $path = 'test'; $this->tree->expects($this->any()) @@ -193,7 +196,7 @@ public function testOnReportInvalidReportName() { $this->assertNull($this->plugin->onReport('{whoever}whatever', [], '/' . $path)); } - public function testOnReport() { + public function testOnReport(): void { $path = 'test'; $parameters = [ @@ -217,15 +220,6 @@ public function testOnReport() { ->method('isAdmin') ->willReturn(true); - $this->tagMapper->expects($this->at(0)) - ->method('getObjectIdsForTags') - ->with('123', 'files') - ->willReturn(['111', '222']); - $this->tagMapper->expects($this->at(1)) - ->method('getObjectIdsForTags') - ->with('456', 'files') - ->willReturn(['111', '222', '333']); - $reportTargetNode = $this->getMockBuilder(Directory::class) ->disableOriginalConstructor() ->getMock(); @@ -253,21 +247,45 @@ public function testOnReport() { ->with('/' . $path) ->willReturn($reportTargetNode); - $filesNode1 = $this->getMockBuilder(Folder::class) - ->disableOriginalConstructor() - ->getMock(); - $filesNode2 = $this->getMockBuilder(File::class) - ->disableOriginalConstructor() - ->getMock(); + $filesNode1 = $this->createMock(File::class); + $filesNode1->expects($this->any()) + ->method('getSize') + ->willReturn(12); + $filesNode2 = $this->createMock(Folder::class); + $filesNode2->expects($this->any()) + ->method('getSize') + ->willReturn(10); - $this->userFolder->expects($this->at(0)) - ->method('getById') - ->with('111') - ->willReturn([$filesNode1]); - $this->userFolder->expects($this->at(1)) - ->method('getById') - ->with('222') - ->willReturn([$filesNode2]); + $tag123 = $this->createMock(ISystemTag::class); + $tag123->expects($this->any()) + ->method('getName') + ->willReturn('OneTwoThree'); + $tag123->expects($this->any()) + ->method('isUserVisible') + ->willReturn(true); + $tag456 = $this->createMock(ISystemTag::class); + $tag456->expects($this->any()) + ->method('getName') + ->willReturn('FourFiveSix'); + $tag456->expects($this->any()) + ->method('isUserVisible') + ->willReturn(true); + + $this->tagManager->expects($this->once()) + ->method('getTagsByIds') + ->with(['123', '456']) + ->willReturn([$tag123, $tag456]); + + $this->userFolder->expects($this->exactly(2)) + ->method('searchBySystemTag') + ->withConsecutive( + ['OneTwoThree'], + ['FourFiveSix'], + ) + ->willReturnOnConsecutiveCalls( + [$filesNode1], + [$filesNode2], + ); $this->server->expects($this->any()) ->method('getRequestUri') @@ -278,7 +296,7 @@ public function testOnReport() { $this->assertFalse($this->plugin->onReport(FilesReportPluginImplementation::REPORT_NAME, $parameters, '/' . $path)); } - public function testFindNodesByFileIdsRoot() { + public function testFindNodesByFileIdsRoot(): void { $filesNode1 = $this->getMockBuilder(Folder::class) ->disableOriginalConstructor() ->getMock(); @@ -300,16 +318,18 @@ public function testFindNodesByFileIdsRoot() { ->method('getPath') ->willReturn('/'); - $this->userFolder->expects($this->at(0)) + $this->userFolder->expects($this->exactly(2)) ->method('getById') - ->with('111') - ->willReturn([$filesNode1]); - $this->userFolder->expects($this->at(1)) - ->method('getById') - ->with('222') - ->willReturn([$filesNode2]); + ->withConsecutive( + ['111'], + ['222'], + ) + ->willReturnOnConsecutiveCalls( + [$filesNode1], + [$filesNode2], + ); - /** @var \OCA\DAV\Connector\Sabre\Directory|\PHPUnit\Framework\MockObject\MockObject $reportTargetNode */ + /** @var \OCA\DAV\Connector\Sabre\Directory|MockObject $reportTargetNode */ $result = $this->plugin->findNodesByFileIds($reportTargetNode, ['111', '222']); $this->assertCount(2, $result); @@ -319,7 +339,7 @@ public function testFindNodesByFileIdsRoot() { $this->assertEquals('second node', $result[1]->getName()); } - public function testFindNodesByFileIdsSubDir() { + public function testFindNodesByFileIdsSubDir(): void { $filesNode1 = $this->getMockBuilder(Folder::class) ->disableOriginalConstructor() ->getMock(); @@ -346,21 +366,23 @@ public function testFindNodesByFileIdsSubDir() { ->disableOriginalConstructor() ->getMock(); - $this->userFolder->expects($this->at(0)) + $this->userFolder->expects($this->once()) ->method('get') ->with('/sub1/sub2') ->willReturn($subNode); - $subNode->expects($this->at(0)) - ->method('getById') - ->with('111') - ->willReturn([$filesNode1]); - $subNode->expects($this->at(1)) + $subNode->expects($this->exactly(2)) ->method('getById') - ->with('222') - ->willReturn([$filesNode2]); + ->withConsecutive( + ['111'], + ['222'], + ) + ->willReturnOnConsecutiveCalls( + [$filesNode1], + [$filesNode2], + ); - /** @var \OCA\DAV\Connector\Sabre\Directory|\PHPUnit\Framework\MockObject\MockObject $reportTargetNode */ + /** @var \OCA\DAV\Connector\Sabre\Directory|MockObject $reportTargetNode */ $result = $this->plugin->findNodesByFileIds($reportTargetNode, ['111', '222']); $this->assertCount(2, $result); @@ -370,7 +392,7 @@ public function testFindNodesByFileIdsSubDir() { $this->assertEquals('second node', $result[1]->getName()); } - public function testPrepareResponses() { + public function testPrepareResponses(): void { $requestedProps = ['{DAV:}getcontentlength', '{http://owncloud.org/ns}fileid', '{DAV:}resourcetype']; $fileInfo = $this->createMock(FileInfo::class); @@ -439,116 +461,288 @@ public function testPrepareResponses() { $this->assertCount(0, $props2[200]['{DAV:}resourcetype']->getValue()); } - public function testProcessFilterRulesSingle() { + public function testProcessFilterRulesSingle(): void { $this->groupManager->expects($this->any()) ->method('isAdmin') ->willReturn(true); - $this->tagMapper->expects($this->exactly(1)) - ->method('getObjectIdsForTags') - ->withConsecutive( - ['123', 'files'] - ) - ->willReturnMap([ - ['123', 'files', 0, '', ['111', '222']], - ]); - $rules = [ ['name' => '{http://owncloud.org/ns}systemtag', 'value' => '123'], ]; - $this->assertEquals(['111', '222'], $this->invokePrivate($this->plugin, 'processFilterRules', [$rules])); + $filesNode1 = $this->createMock(File::class); + $filesNode1->expects($this->any()) + ->method('getSize') + ->willReturn(12); + $filesNode2 = $this->createMock(Folder::class); + $filesNode2->expects($this->any()) + ->method('getSize') + ->willReturn(10); + + $tag123 = $this->createMock(ISystemTag::class); + $tag123->expects($this->any()) + ->method('getName') + ->willReturn('OneTwoThree'); + $tag123->expects($this->any()) + ->method('isUserVisible') + ->willReturn(true); + + $this->tagManager->expects($this->once()) + ->method('getTagsByIds') + ->with(['123']) + ->willReturn([$tag123]); + + $this->userFolder->expects($this->once()) + ->method('searchBySystemTag') + ->with('OneTwoThree') + ->willReturn([$filesNode1, $filesNode2]); + + $this->assertEquals([$filesNode1, $filesNode2], $this->invokePrivate($this->plugin, 'processFilterRulesForFileNodes', [$rules, 0, 0])); } - public function testProcessFilterRulesAndCondition() { + public function testProcessFilterRulesAndCondition(): void { $this->groupManager->expects($this->any()) ->method('isAdmin') ->willReturn(true); - $this->tagMapper->expects($this->exactly(2)) - ->method('getObjectIdsForTags') + $filesNode1 = $this->createMock(File::class); + $filesNode1->expects($this->any()) + ->method('getSize') + ->willReturn(12); + $filesNode1->expects($this->any()) + ->method('getId') + ->willReturn(111); + $filesNode2 = $this->createMock(Folder::class); + $filesNode2->expects($this->any()) + ->method('getSize') + ->willReturn(10); + $filesNode2->expects($this->any()) + ->method('getId') + ->willReturn(222); + $filesNode3 = $this->createMock(File::class); + $filesNode3->expects($this->any()) + ->method('getSize') + ->willReturn(14); + $filesNode3->expects($this->any()) + ->method('getId') + ->willReturn(333); + + $tag123 = $this->createMock(ISystemTag::class); + $tag123->expects($this->any()) + ->method('getName') + ->willReturn('OneTwoThree'); + $tag123->expects($this->any()) + ->method('isUserVisible') + ->willReturn(true); + $tag456 = $this->createMock(ISystemTag::class); + $tag456->expects($this->any()) + ->method('getName') + ->willReturn('FourFiveSix'); + $tag456->expects($this->any()) + ->method('isUserVisible') + ->willReturn(true); + + $this->tagManager->expects($this->once()) + ->method('getTagsByIds') + ->with(['123', '456']) + ->willReturn([$tag123, $tag456]); + + $this->userFolder->expects($this->exactly(2)) + ->method('searchBySystemTag') ->withConsecutive( - ['123', 'files'], - ['456', 'files'] + ['OneTwoThree'], + ['FourFiveSix'], ) - ->willReturnMap([ - ['123', 'files', 0, '', ['111', '222']], - ['456', 'files', 0, '', ['222', '333']], - ]); + ->willReturnOnConsecutiveCalls( + [$filesNode1, $filesNode2], + [$filesNode2, $filesNode3], + ); $rules = [ ['name' => '{http://owncloud.org/ns}systemtag', 'value' => '123'], ['name' => '{http://owncloud.org/ns}systemtag', 'value' => '456'], ]; - $this->assertEquals(['222'], array_values($this->invokePrivate($this->plugin, 'processFilterRules', [$rules]))); + $this->assertEquals([$filesNode2], array_values($this->invokePrivate($this->plugin, 'processFilterRulesForFileNodes', [$rules, null, null]))); } - public function testProcessFilterRulesAndConditionWithOneEmptyResult() { + public function testProcessFilterRulesAndConditionWithOneEmptyResult(): void { $this->groupManager->expects($this->any()) ->method('isAdmin') ->willReturn(true); - $this->tagMapper->expects($this->exactly(2)) - ->method('getObjectIdsForTags') + $filesNode1 = $this->createMock(File::class); + $filesNode1->expects($this->any()) + ->method('getSize') + ->willReturn(12); + $filesNode1->expects($this->any()) + ->method('getId') + ->willReturn(111); + $filesNode2 = $this->createMock(Folder::class); + $filesNode2->expects($this->any()) + ->method('getSize') + ->willReturn(10); + $filesNode2->expects($this->any()) + ->method('getId') + ->willReturn(222); + + $tag123 = $this->createMock(ISystemTag::class); + $tag123->expects($this->any()) + ->method('getName') + ->willReturn('OneTwoThree'); + $tag123->expects($this->any()) + ->method('isUserVisible') + ->willReturn(true); + $tag456 = $this->createMock(ISystemTag::class); + $tag456->expects($this->any()) + ->method('getName') + ->willReturn('FourFiveSix'); + $tag456->expects($this->any()) + ->method('isUserVisible') + ->willReturn(true); + + $this->tagManager->expects($this->once()) + ->method('getTagsByIds') + ->with(['123', '456']) + ->willReturn([$tag123, $tag456]); + + $this->userFolder->expects($this->exactly(2)) + ->method('searchBySystemTag') ->withConsecutive( - ['123', 'files'], - ['456', 'files'] + ['OneTwoThree'], + ['FourFiveSix'], ) - ->willReturnMap([ - ['123', 'files', 0, '', ['111', '222']], - ['456', 'files', 0, '', []], - ]); + ->willReturnOnConsecutiveCalls( + [$filesNode1, $filesNode2], + [], + ); $rules = [ ['name' => '{http://owncloud.org/ns}systemtag', 'value' => '123'], ['name' => '{http://owncloud.org/ns}systemtag', 'value' => '456'], ]; - $this->assertEquals([], array_values($this->invokePrivate($this->plugin, 'processFilterRules', [$rules]))); + $this->assertEquals([], $this->invokePrivate($this->plugin, 'processFilterRulesForFileNodes', [$rules, null, null])); } - public function testProcessFilterRulesAndConditionWithFirstEmptyResult() { + public function testProcessFilterRulesAndConditionWithFirstEmptyResult(): void { $this->groupManager->expects($this->any()) ->method('isAdmin') ->willReturn(true); - $this->tagMapper->expects($this->exactly(1)) - ->method('getObjectIdsForTags') - ->withConsecutive( - ['123', 'files'], - ['456', 'files'] - ) - ->willReturnMap([ - ['123', 'files', 0, '', []], - ['456', 'files', 0, '', ['111', '222']], - ]); + $filesNode1 = $this->createMock(File::class); + $filesNode1->expects($this->any()) + ->method('getSize') + ->willReturn(12); + $filesNode1->expects($this->any()) + ->method('getId') + ->willReturn(111); + $filesNode2 = $this->createMock(Folder::class); + $filesNode2->expects($this->any()) + ->method('getSize') + ->willReturn(10); + $filesNode2->expects($this->any()) + ->method('getId') + ->willReturn(222); + + $tag123 = $this->createMock(ISystemTag::class); + $tag123->expects($this->any()) + ->method('getName') + ->willReturn('OneTwoThree'); + $tag123->expects($this->any()) + ->method('isUserVisible') + ->willReturn(true); + $tag456 = $this->createMock(ISystemTag::class); + $tag456->expects($this->any()) + ->method('getName') + ->willReturn('FourFiveSix'); + $tag456->expects($this->any()) + ->method('isUserVisible') + ->willReturn(true); + + $this->tagManager->expects($this->once()) + ->method('getTagsByIds') + ->with(['123', '456']) + ->willReturn([$tag123, $tag456]); + + $this->userFolder->expects($this->once()) + ->method('searchBySystemTag') + ->with('OneTwoThree') + ->willReturnOnConsecutiveCalls( + [], + [$filesNode1, $filesNode2], + ); $rules = [ ['name' => '{http://owncloud.org/ns}systemtag', 'value' => '123'], ['name' => '{http://owncloud.org/ns}systemtag', 'value' => '456'], ]; - $this->assertEquals([], array_values($this->invokePrivate($this->plugin, 'processFilterRules', [$rules]))); + $this->assertEquals([], $this->invokePrivate($this->plugin, 'processFilterRulesForFileNodes', [$rules, null, null])); } - public function testProcessFilterRulesAndConditionWithEmptyMidResult() { + public function testProcessFilterRulesAndConditionWithEmptyMidResult(): void { $this->groupManager->expects($this->any()) ->method('isAdmin') ->willReturn(true); - $this->tagMapper->expects($this->exactly(2)) - ->method('getObjectIdsForTags') - ->withConsecutive( - ['123', 'files'], - ['456', 'files'], - ['789', 'files'] - ) - ->willReturnMap([ - ['123', 'files', 0, '', ['111', '222']], - ['456', 'files', 0, '', ['333']], - ['789', 'files', 0, '', ['111', '222']], - ]); + $filesNode1 = $this->createMock(File::class); + $filesNode1->expects($this->any()) + ->method('getSize') + ->willReturn(12); + $filesNode1->expects($this->any()) + ->method('getId') + ->willReturn(111); + $filesNode2 = $this->createMock(Folder::class); + $filesNode2->expects($this->any()) + ->method('getSize') + ->willReturn(10); + $filesNode2->expects($this->any()) + ->method('getId') + ->willReturn(222); + $filesNode3 = $this->createMock(Folder::class); + $filesNode3->expects($this->any()) + ->method('getSize') + ->willReturn(13); + $filesNode3->expects($this->any()) + ->method('getId') + ->willReturn(333); + + $tag123 = $this->createMock(ISystemTag::class); + $tag123->expects($this->any()) + ->method('getName') + ->willReturn('OneTwoThree'); + $tag123->expects($this->any()) + ->method('isUserVisible') + ->willReturn(true); + $tag456 = $this->createMock(ISystemTag::class); + $tag456->expects($this->any()) + ->method('getName') + ->willReturn('FourFiveSix'); + $tag456->expects($this->any()) + ->method('isUserVisible') + ->willReturn(true); + $tag789 = $this->createMock(ISystemTag::class); + $tag789->expects($this->any()) + ->method('getName') + ->willReturn('SevenEightNein'); + $tag789->expects($this->any()) + ->method('isUserVisible') + ->willReturn(true); + + $this->tagManager->expects($this->once()) + ->method('getTagsByIds') + ->with(['123', '456', '789']) + ->willReturn([$tag123, $tag456, $tag789]); + + $this->userFolder->expects($this->exactly(2)) + ->method('searchBySystemTag') + ->withConsecutive(['OneTwoThree'], ['FourFiveSix'], ['SevenEightNein']) + ->willReturnOnConsecutiveCalls( + [$filesNode1, $filesNode2], + [$filesNode3], + [$filesNode1, $filesNode2], + ); $rules = [ ['name' => '{http://owncloud.org/ns}systemtag', 'value' => '123'], @@ -556,10 +750,10 @@ public function testProcessFilterRulesAndConditionWithEmptyMidResult() { ['name' => '{http://owncloud.org/ns}systemtag', 'value' => '789'], ]; - $this->assertEquals([], array_values($this->invokePrivate($this->plugin, 'processFilterRules', [$rules]))); + $this->assertEquals([], array_values($this->invokePrivate($this->plugin, 'processFilterRulesForFileNodes', [$rules, null, null]))); } - public function testProcessFilterRulesInvisibleTagAsAdmin() { + public function testProcessFilterRulesInvisibleTagAsAdmin(): void { $this->groupManager->expects($this->any()) ->method('isAdmin') ->willReturn(true); @@ -588,25 +782,27 @@ public function testProcessFilterRulesInvisibleTagAsAdmin() { $this->tagManager->expects($this->never()) ->method('getTagsByIds'); - $this->tagMapper->expects($this->at(0)) - ->method('getObjectIdsForTags') - ->with('123') - ->willReturn(['111', '222']); - $this->tagMapper->expects($this->at(1)) + $this->tagMapper->expects($this->exactly(2)) ->method('getObjectIdsForTags') - ->with('456') - ->willReturn(['222', '333']); + ->withConsecutive( + ['123'], + ['456'], + ) + ->willReturnOnConsecutiveCalls( + ['111', '222'], + ['222', '333'], + ); $rules = [ ['name' => '{http://owncloud.org/ns}systemtag', 'value' => '123'], ['name' => '{http://owncloud.org/ns}systemtag', 'value' => '456'], ]; - $this->assertEquals(['222'], array_values($this->invokePrivate($this->plugin, 'processFilterRules', [$rules]))); + $this->assertEquals(['222'], array_values($this->invokePrivate($this->plugin, 'processFilterRulesForFileNodes', [$rules, null, null]))); } - public function testProcessFilterRulesInvisibleTagAsUser() { + public function testProcessFilterRulesInvisibleTagAsUser(): void { $this->expectException(\OCP\SystemTag\TagNotFoundException::class); $this->groupManager->expects($this->any()) @@ -643,57 +839,69 @@ public function testProcessFilterRulesInvisibleTagAsUser() { ['name' => '{http://owncloud.org/ns}systemtag', 'value' => '456'], ]; - $this->invokePrivate($this->plugin, 'processFilterRules', [$rules]); + $this->invokePrivate($this->plugin, 'processFilterRulesForFileNodes', [$rules, null, null]); } - public function testProcessFilterRulesVisibleTagAsUser() { + public function testProcessFilterRulesVisibleTagAsUser(): void { $this->groupManager->expects($this->any()) ->method('isAdmin') ->willReturn(false); - $tag1 = $this->getMockBuilder(ISystemTag::class) - ->disableOriginalConstructor() - ->getMock(); + $tag1 = $this->createMock(ISystemTag::class); $tag1->expects($this->any()) ->method('getId') ->willReturn('123'); $tag1->expects($this->any()) ->method('isUserVisible') ->willReturn(true); + $tag1->expects($this->any()) + ->method('getName') + ->willReturn('OneTwoThree'); - $tag2 = $this->getMockBuilder(ISystemTag::class) - ->disableOriginalConstructor() - ->getMock(); + $tag2 = $this->createMock(ISystemTag::class); $tag2->expects($this->any()) ->method('getId') ->willReturn('123'); $tag2->expects($this->any()) ->method('isUserVisible') ->willReturn(true); + $tag2->expects($this->any()) + ->method('getName') + ->willReturn('FourFiveSix'); $this->tagManager->expects($this->once()) ->method('getTagsByIds') ->with(['123', '456']) ->willReturn([$tag1, $tag2]); - $this->tagMapper->expects($this->at(0)) - ->method('getObjectIdsForTags') - ->with('123') - ->willReturn(['111', '222']); - $this->tagMapper->expects($this->at(1)) - ->method('getObjectIdsForTags') - ->with('456') - ->willReturn(['222', '333']); + $filesNode1 = $this->createMock(File::class); + $filesNode1->expects($this->any()) + ->method('getSize') + ->willReturn(12); + $filesNode2 = $this->createMock(Folder::class); + $filesNode2->expects($this->any()) + ->method('getSize') + ->willReturn(10); + + $this->tagManager->expects($this->once()) + ->method('getTagsByIds') + ->with(['123', '456']) + ->willReturn([$tag1, $tag2]); + + // main assertion: only user visible tags are being passed through. + $this->userFolder->expects($this->exactly(1)) + ->method('searchBySystemTag') + ->with('FourFiveSix', $this->anything(), $this->anything(), $this->anything()); $rules = [ ['name' => '{http://owncloud.org/ns}systemtag', 'value' => '123'], ['name' => '{http://owncloud.org/ns}systemtag', 'value' => '456'], ]; - $this->assertEquals(['222'], array_values($this->invokePrivate($this->plugin, 'processFilterRules', [$rules]))); + $this->invokePrivate($this->plugin, 'processFilterRulesForFileNodes', [$rules, null, null]); } - public function testProcessFavoriteFilter() { + public function testProcessFavoriteFilter(): void { $rules = [ ['name' => '{http://owncloud.org/ns}favorite', 'value' => '1'], ]; @@ -702,7 +910,7 @@ public function testProcessFavoriteFilter() { ->method('getFavorites') ->willReturn(['456', '789']); - $this->assertEquals(['456', '789'], array_values($this->invokePrivate($this->plugin, 'processFilterRules', [$rules]))); + $this->assertEquals(['456', '789'], array_values($this->invokePrivate($this->plugin, 'processFilterRulesForFileIDs', [$rules]))); } public function filesBaseUriProvider() { @@ -718,7 +926,7 @@ public function filesBaseUriProvider() { /** * @dataProvider filesBaseUriProvider */ - public function testFilesBaseUri($uri, $reportPath, $expectedUri) { + public function testFilesBaseUri($uri, $reportPath, $expectedUri): void { $this->assertEquals($expectedUri, $this->invokePrivate($this->plugin, 'getFilesBaseUri', [$uri, $reportPath])); } } From d21b251a8581a5035692e3e75ee1d9c89b9d0558 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Thu, 15 Jun 2023 22:46:04 +0200 Subject: [PATCH 08/15] fix: include invisible tags for admins Signed-off-by: Arthur Schiwon --- .../lib/Connector/Sabre/FilesReportPlugin.php | 8 +- .../Connector/Sabre/FilesReportPluginTest.php | 104 ++++++++++-------- lib/private/Files/Cache/QuerySearchHelper.php | 60 +++++----- lib/private/Files/Cache/SearchBuilder.php | 17 ++- lib/private/SystemTag/SystemTagManager.php | 9 +- lib/public/SystemTag/ISystemTagManager.php | 5 +- 6 files changed, 106 insertions(+), 97 deletions(-) diff --git a/apps/dav/lib/Connector/Sabre/FilesReportPlugin.php b/apps/dav/lib/Connector/Sabre/FilesReportPlugin.php index c0cd382d994f7..4fabe9fdd1c0a 100644 --- a/apps/dav/lib/Connector/Sabre/FilesReportPlugin.php +++ b/apps/dav/lib/Connector/Sabre/FilesReportPlugin.php @@ -283,8 +283,6 @@ private function getFilesBaseUri(string $uri, string $subPath): string { * * @param array $filterRules * @return array array of unique file id results - * - * @throws TagNotFoundException whenever a tag was not found */ protected function processFilterRulesForFileIDs($filterRules) { $ns = '{' . $this::NS_OWNCLOUD . '}'; @@ -335,12 +333,8 @@ protected function processFilterRulesForFileNodes(array $filterRules, ?int $limi !empty($systemTagIds) && (method_exists($this->userFolder, 'searchBySystemTag')) ) { - $tags = $this->tagManager->getTagsByIds($systemTagIds); + $tags = $this->tagManager->getTagsByIds($systemTagIds, $this->userSession->getUser()); foreach ($tags as $tag) { - if (!$tag->isUserVisible()) { - // searchBySystemTag() also has the criteria to include only user visible tags. They can be skipped early nevertheless. - continue; - } $tagName = $tag->getName(); $tmpNodes = $this->userFolder->searchBySystemTag($tagName, $this->userSession->getUser()->getUID(), $limit ?? 0, $offset ?? 0); if (count($nodes) === 0) { diff --git a/apps/dav/tests/unit/Connector/Sabre/FilesReportPluginTest.php b/apps/dav/tests/unit/Connector/Sabre/FilesReportPluginTest.php index 5c81757b11ac6..7cb2d85fe1ff6 100644 --- a/apps/dav/tests/unit/Connector/Sabre/FilesReportPluginTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/FilesReportPluginTest.php @@ -44,6 +44,7 @@ use OCP\SystemTag\ISystemTag; use OCP\SystemTag\ISystemTagManager; use OCP\SystemTag\ISystemTagObjectMapper; +use OCP\SystemTag\TagNotFoundException; use PHPUnit\Framework\MockObject\MockObject; use Sabre\DAV\INode; use Sabre\DAV\Tree; @@ -734,7 +735,7 @@ public function testProcessFilterRulesAndConditionWithEmptyMidResult(): void { ->method('getTagsByIds') ->with(['123', '456', '789']) ->willReturn([$tag123, $tag456, $tag789]); - + $this->userFolder->expects($this->exactly(2)) ->method('searchBySystemTag') ->withConsecutive(['OneTwoThree'], ['FourFiveSix'], ['SevenEightNein']) @@ -758,39 +759,54 @@ public function testProcessFilterRulesInvisibleTagAsAdmin(): void { ->method('isAdmin') ->willReturn(true); - $tag1 = $this->getMockBuilder(ISystemTag::class) - ->disableOriginalConstructor() - ->getMock(); - $tag1->expects($this->any()) + $filesNode1 = $this->createMock(File::class); + $filesNode1->expects($this->any()) + ->method('getSize') + ->willReturn(12); + $filesNode1->expects($this->any()) ->method('getId') - ->willReturn('123'); - $tag1->expects($this->any()) + ->willReturn(111); + $filesNode2 = $this->createMock(Folder::class); + $filesNode2->expects($this->any()) + ->method('getSize') + ->willReturn(10); + $filesNode2->expects($this->any()) + ->method('getId') + ->willReturn(222); + $filesNode3 = $this->createMock(Folder::class); + $filesNode3->expects($this->any()) + ->method('getSize') + ->willReturn(13); + $filesNode3->expects($this->any()) + ->method('getId') + ->willReturn(333); + + $tag123 = $this->createMock(ISystemTag::class); + $tag123->expects($this->any()) + ->method('getName') + ->willReturn('OneTwoThree'); + $tag123->expects($this->any()) ->method('isUserVisible') ->willReturn(true); - - $tag2 = $this->getMockBuilder(ISystemTag::class) - ->disableOriginalConstructor() - ->getMock(); - $tag2->expects($this->any()) - ->method('getId') - ->willReturn('123'); - $tag2->expects($this->any()) + $tag456 = $this->createMock(ISystemTag::class); + $tag456->expects($this->any()) + ->method('getName') + ->willReturn('FourFiveSix'); + $tag456->expects($this->any()) ->method('isUserVisible') ->willReturn(false); - // no need to fetch tags to check permissions - $this->tagManager->expects($this->never()) - ->method('getTagsByIds'); + $this->tagManager->expects($this->once()) + ->method('getTagsByIds') + ->with(['123', '456']) + ->willReturn([$tag123, $tag456]); - $this->tagMapper->expects($this->exactly(2)) - ->method('getObjectIdsForTags') - ->withConsecutive( - ['123'], - ['456'], - ) + $this->userFolder->expects($this->exactly(2)) + ->method('searchBySystemTag') + ->withConsecutive(['OneTwoThree'], ['FourFiveSix']) ->willReturnOnConsecutiveCalls( - ['111', '222'], - ['222', '333'], + [$filesNode1, $filesNode2], + [$filesNode2, $filesNode3], ); $rules = [ @@ -798,41 +814,39 @@ public function testProcessFilterRulesInvisibleTagAsAdmin(): void { ['name' => '{http://owncloud.org/ns}systemtag', 'value' => '456'], ]; - $this->assertEquals(['222'], array_values($this->invokePrivate($this->plugin, 'processFilterRulesForFileNodes', [$rules, null, null]))); + $this->assertEquals([$filesNode2], array_values($this->invokePrivate($this->plugin, 'processFilterRulesForFileNodes', [$rules, null, null]))); } public function testProcessFilterRulesInvisibleTagAsUser(): void { - $this->expectException(\OCP\SystemTag\TagNotFoundException::class); + $this->expectException(TagNotFoundException::class); $this->groupManager->expects($this->any()) ->method('isAdmin') ->willReturn(false); - $tag1 = $this->getMockBuilder(ISystemTag::class) - ->disableOriginalConstructor() - ->getMock(); - $tag1->expects($this->any()) - ->method('getId') - ->willReturn('123'); - $tag1->expects($this->any()) + $tag123 = $this->createMock(ISystemTag::class); + $tag123->expects($this->any()) + ->method('getName') + ->willReturn('OneTwoThree'); + $tag123->expects($this->any()) ->method('isUserVisible') ->willReturn(true); - - $tag2 = $this->getMockBuilder(ISystemTag::class) - ->disableOriginalConstructor() - ->getMock(); - $tag2->expects($this->any()) - ->method('getId') - ->willReturn('123'); - $tag2->expects($this->any()) + $tag456 = $this->createMock(ISystemTag::class); + $tag456->expects($this->any()) + ->method('getName') + ->willReturn('FourFiveSix'); + $tag456->expects($this->any()) ->method('isUserVisible') - ->willReturn(false); // invisible + ->willReturn(false); $this->tagManager->expects($this->once()) ->method('getTagsByIds') ->with(['123', '456']) - ->willReturn([$tag1, $tag2]); + ->willThrowException(new TagNotFoundException()); + + $this->userFolder->expects($this->never()) + ->method('searchBySystemTag'); $rules = [ ['name' => '{http://owncloud.org/ns}systemtag', 'value' => '123'], diff --git a/lib/private/Files/Cache/QuerySearchHelper.php b/lib/private/Files/Cache/QuerySearchHelper.php index d020ec00cea5e..8732769040ef0 100644 --- a/lib/private/Files/Cache/QuerySearchHelper.php +++ b/lib/private/Files/Cache/QuerySearchHelper.php @@ -36,9 +36,9 @@ use OCP\Files\IRootFolder; use OCP\Files\Mount\IMountPoint; use OCP\Files\Search\ISearchBinaryOperator; -use OCP\Files\Search\ISearchComparison; use OCP\Files\Search\ISearchQuery; use OCP\IDBConnection; +use OCP\IGroupManager; use OCP\IUser; use Psr\Log\LoggerInterface; @@ -55,6 +55,7 @@ class QuerySearchHelper { private $searchBuilder; /** @var QueryOptimizer */ private $queryOptimizer; + private IGroupManager $groupManager; public function __construct( IMimeTypeLoader $mimetypeLoader, @@ -62,7 +63,8 @@ public function __construct( SystemConfig $systemConfig, LoggerInterface $logger, SearchBuilder $searchBuilder, - QueryOptimizer $queryOptimizer + QueryOptimizer $queryOptimizer, + IGroupManager $groupManager, ) { $this->mimetypeLoader = $mimetypeLoader; $this->connection = $connection; @@ -70,6 +72,7 @@ public function __construct( $this->logger = $logger; $this->searchBuilder = $searchBuilder; $this->queryOptimizer = $queryOptimizer; + $this->groupManager = $groupManager; } protected function getQueryBuilder() { @@ -119,16 +122,16 @@ public function findUsedTagsInCaches(ISearchQuery $searchQuery, array $caches): return $tags; } - protected function equipQueryForSystemTags(CacheQueryBuilder $query): void { - $query - ->leftJoin('file', 'systemtag_object_mapping', 'systemtagmap', $query->expr()->andX( - $query->expr()->eq('file.fileid', $query->expr()->castColumn('systemtagmap.objectid', IQueryBuilder::PARAM_INT)), - $query->expr()->eq('systemtagmap.objecttype', $query->createNamedParameter('files')) - )) - ->leftJoin('systemtagmap', 'systemtag', 'systemtag', $query->expr()->andX( - $query->expr()->eq('systemtag.id', 'systemtagmap.systemtagid'), - $query->expr()->eq('systemtag.visibility', $query->createNamedParameter(true)) - )); + protected function equipQueryForSystemTags(CacheQueryBuilder $query, IUser $user): void { + $query->leftJoin('file', 'systemtag_object_mapping', 'systemtagmap', $query->expr()->andX( + $query->expr()->eq('file.fileid', $query->expr()->castColumn('systemtagmap.objectid', IQueryBuilder::PARAM_INT)), + $query->expr()->eq('systemtagmap.objecttype', $query->createNamedParameter('files')) + )); + $on = $query->expr()->andX($query->expr()->eq('systemtag.id', 'systemtagmap.systemtagid')); + if (!$this->groupManager->isAdmin($user->getUID())) { + $on->add($query->expr()->eq('systemtag.visibility', $query->createNamedParameter(true))); + } + $query->leftJoin('systemtagmap', 'systemtag', 'systemtag', $on); } protected function equipQueryForDavTags(CacheQueryBuilder $query, IUser $user): void { @@ -172,25 +175,12 @@ public function searchInCaches(ISearchQuery $searchQuery, array $caches): array $query = $builder->selectFileCache('file', false); - if ($this->searchBuilder->shouldJoinTags($searchQuery->getSearchOperation())) { - $user = $searchQuery->getUser(); - if ($user === null) { - throw new \InvalidArgumentException("Searching by tag requires the user to be set in the query"); - } - if ($searchQuery->getSearchOperation() instanceof ISearchComparison) { - switch ($searchQuery->getSearchOperation()->getField()) { - case 'systemtag': - $this->equipQueryForSystemTags($query); - break; - case 'tagname': - case 'favorite': - $this->equipQueryForDavTags($query, $user); - break; - } - } elseif ($searchQuery->getSearchOperation() instanceof SearchBinaryOperator) { - $this->equipQueryForSystemTags($query); - $this->equipQueryForDavTags($query, $user); - } + $requestedFields = $this->searchBuilder->extractRequestedFields($searchQuery->getSearchOperation()); + if (in_array('systemtag', $requestedFields)) { + $this->equipQueryForSystemTags($query, $this->requireUser($searchQuery)); + } + if (in_array('tagname', $requestedFields) || in_array('favorite', $requestedFields)) { + $this->equipQueryForDavTags($query, $this->requireUser($searchQuery)); } $this->applySearchConstraints($query, $searchQuery, $caches); @@ -218,6 +208,14 @@ public function searchInCaches(ISearchQuery $searchQuery, array $caches): array return $results; } + protected function requireUser(ISearchQuery $searchQuery): IUser { + $user = $searchQuery->getUser(); + if ($user === null) { + throw new \InvalidArgumentException("This search operation requires the user to be set in the query"); + } + return $user; + } + /** * @return array{0?: array, 1?: array} */ diff --git a/lib/private/Files/Cache/SearchBuilder.php b/lib/private/Files/Cache/SearchBuilder.php index 1a8c3637063de..a14edc3e55e99 100644 --- a/lib/private/Files/Cache/SearchBuilder.php +++ b/lib/private/Files/Cache/SearchBuilder.php @@ -69,20 +69,17 @@ public function __construct( } /** - * Whether or not the tag tables should be joined to complete the search - * - * @param ISearchOperator $operator - * @return boolean + * @return string[] */ - public function shouldJoinTags(ISearchOperator $operator) { + public function extractRequestedFields(ISearchOperator $operator): array { if ($operator instanceof ISearchBinaryOperator) { - return array_reduce($operator->getArguments(), function ($shouldJoin, ISearchOperator $operator) { - return $shouldJoin || $this->shouldJoinTags($operator); - }, false); + return array_reduce($operator->getArguments(), function (array $fields, ISearchOperator $operator) { + return array_unique(array_merge($fields, $this->extractRequestedFields($operator))); + }, []); } elseif ($operator instanceof ISearchComparison) { - return $operator->getField() === 'tagname' || $operator->getField() === 'favorite' || $operator->getField() === 'systemtag'; + return [$operator->getField()]; } - return false; + return []; } /** diff --git a/lib/private/SystemTag/SystemTagManager.php b/lib/private/SystemTag/SystemTagManager.php index 79c5adcf450ad..65046ebf000b7 100644 --- a/lib/private/SystemTag/SystemTagManager.php +++ b/lib/private/SystemTag/SystemTagManager.php @@ -91,7 +91,7 @@ public function __construct( /** * {@inheritdoc} */ - public function getTagsByIds($tagIds): array { + public function getTagsByIds($tagIds, ?IUser $user = null): array { if (!\is_array($tagIds)) { $tagIds = [$tagIds]; } @@ -116,7 +116,12 @@ public function getTagsByIds($tagIds): array { $result = $query->execute(); while ($row = $result->fetch()) { - $tags[$row['id']] = $this->createSystemTagFromRow($row); + $tag = $this->createSystemTagFromRow($row); + if ($user && !$this->canUserSeeTag($tag, $user)) { + // if a user is given, hide invisible tags + continue; + } + $tags[$row['id']] = $tag; } $result->closeCursor(); diff --git a/lib/public/SystemTag/ISystemTagManager.php b/lib/public/SystemTag/ISystemTagManager.php index 500d80ea2783a..3616bb1ef4109 100644 --- a/lib/public/SystemTag/ISystemTagManager.php +++ b/lib/public/SystemTag/ISystemTagManager.php @@ -39,6 +39,7 @@ interface ISystemTagManager { * Returns the tag objects matching the given tag ids. * * @param array|string $tagIds id or array of unique ids of the tag to retrieve + * @param ?IUser $user optional user to run a visibility check against for each tag * * @return ISystemTag[] array of system tags with tag id as key * @@ -46,9 +47,9 @@ interface ISystemTagManager { * @throws TagNotFoundException if at least one given tag ids did no exist * The message contains a json_encoded array of the ids that could not be found * - * @since 9.0.0 + * @since 9.0.0, optional parameter $user added in 28.0.0 */ - public function getTagsByIds($tagIds): array; + public function getTagsByIds($tagIds, ?IUser $user = null): array; /** * Returns the tag object matching the given attributes. From 1ffae9d110343d9dbf48e0fb96ce83e6b51cd763 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Wed, 21 Jun 2023 18:01:49 +0200 Subject: [PATCH 09/15] fix: cominbation of small fixes - possible null return - parameter name mismatch in implementation - incomplete unit test Signed-off-by: Arthur Schiwon --- .../lib/Connector/Sabre/FilesReportPlugin.php | 6 ++--- .../Connector/Sabre/FilesReportPluginTest.php | 23 ++++++++++++++++--- lib/private/Files/Node/Folder.php | 4 ++-- 3 files changed, 25 insertions(+), 8 deletions(-) diff --git a/apps/dav/lib/Connector/Sabre/FilesReportPlugin.php b/apps/dav/lib/Connector/Sabre/FilesReportPlugin.php index 4fabe9fdd1c0a..450fe1347725d 100644 --- a/apps/dav/lib/Connector/Sabre/FilesReportPlugin.php +++ b/apps/dav/lib/Connector/Sabre/FilesReportPlugin.php @@ -284,9 +284,9 @@ private function getFilesBaseUri(string $uri, string $subPath): string { * @param array $filterRules * @return array array of unique file id results */ - protected function processFilterRulesForFileIDs($filterRules) { + protected function processFilterRulesForFileIDs(array $filterRules): array { $ns = '{' . $this::NS_OWNCLOUD . '}'; - $resultFileIds = null; + $resultFileIds = []; $circlesIds = []; $favoriteFilter = null; foreach ($filterRules as $filterRule) { @@ -407,7 +407,7 @@ public function prepareResponses($filesUri, $requestedProps, $nodes) { * @param array $fileIds file ids * @return Node[] array of Sabre nodes */ - public function findNodesByFileIds($rootNode, $fileIds): array { + public function findNodesByFileIds(Node $rootNode, array $fileIds): array { if (empty($fileIds)) { return []; } diff --git a/apps/dav/tests/unit/Connector/Sabre/FilesReportPluginTest.php b/apps/dav/tests/unit/Connector/Sabre/FilesReportPluginTest.php index 7cb2d85fe1ff6..f1f1cc8b27f5e 100644 --- a/apps/dav/tests/unit/Connector/Sabre/FilesReportPluginTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/FilesReportPluginTest.php @@ -889,13 +889,26 @@ public function testProcessFilterRulesVisibleTagAsUser(): void { ->willReturn([$tag1, $tag2]); $filesNode1 = $this->createMock(File::class); + $filesNode1->expects($this->any()) + ->method('getId') + ->willReturn(111); $filesNode1->expects($this->any()) ->method('getSize') ->willReturn(12); $filesNode2 = $this->createMock(Folder::class); + $filesNode2->expects($this->any()) + ->method('getId') + ->willReturn(222); $filesNode2->expects($this->any()) ->method('getSize') ->willReturn(10); + $filesNode3 = $this->createMock(Folder::class); + $filesNode3->expects($this->any()) + ->method('getId') + ->willReturn(333); + $filesNode3->expects($this->any()) + ->method('getSize') + ->willReturn(33); $this->tagManager->expects($this->once()) ->method('getTagsByIds') @@ -903,16 +916,20 @@ public function testProcessFilterRulesVisibleTagAsUser(): void { ->willReturn([$tag1, $tag2]); // main assertion: only user visible tags are being passed through. - $this->userFolder->expects($this->exactly(1)) + $this->userFolder->expects($this->exactly(2)) ->method('searchBySystemTag') - ->with('FourFiveSix', $this->anything(), $this->anything(), $this->anything()); + ->withConsecutive(['OneTwoThree'], ['FourFiveSix']) + ->willReturnOnConsecutiveCalls( + [$filesNode1, $filesNode2], + [$filesNode2, $filesNode3], + ); $rules = [ ['name' => '{http://owncloud.org/ns}systemtag', 'value' => '123'], ['name' => '{http://owncloud.org/ns}systemtag', 'value' => '456'], ]; - $this->invokePrivate($this->plugin, 'processFilterRulesForFileNodes', [$rules, null, null]); + $this->assertEquals([$filesNode2], array_values($this->invokePrivate($this->plugin, 'processFilterRulesForFileNodes', [$rules, null, null]))); } public function testProcessFavoriteFilter(): void { diff --git a/lib/private/Files/Node/Folder.php b/lib/private/Files/Node/Folder.php index f409f5e63f405..9f84993000f42 100644 --- a/lib/private/Files/Node/Folder.php +++ b/lib/private/Files/Node/Folder.php @@ -301,8 +301,8 @@ public function searchByTag($tag, $userId) { return $this->search($query); } - public function searchBySystemTag(string $tag, string $userId, int $limit = 0, int $offset = 0): array { - $query = $this->queryFromOperator(new SearchComparison(ISearchComparison::COMPARE_EQUAL, 'systemtag', $tag), $userId, $limit, $offset); + public function searchBySystemTag(string $tagName, string $userId, int $limit = 0, int $offset = 0): array { + $query = $this->queryFromOperator(new SearchComparison(ISearchComparison::COMPARE_EQUAL, 'systemtag', $tagName), $userId, $limit, $offset); return $this->search($query); } From 9f549013b00fdf937619d2bf9b47d6c20d7522f1 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Wed, 21 Jun 2023 20:04:35 +0200 Subject: [PATCH 10/15] fix: obey offset and limit for results from favs and circles Signed-off-by: Arthur Schiwon --- apps/dav/lib/Connector/Sabre/FilesReportPlugin.php | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/apps/dav/lib/Connector/Sabre/FilesReportPlugin.php b/apps/dav/lib/Connector/Sabre/FilesReportPlugin.php index 450fe1347725d..a828f873086e0 100644 --- a/apps/dav/lib/Connector/Sabre/FilesReportPlugin.php +++ b/apps/dav/lib/Connector/Sabre/FilesReportPlugin.php @@ -219,6 +219,10 @@ public function onReport($reportName, $report, $uri) { // gather all file ids matching filter try { $resultFileIds = $this->processFilterRulesForFileIDs($filterRules); + // no logic in circles and favorites for paging, we always have all results, and slice later on + $resultFileIds = array_slice($resultFileIds, $offset ?? 0, $limit ?? null); + // fetching nodes has paging on DB level – therefore we cannot mix and slice the results, similar + // to user backends. I.e. the final result may return more results than requested. $resultNodes = $this->processFilterRulesForFileNodes($filterRules, $limit ?? null, $offset ?? null); } catch (TagNotFoundException $e) { throw new PreconditionFailed('Cannot filter by non-existing tag', 0, $e); From 2c162694f27e1b57a3b49f1c128242793fbbc894 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Wed, 21 Jun 2023 20:07:00 +0200 Subject: [PATCH 11/15] fix: use array_unitersect against objects Signed-off-by: Arthur Schiwon --- apps/dav/lib/Connector/Sabre/FilesReportPlugin.php | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/apps/dav/lib/Connector/Sabre/FilesReportPlugin.php b/apps/dav/lib/Connector/Sabre/FilesReportPlugin.php index a828f873086e0..1b66dee189662 100644 --- a/apps/dav/lib/Connector/Sabre/FilesReportPlugin.php +++ b/apps/dav/lib/Connector/Sabre/FilesReportPlugin.php @@ -30,6 +30,7 @@ use OC\Files\View; use OCP\App\IAppManager; use OCP\Files\Folder; +use OCP\Files\Node as INode; use OCP\IGroupManager; use OCP\ITagManager; use OCP\IUserSession; @@ -238,7 +239,9 @@ public function onReport($reportName, $report, $uri) { // find sabre nodes by file id, restricted to the root node path $additionalNodes = $this->findNodesByFileIds($reportTargetNode, $resultFileIds); if ($additionalNodes && $results) { - $results = array_intersect($results, $additionalNodes); + $results = array_uintersect($results, $additionalNodes, function (Node $a, Node $b): int { + return $a->getId() - $b->getId(); + }); } elseif (!$results && $additionalNodes) { $results = $additionalNodes; } @@ -344,7 +347,7 @@ protected function processFilterRulesForFileNodes(array $filterRules, ?int $limi if (count($nodes) === 0) { $nodes = $tmpNodes; } else { - $nodes = array_uintersect($nodes, $tmpNodes, function (\OCP\Files\Node $a, \OCP\Files\Node $b): int { + $nodes = array_uintersect($nodes, $tmpNodes, function (INode $a, INode $b): int { return $a->getId() - $b->getId(); }); } From 5cd984b76b21a10d14ee56520fbe71538e9f5ee2 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Wed, 21 Jun 2023 20:35:41 +0200 Subject: [PATCH 12/15] refactor: save unnecessary method_exists Signed-off-by: Arthur Schiwon --- apps/dav/lib/Connector/Sabre/FilesReportPlugin.php | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/apps/dav/lib/Connector/Sabre/FilesReportPlugin.php b/apps/dav/lib/Connector/Sabre/FilesReportPlugin.php index 1b66dee189662..6122ba1bfe717 100644 --- a/apps/dav/lib/Connector/Sabre/FilesReportPlugin.php +++ b/apps/dav/lib/Connector/Sabre/FilesReportPlugin.php @@ -336,10 +336,7 @@ protected function processFilterRulesForFileNodes(array $filterRules, ?int $limi // type check to ensure searchBySystemTag is available, it is not // exposed in API yet - if ( - !empty($systemTagIds) - && (method_exists($this->userFolder, 'searchBySystemTag')) - ) { + if (!empty($systemTagIds)) { $tags = $this->tagManager->getTagsByIds($systemTagIds, $this->userSession->getUser()); foreach ($tags as $tag) { $tagName = $tag->getName(); From 6256da1ade26e803887971424e113b8183f52d2a Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Tue, 27 Jun 2023 22:49:08 +0200 Subject: [PATCH 13/15] fix: cannot apply limit+offset on multi-tag-search Signed-off-by: Arthur Schiwon --- apps/dav/lib/Connector/Sabre/FilesReportPlugin.php | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/apps/dav/lib/Connector/Sabre/FilesReportPlugin.php b/apps/dav/lib/Connector/Sabre/FilesReportPlugin.php index 6122ba1bfe717..4b26d83777925 100644 --- a/apps/dav/lib/Connector/Sabre/FilesReportPlugin.php +++ b/apps/dav/lib/Connector/Sabre/FilesReportPlugin.php @@ -338,9 +338,15 @@ protected function processFilterRulesForFileNodes(array $filterRules, ?int $limi // exposed in API yet if (!empty($systemTagIds)) { $tags = $this->tagManager->getTagsByIds($systemTagIds, $this->userSession->getUser()); + + // For we run DB queries per tag and require intersection, we cannot apply limit and offset for DB queries on multi tag search. + $oneTagSearch = count($tags) === 1; + $dbLimit = $oneTagSearch ? $limit ?? 0 : 0; + $dbOffset = $oneTagSearch ? $offset ?? 0 : 0; + foreach ($tags as $tag) { $tagName = $tag->getName(); - $tmpNodes = $this->userFolder->searchBySystemTag($tagName, $this->userSession->getUser()->getUID(), $limit ?? 0, $offset ?? 0); + $tmpNodes = $this->userFolder->searchBySystemTag($tagName, $this->userSession->getUser()->getUID(), $dbLimit, $dbOffset); if (count($nodes) === 0) { $nodes = $tmpNodes; } else { @@ -353,6 +359,10 @@ protected function processFilterRulesForFileNodes(array $filterRules, ?int $limi return $nodes; } } + + if (!$oneTagSearch && ($limit !== null || $offset !== null)) { + $nodes = array_slice($nodes, $offset, $limit); + } } return $nodes; From 33b9ba5cd9d305ec15dc37f0650402d25c019066 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Thu, 6 Jul 2023 22:33:20 +0200 Subject: [PATCH 14/15] refactor: adjust to unexposed searchBySystemTag - in this backport we have to drop the breaking addition in \OCP\Files\Folder - this requires adjustments in check for the existance of the method but also in testing - another change in \OCP\SystemTag\ISystemTagManager can be applied as this interface is not implemented elsewhere Signed-off-by: Arthur Schiwon --- apps/dav/lib/Connector/Sabre/FilesReportPlugin.php | 2 +- apps/dav/tests/unit/Connector/Sabre/FilesReportPluginTest.php | 2 ++ lib/private/Files/Node/LazyFolder.php | 4 ++++ lib/public/SystemTag/ISystemTagManager.php | 2 +- 4 files changed, 8 insertions(+), 2 deletions(-) diff --git a/apps/dav/lib/Connector/Sabre/FilesReportPlugin.php b/apps/dav/lib/Connector/Sabre/FilesReportPlugin.php index 4b26d83777925..1c6727e68ca44 100644 --- a/apps/dav/lib/Connector/Sabre/FilesReportPlugin.php +++ b/apps/dav/lib/Connector/Sabre/FilesReportPlugin.php @@ -336,7 +336,7 @@ protected function processFilterRulesForFileNodes(array $filterRules, ?int $limi // type check to ensure searchBySystemTag is available, it is not // exposed in API yet - if (!empty($systemTagIds)) { + if (!empty($systemTagIds) && method_exists($this->userFolder, 'searchBySystemTag')) { $tags = $this->tagManager->getTagsByIds($systemTagIds, $this->userSession->getUser()); // For we run DB queries per tag and require intersection, we cannot apply limit and offset for DB queries on multi tag search. diff --git a/apps/dav/tests/unit/Connector/Sabre/FilesReportPluginTest.php b/apps/dav/tests/unit/Connector/Sabre/FilesReportPluginTest.php index f1f1cc8b27f5e..2bbe7bef6defc 100644 --- a/apps/dav/tests/unit/Connector/Sabre/FilesReportPluginTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/FilesReportPluginTest.php @@ -114,6 +114,8 @@ protected function setUp(): void { $this->userFolder = $this->getMockBuilder(Folder::class) ->disableOriginalConstructor() + ->addMethods(['searchBySystemTag']) + ->onlyMethods(get_class_methods(Folder::class)) ->getMock(); $this->previewManager = $this->getMockBuilder(IPreview::class) diff --git a/lib/private/Files/Node/LazyFolder.php b/lib/private/Files/Node/LazyFolder.php index fc17ed2eb52b4..859ac3439be0d 100644 --- a/lib/private/Files/Node/LazyFolder.php +++ b/lib/private/Files/Node/LazyFolder.php @@ -448,6 +448,10 @@ public function searchByTag($tag, $userId) { return $this->__call(__FUNCTION__, func_get_args()); } + public function searchBySystemTag(string $tagName, string $userId, int $limit = 0, int $offset = 0): array { + return $this->__call(__FUNCTION__, func_get_args()); + } + /** * @inheritDoc */ diff --git a/lib/public/SystemTag/ISystemTagManager.php b/lib/public/SystemTag/ISystemTagManager.php index 3616bb1ef4109..d28fef560aa76 100644 --- a/lib/public/SystemTag/ISystemTagManager.php +++ b/lib/public/SystemTag/ISystemTagManager.php @@ -47,7 +47,7 @@ interface ISystemTagManager { * @throws TagNotFoundException if at least one given tag ids did no exist * The message contains a json_encoded array of the ids that could not be found * - * @since 9.0.0, optional parameter $user added in 28.0.0 + * @since 9.0.0, optional parameter $user added in 25.0.9 */ public function getTagsByIds($tagIds, ?IUser $user = null): array; From 2ca8c7102bcf65d3f5ce58696d9bade504416c87 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Mon, 10 Jul 2023 12:07:08 +0200 Subject: [PATCH 15/15] fix: PHP 7.4 compatibility Signed-off-by: Arthur Schiwon --- apps/dav/lib/Connector/Sabre/FilesReportPlugin.php | 6 +++++- .../tests/unit/Connector/Sabre/FilesReportPluginTest.php | 3 ++- lib/private/Files/Cache/QuerySearchHelper.php | 2 +- lib/private/Files/Node/Folder.php | 2 +- 4 files changed, 9 insertions(+), 4 deletions(-) diff --git a/apps/dav/lib/Connector/Sabre/FilesReportPlugin.php b/apps/dav/lib/Connector/Sabre/FilesReportPlugin.php index 1c6727e68ca44..8ae457b2e59a1 100644 --- a/apps/dav/lib/Connector/Sabre/FilesReportPlugin.php +++ b/apps/dav/lib/Connector/Sabre/FilesReportPlugin.php @@ -442,7 +442,11 @@ public function findNodesByFileIds(Node $rootNode, array $fileIds): array { return $results; } - protected function wrapNode(\OCP\Files\File|\OCP\Files\Folder $node): File|Directory { + /** + * @param \OCP\Files\File|\OCP\Files\Folder $node + * @return File|Directory + */ + protected function wrapNode($node): \OCA\DAV\Connector\Sabre\Node { if ($node instanceof \OCP\Files\File) { return new File($this->fileView, $node); } else { diff --git a/apps/dav/tests/unit/Connector/Sabre/FilesReportPluginTest.php b/apps/dav/tests/unit/Connector/Sabre/FilesReportPluginTest.php index 2bbe7bef6defc..db355d6500c86 100644 --- a/apps/dav/tests/unit/Connector/Sabre/FilesReportPluginTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/FilesReportPluginTest.php @@ -66,7 +66,8 @@ class FilesReportPluginTest extends \Test\TestCase { /** @var ITags|MockObject */ private $privateTags; - private ITagManager|MockObject $privateTagManager; + /** @var ITagManager|MockObject|(object&MockObject)|(ITagManager&object&MockObject)|(ITagManager&MockObject) */ + private $privateTagManager; /** @var \OCP\IUserSession */ private $userSession; diff --git a/lib/private/Files/Cache/QuerySearchHelper.php b/lib/private/Files/Cache/QuerySearchHelper.php index 8732769040ef0..df89f1261a306 100644 --- a/lib/private/Files/Cache/QuerySearchHelper.php +++ b/lib/private/Files/Cache/QuerySearchHelper.php @@ -64,7 +64,7 @@ public function __construct( LoggerInterface $logger, SearchBuilder $searchBuilder, QueryOptimizer $queryOptimizer, - IGroupManager $groupManager, + IGroupManager $groupManager ) { $this->mimetypeLoader = $mimetypeLoader; $this->connection = $connection; diff --git a/lib/private/Files/Node/Folder.php b/lib/private/Files/Node/Folder.php index 9f84993000f42..cb97c5a6d3270 100644 --- a/lib/private/Files/Node/Folder.php +++ b/lib/private/Files/Node/Folder.php @@ -331,7 +331,7 @@ protected function getAppDataDirectoryName(): string { * @param int $id * @return array */ - protected function getByIdInRootMount(int $id): array { + protected function getByIdInRootMount(int $id): array { if (!method_exists($this->root, 'createNode')) { // Always expected to be false. Being a method of Folder, this is // always implemented. For it is an internal method and should not