Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 46 additions & 0 deletions .changeset/loader-item-row-key-identity.md
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
---
"@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)

`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.
2 changes: 1 addition & 1 deletion packages/metadata/src/index.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -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';
Expand Down
67 changes: 58 additions & 9 deletions packages/metadata/src/loaders/database-loader.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -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
Expand DownExpand Up@@ -870,25 +870,43 @@ export class DatabaseLoader implements MetadataLoader {
}
}

async loadMany<T = any>(
type: string,
_options?: MetadataLoadOptions
): Promise<T[]> {
/**
* 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<Array<{ name: string | null; data: Record<string, unknown> }>> {
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<string, unknown> }>;
}
}

try {
const rows = await this._find(this.tableName, {
where: this.baseFilter(type),
});

const result = rows
.map(row => this.rowToData(row))
.filter((data): data is Record<string, unknown> => data !== null) as T[];
const result: Array<{ name: string | null; data: Record<string, unknown> }> = [];
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;
Expand All@@ -899,6 +917,37 @@ export class DatabaseLoader implements MetadataLoader {
}
}

async loadMany<T = any>(
type: string,
_options?: MetadataLoadOptions
): Promise<T[]> {
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<T = any>(
type: string,
_options?: MetadataLoadOptions
): Promise<MetadataKeyedItem<T>[]> {
const entries = await this.readTypeRows(type);
const keyed: MetadataKeyedItem<T>[] = [];
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<boolean> {
await this.ensureSchema();

Expand Down
56 changes: 56 additions & 0 deletions packages/metadata/src/loaders/loader-interface.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -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<T = any> {
/** 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.
Expand DownExpand Up@@ -49,6 +73,38 @@ export interface MetadataLoader {
options?: MetadataLoadOptions
): Promise<T[]>;

/**
* 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?<T = any>(
type: string,
options?: MetadataLoadOptions
): Promise<MetadataKeyedItem<T>[]>;

/**
* Check if item exists
* @param type The metadata type
Expand Down
20 changes: 19 additions & 1 deletion packages/metadata/src/loaders/memory-loader.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -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 = {
Expand DownExpand Up@@ -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<T = any>(
type: string,
_options?: MetadataLoadOptions
): Promise<MetadataKeyedItem<T>[]> {
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<boolean> {
return this.storage.get(type)?.has(name) ?? false;
}
Expand Down
Original file line numberDiff line numberDiff line change
Expand Up@@ -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);
});
});

Expand All@@ -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');
Expand All@@ -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);
});
});
28 changes: 25 additions & 3 deletions packages/metadata/src/metadata-manager-list-diagnosed.test.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -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);
});
Expand Down
Loading
Loading
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 46 additions & 0 deletions .changeset/loader-item-row-key-identity.md
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
---
"@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)

`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.
2 changes: 1 addition & 1 deletion packages/metadata/src/index.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -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';
Expand Down
67 changes: 58 additions & 9 deletions packages/metadata/src/loaders/database-loader.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -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
Expand DownExpand Up@@ -870,25 +870,43 @@ export class DatabaseLoader implements MetadataLoader {
}
}

