From baf8a5cf76c50af2cb428b1c8b36d5e7d702da6a Mon Sep 17 00:00:00 2001 From: Kostiantyn Miakshyn Date: Sun, 28 Dec 2025 16:49:54 +0100 Subject: [PATCH] Perf: Optimize performance of the get rows endpoints Signed-off-by: Kostiantyn Miakshyn --- appinfo/info.xml | 8 +- lib/Config/ConfigLexicon.php | 2 + lib/Db/ColumnMapper.php | 4 +- lib/Db/Row2Mapper.php | 301 ++++++++---------- lib/Db/RowCellMapperSuper.php | 31 ++ lib/Db/RowCellUsergroupMapper.php | 7 + lib/Db/RowLoader/CachedRowLoader.php | 94 ++++++ lib/Db/RowLoader/NormalizedRowLoader.php | 157 +++++++++ lib/Db/RowLoader/RowLoader.php | 31 ++ lib/Db/RowSleeve.php | 21 ++ lib/Db/RowSleeveMapper.php | 46 ++- lib/Helper/ColumnsHelper.php | 51 ++- lib/Migration/CacheSleeveCells.php | 115 +++++++ .../Version2232Date20260906000000.php | 41 +++ lib/UserMigration/TablesMigrator.php | 26 ++ tests/integration/features/APIv2.feature | 48 ++- .../features/bootstrap/FeatureContext.php | 71 +++-- tests/unit/Database/DatabaseTestCase.php | 62 ++++ tests/unit/Db/Row2MapperTest.php | 58 ++++ tests/unit/Db/Row2MapperTestDependencies.php | 78 ++++- tests/unit/TablesMigratorTest.php | 10 +- 21 files changed, 1052 insertions(+), 210 deletions(-) create mode 100644 lib/Db/RowLoader/CachedRowLoader.php create mode 100644 lib/Db/RowLoader/NormalizedRowLoader.php create mode 100644 lib/Db/RowLoader/RowLoader.php create mode 100644 lib/Migration/CacheSleeveCells.php create mode 100644 lib/Migration/Version2232Date20260906000000.php diff --git a/appinfo/info.xml b/appinfo/info.xml index 8545043c14..094e6c7eac 100644 --- a/appinfo/info.xml +++ b/appinfo/info.xml @@ -26,7 +26,7 @@ Share your tables and views with users and groups within your cloud. Have a good time and manage whatever you want. ]]> - 2.4.0-dev.0 + 2.4.0-dev.1 AGPL-3.0-or-later Nextcloud GmbH and Nextcloud contributors Tables @@ -59,6 +59,12 @@ Have a good time and manage whatever you want. OCA\Tables\Migration\NewDbStructureRepairStep OCA\Tables\Migration\DbRowSleeveSequence + + OCA\Tables\Migration\CacheSleeveCells + + + OCA\Tables\Migration\CacheSleeveCells + OCA\Tables\Command\ListTables diff --git a/lib/Config/ConfigLexicon.php b/lib/Config/ConfigLexicon.php index 5fed49ccd9..e84520dc87 100644 --- a/lib/Config/ConfigLexicon.php +++ b/lib/Config/ConfigLexicon.php @@ -23,6 +23,7 @@ */ class ConfigLexicon implements ILexicon { public const FEDERATION_ENABLED = 'federationEnabled'; + public const CACHING_SLEEVE_CELLS_COMPLETE = 'cachingSleeveCellsComplete'; #[\Override] public function getStrictness(): Strictness { @@ -33,6 +34,7 @@ public function getStrictness(): Strictness { public function getAppConfigs(): array { return [ new Entry(self::FEDERATION_ENABLED, ValueType::BOOL, true, 'Enable or disable federated table sharing'), + new Entry(self::CACHING_SLEEVE_CELLS_COMPLETE, ValueType::BOOL, false, 'Cells of all rows have been cached into the row-sleeves table'), ]; } diff --git a/lib/Db/ColumnMapper.php b/lib/Db/ColumnMapper.php index ac9de1512c..9b9aa522c4 100644 --- a/lib/Db/ColumnMapper.php +++ b/lib/Db/ColumnMapper.php @@ -92,7 +92,7 @@ public function findAll(array $id): array { /** * @param integer $tableId - * @return array + * @return Column[] * @throws Exception */ public function findAllByTable(int $tableId): array { @@ -117,7 +117,7 @@ public function findAllByTable(int $tableId): array { /** * @param integer $tableID - * @return array + * @return int[] * @throws Exception */ public function findAllIdsByTable(int $tableID): array { diff --git a/lib/Db/Row2Mapper.php b/lib/Db/Row2Mapper.php index aeb7c59c8c..5eb2e87e4c 100644 --- a/lib/Db/Row2Mapper.php +++ b/lib/Db/Row2Mapper.php @@ -9,7 +9,11 @@ use DateTime; use DateTimeImmutable; +use OCA\Tables\AppInfo\Application; +use OCA\Tables\Config\ConfigLexicon; use OCA\Tables\Constants\UsergroupType; +use OCA\Tables\Db\RowLoader\CachedRowLoader; +use OCA\Tables\Db\RowLoader\NormalizedRowLoader; use OCA\Tables\Errors\InternalError; use OCA\Tables\Errors\NotFoundError; use OCA\Tables\Helper\ColumnsHelper; @@ -18,19 +22,19 @@ use OCP\AppFramework\Db\MultipleObjectsReturnedException; use OCP\AppFramework\Db\TTransactional; use OCP\DB\Exception; -use OCP\DB\IResult; use OCP\DB\QueryBuilder\IQueryBuilder; +use OCP\IAppConfig; use OCP\IDBConnection; -use OCP\Server; -use Psr\Container\ContainerExceptionInterface; -use Psr\Container\NotFoundExceptionInterface; use Psr\Log\LoggerInterface; use Throwable; class Row2Mapper { use TTransactional; - private const DB_CHUNK_SIZE = 1_000; + /** + * @var array + */ + private array $rowLoaders; public function __construct( private ?string $userId, @@ -40,7 +44,14 @@ public function __construct( private RowSleeveMapper $rowSleeveMapper, private ColumnsHelper $columnsHelper, protected ColumnMapper $columnMapper, + NormalizedRowLoader $normalizedRowLoader, + CachedRowLoader $cachedRowLoader, + private IAppConfig $appConfig, ) { + $this->rowLoaders = [ + RowLoader\RowLoader::LOADER_NORMALIZED => $normalizedRowLoader, + RowLoader\RowLoader::LOADER_CACHED => $cachedRowLoader, + ]; } /** @@ -49,12 +60,13 @@ public function __construct( * @throws Exception */ public function delete(Row2 $row): Row2 { + $rowId = $row->getId(); $this->db->beginTransaction(); try { foreach ($this->columnsHelper->columns as $columnType) { - $this->getCellMapperFromType($columnType)->deleteAllForRow($row->getId()); + $this->columnsHelper->getCellMapperFromType($columnType)->deleteAllForRow($rowId); } - $this->rowSleeveMapper->deleteById($row->getId()); + $this->rowSleeveMapper->deleteById($rowId); $this->db->commit(); } catch (Throwable $e) { $this->db->rollBack(); @@ -177,7 +189,7 @@ public function findAll(array $showColumnIds, int $tableId, ?int $limit = null, $wantedRowIdsArray = $this->getWantedRowIds($userId, $tableId, $filter, $sort, $limit, $offset); // Get rows without SQL sorting - $rows = $this->getRows($wantedRowIdsArray, $showColumnIds); + $rows = $this->getRows($wantedRowIdsArray, $showColumnIds, $this->getRowLoader()); // Sort rows in PHP to preserve the order from getWantedRowIds return $this->sortRowsByIds($rows, $wantedRowIdsArray); @@ -188,81 +200,45 @@ public function findAll(array $showColumnIds, int $tableId, ?int $limit = null, } /** - * @param array $rowIds - * @param array $columnIds - * @return Row2[] - * @throws InternalError + * The cached_cells column is only fully populated once the live-migration + * has run, until then the normalized loader has to be used. + * + * @return RowLoader\RowLoader::LOADER_* */ - private function getRows(array $rowIds, array $columnIds): array { - if (empty($rowIds) || empty($columnIds)) { - return []; - } - - $allRows = []; - foreach (array_chunk($rowIds, self::DB_CHUNK_SIZE) as $rowIdChunk) { - $allRows[] = $this->getRowsChunk($rowIdChunk, $columnIds); - } - return array_merge(...$allRows); + private function getRowLoader(): string { + return $this->appConfig->getValueBool(Application::APP_ID, ConfigLexicon::CACHING_SLEEVE_CELLS_COMPLETE) + ? RowLoader\RowLoader::LOADER_CACHED + : RowLoader\RowLoader::LOADER_NORMALIZED; } /** * @param array $rowIds * @param array $columnIds + * @param RowLoader\RowLoader::LOADER_* $loader * @return Row2[] * @throws InternalError */ - private function getRowsChunk(array $rowIds, array $columnIds): array { - $qb = $this->db->getQueryBuilder(); - - $qbSqlForColumnTypes = null; - foreach ($this->columnsHelper->columns as $columnType) { - $qbTmp = $this->db->getQueryBuilder(); - $qbTmp->select('row_id', 'column_id', 'last_edit_at', 'last_edit_by') - ->selectAlias($qb->expr()->castColumn('value', IQueryBuilder::PARAM_STR), 'value'); - - // This is not ideal but I cannot think of a good way to abstract this away into the mapper right now - // Ideally we dynamically construct this query depending on what additional selects the column type requires - // however the union requires us to match the exact number of selects for each column type - if ($columnType === Column::TYPE_USERGROUP) { - $qbTmp->selectAlias($qb->expr()->castColumn('value_type', IQueryBuilder::PARAM_STR), 'value_type'); - } else { - $qbTmp->selectAlias($qbTmp->createFunction('NULL'), 'value_type'); - } - - $qbTmp - ->from('tables_row_cells_' . $columnType) - ->where($qb->expr()->in('column_id', $qb->createNamedParameter($columnIds, IQueryBuilder::PARAM_INT_ARRAY, ':columnIds'))) - ->andWhere($qb->expr()->in('row_id', $qb->createNamedParameter($rowIds, IQueryBuilder::PARAM_INT_ARRAY, ':rowsIds'))); - - if ($qbSqlForColumnTypes) { - $qbSqlForColumnTypes .= ' UNION ALL ' . $qbTmp->getSQL() . ' '; - } else { - $qbSqlForColumnTypes = '(' . $qbTmp->getSQL(); - } + private function getRows(array $rowIds, array $columnIds, string $loader = RowLoader\RowLoader::LOADER_NORMALIZED): array { + if (empty($rowIds) || empty($columnIds)) { + return []; } - $qbSqlForColumnTypes .= ')'; - - $qb->select('row_id', 'column_id', 'created_by', 'created_at', 't1.last_edit_by', 't1.last_edit_at', 'value', 'table_id') - // Also should be more generic (see above) - ->addSelect('value_type') - ->from($qb->createFunction($qbSqlForColumnTypes), 't1') - ->innerJoin('t1', 'tables_row_sleeves', 'rs', 'rs.id = t1.row_id'); - try { - $result = $qb->executeQuery(); - } catch (Exception $e) { - $this->logger->error($e->getMessage(), ['exception' => $e]); - throw new InternalError(static::class . ' - ' . __FUNCTION__ . ': ' . $e->getMessage(), ); + $columns = $mappers = []; + foreach ($columnIds as $columnId) { + // meta columns are not stored in tables_columns, their values are + // taken from the row sleeve fields by the loaders + if ($columnId < 0) { + continue; + } + $columns[$columnId] = $this->columnMapper->find($columnId); + $mappers[$columnId] = $this->getCellMapper($columns[$columnId]); } - try { - $sleeves = $this->rowSleeveMapper->findMultiple($rowIds); - } catch (Exception $e) { - $this->logger->error($e->getMessage(), ['exception' => $e]); - throw new InternalError(static::class . ' - ' . __FUNCTION__ . ': ' . $e->getMessage()); + $rows = []; + foreach ($this->rowLoaders[$loader]->getRows($rowIds, $columns, $mappers) as $row) { + $rows[] = $row; } - - return $this->parseEntities($result, $sleeves); + return $rows; } /** @@ -711,59 +687,6 @@ private function getSqlOperator(string $operator, IQueryBuilder $qb, string $col }; } - /** - * @param IResult $result - * @param RowSleeve[] $sleeves - * @return Row2[] - * @throws InternalError - */ - private function parseEntities(IResult $result, array $sleeves): array { - $rows = []; - foreach ($sleeves as $sleeve) { - $rows[$sleeve->getId()] = new Row2(); - $rows[$sleeve->getId()]->setId($sleeve->getId()); - $rows[$sleeve->getId()]->setCreatedBy($sleeve->getCreatedBy()); - $rows[$sleeve->getId()]->setCreatedAt($sleeve->getCreatedAt()); - $rows[$sleeve->getId()]->setLastEditBy($sleeve->getLastEditBy()); - $rows[$sleeve->getId()]->setLastEditAt($sleeve->getLastEditAt()); - $rows[$sleeve->getId()]->setTableId($sleeve->getTableId()); - } - - $rowValues = []; - $keyToColumnId = []; - $keyToRowId = []; - $cellMapperCache = []; - - while ($rowData = $result->fetchAssociative()) { - if (!isset($rowData['row_id'], $rows[$rowData['row_id']])) { - break; - } - - $column = $this->columnMapper->find($rowData['column_id']); - $columnType = $column->getType(); - $cellMapperCache[$columnType] ??= $this->getCellMapperFromType($columnType); - $value = $cellMapperCache[$columnType]->formatRowData($column, $rowData); - $compositeKey = (string)$rowData['row_id'] . ',' . (string)$rowData['column_id']; - if ($cellMapperCache[$columnType]->hasMultipleValues()) { - if (array_key_exists($compositeKey, $rowValues)) { - $rowValues[$compositeKey][] = $value; - } else { - $rowValues[$compositeKey] = [$value]; - } - } else { - $rowValues[$compositeKey] = $value; - } - $keyToColumnId[$compositeKey] = $rowData['column_id']; - $keyToRowId[$compositeKey] = $rowData['row_id']; - } - - foreach ($rowValues as $compositeKey => $value) { - $rows[$keyToRowId[$compositeKey]]->addCell($keyToColumnId[$compositeKey], $value); - } - - return array_values($rows); - } - /** * @throws InternalError */ @@ -782,21 +705,27 @@ public function insert(Row2 $row, ?string $userId = null): Row2 { if ($userId) { $this->userId = $userId; } - if ($row->getId()) { - // if row has an id from migration or import etc. - $rowSleeve = $this->createRowSleeveFromExistingData($row->getId(), $row->getTableId(), $row->getCreatedAt(), $row->getCreatedBy(), $row->getLastEditBy(), $row->getLastEditAt()); - } else { - // create a new row sleeve to get a new rowId - $rowSleeve = $this->createNewRowSleeve($row->getTableId()); - $row->setId($rowSleeve->getId()); - } - // write all cells to its db-table - foreach ($row->getData() as $cell) { - $this->insertCell($rowSleeve->getId(), $cell['columnId'], $cell['value'], $rowSleeve->getLastEditAt(), $rowSleeve->getLastEditBy()); - } + return $this->atomic(function () use ($row): Row2 { + if ($row->getId()) { + // if row has an id from migration or import etc. + $rowSleeve = $this->createRowSleeveFromExistingData($row->getId(), $row->getTableId(), $row->getCreatedAt(), $row->getCreatedBy(), $row->getLastEditBy(), $row->getLastEditAt()); + } else { + // create a new row sleeve to get a new rowId + $rowSleeve = $this->createNewRowSleeve($row->getTableId()); + $row->setId($rowSleeve->getId()); + } - return $row; + // write all cells to its db-table + $cachedCells = []; + foreach ($row->getData() as $cell) { + $cachedCells[$cell['columnId']] = $this->insertCell($rowSleeve->getId(), $cell['columnId'], $cell['value'], $rowSleeve->getLastEditAt(), $rowSleeve->getLastEditBy()); + } + $rowSleeve->setCachedCellsArray($cachedCells); + $this->rowSleeveMapper->update($rowSleeve); + + return $row; + }, $this->db); } /** @@ -815,24 +744,36 @@ public function update(Row2 $row, ?string $userId = null): Row2 { return $row; } - // update meta data for sleeve - try { - $sleeve = $this->rowSleeveMapper->find($row->getId()); - $this->updateMetaData($sleeve); - $this->rowSleeveMapper->update($sleeve); - } catch (DoesNotExistException|MultipleObjectsReturnedException|Exception $e) { - $this->logger->error($e->getMessage(), ['exception' => $e]); - throw new InternalError(static::class . ' - ' . __FUNCTION__ . ': ' . $e->getMessage()); - } + return $this->atomic(function () use ($row, $changedCells): Row2 { + // update meta data for sleeve + try { + $sleeve = $this->rowSleeveMapper->findForUpdate($row->getId()); + $this->updateMetaData($sleeve); + } catch (DoesNotExistException|MultipleObjectsReturnedException|Exception $e) { + $this->logger->error($e->getMessage(), ['exception' => $e]); + throw new InternalError(static::class . ' - ' . __FUNCTION__ . ': ' . $e->getMessage()); + } - $this->columnMapper->preloadColumns(array_column($changedCells, 'columnId')); + $this->columnMapper->preloadColumns(array_column($changedCells, 'columnId')); - // write all changed cells to its db-table - foreach ($changedCells as $cell) { - $this->insertOrUpdateCell($sleeve->getId(), $cell['columnId'], $cell['value']); - } + // write all changed cells to its db-table + $cachedCells = $sleeve->getCachedCellsArray(); + if ($sleeve->getCachedCells() === null) { + // the row is not yet covered by the live-migration, so the cache has + // to be built completely instead of only holding the changed cells + $cachedCells = $this->columnsHelper->getCachedCellsForRow( + $sleeve->getId(), + $this->columnMapper->findAllByTable($sleeve->getTableId()) + ); + } + foreach ($changedCells as $cell) { + $cachedCells[$cell['columnId']] = $this->insertOrUpdateCell($sleeve->getId(), $cell['columnId'], $cell['value']); + } + $sleeve->setCachedCellsArray($cachedCells); + $this->rowSleeveMapper->update($sleeve); - return $row; + return $row; + }, $this->db); } /** @@ -882,9 +823,11 @@ private function updateMetaData($entity, bool $setCreate = false, ?string $lastE /** * Insert a cell to its specific db-table * + * @return array normalized cell data + * * @throws InternalError */ - private function insertCell(int $rowId, int $columnId, $value, ?string $lastEditAt = null, ?string $lastEditBy = null): void { + private function insertCell(int $rowId, int $columnId, $value, ?string $lastEditAt = null, ?string $lastEditBy = null): array { try { $column = $this->columnMapper->find($columnId); } catch (DoesNotExistException $e) { @@ -895,6 +838,7 @@ private function insertCell(int $rowId, int $columnId, $value, ?string $lastEdit // insert new cell $cellMapper = $this->getCellMapper($column); + $cachedCell = []; try { $cellClassName = 'OCA\Tables\Db\RowCell' . ucfirst($column->getType()); if ($cellMapper->hasMultipleValues()) { @@ -906,6 +850,7 @@ private function insertCell(int $rowId, int $columnId, $value, ?string $lastEdit $this->updateMetaData($cell, false, $lastEditAt, $lastEditBy); $cellMapper->applyDataToEntity($column, $cell, $val); $cellMapper->insert($cell); + $cachedCell[] = $cellMapper->toArray($cell); } } else { /** @var RowCellSuper $cell */ @@ -915,11 +860,14 @@ private function insertCell(int $rowId, int $columnId, $value, ?string $lastEdit $this->updateMetaData($cell, false, $lastEditAt, $lastEditBy); $cellMapper->applyDataToEntity($column, $cell, $value); $cellMapper->insert($cell); + $cachedCell = $cellMapper->toArray($cell); } } catch (Exception $e) { $this->logger->error($e->getMessage(), ['exception' => $e]); throw new InternalError('Failed to insert column: ' . $e->getMessage(), 0, $e); } + + return $cachedCell; } /** @@ -927,53 +875,50 @@ private function insertCell(int $rowId, int $columnId, $value, ?string $lastEdit * @param RowCellMapperSuper $mapper * @param mixed $value the value should be parsed to the correct format within the row service * @param Column $column + * + * @return array normalized cell data * @throws InternalError */ - private function updateCell(RowCellSuper $cell, RowCellMapperSuper $mapper, $value, Column $column): void { - $this->getCellMapper($column)->applyDataToEntity($column, $cell, $value); + private function updateCell(RowCellSuper $cell, RowCellMapperSuper $mapper, $value, Column $column): array { + $cellMapper = $this->getCellMapper($column); + $cellMapper->applyDataToEntity($column, $cell, $value); $this->updateMetaData($cell); $mapper->updateWrapper($cell); + + return $cellMapper->toArray($cell); } /** + * @return array normalized cell data * @throws InternalError */ - private function insertOrUpdateCell(int $rowId, int $columnId, $value): void { + private function insertOrUpdateCell(int $rowId, int $columnId, $value): array { $column = $this->columnMapper->find($columnId); $cellMapper = $this->getCellMapper($column); + $cachedCell = []; try { if ($cellMapper->hasMultipleValues()) { - $this->atomic(function () use ($cellMapper, $rowId, $columnId, $value): void { + $this->atomic(function () use ($cellMapper, $rowId, $columnId, $value, &$cachedCell): void { // For a usergroup field with mutiple values, each is inserted as a new cell // we need to delete all previous cells for this row and column, otherwise we get duplicates $cellMapper->deleteAllForColumnAndRow($columnId, $rowId); - $this->insertCell($rowId, $columnId, $value); + $cachedCell = $this->insertCell($rowId, $columnId, $value); }, $this->db); } else { $cell = $cellMapper->findByRowAndColumn($rowId, $columnId); - $this->updateCell($cell, $cellMapper, $value, $column); + $cachedCell = $this->updateCell($cell, $cellMapper, $value, $column); } } catch (DoesNotExistException) { - $this->insertCell($rowId, $columnId, $value); + $cachedCell = $this->insertCell($rowId, $columnId, $value); } catch (MultipleObjectsReturnedException|Exception $e) { $this->logger->error($e->getMessage(), ['exception' => $e]); - throw new InternalError(static::class . ' - ' . __FUNCTION__ . ': ' . $e->getMessage()); + throw new InternalError(static::class . ' - ' . __FUNCTION__ . ': ' . $e->getMessage(), $e->getCode(), $e); } + return $cachedCell; } private function getCellMapper(Column $column): RowCellMapperSuper { - return $this->getCellMapperFromType($column->getType()); - } - - private function getCellMapperFromType(string $columnType): RowCellMapperSuper { - $cellMapperClassName = 'OCA\Tables\Db\RowCell' . ucfirst($columnType) . 'Mapper'; - /** @var RowCellMapperSuper $cellMapper */ - try { - return Server::get($cellMapperClassName); - } catch (NotFoundExceptionInterface|ContainerExceptionInterface $e) { - $this->logger->error($e->getMessage(), ['exception' => $e]); - throw new InternalError(static::class . ' - ' . __FUNCTION__ . ': ' . $e->getMessage()); - } + return $this->columnsHelper->getCellMapperFromType($column->getType()); } /** @@ -984,11 +929,19 @@ private function getColumnDbParamType(Column $column): int { } /** + * @param bool $purgeCachedCells also remove the column's data from the + * cached_cells of all row sleeves (can be skipped if the sleeves are + * deleted anyway, e.g. when deleting the whole table) * @throws InternalError */ - public function deleteDataForColumn(Column $column): void { + public function deleteDataForColumn(Column $column, bool $purgeCachedCells = true): void { try { - $this->getCellMapper($column)->deleteAllForColumn($column->getId()); + $this->atomic(function () use ($column, $purgeCachedCells): void { + $this->getCellMapper($column)->deleteAllForColumn($column->getId()); + if ($purgeCachedCells) { + $this->rowSleeveMapper->removeCachedCellsForColumn($column->getTableId(), $column->getId()); + } + }, $this->db); } catch (Exception $e) { $this->logger->error($e->getMessage(), ['exception' => $e]); throw new InternalError(static::class . ' - ' . __FUNCTION__ . ': ' . $e->getMessage()); @@ -1003,7 +956,7 @@ public function deleteDataForColumn(Column $column): void { public function deleteAllForTable(int $tableId, array $columns): void { foreach ($columns as $column) { try { - $this->deleteDataForColumn($column); + $this->deleteDataForColumn($column, purgeCachedCells: false); } catch (InternalError $e) { $this->logger->error($e->getMessage(), ['exception' => $e]); } diff --git a/lib/Db/RowCellMapperSuper.php b/lib/Db/RowCellMapperSuper.php index 460e2095e8..dc13a34b9a 100644 --- a/lib/Db/RowCellMapperSuper.php +++ b/lib/Db/RowCellMapperSuper.php @@ -52,6 +52,10 @@ public function applyDataToEntity(Column $column, RowCellSuper $cell, $data): vo $cell->setValue($data); } + public function toArray(RowCellSuper $cell): array { + return ['value' => $cell->getValue()]; + } + public function getDbParamType() { return IQueryBuilder::PARAM_STR; } @@ -116,6 +120,33 @@ public function findByRowAndColumn(int $rowId, int $columnId): RowCellSuper { return $this->findEntity($qb); } + /** + * @throws MultipleObjectsReturnedException + * @throws DoesNotExistException + * @throws Exception + * @return RowCellSuper[] + */ + public function findManyByRowAndColumn(int $rowId, int $columnId): array { + $qb = $this->db->getQueryBuilder(); + $qb->select('*') + ->from($this->tableName) + ->where($qb->expr()->eq('row_id', $qb->createNamedParameter($rowId, IQueryBuilder::PARAM_INT))) + ->andWhere($qb->expr()->eq('column_id', $qb->createNamedParameter($columnId, IQueryBuilder::PARAM_INT))); + return $this->findEntities($qb); + } + + /** + * @throws Exception + * @return RowCellSuper[] + */ + public function findAllForRow(int $rowId): array { + $qb = $this->db->getQueryBuilder(); + $qb->select('*') + ->from($this->tableName) + ->where($qb->expr()->eq('row_id', $qb->createNamedParameter($rowId, IQueryBuilder::PARAM_INT))); + return $this->findEntities($qb); + } + /** * @throws MultipleObjectsReturnedException * @throws DoesNotExistException diff --git a/lib/Db/RowCellUsergroupMapper.php b/lib/Db/RowCellUsergroupMapper.php index af975a8c37..2ea15230d0 100644 --- a/lib/Db/RowCellUsergroupMapper.php +++ b/lib/Db/RowCellUsergroupMapper.php @@ -60,6 +60,13 @@ public function formatRowData(Column $column, array $row) { ]; } + public function toArray(RowCellSuper $cell): array { + return [ + 'value' => $cell->getValue(), + 'value_type' => $cell->getValueType(), + ]; + } + public function hasMultipleValues(): bool { return true; } diff --git a/lib/Db/RowLoader/CachedRowLoader.php b/lib/Db/RowLoader/CachedRowLoader.php new file mode 100644 index 0000000000..e0aafa408f --- /dev/null +++ b/lib/Db/RowLoader/CachedRowLoader.php @@ -0,0 +1,94 @@ +getRowsChunk($chunkedRowIds, $columns, $mappers); + } + } + + /** + * @param array $rowIds + * @param array $columns Column per columnId + * @param array $mappers Mapper per columnId + * @return Row2[] + * @throws InternalError + */ + private function getRowsChunk(array $rowIds, array $columns, array $mappers): array { + try { + $sleeves = $this->rowSleeveMapper->findMultiple($rowIds); + } catch (Exception $e) { + $this->logger->error($e->getMessage(), ['exception' => $e]); + throw new InternalError(static::class . ' - ' . __FUNCTION__ . ': ' . $e->getMessage(), $e->getCode(), $e); + } + + return $this->parseResult($sleeves, $columns, $mappers); + } + + /** + * @param list> $sleeves + * @param array $columns Column per columnId + * @param array $mappers Mapper per columnId + * @return Row2[] + */ + private function parseResult(array $sleeves, array $columns, $mappers): array { + $rows = []; + foreach ($sleeves as $sleeve) { + $id = (int)$sleeve['id']; + $row = new Row2(); + $row->setId($id); + $row->setCreatedBy($sleeve['created_by']); + $row->setCreatedAt($sleeve['created_at']); + $row->setLastEditBy($sleeve['last_edit_by']); + $row->setLastEditAt($sleeve['last_edit_at']); + $row->setTableId((int)$sleeve['table_id']); + + $cachedCells = json_decode($sleeve['cached_cells'] ?? '{}', true); + foreach ($columns as $columnId => $column) { + if (empty($cachedCells[$columnId])) { + continue; + } + + $cellMapper = $mappers[$columnId]; + if ($cellMapper->hasMultipleValues()) { + $value = array_map(static fn ($rowData) => $cellMapper->formatRowData($column, $rowData), + $cachedCells[$columnId]); + } else { + $value = $cellMapper->formatRowData($column, $cachedCells[$columnId]); + } + + $row->addCell($columnId, $value); + } + + $rows[] = $row; + } + + return $rows; + } +} diff --git a/lib/Db/RowLoader/NormalizedRowLoader.php b/lib/Db/RowLoader/NormalizedRowLoader.php new file mode 100644 index 0000000000..54f7dd51a9 --- /dev/null +++ b/lib/Db/RowLoader/NormalizedRowLoader.php @@ -0,0 +1,157 @@ +columnsHelper->columns)); + $chunkSize = max(1, $maxParametersPerType - count($columns)); + + foreach (array_chunk($rowIds, $chunkSize) as $chunkedRowIds) { + yield from $this->getRowsChunk($chunkedRowIds, $columns, $mappers); + } + } + + /** + * Builds and executes the UNION ALL query for a specific chunk of rows. + * @param array $rowIds + * @param array $columns Column per columnId + * @param array $mappers Mapper per columnId + * @return Row2[] + * @throws InternalError + */ + private function getRowsChunk(array $rowIds, array $columns, array $mappers): array { + try { + $sleeves = $this->rowSleeveMapper->findMultiple($rowIds); + } catch (Exception $e) { + $this->logger->error($e->getMessage(), ['exception' => $e]); + throw new InternalError(static::class . ' - ' . __FUNCTION__ . ': ' . $e->getMessage(), $e->getCode(), $e); + } + + if (empty($columns)) { + // only meta columns were requested, there are no cells to load + return $this->parseResult(null, $sleeves, $columns, $mappers); + } + + $qb = $this->db->getQueryBuilder(); + + $subqueries = []; + foreach ($this->columnsHelper->columns as $columnType) { + $qbValues = $this->db->getQueryBuilder(); + $qbValues->select('row_id', 'column_id') + ->selectAlias($qb->expr()->castColumn('value', IQueryBuilder::PARAM_STR), 'value'); + + // This is not ideal but I cannot think of a good way to abstract this away into the mapper right now + // Ideally we dynamically construct this query depending on what additional selects the column type requires + // however the union requires us to match the exact number of selects for each column type + if ($columnType === Column::TYPE_USERGROUP) { + $qbValues->selectAlias($qb->expr()->castColumn('value_type', IQueryBuilder::PARAM_STR), 'value_type'); + } else { + $qbValues->selectAlias($qbValues->createFunction('NULL'), 'value_type'); + } + + $qbValues + ->from('tables_row_cells_' . $columnType) + ->where($qb->expr()->in('column_id', $qb->createNamedParameter(array_keys($columns), IQueryBuilder::PARAM_INT_ARRAY, ':columnIds'))) + ->andWhere($qb->expr()->in('row_id', $qb->createNamedParameter($rowIds, IQueryBuilder::PARAM_INT_ARRAY, ':rowsIds'))); + + $subqueries[] = $qbValues->getSQL(); + } + + $qb->select('row_id', 'column_id', 'value', 'value_type') + ->from($qb->createFunction('(' . implode(' UNION ALL ', $subqueries) . ')'), 't1'); + + try { + $result = $qb->executeQuery(); + } catch (Exception $e) { + $this->logger->error($e->getMessage(), ['exception' => $e]); + throw new InternalError(static::class . ' - ' . __FUNCTION__ . ': ' . $e->getMessage(), $e->getCode(), $e); + } + + return $this->parseResult($result, $sleeves, $columns, $mappers); + } + + /** + * @param IResult|null $result + * @param list> $sleeves + * @param array $columns Column per columnId + * @param array $mappers Mapper per columnId + * @return Row2[] + * @throws InternalError + */ + private function parseResult(?IResult $result, array $sleeves, array $columns, array $mappers): array { + $rows = []; + foreach ($sleeves as $sleeve) { + $id = (int)$sleeve['id']; + $rows[$id] = new Row2(); + $rows[$id]->setId($id); + $rows[$id]->setCreatedBy($sleeve['created_by']); + $rows[$id]->setCreatedAt($sleeve['created_at']); + $rows[$id]->setLastEditBy($sleeve['last_edit_by']); + $rows[$id]->setLastEditAt($sleeve['last_edit_at']); + $rows[$id]->setTableId((int)$sleeve['table_id']); + } + + $rowValues = []; + $keyToColumnId = []; + $keyToRowId = []; + + while ($result !== null && ($rowData = $result->fetchAssociative())) { + if (!isset($rowData['row_id'], $rows[$rowData['row_id']])) { + continue; + } + + $column = $columns[$rowData['column_id']]; + $cellMapper = $mappers[$rowData['column_id']]; + $value = $cellMapper->formatRowData($column, $rowData); + $compositeKey = (string)$rowData['row_id'] . ',' . (string)$rowData['column_id']; + if ($cellMapper->hasMultipleValues()) { + if (array_key_exists($compositeKey, $rowValues)) { + $rowValues[$compositeKey][] = $value; + } else { + $rowValues[$compositeKey] = [$value]; + } + } else { + $rowValues[$compositeKey] = $value; + } + $keyToColumnId[$compositeKey] = $rowData['column_id']; + $keyToRowId[$compositeKey] = $rowData['row_id']; + } + + foreach ($rowValues as $compositeKey => $value) { + $rows[$keyToRowId[$compositeKey]]->addCell($keyToColumnId[$compositeKey], $value); + } + + return array_values($rows); + } +} diff --git a/lib/Db/RowLoader/RowLoader.php b/lib/Db/RowLoader/RowLoader.php new file mode 100644 index 0000000000..a481a139fe --- /dev/null +++ b/lib/Db/RowLoader/RowLoader.php @@ -0,0 +1,31 @@ + $columns Column per columnId + * @param array $mappers Mapper per columnId + * @return iterable + * @throws InternalError + */ + public function getRows(array $rowIds, array $columns, array $mappers): iterable; +} diff --git a/lib/Db/RowSleeve.php b/lib/Db/RowSleeve.php index b909a3f44f..75fa362b96 100644 --- a/lib/Db/RowSleeve.php +++ b/lib/Db/RowSleeve.php @@ -14,6 +14,8 @@ * @psalm-suppress PropertyNotSetInConstructor * @method getTableId(): ?int * @method setTableId(int $columnId) + * @method getCachedCells(): ?string + * @method setCachedCells(string $cachedCells) * @method getCreatedBy(): string * @method setCreatedBy(string $createdBy) * @method getCreatedAt(): string @@ -25,6 +27,7 @@ */ class RowSleeve extends Entity implements JsonSerializable { protected ?int $tableId = null; + protected ?string $cachedCells = null; protected ?string $createdBy = null; protected ?string $createdAt = null; protected ?string $lastEditBy = null; @@ -33,6 +36,24 @@ class RowSleeve extends Entity implements JsonSerializable { public function __construct() { $this->addType('id', 'integer'); $this->addType('tableId', 'integer'); + $this->addType('cachedCells', 'string'); + } + + /** + * @return array Indexed by column ID + */ + public function getCachedCellsArray(): array { + if ($this->cachedCells === null) { + return []; + } + return json_decode($this->cachedCells, true, flags: JSON_THROW_ON_ERROR) ?: []; + } + + /** + * @param array $cachedCells Indexed by column ID + */ + public function setCachedCellsArray(array $cachedCells): void { + $this->setCachedCells(json_encode($cachedCells, JSON_THROW_ON_ERROR)); } public function jsonSerialize(): array { diff --git a/lib/Db/RowSleeveMapper.php b/lib/Db/RowSleeveMapper.php index b627424ba5..c7ce47c408 100644 --- a/lib/Db/RowSleeveMapper.php +++ b/lib/Db/RowSleeveMapper.php @@ -41,9 +41,25 @@ public function find(int $id): RowSleeve { return $this->findEntity($qb); } + /** + * @throws MultipleObjectsReturnedException + * @throws DoesNotExistException + * @throws Exception + */ + public function findForUpdate(int $id): RowSleeve { + $qb = $this->db->getQueryBuilder(); + $qb->select('*') + ->from($this->table) + ->where($qb->expr()->eq('id', $qb->createNamedParameter($id, IQueryBuilder::PARAM_INT))); + if ($this->db->getDatabaseProvider() !== IDBConnection::PLATFORM_SQLITE) { + $qb->forUpdate(); + } + return $this->findEntity($qb); + } + /** * @param int[] $ids - * @return RowSleeve[] + * @return list> Raw data from DB for the best performance * @throws Exception */ public function findMultiple(array $ids): array { @@ -52,6 +68,7 @@ public function findMultiple(array $ids): array { $qb->select( $sleeveAlias . '.id', $sleeveAlias . '.table_id', + $sleeveAlias . '.cached_cells', $sleeveAlias . '.created_by', $sleeveAlias . '.created_at', $sleeveAlias . '.last_edit_by', @@ -59,7 +76,8 @@ public function findMultiple(array $ids): array { ) ->from($this->table, $sleeveAlias) ->where($qb->expr()->in($sleeveAlias . '.id', $qb->createNamedParameter($ids, IQueryBuilder::PARAM_INT_ARRAY))); - return $this->findEntities($qb); + + return $qb->executeQuery()->fetchAllAssociative(); } /** @@ -91,6 +109,30 @@ public function deleteById(int $sleeveId) { $qb->executeStatement(); } + /** + * Removes the cells of a single column from the cached_cells of all rows in a table + * + * @param int $tableId + * @param int $columnId + * @throws Exception + */ + public function removeCachedCellsForColumn(int $tableId, int $columnId): void { + $qb = $this->db->getQueryBuilder(); + if ($this->db->getDatabaseProvider() === IDBConnection::PLATFORM_POSTGRES) { + $expression = '(cached_cells::jsonb - :columnKey)::text'; + $qb->setParameter('columnKey', (string)$columnId, IQueryBuilder::PARAM_STR); + } else { // mysql / mariadb / sqlite + $expression = 'JSON_REMOVE(cached_cells, :columnKey)'; + $qb->setParameter('columnKey', '$."' . $columnId . '"', IQueryBuilder::PARAM_STR); + } + + $qb->update($this->table) + ->set('cached_cells', $qb->createFunction($expression)) + ->where($qb->expr()->eq('table_id', $qb->createNamedParameter($tableId, IQueryBuilder::PARAM_INT))) + ->andWhere($qb->expr()->like('cached_cells', $qb->createNamedParameter('%"' . $columnId . '":%'))); + $qb->executeStatement(); + } + /** * @param int $tableId * @return int Effected rows diff --git a/lib/Helper/ColumnsHelper.php b/lib/Helper/ColumnsHelper.php index 2ebe0e254d..9e35f3bc89 100644 --- a/lib/Helper/ColumnsHelper.php +++ b/lib/Helper/ColumnsHelper.php @@ -9,11 +9,15 @@ use OCA\Tables\Constants\UsergroupType; use OCA\Tables\Db\Column; +use OCA\Tables\Db\RowCellMapperSuper; +use OCA\Tables\Errors\InternalError; use OCA\Tables\Service\ColumnTypes\IColumnTypeBusiness; use OCP\Server; +use Psr\Container\ContainerExceptionInterface; +use Psr\Container\NotFoundExceptionInterface; +use Psr\Log\LoggerInterface; class ColumnsHelper { - public array $columns = [ Column::TYPE_TEXT, Column::TYPE_NUMBER, @@ -28,9 +32,15 @@ class ColumnsHelper { */ private array $columnBusinesses = []; + /** + * @var array + */ + private array $cellMappers = []; + public function __construct( private readonly UserHelper $userHelper, private readonly CircleHelper $circleHelper, + private readonly LoggerInterface $logger, ) { } @@ -104,4 +114,43 @@ public function getColumnBusinessObject(Column $column): IColumnTypeBusiness { return $this->columnBusinesses[$cacheKey] = $columnBusiness; } + + public function getCellMapperFromType(string $columnType): RowCellMapperSuper { + if (isset($this->cellMappers[$columnType])) { + return $this->cellMappers[$columnType]; + } + + $cellMapperClassName = 'OCA\Tables\Db\RowCell' . ucfirst($columnType) . 'Mapper'; + /** @var RowCellMapperSuper $cellMapper */ + try { + $cellMapper = Server::get($cellMapperClassName); + } catch (NotFoundExceptionInterface|ContainerExceptionInterface $e) { + $this->logger->error($e->getMessage(), ['exception' => $e]); + throw new InternalError(static::class . ' - ' . __FUNCTION__ . ': ' . $e->getMessage(), previous: $e); + } + + return $this->cellMappers[$columnType] = $cellMapper; + } + + /** + * Collects all cells of a row in the format used for the cached_cells column + * + * @param Column[] $columns + * @return array + */ + public function getCachedCellsForRow(int $rowId, array $columns): array { + $cachedCells = []; + foreach ($columns as $column) { + $cellMapper = $this->getCellMapperFromType($column->getType()); + $columnId = $column->getId(); + foreach ($cellMapper->findManyByRowAndColumn($rowId, $columnId) as $cell) { + if ($cellMapper->hasMultipleValues()) { + $cachedCells[$columnId][] = $cellMapper->toArray($cell); + } else { + $cachedCells[$columnId] = $cellMapper->toArray($cell); + } + } + } + return $cachedCells; + } } diff --git a/lib/Migration/CacheSleeveCells.php b/lib/Migration/CacheSleeveCells.php new file mode 100644 index 0000000000..610ed75795 --- /dev/null +++ b/lib/Migration/CacheSleeveCells.php @@ -0,0 +1,115 @@ +appConfig->getValueBool(Application::APP_ID, ConfigLexicon::CACHING_SLEEVE_CELLS_COMPLETE)) { + return; + } + + $pendingRowsQuery = $this->buildPendingRowIdsQuery(); + foreach ($this->getTableIds() as $tableId) { + $pendingRowsQuery->setParameter('tableId', $tableId, IQueryBuilder::PARAM_INT); + $columns = $this->columnMapper->findAllByTable($tableId); + + while ($rowIds = $this->fetchPendingRowIds($pendingRowsQuery)) { + foreach ($rowIds as $rowId) { + $this->cacheCellsForRow($rowId, $columns); + } + } + + $this->logger->info('Finished caching cells for table ' . $tableId); + } + + $this->appConfig->setValueBool(Application::APP_ID, ConfigLexicon::CACHING_SLEEVE_CELLS_COMPLETE, true); + } + + /** + * @param Column[] $columns + */ + private function cacheCellsForRow(int $rowId, array $columns): void { + try { + $sleeve = $this->rowSleeveMapper->find($rowId); + } catch (DoesNotExistException) { + // the row was deleted while the migration is running + return; + } + + $cachedCells = $this->columnsHelper->getCachedCellsForRow($rowId, $columns); + $sleeve->setCachedCellsArray($cachedCells); + $this->rowSleeveMapper->update($sleeve); + } + + /** + * @return int[] + */ + private function getTableIds(): array { + return $this->db->getQueryBuilder() + ->select('id') + ->from('tables_tables') + ->orderBy('id') + ->executeQuery()->fetchFirstColumn(); + } + + /** + * @return int[] + */ + private function fetchPendingRowIds(IQueryBuilder $pendingRowsQuery): array { + return $pendingRowsQuery->executeQuery()->fetchFirstColumn(); + } + + private function buildPendingRowIdsQuery(): IQueryBuilder { + $qb = $this->db->getQueryBuilder(); + + return $qb->select('id') + ->from('tables_row_sleeves') + ->where($qb->expr()->isNull('cached_cells')) + ->andWhere($qb->expr()->eq('table_id', $qb->createParameter('tableId'))) + ->orderBy('id') + ->setMaxResults(self::ROW_BATCH_SIZE); + } +} diff --git a/lib/Migration/Version2232Date20260906000000.php b/lib/Migration/Version2232Date20260906000000.php new file mode 100644 index 0000000000..c6d9702559 --- /dev/null +++ b/lib/Migration/Version2232Date20260906000000.php @@ -0,0 +1,41 @@ +getTable('tables_row_sleeves'); + if (!$table->hasColumn('cached_cells')) { + $table->addColumn('cached_cells', Types::TEXT, [ + 'notnull' => false, + ]); + } + + return $schema; + } +} diff --git a/lib/UserMigration/TablesMigrator.php b/lib/UserMigration/TablesMigrator.php index ec4d528307..8c611ba8e8 100644 --- a/lib/UserMigration/TablesMigrator.php +++ b/lib/UserMigration/TablesMigrator.php @@ -27,6 +27,7 @@ use OCA\Tables\Db\Table; use OCA\Tables\Db\TableMapper; use OCA\Tables\Db\ViewMapper; +use OCA\Tables\Helper\ColumnsHelper; use OCA\Tables\Model\ViewSettings; use OCA\Tables\Service\ColumnService; use OCA\Tables\Service\ContextService; @@ -42,6 +43,7 @@ use OCP\UserMigration\IMigrator; use OCP\UserMigration\ISizeEstimationMigrator; use OCP\UserMigration\TMigratorBasicVersionHandling; +use Psr\Log\LoggerInterface; use Symfony\Component\Console\Output\OutputInterface; class TablesMigrator implements IMigrator, ISizeEstimationMigrator { @@ -68,6 +70,7 @@ public function __construct( protected TableMapper $tableMapper, protected ColumnMapper $columnMapper, protected RowSleeveMapper $rowSleeveMapper, + protected ColumnsHelper $columnsHelper, protected ViewMapper $viewMapper, protected ContextMapper $contextMapper, protected ShareMapper $shareMapper, @@ -84,6 +87,7 @@ public function __construct( protected RowService $rowService, private ContextService $contextService, private ShareService $shareService, + protected LoggerInterface $logger, ) { } @@ -280,6 +284,8 @@ public function import( $userId, ); + $this->rebuildCachedCells($rowIdMap); + $connection->commit(); } catch (\Throwable $e) { $connection->rollBack(); @@ -336,6 +342,26 @@ private function importRowCells(array $data, array $rowIdMap, array $columnIdMap } } + private function rebuildCachedCells(array $rowIdMap): void { + $columnsByTable = []; + foreach ($rowIdMap as $newRowId) { + try { + $sleeve = $this->rowSleeveMapper->find($newRowId); + $tableId = $sleeve->getTableId(); + $columnsByTable[$tableId] ??= $this->columnMapper->findAllByTable($tableId); + $cachedCells = $this->columnsHelper->getCachedCellsForRow($newRowId, $columnsByTable[$tableId]); + $sleeve->setCachedCellsArray($cachedCells); + $this->rowSleeveMapper->update($sleeve); + } catch (\Throwable $e) { + $this->logger->error('Failed to load cells for row during cached_cells rebuild', [ + 'rowId' => $newRowId, + 'exception' => $e, + ]); + throw $e; + } + } + } + /** * @param IImportSource $importSource * @param array $tableIdMap diff --git a/tests/integration/features/APIv2.feature b/tests/integration/features/APIv2.feature index 09c5361c22..bb7162b710 100644 --- a/tests/integration/features/APIv2.feature +++ b/tests/integration/features/APIv2.feature @@ -561,8 +561,8 @@ Feature: APIv2 | t2 | table | read | And user "participant1-v2" shares the Context "c1" to "user" "participant2-v2" with permissions "read,create,update,delete,manage" When user "participant2-v2" transfers the Context "c1" to "participant3-v2" - Then the reported status is "403" - + Then the reported status is "403" + @api2 @contexts @contexts-ownership Scenario: Transfer an inaccessible context Given table "Table 1 via api v2" with emoji "👋" exists for user "participant1-v2" as "t1" via v2 @@ -1463,6 +1463,50 @@ Feature: APIv2 | code | wil02 | Then "view" "v1" has exactly these rows "r3,r2,r1" in exactly this order + @api2 @rows @views + Scenario: have a view with meta columns in the column settings + Given as user "participant1-v2" + And table "Table 1 via api v2" with emoji "👋" exists for user "participant1-v2" as "t1" via v2 + And column "statement" exists with following properties + | type | text | + | subtype | line | + | mandatory | 1 | + | description | State your business | + And column "weight" exists with following properties + | type | number | + | mandatory | 1 | + | description | Importance of statement | + And user "participant1-v2" create view "Important Statements" with emoji "⚡️" for "t1" as "v1" + And user "participant1-v2" sets columnSettings "meta-id,statement,weight,meta-created-by" to view "v1" + Then the reported status is "200" + And using "view" "v1" + And user "participant1-v2" creates row "r1" with following values: + | statement | Be yourself; everyone else is already taken. | + | weight | 75 | + And user "participant1-v2" creates row "r2" with following values: + | statement | A room without books is like a body without a soul. | + | weight | 67 | + Then "view" "v1" has exactly these rows "r1,r2" in exactly this order + And the user "participant1-v2" fetches the rows of view "v1" via the UI endpoint, it has exactly these rows "r1,r2" + + @api2 @rows @views + Scenario: have a view with only meta columns in the column settings + Given as user "participant1-v2" + And table "Table 1 via api v2" with emoji "👋" exists for user "participant1-v2" as "t1" via v2 + And column "statement" exists with following properties + | type | text | + | subtype | line | + | mandatory | 1 | + | description | State your business | + And user "participant1-v2" create view "IDs only" with emoji "🆔" for "t1" as "v1" + And user "participant1-v2" sets columnSettings "meta-id,meta-updated-at" to view "v1" + Then the reported status is "200" + And using "table" "t1" + And user "participant1-v2" creates row "r1" with following values: + | statement | Be yourself; everyone else is already taken. | + Then "view" "v1" has exactly these rows "r1" in exactly this order + And the user "participant1-v2" fetches the rows of view "v1" via the UI endpoint, it has exactly these rows "r1" + @views Scenario: have a view with a multiple sort orders Given as user "participant1-v2" diff --git a/tests/integration/features/bootstrap/FeatureContext.php b/tests/integration/features/bootstrap/FeatureContext.php index 37a5fdea98..08a0c8b8e7 100644 --- a/tests/integration/features/bootstrap/FeatureContext.php +++ b/tests/integration/features/bootstrap/FeatureContext.php @@ -852,22 +852,26 @@ protected function sendUpdateViewRequest(string $viewName, array $data): void { ); } + private function resolveColumnId(string $columnAlias): int { + return match ($columnAlias) { + 'meta-id' => -1, + 'meta-created-by' => -2, + 'meta-updated-by' => -3, + 'meta-created-at' => -4, + 'meta-updated-at' => -5, + default => is_numeric($columnAlias) + ? (int)$columnAlias + : $this->collectionManager->getByAlias('column', $columnAlias)['id'], + }; + } + #[When('following sort order is applied to view :viewName:')] public function applySortToView(string $viewName, TableNode $sortOrder): void { $sortData = []; foreach ($sortOrder->getRows() as $row) { - $columnId = match ($row[0]) { - 'meta-id' => -1, - 'meta-created-by' => -2, - 'meta-updated-by' => -3, - 'meta-created-at' => -4, - 'meta-updated-at' => -5, - default => $this->collectionManager->getByAlias('column', $row[0])['id'], - }; - $sortData[] = [ - 'columnId' => $columnId, + 'columnId' => $this->resolveColumnId($row[0]), 'mode' => $row[1] ]; @@ -948,21 +952,10 @@ public function applyColumnsToView(string $user, string $columnList, string $vie $this->setCurrentUser($user); $columns = explode(',', $columnList); - $columnSettings = array_map(function (string $columnAlias, int $index) { - if (is_numeric($columnAlias)) { - return [ - 'columnId' => (int)$columnAlias, - 'order' => $index - ]; - } - - $col = $this->collectionManager->getByAlias('column', $columnAlias); - - return [ - 'columnId' => $col['id'], - 'order' => $index - ]; - }, $columns, array_keys($columns)); + $columnSettings = array_map(fn (string $columnAlias, int $index) => [ + 'columnId' => $this->resolveColumnId($columnAlias), + 'order' => $index + ], $columns, array_keys($columns)); $this->sendUpdateViewRequest($viewAlias, ['columnSettings' => json_encode($columnSettings)]); @@ -3196,6 +3189,34 @@ public function nodeHasExactlyThoseSortedRows(string $nodeType, string $nodeAlia Assert::assertSame($expectedIds, $actualRowIds); } + /** + * @Then the user :user fetches the rows of view :viewAlias via the UI endpoint, it has exactly these rows :rowAliasList + */ + public function viewHasExactlyThoseRowsViaUiEndpoint(string $user, string $viewAlias, string $rowAliasList): void { + $this->setCurrentUser($user); + $view = $this->collectionManager->getByAlias('view', $viewAlias); + + $this->sendRequest( + 'GET', + sprintf('/apps/tables/row/view/%d', $view['id']), + ); + + Assert::assertEquals(200, $this->response->getStatusCode()); + $allRows = $this->getDataFromResponse($this->response); + + $rowAliases = explode(',', $rowAliasList); + $expectedIds = array_map(function ($rowAlias): int { + $row = $this->collectionManager->getByAlias('row', $rowAlias); + return $row['id']; + }, $rowAliases); + sort($expectedIds); + + $actualRowIds = array_map(static fn (array $row): int => (int)$row['id'], $allRows); + sort($actualRowIds); + + Assert::assertSame($expectedIds, $actualRowIds); + } + /** * @When the column :columnName of row :rowAlias has the value :value */ diff --git a/tests/unit/Database/DatabaseTestCase.php b/tests/unit/Database/DatabaseTestCase.php index 4519504a84..ad05894842 100644 --- a/tests/unit/Database/DatabaseTestCase.php +++ b/tests/unit/Database/DatabaseTestCase.php @@ -295,6 +295,7 @@ protected function createTestRowWithData(int $tableId, array $rowData = [], arra if (!empty($cellsData)) { $this->addCellsToRow($rowId, $cellsData, $columnMapping); + $this->updateCachedCells($rowId, $cellsData, $columnMapping); } return $result; @@ -336,6 +337,42 @@ protected function insertCellData(int $rowId, int $columnId, $value): void { $this->insertCellIntoTypeTable($rowId, $columnId, $value, $column['type'], $column['subtype']); } + /** + * Converts a test value to the format stored in cached_cells. + * + * Mirrors the array produced by the cell mappers' toArray() methods. + */ + private function convertCachedCellFormat(int $columnId, $value): array { + $qb = $this->connection->getQueryBuilder(); + $result = $qb->select('type', 'subtype') + ->from('tables_columns') + ->where($qb->expr()->eq('id', $qb->createNamedParameter($columnId))) + ->executeQuery(); + + $column = $result->fetch(); + $result->closeCursor(); + + if (!$column) { + throw new \InvalidArgumentException("Column with ID $columnId not found"); + } + + if ($column['type'] === 'usergroup') { + if (!is_array($value)) { + $value = [$value]; + } + return array_values(array_map(static fn ($entry) => [ + 'value' => (string)($entry['id'] ?? ''), + 'value_type' => (int)($entry['type'] ?? 0), + ], $value)); + } + + if ($column['type'] === 'selection' && $column['subtype'] !== 'check') { + return ['value' => $this->convertSelectionValuesToIds($columnId, $value)]; + } + + return ['value' => $value]; + } + /** * Inserts cell data into the appropriate type-specific table */ @@ -358,6 +395,31 @@ protected function insertCellIntoTypeTable(int $rowId, int $columnId, $value, st $qb->executeStatement(); } + /** + * Updates the cached_cells column for a row + */ + protected function updateCachedCells(int $rowId, array $cellsData, array $columnMapping = []): void { + $cachedCells = []; + foreach ($cellsData as $columnIdentifier => $value) { + // Convert test_ident to actual column ID if mapping is provided + if (is_string($columnIdentifier) && isset($columnMapping[$columnIdentifier])) { + $columnId = $columnMapping[$columnIdentifier]; + } else { + $columnId = $columnIdentifier; + } + + // Format the value as expected by CachedRowLoader (matches cell mapper toArray format) + $cachedCells[$columnId] = $this->convertCachedCellFormat($columnId, $value); + } + + $qb = $this->connection->getQueryBuilder(); + $qb->update('tables_row_sleeves') + ->set('cached_cells', $qb->createNamedParameter(json_encode($cachedCells))) + ->where($qb->expr()->eq('id', $qb->createNamedParameter($rowId))); + + $qb->executeStatement(); + } + /** * Creates a complete test table with columns and rows with data */ diff --git a/tests/unit/Db/Row2MapperTest.php b/tests/unit/Db/Row2MapperTest.php index a5a82b25ab..1838e389eb 100644 --- a/tests/unit/Db/Row2MapperTest.php +++ b/tests/unit/Db/Row2MapperTest.php @@ -11,6 +11,7 @@ use OCA\Tables\Db\Column; use OCA\Tables\Tests\Unit\Database\DatabaseTestCase; +use OCP\DB\QueryBuilder\IQueryBuilder; use PHPUnit\Framework\Attributes\DataProvider; /** @@ -252,6 +253,63 @@ public function testFindAllWithMixedExistingAndNonExistentColumns(): void { $this->assertEquals($expectedAgeOrder, $actualAgeOrder, 'Should sort by Age ASC, then Name ASC'); } + /** + * Until the live-migration populated cached_cells for all rows, rows have to be + * loaded via the normalized loader + */ + public function testFindAllFallsBackToNormalizedLoaderWhileMigrationIsPending(): void { + $this->setupRealColumnMapper(self::$testTableId); + $this->sleeveCellCachingComplete = false; + + // simulate rows that were not processed by the live-migration yet + $qb = $this->connection->getQueryBuilder(); + $qb->update('tables_row_sleeves') + ->set('cached_cells', $qb->createNamedParameter(null, IQueryBuilder::PARAM_NULL)) + ->where($qb->expr()->eq('table_id', $qb->createNamedParameter(self::$testTableId))) + ->executeStatement(); + + $rows = $this->mapper->findAll(self::$testColumnIds, self::$testTableId, null, null, null, null, 'test_user'); + + $this->assertCount(5, $rows, 'Should return all 5 rows'); + $columnMapping = $this->extractTestIdentMapping(self::$testDataResult['columns']); + $actualNameOrder = array_map(fn ($row) => $this->getCellValue($row, $columnMapping['name']), $rows); + $this->assertEquals(['Alice', 'Bob', 'Charlie', 'Diana', 'Eve'], $actualNameOrder); + } + + /** + * Updating a row that was not processed by the live-migration yet has to build + * the complete cached_cells instead of only caching the changed cells + */ + public function testUpdateRebuildsCompleteCacheWhileMigrationIsPending(): void { + $this->setupRealColumnMapper(self::$testTableId); + + $columnMapping = $this->extractTestIdentMapping(self::$testDataResult['columns']); + $rowId = self::$testRowIds[0]; + + // simulate a row that was not processed by the live-migration yet + $qb = $this->connection->getQueryBuilder(); + $qb->update('tables_row_sleeves') + ->set('cached_cells', $qb->createNamedParameter(null, IQueryBuilder::PARAM_NULL)) + ->where($qb->expr()->eq('id', $qb->createNamedParameter($rowId))) + ->executeStatement(); + + $columns = array_map( + fn (int $columnId) => $this->columnMapper->find($columnId), + self::$testColumnIds + ); + $row = $this->mapper->find($rowId, $columns); + $row->markAsLoaded(); + $row->insertOrUpdateCell(['columnId' => $columnMapping['name'], 'value' => 'Alice Updated']); + + $this->mapper->update($row); + + $cachedCells = $this->rowSleeveMapper->find($rowId)->getCachedCellsArray(); + + $this->assertSame('Alice Updated', $cachedCells[$columnMapping['name']]['value'] ?? null); + $this->assertArrayHasKey($columnMapping['age'], $cachedCells); + $this->assertArrayHasKey($columnMapping['skills'], $cachedCells); + } + /** * Converts column names to IDs for Row2Mapper */ diff --git a/tests/unit/Db/Row2MapperTestDependencies.php b/tests/unit/Db/Row2MapperTestDependencies.php index a3dd5c34fb..1e3d3a5025 100644 --- a/tests/unit/Db/Row2MapperTestDependencies.php +++ b/tests/unit/Db/Row2MapperTestDependencies.php @@ -12,11 +12,22 @@ use OCA\Tables\Db\Column; use OCA\Tables\Db\ColumnMapper; use OCA\Tables\Db\Row2Mapper; +use OCA\Tables\Db\RowCellDatetimeMapper; +use OCA\Tables\Db\RowCellNumberMapper; +use OCA\Tables\Db\RowCellSelectionMapper; +use OCA\Tables\Db\RowCellTextMapper; +use OCA\Tables\Db\RowCellUsergroupMapper; +use OCA\Tables\Db\RowLoader\CachedRowLoader; +use OCA\Tables\Db\RowLoader\NormalizedRowLoader; use OCA\Tables\Db\RowSleeveMapper; use OCA\Tables\Helper\CircleHelper; use OCA\Tables\Helper\ColumnsHelper; +use OCA\Tables\Helper\GroupHelper; use OCA\Tables\Helper\UserHelper; use OCP\AppFramework\Db\DoesNotExistException; +use OCP\IAppConfig; +use OCP\IUserManager; +use OCP\IUserSession; use PHPUnit\Framework\MockObject\MockObject; use Psr\Log\LoggerInterface; @@ -35,6 +46,10 @@ trait Row2MapperTestDependencies { protected ColumnsHelper|MockObject $columnsHelper; protected LoggerInterface|MockObject $logger; protected CircleHelper|MockObject $circleHelper; + protected NormalizedRowLoader $normalizedRowLoader; + protected CachedRowLoader $cachedRowLoader; + protected IAppConfig|MockObject $appConfig; + protected bool $sleeveCellCachingComplete = true; protected static bool $testDataInitialized = false; protected static int $testTableId; @@ -56,7 +71,32 @@ protected function setupDependencies(): void { $this->logger = $this->createMock(LoggerInterface::class); $this->rowSleeveMapper = new RowSleeveMapper($this->connectionAdapter, $this->logger); - $this->columnsHelper = new ColumnsHelper($this->userHelper, $this->circleHelper); + + // Partial mock so getCachedCellsForRow() keeps working with the real + // implementation on top of the mocked cell mapper resolution + $this->columnsHelper = $this->getMockBuilder(ColumnsHelper::class) + ->setConstructorArgs([$this->userHelper, $this->circleHelper, $this->logger]) + ->onlyMethods(['getCellMapperFromType', 'resolveSearchValue']) + ->getMock(); + + // Mock getCellMapperFromType to return real cell mappers + $this->setupCellMappers(); + + // Use real loader instances since we have a real database + $this->normalizedRowLoader = new NormalizedRowLoader( + $this->connectionAdapter, + $this->rowSleeveMapper, + $this->columnsHelper, + $this->logger + ); + $this->cachedRowLoader = new CachedRowLoader( + $this->rowSleeveMapper, + $this->logger + ); + + $this->appConfig = $this->createMock(IAppConfig::class); + $this->appConfig->method('getValueBool') + ->willReturnCallback(fn () => $this->sleeveCellCachingComplete); $this->mapper = new Row2Mapper( 'test_user', @@ -65,7 +105,10 @@ protected function setupDependencies(): void { $this->userHelper, $this->rowSleeveMapper, $this->columnsHelper, - $this->columnMapper + $this->columnMapper, + $this->normalizedRowLoader, + $this->cachedRowLoader, + $this->appConfig ); if (!self::$testDataInitialized) { @@ -74,6 +117,31 @@ protected function setupDependencies(): void { } } + /** + * Sets up real cell mappers by mocking getCellMapperFromType + * This is needed because ColumnsHelper uses Server::get() which doesn't work in unit tests + */ + private function setupCellMappers(): void { + $userManager = $this->createMock(IUserManager::class); + $userSession = $this->createMock(IUserSession::class); + + // Delegate resolveSearchValue to a real instance so magic values like + // '@selection-id-1' or '@checked' are resolved as in production + $realColumnsHelper = new ColumnsHelper($this->userHelper, $this->circleHelper, $this->logger); + $this->columnsHelper->method('resolveSearchValue') + ->willReturnCallback(fn (string $placeholder, string $userId, ?Column $column = null) => $realColumnsHelper->resolveSearchValue($placeholder, $userId, $column)); + + $this->columnsHelper->method('getCellMapperFromType') + ->willReturnCallback(fn ($columnType) => match ($columnType) { + Column::TYPE_TEXT => new RowCellTextMapper($this->connectionAdapter), + Column::TYPE_NUMBER => new RowCellNumberMapper($this->connectionAdapter), + Column::TYPE_DATETIME => new RowCellDatetimeMapper($this->connectionAdapter), + Column::TYPE_SELECTION => new RowCellSelectionMapper($this->connectionAdapter), + Column::TYPE_USERGROUP => new RowCellUsergroupMapper($this->connectionAdapter, $userManager, $this->circleHelper, $this->createMock(GroupHelper::class), $userSession), + default => throw new \InvalidArgumentException("Unknown column type: $columnType"), + }); + } + /** * Initializes comprehensive test data for sorting and querying tests * @@ -219,6 +287,12 @@ protected function setupRealColumnMapper(int $tableId): void { $this->columnMapper->method('find') ->willReturnCallback(fn ($id) => $columns[$id] ?? throw new DoesNotExistException('test')); + $this->columnMapper->method('findAllByTable') + ->willReturnCallback(fn (int $tableId) => array_values(array_filter( + $columns, + fn (Column $column) => $column->getTableId() === $tableId + ))); + $this->columnMapper->method('preloadColumns'); $this->columnMapper->method('getColumnTypes')->willReturn($columnTypes); } diff --git a/tests/unit/TablesMigratorTest.php b/tests/unit/TablesMigratorTest.php index c68dcf0198..31f57a74e3 100644 --- a/tests/unit/TablesMigratorTest.php +++ b/tests/unit/TablesMigratorTest.php @@ -23,6 +23,7 @@ use OCA\Tables\Db\Table; use OCA\Tables\Db\TableMapper; use OCA\Tables\Db\ViewMapper; +use OCA\Tables\Helper\ColumnsHelper; use OCA\Tables\Service\ColumnService; use OCA\Tables\Service\ContextService; use OCA\Tables\Service\FavoritesService; @@ -36,6 +37,7 @@ use OCP\UserMigration\IExportDestination; use OCP\UserMigration\IImportSource; use PHPUnit\Framework\TestCase; +use Psr\Log\LoggerInterface; use Symfony\Component\Console\Output\NullOutput; class TablesMigratorTest extends TestCase { @@ -44,6 +46,7 @@ class TablesMigratorTest extends TestCase { private $tableMapper; private $columnMapper; private $rowSleeveMapper; + private $columnsHelper; private $viewMapper; private $contextMapper; private $shareMapper; @@ -60,12 +63,14 @@ class TablesMigratorTest extends TestCase { private $rowService; private $contextService; private $shareService; + private $logger; protected function setUp(): void { $this->l10n = $this->createMock(IL10N::class); $this->tableMapper = $this->createMock(TableMapper::class); $this->columnMapper = $this->createMock(ColumnMapper::class); $this->rowSleeveMapper = $this->createMock(RowSleeveMapper::class); + $this->columnsHelper = $this->createMock(ColumnsHelper::class); $this->viewMapper = $this->createMock(ViewMapper::class); $this->contextMapper = $this->createMock(ContextMapper::class); $this->shareMapper = $this->createMock(ShareMapper::class); @@ -82,12 +87,14 @@ protected function setUp(): void { $this->rowService = $this->createMock(RowService::class); $this->contextService = $this->createMock(ContextService::class); $this->shareService = $this->createMock(ShareService::class); + $this->logger = $this->createMock(LoggerInterface::class); $this->migrator = new TablesMigrator( $this->l10n, $this->tableMapper, $this->columnMapper, $this->rowSleeveMapper, + $this->columnsHelper, $this->viewMapper, $this->contextMapper, $this->shareMapper, @@ -103,7 +110,8 @@ protected function setUp(): void { $this->columnService, $this->rowService, $this->contextService, - $this->shareService + $this->shareService, + $this->logger ); }