Skip to content

Commit 99ffc04

Browse files
os-zhuangclaude
andauthored
fix(analytics)!: a measure emits what it declares, instead of COUNT(*) (#4157) (#4184)
resolveMeasureSql answered `COUNT(*)` to three questions it could not otherwise answer, each time aliased under the name the caller asked for so the result looked like an answer: 1. A measure the cube does not declare. lookupMember's synthetic relation fallback is dimension-only, so any undeclared or mistyped measure landed here — `measures: ['revenue']` returned `COUNT(*) AS "revenue"`. 2. A number/string/boolean metric. AggregationMetricType documents these as "Custom SQL expression returning a …": the measure's `sql` IS the computation. It was discarded and replaced by a row count. 3. An unrecognised `type`. Same silent substitution. Now the first and third throw — naming the declared measures, and both accepted vocabularies — and a custom-expression type emits its expression unwrapped. The six aggregates are unchanged. Also: a dot no longer implies a relationship hop. qualifyAndRegisterJoin split any dotted string into a join chain, so `SUM(account.amount)` became `"SUM(account"."amount)"` plus a `LEFT JOIN "SUM(account"` — invalid SQL naming a table that does not exist. Harmless only while the result was discarded for COUNT(*). A dotted string is a path only when every segment is a bare identifier, so `account.amount` still lowers to a qualified column and a join. That also fixes the same mangling for an AGGREGATE measure whose sql is an expression. Breaking, narrowly: two inputs that used to produce SQL now raise. Both were returning a wrong number rather than data, so nothing correct can depend on them — the trade #3948 settled for the drivers. Datasets are unaffected; aggregateToMetricType only ever emits an AggregationFunction member, so the reachable path is a hand-authored Cube. metric-type-coverage.test.ts asserts the aggregate and expression sets PARTITION AggregationMetricType, so a tenth type fails a test rather than reaching the throw. Both sets are named rather than derived as each other's complement — deriving would classify a new aggregate as an expression and emit a bare column. 460 tests / 35 files green, including the four suites asserting COUNT(*) (all use a declared `type: 'count'` metric). The 14 new tests were confirmed failing against the old behaviour before the fix. Closes #4157. Refs #4153, #3948, objectstack-ai/objectui#2945 Co-authored-by: Jack Zhuang <277994282+os-zhuang@users.noreply.github.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
1 parent 5b08389 commit 99ffc04

4 files changed

Lines changed: 328 additions & 14 deletions

File tree

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,58 @@
1+
---
2+
"@objectstack/service-analytics": minor
3+
---
4+
5+
fix(analytics)!: a measure emits what it declares, instead of `COUNT(*)` (#4157)
6+
7+
`NativeSQLStrategy.resolveMeasureSql` answered `COUNT(*)` to three different
8+
questions it could not otherwise answer — each time aliased under the name the
9+
caller asked for, so the result looked like an answer:
10+
11+
1. **A measure the cube does not declare.** `lookupMember`'s synthetic
12+
relation fallback is dimension-only, so any undeclared or mistyped measure
13+
name landed here. `measures: ['revenue']` against a cube without it returned
14+
`COUNT(*) AS "revenue"` — a row count presented as revenue.
15+
2. **A `number`/`string`/`boolean` metric.** `AggregationMetricType` documents
16+
these as *"Custom SQL expression returning a number / string / boolean"*: the
17+
measure's `sql` **is** the computation — a ratio, a `CASE`, a window
18+
function. The expression was discarded and replaced by a row count.
19+
3. **An unrecognised `type`.** Same silent substitution.
20+
21+
Now: an undeclared measure and an unrecognised type **throw**, naming the
22+
declared measures and both accepted vocabularies respectively; a custom-
23+
expression type emits its expression unwrapped. The six aggregates are
24+
unchanged.
25+
26+
**A dot no longer implies a relationship hop.** `qualifyAndRegisterJoin` split
27+
any dotted string into a join chain, so the expression `SUM(account.amount)`
28+
became `"SUM(account"."amount)"` *plus* a `LEFT JOIN "SUM(account"` — invalid
29+
SQL naming a table that does not exist. Harmless only while the result was
30+
being thrown away for `COUNT(*)`; emitting the expression makes it matter. A
31+
dotted string is now treated as a path only when every segment is a bare
32+
identifier, so `account.amount` still lowers to a qualified column and a join,
33+
and an expression is emitted as written. That also fixes the same mangling for
34+
an *aggregate* measure whose `sql` is an expression — `type: 'sum'` with
35+
`sql: 'SUM(account.amount)'` was producing the same garbage.
36+
37+
**Breaking, narrowly.** Two inputs that used to produce SQL now raise: a query
38+
naming an undeclared measure, and a cube measure with a type outside
39+
`AggregationMetricType`. Both were returning a wrong number rather than data,
40+
so nothing correct can depend on them — but a caller that was silently getting
41+
row counts will now see an error, which is the point. This is the trade #3948
42+
settled for the drivers.
43+
44+
Datasets are unaffected: `aggregateToMetricType` only ever emits an
45+
`AggregationFunction` member, so a compiled dataset never had a
46+
custom-expression measure or an unknown type. The reachable path is a
47+
hand-authored Cube.
48+
49+
`metric-type-coverage.test.ts` asserts the aggregate and expression sets
50+
*partition* `AggregationMetricType`, so a tenth metric type fails a test rather
51+
than reaching the throw. Both sets are named, not derived as each other's
52+
complement — deriving would classify a new *aggregate* as an expression and emit
53+
a bare column, a different silent wrong answer.
54+
55+
Verified: **460 tests across 35 files** green, including the four suites that
56+
assert `COUNT(*)` — all of them use a *declared* `type: 'count'` metric, so none
57+
relied on a fallback. The 14 new tests were confirmed to fail against the old
58+
behaviour (6 of 10 in the behaviour suite) before the fix.
Lines changed: 153 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,153 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* A measure emits what it declares (#4157).
5+
*
6+
* `resolveMeasureSql` answered `COUNT(*)` to three questions it could not
7+
* otherwise answer, each time aliased under the name the caller asked for:
8+
*
9+
* 1. a measure the cube does not declare — a typo returned a row count;
10+
* 2. a `number`/`string`/`boolean` metric — a custom SQL *expression*, per
11+
* `AggregationMetricType` — whose expression was discarded;
12+
* 3. an unrecognised `type`.
13+
*
14+
* And `qualifyAndRegisterJoin` treated any dot as a relationship hop, so an
15+
* expression like `SUM(account.amount)` was split into `"SUM(account"."amount)"`
16+
* plus a `LEFT JOIN "SUM(account"` — invalid SQL naming a table that does not
17+
* exist. That damage was invisible while the result was thrown away for
18+
* `COUNT(*)`; emitting the expression makes it matter.
19+
*/
20+
import { describe, it, expect } from 'vitest';
21+
import type { Cube } from '@objectstack/spec/data';
22+
import type { AnalyticsQuery } from '@objectstack/spec/contracts';
23+
import { NativeSQLStrategy } from '../strategies/native-sql-strategy.js';
24+
25+
/** A cube whose measures cover both aggregate and custom-expression types. */
26+
const cube: Cube = {
27+
name: 'orders',
28+
title: 'Orders',
29+
sql: 'orders',
30+
measures: {
31+
count: { name: 'count', label: 'Count', type: 'count', sql: '*' },
32+
total: { name: 'total', label: 'Total', type: 'sum', sql: 'amount' },
33+
// The three custom-expression types. `sql` IS the computation.
34+
margin: {
35+
name: 'margin', label: 'Margin', type: 'number',
36+
sql: 'SUM(revenue) / NULLIF(SUM(cost), 0)',
37+
},
38+
top_status: {
39+
name: 'top_status', label: 'Top status', type: 'string',
40+
sql: "MAX(CASE WHEN paid THEN 'paid' ELSE 'open' END)",
41+
},
42+
any_paid: { name: 'any_paid', label: 'Any paid', type: 'boolean', sql: 'MAX(paid)' },
43+
},
44+
dimensions: {
45+
status: { name: 'status', label: 'Status', type: 'string', sql: 'status' },
46+
},
47+
} as never;
48+
49+
const ctx = {
50+
getCube: () => cube,
51+
queryCapabilities: () => ({ nativeSql: true, objectqlAggregate: false, inMemory: false }),
52+
executeRawSql: async () => [],
53+
} as never;
54+
55+
const sqlFor = async (query: AnalyticsQuery) =>
56+
(await new NativeSQLStrategy().generateSql(query, ctx)).sql;
57+
58+
describe('custom-expression measures emit their expression', () => {
59+
it('emits a number expression verbatim, ungrouped', async () => {
60+
const sql = await sqlFor({ cube: 'orders', measures: ['margin'] });
61+
expect(sql).toContain('SUM(revenue) / NULLIF(SUM(cost), 0) AS "margin"');
62+
expect(sql).not.toContain('COUNT(*)');
63+
});
64+
65+
it('emits a number expression verbatim in a grouped query', async () => {
66+
const sql = await sqlFor({ cube: 'orders', measures: ['margin'], dimensions: ['status'] });
67+
expect(sql).toContain('SUM(revenue) / NULLIF(SUM(cost), 0) AS "margin"');
68+
expect(sql).toContain('GROUP BY');
69+
// Measures never join GROUP BY — only dimensions do. The expression must
70+
// therefore be aggregate-shaped, which is the author's contract.
71+
expect(sql.slice(sql.indexOf('GROUP BY'))).not.toContain('NULLIF');
72+
});
73+
74+
it('emits string and boolean expressions verbatim', async () => {
75+
const sql = await sqlFor({ cube: 'orders', measures: ['top_status', 'any_paid'] });
76+
expect(sql).toContain(`MAX(CASE WHEN paid THEN 'paid' ELSE 'open' END) AS "top_status"`);
77+
expect(sql).toContain('MAX(paid) AS "any_paid"');
78+
});
79+
80+
it('still wraps the aggregate types', async () => {
81+
const sql = await sqlFor({ cube: 'orders', measures: ['count', 'total'] });
82+
expect(sql).toContain('COUNT(*) AS "count"');
83+
expect(sql).toContain('SUM(amount) AS "total"');
84+
});
85+
});
86+
87+
describe('an expression containing a dot is not mistaken for a join path', () => {
88+
const dotted: Cube = {
89+
...cube,
90+
joins: { account: { name: 'account', relationship: 'belongsTo', sql: '' } },
91+
measures: {
92+
...cube.measures,
93+
// A dot inside a function call — an expression, not `relation.column`.
94+
acct_total: {
95+
name: 'acct_total', label: 'Account total', type: 'number',
96+
sql: 'SUM(account.amount) / 2',
97+
},
98+
// A genuine relationship path, which MUST still be qualified and joined.
99+
acct_amount: { name: 'acct_amount', label: 'Account amount', type: 'sum', sql: 'account.amount' },
100+
},
101+
} as never;
102+
const dottedCtx = { ...(ctx as object), getCube: () => dotted } as never;
103+
const dottedSql = async (q: AnalyticsQuery) =>
104+
(await new NativeSQLStrategy().generateSql(q, dottedCtx)).sql;
105+
106+
it('emits the expression intact and registers no phantom join', async () => {
107+
const sql = await dottedSql({ cube: 'orders', measures: ['acct_total'] });
108+
expect(sql).toContain('SUM(account.amount) / 2 AS "acct_total"');
109+
expect(sql).not.toContain('"SUM(account"');
110+
expect(sql).not.toContain('LEFT JOIN "SUM(account"');
111+
});
112+
113+
it('still lowers a real relationship path into a qualified column and a join', async () => {
114+
const sql = await dottedSql({ cube: 'orders', measures: ['acct_amount'] });
115+
expect(sql).toContain('SUM("account"."amount") AS "acct_amount"');
116+
expect(sql).toContain('LEFT JOIN "account" ON "orders"."account" = "account"."id"');
117+
});
118+
});
119+
120+
describe('the questions COUNT(*) used to answer now fail loudly', () => {
121+
it('throws for a measure the cube does not declare', async () => {
122+
await expect(sqlFor({ cube: 'orders', measures: ['revenue'] }))
123+
.rejects.toThrow(/declares no measure "revenue"/);
124+
});
125+
126+
it('names the declared measures, so a typo is self-correcting', async () => {
127+
await expect(sqlFor({ cube: 'orders', measures: ['totl'] }))
128+
.rejects.toThrow(/declared: count, total, margin, top_status, any_paid/);
129+
});
130+
131+
it('throws for an unrecognised metric type', async () => {
132+
const bad = {
133+
...cube,
134+
measures: { weird: { name: 'weird', label: 'Weird', type: 'median', sql: 'amount' } },
135+
} as never;
136+
const badCtx = { ...(ctx as object), getCube: () => bad } as never;
137+
await expect(new NativeSQLStrategy().generateSql({ cube: 'orders', measures: ['weird'] }, badCtx))
138+
.rejects.toThrow(/unrecognised type "median"/);
139+
});
140+
141+
it('the unrecognised-type error lists both vocabularies', async () => {
142+
const bad = {
143+
...cube,
144+
measures: { weird: { name: 'weird', label: 'Weird', type: 'median', sql: 'amount' } },
145+
} as never;
146+
const badCtx = { ...(ctx as object), getCube: () => bad } as never;
147+
const err = await new NativeSQLStrategy()
148+
.generateSql({ cube: 'orders', measures: ['weird'] }, badCtx)
149+
.catch((e: Error) => e.message);
150+
expect(err).toContain('count_distinct');
151+
expect(err).toContain('number');
152+
});
153+
});
Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,50 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* Every `AggregationMetricType` a measure can declare is handled, and handled
5+
* as itself (#4157).
6+
*
7+
* `resolveMeasureSql` used to answer `COUNT(*)` to three different questions:
8+
* an undeclared measure, a custom-SQL-expression metric type, and an
9+
* unrecognised type. Each returned a plausible number — aliased under the name
10+
* the caller asked for — for a query that asked for something else.
11+
*
12+
* The two sets below must partition the spec's vocabulary. Deriving one as "the
13+
* complement of the other" would defeat the point: a *new aggregate* the spec
14+
* grows would be classified as an expression and emitted as a bare column,
15+
* which is a different silent wrong answer. Naming both makes a new member fail
16+
* here instead.
17+
*/
18+
import { describe, it, expect } from 'vitest';
19+
import { AggregationMetricType } from '@objectstack/spec/data';
20+
import {
21+
SUPPORTED_AGGREGATE_SQL_KEYS,
22+
EXPRESSION_METRIC_TYPES,
23+
} from './strategies/native-sql-strategy.js';
24+
25+
describe('AggregationMetricType coverage', () => {
26+
it('is partitioned by the aggregate and expression sets', () => {
27+
const handled = [...SUPPORTED_AGGREGATE_SQL_KEYS, ...EXPRESSION_METRIC_TYPES].sort();
28+
expect(handled).toEqual([...AggregationMetricType.options].sort());
29+
});
30+
31+
it('leaves no metric type to the unrecognised-type throw', () => {
32+
const handled = new Set([...SUPPORTED_AGGREGATE_SQL_KEYS, ...EXPRESSION_METRIC_TYPES]);
33+
const unhandled = AggregationMetricType.options.filter((t: string) => !handled.has(t));
34+
expect(
35+
unhandled,
36+
'a declared metric type that reaches the throw is a spec member no query can use',
37+
).toEqual([]);
38+
});
39+
40+
it('classifies nothing as both an aggregate and an expression', () => {
41+
const both = SUPPORTED_AGGREGATE_SQL_KEYS.filter((a) => EXPRESSION_METRIC_TYPES.has(a));
42+
expect(both).toEqual([]);
43+
});
44+
45+
it('records the current split, so a vocabulary change shows up in review', () => {
46+
expect([...SUPPORTED_AGGREGATE_SQL_KEYS].sort())
47+
.toEqual(['avg', 'count', 'count_distinct', 'max', 'min', 'sum']);
48+
expect([...EXPRESSION_METRIC_TYPES].sort()).toEqual(['boolean', 'number', 'string']);
49+
});
50+
});

packages/services/service-analytics/src/strategies/native-sql-strategy.ts

Lines changed: 67 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -22,10 +22,9 @@ import { nextUtcCalendarDay } from '@objectstack/core';
2222
* `default: COUNT(*)`, so an aggregate the spec grew would have returned a row
2323
* count instead of the number the author asked for, silently. objectui#2945.
2424
*
25-
* Non-aggregate metric types (`number`/`string`/`boolean` — custom SQL
26-
* expressions, `AggregationMetricType` in `data/analytics.zod.ts`) are
27-
* deliberately absent, and keep the caller's existing fallback rather than
28-
* changing behaviour here; see the note at {@link NativeSQLStrategy}.
25+
* Non-aggregate metric types (`number`/`string`/`boolean`) are deliberately
26+
* absent — they are handled by {@link EXPRESSION_METRIC_TYPES}, which emits the
27+
* author's expression rather than wrapping it.
2928
*/
3029
const AGGREGATE_SQL: Record<string, (col: string) => string> = {
3130
'count': () => 'COUNT(*)',
@@ -39,21 +38,44 @@ const AGGREGATE_SQL: Record<string, (col: string) => string> = {
3938
/** Exported for the lockstep guard — the aggregates this strategy can lower. */
4039
export const SUPPORTED_AGGREGATE_SQL_KEYS = Object.keys(AGGREGATE_SQL);
4140

41+
/**
42+
* Metric types that are a custom SQL *expression*, not an aggregate to wrap.
43+
*
44+
* `AggregationMetricType` (`data/analytics.zod.ts`) documents these three as
45+
* "Custom SQL expression returning a number / string / boolean" — the measure's
46+
* `sql` IS the whole computation (a ratio, a `CASE`, a window function), so the
47+
* only correct emission is the expression itself. They used to fall through to
48+
* `resolveMeasureSql`'s `COUNT(*)` fallback, which threw the expression away and
49+
* returned a row count. #4157.
50+
*
51+
* Named rather than derived as "everything that is not an aggregate": deriving it
52+
* would silently classify a *new* aggregate the spec grows (`median`, …) as an
53+
* expression and emit a bare column. `metric-type-coverage.test.ts` asserts these
54+
* two sets partition `AggregationMetricType`, so a new member fails a test
55+
* instead of picking a default.
56+
*/
57+
export const EXPRESSION_METRIC_TYPES = new Set(['number', 'string', 'boolean']);
58+
59+
/**
60+
* A dot-separated chain of bare identifiers — `amount`, `account.amount`,
61+
* `account.owner.region`. Distinguishes a relationship PATH, which
62+
* {@link NativeSQLStrategy.qualifyAndRegisterJoin} lowers into joins, from a SQL
63+
* expression that merely contains a dot. #4157.
64+
*/
65+
const IDENTIFIER_PATH = /^[A-Za-z_][A-Za-z0-9_]*(\.[A-Za-z_][A-Za-z0-9_]*)*$/;
66+
4267
/**
4368
* NativeSQLStrategy — Priority 1
4469
*
4570
* Pushes the analytics query down to the database as a native SQL statement.
4671
* This is the most efficient path and is preferred whenever the backing driver
4772
* supports raw SQL execution (e.g. Postgres, MySQL, SQLite).
4873
*
49-
* Known gap, unchanged by the lockstep work and reported separately: a measure
50-
* whose `type` is `number`/`string`/`boolean` — a custom SQL *expression*, not
51-
* an aggregate — also lands on the `COUNT(*)` fallback in
52-
* `resolveMeasureSql`, so its expression is replaced by a row count. Datasets
53-
* cannot produce such a measure (`aggregateToMetricType` only ever returns an
54-
* `AggregationFunction` member), so this is reachable only from a hand-authored
55-
* Cube. Left as-is on purpose: emitting `col` instead is a behavioural change
56-
* in an analytics SQL path, and deserves its own change with its own tests.
74+
* `resolveMeasureSql` used to answer `COUNT(*)` to three different questions it
75+
* could not otherwise answer — an undeclared measure, a custom-SQL-expression
76+
* metric type, and an unrecognised type. All three returned a plausible number
77+
* for a query that asked for something else. They now emit the expression or
78+
* throw; see that method. #4157.
5779
*/
5880
export class NativeSQLStrategy implements AnalyticsStrategy {
5981
readonly name = 'NativeSQLStrategy';
@@ -313,6 +335,13 @@ export class NativeSQLStrategy implements AnalyticsStrategy {
313335
}
314336
return rawSql;
315337
}
338+
// A dot does not by itself mean "relationship path". `SUM(account.amount)`
339+
// is one SQL EXPRESSION that happens to contain a dot, and splitting it as a
340+
// path produced `"SUM(account"."amount)"` plus a phantom
341+
// `LEFT JOIN "SUM(account"` — invalid SQL and a join to a table that does not
342+
// exist. Only qualify when every segment is a bare identifier; otherwise the
343+
// author wrote an expression and it is returned as-is. #4157.
344+
if (!IDENTIFIER_PATH.test(rawSql)) return rawSql;
316345
// Multi-hop (ADR-0071): the dotted path IS the join chain. Every segment but
317346
// the last is a relationship hop; the last is the column. The join ALIAS at
318347
// each hop is the full path PREFIX (`account`, then `account.owner`), which
@@ -406,13 +435,37 @@ export class NativeSQLStrategy implements AnalyticsStrategy {
406435
const measure = this.lookupMember(cube, member, 'measure') as
407436
| { sql: string; type: string }
408437
| undefined;
409-
if (!measure) return `COUNT(*)`;
438+
// `lookupMember`'s synthetic relation fallback is dimension-only, so an
439+
// undeclared measure name lands here — a typo, or a query naming a metric
440+
// this cube does not have. It used to return `COUNT(*)`: the caller asked
441+
// for revenue and got a row count, aliased AS "revenue". #4157.
442+
if (!measure) {
443+
const declared = Object.keys(cube.measures ?? {});
444+
throw new Error(
445+
`[native-sql-strategy] cube "${cube.name}" declares no measure "${member}"` +
446+
(declared.length ? ` (declared: ${declared.join(', ')})` : ' (it declares none)'),
447+
);
448+
}
410449

411450
const col = measure.sql === '*'
412451
? '*'
413452
: this.qualifyAndRegisterJoin(measure.sql, parentTable, joins, cube);
453+
414454
const wrap = AGGREGATE_SQL[measure.type];
415-
return wrap ? wrap(col) : `COUNT(*)`;
455+
if (wrap) return wrap(col);
456+
// A custom SQL expression: the measure's `sql` IS the computation, so emit
457+
// it unwrapped. In a grouped query the expression must itself be
458+
// aggregate-shaped — measures never join `GROUP BY` (only dimensions do), so
459+
// a scalar expression there is invalid SQL. That is the author's contract to
460+
// keep; silently substituting `COUNT(*)` did not keep it for them.
461+
if (EXPRESSION_METRIC_TYPES.has(measure.type)) return col;
462+
463+
throw new Error(
464+
`[native-sql-strategy] measure "${member}" on cube "${cube.name}" has ` +
465+
`unrecognised type "${measure.type}" — expected an aggregate ` +
466+
`(${SUPPORTED_AGGREGATE_SQL_KEYS.join(', ')}) or a custom-expression type ` +
467+
`(${[...EXPRESSION_METRIC_TYPES].join(', ')}).`,
468+
);
416469
}
417470

418471
private resolveFieldSql(

0 commit comments

Comments
 (0)