From d235e6e193257aeea5bfb9d507a106f84686cc34 Mon Sep 17 00:00:00 2001 From: Jack Zhuang <277994282+os-zhuang@users.noreply.github.com> Date: Thu, 30 Jul 2026 20:44:30 +0800 Subject: [PATCH] fix(analytics): a new spec aggregate can no longer silently return a row count MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Track C item 4 of objectui#2945 — "AggregationFunction: three places in lockstep". They agreed only by coincidence: AggregationFunction (spec) 8 members UNSUPPORTED_AGGREGATES 2 rejected at compile native-sql-strategy switch 6 cases + `default: COUNT(*)` 8 − 2 = 6 = the six cases, today. Add a ninth to the spec and it passes the compiler's gate (not in UNSUPPORTED), is advertised as supported by that gate's error message (a hand-written prose list — a third copy of the vocabulary), reaches the switch, matches nothing, and falls to `default: COUNT(*)`. The author asks for a median and gets a row count: no error, no log, wrong figures on a dashboard. Same shape as #3948, in the analytics SQL builder. Derivation plus a guard, no behaviour change: - the switch becomes AGGREGATE_SQL, a table whose coverage is assertable; - the error message's prose becomes SUPPORTED_AGGREGATES, derived as AggregationFunction.options minus UNSUPPORTED_AGGREGATES; - aggregation-lockstep.test.ts asserts the arithmetic in both directions. Verified by adding a hypothetical `median` to the spec: three assertions now fail naming it, including "these would fall through to the COUNT(*) fallback and return a row count". The same edit was green before. Same six aggregates, same six SQL expressions, same fallback for everything else. Reported not fixed (recorded in the strategy's doc comment): the fallback is also reached by a `number`/`string`/`boolean` metric — a custom SQL expression per AggregationMetricType — whose expression is replaced by a row count. Datasets cannot produce one, so it needs a hand-authored Cube; emitting `col` instead is a behavioural change deserving its own tests. service-analytics: 420 tests / 32 files green; tsc clean in the touched files. Refs objectstack-ai/objectui#2945, #3948 Co-Authored-By: Claude Opus 5 --- .changeset/aggregation-vocabulary-lockstep.md | 56 ++++++++++++ .../src/aggregation-lockstep.test.ts | 89 +++++++++++++++++++ .../service-analytics/src/dataset-compiler.ts | 18 +++- .../src/strategies/native-sql-strategy.ts | 48 ++++++++-- 4 files changed, 200 insertions(+), 11 deletions(-) create mode 100644 .changeset/aggregation-vocabulary-lockstep.md create mode 100644 packages/services/service-analytics/src/aggregation-lockstep.test.ts diff --git a/.changeset/aggregation-vocabulary-lockstep.md b/.changeset/aggregation-vocabulary-lockstep.md new file mode 100644 index 0000000000..d418591c9b --- /dev/null +++ b/.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. diff --git a/packages/services/service-analytics/src/aggregation-lockstep.test.ts b/packages/services/service-analytics/src/aggregation-lockstep.test.ts new file mode 100644 index 0000000000..cb2f88cf6c --- /dev/null +++ b/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,'); + }); +}); diff --git a/packages/services/service-analytics/src/dataset-compiler.ts b/packages/services/service-analytics/src/dataset-compiler.ts index db4303cc96..28b7c8998a 100644 --- a/packages/services/service-analytics/src/dataset-compiler.ts +++ b/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']; 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 3b4cb0b05b..08656d0216 100644 --- a/packages/services/service-analytics/src/strategies/native-sql-strategy.ts +++ b/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> = { + '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(