Merge pull request #2468 from nextcloud/fix/long-page-load

perf(WebViewController): Improve countDuplicated performance

Author: marcelklehr

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());
 

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