From 638d062d010eb56a2bd7a852c491d6ac0da5b708 Mon Sep 17 00:00:00 2001 From: Marcel Klehr Date: Sat, 20 Jun 2026 08:50:27 +0200 Subject: [PATCH 1/4] fix(BookmarkMapper): Fix countDuplicated to use the recursive CTE Signed-off-by: Marcel Klehr --- lib/Db/BookmarkMapper.php | 57 ++++++++++------------- tests/BookmarkMapperTest.php | 89 ++++++++++++++++++++++++++++++++++++ 2 files changed, 112 insertions(+), 34 deletions(-) diff --git a/lib/Db/BookmarkMapper.php b/lib/Db/BookmarkMapper.php index c928ea521..769e5aa50 100644 --- a/lib/Db/BookmarkMapper.php +++ b/lib/Db/BookmarkMapper.php @@ -708,44 +708,33 @@ public function countUnavailable(string $userId): int { * @throws Exception */ public function countDuplicated(string $userId): int { - $qb = $this->db->getQueryBuilder(); - $qb->selectDistinct($qb->func()->count('b.id')); - $qb - ->from('bookmarks', 'b') - ->innerJoin('b', 'bookmarks_tree', 'tr', 'b.id = tr.id AND tr.type = ' . $qb->createPositionalParameter(TreeMapper::TYPE_BOOKMARK) . ' AND tr.soft_deleted_at is NULL') - ->where($qb->expr()->eq('b.user_id', $qb->createPositionalParameter($userId))); - $subQuery = $this->db->getQueryBuilder(); - $subQuery->select('trdup.parent_folder') - ->from('bookmarks_tree', 'trdup') - ->where($subQuery->expr()->eq('b.id', 'trdup.id')) - ->andWhere($subQuery->expr()->neq('trdup.parent_folder', 'tr.parent_folder')) - ->andWhere($subQuery->expr()->eq('trdup.type', $qb->createPositionalParameter(TreeMapper::TYPE_BOOKMARK))) - ->andWhere($subQuery->expr()->isNull('trdup.soft_deleted_at')); - $qb->andWhere($qb->createFunction('EXISTS(' . $subQuery->getSQL() . ')')); - $result = $qb->executeQuery(); - $userOwnerDuplicatesCount = $result->fetch(PDO::FETCH_COLUMN); - $result->closeCursor(); + // 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->selectDistinct($qb->func()->count('b.id')); - $qb - ->from('bookmarks', 'b') - ->innerJoin('b', 'bookmarks_tree', 'tr', 'b.id = tr.id AND tr.type = ' . $qb->createPositionalParameter(TreeMapper::TYPE_BOOKMARK) . ' AND tr.soft_deleted_at is NULL') - ->innerJoin('tr', 'bookmarks_shared_folders', 'sf', $qb->expr()->eq('tr.parent_folder', 'sf.folder_id')) - ->where($qb->expr()->eq('sf.user_id', $qb->createPositionalParameter($userId))); - $subQuery = $this->db->getQueryBuilder(); - $subQuery->select('trdup.parent_folder') - ->from('bookmarks_tree', 'trdup') - ->where($subQuery->expr()->eq('b.id', 'trdup.id')) - ->andWhere($subQuery->expr()->neq('trdup.parent_folder', 'tr.parent_folder')) - ->andWhere($subQuery->expr()->eq('trdup.type', $qb->createPositionalParameter(TreeMapper::TYPE_BOOKMARK))) - ->andWhere($subQuery->expr()->isNull('trdup.soft_deleted_at')); - $qb->andWhere($qb->createFunction('EXISTS(' . $subQuery->getSQL() . ')')); - $result = $qb->executeQuery(); - $foreignDuplicatesCount = $result->fetch(PDO::FETCH_COLUMN); + $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(); + $params = array_merge($params, $qb->getParameters()); + $paramTypes = array_merge($paramTypes, $qb->getParameterTypes()); + + $result = $this->db->executeQuery($finalQuery, $params, $paramTypes); + $count = (int)$result->fetchOne(); $result->closeCursor(); - return $userOwnerDuplicatesCount + $foreignDuplicatesCount; + return $count; } /** diff --git a/tests/BookmarkMapperTest.php b/tests/BookmarkMapperTest.php index c00b3dbe8..b11b3a2a2 100644 --- a/tests/BookmarkMapperTest.php +++ b/tests/BookmarkMapperTest.php @@ -6,10 +6,12 @@ use OCA\Bookmarks\Exception\AlreadyExistsError; use OCA\Bookmarks\Exception\UrlParseError; use OCA\Bookmarks\Exception\UserLimitExceededError; +use OCA\Bookmarks\Service\FolderService; use OCP\AppFramework\Db\DoesNotExistException; use OCP\AppFramework\Db\Entity; use OCP\AppFramework\Db\MultipleObjectsReturnedException; use OCP\IUserManager; +use OCP\Share\IShare; class BookmarkMapperTest extends TestCase { /** @@ -124,6 +126,93 @@ public function testDelete(Entity $bookmark) { $this->bookmarkMapper->find($foundEntity->getId()); } + /** + * Regression test: countDuplicated() must count a bookmark that is duplicated + * across two subfolders of a *shared* folder, for the sharee. The previous + * implementation queried the raw bookmarks_tree table (owner's bookmarks + + * bookmarks directly inside a shared folder) and therefore under-counted: it + * never saw duplicates living in subfolders of shared folders. The count must + * match what the "Duplicated" list (findAll + setDuplicated) actually shows. + * + * @throws \OCA\Bookmarks\Exception\AlreadyExistsError + * @throws \OCA\Bookmarks\Exception\UserLimitExceededError + * @throws UrlParseError + * @throws MultipleObjectsReturnedException + */ + public function testCountDuplicatedInSubfolderOfSharedFolder() { + $owner = 'dup_share_owner'; + $recipient = 'dup_share_recipient'; + if (!$this->userManager->userExists($owner)) { + $this->userManager->createUser($owner, 'password'); + } + if (!$this->userManager->userExists($recipient)) { + $this->userManager->createUser($recipient, 'password'); + } + $ownerId = $this->userManager->get($owner)->getUID(); + $recipientId = $this->userManager->get($recipient)->getUID(); + + /** @var FolderService $folderService */ + $folderService = \OCP\Server::get(FolderService::class); + + // Owner creates a folder that will be shared... + $sharedFolder = new Db\Folder(); + $sharedFolder->setTitle('shared-root'); + $sharedFolder->setUserId($ownerId); + $this->folderMapper->insert($sharedFolder); + $this->treeMapper->move( + Db\TreeMapper::TYPE_FOLDER, + $sharedFolder->getId(), + $this->folderMapper->findRootFolder($ownerId)->getId(), + ); + + // ...with two subfolders inside it. + $subFolderA = new Db\Folder(); + $subFolderA->setTitle('sub-a'); + $subFolderA->setUserId($ownerId); + $this->folderMapper->insert($subFolderA); + $this->treeMapper->move(Db\TreeMapper::TYPE_FOLDER, $subFolderA->getId(), $sharedFolder->getId()); + + $subFolderB = new Db\Folder(); + $subFolderB->setTitle('sub-b'); + $subFolderB->setUserId($ownerId); + $this->folderMapper->insert($subFolderB); + $this->treeMapper->move(Db\TreeMapper::TYPE_FOLDER, $subFolderB->getId(), $sharedFolder->getId()); + + // A single bookmark placed in BOTH subfolders -> it is duplicated. + $bookmark = Db\Bookmark::fromArray([ + 'userId' => $ownerId, + 'url' => 'https://example.org/duplicated-in-shared-subfolders', + 'title' => 'Nested duplicate', + 'description' => '', + ]); + $bookmark = $this->bookmarkMapper->insertOrUpdate($bookmark); + $this->treeMapper->addToFolders( + Db\TreeMapper::TYPE_BOOKMARK, + $bookmark->getId(), + [$subFolderA->getId(), $subFolderB->getId()], + ); + + $folderService->createShare( + $sharedFolder->getId(), + $recipient, + IShare::TYPE_USER, + true, + false, + ); + + // The owner reaches both subfolders directly -> sees 1 duplicate. + $this->assertSame(1, $this->bookmarkMapper->countDuplicated($ownerId)); + + // The sharee reaches both subfolders through the share -> must also see 1 + // duplicate. This is the case the old implementation missed (returned 0). + $this->assertSame(1, $this->bookmarkMapper->countDuplicated($recipientId)); + + // And the count must agree with the actual "Duplicated" list. + $params = new \OCA\Bookmarks\QueryParameters(); + $duplicatedList = $this->bookmarkMapper->findAll($recipientId, $params->setDuplicated(true)); + $this->assertCount(1, $duplicatedList); + } + /** * @return array */ From 4db81d8cc478c2c2d8bcb3477cfe3a3c9cd2a68a Mon Sep 17 00:00:00 2001 From: Marcel Klehr Date: Sat, 20 Jun 2026 08:59:11 +0200 Subject: [PATCH 2/4] fix(BookmarkMapper): Fix countUnavailable to use recursive CTE Signed-off-by: Marcel Klehr --- lib/Db/BookmarkMapper.php | 43 +++++++++++++++++++++------------------ 1 file changed, 23 insertions(+), 20 deletions(-) diff --git a/lib/Db/BookmarkMapper.php b/lib/Db/BookmarkMapper.php index 769e5aa50..aea6b84ac 100644 --- a/lib/Db/BookmarkMapper.php +++ b/lib/Db/BookmarkMapper.php @@ -676,30 +676,33 @@ public function countWithClicks(string $userId): int { * @throws Exception */ public function countUnavailable(string $userId): int { - $qb = $this->db->getQueryBuilder(); - $qb->selectAlias($qb->func()->count('b.id'), 'count'); - $qb - ->from('bookmarks', 'b') - ->innerJoin('b', 'bookmarks_tree', 'tr', 'b.id = tr.id AND tr.type = ' . $qb->createPositionalParameter(TreeMapper::TYPE_BOOKMARK) . ' AND tr.soft_deleted_at is NULL') - ->where($qb->expr()->eq('b.user_id', $qb->createPositionalParameter($userId))) - ->andWhere($qb->expr()->eq('b.available', $qb->createPositionalParameter(false, IQueryBuilder::PARAM_BOOL))); - $result = $qb->executeQuery(); - $userOwnerUnavailableCount = $result->fetch(PDO::FETCH_COLUMN); - $result->closeCursor(); + // Count unavailable bookmarks the exact same way the "Unavailable" 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 _filterUnavailable() + // 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->selectAlias($qb->func()->count('b.id'), 'count'); - $qb - ->from('bookmarks', 'b') - ->innerJoin('b', 'bookmarks_tree', 'tr', 'b.id = tr.id AND tr.type = ' . $qb->createPositionalParameter(TreeMapper::TYPE_BOOKMARK) . ' AND tr.soft_deleted_at is NULL') - ->innerJoin('tr', 'bookmarks_shared_folders', 'sf', $qb->expr()->eq('tr.parent_folder', 'sf.folder_id')) - ->where($qb->expr()->eq('sf.user_id', $qb->createPositionalParameter($userId))) - ->andWhere($qb->expr()->eq('b.available', $qb->createPositionalParameter(false, IQueryBuilder::PARAM_BOOL))); - $result = $qb->executeQuery(); - $foreignUnavailableCount = $result->fetch(PDO::FETCH_COLUMN); + $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->setUnavailable(true); + $this->_filterUnavailable($qb, $queryParams); + + $finalQuery = $cte . ' ' . $qb->getSQL(); + $params = array_merge($params, $qb->getParameters()); + $paramTypes = array_merge($paramTypes, $qb->getParameterTypes()); + + $result = $this->db->executeQuery($finalQuery, $params, $paramTypes); + $count = (int)$result->fetchOne(); $result->closeCursor(); - return $userOwnerUnavailableCount + $foreignUnavailableCount; + return $count; } /** From 0f8e0cd695f47c3752dcf87390958f27c39c665f Mon Sep 17 00:00:00 2001 From: Marcel Klehr Date: Sat, 20 Jun 2026 09:01:28 +0200 Subject: [PATCH 3/4] fix(BookmarkMapper): Fix click count methods to use recursive CTE Signed-off-by: Marcel Klehr --- lib/Db/BookmarkMapper.php | 79 +++++++++++++++++++-------------------- 1 file changed, 39 insertions(+), 40 deletions(-) diff --git a/lib/Db/BookmarkMapper.php b/lib/Db/BookmarkMapper.php index aea6b84ac..1e9f6d263 100644 --- a/lib/Db/BookmarkMapper.php +++ b/lib/Db/BookmarkMapper.php @@ -612,30 +612,30 @@ public function countArchived(string $userId): int { * @throws Exception */ public function countAllClicks(string $userId): int { - $qb = $this->db->getQueryBuilder(); - $qb->selectAlias($qb->func()->sum('b.clickcount'), 'count'); - $qb - ->from('bookmarks', 'b') - ->innerJoin('b', 'bookmarks_tree', 'tr', 'b.id = tr.id AND tr.type = ' . $qb->createPositionalParameter(TreeMapper::TYPE_BOOKMARK) . ' AND tr.soft_deleted_at is NULL') - ->where($qb->expr()->eq('b.user_id', $qb->createPositionalParameter($userId))) - ->andWhere($qb->expr()->neq('b.clickcount', $qb->createPositionalParameter(0, IQueryBuilder::PARAM_INT))); - $result = $qb->executeQuery(); - $userOwnerClickCount = $result->fetch(PDO::FETCH_COLUMN); - $result->closeCursor(); + // Sum clicks across the recursive folder_tree CTE so that bookmarks nested in subfolders of + // shared folders are included. Because the CTE yields one row per (bookmark, reachable + // parent_folder), we first deduplicate to one row per bookmark and then sum, so a bookmark + // reachable through several folders doesn't have its clicks counted more than once. + // Hand-rolled queries against the raw bookmarks_tree table missed the nested case entirely. + $rootFolder = $this->folderMapper->findRootFolder($userId); + [$cte, $params, $paramTypes] = $this->generateCTE($rootFolder->getId(), false); $qb = $this->db->getQueryBuilder(); - $qb->selectAlias($qb->func()->sum('b.clickcount'), 'count'); - $qb - ->from('bookmarks', 'b') - ->innerJoin('b', 'bookmarks_tree', 'tr', 'b.id = tr.id AND tr.type = ' . $qb->createPositionalParameter(TreeMapper::TYPE_BOOKMARK) . ' AND tr.soft_deleted_at is NULL') - ->innerJoin('tr', 'bookmarks_shared_folders', 'sf', $qb->expr()->eq('tr.parent_folder', 'sf.folder_id')) - ->where($qb->expr()->eq('sf.user_id', $qb->createPositionalParameter($userId))) - ->andWhere($qb->expr()->neq('b.clickcount', $qb->createPositionalParameter(0, IQueryBuilder::PARAM_INT))); - $result = $qb->executeQuery(); - $foreignClickCount = $result->fetch(PDO::FETCH_COLUMN); + $qb->automaticTablePrefix(false); + $qb->selectDistinct(['b.id', 'b.clickcount']) + ->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') + ->where($qb->expr()->neq('b.clickcount', $qb->createPositionalParameter(0, IQueryBuilder::PARAM_INT))); + + $finalQuery = $cte . ' SELECT COALESCE(SUM(sub.clickcount), 0) FROM (' . $qb->getSQL() . ') sub'; + $params = array_merge($params, $qb->getParameters()); + $paramTypes = array_merge($paramTypes, $qb->getParameterTypes()); + + $result = $this->db->executeQuery($finalQuery, $params, $paramTypes); + $count = (int)$result->fetchOne(); $result->closeCursor(); - return $userOwnerClickCount + $foreignClickCount; + return $count; } /** @@ -644,30 +644,29 @@ public function countAllClicks(string $userId): int { * @throws Exception */ public function countWithClicks(string $userId): int { - $qb = $this->db->getQueryBuilder(); - $qb->selectAlias($qb->func()->count('b.id'), 'count'); - $qb - ->from('bookmarks', 'b') - ->innerJoin('b', 'bookmarks_tree', 'tr', 'b.id = tr.id AND tr.type = ' . $qb->createPositionalParameter(TreeMapper::TYPE_BOOKMARK) . ' AND tr.soft_deleted_at is NULL') - ->where($qb->expr()->eq('b.user_id', $qb->createPositionalParameter($userId))) - ->andWhere($qb->expr()->neq('b.clickcount', $qb->createPositionalParameter(0, IQueryBuilder::PARAM_INT))); - $result = $qb->executeQuery(); - $userOwnerWithClicksCount = $result->fetch(PDO::FETCH_COLUMN); - $result->closeCursor(); + // Count clicked bookmarks against the recursive folder_tree CTE so that bookmarks nested in + // subfolders of shared folders are included, and count each bookmark once (COUNT DISTINCT). + // 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->selectAlias($qb->func()->count('b.id'), 'count'); - $qb - ->from('bookmarks', 'b') - ->innerJoin('b', 'bookmarks_tree', 'tr', 'b.id = tr.id AND tr.type = ' . $qb->createPositionalParameter(TreeMapper::TYPE_BOOKMARK) . ' AND tr.soft_deleted_at is NULL') - ->innerJoin('tr', 'bookmarks_shared_folders', 'sf', $qb->expr()->eq('tr.parent_folder', 'sf.folder_id')) - ->where($qb->expr()->eq('sf.user_id', $qb->createPositionalParameter($userId))) - ->andWhere($qb->expr()->neq('b.clickcount', $qb->createPositionalParameter(0, IQueryBuilder::PARAM_INT))); - $result = $qb->executeQuery(); - $foreignWithClicksCount = $result->fetch(PDO::FETCH_COLUMN); + $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') + ->where($qb->expr()->neq('b.clickcount', $qb->createPositionalParameter(0, IQueryBuilder::PARAM_INT))); + + $finalQuery = $cte . ' ' . $qb->getSQL(); + $params = array_merge($params, $qb->getParameters()); + $paramTypes = array_merge($paramTypes, $qb->getParameterTypes()); + + $result = $this->db->executeQuery($finalQuery, $params, $paramTypes); + $count = (int)$result->fetchOne(); $result->closeCursor(); - return $userOwnerWithClicksCount + $foreignWithClicksCount; + return $count; } /** From 9a0e7df0d47ed741d38d9571e5ecefc7c044cb54 Mon Sep 17 00:00:00 2001 From: Marcel Klehr Date: Sat, 20 Jun 2026 09:12:17 +0200 Subject: [PATCH 4/4] tests: Add regression tests for nested share counting Signed-off-by: Marcel Klehr --- tests/BookmarkMapperTest.php | 188 +++++++++++++++++++++++++++++------ 1 file changed, 155 insertions(+), 33 deletions(-) diff --git a/tests/BookmarkMapperTest.php b/tests/BookmarkMapperTest.php index b11b3a2a2..a3ef20484 100644 --- a/tests/BookmarkMapperTest.php +++ b/tests/BookmarkMapperTest.php @@ -127,21 +127,18 @@ public function testDelete(Entity $bookmark) { } /** - * Regression test: countDuplicated() must count a bookmark that is duplicated - * across two subfolders of a *shared* folder, for the sharee. The previous - * implementation queried the raw bookmarks_tree table (owner's bookmarks + - * bookmarks directly inside a shared folder) and therefore under-counted: it - * never saw duplicates living in subfolders of shared folders. The count must - * match what the "Duplicated" list (findAll + setDuplicated) actually shows. + * Creates an owner and a recipient user, gives the owner a folder (mounted under + * their root) that contains one subfolder, and shares that folder with the + * recipient. The recipient therefore only reaches the subfolder *through* the + * share — exactly the case the count*() methods used to miss because they queried + * the raw bookmarks_tree table instead of the recursive folder_tree CTE. * - * @throws \OCA\Bookmarks\Exception\AlreadyExistsError - * @throws \OCA\Bookmarks\Exception\UserLimitExceededError - * @throws UrlParseError - * @throws MultipleObjectsReturnedException + * @param string $suffix unique suffix so each test gets its own users/folders + * @return array{0: string, 1: string, 2: int, 3: int} [ownerId, recipientId, sharedFolderId, subFolderId] */ - public function testCountDuplicatedInSubfolderOfSharedFolder() { - $owner = 'dup_share_owner'; - $recipient = 'dup_share_recipient'; + private function createSharedFolderWithSubfolder(string $suffix): array { + $owner = 'count_share_owner_' . $suffix; + $recipient = 'count_share_recipient_' . $suffix; if (!$this->userManager->userExists($owner)) { $this->userManager->createUser($owner, 'password'); } @@ -151,10 +148,6 @@ public function testCountDuplicatedInSubfolderOfSharedFolder() { $ownerId = $this->userManager->get($owner)->getUID(); $recipientId = $this->userManager->get($recipient)->getUID(); - /** @var FolderService $folderService */ - $folderService = \OCP\Server::get(FolderService::class); - - // Owner creates a folder that will be shared... $sharedFolder = new Db\Folder(); $sharedFolder->setTitle('shared-root'); $sharedFolder->setUserId($ownerId); @@ -165,18 +158,47 @@ public function testCountDuplicatedInSubfolderOfSharedFolder() { $this->folderMapper->findRootFolder($ownerId)->getId(), ); - // ...with two subfolders inside it. - $subFolderA = new Db\Folder(); - $subFolderA->setTitle('sub-a'); - $subFolderA->setUserId($ownerId); - $this->folderMapper->insert($subFolderA); - $this->treeMapper->move(Db\TreeMapper::TYPE_FOLDER, $subFolderA->getId(), $sharedFolder->getId()); + $subFolder = new Db\Folder(); + $subFolder->setTitle('sub'); + $subFolder->setUserId($ownerId); + $this->folderMapper->insert($subFolder); + $this->treeMapper->move(Db\TreeMapper::TYPE_FOLDER, $subFolder->getId(), $sharedFolder->getId()); + + /** @var FolderService $folderService */ + $folderService = \OCP\Server::get(FolderService::class); + $folderService->createShare( + $sharedFolder->getId(), + $recipient, + IShare::TYPE_USER, + true, + false, + ); + + return [$ownerId, $recipientId, $sharedFolder->getId(), $subFolder->getId()]; + } + + /** + * Regression test: countDuplicated() must count a bookmark that is duplicated + * across two subfolders of a *shared* folder, for the sharee. The previous + * implementation queried the raw bookmarks_tree table (owner's bookmarks + + * bookmarks directly inside a shared folder) and therefore under-counted: it + * never saw duplicates living in subfolders of shared folders. The count must + * match what the "Duplicated" list (findAll + setDuplicated) actually shows. + * + * @throws \OCA\Bookmarks\Exception\AlreadyExistsError + * @throws \OCA\Bookmarks\Exception\UserLimitExceededError + * @throws UrlParseError + * @throws MultipleObjectsReturnedException + */ + public function testCountDuplicatedInSubfolderOfSharedFolder() { + [$ownerId, $recipientId, $sharedFolderId, $subFolderA] = $this->createSharedFolderWithSubfolder('dup'); + // A second subfolder, so the bookmark can live in two distinct places. $subFolderB = new Db\Folder(); $subFolderB->setTitle('sub-b'); $subFolderB->setUserId($ownerId); $this->folderMapper->insert($subFolderB); - $this->treeMapper->move(Db\TreeMapper::TYPE_FOLDER, $subFolderB->getId(), $sharedFolder->getId()); + $this->treeMapper->move(Db\TreeMapper::TYPE_FOLDER, $subFolderB->getId(), $sharedFolderId); // A single bookmark placed in BOTH subfolders -> it is duplicated. $bookmark = Db\Bookmark::fromArray([ @@ -189,15 +211,7 @@ public function testCountDuplicatedInSubfolderOfSharedFolder() { $this->treeMapper->addToFolders( Db\TreeMapper::TYPE_BOOKMARK, $bookmark->getId(), - [$subFolderA->getId(), $subFolderB->getId()], - ); - - $folderService->createShare( - $sharedFolder->getId(), - $recipient, - IShare::TYPE_USER, - true, - false, + [$subFolderA, $subFolderB->getId()], ); // The owner reaches both subfolders directly -> sees 1 duplicate. @@ -213,6 +227,114 @@ public function testCountDuplicatedInSubfolderOfSharedFolder() { $this->assertCount(1, $duplicatedList); } + /** + * Regression test: countUnavailable() must count an unavailable bookmark living + * in a subfolder of a shared folder, for the sharee. The old implementation only + * looked at bookmarks owned by the user or *directly* in a shared folder, so it + * missed the nested case. The count must match the "Unavailable" list. + * + * @throws \OCA\Bookmarks\Exception\AlreadyExistsError + * @throws \OCA\Bookmarks\Exception\UserLimitExceededError + * @throws UrlParseError + * @throws MultipleObjectsReturnedException + */ + public function testCountUnavailableInSubfolderOfSharedFolder() { + [$ownerId, $recipientId, , $subFolderId] = $this->createSharedFolderWithSubfolder('unavail'); + + $bookmark = Db\Bookmark::fromArray([ + 'userId' => $ownerId, + 'url' => 'https://example.org/unavailable-in-shared-subfolder', + 'title' => 'Nested unavailable', + 'description' => '', + ]); + $bookmark = $this->bookmarkMapper->insertOrUpdate($bookmark); + $bookmark->setAvailable(false); + $this->bookmarkMapper->update($bookmark); + $this->treeMapper->addToFolders(Db\TreeMapper::TYPE_BOOKMARK, $bookmark->getId(), [$subFolderId]); + + // Owner reaches the subfolder directly; sharee reaches it through the share. + // The old implementation missed the nested case for the sharee (returned 0). + $this->assertSame(1, $this->bookmarkMapper->countUnavailable($ownerId)); + $this->assertSame(1, $this->bookmarkMapper->countUnavailable($recipientId)); + + // And the count must agree with the actual "Unavailable" list. + $params = new \OCA\Bookmarks\QueryParameters(); + $unavailableList = $this->bookmarkMapper->findAll($recipientId, $params->setUnavailable(true)); + $this->assertCount(1, $unavailableList); + } + + /** + * Regression test: countWithClicks() must count a clicked bookmark living in a + * subfolder of a shared folder, for the sharee. The old implementation missed the + * nested case (returned 0 for the sharee). + * + * @throws \OCA\Bookmarks\Exception\AlreadyExistsError + * @throws \OCA\Bookmarks\Exception\UserLimitExceededError + * @throws UrlParseError + * @throws MultipleObjectsReturnedException + */ + public function testCountWithClicksInSubfolderOfSharedFolder() { + [$ownerId, $recipientId, , $subFolderId] = $this->createSharedFolderWithSubfolder('withclicks'); + + $bookmark = Db\Bookmark::fromArray([ + 'userId' => $ownerId, + 'url' => 'https://example.org/clicked-in-shared-subfolder', + 'title' => 'Nested clicked', + 'description' => '', + ]); + $bookmark = $this->bookmarkMapper->insertOrUpdate($bookmark); + $bookmark->setClickcount(3); + $this->bookmarkMapper->update($bookmark); + $this->treeMapper->addToFolders(Db\TreeMapper::TYPE_BOOKMARK, $bookmark->getId(), [$subFolderId]); + + // Owner reaches the subfolder directly; sharee reaches it through the share. + $this->assertSame(1, $this->bookmarkMapper->countWithClicks($ownerId)); + $this->assertSame(1, $this->bookmarkMapper->countWithClicks($recipientId)); + } + + /** + * Regression test: countAllClicks() must (a) include clicks of a bookmark living + * in a subfolder of a shared folder for the sharee, and (b) count each bookmark's + * clicks only once even when it is reachable through several folders. The old + * implementation summed per tree row (over-counting duplicates) and missed the + * nested case for the sharee entirely. + * + * @throws \OCA\Bookmarks\Exception\AlreadyExistsError + * @throws \OCA\Bookmarks\Exception\UserLimitExceededError + * @throws UrlParseError + * @throws MultipleObjectsReturnedException + */ + public function testCountAllClicksInSubfolderOfSharedFolderCountsEachBookmarkOnce() { + [$ownerId, $recipientId, $sharedFolderId, $subFolderA] = $this->createSharedFolderWithSubfolder('allclicks'); + + // A second subfolder so the same bookmark is reachable through two nested folders. + $subFolderB = new Db\Folder(); + $subFolderB->setTitle('sub-b'); + $subFolderB->setUserId($ownerId); + $this->folderMapper->insert($subFolderB); + $this->treeMapper->move(Db\TreeMapper::TYPE_FOLDER, $subFolderB->getId(), $sharedFolderId); + + $bookmark = Db\Bookmark::fromArray([ + 'userId' => $ownerId, + 'url' => 'https://example.org/clicks-in-shared-subfolders', + 'title' => 'Nested clicks', + 'description' => '', + ]); + $bookmark = $this->bookmarkMapper->insertOrUpdate($bookmark); + $bookmark->setClickcount(7); + $this->bookmarkMapper->update($bookmark); + $this->treeMapper->addToFolders( + Db\TreeMapper::TYPE_BOOKMARK, + $bookmark->getId(), + [$subFolderA, $subFolderB->getId()], + ); + + // Reachable through two nested folders, but its 7 clicks must be summed once. + // (Owner: old code summed 14; sharee: old code returned 0.) + $this->assertSame(7, $this->bookmarkMapper->countAllClicks($ownerId)); + $this->assertSame(7, $this->bookmarkMapper->countAllClicks($recipientId)); + } + /** * @return array */