From 987279b5d3a5972bd3e60e827a7cbef8b80b8345 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 15 Aug 2026 19:26:14 -0400 Subject: [PATCH] fix: stop the hoist dropping wrapper constraints, and let an explicit type decide MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot round 7: both findings are the same principle applied one level further — JSON Schema siblings are conjunctive, and this code was treating them as alternatives. 1. The hoist is a spread, so a branch keyword *replaced* the wrapper's. That is not a conjunction: a wrapper `enum: ["a"]` around a branch `enum: ["a", "b"]` means `"a"`, but the spread produced `["a", "b"]` and the dropdown would have offered — and submitted — a `"b"` the schema rejects. `type`, bounds, and the object keywords fail the same way. The collapse now declines when wrapper and branch carry different values for the same validation keyword, and the field renders through the JSON editor with its schema intact. Intersecting them properly is a much larger surface, and a subtly wrong intersection would be worse than none. Identical values are not a conflict, so the ordinary redundant-`type` shape still collapses; metadata (`title`, `description`, `default`, `enumNames`) never conflicts, since nothing validates against it. 2. `admitsNull` let a `{ type: "null" }` branch outvote an explicit non-null top-level `type`. `{ type: "string", anyOf: [..., { type: "null" }] }` rejects null — the value must satisfy the type *and* the union — so an explicit `type` now decides instead of falling through to the branch scan. Signed-off-by: cliffhall --- .../web/src/test/core/nullableUnion.test.ts | 88 +++++++++++++++++++ core/json/nullableUnion.ts | 84 ++++++++++++++++-- 2 files changed, 166 insertions(+), 6 deletions(-) diff --git a/clients/web/src/test/core/nullableUnion.test.ts b/clients/web/src/test/core/nullableUnion.test.ts index d5d55bba..a2127f2b 100644 --- a/clients/web/src/test/core/nullableUnion.test.ts +++ b/clients/web/src/test/core/nullableUnion.test.ts @@ -390,6 +390,70 @@ describe("normalizeNullableUnion", () => { expect(normalizeNullableUnion(schema)).toBe(schema); }); + // The hoist is a spread, but JSON Schema applies sibling keywords + // conjunctively — so a branch keyword replacing the wrapper's would *widen* + // the field. `enum: ["a"]` around a branch `enum: ["a","b"]` means "a", yet + // the spread would offer "b" in the dropdown and submit it. + it("declines when the branch would drop a conflicting wrapper enum", () => { + const schema = { + enum: ["a"], + anyOf: [{ type: "string", enum: ["a", "b"] }, { type: "null" as const }], + }; + expect(normalizeNullableUnion(schema)).toBe(schema); + }); + + it("declines when wrapper and branch disagree on type", () => { + const schema = { + type: "string", + anyOf: [{ type: "integer" }, { type: "null" as const }], + }; + expect(normalizeNullableUnion(schema)).toBe(schema); + }); + + it("declines when wrapper and branch disagree on bounds", () => { + const schema = { + minimum: 5, + anyOf: [{ type: "integer", minimum: 1 }, { type: "null" as const }], + }; + expect(normalizeNullableUnion(schema)).toBe(schema); + }); + + // Identical values are no conflict, so the ordinary redundant-type shape + // still collapses rather than being punished for being explicit. + it("collapses when wrapper and branch agree on a shared keyword", () => { + expect( + normalizeNullableUnion({ + type: "string", + anyOf: [{ type: "string", enum: ["a"] }, { type: "null" }], + }), + ).toEqual({ + type: "string", + enum: ["a"], + anyOf: undefined, + // The explicit non-null `type` rules null out, conjunctively. + nullable: false, + }); + }); + + it("treats wrapper metadata as safe to keep alongside branch constraints", () => { + expect( + normalizeNullableUnion({ + title: "Direction", + description: "Which way", + default: "envio", + anyOf: [{ type: "string", enum: ["envio"] }, { type: "null" }], + }), + ).toEqual({ + title: "Direction", + description: "Which way", + default: "envio", + type: "string", + enum: ["envio"], + anyOf: undefined, + nullable: true, + }); + }); + it("ignores a schema with no union keywords at all", () => { const schema = { type: "object" as const, properties: {} }; expect(normalizeNullableUnion(schema)).toBe(schema); @@ -470,6 +534,30 @@ describe("admitsNull", () => { // 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. + // An explicit `type` is conjunctive with the union, so it decides rather than + // merely offering another way to say yes. A `{ type: "null" }` branch cannot + // override a top-level `type: "string"`. + it("lets an explicit non-null type override a null branch", () => { + expect( + admitsNull({ + type: "string", + anyOf: [{ type: "string", enum: ["a"] }, { type: "null" }], + }), + ).toBe(false); + }); + + it("still honors an explicit type that does admit null", () => { + expect( + admitsNull({ + type: ["string", "null"], + anyOf: [{ type: "string" }, { type: "null" }], + }), + ).toBe(true); + expect(admitsNull({ type: "null", anyOf: [{ type: "string" }] })).toBe( + true, + ); + }); + it("is unaffected by an enum that lives inside an anyOf branch", () => { expect( admitsNull({ diff --git a/core/json/nullableUnion.ts b/core/json/nullableUnion.ts index 6141bcdd..c24fec92 100644 --- a/core/json/nullableUnion.ts +++ b/core/json/nullableUnion.ts @@ -126,6 +126,72 @@ function nullExcludedBySiblings(schema: NullableUnionSchema): boolean { return schema.const !== undefined && schema.const !== null; } +/** + * Keywords that *constrain what values are valid*, as opposed to describing the + * field (`title`, `description`, `default`, `enumNames`). + * + * Only these can conflict in a way that matters: a wrapper and its branch each + * carrying one means the value must satisfy **both**, which the hoist cannot + * express. Metadata is safe to merge because nothing validates against it. + */ +const VALIDATION_KEYWORDS = [ + "type", + "enum", + "const", + "items", + "properties", + "required", + "additionalProperties", + "minimum", + "maximum", + "exclusiveMinimum", + "exclusiveMaximum", + "multipleOf", + "minLength", + "maxLength", + "pattern", + "format", + "minItems", + "maxItems", + "uniqueItems", +] as const; + +/** + * Whether hoisting this branch would *drop* one of the wrapper's constraints. + * + * The hoist is a spread, so a branch key replaces the wrapper's — but JSON + * Schema applies sibling keywords **conjunctively**, and a replacement is not a + * conjunction. A wrapper `enum: ["a"]` around a branch `enum: ["a", "b"]` means + * `"a"` (both must hold), yet the spread yields `["a", "b"]` and the rendered + * dropdown would offer — and submit — a `"b"` the schema rejects. `type`, + * bounds, and the object keywords fail the same way. + * + * Rather than implement intersection for every keyword (a much larger surface, + * and one where a subtly wrong intersection is worse than none), the collapse + * **declines** when wrapper and branch disagree, and the field renders through + * the JSON editor with its full schema intact. Identical values are not a + * conflict, so the common `{ type: "string", anyOf: [{ type: "string", … }] }` + * still collapses. + * + * Comparison is by canonical JSON, so two structurally equal objects written + * with different key order read as a conflict. That is conservative in the safe + * direction: the field falls back rather than mis-renders. + */ +function branchDropsWrapperConstraint( + schema: NullableUnionSchema, + branch: Record, +): boolean { + const wrapper = schema as Record; + return VALIDATION_KEYWORDS.some((keyword) => { + const wrapperValue = wrapper[keyword]; + const branchValue = branch[keyword]; + if (wrapperValue === undefined || branchValue === undefined) { + return false; + } + return JSON.stringify(wrapperValue) !== JSON.stringify(branchValue); + }); +} + /** * What {@link stripNullEnumMembers} concluded about an `enum`. * @@ -250,11 +316,15 @@ export function admitsNull(schema: NullableUnionSchema): boolean { if (schema.nullable === true) { return true; } - if (schema.type === "null") { - return true; - } - if (Array.isArray(schema.type) && schema.type.includes("null")) { - return true; + // An explicit `type` is a sibling constraint too, so it *decides* — it does + // not merely add a way to say yes. `{ type: "string", anyOf: [..., { type: + // "null" }] }` rejects null, because a value must satisfy the `type` as well + // as the union. Falling through to the branch scan here would let the union + // override a constraint that outranks it. + if (schema.type !== undefined) { + return Array.isArray(schema.type) + ? schema.type.includes("null") + : schema.type === "null"; } return [schema.anyOf, schema.oneOf].some((branches) => branches?.some((entry) => { @@ -303,7 +373,9 @@ export function normalizeNullableUnion( (entry) => entry !== null && entry.type !== "null", ); - if (nullBranch && branch) { + // Hoisting would silently drop a wrapper constraint the branch also + // carries, so decline instead; see `branchDropsWrapperConstraint`. + if (nullBranch && branch && !branchDropsWrapperConstraint(schema, branch)) { // 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.