Skip to content

Commit e9936d5

Browse files
os-zhuangclaude
andcommitted
fix(driver-sql,objectql): honor external.remoteName/remoteSchema in the federation read path (ADR-0015)
The query path ignored an external object's remoteName/remoteSchema and resolved the physical table from the object name, so a federated object bound to a differently-named remote table failed with "no such table". ADR-0015's own canonical example (wh_order -> mart.fact_orders) was therefore broken. - SqlDriver.registerExternalObject(): DDL-free metadata for external objects (physical remote table + read-coercion maps) — the read-path counterpart to initObjects(), which is DDL-gated off for external schemaMode. - getBuilder() resolves the physical remote table (+ .withSchema for pg/mysql; sqlite no-op). Coercion re-keyed via coercionKey() so date/datetime filters still resolve after the table switch. Managed path byte-for-byte unchanged. - engine/plugin route objects with external!=null to registerExternalObject in both boot schema-sync and the on-demand syncObjectSchema path. - IDataDriver.registerExternalObject?() declared optional (non-SQL drivers skip). - Tests: sql-driver-external-remote-name.test.ts + a ddl-gate case. ADR-0015 §18 addendum. Scope: remoteName (all dialects) + remoteSchema (pg/mysql). columnMap, native-analytics SQL over external objects, and auto-connecting declared datasources are tracked follow-ups (ADR-0015 §18). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent 47d978a commit e9936d5

7 files changed

Lines changed: 438 additions & 6 deletions

File tree

docs/adr/0015-external-datasource-federation.md

