Skip to content

Commit c6f2374

Browse files
committed
Merge remote-tracking branch 'origin/main' into claude/issue-21876-datasource-metadata-read-at-use
2 parents b6b5e91 + e864db5 commit c6f2374

16 files changed

Lines changed: 1006 additions & 38 deletions
Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
---
2+
'@objectstack/spec': minor
3+
'@objectstack/metadata-protocol': minor
4+
'@objectstack/service-datasource': patch
5+
---
6+
7+
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)
8+
9+
Clause-②: yes (widening)
10+
11+
- **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.
12+
- **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.
13+
- **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.
14+
- **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`.
Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
---
2+
'@objectstack/plugin-security': patch
3+
---
4+
5+
Discard Overlay no longer deletes the stored definition of a permission set saved into a writable runtime package
6+
7+
Clause-②: no
8+
9+
The Discard Overlay action (`POST /api/v1/security/permission-sets/:id/discard-overlay`) is declared to refuse any set that is not package-declared, so that it can never destroy a set the environment authored. It decided that by asking whether any engine-registry item of the set's name carried a package id. The registry also holds the stored definition rows, and a metadata list read (`GET /api/v1/meta/permission`, which every Studio page load issues) stamps a stored row's package binding onto it. A set saved into a writable runtime package (`PUT /api/v1/meta/permission/:name?package=<id>`) therefore passed the check after the first list read: the action answered `200` and deleted the set's only `sys_metadata` row. It now asks the same classifier the packaged-permission-set lock's write doors ask, and refuses every set that classifier does not judge shipped by a code package (including when it cannot decide), with the action's existing `403 PERMISSION_DENIED`.
10+
11+
The drift diagnostics behind the record's `drift_status` / `drift_detail` (whose `overlay_shadow` detail names Discard Overlay as the remedy) read the package id the same way. They now judge the same population: only sets a code package ships.
12+
13+
Unchanged: a set a code package ships still has its overlay discarded and its record resynced to the shipped artifact, its drift is still reported as before, and no error code, route or field moves. The refusal's message now says the set is not shipped by any installed code package.

‎content/docs/permissions/permission-sets.mdx‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -365,9 +365,10 @@ either can be live on one row, and they need different remedies:
365365
the package ships next. **Remedy:** the record's **Discard Overlay** Setup
366366
action (`POST /api/v1/security/permission-sets/:id/discard-overlay`,
367367
tenant-admin only) removes the stale overlay and resyncs the record to the
368-
current artifact immediately. It refuses on any set that is not currently
369-
package-declared, so it can never destroy a genuinely environment-authored
370-
set.
368+
current artifact immediately. It refuses on any set that no installed code
369+
package ships (a set created in this environment, a clone, or a set saved
370+
into a writable runtime package), so it can never destroy a genuinely
371+
environment-authored set.
371372
- **Provenance skip.** The record's `managed_by` column predates package
372373
provenance tracking (a legacy insert without `managed_by: 'package'` —
373374
typically a database first initialized on an older release line), so boot

‎content/docs/references/api/protocol.mdx‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2588,7 +2588,7 @@ Installed package with runtime lifecycle state
25882588
| **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. |
25892589
| **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`. |
25902590
| **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. |
2591-
| **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. |
2591+
| **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. |
25922592

25932593

25942594
---

‎packages/metadata-protocol/src/protocol.destructive-409-face-inventory.test.ts‎

