diff --git a/.changeset/metadata-loader-save-contract.md b/.changeset/metadata-loader-save-contract.md new file mode 100644 index 0000000000..3ad8c16c9c --- /dev/null +++ b/.changeset/metadata-loader-save-contract.md @@ -0,0 +1,50 @@ +--- +"@objectstack/metadata": patch +--- + +fix(metadata): `capabilities.write` now also binds `save()` — a writable datasource loader must implement both halves of the write (#5654) + +#5276 (shipped in v17.0.0-rc) made `capabilities.write` binding on `delete()`: +a loader declaring `protocol: 'datasource:'` with `capabilities.write: true` +and no `delete()` is refused at registration, because `unregister()` used to +skip it silently and announce the deletion anyway. The gate stopped there, so +**one declaration was binding at one end of an item's life and decorative at +the other**. + +`MetadataManager.register()` had the identical hole one direction over. Its +persistence loop read `loader.save &&` first, so a `datasource:` loader +declaring `capabilities.write: true` **without** a `save()` method was +**silently skipped** — no warn, no error. `register()` then wrote the in-memory +registry, invalidated the list cache, announced `created`/`updated` and notified +watchers, so the caller (Studio/Setup, REST PUT, the CLI, a package publish) was +told the write succeeded. The item read back correctly for the life of the +process and was **gone at the next restart**, with nothing to retry it — a +durability degradation that leaves the system looking entirely healthy. + +`registerLoader()`'s gate (renamed `assertWritableLoaderContract`) now requires +**both** `save()` and `delete()` for that combination, and rejects with one +message naming which method is missing, the consequence, and both repairs. +`registerLoader()` is the sole writer of the loader map — the constructor's +`config.loaders` funnel through it — so the combination can no longer reach the +runtime and lose a write there. The `save` short-circuit inside `register()` +survives as defensive code whose unreachability is now guaranteed by +construction, exactly like `unregister()`'s. + +**Does this affect you?** Only if you register a custom metadata loader that +declares `protocol: 'datasource:'` with `capabilities.write: true`. If it does +and has no `save()`, registration now throws where it previously succeeded and +quietly discarded your writes. Two ways to fix it, both stated in the error: + +1. implement + `async save(type: string, name: string, data: any, options?: MetadataSaveOptions): Promise` + on the loader, persisting the item into its store (`DatabaseLoader` in this + package is the reference implementation); or +2. if the loader is genuinely read-only, declare `capabilities.write: false` — a + read-only `datasource:` loader registers without complaint and is never + written to in the first place. + +Loaders on the other protocols (`file:`, `memory:`, `http:`, `s3:`) are +unaffected: `MetadataManager` never persists to them at runtime, so they may +declare `capabilities.write` without a `save()`/`delete()` exactly as before. +The one `datasource:` loader shipped in this package, `DatabaseLoader`, has +always implemented both and is unchanged. diff --git a/packages/metadata/src/loaders/loader-interface.ts b/packages/metadata/src/loaders/loader-interface.ts index cbfa2592e0..470bd8505c 100644 --- a/packages/metadata/src/loaders/loader-interface.ts +++ b/packages/metadata/src/loaders/loader-interface.ts @@ -73,7 +73,27 @@ export interface MetadataLoader { list(type: string): Promise; /** - * Save metadata item + * Save metadata item into this loader's store. + * + * [#5654] Optional on the interface, **mandatory for a `datasource:` loader + * that declares `capabilities.write`** — `MetadataManager.registerLoader()` + * refuses to register such a loader when this method is missing, so the + * combination "declared writable, cannot persist" never reaches the runtime. + * + * The reason it is enforced at registration rather than tolerated at the write + * site: `MetadataManager.register()` persists into every writable + * `datasource:` loader, and it used to read `loader.save &&` first — a loader + * that declares it can be written to but has no `save()` made every write a + * silent lie. `register()` would skip it, then write the in-memory registry, + * invalidate the list cache, announce a `created`/`updated` event and notify + * watchers, so the caller is told the write succeeded; the item reads back + * correctly for the life of the process and is **gone at the next restart**, + * with nothing to retry it. + * + * Loaders on the other protocols (`file:`, `memory:`, `http:`, `s3:`) are not + * gated: `MetadataManager` never persists to them at runtime — `register()` + * filters on `datasource:` — so a missing `save()` there loses nothing. + * * @param type The metadata type * @param name The item name * @param data The data to save @@ -89,8 +109,8 @@ export interface MetadataLoader { /** * Delete a metadata item from this loader's store. * - * [#5276] Optional on the interface, **mandatory for a `datasource:` loader - * that declares `capabilities.write`** — `MetadataManager.registerLoader()` + * [#5276, #5654] Optional on the interface, **mandatory for a `datasource:` + * loader that declares `capabilities.write`** — `MetadataManager.registerLoader()` * refuses to register such a loader when this method is missing, so the * combination "declared writable, cannot delete" never reaches the runtime. * @@ -105,6 +125,12 @@ export interface MetadataLoader { * therefore means *both* directions of the write, on both ends of the item's * life — declared = enforced. * + * One gate covers both halves: `assertWritableLoaderContract` in + * `metadata-manager.ts` requires `save()` **and** `delete()` for this + * combination and names whichever is missing. #5276 built it for `delete`; + * #5654 widened it to `save`, which had the identical silent skip in + * `register()` — see the note on `save?` above. + * * Loaders on the other protocols (`file:`, `memory:`, `http:`, `s3:`) are not * gated: `MetadataManager` never writes to them at runtime, so it never has a * deletion of its own to take back. diff --git a/packages/metadata/src/metadata-manager-loader-delete-contract.test.ts b/packages/metadata/src/metadata-manager-loader-delete-contract.test.ts index e409c3ff66..2e7d90afb3 100644 --- a/packages/metadata/src/metadata-manager-loader-delete-contract.test.ts +++ b/packages/metadata/src/metadata-manager-loader-delete-contract.test.ts @@ -35,6 +35,14 @@ * register without a `delete`, because the manager never writes to them; * 5. `DatabaseLoader`, the repo's only real `datasource:` loader, passes the * gate unchanged. + * + * [#5654] The gate this file pins has since been widened — it is + * `assertWritableLoaderContract` now, and `capabilities.write` requires `save()` + * as well, because `register()` had the identical silent skip one direction + * over. Everything below still holds verbatim: these loaders all implement + * `save`, so `delete` is the only thing missing and the message is unchanged. + * The `save` half is pinned next door in + * `metadata-manager-loader-save-contract.test.ts`. */ import { describe, it, expect, vi, beforeEach } from 'vitest'; diff --git a/packages/metadata/src/metadata-manager-loader-save-contract.test.ts b/packages/metadata/src/metadata-manager-loader-save-contract.test.ts new file mode 100644 index 0000000000..e609397756 --- /dev/null +++ b/packages/metadata/src/metadata-manager-loader-save-contract.test.ts @@ -0,0 +1,350 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #5654 — the `save` half of the same gate #5276 built for `delete`. + * + * `register()` persists into every writable `datasource:` loader, and it read + * `loader.save &&` FIRST: a loader declaring `protocol: 'datasource:'` with + * `capabilities.write: true` but implementing no `save()` was **silently + * skipped** — no warn, no error, nothing. `register()` then wrote the in-memory + * registry, invalidated the list cache, announced `created`/`updated` and + * notified watchers, so the caller was told the write succeeded. The item read + * back correctly for the life of the process and was gone at the next restart, + * with nothing to retry it. Durability degradation of the exact shape AGENTS.md + * → "Degradation log levels" says must not even be a `warn`. + * + * #5276 (PR #5652) closed the mirror-image hole on `delete` and deliberately + * stopped there, which left ONE declaration binding at one end of an item's + * life and decorative at the other: + * + * - delete side: declared = enforced (registration throws); + * - save side: declared ≠ enforced (registration passes, the write is lost). + * + * The fix is the same cure one direction over — `registerLoader()`'s gate, + * renamed `assertWritableLoaderContract`, now requires BOTH methods and says so + * in one message. `register()`'s `save` short-circuit survives as defensive + * code whose unreachability is guaranteed by construction, exactly like + * `unregister()`'s. + * + * What these tests pin: + * 1. the rejection of a `save`-less writable datasource loader, on both entry + * points (constructor config and the direct `registerLoader()` call), + * including that nothing is half-registered; + * 2. the message is actionable — it names the loader, the consequence, and + * both repairs — and is the ONE text `buildWritableLoaderMissingMethodsMessage` + * builds, quoted rather than paraphrased; + * 3. a loader missing BOTH methods is rejected once, naming both; + * 4. the positive case is untouched and really durable: a loader with both + * methods registers, `register()` reaches its store, and the row is still + * there for a fresh manager over the same store — the "restart" the silent + * skip used to lose; + * 5. the gate's scope is exactly the combination `register()` acts on — a + * read-only `datasource:` loader and every non-`datasource:` protocol + * register without a `save`, because the manager never persists to them; + * 6. `DatabaseLoader`, the repo's only real `datasource:` loader, passes the + * widened gate unchanged. + */ + +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import type { + MetadataLoadResult, + MetadataLoaderContract, + MetadataSaveResult, + MetadataStats, +} from '@objectstack/spec/system'; +import type { IDataDriver } from '@objectstack/spec/contracts'; +import { MetadataManager, buildWritableLoaderMissingMethodsMessage } from './metadata-manager.js'; +import { DatabaseLoader } from './loaders/database-loader.js'; +import type { MetadataLoader } from './loaders/loader-interface.js'; + +const logger = vi.hoisted(() => ({ + info: vi.fn(), + warn: vi.fn(), + error: vi.fn(), + debug: vi.fn(), +})); + +vi.mock('@objectstack/core', () => ({ + createLogger: () => logger, +})); + +type Protocol = MetadataLoaderContract['protocol']; + +/** + * A loader whose contract is dictated per test and whose `save`/`delete` are + * present or absent on demand — the axes the gate reads, and nothing else. The + * backing store is shared by reference so a second manager can be pointed at it + * to play "restart". + */ +function makeLoader(opts: { + name: string; + protocol: Protocol; + write: boolean; + withSave: boolean; + withDelete: boolean; + store?: Map; +}): MetadataLoader & { + store: Map; + saveCalls: Array<[string, string]>; + deleteCalls: Array<[string, string]>; +} { + const saveCalls: Array<[string, string]> = []; + const deleteCalls: Array<[string, string]> = []; + const store = opts.store ?? new Map(); + const key = (type: string, name: string) => `${type}/${name}`; + + const loader: MetadataLoader & { + store: Map; + saveCalls: Array<[string, string]>; + deleteCalls: Array<[string, string]>; + } = { + contract: { + name: opts.name, + protocol: opts.protocol, + capabilities: { read: true, write: opts.write, watch: false, list: true }, + }, + store, + saveCalls, + deleteCalls, + async load(type: string, name: string): Promise { + const data = store.get(key(type, name)); + return data === undefined ? { data: null } : { data }; + }, + async loadMany(): Promise { + return Array.from(store.values()) as T[]; + }, + async exists(type: string, name: string): Promise { + return store.has(key(type, name)); + }, + async stat(): Promise { + return null; + }, + async list(): Promise { + return []; + }, + }; + + if (opts.withSave) { + loader.save = async (type: string, name: string, data: unknown): Promise => { + saveCalls.push([type, name]); + store.set(key(type, name), data); + return { success: true }; + }; + } + + if (opts.withDelete) { + loader.delete = async (type: string, name: string): Promise => { + deleteCalls.push([type, name]); + store.delete(key(type, name)); + }; + } + + return loader; +} + +/** Read the manager's private loader map — the thing registration writes. */ +const registeredLoaderNames = (mgr: MetadataManager): string[] => + Array.from((mgr as unknown as { loaders: Map }).loaders.keys()); + +const messageOf = (run: () => unknown): string => { + try { + run(); + } catch (error) { + return error instanceof Error ? error.message : String(error); + } + throw new Error('expected the call to throw, but it returned normally'); +}; + +beforeEach(() => { + logger.info.mockClear(); + logger.warn.mockClear(); + logger.error.mockClear(); + logger.debug.mockClear(); +}); + +describe('a `datasource:` loader that declares `capabilities.write` MUST implement `save()`', () => { + const unsavable = () => + makeLoader({ + name: 'half_writable_store', + protocol: 'datasource:', + write: true, + withSave: false, + withDelete: true, + }); + + it('registerLoader() throws rather than accepting a loader it can never persist into', () => { + const mgr = new MetadataManager({ formats: ['json'], loaders: [] }); + + expect(() => mgr.registerLoader(unsavable())).toThrow(/half_writable_store/); + }); + + it('…and nothing is half-registered — the rejected loader is not in the map', () => { + const mgr = new MetadataManager({ formats: ['json'], loaders: [] }); + + expect(() => mgr.registerLoader(unsavable())).toThrow(); + expect(registeredLoaderNames(mgr)).not.toContain('half_writable_store'); + }); + + it('the constructor rejects it too — `config.loaders` is not a back door', () => { + expect( + () => new MetadataManager({ formats: ['json'], loaders: [unsavable()] }), + ).toThrow(/half_writable_store/); + }); + + it('the message names the loader, the consequence, and BOTH repairs', () => { + const mgr = new MetadataManager({ formats: ['json'], loaders: [] }); + const message = messageOf(() => mgr.registerLoader(unsavable())); + + // Which loader, and what it declared. + expect(message).toContain('half_writable_store'); + expect(message).toContain("protocol: 'datasource:'"); + expect(message).toContain('capabilities.write: true'); + expect(message).toContain('implements no `save()` method'); + // The consequence: the write is announced, never lands, and vanishes at + // restart while everything keeps looking healthy. + expect(message).toContain('`register()`'); + expect(message).toContain('`created`/`updated`'); + expect(message).toContain('gone at the next restart'); + // Repair A — implement it. Repair B — stop declaring the capability. + expect(message).toContain('save(type: string, name: string, data: any'); + expect(message).toContain('capabilities.write: false'); + // …and it is the one text the builder owns, not a paraphrase of it. + expect(message).toBe(buildWritableLoaderMissingMethodsMessage('half_writable_store', ['save'])); + }); + + it('a loader missing BOTH methods is rejected once, naming both', () => { + const mgr = new MetadataManager({ formats: ['json'], loaders: [] }); + const neither = makeLoader({ + name: 'inert_store', + protocol: 'datasource:', + write: true, + withSave: false, + withDelete: false, + }); + + const message = messageOf(() => mgr.registerLoader(neither)); + + expect(message).toContain('implements neither a `save()` nor a `delete()` method'); + expect(message).toContain('save(type: string, name: string, data: any'); + expect(message).toContain('delete(type: string, name: string)'); + expect(message).toBe( + buildWritableLoaderMissingMethodsMessage('inert_store', ['save', 'delete']), + ); + }); + + it('the `delete`-only rejection #5276 shipped still reads exactly as it did', () => { + const mgr = new MetadataManager({ formats: ['json'], loaders: [] }); + const undeletable = makeLoader({ + name: 'append_only_store', + protocol: 'datasource:', + write: true, + withSave: true, + withDelete: false, + }); + + const message = messageOf(() => mgr.registerLoader(undeletable)); + + expect(message).toContain('implements no `delete()` method'); + expect(message).toContain('`unregister()`'); + expect(message).toContain('`deleted`'); + expect(message).not.toContain('`save()`'); + }); +}); + +describe('the positive case is untouched — and the write is really durable', () => { + it('a loader with both methods registers, is written to, and survives a restart', async () => { + const store = new Map(); + const writable = makeLoader({ + name: 'writable_store', + protocol: 'datasource:', + write: true, + withSave: true, + withDelete: true, + store, + }); + + const mgr = new MetadataManager({ formats: ['json'], loaders: [writable] }); + expect(registeredLoaderNames(mgr)).toContain('writable_store'); + + await mgr.register('object', 'account', { name: 'account' }); + expect(writable.saveCalls).toEqual([['object', 'account']]); + + // The restart the silent skip used to lose: a fresh manager holding an + // empty registry, reading the same store. + const afterRestart = new MetadataManager({ + formats: ['json'], + loaders: [ + makeLoader({ + name: 'writable_store', + protocol: 'datasource:', + write: true, + withSave: true, + withDelete: true, + store, + }), + ], + }); + expect(await afterRestart.get('object', 'account')).toEqual({ name: 'account' }); + + // …and the other half of the contract still works through the same gate. + await mgr.unregister('object', 'account'); + expect(writable.deleteCalls).toEqual([['object', 'account']]); + expect(store.has('object/account')).toBe(false); + }); +}); + +describe('the gate covers exactly the combination `register()` acts on', () => { + it('a read-only `datasource:` loader needs no `save` — nothing ever writes to it', async () => { + const readOnly = makeLoader({ + name: 'reporting_replica', + protocol: 'datasource:', + write: false, + withSave: false, + withDelete: false, + }); + + const mgr = new MetadataManager({ formats: ['json'], loaders: [readOnly] }); + expect(registeredLoaderNames(mgr)).toContain('reporting_replica'); + + await expect(mgr.register('object', 'account', { name: 'account' })).resolves.toBeUndefined(); + expect(readOnly.saveCalls).toEqual([]); + }); + + it.each(['file:', 'memory:', 'http:', 's3:'])( + 'a `%s` loader may declare write without a `save` — the manager never persists there', + (protocol) => { + const loader = makeLoader({ + name: `loader_${protocol.replace(':', '')}`, + protocol, + write: true, + withSave: false, + withDelete: false, + }); + + const mgr = new MetadataManager({ formats: ['json'], loaders: [] }); + expect(() => mgr.registerLoader(loader)).not.toThrow(); + expect(registeredLoaderNames(mgr)).toContain(loader.contract.name); + }, + ); +}); + +describe('regression — the real `datasource:` loader is unaffected', () => { + /** + * `DatabaseLoader` declares `datasource:` + `capabilities.write` and has + * implemented both `save()` and `delete()` all along; the widened gate must + * be a no-op for it. The driver is a stub because registration touches no + * storage — construction and the contract are the whole surface here. + */ + it('DatabaseLoader registers under the widened gate', () => { + const loader = new DatabaseLoader({ driver: {} as IDataDriver }); + + expect(loader.contract.protocol).toBe('datasource:'); + expect(loader.contract.capabilities.write).toBe(true); + expect(typeof loader.save).toBe('function'); + expect(typeof loader.delete).toBe('function'); + + const mgr = new MetadataManager({ formats: ['json'], loaders: [] }); + expect(() => mgr.registerLoader(loader)).not.toThrow(); + expect(registeredLoaderNames(mgr)).toContain('database'); + }); +}); diff --git a/packages/metadata/src/metadata-manager.ts b/packages/metadata/src/metadata-manager.ts index 3a82a53b8b..7e70e8b2f7 100644 --- a/packages/metadata/src/metadata-manager.ts +++ b/packages/metadata/src/metadata-manager.ts @@ -79,27 +79,73 @@ import type { ApiEndpointMatch } from '@objectstack/spec/contracts'; export type WatchCallback = (event: MetadataWatchEvent) => void | Promise; /** - * [#5276] The registration gate's message for a loader that declares it can be - * written to but cannot be deleted from. + * [#5654] The two methods `capabilities.write` promises on a `datasource:` + * loader, in the order an item meets them: written by + * {@link MetadataManager.register}, taken back out by + * {@link MetadataManager.unregister}. Both are `?` on {@link MetadataLoader} + * because the other protocols legitimately have neither. + */ +export type WritableLoaderMethod = 'save' | 'delete'; + +const WRITABLE_LOADER_METHODS: readonly WritableLoaderMethod[] = ['save', 'delete']; + +/** How the message names each method, and what the author has to write. */ +const WRITABLE_LOADER_METHOD_SIGNATURE: Record = { + save: 'save(type: string, name: string, data: any, options?: MetadataSaveOptions): Promise', + delete: 'delete(type: string, name: string): Promise', +}; + +/** + * [#5276, #5654] The registration gate's message for a loader that declares it + * can be written to but cannot actually carry out one (or both) halves of that + * write. * * Built here rather than inline so the gate and its tests quote one text, in the * shape AGENTS.md → "Degradation log levels" asks of a loud failure: the * **consequence** (concretely, and that the system keeps looking healthy) and * the **fix** (both ways out, so the author does not have to guess which one * their loader wants). + * + * `missing` is the subset of {@link WRITABLE_LOADER_METHODS} the loader does not + * implement; each one contributes its own consequence sentence, because the two + * failures are durability failures in opposite directions — a `save`-less loader + * loses the row that was never written, a `delete`-less one keeps the row that + * was supposed to go. */ -export function buildWritableLoaderMissingDeleteMessage(loaderName: string): string { +export function buildWritableLoaderMissingMethodsMessage( + loaderName: string, + missing: readonly WritableLoaderMethod[], +): string { + const missingPhrase = + missing.length === 2 + ? 'implements neither a `save()` nor a `delete()` method' + : `implements no \`${missing[0]}()\` method`; + + const consequences = missing.map((method) => + method === 'save' + ? 'Registered as-is, every write would be a silent lie: `register()` skips a loader that cannot save, then ' + + 'writes the in-memory registry, invalidates the list cache, announces a `created`/`updated` event and ' + + 'notifies watchers, so the caller (Studio/Setup, REST PUT, the CLI, a package publish) is told the write ' + + `succeeded while nothing ever reaches \`${loaderName}\` — the item reads back correctly for the life of ` + + 'this process and is gone at the next restart, with nothing to retry it. ' + : 'Registered as-is, every deletion would be a silent lie: `unregister()` skips a loader that cannot delete, ' + + 'then drops the registry entry, invalidates the list cache and announces a `deleted` event, so the caller ' + + '(Studio/Setup, REST DELETE, the CLI, a package teardown) is told the delete succeeded while the row stays ' + + `in \`${loaderName}\` and is read straight back out by the very next \`list()\`/\`get()\` — across ` + + 'restarts, with nothing to retry it. ', + ); + + const repair = missing + .map((method) => `\`${WRITABLE_LOADER_METHOD_SIGNATURE[method]}\``) + .join(' and '); + return ( `[MetadataManager] Refusing to register metadata loader \`${loaderName}\`: it declares ` + - "`protocol: 'datasource:'` with `capabilities.write: true` but implements no `delete()` method. " + + `\`protocol: 'datasource:'\` with \`capabilities.write: true\` but ${missingPhrase}. ` + 'A write-capable datasource loader is written to AND deleted from — `register()` persists every item into it, ' + 'and `unregister()` has to take those rows back out again. ' + - 'Registered as-is, every deletion would be a silent lie: `unregister()` skips a loader that cannot delete, then ' + - 'drops the registry entry, invalidates the list cache and announces a `deleted` event, so the caller ' + - '(Studio/Setup, REST DELETE, the CLI, a package teardown) is told the delete succeeded while the row stays in ' + - `\`${loaderName}\` and is read straight back out by the very next \`list()\`/\`get()\` — across restarts, with ` + - 'nothing to retry it. ' + - `Fix: either implement \`delete(type: string, name: string): Promise\` on \`${loaderName}\` ` + + consequences.join('') + + `Fix: either implement ${repair} on \`${loaderName}\` ` + '(`DatabaseLoader` in this package is the reference implementation), or, if the loader is genuinely read-only, ' + "declare `capabilities.write: false` — a read-only `datasource:` loader registers without complaint and is " + 'never written to in the first place.' @@ -107,30 +153,36 @@ export function buildWritableLoaderMissingDeleteMessage(loaderName: string): str } /** - * [#5276] Registration gate: a `datasource:` loader that declares - * `capabilities.write` MUST implement `delete()`. + * [#5276, #5654] Registration gate: a `datasource:` loader that declares + * `capabilities.write` MUST implement **both** `save()` and `delete()`. * - * `capabilities.write` used to mean two different things at the two ends of an - * item's life — "persist into me" to {@link MetadataManager.register}, and - * nothing at all to {@link MetadataManager.unregister}, which duck-typed - * `delete` at the call site and **silently skipped** a loader that had none - * before announcing the deletion anyway. That is the declared ≠ enforced shape - * (Prime Directive #10), and the cure it prescribes is to enforce the - * declaration, not to tolerate the gap: the loader is rejected at registration, - * where the author is standing, instead of losing a row at delete time in a - * deployment nobody is watching. + * `capabilities.write` used to mean three different things at the three places + * it is read — "persist into me" to {@link MetadataManager.register}, which + * duck-typed `save` and **silently skipped** a loader that had none; nothing at + * all to {@link MetadataManager.unregister}, which did the same with `delete`; + * and, on the contract, a flat promise that the store can be written. That is + * the declared ≠ enforced shape (Prime Directive #10), and the cure it + * prescribes is to enforce the declaration, not to tolerate the gap: the loader + * is rejected at registration, where the author is standing, instead of losing a + * row at write or delete time in a deployment nobody is watching. * - * Scope is deliberately exactly the combination `unregister()` acts on. Other - * protocols (`file:`, `memory:`, `http:`, `s3:`) are never written to by the - * manager at runtime — `register()` filters on `datasource:` too — so they have - * no deletion of their own to take back and are not gated. A `datasource:` - * loader with `capabilities.write: false` is likewise untouched by both paths. + * #5276 closed the `delete` half; #5654 closed the `save` half, which was the + * same shape one direction over — and leaving it open meant one declaration was + * binding at one end of an item's life and decorative at the other. + * + * Scope is deliberately exactly the combination `register()`/`unregister()` act + * on. Other protocols (`file:`, `memory:`, `http:`, `s3:`) are never written to + * by the manager at runtime — both loops filter on `datasource:` too — so they + * have neither a write nor a deletion of their own to lose and are not gated. A + * `datasource:` loader with `capabilities.write: false` is likewise untouched by + * both paths. */ -function assertWritableLoaderCanDelete(loader: MetadataLoader): void { +function assertWritableLoaderContract(loader: MetadataLoader): void { const { name, protocol, capabilities } = loader.contract; if (protocol !== 'datasource:' || capabilities.write !== true) return; - if (typeof loader.delete === 'function') return; - throw new Error(buildWritableLoaderMissingDeleteMessage(name)); + const missing = WRITABLE_LOADER_METHODS.filter((method) => typeof loader[method] !== 'function'); + if (missing.length === 0) return; + throw new Error(buildWritableLoaderMissingMethodsMessage(name, missing)); } /** @@ -600,14 +652,15 @@ export class MetadataManager implements IMetadataService { /** * Register a new metadata loader (data source) * - * [#5276] Rejects — loudly, before the loader is stored — a `datasource:` - * loader that declares `capabilities.write` without implementing `delete()`. - * This is the **only** way into `this.loaders` (the constructor's - * `config.loaders` come through here too), which is what lets every later - * delete-capability guard be defensive rather than load-bearing. + * [#5276, #5654] Rejects — loudly, before the loader is stored — a + * `datasource:` loader that declares `capabilities.write` without + * implementing `save()` **and** `delete()`. This is the **only** way into + * `this.loaders` (the constructor's `config.loaders` come through here too), + * which is what lets every later write-capability guard be defensive rather + * than load-bearing. */ registerLoader(loader: MetadataLoader) { - assertWritableLoaderCanDelete(loader); + assertWritableLoaderContract(loader); this.loaders.set(loader.contract.name, loader); this.logger.info(`Registered metadata loader: ${loader.contract.name} (${loader.contract.protocol})`); } @@ -662,9 +715,18 @@ export class MetadataManager implements IMetadataService { // FilesystemLoader is read-only at runtime — writing to it can crash in // read-only environments (e.g. serverless, containerized deployments). for (const loader of this.loaders.values()) { - if (loader.save && loader.contract.protocol === 'datasource:' && loader.contract.capabilities.write) { - await loader.save(type, name, data); - } + if (loader.contract.protocol !== 'datasource:' || !loader.contract.capabilities.write) continue; + // [#5654] Defensive only — unreachable for a registered loader, and read + // in this order on purpose: the protocol/capability test is the policy, + // the method test is the type narrowing. This exact combination + // (`datasource:` + `capabilities.write`, no `save`) is rejected by + // `registerLoader()`, the sole writer of `this.loaders`, so reaching this + // `continue` would mean a loader entered the map without passing the + // gate. Kept because the alternative is a TypeError on the line below, + // and because `save?` stays optional on the interface for the protocols + // the gate does not cover. Mirrors the same guard in `unregister()`. + if (typeof loader.save !== 'function') continue; + await loader.save(type, name, data); } // Publish metadata.{type}.created / .updated event to realtime service.