Lines changed: 84 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -865,3 +865,87 @@ The ADR is considered "delivered" when:
865865
- `packages/plugins/driver-sql/src/sql-driver.ts` (introspectSchema, createTable, alterTable)
866866
- `packages/services/service-ai/src/tools/query-data.tool.ts`
867867
- `packages/services/service-ai/src/schema-retriever.ts`
868+
869+
---
870+
871+
## 18. Addendum (2026-06): Federation read path honours `remoteName` / `remoteSchema`
872+
873+
**Status:** accepted — implements §3.1 / §8 of this ADR that shipped only partially.
874+
875+
### Problem
876+
877+
The spec, introspection (`os datasource introspect`), boot validation (§5.2), and
878+
the write gate (§5.3) all honoured an object's `external.remoteName` /
879+
`external.remoteSchema`. **The query-execution path did not.** Reads resolved the
880+
physical table from the *object name* alone, ignoring the binding — so the
881+
canonical example in §8 (object `wh_order` → `external: { remoteSchema: 'mart',
882+
remoteName: 'fact_orders' }`) could not actually be queried.
883+
884+
Reproduced with the real engine + a real better-sqlite3 driver: an object
885+
`ext_customer` bound to remote table `remote_customers` via `remoteName` failed
886+
with `no such table: ext_customer`; only naming the object `remote_customers`
887+
returned rows. This is a correctness gap, not a feature request — the declared
888+
contract validated green and then queried the wrong (non-existent) table.
889+
890+
### Root cause
891+
892+
- `SqlDriver.getBuilder(object)` did `this.knex(object)` — the object name *was*
893+
the physical table.
894+
- Per-object read-coercion metadata (`json`/`boolean`/`numeric`/`date`/`datetime`)
895+
is populated only inside `initObjects()`, whose first statement is
896+
`assertSchemaMutable()` (the DDL gate, §5.1) → it throws for any
897+
`schemaMode !== 'managed'` driver. The boot schema-sync swallowed that throw
898+
per-object, leaving external objects with **zero** coercion metadata and no
899+
table-name mapping.
900+
901+
### Fix
902+
903+
- **`SqlDriver.registerExternalObject(schema)`** — a DDL-free counterpart to
904+
`initObjects()`. It records the physical remote table
905+
(`physicalTableByObject` / `physicalSchemaByObject`) and populates the same
906+
coercion maps, keyed by **object name** (matching `formatInput`/`formatOutput`/
907+
`coerceFilterValue`), without running any DDL.
908+
- **`getBuilder()`** now resolves `physicalTableByObject[object] ?? object`, and
909+
applies `.withSchema()` when a remote schema is recorded. Managed objects miss
910+
both maps, so the path is unchanged (one `undefined` lookup).
911+
- **Coercion re-keying**`applyFilters`/`applyFilterCondition` map the builder's
912+
physical table back to the object name (`coercionKey`) so date/datetime filter
913+
coercion still resolves after the table switch.
914+
- **Engine/plugin routing**`ObjectQLPlugin.syncRegisteredSchemas` (boot) and
915+
`ObjectQL.syncObjectSchema` (on-demand) route objects with `external != null`
916+
to `driver.registerExternalObject()` instead of the DDL `syncSchema`. The
917+
on-demand path lets an app register a *late* external driver (one added via an
918+
`onEnable` hook) and then make its objects queryable.
919+
- **Driver contract**`IDataDriver.registerExternalObject?()` is declared
920+
optional, so non-SQL drivers degrade gracefully (the engine skips external
921+
objects they can't serve).
922+
923+
### Scope
924+
925+
- `remoteName` is honoured on **all** dialects.
926+
- `remoteSchema` is applied via `knex.withSchema()` on Postgres / MySQL; on
927+
**SQLite it is a no-op** (no schema namespace) and logs a one-time warning.
928+
- External reads use **best-effort coercion**: with no DDL/`columnInfo`, coercion
929+
is driven purely by the declared field types. Keep external object fields to
930+
well-understood scalar types.
931+
932+
### Explicitly out of scope (separate follow-ups)
933+
934+
1. **`external.columnMap`** (remote column name ≠ local field key). The driver's
935+
`select` / `where` / `orderBy` do not currently apply column-name translation,
936+
and `columnMap` is the inverse of per-field `field.columnName` — reconciling
937+
the two into one source of truth is its own change.
938+
2. **Native-analytics SQL over external objects** — the analytics service compiles
939+
its own `FROM "<table>"` outside the driver and needs the same remote-table
940+
awareness.
941+
3. **Auto-connecting declared datasources** as queryable ObjectQL drivers in the
942+
standalone runtime. Today a declared non-default datasource appears in the
943+
metadata registry (Setup → Datasources) but is only made queryable by
944+
registering a live driver under its name (e.g. via an app `onEnable` hook).
945+
946+
### Tests
947+
948+
`packages/plugins/driver-sql/src/sql-driver-external-remote-name.test.ts` (read /
949+
filter / coercion against a differently-named remote table, no DDL leakage) and an
950+
added case in `sql-driver-ddl-gate.test.ts` (`registerExternalObject` is DDL-free).
951+
File-based better-sqlite3 tests require Node ≥ 25 (ABI 141) in this repo.

packages/objectql/src/engine.ts

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2815,7 +2815,18 @@ export class ObjectQL implements IDataEngine {
28152815
const obj = this._registry.getObject(objectName) as any;
28162816
if (!obj) return;
28172817
const driver = this.getDriverForObject(objectName);
2818-
if (!driver || typeof (driver as any).syncSchema !== 'function') return;
2818+
if (!driver) return;
2819+
// Federated (external) object (ADR-0015): register read metadata WITHOUT DDL
2820+
// (its remote schema is owned externally). This is what an app's onEnable
2821+
// calls after registering a late external driver so coercion maps + the
2822+
// physical-table mapping exist for queries. See SqlDriver.registerExternalObject.
2823+
if (obj.external != null) {
2824+
if (typeof (driver as any).registerExternalObject === 'function') {
2825+
await (driver as any).registerExternalObject(obj);
2826+
}
2827+
return;
2828+
}
2829+
if (typeof (driver as any).syncSchema !== 'function') return;
28192830
const tableName = StorageNameMapping.resolveTableName(obj);
28202831
await (driver as any).syncSchema(tableName, obj);
28212832
}

packages/objectql/src/plugin.ts

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -698,6 +698,33 @@ export class ObjectQLPlugin implements Plugin {
698698
continue;
699699
}
700700

701+
// Federated (external) objects (ADR-0015): their schema is owned by the
702+
// remote database, so DDL (syncSchema/initObjects) is forbidden and would
703+
// throw. Register read metadata (physical remote table + coercion maps)
704+
// without DDL so the query path resolves to the remote table, then skip
705+
// the DDL grouping below.
706+
if (obj.external != null) {
707+
if (typeof driver.registerExternalObject === 'function') {
708+
try {
709+
await driver.registerExternalObject(obj);
710+
synced++;
711+
} catch (e: unknown) {
712+
ctx.logger.warn('Failed to register external object metadata', {
713+
object: obj.name,
714+
driver: driver.name,
715+
error: e instanceof Error ? e.message : String(e),
716+
});
717+
}
718+
} else {
719+
ctx.logger.debug('Driver does not support registerExternalObject, skipping external object', {
720+
object: obj.name,
721+
driver: driver.name,
722+
});
723+
skipped++;
724+
}
725+
continue;
726+
}
727+
701728
if (typeof driver.syncSchema !== 'function') {
702729
ctx.logger.debug('Driver does not support syncSchema, skipping', {
703730
object: obj.name,

packages/plugins/driver-sql/src/sql-driver-ddl-gate.test.ts

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,21 @@ describe('SqlDriver DDL gate (ADR-0015)', () => {
6868
);
6969
});
7070

71+
it('registerExternalObject is DDL-free: does not throw on external mode and creates no table', async () => {
72+
driver = makeDriver('external');
73+
expect(() =>
74+
driver.registerExternalObject!({
75+
name: 'ext_widget',
76+
external: { remoteName: 'widgets' },
77+
fields: { sku: { type: 'text' } },
78+
}),
79+
).not.toThrow();
80+
const k = (driver as any).knex;
81+
// No DDL ran — neither the object name nor the remote name was created.
82+
expect(await k.schema.hasTable('ext_widget')).toBe(false);
83+
expect(await k.schema.hasTable('widgets')).toBe(false);
84+
});
85+
7186
it('also blocks DDL in validate-only mode', async () => {
7287
driver = makeDriver('validate-only');
7388
await expect(
Lines changed: 165 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,165 @@
1+
// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* Federation read-path tests (ADR-0015 addendum).
5+
*
6+
* Before this fix the query path resolved an external object to a table named
7+
* after the OBJECT, ignoring `external.remoteName` — so `find('ext_customer')`
8+
* threw `no such table: ext_customer` even though the object bound to a real
9+
* remote table `remote_customers`. ADR-0015's own canonical example
10+
* (`wh_order` → `mart.fact_orders`) was therefore broken.
11+
*
12+
* These tests stand up one sqlite file as the "remote" database (populated with
13+
* a managed driver), then open a second `schemaMode: 'external'` driver over the
14+
* same file and assert that an object whose name differs from the remote table
15+
* is fully queryable — with read coercion (boolean/json/date) working even
16+
* though no DDL ran for the external object.
17+
*/
18+
19+
import { describe, it, expect, afterAll } from 'vitest';
20+
import { rmSync } from 'node:fs';
21+
import { tmpdir } from 'node:os';
22+
import { join } from 'node:path';
23+
import { SqlDriver } from '../src/index.js';
24+
import { ExternalSchemaModeViolationError } from '@objectstack/spec/shared';
25+
26+
const FIELDS = {
27+
name: { type: 'text' },
28+
flag: { type: 'boolean' },
29+
meta: { type: 'json' },
30+
when: { type: 'date' },
31+
amount: { type: 'number' },
32+
seen_at: { type: 'datetime' },
33+
} as const;
34+
35+
let file: string;
36+
37+
afterAll(() => {
38+
if (file) {
39+
try { rmSync(file, { force: true }); } catch { /* ignore */ }
40+
}
41+
});
42+
43+
async function seedRemote(path: string) {
44+
const fixture = new SqlDriver({
45+
client: 'better-sqlite3',
46+
connection: { filename: path },
47+
useNullAsDefault: true,
48+
});
49+
(fixture as any).name = 'fixture';
50+
await fixture.connect?.();
51+
// Physical table name deliberately differs from the object name used later.
52+
await (fixture as any).initObjects([{ name: 'remote_customers', fields: FIELDS }]);
53+
await (fixture as any).create('remote_customers', {
54+
id: 'c1', name: 'Acme', flag: true, meta: { tier: 'gold' },
55+
when: '2026-01-01', amount: 100, seen_at: new Date('2026-01-02T10:00:00.000Z'),
56+
});
57+
await (fixture as any).create('remote_customers', {
58+
id: 'c2', name: 'Globex', flag: false, meta: { tier: 'silver' },
59+
when: '2026-02-15', amount: 250, seen_at: new Date('2026-02-16T08:00:00.000Z'),
60+
});
61+
await fixture.disconnect?.();
62+
}
63+
64+
function externalDriver(path: string): SqlDriver {
65+
const ext = new SqlDriver({
66+
client: 'better-sqlite3',
67+
connection: { filename: path },
68+
useNullAsDefault: true,
69+
schemaMode: 'external',
70+
} as any);
71+
(ext as any).name = 'extds';
72+
return ext;
73+
}
74+
75+
describe('SqlDriver external read path — remoteName resolution (ADR-0015)', () => {
76+
it('queries a remote table whose name differs from the object name, with coercion', async () => {
77+
file = join(tmpdir(), `os-ext-read-${process.pid}-${Date.now()}.db`);
78+
await seedRemote(file);
79+
80+
const ext = externalDriver(file);
81+
await ext.connect?.();
82+
try {
83+
// DDL-free registration — must NOT throw (unlike initObjects/syncSchema).
84+
expect(() =>
85+
ext.registerExternalObject!({ name: 'ext_customer', external: { remoteName: 'remote_customers' }, fields: FIELDS as any }),
86+
).not.toThrow();
87+
88+
// The bug: this used to throw `no such table: ext_customer`.
89+
const rows = await ext.find('ext_customer', {} as any);
90+
expect(rows).toHaveLength(2);
91+
92+
const acme = rows.find((r: any) => r.name === 'Acme');
93+
expect(acme).toBeTruthy();
94+
// Coercion populated despite no DDL having run for the external object:
95+
expect(acme.flag).toBe(true); // boolean (stored 1/0)
96+
expect(acme.meta).toEqual({ tier: 'gold' }); // json (stored as text)
97+
expect(acme.when).toBe('2026-01-01'); // date → YYYY-MM-DD
98+
expect(typeof acme.amount).toBe('number'); // numeric scalar
99+
100+
// count + findOne route to the remote table too.
101+
expect(await ext.count('ext_customer', {} as any)).toBe(2);
102+
const one = await ext.findOne('ext_customer', { where: { name: 'Globex' } } as any);
103+
expect(one?.name).toBe('Globex');
104+
105+
// Filtered reads hit the remote table.
106+
const filtered = await ext.find('ext_customer', { where: { name: 'Acme' } } as any);
107+
expect(filtered).toHaveLength(1);
108+
expect(filtered[0].name).toBe('Acme');
109+
110+
// Date filter — guards the coercion re-keying (§3): coercion maps are keyed
111+
// by the OBJECT name even though the builder now targets the remote table.
112+
const byDate = await ext.find('ext_customer', { where: { when: '2026-02-15' } } as any);
113+
expect(byDate).toHaveLength(1);
114+
expect(byDate[0].name).toBe('Globex');
115+
116+
// Datetime filter — the SQLite epoch-affinity case the §3 trap would break
117+
// if coercion were keyed by the physical (remote) name instead of object.
118+
const byDatetime = await ext.find('ext_customer', { where: { seen_at: '2026-01-02T10:00:00.000Z' } } as any);
119+
expect(byDatetime.map((r: any) => r.name)).toContain('Acme');
120+
121+
// No object-named table was ever created in the remote db (no DDL leaked).
122+
const k = (ext as any).knex;
123+
expect(await k.schema.hasTable('ext_customer')).toBe(false);
124+
expect(await k.schema.hasTable('remote_customers')).toBe(true);
125+
} finally {
126+
await ext.disconnect?.();
127+
}
128+
});
129+
130+
it('still throws on initObjects/syncSchema (DDL) for external objects', async () => {
131+
const ext = externalDriver(file);
132+
await ext.connect?.();
133+
try {
134+
await expect(
135+
ext.initObjects([{ name: 'ext_customer', fields: FIELDS as any }]),
136+
).rejects.toBeInstanceOf(ExternalSchemaModeViolationError);
137+
} finally {
138+
await ext.disconnect?.();
139+
}
140+
});
141+
142+
it('an object whose name equals the remote table also works (no remap)', async () => {
143+
const ext = externalDriver(file);
144+
await ext.connect?.();
145+
try {
146+
ext.registerExternalObject!({ name: 'remote_customers', external: {}, fields: FIELDS as any });
147+
const rows = await ext.find('remote_customers', {} as any);
148+
expect(rows.length).toBe(2);
149+
} finally {
150+
await ext.disconnect?.();
151+
}
152+
});
153+
154+
it('treats remoteSchema as a no-op on sqlite (bare table)', async () => {
155+
const ext = externalDriver(file);
156+
await ext.connect?.();
157+
try {
158+
ext.registerExternalObject!({ name: 'ext_cust2', external: { remoteName: 'remote_customers', remoteSchema: 'mart' }, fields: FIELDS as any });
159+
const rows = await ext.find('ext_cust2', {} as any);
160+
expect(rows.length).toBe(2);
161+
} finally {
162+
await ext.disconnect?.();
163+
}
164+
});
165+
});

0 commit comments

Comments
 (0)