diff --git a/clients/web/src/test/core/nullableUnion.test.ts b/clients/web/src/test/core/nullableUnion.test.ts index a2127f2b..3a59953a 100644 --- a/clients/web/src/test/core/nullableUnion.test.ts +++ b/clients/web/src/test/core/nullableUnion.test.ts @@ -418,21 +418,24 @@ describe("normalizeNullableUnion", () => { 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({ + // Deliberately stricter than value-equality: a wrapper restating its branch's + // `type` also declines. That costs a dropdown on a rare redundant shape and + // buys a rule with no gap, which is the better trade for a module that cannot + // evaluate JSON Schema. + it("declines even when wrapper and branch agree on a shared keyword", () => { + const schema = { type: "string", - enum: ["a"], - anyOf: undefined, - // The explicit non-null `type` rules null out, conjunctively. - nullable: false, - }); + anyOf: [{ type: "string", enum: ["a"] }, { type: "null" as const }], + }; + expect(normalizeNullableUnion(schema)).toBe(schema); + }); + + it("declines when the wrapper carries an applicator the module cannot read", () => { + const schema = { + allOf: [{ minLength: 2 }], + anyOf: [{ type: "string" }, { type: "null" as const }], + }; + expect(normalizeNullableUnion(schema)).toBe(schema); }); it("treats wrapper metadata as safe to keep alongside branch constraints", () => { @@ -484,13 +487,51 @@ describe("admitsNull", () => { expect(admitsNull(wide)).toBe(true); }); - it("recognizes a null branch in a oneOf, and a nested type array", () => { - expect(admitsNull({ oneOf: [{ type: "string" }, { type: "null" }] })).toBe( - true, - ); + it("recognizes a null branch declared as a nested type array", () => { expect(admitsNull({ anyOf: [{ type: ["string", "null"] }] })).toBe(true); }); + // Applicators this module does not evaluate. `oneOf` requires *exactly one* + // branch to match, so a null branch does not by itself mean null validates; + // `not` and `allOf` can rule it out outright. Under-claiming costs a clear + // button, over-claiming lets the form emit a value the schema rejects. + it("declines to claim nullability for applicators it cannot evaluate", () => { + expect(admitsNull({ oneOf: [{ type: "string" }, { type: "null" }] })).toBe( + false, + ); + expect( + admitsNull({ not: { type: "null" }, anyOf: [{ type: "null" }] }), + ).toBe(false); + expect( + admitsNull({ allOf: [{ type: "string" }], anyOf: [{ type: "null" }] }), + ).toBe(false); + }); + + // A branch can name null and still admit nothing. + it("ignores an unsatisfiable null branch", () => { + expect( + admitsNull({ anyOf: [{ type: "string" }, { type: "null", const: "x" }] }), + ).toBe(false); + expect( + admitsNull({ + anyOf: [{ type: "string" }, { type: "null", enum: ["x"] }], + }), + ).toBe(false); + expect( + admitsNull({ + anyOf: [{ type: "string" }, { type: "null", not: { type: "null" } }], + }), + ).toBe(false); + }); + + it("still accepts a plain null branch alongside an unsatisfiable one", () => { + expect( + admitsNull({ + anyOf: [{ type: "null", const: "x" }, { type: "null" }], + }), + ).toBe(true); + }); + it("is false for a schema that does not permit null", () => { expect(admitsNull({ type: "string" })).toBe(false); expect(admitsNull({ type: ["string", "number"] })).toBe(false); diff --git a/core/json/nullableUnion.ts b/core/json/nullableUnion.ts index c24fec92..9f79af3d 100644 --- a/core/json/nullableUnion.ts +++ b/core/json/nullableUnion.ts @@ -45,6 +45,8 @@ export interface NullableUnionSchema { // Read only by `admitsNull`, which considers encodings the collapse itself // declines to flatten, and the sibling constraints that can rule null out. oneOf?: readonly unknown[]; + allOf?: readonly unknown[]; + not?: unknown; nullable?: boolean; const?: unknown; } @@ -127,69 +129,63 @@ function nullExcludedBySiblings(schema: NullableUnionSchema): boolean { } /** - * Keywords that *constrain what values are valid*, as opposed to describing the - * field (`title`, `description`, `default`, `enumNames`). + * Keywords that only *describe* a field. Everything else constrains what values + * are valid. * - * 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. + * This is an **allowlist on purpose**, and the direction matters. The previous + * version enumerated the *validation* keywords and declined on a conflict among + * them, which meant any keyword nobody thought of — `oneOf`, `allOf`, `not`, + * `$ref`, a future vocabulary — was silently treated as safe to drop. A missing + * entry there fails open, and open means rendering a widget for a constraint + * that is not the schema's. Listing the harmless keys instead fails closed: an + * unrecognized keyword sends the field to the JSON editor, which is the correct + * answer for a schema this module does not understand. */ -const VALIDATION_KEYWORDS = [ - "type", - "enum", - "const", - "items", - "properties", - "required", - "additionalProperties", - "minimum", - "maximum", - "exclusiveMinimum", - "exclusiveMaximum", - "multipleOf", - "minLength", - "maxLength", - "pattern", - "format", - "minItems", - "maxItems", - "uniqueItems", -] as const; +const ANNOTATION_KEYWORDS = new Set([ + "title", + "description", + "default", + "enumNames", + "examples", + "deprecated", + "readOnly", + "writeOnly", + "$comment", + "$schema", + "$id", +]); /** - * Whether hoisting this branch would *drop* one of the wrapper's constraints. + * Whether hoisting a branch would *drop* something the wrapper constrains. * * 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. + * bounds, the object keywords, and the applicators (`oneOf`, `allOf`, `not`) + * all 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. + * Rather than intersect them — a much larger surface, and one where a subtly + * wrong intersection is worse than none because it renders a *plausible* widget + * for the wrong constraint — the collapse **declines** whenever the wrapper + * carries anything beyond annotations and the union itself. The field then + * renders through the JSON editor with its full schema intact. * - * 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. + * This is stricter than comparing values for equality: a wrapper that merely + * restates its branch's `type` now also declines. That costs a dropdown on a + * rare redundant shape and buys a rule with no gap in it, which is the better + * trade for a module that cannot evaluate JSON Schema. */ -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); - }); +function wrapperCarriesConstraints(schema: NullableUnionSchema): boolean { + return Object.keys(schema).some( + (key) => key !== "anyOf" && !ANNOTATION_KEYWORDS.has(key), + ); +} + +/** Whether a schema's `type` names `"null"` at all. */ +function typeNamesNull(type: unknown): boolean { + return Array.isArray(type) ? type.includes("null") : type === "null"; } /** @@ -313,6 +309,19 @@ export function admitsNull(schema: NullableUnionSchema): boolean { if (nullExcludedBySiblings(schema)) { return false; } + // Applicators this module does not evaluate. `not: { type: "null" }` rules + // null out; `allOf` can add a member that does; and `oneOf` requires + // **exactly one** branch to match, so a null branch does not by itself mean + // `null` validates — two matching branches make it fail. Rather than answer + // these half-correctly, say no: under-claiming nullability costs a clear + // button, while over-claiming lets the form emit a value the schema rejects. + if ( + schema.not !== undefined || + schema.allOf !== undefined || + schema.oneOf !== undefined + ) { + return false; + } if (schema.nullable === true) { return true; } @@ -322,21 +331,25 @@ export function admitsNull(schema: NullableUnionSchema): boolean { // 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 typeNamesNull(schema.type); } - return [schema.anyOf, schema.oneOf].some((branches) => - branches?.some((entry) => { + return ( + schema.anyOf?.some((entry) => { const branch = toBranch(entry); - if (branch === null) { + if (branch === null || !typeNamesNull(branch.type)) { return false; } + // A branch that names null can still admit nothing: `{ type: "null", + // const: "x" }` is unsatisfiable, and its own applicators are as opaque + // here as the wrapper's. + const branchSchema = branch as NullableUnionSchema; return ( - branch.type === "null" || - (Array.isArray(branch.type) && branch.type.includes("null")) + !nullExcludedBySiblings(branchSchema) && + branchSchema.not === undefined && + branchSchema.allOf === undefined && + branchSchema.oneOf === undefined ); - }), + }) ?? false ); } @@ -375,7 +388,7 @@ export function normalizeNullableUnion( // Hoisting would silently drop a wrapper constraint the branch also // carries, so decline instead; see `branchDropsWrapperConstraint`. - if (nullBranch && branch && !branchDropsWrapperConstraint(schema, branch)) { + if (nullBranch && branch && !wrapperCarriesConstraints(schema)) { // 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.