diff --git a/.changeset/6240-object-payload-fields-map.md b/.changeset/6240-object-payload-fields-map.md new file mode 100644 index 000000000..8262d3150 --- /dev/null +++ b/.changeset/6240-object-payload-fields-map.md @@ -0,0 +1,33 @@ +--- +'@object-ui/app-shell': patch +--- + +`MetadataService`'s object writers PUT `fields` as the name-keyed MAP `ObjectSchema` +requires, not an array (objectui#6240). Both of the designer's write paths were affected, +and `saveFields` ran the conversion in the wrong direction outright: the server's own +document arrives with `fields` as a map, and `fields.map(toFieldPayload)` turned it into an +array on every field save. + +Measured against the installed `@objectstack/spec` 17.2.0 and against the framework's own +write door. `ObjectSchema.fields` is a required record: an array — empty or not — is +refused `invalid_type @ fields`, a map parses. `metadata-protocol`'s `saveMetaItem` +resolves metadata type `object` to that same `ObjectSchema`, `safeParse`s the whole item +and throws `422 INVALID_METADATA` **before** persisting, so the array was refused rather +than stripped or stored: every designer object save and every designer field save that went +through this service was a 422 that wrote nothing. + +This is the value-level half of the objectui#5761 parity family and is invisible to that +family's key-name gate — `fields` sits in the accept set under either shape, which is the +gate's own coverage note 4. The pins are runtime assertions on the captured request bytes. + +The conversion refuses, loudly, what it cannot key: a field with a missing or blank `name` +throws instead of writing a `{ undefined: … }` entry (measured: the spec ACCEPTS that +document, so nothing downstream would have caught it), and a duplicate name throws instead +of letting the later field silently replace the earlier — a loss an array does not have. +`saveFields` keeps preserving unknown keys of the fetched server document, which now +actually reaches storage. `saveObject` with no `existingFields` still omits the key rather +than writing `{}`: a PUT is an upsert, so `{}` would delete every field of an object on a +save that only meant to rename it. + +`saveObject(obj, existingFields)` keeps its `FieldMetadataPayload[]` parameter type — the +array is converted inside — so no caller's call site changes. diff --git a/packages/app-shell/src/services/MetadataService.objectPayloadFieldsMap.test.ts b/packages/app-shell/src/services/MetadataService.objectPayloadFieldsMap.test.ts new file mode 100644 index 000000000..cbf8a343e --- /dev/null +++ b/packages/app-shell/src/services/MetadataService.objectPayloadFieldsMap.test.ts @@ -0,0 +1,376 @@ +/** + * ObjectUI + * Copyright (c) 2024-present ObjectStack Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +/** + * objectui#6240 — the object payload `MetadataService` PUTs carries `fields` as + * the name-keyed MAP `ObjectSchema` requires, from BOTH of its object writers. + * + * This is the VALUE-level half of the objectui#5761 parity family, and it is + * the half that family's gate cannot see. `scripts/check-designer-field-key-parity.mjs` + * compares KEY NAMES against the installed spec's accept sets; `fields` is in + * `ObjectSchema`'s accept set under either shape, so the gate was green for the + * whole time an array sat on the wire — its own coverage note 4 says so. A + * declaration-reading gate and a runtime assertion on the request BYTES cover + * different halves; this file is the second half, and every assertion below + * reads `JSON.parse` of a captured request body rather than an in-memory + * object. + * + * ## What the route does with the array — measured, not argued + * + * The card filed this as an unmeasured premise ("whether the route is lenient + * about this today is unmeasured"). It is not lenient, and there is no third + * outcome here: + * + * - `metadata-protocol`'s `saveMetaItem` resolves metadata type `object` to + * this very `ObjectSchema` (`spec/kernel/metadata-type-schemas.ts` binds + * `object: ObjectSchema`, and `resolveOverlaySchema` reads that registry); + * - it `safeParse`s the WHOLE item and, on failure, throws + * `422 INVALID_METADATA` with the zod issues attached — **before** any + * persistence; + * - so the array was REFUSED. Not stripped (nothing strips it), not stored + * (the throw precedes the write). The "stored verbatim" outcome objectui#6238 + * measured applies to types whose schema is tolerant or unregistered, and + * `object` is neither. + * + * Every designer object save and every designer field save that went through + * this service was therefore a 422 that persisted nothing. + * + * ## The `{ undefined: … }` trap, and why WE have to be the one that fails + * + * Measured on the installed 17.2.0 and asserted in `the instrument` below: + * `fields: { undefined: { … } }` PARSES GREEN. The spec cannot catch a + * nameless field once it has been keyed, so a conversion that keyed blindly + * would have traded a loud, harmless 422 for a silently corrupt STORED + * document. `toFieldsMap` throws instead, and the pins below assert both that + * it throws and that NO request was issued. + */ + +import { describe, expect, it, vi } from 'vitest'; +import { ObjectSchema } from '@objectstack/spec/data'; +import { ObjectStackAdapter } from '@object-ui/data-objectstack'; +import type { DesignerFieldDefinition, ObjectDefinition } from '@object-ui/types'; +import { MetadataService } from './MetadataService'; + +/** + * Captures the bodies of every PUT the SDK issued, exactly as they went over + * the wire, and serves a caller-supplied document to the GET `saveFields` does. + */ +function makeCapturingAdapter(served?: Record) { + const puts: Array> = []; + const gets: string[] = []; + const adapter = new ObjectStackAdapter({ + baseUrl: 'http://test.local', + fetch: vi.fn(async (input: RequestInfo | URL, init?: RequestInit) => { + const method = (init?.method ?? 'GET').toUpperCase(); + if (method === 'PUT') { + puts.push(JSON.parse(String(init?.body ?? '{}')) as Record); + } + if (method === 'GET') { + gets.push(String(input)); + if (served) { + return new Response(JSON.stringify({ item: served }), { + status: 200, + headers: { 'content-type': 'application/json' }, + }); + } + } + return new Response(JSON.stringify({ success: true }), { + status: 200, + headers: { 'content-type': 'application/json' }, + }); + }) as unknown as typeof fetch, + }); + return { adapter, puts, gets }; +} + +const issuesOf = (result: ReturnType): string[] => + result.success ? [] : result.error.issues.map((i) => `${i.code} @ ${i.path.join('.')}`); + +/** The `fields` value of the last PUT, as it was serialised. */ +const fieldsOf = (puts: Array>): Record> => + puts[puts.length - 1].fields as Record>; + +const ACCOUNT: ObjectDefinition = { + id: 'account', + name: 'account', + label: 'Account', + pluralLabel: 'Accounts', + isSystem: false, + fieldCount: 2, +}; + +const designerField = (name: string, over: Partial = {}): DesignerFieldDefinition => ({ + id: name, + name, + label: name, + type: 'text', + ...over, +}); + +// --------------------------------------------------------------------------- + +describe('the instrument', () => { + it('is the installed spec schema, and it refuses an ARRAY at the VALUE level', () => { + // The defect, stated on the schema before any claim about the fix. + expect(issuesOf(ObjectSchema.safeParse({ name: 'account', label: 'Account', fields: [{ name: 'n', type: 'text', label: 'N' }] }))).toEqual([ + 'invalid_type @ fields', + ]); + // An EMPTY array too — the refusal is the container's type, not its contents. + expect(issuesOf(ObjectSchema.safeParse({ name: 'account', label: 'Account', fields: [] }))).toEqual([ + 'invalid_type @ fields', + ]); + }); + + it('accepts the MAP — the control that makes the line above a shape result, not a schema refusing everything', () => { + expect(ObjectSchema.safeParse({ name: 'account', label: 'Account', fields: { n: { type: 'text', label: 'N' } } }).success).toBe(true); + expect(ObjectSchema.safeParse({ name: 'account', label: 'Account', fields: {} }).success).toBe(true); + // …and it accepts an entry that still carries its own `name`, which is what + // `toFieldPayload` produces. The conversion keys the entries; it does not + // have to strip them. + expect(ObjectSchema.safeParse({ name: 'account', label: 'Account', fields: { n: { name: 'n', type: 'text', label: 'N' } } }).success).toBe(true); + }); + + it('requires `fields` — omitting the key is refused with the same issue an array is', () => { + // Why `toObjectPayload` may not spell "the caller did not say" as `{}`: + // both of these are refusals, and only one of them is safe. + expect(issuesOf(ObjectSchema.safeParse({ name: 'account', label: 'Account' }))).toEqual([ + 'invalid_type @ fields', + ]); + }); + + it('does NOT catch a nameless field once keyed — `{ undefined: … }` parses GREEN', () => { + // The measured reason `toFieldsMap` throws. If this ever goes false the + // guard is still right, but its justification changed and should be re-read. + expect(ObjectSchema.safeParse({ name: 'account', label: 'Account', fields: { undefined: { type: 'text', label: 'N' } } }).success).toBe(true); + }); + + it('keys the record with a snake_case rule — `__proto__` is a LEGAL field name', () => { + // Which is why `toFieldsMap` builds through `Object.fromEntries`: plain + // assignment would invoke the prototype setter and drop the field silently. + expect(ObjectSchema.safeParse({ name: 'account', label: 'Account', fields: { __proto__: { type: 'text', label: 'P' } } }).success).toBe(true); + expect(issuesOf(ObjectSchema.safeParse({ name: 'account', label: 'Account', fields: { firstName: { type: 'text', label: 'F' } } }))).toEqual([ + 'invalid_key @ fields.firstName', + ]); + }); +}); + +describe('objectui#6240 · saveObject PUTs `fields` as a name-keyed map', () => { + it('writes a map, not an array — asserted on the request bytes', async () => { + const { adapter, puts } = makeCapturingAdapter(); + + await new MetadataService(adapter).saveObject(ACCOUNT, [ + { name: 'name', type: 'text', label: 'Name' }, + { name: 'amount', type: 'number', label: 'Amount' }, + ]); + + // Falsification: the save really happened and really described this object. + expect(puts).toHaveLength(1); + expect(puts[0].name).toBe('account'); + expect(puts[0].label).toBe('Account'); + + expect(Array.isArray(puts[0].fields)).toBe(false); + expect(fieldsOf(puts)).toEqual({ + name: { name: 'name', type: 'text', label: 'Name' }, + amount: { name: 'amount', type: 'number', label: 'Amount' }, + }); + }); + + it('and the whole body now parses green — the red-to-green witness of this card', async () => { + const { adapter, puts } = makeCapturingAdapter(); + await new MetadataService(adapter).saveObject(ACCOUNT, [{ name: 'name', type: 'text', label: 'Name' }]); + // Before this change: ['invalid_type @ fields']. + expect(issuesOf(ObjectSchema.safeParse(puts[0]))).toEqual([]); + expect(ObjectSchema.safeParse(puts[0]).success).toBe(true); + }); + + it('preserves declaration order, which is the only field order the spec has', async () => { + // objectui#6045 removed the field-level `sortOrder` precisely because order + // IS the record's insertion order. A conversion that sorted or grouped keys + // would silently reorder every object's fields. + const { adapter, puts } = makeCapturingAdapter(); + await new MetadataService(adapter).saveObject(ACCOUNT, [ + { name: 'zeta', type: 'text', label: 'Z' }, + { name: 'alpha', type: 'text', label: 'A' }, + { name: 'mid', type: 'text', label: 'M' }, + ]); + expect(Object.keys(fieldsOf(puts))).toEqual(['zeta', 'alpha', 'mid']); + }); + + it('omits `fields` entirely when the caller supplied none — it does NOT write `{}`', async () => { + // The anti-wipe control, and the cell where the two readings of "no fields" + // disagree. `{}` parses green and a PUT is an upsert, so emitting it for a + // caller that simply did not pass `existingFields` would delete every field + // of the object. The body stays refused instead — unchanged from before + // this card, and deliberately so. + const { adapter, puts } = makeCapturingAdapter(); + await new MetadataService(adapter).saveObject(ACCOUNT); + + expect(puts).toHaveLength(1); + expect('fields' in puts[0]).toBe(false); + expect(issuesOf(ObjectSchema.safeParse(puts[0]))).toEqual(['invalid_type @ fields']); + // Positive control: the rest of the object still went out. + expect(puts[0]).toMatchObject({ name: 'account', label: 'Account', pluralLabel: 'Accounts' }); + }); +}); + +describe('objectui#6240 · saveFields no longer converts the server’s map INTO an array', () => { + it('writes a name-keyed map built from the designer fields', async () => { + const { adapter, puts } = makeCapturingAdapter({ + name: 'account', + label: 'Account', + fields: { legacy: { type: 'text', label: 'Legacy' } }, + }); + + await new MetadataService(adapter).saveFields('account', [ + designerField('first_name', { label: 'First name' }), + designerField('amount', { type: 'number', label: 'Amount' }), + ]); + + expect(puts).toHaveLength(1); + expect(Array.isArray(puts[0].fields)).toBe(false); + expect(Object.keys(fieldsOf(puts))).toEqual(['first_name', 'amount']); + expect(fieldsOf(puts).first_name).toMatchObject({ name: 'first_name', type: 'text', label: 'First name' }); + // The designer's list is authoritative: the server's `legacy` field is gone + // because the designer no longer lists it, which is what a field save means. + expect('legacy' in fieldsOf(puts)).toBe(false); + }); + + it('and that body parses green, where the array made it a 422', async () => { + const { adapter, puts } = makeCapturingAdapter({ name: 'account', label: 'Account' }); + await new MetadataService(adapter).saveFields('account', [designerField('first_name', { label: 'First name' })]); + expect(issuesOf(ObjectSchema.safeParse(puts[0]))).toEqual([]); + }); + + it('STILL preserves unknown server keys through the conversion — the property the spread carries', async () => { + // The reshape must not cost what `...existingObject` was already buying. + // It matters more now than it did, not less: while the body was refused, + // nothing it preserved ever reached storage. + const { adapter, puts } = makeCapturingAdapter({ + name: 'account', + label: 'Account', + pluralLabel: 'Accounts', + icon: 'Building', + fieldGroups: { contact: { label: 'Contact' } }, + fields: { legacy: { type: 'text', label: 'Legacy' } }, + }); + + await new MetadataService(adapter).saveFields('account', [designerField('first_name', { label: 'First name' })]); + + expect(puts[0]).toMatchObject({ + name: 'account', + label: 'Account', + pluralLabel: 'Accounts', + icon: 'Building', + fieldGroups: { contact: { label: 'Contact' } }, + }); + // Positive control in the same output: `fields` really WAS replaced, so the + // assertion above is about preservation and not about a body that was + // echoed back whole. + expect(Object.keys(fieldsOf(puts))).toEqual(['first_name']); + }); + + it('treats an empty field list as authoritative and writes `{}` — the asymmetry with saveObject', async () => { + // Here the designer IS stating the object's complete field set, so "no + // fields" is something it can mean. `saveObject`'s optional parameter is + // the opposite case and is pinned above. + const { adapter, puts } = makeCapturingAdapter({ name: 'account', label: 'Account' }); + await new MetadataService(adapter).saveFields('account', []); + expect(puts[0].fields).toEqual({}); + expect(ObjectSchema.safeParse(puts[0]).success).toBe(true); + }); + + it('agrees with saveObject on the container shape — the two writers no longer disagree', async () => { + // The card's headline: "the designer's two write paths disagree with each + // other". Measured on both bodies at once so a fix to one alone reds this. + const a = makeCapturingAdapter(); + await new MetadataService(a.adapter).saveObject(ACCOUNT, [{ name: 'first_name', type: 'text', label: 'First name' }]); + const b = makeCapturingAdapter({ name: 'account', label: 'Account' }); + await new MetadataService(b.adapter).saveFields('account', [designerField('first_name', { label: 'First name' })]); + + expect(Object.keys(fieldsOf(a.puts))).toEqual(Object.keys(fieldsOf(b.puts))); + expect(Array.isArray(a.puts[0].fields)).toBe(Array.isArray(b.puts[0].fields)); + expect(Array.isArray(a.puts[0].fields)).toBe(false); + }); +}); + +describe('objectui#6240 · a field with no name FAILS LOUDLY, and nothing is sent', () => { + const nameless = { type: 'text', label: 'Nameless' } as unknown as { name: string; type: string; label: string }; + + it('throws rather than writing a `{ undefined: … }` entry — saveObject', async () => { + const { adapter, puts } = makeCapturingAdapter(); + await expect(new MetadataService(adapter).saveObject(ACCOUNT, [nameless])).rejects.toThrow(/has no `name`/); + // The half a "it threw" assertion cannot see: no request was issued, so + // there is no half-written document behind the throw. + expect(puts).toHaveLength(0); + }); + + it('throws rather than writing a `{ undefined: … }` entry — saveFields', async () => { + const { adapter, puts } = makeCapturingAdapter({ name: 'account', label: 'Account' }); + await expect( + new MetadataService(adapter).saveFields('account', [ + designerField('ok'), + { id: 'x', label: 'Nameless', type: 'text' } as unknown as DesignerFieldDefinition, + ]), + ).rejects.toThrow(/has no `name`/); + expect(puts).toHaveLength(0); + }); + + it('names the offending position, so the message is actionable', async () => { + const { adapter } = makeCapturingAdapter(); + await expect( + new MetadataService(adapter).saveObject(ACCOUNT, [{ name: 'ok', type: 'text', label: 'OK' }, nameless]), + ).rejects.toThrow(/index 1/); + }); + + it('counts a blank name as no name — `""` would key as the empty string', async () => { + const { adapter, puts } = makeCapturingAdapter(); + await expect( + new MetadataService(adapter).saveObject(ACCOUNT, [{ name: ' ', type: 'text', label: 'Blank' }]), + ).rejects.toThrow(/has no `name`/); + expect(puts).toHaveLength(0); + }); + + it('refuses duplicate names — the hazard the ARRAY did not have', async () => { + // An array carries two entries called `n`; a map cannot, so the second + // would silently swallow the first. That loss is introduced BY the + // conversion, so the conversion is what has to refuse it. + const { adapter, puts } = makeCapturingAdapter(); + await expect( + new MetadataService(adapter).saveObject(ACCOUNT, [ + { name: 'amount', type: 'number', label: 'Amount' }, + { name: 'amount', type: 'text', label: 'Amount again' }, + ]), + ).rejects.toThrow(/duplicate field name `amount`/); + expect(puts).toHaveLength(0); + }); + + it('keys a field literally named `__proto__` instead of silently dropping it', async () => { + // `__proto__` matches the record's key rule, so it is authorable. Built by + // assignment it would set the prototype and vanish from the serialised + // body; built by `Object.fromEntries` it is an own property. + const { adapter, puts } = makeCapturingAdapter(); + await new MetadataService(adapter).saveObject(ACCOUNT, [ + { name: '__proto__', type: 'text', label: 'Proto' }, + { name: 'amount', type: 'number', label: 'Amount' }, + ]); + expect(Object.keys(fieldsOf(puts))).toEqual(['__proto__', 'amount']); + expect(fieldsOf(puts).__proto__).toMatchObject({ type: 'text', label: 'Proto' }); + }); +}); + +describe('objectui#6240 · the honest limit of this fix', () => { + it('does not make every designer field name spec-legal — the key rule is still the server’s', async () => { + // A camelCase field name is refused at the KEY level now instead of the + // container level. Both are 422s; the difference is that the author is told + // which field, which is what the framework's own container-issue descent + // exists to surface. Fixing the designer's naming is not this card. + const { adapter, puts } = makeCapturingAdapter(); + await new MetadataService(adapter).saveObject(ACCOUNT, [{ name: 'firstName', type: 'text', label: 'First' }]); + expect(issuesOf(ObjectSchema.safeParse(puts[0]))).toEqual(['invalid_key @ fields.firstName']); + }); +}); diff --git a/packages/app-shell/src/services/MetadataService.retiredFieldSortOrder.test.ts b/packages/app-shell/src/services/MetadataService.retiredFieldSortOrder.test.ts index 35abb69ed..eab3293cd 100644 --- a/packages/app-shell/src/services/MetadataService.retiredFieldSortOrder.test.ts +++ b/packages/app-shell/src/services/MetadataService.retiredFieldSortOrder.test.ts @@ -96,9 +96,18 @@ function makeCapturingAdapter() { return { adapter, puts }; } -/** The field defs of the last PUT, in wire order. */ +/** + * The field defs of the last PUT, in wire order. + * + * `fields` is a name-keyed MAP on the wire (objectui#6240 — `ObjectSchema` + * refuses an array at the value level), and this file's subject is what is + * INSIDE one field def, so it reads the map's values in insertion order, which + * is the only field order the spec has. The CONTAINER shape is pinned by + * `MetadataService.objectPayloadFieldsMap.test.ts`, deliberately not here. + */ function savedFields(puts: Array>): Record[] { - return puts[puts.length - 1].fields as Record[]; + const fields = puts[puts.length - 1].fields as Record>; + return Object.values(fields); } const unrecognizedKeys = (result: ReturnType): string[] => diff --git a/packages/app-shell/src/services/MetadataService.retiredFormula.test.ts b/packages/app-shell/src/services/MetadataService.retiredFormula.test.ts index f9464471e..b9d519f34 100644 --- a/packages/app-shell/src/services/MetadataService.retiredFormula.test.ts +++ b/packages/app-shell/src/services/MetadataService.retiredFormula.test.ts @@ -69,9 +69,18 @@ function makeCapturingAdapter() { return { adapter, puts }; } -/** The field defs of the last PUT, in wire order. */ +/** + * The field defs of the last PUT, in wire order. + * + * `fields` is a name-keyed MAP on the wire (objectui#6240 — `ObjectSchema` + * refuses an array at the value level), and this file's subject is what is + * INSIDE one field def, so it reads the map's values in insertion order, which + * is the only field order the spec has. The CONTAINER shape is pinned by + * `MetadataService.objectPayloadFieldsMap.test.ts`, deliberately not here. + */ function savedFields(puts: Array>): Record[] { - return puts[puts.length - 1].fields as Record[]; + const fields = puts[puts.length - 1].fields as Record>; + return Object.values(fields); } const unrecognizedKeys = (result: ReturnType): string[] => diff --git a/packages/app-shell/src/services/MetadataService.specKeyObjectPayload.test.ts b/packages/app-shell/src/services/MetadataService.specKeyObjectPayload.test.ts index 1adda2ff1..c70ab88fe 100644 --- a/packages/app-shell/src/services/MetadataService.specKeyObjectPayload.test.ts +++ b/packages/app-shell/src/services/MetadataService.specKeyObjectPayload.test.ts @@ -232,19 +232,33 @@ describe('objectui#6223 · the whole body, and the honest limit of this fix', () }); }); - it('does NOT parse green as a whole — `fields` is an array where the spec wants a map (objectui#6240)', async () => { - // Stated as an assertion rather than left as prose, because a reader who - // saw only the `unrecognized_keys` case above would reasonably conclude the - // designer's object payload is now spec-valid. It is not: this fix closes - // the KEY-NAME class, which is the class objectui#6223 and its parity gate - // are about. What remains is a VALUE-level rejection — the gate's coverage - // note 4 — and it is filed separately as objectui#6240. - const result = ObjectSchema.safeParse(await putFor()); - expect(result.success).toBe(false); + it('parses green as a whole — the VALUE-level half closed too (objectui#6240)', async () => { + // ⭐ THIS CASE IS THE RED-TO-GREEN WITNESS OF objectui#6240, and it used to + // assert the OPPOSITE: + // + // expect(result.success).toBe(false); + // expect(issues).toEqual(['invalid_type @ fields']); + // + // That was correct and load-bearing while objectui#6240 was open. This file + // closed the KEY-NAME class (objectui#6223) and deliberately pinned the + // remaining VALUE-level rejection — the parity gate's coverage note 4 — so + // that it could not silently change. It has now changed, on purpose: + // `toObjectPayload` emits `fields` as the name-keyed MAP `ObjectSchema` + // requires instead of an array, so the whole document parses. + // + // The pin is REPLACED rather than deleted, because the claim it was really + // making — "judge the whole body, not just its key names" — is still the + // claim, and it is stronger green than red. The container shape itself, + // both writers, and the failure modes the conversion introduces are pinned + // in `MetadataService.objectPayloadFieldsMap.test.ts`. + const put = await putFor(); + const result = ObjectSchema.safeParse(put); expect(unrecognizedKeys(result)).toEqual([]); - expect(result.error?.issues.map((i) => `${i.code} @ ${i.path.join('.')}`)).toEqual([ - 'invalid_type @ fields', - ]); + expect(result.error?.issues.map((i) => `${i.code} @ ${i.path.join('.')}`)).toBeUndefined(); + expect(result.success).toBe(true); + // Falsification: green over a body that really carries the field, rather + // than over an emptied one. + expect(put.fields).toEqual({ name: { name: 'name', type: 'text', label: 'Name' } }); }); it('a half-filled object — no group, no sortOrder, no relationships — puts identical bytes, as it always did', async () => { diff --git a/packages/app-shell/src/services/MetadataService.specKeyReference.test.ts b/packages/app-shell/src/services/MetadataService.specKeyReference.test.ts index 607f58bdf..90bdabb7f 100644 --- a/packages/app-shell/src/services/MetadataService.specKeyReference.test.ts +++ b/packages/app-shell/src/services/MetadataService.specKeyReference.test.ts @@ -70,10 +70,18 @@ function makeCapturingAdapter() { return { adapter, puts }; } -/** The field defs of the last PUT, keyed by field name. */ +/** + * The field defs of the last PUT, in wire order. + * + * `fields` is a name-keyed MAP on the wire (objectui#6240 — `ObjectSchema` + * refuses an array at the value level), and this file's subject is what is + * INSIDE one field def, so it reads the map's values in insertion order, which + * is the only field order the spec has. The CONTAINER shape is pinned by + * `MetadataService.objectPayloadFieldsMap.test.ts`, deliberately not here. + */ function savedFields(puts: Array>): Record[] { - const last = puts[puts.length - 1]; - return last.fields as Record[]; + const fields = puts[puts.length - 1].fields as Record>; + return Object.values(fields); } const unrecognizedKeys = (result: ReturnType): string[] => diff --git a/packages/app-shell/src/services/MetadataService.ts b/packages/app-shell/src/services/MetadataService.ts index 27c4cf65d..403bc0a95 100644 --- a/packages/app-shell/src/services/MetadataService.ts +++ b/packages/app-shell/src/services/MetadataService.ts @@ -52,7 +52,36 @@ export interface ObjectMetadataPayload { // `toObjectPayload`); the key reached the wire only through the tombstone // bodies the two delete methods wrote by hand, and those now go through the // metadata API's own delete door instead — see `deleteMetadataItem`. - fields?: FieldMetadataPayload[]; + /** + * The object's fields, keyed by field NAME — the map `ObjectSchema` requires + * (objectui#6240). This used to be `FieldMetadataPayload[]`. + * + * An array is a VALUE-level refusal, which is the class the key-name parity + * gate (`scripts/check-designer-field-key-parity.mjs`, coverage note 4) + * cannot see: `fields` is in `ObjectSchema`'s accept set under either shape, + * so the gate was green for as long as the array was on the wire. + * + * Measured against the installed `@objectstack/spec` 17.2.0 (ESM build): + * + * fields: [{ name: 'n', type: 'text', label: 'N' }] + * => invalid_type @ fields ("expected record, received array") + * fields: [] => invalid_type @ fields + * fields: { n: { type: 'text' } } => parses green + * fields: {} => parses green + * (key absent) => invalid_type @ fields — it is REQUIRED + * + * And the route is not lenient about it. `metadata-protocol`'s + * `saveMetaItem` resolves type `object` to this very `ObjectSchema` + * (`spec/kernel/metadata-type-schemas.ts`), `safeParse`s the whole item and + * THROWS `422 INVALID_METADATA` before anything is persisted — the array was + * refused, never stripped and never stored. + * + * Still OPTIONAL, deliberately — see `toObjectPayload`. `undefined` here + * means "the caller did not say what the fields are", and that must NOT be + * spelled `{}`: `{}` parses green and a PUT is an upsert, so it would land as + * "this object has no fields" and wipe them. + */ + fields?: Record; // No `relationships` (objectui#6223): the spec models relationships on the // FIELD — `reference` / `master_detail` plus object-level `indexes` — and // `ObjectSchema` refuses an object-level `relationships` array by name. What @@ -133,10 +162,88 @@ function toObjectPayload(obj: ObjectDefinition, fields?: FieldMetadataPayload[]) pluralLabel: obj.pluralLabel, description: obj.description, icon: obj.icon, - fields, + // No fields supplied means the CALLER DID NOT SAY, which is not the same + // statement as `{}` ("this object has no fields") — and a PUT is an upsert, + // so writing `{}` here would delete every field of the object on a save + // that only meant to rename it. The body then still fails `ObjectSchema` + // (`fields` is required), which is a loud 422 that persists nothing — the + // right outcome for a caller that under-specified an upsert, and the same + // outcome as before this change. `saveFields` is the opposite case and + // treats its argument as authoritative; see there. + fields: fields ? toFieldsMap(fields) : undefined, }; } +/** + * Key a list of field payloads by field NAME — the shape `ObjectSchema.fields` + * requires (objectui#6240). + * + * Declaration order is preserved, and that is load-bearing rather than + * incidental: the spec models field order as DECLARATION ORDER in this record + * and has no field-level ordering key at all (objectui#6045), so insertion + * order here IS the designer's order. + * + * ## Why a missing name THROWS instead of writing `{ undefined: … }` + * + * The key can only come from the field's `name`. Both writer inputs declare it + * required (`FieldMetadataPayload.name`, `DesignerFieldDefinition.name`), but + * neither writer owns its input at runtime: `saveObject`'s `existingFields` is + * public API (`MetadataService` is reachable from app-shell's barrel through + * `useMetadataService`) and `saveFields` is handed whatever the designer's + * in-memory model holds. A nameless field keys as the literal string + * `"undefined"` — and the spec does NOT catch that. Measured on 17.2.0: + * + * ObjectSchema.safeParse({ …, fields: { undefined: { type: 'text', label: 'N' } } }) + * => success = true + * + * So the loud, immediate, harmless array-shaped 422 would have been traded for + * a silently corrupt STORED document. That is the AI-authored-metadata failure + * mode this repo keeps closing, and this conversion is exactly where it would + * have been opened. + * + * ## Why a duplicate name throws too + * + * That one is the conversion's OWN hazard rather than an inherited one: an + * array can carry two entries named `n` and a map cannot, so the later entry + * would silently swallow the earlier. Refusing is the only reading that does + * not lose a field the caller declared. + * + * ## Why `Object.fromEntries` and not assignment into a literal + * + * `map['__proto__'] = field` does not create a key — it invokes the prototype + * setter — and `__proto__` is a SPEC-LEGAL field name (the record's key schema + * is `/^[a-z_][a-z0-9_]*$/`). The assignment form would therefore drop such a + * field silently, which is this function's whole subject wearing a different + * spelling. `Object.fromEntries` defines an own property instead. + */ +function toFieldsMap(fields: FieldMetadataPayload[]): Record { + const entries: Array<[string, FieldMetadataPayload]> = []; + const seen = new Set(); + + fields.forEach((field, index) => { + const name = field?.name; + if (typeof name !== 'string' || name.trim() === '') { + throw new Error( + `[MetadataService] cannot build the object's \`fields\` map: the field at index ${index} has no ` + + '`name`. `ObjectSchema.fields` is keyed by field name, so a nameless field would be written under ' + + 'the literal key "undefined" — which the spec ACCEPTS, leaving a corrupt document stored with ' + + 'nothing to report it. Give the field a name.', + ); + } + if (seen.has(name)) { + throw new Error( + `[MetadataService] cannot build the object's \`fields\` map: duplicate field name \`${name}\` at ` + + `index ${index}. A name-keyed map cannot carry two fields under one name, so the later one would ` + + 'silently replace the earlier. Rename or remove one of them.', + ); + } + seen.add(name); + entries.push([name, field]); + }); + + return Object.fromEntries(entries); +} + /** * Convert a `DesignerFieldDefinition` (UI) to the API payload shape. * @@ -314,8 +421,38 @@ export class MetadataService { /** * Persist updated fields for an object. * - * Fetches the current object metadata, replaces its `fields` array with the + * Fetches the current object metadata, replaces its `fields` MAP with the * provided designer fields, and saves the whole object back. + * + * It used to write an ARRAY here (objectui#6240), and note which direction + * that ran in: the server's own document arrives with `fields` as a map, and + * `fields.map(toFieldPayload)` converted the correct shape INTO the refused + * one on every field save. `ObjectSchema` requires a record, so the resulting + * PUT was answered `422 INVALID_METADATA` (`invalid_type @ fields`) and + * nothing persisted — measured, not inferred: `metadata-protocol`'s + * `saveMetaItem` parses the whole item against `ObjectSchema` and throws + * before it writes. + * + * Two properties of this body are deliberate and are pinned in + * `MetadataService.objectPayloadFieldsMap.test.ts`: + * + * - **The spread still preserves unknown server keys.** `...existingObject` + * is what carries every key of the fetched document this service does not + * model, and reshaping `fields` must not cost that. It now matters more + * than it did, not less: while the body was refused, nothing it preserved + * ever reached storage. + * - **The field list is AUTHORITATIVE, so an empty one writes `{}`.** That + * is the opposite of `saveObject`'s optional `existingFields`, and the + * asymmetry is the point: here the designer is stating the object's + * complete field set, so "no fields" is a thing it can mean; there, a + * missing argument means the caller did not say. + * + * ⚠ Per-FIELD unknown keys are still not carried over — the entries are built + * fresh from the designer model, so a key the server sent inside one field + * (an `expression`, a `precision`) is dropped. That is unchanged by this + * card and its sibling writer already solves it (`MetadataFieldsPage`'s + * `carryOver`), but it becomes REACHABLE here for the first time now that the + * body is no longer refused. Filed separately rather than folded in. */ async saveFields(objectName: string, fields: DesignerFieldDefinition[]): Promise { const client = this.adapter.getClient(); @@ -332,7 +469,7 @@ export class MetadataService { const updatedObject = { ...existingObject, name: objectName, - fields: fields.map(toFieldPayload), + fields: toFieldsMap(fields.map(toFieldPayload)), }; await client.meta.saveItem('object', objectName, updatedObject);