mirror of
https://github.com/galaxyproject/galaxy.git
synced 2026-09-24 16:30:27 +08:00
Fix syncServerAttributes clobbering conditional case options for shared param names
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
This commit is contained in:
@@ -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());
|
||||
|
||||
@@ -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<string, FormInputNode> = {};
|
||||
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<string, unknown> = {};
|
||||
const raw = serverNode as unknown as Record<string, unknown>;
|
||||
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<string, unknown> = {};
|
||||
const raw = serverNode as unknown as Record<string, unknown>;
|
||||
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) {
|
||||
|
||||
@@ -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));
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user