From f5e55ff2042a7b0047dce6b8c9de45f932254d46 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 2 Sep 2026 02:19:13 +0000 Subject: [PATCH 1/2] fix(metadata): key loader-held items by the row key, not by body.name (#14205) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `readListUncached()` and its no-catch sibling `listForIndex()` merged each loader's answer keyed by `body.name`, admitting an item ONLY when the stored body carried a string `name`. A metadata body is not required to name itself: `register(type, name, data)` takes the key as its ARGUMENT, and `assertMetadataRegisterContract` explicitly allows a document with no `name` of its own. An aggregated `defineView` container is that shape by design — its identity is the target object, carried in the row's `name` COLUMN — so a container written by `register('view', OBJECT, container)` vanished from `list('view')` at the next restart, with `listDiagnosed()` calling the short answer complete. A loader-held item's identity is the key its store holds it under, so the manager asks the loader for that key instead of guessing it from the body: `MetadataLoader` gains an OPTIONAL `loadManyKeyed()` returning (name, body) pairs, implemented by `DatabaseLoader` (row `name` column) and `MemoryLoader` (storage map key). The key travels beside the body and is never folded into it, so nothing synthesises a `name` and the register contract's `data.name` check is untouched. Loaders without the method keep the previous keying verbatim. `MetadataManager.loadMany()` is deliberately unchanged: its `body.name` test is a de-duplication guard, not an admission gate, so it never had this defect. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_0112hMx9hjJ9BgB28X97DS68 --- .changeset/loader-item-row-key-identity.md | 46 +++ packages/metadata/src/index.ts | 2 +- .../metadata/src/loaders/database-loader.ts | 67 ++++- .../metadata/src/loaders/loader-interface.ts | 56 ++++ .../metadata/src/loaders/memory-loader.ts | 20 +- ...tadata-manager-degraded-list-cache.test.ts | 36 ++- .../metadata-manager-list-diagnosed.test.ts | 28 +- ...nager-loader-item-row-key-identity.test.ts | 280 ++++++++++++++++++ packages/metadata/src/metadata-manager.ts | 83 +++++- 9 files changed, 583 insertions(+), 35 deletions(-) create mode 100644 .changeset/loader-item-row-key-identity.md create mode 100644 packages/metadata/src/metadata-manager-loader-item-row-key-identity.test.ts diff --git a/.changeset/loader-item-row-key-identity.md b/.changeset/loader-item-row-key-identity.md new file mode 100644 index 0000000000..5e6e3fb144 --- /dev/null +++ b/.changeset/loader-item-row-key-identity.md @@ -0,0 +1,46 @@ +--- +"@objectstack/metadata": patch +--- + +fix(metadata): key loader-held items by the row key they were stored under, so a body with no top-level `name` is no longer dropped from `list()` (#14205) + +`MetadataManager.readListUncached()` — and its no-catch sibling +`listForIndex()`, which builds the endpoint index — merged each loader's answer +into the result set keyed by `body.name`, and admitted an item ONLY when the +stored body carried a string `name`. + +A metadata body is not required to name itself. `register(type, name, data)` +takes the key as its ARGUMENT, and `assertMetadataRegisterContract` says so in +as many words: "A document with NO `name` of its own is fine — the argument is +the key". An aggregated `defineView` container is exactly that shape — no own +`name` by design, its identity being the target object, carried in the row's +`name` COLUMN — and `DatabaseLoader.rowToData()` returns the stored body +without folding the column into it. + +So a container written by `register('view', OBJECT, container)` lived in the +registry for the life of the process and was written to `sys_metadata`, and +then **disappeared at the next restart**: cold registry, only the loader +answering, and `list('view')` refused the row. `listDiagnosed()` reported that +short answer as complete (`degraded: false`) because no loader had thrown. Not +scoped to views — any loader-held body with no top-level `name` was invisible. + +**The repair.** A loader-held item's identity is the key its store holds it +under, so the manager now asks the loader for that key rather than guessing it +from the body: `MetadataLoader` gains an OPTIONAL `loadManyKeyed()` returning +`(name, body)` pairs, implemented by `DatabaseLoader` (from the row's `name` +column) and `MemoryLoader` (from its storage map key). The key travels BESIDE +the body and is never folded into it, so nothing synthesises a `name` into a +body that deliberately has none and the register contract's refusal of a +disagreeing `data.name` keeps meaning what it says. + +**Nothing consumers see today changes shape.** For any item that went through +`register()`, a `data.name` that exists is required to equal the key, so the +keyed merge produces the identical entry; what is new is only the items the old +gate refused. `loadManyKeyed()` is optional, and a loader without it (a +`RemoteLoader`, whose wire format carries bodies only) falls back to the +previous `body.name` keying unchanged — so no implementor of the published +`MetadataLoader` interface needs to change. + +`MetadataManager.loadMany()` is deliberately untouched: its `body.name` test is +a de-duplication guard, not an admission gate — a nameless item already fell +past it and was returned — so it never carried this defect. diff --git a/packages/metadata/src/index.ts b/packages/metadata/src/index.ts index e976446a26..9580306c2c 100644 --- a/packages/metadata/src/index.ts +++ b/packages/metadata/src/index.ts @@ -14,7 +14,7 @@ export { MetadataManager, type WatchCallback, type MetadataManagerOptions } from export { MetadataPlugin } from './plugin.js'; // Loaders -export { type MetadataLoader } from './loaders/loader-interface.js'; +export { type MetadataLoader, type MetadataKeyedItem } from './loaders/loader-interface.js'; export { MemoryLoader } from './loaders/memory-loader.js'; export { RemoteLoader } from './loaders/remote-loader.js'; export { DatabaseLoader, type DatabaseLoaderOptions } from './loaders/database-loader.js'; diff --git a/packages/metadata/src/loaders/database-loader.ts b/packages/metadata/src/loaders/database-loader.ts index 92b0b5e829..9e1f2aa097 100644 --- a/packages/metadata/src/loaders/database-loader.ts +++ b/packages/metadata/src/loaders/database-loader.ts @@ -23,7 +23,7 @@ import { SysMetadataObject, SysMetadataHistoryObject } from '@objectstack/metada import { applyConversionsToStoredItem } from '@objectstack/spec'; import { PLURAL_TO_SINGULAR } from '@objectstack/spec/shared'; import type { IDataDriver, IDataEngine, DriverQuery } from '@objectstack/spec/contracts'; -import type { MetadataLoader } from './loader-interface.js'; +import type { MetadataLoader, MetadataKeyedItem } from './loader-interface.js'; import { calculateChecksum } from '../utils/metadata-history-utils.js'; import { LRUCache } from '../utils/lru-cache.js'; // [#13279] Both predicates moved to `@objectstack/types` — see its @@ -870,15 +870,29 @@ export class DatabaseLoader implements MetadataLoader { } } - async loadMany( - type: string, - _options?: MetadataLoadOptions - ): Promise { + /** + * The one type-wide read both plural readers share: every row of `type`, each + * body paired with the `name` COLUMN it was stored under. + * + * [#14205] `name` is `null` only for a row whose key column does not hold a + * string. Such a row is still a body {@link loadMany} must return — dropping + * it would change what consumers see today — but it has no usable identity, + * so {@link loadManyKeyed} filters it out rather than invent one. + * + * One query and one cache entry serve both methods: `loadMany()` used to own + * them, and splitting them would have made every keyed `list()` read miss the + * cache and re-hit the database. + */ + private async readTypeRows( + type: string + ): Promise }>> { await this.ensureSchema(); if (this.loadManyCache) { const cached = this.loadManyCache.get(type); - if (cached !== undefined) return cached as T[]; + if (cached !== undefined) { + return cached as Array<{ name: string | null; data: Record }>; + } } try { @@ -886,9 +900,13 @@ export class DatabaseLoader implements MetadataLoader { where: this.baseFilter(type), }); - const result = rows - .map(row => this.rowToData(row)) - .filter((data): data is Record => data !== null) as T[]; + const result: Array<{ name: string | null; data: Record }> = []; + for (const row of rows) { + const data = this.rowToData(row); + if (data === null) continue; + const name = row.name; + result.push({ name: typeof name === 'string' && name !== '' ? name : null, data }); + } this.loadManyCache?.set(type, result); return result; @@ -899,6 +917,37 @@ export class DatabaseLoader implements MetadataLoader { } } + async loadMany( + type: string, + _options?: MetadataLoadOptions + ): Promise { + return (await this.readTypeRows(type)).map(entry => entry.data) as T[]; + } + + /** + * [#14205] The keyed half of {@link loadMany} — see + * {@link MetadataKeyedItem} for why the row key travels beside the body + * instead of inside it. + * + * `DatabaseLoader` is where the defect was measured: an aggregated view + * container is written by `register('view', OBJECT, container)` and stored + * verbatim, so its `sys_metadata` row carries the identity in the `name` + * COLUMN and the body has none. {@link rowToData} returns that body without + * folding the column in — deliberately, and unchanged here. + */ + async loadManyKeyed( + type: string, + _options?: MetadataLoadOptions + ): Promise[]> { + const entries = await this.readTypeRows(type); + const keyed: MetadataKeyedItem[] = []; + for (const entry of entries) { + if (entry.name === null) continue; + keyed.push({ name: entry.name, data: entry.data as T }); + } + return keyed; + } + async exists(type: string, name: string): Promise { await this.ensureSchema(); diff --git a/packages/metadata/src/loaders/loader-interface.ts b/packages/metadata/src/loaders/loader-interface.ts index 470bd8505c..990ed402b5 100644 --- a/packages/metadata/src/loaders/loader-interface.ts +++ b/packages/metadata/src/loaders/loader-interface.ts @@ -15,6 +15,30 @@ import type { MetadataSaveResult, } from '@objectstack/spec/system'; +/** + * [#14205] One loaded item paired with the KEY its store holds it under. + * + * The pair exists because a metadata body is not required to name itself. Most + * do — and for those the key and `data.name` agree, because + * `assertMetadataRegisterContract` refuses a `register(type, name, data)` whose + * `data.name` disagrees with the `name` argument. But an aggregated `defineView` + * container has no own `name` BY DESIGN (its identity is the target object), and + * `register()` explicitly allows that: "A document with NO `name` of its own is + * fine — the argument is the key". + * + * So the key is a fact about the STORE, not about the body, and it is the only + * identity a nameless item has. Carrying it BESIDE `data` rather than folding it + * into `data` is the whole point: the body stays byte-identical to what was + * stored, so no consumer sees a synthesised `name` and the register contract's + * `data.name` check keeps meaning what it means. + */ +export interface MetadataKeyedItem { + /** The key this item is stored under — `register()`'s `name` argument. */ + readonly name: string; + /** The stored body, exactly as {@link MetadataLoader.loadMany} would return it. */ + readonly data: T; +} + /** * Abstract interface for metadata loaders * Implementations can load from filesystem, HTTP, S3, databases, etc. @@ -49,6 +73,38 @@ export interface MetadataLoader { options?: MetadataLoadOptions ): Promise; + /** + * Load multiple items of a type, each paired with the KEY this loader holds + * it under. + * + * [#14205] Optional, and the reason it is a second method rather than a + * widened `loadMany()`: `MetadataLoader` is exported from this package's + * public entry, with implementors outside it (`packages/objectql`'s + * conformance fixtures among them). Changing `loadMany()`'s return type would + * break every one of them; an optional member breaks none, and a loader that + * cannot produce keys — `RemoteLoader`, whose wire format carries bodies only + * — simply does not declare it. + * + * `MetadataManager` prefers this method wherever it merges a loader's answer + * into a keyed set (`list()`, and the endpoint index), and falls back to + * `loadMany()` keyed by `data.name` when it is absent. That fallback is + * exactly the pre-#14205 behaviour, so it drops items whose body has no + * top-level `name`: implement this method on any loader that can be asked to + * hold one. + * + * `data` MUST be the same body `loadMany()` would return for the item — + * unmodified, in particular with no `name` folded in. `name` is the store's + * key, carried beside the body, never written into it. + * + * @param type The metadata type + * @param options Load options with patterns + * @returns Array of (key, body) pairs + */ + loadManyKeyed?( + type: string, + options?: MetadataLoadOptions + ): Promise[]>; + /** * Check if item exists * @param type The metadata type diff --git a/packages/metadata/src/loaders/memory-loader.ts b/packages/metadata/src/loaders/memory-loader.ts index 6a0a5e801c..aec7bd8fcc 100644 --- a/packages/metadata/src/loaders/memory-loader.ts +++ b/packages/metadata/src/loaders/memory-loader.ts @@ -15,7 +15,7 @@ import type { MetadataSaveOptions, MetadataSaveResult, } from '@objectstack/spec/system'; -import type { MetadataLoader } from './loader-interface.js'; +import type { MetadataLoader, MetadataKeyedItem } from './loader-interface.js'; export class MemoryLoader implements MetadataLoader { readonly contract: MetadataLoaderContract = { @@ -61,6 +61,24 @@ export class MemoryLoader implements MetadataLoader { return Array.from(typeStore.values()) as T[]; } + /** + * [#14205] The keyed half of {@link loadMany}. The storage map is already + * `Type -> Name -> Data`, so the key this loader holds an item under is the + * map key — `loadMany()` was simply discarding it, which dropped every + * nameless body out of `MetadataManager.list()` and out of the endpoint index. + * + * The body is handed back by reference, unchanged: the key travels beside it, + * never folded into it. + */ + async loadManyKeyed( + type: string, + _options?: MetadataLoadOptions + ): Promise[]> { + const typeStore = this.storage.get(type); + if (!typeStore) return []; + return Array.from(typeStore, ([name, data]) => ({ name, data: data as T })); + } + async exists(type: string, name: string): Promise { return this.storage.get(type)?.has(name) ?? false; } diff --git a/packages/metadata/src/metadata-manager-degraded-list-cache.test.ts b/packages/metadata/src/metadata-manager-degraded-list-cache.test.ts index e5a75afd78..41f7a20592 100644 --- a/packages/metadata/src/metadata-manager-degraded-list-cache.test.ts +++ b/packages/metadata/src/metadata-manager-degraded-list-cache.test.ts @@ -249,30 +249,52 @@ describe('#5184 — the issue repro: a healed store is not shadowed by the degra }); }); + +/** + * [#14205] Counts loader WALKS, not calls to one method name. + * + * `list()` reads a loader through `loadManyKeyed()` when the loader has one — a + * loader-held item's identity is the key its store holds it under, not + * `body.name` — and falls back to `loadMany()` when it does not. The invariant + * these cases pin is "the manager walked this loader once", which is the SUM of + * the two. Spying only `loadMany` counted 0 walks after the read moved, which + * reads exactly like a cache hit: green for the wrong reason in one direction, + * red for the wrong reason in the other. + */ +function loaderWalks(loader: MemoryLoader): { readonly count: number } { + const many = vi.spyOn(loader, 'loadMany'); + const keyed = vi.spyOn(loader, 'loadManyKeyed'); + return { + get count() { + return many.mock.calls.length + keyed.mock.calls.length; + }, + }; +} + describe('#5184 — the healthy TTL is untouched', () => { it('a complete read is still served from cache for the full 30s', async () => { const memory = new MemoryLoader(); await memory.save('permission', 'stored', { name: 'stored' }); const manager = new MetadataManager({ formats: ['json'], loaders: [memory] }); - const loadMany = vi.spyOn(memory, 'loadMany'); + const walks = loaderWalks(memory); expect(names(await manager.list('permission'))).toEqual(['stored']); - expect(loadMany).toHaveBeenCalledTimes(1); + expect(walks.count).toBe(1); // Past the degraded TTL, nowhere near the healthy one. vi.advanceTimersByTime(ttls().degraded * 3); await manager.list('permission'); - expect(loadMany).toHaveBeenCalledTimes(1); + expect(walks.count).toBe(1); // Just short of 30s — still cached. vi.advanceTimersByTime(ttls().healthy - ttls().degraded * 3 - 1); await manager.list('permission'); - expect(loadMany).toHaveBeenCalledTimes(1); + expect(walks.count).toBe(1); // Past 30s — re-read, exactly as before. vi.advanceTimersByTime(2); await manager.list('permission'); - expect(loadMany).toHaveBeenCalledTimes(2); + expect(walks.count).toBe(2); }); }); @@ -286,7 +308,7 @@ describe('#5184 — 现象二: the comment now describes the code', () => { it('an empty complete read is cached too — there is no non-empty condition', async () => { const memory = new MemoryLoader(); const manager = new MetadataManager({ formats: ['json'], loaders: [memory] }); - const loadMany = vi.spyOn(memory, 'loadMany'); + const walks = loaderWalks(memory); expect(await manager.list('permission')).toEqual([]); const entry = peekEntry(manager, 'permission'); @@ -296,6 +318,6 @@ describe('#5184 — 现象二: the comment now describes the code', () => { // And it is served from cache, not re-read. await manager.list('permission'); - expect(loadMany).toHaveBeenCalledTimes(1); + expect(walks.count).toBe(1); }); }); diff --git a/packages/metadata/src/metadata-manager-list-diagnosed.test.ts b/packages/metadata/src/metadata-manager-list-diagnosed.test.ts index 7cc598f894..a2e3531588 100644 --- a/packages/metadata/src/metadata-manager-list-diagnosed.test.ts +++ b/packages/metadata/src/metadata-manager-list-diagnosed.test.ts @@ -270,19 +270,41 @@ describe('#6504 — list() and listDiagnosed() are one read seen at two widths', expect((await working.listDiagnosed('permission')).items).toBe(workingItems); }); + +/** + * [#14205] Counts loader WALKS, not calls to one method name. + * + * `list()` reads a loader through `loadManyKeyed()` when the loader has one — a + * loader-held item's identity is the key its store holds it under, not + * `body.name` — and falls back to `loadMany()` when it does not. The invariant + * these cases pin is "the manager walked this loader once", which is the SUM of + * the two. Spying only `loadMany` counted 0 walks after the read moved, which + * reads exactly like a cache hit: green for the wrong reason in one direction, + * red for the wrong reason in the other. + */ +function loaderWalks(loader: MemoryLoader): { readonly count: number } { + const many = vi.spyOn(loader, 'loadMany'); + const keyed = vi.spyOn(loader, 'loadManyKeyed'); + return { + get count() { + return many.mock.calls.length + keyed.mock.calls.length; + }, + }; +} + it('asking for the verdict costs no extra loader walk — one cache entry serves both', async () => { const memory = new MemoryLoader(); await memory.save('permission', 'stored', { name: 'stored' }); const manager = new MetadataManager({ formats: ['json'], loaders: [memory] }); - const loadMany = vi.spyOn(memory, 'loadMany'); + const walks = loaderWalks(memory); await manager.list('permission'); - expect(loadMany).toHaveBeenCalledTimes(1); + expect(walks.count).toBe(1); // Served from the entry the `list()` above filled — `listDiagnosed` is // the same read, not a second one. const diagnosed = await manager.listDiagnosed('permission'); - expect(loadMany).toHaveBeenCalledTimes(1); + expect(walks.count).toBe(1); expect(names(diagnosed.items)).toEqual(['stored']); expect(diagnosed.degraded).toBe(false); }); diff --git a/packages/metadata/src/metadata-manager-loader-item-row-key-identity.test.ts b/packages/metadata/src/metadata-manager-loader-item-row-key-identity.test.ts new file mode 100644 index 0000000000..479ec54a0e --- /dev/null +++ b/packages/metadata/src/metadata-manager-loader-item-row-key-identity.test.ts @@ -0,0 +1,280 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #14205 — a loader-held item's identity is the ROW KEY the loader persisted it + * under, not `body.name`. + * + * --------------------------------------------------------------------------- + * The defect + * --------------------------------------------------------------------------- + * `MetadataManager.readListUncached()` (and its no-catch counterpart + * `listForIndex()`) merged each loader's answer into the result map keyed by + * `body.name`, admitting an item ONLY when its stored body carried a string + * `name`: + * + * ```ts + * if (itemAny && typeof itemAny.name === 'string' && !items.has(itemAny.name)) { + * items.set(itemAny.name, item); + * } + * ``` + * + * An aggregated `defineView` container has no own `name` BY DESIGN — its + * identity is the target object, carried in the row's `name` COLUMN, and + * `DatabaseLoader.rowToData()` returns the stored body without folding the + * column into it. So a container written by + * `register('view', OBJECT, container)` survives in-process (the registry keys + * it by the register argument) and is written to `sys_metadata`, but on the + * next process start — cold registry, only the loader answering — `list('view')` + * drops it outright, and `listDiagnosed()` calls the short answer complete. + * + * Not scoped to views: ANY loader-held body with no top-level `name` was + * invisible, including an `api` row, whose absence from the endpoint index + * reads as "nothing declares this route". + * + * --------------------------------------------------------------------------- + * The ruling this pins (triage, 2026-09-02) + * --------------------------------------------------------------------------- + * "a loader-held item's identity is the row key the loader persisted it under + * — `register(type, name, data)` stored it by `name`, so `readListUncached()` + * keys loader items by that name, never by `body.name` alone, and does NOT + * synthesise a `name` into bodies that deliberately have none". + * + * Both halves are pinned here: the nameless body is LISTED (`admits …`), and + * what comes back is the stored body verbatim, still carrying no own `name` + * (`never synthesises a name …`). The second is why the register contract's + * `data.name` check (`assertMetadataRegisterContract`) is untouched by this + * repair — nothing writes a name into a body that has none. + * + * --------------------------------------------------------------------------- + * Controls, and what they are controls AGAINST + * --------------------------------------------------------------------------- + * `CONTROL:` cases are green in BOTH directions and were measured so against + * the pre-repair tree; a red one there would report a regression rather than + * this fix. They pin the "nothing consumers see today changes shape" half of + * the ruling: a body that DOES carry a name is still listed and still keyed by + * that name, a registry entry still wins over a loader row of the same key, and + * `loadMany()` — whose `body.name` test is a DEDUPE guard, not an admission + * gate, and which therefore never had this defect — still answers identically. + */ + +import { describe, it, expect, vi } from 'vitest'; +import type { IDataDriver } from '@objectstack/spec/contracts'; +import { MetadataManager } from './metadata-manager.js'; +import { DatabaseLoader } from './loaders/database-loader.js'; +import { MemoryLoader } from './loaders/memory-loader.js'; + +const logger = vi.hoisted(() => ({ + info: vi.fn(), + warn: vi.fn(), + error: vi.fn(), + debug: vi.fn(), +})); + +vi.mock('@objectstack/core', async (orig) => ({ + ...((await orig()) as object), + createLogger: () => logger, +})); + +/** + * The card's container shape, verbatim from + * `metadata-manager-views-by-object-container.test.ts` — the aggregated + * container whose identity is the target object and which carries NO top-level + * `name`. + */ +const runtimeContainer = { + object: 'crm_lead', + list: { + label: 'All Leads', + type: 'grid', + data: { provider: 'object' }, + columns: [{ field: 'name' }, { field: 'company' }], + }, + listViews: { + pipeline: { + label: 'Lead Pipeline', + type: 'kanban', + data: { provider: 'object' }, + columns: ['name', 'company'], + kanban: { groupByField: 'status' }, + }, + }, + formViews: { + edit: { type: 'simple', sections: [{ label: 'Info', fields: [{ field: 'name' }] }] }, + }, +}; + +/** What `runtimeContainer` expands to, sorted — the oracle consumer's answer. */ +const EXPANDED = ['crm_lead.default', 'crm_lead.edit', 'crm_lead.pipeline']; + +const names = (items: unknown[]): string[] => + (items as { name: string }[]).map((i) => i.name).sort(); + +/** + * A `sys_metadata` store serving the rows it is handed. Minimal on purpose: + * `DatabaseLoader.loadMany()` only reaches `syncSchema` and `find`. + */ +function storeServing(rows: Record[]): IDataDriver { + return { + name: 'mock', + version: '1.0.0', + supports: {}, + connect: async (): Promise => {}, + disconnect: async (): Promise => {}, + syncSchema: async (): Promise => {}, + find: async (): Promise[]> => rows, + } as unknown as IDataDriver; +} + +/** + * A cold manager — empty registry, one `sys_metadata`-backed loader — serving + * one `view` row keyed `crm_lead`. `body` is what the row's `metadata` column + * holds; the ONLY difference between the two cases below is whether it repeats + * the row key as a top-level `name`. + */ +function coldManagerServingViewRow(body: Record): MetadataManager { + const manager = new MetadataManager({ formats: ['json'], loaders: [] }); + manager.registerLoader( + new DatabaseLoader({ + driver: storeServing([ + { id: 'r1', name: 'crm_lead', type: 'view', metadata: JSON.stringify(body) }, + ]), + cache: { enabled: false }, + }), + ); + return manager; +} + +/** The stored body WITH the name repeated — the shape that was already listed. */ +const namedBody = { name: 'crm_lead', ...runtimeContainer }; +/** The stored body WITHOUT it — the container's real on-disk shape. */ +const namelessBody = { ...runtimeContainer }; + +describe('#14205 loader-held items are keyed by the row key, not by body.name', () => { + it('CONTROL: a stored body that DOES carry a top-level name is listed', async () => { + const items = await coldManagerServingViewRow(namedBody).list('view'); + + expect(items).toHaveLength(1); + expect(items[0]).toMatchObject({ name: 'crm_lead', object: 'crm_lead' }); + }); + + it('admits a stored body with NO top-level name, keyed by the row key', async () => { + // Pre-repair this was `[]`: `readListUncached()` required `body.name`. + const items = await coldManagerServingViewRow(namelessBody).list('view'); + + expect(items).toHaveLength(1); + expect(items[0]).toMatchObject({ object: 'crm_lead' }); + }); + + it('never synthesises a name into a body that deliberately has none', async () => { + const items = await coldManagerServingViewRow(namelessBody).list('view'); + + // The ruling's second half. The row key is the item's identity; it is NOT + // written into the body, so `assertMetadataRegisterContract`'s refusal of a + // disagreeing `data.name` keeps meaning what it means. + expect(items).toHaveLength(1); + expect(Object.prototype.hasOwnProperty.call(items[0] as object, 'name')).toBe(false); + expect(items[0]).toEqual(namelessBody); + }); + + it("reaches the oracle consumer: getViewsByObject() expands the nameless container", async () => { + // The card's own mutation test: #13913's expansion is correct and complete + // for whatever `list('view')` holds — it just never saw this row. + const views = await coldManagerServingViewRow(namelessBody).getViewsByObject('crm_lead'); + + expect(names(views)).toEqual(EXPANDED); + }); + + it('listDiagnosed() counts the nameless body and stays complete-and-not-degraded', async () => { + const result = await coldManagerServingViewRow(namelessBody).listDiagnosed('view'); + + // Nothing threw before the repair and nothing throws after: the loader + // answered successfully both times. What changes is that the short answer + // is no longer reported as a full one. + expect(result.degraded).toBe(false); + expect(result.errors).toEqual([]); + expect(result.items).toHaveLength(1); + }); + + it('CONTROL: a registry entry still wins over a loader row of the same key', async () => { + const manager = coldManagerServingViewRow(namelessBody); + const registered = { object: 'crm_lead', registryCopy: true }; + manager.registerInMemory('view', 'crm_lead', registered); + + const items = await manager.list('view'); + + // One entry, and it is the registry's own object — not a second copy + // contributed by the loader under the same identity. + expect(items).toHaveLength(1); + expect(items[0]).toBe(registered); + }); + + it('CONTROL: loadMany() answered with the nameless body before the repair too', async () => { + // `loadMany()`'s `typeof itemAny.name === 'string'` test is a DEDUPE guard + // — a nameless item falls past it and is pushed unconditionally — so that + // site is a different door and is deliberately untouched by this repair. + const items = await coldManagerServingViewRow(namelessBody).loadMany('view'); + + expect(items).toEqual([namelessBody]); + }); + + it('the endpoint index (listForIndex) SEES a nameless api row instead of dropping it silently', async () => { + // `matchEndpoint()` reads through `listForIndex()`, the no-catch sibling of + // `readListUncached()` that carried the identical admission gate. This case + // pins that second site — and MEASURES, rather than assumes, how far the + // repair carries there. + // + // It does not make the route answer, and that is not this repair's job: + // `ApiEndpointSchema` declares `name` REQUIRED, so a nameless `api` body is + // not a valid endpoint declaration at all and `buildEndpointIndex` skips it + // at its own, separate, declared door. What changes is that the item now + // REACHES that door. Before, `listForIndex()` dropped it upstream and the + // author got silence — the exact failure posture `EndpointMatcher`'s LOUD + // skip exists to prevent. After, the exclusion is stated at `error` level + // with its consequence. + const loader = new MemoryLoader(); + await loader.save('api', 'list_tasks', { + path: '/api/v1/apps/showcase/tasks', + method: 'GET', + type: 'object_operation', + target: 'showcase_task', + objectParams: { object: 'showcase_task', operation: 'find' }, + }); + + const manager = new MetadataManager({ formats: ['json'], loaders: [] }); + manager.registerLoader(loader); + logger.error.mockClear(); + + const match = await manager.matchEndpoint({ + path: '/api/v1/apps/showcase/tasks', + method: 'GET', + }); + + expect(match).toBeUndefined(); + expect(logger.error).toHaveBeenCalledTimes(1); + expect(String(logger.error.mock.calls[0][0])).toContain( + 'EXCLUDED from endpoint matching', + ); + }); + + it('CONTROL: the endpoint index still admits a NAMED api row', async () => { + const loader = new MemoryLoader(); + await loader.save('api', 'list_tasks', { + name: 'list_tasks', + path: '/api/v1/apps/showcase/tasks', + method: 'GET', + type: 'object_operation', + target: 'showcase_task', + objectParams: { object: 'showcase_task', operation: 'find' }, + }); + + const manager = new MetadataManager({ formats: ['json'], loaders: [] }); + manager.registerLoader(loader); + + const match = await manager.matchEndpoint({ + path: '/api/v1/apps/showcase/tasks', + method: 'GET', + }); + + expect(match).toBeDefined(); + }); +}); diff --git a/packages/metadata/src/metadata-manager.ts b/packages/metadata/src/metadata-manager.ts index 14ffa5d9a9..042f99b9c7 100644 --- a/packages/metadata/src/metadata-manager.ts +++ b/packages/metadata/src/metadata-manager.ts @@ -1070,6 +1070,73 @@ export class MetadataManager implements IMetadataService { * result may be memoized depends on what happened to the read's registration * while it ran, which only `list()` can see. */ + /** + * Merge one loader's answer for `type` into `items`, under the identity that + * loader holds each item by. + * + * ## [#14205] The identity of a loader-held item is its ROW KEY + * + * Both plural readers used to key a loader's items by `body.name`, and admit + * an item only when the body carried a string one: + * + * ```ts + * if (itemAny && typeof itemAny.name === 'string' && !items.has(itemAny.name)) + * ``` + * + * A body is not required to name itself. `register(type, name, data)` takes + * the key as its ARGUMENT, and `assertMetadataRegisterContract` says in as + * many words that "A document with NO `name` of its own is fine — the argument + * is the key". An aggregated `defineView` container is exactly that: no own + * `name` by design, identity carried in the row's `name` column. + * + * So the old gate dropped every such item the moment the registry went cold + * and only the loader could answer — a persisted view container vanished from + * `list('view')` after a restart, and `listDiagnosed()` called the short + * answer complete because no loader had thrown. Same gate, same effect, in + * `listForIndex()`: a nameless `api` row fell out of the endpoint index, where + * a miss reads as "nothing declares this route". + * + * The repair is to ask the loader for the key instead of guessing it from the + * body ({@link MetadataLoader.loadManyKeyed}), and to keep the key BESIDE the + * body: nothing is written into a body that deliberately has none, so the + * register contract's refusal of a disagreeing `data.name` still means what it + * says. + * + * Nothing consumers see today changes shape. For any item that went through + * `register()`, a `data.name` that exists is required to EQUAL the key, so the + * keyed merge produces the identical map entry; what is new is only the + * entries the old gate refused. The `loadMany()` fallback below is the + * pre-#14205 behaviour verbatim, for loaders that cannot produce keys + * (`RemoteLoader`'s wire format carries bodies only). + * + * Read failures are NOT caught here: `readListUncached` warns-and-continues, + * `listForIndex` deliberately throws, and that difference is each caller's to + * keep. + */ + private async admitLoaderItems( + loader: MetadataLoader, + type: string, + items: Map + ): Promise { + if (typeof loader.loadManyKeyed === 'function') { + const keyed = await loader.loadManyKeyed(type); + for (const entry of keyed) { + if (!entry || typeof entry.name !== 'string' || entry.name === '') continue; + if (items.has(entry.name)) continue; + items.set(entry.name, entry.data); + } + return; + } + + const loaderItems = await loader.loadMany(type); + for (const item of loaderItems) { + const itemAny = item as { name?: unknown } | null; + if (itemAny && typeof itemAny.name === 'string' && !items.has(itemAny.name)) { + items.set(itemAny.name, item); + } + } + } + private async readListUncached(type: string): Promise { const items = new Map(); @@ -1095,13 +1162,7 @@ export class MetadataManager implements IMetadataService { const errors: string[] = []; for (const loader of this.loaders.values()) { try { - const loaderItems = await loader.loadMany(type); - for (const item of loaderItems) { - const itemAny = item as any; - if (itemAny && typeof itemAny.name === 'string' && !items.has(itemAny.name)) { - items.set(itemAny.name, item); - } - } + await this.admitLoaderItems(loader, type, items); this.reportLoaderReadRecovered(loader.contract.name); } catch (e) { degraded = true; @@ -1277,13 +1338,7 @@ export class MetadataManager implements IMetadataService { for (const loader of this.loaders.values()) { // No try/catch, on purpose — see the doc comment above. - const loaderItems = await loader.loadMany(type); - for (const item of loaderItems) { - const itemAny = item as { name?: unknown }; - if (itemAny && typeof itemAny.name === 'string' && !items.has(itemAny.name)) { - items.set(itemAny.name, item); - } - } + await this.admitLoaderItems(loader, type, items); } return Array.from(items.values()); From 3cb0b0e257bf74b134756ffff96f097cfeed9c9e Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 2 Sep 2026 02:51:02 +0000 Subject: [PATCH 2/2] chore(changeset): raise @objectstack/metadata to minor for the additive published surface The diff widens the published surface additively: `@objectstack/metadata`'s entry gains a named type (`MetadataKeyedItem`) and `MetadataLoader` gains an optional member (`loadManyKeyed?`). This repo's precedent for additive public-surface widening is `minor`, not `patch` (R12: #14262's `job-handler-data-reach.md` and #14247, both `"@objectstack/runtime": minor`). Front matter only; the changeset body is byte-identical. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_0112hMx9hjJ9BgB28X97DS68 --- .changeset/loader-item-row-key-identity.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/loader-item-row-key-identity.md b/.changeset/loader-item-row-key-identity.md index 5e6e3fb144..0849fd7ea7 100644 --- a/.changeset/loader-item-row-key-identity.md +++ b/.changeset/loader-item-row-key-identity.md @@ -1,5 +1,5 @@ --- -"@objectstack/metadata": patch +"@objectstack/metadata": minor --- fix(metadata): key loader-held items by the row key they were stored under, so a body with no top-level `name` is no longer dropped from `list()` (#14205)