From 86ba42633cd4cb44d3f7edaa1972de2650da1099 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 15 Aug 2026 19:11:49 -0400 Subject: [PATCH] fix: honor sibling constraints on nullability, guard numeric item enums MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot round 6: no visible comments, three suppressed. All valid. 1. JSON Schema keywords at one level are conjunctive, so a syntactic null does not by itself mean the schema accepts null. `{ type: ["string", "null"], enum: ["envio"] }` names "null" and still rejects it — yet the collapse marked it `nullable: true`, giving the field a clear button that emits a value the schema forbids, and `admitsNull` let required-field gating accept it. Both now consult `nullExcludedBySiblings`. The distinction that makes this safe: a *branch's* `enum` is not a sibling. In `anyOf: [{ type: "string", enum: [...] }, { type: "null" }]` — the #1928 shape — the enum constrains that branch alone and the union still permits null, so `nullable` is read off the ORIGINAL schema rather than the hoisted merge. Reading the merge would have called the headline case non-nullable. Flattening is unaffected either way; only the nullability claim is withheld. 2. The collapse newly routed a nullable array of numeric item enums into the string-only `MultiSelect` path, which would submit `["1"]` for `[1]`. Gated on `isStringEnum`, matching `toConstOptions`; `isStringEnum` is now a type predicate so the narrowing is real rather than asserted. 3. Invalid JSON and an empty field both report `undefined`, so an optional complex argument could be dropped from the submission while invalid text sat visible in the box. The field now says so via `error`. Note a *required* field was already gated — its value is `undefined`, which `hasMissingRequiredFields` treats as missing — so this is specifically the optional case. Disabling Execute/Open/Submit on an invalid optional draft needs a validity channel out of `SchemaForm` into all four call sites, which is wider than this fix; filed separately. Signed-off-by: cliffhall --- .../groups/SchemaForm/SchemaForm.test.tsx | 85 +++++++++++++++++++ .../groups/SchemaForm/SchemaForm.tsx | 41 +++++++-- .../web/src/test/core/nullableUnion.test.ts | 81 +++++++++++++++--- core/json/nullableUnion.ts | 37 +++++++- 4 files changed, 220 insertions(+), 24 deletions(-) diff --git a/clients/web/src/components/groups/SchemaForm/SchemaForm.test.tsx b/clients/web/src/components/groups/SchemaForm/SchemaForm.test.tsx index bc6b75c9..c2392ec8 100644 --- a/clients/web/src/components/groups/SchemaForm/SchemaForm.test.tsx +++ b/clients/web/src/components/groups/SchemaForm/SchemaForm.test.tsx @@ -744,6 +744,45 @@ describe("SchemaForm", () => { // original symptom, and it lived here rather than in the dispatch. It matters // more since #2007, whose fix deliberately routes object unions to this // editor: a fallback nobody can type into is not a fallback. + it("shows an error while the JSON draft does not parse", async () => { + const user = userEvent.setup(); + const schema: InspectorFormSchema = { + type: "object", + properties: { + config: { type: "array", title: "Config" }, + }, + }; + function Harness() { + const [values, setValues] = useState>({}); + return ( + + ); + } + renderWithMantine(); + + const jsonInput = screen.getByLabelText(/Config/) as HTMLTextAreaElement; + await user.type(jsonInput, "[[1,"); + // Invalid text yields no value, so without this the field would submit as + // absent while the user is still looking at what they typed. + expect(screen.getByText(/Not valid JSON/)).toBeInTheDocument(); + + await user.type(jsonInput, "2]"); + expect(screen.queryByText(/Not valid JSON/)).not.toBeInTheDocument(); + }); + + it("shows no error for an empty optional field", () => { + const schema: InspectorFormSchema = { + type: "object", + properties: { + config: { type: "array", title: "Config" }, + }, + }; + renderWithMantine( + , + ); + expect(screen.queryByText(/Not valid JSON/)).not.toBeInTheDocument(); + }); + it("reports no value, not raw text, while the JSON is mid-edit", async () => { const user = userEvent.setup(); const onChange = vi.fn(); @@ -1183,6 +1222,52 @@ describe("SchemaForm nullable unions", () => { expect(screen.getByRole("textbox", { name: "Items" })).toBeInTheDocument(); }); + // Reachable only since the collapse landed: the nullable wrapper had no + // top-level `type` before, so it fell to the JSON editor rather than into a + // string-valued MultiSelect that would submit ["1"] for [1]. + it("falls back to the JSON input for a nullable array of numeric item enums", () => { + // Wire data again — a numeric `enum` is valid JSON Schema but outside + // `InspectorFormSchema`'s `string[]`, so it comes in through the narrow. + const schema = toFormSchema({ + type: "object", + properties: { + levels: { + title: "Levels", + anyOf: [{ type: "array", items: { enum: [1, 2] } }, { type: "null" }], + }, + }, + }); + renderWithMantine( + , + ); + expect(screen.getByLabelText(/Levels/).tagName).toBe("TEXTAREA"); + }); + + // The sibling enum rules null out even though the type list names it, so the + // field must not get a clear button that emits a value the schema rejects. + it("offers no clear button when a sibling enum excludes null", () => { + const schema = toFormSchema({ + type: "object", + properties: { + direction: { + title: "Direction", + type: ["string", "null"], + enum: ["envio", "recebimento"], + }, + }, + }); + renderWithMantine( + , + ); + expect( + screen.queryByRole("button", { hidden: true }), + ).not.toBeInTheDocument(); + }); + it("still falls back to the JSON input for a union of two real types", () => { const schema: InspectorFormSchema = { type: "object", diff --git a/clients/web/src/components/groups/SchemaForm/SchemaForm.tsx b/clients/web/src/components/groups/SchemaForm/SchemaForm.tsx index fc64c218..89a88c23 100644 --- a/clients/web/src/components/groups/SchemaForm/SchemaForm.tsx +++ b/clients/web/src/components/groups/SchemaForm/SchemaForm.tsx @@ -15,7 +15,10 @@ import type { InspectorFormSchema, JsonSchemaConst, } from "../../../utils/jsonUtils"; -import { normalizeNullableUnion } from "@inspector/core/json/nullableUnion.js"; +import { + isStringEnum, + normalizeNullableUnion, +} from "@inspector/core/json/nullableUnion.js"; const FieldLabel = Text.withProps({ fw: 500, @@ -196,6 +199,15 @@ interface SchemaJsonFieldProps { * `undefined` rather than handed the text. An inspector must not report a value * it cannot send, and `undefined` is how "no value supplied" is spelled * everywhere else in this form. + * + * That leaves invalid text and an empty field indistinguishable *to the + * parent*, so the field says so itself via `error`. A **required** field is + * already gated — its value is `undefined`, which `hasMissingRequiredFields` + * treats as missing — but an **optional** one would otherwise be dropped from + * the submission with no indication. Disabling Execute/Open/Submit on an + * invalid optional draft needs a validity channel out of this component and + * into all four callers, which is a wider change than this fix; tracked + * separately. */ function SchemaJsonField({ value, @@ -216,10 +228,23 @@ function SchemaJsonField({ } }); + // Text that is present but does not parse yields no value, so the field would + // otherwise submit as absent while the user is still looking at what they + // typed. Saying so on the field is what keeps that from being silent; see the + // note on `SchemaJsonField` for the submit-gating half, which is not local to + // this component. + const hasInvalidDraft = + draft.trim() !== "" && parseJsonDraft(draft) === undefined; + return ( { setDraft(text); onChange(parseJsonDraft(text)); @@ -458,12 +483,14 @@ export function SchemaForm({ ); } - // array of enum values (multi-select) - if (fieldSchema.type === "array" && fieldSchema.items?.enum) { - const data = toEnumData( - fieldSchema.items.enum, - fieldSchema.items.enumNames, - ); + // array of enum values (multi-select). Gated on the members being strings + // for the same reason `toConstOptions` is: `MultiSelect` is string-valued, + // so `items: { enum: [1, 2] }` would submit `["1"]` where the schema says + // `[1]`. Reachable from a nullable array since the collapse landed, which + // is what makes the guard load-bearing rather than theoretical. + const itemsEnum = fieldSchema.items?.enum; + if (fieldSchema.type === "array" && isStringEnum(itemsEnum)) { + const data = toEnumData(itemsEnum, fieldSchema.items?.enumNames); return ( { }); }); + // Flattening still happens — only the nullability claim is withheld, because + // the sibling enum rules null out even though the type list names it. + it("flattens but does not mark nullable when a sibling enum excludes null", () => { + expect( + normalizeNullableUnion({ + type: ["string", "null"], + enum: ["envio", "recebimento"], + }), + ).toEqual({ + type: "string", + enum: ["envio", "recebimento"], + anyOf: undefined, + nullable: false, + }); + }); + + it("still marks nullable when the enum lives on an anyOf branch", () => { + expect( + normalizeNullableUnion({ + anyOf: [{ type: "string", enum: ["envio"] }, { type: "null" }], + }), + ).toEqual({ + type: "string", + enum: ["envio"], + anyOf: undefined, + nullable: true, + }); + }); + it("leaves a mismatched enumNames alone rather than inventing an alignment", () => { expect( normalizeNullableUnion({ @@ -328,20 +357,6 @@ describe("normalizeNullableUnion", () => { }); }); - it("leaves a null-free enum untouched", () => { - expect( - normalizeNullableUnion({ - type: ["string", "null"], - enum: ["a", "b"], - }), - ).toEqual({ - type: "string", - enum: ["a", "b"], - anyOf: undefined, - nullable: true, - }); - }); - it("normalizes type array array|null", () => { expect(normalizeNullableUnion({ type: ["array", "null"] })).toEqual({ type: "array", @@ -424,4 +439,42 @@ describe("admitsNull", () => { it("ignores non-object anyOf members instead of throwing", () => { expect(admitsNull({ anyOf: ["null", ["null"], 7] })).toBe(false); }); + + // JSON Schema keywords at one level are conjunctive, so a syntactic null does + // not by itself mean the schema accepts null. `{ type: ["string","null"], + // enum: ["envio"] }` names "null" and still rejects it. + it("is false when a sibling enum excludes null", () => { + expect(admitsNull({ type: ["string", "null"], enum: ["envio"] })).toBe( + false, + ); + expect( + admitsNull({ + anyOf: [{ type: "string" }, { type: "null" }], + enum: ["a"], + }), + ).toBe(false); + }); + + it("is true when a sibling enum offers null", () => { + expect( + admitsNull({ type: ["string", "null"], enum: ["envio", null] }), + ).toBe(true); + }); + + it("is false when a sibling const pins a non-null value", () => { + expect(admitsNull({ type: ["string", "null"], const: "envio" })).toBe( + false, + ); + expect(admitsNull({ type: ["string", "null"], const: null })).toBe(true); + }); + + // A branch's enum is scoped to that branch, not a sibling of the union — this + // is the #1928 shape, and reading it as a sibling would call it non-nullable. + it("is unaffected by an enum that lives inside an anyOf branch", () => { + expect( + admitsNull({ + anyOf: [{ type: "string", enum: ["envio"] }, { type: "null" }], + }), + ).toBe(true); + }); }); diff --git a/core/json/nullableUnion.ts b/core/json/nullableUnion.ts index 1b5c9f9f..6141bcdd 100644 --- a/core/json/nullableUnion.ts +++ b/core/json/nullableUnion.ts @@ -43,9 +43,10 @@ export interface NullableUnionSchema { enum?: unknown[]; anyOf?: readonly unknown[]; // Read only by `admitsNull`, which considers encodings the collapse itself - // declines to flatten. + // declines to flatten, and the sibling constraints that can rule null out. oneOf?: readonly unknown[]; nullable?: boolean; + const?: unknown; } /** Narrow an `anyOf` member to a readable object, or `null` if it isn't one. */ @@ -71,7 +72,7 @@ function toBranch(value: unknown): Record | null { * an enum to a select at all. The TUI's does, and its options are stringified, * so an unguarded numeric enum there submits `"1"` where the server wants `1`. */ -export function isStringEnum(value: unknown): boolean { +export function isStringEnum(value: unknown): value is string[] { return ( Array.isArray(value) && value.length > 0 && @@ -101,6 +102,30 @@ export type NormalizedNullableUnion = Omit< nullable?: boolean; }; +/** + * Whether a schema's **own** `enum`/`const` rules `null` out, regardless of what + * its `type` or union branches say. + * + * JSON Schema keywords at one level are **conjunctive** — a value must satisfy + * all of them — so a syntactic null does not by itself mean the schema accepts + * null. `{ type: ["string", "null"], enum: ["envio"] }` names `"null"` in its + * type list and still rejects `null`, because the `enum` does not offer it. + * Marking that field nullable would give it a clear button that emits a value + * the schema forbids, and would make required-field gating accept it. + * + * Evaluated against the schema's own level only. A branch's `enum` is *not* a + * sibling — in `anyOf: [{ type: "string", enum: [...] }, { type: "null" }]` the + * enum constrains that branch alone, and the union still permits null. Reading + * it as a sibling would break exactly the shape #1928 is about, so callers must + * pass the original schema here, never the hoisted merge. + */ +function nullExcludedBySiblings(schema: NullableUnionSchema): boolean { + if (Array.isArray(schema.enum) && !schema.enum.includes(null)) { + return true; + } + return schema.const !== undefined && schema.const !== null; +} + /** * What {@link stripNullEnumMembers} concluded about an `enum`. * @@ -192,7 +217,10 @@ function collapsed( ...merged, type, anyOf: undefined, - nullable: true, + // Read off the ORIGINAL schema, not the merge: a hoisted branch `enum` is + // not a sibling constraint, and treating it as one would mark the very + // shape #1928 is about as non-nullable. See `nullExcludedBySiblings`. + nullable: admitsNull(schema), ...(strip.kind === "filtered" ? { enum: strip.members, enumNames: strip.names } : {}), @@ -216,6 +244,9 @@ function collapsed( * branch anywhere in an `anyOf` or `oneOf` of any size. */ export function admitsNull(schema: NullableUnionSchema): boolean { + if (nullExcludedBySiblings(schema)) { + return false; + } if (schema.nullable === true) { return true; }