Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
195 changes: 195 additions & 0 deletions lib/Controller/ContextPageController.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,195 @@
<?php

declare(strict_types=1);

/**
* SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors
* SPDX-License-Identifier: AGPL-3.0-or-later
*/

namespace OCA\Tables\Controller;

use OCA\Tables\AppInfo\Application;
use OCA\Tables\Errors\BadRequestError;
use OCA\Tables\Errors\NotFoundError;
use OCA\Tables\Errors\PermissionError;
use OCA\Tables\Middleware\Attribute\RequirePermission;
use OCA\Tables\ResponseDefinitions;
use OCA\Tables\Service\GridWidgetTypes;
use OCA\Tables\Service\PageService;
use OCP\AppFramework\Db\DoesNotExistException;
use OCP\AppFramework\Db\MultipleObjectsReturnedException;
use OCP\AppFramework\Http;
use OCP\AppFramework\Http\Attribute\ApiRoute;
use OCP\AppFramework\Http\Attribute\NoAdminRequired;
use OCP\AppFramework\Http\Attribute\UserRateLimit;
use OCP\AppFramework\Http\DataResponse;
use OCP\DB\Exception;
use OCP\IL10N;
use OCP\IRequest;
use Psr\Log\LoggerInterface;

/**
* @psalm-import-type TablesPage from ResponseDefinitions
* @psalm-import-type TablesWidgetType from ResponseDefinitions
*/
class ContextPageController extends AOCSController {
public function __construct(
IRequest $request,
LoggerInterface $logger,
IL10N $n,
string $userId,
private readonly PageService $pageService,
private readonly GridWidgetTypes $gridWidgetTypes,
) {
parent::__construct($request, $logger, $n, $userId);
}

/**
* [api v2] Get a page of an application with its widgets
*
* @param int $contextId ID of the application
* @param int $pageId ID of the page
* @return DataResponse<Http::STATUS_OK, TablesPage, array{}>|DataResponse<Http::STATUS_INTERNAL_SERVER_ERROR|Http::STATUS_NOT_FOUND, array{message: string}, array{}>
*
* 200: Page returned
* 404: Not found, also when the user has no access to the application
*/
#[NoAdminRequired]
#[RequirePermission(permission: Application::PERMISSION_READ, typeParam: 'context', idParam: 'contextId')]
#[ApiRoute(verb: 'GET', url: '/api/2/contexts/{contextId}/pages/{pageId}', requirements: ['contextId' => '\d+', 'pageId' => '\d+'])]
public function show(int $contextId, int $pageId): DataResponse {
try {
return new DataResponse($this->pageService->find($contextId, $pageId, $this->userId));
} catch (NotFoundError $e) {
return $this->handleNotFoundError($e);
} catch (Exception $e) {
return $this->handleError($e);
}
}

/**
* [api v2] Add a grid page to an application
*
* @param int $contextId ID of the application
* @param string $title Title of the page
* @param string|null $technicalName Technical name of the page, unique in the application; `page_<id>` when left out, a form reserved for automatic names
* @param string|null $icon Name of the icon shown with the title
* @param string|null $description Description of the page
* @return DataResponse<Http::STATUS_OK, TablesPage, array{}>|DataResponse<Http::STATUS_BAD_REQUEST|Http::STATUS_FORBIDDEN|Http::STATUS_NOT_FOUND|Http::STATUS_INTERNAL_SERVER_ERROR, array{message: string}, array{}>
*
* 200: Page created
* 400: Invalid title, icon or description, or an invalid or taken technical name
* 403: No permissions
* 404: Not found
*/
#[NoAdminRequired]
#[UserRateLimit(limit: 20, period: 60)]
#[RequirePermission(permission: Application::PERMISSION_MANAGE, typeParam: 'context', idParam: 'contextId')]
#[ApiRoute(verb: 'POST', url: '/api/2/contexts/{contextId}/pages', requirements: ['contextId' => '\d+'])]
public function create(int $contextId, string $title, ?string $technicalName = null, ?string $icon = null, ?string $description = null): DataResponse {
try {
return new DataResponse($this->pageService->create($contextId, $title, $technicalName, $icon, $description, $this->userId));
} catch (BadRequestError $e) {
return $this->handleBadRequestError($e);
} catch (PermissionError $e) {
return $this->handlePermissionError($e);
} catch (NotFoundError $e) {
return $this->handleNotFoundError($e);
} catch (Exception $e) {
return $this->handleError($e);
}
}

/**
* [api v2] Change a page or its widgets
*
* Fields left out keep their value; an empty icon or description removes it. A given
* `widgets` list replaces the widgets of the grid page: listed widgets with an `id`
* are updated, others are added, and widgets not listed are deleted.
*
* A widget has a `type`, `gridX`, `gridY`, `gridWidth` and `gridHeight` on a grid of
* 12 columns, `settings` following the schema of its type, and optionally an `id`,
* `title`, `icon` and `description`. A data widget's `source` is `{nodeRelId}` of a
* table or view of this application, or null.
*
* @param int $contextId ID of the application
* @param int $pageId ID of the page
* @param string|null $title New title
* @param string|null $technicalName New technical name
* @param string|null $icon New icon name, empty to remove it
* @param string|null $description New description, empty to remove it
* @param list<array<string, mixed>>|null $widgets All widgets of the page

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The generator turns list<array<string, mixed>> into additionalProperties: {type: object}: openapi.json says every field of a widget is an object, and openapi.ts types each one as Record<string, never>, so a client built from the spec can't send type: 'header' or gridX: 0 without a type error, and the widget shape (which fields, which are required, source: {nodeRelId}) exists only in the prose above.

The generator handles array shapes in parameters ($nodes of ContextController::importScheme() comes out with its fields and required list), so a shape such as list<array{id?: ?int, type: string, title?: ?string, icon?: ?string, description?: ?string, gridX: int, gridY: int, gridWidth: int, gridHeight: int, settings?: ?array<string, TablesWidgetValue>, source?: ?array{nodeRelId: int}}>, best as a named type in ResponseDefinitions (the internal TablesWidgetInput in PageService describes the parsed form), documents what the endpoint accepts. It would also be the natural input type for the value object discussed in #3132 (comment).

* @return DataResponse<Http::STATUS_OK, TablesPage, array{}>|DataResponse<Http::STATUS_BAD_REQUEST|Http::STATUS_FORBIDDEN|Http::STATUS_NOT_FOUND|Http::STATUS_INTERNAL_SERVER_ERROR, array{message: string}, array{}>
*
* 200: Page updated
* 400: Invalid page field or widget, too many widgets, or the page changed in between
* 403: No permissions
* 404: Not found
*/
#[NoAdminRequired]
#[UserRateLimit(limit: 60, period: 60)]
#[RequirePermission(permission: Application::PERMISSION_MANAGE, typeParam: 'context', idParam: 'contextId')]
#[ApiRoute(verb: 'PATCH', url: '/api/2/contexts/{contextId}/pages/{pageId}', requirements: ['contextId' => '\d+', 'pageId' => '\d+'])]
public function update(int $contextId, int $pageId, ?string $title = null, ?string $technicalName = null, ?string $icon = null, ?string $description = null, ?array $widgets = null): DataResponse {
try {
return new DataResponse($this->pageService->update($contextId, $pageId, $title, $technicalName, $icon, $description, $widgets, $this->userId));
} catch (BadRequestError $e) {
return $this->handleBadRequestError($e);
} catch (PermissionError $e) {
return $this->handlePermissionError($e);
} catch (NotFoundError $e) {
return $this->handleNotFoundError($e);
} catch (DoesNotExistException|MultipleObjectsReturnedException $e) {
return $this->handleNotFoundError(new NotFoundError($e->getMessage(), $e->getCode(), $e));
} catch (Exception $e) {
return $this->handleError($e);
}
}

/**
* [api v2] Delete a grid page with its widgets
*
* The start page of an application cannot be deleted.
*
* @param int $contextId ID of the application
* @param int $pageId ID of the page
* @return DataResponse<Http::STATUS_OK, TablesPage, array{}>|DataResponse<Http::STATUS_BAD_REQUEST|Http::STATUS_FORBIDDEN|Http::STATUS_NOT_FOUND|Http::STATUS_INTERNAL_SERVER_ERROR, array{message: string}, array{}>
*
* 200: Page deleted
* 400: The page is the start page
* 403: No permissions
* 404: Not found
*/
#[NoAdminRequired]
#[RequirePermission(permission: Application::PERMISSION_MANAGE, typeParam: 'context', idParam: 'contextId')]
#[ApiRoute(verb: 'DELETE', url: '/api/2/contexts/{contextId}/pages/{pageId}', requirements: ['contextId' => '\d+', 'pageId' => '\d+'])]
Comment on lines +164 to +166

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

AGENTS.md asks for #[UserRateLimit(limit: 20, period: 60)] on mutation endpoints. create() and update() carry one, destroy() doesn't: on the dev instance 25 DELETEs in 16 seconds all returned 200, while the 21st POST within a minute got a 429.

public function destroy(int $contextId, int $pageId): DataResponse {
try {
return new DataResponse($this->pageService->delete($contextId, $pageId, $this->userId));
} catch (BadRequestError $e) {
return $this->handleBadRequestError($e);
} catch (PermissionError $e) {
return $this->handlePermissionError($e);
} catch (NotFoundError $e) {
return $this->handleNotFoundError($e);
} catch (Exception $e) {
return $this->handleError($e);
}
}

/**
* [api v2] Get the widget types a grid page can hold
*
* Every type carries the schema of its settings and whether it shows a table or view.
*
* @return DataResponse<Http::STATUS_OK, list<TablesWidgetType>, array{}>
*
* 200: Widget types returned
*/
#[NoAdminRequired]
#[ApiRoute(verb: 'GET', url: '/api/2/widget-types')]
public function widgetTypes(): DataResponse {
return new DataResponse(array_values($this->gridWidgetTypes->all()));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The data type has no settings, and json_encode() writes its empty settings array as [], while TablesWidgetType and the generated spec declare an object (additionalProperties: WidgetSettingSchema). On the dev instance GET /api/2/widget-types returns "settings":[] for data and objects for the other two types, so a client generated from openapi.json that maps this to a dictionary (Kotlin, Swift, Dart) can't read the list, and a response validator flags it. The Vue code in the stack only gets by because Object.entries([]) is empty.

Widget::getSettingsObject() and the |\stdClass on TablesWidget.settings already handle this for a widget's own settings. Returning new \stdClass() here only when a type has no settings (typed array<string, TablesWidgetSettingSchema>|\stdClass), and asserting {} for data in testWidgetTypesListsEveryType(), fixes it; casting every schema to an object would lose the non-empty ones in OCS XML (see the comment on Widget::getSettingsObject()).

}
}
16 changes: 16 additions & 0 deletions lib/Db/PageMapper.php
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@

