diff --git a/.changeset/6041-designer-reference-key.md b/.changeset/6041-designer-reference-key.md new file mode 100644 index 0000000000..803ca1328c --- /dev/null +++ b/.changeset/6041-designer-reference-key.md @@ -0,0 +1,44 @@ +--- +'@object-ui/plugin-designer': patch +'@object-ui/app-shell': patch +--- + +The field designer now reads and writes a lookup field's relationship target under the +spec's spelling `reference` (objectui#6041), in both directions. + +`referenceTo` is not in `FieldSchema`'s accept set. Measured against the installed +`@objectstack/spec` 17.2.0, through the whole object document that +`PUT /api/v1/meta/object/:name` validates: + +``` +ObjectSchema.safeParse({ …, fields: { rel: { type: 'lookup', label: 'Owner', + referenceTo: 'user' } } }) + => success = false + => unrecognized_keys at ["fields","rel"] keys=["referenceTo"] + "Did you mean `referenceTo` -> `reference`?" +``` + +so authoring a lookup field through the designer returned a hard 422 `INVALID_METADATA`, +and — because the key is then stored — blocked **every subsequent save** of that object, +with nothing in the UI to say which key did it. + +The read direction was broken symmetrically and is the half that would have survived a +write-only fix: `toDesignerField` read `raw.referenceTo` while a spec-parsed server sends +`reference`, so every already-saved lookup field loaded into the designer with an **empty +reference box**. Both wire-bound payload shapes move — `FieldMetadataPayload` +(`MetadataService.toFieldPayload`) and `ServerFieldSchema` +(`MetadataFieldsPage.fromDesignerField`). + +`referenceTo` also joins `RETIRED_FIELD_KEYS`. Renaming the emit sites alone does not +unblock an object whose stored fields already carry the misspelling: `carryOver` spreads +the previous server def verbatim, so the key would ride straight back out to the same 422. +The designer's in-memory `DesignerFieldDefinition` keeps `referenceTo` — that is the +internal prop name every other UI surface in this repo already uses (`LookupField`, +`filter-builder`, `ObjectChart`, `ListView`, `UserFilters`), it reaches no wire-bound +shape, and the parity gate classifies it as `uiOnly` rather than a violation. + +No behavioural change for a half-filled draft: the spec's prose calls `reference` +"required for relationship types", but that is not enforced by the zod parse at 17.2.0 — +`{ type: 'lookup', label: 'L' }` parses green at field level and through `ObjectSchema`, +and `undefined` is dropped by `JSON.stringify` under either spelling, so the wire bytes +are identical before and after. diff --git a/.changeset/6044-designer-system-key.md b/.changeset/6044-designer-system-key.md new file mode 100644 index 0000000000..09a3c92462 --- /dev/null +++ b/.changeset/6044-designer-system-key.md @@ -0,0 +1,36 @@ +--- +'@object-ui/plugin-designer': patch +--- + +The field designer now reads the system-field marker under the spec's spelling `system`, +and never hands `isSystem` back to the metadata API (objectui#6044). + +`isSystem` is not in `FieldSchema`'s accept set. Measured against the installed +`@objectstack/spec` 17.2.0: + +``` +FieldSchema.safeParse({ type: 'text', label: 'L', isSystem: true }) + => success = false + => unrecognized_keys keys=["isSystem"] "Did you mean `isSystem` -> `system`?" +``` + +Two defects, one misspelling, and they are two different sites. + +**The read was dead** — the quieter and worse half. `toDesignerField` read `raw.isSystem` +while a spec-parsed server sends `system`, so the flag was always `undefined`. Nothing went +red, because the flag is optional and `undefined` is a valid "not a system field". But it is +load-bearing: `FieldDesigner` refuses to delete a system field and disables its name and +type inputs, so with the read dead `organization_id`, `created_at` and friends presented as +ordinary editable, **deletable** business fields. + +**The write had no emit site at all.** `fromDesignerField` never names `isSystem`; its only +route out is the verbatim `...carryOver(prev)` spread, so a stored misspelling round-tripped +back to `PUT /api/v1/meta/object/:name` as a hard 422 `INVALID_METADATA` that blocks every +later save. The repair is a `RETIRED_FIELD_KEYS` tombstone rather than a renamed line — and +it is deliberately paired with the read fix, never a substitute for it: stripping alone would +close the 422 and fossilize the dead detection. The spec spelling `system` is not stripped, +so a server-injected flag rides through untouched and feeds the read. + +`app-shell`'s `FieldMetadataPayload` never declared the key, so `toFieldPayload` had nothing +to fix. The designer's in-memory `DesignerFieldDefinition` keeps `isSystem`: it reaches no +wire-bound shape and the parity gate classifies it as `uiOnly`. diff --git a/packages/app-shell/src/services/MetadataService.specKeyReference.test.ts b/packages/app-shell/src/services/MetadataService.specKeyReference.test.ts new file mode 100644 index 0000000000..607f58bdf7 --- /dev/null +++ b/packages/app-shell/src/services/MetadataService.specKeyReference.test.ts @@ -0,0 +1,156 @@ +/** + * 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#6041 — `MetadataService` writes the relationship target under the + * spec's spelling `reference`, never `referenceTo`. + * + * Surfaced by the key-level parity gate built for objectui#5761 + * (`scripts/check-designer-field-key-parity.mjs`). `FieldMetadataPayload` is + * one of that gate's two `wire` shapes: `toFieldPayload` builds it and + * `saveFields` PUTs `fields.map(toFieldPayload)` to + * `PUT /api/v1/meta/object/:name`. + * + * `referenceTo` is not in `FieldSchema`'s accept set. Measured against the + * installed `@objectstack/spec` 17.2.0, both at field level and through the + * whole object document: + * + * ObjectSchema.safeParse({ …, fields: { rel: { type: 'lookup', label: 'Owner', + * referenceTo: 'user' } } }) + * => success = false + * => unrecognized_keys at ["fields","rel"] keys=["referenceTo"] + * "Did you mean `referenceTo` -> `reference`?" + * + * which the route returns as a hard 422 `INVALID_METADATA`. Because the key is + * then STORED, every later save of that object fails the same way until it is + * cleared by hand. + * + * ## Why the negative controls are the deliverable + * + * A green parity assertion proves nothing on its own: `FieldSchema` could be + * resolved to a look-alike or loosened to a passthrough and every positive + * assertion here would stay green while the 422 still happened server-side. So + * the instrument is asserted first, and each positive claim is paired with a + * control that must fail. + * + * Assertions are made on the bytes the SDK actually PUT — `JSON.parse` of the + * captured request body — not on the object handed to the client. That + * distinction is load-bearing for this key: a property whose value is + * `undefined` is a key that zod's strict object COUNTS but that + * `JSON.stringify` DROPS, so an in-memory assertion and a wire assertion + * disagree exactly on the half-filled draft this card had to measure. + */ + +import { describe, expect, it, vi } from 'vitest'; +import { FieldSchema } from '@objectstack/spec/data'; +import { ObjectStackAdapter } from '@object-ui/data-objectstack'; +import type { DesignerFieldDefinition } from '@object-ui/types'; +import { MetadataService } from './MetadataService'; + +/** The bodies of every PUT the SDK issued, exactly as they went over the wire. */ +function makeCapturingAdapter() { + const puts: Array> = []; + const adapter = new ObjectStackAdapter({ + baseUrl: 'http://test.local', + fetch: vi.fn(async (_input: RequestInfo | URL, init?: RequestInit) => { + if ((init?.method ?? 'GET').toUpperCase() === 'PUT') { + puts.push(JSON.parse(String(init?.body ?? '{}')) as Record); + } + return new Response(JSON.stringify({ success: true }), { + status: 200, + headers: { 'content-type': 'application/json' }, + }); + }) as unknown as typeof fetch, + }); + return { adapter, puts }; +} + +/** The field defs of the last PUT, keyed by field name. */ +function savedFields(puts: Array>): Record[] { + const last = puts[puts.length - 1]; + return last.fields as Record[]; +} + +const unrecognizedKeys = (result: ReturnType): string[] => + result.success + ? [] + : result.error.issues + .filter((i) => i.code === 'unrecognized_keys') + .flatMap((i) => (i as unknown as { keys: string[] }).keys); + +const LOOKUP: DesignerFieldDefinition = { + id: 'owner_id', + name: 'owner_id', + label: 'Owner', + type: 'lookup', + referenceTo: 'account', +}; + +describe('the instrument', () => { + it('is the installed spec schema and it is STRICT — unknown keys are refused, not stripped', () => { + // objectstack#4001 closed the silent-drop shape. Every parity assertion + // below depends on it: a stripping schema would make them all trivially + // green while the 422 still happened server-side. + const result = FieldSchema.safeParse({ type: 'text', label: 'L', zzzDefinitelyNotAKey: 1 }); + expect(result.success).toBe(false); + expect(unrecognizedKeys(result)).toContain('zzzDefinitelyNotAKey'); + }); + + it('refuses `referenceTo` by name and accepts `reference` — the two states this file distinguishes', () => { + expect(unrecognizedKeys(FieldSchema.safeParse({ type: 'lookup', label: 'Owner', referenceTo: 'account' }))) + .toEqual(['referenceTo']); + expect(FieldSchema.safeParse({ type: 'lookup', label: 'Owner', reference: 'account' }).success).toBe(true); + }); +}); + +describe('objectui#6041 · saveFields PUTs the relationship target as `reference`', () => { + it('carries `reference` and no `referenceTo` on the wire', async () => { + const { adapter, puts } = makeCapturingAdapter(); + + await new MetadataService(adapter).saveFields('account', [LOOKUP]); + + const [def] = savedFields(puts); + expect(def.reference).toBe('account'); + expect('referenceTo' in def).toBe(false); + }); + + it('the PUT body parses through the real FieldSchema', async () => { + const { adapter, puts } = makeCapturingAdapter(); + + await new MetadataService(adapter).saveFields('account', [LOOKUP]); + + const [def] = savedFields(puts); + const result = FieldSchema.safeParse(def); + expect(unrecognizedKeys(result)).toEqual([]); + expect(result.success).toBe(true); + // Falsification: the target actually made the trip. A payload that simply + // dropped the key would also parse green, and that is not the fix. + expect(def.reference).toBe('account'); + }); + + it('a HALF-FILLED draft — type `lookup`, target left empty — still saves, exactly as before', async () => { + // The behavioural edge this card had to measure. The spec's prose calls + // `reference` "Required for relationship types", but that requirement is + // NOT enforced by the zod parse at 17.2.0: `{ type: 'lookup', label: 'L' }` + // parses green at field level AND through `ObjectSchema`. `undefined` is + // dropped by `JSON.stringify` under either spelling, so the wire bytes are + // byte-identical before and after this fix. + // + // ⚠ This case would still pass on a revert, and says so deliberately: it + // is here to prove the rename did NOT newly block a draft, which is a + // claim about the unchanged half. + const { adapter, puts } = makeCapturingAdapter(); + + await new MetadataService(adapter).saveFields('account', [{ ...LOOKUP, referenceTo: undefined }]); + + const [def] = savedFields(puts); + expect('reference' in def).toBe(false); + expect('referenceTo' in def).toBe(false); + expect(FieldSchema.safeParse(def).success).toBe(true); + }); +}); diff --git a/packages/app-shell/src/services/MetadataService.ts b/packages/app-shell/src/services/MetadataService.ts index 457da98f0d..03c1e93f31 100644 --- a/packages/app-shell/src/services/MetadataService.ts +++ b/packages/app-shell/src/services/MetadataService.ts @@ -62,7 +62,12 @@ export interface FieldMetadataPayload { // `FieldSchema.safeParse` rejects the key by name, so writing it made // `PUT /api/v1/meta/object/:name` fail with 422 `INVALID_METADATA`. // Object-level `indexes[]` is the real surface. - referenceTo?: string; + // No `referenceTo` (objectui#6041): the spec spells the relationship + // target `reference`. `FieldSchema.safeParse` refuses `referenceTo` BY NAME + // ("Did you mean `referenceTo` -> `reference`?"), so a lookup field authored + // in the designer made `PUT /api/v1/meta/object/:name` fail 422 + // `INVALID_METADATA` and blocked every later save of that object. + reference?: string; formula?: string; sortOrder?: number; } @@ -103,7 +108,7 @@ function toFieldPayload(field: DesignerFieldDefinition): FieldMetadataPayload { options: field.options, externalId: field.externalId, trackHistory: field.trackHistory, - referenceTo: field.referenceTo, + reference: field.referenceTo, formula: field.formula, sortOrder: field.sortOrder, }; diff --git a/packages/plugin-designer/src/MetadataFieldsPage.specKeyReference.test.tsx b/packages/plugin-designer/src/MetadataFieldsPage.specKeyReference.test.tsx new file mode 100644 index 0000000000..b6ff8f10bc --- /dev/null +++ b/packages/plugin-designer/src/MetadataFieldsPage.specKeyReference.test.tsx @@ -0,0 +1,284 @@ +/** + * 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#6041 — the Field Designer reads and writes the relationship target + * under the spec's spelling `reference`, in BOTH directions. + * + * Surfaced by the key-level parity gate built for objectui#5761 + * (`scripts/check-designer-field-key-parity.mjs`). `ServerFieldSchema` is one + * of that gate's two `wire` shapes: `fromDesignerField` builds it and this page + * PUTs the assembled `fields` map. + * + * `referenceTo` is not in `FieldSchema`'s accept set — measured against the + * installed `@objectstack/spec` 17.2.0, through the whole object document: + * + * ObjectSchema.safeParse({ …, fields: { rel: { type: 'lookup', label: 'Owner', + * referenceTo: 'user' } } }) + * => success = false + * => unrecognized_keys at ["fields","rel"] keys=["referenceTo"] + * + * i.e. a hard 422 `INVALID_METADATA` that blocks every later save of the object. + * + * Two directions, both broken by one misspelling and both pinned here: + * + * WRITE — `fromDesignerField` emitted `referenceTo`, so authoring a lookup + * field produced the 422. + * READ — `toDesignerField` read `raw.referenceTo` from the server payload. + * A spec-parsed server sends `reference`, so every EXISTING lookup + * field loaded into the designer with an empty reference box. + * + * Fixing only the write side would leave every already-saved field unreadable, + * so the read case below is not a bonus assertion — it is half the card. + * + * Written against the wire like its siblings `MetadataFieldsPage.saveEnvelope` + * and `MetadataFieldsPage.retiredIndexed`: a REAL `MetadataClient` over a fetch + * double, assertions on the captured PUT bytes rather than on the argument + * handed to the client. That distinction matters for this key — a property + * whose value is `undefined` is a key zod's strict object COUNTS but + * `JSON.stringify` DROPS. + * + * This file names no `isSystem`/`system` key anywhere, and its sibling + * `MetadataFieldsPage.specKeySystem.test.tsx` names no reference key: the two + * cards of this fold are independently verifiable, and reverting one fix must + * red only its own file. + */ + +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { act, render, waitFor } from '@testing-library/react'; +import { FieldSchema } from '@objectstack/spec/data'; +import { MetadataClient } from '@object-ui/data-objectstack'; +import type { DesignerFieldDefinition } from '@object-ui/types'; + +/** + * The object document as it lives in the database. + * + * `owner_id` carries what a SPEC-PARSED SERVER sends — `reference`. That is + * the read case: before this fix the designer looked for `referenceTo` and + * found nothing. + * `legacy_id` carries the misspelling a pre-fix designer build could have + * left behind. `carryOver` spreads the previous server def verbatim, so + * without a tombstone the key rides straight back out to the route that + * rejects it — and the object stays blocked forever. + */ +const OBJECT_BODY = { + name: 'probe_widget', + label: 'Widget', + fields: { + name: { type: 'text', label: 'Name', required: true }, + owner_id: { type: 'lookup', label: 'Owner', reference: 'account', inlineHelpText: 'Record owner.' }, + legacy_id: { type: 'lookup', label: 'Legacy', referenceTo: 'contact' }, + }, +}; + +const OBJECT_ENVELOPE = { + type: 'object', + name: 'probe_widget', + item: OBJECT_BODY, + lock: 'none', + provenance: 'org', + editable: true, +}; + +interface RecordedDesignerProps { + objectName: string; + fields: DesignerFieldDefinition[]; + onFieldsChange?: (fields: DesignerFieldDefinition[]) => void; + readOnly?: boolean; +} + +let designerProps: RecordedDesignerProps | null = null; + +vi.mock('./FieldDesigner', () => ({ + FieldDesigner: (props: RecordedDesignerProps) => { + designerProps = props; + return null; + }, +})); + +import { MetadataFieldsPage } from './MetadataFieldsPage'; + +let puts: Array> = []; + +function json(body: unknown, status = 200): Response { + return new Response(JSON.stringify(body), { + status, + headers: { 'content-type': 'application/json' }, + }); +} + +function realClient(): MetadataClient { + return new MetadataClient({ + baseUrl: 'http://localhost:3000', + fetch: (async (input: RequestInfo | URL, init?: RequestInit) => { + const url = String(input); + const method = (init?.method ?? 'GET').toUpperCase(); + if (method === 'PUT') { + puts.push(JSON.parse(String(init?.body ?? '{}')) as Record); + return json({ success: true, name: 'probe_widget' }); + } + if (/\/meta\/object\/probe_widget(\?|$)/.test(url)) return json(OBJECT_ENVELOPE); + return json({ items: [] }); + }) as unknown as typeof fetch, + }); +} + +async function renderPage() { + render(); + await waitFor(() => expect(designerProps).not.toBeNull()); +} + +/** The body of the last PUT, exactly as it went over the wire. */ +function lastPut(): Record { + // Indexed rather than `.at(-1)`: this package's tsconfig `lib` predates + // ES2022, so `Array.prototype.at` does not type-check here. + return puts[puts.length - 1]; +} + +/** The fields map exactly as it went over the wire on the last PUT. */ +function savedFields(): Record> { + return lastPut().fields as Record>; +} + +const unrecognizedKeys = (result: ReturnType): string[] => + result.success + ? [] + : result.error.issues + .filter((i) => i.code === 'unrecognized_keys') + .flatMap((i) => (i as unknown as { keys: string[] }).keys); + +/** A plain relabel — the smallest edit that re-serialises every field. */ +async function relabel(fieldName: string, label: string) { + const next = designerProps!.fields.map((f) => (f.name === fieldName ? { ...f, label } : f)); + await act(async () => { + designerProps!.onFieldsChange!(next); + }); + await waitFor(() => expect(puts).toHaveLength(1)); +} + +beforeEach(() => { + puts = []; + designerProps = null; +}); + +afterEach(() => { + designerProps = null; +}); + +describe('the instrument', () => { + it('is the installed spec schema and it is STRICT — unknown keys are refused, not stripped', () => { + // objectstack#4001 closed the silent-drop shape. Without it every parity + // assertion here would be trivially green while the 422 still happened. + const result = FieldSchema.safeParse({ type: 'text', label: 'L', zzzDefinitelyNotAKey: 1 }); + expect(result.success).toBe(false); + expect(unrecognizedKeys(result)).toContain('zzzDefinitelyNotAKey'); + }); + + it('refuses `referenceTo` by name and accepts `reference` — the two states this file distinguishes', () => { + expect(unrecognizedKeys(FieldSchema.safeParse({ type: 'lookup', label: 'Owner', referenceTo: 'account' }))) + .toEqual(['referenceTo']); + expect(FieldSchema.safeParse({ type: 'lookup', label: 'Owner', reference: 'account' }).success).toBe(true); + }); +}); + +describe('objectui#6041 · READ — an existing lookup field loads with its target', () => { + it('hands the stored `reference` down to the designer', async () => { + await renderPage(); + const owner = designerProps!.fields.find((f) => f.name === 'owner_id')!; + // Before this fix the reader looked for `raw.referenceTo`, so this was + // `undefined` and the reference box rendered EMPTY for every saved field. + expect(owner.referenceTo).toBe('account'); + // Falsification: the field itself arrived, with its other keys intact. + expect(owner.label).toBe('Owner'); + expect(owner.type).toBe('lookup'); + }); +}); + +describe('objectui#6041 · WRITE — the save carries `reference`, never `referenceTo`', () => { + it('PUTs the target under the spec spelling', async () => { + await renderPage(); + await relabel('owner_id', 'Record owner'); + + const fields = savedFields(); + expect(fields.owner_id.reference).toBe('account'); + expect('referenceTo' in fields.owner_id).toBe(false); + // Falsification, twice: the edit landed, and the unknown per-field key the + // designer does not RENDER (but the spec accepts) survived the round-trip. + expect(fields.owner_id.label).toBe('Record owner'); + expect(fields.owner_id.inlineHelpText).toBe('Record owner.'); + }); + + it('every field it PUTs parses through the real FieldSchema', async () => { + await renderPage(); + await relabel('owner_id', 'Record owner'); + + for (const [name, def] of Object.entries(savedFields())) { + const result = FieldSchema.safeParse(def); + expect(unrecognizedKeys(result), `field \`${name}\` emitted a refused key`).toEqual([]); + expect(result.success, `field \`${name}\` did not parse`).toBe(true); + } + }); + + it('a newly authored lookup field emits `reference`', async () => { + await renderPage(); + const next: DesignerFieldDefinition[] = [ + ...designerProps!.fields, + { id: 'fld_new', name: 'billing_id', label: 'Billing', type: 'lookup', referenceTo: 'invoice' }, + ]; + await act(async () => { + designerProps!.onFieldsChange!(next); + }); + await waitFor(() => expect(puts).toHaveLength(1)); + + const fields = savedFields(); + expect(fields.billing_id.reference).toBe('invoice'); + expect('referenceTo' in fields.billing_id).toBe(false); + expect(FieldSchema.safeParse(fields.billing_id).success).toBe(true); + }); + + it('a save of an object ALREADY carrying `referenceTo` puts it back without the key', async () => { + // Without the tombstone this is the case that keeps a blocked object + // blocked: renaming the emit site does not touch what `carryOver` spreads, + // so the stored misspelling would ride back out to the same 422. + await renderPage(); + await relabel('owner_id', 'Record owner'); + + const fields = savedFields(); + expect('referenceTo' in fields.legacy_id).toBe(false); + // Falsification: the field is still there and still a lookup — the strip + // removed a key, not the field. + expect(fields.legacy_id.type).toBe('lookup'); + expect(FieldSchema.safeParse(fields.legacy_id).success).toBe(true); + }); + + it('a HALF-FILLED draft — type `lookup`, target left empty — still saves, exactly as before', async () => { + // The behavioural edge this card had to measure. The spec's prose calls + // `reference` "Required for relationship types", but that requirement is + // NOT enforced by the zod parse at 17.2.0: `{ type: 'lookup', label: 'L' }` + // parses green at field level AND through `ObjectSchema`. `undefined` is + // dropped by `JSON.stringify` under either spelling, so the wire bytes are + // byte-identical before and after this fix. + // + // ⚠ This case would still pass on a revert, and says so deliberately: it + // exists to prove the rename did NOT newly block a draft. + await renderPage(); + const next: DesignerFieldDefinition[] = [ + ...designerProps!.fields, + { id: 'fld_half', name: 'half_id', label: 'Half', type: 'lookup' }, + ]; + await act(async () => { + designerProps!.onFieldsChange!(next); + }); + await waitFor(() => expect(puts).toHaveLength(1)); + + const fields = savedFields(); + expect('reference' in fields.half_id).toBe(false); + expect('referenceTo' in fields.half_id).toBe(false); + expect(FieldSchema.safeParse(fields.half_id).success).toBe(true); + }); +}); diff --git a/packages/plugin-designer/src/MetadataFieldsPage.specKeySystem.test.tsx b/packages/plugin-designer/src/MetadataFieldsPage.specKeySystem.test.tsx new file mode 100644 index 0000000000..092a15cc68 --- /dev/null +++ b/packages/plugin-designer/src/MetadataFieldsPage.specKeySystem.test.tsx @@ -0,0 +1,241 @@ +/** + * 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#6044 — the Field Designer reads the system-field marker under the + * spec's spelling `system`, and never hands `isSystem` back to the API. + * + * Surfaced by the key-level parity gate built for objectui#5761 + * (`scripts/check-designer-field-key-parity.mjs`). `isSystem` is not in + * `FieldSchema`'s accept set; the spec spells it `system`. Measured against the + * installed `@objectstack/spec` 17.2.0: + * + * FieldSchema.safeParse({ type: 'text', label: 'L', isSystem: true }) + * => success = false + * => unrecognized_keys keys=["isSystem"] + * "Did you mean `isSystem` -> `system`?" + * + * ## Two defects, one misspelling — and they are two DIFFERENT sites + * + * READ (the quieter, worse half). `toDesignerField` read `raw.isSystem`. `raw` + * is what the server sent and a spec-parsed server sends `system`, so the flag + * was always `undefined`. Nothing went red, because the flag is OPTIONAL — + * `undefined` is a valid "not a system field". But it is load-bearing in the + * UI: `FieldDesigner` refuses to delete a system field and disables its name + * and type inputs, so with the read dead `organization_id`, `created_at` and + * friends presented as ordinary editable, DELETABLE business fields. + * + * WRITE. This one has no emit site at all — `fromDesignerField` never names + * `isSystem`. Its only route out is the verbatim `...carryOver(prev)` spread, + * so the fix is a `RETIRED_FIELD_KEYS` tombstone rather than a renamed line. + * That answers the question the card left open: the round-trip and the + * detection read are SEPARATE sites, and neither of them is in + * `app-shell/services/MetadataService.ts` — `FieldMetadataPayload` never + * declared the key, so `toFieldPayload` has nothing to fix. + * + * ⛔ Not resolved by the tombstone alone. Stripping `isSystem` without fixing + * the read would close the 422 and FOSSILIZE the dead detection, which is the + * scope constraint this card carries. + * + * Written against the wire like its siblings: a REAL `MetadataClient` over a + * fetch double, assertions on the captured PUT bytes. + * + * This file names no `reference`/`referenceTo` key anywhere, and its sibling + * `MetadataFieldsPage.specKeyReference.test.tsx` names no system key: the two + * cards of this fold are independently verifiable, and reverting one fix must + * red only its own file. + */ + +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { act, render, waitFor } from '@testing-library/react'; +import { FieldSchema } from '@objectstack/spec/data'; +import { MetadataClient } from '@object-ui/data-objectstack'; + +/** + * The object document as it lives in the database. + * + * `organization_id` carries what a SPEC-PARSED SERVER sends — `system`. That + * is the read case. + * `legacy_flag` carries the misspelling, which `carryOver` would otherwise + * spread straight back out to the route that rejects it. + * `nickname` is an ordinary business field, and it is the control: a harness + * whose two principals degenerate to the same value cannot tell a working + * read from a dead one, so the suite asserts BOTH states. + */ +const OBJECT_BODY = { + name: 'probe_widget', + label: 'Widget', + fields: { + nickname: { type: 'text', label: 'Nickname' }, + organization_id: { type: 'text', label: 'Organization', system: true, readonly: true }, + legacy_flag: { type: 'text', label: 'Legacy', isSystem: true }, + }, +}; + +const OBJECT_ENVELOPE = { + type: 'object', + name: 'probe_widget', + item: OBJECT_BODY, + lock: 'none', + provenance: 'org', + editable: true, +}; + +interface RecordedDesignerProps { + objectName: string; + fields: Array<{ name: string; label: string; isSystem?: boolean }>; + onFieldsChange?: (fields: RecordedDesignerProps['fields']) => void; + readOnly?: boolean; +} + +let designerProps: RecordedDesignerProps | null = null; + +vi.mock('./FieldDesigner', () => ({ + FieldDesigner: (props: RecordedDesignerProps) => { + designerProps = props; + return null; + }, +})); + +import { MetadataFieldsPage } from './MetadataFieldsPage'; + +let puts: Array> = []; + +function json(body: unknown, status = 200): Response { + return new Response(JSON.stringify(body), { + status, + headers: { 'content-type': 'application/json' }, + }); +} + +function realClient(): MetadataClient { + return new MetadataClient({ + baseUrl: 'http://localhost:3000', + fetch: (async (input: RequestInfo | URL, init?: RequestInit) => { + const url = String(input); + const method = (init?.method ?? 'GET').toUpperCase(); + if (method === 'PUT') { + puts.push(JSON.parse(String(init?.body ?? '{}')) as Record); + return json({ success: true, name: 'probe_widget' }); + } + if (/\/meta\/object\/probe_widget(\?|$)/.test(url)) return json(OBJECT_ENVELOPE); + return json({ items: [] }); + }) as unknown as typeof fetch, + }); +} + +async function renderPage() { + render(); + await waitFor(() => expect(designerProps).not.toBeNull()); +} + +/** The fields map exactly as it went over the wire on the last PUT. */ +function savedFields(): Record> { + // Indexed rather than `.at(-1)`: this package's tsconfig `lib` predates + // ES2022, so `Array.prototype.at` does not type-check here. + return puts[puts.length - 1].fields as Record>; +} + +const unrecognizedKeys = (result: ReturnType): string[] => + result.success + ? [] + : result.error.issues + .filter((i) => i.code === 'unrecognized_keys') + .flatMap((i) => (i as unknown as { keys: string[] }).keys); + +/** A plain relabel — the smallest edit that re-serialises every field. */ +async function relabel(fieldName: string, label: string) { + const next = designerProps!.fields.map((f) => (f.name === fieldName ? { ...f, label } : f)); + await act(async () => { + designerProps!.onFieldsChange!(next); + }); + await waitFor(() => expect(puts).toHaveLength(1)); +} + +beforeEach(() => { + puts = []; + designerProps = null; +}); + +afterEach(() => { + designerProps = null; +}); + +describe('the instrument', () => { + it('is the installed spec schema and it is STRICT — unknown keys are refused, not stripped', () => { + const result = FieldSchema.safeParse({ type: 'text', label: 'L', zzzDefinitelyNotAKey: 1 }); + expect(result.success).toBe(false); + expect(unrecognizedKeys(result)).toContain('zzzDefinitelyNotAKey'); + }); + + it('refuses `isSystem` by name and accepts `system` — the two states this file distinguishes', () => { + expect(unrecognizedKeys(FieldSchema.safeParse({ type: 'text', label: 'L', isSystem: true }))) + .toEqual(['isSystem']); + expect(FieldSchema.safeParse({ type: 'text', label: 'L', system: true }).success).toBe(true); + }); +}); + +describe('objectui#6044 · READ — system-field detection sees the key the server sends', () => { + it('a field the server marks `system` reaches the designer as a system field', async () => { + await renderPage(); + const org = designerProps!.fields.find((f) => f.name === 'organization_id')!; + // Before this fix the reader looked for `raw.isSystem`, so this was + // `undefined` — and `undefined` is a VALID "not a system field", which is + // why nothing ever went red while `organization_id` rendered as an + // ordinary editable, deletable business field. + expect(org.isSystem).toBe(true); + // Falsification: the field itself arrived intact. + expect(org.label).toBe('Organization'); + }); + + it('an ordinary business field is NOT flagged — the other state of the world', async () => { + await renderPage(); + const nickname = designerProps!.fields.find((f) => f.name === 'nickname')!; + expect(nickname.isSystem).toBeFalsy(); + }); +}); + +describe('objectui#6044 · WRITE — the save never hands `isSystem` back', () => { + it('strips a stored `isSystem` from the round-trip', async () => { + // `fromDesignerField` never names this key: its only route out is the + // verbatim `carryOver` spread, so this asserts the tombstone, not an emit + // site. + await renderPage(); + await relabel('nickname', 'Nick'); + + const fields = savedFields(); + expect('isSystem' in fields.legacy_flag).toBe(false); + // Falsification: the strip removed a key, not the field. + expect(fields.legacy_flag.type).toBe('text'); + expect(fields.legacy_flag.label).toBe('Legacy'); + }); + + it('keeps the spec-spelled `system` the server injected', async () => { + // ⚠ This case would still pass on a revert of either half: `system` is a + // real `FieldSchema` key and `carryOver` has always spread it through. It + // is asserted because the tombstone must not over-reach — stripping the + // spec spelling too would make the read it feeds permanently dead. + await renderPage(); + await relabel('nickname', 'Nick'); + + expect(savedFields().organization_id.system).toBe(true); + }); + + it('every field it PUTs parses through the real FieldSchema', async () => { + await renderPage(); + await relabel('nickname', 'Nick'); + + for (const [name, def] of Object.entries(savedFields())) { + const result = FieldSchema.safeParse(def); + expect(unrecognizedKeys(result), `field \`${name}\` emitted a refused key`).toEqual([]); + expect(result.success, `field \`${name}\` did not parse`).toBe(true); + } + // Falsification: the edit that triggered the save actually landed. + expect(savedFields().nickname.label).toBe('Nick'); + }); +}); diff --git a/packages/plugin-designer/src/MetadataFieldsPage.tsx b/packages/plugin-designer/src/MetadataFieldsPage.tsx index 78c0b16d2e..bacfbec4df 100644 --- a/packages/plugin-designer/src/MetadataFieldsPage.tsx +++ b/packages/plugin-designer/src/MetadataFieldsPage.tsx @@ -51,13 +51,26 @@ interface ServerFieldSchema { group?: string; externalId?: boolean; trackHistory?: boolean; - referenceTo?: string; + /** + * Relationship target object name. The spec spells it `reference` + * (objectui#6041) — `referenceTo` is refused BY NAME by `FieldSchema`, so + * emitting it made `PUT /api/v1/meta/object/:name` fail 422 and blocked + * every later save of the object. See {@link RETIRED_FIELD_KEYS}. + */ + reference?: string; formula?: string; // The framework also stores `select` field options as `options: string[] | // {label, value}[]`; we passthrough the raw structure for now. options?: unknown; - // Marker used by the framework's system-field injection (organization_id). - isSystem?: boolean; + /** + * Marker set by the framework's system-field injection (`organization_id`, + * `created_at`, `updated_by`, …). The spec spells it `system` + * (objectui#6044); `isSystem` is refused BY NAME by `FieldSchema`, and — being + * an OPTIONAL flag — reading the wrong spelling went unnoticed: `undefined` + * is a valid "not a system field", so system fields presented as ordinary + * editable, deletable business fields. + */ + system?: boolean; [key: string]: unknown; } @@ -92,10 +105,10 @@ function toDesignerField(name: string, raw: ServerFieldSchema): DesignerFieldDef hidden: raw.hidden, defaultValue: raw.defaultValue, placeholder: raw.placeholder, - isSystem: raw.isSystem, + isSystem: raw.system, externalId: raw.externalId, trackHistory: raw.trackHistory, - referenceTo: raw.referenceTo, + referenceTo: raw.reference, formula: raw.formula, }; } @@ -117,7 +130,23 @@ function toDesignerField(name: string, raw: ServerFieldSchema): DesignerFieldDef * parseable; it is keyed to the tombstone, so every other unknown key the * designer does not render still survives. */ -const RETIRED_FIELD_KEYS = ['indexed'] as const; +/* + * objectui#6041 adds `referenceTo`. Renaming the emit site alone does not + * unblock an object a previous designer build already saved: that stored + * payload still carries `referenceTo`, `carryOver` spreads `prev` verbatim, + * and the key would round-trip straight back out to the same 422. Stripping it + * on the way out is what makes an edit-and-save of an ALREADY-BLOCKED object + * come out parseable. The target itself is not lost — `fromDesignerField` + * re-emits it under the spec spelling `reference` on the very next line. + * + * objectui#6044 adds `isSystem` for the same reason and with one difference + * worth stating: `fromDesignerField` never NAMES it, so the only way out is the + * verbatim `carryOver` spread — which makes this line, not any emit site, the + * whole write half of that card. The spec spelling `system` is not stripped: it + * is a real `FieldSchema` key, so a server-injected flag rides through + * untouched, which is exactly what lets `toDesignerField` read it back. + */ +const RETIRED_FIELD_KEYS = ['indexed', 'referenceTo', 'isSystem'] as const; /** Carry over `prev`'s unknown keys, minus {@link RETIRED_FIELD_KEYS}. */ function carryOver(prev?: ServerFieldSchema): ServerFieldSchema { @@ -145,7 +174,7 @@ function fromDesignerField( group: designed.group, externalId: designed.externalId, trackHistory: designed.trackHistory, - referenceTo: designed.referenceTo, + reference: designed.referenceTo, formula: designed.formula, }; } diff --git a/scripts/check-designer-field-key-parity.mjs b/scripts/check-designer-field-key-parity.mjs index 254a987a2d..bb23ee72aa 100644 --- a/scripts/check-designer-field-key-parity.mjs +++ b/scripts/check-designer-field-key-parity.mjs @@ -205,21 +205,11 @@ export const PAYLOAD_SHAPES = [ * answer even when a near-spelling exists. */ export const KNOWN_UNPARSEABLE_KEYS = { - referenceTo: { - card: "objectui#6041", - spec: "reference", - note: "LIVE. FieldDesigner renders a control for it on `type == 'lookup'` and both write paths populate it, so authoring a lookup field produces the 422.", - }, formula: { card: "objectui#6043", spec: "expression (+ returnType)", note: "LIVE. FieldDesigner renders a textarea for it on `type == 'formula'`. Not a rename: the spec's `expression` is CEL, so the key and the expression LANGUAGE move together.", }, - isSystem: { - card: "objectui#6044", - spec: "system", - note: "Two defects. The READ (`toDesignerField` reads `raw.isSystem`) never matches what a spec-parsed server sends, so system fields present as ordinary editable ones; the WRITE can round-trip out through `carryOver`'s verbatim spread.", - }, sortOrder: { card: "objectui#6045", spec: null, // the spec has `sortable` (a boolean), and no field-level ordering key