Skip to content

Commit 638d062

Browse files
marcelklehrmarcelklehr
committed
fix(BookmarkMapper): Fix countDuplicated to use the recursive CTE
Signed-off-by: Marcel Klehr <mklehr@gmx.net>
1 parent 369dc7b commit 638d062

2 files changed

Lines changed: 112 additions & 34 deletions

File tree

‎lib/Db/BookmarkMapper.php‎

Lines changed: 23 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -708,44 +708,33 @@ public function countUnavailable(string $userId): int {
708708
* @throws Exception
709709
*/
710710
public function countDuplicated(string $userId): int {
711-
$qb = $this->db->getQueryBuilder();
712-
$qb->selectDistinct($qb->func()->count('b.id'));
713-
$qb
714-
->from('bookmarks', 'b')
715-
->innerJoin('b', 'bookmarks_tree', 'tr', 'b.id = tr.id AND tr.type = ' . $qb->createPositionalParameter(TreeMapper::TYPE_BOOKMARK) . ' AND tr.soft_deleted_at is NULL')
716-
->where($qb->expr()->eq('b.user_id', $qb->createPositionalParameter($userId)));
717-
$subQuery = $this->db->getQueryBuilder();
718-
$subQuery->select('trdup.parent_folder')
719-
->from('bookmarks_tree', 'trdup')
720-
->where($subQuery->expr()->eq('b.id', 'trdup.id'))
721-
->andWhere($subQuery->expr()->neq('trdup.parent_folder', 'tr.parent_folder'))
722-
->andWhere($subQuery->expr()->eq('trdup.type', $qb->createPositionalParameter(TreeMapper::TYPE_BOOKMARK)))
723-
->andWhere($subQuery->expr()->isNull('trdup.soft_deleted_at'));
724-
$qb->andWhere($qb->createFunction('EXISTS(' . $subQuery->getSQL() . ')'));
725-
$result = $qb->executeQuery();
726-
$userOwnerDuplicatesCount = $result->fetch(PDO::FETCH_COLUMN);
727-
$result->closeCursor();
711+
// Count duplicates the exact same way the "Duplicated" list is computed in findAll():
712+
// against the recursive folder_tree CTE (which covers nested folders and bookmarks inside
713+
// shared folders/subfolders) and using the same _filterDuplicated() predicate. Hand-rolled
714+
// queries against the raw bookmarks_tree table miss everything that only becomes visible
715+
// through the recursive expansion, which made this method under-count.
716+
$rootFolder = $this->folderMapper->findRootFolder($userId);
717+
[$cte, $params, $paramTypes] = $this->generateCTE($rootFolder->getId(), false);
728718

729719
$qb = $this->db->getQueryBuilder();
730-
$qb->selectDistinct($qb->func()->count('b.id'));
731-
$qb
732-
->from('bookmarks', 'b')
733-
->innerJoin('b', 'bookmarks_tree', 'tr', 'b.id = tr.id AND tr.type = ' . $qb->createPositionalParameter(TreeMapper::TYPE_BOOKMARK) . ' AND tr.soft_deleted_at is NULL')
734-
->innerJoin('tr', 'bookmarks_shared_folders', 'sf', $qb->expr()->eq('tr.parent_folder', 'sf.folder_id'))
735-
->where($qb->expr()->eq('sf.user_id', $qb->createPositionalParameter($userId)));
736-
$subQuery = $this->db->getQueryBuilder();
737-
$subQuery->select('trdup.parent_folder')
738-
->from('bookmarks_tree', 'trdup')
739-
->where($subQuery->expr()->eq('b.id', 'trdup.id'))
740-
->andWhere($subQuery->expr()->neq('trdup.parent_folder', 'tr.parent_folder'))
741-
->andWhere($subQuery->expr()->eq('trdup.type', $qb->createPositionalParameter(TreeMapper::TYPE_BOOKMARK)))
742-
->andWhere($subQuery->expr()->isNull('trdup.soft_deleted_at'));
743-
$qb->andWhere($qb->createFunction('EXISTS(' . $subQuery->getSQL() . ')'));
744-
$result = $qb->executeQuery();
745-
$foreignDuplicatesCount = $result->fetch(PDO::FETCH_COLUMN);
720+
$qb->automaticTablePrefix(false);
721+
$qb->select($qb->createFunction('COUNT(DISTINCT b.id)'))
722+
->from('*PREFIX*bookmarks', 'b')
723+
->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');
724+
725+
$queryParams = new QueryParameters();
726+
$queryParams->setDuplicated(true);
727+
$this->_filterDuplicated($qb, $queryParams);
728+
729+
$finalQuery = $cte . ' ' . $qb->getSQL();
730+
$params = array_merge($params, $qb->getParameters());
731+
$paramTypes = array_merge($paramTypes, $qb->getParameterTypes());
732+
733+
$result = $this->db->executeQuery($finalQuery, $params, $paramTypes);
734+
$count = (int)$result->fetchOne();
746735
$result->closeCursor();
747736

748-
return $userOwnerDuplicatesCount + $foreignDuplicatesCount;
737+
return $count;
749738
}
750739

751740
/**

‎tests/BookmarkMapperTest.php‎

Lines changed: 89 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,10 +6,12 @@
66
use OCA\Bookmarks\Exception\AlreadyExistsError;
77
use OCA\Bookmarks\Exception\UrlParseError;
88
use OCA\Bookmarks\Exception\UserLimitExceededError;
9+
use OCA\Bookmarks\Service\FolderService;
910
use OCP\AppFramework\Db\DoesNotExistException;
1011
use OCP\AppFramework\Db\Entity;
1112
use OCP\AppFramework\Db\MultipleObjectsReturnedException;
1213
use OCP\IUserManager;
14+
use OCP\Share\IShare;
1315

1416
class BookmarkMapperTest extends TestCase {
1517
/**
@@ -124,6 +126,93 @@ public function testDelete(Entity $bookmark) {
124126
$this->bookmarkMapper->find($foundEntity->getId());
125127
}
126128

129+
/**
130+
* Regression test: countDuplicated() must count a bookmark that is duplicated
131+
* across two subfolders of a *shared* folder, for the sharee. The previous
132+
* implementation queried the raw bookmarks_tree table (owner's bookmarks +
133+
* bookmarks directly inside a shared folder) and therefore under-counted: it
134+
* never saw duplicates living in subfolders of shared folders. The count must
135+
* match what the "Duplicated" list (findAll + setDuplicated) actually shows.
136+
*
137+
* @throws \OCA\Bookmarks\Exception\AlreadyExistsError
138+
* @throws \OCA\Bookmarks\Exception\UserLimitExceededError
139+
* @throws UrlParseError
140+
* @throws MultipleObjectsReturnedException
141+
*/
142+
public function testCountDuplicatedInSubfolderOfSharedFolder() {
143+
$owner = 'dup_share_owner';
144+
$recipient = 'dup_share_recipient';
145+
if (!$this->userManager->userExists($owner)) {
146+
$this->userManager->createUser($owner, 'password');
147+
}
148+
if (!$this->userManager->userExists($recipient)) {
149+
$this->userManager->createUser($recipient, 'password');
150+
}
151+
$ownerId = $this->userManager->get($owner)->getUID();
152+
$recipientId = $this->userManager->get($recipient)->getUID();
153+
154+
/** @var FolderService $folderService */
155+
$folderService = \OCP\Server::get(FolderService::class);
156+
157+
// Owner creates a folder that will be shared...
158+
$sharedFolder = new Db\Folder();
159+
$sharedFolder->setTitle('shared-root');
160+
$sharedFolder->setUserId($ownerId);
161+
$this->folderMapper->insert($sharedFolder);
162+
$this->treeMapper->move(
163+
Db\TreeMapper::TYPE_FOLDER,
164+
$sharedFolder->getId(),
165+
$this->folderMapper->findRootFolder($ownerId)->getId(),
166+
);
167+
168+
// ...with two subfolders inside it.
169+
$subFolderA = new Db\Folder();
170+
$subFolderA->setTitle('sub-a');
171+
$subFolderA->setUserId($ownerId);
172+
$this->folderMapper->insert($subFolderA);
173+
$this->treeMapper->move(Db\TreeMapper::TYPE_FOLDER, $subFolderA->getId(), $sharedFolder->getId());
174+
175+
$subFolderB = new Db\Folder();
176+
$subFolderB->setTitle('sub-b');
177+
$subFolderB->setUserId($ownerId);
178+
$this->folderMapper->insert($subFolderB);
179+
$this->treeMapper->move(Db\TreeMapper::TYPE_FOLDER, $subFolderB->getId(), $sharedFolder->getId());
180+
181+
// A single bookmark placed in BOTH subfolders -> it is duplicated.
182+
$bookmark = Db\Bookmark::fromArray([
183+
'userId' => $ownerId,
184+
'url' => 'https://example.org/duplicated-in-shared-subfolders',
185+
'title' => 'Nested duplicate',
186+
'description' => '',
187+
]);
188+
$bookmark = $this->bookmarkMapper->insertOrUpdate($bookmark);
189+
$this->treeMapper->addToFolders(
190+
Db\TreeMapper::TYPE_BOOKMARK,
191+
$bookmark->getId(),
192+
[$subFolderA->getId(), $subFolderB->getId()],
193+
);
194+
195+
$folderService->createShare(
196+
$sharedFolder->getId(),
197+
$recipient,
198+
IShare::TYPE_USER,
199+
true,
200+
false,
201+
);
202+
203+
// The owner reaches both subfolders directly -> sees 1 duplicate.
204+
$this->assertSame(1, $this->bookmarkMapper->countDuplicated($ownerId));
205+
206+
// The sharee reaches both subfolders through the share -> must also see 1
207+
// duplicate. This is the case the old implementation missed (returned 0).
208+
$this->assertSame(1, $this->bookmarkMapper->countDuplicated($recipientId));
209+
210+
// And the count must agree with the actual "Duplicated" list.
211+
$params = new \OCA\Bookmarks\QueryParameters();
212+
$duplicatedList = $this->bookmarkMapper->findAll($recipientId, $params->setDuplicated(true));
213+
$this->assertCount(1, $duplicatedList);
214+
}
215+
127216
/**
128217
* @return array
129218
*/

0 commit comments

Comments
 (0)