From b0027d3c8b6275a4ec79c54b15f91b1d2dc44de1 Mon Sep 17 00:00:00 2001 From: Remko Date: Mon, 12 Oct 2026 10:06:46 +0200 Subject: [PATCH 1/5] feat(views): allow views without a table and give views a type A view can now exist without a table and carries a type (table or grid), so an application can hold pages that are not tied to one table. - tables_views gets a type column and table_id becomes nullable - a view without a table is owned by its creator and listed for them - row, column, import, relation, permission and activity code skips views without a table instead of failing on the missing table - views without a table are removed with their owner Co-authored-by: Ruben van der Linde Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: Remko --- lib/Activity/ActivityManager.php | 7 + lib/Constants/ViewUpdatableParameters.php | 1 + lib/Db/View.php | 25 ++- lib/Db/ViewMapper.php | 29 +++- lib/Listener/UserDeletedListener.php | 26 +++ .../Version2400Date20261012000000.php | 43 +++++ lib/Model/ViewUpdateInput.php | 22 ++- lib/ResponseDefinitions.php | 3 +- lib/Service/ColumnService.php | 6 + lib/Service/ContextService.php | 5 +- lib/Service/ImportService.php | 6 + lib/Service/PermissionsService.php | 6 +- lib/Service/RelationService.php | 4 + lib/Service/RowService.php | 8 + lib/Service/ViewService.php | 59 ++++++- openapi.json | 9 +- src/types/openapi/openapi.ts | 3 +- tests/unit/Activity/ActivityManagerTest.php | 48 +++++ tests/unit/Db/ViewTest.php | 49 ++++++ tests/unit/Model/ViewUpdateInputTest.php | 31 ++++ tests/unit/Service/PermissionsServiceTest.php | 22 +++ tests/unit/Service/ViewServiceTest.php | 164 ++++++++++++++++++ 22 files changed, 553 insertions(+), 23 deletions(-) create mode 100644 lib/Migration/Version2400Date20261012000000.php create mode 100644 tests/unit/Activity/ActivityManagerTest.php create mode 100644 tests/unit/Db/ViewTest.php create mode 100644 tests/unit/Service/ViewServiceTest.php diff --git a/lib/Activity/ActivityManager.php b/lib/Activity/ActivityManager.php index 64c30201f9..a464595fb5 100644 --- a/lib/Activity/ActivityManager.php +++ b/lib/Activity/ActivityManager.php @@ -158,6 +158,10 @@ private function createEvent(string $objectType, Row2|Table|View|Column $object, $objectTitle = $object->getTitle(); $table = $object; } elseif ($object instanceof View) { + if ($object->getTableId() === null) { + // activities are attached to a table, a view without one has no stream + return null; + } $objectTitle = $object->getTitle(); $table = $this->tableMapper->find($object->getTableId()); } elseif ($object instanceof Row2) { @@ -434,6 +438,9 @@ private function sendShareEventToUsers(IEvent $event, Row2|Table|View|Column $ob } $tableId = $object instanceof Table ? $object->getId() : $object->getTableId(); + if ($tableId === null) { + return; + } $table = $object instanceof Table ? $object : $this->tableMapper->find($tableId); $tableOwner = $table->getOwnership(); $eventAuthor = $event->getAuthor(); diff --git a/lib/Constants/ViewUpdatableParameters.php b/lib/Constants/ViewUpdatableParameters.php index b6a5d8139b..6e97996ce8 100644 --- a/lib/Constants/ViewUpdatableParameters.php +++ b/lib/Constants/ViewUpdatableParameters.php @@ -18,4 +18,5 @@ enum ViewUpdatableParameters: string { case COLUMN_SETTINGS = 'columns'; case TECHNICAL_NAME = 'technicalName'; case SIDEBAR_ORDER = 'sidebarOrder'; + case TYPE = 'type'; } diff --git a/lib/Db/View.php b/lib/Db/View.php index fb86c02c9f..7fe177ff3f 100644 --- a/lib/Db/View.php +++ b/lib/Db/View.php @@ -30,8 +30,10 @@ * @method setTitle(string $title) * @method getTechnicalName(): string * @method setTechnicalName(?string $technicalName) - * @method getTableId(): int - * @method setTableId(int $tableId) + * @method getTableId(): ?int + * @method setTableId(?int $tableId) + * @method getType(): string + * @method setType(string $type) * @method getColumns(): string * @method setColumns(string $columns) * @method getColumnSettings(): string @@ -74,6 +76,10 @@ * @method setSidebarOrder(?int $sidebarOrder) */ class View extends EntitySuper implements JsonSerializable { + public const TYPE_TABLE = 'table'; + public const TYPE_GRID = 'grid'; + public const TYPES = [self::TYPE_TABLE, self::TYPE_GRID]; + protected ?string $uuid = null; protected ?string $title = null; protected ?string $technicalName = null; @@ -88,6 +94,7 @@ class View extends EntitySuper implements JsonSerializable { protected ?string $columns = null; // json protected ?string $sort = null; // json protected ?string $filter = null; // json + protected ?string $type = null; protected ?int $externalId = null; protected ?string $shareToken = null; @@ -212,6 +219,17 @@ public function setFilterArray(array $array):void { $this->setFilter(\json_encode($array)); } + public function getTypeOrDefault(): string { + return in_array($this->type, self::TYPES, true) ? $this->type : self::TYPE_TABLE; + } + + /** + * A view inherits its ownership from its table; a view without a table is owned by its creator. + */ + public function getOwnership(): ?string { + return $this->ownership ?? $this->createdBy; + } + private function getSharePermissions(): ?Permissions { return $this->getOnSharePermissions(); } @@ -228,7 +246,7 @@ public function jsonSerialize(): array { 'technicalName' => $this->technicalName, 'description' => $this->description, 'emoji' => $this->emoji, - 'ownership' => $this->ownership ?: '', + 'ownership' => $this->getOwnership() ?: '', 'createdBy' => $this->createdBy ?: '', 'createdAt' => $this->createdAt ?: '', 'lastEditBy' => $this->lastEditBy ?: '', @@ -244,6 +262,7 @@ public function jsonSerialize(): array { 'ownerDisplayName' => $this->ownerDisplayName, 'isFederated' => $this->isFederated(), 'sidebarOrder' => $this->sidebarOrder, + 'type' => $this->getTypeOrDefault(), ]; $serialisedJson['filter'] = $this->getFilterArray(); diff --git a/lib/Db/ViewMapper.php b/lib/Db/ViewMapper.php index f5bbca13e8..78319a3a40 100644 --- a/lib/Db/ViewMapper.php +++ b/lib/Db/ViewMapper.php @@ -89,6 +89,22 @@ public function findMany(array $ids): array { return $result; } + /** + * Views that live without a table and belong to the given user. + * + * @return View[] + * @throws Exception + */ + public function findAllStandaloneOwnedBy(string $userId): array { + $qb = $this->db->getQueryBuilder(); + $qb->select('*') + ->from($this->table) + ->where($qb->expr()->isNull('table_id')) + ->andWhere($qb->expr()->eq('created_by', $qb->createNamedParameter($userId, IQueryBuilder::PARAM_STR))) + ->orderBy('id', 'ASC'); + return $this->findEntities($qb); + } + public function findByExternalIdAndToken(int $externalId, string $shareToken): ?View { $qb = $this->db->getQueryBuilder(); $qb->select('*') @@ -134,7 +150,7 @@ public function findAll(?int $tableId = null): array { $qb = $this->db->getQueryBuilder(); $qb->select('v.*', 't.ownership') ->from($this->table, 'v') - ->innerJoin('v', 'tables_tables', 't', 't.id = v.table_id'); + ->leftJoin('v', 'tables_tables', 't', 't.id = v.table_id'); if ($tableId !== null) { $qb->where($qb->expr()->eq('v.table_id', $qb->createNamedParameter($tableId, IQueryBuilder::PARAM_INT))); @@ -239,16 +255,19 @@ public function findIdToTitleMap(array $ids): array { } /** - * @throws DoesNotExistException - * @throws Exception * @throws MultipleObjectsReturnedException + * @throws Exception */ public function findByUuid(string $uuid): ?View { $qb = $this->db->getQueryBuilder(); $qb->select('v.*', 't.ownership') ->from($this->table, 'v') - ->innerJoin('v', 'tables_tables', 't', 't.id = v.table_id') + ->leftJoin('v', 'tables_tables', 't', 't.id = v.table_id') ->where($qb->expr()->eq('v.uuid', $qb->createNamedParameter($uuid))); - return $this->findEntity($qb); + try { + return $this->findEntity($qb); + } catch (DoesNotExistException) { + return null; + } } } diff --git a/lib/Listener/UserDeletedListener.php b/lib/Listener/UserDeletedListener.php index 69b1944611..c63b3547b0 100644 --- a/lib/Listener/UserDeletedListener.php +++ b/lib/Listener/UserDeletedListener.php @@ -7,8 +7,11 @@ namespace OCA\Tables\Listener; +use OCA\Tables\Db\ViewMapper; use OCA\Tables\Errors\InternalError; use OCA\Tables\Service\TableService; +use OCA\Tables\Service\ViewService; +use OCP\DB\Exception; use OCP\EventDispatcher\Event; use OCP\EventDispatcher\IEventListener; use OCP\User\Events\BeforeUserDeletedEvent; @@ -20,6 +23,8 @@ class UserDeletedListener implements IEventListener { public function __construct( private readonly TableService $tableService, + private readonly ViewService $viewService, + private readonly ViewMapper $viewMapper, private readonly LoggerInterface $logger, ) { } @@ -41,5 +46,26 @@ public function handle(Event $event): void { $this->logger->debug('tables for the deleted user removed'); } catch (InternalError) { } + + $this->deleteViewsWithoutTable($event->getUser()->getUID()); + } + + /** + * Views without a table belong to their creator, so they go with the user. + */ + private function deleteViewsWithoutTable(string $userId): void { + try { + $views = $this->viewMapper->findAllStandaloneOwnedBy($userId); + } catch (Exception $e) { + $this->logger->error('Views without a table of the deleted user could not be loaded', ['exception' => $e]); + return; + } + foreach ($views as $view) { + try { + $this->viewService->deleteByObject($view, $userId); + } catch (InternalError $e) { + $this->logger->error('View ' . $view->getId() . ' of the deleted user could not be deleted', ['exception' => $e]); + } + } } } diff --git a/lib/Migration/Version2400Date20261012000000.php b/lib/Migration/Version2400Date20261012000000.php new file mode 100644 index 0000000000..859dfb0a96 --- /dev/null +++ b/lib/Migration/Version2400Date20261012000000.php @@ -0,0 +1,43 @@ +hasTable('tables_views')) { + $table = $schema->getTable('tables_views'); + if (!$table->hasColumn('type')) { + $table->addColumn('type', Types::STRING, [ + 'notnull' => true, + 'default' => 'table', + 'length' => 16, + ]); + } + // A grid view does not need a table + $table->getColumn('table_id')->setNotnull(false); + } + + return $schema; + } +} diff --git a/lib/Model/ViewUpdateInput.php b/lib/Model/ViewUpdateInput.php index 8cfd5c0a49..a43e30cfd2 100644 --- a/lib/Model/ViewUpdateInput.php +++ b/lib/Model/ViewUpdateInput.php @@ -10,6 +10,7 @@ namespace OCA\Tables\Model; use Generator; +use InvalidArgumentException; use OCA\Tables\AppInfo\Application; use OCA\Tables\Constants\ViewUpdatableParameters; use OCA\Tables\Service\ValueObject\Emoji; @@ -31,6 +32,7 @@ public function __construct( protected readonly ?FilterSet $filterSet = null, protected readonly ?SortRuleSet $sortRuleSet = null, protected readonly ?int $sidebarOrder = null, + protected readonly ?string $type = null, ) { } @@ -59,6 +61,9 @@ public function updateDetail(): Generator { if ($this->sortRuleSet) { yield ViewUpdatableParameters::SORT => $this->sortRuleSet; } + if ($this->type !== null) { + yield ViewUpdatableParameters::TYPE => $this->type; + } } /** @@ -71,7 +76,8 @@ public function updateDetail(): Generator { * columnSettings?: list, * sort?: list, * filter?: list}>>, - * sidebarOrder?: int + * sidebarOrder?: int, + * type?: string * } $data * @param array $columnsMap */ @@ -100,9 +106,23 @@ public static function fromInputArray(array $data, array $columnsMap = []): self filterSet: isset($data['filter']) ? FilterSet::createFromInputArray($data['filter'], $columnsMap) : null, sortRuleSet: isset($data['sort']) ? SortRuleSet::createFromInputArray($data['sort'], $columnsMap) : null, sidebarOrder: (array_key_exists('sidebarOrder', $data) && $data['sidebarOrder'] !== null) ? (int)$data['sidebarOrder'] : null, + type: self::optionalString($data, 'type'), ); } + /** + * @throws InvalidArgumentException + */ + private static function optionalString(array $data, string $key): ?string { + if (!isset($data[$key])) { + return null; + } + if (!is_string($data[$key])) { + throw new InvalidArgumentException('The view property "' . $key . '" must be a string.'); + } + return $data[$key]; + } + protected static function transformJsonToArrayInPayload(array $input, array $keys): array { $output = $input; foreach ($keys as $targetKey) { diff --git a/lib/ResponseDefinitions.php b/lib/ResponseDefinitions.php index c935d72524..a2a858d788 100644 --- a/lib/ResponseDefinitions.php +++ b/lib/ResponseDefinitions.php @@ -19,7 +19,7 @@ * title: string, * technicalName: string|null, * emoji: string|null, - * tableId: int, + * tableId: int|null, * ownership: string, * ownerDisplayName: string|null, * createdBy: string, @@ -45,6 +45,7 @@ * rowsCount: int, * isFederated: bool, * sidebarOrder: int|null, + * type: string, * } * * @psalm-type TablesTable = array{ diff --git a/lib/Service/ColumnService.php b/lib/Service/ColumnService.php index 7bb5c79d59..50b9b72ea6 100644 --- a/lib/Service/ColumnService.php +++ b/lib/Service/ColumnService.php @@ -112,6 +112,9 @@ public function findAllByTable(int $tableId, ?string $userId = null, ?Table $tab * @return Column[] */ public function findAllByManagedView(View $view, string $userId): array { + if ($view->getTableId() === null) { + return []; + } if ($this->permissionsService->canManageView($view, $userId)) { try { return $this->enhanceColumns($this->mapper->findAllByTable($view->getTableId()), $view); @@ -243,6 +246,9 @@ public function create( $this->logger->error($e->getMessage(), ['exception' => $e]); throw new NotFoundError(static::class . ' - ' . __FUNCTION__ . ': ' . $e->getMessage()); } + if ($view->getTableId() === null) { + throw new NotFoundError('The view with id = ' . $viewId . ' has no table to add a column to.'); + } try { $table = $this->tableMapper->find($view->getTableId()); } catch (DoesNotExistException $e) { diff --git a/lib/Service/ContextService.php b/lib/Service/ContextService.php index edfd7d9a6e..10b02451c3 100644 --- a/lib/Service/ContextService.php +++ b/lib/Service/ContextService.php @@ -689,7 +689,7 @@ public function getScheme(int $contextId, string $userId): ContextScheme { $view = $viewService->find($node['node_id']); $node['node_uuid'] = $view->getUuid(); $node['node_title'] = $view->getTitle(); - if (isset($tables[$view->getTableId()])) { + if ($view->getTableId() === null || isset($tables[$view->getTableId()])) { continue; } $tables[$view->getTableId()] = $tableService->getScheme($view->getTableId(), $userId)->jsonSerialize(); @@ -850,6 +850,9 @@ public function importScheme(int $contextId, string $name, string $iconName, str ]; } elseif ($node['node_type'] === Application::NODE_TYPE_VIEW) { $view = $this->viewMapper->findByUuid($node['node_uuid']); + if ($view === null) { + throw new DoesNotExistException('View ' . $node['node_uuid'] . ' does not exist'); + } $resolvedNodes[] = [ 'id' => $view->getId(), 'type' => $node['node_type'], diff --git a/lib/Service/ImportService.php b/lib/Service/ImportService.php index a5acf500b5..5e3f446985 100644 --- a/lib/Service/ImportService.php +++ b/lib/Service/ImportService.php @@ -310,6 +310,9 @@ private function getPreviewData(Worksheet $worksheet): array { public function import(?int $tableId, ?int $viewId, string $path, bool $createMissingColumns = true, array $columnsConfig = []): array { if ($viewId !== null) { $view = $this->viewService->find($viewId); + if ($view->getTableId() === null) { + throw new NotFoundError('The view with id = ' . $viewId . ' has no table to import rows into.'); + } if (!$this->permissionsService->canCreateRows($view)) { throw new PermissionError('create row at the view id = ' . $viewId . ' is not allowed.'); } @@ -400,6 +403,9 @@ public function import(?int $tableId, ?int $viewId, string $path, bool $createMi public function scheduleImport(?int $tableId, ?int $viewId, string $path, bool $createMissingColumns = true, array $columnsConfig = []) { if ($viewId !== null) { $view = $this->viewService->find($viewId); + if ($view->getTableId() === null) { + throw new NotFoundError('The view with id = ' . $viewId . ' has no table to import rows into.'); + } if (!$this->permissionsService->canCreateRows($view)) { throw new PermissionError('create row at the view id = ' . $viewId . ' is not allowed.'); } diff --git a/lib/Service/PermissionsService.php b/lib/Service/PermissionsService.php index dc68798e23..4432060a90 100644 --- a/lib/Service/PermissionsService.php +++ b/lib/Service/PermissionsService.php @@ -456,7 +456,7 @@ public function getSharedPermissionsIfSharedWithMe(int $elementId, string $eleme // We need to take possibly inherited permissions into account try { $view = $this->viewMapper->find($elementId); - if (!$view->isFederated()) { + if (!$view->isFederated() && $view->getTableId() !== null) { $table = $this->tableMapper->find($view->getTableId()); } } catch (DoesNotExistException $e) { @@ -701,6 +701,10 @@ private function basisCheck(Table|View|Context $element, string $nodeType, ?stri } $shareNodeId = $nodeType === 'view' ? $element->getTableId() : $element->getId(); + if ($shareNodeId === null) { + // a view without a table has nothing to inherit from + return false; + } // Views inherit manage permissions from their parent table, while contexts // must resolve against context shares to avoid cross-type ID collisions. $shareNodeType = $nodeType === 'context' ? 'context' : 'table'; diff --git a/lib/Service/RelationService.php b/lib/Service/RelationService.php index 6d0ca9fade..7d9bb242e8 100644 --- a/lib/Service/RelationService.php +++ b/lib/Service/RelationService.php @@ -160,6 +160,10 @@ private function getRelationDataForTarget(string $target, Column $column): array $targetColumn = $this->columnMapper->find($settings[Column::RELATION_LABEL_COLUMN]); if ($isView) { $view = $this->viewMapper->find($targetId); + if ($view->getTableId() === null) { + $this->cacheRelationData[$cacheKey] = []; + return []; + } $rows = $this->row2Mapper->findAll( [$targetColumn->getId()], $view->getTableId(), diff --git a/lib/Service/RowService.php b/lib/Service/RowService.php index 3e98e7d4a2..862a5dbfad 100644 --- a/lib/Service/RowService.php +++ b/lib/Service/RowService.php @@ -130,6 +130,10 @@ public function findAllByView(int $viewId, string $userId, ?int $limit = null, ? try { if ($this->permissionsService->canReadRowsByElementId($viewId, 'view', $userId)) { $view = $this->viewMapper->find($viewId); + if ($view->getTableId() === null) { + // a view without a table, such as a grid view, holds no rows + return []; + } $rows = $this->row2Mapper->findAll( $view->getColumnIds(), @@ -230,6 +234,10 @@ public function create(?int $tableId, ?int $viewId, RowDataInput|array $data, ?s throw new InternalError(static::class . ' - ' . __FUNCTION__ . ': ' . $e->getMessage()); } + if ($view->getTableId() === null) { + throw new NotFoundError('The view with id = ' . $viewId . ' has no table to create a row in.'); + } + // security if (!$this->permissionsService->canCreateRows($view, 'view', $this->userId)) { throw new PermissionError('create row at the view id = ' . $viewId . ' is not allowed.'); diff --git a/lib/Service/ViewService.php b/lib/Service/ViewService.php index c20ff4bedb..f7e5944393 100644 --- a/lib/Service/ViewService.php +++ b/lib/Service/ViewService.php @@ -147,6 +147,10 @@ public function findSharedViewsWithMe(?string $userId = null): array { } $sharedViews = $this->shareService->findViewsSharedWithMe($userId); + // views without a table are not listed under any table, so the user's own ones are listed here + foreach ($this->mapper->findAllStandaloneOwnedBy($userId) as $standaloneView) { + $sharedViews[$standaloneView->getId()] ??= $standaloneView; + } $contexts = $this->contextService->findAll($userId); foreach ($contexts as $context) { $nodes = $context->getNodes(); @@ -181,18 +185,28 @@ public function findSharedViewsWithMe(?string $userId = null): array { public function create( string $title, ?string $emoji, - Table $table, + ?Table $table, ?string $userId = null, ?string $technicalName = null, ?string $uuid = null, + string $type = View::TYPE_TABLE, + string $description = '', ): View { /** @var string $userId */ $userId = $this->permissionsService->preCheckUserId($userId, false); // $userId is set + $this->assertTypeValid($type); + if ($table === null && $type === View::TYPE_TABLE) { + throw new BadRequestError('A table view needs a table.'); + } + // security - if (!$this->permissionsService->canManageTable($table, $userId)) { + if ($table !== null && !$this->permissionsService->canManageTable($table, $userId)) { throw new PermissionError('PermissionError: can not create view'); } + if ($table === null && $userId === '') { + throw new PermissionError('PermissionError: a view without a table needs an owner'); + } $time = new DateTime(); $item = new View(); @@ -201,8 +215,9 @@ public function create( if ($emoji) { $item->setEmoji($emoji); } - $item->setDescription(''); - $item->setTableId($table->getId()); + $item->setDescription($description); + $item->setTableId($table?->getId()); + $item->setType($type); $item->setCreatedBy($userId); $item->setLastEditBy($userId); $item->setCreatedAt($time->format('Y-m-d H:i:s')); @@ -213,8 +228,8 @@ public function create( } // ownership is not stored with the record, but it might be necessary upon // further interaction with the view in the running process, as the instance - // is cached now. The ownership is always inherited from the table. - $item->setOwnership($table->getOwnership()); + // is cached now. The ownership is inherited from the table, or it is the creator's. + $item->setOwnership($table?->getOwnership() ?? $userId); try { $newItem = $this->mapper->insert($item); } catch (\OCP\DB\Exception $e) { @@ -258,6 +273,7 @@ public function update(int $id, ViewUpdateInput $data, ?string $userId = null, b } foreach ($data->updateDetail() as $parameter => $value) { + $insertableValue = null; if ($parameter === ViewUpdatableParameters::COLUMN_SETTINGS && $value instanceof ColumnSettings ) { @@ -268,6 +284,13 @@ public function update(int $id, ViewUpdateInput $data, ?string $userId = null, b $this->assertTechnicalNameValid($value); } + if ($parameter === ViewUpdatableParameters::TYPE) { + $this->assertTypeValid($value); + if ($value === View::TYPE_TABLE && $view->getTableId() === null) { + throw new BadRequestError('A view without a table cannot become a table view.'); + } + } + if ($value instanceof JsonSerializable) { $insertableValue = json_encode($value); } @@ -427,6 +450,10 @@ private function enhanceView(View $view, string $userId): void { $this->setIsSharedState($view, $userId); + if ($view->getTableId() === null) { + return; + } + if (!$this->permissionsService->canReadRowsByElement($view, 'view', $userId)) { return; } @@ -496,6 +523,9 @@ private function setIsSharedState(View $view, string $userId): void { $view->setIsShared(true); $canManageTable = false; try { + if ($view->getTableId() === null) { + throw new NotFoundError('The view has no table'); + } try { $manageTableShare = $this->shareService->getSharedPermissionsIfSharedWithMe($view->getTableId(), 'table', $userId); } catch (NotFoundError) { @@ -654,15 +684,16 @@ public function addColumnToView(View $view, Column $column, ?string $userId = nu } /** - * @param int $tableId + * @param int|null $tableId null for a view without a table, such as a grid view * @param array $view * @param string $userId * * @return View * * @throws InternalError + * @throws BadRequestError */ - public function importView(int $tableId, array $view, string $userId): View { + public function importView(?int $tableId, array $view, string $userId): View { $item = new View(); $item->setUuid((isset($view['uuid']) && Uuid::isValid($view['uuid'])) ? $view['uuid'] : null); $item->setTableId($tableId); @@ -682,6 +713,9 @@ public function importView(int $tableId, array $view, string $userId): View { $item->setColumns(json_encode($view['columnSettings'])); $item->setSort(json_encode($view['sort'])); $item->setFilter(json_encode($view['filter'])); + $type = $view['type'] ?? View::TYPE_TABLE; + $this->assertTypeValid($type); + $item->setType($type); try { $importedView = $this->mapper->insert($item); if ($item->getTechnicalName() === null || $item->getTechnicalName() === '') { @@ -701,6 +735,15 @@ private function buildDefaultTechnicalName(int $viewId): string { return 'view_' . $viewId; } + /** + * @throws BadRequestError + */ + private function assertTypeValid(string $type): void { + if (!in_array($type, View::TYPES, true)) { + throw new BadRequestError('Unknown view type "' . $type . '", expected one of: ' . implode(', ', View::TYPES)); + } + } + /** * @throws BadRequestError */ diff --git a/openapi.json b/openapi.json index 80e327a7a7..f4dd2ba630 100644 --- a/openapi.json +++ b/openapi.json @@ -1014,7 +1014,8 @@ "hasShares", "rowsCount", "isFederated", - "sidebarOrder" + "sidebarOrder", + "type" ], "properties": { "id": { @@ -1037,7 +1038,8 @@ }, "tableId": { "type": "integer", - "format": "int64" + "format": "int64", + "nullable": true }, "ownership": { "type": "string" @@ -1257,6 +1259,9 @@ "type": "integer", "format": "int64", "nullable": true + }, + "type": { + "type": "string" } } } diff --git a/src/types/openapi/openapi.ts b/src/types/openapi/openapi.ts index e8d5e3a0c9..875c86775c 100644 --- a/src/types/openapi/openapi.ts +++ b/src/types/openapi/openapi.ts @@ -1360,7 +1360,7 @@ export type components = { readonly technicalName: string | null; readonly emoji: string | null; /** Format: int64 */ - readonly tableId: number; + readonly tableId: number | null; readonly ownership: string; readonly ownerDisplayName: string | null; readonly createdBy: string; @@ -1416,6 +1416,7 @@ export type components = { readonly isFederated: boolean; /** Format: int64 */ readonly sidebarOrder: number | null; + readonly type: string; }; }; responses: never; diff --git a/tests/unit/Activity/ActivityManagerTest.php b/tests/unit/Activity/ActivityManagerTest.php new file mode 100644 index 0000000000..ecaacc3966 --- /dev/null +++ b/tests/unit/Activity/ActivityManagerTest.php @@ -0,0 +1,48 @@ +createMock(IManager::class); + $activityApi->expects($this->never())->method('publish'); + $tableMapper = $this->createMock(TableMapper::class); + $tableMapper->expects($this->never())->method('find'); + + $manager = new ActivityManager( + $activityApi, + $this->createMock(IFactory::class), + $tableMapper, + $this->createMock(ViewMapper::class), + $this->createMock(ColumnMapper::class), + $this->createMock(ShareService::class), + new CappedMemoryCache(), + 'alice', + ); + $view = new View(); + $view->setId(6); + $view->setTitle('Intake home'); + $view->setTableId(null); + $view->setCreatedBy('alice'); + + $manager->triggerEvent(ActivityManager::TABLES_OBJECT_VIEW, $view, ActivityManager::SUBJECT_VIEW_CREATE, [], 'alice'); + } +} diff --git a/tests/unit/Db/ViewTest.php b/tests/unit/Db/ViewTest.php new file mode 100644 index 0000000000..57ce3928d7 --- /dev/null +++ b/tests/unit/Db/ViewTest.php @@ -0,0 +1,49 @@ +assertSame(View::TYPE_TABLE, $view->getTypeOrDefault()); + + $view->setType('something-else'); + $this->assertSame(View::TYPE_TABLE, $view->getTypeOrDefault()); + + $view->setType(View::TYPE_GRID); + $this->assertSame(View::TYPE_GRID, $view->getTypeOrDefault()); + } + + public function testOwnershipFallsBackToCreatorForViewsWithoutTable(): void { + $view = new View(); + $view->setCreatedBy('alice'); + $this->assertSame('alice', $view->getOwnership()); + + $view->setOwnership('table-owner'); + $this->assertSame('table-owner', $view->getOwnership()); + } + + public function testJsonSerializeCarriesTypeAndTechnicalName(): void { + $view = new View(); + $view->setType(View::TYPE_GRID); + $view->setTechnicalName('home'); + $view->setCreatedBy('alice'); + + $json = $view->jsonSerialize(); + + $this->assertSame('grid', $json['type']); + $this->assertSame('home', $json['technicalName']); + $this->assertSame(-1, $json['tableId']); + $this->assertSame('alice', $json['ownership']); + } +} diff --git a/tests/unit/Model/ViewUpdateInputTest.php b/tests/unit/Model/ViewUpdateInputTest.php index 8df9e05705..dfd95a18aa 100644 --- a/tests/unit/Model/ViewUpdateInputTest.php +++ b/tests/unit/Model/ViewUpdateInputTest.php @@ -9,6 +9,7 @@ namespace OCA\Tables\Tests\Unit\Model; +use InvalidArgumentException; use OCA\Tables\Constants\ViewUpdatableParameters; use OCA\Tables\Model\ViewUpdateInput; use PHPUnit\Framework\TestCase; @@ -37,4 +38,34 @@ public function testUpdateDetailIncludesEmptyDescription(): void { $this->assertArrayHasKey(ViewUpdatableParameters::DESCRIPTION->value, $updates); $this->assertSame('', $updates[ViewUpdatableParameters::DESCRIPTION->value]); } + + public function testUpdateDetailIncludesTypeAndTechnicalName(): void { + $input = ViewUpdateInput::fromInputArray(['type' => 'grid', 'technicalName' => 'home']); + $updates = []; + + foreach ($input->updateDetail() as $parameter => $value) { + $updates[$parameter->value] = $value; + } + + $this->assertSame('grid', $updates[ViewUpdatableParameters::TYPE->value]); + $this->assertSame('home', $updates[ViewUpdatableParameters::TECHNICAL_NAME->value]); + } + + public function testUpdateDetailSkipsTypeWhenAbsent(): void { + $input = ViewUpdateInput::fromInputArray(['title' => 'Only a title']); + $updates = []; + + foreach ($input->updateDetail() as $parameter => $value) { + $updates[$parameter->value] = $value; + } + + $this->assertArrayNotHasKey(ViewUpdatableParameters::TYPE->value, $updates); + $this->assertArrayNotHasKey(ViewUpdatableParameters::TECHNICAL_NAME->value, $updates); + } + + public function testATypeThatIsNotAStringIsRejected(): void { + $this->expectException(InvalidArgumentException::class); + $this->expectExceptionMessage('"type" must be a string'); + ViewUpdateInput::fromInputArray(['type' => ['grid']]); + } } diff --git a/tests/unit/Service/PermissionsServiceTest.php b/tests/unit/Service/PermissionsServiceTest.php index 7d1dfe8468..9b7d620c28 100644 --- a/tests/unit/Service/PermissionsServiceTest.php +++ b/tests/unit/Service/PermissionsServiceTest.php @@ -12,6 +12,7 @@ use OCA\Tables\Db\ContextMapper; use OCA\Tables\Db\ShareMapper; use OCA\Tables\Db\TableMapper; +use OCA\Tables\Db\View; use OCA\Tables\Db\ViewMapper; use OCA\Tables\Errors\InternalError; use OCA\Tables\Helper\CircleHelper; @@ -71,4 +72,25 @@ public function testPreCheckUserIdNoUserAllowed() { self::assertEquals($userId, $permissionsService->preCheckUserId($userId)); } + + private function tablelessView(string $createdBy): View { + $view = new View(); + $view->setId(6); + $view->setTableId(null); + $view->setCreatedBy($createdBy); + $view->setType(View::TYPE_GRID); + return $view; + } + + public function testTheCreatorManagesAViewWithoutTable(): void { + $permissionsService = $this->getPermissionServiceWithUserId('alice'); + + $this->assertTrue($permissionsService->canManageView($this->tablelessView('alice'), 'alice')); + } + + public function testOthersDoNotInheritAnythingOnAViewWithoutTable(): void { + $permissionsService = $this->getPermissionServiceWithUserId('bob'); + + $this->assertFalse($permissionsService->canManageView($this->tablelessView('alice'), 'bob')); + } } diff --git a/tests/unit/Service/ViewServiceTest.php b/tests/unit/Service/ViewServiceTest.php new file mode 100644 index 0000000000..5a58a0d210 --- /dev/null +++ b/tests/unit/Service/ViewServiceTest.php @@ -0,0 +1,164 @@ +permissionsService = $this->createMock(PermissionsService::class); + $this->permissionsService->method('preCheckUserId')->willReturnCallback(static fn (?string $userId) => $userId ?? 'alice'); + $this->mapper = $this->createMock(ViewMapper::class); + $this->activityManager = $this->createMock(ActivityManager::class); + $l10n = $this->createMock(IL10N::class); + $l10n->method('t')->willReturnCallback(static fn (string $text): string => $text); + + $this->service = new ViewService( + $this->permissionsService, + $this->createMock(LoggerInterface::class), + 'alice', + $this->mapper, + $this->createMock(ShareService::class), + $this->createMock(RowService::class), + $this->createMock(UserHelper::class), + $this->createMock(FavoritesService::class), + $this->createMock(IEventDispatcher::class), + $this->createMock(ContextService::class), + $l10n, + $this->createMock(FederationService::class), + $this->activityManager, + ); + } + + private function storedView(?int $tableId = 4): View { + $view = new View(); + $view->setId(6); + $view->setTitle('Intake home'); + $view->setTableId($tableId); + $view->setCreatedBy('alice'); + $view->setType($tableId === null ? View::TYPE_GRID : View::TYPE_TABLE); + $view->resetUpdatedFields(); + return $view; + } + + private function expectUpdateOf(View $view): void { + $this->permissionsService->method('canManageView')->willReturn(true); + $this->mapper->method('find')->with(6)->willReturn($view); + $this->mapper->method('update')->willReturnArgument(0); + } + + public function testUpdateWritesEveryParameterToItsOwnColumn(): void { + $this->expectUpdateOf($this->storedView(4)); + + $updated = $this->service->update(6, ViewUpdateInput::fromInputArray([ + 'sort' => [['columnId' => 3, 'mode' => 'ASC']], + 'type' => 'grid', + ]), 'alice', true); + + $this->assertSame('grid', $updated->getType()); + $this->assertSame([['columnId' => 3, 'mode' => 'ASC']], $updated->getSortArray()); + } + + public function testUpdateRejectsAnUnknownType(): void { + $this->expectUpdateOf($this->storedView()); + + $this->expectException(BadRequestError::class); + $this->expectExceptionMessage('Unknown view type'); + $this->service->update(6, ViewUpdateInput::fromInputArray(['type' => 'kanban']), 'alice', true); + } + + public function testUpdateRefusesToTurnATablelessViewIntoATableView(): void { + $this->expectUpdateOf($this->storedView(null)); + + $this->expectException(BadRequestError::class); + $this->expectExceptionMessage('cannot become a table view'); + $this->service->update(6, ViewUpdateInput::fromInputArray(['type' => 'table']), 'alice', true); + } + + public function testUpdateRejectsAMalformedTechnicalName(): void { + $this->expectUpdateOf($this->storedView()); + + $this->expectException(BadRequestError::class); + $this->expectExceptionMessage('Technical name'); + $this->service->update(6, ViewUpdateInput::fromInputArray(['technicalName' => 'Not A Name']), 'alice', true); + } + + public function testUpdateStoresAValidTechnicalNameAndType(): void { + $view = $this->storedView(); + $this->expectUpdateOf($view); + + $updated = $this->service->update(6, ViewUpdateInput::fromInputArray(['technicalName' => 'intake_2', 'type' => 'grid']), 'alice', true); + + $this->assertSame('intake_2', $updated->getTechnicalName()); + $this->assertSame('grid', $updated->getType()); + } + + public function testCreateWithoutTableNeedsAGridType(): void { + $this->expectException(BadRequestError::class); + $this->expectExceptionMessage('needs a table'); + $this->service->create('Home', null, null, 'alice', null, null, View::TYPE_TABLE); + } + + public function testCreateWithoutTableNeedsAUser(): void { + $this->expectException(PermissionError::class); + $this->service->create('Home', null, null, '', null, null, View::TYPE_GRID); + } + + public function testCreateWithoutTableIsOwnedByItsCreator(): void { + $this->mapper->method('insert')->willReturnCallback(static function (View $view): View { + $view->setId(6); + return $view; + }); + $this->mapper->method('update')->willReturnArgument(0); + + $created = $this->service->create('Home', '🧩', null, 'alice', 'home', null, View::TYPE_GRID, 'Start here'); + + $this->assertNull($created->getTableId()); + $this->assertSame('alice', $created->getOwnership()); + $this->assertSame('grid', $created->getType()); + $this->assertSame('Start here', $created->getDescription()); + $this->assertSame('home', $created->getTechnicalName()); + } + + public function testCreateWithATableStillChecksTheTablePermission(): void { + $table = new Table(); + $table->setId(4); + $this->permissionsService->method('canManageTable')->willReturn(false); + + $this->expectException(PermissionError::class); + $this->service->create('Home', null, $table, 'alice'); + } +} From 631def22192ec2936bd8a89c9235e44716fc59e4 Mon Sep 17 00:00:00 2001 From: Remko Date: Mon, 12 Oct 2026 11:06:56 +0200 Subject: [PATCH 2/5] fix(views): tell standalone views apart from federated views Federated views have no table_id either, so a missing table alone does not mean a view is standalone. View::isStandalone() checks for no table and no external id, and the guards, the owner fallback and the query for a user's standalone views now use it. - a row update, delete or presence check against a view without a table no longer ends in a TypeError: the view holds no rows - the view type is validated with the rest of the input and is typed as table or grid in the API - the migration no longer makes table_id nullable, it already is Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: Remko --- lib/Activity/ActivityManager.php | 2 +- lib/Db/Row2Mapper.php | 7 +++ lib/Db/View.php | 20 ++++++- lib/Db/ViewMapper.php | 4 +- .../Version2400Date20261012000000.php | 5 +- lib/Model/ViewUpdateInput.php | 16 ++++- lib/ResponseDefinitions.php | 2 +- lib/Service/ColumnService.php | 4 +- lib/Service/ImportService.php | 4 +- lib/Service/RelationService.php | 2 +- lib/Service/RowService.php | 4 +- lib/Service/ViewService.php | 6 +- openapi.json | 6 +- src/types/openapi/openapi.ts | 3 +- .../Db/Row2MapperViewWithoutTableTest.php | 59 +++++++++++++++++++ tests/unit/Db/ViewTest.php | 28 +++++++++ tests/unit/Model/ViewUpdateInputTest.php | 6 ++ tests/unit/Service/ViewServiceTest.php | 8 --- 18 files changed, 155 insertions(+), 31 deletions(-) create mode 100644 tests/unit/Db/Row2MapperViewWithoutTableTest.php diff --git a/lib/Activity/ActivityManager.php b/lib/Activity/ActivityManager.php index a464595fb5..06ef4dc949 100644 --- a/lib/Activity/ActivityManager.php +++ b/lib/Activity/ActivityManager.php @@ -158,7 +158,7 @@ private function createEvent(string $objectType, Row2|Table|View|Column $object, $objectTitle = $object->getTitle(); $table = $object; } elseif ($object instanceof View) { - if ($object->getTableId() === null) { + if ($object->isStandalone()) { // activities are attached to a table, a view without one has no stream return null; } diff --git a/lib/Db/Row2Mapper.php b/lib/Db/Row2Mapper.php index aeb7c59c8c..c6b268d9b8 100644 --- a/lib/Db/Row2Mapper.php +++ b/lib/Db/Row2Mapper.php @@ -768,6 +768,10 @@ private function parseEntities(IResult $result, array $sleeves): array { * @throws InternalError */ public function isRowInViewPresent(int $rowId, View $view, string $userId): bool { + if ($view->getTableId() === null) { + // a view without a table holds no rows + return false; + } return in_array($rowId, $this->getWantedRowIds($userId, $view->getTableId(), $view->getFilterArray())); } @@ -1033,6 +1037,9 @@ public function countRowsForTables(array $tableIds): array { * @return int */ public function countRowsForView(View $view, string $userId): int { + if ($view->getTableId() === null) { + return 0; + } $filter = $view->getFilterArray(); if (empty($filter)) { diff --git a/lib/Db/View.php b/lib/Db/View.php index 7fe177ff3f..f5a28e4e13 100644 --- a/lib/Db/View.php +++ b/lib/Db/View.php @@ -219,15 +219,29 @@ public function setFilterArray(array $array):void { $this->setFilter(\json_encode($array)); } + /** + * @return 'table'|'grid' + */ public function getTypeOrDefault(): string { - return in_array($this->type, self::TYPES, true) ? $this->type : self::TYPE_TABLE; + return $this->type === self::TYPE_GRID ? self::TYPE_GRID : self::TYPE_TABLE; } /** - * A view inherits its ownership from its table; a view without a table is owned by its creator. + * A view of its own, such as a grid view: it has no table and is not a federated view, + * which has no local table either. + */ + public function isStandalone(): bool { + return $this->tableId === null && $this->externalId === null; + } + + /** + * A view inherits its ownership from its table; a standalone view is owned by its creator. */ public function getOwnership(): ?string { - return $this->ownership ?? $this->createdBy; + if ($this->ownership === null && $this->isStandalone()) { + return $this->createdBy; + } + return $this->ownership; } private function getSharePermissions(): ?Permissions { diff --git a/lib/Db/ViewMapper.php b/lib/Db/ViewMapper.php index 78319a3a40..a1fd683cd1 100644 --- a/lib/Db/ViewMapper.php +++ b/lib/Db/ViewMapper.php @@ -90,7 +90,8 @@ public function findMany(array $ids): array { } /** - * Views that live without a table and belong to the given user. + * Views that live without a table and belong to the given user. Federated views have no + * table either, so they are left out by their external id. * * @return View[] * @throws Exception @@ -100,6 +101,7 @@ public function findAllStandaloneOwnedBy(string $userId): array { $qb->select('*') ->from($this->table) ->where($qb->expr()->isNull('table_id')) + ->andWhere($qb->expr()->isNull('external_id')) ->andWhere($qb->expr()->eq('created_by', $qb->createNamedParameter($userId, IQueryBuilder::PARAM_STR))) ->orderBy('id', 'ASC'); return $this->findEntities($qb); diff --git a/lib/Migration/Version2400Date20261012000000.php b/lib/Migration/Version2400Date20261012000000.php index 859dfb0a96..51efddaf11 100644 --- a/lib/Migration/Version2400Date20261012000000.php +++ b/lib/Migration/Version2400Date20261012000000.php @@ -17,7 +17,8 @@ use Override; /** - * Views get a type (table, grid) and may exist without a table. + * Views get a type (table, grid). table_id is already nullable for federated views + * (Version2203Date20260821000000), so a view without a table needs no schema change. */ class Version2400Date20261012000000 extends SimpleMigrationStep { #[Override] @@ -34,8 +35,6 @@ public function changeSchema(IOutput $output, Closure $schemaClosure, array $opt 'length' => 16, ]); } - // A grid view does not need a table - $table->getColumn('table_id')->setNotnull(false); } return $schema; diff --git a/lib/Model/ViewUpdateInput.php b/lib/Model/ViewUpdateInput.php index a43e30cfd2..e0f3168d3b 100644 --- a/lib/Model/ViewUpdateInput.php +++ b/lib/Model/ViewUpdateInput.php @@ -13,6 +13,7 @@ use InvalidArgumentException; use OCA\Tables\AppInfo\Application; use OCA\Tables\Constants\ViewUpdatableParameters; +use OCA\Tables\Db\View; use OCA\Tables\Service\ValueObject\Emoji; use OCA\Tables\Service\ValueObject\Title; use OCA\Tables\Service\ValueObject\ViewColumnInformation; @@ -77,7 +78,7 @@ public function updateDetail(): Generator { * sort?: list, * filter?: list}>>, * sidebarOrder?: int, - * type?: string + * type?: 'table'|'grid' * } $data * @param array $columnsMap */ @@ -106,7 +107,7 @@ public static function fromInputArray(array $data, array $columnsMap = []): self filterSet: isset($data['filter']) ? FilterSet::createFromInputArray($data['filter'], $columnsMap) : null, sortRuleSet: isset($data['sort']) ? SortRuleSet::createFromInputArray($data['sort'], $columnsMap) : null, sidebarOrder: (array_key_exists('sidebarOrder', $data) && $data['sidebarOrder'] !== null) ? (int)$data['sidebarOrder'] : null, - type: self::optionalString($data, 'type'), + type: self::optionalType($data), ); } @@ -123,6 +124,17 @@ private static function optionalString(array $data, string $key): ?string { return $data[$key]; } + /** + * @throws InvalidArgumentException + */ + private static function optionalType(array $data): ?string { + $type = self::optionalString($data, 'type'); + if ($type !== null && !in_array($type, View::TYPES, true)) { + throw new InvalidArgumentException('Unknown view type "' . $type . '", expected one of: ' . implode(', ', View::TYPES) . '.'); + } + return $type; + } + protected static function transformJsonToArrayInPayload(array $input, array $keys): array { $output = $input; foreach ($keys as $targetKey) { diff --git a/lib/ResponseDefinitions.php b/lib/ResponseDefinitions.php index a2a858d788..4c0724fce7 100644 --- a/lib/ResponseDefinitions.php +++ b/lib/ResponseDefinitions.php @@ -45,7 +45,7 @@ * rowsCount: int, * isFederated: bool, * sidebarOrder: int|null, - * type: string, + * type: 'table'|'grid', * } * * @psalm-type TablesTable = array{ diff --git a/lib/Service/ColumnService.php b/lib/Service/ColumnService.php index 50b9b72ea6..dd1eb25596 100644 --- a/lib/Service/ColumnService.php +++ b/lib/Service/ColumnService.php @@ -112,7 +112,7 @@ public function findAllByTable(int $tableId, ?string $userId = null, ?Table $tab * @return Column[] */ public function findAllByManagedView(View $view, string $userId): array { - if ($view->getTableId() === null) { + if ($view->isStandalone()) { return []; } if ($this->permissionsService->canManageView($view, $userId)) { @@ -246,7 +246,7 @@ public function create( $this->logger->error($e->getMessage(), ['exception' => $e]); throw new NotFoundError(static::class . ' - ' . __FUNCTION__ . ': ' . $e->getMessage()); } - if ($view->getTableId() === null) { + if ($view->isStandalone()) { throw new NotFoundError('The view with id = ' . $viewId . ' has no table to add a column to.'); } try { diff --git a/lib/Service/ImportService.php b/lib/Service/ImportService.php index 5e3f446985..63cc725623 100644 --- a/lib/Service/ImportService.php +++ b/lib/Service/ImportService.php @@ -310,7 +310,7 @@ private function getPreviewData(Worksheet $worksheet): array { public function import(?int $tableId, ?int $viewId, string $path, bool $createMissingColumns = true, array $columnsConfig = []): array { if ($viewId !== null) { $view = $this->viewService->find($viewId); - if ($view->getTableId() === null) { + if ($view->isStandalone()) { throw new NotFoundError('The view with id = ' . $viewId . ' has no table to import rows into.'); } if (!$this->permissionsService->canCreateRows($view)) { @@ -403,7 +403,7 @@ public function import(?int $tableId, ?int $viewId, string $path, bool $createMi public function scheduleImport(?int $tableId, ?int $viewId, string $path, bool $createMissingColumns = true, array $columnsConfig = []) { if ($viewId !== null) { $view = $this->viewService->find($viewId); - if ($view->getTableId() === null) { + if ($view->isStandalone()) { throw new NotFoundError('The view with id = ' . $viewId . ' has no table to import rows into.'); } if (!$this->permissionsService->canCreateRows($view)) { diff --git a/lib/Service/RelationService.php b/lib/Service/RelationService.php index 7d9bb242e8..0d5d2ac1ff 100644 --- a/lib/Service/RelationService.php +++ b/lib/Service/RelationService.php @@ -160,7 +160,7 @@ private function getRelationDataForTarget(string $target, Column $column): array $targetColumn = $this->columnMapper->find($settings[Column::RELATION_LABEL_COLUMN]); if ($isView) { $view = $this->viewMapper->find($targetId); - if ($view->getTableId() === null) { + if ($view->isStandalone()) { $this->cacheRelationData[$cacheKey] = []; return []; } diff --git a/lib/Service/RowService.php b/lib/Service/RowService.php index 862a5dbfad..6ede133df8 100644 --- a/lib/Service/RowService.php +++ b/lib/Service/RowService.php @@ -130,7 +130,7 @@ public function findAllByView(int $viewId, string $userId, ?int $limit = null, ? try { if ($this->permissionsService->canReadRowsByElementId($viewId, 'view', $userId)) { $view = $this->viewMapper->find($viewId); - if ($view->getTableId() === null) { + if ($view->isStandalone()) { // a view without a table, such as a grid view, holds no rows return []; } @@ -234,7 +234,7 @@ public function create(?int $tableId, ?int $viewId, RowDataInput|array $data, ?s throw new InternalError(static::class . ' - ' . __FUNCTION__ . ': ' . $e->getMessage()); } - if ($view->getTableId() === null) { + if ($view->isStandalone()) { throw new NotFoundError('The view with id = ' . $viewId . ' has no table to create a row in.'); } diff --git a/lib/Service/ViewService.php b/lib/Service/ViewService.php index f7e5944393..3832b16dd0 100644 --- a/lib/Service/ViewService.php +++ b/lib/Service/ViewService.php @@ -285,7 +285,7 @@ public function update(int $id, ViewUpdateInput $data, ?string $userId = null, b } if ($parameter === ViewUpdatableParameters::TYPE) { - $this->assertTypeValid($value); + // the value is validated by ViewUpdateInput; this rule needs the stored view if ($value === View::TYPE_TABLE && $view->getTableId() === null) { throw new BadRequestError('A view without a table cannot become a table view.'); } @@ -450,7 +450,7 @@ private function enhanceView(View $view, string $userId): void { $this->setIsSharedState($view, $userId); - if ($view->getTableId() === null) { + if ($view->isStandalone()) { return; } @@ -523,7 +523,7 @@ private function setIsSharedState(View $view, string $userId): void { $view->setIsShared(true); $canManageTable = false; try { - if ($view->getTableId() === null) { + if ($view->isStandalone()) { throw new NotFoundError('The view has no table'); } try { diff --git a/openapi.json b/openapi.json index f4dd2ba630..5e739b2b31 100644 --- a/openapi.json +++ b/openapi.json @@ -1261,7 +1261,11 @@ "nullable": true }, "type": { - "type": "string" + "type": "string", + "enum": [ + "table", + "grid" + ] } } } diff --git a/src/types/openapi/openapi.ts b/src/types/openapi/openapi.ts index 875c86775c..e5aa457741 100644 --- a/src/types/openapi/openapi.ts +++ b/src/types/openapi/openapi.ts @@ -1416,7 +1416,8 @@ export type components = { readonly isFederated: boolean; /** Format: int64 */ readonly sidebarOrder: number | null; - readonly type: string; + /** @enum {string} */ + readonly type: "table" | "grid"; }; }; responses: never; diff --git a/tests/unit/Db/Row2MapperViewWithoutTableTest.php b/tests/unit/Db/Row2MapperViewWithoutTableTest.php new file mode 100644 index 0000000000..21092fb0d8 --- /dev/null +++ b/tests/unit/Db/Row2MapperViewWithoutTableTest.php @@ -0,0 +1,59 @@ +createMock(IDBConnection::class); + $db->expects($this->never())->method('getQueryBuilder'); + $rowSleeveMapper = $this->createMock(RowSleeveMapper::class); + $rowSleeveMapper->expects($this->never())->method('countRows'); + + $this->mapper = new Row2Mapper( + 'alice', + $db, + $this->createMock(LoggerInterface::class), + $this->createMock(UserHelper::class), + $rowSleeveMapper, + $this->createMock(ColumnsHelper::class), + $this->createMock(ColumnMapper::class), + ); + } + + private function viewWithoutTable(): View { + $view = new View(); + $view->setId(6); + $view->setTableId(null); + $view->setType(View::TYPE_GRID); + return $view; + } + + public function testNoRowIsPresentInAViewWithoutATable(): void { + $this->assertFalse($this->mapper->isRowInViewPresent(12, $this->viewWithoutTable(), 'alice')); + } + + public function testAViewWithoutATableCountsNoRows(): void { + $this->assertSame(0, $this->mapper->countRowsForView($this->viewWithoutTable(), 'alice')); + } +} diff --git a/tests/unit/Db/ViewTest.php b/tests/unit/Db/ViewTest.php index 57ce3928d7..ba5cb24f50 100644 --- a/tests/unit/Db/ViewTest.php +++ b/tests/unit/Db/ViewTest.php @@ -33,6 +33,34 @@ public function testOwnershipFallsBackToCreatorForViewsWithoutTable(): void { $this->assertSame('table-owner', $view->getOwnership()); } + public function testOwnershipDoesNotFallBackToCreatorForAViewWithATable(): void { + $view = new View(); + $view->setTableId(4); + $view->setCreatedBy('alice'); + + $this->assertNull($view->getOwnership(), 'without the joined table owner the creator must not become the owner'); + } + + public function testOwnershipDoesNotFallBackToCreatorForAFederatedView(): void { + $view = new View(); + $view->setExternalId(17); + $view->setCreatedBy('remote-sharer'); + + $this->assertNull($view->getOwnership()); + } + + public function testOnlyAViewWithoutTableAndExternalIdIsStandalone(): void { + $view = new View(); + $this->assertTrue($view->isStandalone()); + + $view->setExternalId(17); + $this->assertFalse($view->isStandalone(), 'a federated view has no local table but is not standalone'); + + $view->setExternalId(null); + $view->setTableId(4); + $this->assertFalse($view->isStandalone()); + } + public function testJsonSerializeCarriesTypeAndTechnicalName(): void { $view = new View(); $view->setType(View::TYPE_GRID); diff --git a/tests/unit/Model/ViewUpdateInputTest.php b/tests/unit/Model/ViewUpdateInputTest.php index dfd95a18aa..366a7eb513 100644 --- a/tests/unit/Model/ViewUpdateInputTest.php +++ b/tests/unit/Model/ViewUpdateInputTest.php @@ -68,4 +68,10 @@ public function testATypeThatIsNotAStringIsRejected(): void { $this->expectExceptionMessage('"type" must be a string'); ViewUpdateInput::fromInputArray(['type' => ['grid']]); } + + public function testAnUnknownTypeIsRejected(): void { + $this->expectException(InvalidArgumentException::class); + $this->expectExceptionMessage('Unknown view type "kanban"'); + ViewUpdateInput::fromInputArray(['type' => 'kanban']); + } } diff --git a/tests/unit/Service/ViewServiceTest.php b/tests/unit/Service/ViewServiceTest.php index 5a58a0d210..ce55631f15 100644 --- a/tests/unit/Service/ViewServiceTest.php +++ b/tests/unit/Service/ViewServiceTest.php @@ -92,14 +92,6 @@ public function testUpdateWritesEveryParameterToItsOwnColumn(): void { $this->assertSame([['columnId' => 3, 'mode' => 'ASC']], $updated->getSortArray()); } - public function testUpdateRejectsAnUnknownType(): void { - $this->expectUpdateOf($this->storedView()); - - $this->expectException(BadRequestError::class); - $this->expectExceptionMessage('Unknown view type'); - $this->service->update(6, ViewUpdateInput::fromInputArray(['type' => 'kanban']), 'alice', true); - } - public function testUpdateRefusesToTurnATablelessViewIntoATableView(): void { $this->expectUpdateOf($this->storedView(null)); From 2ebf8c716b05182513091361b4c3207313a1d362 Mon Sep 17 00:00:00 2001 From: Remko Date: Mon, 12 Oct 2026 11:23:51 +0200 Subject: [PATCH 3/5] fix(views): name views without a table by their id The unique index on (table_id, technical_name) does not hold for a missing table on MySQL, PostgreSQL and SQLite, and on Oracle it makes a name unique across the whole instance. A view without a table is therefore always named view_: create and update refuse a chosen name, and an import ignores the name from its source. The page a user names is the application's menu item, whose technical name is unique within the application. Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: Remko --- lib/Service/ViewService.php | 13 +++++++++- tests/unit/Service/ViewServiceTest.php | 35 ++++++++++++++++++++++++-- 2 files changed, 45 insertions(+), 3 deletions(-) diff --git a/lib/Service/ViewService.php b/lib/Service/ViewService.php index 3832b16dd0..82f74fc3ac 100644 --- a/lib/Service/ViewService.php +++ b/lib/Service/ViewService.php @@ -45,6 +45,10 @@ * @psalm-import-type TablesView from ResponseDefinitions */ class ViewService extends SuperService { + // The unique index on (table_id, technical_name) treats a missing table differently per database, + // so a view without a table is always named by its id. Its page is named by its menu item instead. + private const STANDALONE_TECHNICAL_NAME_ERROR = 'A view without a table is named by its id. Name its page in the application menu instead.'; + public function __construct( PermissionsService $permissionsService, LoggerInterface $logger, @@ -199,6 +203,9 @@ public function create( if ($table === null && $type === View::TYPE_TABLE) { throw new BadRequestError('A table view needs a table.'); } + if ($table === null && $technicalName !== null) { + throw new BadRequestError(self::STANDALONE_TECHNICAL_NAME_ERROR); + } // security if ($table !== null && !$this->permissionsService->canManageTable($table, $userId)) { @@ -281,6 +288,9 @@ public function update(int $id, ViewUpdateInput $data, ?string $userId = null, b } if ($parameter === ViewUpdatableParameters::TECHNICAL_NAME) { + if ($view->isStandalone()) { + throw new BadRequestError(self::STANDALONE_TECHNICAL_NAME_ERROR); + } $this->assertTechnicalNameValid($value); } @@ -699,7 +709,8 @@ public function importView(?int $tableId, array $view, string $userId): View { $item->setTableId($tableId); $item->setTitle($view['title']); $item->setEmoji($view['emoji']); - $technicalName = $view['technicalName'] ?? null; + // a view without a table is always named by its id, whatever the source called it + $technicalName = $tableId === null ? null : ($view['technicalName'] ?? null); if ($technicalName !== null) { $this->assertTechnicalNameValid($technicalName); $item->setTechnicalName($technicalName); diff --git a/tests/unit/Service/ViewServiceTest.php b/tests/unit/Service/ViewServiceTest.php index ce55631f15..6fe9b7b4b6 100644 --- a/tests/unit/Service/ViewServiceTest.php +++ b/tests/unit/Service/ViewServiceTest.php @@ -136,13 +136,44 @@ public function testCreateWithoutTableIsOwnedByItsCreator(): void { }); $this->mapper->method('update')->willReturnArgument(0); - $created = $this->service->create('Home', '🧩', null, 'alice', 'home', null, View::TYPE_GRID, 'Start here'); + $created = $this->service->create('Home', '🧩', null, 'alice', null, null, View::TYPE_GRID, 'Start here'); $this->assertNull($created->getTableId()); $this->assertSame('alice', $created->getOwnership()); $this->assertSame('grid', $created->getType()); $this->assertSame('Start here', $created->getDescription()); - $this->assertSame('home', $created->getTechnicalName()); + $this->assertSame('view_6', $created->getTechnicalName(), 'named by its id, so it is unique on every database'); + } + + public function testCreateWithoutTableRefusesAChosenTechnicalName(): void { + $this->mapper->expects($this->never())->method('insert'); + + $this->expectException(BadRequestError::class); + $this->expectExceptionMessage('named by its id'); + $this->service->create('Home', null, null, 'alice', 'home', null, View::TYPE_GRID); + } + + public function testUpdateRefusesATechnicalNameOnAViewWithoutTable(): void { + $this->expectUpdateOf($this->storedView(null)); + + $this->expectException(BadRequestError::class); + $this->expectExceptionMessage('named by its id'); + $this->service->update(6, ViewUpdateInput::fromInputArray(['technicalName' => 'home']), 'alice', true); + } + + public function testImportWithoutTableIgnoresTheSourceTechnicalName(): void { + $this->mapper->method('insert')->willReturnCallback(static function (View $view): View { + $view->setId(9); + return $view; + }); + $this->mapper->method('update')->willReturnArgument(0); + + $imported = $this->service->importView(null, [ + 'title' => 'Home', 'emoji' => null, 'description' => '', 'technicalName' => 'home', 'type' => View::TYPE_GRID, + 'createdAt' => '2026-10-12 10:00:00', 'lastEditAt' => '2026-10-12 10:00:00', 'columnSettings' => [], 'sort' => [], 'filter' => [], + ], 'alice'); + + $this->assertSame('view_9', $imported->getTechnicalName()); } public function testCreateWithATableStillChecksTheTablePermission(): void { From 4a25495c7398c412a070e1f184c92bb50dd072b0 Mon Sep 17 00:00:00 2001 From: Remko Date: Tue, 13 Oct 2026 14:41:31 +0200 Subject: [PATCH 4/5] fix(views): address review on views without a table - the row checks use isStandalone(), like the rest of the row code - deleting a user's views without a table lives in ViewService; the listener only calls it - a grid view cannot have a table: refused on create and when changing the type - changing or deleting a row through a view without a table is a 404 instead of a 500 - a view's uuid is unique on its own: duplicates get a fresh uuid in a migration before the index is added, and an import only keeps the source uuid when it is still free Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: Remko --- lib/Db/Row2Mapper.php | 4 +- lib/Listener/UserDeletedListener.php | 25 +----- .../Version2400Date20261013000000.php | 79 ++++++++++++++++++ lib/Service/RowService.php | 7 ++ lib/Service/ViewService.php | 47 ++++++++++- tests/unit/Db/ViewUuidIndexTest.php | 52 ++++++++++++ .../RowServiceViewWithoutTableTest.php | 82 +++++++++++++++++++ tests/unit/Service/ViewServiceTest.php | 69 +++++++++++++++- 8 files changed, 336 insertions(+), 29 deletions(-) create mode 100644 lib/Migration/Version2400Date20261013000000.php create mode 100644 tests/unit/Db/ViewUuidIndexTest.php create mode 100644 tests/unit/Service/RowServiceViewWithoutTableTest.php diff --git a/lib/Db/Row2Mapper.php b/lib/Db/Row2Mapper.php index c6b268d9b8..bbb675107d 100644 --- a/lib/Db/Row2Mapper.php +++ b/lib/Db/Row2Mapper.php @@ -768,7 +768,7 @@ private function parseEntities(IResult $result, array $sleeves): array { * @throws InternalError */ public function isRowInViewPresent(int $rowId, View $view, string $userId): bool { - if ($view->getTableId() === null) { + if ($view->isStandalone()) { // a view without a table holds no rows return false; } @@ -1037,7 +1037,7 @@ public function countRowsForTables(array $tableIds): array { * @return int */ public function countRowsForView(View $view, string $userId): int { - if ($view->getTableId() === null) { + if ($view->isStandalone()) { return 0; } $filter = $view->getFilterArray(); diff --git a/lib/Listener/UserDeletedListener.php b/lib/Listener/UserDeletedListener.php index c63b3547b0..02d8056c79 100644 --- a/lib/Listener/UserDeletedListener.php +++ b/lib/Listener/UserDeletedListener.php @@ -7,11 +7,9 @@ namespace OCA\Tables\Listener; -use OCA\Tables\Db\ViewMapper; use OCA\Tables\Errors\InternalError; use OCA\Tables\Service\TableService; use OCA\Tables\Service\ViewService; -use OCP\DB\Exception; use OCP\EventDispatcher\Event; use OCP\EventDispatcher\IEventListener; use OCP\User\Events\BeforeUserDeletedEvent; @@ -24,7 +22,6 @@ class UserDeletedListener implements IEventListener { public function __construct( private readonly TableService $tableService, private readonly ViewService $viewService, - private readonly ViewMapper $viewMapper, private readonly LoggerInterface $logger, ) { } @@ -47,25 +44,11 @@ public function handle(Event $event): void { } catch (InternalError) { } - $this->deleteViewsWithoutTable($event->getUser()->getUID()); - } - - /** - * Views without a table belong to their creator, so they go with the user. - */ - private function deleteViewsWithoutTable(string $userId): void { + // views without a table belong to their creator, so they go with the user try { - $views = $this->viewMapper->findAllStandaloneOwnedBy($userId); - } catch (Exception $e) { - $this->logger->error('Views without a table of the deleted user could not be loaded', ['exception' => $e]); - return; - } - foreach ($views as $view) { - try { - $this->viewService->deleteByObject($view, $userId); - } catch (InternalError $e) { - $this->logger->error('View ' . $view->getId() . ' of the deleted user could not be deleted', ['exception' => $e]); - } + $this->viewService->deleteAllStandaloneOwnedBy($event->getUser()->getUID()); + } catch (InternalError) { + // logged by the service } } } diff --git a/lib/Migration/Version2400Date20261013000000.php b/lib/Migration/Version2400Date20261013000000.php new file mode 100644 index 0000000000..aa07625cc8 --- /dev/null +++ b/lib/Migration/Version2400Date20261013000000.php @@ -0,0 +1,79 @@ +connection->getQueryBuilder(); + $select->select('id', 'uuid') + ->from('tables_views') + ->where($select->expr()->isNotNull('uuid')) + ->orderBy('id', 'ASC'); + $result = $select->executeQuery(); + $seen = []; + $duplicateIds = []; + while ($row = $result->fetchAssociative()) { + if (isset($seen[$row['uuid']])) { + $duplicateIds[] = (int)$row['id']; + continue; + } + $seen[$row['uuid']] = true; + } + $result->closeCursor(); + + $update = $this->connection->getQueryBuilder(); + $update->update('tables_views') + ->set('uuid', $update->createParameter('uuid')) + ->where($update->expr()->eq('id', $update->createParameter('id'))); + foreach ($duplicateIds as $id) { + $update->setParameter('uuid', Uuid::v7()->toRfc4122()); + $update->setParameter('id', $id, IQueryBuilder::PARAM_INT); + $update->executeStatement(); + } + if ($duplicateIds !== []) { + $output->info(count($duplicateIds) . ' views got a fresh uuid because another view already used theirs'); + } + } + + #[Override] + public function changeSchema(IOutput $output, Closure $schemaClosure, array $options): ?ISchemaWrapper { + /** @var ISchemaWrapper $schema */ + $schema = $schemaClosure(); + + if ($schema->hasTable('tables_views')) { + $table = $schema->getTable('tables_views'); + if (!$table->hasIndex('tables_views_uuid_only_uniq')) { + $table->addUniqueIndex(['uuid'], 'tables_views_uuid_only_uniq'); + } + } + + return $schema; + } +} diff --git a/lib/Service/RowService.php b/lib/Service/RowService.php index 6ede133df8..ad2dce2db9 100644 --- a/lib/Service/RowService.php +++ b/lib/Service/RowService.php @@ -641,6 +641,10 @@ public function updateSet( throw new NotFoundError(static::class . ' - ' . __FUNCTION__ . ': ' . $e->getMessage()); } + if ($view->isStandalone()) { + throw new NotFoundError('The view with id = ' . $viewId . ' has no rows to update.'); + } + // is row in view? if (!$this->row2Mapper->isRowInViewPresent($id, $view, $userId)) { $e = new \Exception('Update row is not allowed.'); @@ -767,6 +771,9 @@ public function delete(int $id, ?int $viewId, string $userId, ?int $tableId = nu $this->logger->error($e->getMessage(), ['exception' => $e]); throw new InternalError(static::class . ' - ' . __FUNCTION__ . ': ' . $e->getMessage()); } + if ($view->isStandalone()) { + throw new NotFoundError('The view with id = ' . $viewId . ' has no rows to delete.'); + } if (!$this->row2Mapper->isRowInViewPresent($id, $view, $userId)) { $e = new \Exception('Update row is not allowed.'); $this->logger->error($e->getMessage(), ['exception' => $e]); diff --git a/lib/Service/ViewService.php b/lib/Service/ViewService.php index 82f74fc3ac..d2c625c448 100644 --- a/lib/Service/ViewService.php +++ b/lib/Service/ViewService.php @@ -203,6 +203,9 @@ public function create( if ($table === null && $type === View::TYPE_TABLE) { throw new BadRequestError('A table view needs a table.'); } + if ($table !== null && $type === View::TYPE_GRID) { + throw new BadRequestError('A grid view cannot have a table.'); + } if ($table === null && $technicalName !== null) { throw new BadRequestError(self::STANDALONE_TECHNICAL_NAME_ERROR); } @@ -295,10 +298,13 @@ public function update(int $id, ViewUpdateInput $data, ?string $userId = null, b } if ($parameter === ViewUpdatableParameters::TYPE) { - // the value is validated by ViewUpdateInput; this rule needs the stored view + // the value is validated by ViewUpdateInput; these rules need the stored view if ($value === View::TYPE_TABLE && $view->getTableId() === null) { throw new BadRequestError('A view without a table cannot become a table view.'); } + if ($value === View::TYPE_GRID && $view->getTableId() !== null) { + throw new BadRequestError('A view with a table cannot become a grid view.'); + } } if ($value instanceof JsonSerializable) { @@ -444,6 +450,28 @@ public function deleteByObject(View $view, ?string $userId = null): View { } } + /** + * Deletes the views without a table that the user owns, such as grid views: no table + * takes them along when the user's tables are deleted. + * + * @throws InternalError + */ + public function deleteAllStandaloneOwnedBy(string $userId): void { + try { + $views = $this->mapper->findAllStandaloneOwnedBy($userId); + } catch (\OCP\DB\Exception $e) { + $this->logger->error('Views without a table of user ' . $userId . ' could not be loaded', ['exception' => $e]); + throw new InternalError($e->getMessage()); + } + foreach ($views as $view) { + try { + $this->deleteByObject($view, $userId); + } catch (InternalError $e) { + $this->logger->error('View ' . $view->getId() . ' of user ' . $userId . ' could not be deleted', ['exception' => $e]); + } + } + } + /** * add some basic values related to this view in context * @@ -705,7 +733,7 @@ public function addColumnToView(View $view, Column $column, ?string $userId = nu */ public function importView(?int $tableId, array $view, string $userId): View { $item = new View(); - $item->setUuid((isset($view['uuid']) && Uuid::isValid($view['uuid'])) ? $view['uuid'] : null); + $item->setUuid($this->freeUuid($view['uuid'] ?? null)); $item->setTableId($tableId); $item->setTitle($view['title']); $item->setEmoji($view['emoji']); @@ -746,6 +774,21 @@ private function buildDefaultTechnicalName(int $viewId): string { return 'view_' . $viewId; } + /** + * The uuid of the source when it is valid and not taken here yet, so an import does not + * create a second view with the same uuid; otherwise null, and the view gets a fresh one. + */ + private function freeUuid(mixed $uuid): ?string { + if (!is_string($uuid) || !Uuid::isValid($uuid)) { + return null; + } + try { + return $this->mapper->findByUuid($uuid) === null ? $uuid : null; + } catch (MultipleObjectsReturnedException|\OCP\DB\Exception) { + return null; + } + } + /** * @throws BadRequestError */ diff --git a/tests/unit/Db/ViewUuidIndexTest.php b/tests/unit/Db/ViewUuidIndexTest.php new file mode 100644 index 0000000000..8525c9de2a --- /dev/null +++ b/tests/unit/Db/ViewUuidIndexTest.php @@ -0,0 +1,52 @@ +cleanupTablesData(); + } + + public function testASecondViewWithoutATableCannotReuseAUuid(): void { + $this->insertViewWithoutTable('First'); + + try { + $this->insertViewWithoutTable('Second'); + $this->fail('A second view with the same uuid was stored'); + } catch (Exception $e) { + $this->assertSame(Exception::REASON_UNIQUE_CONSTRAINT_VIOLATION, $e->getReason()); + } + } + + private function insertViewWithoutTable(string $title): void { + $qb = $this->connection->getQueryBuilder(); + $qb->insert('tables_views') + ->values([ + 'uuid' => $qb->createNamedParameter(self::UUID), + 'title' => $qb->createNamedParameter($title), + 'description' => $qb->createNamedParameter(''), + 'type' => $qb->createNamedParameter('grid'), + 'created_by' => $qb->createNamedParameter('user1'), + 'created_at' => $qb->createNamedParameter(date('Y-m-d H:i:s')), + 'last_edit_by' => $qb->createNamedParameter('user1'), + 'last_edit_at' => $qb->createNamedParameter(date('Y-m-d H:i:s')), + ]) + ->executeStatement(); + } +} diff --git a/tests/unit/Service/RowServiceViewWithoutTableTest.php b/tests/unit/Service/RowServiceViewWithoutTableTest.php new file mode 100644 index 0000000000..d1024d6c45 --- /dev/null +++ b/tests/unit/Service/RowServiceViewWithoutTableTest.php @@ -0,0 +1,82 @@ +createMock(Row2Mapper::class); + $row2Mapper->method('getTableIdForRow')->willReturn(4); + $row2Mapper->method('find')->willReturn($this->createMock(Row2::class)); + $row2Mapper->expects($this->never())->method('isRowInViewPresent'); + + $permissionsService = $this->createMock(PermissionsService::class); + $permissionsService->method('canReadRowsByElementId')->willReturn(true); + $permissionsService->method('canUpdateRowsByViewId')->willReturn(true); + $permissionsService->method('canDeleteRowsByViewId')->willReturn(true); + + $view = new View(); + $view->setId(6); + $view->setTableId(null); + $view->setType(View::TYPE_GRID); + $viewMapper = $this->createMock(ViewMapper::class); + $viewMapper->method('find')->with(6)->willReturn($view); + + $this->service = new RowService( + $this->createMock(LoggerInterface::class), + 'alice', + $permissionsService, + $this->createMock(ColumnMapper::class), + $viewMapper, + $this->createMock(TableMapper::class), + $row2Mapper, + $this->createMock(IEventDispatcher::class), + $this->createMock(ColumnsHelper::class), + $this->createMock(ActivityManager::class), + $this->createMock(NotificationHelper::class), + $this->createMock(IDBConnection::class), + ); + } + + public function testUpdatingARowThroughAViewWithoutATableIsNotFound(): void { + $this->expectException(NotFoundError::class); + $this->expectExceptionMessage('has no rows to update'); + $this->service->updateSet(12, 6, [], 'alice', null); + } + + public function testDeletingARowThroughAViewWithoutATableIsNotFound(): void { + $this->expectException(NotFoundError::class); + $this->expectExceptionMessage('has no rows to delete'); + $this->service->delete(12, 6, 'alice'); + } +} diff --git a/tests/unit/Service/ViewServiceTest.php b/tests/unit/Service/ViewServiceTest.php index 6fe9b7b4b6..a4813e3af6 100644 --- a/tests/unit/Service/ViewServiceTest.php +++ b/tests/unit/Service/ViewServiceTest.php @@ -85,10 +85,10 @@ public function testUpdateWritesEveryParameterToItsOwnColumn(): void { $updated = $this->service->update(6, ViewUpdateInput::fromInputArray([ 'sort' => [['columnId' => 3, 'mode' => 'ASC']], - 'type' => 'grid', + 'type' => 'table', ]), 'alice', true); - $this->assertSame('grid', $updated->getType()); + $this->assertSame('table', $updated->getType()); $this->assertSame([['columnId' => 3, 'mode' => 'ASC']], $updated->getSortArray()); } @@ -112,10 +112,71 @@ public function testUpdateStoresAValidTechnicalNameAndType(): void { $view = $this->storedView(); $this->expectUpdateOf($view); - $updated = $this->service->update(6, ViewUpdateInput::fromInputArray(['technicalName' => 'intake_2', 'type' => 'grid']), 'alice', true); + $updated = $this->service->update(6, ViewUpdateInput::fromInputArray(['technicalName' => 'intake_2', 'type' => 'table']), 'alice', true); $this->assertSame('intake_2', $updated->getTechnicalName()); - $this->assertSame('grid', $updated->getType()); + $this->assertSame('table', $updated->getType()); + } + + public function testUpdateRefusesToTurnAViewWithATableIntoAGridView(): void { + $this->expectUpdateOf($this->storedView(4)); + + $this->expectException(BadRequestError::class); + $this->expectExceptionMessage('cannot become a grid view'); + $this->service->update(6, ViewUpdateInput::fromInputArray(['type' => 'grid']), 'alice', true); + } + + public function testCreateWithATableRefusesAGridType(): void { + $table = new Table(); + $table->setId(4); + $this->mapper->expects($this->never())->method('insert'); + + $this->expectException(BadRequestError::class); + $this->expectExceptionMessage('cannot have a table'); + $this->service->create('Home', null, $table, 'alice', null, null, View::TYPE_GRID); + } + + public function testDeleteAllStandaloneOwnedByDeletesEachOfTheUsersViewsWithoutTable(): void { + $this->mapper->method('findAllStandaloneOwnedBy')->with('alice')->willReturn([$this->storedView(null), $this->storedView(null)]); + $this->permissionsService->method('canManageView')->willReturn(true); + + $this->mapper->expects($this->exactly(2))->method('delete'); + + $this->service->deleteAllStandaloneOwnedBy('alice'); + } + + public function testImportKeepsTheSourceUuidWhenItIsFree(): void { + $this->mapper->method('findByUuid')->willReturn(null); + $this->expectImportInsert(); + + $imported = $this->service->importView(null, $this->importData('0199c8a0-1a2b-7c3d-8e4f-123456789abc'), 'alice'); + + $this->assertSame('0199c8a0-1a2b-7c3d-8e4f-123456789abc', $imported->getUuid()); + } + + public function testImportGivesAViewWhoseUuidIsTakenAFreshOne(): void { + $this->mapper->method('findByUuid')->willReturn($this->storedView(null)); + $this->expectImportInsert(); + + $imported = $this->service->importView(null, $this->importData('0199c8a0-1a2b-7c3d-8e4f-123456789abc'), 'alice'); + + $this->assertNotSame('0199c8a0-1a2b-7c3d-8e4f-123456789abc', $imported->getUuid()); + $this->assertNotNull($imported->getUuid()); + } + + private function expectImportInsert(): void { + $this->mapper->method('insert')->willReturnCallback(static function (View $view): View { + $view->setId(9); + return $view; + }); + $this->mapper->method('update')->willReturnArgument(0); + } + + private function importData(string $uuid): array { + return [ + 'uuid' => $uuid, 'title' => 'Home', 'emoji' => null, 'description' => '', 'type' => View::TYPE_GRID, + 'createdAt' => '2026-10-12 10:00:00', 'lastEditAt' => '2026-10-12 10:00:00', 'columnSettings' => [], 'sort' => [], 'filter' => [], + ]; } public function testCreateWithoutTableNeedsAGridType(): void { From 475a169b4a946cb33eed67e7f7c0a3a035123a47 Mon Sep 17 00:00:00 2001 From: Remko Date: Tue, 13 Oct 2026 15:41:23 +0200 Subject: [PATCH 5/5] fix(views): give federated proxy views their own uuid - a proxy view no longer copies the uuid of the remote view, so one view federated to several users on the same instance does not clash on the uuid index of views; tables keep the source uuid as on main - the migration keeps the uuid on the local view when a proxy shares it, and drops the old (table_id, uuid) index that the uuid-only index covers - the row checks say that federated views read their rows through the federation proxy Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: Remko --- lib/Db/Row2Mapper.php | 5 +++- lib/Federation/FederationProvider.php | 6 ++++- .../Version2400Date20261013000000.php | 22 +++++++++++++---- .../Federation/FederationProviderTest.php | 24 +++++++++++++++++++ 4 files changed, 50 insertions(+), 7 deletions(-) diff --git a/lib/Db/Row2Mapper.php b/lib/Db/Row2Mapper.php index bbb675107d..26c02c1aba 100644 --- a/lib/Db/Row2Mapper.php +++ b/lib/Db/Row2Mapper.php @@ -769,7 +769,8 @@ private function parseEntities(IResult $result, array $sleeves): array { */ public function isRowInViewPresent(int $rowId, View $view, string $userId): bool { if ($view->isStandalone()) { - // a view without a table holds no rows + // A view without a table holds no rows. Federated views read their rows through the + // federation proxy, not here. return false; } return in_array($rowId, $this->getWantedRowIds($userId, $view->getTableId(), $view->getFilterArray())); @@ -1038,6 +1039,8 @@ public function countRowsForTables(array $tableIds): array { */ public function countRowsForView(View $view, string $userId): int { if ($view->isStandalone()) { + // A view without a table holds no rows. Federated views read their rows through the + // federation proxy, not here. return 0; } $filter = $view->getFilterArray(); diff --git a/lib/Federation/FederationProvider.php b/lib/Federation/FederationProvider.php index d9b0155471..a779c68945 100644 --- a/lib/Federation/FederationProvider.php +++ b/lib/Federation/FederationProvider.php @@ -151,7 +151,11 @@ private function buildNodeForFederationShare(ICloudFederationShare $share, array $node->setDescription(''); } - if (property_exists($node, 'uuid')) { + if ($node instanceof View) { + // Every proxy view gets its own uuid: a view's uuid is unique, and one view can be + // federated to several users on the same instance + $node->setUuid(null); + } elseif (property_exists($node, 'uuid')) { $node->setUuid($metaData['uuid'] ?? null); } diff --git a/lib/Migration/Version2400Date20261013000000.php b/lib/Migration/Version2400Date20261013000000.php index aa07625cc8..512cb62ea4 100644 --- a/lib/Migration/Version2400Date20261013000000.php +++ b/lib/Migration/Version2400Date20261013000000.php @@ -20,8 +20,9 @@ /** * A view's uuid is unique on its own, also for views without a table: the index on - * (table_id, uuid) does not hold when table_id is null. Imports used to keep the uuid of the - * source, so views that share one get a fresh uuid first. + * (table_id, uuid) does not hold when table_id is null, so it gives way to an index on uuid alone. + * Imports and federated proxy views used to keep the uuid of their source, so views that share + * one get a fresh uuid first; a local view keeps its own. */ class Version2400Date20261013000000 extends SimpleMigrationStep { public function __construct( @@ -32,21 +33,28 @@ public function __construct( #[Override] public function preSchemaChange(IOutput $output, Closure $schemaClosure, array $options): void { $select = $this->connection->getQueryBuilder(); - $select->select('id', 'uuid') + $select->select('id', 'uuid', 'external_id') ->from('tables_views') ->where($select->expr()->isNotNull('uuid')) ->orderBy('id', 'ASC'); $result = $select->executeQuery(); + $rows = []; + while ($row = $result->fetchAssociative()) { + $rows[] = $row; + } + $result->closeCursor(); + + // A local view keeps its uuid; a federated proxy that copied it from its source gets a fresh one + usort($rows, static fn (array $a, array $b): int => [$a['external_id'] !== null, (int)$a['id']] <=> [$b['external_id'] !== null, (int)$b['id']]); $seen = []; $duplicateIds = []; - while ($row = $result->fetchAssociative()) { + foreach ($rows as $row) { if (isset($seen[$row['uuid']])) { $duplicateIds[] = (int)$row['id']; continue; } $seen[$row['uuid']] = true; } - $result->closeCursor(); $update = $this->connection->getQueryBuilder(); $update->update('tables_views') @@ -72,6 +80,10 @@ public function changeSchema(IOutput $output, Closure $schemaClosure, array $opt if (!$table->hasIndex('tables_views_uuid_only_uniq')) { $table->addUniqueIndex(['uuid'], 'tables_views_uuid_only_uniq'); } + // The uuid-only index covers the old (table_id, uuid) one + if ($table->hasIndex('tables_views_uuid_uniq')) { + $table->dropIndex('tables_views_uuid_uniq'); + } } return $schema; diff --git a/tests/unit/Federation/FederationProviderTest.php b/tests/unit/Federation/FederationProviderTest.php index 07336a3d1e..11107cc2f5 100644 --- a/tests/unit/Federation/FederationProviderTest.php +++ b/tests/unit/Federation/FederationProviderTest.php @@ -127,6 +127,30 @@ public function testShareReceivedCreatesView(): void { $this->assertEquals('99', $result); } + public function testAProxyViewGetsItsOwnUuidAndAProxyTableKeepsTheSourceOne(): void { + $sourceUuid = '0199c8a0-1a2b-7c3d-8e4f-123456789abc'; + $stored = []; + foreach (['view' => $this->viewMapper, 'table' => $this->tableMapper] as $nodeType => $mapper) { + $share = $this->createMock(ICloudFederationShare::class); + $share->method('getShareWith')->willReturn('admin'); + $share->method('getResourceName')->willReturn('Federated ' . $nodeType); + $share->method('getProviderId')->willReturn('5'); + $share->method('getShareSecret')->willReturn('secret-' . $nodeType); + $share->method('getDescription')->willReturn(json_encode(['nodeType' => $nodeType, 'uuid' => $sourceUuid])); + $mapper->method('insert')->willReturnCallback(static function (Table|View $node) use (&$stored, $nodeType): Table|View { + $node->setId(7); + $stored[$nodeType] = $node->getUuid(); + return $node; + }); + $this->provider->shareReceived($share); + } + + // Two proxies of one view on this instance would otherwise clash on the uuid index of views + $this->assertNotNull($stored['view']); + $this->assertNotSame($sourceUuid, $stored['view']); + $this->assertSame($sourceUuid, $stored['table']); + } + public function testNotificationReceivedPermissionUpdate(): void { $share = new Share(); $share->setNodeId(1);