diff --git a/.changeset/savemeta-422-union-branch-issues.md b/.changeset/savemeta-422-union-branch-issues.md new file mode 100644 index 0000000000..4d67ff0503 --- /dev/null +++ b/.changeset/savemeta-422-union-branch-issues.md @@ -0,0 +1,41 @@ +--- +"@objectstack/metadata-protocol": patch +--- + +fix(metadata-protocol): 元数据保存的 422 保留 union 分支处方,Studio 重新拿得到字段名 (#5364) + +`saveMetaItem` 的 spec-conformance 检查在自己的注释里承诺 "structured Zod issues +so the Studio form can highlight the offending field"。顶层 `z.union` 让这句承诺 +彻底落空:zod 把一个失败 union 的**所有**分支折叠成**一条**顶层 issue,`path` 是 +空串、`message` 是字面量 `"Invalid input"`,而旧代码的 `parsed.error.issues.map(…)` +映射的正是这一条。 + +代价不是"文案不够好",而是**字段定位本身消失了**。`ViewMetadataSchema` 顶层就是一个 +union(`view.zod.ts` 的 `z.preprocess(…, z.union([…]))`),所以**每一次** view 保存 +失败都退化成: + +```json +[{ "path": "", "message": "Invalid input", "code": "invalid_union" }] +``` + +一个字段名都没有到达作者,Studio 表单没有任何东西可以高亮;422 的摘要行也只是 +`... failed spec validation: : Invalid input`。被丢掉的分支里躺着的恰恰是 +#4001 那批策展处方(点名真实键名的 unrecognized_keys)和带绝对路径、带合法枚举的 +逐槽位判决。 + +现在这些分支被展开进 `issues[]`:union 自己那条**保留不动**(展开是严格叠加的, +今天读 `issues[0]` 的消费者不会少读到任何东西),后面跟上真正解释这次拒绝的分支, +路径按绝对路径拼好——分支 issue 的 `path` 是**相对于 union** 的,这是 #5014 付过 +学费的坑。422 的 `message` 摘要行随之变得可读。 + +分支选择策略与已落地的两处**逐条一致**:丢弃只报根部 kind 不匹配的分支;报得最少 +的分支胜出;`unrecognized_keys` 破平局;声明顺序决定其余;并列的全部输出(上限 3); +嵌套 union 递归展开(上限 3 层)。这是同一机制的**第三份**拷贝——`packages/spec` +的 `formatZodError`(#4971)只导出字符串渲染器,`packages/rest` 的 +`zodIssuesToFields`(#5014)产出 ADR-0114 的 `{field, code}` 目录条目,而本处的信封 +是 `{path, message, code}` 且 `code` 透传 zod 原码——形态不同,**判决必须相同**, +否则同一个错误会因为作者是从终端发布、还是 POST 数据 API、还是在 Studio 里保存, +拿到三套说法。 + +行为边界:合法的元数据照常保存,非法的元数据照常被 422 拒绝且不落库;变的只是 +`issues[]` 从"一条无字段的 `Invalid input`"变成"那一条 + 真正解释它的分支"。 diff --git a/packages/metadata-protocol/src/protocol.save-union-issues.test.ts b/packages/metadata-protocol/src/protocol.save-union-issues.test.ts new file mode 100644 index 0000000000..1cd2e4f70d --- /dev/null +++ b/packages/metadata-protocol/src/protocol.save-union-issues.test.ts @@ -0,0 +1,248 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #5364 — `saveMetaItem`'s `422 INVALID_METADATA` keeps the union branch that + * explains the rejection. + * + * The 422's own comment promises "structured Zod issues so the Studio form can + * highlight the offending field". A top-level `z.union` broke that promise + * completely: zod folds every branch of a failed union into ONE issue whose + * path is `''` and whose message is the literal `"Invalid input"`, and the old + * `parsed.error.issues.map(…)` mapped exactly that. Since `ViewMetadataSchema` + * IS a top-level union (`view.zod.ts` — `z.preprocess(…, z.union([…]))`), EVERY + * failed `view` save arrived at Studio as one rootless line with no field name + * in it at all. + * + * This is the fourth consumer of one mechanism, and the verdict must match the + * other three by construction — `formatZodError` (#4971, spec), + * `zodIssuesToFields` (#5014, rest), `formatZodErrors` (#5341, cli). The tests + * below therefore pin the SHARED ranking's behaviour, not a locally-nicer one. + * + * Harness: the real repository write path over a stub engine, same shape as + * `protocol.save-flow-canonicalization.test.ts` — a fix INSIDE `saveMetaItem` + * cannot use a harness that mocks `saveMetaItem`. + */ +import { describe, expect, it } from 'vitest'; +import { getMetadataTypeSchema } from '@objectstack/spec/kernel'; +import { ObjectStackProtocolImplementation, zodIssuesToMetadataIssues } from './protocol.js'; + +interface Row { + id: string; + type: string; + name: string; + organization_id: string | null; + state: string; + metadata: string; +} + +const keyOf = (w: Record) => + `${w.type}|${w.name}|${w.organization_id ?? '__env__'}|${w.state ?? 'active'}`; + +/** The engine surface the repository write path touches. */ +function makeProtocol() { + const rows = new Map(); + let nextId = 0; + const engine: any = { + async findOne() { return null; }, + async find() { return []; }, + async insert(table: string, data: Record) { + if (table === 'sys_metadata_audit') return { id: 'audit_skip' }; + nextId += 1; + const row = { id: `r_${nextId}`, ...(data as any) } as Row; + rows.set(keyOf(data), row); + return { id: row.id }; + }, + async update() { return { id: null }; }, + async delete() { return { deleted: 0 }; }, + registry: { registerItem: () => {}, registerObject: () => {} }, + }; + const protocol: any = new ObjectStackProtocolImplementation(engine, () => new Map()); + return { protocol, rows }; +} + +const save = (protocol: any, item: unknown, name = 'task_list', type = 'view') => + protocol.saveMetaItem({ type, name, item }); + +async function rejection(promise: Promise): Promise { + try { + await promise; + } catch (err) { + return err; + } + throw new Error('expected the save to be rejected, but it resolved'); +} + +/** The issue's verbatim repro body: a list view whose `summary` has a typo'd key. */ +const issueReproView = () => ({ + name: 'task_list', + object: 'task', + type: 'list', + label: 'Tasks', + columns: [{ field: 'title', summary: { type: 'sum', fieldd: 'amount' } }], +}); + +describe('#5364 saveMetaItem 422 expands union branches', () => { + it('zod really does fold the whole rejection into one rootless issue (the defect, pinned)', () => { + // The "reverse verification" for this change, stated as a fact about + // zod rather than as a code revert: this is EXACTLY what the old + // `parsed.error.issues.map(…)` had to work with. Restore that map and + // every assertion in the next two tests goes red, because the branch + // payload below is the only place a field name exists. + const schema = getMetadataTypeSchema('view')!; + const parsed = (schema as any).safeParse(issueReproView()); + + expect(parsed.success).toBe(false); + expect(parsed.error.issues).toHaveLength(1); + expect(parsed.error.issues[0]).toMatchObject({ code: 'invalid_union', message: 'Invalid input' }); + expect(parsed.error.issues[0].path).toEqual([]); + // …while four branches, each with a real reason, hang off `errors`. + expect(parsed.error.issues[0].errors.length).toBeGreaterThan(1); + }); + + it('the issue\'s view body: real key names now reach the author instead of "Invalid input"', async () => { + const { protocol, rows } = makeProtocol(); + + const err = await rejection(save(protocol, issueReproView())); + + expect(err.code).toBe('INVALID_METADATA'); + expect(err.status).toBe(422); + // Load-bearing: an invalid body is still refused, and still persists nothing. + expect(rows.size).toBe(0); + + // The union's own entry is KEPT — the expansion is strictly additive, so + // no consumer reading `issues[0]` today loses what it reads. + expect(err.issues[0]).toEqual({ path: '', message: 'Invalid input', code: 'invalid_union' }); + expect(err.issues.length).toBeGreaterThan(1); + + // …and the branch that explains the rejection now rides along, carrying + // the #4001 curated prose WITH the offending key names in it. + const unknownKey = err.issues.find((i: any) => i.code === 'unrecognized_keys'); + expect(unknownKey).toBeDefined(); + expect(unknownKey.message).toContain('`type`'); + expect(unknownKey.message).toContain('`columns`'); + + // The 422's summary line is readable now — before this it was the + // whole-message `…failed spec validation: : Invalid input`. + expect(err.message).toContain('Unrecognized key(s)'); + }); + + it('a container body localises the failure to a real path Studio can highlight', async () => { + // Ranking note (identical in all copies): the branch with the FEWEST + // issues wins, and `unrecognized_keys` breaks a tie. For a container + // body no branch reports an unknown key, so what survives is the + // per-slot verdict — an absolute path plus the legal enum. + const { protocol, rows } = makeProtocol(); + + const err = await rejection(save(protocol, { list: { type: 'nope', columns: [{ field: 'a' }] } })); + + expect(err.status).toBe(422); + expect(rows.size).toBe(0); + + const badType = err.issues.find((i: any) => i.path === 'list.type'); + expect(badType).toBeDefined(); + expect(badType.code).toBe('invalid_value'); + expect(badType.message).toContain('"grid"'); + // Absolute, not branch-relative: the branch raised this at `['list','type']` + // relative to the union sitting at the document root (#5014's trap). + expect(badType.path).toBe('list.type'); + }); + + it('a spec-valid view still saves — the expansion never invents a rejection', async () => { + const { protocol, rows } = makeProtocol(); + + const result = await save(protocol, { + name: 'task_list', object: 'task', type: 'grid', label: 'Tasks', + columns: [{ field: 'title' }], + }); + + expect(result.success).toBe(true); + expect(rows.size).toBe(1); + }); +}); + +describe('#5364 zodIssuesToMetadataIssues — the shared ranking, verbatim', () => { + const union = (errors: unknown[][], path: unknown[] = []) => + ({ code: 'invalid_union', message: 'Invalid input', path, errors }); + + it('a non-union issue passes through byte-identical', () => { + const issues = [{ code: 'invalid_type', message: 'Required', path: ['label'] }]; + expect(zodIssuesToMetadataIssues(issues)).toEqual([ + { path: 'label', message: 'Required', code: 'invalid_type' }, + ]); + }); + + it('every branch a bare kind mismatch → output unchanged (no noise added)', () => { + // `z.union([z.string(), z.number()])` handed an object. Neither branch + // has a prescription; emitting both would be N× the noise for nothing. + const issues = [union([ + [{ code: 'invalid_type', message: 'expected string', path: [] }], + [{ code: 'invalid_type', message: 'expected number', path: [] }], + ], ['mode'])]; + expect(zodIssuesToMetadataIssues(issues)).toEqual([ + { path: 'mode', message: 'Invalid input', code: 'invalid_union' }, + ]); + }); + + it('zod\'s "matched multiple" variant (errors: []) adds nothing', () => { + expect(zodIssuesToMetadataIssues([union([])])).toEqual([ + { path: '', message: 'Invalid input', code: 'invalid_union' }, + ]); + }); + + it('fewest issues wins; unrecognized_keys breaks the tie', () => { + const out = zodIssuesToMetadataIssues([union([ + [{ code: 'invalid_value', message: 'wrong discriminator', path: ['kind'] }], + [{ code: 'unrecognized_keys', message: 'Unrecognized key(s): `nmae`', path: [] }], + [ + { code: 'invalid_value', message: 'wrong discriminator', path: ['kind'] }, + { code: 'invalid_type', message: 'Required', path: ['title'] }, + ], + ])]); + expect(out).toEqual([ + { path: '', message: 'Invalid input', code: 'invalid_union' }, + { path: '', message: 'Unrecognized key(s): `nmae`', code: 'unrecognized_keys' }, + ]); + }); + + it('branches that tie at the top are all emitted, capped at three', () => { + const branch = (n: number) => [{ code: 'invalid_type', message: `bad ${n}`, path: [`f${n}`] }]; + const out = zodIssuesToMetadataIssues([union([branch(1), branch(2), branch(3), branch(4)])]); + expect(out.map((i) => i.path)).toEqual(['', 'f1', 'f2', 'f3']); + }); + + it('branch paths are resolved against the union\'s own, at every level', () => { + const inner = union([[{ code: 'invalid_type', message: 'Required', path: ['id'] }]], ['nodes', 0]); + const out = zodIssuesToMetadataIssues([union([[inner]], ['flow'])]); + expect(out.map((i) => i.path)).toEqual(['flow', 'flow.nodes.0', 'flow.nodes.0.id']); + }); + + it('nesting is bounded at three levels — the fourth union is not expanded', () => { + const leaf = { code: 'invalid_type', message: 'Required', path: ['leaf'] }; + const level4 = union([[leaf]], ['d']); + const level3 = union([[level4]], ['c']); + const level2 = union([[level3]], ['b']); + const level1 = union([[level2]], ['a']); + const out = zodIssuesToMetadataIssues([level1]); + // a → a.b → a.b.c → a.b.c.d, and there it stops: `leaf` never appears. + expect(out.map((i) => i.path)).toEqual(['a', 'a.b', 'a.b.c', 'a.b.c.d']); + expect(out.some((i) => i.path.endsWith('leaf'))).toBe(false); + }); + + it('two branches rejecting the same key with the same words say it once', () => { + const same = () => [{ code: 'unrecognized_keys', message: 'Unrecognized key(s): `nmae`', path: [] }]; + const out = zodIssuesToMetadataIssues([union([same(), same()])]); + expect(out).toHaveLength(2); + expect(out[1]!.code).toBe('unrecognized_keys'); + }); + + it('de-duplication is per top-level issue, never across two independent ones', () => { + const issue = { code: 'invalid_type', message: 'Required', path: ['label'] }; + const out = zodIssuesToMetadataIssues([issue, issue]); + expect(out).toHaveLength(2); + }); + + it('a non-array `issues` yields an empty envelope rather than throwing', () => { + expect(zodIssuesToMetadataIssues(undefined)).toEqual([]); + expect(zodIssuesToMetadataIssues(null)).toEqual([]); + }); +}); diff --git a/packages/metadata-protocol/src/protocol.ts b/packages/metadata-protocol/src/protocol.ts index e77793a61f..8961c23074 100644 --- a/packages/metadata-protocol/src/protocol.ts +++ b/packages/metadata-protocol/src/protocol.ts @@ -325,6 +325,225 @@ function resolveOverlaySchema(type: string, _item: unknown): z.ZodTypeAny | null return getMetadataTypeSchema(singular) ?? null; } +/** + * One entry of the `422 INVALID_METADATA` envelope's `issues[]` — the shape + * Studio's designer keys on to highlight the offending form control. + * + * `code` is zod's OWN issue code, passed through verbatim. That is deliberately + * NOT the ADR-0114 `fields[]` catalog the data surface speaks: this envelope is + * a metadata-authoring diagnostic, its consumers already read raw zod codes, and + * aligning the two vocabularies is a separate decision (#5364). + */ +interface MetadataIssueEntry { + path: string; + message: string; + code: string | undefined; +} + +/** + * 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. + * + * Both bounds — and the whole selection policy below — are the ones + * `formatZodError` landed for the CLI/spec side of this defect (#4971, + * `spec/src/shared/error-map.zod.ts`) and `zodIssuesToFields` landed for the + * REST wire (#5014, `rest/src/rest-server.ts`). This is the THIRD copy: spec + * exports only the STRING renderer and rest's version emits ADR-0114 + * `{field, code}` catalog entries, while this envelope is + * `{path, message, code}` with zod's raw code. The *verdict* must match all the + * same, or one mistake gets three different prescriptions depending on whether + * the author published from the terminal, POSTed to the data API, or saved from + * Studio (#5364). + */ +const UNION_EXPANSION_DEPTH_LIMIT = 3; +const UNION_BRANCH_EMIT_LIMIT = 3; + +/** + * A zod issue, as much of it as the expansion reads. + * + * `errors` exists only on `invalid_union`: one issue list **per union branch**, + * with each branch's paths RELATIVE to the union issue's own path. Zod raises a + * single `invalid_union` issue whose own `message` is the literal + * `"Invalid input"`, so everything a failing branch has to say lives down there. + */ +interface ZodIssueLike { + path?: unknown; + message?: unknown; + code?: unknown; + errors?: unknown; +} + +/** A zod issue path, normalised to the array zod always produces. */ +function issuePathOf(issue: ZodIssueLike): Array { + return Array.isArray(issue?.path) ? (issue.path as Array) : []; +} + +/** + * True when a branch only complains that the value is the wrong *kind* at the + * branch root — `expected string, received object` for the string member of + * `z.union([z.string(), SomeObject])`. + * + * Such a branch carries no prescription: the author never intended it, and + * emitting it is the "N branches, N times the noise" failure. An empty branch + * (zod's "matched multiple" variant carries `errors: []`) counts as + * uninformative too — `every` on an empty list is `true`. + */ +function isKindMismatchOnly(issues: readonly ZodIssueLike[]): boolean { + return issues.every( + (issue) => + issuePathOf(issue).length === 0 + && (issue?.code === 'invalid_type' || issue?.code === 'invalid_value'), + ); +} + +/** True when a branch carries the #4001 campaign's unknown-key prescription. */ +function carriesUnknownKey(issues: readonly ZodIssueLike[]): boolean { + return issues.some((issue) => issue?.code === 'unrecognized_keys'); +} + +/** + * Pick the branch(es) of a failed union whose issues actually explain the + * failure. Ranking, in order (identical to `selectUnionBranches` in + * `spec/src/shared/error-map.zod.ts` and `rest/src/rest-server.ts`): + * + * 1. **Kind-mismatch-only branches are dropped entirely.** If *every* branch is + * one — a plain `z.union([z.string(), z.number()])` handed an object — + * nothing is selected and the union reports exactly what it always has. + * 2. **Fewest issues wins.** The branch the author was closest to hitting + * complains least, so "fewest" is what keeps ONE unknown key from arriving as + * N `issues[]` entries, one per branch. + * 3. **A branch carrying `unrecognized_keys` breaks a tie**, because that is + * where the curated prose lives. + * 4. Declaration order breaks what remains, so the envelope is deterministic. + * + * Branches that tie at the top are all emitted (capped): when two shapes explain + * the failure equally well, privileging the first by accident of declaration + * order would be a lie about which shape was expected. + */ +function selectUnionBranches( + branches: readonly (readonly ZodIssueLike[])[], +): readonly (readonly ZodIssueLike[])[] { + const informative = branches + .map((issues, index) => ({ issues, index })) + .filter((branch) => !isKindMismatchOnly(branch.issues)); + if (informative.length === 0) return []; + + const rank = (branch: { issues: readonly ZodIssueLike[] }): [number, number] => [ + branch.issues.length, + carriesUnknownKey(branch.issues) ? 0 : 1, + ]; + + const sorted = [...informative].sort((a, b) => { + const [aCount, aKeys] = rank(a); + const [bCount, bKeys] = rank(b); + return aCount - bCount || aKeys - bKeys || a.index - b.index; + }); + + const [bestCount, bestKeys] = rank(sorted[0]!); + return sorted + .filter((branch) => { + const [count, keys] = rank(branch); + return count === bestCount && keys === bestKeys; + }) + .slice(0, UNION_BRANCH_EMIT_LIMIT) + .map((branch) => branch.issues); +} + +/** + * One issue → its `issues[]` entries, appended to `out`. + * + * An ordinary issue is one entry. An `invalid_union` is its own entry (zod's + * bare `"Invalid input"`) FOLLOWED by the entries of the branches that explain + * it, with `path` resolved against the union's own — branch paths are RELATIVE + * to it, which is the trap #5014 paid for: a branch issue's `path` names a slot + * inside the union member, not inside the document. + * + * The union's entry is kept rather than replaced: it is the only entry naming + * the slot the client sent, existing consumers already read it, and when every + * branch is uninformative it is still the whole answer. So the expansion is + * strictly ADDITIVE — no entry that shipped before this changed is gone or + * renumbered, only newly accompanied. + * + * `seen` de-duplicates entries *within one top-level issue*: two branches that + * reject the same key with the same words say it once. Union entries themselves + * are exempt, since two same-path `"Invalid input"` entries can head genuinely + * different sub-trees. + * + * Deliberate divergence from the spec-side renderer: where it prints a trailing + * "… and N more branches rejected this value", this emits nothing. That line is + * a rendering affordance for a terminal; an `issues[]` entry is a machine-read + * record that must name a slot and carry a code, and the omission note has + * neither. + */ +function collectMetadataIssues( + issue: ZodIssueLike, + parentPath: Array, + depth: number, + seen: Set, + out: MetadataIssueEntry[], +): void { + const path = [...parentPath, ...issuePathOf(issue)]; + const branches: readonly (readonly ZodIssueLike[])[] = + issue?.code === 'invalid_union' && Array.isArray(issue?.errors) + ? (issue.errors as unknown[]).filter( + (branch): branch is ZodIssueLike[] => Array.isArray(branch), + ) + : []; + const expandable = branches.length > 0 && depth < UNION_EXPANSION_DEPTH_LIMIT; + + const entry: MetadataIssueEntry = { + path: path.join('.'), + message: String(issue?.message ?? 'Invalid value'), + code: issue?.code === undefined ? undefined : String(issue.code), + }; + + if (!expandable) { + const key = JSON.stringify([entry.path, entry.code, entry.message]); + if (seen.has(key)) return; + seen.add(key); + } + out.push(entry); + if (!expandable) return; + + for (const branch of selectUnionBranches(branches)) { + for (const nested of branch) { + collectMetadataIssues(nested, path, depth + 1, seen, out); + } + } +} + +/** + * Zod issues → the `422 INVALID_METADATA` envelope's `issues[]`. + * + * A rejection behind a `z.union` is expanded (#5364): zod folds every branch of + * a failed union into ONE top-level issue whose message is the literal + * `"Invalid input"`, so mapping only top-level issues put + * `[{path: '', message: 'Invalid input', code: 'invalid_union'}]` on the wire — + * not one field name — while the branch that says WHICH key is wrong (the #4001 + * curated unknown-key prose, the legal enum of a mistyped discriminator) was + * produced and dropped at the `.map()`. + * + * That mattered most HERE of the four consumers of this defect: `ViewMetadataSchema` + * is itself a top-level union, so EVERY failed `view` save degraded to that one + * rootless line and Studio's form had nothing to highlight. The other three — + * `formatZodError` (#4971), `zodIssuesToFields` (#5014), the CLI's + * `formatZodErrors` (#5341) — lost prescriptions; this one lost field + * localisation itself. + * + * Branch selection is described on {@link selectUnionBranches} and is identical + * to the other copies by construction. + */ +export function zodIssuesToMetadataIssues(issues: unknown): MetadataIssueEntry[] { + if (!Array.isArray(issues)) return []; + const out: MetadataIssueEntry[] = []; + for (const issue of issues) { + // A fresh `seen` per top-level issue: de-duplication is about one + // union's branches agreeing, never about two independent issues. + collectMetadataIssues(issue as ZodIssueLike, [], 0, new Set(), out); + } + return out; +} + /** * [#4435] The 404 a single-record operation answers when the id names no row. * @@ -7132,16 +7351,21 @@ export class ObjectStackProtocolImplementation implements // — `parsed.data` would strip Studio-only auxiliary fields (e.g. // isPinned, isDefault, sortOrder) that intentionally ride along with // the overlay document. ADR-0005 §"Validation". + // + // [#5364] "so the Studio form can highlight the offending field" was + // the promise; a top-level `z.union` broke it. Mapping only the issues + // zod raises at the TOP level sent `[{path: '', message: 'Invalid + // input', code: 'invalid_union'}]` and nothing else — and since + // `ViewMetadataSchema` IS a top-level union, that was every failed view + // save. {@link zodIssuesToMetadataIssues} expands the branches that + // explain the rejection, which is where the #4001 curated prose and the + // real key names live. { const schema = resolveOverlaySchema(request.type, request.item); if (schema) { const parsed = schema.safeParse(request.item); if (!parsed.success) { - const issues = parsed.error.issues.map((i: z.ZodIssue) => ({ - path: i.path.join('.'), - message: i.message, - code: i.code, - })); + const issues = zodIssuesToMetadataIssues(parsed.error.issues); const summary = issues.slice(0, 3) .map((i: { path: string; message: string }) => `${i.path || ''}: ${i.message}`) .join('; ');