namespace OCA\Tables\Db;

use OCP\AppFramework\Db\DoesNotExistException;
use OCP\AppFramework\Db\MultipleObjectsReturnedException;
use OCP\AppFramework\Db\QBMapper;
use OCP\AppFramework\Db\TTransactional;
use OCP\DB\Exception;
Expand Down Expand Up @@ -36,6 +38,20 @@ public function insertWithDefaultName(Page $page): Page {
}, $this->db);
}

/**
* @throws DoesNotExistException
* @throws MultipleObjectsReturnedException
* @throws Exception
*/
public function findById(int $pageId): Page {
$qb = $this->db->getQueryBuilder();
$qb->select('*')
->from($this->table)
->where($qb->expr()->eq('id', $qb->createNamedParameter($pageId, IQueryBuilder::PARAM_INT)));

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

/**
* @return int[]
*/
Expand Down
35 changes: 6 additions & 29 deletions lib/Db/Widget.php
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,12 @@
namespace OCA\Tables\Db;

use OCP\AppFramework\Db\Entity;
use OCP\DB\Types;

/**
* A widget on a grid page: its type, optional title, icon and description, its place
* on the grid and its settings, stored as one JSON object.
*
* @psalm-type TablesWidgetSettingValue = string|int|float|bool|null
*
* @method getPageId(): int
* @method setPageId(int $value): void
* @method getType(): string
Expand All @@ -35,8 +34,8 @@
* @method setGridWidth(int $value): void
* @method getGridHeight(): int
* @method setGridHeight(int $value): void
* @method getSettings(): ?string
* @method setSettings(?string $value): void
* @method getSettings(): ?array
* @method setSettings(array $value): void
*/
class Widget extends Entity implements \JsonSerializable {
protected ?int $pageId = null;
Expand All @@ -48,7 +47,7 @@ class Widget extends Entity implements \JsonSerializable {
protected ?int $gridY = null;
protected ?int $gridWidth = null;
protected ?int $gridHeight = null;
protected ?string $settings = null;
protected ?array $settings = null;

public function __construct() {
$this->addType('id', 'integer');
Expand All @@ -57,37 +56,15 @@ public function __construct() {
$this->addType('gridY', 'integer');
$this->addType('gridWidth', 'integer');
$this->addType('gridHeight', 'integer');
}

/**
* @return array<string, TablesWidgetSettingValue>
*/
public function getSettingsArray(): array {
$settings = $this->getSettings();
if ($settings === null || $settings === '') {
return [];
}
$decoded = json_decode($settings, true);
/** @var array<string, TablesWidgetSettingValue> */
return is_array($decoded) ? $decoded : [];
}

/**
* Stores the settings as one JSON object, `{}` when there are none.
*
* @param array<string, TablesWidgetSettingValue> $settings
* @throws \JsonException
*/
public function setSettingsArray(array $settings): void {
$this->setSettings(json_encode((object)$settings, JSON_THROW_ON_ERROR));
$this->addType('settings', Types::JSON);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

string (or array) not JSON

}

/**
* The settings as an object, so they serialise as a JSON object also when empty,
* ordered by name: a JSON column does not keep the stored order on every database.
*/
public function getSettingsObject(): \stdClass {
$settings = $this->getSettingsArray();
$settings = $this->getSettings() ?? [];

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The removed getSettingsArray() turned a stored value that isn't a JSON object into {}, and the deleted WidgetTest case "not an object" covered that. With Types::JSON and a ?array property, Nextcloud's Entity setter assigns whatever json_decode() returns, so a stored scalar such as "text" now throws a TypeError while the row is hydrated (broken JSON still decodes to null and is fine). The app doesn't write such values itself, but a manual fix, a restored dump or a later migration can, and then the page can't be read, saved or deleted through this API (the controller doesn't catch \Error), and the exports that read its widgets fail too.

Reproduced on the dev instance by setting one widget's settings to "text": GET and DELETE .../pages/{id} answered 500 with "Cannot assign string to property OCA\Tables\Db\Widget::$settings of type ?array", and so did the application's scheme export.

Whatever replaces the JSON type after blizzz's comment above, an is_array() guard where the stored value is decoded keeps one bad row from locking the whole page.

ksort($settings);
return (object)$settings;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The page endpoints return every widget's settings as a \stdClass, and Nextcloud's OCS XML renderer turns any \stdClass into an empty element (BaseResponse::toXML(): if ($v instanceof \stdClass) { $v = []; }). XML is what OCS answers when a client sends neither format=json nor Accept: application/json, so in that format GET, PATCH and DELETE .../pages/{pageId} return <settings/> for every widget and its heading, text and colours are gone. On the dev instance the same page gives {"text": "Edited by A"} for a text widget in JSON and <settings/> in XML.

Returning the sorted array when it has entries and new \stdClass() only when it is empty keeps the JSON output as it is (TablesWidget.settings already allows both) and keeps the values in XML.

}
Expand Down
44 changes: 44 additions & 0 deletions lib/ResponseDefinitions.php
Original file line number Diff line number Diff line change
Expand Up @@ -243,6 +243,50 @@
* ownerType: int,
* }
*
* @psalm-type TablesWidgetValue = string|int|float|bool|null
*
* @psalm-type TablesWidgetSettingSchema = array{
* type: 'string'|'text'|'color'|'boolean'|'enum',
* title: string,
* default: TablesWidgetValue,
* options?: list<array{value: string, label: string}>,
* required?: bool,
* }
*
* @psalm-type TablesWidgetType = array{
* type: string,
* title: string,
* defaultWidth: int,
* defaultHeight: int,
* hasSource: bool,
* settings: array<string, TablesWidgetSettingSchema>,
* }
*
* @psalm-type TablesWidget = array{
* id: int,
* type: string,
* title: ?string,
* icon: ?string,
* description: ?string,
* gridX: int,
* gridY: int,
* gridWidth: int,
* gridHeight: int,
* settings: array<string, TablesWidgetValue>|\stdClass,
* source: ?array{nodeRelId: int},
* }
*
* @psalm-type TablesPage = array{
* id: int,
* contextId: int,
* pageType: string,
* technicalName: string,
* title: ?string,
* icon: ?string,
* description: ?string,
* widgets: list<TablesWidget>,
* }
*
* @psalm-type TablesContextNavigation = array{
* id: int,
* shareId: int,
Expand Down
1 change: 0 additions & 1 deletion lib/Service/ContextService.php
Original file line number Diff line number Diff line change
Expand Up @@ -798,7 +798,6 @@ public function getPagesWithWidgets(Context $context): array {
*
* @param array<array-key, ContextNodeRelation> $nodeRelMap imported node relations of the table by their exported id
* @throws Exception
* @throws \JsonException
*/
private function importWidget(int $pageId, int $index, mixed $widgetArr, array $nodeRelMap): void {
if (!is_array($widgetArr) || !is_string($widgetArr['type'] ?? null)) {
Expand Down
Loading
Loading