diff --git a/lib/Db/BookmarkMapper.php b/lib/Db/BookmarkMapper.php index 1e9f6d263..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() . ')')); } } @@ -710,25 +712,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))) + ->andWhere($qb->expr()->isNull('tree.soft_deleted_at')) + ->groupBy('tree.item_id') + ->having('COUNT(DISTINCT tree.parent_folder) > 1'); + + $finalQuery = $cte . ' SELECT COUNT(*) FROM (' . $qb->getSQL() . ') dup'; $params = array_merge($params, $qb->getParameters()); $paramTypes = array_merge($paramTypes, $qb->getParameterTypes()); 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