[rubenvdlinde]

Part of #3099, on top of #3131 (data model). This adds the behaviour #3100 describes for widgets, still without an API or UI.

Size: 1601 lines: 679 code and 922 tests.

What changes

• Saving a page replaces its widgets in one transaction (syncPage() runs in atomic()), matched by id. Existing widgets are updated, new ones added, and missing ones removed.
• Deleting a page or an application deletes its widgets and their sources.
• Removing a table or view from an application deletes only the source rows that point at it, first and in the same transaction as the rest. The widget stays, and later shows that it needs a new table.
• Moving a user's data (TablesMigrator) carries an application's pages, widgets and settings. A widget source only comes along when it points at the table being imported: importContext() maps every node to that table, so a source for another table would land on the wrong one. Imported widgets and page texts are checked the same way as saved ones, and invalid ones are skipped with a warning in the log. An application with more than one table still comes along once per table, an older bug that #C1 fixes.
• Exporting an application's scheme includes its pages and widgets; importing them follows in #C3.
• Widget types (header, text, table or view) are defined in one place on the server, each with the schema its settings are checked against. Colours must be #rgb or #rrggbb, and a header's text alignment is start, center or end.
• Limits: at most 100 widgets per page and 60,000 bytes of settings per widget. An import skips what goes over.
• A widget's source must be a table or view of the page's own application. The service checks this itself, not only the API.

Data exported with widgets can't be imported into an older version of Tables. That belongs in the changelog.

Three smaller changes come with it:

• Context responses now carry each page's title, icon, description and technical name. The context query still joins only the start page; findAll() and findById() load the other pages in one extra query for all applications in the result, so grid pages don't multiply the joined rows. The navigation bar and the permission checks don't read pages and skip that query.
• A grid page no longer gets an empty entry in the start page list.
• Importing skips a start page entry whose table didn't come along. Before, it wrote an empty value into a column that doesn't allow one.

The widget types aren't in ResponseDefinitions yet, because the OpenAPI generator refuses schemas that no endpoint uses. They move there together with the endpoints in the next pull request.

Why nothing breaks

There is still no way to create a widget, so all of this runs on empty tables. Deleting an application or removing a table behaves as before; it now also checks two more tables.

How it was tested

On Nextcloud 34 with PostgreSQL:

