fix(analytics): a new spec aggregate can no longer silently return a row count

.changeset/aggregation-vocabulary-lockstep.md

@@ -0,0 +1,56 @@
+---
+"@objectstack/service-analytics": patch
+---
+
+fix(analytics): a new spec aggregate can no longer silently return a row count
+
+Track C item 4 of objectstack-ai/objectui#2945 — *"`AggregationFunction`: three
+places in lockstep"*. They agreed only by coincidence, and the failure mode when
+they stopped agreeing was silent wrong numbers.
+
+The three:
+
+1. `AggregationFunction` (`@objectstack/spec/data`) — eight members, what an
+ author may declare as a dataset measure's `aggregate`.
+2. `UNSUPPORTED_AGGREGATES` (`dataset-compiler.ts`) — `array_agg`/`string_agg`,
+ rejected at compile time with a clear error.
+3. The aggregate `switch` in `native-sql-strategy.ts` — six cases, then
+ `default: return 'COUNT(*)'`.
+
+8 − 2 = 6 = the six cases, today. Add a ninth member to the spec — `median`,
+`percentile`, anything — and it would:
+
+- pass the compiler's gate, since it is not in `UNSUPPORTED_AGGREGATES`;
+- be **advertised as supported** by that gate's error message, which listed
+ `count, sum, avg, min, max, count_distinct` as hand-written prose — a third
+ copy of the vocabulary;
+- reach the strategy's `switch`, match no case, and fall to
+ `default: COUNT(*)`.
+
+The author asks for a median and gets a row count. No error, no log, wrong
+figures on a dashboard — the same silent-wrong-answer shape as the filter
+operators in #3948, in the analytics SQL builder.
+
+**The fix is derivation plus a guard, with no behaviour change.** The `switch`
+becomes `AGGREGATE_SQL`, a table whose coverage is assertable; the error
+message's prose list becomes `SUPPORTED_AGGREGATES`, derived as
+`AggregationFunction.options` minus `UNSUPPORTED_AGGREGATES`; and
+`aggregation-lockstep.test.ts` asserts the arithmetic — the lowered set equals
+the admitted set, every spec member is either lowered or explicitly rejected,
+nothing is both, and the rejection list names only aggregates the spec has.
+
+Verified by adding a hypothetical `median` to the spec, which now fails three
+assertions naming it, including *"these would fall through to the COUNT(*)
+fallback and return a row count"*. Before this change the same edit was green.
+
+Nothing is narrowed and no SQL changes: the same six aggregates lower to the
+same six expressions, and the `COUNT(*)` fallback still catches everything else.
+
+**Reported, not fixed:** that fallback is also reached by a measure whose `type`
+is `number`/`string`/`boolean` — a custom SQL *expression*, per
+`AggregationMetricType` — whose expression is then replaced by a row count.
+Datasets cannot produce one (`aggregateToMetricType` only ever returns an
+`AggregationFunction` member), so it is reachable only from a hand-authored
+Cube. Emitting `col` instead is a behavioural change in an analytics SQL path
+and deserves its own change with its own tests; the strategy's doc comment now
+records it.

packages/services/service-analytics/src/aggregation-lockstep.test.ts

