Skip to content

Commit 0bef51a

Browse files
KocKoc
committed
Perf: Optimize shares loading by eliminating N+1 queries
Signed-off-by: Kostiantyn Miakshyn <molodchick@gmail.com>
1 parent 33aaf1b commit 0bef51a

7 files changed

Lines changed: 115 additions & 62 deletions

File tree

‎lib/Db/ShareMapper.php‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -91,19 +91,19 @@ public function findShareForNode(int $nodeId, string $nodeType, string $receiver
9191

9292
/**
9393
* @param string $nodeType
94-
* @param string $receiver
94+
* @param string[]|int[] $receivers
9595
* @param string $userId
9696
* @param string|null $receiverType
9797
*
98-
* @return array
98+
* @return Share[]
9999
*
100100
* @throws Exception
101101
*/
102-
public function findAllSharesFor(string $nodeType, string $receiver, string $userId, ?string $receiverType = 'user'): array {
102+
public function findAllSharesFor(string $nodeType, array $receivers, string $userId, ?string $receiverType = 'user'): array {
103103
$qb = $this->db->getQueryBuilder();
104104
$qb->select('*')
105105
->from($this->table)
106-
->where($qb->expr()->eq('receiver', $qb->createNamedParameter($receiver, IQueryBuilder::PARAM_STR)))
106+
->where($qb->expr()->in('receiver', $qb->createNamedParameter($receivers, IQueryBuilder::PARAM_STR_ARRAY)))
107107
->andWhere($qb->expr()->neq('sender', $qb->createNamedParameter($userId, IQueryBuilder::PARAM_STR)))
108108
->andWhere($qb->expr()->eq('node_type', $qb->createNamedParameter($nodeType, IQueryBuilder::PARAM_STR)))
109109
->andWhere($qb->expr()->eq('receiver_type', $qb->createNamedParameter($receiverType, IQueryBuilder::PARAM_STR)));

‎lib/Db/TableMapper.php‎

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,44 @@ public function find(int $id): Table {
5050
return $this->cache[$cacheKey];
5151
}
5252

53+
/**
54+
* @param int[] $ids
55+
* @return array<int, Table> indexed by table id
56+
*
57+
* @throws DoesNotExistException
58+
* @throws Exception
59+
* @throws MultipleObjectsReturnedException
60+
*/
61+
public function findMany(array $ids): array {
62+
$missing = [];
63+
$result = [];
64+
foreach ($ids as $id) {
65+
$cacheKey = (string)$id;
66+
if (isset($this->cache[$cacheKey])) {
67+
$result[$id] = $this->cache[$cacheKey];
68+
} else {
69+
$missing[$id] = true;
70+
}
71+
}
72+
73+
if (!$missing) {
74+
return $result;
75+
}
76+
77+
$qb = $this->db->getQueryBuilder();
78+
$qb->select('*')
79+
->from($this->table)
80+
->where($qb->expr()->in('id', $qb->createNamedParameter($ids, IQueryBuilder::PARAM_INT_ARRAY)));
81+
82+
$entities = $this->findEntities($qb);
83+
foreach ($entities as $entity) {
84+
$id = $entity->getId();
85+
$this->cache[(string)$id] = $entity;
86+
$result[$id] = $entity;
87+
}
88+
return $result;
89+
}
90+
5391
/**
5492
* @param string|null $userId
5593
* @return Table[]

‎lib/Db/ViewMapper.php‎

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,44 @@ public function find(int $id): View {
5050
return $this->cache[$cacheKey];
5151
}
5252

53+
/**
54+
* @param int[] $ids
55+
* @return array<int, View> indexed by view id
56+
* @throws DoesNotExistException
57+
* @throws Exception
58+
* @throws MultipleObjectsReturnedException
59+
*/
60+
public function findMany(array $ids): array {
61+
$missing = [];
62+
$result = [];
63+
foreach ($ids as $id) {
64+
$cacheKey = (string)$id;
65+
if (isset($this->cache[$cacheKey])) {
66+
$result[$id] = $this->cache[$cacheKey];
67+
} else {
68+
$missing[$id] = true;
69+
}
70+
}
71+
72+
if (!$missing) {
73+
return $result;
74+
}
75+
76+
$qb = $this->db->getQueryBuilder();
77+
$qb->select('v.*', 't.ownership')
78+
->from($this->table, 'v')
79+
->innerJoin('v', 'tables_tables', 't', 't.id = v.table_id')
80+
->where($qb->expr()->in('v.id', $qb->createNamedParameter($ids, IQueryBuilder::PARAM_INT_ARRAY)));
81+
82+
$entities = $this->findEntities($qb);
83+
foreach ($entities as $entity) {
84+
$id = $entity->getId();
85+
$this->cache[(string)$id] = $entity;
86+
$result[$id] = $entity;
87+
}
88+
return $result;
89+
}
90+
5391
public function delete(Entity $entity): View {
5492
unset($this->cache[(string)$entity->getId()]);
5593
return parent::delete($entity);

‎lib/Helper/CircleHelper.php‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
use OCA\Circles\Model\Circle;
1212
use OCA\Circles\Model\Member;
1313
use OCA\Circles\Model\Probes\CircleProbe;
14+
use OCA\Tables\Errors\InternalError;
1415
use OCP\App\IAppManager;
1516
use OCP\IL10N;
1617
use OCP\Server;

‎lib/Service/ShareService.php‎

Lines changed: 27 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
use DateTime;
1313

1414
use InvalidArgumentException;
15+
use OCA\Circles\Model\Circle;
1516
use OCA\Tables\AppInfo\Application;
1617
use OCA\Tables\Constants\ShareReceiverType;
1718
use OCA\Tables\Db\Context;
@@ -178,7 +179,7 @@ protected function generateShareToken(): ShareToken {
178179

179180
/**
180181
* @param string|null $userId
181-
* @return array
182+
* @return array<int, View> Indexed by view id
182183
* @throws InternalError
183184
*/
184185
public function findViewsSharedWithMe(?string $userId = null): array {
@@ -187,7 +188,7 @@ public function findViewsSharedWithMe(?string $userId = null): array {
187188

188189
/**
189190
* @param string|null $userId
190-
* @return array
191+
* @return array<int, Table> Indexed by table id
191192
* @throws InternalError
192193
*/
193194
public function findTablesSharedWithMe(?string $userId = null): array {
@@ -200,52 +201,38 @@ public function findTablesSharedWithMe(?string $userId = null): array {
200201
private function findElementsSharedWithMe(string $elementType = 'table', ?string $userId = null): array {
201202
$userId = $this->permissionsService->preCheckUserId($userId);
202203

203-
$returnArray = [];
204-
204+
$shares = [];
205205
try {
206-
// get all views or tables that are shared with me as user
207-
$elementsSharedWithMe = $this->mapper->findAllSharesFor($elementType, $userId, $userId);
206+
$shares['user'] = $this->mapper->findAllSharesFor($elementType, [$userId], $userId);
208207

209-
// get all views or tables that are shared with me by group
210208
$userGroups = $this->userHelper->getGroupsForUser($userId);
211-
foreach ($userGroups as $userGroup) {
212-
$shares = $this->mapper->findAllSharesFor($elementType, $userGroup->getGid(), $userId, ShareReceiverType::GROUP);
213-
$elementsSharedWithMe = array_merge($elementsSharedWithMe, $shares);
214-
}
209+
$userGroupIds = array_map(static fn ($group) => $group->getGid(), $userGroups);
210+
$shares['groups'] = $this->mapper->findAllSharesFor($elementType, $userGroupIds, $userId, ShareReceiverType::GROUP);
215211

216-
// get all views or tables that are shared with me by circle
217-
if ($this->circleHelper->isCirclesEnabled()) {
218-
$userCircles = $this->circleHelper->getUserCircles($userId);
219-
220-
foreach ($userCircles as $userCircle) {
221-
$shares = $this->mapper->findAllSharesFor($elementType, $userCircle->getSingleId(), $userId, ShareReceiverType::CIRCLE);
222-
$elementsSharedWithMe = array_merge($elementsSharedWithMe, $shares);
223-
}
224-
}
212+
$userCircles = $this->circleHelper->getUserCircles($userId);
213+
$userCircleIds = array_map(static fn (Circle $circle) => $circle->getSingleId(), $userCircles);
214+
$shares['circles'] = $this->mapper->findAllSharesFor($elementType, $userCircleIds, $userId, ShareReceiverType::CIRCLE);
225215
} catch (Throwable $e) {
226-
throw new InternalError($e->getMessage());
216+
throw new InternalError($e->getMessage(), previous: $e);
227217
}
228-
foreach ($elementsSharedWithMe as $share) {
229-
try {
230-
if ($elementType === 'table') {
231-
$element = $this->tableMapper->find($share->getNodeId());
232-
} elseif ($elementType === 'view') {
233-
$element = $this->viewMapper->find($share->getNodeId());
234-
} else {
235-
throw new InternalError('Cannot find element of type ' . $elementType);
236-
}
237-
// Check if en element with this id is already in the result array
238-
$index = array_search($element->getId(), array_column($returnArray, 'id'));
239-
if (!$index) {
240-
$returnArray[] = $element;
241-
}
242-
} catch (DoesNotExistException|Exception|MultipleObjectsReturnedException $e) {
243-
throw new InternalError($e->getMessage());
244-
}
218+
219+
$elementIds = [];
220+
foreach (array_merge($shares['user'], $shares['groups'], $shares['circles']) as $share) {
221+
/** @var Share $share */
222+
$elementIds[$share->getNodeId()] = true;
245223
}
246-
return $returnArray;
247-
}
224+
$elementIds = array_keys($elementIds);
248225

226+
if ($elementType === 'table') {
227+
return $this->tableMapper->findMany($elementIds);
228+
}
229+
230+
if ($elementType === 'view') {
231+
return $this->viewMapper->findMany($elementIds);
232+
}
233+
234+
throw new InternalError('Cannot find element of type ' . $elementType);
235+
}
249236

250237
/**
251238
* @param int $elementId

‎lib/Service/TableService.php‎

Lines changed: 2 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -102,21 +102,14 @@ public function findAll(?string $userId = null, bool $skipTableEnhancement = fal
102102

103103
if (!$skipSharedTables && $userId !== '') {
104104
$sharedTables = $this->shareService->findTablesSharedWithMe($userId);
105-
106-
// clean duplicates
107-
foreach ($sharedTables as $sharedTable) {
108-
if (!isset($allTables[$sharedTable->getId()])) {
109-
$allTables[$sharedTable->getId()] = $sharedTable;
110-
}
111-
}
112105
}
113106

114107
$contexts = $this->contextService->findAll($userId);
115108
foreach ($contexts as $context) {
116109
$nodes = $context->getNodes();
117110
foreach ($nodes as $node) {
118111
if ($node['node_type'] !== Application::NODE_TYPE_TABLE
119-
|| isset($allTables[$node['node_id']])
112+
|| isset($sharedTables[$node['node_id']])
120113
) {
121114
continue;
122115
}
@@ -169,6 +162,7 @@ private function enhanceTable(Table $table, string $userId): void {
169162
// (senseless if we have no user in context)
170163
if ($userId !== '') {
171164
try {
165+
// fixme: use more efficient query (batched, do not load entities, just do count)
172166
$shares = $this->shareService->findAll('table', $table->getId());
173167
$table->setHasShares(count($shares) !== 0);
174168
} catch (InternalError $e) {

‎lib/Service/ViewService.php‎

Lines changed: 5 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -157,30 +157,24 @@ public function findSharedViewsWithMe(?string $userId = null): array {
157157
return [];
158158
}
159159

160-
$allViews = [];
161-
162160
$sharedViews = $this->shareService->findViewsSharedWithMe($userId);
163-
foreach ($sharedViews as $sharedView) {
164-
$allViews[$sharedView->getId()] = $sharedView;
165-
}
166-
167161
$contexts = $this->contextService->findAll($userId);
168162
foreach ($contexts as $context) {
169163
$nodes = $context->getNodes();
170164
foreach ($nodes as $node) {
171165
if ($node['node_type'] !== Application::NODE_TYPE_VIEW
172-
|| isset($allViews[$node['node_id']])
166+
|| isset($sharedViews[$node['node_id']])
173167
) {
174168
continue;
175169
}
176-
$allViews[$node['node_id']] = $this->find($node['node_id'], false, $userId);
170+
$sharedViews[$node['node_id']] = $this->find($node['node_id'], false, $userId);
177171
}
178172
}
179173

180-
foreach ($allViews as $view) {
174+
foreach ($sharedViews as $view) {
181175
$this->enhanceView($view, $userId);
182176
}
183-
return array_values($allViews);
177+
return array_values($sharedViews);
184178
}
185179

186180

@@ -409,6 +403,7 @@ public function deleteByObject(View $view, ?string $userId = null): View {
409403
* $userId can be set or ''
410404
*/
411405
private function enhanceView(View $view, string $userId): void {
406+
// fixme: optimize this method as well in the next iteration
412407
// add owner display name for UI
413408
$view->setOwnerDisplayName($this->userHelper->getUserDisplayName($view->getOwnership()));
414409

0 commit comments

Comments
 (0)