fix: decline a union branch requiring a name it never declares (#2224)
`isOfferable` in `core/json/rootUnion.ts` checked that a merged branch carries
at least one property and that every property value is readable, but never that
each name in `required` resolves to a property a form can render. A branch like
`{ properties: { kind }, required: ["kind", "payload"] }` was therefore offered
in the picker, rendered no control for `payload`, and had the submit-time check
report it missing permanently — the exact dead end the function's own comment
already reasons through for a `false`-schema required field, one case short.
Judge the merge, so a branch requiring only names the ROOT declares —
`anyOf: [{ required: ["email"] }, …]` — stays offerable as before. `hasOwn`
rather than `in`, since an argument legally named `constructor` would otherwise
resolve through the prototype and read as declared.
Declining every branch of a union leaves the web form with neither a picker nor
a field, which is a different dead end. `SchemaForm` now seeds the raw-JSON
editor (#2151) in that one case — a root union that resolved to no branches and
no properties — so the arguments stay expressible. Seeded rather than forced, so
the switch still works, and re-seeded during render (never in an effect) because
the form is reused across tools rather than remounted. A tool that genuinely
takes no arguments is untouched. The TUI has no such editor; there the union is
simply not offered, which is already better than a section that can never be
submitted.
Adds a `record_shipment_by` preset and wires it into the
`root-union-schemas-http.json` showcase so the shape is reproducible by hand,
with docs/test-servers.md covering it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PxYT4eKdnx1yRw7x4bW7Ts
Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
parent
23698afbe5
commit
81f2115e8b
4 changed files with 312 additions and 6 deletions
|
|
@ -656,6 +656,58 @@ describe("schemaToForm", () => {
|
|||
expect(names).toContain("__b_0____b0__x");
|
||||
});
|
||||
|
||||
// #2224: `required` lists names, not declarations, so a member may require
|
||||
// one it never declares. `buildFields` enumerates `properties` alone, so no
|
||||
// control was rendered for it while `missingRequiredFields` reported it
|
||||
// missing at every submit — a section the user could never complete.
|
||||
it("declines a member requiring a name it never declares", () => {
|
||||
const schema = {
|
||||
type: "object",
|
||||
anyOf: [
|
||||
{
|
||||
type: "object",
|
||||
properties: { kind: { type: "string", const: "a" } },
|
||||
required: ["kind", "payload"],
|
||||
},
|
||||
{
|
||||
type: "object",
|
||||
properties: { kind: { type: "string", const: "b" } },
|
||||
required: ["kind"],
|
||||
},
|
||||
],
|
||||
};
|
||||
const form = schemaToForm(schema, "undeclared_required");
|
||||
// No variant select and no per-branch section: the union is declined, so
|
||||
// the root's own (here empty) properties are what render.
|
||||
expect(form.sections).toEqual([{ title: "Parameters", fields: [] }]);
|
||||
// And nothing is reported missing, so the call is no longer blocked on a
|
||||
// field that has nowhere to be typed.
|
||||
expect(
|
||||
missingRequiredFields(schema, decodeFormValues(schema, {})),
|
||||
).toEqual([]);
|
||||
});
|
||||
|
||||
it("keeps offering a member requiring a name the root declares", () => {
|
||||
// The regression guard for the check above: the merge is what is judged,
|
||||
// and `anyOf: [{ required: ["email"] }, …]` is an ordinary union.
|
||||
const form = schemaToForm(
|
||||
{
|
||||
type: "object",
|
||||
properties: {
|
||||
email: { type: "string" },
|
||||
phone: { type: "string" },
|
||||
},
|
||||
anyOf: [
|
||||
{ type: "object", required: ["email"] },
|
||||
{ type: "object", required: ["phone"] },
|
||||
],
|
||||
},
|
||||
"inherited_required",
|
||||
);
|
||||
expect(form.sections).toHaveLength(3);
|
||||
expect(form.sections[0]!.fields[0]).toMatchObject({ name: "__variant" });
|
||||
});
|
||||
|
||||
describe("decodeFormValues", () => {
|
||||
it("submits the chosen branch's fields under their real names", () => {
|
||||
expect(
|
||||
|
|
|
|||
|
|
@ -2284,6 +2284,86 @@ describe("SchemaForm raw JSON (#2151)", () => {
|
|||
await enableRawJson(user);
|
||||
expect(screen.getAllByLabelText("Edit as JSON")).toHaveLength(1);
|
||||
});
|
||||
|
||||
// #2224: a root union whose alternatives are all declined resolves to a base
|
||||
// with nothing on it, so the form renders no picker and no fields. The switch
|
||||
// is the only way to make the call — open it rather than leaving the user in
|
||||
// front of a blank form to work that out.
|
||||
describe("fallback for an unrenderable root union (#2224)", () => {
|
||||
// Every member requires a name it never declares, so none is offerable.
|
||||
const deadEnd: InspectorFormSchema = {
|
||||
type: "object",
|
||||
anyOf: [
|
||||
{
|
||||
type: "object",
|
||||
properties: { kind: { type: "string", const: "a" } },
|
||||
required: ["kind", "payload"],
|
||||
},
|
||||
{
|
||||
type: "object",
|
||||
properties: { kind: { type: "string", const: "b" } },
|
||||
required: ["kind", "payload"],
|
||||
},
|
||||
],
|
||||
};
|
||||
|
||||
function switchElement(): HTMLInputElement {
|
||||
return screen.getByLabelText("Edit as JSON") as HTMLInputElement;
|
||||
}
|
||||
|
||||
it("opens the editor when the form would otherwise be empty", () => {
|
||||
renderWithMantine(<RawHarness schema={deadEnd} />);
|
||||
expect(switchElement().checked).toBe(true);
|
||||
expect(getAceTextByLabel(/Arguments JSON/)).toBe("{}");
|
||||
});
|
||||
|
||||
it("leaves the editor closed for a tool that takes no arguments", () => {
|
||||
// Nothing to render here either, but nothing is missing: seeding a JSON
|
||||
// editor for `{}` would be noise on every no-argument tool.
|
||||
renderWithMantine(
|
||||
<RawHarness schema={{ type: "object", properties: {} }} />,
|
||||
);
|
||||
expect(switchElement().checked).toBe(false);
|
||||
});
|
||||
|
||||
it("still lets the user switch back to the fields", async () => {
|
||||
const user = userEvent.setup();
|
||||
renderWithMantine(<RawHarness schema={deadEnd} />);
|
||||
await user.click(switchElement());
|
||||
expect(switchElement().checked).toBe(false);
|
||||
});
|
||||
|
||||
it("opens the editor for a schema that arrives after the first render", () => {
|
||||
// The form is reused across tools rather than remounted, so a `useState`
|
||||
// initializer alone would only ever see the tool it mounted on.
|
||||
const { rerender } = renderWithMantine(
|
||||
<SchemaForm schema={schema} values={{}} onChange={vi.fn()} />,
|
||||
);
|
||||
expect(switchElement().checked).toBe(false);
|
||||
|
||||
rerender(<SchemaForm schema={deadEnd} values={{}} onChange={vi.fn()} />);
|
||||
expect(switchElement().checked).toBe(true);
|
||||
});
|
||||
|
||||
it("does not close an editor the user opened when the schema changes", async () => {
|
||||
// One-way: the fallback opens the editor, and nothing closes it but the
|
||||
// user.
|
||||
const user = userEvent.setup();
|
||||
const { rerender } = renderWithMantine(
|
||||
<SchemaForm schema={schema} values={{}} onChange={vi.fn()} />,
|
||||
);
|
||||
await enableRawJson(user);
|
||||
|
||||
rerender(
|
||||
<SchemaForm
|
||||
schema={{ type: "object", properties: {} }}
|
||||
values={{}}
|
||||
onChange={vi.fn()}
|
||||
/>,
|
||||
);
|
||||
expect(switchElement().checked).toBe(true);
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
// A plain `<input>` swallows Enter, so a string argument could not be given a
|
||||
|
|
|
|||
|
|
@ -931,7 +931,26 @@ export function SchemaForm({
|
|||
// rather than reported under a reserved name: field names come straight out
|
||||
// of a server's schema, so any sentinel this form invented could collide with
|
||||
// a real argument and clear a block the user cannot see.
|
||||
const [rawJsonMode, setRawJsonMode] = useState(false);
|
||||
//
|
||||
// A root union whose alternatives are all declined resolves to a base with
|
||||
// nothing on it, so the form renders neither a picker nor a field — the user
|
||||
// is left staring at a switch they have to know to reach for in order to make
|
||||
// the call at all (#2224). Open the editor for them in that one case: the
|
||||
// arguments are still expressible, just not as fields. Seeded rather than
|
||||
// forced, so the switch keeps working; re-seeded during render (never in an
|
||||
// effect) because this form is reused across tools rather than remounted, and
|
||||
// a `useState` initializer would only ever see the first one.
|
||||
const rawJsonFallback =
|
||||
allowRawJson &&
|
||||
branches.length === 0 &&
|
||||
(schema.oneOf !== undefined || schema.anyOf !== undefined) &&
|
||||
Object.keys(properties).length === 0;
|
||||
const [rawJsonMode, setRawJsonMode] = useState(rawJsonFallback);
|
||||
useValueChange(rawJsonFallback, (fallback) => {
|
||||
// One-way: a schema that stops needing the fallback does not close an
|
||||
// editor the user may have opened on purpose.
|
||||
if (fallback) setRawJsonMode(true);
|
||||
});
|
||||
const [rawJsonInvalid, setRawJsonInvalid] = useState(false);
|
||||
|
||||
// Stable so `RawArgumentsField`'s reporting effect subscribes once rather
|
||||
|
|
|
|||
|
|
@ -809,6 +809,84 @@ describe("resolveRootUnion", () => {
|
|||
expect(branches).toHaveLength(2);
|
||||
});
|
||||
|
||||
it("declines a member requiring a name nothing declares", () => {
|
||||
// `required` is a list of names, not of declarations. Both form builders
|
||||
// enumerate `properties` alone, so `payload` gets no control while the
|
||||
// submit gate reports it missing forever — the picker offers an option
|
||||
// that can never be completed (#2224).
|
||||
const { branches } = resolveRootUnion({
|
||||
type: "object",
|
||||
anyOf: [
|
||||
{
|
||||
type: "object",
|
||||
properties: { kind: { const: "a" } },
|
||||
required: ["kind", "payload"],
|
||||
},
|
||||
SMS,
|
||||
],
|
||||
});
|
||||
expect(branches).toEqual([]);
|
||||
});
|
||||
|
||||
it("declines a member requiring a name only ANOTHER member declares", () => {
|
||||
// Branches are alternatives, not a conjunction: a name the sibling
|
||||
// declares is not one this branch can render, so the dead end is the
|
||||
// same one.
|
||||
const { branches } = resolveRootUnion({
|
||||
type: "object",
|
||||
anyOf: [
|
||||
{
|
||||
type: "object",
|
||||
properties: { kind: { const: "a" } },
|
||||
required: ["kind", "phone"],
|
||||
},
|
||||
SMS,
|
||||
],
|
||||
});
|
||||
expect(branches).toEqual([]);
|
||||
});
|
||||
|
||||
it("offers a member requiring a name it inherits from the root", () => {
|
||||
// The merge is what is judged, so a member requiring a name the ROOT
|
||||
// declares stays offerable — the `anyOf: [{ required: ["email"] }, …]`
|
||||
// shape above must not regress.
|
||||
const { branches } = resolveRootUnion({
|
||||
type: "object",
|
||||
properties: { token: { type: "string" } },
|
||||
anyOf: [
|
||||
{
|
||||
type: "object",
|
||||
properties: { a: { type: "string" } },
|
||||
required: ["token"],
|
||||
},
|
||||
{
|
||||
type: "object",
|
||||
properties: { b: { type: "string" } },
|
||||
required: ["token"],
|
||||
},
|
||||
],
|
||||
});
|
||||
expect(branches).toHaveLength(2);
|
||||
});
|
||||
|
||||
it("does not read an inherited name as a declared property", () => {
|
||||
// An argument legally named `constructor` resolves through the prototype
|
||||
// on a plain object, so a membership test that is not `hasOwn` would call
|
||||
// this branch offerable and render nothing for it.
|
||||
const { branches } = resolveRootUnion({
|
||||
type: "object",
|
||||
anyOf: [
|
||||
{
|
||||
type: "object",
|
||||
properties: { kind: { const: "a" } },
|
||||
required: ["constructor"],
|
||||
},
|
||||
SMS,
|
||||
],
|
||||
});
|
||||
expect(branches).toEqual([]);
|
||||
});
|
||||
|
||||
it("declines a member carrying a `false` property schema", () => {
|
||||
// `false` admits no value at all, so the field can never be filled — and
|
||||
// a required one makes the branch unsatisfiable.
|
||||
|
|
|
|||
Loading…
Reference in a new issue