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
2 changes: 1 addition & 1 deletion appinfo/info.xml
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ Share your tables and views with users and groups within your cloud.
Have a good time and manage whatever you want.

]]></description>
<version>2.4.0</version>
<version>2.5.0-dev.0</version>

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.

#2476 bumps to the same 2.5.0-dev.0 and ships its own migration (Version2500Date20261031000000). The two branches merge without a conflict on this line, so whichever lands second leaves the version unchanged, and Nextcloud only upgrades an app when the info.xml version differs from installed_version (AppManager::isUpgradeRequired()).

On an instance that already runs the first one (dev or nightly setups tracking main), Version2500Date20261022000000 then stays pending until the next bump: tables_contexts_page has no technical_name column, and ContextService::insertPage() now always writes it, so creating any application fails with an unknown-column error. A dev instance that ran #2476 is in exactly that state (installed_version 2.5.0-dev.0, page table without technical_name). In the other merge order, #2476's archive tables are the ones missing.

Whichever of the two lands second needs to bump past what main has at that point, and #3135 then one higher.

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.

One more thing on the ordering, checked on a dev instance running #2476: it has already applied Version2500Date20261031000000 and Version2500Date20261101000000, while this PR's migration is Version2500Date20261022000000, so it sorts before both. Whichever lands second, please rename it to the then-current target version and a current date, as AGENTS.md asks for long-lived PRs, so it sorts after everything already applied.

<licence>AGPL-3.0-or-later</licence>
<author>Nextcloud GmbH and Nextcloud contributors</author>
<namespace>Tables</namespace>
Expand Down
7 changes: 2 additions & 5 deletions lib/Controller/ContextController.php
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
use OCA\Tables\Errors\InternalError;
use OCA\Tables\Errors\NotFoundError;
use OCA\Tables\Errors\PermissionError;
use OCA\Tables\Helper\IconHelper;
use OCA\Tables\Middleware\Attribute\RequirePermission;
use OCA\Tables\ResponseDefinitions;
use OCA\Tables\Service\ColumnService;
Expand Down Expand Up @@ -386,11 +387,7 @@ public function importScheme(int $contextId, string $name, string $iconName, str
}

protected function isValidIcon(string $iconName): bool {

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.

Now that the rule lives in IconHelper, this method only forwards to it. Nothing overrides or mocks it, and the later PRs call IconHelper::isValid() directly, so the two callers (lines 114 and 155) could do the same and the method could go.

if ($iconName === '' || !preg_match('/^[a-zA-Z0-9-]+$/', $iconName)) {
return false;
}
$iconPath = dirname(__DIR__, 2) . '/img/material/' . $iconName . '.svg';
return file_exists($iconPath);
return IconHelper::isValid($iconName);
}

/**
Expand Down
27 changes: 27 additions & 0 deletions lib/Db/Page.php
Original file line number Diff line number Diff line change
Expand Up @@ -15,22 +15,49 @@
* @method setContextId(int $value): void
* @method getPageType(): string
* @method setPageType(string $value): void
* @method getTechnicalName(): ?string
* @method setTechnicalName(?string $value): void
* @method getTitle(): ?string
* @method setTitle(?string $value): void
* @method getIcon(): ?string
* @method setIcon(?string $value): void
* @method getDescription(): ?string
* @method setDescription(?string $value): void
*/
class Page extends Entity implements \JsonSerializable {
public const TYPE_STARTPAGE = 'startpage';
public const TYPE_GRID = 'grid';
public const START_PAGE_NAME = 'dashboard';
public const DEFAULT_NAME_PREFIX = 'page_';

protected ?int $contextId = null;
protected ?string $pageType = null;
protected ?string $technicalName = null;

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.

Same concern for technical names as noted about menu entries in #3100 (comment)

protected ?string $title = null;
protected ?string $icon = null;
protected ?string $description = null;

public function __construct() {
$this->addType('id', 'integer');
}

/**
* Whether a technical name has the form of the automatic `page_<id>`. Such names are
* reserved, so a given name never collides with the name a later page gets.
*/
public static function isDefaultName(string $technicalName): bool {
return preg_match('/^' . self::DEFAULT_NAME_PREFIX . '[0-9]+$/', $technicalName) === 1;
}

public function jsonSerialize(): array {
return [
'id' => $this->getId(),
'contextId' => $this->getContextId(),
'pageType' => $this->getPageType(),
'technicalName' => $this->getTechnicalName(),
'title' => $this->getTitle(),
'icon' => $this->getIcon(),
'description' => $this->getDescription(),
];
}
}
17 changes: 17 additions & 0 deletions lib/Db/PageMapper.php
Original file line number Diff line number Diff line change
Expand Up @@ -8,17 +8,34 @@
namespace OCA\Tables\Db;