@@ -0,0 +1,89 @@
+// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
+
+/**
+ * One aggregate vocabulary, three places that must agree (objectui#2945 Track C).
+ *
+ * 1. `AggregationFunction` (`@objectstack/spec/data`) — what an author may
+ * declare as a dataset measure's `aggregate`.
+ * 2. `UNSUPPORTED_AGGREGATES` (`dataset-compiler.ts`) — the ones v1 rejects at
+ * compile time, with a clear error.
+ * 3. `AGGREGATE_SQL` (`native-sql-strategy.ts`) — the ones it can lower to SQL.
+ *
+ * (2) and (3) are complements of each other over (1), and nothing enforced it.
+ * The strategy used to `switch` with `default: return 'COUNT(*)'`, so an
+ * aggregate added to the spec would pass the compiler's gate, be advertised as
+ * supported by its error message, and then return **a row count in place of the
+ * number the author asked for** — no error, no log, wrong analytics.
+ *
+ * These tests make that arithmetic explicit, so growing the vocabulary fails
+ * here instead of shipping a silently wrong figure.
+ */
+import { describe, it, expect } from 'vitest';
+import { AggregationFunction } from '@objectstack/spec/data';
+import { UNSUPPORTED_AGGREGATES, SUPPORTED_AGGREGATES } from './dataset-compiler.js';
+import { SUPPORTED_AGGREGATE_SQL_KEYS } from './strategies/native-sql-strategy.js';
+
+describe('aggregate vocabulary lockstep', () => {
+ it('the strategy lowers exactly the aggregates the compiler admits', () => {
+ expect([...SUPPORTED_AGGREGATE_SQL_KEYS].sort()).toEqual([...SUPPORTED_AGGREGATES].sort());
+ });
+
+ it('every spec aggregate is either lowered or explicitly rejected', () => {
+ const lowered = new Set(SUPPORTED_AGGREGATE_SQL_KEYS);
+ const unhandled = AggregationFunction.options.filter(
+ (a: string) => !lowered.has(a) && !UNSUPPORTED_AGGREGATES.has(a),
+ );
+ expect(
+ unhandled,
+ 'these would fall through to the COUNT(*) fallback and return a row count',
+ ).toEqual([]);
+ });
+
+ it('nothing is both rejected and lowered', () => {
+ const both = SUPPORTED_AGGREGATE_SQL_KEYS.filter((a) => UNSUPPORTED_AGGREGATES.has(a));
+ expect(both).toEqual([]);
+ });
+
+ it('the rejection list names only aggregates the spec actually has', () => {
+ // A stale entry here silently *widens* what v1 claims to support: the name
+ // is subtracted from SUPPORTED_AGGREGATES for nothing.
+ const stray = [...UNSUPPORTED_AGGREGATES].filter(
+ (a) => !(AggregationFunction.options as string[]).includes(a),
+ );
+ expect(stray).toEqual([]);
+ });
+
+ it('the two halves partition the vocabulary', () => {
+ expect(SUPPORTED_AGGREGATES.length + UNSUPPORTED_AGGREGATES.size)
+ .toBe(AggregationFunction.options.length);
+ });
+
+ it('records the current split, so a vocabulary change is visible in review', () => {
+ expect([...SUPPORTED_AGGREGATES].sort())
+ .toEqual(['avg', 'count', 'count_distinct', 'max', 'min', 'sum']);
+ expect([...UNSUPPORTED_AGGREGATES].sort()).toEqual(['array_agg', 'string_agg']);
+ });
+});
+
+describe('the compiler error message is derived, not restated', () => {
+ it('names every supported aggregate, and none of the unsupported ones', async () => {
+ const { compileDataset } = await import('./dataset-compiler.js');
+ let message = '';
+ try {
+ compileDataset({
+ name: 'agg_probe',
+ object: 'showcase_task',
+ dimensions: [{ name: 'status', field: 'status', type: 'string' }],
+ measures: [{ name: 'names', field: 'title', aggregate: 'string_agg' }],
+ } as never);
+ } catch (e) {
+ message = (e as Error).message;
+ }
+ expect(message).toContain('string_agg');
+ for (const a of SUPPORTED_AGGREGATES) {
+ expect(message, `error message omits supported aggregate "${a}"`).toContain(a);
+ }
+ // The prose list it replaced would have kept claiming these are supported.
+ expect(message).not.toContain('array_agg,');
+ });
+});

packages/services/service-analytics/src/dataset-compiler.ts

@@ -1,6 +1,7 @@
 // Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
 
 import type { Cube, Metric, Dimension as CubeDimension, CubeJoin } from '@objectstack/spec/data';
+import { AggregationFunction } from '@objectstack/spec/data';
 import type { Dataset, DatasetMeasure, DatasetDimension } from '@objectstack/spec/ui';
 import type { FilterCondition } from '@objectstack/spec/data';
 
