From 70f22bcaf6dc5024895dca3ad37abcce2f859796 Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Wed, 10 Jun 2026 14:35:26 +0200 Subject: [PATCH] Fix syncServerAttributes clobbering conditional case options for shared param names MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a conditional has multiple cases that all define a parameter with the same name (e.g. `feature` in every `when` branch of `Extract features from GFF`), the previous flat-key approach in `syncServerAttributes` would store only the last case's server node under that key. Every case then received the last case's options, causing `FormSelect.setInitialValue()` to auto-select the first option of the wrong case — e.g. `"0"` from the Frame column — as the value for a different active case. The client then submitted that invalid value to `POST /api/jobs`, which passed lenient Pydantic validation at request time (dynamic selects accept any `StrictStr`) but failed with `RequestParameterInvalidException` when the Celery worker validated the value against the real dataset options. Replace the flat-dict + `visitAllInputs` approach with a structural recursive `syncInputsStructural`/`syncNodeAttributes` pair that matches each conditional case's inputs to the corresponding server case by index. Also align `_buildLevel`'s conditional case selection in `utilities.js` to use the actual value from `formData` rather than the potentially-stale `node.test_param.value` from `formConfig.inputs`. Fixes #22864 --- .../Form/composables/useFormState.test.js | 53 ++++++++++++++++ .../Form/composables/useFormState.ts | 63 ++++++++++++++----- client/src/components/Form/utilities.js | 5 +- 3 files changed, 105 insertions(+), 16 deletions(-) diff --git a/client/src/components/Form/composables/useFormState.test.js b/client/src/components/Form/composables/useFormState.test.js index 7618b129784..c74e329236b 100644 --- a/client/src/components/Form/composables/useFormState.test.js +++ b/client/src/components/Form/composables/useFormState.test.js @@ -153,6 +153,59 @@ describe("useFormState", () => { expect(caseBInput.attributes.options).toEqual([["col1", "col1"]]); }); + it("should not overwrite earlier case attributes when multiple conditional cases share a parameter name", () => { + const { cloneInputs, formInputs, syncServerAttributes } = useFormState(); + + // All cases have a "feature" parameter — same name, different options per case + cloneInputs([ + { + name: "col_choice", + type: "conditional", + test_param: { name: "col", type: "select", value: "0" }, + cases: [ + { value: "0", inputs: [{ name: "feature", type: "select", value: null }] }, + { value: "2", inputs: [{ name: "feature", type: "select", value: null }] }, + ], + }, + ]); + + // Server returns distinct options for each case + syncServerAttributes([ + { + name: "col_choice", + type: "conditional", + test_param: { name: "col", type: "select" }, + cases: [ + { value: "0", inputs: [{ name: "feature", type: "select", options: [["seqA", "seqA"]] }] }, + { + value: "2", + inputs: [ + { + name: "feature", + type: "select", + options: [ + ["mRNA", "mRNA"], + ["exon", "exon"], + ], + }, + ], + }, + ], + }, + ]); + + const cond = formInputs.value[0]; + const case0Feature = cond.cases[0].inputs[0]; + const case2Feature = cond.cases[1].inputs[0]; + + // Each case must keep its OWN options, not inherit the last case's options + expect(case0Feature.attributes.options).toEqual([["seqA", "seqA"]]); + expect(case2Feature.attributes.options).toEqual([ + ["mRNA", "mRNA"], + ["exon", "exon"], + ]); + }); + it("should update formIndex and formData on conditional switch", () => { const { cloneInputs, formInputs, rebuildIndex, buildFormData, formIndex, formData } = useFormState(); cloneInputs(makeConditionalInputs()); diff --git a/client/src/components/Form/composables/useFormState.ts b/client/src/components/Form/composables/useFormState.ts index 58ef1c7ab22..034406864b7 100644 --- a/client/src/components/Form/composables/useFormState.ts +++ b/client/src/components/Form/composables/useFormState.ts @@ -122,28 +122,63 @@ export function useFormState(options: UseFormStateOptions = {}): UseFormStateRet * Only copies server-owned fields; value, error, and warning are excluded * to maintain the separation between server state (attributes) and client * state (the clone's own properties). + * + * Uses a structural recursive approach so that conditional cases with the + * same parameter name (e.g. "feature" in every `when` branch) are each + * synced from their corresponding server case rather than from the last + * case (which is what a flat-key dict would produce). */ function syncServerAttributes(newInputs: FormInputNode[]): void { - const newAttributes: Record = {}; - visitAllInputs(newInputs, (input: FormInputNode, name: string) => { - newAttributes[name] = input; - }); - visitAllInputs(formInputs.value, (input: FormInputNode, name: string) => { - const serverNode = newAttributes[name]; - if (serverNode != undefined) { - const attrs: Record = {}; - const raw = serverNode as unknown as Record; - for (const key in raw) { - if (!CLIENT_OWNED_FIELDS.has(key)) { - attrs[key] = raw[key]; + syncInputsStructural(formInputs.value, newInputs); + } + + function syncInputsStructural(cloneInputs: FormInputNode[], serverInputs: FormInputNode[]): void { + const serverByName = new Map(serverInputs.map((n) => [String(n.name ?? ""), n])); + cloneInputs.forEach((cloneNode) => { + const serverNode = serverByName.get(String(cloneNode.name ?? "")); + if (!serverNode) { + return; + } + if (cloneNode.type === "conditional" && cloneNode.cases && serverNode.cases) { + syncNodeAttributes(cloneNode.test_param, serverNode.test_param); + cloneNode.cases.forEach((cloneCase, i) => { + const serverCase = serverNode.cases![i]; + if (serverCase) { + syncInputsStructural(cloneCase.inputs, serverCase.inputs); } + }); + } else if (cloneNode.type === "section" && cloneNode.inputs && serverNode.inputs) { + syncInputsStructural(cloneNode.inputs, serverNode.inputs); + } else if (cloneNode.type === "repeat") { + if (cloneNode.cache && serverNode.cache) { + cloneNode.cache.forEach((instance, i) => { + const serverInstance = serverNode.cache![i]; + if (serverInstance) { + syncInputsStructural(instance, serverInstance); + } + }); } - // set() required: attributes is a genuinely new property on clone nodes. - set(input, "attributes", attrs); + } else { + syncNodeAttributes(cloneNode, serverNode); } }); } + function syncNodeAttributes(cloneNode: FormInputNode | undefined, serverNode: FormInputNode | undefined): void { + if (!cloneNode || !serverNode) { + return; + } + const attrs: Record = {}; + const raw = serverNode as unknown as Record; + for (const key in raw) { + if (!CLIENT_OWNED_FIELDS.has(key)) { + attrs[key] = raw[key]; + } + } + // set() required: attributes is a genuinely new property on clone nodes. + set(cloneNode, "attributes", attrs); + } + function applyErrors(errors: FormMessages | null): void { resetErrors(); if (errors) { diff --git a/client/src/components/Form/utilities.js b/client/src/components/Form/utilities.js index d4831b389af..99b2e2de474 100644 --- a/client/src/components/Form/utilities.js +++ b/client/src/components/Form/utilities.js @@ -190,8 +190,9 @@ function _buildLevel(inputs, formData, prefix) { const condResult = {}; if (node.test_param) { const testKey = `${flatKey}|${node.test_param.name}`; - condResult[node.test_param.name] = _convertValue(node.test_param, formData[testKey]); - const selectedCase = matchCase(node, node.test_param.value); + const testValue = _convertValue(node.test_param, formData[testKey]); + condResult[node.test_param.name] = testValue; + const selectedCase = matchCase(node, testValue ?? node.test_param.value); if (selectedCase !== -1) { Object.assign(condResult, _buildLevel(node.cases[selectedCase].inputs, formData, flatKey)); }