Skip to content

Commit 2e471e3

Browse files
os-muskclaude
andauthored
fix(metadata): FilesystemLoader keys loader-held items by the name it can resolve (#14341) (#14497)
`loadMany()` discarded the path it had just read, so `MetadataManager` fell back to keying by `body.name` and dropped every nameless body held by this loader — the #14205 defect, unrepaired here. `loadManyKeyed()` keys an item by this loader's own name-to-path derivation (the basename minus extension, the same one `list()` reports) ONLY where that derivation is a bijection for the file: directly under `ROOT/TYPE/`, with an extension `findFile()` tries. Every other shape keeps the pre-#14205 behaviour verbatim — keyed by `body.name` when it has one, dropped when it has none — because `list()` and `findFile()` disagree there, and a minted key that `get()`/`exists()` cannot open is what the card refused. Both methods now share one file walk, so the body `loadManyKeyed()` reports can never drift from the one `loadMany()` returns. Claude-Session: https://claude.ai/code/session_0112hMx9hjJ9BgB28X97DS68 Co-authored-by: Claude <noreply@anthropic.com>
1 parent b339a38 commit 2e471e3

3 files changed

Lines changed: 404 additions & 9 deletions

File tree

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
---
2+
'@objectstack/metadata': patch
3+
---
4+
5+
`FilesystemLoader` now implements `loadManyKeyed()`, so a metadata file whose
6+
body has no top-level `name` is no longer invisible to `MetadataManager.list()`.
7+
`loadMany()` globbed files and pushed bodies, discarding the path it had just
8+
read; the manager then fell back to keying by `body.name`, which drops every
9+
nameless body — an aggregated `defineView` container has none by design. That is
10+
the #14205 defect, unrepaired for this loader until now.
11+
12+
The key is this loader's own name-to-path derivation — the basename minus
13+
extension, the same one `list()` reports — but only where that derivation is a
14+
bijection for the file: it sits directly under `ROOT/TYPE/` and carries an
15+
extension `findFile()` tries, so `findFile(type, key)` resolves back to that same
16+
file. Every other shape (a nested path, an extension-less file) keeps the
17+
previous behaviour verbatim: keyed by `body.name` when it has one, dropped when
18+
it has none. `list()` and `findFile()` disagree outside the flat shape — `list()`
19+
reports the bare basename for a nested file and `findFile()` cannot resolve it —
20+
so keying those by the basename would mint names `get()` and `exists()` cannot
21+
open, and two directories holding one basename would collide silently. Repairing
22+
the derivation itself is tracked separately.
23+
24+
One deliberate consequence: a flat file whose `body.name` disagrees with its
25+
basename is now keyed by the basename. That is #14205's rule (identity is the
26+
key the store holds an item under, not `body.name`) applied to this loader, and
27+
it aligns `list()` with `listNames()` for that shape. `loadMany()`'s own
28+
signature and answer are unchanged; both methods now share one file walk so
29+
their bodies cannot drift.
Lines changed: 240 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,240 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* #14341 — `FilesystemLoader.loadManyKeyed()`: a file-held item is keyed by the
5+
* name this loader can actually RESOLVE for it, and by nothing else.
6+
*
7+
* ---------------------------------------------------------------------------
8+
* The defect
9+
* ---------------------------------------------------------------------------
10+
* `loadMany()` globbed files and pushed bodies, throwing away the path it had
11+
* just read. `MetadataManager.admitLoaderItems()` then fell back to keying by
12+
* `body.name`, which drops every body that has no top-level `name` — the exact
13+
* #14205 failure, unrepaired for this loader. A `defineView` container has no
14+
* own `name` BY DESIGN, so a file holding one was absent from `list('view')`
15+
* while `listDiagnosed()` called the short answer complete.
16+
*
17+
* ---------------------------------------------------------------------------
18+
* The rule this pins (PM ruling on #14341, 2026-09-02 — option D)
19+
* ---------------------------------------------------------------------------
20+
* An item is keyed by this loader's own name-to-path derivation (the basename
21+
* minus extension, the same derivation `list()` reports) ONLY where that
22+
* derivation is a BIJECTION for the file — it sits directly under `ROOT/TYPE/`
23+
* and carries an extension `findFile()` tries, so `findFile(type, key)` resolves
24+
* back to that same file. Every other shape keeps the pre-#14205 behaviour
25+
* verbatim: keyed by `body.name` when it has one, dropped when it has none.
26+
*
27+
* The ruling was taken over triage's "a nested path keeps whatever `list()`
28+
* reports for it today", knowingly, because the two derivations DISAGREE
29+
* outside the flat shape (measured on `origin/main` @ 253da34c4): `list()`
30+
* reports `account` for `ROOT/TYPE/crm/account.json`, and `findFile()` resolves
31+
* that name against `ROOT/TYPE/account.json` and finds nothing. Keying nested
32+
* files by their basename would mint names `get()` / `load()` / `exists()`
33+
* cannot open, and two directories holding one basename would collide in
34+
* silence. The card's own fence: "keying items under names nothing else uses
35+
* ... is worse than today's honest drop".
36+
*
37+
* ---------------------------------------------------------------------------
38+
* What the RECORD cases are for
39+
* ---------------------------------------------------------------------------
40+
* `RECORD:` cases pin behaviour this ruling deliberately LEAVES ALONE — the
41+
* nested nameless file is still dropped. They exist so the derivation repair
42+
* (#14486: one shared name-to-path function for `list()`, `findFile()` and
43+
* `loadManyKeyed()`) inverts them deliberately, with the change visible in a
44+
* diff, instead of silently.
45+
*
46+
* `CONTROL:` cases are green in both directions and pin "nothing consumers see
47+
* today changes shape".
48+
*/
49+
50+
import { describe, it, expect, beforeAll, afterAll } from 'vitest';
51+
import * as fs from 'node:fs/promises';
52+
import * as os from 'node:os';
53+
import * as path from 'node:path';
54+
import type { MetadataFormat } from '@objectstack/spec/system';
55+
import { MetadataManager } from '../metadata-manager.js';
56+
import { FilesystemLoader } from './filesystem-loader.js';
57+
import { JSONSerializer } from '../serializers/json-serializer.js';
58+
import type { MetadataSerializer } from '../serializers/serializer-interface.js';
59+
60+
const TYPE = 'view';
61+
62+
/** The aggregated container shape: identity is the target object, no own `name`. */
63+
const NAMELESS_CONTAINER = { object: 'account', views: [{ label: 'All' }] };
64+
65+
let root: string;
66+
67+
/**
68+
* One tree holding every shape the rule distinguishes. The two `crm/` files and
69+
* the extension-less one are the shapes where `list()` and `findFile()` disagree.
70+
*/
71+
beforeAll(async () => {
72+
root = await fs.mkdtemp(path.join(os.tmpdir(), 'fsloader-keyed-'));
73+
const typeDir = path.join(root, TYPE);
74+
await fs.mkdir(path.join(typeDir, 'crm'), { recursive: true });
75+
76+
const write = (rel: string, body: unknown) =>
77+
fs.writeFile(path.join(typeDir, rel), JSON.stringify(body), 'utf-8');
78+
79+
await write('flat_nameless.json', NAMELESS_CONTAINER);
80+
await write('flat_named.json', { name: 'flat_named', label: 'agrees with its basename' });
81+
await write('flat_disagreeing.json', { name: 'not_the_basename', label: 'disagrees' });
82+
await write('dotted.config.json', { name: 'dotted.config' });
83+
await write(path.join('crm', 'nested_named.json'), { name: 'nested_named' });
84+
await write(path.join('crm', 'nested_nameless.json'), { ...NAMELESS_CONTAINER, object: 'lead' });
85+
await write('extensionless', { name: 'extensionless_named' });
86+
});
87+
88+
afterAll(async () => {
89+
await fs.rm(root, { recursive: true, force: true });
90+
});
91+
92+
function loader(): FilesystemLoader {
93+
const serializers = new Map<MetadataFormat, MetadataSerializer>([
94+
['json', new JSONSerializer()],
95+
]);
96+
return new FilesystemLoader(root, serializers);
97+
}
98+
99+
/** A cold manager — empty registry, one filesystem loader answering. */
100+
function coldManager(): MetadataManager {
101+
const manager = new MetadataManager({ formats: ['json'], loaders: [] });
102+
manager.registerLoader(loader());
103+
return manager;
104+
}
105+
106+
async function keys(): Promise<string[]> {
107+
const keyed = await loader().loadManyKeyed(TYPE);
108+
return keyed.map(entry => entry.name).sort();
109+
}
110+
111+
describe('#14341 FilesystemLoader.loadManyKeyed() keys by the resolvable name', () => {
112+
it('keys a FLAT file by its basename, the derivation list() reports', async () => {
113+
expect(await keys()).toEqual(
114+
['dotted.config', 'extensionless_named', 'flat_disagreeing', 'flat_named', 'flat_nameless', 'nested_named'],
115+
);
116+
});
117+
118+
it('admits a flat NAMELESS body, keyed by the file name', async () => {
119+
// Pre-repair this body had no key at all and fell out of the merge.
120+
const keyed = await loader().loadManyKeyed(TYPE);
121+
const entry = keyed.find(item => item.name === 'flat_nameless');
122+
123+
expect(entry).toBeDefined();
124+
expect(entry!.data).toEqual(NAMELESS_CONTAINER);
125+
});
126+
127+
it('never synthesises a name into a body that deliberately has none', async () => {
128+
const keyed = await loader().loadManyKeyed(TYPE);
129+
const entry = keyed.find(item => item.name === 'flat_nameless')!;
130+
131+
// The key travels BESIDE the body; the body stays byte-identical to disk,
132+
// so `assertMetadataRegisterContract`'s `data.name` check keeps its meaning.
133+
expect(Object.prototype.hasOwnProperty.call(entry.data as object, 'name')).toBe(false);
134+
});
135+
136+
it('keys a flat file by its BASENAME even when body.name disagrees', async () => {
137+
// #14205's rule applied to this loader: identity is the key the store holds
138+
// the item under, not `body.name`. `flat_disagreeing.json` says
139+
// `name: 'not_the_basename'`, and the store's key is the file's.
140+
const keyed = await loader().loadManyKeyed(TYPE);
141+
142+
expect(keyed.map(entry => entry.name)).toContain('flat_disagreeing');
143+
expect(keyed.map(entry => entry.name)).not.toContain('not_the_basename');
144+
// ...and the disagreeing body is handed back unedited.
145+
expect(keyed.find(entry => entry.name === 'flat_disagreeing')!.data).toEqual({
146+
name: 'not_the_basename',
147+
label: 'disagrees',
148+
});
149+
});
150+
151+
it('strips only the final extension, so dotted.config.json keys as dotted.config', async () => {
152+
expect(await keys()).toContain('dotted.config');
153+
});
154+
155+
it('EVERY key it mints resolves back to a file through findFile()', async () => {
156+
// The bijection claim itself, and the reason the disagreeing shapes below
157+
// are NOT keyed by their basename: a minted key that `exists()` cannot open
158+
// is exactly what the card refused.
159+
const fsLoader = loader();
160+
const derived = ['dotted.config', 'flat_disagreeing', 'flat_named', 'flat_nameless'];
161+
162+
for (const key of derived) {
163+
expect(await fsLoader.exists(TYPE, key)).toBe(true);
164+
}
165+
});
166+
167+
it('keys a NESTED file by body.name — the pre-#14205 behaviour, unchanged', async () => {
168+
// `list()` reports `nested_named` for it too, but `findFile()` resolves that
169+
// name against `ROOT/view/nested_named.json`, which does not exist: the
170+
// derivation is not a bijection here, so it is not used.
171+
const fsLoader = loader();
172+
173+
expect(await keys()).toContain('nested_named');
174+
expect(await fsLoader.exists(TYPE, 'nested_named')).toBe(false);
175+
expect(await fsLoader.exists(TYPE, path.join('crm', 'nested_named'))).toBe(true);
176+
});
177+
178+
it('RECORD: a nested NAMELESS file is still dropped — the honest drop, #14486', async () => {
179+
// Not a repair this ruling makes: there is no name for it that any other
180+
// door reports. #14486 (one shared name-to-path derivation) is where this
181+
// inverts, deliberately.
182+
const keyed = await loader().loadManyKeyed(TYPE);
183+
184+
expect(keyed.some(entry => (entry.data as { object?: string }).object === 'lead')).toBe(false);
185+
});
186+
187+
it('keys an EXTENSION-LESS file by body.name — findFile() cannot resolve it either', async () => {
188+
const fsLoader = loader();
189+
190+
expect(await keys()).toContain('extensionless_named');
191+
// `findFile()` always appends one of its extensions, so the bare file name
192+
// resolves to nothing; keying by the basename would mint a dead name.
193+
expect(await fsLoader.exists(TYPE, 'extensionless')).toBe(false);
194+
});
195+
196+
it('CONTROL: loadMany() still answers with bodies only, and with every file', async () => {
197+
// The shared walk behind both methods must not leak its envelope, and must
198+
// not start dropping what it read: `loadMany()`'s callers are untouched.
199+
const items = await loader().loadMany(TYPE);
200+
201+
expect(items).toHaveLength(7);
202+
for (const item of items) {
203+
expect(Object.prototype.hasOwnProperty.call(item as object, 'file')).toBe(false);
204+
expect(Object.prototype.hasOwnProperty.call(item as object, 'data')).toBe(false);
205+
}
206+
expect(items).toContainEqual(NAMELESS_CONTAINER);
207+
});
208+
});
209+
210+
describe('#14341 the repair reaches MetadataManager.list()', () => {
211+
it('a flat nameless body reaches list() end to end', async () => {
212+
const items = await coldManager().list(TYPE);
213+
214+
expect(items).toContainEqual(NAMELESS_CONTAINER);
215+
});
216+
217+
it('listDiagnosed() counts it and stays complete-and-not-degraded', async () => {
218+
const result = await coldManager().listDiagnosed(TYPE);
219+
220+
// Nothing threw before the repair and nothing throws after — the loader
221+
// answered successfully both times. What changes is that the short answer
222+
// is no longer served as a full one.
223+
expect(result.degraded).toBe(false);
224+
expect(result.errors).toEqual([]);
225+
expect(result.items).toContainEqual(NAMELESS_CONTAINER);
226+
});
227+
228+
it('CONTROL: a named body is still listed, and still only once', async () => {
229+
const items = await coldManager().list(TYPE);
230+
231+
expect(items).toContainEqual({ name: 'flat_named', label: 'agrees with its basename' });
232+
expect(items.filter(item => (item as { name?: string }).name === 'flat_named')).toHaveLength(1);
233+
});
234+
235+
it('RECORD: the nested nameless body is still absent from list()', async () => {
236+
const items = await coldManager().list(TYPE);
237+
238+
expect(items.some(item => (item as { object?: string }).object === 'lead')).toBe(false);
239+
});
240+
});

0 commit comments

Comments
 (0)