@@ -21,7 +22,20 @@ import type { FilterCondition } from '@objectstack/spec/data';
  */
 
 /** Operators v1 does NOT compile to the Cube SQL switch — surfaced as a clear error. */
-const UNSUPPORTED_AGGREGATES = new Set(['array_agg', 'string_agg']);
+export const UNSUPPORTED_AGGREGATES = new Set(['array_agg', 'string_agg']);
+
+/**
+ * What v1 *can* lower — derived from the spec's vocabulary rather than restated.
+ *
+ * The list used to be hand-written prose inside the error message below, which
+ * made it a third copy of one vocabulary (after `AggregationFunction` and the
+ * `native-sql-strategy` switch) with nothing keeping the three in step. An
+ * aggregate added to the spec would have passed this gate, been reported as
+ * supported by that message, and then hit the strategy's `default` — returning
+ * a row count in place of the requested number. objectui#2945.
+ */
+export const SUPPORTED_AGGREGATES: string[] = AggregationFunction.options
+ .filter((a: string) => !UNSUPPORTED_AGGREGATES.has(a));
 
 export interface DerivedMeasureSpec {
  name: string;
@@ -82,7 +96,7 @@ function aggregateToMetricType(m: DatasetMeasure): Metric['type'] {
  if (UNSUPPORTED_AGGREGATES.has(m.aggregate)) {
  throw new Error(
  `[dataset-compiler] measure "${m.name}" uses aggregate "${m.aggregate}" which is ` +
- `not supported by the v1 dataset runtime (supported: count, sum, avg, min, max, count_distinct).`,
+ `not supported by the v1 dataset runtime (supported: ${SUPPORTED_AGGREGATES.join(', ')}).`,
  );
  }
  return m.aggregate as Metric['type'];

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

@@ -7,12 +7,49 @@ import { normalizeAnalyticsFilters, coerceFilterValueForSql } from './filter-nor
 import { compileScopedFilterToSql } from '../read-scope-sql.js';
 import { nextUtcCalendarDay } from '@objectstack/core';
 
+/**
+ * The SQL wrapper for each aggregate a measure's `type` can name.
+ *
+ * A table rather than a `switch` so its coverage is *assertable*: the aggregate
+ * vocabulary lives in `@objectstack/spec` (`AggregationFunction`), the dataset
+ * compiler subtracts the two it cannot lower (`array_agg`, `string_agg`), and
+ * `aggregation-lockstep.test.ts` checks that what remains is exactly the keys
+ * below. A `switch` gave that no purchase — the missing case fell to
+ * `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}.
+ */
+const AGGREGATE_SQL: Record<string, (col: string) => string> = {
+ 'count': () => 'COUNT(*)',
+ 'sum': (col) => `SUM(${col})`,
+ 'avg': (col) => `AVG(${col})`,
+ 'min': (col) => `MIN(${col})`,
+ 'max': (col) => `MAX(${col})`,
+ 'count_distinct': (col) => `COUNT(DISTINCT ${col})`,
+};
+
+/** Exported for the lockstep guard — the aggregates this strategy can lower. */
+export const SUPPORTED_AGGREGATE_SQL_KEYS = Object.keys(AGGREGATE_SQL);
+
 /**
  * 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.
  */
 export class NativeSQLStrategy implements AnalyticsStrategy {
  readonly name = 'NativeSQLStrategy';
@@ -370,15 +407,8 @@ export class NativeSQLStrategy implements AnalyticsStrategy {
  const col = measure.sql === '*'
  ? '*'
  : this.qualifyAndRegisterJoin(measure.sql, parentTable, joins, cube);
- switch (measure.type) {
- case 'count': return 'COUNT(*)';
- case 'sum': return `SUM(${col})`;
- case 'avg': return `AVG(${col})`;
- case 'min': return `MIN(${col})`;
- case 'max': return `MAX(${col})`;
- case 'count_distinct': return `COUNT(DISTINCT ${col})`;
- default: return `COUNT(*)`;
- }
+ const wrap = AGGREGATE_SQL[measure.type];
+ return wrap ? wrap(col) : `COUNT(*)`;
  }
 
  private resolveFieldSql(