Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions .changeset/21841-import-refusal-working-remedy.md
Original file line number Diff line number Diff line change
@@ -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`.
2 changes: 1 addition & 1 deletion content/docs/references/api/protocol.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -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=<id>` 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. |


---
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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.";

Expand Down Expand Up @@ -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);
});
});
42 changes: 41 additions & 1 deletion packages/metadata-protocol/src/protocol.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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.';
}
Expand Down
Loading
Loading