Skip to content

Commit 50fdced

Browse files
KocKoc
committed
Perf: Optimize shares loading by eliminating N+1 queries
Signed-off-by: Kostiantyn Miakshyn <molodchick@gmail.com>
1 parent 41276d7 commit 50fdced

6 files changed

Lines changed: 108 additions & 57 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: 30 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,9 @@
1212
use DateTime;
1313

1414
use InvalidArgumentException;
15+
use OCA\Circles\Model\Circle;
1516
use OC\User\User;
17+
use OCP\IUserManager;
1618
use OCA\Tables\AppInfo\Application;
1719
use OCA\Tables\Constants\ShareReceiverType;
1820
use OCA\Tables\Db\Context;
@@ -39,7 +41,7 @@
3941
use OCP\AppFramework\Db\TTransactional;
4042
use OCP\DB\Exception;
4143
use OCP\IDBConnection;
42-
use OCP\IUserManager;
44+
use OCP\IGroup;
4345
use OCP\Security\ISecureRandom;
4446
use Psr\Log\LoggerInterface;
4547
use Throwable;
@@ -181,7 +183,7 @@ protected function generateShareToken(): ShareToken {
181183

182184
/**
183185
* @param string|null $userId
184-
* @return array
186+
* @return array<int, View> Indexed by view id
185187
* @throws InternalError
186188
*/
187189
public function findViewsSharedWithMe(?string $userId = null): array {
@@ -190,7 +192,7 @@ public function findViewsSharedWithMe(?string $userId = null): array {
190192

191193
/**
192194
* @param string|null $userId
193-
* @return array
195+
* @return array<int, Table> Indexed by table id
194196
* @throws InternalError
195197
*/
196198
public function findTablesSharedWithMe(?string $userId = null): array {
@@ -203,52 +205,38 @@ public function findTablesSharedWithMe(?string $userId = null): array {
203205
private function findElementsSharedWithMe(string $elementType = 'table', ?string $userId = null): array {
204206
$userId = $this->permissionsService->preCheckUserId($userId);
205207

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

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

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

234+
if ($elementType === 'view') {
235+
return $this->viewMapper->findMany($elementIds);
236+
}
237+
238+
throw new InternalError('Cannot find element of type ' . $elementType);
239+
}
252240

253241
/**
254242
* @param int $elementId
@@ -286,7 +274,6 @@ public function create(int $nodeId, string $nodeType, string $receiver, string $
286274
$this->logger->error($e->getMessage(), ['exception' => $e]);
287275
throw new InternalError(get_class($this) . ' - ' . __FUNCTION__ . ': ' . $e->getMessage());
288276
}
289-
290277
$time = new DateTime();
291278
$item = new Share();
292279
$item->setSender($sender);
@@ -583,6 +570,7 @@ public function findSharedWithUserIds(int $elementId, string $elementType): arra
583570
}
584571
}
585572

573+
586574
/**
587575
* @param int $nodeId
588576
* @param array $share
@@ -636,5 +624,4 @@ public function importShare(int $nodeId, array $share, string $userId): void {
636624
$this->logger->error('Failed to import share: ' . $e->getMessage(), ['exception' => $e, 'share' => $share]);
637625
}
638626
}
639-
640627
}

‎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)