From 3476211e3b286177dae6be4e5b3a1ea88e62853b Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 1 Aug 2026 13:44:11 +0000 Subject: [PATCH 1/4] fix(cli,lint): run validateFormLayout, and close the rule registry from the other side (#4449) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `validateFormLayout` was implemented, unit-tested, exported and given published rule ids — and no command ever called it. A whole-repo search found the implementation, the barrel export line and its own unit test, and nothing else: the rule ran on zero stacks for as long as it existed. Two changes: * register it in `AUTHORING_RULES` as `advisory` on all three commands. It walks structured metadata only (no lazy dependency), so `os validate`, `os build` and `os lint` pay nothing measurable for it. * add the reverse closure to the wiring guard. Every invariant #4409 shipped starts FROM a registry and looks at the commands, which cannot see a rule that never entered a registry — the same blind spot as #4402's name list, one layer up. The guard now subtracts both registries from the `validate*` / `lint*` symbols on `@objectstack/lint`'s public barrel; the difference must be empty or ledgered with a reason in `UNWIRED_RULE_LEDGER`, which ships empty because today's difference was exactly this one rule. The new tests fail without the registry entry: the closure reports `validateFormLayout` as unwired, and the liveness test asserts the entry's own `run` adapter returns both findings for a stack that earns them — membership alone is not evidence a rule produces output. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_017gEHJN2NFpS9VMeURvakgD --- .changeset/form-layout-lint-wired.md | 25 +++++ .../commands/authoring-rule-wiring.test.ts | 106 ++++++++++++++++++ packages/cli/src/lint/authoring-rules.ts | 16 +++ 3 files changed, 147 insertions(+) create mode 100644 .changeset/form-layout-lint-wired.md diff --git a/.changeset/form-layout-lint-wired.md b/.changeset/form-layout-lint-wired.md new file mode 100644 index 0000000000..5bcb972a16 --- /dev/null +++ b/.changeset/form-layout-lint-wired.md @@ -0,0 +1,25 @@ +--- +"@objectstack/cli": minor +--- + +Wire `validateFormLayout` into the authoring-rule registry, and close the +registry from the other direction (#4449). + +`validateFormLayout` was implemented, unit-tested, exported from +`@objectstack/lint` and given published rule ids (`form-field-unknown`, +`absolute-colspan-discouraged`) — and **no command ever called it**. It ran on +zero stacks for as long as it existed, so a form section referencing a field +that is not on the bound object, or pinning an absolute `colSpan` under a +per-surface derived column count, produced no output anywhere. It is now an +`advisory` entry in `AUTHORING_RULES`, so `os validate`, `os build` and +`os lint` all run it. It is a pure structured-metadata walk with no lazy +dependency, so all three commands pay nothing measurable. + +The wiring guard (#4409) could not have found this. Every one of its invariants +starts FROM a registry and looks at the commands, which is blind by construction +to a rule that never entered a registry — the same shape as #4402's name list +guarding only the names on it, one layer up. The guard now also runs the reverse +subtraction: every `validate*` / `lint*` symbol on `@objectstack/lint`'s public +barrel, minus `AUTHORING_RULES` ∪ `REFERENCE_INTEGRITY_RULES`, must be empty or +carry a written reason in `UNWIRED_RULE_LEDGER`. The ledger ships empty: today's +difference was exactly this one rule. diff --git a/packages/cli/src/commands/authoring-rule-wiring.test.ts b/packages/cli/src/commands/authoring-rule-wiring.test.ts index 4e87a70a5c..7ef82819a7 100644 --- a/packages/cli/src/commands/authoring-rule-wiring.test.ts +++ b/packages/cli/src/commands/authoring-rule-wiring.test.ts @@ -120,6 +120,36 @@ const REGISTRY_NAMES = new Set([ ...REFERENCE_INTEGRITY_RULES.map((r) => r.name), ]); +/** + * Rules `@objectstack/lint` EXPORTS but that no authoring command runs, each + * with the reason it is legitimately unwired (#4449). + * + * Empty, and that is the healthy state. An entry here is a written claim that + * the rule has a consumer OTHER than the three commands (a Studio panel, an MCP + * authoring surface) — not a parking space for one nobody got round to wiring. + * Under ADR-0049 enforce-or-remove, a rule with no consumer at all is deleted, + * not ledgered. + */ +const UNWIRED_RULE_LEDGER: Readonly> = {}; + +/** + * Every `validate*` / `lint*` symbol the lint package's public barrel exports — + * read from source for the same reason the call-site scans are: vitest inlines + * imports, so the module namespace object cannot tell an exported RULE from an + * exported helper the way the naming convention can. + */ +function exportedLintRules(): string[] { + const source = readFileSync(join(repoRoot, 'packages/lint/src/index.ts'), 'utf8'); + const names = new Set(); + for (const m of source.matchAll(/export\s+(?:type\s+)?\{([^}]*)\}/g)) { + for (const raw of m[1].split(',')) { + const name = raw.trim().replace(/^type\s+/, '').split(/\s+as\s+/).pop()?.trim(); + if (name && /^(?:validate|lint)[A-Z]/.test(name)) names.add(name); + } + } + return [...names].sort(); +} + /** Every `lintFoo(`/`validateFoo(` call site in a source file. */ function ruleCallsIn(source: string): string[] { return [...new Set(source.match(/\b(?:lint|validate)[A-Z]\w*(?=\s*\()/g) ?? [])]; @@ -285,6 +315,76 @@ describe('authoring-rule registry wiring (#4409)', () => { ).toEqual([]); }); + // ── The other direction: a rule wired NOWHERE (#4449) ──────────────── + + /** + * The invariants above all start FROM a registry and look at the commands. + * That view is blind by construction to a rule that never entered a registry: + * `validateFormLayout` was implemented, unit-tested, exported and given four + * published rule ids, and ran on zero stacks for as long as it existed. The + * closure is the reverse subtraction — exported rules MINUS both registries — + * which is the shape #4402 (a name list guards only the names on it) and + * #4409 (a registry guards only what entered it) each missed one layer down. + */ + it('every rule @objectstack/lint exports is wired into a registry', () => { + const unwired = exportedLintRules() + .filter((name) => !REGISTRY_NAMES.has(name)) + .filter((name) => !(name in UNWIRED_RULE_LEDGER)); + + expect( + unwired, + `@objectstack/lint exports ${unwired.length} rule(s) that no authoring command runs: ` + + `${unwired.join(', ')}.\n` + + `A rule on the public export surface reads — to a human and to an AI author alike — as a ` + + `check the platform performs. Either register it in AUTHORING_RULES ` + + `(packages/cli/src/lint/authoring-rules.ts) so all three commands run it, or add it to ` + + `UNWIRED_RULE_LEDGER in this file WITH the real consumer that justifies it — or delete it ` + + `under ADR-0049 enforce-or-remove. Advertising it while running it nowhere is the one option ` + + `that is not available (Prime Directive #10).`, + ).toEqual([]); + }); + + it('the form-layout rule really runs, and really finds something', () => { + // The wiring assertion above proves membership. This proves the entry is + // live end to end: the rule reaches all three commands AND its `run` + // adapter returns the finding a broken stack earns. Both halves matter — + // #4449 is precisely a rule that existed, passed its own unit tests, and + // produced no output on any real stack. + for (const command of AUTHORING_COMMANDS) { + expect( + authoringRulesFor(command).map((r) => r.name), + `os ${command} must run validateFormLayout`, + ).toContain('validateFormLayout'); + } + + const entry = AUTHORING_RULES.find((r) => r.name === 'validateFormLayout')!; + const findings = entry.run( + { + objects: [{ name: 'widget', fields: { title: { type: 'text' } } }], + views: [ + { + name: 'widget_form', + data: { object: 'widget' }, + sections: [{ fields: [{ field: 'no_such_field', colSpan: 2 }] }], + }, + ], + }, + {}, + ); + expect(findings.map((f) => f.rule).sort()).toEqual([ + 'absolute-colspan-discouraged', + 'form-field-unknown', + ]); + }); + + it('every ledger entry is still an exported rule', () => { + // Same anti-rot discipline as the two ratchets: an entry naming a rule that + // no longer exists silently widens the allowance for the next one. + const exported = new Set(exportedLintRules()); + const stale = Object.keys(UNWIRED_RULE_LEDGER).filter((n) => !exported.has(n)); + expect(stale, `UNWIRED_RULE_LEDGER entries no longer exported: ${stale.join(', ')}`).toEqual([]); + }); + // ── Guards the guard ───────────────────────────────────────────────── it('the registry is non-empty and still holds the rules that motivated it', () => { @@ -314,6 +414,12 @@ describe('authoring-rule registry wiring (#4409)', () => { expect(emitsError("severity: 'error',")).toBe(true); expect(emitsError("severity: 'error' | 'warning';")).toBe(false); expect(emitsError("if (f.severity === 'error') return;")).toBe(false); + // The export scan feeding the unwired-rule closure: if it stops matching, + // that set difference is empty for the wrong reason. + const exported = exportedLintRules(); + expect(exported.length).toBeGreaterThan(20); + expect(exported).toContain('validateReferenceIntegrity'); + expect(exported).toContain('validateFormLayout'); }); it('every ratchet entry is still load-bearing', () => { diff --git a/packages/cli/src/lint/authoring-rules.ts b/packages/cli/src/lint/authoring-rules.ts index 60708afc8b..f9e7c62fea 100644 --- a/packages/cli/src/lint/authoring-rules.ts +++ b/packages/cli/src/lint/authoring-rules.ts @@ -89,6 +89,7 @@ import { validateApprovalApprovers, validateRecordTitle, validateSemanticRoles, + validateFormLayout, validateSeedReplaySafety, validateSeedStateMachine, validateVisibilityPredicates, @@ -386,6 +387,21 @@ export const AUTHORING_RULES: readonly AuthoringRule[] = [ source: 'packages/lint/src/validate-semantic-roles.ts', run: (stack) => validateSemanticRoles(stack), }, + // #2578 / #4449 — a form section's field reference that resolves to nothing + // (silently not rendered) and an absolute `colSpan` under a per-surface + // derived column count. Advisory: the renderer skips the unknown field and + // clamps the span, so nothing is broken — but each is almost certainly an + // authoring mistake, and until #4449 this rule ran on no command at all. + // Pure structured-metadata walk (no lazy dependency), so wiring it to all + // three costs nothing measurable. + { + name: 'validateFormLayout', + tier: 'advisory', + input: 'parsed', + commands: ALL, + source: 'packages/lint/src/validate-form-layout.ts', + run: (stack) => validateFormLayout(stack), + }, // ADR-0078 Phase 3 (Tier-A `action-locations`) — an action that declares no // `locations` and that no view places by name renders on no surface at all. // objectui#3142 made that measurable: four renderers used to show an From c870ea9fcbcc6d36712379f9efa69f03acd8ce04 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 1 Aug 2026 13:52:15 +0000 Subject: [PATCH 2/4] fix(spec): a stored reference holding an embedded record is not a valid id (#4455) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `os migrate value-shapes` is the evidence half of the ADR-0104 D1 per-deployment gate, and the scan's own header names the case it exists for: "a `location` stored as `{latitude, longitude}` or a `lookup` holding an expanded record object". The second case was never detected. `ReferenceIdValueSchema` was `z.string().min(1)`, and in a SQL deployment a legacy embedded reference reaches storage as JSON TEXT — a non-empty string. So a deployment carrying exactly the values the gate exists to find ran the scan, was told it was clean, and closed the gate with `--apply`; and because the scan deliberately imports the write-path predicate, the write path was equally blind, so the value also survived future writes. `ReferenceIdValueSchema` now rejects a value whose first non-space character is `{` or `[`, in the expanded form too — `$expand` produces an object, never its serialization. Deliberately narrower than an id charset. `FileReferenceIdValueSchema` can bound its alphabet because a `sys_file` id is minted by the platform and nothing else; a reference id is whatever the target object's key holds, including an external key an ADR-0015 federated datasource supplies. So this rejects the shape that is provably not an id and leaves the alphabet to the object that owns it — `CB0-2026-0001`, `SFDC:001xx…` and `ops/eu-west/tenant-7` stay valid, and the tests pin that. Regression coverage is at the GATE, not just the schema: the scan test plants the serialized embedded record, asserts it is counted, and asserts `valueShapeScanPassed()` is false — the deployment may not record the flag — then asserts the same value is a write rejection under strict, so the scan and the validator still answer with one predicate. Reaches authors through the ADR-0104 warn-first path until a deployment opts into strict, so nothing starts rejecting writes on upgrade. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_017gEHJN2NFpS9VMeURvakgD --- .changeset/reference-id-embedded-record.md | 36 ++++++++++++++++ .../src/validation/scan-value-shapes.test.ts | 39 +++++++++++++++++ packages/spec/src/data/field-value.test.ts | 27 ++++++++++++ packages/spec/src/data/field-value.zod.ts | 43 ++++++++++++++++++- 4 files changed, 143 insertions(+), 2 deletions(-) create mode 100644 .changeset/reference-id-embedded-record.md diff --git a/.changeset/reference-id-embedded-record.md b/.changeset/reference-id-embedded-record.md new file mode 100644 index 0000000000..7512c8bd44 --- /dev/null +++ b/.changeset/reference-id-embedded-record.md @@ -0,0 +1,36 @@ +--- +"@objectstack/spec": patch +--- + +A stored reference value that is an embedded record is no longer a valid id +(#4455). + +`os migrate value-shapes` is the evidence half of the ADR-0104 D1 per-deployment +gate, and its own header names the case it exists for: "a `location` stored as +`{latitude, longitude}` **or a `lookup` holding an expanded record object**". The +second case was not detected. `ReferenceIdValueSchema` was +`z.string().min(1)`, and in a SQL deployment a legacy embedded reference reaches +storage as JSON *text* in a TEXT column — a non-empty string. So a deployment +carrying exactly the values the gate exists to find ran the scan, was told it was +clean, and closed the gate with `--apply`; because the scan deliberately imports +the write-path predicate, the write path was equally blind and the value survived +future writes too. + +`ReferenceIdValueSchema` now rejects a value whose first non-space character is +`{` or `[`, in both the stored and the expanded form (`$expand` produces an +object, never its serialization). + +The rejection is deliberately narrower than the issue's first suggestion. Its +file sibling `FileReferenceIdValueSchema` can bound its charset because a +`sys_file` id is minted by the platform and by nothing else; a reference id is +whatever the target object's primary key holds, including an external key an +ADR-0015 federated datasource supplies. So this rejects the shape that is +provably not an id (`{"id":"acc_1","name":"embedded"}`) and leaves the id +alphabet to the object that owns it — `CB0-2026-0001`, `SFDC:001xx…` and +`ops/eu-west/tenant-7` all remain valid. Widening it further needs evidence about +real external keys, not a guess. + +Reaches authors through the ADR-0104 warn-first path (a `[value-shape]` log line) +until a deployment opts into strict, so nothing starts rejecting writes on +upgrade — but the scan now counts these values, and a deployment holding them can +no longer close the gate. diff --git a/packages/objectql/src/validation/scan-value-shapes.test.ts b/packages/objectql/src/validation/scan-value-shapes.test.ts index 59a69c7142..1451290074 100644 --- a/packages/objectql/src/validation/scan-value-shapes.test.ts +++ b/packages/objectql/src/validation/scan-value-shapes.test.ts @@ -103,6 +103,45 @@ describe('scanValueShapes (ADR-0104 D1 / #3438)', () => { expect(valueShapeScanPassed(report)).toBe(false); }); + it('#4455: a lookup holding a SERIALIZED embedded record is found, and closes the gate', async () => { + // The exact case the scan's own header names — and the exact way it reaches + // a SQL deployment: the expanded record object stored as JSON text in a + // TEXT column. It read as a non-empty string, so `ReferenceIdValueSchema` + // waved it through, the scan reported "✓ No malformed values found", and + // `--apply` closed the gate on evidence that was never collected. + const engine = makeEngine({ + contact: [ + { id: 'c1', account: 'acc_1' }, // a real id — must stay clean + { id: 'c2', account: '{"id":"acc_1","name":"embedded"}' }, + { id: 'c3', account: ' {"id":"acc_2"}' }, + ], + }); + const report = await scanValueShapes(engine, silent); + + expect(report.scannedRecords).toBe(3); + expect(report.blocking).toBe(2); + const account = report.findings.find((f) => f.field === 'account')!; + expect(account.count).toBe(2); + expect(account.sampleRecordIds).toEqual(['c2', 'c3']); + expect(account.detail).toMatch(/embedded record object/); + // The verdict the gate reads: this deployment may NOT record the flag. + expect(valueShapeScanPassed(report)).toBe(false); + + // …and the same value is a write rejection under strict, so the flag would + // not have been attesting something the validator disagrees with. + expect(() => + validateRecord( + OBJECTS.contact, + { account: '{"id":"acc_1","name":"embedded"}' }, + 'update', + { valueShapeStrict: true }, + ), + ).toThrow(ValidationError); + expect(() => + validateRecord(OBJECTS.contact, { account: 'acc_1' }, 'update', { valueShapeStrict: true }), + ).not.toThrow(); + }); + it('the scan counts exactly what strict mode rejects — one predicate, not two', async () => { // The anti-drift property: every value the scan flags must also be a write // rejection under the strict gate, and every value it passes must write. diff --git a/packages/spec/src/data/field-value.test.ts b/packages/spec/src/data/field-value.test.ts index aaeeb47ce3..541b4de3b6 100644 --- a/packages/spec/src/data/field-value.test.ts +++ b/packages/spec/src/data/field-value.test.ts @@ -193,6 +193,33 @@ describe('valueSchemaFor — stored form (field-zoo reality)', () => { ok({ type: 'lookup' }, 'acc_1', 'expanded'); // unresolvable ids stay ids }); + it('#4455: a SERIALIZED embedded record is not an id, in either form', () => { + // The shape the ADR-0104 D1 scan's own header names — "a `lookup` holding + // an expanded record object" — as it actually reaches a SQL deployment: as + // JSON text in a TEXT column. `z.string().min(1)` accepted it, so the scan + // reported clean on the one case it exists to find. + for (const type of ['lookup', 'master_detail', 'user', 'tree']) { + bad({ type }, '{"id":"acc_1","name":"embedded"}'); + bad({ type }, ' {"id":"acc_1"}'); // padded — same value, still not an id + bad({ type }, '[{"id":"acc_1"}]'); // the multi-value flavour + // …and the expanded read form must not launder it either: `$expand` + // produces an OBJECT, never its serialization. + bad({ type }, '{"id":"acc_1","name":"embedded"}', 'expanded'); + } + bad({ type: 'lookup', multiple: true }, ['acc_1', '{"id":"acc_2"}']); + + // Narrow on purpose: the rejection is "this is an embedded record", not an + // id alphabet. A reference id is whatever the target object's key holds — + // including an external key an ADR-0015 federated datasource supplies — so + // every one of these stays valid. + ok({ type: 'lookup' }, 'acc_synthetic_0001'); + ok({ type: 'lookup' }, '0e2f4c1a-9b7d-4e3f-8a1b-2c3d4e5f6a7b'); + ok({ type: 'lookup' }, 'CB0-2026-0001'); + ok({ type: 'lookup' }, 'SFDC:001xx000003DGb2AAG'); // external key, punctuated + ok({ type: 'lookup' }, 'ops/eu-west/tenant-7'); // and pathy + ok({ type: 'user' }, 'usr_system'); + }); + it('D3 wave 2: the STORED media form is an opaque sys_file id', () => { ok({ type: 'file' }, 'file_01HXYZ'); ok({ type: 'file' }, '0e2f4c1a-9b7d-4e3f-8a1b-2c3d4e5f6a7b'); diff --git a/packages/spec/src/data/field-value.zod.ts b/packages/spec/src/data/field-value.zod.ts index 5175ecfeb7..175c64e93a 100644 --- a/packages/spec/src/data/field-value.zod.ts +++ b/packages/spec/src/data/field-value.zod.ts @@ -312,8 +312,47 @@ export const FileLikeValueSchema = lazySchema(() => z.union([ FileValueSchema, ])); -/** Record-id string — the stored form of every reference type. */ -export const ReferenceIdValueSchema = lazySchema(() => z.string().min(1)); +/** + * A stored reference value that is really an EMBEDDED RECORD, serialized. + * + * In a document store the expanded form arrives as an object and `z.string()` + * already rejects it. In a SQL deployment the same value reaches storage as + * JSON *text* in a TEXT column — a non-empty string — which is exactly how a + * legacy embedded reference survives into a relational table. Anchored on the + * first non-space character rather than a `JSON.parse` attempt so the check + * stays allocation-free on the write path: no record id the platform mints, and + * no external key any datasource can supply, begins with `{` or `[`. + */ +const EMBEDDED_REFERENCE_TEXT = /^\s*[[{]/; + +/** + * Record-id string — the stored form of every reference type. + * + * Non-empty is not the whole contract. `os migrate value-shapes` is the + * evidence half of the ADR-0104 D1 per-deployment gate, and its own header + * names "a `lookup` holding an expanded record object" as a case it exists to + * find — but a bare `z.string().min(1)` accepts the JSON text such a value is + * stored as, so the gate closed on evidence it never collected (#4455). The + * scan deliberately imports the write-path predicate, so the write path was + * equally blind and the value survived future writes too. + * + * The rejection is deliberately NARROW — an embedded object/array, not an id + * charset. Its file sibling {@link FileReferenceIdValueSchema} can bound its + * charset because a `sys_file` id is minted by the platform and by nothing + * else; a reference id is whatever the target object's primary key holds, + * including an external key an ADR-0015 federated datasource supplies. So this + * rejects the shape that is provably not an id and leaves the id alphabet to + * the object that owns it. Widening it further needs evidence about real + * external keys, not a guess. + */ +export const ReferenceIdValueSchema = lazySchema(() => + z.string().min(1).refine((v) => !EMBEDDED_REFERENCE_TEXT.test(v), { + message: + 'Expected a record id, but the value is an embedded record object. A reference stores an ' + + 'opaque id; the expanded record is the READ shape ($expand produces it) and is never stored. ' + + 'Replace the value with the referenced record\'s id.', + }), +); function optionCodes(def: ValueShapeFieldDef): string[] { if (!Array.isArray(def.options)) return []; From 4e699b5e5ee90e90fcd1897a972cdacb36874648 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 1 Aug 2026 13:58:51 +0000 Subject: [PATCH 3/4] fix(metadata-protocol): one canonical type key at the /meta boundary (#4432) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #3985 taught the per-type gates to accept both spellings of the `/meta` type segment. It did not FOLD them, so `/meta/actions/x` and `/meta/action/x` addressed two namespaces and the layers below disagreed about which one an item lived in — `SysMetadataRepository` folded to singular on its own, while the authorization tier above it (`isOverlayAllowed`, `isArtifactBacked`), the registry heal below it (`restoreArtifactRegistryView`) and the list hydration all read the caller's spelling. The damaging half was the hydration. `getMetaItems` registered overlay rows back into the SchemaRegistry under `request.type`, so one plural-spelled read minted a PLURAL registry entry; from the next read on `listItems('actions')` was no longer empty, the singular fallback that had been supplying every code-authored action stopped running, and one overlay row hid the entire code-authored listing — on a spelling no DELETE addresses, so it outlived the delete that was meant to lift it and left listing and dispatch disagreeing about a removed item. `saveMetaItem`, `getMetaItem`, `getMetaItems`, `getMetaItemLayered`, `getMetaItemCached` and `deleteMetaItem` now fold the type to its canonical singular as their first act. Reads of data AT REST keep the other-spelling fallback: rows written under a plural `type` before this fix are real and nothing rewrites them on upgrade. What changed is that nothing WRITES or REGISTERS a non-canonical key any more. Regression tests fail without the fold: a plural-spelled read mints a phantom `actions` registry entry and the second read drops the code-authored actions, and `getMetaItem` echoes back the caller's spelling so a client can round-trip it into a second namespace. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_017gEHJN2NFpS9VMeURvakgD --- .changeset/meta-canonical-type-segment.md | 27 +++ packages/metadata-protocol/src/protocol.ts | 64 +++++++ ...rotocol-meta-type-canonicalization.test.ts | 172 ++++++++++++++++++ 3 files changed, 263 insertions(+) create mode 100644 .changeset/meta-canonical-type-segment.md create mode 100644 packages/objectql/src/protocol-meta-type-canonicalization.test.ts diff --git a/.changeset/meta-canonical-type-segment.md b/.changeset/meta-canonical-type-segment.md new file mode 100644 index 0000000000..b01b33fe2b --- /dev/null +++ b/.changeset/meta-canonical-type-segment.md @@ -0,0 +1,27 @@ +--- +"@objectstack/metadata-protocol": patch +--- + +One canonical type key at the `/meta` read/write/delete boundary (#4432). + +#3985 made the per-type gates accept both spellings of the `/meta` type segment +(`/meta/actions` and `/meta/action`). It did not FOLD them, so the two spellings +addressed two different namespaces and the layers below disagreed about which +one an item lived in. `saveMetaItem`, `getMetaItem`, `getMetaItems`, +`getMetaItemLayered`, `getMetaItemCached` and `deleteMetaItem` now fold the type +to its canonical singular (Prime Directive #3) as their first act, so every layer +below them reads one key. + +The damaging consequence was not the duplicate row — it was the shadowing. +`getMetaItems` hydrated overlay rows back into the SchemaRegistry under the +CALLER's spelling, so one plural-spelled read minted a plural registry entry; +from the next read on, `listItems('actions')` was no longer empty, the singular +fallback that had been supplying every code-authored action stopped running, and +a single overlay row hid the entire code-authored listing — on a spelling no +DELETE could address, because the delete path resolved the singular. Listing and +dispatch then disagreed about an item that had been deleted. + +Reads of data AT REST still try the other spelling as a fallback: rows written +under a plural `type` before this fix are real, and nothing rewrites them on +upgrade. What changed is that nothing WRITES or REGISTERS a non-canonical key any +more. diff --git a/packages/metadata-protocol/src/protocol.ts b/packages/metadata-protocol/src/protocol.ts index 03e2841d52..e9bb7fab61 100644 --- a/packages/metadata-protocol/src/protocol.ts +++ b/packages/metadata-protocol/src/protocol.ts @@ -78,6 +78,43 @@ import type { */ const TYPE_TO_FORM: Readonly> = METADATA_FORM_REGISTRY; +/** + * The ONE canonical spelling of a metadata type at the `/meta` read/write/delete + * boundary (#4432). + * + * Prime Directive #3 already fixes the answer — metadata type names are + * SINGULAR (`'action'`, `'view'`), REST paths are plural (`/meta/actions`) — and + * #3985 taught the per-type gates to accept both spellings. What it did not do + * is fold them, so the two spellings addressed two different namespaces and the + * layers below disagreed about which one an item lived in: + * + * - the `SysMetadataRepository` write/delete path already folded to singular, + * while the authorization tier above it (`isOverlayAllowed`, + * `isArtifactBacked`) and the registry heal below it + * (`restoreArtifactRegistryView`) read the caller's spelling; + * - `getMetaItems` registered overlay rows back into the SchemaRegistry under + * the caller's spelling. One plural-spelled read minted a plural registry + * entry, `listItems('actions')` stopped being empty, and the singular + * fallback that had been supplying the code-authored items never ran again — + * so one overlay row shadowed an entire code-authored listing, and survived + * the DELETE that was supposed to lift it. + * + * Folding at the boundary (rather than adding another spelling-tolerant lookup + * one layer down) is Prime Directive #12 applied to a type key: one contract, + * not N dialects. Reads of data AT REST still try the other spelling as a + * fallback — rows written under a plural `type` before this fix are real, and + * nothing rewrites them on upgrade. + */ +function canonicalMetaType(type: string): string { + return PLURAL_TO_SINGULAR[type] ?? type; +} + +/** {@link canonicalMetaType} applied to a `{ type }` request, without mutating the caller's object. */ +function canonicalizeMetaRequestType(request: T): T { + const type = canonicalMetaType(request.type); + return type === request.type ? request : { ...request, type }; +} + /** * [#3770] One-shot flag for the "engine has no schema registry" warning emitted * by {@link ObjectStackProtocolImplementation.assertObjectRegistered}. The @@ -2453,6 +2490,15 @@ export class ObjectStackProtocolImplementation implements } async getMetaItems(request: { type: string; packageId?: string; organizationId?: string; previewDrafts?: boolean }) { + // #4432 — CANONICAL TYPE KEY. See {@link canonicalMetaType}. This one + // is load-bearing twice over: the SchemaRegistry indexes code-authored + // items under the SINGULAR type, and the overlay-hydration branch below + // registers overlay rows back into it under `request.type`. Called with + // the plural spelling, that branch minted a PLURAL registry entry — and + // once `listItems('actions')` was non-empty, the singular fallback that + // had been supplying the 11 code-authored actions stopped running. One + // overlay row shadowed the entire code-authored listing. + request = canonicalizeMetaRequestType(request); const { packageId } = request; let items: unknown[] = []; @@ -2752,6 +2798,8 @@ export class ObjectStackProtocolImplementation implements } async getMetaItem(request: { type: string, name: string, packageId?: string, organizationId?: string, state?: 'active' | 'draft', previewDrafts?: boolean }) { + // #4432 — CANONICAL TYPE KEY. See {@link canonicalMetaType}. + request = canonicalizeMetaRequestType(request); let item: unknown; const orgId = request.organizationId; // Studio's editor opens a draft buffer with `state: 'draft'`; @@ -3069,6 +3117,10 @@ export class ObjectStackProtocolImplementation implements }> { const orgId = request.organizationId; + // #4432 — CANONICAL TYPE KEY. See {@link canonicalMetaType}. The + // three-layer diagnostic must answer for ONE namespace, or `code` and + // `overlay` can be read from two. + request = canonicalizeMetaRequestType(request); // ── code layer: MetadataService.get + registry, BYPASSING overlay ── let code: unknown | null = null; try { @@ -4997,6 +5049,10 @@ export class ObjectStackProtocolImplementation implements // ========================================== async getMetaItemCached(request: { type: string, name: string, cacheRequest?: MetadataCacheRequest, locale?: string }): Promise { + // #4432 — CANONICAL TYPE KEY. See {@link canonicalMetaType}. The ETag + // and the cache entry are keyed by type, so two spellings would cache + // the same item twice and invalidate only one of them. + request = canonicalizeMetaRequestType(request); try { // Delegate to getMetaItem so the customization-overlay read order // (sys_metadata → registry → MetadataService) is honoured here too @@ -5909,6 +5965,8 @@ export class ObjectStackProtocolImplementation implements if (!request.item) { throw new Error('Item data is required'); } + // #4432 — CANONICAL TYPE KEY. See {@link canonicalMetaType}. + request = canonicalizeMetaRequestType(request); // What the history row, the audit row and the watch event record as the // origin of this write. Defaults to this method — the ordinary Studio / // REST / SDK save. The only caller that overrides it is @@ -8237,6 +8295,12 @@ export class ObjectStackProtocolImplementation implements /** [ADR-0094] Outcome of the awaited mutation projector, when one is registered. */ projectionApplied?: MutationProjectionOutcome; }> { + // #4432 — CANONICAL TYPE KEY. See {@link canonicalMetaType}. Without it + // the authorization tier (`isOverlayAllowed` / `isArtifactBacked`) and + // the registry heal (`restoreArtifactRegistryView`) read the caller's + // spelling while the repository deletes under the singular — so a + // DELETE could remove the row and leave the shadow it was meant to lift. + request = canonicalizeMetaRequestType(request); // Two-tier authorization for delete (mirrors saveMetaItem). // • Artifact-backed item → delete becomes a tombstone overlay, // requires `allowOrgOverride`. diff --git a/packages/objectql/src/protocol-meta-type-canonicalization.test.ts b/packages/objectql/src/protocol-meta-type-canonicalization.test.ts new file mode 100644 index 0000000000..e22cc449aa --- /dev/null +++ b/packages/objectql/src/protocol-meta-type-canonicalization.test.ts @@ -0,0 +1,172 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #4432 — the `/meta` type segment folds to ONE canonical namespace. + * + * #3985 made the per-type GATES accept both spellings of the type segment. It + * did not fold them, so `PUT /meta/actions/x` and `PUT /meta/action/x` addressed + * two different overlay namespaces and the layers below disagreed about which + * one an item lived in. The worst of it was not the duplicate row — it was the + * shadowing: `getMetaItems` registered overlay rows back into the SchemaRegistry + * under the CALLER's spelling, so one plural-spelled read minted a plural + * registry entry, `listItems('actions')` stopped being empty, and the singular + * fallback that had been supplying every code-authored action never ran again. + * One overlay row hid an entire code-authored listing, on a spelling that no + * DELETE could reach. + * + * These tests are written against the shape of the defect, not its wording: + * each fails if the fold is removed from `getMetaItems` / `getMetaItem` / + * `saveMetaItem` / `deleteMetaItem`. + */ + +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { ObjectStackProtocolImplementation } from '@objectstack/metadata-protocol'; +import { SchemaRegistry } from './registry.js'; + +/** One env-wide, active overlay row for `rc1_probe`, stored under the canonical type. */ +const OVERLAY_ROW = { + id: 'row_1', + type: 'action', + name: 'rc1_probe', + state: 'active', + organization_id: null, + package_id: null, + version: 1, + metadata: JSON.stringify({ name: 'rc1_probe', label: 'Probe' }), +}; + +describe('#4432 — canonical `/meta` type segment', () => { + let registry: SchemaRegistry; + let engine: any; + let protocol: ObjectStackProtocolImplementation; + let rows: any[]; + + beforeEach(() => { + registry = new SchemaRegistry({ multiTenant: false }); + // Two CODE-AUTHORED actions, indexed the way the artifact loader indexes + // them: under the SINGULAR metadata type name (Prime Directive #3). + registry.registerItem('action', { name: 'code_one', label: 'One' }, 'name'); + registry.registerItem('action', { name: 'code_two', label: 'Two' }, 'name'); + + rows = [OVERLAY_ROW]; + engine = { + registry, + find: vi.fn(async (_table: string, opts: any) => { + const w = opts?.where ?? {}; + return rows.filter((r) => + (w.type === undefined || r.type === w.type) + && (w.name === undefined || r.name === w.name) + && (w.state === undefined || r.state === w.state) + && (w.organization_id === undefined || r.organization_id === w.organization_id)); + }), + findOne: vi.fn(async (table: string, opts: any) => { + const found = await engine.find(table, opts); + return found[0] ?? null; + }), + insert: vi.fn(async () => ({ id: 'new' })), + update: vi.fn(async () => ({ id: 'row_1' })), + delete: vi.fn(async (_t: string, opts: any) => { + const id = opts?.where?.id; + rows = rows.filter((r) => r.id !== id); + return { deleted: 1 }; + }), + count: vi.fn(async () => 0), + aggregate: vi.fn(async () => []), + }; + protocol = new ObjectStackProtocolImplementation(engine); + }); + + const namesOf = (res: any): string[] => + (Array.isArray(res) ? res : res?.items ?? []).map((i: any) => i?.name).sort(); + + it('a plural-spelled list is artifact ∪ overlay — never overlay-only', async () => { + // The step-3 symptom: `GET /meta/actions` returned ONLY the overlay and + // hid every code-authored action. + expect(namesOf(await protocol.getMetaItems({ type: 'actions' }))) + .toEqual(['code_one', 'code_two', 'rc1_probe']); + expect(namesOf(await protocol.getMetaItems({ type: 'action' }))) + .toEqual(['code_one', 'code_two', 'rc1_probe']); + }); + + it('reading the plural spelling does not mint a phantom namespace', async () => { + // The mechanism, pinned directly. The first read used to REGISTER the + // overlay under the plural key; from the second read on, the non-empty + // `listItems('actions')` suppressed the singular fallback and the + // code-authored actions were gone for the rest of the process — and the + // phantom outlived any DELETE, because nothing addresses that key. + await protocol.getMetaItems({ type: 'actions' }); + + expect(registry.listItems('actions')).toEqual([]); + expect(namesOf(await protocol.getMetaItems({ type: 'actions' }))) + .toEqual(['code_one', 'code_two', 'rc1_probe']); + // …and repeated reads stay stable rather than degrading once more. + await protocol.getMetaItems({ type: 'actions' }); + expect(namesOf(await protocol.getMetaItems({ type: 'action' }))) + .toEqual(['code_one', 'code_two', 'rc1_probe']); + }); + + it('both spellings resolve the same single item', async () => { + const plural: any = await protocol.getMetaItem({ type: 'actions', name: 'rc1_probe' }); + const singular: any = await protocol.getMetaItem({ type: 'action', name: 'rc1_probe' }); + expect(plural?.item?.name).toBe('rc1_probe'); + expect(singular?.item?.name).toBe('rc1_probe'); + // The response states the CANONICAL type, so a client cannot round-trip + // a non-canonical spelling back into a second namespace. + expect(plural?.type).toBe('action'); + expect(singular?.type).toBe('action'); + }); + + /** + * Every `type` any storage lookup was issued against during the calls so + * far. The write and delete paths run through `SysMetadataRepository`, + * whose full transaction/history machinery is out of scope for a mock — but + * the namespace question is answerable without it. + * + * Stated plainly: the two assertions below already held before this fix. + * `SysMetadataRepository` folded to singular on its own, so the ROW was + * always canonical; what read the caller's spelling was everything around + * it — the authorization tier, the registry heal, and (the damaging one) + * `getMetaItems`' registry hydration. These pin the property so a future + * change cannot reintroduce the split from the write side either. + */ + const queriedTypes = (): string[] => { + const types = new Set(); + for (const call of [...engine.find.mock.calls, ...engine.findOne.mock.calls]) { + const t = call?.[1]?.where?.type; + if (typeof t === 'string') types.add(t); + } + return [...types].sort(); + }; + + it('a plural-spelled write addresses the canonical namespace and no other', async () => { + await protocol.saveMetaItem({ + type: 'actions', + name: 'rc1_probe2', + item: { name: 'rc1_probe2', label: 'Probe 2', target: 'noop' }, + }).catch(() => { /* repository machinery is out of scope — the lookups are not */ }); + + const types = queriedTypes(); + expect(types.length).toBeGreaterThan(0); + expect(types).toContain('action'); + expect(types).not.toContain('actions'); + + const writtenTypes = [ + ...engine.insert.mock.calls.map((c: any[]) => c[1]?.type), + ...engine.update.mock.calls.map((c: any[]) => c[1]?.type), + ].filter((t) => typeof t === 'string'); + expect(writtenTypes).not.toContain('actions'); + }); + + it('a plural-spelled DELETE addresses the canonical namespace and no other', async () => { + // The step-4 symptom: the row a plural PUT created was unreachable by + // either spelling, because the authorization tier and the registry heal + // read the caller's spelling while the repository deleted the singular. + await protocol.deleteMetaItem({ type: 'actions', name: 'rc1_probe' }) + .catch(() => { /* as above */ }); + + const types = queriedTypes(); + expect(types.length).toBeGreaterThan(0); + expect(types).toContain('action'); + expect(types).not.toContain('actions'); + }); +}); From f846babe9d1ece4d1dfa6f234cf10b5861201b6a Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 1 Aug 2026 14:48:16 +0000 Subject: [PATCH 4/4] fix(objectql,service-datasource,runtime): a datasourceMapping rule is routing, not a hint (#4462) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Measured on `main` during the v17 verification: map an object to a Postgres datasource with a bad URL and the boot SUCCEEDS, `/ready` answers 200, the datasource name appears in ZERO log lines, the write returns 201 — and the row is physically in the DEFAULT store. The operator finds out by opening the database they declared and finding it empty. Two causes, one per layer, and each is what makes fixing the other correct: * `ObjectQLEngine.getDriver` step 2 read `mapped && drivers.has(mapped)`, so a MATCHED mapping rule naming a datasource with no live driver fell silently through to the default driver. It now throws — `DatasourceUnavailableError` when the connect layer recorded a verdict (#3828), otherwise an error naming the object, the datasource and the two remedies. `default` still resolves onward: the default driver keeps its natural name (#3826), so `drivers.has('default')` is false by construction and step 5 IS how routing to it works. * ADR-0062 D2's phase-1 note deliberately excluded "mapped" from the auto-connect gate, to keep `examples/app-crm` byte-for-byte unchanged. That note priced only one side. Gate (d) now fires when a mapping rule routes at least one object to a datasource, and a `declared-auto` failure is FATAL with an operator-readable reason — the same call gate (b) already makes for an explicit `object.datasource` binding, correct for (d) now that routing no longer supplies a fallback. `OS_ALLOW_DRIVER_CONNECT_FAILURE` still degrades. The mapped-object list comes from the engine's own matcher (`ObjectQLEngine.resolveMappedDatasource`, newly public) via `connectDeclared({ mappedObjects })`. The connection service never re-derives rule matching: two matchers drifting by one clause would connect a datasource routing never uses, or route to one nothing connects — the defect again. `examples/app-crm`'s mapping is DELETED, and that is what keeps the example unchanged rather than what breaks it. Its `namespace: 'crm'` rule never matched (`namespace` is deprecated; no object sets it) and its `default: true → crm_primary` rule routed everything to an unconnected `:memory:` datasource, i.e. to the default store by fall-through. Honouring it would move the whole app — platform objects included — onto a database empty on every boot. Verified against a real boot on a private port, not only in unit tests: * unchanged CRM example boots healthy; crm_primary/crm_analytics stay `unvalidated` (metadata-only) exactly as before; * with a mapping to `postgres://…@127.0.0.1:1/nonexistent_db`, boot exits 1 with "1 object(s) are routed to it by a datasourceMapping rule (crm_account) and have no fallback datasource — their reads/writes would otherwise land in a DIFFERENT database than the one they declare ⇒ fail-fast per ADR-0062 D5"; * under OS_ALLOW_DRIVER_CONNECT_FAILURE=1 the degraded-boot banner carries the same sentence and the mapped object's seeds fail instead of silently populating the default store. ADR-0062 D2 carries the amendment; the docs page and the data skill now state that a mapping rule is routing and fails the boot when it cannot be honoured. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_017gEHJN2NFpS9VMeURvakgD --- .changeset/datasource-mapping-is-routing.md | 47 +++++++++ .../data-modeling/external-datasources.mdx | 20 +++- docs/adr/0062-external-datasource-runtime.md | 9 ++ examples/app-crm/objectstack.config.ts | 17 +++- .../objectql/src/datasource-mapping.test.ts | 87 +++++++++++++++++ packages/objectql/src/engine.ts | 64 ++++++++++-- packages/runtime/src/app-plugin.ts | 58 ++++++++++- .../datasource-connection-service.test.ts | 63 +++++++++++- .../src/datasource-connection-service.ts | 97 ++++++++++++++----- skills/objectstack-data/rules/datasources.md | 13 ++- 10 files changed, 424 insertions(+), 51 deletions(-) create mode 100644 .changeset/datasource-mapping-is-routing.md diff --git a/.changeset/datasource-mapping-is-routing.md b/.changeset/datasource-mapping-is-routing.md new file mode 100644 index 0000000000..fbb17d71c8 --- /dev/null +++ b/.changeset/datasource-mapping-is-routing.md @@ -0,0 +1,47 @@ +--- +"@objectstack/objectql": minor +"@objectstack/service-datasource": minor +"@objectstack/runtime": minor +--- + +A `datasourceMapping` rule is routing, not a hint — an object mapped to an +unreachable datasource no longer silently reads and writes the DEFAULT store +(#4462). + +**Observable behavior change; read this before upgrading.** Measured on `main` +during the v17 verification: map an object to a Postgres datasource with a bad +URL and the boot succeeds, `/ready` answers `200`, the datasource name appears in +**zero** log lines, `POST /api/v1/data/` returns `201` — and the +row is physically in the default store. The operator finds out by opening the +database they declared and finding it empty. ADR-0062 D2's phase-1 note called a +mapping-only datasource "decorative" to keep an example byte-for-byte unchanged; +what that bought was a silent data-placement bug. + +The fix is a pair, and each half is what makes the other correct: + +1. **Routing stops falling through** (`@objectstack/objectql`). `getDriver` step + 2: a mapping rule that MATCHES and names a datasource with no live driver now + throws — `DatasourceUnavailableError` when the connect layer recorded a + verdict, otherwise an error naming the object, the datasource and the two + remedies. `default` still resolves onward: the default driver keeps its + natural name (#3826), so step 5 is how routing to it works. +2. **ADR-0062 D2 grows gate (d)** (`@objectstack/service-datasource`, + `@objectstack/runtime`). A datasource a mapping rule routes at least one + object to is auto-connected at boot, and a boot-time connect failure is + **fatal** with an operator-readable reason — the same call gate (b) already + makes for an explicit `object.datasource` binding, now correct for (d) + because half 1 removed the fallback. `OS_ALLOW_DRIVER_CONNECT_FAILURE` still + degrades the boot instead, as for every other fatal connect. + +The mapped-object list is resolved by the boot path from the engine's own +matcher (`ObjectQLEngine.resolveMappedDatasource`, newly public) and passed to +`connectDeclared({ mappedObjects })`; the connection service never re-derives +rule matching. Two matchers drifting by one clause would connect a datasource +routing never uses, or route to one nothing connects — the defect again. + +**What to do if this breaks your boot.** It means a `datasourceMapping` rule in +your stack points at a datasource that cannot be connected. Either fix the +datasource configuration, or delete the rule — the second is what +`examples/app-crm` did in this change, and it is what keeps that example's +runtime behavior identical: its rules routed everything to an unconnected +`:memory:` datasource, i.e. to the default store by fall-through. diff --git a/content/docs/data-modeling/external-datasources.mdx b/content/docs/data-modeling/external-datasources.mdx index 0d40289fbe..2c95e566ea 100644 --- a/content/docs/data-modeling/external-datasources.mdx +++ b/content/docs/data-modeling/external-datasources.mdx @@ -108,12 +108,22 @@ A declared datasource auto-connects when it is **meaningfully addressed**: 1. it is **external** (`schemaMode !== 'managed'`), **or** 2. an object **explicitly** binds to it via `object.datasource === `, **or** -3. it sets **`autoConnect: true`**. +3. it sets **`autoConnect: true`**, **or** +4. a **`datasourceMapping` rule routes at least one object to it**. -A `managed` datasource that nothing explicitly binds to (for example one that is -only referenced by a `datasourceMapping` rule) stays *metadata-only* — visible in -Setup, but not connected — so existing apps are unchanged. Use `autoConnect: true` -to opt such a datasource into a live connection at boot. +A `managed` datasource that nothing routes to stays *metadata-only* — visible in +Setup, but not connected. Use `autoConnect: true` to opt such a datasource into a +live connection at boot. + + +**A mapping rule is routing, not a hint.** If a `datasourceMapping` rule routes an +object to a datasource that cannot be connected, the boot **fails** with the +connect error, and a query against that object throws rather than resolving the +default store. Before v17 it fell through silently: the app booted clean, `/ready` +answered `200`, and the object's rows were written to the *default* database +instead of the one it declared. If you want a declared datasource that routes +nothing, remove the mapping rule rather than relying on the fall-through. + **Escape hatch.** An `onEnable` hook calling `ctx.drivers.register(driver)` is diff --git a/docs/adr/0062-external-datasource-runtime.md b/docs/adr/0062-external-datasource-runtime.md index cbf79f8378..62b6902bb4 100644 --- a/docs/adr/0062-external-datasource-runtime.md +++ b/docs/adr/0062-external-datasource-runtime.md @@ -77,6 +77,15 @@ Introduce a single service that, given a datasource definition, builds a driver Auto-connect must not change apps that today declare datasources that are *decorative* or routed via `datasourceMapping` (e.g. `examples/app-crm`'s `crm_primary`/`crm_analytics`). Gate auto-connect so a declared datasource is only connected when it is meaningfully addressed: **(a)** it is `external` (`schemaMode !== 'managed'`), or **(b)** an object/`datasourceMapping` actually routes to it, or **(c)** it sets an explicit `autoConnect: true`. A managed datasource that nothing routes to stays metadata-only (today's behavior). The `default` datasource keeps its current dedicated bootstrap. This is the load-bearing backward-compat decision. > **Phase 1 implementation note (#2163) — gate (b) is "explicit `object.datasource`", not "mapped".** Implementing D2 against `examples/app-crm` surfaced a conflict between "an object/`datasourceMapping` routes to it" and the "byte-for-byte unchanged" mandate. `app-crm`'s `crm_primary` (`:memory:`, `managed`) *is* referenced by a `datasourceMapping` rule (and is the `default:true` fallback) but has **no** `onEnable` driver, so today `engine.getDriver` finds no `crm_primary` driver and its objects fall through to the `default` driver. Auto-connecting it on the strength of the mapping rule would build a fresh, empty `:memory:` driver and silently divert those objects — a behavior change. So the gate **does not** auto-connect on a `datasourceMapping` rule alone: a *managed* datasource that is only mapped (namespace/package/`default`) is treated as decorative and left metadata-only. Gate (b) fires only when an object **explicitly** binds via `object.datasource === ` — a binding that today *throws* when the driver is unregistered, so auto-connecting it is a strict improvement, never a change. External datasources (a) and `autoConnect:true` (c) are unaffected. See `isDatasourceAddressed()` in `@objectstack/service-datasource`. +> +> **Amendment (#4462) — the phase-1 note is REVERSED: gate (d) is "a mapping rule routes objects here", and mapping-only is no longer decorative.** The note above priced the trade-off with only one side on the table. The other side, measured on `main` during the v17 verification, is what a mapping to an **unreachable** datasource does today: the boot succeeds, `/ready` answers `200`, the datasource name appears in **zero** log lines, `POST /api/v1/data/` returns `201` — and the row is physically in the DEFAULT store. The operator discovers it by opening the database they declared and finding it empty. Weighed against that, "decorative" is not a backward-compatibility guarantee; it is a silent data-placement bug wearing one. `datasourceMapping` reads as routing to every author who writes it, and Route-ownership rule #3 ("absence must be loud; prefer failing to falling back") applies to a routing decision as much as to a mounted surface. +> +> The amendment is a **pair**, and each half is what makes the other correct: +> +> 1. **Routing stops falling through.** `ObjectQLEngine.getDriver` step 2: a mapping rule that MATCHES and names a datasource with no live driver now throws — `DatasourceUnavailableError` when the connect layer recorded a verdict (framework#3828), otherwise a "mapped for object … is not registered" error naming the two remedies. `default` is the one name that still resolves onward: the default driver keeps its natural name (#3826), so `drivers.has('default')` is false by construction and step 5 is how routing to it works. +> 2. **The D2 gate grows (d).** A datasource a mapping rule routes at least one registered object to is auto-connected at boot, and a `declared-auto` failure is **fatal** — the same argument (b) already makes, now true of (d) because half 1 removed the fallback. The object list is resolved by the boot path from the engine's own matcher (`ObjectQLEngine.resolveMappedDatasource`), never re-derived in the connection service: two matchers drifting by one clause would connect a datasource routing never uses, or route to one nothing connects, which is the defect again. +> +> `examples/app-crm`'s mapping was **deleted** in the same change, and that is what keeps the example byte-for-byte unchanged rather than what breaks it: its `namespace: 'crm'` rule never matched (`namespace` is deprecated and no object sets it), and its `default: true → crm_primary` rule routed everything to an unconnected `:memory:` datasource, i.e. to the `default` store by fall-through. Honouring that rule would move the entire app — platform objects included — onto a database that is empty on every boot. Removing the rule states what the example actually does. The general lesson is the one #2163 half-saw: a rule the runtime ignores is not compatibility, it is an unpaid bill. ### D3 — Credentials resolved at connect via `SecretBinder`/`ICryptoProvider` diff --git a/examples/app-crm/objectstack.config.ts b/examples/app-crm/objectstack.config.ts index 20fffd2d59..e705313879 100644 --- a/examples/app-crm/objectstack.config.ts +++ b/examples/app-crm/objectstack.config.ts @@ -56,11 +56,20 @@ export default defineStack({ requires: ['ui', 'automation'], // Infrastructure + // + // No `datasourceMapping`. These two datasources are declared to exercise the + // metadata surface, not to route anything: both are `:memory:`, and every + // object here has always been served by the host's `default` store. The + // mapping that used to sit here (`namespace: 'crm'` + `default: true` → + // `crm_primary`) was decorative — `namespace` is deprecated and no object + // sets it, and `crm_primary` had no live driver, so routing fell through to + // `default`. #4462 stopped routing from falling through, because that + // fall-through is what silently put a mapped object's rows in a different + // database than the one it declared. Deleting the rule is what keeps this + // example's behavior IDENTICAL under the new posture; keeping it would move + // the whole app — platform objects included — onto an in-memory database + // that is empty on every boot. datasources: [CrmDatasource, CrmAnalyticsDatasource], - datasourceMapping: [ - { namespace: 'crm', datasource: 'crm_primary' }, - { default: true, datasource: 'crm_primary' }, - ], // Internationalisation translations: [CrmTranslationBundle], diff --git a/packages/objectql/src/datasource-mapping.test.ts b/packages/objectql/src/datasource-mapping.test.ts index 11598ab7ff..446d49f41d 100644 --- a/packages/objectql/src/datasource-mapping.test.ts +++ b/packages/objectql/src/datasource-mapping.test.ts @@ -177,4 +177,91 @@ describe('DatasourceMapping', () => { const result = await engine.insert('account', { name: 'Test' }); expect(result).toBeDefined(); }); + + // ── #4462: a matched mapping rule is routing, not a hint ────────────── + + describe('a mapped datasource with no live driver never falls through (#4462)', () => { + /** + * The defect this pins: `getDriver` step 2 read + * `mapped && this.drivers.has(mapped)`, so a mapping rule naming a + * datasource that failed to connect (or was never connected at all) fell + * silently to step 5 and the object's rows went to the DEFAULT store. + * Boot succeeded, `/ready` answered 200, the datasource name appeared in + * zero log lines, and the write returned 201 — the operator found out by + * looking in the database they declared and finding it empty. + */ + const registerTask = () => + engine.registry.registerObject( + { name: 'rc1_audit', fields: { title: { type: 'text' } } }, + 'com.example.probe', + 'probe', + 'own', + ); + + it('a write to a mapped-but-unconnected datasource fails loudly instead of hitting default', async () => { + const defaultDriver = createMockDriver('sqlite'); + engine.registerDriver(defaultDriver, true); + engine.setDatasourceMapping([{ objectPattern: 'rc1_audit', datasource: 'broken' }]); + registerTask(); + + await expect(engine.insert('rc1_audit', { title: 'ds-probe' })).rejects.toThrow( + /Datasource 'broken' mapped for object 'rc1_audit' is not registered/, + ); + await expect(engine.find('rc1_audit', {})).rejects.toThrow(/'broken'/); + }); + + it('when the connect verdict is known, the error says WHY rather than "not registered"', async () => { + engine.registerDriver(createMockDriver('sqlite'), true); + engine.setDatasourceMapping([{ objectPattern: 'rc1_*', datasource: 'broken' }]); + registerTask(); + // What `DatasourceConnectionService.recordState` reports after a failed + // boot connect under OS_ALLOW_DRIVER_CONNECT_FAILURE. + engine.markDatasourceUnavailable({ + name: 'broken', + kind: 'failed', + publicDetail: 'analytics database unreachable', + }); + + await expect(engine.insert('rc1_audit', { title: 'x' })).rejects.toThrow( + /analytics database unreachable|ERR_DATASOURCE_UNAVAILABLE|broken/, + ); + }); + + it('a mapping to `default` still resolves — the default driver keeps its natural name', async () => { + // #3826: the default is registered under `sqlite`/`memory`, never under + // the literal `default`, so `drivers.has('default')` is false by + // construction and step 5 is how routing to it works. Turning the + // fall-through into a throw must not break that. + engine.registerDriver(createMockDriver('sqlite'), true); + engine.setDatasourceMapping([{ default: true, datasource: 'default' }]); + registerTask(); + + await expect(engine.insert('rc1_audit', { title: 'ok' })).resolves.toBeDefined(); + }); + + it('an unmatched mapping leaves an object on the default store', async () => { + // The rule set is not a claim about EVERY object — only the ones it + // matches. An object no rule names keeps its old resolution. + engine.registerDriver(createMockDriver('sqlite'), true); + engine.setDatasourceMapping([{ objectPattern: 'other_*', datasource: 'broken' }]); + registerTask(); + + await expect(engine.insert('rc1_audit', { title: 'ok' })).resolves.toBeDefined(); + }); + + it('resolveMappedDatasource is the one matcher the boot path may ask', async () => { + // The boot gate (`isDatasourceAddressed` (d)) must learn which objects a + // rule routes from the SAME resolver routing uses. Two matchers drifting + // by one clause is how you get a datasource connected that routing never + // uses, or routed to and never connected — the defect itself. + engine.setDatasourceMapping([ + { objectPattern: 'rc1_*', datasource: 'broken' }, + { default: true, datasource: 'default' }, + ]); + registerTask(); + + expect(engine.resolveMappedDatasource('rc1_audit')).toBe('broken'); + expect(engine.resolveMappedDatasource('unrelated_object')).toBe('default'); + }); + }); }); diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index faf4f79916..5ede7b877c 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -2205,13 +2205,45 @@ export class ObjectQL implements IObjectQLEngine { } // 2. Check datasourceMapping rules + // + // A rule that MATCHES is a routing decision, not a hint (#4462). It used to + // fall through to steps 3-5 whenever the named datasource had no live + // driver, which put an object's rows in the DEFAULT store while every + // signal said otherwise: boot succeeded, `/ready` answered 200, the + // datasource name appeared nowhere in the log, and the write returned 201. + // An operator who routes an object to Postgres and gets the URL wrong finds + // out by going to look in Postgres and finding it empty. + // + // `default` is the one name that legitimately resolves onward: the default + // driver keeps its NATURAL name (#3826), so `drivers.has('default')` is + // false by construction and step 5 is how routing to it works. const mappedDatasource = this.resolveDatasourceFromMapping(objectName, object); - if (mappedDatasource && this.drivers.has(mappedDatasource)) { - this.logger.debug('Resolved datasource from mapping', { - object: objectName, - datasource: mappedDatasource - }); - return this.drivers.get(mappedDatasource)!; + if (mappedDatasource && mappedDatasource !== 'default') { + if (this.drivers.has(mappedDatasource)) { + this.logger.debug('Resolved datasource from mapping', { + object: objectName, + datasource: mappedDatasource + }); + return this.drivers.get(mappedDatasource)!; + } + // Same three-way diagnosis as an explicit `object.datasource` binding — + // the two are the same promise made in two places, so they owe the reader + // the same answer. + const unavailable = this.unavailableDatasources.get(mappedDatasource); + if (unavailable) { + throw new DatasourceUnavailableError( + mappedDatasource, + objectName, + unavailable.kind, + unavailable.publicDetail, + ); + } + throw new Error( + `[ObjectQL] Datasource '${mappedDatasource}' mapped for object '${objectName}' is not registered. ` + + `A datasourceMapping rule routes this object to it, so falling back to the default store would ` + + `write the object's data to a different database than the one it declares. Fix the datasource ` + + `configuration, or remove the mapping rule.`, + ); } // 3. Lifecycle-class separation (ADR-0057 §3.6): high-frequency @@ -2255,6 +2287,26 @@ export class ObjectQL implements IObjectQLEngine { throw new Error(`[ObjectQL] No driver available for object '${objectName}'`); } + /** + * Which datasource do the mapping rules route `objectName` to, if any? + * + * The PUBLIC face of {@link resolveDatasourceFromMapping}, added for the boot + * path (#4462): the datasource-connection service must connect the + * datasources a mapping actually routes objects to, and it must learn which + * those are from the same resolver the query path uses. A second + * implementation of "does this rule match?" living in the connection service + * would drift by one clause and produce the worst of both postures — a + * datasource connected that routing does not use, or routed to and never + * connected, which is the defect itself. + * + * Returns `null` when no rule matches, and the datasource name (including + * `'default'`) when one does. Rule matching only — an explicit + * `object.datasource` binding outranks this and is not consulted here. + */ + resolveMappedDatasource(objectName: string): string | null { + return this.resolveDatasourceFromMapping(objectName, this._registry.getObject(objectName)); + } + /** * Resolve datasource from mapping rules * diff --git a/packages/runtime/src/app-plugin.ts b/packages/runtime/src/app-plugin.ts index a0a4a7eadc..2ba0d0bc66 100644 --- a/packages/runtime/src/app-plugin.ts +++ b/packages/runtime/src/app-plugin.ts @@ -374,6 +374,49 @@ export class AppPlugin implements Plugin { ctx.logger.debug('[AppPlugin] Installed hook-metrics Server-Timing feed'); } + /** + * Datasource name → the objects a `datasourceMapping` rule routes to it + * (#4462), asked of the ENGINE rather than re-derived here. + * + * The gate this feeds (`isDatasourceAddressed` (d)) and the routing that + * makes it correct (`ObjectQLEngine.getDriver` step 2) must agree exactly + * about which rules match which objects. A second matcher living in this + * plugin — or in the connection service — would drift by one clause and + * produce either a datasource connected that routing never uses, or one + * routed to and never connected, which is the defect itself. + * + * Objects with an EXPLICIT `object.datasource` binding are excluded: that + * binding outranks mapping in `getDriver`, so counting them here would let + * a mapping rule they never obey force a fail-fast on their behalf. + * `default` is excluded for the same reason `getDriver` lets it through — + * the host's default driver is registered under its natural name and needs + * no per-app connect. + */ + private resolveMappedObjects( + ql: IObjectQLEngine, + objects: Array<{ name?: string; datasource?: string }>, + ): Record { + const resolve = (ql as unknown as { + resolveMappedDatasource?: (objectName: string) => string | null; + }).resolveMappedDatasource; + if (typeof resolve !== 'function') return {}; + const out: Record = {}; + for (const obj of objects) { + const name = obj?.name; + if (typeof name !== 'string' || !name) continue; + if (obj.datasource && obj.datasource !== 'default') continue; + let mapped: string | null = null; + try { + mapped = resolve.call(ql, name); + } catch { + continue; // a resolver that throws must not brick boot + } + if (!mapped || mapped === 'default') continue; + (out[mapped] ??= []).push(name); + } + return out; + } + start = async (ctx: PluginContext) => { if (this.empty) { ctx.logger.debug('[AppPlugin] empty env — no app payload, skipping start', { @@ -480,10 +523,10 @@ export class AppPlugin implements Plugin { // + register a live driver via the shared `'datasource-connection'` // service (when present — wired by the datasource-admin plugin). The // service applies the D2 gate (connect only when `external`, an object - // explicitly binds via `object.datasource`, or `autoConnect:true`) and - // the host connect policy, so managed+unrouted datasources stay - // metadata-only (e.g. app-crm's `:memory:` datasources — byte-for-byte - // unchanged). Idempotent vs. a legacy `onEnable` driver registration. + // explicitly binds via `object.datasource`, a `datasourceMapping` rule + // routes objects to it, or `autoConnect:true`) and the host connect + // policy, so a managed datasource nothing routes to stays metadata-only. + // Idempotent vs. a legacy `onEnable` driver registration. // // Runs in `start()` (before the `kernel:ready` external-validation gate) // so the kernel's init-all-then-start-all ordering guarantees the @@ -506,6 +549,7 @@ export class AppPlugin implements Plugin { connectDeclared?: (input: { datasources: any[]; objects?: Array<{ name?: string; datasource?: string }>; + mappedObjects?: Record; }) => Promise>; } | undefined; @@ -516,7 +560,11 @@ export class AppPlugin implements Plugin { } if (typeof connection?.connectDeclared === 'function') { const objects = Array.isArray(this.bundle.objects) ? this.bundle.objects : []; - const results = await connection.connectDeclared({ datasources: dsList, objects }); + const results = await connection.connectDeclared({ + datasources: dsList, + objects, + mappedObjects: this.resolveMappedObjects(ql, objects), + }); const connected = results.filter((r) => r.status === 'connected'); if (connected.length > 0) { ctx.logger.info('Auto-connected declared datasources', { diff --git a/packages/services/service-datasource/src/__tests__/datasource-connection-service.test.ts b/packages/services/service-datasource/src/__tests__/datasource-connection-service.test.ts index f6a43a4be3..1687021a86 100644 --- a/packages/services/service-datasource/src/__tests__/datasource-connection-service.test.ts +++ b/packages/services/service-datasource/src/__tests__/datasource-connection-service.test.ts @@ -117,15 +117,38 @@ describe('isDatasourceAddressed (ADR-0062 D2 gate)', () => { expect(isDatasourceAddressed({ name: 'x', schemaMode: 'managed', autoConnect: true }, { objects: [] })).toBe(true); }); - it('does NOT connect a managed datasource that is only mapped / unrouted (app-crm byte-for-byte unchanged)', () => { - // app-crm: crm_primary is managed + referenced by datasourceMapping only, - // crm_analytics is managed + unrouted. Neither has an object binding. - expect(isDatasourceAddressed({ name: 'crm_primary', schemaMode: 'managed' }, { objects: [] })).toBe(false); + it('connects when a datasourceMapping rule routes objects to it (d) — #4462', () => { + // The gate D2 originally excluded, to keep a mapped-but-unconnected + // datasource falling through to `default`. That fall-through is exactly how + // an object's rows ended up in a database nobody declared, so routing no + // longer performs it — and once a mapped object has no fallback, connecting + // its datasource at boot is the same call gate (b) already makes. + expect( + isDatasourceAddressed( + { name: 'broken', schemaMode: 'managed' }, + { objects: [], mappedObjects: { broken: ['rc1_audit'] } }, + ), + ).toBe(true); + }); + + it('does NOT connect a managed datasource nothing routes to', () => { + // No object binding, no mapping rule that matches anything. expect(isDatasourceAddressed({ name: 'crm_analytics', schemaMode: 'managed' }, { objects: [] })).toBe(false); // An object bound to a DIFFERENT datasource must not flip the gate. expect( isDatasourceAddressed({ name: 'crm_primary', schemaMode: 'managed' }, { objects: [{ name: 'acct', datasource: 'default' }] }), ).toBe(false); + // Nor may a mapping that routes objects SOMEWHERE ELSE, or one that + // matches no object at all (an empty list is not a route). + expect( + isDatasourceAddressed( + { name: 'crm_primary', schemaMode: 'managed' }, + { objects: [], mappedObjects: { other_ds: ['task'], crm_primary: [] } }, + ), + ).toBe(false); + // A host that supplies no mapping information at all keeps the pre-#4462 + // behavior rather than guessing. + expect(isDatasourceAddressed({ name: 'crm_primary', schemaMode: 'managed' }, {})).toBe(false); }); }); @@ -492,6 +515,38 @@ describe('DatasourceConnectionService.connectDeclared', () => { } }); + // #4462 — the boot half of the pair. Before this, an object mapped to an + // unreachable datasource produced NO connect attempt at all: the D2 gate left + // it metadata-only, so the name never appeared in the log, `/ready` stayed + // 200, and the write went to the default store with a 201. + it('a mapping-routed datasource is attempted at boot, and its failure is fatal', async () => { + const ENV = 'OS_ALLOW_DRIVER_CONNECT_FAILURE'; + const saved = process.env[ENV]; + delete process.env[ENV]; + try { + const { service } = svc({ factory: fakeFactory({ connectThrows: true }) }); + const err = await service + .connectDeclared({ + datasources: [{ name: 'broken', driver: 'sqlite', schemaMode: 'managed', config: {} }], + objects: [{ name: 'rc1_audit' }], // no explicit binding — routed by the rule + mappedObjects: { broken: ['rc1_audit'] }, + }) + .then( + () => { throw new Error('connectDeclared() resolved but should have thrown'); }, + (e: unknown) => e as Error, + ); + expect(err.message).toMatch(/^datasource 'broken': connect failed/); + expect(err.message).toContain('datasourceMapping rule'); + expect(err.message).toContain('rc1_audit'); + // The sentence an operator has to be able to act on: their data is NOT + // quietly going somewhere else. + expect(err.message).toContain('DIFFERENT database'); + } finally { + if (saved === undefined) delete process.env[ENV]; + else process.env[ENV] = saved; + } + }); + it('a single fatal failure propagates as-is (no aggregate wrapper to read past)', async () => { const ENV = 'OS_ALLOW_DRIVER_CONNECT_FAILURE'; const saved = process.env[ENV]; diff --git a/packages/services/service-datasource/src/datasource-connection-service.ts b/packages/services/service-datasource/src/datasource-connection-service.ts index 36a5914f41..77a0dda750 100644 --- a/packages/services/service-datasource/src/datasource-connection-service.ts +++ b/packages/services/service-datasource/src/datasource-connection-service.ts @@ -210,32 +210,50 @@ export function availabilityOf(status: ConnectStatus): DatasourceAvailability { * Returns true when: * - (a) it is external (`schemaMode !== 'managed'`), OR * - (b) some object **explicitly** binds to it (`object.datasource === name`), OR - * - (c) it sets `autoConnect: true`. + * - (c) it sets `autoConnect: true`, OR + * - (d) a `datasourceMapping` rule ROUTES at least one registered object to it. * - * Deliberately NOT triggered by a `datasourceMapping` rule alone. A managed - * datasource that is only *mapped* (namespace/package/default) but has no live - * driver historically falls through to the `default` driver at query time - * (`engine.getDriver` step 4) — e.g. `examples/app-crm`'s `crm_primary` - * (`:memory:`, mapped + default-fallback, no `onEnable`). Connecting it would - * divert those objects to a fresh, empty connection and silently change app - * behavior. So mapping-only routing to a *managed* datasource is treated as - * decorative, keeping existing apps byte-for-byte unchanged (D2's load-bearing - * backward-compat guarantee). External datasources and explicit - * `object.datasource` bindings never resolved to `default` (they throw when - * unregistered), so auto-connecting them is a strict improvement, not a change. + * ## (d), and why D2's phase-1 note no longer holds (#4462) * - * That same "no fallback" property is why gate (b) is also a **fail-fast** - * trigger when the connect fails (framework#3758) — see - * {@link DatasourceConnectionService.handleFailure}. Gate (c) is not: nothing - * declares a dependency on an `autoConnect` datasource. + * D2 originally excluded (d) to keep `examples/app-crm` byte-for-byte + * unchanged: its `crm_primary` was mapped but had no driver, so + * `engine.getDriver` fell through to `default` and the app worked. Connecting + * it would have diverted those objects to a fresh, empty connection — a + * behavior change. So a mapping-only managed datasource was declared + * "decorative". + * + * What that traded away was not visible from inside the boot path. An operator + * who maps an object to an unreachable Postgres gets: a clean boot, `/ready` + * 200, the datasource name in zero log lines, a `201` on the write, and their + * rows in the DEFAULT store. They find out by going to look in the database + * they declared and finding it empty. "Decorative" is not what a mapping rule + * reads as; it reads as routing. + * + * The fix is the pair, and each half is what makes the other correct: routing + * no longer falls through when a mapped datasource has no driver, so a mapped + * object now has NO FALLBACK — which is exactly the property that made (b) + * safe to auto-connect and fatal to fail. (d) inherits both. + * + * `ctx.mappedObjects` is supplied by the boot path from the ENGINE's own + * resolver, never re-derived here — see `ObjectQLEngine.resolveMappedDatasource`. + * A host that cannot supply it (no engine yet, no mapping configured) passes + * nothing and (d) simply never fires, which is the pre-#4462 behavior. + * + * Gate (c) is not a fail-fast trigger: nothing declares a dependency on an + * `autoConnect` datasource. */ export function isDatasourceAddressed( ds: Pick, - ctx: { objects?: readonly DatasourceBoundObject[] }, + ctx: { + objects?: readonly DatasourceBoundObject[]; + /** Datasource name → the objects a `datasourceMapping` rule routes to it. */ + mappedObjects?: Readonly>; + }, ): boolean { if (ds.schemaMode && ds.schemaMode !== 'managed') return true; // (a) if (ds.autoConnect === true) return true; // (c) if (ctx.objects?.some((o) => o?.datasource === ds.name)) return true; // (b) + if ((ctx.mappedObjects?.[ds.name]?.length ?? 0) > 0) return true; // (d) return false; } @@ -287,20 +305,33 @@ export class DatasourceConnectionService { async connectDeclared(input: { datasources: readonly ConnectableDatasource[]; objects?: readonly DatasourceBoundObject[]; + /** + * Datasource name → the objects a `datasourceMapping` rule routes to it + * (#4462), resolved by the caller from the ENGINE's own rule matcher so + * this service never re-implements "does this rule match?". Absent ⇒ gate + * (d) never fires, which is the pre-#4462 behavior. + */ + mappedObjects?: Readonly>; }): Promise { const objects = input.objects ?? []; + const mappedObjects = input.mappedObjects ?? {}; const results: ConnectResult[] = []; const fatal: Error[] = []; for (const ds of input.datasources) { if (!ds?.name) continue; if (ds.active === false) continue; - if (!isDatasourceAddressed(ds, { objects })) continue; // D2 gate + if (!isDatasourceAddressed(ds, { objects, mappedObjects })) continue; // D2 gate const bound = objects .filter((o) => o?.datasource === ds.name && typeof o?.name === 'string') .map((o) => o.name as string); + const mapped = mappedObjects[ds.name] ?? []; try { results.push( - await this.connect(ds, { objects: bound, context: { origin: ds.origin ?? 'code', trigger: 'declared-auto' } }), + await this.connect(ds, { + objects: bound, + mappedObjects: mapped, + context: { origin: ds.origin ?? 'code', trigger: 'declared-auto' }, + }), ); } catch (err) { fatal.push(err instanceof Error ? err : new Error(String(err))); @@ -337,6 +368,12 @@ export class DatasourceConnectionService { record: ConnectableDatasource, opts: { objects?: readonly string[]; + /** + * Objects a `datasourceMapping` rule routes here (#4462). Like + * `objects`, these have no fallback since routing stopped falling + * through — so a boot-time failure with any of them is fatal. + */ + mappedObjects?: readonly string[]; context?: DatasourceConnectContext; /** * Register the built driver as the engine's DEFAULT driver, under the @@ -395,7 +432,7 @@ export class DatasourceConnectionService { private async attemptConnect( record: ConnectableDatasource, - opts: { objects?: readonly string[]; context?: DatasourceConnectContext; asDefault?: boolean } = {}, + opts: { objects?: readonly string[]; mappedObjects?: readonly string[]; context?: DatasourceConnectContext; asDefault?: boolean } = {}, ): Promise { const name = record.name; const engine = this.cfg.engine(); @@ -442,6 +479,7 @@ export class DatasourceConnectionService { `no driver factory supports driver '${record.driver}'`, opts.context, opts.objects, + opts.mappedObjects, ); } @@ -469,7 +507,7 @@ export class DatasourceConnectionService { try { secret = await resolver(credentialsRef); } catch (err) { - return this.handleFailure(record, 'failed-credentials', `resolving credential '${credentialsRef}' threw: ${errMsg(err)}`, opts.context, opts.objects); + return this.handleFailure(record, 'failed-credentials', `resolving credential '${credentialsRef}' threw: ${errMsg(err)}`, opts.context, opts.objects, opts.mappedObjects); } if (secret == null || secret === '') { return this.handleFailure( @@ -521,7 +559,7 @@ export class DatasourceConnectionService { this.logger?.info?.(`datasource '${name}': connected (driver=${record.driver}, schemaMode=${record.schemaMode ?? 'managed'})`); return { name, status: 'connected', ...(handle.ownership ? { ownership: handle.ownership } : {}) }; } catch (err) { - return this.handleFailure(record, 'failed-degraded', errMsg(err), opts.context, opts.objects); + return this.handleFailure(record, 'failed-degraded', errMsg(err), opts.context, opts.objects, opts.mappedObjects); } } @@ -593,7 +631,12 @@ export class DatasourceConnectionService { * - **(c)** the host marked it {@link ConnectableDatasource.bootCritical} — * the standalone `default` (#3826): everything WITHOUT a binding routes to * it, so "no fallback" holds by construction, mirroring the engine-level - * guard (#3741) this connect path replaces. + * guard (#3741) this connect path replaces; or + * - **(d)** a `datasourceMapping` rule routes objects to it (#4462). Same + * argument as (b), reached one clause later: since routing stopped falling + * through on a mapped-but-unconnected datasource, those objects have no + * fallback either. Before that, this case was not merely non-fatal — it was + * SILENT, and the objects' rows went to the default store. * * Anything else degrades with a warning: `autoConnect:true` means "connect it * if you can" with nothing declaring a dependency on it, and runtime-admin @@ -616,6 +659,7 @@ export class DatasourceConnectionService { reason: string, context?: DatasourceConnectContext, boundObjects: readonly string[] = [], + mappedObjects: readonly string[] = [], ): ConnectResult { const isExternal = record.schemaMode && record.schemaMode !== 'managed'; const msg = `datasource '${record.name}': connect failed — ${reason}`; @@ -631,6 +675,13 @@ export class DatasourceConnectionService { `and have no fallback datasource — every read/write of them would fail`, ); } + if (mappedObjects.length > 0) { + causes.push( + `${mappedObjects.length} object(s) are routed to it by a datasourceMapping rule ` + + `(${formatObjectList(mappedObjects)}) and have no fallback datasource — their reads/writes ` + + `would otherwise land in a DIFFERENT database than the one they declare`, + ); + } if (record.bootCritical === true) { causes.push( `declared boot-critical by the host — it is the platform's primary datasource and ` + diff --git a/skills/objectstack-data/rules/datasources.md b/skills/objectstack-data/rules/datasources.md index eab276f736..fb40dedb40 100644 --- a/skills/objectstack-data/rules/datasources.md +++ b/skills/objectstack-data/rules/datasources.md @@ -51,11 +51,16 @@ objects' read metadata registered **automatically at boot** — no `onEnable` / 1. it is **external** (`schemaMode !== 'managed'`), **or** 2. an object **explicitly** binds via `object.datasource === `, **or** -3. it sets **`autoConnect: true`**. +3. it sets **`autoConnect: true`**, **or** +4. a **`datasourceMapping` rule routes at least one object to it**. -A `managed` datasource that nothing explicitly binds (e.g. only referenced by a -`datasourceMapping` rule) stays **metadata-only** — visible but not connected — so -existing apps are unchanged. Set `autoConnect: true` to force a live connection. +A `managed` datasource that nothing routes to stays **metadata-only** — visible but +not connected. Set `autoConnect: true` to force a live connection. + +⚠️ A `datasourceMapping` rule is **routing, not a hint**. A rule pointing at a +datasource that cannot be connected fails the boot, and a query against a mapped +object throws instead of silently resolving the default store. Do not declare a +mapping you do not mean. > `onEnable` + `ctx.drivers.register(driver)` remains supported only as an escape > hatch for drivers built dynamically at runtime; it is idempotent with auto-connect.