From c8165d2d6790e1923f4e28e60a5b0c8bdd81164c Mon Sep 17 00:00:00 2001 From: Marcel Klehr Date: Sat, 11 Jul 2026 10:26:45 +0200 Subject: [PATCH 1/2] perf(BookmarkMapper): Improve countDuplicated performance Assisted-by: ClaudeCode:claude-opus-4-8 Signed-off-by: Marcel Klehr --- lib/Db/BookmarkMapper.php | 22 ++++++++-------------- 1 file changed, 8 insertions(+), 14 deletions(-) diff --git a/lib/Db/BookmarkMapper.php b/lib/Db/BookmarkMapper.php index 1e9f6d263..5533da63b 100644 --- a/lib/Db/BookmarkMapper.php +++ b/lib/Db/BookmarkMapper.php @@ -710,25 +710,19 @@ public function countUnavailable(string $userId): int { * @throws Exception */ public function countDuplicated(string $userId): int { - // Count duplicates the exact same way the "Duplicated" list is computed in findAll(): - // against the recursive folder_tree CTE (which covers nested folders and bookmarks inside - // shared folders/subfolders) and using the same _filterDuplicated() predicate. Hand-rolled - // queries against the raw bookmarks_tree table miss everything that only becomes visible - // through the recursive expansion, which made this method under-count. $rootFolder = $this->folderMapper->findRootFolder($userId); [$cte, $params, $paramTypes] = $this->generateCTE($rootFolder->getId(), false); $qb = $this->db->getQueryBuilder(); $qb->automaticTablePrefix(false); - $qb->select($qb->createFunction('COUNT(DISTINCT b.id)')) - ->from('*PREFIX*bookmarks', 'b') - ->innerJoin('b', 'folder_tree', 'tree', 'tree.item_id = b.id AND tree.type = ' . $qb->createPositionalParameter(TreeMapper::TYPE_BOOKMARK) . ' AND tree.soft_deleted_at is NULL'); - - $queryParams = new QueryParameters(); - $queryParams->setDuplicated(true); - $this->_filterDuplicated($qb, $queryParams); - - $finalQuery = $cte . ' ' . $qb->getSQL(); + $qb->select('tree.item_id') + ->from('folder_tree', 'tree') + ->where($qb->expr()->eq('tree.type', $qb->createPositionalParameter(TreeMapper::TYPE_BOOKMARK))) + ->groupBy('tree.item_id') + ->having('COUNT(DISTINCT tree.parent_folder) > 1') + ->andHaving('SUM(CASE WHEN tree.soft_deleted_at IS NULL THEN 1 ELSE 0 END) > 0'); + + $finalQuery = $cte . ' SELECT COUNT(*) FROM (' . $qb->getSQL() . ') dup'; $params = array_merge($params, $qb->getParameters()); $paramTypes = array_merge($paramTypes, $qb->getParameterTypes()); From c415f673fb565af740a535ce85df16c156dcf491 Mon Sep 17 00:00:00 2001 From: Marcel Klehr Date: Sat, 11 Jul 2026 10:30:50 +0200 Subject: [PATCH 2/2] fix(BookmarkMapper): Do not count trashed bookmarks as duplicated Assisted-by: ClaudeCode:claude-opus-4-8 Signed-off-by: Marcel Klehr --- lib/Db/BookmarkMapper.php | 8 +++--- tests/BookmarkMapperTest.php | 52 ++++++++++++++++++++++++++++++++++++ 2 files changed, 57 insertions(+), 3 deletions(-) diff --git a/lib/Db/BookmarkMapper.php b/lib/Db/BookmarkMapper.php index 5533da63b..373a7ddeb 100644 --- a/lib/Db/BookmarkMapper.php +++ b/lib/Db/BookmarkMapper.php @@ -491,7 +491,9 @@ private function _filterDuplicated(IQueryBuilder $qb, QueryParameters $params) { ->from('folder_tree', 'trdup') ->where($subQuery->expr()->eq('b.id', 'trdup.item_id')) ->andWhere($subQuery->expr()->neq('trdup.parent_folder', 'tree.parent_folder')) - ->andWhere($subQuery->expr()->eq('trdup.type', $qb->createPositionalParameter(TreeMapper::TYPE_BOOKMARK))); + ->andWhere($subQuery->expr()->eq('trdup.type', $qb->createPositionalParameter(TreeMapper::TYPE_BOOKMARK))) + // Only live copies make a bookmark a duplicate; a trashed copy in another folder doesn't count. + ->andWhere($subQuery->expr()->isNull('trdup.soft_deleted_at')); $qb->andWhere($qb->createFunction('EXISTS(' . $subQuery->getSQL() . ')')); } } @@ -718,9 +720,9 @@ public function countDuplicated(string $userId): int { $qb->select('tree.item_id') ->from('folder_tree', 'tree') ->where($qb->expr()->eq('tree.type', $qb->createPositionalParameter(TreeMapper::TYPE_BOOKMARK))) + ->andWhere($qb->expr()->isNull('tree.soft_deleted_at')) ->groupBy('tree.item_id') - ->having('COUNT(DISTINCT tree.parent_folder) > 1') - ->andHaving('SUM(CASE WHEN tree.soft_deleted_at IS NULL THEN 1 ELSE 0 END) > 0'); + ->having('COUNT(DISTINCT tree.parent_folder) > 1'); $finalQuery = $cte . ' SELECT COUNT(*) FROM (' . $qb->getSQL() . ') dup'; $params = array_merge($params, $qb->getParameters()); diff --git a/tests/BookmarkMapperTest.php b/tests/BookmarkMapperTest.php index a3ef20484..1861cbf72 100644 --- a/tests/BookmarkMapperTest.php +++ b/tests/BookmarkMapperTest.php @@ -227,6 +227,58 @@ public function testCountDuplicatedInSubfolderOfSharedFolder() { $this->assertCount(1, $duplicatedList); } + /** + * A bookmark that lives in two folders but whose second copy has been trashed is + * NOT a duplicate: only live copies count. countDuplicated() and the "Duplicated" + * list (findAll + setDuplicated) must both ignore the trashed copy and agree. + * + * @throws AlreadyExistsError + * @throws UserLimitExceededError + * @throws UrlParseError + * @throws DoesNotExistException + * @throws MultipleObjectsReturnedException + */ + public function testCountDuplicatedIgnoresTrashedCopies() { + $rootFolderId = $this->folderMapper->findRootFolder($this->userId)->getId(); + + $folderA = new Db\Folder(); + $folderA->setTitle('dup-a'); + $folderA->setUserId($this->userId); + $this->folderMapper->insert($folderA); + $this->treeMapper->move(Db\TreeMapper::TYPE_FOLDER, $folderA->getId(), $rootFolderId); + + $folderB = new Db\Folder(); + $folderB->setTitle('dup-b'); + $folderB->setUserId($this->userId); + $this->folderMapper->insert($folderB); + $this->treeMapper->move(Db\TreeMapper::TYPE_FOLDER, $folderB->getId(), $rootFolderId); + + // One bookmark placed in both folders -> duplicated while both copies are live. + $bookmark = Db\Bookmark::fromArray([ + 'userId' => $this->userId, + 'url' => 'https://example.org/duplicated-then-trashed', + 'title' => 'Trashed duplicate', + 'description' => '', + ]); + $bookmark = $this->bookmarkMapper->insertOrUpdate($bookmark); + $this->treeMapper->addToFolders( + Db\TreeMapper::TYPE_BOOKMARK, + $bookmark->getId(), + [$folderA->getId(), $folderB->getId()], + ); + + $this->assertSame(1, $this->bookmarkMapper->countDuplicated($this->userId)); + + // Trash the copy in folder B -> only one live copy remains -> not a duplicate. + $this->treeMapper->softDeleteEntry(Db\TreeMapper::TYPE_BOOKMARK, $bookmark->getId(), $folderB->getId()); + + $this->assertSame(0, $this->bookmarkMapper->countDuplicated($this->userId)); + + $params = new \OCA\Bookmarks\QueryParameters(); + $duplicatedList = $this->bookmarkMapper->findAll($this->userId, $params->setDuplicated(true)); + $this->assertCount(0, $duplicatedList); + } + /** * Regression test: countUnavailable() must count an unavailable bookmark living * in a subfolder of a shared folder, for the sharee. The old implementation only