Skip to content

Commit d5ac3e9

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 d5ac3e9

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\Security\ISecureRandom;
4244
use Psr\Log\LoggerInterface;
4345
use Throwable;
@@ -178,7 +180,7 @@ protected function generateShareToken(): ShareToken {
178180

179181
/**
180182
* @param string|null $userId
181-
* @return array
183+
* @return array<int, View> Indexed by view id
182184
* @throws InternalError
183185
*/
184186
public function findViewsSharedWithMe(?string $userId = null): array {
@@ -187,7 +189,7 @@ public function findViewsSharedWithMe(?string $userId = null): array {
187189

188190
/**
189191
* @param string|null $userId
190-
* @return array
192+
* @return array<int, Table> Indexed by table id
191193
* @throws InternalError
192194
*/
193195
public function findTablesSharedWithMe(?string $userId = null): array {
@@ -200,52 +202,38 @@ public function findTablesSharedWithMe(?string $userId = null): array {
200202
private function findElementsSharedWithMe(string $elementType = 'table', ?string $userId = null): array {
201203
$userId = $this->permissionsService->preCheckUserId($userId);
202204

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

209-
// get all views or tables that are shared with me by group
210209
$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-
}
210+
$userGroupIds = array_map(static fn (IGroup $group) => $group->getGid(), $userGroups);
211+
$shares['groups'] = $this->mapper->findAllSharesFor($elementType, $userGroupIds, $userId, ShareReceiverType::GROUP);
215212

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-
}
213+
$userCircles = $this->circleHelper->getUserCircles($userId);
214+
$userCircleIds = array_map(static fn (Circle $circle) => $circle->getSingleId(), $userCircles);
215+
$shares['circles'] = $this->mapper->findAllSharesFor($elementType, $userCircleIds, $userId, ShareReceiverType::CIRCLE);
225216
} catch (Throwable $e) {
226-
throw new InternalError($e->getMessage());
217+
throw new InternalError($e->getMessage(), previous: $e);
227218
}
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-
}
219+
220+
$elementIds = [];
221+
foreach (array_merge($shares['user'], $shares['groups'], $shares['circles']) as $share) {
222+
/** @var Share $share */
223+
$elementIds[$share->getNodeId()] = true;
245224
}
246-
return $returnArray;
247-
}
225+
$elementIds = array_keys($elementIds);
248226

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

250238
/**
251239
* @param int $elementId

‎lib/Service/ViewService.php‎

Lines changed: 4 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

0 commit comments

Comments
 (0)