From 9798788b7156308471087ef8686554a6ea4ca2ce Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 25 Aug 2026 12:05:12 +0000 Subject: [PATCH] fix(designer): retire the formula-expression control instead of renaming its key MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Field Designer's formula textarea wrote `formula`, a key `FieldSchema` refuses BY NAME, so `PUT /api/v1/meta/object/:name` returned a hard 422 `INVALID_METADATA` that blocked every later save of the object. The spec spells the concept `expression`, and the rename was deliberately NOT taken. `FieldSchema` judges the key and never the expression LANGUAGE: measured on @objectstack/spec 17.2.0 it accepts `expression: 'price * quantity'` and even `expression: '!!!not cel at all!!!'`. Spec `expression` is CEL rooted at `record`, while this control's own placeholder taught `price * quantity` — bare field refs that evaluate to null silently under the scope formulas bind. A rename would have converted a loud 422 into a formula that saves clean and computes nothing. Making refusals loud in the control needs CEL lint and returnType inference, i.e. CelPredicateField — which lives in @object-ui/app-shell, and app-shell depends on @object-ui/plugin-designer, so it cannot be imported back without a cycle. Expressions are authored in metadata-admin's ObjectFieldInspector, which lints against the real @objectstack/formula engine. The field TYPE `formula` stays: it is a valid spec FieldType and only the expression key was refused. `formula` joins RETIRED_FIELD_KEYS so an object already carrying it is stripped clean on its next save rather than staying blocked forever — with the control gone there is no other way left to clear it. A stored `expression` is untouched. Also drops the key from object-fields-bridge.ts, a third emit site the parity gate does not cover, and clears the ledger entry, which ratchets both ways. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_011SfZeFWrhGLHmfq61xbz4q --- .../6043-retire-designer-formula-control.md | 59 ++++ .../MetadataService.retiredFormula.test.ts | 158 +++++++++ .../app-shell/src/services/MetadataService.ts | 15 +- .../previews/object-fields-bridge.ts | 13 +- .../plugin-designer/src/FieldDesigner.tsx | 25 +- ...MetadataFieldsPage.retiredFormula.test.tsx | 312 ++++++++++++++++++ .../src/MetadataFieldsPage.tsx | 41 ++- ...eldDesigner.formulaControlRetired.test.tsx | 186 +++++++++++ packages/types/src/designer.ts | 26 +- scripts/check-designer-field-key-parity.mjs | 14 +- 10 files changed, 828 insertions(+), 21 deletions(-) create mode 100644 .changeset/6043-retire-designer-formula-control.md create mode 100644 packages/app-shell/src/services/MetadataService.retiredFormula.test.ts create mode 100644 packages/plugin-designer/src/MetadataFieldsPage.retiredFormula.test.tsx create mode 100644 packages/plugin-designer/src/__tests__/FieldDesigner.formulaControlRetired.test.tsx diff --git a/.changeset/6043-retire-designer-formula-control.md b/.changeset/6043-retire-designer-formula-control.md new file mode 100644 index 0000000000..00f6f3a85e --- /dev/null +++ b/.changeset/6043-retire-designer-formula-control.md @@ -0,0 +1,59 @@ +--- +'@object-ui/types': minor +'@object-ui/plugin-designer': minor +'@object-ui/app-shell': minor +--- + +The Field Designer no longer offers a formula-expression textarea, and no designer write +path emits a `formula` key (objectui#6043). + +**This is a behaviour change on an authoring surface: a control is removed.** A field's +`type` may still be set to `formula` — that is a valid spec `FieldType` and stays in the +palette — but the expression itself is no longer authored here. Authors write formula +expressions in metadata-admin's field inspector, where they are checked. + +The control wrote `formula`, which is not in `FieldSchema`'s accept set. Measured against +the installed `@objectstack/spec` 17.2.0: + +``` +FieldSchema.safeParse({ type:'formula', label:'Tax', formula:'price * quantity' }) + => success = false + => unrecognized_keys ['formula'] "Did you mean `formula` -> `expression`?" +``` + +so `PUT /api/v1/meta/object/:name` returned a hard 422 `INVALID_METADATA` — and because +the key was then stored, it blocked **every later save of that object**, not just the one +that introduced it. + +**The key was deliberately NOT renamed to the spec's `expression`.** `FieldSchema` judges +the key name and never the expression LANGUAGE — measured, it accepts +`expression: 'price * quantity'` and even `expression: '!!!not cel at all!!!'`; only the +empty string is refused. Spec `expression` is CEL rooted at `record` +(`record.amount * 0.1`), whereas this control's own placeholder taught `price * quantity` +— bare field refs, which under the scope formulas bind evaluate to null silently. A rename +would therefore have converted a loud, immediate 422 into a formula that saves clean and +then quietly computes nothing, which is strictly worse than the bug it appears to fix. + +Making refusals loud *in the control* would need CEL lint, autocomplete and `returnType` +inference — that is `CelPredicateField`, which lives in `@object-ui/app-shell`, and +app-shell depends on `@object-ui/plugin-designer`, so it cannot be imported back without a +dependency cycle. Growing a second formula-authoring surface inside plugin-designer is a +feature, not this fix. `returnType` is likewise not authored here: it is only derivable by +inferring the CEL result type, and with no expression control there is nothing to infer +from. + +`formula` joins the retired-key tombstone in `MetadataFieldsPage`, so an object already +carrying the key is stripped clean on its next save instead of staying blocked forever — +which matters more than usual here, because with the control gone an author would +otherwise have no way left to clear it. It is dropped rather than migrated to `expression`, +for the same reason the rename was refused. A `expression` authored in metadata-admin is +**not** touched: it is a real `FieldSchema` key and rides through the designer's +round-trip untouched. + +Also removes the now-unreachable `formula` read/write from +`views/metadata-admin/previews/object-fields-bridge.ts`, which was a third emit site for +the key that neither the card nor the parity gate named. + +The `formula` entry is removed from `check-designer-field-key-parity.mjs`'s +`KNOWN_UNPARSEABLE_KEYS` ledger, which ratchets in both directions — a resolved key that +left a stale entry behind would be as red as a new offender. diff --git a/packages/app-shell/src/services/MetadataService.retiredFormula.test.ts b/packages/app-shell/src/services/MetadataService.retiredFormula.test.ts new file mode 100644 index 0000000000..f9464471e2 --- /dev/null +++ b/packages/app-shell/src/services/MetadataService.retiredFormula.test.ts @@ -0,0 +1,158 @@ +/** + * 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#6043 — `MetadataService` never writes `formula`, and does not rename + * it to `expression` either. + * + * 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`. + * + * `formula` is not in `FieldSchema`'s accept set. Measured against the installed + * `@objectstack/spec` 17.2.0: + * + * FieldSchema.safeParse({ type:'formula', label:'Total', formula:'price * quantity' }) + * => success = false + * => unrecognized_keys ['formula'] + * "Did you mean `formula` -> `expression`?" + * + * 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. + * + * ## The rename this file proves was NOT taken + * + * The obvious repair — emit `expression` instead — was refused, and the control + * below is what keeps that decision legible. `FieldSchema` judges the KEY, never + * the expression LANGUAGE: it accepts `expression: '!!!not cel at all!!!'`. + * Spec `expression` is CEL rooted at `record`, whereas the designer control this + * card retired taught `price * quantity` — bare field refs that evaluate to null + * silently under the scope formulas bind. Renaming would therefore have replaced + * a loud 422 with a formula that saves clean and computes nothing. + * + * Assertions are on the bytes actually PUT — `JSON.parse` of the captured + * request body — not on the object handed to the client. A property whose value + * is `undefined` is a key zod's strict object COUNTS but `JSON.stringify` DROPS, + * so an in-memory assertion and a wire assertion disagree exactly here. + * + * This file names no `reference`/`referenceTo` key, so reverting objectui#6041 + * cannot red it, and vice versa. + */ + +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, in wire order. */ +function savedFields(puts: Array>): 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); + +const FORMULA_FIELD: DesignerFieldDefinition = { + id: 'total', + name: 'total', + label: 'Total', + type: 'formula', +}; + +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 `formula` by name while accepting the field TYPE `formula`', () => { + expect( + unrecognizedKeys(FieldSchema.safeParse({ type: 'formula', label: 'Total', formula: 'price * quantity' })), + ).toEqual(['formula']); + // Only the expression key was ever the defect. The type stays authorable. + expect(FieldSchema.safeParse({ type: 'formula', label: 'Total' }).success).toBe(true); + }); + + it('accepts `expression` without parsing the CEL in it — why the rename was refused', () => { + expect(FieldSchema.safeParse({ type: 'formula', label: 'T', expression: '!!!not cel at all!!!' }).success).toBe( + true, + ); + }); +}); + +describe('objectui#6043 · saveFields never PUTs a formula expression', () => { + it('emits neither `formula` nor `expression` for a formula field', async () => { + const { adapter, puts } = makeCapturingAdapter(); + + await new MetadataService(adapter).saveFields('invoice', [FORMULA_FIELD]); + + const [def] = savedFields(puts); + expect('formula' in def).toBe(false); + // Not renamed. `toFieldPayload` has no expression source to write and must + // not invent one — see this file's header for why that is the fix. + expect('expression' in def).toBe(false); + // Falsification: the field itself made the trip, so the two absences above + // are a payload builder that stopped copying the key, not an empty PUT. + expect(def.type).toBe('formula'); + expect(def.label).toBe('Total'); + }); + + it('drops a `formula` smuggled onto the field instead of copying it through', async () => { + // `DesignerFieldDefinition` no longer DECLARES `formula`, so this cast is + // the point rather than a workaround: it proves `toFieldPayload` is closed + // at RUNTIME, not merely that the type forbids the key. A stale build, a JS + // caller, or a re-added designer control all arrive by exactly this route. + const { adapter, puts } = makeCapturingAdapter(); + const smuggled = { ...FORMULA_FIELD, formula: 'price * quantity' } as DesignerFieldDefinition; + + await new MetadataService(adapter).saveFields('invoice', [smuggled]); + + const [def] = savedFields(puts); + expect('formula' in def).toBe(false); + expect('expression' in def).toBe(false); + }); + + it('the PUT body parses through the real FieldSchema', async () => { + const { adapter, puts } = makeCapturingAdapter(); + + await new MetadataService(adapter).saveFields('invoice', [FORMULA_FIELD]); + + const [def] = savedFields(puts); + const result = FieldSchema.safeParse(def); + expect(unrecognizedKeys(result)).toEqual([]); + expect(result.success).toBe(true); + }); +}); diff --git a/packages/app-shell/src/services/MetadataService.ts b/packages/app-shell/src/services/MetadataService.ts index 53da577dae..f49ce1f236 100644 --- a/packages/app-shell/src/services/MetadataService.ts +++ b/packages/app-shell/src/services/MetadataService.ts @@ -79,7 +79,19 @@ export interface FieldMetadataPayload { // 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; + // No `formula` (objectui#6043): the spec spells a formula field's expression + // `expression`, and it is CEL. `FieldSchema.safeParse` refuses `formula` BY + // NAME ("Did you mean `formula` -> `expression`?"), so a formula field + // authored in the designer made `PUT /api/v1/meta/object/:name` fail 422 + // `INVALID_METADATA` and blocked every later save of that object. + // + // Deliberately NOT renamed to `expression`. `FieldSchema` validates the key + // but not the LANGUAGE: measured on 17.2.0 it accepts + // `expression: '!!!not cel at all!!!'`. Emitting the retired textarea's + // non-CEL contents under the accepted spelling would have turned a loud, + // immediate 422 into a formula that parses and then silently evaluates to + // null. Expressions are authored in metadata-admin's `ObjectFieldInspector`, + // which lints them against the real `@objectstack/formula` engine. sortOrder?: number; } @@ -126,7 +138,6 @@ function toFieldPayload(field: DesignerFieldDefinition): FieldMetadataPayload { externalId: field.externalId, trackHistory: field.trackHistory, reference: field.referenceTo, - formula: field.formula, sortOrder: field.sortOrder, }; } diff --git a/packages/app-shell/src/views/metadata-admin/previews/object-fields-bridge.ts b/packages/app-shell/src/views/metadata-admin/previews/object-fields-bridge.ts index 8e90fd2fd3..436cf37791 100644 --- a/packages/app-shell/src/views/metadata-admin/previews/object-fields-bridge.ts +++ b/packages/app-shell/src/views/metadata-admin/previews/object-fields-bridge.ts @@ -43,7 +43,9 @@ interface FrameworkFieldDef { placeholder?: string; options?: Array<{ label?: string; value: string; color?: string }>; reference?: string; - formula?: string; + // No declared `formula` (objectui#6043) — nothing reads or writes it any + // more. A legacy def that still carries one reaches this type through the + // index signature below, and a QUARANTINED field round-trips verbatim. group?: string; [k: string]: unknown; } @@ -139,7 +141,6 @@ export function bridgeFromDraft(fieldsInput: unknown): FieldsBridgeResult { })) : undefined, referenceTo: typeof def?.reference === 'string' ? def.reference : undefined, - formula: typeof def?.formula === 'string' ? def.formula : undefined, }); } @@ -207,6 +208,12 @@ function serializeDesignerField(f: DesignerFieldDefinition): FrameworkFieldDef { if (f.group) out.group = f.group; if (f.options && f.options.length > 0) out.options = f.options; if (f.referenceTo) out.reference = f.referenceTo; - if (f.formula) out.formula = f.formula; + // No `formula` (objectui#6043). This bridge was a THIRD emit site for the + // key, named by neither the card nor `check-designer-field-key-parity.mjs` — + // `FrameworkFieldDef` is not one of that gate's declared `PAYLOAD_SHAPES`, so + // the gate was green over it. `FieldSchema` refuses `formula` BY NAME, and it + // is not renamed to `expression` here for the same reason as everywhere else + // on this card: the schema accepts the key without parsing the CEL, so a + // rename ships an unevaluatable expression under a valid name. return out; } diff --git a/packages/plugin-designer/src/FieldDesigner.tsx b/packages/plugin-designer/src/FieldDesigner.tsx index 2a7609051e..f857c29910 100644 --- a/packages/plugin-designer/src/FieldDesigner.tsx +++ b/packages/plugin-designer/src/FieldDesigner.tsx @@ -219,7 +219,6 @@ export function FieldDesigner({ defaultValue: data.defaultValue ? String(data.defaultValue) : undefined, placeholder: data.placeholder ? String(data.placeholder) : undefined, referenceTo: data.referenceTo ? String(data.referenceTo) : undefined, - formula: data.formula ? String(data.formula) : undefined, }; onFieldsChange?.(fields.map((f) => (f.id === editingField.id ? updated : f))); } else { @@ -240,7 +239,6 @@ export function FieldDesigner({ defaultValue: data.defaultValue ? String(data.defaultValue) : undefined, placeholder: data.placeholder ? String(data.placeholder) : undefined, referenceTo: data.referenceTo ? String(data.referenceTo) : undefined, - formula: data.formula ? String(data.formula) : undefined, isSystem: false, }; onFieldsChange?.([...fields, newField]); @@ -309,7 +307,27 @@ export function FieldDesigner({ label: t('appDesigner.fieldDesigner.typeSpecificSection'), fields: [ { name: 'referenceTo', label: t('appDesigner.fieldDesigner.referenceTo'), type: 'text', placeholder: 'Referenced object', disabled: readOnly, visibleWhen: "record.type == 'lookup'" }, - { name: 'formula', label: t('appDesigner.fieldDesigner.formula'), type: 'textarea', placeholder: 'e.g. price * quantity', disabled: readOnly, visibleWhen: "record.type == 'formula'" }, + // No `formula` control (objectui#6043). It wrote a key the spec + // refuses BY NAME — `FieldSchema` spells a formula field's expression + // `expression` — so typing in this box made + // `PUT /api/v1/meta/object/:name` fail 422 `INVALID_METADATA` and + // blocked every later save of the object. + // + // Renaming the key was considered and REFUSED. `FieldSchema` does not + // parse CEL at the key level (measured on 17.2.0: it accepts + // `expression: '!!!not cel at all!!!'`), and this control's own + // placeholder was `e.g. price * quantity` — bare field refs, which + // formulas' `record` scope evaluates to null silently. A rename would + // have traded a loud 422 for a silent wrong answer. + // + // Making it loud instead needs the CEL engine: lint, autocomplete and + // `returnType` inference. That is `CelPredicateField`, which lives in + // `@object-ui/app-shell` — and app-shell DEPENDS on this package, so + // it cannot be imported back without a cycle. Formula expressions are + // therefore authored in metadata-admin's `ObjectFieldInspector`, + // where they are checked against the real `@objectstack/formula` + // engine. The field TYPE `formula` is a valid spec `FieldType` and + // stays in the palette above; only this unvalidatable box is gone. { name: 'defaultValue', label: t('appDesigner.fieldDesigner.defaultValue'), type: 'text', placeholder: 'Default value', disabled: readOnly }, { name: 'placeholder', label: t('appDesigner.fieldDesigner.placeholder'), type: 'text', placeholder: 'Placeholder text', disabled: readOnly }, { name: 'group', label: t('appDesigner.fieldDesigner.fieldGroup'), type: 'text', placeholder: 'Field Group', disabled: readOnly }, @@ -348,7 +366,6 @@ export function FieldDesigner({ defaultValue: editingField.defaultValue != null ? String(editingField.defaultValue) : '', placeholder: editingField.placeholder || '', referenceTo: editingField.referenceTo || '', - formula: editingField.formula || '', } : { type: 'text', required: false, unique: false }, onSuccess: handleFormSuccess, diff --git a/packages/plugin-designer/src/MetadataFieldsPage.retiredFormula.test.tsx b/packages/plugin-designer/src/MetadataFieldsPage.retiredFormula.test.tsx new file mode 100644 index 0000000000..dd3914fe75 --- /dev/null +++ b/packages/plugin-designer/src/MetadataFieldsPage.retiredFormula.test.tsx @@ -0,0 +1,312 @@ +/** + * 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#6043 — the Field Designer never puts `formula` on the wire, and does + * not rename it to `expression` either. + * + * 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. + * + * `formula` is not in `FieldSchema`'s accept set. Measured against the installed + * `@objectstack/spec` 17.2.0: + * + * FieldSchema.safeParse({ type:'formula', label:'Tax', formula:'price * quantity' }) + * => success = false + * => unrecognized_keys ['formula'] + * "Did you mean `formula` -> `expression`?" + * + * i.e. a hard 422 `INVALID_METADATA` that blocks every later save of the object. + * + * ## Why this card did NOT take its siblings' shape + * + * `referenceTo` (objectui#6041) and `isSystem` (objectui#6044) were renames: the + * spec had an accepted spelling for the same value, so the emit site moved and + * nothing was lost. The spec has a spelling here too — `expression` — and this + * card refused to use it. The reason is measurable, and the control below pins + * it so a later reader cannot mistake the refusal for an oversight: + * + * FieldSchema.safeParse({ type:'formula', expression:'!!!not cel at all!!!' }) + * => success = TRUE + * + * `FieldSchema` validates the KEY, never the expression LANGUAGE. `expression` + * is CEL rooted at `record`, while the retired control's own placeholder taught + * `price * quantity` — bare field refs, which `celAuthoring.ts` records as + * silently evaluating to null at runtime under the scope formulas bind. So a + * rename would have moved the failure from a loud, immediate 422 to a formula + * that saves clean and then quietly computes nothing. The control was removed + * instead; expressions are authored in metadata-admin's `ObjectFieldInspector`, + * where the real `@objectstack/formula` engine lints them. + * + * Written against the wire like its siblings `MetadataFieldsPage.saveEnvelope` + * and `MetadataFieldsPage.specKeyReference`: a REAL `MetadataClient` over a + * fetch double, assertions on the captured PUT bytes rather than on the argument + * handed to the client. That distinction is load-bearing here — a property whose + * value is `undefined` is a key zod's strict object COUNTS but `JSON.stringify` + * DROPS, so an in-memory assertion and a wire assertion disagree on exactly the + * fields this card touches. + * + * This file names no `reference`/`referenceTo` or `system`/`isSystem` key: the + * cards of this family 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. + * + * `total` carries what a SPEC-PARSED SERVER sends for a formula field — + * `expression` plus `returnType`, neither of which this page renders a + * control for. Both must survive an edit-and-save here untouched. + * `legacy_total` carries the refused key a pre-fix designer build wrote. + * `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, with the control that could once + * clear it now gone. + */ +const OBJECT_BODY = { + name: 'probe_invoice', + label: 'Invoice', + fields: { + amount: { type: 'number', label: 'Amount' }, + total: { + type: 'formula', + label: 'Total', + expression: 'record.amount * 0.2', + returnType: 'number', + }, + legacy_total: { type: 'formula', label: 'Legacy Total', formula: 'price * quantity' }, + }, +}; + +const OBJECT_ENVELOPE = { + type: 'object', + name: 'probe_invoice', + 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_invoice' }); + } + if (/\/meta\/object\/probe_invoice(\?|$)/.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> { + 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', () => { + // 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 `formula` by name — the state this file keeps off the wire', () => { + expect( + unrecognizedKeys(FieldSchema.safeParse({ type: 'formula', label: 'Tax', formula: 'price * quantity' })), + ).toEqual(['formula']); + }); + + it('accepts the field TYPE `formula` — only the expression key was refused', () => { + // Load-bearing: the fix removes a CONTROL, not a field type. If the type + // were refused too the card's shape would have been different, and this + // assertion is what makes that claim checkable rather than assumed. + expect(FieldSchema.safeParse({ type: 'formula', label: 'Tax' }).success).toBe(true); + }); + + it('accepts `expression` WITHOUT parsing the CEL in it — why the rename was refused', () => { + // The card's establishing argument, as an executable fact. `FieldSchema` + // is a key-name oracle: it cannot tell a formula from a shopping list, so + // renaming would have bought a green parse for anything at all. + expect(FieldSchema.safeParse({ type: 'formula', label: 'T', expression: 'record.amount * 0.2' }).success).toBe(true); + expect(FieldSchema.safeParse({ type: 'formula', label: 'T', expression: 'price * quantity' }).success).toBe(true); + expect(FieldSchema.safeParse({ type: 'formula', label: 'T', expression: '!!!not cel at all!!!' }).success).toBe(true); + }); +}); + +describe('objectui#6043 · WRITE — no save carries `formula`, under any spelling', () => { + it('strips the refused key from an object ALREADY carrying it, unblocking the object', async () => { + // The case that keeps a blocked object blocked without the tombstone: + // removing the control does not touch what `carryOver` spreads, so the + // stored key would ride back out to the same 422 — and with the control + // gone the author would have no way left to clear it. + await renderPage(); + await relabel('amount', 'Amount (net)'); + + const fields = savedFields(); + expect('formula' in fields.legacy_total).toBe(false); + // Falsification: the strip removed a KEY, not the field, and did not + // launder the value into the accepted spelling either — that laundering is + // precisely what this card refused. + expect(fields.legacy_total.type).toBe('formula'); + expect(fields.legacy_total.label).toBe('Legacy Total'); + expect('expression' in fields.legacy_total).toBe(false); + expect(FieldSchema.safeParse(fields.legacy_total).success).toBe(true); + }); + + it('drops a `formula` smuggled onto a designer field instead of emitting it', async () => { + // `DesignerFieldDefinition` no longer DECLARES `formula`, so this cast is + // the point rather than a workaround: it proves the converter is closed at + // RUNTIME, not merely that the type forbids the key. A stale build, a JS + // caller or a re-added control all reach `fromDesignerField` this way. + await renderPage(); + const next = designerProps!.fields.map((f) => + f.name === 'total' ? ({ ...f, formula: 'price * quantity' } as DesignerFieldDefinition) : f, + ); + await act(async () => { + designerProps!.onFieldsChange!(next); + }); + await waitFor(() => expect(puts).toHaveLength(1)); + + expect('formula' in savedFields().total).toBe(false); + }); + + it('every field it PUTs parses through the real FieldSchema', async () => { + await renderPage(); + await relabel('amount', 'Amount (net)'); + + 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 formula field emits a parseable def with no expression key at all', async () => { + await renderPage(); + const next: DesignerFieldDefinition[] = [ + ...designerProps!.fields, + { id: 'fld_new', name: 'tax', label: 'Tax', type: 'formula' }, + ]; + await act(async () => { + designerProps!.onFieldsChange!(next); + }); + await waitFor(() => expect(puts).toHaveLength(1)); + + const tax = savedFields().tax; + expect('formula' in tax).toBe(false); + expect('expression' in tax).toBe(false); + // The field itself is still authored and still saves — removing the control + // did not remove the ability to declare a field computed. + expect(tax.type).toBe('formula'); + expect(FieldSchema.safeParse(tax).success).toBe(true); + }); +}); + +describe('objectui#6043 · READ — an expression authored elsewhere survives this page', () => { + it('round-trips `expression` and `returnType` untouched', async () => { + // The read half, in the shape route (B) permits. The card asked whether an + // existing formula field "loads with its expression populated"; with the + // control removed there is no box to populate, so the obligation becomes + // the stronger one: this page must not DESTROY an expression authored in + // metadata-admin, where the CEL engine checked it. + // + // ⚠ This case also passes on a revert, and says so deliberately: `carryOver` + // already preserved both keys. It is a must-not-change pin, not a two-world + // assertion — the two-world rows are in the WRITE block above. + await renderPage(); + await relabel('amount', 'Amount (net)'); + + const total = savedFields().total; + expect(total.expression).toBe('record.amount * 0.2'); + expect(total.returnType).toBe('number'); + expect(FieldSchema.safeParse(total).success).toBe(true); + }); + + it('hands no formula expression down to the designer in any form', async () => { + await renderPage(); + const total = designerProps!.fields.find((f) => f.name === 'total')!; + expect('formula' in total).toBe(false); + // Falsification: the field itself arrived, with its other keys intact, so + // the absence above is a reader that stopped reading the key — not a field + // that failed to load. + expect(total.label).toBe('Total'); + expect(total.type).toBe('formula'); + }); +}); diff --git a/packages/plugin-designer/src/MetadataFieldsPage.tsx b/packages/plugin-designer/src/MetadataFieldsPage.tsx index bacfbec4df..7150ee2e43 100644 --- a/packages/plugin-designer/src/MetadataFieldsPage.tsx +++ b/packages/plugin-designer/src/MetadataFieldsPage.tsx @@ -58,7 +58,19 @@ interface ServerFieldSchema { * every later save of the object. See {@link RETIRED_FIELD_KEYS}. */ reference?: string; - formula?: string; + /* + * No `formula` (objectui#6043). The spec spells a formula field's expression + * `expression` and it is CEL; `FieldSchema` refuses `formula` BY NAME, so + * emitting it made `PUT /api/v1/meta/object/:name` fail 422 and blocked every + * later save. It is NOT renamed here — see {@link RETIRED_FIELD_KEYS} and the + * tombstone on `DesignerFieldDefinition` for why a rename was refused. + * + * `expression` itself is deliberately NOT declared: this page renders no + * control for it, and the index signature below plus `carryOver` already + * round-trip it verbatim, so a formula authored in metadata-admin survives an + * edit-and-save here untouched. Declaring it would put it back in this gate's + * reach for no reader. + */ // The framework also stores `select` field options as `options: string[] | // {label, value}[]`; we passthrough the raw structure for now. options?: unknown; @@ -109,7 +121,6 @@ function toDesignerField(name: string, raw: ServerFieldSchema): DesignerFieldDef externalId: raw.externalId, trackHistory: raw.trackHistory, referenceTo: raw.reference, - formula: raw.formula, }; } @@ -145,8 +156,31 @@ function toDesignerField(name: string, raw: ServerFieldSchema): DesignerFieldDef * 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. + * + * objectui#6043 adds `formula`, and it is the one entry here that is NOT half + * of a rename — the difference matters, because it is the only reason this + * strip loses anything: + * + * - For `referenceTo` and `isSystem`, `fromDesignerField` re-emits the value + * under the spec spelling on a later line, so stripping costs nothing. + * - For `formula` there is no re-emit, because the card REFUSED the rename. + * `FieldSchema` does not parse CEL at the key level (17.2.0 accepts + * `expression: '!!!not cel at all!!!'`), so migrating a stored `formula` + * into `expression` would launder a non-CEL string — typically the + * `price * quantity` the retired control's own placeholder taught — into a + * valid key name, where it parses green and then evaluates to null at + * runtime. That is the silent failure the card exists to avoid, so the key + * is dropped rather than renamed. + * + * Dropping it is what makes an already-blocked object saveable again, and there + * is no gentler option: with the control gone, an author has no other way to + * clear the key, so leaving it would keep the object 422-blocked forever. The + * value being dropped is one the server already refuses to store, so nothing + * that ever persisted is lost. `expression` is NOT stripped — it is a real + * `FieldSchema` key, so a formula authored in metadata-admin rides through + * `carryOver` untouched. */ -const RETIRED_FIELD_KEYS = ['indexed', 'referenceTo', 'isSystem'] as const; +const RETIRED_FIELD_KEYS = ['indexed', 'referenceTo', 'isSystem', 'formula'] as const; /** Carry over `prev`'s unknown keys, minus {@link RETIRED_FIELD_KEYS}. */ function carryOver(prev?: ServerFieldSchema): ServerFieldSchema { @@ -175,7 +209,6 @@ function fromDesignerField( externalId: designed.externalId, trackHistory: designed.trackHistory, reference: designed.referenceTo, - formula: designed.formula, }; } diff --git a/packages/plugin-designer/src/__tests__/FieldDesigner.formulaControlRetired.test.tsx b/packages/plugin-designer/src/__tests__/FieldDesigner.formulaControlRetired.test.tsx new file mode 100644 index 0000000000..01eda18906 --- /dev/null +++ b/packages/plugin-designer/src/__tests__/FieldDesigner.formulaControlRetired.test.tsx @@ -0,0 +1,186 @@ +/** + * 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#6043 — the Field Designer offers no formula-expression control, and + * its form no longer carries the key in either direction. + * + * The wire halves of this card are pinned by + * `MetadataFieldsPage.retiredFormula.test.tsx` and + * `MetadataService.retiredFormula.test.ts`. This file pins the half neither of + * those can see: the CONTROL. A converter that drops `formula` keeps the PUT + * parseable even while the drawer still renders a formula textarea — the author + * would then type an expression, watch the save succeed, and get a field that + * computes nothing. That failure is invisible to a wire assertion, so it needs + * its own instrument here. + * + * ## What was removed, and why not renamed + * + * The control wrote `formula`, a key `FieldSchema` refuses BY NAME, so saving a + * formula field returned a hard 422 `INVALID_METADATA` that blocked every later + * save of the object. The spec spells the concept `expression` — and the rename + * was refused, because `FieldSchema` judges the key and never the expression + * LANGUAGE (it accepts `expression: '!!!not cel at all!!!'`). Spec `expression` + * is CEL rooted at `record`; this control's placeholder taught + * `e.g. price * quantity`, whose bare field refs `celAuthoring.ts` records as + * evaluating to null silently. Renaming would have bought a green save for an + * expression that quietly computes nothing. + * + * Teaching CEL in the control instead would need lint, autocomplete and + * `returnType` inference — i.e. `CelPredicateField`, which lives in + * `@object-ui/app-shell`. app-shell DEPENDS on this package, so it cannot be + * imported back without a cycle, and this package has no CEL engine of its own. + * Formula expressions are therefore authored in metadata-admin's + * `ObjectFieldInspector`, where the real `@objectstack/formula` engine checks + * them. + * + * ## Every absence here is paired with a presence + * + * A `queryByTestId(...)` that returns null is green when the control is gone AND + * green when the drawer never opened, the mock never rendered, or the section + * list came back empty. So each absence below is asserted next to a POSITIVE + * CONTROL — a sibling control of the same section, captured through the same + * testid channel in the same render. If the harness did nothing, the control + * fails first and the absence is never read as a result. + */ + +import React from 'react'; +import { describe, it, expect, vi, afterEach } from 'vitest'; +import { render, screen, fireEvent, waitFor, cleanup } from '@testing-library/react'; +import type { DesignerFieldDefinition } from '@object-ui/types'; +import { FieldDesigner } from '../FieldDesigner'; + +vi.mock('@object-ui/plugin-grid', () => import('./__mocks__/plugin-grid')); +vi.mock('@object-ui/plugin-form', () => import('./__mocks__/plugin-form')); + +/** + * A formula field as it exists after this card: a real, authorable field TYPE + * (`FieldSchema` accepts `{ type: 'formula', label }`) with no expression key + * on the designer's model at all. + */ +const FIELDS: DesignerFieldDefinition[] = [ + { id: 'amount', name: 'amount', label: 'Amount', type: 'number' }, + { id: 'total', name: 'total', label: 'Total', type: 'formula' }, + { id: 'owner_id', name: 'owner_id', label: 'Owner', type: 'lookup', referenceTo: 'account' }, +]; + +afterEach(cleanup); + +function renderDesigner(onFieldsChange?: (f: DesignerFieldDefinition[]) => void) { + render( + , + ); +} + +/** + * Open the edit drawer for one field. + * + * The grid mock fills its rows from `dataSource.find()`, a PROMISE, so the row + * buttons do not exist on the first paint. Querying for one synchronously threw + * "unable to find an element" while the tree was still empty — and had this file + * only asserted absences, that same empty tree would have made every one of them + * pass. Waiting for the row is what turns the assertions that follow into + * readings of a mounted drawer. + */ +async function openEditDrawer(fieldName: string) { + const row = await screen.findByTestId(`grid-edit-${fieldName}`); + fireEvent.click(row); + await waitFor(() => expect(screen.getByTestId('mock-drawer-form')).toBeTruthy()); +} + +describe('objectui#6043 · the drawer offers no formula-expression control', () => { + it('renders no `formula` field in the create drawer', async () => { + renderDesigner(); + fireEvent.click(screen.getByTestId('grid-add-btn')); + await waitFor(() => expect(screen.getByTestId('mock-drawer-form')).toBeTruthy()); + + // POSITIVE CONTROL first. `referenceTo` is the sibling control in the very + // same `typeSpecific` section, reached through the same testid channel. If + // it is missing the harness rendered nothing and the absence below would be + // a vacuous pass rather than a measurement. + expect(screen.getByTestId('drawer-field-referenceTo')).toBeTruthy(); + expect(screen.getByTestId('drawer-section-typeSpecific')).toBeTruthy(); + + expect(screen.queryByTestId('drawer-field-formula')).toBeNull(); + }); + + it('renders no `formula` field in the edit drawer either', async () => { + // Create and edit build the drawer from the same schema but different + // `initialValues`, and the pre-fix code seeded the key on BOTH paths. + renderDesigner(); + await openEditDrawer('total'); + + expect(screen.getByTestId('drawer-field-referenceTo')).toBeTruthy(); + expect(screen.getByTestId('drawer-mode').textContent).toBe('edit'); + + expect(screen.queryByTestId('drawer-field-formula')).toBeNull(); + }); +}); + +describe('objectui#6043 · the form model carries no formula expression', () => { + it('emits no `formula` key when an existing formula field is edited and saved', async () => { + // End-to-end through the real code: the mock submits `schema.initialValues` + // back into the real `handleFormSuccess`, so this exercises the edit SEED + // and the update WRITE path together — the two sites that both named the + // key before this card. + const changed = vi.fn(); + renderDesigner(changed); + await openEditDrawer('total'); + fireEvent.click(screen.getByTestId('drawer-submit')); + + await waitFor(() => expect(changed).toHaveBeenCalledTimes(1)); + const emitted: DesignerFieldDefinition[] = changed.mock.calls[0][0]; + const total = emitted.find((f) => f.name === 'total')!; + + expect('formula' in total).toBe(false); + // Not renamed either — the designer has no expression source and must not + // invent one under the accepted spelling. + expect('expression' in total).toBe(false); + // Falsification: the round-trip actually happened and preserved the field, + // so the absences above are a model that stopped carrying the key rather + // than a submit that never fired. + expect(total.type).toBe('formula'); + expect(total.label).toBe('Total'); + }); + + it('still round-trips a lookup target through the same path', async () => { + // The must-not-change half. This is the control for the test above: it + // proves the edit seed and update path still carry type-specific values in + // general, so their silence on `formula` is specific rather than a broken + // form. + // + // ⚠ This case also passes on a revert of this card, and says so + // deliberately — objectui#6041 owns the `reference` spelling, not this file. + const changed = vi.fn(); + renderDesigner(changed); + await openEditDrawer('owner_id'); + fireEvent.click(screen.getByTestId('drawer-submit')); + + await waitFor(() => expect(changed).toHaveBeenCalledTimes(1)); + const emitted: DesignerFieldDefinition[] = changed.mock.calls[0][0]; + const owner = emitted.find((f) => f.name === 'owner_id')!; + + expect(owner.referenceTo).toBe('account'); + }); + + it('keeps `formula` in the type palette — the TYPE was never the defect', async () => { + // Measured on `@objectstack/spec` 17.2.0: `FieldSchema.safeParse({ type: + // 'formula', label: 'Tax' })` succeeds. Only the expression key was refused, + // so removing the type would have broken formula fields outright — the + // opposite of this card. The grouped type `