From 2503a710dfdac315140aea8ca1df1cb9c10a641d Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 28 Aug 2026 07:52:28 +0000 Subject: [PATCH] fix(app-shell): make MetadataService.saveObject's fields argument required MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `ObjectSchema.fields` is REQUIRED, and `metadata-protocol`'s `saveMetaItem` parses the whole item against that schema and throws before it persists. So a `saveObject` call that omitted the field list built a body the server was guaranteed to refuse `422 INVALID_METADATA` — every time it ran. The method cannot write a valid document without it, so the argument is now required: the guaranteed runtime failure moves to compile time and runtime accept/reject is unchanged. Nothing new is exported. The objectui#6240 anti-wipe control moves with the signature rather than being deleted as newly-unreachable — its runtime half still guards the path a JavaScript consumer can reach, and its new `@ts-expect-error` is the compile-time half, which reds if the argument ever goes back to optional. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01CRJge11jso9TpXRWFt1Z49 --- .changeset/6490-saveobject-fields-required.md | 42 ++++++++++++++++ ...dataService.objectPayloadFieldsMap.test.ts | 35 ++++++++++++++ .../MetadataService.saveAdvisories.test.ts | 19 ++++++-- .../app-shell/src/services/MetadataService.ts | 48 ++++++++++++++++++- 4 files changed, 138 insertions(+), 6 deletions(-) create mode 100644 .changeset/6490-saveobject-fields-required.md diff --git a/.changeset/6490-saveobject-fields-required.md b/.changeset/6490-saveobject-fields-required.md new file mode 100644 index 0000000000..7b67e3ed26 --- /dev/null +++ b/.changeset/6490-saveobject-fields-required.md @@ -0,0 +1,42 @@ +--- +'@object-ui/app-shell': minor +--- + +**BREAKING (in name only):** `MetadataService.saveObject(obj, existingFields)` now requires +its second argument (objectui#6490). Calls that omitted it no longer compile. + +**Runtime behaviour is unchanged, and that is the whole justification.** A call that omitted +the field list was already a guaranteed `422` — every time it ran, against every backend. +`ObjectSchema.fields` is not merely typed, it is REQUIRED: measured against the installed +`@objectstack/spec` 17.2.0, `ObjectSchema.safeParse({ name: 'account', label: 'Account' })` +fails with `invalid_type @ fields`, and `metadata-protocol`'s `saveMetaItem` parses the whole +item against that same schema and throws `422 INVALID_METADATA` **before** it persists. The +method cannot build a valid document without the argument, so the only calls this break +breaks are calls that already failed. The signature is now honest about it, and the diagnosis +moves from a round trip at runtime to the compiler. + +Nothing new is exported and nothing new is accepted — this narrows the published surface +rather than widening it. In-repo production call sites were measured at **zero** (only tests +called it), so the migration for an external consumer is to pass the field list it was +already required to send: `saveObject(obj, fields)`. + +⛔ Two readings were considered and declined, recorded so neither is taken later as a +shortcut. **Not a `{}` default** — `{}` parses GREEN and `PUT /api/v1/meta/object/:name` is +an upsert, so defaulting would delete every field of the object on a save that only meant to +rename it, trading a loud, harmless 422 for silent data loss; the anti-wipe control from +objectui#6240 (`omits fields entirely when the caller supplied none — it does NOT write {}`) +moves with the signature and still guards the path a JavaScript consumer can reach. **Not +fetch-and-merge** — an object save could GET the current document and preserve its stored +`fields` the way `saveFields` does, but that builds capability for a path with zero measured +pull and makes the parameter redundant. + +An EMPTY list stays a different statement from a missing one: `[]` means "this object has no +fields", writes `{}`, and under the upsert performs the wipe the caller asked for — the same +authoritative reading `saveFields` gives its own empty list. Unchanged, and now pinned, +because the required parameter is what routes a caller with nothing to hand toward it. + +Scored `minor` and not `major` per AGENTS.md §版本号策略 — objectui's major is pinned to the +`@objectstack` major so that "same major ⇒ compatible" holds across the two repos, and every +publishable package sits in one `fixed` group, so objectui's own breaking changes ship as +`minor` with the break spelled out in the body. That is the convention, which is why the +break is stated in words above. diff --git a/packages/app-shell/src/services/MetadataService.objectPayloadFieldsMap.test.ts b/packages/app-shell/src/services/MetadataService.objectPayloadFieldsMap.test.ts index 05beca166f..2dc2f59a16 100644 --- a/packages/app-shell/src/services/MetadataService.objectPayloadFieldsMap.test.ts +++ b/packages/app-shell/src/services/MetadataService.objectPayloadFieldsMap.test.ts @@ -217,7 +217,24 @@ describe('objectui#6240 · saveObject PUTs `fields` as a name-keyed map', () => // caller that simply did not pass `existingFields` would delete every field // of the object. The body stays refused instead — unchanged from before // this card, and deliberately so. + // + // objectui#6490 made `existingFields` REQUIRED, and this control MOVED with + // the signature rather than being deleted as newly-unreachable. Both halves + // of it are still live, and they measure different things: + // + // 1. The `@ts-expect-error` below is the COMPILE-TIME half of #6490. This + // project (`tsconfig.test.json`) compiles this file, and an unused + // `@ts-expect-error` is itself an error — so the line reds if the + // argument ever goes back to optional. That is the only direction the + // narrowing can regress in, and this is what pins it. + // 2. The RUNTIME half is unchanged and still reachable. `MetadataService` + // is public API (app-shell exports `useMetadataService`), so a + // JavaScript consumer — or a TypeScript one that casts past the types — + // can still arrive here with no field list. What it must get is the + // loud, harmless 422 below, never the silent wipe. const { adapter, puts } = makeCapturingAdapter(); + // @ts-expect-error objectui#6490 — `existingFields` is required. This call is + // exactly what the new signature refuses, and it is compiled to prove it does. await new MetadataService(adapter).saveObject(ACCOUNT); expect(puts).toHaveLength(1); @@ -226,6 +243,24 @@ describe('objectui#6240 · saveObject PUTs `fields` as a name-keyed map', () => // Positive control: the rest of the object still went out. expect(puts[0]).toMatchObject({ name: 'account', label: 'Account', pluralLabel: 'Accounts' }); }); + + it('objectui#6490 · an EMPTY list is a STATEMENT, not a missing argument — it writes `{}`', async () => { + // The cell the required parameter makes canonical: with the argument no + // longer omittable, `[]` is the only spelling left for "no fields", and it + // means what it says. Unchanged behaviour — `[]` is truthy, so it has always + // converted to `{}` — pinned now because the signature change is what routes + // callers to it. The two readings must stay distinguishable: the control + // above is about a MISSING argument and never about an empty one, and + // `saveFields` reads its own empty list the same authoritative way. + const { adapter, puts } = makeCapturingAdapter(); + await new MetadataService(adapter).saveObject(ACCOUNT, []); + + expect(puts).toHaveLength(1); + expect(puts[0].fields).toEqual({}); + // And unlike the missing-argument body, this one is one the server ACCEPTS — + // which is the whole hazard, and why it may only ever happen on request. + expect(issuesOf(ObjectSchema.safeParse(puts[0]))).toEqual([]); + }); }); describe('objectui#6240 · saveFields no longer converts the server’s map INTO an array', () => { diff --git a/packages/app-shell/src/services/MetadataService.saveAdvisories.test.ts b/packages/app-shell/src/services/MetadataService.saveAdvisories.test.ts index 1f28e4defc..e5936f6a91 100644 --- a/packages/app-shell/src/services/MetadataService.saveAdvisories.test.ts +++ b/packages/app-shell/src/services/MetadataService.saveAdvisories.test.ts @@ -34,7 +34,7 @@ import { describe, it, expect, vi } from 'vitest'; import { ObjectStackAdapter, type MetadataSaveAdvisoryEvent } from '@object-ui/data-objectstack'; import type { ObjectDefinition } from '@object-ui/types'; -import { MetadataService } from './MetadataService'; +import { MetadataService, type FieldMetadataPayload } from './MetadataService'; import { emitSaveAdvisories, type TranslateFn } from '../providers/saveAdvisoryToast'; const PURGE_ADVISORY = { @@ -100,6 +100,15 @@ function makeWiredAdapter(body: unknown) { // (objectui#4040). Declared properly instead of widening the cast. const ACCOUNT: ObjectDefinition = { id: 'account', name: 'account', label: 'Account' }; +// objectui#6490 made that second parameter REQUIRED. `ObjectSchema.fields` is a +// required record, so a `saveObject` call that omitted the list built a body the +// server refused `422 INVALID_METADATA` every time it ran — these suites were +// asserting the advisory channel on top of a save that could never have +// succeeded against a real backend. The field list is not this file's subject, +// so it hands over the smallest well-formed one; nothing else about these +// assertions changes. +const ACCOUNT_FIELDS: FieldMetadataPayload[] = [{ name: 'name', type: 'text', label: 'Name' }]; + describe('MetadataService saves reach the shell advisory surface (#4237)', () => { it('renders the gate findings for a save that succeeded', async () => { const { adapter, sink, events } = makeWiredAdapter({ @@ -107,7 +116,7 @@ describe('MetadataService saves reach the shell advisory surface (#4237)', () => advisories: [PURGE_ADVISORY], }); - await new MetadataService(adapter).saveObject(ACCOUNT); + await new MetadataService(adapter).saveObject(ACCOUNT, ACCOUNT_FIELDS); expect(events).toHaveLength(1); expect(events[0]).toMatchObject({ type: 'object', name: 'account', mode: 'publish' }); @@ -117,7 +126,7 @@ describe('MetadataService saves reach the shell advisory surface (#4237)', () => it('lands on the WARNING tier and says "Saved" first — the write succeeded', async () => { const { adapter, sink } = makeWiredAdapter({ ...CLEAN_BODY, advisories: [PURGE_ADVISORY] }); - await new MetadataService(adapter).saveObject(ACCOUNT); + await new MetadataService(adapter).saveObject(ACCOUNT, ACCOUNT_FIELDS); const [title, opts] = sink.warning.mock.calls[0]!; expect(title).toMatch(/^Saved/); @@ -130,7 +139,7 @@ describe('MetadataService saves reach the shell advisory surface (#4237)', () => it('a clean save renders no new UI', async () => { const { adapter, sink, events } = makeWiredAdapter(CLEAN_BODY); - await new MetadataService(adapter).saveObject(ACCOUNT); + await new MetadataService(adapter).saveObject(ACCOUNT, ACCOUNT_FIELDS); expect(events).toEqual([]); expect(sink.warning).not.toHaveBeenCalled(); @@ -152,7 +161,7 @@ describe('MetadataService saves reach the shell advisory surface (#4237)', () => const { adapter } = makeWiredAdapter({ ...CLEAN_BODY, advisories: [PURGE_ADVISORY] }); const invalidate = vi.spyOn(adapter, 'invalidateCache'); - await expect(new MetadataService(adapter).saveObject(ACCOUNT)).resolves.toBeUndefined(); + await expect(new MetadataService(adapter).saveObject(ACCOUNT, ACCOUNT_FIELDS)).resolves.toBeUndefined(); // The service's own post-save step still runs. expect(invalidate).toHaveBeenCalledWith('object:account'); diff --git a/packages/app-shell/src/services/MetadataService.ts b/packages/app-shell/src/services/MetadataService.ts index 958eda1174..0a52311700 100644 --- a/packages/app-shell/src/services/MetadataService.ts +++ b/packages/app-shell/src/services/MetadataService.ts @@ -171,6 +171,13 @@ function toObjectPayload(obj: ObjectDefinition, fields?: FieldMetadataPayload[]) // right outcome for a caller that under-specified an upsert, and the same // outcome as before this change. `saveFields` is the opposite case and // treats its argument as authoritative; see there. + // + // This parameter stays OPTIONAL after objectui#6490 made `saveObject`'s + // public one required, and the optionality is the point rather than a + // leftover: the type now says no in-repo caller can reach this branch, and + // the branch is what a caller who ignores the types still lands in. It is + // the last line of defence against the wipe, so it may not be deleted as + // newly-unreachable code. fields: fields ? toFieldsMap(fields) : undefined, }; } @@ -493,8 +500,47 @@ export class MetadataService { /** * Persist an object definition to the backend. * Works for both create and update (the API is an upsert). + * + * ## Why `existingFields` is REQUIRED (objectui#6490) + * + * `ObjectSchema.fields` is not merely typed, it is REQUIRED. Measured against + * the installed `@objectstack/spec` 17.2.0: + * + * ObjectSchema.safeParse({ name: 'account', label: 'Account' }) + * => success = false invalid_type @ fields + * + * and `metadata-protocol`'s `saveMetaItem` parses the whole item against that + * same schema and throws BEFORE it persists. So a call that omitted the field + * list built a body the server was guaranteed to refuse with + * `422 INVALID_METADATA` — this method cannot write a valid document without + * it. Requiring the argument moves that guaranteed RUNTIME failure to compile + * time; runtime accept/reject is unchanged, because the only calls it breaks + * are calls that already failed, every time they ran. + * + * An EMPTY list is a different statement from a missing one, and it is + * accepted: `[]` says "this object has no fields", writes `{}`, and under the + * upsert that is a field wipe the caller asked for — the same authoritative + * reading `saveFields` gives its own empty list. Pinned alongside the + * anti-wipe control in `MetadataService.objectPayloadFieldsMap.test.ts`, so + * the two readings of "no fields" stay distinguishable. + * + * Two readings were declined by the maintainer ruling (2026-08-27), recorded + * here so neither returns as a shortcut: + * + * - ⛔ **Not a `{}` default.** `{}` parses GREEN, so defaulting would delete + * every field of the object on a save that only meant to rename it — + * trading a loud, harmless 422 for silent data loss. `toObjectPayload` + * still omits the key for an input that supplies nothing, and that branch + * is still reachable: this class is public API through + * `useMetadataService`, so a JavaScript consumer, or one that casts past + * the types, can arrive there. What it must get is the 422, not the wipe. + * - ⛔ **Not fetch-and-merge.** `saveFields` GETs the current document and + * spreads it, and an object save could preserve the stored `fields` the + * same way — but that builds capability for a path with zero measured + * pull and makes this parameter redundant, which then wants retiring on + * its own terms (ADR-0049 shape). */ - async saveObject(obj: ObjectDefinition, existingFields?: FieldMetadataPayload[]): Promise { + async saveObject(obj: ObjectDefinition, existingFields: FieldMetadataPayload[]): Promise { const client = this.adapter.getClient(); const payload = toObjectPayload(obj, existingFields); await client.meta.saveItem('object', obj.name, payload);