diff --git a/lib/Db/BookmarkMapper.php b/lib/Db/BookmarkMapper.php index b786248d5..bbeae161c 100644 --- a/lib/Db/BookmarkMapper.php +++ b/lib/Db/BookmarkMapper.php @@ -213,7 +213,7 @@ public function find(int $id): Bookmark { public function findAll(string $userId, QueryParameters $queryParams, bool $withGroupBy = true): array { $rootFolder = $this->folderMapper->findRootFolder($userId); // gives us all bookmarks in this folder, recursively - [$cte, $params, $paramTypes] = $this->_generateCTE($rootFolder->getId(), $queryParams->getSoftDeletedFolders()); + [$cte, $params, $paramTypes] = $this->generateCTE($rootFolder->getId(), $queryParams->getSoftDeletedFolders()); $qb = $this->db->getQueryBuilder(); $bookmark_cols = array_map(static function ($c) { @@ -282,11 +282,14 @@ protected function getIteratorWithRawQuery(string $query, array $params, array $ } /** - * Common table expression that lists all items in a given folder, recursively + * Common table expression that lists all items in a given folder, recursively. + * Public so other mappers (e.g. TagMapper) can reuse the same tree-traversal + * logic and stay consistent with how visibility is computed for bookmarks. + * * @param int $folderId * @return array */ - private function _generateCTE(int $folderId, bool $withSoftDeleted) : array { + public function generateCTE(int $folderId, bool $withSoftDeleted) : array { // The base case of the recursion is just the folder we're given $baseCase = $this->db->getQueryBuilder(); $baseCase @@ -761,7 +764,7 @@ public function findAllInPublicFolder(string $token, QueryParameters $queryParam $folder = $this->folderMapper->find($publicFolder->getFolderId()); // gives us all bookmarks in this folder, recursively - [$cte, $params, $paramTypes] = $this->_generateCTE($folder->getId(), false); + [$cte, $params, $paramTypes] = $this->generateCTE($folder->getId(), false); $qb = $this->db->getQueryBuilder(); $qb->automaticTablePrefix(false); @@ -1033,7 +1036,7 @@ private function _findSharersFor(string $userId) :array { public function getIterator(string $userId, QueryParameters $queryParams): \Generator { $rootFolder = $this->folderMapper->findRootFolder($userId); // gives us all bookmarks in this folder, recursively - [$cte, $params, $paramTypes] = $this->_generateCTE($rootFolder->getId(), $queryParams->getSoftDeletedFolders()); + [$cte, $params, $paramTypes] = $this->generateCTE($rootFolder->getId(), $queryParams->getSoftDeletedFolders()); $qb = $this->db->getQueryBuilder(); $bookmark_cols = array_map(static function ($c) { diff --git a/lib/Db/TagMapper.php b/lib/Db/TagMapper.php index f51e3d9d4..6eea5e81c 100644 --- a/lib/Db/TagMapper.php +++ b/lib/Db/TagMapper.php @@ -24,13 +24,21 @@ class TagMapper { */ protected $db; + private FolderMapper $folderMapper; + + private BookmarkMapper $bookmarkMapper; + /** * TagMapper constructor. * * @param IDBConnection $db + * @param FolderMapper $folderMapper + * @param BookmarkMapper $bookmarkMapper */ - public function __construct(IDBConnection $db) { + public function __construct(IDBConnection $db, FolderMapper $folderMapper, BookmarkMapper $bookmarkMapper) { $this->db = $db; + $this->folderMapper = $folderMapper; + $this->bookmarkMapper = $bookmarkMapper; } /** @@ -39,23 +47,30 @@ public function __construct(IDBConnection $db) { * @throws Exception */ public function findAllWithCount($userId): array { + $rootFolder = $this->folderMapper->findRootFolder($userId); + [$cte, $cteParams, $cteParamTypes] = $this->bookmarkMapper->generateCTE($rootFolder->getId(), false); + $qb = $this->db->getQueryBuilder(); + $qb->automaticTablePrefix(false); $qb - ->select('t.tag AS name') + ->selectAlias('t.tag', 'name') ->selectAlias($qb->createFunction('COUNT(DISTINCT ' . $qb->getColumnName('t.bookmark_id') . ')'), 'count') - ->from('bookmarks_tags', 't') - ->innerJoin('t', 'bookmarks', 'b', $qb->expr()->eq('b.id', 't.bookmark_id')) - ->innerJoin('b', 'bookmarks_tree', 'tr', 'b.id = tr.id AND tr.type = ' . $qb->createPositionalParameter(TreeMapper::TYPE_BOOKMARK) . ' AND tr.soft_deleted_at IS NULL') - ->leftJoin('tr', 'bookmarks_shared_folders', 'sf', $qb->expr()->eq('tr.parent_folder', 'sf.folder_id')) - ->where($qb->expr()->eq('b.user_id', $qb->createPositionalParameter($userId))) - ->orWhere($qb->expr()->andX( - $qb->expr()->eq('sf.user_id', $qb->createPositionalParameter($userId)), - $qb->expr()->eq('tr.type', $qb->createPositionalParameter(TreeMapper::TYPE_BOOKMARK))) - ) + ->from('*PREFIX*bookmarks_tags', 't') + ->innerJoin('t', 'folder_tree', 'tree', 'tree.item_id = t.bookmark_id AND tree.type = ' . $qb->createPositionalParameter(TreeMapper::TYPE_BOOKMARK) . ' AND tree.soft_deleted_at IS NULL') ->groupBy('t.tag') ->orderBy('count', 'DESC'); - return $qb->executeQuery()->fetchAll(); + $finalQuery = $cte . ' ' . $qb->getSQL(); + $params = array_merge($cteParams, $qb->getParameters()); + $paramTypes = array_merge($cteParamTypes, $qb->getParameterTypes()); + + $cursor = $this->db->executeQuery($finalQuery, $params, $paramTypes); + $rows = []; + while ($row = $cursor->fetch()) { + $rows[] = $row; + } + $cursor->closeCursor(); + return $rows; } /** @@ -64,20 +79,28 @@ public function findAllWithCount($userId): array { * @throws Exception */ public function findAll($userId): array { + $rootFolder = $this->folderMapper->findRootFolder($userId); + [$cte, $cteParams, $cteParamTypes] = $this->bookmarkMapper->generateCTE($rootFolder->getId(), false); + $qb = $this->db->getQueryBuilder(); + $qb->automaticTablePrefix(false); $qb ->select('t.tag') - ->from('bookmarks_tags', 't') - ->innerJoin('t', 'bookmarks', 'b', $qb->expr()->eq('b.id', 't.bookmark_id')) - ->leftJoin('b', 'bookmarks_tree', 'tr', 'b.id = tr.id AND tr.type = ' . $qb->createPositionalParameter(TreeMapper::TYPE_BOOKMARK)) - ->leftJoin('tr', 'bookmarks_shared_folders', 'sf', $qb->expr()->eq('tr.parent_folder', 'sf.folder_id')) - ->where($qb->expr()->eq('b.user_id', $qb->createPositionalParameter($userId))) - ->orWhere($qb->expr()->andX( - $qb->expr()->eq('sf.user_id', $qb->createPositionalParameter($userId)), - $qb->expr()->eq('tr.type', $qb->createPositionalParameter(TreeMapper::TYPE_SHARE))) - ) + ->from('*PREFIX*bookmarks_tags', 't') + ->innerJoin('t', 'folder_tree', 'tree', 'tree.item_id = t.bookmark_id AND tree.type = ' . $qb->createPositionalParameter(TreeMapper::TYPE_BOOKMARK) . ' AND tree.soft_deleted_at IS NULL') ->groupBy('t.tag'); - return $qb->executeQuery()->fetchAll(PDO::FETCH_COLUMN); + + $finalQuery = $cte . ' ' . $qb->getSQL(); + $params = array_merge($cteParams, $qb->getParameters()); + $paramTypes = array_merge($cteParamTypes, $qb->getParameterTypes()); + + $cursor = $this->db->executeQuery($finalQuery, $params, $paramTypes); + $tags = []; + while ($row = $cursor->fetch()) { + $tags[] = $row['tag']; + } + $cursor->closeCursor(); + return $tags; } /** diff --git a/tests/TagMapperTest.php b/tests/TagMapperTest.php index 86181fb41..9d9dcd453 100644 --- a/tests/TagMapperTest.php +++ b/tests/TagMapperTest.php @@ -6,10 +6,12 @@ use OCA\Bookmarks\Db\Bookmark; use OCA\Bookmarks\Exception\UrlParseError; use OCA\Bookmarks\QueryParameters; +use OCA\Bookmarks\Service\FolderService; use OCP\AppFramework\Db\DoesNotExistException; use OCP\AppFramework\Db\MultipleObjectsReturnedException; use OCP\AppFramework\QueryException; use OCP\IUserManager; +use OCP\Share\IShare; use PHPUnit\Framework\TestCase; class TagMapperTest extends TestCase { @@ -161,6 +163,94 @@ public function testSetOn(array $tags, Bookmark $bookmark) { } } + /** + * Regression test for https://github.com/nextcloud/bookmarks/issues/1982: + * tags on bookmarks placed inside a subfolder of a shared folder must be + * visible to the sharee, not just tags on bookmarks directly in the shared + * folder. + * + * @throws \OCA\Bookmarks\Exception\AlreadyExistsError + * @throws \OCA\Bookmarks\Exception\UnsupportedOperation + * @throws \OCA\Bookmarks\Exception\UserLimitExceededError + * @throws DoesNotExistException + * @throws MultipleObjectsReturnedException + * @throws UrlParseError + * @throws \OCP\DB\Exception + */ + public function testFindAllSeesTagsInSubfolderOfSharedFolder() { + $owner = 'tag_share_owner'; + $recipient = 'tag_share_recipient'; + if (!$this->userManager->userExists($owner)) { + $this->userManager->createUser($owner, 'password'); + } + if (!$this->userManager->userExists($recipient)) { + $this->userManager->createUser($recipient, 'password'); + } + $ownerId = $this->userManager->get($owner)->getUID(); + $recipientId = $this->userManager->get($recipient)->getUID(); + + /** @var FolderService $folderService */ + $folderService = \OCP\Server::get(FolderService::class); + + $sharedFolder = new Db\Folder(); + $sharedFolder->setTitle('shared-root'); + $sharedFolder->setUserId($ownerId); + $this->folderMapper->insert($sharedFolder); + $this->treeMapper->move( + Db\TreeMapper::TYPE_FOLDER, + $sharedFolder->getId(), + $this->folderMapper->findRootFolder($ownerId)->getId(), + ); + + $subFolder = new Db\Folder(); + $subFolder->setTitle('sub'); + $subFolder->setUserId($ownerId); + $this->folderMapper->insert($subFolder); + $this->treeMapper->move( + Db\TreeMapper::TYPE_FOLDER, + $subFolder->getId(), + $sharedFolder->getId(), + ); + + $bookmark = Bookmark::fromArray([ + 'userId' => $ownerId, + 'url' => 'https://example.org/issue-1982', + 'title' => 'Nested', + 'description' => '', + ]); + $bookmark = $this->bookmarkMapper->insertOrUpdate($bookmark); + $nestedTag = 'nested-shared-1982'; + $this->tagMapper->addTo([$nestedTag], $bookmark->getId()); + $this->treeMapper->addToFolders( + Db\TreeMapper::TYPE_BOOKMARK, + $bookmark->getId(), + [$subFolder->getId()], + ); + + $folderService->createShare( + $sharedFolder->getId(), + $recipient, + IShare::TYPE_USER, + true, + false, + ); + + $recipientTags = $this->tagMapper->findAll($recipientId); + $this->assertContains( + $nestedTag, + $recipientTags, + 'Tag on a bookmark inside a subfolder of a shared folder must be visible to the sharee', + ); + + $recipientTagsWithCount = $this->tagMapper->findAllWithCount($recipientId); + $matched = array_values(array_filter( + $recipientTagsWithCount, + static fn ($row) => $row['name'] === $nestedTag, + )); + $this->assertCount(1, $matched); + $this->assertEquals(1, (int)$matched[0]['count']); + } + /** * @return array */