diff --git a/.changeset/client-meta-saveitem-query-options.md b/.changeset/client-meta-saveitem-query-options.md new file mode 100644 index 0000000000..76e4d3b4b7 --- /dev/null +++ b/.changeset/client-meta-saveitem-query-options.md @@ -0,0 +1,55 @@ +--- +'@objectstack/client': minor +--- + +`meta.saveItem` accepts the query-string options bag its route already reads — +`force`, `packageId`, `mode` — on both clients + +The Phase 3a-destructive gate refuses a metadata save with +`409 DESTRUCTIVE_CHANGE` and ends the message `— re-submit with ?force=true to +proceed.` Both REST `PUT` doors read `?force` off the query string and thread +it, so that sentence is true of an HTTP caller. It was **false of a +first-party SDK caller**: `meta.saveItem(type, name, item)` built a bare path +and a body and sent no query string at all, on either declaration. A caller +who did literally what the refusal prescribed got the identical refusal back, +and the only way to act on it was to abandon `@objectstack/client` for raw +`fetch`. + +Three parameters are newly reachable, and they are exactly the three +`PUT /api/v1/meta/:type/:name` reads: + +- **`force?: boolean`** — `?force=true`, the destructive-change opt-in the 409 + message names. Only the opt-IN is spelled on the wire: `false` and + `undefined` both omit the parameter rather than sending `?force=false`. + That is a hazard avoided, not tidiness — the door refuses a *repeated* + `force` because a repeated value arrives as an array and a non-empty array + is truthy, so an opt-OUT that reached the wire twice would switch the guard + ON. +- **`packageId?: string`** — `?package=`, binding the saved row to a + software package (`sys_metadata.package_id`). Named `packageId` to match the + sibling `getItem` / `getItems` options on the same object. +- **`mode?: 'draft' | 'publish'`** — `?mode=draft`, staging the write as a + pending draft. `'publish'` is the default said out loud and deliberately + sends nothing, since the door acts on `mode=draft` alone. + +**Backward compatible.** The bag is optional and an options-less call builds a +byte-identical URL to before — `''`, not a trailing `?`. Existing +three-argument callers are unaffected, and pins measure that rather than +assuming it. + +Both declarations move together — the unscoped `ObjectStackClient.meta` and +the environment-scoped `ScopedProjectClient.meta` — sharing ONE exported +`SaveMetaItemOptions` type and ONE query builder rather than a literal copied +into each. They are the same method on two clients reaching one pair of routes +(the scoped mount is the same route registration replayed under +`/environments/:environmentId`, so it reads the same three parameters), and +every divergence measured between these twins so far has been closed as a +defect. A bag spelled twice is the next one waiting to be introduced. + +The branch was selected by measurement, not preference: the SDK is the real +metadata-write path for both surfaces the ruling named. The CLI's `os meta +register` goes through `client.meta.saveItem` and the CLI has no raw-HTTP +metadata-save path at all; Studio reaches it from 21 production call sites +across `@object-ui/app-shell`, `plugin-designer`, `data-objectstack` and the +console app — including the object and field designers, where dropping a field +and saving is precisely what raises the destructive 409. diff --git a/packages/client/src/client.test.ts b/packages/client/src/client.test.ts index e5692a9a6e..20f1f2b4f6 100644 --- a/packages/client/src/client.test.ts +++ b/packages/client/src/client.test.ts @@ -2562,3 +2562,200 @@ describe('Analytics namespace (#3584 dispatcher alignment)', () => { ); }); }); + +// ---------------------------------------------------------------------- +// [#11391] `meta.saveItem`'s query string — the destructive-409 remedy has +// to be reachable from the SDK, on BOTH clients. +// +// The Phase 3a-destructive gate refuses with `409 DESTRUCTIVE_CHANGE` and +// ends `— re-submit with ?force=true to proceed.` Both REST `PUT` doors read +// `?force` and thread it, so that sentence is true of an HTTP caller. It was +// FALSE of a first-party SDK caller: `saveItem` built a bare path and a body +// and sent no query string at all, so doing exactly what the refusal said +// returned the identical refusal and the only way out was raw `fetch`. +// +// These pins are on the URL the client BUILDS, deliberately. A test that only +// checks the method accepts an option would stay green against a client that +// swallows it — which is the same defect one layer in. +// ---------------------------------------------------------------------- + +describe('[#11391] meta.saveItem query string (unscoped client)', () => { + it('threads `force: true` onto the URL as ?force=true', async () => { + const { client, fetchMock } = createMockClient({ success: true, version: 2 }); + await client.meta.saveItem('object', 'customer', { name: 'customer' }, { force: true }); + const [url, init] = fetchMock.mock.calls[0]; + expect(String(url)).toBe('http://localhost:3000/api/v1/meta/object/customer?force=true'); + expect(init.method).toBe('PUT'); + // The body is untouched by the option — `force` is a query parameter, + // not a field the server reads off the document being saved. + expect(JSON.parse(init.body)).toEqual({ name: 'customer' }); + }); + + it('exposes ?package and ?mode=draft in the same bag, in a stable order', async () => { + const { client, fetchMock } = createMockClient({ success: true }); + await client.meta.saveItem('object', 'customer', { name: 'customer' }, { + force: true, + packageId: 'app.crm', + mode: 'draft', + }); + expect(String(fetchMock.mock.calls[0][0])).toBe( + 'http://localhost:3000/api/v1/meta/object/customer?force=true&package=app.crm&mode=draft', + ); + }); + + it('sends `package` alone when that is all the caller set', async () => { + const { client, fetchMock } = createMockClient({ success: true }); + await client.meta.saveItem('object', 'customer', {}, { packageId: 'app.crm' }); + expect(String(fetchMock.mock.calls[0][0])).toBe( + 'http://localhost:3000/api/v1/meta/object/customer?package=app.crm', + ); + }); + + it('url-encodes a packageId that needs it', async () => { + const { client, fetchMock } = createMockClient({ success: true }); + await client.meta.saveItem('object', 'customer', {}, { packageId: 'acme/crm suite' }); + expect(String(fetchMock.mock.calls[0][0])).toBe( + 'http://localhost:3000/api/v1/meta/object/customer?package=acme%2Fcrm+suite', + ); + }); + + it('BACKWARD COMPATIBLE: a 3-argument call still sends no query string at all', async () => { + const { client, fetchMock } = createMockClient({ success: true }); + await client.meta.saveItem('object', 'customer', { name: 'customer' }); + // Byte-identical to the pre-#11391 URL — not `…/customer?`. + expect(String(fetchMock.mock.calls[0][0])).toBe( + 'http://localhost:3000/api/v1/meta/object/customer', + ); + }); + + it('an empty bag is also byte-identical to no bag', async () => { + const { client, fetchMock } = createMockClient({ success: true }); + await client.meta.saveItem('object', 'customer', {}, {}); + expect(String(fetchMock.mock.calls[0][0])).toBe( + 'http://localhost:3000/api/v1/meta/object/customer', + ); + }); + + it('NEVER spells the opt-OUT on the wire (#6877): force:false sends nothing', async () => { + const { client, fetchMock } = createMockClient({ success: true }); + await client.meta.saveItem('object', 'customer', {}, { force: false }); + // `?force=false` would be a live hazard rather than a no-op: the door + // refuses a REPEATED `force` because a repeated value arrives as an + // array and a non-empty array is truthy — a spelled-out opt-OUT that + // reached the wire twice would turn the destructive guard ON. Emitting + // nothing keeps this client clear of that edge. + expect(String(fetchMock.mock.calls[0][0])).toBe( + 'http://localhost:3000/api/v1/meta/object/customer', + ); + }); + + it("mode:'publish' is the default said out loud and sends nothing", async () => { + const { client, fetchMock } = createMockClient({ success: true }); + await client.meta.saveItem('object', 'customer', {}, { mode: 'publish' }); + expect(String(fetchMock.mock.calls[0][0])).toBe( + 'http://localhost:3000/api/v1/meta/object/customer', + ); + }); + + it('a compound name keeps its unencoded slash AND gets the query string', async () => { + const { client, fetchMock } = createMockClient({ success: true }); + await client.meta.saveItem('object', 'views/all_leads', { label: 'All leads' }, { force: true }); + // The slash must still survive (%2F would collapse this onto the + // 3-segment route and miss `PUT /meta/:type/:section/:name`), and the + // compound door reads `?force` too since #11095. + expect(String(fetchMock.mock.calls[0][0])).toBe( + 'http://localhost:3000/api/v1/meta/object/views/all_leads?force=true', + ); + }); +}); + +describe('[#11391] meta.saveItem query string (environment-scoped twin)', () => { + it('threads `force: true` on the scoped client too', async () => { + const { client, fetchMock } = createMockClient({ success: true }); + await client.project('proj-123').meta.saveItem( + 'object', 'customer', { name: 'customer' }, { force: true }, + ); + const [url, init] = fetchMock.mock.calls[0]; + expect(String(url)).toBe( + 'http://localhost:3000/api/v1/environments/proj-123/meta/object/customer?force=true', + ); + expect(init.method).toBe('PUT'); + }); + + it('exposes the same three parameters as the unscoped twin', async () => { + const { client, fetchMock } = createMockClient({ success: true }); + await client.project('proj-123').meta.saveItem('object', 'customer', {}, { + force: true, + packageId: 'app.crm', + mode: 'draft', + }); + expect(String(fetchMock.mock.calls[0][0])).toBe( + 'http://localhost:3000/api/v1/environments/proj-123/meta/object/customer' + + '?force=true&package=app.crm&mode=draft', + ); + }); + + it('BACKWARD COMPATIBLE: a 3-argument scoped call still sends no query string', async () => { + const { client, fetchMock } = createMockClient({ success: true }); + await client.project('proj-123').meta.saveItem('object', 'customer', {}); + expect(String(fetchMock.mock.calls[0][0])).toBe( + 'http://localhost:3000/api/v1/environments/proj-123/meta/object/customer', + ); + }); + + it('IN STEP with the unscoped twin: identical query for identical options', async () => { + // The twins are one method on two clients reaching one pair of routes + // (the scoped mount is the same `registerForBase` replayed under + // `/environments/:id`). This compares the QUERY they build rather than + // restating both URLs, so it keeps holding if either path changes. + const { client, fetchMock } = createMockClient({ success: true }); + const opts = { force: true, packageId: 'app.crm', mode: 'draft' } as const; + await client.meta.saveItem('object', 'customer', {}, opts); + await client.project('proj-123').meta.saveItem('object', 'customer', {}, opts); + const queryOf = (u: unknown) => new URL(String(u)).search; + expect(queryOf(fetchMock.mock.calls[1][0])).toBe(queryOf(fetchMock.mock.calls[0][0])); + expect(queryOf(fetchMock.mock.calls[0][0])).toBe('?force=true&package=app.crm&mode=draft'); + }); +}); + +describe('[#11391] the destructive-409 remedy loop is now closed for an SDK caller', () => { + it('refused with DESTRUCTIVE_CHANGE, the caller can do what the message says', async () => { + // Round 1 answers the real refusal envelope; round 2 answers a save. + const refusal = { + error: "[destructive_change] object/customer would drop or transform existing data:" + + " field 'legacy_code' removed — re-submit with ?force=true to proceed.", + code: 'DESTRUCTIVE_CHANGE', + }; + const responses = [ + { ok: false, status: 409, statusText: 'Conflict', json: async () => refusal, headers: new Headers() }, + { ok: true, status: 200, statusText: 'OK', json: async () => ({ success: true, version: 3 }), headers: new Headers() }, + ]; + const fetchMock = vi.fn().mockImplementation(() => Promise.resolve(responses.shift())); + const client = new ObjectStackClient({ baseUrl: 'http://localhost:3000', fetch: fetchMock }); + + // 1. Refused. Assert the ENVELOPE the caller branches on, not merely + // that something threw: a bare `.toThrow()` would stay green + // against any error at all. + const err: any = await client.meta.saveItem('object', 'customer', { name: 'customer' }) + .then(() => { throw new Error('expected the destructive save to be refused'); }, (e) => e); + expect(err.code).toBe('DESTRUCTIVE_CHANGE'); + // The SDK parks the numeric on `httpStatus`; `status` is only set on + // the auth-login path, so this is the carrier to read here. + expect(err.httpStatus).toBe(409); + expect(String(err.message)).toContain('re-submit with ?force=true to proceed.'); + expect(String(fetchMock.mock.calls[0][0])).toBe( + 'http://localhost:3000/api/v1/meta/object/customer', + ); + + // 2. Do literally what the refusal prescribed — and the parameter it + // names actually reaches the route. THIS is the acceptance + // criterion: before #11391 there was no argument to pass here. + const saved = await client.meta.saveItem( + 'object', 'customer', { name: 'customer' }, { force: true }, + ); + expect(String(fetchMock.mock.calls[1][0])).toBe( + 'http://localhost:3000/api/v1/meta/object/customer?force=true', + ); + expect(saved).toEqual({ success: true, version: 3 }); + }); +}); diff --git a/packages/client/src/index.ts b/packages/client/src/index.ts index 742a91c0bb..cc5b84fb94 100644 --- a/packages/client/src/index.ts +++ b/packages/client/src/index.ts @@ -484,6 +484,123 @@ function normalizeActionResult(payload: any): { success: boolean; data?: T; e return { success: true, data: payload as T }; } +/** + * Query-string options for `meta.saveItem` on BOTH clients — the unscoped + * `ObjectStackClient.meta` and {@link ScopedProjectClient.meta}. + * + * ONE exported type deliberately shared by the two declarations, rather than + * an inline literal copied into each. The two `saveItem`s are the same method + * on two clients reaching one pair of routes; every divergence measured + * between them so far has been closed as a defect (#7019 and the cards citing + * it), and a bag spelled twice is a divergence waiting to be introduced by + * whoever next extends only the copy they happened to open. + * + * ## Why these three, and why they are the ones that exist + * + * `PUT /api/v1/meta/:type/:name` reads exactly these three query parameters, + * and until this type existed the SDK sent NONE of them — `saveItem` built a + * bare path and a body. The sharpest consequence is `force`: `saveMetaItem`'s + * Phase 3a-destructive gate refuses with `409 DESTRUCTIVE_CHANGE` and ends the + * message `— re-submit with ?force=true to proceed.`, so a first-party SDK + * caller was told to set a parameter their client had no way to set. Doing + * literally what the refusal said returned the identical refusal, forever; the + * only way out was to abandon the SDK for raw `fetch`. That is not a + * hypothetical — the platform QA checklist instructs its own authors to issue + * these steps "as raw HTTP with the query string appended" for exactly this + * reason, and `@object-ui/data-objectstack` carries a hand-rolled + * `MetadataClient.save` that composes these same three parameters itself. + * + * ⛔ The remedy clause is the contract here, not a nicety: it is a + * risk-acknowledgement refusal, and `destructiveChangeRemedy` renders it per + * write FACE precisely so that no caller is ever prescribed a mechanism their + * door does not have. Both REST `PUT` doors state face `'meta-envelope'`, + * whose clause names `?force=true` — so the clause is only true of an SDK + * caller while this bag exists. Removing it re-breaks the prose, not just the + * feature. + * + * The face is stated by the SERVER and is not derivable from the caller's + * identity: an SDK save and a raw `fetch` save arrive at the same door as the + * same request. There is therefore no "SDK-flavoured" wording available to + * repair this from the message side — the parameter had to become reachable. + */ +export interface SaveMetaItemOptions { + /** + * `?force=true` — acknowledge and proceed past the Phase 3a + * destructive-change refusal (`409 DESTRUCTIVE_CHANGE`), whose message + * names this parameter as the remedy. + * + * Only `true` reaches the wire. `false` and `undefined` both OMIT the + * parameter rather than sending `?force=false`, which is not merely + * tidier: the door refuses a REPEATED `?force` (#6877) because a repeated + * value arrives as an array and a non-empty array is truthy — so a + * spelled-out opt-OUT could turn the guard ON. Never emitting the + * opt-out spelling keeps this client clear of that edge entirely. + */ + force?: boolean; + /** + * `?package=` — bind the saved row to that software package + * (`sys_metadata.package_id`). Omit for an environment-local overlay. + * Named `packageId` rather than `package` to match the sibling + * `getItem`/`getItems` options on this same object (`package` is also a + * reserved word). + */ + packageId?: string; + /** + * `?mode=draft` — stage the write as a pending draft instead of + * publishing it to the active overlay. + * + * `'publish'` is the explicit spelling of the default and deliberately + * sends NOTHING: the door acts on `mode=draft` alone and treats every + * other value as publish, so emitting `?mode=publish` would put a value + * on the wire that the server ignores. Same shape the first-party + * `@object-ui/data-objectstack` `MetadataClient.save` already uses. + * + * ⚠️ COMPOUND NAMES DO NOT STAGE. `mode` reaches only the single-segment + * `PUT /meta/:type/:name`. Its compound-name twin + * `PUT /meta/:type/:section/:name` — the door a `name` containing a slash + * lands on, e.g. `saveItem('object', 'views/all_leads', item)` — never + * reads this parameter, so `{ mode: 'draft' }` there is IGNORED and the + * write is PUBLISHED LIVE, answered 200. It is not refused; there is no + * signal at the call site. Filed as objectstack#11712 and deliberately not + * repaired from this side: threading it is the route's decision, and a + * client-side guess would be a second place the two doors disagree. + * + * ⛔ Do not "fix" this by rejecting compound names here. `force` and + * `packageId` DO reach both doors (measured: the compound handler reads + * and threads `?force` since objectstack#11095 and `?package` alongside + * it), so refusing the whole bag on a compound name would break the two + * parameters that work in order to warn about the one that does not. + */ + mode?: 'draft' | 'publish'; +} + +/** + * Compose the `meta.saveItem` query string — the ONE builder both `saveItem` + * declarations call, for the same reason {@link SaveMetaItemOptions} is one + * type: the twins must not be able to drift in what they put on the wire. + * + * Returns `''` (not `'?'`) when nothing is set, so an options-less call is + * BYTE-IDENTICAL to what this method sent before the bag existed. That is the + * backward-compatibility guarantee, and it is what the pre-existing URL pins + * measure. + * + * `URLSearchParams.set` (never `append`) is load-bearing: the door REFUSES a + * repeated `force` / `package` / `mode`, so a builder that could emit a key + * twice would turn a caller's option into a 400. + */ +function metaSaveQuery(options?: SaveMetaItemOptions): string { + if (!options) return ''; + const params = new URLSearchParams(); + // Only the opt-IN is spelled on the wire — see `force`'s doc comment. + if (options.force) params.set('force', 'true'); + if (options.packageId) params.set('package', options.packageId); + // Only `'draft'` is actionable server-side; `'publish'` is the default + // said out loud and sends nothing. + if (options.mode === 'draft') params.set('mode', 'draft'); + const qs = params.toString(); + return qs ? `?${qs}` : ''; +} + export class ObjectStackClient { private baseUrl: string; private token?: string; @@ -700,9 +817,26 @@ export class ObjectStackClient { * declaration used to stop at `{ success, message }`, and annotating * against that subset would have hidden the OCC carrier (#5545). */ - saveItem: async (type: string, name: string, item: any): Promise => { + saveItem: async ( + type: string, + name: string, + item: any, + options?: SaveMetaItemOptions, + ): Promise => { const route = this.getRoute('metadata'); - const res = await this.fetch(`${this.baseUrl}${route}/${type}/${name}`, { + // Named `query`, not `qs`. Measured on this file: of 37 `const qs =` + // bindings, 27 hold a BARE `params.toString()` (the `?` is added at + // the interpolation site), 8 hold a string that CARRIES its own `?`, + // and 2 hold a `URLSearchParams` object. One name, three meanings — + // so a reader cannot tell from `${qs}` whether a `?` is already + // there, and picking the wrong one builds `…name??force=true` or + // `…nameforce=true`. This value carries its `?`; the distinct name + // says so without the reader having to go and look. + const query = metaSaveQuery(options); + // `type`/`name` stay UNENCODED — a compound name's slash must survive + // so the request reaches `PUT /meta/:type/:section/:name` instead of + // collapsing onto the 3-segment route (pinned in client.test.ts). + const res = await this.fetch(`${this.baseUrl}${route}/${type}/${name}${query}`, { method: 'PUT', body: JSON.stringify(item) }); @@ -5169,9 +5303,26 @@ export class ScopedProjectClient { const res = await this.parent._fetch(this.url(`/meta/${type}/${name}${qs ? `?${qs}` : ''}`)); return this.parent._unwrap(res); }, - /** Carries the ADR-0008 OCC token in `version` — see the unscoped twin. */ - saveItem: async (type: string, name: string, item: any): Promise => { - const res = await this.parent._fetch(this.url(`/meta/${type}/${name}`), { + /** + * Carries the ADR-0008 OCC token in `version` — see the unscoped twin. + * + * `options` is the SAME {@link SaveMetaItemOptions} bag the unscoped twin + * takes, and reaches the SAME handler: the scoped mount is not a second + * implementation, it is one `registerForBase` call replayed against + * `/environments/:environmentId` (see `RestServer`), so this door reads + * `?force` / `?package` / `?mode` byte-identically. A bag on only one of + * the two clients would be a fresh divergence of the kind #7019 rules + * against, not half a fix. + */ + saveItem: async ( + type: string, + name: string, + item: any, + options?: SaveMetaItemOptions, + ): Promise => { + // `query`, not `qs` — it carries its own `?`; see the unscoped twin. + const query = metaSaveQuery(options); + const res = await this.parent._fetch(this.url(`/meta/${type}/${name}${query}`), { method: 'PUT', body: JSON.stringify(item), });