diff --git a/.changeset/registry-unregister-object.md b/.changeset/registry-unregister-object.md new file mode 100644 index 0000000000..624aa1719f --- /dev/null +++ b/.changeset/registry-unregister-object.md @@ -0,0 +1,75 @@ +--- +"@objectstack/objectql": patch +"@objectstack/metadata-protocol": patch +--- + +fix(objectql): deleting an `object` really unregisters it — a name-addressed `SchemaRegistry.unregisterObject` (#6808) + +Deleting a runtime-created `object` removed its `sys_metadata` row and left the +object serving. `deleteMetaItem` ends its repository delete with +`restoreArtifactRegistryView` (the #6687 three-tier heal), and every verb that +walk uses — `removeRuntimeShadow`, `registerItem`, `removeOverlayEntry` — +addresses `SchemaRegistry`'s generic `metadata` map. An `object` is written into +**two** places on the way in: + +```ts +registry.registerItem('object', item, 'name'); // metadata map +registry.registerObject({ ...item, _provenance: 'org' }, pkg); // objectContributors +``` + +The heal only undid the first. Measured with the real `SysMetadataRepository` +over an in-memory engine: + +``` +BEFORE delete: metadata['object'] -> ["myapp_invoice"] | objectContributors -> ["myapp_invoice"] +AFTER delete: metadata['object'] -> [] | objectContributors -> ["myapp_invoice"] +registry.getObject('myapp_invoice') -> STILL SERVED +registry.getItem('object','myapp_invoice') -> STILL SERVED (it special-cases back to getObject) +``` + +The surviving half is the load-bearing one. `getObject` is what the data plane +dispatches on (`assertObjectRegistered`, #3770), so the row was gone from +`sys_metadata` while the object stayed resolvable, syncable and **writable** for +the life of the process — a `createData` against the deleted object still +inserted rows. Reachable on the ordinary Studio delete path, and on +`revertCommit`'s soft-remove limb, which #6807 had just wired to the same heal. + +There was no one-line fix because `SchemaRegistry` had no per-name object +removal at all: the only removal verb was `unregisterObjectsByPackage`, which is +addressed by PACKAGE. Routing a single delete through it would mean synthesising +a package identity for a runtime-created object and tearing down every sibling +object registered under it — a far wider blast radius than the delete the +operator asked for. + +So `SchemaRegistry` gains the verb that was missing: + +- **`unregisterObject(name, { force? })`** — removes one object's contributor + entry and the per-object state `registerObject` created (merged-object cache, + `objectRevision`). Names resolve through the same path `getObject` uses, so it + removes precisely the entry that was being served. Package namespaces are left + alone: they are per-package and shared by every object that package ships. +- **The ADR-0029 guard is borrowed, not re-invented.** An object still extended + by another package refuses loudly, naming every extender — the same judgement + `unregisterObjectsByPackage(force)` already encodes, with the address changed + from package to name. Both facts it needs (owner, extenders) were already in + the contributor list, so no new bookkeeping was added. + +`restoreArtifactRegistryView` calls it from **tier 3 only**, and only for a name +that is not artifact-backed — the tier that has already established no lower +layer serves the name. Tiers 1 and 2 concluded a +packaged artifact or a MetadataService baseline still does, and an object that is +still served must stay registered: `assertObjectRegistered` fails CLOSED, so +retiring it there would turn "reset to artifact default" into a data-plane +outage. It also carries the same artifact refusal `removeOverlayEntry` applies +one line up, asked through the protocol's own `isArtifactBacked`: a code-shipped +object is never retired by this walk. That is not already covered by the gates in +front of it — the two-tier delete authorization runs only when `environmentId !== +undefined`, and the no-row leg of a control-plane delete reaches the heal without +touching the repository's `assertAllowed` at all. + +Because the heal runs after the repository delete has committed, an extender +refusal is caught and logged by name rather than propagated (the row is gone +either way) — and deliberately not left to the heal's silent outer `catch`, so a +runtime that disagrees with `sys_metadata` is visible rather than inferred. + +`unregisterObjectsByPackage` keeps its signature and semantics unchanged. diff --git a/packages/metadata-protocol/src/protocol.delete-object-registry-unregister.test.ts b/packages/metadata-protocol/src/protocol.delete-object-registry-unregister.test.ts new file mode 100644 index 0000000000..3301e632c6 --- /dev/null +++ b/packages/metadata-protocol/src/protocol.delete-object-registry-unregister.test.ts @@ -0,0 +1,301 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #6808 — the registry heal's OBJECT limb, pinned at this package's own seam. + * + * `deleteMetaItem` ends its repository delete with `restoreArtifactRegistryView` + * (the #6687 three-tier walk). Every verb that walk used — + * `removeRuntimeShadow`, `registerItem`, `removeOverlayEntry` — addresses + * `SchemaRegistry`'s generic `metadata` map. An `object` is written into TWO + * places on the way in (`registerItem` into that map, `registerObject` into + * `objectContributors`), so the walk retired the LISTING copy and left the + * DISPATCH copy: `registry.getObject(name)` kept serving a deleted object, and + * with it every data-plane write, because `assertObjectRegistered` reads + * exactly that. + * + * This file pins the CONTRACT between the two packages — that the heal calls a + * name-addressed object removal, on the right tier and for the right type, and + * degrades rather than throwing when the registry in hand does not have one. + * The behavioural half (a real `SchemaRegistry`, real repository, both exits + * measured, data CRUD refused afterwards) lives in `@objectstack/objectql`, + * which is where the registry lives: + * `protocol-delete-object-registry-heal.test.ts`. + * + * --------------------------------------------------------------------------- + * Reverse verification, direction predicted BEFORE running + * --------------------------------------------------------------------------- + * Removing the `registry.unregisterObject(name)` call from the heal turns the + * two "calls it" cases in this file red on the call count (`[]` vs + * `['rc9_widget']`) and leaves every negative case green — they assert the call + * is NOT made, which is trivially true with no call site at all. That asymmetry + * is intended: the negatives constrain the SHAPE of the fix (tier and type), + * not its presence. + */ +import { describe, expect, it, vi } from 'vitest'; +// [#5619] The producer's OWN write-verb dispatch decisions (#4550 delete / +// #5480 update). Imported from `@objectstack/metadata-core`, never from +// `@objectstack/objectql`: objectql DEPENDS ON this package, so that import +// would close a dependency cycle turbo rejects outright. +import { assertEngineDeleteDispatch, assertEngineUpdateDispatch } from '@objectstack/metadata-core'; +import { ObjectStackProtocolImplementation } from './protocol.js'; + +interface Row { + id: string; + type: string; + name: string; + organization_id: string | null; + state: string; + checksum: string; + metadata: string; +} + +/** An overlay row as `deleteMetaItem` finds it — a real `checksum` or the OCC */ +/** parent-version check 409s before the heal is ever reached. */ +const overlayRow = (type: string, name: string): Row => ({ + id: `row_${type}_${name}`, + type, + name, + organization_id: null, + state: 'active', + checksum: 'sha256:stored-head', + metadata: JSON.stringify({ name, label: 'Stored' }), +}); + +function makeHarness(opts: { + rows?: Row[]; + /** Tier 1 answers "a packaged artifact is underneath" for these `type|name`s. */ + shadowed?: Array<{ type: string; name: string }>; + /** Simulate an older registry double that has no name-addressed removal. */ + omitUnregisterObject?: boolean; + /** The ADR-0029 refusal, raised by the registry the way the real verb does. */ + unregisterThrows?: Error; + /** + * ADR-0010 `_provenance` of what the registry currently serves for the + * name. `'org'` is what BOTH paths that register a tenant's object stamp + * (the write-through and the boot rehydration); `'package'` is a + * loader-introduced artifact. + */ + servedProvenance?: 'org' | 'package' | null; + /** `environmentId === undefined` — the kernel on which the two-tier delete + * authorization does not run. A FLAG, never an `environmentId` parameter + * with a default: passing `undefined` explicitly to a defaulted parameter + * re-applies the default (#6621). */ + controlPlane?: boolean; +} = {}) { + const rows = [...(opts.rows ?? [])]; + const shadowKeys = new Set((opts.shadowed ?? []).map((s) => `${s.type}|${s.name}`)); + const unregisterObjectCalls: string[] = []; + const removeOverlayEntryCalls: string[] = []; + const matches = (row: Row, where: Record = {}) => + Object.entries(where).every(([k, v]) => (row as any)[k] === v); + + const provenance = opts.servedProvenance === undefined ? 'org' : opts.servedProvenance; + const registry: any = { + getObject: (name: string) => + provenance === null ? undefined : { name, _packageId: 'app.myapp', _provenance: provenance }, + getItem: () => undefined, + listItems: () => [], + registerItem: () => {}, + registerObject: () => {}, + applyNavContributions: (x: unknown) => x, + isPackageDisabled: () => false, + getObjectOwner: () => undefined, + // Mirrors the REAL `SchemaRegistry.getArtifactItem` for `object`: it + // reads `getObject` and returns it only when it looks like packaged + // code (`_packageId` real, and NOT `_provenance: 'org'`). A double that + // always answered `undefined` here would report every object as + // runtime-authored and hide the artifact refusal entirely — looser than + // the implementation it stands in for, which is no test at all (#4550). + getArtifactItem: (type: string, name: string) => { + if (type !== 'object' && type !== 'objects') return undefined; + const obj: any = registry.getObject(name); + return obj && obj._packageId && obj._packageId !== 'sys_metadata' && obj._provenance !== 'org' + ? obj + : undefined; + }, + removeRuntimeShadow: (type: string, name: string) => shadowKeys.has(`${type}|${name}`), + removeOverlayEntry: (type: string, name: string) => { + removeOverlayEntryCalls.push(`${type}|${name}`); + return true; + }, + }; + if (!opts.omitUnregisterObject) { + registry.unregisterObject = (name: string) => { + unregisterObjectCalls.push(name); + if (opts.unregisterThrows) throw opts.unregisterThrows; + return true; + }; + } + + const engine: any = { + async findOne(table: string, query: { where?: Record } = {}) { + if (table !== 'sys_metadata') return null; + return rows.find((r) => matches(r, query.where)) ?? null; + }, + async find() { return []; }, + async insert(_table: string, data: Record) { return { id: 'inserted', ...data }; }, + async update(_table: string, data: Record, options?: Record) { + assertEngineUpdateDispatch(data, options); + return { id: null }; + }, + async delete(_table: string, options?: Record) { + assertEngineDeleteDispatch(options); + const id = (options as any)?.where?.id; + const at = rows.findIndex((r) => r.id === id); + if (at >= 0) rows.splice(at, 1); + return { deleted: at >= 0 ? 1 : 0 }; + }, + async count() { return 0; }, + async transaction(fn: (ctx: unknown) => Promise) { return fn(undefined); }, + async execute() { return {}; }, + async getObjectSchema() { return undefined; }, + async syncObjectSchema() { /* no physical storage in this double */ }, + registry, + }; + + // An EMPTY services registry: with no `metadata` service, tier 2 answers + // "no baseline, not degraded", which is the verdict that licenses tier 3. + const protocol = new ObjectStackProtocolImplementation( + engine, () => new Map(), opts.controlPlane === true ? undefined : 'env_1', + ) as any; + return { protocol, rows, unregisterObjectCalls, removeOverlayEntryCalls }; +} + +describe('#6808 — the heal retires the object contributor, not just the metadata entry', () => { + it('a deleted runtime-only object is unregistered BY NAME', async () => { + const h = makeHarness({ rows: [overlayRow('object', 'rc9_widget')] }); + + const result = await h.protocol.deleteMetaItem({ type: 'object', name: 'rc9_widget' }); + + expect(result.success).toBe(true); + expect(h.rows).toHaveLength(0); + // Pre-fix: `[]`. The generic-map half ran and the contributor half did not. + expect(h.unregisterObjectCalls).toEqual(['rc9_widget']); + // …alongside the half that always ran, not instead of it. + expect(h.removeOverlayEntryCalls).toContain('object|rc9_widget'); + }); + + it('the plural `objects` spelling reaches the same limb, once', async () => { + const h = makeHarness({ rows: [overlayRow('object', 'rc9_widget')] }); + + await h.protocol.deleteMetaItem({ type: 'objects', name: 'rc9_widget' }); + + // Keyed off the SINGULAR, so the twin spelling neither misses it nor + // doubles it (#4432 — every surface in agreement). + expect(h.unregisterObjectCalls).toEqual(['rc9_widget']); + }); + + it('a NON-object type never reaches it — only `object` has a second home', async () => { + const h = makeHarness({ rows: [overlayRow('view', 'rc9_grid')] }); + + const result = await h.protocol.deleteMetaItem({ type: 'view', name: 'rc9_grid' }); + + expect(result.success).toBe(true); + expect(h.removeOverlayEntryCalls).toContain('view|rc9_grid'); + expect(h.unregisterObjectCalls).toEqual([]); + }); + + /** + * The tier discipline. Tier 1 concluded a packaged artifact still serves the + * name, so the walk returns before tier 3 — and an object that is still + * served must stay registered: `assertObjectRegistered` fails CLOSED, so + * retiring it here would turn "reset to artifact default" into a data-plane + * outage for a name a code package still ships. + */ + it('an object whose overlay shadows an artifact is NOT unregistered (tier 1 stops the walk)', async () => { + const h = makeHarness({ + rows: [overlayRow('object', 'rc9_widget')], + shadowed: [{ type: 'object', name: 'rc9_widget' }], + }); + + const result = await h.protocol.deleteMetaItem({ type: 'object', name: 'rc9_widget' }); + + expect(result.success).toBe(true); + expect(h.rows).toHaveLength(0); + expect(h.removeOverlayEntryCalls).toEqual([]); + expect(h.unregisterObjectCalls).toEqual([]); + }); + + /** + * The second refusal, and the one that is NOT theoretical for objects: + * `engine.registerApp` registers a package's objects straight into + * `objectContributors` without writing the generic `metadata` map, so + * `isArtifactBacked` does not see them and an overlay row for such a name + * CAN be authored. Retiring the contributor on that row's delete would take + * a code package's object off the data plane until restart. + * + * The axis is ADR-0010 `_provenance`, the same one `removeOverlayEntry` + * uses one line up — both paths that register a TENANT's object stamp + * `'org'` server-side, an artifact carries `'package'`. + */ + it('a CODE-SHIPPED object is never unregistered by this walk', async () => { + // The reachable shape: a CONTROL-PLANE kernel (the two-tier delete + // authorization that refuses an artifact-backed `object` with + // `NOT_OVERRIDABLE` is wrapped in `environmentId !== undefined`), on the + // NO-ROW leg — which runs the heal without ever reaching the + // repository's own `assertAllowed`. `revertCommit`'s soft-remove limb + // reaches the walk without that gate either. + const h = makeHarness({ controlPlane: true, servedProvenance: 'package' }); + + const result = await h.protocol.deleteMetaItem({ type: 'object', name: 'rc9_widget' }); + + expect(result.success).toBe(true); + // The generic-map half still runs — that entry IS the overlay's slot, + // and `removeOverlayEntry` applies its own artifact refusal inside. + expect(h.removeOverlayEntryCalls).toContain('object|rc9_widget'); + expect(h.unregisterObjectCalls).toEqual([]); + }); + + /** + * The ADR-0029 refusal, at THIS seam. The registry verb throws when an + * extender still depends on the owner; the heal runs after the repository + * delete has committed, so it must not propagate that — the row is gone + * either way and a throw here would turn a successful delete into a 500. + * What it must not do is swallow it into the silent outer `catch`: a runtime + * that disagrees with `sys_metadata` has to be visible in the log. + */ + it('an extender refusal is stated, not swallowed — and the delete still succeeds', async () => { + const h = makeHarness({ + rows: [overlayRow('object', 'rc9_widget')], + unregisterThrows: new Error( + 'Cannot unregister object "rc9_widget": it is extended by app.addon. Unregister the extenders first.', + ), + }); + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + let result: any; + let warned: string[]; + try { + result = await h.protocol.deleteMetaItem({ type: 'object', name: 'rc9_widget' }); + } finally { + // Read BEFORE restoring — `mockRestore` also resets recorded calls. + warned = warn.mock.calls.map((c) => String(c[0])); + warn.mockRestore(); + } + + expect(result.success).toBe(true); + expect(h.rows).toHaveLength(0); + const refusals = warned.filter((m) => m.includes('stays registered')); + expect(refusals).toHaveLength(1); + expect(refusals[0]).toContain('rc9_widget'); + expect(refusals[0]).toContain('app.addon'); + }); + + /** + * The same `typeof … === 'function'` courtesy every other verb in this walk + * extends to a partial registry double (edge/Lite embeddings, engine + * doubles). A missing verb is a no-op, never a crash that would strand the + * rest of the delete. + */ + it('a registry without the verb degrades quietly — the delete still succeeds', async () => { + const h = makeHarness({ + rows: [overlayRow('object', 'rc9_widget')], + omitUnregisterObject: true, + }); + + const result = await h.protocol.deleteMetaItem({ type: 'object', name: 'rc9_widget' }); + + expect(result.success).toBe(true); + expect(h.rows).toHaveLength(0); + expect(h.removeOverlayEntryCalls).toContain('object|rc9_widget'); + }); +}); diff --git a/packages/metadata-protocol/src/protocol.ts b/packages/metadata-protocol/src/protocol.ts index de71ce9ccb..09750eaa06 100644 --- a/packages/metadata-protocol/src/protocol.ts +++ b/packages/metadata-protocol/src/protocol.ts @@ -7958,6 +7958,78 @@ export class ObjectStackProtocolImplementation implements registry.removeOverlayEntry(singular, name); if (type !== singular) registry.removeOverlayEntry(type, name); } + // [#6808] …and an `object` lives in a SECOND place, so tier 3 has a + // second limb. `applyObjectRegistryMutation` writes both halves on + // the way in — `registerItem` into the generic `metadata` map, and + // `registerObject` into `objectContributors` — while every verb + // this walk used above (`removeRuntimeShadow`, `registerItem`, + // `removeOverlayEntry`) addresses only the first. So the walk + // retired the listing copy and left the DISPATCH copy: measured + // over the real `SysMetadataRepository`, after the delete + // `metadata['object']` was empty while `registry.getObject(name)` + // — and `getItem('object', name)`, which special-cases back to it — + // still served the object, keeping the deleted row's schema + // readable and WRITABLE for the life of the process. + // + // Same tier as `removeOverlayEntry`, and only that tier: tiers 1 + // and 2 both concluded that a lower layer still serves this name + // (a packaged artifact, or a MetadataService baseline), and an + // object that is still served must stay registered — retiring it + // there would turn "reset to artifact default" into an outage, + // because `assertObjectRegistered` fails CLOSED for the whole data + // plane. Only "no layer serves it" licenses removal, which is the + // verdict this branch already carries for the other half. + // + // The ADR-0029 extender guard lives in the registry verb (see + // {@link SchemaRegistry.unregisterObject}) and it THROWS — which + // this best-effort heal must not propagate: the repository delete + // has already committed, and the operator's row is gone either + // way. So the refusal is caught and stated, deliberately NOT left + // to the silent outer `catch`: an extended object surviving a + // delete is a real divergence between the store and the runtime, + // and it must be visible in the log rather than inferred later + // from a registry that disagrees with `sys_metadata`. + // + // ── AND IT NEVER RETIRES A CODE-SHIPPED OBJECT ── + // + // The same refusal `removeOverlayEntry` carries one line up, for + // the same reason: unregistering shipped code that the overlay + // delete never touched would be a worse bug than the one this + // closes. It is asked through the protocol's OWN existing predicate + // ({@link isArtifactBacked} → `SchemaRegistry.getArtifactItem`, + // which for `object` reads the contributor definition and applies + // exactly the artifact test the sibling verb applies to the plain + // key), so this limb inherits that judgement instead of open-coding + // a second one. + // + // Not theoretical, and NOT already covered by the gate at the top of + // `deleteMetaItem`: that two-tier authorization — which refuses an + // artifact-backed `object` outright with `not_overridable` — runs + // only when `environmentId !== undefined`. On a CONTROL-PLANE + // kernel it is skipped, and `revertCommit`'s soft-remove limb + // reaches this walk without it either, so the delete can arrive + // here for a name a code package still ships. Retiring it would + // take that object off the whole data plane until restart, because + // `assertObjectRegistered` fails closed. + // + // The check lives HERE rather than in the verb because it is a + // statement about LAYERS, which is what this walk reasons about; + // `unregisterObject` stays a general removal whose only refusal is + // ADR-0029's extender rule. + if ( + singular === 'object' + && !this.isArtifactBacked(singular, name) + && typeof registry.unregisterObject === 'function' + ) { + try { + registry.unregisterObject(name); + } catch (err: any) { + console.warn( + `[Protocol] object '${name}' was deleted from sys_metadata but stays registered: ` + + `${err?.message ?? err}`, + ); + } + } } catch { // Best-effort registry refresh; next read fixes it anyway } diff --git a/packages/objectql/src/protocol-commit-history.test.ts b/packages/objectql/src/protocol-commit-history.test.ts index 2891944b86..5dae11dc78 100644 --- a/packages/objectql/src/protocol-commit-history.test.ts +++ b/packages/objectql/src/protocol-commit-history.test.ts @@ -1029,13 +1029,21 @@ describe('#6621 — revertCommit RESTORE limb refreshes the registry', () => { * * These assert PARITY with `deleteMetaItem` rather than a literal registry * state, deliberately. The two callers perform the same repository delete and - * should leave the same runtime view; stating it as parity also keeps the pin - * honest about a gap it does NOT close — `restoreArtifactRegistryView` reaches - * the `metadata` map but not `objectContributors`, so `getObject` still serves - * a soft-removed runtime object. That gap is `deleteMetaItem`'s too (there is + * should leave the same runtime view; stating it as parity also kept the pin + * honest about a gap it did NOT close — `restoreArtifactRegistryView` reached + * the `metadata` map but not `objectContributors`, so `getObject` still served + * a soft-removed runtime object. That gap was `deleteMetaItem`'s too (there was * no per-name object unregister in `SchemaRegistry` at all, only - * `unregisterObjectsByPackage`), it is not introduced here, and a parity - * assertion stays green when it is fixed for both. + * `unregisterObjectsByPackage`), it was not introduced here, and the parity + * assertion was written to stay green when it was fixed for both. + * + * [#6808] It has been: `SchemaRegistry.unregisterObject` is the name-addressed + * removal that was missing, and the heal's tier-3 branch now calls it, so both + * callers stop serving the object as well as listing it. The parity assertion + * is unchanged and still green — but `objectServed` now reads `false` on both + * sides rather than `true` on both, so the object case below states that + * value outright. A parity pin that never names the value it agrees on can + * freeze a shared bug as "consistent"; this one no longer can. */ /** The registry facts a soft-remove is allowed to change, as one comparable value. */ @@ -1115,10 +1123,17 @@ describe('#6621 — revertCommit SOFT-REMOVE limb heals the registry, like delet // THE LINE THAT WAS RED: pre-fix the plain-key entry stayed for the life of // the process, so `GET /meta/object` kept enumerating a reverted-away item. expect(registryShapeFor(viaRevert.registry, 'object', 'myapp_invoice').plainKeyEntry).toBe(false); + // [#6808] THE SECOND LINE THAT WAS RED, and the load-bearing one: the + // contributor entry survived, so `getObject` — the surface data CRUD + // dispatches on — kept serving a soft-removed object. Named outright rather + // than left to the parity comparison below, which agreed on `true` before + // this was fixed and agrees on `false` after. + expect(registryShapeFor(viaRevert.registry, 'object', 'myapp_invoice').objectServed).toBe(false); const viaDelete = makeRealRepoHarness([], { controlPlane: true }); await seedCreatedObject(viaDelete.protocol, 'myapp_invoice', APP_PKG); await viaDelete.protocol.deleteMetaItem({ type: 'object', name: 'myapp_invoice' }); + expect(registryShapeFor(viaDelete.registry, 'object', 'myapp_invoice').objectServed).toBe(false); expect(registryShapeFor(viaRevert.registry, 'object', 'myapp_invoice')) .toEqual(registryShapeFor(viaDelete.registry, 'object', 'myapp_invoice')); }); diff --git a/packages/objectql/src/protocol-delete-object-registry-heal.test.ts b/packages/objectql/src/protocol-delete-object-registry-heal.test.ts new file mode 100644 index 0000000000..be8706f119 --- /dev/null +++ b/packages/objectql/src/protocol-delete-object-registry-heal.test.ts @@ -0,0 +1,407 @@ +// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. + +import { describe, it, expect, vi } from 'vitest'; +import { ObjectStackProtocolImplementation } from '@objectstack/metadata-protocol'; +import { SchemaRegistry } from './registry.js'; +// [#4550 / #5480] The producer's OWN write-verb dispatch decisions, so the +// engine double below cannot accept a call `ObjectQL.delete` / `ObjectQL.update` +// refuses — a double looser than the implementation is no test at all. +import { assertEngineDeleteDispatch } from './engine-delete-dispatch.js'; +import { assertEngineUpdateDispatch } from './engine-update-dispatch.js'; + +/** + * #6808 — deleting an `object` must stop BOTH registry exits serving it. + * + * `deleteMetaItem` ends its repository delete with `restoreArtifactRegistryView` + * (the #6687 three-tier heal). That walk operates entirely on `SchemaRegistry`'s + * generic `metadata` map — `removeRuntimeShadow`, `registerItem` and + * `removeOverlayEntry` all address `this.metadata.get(type)`. An `object`, + * though, is written into TWO places by `applyObjectRegistryMutation`: + * + * registry.registerItem('object', item, 'name') → metadata map + * registry.registerObject({ …item, _provenance: 'org' }, …) → objectContributors + * + * The heal only undid the first. Measured on `origin/main` with this file's + * harness (the real `SysMetadataRepository` over an in-memory engine): + * + * BEFORE delete: metadata['object'] -> ["myapp_invoice"] | contributors -> ["myapp_invoice"] + * AFTER delete: metadata['object'] -> [] | contributors -> ["myapp_invoice"] + * registry.getObject('myapp_invoice') -> STILL SERVED + * registry.getItem('object','myapp_invoice') -> STILL SERVED (it special-cases back to getObject) + * + * The surviving half is the load-bearing one: `getObject` is what the data + * plane dispatches on (`assertObjectRegistered`), so the `sys_metadata` row was + * gone while the object stayed resolvable — and writable — for the life of the + * process. Reachable on the ordinary Studio delete path. + * + * The fix is a name-addressed removal verb on the registry + * (`SchemaRegistry.unregisterObject`, honouring ADR-0029's single-owner / + * extender rules) called from the heal's tier-3 branch — the tier that has + * already established no lower layer serves the name. + */ + +const APP_PKG = 'app.myapp'; + +const invoiceBody = (name: string) => ({ + name, + label: 'Invoice', + fields: { + name: { name: 'name', type: 'text', label: 'Name' }, + amount: { name: 'amount', type: 'number', label: 'Amount' }, + }, +}); + +/** ADR-0048: the overlay key includes `package_id`, so the double keys on it too. */ +const rowKey = (w: Record) => + [w.type, w.name, w.organization_id ?? '', w.package_id ?? '', w.state ?? 'active'].join('|'); + +const matchesWhere = (row: Record, where: Record) => + Object.entries(where ?? {}).every(([k, v]) => { + if (v === null) return row[k] === null || row[k] === undefined; + return row[k] === v; + }); + +/** + * In-memory `sys_metadata` + `sys_metadata_history` engine carrying a REAL + * `SchemaRegistry`, plus a data-plane `insert` so the CRUD-refusal pin below + * measures a write that genuinely dispatched before the delete. + */ +function makeHarness(opts: { controlPlane?: boolean } = {}) { + const environmentId: string | undefined = opts.controlPlane === true ? undefined : 'env_test'; + const registry = new SchemaRegistry({ multiTenant: false }); + (registry as any).logLevel = 'silent'; + const rows = new Map(); + const historyRows: any[] = []; + const dataRows: any[] = []; + let nextId = 0; + + const findRow = (w: Record) => { + for (const [k, r] of rows) if (matchesWhere(r, w)) return { key: k, row: r }; + return null; + }; + + const engine: any = { + registry, + async findOne(table: string, o: { where: Record }) { + if (table === 'sys_metadata_history') return historyRows.find((h) => matchesWhere(h, o.where)) ?? null; + if (table !== 'sys_metadata') return null; + return findRow(o.where)?.row ?? null; + }, + async find(table: string, o: { where: Record }) { + if (table === 'sys_metadata_history') return historyRows.filter((h) => matchesWhere(h, o.where)); + if (table !== 'sys_metadata') return []; + return Array.from(rows.values()).filter((r) => matchesWhere(r, o.where)); + }, + async insert(table: string, data: Record) { + if (table === 'sys_metadata_history') { + const h = { id: `h_${++nextId}`, ...(data as any) }; + historyRows.push(h); + return { id: h.id }; + } + if (table !== 'sys_metadata') { + // The data plane: a real write for a real (registered) object. Platform + // side tables (`sys_metadata_commit`, …) are NOT data-plane rows — counting + // them here would make the CRUD pin below measure metadata bookkeeping. + const rec = { id: `rec_${++nextId}`, ...(data as any) }; + if (!table.startsWith('sys_')) dataRows.push({ object: table, ...rec }); + return rec; + } + const row = { id: `r_${++nextId}`, ...(data as any) }; + rows.set(rowKey(data), row); + return { id: row.id }; + }, + async update(table: string, data: Record, o: { where: Record }) { + assertEngineUpdateDispatch(data, o); + if (table !== 'sys_metadata') return { id: null }; + const found = findRow(o.where); + if (!found) return { id: null }; + const merged = { ...found.row, ...(data as any) }; + rows.delete(found.key); + rows.set(rowKey(merged), merged); + return { id: merged.id }; + }, + async delete(table: string, o?: Record) { + assertEngineDeleteDispatch(o); + if (table !== 'sys_metadata') return { deleted: 0 }; + const found = findRow(((o as any)?.where ?? {}) as Record); + if (!found) return { deleted: 0 }; + rows.delete(found.key); + return { deleted: 1 }; + }, + async syncObjectSchema() { /* no physical storage in this double */ }, + }; + + const protocol = new ObjectStackProtocolImplementation(engine, undefined, environmentId); + return { protocol, engine, registry, rows, dataRows }; +} + +/** The first-build shape: a runtime-CREATED object, authored once through Studio. */ +async function seedCreatedObject(protocol: any, name: string, packageId?: string) { + await protocol.saveMetaItem({ + type: 'object', name, ...(packageId ? { packageId } : {}), item: invoiceBody(name), + }); +} + +/** The generic `metadata` map's plain-key entry — the LISTING half. */ +const plainKeyEntry = (registry: SchemaRegistry, name: string) => + Array.from( + ((registry as any).metadata as Map>).get('object')?.keys() ?? [], + ).includes(name); + +const storedRows = (rows: Map, name: string) => + Array.from(rows.values()).filter((r) => r.name === name); + +describe('#6808 — deleteMetaItem stops BOTH registry exits serving a deleted object', () => { + it('the object is gone from getObject AND getItem, not just from the listing map', async () => { + const { protocol, registry, rows } = makeHarness(); + await seedCreatedObject(protocol, 'myapp_invoice', APP_PKG); + + // Both halves are live before the delete — otherwise "gone" below could be + // true for the empty reason. + expect(plainKeyEntry(registry, 'myapp_invoice')).toBe(true); + expect(registry.getObject('myapp_invoice')).toBeDefined(); + expect(registry.getItem('object', 'myapp_invoice')).toBeDefined(); + + const res = await protocol.deleteMetaItem({ type: 'object', name: 'myapp_invoice' }); + + expect(res.success).toBe(true); + expect(storedRows(rows, 'myapp_invoice')).toHaveLength(0); + // Already green pre-fix — the half the heal always reached. + expect(plainKeyEntry(registry, 'myapp_invoice')).toBe(false); + // THE LINES THAT WERE RED: `objectContributors` kept the object, so the + // surface data CRUD dispatches on kept serving it. + expect(registry.getObject('myapp_invoice')).toBeUndefined(); + expect(registry.getItem('object', 'myapp_invoice')).toBeUndefined(); + // …and the listing verb agrees with both. + expect(registry.getAllObjects().map((o: any) => o.name)).not.toContain('myapp_invoice'); + }); + + /** + * The consequence, stated as the thing an operator would actually hit: the + * data plane kept accepting writes to a deleted object. `assertObjectRegistered` + * (#3770) reads `registry.getObject`, so the surviving contributor entry was a + * live write door. Asserted by REFUSAL IDENTITY (`OBJECT_NOT_FOUND` / 404), + * never a bare `toThrow` — a throw for any other reason would pass that. + */ + it('data CRUD dispatched before the delete and is REFUSED after it (OBJECT_NOT_FOUND/404)', async () => { + const { protocol, dataRows } = makeHarness(); + await seedCreatedObject(protocol, 'myapp_invoice', APP_PKG); + + const created = await protocol.createData({ + object: 'myapp_invoice', data: { name: 'INV-1', amount: 10 }, + }); + expect(created.id).toBeTruthy(); + expect(dataRows).toHaveLength(1); + + await protocol.deleteMetaItem({ type: 'object', name: 'myapp_invoice' }); + + const err = await protocol + .createData({ object: 'myapp_invoice', data: { name: 'INV-2', amount: 20 } }) + .then(() => null, (e: any) => e); + // Pre-fix this write SUCCEEDED — rows into a table whose metadata is gone. + expect(err).toBeInstanceOf(Error); + expect(err.code).toBe('OBJECT_NOT_FOUND'); + expect(err.status).toBe(404); + expect(err.object).toBe('myapp_invoice'); + expect(dataRows).toHaveLength(1); + }); + + /** + * The tier discipline the fix must NOT break. The removal fires only in the + * heal's tier 3 ("no layer serves this name"); tiers 1 and 2 concluded a lower + * layer still does, and an object that is still served must stay registered — + * `assertObjectRegistered` fails CLOSED, so retiring it there would turn + * "reset to artifact default" into a data-plane outage. + * + * Seed order is deliberate: the packaged artifact is registered AFTER the + * overlay save. Registered first, its package provenance would make + * `isArtifactBacked` true and `saveMetaItem`'s overlay gate would refuse the + * write outright (`object` is `allowOrgOverride: false`), so there would be no + * overlay to delete and the pin would measure nothing. + */ + it('an object whose overlay shadows a packaged artifact STAYS registered (tier 1)', async () => { + const { protocol, registry, rows } = makeHarness(); + await seedCreatedObject(protocol, 'myapp_invoice', APP_PKG); + // The code package's own artifact, under its composite key. + (registry as any).registerItem('object', invoiceBody('myapp_invoice'), 'name', APP_PKG); + + await protocol.deleteMetaItem({ type: 'object', name: 'myapp_invoice' }); + + expect(storedRows(rows, 'myapp_invoice')).toHaveLength(0); + // Tier 1 healed by un-shadowing, so the name is still served — by the + // artifact. Removing the contributor here would have made the shipped + // object unreachable for data CRUD. + expect(registry.getObject('myapp_invoice')).toBeDefined(); + }); + + /** + * The provenance refusal, measured against a REAL registry — and the reason + * it is not theoretical for objects. `engine.registerApp` registers a code + * package's objects straight into `objectContributors` and never writes the + * generic `metadata` map, so `isArtifactBacked` cannot see them (tier 1 has + * no composite entry to find either) and an overlay row for such a name can + * still be authored. Without this refusal the heal would reach tier 3 and + * take a shipped object off the data plane until restart — a strictly worse + * bug than the one being fixed, which is the same argument + * `removeOverlayEntry` records for its own artifact refusal. + * + * ADR-0010 `_provenance` is the axis: both paths that register a TENANT's + * object (`applyObjectRegistryMutation` and the boot rehydration) stamp + * `'org'` server-side; a loader-introduced artifact carries `'package'`. + */ + it('a CODE-SHIPPED object survives the delete of a same-named row', async () => { + // The reachable shape, and the reason this is not covered by the gates in + // front of the heal. `deleteMetaItem`'s two-tier authorization refuses an + // artifact-backed `object` with `NOT_OVERRIDABLE` (pinned below) but runs + // only when `environmentId !== undefined`; on a CONTROL-PLANE kernel the + // repository's own `assertAllowed` refuses it instead — except on the + // NO-ROW leg, which never reaches the repository's write verb at all. That + // leg still runs the heal (deliberately: "a stale shadow can outlive the + // row it came from"), so the walk arrives for a name a code package ships. + const { protocol, registry, rows } = makeHarness({ controlPlane: true }); + // The package's object, as `registerApp` registers it: contributors only, + // no composite `metadata` entry — so tier 1 finds no shadow to lift, and + // with no MetadataService here tier 2 finds no baseline either. The walk + // reaches tier 3 with the object still shipped. + registry.registerObject( + { ...invoiceBody('myapp_invoice'), _packageId: APP_PKG, _provenance: 'package' } as any, + APP_PKG, + ); + expect((registry.getObject('myapp_invoice') as any)._provenance).toBe('package'); + + const res = await protocol.deleteMetaItem({ type: 'object', name: 'myapp_invoice' }); + + expect(res.success).toBe(true); + expect(res.reset).toBe(false); // nothing to delete — no overlay row exists + expect(storedRows(rows, 'myapp_invoice')).toHaveLength(0); + // THE OBJECT SURVIVES — the package still ships it, and unregistering code + // the overlay delete never touched would be a worse bug than the one fixed. + expect(registry.getObject('myapp_invoice')).toBeDefined(); + expect((registry.getObject('myapp_invoice') as any)._provenance).toBe('package'); + }); + + /** + * …and on a TENANT kernel the same delete never reaches the heal at all: the + * two-tier authorization refuses it first. Pinned as the pair of the case + * above so the tier-3 refusal is not mistaken for the only thing standing + * between a code package's object and this walk — and so a future change to + * either gate cannot quietly leave the pair with no protection. + */ + it('the same delete on a tenant kernel is refused before the heal (NOT_OVERRIDABLE)', async () => { + const { protocol, registry } = makeHarness(); + await seedCreatedObject(protocol, 'myapp_invoice', APP_PKG); + registry.registerObject( + { ...invoiceBody('myapp_invoice'), _packageId: APP_PKG, _provenance: 'package' } as any, + APP_PKG, + ); + + const err = await protocol + .deleteMetaItem({ type: 'object', name: 'myapp_invoice' }) + .then(() => null, (e: any) => e); + + expect(err).toBeInstanceOf(Error); + expect(err.code).toBe('NOT_OVERRIDABLE'); + expect(registry.getObject('myapp_invoice')).toBeDefined(); + }); + + /** + * ADR-0029 — the extender guard, at the delete seam. An extension is merged + * into the owner's definition (`resolveObject`), so tearing the owner out from + * under a live extender leaves contributions that resolve to nothing: the + * "has extenders but no owner" violation `assertSingleOwnerPerObject` exists + * to catch, reached at runtime instead of at bootstrap. + * + * The heal is best-effort and runs AFTER the repository delete has committed, + * so it cannot propagate the refusal — the row is gone either way. What it + * must not do is swallow it: the refusal is caught and logged by name, so a + * runtime that disagrees with `sys_metadata` is visible rather than inferred. + */ + it('refuses to unregister an object another package still extends, naming the extender', async () => { + const { protocol, registry, rows } = makeHarness(); + await seedCreatedObject(protocol, 'myapp_invoice', APP_PKG); + registry.registerObject( + { name: 'myapp_invoice', fields: { note: { name: 'note', type: 'text' } } } as any, + 'app.addon', undefined, 'extend', + ); + + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + let res: any; + let warned: string[]; + try { + res = await protocol.deleteMetaItem({ type: 'object', name: 'myapp_invoice' }); + } finally { + // Read the calls BEFORE restoring: `mockRestore` also resets the mock's + // recorded state, so a spy asserted after it always reads empty. + warned = warn.mock.calls.map((c) => String(c[0])); + warn.mockRestore(); + } + + // The delete itself still succeeds — the row really is gone. + expect(res.success).toBe(true); + expect(storedRows(rows, 'myapp_invoice')).toHaveLength(0); + // …and the extended object is NOT torn down under its extender. + expect(registry.getObject('myapp_invoice')).toBeDefined(); + expect(registry.getObjectOwner('myapp_invoice')?.packageId).toBe(APP_PKG); + // The refusal is loud and names who is depending on it — not swallowed by + // the heal's silent outer catch. + const refusals = warned.filter((m) => m.includes('stays registered')); + expect(refusals).toHaveLength(1); + expect(refusals[0]).toContain('app.addon'); + expect(refusals[0]).toContain('myapp_invoice'); + }); + + /** + * The same delete with the extender gone succeeds — the guard is a guard, not + * a blanket refusal. Stated as a pair with the case above so a fix that simply + * never removes anything cannot satisfy both. + */ + it('the same object with no extenders is unregistered', async () => { + const { protocol, registry } = makeHarness(); + await seedCreatedObject(protocol, 'myapp_invoice', APP_PKG); + registry.registerObject( + { name: 'myapp_invoice', fields: { note: { name: 'note', type: 'text' } } } as any, + 'app.addon', undefined, 'extend', + ); + // The extender uninstalls first, the way the guard's message tells it to. + registry.unregisterObjectsByPackage('app.addon'); + + await protocol.deleteMetaItem({ type: 'object', name: 'myapp_invoice' }); + + expect(registry.getObject('myapp_invoice')).toBeUndefined(); + expect(registry.getItem('object', 'myapp_invoice')).toBeUndefined(); + }); + + /** + * The plural spelling reaches the same limb. `canonicalizeMetaRequestType` + * folds `objects` → `object` at the top of `deleteMetaItem`, and the heal + * re-folds through `PLURAL_TO_SINGULAR`; the removal keys off the SINGULAR, so + * neither spelling can miss it (#4432's "every surface in agreement"). + */ + it('the plural `objects` spelling heals identically', async () => { + const { protocol, registry } = makeHarness(); + await seedCreatedObject(protocol, 'myapp_invoice', APP_PKG); + + await protocol.deleteMetaItem({ type: 'objects', name: 'myapp_invoice' }); + + expect(registry.getObject('myapp_invoice')).toBeUndefined(); + expect(registry.getItem('object', 'myapp_invoice')).toBeUndefined(); + }); + + /** + * A control-plane kernel (`environmentId === undefined`) runs the same walk — + * the tier-2 MetadataService re-registration is the only control-plane-gated + * step, and it is not this limb. Pinned because the sibling caller + * (`revertCommit`'s soft-remove) is only reachable on that kernel in the + * parity pins of `protocol-commit-history.test.ts`. + */ + it('a control-plane kernel heals both halves too', async () => { + const { protocol, registry } = makeHarness({ controlPlane: true }); + await seedCreatedObject(protocol, 'myapp_invoice', APP_PKG); + expect(registry.getObject('myapp_invoice')).toBeDefined(); + + await protocol.deleteMetaItem({ type: 'object', name: 'myapp_invoice' }); + + expect(registry.getObject('myapp_invoice')).toBeUndefined(); + expect(plainKeyEntry(registry, 'myapp_invoice')).toBe(false); + }); +}); diff --git a/packages/objectql/src/registry-unregister-object.test.ts b/packages/objectql/src/registry-unregister-object.test.ts new file mode 100644 index 0000000000..6a9c87d02c --- /dev/null +++ b/packages/objectql/src/registry-unregister-object.test.ts @@ -0,0 +1,276 @@ +// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. + +import { describe, it, expect, vi } from 'vitest'; +import { SchemaRegistry } from './registry.js'; + +/** + * #6808 — `SchemaRegistry.unregisterObject`, the NAME-addressed removal verb. + * + * Until it existed, the only way an object left `objectContributors` was + * `unregisterObjectsByPackage` — addressed by PACKAGE, which is the right verb + * for an uninstall and the wrong one for a single delete. The gap was not + * academic: `deleteMetaItem`'s registry heal walks the generic `metadata` map + * only, so a deleted `object` kept being served by `getObject` (and therefore + * by `getItem('object', …)`, which special-cases back to it) for the life of + * the process — the surface the data plane dispatches on. The protocol half is + * pinned in `protocol-delete-object-registry-heal.test.ts`; this file pins the + * verb itself. + * + * The guard it carries is ADR-0029's, borrowed rather than re-invented: exactly + * one package owns an object and others may only extend it, so removing an + * owner out from under a live extender leaves contributions that resolve to + * nothing. `unregisterObjectsByPackage` already encodes that judgement (refuse, + * name the extenders, `force` overrides); this verb mirrors it with the address + * changed from package to name — and needs no new bookkeeping to do it, because + * the owner and the extenders are both already in the contributor list. + */ + +const quiet = () => { + const r = new SchemaRegistry({ multiTenant: false }); + (r as any).logLevel = 'silent'; + return r; +}; + +const objectBody = (name: string, field = 'name') => ({ + name, + label: name, + fields: { [field]: { name: field, type: 'text', label: field } }, +}) as any; + +describe('#6808 — SchemaRegistry.unregisterObject', () => { + it('removes an owned object from BOTH object exits, and reports it', () => { + const r = quiet(); + r.registerObject(objectBody('myapp_invoice'), 'app.myapp'); + expect(r.getObject('myapp_invoice')).toBeDefined(); + expect(r.getItem('object', 'myapp_invoice')).toBeDefined(); + + expect(r.unregisterObject('myapp_invoice')).toBe(true); + + expect(r.getObject('myapp_invoice')).toBeUndefined(); + // `getItem` special-cases `object` straight back to `getObject`, which is + // exactly why the listing map looked clean while dispatch did not. + expect(r.getItem('object', 'myapp_invoice')).toBeUndefined(); + expect(r.getAllObjects().map((o: any) => o.name)).not.toContain('myapp_invoice'); + expect(r.getObjectContributors('myapp_invoice')).toEqual([]); + expect(r.getObjectOwner('myapp_invoice')).toBeUndefined(); + }); + + it('is idempotent and answers `false` for a name that was never registered', () => { + const r = quiet(); + r.registerObject(objectBody('myapp_invoice'), 'app.myapp'); + + expect(r.unregisterObject('myapp_invoice')).toBe(true); + expect(r.unregisterObject('myapp_invoice')).toBe(false); + expect(r.unregisterObject('never_existed')).toBe(false); + }); + + /** + * The merged-object cache is populated by `resolveObject` on the way in, so a + * removal that forgot to invalidate it would keep answering for a name with + * no contributors at all. Read the object FIRST so the cache is warm, then + * remove — the only ordering that can catch it. + */ + it('invalidates the merged-object cache, so a warm read cannot outlive the removal', () => { + const r = quiet(); + r.registerObject(objectBody('myapp_invoice'), 'app.myapp'); + expect(r.getObject('myapp_invoice')).toBeDefined(); // warms the cache + + r.unregisterObject('myapp_invoice'); + + expect(r.getObject('myapp_invoice')).toBeUndefined(); + expect(r.resolveObject('myapp_invoice')).toBeUndefined(); + }); + + /** + * `objectRevision` is what registry-DERIVED caches key on (the engine's + * roll-up summary index). A removal that leaves it still would let a consumer + * keep an index built while the object existed. + */ + it('bumps objectRevision so registry-derived caches learn the set moved', () => { + const r = quiet(); + r.registerObject(objectBody('myapp_invoice'), 'app.myapp'); + const before = r.objectRevision; + + r.unregisterObject('myapp_invoice'); + + expect(r.objectRevision).toBeGreaterThan(before); + // A no-op removal changes nothing — including the revision. + const after = r.objectRevision; + r.unregisterObject('myapp_invoice'); + expect(r.objectRevision).toBe(after); + }); + + describe('ADR-0029 — the extender guard', () => { + it('REFUSES an owner another package still extends, naming the extender', () => { + const r = quiet(); + r.registerObject(objectBody('myapp_invoice'), 'app.myapp'); + r.registerObject(objectBody('myapp_invoice', 'note'), 'app.addon', undefined, 'extend'); + + expect(() => r.unregisterObject('myapp_invoice')).toThrowError(/app\.addon/); + // The refusal must LEAVE the object intact — a guard that throws after + // mutating is worse than no guard. + expect(r.getObject('myapp_invoice')).toBeDefined(); + expect(r.getObjectOwner('myapp_invoice')?.packageId).toBe('app.myapp'); + expect(r.getObjectContributors('myapp_invoice')).toHaveLength(2); + }); + + it('the refusal names the OBJECT and tells the caller what to do first', () => { + const r = quiet(); + r.registerObject(objectBody('myapp_invoice'), 'app.myapp'); + r.registerObject(objectBody('myapp_invoice', 'note'), 'app.addon', undefined, 'extend'); + r.registerObject(objectBody('myapp_invoice', 'tag'), 'app.tags', undefined, 'extend'); + + const err = (() => { try { r.unregisterObject('myapp_invoice'); return null; } catch (e) { return e as Error; } })(); + + expect(err).toBeInstanceOf(Error); + expect(err!.message).toContain('myapp_invoice'); + // EVERY extender is named, not just the first — the same shape + // `unregisterObjectsByPackage` reports for an uninstall. + expect(err!.message).toContain('app.addon'); + expect(err!.message).toContain('app.tags'); + expect(err!.message).toContain('Unregister the extenders first'); + }); + + it('succeeds once the extender is gone — the guard is a guard, not a refusal', () => { + const r = quiet(); + r.registerObject(objectBody('myapp_invoice'), 'app.myapp'); + r.registerObject(objectBody('myapp_invoice', 'note'), 'app.addon', undefined, 'extend'); + r.unregisterObjectsByPackage('app.addon'); + + expect(r.unregisterObject('myapp_invoice')).toBe(true); + expect(r.getObject('myapp_invoice')).toBeUndefined(); + }); + + /** + * "Other" means "not the owner's package" — the same relation the sibling + * verb expresses as "not the package being uninstalled". An extension the + * OWNER contributed goes away with the object it extends, exactly as it + * would on an uninstall of that package. + */ + it('an extension from the OWNER\'s own package does not block the removal', () => { + const r = quiet(); + r.registerObject(objectBody('myapp_invoice'), 'app.myapp'); + r.registerObject(objectBody('myapp_invoice', 'note'), 'app.myapp', undefined, 'extend'); + + expect(r.unregisterObject('myapp_invoice')).toBe(true); + expect(r.getObjectContributors('myapp_invoice')).toEqual([]); + }); + + it('`force` overrides the guard, like the package-scoped verb\'s', () => { + const r = quiet(); + r.registerObject(objectBody('myapp_invoice'), 'app.myapp'); + r.registerObject(objectBody('myapp_invoice', 'note'), 'app.addon', undefined, 'extend'); + + expect(r.unregisterObject('myapp_invoice', { force: true })).toBe(true); + expect(r.getObject('myapp_invoice')).toBeUndefined(); + expect(r.getObjectContributors('myapp_invoice')).toEqual([]); + }); + + /** + * An owner-less object (only `extend` contributions) is ALREADY an ADR-0029 + * violation — `assertSingleOwnerPerObject` fails it at bootstrap. Removing + * it silently would destroy contributions nobody asked to remove, so every + * extender counts as "other" and the refusal is the same one. + */ + it('refuses an owner-less object rather than silently tearing the extenders down', () => { + const r = quiet(); + r.registerObject(objectBody('orphan_ext', 'note'), 'app.addon', undefined, 'extend'); + + expect(() => r.unregisterObject('orphan_ext')).toThrowError(/app\.addon/); + expect(r.getObjectContributors('orphan_ext')).toHaveLength(1); + expect(r.unregisterObject('orphan_ext', { force: true })).toBe(true); + }); + }); + + /** + * `computeFQN` is the identity function (Prime Directive #6 — object name IS + * the table name; `namespace` is deprecated), so a contributor key differs + * from the short name only for LEGACY `__` names, which `parseFQN` + * still splits. Both forms are exercised because `getObject` accepts both, + * and this verb resolves names through the SAME code path it does — a + * remover with its own resolution could remove an entry `getObject` never + * served and leave the served one behind, which is this bug's shape one + * layer down. + */ + describe('name resolution — the same one `getObject` uses', () => { + it('removes exactly the entry that was being SERVED, addressed by short name', () => { + const r = quiet(); + r.registerObject(objectBody('crm__account'), 'app.crm'); + expect(r.getObject('account')).toBeDefined(); // legacy short-name resolution + + expect(r.unregisterObject('account')).toBe(true); + + expect(r.getObject('account')).toBeUndefined(); + // …and the key it lived under is gone with it. + expect(r.resolveObject('crm__account')).toBeUndefined(); + }); + + it('accepts the full key too, the disambiguation form `getObject` accepts', () => { + const r = quiet(); + r.registerObject(objectBody('crm__account'), 'app.crm'); + + expect(r.unregisterObject('crm__account')).toBe(true); + expect(r.getObject('account')).toBeUndefined(); + }); + + /** + * Ambiguity resolves the way the READ resolves it — first match, same + * warning — because "stop serving what you were serving" is the whole + * contract. + */ + it('an ambiguous short name removes the entry `getObject` would have returned', () => { + const r = quiet(); + r.registerObject(objectBody('crm__account'), 'app.crm'); + r.registerObject(objectBody('erp__account'), 'app.erp'); + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + try { + const servedFqn = (r.getObject('account') as any)?.name; + expect(servedFqn).toBeTruthy(); + + expect(r.unregisterObject('account')).toBe(true); + + expect(r.resolveObject(servedFqn)).toBeUndefined(); + // The other one is untouched — one name removed, not two. + expect(r.getObject('account')).toBeDefined(); + expect((r.getObject('account') as any).name).not.toBe(servedFqn); + } finally { + warn.mockRestore(); + } + }); + }); + + /** + * The namespace is registered per PACKAGE and shared by every object that + * package ships, so a single object's removal must not drop it — + * `uninstallPackage` owns that half. Stated because the co-located state this + * verb DOES clean up (contributors, merged cache, revision) makes "clean up + * everything `registerObject` touched" a tempting over-reach. + */ + it('leaves the package namespace registered — a sibling object still needs it', () => { + const r = quiet(); + r.registerObject(objectBody('crm_account'), 'app.crm', 'crm'); + r.registerObject(objectBody('crm_contact'), 'app.crm', 'crm'); + + r.unregisterObject('crm_account'); + + expect(r.getNamespaceOwners('crm')).toContain('app.crm'); + expect(r.getObject('crm_contact')).toBeDefined(); + }); + + /** + * The invariant the guard protects, asserted end-to-end: after a legitimate + * removal the registry still passes ADR-0029's bootstrap check. A removal that + * left extenders behind would fail it with "has extenders but no owner". + */ + it('leaves the ADR-0029 single-owner invariant intact', () => { + const r = quiet(); + r.registerObject(objectBody('myapp_invoice'), 'app.myapp'); + r.registerObject(objectBody('myapp_quote'), 'app.myapp'); + r.registerObject(objectBody('myapp_quote', 'note'), 'app.addon', undefined, 'extend'); + + r.unregisterObject('myapp_invoice'); + + expect(() => r.assertSingleOwnerPerObject()).not.toThrow(); + expect(r.getObject('myapp_quote')).toBeDefined(); + }); +}); diff --git a/packages/objectql/src/registry.ts b/packages/objectql/src/registry.ts index bd3d6edbb6..2126812e1e 100644 --- a/packages/objectql/src/registry.ts +++ b/packages/objectql/src/registry.ts @@ -1249,6 +1249,27 @@ export class SchemaRegistry { * 2. Legacy FQN match (e.g., 'crm__account') — backward compat. */ getObject(name: string): ServiceObject | undefined { + const fqn = this.resolveObjectKey(name); + return fqn === undefined ? undefined : this.resolveObject(fqn); + } + + /** + * [#6808] The name→FQN half of {@link getObject}, extracted so the READ and + * the name-addressed REMOVAL ({@link unregisterObject}) cannot disagree + * about which contributor entry a bare name addresses. + * + * That disagreement is not hypothetical — it is the shape of the bug this + * was extracted for: `deleteMetaItem`'s registry heal reached one of the two + * places an `object` lives, and the surface data CRUD dispatches on + * (`getObject`) kept serving a deleted object. A remover that resolved names + * its own way would re-open the same seam one layer down: it could remove an + * entry `getObject` never served, leaving the served one behind. + * + * Returns `undefined` when nothing is registered under the name, so + * `getObject` keeps its exact previous behaviour: `resolveObject` on an + * unknown FQN also answered `undefined`. + */ + private resolveObjectKey(name: string): string | undefined { // Canonical: short name lookup const matches: string[] = []; for (const fqn of this.objectContributors.keys()) { @@ -1264,11 +1285,11 @@ export class SchemaRegistry { `Returning first match. Use FQN to disambiguate.` ); } - return this.resolveObject(matches[0]); + return matches[0]; } // Fallback: explicit FQN - return this.resolveObject(name); + return this.objectContributors.has(name) ? name : undefined; } /** @@ -1399,6 +1420,104 @@ export class SchemaRegistry { } } + /** + * [#6808] Unregister ONE object, addressed by NAME — the removal verb this + * registry was missing, and the reason a deleted object stayed servable. + * + * ## Why a second removal verb, and not a call to the first one + * + * Until this method, {@link unregisterObjectsByPackage} was the ONLY way an + * object left `objectContributors`, and it is addressed by PACKAGE. That is + * the right verb for an uninstall and the wrong one for a delete: an + * `object` created at runtime has no package identity of its own (the write + * path keys its contributor by the row's `package_id`, or the + * `'sys_metadata'` sentinel when the row is package-less), so routing a + * single delete through it would mean synthesising an identity and then + * tearing down every SIBLING object registered under it — a far wider blast + * radius than the delete the operator asked for. + * + * The gap it left was load-bearing. A runtime-authored `object` is written + * into TWO places (`metadata['object']` via {@link registerItem} and + * `objectContributors` via {@link registerObject}), and the metadata-protocol + * heal that runs after a `sys_metadata` delete only ever reached the first. + * Measured over the real repository: after `DELETE /meta/object/` the + * row was gone and `metadata['object']` was empty, while `getObject(name)` + * — and therefore `getItem('object', name)`, which special-cases straight + * back to it — kept serving the object for the life of the process. Since + * `getObject` is what the data plane dispatches on + * (`assertObjectRegistered`), the deleted object stayed readable, syncable + * and WRITABLE. + * + * ## The ADR-0029 guard, and why it is the SAME judgement, not a second one + * + * ADR-0029: exactly one package owns an object; others may only `extend` it. + * An extender's fields are merged into the owner's definition + * ({@link resolveObject}), so removing an owned object out from under a live + * extender leaves contributions that resolve to nothing — + * {@link assertSingleOwnerPerObject}'s "has extenders but no owner" + * violation, reached at runtime instead of at bootstrap. + * `unregisterObjectsByPackage` already encodes the answer (refuse, name the + * extenders, force overrides), so this mirrors that judgement rather than + * inventing a second one; only the address changes, from package to name. + * Both facts it needs — the owner and the extenders — are already in the + * contributor list, so no new bookkeeping structure exists to drift. + * + * "Other" means "not the owner's package", the same relation the sibling + * verb expresses as "not the package being uninstalled": an extension the + * OWNER itself contributed goes away with the object it extends, exactly as + * it would on an uninstall. An object with no owner at all (only extenders — + * already an ADR-0029 violation) counts every extender as other, so it is + * refused rather than silently torn down. + * + * @param name Short name (canonical) or FQN — resolved exactly as + * {@link getObject} resolves it, so this removes precisely the entry that + * was being served. + * @param options.force Skip the extender guard. The escape hatch the + * package-scoped verb has, for a caller that has already decided. + * @returns whether an object was removed (`false` = nothing registered + * under that name; removal is idempotent). + * @throws Error naming the extenders when the object is still extended by + * another package and `force` is not set. + */ + unregisterObject(name: string, options: { force?: boolean } = {}): boolean { + const fqn = this.resolveObjectKey(name); + if (fqn === undefined) return false; + const contributors = this.objectContributors.get(fqn) ?? []; + + const owner = contributors.find(c => c.ownership === 'own'); + if (!options.force) { + const otherExtenders = contributors.filter( + c => c.ownership === 'extend' && c.packageId !== owner?.packageId + ); + if (otherExtenders.length > 0) { + throw new Error( + `Cannot unregister object "${fqn}": it is extended by ` + + `${otherExtenders.map(c => c.packageId).join(', ')}. Unregister the extenders first.` + ); + } + } + + // The whole entry goes: the object no longer exists, so no contribution to + // it does either. Leaving the extenders behind would be the owner-less + // state the guard above exists to prevent. + this.objectContributors.delete(fqn); + // The same two invalidations every other contributor mutation performs — + // the merged-object cache would otherwise keep answering `resolveObject` + // for a name with no contributors, and registry-derived caches (the + // engine's roll-up summary index) would never learn the set moved. + this.mergedObjectCache.delete(fqn); + this._objectRevision += 1; + // Namespaces are deliberately NOT touched: a namespace is registered per + // PACKAGE and shared by every object that package ships, so dropping one + // object must not unregister it. `unregisterObjectsByPackage` leaves it + // alone too — `uninstallPackage` owns that half. + this.log( + `[Registry] Unregistered object: ${fqn} ` + + `(${contributors.length} contribution(s), owner ${owner?.packageId ?? '(none)'})` + ); + return true; + } + // ========================================== // Generic Metadata (Non-Object Types) // ==========================================