Skip to content

Commit c415f67

Browse files
committed
fix(BookmarkMapper): Do not count trashed bookmarks as duplicated
Assisted-by: ClaudeCode:claude-opus-4-8 Signed-off-by: Marcel Klehr <mklehr@gmx.net>
1 parent c8165d2 commit c415f67

2 files changed

Lines changed: 57 additions & 3 deletions

File tree

lib/Db/BookmarkMapper.php

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -491,7 +491,9 @@ private function _filterDuplicated(IQueryBuilder $qb, QueryParameters $params) {
491491
->from('folder_tree', 'trdup')
492492
->where($subQuery->expr()->eq('b.id', 'trdup.item_id'))
493493
->andWhere($subQuery->expr()->neq('trdup.parent_folder', 'tree.parent_folder'))
494-
->andWhere($subQuery->expr()->eq('trdup.type', $qb->createPositionalParameter(TreeMapper::TYPE_BOOKMARK)));
494+
->andWhere($subQuery->expr()->eq('trdup.type', $qb->createPositionalParameter(TreeMapper::TYPE_BOOKMARK)))
495+
// Only live copies make a bookmark a duplicate; a trashed copy in another folder doesn't count.
496+
->andWhere($subQuery->expr()->isNull('trdup.soft_deleted_at'));
495497
$qb->andWhere($qb->createFunction('EXISTS(' . $subQuery->getSQL() . ')'));
496498
}
497499
}
@@ -718,9 +720,9 @@ public function countDuplicated(string $userId): int {
718720
$qb->select('tree.item_id')
719721
->from('folder_tree', 'tree')
720722
->where($qb->expr()->eq('tree.type', $qb->createPositionalParameter(TreeMapper::TYPE_BOOKMARK)))
723+
->andWhere($qb->expr()->isNull('tree.soft_deleted_at'))
721724
->groupBy('tree.item_id')
722-
->having('COUNT(DISTINCT tree.parent_folder) > 1')
723-
->andHaving('SUM(CASE WHEN tree.soft_deleted_at IS NULL THEN 1 ELSE 0 END) > 0');
725+
->having('COUNT(DISTINCT tree.parent_folder) > 1');
724726

725727
$finalQuery = $cte . ' SELECT COUNT(*) FROM (' . $qb->getSQL() . ') dup';
726728
$params = array_merge($params, $qb->getParameters());

tests/BookmarkMapperTest.php

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -227,6 +227,58 @@ public function testCountDuplicatedInSubfolderOfSharedFolder() {
227227
$this->assertCount(1, $duplicatedList);
228228
}
229229

230+
/**
231+
* A bookmark that lives in two folders but whose second copy has been trashed is
232+
* NOT a duplicate: only live copies count. countDuplicated() and the "Duplicated"
233+
* list (findAll + setDuplicated) must both ignore the trashed copy and agree.
234+
*
235+
* @throws AlreadyExistsError
236+
* @throws UserLimitExceededError
237+
* @throws UrlParseError
238+
* @throws DoesNotExistException
239+
* @throws MultipleObjectsReturnedException
240+
*/
241+
public function testCountDuplicatedIgnoresTrashedCopies() {
242+
$rootFolderId = $this->folderMapper->findRootFolder($this->userId)->getId();
243+
244+
$folderA = new Db\Folder();
245+
$folderA->setTitle('dup-a');
246+
$folderA->setUserId($this->userId);
247+
$this->folderMapper->insert($folderA);
248+
$this->treeMapper->move(Db\TreeMapper::TYPE_FOLDER, $folderA->getId(), $rootFolderId);
249+
250+
$folderB = new Db\Folder();
251+
$folderB->setTitle('dup-b');
252+
$folderB->setUserId($this->userId);
253+
$this->folderMapper->insert($folderB);
254+
$this->treeMapper->move(Db\TreeMapper::TYPE_FOLDER, $folderB->getId(), $rootFolderId);
255+
256+
// One bookmark placed in both folders -> duplicated while both copies are live.
257+
$bookmark = Db\Bookmark::fromArray([
258+
'userId' => $this->userId,
259+
'url' => 'https://example.org/duplicated-then-trashed',
260+
'title' => 'Trashed duplicate',
261+
'description' => '',
262+
]);
263+
$bookmark = $this->bookmarkMapper->insertOrUpdate($bookmark);
264+
$this->treeMapper->addToFolders(
265+
Db\TreeMapper::TYPE_BOOKMARK,
266+
$bookmark->getId(),
267+
[$folderA->getId(), $folderB->getId()],
268+
);
269+
270+
$this->assertSame(1, $this->bookmarkMapper->countDuplicated($this->userId));
271+
272+
// Trash the copy in folder B -> only one live copy remains -> not a duplicate.
273+
$this->treeMapper->softDeleteEntry(Db\TreeMapper::TYPE_BOOKMARK, $bookmark->getId(), $folderB->getId());
274+
275+
$this->assertSame(0, $this->bookmarkMapper->countDuplicated($this->userId));
276+
277+
$params = new \OCA\Bookmarks\QueryParameters();
278+
$duplicatedList = $this->bookmarkMapper->findAll($this->userId, $params->setDuplicated(true));
279+
$this->assertCount(0, $duplicatedList);
280+
}
281+
230282
/**
231283
* Regression test: countUnavailable() must count an unavailable bookmark living
232284
* in a subfolder of a shared folder, for the sharee. The old implementation only

0 commit comments

Comments
 (0)