fix: address Copilot review round 5 — union member faithfulness, partial discriminators, const-null siblings
- rootUnion: union members get the same flattenability test as allOf members,
so a branch whose nested `allOf: [false]` makes it unsatisfiable is no
longer offered as a callable alternative with that constraint erased.
- selectBranchIndex: weigh only the pinned constants the caller SUPPLIED. One
it did not supply is a value this identification exists to seed, so
requiring it meant a deep link naming `kind` alone matched no branch
whenever the branches also pinned, say, a `version`. A supplied constant
that disagrees is still evidence against.
- admitsNull: `const: null` is conjunctive with its siblings, not an
override — `{ type: "string", const: null }` and
`{ const: null, anyOf: [...] }` reject every value, so the claim is made
only where the type and a sibling union leave null on the table.
Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
parent
7e4af5ed34
commit
f556f8831b
6 changed files with 103 additions and 14 deletions
11
AGENTS.md
11
AGENTS.md
|
|
@ -211,10 +211,13 @@ v2/main/
|
|||
│ │ # intersections: a union whose members are not ALL
|
||||
│ │ # field-carrying objects (or whose member `type`
|
||||
│ │ # rules objects out), a branch restating a
|
||||
│ │ # constraint the root states differently, an allOf
|
||||
│ │ # carrying a member it cannot fold in (`false`, a
|
||||
│ │ # $ref) — dropping the keyword there would turn an
|
||||
│ │ # UNSATISFIABLE schema into a fillable form — and a
|
||||
│ │ # constraint the root states differently, ANY member
|
||||
│ │ # stating more than the merge applies (only
|
||||
│ │ # type/properties/required — a `false`, a $ref, a
|
||||
│ │ # nested applicator would have its constraint erased
|
||||
│ │ # with the keyword, turning an UNSATISFIABLE schema
|
||||
│ │ # into a fillable form; allOf members are checked
|
||||
│ │ # against the ACCUMULATED merge, not the root), and a
|
||||
│ │ # schema carrying BOTH oneOf and anyOf. Does not
|
||||
│ │ # interpret `not` at all. Declining changes what
|
||||
│ │ # RENDERS, never whether the tool takes arguments:
|
||||
|
|
|
|||
|
|
@ -312,7 +312,7 @@ What it declines to flatten is as deliberate as what it flattens, and every case
|
|||
|
||||
- **A union whose members are not all field-carrying object schemas** — including one whose member `type` rules objects out, since tool arguments are a JSON object and such a member can never match. A picker whose options render nothing is no better than no picker.
|
||||
- **A branch that restates a constraint the root already states.** The two are conjunctive, so root `minimum: 10` under branch `minimum: 0` is still 10, disjoint `enum`s leave nothing satisfiable, and `type: "string"` under `type: "number"` describes a value that cannot exist — rendering either side would accept what the schema rejects. A property both declare *compatibly* is merged rather than replaced, so a root's `minimum` survives a branch's `maximum`, and a disagreement about `title`/`description` is not a conflict at all.
|
||||
- **An `allOf` with a member it cannot fold in** — JSON Schema's boolean form (`allOf: [false, …]` admits nothing) or a `$ref`, whose constraints are unknown rather than absent. Merging the rest and dropping the keyword would turn an unsatisfiable schema into a fillable form.
|
||||
- **A composition member stating anything the merge cannot apply.** Only `type`, `properties` and `required` are folded in, so a member carrying a nested `allOf`/`anyOf`, a `not`, an `additionalProperties`, or a `$ref` would have that constraint erased along with the keyword — turning an unsatisfiable schema (`allOf: [false, …]` admits nothing) into a fillable form. `allOf` members are checked against the accumulated merge rather than the root alone, so two of them contradicting each other is caught even when neither contradicts the root.
|
||||
- **A schema carrying both `oneOf` and `anyOf`** — independent keywords a value satisfies *together*, not two spellings of one union, so reading one and dropping the other omits real constraints while looking complete. Satisfying both honestly means the cross product of their alternatives, which no real schema has yet asked for.
|
||||
- **`not`**, which is not interpreted at all: there is no faithful form for "anything except this".
|
||||
|
||||
|
|
|
|||
|
|
@ -731,4 +731,23 @@ describe("admitsNull", () => {
|
|||
}),
|
||||
).toBe(true);
|
||||
});
|
||||
|
||||
describe("a const-null schema (#2123)", () => {
|
||||
it("admits null when nothing else rules it out", () => {
|
||||
expect(admitsNull({ const: null })).toBe(true);
|
||||
expect(admitsNull({ type: "null", const: null })).toBe(true);
|
||||
expect(admitsNull({ type: ["string", "null"], const: null })).toBe(true);
|
||||
});
|
||||
|
||||
it("does not override a sibling that rejects null", () => {
|
||||
// `const` is conjunctive with its siblings, not an override: both of
|
||||
// these reject every value, so claiming nullability would let the
|
||||
// required-field gate accept a `null` the schema forbids.
|
||||
expect(admitsNull({ type: "string", const: null })).toBe(false);
|
||||
expect(admitsNull({ const: null, anyOf: [{ type: "string" }] })).toBe(
|
||||
false,
|
||||
);
|
||||
expect(admitsNull({ const: null, not: { type: "null" } })).toBe(false);
|
||||
});
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -397,6 +397,24 @@ describe("resolveRootUnion", () => {
|
|||
).toEqual([]);
|
||||
});
|
||||
|
||||
it("declines a member carrying a constraint the merge cannot apply", () => {
|
||||
// Same faithfulness test the `allOf` fold applies — otherwise a branch
|
||||
// whose nested `allOf: [false]` makes it unsatisfiable is offered as a
|
||||
// callable alternative.
|
||||
const { branches } = resolveRootUnion({
|
||||
type: "object",
|
||||
anyOf: [
|
||||
EMAIL,
|
||||
{
|
||||
type: "object",
|
||||
properties: { x: { type: "string" } },
|
||||
allOf: [false as unknown],
|
||||
},
|
||||
],
|
||||
});
|
||||
expect(branches).toEqual([]);
|
||||
});
|
||||
|
||||
it("declines an empty union", () => {
|
||||
expect(resolveRootUnion({ type: "object", anyOf: [] }).branches).toEqual(
|
||||
[],
|
||||
|
|
@ -450,6 +468,33 @@ describe("resolveRootUnion", () => {
|
|||
expect(selectBranchIndex(branches, { kind: "sms" })).toBe(1);
|
||||
});
|
||||
|
||||
it("identifies a branch from the constants that were supplied", () => {
|
||||
// Both branches pin `version` as well; a deep link naming only `kind`
|
||||
// must still find its branch — the unsupplied constant is one this
|
||||
// identification exists to seed.
|
||||
const versioned = resolveRootUnion({
|
||||
type: "object",
|
||||
anyOf: [
|
||||
{
|
||||
type: "object",
|
||||
properties: {
|
||||
version: { const: "1" },
|
||||
kind: { const: "email" },
|
||||
},
|
||||
},
|
||||
{
|
||||
type: "object",
|
||||
properties: { version: { const: "1" }, kind: { const: "sms" } },
|
||||
},
|
||||
],
|
||||
}).branches;
|
||||
expect(selectBranchIndex(versioned, { kind: "sms" })).toBe(1);
|
||||
// A supplied constant that disagrees is still evidence against.
|
||||
expect(selectBranchIndex(versioned, { version: "2", kind: "sms" })).toBe(
|
||||
null,
|
||||
);
|
||||
});
|
||||
|
||||
it("reports none when the values identify nothing", () => {
|
||||
expect(selectBranchIndex(branches, {})).toBeNull();
|
||||
expect(selectBranchIndex(branches, { kind: "other" })).toBeNull();
|
||||
|
|
|
|||
|
|
@ -368,17 +368,15 @@ function collapsed<T extends NullableUnionSchema>(
|
|||
* an optimistic guess, and a schema this module cannot read renders through the
|
||||
* JSON editor with its constraints intact.
|
||||
*/
|
||||
/** Whether an absent or null-naming `type` leaves null on the table. */
|
||||
function typeAdmitsNull(type: string | string[] | undefined): boolean {
|
||||
return type === undefined || typeNamesNull(type);
|
||||
}
|
||||
|
||||
export function admitsNull(schema: NullableUnionSchema): boolean {
|
||||
if (nullExcludedBySiblings(schema)) {
|
||||
return false;
|
||||
}
|
||||
// `const: null` admits null and nothing else, whatever the schema says (or
|
||||
// omits) about `type`. Without this a required field pinned to null is seeded
|
||||
// with the only value it accepts and then reported as missing, leaving submit
|
||||
// permanently disabled on a form the user cannot change (#2123).
|
||||
if (schema.const === null) {
|
||||
return true;
|
||||
}
|
||||
// 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
|
||||
|
|
@ -388,6 +386,20 @@ export function admitsNull(schema: NullableUnionSchema): boolean {
|
|||
if (hasOpaqueApplicator(schema)) {
|
||||
return false;
|
||||
}
|
||||
// `const: null` admits null and nothing else — the case a required field
|
||||
// pinned to null needs, or it is seeded with the only value it accepts and
|
||||
// then reported missing, leaving submit permanently disabled on a field the
|
||||
// user cannot change (#2123).
|
||||
//
|
||||
// Claimed only where the siblings agree, since `const` is conjunctive with
|
||||
// them rather than an override: `{ type: "string", const: null }` and
|
||||
// `{ const: null, anyOf: [...] }` reject every value, and saying otherwise
|
||||
// would let the gate accept a `null` the schema forbids. The opaque
|
||||
// applicators (`not`, `allOf`, `oneOf`) are already refused above.
|
||||
if (schema.const === null) {
|
||||
return schema.anyOf === undefined && typeAdmitsNull(schema.type);
|
||||
}
|
||||
|
||||
if (schema.nullable === true) {
|
||||
return true;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -430,6 +430,11 @@ export function resolveRootUnion<T extends RootUnionSchema>(
|
|||
(branch) =>
|
||||
branch === null ||
|
||||
!isOfferable(branch) ||
|
||||
// The same faithfulness test the `allOf` fold applies: a member
|
||||
// carrying a constraint the merge does not copy — a nested `allOf`, a
|
||||
// `not`, a `$ref` — would have it erased along with the union keyword,
|
||||
// which can present an unsatisfiable branch as a callable one.
|
||||
!isFlattenable(branch) ||
|
||||
conflictsWithBase(base, branch),
|
||||
)
|
||||
) {
|
||||
|
|
@ -476,9 +481,14 @@ export function selectBranchIndex<T extends RootUnionSchema>(
|
|||
] as const,
|
||||
)
|
||||
.filter(([, constValue]) => constValue !== undefined);
|
||||
// Only the pinned names the caller actually supplied are evidence. A
|
||||
// constant it did not supply is one this identification exists to *seed* —
|
||||
// requiring it would mean a deep link naming `kind` alone matched no branch
|
||||
// whenever the branches also pin, say, a `version`.
|
||||
const supplied = pinned.filter(([name]) => values[name] !== undefined);
|
||||
if (
|
||||
pinned.length > 0 &&
|
||||
pinned.every(([name, constValue]) => values[name] === constValue)
|
||||
supplied.length > 0 &&
|
||||
supplied.every(([name, constValue]) => values[name] === constValue)
|
||||
) {
|
||||
matches.push(index);
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in a new issue