use OCP\AppFramework\Db\QBMapper;
use OCP\AppFramework\Db\TTransactional;
use OCP\DB\Exception;
use OCP\DB\QueryBuilder\IQueryBuilder;
use OCP\IDBConnection;

/** @template-extends QBMapper<Page> */
class PageMapper extends QBMapper {
use TTransactional;

protected string $table = 'tables_contexts_page';

public function __construct(IDBConnection $db) {
parent::__construct($db, $this->table, Page::class);
}

/**
* Inserts a page under the default technical name `page_<id>`.
*
* @throws Exception
*/
public function insertWithDefaultName(Page $page): Page {
return $this->atomic(function () use ($page): Page {
$page = $this->insert($page);
$page->setTechnicalName(Page::DEFAULT_NAME_PREFIX . $page->getId());

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.

QBMapper::insert() doesn't reset the updated fields, so the update() below re-sends every field set before the insert (context_id, page_type, ...) together with technical_name. Resetting first narrows it to SET technical_name = ?, the same way ContextService::create() resets after its insert.

Suggested change
$page->setTechnicalName(Page::DEFAULT_NAME_PREFIX . $page->getId());
$page->resetUpdatedFields();
$page->setTechnicalName(Page::DEFAULT_NAME_PREFIX . $page->getId());

return $this->update($page);
}, $this->db);
}

/**
* @return int[]
*/
Expand Down
110 changes: 110 additions & 0 deletions lib/Db/Widget.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,110 @@
<?php

declare(strict_types=1);

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

namespace OCA\Tables\Db;

use OCP\AppFramework\Db\Entity;

/**
* 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
* @method setType(string $value): void
* @method getTitle(): ?string
* @method setTitle(?string $value): void
* @method getIcon(): ?string
* @method setIcon(?string $value): void
* @method getDescription(): ?string
* @method setDescription(?string $value): void
* @method getGridX(): int
* @method setGridX(int $value): void
* @method getGridY(): int
* @method setGridY(int $value): void
* @method getGridWidth(): int
* @method setGridWidth(int $value): void
* @method getGridHeight(): int
* @method setGridHeight(int $value): void
* @method getSettings(): ?string
* @method setSettings(?string $value): void
*/
class Widget extends Entity implements \JsonSerializable {
protected ?int $pageId = null;
protected ?string $type = null;
protected ?string $title = null;
protected ?string $icon = null;
protected ?string $description = null;
protected ?int $gridX = null;
protected ?int $gridY = null;
protected ?int $gridWidth = null;
protected ?int $gridHeight = null;
protected ?string $settings = null;

public function __construct() {
$this->addType('id', 'integer');
$this->addType('pageId', 'integer');
$this->addType('gridX', 'integer');
$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));
}

