diff --git a/.changeset/filesystem-loader-keyed-items.md b/.changeset/filesystem-loader-keyed-items.md new file mode 100644 index 0000000000..9c76af8f4c --- /dev/null +++ b/.changeset/filesystem-loader-keyed-items.md @@ -0,0 +1,29 @@ +--- +'@objectstack/metadata': patch +--- + +`FilesystemLoader` now implements `loadManyKeyed()`, so a metadata file whose +body has no top-level `name` is no longer invisible to `MetadataManager.list()`. +`loadMany()` globbed files and pushed bodies, discarding the path it had just +read; the manager then fell back to keying by `body.name`, which drops every +nameless body — an aggregated `defineView` container has none by design. That is +the #14205 defect, unrepaired for this loader until now. + +The key is this loader's own name-to-path derivation — the basename minus +extension, the same one `list()` reports — but only where that derivation is a +bijection for the file: it sits directly under `ROOT/TYPE/` and carries an +extension `findFile()` tries, so `findFile(type, key)` resolves back to that same +file. Every other shape (a nested path, an extension-less file) keeps the +previous behaviour verbatim: keyed by `body.name` when it has one, dropped when +it has none. `list()` and `findFile()` disagree outside the flat shape — `list()` +reports the bare basename for a nested file and `findFile()` cannot resolve it — +so keying those by the basename would mint names `get()` and `exists()` cannot +open, and two directories holding one basename would collide silently. Repairing +the derivation itself is tracked separately. + +One deliberate consequence: a flat file whose `body.name` disagrees with its +basename is now keyed by the basename. That is #14205's rule (identity is the +key the store holds an item under, not `body.name`) applied to this loader, and +it aligns `list()` with `listNames()` for that shape. `loadMany()`'s own +signature and answer are unchanged; both methods now share one file walk so +their bodies cannot drift. diff --git a/packages/metadata/src/loaders/filesystem-loader-keyed-items.test.ts b/packages/metadata/src/loaders/filesystem-loader-keyed-items.test.ts new file mode 100644 index 0000000000..8de12162ec --- /dev/null +++ b/packages/metadata/src/loaders/filesystem-loader-keyed-items.test.ts @@ -0,0 +1,240 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #14341 — `FilesystemLoader.loadManyKeyed()`: a file-held item is keyed by the + * name this loader can actually RESOLVE for it, and by nothing else. + * + * --------------------------------------------------------------------------- + * The defect + * --------------------------------------------------------------------------- + * `loadMany()` globbed files and pushed bodies, throwing away the path it had + * just read. `MetadataManager.admitLoaderItems()` then fell back to keying by + * `body.name`, which drops every body that has no top-level `name` — the exact + * #14205 failure, unrepaired for this loader. A `defineView` container has no + * own `name` BY DESIGN, so a file holding one was absent from `list('view')` + * while `listDiagnosed()` called the short answer complete. + * + * --------------------------------------------------------------------------- + * The rule this pins (PM ruling on #14341, 2026-09-02 — option D) + * --------------------------------------------------------------------------- + * An item is keyed by this loader's own name-to-path derivation (the basename + * minus extension, the same derivation `list()` reports) ONLY where that + * derivation is a BIJECTION for the file — it sits directly under `ROOT/TYPE/` + * and carries an extension `findFile()` tries, so `findFile(type, key)` resolves + * back to that same file. Every other shape keeps the pre-#14205 behaviour + * verbatim: keyed by `body.name` when it has one, dropped when it has none. + * + * The ruling was taken over triage's "a nested path keeps whatever `list()` + * reports for it today", knowingly, because the two derivations DISAGREE + * outside the flat shape (measured on `origin/main` @ 253da34c4): `list()` + * reports `account` for `ROOT/TYPE/crm/account.json`, and `findFile()` resolves + * that name against `ROOT/TYPE/account.json` and finds nothing. Keying nested + * files by their basename would mint names `get()` / `load()` / `exists()` + * cannot open, and two directories holding one basename would collide in + * silence. The card's own fence: "keying items under names nothing else uses + * ... is worse than today's honest drop". + * + * --------------------------------------------------------------------------- + * What the RECORD cases are for + * --------------------------------------------------------------------------- + * `RECORD:` cases pin behaviour this ruling deliberately LEAVES ALONE — the + * nested nameless file is still dropped. They exist so the derivation repair + * (#14486: one shared name-to-path function for `list()`, `findFile()` and + * `loadManyKeyed()`) inverts them deliberately, with the change visible in a + * diff, instead of silently. + * + * `CONTROL:` cases are green in both directions and pin "nothing consumers see + * today changes shape". + */ + +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import * as fs from 'node:fs/promises'; +import * as os from 'node:os'; +import * as path from 'node:path'; +import type { MetadataFormat } from '@objectstack/spec/system'; +import { MetadataManager } from '../metadata-manager.js'; +import { FilesystemLoader } from './filesystem-loader.js'; +import { JSONSerializer } from '../serializers/json-serializer.js'; +import type { MetadataSerializer } from '../serializers/serializer-interface.js'; + +const TYPE = 'view'; + +/** The aggregated container shape: identity is the target object, no own `name`. */ +const NAMELESS_CONTAINER = { object: 'account', views: [{ label: 'All' }] }; + +let root: string; + +/** + * One tree holding every shape the rule distinguishes. The two `crm/` files and + * the extension-less one are the shapes where `list()` and `findFile()` disagree. + */ +beforeAll(async () => { + root = await fs.mkdtemp(path.join(os.tmpdir(), 'fsloader-keyed-')); + const typeDir = path.join(root, TYPE); + await fs.mkdir(path.join(typeDir, 'crm'), { recursive: true }); + + const write = (rel: string, body: unknown) => + fs.writeFile(path.join(typeDir, rel), JSON.stringify(body), 'utf-8'); + + await write('flat_nameless.json', NAMELESS_CONTAINER); + await write('flat_named.json', { name: 'flat_named', label: 'agrees with its basename' }); + await write('flat_disagreeing.json', { name: 'not_the_basename', label: 'disagrees' }); + await write('dotted.config.json', { name: 'dotted.config' }); + await write(path.join('crm', 'nested_named.json'), { name: 'nested_named' }); + await write(path.join('crm', 'nested_nameless.json'), { ...NAMELESS_CONTAINER, object: 'lead' }); + await write('extensionless', { name: 'extensionless_named' }); +}); + +afterAll(async () => { + await fs.rm(root, { recursive: true, force: true }); +}); + +function loader(): FilesystemLoader { + const serializers = new Map([ + ['json', new JSONSerializer()], + ]); + return new FilesystemLoader(root, serializers); +} + +/** A cold manager — empty registry, one filesystem loader answering. */ +function coldManager(): MetadataManager { + const manager = new MetadataManager({ formats: ['json'], loaders: [] }); + manager.registerLoader(loader()); + return manager; +} + +async function keys(): Promise { + const keyed = await loader().loadManyKeyed(TYPE); + return keyed.map(entry => entry.name).sort(); +} + +describe('#14341 FilesystemLoader.loadManyKeyed() keys by the resolvable name', () => { + it('keys a FLAT file by its basename, the derivation list() reports', async () => { + expect(await keys()).toEqual( + ['dotted.config', 'extensionless_named', 'flat_disagreeing', 'flat_named', 'flat_nameless', 'nested_named'], + ); + }); + + it('admits a flat NAMELESS body, keyed by the file name', async () => { + // Pre-repair this body had no key at all and fell out of the merge. + const keyed = await loader().loadManyKeyed(TYPE); + const entry = keyed.find(item => item.name === 'flat_nameless'); + + expect(entry).toBeDefined(); + expect(entry!.data).toEqual(NAMELESS_CONTAINER); + }); + + it('never synthesises a name into a body that deliberately has none', async () => { + const keyed = await loader().loadManyKeyed(TYPE); + const entry = keyed.find(item => item.name === 'flat_nameless')!; + + // The key travels BESIDE the body; the body stays byte-identical to disk, + // so `assertMetadataRegisterContract`'s `data.name` check keeps its meaning. + expect(Object.prototype.hasOwnProperty.call(entry.data as object, 'name')).toBe(false); + }); + + it('keys a flat file by its BASENAME even when body.name disagrees', async () => { + // #14205's rule applied to this loader: identity is the key the store holds + // the item under, not `body.name`. `flat_disagreeing.json` says + // `name: 'not_the_basename'`, and the store's key is the file's. + const keyed = await loader().loadManyKeyed(TYPE); + + expect(keyed.map(entry => entry.name)).toContain('flat_disagreeing'); + expect(keyed.map(entry => entry.name)).not.toContain('not_the_basename'); + // ...and the disagreeing body is handed back unedited. + expect(keyed.find(entry => entry.name === 'flat_disagreeing')!.data).toEqual({ + name: 'not_the_basename', + label: 'disagrees', + }); + }); + + it('strips only the final extension, so dotted.config.json keys as dotted.config', async () => { + expect(await keys()).toContain('dotted.config'); + }); + + it('EVERY key it mints resolves back to a file through findFile()', async () => { + // The bijection claim itself, and the reason the disagreeing shapes below + // are NOT keyed by their basename: a minted key that `exists()` cannot open + // is exactly what the card refused. + const fsLoader = loader(); + const derived = ['dotted.config', 'flat_disagreeing', 'flat_named', 'flat_nameless']; + + for (const key of derived) { + expect(await fsLoader.exists(TYPE, key)).toBe(true); + } + }); + + it('keys a NESTED file by body.name — the pre-#14205 behaviour, unchanged', async () => { + // `list()` reports `nested_named` for it too, but `findFile()` resolves that + // name against `ROOT/view/nested_named.json`, which does not exist: the + // derivation is not a bijection here, so it is not used. + const fsLoader = loader(); + + expect(await keys()).toContain('nested_named'); + expect(await fsLoader.exists(TYPE, 'nested_named')).toBe(false); + expect(await fsLoader.exists(TYPE, path.join('crm', 'nested_named'))).toBe(true); + }); + + it('RECORD: a nested NAMELESS file is still dropped — the honest drop, #14486', async () => { + // Not a repair this ruling makes: there is no name for it that any other + // door reports. #14486 (one shared name-to-path derivation) is where this + // inverts, deliberately. + const keyed = await loader().loadManyKeyed(TYPE); + + expect(keyed.some(entry => (entry.data as { object?: string }).object === 'lead')).toBe(false); + }); + + it('keys an EXTENSION-LESS file by body.name — findFile() cannot resolve it either', async () => { + const fsLoader = loader(); + + expect(await keys()).toContain('extensionless_named'); + // `findFile()` always appends one of its extensions, so the bare file name + // resolves to nothing; keying by the basename would mint a dead name. + expect(await fsLoader.exists(TYPE, 'extensionless')).toBe(false); + }); + + it('CONTROL: loadMany() still answers with bodies only, and with every file', async () => { + // The shared walk behind both methods must not leak its envelope, and must + // not start dropping what it read: `loadMany()`'s callers are untouched. + const items = await loader().loadMany(TYPE); + + expect(items).toHaveLength(7); + for (const item of items) { + expect(Object.prototype.hasOwnProperty.call(item as object, 'file')).toBe(false); + expect(Object.prototype.hasOwnProperty.call(item as object, 'data')).toBe(false); + } + expect(items).toContainEqual(NAMELESS_CONTAINER); + }); +}); + +describe('#14341 the repair reaches MetadataManager.list()', () => { + it('a flat nameless body reaches list() end to end', async () => { + const items = await coldManager().list(TYPE); + + expect(items).toContainEqual(NAMELESS_CONTAINER); + }); + + it('listDiagnosed() counts it and stays complete-and-not-degraded', async () => { + const result = await coldManager().listDiagnosed(TYPE); + + // Nothing threw before the repair and nothing throws after — the loader + // answered successfully both times. What changes is that the short answer + // is no longer served as a full one. + expect(result.degraded).toBe(false); + expect(result.errors).toEqual([]); + expect(result.items).toContainEqual(NAMELESS_CONTAINER); + }); + + it('CONTROL: a named body is still listed, and still only once', async () => { + const items = await coldManager().list(TYPE); + + expect(items).toContainEqual({ name: 'flat_named', label: 'agrees with its basename' }); + expect(items.filter(item => (item as { name?: string }).name === 'flat_named')).toHaveLength(1); + }); + + it('RECORD: the nested nameless body is still absent from list()', async () => { + const items = await coldManager().list(TYPE); + + expect(items.some(item => (item as { object?: string }).object === 'lead')).toBe(false); + }); +}); diff --git a/packages/metadata/src/loaders/filesystem-loader.ts b/packages/metadata/src/loaders/filesystem-loader.ts index fe04c0bdb0..14fe5e4ced 100644 --- a/packages/metadata/src/loaders/filesystem-loader.ts +++ b/packages/metadata/src/loaders/filesystem-loader.ts @@ -20,9 +20,20 @@ import type { MetadataSaveResult, } from '@objectstack/spec/system'; import type { Logger } from '@objectstack/core'; -import type { MetadataLoader } from './loader-interface.js'; +import type { MetadataLoader, MetadataKeyedItem } from './loader-interface.js'; import type { MetadataSerializer } from '../serializers/serializer-interface.js'; +/** + * The pre-#14205 key: a body's own top-level `name`, when it has one. Kept for + * exactly the shapes {@link FilesystemLoader.loadManyKeyed} refuses to mint a + * key for, so those items behave precisely as they did before that method + * existed — no regression, and no invented name either. + */ +function ownNameOf(data: unknown): string | null { + const own = (data as { name?: unknown } | null)?.name; + return typeof own === 'string' && own !== '' ? own : null; +} + export class FilesystemLoader implements MetadataLoader { readonly contract: MetadataLoaderContract = { name: 'filesystem', @@ -149,10 +160,86 @@ export class FilesystemLoader implements MetadataLoader { type: string, options?: MetadataLoadOptions ): Promise { + return (await this.loadManyEntries(type, options)).map(entry => entry.data); + } + + /** + * [#14341] The keyed half of {@link loadMany} — see {@link MetadataKeyedItem} + * for why the store's key travels BESIDE the body instead of being folded + * into it. + * + * THE RULE, in one sentence: an item is keyed by this loader's own + * name-to-path derivation — {@link nameFromFilename}, the very basename + * derivation `list()` reports — ONLY where that derivation is a bijection for + * the file (it sits directly under `ROOT/TYPE/` and carries one of the + * extensions {@link findFile} tries, so `findFile(type, key)` resolves back to + * this same file); every other shape keeps the pre-#14205 behaviour verbatim, + * keyed by `body.name` when it has one and dropped when it has none. + * + * Why the rule stops there (PM ruling on #14341, 2026-09-02, knowingly over + * triage's "a nested path keeps whatever `list()` reports for it today"): + * `list()` and `findFile()` DISAGREE outside that shape. For + * `ROOT/TYPE/crm/account.json`, `list()` reports the bare `account`, but + * `findFile()` resolves that name against `ROOT/TYPE/account.json` and finds + * nothing — the only name reaching the file is `crm/account`, which nothing + * reports. An extension-less file is read by `loadMany()` and reported by + * `list()`, and `findFile()` resolves neither. Keying by either side would + * mint a name some other door cannot open, and two directories holding the + * same basename would collide in silence + * (`MetadataManager.admitLoaderItems()` keeps the first and says nothing). + * The card's own fence: "keying items under names nothing else uses … is + * worse than today's honest drop". So the drop stays exactly where the key is + * unsettled, and is pinned as a RECORD in + * `filesystem-loader-keyed-items.test.ts`. Repairing the derivation itself — + * one shared name-to-path function for `list()`, `findFile()` and this method + * — moves `listNames()` output and is #14486, NOT this method's business. + * + * One consequence, deliberate: a flat file whose `body.name` DISAGREES with + * its basename is now keyed by the BASENAME. That is #14205's rule (identity + * is the key the store holds an item under, not `body.name`) applied to this + * loader, and it aligns `MetadataManager.list()` with `listNames()` for that + * shape. + * + * The body is handed back by reference, unchanged: nothing is written into a + * body that deliberately has no `name`. `limit` bounds the items LOADED, + * exactly as `loadMany()` does — an entry the key rule drops has still been + * read and still counts against it. + */ + async loadManyKeyed( + type: string, + options?: MetadataLoadOptions + ): Promise[]> { + const typeDir = path.join(this.rootDir, type); + const keyed: MetadataKeyedItem[] = []; + + for (const entry of await this.loadManyEntries(type, options)) { + const name = + this.resolvableNameForPath(typeDir, entry.file) ?? ownNameOf(entry.data); + + if (name) { + keyed.push({ name, data: entry.data }); + } + } + + return keyed; + } + + /** + * The single walk behind {@link loadMany} and {@link loadManyKeyed}: one glob, + * one serializer pass, one `limit`. Shared so the two can never answer with + * different bodies for the same file — {@link MetadataLoader.loadManyKeyed} + * requires `data` to be "the same body `loadMany()` would return for the + * item", and a second copy of this walk is how that would quietly stop being + * true. + */ + private async loadManyEntries( + type: string, + options?: MetadataLoadOptions + ): Promise<{ file: string; data: T }[]> { const { patterns = ['**/*'], recursive: _recursive = true, limit } = options || {}; const typeDir = path.join(this.rootDir, type); - const items: T[] = []; + const items: { file: string; data: T }[] = []; try { // Build glob patterns @@ -178,7 +265,7 @@ export class FilesystemLoader implements MetadataLoader { if (serializer) { const data = serializer.deserialize(content); - items.push(data); + items.push({ file, data }); } } catch (error) { this.logger?.warn('Failed to load file', { @@ -250,11 +337,7 @@ export class FilesystemLoader implements MetadataLoader { nodir: true, }); - return files.map(file => { - const ext = path.extname(file); - const basename = path.basename(file, ext); - return basename; - }); + return files.map(file => FilesystemLoader.nameFromFilename(file)); } catch (error) { this.logger?.error('Failed to list', undefined, { type, @@ -359,12 +442,55 @@ export class FilesystemLoader implements MetadataLoader { } } + /** + * The extensions {@link findFile} tries, in the order it tries them. Shared + * with {@link resolvableNameForPath} so the set a name can be RESOLVED under + * cannot drift from the set {@link loadManyKeyed} is willing to KEY by. + */ + private static readonly RESOLVABLE_EXTENSIONS = ['.json', '.yaml', '.yml', '.ts', '.js']; + + /** + * The metadata name this loader reports for a file: the basename with its + * extension stripped. One derivation, shared by {@link list} and + * {@link loadManyKeyed}, so the two cannot drift for the shape where they + * agree — `dotted.config.json` is `dotted.config` for both. + */ + private static nameFromFilename(file: string): string { + return path.basename(file, path.extname(file)); + } + + /** + * The key for a file IF this loader's name-to-path mapping is a bijection for + * it: a file directly under `ROOT/TYPE/` carrying an extension + * {@link findFile} tries, so `findFile(type, key)` resolves back to this very + * file. `null` for every other shape — a nested path, an extension-less file, + * an extension spelled in a case `findFile()` does not compose — which is why + * {@link loadManyKeyed} falls back to `body.name` there rather than minting a + * key no other door can open. + */ + private resolvableNameForPath(typeDir: string, file: string): string | null { + const rel = path.relative(typeDir, file); + + // Nested, or outside the type directory altogether. + if (rel === '' || rel.split(path.sep).length !== 1) { + return null; + } + + // Case-SENSITIVE on purpose: `findFile()` composes `name + ext` with these + // exact spellings, so `Foo.JSON` is not resolvable under `Foo`. + if (!FilesystemLoader.RESOLVABLE_EXTENSIONS.includes(path.extname(rel))) { + return null; + } + + return FilesystemLoader.nameFromFilename(rel); + } + /** * Find file for a given type and name */ private async findFile(type: string, name: string): Promise { const typeDir = path.join(this.rootDir, type); - const extensions = ['.json', '.yaml', '.yml', '.ts', '.js']; + const extensions = FilesystemLoader.RESOLVABLE_EXTENSIONS; for (const ext of extensions) { const filePath = path.join(typeDir, `${name}${ext}`);