fix: honor sibling constraints on nullability, guard numeric item enums
Copilot round 6: no visible comments, three suppressed. All valid.
1. JSON Schema keywords at one level are conjunctive, so a syntactic null does
not by itself mean the schema accepts null. `{ type: ["string", "null"],
enum: ["envio"] }` names "null" and still rejects it — yet the collapse
marked it `nullable: true`, giving the field a clear button that emits a
value the schema forbids, and `admitsNull` let required-field gating accept
it. Both now consult `nullExcludedBySiblings`.
The distinction that makes this safe: a *branch's* `enum` is not a sibling.
In `anyOf: [{ type: "string", enum: [...] }, { type: "null" }]` — the #1928
shape — the enum constrains that branch alone and the union still permits
null, so `nullable` is read off the ORIGINAL schema rather than the hoisted
merge. Reading the merge would have called the headline case non-nullable.
Flattening is unaffected either way; only the nullability claim is withheld.
2. The collapse newly routed a nullable array of numeric item enums into the
string-only `MultiSelect` path, which would submit `["1"]` for `[1]`. Gated
on `isStringEnum`, matching `toConstOptions`; `isStringEnum` is now a type
predicate so the narrowing is real rather than asserted.
3. Invalid JSON and an empty field both report `undefined`, so an optional
complex argument could be dropped from the submission while invalid text sat
visible in the box. The field now says so via `error`. Note a *required*
field was already gated — its value is `undefined`, which
`hasMissingRequiredFields` treats as missing — so this is specifically the
optional case. Disabling Execute/Open/Submit on an invalid optional draft
needs a validity channel out of `SchemaForm` into all four call sites, which
is wider than this fix; filed separately.
Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
parent
32d6f80bc1
commit
86ba42633c
4 changed files with 214 additions and 18 deletions
|
|
@ -744,6 +744,45 @@ describe("SchemaForm", () => {
|
|||
// original symptom, and it lived here rather than in the dispatch. It matters
|
||||
// more since #2007, whose fix deliberately routes object unions to this
|
||||
// editor: a fallback nobody can type into is not a fallback.
|
||||
it("shows an error while the JSON draft does not parse", async () => {
|
||||
const user = userEvent.setup();
|
||||
const schema: InspectorFormSchema = {
|
||||
type: "object",
|
||||
properties: {
|
||||
config: { type: "array", title: "Config" },
|
||||
},
|
||||
};
|
||||
function Harness() {
|
||||
const [values, setValues] = useState<Record<string, unknown>>({});
|
||||
return (
|
||||
<SchemaForm schema={schema} values={values} onChange={setValues} />
|
||||
);
|
||||
}
|
||||
renderWithMantine(<Harness />);
|
||||
|
||||
const jsonInput = screen.getByLabelText(/Config/) as HTMLTextAreaElement;
|
||||
await user.type(jsonInput, "[[1,");
|
||||
// Invalid text yields no value, so without this the field would submit as
|
||||
// absent while the user is still looking at what they typed.
|
||||
expect(screen.getByText(/Not valid JSON/)).toBeInTheDocument();
|
||||
|
||||
await user.type(jsonInput, "2]");
|
||||
expect(screen.queryByText(/Not valid JSON/)).not.toBeInTheDocument();
|
||||
});
|
||||
|
||||
it("shows no error for an empty optional field", () => {
|
||||
const schema: InspectorFormSchema = {
|
||||
type: "object",
|
||||
properties: {
|
||||
config: { type: "array", title: "Config" },
|
||||
},
|
||||
};
|
||||
renderWithMantine(
|
||||
<SchemaForm schema={schema} values={{}} onChange={vi.fn()} />,
|
||||
);
|
||||
expect(screen.queryByText(/Not valid JSON/)).not.toBeInTheDocument();
|
||||
});
|
||||
|
||||
it("reports no value, not raw text, while the JSON is mid-edit", async () => {
|
||||
const user = userEvent.setup();
|
||||
const onChange = vi.fn();
|
||||
|
|
@ -1183,6 +1222,52 @@ describe("SchemaForm nullable unions", () => {
|
|||
expect(screen.getByRole("textbox", { name: "Items" })).toBeInTheDocument();
|
||||
});
|
||||
|
||||
// Reachable only since the collapse landed: the nullable wrapper had no
|
||||
// top-level `type` before, so it fell to the JSON editor rather than into a
|
||||
// string-valued MultiSelect that would submit ["1"] for [1].
|
||||
it("falls back to the JSON input for a nullable array of numeric item enums", () => {
|
||||
// Wire data again — a numeric `enum` is valid JSON Schema but outside
|
||||
// `InspectorFormSchema`'s `string[]`, so it comes in through the narrow.
|
||||
const schema = toFormSchema({
|
||||
type: "object",
|
||||
properties: {
|
||||
levels: {
|
||||
title: "Levels",
|
||||
anyOf: [{ type: "array", items: { enum: [1, 2] } }, { type: "null" }],
|
||||
},
|
||||
},
|
||||
});
|
||||
renderWithMantine(
|
||||
<SchemaForm schema={schema!} values={{}} onChange={vi.fn()} />,
|
||||
);
|
||||
expect(screen.getByLabelText(/Levels/).tagName).toBe("TEXTAREA");
|
||||
});
|
||||
|
||||
// The sibling enum rules null out even though the type list names it, so the
|
||||
// field must not get a clear button that emits a value the schema rejects.
|
||||
it("offers no clear button when a sibling enum excludes null", () => {
|
||||
const schema = toFormSchema({
|
||||
type: "object",
|
||||
properties: {
|
||||
direction: {
|
||||
title: "Direction",
|
||||
type: ["string", "null"],
|
||||
enum: ["envio", "recebimento"],
|
||||
},
|
||||
},
|
||||
});
|
||||
renderWithMantine(
|
||||
<SchemaForm
|
||||
schema={schema!}
|
||||
values={{ direction: "envio" }}
|
||||
onChange={vi.fn()}
|
||||
/>,
|
||||
);
|
||||
expect(
|
||||
screen.queryByRole("button", { hidden: true }),
|
||||
).not.toBeInTheDocument();
|
||||
});
|
||||
|
||||
it("still falls back to the JSON input for a union of two real types", () => {
|
||||
const schema: InspectorFormSchema = {
|
||||
type: "object",
|
||||
|
|
|
|||
|
|
@ -15,7 +15,10 @@ import type {
|
|||
InspectorFormSchema,
|
||||
JsonSchemaConst,
|
||||
} from "../../../utils/jsonUtils";
|
||||
import { normalizeNullableUnion } from "@inspector/core/json/nullableUnion.js";
|
||||
import {
|
||||
isStringEnum,
|
||||
normalizeNullableUnion,
|
||||
} from "@inspector/core/json/nullableUnion.js";
|
||||
|
||||
const FieldLabel = Text.withProps({
|
||||
fw: 500,
|
||||
|
|
@ -196,6 +199,15 @@ interface SchemaJsonFieldProps {
|
|||
* `undefined` rather than handed the text. An inspector must not report a value
|
||||
* it cannot send, and `undefined` is how "no value supplied" is spelled
|
||||
* everywhere else in this form.
|
||||
*
|
||||
* That leaves invalid text and an empty field indistinguishable *to the
|
||||
* parent*, so the field says so itself via `error`. A **required** field is
|
||||
* already gated — its value is `undefined`, which `hasMissingRequiredFields`
|
||||
* treats as missing — but an **optional** one would otherwise be dropped from
|
||||
* the submission with no indication. Disabling Execute/Open/Submit on an
|
||||
* invalid optional draft needs a validity channel out of this component and
|
||||
* into all four callers, which is a wider change than this fix; tracked
|
||||
* separately.
|
||||
*/
|
||||
function SchemaJsonField({
|
||||
value,
|
||||
|
|
@ -216,10 +228,23 @@ function SchemaJsonField({
|
|||
}
|
||||
});
|
||||
|
||||
// Text that is present but does not parse yields no value, so the field would
|
||||
// otherwise submit as absent while the user is still looking at what they
|
||||
// typed. Saying so on the field is what keeps that from being silent; see the
|
||||
// note on `SchemaJsonField` for the submit-gating half, which is not local to
|
||||
// this component.
|
||||
const hasInvalidDraft =
|
||||
draft.trim() !== "" && parseJsonDraft(draft) === undefined;
|
||||
|
||||
return (
|
||||
<SchemaJsonInput
|
||||
{...inputProps}
|
||||
value={draft}
|
||||
error={
|
||||
hasInvalidDraft
|
||||
? "Not valid JSON — this field will be omitted"
|
||||
: undefined
|
||||
}
|
||||
onChange={(text) => {
|
||||
setDraft(text);
|
||||
onChange(parseJsonDraft(text));
|
||||
|
|
@ -458,12 +483,14 @@ export function SchemaForm({
|
|||
);
|
||||
}
|
||||
|
||||
// array of enum values (multi-select)
|
||||
if (fieldSchema.type === "array" && fieldSchema.items?.enum) {
|
||||
const data = toEnumData(
|
||||
fieldSchema.items.enum,
|
||||
fieldSchema.items.enumNames,
|
||||
);
|
||||
// array of enum values (multi-select). Gated on the members being strings
|
||||
// for the same reason `toConstOptions` is: `MultiSelect` is string-valued,
|
||||
// so `items: { enum: [1, 2] }` would submit `["1"]` where the schema says
|
||||
// `[1]`. Reachable from a nullable array since the collapse landed, which
|
||||
// is what makes the guard load-bearing rather than theoretical.
|
||||
const itemsEnum = fieldSchema.items?.enum;
|
||||
if (fieldSchema.type === "array" && isStringEnum(itemsEnum)) {
|
||||
const data = toEnumData(itemsEnum, fieldSchema.items?.enumNames);
|
||||
return (
|
||||
<MultiSelect
|
||||
key={fieldName}
|
||||
|
|
|
|||
|
|
@ -312,6 +312,35 @@ describe("normalizeNullableUnion", () => {
|
|||
});
|
||||
});
|
||||
|
||||
// Flattening still happens — only the nullability claim is withheld, because
|
||||
// the sibling enum rules null out even though the type list names it.
|
||||
it("flattens but does not mark nullable when a sibling enum excludes null", () => {
|
||||
expect(
|
||||
normalizeNullableUnion({
|
||||
type: ["string", "null"],
|
||||
enum: ["envio", "recebimento"],
|
||||
}),
|
||||
).toEqual({
|
||||
type: "string",
|
||||
enum: ["envio", "recebimento"],
|
||||
anyOf: undefined,
|
||||
nullable: false,
|
||||
});
|
||||
});
|
||||
|
||||
it("still marks nullable when the enum lives on an anyOf branch", () => {
|
||||
expect(
|
||||
normalizeNullableUnion({
|
||||
anyOf: [{ type: "string", enum: ["envio"] }, { type: "null" }],
|
||||
}),
|
||||
).toEqual({
|
||||
type: "string",
|
||||
enum: ["envio"],
|
||||
anyOf: undefined,
|
||||
nullable: true,
|
||||
});
|
||||
});
|
||||
|
||||
it("leaves a mismatched enumNames alone rather than inventing an alignment", () => {
|
||||
expect(
|
||||
normalizeNullableUnion({
|
||||
|
|
@ -328,20 +357,6 @@ describe("normalizeNullableUnion", () => {
|
|||
});
|
||||
});
|
||||
|
||||
it("leaves a null-free enum untouched", () => {
|
||||
expect(
|
||||
normalizeNullableUnion({
|
||||
type: ["string", "null"],
|
||||
enum: ["a", "b"],
|
||||
}),
|
||||
).toEqual({
|
||||
type: "string",
|
||||
enum: ["a", "b"],
|
||||
anyOf: undefined,
|
||||
nullable: true,
|
||||
});
|
||||
});
|
||||
|
||||
it("normalizes type array array|null", () => {
|
||||
expect(normalizeNullableUnion({ type: ["array", "null"] })).toEqual({
|
||||
type: "array",
|
||||
|
|
@ -424,4 +439,42 @@ describe("admitsNull", () => {
|
|||
it("ignores non-object anyOf members instead of throwing", () => {
|
||||
expect(admitsNull({ anyOf: ["null", ["null"], 7] })).toBe(false);
|
||||
});
|
||||
|
||||
// JSON Schema keywords at one level are conjunctive, so a syntactic null does
|
||||
// not by itself mean the schema accepts null. `{ type: ["string","null"],
|
||||
// enum: ["envio"] }` names "null" and still rejects it.
|
||||
it("is false when a sibling enum excludes null", () => {
|
||||
expect(admitsNull({ type: ["string", "null"], enum: ["envio"] })).toBe(
|
||||
false,
|
||||
);
|
||||
expect(
|
||||
admitsNull({
|
||||
anyOf: [{ type: "string" }, { type: "null" }],
|
||||
enum: ["a"],
|
||||
}),
|
||||
).toBe(false);
|
||||
});
|
||||
|
||||
it("is true when a sibling enum offers null", () => {
|
||||
expect(
|
||||
admitsNull({ type: ["string", "null"], enum: ["envio", null] }),
|
||||
).toBe(true);
|
||||
});
|
||||
|
||||
it("is false when a sibling const pins a non-null value", () => {
|
||||
expect(admitsNull({ type: ["string", "null"], const: "envio" })).toBe(
|
||||
false,
|
||||
);
|
||||
expect(admitsNull({ type: ["string", "null"], const: null })).toBe(true);
|
||||
});
|
||||
|
||||
// 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.
|
||||
it("is unaffected by an enum that lives inside an anyOf branch", () => {
|
||||
expect(
|
||||
admitsNull({
|
||||
anyOf: [{ type: "string", enum: ["envio"] }, { type: "null" }],
|
||||
}),
|
||||
).toBe(true);
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -43,9 +43,10 @@ export interface NullableUnionSchema {
|
|||
enum?: unknown[];
|
||||
anyOf?: readonly unknown[];
|
||||
// Read only by `admitsNull`, which considers encodings the collapse itself
|
||||
// declines to flatten.
|
||||
// declines to flatten, and the sibling constraints that can rule null out.
|
||||
oneOf?: readonly unknown[];
|
||||
nullable?: boolean;
|
||||
const?: unknown;
|
||||
}
|
||||
|
||||
/** Narrow an `anyOf` member to a readable object, or `null` if it isn't one. */
|
||||
|
|
@ -71,7 +72,7 @@ function toBranch(value: unknown): Record<string, unknown> | null {
|
|||
* an enum to a select at all. The TUI's does, and its options are stringified,
|
||||
* so an unguarded numeric enum there submits `"1"` where the server wants `1`.
|
||||
*/
|
||||
export function isStringEnum(value: unknown): boolean {
|
||||
export function isStringEnum(value: unknown): value is string[] {
|
||||
return (
|
||||
Array.isArray(value) &&
|
||||
value.length > 0 &&
|
||||
|
|
@ -101,6 +102,30 @@ export type NormalizedNullableUnion<T extends NullableUnionSchema> = Omit<
|
|||
nullable?: boolean;
|
||||
};
|
||||
|
||||
/**
|
||||
* Whether a schema's **own** `enum`/`const` rules `null` out, regardless of what
|
||||
* its `type` or union branches say.
|
||||
*
|
||||
* JSON Schema keywords at one level are **conjunctive** — a value must satisfy
|
||||
* all of them — so a syntactic null does not by itself mean the schema accepts
|
||||
* null. `{ type: ["string", "null"], enum: ["envio"] }` names `"null"` in its
|
||||
* type list and still rejects `null`, because the `enum` does not offer it.
|
||||
* Marking that field nullable would give it a clear button that emits a value
|
||||
* the schema forbids, and would make required-field gating accept it.
|
||||
*
|
||||
* Evaluated against the schema's own level only. A branch's `enum` is *not* a
|
||||
* sibling — in `anyOf: [{ type: "string", enum: [...] }, { type: "null" }]` the
|
||||
* enum constrains that branch alone, and the union still permits null. Reading
|
||||
* it as a sibling would break exactly the shape #1928 is about, so callers must
|
||||
* pass the original schema here, never the hoisted merge.
|
||||
*/
|
||||
function nullExcludedBySiblings(schema: NullableUnionSchema): boolean {
|
||||
if (Array.isArray(schema.enum) && !schema.enum.includes(null)) {
|
||||
return true;
|
||||
}
|
||||
return schema.const !== undefined && schema.const !== null;
|
||||
}
|
||||
|
||||
/**
|
||||
* What {@link stripNullEnumMembers} concluded about an `enum`.
|
||||
*
|
||||
|
|
@ -192,7 +217,10 @@ function collapsed<T extends NullableUnionSchema>(
|
|||
...merged,
|
||||
type,
|
||||
anyOf: undefined,
|
||||
nullable: true,
|
||||
// Read off the ORIGINAL schema, not the merge: a hoisted branch `enum` is
|
||||
// not a sibling constraint, and treating it as one would mark the very
|
||||
// shape #1928 is about as non-nullable. See `nullExcludedBySiblings`.
|
||||
nullable: admitsNull(schema),
|
||||
...(strip.kind === "filtered"
|
||||
? { enum: strip.members, enumNames: strip.names }
|
||||
: {}),
|
||||
|
|
@ -216,6 +244,9 @@ function collapsed<T extends NullableUnionSchema>(
|
|||
* branch anywhere in an `anyOf` or `oneOf` of any size.
|
||||
*/
|
||||
export function admitsNull(schema: NullableUnionSchema): boolean {
|
||||
if (nullExcludedBySiblings(schema)) {
|
||||
return false;
|
||||
}
|
||||
if (schema.nullable === true) {
|
||||
return true;
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in a new issue