Skip to content

Commit 1f0e7cb

Browse files
claude[bot]claude
andauthored
fix(service-analytics): 编译期拒绝 dataset 的跨 datasource JOIN (#5115) (#5287)
#5033 把 dataset 的原始 SQL 路由到基对象自己的 datasource 之后,join 目标 落在另一个库上的 dataset 会在查询期响亮失败 —— 正确,但太晚:这类 dataset 仍然可以被保存、发布、挂上看板,失败最终发生在别的环境、别的时间、别人面前。 它是一个纯元数据错误:整个 dataset 被降解为基对象 datasource 上的一条语句, 绑在别处的 join 目标根本不在那里,这件事在编译的那一刻就完全可判定。 现在由 `compileDataset` 判定。`AnalyticsService.registerDataset` —— 每个 dataset(启动预注册、保存、Studio 草稿预览)都要走的那一道门 —— 把 `AnalyticsServiceConfig` 上早已存在的 datasource / federation 探针交给编译器, 被证明的冲突在任何 SQL 被构造之前就被拒绝。错误文案点名两个对象、两个 datasource、出问题的 `include` 路径,以及两条出路(把两个对象绑到同一个 datasource,或者删掉这条关系),与 #5033 的查询期文案同源,读起来不会像两个 bug。 判据刻意收窄:只在元数据自己能证明冲突时才拒绝 —— 基对象与某个 join 目标 各自显式声明了 `object.datasource` 且两个名字不同。以下情形一律按仓里既有的 "cannot answer, do not block" 分级放行: - 宿主根本没有接 datasource 探针(无 data engine)—— 编译行为与改动前完全一致; - 任意一侧停留在默认值。`'default'` 是 schema 的默认**值**,不是路由决定: `ObjectQL.getDriver` 只在显式的非 `'default'` 名字上短路,随后落到 `datasourceMapping` 规则、ADR-0057 §3.6 生命周期分流(audit/telemetry/event) 和所属 package 的 `defaultDatasource` —— 这些编译器都看不见。把 `'default'` 当成「主库」会误拒那些实际被 mapping 规则落到同一个库的 dataset, 并且会让判定取决于该对象是否恰好被 Zod parse 过(那才会把默认值物化出来); - 任意一侧是 federated(external)对象。`NativeSQLStrategy` 本就会 decline 这样的 cube(ADR-0062 D6),查询由 ObjectQL FK-expand 路径承接, 那条路径天然跨库 —— 在这里拒绝会打断今天能工作的路径。 证明不了的部分继续由 #5033 的查询期防线响亮兜底。让跨库看板真正能工作 (在 `NativeSQLStrategy` decline + 两段读内存 join)是另一件事,不在本次改动内。 Claude-Session: https://claude.ai/code/session_01Pbu27iNUfQCHeuS551Rqo7 Co-authored-by: Claude <noreply@anthropic.com>
1 parent 674ac99 commit 1f0e7cb

6 files changed

Lines changed: 507 additions & 9 deletions

File tree

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,52 @@
1+
---
2+
"@objectstack/service-analytics": minor
3+
---
4+
5+
fix(service-analytics): reject a dataset's cross-datasource JOIN when it is compiled, not when it is queried (#5115)
6+
7+
#5033 routed a dataset's raw SQL to its base object's own datasource, which
8+
turned a JOIN whose target lives in another database into a **loud query-time
9+
failure** — correct, but late: the dataset can still be saved, published and
10+
put on a dashboard, and the failure lands in front of whoever opens that
11+
dashboard, usually in another environment on another day. It is a pure metadata
12+
error, decidable the moment the dataset is compiled: the whole dataset is
13+
lowered into ONE statement on the base object's datasource, so a join target
14+
bound elsewhere is simply not there.
15+
16+
`compileDataset` now decides it. `AnalyticsService.registerDataset` — the single
17+
door every dataset passes through, whether pre-registered at boot, saved, or
18+
previewed as a Studio draft — hands the compiler the datasource and federation
19+
probes that already existed on `AnalyticsServiceConfig`, and a proven conflict
20+
is rejected before any SQL is built. The message names both objects, both
21+
datasources, the offending `include` path, and the two ways out (bind both
22+
objects to the same datasource, or drop the relationship), in the same wording
23+
family as the #5033 query-time diagnostic so the two never read as two bugs.
24+
25+
**Who is affected.** This is a tightening: a dataset that used to compile and
26+
then fail (or, before #5033, silently read the wrong database) now fails at
27+
registration. It fires only where the metadata *proves* the conflict — the base
28+
object and a join target each declare an explicit `object.datasource` and the
29+
two names differ. A dataset registered at boot is skipped with a WARN naming the
30+
conflict, as before; the rest of the host's datasets still register.
31+
32+
**What is deliberately not rejected** ("cannot answer, do not block", the same
33+
tiering as `isRegisteredObject` / `getObjectFieldNames`):
34+
35+
- a host that wires no datasource probe at all (no data engine) — compiles
36+
exactly as it did before;
37+
- either side leaving `datasource` at its default. `'default'` is the schema's
38+
default *value*, not a routing decision: `ObjectQL.getDriver` short-circuits
39+
only on an explicit non-`'default'` name, then falls through to
40+
`datasourceMapping` rules, the ADR-0057 §3.6 lifecycle split
41+
(audit/telemetry/event) and the owning package's `defaultDatasource` — none of
42+
which are visible to the compiler. Treating `'default'` as "the primary DB"
43+
would reject datasets whose objects a mapping rule in fact lands on the *same*
44+
database;
45+
- a federated (external) participant on either side. `NativeSQLStrategy` already
46+
declines such a cube (ADR-0062 D6), so the query is served by the ObjectQL
47+
FK-expand path, which crosses datasources by construction.
48+
49+
Everything not proven here keeps failing loudly at query time via #5033.
50+
Making cross-datasource dashboards actually *work* (declining in
51+
`NativeSQLStrategy` and serving the join with two reads) is separate and not
52+
part of this change.

‎packages/services/service-analytics/src/__tests__/dataset-compiler.test.ts‎

Lines changed: 196 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -217,3 +217,199 @@ describe('compileDataset — multi-hop joins (ADR-0071)', () => {
217217
expect(cube.joins?.['account__owner']?.name).toBe('owner');
218218
});
219219
});
220+
221+
222+
/**
223+
* #5115 — a dataset whose JOIN crosses datasources is metadata that can never
224+
* execute: the whole dataset is lowered into ONE statement on the base object's
225+
* datasource (raw SQL routes by object since #5033), so the joined table is
226+
* simply not there. #5033 made that failure loud at QUERY time, in front of
227+
* whoever opened the dashboard; this suite pins the same verdict at COMPILE
228+
* time, while the author (often an AI author) is still holding the metadata.
229+
*
230+
* The gate is deliberately narrow, and these cases pin the boundary as much as
231+
* the rejection: it fires only on a conflict the METADATA PROVES — two explicit,
232+
* different `object.datasource` bindings — because `'default'` is the schema's
233+
* default value rather than a routing decision (mapping rules / the ADR-0057
234+
* lifecycle split / a package's `defaultDatasource` all route objects that
235+
* never say a word about `datasource`). Everything it cannot prove stays
236+
* compilable and is caught by #5033's query-time defence.
237+
*/
238+
describe('compileDataset — cross-datasource join gate (#5115)', () => {
239+
/** opportunity → crm_account → core_user (all to-one). */
240+
const chainResolver = (obj: string, rel: string) => {
241+
const graph: Record<string, Record<string, { object: string; table: string }>> = {
242+
opportunity: { account: { object: 'crm_account', table: 'crm_account' } },
243+
crm_account: { owner: { object: 'core_user', table: 'core_user' } },
244+
};
245+
return graph[obj]?.[rel];
246+
};
247+
248+
const datasetWith = (include: string[], dimensions: Record<string, string>[]) =>
249+
DatasetSchema.parse({
250+
name: 'revenue_by_region',
251+
label: 'Revenue by region',
252+
object: 'opportunity',
253+
include,
254+
dimensions: dimensions.map((d) => ({ ...d, type: 'string' })),
255+
measures: [{ name: 'revenue', label: 'Revenue', aggregate: 'sum', field: 'amount' }],
256+
});
257+
258+
const crossDs = datasetWith(['account'], [{ name: 'region', field: 'account.region' }]);
259+
260+
/** Compile `crossDs` with a datasource map; returns the thrown error, if any. */
261+
const compileWith = (
262+
datasources: Record<string, string | undefined>,
263+
isExternalObject?: (o: string) => boolean,
264+
): Error | undefined => {
265+
try {
266+
compileDataset(crossDs, chainResolver, {
267+
getObjectDatasource: (o) => datasources[o],
268+
...(isExternalObject ? { isExternalObject } : {}),
269+
});
270+
return undefined;
271+
} catch (e) {
272+
return e as Error;
273+
}
274+
};
275+
276+
it('rejects two explicitly-bound objects on different datasources, naming BOTH sides and the fix', () => {
277+
const err = compileWith({ opportunity: 'billing_db', crm_account: 'crm_db' });
278+
279+
expect(err).toBeDefined();
280+
const msg = String(err?.message);
281+
// Both objects …
282+
expect(msg).toContain('base object "opportunity"');
283+
expect(msg).toContain('joined object "crm_account"');
284+
// … both datasources …
285+
expect(msg).toContain('datasource "billing_db"');
286+
expect(msg).toContain('datasource "crm_db"');
287+
// … the offending include path …
288+
expect(msg).toContain('"account"');
289+
// … the rule, and a remedy the author can act on — same wording family as
290+
// the #5033 query-time diagnostic, so the two never read as two bugs.
291+
expect(msg).toMatch(/JOIN cannot cross datasources/);
292+
expect(msg).toMatch(/binding both objects to the same datasource/);
293+
expect(msg).toMatch(/dropping "account" from the dataset's `include`/);
294+
});
295+
296+
it('compiles normally when both sides declare the SAME datasource', () => {
297+
expect(compileWith({ opportunity: 'crm_db', crm_account: 'crm_db' })).toBeUndefined();
298+
// …and the join is still emitted, unchanged.
299+
const { cube } = compileDataset(crossDs, chainResolver, { getObjectDatasource: () => 'crm_db' });
300+
expect(cube.joins?.account?.name).toBe('crm_account');
301+
});
302+
303+
it('compares datasource ids case-insensitively (casing is not two databases)', () => {
304+
expect(compileWith({ opportunity: 'CRM_DB', crm_account: 'crm_db' })).toBeUndefined();
305+
});
306+
307+
// ── "cannot answer, do not block" — the tiering, pinned case by case ───────
308+
309+
it('a host with NO datasource probe compiles exactly as before', () => {
310+
// The headline tiering case: an embedding with no data engine passes no
311+
// options at all, and every dataset it registers must still compile.
312+
expect(() => compileDataset(crossDs, chainResolver)).not.toThrow();
313+
expect(() => compileDataset(crossDs, chainResolver, {})).not.toThrow();
314+
const { cube } = compileDataset(crossDs, chainResolver, {});
315+
expect(cube.joins?.account?.name).toBe('crm_account');
316+
});
317+
318+
it('does not block when the probe cannot place the JOIN TARGET', () => {
319+
expect(compileWith({ opportunity: 'billing_db', crm_account: undefined })).toBeUndefined();
320+
});
321+
322+
it('does not block when the probe cannot place the BASE object', () => {
323+
// "Cannot answer for the base" must not reject every join — the base is the
324+
// side every comparison is made against.
325+
expect(compileWith({ opportunity: undefined, crm_account: 'crm_db' })).toBeUndefined();
326+
});
327+
328+
it('treats the DEFAULT binding as unanswered on either side', () => {
329+
// `datasource: 'default'` is the schema's default VALUE, not a routing
330+
// decision: `ObjectQL.getDriver` only short-circuits on a name OTHER than
331+
// 'default', then falls through to datasourceMapping rules, the ADR-0057
332+
// lifecycle split and the package's defaultDatasource. An object that says
333+
// 'default' may well be routed elsewhere — and may land on exactly the
334+
// datasource the other side declares, which is why rejecting here would
335+
// blank a working dashboard.
336+
expect(compileWith({ opportunity: 'default', crm_account: 'crm_db' })).toBeUndefined();
337+
expect(compileWith({ opportunity: 'billing_db', crm_account: 'default' })).toBeUndefined();
338+
expect(compileWith({ opportunity: 'default', crm_account: 'DEFAULT' })).toBeUndefined();
339+
});
340+
341+
it('exempts a FEDERATED join target (ADR-0062 D6 — served by the FK-expand path)', () => {
342+
// NativeSQLStrategy already declines a cube whose base or joined object is
343+
// external, so such a query runs on the ObjectQL FK-expand path (two reads
344+
// joined in memory), which crosses datasources by construction. Rejecting
345+
// it here would break a path that works today.
346+
expect(
347+
compileWith({ opportunity: 'billing_db', crm_account: 'sf_prod' }, (o) => o === 'crm_account'),
348+
).toBeUndefined();
349+
});
350+
351+
it('exempts a FEDERATED base object', () => {
352+
expect(
353+
compileWith({ opportunity: 'sf_prod', crm_account: 'crm_db' }, (o) => o === 'opportunity'),
354+
).toBeUndefined();
355+
});
356+
357+
// ── which hop gets named ──────────────────────────────────────────────────
358+
359+
it('names the ONE offending hop when a multi-hop path crosses on its second hop', () => {
360+
const twoHop = datasetWith(
361+
['account', 'account.owner'],
362+
[{ name: 'owner_region', field: 'account.owner.region' }],
363+
);
364+
const datasources: Record<string, string> = {
365+
opportunity: 'billing_db',
366+
crm_account: 'billing_db', // first hop stays home …
367+
core_user: 'identity_db', // … the second one leaves
368+
};
369+
let thrown: Error | undefined;
370+
try {
371+
compileDataset(twoHop, chainResolver, { getObjectDatasource: (o) => datasources[o] });
372+
} catch (e) {
373+
thrown = e as Error;
374+
}
375+
const msg = String(thrown?.message);
376+
expect(msg).toContain('joined object "core_user"');
377+
expect(msg).toContain('datasource "identity_db"');
378+
expect(msg).toContain('path "account.owner"');
379+
// The innocent intermediate hop is not blamed.
380+
expect(msg).not.toContain('joined object "crm_account"');
381+
});
382+
383+
it('rejects only the crossing target when several joins are declared', () => {
384+
const multi = DatasetSchema.parse({
385+
name: 'multi',
386+
label: 'Multi',
387+
object: 'opportunity',
388+
include: ['owner', 'account'],
389+
dimensions: [
390+
{ name: 'owner_name', field: 'owner.name', type: 'string' },
391+
{ name: 'region', field: 'account.region', type: 'string' },
392+
],
393+
measures: [{ name: 'cnt', label: 'Count', aggregate: 'count' }],
394+
});
395+
const flatResolver = (obj: string, rel: string) => {
396+
if (obj !== 'opportunity') return undefined;
397+
if (rel === 'owner') return { object: 'core_user', table: 'core_user' };
398+
if (rel === 'account') return { object: 'crm_account', table: 'crm_account' };
399+
return undefined;
400+
};
401+
const datasources: Record<string, string> = {
402+
opportunity: 'billing_db',
403+
core_user: 'billing_db', // same datasource — fine
404+
crm_account: 'crm_db', // the offender
405+
};
406+
let thrown: Error | undefined;
407+
try {
408+
compileDataset(multi, flatResolver, { getObjectDatasource: (o) => datasources[o] });
409+
} catch (e) {
410+
thrown = e as Error;
411+
}
412+
expect(String(thrown?.message)).toContain('joined object "crm_account"');
413+
expect(String(thrown?.message)).not.toContain('core_user');
414+
});
415+
});
Lines changed: 121 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,121 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* #5115 — the WIRING half of the compile-time cross-datasource gate.
5+
*
6+
* `dataset-compiler.test.ts` pins the verdict; this file pins that the service
7+
* actually asks the question. `registerDataset` is the one door every dataset
8+
* goes through (pre-registered datasets at construction, `queryDataset` for
9+
* saved and Studio-draft datasets alike), so hooking the probes up there is
10+
* what moves the failure off the dashboard and onto the author. A compiler that
11+
* can reject but is never handed the probes would pass every unit test above
12+
* and change nothing in production.
13+
*/
14+
15+
import { describe, it, expect, vi } from 'vitest';
16+
import { DatasetSchema } from '@objectstack/spec/ui';
17+
import { AnalyticsService, type AnalyticsServiceConfig } from '../analytics-service.js';
18+
19+
/** opportunity (billing_db) --account--> crm_account (crm_db). */
20+
const DATASOURCES: Record<string, string> = {
21+
opportunity: 'billing_db',
22+
crm_account: 'crm_db',
23+
};
24+
25+
const relationshipResolver = (obj: string, rel: string) =>
26+
obj === 'opportunity' && rel === 'account'
27+
? { object: 'crm_account', table: 'crm_account' }
28+
: undefined;
29+
30+
const crossDsDataset = DatasetSchema.parse({
31+
name: 'revenue_by_region',
32+
label: 'Revenue by region',
33+
object: 'opportunity',
34+
include: ['account'],
35+
dimensions: [{ name: 'region', label: 'Region', field: 'account.region', type: 'string' }],
36+
measures: [{ name: 'revenue', label: 'Revenue', aggregate: 'sum', field: 'amount' }],
37+
});
38+
39+
/** Silent by default — one case swaps in a spy to read the boot-time WARN. */
40+
const silentLogger = { info() {}, warn() {}, error() {}, debug() {} } as AnalyticsServiceConfig['logger'];
41+
42+
const serviceWith = (config: AnalyticsServiceConfig = {}) =>
43+
new AnalyticsService({
44+
relationshipResolver,
45+
getObjectDatasource: (o: string) => DATASOURCES[o],
46+
logger: silentLogger,
47+
...config,
48+
});
49+
50+
describe('registerDataset rejects a cross-datasource join at compile time (#5115)', () => {
51+
it('throws when registering the dataset — no query needed', () => {
52+
expect(() => serviceWith().registerDataset(crossDsDataset)).toThrowError(
53+
/JOIN cannot cross datasources/,
54+
);
55+
});
56+
57+
it('names both objects and both datasources, so the author can act', () => {
58+
const err = (() => {
59+
try {
60+
serviceWith().registerDataset(crossDsDataset);
61+
return undefined;
62+
} catch (e) {
63+
return e as Error;
64+
}
65+
})();
66+
expect(err?.message).toContain('base object "opportunity"');
67+
expect(err?.message).toContain('datasource "billing_db"');
68+
expect(err?.message).toContain('joined object "crm_account"');
69+
expect(err?.message).toContain('datasource "crm_db"');
70+
});
71+
72+
it('fails the QUERY too — before any SQL is built or any driver is touched', async () => {
73+
// The point of the gate: the widget no longer gets as far as a statement,
74+
// so nothing depends on the driver's error text (that was #5033's job).
75+
const executeRawSql = vi.fn();
76+
const executeAggregate = vi.fn();
77+
const service = serviceWith({ executeRawSql, executeAggregate });
78+
79+
await expect(
80+
service.queryDataset(crossDsDataset, { dimensions: ['region'], measures: ['revenue'] }),
81+
).rejects.toThrow(/JOIN cannot cross datasources/);
82+
83+
expect(executeRawSql).not.toHaveBeenCalled();
84+
expect(executeAggregate).not.toHaveBeenCalled();
85+
});
86+
87+
it('does not take the kernel down at boot — a bad pre-registered dataset warns and is skipped', () => {
88+
// Pre-registered datasets are compiled in the constructor, which already
89+
// catches and warns per dataset. A metadata error must stay a metadata
90+
// error: the OTHER datasets in the same host still register.
91+
const warn = vi.fn();
92+
const sane = DatasetSchema.parse({
93+
name: 'pipeline',
94+
label: 'Pipeline',
95+
object: 'opportunity',
96+
dimensions: [{ name: 'stage', label: 'Stage', field: 'stage', type: 'string' }],
97+
measures: [{ name: 'cnt', label: 'Count', aggregate: 'count' }],
98+
});
99+
const service = serviceWith({
100+
datasets: [crossDsDataset, sane],
101+
logger: { info() {}, warn, error() {}, debug() {} } as AnalyticsServiceConfig['logger'],
102+
});
103+
104+
expect(warn.mock.calls.map(String).join('\n')).toMatch(/JOIN cannot cross datasources/);
105+
expect(service.cubeRegistry.get('pipeline')).toBeDefined();
106+
expect(service.cubeRegistry.get('revenue_by_region')).toBeUndefined();
107+
});
108+
109+
it('a host that wires NO datasource probe registers the same dataset unchanged', () => {
110+
// The tiering, at the seam that matters: every embedding without a data
111+
// engine (and every existing test double) must be unaffected by #5115.
112+
const service = new AnalyticsService({ relationshipResolver });
113+
const compiled = service.registerDataset(crossDsDataset);
114+
expect(compiled.cube.joins?.account?.name).toBe('crm_account');
115+
});
116+
117+
it('a FEDERATED join target still registers (ADR-0062 D6 — FK-expand serves it)', () => {
118+
const service = serviceWith({ isExternalObject: (o) => o === 'crm_account' });
119+
expect(() => service.registerDataset(crossDsDataset)).not.toThrow();
120+
});
121+
});

0 commit comments

Comments
 (0)