Skip to content

perf: Enhance objects using constant sql queries count to get rid of N+1 queries #2857

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Koc wants to merge 1 commit into
base: main
Choose a base branch
from
Open

perf: Enhance objects using constant sql queries count to get rid of N+1 queries #2857

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 3 additions & 10 deletions lib/Analytics/AnalyticsDatasource.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,16 +103,9 @@ public function getTemplate(): array {
// get all tables and subsequent views
foreach ($tables as $table) {
$tableString = $tableString . $table->getId() . '-' . $table->getTitle() . '/';
// get all views per table
try {
$views = $this->viewService->findAll($this->tableService->find($table->getId()), $this->userId);
foreach ($views as $view) {
// concatenate the option-string. The format is tableId:viewId-title
$tableString = $tableString . $table->getId() . ':' . $view->getId() . '-' . $view->getTitle() . '/';
}
} catch (PermissionError $e) {
// this is a shared table without shared views;
continue;
foreach ($table->getViews() ?? [] as $view) {
// concatenate the option-string. The format is tableId:viewId-title
$tableString = $tableString . $table->getId() . ':' . $view->getId() . '-' . $view->getTitle() . '/';
}
}

Expand Down
3 changes: 2 additions & 1 deletion lib/Controller/Api1Controller.php
View file
Open in desktop

@enjeck enjeck Aug 25, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a downstream effect of the findAllSharesForNode change above, the list all shares" endpoint now returns only shares created by the current user. An owner will no longer see shares created by co-managers on their own table or view, so they cannot review or revoke them here?

@enjeck enjeck Aug 25, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same for sharecontroller.php

Original file line number Diff line number Diff line change
Expand Up @@ -328,7 +328,8 @@ public function deleteTable(int $tableId): DataResponse {
#[OpenAPI(scope: OpenAPI::SCOPE_DEFAULT)]
public function indexViews(int $tableId): DataResponse {
try {
return new DataResponse($this->viewService->formatViews($this->viewService->findAll($this->tableService->find($tableId))));
$table = $this->tableService->find($tableId);
return new DataResponse($this->viewService->formatViews($table->getViews() ?? []));

@enjeck enjeck Aug 25, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

initially, we return 403 for read-only users . Now, read-only user gets 200 with an empty list, since enhanceTables only loads views for owned or managed tables. So we need to mofify the annotation?

} catch (PermissionError $e) {
$this->logger->warning('A permission error occurred: ' . $e->getMessage(), ['exception' => $e]);
$message = ['message' => $e->getMessage()];
Expand Down
36 changes: 21 additions & 15 deletions lib/Db/ColumnMapper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,24 +165,30 @@ public function getColumnTypes(array $neededColumnIds): array {
}

/**
* @param int $tableId
* @return int
* @param int[] $tableIds
* @return array<int, int>
*/
public function countColumns(int $tableId): int {
$qb = $this->db->getQueryBuilder();
$qb->select($qb->func()->count('*', 'counter'));
$qb->from($this->table);
$qb->where(
$qb->expr()->eq('table_id', $qb->createNamedParameter($tableId))
);
public function countColumnsForTables(array $tableIds): array {
if (empty($tableIds)) {
return [];
}

try {
$result = $this->findOneQuery($qb);
return (int)$result['counter'];
} catch (DoesNotExistException|MultipleObjectsReturnedException|Exception $e) {
$this->logger->warning('Exception occurred: ' . $e->getMessage() . ' Returning 0.');
return 0;
$counts = array_fill_keys($tableIds, 0);
foreach (array_chunk($tableIds, 1000 - 1) as $tableIdsChunk) {
$qb = $this->db->getQueryBuilder();
$qb->select('table_id', $qb->func()->count('*', 'counter'))
->from($this->table)
->where($qb->expr()->in('table_id', $qb->createNamedParameter($tableIdsChunk, IQueryBuilder::PARAM_INT_ARRAY)))
->groupBy('table_id');

$result = $qb->executeQuery();
while ($row = $result->fetch()) {
$counts[(int)$row['table_id']] = (int)$row['counter'];
}
$result->closeCursor();
Comment on lines +176 to +188

@enjeck enjeck Aug 25, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

without try...catch, a \OCP\DB\Exception now propagates?

}

return $counts;
}

/**
Expand Down
8 changes: 6 additions & 2 deletions lib/Db/Row2Mapper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -962,8 +962,12 @@ public function deleteAllForTable(int $tableId, array $columns): void {
}
}

public function countRowsForTable(int $tableId): int {
return $this->rowSleeveMapper->countRows($tableId);
/**
* @param int[] $tableIds
* @return array<int, int>
*/
public function countRowsForTables(array $tableIds): array {
return $this->rowSleeveMapper->countRowsForTables($tableIds);
}

/**
Expand Down
38 changes: 22 additions & 16 deletions lib/Db/RowSleeveMapper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,24 +103,30 @@ public function deleteAllForTable(int $tableId): int {
}

/**
* @param int $tableId
* @return int
* @param int[] $tableIds
* @return array<int, int>
*/
public function countRows(int $tableId): int {
$qb = $this->db->getQueryBuilder();
$qb->select($qb->func()->count('*', 'counter'));
$qb->from($this->table, 't1');
$qb->where(
$qb->expr()->eq('table_id', $qb->createNamedParameter($tableId))
);

try {
$result = $this->findOneQuery($qb);
return (int)$result['counter'];
} catch (DoesNotExistException|MultipleObjectsReturnedException|Exception $e) {
$this->logger->warning('Exception occurred: ' . $e->getMessage() . ' Will return 0.');
return 0;
public function countRowsForTables(array $tableIds): array {
if (empty($tableIds)) {
return [];
}

$counts = array_fill_keys($tableIds, 0);
foreach (array_chunk($tableIds, 1000 - 1) as $tableIdsChunk) {
$qb = $this->db->getQueryBuilder();
$qb->select('table_id', $qb->func()->count('*', 'counter'))
->from($this->table)
->where($qb->expr()->in('table_id', $qb->createNamedParameter($tableIdsChunk, IQueryBuilder::PARAM_INT_ARRAY)))
->groupBy('table_id');

$result = $qb->executeQuery();
while ($row = $result->fetch()) {
$counts[(int)$row['table_id']] = (int)$row['counter'];
}
$result->closeCursor();
}

return $counts;
}

/**
Expand Down
77 changes: 76 additions & 1 deletion lib/Db/ShareMapper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ public function findAllSharesFor(string $nodeType, array $receivers, string $use
return [];
}

$chunks = [];
$chunks = [[]];
// deduct extra parameters (sender, node type, receiver type)
foreach (array_chunk($receivers, 1000 - 3) as $receiversChunk) {
$qb = $this->db->getQueryBuilder();
Expand Down Expand Up @@ -130,13 +130,88 @@ public function findAllSharesForNode(string $nodeType, int $nodeId, string $send
->andWhere($qb->expr()->eq('node_type', $qb->createNamedParameter($nodeType, IQueryBuilder::PARAM_STR)))
->andWhere($qb->expr()->eq('node_id', $qb->createNamedParameter($nodeId, IQueryBuilder::PARAM_INT)));

if ($sender !== '') {
$qb->andWhere($qb->expr()->eq('sender', $qb->createNamedParameter($sender, IQueryBuilder::PARAM_STR)));
}

Comment on lines +133 to +136

@enjeck enjeck Aug 25, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be moved to plural findAllSharesForNodes only (which is what the count methods need)? This adds sender filtering to the shared singular method, which previously ignored $sender and returned all shares for a node. now, it changes every existing caller that passes a user id.

if (!empty($excluded)) {
$qb->andWhere($qb->expr()->notIn('receiver_type', $qb->createNamedParameter($excluded, IQueryBuilder::PARAM_STR_ARRAY)));
}

return $this->findEntities($qb);
}

/**
* @param string $nodeType
* @param int[] $nodeIds
* @param string $sender
* @param array<string> $excluded receiver types to exclude from results
* @return Share[]
* @throws Exception
*/
public function findAllSharesForNodes(string $nodeType, array $nodeIds, string $sender = '', array $excluded = []): array {
Comment thread
Koc marked this conversation as resolved.
if (empty($nodeIds)) {
return [];
}

$extraParams = 2;
if ($sender !== '') {
$extraParams++;
}
if (!empty($excluded)) {
$extraParams++;
}

$chunks = [[]];
foreach (array_chunk($nodeIds, 1000 - $extraParams) as $nodeIdsChunk) {
$qb = $this->db->getQueryBuilder();
$qb->select('*')
->from($this->table)
->andWhere($qb->expr()->eq('node_type', $qb->createNamedParameter($nodeType, IQueryBuilder::PARAM_STR)))
->andWhere($qb->expr()->in('node_id', $qb->createNamedParameter($nodeIdsChunk, IQueryBuilder::PARAM_INT_ARRAY)));

if ($sender !== '') {
$qb->andWhere($qb->expr()->eq('sender', $qb->createNamedParameter($sender, IQueryBuilder::PARAM_STR)));
}

if (!empty($excluded)) {
$qb->andWhere($qb->expr()->notIn('receiver_type', $qb->createNamedParameter($excluded, IQueryBuilder::PARAM_STR_ARRAY)));
}

$chunks[] = $this->findEntities($qb);
}

return array_merge(...$chunks);
}

/**
* @param int[] $tableIds
* @return int[]
*/
public function findLinkSharesForTables(array $tableIds): array {
if (empty($tableIds)) {
return [];
}

$linkTableIds = [];
foreach (array_chunk($tableIds, 1000 - 2) as $tableIdsChunk) {
$qb = $this->db->getQueryBuilder();
$qb->selectDistinct('node_id')
->from($this->table)
->andWhere($qb->expr()->eq('node_type', $qb->createNamedParameter('table', IQueryBuilder::PARAM_STR)))
->andWhere($qb->expr()->eq('receiver_type', $qb->createNamedParameter(ShareReceiverType::LINK, IQueryBuilder::PARAM_STR)))
->andWhere($qb->expr()->in('node_id', $qb->createNamedParameter($tableIdsChunk, IQueryBuilder::PARAM_INT_ARRAY)));

$result = $qb->executeQuery();
while ($row = $result->fetch()) {
$linkTableIds[] = (int)$row['node_id'];
}
$result->closeCursor();
}

return $linkTableIds;
}

/**
* @param string $nodeType
* @param int $nodeId
Expand Down
21 changes: 21 additions & 0 deletions lib/Db/ViewMapper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,8 @@ public function insert(Entity $entity): View {
}

/**
* @deprecated Use {@link findAllByTableIds} instead
*
* @return View[]
* @throws Exception
*/
Expand All @@ -142,6 +144,25 @@ public function findAll(?int $tableId = null): array {
return $this->findEntities($qb);
}

/**
* @param int[] $tableIds
* @return View[]
* @throws Exception
*/
public function findAllByTableIds(array $tableIds): array {
if (empty($tableIds)) {
return [];
}

$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.table_id', $qb->createNamedParameter($tableIds, IQueryBuilder::PARAM_INT_ARRAY)));

@enjeck enjeck Aug 25, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't we need same array_chunk here? Since every other batch method you added chunks the IN list at about 1000, and findMany does too, but this one passes all table ids in a single IN. What if the user has more than ~1000 tables, or the DB has a 1000 element IN limit (Oracle does, i think?)??


return $this->findEntities($qb);
}

/**
* @return View[]
* @throws Exception
Expand Down
17 changes: 17 additions & 0 deletions lib/Helper/UserHelper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,23 @@ public function getUserDisplayName(string $userId): string {
}
}

/**
* @param string[] $userIds
* @return array<string, null|string>
*/
public function getUsersDisplayNames(array $userIds): array {
$displayNames = [];
foreach (array_unique($userIds) as $userId) {
if ($userId === '' || $userId === null) {
$displayNames[$userId] = $userId;
continue;
}
$displayName = $this->userManager->getDisplayName($userId);
$displayNames[$userId] = $displayName ?: $userId;
}
return $displayNames;
}

/**
* @throws InternalError
*/
Expand Down
19 changes: 11 additions & 8 deletions lib/Service/ColumnService.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -695,16 +695,19 @@ public function findOrCreateColumnsByTitleForTableAsArray(?int $tableId, ?int $v
}

/**
* @param int $tableId
* @return int
* @throws PermissionError
* @param int[] $tableIds
* @param string|null $userId
* @return array<int, int>
*/
public function getColumnsCount(int $tableId): int {
if ($this->permissionsService->canReadColumnsByTableId($tableId)) {
return $this->mapper->countColumns($tableId);
} else {
throw new PermissionError('no read access for counting to table id = ' . $tableId);
public function getColumnsCountForTables(array $tableIds, ?string $userId = null): array {
$userId = $this->permissionsService->preCheckUserId($userId);
$counts = $this->mapper->countColumnsForTables($tableIds);
foreach ($tableIds as $tableId) {
if (!$this->permissionsService->canReadColumnsByTableId($tableId, $userId)) {
$counts[$tableId] = 0;
}
}
return $counts;
}

/**
Expand Down
20 changes: 11 additions & 9 deletions lib/Service/RowService.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -861,17 +861,19 @@ public function deleteColumnDataFromRows(Column $column):void {
}

/**
* @param int $tableId
* @return int
*
* @throws PermissionError
* @param int[] $tableIds
* @param string|null $userId
* @return array<int, int>
*/
public function getRowsCount(int $tableId): int {
if ($this->permissionsService->canReadRowsByElementId($tableId, 'table')) {
return $this->row2Mapper->countRowsForTable($tableId);
} else {
throw new PermissionError('no read access for counting to table id = ' . $tableId);
public function getRowsCountForTables(array $tableIds, ?string $userId = null): array {
$userId = $this->permissionsService->preCheckUserId($userId);
$counts = $this->row2Mapper->countRowsForTables($tableIds);
foreach ($tableIds as $tableId) {
if (!$this->permissionsService->canReadRowsByElementId($tableId, 'table', $userId)) {
$counts[$tableId] = 0;
}
}
return $counts;
}

/**
Expand Down
Loading
Loading