diff --git a/.changeset/view-union-and-container-issue-diagnostics.md b/.changeset/view-union-and-container-issue-diagnostics.md new file mode 100644 index 0000000000..59a187202e --- /dev/null +++ b/.changeset/view-union-and-container-issue-diagnostics.md @@ -0,0 +1,60 @@ +--- +"@objectstack/spec": minor +"@objectstack/rest": patch +"@objectstack/cli": patch +--- + +fix(spec,rest,cli): validation diagnostics reach the real defect — named view-union branches, and `invalid_key` / `invalid_element` descent (#6391, #5389) + +Two cases where a refusal fired correctly but its *diagnostic* could not reach the +element that actually failed. Both fixes change the DIAGNOSTIC face only: every +input that parsed before parses after, every input refused before is refused +after, and each refusal keeps its issue codes (ADR-0112 / #6142 — a better +diagnostic never weakens the envelope). Pinned in both directions. + +**#6391 — `ViewMetadataSchema`'s union members are now contractual.** Three of +its four members were inline expressions with no name, so a consumer diagnosing a +failure could only reach a branch by indexing the nested `invalid_union` +`errors[]` **by member position**; objectui shipped exactly that and had to hold +the coupling down with a canary test (objectui#3606 / PR objectui#3624). The +union is now built from a named record: + +- `VIEW_METADATA_BRANCHES` — the branch names, in the union's own order; +- `VIEW_METADATA_MEMBERS` — branch name → the schema the union actually holds + (`viewItem` is identically `ViewItemWireSchema`, as before); +- `selectViewMetadataBranch(body)` — which branch a body claims, by the + discriminants the members already declare; +- `diagnoseViewMetadata(body)` — the failing branch **named**, with that branch's + own leaf issues and real field paths, so no consumer needs `errors[i]`. + +The union is **not** converted to `z.discriminatedUnion`. That would move the +acceptance face — a discriminated union refuses an unknown discriminant outright +where this one falls through all four members, and several of these shapes carry +no discriminant at all. `ViewMetadataSchema` remains the only judge of +acceptance; the dispatch only explains a verdict it did not make, and a pin +asserts the two never disagree. + +**#5389 — `invalid_key` / `invalid_element` are descended, in all three +consumers.** Zod hangs a failing record-key / map-element schema's real issues on +`issue.issues` — the same shape as `invalid_union`'s `issue.errors`, one property +name over. The family had already been fixed three times for `errors` (#4971, +#5014, #5341) and none of the three consumers read `issues`, so both codes +surfaced as a bare wrapper line with the prescription stranded in the payload. +Now `formatZodError`/`formatZodIssue` (spec), `zodIssuesToFields` (the REST wire) +and `formatZodErrors` (the CLI terminal) all descend it. + +Before / after, a `z.record` with a constrained key: + +``` + ✗ fields.First Name: Invalid key in record +``` +``` + ✗ fields.First Name: Invalid key in record + ✗ fields.First Name: Invalid identifier. Must be lowercase snake_case … +``` + +The expansion is strictly additive on every surface: the container's own line +(and, on the wire, its own `{field, code: 'invalid_shape', message}` entry) is +unchanged, and the leaves follow it. Unlike a union's branches — competing +candidates, therefore ranked and capped — a container's `issues` are the one list +the inner schema produced, so every one of them is reported. diff --git a/packages/cli/src/utils/format.ts b/packages/cli/src/utils/format.ts index a446826a6d..ac1f87ee61 100644 --- a/packages/cli/src/utils/format.ts +++ b/packages/cli/src/utils/format.ts @@ -208,18 +208,37 @@ export function createTimer() { // ─── Zod Error Formatting ─────────────────────────────────────────── /** - * How far the branch lines of an expanded union are pushed to sit UNDER the - * `code: message` line this file prints for the union itself. + * How far the nested lines of an expanded issue are pushed to sit UNDER the + * `code: message` line this file prints for the issue itself. * * `formatZodIssue` indents its own depth-0 line by 2 spaces and each nested * level by 2 more; this file's per-issue block is at 4/6. Adding 4 puts the - * first branch level at 8 — one step below the `invalid_union: Invalid input` + * first nested level at 8 — one step below the `invalid_union: Invalid input` * line it explains — and keeps every deeper level nested relative to it. */ -const UNION_BRANCH_REINDENT = ' '; +const NESTED_ISSUE_REINDENT = ' '; /** - * The lines that explain an `invalid_union`, or nothing at all. + * [#5389] The issue codes whose real diagnosis is nested one level down, and + * which `formatZodIssue` therefore renders as more than one line. + * + * `invalid_union` puts each candidate branch on `issue.errors[]`; `invalid_key` + * / `invalid_element` put the key/element schema's own issues on + * `issue.issues[]`. Same defect, two property names — and the gate below has to + * name both, or the terminal keeps printing `invalid_key: Invalid key in + * record` with the prescription stranded in the payload. + * + * Kept in step with `CONTAINER_ISSUE_CODES` in `@objectstack/spec`'s + * `error-map.zod.ts`, which is where the descent itself lives. + */ +const EXPANDABLE_ISSUE_CODES: ReadonlySet = new Set([ + 'invalid_union', + 'invalid_key', + 'invalid_element', +]); + +/** + * The lines that explain an expandable issue, or nothing at all. * * Zod folds every branch of a failed union into ONE issue whose own `message` * is the literal `"Invalid input"`; each branch's real rejection sits in @@ -241,18 +260,24 @@ const UNION_BRANCH_REINDENT = ' '; * so had to re-implement the ranking — the terminal needs exactly the STRING * that spec already exports, so here the reuse is a plain import. * - * Line 0 of that render is the union's own verdict, which the caller has + * [#5389] The same import now also covers `invalid_key` / `invalid_element`, + * whose diagnosis hangs on `issue.issues[]` instead. Widening the gate is the + * WHOLE of this file's share of that fix — the descent is spec's, so the + * terminal inherits it the moment it stops refusing to ask. + * + * Line 0 of that render is the issue's own verdict, which the caller has * already printed in this file's own idiom; only the explanation is returned, * so the change is strictly ADDITIVE — nothing that printed before #5341 stops - * printing. A non-union issue renders as a single line, hence never reaches - * here and could not add one anyway. + * printing. An issue with nothing nested renders as a single line, hence never + * reaches here and could not add one anyway. */ -function unionBranchLines(issue: unknown): string[] { - if ((issue as { code?: unknown } | null)?.code !== 'invalid_union') return []; +function nestedIssueLines(issue: unknown): string[] { + const code = (issue as { code?: unknown } | null)?.code; + if (typeof code !== 'string' || !EXPANDABLE_ISSUE_CODES.has(code)) return []; return formatZodIssue(issue as Parameters[0]) .split('\n') .slice(1) - .map((line) => `${UNION_BRANCH_REINDENT}${line}`); + .map((line) => `${NESTED_ISSUE_REINDENT}${line}`); } export function formatZodErrors(error: ZodError) { @@ -291,8 +316,9 @@ export function formatZodErrors(error: ZodError) { console.log(chalk.dim(` received: ${chalk.red((issue as any).received)}`)); } - // [#5341] …and, for a union, the branch that actually explains it. - for (const line of unionBranchLines(issue)) { + // [#5341] …and, for a union — [#5389] or a record key / map element — + // the nested issues that actually explain it. + for (const line of nestedIssueLines(issue)) { console.log(chalk.dim(line)); } } diff --git a/packages/cli/test/format-zod-union.test.ts b/packages/cli/test/format-zod-union.test.ts index 9b996eb1df..a96cbff70e 100644 --- a/packages/cli/test/format-zod-union.test.ts +++ b/packages/cli/test/format-zod-union.test.ts @@ -224,3 +224,42 @@ describe('[#5341] `os validate` delivers a union branch prescription', () => { expect(JSON.stringify(payload.errors[0].errors)).toContain('`direction` → `order`'); }, 120_000); }); + +/** + * [#5389] The terminal's share of the 4th/5th family members. + * + * `invalid_key` / `invalid_element` hang the key/element schema's real + * complaint on `issue.issues[]` rather than on `invalid_union`'s + * `issue.errors[]`. The descent itself is `@objectstack/spec`'s — reused, not + * re-derived, exactly as #5341 reused it for the union — so this file's whole + * share of the fix is that its gate stopped saying `code !== 'invalid_union'` + * and started naming all three expandable codes. + * + * Strictly additive, same as #5341: the `code: message` line the terminal + * already printed for the container is untouched; only the explanation is new. + */ +describe('[#5389] formatZodErrors expands invalid_key / invalid_element', () => { + const SnakeKey = z + .string() + .regex(/^[a-z][a-z0-9_]*$/, "Invalid identifier. Must be lowercase snake_case (e.g. 'first_name')."); + + it('prints the key schema prose under the container line', () => { + const schema = z.object({ fields: z.record(SnakeKey, z.object({ type: z.string() })) }); + const out = render(schema.safeParse({ fields: { 'First Name': { type: 'text' } } }).error!); + expect(out).toContain('invalid_key: Invalid key in record'); + expect(out).toContain("Invalid identifier. Must be lowercase snake_case (e.g. 'first_name')."); + }); + + it('prints the element schema prose for a map', () => { + const schema = z.map(z.object({ id: z.string() }), z.number().min(5, 'too small')); + const out = render(schema.safeParse(new Map([[{ id: 'a' }, 1]])).error!); + expect(out).toContain('invalid_element'); + expect(out).toContain('too small'); + }); + + it('adds nothing to an ordinary issue', () => { + // The conservative half: only the three expandable codes may grow lines. + const before = render(z.object({ a: z.string() }).safeParse({}).error!); + expect(before.split('\n').filter((l) => l.includes('✗'))).toHaveLength(1); + }); +}); diff --git a/packages/rest/src/rest-server.ts b/packages/rest/src/rest-server.ts index 783503a074..a86e1ec633 100644 --- a/packages/rest/src/rest-server.ts +++ b/packages/rest/src/rest-server.ts @@ -212,8 +212,8 @@ function valueAtPath(input: unknown, path: unknown): unknown { } /** - * How many levels of nested `invalid_union` are expanded below a top-level - * issue, and how many equally-informative branches are emitted at one level. + * How many levels of nested issues are expanded below a top-level issue, and + * how many equally-informative union branches are emitted at one level. * * Both bounds — and the whole selection policy below — are the ones * `formatZodError` landed for the CLI/spec side of this defect (#4971, @@ -223,9 +223,22 @@ function valueAtPath(input: unknown, path: unknown): unknown { * the same, or one mistake gets two different prescriptions depending on whether * the author published from the terminal or POSTed to the API (#5014). */ -const UNION_EXPANSION_DEPTH_LIMIT = 3; +const NESTED_EXPANSION_DEPTH_LIMIT = 3; const UNION_BRANCH_EMIT_LIMIT = 3; +/** + * [#5389] The issue codes that hang their real diagnosis on `issue.issues` + * rather than on `invalid_union`'s `issue.errors`. + * + * `invalid_key` is raised when `z.record(K, V)`'s KEY schema rejects a key (and + * by `z.map` for a non-`PropertyKey` key); `invalid_element` when `z.map`'s + * VALUE schema rejects the value under such a key. Both carry a bare wrapper + * message ("Invalid key in record") with everything the client needs one level + * down — the same defect as #5014, one property name over. Kept in step with + * `CONTAINER_ISSUE_CODES` in `spec/src/shared/error-map.zod.ts`. + */ +const CONTAINER_ISSUE_CODES: ReadonlySet = new Set(['invalid_key', 'invalid_element']); + /** A Zod issue path, normalised to the array Zod always produces. */ function issuePathOf(issue: any): Array { return Array.isArray(issue?.path) ? issue.path : []; @@ -308,6 +321,14 @@ function selectUnionBranches(branches: readonly (readonly any[])[]): readonly (r * the branches that explain it, with `field` resolved against the union's own * path — branch paths are relative to it. * + * [#5389] An `invalid_key` / `invalid_element` behaves the same way one property + * name over: its own entry (zod's `"Invalid key in record"`, also + * `invalid_shape`) followed by the entries on `issue.issues`, whose paths are + * likewise relative. The one difference from a union: those issues are not + * competing candidates, so they are NOT ranked or capped — every one of them is + * a true statement about the value, and dropping any would be dropping a real + * diagnosis rather than declining to guess. + * * The union's entry is kept rather than replaced: it is the only entry naming * the slot the client sent, existing clients already read it, and when every * branch is uninformative it is still the whole answer. So the expansion is @@ -343,7 +364,11 @@ function collectIssueFields( const branches: readonly (readonly any[])[] = issue?.code === 'invalid_union' && Array.isArray(issue?.errors) ? issue.errors.filter((branch: unknown): branch is any[] => Array.isArray(branch)) : []; - const expandable = branches.length > 0 && depth < UNION_EXPANSION_DEPTH_LIMIT; + const contained: readonly any[] = CONTAINER_ISSUE_CODES.has(issue?.code) && Array.isArray(issue?.issues) + ? issue.issues + : []; + const expandable = (branches.length > 0 || contained.length > 0) + && depth < NESTED_EXPANSION_DEPTH_LIMIT; const entry = { field, @@ -361,10 +386,17 @@ function collectIssueFields( out.push(entry); if (!expandable) return; - for (const branch of selectUnionBranches(branches)) { - for (const nested of branch) { - collectIssueFields(nested, path, depth + 1, seen, input, inputProvided, out); + if (branches.length > 0) { + for (const branch of selectUnionBranches(branches)) { + for (const nested of branch) { + collectIssueFields(nested, path, depth + 1, seen, input, inputProvided, out); + } } + return; + } + + for (const nested of contained) { + collectIssueFields(nested, path, depth + 1, seen, input, inputProvided, out); } } diff --git a/packages/rest/src/zod-union-fields.test.ts b/packages/rest/src/zod-union-fields.test.ts index 54afe399c4..90fbd17461 100644 --- a/packages/rest/src/zod-union-fields.test.ts +++ b/packages/rest/src/zod-union-fields.test.ts @@ -238,3 +238,85 @@ describe('zodIssuesToFields — invalid_union expansion (#5014)', () => { }); }); }); + +/** + * [#5389] `invalid_key` / `invalid_element` — the same defect, one property + * name over, on the same wire. + * + * `invalid_union` buries a branch's real complaint in `issue.errors[]`; these + * two bury the key/element schema's real complaint in `issue.issues[]`. Before + * this, a client POSTing a record with a bad KEY got exactly one `fields[]` + * entry — `{field: 'fields.First Name', code: 'invalid_shape', message: + * 'Invalid key in record'}` — with the sentence naming the rule produced and + * dropped, which is #5014 verbatim. + * + * The expansion is strictly ADDITIVE, same contract as #5014: the container's + * own entry stays (it is the only one naming the slot, and existing clients + * read it) and the leaf entries follow it. ADR-0114's `{field, code, message}` + * shape is unchanged, and the container entry keeps mapping to `invalid_shape`. + * + * Every fixture drives a REAL `safeParse`, for the reason the file's header + * gives: `issue.issues` is Zod's internal shape. + */ +describe('zodIssuesToFields — invalid_key / invalid_element descent (#5389)', () => { + const fieldsFor = (schema: z.ZodType, value: unknown) => { + const r = schema.safeParse(value); + expect(r.success, 'the fixture must actually fail to parse').toBe(false); + return zodIssuesToFields((r as { error: { issues: unknown[] } }).error.issues, value); + }; + + const SnakeKey = z + .string() + .regex(/^[a-z][a-z0-9_]*$/, "Invalid identifier. Must be lowercase snake_case (e.g. 'first_name')."); + + it('delivers the KEY schema prescription beside the container entry', () => { + const schema = z.object({ fields: z.record(SnakeKey, z.object({ type: z.string() })) }); + const fields = fieldsFor(schema, { fields: { 'First Name': { type: 'text' } } }); + + expect(fields).toEqual([ + // Unchanged — the entry clients read today. + { field: 'fields.First Name', code: 'invalid_shape', message: 'Invalid key in record' }, + // …and the leaf that says WHY, with the container's path resolved. + { + field: 'fields.First Name', + code: 'invalid_format', + message: "Invalid identifier. Must be lowercase snake_case (e.g. 'first_name').", + }, + ]); + }); + + it('emits EVERY nested issue — a container has no branches to rank', () => { + // Deliberate divergence from the union descent: `errors[]` holds + // competing candidates, so it is ranked and capped; `issues[]` is the + // one list the element schema produced, and each entry is a real field. + const schema = z.map( + z.object({ id: z.string() }), + z.object({ a: z.string(), b: z.string(), c: z.string() }), + ); + const fields = fieldsFor(schema, new Map([[{ id: 'x' }, {}]])); + expect(fields[0]).toMatchObject({ code: 'invalid_shape' }); + expect(fields.map((f) => f.field)).toEqual(['', 'a', 'b', 'c']); + }); + + it('keeps the catalog code an ADR-0114 member on every entry', () => { + const schema = z.object({ fields: z.record(SnakeKey, z.unknown()) }); + const codes = FieldErrorCode.options as readonly string[]; + for (const entry of fieldsFor(schema, { fields: { 'Bad Key': 1 } })) { + expect(codes).toContain(entry.code); + } + }); + + it('leaves an ordinary issue exactly as it was', () => { + const schema = z.object({ a: z.string() }); + expect(fieldsFor(schema, {})).toEqual([ + { field: 'a', code: 'required', message: 'Invalid input: expected string, received undefined' }, + ]); + }); + + it('still tolerates a container issue with no nested list', () => { + expect(zodIssuesToFields([{ code: 'invalid_key', path: ['m', 'k'], message: 'Invalid key in record' }])) + .toEqual([{ field: 'm.k', code: 'invalid_shape', message: 'Invalid key in record' }]); + expect(zodIssuesToFields([{ code: 'invalid_element', path: ['m'], message: 'Invalid value', issues: 'nope' }])) + .toEqual([{ field: 'm', code: 'invalid_shape', message: 'Invalid value' }]); + }); +}); diff --git a/packages/spec/api-surface/ui.json b/packages/spec/api-surface/ui.json index 3e8204d5e8..d53ff887f8 100644 --- a/packages/spec/api-surface/ui.json +++ b/packages/spec/api-surface/ui.json @@ -329,6 +329,8 @@ "VIEW_CONSOLE_ROW_DECORATIONS (const)", "VIEW_FILTER_OPERATORS (const)", "VIEW_FILTER_OPERATOR_ALIASES (const)", + "VIEW_METADATA_BRANCHES (const)", + "VIEW_METADATA_MEMBERS (const)", "VIEW_WRITE_PATH_IDENTITY_KEYS (const)", "View (type)", "ViewData (type)", @@ -348,6 +350,8 @@ "ViewKind (type)", "ViewKindSchema (const)", "ViewMetadata (type)", + "ViewMetadataBranch (type)", + "ViewMetadataDiagnosis (type)", "ViewMetadataParsed (type)", "ViewMetadataSchema (const)", "ViewParsed (type)", @@ -382,6 +386,7 @@ "defineTheme (function)", "defineView (function)", "defineViewItem (function)", + "diagnoseViewMetadata (function)", "expandViewContainer (function)", "expandViewContainerWithDiagnostics (function)", "isAggregatedViewContainer (function)", @@ -393,6 +398,7 @@ "reportForm (const)", "reportSelectionOrder (function)", "resolveI18nLabel (function)", + "selectViewMetadataBranch (function)", "stripViewConsoleDecorations (function)", "validateActionParams (function)", "viewForm (const)" diff --git a/packages/spec/src/shared/error-map.test.ts b/packages/spec/src/shared/error-map.test.ts index 9ab90dc3f5..7d5bb57b6b 100644 --- a/packages/spec/src/shared/error-map.test.ts +++ b/packages/spec/src/shared/error-map.test.ts @@ -356,3 +356,105 @@ describe('safeParsePretty', () => { } }); }); + +// ─────────────────────────────────────────────────────────────────────────── +// [#5389] `invalid_key` / `invalid_element` — the 4th and 5th members of the +// same family, one property name over. +// +// `invalid_union` hides a branch's real complaint on `issue.errors[]`; these two +// hide the key/element schema's real complaint on `issue.issues[]`. Same defect, +// same author-facing symptom (a bare wrapper line with the prescription stranded +// in the payload), and the family had already been fixed three times for the +// FIRST property name — #4971 here, #5014 on the REST wire, #5341 in the CLI — +// while these two were never read by any of the three. +// +// Zod's OWN formatters (`treeifyError`, `formatError`) descend both codes with +// `[...path, ...issue.path]` as the parent path, which is the semantics mirrored +// here. +// +// ⚠️ Every fixture drives a REAL `safeParse`. `issue.issues` is zod's internal +// shape, so a hand-written issue would keep this file green after an upgrade +// moved it while the author's terminal went back to saying nothing. +// +// Reachability, measured on zod 4.4.3 (`v4/core/schemas.js`): +// • `z.record(K, V)` raises `invalid_key` when the KEY schema rejects a key. +// • `z.map(K, V)` raises `invalid_key` / `invalid_element` only for keys that +// are not `PropertyKey`s — a `string`-keyed map prefixes the issue instead. +// • `z.set(V)` never raises `invalid_element`; it flattens. +// The `packages/spec` authoring surface produces none of these today (every +// `z.record` key schema there is `z.string()` or an enum — #5389's dormancy +// table), which is why these fixtures are local schemas: the defect is in the +// CONSUMER, and the consumer is reachable from any caller's schema. +describe('[#5389] formatZodError descends invalid_key / invalid_element', () => { + const SnakeKey = z + .string() + .regex(/^[a-z][a-z0-9_]*$/, "Invalid identifier. Must be lowercase snake_case (e.g. 'first_name')."); + + it('renders the KEY schema prose under the record line', () => { + const schema = z.object({ fields: z.record(SnakeKey, z.object({ type: z.string() })) }); + const result = schema.safeParse({ fields: { 'First Name': { type: 'text' } } }); + expect(result.success).toBe(false); + + const formatted = formatZodError(result.error!, 'Stack validation failed'); + // The wrapper line is PRESERVED — it is what names the slot, and keeping it + // makes this strictly additive, exactly as #4971 kept the union's own line. + expect(formatted).toContain('✗ fields.First Name: Invalid key in record'); + // …and the prescription now arrives with it, indented one level. + expect(formatted).toContain( + " ✗ fields.First Name: Invalid identifier. Must be lowercase snake_case (e.g. 'first_name').", + ); + }); + + it('resolves the nested path against the container, not relative to it', () => { + // The key schema's own issues carry `path: []`; the container issue carries + // `['fields', '']`. A naive splice would print `(root)`. + const schema = z.object({ fields: z.record(SnakeKey, z.unknown()) }); + const formatted = formatZodError(schema.safeParse({ fields: { 'Bad Key': 1 } }).error!); + expect(formatted).not.toContain('✗ (root):'); + }); + + it('descends invalid_element on a map with non-PropertyKey keys', () => { + const schema = z.map(z.object({ id: z.string() }), z.number().min(5, 'too small')); + const result = schema.safeParse(new Map([[{ id: 'a' }, 1]])); + expect(result.success).toBe(false); + expect(result.error!.issues[0]!.code).toBe('invalid_element'); + + const formatted = formatZodError(result.error!); + expect(formatted).toContain('too small'); + }); + + it('renders EVERY nested issue — a container has no branches to choose between', () => { + // The one deliberate difference from the union descent: `errors[]` holds + // competing candidates, so it is ranked and capped; `issues[]` is the one + // list the key schema produced, and every line of it is true. + const schema = z.map( + z.object({ id: z.string() }), + z.object({ a: z.string(), b: z.string(), c: z.string() }), + ); + const formatted = formatZodError(schema.safeParse(new Map([[{ id: 'x' }, {}]])).error!); + // Three missing requireds, three lines — none ranked away. + expect(formatted.match(/received undefined/g)?.length).toBe(3); + for (const key of ['a', 'b', 'c']) expect(formatted).toContain(`✗ ${key}:`); + }); + + it('counts the container as the one issue zod raised', () => { + const schema = z.object({ fields: z.record(SnakeKey, z.unknown()) }); + const result = schema.safeParse({ fields: { 'Bad Key': 1 } }); + expect(result.error!.issues).toHaveLength(1); + expect(formatZodError(result.error!)).toContain('(1 issue)'); + }); + + it('leaves an ordinary issue byte-identical', () => { + // The conservative half: nothing that is not one of these two codes may + // gain or lose a line because the descent exists. + const formatted = formatZodError(z.object({ a: z.string() }).safeParse({}).error!); + expect(formatted).toBe( + 'Validation failed (1 issue):\n\n ✗ a: Invalid input: expected string, received undefined', + ); + }); + + it('tolerates a container issue with no nested list', () => { + expect(formatZodIssue({ code: 'invalid_key', path: ['m', 'k'], message: 'Invalid key in record' })) + .toBe(' ✗ m.k: Invalid key in record'); + }); +}); diff --git a/packages/spec/src/shared/error-map.zod.ts b/packages/spec/src/shared/error-map.zod.ts index 3c6b855919..7ca0657306 100644 --- a/packages/spec/src/shared/error-map.zod.ts +++ b/packages/spec/src/shared/error-map.zod.ts @@ -140,19 +140,54 @@ interface ZodIssueMinimal { * so everything a failing branch has to say lives down here. */ errors?: readonly (readonly ZodIssueMinimal[])[]; + /** + * [#5389] Only on `invalid_key` / `invalid_element`: the ONE issue list the + * failing key / element schema produced, with paths RELATIVE to this issue's + * own path — the same relationship `errors` has, spelled with a different + * property name and without the per-branch nesting (a container has one + * inner schema, not N alternatives). + * + * The distinction that makes this a separate field rather than a second + * shape of `errors`: a union's branches are *candidates* and have to be + * ranked ({@link selectUnionBranches}), while these issues are simply what + * went wrong — every one of them is true and none of them is speculative. + */ + issues?: readonly ZodIssueMinimal[]; } +/** + * [#5389] The issue codes that hang their real diagnosis on `issue.issues`. + * + * Zod raises these when a CONTAINER's inner schema rejects a key or an element + * that cannot be addressed by a path segment: + * + * - `invalid_key` — `z.record(K, V)`'s **key** schema rejected a key, and + * `z.map(K, V)`'s key schema rejected a non-`PropertyKey` key; + * - `invalid_element` — `z.map(K, V)`'s **value** schema rejected the value + * under a non-`PropertyKey` key. + * + * In both cases the issue's own `message` is a bare wrapper ("Invalid key in + * record") and everything the author needs sits one level down, exactly as + * `invalid_union` hides a branch's prescription in `errors` (#4971). Zod's own + * `treeifyError` / `formatError` descend both codes with `[...path, + * ...issue.path]` as the parent path; this renderer did not, which is the + * defect #5389 records. + */ +const CONTAINER_ISSUE_CODES: ReadonlySet = new Set(['invalid_key', 'invalid_element']); + /** One indent step of a formatted issue line. */ const ISSUE_INDENT = ' '; /** - * How many levels of nested `invalid_union` are expanded below a top-level - * issue. Unions nest (a union member that is itself a union — `StateMachine → - * on.GO → actions[0]` is two levels in this repo today), and each level can + * How many levels of nested issues are expanded below a top-level issue — + * `invalid_union` branches and, since #5389, `invalid_key` / `invalid_element` + * container issues alike. Both nest (a union member that is itself a union — + * `StateMachine → on.GO → actions[0]` is two levels in this repo today; a + * record whose value schema is a union is another), and a union level can * render several branches, so the expansion is bounded rather than left to the * shape of whatever the author typed. */ -const UNION_EXPANSION_DEPTH_LIMIT = 3; +const NESTED_EXPANSION_DEPTH_LIMIT = 3; /** How many equally-informative branches are rendered at one level. */ const UNION_BRANCH_RENDER_LIMIT = 3; @@ -243,13 +278,21 @@ function renderPath(path: PropertyKey[]): string { } /** - * Render one issue and — for `invalid_union` — the selected branches beneath - * it, one indent level deeper, with paths resolved against the union's own. + * Render one issue and — for `invalid_union`, and since #5389 for + * `invalid_key` / `invalid_element` — the issues beneath it, one indent level + * deeper, with paths resolved against the parent issue's own. + * + * The two descents differ in exactly one way, and it is the reason they are not + * merged into one loop: a union's branches are competing CANDIDATES, so they + * are ranked and capped ({@link selectUnionBranches}) to keep one mistake from + * being reported once per member; a container's `issues` are the one list the + * key/element schema actually produced, so every one of them is rendered — + * there is no branch to choose between and nothing to omit. * * `seen` de-duplicates leaf lines *within one top-level issue*: two branches - * that reject the same key with the same words say it once. Union lines - * themselves are never de-duplicated, since two same-path `"Invalid input"` - * lines can head genuinely different sub-trees. + * that reject the same key with the same words say it once. Expanded lines + * (union and container heads) are themselves never de-duplicated, since two + * same-path wrapper lines can head genuinely different sub-trees. */ function renderIssue( issue: ZodIssueMinimal, @@ -260,7 +303,9 @@ function renderIssue( const path = [...parentPath, ...issue.path]; const rendered = renderPath(path); const branches = issue.code === 'invalid_union' ? (issue.errors ?? []) : []; - const expandable = branches.length > 0 && depth < UNION_EXPANSION_DEPTH_LIMIT; + const contained = issue.code && CONTAINER_ISSUE_CODES.has(issue.code) ? (issue.issues ?? []) : []; + const expandable = + (branches.length > 0 || contained.length > 0) && depth < NESTED_EXPANSION_DEPTH_LIMIT; if (!expandable) { const key = JSON.stringify([depth, rendered, issue.message]); @@ -271,16 +316,23 @@ function renderIssue( const lines = [`${ISSUE_INDENT.repeat(depth + 1)}✗ ${rendered}: ${issue.message}`]; if (!expandable) return lines; - const { selected, omitted } = selectUnionBranches(branches); - for (const branch of selected) { - for (const nested of branch) { - lines.push(...renderIssue(nested, path, depth + 1, seen)); + if (branches.length > 0) { + const { selected, omitted } = selectUnionBranches(branches); + for (const branch of selected) { + for (const nested of branch) { + lines.push(...renderIssue(nested, path, depth + 1, seen)); + } + } + if (selected.length > 0 && omitted > 0) { + lines.push( + `${ISSUE_INDENT.repeat(depth + 2)}… and ${omitted} more branch${omitted === 1 ? '' : 'es'} rejected this value`, + ); } + return lines; } - if (selected.length > 0 && omitted > 0) { - lines.push( - `${ISSUE_INDENT.repeat(depth + 2)}… and ${omitted} more branch${omitted === 1 ? '' : 'es'} rejected this value`, - ); + + for (const nested of contained) { + lines.push(...renderIssue(nested, path, depth + 1, seen)); } return lines; } @@ -339,9 +391,22 @@ export function formatZodIssue(issue: ZodIssueMinimal): string { * ✗ states.s.on.GO.actions.0: Invalid input * ✗ states.s.on.GO.actions.0: Unrecognized key(s) on this action reference: `args`. … * ``` - * The issue **count** stays the count of `error.issues` — the union is one - * issue no matter how many lines explain it, which keeps this header agreeing - * with the structural consumers (REST error bodies, `ZodError.message`). + * + * [#5389] `invalid_key` / `invalid_element` hide their diagnosis the same way — + * on `issue.issues` rather than `issue.errors` — and are expanded the same way, + * so a constrained `z.record` KEY reaches the author as the key schema's own + * words instead of zod's bare `"Invalid key in record"`: + * ``` + * Validation failed (1 issue): + * + * ✗ fields.First Name: Invalid key in record + * ✗ fields.First Name: Invalid identifier 'First Name'. Must be lowercase snake_case … + * ``` + * + * The issue **count** stays the count of `error.issues` — the union (or the + * container) is one issue no matter how many lines explain it, which keeps this + * header agreeing with the structural consumers (REST error bodies, + * `ZodError.message`). */ export function formatZodError(error: z.ZodError, label?: string): string { const count = error.issues.length; diff --git a/packages/spec/src/ui/view-union-diagnostics.test.ts b/packages/spec/src/ui/view-union-diagnostics.test.ts new file mode 100644 index 0000000000..2a2c53cedd --- /dev/null +++ b/packages/spec/src/ui/view-union-diagnostics.test.ts @@ -0,0 +1,336 @@ +// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#6391] `ViewMetadataSchema`'s union: contractual member handles + a + * branch-naming diagnostic entry, with the ACCEPTANCE face pinned unmoved. + * + * ## What was measured, and by whom + * + * objectui's metadata-admin editor validates `view` bodies against + * `ViewMetadataSchema` (objectstack#5316's ruling). When that fails, zod raises + * ONE root `invalid_union` issue and buries every per-field truth in nested + * `errors[]`. objectui#3624 shipped an expansion for it — and could only reach + * a branch BY MEMBER POSITION, because three of the four members were inline + * expressions with no exported name. Position is a spec-internal detail nobody + * promised, so that PR had to pin it with a CANARY test that rings on every + * reorder. objectstack#6391 records the cost and asks spec to remove the need. + * + * ## What this file pins + * + * 1. **Identity by construction.** The union's `options` ARE + * {@link VIEW_METADATA_MEMBERS}' values, same objects, same order. This is + * the assertion objectui's canary was standing in for; it now lives on the + * side that can actually break it. + * 2. **A branch names itself.** {@link diagnoseViewMetadata} returns the branch + * NAME and that branch's own leaf issues — no `errors[i]`, no position. + * 3. ⛔ **The acceptance face did not move.** Every body below is asserted in + * BOTH directions: what parsed before parses now, what was refused before is + * refused now, and the refusals still carry their issue codes. The union + * gained a diagnostic dispatch, NOT a discriminant — a `z.discriminatedUnion` + * would refuse an unknown discriminant outright where this union still falls + * through all four members, which is a membership change and is exactly what + * #7025's sweep rule forbids. + * + * The expected values below were measured on `origin/main` @ `0f539bd` before + * the change and re-measured after: 42 bodies, identical verdicts, identical + * parse output, identical issue-code sets (report sha256 + * `fca9df8937bbb9f736f11895a6e1ddf23b7fb25d9b13cfa7e67c71c8dfaaf2b2`). + */ + +import { describe, it, expect } from 'vitest'; +import { z } from 'zod'; +import { + ViewMetadataSchema, + ViewItemWireSchema, + VIEW_METADATA_BRANCHES, + VIEW_METADATA_MEMBERS, + selectViewMetadataBranch, + diagnoseViewMetadata, + type ViewMetadataBranch, +} from './view.zod'; + +const LIST_CFG = { type: 'grid', data: { provider: 'object', object: 'crm_lead' }, columns: ['name'] }; +const FORM_CFG = { type: 'simple', sections: [{ label: 'Main', fields: ['name'] }] }; + +/** The union zod actually runs, unwrapped from the `z.preprocess` pipe. */ +function unionOptions(): readonly unknown[] { + const def = (ViewMetadataSchema as unknown as { _zod: { def: Record } })._zod.def; + const out = def.out ?? def; + return (out._zod?.def?.options ?? []) as readonly unknown[]; +} + +describe('[#6391] the union members are contractual, not positional', () => { + it('exposes exactly four branches, in the union\'s own order', () => { + expect([...VIEW_METADATA_BRANCHES]).toEqual([ + 'viewItem', + 'container', + 'listOverlay', + 'formOverlay', + ]); + expect(Object.keys(VIEW_METADATA_MEMBERS)).toEqual([...VIEW_METADATA_BRANCHES]); + }); + + // ⚠️ THE pin. Before this change member 2 was `ViewSchema.refine(…)` written + // inline: key-for-key identical to the exported `ViewSchema` and NOT the same + // object, so a consumer selecting "the container branch" was selecting a + // resemblance. Now the union is BUILT from this record, so a reorder or a + // swap cannot make the two disagree — it is one declaration. + it('builds the union FROM the named members — same objects, same order', () => { + const options = unionOptions(); + expect(options).toHaveLength(VIEW_METADATA_BRANCHES.length); + VIEW_METADATA_BRANCHES.forEach((branch, index) => { + expect(options[index]).toBe(VIEW_METADATA_MEMBERS[branch]); + }); + }); + + it('keeps member 1 identically the already-exported ViewItemWireSchema', () => { + expect(VIEW_METADATA_MEMBERS.viewItem).toBe(ViewItemWireSchema); + }); + + it('lets a consumer validate against one named branch directly', () => { + // The route objectui had to reach for `errors[2]` to approximate. + expect(VIEW_METADATA_MEMBERS.container.safeParse({ object: 'crm_lead', list: LIST_CFG }).success).toBe(true); + expect(VIEW_METADATA_MEMBERS.container.safeParse({ object: 'crm_lead', listViews: {} }).success).toBe(false); + expect(VIEW_METADATA_MEMBERS.listOverlay.safeParse({ type: 'grid', columns: ['name'] }).success).toBe(true); + expect(VIEW_METADATA_MEMBERS.formOverlay.safeParse({ type: 'wizard' }).success).toBe(true); + }); +}); + +describe('[#6391] selectViewMetadataBranch reads the members\' own discriminants', () => { + const CASES: Array<[string, unknown, ViewMetadataBranch | null]> = [ + ['a nested config is a ViewItem record', { viewKind: 'list', config: LIST_CFG }, 'viewItem'], + ['…even when the config is the broken part', { viewKind: 'list', config: { type: 'grid', columns: 1 } }, 'viewItem'], + ['a container slot is a container', { object: 'a', list: LIST_CFG }, 'container'], + ['listViews too', { object: 'a', listViews: {} }, 'container'], + ['an inherited viewKind settles a flat overlay', { viewKind: 'form', title: 'T' }, 'formOverlay'], + ['so does a list-family type', { type: 'kanban' }, 'listOverlay'], + ['and a form-family type', { type: 'wizard' }, 'formOverlay'], + ['nothing settles a bare aux PUT', { isPinned: true }, null], + ['nor an unknown type', { type: 'sideways' }, null], + ['non-objects claim nothing', 'nope', null], + ]; + + for (const [label, body, expected] of CASES) { + it(label, () => expect(selectViewMetadataBranch(body)).toBe(expected)); + } + + // The `type` enums are read off the member schemas, never re-listed here, so + // a new view type cannot make the dispatch disagree with the members. + it('covers every declared list/form type', () => { + const listTypes = ['grid', 'kanban', 'gallery', 'calendar', 'timeline', 'gantt', 'map', 'chart', 'tree']; + const formTypes = ['simple', 'tabbed', 'wizard', 'split', 'drawer', 'modal']; + for (const type of listTypes) expect(selectViewMetadataBranch({ type })).toBe('listOverlay'); + for (const type of formTypes) expect(selectViewMetadataBranch({ type })).toBe('formOverlay'); + }); +}); + +describe('[#6391] diagnoseViewMetadata names the failing branch', () => { + // The worked example from the issue: a flattened list overlay whose `columns` + // is a string. Before this change the ONLY structural route to that leaf was + // `error.issues[0].errors[2]` — member position 2 — while the rendered + // message came from the CONTAINER branch (fewest issues wins) and told the + // author to wrap the body in `defineView`, which is not the defect. + const BAD_OVERLAY = { type: 'grid', columns: 'not-an-array' }; + + it('reports the leaf issue under the branch that owns it', () => { + const d = diagnoseViewMetadata(BAD_OVERLAY); + expect(d.success).toBe(false); + if (d.success) return; + expect(d.branch).toBe('listOverlay'); + expect(d.issues).toHaveLength(1); + expect(d.issues[0]!.path).toEqual(['columns']); + }); + + it('still shows the union hiding it behind member POSITION', () => { + // Kept as the contrast the fix exists for: this is what a consumer had to + // write, and what it no longer has to write. + const root = ViewMetadataSchema.safeParse(BAD_OVERLAY).error!.issues[0] as unknown as { + code: string; + errors: unknown[][]; + }; + expect(root.code).toBe('invalid_union'); + expect(root.errors).toHaveLength(4); + expect(root.errors[2]).toMatchObject([{ path: ['columns'] }]); + }); + + it('names the branch on success too', () => { + const d = diagnoseViewMetadata({ object: 'crm_lead', list: LIST_CFG }); + expect(d.success).toBe(true); + if (!d.success) return; + expect(d.branch).toBe('container'); + }); + + it('reports a broken ViewItem config against the viewItem branch', () => { + const d = diagnoseViewMetadata({ + name: 'a.b', + object: 'a', + viewKind: 'list', + config: { type: 'grid', columns: 'nope' }, + }); + expect(d.success).toBe(false); + if (d.success) return; + expect(d.branch).toBe('viewItem'); + expect(d.issues.some((i) => i.path.includes('config'))).toBe(true); + }); + + it('reports an empty container against the container branch', () => { + const d = diagnoseViewMetadata({ object: 'crm_lead', listViews: {} }); + expect(d.success).toBe(false); + if (d.success) return; + expect(d.branch).toBe('container'); + expect(d.issues[0]!.message).toContain('must define at least one of'); + }); + + // #5599's identity precondition short-circuits the pipe before the union + // runs. Naming a branch here would send the author to fix a shape they were + // never writing, so the contract is `branch: null`. + it('names NO branch for a body that is not a view at all', () => { + for (const body of [{ nope: 1 }, {}, { id: 'x' }, { name: 'garbage_view' }]) { + const d = diagnoseViewMetadata(body); + expect(d.success).toBe(false); + if (d.success) continue; + expect(d.branch).toBeNull(); + expect(d.issues[0]!.message).toContain('Not a `view` body'); + } + }); + + it('falls back to the least-complaining member when nothing settles the claim', () => { + // `{ isPinned: true, columns: 'nope' }` claims no branch by discriminant — + // every member is tried and the one with the fewest issues is reported, + // the same ranking `selectUnionBranches` uses for this family. + const d = diagnoseViewMetadata({ isPinned: true, columns: 'nope' }); + expect(d.success).toBe(false); + if (d.success) return; + expect(VIEW_METADATA_BRANCHES).toContain(d.branch!); + expect(d.issues.length).toBeGreaterThan(0); + }); + + // ⛔ The invariant that keeps this a DIAGNOSTIC and not a second gate. + it('never disagrees with ViewMetadataSchema about acceptance', () => { + const bodies: unknown[] = [ + { name: 'a.b', object: 'a', viewKind: 'list', config: LIST_CFG }, + { object: 'a', form: FORM_CFG }, + { type: 'grid', columns: ['name'] }, + { type: 'wizard' }, + { isPinned: true }, + { nope: 1 }, + {}, + 'nope', + 42, + null, + [LIST_CFG], + ]; + for (const body of bodies) { + expect(diagnoseViewMetadata(body).success).toBe(ViewMetadataSchema.safeParse(body).success); + } + }); +}); + +// ⛔ #7025's red line, asserted directly. A discriminated union changes error +// SHAPE, not membership — so membership is what this block pins, in both +// directions, on the corpus the before/after diff was measured over. +describe('[#7025] the acceptance face of ViewMetadataSchema did not move', () => { + const SECTION = { label: 'Main', collapsible: false, collapsed: false, columns: 1, fields: ['name'] }; + + /** + * Every entry is a verdict MEASURED on `origin/main` before the change and + * re-measured after — never a verdict predicted from reading the schema. Some + * of them are surprising, and they are in the table precisely because a + * surprising accept is the one a refactor silently loses: + * + * - `overlay.badOperator` / `overlay.console*Id` ACCEPT as `{type:'simple'}` — + * the form overlay does not declare `filter`/`sort`, and `.strip()` drops + * them. Pre-existing, load-bearing for Studio's round-trip, and untouched. + * - `overlay.list.min` (`{type:'grid'}` alone) REJECTS. + * - `container.empty` REJECTS with `custom`, not `invalid_union`: when + * exactly one branch is non-aborted zod's `handleUnionResults` returns THAT + * branch's issues verbatim instead of wrapping them — one more reason a + * consumer cannot rely on the wrapper's shape, which is #6391's point. + */ + const ACCEPTED: Array<[string, unknown, unknown]> = [ + ['viewItem.list', { name: 'crm_lead.all', object: 'crm_lead', viewKind: 'list', config: LIST_CFG }, + { viewKind: 'list', config: LIST_CFG, name: 'crm_lead.all', object: 'crm_lead' }], + ['viewItem.form', { name: 'crm_lead.edit', object: 'crm_lead', viewKind: 'form', config: FORM_CFG }, + { viewKind: 'form', config: { type: 'simple', sections: [SECTION] }, name: 'crm_lead.edit', object: 'crm_lead' }], + ['viewItem.wire.aux', { name: 'a.b', object: 'a', viewKind: 'list', config: LIST_CFG, isPinned: true, sortOrder: 3 }, + { viewKind: 'list', config: LIST_CFG, name: 'a.b', object: 'a', isPinned: true, sortOrder: 3 }], + ['container.list', { object: 'crm_lead', list: LIST_CFG }, { object: 'crm_lead', list: LIST_CFG }], + ['container.form', { object: 'crm_lead', form: FORM_CFG }, { object: 'crm_lead', form: { type: 'simple', sections: [SECTION] } }], + ['container.listViews', { object: 'crm_lead', listViews: { my: LIST_CFG } }, { object: 'crm_lead', listViews: { my: LIST_CFG } }], + ['container.formViews', { object: 'crm_lead', formViews: { my: FORM_CFG } }, + { object: 'crm_lead', formViews: { my: { type: 'simple', sections: [SECTION] } } }], + ['overlay.list.columns', { columns: ['name', 'stage'] }, { type: 'grid', columns: ['name', 'stage'] }], + ['overlay.list.aux', { type: 'grid', columns: ['name'], isDefault: true, order: 2, hidden: false }, + { type: 'grid', columns: ['name'], isDefault: true, order: 2, hidden: false }], + ['overlay.form.min', { type: 'simple' }, { type: 'simple' }], + ['overlay.form.sections', { sections: [{ label: 'Main', fields: ['name'] }] }, { type: 'simple', sections: [SECTION] }], + ['overlay.form.identity', { name: 'x', object: 'crm_lead', viewKind: 'form', type: 'wizard' }, + { type: 'wizard', name: 'x', object: 'crm_lead', viewKind: 'form' }], + ['overlay.badOperator', { filter: [{ field: 'name', operator: 'sorta_equals', value: 'x' }] }, { type: 'simple' }], + ['overlay.consoleSortId', { sort: [{ id: 'row-1', field: 'name', order: 'asc' }] }, { type: 'simple' }], + ['overlay.consoleFilterId', { filter: [{ id: 'row-1', field: 'name', operator: '=', value: 'x' }] }, { type: 'simple' }], + ['put.isPinned', { isPinned: true }, { type: 'simple' }], + ['put.sortOrder', { sortOrder: 3 }, { type: 'simple' }], + ['put.hidden', { hidden: true }, { type: 'simple', hidden: true }], + ['put.pinAndOrder', { isPinned: true, sortOrder: 3 }, { type: 'simple' }], + ]; + + const REFUSED: Array<[string, unknown, string[]]> = [ + ['viewItem.badKind', { name: 'a.b', object: 'a', viewKind: 'chart', config: LIST_CFG }, ['invalid_union']], + ['viewItem.badConfig', { name: 'a.b', object: 'a', viewKind: 'list', config: { type: 'grid', columns: 'nope' } }, ['invalid_union']], + ['viewItem.typoConfg', { name: 'a.b', object: 'a', viewKind: 'list', confg: LIST_CFG }, ['custom']], + ['viewItem.consoleDecor', { name: 'a.b', object: 'a', viewKind: 'list', config: { ...LIST_CFG, filter: [{ id: 'x', field: 'name', operator: '=', value: 'q' }] } }, ['invalid_union']], + ['container.empty', { object: 'crm_lead', listViews: {} }, ['custom']], + ['container.withAux', { object: 'crm_lead', list: LIST_CFG, isPinned: true }, ['invalid_union']], + ['container.badInner', { object: 'crm_lead', list: { type: 'nope' } }, ['invalid_union']], + ['overlay.list.min', { type: 'grid' }, ['invalid_union']], + ['overlay.list.identity', { name: 'x', object: 'crm_lead', viewKind: 'list', type: 'kanban', groupByField: 'stage' }, ['invalid_union']], + ['overlay.badType', { type: 'sideways' }, ['invalid_union']], + ['overlay.badColumns', { type: 'grid', columns: 'not-an-array' }, ['invalid_union']], + ['overlay.emptyState.badKey', { type: 'grid', emptyState: { title: 'None', notAnEmptyStateKey: 1 } }, ['invalid_union']], + ['identity.nope', { nope: 1 }, ['custom']], + ['identity.empty', {}, ['custom']], + ['identity.idOnly', { id: 'x' }, ['custom']], + ['identity.nameOnly', { name: 'garbage_view' }, ['custom']], + ['identity.stampedOnly', { name: 'v', viewKind: 'list', object: 'a', label: 'L' }, ['custom']], + ['scalar.string', 'nope', ['invalid_union']], + ['scalar.number', 42, ['invalid_union']], + ['scalar.null', null, ['invalid_union']], + ['scalar.undefined', undefined, ['invalid_union']], + ['scalar.array', [LIST_CFG], ['invalid_union']], + ['scalar.date', new Date(0), ['custom']], + ]; + + for (const [label, body, output] of ACCEPTED) { + it(`still ACCEPTS ${label}`, () => { + const r = ViewMetadataSchema.safeParse(body); + expect(r.success).toBe(true); + // The parse OUTPUT is pinned too, not just the verdict: three of these + // bodies are accepted only because a member strips most of them away, and + // "accepted" alone would not notice that stopping. + expect(r.data).toEqual(output); + }); + } + + for (const [label, body, codes] of REFUSED) { + it(`still REFUSES ${label}`, () => { + const r = ViewMetadataSchema.safeParse(body); + expect(r.success).toBe(false); + // The refusal's SHAPE is what a better diagnostic is allowed to change; + // its issue codes are the envelope other consumers key on, so they are + // pinned too (ADR-0112 / #6142's "a better diagnostic never weakens the + // envelope", read at the issue level). + expect([...new Set(r.error!.issues.map((i) => i.code))].sort()).toEqual(codes); + }); + } + + // The JSON-Schema face the `/api/v1/meta/types/view` endpoint serves is + // pinned in `view-metadata-schema.test.ts`; re-asserted here in the one + // dimension this change could have moved — the member COUNT. + it('still emits an anyOf of exactly four members', () => { + const json = z.toJSONSchema(ViewMetadataSchema, { unrepresentable: 'any', io: 'input' }) as { + anyOf?: unknown[]; + }; + expect(json.anyOf).toHaveLength(4); + }); +}); diff --git a/packages/spec/src/ui/view.zod.ts b/packages/spec/src/ui/view.zod.ts index de1c618080..714c9fbf35 100644 --- a/packages/spec/src/ui/view.zod.ts +++ b/packages/spec/src/ui/view.zod.ts @@ -2334,7 +2334,7 @@ export function defineViewItem(config: z.input): ViewItem // an arbitrary body — Prime Directive #10's "declared ≠ enforced", one layer // above the object schemas #4001 closed: at union-MEMBER SELECTION, not at // any single member. The fix is a precondition, NOT a strictness flip on the -// members — see {@link viewIdentityVocabulary}. +// members — see {@link assertViewIdentity} and {@link viewMetadataVocabulary}. /** * Optional identity + structural-guard fields layered onto the two "flattened @@ -2450,6 +2450,21 @@ function collectDeclaredTopLevelKeys(schema: unknown, into: Set, depth = } } +/** + * [#5599] The precondition's predicate half, extracted (#6391) so + * {@link diagnoseViewMetadata} can ask the SAME question the parse path asks + * without having to recognise the issue text the other half writes. + * + * True for anything this precondition does not judge (non-objects, arrays — + * left to the union, which already rejects them) and for any object carrying at + * least one key some member declares that the write path did not stamp itself. + */ +function speaksViewVocabulary(body: unknown): boolean { + if (typeof body !== 'object' || body === null || Array.isArray(body)) return true; + const vocabulary = viewMetadataVocabulary(); + return Object.keys(body as Record).some((key) => vocabulary.has(key)); +} + /** * [#5599] The minimal identity precondition, run BEFORE the four-arm union. * @@ -2488,12 +2503,12 @@ function collectDeclaredTopLevelKeys(schema: unknown, into: Set, depth = * so a rejected body yields ONE named issue instead of that issue plus four * `invalid_union` branches for arms that were never the point. */ -function assertViewIdentity(body: unknown, ctx: z.RefinementCtx, vocabulary: Set): boolean { +function assertViewIdentity(body: unknown, ctx: z.RefinementCtx): boolean { // Non-objects and arrays are left to the union, which already rejects them — // this precondition answers "which object is not a view", nothing else. if (typeof body !== 'object' || body === null || Array.isArray(body)) return true; + if (speaksViewVocabulary(body)) return true; const keys = Object.keys(body as Record); - if (keys.some((key) => vocabulary.has(key))) return true; // Report the two halves separately. Lumping them together would call `name` // "unrecognized", which is false and would send an author to fix the wrong // key: `name` is declared, it is just discounted as evidence. @@ -2520,6 +2535,292 @@ function assertViewIdentity(body: unknown, ctx: z.RefinementCtx, vocabulary: Set return false; } +// ─────────────────────────────────────────────────────────────────────────── +// [#6391] The union's members, named and published +// ─────────────────────────────────────────────────────────────────────────── +// +// Measured from the consumer side (objectui#3606 / PR objectui#3624, against +// `@objectstack/spec` 17.0.0-rc.5): a consumer that has to turn a +// `ViewMetadataSchema` failure back into per-field diagnostics had exactly two +// routes, and both couple to spec internals. It could index the nested +// `invalid_union` `errors[]` BY MEMBER POSITION — position is an internal +// detail nobody promised, so objectui pinned it with a CANARY test that rings +// on every reorder — or it could re-declare each member itself, which is a +// deeper coupling still and was measured and rejected there. +// +// The cause was never the union: member 1 was already the exported +// `ViewItemWireSchema`, and a consumer could reference THAT contractually. The +// other three were inline expressions with no name, so member 2 was merely +// "shaped like `ViewSchema`" — key-for-key identical, behaviourally identical, +// and NOT the same object, which is precisely the difference between a contract +// and a resemblance. +// +// So the members get names, the union is built FROM those names (the identity +// is guaranteed by construction, not maintained by hand), and a branch-naming +// diagnostic entry point sits beside them so a consumer never has to know the +// order at all — {@link diagnoseViewMetadata}. +// +// The names are published as ONE export — {@link VIEW_METADATA_MEMBERS}, keyed +// by branch — rather than as three individual `…Schema` consts, and that is a +// decision rather than a style: a top-level exported schema binding mints a new +// protocol def (`ui/ViewContainerWire`, …) in `json-schema.manifest/` and, for +// anything with a derivable object shape, a full set of keys in the ratcheted +// `authorable-surface/`. The container member's 15 keys ARE `ui/View`'s 15 +// keys — duplicating them there would add 15 phantom authorable properties to +// the ADR-0049 liveness worklist for a wire door nobody authors against. The +// record gives a consumer the same contractual handle +// (`VIEW_METADATA_MEMBERS.container`) with none of that. +// +// ⛔ What this deliberately does NOT do: convert the union to +// `z.discriminatedUnion`. That was the filer's other option, and it would move +// the ACCEPTANCE face — a discriminated union refuses an unknown discriminant +// outright where this union falls through all four members, and several of +// these shapes have no discriminant key at all (a flattened overlay may carry +// nothing but `columns`). The dispatch below is therefore a DIAGNOSTIC +// dispatch: it names the branch a failure belongs to, and decides nothing about +// whether the body is accepted. `ViewMetadataSchema` remains the only judge. + +/** + * [#6391] Member 2 of {@link ViewMetadataSchema} — a non-empty `defineView` + * container. Published as `VIEW_METADATA_MEMBERS.container`. + * + * `ViewSchema` itself accepts `{}` (every slot optional), which would register + * zero views; the refinement is what makes this member "a container that + * actually defines a view" — see {@link containerHasAView}. It is published + * because that refinement, not `ViewSchema`, is what the union holds: a + * consumer selecting "the container branch" needs the object the union is + * actually going to run, or its diagnosis diverges from the platform's on + * exactly the empty-container case. + */ +const ViewContainerWireSchema = lazySchema(() => + ViewSchema.refine(containerHasAView, { + message: + 'A view container must define at least one of `list`, `form`, `listViews`, or `formViews`.', + }), +); + +/** + * [#6391] Member 3 of {@link ViewMetadataSchema} — a flattened runtime LIST + * overlay: an inline `ListView` config at the top level plus the optional + * identity/round-trip fields a personalization PUT carries. Published as + * `VIEW_METADATA_MEMBERS.listOverlay`. + * + * `.strip()` is load-bearing, not leftover. `.extend()` INHERITS strictness, so + * closing `ListViewSchema` for authoring (#4001) silently made this overlay + * strict too — and this member exists precisely to carry Studio's auxiliary + * round-trip keys (`isPinned`, `sortOrder`, …) that `saveMetaItem` persists + * verbatim. Strict here is a 422 on a shape the platform itself writes. The + * ledger names this as the trap to watch while batching: a response-side + * extension of an authoring schema must strip back, or an upstream field + * addition becomes a crash. + */ +const ListViewOverlayWireSchema = lazySchema(() => + ListViewSchema.extend(flattenedViewOverlayFields()).strip(), +); + +/** + * [#6391] Member 4 of {@link ViewMetadataSchema} — a flattened runtime FORM + * overlay, published as `VIEW_METADATA_MEMBERS.formOverlay`. Same construction + * and the same `.strip()` rationale as {@link ListViewOverlayWireSchema}; the + * list member is tried first, and a flattened form (no required `columns`, + * disjoint `type` enum) then matches here. + */ +const FormViewOverlayWireSchema = lazySchema(() => + FormViewSchema.extend(flattenedViewOverlayFields()).strip(), +); + +/** + * [#6391] The branch names of {@link ViewMetadataSchema}'s union, in + * declaration order. + * + * The order is still the union's evaluation order — that has not changed and is + * observable — but a consumer no longer has to *encode* it: it can ask for a + * branch by name via {@link VIEW_METADATA_MEMBERS}, or let + * {@link diagnoseViewMetadata} name the branch for it. + */ +export const VIEW_METADATA_BRANCHES = ['viewItem', 'container', 'listOverlay', 'formOverlay'] as const; + +/** [#6391] One branch of {@link ViewMetadataSchema}'s union, by name. */ +export type ViewMetadataBranch = (typeof VIEW_METADATA_BRANCHES)[number]; + +/** + * [#6391] Branch name → the schema {@link ViewMetadataSchema}'s union actually + * holds for that branch. + * + * This record IS the union's member list: the schema below is built by mapping + * {@link VIEW_METADATA_BRANCHES} over it, so "member N is the published schema" + * is guaranteed by construction rather than by two declarations agreeing. Add a + * member here and it is in the union, named, in one edit. + * + * The values are {@link lazySchema} proxies, so naming them costs no eager + * materialisation (ADR-0089 D3a) — reading this record does not build a single + * view schema. + */ +export const VIEW_METADATA_MEMBERS = { + // 1. Standalone ViewItem record — nested config validated genuinely, and the + // WIRE variant, so Studio's round-trip keys have a declared home. + viewItem: ViewItemWireSchema, + // 2. Non-empty defineView container. + container: ViewContainerWireSchema, + // 3/4. Flattened runtime overlay — inline ListView / FormView config + identity. + listOverlay: ListViewOverlayWireSchema, + formOverlay: FormViewOverlayWireSchema, +} as const satisfies Record; + +/** + * [#5599] The `view` vocabulary, derived once from the members on first use. + * + * Hoisted out of {@link ViewMetadataSchema}'s factory closure (#6391) so + * {@link diagnoseViewMetadata} can consult the SAME derivation the parse path + * consults. Two copies of this rule would be two answers to "is this a view + * body at all?", and the diagnostic one would be the wrong one. + * + * Still computed on first call rather than at module load: the members are + * {@link lazySchema} proxies whose factories run on first `_zod` touch, and + * deriving eagerly would force every view schema in the file to materialise as + * a side effect of this module loading (ADR-0089 D3a). + */ +let viewVocabularyCache: Set | undefined; +function viewMetadataVocabulary(): Set { + if (!viewVocabularyCache) { + const vocabulary = new Set(); + for (const branch of VIEW_METADATA_BRANCHES) { + collectDeclaredTopLevelKeys(VIEW_METADATA_MEMBERS[branch], vocabulary); + } + // Identity the write path supplies is not evidence of shape — see + // `VIEW_WRITE_PATH_IDENTITY_KEYS` for why this subtraction is the + // difference between closing #5599 and only appearing to. + for (const key of VIEW_WRITE_PATH_IDENTITY_KEYS) vocabulary.delete(key); + viewVocabularyCache = vocabulary; + } + return viewVocabularyCache; +} + +/** + * [#6391] The `type` values that tell the two flattened overlay branches apart. + * + * `ListViewSchema.type` and `FormViewSchema.type` are disjoint enums — the + * property the union's own comment already relies on ("a flattened form (no + * required `columns`, disjoint `type` enum) then matches the form member"). Read + * off the schemas rather than re-listed, so a new view type cannot make the + * dispatch disagree with the members. + */ +function overlayTypeValues(schema: z.ZodTypeAny): ReadonlySet { + const shape = (schema as unknown as { _zod?: { def?: { shape?: Record } } }) + ._zod?.def?.shape; + const values = (shape?.type as { _zod?: { values?: Set } } | undefined)?._zod?.values; + return new Set([...(values ?? [])].filter((v): v is string => typeof v === 'string')); +} + +/** + * [#6391] Which branch of {@link ViewMetadataSchema} a body CLAIMS to be, by + * its structural discriminants — or `null` when nothing in the body settles it. + * + * This is the "select the member by body" entry the filer asked for, and it + * encodes exactly the discriminants the members themselves already declare: + * + * - a nested `config` means a ViewItem record — both overlay members pin + * `config: z.undefined()` precisely to exclude that shape; + * - a container slot (`list` / `form` / `listViews` / `formViews`) means a + * container — both overlay members pin those `undefined` too; + * - otherwise the body is a flattened overlay, and `viewKind` (when the write + * path inherited one — #2555) or the disjoint `type` enums say which family. + * + * ⚠️ A `null` answer is not a rejection and a non-`null` one is not an + * acceptance. This function decides which branch is worth EXPLAINING; whether + * the body parses is `ViewMetadataSchema`'s answer and only its answer. + */ +export function selectViewMetadataBranch(body: unknown): ViewMetadataBranch | null { + if (typeof body !== 'object' || body === null || Array.isArray(body)) return null; + const b = body as Record; + if (b.config !== undefined) return 'viewItem'; + if (b.list !== undefined || b.form !== undefined || b.listViews !== undefined || b.formViews !== undefined) { + return 'container'; + } + if (b.viewKind === 'form') return 'formOverlay'; + if (b.viewKind === 'list') return 'listOverlay'; + if (typeof b.type === 'string') { + if (overlayTypeValues(FormViewSchema).has(b.type)) return 'formOverlay'; + if (overlayTypeValues(ListViewSchema).has(b.type)) return 'listOverlay'; + } + return null; +} + +/** [#6391] The result of {@link diagnoseViewMetadata}. */ +export type ViewMetadataDiagnosis = + | { success: true; branch: ViewMetadataBranch; data: ViewMetadataParsed } + | { success: false; branch: ViewMetadataBranch | null; issues: readonly z.core.$ZodIssue[] }; + +/** + * [#6391] Explain a `view` body the way a consumer needs it explained: with the + * failing BRANCH named and that branch's own per-field issues, flat. + * + * The problem this closes, measured in objectui#3624: `ViewMetadataSchema` + * failing produces ONE root `invalid_union` issue with the per-field truth + * buried in nested `errors[]`, addressable only by member position. Here the + * position never appears — the caller gets `branch: 'listOverlay'` and the list + * overlay's own issues, with real field paths. + * + * Three outcomes, and the middle one is the one worth reading twice: + * + * 1. **The body parses.** `ViewMetadataSchema` accepted it; the branch reported + * is the first member that accepts it. + * 2. **The body is not a `view` at all** (#5599's identity precondition — + * `{ nope: 1 }`, `{}`, identity keys only). `branch` is `null` and the + * issues are the precondition's own. Naming a branch here would be a lie: + * the union never ran, and pointing the author at "the form overlay" would + * send them to fix a shape they were not writing. + * 3. **The body claims a branch and that branch rejects it.** `branch` names + * it, `issues` are that member's, resolved against the body's own paths. + * When no discriminant settles the claim, every member is tried and the one + * complaining LEAST is reported — the same "fewest issues wins" ranking + * `selectUnionBranches` (`shared/error-map.zod.ts`) already uses for this + * family, so one mistake gets one prescription across every surface. + * + * ⛔ **Acceptance is not decided here.** This function delegates the verdict to + * `ViewMetadataSchema` and only ever explains a verdict it did not make: if the + * schema accepts, this reports success no matter what the branch dispatch + * thinks. Pinned in `view-union-diagnostics.test.ts`. + */ +export function diagnoseViewMetadata(body: unknown): ViewMetadataDiagnosis { + const parsed = ViewMetadataSchema.safeParse(body); + const stripped = stripViewConsoleDecorations(body); + + if (parsed.success) { + const accepting = VIEW_METADATA_BRANCHES.find( + (branch) => VIEW_METADATA_MEMBERS[branch].safeParse(stripped).success, + ); + // The union accepted, so some member did; `selectViewMetadataBranch` is the + // fallback only for the impossible case, never the primary answer. + return { success: true, branch: accepting ?? selectViewMetadataBranch(stripped) ?? 'viewItem', data: parsed.data }; + } + + // (2) The identity precondition short-circuited the pipe with `z.NEVER`, so + // the union never ran and there is no branch to name. Detected by re-running + // the precondition's own predicate rather than by pattern-matching its issue + // text — same rule, one source. + if (!speaksViewVocabulary(body)) { + return { success: false, branch: null, issues: parsed.error.issues }; + } + + const claimed = selectViewMetadataBranch(stripped); + const candidates = claimed ? [claimed] : [...VIEW_METADATA_BRANCHES]; + let best: { branch: ViewMetadataBranch; issues: readonly z.core.$ZodIssue[] } | undefined; + for (const branch of candidates) { + const result = VIEW_METADATA_MEMBERS[branch].safeParse(stripped); + // A member that accepts what the union rejected cannot happen (the union is + // exactly these members), but if it ever did, reporting zero issues under a + // `success: false` would be the worst possible answer — so skip it. + if (result.success) continue; + if (!best || result.error.issues.length < best.issues.length) { + best = { branch, issues: result.error.issues }; + } + } + return best + ? { success: false, branch: best.branch, issues: best.issues } + : { success: false, branch: claimed, issues: parsed.error.issues }; +} + /** * Canonical schema for ANY persisted `view` metadata body — the schema the * `view` type registers in `metadata-type-schemas.ts`. A union over the three @@ -2545,48 +2846,15 @@ function assertViewIdentity(body: unknown, ctx: z.RefinementCtx, vocabulary: Set * on member 4. */ export const ViewMetadataSchema = lazySchema(() => { - const members = [ - // 1. Standalone ViewItem record — nested config validated genuinely, and - // the WIRE variant, so Studio's round-trip keys have a declared home. - ViewItemWireSchema, - // 2. Non-empty defineView container. - ViewSchema.refine(containerHasAView, { - message: - 'A view container must define at least one of `list`, `form`, `listViews`, or `formViews`.', - }), - // 3. Flattened runtime overlay — inline ListView / FormView config + identity. - // The list member is tried first; a flattened form (no required - // `columns`, disjoint `type` enum) then matches the form member. - // - // `.strip()` is load-bearing, not leftover. `.extend()` INHERITS - // strictness, so closing `ListViewSchema`/`FormViewSchema` for authoring - // (#4001) silently made this overlay strict too — and this member exists - // precisely to carry Studio's auxiliary round-trip keys (`isPinned`, - // `sortOrder`, …) that `saveMetaItem` persists verbatim. Strict here is a - // 422 on a shape the platform itself writes. The ledger names this as the - // trap to watch while batching: a response-side extension of an authoring - // schema must strip back, or an upstream field addition becomes a crash. - ListViewSchema.extend(flattenedViewOverlayFields()).strip(), - FormViewSchema.extend(flattenedViewOverlayFields()).strip(), - ] as const; - - // [#5599] Derived once, on first parse — see `collectDeclaredTopLevelKeys`. - let vocabulary: Set | undefined; - const viewVocabulary = (): Set => { - if (!vocabulary) { - vocabulary = new Set(); - for (const member of members) collectDeclaredTopLevelKeys(member, vocabulary); - // Identity the write path supplies is not evidence of shape — see - // `VIEW_WRITE_PATH_IDENTITY_KEYS` for why this subtraction is the - // difference between closing #5599 and only appearing to. - for (const key of VIEW_WRITE_PATH_IDENTITY_KEYS) vocabulary.delete(key); - } - return vocabulary; - }; + // [#6391] The members ARE {@link VIEW_METADATA_MEMBERS}, read in + // {@link VIEW_METADATA_BRANCHES} order. Built by mapping rather than + // re-listed, so "the published schema is the member the union runs" cannot + // drift into "the published schema resembles it" — the exact gap objectui had + // to bridge with a canary test. + const members = VIEW_METADATA_BRANCHES.map((branch) => VIEW_METADATA_MEMBERS[branch]); return z.preprocess( - (body, ctx) => - assertViewIdentity(body, ctx, viewVocabulary()) ? stripViewConsoleDecorations(body) : z.NEVER, + (body, ctx) => (assertViewIdentity(body, ctx) ? stripViewConsoleDecorations(body) : z.NEVER), z.union(members as unknown as readonly [z.ZodTypeAny, z.ZodTypeAny, ...z.ZodTypeAny[]]), ); });