Lines changed: 136 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -49,8 +49,9 @@
4949
* | 5 | `migrateStoredMetadata` (this file's protocol) | any | **true** | no — `force` | (`rows[].reason`) | n/a |
5050
* | 6 | {@link ObjectStackProtocolImplementation.duplicatePackage} | `row.type` incl. `object` | no | **yes** | `failed[].error` on a **200** | ⛔ **NO — sole carrier** |
5151
* | 7 | `plugin-security` permission-set projection ×4 | `'permission'` | no | no — type | n/a | n/a |
52+
* | 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** |
5253
*
53-
* Rows 1-3 and 6 are pinned below. Rows 4, 5 and 7 are eliminated by a
54+
* Rows 1-3, 6 and 8 are pinned below. Rows 4, 5 and 7 are eliminated by a
5455
* constant in the call itself (a literal `type`, or `force: true`), which is
5556
* why they are argued rather than pinned: there is no runtime state that could
5657
* make them reach the gate.
@@ -159,6 +160,25 @@
159160
* by giving the dispatcher a `force` or by taking row 2's back out — is undoing
160161
* a decision, not tidying an inconsistency.
161162
*
163+
* ## [#21841] Row 8 — the external-table import, a face of its own
164+
*
165+
* "Import as Object" saves through this very `saveMetaItem` since #21788, so a
166+
* re-import that would shrink an object it already created reaches the gate.
167+
* The import route relays every throw as `400 EXTERNAL_IMPORT_ERROR` with the
168+
* message alone, and it reads no `force`, so the default clause sent a caller
169+
* round in a circle exactly as rows 2, 3 and 6 once did. It went row 3's way,
170+
* not row 2's: the route has no twin that reads `?force`, and no first-party
171+
* caller re-imports through it (the console's import dialog saves its draft
172+
* through `PUT /meta/object/:name`), so threading a `force` would be a new
173+
* capability with no pull. `'external-import'` names the two remedies that
174+
* exist instead — a new `name`, or the metadata door with `?force=true` — and
175+
* section 6 below pins it.
176+
*
177+
* Row 8 is also a second SOLE CARRIER beside row 6: the route's envelope has
178+
* no `issues`, so the findings prose reaches the caller through the message
179+
* alone. That is one more reason the commit 809e61221 verdict stands, and it is
180+
* why `'external-import'` keeps the 422's default, full-prose clause.
181+
*
162182
* ⛔ Never a bare `toThrow()` here. `duplicatePackage` does not throw, it
163183
* REPORTS, and what the report says IS the defect; and for the throw itself
164184
* the minimum assertion is `code` + `status` (ADR-0112 envelope), with the
@@ -308,6 +328,14 @@ const DUPLICATE_REMEDY_HEAD = 'this copy cannot be forced';
308328
* See section 5.
309329
*/
310330
const DISPATCH_REMEDY_HEAD = 'this save cannot be forced';
331+
/**
332+
* [#21841] …and as the external-table IMPORT renders it — the fourth answer.
333+
* Unlike the dispatcher, a door that DOES read `?force` exists for this very
334+
* object, so the clause sends the caller there by name. See section 6.
335+
*/
336+
const IMPORT_REMEDY_HEAD = 'this import cannot be forced';
337+
/** The metadata door the import face prescribes, spelled for the fixture's item. */
338+
const IMPORT_REMEDY_DOOR = 'PUT /api/v1/meta/object/crm_task?force=true';
311339
/** One finding's prose, as `detectDestructiveObjectChanges` words it. */
312340
const FINDING_PROSE = "Field 'b' removed — existing data in this column will become inaccessible.";
313341

@@ -689,3 +717,110 @@ describe('[#11095] [GUARD] the `meta-dispatch` face prescribes a remedy that doo
689717
for (const i of dispatch.issues) expect(dispatch.message).not.toContain(i.message);
690718
});
691719
});
720+
721+
// ═══════════════════════════════════════════════════════════════════════════
722+
// 6. [#21841] [GUARD] Inventory row 8 — the external-table import face, which
723+
// names the door that DOES read `force` for this object
724+
// ═══════════════════════════════════════════════════════════════════════════
725+
726+
describe('[#21841] [GUARD] the `external-import` face prescribes remedies that exist', () => {
727+
it('declares the ADR-0112 envelope — the refusal itself is unchanged', async () => {
728+
const err = await destructiveRefusal('external-import');
729+
730+
// This card moves a SENTENCE. The import route restamps the envelope as
731+
// `400 EXTERNAL_IMPORT_ERROR`, but what it relays is this refusal, so the
732+
// producer's own code, status and findings must not move under it.
733+
expect(err.code).toBe('DESTRUCTIVE_CHANGE');
734+
expect(err.status).toBe(409);
735+
expect(err.issues).toEqual(expect.arrayContaining([
736+
expect.objectContaining({ code: 'field_removed', field: 'b' }),
737+
]));
738+
});
739+
740+
it('⛔ never tells the caller to re-submit the import with `?force=true`', async () => {
741+
const err = await destructiveRefusal('external-import');
742+
743+
// The defect, stated as the assertion that fails without the fix: the
744+
// import route reads no `force`, so this sentence sent a caller round.
745+
expect(err.message).not.toContain(PUT_REMEDY);
746+
expect(err.message).not.toContain('re-submit with ?force=true');
747+
});
748+
749+
it('names the door, denies the mechanism, then prescribes the two remedies that exist', async () => {
750+
const err = await destructiveRefusal('external-import');
751+
752+
expect(err.message).toContain(IMPORT_REMEDY_HEAD);
753+
expect(err.message).toContain('accepts no `force`');
754+
// Remedy one: the import itself, under a name nothing stores yet.
755+
expect(err.message).toContain('under a new `name`');
756+
// Remedy two: the metadata door, which reads `?force` for this item.
757+
expect(err.message).toContain(IMPORT_REMEDY_DOOR);
758+
});
759+
760+
it('[#10886 non-effect] the per-field findings prose is still there, untrimmed', async () => {
761+
const err = await destructiveRefusal('external-import');
762+
763+
// Row 8 is a sole carrier (the route relays the message alone), so the
764+
// findings must stay in the sentence on this face above all.
765+
expect(err.message).toContain(FINDING_PROSE);
766+
expect(err.message).toContain('would drop or transform existing data');
767+
});
768+
769+
it('⛔ the four faces are a SWITCH — the new one moved none of the others', async () => {
770+
const [plain, envelope, dispatch, imported] = await Promise.all([
771+
destructiveRefusal(),
772+
destructiveRefusal('meta-envelope'),
773+
destructiveRefusal('meta-dispatch'),
774+
destructiveRefusal('external-import'),
775+
]);
776+
777+
expect(plain.message).toContain(PUT_REMEDY);
778+
expect(envelope.message).toContain(PUT_REMEDY);
779+
expect(dispatch.message).toContain(DISPATCH_REMEDY_HEAD);
780+
for (const other of [plain, envelope, dispatch]) {
781+
expect(other.message).not.toContain(IMPORT_REMEDY_HEAD);
782+
}
783+
expect(imported.message).not.toContain(DISPATCH_REMEDY_HEAD);
784+
expect(imported.message).not.toContain(DUPLICATE_REMEDY_HEAD);
785+
});
786+
787+
/**
788+
* ⭐ The coupling, from the other side of section 5's.
789+
*
790+
* `'meta-dispatch'` had to JOIN the 422's trimming case because its door
791+
* carries `issues[]`. The import route does not: it answers
792+
* `sendError(res, 400, 'EXTERNAL_IMPORT_ERROR', message)` and nothing else,
793+
* so the 422's findings reach that caller through the message alone. The
794+
* import face must therefore stay OUT of the trimming case and keep the
795+
* full-prose default. Adding it there would delete the author's findings
796+
* from that wire with every 409 assertion above still green.
797+
*/
798+
it('⛔ [COUPLING] the new face changes the 409 clause and NOTHING about the 422', async () => {
799+
const { protocol } = makeKernel({ seed: [objectRow('crm_task', ['a', 'b', 'c', 'd'])] });
800+
const invalid = async (writeFace?: string) => {
801+
try {
802+
await protocol.saveMetaItem({
803+
type: 'view',
804+
name: 'task_list',
805+
item: {
806+
name: 'task_list', object: 'task', type: 'list', label: 'Tasks',
807+
columns: [{ field: 'title', summary: { type: 'sum', fieldd: 'amount' } }],
808+
},
809+
...(writeFace ? { writeFace } : {}),
810+
});
811+
} catch (e: any) { return e; }
812+
throw new Error('expected saveMetaItem to refuse the invalid body');
813+
};
814+
815+
const plain = await invalid();
816+
const imported = await invalid('external-import');
817+
818+
expect(imported.code).toBe('INVALID_METADATA');
819+
expect(imported.status).toBe(422);
820+
// Byte-for-byte the default clause, which restates the findings (its
821+
// first three; the fixture has fewer) in the sentence itself.
822+
expect(imported.message).toBe(plain.message);
823+
expect(imported.issues.length).toBeGreaterThan(0);
824+
for (const i of imported.issues.slice(0, 3)) expect(imported.message).toContain(i.message);
825+
});
826+
});

0 commit comments

Comments
 (0)