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/.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/.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/.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/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/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 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/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/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'); + }); +}); 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/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/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 []; 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.