diff --git a/backend/windmill-common/src/cache.rs b/backend/windmill-common/src/cache.rs index b3186cc6a2..e39fd6ea43 100644 --- a/backend/windmill-common/src/cache.rs +++ b/backend/windmill-common/src/cache.rs @@ -1231,3 +1231,75 @@ const _: () = { } } }; + +#[cfg(test)] +mod tests { + use super::*; + use serde_json::json; + + #[test] + fn flow_data_extras_preserves_notes_and_groups() { + let raw = serde_json::value::to_raw_value(&json!({ + "modules": [], + "notes": [{"id": "n1", "text": "hello", "color": "blue", "type": "group", + "contained_node_ids": ["a", "b"], "locked": false}], + "groups": [{"start_id": "a", "end_id": "b", "summary": "grp", "color": "green"}] + })) + .unwrap(); + + let data = FlowData::from_raw(raw).unwrap(); + + // FlowValue ignores notes/groups + assert!(data.value().modules.is_empty()); + + // But extras() recovers them from the raw JSON + let extras = data.extras().expect("extras should parse"); + let notes: serde_json::Value = + serde_json::from_str(extras.notes.expect("notes present").get()).unwrap(); + assert_eq!(notes.as_array().unwrap().len(), 1); + assert_eq!(notes[0]["id"], "n1"); + assert_eq!(notes[0]["color"], "blue"); + + let groups: serde_json::Value = + serde_json::from_str(extras.groups.expect("groups present").get()).unwrap(); + assert_eq!(groups.as_array().unwrap().len(), 1); + assert_eq!(groups[0]["start_id"], "a"); + } + + #[test] + fn flow_data_extras_returns_none_when_missing() { + let raw = serde_json::value::to_raw_value(&json!({"modules": []})).unwrap(); + let data = FlowData::from_raw(raw).unwrap(); + + let extras = data + .extras() + .expect("extras should parse even without notes/groups"); + assert!(extras.notes.is_none()); + assert!(extras.groups.is_none()); + } + + #[test] + fn flow_data_extras_lost_after_flow_value_roundtrip() { + // Demonstrates the bug: serializing through FlowValue drops notes/groups. + // This is the root cause of #8641. + let raw = serde_json::value::to_raw_value(&json!({ + "modules": [], + "notes": [{"id": "n1", "text": "t", "color": "blue", "type": "free"}] + })) + .unwrap(); + + let data = FlowData::from_raw(raw).unwrap(); + + // Re-serialize through FlowValue (what RunFlowDependenciesRequest does) + let stripped = serde_json::to_string(data.value()).unwrap(); + let stripped_raw = RawValue::from_string(stripped).unwrap(); + let data2 = FlowData::from_raw(stripped_raw).unwrap(); + + // Notes are gone after the FlowValue round-trip + let extras = data2.extras().expect("extras should parse"); + assert!( + extras.notes.is_none(), + "notes lost after FlowValue round-trip" + ); + } +} diff --git a/cli/src/commands/flow/flow_metadata.ts b/cli/src/commands/flow/flow_metadata.ts index 9805391d05..57a9289fa1 100644 --- a/cli/src/commands/flow/flow_metadata.ts +++ b/cli/src/commands/flow/flow_metadata.ts @@ -23,7 +23,7 @@ import { newPathAssigner } from "../../../windmill-utils-internal/src/path-utils import { generateHash, getHeaders, writeIfChanged } from "../../utils/utils.ts"; import { exts } from "../script/script.ts"; -import { FSFSElement } from "../sync/sync.ts"; +import { FSFSElement, yamlOptions } from "../sync/sync.ts"; import { Workspace } from "../workspace/workspace.ts"; import { FlowFile } from "./flow.ts"; import { FlowValue } from "../../../gen/types.gen.ts"; @@ -226,6 +226,12 @@ export async function generateFlowLockInternal( //removeChangedLocks const tempScriptRefs = tree?.getTempScriptRefs(folderNormalized); + + // Preserve notes and groups — the backend round-trips through FlowValue + // which doesn't include these fields, so they'd be lost (#8641). + const savedNotes = flowValue.value.notes; + const savedGroups = flowValue.value.groups; + flowValue.value = await updateFlow( workspace, flowValue.value, @@ -234,6 +240,10 @@ export async function generateFlowLockInternal( tempScriptRefs ); + // Restore notes and groups that the backend stripped + if (savedNotes !== undefined) flowValue.value.notes = savedNotes; + if (savedGroups !== undefined) flowValue.value.groups = savedGroups; + const lockAssigner = newPathAssigner(opts.defaultTs ?? "bun", { skipInlineScriptSuffix: getNonDottedPaths(), }); @@ -257,7 +267,7 @@ export async function generateFlowLockInternal( // Overwrite `flow.yaml` with the new lockfile references writeIfChanged( process.cwd() + SEP + folder + SEP + "flow.yaml", - yamlStringify(flowValue as Record) + yamlStringify(flowValue as Record, yamlOptions) ); } diff --git a/cli/test/flow_notes_ordering_unit.test.ts b/cli/test/flow_notes_ordering_unit.test.ts new file mode 100644 index 0000000000..b5906cb548 --- /dev/null +++ b/cli/test/flow_notes_ordering_unit.test.ts @@ -0,0 +1,219 @@ +/** + * Unit tests for flow notes/groups preservation and YAML field ordering. + * + * Verifies that: + * - Notes and groups survive a round-trip through generate-metadata (#8641) + * - YAML output uses consistent field ordering via yamlOptions + * + * No backend required — tests the YAML parse/stringify layer. + */ + +import { expect, test, describe } from "bun:test"; +import { yamlParseContent } from "../src/utils/yaml.ts"; +import { stringify as yamlStringify } from "yaml"; +import { yamlOptions } from "../src/commands/sync/sync.ts"; + +const FLOW_WITH_NOTES = ` +summary: Sync item +description: '' +value: + modules: + - id: fetch + summary: Fetch product + value: + type: script + input_transforms: + connection: + type: static + value: some_resource + is_trigger: false + path: f/api/product_get + - id: map + summary: Map item + value: + type: script + input_transforms: + bc_item: + type: javascript + expr: flow_input.bc_item + is_trigger: false + path: f/mapping/item_to_product + notes: + - id: note-abc123 + type: group + color: blue + contained_node_ids: + - fetch + - map + locked: false + text: These steps must run together +schema: + $schema: https://json-schema.org/draft/2020-12/schema + type: object +`; + +const FLOW_WITH_GROUPS = ` +summary: Test flow +description: '' +value: + modules: + - id: a + value: + type: identity + groups: + - summary: My group + start_id: a + end_id: a + color: green +schema: + type: object +`; + +describe("flow notes preservation (#8641)", () => { + test("notes survive YAML round-trip with yamlOptions", () => { + const parsed = yamlParseContent("flow.yaml", FLOW_WITH_NOTES); + + // Verify notes were parsed + expect(parsed.value.notes).toBeDefined(); + expect(parsed.value.notes).toHaveLength(1); + expect(parsed.value.notes[0].id).toBe("note-abc123"); + expect(parsed.value.notes[0].color).toBe("blue"); + expect(parsed.value.notes[0].text).toBe("These steps must run together"); + + // Simulate the generate-metadata round-trip: + // 1. Backend returns a new value WITHOUT notes (like FlowValue does) + const backendResponse = { ...parsed.value }; + delete backendResponse.notes; + + // 2. CLI preserves notes (our fix) + const savedNotes = parsed.value.notes; + parsed.value = backendResponse; + if (savedNotes !== undefined) parsed.value.notes = savedNotes; + + // 3. Serialize back to YAML + const output = yamlStringify(parsed, yamlOptions); + + // 4. Re-parse and verify notes are intact + const reparsed = yamlParseContent("flow.yaml", output); + expect(reparsed.value.notes).toBeDefined(); + expect(reparsed.value.notes).toHaveLength(1); + expect(reparsed.value.notes[0].id).toBe("note-abc123"); + expect(reparsed.value.notes[0].color).toBe("blue"); + expect(reparsed.value.notes[0].contained_node_ids).toEqual(["fetch", "map"]); + expect(reparsed.value.notes[0].text).toBe("These steps must run together"); + }); + + test("groups survive YAML round-trip with yamlOptions", () => { + const parsed = yamlParseContent("flow.yaml", FLOW_WITH_GROUPS); + + expect(parsed.value.groups).toBeDefined(); + expect(parsed.value.groups).toHaveLength(1); + expect(parsed.value.groups[0].summary).toBe("My group"); + + // Simulate backend stripping groups + const backendResponse = { ...parsed.value }; + delete backendResponse.groups; + + const savedGroups = parsed.value.groups; + parsed.value = backendResponse; + if (savedGroups !== undefined) parsed.value.groups = savedGroups; + + const output = yamlStringify(parsed, yamlOptions); + const reparsed = yamlParseContent("flow.yaml", output); + expect(reparsed.value.groups).toBeDefined(); + expect(reparsed.value.groups).toHaveLength(1); + expect(reparsed.value.groups[0].summary).toBe("My group"); + }); + + test("flow without notes or groups is unaffected", () => { + const yaml = ` +summary: Simple flow +value: + modules: + - id: a + value: + type: identity +schema: + type: object +`; + const parsed = yamlParseContent("flow.yaml", yaml); + expect(parsed.value.notes).toBeUndefined(); + expect(parsed.value.groups).toBeUndefined(); + + // Simulate the save/restore logic with undefined + const savedNotes = parsed.value.notes; + const savedGroups = parsed.value.groups; + // Replace value (simulating backend response) + parsed.value = { ...parsed.value }; + if (savedNotes !== undefined) parsed.value.notes = savedNotes; + if (savedGroups !== undefined) parsed.value.groups = savedGroups; + + const output = yamlStringify(parsed, yamlOptions); + const reparsed = yamlParseContent("flow.yaml", output); + expect(reparsed.value.notes).toBeUndefined(); + expect(reparsed.value.groups).toBeUndefined(); + }); +}); + +describe("flow YAML field ordering", () => { + test("yamlOptions produces consistent field order for flow modules", () => { + // Simulate a flow value with fields in random order (like backend response) + const unordered = { + summary: "Test", + value: { + modules: [ + { + value: { type: "script", path: "f/test", is_trigger: false, input_transforms: {} }, + id: "step1", + summary: "Step 1", + }, + ], + }, + schema: { type: "object" }, + description: "", + }; + + const output = yamlStringify(unordered, yamlOptions); + + // With yamlOptions, 'id' should come before 'summary' and 'value' + // because prioritizeName gives "id" → "aa", "summary" → "ad", "value" → "ah" + // Note: YAML sequence items start with "- id:" on the first key + const lines = output.split("\n"); + const idLine = lines.findIndex((l) => /^\s*-?\s*id:/.test(l)); + const summaryLine = lines.findIndex((l, i) => i > idLine && /^\s+summary:/.test(l)); + + expect(idLine).toBeGreaterThan(-1); + expect(summaryLine).toBeGreaterThan(idLine); + }); + + test("yamlOptions produces same output regardless of input key order", () => { + const order1 = { + summary: "Flow", + description: "", + value: { modules: [{ id: "a", summary: "S", value: { type: "identity" } }] }, + schema: { type: "object" }, + }; + const order2 = { + schema: { type: "object" }, + value: { modules: [{ value: { type: "identity" }, summary: "S", id: "a" }] }, + description: "", + summary: "Flow", + }; + + const output1 = yamlStringify(order1, yamlOptions); + const output2 = yamlStringify(order2, yamlOptions); + expect(output1).toBe(output2); + }); + + test("notes field is preserved in correct position after modules", () => { + const parsed = yamlParseContent("flow.yaml", FLOW_WITH_NOTES); + const output = yamlStringify(parsed, yamlOptions); + + // 'modules' should appear before 'notes' in the output + const modulesIdx = output.indexOf("modules:"); + const notesIdx = output.indexOf("notes:"); + expect(modulesIdx).toBeGreaterThan(-1); + expect(notesIdx).toBeGreaterThan(-1); + expect(modulesIdx).toBeLessThan(notesIdx); + }); +});