async loadMany<T = any>(
type: string,
_options?: MetadataLoadOptions
): Promise<T[]> {
/**
* 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<Array<{ name: string | null; data: Record<string, unknown> }>> {
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<string, unknown> }>;
}
}

try {
const rows = await this._find(this.tableName, {
where: this.baseFilter(type),
});

const result = rows
.map(row => this.rowToData(row))
.filter((data): data is Record<string, unknown> => data !== null) as T[];
const result: Array<{ name: string | null; data: Record<string, unknown> }> = [];
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;
Expand All@@ -899,6 +917,37 @@ export class DatabaseLoader implements MetadataLoader {
}
}

async loadMany<T = any>(
type: string,
_options?: MetadataLoadOptions
): Promise<T[]> {
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<T = any>(
type: string,
_options?: MetadataLoadOptions
): Promise<MetadataKeyedItem<T>[]> {
const entries = await this.readTypeRows(type);
const keyed: MetadataKeyedItem<T>[] = [];
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<boolean> {
await this.ensureSchema();

Expand Down
56 changes: 56 additions & 0 deletions packages/metadata/src/loaders/loader-interface.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -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<T = any> {
/** 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.
Expand DownExpand Up@@ -49,6 +73,38 @@ export interface MetadataLoader {
options?: MetadataLoadOptions
): Promise<T[]>;

/**
* 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?<T = any>(
type: string,
options?: MetadataLoadOptions
): Promise<MetadataKeyedItem<T>[]>;

/**
* Check if item exists
* @param type The metadata type
Expand Down
20 changes: 19 additions & 1 deletion packages/metadata/src/loaders/memory-loader.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -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 = {
Expand DownExpand Up@@ -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<T = any>(
type: string,
_options?: MetadataLoadOptions
): Promise<MetadataKeyedItem<T>[]> {
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<boolean> {
return this.storage.get(type)?.has(name) ?? false;
}
Expand Down
Original file line numberDiff line numberDiff line change
Expand Up@@ -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);
});
});

Expand All@@ -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');
Expand All@@ -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);
});
});
28 changes: 25 additions & 3 deletions packages/metadata/src/metadata-manager-list-diagnosed.test.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -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);
});
Expand Down
Loading
Loading
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 46 additions & 0 deletions .changeset/loader-item-row-key-identity.md
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
---
"@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)

`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.
2 changes: 1 addition & 1 deletion packages/metadata/src/index.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -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';
Expand Down
67 changes: 58 additions & 9 deletions packages/metadata/src/loaders/database-loader.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -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
Expand DownExpand Up@@ -870,25 +870,43 @@ export class DatabaseLoader implements MetadataLoader {
}
}

async loadMany<T = any>(
type: string,
_options?: MetadataLoadOptions
): Promise<T[]> {
/**
* 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<Array<{ name: string | null; data: Record<string, unknown> }>> {
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<string, unknown> }>;
}
}

try {
const rows = await this._find(this.tableName, {
where: this.baseFilter(type),
});

const result = rows
.map(row => this.rowToData(row))
.filter((data): data is Record<string, unknown> => data !== null) as T[];
const result: Array<{ name: string | null; data: Record<string, unknown> }> = [];
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;
Expand All@@ -899,6 +917,37 @@ export class DatabaseLoader implements MetadataLoader {
}
}

async loadMany<T = any>(
type: string,
_options?: MetadataLoadOptions
): Promise<T[]> {
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<T = any>(
type: string,
_options?: MetadataLoadOptions
): Promise<MetadataKeyedItem<T>[]> {
const entries = await this.readTypeRows(type);
const keyed: MetadataKeyedItem<T>[] = [];
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<boolean> {
await this.ensureSchema();

Expand Down
56 changes: 56 additions & 0 deletions packages/metadata/src/loaders/loader-interface.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -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<T = any> {
/** 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.
Expand DownExpand Up@@ -49,6 +73,38 @@ export interface MetadataLoader {
options?: MetadataLoadOptions
): Promise<T[]>;

/**
* 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?<T = any>(
type: string,
options?: MetadataLoadOptions
): Promise<MetadataKeyedItem<T>[]>;

/**
* Check if item exists
* @param type The metadata type
Expand Down
20 changes: 19 additions & 1 deletion packages/metadata/src/loaders/memory-loader.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -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 = {
Expand DownExpand Up@@ -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<T = any>(
type: string,
_options?: MetadataLoadOptions
): Promise<MetadataKeyedItem<T>[]> {
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<boolean> {
return this.storage.get(type)?.has(name) ?? false;
}
Expand Down
Original file line numberDiff line numberDiff line change
Expand Up@@ -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);
});
});

Expand All@@ -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');
Expand All@@ -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);
});
});
28 changes: 25 additions & 3 deletions packages/metadata/src/metadata-manager-list-diagnosed.test.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -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);
});
Expand Down
Loading
Loading
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 46 additions & 0 deletions .changeset/loader-item-row-key-identity.md
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
---
"@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)

`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.
2 changes: 1 addition & 1 deletion packages/metadata/src/index.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -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';
Expand Down
67 changes: 58 additions & 9 deletions packages/metadata/src/loaders/database-loader.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -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
Expand DownExpand Up@@ -870,25 +870,43 @@ export class DatabaseLoader implements MetadataLoader {
}
}

async loadMany<T = any>(
type: string,
_options?: MetadataLoadOptions
): Promise<T[]> {
/**
* 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<Array<{ name: string | null; data: Record<string, unknown> }>> {
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<string, unknown> }>;
}
}

try {
const rows = await this._find(this.tableName, {
where: this.baseFilter(type),
});

const result = rows
.map(row => this.rowToData(row))
.filter((data): data is Record<string, unknown> => data !== null) as T[];
const result: Array<{ name: string | null; data: Record<string, unknown> }> = [];
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;
Expand All@@ -899,6 +917,37 @@ export class DatabaseLoader implements MetadataLoader {
}
}

async loadMany<T = any>(
type: string,
_options?: MetadataLoadOptions
): Promise<T[]> {
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<T = any>(
type: string,
_options?: MetadataLoadOptions
): Promise<MetadataKeyedItem<T>[]> {
const entries = await this.readTypeRows(type);
const keyed: MetadataKeyedItem<T>[] = [];
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<boolean> {
await this.ensureSchema();

Expand Down
56 changes: 56 additions & 0 deletions packages/metadata/src/loaders/loader-interface.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -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<T = any> {
/** 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.
Expand DownExpand Up@@ -49,6 +73,38 @@ export interface MetadataLoader {
options?: MetadataLoadOptions
): Promise<T[]>;

/**
* 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?<T = any>(
type: string,
options?: MetadataLoadOptions
): Promise<MetadataKeyedItem<T>[]>;

/**
* Check if item exists
* @param type The metadata type
Expand Down
20 changes: 19 additions & 1 deletion packages/metadata/src/loaders/memory-loader.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -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 = {
Expand DownExpand Up@@ -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<T = any>(
type: string,
_options?: MetadataLoadOptions
): Promise<MetadataKeyedItem<T>[]> {
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<boolean> {
return this.storage.get(type)?.has(name) ?? false;
}
Expand Down
Original file line numberDiff line numberDiff line change
Expand Up@@ -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);
});
});

Expand All@@ -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');
Expand All@@ -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);
});
});
28 changes: 25 additions & 3 deletions packages/metadata/src/metadata-manager-list-diagnosed.test.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -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);
});
Expand Down
Loading
Loading
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 46 additions & 0 deletions .changeset/loader-item-row-key-identity.md
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
---
"@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)

`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.
2 changes: 1 addition & 1 deletion packages/metadata/src/index.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -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';
Expand Down
67 changes: 58 additions & 9 deletions packages/metadata/src/loaders/database-loader.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -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
Expand DownExpand Up@@ -870,25 +870,43 @@ export class DatabaseLoader implements MetadataLoader {
}
}

async loadMany<T = any>(
type: string,
_options?: MetadataLoadOptions
): Promise<T[]> {
/**
* 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<Array<{ name: string | null; data: Record<string, unknown> }>> {
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<string, unknown> }>;
}
}

try {
const rows = await this._find(this.tableName, {
where: this.baseFilter(type),
});

const result = rows
.map(row => this.rowToData(row))
.filter((data): data is Record<string, unknown> => data !== null) as T[];
const result: Array<{ name: string | null; data: Record<string, unknown> }> = [];
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;
Expand All@@ -899,6 +917,37 @@ export class DatabaseLoader implements MetadataLoader {
}
}

async loadMany<T = any>(
type: string,
_options?: MetadataLoadOptions
): Promise<T[]> {
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<T = any>(
type: string,
_options?: MetadataLoadOptions
): Promise<MetadataKeyedItem<T>[]> {
const entries = await this.readTypeRows(type);
const keyed: MetadataKeyedItem<T>[] = [];
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<boolean> {
await this.ensureSchema();

Expand Down
56 changes: 56 additions & 0 deletions packages/metadata/src/loaders/loader-interface.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -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<T = any> {
/** 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.
Expand DownExpand Up@@ -49,6 +73,38 @@ export interface MetadataLoader {
options?: MetadataLoadOptions
): Promise<T[]>;

/**
* 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?<T = any>(
type: string,
options?: MetadataLoadOptions
): Promise<MetadataKeyedItem<T>[]>;

/**
* Check if item exists
* @param type The metadata type
Expand Down
20 changes: 19 additions & 1 deletion packages/metadata/src/loaders/memory-loader.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -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 = {
Expand DownExpand Up@@ -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<T = any>(
type: string,
_options?: MetadataLoadOptions
): Promise<MetadataKeyedItem<T>[]> {
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<boolean> {
return this.storage.get(type)?.has(name) ?? false;
}
Expand Down
Original file line numberDiff line numberDiff line change
Expand Up@@ -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);
});
});

Expand All@@ -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');
Expand All@@ -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);
});
});
28 changes: 25 additions & 3 deletions packages/metadata/src/metadata-manager-list-diagnosed.test.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -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);
});
Expand Down
Loading
Loading
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 46 additions & 0 deletions .changeset/loader-item-row-key-identity.md
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
---
"@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)

`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.
2 changes: 1 addition & 1 deletion packages/metadata/src/index.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -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';
Expand Down
67 changes: 58 additions & 9 deletions packages/metadata/src/loaders/database-loader.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -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
Expand DownExpand Up@@ -870,25 +870,43 @@ export class DatabaseLoader implements MetadataLoader {
}
}

async loadMany<T = any>(
type: string,
_options?: MetadataLoadOptions
): Promise<T[]> {
/**
* 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<Array<{ name: string | null; data: Record<string, unknown> }>> {
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<string, unknown> }>;
}
}

try {
const rows = await this._find(this.tableName, {
where: this.baseFilter(type),
});

const result = rows
.map(row => this.rowToData(row))
.filter((data): data is Record<string, unknown> => data !== null) as T[];
const result: Array<{ name: string | null; data: Record<string, unknown> }> = [];
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;
Expand All@@ -899,6 +917,37 @@ export class DatabaseLoader implements MetadataLoader {
}
}

async loadMany<T = any>(
type: string,
_options?: MetadataLoadOptions
): Promise<T[]> {
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<T = any>(
type: string,
_options?: MetadataLoadOptions
): Promise<MetadataKeyedItem<T>[]> {
const entries = await this.readTypeRows(type);
const keyed: MetadataKeyedItem<T>[] = [];
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<boolean> {
await this.ensureSchema();

Expand Down
56 changes: 56 additions & 0 deletions packages/metadata/src/loaders/loader-interface.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -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<T = any> {
/** 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.
Expand DownExpand Up@@ -49,6 +73,38 @@ export interface MetadataLoader {
options?: MetadataLoadOptions
): Promise<T[]>;

/**
* 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?<T = any>(
type: string,
options?: MetadataLoadOptions
): Promise<MetadataKeyedItem<T>[]>;

/**
* Check if item exists
* @param type The metadata type
Expand Down
20 changes: 19 additions & 1 deletion packages/metadata/src/loaders/memory-loader.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -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 = {
Expand DownExpand Up@@ -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<T = any>(
type: string,
_options?: MetadataLoadOptions
): Promise<MetadataKeyedItem<T>[]> {
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<boolean> {
return this.storage.get(type)?.has(name) ?? false;
}
Expand Down
Original file line numberDiff line numberDiff line change
Expand Up@@ -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);
});
});

Expand All@@ -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');
Expand All@@ -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);
});
});
28 changes: 25 additions & 3 deletions packages/metadata/src/metadata-manager-list-diagnosed.test.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -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);
});
Expand Down
Loading
Loading
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 46 additions & 0 deletions .changeset/loader-item-row-key-identity.md
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
---
"@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)

`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.
2 changes: 1 addition & 1 deletion packages/metadata/src/index.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -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';
Expand Down
67 changes: 58 additions & 9 deletions packages/metadata/src/loaders/database-loader.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -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
Expand DownExpand Up@@ -870,25 +870,43 @@ export class DatabaseLoader implements MetadataLoader {
}
}

async loadMany<T = any>(
type: string,
_options?: MetadataLoadOptions
): Promise<T[]> {
/**
* 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<Array<{ name: string | null; data: Record<string, unknown> }>> {
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<string, unknown> }>;
}
}

try {
const rows = await this._find(this.tableName, {
where: this.baseFilter(type),
});

const result = rows
.map(row => this.rowToData(row))
.filter((data): data is Record<string, unknown> => data !== null) as T[];
const result: Array<{ name: string | null; data: Record<string, unknown> }> = [];
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;
Expand All@@ -899,6 +917,37 @@ export class DatabaseLoader implements MetadataLoader {
}
}

async loadMany<T = any>(
type: string,
_options?: MetadataLoadOptions
): Promise<T[]> {
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<T = any>(
type: string,
_options?: MetadataLoadOptions
): Promise<MetadataKeyedItem<T>[]> {
const entries = await this.readTypeRows(type);
const keyed: MetadataKeyedItem<T>[] = [];
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<boolean> {
await this.ensureSchema();

Expand Down
56 changes: 56 additions & 0 deletions packages/metadata/src/loaders/loader-interface.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -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<T = any> {
/** 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.
Expand DownExpand Up@@ -49,6 +73,38 @@ export interface MetadataLoader {
options?: MetadataLoadOptions
): Promise<T[]>;

/**
* 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?<T = any>(
type: string,
options?: MetadataLoadOptions
): Promise<MetadataKeyedItem<T>[]>;

/**
* Check if item exists
* @param type The metadata type
Expand Down
20 changes: 19 additions & 1 deletion packages/metadata/src/loaders/memory-loader.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -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 = {
Expand DownExpand Up@@ -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<T = any>(
type: string,
_options?: MetadataLoadOptions
): Promise<MetadataKeyedItem<T>[]> {
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<boolean> {
return this.storage.get(type)?.has(name) ?? false;
}
Expand Down
Original file line numberDiff line numberDiff line change
Expand Up@@ -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);
});
});

Expand All@@ -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');
Expand All@@ -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);
});
});
28 changes: 25 additions & 3 deletions packages/metadata/src/metadata-manager-list-diagnosed.test.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -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);
});
Expand Down
Loading
Loading
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 46 additions & 0 deletions .changeset/loader-item-row-key-identity.md
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
---
"@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)

`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.
2 changes: 1 addition & 1 deletion packages/metadata/src/index.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -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';
Expand Down
67 changes: 58 additions & 9 deletions packages/metadata/src/loaders/database-loader.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -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
Expand DownExpand Up@@ -870,25 +870,43 @@ export class DatabaseLoader implements MetadataLoader {
}
}

async loadMany<T = any>(
type: string,
_options?: MetadataLoadOptions
): Promise<T[]> {
/**
* 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<Array<{ name: string | null; data: Record<string, unknown> }>> {
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<string, unknown> }>;
}
}

try {
const rows = await this._find(this.tableName, {
where: this.baseFilter(type),
});

const result = rows
.map(row => this.rowToData(row))
.filter((data): data is Record<string, unknown> => data !== null) as T[];
const result: Array<{ name: string | null; data: Record<string, unknown> }> = [];
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;
Expand All@@ -899,6 +917,37 @@ export class DatabaseLoader implements MetadataLoader {
}
}

async loadMany<T = any>(
type: string,
_options?: MetadataLoadOptions
): Promise<T[]> {
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<T = any>(
type: string,
_options?: MetadataLoadOptions
): Promise<MetadataKeyedItem<T>[]> {
const entries = await this.readTypeRows(type);
const keyed: MetadataKeyedItem<T>[] = [];
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<boolean> {
await this.ensureSchema();

Expand Down
56 changes: 56 additions & 0 deletions packages/metadata/src/loaders/loader-interface.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -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<T = any> {
/** 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.
Expand DownExpand Up@@ -49,6 +73,38 @@ export interface MetadataLoader {
options?: MetadataLoadOptions
): Promise<T[]>;

/**
* 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?<T = any>(
type: string,
options?: MetadataLoadOptions
): Promise<MetadataKeyedItem<T>[]>;

/**
* Check if item exists
* @param type The metadata type
Expand Down
20 changes: 19 additions & 1 deletion packages/metadata/src/loaders/memory-loader.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -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 = {
Expand DownExpand Up@@ -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<T = any>(
type: string,
_options?: MetadataLoadOptions
): Promise<MetadataKeyedItem<T>[]> {
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<boolean> {
return this.storage.get(type)?.has(name) ?? false;
}
Expand Down
Original file line numberDiff line numberDiff line change
Expand Up@@ -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);
});
});

Expand All@@ -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');
Expand All@@ -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);
});
});
28 changes: 25 additions & 3 deletions packages/metadata/src/metadata-manager-list-diagnosed.test.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -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);
});
Expand Down
Loading
Loading