diff --git a/.changeset/6488-carry-over-field-keys.md b/.changeset/6488-carry-over-field-keys.md new file mode 100644 index 0000000000..619f637a0f --- /dev/null +++ b/.changeset/6488-carry-over-field-keys.md @@ -0,0 +1,39 @@ +--- +'@object-ui/app-shell': patch +--- + +`MetadataService.saveFields` carries the server's per-FIELD keys through a field save +instead of rebuilding every entry from the designer model (objectui#6488). + +The method preserved unknown keys of the OBJECT document by spreading it, but that spread +is object-level and said nothing about keys INSIDE a field. Every entry was rebuilt by +`toFieldPayload`, so every key the server sent inside a field that the designer does not +model was dropped on every field save: `expression` (a formula authored in metadata-admin), +`precision`, `scale`, `system`, `sortable`, and anything a plugin registered. Measured +against the installed `@objectstack/spec` 17.2.0, `FieldSchema` accepts all five — the +designer's model is a subset of what a field may hold, and the difference was being +deleted. + +The loss is not new but was UNREACHABLE. While `fields` went out as an array the whole +body was refused `422 INVALID_METADATA` before persistence, so nothing `saveFields` +dropped ever reached storage; objectui#6240 made the body parse, and a PUT is an upsert, +so from that fix onward the drop lands. + +`toFieldPayload` now merges onto the previous SERVER entry, read from the document +`saveFields` already fetches for the object-level spread — the form +`MetadataFieldsPage.fromDesignerField` has used one writer over all along, and no extra +request. + +Two properties keep the fix from becoming its own mirror image, both pinned in +`MetadataService.fieldKeyCarryOver.test.ts`: + +- **A property the author CLEARED stays cleared.** Every modelled key is still written + unconditionally, so a cleared property arrives as an explicit `undefined` that overrides + the carried value and is dropped by `JSON.stringify` — absent from the body, which on an + upsert is the deletion. A conditional merge would leave the server's old value standing + and fail the author's deletion silently. +- **Retired designer keys do not ride back out.** `indexed`, `referenceTo`, `formula`, + `isSystem` and `sortOrder` are refused BY NAME by `FieldSchema`; a stored document can + still carry them, and echoing one back is a hard 422 that blocks every later save of the + object with no UI path to clear it. Everything else the server sent still survives — the + strip is keyed to those tombstones, not a blanket unknown-key purge. diff --git a/packages/app-shell/src/services/MetadataService.fieldKeyCarryOver.test.ts b/packages/app-shell/src/services/MetadataService.fieldKeyCarryOver.test.ts new file mode 100644 index 0000000000..904283f8ef --- /dev/null +++ b/packages/app-shell/src/services/MetadataService.fieldKeyCarryOver.test.ts @@ -0,0 +1,515 @@ +/** + * 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#6488 — `saveFields` carries the server's per-FIELD keys through a + * field save instead of rebuilding every entry from the designer model. + * + * `saveFields` preserves unknown keys of the DOCUMENT by spreading it + * (`...existingObject`, pinned in `MetadataService.objectPayloadFieldsMap.test.ts`). + * That spread is object-level and says nothing about keys INSIDE a field, and + * the entries were rebuilt wholesale by `toFieldPayload`, so every key the + * SERVER sent inside a field that the designer does not model was dropped on + * every field save: `expression` (a formula authored in metadata-admin), + * `precision`, `scale`, `system`, `sortable`, and anything a plugin registered. + * + * ## Why it lands now + * + * The loss is not new but was UNREACHABLE. While `fields` went out as an array + * the whole body was refused `422 INVALID_METADATA` before persistence + * (objectui#6240), so nothing `saveFields` dropped ever reached storage. + * objectui#6240 made the body parse; from that fix onward a PUT is an upsert + * and the drop lands. + * + * ## The two directions, and why the second one is here at all + * + * Adding carry-over opens the MIRROR of the defect: `{...prev, ...next}` can + * resurrect a value the author deliberately CLEARED, and a deletion that fails + * to persist is the same silent data loss pointing the other way. Every + * describe below therefore comes in a pair — a server key the designer does not + * model must SURVIVE, and a modelled property the designer cleared must stay + * ABSENT. Both are asserted on the captured request BYTES rather than on an + * in-memory object, because `undefined` is the whole mechanism of the second + * one: an explicitly-`undefined` modelled key overrides the carried value and + * is then dropped by `JSON.stringify`, which on an upsert IS the deletion. + * + * ## The neighbour + * + * objectui#6480 landed `MetadataService.readDecorationStrip.test.ts` on the + * neighbouring expression of this same function, running the OPPOSITE way: it + * drops framework read decorations the schema REFUSES, this one keeps author + * and plugin keys that should SURVIVE. The last describe asserts both hold at + * once, so a later edit cannot quietly undo one in service of the other. + */ + +import { describe, expect, it, vi } from 'vitest'; +import { FieldSchema, ObjectSchema } from '@objectstack/spec/data'; +import { ObjectStackAdapter } from '@object-ui/data-objectstack'; +import type { DesignerFieldDefinition } 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. + * + * Deliberately the same harness as `MetadataService.readDecorationStrip.test.ts` + * and `MetadataService.objectPayloadFieldsMap.test.ts`: assertions read + * `JSON.parse` of a captured request body, so what is measured is the bytes + * rather than an in-memory object that never had to serialise. + */ +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 designerField = (name: string, over: Partial = {}): DesignerFieldDefinition => ({ + id: name, + name, + label: name, + type: 'text', + ...over, +}); + +const fieldsOf = (put: Record): Record> => + put.fields as Record>; + +/** Every `unrecognized_keys` key the schema named, flattened. */ +const refusedKeys = (doc: unknown): string[] => { + const r = ObjectSchema.safeParse(doc); + if (r.success) return []; + return r.error.issues.flatMap((i) => + i.code === 'unrecognized_keys' ? ((i as unknown as { keys: string[] }).keys ?? []) : [], + ); +}; + +const issuesOf = (doc: unknown): string[] => { + const r = ObjectSchema.safeParse(doc); + return r.success ? [] : r.error.issues.map((i) => `${i.code} @ ${i.path.join('.')}`); +}; + +/** + * The keys this card is about: sent by the server INSIDE a field, accepted by + * `FieldSchema`, and named nowhere in `toFieldPayload`. + */ +const UNMODELLED_SERVER_KEYS = { + expression: 'price * quantity', + precision: 18, + scale: 2, + system: true, + sortable: true, +} as const; + +/** Keys a designer once wrote that `FieldSchema` refuses BY NAME. */ +const RETIRED_KEYS = { + indexed: true, + referenceTo: 'account', + formula: 'price * quantity', + isSystem: true, + sortOrder: 3, +} as const; + +// --------------------------------------------------------------------------- + +describe('the instrument', () => { + const base = { name: 'amount', type: 'number', label: 'Amount' }; + + it('ACCEPTS every key this card preserves — the reason the drop is a loss', () => { + // Control first, so what follows is a result about these keys rather than a + // schema that accepts everything. + expect(FieldSchema.safeParse(base).success).toBe(true); + for (const [key, value] of Object.entries(UNMODELLED_SERVER_KEYS)) { + expect(FieldSchema.safeParse({ ...base, [key]: value }).success).toBe(true); + } + // Together, and nested where they actually live. + expect(issuesOf({ name: 'account', label: 'Account', fields: { amount: { ...base, ...UNMODELLED_SERVER_KEYS } } })).toEqual([]); + }); + + it('REFUSES each retired designer key BY NAME — the reason carry-over is not verbatim', () => { + for (const [key, value] of Object.entries(RETIRED_KEYS)) { + const r = FieldSchema.safeParse({ ...base, [key]: value }); + expect(r.success).toBe(false); + expect(r.success ? [] : r.error.issues.map((i) => i.code)).toContain('unrecognized_keys'); + } + // Nested, this is the hard 422 that blocks EVERY later save of the object. + expect(refusedKeys({ name: 'account', label: 'Account', fields: { amount: { ...base, ...RETIRED_KEYS } } }).sort()).toEqual( + ['formula', 'indexed', 'isSystem', 'referenceTo', 'sortOrder'], + ); + }); + + it('refuses a plugin-registered key too — why the carry-over is NOT filtered by this schema', () => { + // The honest limit, stated on the instrument rather than in prose. The + // SERVER that sent such a key accepts it; the client's INSTALLED spec does + // not know it. Filtering the carry-over through `FieldSchema` here would + // therefore drop precisely the keys this card exists to preserve, which is + // why the strip is keyed to the retired-key tombstones instead. + expect(refusedKeys({ name: 'account', label: 'Account', fields: { amount: { ...base, x_plugin_thing: { a: 1 } } } })).toEqual([ + 'x_plugin_thing', + ]); + }); +}); + +// --------------------------------------------------------------------------- + +describe('objectui#6488 · a server key the designer does not model SURVIVES a field save', () => { + const SERVED = { + name: 'account', + label: 'Account', + fields: { + amount: { type: 'number', label: 'Amount', ...UNMODELLED_SERVER_KEYS, x_plugin_thing: { a: 1 } }, + }, + } satisfies Record; + + it('carries every one of them onto the PUT body — asserted on the request bytes', async () => { + const { adapter, puts } = makeCapturingAdapter(SERVED); + + await new MetadataService(adapter).saveFields('account', [ + designerField('amount', { type: 'number', label: 'Amount' }), + ]); + + // Falsification: the save really happened and really described this object, + // so what follows is a statement about a body that exists. + expect(puts).toHaveLength(1); + expect(puts[0].name).toBe('account'); + expect(Object.keys(fieldsOf(puts[0]))).toEqual(['amount']); + + // Before this fix each of these was ABSENT — the entry was rebuilt from the + // designer model, which names none of them. + expect(fieldsOf(puts[0]).amount).toMatchObject({ ...UNMODELLED_SERVER_KEYS, x_plugin_thing: { a: 1 } }); + }); + + it('and the designer model still wins on every key it DOES model', async () => { + const { adapter, puts } = makeCapturingAdapter(SERVED); + + await new MetadataService(adapter).saveFields('account', [ + designerField('amount', { type: 'currency', label: 'Deal amount', description: 'What it is worth' }), + ]); + + expect(fieldsOf(puts[0]).amount).toMatchObject({ + name: 'amount', + type: 'currency', + label: 'Deal amount', + description: 'What it is worth', + // …while the unmodelled keys rode along untouched. + expression: 'price * quantity', + precision: 18, + }); + }); + + it('carries onto the field of the SAME NAME only — a rename starts clean', async () => { + const { adapter, puts } = makeCapturingAdapter(SERVED); + + await new MetadataService(adapter).saveFields('account', [ + designerField('amount_v2', { type: 'number', label: 'Amount' }), + ]); + + // No previous entry under this name, so nothing to carry: the payload is + // exactly what the designer stated. + expect(fieldsOf(puts[0])).toEqual({ amount_v2: { name: 'amount_v2', type: 'number', label: 'Amount' } }); + // And the designer's list is still authoritative — the server's `amount` is + // gone because the designer no longer lists it (objectui#6240's property). + expect('amount' in fieldsOf(puts[0])).toBe(false); + }); + + it('adds NO request — the previous entries ride in on the document already fetched', async () => { + const { adapter, puts, gets } = makeCapturingAdapter(SERVED); + + await new MetadataService(adapter).saveFields('account', [designerField('amount', { type: 'number' })]); + + expect(gets).toHaveLength(1); + expect(puts).toHaveLength(1); + }); + + it('is a no-op when the served document has no usable `fields`', async () => { + // `undefined`, and the ARRAY shape a stored document cannot have + // (`ObjectSchema` answers `fields: []` with `invalid_type`, objectui#6240): + // both read as "no previous entries" rather than being guessed at. + for (const served of [ + { name: 'account', label: 'Account' }, + { name: 'account', label: 'Account', fields: [{ name: 'amount', type: 'number' }] }, + ]) { + const { adapter, puts } = makeCapturingAdapter(served as Record); + await new MetadataService(adapter).saveFields('account', [designerField('amount', { type: 'number', label: 'Amount' })]); + expect(fieldsOf(puts[0])).toEqual({ amount: { name: 'amount', type: 'number', label: 'Amount' } }); + expect(issuesOf(puts[0])).toEqual([]); + } + }); + + it('does not read `Object.prototype` as a previous entry for a field named `__proto__`', async () => { + // `__proto__` is a SPEC-LEGAL field name (the record's key rule is + // `/^[a-z_][a-z0-9_]*$/`, measured green), and a plain `previous['__proto__']` + // lookup returns the inherited `Object.prototype` — an object that is not a + // previous field entry at all. The other end of this same map already + // documents the hazard in `toFieldsMap`. + const { adapter, puts } = makeCapturingAdapter({ name: 'account', label: 'Account', fields: {} }); + + await new MetadataService(adapter).saveFields('account', [designerField('__proto__', { label: 'P' })]); + + // Read through a descriptor rather than a literal: `{ __proto__: … }` as an + // EXPECTED value sets the prototype instead of declaring a key, so the + // obvious spelling of this assertion compares against `{}` and passes on a + // body that dropped the field. (Measured — it did, on the first run.) + expect(Object.keys(fieldsOf(puts[0]))).toEqual(['__proto__']); + const entry = Object.getOwnPropertyDescriptor(fieldsOf(puts[0]), '__proto__')?.value as Record; + expect(entry).toEqual({ name: '__proto__', type: 'text', label: 'P' }); + // Nothing inherited rode in: `Object.prototype`'s members are all + // non-enumerable, so a spread of it is empty — the guard is what keeps this + // a statement about the designer's field rather than a lucky no-op. + expect(Object.keys(entry).sort()).toEqual(['label', 'name', 'type']); + expect(issuesOf(puts[0])).toEqual([]); + }); +}); + +// --------------------------------------------------------------------------- + +describe('objectui#6488 · the mirror — carry-over must NOT resurrect what the author cleared', () => { + /** A served entry with every modelled property populated. */ + const POPULATED = { + name: 'account', + label: 'Account', + fields: { + amount: { + name: 'amount', + type: 'number', + label: 'Amount', + group: 'financials', + description: 'The old help text', + required: true, + unique: true, + readonly: true, + hidden: true, + defaultValue: '0', + placeholder: 'Old placeholder', + options: [{ label: 'One', value: '1' }], + externalId: true, + trackHistory: true, + reference: 'opportunity', + // …and one key the designer does not model, as the positive control. + expression: 'price * quantity', + }, + }, + } satisfies Record; + + /** Every key `toFieldPayload` names, other than the two it always fills. */ + const CLEARABLE = [ + 'group', + 'description', + 'required', + 'unique', + 'readonly', + 'hidden', + 'defaultValue', + 'placeholder', + 'options', + 'externalId', + 'trackHistory', + 'reference', + ] as const; + + it('drops every modelled property the designer cleared — asserted on the request bytes', async () => { + const { adapter, puts } = makeCapturingAdapter(POPULATED); + + // The designer states the field with all of those properties removed. This + // is what a user clearing them in the FieldDesigner produces. + await new MetadataService(adapter).saveFields('account', [ + designerField('amount', { type: 'number', label: 'Amount' }), + ]); + + const entry = fieldsOf(puts[0]).amount; + // A PUT is an upsert, so ABSENT from the body is the deletion. Present with + // the server's old value would be the author's deletion silently failing — + // this card's own defect, pointing the other way. + for (const key of CLEARABLE) { + expect({ key, present: key in entry }).toEqual({ key, present: false }); + } + // The positive control in the same body: the fix is doing its job on the + // key the designer does NOT model, so the absences above are about clearing + // rather than about a carry-over that never ran. + expect(entry.expression).toBe('price * quantity'); + expect(entry).toEqual({ name: 'amount', type: 'number', label: 'Amount', expression: 'price * quantity' }); + }); + + it('clears them one at a time too — not only when the whole entry is emptied', async () => { + // A merge keyed on "the designer supplied nothing" would pass the case + // above and still resurrect a single cleared property. + for (const key of CLEARABLE) { + const { adapter, puts } = makeCapturingAdapter({ + name: 'account', + label: 'Account', + fields: { amount: { name: 'amount', type: 'number', label: 'Amount', [key]: (POPULATED.fields.amount as Record)[key] } }, + }); + + await new MetadataService(adapter).saveFields('account', [ + // Everything the designer still holds, with exactly this key cleared. + designerField('amount', { type: 'number', label: 'Amount', description: 'kept' }), + ]); + + const entry = fieldsOf(puts[0]).amount; + expect({ key, present: key in entry }).toEqual({ key, present: key === 'description' }); + } + }); + + it('overwrites rather than merges a modelled property the designer CHANGED', async () => { + const { adapter, puts } = makeCapturingAdapter(POPULATED); + + await new MetadataService(adapter).saveFields('account', [ + designerField('amount', { + type: 'number', + label: 'Amount', + description: 'The new help text', + options: [{ label: 'Two', value: '2' }], + required: false, + }), + ]); + + const entry = fieldsOf(puts[0]).amount; + expect(entry.description).toBe('The new help text'); + expect(entry.options).toEqual([{ label: 'Two', value: '2' }]); + // `false` is a VALUE, not a clearing — it must reach the wire as `false` + // rather than fall through to the server's `true`. + expect(entry.required).toBe(false); + }); + + it('clears a relationship target the author removed — `reference` does not come back', async () => { + // Called out on its own because it is the one modelled key whose designer + // spelling differs (`referenceTo` -> `reference`), so a carry-over that + // matched on the DESIGNER's key name would leave the server's `reference` + // standing and silently keep the lookup pointing at the old object. + const { adapter, puts } = makeCapturingAdapter(POPULATED); + + await new MetadataService(adapter).saveFields('account', [ + designerField('amount', { type: 'number', label: 'Amount', referenceTo: undefined }), + ]); + + expect('reference' in fieldsOf(puts[0]).amount).toBe(false); + }); +}); + +// --------------------------------------------------------------------------- + +describe('objectui#6488 · the carry-over is bounded — retired keys do not ride back out', () => { + it('drops each retired key, and the whole body parses green', async () => { + // Without this the fix would be a regression rather than one: a stored + // document carrying any of these would produce a hard 422 on every field + // save, and with the controls retired an author has no way to clear it. + const { adapter, puts } = makeCapturingAdapter({ + name: 'account', + label: 'Account', + fields: { amount: { name: 'amount', type: 'number', label: 'Amount', ...RETIRED_KEYS, ...UNMODELLED_SERVER_KEYS } }, + }); + + await new MetadataService(adapter).saveFields('account', [designerField('amount', { type: 'number', label: 'Amount' })]); + + for (const key of Object.keys(RETIRED_KEYS)) { + expect({ key, present: key in fieldsOf(puts[0]).amount }).toEqual({ key, present: false }); + } + // Positive control: the keys that SHOULD survive did, in the same body. + expect(fieldsOf(puts[0]).amount).toMatchObject(UNMODELLED_SERVER_KEYS); + expect(issuesOf(puts[0])).toEqual([]); + }); + + it('strips each one on its own, not only when all five are present', async () => { + for (const [key, value] of Object.entries(RETIRED_KEYS)) { + const { adapter, puts } = makeCapturingAdapter({ + name: 'account', + label: 'Account', + fields: { amount: { name: 'amount', type: 'number', label: 'Amount', [key]: value } }, + }); + await new MetadataService(adapter).saveFields('account', [designerField('amount', { type: 'number', label: 'Amount' })]); + + expect({ key, present: key in fieldsOf(puts[0]).amount }).toEqual({ key, present: false }); + expect(issuesOf(puts[0])).toEqual([]); + } + }); + + it('is NOT a lenient "drop whatever the schema refuses" pass (AGENTS.md #0.1)', async () => { + // A blanket purge would swallow this key and hide the producer's bug. The + // strip is bounded to the retired-key tombstones, so an off-spec key the + // AUTHOR owns still goes out and is still refused — loudly, where someone + // can see it. Same bounding assertion objectui#6480 makes one level up. + const { adapter, puts } = makeCapturingAdapter({ + name: 'account', + label: 'Account', + fields: { amount: { name: 'amount', type: 'number', label: 'Amount', indexed: true, notASpecKey: 'authored, wrong, and it must stay visible' } }, + }); + + await new MetadataService(adapter).saveFields('account', [designerField('amount', { type: 'number', label: 'Amount' })]); + + expect(fieldsOf(puts[0]).amount.notASpecKey).toBe('authored, wrong, and it must stay visible'); + expect('indexed' in fieldsOf(puts[0]).amount).toBe(false); + expect(refusedKeys(puts[0])).toEqual(['notASpecKey']); + }); +}); + +// --------------------------------------------------------------------------- + +describe('objectui#6488 · the neighbour — objectui#6480’s strip still holds', () => { + it('strips the object-level read decorations while carrying the field keys', async () => { + // The two edits sit on neighbouring expressions of one function and run in + // OPPOSITE directions. Asserted in one body so neither can be quietly + // undone in service of the other; `MetadataService.readDecorationStrip.test.ts` + // is the other half of this pin and must stay green beside it. + const { adapter, puts } = makeCapturingAdapter({ + name: 'account', + label: 'Account', + pluralLabel: 'Accounts', + fields: { amount: { name: 'amount', type: 'number', label: 'Amount', ...UNMODELLED_SERVER_KEYS } }, + _diagnostics: { valid: false, errors: [{ path: 'fields.amount', message: 'stale' }] }, + _draft: true, + }); + + await new MetadataService(adapter).saveFields('account', [designerField('amount', { type: 'number', label: 'Amount' })]); + + expect('_diagnostics' in puts[0]).toBe(false); + expect('_draft' in puts[0]).toBe(false); + expect(fieldsOf(puts[0]).amount).toMatchObject(UNMODELLED_SERVER_KEYS); + expect(puts[0].pluralLabel).toBe('Accounts'); + expect(issuesOf(puts[0])).toEqual([]); + }); + + it('does not carry a read decoration INSIDE a field, because none is served there', async () => { + // Measured upstream rather than assumed: `decorateMetadataItem` + // (`metadata-protocol/src/metadata-diagnostics.ts`) attaches `_diagnostics` + // to the ITEM, never to a nested field entry, which is why the object-level + // strip is sufficient and this carry-over needs no strip of its own. If the + // framework ever decorated per-field, `FieldSchema` refuses both keys by + // name and this case turns red rather than shipping a 422 to an author. + const { adapter, puts } = makeCapturingAdapter({ + name: 'account', + label: 'Account', + fields: { amount: { name: 'amount', type: 'number', label: 'Amount' } }, + }); + + await new MetadataService(adapter).saveFields('account', [designerField('amount', { type: 'number', label: 'Amount' })]); + + expect('_diagnostics' in fieldsOf(puts[0]).amount).toBe(false); + expect('_draft' in fieldsOf(puts[0]).amount).toBe(false); + }); +}); diff --git a/packages/app-shell/src/services/MetadataService.ts b/packages/app-shell/src/services/MetadataService.ts index d32a488a23..958eda1174 100644 --- a/packages/app-shell/src/services/MetadataService.ts +++ b/packages/app-shell/src/services/MetadataService.ts @@ -246,15 +246,129 @@ function toFieldsMap(fields: FieldMetadataPayload[]): Record unrecognized_keys — "never a FieldSchema key" (objectui#4644) + * referenceTo => unrecognized_keys — "Did you mean `referenceTo` -> `reference`?" (objectui#6041) + * formula => unrecognized_keys — "Did you mean `formula` -> `expression`?" (objectui#6043) + * isSystem => unrecognized_keys — "Did you mean `isSystem` -> `system`?" (objectui#6044) + * sortOrder => unrecognized_keys (objectui#6045) + * + * Every one of them is a key SOME designer build emitted before its card + * retired it, so a document stored back then can still carry it inside a field. + * Carrying it out again would be a hard `422 INVALID_METADATA` that blocks + * EVERY later save of that object — and with the controls gone, an author has + * no way to clear it from the UI. Stripping is what makes an edit-and-save + * round-trip of such an object come out parseable; nothing that the server + * would store is lost, because these are exactly the values it refuses. + * + * The list is keyed to those tombstones and is NOT a blanket unknown-key purge: + * every other key the server sent still rides through, which is the whole point + * of the carry-over. Deliberately not derived from `FieldSchema`'s accept set + * either — measured on 17.2.0, a plugin-registered key (`x_plugin_thing`) is + * `unrecognized_keys` to the INSTALLED spec while the SERVER that sent it + * accepts it, so filtering by the client's schema would drop precisely the keys + * this card exists to preserve. + * + * ⚠ This is the repo's THIRD copy of a retired-field-key list, each scoped to + * one writer's own history (`plugin-designer`'s `MetadataFieldsPage` carries + * four, `app-shell`'s `previews/object-fields-io` carries `['indexed']`). + * Unifying them spans `MetadataFieldsPage.tsx`, which objectui#6489 owns on + * this same seam, so it is filed rather than folded in here. + */ +const RETIRED_FIELD_KEYS = ['indexed', 'referenceTo', 'formula', 'isSystem', 'sortOrder'] as const; + +/** + * The previous SERVER entry for one field, minus {@link RETIRED_FIELD_KEYS} — + * the keys {@link toFieldPayload} spreads so a save cannot drop what the + * designer does not model (objectui#6488). + * + * Same shape, same name and the same reason as `MetadataFieldsPage`'s + * `carryOver`, which has solved this one writer over all along. + */ +function carryOver(prev?: Record): Record { + if (!prev) return {}; + const present = RETIRED_FIELD_KEYS.filter((k) => k in prev); + if (present.length === 0) return { ...prev }; + const next = { ...prev }; + for (const k of present) delete next[k]; + return next; +} + +/** + * The `fields` map of the fetched document, as a lookup for {@link carryOver}. + * + * Anything that is not a record is read as "no previous entries" rather than + * coerced. An ARRAY in particular cannot be a stored document's shape — + * `ObjectSchema` answers `fields: []` with `invalid_type` (objectui#6240), so a + * served array means something other than metadata came back, and guessing at + * its entries would be inventing a previous state to carry over. + */ +function previousFieldsOf(existingObject: Record): Record { + const raw = existingObject.fields; + if (!raw || typeof raw !== 'object' || Array.isArray(raw)) return {}; + return raw as Record; +} + +/** + * The previous entry for `name`, or `undefined` when there is none to carry. + * + * `hasOwnProperty` rather than a plain lookup, for the reason {@link toFieldsMap} + * documents at the other end of the same map: `__proto__` is a SPEC-LEGAL field + * name, and `previous['__proto__']` reads `Object.prototype` — an inherited + * object that is not a previous field entry at all. + */ +function previousFieldEntry(previous: Record, name: string): Record | undefined { + if (!Object.prototype.hasOwnProperty.call(previous, name)) return undefined; + const entry = previous[name]; + if (!entry || typeof entry !== 'object' || Array.isArray(entry)) return undefined; + return entry as Record; +} + +/** + * Convert a `DesignerFieldDefinition` (UI) to the API payload shape, carrying + * over the previous SERVER entry's unmodelled keys (objectui#6488). + * + * ## Why the carry-over + * + * This shape models what the FIELD DESIGNER can author, which is a subset of + * what `FieldSchema` accepts. Measured on the installed `@objectstack/spec` + * 17.2.0, `expression` (a formula authored in metadata-admin), `precision`, + * `scale`, `system` and `sortable` all parse green on a field — and none of + * them is a key this converter names. Rebuilding the entry from the designer + * model alone therefore DROPPED every one of them on every field save, and a + * PUT is an upsert, so the drop lands in storage. + * + * That loss is not new but was unreachable: while `fields` went out as an array + * the whole body was refused `422` before persistence (objectui#6240), so + * nothing this dropped ever reached storage. From that fix onward it does. + * + * ## Why the modelled keys are still written UNCONDITIONALLY + * + * The mirror hazard, and the one thing that would make this fix worse than the + * bug: carry-over must not resurrect a value the author deliberately CLEARED. + * Every key below is named on every call, so a cleared property arrives as an + * explicit `undefined` that OVERRIDES the carried value and is then dropped by + * `JSON.stringify` — absent from the body, which on an upsert is the deletion. + * A conditional spread (`...(field.x ? { x: field.x } : {})`) would leave the + * server's old value standing and fail the author's deletion silently. Pinned + * both ways in `MetadataService.fieldKeyCarryOver.test.ts`. * * It no longer copies `sortOrder` (objectui#6045). `FieldSchema` refuses that * key by name and nothing on the tree ever populated it, so the write was * latent — `JSON.stringify` drops the `undefined` — but one reorder feature * away from a hard 422 that blocks every later save of the object. */ -function toFieldPayload(field: DesignerFieldDefinition): FieldMetadataPayload { +function toFieldPayload( + field: DesignerFieldDefinition, + prev?: Record, +): FieldMetadataPayload & Record { return { + ...carryOver(prev), name: field.name, label: field.label, type: field.type, @@ -460,12 +574,16 @@ export class MetadataService { * Only the second kind may be dropped, and only because the read path * regenerates them. * - * ⚠ 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. + * …and a fourth, pinned in `MetadataService.fieldKeyCarryOver.test.ts`: + * + * - **Per-FIELD server keys are carried over** (objectui#6488). The spread + * above is object-level and does nothing for keys INSIDE a field, so + * entries rebuilt from the designer model dropped every key the server + * sent that this converter does not name — `expression`, `precision`, + * `scale`, `system`, `sortable`, anything a plugin registered. The + * previous entries ride in on the very document this method already + * fetched, so `toFieldPayload` merges onto them; see there for the mirror + * property that keeps a CLEARED designer property cleared. */ async saveFields(objectName: string, fields: DesignerFieldDefinition[]): Promise { const client = this.adapter.getClient(); @@ -479,6 +597,15 @@ export class MetadataService { // Object may not exist yet on the backend; proceed with fields-only save } + // The per-FIELD half of the same preservation property (objectui#6488). + // `...existingObject` below carries unknown keys of the DOCUMENT; it does + // nothing for keys INSIDE a field, and those entries are rebuilt from the + // designer model. The previous entries are already in hand — this is the + // document the object-level spread just fetched — so the carry-over costs + // no extra request, which is also why it belongs at this drop site rather + // than anywhere upstream. + const previousFields = previousFieldsOf(existingObject); + // `...existingObject` is a verbatim spread of whatever the server sent, so // simply not writing `_diagnostics` / `_draft` is not enough: a served // document that carries either one spreads it straight back out, and @@ -504,7 +631,7 @@ export class MetadataService { const updatedObject = stripReadDecorations({ ...existingObject, name: objectName, - fields: toFieldsMap(fields.map(toFieldPayload)), + fields: toFieldsMap(fields.map((field) => toFieldPayload(field, previousFieldEntry(previousFields, field.name)))), }) as Record; await client.meta.saveItem('object', objectName, updatedObject);