/**
* 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();
ksort($settings);
return (object)$settings;
}

public function jsonSerialize(): array {

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.

Nothing in the stack calls this outside the tests. The API and export shape is built by hand in WidgetService::findByPageIds() (#3132) and differs from this one: it has source and no pageId, matching TablesWidget in #3133. Dropping JsonSerializable and asserting getSettingsObject() directly in WidgetTest and WidgetMapperTest would leave one widget shape.

return [
'id' => $this->getId(),
'pageId' => $this->getPageId(),
'type' => $this->getType(),
'title' => $this->getTitle(),
'icon' => $this->getIcon(),
'description' => $this->getDescription(),
'gridX' => $this->getGridX(),
'gridY' => $this->getGridY(),
'gridWidth' => $this->getGridWidth(),
'gridHeight' => $this->getGridHeight(),
'settings' => $this->getSettingsObject(),
];
}
}
78 changes: 78 additions & 0 deletions lib/Db/WidgetMapper.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
<?php

declare(strict_types=1);

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

namespace OCA\Tables\Db;

use OCP\AppFramework\Db\QBMapper;
use OCP\DB\Exception;
use OCP\DB\QueryBuilder\IQueryBuilder;
use OCP\IDBConnection;

/** @template-extends QBMapper<Widget> */
class WidgetMapper extends QBMapper {
protected string $table = 'tables_contexts_widget';

public function __construct(IDBConnection $db) {
parent::__construct($db, $this->table, Widget::class);
}

/**
* @param int[] $pageIds
* @return Widget[] ordered by page and grid position
* @throws Exception
*/
public function findByPageIds(array $pageIds): array {
if ($pageIds === []) {
return [];
}
$qb = $this->db->getQueryBuilder();
$qb->select('*')
->from($this->table)
->where($qb->expr()->in('page_id', $qb->createNamedParameter($pageIds, IQueryBuilder::PARAM_INT_ARRAY)))

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: "IN clauses must be chunked to at most 1 000 items for Oracle compatibility. Use a named constant (DB_CHUNK_SIZE = 1_000)", as ContextMapper, ViewMapper, TableMapper, Row2Mapper and RowSleeveMapper do. None of the five new IN queries chunk: findByPageIds() and deleteByIds() here, and findNodeRelIdsByWidgetIds(), deleteByWidgetIds() and deleteByNodeRelIds() in WidgetSourceMapper.

In the stack, deleting or exporting an application passes every widget id of that application. With 100 widgets per page and no page limit, more than 1000 ids are reachable, and the server then logs "More than 1000 expressions in a list are not allowed on Oracle." at error level on every database. When chunking findNodeRelIdsByWidgetIds(), write each chunk into the same map: array_merge(...) would renumber the widget-id keys.

Related, in #3132: WidgetService::deleteByPageIds() calls findIdsByPageId() once per page with array_merge inside the loop, which AGENTS.md also rules out. findByPageIds() already covers it in one query.

->orderBy('page_id', 'ASC')
->addOrderBy('grid_y', 'ASC')
->addOrderBy('grid_x', 'ASC')
->addOrderBy('id', 'ASC');

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

/**
* @return int[]
* @throws Exception
*/
public function findIdsByPageId(int $pageId): array {
$qb = $this->db->getQueryBuilder();
$qb->select('id')
->from($this->table)
->where($qb->expr()->eq('page_id', $qb->createNamedParameter($pageId, IQueryBuilder::PARAM_INT)));

$result = $qb->executeQuery();
$ids = [];
while ($row = $result->fetchAssociative()) {
$ids[] = (int)$row['id'];
}
$result->closeCursor();
return $ids;
}

/**
* @param int[] $widgetIds
* @throws Exception
*/
public function deleteByIds(array $widgetIds): void {
if ($widgetIds === []) {
return;
}
$qb = $this->db->getQueryBuilder();
$qb->delete($this->table)
->where($qb->expr()->in('id', $qb->createNamedParameter($widgetIds, IQueryBuilder::PARAM_INT_ARRAY)))
->executeStatement();
}
}
31 changes: 31 additions & 0 deletions lib/Db/WidgetSource.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
<?php

declare(strict_types=1);

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

namespace OCA\Tables\Db;

use OCP\AppFramework\Db\Entity;

/**
* The node of its application a data widget shows. The widget id is the key;
* the inherited id is not used.
*
* @method getWidgetId(): int
* @method setWidgetId(int $value): void
* @method getNodeRelId(): int
* @method setNodeRelId(int $value): void
*/
class WidgetSource extends Entity {
protected ?int $widgetId = null;
protected ?int $nodeRelId = null;

public function __construct() {
$this->addType('widgetId', 'integer');
$this->addType('nodeRelId', 'integer');
}
}
Loading
Loading