diff --git a/clients/tui/__tests__/schemaToForm.test.ts b/clients/tui/__tests__/schemaToForm.test.ts index be0d2b10..ec8d3aeb 100644 --- a/clients/tui/__tests__/schemaToForm.test.ts +++ b/clients/tui/__tests__/schemaToForm.test.ts @@ -743,9 +743,10 @@ describe("schemaToForm", () => { expect(decodeFormValues({ type: "object" }, values)).toBe(values); }); - it("ignores a malformed property declaration when restoring constants", () => { - // `properties` values are `unknown`; a `null` entry must not throw on the - // way out of the form any more than it does on the way in. + it("declines a union carrying a malformed property declaration", () => { + // `properties` values are `unknown`; a `null` entry is not a schema, so + // the union is declined — and, the point of the test, nothing throws on + // the way out of the form any more than on the way in. const schema = { type: "object", anyOf: [ @@ -756,9 +757,10 @@ describe("schemaToForm", () => { { type: "object", properties: { kind: { const: "b" } } }, ] as unknown[], }; + // No union, so the form values pass through as they are. expect( decodeFormValues(schema, { __variant: "0", __b0__kind: "tampered" }), - ).toEqual({ kind: "a" }); + ).toEqual({ __variant: "0", __b0__kind: "tampered" }); }); }); diff --git a/clients/web/src/test/core/jsonUtils.test.ts b/clients/web/src/test/core/jsonUtils.test.ts index 486f902b..6cd4c5f3 100644 --- a/clients/web/src/test/core/jsonUtils.test.ts +++ b/clients/web/src/test/core/jsonUtils.test.ts @@ -291,11 +291,12 @@ describe("JSON Utils", () => { ], }, }; - // A `properties: { broken: null }` entry must not throw, and a branch is - // still identifiable by the discriminator that *is* well-formed. + // A `properties: { broken: null }` entry must not throw. The union is + // declined — a `null` is not a schema, and handing it on would crash a + // renderer — so nothing is coerced and the strings pass through. expect( convertToolParameters(malformed, { kind: "a", value: "3" }), - ).toEqual({ kind: "a", value: 3 }); + ).toEqual({ kind: "a", value: "3" }); }); it("falls back to the branch-agreement path when no constant is supplied (#2123)", () => { @@ -342,11 +343,11 @@ describe("JSON Utils", () => { ], }, }; - // The `null` is not a vote about the type, and must not end up standing - // in for one — the surviving declaration is what coerces. + // The `null` makes the whole union unofferable, so nothing is coerced — + // and, the point of the test, nothing throws either. expect( convertToolParameters(malformedDeclaration, { count: "3" }), - ).toEqual({ count: 3 }); + ).toEqual({ count: "3" }); }); it("coerces a value whose schema lives on a root allOf branch (#2123)", () => { diff --git a/clients/web/src/test/core/rootUnion.test.ts b/clients/web/src/test/core/rootUnion.test.ts index c408680a..bc29fbfe 100644 --- a/clients/web/src/test/core/rootUnion.test.ts +++ b/clients/web/src/test/core/rootUnion.test.ts @@ -748,6 +748,35 @@ describe("resolveRootUnion", () => { expect(branches).toEqual([]); }); + it("declines a member carrying a property that is not a schema", () => { + // A `null` or an array is not a schema, and the web form dereferences + // one on the way to choosing a widget — so the branch is declined rather + // than handed on to crash a tool panel. + for (const property of [null, [1]] as unknown[]) { + const { branches } = resolveRootUnion({ + type: "object", + anyOf: [ + EMAIL, + { type: "object", properties: { broken: property } }, + ] as unknown[], + }); + expect(branches).toEqual([]); + } + }); + + it("offers a member carrying a boolean property schema", () => { + // JSON Schema's boolean form is legal and answers every keyword lookup + // with `undefined`, so it renders through the JSON fallback harmlessly. + const { branches } = resolveRootUnion({ + type: "object", + anyOf: [ + { type: "object", properties: { anything: true } }, + { type: "object", properties: { other: { type: "string" } } }, + ] as unknown[], + }); + expect(branches).toHaveLength(2); + }); + it("declines a member whose properties are not an object", () => { // Members arrive as `unknown`, so this is reachable and must not throw. expect( @@ -979,6 +1008,33 @@ describe("resolveRootUnion", () => { ); }); + it("prefers the branch the supplied names satisfy over a matching constant", () => { + // `{ kind: "email", phone: "555" }` agrees with the email branch's + // discriminator while missing its `address`, and satisfies the phone + // branch outright — the picker must show the one that can be called. + const mixed = resolveRootUnion({ + type: "object", + anyOf: [ + { + type: "object", + properties: { + kind: { const: "email" }, + address: { type: "string" }, + }, + required: ["kind", "address"], + }, + { + type: "object", + properties: { phone: { type: "string" } }, + required: ["phone"], + }, + ], + }).branches; + expect(selectBranchIndex(mixed, { kind: "email", phone: "555" })).toBe(1); + // …and the constant still decides when the names settle nothing. + expect(selectBranchIndex(mixed, { kind: "email" })).toBe(0); + }); + it("reports none when the values identify nothing", () => { expect(selectBranchIndex(branches, {})).toBeNull(); expect(selectBranchIndex(branches, { kind: "other" })).toBeNull(); diff --git a/core/json/rootUnion.ts b/core/json/rootUnion.ts index ad9f31da..25059afe 100644 --- a/core/json/rootUnion.ts +++ b/core/json/rootUnion.ts @@ -176,10 +176,23 @@ function propertiesOf(schema: RootUnionSchema): Record { * as a fillable form would offer a call that cannot be valid. */ function isOfferable(branch: RootUnionSchema): boolean { + if (!hasReadableProperties(branch) || !admitsObject(branch)) return false; + const properties = Object.values(propertiesOf(branch)); return ( - hasReadableProperties(branch) && - Object.keys(propertiesOf(branch)).length > 0 && - admitsObject(branch) + properties.length > 0 && + // Every value has to be something a renderer can read. JSON Schema's + // boolean form is legal and harmless — `true`/`false` answer every keyword + // lookup with `undefined` — but a `null` or an array is not a schema at + // all, and the web form dereferences one on the way to choosing a widget. + // A branch carrying one is declined rather than handed on to crash a tool + // panel that would otherwise have rendered. + properties.every( + (property) => + typeof property === "boolean" || + (typeof property === "object" && + property !== null && + !Array.isArray(property)), + ) ); } @@ -748,17 +761,22 @@ export function selectBranchIndex( if (pinned.length > 0) agreeing.push(index); }); - // One branch's discriminator matched and no other's did — the plain case. - if (agreeing.length === 1) return agreeing[0]; - - // Several branches remain — they share the constant that was supplied, or - // none was. The values still belong to a shape, so keep looking among the - // candidates by the names that were supplied. - return narrowBySuppliedNames( + // What the supplied NAMES say comes first: a matching constant does not + // identify a branch on its own while another candidate leaves that property + // unpinned — `{ kind: "email", phone: "555" }` agrees with an email branch + // whose `address` is missing while satisfying a phone branch outright, and + // the picker must show the one that could actually be called. + const narrowed = narrowBySuppliedNames( branches, candidates, Object.keys(values).filter(supplied), ); + if (narrowed !== null) return narrowed; + + // Nothing in the names settled it, so a lone agreeing constant is the last + // evidence left — a branch pinning `kind` to what was supplied says more + // than one that merely permits any value there. + return agreeing.length === 1 ? agreeing[0] : null; } /**