From 4097a87051d541fc2cbe0d00f2dd223f0430d503 Mon Sep 17 00:00:00 2001 From: Khairul Syazwan Date: Wed, 26 Aug 2026 10:39:55 +0800 Subject: [PATCH 1/5] fix(d2e-webapi): stop the duplicate-name check needing a WebAPI read permission A researcher-only user could not save a concept set. The save runs a duplicate-name pre-check first, and that check asked WebAPI through GET /conceptset/{id}/exists, guarded by isAnyPermitted(anyOf('read:conceptset','write:conceptset')) with no isOwner fallback. The 'concept set creator' role holds neither permission, so the check returned 403 and the save stopped before the create was ever attempted. The create itself was always permitted, because it is guarded by isPermitted('create:conceptset'). Atlas3 never asks this question and works for the same user. The facade merges two stores in that check. Only the WebAPI half fails, and the WebAPI store already enforces the same rule: webapi.concept_set carries the uq_cs_name unique constraint, which rejects a duplicate on write and reports it as HTTP 409. Keep the legacy half, which needs no permission, and let the constraint answer for the WebAPI store. Map the 409 on create and on update to a typed error, so that a duplicate name reaches the browser as a 409 with a readable message rather than a 500. Remove the now-unreachable WebAPI probe. No permission grant is required for a researcher to save a concept set. --- .../d2e-webapi/src/api/WebApiConceptSetAPI.ts | 43 ++----- .../d2e-webapi/src/dto/conceptset.ts | 9 ++ .../d2e-webapi/src/errors/ConceptSetErrors.ts | 14 ++ .../d2e-webapi/src/routes/conceptset.ts | 74 ++++++++--- .../src/services/conceptset.service.test.ts | 121 ++++++++++++++---- .../src/services/conceptset.service.ts | 28 ++-- 6 files changed, 197 insertions(+), 92 deletions(-) diff --git a/plugins/functions/d2e-webapi/src/api/WebApiConceptSetAPI.ts b/plugins/functions/d2e-webapi/src/api/WebApiConceptSetAPI.ts index ba82b1c10b..c4ab43fceb 100644 --- a/plugins/functions/d2e-webapi/src/api/WebApiConceptSetAPI.ts +++ b/plugins/functions/d2e-webapi/src/api/WebApiConceptSetAPI.ts @@ -1,3 +1,5 @@ +import { ConceptSetNameConflictError } from "../errors/ConceptSetErrors.ts"; + const DEFAULT_WEBAPI_URL = "http://localhost:33001/WebAPI"; export interface IWebApiConceptSetHeader { @@ -52,16 +54,6 @@ export interface IWebApiConcept { const CONTROL_CHAR_REGEX = /[\x00-\x1F\x7F]/; -const assertNonNegativeInteger = (value: unknown, field: string): number => { - if ( - typeof value !== "number" || !Number.isInteger(value) || value < 0 || - value > Number.MAX_SAFE_INTEGER - ) { - throw new Error(`Invalid ${field}: expected non-negative integer`); - } - return value; -}; - const assertPositiveInteger = (value: unknown, field: string): number => { if ( typeof value !== "number" || !Number.isInteger(value) || value <= 0 || @@ -224,6 +216,11 @@ export class WebApiConceptSetAPI { }); if (!response.ok) { + // WebAPI has no duplicate-name check. The `uq_cs_name` constraint on + // `webapi.concept_set` rejects the insert and surfaces as a 409. + if (response.status === 409) { + throw new ConceptSetNameConflictError(payload.name); + } throw new Error( `Failed to create WebAPI concept set: ${response.status}`, ); @@ -259,6 +256,11 @@ export class WebApiConceptSetAPI { ); if (!response.ok) { + // A rename onto a name another concept set already holds trips the same + // `uq_cs_name` constraint as a create. + if (response.status === 409) { + throw new ConceptSetNameConflictError(payload.name); + } throw new Error( `Failed to update WebAPI concept set ${validatedId}: ${response.status}`, ); @@ -318,25 +320,4 @@ export class WebApiConceptSetAPI { ); } } - - async checkIfConceptSetExists(id: number, name: string): Promise { - const validatedId = assertNonNegativeInteger(id, "id"); - const validatedName = assertName(name); - - const url = buildUrl(this.baseUrl, "conceptset", validatedId, "exists"); - url.searchParams.set("name", validatedName); - - const response = await fetch(url, { - method: "GET", - headers: buildHeaders(this.token), - }); - - if (!response.ok) { - throw new Error( - `Failed to check WebAPI concept set existence for ${validatedId}: ${response.status}`, - ); - } - - return response.json(); - } } diff --git a/plugins/functions/d2e-webapi/src/dto/conceptset.ts b/plugins/functions/d2e-webapi/src/dto/conceptset.ts index 8fee5fcf11..52da5c9a25 100644 --- a/plugins/functions/d2e-webapi/src/dto/conceptset.ts +++ b/plugins/functions/d2e-webapi/src/dto/conceptset.ts @@ -157,6 +157,15 @@ export const ConceptSetInUseErrorDto = z.object({ }); export type IConceptSetInUseErrorDto = z.infer; +export const ConceptSetNameConflictErrorDto = z.object({ + error: z.literal("CONCEPT_SET_NAME_EXISTS"), + message: z.string(), + conceptSetName: z.string(), +}); +export type IConceptSetNameConflictErrorDto = z.infer< + typeof ConceptSetNameConflictErrorDto +>; + export const IncludedConceptDto = z.object({ CONCEPT_ID: z.number(), CONCEPT_NAME: z.string(), diff --git a/plugins/functions/d2e-webapi/src/errors/ConceptSetErrors.ts b/plugins/functions/d2e-webapi/src/errors/ConceptSetErrors.ts index b4091ce206..ac86eb5d3f 100644 --- a/plugins/functions/d2e-webapi/src/errors/ConceptSetErrors.ts +++ b/plugins/functions/d2e-webapi/src/errors/ConceptSetErrors.ts @@ -18,6 +18,20 @@ export class ConceptSetInUseError extends Error { } } +/** + * Thrown when WebAPI rejects a concept set name that is already taken. + * WebAPI has no duplicate-name check of its own; the `uq_cs_name` unique + * constraint on `webapi.concept_set` rejects the write and WebAPI reports it + * as HTTP 409. Route handlers map this error back to a 409 so that the browser + * can show a duplicate-name message instead of a generic failure. + */ +export class ConceptSetNameConflictError extends Error { + constructor(public readonly conceptSetName: string) { + super(`A concept set named "${conceptSetName}" already exists`); + this.name = "ConceptSetNameConflictError"; + } +} + /** * Thrown when concept set validation fails (e.g., invalid ID format). */ diff --git a/plugins/functions/d2e-webapi/src/routes/conceptset.ts b/plugins/functions/d2e-webapi/src/routes/conceptset.ts index 23db4d9b20..95ce2a3a97 100644 --- a/plugins/functions/d2e-webapi/src/routes/conceptset.ts +++ b/plugins/functions/d2e-webapi/src/routes/conceptset.ts @@ -9,11 +9,15 @@ import { ConceptSetItemsResponseDto, ConceptSetCreateDto, ConceptSetInUseErrorDto, + ConceptSetNameConflictErrorDto, IConceptSetCheckResponseDto, IncludedConceptsRequestDto, IncludedConceptsResponseDto, } from "../dto/conceptset.ts"; -import { ConceptSetInUseError } from "../errors/ConceptSetErrors.ts"; +import { + ConceptSetInUseError, + ConceptSetNameConflictError, +} from "../errors/ConceptSetErrors.ts"; import { getConceptSet, @@ -55,10 +59,14 @@ export const conceptset: FastifyPluginAsyncZod = async function (app) { "/", { schema: { - description: "Save a new concept set to the database", + description: + "Save a new concept set to the database. Returns 409 if the name is already taken.", body: ConceptSetCreateDto, tags: ["conceptset"], - response: { 200: ConceptSetResponseDto }, + response: { + 200: ConceptSetResponseDto, + 409: ConceptSetNameConflictErrorDto, + }, security: [ { bearerAuth: [], @@ -68,12 +76,25 @@ export const conceptset: FastifyPluginAsyncZod = async function (app) { }, }, async (req, res) => { - const results = await createConceptSet( - req.token, - req.datasetId, - req.body - ); - res.send(results); + try { + const results = await createConceptSet( + req.token, + req.datasetId, + req.body + ); + res.send(results); + } catch (error) { + if (error instanceof ConceptSetNameConflictError) { + res.status(409).send({ + error: "CONCEPT_SET_NAME_EXISTS", + message: + `A concept set named "${error.conceptSetName}" already exists. Choose another name.`, + conceptSetName: error.conceptSetName, + }); + return; + } + throw error; + } } ); @@ -155,11 +176,15 @@ export const conceptset: FastifyPluginAsyncZod = async function (app) { "/:id", { schema: { - description: "Updates the concept set for the selected concept set.", + description: + "Updates the concept set for the selected concept set. Returns 409 if the name is already taken.", tags: ["conceptset"], params: z.object({ id: ConceptSetIdParamSchema }), body: ConceptSetCreateDto, - response: { 200: z.boolean() }, + response: { + 200: z.boolean(), + 409: ConceptSetNameConflictErrorDto, + }, security: [ { bearerAuth: [], @@ -170,13 +195,26 @@ export const conceptset: FastifyPluginAsyncZod = async function (app) { }, async (req, res) => { const { id } = req.params; - const results = await updateConceptSet( - req.token, - req.datasetId, - id, - req.body - ); - res.send(results); + try { + const results = await updateConceptSet( + req.token, + req.datasetId, + id, + req.body + ); + res.send(results); + } catch (error) { + if (error instanceof ConceptSetNameConflictError) { + res.status(409).send({ + error: "CONCEPT_SET_NAME_EXISTS", + message: + `A concept set named "${error.conceptSetName}" already exists. Choose another name.`, + conceptSetName: error.conceptSetName, + }); + return; + } + throw error; + } } ); diff --git a/plugins/functions/d2e-webapi/src/services/conceptset.service.test.ts b/plugins/functions/d2e-webapi/src/services/conceptset.service.test.ts index cf3d13ff48..fde73270db 100644 --- a/plugins/functions/d2e-webapi/src/services/conceptset.service.test.ts +++ b/plugins/functions/d2e-webapi/src/services/conceptset.service.test.ts @@ -35,7 +35,7 @@ const { mapWebApiConceptSetToFacadeConceptSet, } = await import("./conceptset.service.ts"); -const { ConceptSetExpressionError } = await import( +const { ConceptSetExpressionError, ConceptSetNameConflictError } = await import( "../errors/ConceptSetErrors.ts" ); const { WebApiConceptSetAPI } = await import("../api/WebApiConceptSetAPI.ts"); @@ -846,56 +846,121 @@ Deno.test("getConceptSets propagates WebAPI errors instead of returning silent e } }); -Deno.test("checkIfConceptSetExists allows webapi exclude id 0 for new concept sets", async () => { +Deno.test("checkIfConceptSetExists checks the legacy store only", async () => { + // WebAPI is deliberately not asked. Its `uq_cs_name` constraint rejects a + // duplicate at write time, and asking it here needs a permission that the + // `concept set creator` role does not hold. const originalGetConceptSetsTerm = TerminologySvcAPI.prototype.getConceptSets; - const originalCheckIfConceptSetExists = - WebApiConceptSetAPI.prototype.checkIfConceptSetExists; try { - TerminologySvcAPI.prototype.getConceptSets = () => - Promise.resolve([] as unknown as ITerminologyConceptSet[]); - WebApiConceptSetAPI.prototype.checkIfConceptSetExists = ( - id: number, - name: string, - ) => { - assertEquals(id, 0); - assertEquals(name, "Name"); - return Promise.resolve(0); + let legacyCalls = 0; + TerminologySvcAPI.prototype.getConceptSets = () => { + legacyCalls += 1; + return Promise.resolve([] as unknown as ITerminologyConceptSet[]); }; const result = await checkIfConceptSetExists( "token", "dataset-1", - 0, + "webapi:7", "Name", ); + assertEquals(result, 0); + assertEquals(legacyCalls, 1); } finally { TerminologySvcAPI.prototype.getConceptSets = originalGetConceptSetsTerm; - WebApiConceptSetAPI.prototype.checkIfConceptSetExists = - originalCheckIfConceptSetExists; } }); -Deno.test("checkIfConceptSetExists propagates WebAPI errors instead of returning silent zero", async () => { +Deno.test("checkIfConceptSetExists reports a legacy concept set with the same name", async () => { const originalGetConceptSetsTerm = TerminologySvcAPI.prototype.getConceptSets; - const originalCheckIfConceptSetExists = - WebApiConceptSetAPI.prototype.checkIfConceptSetExists; try { TerminologySvcAPI.prototype.getConceptSets = () => - Promise.resolve([] as unknown as ITerminologyConceptSet[]); - WebApiConceptSetAPI.prototype.checkIfConceptSetExists = () => - Promise.reject(new Error("WebAPI unavailable")); + Promise.resolve( + [{ id: 3, name: "Name" }] as unknown as ITerminologyConceptSet[], + ); - await assertRejects( - () => checkIfConceptSetExists("token", "dataset-1", "webapi:7", "Name"), - Error, - "WebAPI unavailable", + assertEquals( + await checkIfConceptSetExists("token", "dataset-1", "webapi:7", "Name"), + 1, ); } finally { TerminologySvcAPI.prototype.getConceptSets = originalGetConceptSetsTerm; - WebApiConceptSetAPI.prototype.checkIfConceptSetExists = - originalCheckIfConceptSetExists; + } +}); + +Deno.test("checkIfConceptSetExists excludes the legacy row being renamed", async () => { + const originalGetConceptSetsTerm = TerminologySvcAPI.prototype.getConceptSets; + + try { + TerminologySvcAPI.prototype.getConceptSets = () => + Promise.resolve( + [{ id: 3, name: "Name" }] as unknown as ITerminologyConceptSet[], + ); + + // Renaming legacy:3 to the name it already holds is not a duplicate. + assertEquals( + await checkIfConceptSetExists("token", "dataset-1", "legacy:3", "Name"), + 0, + ); + } finally { + TerminologySvcAPI.prototype.getConceptSets = originalGetConceptSetsTerm; + } +}); + +Deno.test("WebApiConceptSetAPI.createConceptSet maps a 409 to a name conflict", async () => { + const originalFetch = globalThis.fetch; + + try { + globalThis.fetch = (() => + Promise.resolve(new Response(null, { status: 409 }))) as typeof fetch; + + const api = new WebApiConceptSetAPI("token"); + const error = await assertRejects( + () => api.createConceptSet({ name: "Taken" }), + ConceptSetNameConflictError, + ); + assertEquals(error.conceptSetName, "Taken"); + } finally { + globalThis.fetch = originalFetch; + } +}); + +Deno.test("WebApiConceptSetAPI.updateConceptSet maps a 409 to a name conflict", async () => { + const originalFetch = globalThis.fetch; + + try { + globalThis.fetch = (() => + Promise.resolve(new Response(null, { status: 409 }))) as typeof fetch; + + const api = new WebApiConceptSetAPI("token"); + const error = await assertRejects( + () => api.updateConceptSet(7, { id: 7, name: "Taken" }), + ConceptSetNameConflictError, + ); + assertEquals(error.conceptSetName, "Taken"); + } finally { + globalThis.fetch = originalFetch; + } +}); + +Deno.test("WebApiConceptSetAPI.createConceptSet keeps other failures as plain errors", async () => { + const originalFetch = globalThis.fetch; + + try { + globalThis.fetch = (() => + Promise.resolve(new Response(null, { status: 500 }))) as typeof fetch; + + const api = new WebApiConceptSetAPI("token"); + const error = await assertRejects( + () => api.createConceptSet({ name: "Any" }), + Error, + "Failed to create WebAPI concept set: 500", + ); + assertEquals(error instanceof ConceptSetNameConflictError, false); + } finally { + globalThis.fetch = originalFetch; } }); diff --git a/plugins/functions/d2e-webapi/src/services/conceptset.service.ts b/plugins/functions/d2e-webapi/src/services/conceptset.service.ts index 13b68fa510..7ad238b1eb 100644 --- a/plugins/functions/d2e-webapi/src/services/conceptset.service.ts +++ b/plugins/functions/d2e-webapi/src/services/conceptset.service.ts @@ -437,21 +437,19 @@ export const checkIfConceptSetExists = async ( ): Promise => { const ref = parseConceptSetRef(conceptSetId); const terminologySvcApi = new TerminologySvcAPI(token); - const webApiConceptSetApi = new WebApiConceptSetAPI(token); - - // Probe WebAPI with the source-scoped externalId so that an in-flight - // rename of the same row doesn't false-positive against itself. For - // legacy refs there is no WebAPI counterpart to exclude, so use 0 (a - // never-existing id) which still surfaces unrelated WebAPI duplicates. - const webApiExcludeId = ref.source === "webapi" ? ref.externalId : 0; - const [terminologyConceptSets, webApiExistsCount] = await Promise.all([ - terminologySvcApi.getConceptSets(datasetId), - webApiConceptSetApi.checkIfConceptSetExists( - webApiExcludeId, - conceptSetName, - ), - ]); + // Only the legacy store is probed here. The WebAPI store enforces name + // uniqueness with the `uq_cs_name` constraint and reports a duplicate as + // HTTP 409 on create and on update, which the routes map to a typed error. + // + // Asking WebAPI the same question needs `read:conceptset` or + // `write:conceptset`. The `concept set creator` role holds neither, so a + // researcher-only user was denied here and could never save a concept set, + // even though the create itself was permitted. Atlas3 does not ask this + // question at all. + const terminologyConceptSets = await terminologySvcApi.getConceptSets( + datasetId, + ); // For legacy refs we must exclude the same legacy row by id; for webapi // refs the legacy table is a disjoint namespace, so no row should match @@ -464,7 +462,7 @@ export const checkIfConceptSetExists = async ( : terminologyConceptSet.name === conceptSetName ); - return (result === undefined ? 0 : 1) + webApiExistsCount; + return result === undefined ? 0 : 1; }; const parseDateValue = (value: string | number): number => { From 620b9f5e8eb3a4fa79769fce990b9be9639c83d7 Mon Sep 17 00:00:00 2001 From: Khairul Syazwan Date: Wed, 26 Aug 2026 10:39:57 +0800 Subject: [PATCH 2/5] fix(concept-sets): show the duplicate-name message when the save returns 409 The save flow discarded the error and always rendered the generic 'Error creating/updating' text. A duplicate name in the WebAPI store is now reported as a 409 by the facade, so read it and show the same message the pre-check already shows for the legacy store. Every other failure keeps the generic text. --- .../src/Terminology/Terminology.tsx | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/plugins/ui/apps/concept-sets/src/Terminology/Terminology.tsx b/plugins/ui/apps/concept-sets/src/Terminology/Terminology.tsx index 9aca8ee3e6..0000075afe 100644 --- a/plugins/ui/apps/concept-sets/src/Terminology/Terminology.tsx +++ b/plugins/ui/apps/concept-sets/src/Terminology/Terminology.tsx @@ -544,8 +544,11 @@ export const Terminology: FC = ({ try { // When creating a new concept set there is no id yet. Use "0" (a // never-existing id) as the exclusion sentinel: the backend route param - // schema rejects an empty segment ("/conceptset//exists" -> 400), and - // "0" still surfaces same-name duplicates across both stores. + // schema rejects an empty segment ("/conceptset//exists" -> 400). + // + // This check now covers the legacy store only. A duplicate in the WebAPI + // store is rejected by its `uq_cs_name` constraint at save time and comes + // back as a 409, handled in the catch below. const isNameUsed = await checkIfConceptSetExists( conceptSetId || "0", conceptSet.name, @@ -574,7 +577,17 @@ export const Terminology: FC = ({ setCurrentConceptSet(savedConceptSet); setConceptSetId(updatedConceptSetId); return; - } catch { + } catch (err: any) { + // request() rejects with error.response directly, not the full axios + // error, so the status and body sit at the top level. + if (err?.status === 409 && err?.data?.error === "CONCEPT_SET_NAME_EXISTS") { + setErrorMsg( + getText(i18nKeys.TERMINOLOGY__CONCEPT_SET_NAME_USED_ERROR, [ + `"${conceptSet.name}"`, + ]), + ); + return; + } setErrorMsg( getText(i18nKeys.TERMINOLOGY__ERROR, [ conceptSetId From cbce2af4750707056fe779ba821f9f2104e5e659 Mon Sep 17 00:00:00 2001 From: Khairul Syazwan Date: Wed, 26 Aug 2026 14:28:16 +0800 Subject: [PATCH 3/5] fix(concept-sets): disable Update for concept sets the user does not own The Concept Sets list shows read-only rows (eye icon) for sets the user does not own, but opening one still showed an active Update button. The drawer re-fetches the set by id, and that get-by-id response carries an unreliable hasWriteAccess flag (false for owned sets, and 500 for a non-owned researcher set). The list row is the reliable source: it already marks non-owned sets with an eye. Pass the list row's writability into the drawer and use it as the authoritative ownership signal. Reset the ownership state when a set opens, so a failed get-by-id cannot leave a stale value. Keep the Update button visible but disabled for sets the user does not own, and enabled for owned sets, matching the read-only affordance already shown in the list. --- .../src/ConceptSets/ConceptSets.tsx | 3 +- .../src/ConceptSets/ConceptSetsTable.tsx | 4 +-- .../src/Terminology/Terminology.tsx | 33 ++++++++++--------- .../TerminologyWithEventListener.tsx | 1 + 4 files changed, 23 insertions(+), 18 deletions(-) diff --git a/plugins/ui/apps/concept-sets/src/ConceptSets/ConceptSets.tsx b/plugins/ui/apps/concept-sets/src/ConceptSets/ConceptSets.tsx index 72b4347975..3046262931 100644 --- a/plugins/ui/apps/concept-sets/src/ConceptSets/ConceptSets.tsx +++ b/plugins/ui/apps/concept-sets/src/ConceptSets/ConceptSets.tsx @@ -77,7 +77,7 @@ export const ConceptSets: FC = ({ isAtlas }) => { }, [fetchData]); const handleAddAndEditConceptSet = useCallback( - (conceptSetId?: string) => { + (conceptSetId?: string, canWrite?: boolean) => { if (!datasetId) return; const event = new CustomEvent<{ props: TerminologyProps }>( "alp-terminology-open", @@ -85,6 +85,7 @@ export const ConceptSets: FC = ({ isAtlas }) => { detail: { props: { selectedConceptSetId: conceptSetId, + selectedConceptSetCanWrite: canWrite ?? true, onClose: () => fetchData(), mode: "CONCEPT_SET", selectedDatasetId: datasetId, diff --git a/plugins/ui/apps/concept-sets/src/ConceptSets/ConceptSetsTable.tsx b/plugins/ui/apps/concept-sets/src/ConceptSets/ConceptSetsTable.tsx index bfdbe48fd6..387d01c632 100644 --- a/plugins/ui/apps/concept-sets/src/ConceptSets/ConceptSetsTable.tsx +++ b/plugins/ui/apps/concept-sets/src/ConceptSets/ConceptSetsTable.tsx @@ -21,7 +21,7 @@ import "./ConceptSets.scss"; interface ConceptSetsTableProps { data: ConceptSet[]; isLoading: boolean; - onAddEdit: (conceptSetId?: string) => void; + onAddEdit: (conceptSetId?: string, canWrite?: boolean) => void; onDelete: (conceptSet: ConceptSet) => void; userName?: string; } @@ -124,7 +124,7 @@ export const ConceptSetsTable: FC = ({ ) } - onClick={() => onAddEdit(row.original.id)} + onClick={() => onAddEdit(row.original.id, isWritable)} /> {isWritable && ( void; selectedConceptSetId?: string; + selectedConceptSetCanWrite?: boolean; mode?: | "CONCEPT_MAPPING" | "CONCEPT_SET" @@ -210,18 +211,16 @@ const NameSection = ({ /> )} - {isUserConceptSet && ( -