diff --git a/.changeset/settings-getmany-all-or-nothing-doc.md b/.changeset/settings-getmany-all-or-nothing-doc.md new file mode 100644 index 0000000000..8638452552 --- /dev/null +++ b/.changeset/settings-getmany-all-or-nothing-doc.md @@ -0,0 +1,36 @@ +--- +"@objectstack/service-settings": patch +--- + +docs(service-settings): state `getMany`'s all-or-nothing key validation on the declaration that owns it (#11680) + +Documentation and a pin. **No behaviour change** — the accept set and every +resolved value are byte-identical. + +`SettingsService.getMany` validates **every** requested key against the +namespace manifest before it reads a single env override and before it loads a +single row, so one undeclared key rejects the whole call with +`UnknownKeyError` (`code: 'SETTINGS_UNKNOWN_KEY'`) and the caller receives +nothing — not the subset it was entitled to. N per-key `get()` calls behave +differently on exactly that input: each declared key still answers, and only +the undeclared one throws. + +The doc comment was otherwise detailed — it explained the grouped row load and +the env-override ordering, and claimed row-for-row equivalence with per-key +`get` "BY CONSTRUCTION" — but never drew this line. That equivalence claim +holds for every key that *resolves* and not for the refusal, so a batched +consumer had to rediscover the rule from a test. `resolveLocalizationContext` +was the first to inherit it and had to record the consequence locally: a host +registering a **partial** `localization` manifest loses all its keys at once +and drops to a shorter cascade, where the per-key path would still have +resolved the declared ones. + +`getMany`'s doc comment now states the rule, its blast radius, why validating +ahead of the grouped walk makes the refusal independent of key order and scope +grouping, and what a caller on a partial manifest should expect. + +The pre-existing pin asserted only `rejects.toThrow(/nope/)` — green whatever +the blast radius is. A sibling pin now asserts the property instead: the error +envelope (`code: 'SETTINGS_UNKNOWN_KEY'`), that **zero** rows were loaded +(the refusal is up-front, with the undeclared key last in the request), and +the contrast that per-key `get()` still answers each declared key. diff --git a/packages/services/service-settings/src/settings-getmany.test.ts b/packages/services/service-settings/src/settings-getmany.test.ts index b8e2d321a1..db0cee8f32 100644 --- a/packages/services/service-settings/src/settings-getmany.test.ts +++ b/packages/services/service-settings/src/settings-getmany.test.ts @@ -20,6 +20,7 @@ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; import { SettingsService } from './settings-service.js'; +import { UnknownKeyError } from './settings-service.types.js'; // WHERE-matcher gate: implement exactly the combinators the service emits and // THROW on the rest — a bare field-equality read of `$or` would silently match @@ -118,6 +119,42 @@ describe('[#10826] SettingsService.getMany', () => { await expect(svc.get('localization', 'nope')).rejects.toThrow(/nope/); }); + // [#11680] The rule the doc comment now states, pinned as a PROPERTY rather + // than as "it throws": the refusal is TOTAL (no partial Record), it lands + // BEFORE any row load, and it is the one input on which `getMany` and N + // per-key `get()` calls part ways. The sibling above asserts only that the + // message names the bad key — which stays green whatever the blast radius is. + it('one undeclared key rejects the WHOLE call — before any row load, no partial result', async () => { + const { svc, engine } = await makeService(); + engine.find.mockClear(); + + const err: unknown = await svc + .getMany('localization', ['timezone', 'currency', 'nope']) + .then(() => null, (e: unknown) => e); + + // The envelope, not just the throw. This is a service-layer error class: + // it carries `code` and no `status` (no HTTP boundary here), so `code` is + // the whole machine-readable envelope there is to assert. + expect(err).toBeInstanceOf(UnknownKeyError); + expect((err as UnknownKeyError).code).toBe('SETTINGS_UNKNOWN_KEY'); + expect((err as Error).message).toMatch( + /Key 'nope' is not declared in manifest 'localization'/, + ); + + // TOTAL and UP-FRONT: the two DECLARED keys were neither answered nor even + // loaded — validation runs ahead of the grouped `loadRows`. + expect(engine.find).toHaveBeenCalledTimes(0); + + // ...and here is the non-equivalence: per-key `get()` still answers every + // declared key on the same input; only the undeclared one throws. Asserted + // on the resolved cascade LAYER, not on the literal — this fixture stores + // JSON text in `value` while the service persists values verbatim, so a + // literal here would pin the fixture's encoding rather than the rule. + expect((await svc.get('localization', 'timezone')).source).toBe('global'); + expect((await svc.get('localization', 'currency')).source).toBe('tenant'); + await expect(svc.get('localization', 'nope')).rejects.toBeInstanceOf(UnknownKeyError); + }); + it('getNamespace resolves through the grouped path with unchanged answers', async () => { const { svc, engine } = await makeService(); const ctx = { userId: 'u1' }; diff --git a/packages/services/service-settings/src/settings-service.ts b/packages/services/service-settings/src/settings-service.ts index cf4a18920d..f28fa8f381 100644 --- a/packages/services/service-settings/src/settings-service.ts +++ b/packages/services/service-settings/src/settings-service.ts @@ -1218,6 +1218,40 @@ export class SettingsService { * the env-override branch, the scope→userId mapping, and the cascade are * the same code ({@link resolveKeyFromRows} is extracted from `get`, not * copied). Nothing is cached; nothing survives the call. + * + * ## Key validation is UP-FRONT and TOTAL + * + * That equivalence covers every key that RESOLVES. It does not cover the + * refusal, and this is the one respect in which `getMany` is not N `get` + * calls — so it is stated here rather than left to be rediscovered from a + * test. + * + * EVERY requested key is checked against the namespace's manifest before a + * single env override is read and before any row is loaded. One undeclared + * key therefore rejects the WHOLE call — {@link UnknownKeyError}, `code: + * 'SETTINGS_UNKNOWN_KEY'` — and the caller receives NOTHING: no partial + * `Record`, not even the subset it was entitled to. N per-key {@link get} + * calls part ways on exactly this input: each declared key still answers, + * and only the undeclared one throws. Same error class, same code; the + * blast radius is what differs. (An unregistered NAMESPACE is refused first + * and identically to `get`: {@link UnknownNamespaceError}.) + * + * Validating before the grouped walk rather than inside it is what makes + * the refusal independent of key order, of scope grouping, and of which + * keys happened to carry an env override — the call either refuses or + * answers all of them, never something in between. `setMany` pre-flights + * its whole patch the same way. + * + * What it costs a caller: against a host that registers a PARTIAL manifest, + * a batched consumer loses ALL of its keys at once and must degrade for the + * whole set — it cannot fall back key by key. `resolveLocalizationContext` + * is the first consumer to inherit this and records its own degradation + * locally: a partial `localization` manifest drops it to a shorter cascade + * (no `global` scope layer, no `OS_LOCALIZATION_*` override) where the + * per-key path would still have resolved the declared keys. A caller that + * cannot afford the rule should intersect `keys` with the manifest itself, + * or read per key. {@link getNamespace} can never trip it — it passes + * exactly the registered keys. */ async getMany( namespace: string,