Skip to content

Commit 41bb0e5

Browse files
marcelklehrmarcelklehr
authored
Merge pull request #2462 from nextcloud/fix/counts
Fix nested share coutning
2 parents 369dc7b + 9a0e7df commit 41bb0e5

2 files changed

Lines changed: 296 additions & 94 deletions

File tree

‎lib/Db/BookmarkMapper.php‎

Lines changed: 85 additions & 94 deletions
Original file line numberDiff line numberDiff line change
@@ -612,30 +612,30 @@ public function countArchived(string $userId): int {
612612
* @throws Exception
613613
*/
614614
public function countAllClicks(string $userId): int {
615-
$qb = $this->db->getQueryBuilder();
616-
$qb->selectAlias($qb->func()->sum('b.clickcount'), 'count');
617-
$qb
618-
->from('bookmarks', 'b')
619-
->innerJoin('b', 'bookmarks_tree', 'tr', 'b.id = tr.id AND tr.type = ' . $qb->createPositionalParameter(TreeMapper::TYPE_BOOKMARK) . ' AND tr.soft_deleted_at is NULL')
620-
->where($qb->expr()->eq('b.user_id', $qb->createPositionalParameter($userId)))
621-
->andWhere($qb->expr()->neq('b.clickcount', $qb->createPositionalParameter(0, IQueryBuilder::PARAM_INT)));
622-
$result = $qb->executeQuery();
623-
$userOwnerClickCount = $result->fetch(PDO::FETCH_COLUMN);
624-
$result->closeCursor();
615+
// Sum clicks across the recursive folder_tree CTE so that bookmarks nested in subfolders of
616+
// shared folders are included. Because the CTE yields one row per (bookmark, reachable
617+
// parent_folder), we first deduplicate to one row per bookmark and then sum, so a bookmark
618+
// reachable through several folders doesn't have its clicks counted more than once.
619+
// Hand-rolled queries against the raw bookmarks_tree table missed the nested case entirely.
620+
$rootFolder = $this->folderMapper->findRootFolder($userId);
621+
[$cte, $params, $paramTypes] = $this->generateCTE($rootFolder->getId(), false);
625622

626623
$qb = $this->db->getQueryBuilder();
627-
$qb->selectAlias($qb->func()->sum('b.clickcount'), 'count');
628-
$qb
629-
->from('bookmarks', 'b')
630-
->innerJoin('b', 'bookmarks_tree', 'tr', 'b.id = tr.id AND tr.type = ' . $qb->createPositionalParameter(TreeMapper::TYPE_BOOKMARK) . ' AND tr.soft_deleted_at is NULL')
631-
->innerJoin('tr', 'bookmarks_shared_folders', 'sf', $qb->expr()->eq('tr.parent_folder', 'sf.folder_id'))
632-
->where($qb->expr()->eq('sf.user_id', $qb->createPositionalParameter($userId)))
633-
->andWhere($qb->expr()->neq('b.clickcount', $qb->createPositionalParameter(0, IQueryBuilder::PARAM_INT)));
634-
$result = $qb->executeQuery();
635-
$foreignClickCount = $result->fetch(PDO::FETCH_COLUMN);
624+
$qb->automaticTablePrefix(false);
625+
$qb->selectDistinct(['b.id', 'b.clickcount'])
626+
->from('*PREFIX*bookmarks', 'b')
627+
->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')
628+
->where($qb->expr()->neq('b.clickcount', $qb->createPositionalParameter(0, IQueryBuilder::PARAM_INT)));
629+
630+
$finalQuery = $cte . ' SELECT COALESCE(SUM(sub.clickcount), 0) FROM (' . $qb->getSQL() . ') sub';
631+
$params = array_merge($params, $qb->getParameters());
632+
$paramTypes = array_merge($paramTypes, $qb->getParameterTypes());
633+
634+
$result = $this->db->executeQuery($finalQuery, $params, $paramTypes);
635+
$count = (int)$result->fetchOne();
636636
$result->closeCursor();
637637

638-
return $userOwnerClickCount + $foreignClickCount;
638+
return $count;
639639
}
640640

641641
/**
@@ -644,30 +644,29 @@ public function countAllClicks(string $userId): int {
644644
* @throws Exception
645645
*/
646646
public function countWithClicks(string $userId): int {
647-
$qb = $this->db->getQueryBuilder();
648-
$qb->selectAlias($qb->func()->count('b.id'), 'count');
649-
$qb
650-
->from('bookmarks', 'b')
651-
->innerJoin('b', 'bookmarks_tree', 'tr', 'b.id = tr.id AND tr.type = ' . $qb->createPositionalParameter(TreeMapper::TYPE_BOOKMARK) . ' AND tr.soft_deleted_at is NULL')
652-
->where($qb->expr()->eq('b.user_id', $qb->createPositionalParameter($userId)))
653-
->andWhere($qb->expr()->neq('b.clickcount', $qb->createPositionalParameter(0, IQueryBuilder::PARAM_INT)));
654-
$result = $qb->executeQuery();
655-
$userOwnerWithClicksCount = $result->fetch(PDO::FETCH_COLUMN);
656-
$result->closeCursor();
647+
// Count clicked bookmarks against the recursive folder_tree CTE so that bookmarks nested in
648+
// subfolders of shared folders are included, and count each bookmark once (COUNT DISTINCT).
649+
// Hand-rolled queries against the raw bookmarks_tree table miss everything that only becomes
650+
// visible through the recursive expansion, which made this method under-count.
651+
$rootFolder = $this->folderMapper->findRootFolder($userId);
652+
[$cte, $params, $paramTypes] = $this->generateCTE($rootFolder->getId(), false);
657653

658654
$qb = $this->db->getQueryBuilder();
659-
$qb->selectAlias($qb->func()->count('b.id'), 'count');
660-
$qb
661-
->from('bookmarks', 'b')
662-
->innerJoin('b', 'bookmarks_tree', 'tr', 'b.id = tr.id AND tr.type = ' . $qb->createPositionalParameter(TreeMapper::TYPE_BOOKMARK) . ' AND tr.soft_deleted_at is NULL')
663-
->innerJoin('tr', 'bookmarks_shared_folders', 'sf', $qb->expr()->eq('tr.parent_folder', 'sf.folder_id'))
664-
->where($qb->expr()->eq('sf.user_id', $qb->createPositionalParameter($userId)))
665-
->andWhere($qb->expr()->neq('b.clickcount', $qb->createPositionalParameter(0, IQueryBuilder::PARAM_INT)));
666-
$result = $qb->executeQuery();
667-
$foreignWithClicksCount = $result->fetch(PDO::FETCH_COLUMN);
655+
$qb->automaticTablePrefix(false);
656+
$qb->select($qb->createFunction('COUNT(DISTINCT b.id)'))
657+
->from('*PREFIX*bookmarks', 'b')
658+
->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')
659+
->where($qb->expr()->neq('b.clickcount', $qb->createPositionalParameter(0, IQueryBuilder::PARAM_INT)));
660+
661+
$finalQuery = $cte . ' ' . $qb->getSQL();
662+
$params = array_merge($params, $qb->getParameters());
663+
$paramTypes = array_merge($paramTypes, $qb->getParameterTypes());
664+
665+
$result = $this->db->executeQuery($finalQuery, $params, $paramTypes);
666+
$count = (int)$result->fetchOne();
668667
$result->closeCursor();
669668

670-
return $userOwnerWithClicksCount + $foreignWithClicksCount;
669+
return $count;
671670
}
672671

673672
/**
@@ -676,30 +675,33 @@ public function countWithClicks(string $userId): int {
676675
* @throws Exception
677676
*/
678677
public function countUnavailable(string $userId): int {
679-
$qb = $this->db->getQueryBuilder();
680-
$qb->selectAlias($qb->func()->count('b.id'), 'count');
681-
$qb
682-
->from('bookmarks', 'b')
683-
->innerJoin('b', 'bookmarks_tree', 'tr', 'b.id = tr.id AND tr.type = ' . $qb->createPositionalParameter(TreeMapper::TYPE_BOOKMARK) . ' AND tr.soft_deleted_at is NULL')
684-
->where($qb->expr()->eq('b.user_id', $qb->createPositionalParameter($userId)))
685-
->andWhere($qb->expr()->eq('b.available', $qb->createPositionalParameter(false, IQueryBuilder::PARAM_BOOL)));
686-
$result = $qb->executeQuery();
687-
$userOwnerUnavailableCount = $result->fetch(PDO::FETCH_COLUMN);
688-
$result->closeCursor();
678+
// Count unavailable bookmarks the exact same way the "Unavailable" list is computed in
679+
// findAll(): against the recursive folder_tree CTE (which covers nested folders and
680+
// bookmarks inside shared folders/subfolders) and using the same _filterUnavailable()
681+
// predicate. Hand-rolled queries against the raw bookmarks_tree table miss everything that
682+
// only becomes visible through the recursive expansion, which made this method under-count.
683+
$rootFolder = $this->folderMapper->findRootFolder($userId);
684+
[$cte, $params, $paramTypes] = $this->generateCTE($rootFolder->getId(), false);
689685

690686
$qb = $this->db->getQueryBuilder();
691-
$qb->selectAlias($qb->func()->count('b.id'), 'count');
692-
$qb
693-
->from('bookmarks', 'b')
694-
->innerJoin('b', 'bookmarks_tree', 'tr', 'b.id = tr.id AND tr.type = ' . $qb->createPositionalParameter(TreeMapper::TYPE_BOOKMARK) . ' AND tr.soft_deleted_at is NULL')
695-
->innerJoin('tr', 'bookmarks_shared_folders', 'sf', $qb->expr()->eq('tr.parent_folder', 'sf.folder_id'))
696-
->where($qb->expr()->eq('sf.user_id', $qb->createPositionalParameter($userId)))
697-
->andWhere($qb->expr()->eq('b.available', $qb->createPositionalParameter(false, IQueryBuilder::PARAM_BOOL)));
698-
$result = $qb->executeQuery();
699-
$foreignUnavailableCount = $result->fetch(PDO::FETCH_COLUMN);
687+
$qb->automaticTablePrefix(false);
688+
$qb->select($qb->createFunction('COUNT(DISTINCT b.id)'))
689+
->from('*PREFIX*bookmarks', 'b')
690+
->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');
691+
692+
$queryParams = new QueryParameters();
693+
$queryParams->setUnavailable(true);
694+
$this->_filterUnavailable($qb, $queryParams);
695+
696+
$finalQuery = $cte . ' ' . $qb->getSQL();
697+
$params = array_merge($params, $qb->getParameters());
698+
$paramTypes = array_merge($paramTypes, $qb->getParameterTypes());
699+
700+
$result = $this->db->executeQuery($finalQuery, $params, $paramTypes);
701+
$count = (int)$result->fetchOne();
700702
$result->closeCursor();
701703

702-
return $userOwnerUnavailableCount + $foreignUnavailableCount;
704+
return $count;
703705
}
704706

705707
/**
@@ -708,44 +710,33 @@ public function countUnavailable(string $userId): int {
708710
* @throws Exception
709711
*/
710712
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();
713+
// Count duplicates the exact same way the "Duplicated" list is computed in findAll():
714+
// against the recursive folder_tree CTE (which covers nested folders and bookmarks inside
715+
// shared folders/subfolders) and using the same _filterDuplicated() predicate. Hand-rolled
716+
// queries against the raw bookmarks_tree table miss everything that only becomes visible
717+
// through the recursive expansion, which made this method under-count.
718+
$rootFolder = $this->folderMapper->findRootFolder($userId);
719+
[$cte, $params, $paramTypes] = $this->generateCTE($rootFolder->getId(), false);
728720

729721
$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);
722+
$qb->automaticTablePrefix(false);
723+
$qb->select($qb->createFunction('COUNT(DISTINCT b.id)'))
724+
->from('*PREFIX*bookmarks', 'b')
725+
->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');
726+
727+
$queryParams = new QueryParameters();
728+
$queryParams->setDuplicated(true);
729+
$this->_filterDuplicated($qb, $queryParams);
730+
731+
$finalQuery = $cte . ' ' . $qb->getSQL();
732+
$params = array_merge($params, $qb->getParameters());
733+
$paramTypes = array_merge($paramTypes, $qb->getParameterTypes());
734+
735+
$result = $this->db->executeQuery($finalQuery, $params, $paramTypes);
736+
$count = (int)$result->fetchOne();
746737
$result->closeCursor();
747738

748-
return $userOwnerDuplicatesCount + $foreignDuplicatesCount;
739+
return $count;
749740
}
750741

751742
/**

0 commit comments

Comments
 (0)