From c827e5e35adf2e85a9100724e4427811caf83c72 Mon Sep 17 00:00:00 2001 From: Remko Date: Mon, 12 Oct 2026 10:08:13 +0200 Subject: [PATCH 1/5] feat(views): store a grid of widgets on grid views Grid views keep their widgets and layout in a new grid column. The widget types (header, text and data) and the schemas of their configuration and content are defined on the server, and every grid is validated against them before it is stored. - unknown properties are dropped and missing ones get their default - colors must be hex values - a data widget cannot point at another view without a table - grids are validated on update and on import Co-authored-by: Ruben van der Linde Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: Remko --- lib/Constants/ViewUpdatableParameters.php | 1 + lib/Db/View.php | 54 +++ .../Version2400Date20261012010000.php | 40 +++ lib/Model/ViewUpdateInput.php | 23 +- lib/ResponseDefinitions.php | 17 + lib/Service/GridWidgetTypes.php | 323 ++++++++++++++++++ lib/Service/ViewService.php | 10 +- openapi.json | 88 ++++- src/types/openapi/openapi.ts | 26 ++ tests/unit/Db/ViewTest.php | 44 ++- tests/unit/Model/ViewUpdateInputTest.php | 15 +- tests/unit/Service/GridWidgetTypesTest.php | 203 +++++++++++ tests/unit/Service/ViewServiceTest.php | 48 +++ 13 files changed, 884 insertions(+), 8 deletions(-) create mode 100644 lib/Migration/Version2400Date20261012010000.php create mode 100644 lib/Service/GridWidgetTypes.php create mode 100644 tests/unit/Service/GridWidgetTypesTest.php diff --git a/lib/Constants/ViewUpdatableParameters.php b/lib/Constants/ViewUpdatableParameters.php index 6e97996ce8..4fdb331c84 100644 --- a/lib/Constants/ViewUpdatableParameters.php +++ b/lib/Constants/ViewUpdatableParameters.php @@ -19,4 +19,5 @@ enum ViewUpdatableParameters: string { case TECHNICAL_NAME = 'technicalName'; case SIDEBAR_ORDER = 'sidebarOrder'; case TYPE = 'type'; + case GRID = 'grid'; } diff --git a/lib/Db/View.php b/lib/Db/View.php index 7fe177ff3f..bc6edd6e25 100644 --- a/lib/Db/View.php +++ b/lib/Db/View.php @@ -34,6 +34,8 @@ * @method setTableId(?int $tableId) * @method getType(): string * @method setType(string $type) + * @method getGrid(): ?string + * @method setGrid(?string $grid) * @method getColumns(): string * @method setColumns(string $columns) * @method getColumnSettings(): string @@ -95,6 +97,7 @@ class View extends EntitySuper implements JsonSerializable { protected ?string $sort = null; // json protected ?string $filter = null; // json protected ?string $type = null; + protected ?string $grid = null; // json protected ?int $externalId = null; protected ?string $shareToken = null; @@ -219,6 +222,56 @@ public function setFilterArray(array $array):void { $this->setFilter(\json_encode($array)); } + /** + * The widgets of a grid view and where they sit. + * + * @return array{widgets: list>, layout: list>} + */ + public function getGridArray(): array { + $grid = $this->getArray($this->getGrid()); + $widgets = []; + foreach (is_array($grid['widgets'] ?? null) ? $grid['widgets'] : [] as $widget) { + if (is_array($widget)) { + $widgets[] = self::upgradeWidgetShape($widget); + } + } + $layout = []; + foreach (is_array($grid['layout'] ?? null) ? $grid['layout'] : [] as $item) { + if (is_array($item) && is_string($item['widgetId'] ?? null)) { + // a layout item is identified by its widget + $item['id'] = $item['widgetId']; + $layout[] = $item; + } + } + return ['widgets' => $widgets, 'layout' => $layout]; + } + + /** + * Grids saved before widgets had a configuration carried title and showTitle on the + * widget itself; they read as the current shape until saved again. + * + * @param array $widget + * @return array + */ + private static function upgradeWidgetShape(array $widget): array { + $configuration = is_array($widget['configuration'] ?? null) ? $widget['configuration'] : []; + foreach (['title', 'showTitle'] as $legacy) { + if (array_key_exists($legacy, $widget)) { + if (!array_key_exists($legacy, $configuration)) { + $configuration[$legacy] = $widget[$legacy]; + } + unset($widget[$legacy]); + } + } + $widget['configuration'] = $configuration; + $widget['content'] = is_array($widget['content'] ?? null) ? $widget['content'] : []; + return $widget; + } + + public function setGridArray(array $array): void { + $this->setGrid(\json_encode($array, JSON_THROW_ON_ERROR)); + } + public function getTypeOrDefault(): string { return in_array($this->type, self::TYPES, true) ? $this->type : self::TYPE_TABLE; } @@ -263,6 +316,7 @@ public function jsonSerialize(): array { 'isFederated' => $this->isFederated(), 'sidebarOrder' => $this->sidebarOrder, 'type' => $this->getTypeOrDefault(), + 'grid' => $this->getGridArray(), ]; $serialisedJson['filter'] = $this->getFilterArray(); diff --git a/lib/Migration/Version2400Date20261012010000.php b/lib/Migration/Version2400Date20261012010000.php new file mode 100644 index 0000000000..6d5917f367 --- /dev/null +++ b/lib/Migration/Version2400Date20261012010000.php @@ -0,0 +1,40 @@ +hasTable('tables_views')) { + $table = $schema->getTable('tables_views'); + if (!$table->hasColumn('grid')) { + $table->addColumn('grid', Types::TEXT, [ + 'notnull' => false, + 'default' => null, + ]); + } + } + + return $schema; + } +} diff --git a/lib/Model/ViewUpdateInput.php b/lib/Model/ViewUpdateInput.php index a43e30cfd2..4ef899200d 100644 --- a/lib/Model/ViewUpdateInput.php +++ b/lib/Model/ViewUpdateInput.php @@ -33,6 +33,7 @@ public function __construct( protected readonly ?SortRuleSet $sortRuleSet = null, protected readonly ?int $sidebarOrder = null, protected readonly ?string $type = null, + protected readonly ?array $grid = null, ) { } @@ -64,6 +65,9 @@ public function updateDetail(): Generator { if ($this->type !== null) { yield ViewUpdatableParameters::TYPE => $this->type; } + if ($this->grid !== null) { + yield ViewUpdatableParameters::GRID => $this->grid; + } } /** @@ -77,12 +81,13 @@ public function updateDetail(): Generator { * sort?: list, * filter?: list}>>, * sidebarOrder?: int, - * type?: string + * type?: string, + * grid?: array{widgets?: list>, layout?: list>} * } $data * @param array $columnsMap */ public static function fromInputArray(array $data, array $columnsMap = []): self { - $data = self::transformJsonToArrayInPayload($data, ['columnSettings', 'filter', 'sort']); + $data = self::transformJsonToArrayInPayload($data, ['columnSettings', 'filter', 'sort', 'grid']); if (isset($data['columns']) && !isset($data['columnSettings'])) { $logger = Server::get(LoggerInterface::class); @@ -107,6 +112,7 @@ public static function fromInputArray(array $data, array $columnsMap = []): self 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'), + grid: self::optionalArray($data, 'grid'), ); } @@ -123,6 +129,19 @@ private static function optionalString(array $data, string $key): ?string { return $data[$key]; } + /** + * @throws InvalidArgumentException + */ + private static function optionalArray(array $data, string $key): ?array { + if (!isset($data[$key])) { + return null; + } + if (!is_array($data[$key])) { + throw new InvalidArgumentException('The view property "' . $key . '" must be an object.'); + } + 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 a2a858d788..e4b1ad7e9b 100644 --- a/lib/ResponseDefinitions.php +++ b/lib/ResponseDefinitions.php @@ -13,6 +13,22 @@ namespace OCA\Tables; /** + * @psalm-type TablesGridWidget = array{ + * id: string, + * type: string, + * configuration: array, + * content: array, + * } + * + * @psalm-type TablesGridLayoutItem = array{ + * id: string, + * widgetId: string, + * gridX: int, + * gridY: int, + * gridWidth: int, + * gridHeight: int, + * } + * * @psalm-type TablesView = array{ * id: int, * uuid: string, @@ -46,6 +62,7 @@ * isFederated: bool, * sidebarOrder: int|null, * type: string, + * grid: array{widgets: list, layout: list}, * } * * @psalm-type TablesTable = array{ diff --git a/lib/Service/GridWidgetTypes.php b/lib/Service/GridWidgetTypes.php new file mode 100644 index 0000000000..d3501b86d6 --- /dev/null +++ b/lib/Service/GridWidgetTypes.php @@ -0,0 +1,323 @@ +, required?: bool} + * @psalm-type GridWidgetType = array{type: string, title: string, defaultWidth: int, defaultHeight: int, configuration: array, properties: array} + * @psalm-type GridWidget = array{id: string, type: string, configuration: array, content: array} + * @psalm-type GridLayoutItem = array{id: string, widgetId: string, gridX: int, gridY: int, gridWidth: int, gridHeight: int} + */ +class GridWidgetTypes { + public const TYPE_HEADER = 'header'; + public const TYPE_TEXT = 'text'; + public const TYPE_DATA = 'data'; + + public const PROPERTY_TYPES = ['string', 'text', 'color', 'boolean', 'integer', 'enum', 'target']; + + private const HEX_COLOR = '/^#[0-9a-fA-F]{3,6}$/'; + private const LAYOUT_POSITIONS = ['gridX', 'gridY', 'gridWidth', 'gridHeight']; + + public function __construct( + private readonly IL10N $l10n, + private readonly ViewMapper $viewMapper, + ) { + } + + /** + * @return array + */ + public function all(): array { + return [ + self::TYPE_HEADER => [ + 'type' => self::TYPE_HEADER, + 'title' => $this->l10n->t('Header'), + 'defaultWidth' => 12, + 'defaultHeight' => 2, + 'configuration' => $this->titleConfiguration(false), + 'properties' => [ + 'title' => ['type' => 'string', 'title' => $this->l10n->t('Heading'), 'default' => '', 'required' => true], + 'subtitle' => ['type' => 'string', 'title' => $this->l10n->t('Subheading'), 'default' => ''], + 'textAlign' => ['type' => 'enum', 'title' => $this->l10n->t('Text alignment'), 'default' => 'left', 'options' => [ + ['value' => 'left', 'label' => $this->l10n->t('Start')], + ['value' => 'center', 'label' => $this->l10n->t('Centered')], + ['value' => 'right', 'label' => $this->l10n->t('End')], + ]], + 'backgroundColor' => ['type' => 'color', 'title' => $this->l10n->t('Background color'), 'default' => ''], + 'textColor' => ['type' => 'color', 'title' => $this->l10n->t('Text color'), 'default' => ''], + ], + ], + self::TYPE_TEXT => [ + 'type' => self::TYPE_TEXT, + 'title' => $this->l10n->t('Description'), + 'defaultWidth' => 6, + 'defaultHeight' => 2, + 'configuration' => $this->titleConfiguration(true), + 'properties' => [ + 'text' => ['type' => 'text', 'title' => $this->l10n->t('Text'), 'default' => ''], + ], + ], + self::TYPE_DATA => [ + 'type' => self::TYPE_DATA, + 'title' => $this->l10n->t('Table or view'), + 'defaultWidth' => 12, + 'defaultHeight' => 5, + 'configuration' => $this->titleConfiguration(false), + 'properties' => [ + 'target' => ['type' => 'target', 'title' => $this->l10n->t('Table or view'), 'default' => null, 'required' => true], + ], + ], + ]; + } + + /** + * @return array + */ + private function titleConfiguration(bool $showTitle): array { + return [ + 'title' => ['type' => 'string', 'title' => $this->l10n->t('Title'), 'default' => ''], + 'showTitle' => ['type' => 'boolean', 'title' => $this->l10n->t('Show the title'), 'default' => $showTitle], + ]; + } + + /** + * @return GridWidgetType + * @throws BadRequestError + */ + public function get(string $type): array { + $types = $this->all(); + if (!isset($types[$type])) { + throw new BadRequestError('Unknown widget type "' . $type . '", expected one of: ' . implode(', ', array_keys($types))); + } + return $types[$type]; + } + + /** + * Validates a grid and returns it with every widget reduced to what its schemas allow. + * + * @return array{widgets: list, layout: list} + * @throws BadRequestError + */ + public function sanitizeGrid(mixed $grid): array { + if (!is_array($grid)) { + throw new BadRequestError('The grid must be an object.'); + } + foreach (['widgets', 'layout'] as $key) { + if (isset($grid[$key]) && (!is_array($grid[$key]) || !array_is_list($grid[$key]))) { + throw new BadRequestError('The grid property "' . $key . '" must be a list.'); + } + } + + $widgets = []; + foreach ($grid['widgets'] ?? [] as $widget) { + if (!is_array($widget)) { + throw new BadRequestError('Every widget needs a string id and type.'); + } + $sanitized = $this->sanitizeWidget($widget); + if (isset($widgets[$sanitized['id']])) { + throw new BadRequestError('Two widgets share the id ' . $sanitized['id'] . '.'); + } + $widgets[$sanitized['id']] = $sanitized; + } + $this->assertTargetsAreNotGridViews($widgets); + + $layout = []; + foreach ($grid['layout'] ?? [] as $item) { + $layout[] = $this->sanitizeLayoutItem($item, $widgets); + } + + return ['widgets' => array_values($widgets), 'layout' => $layout]; + } + + /** + * Returns a widget in the stored shape. A widget saved before the configuration existed + * carried title and showTitle on itself; they move into the configuration. + * + * @param array $widget + * @return GridWidget + * @throws BadRequestError + */ + public function sanitizeWidget(array $widget): array { + if (!is_string($widget['id'] ?? null) || $widget['id'] === '' || !is_string($widget['type'] ?? null)) { + throw new BadRequestError('Every widget needs a string id and type.'); + } + $configuration = $widget['configuration'] ?? []; + if (!is_array($configuration)) { + throw new BadRequestError('The configuration of widget ' . $widget['id'] . ' must be an object.'); + } + foreach (['title', 'showTitle'] as $legacy) { + if (array_key_exists($legacy, $widget) && !array_key_exists($legacy, $configuration)) { + $configuration[$legacy] = $widget[$legacy]; + } + } + $content = $widget['content'] ?? []; + if (!is_array($content)) { + throw new BadRequestError('The content of widget ' . $widget['id'] . ' must be an object.'); + } + $type = $this->get($widget['type']); + if ($widget['type'] === self::TYPE_DATA && !isset($content['target']) && isset($content['targetType'], $content['targetId'])) { + $content['target'] = ['type' => $content['targetType'], 'id' => (int)$content['targetId']]; + } + return [ + 'id' => $widget['id'], + 'type' => $widget['type'], + 'configuration' => $this->sanitizeProperties($widget['type'], $type['configuration'], $configuration), + 'content' => $this->sanitizeProperties($widget['type'], $type['properties'], $content), + ]; + } + + /** + * @param array $widgets + * @return GridLayoutItem + * @throws BadRequestError + */ + private function sanitizeLayoutItem(mixed $item, array $widgets): array { + if (!is_array($item) || !is_string($item['widgetId'] ?? null)) { + throw new BadRequestError('Every layout item needs the id of its widget.'); + } + if (!isset($widgets[$item['widgetId']])) { + throw new BadRequestError('The layout places widget ' . $item['widgetId'] . ', which the grid does not have.'); + } + foreach (self::LAYOUT_POSITIONS as $position) { + if (!is_int($item[$position] ?? null) || $item[$position] < 0) { + throw new BadRequestError('The layout position "' . $position . '" must be a non-negative integer.'); + } + } + return [ + 'id' => $item['widgetId'], + 'widgetId' => $item['widgetId'], + 'gridX' => $item['gridX'], + 'gridY' => $item['gridY'], + 'gridWidth' => $item['gridWidth'], + 'gridHeight' => $item['gridHeight'], + ]; + } + + /** + * A data widget shows rows, so it cannot point at another grid view. + * + * @param array $widgets + * @throws BadRequestError + */ + private function assertTargetsAreNotGridViews(array $widgets): void { + $viewIds = []; + foreach ($widgets as $widget) { + $target = $widget['content']['target'] ?? null; + if (is_array($target) && $target['type'] === 'view') { + $viewIds[] = $target['id']; + } + } + if ($viewIds === []) { + return; + } + foreach ($this->viewMapper->findMany($viewIds) as $view) { + if ($view->getTableId() === null) { + throw new BadRequestError('A table or view widget cannot show the view ' . $view->getId() . ', which has no table.'); + } + } + } + + /** + * Every property checked against its schema, defaults filled in and unknown properties dropped. + * + * @param array $schema + * @param array $values + * @return array + * @throws BadRequestError + */ + private function sanitizeProperties(string $type, array $schema, array $values): array { + $sanitized = []; + foreach ($schema as $name => $property) { + $value = array_key_exists($name, $values) ? $values[$name] : $property['default']; + $sanitized[$name] = $this->sanitizeProperty($type . '.' . $name, $property, $value); + } + return $sanitized; + } + + /** + * @param GridWidgetProperty $property + * @throws BadRequestError + */ + private function sanitizeProperty(string $label, array $property, mixed $value): mixed { + $isRequired = $property['required'] ?? false; + return match ($property['type']) { + 'string', 'text' => $this->sanitizeText($label, $value, $isRequired), + 'color' => $this->sanitizeColor($label, $value), + 'boolean' => is_bool($value) ? $value : throw new BadRequestError('Widget property ' . $label . ' must be true or false.'), + 'integer' => is_int($value) ? $value : throw new BadRequestError('Widget property ' . $label . ' must be an integer.'), + 'enum' => $this->sanitizeEnum($label, $value, array_column($property['options'] ?? [], 'value')), + 'target' => $this->sanitizeTarget($label, $value, $isRequired), + default => throw new BadRequestError('Widget property ' . $label . ' has an unknown schema type.'), + }; + } + + /** + * @throws BadRequestError + */ + private function sanitizeText(string $label, mixed $value, bool $isRequired): string { + $value ??= ''; + if (!is_string($value)) { + throw new BadRequestError('Widget property ' . $label . ' must be a string.'); + } + if ($isRequired && trim($value) === '') { + throw new BadRequestError('Widget property ' . $label . ' is required.'); + } + return $value; + } + + /** + * @throws BadRequestError + */ + private function sanitizeColor(string $label, mixed $value): string { + if ($value === null || $value === '') { + return ''; + } + if (!is_string($value) || !preg_match(self::HEX_COLOR, $value)) { + throw new BadRequestError('Widget property ' . $label . ' must be a hex color.'); + } + return $value; + } + + /** + * @param list $allowed + * @throws BadRequestError + */ + private function sanitizeEnum(string $label, mixed $value, array $allowed): mixed { + if (!in_array($value, $allowed, true)) { + throw new BadRequestError('Widget property ' . $label . ' must be one of: ' . implode(', ', $allowed) . '.'); + } + return $value; + } + + /** + * @return array{type: string, id: int}|null + * @throws BadRequestError + */ + private function sanitizeTarget(string $label, mixed $value, bool $isRequired): ?array { + if ($value === null) { + if ($isRequired) { + throw new BadRequestError('Widget property ' . $label . ' is required.'); + } + return null; + } + if (!is_array($value) || !in_array($value['type'] ?? null, ['table', 'view'], true) || !is_int($value['id'] ?? null) || $value['id'] <= 0) { + throw new BadRequestError('Widget property ' . $label . ' must name a table or view by id.'); + } + return ['type' => $value['type'], 'id' => $value['id']]; + } +} diff --git a/lib/Service/ViewService.php b/lib/Service/ViewService.php index f7e5944393..ec92f0081e 100644 --- a/lib/Service/ViewService.php +++ b/lib/Service/ViewService.php @@ -59,6 +59,7 @@ public function __construct( protected IL10N $l, private readonly FederationService $federationService, private readonly ActivityManager $activityManager, + private readonly GridWidgetTypes $gridWidgetTypes, ) { parent::__construct($logger, $userId, $permissionsService); } @@ -291,7 +292,11 @@ public function update(int $id, ViewUpdateInput $data, ?string $userId = null, b } } - if ($value instanceof JsonSerializable) { + if ($parameter === ViewUpdatableParameters::GRID) { + $value = $this->gridWidgetTypes->sanitizeGrid($value); + } + + if ($value instanceof JsonSerializable || is_array($value)) { $insertableValue = json_encode($value); } @@ -716,6 +721,9 @@ public function importView(?int $tableId, array $view, string $userId): View { $type = $view['type'] ?? View::TYPE_TABLE; $this->assertTypeValid($type); $item->setType($type); + if (isset($view['grid'])) { + $item->setGridArray($this->gridWidgetTypes->sanitizeGrid($view['grid'])); + } try { $importedView = $this->mapper->insert($item); if ($item->getTechnicalName() === null || $item->getTechnicalName() === '') { diff --git a/openapi.json b/openapi.json index f4dd2ba630..6aab5f9eb4 100644 --- a/openapi.json +++ b/openapi.json @@ -330,6 +330,70 @@ } } }, + "GridLayoutItem": { + "type": "object", + "required": [ + "id", + "widgetId", + "gridX", + "gridY", + "gridWidth", + "gridHeight" + ], + "properties": { + "id": { + "type": "string" + }, + "widgetId": { + "type": "string" + }, + "gridX": { + "type": "integer", + "format": "int64" + }, + "gridY": { + "type": "integer", + "format": "int64" + }, + "gridWidth": { + "type": "integer", + "format": "int64" + }, + "gridHeight": { + "type": "integer", + "format": "int64" + } + } + }, + "GridWidget": { + "type": "object", + "required": [ + "id", + "type", + "configuration", + "content" + ], + "properties": { + "id": { + "type": "string" + }, + "type": { + "type": "string" + }, + "configuration": { + "type": "object", + "additionalProperties": { + "type": "object" + } + }, + "content": { + "type": "object", + "additionalProperties": { + "type": "object" + } + } + } + }, "ImportState": { "type": "object", "required": [ @@ -1015,7 +1079,8 @@ "rowsCount", "isFederated", "sidebarOrder", - "type" + "type", + "grid" ], "properties": { "id": { @@ -1262,6 +1327,27 @@ }, "type": { "type": "string" + }, + "grid": { + "type": "object", + "required": [ + "widgets", + "layout" + ], + "properties": { + "widgets": { + "type": "array", + "items": { + "$ref": "#/components/schemas/GridWidget" + } + }, + "layout": { + "type": "array", + "items": { + "$ref": "#/components/schemas/GridLayoutItem" + } + } + } } } } diff --git a/src/types/openapi/openapi.ts b/src/types/openapi/openapi.ts index 875c86775c..cee97d2e5f 100644 --- a/src/types/openapi/openapi.ts +++ b/src/types/openapi/openapi.ts @@ -1162,6 +1162,28 @@ export type components = { readonly displayMode: number; readonly userId: string; }; + readonly GridLayoutItem: { + readonly id: string; + readonly widgetId: string; + /** Format: int64 */ + readonly gridX: number; + /** Format: int64 */ + readonly gridY: number; + /** Format: int64 */ + readonly gridWidth: number; + /** Format: int64 */ + readonly gridHeight: number; + }; + readonly GridWidget: { + readonly id: string; + readonly type: string; + readonly configuration: { + readonly [key: string]: Record; + }; + readonly content: { + readonly [key: string]: Record; + }; + }; readonly ImportState: { /** Format: int64 */ readonly found_columns_count: number; @@ -1417,6 +1439,10 @@ export type components = { /** Format: int64 */ readonly sidebarOrder: number | null; readonly type: string; + readonly grid: { + readonly widgets: readonly components["schemas"]["GridWidget"][]; + readonly layout: readonly components["schemas"]["GridLayoutItem"][]; + }; }; }; responses: never; diff --git a/tests/unit/Db/ViewTest.php b/tests/unit/Db/ViewTest.php index 57ce3928d7..3ae6b923c3 100644 --- a/tests/unit/Db/ViewTest.php +++ b/tests/unit/Db/ViewTest.php @@ -13,6 +13,46 @@ use PHPUnit\Framework\TestCase; class ViewTest extends TestCase { + public function testGridArrayIsEmptyWhenNothingIsStored(): void { + $view = new View(); + $this->assertSame(['widgets' => [], 'layout' => []], $view->getGridArray()); + + $view->setGrid('null'); + $this->assertSame(['widgets' => [], 'layout' => []], $view->getGridArray()); + } + + public function testGridArrayDropsUnknownKeysAndNonLists(): void { + $view = new View(); + $view->setGrid(json_encode(['widgets' => 'oops', 'layout' => [['widgetId' => 'w-1']], 'extra' => true])); + + $this->assertSame(['widgets' => [], 'layout' => [['widgetId' => 'w-1', 'id' => 'w-1']]], $view->getGridArray()); + } + + public function testGridArrayRoundTrips(): void { + $grid = [ + 'widgets' => [['id' => 'w-1', 'type' => 'header', 'configuration' => ['title' => 'Hi', 'showTitle' => false], 'content' => ['title' => 'Hi']]], + 'layout' => [['id' => 'w-1', 'widgetId' => 'w-1', 'gridX' => 0, 'gridY' => 0, 'gridWidth' => 12, 'gridHeight' => 2]], + ]; + $view = new View(); + $view->setGridArray($grid); + + $this->assertSame($grid, $view->getGridArray()); + } + + public function testGridArrayIdentifiesLayoutItemsSavedWithANumberByTheirWidget(): void { + $view = new View(); + $view->setGridArray(['widgets' => [], 'layout' => [['id' => 1, 'widgetId' => 'w-1', 'gridX' => 0], ['gridX' => 0]]]); + + $this->assertSame([['id' => 'w-1', 'widgetId' => 'w-1', 'gridX' => 0]], $view->getGridArray()['layout']); + } + + public function testGridArrayUpgradesWidgetsSavedBeforeTheConfiguration(): void { + $view = new View(); + $view->setGrid(json_encode(['widgets' => [['id' => 'w-1', 'type' => 'text', 'title' => 'How', 'showTitle' => true, 'content' => ['text' => 'x']]], 'layout' => []])); + + $this->assertSame([['id' => 'w-1', 'type' => 'text', 'content' => ['text' => 'x'], 'configuration' => ['title' => 'How', 'showTitle' => true]]], $view->getGridArray()['widgets']); + } + public function testTypeFallsBackToTable(): void { $view = new View(); $this->assertSame(View::TYPE_TABLE, $view->getTypeOrDefault()); @@ -33,16 +73,18 @@ public function testOwnershipFallsBackToCreatorForViewsWithoutTable(): void { $this->assertSame('table-owner', $view->getOwnership()); } - public function testJsonSerializeCarriesTypeAndTechnicalName(): void { + public function testJsonSerializeCarriesTypeGridAndTechnicalName(): void { $view = new View(); $view->setType(View::TYPE_GRID); $view->setTechnicalName('home'); $view->setCreatedBy('alice'); + $view->setGridArray(['widgets' => [], 'layout' => []]); $json = $view->jsonSerialize(); $this->assertSame('grid', $json['type']); $this->assertSame('home', $json['technicalName']); + $this->assertSame(['widgets' => [], 'layout' => []], $json['grid']); $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 dfd95a18aa..8623e1b859 100644 --- a/tests/unit/Model/ViewUpdateInputTest.php +++ b/tests/unit/Model/ViewUpdateInputTest.php @@ -39,8 +39,9 @@ public function testUpdateDetailIncludesEmptyDescription(): void { $this->assertSame('', $updates[ViewUpdatableParameters::DESCRIPTION->value]); } - public function testUpdateDetailIncludesTypeAndTechnicalName(): void { - $input = ViewUpdateInput::fromInputArray(['type' => 'grid', 'technicalName' => 'home']); + public function testUpdateDetailIncludesTypeGridAndTechnicalName(): void { + $grid = ['widgets' => [['id' => 'w-1', 'type' => 'header']], 'layout' => [['id' => 1, 'widgetId' => 'w-1', 'gridX' => 0, 'gridY' => 0, 'gridWidth' => 12, 'gridHeight' => 2]]]; + $input = ViewUpdateInput::fromInputArray(['type' => 'grid', 'grid' => json_encode($grid), 'technicalName' => 'home']); $updates = []; foreach ($input->updateDetail() as $parameter => $value) { @@ -48,10 +49,11 @@ public function testUpdateDetailIncludesTypeAndTechnicalName(): void { } $this->assertSame('grid', $updates[ViewUpdatableParameters::TYPE->value]); + $this->assertSame($grid, $updates[ViewUpdatableParameters::GRID->value]); $this->assertSame('home', $updates[ViewUpdatableParameters::TECHNICAL_NAME->value]); } - public function testUpdateDetailSkipsTypeWhenAbsent(): void { + public function testUpdateDetailSkipsGridWhenAbsent(): void { $input = ViewUpdateInput::fromInputArray(['title' => 'Only a title']); $updates = []; @@ -59,6 +61,7 @@ public function testUpdateDetailSkipsTypeWhenAbsent(): void { $updates[$parameter->value] = $value; } + $this->assertArrayNotHasKey(ViewUpdatableParameters::GRID->value, $updates); $this->assertArrayNotHasKey(ViewUpdatableParameters::TYPE->value, $updates); $this->assertArrayNotHasKey(ViewUpdatableParameters::TECHNICAL_NAME->value, $updates); } @@ -68,4 +71,10 @@ public function testATypeThatIsNotAStringIsRejected(): void { $this->expectExceptionMessage('"type" must be a string'); ViewUpdateInput::fromInputArray(['type' => ['grid']]); } + + public function testAGridThatIsNotAnObjectIsRejected(): void { + $this->expectException(InvalidArgumentException::class); + $this->expectExceptionMessage('"grid" must be an object'); + ViewUpdateInput::fromInputArray(['grid' => 'not json']); + } } diff --git a/tests/unit/Service/GridWidgetTypesTest.php b/tests/unit/Service/GridWidgetTypesTest.php new file mode 100644 index 0000000000..8ef241c871 --- /dev/null +++ b/tests/unit/Service/GridWidgetTypesTest.php @@ -0,0 +1,203 @@ +createMock(IL10N::class); + $l10n->method('t')->willReturnCallback(static fn (string $text): string => $text); + $this->viewMapper = $this->createMock(ViewMapper::class); + $this->types = new GridWidgetTypes($l10n, $this->viewMapper); + } + + public function testEveryTypeDescribesItselfAndItsProperties(): void { + foreach ($this->types->all() as $type => $definition) { + $this->assertSame($type, $definition['type']); + $this->assertNotSame('', $definition['title']); + $this->assertGreaterThan(0, $definition['defaultWidth']); + $this->assertGreaterThan(0, $definition['defaultHeight']); + foreach ($definition['properties'] as $name => $property) { + $this->assertContains($property['type'], GridWidgetTypes::PROPERTY_TYPES, $type . '.' . $name); + $this->assertArrayHasKey('default', $property, $type . '.' . $name); + if ($property['type'] === 'enum') { + $this->assertContains($property['default'], array_column($property['options'], 'value'), $type . '.' . $name); + } + } + } + } + + public function testTitlesAreTranslated(): void { + $l10n = $this->createMock(IL10N::class); + $l10n->method('t')->willReturnCallback(static fn (string $text): string => 'nl:' . $text); + $types = (new GridWidgetTypes($l10n, $this->viewMapper))->all(); + + $this->assertSame('nl:Header', $types['header']['title']); + $this->assertSame('nl:Heading', $types['header']['properties']['title']['title']); + $this->assertSame('nl:Centered', $types['header']['properties']['textAlign']['options'][1]['label']); + } + + public function testEveryTypeHasATitleAndShowTitleConfiguration(): void { + foreach ($this->types->all() as $definition) { + $this->assertSame('string', $definition['configuration']['title']['type']); + $this->assertSame('boolean', $definition['configuration']['showTitle']['type']); + } + } + + public function testSanitizeWidgetSplitsConfigurationFromContent(): void { + $widget = $this->types->sanitizeWidget(['id' => 'w-1', 'type' => 'text', 'configuration' => ['title' => 'How', 'showTitle' => true, 'stray' => 'x'], 'content' => ['text' => 'it works']]); + + $this->assertSame(['id' => 'w-1', 'type' => 'text', 'configuration' => ['title' => 'How', 'showTitle' => true], 'content' => ['text' => 'it works']], $widget); + } + + public function testSanitizeWidgetUpgradesTitleAndShowTitleFromTheOldShape(): void { + $widget = $this->types->sanitizeWidget(['id' => 'w-1', 'type' => 'header', 'title' => 'Old', 'showTitle' => true, 'content' => ['title' => 'Hi']]); + + $this->assertSame(['title' => 'Old', 'showTitle' => true], $widget['configuration']); + $this->assertSame(['id', 'type', 'configuration', 'content'], array_keys($widget)); + } + + public function testSanitizeWidgetFillsTheConfigurationDefaultsOfTheType(): void { + $this->assertFalse($this->types->sanitizeWidget(['id' => 'w-1', 'type' => 'header', 'content' => ['title' => 'Hi']])['configuration']['showTitle']); + $this->assertTrue($this->types->sanitizeWidget(['id' => 'w-2', 'type' => 'text'])['configuration']['showTitle']); + } + + public function testSanitizeWidgetRejectsAWidgetWithoutIdOrType(): void { + $this->expectException(BadRequestError::class); + $this->types->sanitizeWidget(['type' => 'text']); + } + + public function testUnknownTypeIsRejected(): void { + $this->expectException(BadRequestError::class); + $this->expectExceptionMessage('Unknown widget type'); + $this->types->get('kanban'); + } + + public function testSanitizeFillsDefaultsAndDropsUnknownProperties(): void { + $widget = $this->types->sanitizeWidget(['id' => 'w-1', 'type' => 'header', 'content' => ['title' => 'Hi', 'stray' => true]]); + + $this->assertSame(['title' => 'Hi', 'subtitle' => '', 'textAlign' => 'left', 'backgroundColor' => '', 'textColor' => ''], $widget['content']); + } + + public function testSanitizeRejectsAMissingRequiredString(): void { + $this->expectException(BadRequestError::class); + $this->expectExceptionMessage('header.title is required'); + $this->types->sanitizeWidget(['id' => 'w-1', 'type' => 'header', 'content' => ['title' => ' ']]); + } + + public function testSanitizeRejectsANonHexColor(): void { + $this->expectException(BadRequestError::class); + $this->expectExceptionMessage('must be a hex color'); + $this->types->sanitizeWidget(['id' => 'w-1', 'type' => 'header', 'content' => ['title' => 'Hi', 'backgroundColor' => 'red']]); + } + + public function testSanitizeRejectsAnUnknownEnumValue(): void { + $this->expectException(BadRequestError::class); + $this->expectExceptionMessage('textAlign must be one of'); + $this->types->sanitizeWidget(['id' => 'w-1', 'type' => 'header', 'content' => ['title' => 'Hi', 'textAlign' => 'justify']]); + } + + public function testSanitizeRejectsANonBooleanInsteadOfCastingIt(): void { + $this->expectException(BadRequestError::class); + $this->expectExceptionMessage('showTitle must be true or false'); + $this->types->sanitizeWidget(['id' => 'w-1', 'type' => 'text', 'configuration' => ['showTitle' => 1]]); + } + + public function testSanitizeKeepsAValidTarget(): void { + $widget = $this->types->sanitizeWidget(['id' => 'w-1', 'type' => 'data', 'content' => ['target' => ['type' => 'view', 'id' => 6, 'extra' => 1]]]); + + $this->assertSame(['target' => ['type' => 'view', 'id' => 6]], $widget['content']); + } + + public function testSanitizeUpgradesTheOldDataShape(): void { + $widget = $this->types->sanitizeWidget(['id' => 'w-1', 'type' => 'data', 'content' => ['targetType' => 'table', 'targetId' => '4']]); + + $this->assertSame(['target' => ['type' => 'table', 'id' => 4]], $widget['content']); + } + + public function testSanitizeRejectsATargetWithoutId(): void { + $this->expectException(BadRequestError::class); + $this->expectExceptionMessage('name a table or view by id'); + $this->types->sanitizeWidget(['id' => 'w-1', 'type' => 'data', 'content' => ['target' => ['type' => 'table']]]); + } + + public function testSanitizeRejectsAMissingRequiredTarget(): void { + $this->expectException(BadRequestError::class); + $this->expectExceptionMessage('data.target is required'); + $this->types->sanitizeWidget(['id' => 'w-1', 'type' => 'data']); + } + + public function testSanitizeRejectsANonStringText(): void { + $this->expectException(BadRequestError::class); + $this->expectExceptionMessage('text.text must be a string'); + $this->types->sanitizeWidget(['id' => 'w-1', 'type' => 'text', 'content' => ['text' => ['nested']]]); + } + + public function testSanitizeGridIdentifiesLayoutItemsByTheirWidget(): void { + $grid = $this->types->sanitizeGrid([ + 'widgets' => [['id' => 'w-1', 'type' => 'text']], + 'layout' => [['id' => 7, 'widgetId' => 'w-1', 'gridX' => 0, 'gridY' => 0, 'gridWidth' => 6, 'gridHeight' => 2, 'stray' => true]], + ]); + + $this->assertSame([['id' => 'w-1', 'widgetId' => 'w-1', 'gridX' => 0, 'gridY' => 0, 'gridWidth' => 6, 'gridHeight' => 2]], $grid['layout']); + } + + public function testSanitizeGridRejectsALayoutItemWithoutItsWidget(): void { + $this->expectException(BadRequestError::class); + $this->expectExceptionMessage('which the grid does not have'); + $this->types->sanitizeGrid([ + 'widgets' => [], + 'layout' => [['widgetId' => 'w-gone', 'gridX' => 0, 'gridY' => 0, 'gridWidth' => 6, 'gridHeight' => 2]], + ]); + } + + public function testSanitizeGridRejectsTwoWidgetsWithTheSameId(): void { + $this->expectException(BadRequestError::class); + $this->expectExceptionMessage('share the id'); + $this->types->sanitizeGrid(['widgets' => [['id' => 'w-1', 'type' => 'text'], ['id' => 'w-1', 'type' => 'text']]]); + } + + public function testSanitizeGridRejectsANegativePosition(): void { + $this->expectException(BadRequestError::class); + $this->expectExceptionMessage('gridX'); + $this->types->sanitizeGrid([ + 'widgets' => [['id' => 'w-1', 'type' => 'text']], + 'layout' => [['widgetId' => 'w-1', 'gridX' => -1, 'gridY' => 0, 'gridWidth' => 6, 'gridHeight' => 2]], + ]); + } + + public function testSanitizeGridRejectsAWidgetShowingAViewWithoutTable(): void { + $gridView = new View(); + $gridView->setId(9); + $gridView->setTableId(null); + $this->viewMapper->method('findMany')->with([9])->willReturn([9 => $gridView]); + + $this->expectException(BadRequestError::class); + $this->expectExceptionMessage('which has no table'); + $this->types->sanitizeGrid(['widgets' => [['id' => 'w-1', 'type' => 'data', 'content' => ['target' => ['type' => 'view', 'id' => 9]]]]]); + } + + public function testSanitizeGridRejectsWidgetsThatAreNotAList(): void { + $this->expectException(BadRequestError::class); + $this->expectExceptionMessage('"widgets" must be a list'); + $this->types->sanitizeGrid(['widgets' => ['a' => ['id' => 'w-1', 'type' => 'text']]]); + } +} diff --git a/tests/unit/Service/ViewServiceTest.php b/tests/unit/Service/ViewServiceTest.php index 5a58a0d210..a9d65f50a9 100644 --- a/tests/unit/Service/ViewServiceTest.php +++ b/tests/unit/Service/ViewServiceTest.php @@ -20,6 +20,7 @@ use OCA\Tables\Service\ContextService; use OCA\Tables\Service\FavoritesService; use OCA\Tables\Service\FederationService; +use OCA\Tables\Service\GridWidgetTypes; use OCA\Tables\Service\PermissionsService; use OCA\Tables\Service\RowService; use OCA\Tables\Service\ShareService; @@ -60,6 +61,7 @@ protected function setUp(): void { $l10n, $this->createMock(FederationService::class), $this->activityManager, + new GridWidgetTypes($l10n, $this->mapper), ); } @@ -80,6 +82,25 @@ private function expectUpdateOf(View $view): void { $this->mapper->method('update')->willReturnArgument(0); } + public function testUpdateStoresAValidGridAsJson(): void { + $view = $this->storedView(null); + $this->expectUpdateOf($view); + $grid = [ + 'widgets' => [['id' => 'w-1', 'type' => 'header', 'content' => ['title' => 'Hi', 'backgroundColor' => '#abc', 'stray' => 'dropped']]], + 'layout' => [['id' => 1, 'widgetId' => 'w-1', 'gridX' => 0, 'gridY' => 0, 'gridWidth' => 12, 'gridHeight' => 2]], + ]; + + $updated = $this->service->update(6, ViewUpdateInput::fromInputArray(['grid' => json_encode($grid)]), 'alice', true); + + $stored = $updated->getGridArray(); + $this->assertSame([['id' => 'w-1', 'widgetId' => 'w-1', 'gridX' => 0, 'gridY' => 0, 'gridWidth' => 12, 'gridHeight' => 2]], $stored['layout'], 'a layout item is identified by its widget'); + $this->assertSame('Hi', $stored['widgets'][0]['content']['title']); + $this->assertSame('#abc', $stored['widgets'][0]['content']['backgroundColor']); + $this->assertSame('left', $stored['widgets'][0]['content']['textAlign'], 'defaults are filled in'); + $this->assertArrayNotHasKey('stray', $stored['widgets'][0]['content'], 'unknown properties are dropped'); + $this->assertSame(['title' => '', 'showTitle' => false], $stored['widgets'][0]['configuration'], 'the configuration is filled from the type'); + } + public function testUpdateWritesEveryParameterToItsOwnColumn(): void { $this->expectUpdateOf($this->storedView(4)); @@ -92,6 +113,33 @@ public function testUpdateWritesEveryParameterToItsOwnColumn(): void { $this->assertSame([['columnId' => 3, 'mode' => 'ASC']], $updated->getSortArray()); } + public function testUpdateRejectsNonHexWidgetColors(): void { + $this->expectUpdateOf($this->storedView(null)); + $grid = ['widgets' => [['id' => 'w-1', 'type' => 'header', 'content' => ['title' => 'Hi', 'backgroundColor' => 'url(evil)']]], 'layout' => []]; + + $this->expectException(BadRequestError::class); + $this->expectExceptionMessage('hex color'); + $this->service->update(6, ViewUpdateInput::fromInputArray(['grid' => json_encode($grid)]), 'alice', true); + } + + public function testUpdateRejectsLayoutItemsWithoutPositions(): void { + $this->expectUpdateOf($this->storedView(null)); + $grid = ['widgets' => [['id' => 'w-1', 'type' => 'text', 'content' => ['text' => 'x']]], 'layout' => [['widgetId' => 'w-1', 'gridX' => 0]]]; + + $this->expectException(BadRequestError::class); + $this->expectExceptionMessage('gridY'); + $this->service->update(6, ViewUpdateInput::fromInputArray(['grid' => json_encode($grid)]), 'alice', true); + } + + public function testUpdateRejectsWidgetsWithoutIdOrType(): void { + $this->expectUpdateOf($this->storedView(null)); + $grid = ['widgets' => [['type' => 'text']], 'layout' => []]; + + $this->expectException(BadRequestError::class); + $this->expectExceptionMessage('string id and type'); + $this->service->update(6, ViewUpdateInput::fromInputArray(['grid' => json_encode($grid)]), 'alice', true); + } + public function testUpdateRejectsAnUnknownType(): void { $this->expectUpdateOf($this->storedView()); From edc81b9a75dbe2ec2cba8afaa73d4a03989cd1ed Mon Sep 17 00:00:00 2001 From: Remko Date: Mon, 12 Oct 2026 11:46:18 +0200 Subject: [PATCH 2/5] fix(grid): check widget targets and tighten grid validation - a data widget only shows a table or view that exists, holds rows and is readable for the user; every other target gets the same error - only a grid view stores a grid, also when the type changes in the same request; an import only takes the grid over for a grid view - a layout places every widget once and with a size of at least one cell - a grid that cannot be encoded is refused instead of being stored empty - widget values are typed in the API instead of mixed, and the shapes are defined once in ResponseDefinitions - the conversion of a grid shape that only prototype data had is gone Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: Remko --- lib/Db/View.php | 43 +------- lib/ResponseDefinitions.php | 6 +- lib/Service/GridWidgetTypes.php | 120 ++++++++++++--------- lib/Service/ViewService.php | 18 +++- openapi.json | 39 ++++++- src/types/openapi/openapi.ts | 10 +- tests/unit/Db/ViewTest.php | 16 +-- tests/unit/Service/GridWidgetTypesTest.php | 104 +++++++++++++----- tests/unit/Service/ViewServiceTest.php | 28 ++++- 9 files changed, 243 insertions(+), 141 deletions(-) diff --git a/lib/Db/View.php b/lib/Db/View.php index b7d6939fa8..df6069ac02 100644 --- a/lib/Db/View.php +++ b/lib/Db/View.php @@ -223,49 +223,14 @@ public function setFilterArray(array $array):void { } /** - * The widgets of a grid view and where they sit. + * The widgets of a grid view and where they sit, as GridWidgetTypes stored them. * - * @return array{widgets: list>, layout: list>} + * @return array{widgets: list>, layout: list>} */ public function getGridArray(): array { $grid = $this->getArray($this->getGrid()); - $widgets = []; - foreach (is_array($grid['widgets'] ?? null) ? $grid['widgets'] : [] as $widget) { - if (is_array($widget)) { - $widgets[] = self::upgradeWidgetShape($widget); - } - } - $layout = []; - foreach (is_array($grid['layout'] ?? null) ? $grid['layout'] : [] as $item) { - if (is_array($item) && is_string($item['widgetId'] ?? null)) { - // a layout item is identified by its widget - $item['id'] = $item['widgetId']; - $layout[] = $item; - } - } - return ['widgets' => $widgets, 'layout' => $layout]; - } - - /** - * Grids saved before widgets had a configuration carried title and showTitle on the - * widget itself; they read as the current shape until saved again. - * - * @param array $widget - * @return array - */ - private static function upgradeWidgetShape(array $widget): array { - $configuration = is_array($widget['configuration'] ?? null) ? $widget['configuration'] : []; - foreach (['title', 'showTitle'] as $legacy) { - if (array_key_exists($legacy, $widget)) { - if (!array_key_exists($legacy, $configuration)) { - $configuration[$legacy] = $widget[$legacy]; - } - unset($widget[$legacy]); - } - } - $widget['configuration'] = $configuration; - $widget['content'] = is_array($widget['content'] ?? null) ? $widget['content'] : []; - return $widget; + $list = static fn (mixed $items): array => array_values(array_filter(is_array($items) ? $items : [], 'is_array')); + return ['widgets' => $list($grid['widgets'] ?? null), 'layout' => $list($grid['layout'] ?? null)]; } public function setGridArray(array $array): void { diff --git a/lib/ResponseDefinitions.php b/lib/ResponseDefinitions.php index 60b0d32899..e0855cc161 100644 --- a/lib/ResponseDefinitions.php +++ b/lib/ResponseDefinitions.php @@ -13,11 +13,13 @@ namespace OCA\Tables; /** + * @psalm-type TablesGridWidgetValue = string|bool|int|array{type: 'table'|'view', id: int}|null + * * @psalm-type TablesGridWidget = array{ * id: string, * type: string, - * configuration: array, - * content: array, + * configuration: array, + * content: array, * } * * @psalm-type TablesGridLayoutItem = array{ diff --git a/lib/Service/GridWidgetTypes.php b/lib/Service/GridWidgetTypes.php index d3501b86d6..021af4d613 100644 --- a/lib/Service/GridWidgetTypes.php +++ b/lib/Service/GridWidgetTypes.php @@ -9,8 +9,12 @@ namespace OCA\Tables\Service; +use OCA\Tables\Db\TableMapper; +use OCA\Tables\Db\View; use OCA\Tables\Db\ViewMapper; use OCA\Tables\Errors\BadRequestError; +use OCA\Tables\ResponseDefinitions; +use OCP\DB\Exception; use OCP\IL10N; /** @@ -18,10 +22,11 @@ * and one for its content; the frontend renders its forms from them and stored grids * are validated against them. * - * @psalm-type GridWidgetProperty = array{type: string, title: string, default: mixed, options?: list, required?: bool} - * @psalm-type GridWidgetType = array{type: string, title: string, defaultWidth: int, defaultHeight: int, configuration: array, properties: array} - * @psalm-type GridWidget = array{id: string, type: string, configuration: array, content: array} - * @psalm-type GridLayoutItem = array{id: string, widgetId: string, gridX: int, gridY: int, gridWidth: int, gridHeight: int} + * @psalm-import-type TablesGridWidgetValue from ResponseDefinitions + * @psalm-import-type TablesGridWidget from ResponseDefinitions + * @psalm-import-type TablesGridLayoutItem from ResponseDefinitions + * @psalm-type TablesGridWidgetProperty = array{type: string, title: string, default: TablesGridWidgetValue, options?: list, required?: bool} + * @psalm-type TablesGridWidgetType = array{type: string, title: string, defaultWidth: int, defaultHeight: int, configuration: array, properties: array} */ class GridWidgetTypes { public const TYPE_HEADER = 'header'; @@ -31,16 +36,19 @@ class GridWidgetTypes { public const PROPERTY_TYPES = ['string', 'text', 'color', 'boolean', 'integer', 'enum', 'target']; private const HEX_COLOR = '/^#[0-9a-fA-F]{3,6}$/'; - private const LAYOUT_POSITIONS = ['gridX', 'gridY', 'gridWidth', 'gridHeight']; + private const LAYOUT_POSITIONS = ['gridX', 'gridY']; + private const LAYOUT_SIZES = ['gridWidth', 'gridHeight']; public function __construct( private readonly IL10N $l10n, private readonly ViewMapper $viewMapper, + private readonly TableMapper $tableMapper, + private readonly PermissionsService $permissionsService, ) { } /** - * @return array + * @return array */ public function all(): array { return [ @@ -86,7 +94,7 @@ public function all(): array { } /** - * @return array + * @return array */ private function titleConfiguration(bool $showTitle): array { return [ @@ -96,7 +104,7 @@ private function titleConfiguration(bool $showTitle): array { } /** - * @return GridWidgetType + * @return TablesGridWidgetType * @throws BadRequestError */ public function get(string $type): array { @@ -108,12 +116,14 @@ public function get(string $type): array { } /** - * Validates a grid and returns it with every widget reduced to what its schemas allow. + * Validates a grid for the user who stores it and returns it with every widget reduced + * to what its schemas allow. * - * @return array{widgets: list, layout: list} + * @return array{widgets: list, layout: list} * @throws BadRequestError + * @throws Exception */ - public function sanitizeGrid(mixed $grid): array { + public function sanitizeGrid(mixed $grid, string $userId): array { if (!is_array($grid)) { throw new BadRequestError('The grid must be an object.'); } @@ -134,22 +144,25 @@ public function sanitizeGrid(mixed $grid): array { } $widgets[$sanitized['id']] = $sanitized; } - $this->assertTargetsAreNotGridViews($widgets); + $this->assertTargetsAreReadable($widgets, $userId); $layout = []; foreach ($grid['layout'] ?? [] as $item) { - $layout[] = $this->sanitizeLayoutItem($item, $widgets); + $sanitized = $this->sanitizeLayoutItem($item, $widgets); + if (isset($layout[$sanitized['widgetId']])) { + throw new BadRequestError('The layout places widget ' . $sanitized['widgetId'] . ' twice.'); + } + $layout[$sanitized['widgetId']] = $sanitized; } - return ['widgets' => array_values($widgets), 'layout' => $layout]; + return ['widgets' => array_values($widgets), 'layout' => array_values($layout)]; } /** - * Returns a widget in the stored shape. A widget saved before the configuration existed - * carried title and showTitle on itself; they move into the configuration. + * Returns a widget in the stored shape. * * @param array $widget - * @return GridWidget + * @return TablesGridWidget * @throws BadRequestError */ public function sanitizeWidget(array $widget): array { @@ -160,19 +173,11 @@ public function sanitizeWidget(array $widget): array { if (!is_array($configuration)) { throw new BadRequestError('The configuration of widget ' . $widget['id'] . ' must be an object.'); } - foreach (['title', 'showTitle'] as $legacy) { - if (array_key_exists($legacy, $widget) && !array_key_exists($legacy, $configuration)) { - $configuration[$legacy] = $widget[$legacy]; - } - } $content = $widget['content'] ?? []; if (!is_array($content)) { throw new BadRequestError('The content of widget ' . $widget['id'] . ' must be an object.'); } $type = $this->get($widget['type']); - if ($widget['type'] === self::TYPE_DATA && !isset($content['target']) && isset($content['targetType'], $content['targetId'])) { - $content['target'] = ['type' => $content['targetType'], 'id' => (int)$content['targetId']]; - } return [ 'id' => $widget['id'], 'type' => $widget['type'], @@ -182,8 +187,8 @@ public function sanitizeWidget(array $widget): array { } /** - * @param array $widgets - * @return GridLayoutItem + * @param array $widgets + * @return TablesGridLayoutItem * @throws BadRequestError */ private function sanitizeLayoutItem(mixed $item, array $widgets): array { @@ -198,6 +203,11 @@ private function sanitizeLayoutItem(mixed $item, array $widgets): array { throw new BadRequestError('The layout position "' . $position . '" must be a non-negative integer.'); } } + foreach (self::LAYOUT_SIZES as $size) { + if (!is_int($item[$size] ?? null) || $item[$size] < 1) { + throw new BadRequestError('The layout size "' . $size . '" must be a positive integer.'); + } + } return [ 'id' => $item['widgetId'], 'widgetId' => $item['widgetId'], @@ -209,25 +219,34 @@ private function sanitizeLayoutItem(mixed $item, array $widgets): array { } /** - * A data widget shows rows, so it cannot point at another grid view. + * A data widget shows rows, so its table or view must exist, hold rows and be readable for + * the user. Every unusable target gets the same error, so it reveals nothing about others' views. * - * @param array $widgets + * @param array $widgets * @throws BadRequestError + * @throws Exception */ - private function assertTargetsAreNotGridViews(array $widgets): void { - $viewIds = []; + private function assertTargetsAreReadable(array $widgets, string $userId): void { + $targetIds = ['table' => [], 'view' => []]; foreach ($widgets as $widget) { $target = $widget['content']['target'] ?? null; - if (is_array($target) && $target['type'] === 'view') { - $viewIds[] = $target['id']; + if (is_array($target)) { + $targetIds[$target['type']][] = $target['id']; } } - if ($viewIds === []) { - return; - } - foreach ($this->viewMapper->findMany($viewIds) as $view) { - if ($view->getTableId() === null) { - throw new BadRequestError('A table or view widget cannot show the view ' . $view->getId() . ', which has no table.'); + $elements = [ + 'table' => $targetIds['table'] === [] ? [] : $this->tableMapper->findMany($targetIds['table']), + 'view' => $targetIds['view'] === [] ? [] : $this->viewMapper->findMany($targetIds['view']), + ]; + foreach ($targetIds as $type => $ids) { + foreach ($ids as $id) { + $element = $elements[$type][$id] ?? null; + $isUsable = $element !== null + && !($element instanceof View && $element->isStandalone()) + && $this->permissionsService->canReadRowsByElement($element, $type, $userId); + if (!$isUsable) { + throw new BadRequestError('A table or view widget can only show a table or view whose rows you can read.'); + } } } } @@ -235,9 +254,9 @@ private function assertTargetsAreNotGridViews(array $widgets): void { /** * Every property checked against its schema, defaults filled in and unknown properties dropped. * - * @param array $schema + * @param array $schema * @param array $values - * @return array + * @return array * @throws BadRequestError */ private function sanitizeProperties(string $type, array $schema, array $values): array { @@ -250,10 +269,11 @@ private function sanitizeProperties(string $type, array $schema, array $values): } /** - * @param GridWidgetProperty $property + * @param TablesGridWidgetProperty $property + * @return TablesGridWidgetValue * @throws BadRequestError */ - private function sanitizeProperty(string $label, array $property, mixed $value): mixed { + private function sanitizeProperty(string $label, array $property, mixed $value): string|bool|int|array|null { $isRequired = $property['required'] ?? false; return match ($property['type']) { 'string', 'text' => $this->sanitizeText($label, $value, $isRequired), @@ -294,18 +314,18 @@ private function sanitizeColor(string $label, mixed $value): string { } /** - * @param list $allowed + * @param list $allowed * @throws BadRequestError */ - private function sanitizeEnum(string $label, mixed $value, array $allowed): mixed { - if (!in_array($value, $allowed, true)) { + private function sanitizeEnum(string $label, mixed $value, array $allowed): string { + if (!is_string($value) || !in_array($value, $allowed, true)) { throw new BadRequestError('Widget property ' . $label . ' must be one of: ' . implode(', ', $allowed) . '.'); } return $value; } /** - * @return array{type: string, id: int}|null + * @return array{type: 'table'|'view', id: int}|null * @throws BadRequestError */ private function sanitizeTarget(string $label, mixed $value, bool $isRequired): ?array { @@ -315,9 +335,11 @@ private function sanitizeTarget(string $label, mixed $value, bool $isRequired): } return null; } - if (!is_array($value) || !in_array($value['type'] ?? null, ['table', 'view'], true) || !is_int($value['id'] ?? null) || $value['id'] <= 0) { + $type = is_array($value) ? ($value['type'] ?? null) : null; + $id = is_array($value) ? ($value['id'] ?? null) : null; + if (($type !== 'table' && $type !== 'view') || !is_int($id) || $id <= 0) { throw new BadRequestError('Widget property ' . $label . ' must name a table or view by id.'); } - return ['type' => $value['type'], 'id' => $value['id']]; + return ['type' => $type, 'id' => $id]; } } diff --git a/lib/Service/ViewService.php b/lib/Service/ViewService.php index 33a21f9aab..ff12cfca46 100644 --- a/lib/Service/ViewService.php +++ b/lib/Service/ViewService.php @@ -12,6 +12,7 @@ use DateTime; use Exception; use InvalidArgumentException; +use JsonException; use JsonSerializable; use OCA\Tables\Activity\ActivityManager; use OCA\Tables\Activity\ChangeSet; @@ -303,11 +304,19 @@ public function update(int $id, ViewUpdateInput $data, ?string $userId = null, b } if ($parameter === ViewUpdatableParameters::GRID) { - $value = $this->gridWidgetTypes->sanitizeGrid($value); + // the type is applied before the grid, so a request can turn a view into a grid view and fill it + if ($view->getTypeOrDefault() !== View::TYPE_GRID) { + throw new BadRequestError('Only a grid view holds a grid.'); + } + $value = $this->gridWidgetTypes->sanitizeGrid($value, $userId); } if ($value instanceof JsonSerializable || is_array($value)) { - $insertableValue = json_encode($value); + try { + $insertableValue = json_encode($value, JSON_THROW_ON_ERROR); + } catch (JsonException $e) { + throw new BadRequestError('The view property "' . $parameter->value . '" cannot be stored: ' . $e->getMessage()); + } } $setterMethod = 'set' . ucfirst((string)$parameter->value); @@ -732,8 +741,9 @@ public function importView(?int $tableId, array $view, string $userId): View { $type = $view['type'] ?? View::TYPE_TABLE; $this->assertTypeValid($type); $item->setType($type); - if (isset($view['grid'])) { - $item->setGridArray($this->gridWidgetTypes->sanitizeGrid($view['grid'])); + // every exported view carries a grid, so only a grid view takes it over + if ($type === View::TYPE_GRID && isset($view['grid'])) { + $item->setGridArray($this->gridWidgetTypes->sanitizeGrid($view['grid'], $userId)); } try { $importedView = $this->mapper->insert($item); diff --git a/openapi.json b/openapi.json index 7ab01eead9..ce9f2a4c29 100644 --- a/openapi.json +++ b/openapi.json @@ -383,17 +383,52 @@ "configuration": { "type": "object", "additionalProperties": { - "type": "object" + "$ref": "#/components/schemas/GridWidgetValue" } }, "content": { "type": "object", "additionalProperties": { - "type": "object" + "$ref": "#/components/schemas/GridWidgetValue" } } } }, + "GridWidgetValue": { + "nullable": true, + "oneOf": [ + { + "type": "string" + }, + { + "type": "boolean" + }, + { + "type": "integer", + "format": "int64" + }, + { + "type": "object", + "required": [ + "type", + "id" + ], + "properties": { + "type": { + "type": "string", + "enum": [ + "table", + "view" + ] + }, + "id": { + "type": "integer", + "format": "int64" + } + } + } + ] + }, "ImportState": { "type": "object", "required": [ diff --git a/src/types/openapi/openapi.ts b/src/types/openapi/openapi.ts index 7e2ea13246..99e1857b05 100644 --- a/src/types/openapi/openapi.ts +++ b/src/types/openapi/openapi.ts @@ -1178,12 +1178,18 @@ export type components = { readonly id: string; readonly type: string; readonly configuration: { - readonly [key: string]: Record; + readonly [key: string]: components["schemas"]["GridWidgetValue"]; }; readonly content: { - readonly [key: string]: Record; + readonly [key: string]: components["schemas"]["GridWidgetValue"]; }; }; + readonly GridWidgetValue: (string | boolean | number | { + /** @enum {string} */ + readonly type: "table" | "view"; + /** Format: int64 */ + readonly id: number; + }) | null; readonly ImportState: { /** Format: int64 */ readonly found_columns_count: number; diff --git a/tests/unit/Db/ViewTest.php b/tests/unit/Db/ViewTest.php index b22c1fd380..4bb5e8c5db 100644 --- a/tests/unit/Db/ViewTest.php +++ b/tests/unit/Db/ViewTest.php @@ -25,7 +25,7 @@ public function testGridArrayDropsUnknownKeysAndNonLists(): void { $view = new View(); $view->setGrid(json_encode(['widgets' => 'oops', 'layout' => [['widgetId' => 'w-1']], 'extra' => true])); - $this->assertSame(['widgets' => [], 'layout' => [['widgetId' => 'w-1', 'id' => 'w-1']]], $view->getGridArray()); + $this->assertSame(['widgets' => [], 'layout' => [['widgetId' => 'w-1']]], $view->getGridArray()); } public function testGridArrayRoundTrips(): void { @@ -39,20 +39,6 @@ public function testGridArrayRoundTrips(): void { $this->assertSame($grid, $view->getGridArray()); } - public function testGridArrayIdentifiesLayoutItemsSavedWithANumberByTheirWidget(): void { - $view = new View(); - $view->setGridArray(['widgets' => [], 'layout' => [['id' => 1, 'widgetId' => 'w-1', 'gridX' => 0], ['gridX' => 0]]]); - - $this->assertSame([['id' => 'w-1', 'widgetId' => 'w-1', 'gridX' => 0]], $view->getGridArray()['layout']); - } - - public function testGridArrayUpgradesWidgetsSavedBeforeTheConfiguration(): void { - $view = new View(); - $view->setGrid(json_encode(['widgets' => [['id' => 'w-1', 'type' => 'text', 'title' => 'How', 'showTitle' => true, 'content' => ['text' => 'x']]], 'layout' => []])); - - $this->assertSame([['id' => 'w-1', 'type' => 'text', 'content' => ['text' => 'x'], 'configuration' => ['title' => 'How', 'showTitle' => true]]], $view->getGridArray()['widgets']); - } - public function testTypeFallsBackToTable(): void { $view = new View(); $this->assertSame(View::TYPE_TABLE, $view->getTypeOrDefault()); diff --git a/tests/unit/Service/GridWidgetTypesTest.php b/tests/unit/Service/GridWidgetTypesTest.php index 8ef241c871..e3f3a7e46f 100644 --- a/tests/unit/Service/GridWidgetTypesTest.php +++ b/tests/unit/Service/GridWidgetTypesTest.php @@ -9,16 +9,21 @@ namespace OCA\Tables\Tests\Unit\Service; +use OCA\Tables\Db\Table; +use OCA\Tables\Db\TableMapper; use OCA\Tables\Db\View; use OCA\Tables\Db\ViewMapper; use OCA\Tables\Errors\BadRequestError; use OCA\Tables\Service\GridWidgetTypes; +use OCA\Tables\Service\PermissionsService; use OCP\IL10N; use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; class GridWidgetTypesTest extends TestCase { private ViewMapper&MockObject $viewMapper; + private TableMapper&MockObject $tableMapper; + private PermissionsService&MockObject $permissionsService; private GridWidgetTypes $types; protected function setUp(): void { @@ -26,7 +31,9 @@ protected function setUp(): void { $l10n = $this->createMock(IL10N::class); $l10n->method('t')->willReturnCallback(static fn (string $text): string => $text); $this->viewMapper = $this->createMock(ViewMapper::class); - $this->types = new GridWidgetTypes($l10n, $this->viewMapper); + $this->tableMapper = $this->createMock(TableMapper::class); + $this->permissionsService = $this->createMock(PermissionsService::class); + $this->types = new GridWidgetTypes($l10n, $this->viewMapper, $this->tableMapper, $this->permissionsService); } public function testEveryTypeDescribesItselfAndItsProperties(): void { @@ -48,7 +55,7 @@ public function testEveryTypeDescribesItselfAndItsProperties(): void { public function testTitlesAreTranslated(): void { $l10n = $this->createMock(IL10N::class); $l10n->method('t')->willReturnCallback(static fn (string $text): string => 'nl:' . $text); - $types = (new GridWidgetTypes($l10n, $this->viewMapper))->all(); + $types = (new GridWidgetTypes($l10n, $this->viewMapper, $this->tableMapper, $this->permissionsService))->all(); $this->assertSame('nl:Header', $types['header']['title']); $this->assertSame('nl:Heading', $types['header']['properties']['title']['title']); @@ -68,13 +75,6 @@ public function testSanitizeWidgetSplitsConfigurationFromContent(): void { $this->assertSame(['id' => 'w-1', 'type' => 'text', 'configuration' => ['title' => 'How', 'showTitle' => true], 'content' => ['text' => 'it works']], $widget); } - public function testSanitizeWidgetUpgradesTitleAndShowTitleFromTheOldShape(): void { - $widget = $this->types->sanitizeWidget(['id' => 'w-1', 'type' => 'header', 'title' => 'Old', 'showTitle' => true, 'content' => ['title' => 'Hi']]); - - $this->assertSame(['title' => 'Old', 'showTitle' => true], $widget['configuration']); - $this->assertSame(['id', 'type', 'configuration', 'content'], array_keys($widget)); - } - public function testSanitizeWidgetFillsTheConfigurationDefaultsOfTheType(): void { $this->assertFalse($this->types->sanitizeWidget(['id' => 'w-1', 'type' => 'header', 'content' => ['title' => 'Hi']])['configuration']['showTitle']); $this->assertTrue($this->types->sanitizeWidget(['id' => 'w-2', 'type' => 'text'])['configuration']['showTitle']); @@ -127,12 +127,6 @@ public function testSanitizeKeepsAValidTarget(): void { $this->assertSame(['target' => ['type' => 'view', 'id' => 6]], $widget['content']); } - public function testSanitizeUpgradesTheOldDataShape(): void { - $widget = $this->types->sanitizeWidget(['id' => 'w-1', 'type' => 'data', 'content' => ['targetType' => 'table', 'targetId' => '4']]); - - $this->assertSame(['target' => ['type' => 'table', 'id' => 4]], $widget['content']); - } - public function testSanitizeRejectsATargetWithoutId(): void { $this->expectException(BadRequestError::class); $this->expectExceptionMessage('name a table or view by id'); @@ -155,7 +149,7 @@ public function testSanitizeGridIdentifiesLayoutItemsByTheirWidget(): void { $grid = $this->types->sanitizeGrid([ 'widgets' => [['id' => 'w-1', 'type' => 'text']], 'layout' => [['id' => 7, 'widgetId' => 'w-1', 'gridX' => 0, 'gridY' => 0, 'gridWidth' => 6, 'gridHeight' => 2, 'stray' => true]], - ]); + ], 'alice'); $this->assertSame([['id' => 'w-1', 'widgetId' => 'w-1', 'gridX' => 0, 'gridY' => 0, 'gridWidth' => 6, 'gridHeight' => 2]], $grid['layout']); } @@ -166,13 +160,13 @@ public function testSanitizeGridRejectsALayoutItemWithoutItsWidget(): void { $this->types->sanitizeGrid([ 'widgets' => [], 'layout' => [['widgetId' => 'w-gone', 'gridX' => 0, 'gridY' => 0, 'gridWidth' => 6, 'gridHeight' => 2]], - ]); + ], 'alice'); } public function testSanitizeGridRejectsTwoWidgetsWithTheSameId(): void { $this->expectException(BadRequestError::class); $this->expectExceptionMessage('share the id'); - $this->types->sanitizeGrid(['widgets' => [['id' => 'w-1', 'type' => 'text'], ['id' => 'w-1', 'type' => 'text']]]); + $this->types->sanitizeGrid(['widgets' => [['id' => 'w-1', 'type' => 'text'], ['id' => 'w-1', 'type' => 'text']]], 'alice'); } public function testSanitizeGridRejectsANegativePosition(): void { @@ -181,23 +175,79 @@ public function testSanitizeGridRejectsANegativePosition(): void { $this->types->sanitizeGrid([ 'widgets' => [['id' => 'w-1', 'type' => 'text']], 'layout' => [['widgetId' => 'w-1', 'gridX' => -1, 'gridY' => 0, 'gridWidth' => 6, 'gridHeight' => 2]], - ]); + ], 'alice'); } - public function testSanitizeGridRejectsAWidgetShowingAViewWithoutTable(): void { + public function testSanitizeGridRejectsWidgetsThatAreNotAList(): void { + $this->expectException(BadRequestError::class); + $this->expectExceptionMessage('"widgets" must be a list'); + $this->types->sanitizeGrid(['widgets' => ['a' => ['id' => 'w-1', 'type' => 'text']]], 'alice'); + } + + private function dataWidget(string $type, int $id): array { + return ['id' => 'w-1', 'type' => 'data', 'content' => ['target' => ['type' => $type, 'id' => $id]]]; + } + + public function testSanitizeGridKeepsATargetTheUserCanRead(): void { + $table = new Table(); + $table->setId(4); + $this->tableMapper->method('findMany')->with([4])->willReturn([4 => $table]); + $this->permissionsService->method('canReadRowsByElement')->with($table, 'table', 'alice')->willReturn(true); + + $grid = $this->types->sanitizeGrid(['widgets' => [$this->dataWidget('table', 4)]], 'alice'); + + $this->assertSame(['type' => 'table', 'id' => 4], $grid['widgets'][0]['content']['target']); + } + + public function testSanitizeGridRefusesTargetsTheUserCannotUseWithOneMessage(): void { $gridView = new View(); $gridView->setId(9); - $gridView->setTableId(null); - $this->viewMapper->method('findMany')->with([9])->willReturn([9 => $gridView]); + $readableTable = new Table(); + $readableTable->setId(4); + $this->viewMapper->method('findMany')->willReturn([9 => $gridView]); + $this->tableMapper->method('findMany')->willReturn([4 => $readableTable]); + $this->permissionsService->method('canReadRowsByElement')->willReturnCallback(static fn ($element): bool => $element === $readableTable); + + $cases = [ + 'a view without a table' => $this->dataWidget('view', 9), + 'a table that does not exist' => $this->dataWidget('table', 5), + 'a view that does not exist' => $this->dataWidget('view', 10), + ]; + foreach ($cases as $case => $widget) { + try { + $this->types->sanitizeGrid(['widgets' => [$widget]], 'alice'); + $this->fail($case . ' was accepted'); + } catch (BadRequestError $e) { + $this->assertSame('A table or view widget can only show a table or view whose rows you can read.', $e->getMessage(), $case); + } + } + } + + public function testSanitizeGridRefusesATableTheUserCannotRead(): void { + $table = new Table(); + $table->setId(4); + $this->tableMapper->method('findMany')->willReturn([4 => $table]); + $this->permissionsService->method('canReadRowsByElement')->willReturn(false); $this->expectException(BadRequestError::class); - $this->expectExceptionMessage('which has no table'); - $this->types->sanitizeGrid(['widgets' => [['id' => 'w-1', 'type' => 'data', 'content' => ['target' => ['type' => 'view', 'id' => 9]]]]]); + $this->expectExceptionMessage('whose rows you can read'); + $this->types->sanitizeGrid(['widgets' => [$this->dataWidget('table', 4)]], 'alice'); } - public function testSanitizeGridRejectsWidgetsThatAreNotAList(): void { + public function testSanitizeGridRejectsAWidgetPlacedTwice(): void { + $item = ['widgetId' => 'w-1', 'gridX' => 0, 'gridY' => 0, 'gridWidth' => 6, 'gridHeight' => 2]; + $this->expectException(BadRequestError::class); - $this->expectExceptionMessage('"widgets" must be a list'); - $this->types->sanitizeGrid(['widgets' => ['a' => ['id' => 'w-1', 'type' => 'text']]]); + $this->expectExceptionMessage('places widget w-1 twice'); + $this->types->sanitizeGrid(['widgets' => [['id' => 'w-1', 'type' => 'text']], 'layout' => [$item, $item]], 'alice'); + } + + public function testSanitizeGridRejectsALayoutItemWithoutSize(): void { + $this->expectException(BadRequestError::class); + $this->expectExceptionMessage('"gridWidth" must be a positive integer'); + $this->types->sanitizeGrid([ + 'widgets' => [['id' => 'w-1', 'type' => 'text']], + 'layout' => [['widgetId' => 'w-1', 'gridX' => 0, 'gridY' => 0, 'gridWidth' => 0, 'gridHeight' => 2]], + ], 'alice'); } } diff --git a/tests/unit/Service/ViewServiceTest.php b/tests/unit/Service/ViewServiceTest.php index b95fa18a33..cc18685a9c 100644 --- a/tests/unit/Service/ViewServiceTest.php +++ b/tests/unit/Service/ViewServiceTest.php @@ -11,6 +11,7 @@ use OCA\Tables\Activity\ActivityManager; use OCA\Tables\Db\Table; +use OCA\Tables\Db\TableMapper; use OCA\Tables\Db\View; use OCA\Tables\Db\ViewMapper; use OCA\Tables\Errors\BadRequestError; @@ -61,7 +62,7 @@ protected function setUp(): void { $l10n, $this->createMock(FederationService::class), $this->activityManager, - new GridWidgetTypes($l10n, $this->mapper), + new GridWidgetTypes($l10n, $this->mapper, $this->createMock(TableMapper::class), $this->permissionsService), ); } @@ -101,6 +102,31 @@ public function testUpdateStoresAValidGridAsJson(): void { $this->assertSame(['title' => '', 'showTitle' => false], $stored['widgets'][0]['configuration'], 'the configuration is filled from the type'); } + public function testUpdateRefusesAGridOnATableView(): void { + $this->expectUpdateOf($this->storedView(4)); + + $this->expectException(BadRequestError::class); + $this->expectExceptionMessage('Only a grid view holds a grid'); + $this->service->update(6, ViewUpdateInput::fromInputArray(['grid' => json_encode(['widgets' => [], 'layout' => []])]), 'alice', true); + } + + public function testUpdateAcceptsTheGridTypeAndAGridInOneRequest(): void { + $this->expectUpdateOf($this->storedView(4)); + + $updated = $this->service->update(6, ViewUpdateInput::fromInputArray(['type' => 'grid', 'grid' => json_encode(['widgets' => [], 'layout' => []])]), 'alice', true); + + $this->assertSame(['widgets' => [], 'layout' => []], $updated->getGridArray()); + } + + public function testUpdateRefusesAGridThatCannotBeEncodedInsteadOfWipingIt(): void { + $this->expectUpdateOf($this->storedView(null)); + $grid = ['widgets' => [['id' => 'w-1', 'type' => 'text', 'content' => ['text' => "\xFF"]]], 'layout' => []]; + + $this->expectException(BadRequestError::class); + $this->expectExceptionMessage('"grid" cannot be stored'); + $this->service->update(6, ViewUpdateInput::fromInputArray(['grid' => $grid]), 'alice', true); + } + public function testUpdateWritesEveryParameterToItsOwnColumn(): void { $this->expectUpdateOf($this->storedView(4)); From 2da6fa598cc414017800583bc140a23abbfc8d11 Mon Sep 17 00:00:00 2001 From: Remko Date: Mon, 12 Oct 2026 12:52:18 +0200 Subject: [PATCH 3/5] fix(grid): keep stored widget targets and mark missing ones broken A save now only checks targets that are new or changed against the user who saves. A target that was stored before is kept, so a save no longer depends on the saver's access to widgets they did not touch, and a target that no longer exists is marked broken instead of blocking every save; the mark clears once the widget is saved in a valid state again. Tests now cover the refusal of a view without a table on its own, and the import of a table view whose source carries a grid. Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: Remko --- lib/ResponseDefinitions.php | 1 + lib/Service/GridWidgetTypes.php | 57 ++++++++++++++++------ lib/Service/ViewService.php | 2 +- openapi.json | 3 ++ src/types/openapi/openapi.ts | 1 + tests/unit/Service/GridWidgetTypesTest.php | 48 +++++++++++++++++- tests/unit/Service/ViewServiceTest.php | 16 ++++++ 7 files changed, 112 insertions(+), 16 deletions(-) diff --git a/lib/ResponseDefinitions.php b/lib/ResponseDefinitions.php index e0855cc161..026d9ea730 100644 --- a/lib/ResponseDefinitions.php +++ b/lib/ResponseDefinitions.php @@ -20,6 +20,7 @@ * type: string, * configuration: array, * content: array, + * broken?: bool, * } * * @psalm-type TablesGridLayoutItem = array{ diff --git a/lib/Service/GridWidgetTypes.php b/lib/Service/GridWidgetTypes.php index 021af4d613..36d8333946 100644 --- a/lib/Service/GridWidgetTypes.php +++ b/lib/Service/GridWidgetTypes.php @@ -117,13 +117,14 @@ public function get(string $type): array { /** * Validates a grid for the user who stores it and returns it with every widget reduced - * to what its schemas allow. + * to what its schemas allow. The grid stored so far tells which targets are unchanged. * + * @param array $storedGrid * @return array{widgets: list, layout: list} * @throws BadRequestError * @throws Exception */ - public function sanitizeGrid(mixed $grid, string $userId): array { + public function sanitizeGrid(mixed $grid, string $userId, array $storedGrid = []): array { if (!is_array($grid)) { throw new BadRequestError('The grid must be an object.'); } @@ -144,7 +145,7 @@ public function sanitizeGrid(mixed $grid, string $userId): array { } $widgets[$sanitized['id']] = $sanitized; } - $this->assertTargetsAreReadable($widgets, $userId); + $widgets = $this->checkTargets($widgets, $userId, $this->storedTargets($storedGrid)); $layout = []; foreach ($grid['layout'] ?? [] as $item) { @@ -219,14 +220,18 @@ private function sanitizeLayoutItem(mixed $item, array $widgets): array { } /** - * A data widget shows rows, so its table or view must exist, hold rows and be readable for - * the user. Every unusable target gets the same error, so it reveals nothing about others' views. + * A data widget shows rows, so a new or changed target must exist, hold rows and be readable + * for the user; every unusable one gets the same error, so it reveals nothing about others' + * views. A target that was stored before is kept as it is, so a save does not depend on the + * saver's access to widgets they did not touch, and is marked broken once it no longer exists. * * @param array $widgets + * @param array $storedTargets + * @return array * @throws BadRequestError * @throws Exception */ - private function assertTargetsAreReadable(array $widgets, string $userId): void { + private function checkTargets(array $widgets, string $userId, array $storedTargets): array { $targetIds = ['table' => [], 'view' => []]; foreach ($widgets as $widget) { $target = $widget['content']['target'] ?? null; @@ -238,17 +243,41 @@ private function assertTargetsAreReadable(array $widgets, string $userId): void 'table' => $targetIds['table'] === [] ? [] : $this->tableMapper->findMany($targetIds['table']), 'view' => $targetIds['view'] === [] ? [] : $this->viewMapper->findMany($targetIds['view']), ]; - foreach ($targetIds as $type => $ids) { - foreach ($ids as $id) { - $element = $elements[$type][$id] ?? null; - $isUsable = $element !== null - && !($element instanceof View && $element->isStandalone()) - && $this->permissionsService->canReadRowsByElement($element, $type, $userId); - if (!$isUsable) { - throw new BadRequestError('A table or view widget can only show a table or view whose rows you can read.'); + foreach ($widgets as $widgetId => $widget) { + $target = $widget['content']['target'] ?? null; + if (!is_array($target)) { + continue; + } + $element = $elements[$target['type']][$target['id']] ?? null; + $exists = $element !== null && !($element instanceof View && $element->isStandalone()); + if (($storedTargets[$widgetId] ?? null) === $target) { + // an unchanged target is kept; one that no longer exists is marked until the widget is saved valid again + if (!$exists) { + $widgets[$widgetId]['broken'] = true; } + continue; + } + if (!$exists || !$this->permissionsService->canReadRowsByElement($element, $target['type'], $userId)) { + throw new BadRequestError('A table or view widget can only show a table or view whose rows you can read.'); + } + } + return $widgets; + } + + /** + * The target of every widget in the stored grid, by widget id. + * + * @return array + */ + private function storedTargets(array $storedGrid): array { + $targets = []; + foreach (is_array($storedGrid['widgets'] ?? null) ? $storedGrid['widgets'] : [] as $widget) { + $target = is_array($widget) ? ($widget['content']['target'] ?? null) : null; + if (is_string($widget['id'] ?? null) && is_array($target) && in_array($target['type'] ?? null, ['table', 'view'], true) && is_int($target['id'] ?? null)) { + $targets[$widget['id']] = ['type' => $target['type'], 'id' => $target['id']]; } } + return $targets; } /** diff --git a/lib/Service/ViewService.php b/lib/Service/ViewService.php index ff12cfca46..b5951a5b13 100644 --- a/lib/Service/ViewService.php +++ b/lib/Service/ViewService.php @@ -308,7 +308,7 @@ public function update(int $id, ViewUpdateInput $data, ?string $userId = null, b if ($view->getTypeOrDefault() !== View::TYPE_GRID) { throw new BadRequestError('Only a grid view holds a grid.'); } - $value = $this->gridWidgetTypes->sanitizeGrid($value, $userId); + $value = $this->gridWidgetTypes->sanitizeGrid($value, $userId, $view->getGridArray()); } if ($value instanceof JsonSerializable || is_array($value)) { diff --git a/openapi.json b/openapi.json index ce9f2a4c29..d4e753ee70 100644 --- a/openapi.json +++ b/openapi.json @@ -391,6 +391,9 @@ "additionalProperties": { "$ref": "#/components/schemas/GridWidgetValue" } + }, + "broken": { + "type": "boolean" } } }, diff --git a/src/types/openapi/openapi.ts b/src/types/openapi/openapi.ts index 99e1857b05..590e20a857 100644 --- a/src/types/openapi/openapi.ts +++ b/src/types/openapi/openapi.ts @@ -1183,6 +1183,7 @@ export type components = { readonly content: { readonly [key: string]: components["schemas"]["GridWidgetValue"]; }; + readonly broken?: boolean; }; readonly GridWidgetValue: (string | boolean | number | { /** @enum {string} */ diff --git a/tests/unit/Service/GridWidgetTypesTest.php b/tests/unit/Service/GridWidgetTypesTest.php index e3f3a7e46f..0ef7cc498e 100644 --- a/tests/unit/Service/GridWidgetTypesTest.php +++ b/tests/unit/Service/GridWidgetTypesTest.php @@ -206,7 +206,8 @@ public function testSanitizeGridRefusesTargetsTheUserCannotUseWithOneMessage(): $readableTable->setId(4); $this->viewMapper->method('findMany')->willReturn([9 => $gridView]); $this->tableMapper->method('findMany')->willReturn([4 => $readableTable]); - $this->permissionsService->method('canReadRowsByElement')->willReturnCallback(static fn ($element): bool => $element === $readableTable); + // the owner can read their own grid view, so only the "has a table" check may refuse it + $this->permissionsService->method('canReadRowsByElement')->willReturnCallback(static fn ($element): bool => $element === $readableTable || $element === $gridView); $cases = [ 'a view without a table' => $this->dataWidget('view', 9), @@ -234,6 +235,51 @@ public function testSanitizeGridRefusesATableTheUserCannotRead(): void { $this->types->sanitizeGrid(['widgets' => [$this->dataWidget('table', 4)]], 'alice'); } + public function testSanitizeGridKeepsAnUnchangedTargetTheSaverCannotRead(): void { + $table = new Table(); + $table->setId(4); + $this->tableMapper->method('findMany')->willReturn([4 => $table]); + $this->permissionsService->expects($this->never())->method('canReadRowsByElement'); + $widget = $this->dataWidget('table', 4); + + $grid = $this->types->sanitizeGrid(['widgets' => [$widget]], 'bob', ['widgets' => [$widget]]); + + $this->assertSame(['type' => 'table', 'id' => 4], $grid['widgets'][0]['content']['target']); + $this->assertArrayNotHasKey('broken', $grid['widgets'][0]); + } + + public function testSanitizeGridMarksAnUnchangedTargetThatNoLongerExistsAsBroken(): void { + $this->tableMapper->method('findMany')->willReturn([]); + $widget = $this->dataWidget('table', 4); + + $grid = $this->types->sanitizeGrid(['widgets' => [$widget]], 'alice', ['widgets' => [$widget]]); + + $this->assertTrue($grid['widgets'][0]['broken']); + $this->assertSame(['type' => 'table', 'id' => 4], $grid['widgets'][0]['content']['target'], 'the reference is kept'); + } + + public function testSanitizeGridClearsTheBrokenMarkWhenTheTargetIsValidAgain(): void { + $table = new Table(); + $table->setId(4); + $this->tableMapper->method('findMany')->willReturn([4 => $table]); + $widget = $this->dataWidget('table', 4) + ['broken' => true]; + + $grid = $this->types->sanitizeGrid(['widgets' => [$widget]], 'alice', ['widgets' => [$widget]]); + + $this->assertArrayNotHasKey('broken', $grid['widgets'][0]); + } + + public function testSanitizeGridChecksAChangedTargetAgainstTheSaver(): void { + $table = new Table(); + $table->setId(5); + $this->tableMapper->method('findMany')->willReturn([5 => $table]); + $this->permissionsService->method('canReadRowsByElement')->willReturn(false); + + $this->expectException(BadRequestError::class); + $this->expectExceptionMessage('whose rows you can read'); + $this->types->sanitizeGrid(['widgets' => [$this->dataWidget('table', 5)]], 'bob', ['widgets' => [$this->dataWidget('table', 4)]]); + } + public function testSanitizeGridRejectsAWidgetPlacedTwice(): void { $item = ['widgetId' => 'w-1', 'gridX' => 0, 'gridY' => 0, 'gridWidth' => 6, 'gridHeight' => 2]; diff --git a/tests/unit/Service/ViewServiceTest.php b/tests/unit/Service/ViewServiceTest.php index cc18685a9c..ac0d1462de 100644 --- a/tests/unit/Service/ViewServiceTest.php +++ b/tests/unit/Service/ViewServiceTest.php @@ -235,6 +235,22 @@ public function testUpdateRefusesATechnicalNameOnAViewWithoutTable(): void { $this->service->update(6, ViewUpdateInput::fromInputArray(['technicalName' => 'home']), 'alice', true); } + public function testImportOfATableViewDoesNotTakeOverAGrid(): void { + $this->mapper->method('insert')->willReturnCallback(static function (View $view): View { + $view->setId(9); + return $view; + }); + $this->mapper->method('update')->willReturnArgument(0); + $grid = ['widgets' => [['id' => 'w-1', 'type' => 'text', 'content' => ['text' => 'x']]], 'layout' => []]; + + $imported = $this->service->importView(4, [ + 'title' => 'Open', 'emoji' => null, 'description' => '', 'type' => View::TYPE_TABLE, 'grid' => $grid, + 'createdAt' => '2026-10-12 10:00:00', 'lastEditAt' => '2026-10-12 10:00:00', 'columnSettings' => [], 'sort' => [], 'filter' => [], + ], 'alice'); + + $this->assertNull($imported->getGrid()); + } + public function testImportWithoutTableIgnoresTheSourceTechnicalName(): void { $this->mapper->method('insert')->willReturnCallback(static function (View $view): View { $view->setId(9); From a04ff722e3a63f50b359a9f75e348ba4ea16b042 Mon Sep 17 00:00:00 2001 From: Remko Date: Mon, 12 Oct 2026 13:15:24 +0200 Subject: [PATCH 4/5] test(grid): cover stored targets through update and fold the rejection cases A test now saves a grid through ViewService::update() with a stored target the saver cannot read, so dropping the stored grid from that call fails it. The input-rejection cases of GridWidgetTypesTest run as one data-provider test, each with its own name and expected message. Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: Remko --- tests/unit/Service/GridWidgetTypesTest.php | 122 +++++---------------- tests/unit/Service/ViewServiceTest.php | 20 +++- 2 files changed, 46 insertions(+), 96 deletions(-) diff --git a/tests/unit/Service/GridWidgetTypesTest.php b/tests/unit/Service/GridWidgetTypesTest.php index 0ef7cc498e..a587423786 100644 --- a/tests/unit/Service/GridWidgetTypesTest.php +++ b/tests/unit/Service/GridWidgetTypesTest.php @@ -80,71 +80,18 @@ public function testSanitizeWidgetFillsTheConfigurationDefaultsOfTheType(): void $this->assertTrue($this->types->sanitizeWidget(['id' => 'w-2', 'type' => 'text'])['configuration']['showTitle']); } - public function testSanitizeWidgetRejectsAWidgetWithoutIdOrType(): void { - $this->expectException(BadRequestError::class); - $this->types->sanitizeWidget(['type' => 'text']); - } - - public function testUnknownTypeIsRejected(): void { - $this->expectException(BadRequestError::class); - $this->expectExceptionMessage('Unknown widget type'); - $this->types->get('kanban'); - } - public function testSanitizeFillsDefaultsAndDropsUnknownProperties(): void { $widget = $this->types->sanitizeWidget(['id' => 'w-1', 'type' => 'header', 'content' => ['title' => 'Hi', 'stray' => true]]); $this->assertSame(['title' => 'Hi', 'subtitle' => '', 'textAlign' => 'left', 'backgroundColor' => '', 'textColor' => ''], $widget['content']); } - public function testSanitizeRejectsAMissingRequiredString(): void { - $this->expectException(BadRequestError::class); - $this->expectExceptionMessage('header.title is required'); - $this->types->sanitizeWidget(['id' => 'w-1', 'type' => 'header', 'content' => ['title' => ' ']]); - } - - public function testSanitizeRejectsANonHexColor(): void { - $this->expectException(BadRequestError::class); - $this->expectExceptionMessage('must be a hex color'); - $this->types->sanitizeWidget(['id' => 'w-1', 'type' => 'header', 'content' => ['title' => 'Hi', 'backgroundColor' => 'red']]); - } - - public function testSanitizeRejectsAnUnknownEnumValue(): void { - $this->expectException(BadRequestError::class); - $this->expectExceptionMessage('textAlign must be one of'); - $this->types->sanitizeWidget(['id' => 'w-1', 'type' => 'header', 'content' => ['title' => 'Hi', 'textAlign' => 'justify']]); - } - - public function testSanitizeRejectsANonBooleanInsteadOfCastingIt(): void { - $this->expectException(BadRequestError::class); - $this->expectExceptionMessage('showTitle must be true or false'); - $this->types->sanitizeWidget(['id' => 'w-1', 'type' => 'text', 'configuration' => ['showTitle' => 1]]); - } - public function testSanitizeKeepsAValidTarget(): void { $widget = $this->types->sanitizeWidget(['id' => 'w-1', 'type' => 'data', 'content' => ['target' => ['type' => 'view', 'id' => 6, 'extra' => 1]]]); $this->assertSame(['target' => ['type' => 'view', 'id' => 6]], $widget['content']); } - public function testSanitizeRejectsATargetWithoutId(): void { - $this->expectException(BadRequestError::class); - $this->expectExceptionMessage('name a table or view by id'); - $this->types->sanitizeWidget(['id' => 'w-1', 'type' => 'data', 'content' => ['target' => ['type' => 'table']]]); - } - - public function testSanitizeRejectsAMissingRequiredTarget(): void { - $this->expectException(BadRequestError::class); - $this->expectExceptionMessage('data.target is required'); - $this->types->sanitizeWidget(['id' => 'w-1', 'type' => 'data']); - } - - public function testSanitizeRejectsANonStringText(): void { - $this->expectException(BadRequestError::class); - $this->expectExceptionMessage('text.text must be a string'); - $this->types->sanitizeWidget(['id' => 'w-1', 'type' => 'text', 'content' => ['text' => ['nested']]]); - } - public function testSanitizeGridIdentifiesLayoutItemsByTheirWidget(): void { $grid = $this->types->sanitizeGrid([ 'widgets' => [['id' => 'w-1', 'type' => 'text']], @@ -154,34 +101,36 @@ public function testSanitizeGridIdentifiesLayoutItemsByTheirWidget(): void { $this->assertSame([['id' => 'w-1', 'widgetId' => 'w-1', 'gridX' => 0, 'gridY' => 0, 'gridWidth' => 6, 'gridHeight' => 2]], $grid['layout']); } - public function testSanitizeGridRejectsALayoutItemWithoutItsWidget(): void { - $this->expectException(BadRequestError::class); - $this->expectExceptionMessage('which the grid does not have'); - $this->types->sanitizeGrid([ - 'widgets' => [], - 'layout' => [['widgetId' => 'w-gone', 'gridX' => 0, 'gridY' => 0, 'gridWidth' => 6, 'gridHeight' => 2]], - ], 'alice'); - } - - public function testSanitizeGridRejectsTwoWidgetsWithTheSameId(): void { - $this->expectException(BadRequestError::class); - $this->expectExceptionMessage('share the id'); - $this->types->sanitizeGrid(['widgets' => [['id' => 'w-1', 'type' => 'text'], ['id' => 'w-1', 'type' => 'text']]], 'alice'); - } - - public function testSanitizeGridRejectsANegativePosition(): void { - $this->expectException(BadRequestError::class); - $this->expectExceptionMessage('gridX'); - $this->types->sanitizeGrid([ - 'widgets' => [['id' => 'w-1', 'type' => 'text']], - 'layout' => [['widgetId' => 'w-1', 'gridX' => -1, 'gridY' => 0, 'gridWidth' => 6, 'gridHeight' => 2]], - ], 'alice'); + public static function invalidGrids(): array { + $text = ['id' => 'w-1', 'type' => 'text']; + $place = static fn (array $item): array => $item + ['widgetId' => 'w-1', 'gridX' => 0, 'gridY' => 0, 'gridWidth' => 6, 'gridHeight' => 2]; + $widget = static fn (array $widget): array => ['widgets' => [$widget + ['id' => 'w-1']]]; + return [ + 'widget without id' => [['widgets' => [['type' => 'text']]], 'string id and type'], + 'unknown widget type' => [$widget(['type' => 'kanban']), 'Unknown widget type'], + 'missing required string' => [$widget(['type' => 'header', 'content' => ['title' => ' ']]), 'header.title is required'], + 'non-hex color' => [$widget(['type' => 'header', 'content' => ['title' => 'Hi', 'backgroundColor' => 'red']]), 'must be a hex color'], + 'unknown enum value' => [$widget(['type' => 'header', 'content' => ['title' => 'Hi', 'textAlign' => 'justify']]), 'textAlign must be one of'], + 'non-boolean is not cast' => [$widget(['type' => 'text', 'configuration' => ['showTitle' => 1]]), 'showTitle must be true or false'], + 'target without id' => [$widget(['type' => 'data', 'content' => ['target' => ['type' => 'table']]]), 'name a table or view by id'], + 'missing required target' => [$widget(['type' => 'data']), 'data.target is required'], + 'non-string text' => [$widget(['type' => 'text', 'content' => ['text' => ['nested']]]), 'text.text must be a string'], + 'widgets not a list' => [['widgets' => ['a' => $text]], '"widgets" must be a list'], + 'two widgets with one id' => [['widgets' => [$text, $text]], 'share the id'], + 'layout item without its widget' => [['widgets' => [], 'layout' => [$place(['widgetId' => 'w-gone'])]], 'which the grid does not have'], + 'negative position' => [['widgets' => [$text], 'layout' => [$place(['gridX' => -1])]], 'gridX'], + 'widget placed twice' => [['widgets' => [$text], 'layout' => [$place([]), $place([])]], 'places widget w-1 twice'], + 'layout item without size' => [['widgets' => [$text], 'layout' => [$place(['gridWidth' => 0])]], '"gridWidth" must be a positive integer'], + ]; } - public function testSanitizeGridRejectsWidgetsThatAreNotAList(): void { + /** + * @dataProvider invalidGrids + */ + public function testSanitizeGridRejectsInvalidInput(array $grid, string $message): void { $this->expectException(BadRequestError::class); - $this->expectExceptionMessage('"widgets" must be a list'); - $this->types->sanitizeGrid(['widgets' => ['a' => ['id' => 'w-1', 'type' => 'text']]], 'alice'); + $this->expectExceptionMessage($message); + $this->types->sanitizeGrid($grid, 'alice'); } private function dataWidget(string $type, int $id): array { @@ -279,21 +228,4 @@ public function testSanitizeGridChecksAChangedTargetAgainstTheSaver(): void { $this->expectExceptionMessage('whose rows you can read'); $this->types->sanitizeGrid(['widgets' => [$this->dataWidget('table', 5)]], 'bob', ['widgets' => [$this->dataWidget('table', 4)]]); } - - public function testSanitizeGridRejectsAWidgetPlacedTwice(): void { - $item = ['widgetId' => 'w-1', 'gridX' => 0, 'gridY' => 0, 'gridWidth' => 6, 'gridHeight' => 2]; - - $this->expectException(BadRequestError::class); - $this->expectExceptionMessage('places widget w-1 twice'); - $this->types->sanitizeGrid(['widgets' => [['id' => 'w-1', 'type' => 'text']], 'layout' => [$item, $item]], 'alice'); - } - - public function testSanitizeGridRejectsALayoutItemWithoutSize(): void { - $this->expectException(BadRequestError::class); - $this->expectExceptionMessage('"gridWidth" must be a positive integer'); - $this->types->sanitizeGrid([ - 'widgets' => [['id' => 'w-1', 'type' => 'text']], - 'layout' => [['widgetId' => 'w-1', 'gridX' => 0, 'gridY' => 0, 'gridWidth' => 0, 'gridHeight' => 2]], - ], 'alice'); - } } diff --git a/tests/unit/Service/ViewServiceTest.php b/tests/unit/Service/ViewServiceTest.php index ac0d1462de..e365aba29c 100644 --- a/tests/unit/Service/ViewServiceTest.php +++ b/tests/unit/Service/ViewServiceTest.php @@ -37,6 +37,7 @@ class ViewServiceTest extends TestCase { private MockObject $permissionsService; private MockObject $mapper; private MockObject $activityManager; + private MockObject $tableMapper; protected function setUp(): void { parent::setUp(); @@ -45,6 +46,7 @@ protected function setUp(): void { $this->permissionsService->method('preCheckUserId')->willReturnCallback(static fn (?string $userId) => $userId ?? 'alice'); $this->mapper = $this->createMock(ViewMapper::class); $this->activityManager = $this->createMock(ActivityManager::class); + $this->tableMapper = $this->createMock(TableMapper::class); $l10n = $this->createMock(IL10N::class); $l10n->method('t')->willReturnCallback(static fn (string $text): string => $text); @@ -62,7 +64,7 @@ protected function setUp(): void { $l10n, $this->createMock(FederationService::class), $this->activityManager, - new GridWidgetTypes($l10n, $this->mapper, $this->createMock(TableMapper::class), $this->permissionsService), + new GridWidgetTypes($l10n, $this->mapper, $this->tableMapper, $this->permissionsService), ); } @@ -83,6 +85,22 @@ private function expectUpdateOf(View $view): void { $this->mapper->method('update')->willReturnArgument(0); } + public function testUpdateKeepsAStoredTargetTheSaverCannotRead(): void { + $grid = ['widgets' => [['id' => 'w-1', 'type' => 'data', 'content' => ['target' => ['type' => 'table', 'id' => 4]]]], 'layout' => []]; + $view = $this->storedView(null); + $view->setGridArray($grid); + $view->resetUpdatedFields(); + $this->expectUpdateOf($view); + $table = new Table(); + $table->setId(4); + $this->tableMapper->method('findMany')->willReturn([4 => $table]); + $this->permissionsService->method('canReadRowsByElement')->willReturn(false); + + $updated = $this->service->update(6, ViewUpdateInput::fromInputArray(['grid' => json_encode($grid)]), 'bob', true); + + $this->assertSame(['type' => 'table', 'id' => 4], $updated->getGridArray()['widgets'][0]['content']['target'], 'the stored grid tells the target is unchanged'); + } + public function testUpdateStoresAValidGridAsJson(): void { $view = $this->storedView(null); $this->expectUpdateOf($view); From dccff5760fd4c30465c9c47edf6a02bea5cb363c Mon Sep 17 00:00:00 2001 From: Remko Date: Mon, 12 Oct 2026 17:13:54 +0200 Subject: [PATCH 5/5] style(views): use a first-class callable in getGridArray Rector's PHP 8.1 set prefers is_array(...) over the string callable. Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: Remko --- lib/Db/View.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/Db/View.php b/lib/Db/View.php index df6069ac02..fb9f2513aa 100644 --- a/lib/Db/View.php +++ b/lib/Db/View.php @@ -229,7 +229,7 @@ public function setFilterArray(array $array):void { */ public function getGridArray(): array { $grid = $this->getArray($this->getGrid()); - $list = static fn (mixed $items): array => array_values(array_filter(is_array($items) ? $items : [], 'is_array')); + $list = static fn (mixed $items): array => array_values(array_filter(is_array($items) ? $items : [], is_array(...))); return ['widgets' => $list($grid['widgets'] ?? null), 'layout' => $list($grid['layout'] ?? null)]; }