diff --git a/.changeset/21841-import-refusal-working-remedy.md b/.changeset/21841-import-refusal-working-remedy.md new file mode 100644 index 00000000000..f77c825104d --- /dev/null +++ b/.changeset/21841-import-refusal-working-remedy.md @@ -0,0 +1,14 @@ +--- +'@objectstack/spec': minor +'@objectstack/metadata-protocol': minor +'@objectstack/service-datasource': patch +--- + +fix(service-datasource): a destructive re-import's refusal names the remedies that work from the import route, instead of a `?force=true` that route never reads (#21841) + +Clause-②: yes (widening) + +- **What was wrong.** "Import as Object" (`POST /api/v1/datasources/:name/external/tables/:remote/import`) saves through the metadata door's own `saveMetaItem`. A re-import that would drop or retype a field the stored object still carries is refused by that save's destructive-change gate, and the import route relays the refusal as `400 EXTERNAL_IMPORT_ERROR`. The refusal ended `re-submit with ?force=true to proceed.` The import route reads no `force`, so a caller who did exactly that got the identical refusal back. +- **What the refusal says now.** The import states its own write face, and the refusal ends: this import cannot be forced, because the external-table import route accepts no `force`. Import the table under a new `name`, or save the changed definition through `PUT /api/v1/meta/object/:name?force=true`, which accepts the destructive change on purpose. Both remedies are measured on the showcase: each one answers `201` or `200` where the re-import answered `400`. The same words appear in this package's earlier changeset for the import. +- **What widens.** `SaveMetaItemRequestSchema.writeFace` (`@objectstack/spec`) and `saveMetaItem`'s `writeFace` parameter (`@objectstack/metadata-protocol`) gain one member, `'external-import'`. The member is stated by the server. No door reads it from a request body, and the import's own options cannot carry it, or a `force`, into the save. Nothing accepted today is refused. +- **What does not change.** The refusal itself stays: a destructive re-import is still `400 EXTERNAL_IMPORT_ERROR`, and the stored definition does not move. The import route gains no `force`. Acknowledging a destructive change stays on the metadata door. The other faces' wording is unchanged. A `422 INVALID_METADATA` relayed by the import keeps its full findings in the message, because the import route's envelope carries no `issues`. diff --git a/content/docs/references/api/protocol.mdx b/content/docs/references/api/protocol.mdx index e36b4da1125..935f0ca8af0 100644 --- a/content/docs/references/api/protocol.mdx +++ b/content/docs/references/api/protocol.mdx @@ -2588,7 +2588,7 @@ Installed package with runtime lifecycle state | **force** | `boolean` | optional | Destructive-change acknowledgement (`?force=true` on the REST door): skips the safety diff that refuses an `object` save whose body drops fields or narrows types the stored item still carries (409 with the findings otherwise). Only `object` saves reach that diff, so the flag is inert for every other type. Absent = the guard runs. | | **mode** | `Enum<'draft' \| 'publish'>` | optional | Per-item lifecycle (ADR-0005 drafts): `draft` stages the body as a pending draft overlay (`?mode=draft` on the REST door; the publish door promotes it later); `publish` or ABSENT writes straight to the live `active` row — the legacy default, kept so callers that predate the draft/publish split keep working. Any value other than `draft` is read as `publish`. | | **packageId** | `string \| null` | optional | ADR-0048 — the software package to bind the saved row to (`sys_metadata.package_id`; `?package=` on the REST door, sent only when it names a real package). Set when authoring inside a Studio package workspace; a named read-only base package is refused. On create the row is stamped with this id; on update an existing binding is preserved, never silently re-bound. Absent = env-local overlay (no package stamp); it also scopes which row the unpinned parent-version resolution reads. | -| **writeFace** | `Enum<'package-duplicate' \| 'meta-envelope' \| 'meta-dispatch'>` | optional | Which write door a refusal is being rendered FOR — stated by the SERVER, never by a remote caller: every door builds this request field by field and never spreads a request body into it, so there is no path for a client to smuggle a face in, and a face arriving in a wire body is simply never read. Two refusals branch on it, for different questions: the 409 destructive-change remedy names the acknowledgement mechanism that actually exists on the refusing door (`?force=true` on the REST doors; the dispatcher and the duplicate-package door have none), and the 422 invalid-metadata message adapts to whether a structured `issues[]` channel reaches the consumer beside it. Absent = the conservative default wording. | +| **writeFace** | `Enum<'package-duplicate' \| 'meta-envelope' \| 'meta-dispatch' \| 'external-import'>` | optional | Which write door a refusal is being rendered FOR — stated by the SERVER, never by a remote caller: every door builds this request field by field and never spreads a request body into it, so there is no path for a client to smuggle a face in, and a face arriving in a wire body is simply never read. Two refusals branch on it, for different questions: the 409 destructive-change remedy names the acknowledgement mechanism that actually exists on the refusing door (`?force=true` on the REST doors; the dispatcher, the duplicate-package door and the external-table import have none), and the 422 invalid-metadata message adapts to whether a structured `issues[]` channel reaches the consumer beside it. Absent = the conservative default wording. | --- diff --git a/packages/metadata-protocol/src/protocol.destructive-409-face-inventory.test.ts b/packages/metadata-protocol/src/protocol.destructive-409-face-inventory.test.ts index a86564b0a82..ca73c19dfe4 100644 --- a/packages/metadata-protocol/src/protocol.destructive-409-face-inventory.test.ts +++ b/packages/metadata-protocol/src/protocol.destructive-409-face-inventory.test.ts @@ -49,8 +49,9 @@ * | 5 | `migrateStoredMetadata` (this file's protocol) | any | **true** | no — `force` | (`rows[].reason`) | n/a | * | 6 | {@link ObjectStackProtocolImplementation.duplicatePackage} | `row.type` incl. `object` | no | **yes** | `failed[].error` on a **200** | ⛔ **NO — sole carrier** | * | 7 | `plugin-security` permission-set projection ×4 | `'permission'` | no | no — type | n/a | n/a | + * | 8 | `@objectstack/service-datasource` external-table import (`persistObject`) | `'object'` | never — the import route reads no `force` | **yes** | the import route's `400 EXTERNAL_IMPORT_ERROR`, message only | ⛔ **NO — sole carrier** | * - * Rows 1-3 and 6 are pinned below. Rows 4, 5 and 7 are eliminated by a + * Rows 1-3, 6 and 8 are pinned below. Rows 4, 5 and 7 are eliminated by a * constant in the call itself (a literal `type`, or `force: true`), which is * why they are argued rather than pinned: there is no runtime state that could * make them reach the gate. @@ -159,6 +160,25 @@ * by giving the dispatcher a `force` or by taking row 2's back out — is undoing * a decision, not tidying an inconsistency. * + * ## [#21841] Row 8 — the external-table import, a face of its own + * + * "Import as Object" saves through this very `saveMetaItem` since #21788, so a + * re-import that would shrink an object it already created reaches the gate. + * The import route relays every throw as `400 EXTERNAL_IMPORT_ERROR` with the + * message alone, and it reads no `force`, so the default clause sent a caller + * round in a circle exactly as rows 2, 3 and 6 once did. It went row 3's way, + * not row 2's: the route has no twin that reads `?force`, and no first-party + * caller re-imports through it (the console's import dialog saves its draft + * through `PUT /meta/object/:name`), so threading a `force` would be a new + * capability with no pull. `'external-import'` names the two remedies that + * exist instead — a new `name`, or the metadata door with `?force=true` — and + * section 6 below pins it. + * + * Row 8 is also a second SOLE CARRIER beside row 6: the route's envelope has + * no `issues`, so the findings prose reaches the caller through the message + * alone. That is one more reason the commit 809e61221 verdict stands, and it is + * why `'external-import'` keeps the 422's default, full-prose clause. + * * ⛔ Never a bare `toThrow()` here. `duplicatePackage` does not throw, it * REPORTS, and what the report says IS the defect; and for the throw itself * the minimum assertion is `code` + `status` (ADR-0112 envelope), with the @@ -308,6 +328,14 @@ const DUPLICATE_REMEDY_HEAD = 'this copy cannot be forced'; * See section 5. */ const DISPATCH_REMEDY_HEAD = 'this save cannot be forced'; +/** + * [#21841] …and as the external-table IMPORT renders it — the fourth answer. + * Unlike the dispatcher, a door that DOES read `?force` exists for this very + * object, so the clause sends the caller there by name. See section 6. + */ +const IMPORT_REMEDY_HEAD = 'this import cannot be forced'; +/** The metadata door the import face prescribes, spelled for the fixture's item. */ +const IMPORT_REMEDY_DOOR = 'PUT /api/v1/meta/object/crm_task?force=true'; /** One finding's prose, as `detectDestructiveObjectChanges` words it. */ const FINDING_PROSE = "Field 'b' removed — existing data in this column will become inaccessible."; @@ -689,3 +717,110 @@ describe('[#11095] [GUARD] the `meta-dispatch` face prescribes a remedy that doo for (const i of dispatch.issues) expect(dispatch.message).not.toContain(i.message); }); }); + +// ═══════════════════════════════════════════════════════════════════════════ +// 6. [#21841] [GUARD] Inventory row 8 — the external-table import face, which +// names the door that DOES read `force` for this object +// ═══════════════════════════════════════════════════════════════════════════ + +describe('[#21841] [GUARD] the `external-import` face prescribes remedies that exist', () => { + it('declares the ADR-0112 envelope — the refusal itself is unchanged', async () => { + const err = await destructiveRefusal('external-import'); + + // This card moves a SENTENCE. The import route restamps the envelope as + // `400 EXTERNAL_IMPORT_ERROR`, but what it relays is this refusal, so the + // producer's own code, status and findings must not move under it. + expect(err.code).toBe('DESTRUCTIVE_CHANGE'); + expect(err.status).toBe(409); + expect(err.issues).toEqual(expect.arrayContaining([ + expect.objectContaining({ code: 'field_removed', field: 'b' }), + ])); + }); + + it('⛔ never tells the caller to re-submit the import with `?force=true`', async () => { + const err = await destructiveRefusal('external-import'); + + // The defect, stated as the assertion that fails without the fix: the + // import route reads no `force`, so this sentence sent a caller round. + expect(err.message).not.toContain(PUT_REMEDY); + expect(err.message).not.toContain('re-submit with ?force=true'); + }); + + it('names the door, denies the mechanism, then prescribes the two remedies that exist', async () => { + const err = await destructiveRefusal('external-import'); + + expect(err.message).toContain(IMPORT_REMEDY_HEAD); + expect(err.message).toContain('accepts no `force`'); + // Remedy one: the import itself, under a name nothing stores yet. + expect(err.message).toContain('under a new `name`'); + // Remedy two: the metadata door, which reads `?force` for this item. + expect(err.message).toContain(IMPORT_REMEDY_DOOR); + }); + + it('[#10886 non-effect] the per-field findings prose is still there, untrimmed', async () => { + const err = await destructiveRefusal('external-import'); + + // Row 8 is a sole carrier (the route relays the message alone), so the + // findings must stay in the sentence on this face above all. + expect(err.message).toContain(FINDING_PROSE); + expect(err.message).toContain('would drop or transform existing data'); + }); + + it('⛔ the four faces are a SWITCH — the new one moved none of the others', async () => { + const [plain, envelope, dispatch, imported] = await Promise.all([ + destructiveRefusal(), + destructiveRefusal('meta-envelope'), + destructiveRefusal('meta-dispatch'), + destructiveRefusal('external-import'), + ]); + + expect(plain.message).toContain(PUT_REMEDY); + expect(envelope.message).toContain(PUT_REMEDY); + expect(dispatch.message).toContain(DISPATCH_REMEDY_HEAD); + for (const other of [plain, envelope, dispatch]) { + expect(other.message).not.toContain(IMPORT_REMEDY_HEAD); + } + expect(imported.message).not.toContain(DISPATCH_REMEDY_HEAD); + expect(imported.message).not.toContain(DUPLICATE_REMEDY_HEAD); + }); + + /** + * ⭐ The coupling, from the other side of section 5's. + * + * `'meta-dispatch'` had to JOIN the 422's trimming case because its door + * carries `issues[]`. The import route does not: it answers + * `sendError(res, 400, 'EXTERNAL_IMPORT_ERROR', message)` and nothing else, + * so the 422's findings reach that caller through the message alone. The + * import face must therefore stay OUT of the trimming case and keep the + * full-prose default. Adding it there would delete the author's findings + * from that wire with every 409 assertion above still green. + */ + it('⛔ [COUPLING] the new face changes the 409 clause and NOTHING about the 422', async () => { + const { protocol } = makeKernel({ seed: [objectRow('crm_task', ['a', 'b', 'c', 'd'])] }); + const invalid = async (writeFace?: string) => { + try { + await protocol.saveMetaItem({ + type: 'view', + name: 'task_list', + item: { + name: 'task_list', object: 'task', type: 'list', label: 'Tasks', + columns: [{ field: 'title', summary: { type: 'sum', fieldd: 'amount' } }], + }, + ...(writeFace ? { writeFace } : {}), + }); + } catch (e: any) { return e; } + throw new Error('expected saveMetaItem to refuse the invalid body'); + }; + + const plain = await invalid(); + const imported = await invalid('external-import'); + + expect(imported.code).toBe('INVALID_METADATA'); + expect(imported.status).toBe(422); + // Byte-for-byte the default clause, which restates the findings (its + // first three; the fixture has fewer) in the sentence itself. + expect(imported.message).toBe(plain.message); + expect(imported.issues.length).toBeGreaterThan(0); + for (const i of imported.issues.slice(0, 3)) expect(imported.message).toContain(i.message); + }); +}); diff --git a/packages/metadata-protocol/src/protocol.ts b/packages/metadata-protocol/src/protocol.ts index 182c66778c7..4e881bafc69 100644 --- a/packages/metadata-protocol/src/protocol.ts +++ b/packages/metadata-protocol/src/protocol.ts @@ -4801,6 +4801,24 @@ function detectDestructiveObjectChanges(prev: any, next: any): Array<{ * field. The compound door's parity argument is a statement about ONE pair of * routes that spell one name two ways; it is not a general licence, and the * dispatcher is not the third member of that pair. + * + * ## [#21841] The external-table import, and why it went the dispatcher's way + * + * "Import as Object" (`@objectstack/service-datasource`) saves through this + * gate since #21788, so a re-import that would shrink an object it created is + * refused here and relayed as `400 EXTERNAL_IMPORT_ERROR`. The import route + * reads no `force`, so the default clause sent that caller round in a circle. + * It states `'external-import'` and gets a clause of its own rather than a + * `force`, for the reasons the dispatcher did and one more: the route has no + * twin that reads `?force`, and no first-party caller re-imports through it + * (the console's import dialog saves its draft through + * `PUT /api/v1/meta/object/:name`, the very door this clause names), so a + * `force` there would be a new capability with nothing pulling on it. Unlike + * the dispatcher, a door that DOES read `?force` exists for the same item, so + * the clause names it instead of only telling the caller to reconcile. + * + * ⛔ Same warning as above: the import is not a twin of the `PUT` door, and + * giving it a `force` to "match" would be a new surface, not a repair. */ /** * [commit 82cb6e849 / commit d806081dd] Which write door a `saveMetaItem` refusal is being @@ -4833,8 +4851,16 @@ function detectDestructiveObjectChanges(prev: any, next: any): Array<{ * case rather than a hypothetical — which is why {@link specValidationFindings} * lists the two faces on one `case` instead of letting `'meta-dispatch'` fall * to a default that was never written for it. + * + * ⚠️ [#21841] `'external-import'` differs from `'meta-dispatch'` on that SAME + * second question, in the other direction: the import route relays a refusal + * as `sendError(res, 400, 'EXTERNAL_IMPORT_ERROR', message)` and nothing else, + * so no `issues[]` reaches its caller and the message is the sole carrier of + * the findings. It therefore stays OUT of {@link specValidationFindings}' + * trimming case and keeps the full-prose default — the polarity that comment + * defends, working as designed for a door added after it. */ -type MetadataWriteFace = 'package-duplicate' | 'meta-envelope' | 'meta-dispatch'; +type MetadataWriteFace = 'package-duplicate' | 'meta-envelope' | 'meta-dispatch' | 'external-import'; function destructiveChangeRemedy( face: MetadataWriteFace | undefined, @@ -4865,6 +4891,20 @@ function destructiveChangeRemedy( return `this save cannot be forced: the dispatcher's \`PUT /meta\` accepts no \`force\`. ` + `Re-submit '${name}' with a body that keeps the fields and types named above, ` + `or reconcile that stored item first.`; + case 'external-import': + // [#21841] "Import as Object" (`@objectstack/service-datasource`), + // relayed by the import route as `400 EXTERNAL_IMPORT_ERROR`. That + // route reads no `force` and was deliberately not given one (see the + // section above `MetadataWriteFace`). Both remedies are things the + // caller can do from where they stand: import the table under a + // name nothing stores yet, or save the changed definition through + // the metadata door, which reads `?force=true` for this very item + // and is where a destructive change is acknowledged on purpose. + // Same grammar as the two faces above — name the door, deny the + // mechanism, then prescribe. + return `this import cannot be forced: the external-table import route accepts no \`force\`. ` + + `Import the table under a new \`name\`, or save the changed definition of '${name}' ` + + `through \`PUT /api/v1/meta/object/${name}?force=true\`, which accepts the destructive change on purpose.`; default: return 're-submit with ?force=true to proceed.'; } diff --git a/packages/qa/dogfood/test/external-import-destructive-remedy.dogfood.test.ts b/packages/qa/dogfood/test/external-import-destructive-remedy.dogfood.test.ts new file mode 100644 index 00000000000..eb76927d9ec --- /dev/null +++ b/packages/qa/dogfood/test/external-import-destructive-remedy.dogfood.test.ts @@ -0,0 +1,184 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// [#21841] A destructive re-import's refusal prescribes a remedy that works +// from where the caller stands, over the real showcase composition. +// +// ## What was broken +// +// "Import as Object" (`POST /datasources/:name/external/tables/:remote/import`) +// saves through the metadata door's own `saveMetaItem`, so a re-import that +// would drop a field the stored object still carries is refused by that +// door's destructive-change gate, and the import route relays the refusal as +// `400 EXTERNAL_IMPORT_ERROR`. The refusal ended "re-submit with ?force=true to +// proceed." The import route reads no `force`, so a caller who did exactly +// that got the identical refusal back. +// +// The import now states its own write face, and the refusal names the two +// remedies that exist: import the table under a new `name`, or save the changed +// definition through the metadata door, `PUT /api/v1/meta/object/:name?force=true`, +// which accepts the destructive change on purpose. The refusal itself stays: +// nothing here overwrites a stored definition silently. +// +// ## What each case pins +// +// Every remedy the refusal names is FOLLOWED here, and each reaches a +// different answer than the refusal. The one it no longer names (`?force=true` +// on the import route) is followed too, and still answers the refusal: that is +// the fact the refusal's denial states. +// +// The verify harness does not mount the federation service, so this file +// mounts `ExternalDatasourceServicePlugin` itself, as `objectstack dev` and +// `serve` do. The working directory is a temporary one because the showcase's +// external datasource and its fixture both name a cwd-relative SQLite file. + +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import showcaseStack, { onEnable } from '@objectstack/example-showcase'; +import { ExternalDatasourceServicePlugin } from '@objectstack/service-datasource'; +import { bootStack, type VerifyStack } from '@objectstack/verify'; +import { mkdtempSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; + +/** The showcase's external datasource and its `customers` table (`external-fixture.ts`). */ +const DATASOURCE = 'showcase_external'; +const REMOTE = 'customers'; +/** The object the first import creates and the re-import would shrink. */ +const NAME = 'dogfood_ext_cust_21841'; +/** The remedy's "new `name`". */ +const NEW_NAME = 'dogfood_ext_cust_21841_v2'; +/** The column the re-import leaves out, so the stored object would lose its field. */ +const DROPPED = 'region'; +/** The re-import's options: the same table, one column fewer. */ +const SHRINKING = { name: NAME, excludeColumns: [DROPPED] }; + +const importPath = `/datasources/${DATASOURCE}/external/tables/${REMOTE}/import`; +const draftPath = `/datasources/${DATASOURCE}/external/tables/${REMOTE}/draft`; + +interface Envelope { + success?: boolean; + error?: { code?: string; message?: string }; + item?: { fields?: Record }; + data?: { draft?: { definition?: Record } }; + records?: Array>; +} + +describe('a destructive re-import is refused with a remedy that works (showcase)', () => { + let stack: VerifyStack; + let token: string; + let prevCwd: string; + let dir: string; + /** The stored definition right after the first import, before any refusal. */ + let before: Envelope['item']; + + const call = async (method: string, path: string, body?: unknown) => { + const res = await stack.apiAs(token, method, path, body); + const json = (await res.json().catch(() => ({}))) as Envelope; + return { status: res.status, json }; + }; + + const stored = async () => { + const read = await call('GET', `/meta/object/${NAME}`); + expect(read.status, JSON.stringify(read.json)).toBe(200); + return read.json.item; + }; + + /** The destructive re-import, asserted refused with the envelope it has always had. */ + const refusedReimport = async (query = '') => { + const refused = await call('POST', `${importPath}${query}`, SHRINKING); + expect(refused.status, JSON.stringify(refused.json)).toBe(400); + expect(refused.json.error?.code).toBe('EXTERNAL_IMPORT_ERROR'); + expect(refused.json.error?.message).toContain('would drop or transform existing data'); + return refused.json.error?.message ?? ''; + }; + + beforeAll(async () => { + prevCwd = process.cwd(); + dir = mkdtempSync(join(tmpdir(), 'dogfood-21841-')); + process.chdir(dir); + // Provision the remote tables, exactly as `os dev` does at boot. + await onEnable({ logger: { info() {}, warn() {} } } as never); + stack = await bootStack(showcaseStack, { + databaseFile: join(dir, 'showcase.db'), + extraPlugins: [new ExternalDatasourceServicePlugin()], + }); + token = await stack.signIn(); + + const first = await call('POST', importPath, { name: NAME }); + expect(first.status, JSON.stringify(first.json)).toBe(201); + before = await stored(); + // Premise: the field the re-import would drop is really on the stored object. + expect(Object.keys(before?.fields ?? {})).toContain(DROPPED); + }, 180_000); + + afterAll(async () => { + await stack?.stop(); + if (prevCwd) process.chdir(prevCwd); + if (dir) rmSync(dir, { recursive: true, force: true }); + }); + + it('the destructive re-import is still refused, and the stored definition does not move', async () => { + const message = await refusedReimport(); + + expect(message).toContain(`'${DROPPED}' removed`); + expect(await stored()).toEqual(before); + }); + + it('⛔ the refusal does not prescribe `?force=true` on the import route, which reads none', async () => { + const message = await refusedReimport(); + + // The prescription this refusal used to end with, followed: the import + // with `?force=true` answers the identical refusal and writes nothing. + // That is the fact the refusal's denial now states. + const followed = await refusedReimport('?force=true'); + expect(followed).toBe(message); + expect(await stored()).toEqual(before); + + expect(message).not.toContain('re-submit with ?force=true'); + expect(message).toContain('accepts no `force`'); + }); + + it('following the first remedy, an import under a new `name`, answers 201 and serves the rows', async () => { + const message = await refusedReimport(); + expect(message).toContain('under a new `name`'); + + const renamed = await call('POST', importPath, { ...SHRINKING, name: NEW_NAME }); + expect(renamed.status, JSON.stringify(renamed.json)).toBe(201); + + const rows = await call('GET', `/data/${NEW_NAME}`); + expect(rows.status, JSON.stringify(rows.json)).toBe(200); + expect(rows.json.records).toHaveLength(3); + // …and the object it was refused for is untouched by it. + expect(await stored()).toEqual(before); + }); + + it('following the second remedy, `PUT /api/v1/meta/object/:name?force=true`, saves the change', async () => { + const message = await refusedReimport(); + expect(message).toContain(`PUT /api/v1/meta/object/${NAME}?force=true`); + + // The changed definition, from the import's own draft step with the same + // options: the definition the refused re-import would have saved. + const draft = await call('POST', draftPath, SHRINKING); + expect(draft.status, JSON.stringify(draft.json)).toBe(200); + const drafted = draft.json.data?.draft?.definition; + // Premise: the draft really is the shrunk definition, not an empty body. + expect(Object.keys((drafted?.fields ?? {}) as object)).toEqual(expect.arrayContaining(['name', 'email'])); + expect(Object.keys((drafted?.fields ?? {}) as object)).not.toContain(DROPPED); + const definition = { ...drafted, name: NAME }; + + // Control: on that door it is `force` that lifts the refusal. + const unforced = await call('PUT', `/meta/object/${NAME}`, definition); + expect(unforced.status, JSON.stringify(unforced.json)).toBe(409); + expect(JSON.stringify(unforced.json)).toContain('DESTRUCTIVE_CHANGE'); + expect(await stored()).toEqual(before); + + const forced = await call('PUT', `/meta/object/${NAME}?force=true`, definition); + expect(forced.status, JSON.stringify(forced.json)).toBe(200); + + const after = await stored(); + expect(Object.keys(after?.fields ?? {})).not.toContain(DROPPED); + expect(Object.keys(after?.fields ?? {})).toEqual(expect.arrayContaining(['name', 'email'])); + const rows = await call('GET', `/data/${NAME}`); + expect(rows.status, JSON.stringify(rows.json)).toBe(200); + expect(rows.json.records).toHaveLength(3); + }); +}); diff --git a/packages/services/service-datasource/src/__tests__/external-import-saves-through-metadata-door.test.ts b/packages/services/service-datasource/src/__tests__/external-import-saves-through-metadata-door.test.ts index 6cb9d1e40ec..0fa52b1a0be 100644 --- a/packages/services/service-datasource/src/__tests__/external-import-saves-through-metadata-door.test.ts +++ b/packages/services/service-datasource/src/__tests__/external-import-saves-through-metadata-door.test.ts @@ -23,6 +23,11 @@ * - the import reaches `saveMetaItem` with the request the metadata door * sends for an `object` (env-wide: `object` is not org-overridable), and * registers through no second path; + * - [#21841] that request states the import's own write face, + * `'external-import'`, so a destructive re-import's refusal prescribes the + * remedies that exist from the import route rather than a `?force=true` it + * never reads; the face is the server's, and an import's options cannot + * carry a `force` or a face of their own into the save; * - the save door is resolved when the import runs, not at `init()` — a * protocol registered after this plugin still receives the save; * - a save the door refuses refuses the import with the door's own error; @@ -99,7 +104,12 @@ describe('importObject saves through the metadata door (#21788)', () => { const result = await (await federation(h)).importObject('warehouse', 'customers', { name: 'ext_cust' }); expect(saveMetaItem).toHaveBeenCalledTimes(1); - expect(saveMetaItem).toHaveBeenCalledWith({ type: 'object', name: 'ext_cust', item: result.definition }); + expect(saveMetaItem).toHaveBeenCalledWith({ + type: 'object', + name: 'ext_cust', + item: result.definition, + writeFace: 'external-import', + }); expect(result.definition).toMatchObject({ name: 'ext_cust', datasource: 'warehouse', @@ -108,6 +118,23 @@ describe('importObject saves through the metadata door (#21788)', () => { expect(h.register).not.toHaveBeenCalled(); }); + it('[#21841] states the face itself: options carrying a `force` or a face reach the save as neither', async () => { + const h = harness(); + const saveMetaItem = vi.fn(async () => ({ success: true })); + h.services.set('protocol', { saveMetaItem }); + + // What a caller can put in the import body. `ImportObjectOpts` declares + // neither key, so they arrive here only as untyped wire input. + const smuggled = { name: 'ext_cust', force: true, writeFace: 'meta-envelope' } as Record; + await (await federation(h)).importObject('warehouse', 'customers', smuggled); + + expect(saveMetaItem).toHaveBeenCalledTimes(1); + const request = (saveMetaItem.mock.calls[0] as unknown[])[0] as Record; + expect(request.writeFace).toBe('external-import'); + expect('force' in request).toBe(false); + expect(Object.keys(request).sort()).toEqual(['item', 'name', 'type', 'writeFace']); + }); + it('resolves the save door when the import runs, so a protocol registered after init still receives it', async () => { const h = harness(); const service = await federation(h); diff --git a/packages/services/service-datasource/src/plugin.ts b/packages/services/service-datasource/src/plugin.ts index d1d6a114886..b0385ebd52c 100644 --- a/packages/services/service-datasource/src/plugin.ts +++ b/packages/services/service-datasource/src/plugin.ts @@ -134,6 +134,17 @@ export class ExternalDatasourceServicePlugin implements Plugin { * `packageId`, `mode` or `force`, because the import route takes no * `?package`, `?mode` or `?force`. * + * [#21841] …and one field that door does not send: the import's own + * `writeFace`, stated here by the server and never read from the + * caller's options. A re-import that would drop or retype a field the + * stored object still carries is refused by the save's destructive-change + * gate, and the import route relays that refusal. Without a face the + * refusal ended "re-submit with ?force=true", a parameter this route does + * not read; `'external-import'` makes it name the remedies that exist + * from here (a new `name`, or `PUT /api/v1/meta/object/:name?force=true`). + * ⛔ Not a `force`: the refusal stays, and acknowledging a destructive + * change stays on the metadata door. + * * A GETTER, so the save door is asked for when an import runs: the * service reads this slot before the draft and refuses with its own * "requires a writable metadata store" when it is absent — before any @@ -145,7 +156,7 @@ export class ExternalDatasourceServicePlugin implements Plugin { const door = metadataSaveDoor(); if (!door) return undefined; return async (name: string, definition: Record) => { - await door.saveMetaItem({ type: 'object', name, item: definition }); + await door.saveMetaItem({ type: 'object', name, item: definition, writeFace: 'external-import' }); }; }, /** diff --git a/packages/spec/src/api/protocol.test.ts b/packages/spec/src/api/protocol.test.ts index 185579d64f6..94f195fd920 100644 --- a/packages/spec/src/api/protocol.test.ts +++ b/packages/spec/src/api/protocol.test.ts @@ -2462,7 +2462,7 @@ describe('SaveMetaItemRequestSchema declares the contract members the save door expect(SaveMetaItemRequestSchema.safeParse({ ...base, mode: 'draft' }).success).toBe(true); expect(SaveMetaItemRequestSchema.safeParse({ ...base, mode: 'publish' }).success).toBe(true); expect(SaveMetaItemRequestSchema.safeParse({ ...base, mode: 'stage' }).success).toBe(false); - for (const face of ['package-duplicate', 'meta-envelope', 'meta-dispatch'] as const) { + for (const face of ['package-duplicate', 'meta-envelope', 'meta-dispatch', 'external-import'] as const) { expect(SaveMetaItemRequestSchema.safeParse({ ...base, writeFace: face }).success).toBe(true); } expect(SaveMetaItemRequestSchema.safeParse({ ...base, writeFace: 'rest' }).success).toBe(false); diff --git a/packages/spec/src/api/protocol.zod.ts b/packages/spec/src/api/protocol.zod.ts index 14f88292fc4..b3cbec54dbd 100644 --- a/packages/spec/src/api/protocol.zod.ts +++ b/packages/spec/src/api/protocol.zod.ts @@ -623,8 +623,9 @@ export const RuntimeAuthoringIssueSchema = lazySchema(() => z.object({ * channel the REST layer deliberately never reads. * * `writeFace` IS declared, and the distinction with `source` is the point: - * both are server-stated, but `writeFace` is sent by two doors and the - * duplicate-package internal call — three real producers on this parameter + * both are server-stated, but `writeFace` is sent by three doors (the two + * metadata write doors and the external-table import) and the + * duplicate-package internal call — four real producers on this parameter * — and the implementation branches its refusal envelopes on it. See the * member's own doc for why declaring it does not make it client-authorable. */ @@ -691,7 +692,7 @@ export const SaveMetaItemRequestSchema = lazySchema(() => z.object({ + 'overlay (no package stamp); it also scopes which row the unpinned ' + 'parent-version resolution reads.', ), - writeFace: z.enum(['package-duplicate', 'meta-envelope', 'meta-dispatch']).optional().describe( + writeFace: z.enum(['package-duplicate', 'meta-envelope', 'meta-dispatch', 'external-import']).optional().describe( 'Which write door a refusal is being rendered FOR — stated by the ' + 'SERVER, never by a remote caller: every door builds this request ' + 'field by field and never spreads a request body into it, so there is ' @@ -699,8 +700,9 @@ export const SaveMetaItemRequestSchema = lazySchema(() => z.object({ + 'wire body is simply never read. Two refusals branch on it, for ' + 'different questions: the 409 destructive-change remedy names the ' + 'acknowledgement mechanism that actually exists on the refusing door ' - + '(`?force=true` on the REST doors; the dispatcher and the ' - + 'duplicate-package door have none), and the 422 invalid-metadata ' + + '(`?force=true` on the REST doors; the dispatcher, the ' + + 'duplicate-package door and the external-table import have none), ' + + 'and the 422 invalid-metadata ' + 'message adapts to whether a structured `issues[]` channel reaches ' + 'the consumer beside it. Absent = the conservative default wording.', ),