From 0ecb0d56d66d3d080cc25d81bc47ca0b55700472 Mon Sep 17 00:00:00 2001 From: Kostiantyn Miakshyn Date: Sun, 1 Feb 2026 00:36:22 +0100 Subject: [PATCH 1/2] Perf: Optimize shares loading by eliminating N+1 queries Signed-off-by: Kostiantyn Miakshyn --- lib/Db/ShareMapper.php | 8 ++-- lib/Db/TableMapper.php | 34 +++++++++++++++++ lib/Db/ViewMapper.php | 35 ++++++++++++++++++ lib/Helper/CircleHelper.php | 1 + lib/Service/ShareService.php | 71 +++++++++++++++--------------------- lib/Service/ViewService.php | 14 ++----- 6 files changed, 107 insertions(+), 56 deletions(-) diff --git a/lib/Db/ShareMapper.php b/lib/Db/ShareMapper.php index 959bcd96bf..9c45f40244 100644 --- a/lib/Db/ShareMapper.php +++ b/lib/Db/ShareMapper.php @@ -91,19 +91,19 @@ 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 { + public function findAllSharesFor(string $nodeType, array $receivers, 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))) + ->where($qb->expr()->in('receiver', $qb->createNamedParameter($receivers, IQueryBuilder::PARAM_STR_ARRAY))) ->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))); diff --git a/lib/Db/TableMapper.php b/lib/Db/TableMapper.php index 9c094bcd8b..b10ab0246b 100644 --- a/lib/Db/TableMapper.php +++ b/lib/Db/TableMapper.php @@ -50,6 +50,40 @@ public function find(int $id): Table { return $this->cache[$cacheKey]; } + /** + * @param int[] $ids + * @return array 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; + } + + $qb = $this->db->getQueryBuilder(); + $qb->select('*') + ->from($this->table) + ->where($qb->expr()->in('id', $qb->createNamedParameter(array_keys($missing), IQueryBuilder::PARAM_INT_ARRAY))); + + $entities = $this->findEntities($qb); + foreach ($entities 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 index e6d279d7ed..740bad4a2d 100644 --- a/lib/Db/ViewMapper.php +++ b/lib/Db/ViewMapper.php @@ -50,6 +50,41 @@ public function find(int $id): View { return $this->cache[$cacheKey]; } + /** + * @param int[] $ids + * @return array 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; + } + + $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(array_keys($missing), IQueryBuilder::PARAM_INT_ARRAY))); + + $entities = $this->findEntities($qb); + foreach ($entities 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 index f9ee8c2545..e1572671df 100644 --- a/lib/Helper/CircleHelper.php +++ b/lib/Helper/CircleHelper.php @@ -11,6 +11,7 @@ use OCA\Circles\Model\Circle; use OCA\Circles\Model\Member; use OCA\Circles\Model\Probes\CircleProbe; +use OCA\Tables\Errors\InternalError; use OCP\App\IAppManager; use OCP\IL10N; use OCP\Server; diff --git a/lib/Service/ShareService.php b/lib/Service/ShareService.php index a664478599..a1a72108ad 100644 --- a/lib/Service/ShareService.php +++ b/lib/Service/ShareService.php @@ -13,6 +13,7 @@ use InvalidArgumentException; use OC\User\User; +use OCA\Circles\Model\Circle; use OCA\Tables\AppInfo\Application; use OCA\Tables\Constants\ShareReceiverType; use OCA\Tables\Db\Context; @@ -39,6 +40,7 @@ use OCP\AppFramework\Db\TTransactional; use OCP\DB\Exception; use OCP\IDBConnection; +use OCP\IGroup; use OCP\IUserManager; use OCP\Security\ISecureRandom; use Psr\Log\LoggerInterface; @@ -181,7 +183,7 @@ protected function generateShareToken(): ShareToken { /** * @param string|null $userId - * @return array + * @return array Indexed by view id * @throws InternalError */ public function findViewsSharedWithMe(?string $userId = null): array { @@ -190,7 +192,7 @@ public function findViewsSharedWithMe(?string $userId = null): array { /** * @param string|null $userId - * @return array + * @return array Indexed by table id * @throws InternalError */ public function findTablesSharedWithMe(?string $userId = null): array { @@ -203,52 +205,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); - } - - // get all views or tables that are shared with me by circle - if ($this->circleHelper->isCirclesEnabled()) { - $userCircles = $this->circleHelper->getUserCircles($userId); + $userGroupIds = array_map(static fn (IGroup $group) => $group->getGid(), $userGroups); + $shares['groups'] = $this->mapper->findAllSharesFor($elementType, $userGroupIds, $userId, ShareReceiverType::GROUP); - 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')); - 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 @@ -286,7 +274,6 @@ public function create(int $nodeId, string $nodeType, string $receiver, string $ $this->logger->error($e->getMessage(), ['exception' => $e]); throw new InternalError(get_class($this) . ' - ' . __FUNCTION__ . ': ' . $e->getMessage()); } - $time = new DateTime(); $item = new Share(); $item->setSender($sender); @@ -583,6 +570,7 @@ public function findSharedWithUserIds(int $elementId, string $elementType): arra } } + /** * @param int $nodeId * @param array $share @@ -636,5 +624,4 @@ public function importShare(int $nodeId, array $share, string $userId): void { $this->logger->error('Failed to import share: ' . $e->getMessage(), ['exception' => $e, 'share' => $share]); } } - } diff --git a/lib/Service/ViewService.php b/lib/Service/ViewService.php index 8d6dfd22d6..cc05fc5b81 100644 --- 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); } From 98bb04a9ecc965ce8808e24cf0cf06c64a1d9628 Mon Sep 17 00:00:00 2001 From: Kostiantyn Miakshyn Date: Fri, 13 Feb 2026 22:35:14 +0100 Subject: [PATCH 2/2] Perf: Optimize shares loading by eliminating N+1 queries (code review fixes) Signed-off-by: Kostiantyn Miakshyn --- lib/Db/BulkFetchTrait.php | 23 +++++++++++++++++++++++ lib/Db/ShareMapper.php | 29 +++++++++++++++++++++-------- lib/Db/TableMapper.php | 22 +++++++++++++--------- lib/Db/ViewMapper.php | 26 +++++++++++++++----------- lib/Helper/CircleHelper.php | 2 -- lib/Service/ShareService.php | 2 +- 6 files changed, 73 insertions(+), 31 deletions(-) create mode 100644 lib/Db/BulkFetchTrait.php diff --git a/lib/Db/BulkFetchTrait.php b/lib/Db/BulkFetchTrait.php new file mode 100644 index 0000000000..1d584398f6 --- /dev/null +++ b/lib/Db/BulkFetchTrait.php @@ -0,0 +1,23 @@ +db->getDatabaseProvider()) { + IDBConnection::PLATFORM_ORACLE => 1000, + default => 65_535, + }; + return $maxParameters - $extraParameters; + } +} diff --git a/lib/Db/ShareMapper.php b/lib/Db/ShareMapper.php index 9c45f40244..e41af79de7 100644 --- a/lib/Db/ShareMapper.php +++ b/lib/Db/ShareMapper.php @@ -18,6 +18,8 @@ /** @template-extends QBMapper */ class ShareMapper extends QBMapper { + use BulkFetchTrait; + protected string $table = 'tables_shares'; protected LoggerInterface $logger; @@ -100,14 +102,25 @@ public function findShareForNode(int $nodeId, string $nodeType, string $receiver * @throws Exception */ public function findAllSharesFor(string $nodeType, array $receivers, string $userId, ?string $receiverType = 'user'): array { - $qb = $this->db->getQueryBuilder(); - $qb->select('*') - ->from($this->table) - ->where($qb->expr()->in('receiver', $qb->createNamedParameter($receivers, IQueryBuilder::PARAM_STR_ARRAY))) - ->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); + if (!$receivers) { + return []; + } + + $chunks = []; + // deduct extra parameters (sender, node type, receiver type) + foreach (array_chunk($receivers, $this->getChunkSize(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 index b10ab0246b..7c0ac49f67 100644 --- a/lib/Db/TableMapper.php +++ b/lib/Db/TableMapper.php @@ -19,6 +19,8 @@ /** @template-extends QBMapper */ class TableMapper extends QBMapper { + use BulkFetchTrait; + protected string $table = 'tables_tables'; protected CappedMemoryCache $cache; public function __construct( @@ -70,16 +72,18 @@ public function findMany(array $ids): array { return $result; } - $qb = $this->db->getQueryBuilder(); - $qb->select('*') - ->from($this->table) - ->where($qb->expr()->in('id', $qb->createNamedParameter(array_keys($missing), IQueryBuilder::PARAM_INT_ARRAY))); + $missing = array_keys($missing); + foreach (array_chunk($missing, $this->getChunkSize()) as $missingChunk) { + $qb = $this->db->getQueryBuilder(); + $qb->select('*') + ->from($this->table) + ->where($qb->expr()->in('id', $qb->createNamedParameter($missingChunk, IQueryBuilder::PARAM_INT_ARRAY))); - $entities = $this->findEntities($qb); - foreach ($entities as $entity) { - $id = $entity->getId(); - $this->cache[(string)$id] = $entity; - $result[$id] = $entity; + foreach ($this->findEntities($qb) as $entity) { + $id = $entity->getId(); + $this->cache[(string)$id] = $entity; + $result[$id] = $entity; + } } return $result; } diff --git a/lib/Db/ViewMapper.php b/lib/Db/ViewMapper.php index 740bad4a2d..b4cde66a3a 100644 --- a/lib/Db/ViewMapper.php +++ b/lib/Db/ViewMapper.php @@ -19,6 +19,8 @@ /** @template-extends QBMapper */ class ViewMapper extends QBMapper { + use BulkFetchTrait; + protected string $table = 'tables_views'; protected CappedMemoryCache $cache; @@ -70,17 +72,19 @@ public function findMany(array $ids): array { return $result; } - $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(array_keys($missing), IQueryBuilder::PARAM_INT_ARRAY))); - - $entities = $this->findEntities($qb); - foreach ($entities as $entity) { - $id = $entity->getId(); - $this->cache[(string)$id] = $entity; - $result[$id] = $entity; + $missing = array_keys($missing); + foreach (array_chunk($missing, $this->getChunkSize()) 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; } diff --git a/lib/Helper/CircleHelper.php b/lib/Helper/CircleHelper.php index e1572671df..b47f025d5e 100644 --- a/lib/Helper/CircleHelper.php +++ b/lib/Helper/CircleHelper.php @@ -11,7 +11,6 @@ use OCA\Circles\Model\Circle; use OCA\Circles\Model\Member; use OCA\Circles\Model\Probes\CircleProbe; -use OCA\Tables\Errors\InternalError; use OCP\App\IAppManager; use OCP\IL10N; use OCP\Server; @@ -77,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 index a1a72108ad..f04d5dfc14 100644 --- a/lib/Service/ShareService.php +++ b/lib/Service/ShareService.php @@ -570,7 +570,6 @@ public function findSharedWithUserIds(int $elementId, string $elementType): arra } } - /** * @param int $nodeId * @param array $share @@ -624,4 +623,5 @@ public function importShare(int $nodeId, array $share, string $userId): void { $this->logger->error('Failed to import share: ' . $e->getMessage(), ['exception' => $e, 'share' => $share]); } } + }