From f1832afa5f779997dbc69df143862744d1f2b5ac Mon Sep 17 00:00:00 2001 From: Aleksei Sviridkin Date: Tue, 12 May 2026 22:32:39 +0300 Subject: [PATCH 01/11] feat(console): add immutable schema-path helpers Introduce pure helpers used to recognise and enforce Kubernetes-style immutability rules on resource schemas. findImmutablePaths(schema) walks an OpenAPI/JSON-schema tree and returns every path whose subschema carries an x-kubernetes-validations entry with rule "self == oldSelf". Object properties contribute named segments; array items and additionalProperties contribute the wildcard segment "*". oneOf, anyOf and allOf branches are considered when determining whether a node is immutable. overlayImmutable(submitted, original, paths) deep-clones the submitted value, then for each path copies the value from original. For wildcard segments the array length is aligned to original so an immutable list cannot be reshaped client-side. IMMUTABLE_HELP_TEXT centralises the user-facing string consumed by later UI wiring. Assisted-By: Claude Signed-off-by: Aleksei Sviridkin --- apps/console/src/lib/immutable-paths.test.ts | 255 +++++++++++++++++++ apps/console/src/lib/immutable-paths.ts | 129 ++++++++++ 2 files changed, 384 insertions(+) create mode 100644 apps/console/src/lib/immutable-paths.test.ts create mode 100644 apps/console/src/lib/immutable-paths.ts diff --git a/apps/console/src/lib/immutable-paths.test.ts b/apps/console/src/lib/immutable-paths.test.ts new file mode 100644 index 0000000..87b56bd --- /dev/null +++ b/apps/console/src/lib/immutable-paths.test.ts @@ -0,0 +1,255 @@ +import { describe, it, expect } from "vitest" +import { findImmutablePaths, overlayImmutable } from "./immutable-paths.ts" + +const cel = { + "x-kubernetes-validations": [ + { rule: "self == oldSelf", message: "immutable" }, + ], +} + +describe("findImmutablePaths", () => { + it("returns empty array for non-object input", () => { + expect(findImmutablePaths(null)).toEqual([]) + expect(findImmutablePaths(undefined)).toEqual([]) + expect(findImmutablePaths("string")).toEqual([]) + expect(findImmutablePaths(42)).toEqual([]) + }) + + it("returns empty array when no validations are present", () => { + expect( + findImmutablePaths({ + type: "object", + properties: { foo: { type: "string" } }, + }), + ).toEqual([]) + }) + + it("ignores validations whose rule is not self == oldSelf", () => { + expect( + findImmutablePaths({ + properties: { + foo: { + type: "string", + "x-kubernetes-validations": [{ rule: "self.size() > 0" }], + }, + }, + }), + ).toEqual([]) + }) + + it("finds a top-level immutable property", () => { + const schema = { + properties: { + storageClass: { type: "string", ...cel }, + size: { type: "string" }, + }, + } + expect(findImmutablePaths(schema)).toEqual([["storageClass"]]) + }) + + it("finds nested immutable properties", () => { + const schema = { + properties: { + spec: { + properties: { + backup: { + properties: { + storageClass: { type: "string", ...cel }, + }, + }, + }, + }, + }, + } + expect(findImmutablePaths(schema)).toEqual([ + ["spec", "backup", "storageClass"], + ]) + }) + + it("uses \"*\" for array items", () => { + const schema = { + properties: { + disks: { + type: "array", + items: { type: "string", ...cel }, + }, + }, + } + expect(findImmutablePaths(schema)).toEqual([["disks", "*"]]) + }) + + it("descends into array items to find nested immutable fields", () => { + const schema = { + properties: { + disks: { + type: "array", + items: { + type: "object", + properties: { + name: { type: "string", ...cel }, + size: { type: "string" }, + }, + }, + }, + }, + } + expect(findImmutablePaths(schema)).toEqual([["disks", "*", "name"]]) + }) + + it("uses \"*\" for additionalProperties object maps", () => { + const schema = { + properties: { + labels: { + type: "object", + additionalProperties: { type: "string", ...cel }, + }, + }, + } + expect(findImmutablePaths(schema)).toEqual([["labels", "*"]]) + }) + + it("treats parent as immutable when a oneOf branch carries the rule", () => { + const schema = { + properties: { + size: { + oneOf: [{ type: "integer" }, { type: "string", ...cel }], + }, + }, + } + expect(findImmutablePaths(schema)).toEqual([["size"]]) + }) + + it("treats parent as immutable when anyOf/allOf carries the rule", () => { + expect( + findImmutablePaths({ + properties: { + foo: { anyOf: [{ ...cel }] }, + }, + }), + ).toEqual([["foo"]]) + expect( + findImmutablePaths({ + properties: { + bar: { allOf: [{ ...cel }] }, + }, + }), + ).toEqual([["bar"]]) + }) + + it("handles root-level immutable schema (no properties)", () => { + expect(findImmutablePaths(cel)).toEqual([[]]) + }) + + it("collects multiple immutable paths in deterministic order", () => { + const schema = { + properties: { + a: { type: "string", ...cel }, + b: { + properties: { + c: { type: "string", ...cel }, + }, + }, + }, + } + expect(findImmutablePaths(schema)).toEqual([["a"], ["b", "c"]]) + }) +}) + +describe("overlayImmutable", () => { + it("returns a deep clone when paths is empty", () => { + const submitted = { foo: "x", arr: [1, 2] } + const original = { foo: "y", arr: [9] } + const result = overlayImmutable(submitted, original, []) + expect(result).toEqual(submitted) + expect(result).not.toBe(submitted) + expect((result as { arr: number[] }).arr).not.toBe(submitted.arr) + }) + + it("overlays a top-level immutable field", () => { + const submitted = { storageClass: "fast", size: "10Gi" } + const original = { storageClass: "slow", size: "5Gi" } + expect( + overlayImmutable(submitted, original, [["storageClass"]]), + ).toEqual({ storageClass: "slow", size: "10Gi" }) + }) + + it("preserves siblings when overlaying nested fields", () => { + const submitted = { + spec: { backup: { storageClass: "fast", schedule: "*/5" } }, + } + const original = { + spec: { backup: { storageClass: "slow", schedule: "*/1" } }, + } + expect( + overlayImmutable(submitted, original, [ + ["spec", "backup", "storageClass"], + ]), + ).toEqual({ + spec: { backup: { storageClass: "slow", schedule: "*/5" } }, + }) + }) + + it("inserts the immutable field if it is missing from submitted", () => { + const submitted = { size: "10Gi" } as Record + const original = { size: "5Gi", storageClass: "slow" } + expect( + overlayImmutable(submitted, original, [["storageClass"]]), + ).toEqual({ size: "10Gi", storageClass: "slow" }) + }) + + it("aligns array length to original when array element path is immutable", () => { + const submitted = { disks: ["x", "y", "z"] } + const original = { disks: ["a", "b"] } + expect( + overlayImmutable(submitted, original, [["disks", "*"]]), + ).toEqual({ disks: ["a", "b"] }) + }) + + it("extends submitted array when original is longer", () => { + const submitted = { disks: ["x"] } + const original = { disks: ["a", "b"] } + expect( + overlayImmutable(submitted, original, [["disks", "*"]]), + ).toEqual({ disks: ["a", "b"] }) + }) + + it("overlays nested array-item field when only that field is immutable", () => { + const submitted = { + disks: [ + { name: "renamed1", size: "10Gi" }, + { name: "renamed2", size: "20Gi" }, + ], + } + const original = { + disks: [ + { name: "disk1", size: "5Gi" }, + { name: "disk2", size: "5Gi" }, + ], + } + expect( + overlayImmutable(submitted, original, [["disks", "*", "name"]]), + ).toEqual({ + disks: [ + { name: "disk1", size: "10Gi" }, + { name: "disk2", size: "20Gi" }, + ], + }) + }) + + it("tolerates null/undefined values along the path", () => { + expect( + overlayImmutable({}, { storageClass: "slow" }, [["storageClass"]]), + ).toEqual({ storageClass: "slow" }) + expect( + overlayImmutable({ storageClass: "fast" }, {}, [["storageClass"]]), + ).toEqual({ storageClass: undefined }) + }) + + it("does not mutate inputs", () => { + const submitted = { storageClass: "fast", arr: [1, 2] } + const original = { storageClass: "slow", arr: [9] } + overlayImmutable(submitted, original, [["storageClass"]]) + expect(submitted).toEqual({ storageClass: "fast", arr: [1, 2] }) + expect(original).toEqual({ storageClass: "slow", arr: [9] }) + }) +}) diff --git a/apps/console/src/lib/immutable-paths.ts b/apps/console/src/lib/immutable-paths.ts new file mode 100644 index 0000000..a96c60d --- /dev/null +++ b/apps/console/src/lib/immutable-paths.ts @@ -0,0 +1,129 @@ +/** + * Discovery and enforcement of Kubernetes-style immutability rules + * (`x-kubernetes-validations: [{rule: "self == oldSelf"}]`) on OpenAPI/JSON + * schemas served by Cozystack ApplicationDefinitions and CRDs. + * + * The UI strips this extension before handing the schema to AJV (which has no + * CEL evaluator); the paths it announces are used to (a) mark form fields as + * read-only in edit mode and (b) overlay the original values into PUT bodies + * so the API server never observes a change on those paths. + */ + +export type ImmutablePath = readonly string[] + +export const IMMUTABLE_HELP_TEXT = + "This field cannot be changed after creation." + +const IMMUTABILITY_RULE = "self == oldSelf" + +const isPlainObject = (v: unknown): v is Record => + v !== null && typeof v === "object" && !Array.isArray(v) + +const hasImmutabilityRule = (node: unknown): boolean => { + if (!isPlainObject(node)) return false + const validations = node["x-kubernetes-validations"] + if (!Array.isArray(validations)) return false + return validations.some( + (v) => isPlainObject(v) && v.rule === IMMUTABILITY_RULE, + ) +} + +const branchHasImmutability = (node: unknown): boolean => { + if (!isPlainObject(node)) return false + if (hasImmutabilityRule(node)) return true + for (const key of ["oneOf", "anyOf", "allOf"] as const) { + const branches = node[key] + if (Array.isArray(branches) && branches.some(branchHasImmutability)) { + return true + } + } + return false +} + +export function findImmutablePaths(schema: unknown): ImmutablePath[] { + const out: string[][] = [] + walk(schema, [], out) + return out +} + +function walk(node: unknown, path: string[], out: string[][]): void { + if (!isPlainObject(node)) return + + if (branchHasImmutability(node)) { + out.push([...path]) + return + } + + const properties = node.properties + if (isPlainObject(properties)) { + for (const [key, value] of Object.entries(properties)) { + walk(value, [...path, key], out) + } + } + + const items = node.items + if (isPlainObject(items)) { + walk(items, [...path, "*"], out) + } + + const additional = node.additionalProperties + if (isPlainObject(additional)) { + walk(additional, [...path, "*"], out) + } +} + +export function overlayImmutable( + submitted: T, + original: T, + paths: readonly ImmutablePath[], +): T { + const next = deepClone(submitted) + for (const path of paths) { + overlayPath(next, original, path, 0) + } + return next +} + +function overlayPath( + target: unknown, + source: unknown, + path: ImmutablePath, + depth: number, +): unknown { + if (depth === path.length) { + return deepClone(source) + } + const seg = path[depth] + if (seg === "*") { + if (!Array.isArray(source)) { + return Array.isArray(target) ? target : source + } + const out: unknown[] = [] + for (let i = 0; i < source.length; i++) { + const t = Array.isArray(target) ? target[i] : undefined + out[i] = overlayPath(t, source[i], path, depth + 1) + } + return out + } + const sourceObj = isPlainObject(source) ? source : undefined + const sourceVal = sourceObj ? sourceObj[seg] : undefined + const targetObj = isPlainObject(target) + ? (target as Record) + : null + if (!targetObj) return target + targetObj[seg] = overlayPath(targetObj[seg], sourceVal, path, depth + 1) + return targetObj +} + +function deepClone(value: T): T { + if (value === null || value === undefined) return value + if (Array.isArray(value)) return value.map(deepClone) as unknown as T + if (typeof value === "object") { + const out: Record = {} + for (const [k, v] of Object.entries(value as Record)) { + out[k] = deepClone(v) + } + return out as unknown as T + } + return value +} From 67618aeb9b4770668584bac6e9880524bf4c1c5d Mon Sep 17 00:00:00 2001 From: Aleksei Sviridkin Date: Tue, 12 May 2026 22:34:09 +0300 Subject: [PATCH 02/11] feat(console): strip x-kubernetes-validations during schema sanitisation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit AJV has no CEL evaluator, so the rule does nothing for JSON-Schema validation; the UI consults the raw schema once to harvest immutability paths and then drops the extension so RJSF traversal stays small. Also add characterization tests for keysOrderToUiSchema and sanitizeSchema so existing behaviour (int-or-string coercion, preserve-unknown-fields → additionalProperties, "Chart Values" → "Parameters") is locked in. Assisted-By: Claude Signed-off-by: Aleksei Sviridkin --- apps/console/src/lib/keys-order.test.ts | 101 ++++++++++++++++++++++++ apps/console/src/lib/keys-order.ts | 4 + 2 files changed, 105 insertions(+) create mode 100644 apps/console/src/lib/keys-order.test.ts diff --git a/apps/console/src/lib/keys-order.test.ts b/apps/console/src/lib/keys-order.test.ts new file mode 100644 index 0000000..c281b4c --- /dev/null +++ b/apps/console/src/lib/keys-order.test.ts @@ -0,0 +1,101 @@ +import { describe, it, expect } from "vitest" +import { keysOrderToUiSchema, sanitizeSchema } from "./keys-order.ts" + +describe("keysOrderToUiSchema", () => { + it("returns empty object for missing/empty input", () => { + expect(keysOrderToUiSchema(undefined)).toEqual({}) + expect(keysOrderToUiSchema([])).toEqual({}) + }) + + it("ignores meta paths (apiVersion, kind, metadata) and only emits spec keys", () => { + const ui = keysOrderToUiSchema([ + ["apiVersion"], + ["kind"], + ["metadata"], + ["metadata", "name"], + ["spec", "storageClass"], + ["spec", "size"], + ]) + expect(ui).toEqual({ "ui:order": ["storageClass", "size", "*"] }) + }) + + it("nests ui:order per parent path", () => { + const ui = keysOrderToUiSchema([ + ["spec", "nodeGroups"], + ["spec", "nodeGroups", "md0"], + ["spec", "nodeGroups", "md0", "minReplicas"], + ["spec", "nodeGroups", "md0", "maxReplicas"], + ]) + expect(ui).toEqual({ + "ui:order": ["nodeGroups", "*"], + nodeGroups: { + "ui:order": ["md0", "*"], + md0: { + "ui:order": ["minReplicas", "maxReplicas", "*"], + }, + }, + }) + }) +}) + +describe("sanitizeSchema", () => { + it("returns scalars and null unchanged", () => { + expect(sanitizeSchema(null)).toBe(null) + expect(sanitizeSchema(42)).toBe(42) + expect(sanitizeSchema("x")).toBe("x") + }) + + it("recurses into arrays", () => { + const input = [{ type: "string" }, { type: "integer" }] + expect(sanitizeSchema(input)).toEqual(input) + }) + + it("coerces x-kubernetes-int-or-string to string type and drops anyOf", () => { + const out = sanitizeSchema({ + "x-kubernetes-int-or-string": true, + anyOf: [{ type: "integer" }, { type: "string" }], + }) + expect(out.type).toBe("string") + expect(out.anyOf).toBeUndefined() + }) + + it("coerces x-kubernetes-preserve-unknown-fields on a propertyless object", () => { + const out = sanitizeSchema({ + "x-kubernetes-preserve-unknown-fields": true, + }) + expect(out.type).toBe("object") + expect(out.additionalProperties).toBe(true) + }) + + it("rewrites \"Chart Values\" title to \"Parameters\"", () => { + expect(sanitizeSchema({ title: "Chart Values" }).title).toBe("Parameters") + expect(sanitizeSchema({ title: "Other" }).title).toBe("Other") + }) + + it("strips x-kubernetes-validations from any level", () => { + const input = { + properties: { + storageClass: { + type: "string", + "x-kubernetes-validations": [ + { rule: "self == oldSelf", message: "immutable" }, + ], + }, + nested: { + properties: { + inner: { + type: "integer", + "x-kubernetes-validations": [{ rule: "self > 0" }], + }, + }, + }, + }, + } + const out = sanitizeSchema(input) + expect(out.properties.storageClass["x-kubernetes-validations"]).toBeUndefined() + expect( + out.properties.nested.properties.inner["x-kubernetes-validations"], + ).toBeUndefined() + expect(out.properties.storageClass.type).toBe("string") + }) +}) diff --git a/apps/console/src/lib/keys-order.ts b/apps/console/src/lib/keys-order.ts index cc64a37..dacddca 100644 --- a/apps/console/src/lib/keys-order.ts +++ b/apps/console/src/lib/keys-order.ts @@ -70,6 +70,10 @@ export function sanitizeSchema(schema: any): any { const out: Record = {} for (const [k, v] of Object.entries(schema)) { if (k === "anyOf" && schema["x-kubernetes-int-or-string"]) continue + // CEL validations have no JSON-Schema validator-side semantics; the UI + // extracts immutability paths up-front in SchemaForm and then strips + // the rules so AJV doesn't waste cycles traversing them. + if (k === "x-kubernetes-validations") continue out[k] = sanitizeSchema(v) } From 7c775eac8b4e90348fd3256e471bceaa909082d8 Mon Sep 17 00:00:00 2001 From: Aleksei Sviridkin Date: Tue, 12 May 2026 22:39:14 +0300 Subject: [PATCH 03/11] feat(console): plumb immutable mode through SchemaForm When immutableMode='enforce' is passed, every immutable schema path (as reported by findImmutablePaths) receives ui:readonly=true plus a ui:help string sourced from IMMUTABLE_HELP_TEXT. RJSF then renders the corresponding inputs read-only and surfaces the helper text underneath. The default 'off' keeps every field editable so create flows are unaffected. Wildcard segments map to RJSF's 'items' key for arrays; for object maps (additionalProperties) the readonly flag is set on the field itself, to be propagated by the AdditionalPropertiesField renderer. Add a RTL component test covering enforce / off / omitted paths, plus afterEach(cleanup) in the shared test-setup so renders don't leak between cases. Assisted-By: Claude Signed-off-by: Aleksei Sviridkin --- .../src/components/SchemaForm.test.tsx | 69 ++++++++++++++ apps/console/src/components/SchemaForm.tsx | 95 ++++++++++++++++++- 2 files changed, 162 insertions(+), 2 deletions(-) create mode 100644 apps/console/src/components/SchemaForm.test.tsx diff --git a/apps/console/src/components/SchemaForm.test.tsx b/apps/console/src/components/SchemaForm.test.tsx new file mode 100644 index 0000000..1658753 --- /dev/null +++ b/apps/console/src/components/SchemaForm.test.tsx @@ -0,0 +1,69 @@ +import { describe, it, expect } from "vitest" +import { render, screen } from "@testing-library/react" +import { SchemaForm } from "./SchemaForm.tsx" +import { IMMUTABLE_HELP_TEXT } from "../lib/immutable-paths.ts" + +const schema = JSON.stringify({ + type: "object", + properties: { + version: { + type: "string", + "x-kubernetes-validations": [ + { rule: "self == oldSelf", message: "version is immutable" }, + ], + }, + description: { + type: "string", + }, + }, +}) + +const noop = () => {} + +describe("SchemaForm immutableMode", () => { + it("renders fields editable and without help text when immutableMode is omitted", () => { + render( + , + ) + const versionInput = screen.getByLabelText("version") as HTMLInputElement + expect(versionInput).not.toHaveAttribute("readonly") + expect(versionInput).not.toBeDisabled() + expect(screen.queryByText(IMMUTABLE_HELP_TEXT)).not.toBeInTheDocument() + }) + + it("marks immutable fields read-only and shows help text when immutableMode is enforce", () => { + render( + , + ) + const versionInput = screen.getByLabelText("version") as HTMLInputElement + expect(versionInput).toHaveAttribute("readonly") + const descriptionInput = screen.getByLabelText( + "description", + ) as HTMLInputElement + expect(descriptionInput).not.toHaveAttribute("readonly") + expect(screen.getByText(IMMUTABLE_HELP_TEXT)).toBeInTheDocument() + }) + + it("treats immutableMode='off' the same as omitting the prop", () => { + render( + , + ) + const versionInput = screen.getByLabelText("version") as HTMLInputElement + expect(versionInput).not.toHaveAttribute("readonly") + expect(screen.queryByText(IMMUTABLE_HELP_TEXT)).not.toBeInTheDocument() + }) +}) diff --git a/apps/console/src/components/SchemaForm.tsx b/apps/console/src/components/SchemaForm.tsx index af6a47d..61a479a 100644 --- a/apps/console/src/components/SchemaForm.tsx +++ b/apps/console/src/components/SchemaForm.tsx @@ -5,6 +5,11 @@ import { getDefaultFormState } from "@rjsf/utils" import type { RJSFSchema, UiSchema, TemplatesType } from "@rjsf/utils" import { keysOrderToUiSchema, sanitizeSchema } from "../lib/keys-order.ts" import { addSensitiveStringWidgets } from "../lib/sensitive-fields.ts" +import { + IMMUTABLE_HELP_TEXT, + findImmutablePaths, + type ImmutablePath, +} from "../lib/immutable-paths.ts" import { customTemplates, customWidgets } from "./rjsf-templates.tsx" import { AdditionalPropertiesField } from "./AdditionalPropertiesField.tsx" import { ResourceQuotasField } from "./ResourceQuotasField.tsx" @@ -144,12 +149,88 @@ function addVMDiskWidgets(schema: RJSFSchema, uiSchema: UiSchema = {}): UiSchema return result } +/** + * Apply ui:readonly + ui:help to every path the schema declares immutable. + * Wildcard "*" segments translate to "items" for arrays; for object maps + * (additionalProperties) the readonly flag is set on the field itself so + * AdditionalPropertiesField can propagate it to the nested form. + */ +function addImmutableReadonly( + schema: RJSFSchema, + uiSchema: UiSchema, + paths: readonly ImmutablePath[], +): UiSchema { + if (paths.length === 0) return uiSchema + const next: UiSchema = { ...uiSchema } + for (const path of paths) { + applyImmutablePath(schema, next, path, 0) + } + return next +} + +function applyImmutablePath( + schemaNode: unknown, + uiNode: Record, + path: ImmutablePath, + depth: number, +): void { + if (depth === path.length) { + uiNode["ui:readonly"] = true + uiNode["ui:help"] = IMMUTABLE_HELP_TEXT + return + } + const seg = path[depth] + const schemaObj = + schemaNode && typeof schemaNode === "object" && !Array.isArray(schemaNode) + ? (schemaNode as Record) + : null + if (seg === "*") { + if (schemaObj && schemaObj.items) { + const childUi = ensureChild(uiNode, "items") + applyImmutablePath(schemaObj.items, childUi, path, depth + 1) + return + } + // additionalProperties object map: mark the field itself; the + // AdditionalPropertiesField custom renderer reads ui:readonly and + // disables the inner form. + uiNode["ui:readonly"] = true + uiNode["ui:help"] = IMMUTABLE_HELP_TEXT + return + } + const childSchema = schemaObj + ? (schemaObj.properties as Record | undefined)?.[seg] + : undefined + const childUi = ensureChild(uiNode, seg) + applyImmutablePath(childSchema, childUi, path, depth + 1) +} + +function ensureChild( + uiNode: Record, + key: string, +): Record { + const existing = uiNode[key] + if (existing && typeof existing === "object" && !Array.isArray(existing)) { + const cloned = { ...(existing as Record) } + uiNode[key] = cloned + return cloned + } + const fresh: Record = {} + uiNode[key] = fresh + return fresh +} + interface SchemaFormProps { openAPISchema: string keysOrder?: string[][] formData: unknown onChange: (data: unknown) => void children?: React.ReactNode + /** + * When "enforce", fields whose schema carries a CEL immutability rule + * (`self == oldSelf`) are rendered read-only with helper text. Default + * "off" keeps every field editable — used for create flows. + */ + immutableMode?: "enforce" | "off" } export function SchemaForm({ @@ -158,6 +239,7 @@ export function SchemaForm({ formData, onChange, children, + immutableMode, }: SchemaFormProps) { const schema = useMemo(() => { try { @@ -185,6 +267,15 @@ export function SchemaForm({ // eslint-disable-next-line react-hooks/exhaustive-deps }, [schema]) + const immutablePaths = useMemo(() => { + if (immutableMode !== "enforce") return [] + try { + return findImmutablePaths(JSON.parse(openAPISchema)) + } catch { + return [] + } + }, [openAPISchema, immutableMode]) + const uiSchema = useMemo(() => { const baseUiSchema: UiSchema = { "ui:submitButtonOptions": { norender: true }, @@ -221,8 +312,8 @@ export function SchemaForm({ } } - return withSensitive - }, [keysOrder, schema]) + return addImmutableReadonly(schema, withSensitive, immutablePaths) + }, [keysOrder, schema, immutablePaths]) const customFields = useMemo( () => ({ From 32043d3af30e952c88fbc317cc8cb7050568787e Mon Sep 17 00:00:00 2001 From: Aleksei Sviridkin Date: Tue, 12 May 2026 22:40:14 +0300 Subject: [PATCH 04/11] test(console): cover AdditionalPropertiesField immutable rendering The component already derives isReadonly from readonly || disabled and forwards it to the inner
and Add/Remove controls; the new tests pin that contract by exercising it via SchemaForm + immutableMode. When immutableMode is omitted the Add control and per-entry Remove buttons render. With immutableMode='enforce' those controls disappear, the inner inputs become disabled, and the canonical help text is rendered alongside the map field. Assisted-By: Claude Signed-off-by: Aleksei Sviridkin --- .../AdditionalPropertiesField.test.tsx | 59 +++++++++++++++++++ 1 file changed, 59 insertions(+) create mode 100644 apps/console/src/components/AdditionalPropertiesField.test.tsx diff --git a/apps/console/src/components/AdditionalPropertiesField.test.tsx b/apps/console/src/components/AdditionalPropertiesField.test.tsx new file mode 100644 index 0000000..6871d6c --- /dev/null +++ b/apps/console/src/components/AdditionalPropertiesField.test.tsx @@ -0,0 +1,59 @@ +import { describe, it, expect } from "vitest" +import { render, screen } from "@testing-library/react" +import { SchemaForm } from "./SchemaForm.tsx" +import { IMMUTABLE_HELP_TEXT } from "../lib/immutable-paths.ts" + +const schemaWithMap = JSON.stringify({ + type: "object", + properties: { + labels: { + type: "object", + additionalProperties: { type: "string" }, + "x-kubernetes-validations": [ + { rule: "self == oldSelf", message: "labels are immutable" }, + ], + }, + }, +}) + +const noop = () => {} + +describe("AdditionalPropertiesField immutable", () => { + it("renders Add/Remove controls when not immutable", () => { + render( + , + ) + expect(screen.getByPlaceholderText("Enter key name...")).toBeInTheDocument() + expect(screen.getByRole("button", { name: /add/i })).toBeInTheDocument() + expect(screen.getByRole("button", { name: /remove/i })).toBeInTheDocument() + }) + + it("hides Add/Remove and disables inner fields when the map is immutable in enforce mode", () => { + render( + , + ) + expect( + screen.queryByPlaceholderText("Enter key name..."), + ).not.toBeInTheDocument() + expect( + screen.queryByRole("button", { name: /add/i }), + ).not.toBeInTheDocument() + expect( + screen.queryByRole("button", { name: /remove/i }), + ).not.toBeInTheDocument() + + const innerInput = screen.getByDisplayValue("prod") as HTMLInputElement + expect(innerInput).toBeDisabled() + + expect(screen.getByText(IMMUTABLE_HELP_TEXT)).toBeInTheDocument() + }) +}) From 0ff1fe7d2c9f8f20b5c90b03ff7cb3096ebd9f04 Mon Sep 17 00:00:00 2001 From: Aleksei Sviridkin Date: Tue, 12 May 2026 22:44:24 +0300 Subject: [PATCH 05/11] feat(console): enforce immutable fields in edit pages ApplicationOrderPage and BackupResourceEditPage now opt their SchemaForm into immutableMode='enforce' when in edit flow so the relevant fields render read-only with helper text. On submit, prepareUpdateSpec walks the schema for immutable paths and copies the original values from the persisted spec into the outgoing body. The API server therefore observes no delta on those paths and the CEL immutability check is not triggered, even if the user managed to bypass the read-only UI (YAML editor, devtools, RJSF bug). prepareUpdateSpec is a thin pure wrapper around findImmutablePaths + overlayImmutable so the schema-vs-no-schema and malformed-JSON edges can be locked down without rendering full pages under jsdom. Assisted-By: Claude Signed-off-by: Aleksei Sviridkin --- apps/console/src/lib/prepare-update.test.ts | 56 +++++++++++++++++++ apps/console/src/lib/prepare-update.ts | 28 ++++++++++ .../src/routes/ApplicationOrderPage.tsx | 7 +++ .../src/routes/BackupResourceEditPage.tsx | 4 +- 4 files changed, 94 insertions(+), 1 deletion(-) create mode 100644 apps/console/src/lib/prepare-update.test.ts create mode 100644 apps/console/src/lib/prepare-update.ts diff --git a/apps/console/src/lib/prepare-update.test.ts b/apps/console/src/lib/prepare-update.test.ts new file mode 100644 index 0000000..059dcf8 --- /dev/null +++ b/apps/console/src/lib/prepare-update.test.ts @@ -0,0 +1,56 @@ +import { describe, it, expect } from "vitest" +import { prepareUpdateSpec } from "./prepare-update.ts" + +const immutableStorageClassSchema = JSON.stringify({ + type: "object", + properties: { + storageClass: { + type: "string", + "x-kubernetes-validations": [ + { rule: "self == oldSelf", message: "immutable" }, + ], + }, + size: { type: "string" }, + }, +}) + +const schemaWithoutImmutability = JSON.stringify({ + type: "object", + properties: { + storageClass: { type: "string" }, + size: { type: "string" }, + }, +}) + +describe("prepareUpdateSpec", () => { + it("overlays immutable values from initialSpec into submitted spec", () => { + const submitted = { storageClass: "fast", size: "10Gi" } + const initial = { storageClass: "slow", size: "5Gi" } + expect( + prepareUpdateSpec(submitted, initial, immutableStorageClassSchema), + ).toEqual({ storageClass: "slow", size: "10Gi" }) + }) + + it("returns a clone of submitted unchanged when the schema declares no immutability", () => { + const submitted = { storageClass: "fast", size: "10Gi" } + const initial = { storageClass: "slow", size: "5Gi" } + const result = prepareUpdateSpec(submitted, initial, schemaWithoutImmutability) + expect(result).toEqual(submitted) + expect(result).not.toBe(submitted) + }) + + it("returns a clone of submitted unchanged when the schema is malformed JSON", () => { + const submitted = { storageClass: "fast" } + const initial = { storageClass: "slow" } + const result = prepareUpdateSpec(submitted, initial, "not-json") + expect(result).toEqual(submitted) + }) + + it("does not mutate inputs", () => { + const submitted = { storageClass: "fast", size: "10Gi" } + const initial = { storageClass: "slow", size: "5Gi" } + prepareUpdateSpec(submitted, initial, immutableStorageClassSchema) + expect(submitted).toEqual({ storageClass: "fast", size: "10Gi" }) + expect(initial).toEqual({ storageClass: "slow", size: "5Gi" }) + }) +}) diff --git a/apps/console/src/lib/prepare-update.ts b/apps/console/src/lib/prepare-update.ts new file mode 100644 index 0000000..73fde14 --- /dev/null +++ b/apps/console/src/lib/prepare-update.ts @@ -0,0 +1,28 @@ +import { + findImmutablePaths, + overlayImmutable, + type ImmutablePath, +} from "./immutable-paths.ts" + +/** + * Build the spec to PUT during an edit, overlaying immutable values from + * the original spec so that the API server sees no delta on paths whose + * schema carries `x-kubernetes-validations: [{rule: "self == oldSelf"}]`. + * + * Defence-in-depth: even if the form's read-only state is bypassed (via + * the YAML editor, devtools, or a UI bug) the outgoing body still + * matches the persisted spec on those paths. + */ +export function prepareUpdateSpec( + submitted: T, + original: T, + openAPISchema: string, +): T { + let paths: ImmutablePath[] + try { + paths = findImmutablePaths(JSON.parse(openAPISchema)) + } catch { + paths = [] + } + return overlayImmutable(submitted, original, paths) +} diff --git a/apps/console/src/routes/ApplicationOrderPage.tsx b/apps/console/src/routes/ApplicationOrderPage.tsx index 037bfad..f8b9bad 100644 --- a/apps/console/src/routes/ApplicationOrderPage.tsx +++ b/apps/console/src/routes/ApplicationOrderPage.tsx @@ -12,6 +12,7 @@ import { } from "../lib/app-definitions.ts" import { useTenantContext } from "../lib/tenant-context.tsx" import { composeResource } from "../lib/app-resource.ts" +import { prepareUpdateSpec } from "../lib/prepare-update.ts" import { SchemaForm } from "../components/SchemaForm.tsx" import { YamlEditor } from "../components/YamlEditor.tsx" @@ -109,6 +110,11 @@ export function ApplicationOrderPage({ const body = composeResource(ad, tenantNamespace, snap.name, snap.spec) try { if (editMode) { + body.spec = prepareUpdateSpec>( + body.spec ?? {}, + (editMode.initialSpec ?? {}) as Record, + ad.spec?.application.openAPISchema ?? "", + ) await update.mutateAsync(body) } else { await create.mutateAsync(body) @@ -248,6 +254,7 @@ export function ApplicationOrderPage({ keysOrder={ad.spec?.dashboard?.keysOrder} formData={spec} onChange={setSpec} + immutableMode={editMode ? "enforce" : "off"} /> )} diff --git a/apps/console/src/routes/BackupResourceEditPage.tsx b/apps/console/src/routes/BackupResourceEditPage.tsx index 5dcae76..ab130c8 100644 --- a/apps/console/src/routes/BackupResourceEditPage.tsx +++ b/apps/console/src/routes/BackupResourceEditPage.tsx @@ -5,6 +5,7 @@ import { Button, Section, Spinner } from "@cozystack/ui" import { useK8sGet, useK8sUpdate } from "@cozystack/k8s-client" import { useTenantContext } from "../lib/tenant-context.tsx" import { useCRDSchema } from "../lib/use-crd-schema.ts" +import { prepareUpdateSpec } from "../lib/prepare-update.ts" import { SchemaForm } from "../components/SchemaForm.tsx" interface BackupResourceEditPageProps { @@ -69,7 +70,7 @@ export function BackupResourceEditPage({ const updated = { ...resource, - spec: formData, + spec: prepareUpdateSpec(formData, resource.spec, schema ?? ""), } try { @@ -141,6 +142,7 @@ export function BackupResourceEditPage({ openAPISchema={schema} formData={formData} onChange={setFormData} + immutableMode="enforce" >
From 35a679a91f8b742f65eaa8c4e8818102f53cfc8a Mon Sep 17 00:00:00 2001 From: Aleksei Sviridkin Date: Wed, 13 May 2026 01:04:10 +0300 Subject: [PATCH 06/11] fix(console): address review on immutable-field plumbing - Switch from ui:readonly to ui:disabled so immutable inputs render greyed out, matching the product decision. RJSF readonly only sets the HTML readonly attribute, which leaves the control visually identical to an editable one. - overlayImmutable: warn and replace wholesale when a root-level immutable path is supplied (almost certainly a schema-authoring mistake worth surfacing in DevTools). - overlayImmutable: when a wildcard segment is the last in the path the whole array is replaced from source; when nested fields are immutable the loop iterates the longer of source/target so the user can still add new array entries and they survive the overlay. - prepareUpdateSpec: short-circuit to a deep clone when original is null/undefined to avoid blanking immutable fields with undefined. Log a console.warn when the schema string fails to parse instead of silently disabling immutability enforcement. - SchemaForm: parse openAPISchema once and derive the sanitised schema plus immutable path set from the shared object. - SchemaForm: add a SchemaForm-level test for the array-items wildcard case so the rendering branch is covered end-to-end. - keysOrderToUiSchema: accept ReadonlyArray> so call sites can hand in immutable-path-style arrays without casting. Assisted-By: Claude Signed-off-by: Aleksei Sviridkin --- .../src/components/SchemaForm.test.tsx | 45 ++++++++++++-- apps/console/src/components/SchemaForm.tsx | 61 ++++++++++++------- apps/console/src/lib/immutable-paths.test.ts | 33 +++++++++- apps/console/src/lib/immutable-paths.ts | 35 +++++++++-- apps/console/src/lib/keys-order.ts | 4 +- apps/console/src/lib/prepare-update.test.ts | 20 +++++- apps/console/src/lib/prepare-update.ts | 23 ++++++- 7 files changed, 185 insertions(+), 36 deletions(-) diff --git a/apps/console/src/components/SchemaForm.test.tsx b/apps/console/src/components/SchemaForm.test.tsx index 1658753..67a4590 100644 --- a/apps/console/src/components/SchemaForm.test.tsx +++ b/apps/console/src/components/SchemaForm.test.tsx @@ -30,12 +30,11 @@ describe("SchemaForm immutableMode", () => { />, ) const versionInput = screen.getByLabelText("version") as HTMLInputElement - expect(versionInput).not.toHaveAttribute("readonly") expect(versionInput).not.toBeDisabled() expect(screen.queryByText(IMMUTABLE_HELP_TEXT)).not.toBeInTheDocument() }) - it("marks immutable fields read-only and shows help text when immutableMode is enforce", () => { + it("greys out immutable fields and shows help text when immutableMode is enforce", () => { render( { />, ) const versionInput = screen.getByLabelText("version") as HTMLInputElement - expect(versionInput).toHaveAttribute("readonly") + expect(versionInput).toBeDisabled() const descriptionInput = screen.getByLabelText( "description", ) as HTMLInputElement - expect(descriptionInput).not.toHaveAttribute("readonly") + expect(descriptionInput).not.toBeDisabled() expect(screen.getByText(IMMUTABLE_HELP_TEXT)).toBeInTheDocument() }) @@ -63,7 +62,43 @@ describe("SchemaForm immutableMode", () => { />, ) const versionInput = screen.getByLabelText("version") as HTMLInputElement - expect(versionInput).not.toHaveAttribute("readonly") + expect(versionInput).not.toBeDisabled() expect(screen.queryByText(IMMUTABLE_HELP_TEXT)).not.toBeInTheDocument() }) + + it("greys out immutable nested fields inside array items", () => { + const arraySchema = JSON.stringify({ + type: "object", + properties: { + volumes: { + type: "array", + items: { + type: "object", + properties: { + name: { + type: "string", + "x-kubernetes-validations": [ + { rule: "self == oldSelf", message: "name immutable" }, + ], + }, + size: { type: "string" }, + }, + }, + }, + }, + }) + render( + , + ) + const nameInput = screen.getByDisplayValue("disk1") as HTMLInputElement + const sizeInput = screen.getByDisplayValue("10Gi") as HTMLInputElement + expect(nameInput).toBeDisabled() + expect(sizeInput).not.toBeDisabled() + expect(screen.getByText(IMMUTABLE_HELP_TEXT)).toBeInTheDocument() + }) }) diff --git a/apps/console/src/components/SchemaForm.tsx b/apps/console/src/components/SchemaForm.tsx index 61a479a..021e2e1 100644 --- a/apps/console/src/components/SchemaForm.tsx +++ b/apps/console/src/components/SchemaForm.tsx @@ -150,10 +150,18 @@ function addVMDiskWidgets(schema: RJSFSchema, uiSchema: UiSchema = {}): UiSchema } /** - * Apply ui:readonly + ui:help to every path the schema declares immutable. - * Wildcard "*" segments translate to "items" for arrays; for object maps - * (additionalProperties) the readonly flag is set on the field itself so - * AdditionalPropertiesField can propagate it to the nested form. + * Apply ui:disabled + ui:help to every path the schema declares immutable. + * The disabled flag (not readonly) gives the grey-out treatment specified + * by product. Wildcard "*" segments translate to "items" for arrays. For + * object maps (additionalProperties) the disabled flag is set on the + * field itself so AdditionalPropertiesField hides Add/Remove controls and + * disables the nested forms — see the comment at the additionalProperties + * branch below for the UX trade-off. + * + * NOTE: this walker reads the raw (sanitised) schema for navigation. It + * relies on sanitizeSchema preserving `properties`, `items` and + * `additionalProperties`; if a future sanitisation step ever rewrites + * those structurally the walker needs to be updated in lockstep. */ function addImmutableReadonly( schema: RJSFSchema, @@ -175,7 +183,7 @@ function applyImmutablePath( depth: number, ): void { if (depth === path.length) { - uiNode["ui:readonly"] = true + uiNode["ui:disabled"] = true uiNode["ui:help"] = IMMUTABLE_HELP_TEXT return } @@ -190,10 +198,14 @@ function applyImmutablePath( applyImmutablePath(schemaObj.items, childUi, path, depth + 1) return } - // additionalProperties object map: mark the field itself; the - // AdditionalPropertiesField custom renderer reads ui:readonly and - // disables the inner form. - uiNode["ui:readonly"] = true + // additionalProperties object map. Per-value immutability is rendered + // here as whole-map immutability: the field itself is marked disabled, + // AdditionalPropertiesField hides Add/Remove and disables every inner + // input. Splitting "keys editable, values frozen" needs custom plumbing + // through that field plus a UX decision on whether deleting an entry + // counts as mutating its value — deliberately deferred until a real + // schema asks for it. + uiNode["ui:disabled"] = true uiNode["ui:help"] = IMMUTABLE_HELP_TEXT return } @@ -227,8 +239,9 @@ interface SchemaFormProps { children?: React.ReactNode /** * When "enforce", fields whose schema carries a CEL immutability rule - * (`self == oldSelf`) are rendered read-only with helper text. Default - * "off" keeps every field editable — used for create flows. + * (`self == oldSelf`) are rendered greyed out (disabled) with helper + * text. Default "off" keeps every field editable — used for create + * flows. */ immutableMode?: "enforce" | "off" } @@ -241,14 +254,22 @@ export function SchemaForm({ children, immutableMode, }: SchemaFormProps) { - const schema = useMemo(() => { + // Parse the raw schema once, then derive the sanitised RJSFSchema and the + // immutable-path set from it. We keep the raw object around so we don't + // re-parse the same string twice on every render. + const parsedSchema = useMemo(() => { try { - return sanitizeSchema(JSON.parse(openAPISchema)) as RJSFSchema + return JSON.parse(openAPISchema) } catch { - return {} as RJSFSchema + return {} } }, [openAPISchema]) + const schema = useMemo( + () => sanitizeSchema(parsedSchema) as RJSFSchema, + [parsedSchema], + ) + const onChangeRef = useRef(onChange) onChangeRef.current = onChange const initialFormDataRef = useRef(formData) @@ -267,14 +288,10 @@ export function SchemaForm({ // eslint-disable-next-line react-hooks/exhaustive-deps }, [schema]) - const immutablePaths = useMemo(() => { - if (immutableMode !== "enforce") return [] - try { - return findImmutablePaths(JSON.parse(openAPISchema)) - } catch { - return [] - } - }, [openAPISchema, immutableMode]) + const immutablePaths = useMemo( + () => (immutableMode === "enforce" ? findImmutablePaths(parsedSchema) : []), + [parsedSchema, immutableMode], + ) const uiSchema = useMemo(() => { const baseUiSchema: UiSchema = { diff --git a/apps/console/src/lib/immutable-paths.test.ts b/apps/console/src/lib/immutable-paths.test.ts index 87b56bd..c996e6f 100644 --- a/apps/console/src/lib/immutable-paths.test.ts +++ b/apps/console/src/lib/immutable-paths.test.ts @@ -1,4 +1,4 @@ -import { describe, it, expect } from "vitest" +import { describe, it, expect, vi } from "vitest" import { findImmutablePaths, overlayImmutable } from "./immutable-paths.ts" const cel = { @@ -252,4 +252,35 @@ describe("overlayImmutable", () => { expect(submitted).toEqual({ storageClass: "fast", arr: [1, 2] }) expect(original).toEqual({ storageClass: "slow", arr: [9] }) }) + + it("warns and returns a clone of original when the root path is immutable", () => { + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}) + const submitted = { foo: "x", bar: "y" } + const original = { foo: "X", bar: "Y" } + const result = overlayImmutable(submitted, original, [[]]) + expect(result).toEqual(original) + expect(warn).toHaveBeenCalledTimes(1) + expect(warn.mock.calls[0][0]).toMatch(/root-level immutable/i) + warn.mockRestore() + }) + + it("iterates the longer of source/target for wildcard array elements", () => { + const submitted = { + disks: [ + { name: "a", size: "1Gi" }, + { name: "b", size: "2Gi" }, + { name: "c", size: "3Gi" }, + ], + } + const original = { disks: [{ name: "A", size: "9Gi" }] } + expect( + overlayImmutable(submitted, original, [["disks", "*", "name"]]), + ).toEqual({ + disks: [ + { name: "A", size: "1Gi" }, + { name: "b", size: "2Gi" }, + { name: "c", size: "3Gi" }, + ], + }) + }) }) diff --git a/apps/console/src/lib/immutable-paths.ts b/apps/console/src/lib/immutable-paths.ts index a96c60d..e691c2b 100644 --- a/apps/console/src/lib/immutable-paths.ts +++ b/apps/console/src/lib/immutable-paths.ts @@ -77,6 +77,17 @@ export function overlayImmutable( original: T, paths: readonly ImmutablePath[], ): T { + if (paths.some((p) => p.length === 0)) { + // The schema flags the whole subtree as immutable; whatever the user + // changed in the form is dropped on the floor. That's almost always a + // schema-authoring mistake (rule belongs on a property, not the root) + // so make it visible in the console. + console.warn( + "overlayImmutable: a root-level immutable path was supplied; " + + "the submitted value is replaced wholesale by the original.", + ) + return deepClone(original) + } const next = deepClone(submitted) for (const path of paths) { overlayPath(next, original, path, 0) @@ -95,13 +106,27 @@ function overlayPath( } const seg = path[depth] if (seg === "*") { - if (!Array.isArray(source)) { - return Array.isArray(target) ? target : source + const isLast = depth === path.length - 1 + if (isLast) { + // The whole array is immutable: replace with a clone of source so + // the user can't change length or per-element values. + return Array.isArray(source) ? deepClone(source) : target } + // A field nested inside each array element is immutable. Iterate over + // the longer of the two arrays so the user can still add or remove + // entries, but the immutable nested field is copied from source when + // an index exists in both — new entries (no source counterpart) keep + // the user's input verbatim. + const sourceArr = Array.isArray(source) ? source : [] + const targetArr = Array.isArray(target) ? target : [] + const len = Math.max(sourceArr.length, targetArr.length) const out: unknown[] = [] - for (let i = 0; i < source.length; i++) { - const t = Array.isArray(target) ? target[i] : undefined - out[i] = overlayPath(t, source[i], path, depth + 1) + for (let i = 0; i < len; i++) { + if (i >= sourceArr.length) { + out[i] = deepClone(targetArr[i]) + } else { + out[i] = overlayPath(targetArr[i], sourceArr[i], path, depth + 1) + } } return out } diff --git a/apps/console/src/lib/keys-order.ts b/apps/console/src/lib/keys-order.ts index dacddca..aede9f7 100644 --- a/apps/console/src/lib/keys-order.ts +++ b/apps/console/src/lib/keys-order.ts @@ -23,7 +23,9 @@ import type { UiSchema } from "@rjsf/utils" * nesting level so that RJSF renders top-level spec keys, then nested keys, * in the requested order. */ -export function keysOrderToUiSchema(keysOrder: string[][] | undefined): UiSchema { +export function keysOrderToUiSchema( + keysOrder: ReadonlyArray> | undefined, +): UiSchema { if (!keysOrder || keysOrder.length === 0) return {} // Group paths by their parent path. diff --git a/apps/console/src/lib/prepare-update.test.ts b/apps/console/src/lib/prepare-update.test.ts index 059dcf8..56222a0 100644 --- a/apps/console/src/lib/prepare-update.test.ts +++ b/apps/console/src/lib/prepare-update.test.ts @@ -1,4 +1,4 @@ -import { describe, it, expect } from "vitest" +import { describe, it, expect, vi } from "vitest" import { prepareUpdateSpec } from "./prepare-update.ts" const immutableStorageClassSchema = JSON.stringify({ @@ -53,4 +53,22 @@ describe("prepareUpdateSpec", () => { expect(submitted).toEqual({ storageClass: "fast", size: "10Gi" }) expect(initial).toEqual({ storageClass: "slow", size: "5Gi" }) }) + + it("returns submitted unchanged when original is undefined or null", () => { + const submitted = { storageClass: "fast", size: "10Gi" } + expect( + prepareUpdateSpec(submitted, undefined, immutableStorageClassSchema), + ).toEqual(submitted) + expect( + prepareUpdateSpec(submitted, null, immutableStorageClassSchema), + ).toEqual(submitted) + }) + + it("warns to console when the schema cannot be parsed", () => { + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}) + prepareUpdateSpec({ storageClass: "fast" }, { storageClass: "slow" }, "{") + expect(warn).toHaveBeenCalled() + expect(warn.mock.calls[0][0]).toMatch(/openAPISchema/) + warn.mockRestore() + }) }) diff --git a/apps/console/src/lib/prepare-update.ts b/apps/console/src/lib/prepare-update.ts index 73fde14..7ad5efb 100644 --- a/apps/console/src/lib/prepare-update.ts +++ b/apps/console/src/lib/prepare-update.ts @@ -12,17 +12,38 @@ import { * Defence-in-depth: even if the form's read-only state is bypassed (via * the YAML editor, devtools, or a UI bug) the outgoing body still * matches the persisted spec on those paths. + * + * Edge cases: + * - When `original` is null/undefined there is nothing meaningful to + * overlay (typically the resource hasn't loaded yet, or the persisted + * spec was empty). We return the submitted value unchanged rather than + * blanking immutable fields with undefined. + * - When the schema string cannot be parsed we log a warning so the + * misconfiguration is at least visible in DevTools, then return the + * submitted value unchanged. */ export function prepareUpdateSpec( submitted: T, - original: T, + original: T | null | undefined, openAPISchema: string, ): T { + if (original === undefined || original === null) { + return cloneShallowAsDeep(submitted) + } let paths: ImmutablePath[] try { paths = findImmutablePaths(JSON.parse(openAPISchema)) } catch { + console.warn( + "prepareUpdateSpec: failed to parse openAPISchema; " + + "skipping immutable-field overlay.", + ) paths = [] } return overlayImmutable(submitted, original, paths) } + +function cloneShallowAsDeep(value: T): T { + if (value === null || value === undefined) return value + return JSON.parse(JSON.stringify(value)) as T +} From 30a2bfc22a808446558c38d406385d63f30c8301 Mon Sep 17 00:00:00 2001 From: Aleksei Sviridkin Date: Wed, 13 May 2026 01:23:32 +0300 Subject: [PATCH 07/11] fix(console): correct overlayImmutable for maps and shrinking arrays MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Second-pass review surfaced four defects in overlayImmutable that produced data corruption in the PUT body for cases the schema walker already documents as supported. Fix each, pin behaviour with tests. 1. Nested-field overlay with the submitted array shorter than original used to leave undefined slots ("holes"); the new loop walks the target length so deletions stick. Adds are still preserved because the overlay never touches indices past source. 2. additionalProperties map with *-last used to be a no-op (Array.isArray guarded the only branch). Now an object-source replaces target with a deep clone, so YAML-editor / devtools edits cannot smuggle a mutation past the form's read-only state. 3. additionalProperties map with *-not-last used to coerce the map to an empty array. New code handles object source/target separately: iterate shared keys, overlay the immutable nested subfield from source, keep user-added keys and respect user-deleted ones. 4. When the persisted spec has no value at the immutable path, the overlay used to write undefined into the body, silently erasing the user's input. Now keep the submitted value — the form already greys the field out so interactive edits are blocked, and a missing source value isn't an immutability violation to defend against. Also surface two trade-offs reviewers asked about in plain comments: - SchemaForm.parsedSchema useMemo identity (same string ⇒ same object). - ApplicationOrderPage uses mount-time initialSpec; documented as a re-mount-to-refresh trade-off. README gains a one-line 'pnpm test' section to surface the new suite. Assisted-By: Claude Signed-off-by: Aleksei Sviridkin --- README.md | 8 ++ apps/console/src/components/SchemaForm.tsx | 4 +- apps/console/src/lib/immutable-paths.test.ts | 80 +++++++++++++++- apps/console/src/lib/immutable-paths.ts | 91 ++++++++++++++----- .../src/routes/ApplicationOrderPage.tsx | 4 + 5 files changed, 160 insertions(+), 27 deletions(-) diff --git a/README.md b/README.md index 9f103a9..7c3b2e6 100644 --- a/README.md +++ b/README.md @@ -36,3 +36,11 @@ pnpm build ``` The console is built into `apps/console/dist/`. + +## Test + +```sh +pnpm test +``` + +Runs the workspace test suites (vitest + jsdom for the console). diff --git a/apps/console/src/components/SchemaForm.tsx b/apps/console/src/components/SchemaForm.tsx index 021e2e1..4f21b79 100644 --- a/apps/console/src/components/SchemaForm.tsx +++ b/apps/console/src/components/SchemaForm.tsx @@ -256,7 +256,9 @@ export function SchemaForm({ }: SchemaFormProps) { // Parse the raw schema once, then derive the sanitised RJSFSchema and the // immutable-path set from it. We keep the raw object around so we don't - // re-parse the same string twice on every render. + // re-parse the same string twice on every render. The contract is "same + // openAPISchema string ⇒ same parsedSchema reference"; the dependent + // memos rely on that identity to avoid recomputation. const parsedSchema = useMemo(() => { try { return JSON.parse(openAPISchema) diff --git a/apps/console/src/lib/immutable-paths.test.ts b/apps/console/src/lib/immutable-paths.test.ts index c996e6f..a318e7d 100644 --- a/apps/console/src/lib/immutable-paths.test.ts +++ b/apps/console/src/lib/immutable-paths.test.ts @@ -189,7 +189,7 @@ describe("overlayImmutable", () => { }) }) - it("inserts the immutable field if it is missing from submitted", () => { + it("inserts the immutable field if it is missing from submitted but present in original", () => { const submitted = { size: "10Gi" } as Record const original = { size: "5Gi", storageClass: "slow" } expect( @@ -197,6 +197,18 @@ describe("overlayImmutable", () => { ).toEqual({ size: "10Gi", storageClass: "slow" }) }) + it("keeps the submitted value when the immutable field is absent from original", () => { + // Persisted spec doesn't have the field (server-default, status echo gap, + // freshly-loaded resource). overlay must not blank user's input with undefined. + expect( + overlayImmutable( + { storageClass: "fast", size: "10Gi" }, + { size: "5Gi" } as Record, + [["storageClass"]], + ), + ).toEqual({ storageClass: "fast", size: "10Gi" }) + }) + it("aligns array length to original when array element path is immutable", () => { const submitted = { disks: ["x", "y", "z"] } const original = { disks: ["a", "b"] } @@ -236,13 +248,75 @@ describe("overlayImmutable", () => { }) }) - it("tolerates null/undefined values along the path", () => { + it("tolerates null/undefined values along the path without blanking the user input", () => { expect( overlayImmutable({}, { storageClass: "slow" }, [["storageClass"]]), ).toEqual({ storageClass: "slow" }) + // Source has no value at the path: keep target's value rather than + // erasing the field — see "keeps submitted value when … absent from original". expect( overlayImmutable({ storageClass: "fast" }, {}, [["storageClass"]]), - ).toEqual({ storageClass: undefined }) + ).toEqual({ storageClass: "fast" }) + }) + + it("shrinks array as the user requested even when nested elements are immutable", () => { + // User deleted the second disk; nested field "name" is immutable per element. + // Overlay must respect the deletion (no zombie restore from original). + const submitted = { disks: [{ name: "renamed", size: "10Gi" }] } + const original = { + disks: [ + { name: "disk1", size: "5Gi" }, + { name: "disk2", size: "5Gi" }, + ], + } + expect( + overlayImmutable(submitted, original, [["disks", "*", "name"]]), + ).toEqual({ disks: [{ name: "disk1", size: "10Gi" }] }) + }) + + it("freezes a whole additionalProperties map when * is the last segment", () => { + // The schema declares every value in the map immutable. We treat this as + // whole-map immutable: defence-in-depth must reject mutations from the + // YAML editor or devtools, not just the form. + type LabelsObj = { labels: Record } + const submitted: LabelsObj = { + labels: { env: "prod-changed", added: "new" }, + } + const original: LabelsObj = { + labels: { env: "prod", legacy: "v1" }, + } + expect( + overlayImmutable(submitted, original, [["labels", "*"]]), + ).toEqual({ labels: { env: "prod", legacy: "v1" } }) + }) + + it("overlays shared keys when a map value has a nested immutable field", () => { + // path "labels.*.value" — the value object's "value" subkey is immutable. + // Shared keys: nested immutable subkey is restored from original; other + // subkeys keep the user's edit. Keys present only in submitted (added) + // or only in original (deleted) are kept as the user left them. + type Entry = { value: string; note: string } + type LabelsObj = { labels: Record } + const submitted: LabelsObj = { + labels: { + env: { value: "changed", note: "edit" }, + new: { value: "novel", note: "added" }, + }, + } + const original: LabelsObj = { + labels: { + env: { value: "prod", note: "orig" }, + legacy: { value: "v1", note: "old" }, + }, + } + expect( + overlayImmutable(submitted, original, [["labels", "*", "value"]]), + ).toEqual({ + labels: { + env: { value: "prod", note: "edit" }, + new: { value: "novel", note: "added" }, + }, + }) }) it("does not mutate inputs", () => { diff --git a/apps/console/src/lib/immutable-paths.ts b/apps/console/src/lib/immutable-paths.ts index e691c2b..ce8205a 100644 --- a/apps/console/src/lib/immutable-paths.ts +++ b/apps/console/src/lib/immutable-paths.ts @@ -102,33 +102,15 @@ function overlayPath( depth: number, ): unknown { if (depth === path.length) { + // End of the path: copy the source value. If source has nothing at + // this position keep what the user submitted (avoids blanking fields + // that the persisted spec doesn't echo back). + if (source === undefined) return target return deepClone(source) } const seg = path[depth] if (seg === "*") { - const isLast = depth === path.length - 1 - if (isLast) { - // The whole array is immutable: replace with a clone of source so - // the user can't change length or per-element values. - return Array.isArray(source) ? deepClone(source) : target - } - // A field nested inside each array element is immutable. Iterate over - // the longer of the two arrays so the user can still add or remove - // entries, but the immutable nested field is copied from source when - // an index exists in both — new entries (no source counterpart) keep - // the user's input verbatim. - const sourceArr = Array.isArray(source) ? source : [] - const targetArr = Array.isArray(target) ? target : [] - const len = Math.max(sourceArr.length, targetArr.length) - const out: unknown[] = [] - for (let i = 0; i < len; i++) { - if (i >= sourceArr.length) { - out[i] = deepClone(targetArr[i]) - } else { - out[i] = overlayPath(targetArr[i], sourceArr[i], path, depth + 1) - } - } - return out + return overlayWildcard(target, source, path, depth) } const sourceObj = isPlainObject(source) ? source : undefined const sourceVal = sourceObj ? sourceObj[seg] : undefined @@ -136,10 +118,73 @@ function overlayPath( ? (target as Record) : null if (!targetObj) return target + if (sourceVal === undefined && !targetObj.hasOwnProperty(seg)) { + // Neither side has anything at this path; nothing to overlay or insert. + return targetObj + } targetObj[seg] = overlayPath(targetObj[seg], sourceVal, path, depth + 1) return targetObj } +/** + * A wildcard segment represents either "every element of an array" or + * "every entry of an object map" (additionalProperties). The walker emits + * the same "*" symbol for both because the schema decides at runtime + * which kind of node sits at the path; we dispatch on the actual value + * shape here. + * + * Whole-collection immutable (*-last): replace target with a deep clone + * of source, freezing the collection wholesale. Granular immutable + * (*-not-last): iterate the user's collection (so adds and removes + * survive) and overlay the nested immutable subfield from source on + * shared elements/keys. + */ +function overlayWildcard( + target: unknown, + source: unknown, + path: ImmutablePath, + depth: number, +): unknown { + const isLast = depth === path.length - 1 + + if (Array.isArray(source)) { + if (isLast) return deepClone(source) + const targetArr = Array.isArray(target) ? target : [] + const out: unknown[] = [] + for (let i = 0; i < targetArr.length; i++) { + if (i < source.length) { + out[i] = overlayPath(targetArr[i], source[i], path, depth + 1) + } else { + out[i] = deepClone(targetArr[i]) + } + } + return out + } + + if (isPlainObject(source)) { + if (isLast) return deepClone(source) + const targetObj = isPlainObject(target) ? target : {} + const out: Record = {} + const keys = new Set([ + ...Object.keys(targetObj), + ...Object.keys(source), + ]) + for (const k of keys) { + const hasInTarget = Object.prototype.hasOwnProperty.call(targetObj, k) + const hasInSource = Object.prototype.hasOwnProperty.call(source, k) + if (!hasInTarget) continue + if (hasInSource) { + out[k] = overlayPath(targetObj[k], source[k], path, depth + 1) + } else { + out[k] = deepClone(targetObj[k]) + } + } + return out + } + + return target +} + function deepClone(value: T): T { if (value === null || value === undefined) return value if (Array.isArray(value)) return value.map(deepClone) as unknown as T diff --git a/apps/console/src/routes/ApplicationOrderPage.tsx b/apps/console/src/routes/ApplicationOrderPage.tsx index f8b9bad..fefc414 100644 --- a/apps/console/src/routes/ApplicationOrderPage.tsx +++ b/apps/console/src/routes/ApplicationOrderPage.tsx @@ -110,6 +110,10 @@ export function ApplicationOrderPage({ const body = composeResource(ad, tenantNamespace, snap.name, snap.spec) try { if (editMode) { + // initialSpec is a snapshot taken at mount; if the resource is + // mutated externally between mount and Save, the overlay reinstates + // the mount-time value. That's the documented trade-off — re-mount + // to pick up fresh state. body.spec = prepareUpdateSpec>( body.spec ?? {}, (editMode.initialSpec ?? {}) as Record, From 4416ec39a0bb291fef9f2ce3150c7fde35d61e37 Mon Sep 17 00:00:00 2001 From: Aleksei Sviridkin Date: Wed, 13 May 2026 01:38:40 +0300 Subject: [PATCH 08/11] fix(console): close remaining overlay gaps from second review pass - Replace .hasOwnProperty(seg) with Object.prototype.hasOwnProperty.call so no-prototype-builtins doesn't regress. Add a test pinning the "neither side has the field" code path. - Add three pinned-as-broken tests with FIXME notes for shapes not yet supported by the walker/overlay: oneOf/anyOf/allOf branch properties, intermediate-missing-parent during overlay, and index-aligned overlay on user-reordered arrays. These behaviours stay as-is in this PR. - Add a component test pinning the deliberate UI/overlay asymmetry on additionalProperties when only nested values are immutable: the form freezes the whole map for simplicity while overlay enforces per-entry. - Expand sanitizeSchema's doc block to mention x-kubernetes-validations (stripped here, harvested from the raw schema by immutable-paths). - Tighten SchemaForm's walker doc-comment so "raw (sanitised)" is not self-contradicting. - Spy on console.warn inside the malformed-schema prepareUpdateSpec test so the suite doesn't emit log noise. Assisted-By: Claude Signed-off-by: Aleksei Sviridkin --- .../AdditionalPropertiesField.test.tsx | 40 ++++++++++ apps/console/src/components/SchemaForm.tsx | 10 ++- apps/console/src/lib/immutable-paths.test.ts | 78 ++++++++++++++++++- apps/console/src/lib/immutable-paths.ts | 5 +- apps/console/src/lib/keys-order.ts | 5 ++ apps/console/src/lib/prepare-update.test.ts | 2 + 6 files changed, 134 insertions(+), 6 deletions(-) diff --git a/apps/console/src/components/AdditionalPropertiesField.test.tsx b/apps/console/src/components/AdditionalPropertiesField.test.tsx index 6871d6c..52034a3 100644 --- a/apps/console/src/components/AdditionalPropertiesField.test.tsx +++ b/apps/console/src/components/AdditionalPropertiesField.test.tsx @@ -32,6 +32,46 @@ describe("AdditionalPropertiesField immutable", () => { expect(screen.getByRole("button", { name: /remove/i })).toBeInTheDocument() }) + it("treats *-not-last on additionalProperties as whole-map disabled in the UI even though overlay is per-entry", () => { + // Per-value-immutable schema (additionalProperties carries a nested + // CEL rule). The UI deliberately freezes the whole map for simplicity; + // the overlay enforces per-entry semantics. This asymmetry is a known + // trade-off: a YAML-editor bypass that adds a new key will be accepted + // by the overlay, while the form denies the addition. Documented here + // so it isn't mistaken for a UI bug. + const nestedImmutableSchema = JSON.stringify({ + type: "object", + properties: { + labels: { + type: "object", + additionalProperties: { + type: "object", + properties: { + value: { + type: "string", + "x-kubernetes-validations": [ + { rule: "self == oldSelf", message: "value is immutable" }, + ], + }, + }, + }, + }, + }, + }) + render( + , + ) + expect( + screen.queryByPlaceholderText("Enter key name..."), + ).not.toBeInTheDocument() + expect(screen.getByText(IMMUTABLE_HELP_TEXT)).toBeInTheDocument() + }) + it("hides Add/Remove and disables inner fields when the map is immutable in enforce mode", () => { render( { expect(findImmutablePaths(cel)).toEqual([[]]) }) + it("FIXME: does NOT walk into properties beneath oneOf/anyOf/allOf branches", () => { + // The walker recognises *parent* immutability when any branch carries + // the rule, but it does not descend into a branch's `properties` to + // discover per-field immutability inside the branch. CRDs that use + // allOf composition for property merging are not covered. + // Pinned as current behaviour; revisit if a real schema needs it. + const schema = { + type: "object", + allOf: [ + { + properties: { + storageClass: { type: "string", ...cel }, + }, + }, + ], + } + expect(findImmutablePaths(schema)).toEqual([]) + }) + it("collects multiple immutable paths in deterministic order", () => { const schema = { properties: { @@ -319,6 +338,49 @@ describe("overlayImmutable", () => { }) }) + it("FIXME: does NOT materialise an immutable leaf when its ancestor is missing in target", () => { + // Defence-in-depth gap: if the YAML editor strips the parent object + // entirely, overlay cannot reach the leaf to copy the original value. + // Pin behaviour so a future contributor fixing the gap notices the + // test and revises it. + const submitted = { spec: {} } as Record + const original = { + spec: { backup: { storageClass: "slow" } }, + } + const result = overlayImmutable(submitted, original, [ + ["spec", "backup", "storageClass"], + ]) + expect(result).toEqual({ spec: {} }) + }) + + it("FIXME: array reordering by the user with index-aligned overlay re-anchors source values to the new index", () => { + // The overlay walks by index, not by content identity. Reordering an + // immutable array maps source[i] onto target[i]'s new value — which + // for a per-element-name-immutable schema swaps the names back to the + // source order. Pinned as current behaviour. Identity-based matching + // (e.g. by a stable id field) would be a separate change. + const submitted = { + disks: [ + { name: "b", size: "2Gi" }, + { name: "a", size: "1Gi" }, + ], + } + const original = { + disks: [ + { name: "a", size: "1Gi" }, + { name: "b", size: "2Gi" }, + ], + } + expect( + overlayImmutable(submitted, original, [["disks", "*", "name"]]), + ).toEqual({ + disks: [ + { name: "a", size: "2Gi" }, + { name: "b", size: "1Gi" }, + ], + }) + }) + it("does not mutate inputs", () => { const submitted = { storageClass: "fast", arr: [1, 2] } const original = { storageClass: "slow", arr: [9] } @@ -338,7 +400,10 @@ describe("overlayImmutable", () => { warn.mockRestore() }) - it("iterates the longer of source/target for wildcard array elements", () => { + it("keeps user-added array entries with their submitted (non-source-derived) name", () => { + // User added two entries past the original length. The nested + // immutable field has no source counterpart for those new indices, + // so the submitted values must survive unchanged. const submitted = { disks: [ { name: "a", size: "1Gi" }, @@ -357,4 +422,15 @@ describe("overlayImmutable", () => { ], }) }) + + it("does not access prototype methods unsafely when the field is missing on both sides", () => { + // Regression: targetObj.hasOwnProperty was used directly, tripping + // no-prototype-builtins. Cover the path explicitly: neither side has + // the immutable field — overlay must be a no-op without throwing. + const submitted = { other: "x" } as Record + const original = { other: "y" } as Record + expect( + overlayImmutable(submitted, original, [["storageClass"]]), + ).toEqual({ other: "x" }) + }) }) diff --git a/apps/console/src/lib/immutable-paths.ts b/apps/console/src/lib/immutable-paths.ts index ce8205a..949bf5c 100644 --- a/apps/console/src/lib/immutable-paths.ts +++ b/apps/console/src/lib/immutable-paths.ts @@ -118,7 +118,10 @@ function overlayPath( ? (target as Record) : null if (!targetObj) return target - if (sourceVal === undefined && !targetObj.hasOwnProperty(seg)) { + if ( + sourceVal === undefined && + !Object.prototype.hasOwnProperty.call(targetObj, seg) + ) { // Neither side has anything at this path; nothing to overlay or insert. return targetObj } diff --git a/apps/console/src/lib/keys-order.ts b/apps/console/src/lib/keys-order.ts index aede9f7..080f5ea 100644 --- a/apps/console/src/lib/keys-order.ts +++ b/apps/console/src/lib/keys-order.ts @@ -63,6 +63,11 @@ export function keysOrderToUiSchema( * - `x-kubernetes-preserve-unknown-fields: true` on an object without * properties — arbitrary nested values allowed. Left as a "raw" object * (handled via additionalProperties: true). + * - `x-kubernetes-validations` — CEL rules. We drop them here entirely. + * AJV has no CEL evaluator so the rules are pure noise to the form + * validator; immutability is harvested up-front by `findImmutablePaths` + * on the raw schema (see lib/immutable-paths.ts) and surfaced through + * uiSchema, not through AJV. */ // eslint-disable-next-line @typescript-eslint/no-explicit-any export function sanitizeSchema(schema: any): any { diff --git a/apps/console/src/lib/prepare-update.test.ts b/apps/console/src/lib/prepare-update.test.ts index 56222a0..40afffa 100644 --- a/apps/console/src/lib/prepare-update.test.ts +++ b/apps/console/src/lib/prepare-update.test.ts @@ -40,10 +40,12 @@ describe("prepareUpdateSpec", () => { }) it("returns a clone of submitted unchanged when the schema is malformed JSON", () => { + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}) const submitted = { storageClass: "fast" } const initial = { storageClass: "slow" } const result = prepareUpdateSpec(submitted, initial, "not-json") expect(result).toEqual(submitted) + warn.mockRestore() }) it("does not mutate inputs", () => { From 3dffc2ee61452ca200eca2ec386cbd5754e85a57 Mon Sep 17 00:00:00 2001 From: Aleksei Sviridkin Date: Thu, 14 May 2026 15:04:44 +0300 Subject: [PATCH 09/11] fix(console): snapshot BackupResource spec at init + cover whole-array immutable in tests - BackupResourceEditPage now holds the initial resource.spec in a ref alongside the existing form-data init guard. The overlay source for prepareUpdateSpec switches from the live React-Query value to that ref, so a background refetch can't sneak a different immutable value into the PUT after the form has been opened. Mirrors the snapshot contract ApplicationOrderPage already uses via editMode.initialSpec. - Add a SchemaForm test for the *-as-last array shape (every element of a 'tags' array is immutable). The branch was reachable via applyImmutablePath but only exercised by overlay unit tests; the new test pins the RJSF rendering side too. Assisted-By: Claude Signed-off-by: Aleksei Sviridkin --- .../src/components/SchemaForm.test.tsx | 31 +++++++++++++++++++ .../src/routes/BackupResourceEditPage.tsx | 8 ++++- 2 files changed, 38 insertions(+), 1 deletion(-) diff --git a/apps/console/src/components/SchemaForm.test.tsx b/apps/console/src/components/SchemaForm.test.tsx index 67a4590..327649d 100644 --- a/apps/console/src/components/SchemaForm.test.tsx +++ b/apps/console/src/components/SchemaForm.test.tsx @@ -66,6 +66,37 @@ describe("SchemaForm immutableMode", () => { expect(screen.queryByText(IMMUTABLE_HELP_TEXT)).not.toBeInTheDocument() }) + it("greys out every element of a whole-array immutable field", () => { + // *-as-last: the items schema itself carries the rule, so the entire + // array is immutable. RJSF receives ui:disabled on `items` and + // disables each element's input. + const wholeArraySchema = JSON.stringify({ + type: "object", + properties: { + tags: { + type: "array", + items: { + type: "string", + "x-kubernetes-validations": [ + { rule: "self == oldSelf", message: "tags is immutable" }, + ], + }, + }, + }, + }) + render( + , + ) + expect(screen.getByDisplayValue("alpha")).toBeDisabled() + expect(screen.getByDisplayValue("beta")).toBeDisabled() + expect(screen.getAllByText(IMMUTABLE_HELP_TEXT).length).toBeGreaterThan(0) + }) + it("greys out immutable nested fields inside array items", () => { const arraySchema = JSON.stringify({ type: "object", diff --git a/apps/console/src/routes/BackupResourceEditPage.tsx b/apps/console/src/routes/BackupResourceEditPage.tsx index ab130c8..a68f6df 100644 --- a/apps/console/src/routes/BackupResourceEditPage.tsx +++ b/apps/console/src/routes/BackupResourceEditPage.tsx @@ -24,6 +24,11 @@ export function BackupResourceEditPage({ const { tenantNamespace } = useTenantContext() const [formData, setFormData] = useState({}) const initializedRef = useRef(false) + // Snapshot of resource.spec at the moment the form initialised. Used as + // the source for the immutable-field overlay so the value the user saw + // in the form is the value that goes into the PUT, regardless of any + // React-Query refetches in between. + const initialSpecRef = useRef(null) // Map resourceType to CRD name const crdNameMap = { @@ -62,6 +67,7 @@ export function BackupResourceEditPage({ if (resource?.spec && !initializedRef.current) { initializedRef.current = true setFormData(resource.spec) + initialSpecRef.current = resource.spec } }, [resource]) @@ -70,7 +76,7 @@ export function BackupResourceEditPage({ const updated = { ...resource, - spec: prepareUpdateSpec(formData, resource.spec, schema ?? ""), + spec: prepareUpdateSpec(formData, initialSpecRef.current, schema ?? ""), } try { From a815790018cd750d1f5b6c615c2cae79881adbad Mon Sep 17 00:00:00 2001 From: Aleksei Sviridkin Date: Thu, 14 May 2026 15:17:16 +0300 Subject: [PATCH 10/11] fix(console): bail on shrink for nested-immutable arrays, handle root wildcard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two concrete data-corruption shapes surfaced in review: 1. When the user shrinks an array with a per-element-nested-immutable path (e.g. disks[].name), index-aligned overlay re-anchors source names onto the surviving entries — silently changing which element was deleted. We now detect length shrink at the wildcard segment and skip the overlay for that path with a console.warn. The UI's ui:disabled still prevents interactive edits; admission stays as the authoritative enforcer for YAML/devtools bypass. 2. A top-level wildcard immutable path (root array or map carrying the rule on items/additionalProperties) used to no-op silently — the overlay loop discarded the wildcard return value. Mirror the root-immutable handling: warn and replace wholesale. Existing tests for grow and same-length nested overlay still pass; the shrink and root-wildcard cases get explicit regression tests. Link the three remaining known-broken behaviours from FIXME tests to the tracking issues filed for them (#8, #9, #10). Assisted-By: Claude Signed-off-by: Aleksei Sviridkin --- apps/console/src/lib/immutable-paths.test.ts | 50 ++++++++++++++--- apps/console/src/lib/immutable-paths.ts | 58 ++++++++++++++++++++ 2 files changed, 101 insertions(+), 7 deletions(-) diff --git a/apps/console/src/lib/immutable-paths.test.ts b/apps/console/src/lib/immutable-paths.test.ts index dad6dd6..b38a5c5 100644 --- a/apps/console/src/lib/immutable-paths.test.ts +++ b/apps/console/src/lib/immutable-paths.test.ts @@ -140,7 +140,7 @@ describe("findImmutablePaths", () => { expect(findImmutablePaths(cel)).toEqual([[]]) }) - it("FIXME: does NOT walk into properties beneath oneOf/anyOf/allOf branches", () => { + it("FIXME(#8): does NOT walk into properties beneath oneOf/anyOf/allOf branches", () => { // The walker recognises *parent* immutability when any branch carries // the rule, but it does not descend into a branch's `properties` to // discover per-field immutability inside the branch. CRDs that use @@ -278,9 +278,11 @@ describe("overlayImmutable", () => { ).toEqual({ storageClass: "fast" }) }) - it("shrinks array as the user requested even when nested elements are immutable", () => { - // User deleted the second disk; nested field "name" is immutable per element. - // Overlay must respect the deletion (no zombie restore from original). + it("skips overlay (with warn) when the user shrunk an array with per-element-nested-immutable fields", () => { + // Index-aligned overlay corrupts which element was deleted when the + // length changed (it re-anchors source values onto surviving indices). + // Skip the overlay for this path and let admission enforce the rule. + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}) const submitted = { disks: [{ name: "renamed", size: "10Gi" }] } const original = { disks: [ @@ -290,7 +292,41 @@ describe("overlayImmutable", () => { } expect( overlayImmutable(submitted, original, [["disks", "*", "name"]]), - ).toEqual({ disks: [{ name: "disk1", size: "10Gi" }] }) + ).toEqual({ disks: [{ name: "renamed", size: "10Gi" }] }) + expect(warn).toHaveBeenCalled() + warn.mockRestore() + }) + + it("skips overlay (with warn) when the user deleted the first of two immutable-named entries", () => { + // Direct repro of the index-anchored swap bug discovered in review: + // submitted shrinks from 2 to 1 and the naive overlay would assign + // source[0].name to the surviving entry. We bail instead. + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}) + const submitted = { disks: [{ name: "b", size: "2Gi" }] } + const original = { + disks: [ + { name: "a", size: "1Gi" }, + { name: "b", size: "2Gi" }, + ], + } + expect( + overlayImmutable(submitted, original, [["disks", "*", "name"]]), + ).toEqual({ disks: [{ name: "b", size: "2Gi" }] }) + expect(warn).toHaveBeenCalled() + warn.mockRestore() + }) + + it("warns and clones original when a top-level wildcard immutable path is supplied", () => { + // findImmutablePaths emits [["*"]] for schemas whose root is an array + // or map carrying the rule on items/additionalProperties. The naive + // overlay would no-op (the loop's return value was discarded). Match + // the root-immutable handling: warn and replace wholesale. + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}) + const submitted = ["x", "y"] + const original = ["a", "b"] + expect(overlayImmutable(submitted, original, [["*"]])).toEqual(original) + expect(warn).toHaveBeenCalled() + warn.mockRestore() }) it("freezes a whole additionalProperties map when * is the last segment", () => { @@ -338,7 +374,7 @@ describe("overlayImmutable", () => { }) }) - it("FIXME: does NOT materialise an immutable leaf when its ancestor is missing in target", () => { + it("FIXME(#9): does NOT materialise an immutable leaf when its ancestor is missing in target", () => { // Defence-in-depth gap: if the YAML editor strips the parent object // entirely, overlay cannot reach the leaf to copy the original value. // Pin behaviour so a future contributor fixing the gap notices the @@ -353,7 +389,7 @@ describe("overlayImmutable", () => { expect(result).toEqual({ spec: {} }) }) - it("FIXME: array reordering by the user with index-aligned overlay re-anchors source values to the new index", () => { + it("FIXME(#10): array reordering by the user with index-aligned overlay re-anchors source values to the new index", () => { // The overlay walks by index, not by content identity. Reordering an // immutable array maps source[i] onto target[i]'s new value — which // for a per-element-name-immutable schema swaps the names back to the diff --git a/apps/console/src/lib/immutable-paths.ts b/apps/console/src/lib/immutable-paths.ts index 949bf5c..58c02ae 100644 --- a/apps/console/src/lib/immutable-paths.ts +++ b/apps/console/src/lib/immutable-paths.ts @@ -88,13 +88,71 @@ export function overlayImmutable( ) return deepClone(original) } + if (paths.some((p) => p[0] === "*")) { + // A leading wildcard would land at the very top of the structure and + // overlay nothing useful (the root is an array or map, not an object + // we can deepen into). Mirror the root-immutable handling: warn and + // hand back a clone of original. + console.warn( + "overlayImmutable: a top-level wildcard immutable path was supplied; " + + "the submitted value is replaced wholesale by the original.", + ) + return deepClone(original) + } const next = deepClone(submitted) for (const path of paths) { + if (wildcardArrayLengthChanged(next, original, path)) { + // Index-aligned overlay corrupts the user's deletion/insertion when + // a per-element-nested-immutable path crosses an array whose length + // changed. The UI greys those fields out, so the only ways to land + // here are the YAML editor or devtools — the API server will reject + // a per-element mutation via CEL. Skip the overlay (warn for diag) + // rather than silently re-anchor source values to user indices. + console.warn( + "overlayImmutable: array length changed at wildcard segment of", + path.join("."), + "— skipping overlay for this path; admission will enforce.", + ) + continue + } overlayPath(next, original, path, 0) } return next } +function wildcardArrayLengthChanged( + submitted: unknown, + original: unknown, + path: ImmutablePath, +): boolean { + let subCur: unknown = submitted + let origCur: unknown = original + for (let i = 0; i < path.length; i++) { + const seg = path[i] + if (seg === "*") { + // Whole-array immutability (*-last) is fine to overlay even when the + // user changed the length — the semantics is "freeze, replace from + // source", and that's exactly what the *-last branch does. + const isLast = i === path.length - 1 + if (isLast) return false + // Per-element-nested immutability (*-not-last) is the case that can + // silently corrupt on SHRINK (we'd re-anchor source values onto the + // user's surviving indices, which may belong to a different element + // than the user thought they kept). On grow the shared indices stay + // put and the new entries past source.length keep the user's values, + // so growth is safe to overlay. + if (Array.isArray(subCur) && Array.isArray(origCur)) { + return subCur.length < origCur.length + } + return false + } + if (!isPlainObject(subCur) || !isPlainObject(origCur)) return false + subCur = (subCur as Record)[seg] + origCur = (origCur as Record)[seg] + } + return false +} + function overlayPath( target: unknown, source: unknown, From af926e2b68fbdfcea88ce29b7ad4a4125aa4f789 Mon Sep 17 00:00:00 2001 From: Aleksei Sviridkin Date: Thu, 14 May 2026 15:38:00 +0300 Subject: [PATCH 11/11] fix(console): snapshot ApplicationOrderPage initialSpec, align map overlay with UI MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Second-pass review surfaced three more defects: 1. ApplicationOrderPage read editMode.initialSpec live at submit time. The route reconstructs editMode on every React-Query refetch, so a background refetch could sneak a different immutable value into the PUT. Capture initialSpec into a ref at the first render where editMode is set and read the ref in submit. Mirrors what BackupResourceEditPage already does. 2. additionalProperties asymmetry: UI marked the whole map ui:disabled while overlay merged per-entry, letting a YAML-editor bypass add keys the form refused. Bring overlay in line with UI — freeze the whole map regardless of whether * is the last segment. Updated the corresponding test to assert added keys are dropped and removed keys reinstated. 3. SchemaForm's onChangeRef sync mutated a ref during render (react-hooks/refs). Move the assignment into an effect; the default-emit effect still sees the latest callback via the ref's .current property. Also drop a now-unused eslint-disable directive on the default-emit effect now that its deps are tight. Assisted-By: Claude Signed-off-by: Aleksei Sviridkin --- apps/console/src/components/SchemaForm.tsx | 10 ++++++-- apps/console/src/lib/immutable-paths.test.ts | 16 ++++++------ apps/console/src/lib/immutable-paths.ts | 25 ++++++------------- .../src/routes/ApplicationOrderPage.tsx | 22 +++++++++++----- 4 files changed, 40 insertions(+), 33 deletions(-) diff --git a/apps/console/src/components/SchemaForm.tsx b/apps/console/src/components/SchemaForm.tsx index 594f2cb..e632b48 100644 --- a/apps/console/src/components/SchemaForm.tsx +++ b/apps/console/src/components/SchemaForm.tsx @@ -275,10 +275,17 @@ export function SchemaForm({ ) const onChangeRef = useRef(onChange) - onChangeRef.current = onChange const initialFormDataRef = useRef(formData) const emittedSchemaRef = useRef(null) + // Sync the latest onChange into the ref via an effect so we don't mutate + // refs during render (react-hooks/refs). The default-emit effect below + // reads onChangeRef.current and therefore always sees the latest callback + // — even though that effect only depends on `schema`. + useEffect(() => { + onChangeRef.current = onChange + }, [onChange]) + // Emit defaults to parent once per schema so spec is never empty on first submit. // Uses initialFormDataRef so edit-mode existing values are preserved as base. // emittedSchemaRef prevents re-running on unrelated re-renders and avoids @@ -289,7 +296,6 @@ export function SchemaForm({ emittedSchemaRef.current = schema const defaults = getDefaultFormState(validator, schema, initialFormDataRef.current ?? {}, schema) onChangeRef.current(defaults) - // eslint-disable-next-line react-hooks/exhaustive-deps }, [schema]) const immutablePaths = useMemo( diff --git a/apps/console/src/lib/immutable-paths.test.ts b/apps/console/src/lib/immutable-paths.test.ts index b38a5c5..a423d5b 100644 --- a/apps/console/src/lib/immutable-paths.test.ts +++ b/apps/console/src/lib/immutable-paths.test.ts @@ -345,11 +345,13 @@ describe("overlayImmutable", () => { ).toEqual({ labels: { env: "prod", legacy: "v1" } }) }) - it("overlays shared keys when a map value has a nested immutable field", () => { - // path "labels.*.value" — the value object's "value" subkey is immutable. - // Shared keys: nested immutable subkey is restored from original; other - // subkeys keep the user's edit. Keys present only in submitted (added) - // or only in original (deleted) are kept as the user left them. + it("freezes the whole map even when only a nested value subkey carries the rule", () => { + // Previously the overlay merged per-entry for *-not-last on a map, + // which let a YAML-editor bypass add new keys that the form would + // have refused. The UI marks the whole map ui:disabled; the overlay + // now matches: drop added keys, reinstate removed keys, reset every + // value to source. Different-content semantics (allow add) would need + // both sides to change together — see PR description's UX trade-off. type Entry = { value: string; note: string } type LabelsObj = { labels: Record } const submitted: LabelsObj = { @@ -368,8 +370,8 @@ describe("overlayImmutable", () => { overlayImmutable(submitted, original, [["labels", "*", "value"]]), ).toEqual({ labels: { - env: { value: "prod", note: "edit" }, - new: { value: "novel", note: "added" }, + env: { value: "prod", note: "orig" }, + legacy: { value: "v1", note: "old" }, }, }) }) diff --git a/apps/console/src/lib/immutable-paths.ts b/apps/console/src/lib/immutable-paths.ts index 58c02ae..34acc4b 100644 --- a/apps/console/src/lib/immutable-paths.ts +++ b/apps/console/src/lib/immutable-paths.ts @@ -223,24 +223,13 @@ function overlayWildcard( } if (isPlainObject(source)) { - if (isLast) return deepClone(source) - const targetObj = isPlainObject(target) ? target : {} - const out: Record = {} - const keys = new Set([ - ...Object.keys(targetObj), - ...Object.keys(source), - ]) - for (const k of keys) { - const hasInTarget = Object.prototype.hasOwnProperty.call(targetObj, k) - const hasInSource = Object.prototype.hasOwnProperty.call(source, k) - if (!hasInTarget) continue - if (hasInSource) { - out[k] = overlayPath(targetObj[k], source[k], path, depth + 1) - } else { - out[k] = deepClone(targetObj[k]) - } - } - return out + // For additionalProperties (object maps) we freeze the whole map + // regardless of whether * is last or not. The UI marks the field + // ui:disabled and hides Add/Remove, so this overlay aligns the + // defence-in-depth (YAML editor / devtools) with what the form + // already enforces — added keys are dropped, removed keys are + // reinstated, every value is reset to source. + return deepClone(source) } return target diff --git a/apps/console/src/routes/ApplicationOrderPage.tsx b/apps/console/src/routes/ApplicationOrderPage.tsx index fefc414..2e45620 100644 --- a/apps/console/src/routes/ApplicationOrderPage.tsx +++ b/apps/console/src/routes/ApplicationOrderPage.tsx @@ -1,4 +1,4 @@ -import { useMemo, useState } from "react" +import { useMemo, useRef, useState } from "react" import { useNavigate, useParams, useSearchParams } from "react-router" import { ChevronLeft, FileCode, FormInput } from "lucide-react" import yaml from "js-yaml" @@ -41,6 +41,16 @@ export function ApplicationOrderPage({ const [mode, setMode] = useState("form") const [yamlText, setYamlText] = useState("") const [yamlError, setYamlError] = useState(null) + // Snapshot of the persisted spec captured the first time we see an + // editMode prop. ApplicationEditRoute reconstructs `editMode` on every + // React-Query refetch, so reading `editMode.initialSpec` at save time + // would pick up whatever the cluster has right now, not what the user + // saw when they opened the form. The ref locks the overlay source to + // the value the user actually viewed. + const initialSpecRef = useRef(editMode?.initialSpec) + if (editMode && initialSpecRef.current === undefined) { + initialSpecRef.current = editMode.initialSpec + } const plural = ad?.spec?.application.plural ?? "" @@ -110,13 +120,13 @@ export function ApplicationOrderPage({ const body = composeResource(ad, tenantNamespace, snap.name, snap.spec) try { if (editMode) { - // initialSpec is a snapshot taken at mount; if the resource is - // mutated externally between mount and Save, the overlay reinstates - // the mount-time value. That's the documented trade-off — re-mount - // to pick up fresh state. + // initialSpecRef holds the value the user saw when the form + // opened; if the resource is mutated externally between mount and + // Save, the overlay reinstates the mount-time value. That's the + // documented trade-off — re-mount to pick up fresh state. body.spec = prepareUpdateSpec>( body.spec ?? {}, - (editMode.initialSpec ?? {}) as Record, + (initialSpecRef.current ?? {}) as Record, ad.spec?.application.openAPISchema ?? "", ) await update.mutateAsync(body)