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
13 changes: 9 additions & 4 deletions lib/Controller/ContextController.php
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@ public function show(int $contextId): DataResponse {
* @param string $description Descriptive text of the context
* @psalm-param list<array{id: int, type: int, permissions?: int}> $nodes optional nodes to be connected to this context
* @psalm-param ?list<array{label: string, icon?: string, targetType: 'view'|'table'|'url', targetId?: int, url?: string, technicalName?: string, parentIndex?: int}> $menuItems optional menu of the context, in order; parentIndex is the position of the parent item, for one level of nesting; the views and tables it opens become nodes of the context
* @param ?string $technicalName names the context in its address, lowercase letters, numbers and underscores; unique per owner
*
* @return DataResponse<Http::STATUS_OK, TablesContext, array{}>|DataResponse<Http::STATUS_INTERNAL_SERVER_ERROR|Http::STATUS_BAD_REQUEST|Http::STATUS_FORBIDDEN, array{message: string}, array{}>
*
Expand All @@ -111,7 +112,7 @@ public function show(int $contextId): DataResponse {
* 403: lacking permissions on a resource
*/
#[NoAdminRequired]
public function create(string $name, string $iconName, string $description = '', array $nodes = [], ?array $menuItems = null): DataResponse {
public function create(string $name, string $iconName, string $description = '', array $nodes = [], ?array $menuItems = null, ?string $technicalName = null): DataResponse {
try {
if (!$this->isValidIcon($iconName)) {
return new DataResponse(['message' => 'Invalid icon name'], Http::STATUS_BAD_REQUEST);
Expand All @@ -124,6 +125,7 @@ public function create(string $name, string $iconName, string $description = '',
$this->userId,
0,
$menuItems !== null ? MenuItemInput::createListFromInputArray($menuItems) : null,
$technicalName,
)->jsonSerialize());
} catch (Exception $e) {
return $this->handleError($e);
Expand All @@ -143,6 +145,7 @@ public function create(string $name, string $iconName, string $description = '',
* @param ?string $description provide this parameter to set a new description
* @param ?array{id: int, type: int, permissions: int, order: int} $nodes provide this parameter to set a new list of nodes.
* @psalm-param ?list<array{label: string, icon?: string, targetType: 'view'|'table'|'url', targetId?: int, url?: string, technicalName?: string, parentIndex?: int}> $menuItems provide this parameter to replace the menu; parentIndex is the position of the parent item, for one level of nesting; the views and tables it opens become nodes of the context, and an item without a technicalName keeps the one stored for the same target
* @param ?string $technicalName provide this parameter to set a new technical name; an empty one removes it
*
* @return DataResponse<Http::STATUS_OK, TablesContext, array{}>|DataResponse<Http::STATUS_INTERNAL_SERVER_ERROR|Http::STATUS_NOT_FOUND|Http::STATUS_FORBIDDEN|Http::STATUS_BAD_REQUEST, array{message: string}, array{}>
*
Expand All @@ -154,7 +157,7 @@ public function create(string $name, string $iconName, string $description = '',
* @CanManageContext
*/
#[NoAdminRequired]
public function update(int $contextId, ?string $name, ?string $iconName, ?string $description, ?array $nodes, ?array $menuItems = null): DataResponse {
public function update(int $contextId, ?string $name, ?string $iconName, ?string $description, ?array $nodes, ?array $menuItems = null, ?string $technicalName = null): DataResponse {
try {
if ($iconName !== null && !$this->isValidIcon($iconName)) {
return new DataResponse(['message' => 'Invalid icon name'], Http::STATUS_BAD_REQUEST);
Expand All @@ -168,6 +171,7 @@ public function update(int $contextId, ?string $name, ?string $iconName, ?string
$description,
$nodes,
$menuItems !== null ? MenuItemInput::createListFromInputArray($menuItems) : null,
$technicalName,
)->jsonSerialize());
} catch (Exception|MultipleObjectsReturnedException $e) {
return $this->handleError($e);
Expand Down Expand Up @@ -364,6 +368,7 @@ public function previewSchemeChanges(int $contextId, array $updateScheme): DataR
* @psalm-param array{addTables: list<TablesTable>, modifyTables: list<TablesTable>} $tables Tables to be added or modified
* @psalm-param list<array{label: string, icon?: string, targetType: 'view'|'table'|'url', targetUuid?: string, url?: string, technicalName?: string, parentTechnicalName?: string}> $menuItems Menu of the context, targets by uuid, parents by technical name
* @psalm-param list<array<string, mixed>> $gridViews Views without a table that the menu points at
* @param ?string $technicalName technical name of the context
*
* @return DataResponse<Http::STATUS_OK, array<string, mixed>, array{}>|DataResponse<Http::STATUS_INTERNAL_SERVER_ERROR|Http::STATUS_NOT_FOUND|Http::STATUS_FORBIDDEN|Http::STATUS_BAD_REQUEST, array{message: string}, array{}>
*
Expand All @@ -374,9 +379,9 @@ public function previewSchemeChanges(int $contextId, array $updateScheme): DataR
*/
#[NoAdminRequired]
#[RequirePermission(permission: Application::PERMISSION_MANAGE, typeParam: 'context', idParam: 'contextId')]
public function importScheme(int $contextId, string $name, string $iconName, string $description, array $nodes, array $tables, array $menuItems = [], array $gridViews = []): DataResponse {
public function importScheme(int $contextId, string $name, string $iconName, string $description, array $nodes, array $tables, array $menuItems = [], array $gridViews = [], ?string $technicalName = null): DataResponse {
try {
$scheme = ContextScheme::createFromInputArray(['name' => $name, 'icon' => $iconName, 'description' => $description, 'nodes' => $nodes, 'tables' => $tables, 'menuItems' => $menuItems, 'gridViews' => $gridViews]);
$scheme = ContextScheme::createFromInputArray(['name' => $name, 'icon' => $iconName, 'description' => $description, 'nodes' => $nodes, 'tables' => $tables, 'menuItems' => $menuItems, 'gridViews' => $gridViews, 'technicalName' => $technicalName]);
} catch (InvalidArgumentException $e) {
return $this->handleBadRequestError(new BadRequestError($e->getMessage(), $e->getCode(), $e));
}
Expand Down
17 changes: 11 additions & 6 deletions lib/Controller/PageController.php
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
namespace OCA\Tables\Controller;

use OCA\Tables\AppInfo\Application;
use OCA\Tables\Db\ContextMapper;
use OCA\Tables\Service\NodeService;
use OCA\Tables\Service\ShareService;
use OCA\Text\Event\LoadEditor;
Expand All @@ -32,6 +33,8 @@ public function __construct(
protected IInitialState $initialState,
protected ShareService $shareService,
protected NodeService $nodeService,
protected ContextMapper $contextMapper,
protected ?string $userId,
) {
parent::__construct(Application::APP_ID, $request);
}
Expand All @@ -58,18 +61,20 @@ public function index(): TemplateResponse {
}

/**
* Render default template
*
* @psalm-param int<0, max> $appId
* Render the template for one application, addressed by id or by technical name
*/
#[NoAdminRequired]
#[NoCSRFRequired]
#[OpenAPI(scope: OpenAPI::SCOPE_IGNORE)]
public function context(int $contextId): TemplateResponse {
$navId = Application::APP_ID . '_application_' . $contextId;
public function context(string $contextId): TemplateResponse {
$id = ctype_digit($contextId) ? (int)$contextId : ($this->userId === null ? null : $this->contextMapper->findIdByTechnicalName($contextId, $this->userId));
if ($id === null) {
return $this->index();
}
$navId = Application::APP_ID . '_application_' . $id;
$this->navigationManager->setActiveEntry($navId);

$this->initialState->provideInitialState('contextId', $contextId);
$this->initialState->provideInitialState('contextId', $id);

return $this->index();
}
Expand Down
4 changes: 4 additions & 0 deletions lib/Db/Context.php
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@
* @method setOwnerId(string $value): void
* @method getOwnerType(): int
* @method setOwnerType(int $value): void
* @method getTechnicalName(): ?string
* @method setTechnicalName(?string $value): void
*
* @method getSharing(): array
* @method setSharing(array $value): void
Expand All @@ -37,6 +39,7 @@ class Context extends EntitySuper implements JsonSerializable {
protected ?string $description = null;
protected ?string $ownerId = null;
protected ?int $ownerType = null;
protected ?string $technicalName = null;

// virtual properties
protected ?array $sharing = null;
Expand All @@ -60,6 +63,7 @@ public function jsonSerialize(): array {
'description' => $this->getDescription(),
'owner' => $this->getOwnerId(),
'ownerType' => $this->getOwnerType(),
'technicalName' => $this->getTechnicalName(),
'menuItems' => array_values($this->menuItems ?? []),
];

Expand Down
48 changes: 48 additions & 0 deletions lib/Db/ContextMapper.php
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ protected function formatResultRows(array $rows, ?string $userId, array $menuIte
'description' => $rows[0]['description'],
'owner_id' => $rows[0]['owner_id'],
'owner_type' => $rows[0]['owner_type'],
'technical_name' => $rows[0]['technical_name'] ?? null,
];

$formatted['sharing'] = array_reduce($rows, function (array $carry, array $item) use ($userId) {
Expand Down Expand Up @@ -238,6 +239,53 @@ public function findById(int $contextId, ?string $userId = null): Context {
return $this->formatResultRows($r, $userId, $this->menuItemMapper->findByContextIds([$contextId]));
}

/**
* The id of the context with this technical name that the user owns, else the oldest one
* shared with them. Technical names are unique per owner only.
*
* @throws Exception
*/
public function findIdByTechnicalName(string $technicalName, string $userId): ?int {
$qb = $this->db->getQueryBuilder();
$qb->selectDistinct(['c.id', 'c.owner_id'])
->from($this->table, 'c');
$this->applyOwnedOrSharedQuery($qb, $userId);
$qb->andWhere($qb->expr()->eq('c.technical_name', $qb->createNamedParameter($technicalName, IQueryBuilder::PARAM_STR)))
->orderBy('c.id', 'ASC');
$result = $qb->executeQuery();
$sharedId = null;
while (($row = $result->fetchAssociative()) !== false) {
if ($row['owner_id'] === $userId) {
$result->closeCursor();
return (int)$row['id'];
}
$sharedId ??= (int)$row['id'];
}
$result->closeCursor();
return $sharedId;
}

/**
* Whether the owner already has another context with this technical name.
*
* @throws Exception
*/
public function isTechnicalNameTaken(string $technicalName, string $ownerId, ?int $exceptContextId = null): bool {
$qb = $this->db->getQueryBuilder();
$qb->select('id')
->from($this->table)
->where($qb->expr()->eq('technical_name', $qb->createNamedParameter($technicalName, IQueryBuilder::PARAM_STR)))
->andWhere($qb->expr()->eq('owner_id', $qb->createNamedParameter($ownerId, IQueryBuilder::PARAM_STR)))
->setMaxResults(1);
if ($exceptContextId !== null) {
$qb->andWhere($qb->expr()->neq('id', $qb->createNamedParameter($exceptContextId, IQueryBuilder::PARAM_INT)));
}
$result = $qb->executeQuery();
$taken = $result->fetchOne() !== false;
$result->closeCursor();
return $taken;
}

/**
* @return Context[]
* @throws Exception
Expand Down
42 changes: 42 additions & 0 deletions lib/Migration/Version2400Date20261014000000.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
<?php

declare(strict_types=1);

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

namespace OCA\Tables\Migration;

use Closure;
use OCP\DB\ISchemaWrapper;
use OCP\DB\Types;
use OCP\Migration\IOutput;
use OCP\Migration\SimpleMigrationStep;
use Override;

/**
* Applications (contexts) get an optional technical name, which addresses them at
* /apps/tables/app/{technicalName}. It is unique per owner, which the service checks.

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.

🟡 Concern — Uniqueness is a check-then-write, with no DB constraint

"Unique per owner" relies on isTechnicalNameTaken() only. In create() that check runs before atomic() starts (ContextService.php:152). Two concurrent creates, or a double submit, can both pass and store the same name. The menu items in this same stack use a unique index (tables_ctx_menu_name_uq, Version2400Date20261012020000.php:45). This layer does not.

Impact: the same two-apps-one-name state as with a transfer: both navigation entries open the same application.

Fix: add $table->addUniqueIndex(['owner_id', 'technical_name'], 'tables_ctx_owner_name_uq') (24 chars, so within the Oracle limit). NULL names stay allowed on sqlite, mysql, mariadb and pgsql, because those databases treat NULLs as distinct. Oracle does not: it treats rows with the same owner_id and a NULL name as duplicates. If Oracle is in scope, that needs a decision first; moving the service check into the transaction alone does not stop two concurrent creates. Either way, map a unique-constraint violation to the same 400.

Done when:

  • grep -q "addUniqueIndex(['owner_id', 'technical_name']" lib/Migration/Version2400Date20261014000000.php
  • CI green on sqlite, mysql, mariadb and pgsql with the index

*/
class Version2400Date20261014000000 extends SimpleMigrationStep {
#[Override]
public function changeSchema(IOutput $output, Closure $schemaClosure, array $options): ?ISchemaWrapper {
/** @var ISchemaWrapper $schema */
$schema = $schemaClosure();

if ($schema->hasTable('tables_contexts_context')) {
$table = $schema->getTable('tables_contexts_context');
if (!$table->hasColumn('technical_name')) {
$table->addColumn('technical_name', Types::STRING, [
'notnull' => false,
'default' => null,
'length' => 200,
]);
}
}

return $schema;
}
}
9 changes: 8 additions & 1 deletion lib/Model/ContextScheme.php
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ public function __construct(
protected ?array $tables = [],
protected array $menuItems = [],
protected array $gridViews = [],
protected ?string $technicalName = null,
) {
}

Expand All @@ -43,7 +44,7 @@ public static function createFromInputArray(array $data): self {
if (!is_string($data['name'] ?? null) || trim($data['name']) === '') {
throw new InvalidArgumentException('A scheme needs a name.');
}
foreach (['icon', 'description'] as $optional) {
foreach (['icon', 'description', 'technicalName'] as $optional) {
if (isset($data[$optional]) && !is_string($data[$optional])) {
throw new InvalidArgumentException('The scheme property "' . $optional . '" must be a string.');
}
Expand All @@ -57,9 +58,14 @@ public static function createFromInputArray(array $data): self {
tables: $tables,
menuItems: array_values($data['menuItems'] ?? []),
gridViews: array_values($data['gridViews'] ?? []),
technicalName: isset($data['technicalName']) && $data['technicalName'] !== '' ? $data['technicalName'] : null,
);
}

public function getTechnicalName(): ?string {
return $this->technicalName;
}

public function getName(): ?string {
return $this->name ?? '';
}
Expand Down Expand Up @@ -108,6 +114,7 @@ public function jsonSerialize(): mixed {
'tables' => $this->tables,
'menuItems' => $this->menuItems,
'gridViews' => $this->gridViews,
'technicalName' => $this->technicalName,
];
}
}
1 change: 1 addition & 0 deletions lib/ResponseDefinitions.php
Original file line number Diff line number Diff line change
Expand Up @@ -287,6 +287,7 @@
* description: string,
* owner: string,
* ownerType: int,
* technicalName: string|null,
* menuItems: list<TablesMenuItem>,
* }
*
Expand Down
Loading
Loading