fix: address Copilot review round 24 — a discriminator must be required
A distinct `const` does not make `oneOf` alternatives mutually exclusive
unless the property carrying it is required: two branches pinning an OPTIONAL
`kind` both match `{}`, so arguments omitting it satisfy more than one
alternative — exactly what `oneOf` forbids. The property must now be required
by the root or by every branch.
Also makes `propertiesOf` total (an unreadable `properties` reads as empty,
with a separate predicate for the one caller that must tell them apart), which
removes a row of unreachable `?? {}` fallbacks, and covers the value/type
comparisons the const checks rest on.
Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall
parent
4cea4e1985
commit
54c5e12b2b
4 changed files with 167 additions and 37 deletions
|
|
@ -313,7 +313,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.
|
||||
- **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 `oneOf` whose alternatives are not mutually exclusive.** `oneOf` demands that *exactly one* alternative match, which flattening cannot preserve — the branches are offered as if any would do. It is only safe with a discriminator (a property every branch pins to a `const` of its own), so an undiscriminated `oneOf` is declined. `anyOf` makes no such claim and is offered either way.
|
||||
- **A `oneOf` whose alternatives are not mutually exclusive.** `oneOf` demands that *exactly one* alternative match, which flattening cannot preserve — the branches are offered as if any would do. It is only safe with a discriminator: a property every branch pins to a `const` of its own **and requires**, since an optional one leaves `{}` matching every branch. An undiscriminated `oneOf` is declined; `anyOf` makes no such claim and is offered either way.
|
||||
- **A union that adds fields under a restrictive root `additionalProperties`.** That keyword constrains whatever its *sibling* `properties` does not name, so a root `additionalProperties: false` rejects every field the branches add — flattening would move them beside the keyword, where they read as allowed. An empty schema (`{}`) constrains nothing and is treated as permissive.
|
||||
- **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".
|
||||
|
|
|
|||
|
|
@ -135,6 +135,9 @@ describe("JSON Utils", () => {
|
|||
kind: { type: "string", const: "a" },
|
||||
value: { type: "number" },
|
||||
},
|
||||
// Required, or the alternatives are not exclusive and the
|
||||
// resolver declines the `oneOf` outright.
|
||||
required: ["kind"],
|
||||
},
|
||||
{
|
||||
type: "object",
|
||||
|
|
@ -142,6 +145,7 @@ describe("JSON Utils", () => {
|
|||
kind: { type: "string", const: "b" },
|
||||
value: { type: "boolean" },
|
||||
},
|
||||
required: ["kind"],
|
||||
},
|
||||
],
|
||||
},
|
||||
|
|
|
|||
|
|
@ -444,6 +444,81 @@ describe("resolveRootUnion", () => {
|
|||
expect(branches).toEqual([]);
|
||||
});
|
||||
|
||||
describe("values it compares and types it recognizes", () => {
|
||||
it("offers a branch whose type list names object", () => {
|
||||
const { branches } = resolveRootUnion({
|
||||
type: "object",
|
||||
anyOf: [
|
||||
{
|
||||
type: ["object", "null"],
|
||||
properties: { a: { type: "string" } },
|
||||
},
|
||||
{ type: "object", properties: { b: { type: "string" } } },
|
||||
] as unknown[],
|
||||
});
|
||||
expect(branches).toHaveLength(2);
|
||||
});
|
||||
|
||||
it("compares array-valued constants structurally", () => {
|
||||
const { branches } = resolveRootUnion({
|
||||
type: "object",
|
||||
required: ["tag"],
|
||||
oneOf: [
|
||||
{
|
||||
type: "object",
|
||||
properties: { tag: { const: [1, 2] }, x: { type: "string" } },
|
||||
},
|
||||
{
|
||||
type: "object",
|
||||
properties: { tag: { const: [1, 2] }, y: { type: "string" } },
|
||||
},
|
||||
],
|
||||
});
|
||||
// The same value twice, so the alternatives are not exclusive.
|
||||
expect(branches).toEqual([]);
|
||||
});
|
||||
|
||||
it("treats a keyword explicitly set to undefined as a disagreement", () => {
|
||||
const { branches } = resolveRootUnion({
|
||||
type: "object",
|
||||
properties: { x: { minimum: undefined } },
|
||||
anyOf: [
|
||||
{ type: "object", properties: { x: { minimum: 1 } } },
|
||||
{ type: "object", properties: { other: { type: "string" } } },
|
||||
] as unknown[],
|
||||
});
|
||||
expect(branches).toEqual([]);
|
||||
});
|
||||
|
||||
it("recognizes each JSON type when checking a const", () => {
|
||||
const accepts = (type: unknown, constValue: unknown) =>
|
||||
resolveRootUnion({
|
||||
type: "object",
|
||||
properties: { x: { type } as unknown },
|
||||
anyOf: [
|
||||
{ type: "object", properties: { x: { const: constValue } } },
|
||||
{ type: "object", properties: { other: { type: "string" } } },
|
||||
] as unknown[],
|
||||
}).branches.length > 0;
|
||||
|
||||
expect(accepts("null", null)).toBe(true);
|
||||
expect(accepts("array", [1])).toBe(true);
|
||||
expect(accepts("number", 1)).toBe(true);
|
||||
expect(accepts(["string", "integer"], 1)).toBe(true);
|
||||
expect(accepts("integer", 1.5)).toBe(false);
|
||||
expect(accepts(["string", "boolean"], 1)).toBe(false);
|
||||
});
|
||||
|
||||
it("emits no properties when neither side declares any", () => {
|
||||
const { base } = resolveRootUnion({
|
||||
type: "object",
|
||||
allOf: [{ type: "object", required: ["a"] }],
|
||||
});
|
||||
expect(base.properties).toBeUndefined();
|
||||
expect(base.required).toEqual(["a"]);
|
||||
});
|
||||
});
|
||||
|
||||
describe("branch labels", () => {
|
||||
it("uses the branch's own title first", () => {
|
||||
const { branches } = resolveRootUnion({
|
||||
|
|
@ -597,6 +672,43 @@ describe("resolveRootUnion", () => {
|
|||
expect(branches).toEqual([]);
|
||||
});
|
||||
|
||||
it("declines a oneOf whose discriminator is optional", () => {
|
||||
// Two branches pinning an OPTIONAL `kind` both match `{}`, so arguments
|
||||
// omitting it satisfy more than one alternative.
|
||||
const { branches } = resolveRootUnion({
|
||||
type: "object",
|
||||
oneOf: [
|
||||
{
|
||||
type: "object",
|
||||
properties: { kind: { const: "a" }, x: { type: "string" } },
|
||||
},
|
||||
{
|
||||
type: "object",
|
||||
properties: { kind: { const: "b" }, y: { type: "string" } },
|
||||
},
|
||||
],
|
||||
});
|
||||
expect(branches).toEqual([]);
|
||||
});
|
||||
|
||||
it("accepts a discriminator the root requires", () => {
|
||||
const { branches } = resolveRootUnion({
|
||||
type: "object",
|
||||
required: ["kind"],
|
||||
oneOf: [
|
||||
{
|
||||
type: "object",
|
||||
properties: { kind: { const: "a" }, x: { type: "string" } },
|
||||
},
|
||||
{
|
||||
type: "object",
|
||||
properties: { kind: { const: "b" }, y: { type: "string" } },
|
||||
},
|
||||
],
|
||||
});
|
||||
expect(branches).toHaveLength(2);
|
||||
});
|
||||
|
||||
it("declines a oneOf whose named discriminator does not distinguish", () => {
|
||||
const { branches } = resolveRootUnion({
|
||||
type: "object",
|
||||
|
|
|
|||
|
|
@ -136,17 +136,27 @@ function admitsObject(schema: RootUnionSchema): boolean {
|
|||
return Array.isArray(type) ? type.includes("object") : type === "object";
|
||||
}
|
||||
|
||||
/** A readable `properties` map, or `null` when the value is not one. */
|
||||
function propertiesOf(schema: RootUnionSchema): Record<string, unknown> | null {
|
||||
/** Whether a schema's `properties` is a readable map rather than junk. */
|
||||
function hasReadableProperties(schema: RootUnionSchema): boolean {
|
||||
const { properties } = schema;
|
||||
if (
|
||||
typeof properties !== "object" ||
|
||||
properties === null ||
|
||||
Array.isArray(properties)
|
||||
) {
|
||||
return null;
|
||||
}
|
||||
return properties;
|
||||
return (
|
||||
typeof properties === "object" &&
|
||||
properties !== null &&
|
||||
!Array.isArray(properties)
|
||||
);
|
||||
}
|
||||
|
||||
/**
|
||||
* A schema's `properties`, or an empty map when it has none — or when what it
|
||||
* has is not a map at all, which a wire schema really can be. Total by design:
|
||||
* every caller but {@link isOfferable} wants to enumerate whatever is there,
|
||||
* and a nullable return would leave each of them carrying a `?? {}` that
|
||||
* nothing can reach.
|
||||
*/
|
||||
function propertiesOf(schema: RootUnionSchema): Record<string, unknown> {
|
||||
return hasReadableProperties(schema)
|
||||
? (schema.properties as Record<string, unknown>)
|
||||
: {};
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
@ -166,10 +176,9 @@ function propertiesOf(schema: RootUnionSchema): Record<string, unknown> | null {
|
|||
* as a fillable form would offer a call that cannot be valid.
|
||||
*/
|
||||
function isOfferable(branch: RootUnionSchema): boolean {
|
||||
const properties = propertiesOf(branch);
|
||||
return (
|
||||
properties !== null &&
|
||||
Object.keys(properties).length > 0 &&
|
||||
hasReadableProperties(branch) &&
|
||||
Object.keys(propertiesOf(branch)).length > 0 &&
|
||||
admitsObject(branch)
|
||||
);
|
||||
}
|
||||
|
|
@ -364,8 +373,8 @@ function conflictsWithBase(
|
|||
base: RootUnionSchema,
|
||||
branch: RootUnionSchema,
|
||||
): boolean {
|
||||
const baseProperties = propertiesOf(base) ?? {};
|
||||
const branchProperties = propertiesOf(branch) ?? {};
|
||||
const baseProperties = propertiesOf(base);
|
||||
const branchProperties = propertiesOf(branch);
|
||||
return Object.entries(branchProperties).some(
|
||||
([name, branchProperty]) =>
|
||||
// `hasOwn`, not `in`: `properties` is a JSON record, so `constructor` and
|
||||
|
|
@ -388,12 +397,19 @@ function conflictsWithBase(
|
|||
function hasDiscriminator(
|
||||
members: RootUnionSchema[],
|
||||
named: string | undefined,
|
||||
rootRequired: string[],
|
||||
): boolean {
|
||||
const first = propertiesOf(members[0] ?? {}) ?? {};
|
||||
const first = propertiesOf(members[0] ?? {});
|
||||
const candidates = named !== undefined ? [named] : Object.keys(first);
|
||||
return candidates.some((name) => {
|
||||
// Required, or it discriminates nothing: two branches pinning an OPTIONAL
|
||||
// `kind` to different constants both match `{}`, so arguments omitting it
|
||||
// satisfy more than one alternative — exactly what `oneOf` forbids.
|
||||
const constants = members.map((member) => {
|
||||
const property = toBranch((propertiesOf(member) ?? {})[name]) as {
|
||||
if (!rootRequired.includes(name) && !requiredOf(member).includes(name)) {
|
||||
return undefined;
|
||||
}
|
||||
const property = toBranch(propertiesOf(member)[name]) as {
|
||||
const?: unknown;
|
||||
} | null;
|
||||
return property?.const;
|
||||
|
|
@ -438,8 +454,8 @@ function addsForbiddenNames(
|
|||
member: RootUnionSchema,
|
||||
): boolean {
|
||||
if (!restrictsAdditional(base)) return false;
|
||||
const baseNames = propertiesOf(base) ?? {};
|
||||
return Object.keys(propertiesOf(member) ?? {}).some(
|
||||
const baseNames = propertiesOf(base);
|
||||
return Object.keys(propertiesOf(member)).some(
|
||||
(name) => !Object.hasOwn(baseNames, name),
|
||||
);
|
||||
}
|
||||
|
|
@ -456,7 +472,7 @@ export function declaresAnyFields(
|
|||
schema: RootUnionSchema | undefined,
|
||||
): boolean {
|
||||
if (schema === undefined) return false;
|
||||
if (Object.keys(propertiesOf(schema) ?? {}).length > 0) return true;
|
||||
if (Object.keys(propertiesOf(schema)).length > 0) return true;
|
||||
// A `$ref`'s shape is unknown rather than empty, so it counts. Reporting "no
|
||||
// fields" for `anyOf: [{ $ref: … }, { $ref: … }]` would auto-invoke an App
|
||||
// tool with `{}` on the strength of something never read.
|
||||
|
|
@ -485,8 +501,8 @@ function mergeBranch<T extends RootUnionSchema>(
|
|||
base: T,
|
||||
branch: RootUnionSchema,
|
||||
): ResolvedSchema<T> {
|
||||
const baseProperties = propertiesOf(base) ?? {};
|
||||
const branchProperties = propertiesOf(branch) ?? {};
|
||||
const baseProperties = propertiesOf(base);
|
||||
const branchProperties = propertiesOf(branch);
|
||||
// Built through `fromEntries` rather than by assignment: a property named
|
||||
// `__proto__` is a legal argument name, and assigning it would invoke the
|
||||
// legacy prototype setter instead of creating an own property — losing the
|
||||
|
|
@ -544,7 +560,7 @@ function branchLabel(
|
|||
if (typeof branch.title === "string" && branch.title.trim() !== "") {
|
||||
return branch.title;
|
||||
}
|
||||
const properties = propertiesOf(branch) ?? {};
|
||||
const properties = propertiesOf(branch);
|
||||
const constOf = (name: string): string | null => {
|
||||
const property = toBranch(properties[name]) as { const?: unknown } | null;
|
||||
const value = property?.const;
|
||||
|
|
@ -662,6 +678,7 @@ export function resolveRootUnion<T extends RootUnionSchema>(
|
|||
!hasDiscriminator(
|
||||
branches as RootUnionSchema[],
|
||||
schema.discriminator?.propertyName,
|
||||
requiredOf(base),
|
||||
)
|
||||
) {
|
||||
return { base, branches: [] };
|
||||
|
|
@ -677,7 +694,7 @@ export function resolveRootUnion<T extends RootUnionSchema>(
|
|||
// `properties`/`required` off the branch, so the merge stays that way.
|
||||
schema: mergeBranch(base, branch),
|
||||
label: branchLabel(branch, index, discriminatorProperty),
|
||||
declaredFields: Object.keys(propertiesOf(branch) ?? {}),
|
||||
declaredFields: Object.keys(propertiesOf(branch)),
|
||||
})),
|
||||
};
|
||||
}
|
||||
|
|
@ -709,7 +726,7 @@ export function selectBranchIndex<T extends RootUnionSchema>(
|
|||
const candidates: number[] = [];
|
||||
const agreeing: number[] = [];
|
||||
branches.forEach((branch, index) => {
|
||||
const pinned = Object.entries(propertiesOf(branch.schema) ?? {})
|
||||
const pinned = Object.entries(propertiesOf(branch.schema))
|
||||
.map(
|
||||
([name, schema]) =>
|
||||
[
|
||||
|
|
@ -798,15 +815,12 @@ export function branchAcceptsValues<T extends RootUnionSchema>(
|
|||
branch: RootUnionBranch<T>,
|
||||
values: Record<string, unknown>,
|
||||
): boolean {
|
||||
return Object.entries(propertiesOf(branch.schema) ?? {}).every(
|
||||
([name, schema]) => {
|
||||
const constValue = (toBranch(schema) as { const?: unknown } | null)
|
||||
?.const;
|
||||
if (constValue === undefined) return true;
|
||||
if (!Object.hasOwn(values, name) || values[name] === undefined) {
|
||||
return true;
|
||||
}
|
||||
return sameValue(values[name], constValue);
|
||||
},
|
||||
);
|
||||
return Object.entries(propertiesOf(branch.schema)).every(([name, schema]) => {
|
||||
const constValue = (toBranch(schema) as { const?: unknown } | null)?.const;
|
||||
if (constValue === undefined) return true;
|
||||
if (!Object.hasOwn(values, name) || values[name] === undefined) {
|
||||
return true;
|
||||
}
|
||||
return sameValue(values[name], constValue);
|
||||
});
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in a new issue