Merge pull request #2293 from nextcloud/feature/optimize-shares-loading

blizzz · web-flow · commit 5d8cc676b585 · 2026-03-03T13:29:05.000+01:00
Perf: Optimize shares loading by eliminating N+1 queries
diff --git a/lib/Db/ShareMapper.php b/lib/Db/ShareMapper.php
@@ -84,23 +84,34 @@ public function findShareForNode(int $nodeId, string $nodeType, string $receiver
 
 	/**
 	 * @param string $nodeType
-	 * @param string $receiver
+	 * @param string[]|int[] $receivers
 	 * @param string $userId
 	 * @param string|null $receiverType
 	 *
-	 * @return array
+	 * @return Share[]
 	 *
 	 * @throws Exception
 	 */
-	public function findAllSharesFor(string $nodeType, string $receiver, string $userId, ?string $receiverType = 'user'): array {
-		$qb = $this->db->getQueryBuilder();
-		$qb->select('*')
-			->from($this->table)
-			->where($qb->expr()->eq('receiver', $qb->createNamedParameter($receiver, IQueryBuilder::PARAM_STR)))
-			->andWhere($qb->expr()->neq('sender', $qb->createNamedParameter($userId, IQueryBuilder::PARAM_STR)))
-			->andWhere($qb->expr()->eq('node_type', $qb->createNamedParameter($nodeType, IQueryBuilder::PARAM_STR)))
-			->andWhere($qb->expr()->eq('receiver_type', $qb->createNamedParameter($receiverType, IQueryBuilder::PARAM_STR)));
-		return $this->findEntities($qb);
+	public function findAllSharesFor(string $nodeType, array $receivers, string $userId, ?string $receiverType = 'user'): array {
+		if (!$receivers) {
+			return [];
+		}
+
+		$chunks = [];
+		// deduct extra parameters (sender, node type, receiver type)
+		foreach (array_chunk($receivers, 1000 - 3) as $receiversChunk) {
+			$qb = $this->db->getQueryBuilder();
+			$qb->select('*')
+				->from($this->table)
+				->where($qb->expr()->in('receiver', $qb->createNamedParameter($receiversChunk, IQueryBuilder::PARAM_STR_ARRAY)))
+				->andWhere($qb->expr()->neq('sender', $qb->createNamedParameter($userId)))
+				->andWhere($qb->expr()->eq('node_type', $qb->createNamedParameter($nodeType)))
+				->andWhere($qb->expr()->eq('receiver_type', $qb->createNamedParameter($receiverType)));
+
+			$chunks[] = $this->findEntities($qb);
+		}
+
+		return array_merge(...$chunks);
 	}
 
 	/**
diff --git a/lib/Db/TableMapper.php b/lib/Db/TableMapper.php
@@ -50,6 +50,42 @@ public function find(int $id): Table {
 		return $this->cache[$cacheKey];
 	}
 
+	/**
+	 * @param int[] $ids
+	 * @return array<int, Table> indexed by table id
+	 */
+	public function findMany(array $ids): array {
+		$missing = [];
+		$result = [];
+		foreach ($ids as $id) {
+			$cacheKey = (string)$id;
+			if (isset($this->cache[$cacheKey])) {
+				$result[$id] = $this->cache[$cacheKey];
+			} else {
+				$missing[$id] = true;
+			}
+		}
+
+		if (!$missing) {
+			return $result;
+		}
+
+		$missing = array_keys($missing);
+		foreach (array_chunk($missing, 1000) as $missingChunk) {
+			$qb = $this->db->getQueryBuilder();
+			$qb->select('*')
+				->from($this->table)
+				->where($qb->expr()->in('id', $qb->createNamedParameter($missingChunk, IQueryBuilder::PARAM_INT_ARRAY)));
+
+			foreach ($this->findEntities($qb) as $entity) {
+				$id = $entity->getId();
+				$this->cache[(string)$id] = $entity;
+				$result[$id] = $entity;
+			}
+		}
+		return $result;
+	}
+
 	/**
 	 * @param string|null $userId
 	 * @return Table[]
diff --git a/lib/Db/ViewMapper.php b/lib/Db/ViewMapper.php
@@ -50,6 +50,43 @@ public function find(int $id): View {
 		return $this->cache[$cacheKey];
 	}
 
+	/**
+	 * @param int[] $ids
+	 * @return array<int, View> indexed by view id
+	 */
+	public function findMany(array $ids): array {
+		$missing = [];
+		$result = [];
+		foreach ($ids as $id) {
+			$cacheKey = (string)$id;
+			if (isset($this->cache[$cacheKey])) {
+				$result[$id] = $this->cache[$cacheKey];
+			} else {
+				$missing[$id] = true;
+			}
+		}
+
+		if (!$missing) {
+			return $result;
+		}
+
+		$missing = array_keys($missing);
+		foreach (array_chunk($missing, 1000) as $missingChunk) {
+			$qb = $this->db->getQueryBuilder();
+			$qb->select('v.*', 't.ownership')
+				->from($this->table, 'v')
+				->innerJoin('v', 'tables_tables', 't', 't.id = v.table_id')
+				->where($qb->expr()->in('v.id', $qb->createNamedParameter($missingChunk, IQueryBuilder::PARAM_INT_ARRAY)));
+
+			foreach ($this->findEntities($qb) as $entity) {
+				$id = $entity->getId();
+				$this->cache[(string)$id] = $entity;
+				$result[$id] = $entity;
+			}
+		}
+		return $result;
+	}
+
 	public function delete(Entity $entity): View {
 		unset($this->cache[(string)$entity->getId()]);
 		return parent::delete($entity);
diff --git a/lib/Helper/CircleHelper.php b/lib/Helper/CircleHelper.php
@@ -76,7 +76,6 @@ public function getCircleDisplayName(string $circleId, string $userId): string {
 	/**
 	 * @param string $userId
 	 * @return Circle[]
-	 * @throws InternalError
 	 */
 	public function getUserCircles(string $userId): array {
 		if (!$this->circlesEnabled) {
diff --git a/lib/Service/ShareService.php b/lib/Service/ShareService.php
@@ -12,6 +12,7 @@
 use DateTime;
 
 use InvalidArgumentException;
+use OCA\Circles\Model\Circle;
 use OCA\Tables\AppInfo\Application;
 use OCA\Tables\Constants\ShareReceiverType;
 use OCA\Tables\Db\Context;
@@ -38,6 +39,7 @@
 use OCP\AppFramework\Db\TTransactional;
 use OCP\DB\Exception;
 use OCP\IDBConnection;
+use OCP\IGroup;
 use OCP\IUserManager;
 use OCP\Security\IHasher;
 use OCP\Security\ISecureRandom;
@@ -194,7 +196,7 @@ protected function generateShareToken(): ShareToken {
 
 	/**
 	 * @param string|null $userId
-	 * @return array
+	 * @return array<int, View> Indexed by view id
 	 * @throws InternalError
 	 */
 	public function findViewsSharedWithMe(?string $userId = null): array {
@@ -203,7 +205,7 @@ public function findViewsSharedWithMe(?string $userId = null): array {
 
 	/**
 	 * @param string|null $userId
-	 * @return array
+	 * @return array<int, Table> Indexed by table id
 	 * @throws InternalError
 	 */
 	public function findTablesSharedWithMe(?string $userId = null): array {
@@ -216,52 +218,38 @@ public function findTablesSharedWithMe(?string $userId = null): array {
 	private function findElementsSharedWithMe(string $elementType = 'table', ?string $userId = null): array {
 		$userId = $this->permissionsService->preCheckUserId($userId);
 
-		$returnArray = [];
-
+		$shares = [];
 		try {
-			// get all views or tables that are shared with me as user
-			$elementsSharedWithMe = $this->mapper->findAllSharesFor($elementType, $userId, $userId);
+			$shares['user'] = $this->mapper->findAllSharesFor($elementType, [$userId], $userId);
 
-			// get all views or tables that are shared with me by group
 			$userGroups = $this->userHelper->getGroupsForUser($userId);
-			foreach ($userGroups as $userGroup) {
-				$shares = $this->mapper->findAllSharesFor($elementType, $userGroup->getGid(), $userId, ShareReceiverType::GROUP);
-				$elementsSharedWithMe = array_merge($elementsSharedWithMe, $shares);
-			}
+			$userGroupIds = array_map(static fn (IGroup $group) => $group->getGid(), $userGroups);
+			$shares['groups'] = $this->mapper->findAllSharesFor($elementType, $userGroupIds, $userId, ShareReceiverType::GROUP);
 
-			// get all views or tables that are shared with me by circle
-			if ($this->circleHelper->isCirclesEnabled()) {
-				$userCircles = $this->circleHelper->getUserCircles($userId);
-
-				foreach ($userCircles as $userCircle) {
-					$shares = $this->mapper->findAllSharesFor($elementType, $userCircle->getSingleId(), $userId, ShareReceiverType::CIRCLE);
-					$elementsSharedWithMe = array_merge($elementsSharedWithMe, $shares);
-				}
-			}
+			$userCircles = $this->circleHelper->getUserCircles($userId);
+			$userCircleIds = array_map(static fn (Circle $circle) => $circle->getSingleId(), $userCircles);
+			$shares['circles'] = $this->mapper->findAllSharesFor($elementType, $userCircleIds, $userId, ShareReceiverType::CIRCLE);
 		} catch (Throwable $e) {
-			throw new InternalError($e->getMessage());
+			throw new InternalError($e->getMessage(), previous: $e);
 		}
-		foreach ($elementsSharedWithMe as $share) {
-			try {
-				if ($elementType === 'table') {
-					$element = $this->tableMapper->find($share->getNodeId());
-				} elseif ($elementType === 'view') {
-					$element = $this->viewMapper->find($share->getNodeId());
-				} else {
-					throw new InternalError('Cannot find element of type ' . $elementType);
-				}
-				// Check if en element with this id is already in the result array
-				$index = array_search($element->getId(), array_column($returnArray, 'id'), true);
-				if (!$index) {
-					$returnArray[] = $element;
-				}
-			} catch (DoesNotExistException|Exception|MultipleObjectsReturnedException $e) {
-				throw new InternalError($e->getMessage());
-			}
+
+		$elementIds = [];
+		foreach (array_merge($shares['user'], $shares['groups'], $shares['circles']) as $share) {
+			/** @var Share $share */
+			$elementIds[$share->getNodeId()] = true;
 		}
-		return $returnArray;
-	}
+		$elementIds = array_keys($elementIds);
 
+		if ($elementType === 'table') {
+			return $this->tableMapper->findMany($elementIds);
+		}
+
+		if ($elementType === 'view') {
+			return $this->viewMapper->findMany($elementIds);
+		}
+
+		throw new InternalError('Cannot find element of type ' . $elementType);
+	}
 
 	/**
 	 * @param int $elementId
diff --git a/lib/Service/ViewService.php b/lib/Service/ViewService.php
@@ -163,30 +163,24 @@ public function findSharedViewsWithMe(?string $userId = null): array {
 			return [];
 		}
 
-		$allViews = [];
-
 		$sharedViews = $this->shareService->findViewsSharedWithMe($userId);
-		foreach ($sharedViews as $sharedView) {
-			$allViews[$sharedView->getId()] = $sharedView;
-		}
-
 		$contexts = $this->contextService->findAll($userId);
 		foreach ($contexts as $context) {
 			$nodes = $context->getNodes();
 			foreach ($nodes as $node) {
 				if ($node['node_type'] !== Application::NODE_TYPE_VIEW
-					|| isset($allViews[$node['node_id']])
+					|| isset($sharedViews[$node['node_id']])
 				) {
 					continue;
 				}
-				$allViews[$node['node_id']] = $this->find($node['node_id'], false, $userId);
+				$sharedViews[$node['node_id']] = $this->find($node['node_id'], false, $userId);
 			}
 		}
 
-		foreach ($allViews as $view) {
+		foreach ($sharedViews as $view) {
 			$this->enhanceView($view, $userId);
 		}
-		return array_values($allViews);
+		return array_values($sharedViews);
 	}
 
 

Original file line number	Diff line number	Diff line change
`@@ -163,30 +163,24 @@ public function findSharedViewsWithMe(?string $userId = null): array {`
`163`	`163`	`return [];`
`164`	`164`	`}`
`165`	`165`
`166`		`- $allViews = [];`
`167`		`-`
`168`	`166`	`$sharedViews = $this->shareService->findViewsSharedWithMe($userId);`
`169`		`- foreach ($sharedViews as $sharedView) {`
`170`		`- $allViews[$sharedView->getId()] = $sharedView;`
`171`		`- }`
`172`		`-`
`173`	`167`	`$contexts = $this->contextService->findAll($userId);`
`174`	`168`	`foreach ($contexts as $context) {`
`175`	`169`	`$nodes = $context->getNodes();`
`176`	`170`	`foreach ($nodes as $node) {`
`177`	`171`	`if ($node['node_type'] !== Application::NODE_TYPE_VIEW`
`178`		`- \|\| isset($allViews[$node['node_id']])`
	`172`	`+ \|\| isset($sharedViews[$node['node_id']])`
`179`	`173`	`) {`
`180`	`174`	`continue;`
`181`	`175`	`}`
`182`		`- $allViews[$node['node_id']] = $this->find($node['node_id'], false, $userId);`
	`176`	`+ $sharedViews[$node['node_id']] = $this->find($node['node_id'], false, $userId);`
`183`	`177`	`}`
`184`	`178`	`}`
`185`	`179`
`186`		`- foreach ($allViews as $view) {`
	`180`	`+ foreach ($sharedViews as $view) {`
`187`	`181`	`$this->enhanceView($view, $userId);`
`188`	`182`	`}`
`189`		`- return array_values($allViews);`
	`183`	`+ return array_values($sharedViews);`
`190`	`184`	`}`
`191`	`185`
`192`	`186`