From 5465cfffd57e3e67ffda3075559ff5adafaa725a Mon Sep 17 00:00:00 2001 From: os-warren Date: Thu, 10 Sep 2026 00:08:28 +0000 Subject: [PATCH 1/2] fix(designer): keep a relationship target stored only as `referenceTo` (objectui#8896) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two field-IO sites dropped the retired `referenceTo` key without keeping its value, so a lookup or master-detail field whose target survived only under the pre-objectui#6041 spelling lost the target on the way through. `MetadataFieldsPage`'s carried-through half (objectui#8060) re-emits a stored field verbatim with no read door in front of it, so `carryOver`'s strip was the last thing to touch the field: the target left with the key and `assertRelationshipTargetPresent` then refused the WHOLE save — a save the author triggered by relabelling an unrelated field, on a page that renders the offending field read-only, so "Pick the target object" named a control that does not exist there. metadata-admin's `readFields`, the single read door for `draft.fields`, deleted the same value on load and committed the loss on the next save. Both sites now lift the value onto the spec spelling before dropping the retired key, which is the shape the designable half has always used. The strip itself is unchanged: `FieldSchema` refuses `referenceTo` by name and no site emits it. The recovery is written per site and keyed to this one key — NOT driven off the registry's `specEquivalent`, which objectui#6043 refused as a mechanism — and shares `assertRelationshipTargetPresent`'s own predicate, so it can never hand the guard a target the guard would refuse. The tombstone registry's unconditional "The strip loses nothing" and the read door's "Nothing is lost on the way out" are corrected to say where they hold. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Jmxdo7bmeqCQHLSfmLVX9w --- .changeset/referenceto-carryover-8896.md | 26 ++ ...-fields-io.referenceCarryover-8896.test.ts | 165 ++++++++++++ .../object-fields-io.retiredKeys.test.ts | 38 ++- .../previews/object-fields-io.ts | 73 ++++- ...Page.carriedThroughReference-8896.test.tsx | 251 ++++++++++++++++++ .../src/MetadataFieldsPage.tsx | 90 ++++++- .../types/src/internal/retired-field-keys.ts | 25 +- 7 files changed, 654 insertions(+), 14 deletions(-) create mode 100644 .changeset/referenceto-carryover-8896.md create mode 100644 packages/app-shell/src/views/metadata-admin/previews/object-fields-io.referenceCarryover-8896.test.ts create mode 100644 packages/plugin-designer/src/MetadataFieldsPage.carriedThroughReference-8896.test.tsx diff --git a/.changeset/referenceto-carryover-8896.md b/.changeset/referenceto-carryover-8896.md new file mode 100644 index 0000000000..9d38a7b931 --- /dev/null +++ b/.changeset/referenceto-carryover-8896.md @@ -0,0 +1,26 @@ +--- +"@object-ui/plugin-designer": patch +"@object-ui/app-shell": patch +"@object-ui/types": patch +--- + +Keep a relationship target stored only as the retired `referenceTo` spelling (objectui#8896) + +Two field-IO sites deleted the retired `referenceTo` key without keeping its +value, so an object whose lookup or master-detail target survived only under the +pre-objectui#6041 spelling lost the target on the way through: + +- The Field Designer's carried-through half (fields whose stored type the + designer cannot author, objectui#8060) re-emits the stored document verbatim + with no read door in front of it. The strip took the target and the relationship + guard then refused the whole object's save — including a save the author + triggered by editing an entirely different field, on a page that renders the + offending field read-only, so its "Pick the target object" advice named a + control that does not exist there. +- The object designer's single read door for `draft.fields` deleted the target on + load, leaving the target editor empty and committing the loss on the next save. + +Both sites now lift the value onto the spec spelling `reference` before dropping +the retired key. The retired key still never reaches the wire, a live `reference` +is never overwritten by a stale legacy value, and a field with no usable target +under either spelling is still refused. diff --git a/packages/app-shell/src/views/metadata-admin/previews/object-fields-io.referenceCarryover-8896.test.ts b/packages/app-shell/src/views/metadata-admin/previews/object-fields-io.referenceCarryover-8896.test.ts new file mode 100644 index 0000000000..dc31b2b953 --- /dev/null +++ b/packages/app-shell/src/views/metadata-admin/previews/object-fields-io.referenceCarryover-8896.test.ts @@ -0,0 +1,165 @@ +// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * objectui#8896 — the metadata-admin read door loses a relationship target + * stored only under the retired `referenceTo` spelling. + * + * ## The claim this file falsifies, and where it is written + * + * `RETIRED_FIELD_KEYS`' own docblock in `object-fields-io.ts`: + * + * > Nothing is lost on the way out: where the spec has a spelling for the + * > concept it is a SEPARATE key (`reference`, `system`) that is NOT stripped + * > and rides through untouched, which is what lets the designer read it back. + * + * and the tombstone registry's entry for the key + * (`types/src/internal/retired-field-keys.ts`): + * + * > The strip loses nothing — every write path re-emits the designer's target + * > under `reference`, and the read door's writers never emit the retired + * > spelling. + * + * Both hold only when the draft ALSO carries the spec spelling. A draft holding + * the target ONLY as `referenceTo` has it deleted on load with nothing left + * behind: `readFields` is the single read door for `draft.fields` across the + * whole object designer, and `writeFields` writes each def back verbatim, so + * the loss is committed on the next save. `ObjectFieldInspector`'s target + * editor reads `def.reference` and renders EMPTY, and every reader downstream + * of this door reads the canonical key alone — `resolveReferenceTo` was + * deliberately reduced to `def.reference` by objectui#6837's ruling that + * protocol normalization belongs on the server and the readers just execute + * the protocol. + * + * ## What the repair is, and the two things it is NOT + * + * The retired KEY is still stripped — it must be: `FieldSchema` refuses + * `referenceTo` by name, so carrying it out is the hard 422 the strip exists + * to prevent. What changes is that the VALUE is recovered into the spec + * spelling first, which is the same "recover at the door, read canonically + * everywhere else" shape objectui#6837 settled on. + * + * ⛔ NOT a per-reader fallback arm. objectui#6837 deleted those on a maintainer + * ruling and this file does not bring one back — the pins below assert on what + * comes OUT of the door, and `resolveReferenceTo` keeps reading `reference` + * alone. + * + * ⛔ NOT a generic `specEquivalent` migration driven off the registry. The + * registry says so itself ("Documentation for the reader, NEVER an instruction + * to migrate a value mechanically"), because objectui#6043 refused exactly that + * for `formula`, whose value is a LANGUAGE and not a name. + */ + +import { describe, it, expect } from 'vitest'; +import { FieldSchema } from '@objectstack/spec/data'; +import { readFields, writeFields, RETIRED_FIELD_KEYS } from './object-fields-io'; +import { resolveReferenceTo } from '../inspectors/useDatasetFields'; + +/** Round-trip a `draft.fields` value the way every designer write path does. */ +function roundTrip(fields: unknown) { + return writeFields(readFields(fields)); +} + +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); + +describe('the instrument', () => { + it('the installed `FieldSchema` refuses `referenceTo` by name and accepts `reference`', () => { + // Without this the recovery below could be "fixed" by simply carrying the + // retired key through, which is the 422 this door exists to prevent. + expect(unrecognizedKeys(FieldSchema.safeParse({ type: 'lookup', label: 'L', referenceTo: 'account' }))) + .toContain('referenceTo'); + expect(FieldSchema.safeParse({ type: 'lookup', label: 'L', reference: 'account' }).success).toBe(true); + }); + + it('`referenceTo` is one of the keys this door strips', () => { + expect([...RETIRED_FIELD_KEYS]).toContain('referenceTo'); + }); +}); + +describe('objectui#8896 · a target stored only as `referenceTo` survives the read door', () => { + it('record-shaped draft: the value arrives under the spec spelling, the retired key does not', () => { + const out = roundTrip({ + owner_id: { type: 'lookup', label: 'Owner', referenceTo: 'account' }, + }) as Record>; + + expect(out.owner_id).toEqual({ type: 'lookup', label: 'Owner', reference: 'account' }); + expect('referenceTo' in out.owner_id).toBe(false); + }); + + it('array-shaped draft: the same, through the other branch of the door', () => { + const out = roundTrip([ + { name: 'owner_id', type: 'lookup', label: 'Owner', referenceTo: 'account' }, + ]) as Array>; + + expect(out[0]).toEqual({ name: 'owner_id', type: 'lookup', label: 'Owner', reference: 'account' }); + expect('referenceTo' in out[0]).toBe(false); + }); + + it('the def the designer reads carries the target, so the inspector renders it', () => { + // The consumer-side statement of the same fact: every reader downstream of + // this door reads `reference` alone, by objectui#6837's ruling. + const view = readFields({ owner_id: { type: 'master_detail', label: 'Parent', referenceTo: 'invoice' } }); + expect(view.entries[0].def.reference).toBe('invoice'); + expect(resolveReferenceTo(view.entries[0].def)).toBe('invoice'); + }); + + it('what comes out of the door parses through the real FieldSchema', () => { + const out = roundTrip({ + owner_id: { type: 'lookup', label: 'Owner', referenceTo: 'account' }, + }) as Record>; + + const result = FieldSchema.safeParse(out.owner_id); + expect(unrecognizedKeys(result)).toEqual([]); + expect(result.success).toBe(true); + }); +}); + +/** + * ⭐ The firing controls. Each one fails on a build that "fixed" this by + * recovering unconditionally, and the first two also fail on a build that + * simply stopped stripping the key. + */ +describe('objectui#8896 · firing controls', () => { + it('the spec spelling WINS — a stale legacy value never overwrites a live target', () => { + // A pre-objectui#6041 designer wrote both keys; the canonical one is the + // one the author has been editing ever since. + const out = roundTrip({ + owner_id: { type: 'lookup', label: 'Owner', reference: 'account', referenceTo: 'stale_legacy' }, + }) as Record>; + + expect(out.owner_id.reference).toBe('account'); + expect('referenceTo' in out.owner_id).toBe(false); + }); + + it('a field with NO usable target gains no invented one', () => { + // `unrecognized_keys` fires on the key's PRESENCE, so an empty retired key + // is real and stored. Recovering it would smuggle a target that names no + // object past every gate downstream. + const out = roundTrip({ + a: { type: 'lookup', label: 'A', referenceTo: '' }, + b: { type: 'lookup', label: 'B', referenceTo: ' ' }, + c: { type: 'text', label: 'C' }, + }) as Record>; + + expect('reference' in out.a).toBe(false); + expect('reference' in out.b).toBe(false); + expect('reference' in out.c).toBe(false); + expect(RETIRED_FIELD_KEYS.filter((k) => k in out.a)).toEqual([]); + expect(RETIRED_FIELD_KEYS.filter((k) => k in out.b)).toEqual([]); + }); + + it('the OTHER retired keys are still dropped with nothing left behind', () => { + // The recovery is keyed to ONE tombstone, not to `specEquivalent` in + // general: `isSystem` has a spec equivalent too and is deliberately not + // recovered — its strip IS the whole write half of objectui#6044. + const out = roundTrip({ + code: { type: 'text', label: 'Code', indexed: true, isSystem: true }, + }) as Record>; + + expect(out.code).toEqual({ type: 'text', label: 'Code' }); + }); +}); diff --git a/packages/app-shell/src/views/metadata-admin/previews/object-fields-io.retiredKeys.test.ts b/packages/app-shell/src/views/metadata-admin/previews/object-fields-io.retiredKeys.test.ts index a685914511..142b3dd04f 100644 --- a/packages/app-shell/src/views/metadata-admin/previews/object-fields-io.retiredKeys.test.ts +++ b/packages/app-shell/src/views/metadata-admin/previews/object-fields-io.retiredKeys.test.ts @@ -51,6 +51,32 @@ const SAMPLE: Record<(typeof RETIRED_FIELD_KEYS)[number], unknown> = { isSystem: true, }; +/** + * What is left on `{ type: 'lookup', label: 'Owner' }` after the door has seen + * that key — the falsification half of each case below, and the one place the + * three keys stop being interchangeable (objectui#8896). + * + * `indexed` and `isSystem` leave NOTHING behind, for two different recorded + * reasons: the field-level index flag built no index at all (objectui#4644 — + * the concept moved to the object's `indexes[]`, so there is no field-level + * value to keep), and the system flag's spec spelling `system` is a separate + * key the draft either carries or does not, so the strip IS the whole write + * half of objectui#6044 and re-stamping it would invent a flag the author + * never set. + * + * `referenceTo` is the one whose VALUE the draft may hold nowhere else. It is + * the same CONCEPT as `reference` and the same KIND of value — a bare object + * name — so deleting it destroyed the only copy of a relationship target and + * left the designer's target editor empty. The door keeps that value under the + * spec spelling now; the retired key is dropped exactly as the other two are, + * which is what every `key in out` assertion below is stating. + */ +const RESIDUE: Record<(typeof RETIRED_FIELD_KEYS)[number], Record> = { + indexed: {}, + referenceTo: { reference: 'account' }, + isSystem: {}, +}; + describe('object-fields-io · retired FieldSchema keys (objectui#4644, objectui#6519)', () => { it('names exactly the three keys this door strips', () => { // The list is derived from the tombstone registry (objectui#6527: @@ -77,8 +103,10 @@ describe('object-fields-io · retired FieldSchema keys (objectui#4644, objectui# }) as Record>; expect(key in out.owner_id).toBe(false); - // Falsification: keyed to the tombstone, not a blanket unknown-key purge. - expect(out.owner_id).toEqual({ type: 'lookup', label: 'Owner' }); + // Falsification: keyed to the tombstone, not a blanket unknown-key purge + // — and, for the one key whose value has nowhere else to live, the value + // survives under the spec spelling (see {@link RESIDUE}). + expect(out.owner_id).toEqual({ type: 'lookup', label: 'Owner', ...RESIDUE[key] }); }); it(`drops \`${key}\` from an array-shaped draft on round-trip`, () => { @@ -87,7 +115,7 @@ describe('object-fields-io · retired FieldSchema keys (objectui#4644, objectui# ]) as Array>; expect(key in out[0]).toBe(false); - expect(out[0]).toEqual({ name: 'owner_id', type: 'lookup', label: 'Owner' }); + expect(out[0]).toEqual({ name: 'owner_id', type: 'lookup', label: 'Owner', ...RESIDUE[key] }); }); } @@ -102,7 +130,9 @@ describe('object-fields-io · retired FieldSchema keys (objectui#4644, objectui# >; expect(RETIRED_FIELD_KEYS.filter((k) => k in out.owner_id)).toEqual([]); - expect(out.owner_id).toEqual({ type: 'lookup', label: 'Owner' }); + // Every retired key is gone; the lookup's target is not, because it is a + // VALUE and not a key — the asymmetry {@link RESIDUE} records. + expect(out.owner_id).toEqual({ type: 'lookup', label: 'Owner', reference: 'account' }); }); it('drops falsy values too — the key itself is what the parse rejects', () => { diff --git a/packages/app-shell/src/views/metadata-admin/previews/object-fields-io.ts b/packages/app-shell/src/views/metadata-admin/previews/object-fields-io.ts index 6fd3399e24..680d326598 100644 --- a/packages/app-shell/src/views/metadata-admin/previews/object-fields-io.ts +++ b/packages/app-shell/src/views/metadata-admin/previews/object-fields-io.ts @@ -52,6 +52,14 @@ export type Shape = 'array' | 'record'; * concept it is a SEPARATE key (`reference`, `system`) that is NOT stripped * and rides through untouched, which is what lets the designer read it back. * + * ⚠️ That sentence was UNCONDITIONAL and, for one key, measurably false + * (objectui#8896): it holds only where the draft ALSO carries the spec + * spelling. A draft holding a relationship target ONLY as `referenceTo` had it + * deleted here with nothing left behind. {@link stripRetiredFieldKeys} now + * keeps that one value under `reference` before dropping the key, so the + * sentence is true as written — see there for why the repair is keyed to this + * key alone and is NOT a `specEquivalent` migration. + * * ── The two registry keys this door deliberately does NOT strip ── * * `formula` — RULED, objectui#6526 option B (2026-08-27): this door reads @@ -87,12 +95,75 @@ export type Shape = 'array' | 'record'; */ export const RETIRED_FIELD_KEYS = retiredFieldKeysFor('metadataAdminFieldsReadDoor'); -/** Drop {@link RETIRED_FIELD_KEYS} from one field definition. */ +/** + * The pre-objectui#6041 spelling of a relationship target, and the spec key it + * was renamed to. + * + * Named as a PAIR here rather than derived from the tombstone's + * `specEquivalent`: the registry is explicit that `specEquivalent` is + * "Documentation for the reader, NEVER an instruction to migrate a value + * mechanically", because objectui#6043 refused exactly that for `formula` — + * whose value is a LANGUAGE, so a blind rename launders non-CEL text into a + * formula that parses green and evaluates to null. A relationship target is a + * bare object NAME under both spellings, which is what makes reading it under + * either one a read rather than a migration, and that fact is specific to this + * one key. + */ +const RETIRED_REFERENCE_KEY = 'referenceTo'; +const SPEC_REFERENCE_KEY = 'reference'; + +/** Does this value NAME a target object? Blank and non-string name none. */ +function isUsableTarget(value: unknown): value is string { + return typeof value === 'string' && value.trim() !== ''; +} + +/** + * Drop {@link RETIRED_FIELD_KEYS} from one field definition, keeping a + * relationship target the draft holds ONLY under the retired spelling + * (objectui#8896). + * + * ## The claim this repairs — it is this module's own, one paragraph up + * + * {@link RETIRED_FIELD_KEYS}' note said "Nothing is lost on the way out: where + * the spec has a spelling for the concept it is a SEPARATE key (`reference`, + * `system`) that is NOT stripped and rides through untouched". That holds only + * when the draft ALSO carries the spec spelling. A draft holding the target + * only as `referenceTo` had it deleted here with nothing left behind — and + * this is the SINGLE read door for `draft.fields` across the whole object + * designer, with `writeFields` writing each def back verbatim, so the loss + * committed on the next save. `ObjectFieldInspector`'s target editor reads + * `def.reference` and rendered empty; so did every other reader, because + * objectui#6837's ruling ("protocol normalization belongs on the server, the + * front end just executes the protocol") deleted the per-reader legacy arms. + * + * Recovering HERE is that ruling's own shape rather than an exception to it: + * the door normalizes, the readers stay canonical. `reference_to` is the + * ingestion choke points' business (`normalizeSchemaReferenceKeys`, which also + * stamps a second key `FieldSchema` refuses) and is deliberately not touched by + * this door, which serves a WRITE path. + * + * ## Three things this does not do + * + * ⛔ The retired key still never survives — `FieldSchema` refuses it by name, + * which is the whole reason this door strips. + * ⛔ Never overwrites a live `reference`: the spec spelling is what the author + * has been editing, and a stale legacy value beside it is the older truth. + * ⛔ Never invents a target. `unrecognized_keys` fires on the key's PRESENCE, + * so `referenceTo: ''` is a real stored state; recovering it would hand every + * gate downstream a target that names no object. + */ function stripRetiredFieldKeys(def: Record): Record { const present = RETIRED_FIELD_KEYS.filter((k) => k in def); if (present.length === 0) return def; const next = { ...def }; for (const k of present) delete next[k]; + if ( + (present as readonly string[]).includes(RETIRED_REFERENCE_KEY) + && !isUsableTarget(next[SPEC_REFERENCE_KEY]) + && isUsableTarget(def[RETIRED_REFERENCE_KEY]) + ) { + next[SPEC_REFERENCE_KEY] = def[RETIRED_REFERENCE_KEY]; + } return next; } diff --git a/packages/plugin-designer/src/MetadataFieldsPage.carriedThroughReference-8896.test.tsx b/packages/plugin-designer/src/MetadataFieldsPage.carriedThroughReference-8896.test.tsx new file mode 100644 index 0000000000..2619a887ed --- /dev/null +++ b/packages/plugin-designer/src/MetadataFieldsPage.carriedThroughReference-8896.test.tsx @@ -0,0 +1,251 @@ +/** + * 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#8896 — the CARRIED-THROUGH half loses a target stored only as + * `referenceTo`, and the loss refuses the whole save. + * + * ## The path, and why no read door stands in front of it + * + * objectui#8060 split the stored `fields` map: a field whose stored `type` + * this designer cannot author (`master_detail`, `vector`, the other 20) is + * PRESERVED and re-emitted straight from the stored document as + * `carryOver(keep.raw)`. `toDesignerField` — the read door — is on the + * DESIGNABLE branch only, so nothing on this path ever looks at the stored + * field's keys before `carryOver` strips them. + * + * `carryOver` strips `referenceTo` (the registry's `metadataFieldsPageCarryOver` + * site, objectui#6041/#6527), because `FieldSchema` refuses that spelling BY + * NAME and re-emitting it is the hard 422 that blocks every later save. So for + * a stored `master_detail` whose target survives ONLY under the retired + * spelling, the strip takes the target with it, and objectui#7714's + * `assertRelationshipTargetPresent` then refuses the ENTIRE save: + * + * stored { parent_id: { type: 'master_detail', label: 'Parent', + * referenceTo: 'invoice' } } + * author relabels an UNRELATED `name` field + * => puts = [] (no PUT at all) + * => "[MetadataFieldsPage] cannot save the field `parent_id`: a + * `master_detail` field needs a `reference` naming the object it links + * to, … Pick the target object, or change the field to a + * non-relationship type." + * + * ⭐ The consequence, which is what makes this worth a card: the author edited + * a DIFFERENT field. A preserved field is rendered READ-ONLY on this page by + * design (objectui#8060), so "Pick the target object" names a control that does + * not exist here — every later save of that object is refused from this page + * with no way out of it from this page. + * + * ## The repair, and the two things it deliberately does NOT do + * + * ⛔ NOT loosening `assertRelationshipTargetPresent`. The gate is right: a + * `master_detail` really does need a target, and a genuinely target-less one + * must stay refused — pinned below as this file's firing control, so a build + * that simply removed the gate reds instead of passing. + * + * ⛔ NOT emitting `referenceTo`. That spelling is `unrecognized_keys` against + * the installed `FieldSchema`, so carrying the KEY out to the wire re-creates + * the 422 the strip exists to prevent. The VALUE is recovered into the spec + * spelling `reference`; the retired key still never reaches the wire. + * + * This is the same shape the designable half already uses — read the target + * wherever the stored document put it, write it under the one spelling the + * spec declares. + * + * ⚠️ Scope: this file drives the PRESERVED branch only, with a stored type + * (`master_detail`) `DESIGNER_FIELD_TYPES` does not carry. The designable + * half's own read door is objectui#8058 and is NOT touched here — the pin at + * the bottom of `MetadataFieldsPage.specKeyReference.test.tsx` still owns that + * state and must stay green. + */ + +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { act, cleanup, render, screen, waitFor } from '@testing-library/react'; +import { FieldSchema } from '@objectstack/spec/data'; +import { MetadataClient } from '@object-ui/data-objectstack'; +import { DESIGNER_FIELD_TYPES } from '@object-ui/types'; +import type { DesignerFieldDefinition } from '@object-ui/types'; + +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 clientServing(fields: Record>): 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({ + type: 'object', + name: 'probe_widget', + item: { name: 'probe_widget', label: 'Widget', fields }, + lock: 'none', + provenance: 'org', + editable: true, + }); + } + return json({ items: [] }); + }) as unknown as typeof fetch, + }); +} + +async function renderServing(fields: Record>) { + render(); + await waitFor(() => expect(designerProps).not.toBeNull()); +} + +/** A plain relabel of an UNRELATED field — the author never touches the lookup. */ +async function relabelUnrelated() { + await act(async () => { + designerProps!.onFieldsChange!( + designerProps!.fields.map((f) => (f.name === 'name' ? { ...f, label: 'Full name' } : f)), + ); + }); +} + +function savedFields(): Record> { + 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); + +beforeEach(() => { + puts = []; + designerProps = null; +}); + +afterEach(() => { + cleanup(); + designerProps = null; +}); + +describe('the instrument', () => { + it('`master_detail` is a stored type this designer cannot author — so it takes the PRESERVED branch', () => { + // If this ever became authorable the whole file would silently move to the + // designable branch and stop testing what it is named for. + expect((DESIGNER_FIELD_TYPES as readonly string[]).includes('master_detail')).toBe(false); + }); + + it('the installed `FieldSchema` refuses `referenceTo` by name and accepts `reference`', () => { + expect(unrecognizedKeys(FieldSchema.safeParse({ type: 'master_detail', label: 'P', referenceTo: 'invoice' }))) + .toContain('referenceTo'); + expect(FieldSchema.safeParse({ type: 'master_detail', label: 'P', reference: 'invoice' }).success).toBe(true); + }); +}); + +describe('objectui#8896 · a preserved `master_detail` whose target survives only as `referenceTo`', () => { + const STORED = { + name: { type: 'text', label: 'Name', required: true }, + parent_id: { type: 'master_detail', label: 'Parent', referenceTo: 'invoice' }, + }; + + it('saves — the unrelated edit goes out instead of being refused', async () => { + await renderServing(STORED); + await relabelUnrelated(); + await waitFor(() => expect(puts).toHaveLength(1)); + expect(screen.queryByTestId('metadata-fields-page-error')).toBeNull(); + }); + + it('carries the target through, under the spec spelling and never the retired one', async () => { + await renderServing(STORED); + await relabelUnrelated(); + await waitFor(() => expect(puts).toHaveLength(1)); + + const fields = savedFields(); + expect(fields.parent_id.reference).toBe('invoice'); + expect('referenceTo' in fields.parent_id).toBe(false); + // Falsification: the field is still carried through whole — the stored type + // survives, and the author's actual edit landed. + expect(fields.parent_id.type).toBe('master_detail'); + expect(fields.parent_id.label).toBe('Parent'); + expect(fields.name.label).toBe('Full name'); + }); + + it('every field it PUTs parses through the real FieldSchema', async () => { + await renderServing(STORED); + await relabelUnrelated(); + await waitFor(() => expect(puts).toHaveLength(1)); + + 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); + } + }); +}); + +/** + * ⭐ The firing control. Without it every assertion above would also pass on a + * build that simply deleted `assertRelationshipTargetPresent` — which is the + * one repair this card forbids. + */ +describe('objectui#8896 · firing control — a genuinely target-less preserved field is STILL refused', () => { + it('refuses by name and issues no PUT when no spelling carries a target', async () => { + await renderServing({ + name: { type: 'text', label: 'Name', required: true }, + parent_id: { type: 'master_detail', label: 'Parent' }, + }); + await relabelUnrelated(); + + await waitFor(() => + expect(screen.getByTestId('metadata-fields-page-error').textContent).toMatch( + /cannot save the field `parent_id`: a `master_detail` field needs a `reference`/, + ), + ); + expect(puts).toEqual([]); + }); + + it('refuses a preserved field whose retired spelling holds an unusable value', async () => { + // `referenceTo: ''` names no object, so recovering it would smuggle the + // empty target past the gate the previous case proves is live. + await renderServing({ + name: { type: 'text', label: 'Name', required: true }, + parent_id: { type: 'master_detail', label: 'Parent', referenceTo: ' ' }, + }); + await relabelUnrelated(); + + await waitFor(() => + expect(screen.getByTestId('metadata-fields-page-error').textContent).toMatch(/`parent_id`/), + ); + expect(puts).toEqual([]); + }); +}); diff --git a/packages/plugin-designer/src/MetadataFieldsPage.tsx b/packages/plugin-designer/src/MetadataFieldsPage.tsx index 5df38cf47b..bd91952125 100644 --- a/packages/plugin-designer/src/MetadataFieldsPage.tsx +++ b/packages/plugin-designer/src/MetadataFieldsPage.tsx @@ -204,10 +204,18 @@ function toDesignerField(name: string, raw: ServerFieldSchema): DesignerFieldDef * such an object come out parseable; it is keyed to the tombstones, so every * other unknown key the designer does not render still survives. * - * Two of the four cost nothing: `fromDesignerField` re-emits the lookup target - * under the spec spelling `reference` on the very next line, and the system - * flag is read back from the spec spelling `system` (never re-emitted — the - * strip IS the whole write half of objectui#6044). `formula` is the one entry + * Two of the four cost nothing ON THE DESIGNABLE BRANCH: `fromDesignerField` + * re-emits the lookup target under the spec spelling `reference` on the very + * next line, and the system flag is read back from the spec spelling `system` + * (never re-emitted — the strip IS the whole write half of objectui#6044). + * + * ⚠️ The lookup half of that sentence was written before there WAS a second + * branch, and objectui#8060's preserved fields re-emit from the stored document + * with no `fromDesignerField` and no read door in front of them — so for them + * the strip took the target and the save was refused (objectui#8896). That + * branch calls {@link carryPreservedField} rather than this function; the + * qualifier is what stops the next reader from carrying the unconditional + * reading into a third site. `formula` is the one entry * whose strip DROPS a value, and that is objectui#6043's deliberate trade: the * server refuses to store it, a blind rename to `expression` would launder * non-CEL text into a formula that parses green and evaluates to null, and @@ -223,6 +231,20 @@ function toDesignerField(name: string, raw: ServerFieldSchema): DesignerFieldDef */ const RETIRED_FIELD_KEYS = retiredFieldKeysFor('metadataFieldsPageCarryOver'); +/** + * Does this value NAME a target object? + * + * The one predicate both the recovery in {@link carryPreservedField} and the + * refusal in {@link assertRelationshipTargetPresent} read, so the recovery can + * never hand the guard a value the guard would refuse — a whitespace-only + * target names no object (`ObjectSchema.fields`' key grammar + * `/^[a-z_][a-z0-9_]*$/` admits no whitespace-bearing name for it to resolve + * to), and that stays true under either spelling. + */ +function isUsableTarget(value: unknown): value is string { + return typeof value === 'string' && value.trim() !== ''; +} + /** Carry over `prev`'s unknown keys, minus {@link RETIRED_FIELD_KEYS}. */ function carryOver(prev?: ServerFieldSchema): ServerFieldSchema { if (!prev) return {}; @@ -231,6 +253,62 @@ function carryOver(prev?: ServerFieldSchema): ServerFieldSchema { return next; } +/** + * Carry one PRESERVED field through, recovering a relationship target the + * stored document holds ONLY under the retired spelling (objectui#8896). + * + * ## Why the preserved half needs this and the designable half does not + * + * `carryOver`'s docblock says the `referenceTo` strip costs nothing because + * "`fromDesignerField` re-emits the lookup target under the spec spelling + * `reference` on the very next line". That is true of the DESIGNABLE branch, + * which round-trips through {@link toDesignerField} and re-emits from the + * designer model. The PRESERVED branch (objectui#8060) has no designer model + * and no read door at all: it re-emits the stored document verbatim, so the + * strip is the last thing that touches the field and the target leaves with it. + * + * `assertRelationshipTargetPresent` then refuses the WHOLE save — and a + * preserved field is rendered read-only here by design, so its message ("Pick + * the target object") names a control this page does not have. The author edits + * one field and every later save of the object is refused because of a + * different field they cannot author. That is the defect; the gate is right. + * + * ## What is recovered, and what deliberately is not + * + * ⛔ The retired KEY still never reaches the wire. `FieldSchema` refuses + * `referenceTo` by name, so re-emitting it would be the hard 422 the strip + * exists to prevent — this recovers the VALUE into the one spelling the spec + * declares, which is what the designable half has always done. + * + * ⛔ Only when the spec key carries no usable target. The predicate is + * `assertRelationshipTargetPresent`'s own, and sharing it is load-bearing in + * both directions: a live `reference` is never overwritten by a stale legacy + * value, and a recovery can never produce a target the gate would refuse — + * `referenceTo: ' '` names no object, so it stays refused rather than being + * smuggled past as a target. + * + * ⛔ Keyed to THIS tombstone, not to `specEquivalent` in general. The registry + * is explicit that `specEquivalent` is "Documentation for the reader, NEVER an + * instruction to migrate a value mechanically" — objectui#6043 refused exactly + * that for `formula`, whose value is a LANGUAGE and not an object name, and + * `isSystem`'s strip IS the whole write half of objectui#6044. A relationship + * target is a bare object name under both spellings, which is what makes + * reading it under either one a read rather than a migration. + * + * ⛔ Not `normalizeFieldReferenceKeys` from `@object-ui/core`. That helper is + * the INGESTION-side stamp and it writes `reference_to` as well — a second + * spelling `FieldSchema` also refuses by name, and one this carry-over does not + * strip. On a write path it would trade this 422 for another. + */ +function carryPreservedField(prev: ServerFieldSchema): ServerFieldSchema { + const next = carryOver(prev); + if (isUsableTarget(next.reference)) return next; + const legacy = prev.referenceTo; + if (!isUsableTarget(legacy)) return next; + next.reference = legacy; + return next; +} + /** One stored field the designer cannot author, kept whole. */ interface PreservedField { name: string; @@ -495,7 +573,7 @@ function assertRelationshipTargetPresent( ): void { if (!RELATIONSHIP_TYPES_REQUIRING_REFERENCE.includes(String(field?.type))) return; const reference = field?.reference; - if (typeof reference === 'string' && reference.trim() !== '') return; + if (isUsableTarget(reference)) return; throw new Error( `${writer} cannot save the field \`${fieldName}\`: a \`${field?.type}\` field needs a ` + `\`reference\` naming the object it links to, ${describeUnusableTarget(reference)} ` @@ -632,7 +710,7 @@ function toFieldsMap( // document verbatim — `type` included — rather than being rebuilt from a // designer model that has no way to hold it. The tombstone strip still // applies, because those keys 422 whoever wrote them. - entries.push([keep.name, carryOver(keep.raw)]); + entries.push([keep.name, carryPreservedField(keep.raw)]); } }; diff --git a/packages/types/src/internal/retired-field-keys.ts b/packages/types/src/internal/retired-field-keys.ts index f9951abc39..773d710959 100644 --- a/packages/types/src/internal/retired-field-keys.ts +++ b/packages/types/src/internal/retired-field-keys.ts @@ -139,9 +139,28 @@ export const RETIRED_FIELD_KEY_TOMBSTONES = [ { /* * A rename: the spec spells the lookup target `reference` - * ("Did you mean `referenceTo` -> `reference`?"). The strip loses nothing — - * every write path re-emits the designer's target under `reference`, and - * the read door's writers never emit the retired spelling. + * ("Did you mean `referenceTo` -> `reference`?"). + * + * ⚠️ "The strip loses nothing" was written here without a qualifier, and + * objectui#8896 measured it false at two of the three sites. It was only + * ever true where a READ DOOR had already lifted the target out of the + * stored document — `fromDesignerField` re-emitting the designer's target + * under `reference`. The two sites with no read door in front of them + * (`metadataFieldsPageCarryOver`'s objectui#8060 preserved branch, which + * re-emits a stored document verbatim, and `metadataAdminFieldsReadDoor`, + * which IS the read) deleted the only copy of the target, and + * objectui#7714's guard then refused the whole object's save from a page + * that renders the field read-only. + * + * Each of those two sites now lifts the VALUE onto the spec key before + * dropping the retired one, so the strip loses nothing at all three. The + * strip itself is unchanged everywhere: `FieldSchema` refuses this spelling + * BY NAME, so no site emits it. + * + * ⛔ That recovery is written per site and keyed to THIS key. It is NOT + * driven off `specEquivalent` — see the field's own doc, and objectui#6043, + * for why a mechanical rename is refused in general. What makes this key + * different is that its value is a bare object NAME under both spellings. */ key: 'referenceTo', retiredBy: 'objectui#6041', From 378ef4ec53b93fe20aed1a0ec5c7004a6cbdde0f Mon Sep 17 00:00:00 2001 From: os-warren Date: Thu, 10 Sep 2026 00:38:53 +0000 Subject: [PATCH 2/2] docs(types): state the referenceTo tombstone per site instead of restating it unconditionally (objectui#8896) The third site's read path belongs to its caller, so `saveFields` has no in-repo caller to measure. Recording that as UNMEASURED rather than folding it into a fresh unconditional claim is the whole point of this card. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Jmxdo7bmeqCQHLSfmLVX9w --- .../types/src/internal/retired-field-keys.ts | 23 ++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-) diff --git a/packages/types/src/internal/retired-field-keys.ts b/packages/types/src/internal/retired-field-keys.ts index 773d710959..98ed9c8b51 100644 --- a/packages/types/src/internal/retired-field-keys.ts +++ b/packages/types/src/internal/retired-field-keys.ts @@ -153,9 +153,26 @@ export const RETIRED_FIELD_KEY_TOMBSTONES = [ * that renders the field read-only. * * Each of those two sites now lifts the VALUE onto the spec key before - * dropping the retired one, so the strip loses nothing at all three. The - * strip itself is unchanged everywhere: `FieldSchema` refuses this spelling - * BY NAME, so no site emits it. + * dropping the retired one. The strip itself is unchanged everywhere: + * `FieldSchema` refuses this spelling BY NAME, so no site emits it. + * + * ⚠️ The claim is therefore per site, and is NOT restated as a third + * unconditional sentence — restating the conclusion without its condition is + * how this entry went wrong the first time: + * + * - `metadataFieldsPageCarryOver` — nothing is lost. Its designable branch + * reads through `storedRelationshipTarget` (objectui#8058) and its + * preserved branch through `carryPreservedField` (objectui#8896); both + * re-emit under `reference`. + * - `metadataAdminFieldsReadDoor` — nothing is lost. The door keeps the + * value under `reference` as it drops the key (objectui#8896). + * - `metadataServiceCarryOver` — ⛔ UNMEASURED, deliberately. That site + * strips a stored entry it merges UNDERNEATH an already-built + * `DesignerFieldDefinition`, and `toFieldPayload` writes `reference` + * from that model unconditionally. Whether a target survives therefore + * depends on the CALLER's read door, and `saveFields` has no in-repo + * caller to measure (it is a published service API). Whoever measures + * one: this is the same class, and the answer belongs here. * * ⛔ That recovery is written per site and keyed to THIS key. It is NOT * driven off `specEquivalent` — see the field's own doc, and objectui#6043,