Skip to content

Commit 938b7c8

Browse files
KocKoc
committed
Perf: Optimize shares loading by eliminating N+1 queries
Signed-off-by: Kostiantyn Miakshyn <molodchick@gmail.com>
1 parent 4c6ce16 commit 938b7c8

6 files changed

Lines changed: 106 additions & 54 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: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,40 @@ 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+
public function findMany(array $ids): array {
58+
$missing = [];
59+
$result = [];
60+
foreach ($ids as $id) {
61+
$cacheKey = (string)$id;
62+
if (isset($this->cache[$cacheKey])) {
63+
$result[$id] = $this->cache[$cacheKey];
64+
} else {
65+
$missing[$id] = true;
66+
}
67+
}
68+
69+
if (!$missing) {
70+
return $result;
71+
}
72+
73+
$qb = $this->db->getQueryBuilder();
74+
$qb->select('*')
75+
->from($this->table)
76+
->where($qb->expr()->in('id', $qb->createNamedParameter(array_keys($missing), IQueryBuilder::PARAM_INT_ARRAY)));
77+
78+
$entities = $this->findEntities($qb);
79+
foreach ($entities as $entity) {
80+
$id = $entity->getId();
81+
$this->cache[(string)$id] = $entity;
82+
$result[$id] = $entity;
83+
}
84+
return $result;
85+
}
86+
5387
/**
5488
* @param string|null $userId
5589
* @return Table[]

‎lib/Db/ViewMapper.php‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,41 @@ 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+
*/
57+
public function findMany(array $ids): array {
58+
$missing = [];
59+
$result = [];
60+
foreach ($ids as $id) {
61+
$cacheKey = (string)$id;
62+
if (isset($this->cache[$cacheKey])) {
63+
$result[$id] = $this->cache[$cacheKey];
64+
} else {
65+
$missing[$id] = true;
66+
}
67+
}
68+
69+
if (!$missing) {
70+
return $result;
71+
}
72+
73+
$qb = $this->db->getQueryBuilder();
74+
$qb->select('v.*', 't.ownership')
75+
->from($this->table, 'v')
76+
->innerJoin('v', 'tables_tables', 't', 't.id = v.table_id')
77+
->where($qb->expr()->in('v.id', $qb->createNamedParameter(array_keys($missing), IQueryBuilder::PARAM_INT_ARRAY)));
78+
79+
$entities = $this->findEntities($qb);
80+
foreach ($entities as $entity) {
81+
$id = $entity->getId();
82+
$this->cache[(string)$id] = $entity;
83+
$result[$id] = $entity;
84+
}
85+
return $result;
86+
}
87+
5388
public function delete(Entity $entity): View {
5489
unset($this->cache[(string)$entity->getId()]);
5590
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: 28 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;
@@ -38,6 +39,7 @@
3839
use OCP\AppFramework\Db\TTransactional;
3940
use OCP\DB\Exception;
4041
use OCP\IDBConnection;
42+
use OCP\IGroup;
4143
use OCP\IUserManager;
4244
use OCP\Security\IHasher;
4345
use OCP\Security\ISecureRandom;
@@ -194,7 +196,7 @@ protected function generateShareToken(): ShareToken {
194196

195197
/**
196198
* @param string|null $userId
197-
* @return array
199+
* @return array<int, View> Indexed by view id
198200
* @throws InternalError
199201
*/
200202
public function findViewsSharedWithMe(?string $userId = null): array {
@@ -203,7 +205,7 @@ public function findViewsSharedWithMe(?string $userId = null): array {
203205

204206
/**
205207
* @param string|null $userId
206-
* @return array
208+
* @return array<int, Table> Indexed by table id
207209
* @throws InternalError
208210
*/
209211
public function findTablesSharedWithMe(?string $userId = null): array {
@@ -216,52 +218,38 @@ public function findTablesSharedWithMe(?string $userId = null): array {
216218
private function findElementsSharedWithMe(string $elementType = 'table', ?string $userId = null): array {
217219
$userId = $this->permissionsService->preCheckUserId($userId);
218220

219-
$returnArray = [];
220-
221+
$shares = [];
221222
try {
222-
// get all views or tables that are shared with me as user
223-
$elementsSharedWithMe = $this->mapper->findAllSharesFor($elementType, $userId, $userId);
223+
$shares['user'] = $this->mapper->findAllSharesFor($elementType, [$userId], $userId);
224224

225-
// get all views or tables that are shared with me by group
226225
$userGroups = $this->userHelper->getGroupsForUser($userId);
227-
foreach ($userGroups as $userGroup) {
228-
$shares = $this->mapper->findAllSharesFor($elementType, $userGroup->getGid(), $userId, ShareReceiverType::GROUP);
229-
$elementsSharedWithMe = array_merge($elementsSharedWithMe, $shares);
230-
}
226+
$userGroupIds = array_map(static fn (IGroup $group) => $group->getGid(), $userGroups);
227+
$shares['groups'] = $this->mapper->findAllSharesFor($elementType, $userGroupIds, $userId, ShareReceiverType::GROUP);
231228

232-
// get all views or tables that are shared with me by circle
233-
if ($this->circleHelper->isCirclesEnabled()) {
234-
$userCircles = $this->circleHelper->getUserCircles($userId);
235-
236-
foreach ($userCircles as $userCircle) {
237-
$shares = $this->mapper->findAllSharesFor($elementType, $userCircle->getSingleId(), $userId, ShareReceiverType::CIRCLE);
238-
$elementsSharedWithMe = array_merge($elementsSharedWithMe, $shares);
239-
}
240-
}
229+
$userCircles = $this->circleHelper->getUserCircles($userId);
230+
$userCircleIds = array_map(static fn (Circle $circle) => $circle->getSingleId(), $userCircles);
231+
$shares['circles'] = $this->mapper->findAllSharesFor($elementType, $userCircleIds, $userId, ShareReceiverType::CIRCLE);
241232
} catch (Throwable $e) {
242-
throw new InternalError($e->getMessage());
233+
throw new InternalError($e->getMessage(), previous: $e);
243234
}
244-
foreach ($elementsSharedWithMe as $share) {
245-
try {
246-
if ($elementType === 'table') {
247-
$element = $this->tableMapper->find($share->getNodeId());
248-
} elseif ($elementType === 'view') {
249-
$element = $this->viewMapper->find($share->getNodeId());
250-
} else {
251-
throw new InternalError('Cannot find element of type ' . $elementType);
252-
}
253-
// Check if en element with this id is already in the result array
254-
$index = array_search($element->getId(), array_column($returnArray, 'id'), true);
255-
if (!$index) {
256-
$returnArray[] = $element;
257-
}
258-
} catch (DoesNotExistException|Exception|MultipleObjectsReturnedException $e) {
259-
throw new InternalError($e->getMessage());
260-
}
235+
236+
$elementIds = [];
237+
foreach (array_merge($shares['user'], $shares['groups'], $shares['circles']) as $share) {
238+
/** @var Share $share */
239+
$elementIds[$share->getNodeId()] = true;
261240
}
262-
return $returnArray;
263-
}
241+
$elementIds = array_keys($elementIds);
264242

243+
if ($elementType === 'table') {
244+
return $this->tableMapper->findMany($elementIds);
245+
}
246+
247+
if ($elementType === 'view') {
248+
return $this->viewMapper->findMany($elementIds);
249+
}
250+
251+
throw new InternalError('Cannot find element of type ' . $elementType);
252+
}
265253

266254
/**
267255
* @param int $elementId

‎lib/Service/ViewService.php‎

Lines changed: 4 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -163,30 +163,24 @@ public function findSharedViewsWithMe(?string $userId = null): array {
163163
return [];
164164
}
165165

166-
$allViews = [];
167-
168166
$sharedViews = $this->shareService->findViewsSharedWithMe($userId);
169-
foreach ($sharedViews as $sharedView) {
170-
$allViews[$sharedView->getId()] = $sharedView;
171-
}
172-
173167
$contexts = $this->contextService->findAll($userId);
174168
foreach ($contexts as $context) {
175169
$nodes = $context->getNodes();
176170
foreach ($nodes as $node) {
177171
if ($node['node_type'] !== Application::NODE_TYPE_VIEW
178-
|| isset($allViews[$node['node_id']])
172+
|| isset($sharedViews[$node['node_id']])
179173
) {
180174
continue;
181175
}
182-
$allViews[$node['node_id']] = $this->find($node['node_id'], false, $userId);
176+
$sharedViews[$node['node_id']] = $this->find($node['node_id'], false, $userId);
183177
}
184178
}
185179

186-
foreach ($allViews as $view) {
180+
foreach ($sharedViews as $view) {
187181
$this->enhanceView($view, $userId);
188182
}
189-
return array_values($allViews);
183+
return array_values($sharedViews);
190184
}
191185

192186

0 commit comments

Comments
 (0)