From b78bbed8560911b2201d7bc62daf1b3130503eb1 Mon Sep 17 00:00:00 2001 From: Remko Date: Mon, 12 Oct 2026 13:27:48 +0200 Subject: [PATCH 1/3] feat(contexts): save the menu of an application Creating or updating an application (context) can now set its menu. The menu items are validated at the controller boundary, and invalid input is answered with a 400 instead of a 500. - every save replaces the whole menu inside the same transaction as the rest of the update - the views and tables the menu opens become nodes of the application, with read permission, so everyone it is shared with can open them; the owner must be able to manage them - a menu item without a technical name gets one derived from its label, with a numeric suffix when the menu already uses it - only http and https links are accepted Co-authored-by: Ruben van der Linde Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: Remko --- lib/Controller/ContextController.php | 13 +- lib/Model/MenuItemInput.php | 151 ++++++++++++++++ lib/Service/ContextService.php | 175 ++++++++++++++++++- openapi.json | 78 +++++++++ src/types/openapi/openapi.ts | 28 +++ tests/unit/Model/MenuItemInputTest.php | 72 ++++++++ tests/unit/Service/ContextServiceTest.php | 201 +++++++++++++++++++++- 7 files changed, 707 insertions(+), 11 deletions(-) create mode 100644 lib/Model/MenuItemInput.php create mode 100644 tests/unit/Model/MenuItemInputTest.php diff --git a/lib/Controller/ContextController.php b/lib/Controller/ContextController.php index e87f8ca440..858c1717bb 100644 --- a/lib/Controller/ContextController.php +++ b/lib/Controller/ContextController.php @@ -16,6 +16,7 @@ use OCA\Tables\Errors\NotFoundError; use OCA\Tables\Errors\PermissionError; use OCA\Tables\Middleware\Attribute\RequirePermission; +use OCA\Tables\Model\MenuItemInput; use OCA\Tables\ResponseDefinitions; use OCA\Tables\Service\ColumnService; use OCA\Tables\Service\ContextService; @@ -100,6 +101,7 @@ public function show(int $contextId): DataResponse { * @param string $iconName Material design icon name of the context * @param string $description Descriptive text of the context * @psalm-param list $nodes optional nodes to be connected to this context + * @psalm-param ?list $menuItems optional menu of the context, in order; the views and tables it opens become nodes of the context * * @return DataResponse|DataResponse * @@ -108,7 +110,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 = []): DataResponse { + public function create(string $name, string $iconName, string $description = '', array $nodes = [], ?array $menuItems = null): DataResponse { try { if (!$this->isValidIcon($iconName)) { return new DataResponse(['message' => 'Invalid icon name'], Http::STATUS_BAD_REQUEST); @@ -120,13 +122,14 @@ public function create(string $name, string $iconName, string $description = '', $this->sanitizeInputNodes($nodes), $this->userId, 0, + $menuItems !== null ? MenuItemInput::createListFromInputArray($menuItems) : null, )->jsonSerialize()); } catch (Exception $e) { return $this->handleError($e); } catch (PermissionError $e) { return $this->handlePermissionError($e); } catch (InvalidArgumentException $e) { - return $this->handleError(new InternalError($e->getMessage(), $e->getCode(), $e)); + return $this->handleBadRequestError(new BadRequestError($e->getMessage(), $e->getCode(), $e)); } } @@ -138,6 +141,7 @@ public function create(string $name, string $iconName, string $description = '', * @param ?string $iconName provide this parameter to set a new icon * @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 $menuItems provide this parameter to replace the menu; the views and tables it opens become nodes of the context * * @return DataResponse|DataResponse * @@ -149,7 +153,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): DataResponse { + public function update(int $contextId, ?string $name, ?string $iconName, ?string $description, ?array $nodes, ?array $menuItems = null): DataResponse { try { if ($iconName !== null && !$this->isValidIcon($iconName)) { return new DataResponse(['message' => 'Invalid icon name'], Http::STATUS_BAD_REQUEST); @@ -162,6 +166,7 @@ public function update(int $contextId, ?string $name, ?string $iconName, ?string $iconName, $description, $nodes, + $menuItems !== null ? MenuItemInput::createListFromInputArray($menuItems) : null, )->jsonSerialize()); } catch (Exception|MultipleObjectsReturnedException $e) { return $this->handleError($e); @@ -169,6 +174,8 @@ public function update(int $contextId, ?string $name, ?string $iconName, ?string return $this->handlePermissionError($e); } catch (DoesNotExistException $e) { return $this->handleNotFoundError(new NotFoundError($e->getMessage(), $e->getCode(), $e)); + } catch (InvalidArgumentException $e) { + return $this->handleBadRequestError(new BadRequestError($e->getMessage(), $e->getCode(), $e)); } } diff --git a/lib/Model/MenuItemInput.php b/lib/Model/MenuItemInput.php new file mode 100644 index 0000000000..5f58aba545 --- /dev/null +++ b/lib/Model/MenuItemInput.php @@ -0,0 +1,151 @@ +label) === '' || mb_strlen($this->label) > self::MAX_LABEL_LENGTH) { + throw new InvalidArgumentException('A menu item needs a label of at most ' . self::MAX_LABEL_LENGTH . ' characters.'); + } + if ($this->icon !== null && mb_strlen($this->icon) > self::MAX_ICON_LENGTH) { + throw new InvalidArgumentException('A menu item icon name must not exceed ' . self::MAX_ICON_LENGTH . ' characters.'); + } + if (!in_array($this->targetType, MenuItem::TARGET_TYPES, true)) { + throw new InvalidArgumentException('Unexpected menu item target type, expected one of: ' . implode(', ', MenuItem::TARGET_TYPES) . '.'); + } + if ($this->targetType === MenuItem::TARGET_URL) { + self::assertUrlValid($this->url); + } elseif ($this->targetId === null || $this->targetId <= 0) { + throw new InvalidArgumentException('A menu item pointing at a ' . $this->targetType . ' needs its id.'); + } + if ($this->technicalName !== null) { + self::assertTechnicalNameValid($this->technicalName); + } + } + + /** + * @throws InvalidArgumentException + */ + public static function createFromInputArray(mixed $data): self { + if (!is_array($data)) { + throw new InvalidArgumentException('A menu item must be an object.'); + } + if (!is_string($data['label'] ?? null)) { + throw new InvalidArgumentException('A menu item needs a label.'); + } + $targetType = $data['targetType'] ?? MenuItem::TARGET_URL; + if (!is_string($targetType)) { + throw new InvalidArgumentException('The target type of a menu item must be a string.'); + } + $isUrl = $targetType === MenuItem::TARGET_URL; + $targetId = $data['targetId'] ?? null; + if (!$isUrl && !is_int($targetId)) { + throw new InvalidArgumentException('A menu item pointing at a ' . $targetType . ' needs its id.'); + } + foreach (['icon', 'url', 'technicalName'] as $optional) { + if (isset($data[$optional]) && !is_string($data[$optional])) { + throw new InvalidArgumentException('The menu item property "' . $optional . '" must be a string.'); + } + } + + return new self( + label: trim($data['label']), + icon: self::emptyToNull($data['icon'] ?? null), + targetType: $targetType, + targetId: $isUrl ? null : $targetId, + url: $isUrl ? trim((string)($data['url'] ?? '')) : null, + technicalName: self::emptyToNull($data['technicalName'] ?? null), + ); + } + + /** + * @return list + * @throws InvalidArgumentException + */ + public static function createListFromInputArray(mixed $data): array { + if (!is_array($data) || !array_is_list($data)) { + throw new InvalidArgumentException('The menu items must be a list.'); + } + return array_map(static fn (mixed $item): self => self::createFromInputArray($item), $data); + } + + /** + * @throws InvalidArgumentException + */ + public static function assertTechnicalNameValid(string $technicalName): void { + if (strlen($technicalName) > self::MAX_TECHNICAL_NAME_LENGTH || !preg_match(self::TECHNICAL_NAME_PATTERN, $technicalName)) { + throw new InvalidArgumentException('A technical name must start with a lowercase letter and contain only lowercase letters, numbers and underscores.'); + } + } + + /** + * @throws InvalidArgumentException + */ + private static function assertUrlValid(?string $url): void { + $scheme = $url === null ? null : parse_url($url, PHP_URL_SCHEME); + if ($url === null || mb_strlen($url) > self::MAX_URL_LENGTH + || !is_string($scheme) || !in_array(strtolower($scheme), self::URL_SCHEMES, true) + || parse_url($url, PHP_URL_HOST) === null) { + throw new InvalidArgumentException('A menu item link must be an http or https address of at most ' . self::MAX_URL_LENGTH . ' characters.'); + } + } + + private static function emptyToNull(?string $value): ?string { + if ($value === null) { + return null; + } + $value = trim($value); + return $value === '' ? null : $value; + } + + public function getLabel(): string { + return $this->label; + } + + public function getIcon(): ?string { + return $this->icon; + } + + public function getTargetType(): string { + return $this->targetType; + } + + public function getTargetId(): ?int { + return $this->targetId; + } + + public function getUrl(): ?string { + return $this->url; + } + + public function getTechnicalName(): ?string { + return $this->technicalName; + } +} diff --git a/lib/Service/ContextService.php b/lib/Service/ContextService.php index 32d0ed0c28..bd0ecfca6f 100644 --- a/lib/Service/ContextService.php +++ b/lib/Service/ContextService.php @@ -14,6 +14,7 @@ use OCA\Tables\Db\ContextMapper; use OCA\Tables\Db\ContextNodeRelation; use OCA\Tables\Db\ContextNodeRelationMapper; +use OCA\Tables\Db\MenuItem; use OCA\Tables\Db\MenuItemMapper; use OCA\Tables\Db\Page; use OCA\Tables\Db\PageContent; @@ -29,6 +30,7 @@ use OCA\Tables\Model\ColumnSettings; use OCA\Tables\Model\ContextScheme; use OCA\Tables\Model\FilterSet; +use OCA\Tables\Model\MenuItemInput; use OCA\Tables\Model\SortRuleSet; use OCP\AppFramework\Db\DoesNotExistException; use OCP\AppFramework\Db\MultipleObjectsReturnedException; @@ -132,9 +134,10 @@ public function findById(int $id, ?string $userId): Context { /** * @psalm-param list $nodes + * @param list|null $menuItems * @throws Exception|PermissionError|InvalidArgumentException */ - public function create(string $name, string $iconName, string $description, array $nodes, string $ownerId, int $ownerType): Context { + public function create(string $name, string $iconName, string $description, array $nodes, string $ownerId, int $ownerType, ?array $menuItems = null): Context { $context = new Context(); $context->setName(trim($name)); $context->setIcon(trim($iconName)); @@ -142,7 +145,7 @@ public function create(string $name, string $iconName, string $description, arra $context->setOwnerId($ownerId); $context->setOwnerType($ownerType); - $this->atomic(function () use ($context, $nodes): void { + $this->atomic(function () use ($context, $nodes, $menuItems, $ownerId): void { $this->contextMapper->insert($context); if (!empty($nodes)) { @@ -150,18 +153,179 @@ public function create(string $name, string $iconName, string $description, arra $this->insertNodesFromArray($context, $nodes); } $this->insertPage($context); + $this->replaceMenu($context, $menuItems ?? [], $ownerId); }, $this->dbc); return $context; } + /** + * Replaces the menu of an application and adds the views and tables it points at + * as nodes, so everyone the application is shared with can open them. + * + * @param list $menuItems + * @throws Exception|PermissionError|InvalidArgumentException + */ + protected function replaceMenu(Context $context, array $menuItems, ?string $userId): void { + $this->addMenuTargetsAsNodes($context, $menuItems, $userId); + + $this->menuItemMapper->deleteAllByContextId($context->getId()); + $technicalNames = $this->menuTechnicalNames($menuItems); + foreach ($menuItems as $position => $item) { + $menuItem = new MenuItem(); + $menuItem->setContextId($context->getId()); + $menuItem->setLabel($item->getLabel()); + $menuItem->setIcon($item->getIcon()); + $menuItem->setTargetType($item->getTargetType()); + $menuItem->setTargetId($item->getTargetId()); + $menuItem->setUrl($item->getUrl()); + $menuItem->setTechnicalName($technicalNames[$position]); + $menuItem->setOrder(($position + 1) * 10); + $this->menuItemMapper->insert($menuItem); + } + + $context->setMenuItems(array_map( + static fn (MenuItem $menuItem): array => $menuItem->jsonSerialize(), + $this->menuItemMapper->findByContextId($context->getId()), + )); + } + + /** + * The technical name of every menu item, by position: the one given, or one derived from + * the label with a numeric suffix when this menu already uses it. + * + * @param list $menuItems + * @return array + * @throws InvalidArgumentException + */ + private function menuTechnicalNames(array $menuItems): array { + $names = []; + $taken = []; + foreach ($menuItems as $position => $item) { + $technicalName = $item->getTechnicalName(); + if ($technicalName === null) { + continue; + } + if (isset($taken[$technicalName])) { + throw new InvalidArgumentException('Two menu items share the technical name "' . $technicalName . '".'); + } + $names[$position] = $technicalName; + $taken[$technicalName] = true; + } + foreach ($menuItems as $position => $item) { + if (isset($names[$position])) { + continue; + } + $base = $this->technicalNameFromLabel($item->getLabel()); + $technicalName = $base; + for ($suffix = 2; isset($taken[$technicalName]); $suffix++) { + $technicalName = substr($base, 0, MenuItemInput::MAX_TECHNICAL_NAME_LENGTH - 10) . '_' . $suffix; + } + $names[$position] = $technicalName; + $taken[$technicalName] = true; + } + return $names; + } + + /** + * Targets that no longer exist are left out; the menu marks them broken. + * + * @param list $menuItems + * @throws Exception|PermissionError + */ + private function addMenuTargetsAsNodes(Context $context, array $menuItems, ?string $userId): void { + $missing = []; + foreach ($menuItems as $item) { + $nodeType = match ($item->getTargetType()) { + MenuItem::TARGET_VIEW => Application::NODE_TYPE_VIEW, + MenuItem::TARGET_TABLE => Application::NODE_TYPE_TABLE, + default => null, + }; + $nodeId = $item->getTargetId(); + if ($nodeType !== null && $nodeId !== null) { + $missing[sprintf('t%di%d', $nodeType, $nodeId)] = [$nodeType, $nodeId]; + } + } + $nodes = $context->getNodes() ?? []; + foreach ($nodes as $node) { + unset($missing[sprintf('t%di%d', $node['node_type'] ?? $node['nodeType'], $node['node_id'] ?? $node['nodeId'])]); + } + if ($missing === []) { + return; + } + + $targetIds = [Application::NODE_TYPE_VIEW => [], Application::NODE_TYPE_TABLE => []]; + foreach ($missing as [$nodeType, $nodeId]) { + $targetIds[$nodeType][] = $nodeId; + } + $existing = [ + Application::NODE_TYPE_VIEW => $this->viewMapper->findMany($targetIds[Application::NODE_TYPE_VIEW]), + Application::NODE_TYPE_TABLE => $this->tableMapper->findMany($targetIds[Application::NODE_TYPE_TABLE]), + ]; + + $pages = $context->getPages() ?? []; + $startPageId = null; + foreach ($pages as $pageId => $page) { + if (($page['page_type'] ?? $page['pageType'] ?? null) === Page::TYPE_STARTPAGE) { + $startPageId = (int)$pageId; + break; + } + } + + foreach ($missing as [$nodeType, $nodeId]) { + if (!isset($existing[$nodeType][$nodeId])) { + continue; + } + if (!$this->permissionsService->canManageNodeById($nodeType, $nodeId, $userId)) { + throw new PermissionError(sprintf('Owner cannot manage node %d (type %d)', $nodeId, $nodeType)); + } + $relation = $this->addNodeToContext($context, $nodeId, $nodeType, Application::PERMISSION_READ); + $nodes[$relation->getId()] = [ + 'id' => $relation->getId(), + 'node_id' => $nodeId, + 'node_type' => $nodeType, + 'permissions' => Application::PERMISSION_READ, + ]; + if ($startPageId !== null) { + $content = $this->addNodeRelToPage($relation, null, $startPageId); + $pages[$startPageId]['content'][$content->getId()] = [ + 'order' => $content->getOrder(), + 'node_rel_id' => $relation->getId(), + ]; + } + } + $context->setNodes($nodes); + $context->setPages($pages); + } + + protected function technicalNameFromLabel(string $label): string { + $name = strtolower(trim(preg_replace('/[^a-z0-9]+/i', '_', $label) ?? '', '_')); + $name = preg_replace('/^[^a-z]+/', '', $name) ?? ''; + return substr($name === '' ? 'item' : $name, 0, MenuItemInput::MAX_TECHNICAL_NAME_LENGTH); + } + /** * @psalm-param list $nodes + * @param list|null $menuItems * @throws Exception * @throws DoesNotExistException - * @throws PermissionError|MultipleObjectsReturnedException + * @throws PermissionError|MultipleObjectsReturnedException|InvalidArgumentException */ - public function update(int $contextId, string $userId, ?string $name, ?string $iconName, ?string $description, ?array $nodes): Context { + public function update(int $contextId, string $userId, ?string $name, ?string $iconName, ?string $description, ?array $nodes, ?array $menuItems = null): Context { + return $this->atomic( + fn (): Context => $this->applyUpdate($contextId, $userId, $name, $iconName, $description, $nodes, $menuItems), + $this->dbc, + ); + } + + /** + * @psalm-param list $nodes + * @param list|null $menuItems + * @throws Exception + * @throws DoesNotExistException + * @throws PermissionError|MultipleObjectsReturnedException|InvalidArgumentException + */ + private function applyUpdate(int $contextId, string $userId, ?string $name, ?string $iconName, ?string $description, ?array $nodes, ?array $menuItems): Context { $context = $this->contextMapper->findById($contextId, $userId); if ($name !== null) { @@ -305,6 +469,9 @@ public function update(int $contextId, string $userId, ?string $name, ?string $i $context->setNodes($currentNodes); $context->setPages($currentPages); } + if ($menuItems !== null) { + $this->replaceMenu($context, $menuItems, $userId); + } return $context; } diff --git a/openapi.json b/openapi.json index a816a48ef5..46f89c28f2 100644 --- a/openapi.json +++ b/openapi.json @@ -12013,6 +12013,45 @@ } } } + }, + "menuItems": { + "type": "array", + "nullable": true, + "default": null, + "description": "optional menu of the context, in order; the views and tables it opens become nodes of the context", + "items": { + "type": "object", + "required": [ + "label", + "targetType" + ], + "properties": { + "label": { + "type": "string" + }, + "icon": { + "type": "string" + }, + "targetType": { + "type": "string", + "enum": [ + "view", + "table", + "url" + ] + }, + "targetId": { + "type": "integer", + "format": "int64" + }, + "url": { + "type": "string" + }, + "technicalName": { + "type": "string" + } + } + } } } } @@ -12445,6 +12484,45 @@ "format": "int64" } } + }, + "menuItems": { + "type": "array", + "nullable": true, + "default": null, + "description": "provide this parameter to replace the menu; the views and tables it opens become nodes of the context", + "items": { + "type": "object", + "required": [ + "label", + "targetType" + ], + "properties": { + "label": { + "type": "string" + }, + "icon": { + "type": "string" + }, + "targetType": { + "type": "string", + "enum": [ + "view", + "table", + "url" + ] + }, + "targetId": { + "type": "integer", + "format": "int64" + }, + "url": { + "type": "string" + }, + "technicalName": { + "type": "string" + } + } + } } } } diff --git a/src/types/openapi/openapi.ts b/src/types/openapi/openapi.ts index 9558685b0a..0eeb8a1a25 100644 --- a/src/types/openapi/openapi.ts +++ b/src/types/openapi/openapi.ts @@ -6897,6 +6897,20 @@ export interface operations { /** Format: int64 */ readonly permissions?: number; }[]; + /** + * @description optional menu of the context, in order; the views and tables it opens become nodes of the context + * @default null + */ + readonly menuItems?: readonly { + readonly label: string; + readonly icon?: string; + /** @enum {string} */ + readonly targetType: "view" | "table" | "url"; + /** Format: int64 */ + readonly targetId?: number; + readonly url?: string; + readonly technicalName?: string; + }[] | null; }; }; }; @@ -7087,6 +7101,20 @@ export interface operations { /** Format: int64 */ readonly order: number; } | null; + /** + * @description provide this parameter to replace the menu; the views and tables it opens become nodes of the context + * @default null + */ + readonly menuItems?: readonly { + readonly label: string; + readonly icon?: string; + /** @enum {string} */ + readonly targetType: "view" | "table" | "url"; + /** Format: int64 */ + readonly targetId?: number; + readonly url?: string; + readonly technicalName?: string; + }[] | null; }; }; }; diff --git a/tests/unit/Model/MenuItemInputTest.php b/tests/unit/Model/MenuItemInputTest.php new file mode 100644 index 0000000000..49c9e1cd0f --- /dev/null +++ b/tests/unit/Model/MenuItemInputTest.php @@ -0,0 +1,72 @@ + ' Intake home ', 'targetType' => 'view', 'targetId' => 6, 'technicalName' => 'home', 'url' => 'https://ignored.example'], + ['label' => 'Docs', 'targetType' => 'url', 'url' => 'https://example.org/docs', 'targetId' => 99, 'icon' => 'book'], + ['label' => 'Requests', 'targetType' => 'table', 'targetId' => 4, 'icon' => ''], + ]); + + $this->assertSame(['Intake home', null, 'view', 6, null, 'home'], $this->toArray($items[0])); + $this->assertSame(['Docs', 'book', 'url', null, 'https://example.org/docs', null], $this->toArray($items[1])); + $this->assertSame(['Requests', null, 'table', 4, null, null], $this->toArray($items[2])); + } + + public function testATargetTypeOfUrlIsTheDefault(): void { + $item = MenuItemInput::createFromInputArray(['label' => 'Docs', 'url' => 'http://example.org']); + + $this->assertSame('url', $item->getTargetType()); + } + + public static function invalidItems(): array { + return [ + 'not an object' => ['not an item', 'must be an object'], + 'no label' => [['targetType' => 'url', 'url' => 'https://example.org'], 'needs a label'], + 'blank label' => [['label' => ' ', 'targetType' => 'url', 'url' => 'https://example.org'], 'needs a label'], + 'label too long' => [['label' => str_repeat('x', 201), 'url' => 'https://example.org'], 'needs a label'], + 'unknown target type' => [['label' => 'X', 'targetType' => 'kanban', 'targetId' => 1], 'Unexpected menu item target type'], + 'view without id' => [['label' => 'X', 'targetType' => 'view'], 'needs its id'], + 'id as a string' => [['label' => 'X', 'targetType' => 'table', 'targetId' => '4'], 'needs its id'], + 'id of zero' => [['label' => 'X', 'targetType' => 'table', 'targetId' => 0], 'needs its id'], + 'javascript link' => [['label' => 'X', 'url' => 'javascript:alert(1)'], 'http or https'], + 'data link' => [['label' => 'X', 'url' => 'data:text/html,hi'], 'http or https'], + 'link without host' => [['label' => 'X', 'url' => 'https://'], 'http or https'], + 'empty link' => [['label' => 'X', 'url' => ''], 'http or https'], + 'technical name with a hyphen' => [['label' => 'X', 'url' => 'https://example.org', 'technicalName' => 'my-page'], 'technical name'], + 'icon that is not a string' => [['label' => 'X', 'url' => 'https://example.org', 'icon' => 3], '"icon" must be a string'], + ]; + } + + /** + * @dataProvider invalidItems + */ + public function testRejectsInvalidInput(mixed $input, string $message): void { + $this->expectException(InvalidArgumentException::class); + $this->expectExceptionMessage($message); + MenuItemInput::createFromInputArray($input); + } + + public function testRejectsAMenuThatIsNotAList(): void { + $this->expectException(InvalidArgumentException::class); + $this->expectExceptionMessage('must be a list'); + MenuItemInput::createListFromInputArray(['first' => ['label' => 'X', 'url' => 'https://example.org']]); + } + + private function toArray(MenuItemInput $item): array { + return [$item->getLabel(), $item->getIcon(), $item->getTargetType(), $item->getTargetId(), $item->getUrl(), $item->getTechnicalName()]; + } +} diff --git a/tests/unit/Service/ContextServiceTest.php b/tests/unit/Service/ContextServiceTest.php index 9369e5f9e7..d10d323792 100644 --- a/tests/unit/Service/ContextServiceTest.php +++ b/tests/unit/Service/ContextServiceTest.php @@ -9,16 +9,24 @@ namespace OCA\Tables\Service; +use InvalidArgumentException; +use OCA\Tables\AppInfo\Application; use OCA\Tables\Db\Context; use OCA\Tables\Db\ContextMapper; +use OCA\Tables\Db\ContextNodeRelation; use OCA\Tables\Db\ContextNodeRelationMapper; +use OCA\Tables\Db\MenuItem; use OCA\Tables\Db\MenuItemMapper; use OCA\Tables\Db\Page; use OCA\Tables\Db\PageContent; use OCA\Tables\Db\PageContentMapper; use OCA\Tables\Db\PageMapper; +use OCA\Tables\Db\Table; use OCA\Tables\Db\TableMapper; +use OCA\Tables\Db\View; use OCA\Tables\Db\ViewMapper; +use OCA\Tables\Errors\PermissionError; +use OCA\Tables\Model\MenuItemInput; use OCP\EventDispatcher\IEventDispatcher; use OCP\IDBConnection; use OCP\INavigationManager; @@ -35,6 +43,9 @@ final class ContextServiceTest extends TestCase { private MockObject $pageMapper; private MockObject $pageContentMapper; private MockObject $menuItemMapper; + private MockObject $tableMapper; + private MockObject $viewMapper; + private MockObject $permissionsService; protected function setUp(): void { parent::setUp(); @@ -44,8 +55,10 @@ protected function setUp(): void { $this->pageMapper = $this->createMock(PageMapper::class); $this->pageContentMapper = $this->createMock(PageContentMapper::class); $this->menuItemMapper = $this->createMock(MenuItemMapper::class); + $this->tableMapper = $this->createMock(TableMapper::class); + $this->viewMapper = $this->createMock(ViewMapper::class); + $this->permissionsService = $this->createMock(PermissionsService::class); $logger = $this->createMock(LoggerInterface::class); - $permissionsService = $this->createMock(PermissionsService::class); $userManager = $this->createMock(IUserManager::class); $eventDispatcher = $this->createMock(IEventDispatcher::class); $dbConnection = $this->createMock(IDBConnection::class); @@ -59,7 +72,7 @@ protected function setUp(): void { $this->pageMapper, $this->pageContentMapper, $logger, - $permissionsService, + $this->permissionsService, $userManager, $eventDispatcher, $dbConnection, @@ -67,12 +80,192 @@ protected function setUp(): void { false, $navigationManager, $urlGenerator, - $this->createMock(TableMapper::class), - $this->createMock(ViewMapper::class), + $this->tableMapper, + $this->viewMapper, $this->menuItemMapper, ); } + private function contextWithMenu(int $id, array $menuItems): Context { + $context = new Context(); + $context->id = $id; + $context->setName('Intake portal'); + $context->setOwnerId('user-1'); + $context->setOwnerType(Application::OWNER_TYPE_USER); + $context->setNodes([]); + $context->setPages([]); + $context->setMenuItems($menuItems); + return $context; + } + + /** + * @param list $inserted collects the stored items + */ + private function expectMenuReplacement(int $contextId, array &$inserted): void { + $this->menuItemMapper->expects($this->once())->method('deleteAllByContextId')->with($contextId); + $this->menuItemMapper->method('insert')->willReturnCallback(static function (MenuItem $item) use (&$inserted): MenuItem { + $item->setId(count($inserted) + 1); + $inserted[] = $item; + return $item; + }); + $this->menuItemMapper->method('findByContextId')->willReturnCallback(static function () use (&$inserted): array { + return $inserted; + }); + } + + private function expectExistingTargets(array $views, array $tables): void { + $this->viewMapper->method('findMany')->willReturnCallback(static fn (array $ids): array => array_intersect_key($views, array_flip($ids))); + $this->tableMapper->method('findMany')->willReturnCallback(static fn (array $ids): array => array_intersect_key($tables, array_flip($ids))); + } + + private function expectNodeInsertion(array &$relations): void { + $this->contextNodeRelMapper->method('insert')->willReturnCallback(static function (ContextNodeRelation $relation) use (&$relations): ContextNodeRelation { + $relation->setId(200 + count($relations)); + $relations[] = $relation; + return $relation; + }); + } + + public function testUpdateReplacesTheMenuInOrderAndDerivesTechnicalNamesFromLabels(): void { + $context = $this->contextWithMenu(7, []); + $this->contextMapper->method('findById')->with(7, 'user-1')->willReturn($context); + $this->contextMapper->method('update')->willReturnArgument(0); + $this->permissionsService->method('canManageNodeById')->willReturn(true); + $this->expectExistingTargets([6 => new View()], [4 => new Table()]); + $relations = []; + $this->expectNodeInsertion($relations); + $inserted = []; + $this->expectMenuReplacement(7, $inserted); + + $updated = $this->service->update(7, 'user-1', null, null, null, null, MenuItemInput::createListFromInputArray([ + ['label' => 'Intake home', 'targetType' => 'view', 'targetId' => 6, 'technicalName' => 'home'], + ['label' => 'Welcome table!', 'targetType' => 'table', 'targetId' => 4], + ['label' => 'Docs', 'targetType' => 'url', 'url' => 'https://docs.nextcloud.com'], + ])); + + $this->assertSame([10, 20, 30], array_map(static fn (MenuItem $item) => $item->getOrder(), $inserted)); + $this->assertSame(['home', 'welcome_table', 'docs'], array_map(static fn (MenuItem $item) => $item->getTechnicalName(), $inserted)); + $this->assertSame([6, 4, null], array_map(static fn (MenuItem $item) => $item->getTargetId(), $inserted)); + $this->assertSame([null, null, 'https://docs.nextcloud.com'], array_map(static fn (MenuItem $item) => $item->getUrl(), $inserted)); + $this->assertCount(3, $updated->getMenuItems()); + $this->assertSame('Welcome table!', $updated->getMenuItems()[1]['label']); + } + + public function testMenuTargetsBecomeNodesSoRecipientsCanOpenThem(): void { + $context = $this->contextWithMenu(7, []); + $context->setNodes([101 => ['id' => 101, 'node_id' => 4, 'node_type' => Application::NODE_TYPE_TABLE, 'permissions' => 1]]); + $context->setPages([301 => ['id' => 301, 'page_type' => Page::TYPE_STARTPAGE, 'content' => []]]); + $this->contextMapper->method('findById')->willReturn($context); + $this->contextMapper->method('update')->willReturnArgument(0); + $this->permissionsService->method('canManageNodeById')->willReturn(true); + $this->expectExistingTargets([6 => new View()], [4 => new Table()]); + $relations = []; + $this->expectNodeInsertion($relations); + $this->pageContentMapper->method('findByPageAndNodeRelation')->willReturn(null); + $this->pageContentMapper->method('insert')->willReturnCallback(static function (PageContent $content): PageContent { + $content->setId(501); + return $content; + }); + $inserted = []; + $this->expectMenuReplacement(7, $inserted); + + $updated = $this->service->update(7, 'user-1', null, null, null, null, MenuItemInput::createListFromInputArray([ + ['label' => 'Home', 'targetType' => 'view', 'targetId' => 6], + ['label' => 'Requests', 'targetType' => 'table', 'targetId' => 4], + ])); + + // the table already is a node, only the view is added, with read permission + $this->assertCount(1, $relations); + $this->assertSame([6, Application::NODE_TYPE_VIEW, Application::PERMISSION_READ], [$relations[0]->getNodeId(), $relations[0]->getNodeType(), $relations[0]->getPermissions()]); + $this->assertSame(['id' => 200, 'node_id' => 6, 'node_type' => Application::NODE_TYPE_VIEW, 'permissions' => Application::PERMISSION_READ], $updated->getNodes()[200]); + $this->assertSame(200, $updated->getPages()[301]['content'][501]['node_rel_id']); + } + + public function testAMenuTargetTheOwnerCannotManageIsRefused(): void { + $this->contextMapper->method('findById')->willReturn($this->contextWithMenu(7, [])); + $this->contextMapper->method('update')->willReturnArgument(0); + $this->expectExistingTargets([], [4 => new Table()]); + $this->permissionsService->method('canManageNodeById')->with(Application::NODE_TYPE_TABLE, 4, 'user-1')->willReturn(false); + $this->menuItemMapper->expects($this->never())->method('deleteAllByContextId'); + + $this->expectException(PermissionError::class); + $this->service->update(7, 'user-1', null, null, null, null, MenuItemInput::createListFromInputArray([ + ['label' => 'Requests', 'targetType' => 'table', 'targetId' => 4], + ])); + } + + public function testAMenuTargetThatNoLongerExistsIsKeptWithoutANode(): void { + $this->contextMapper->method('findById')->willReturn($this->contextWithMenu(7, [])); + $this->contextMapper->method('update')->willReturnArgument(0); + $this->expectExistingTargets([], []); + $this->contextNodeRelMapper->expects($this->never())->method('insert'); + $inserted = []; + $this->expectMenuReplacement(7, $inserted); + + $this->service->update(7, 'user-1', null, null, null, null, MenuItemInput::createListFromInputArray([ + ['label' => 'Gone', 'targetType' => 'view', 'targetId' => 99], + ])); + + $this->assertSame([99], array_map(static fn (MenuItem $item) => $item->getTargetId(), $inserted)); + } + + public function testDerivedTechnicalNamesGetASuffixWhenTheMenuAlreadyUsesThem(): void { + $this->contextMapper->method('findById')->willReturn($this->contextWithMenu(7, [])); + $this->contextMapper->method('update')->willReturnArgument(0); + $inserted = []; + $this->expectMenuReplacement(7, $inserted); + + $this->service->update(7, 'user-1', null, null, null, null, MenuItemInput::createListFromInputArray([ + ['label' => 'Docs', 'url' => 'https://example.org/a'], + ['label' => 'Docs', 'url' => 'https://example.org/b'], + ['label' => 'Other', 'url' => 'https://example.org/c', 'technicalName' => 'docs_2'], + ])); + + $this->assertSame(['docs', 'docs_3', 'docs_2'], array_map(static fn (MenuItem $item) => $item->getTechnicalName(), $inserted)); + } + + public function testTwoMenuItemsWithTheSameTechnicalNameAreRefused(): void { + $this->contextMapper->method('findById')->willReturn($this->contextWithMenu(7, [])); + $this->contextMapper->method('update')->willReturnArgument(0); + + $this->expectException(InvalidArgumentException::class); + $this->expectExceptionMessage('share the technical name "home"'); + $this->service->update(7, 'user-1', null, null, null, null, MenuItemInput::createListFromInputArray([ + ['label' => 'A', 'url' => 'https://example.org/a', 'technicalName' => 'home'], + ['label' => 'B', 'url' => 'https://example.org/b', 'technicalName' => 'home'], + ])); + } + + public function testUpdateLeavesTheMenuAloneWhenNoneIsGiven(): void { + $context = $this->contextWithMenu(7, [['id' => 1, 'label' => 'Keep me']]); + $this->contextMapper->method('findById')->willReturn($context); + $this->contextMapper->method('update')->willReturnArgument(0); + $this->menuItemMapper->expects($this->never())->method('deleteAllByContextId'); + + $updated = $this->service->update(7, 'user-1', 'Renamed', null, null, null); + + $this->assertSame('Renamed', $updated->getName()); + $this->assertSame('Keep me', $updated->getMenuItems()[0]['label']); + } + + public function testCreateStoresTheMenuItems(): void { + $this->contextMapper->method('insert')->willReturnCallback(static function (Context $context): Context { + $context->setId(8); + return $context; + }); + $this->pageMapper->method('insert')->willReturnArgument(0); + $inserted = []; + $this->expectMenuReplacement(8, $inserted); + + $created = $this->service->create('Intake portal', 'heart', ' desc ', [], 'user-1', 0, MenuItemInput::createListFromInputArray([ + ['label' => 'Docs', 'targetType' => 'url', 'url' => 'https://example.org'], + ])); + + $this->assertSame('desc', $created->getDescription()); + $this->assertCount(1, $created->getMenuItems()); + $this->assertSame('docs', $inserted[0]->getTechnicalName()); + } + public function testDeleteRemovesTheMenuOfTheApplication(): void { $context = new Context(); $context->id = 7; From c304fd4c2166051f1970d65f65cc9243305060c8 Mon Sep 17 00:00:00 2001 From: Remko Date: Tue, 13 Oct 2026 11:14:29 +0200 Subject: [PATCH 2/3] fix(contexts): answer unusable menu targets alike and keep stored technical names - a new menu target that does not exist gets the same refusal as one the owner cannot manage, so a save reveals nothing about others' ids; a target that was already in the menu is kept and marked broken - an item sent without a technical name keeps the name stored for the same target, so renaming or reordering no longer changes it - removing a menu item keeps its view or table as a resource, as documented - the start page mock gets an id, which PHP 8.5 needs for the array key - a javascript link with a host is tested, which only the scheme check stops - the menu list is mapped with a first-class callable Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: Remko --- lib/Controller/ContextController.php | 2 +- lib/Model/MenuItemInput.php | 2 +- lib/Service/ContextService.php | 104 ++++++++++++++++++---- openapi.json | 2 +- src/types/openapi/openapi.ts | 2 +- tests/unit/Model/MenuItemInputTest.php | 2 + tests/unit/Service/ContextServiceTest.php | 53 +++++++++-- 7 files changed, 138 insertions(+), 29 deletions(-) diff --git a/lib/Controller/ContextController.php b/lib/Controller/ContextController.php index 858c1717bb..ac236b95ae 100644 --- a/lib/Controller/ContextController.php +++ b/lib/Controller/ContextController.php @@ -141,7 +141,7 @@ public function create(string $name, string $iconName, string $description = '', * @param ?string $iconName provide this parameter to set a new icon * @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 $menuItems provide this parameter to replace the menu; the views and tables it opens become nodes of the context + * @psalm-param ?list $menuItems provide this parameter to replace the menu; 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 * * @return DataResponse|DataResponse * diff --git a/lib/Model/MenuItemInput.php b/lib/Model/MenuItemInput.php index 5f58aba545..e8e80906f2 100644 --- a/lib/Model/MenuItemInput.php +++ b/lib/Model/MenuItemInput.php @@ -93,7 +93,7 @@ public static function createListFromInputArray(mixed $data): array { if (!is_array($data) || !array_is_list($data)) { throw new InvalidArgumentException('The menu items must be a list.'); } - return array_map(static fn (mixed $item): self => self::createFromInputArray($item), $data); + return array_map(self::createFromInputArray(...), $data); } /** diff --git a/lib/Service/ContextService.php b/lib/Service/ContextService.php index bd0ecfca6f..6fe5553c19 100644 --- a/lib/Service/ContextService.php +++ b/lib/Service/ContextService.php @@ -161,16 +161,19 @@ public function create(string $name, string $iconName, string $description, arra /** * Replaces the menu of an application and adds the views and tables it points at - * as nodes, so everyone the application is shared with can open them. + * as nodes, so everyone the application is shared with can open them. Removing an + * item leaves its view or table in the application: it stays a resource until it is + * removed as one. * * @param list $menuItems * @throws Exception|PermissionError|InvalidArgumentException */ protected function replaceMenu(Context $context, array $menuItems, ?string $userId): void { - $this->addMenuTargetsAsNodes($context, $menuItems, $userId); + $storedMenu = array_values(array_filter($context->getMenuItems() ?? [], is_array(...))); + $this->addMenuTargetsAsNodes($context, $menuItems, $storedMenu, $userId); $this->menuItemMapper->deleteAllByContextId($context->getId()); - $technicalNames = $this->menuTechnicalNames($menuItems); + $technicalNames = $this->menuTechnicalNames($menuItems, $storedMenu); foreach ($menuItems as $position => $item) { $menuItem = new MenuItem(); $menuItem->setContextId($context->getId()); @@ -191,14 +194,16 @@ protected function replaceMenu(Context $context, array $menuItems, ?string $user } /** - * The technical name of every menu item, by position: the one given, or one derived from - * the label with a numeric suffix when this menu already uses it. + * The technical name of every menu item, by position: the one given; else the stored name of + * an item with the same target, so a rename or a reorder keeps it; else one derived from the + * label, with a numeric suffix when this menu already uses it. * * @param list $menuItems + * @param list> $storedMenu * @return array * @throws InvalidArgumentException */ - private function menuTechnicalNames(array $menuItems): array { + private function menuTechnicalNames(array $menuItems, array $storedMenu): array { $names = []; $taken = []; foreach ($menuItems as $position => $item) { @@ -212,6 +217,19 @@ private function menuTechnicalNames(array $menuItems): array { $names[$position] = $technicalName; $taken[$technicalName] = true; } + $storedNames = $this->storedTechnicalNamesByTarget($storedMenu); + foreach ($menuItems as $position => $item) { + if (isset($names[$position])) { + continue; + } + foreach ($storedNames[self::menuTargetIdentity($item->getTargetType(), $item->getTargetId(), $item->getUrl())] ?? [] as $storedName) { + if (!isset($taken[$storedName])) { + $names[$position] = $storedName; + $taken[$storedName] = true; + break; + } + } + } foreach ($menuItems as $position => $item) { if (isset($names[$position])) { continue; @@ -228,22 +246,69 @@ private function menuTechnicalNames(array $menuItems): array { } /** - * Targets that no longer exist are left out; the menu marks them broken. + * The technical names of the stored menu, grouped by target and in menu order. + * + * @param list> $storedMenu + * @return array> + */ + private function storedTechnicalNamesByTarget(array $storedMenu): array { + $names = []; + foreach ($storedMenu as $storedItem) { + $targetType = $storedItem['targetType'] ?? null; + $technicalName = $storedItem['technicalName'] ?? null; + if (!is_string($targetType) || !is_string($technicalName) || $technicalName === '') { + continue; + } + $targetId = is_int($storedItem['targetId'] ?? null) ? $storedItem['targetId'] : null; + $url = is_string($storedItem['url'] ?? null) ? $storedItem['url'] : null; + $names[self::menuTargetIdentity($targetType, $targetId, $url)][] = $technicalName; + } + return $names; + } + + private static function menuTargetIdentity(string $targetType, ?int $targetId, ?string $url): string { + return $targetType . ':' . ($targetType === MenuItem::TARGET_URL ? (string)$url : (string)$targetId); + } + + /** + * The node a menu target is stored as, keyed so nodes and targets can be compared. + * + * @return array{string, array{int, int}}|null + */ + private static function menuTargetNode(mixed $targetType, mixed $targetId): ?array { + $nodeType = match ($targetType) { + MenuItem::TARGET_VIEW => Application::NODE_TYPE_VIEW, + MenuItem::TARGET_TABLE => Application::NODE_TYPE_TABLE, + default => null, + }; + if ($nodeType === null || !is_int($targetId)) { + return null; + } + return [sprintf('t%di%d', $nodeType, $targetId), [$nodeType, $targetId]]; + } + + /** + * A new target must exist and be manageable by the user, and every unusable one gets the + * same answer, so the save reveals nothing about others' views and tables. A target that was + * in the stored menu before and no longer exists is kept; the menu marks it broken. * * @param list $menuItems + * @param list> $storedMenu * @throws Exception|PermissionError */ - private function addMenuTargetsAsNodes(Context $context, array $menuItems, ?string $userId): void { + private function addMenuTargetsAsNodes(Context $context, array $menuItems, array $storedMenu, ?string $userId): void { $missing = []; foreach ($menuItems as $item) { - $nodeType = match ($item->getTargetType()) { - MenuItem::TARGET_VIEW => Application::NODE_TYPE_VIEW, - MenuItem::TARGET_TABLE => Application::NODE_TYPE_TABLE, - default => null, - }; - $nodeId = $item->getTargetId(); - if ($nodeType !== null && $nodeId !== null) { - $missing[sprintf('t%di%d', $nodeType, $nodeId)] = [$nodeType, $nodeId]; + $target = self::menuTargetNode($item->getTargetType(), $item->getTargetId()); + if ($target !== null) { + $missing[$target[0]] = $target[1]; + } + } + $storedTargets = []; + foreach ($storedMenu as $storedItem) { + $target = self::menuTargetNode($storedItem['targetType'] ?? null, $storedItem['targetId'] ?? null); + if ($target !== null) { + $storedTargets[$target[0]] = true; } } $nodes = $context->getNodes() ?? []; @@ -272,11 +337,12 @@ private function addMenuTargetsAsNodes(Context $context, array $menuItems, ?stri } } - foreach ($missing as [$nodeType, $nodeId]) { - if (!isset($existing[$nodeType][$nodeId])) { + foreach ($missing as $key => [$nodeType, $nodeId]) { + $exists = isset($existing[$nodeType][$nodeId]); + if (!$exists && isset($storedTargets[$key])) { continue; } - if (!$this->permissionsService->canManageNodeById($nodeType, $nodeId, $userId)) { + if (!$exists || !$this->permissionsService->canManageNodeById($nodeType, $nodeId, $userId)) { throw new PermissionError(sprintf('Owner cannot manage node %d (type %d)', $nodeId, $nodeType)); } $relation = $this->addNodeToContext($context, $nodeId, $nodeType, Application::PERMISSION_READ); diff --git a/openapi.json b/openapi.json index fb790ada33..dbf564a889 100644 --- a/openapi.json +++ b/openapi.json @@ -12488,7 +12488,7 @@ "type": "array", "nullable": true, "default": null, - "description": "provide this parameter to replace the menu; the views and tables it opens become nodes of the context", + "description": "provide this parameter to replace the menu; 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", "items": { "type": "object", "required": [ diff --git a/src/types/openapi/openapi.ts b/src/types/openapi/openapi.ts index e7b097f964..992cc9151b 100644 --- a/src/types/openapi/openapi.ts +++ b/src/types/openapi/openapi.ts @@ -7102,7 +7102,7 @@ export interface operations { readonly order: number; } | null; /** - * @description provide this parameter to replace the menu; the views and tables it opens become nodes of the context + * @description provide this parameter to replace the menu; 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 * @default null */ readonly menuItems?: readonly { diff --git a/tests/unit/Model/MenuItemInputTest.php b/tests/unit/Model/MenuItemInputTest.php index 49c9e1cd0f..fe7a667aac 100644 --- a/tests/unit/Model/MenuItemInputTest.php +++ b/tests/unit/Model/MenuItemInputTest.php @@ -44,6 +44,8 @@ public static function invalidItems(): array { 'id of zero' => [['label' => 'X', 'targetType' => 'table', 'targetId' => 0], 'needs its id'], 'javascript link' => [['label' => 'X', 'url' => 'javascript:alert(1)'], 'http or https'], 'data link' => [['label' => 'X', 'url' => 'data:text/html,hi'], 'http or https'], + // carries a host, so only the scheme check stops it + 'javascript link with a host' => [['label' => 'X', 'url' => 'javascript://example.com/%0Aalert(1)'], 'http or https'], 'link without host' => [['label' => 'X', 'url' => 'https://'], 'http or https'], 'empty link' => [['label' => 'X', 'url' => ''], 'http or https'], 'technical name with a hyphen' => [['label' => 'X', 'url' => 'https://example.org', 'technicalName' => 'my-page'], 'technical name'], diff --git a/tests/unit/Service/ContextServiceTest.php b/tests/unit/Service/ContextServiceTest.php index d10d323792..e17ea1b14d 100644 --- a/tests/unit/Service/ContextServiceTest.php +++ b/tests/unit/Service/ContextServiceTest.php @@ -181,21 +181,36 @@ public function testMenuTargetsBecomeNodesSoRecipientsCanOpenThem(): void { $this->assertSame(200, $updated->getPages()[301]['content'][501]['node_rel_id']); } - public function testAMenuTargetTheOwnerCannotManageIsRefused(): void { + public static function unusableNewTargets(): array { + return [ + 'exists but cannot be managed' => [[4 => new Table()]], + 'does not exist' => [[]], + ]; + } + + /** + * Both get the same answer, so a save reveals nothing about which ids exist. + * + * @dataProvider unusableNewTargets + */ + public function testANewMenuTargetThatCannotBeUsedIsRefusedTheSameWay(array $tables): void { $this->contextMapper->method('findById')->willReturn($this->contextWithMenu(7, [])); $this->contextMapper->method('update')->willReturnArgument(0); - $this->expectExistingTargets([], [4 => new Table()]); - $this->permissionsService->method('canManageNodeById')->with(Application::NODE_TYPE_TABLE, 4, 'user-1')->willReturn(false); + $this->expectExistingTargets([], $tables); + $this->permissionsService->method('canManageNodeById')->willReturn(false); $this->menuItemMapper->expects($this->never())->method('deleteAllByContextId'); $this->expectException(PermissionError::class); + $this->expectExceptionMessage('Owner cannot manage node 4 (type 0)'); $this->service->update(7, 'user-1', null, null, null, null, MenuItemInput::createListFromInputArray([ ['label' => 'Requests', 'targetType' => 'table', 'targetId' => 4], ])); } - public function testAMenuTargetThatNoLongerExistsIsKeptWithoutANode(): void { - $this->contextMapper->method('findById')->willReturn($this->contextWithMenu(7, [])); + public function testAStoredMenuTargetThatNoLongerExistsIsKeptWithoutANode(): void { + $this->contextMapper->method('findById')->willReturn($this->contextWithMenu(7, [ + ['targetType' => 'view', 'targetId' => 99, 'url' => null, 'technicalName' => 'gone'], + ])); $this->contextMapper->method('update')->willReturnArgument(0); $this->expectExistingTargets([], []); $this->contextNodeRelMapper->expects($this->never())->method('insert'); @@ -209,6 +224,29 @@ public function testAMenuTargetThatNoLongerExistsIsKeptWithoutANode(): void { $this->assertSame([99], array_map(static fn (MenuItem $item) => $item->getTargetId(), $inserted)); } + public function testAnItemSentWithoutANameKeepsTheStoredNameOfItsTarget(): void { + $context = $this->contextWithMenu(7, [ + ['targetType' => 'view', 'targetId' => 6, 'url' => null, 'technicalName' => 'home'], + ['targetType' => 'url', 'targetId' => null, 'url' => 'https://example.org/a', 'technicalName' => 'docs'], + ['targetType' => 'url', 'targetId' => null, 'url' => 'https://example.org/b', 'technicalName' => 'docs_2'], + ]); + $context->setNodes([101 => ['id' => 101, 'node_id' => 6, 'node_type' => Application::NODE_TYPE_VIEW, 'permissions' => 1]]); + $this->contextMapper->method('findById')->willReturn($context); + $this->contextMapper->method('update')->willReturnArgument(0); + $inserted = []; + $this->expectMenuReplacement(7, $inserted); + + // the view is renamed, the two links swap places and a new link is added + $this->service->update(7, 'user-1', null, null, null, null, MenuItemInput::createListFromInputArray([ + ['label' => 'Start', 'targetType' => 'view', 'targetId' => 6], + ['label' => 'Docs', 'url' => 'https://example.org/b'], + ['label' => 'Docs', 'url' => 'https://example.org/a'], + ['label' => 'Docs', 'url' => 'https://example.org/c'], + ])); + + $this->assertSame(['home', 'docs_2', 'docs', 'docs_3'], array_map(static fn (MenuItem $item) => $item->getTechnicalName(), $inserted)); + } + public function testDerivedTechnicalNamesGetASuffixWhenTheMenuAlreadyUsesThem(): void { $this->contextMapper->method('findById')->willReturn($this->contextWithMenu(7, [])); $this->contextMapper->method('update')->willReturnArgument(0); @@ -253,7 +291,10 @@ public function testCreateStoresTheMenuItems(): void { $context->setId(8); return $context; }); - $this->pageMapper->method('insert')->willReturnArgument(0); + $this->pageMapper->method('insert')->willReturnCallback(static function (Page $page): Page { + $page->setId(3); + return $page; + }); $inserted = []; $this->expectMenuReplacement(8, $inserted); From 0299c48d21bd50c5a536b5749f7ba0104402b362 Mon Sep 17 00:00:00 2001 From: Remko Date: Tue, 13 Oct 2026 11:48:26 +0200 Subject: [PATCH 3/3] fix(contexts): retry an application update that loses its write lock The update runs in one transaction since the menu is saved with it. On SQLite a concurrent request can take the write lock between its read and its write, and the update then failed at once with a 500. It now runs again on a retryable database error, as atomicRetry() does in core. Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: Remko --- lib/Service/ContextService.php | 4 +++- tests/unit/Service/ContextServiceTest.php | 13 +++++++++++++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/lib/Service/ContextService.php b/lib/Service/ContextService.php index 6fe5553c19..8ebb595630 100644 --- a/lib/Service/ContextService.php +++ b/lib/Service/ContextService.php @@ -378,7 +378,9 @@ protected function technicalNameFromLabel(string $label): string { * @throws PermissionError|MultipleObjectsReturnedException|InvalidArgumentException */ public function update(int $contextId, string $userId, ?string $name, ?string $iconName, ?string $description, ?array $nodes, ?array $menuItems = null): Context { - return $this->atomic( + // A transaction that reads before it writes can lose a write lock to a concurrent request + // (SQLite answers "database is locked" at once); running it again then succeeds + return $this->atomicRetry( fn (): Context => $this->applyUpdate($contextId, $userId, $name, $iconName, $description, $nodes, $menuItems), $this->dbc, ); diff --git a/tests/unit/Service/ContextServiceTest.php b/tests/unit/Service/ContextServiceTest.php index e17ea1b14d..520ac51f7d 100644 --- a/tests/unit/Service/ContextServiceTest.php +++ b/tests/unit/Service/ContextServiceTest.php @@ -10,6 +10,7 @@ namespace OCA\Tables\Service; use InvalidArgumentException; +use OC\DB\Exceptions\DbalException; use OCA\Tables\AppInfo\Application; use OCA\Tables\Db\Context; use OCA\Tables\Db\ContextMapper; @@ -274,6 +275,18 @@ public function testTwoMenuItemsWithTheSameTechnicalNameAreRefused(): void { ])); } + public function testUpdateRunsAgainWhenTheDatabaseAsksForARetry(): void { + $locked = $this->createMock(DbalException::class); + $locked->method('isRetryable')->willReturn(true); + $this->contextMapper->expects($this->exactly(2))->method('findById') + ->willReturnOnConsecutiveCalls($this->throwException($locked), $this->contextWithMenu(7, [])); + $this->contextMapper->method('update')->willReturnArgument(0); + + $updated = $this->service->update(7, 'user-1', 'Renamed', null, null, null); + + $this->assertSame('Renamed', $updated->getName()); + } + public function testUpdateLeavesTheMenuAloneWhenNoneIsGiven(): void { $context = $this->contextWithMenu(7, [['id' => 1, 'label' => 'Keep me']]); $this->contextMapper->method('findById')->willReturn($context);