From 767bb4500cd6c5868c5ea945f432339add755615 Mon Sep 17 00:00:00 2001 From: Jack Zhuang <277994282+os-zhuang@users.noreply.github.com> Date: Thu, 30 Jul 2026 22:22:59 +0800 Subject: [PATCH] fix(analytics)!: a measure emits what it declares, instead of COUNT(*) (#4157) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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: Claude Opus 5 --- .changeset/measure-emits-what-it-declares.md | 58 +++++++ .../__tests__/measure-expression-sql.test.ts | 153 ++++++++++++++++++ .../src/metric-type-coverage.test.ts | 50 ++++++ .../src/strategies/native-sql-strategy.ts | 81 ++++++++-- 4 files changed, 328 insertions(+), 14 deletions(-) create mode 100644 .changeset/measure-emits-what-it-declares.md create mode 100644 packages/services/service-analytics/src/__tests__/measure-expression-sql.test.ts create mode 100644 packages/services/service-analytics/src/metric-type-coverage.test.ts diff --git a/.changeset/measure-emits-what-it-declares.md b/.changeset/measure-emits-what-it-declares.md new file mode 100644 index 0000000000..12b4cb3f7b --- /dev/null +++ b/.changeset/measure-emits-what-it-declares.md @@ -0,0 +1,58 @@ +--- +"@objectstack/service-analytics": minor +--- + +fix(analytics)!: a measure emits what it declares, instead of `COUNT(*)` (#4157) + +`NativeSQLStrategy.resolveMeasureSql` answered `COUNT(*)` to three different +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 + name landed here. `measures: ['revenue']` against a cube without it returned + `COUNT(*) AS "revenue"` — a row count presented as revenue. +2. **A `number`/`string`/`boolean` metric.** `AggregationMetricType` documents + these as *"Custom SQL expression returning a number / string / boolean"*: the + measure's `sql` **is** the computation — a ratio, a `CASE`, a window + function. The expression was discarded and replaced by a row count. +3. **An unrecognised `type`.** Same silent substitution. + +Now: an undeclared measure and an unrecognised type **throw**, naming the +declared measures and both accepted vocabularies respectively; a custom- +expression type emits its expression unwrapped. The six aggregates are +unchanged. + +**A dot no longer implies a relationship hop.** `qualifyAndRegisterJoin` split +any dotted string into a join chain, so the expression `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 +being thrown away for `COUNT(*)`; emitting the expression makes it matter. A +dotted string is now treated as a path only when every segment is a bare +identifier, so `account.amount` still lowers to a qualified column and a join, +and an expression is emitted as written. That also fixes the same mangling for +an *aggregate* measure whose `sql` is an expression — `type: 'sum'` with +`sql: 'SUM(account.amount)'` was producing the same garbage. + +**Breaking, narrowly.** Two inputs that used to produce SQL now raise: a query +naming an undeclared measure, and a cube measure with a type outside +`AggregationMetricType`. Both were returning a wrong number rather than data, +so nothing correct can depend on them — but a caller that was silently getting +row counts will now see an error, which is the point. This is the trade #3948 +settled for the drivers. + +Datasets are unaffected: `aggregateToMetricType` only ever emits an +`AggregationFunction` member, so a compiled dataset never had a +custom-expression measure or an unknown type. The reachable path is a +hand-authored Cube. + +`metric-type-coverage.test.ts` asserts the aggregate and expression sets +*partition* `AggregationMetricType`, so a tenth metric type fails a test rather +than reaching the throw. Both sets are named, not derived as each other's +complement — deriving would classify a new *aggregate* as an expression and emit +a bare column, a different silent wrong answer. + +Verified: **460 tests across 35 files** green, including the four suites that +assert `COUNT(*)` — all of them use a *declared* `type: 'count'` metric, so none +relied on a fallback. The 14 new tests were confirmed to fail against the old +behaviour (6 of 10 in the behaviour suite) before the fix. diff --git a/packages/services/service-analytics/src/__tests__/measure-expression-sql.test.ts b/packages/services/service-analytics/src/__tests__/measure-expression-sql.test.ts new file mode 100644 index 0000000000..59a8133f6d --- /dev/null +++ b/packages/services/service-analytics/src/__tests__/measure-expression-sql.test.ts @@ -0,0 +1,153 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * A measure emits what it declares (#4157). + * + * `resolveMeasureSql` answered `COUNT(*)` to three questions it could not + * otherwise answer, each time aliased under the name the caller asked for: + * + * 1. a measure the cube does not declare — a typo returned a row count; + * 2. a `number`/`string`/`boolean` metric — a custom SQL *expression*, per + * `AggregationMetricType` — whose expression was discarded; + * 3. an unrecognised `type`. + * + * And `qualifyAndRegisterJoin` treated any dot as a relationship hop, so an + * expression like `SUM(account.amount)` was split into `"SUM(account"."amount)"` + * plus a `LEFT JOIN "SUM(account"` — invalid SQL naming a table that does not + * exist. That damage was invisible while the result was thrown away for + * `COUNT(*)`; emitting the expression makes it matter. + */ +import { describe, it, expect } from 'vitest'; +import type { Cube } from '@objectstack/spec/data'; +import type { AnalyticsQuery } from '@objectstack/spec/contracts'; +import { NativeSQLStrategy } from '../strategies/native-sql-strategy.js'; + +/** A cube whose measures cover both aggregate and custom-expression types. */ +const cube: Cube = { + name: 'orders', + title: 'Orders', + sql: 'orders', + measures: { + count: { name: 'count', label: 'Count', type: 'count', sql: '*' }, + total: { name: 'total', label: 'Total', type: 'sum', sql: 'amount' }, + // The three custom-expression types. `sql` IS the computation. + margin: { + name: 'margin', label: 'Margin', type: 'number', + sql: 'SUM(revenue) / NULLIF(SUM(cost), 0)', + }, + top_status: { + name: 'top_status', label: 'Top status', type: 'string', + sql: "MAX(CASE WHEN paid THEN 'paid' ELSE 'open' END)", + }, + any_paid: { name: 'any_paid', label: 'Any paid', type: 'boolean', sql: 'MAX(paid)' }, + }, + dimensions: { + status: { name: 'status', label: 'Status', type: 'string', sql: 'status' }, + }, +} as never; + +const ctx = { + getCube: () => cube, + queryCapabilities: () => ({ nativeSql: true, objectqlAggregate: false, inMemory: false }), + executeRawSql: async () => [], +} as never; + +const sqlFor = async (query: AnalyticsQuery) => + (await new NativeSQLStrategy().generateSql(query, ctx)).sql; + +describe('custom-expression measures emit their expression', () => { + it('emits a number expression verbatim, ungrouped', async () => { + const sql = await sqlFor({ cube: 'orders', measures: ['margin'] }); + expect(sql).toContain('SUM(revenue) / NULLIF(SUM(cost), 0) AS "margin"'); + expect(sql).not.toContain('COUNT(*)'); + }); + + it('emits a number expression verbatim in a grouped query', async () => { + const sql = await sqlFor({ cube: 'orders', measures: ['margin'], dimensions: ['status'] }); + expect(sql).toContain('SUM(revenue) / NULLIF(SUM(cost), 0) AS "margin"'); + expect(sql).toContain('GROUP BY'); + // Measures never join GROUP BY — only dimensions do. The expression must + // therefore be aggregate-shaped, which is the author's contract. + expect(sql.slice(sql.indexOf('GROUP BY'))).not.toContain('NULLIF'); + }); + + it('emits string and boolean expressions verbatim', async () => { + const sql = await sqlFor({ cube: 'orders', measures: ['top_status', 'any_paid'] }); + expect(sql).toContain(`MAX(CASE WHEN paid THEN 'paid' ELSE 'open' END) AS "top_status"`); + expect(sql).toContain('MAX(paid) AS "any_paid"'); + }); + + it('still wraps the aggregate types', async () => { + const sql = await sqlFor({ cube: 'orders', measures: ['count', 'total'] }); + expect(sql).toContain('COUNT(*) AS "count"'); + expect(sql).toContain('SUM(amount) AS "total"'); + }); +}); + +describe('an expression containing a dot is not mistaken for a join path', () => { + const dotted: Cube = { + ...cube, + joins: { account: { name: 'account', relationship: 'belongsTo', sql: '' } }, + measures: { + ...cube.measures, + // A dot inside a function call — an expression, not `relation.column`. + acct_total: { + name: 'acct_total', label: 'Account total', type: 'number', + sql: 'SUM(account.amount) / 2', + }, + // A genuine relationship path, which MUST still be qualified and joined. + acct_amount: { name: 'acct_amount', label: 'Account amount', type: 'sum', sql: 'account.amount' }, + }, + } as never; + const dottedCtx = { ...(ctx as object), getCube: () => dotted } as never; + const dottedSql = async (q: AnalyticsQuery) => + (await new NativeSQLStrategy().generateSql(q, dottedCtx)).sql; + + it('emits the expression intact and registers no phantom join', async () => { + const sql = await dottedSql({ cube: 'orders', measures: ['acct_total'] }); + expect(sql).toContain('SUM(account.amount) / 2 AS "acct_total"'); + expect(sql).not.toContain('"SUM(account"'); + expect(sql).not.toContain('LEFT JOIN "SUM(account"'); + }); + + it('still lowers a real relationship path into a qualified column and a join', async () => { + const sql = await dottedSql({ cube: 'orders', measures: ['acct_amount'] }); + expect(sql).toContain('SUM("account"."amount") AS "acct_amount"'); + expect(sql).toContain('LEFT JOIN "account" ON "orders"."account" = "account"."id"'); + }); +}); + +describe('the questions COUNT(*) used to answer now fail loudly', () => { + it('throws for a measure the cube does not declare', async () => { + await expect(sqlFor({ cube: 'orders', measures: ['revenue'] })) + .rejects.toThrow(/declares no measure "revenue"/); + }); + + it('names the declared measures, so a typo is self-correcting', async () => { + await expect(sqlFor({ cube: 'orders', measures: ['totl'] })) + .rejects.toThrow(/declared: count, total, margin, top_status, any_paid/); + }); + + it('throws for an unrecognised metric type', async () => { + const bad = { + ...cube, + measures: { weird: { name: 'weird', label: 'Weird', type: 'median', sql: 'amount' } }, + } as never; + const badCtx = { ...(ctx as object), getCube: () => bad } as never; + await expect(new NativeSQLStrategy().generateSql({ cube: 'orders', measures: ['weird'] }, badCtx)) + .rejects.toThrow(/unrecognised type "median"/); + }); + + it('the unrecognised-type error lists both vocabularies', async () => { + const bad = { + ...cube, + measures: { weird: { name: 'weird', label: 'Weird', type: 'median', sql: 'amount' } }, + } as never; + const badCtx = { ...(ctx as object), getCube: () => bad } as never; + const err = await new NativeSQLStrategy() + .generateSql({ cube: 'orders', measures: ['weird'] }, badCtx) + .catch((e: Error) => e.message); + expect(err).toContain('count_distinct'); + expect(err).toContain('number'); + }); +}); diff --git a/packages/services/service-analytics/src/metric-type-coverage.test.ts b/packages/services/service-analytics/src/metric-type-coverage.test.ts new file mode 100644 index 0000000000..533da175f4 --- /dev/null +++ b/packages/services/service-analytics/src/metric-type-coverage.test.ts @@ -0,0 +1,50 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * Every `AggregationMetricType` a measure can declare is handled, and handled + * as itself (#4157). + * + * `resolveMeasureSql` used to answer `COUNT(*)` to three different questions: + * an undeclared measure, a custom-SQL-expression metric type, and an + * unrecognised type. Each returned a plausible number — aliased under the name + * the caller asked for — for a query that asked for something else. + * + * The two sets below must partition the spec's vocabulary. Deriving one as "the + * complement of the other" would defeat the point: a *new aggregate* the spec + * grows would be classified as an expression and emitted as a bare column, + * which is a different silent wrong answer. Naming both makes a new member fail + * here instead. + */ +import { describe, it, expect } from 'vitest'; +import { AggregationMetricType } from '@objectstack/spec/data'; +import { + SUPPORTED_AGGREGATE_SQL_KEYS, + EXPRESSION_METRIC_TYPES, +} from './strategies/native-sql-strategy.js'; + +describe('AggregationMetricType coverage', () => { + it('is partitioned by the aggregate and expression sets', () => { + const handled = [...SUPPORTED_AGGREGATE_SQL_KEYS, ...EXPRESSION_METRIC_TYPES].sort(); + expect(handled).toEqual([...AggregationMetricType.options].sort()); + }); + + it('leaves no metric type to the unrecognised-type throw', () => { + const handled = new Set([...SUPPORTED_AGGREGATE_SQL_KEYS, ...EXPRESSION_METRIC_TYPES]); + const unhandled = AggregationMetricType.options.filter((t: string) => !handled.has(t)); + expect( + unhandled, + 'a declared metric type that reaches the throw is a spec member no query can use', + ).toEqual([]); + }); + + it('classifies nothing as both an aggregate and an expression', () => { + const both = SUPPORTED_AGGREGATE_SQL_KEYS.filter((a) => EXPRESSION_METRIC_TYPES.has(a)); + expect(both).toEqual([]); + }); + + it('records the current split, so a vocabulary change shows up in review', () => { + expect([...SUPPORTED_AGGREGATE_SQL_KEYS].sort()) + .toEqual(['avg', 'count', 'count_distinct', 'max', 'min', 'sum']); + expect([...EXPRESSION_METRIC_TYPES].sort()).toEqual(['boolean', 'number', 'string']); + }); +}); diff --git a/packages/services/service-analytics/src/strategies/native-sql-strategy.ts b/packages/services/service-analytics/src/strategies/native-sql-strategy.ts index 055bf7ec52..688af29e75 100644 --- a/packages/services/service-analytics/src/strategies/native-sql-strategy.ts +++ b/packages/services/service-analytics/src/strategies/native-sql-strategy.ts @@ -22,10 +22,9 @@ import { nextUtcCalendarDay } from '@objectstack/core'; * `default: COUNT(*)`, so an aggregate the spec grew would have returned a row * count instead of the number the author asked for, silently. objectui#2945. * - * Non-aggregate metric types (`number`/`string`/`boolean` — custom SQL - * expressions, `AggregationMetricType` in `data/analytics.zod.ts`) are - * deliberately absent, and keep the caller's existing fallback rather than - * changing behaviour here; see the note at {@link NativeSQLStrategy}. + * Non-aggregate metric types (`number`/`string`/`boolean`) are deliberately + * absent — they are handled by {@link EXPRESSION_METRIC_TYPES}, which emits the + * author's expression rather than wrapping it. */ const AGGREGATE_SQL: Record string> = { 'count': () => 'COUNT(*)', @@ -39,6 +38,32 @@ const AGGREGATE_SQL: Record string> = { /** Exported for the lockstep guard — the aggregates this strategy can lower. */ export const SUPPORTED_AGGREGATE_SQL_KEYS = Object.keys(AGGREGATE_SQL); +/** + * Metric types that are a custom SQL *expression*, not an aggregate to wrap. + * + * `AggregationMetricType` (`data/analytics.zod.ts`) documents these three as + * "Custom SQL expression returning a number / string / boolean" — the measure's + * `sql` IS the whole computation (a ratio, a `CASE`, a window function), so the + * only correct emission is the expression itself. They used to fall through to + * `resolveMeasureSql`'s `COUNT(*)` fallback, which threw the expression away and + * returned a row count. #4157. + * + * Named rather than derived as "everything that is not an aggregate": deriving it + * would silently classify a *new* aggregate the spec grows (`median`, …) as an + * expression and emit a bare column. `metric-type-coverage.test.ts` asserts these + * two sets partition `AggregationMetricType`, so a new member fails a test + * instead of picking a default. + */ +export const EXPRESSION_METRIC_TYPES = new Set(['number', 'string', 'boolean']); + +/** + * A dot-separated chain of bare identifiers — `amount`, `account.amount`, + * `account.owner.region`. Distinguishes a relationship PATH, which + * {@link NativeSQLStrategy.qualifyAndRegisterJoin} lowers into joins, from a SQL + * expression that merely contains a dot. #4157. + */ +const IDENTIFIER_PATH = /^[A-Za-z_][A-Za-z0-9_]*(\.[A-Za-z_][A-Za-z0-9_]*)*$/; + /** * NativeSQLStrategy — Priority 1 * @@ -46,14 +71,11 @@ export const SUPPORTED_AGGREGATE_SQL_KEYS = Object.keys(AGGREGATE_SQL); * This is the most efficient path and is preferred whenever the backing driver * supports raw SQL execution (e.g. Postgres, MySQL, SQLite). * - * Known gap, unchanged by the lockstep work and reported separately: a measure - * whose `type` is `number`/`string`/`boolean` — a custom SQL *expression*, not - * an aggregate — also lands on the `COUNT(*)` fallback in - * `resolveMeasureSql`, so its expression is replaced by a row count. Datasets - * cannot produce such a measure (`aggregateToMetricType` only ever returns an - * `AggregationFunction` member), so this is reachable only from a hand-authored - * Cube. Left as-is on purpose: emitting `col` instead is a behavioural change - * in an analytics SQL path, and deserves its own change with its own tests. + * `resolveMeasureSql` used to answer `COUNT(*)` to three different questions it + * could not otherwise answer — an undeclared measure, a custom-SQL-expression + * metric type, and an unrecognised type. All three returned a plausible number + * for a query that asked for something else. They now emit the expression or + * throw; see that method. #4157. */ export class NativeSQLStrategy implements AnalyticsStrategy { readonly name = 'NativeSQLStrategy'; @@ -313,6 +335,13 @@ export class NativeSQLStrategy implements AnalyticsStrategy { } return rawSql; } + // A dot does not by itself mean "relationship path". `SUM(account.amount)` + // is one SQL EXPRESSION that happens to contain a dot, and splitting it as a + // path produced `"SUM(account"."amount)"` plus a phantom + // `LEFT JOIN "SUM(account"` — invalid SQL and a join to a table that does not + // exist. Only qualify when every segment is a bare identifier; otherwise the + // author wrote an expression and it is returned as-is. #4157. + if (!IDENTIFIER_PATH.test(rawSql)) return rawSql; // Multi-hop (ADR-0071): the dotted path IS the join chain. Every segment but // the last is a relationship hop; the last is the column. The join ALIAS at // each hop is the full path PREFIX (`account`, then `account.owner`), which @@ -406,13 +435,37 @@ export class NativeSQLStrategy implements AnalyticsStrategy { const measure = this.lookupMember(cube, member, 'measure') as | { sql: string; type: string } | undefined; - if (!measure) return `COUNT(*)`; + // `lookupMember`'s synthetic relation fallback is dimension-only, so an + // undeclared measure name lands here — a typo, or a query naming a metric + // this cube does not have. It used to return `COUNT(*)`: the caller asked + // for revenue and got a row count, aliased AS "revenue". #4157. + if (!measure) { + const declared = Object.keys(cube.measures ?? {}); + throw new Error( + `[native-sql-strategy] cube "${cube.name}" declares no measure "${member}"` + + (declared.length ? ` (declared: ${declared.join(', ')})` : ' (it declares none)'), + ); + } const col = measure.sql === '*' ? '*' : this.qualifyAndRegisterJoin(measure.sql, parentTable, joins, cube); + const wrap = AGGREGATE_SQL[measure.type]; - return wrap ? wrap(col) : `COUNT(*)`; + if (wrap) return wrap(col); + // A custom SQL expression: the measure's `sql` IS the computation, so emit + // it unwrapped. In a grouped query the expression must itself be + // aggregate-shaped — measures never join `GROUP BY` (only dimensions do), so + // a scalar expression there is invalid SQL. That is the author's contract to + // keep; silently substituting `COUNT(*)` did not keep it for them. + if (EXPRESSION_METRIC_TYPES.has(measure.type)) return col; + + throw new Error( + `[native-sql-strategy] measure "${member}" on cube "${cube.name}" has ` + + `unrecognised type "${measure.type}" — expected an aggregate ` + + `(${SUPPORTED_AGGREGATE_SQL_KEYS.join(', ')}) or a custom-expression type ` + + `(${[...EXPRESSION_METRIC_TYPES].join(', ')}).`, + ); } private resolveFieldSql(