fix(analytics)!: a measure emits what it declares, instead of COUNT(*) (#4157)

.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.

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');
+  });
+});

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

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, (col: string) => string> = {
   'count': () => 'COUNT(*)',
@@ -39,21 +38,44 @@ const AGGREGATE_SQL: Record<string, (col: string) => 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
  *
  * Pushes the analytics query down to the database as a native SQL statement.
  * 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(