From 37ca2f75ac49c4b163c64d1823ae7481462a9bd4 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Tue, 17 Jun 2025 13:31:31 +0200 Subject: [PATCH] refactor: simplify Columns::findAllByTable method - simply this method, kill a lot of `null`s in the majority of the calls - add a destinct method to cover the edge case - improves clarity Signed-off-by: Arthur Schiwon --- lib/Analytics/AnalyticsDatasource.php | 2 +- lib/Api/V1Api.php | 2 +- lib/Command/Clean.php | 2 +- lib/Command/TransferLegacyRows.php | 4 ++-- lib/Controller/Api1Controller.php | 20 ++++++++++++++++- lib/Migration/NewDbStructureRepairStep.php | 2 +- lib/Service/ColumnService.php | 25 +++++++++++++++++++--- lib/Service/TableService.php | 2 +- lib/Service/ViewService.php | 2 +- 9 files changed, 49 insertions(+), 12 deletions(-) diff --git a/lib/Analytics/AnalyticsDatasource.php b/lib/Analytics/AnalyticsDatasource.php index 02d64b84..5a61416f 100644 --- a/lib/Analytics/AnalyticsDatasource.php +++ b/lib/Analytics/AnalyticsDatasource.php @@ -208,7 +208,7 @@ class AnalyticsDatasource implements IDatasource { $rows = $this->rowService->findAllByView($nodeId, $this->userId, $limit, $offset); } else { // if no nodeType is provided, the old table selection is used to not break anything - $columns = $this->columnService->findAllByTable($nodeId, null, $this->userId); + $columns = $this->columnService->findAllByTable($nodeId, $this->userId); $rows = $this->rowService->findAllByTable($nodeId, $this->userId, $limit, $offset); } diff --git a/lib/Api/V1Api.php b/lib/Api/V1Api.php index e76f8e43..03b9cccf 100644 --- a/lib/Api/V1Api.php +++ b/lib/Api/V1Api.php @@ -47,7 +47,7 @@ class V1Api { $rows = $this->rowService->findAllByView($nodeId, $this->userId, $limit, $offset); } else { // if no nodeType is provided, the old table selection is used to not break anything - $columns = $this->columnService->findAllByTable($nodeId, null, $this->userId); + $columns = $this->columnService->findAllByTable($nodeId, $this->userId); $rows = $this->rowService->findAllByTable($nodeId, $this->userId, $limit, $offset); } diff --git a/lib/Command/Clean.php b/lib/Command/Clean.php index 80d5e294..b99ba518 100644 --- a/lib/Command/Clean.php +++ b/lib/Command/Clean.php @@ -97,7 +97,7 @@ class Clean extends Command { return; } $tableId = $this->rowMapper->getTableIdForRow($nextRowId); - $columns = $this->columnService->findAllByTable($tableId, null, ''); + $columns = $this->columnService->findAllByTable($tableId, ''); $this->row = $this->rowMapper->find($nextRowId, $columns); $this->offset = $this->row->getId(); } catch (Exception $e) { diff --git a/lib/Command/TransferLegacyRows.php b/lib/Command/TransferLegacyRows.php index fce75e00..801b7482 100644 --- a/lib/Command/TransferLegacyRows.php +++ b/lib/Command/TransferLegacyRows.php @@ -134,7 +134,7 @@ class TransferLegacyRows extends Command { * @throws Exception */ private function transferTable(Table $table, OutputInterface $output): void { - $columns = $this->columnService->findAllByTable($table->getId(), null, ''); + $columns = $this->columnService->findAllByTable($table->getId(), ''); $output->writeln('---- Found ' . count($columns) . ' columns'); $legacyRows = $this->legacyRowMapper->findAllByTable($table->getId()); @@ -155,7 +155,7 @@ class TransferLegacyRows extends Command { $output->writeln('Start deleting data for tables that should be transferred.'); foreach ($tables as $table) { try { - $columns = $this->columnService->findAllByTable($table->getId(), null, ''); + $columns = $this->columnService->findAllByTable($table->getId(), ''); } catch (InternalError|PermissionError $e) { $output->writeln('Could not delete data for table ' . $table->getId()); break; diff --git a/lib/Controller/Api1Controller.php b/lib/Controller/Api1Controller.php index 3783fa5b..b12f8e52 100644 --- a/lib/Controller/Api1Controller.php +++ b/lib/Controller/Api1Controller.php @@ -708,7 +708,21 @@ class Api1Controller extends ApiController { #[CORS] public function indexTableColumns(int $tableId, ?int $viewId): DataResponse { try { - return new DataResponse($this->columnService->formatColumns($this->columnService->findAllByTable($tableId, $viewId))); + try { + // permission check in service class + // they might be missing legally when only the view was shared + $columns = $this->columnService->findAllByTable($tableId); + } catch (PermissionError $e) { + if (!$viewId) { + throw $e; + } + $view = $this->viewService->find($viewId, false, $this->userId); + if ($tableId !== $view->getTableId()) { + throw new PermissionError('Given table is not a parent of the given view.'); + } + $columns = $this->columnService->findAllByManagedView($view, $this->userId); + } + return new DataResponse($this->columnService->formatColumns($columns)); } catch (PermissionError $e) { $this->logger->warning('A permission error occurred: ' . $e->getMessage(), ['exception' => $e]); $message = ['message' => $e->getMessage()]; @@ -717,6 +731,10 @@ class Api1Controller extends ApiController { $this->logger->error('An internal error or exception occurred: ' . $e->getMessage(), ['exception' => $e]); $message = ['message' => $e->getMessage()]; return new DataResponse($message, Http::STATUS_INTERNAL_SERVER_ERROR); + } catch (NotFoundError $e) { + $this->logger->warning('The view could not be found: ' . $e->getMessage(), ['exception' => $e]); + $message = ['message' => $e->getMessage()]; + return new DataResponse($message, Http::STATUS_NOT_FOUND); } } diff --git a/lib/Migration/NewDbStructureRepairStep.php b/lib/Migration/NewDbStructureRepairStep.php index 64ac8219..6f12f2f0 100644 --- a/lib/Migration/NewDbStructureRepairStep.php +++ b/lib/Migration/NewDbStructureRepairStep.php @@ -92,7 +92,7 @@ class NewDbStructureRepairStep implements IRepairStep { * @throws Exception */ private function transferTable(Table $table, IOutput $output) { - $columns = $this->columnService->findAllByTable($table->getId(), null, ''); + $columns = $this->columnService->findAllByTable($table->getId(), ''); $output->info('---- Found ' . count($columns) . ' columns'); $legacyRows = $this->legacyRowMapper->findAllByTable($table->getId()); diff --git a/lib/Service/ColumnService.php b/lib/Service/ColumnService.php index 4f26a09f..06634e3e 100644 --- a/lib/Service/ColumnService.php +++ b/lib/Service/ColumnService.php @@ -12,6 +12,7 @@ use Exception; use OCA\Tables\Db\Column; use OCA\Tables\Db\ColumnMapper; use OCA\Tables\Db\TableMapper; +use OCA\Tables\Db\View; use OCA\Tables\Dto\Column as ColumnDto; use OCA\Tables\Errors\InternalError; use OCA\Tables\Errors\NotFoundError; @@ -64,8 +65,8 @@ class ColumnService extends SuperService { * @throws InternalError * @throws PermissionError */ - public function findAllByTable(int $tableId, ?int $viewId = null, ?string $userId = null): array { - if ($this->permissionsService->canReadColumnsByTableId($tableId, $userId) || ($viewId != null && $this->permissionsService->canReadColumnsByViewId($viewId, $userId))) { + public function findAllByTable(int $tableId, ?string $userId = null): array { + if ($this->permissionsService->canReadColumnsByTableId($tableId, $userId)) { try { return $this->enhanceColumns($this->mapper->findAllByTable($tableId)); } catch (\OCP\DB\Exception $e) { @@ -77,6 +78,24 @@ class ColumnService extends SuperService { } } + /** + * @throws InternalError + * @throws PermissionError + * @return Column[] + */ + public function findAllByManagedView(View $view, string $userId): array { + if ($this->permissionsService->canManageView($view, $userId)) { + try { + return $this->enhanceColumns($this->mapper->findAllByTable($view->getTableId())); + } catch (\OCP\DB\Exception $e) { + $this->logger->error($e->getMessage(), ['exception' => $e]); + throw new InternalError(get_class($this) . ' - ' . __FUNCTION__ . ': ' . $e->getMessage()); + } + } else { + throw new PermissionError('no manage access to view id = ' . $view->getId()); + } + } + /** * @param int $viewId * @param string|null $userId @@ -428,7 +447,7 @@ class ColumnService extends SuperService { if ($viewId) { $allColumns = $this->findAllByView($viewId, $userId); } elseif ($tableId) { - $allColumns = $this->findAllByTable($tableId, null, $userId); + $allColumns = $this->findAllByTable($tableId, $userId); } else { $e = new Exception('Either tableId nor viewId is given.'); $this->logger->error($e->getMessage(), ['exception' => $e]); diff --git a/lib/Service/TableService.php b/lib/Service/TableService.php index 8f3b060e..e5b9eaef 100644 --- a/lib/Service/TableService.php +++ b/lib/Service/TableService.php @@ -404,7 +404,7 @@ class TableService extends SuperService { // delete all columns for that table try { - $columns = $this->columnService->findAllByTable($id, null, $userId); + $columns = $this->columnService->findAllByTable($id, $userId); } catch (InternalError|PermissionError $e) { $this->logger->error($e->getMessage(), ['exception' => $e]); throw new InternalError(get_class($this) . ' - ' . __FUNCTION__ . ': ' . $e->getMessage()); diff --git a/lib/Service/ViewService.php b/lib/Service/ViewService.php index ca35b943..792890ea 100644 --- a/lib/Service/ViewService.php +++ b/lib/Service/ViewService.php @@ -287,7 +287,7 @@ class ViewService extends SuperService { $columnService = \OCP\Server::get(ColumnService::class); $columnIds = array_column(\json_decode($value, true), 'columnId'); - $availableColumns = $columnService->findAllByTable($view->getTableId(), $view->getId(), $this->userId); + $availableColumns = $columnService->findAllByManagedView($view, $userId); $availableColumns = array_map(static fn (Column $column) => $column->getId(), $availableColumns); foreach ($columnIds as $columnId) { if (!Column::isValidMetaTypeId($columnId) && !in_array($columnId, $availableColumns, true)) {