• WidgetServiceTest (database, including clearing a widget's title, icon, description and settings, refusing a widget id from another page, and a save failing halfway leaving the page as it was)
• GridWidgetTypesTest and ContextServiceTest
• ContextPagesRoundTripTest (database: export, import, removing a table, deleting an application)
• ContextMapperTest (every page read by findById() and findAll()) and TablesMigratorTest
• a broken export file and a two-table user migration
• the full unit suite on the top of the stack (feat(contexts): carry the menu through export and import #3138), on PostgreSQL and SQLite: 684 tests. WidgetServiceTest compares the widgets as decoded arrays rather than JSON text, so it also holds on MySQL, whose JSON column reorders keys
• composer cs:check, psalm and rector, all clean

🖼️ Screenshots

Backend only, no UI changes.

🏁 Checklist

• ⛑️ Tests (unit and/or integration) are included or not needed
• 🔙 Backport requests are created or not needed: /backport to stableX.X
• 📅 Milestone is set
• 🌸 PR title is meaningful (if it should be in the changelog: is it meaningful to users?)

🤖 AI (if applicable)

• The content of this PR was partly or fully generated using AI

[enjeck]

It seems like we have more test code than feature code. Most of the extra comes from the same case being tested in two or three places, or from tests that check mocks or config instead of behaviour. We should cut out some tests or merge. Having too many tests which are not useful slows down CI and adds clutter

[enjeck]

Could we join only the start page here, and load the page fields in a separate query? Otherwise, imagine someone opens just a start page, we might be wasting time processing stuff that doesn't matter for them? Maybe i misunderstand

[rubenvdlinde]

You understood it right: every grid page multiplied the joined rows by the number of tables and shares. The base query now joins only the start page, as before this PR. findAll() and findById() load the other pages in one extra query for all applications in the result. findForNavBar() and findAllContainingNode() skip it, because the navigation shows names and the permission checks only read the nodes.

[enjeck]

The scheme export now includes pages and widgets, but the scheme import ignores them though. Why?

[rubenvdlinde]

That was a gap: I added the export side and left the import for later. #3201 imports the pages and widgets of a scheme, with a Pages switch on the import screen next to the Menu one.

[enjeck]

We seem to have a bug: If I have an application with 2 tables and export an account. When i import, the application is created twice 🤔

[rubenvdlinde]

Thanks for finding this. It's older than this PR: the migrator called importContext() once per imported table and pointed every node at that one table. #3200 imports the tables and views first and then each application once, with every node on its own table or view.

[enjeck]

Maybe better if values are start, center and end too for consistency

[rubenvdlinde]

Done, the values are start, center and end now, with start as the default.

[enjeck]

The PR says saving a page happens in one transaction, but syncPage() doesn't seem to. Do we need atomic()?

[rubenvdlinde]

Yes. It relied on its caller to open the transaction. syncPage() now runs in atomic() itself, and a test checks that a save failing halfway leaves the page as it was.

[rubenvdlinde]

About the tests: Same here: 40 test methods down to 25, and 1090 test lines down to 923. What's left is mostly database tests for deleting, unlinking and importing widgets, where a mock would only repeat the code.

#3133

[github-actions]

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

(If you believe you should not receive this message, you can add yourself to the blocklist.)

[AndyScherzinger]

Review focused on performance, accessibility, security, clean code and architecture. The inline comments only cover points that aren't in the existing threads here or on #3131; where one follows from an earlier thread, it links to it. This PR has no UI of its own, so the accessibility points are about what the data model allows. Points marked as reproduced were checked on a dev instance (MariaDB) running this branch.

[AndyScherzinger]

update() calls this for every removed node (line 240) before it checks canManageNodeById() for the added ones (line 251), and each call commits on its own. A request refused by that check therefore still removes the nodes, and with this PR it also deletes the source of every data widget that showed them. Re-adding the table creates a new node relation, so those widgets stay without a source: the partial update itself is older, but now it also loses widget configuration.

Reproduced on the dev instance: an application with one table on its start page and a data widget showing it, then PUT /api/2/contexts/{id} with {"name": "renamed", "nodes": [{"id": 999999, "type": 0, "permissions": 1}]}. The response is 403, but the node relation and the start page content are gone and the widget's widget_source row is deleted; only the name is unchanged.

Checking the added nodes before the removal loop, or running all of update() in one atomic() (nested transactions are savepoints in Nextcloud), keeps a refused request from changing anything.

[AndyScherzinger]

Widgets are imported for every importable page, the start page included, while PageService::update() in #3133 refuses widgets on anything but a grid page ("Only grid pages hold widgets.") and delete() refuses the start page. An export whose start page lists widgets (edited, or written by a later version) therefore leaves widgets on the start page that no API call can change or remove, and every later export writes them out again.

Reproduced on the dev instance by calling importContext() with a start page that holds a header and a data widget: both were stored on the start page, the data widget with its remapped source.

importWidget()'s docblock promises to skip what the API wouldn't save, so checking the page type here keeps that promise. (The mirror case, content on a grid page, is imported too, but withOtherPages() never reads it back.)

Suggested change

	$widgets = is_array($pageArr['widgets'] ?? null) ? array_values($pageArr['widgets']) : [];
	$widgets = $page->getPageType() === Page::TYPE_GRID && is_array($pageArr['widgets'] ?? null) ? array_values($pageArr['widgets']) : [];

[AndyScherzinger]

importedKey() now guards the node and content ids, but the page id a few lines up still goes into an array key unchecked: $pageMap[$pageArr['id'] ?? null] = $page; (line 718). A page whose id is an object or an array in contexts.json throws TypeError: Cannot access offset of type array on array, and TablesMigrator::import() rolls back the whole account import, which is the case this PR otherwise turns into skips.

Reproduced on the dev instance (PHP 8.5): importContext() with a grid page whose id is {"x": 1} aborts with that TypeError.

$pageMap is written but never read, also further up the stack, so removing lines 701 and 718 fixes it. If it stays for referencing pages by their exported id, as suggested in #3131 (comment), self::importedKey() would guard it like the other two maps.

[AndyScherzinger]

The new transaction takes its locks in the opposite order to deleteNodeRel(): here the relation is deleted before its page contents, there the contents before the relations. Removing a table or view from an application while that table or view is being deleted can now deadlock (whenever no widget shows it, the first DELETE locks nothing); before, each statement here committed on its own and never held a lock while waiting for another. If deleteNodeRel() is the victim, its catch only logs, TableService::delete() deletes the table anyway, and its node relations, page contents and widget sources stay behind in the other applications. TableService::findAll() then throws a NotFoundError for that node, so the table list answers 404 for everyone who can see one of those applications (checked on the dev instance by leaving such a relation behind: GET /apps/tables/table returns 404 until the application is deleted).

removeNodeRelFromAllPages() also still catches and only logs each failed page-content DELETE. That suited autocommit; inside the transaction it hides the error from atomic(), which commits anyway (on PostgreSQL an aborted transaction then ends in a rollback that PDO doesn't report).

Deleting the contents with pageContentMapper->deleteByNodeRelIds([$nodeRelationId]) before the relation, in deleteNodeRel()'s order, and dropping that catch closes both; findByNodeRelation() can still provide the return value.

[AndyScherzinger]

syncPage() checks the sources and the page's widget ids with plain SELECTs and writes afterwards. Nothing locks the page or the node relations, and widget_source has no foreign key, so the checks don't hold against a concurrent request:

• A node relation removed from the application in between (its sources and the relation deleted and committed) gets its source row re-inserted by setSource(), pointing at a relation that no longer exists. In feat(api): pages and widgets on api/2 #3133 every later save that sends the widget back is then refused by PageService::sourceOf() until someone changes the source.
• A widget deleted by another save in between is "updated" without a matching row (QBMapper::update() ignores the row count), setSource() inserts a source for it, and the save reports success with its edit of that widget lost, instead of the "changed by another save in the meantime" answer feat(api): pages and widgets on api/2 #3133 builds on the isset() check.

Reading the page's widgets (or the page row) and the relations with SELECT ... FOR UPDATE, and locking the relation in the removal paths before deleting its sources, makes the checks hold; checking the UPDATE's affected rows would at least turn the second case into the intended 400.

[AndyScherzinger]

The per-widget rules are assembled by each caller: here five GridWidgetTypes calls plus a source check, in #3133 PageService::readWidgets() the same five plus its own rules, while WidgetService::insert()/syncPage() store whatever arrays they receive (eight positional parameters in insert()). The paths already disagree on rules that should be shared: widgets on non-grid pages (see line 736), and later in the stack the tabs limit (871f122) exists only in readWidgets(), so an imported tabs widget with more than 20 tabs is stored and every later save of its page fails until the tabs are cut down.

AGENTS.md asks for structured input to be parsed into a value object at the boundary and for service methods to take value objects instead of raw arrays. A WidgetInput::createFromInputArray(), or one GridWidgetTypes::readWidget(), used by both callers, with the import catching its exception to skip the widget, keeps the rules in one place and lets WidgetService rely on what it gets. The class docblock there ("Callers ... run writes in a transaction") also no longer matches syncPage(), which now opens its own.

Smaller, in the skip handling here: a widget that isn't an object or has no type is dropped without a log line (line 804), while every other skip is logged with ['exception' => $e], i.e. a full stack trace at warning level per skipped widget, up to 100 per page, for a case the code expects. The source check also throws a BadRequestError only to catch it two lines later; a plain if that logs and returns reads more directly.

[AndyScherzinger]

getPagesWithWidgets() runs for one application at a time, so exporting an account with N applications costs up to 2N widget queries (widgets, then their sources) where one findByPageIds() over the page ids of all applications would do, chunked as discussed in #3131 (comment).

The import has the same per-widget pattern: WidgetService::insert() runs a source check (SELECT ... JOIN) for every data widget although the relation was created in the same new application a moment earlier and the check can't fail, and setSource() runs a DELETE for every freshly inserted widget id.

[AndyScherzinger]

The limit is measured on json_encode() with its default flags, which writes every non-ASCII character as a 6-byte \uXXXX escape (12 for an emoji) and every / as \/. 10000 Cyrillic or Chinese characters come to 60011 bytes and are refused, while ASCII text gets 59989 characters (checked with PHP 8.5). A text widget therefore holds about six times less in Russian, Greek, Arabic, Hindi or Chinese, and the error names a byte count the text is nowhere near (about 20 KB of UTF-8).

Widget::setSettingsArray() stores the same escaped form. Encoding with JSON_UNESCAPED_UNICODE | JSON_UNESCAPED_SLASHES in both places makes the limit apply to the text as written and shrinks what is stored.

[AndyScherzinger]

These tests run against the instance database unless TEST_MODE=local, and the fixture pages have fixed names in a fixed context. If a run stops between setUp() and tearDown() (fatal error, Ctrl-C, a CI timeout), page one of context 990001 stays behind. On every later run setUp() then fails on the unique index tables_ctx_page_name_uq before $this->page is assigned, PHPUnit 9 still calls tearDown(), and this line reads the uninitialised typed property, so the DELETEs by context id below never run and the leftover is never removed.

Reproduced on the dev instance: the class passes (7 tests); after inserting one leftover row (context 990001, one) all 7 tests error with "Duplicate entry '990001-one' for key 'tables_ctx_page_name_uq'" and the row is still there after the run.

One cleanup helper that looks the fixture pages up by context id, deletes their widgets and then the pages and relations, called at the start of setUp() and in tearDown(), lets the class recover on its own. (ContextPagesRoundTripTest records an imported context only after importContext() returns, and its tearDown() swallows every error, so a failing import leaves that context behind silently.)

[AndyScherzinger]

No widget type declares a boolean setting, so this arm, the 'boolean' member of TablesWidgetSettingSchema and everything but string in this method's return type can't be reached and aren't tested (AGENTS.md: no unreachable branches). It looks left over from the earlier showTitle setting, which became the widget's title column. #3133 publishes the union in ResponseDefinitions and the OpenAPI spec, and #3215 renders a switch for it, so dropping it here keeps the API from advertising a setting type the server never produces.