From e4e16bb3e6783a7ca08e9072e4c899cd33b6544c Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 15 Aug 2026 17:24:46 -0400 Subject: [PATCH] fix: address Copilot review on the nullable-union collapse MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four findings, all valid: 1. Returning `T` was unsound. The collapse genuinely changes two fields — `type` stops being an array, `anyOf` becomes `undefined` — so a caller whose `T` typed either that way could keep treating it so afterwards. The result is now `T | NormalizedNullableUnion`, which states exactly that: `Omit` plus a `RenderableType` `type`, a cleared `anyOf`, and `nullable`. The one assertion left is isolated in a `collapsed()` helper, needed only because `Omit` stays deferred while `T` is open. 2. A typeless `enum` does not imply strings. JSON Schema allows `enum: [1, 2]`, and inferring `"string"` for it would hand numbers to a `Select` declared `string[]`, and make the TUI submit `"1"` for `1`. Now only an all-string enum earns the inference; anything else stays on the JSON fallback. An explicit `type` is still authoritative. 3. `hasMissingRequiredFields` counted every null as missing, so clearing a *required* nullable enum — newly possible via the clear button — would disable submit on a value the schema permits. `required` constrains presence, not content, so null now counts as present exactly when the field's schema admits it. 4. `collectSchemaDefaults` read the raw union, so a default nested inside a nullable object's branch was displayed by the form but never seeded into the submitted values. It normalizes each property first, like the renderer does. Signed-off-by: cliffhall --- .../web/src/test/core/nullableUnion.test.ts | 33 +++++++ clients/web/src/utils/jsonUtils.test.ts | 76 +++++++++++++++ clients/web/src/utils/jsonUtils.ts | 25 ++++- core/json/nullableUnion.ts | 93 +++++++++++++++---- 4 files changed, 207 insertions(+), 20 deletions(-) diff --git a/clients/web/src/test/core/nullableUnion.test.ts b/clients/web/src/test/core/nullableUnion.test.ts index f56abbdd..915e7e8e 100644 --- a/clients/web/src/test/core/nullableUnion.test.ts +++ b/clients/web/src/test/core/nullableUnion.test.ts @@ -144,6 +144,39 @@ describe("normalizeNullableUnion", () => { }); }); + // JSON Schema's `enum` is untyped, so a typeless branch only implies strings + // when every member is one. Guessing otherwise would hand a number to a + // renderer that declared the option list `string[]`. + it("does not infer string for a typeless non-string enum branch", () => { + const schema = { anyOf: [{ enum: [1, 2] }, { type: "null" as const }] }; + expect(normalizeNullableUnion(schema)).toBe(schema); + }); + + it("does not infer string for a typeless mixed enum branch", () => { + const schema = { anyOf: [{ enum: ["a", 2] }, { type: "null" as const }] }; + expect(normalizeNullableUnion(schema)).toBe(schema); + }); + + it("does not infer string for a typeless empty enum branch", () => { + const schema = { anyOf: [{ enum: [] }, { type: "null" as const }] }; + expect(normalizeNullableUnion(schema)).toBe(schema); + }); + + // An explicit `type` is authoritative — the enum members are the server's + // problem at that point, not an inference this function is making. + it("still collapses a non-string enum branch that declares its type", () => { + expect( + normalizeNullableUnion({ + anyOf: [{ type: "number", enum: [1, 2] }, { type: "null" }], + }), + ).toEqual({ + type: "number", + enum: [1, 2], + anyOf: undefined, + nullable: true, + }); + }); + it("infers string for a typeless enum branch", () => { expect( normalizeNullableUnion({ diff --git a/clients/web/src/utils/jsonUtils.test.ts b/clients/web/src/utils/jsonUtils.test.ts index 57a61132..4a69409c 100644 --- a/clients/web/src/utils/jsonUtils.test.ts +++ b/clients/web/src/utils/jsonUtils.test.ts @@ -221,6 +221,42 @@ describe("collectSchemaDefaults", () => { it("returns an empty object when the schema has no properties", () => { expect(collectSchemaDefaults({ type: "object" })).toEqual({}); }); + + // The form normalizes a nullable union before rendering, so this has to as + // well: a nested object's `properties` live on the union's surviving branch, + // and a default only visible there would be displayed but never submitted. + it("collects defaults hoisted out of a nullable object union", () => { + const schema: InspectorFormSchema = { + type: "object", + properties: { + profile: { + anyOf: [ + { + type: "object", + properties: { nick: { type: "string", default: "ada" } }, + }, + { type: "null" }, + ], + }, + }, + }; + expect(collectSchemaDefaults(schema)).toEqual({ + profile: { nick: "ada" }, + }); + }); + + it("keeps a default declared on the union wrapper itself", () => { + const schema: InspectorFormSchema = { + type: "object", + properties: { + direction: { + default: "envio", + anyOf: [{ type: "string", enum: ["envio"] }, { type: "null" }], + }, + }, + }; + expect(collectSchemaDefaults(schema)).toEqual({ direction: "envio" }); + }); }); describe("hasMissingRequiredFields", () => { @@ -247,4 +283,44 @@ describe("hasMissingRequiredFields", () => { false, ); }); + + // JSON Schema `required` constrains presence, not content — so an explicit + // `null` satisfies a required field whose schema admits null. Since #1928 the + // user can produce exactly that by clearing a nullable enum, and treating it + // as missing would disable submit on a value the schema calls valid. + it("accepts an explicit null for a required field that admits null", () => { + const nullable: InspectorFormSchema = { + type: "object", + properties: { + direction: { + anyOf: [{ type: "string", enum: ["envio"] }, { type: "null" }], + }, + }, + required: ["direction"], + }; + expect(hasMissingRequiredFields(nullable, { direction: null })).toBe(false); + expect(hasMissingRequiredFields(nullable, {})).toBe(true); + }); + + it("accepts an explicit null for a required type: [T, null] field", () => { + const nullable: InspectorFormSchema = { + type: "object", + properties: { note: { type: ["string", "null"] } }, + required: ["note"], + }; + expect(hasMissingRequiredFields(nullable, { note: null })).toBe(false); + }); + + it("still rejects null for a required field whose schema is not nullable", () => { + expect(hasMissingRequiredFields(schema, { name: null })).toBe(true); + }); + + it("rejects null for a required field the schema does not describe", () => { + const undescribed: InspectorFormSchema = { + type: "object", + properties: {}, + required: ["ghost"], + }; + expect(hasMissingRequiredFields(undescribed, { ghost: null })).toBe(true); + }); }); diff --git a/clients/web/src/utils/jsonUtils.ts b/clients/web/src/utils/jsonUtils.ts index 98dd905a..2cdb2331 100644 --- a/clients/web/src/utils/jsonUtils.ts +++ b/clients/web/src/utils/jsonUtils.ts @@ -1,3 +1,5 @@ +import { normalizeNullableUnion } from "@inspector/core/json/nullableUnion.js"; + export type JsonValue = | string | number @@ -116,7 +118,12 @@ export function collectSchemaDefaults( ): Record { const properties = schema.properties ?? {}; const result: Record = {}; - for (const [fieldName, fieldSchema] of Object.entries(properties)) { + for (const [fieldName, rawSchema] of Object.entries(properties)) { + // Collapse a nullable union first, for the same reason `SchemaForm` does: + // a nested object's `properties` live on the union's surviving branch, so + // without this the form would *display* a hoisted default that never + // reached the seeded values — the field would submit empty (#1928). + const fieldSchema = normalizeNullableUnion(rawSchema); if (fieldSchema.default !== undefined) { result[fieldName] = fieldSchema.default; } else if (fieldSchema.type === "object" && fieldSchema.properties) { @@ -133,15 +140,29 @@ export function collectSchemaDefaults( * Whether any of the schema's required top-level fields is missing a value in * `values` (absent, null, or empty string). Used to gate a form's submit * action until required fields are supplied. + * + * `null` counts as missing only for a field that does not admit it. JSON + * Schema's `required` constrains *presence*, not content, so a required field + * whose schema is a nullable union is satisfied by an explicit `null` — and + * since #1928 the user can produce exactly that by clearing a nullable enum. + * Treating it as missing would leave the form's submit permanently disabled on + * a value the schema calls valid. */ export function hasMissingRequiredFields( schema: InspectorFormSchema, values: Record, ): boolean { const required = schema.required ?? []; + const properties = schema.properties ?? {}; return required.some((field) => { const value = values[field]; - return value === undefined || value === null || value === ""; + if (value === null) { + const fieldSchema = properties[field]; + return fieldSchema === undefined + ? true + : normalizeNullableUnion(fieldSchema).nullable !== true; + } + return value === undefined || value === ""; }); } diff --git a/core/json/nullableUnion.ts b/core/json/nullableUnion.ts index 4e81b791..2adfdd51 100644 --- a/core/json/nullableUnion.ts +++ b/core/json/nullableUnion.ts @@ -25,7 +25,8 @@ const RENDERABLE_TYPES = [ "object", ] as const; -type RenderableType = (typeof RENDERABLE_TYPES)[number]; +/** A `type` name the collapse is willing to produce. */ +export type RenderableType = (typeof RENDERABLE_TYPES)[number]; function isRenderableType(type: unknown): type is RenderableType { return RENDERABLE_TYPES.includes(type as RenderableType); @@ -51,6 +52,71 @@ function toBranch(value: unknown): Record | null { return value as Record; } +/** + * Whether a branch's `enum` is one a *typeless* branch can be read as a string + * enum. + * + * JSON Schema's `enum` is untyped — `[1, 2]`, `[true, false]`, and `[null]` are + * all legal — so a bare `{ enum: [...] }` does **not** imply strings. Guessing + * `"string"` for a numeric enum would hand non-strings to a renderer that has + * declared them `string[]`: the web `Select` would receive numbers, and the TUI + * would `String(...)` them and submit `"1"` where the server expects `1`. So + * only an all-string enum earns the inference; anything else stays on the + * fallback path, which renders the value honestly as JSON. + */ +function isStringEnum(value: unknown): boolean { + return ( + Array.isArray(value) && + value.length > 0 && + value.every((member) => typeof member === "string") + ); +} + +/** + * The result of a collapse that actually matched. Spelled out rather than + * reusing `T` because the collapse genuinely changes two of `T`'s fields: + * `type` becomes a single renderable name (never an array, never `"null"`), + * and `anyOf` is cleared. Returning `T` for these would let a caller whose `T` + * types `type` as an array go on treating it as one after it has become a + * string, or dereference an `anyOf` that is now `undefined`. + * + * Note this cannot make the *hoist* sound: an `anyOf` branch is `unknown`, so + * any keyword lifted off it is whatever the server sent, however `T` declares + * it. {@link isStringEnum} validates the one keyword the renderers dereference + * as a typed array (`enum`); the rest reach widgets that read them defensively. + */ +export type NormalizedNullableUnion = Omit< + T, + "type" | "anyOf" +> & { + type?: RenderableType; + anyOf?: undefined; + nullable?: boolean; +}; + +/** + * Build the collapsed schema. + * + * The assertion is needed because the spread merges an unresolved generic `T` + * with an `unknown` branch, and `NormalizedNullableUnion`'s `Omit` stays + * deferred while `T` is open — so TS cannot verify the two line up even though + * every field is set right here. Isolated in this one function so the exported + * signature carries the contract and nothing else has to assert it. + */ +function collapsed( + schema: T, + branch: Record | undefined, + type: RenderableType, +): NormalizedNullableUnion { + return { + ...schema, + ...branch, + type, + anyOf: undefined, + nullable: true, + } as NormalizedNullableUnion; +} + /** * Collapse a nullable union — what Zod's `.nullish()` / `.nullable()` and * FastMCP's optional arguments emit — into `{ type: , nullable: true }`. @@ -76,7 +142,7 @@ function toBranch(value: unknown): Record | null { */ export function normalizeNullableUnion( schema: T, -): T { +): T | NormalizedNullableUnion { if (schema.anyOf?.length === 2) { const branches = schema.anyOf.map(toBranch); const nullBranch = branches.find((entry) => entry?.type === "null"); @@ -85,21 +151,13 @@ export function normalizeNullableUnion( ); if (nullBranch && branch) { - // A bare `{ enum: [...] }` branch carries no `type`; JSON Schema allows - // that, and every value such a schema admits here is a string. - const type = branch.type ?? (branch.enum ? "string" : undefined); + // A branch may carry an `enum` and no `type`; JSON Schema allows that, and + // an all-string enum is unambiguously a string field. See isStringEnum for + // why a non-string enum deliberately does not get the same treatment. + const type = + branch.type ?? (isStringEnum(branch.enum) ? "string" : undefined); if (isRenderableType(type)) { - // The result is `schema` with `type` narrowed to a value it already - // admitted, the branch's keywords merged in, and `nullable` added. TS - // cannot express "a spread of T and a subset of T is still a T", and a - // non-generic return would force every caller to cast instead. - return { - ...schema, - ...branch, - type, - anyOf: undefined, - nullable: true, - } as T; + return collapsed(schema, branch, type); } } } @@ -111,8 +169,7 @@ export function normalizeNullableUnion( ) { const type = schema.type.find((member) => member !== "null"); if (isRenderableType(type)) { - // Same reasoning as above. - return { ...schema, type, nullable: true } as T; + return collapsed(schema, undefined, type); } }