Skip to content

Commit 53fa2b2

Browse files
marcelklehrmarcelklehr
committed
fix(TagMapper): Surface tags from bookmarks in deep folders within shared folders
fixes #1982 Assisted-by: ClaudeCode:claude-opus-4.7 Signed-off-by: Marcel Klehr <mklehr@gmx.net>
1 parent 503f40b commit 53fa2b2

3 files changed

Lines changed: 143 additions & 27 deletions

File tree

‎lib/Db/BookmarkMapper.php‎

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -213,7 +213,7 @@ public function find(int $id): Bookmark {
213213
public function findAll(string $userId, QueryParameters $queryParams, bool $withGroupBy = true): array {
214214
$rootFolder = $this->folderMapper->findRootFolder($userId);
215215
// gives us all bookmarks in this folder, recursively
216-
[$cte, $params, $paramTypes] = $this->_generateCTE($rootFolder->getId(), $queryParams->getSoftDeletedFolders());
216+
[$cte, $params, $paramTypes] = $this->generateCTE($rootFolder->getId(), $queryParams->getSoftDeletedFolders());
217217

218218
$qb = $this->db->getQueryBuilder();
219219
$bookmark_cols = array_map(static function ($c) {
@@ -282,11 +282,14 @@ protected function getIteratorWithRawQuery(string $query, array $params, array $
282282
}
283283

284284
/**
285-
* Common table expression that lists all items in a given folder, recursively
285+
* Common table expression that lists all items in a given folder, recursively.
286+
* Public so other mappers (e.g. TagMapper) can reuse the same tree-traversal
287+
* logic and stay consistent with how visibility is computed for bookmarks.
288+
*
286289
* @param int $folderId
287290
* @return array
288291
*/
289-
private function _generateCTE(int $folderId, bool $withSoftDeleted) : array {
292+
public function generateCTE(int $folderId, bool $withSoftDeleted) : array {
290293
// The base case of the recursion is just the folder we're given
291294
$baseCase = $this->db->getQueryBuilder();
292295
$baseCase
@@ -761,7 +764,7 @@ public function findAllInPublicFolder(string $token, QueryParameters $queryParam
761764
$folder = $this->folderMapper->find($publicFolder->getFolderId());
762765

763766
// gives us all bookmarks in this folder, recursively
764-
[$cte, $params, $paramTypes] = $this->_generateCTE($folder->getId(), false);
767+
[$cte, $params, $paramTypes] = $this->generateCTE($folder->getId(), false);
765768

766769
$qb = $this->db->getQueryBuilder();
767770
$qb->automaticTablePrefix(false);
@@ -1033,7 +1036,7 @@ private function _findSharersFor(string $userId) :array {
10331036
public function getIterator(string $userId, QueryParameters $queryParams): \Generator {
10341037
$rootFolder = $this->folderMapper->findRootFolder($userId);
10351038
// gives us all bookmarks in this folder, recursively
1036-
[$cte, $params, $paramTypes] = $this->_generateCTE($rootFolder->getId(), $queryParams->getSoftDeletedFolders());
1039+
[$cte, $params, $paramTypes] = $this->generateCTE($rootFolder->getId(), $queryParams->getSoftDeletedFolders());
10371040

10381041
$qb = $this->db->getQueryBuilder();
10391042
$bookmark_cols = array_map(static function ($c) {

‎lib/Db/TagMapper.php‎

Lines changed: 45 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -24,13 +24,21 @@ class TagMapper {
2424
*/
2525
protected $db;
2626

27+
private FolderMapper $folderMapper;
28+
29+
private BookmarkMapper $bookmarkMapper;
30+
2731
/**
2832
* TagMapper constructor.
2933
*
3034
* @param IDBConnection $db
35+
* @param FolderMapper $folderMapper
36+
* @param BookmarkMapper $bookmarkMapper
3137
*/
32-
public function __construct(IDBConnection $db) {
38+
public function __construct(IDBConnection $db, FolderMapper $folderMapper, BookmarkMapper $bookmarkMapper) {
3339
$this->db = $db;
40+
$this->folderMapper = $folderMapper;
41+
$this->bookmarkMapper = $bookmarkMapper;
3442
}
3543

3644
/**
@@ -39,23 +47,30 @@ public function __construct(IDBConnection $db) {
3947
* @throws Exception
4048
*/
4149
public function findAllWithCount($userId): array {
50+
$rootFolder = $this->folderMapper->findRootFolder($userId);
51+
[$cte, $cteParams, $cteParamTypes] = $this->bookmarkMapper->generateCTE($rootFolder->getId(), false);
52+
4253
$qb = $this->db->getQueryBuilder();
54+
$qb->automaticTablePrefix(false);
4355
$qb
44-
->select('t.tag AS name')
56+
->selectAlias('t.tag', 'name')
4557
->selectAlias($qb->createFunction('COUNT(DISTINCT ' . $qb->getColumnName('t.bookmark_id') . ')'), 'count')
46-
->from('bookmarks_tags', 't')
47-
->innerJoin('t', 'bookmarks', 'b', $qb->expr()->eq('b.id', 't.bookmark_id'))
48-
->innerJoin('b', 'bookmarks_tree', 'tr', 'b.id = tr.id AND tr.type = ' . $qb->createPositionalParameter(TreeMapper::TYPE_BOOKMARK) . ' AND tr.soft_deleted_at IS NULL')
49-
->leftJoin('tr', 'bookmarks_shared_folders', 'sf', $qb->expr()->eq('tr.parent_folder', 'sf.folder_id'))
50-
->where($qb->expr()->eq('b.user_id', $qb->createPositionalParameter($userId)))
51-
->orWhere($qb->expr()->andX(
52-
$qb->expr()->eq('sf.user_id', $qb->createPositionalParameter($userId)),
53-
$qb->expr()->eq('tr.type', $qb->createPositionalParameter(TreeMapper::TYPE_BOOKMARK)))
54-
)
58+
->from('*PREFIX*bookmarks_tags', 't')
59+
->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')
5560
->groupBy('t.tag')
5661
->orderBy('count', 'DESC');
5762

58-
return $qb->executeQuery()->fetchAll();
63+
$finalQuery = $cte . ' ' . $qb->getSQL();
64+
$params = array_merge($cteParams, $qb->getParameters());
65+
$paramTypes = array_merge($cteParamTypes, $qb->getParameterTypes());
66+
67+
$cursor = $this->db->executeQuery($finalQuery, $params, $paramTypes);
68+
$rows = [];
69+
while ($row = $cursor->fetch()) {
70+
$rows[] = $row;
71+
}
72+
$cursor->closeCursor();
73+
return $rows;
5974
}
6075

6176
/**
@@ -64,20 +79,28 @@ public function findAllWithCount($userId): array {
6479
* @throws Exception
6580
*/
6681
public function findAll($userId): array {
82+
$rootFolder = $this->folderMapper->findRootFolder($userId);
83+
[$cte, $cteParams, $cteParamTypes] = $this->bookmarkMapper->generateCTE($rootFolder->getId(), false);
84+
6785
$qb = $this->db->getQueryBuilder();
86+
$qb->automaticTablePrefix(false);
6887
$qb
6988
->select('t.tag')
70-
->from('bookmarks_tags', 't')
71-
->innerJoin('t', 'bookmarks', 'b', $qb->expr()->eq('b.id', 't.bookmark_id'))
72-
->leftJoin('b', 'bookmarks_tree', 'tr', 'b.id = tr.id AND tr.type = ' . $qb->createPositionalParameter(TreeMapper::TYPE_BOOKMARK))
73-
->leftJoin('tr', 'bookmarks_shared_folders', 'sf', $qb->expr()->eq('tr.parent_folder', 'sf.folder_id'))
74-
->where($qb->expr()->eq('b.user_id', $qb->createPositionalParameter($userId)))
75-
->orWhere($qb->expr()->andX(
76-
$qb->expr()->eq('sf.user_id', $qb->createPositionalParameter($userId)),
77-
$qb->expr()->eq('tr.type', $qb->createPositionalParameter(TreeMapper::TYPE_SHARE)))
78-
)
89+
->from('*PREFIX*bookmarks_tags', 't')
90+
->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')
7991
->groupBy('t.tag');
80-
return $qb->executeQuery()->fetchAll(PDO::FETCH_COLUMN);
92+
93+
$finalQuery = $cte . ' ' . $qb->getSQL();
94+
$params = array_merge($cteParams, $qb->getParameters());
95+
$paramTypes = array_merge($cteParamTypes, $qb->getParameterTypes());
96+
97+
$cursor = $this->db->executeQuery($finalQuery, $params, $paramTypes);
98+
$tags = [];
99+
while ($row = $cursor->fetch()) {
100+
$tags[] = $row['tag'];
101+
}
102+
$cursor->closeCursor();
103+
return $tags;
81104
}
82105

83106
/**

‎tests/TagMapperTest.php‎

Lines changed: 90 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,10 +6,12 @@
66
use OCA\Bookmarks\Db\Bookmark;
77
use OCA\Bookmarks\Exception\UrlParseError;
88
use OCA\Bookmarks\QueryParameters;
9+
use OCA\Bookmarks\Service\FolderService;
910
use OCP\AppFramework\Db\DoesNotExistException;
1011
use OCP\AppFramework\Db\MultipleObjectsReturnedException;
1112
use OCP\AppFramework\QueryException;
1213
use OCP\IUserManager;
14+
use OCP\Share\IShare;
1315
use PHPUnit\Framework\TestCase;
1416

1517
class TagMapperTest extends TestCase {
@@ -161,6 +163,94 @@ public function testSetOn(array $tags, Bookmark $bookmark) {
161163
}
162164
}
163165

166+
/**
167+
* Regression test for https://github.com/nextcloud/bookmarks/issues/1982:
168+
* tags on bookmarks placed inside a subfolder of a shared folder must be
169+
* visible to the sharee, not just tags on bookmarks directly in the shared
170+
* folder.
171+
*
172+
* @throws \OCA\Bookmarks\Exception\AlreadyExistsError
173+
* @throws \OCA\Bookmarks\Exception\UnsupportedOperation
174+
* @throws \OCA\Bookmarks\Exception\UserLimitExceededError
175+
* @throws DoesNotExistException
176+
* @throws MultipleObjectsReturnedException
177+
* @throws UrlParseError
178+
* @throws \OCP\DB\Exception
179+
*/
180+
public function testFindAllSeesTagsInSubfolderOfSharedFolder() {
181+
$owner = 'tag_share_owner';
182+
$recipient = 'tag_share_recipient';
183+
if (!$this->userManager->userExists($owner)) {
184+
$this->userManager->createUser($owner, 'password');
185+
}
186+
if (!$this->userManager->userExists($recipient)) {
187+
$this->userManager->createUser($recipient, 'password');
188+
}
189+
$ownerId = $this->userManager->get($owner)->getUID();
190+
$recipientId = $this->userManager->get($recipient)->getUID();
191+
192+
/** @var FolderService $folderService */
193+
$folderService = \OCP\Server::get(FolderService::class);
194+
195+
$sharedFolder = new Db\Folder();
196+
$sharedFolder->setTitle('shared-root');
197+
$sharedFolder->setUserId($ownerId);
198+
$this->folderMapper->insert($sharedFolder);
199+
$this->treeMapper->move(
200+
Db\TreeMapper::TYPE_FOLDER,
201+
$sharedFolder->getId(),
202+
$this->folderMapper->findRootFolder($ownerId)->getId(),
203+
);
204+
205+
$subFolder = new Db\Folder();
206+
$subFolder->setTitle('sub');
207+
$subFolder->setUserId($ownerId);
208+
$this->folderMapper->insert($subFolder);
209+
$this->treeMapper->move(
210+
Db\TreeMapper::TYPE_FOLDER,
211+
$subFolder->getId(),
212+
$sharedFolder->getId(),
213+
);
214+
215+
$bookmark = Bookmark::fromArray([
216+
'userId' => $ownerId,
217+
'url' => 'https://example.org/issue-1982',
218+
'title' => 'Nested',
219+
'description' => '',
220+
]);
221+
$bookmark = $this->bookmarkMapper->insertOrUpdate($bookmark);
222+
$nestedTag = 'nested-shared-1982';
223+
$this->tagMapper->addTo([$nestedTag], $bookmark->getId());
224+
$this->treeMapper->addToFolders(
225+
Db\TreeMapper::TYPE_BOOKMARK,
226+
$bookmark->getId(),
227+
[$subFolder->getId()],
228+
);
229+
230+
$folderService->createShare(
231+
$sharedFolder->getId(),
232+
$recipient,
233+
IShare::TYPE_USER,
234+
true,
235+
false,
236+
);
237+
238+
$recipientTags = $this->tagMapper->findAll($recipientId);
239+
$this->assertContains(
240+
$nestedTag,
241+
$recipientTags,
242+
'Tag on a bookmark inside a subfolder of a shared folder must be visible to the sharee',
243+
);
244+
245+
$recipientTagsWithCount = $this->tagMapper->findAllWithCount($recipientId);
246+
$matched = array_values(array_filter(
247+
$recipientTagsWithCount,
248+
static fn ($row) => $row['name'] === $nestedTag,
249+
));
250+
$this->assertCount(1, $matched);
251+
$this->assertEquals(1, (int)$matched[0]['count']);
252+
}
253+
164254
/**
165255
* @return array
166256
*/

0 commit comments

Comments
 (0)