From 56ceb075306e926f3aae01134fb9adb654001c54 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 10 Aug 2026 02:48:21 +0000 Subject: [PATCH] fix(objectql): MetadataFacade object writes now reach the map its reads use (#6725) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `MetadataFacade.register('object', …)` wrote through `SchemaRegistry.registerItem`, into the generic `metadata` map. Every one of the facade's object reads resolves from `objectContributors`, which only `registerObject` populates: `getObject` goes straight there; `get('object', …)` and `exists` go via `registry.getItem`, which special-cases the object type back to `getObject`; `list`/`listNames` go via `registry.listItems`, which special-cases to `getAllObjects`. So an object written through the public facade was readable back through none of them — `register` resolved and every read answered `undefined`. `IMetadataService` declares `getObject(name)` ≡ `get('object', name)` and its own conformance test round-trips a `register('object', …)` through both members, so this was a shipped contract that could not work. Dormant in-tree only because nothing on `main` installs a `MetadataFacade` into the `metadata` slot. The write now performs both halves of the two-place object write the registry documents (`SchemaRegistry.unregisterObject`'s header; the in-tree precedent is `MetadataProtocol.applyObjectRegistryMutation`): `registerObject` for the contributor entry the reads resolve, plus the existing `registerItem` for the stored document. Both type spellings are covered, since both are special-cased on the read side. The contributor gets a COPY: `applyProtection` stamps in place and `applySystemFields` returns its input unchanged when there is nothing to inject, so a shared reference would have leaked a synthetic package id onto the stored document — what the "never invents a synthetic package id" pin forbids. That pin keeps its direct read of the generic map, because the stored document is what it was written to guard. A package-less object registers under the `'sys_metadata'` sentinel with `_provenance: 'org'`, so it cannot read as code-shipped. `unregister('object', …)` removes both halves too. Without that the fix would have re-opened #6808 from the other side: a removal that empties only the generic map leaves `getObject` — what the data plane dispatches on — serving a deleted object for the life of the process. Refs #6725, #6505, PR #6723, #6808, ADR-0010, ADR-0029. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_0141cZum72My2vskaQSoQ1tZ --- ...metadata-facade-object-write-read-split.md | 67 +++++++ packages/objectql/src/metadata-facade.test.ts | 172 ++++++++++++++++++ packages/objectql/src/metadata-facade.ts | 117 +++++++++++- ...data-service-getobject-equivalence.test.ts | 33 ++-- 4 files changed, 372 insertions(+), 17 deletions(-) create mode 100644 .changeset/metadata-facade-object-write-read-split.md diff --git a/.changeset/metadata-facade-object-write-read-split.md b/.changeset/metadata-facade-object-write-read-split.md new file mode 100644 index 0000000000..5aaf44a4c1 --- /dev/null +++ b/.changeset/metadata-facade-object-write-read-split.md @@ -0,0 +1,67 @@ +--- +'@objectstack/objectql': patch +--- + +**`MetadataFacade.register('object', …)` now writes where its own object reads +look — it was a silent no-op before (#6725).** + +`MetadataFacade` is exported from `@objectstack/objectql`'s root and `core` +entrypoints for hosts that want to occupy the kernel's `metadata` slot with a +`SchemaRegistry`-backed service. Its object write went through +`SchemaRegistry.registerItem`, which stores into the generic `metadata` map — +and **every one of its object reads resolves from `objectContributors`**, which +only `registerObject` populates: + +- `getObject(name)` → `registry.getObject`; +- `get('object', name)` and `exists('object', name)` → `registry.getItem`, which + special-cases the object type straight back to `registry.getObject`; +- `list('object')` and `listNames('object')` → `registry.listItems`, which + special-cases to `registry.getAllObjects`. + +So an object registered through the facade was readable back through **none** of +them: `register` resolved successfully and every subsequent read answered +`undefined` / `[]`. `IMetadataService` (`@objectstack/spec/contracts`) declares +`getObject(name)` ≡ `get('object', name)` and its own conformance test +round-trips a `register('object', …)` through both members, so this was a +shipped contract that could not work. Dormant in-tree only because nothing on +`main` installs a `MetadataFacade` into the `metadata` slot — a downstream host +that did (cloud, a third-party kernel) got the split, including the +write-then-read in ObjectQL's own `bridgeObjectsToMetadataService`, whose +"already registered?" probe would never answer and so re-registered the full +object set on every boot. + +**What changed.** The facade's object write now performs *both* halves of the +two-place write the registry documents for a runtime-authored object +(`SchemaRegistry.unregisterObject`'s header states the invariant; the in-tree +precedent is `MetadataProtocol.applyObjectRegistryMutation`, which does exactly +this): `registerObject` for the contributor entry every read resolves, plus the +existing `registerItem` for the stored document. Both spellings of the type +(`'object'` and `'objects'`) are covered, because both are special-cased on the +read side. + +Consequences a caller can observe: + +- `getObject` / `get` / `exists` / `list` / `listNames` / `listObjects` now + answer a facade-registered object. What they answer is the **runtime-effective** + object the contract promises: system columns injected, primary title + designated, `extend` contributions merged. +- An object arriving with no `_packageId` is runtime-authored by definition, so + it is registered under the platform's `'sys_metadata'` sentinel and stamped + `_provenance: 'org'`. It therefore does **not** read as code-shipped — + `getArtifactItem` / `isArtifactBacked` exclude both — and does not become + un-editable. An object carrying a real `_packageId` is registered under it and + keeps `_provenance: 'package'`. The stored document keeps its as-authored, + unstamped shape; only the contributor copy carries registry coordinates. +- `register('object', …)` can now **throw** where it previously succeeded and did + nothing: claiming an object another package already owns is refused by + ADR-0029. The contributor write runs first so a refusal writes nothing at all. +- `unregister('object', name)` removes the object from both places. Without this + the fix would have re-opened #6808 from the other side — a removal that empties + only the generic map leaves `getObject`, which the data plane dispatches on, + serving a deleted object for the life of the process. It refuses, per ADR-0029, + an object still extended by another package. + +No in-tree caller changes behaviour: `new MetadataFacade(...)` appears nowhere on +`main` outside this package's own tests, and the `metadata` slot is filled by +`MetadataManager` or `createMemoryMetadata`, both of which already round-tripped +correctly. diff --git a/packages/objectql/src/metadata-facade.test.ts b/packages/objectql/src/metadata-facade.test.ts index 850746d434..535a39987e 100644 --- a/packages/objectql/src/metadata-facade.test.ts +++ b/packages/objectql/src/metadata-facade.test.ts @@ -50,9 +50,181 @@ describe('MetadataFacade provenance passthrough', () => { // getItem('object', …) routes to the merged-object path, so read the // generic collection directly to inspect what register() stored. + // + // [#6725] The direct read is STILL the right instrument here, and for + // the same reason as before: this pin is about the STORED document, and + // the two object reads answer the contributor copy — which now exists, + // and which deliberately does carry the `'sys_metadata'` sentinel (see + // the round-trip suite below). Reading through `get('object', …)` would + // silently retarget this assertion at the other document and stop + // guarding what it was written to guard. const stored = (registry as any).metadata.get('object')?.get('task'); expect(stored).toBeDefined(); expect(stored._packageId).toBeUndefined(); expect(stored._provenance).toBeUndefined(); }); + + it('keeps the stored document unstamped even when the contributor copy is stamped', async () => { + // The hazard the copy in `registerObjectBothPlaces` exists for: + // `applyProtection` stamps IN PLACE and `applySystemFields` returns its + // input unchanged when there is nothing to inject (`systemFields: false` + // takes that path), so a shared reference would leak the sentinel into + // the entry the pin above guards. + await facade.register('object', 'nothing_injected', { + name: 'nothing_injected', + label: 'No injection', + fields: {}, + systemFields: false, + }); + + const stored = (registry as any).metadata.get('object')?.get('nothing_injected'); + expect(stored._packageId).toBeUndefined(); + expect(stored._provenance).toBeUndefined(); + + // …while the contributor copy — a different document — is stamped. + expect((registry.getObject('nothing_injected') as any)._packageId).toBe('sys_metadata'); + }); +}); + +/** + * [#6725] The write/read pin. + * + * `MetadataFacade.register('object', …)` wrote through `registerItem` into the + * generic `metadata` map, while every one of this class's object reads resolves + * from `objectContributors` — so an object written through the public facade was + * not readable back through the public facade. `IMetadataService` + * (`@objectstack/spec/contracts`) declares `getObject(name)` ≡ + * `get('object', name)` and its own conformance test round-trips a + * `register('object', …)` through both members; this file is the gate for the + * facade's half of that. + * + * Refs #6725, #6505 / PR #6723, #6808, ADR-0010, ADR-0029. + */ +describe('MetadataFacade object write/read round-trip', () => { + let registry: SchemaRegistry; + let facade: MetadataFacade; + + beforeEach(() => { + registry = new SchemaRegistry({ multiTenant: false }); + facade = new MetadataFacade(registry); + }); + + const taskDefinition = () => ({ name: 'task', label: 'Task', fields: {} }); + + it('reads a registered object back through BOTH getObject and get', async () => { + await facade.register('object', 'task', taskDefinition()); + + const viaGetObject = await facade.getObject('task'); + const viaGet = await facade.get('object', 'task'); + + // Anti-vacuity: before the fix both members answered `undefined`, which + // an identity assertion alone would have called agreement. + expect(viaGetObject).toBeDefined(); + expect((viaGetObject as any).name).toBe('task'); + expect((viaGetObject as any).label).toBe('Task'); + expect(viaGetObject).toBe(viaGet); + }); + + it('reads it back through the enumeration members too', async () => { + await facade.register('object', 'task', taskDefinition()); + + expect(await facade.exists('object', 'task')).toBe(true); + expect(await facade.listNames('object')).toEqual(['task']); + expect(await facade.listObjects()).toHaveLength(1); + const listed = await facade.list('object'); + expect(listed.map((o: any) => o.name)).toEqual(['task']); + }); + + it('closes the same split for the plural `objects` spelling', async () => { + // `registry.getItem` / `listItems` special-case both spellings to the + // contributor path, so a write that handled only the singular left this + // one broken in exactly the same way. + await facade.register('objects', 'lead', { name: 'lead', label: 'Lead', fields: {} }); + + expect(await facade.getObject('lead')).toBeDefined(); + expect(await facade.get('objects', 'lead')).toBeDefined(); + expect(await facade.get('object', 'lead')).toBeDefined(); + }); + + it('serves the runtime-effective object, as the contract says it does', async () => { + // #6505 / PR #6723: `getObject` answers the object as the engine runs + // it, not the document its author wrote. The materialization seam is + // `registerObject`'s, so it only runs now that the write reaches it. + const multiTenantRegistry = new SchemaRegistry({ multiTenant: true }); + const multiTenantFacade = new MetadataFacade(multiTenantRegistry); + + await multiTenantFacade.register('object', 'task', taskDefinition()); + + const effective = (await multiTenantFacade.getObject('task')) as any; + expect(effective.fields.organization_id).toBeDefined(); + expect(effective.fields.created_at).toBeDefined(); + }); + + it('registers a package-less object under the sentinel, not as an artifact', async () => { + await facade.register('object', 'task', taskDefinition()); + + const owner = registry.getObjectOwner('task'); + expect(owner?.packageId).toBe('sys_metadata'); + // ADR-0010: runtime-authored, so it must not read as code-shipped — + // `getArtifactItem` is what write authorization consults. + expect((registry.getObject('task') as any)._provenance).toBe('org'); + expect(registry.getArtifactItem('object', 'task')).toBeUndefined(); + }); + + it('registers a package-stamped object under its own package id', async () => { + await facade.register('object', 'crm_account', { + name: 'crm_account', + label: 'Account', + fields: {}, + _packageId: 'com.example.crm', + }); + + expect(registry.getObjectOwner('crm_account')?.packageId).toBe('com.example.crm'); + const served = registry.getObject('crm_account') as any; + expect(served._packageId).toBe('com.example.crm'); + expect(served._provenance).toBe('package'); + expect(registry.getArtifactItem('object', 'crm_account')).toBeDefined(); + }); + + it('re-registering the same object replaces it rather than accumulating owners', async () => { + await facade.register('object', 'task', taskDefinition()); + await facade.register('object', 'task', { ...taskDefinition(), label: 'Task v2' }); + + expect(((await facade.getObject('task')) as any).label).toBe('Task v2'); + expect(registry.getObjectContributors('task')).toHaveLength(1); + expect(await facade.listObjects()).toHaveLength(1); + }); + + it('refuses to claim an object another package owns, and writes nothing', async () => { + registry.registerObject({ name: 'task', label: 'Owned', fields: {} } as never, 'com.example.owner'); + + // ADR-0029 — one owner per object. The contributor write runs first + // precisely so the refusal leaves the generic map untouched too. + await expect( + facade.register('object', 'task', { ...taskDefinition(), _packageId: 'com.example.other' }), + ).rejects.toThrow(/already owned by package "com.example.owner"/); + + expect((registry as any).metadata.get('object')?.size ?? 0).toBe(0); + expect(((await facade.getObject('task')) as any).label).toBe('Owned'); + }); + + it('unregisters an object out of BOTH places it was written into', async () => { + await facade.register('object', 'task', taskDefinition()); + expect(await facade.getObject('task')).toBeDefined(); + + await facade.unregister('object', 'task'); + + // #6808: removing only the generic-map half left `getObject` — what the + // data plane dispatches on — serving a deleted object for the life of + // the process. + expect(await facade.getObject('task')).toBeUndefined(); + expect(await facade.get('object', 'task')).toBeUndefined(); + expect(await facade.exists('object', 'task')).toBe(false); + expect(await facade.listObjects()).toHaveLength(0); + expect((registry as any).metadata.get('object')?.get('task')).toBeUndefined(); + }); + + it('unregistering an object nothing registered stays a no-op', async () => { + await expect(facade.unregister('object', 'absent')).resolves.toBeUndefined(); + }); }); diff --git a/packages/objectql/src/metadata-facade.ts b/packages/objectql/src/metadata-facade.ts index 18f25129f0..f1f0746a03 100644 --- a/packages/objectql/src/metadata-facade.ts +++ b/packages/objectql/src/metadata-facade.ts @@ -2,6 +2,25 @@ import { SchemaRegistry } from './registry.js'; +/** + * The two spellings of the object metadata type. `SchemaRegistry.getItem` / + * `listItems` special-case BOTH to the contributor path, so a write that + * handled only the singular left the plural with the same read/write split + * (#6725). + */ +function isObjectType(type: string): boolean { + return type === 'object' || type === 'objects'; +} + +/** + * The owning-package id used for an object registered through this facade with + * no `_packageId` of its own. The platform's sentinel for "an overlay row bound + * to no package" — `isArtifactBacked` (metadata-protocol) and + * `SchemaRegistry.getArtifactItem` both exclude it explicitly, so it cannot + * turn a runtime-authored object into an artifact-backed one. + */ +const RUNTIME_AUTHORED_PACKAGE_ID = 'sys_metadata'; + /** * MetadataFacade * @@ -32,13 +51,90 @@ export class MetadataFacade { // definition and must not become artifact-backed (protocol.ts // isArtifactBacked gates write authorization on _packageId). const packageId = definition?._packageId; - if (type === 'object') { - this.registry.registerItem(type, definition, 'name' as any, packageId); + if (isObjectType(type)) { + this.registerObjectBothPlaces(type, definition, packageId); } else { this.registry.registerItem(type, definition, definition.id ? 'id' as any : 'name' as any, packageId); } } + /** + * [#6725] An `object` lives in TWO places in a `SchemaRegistry`, and this + * write has to reach both of them. + * + * `SchemaRegistry.unregisterObject`'s header states the invariant directly: + * "a runtime-authored `object` is written into TWO places (`metadata['object']` + * via `registerItem` and `objectContributors` via `registerObject`)". This + * method used to perform only the first half, so a facade write landed + * nowhere any facade read looks — `registerItem` stores into the generic + * `metadata` map, while EVERY object read resolves from `objectContributors`: + * + * - `getObject` → `registry.getObject` → `objectContributors` + * - `get('object', …)` / `exists` → `registry.getItem`, which special-cases + * the object type straight back to `registry.getObject` + * - `list('object')` / `listNames('object')` → `registry.listItems`, which + * special-cases to `registry.getAllObjects` + * + * So `register('object', …)` was a silent no-op as far as this class's own + * contract is concerned: `IMetadataService` (`@objectstack/spec/contracts`) + * declares `getObject(name)` ≡ `get('object', name)` and its own conformance + * test round-trips a `register('object', …)` through both members. Dormant + * in-tree only because nothing on `main` installs a `MetadataFacade` into the + * `metadata` slot — but the class is exported from this package's root and + * `core` entrypoints, so a downstream host got the split. + * + * The shape mirrors the one in-tree precedent for the same write, + * `MetadataProtocol.applyObjectRegistryMutation` (metadata-protocol), which + * calls `registerItem` AND `registerObject` with `packageId || 'sys_metadata'`. + * Neither half is redundant: the contributor entry is the runtime-effective + * object every read resolves (post-materialization, extensions merged), the + * generic-map entry is the stored document, and the contract's `getObject` + * TSDoc (#6505) already tells consumers those are different things. + * + * ── The contributor copy is a COPY, deliberately ── + * + * `registerObject` runs `applyProtection`, which stamps `_packageId` / + * `_provenance` **in place**, and `applySystemFields` returns its input + * unchanged on the no-injection path (`sys_*`, `systemFields: false`, …). + * Handing it the same reference `registerItem` stores would therefore write a + * synthetic package id onto the generic-map entry — precisely what the + * "never invents a synthetic package id for object registrations" pin in + * `metadata-facade.test.ts` forbids, and what `isArtifactBacked` keys write + * authorization off. The generic-map entry keeps its unstamped, as-authored + * shape; only the contributor copy carries the registry's coordinates. + * + * ── Why `_provenance: 'org'` on the package-less copy ── + * + * `registerObject` demands a package id, and an item that arrived here with no + * `_packageId` is runtime-authored by definition (see `register` above). The + * `'sys_metadata'` sentinel is the id the platform already uses for exactly + * that case. Stamping `'org'` alongside it is not belt-and-braces: without it + * `applyProtection` would default the copy to `_provenance: 'package'` and + * label a runtime-authored object a code artifact — the axis `isTenantAuthored` + * (registry.ts) exists to keep straight, and the misclassification behind + * cloud#970. An item that DID carry a real `_packageId` is registered under it + * and keeps `'package'`, which is true of it. + * + * ── Ordering ── + * + * The contributor write goes first because it is the half that can refuse: + * `registerObject` throws when another package already owns the name + * (ADR-0029). Failing before `registerItem` runs means a refused registration + * writes nothing at all, rather than re-opening the half-written split from + * the other side. + */ + private registerObjectBothPlaces(type: string, definition: any, packageId: string | undefined): void { + this.registry.registerObject( + packageId + ? { ...definition } + : { ...definition, _provenance: 'org' }, + // `||`, not `??`: an empty-string binding is "no package", the same + // normalisation the protocol write path applies. + packageId || RUNTIME_AUTHORED_PACKAGE_ID, + ); + this.registry.registerItem(type, definition, 'name' as any, packageId); + } + /** * Get a metadata item by type and name. * @@ -70,8 +166,25 @@ export class MetadataFacade { /** * Unregister a metadata item + * + * [#6725] An object leaves both places it was written into, for the same + * reason {@link register} writes both: `unregisterItem` only empties the + * generic `metadata` map, which no object read consults. Removing one half is + * the exact shape of #6808 — the row was gone and `metadata['object']` was + * empty while `getObject(name)` kept serving the deleted object for the life + * of the process, and `getObject` is what the data plane dispatches on. Now + * that the write reaches `objectContributors`, a removal that did not would + * make every facade-registered object undeletable through this contract. + * + * `unregisterObject` is idempotent (`false` when nothing is registered under + * the name) and refuses, by design, an object still extended by another + * package — ADR-0029, the same judgement `unregisterObjectsByPackage` + * encodes. It runs first so a refusal removes nothing at all. */ async unregister(type: string, name: string): Promise { + if (isObjectType(type)) { + this.registry.unregisterObject(name); + } this.registry.unregisterItem(type, name); } diff --git a/packages/objectql/src/metadata-service-getobject-equivalence.test.ts b/packages/objectql/src/metadata-service-getobject-equivalence.test.ts index c79571def3..984c90a8b9 100644 --- a/packages/objectql/src/metadata-service-getobject-equivalence.test.ts +++ b/packages/objectql/src/metadata-service-getobject-equivalence.test.ts @@ -18,18 +18,20 @@ * Two things about the shape here are load-bearing, and both are the difference * between this pin and a green-but-empty one: * - * 1. **The facade is seeded through `registry.registerObject`, never through - * `facade.register('object', …)`.** Those are not interchangeable: the facade - * writes objects through `SchemaRegistry.registerItem`, which stores into the - * generic `metadata` map, while BOTH of its object reads resolve from - * `objectContributors` (`registry.getItem` special-cases the `object` type - * straight back to `registry.getObject`). An object written the first way is - * readable back through neither member — measured, both `undefined` — so a pin - * that seeded that way would compare `undefined` to `undefined` and call it - * equivalence. That write/read split is a separate, already-filed finding - * (#6725); this file deliberately does not assert on it, and the - * `expect(...).toBeDefined()` in the present-object case is what stops the - * vacuous version from ever passing here again. + * 1. **The facade is seeded through `registry.registerObject`.** When this pin was + * written that was the ONLY seeding that worked: the facade wrote objects + * through `SchemaRegistry.registerItem`, into the generic `metadata` map, + * while BOTH of its object reads resolve from `objectContributors` + * (`registry.getItem` special-cases the `object` type straight back to + * `registry.getObject`). An object written the first way was readable back + * through neither member — measured, both `undefined` — so a pin seeded that + * way would have compared `undefined` to `undefined` and called it + * equivalence. #6725 has since closed that split (`facade.register('object', + * …)` performs the contributor write too, and pins the round-trip in + * `metadata-facade.test.ts`), so either seeding would work here now; this one + * is kept because it seeds the registry directly, the way a registry-backed + * host actually acquires its objects. The `expect(...).toBeDefined()` in the + * present-object case remains what stops a vacuous version from passing here. * * 2. **MetadataManager appears twice, under both of its resolution paths.** Its * `get` answers from the in-memory registry when it can and falls back to the @@ -163,9 +165,10 @@ const IMPLEMENTATIONS: readonly PinnedImplementation[] = [ async create(objects) { const registry = new SchemaRegistry({ multiTenant: false }); for (const object of objects) { - // NOT `facade.register('object', …)` — that writes where neither of - // the facade's object reads look (#6725), which would make the - // present-object case below compare undefined to undefined. + // Seeded on the registry, not through `facade.register('object', …)` + // — see the header. That write reached neither object read until + // #6725 closed the split; it now would, but this seeding is the + // one a registry-backed host actually performs. registry.registerObject(object.definition as never, 'com.example.pin'); } return new MetadataFacade(registry);