Skip to content

Commit 017130a

Browse files
os-trumpclaude
andauthored
fix(service-analytics): refuse custom-SQL measures on the ObjectQL aggregate path (#12318)
A measure whose AggregationMetricType is number/string/boolean declares a raw SQL expression as its whole computation. ObjectQLStrategy.resolveMeasureAggregation forwarded the metric type verbatim as the engine method with the expression in field, so driver-sql threw INVALID_QUERY blaming a function key the author never wrote, and the in-memory evaluator answered null for every bucket through its switch default. The prior fix for this class landed on NativeSQLStrategy only, and its regression pin forces objectqlAggregate: false, so it covered one strategy of two. The ObjectQL path now refuses such a measure with INVALID_FIELD / 400 (ADR-0112 via invalidMemberError), naming the measure the author wrote and its metric type, in the posture of the in-file cross-object refusal twin. The arm sits in the one resolver both doors call, so /analytics/query and /analytics/sql accept/reject the same set by construction. It is keyed on the declared EXPRESSION_METRIC_TYPES partition (one source shared with NativeSQLStrategy), deliberately not on a method allowlist: an enum-invalid drift type stays the platform's own undeclared-500 tier instead of being re-blamed on the caller. New pin drives one fixture through BOTH strategies: refusal envelope on the ObjectQL profile, expressions still served verbatim on the native profile, all six admitted aggregates still reaching the engine carrying their own methods, and the cross-object non-recombinable refusal keeping its exact message. Claude-Session: https://claude.ai/code/session_01UQgPSniH1GFM9ZDeGyuGUa Co-authored-by: Claude <noreply@anthropic.com>
1 parent 9fffd32 commit 017130a

4 files changed

Lines changed: 359 additions & 0 deletions

File tree

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
---
2+
"@objectstack/service-analytics": patch
3+
---
4+
5+
The ObjectQL analytics strategy now refuses a custom-SQL measure (`AggregationMetricType` `number` / `string` / `boolean`) with a loud `400 INVALID_FIELD` naming the measure and its metric type, instead of forwarding the raw SQL expression into `engine.aggregate` — where `driver-sql` rejected it blaming a `function` key the author never wrote, and the in-memory evaluator silently answered `null` for every bucket under the measure's own name.
6+
7+
What stops being served, and for whom: on deployments whose driver has no native SQL capability (the ObjectQL aggregate path — e.g. Mongo or in-memory), a query or dataset widget selecting a custom-SQL measure now answers a 400 that says to use an aggregate measure (count/sum/avg/min/max/count_distinct) or run the cube on a native-SQL driver. Those queries previously "succeeded" with a per-bucket `null` (or a mis-attributed driver error), never with a correct number. Native-SQL driver behaviour is unchanged: custom-SQL measures still run there, emitted verbatim.
Lines changed: 303 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,303 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* #12209 — a custom-SQL measure is refused loudly on the ObjectQL path, and
5+
* BOTH strategies are pinned from one fixture so neither can hide the other.
6+
*
7+
* #4157 was fixed on one strategy of two: `NativeSQLStrategy` learned to emit
8+
* a `number`/`string`/`boolean` measure's `sql` verbatim, and its regression
9+
* pin (`measure-expression-sql.test.ts`) forces `objectqlAggregate: false` —
10+
* so the pin covered exactly one strategy. `ObjectQLStrategy` never got the
11+
* matching partition: `resolveMeasureAggregation` forwarded `Metric.type`
12+
* verbatim as the engine method with the whole SQL expression in `field`, so
13+
* `driver-sql` threw `INVALID_QUERY` blaming a `function` key the author never
14+
* wrote, and the in-memory evaluator answered `null` for every bucket through
15+
* its `switch` default — the #4157 class in its null variant, measured on
16+
* #12053's probe: an admitted `sum` returned 300 per bucket where the
17+
* custom-SQL measure returned `null`.
18+
*
19+
* This file is the pin the defect could not hide from: ONE cube whose measures
20+
* cover all six aggregates, all three expression types and one enum-invalid
21+
* drift type, driven through the real `AnalyticsService` routing under BOTH
22+
* capability profiles. The native profile pins the expression measures still
23+
* SERVED (emitted verbatim); the ObjectQL profile pins them REFUSED — same
24+
* fixture, so a change that moves either posture turns a case here red.
25+
*
26+
* The load-bearing negatives, and why they are here:
27+
*
28+
* - every admitted AGGREGATE measure is still served on the ObjectQL path and
29+
* still reaches the engine carrying its OWN method (`sum` stays `sum`). An
30+
* implementation that refuses by method membership (e.g. reusing
31+
* `RECOMBINABLE_METHODS`, which lacks `avg`/`count_distinct`) passes the
32+
* refusal cases and goes red here.
33+
* - an enum-INVALID metric type (`median` — host drift, not authorable) is NOT
34+
* refused with the caller-shaped `INVALID_FIELD` envelope. The drift tier is
35+
* the platform's own (`dataset-refusal.ts` header, #5716): an implementation
36+
* that refuses "every method that is not one of the six aggregates" passes
37+
* the refusal cases too, and goes red here — the arm must key on the
38+
* DECLARED expression partition (`EXPRESSION_METRIC_TYPES`), not on a method
39+
* allowlist.
40+
* - the pre-existing cross-object non-recombinable refusal keeps its EXACT
41+
* message — the new arm sits beside it, not over it.
42+
*
43+
* ## Dissolution verification, direction predicted BEFORE running
44+
*
45+
* Restoring the accepting behaviour (deleting the #12209 arm in
46+
* `ObjectQLStrategy.resolveMeasureAggregation`) must turn the ObjectQL-profile
47+
* REFUSAL cases red in the ordinary direction: each asserts the ADR-0112
48+
* envelope (`code`/`status`), the measure's own name in `member` and message,
49+
* AND that nothing reached the engine (`calls`/`sqls` empty) — with the arm
50+
* gone, the query "succeeds", the engine IS reached carrying the expression in
51+
* `field`, so the cases cannot pass vacuously. Every other case — the six
52+
* admitted aggregates, the drift tier, the native-profile SERVED block, the
53+
* cross-object twin — is predicted to stay GREEN in both directions: none of
54+
* them touches the arm.
55+
*/
56+
57+
import { describe, it, expect, vi } from 'vitest';
58+
import type { Cube } from '@objectstack/spec/data';
59+
import { AnalyticsService } from '../analytics-service.js';
60+
61+
const silentLogger = {
62+
info: vi.fn(),
63+
debug: vi.fn(),
64+
warn: vi.fn(),
65+
error: vi.fn(),
66+
child: vi.fn().mockReturnThis(),
67+
} as any;
68+
69+
/** `orders`' real columns — every bare-identifier measure/dimension source. */
70+
const ORDER_FIELDS = ['id', 'amount', 'cost', 'revenue', 'paid', 'buyer', 'status', 'account', 'created_at'];
71+
72+
/**
73+
* One cube, both strategies: all six aggregate types, all three custom-SQL
74+
* expression types. The expression `sql`s are deliberately DOT-FREE — an
75+
* expression containing a dot was already (mis)refused as a cross-object
76+
* measure, so the dot-free ones are the exact shapes that used to reach
77+
* `engine.aggregate` and answer `null`.
78+
*/
79+
const CUBE: Cube = {
80+
name: 'orders',
81+
title: 'Orders',
82+
sql: 'orders',
83+
measures: {
84+
orders_count: { name: 'orders_count', label: 'Count', type: 'count', sql: '*' },
85+
total: { name: 'total', label: 'Total', type: 'sum', sql: 'amount' },
86+
avg_amount: { name: 'avg_amount', label: 'Avg', type: 'avg', sql: 'amount' },
87+
min_amount: { name: 'min_amount', label: 'Min', type: 'min', sql: 'amount' },
88+
max_amount: { name: 'max_amount', label: 'Max', type: 'max', sql: 'amount' },
89+
buyers: { name: 'buyers', label: 'Buyers', type: 'count_distinct', sql: 'buyer' },
90+
margin: {
91+
name: 'margin', label: 'Margin', type: 'number',
92+
sql: 'SUM(revenue) / NULLIF(SUM(cost), 0)',
93+
},
94+
top_status: {
95+
name: 'top_status', label: 'Top status', type: 'string',
96+
sql: "MAX(CASE WHEN paid THEN 'paid' ELSE 'open' END)",
97+
},
98+
any_paid: { name: 'any_paid', label: 'Any paid', type: 'boolean', sql: 'MAX(paid)' },
99+
},
100+
dimensions: {
101+
status: { name: 'status', label: 'Status', type: 'string', sql: 'status' },
102+
},
103+
joins: { account: { name: 'crm_account', relationship: 'belongsTo', sql: '' } },
104+
} as never;
105+
106+
/**
107+
* An enum-INVALID metric type. `AggregationMetricType` is closed and `median`
108+
* is not in it, so no spec-valid cube can declare this — it models host drift
109+
* (a cube registered without meeting `CubeSchema`). The drift tier belongs to
110+
* the platform, never to the caller (#5716 / `dataset-refusal.ts`).
111+
*/
112+
const DRIFT_CUBE: Cube = {
113+
name: 'orders_drift',
114+
title: 'Orders drift',
115+
sql: 'orders',
116+
measures: { weird: { name: 'weird', label: 'Weird', type: 'median', sql: 'amount' } },
117+
dimensions: { status: { name: 'status', label: 'Status', type: 'string', sql: 'status' } },
118+
} as never;
119+
120+
type Refusal = Error & {
121+
code?: string;
122+
status?: number;
123+
member?: string;
124+
param?: string;
125+
cube?: string;
126+
};
127+
128+
function makeService(profile: 'objectql' | 'native') {
129+
const sqls: string[] = [];
130+
const calls: Array<{ object: string; aggregations?: unknown; groupBy?: unknown }> = [];
131+
const service = new AnalyticsService({
132+
logger: silentLogger,
133+
cubes: [CUBE, DRIFT_CUBE],
134+
queryCapabilities: () => ({
135+
nativeSql: profile === 'native',
136+
objectqlAggregate: profile === 'objectql',
137+
inMemory: false,
138+
}),
139+
executeAggregate: async (object: string, options: any) => {
140+
calls.push({ object, aggregations: options?.aggregations, groupBy: options?.groupBy });
141+
return [{ status: 'open', total: 300 }];
142+
},
143+
executeRawSql: async (_object: string, sql: string) => {
144+
sqls.push(sql);
145+
return [{ status: 'open' }];
146+
},
147+
isRegisteredObject: (n: string) => n === 'orders',
148+
getObjectFieldNames: (n: string) => (n === 'orders' ? ORDER_FIELDS : undefined),
149+
} as any);
150+
return { service, sqls, calls };
151+
}
152+
153+
/** Run one query on a fresh service under `profile`, reporting everything. */
154+
async function run(query: unknown, profile: 'objectql' | 'native') {
155+
const { service, sqls, calls } = makeService(profile);
156+
let rows: Array<Record<string, unknown>> | undefined;
157+
let error: Refusal | undefined;
158+
try {
159+
rows = (await service.query(query as never)).rows as Array<Record<string, unknown>>;
160+
} catch (e) {
161+
error = e as Refusal;
162+
}
163+
return { rows, error, sqls, calls };
164+
}
165+
166+
/** The one wire shape every #12209 refusal must have (ADR-0112 / #5716). */
167+
function expectCustomSqlRefusal(
168+
r: { error?: Refusal; sqls: string[]; calls: unknown[] },
169+
member: string,
170+
type: string,
171+
) {
172+
expect(r.error).toBeInstanceOf(Error);
173+
expect(r.error?.code).toBe('INVALID_FIELD');
174+
expect(r.error?.status).toBe(400);
175+
// The measure AS THE AUTHOR WROTE IT — today's failure blames a `function`
176+
// key the author never wrote, or answers null under this very name.
177+
expect(r.error?.member).toBe(member);
178+
expect(r.error?.param).toBe('measures');
179+
expect(r.error?.cube).toBe('orders');
180+
expect(r.error?.message).toContain(`("${member}")`);
181+
expect(r.error?.message).toContain(`type "${type}"`);
182+
// The in-file twin's posture: name the way out, both halves.
183+
expect(r.error?.message).toContain('or run on a native-SQL driver');
184+
// The refusal is a refusal: the engine was never reached, nothing executed.
185+
expect(r.calls).toEqual([]);
186+
expect(r.sqls).toEqual([]);
187+
}
188+
189+
// ── 1. The ObjectQL path REFUSES what it cannot serve ────────────────────────
190+
191+
describe('ObjectQL path: custom-SQL measures are refused loudly', () => {
192+
it.each([
193+
['margin', 'number'],
194+
['top_status', 'string'],
195+
['any_paid', 'boolean'],
196+
] as const)('refuses "%s" (type %s) with INVALID_FIELD/400, engine never reached', async (member, type) => {
197+
const r = await run({ cube: 'orders', measures: [member], dimensions: ['status'] }, 'objectql');
198+
expectCustomSqlRefusal(r, member, type);
199+
});
200+
201+
it('an admitted measure beside it does not rescue the query — the custom-SQL member is named', async () => {
202+
const r = await run({ cube: 'orders', measures: ['total', 'margin'], dimensions: ['status'] }, 'objectql');
203+
expectCustomSqlRefusal(r, 'margin', 'number');
204+
});
205+
206+
it('refuses on the scalar (no-dimension) shape too', async () => {
207+
const r = await run({ cube: 'orders', measures: ['margin'] }, 'objectql');
208+
expectCustomSqlRefusal(r, 'margin', 'number');
209+
});
210+
});
211+
212+
// ── 2. The load-bearing negative: admitted aggregates still served ───────────
213+
214+
describe('ObjectQL path: every admitted aggregate is still served, carrying its own method', () => {
215+
it('an admitted sum measure reaches the engine as {field, method: "sum"} and answers', async () => {
216+
const r = await run({ cube: 'orders', measures: ['total'], dimensions: ['status'] }, 'objectql');
217+
expect(r.error).toBeUndefined();
218+
expect(r.calls).toHaveLength(1);
219+
expect(r.calls[0].aggregations).toEqual([{ field: 'amount', method: 'sum', alias: 'total' }]);
220+
expect(r.rows?.[0]?.total).toBe(300);
221+
});
222+
223+
it('all six aggregate types reach the engine, each carrying its own method', async () => {
224+
const r = await run({
225+
cube: 'orders',
226+
measures: ['orders_count', 'total', 'avg_amount', 'min_amount', 'max_amount', 'buyers'],
227+
dimensions: ['status'],
228+
}, 'objectql');
229+
expect(r.error).toBeUndefined();
230+
expect(r.calls).toHaveLength(1);
231+
expect(r.calls[0].aggregations).toEqual([
232+
{ field: '*', method: 'count', alias: 'orders_count' },
233+
{ field: 'amount', method: 'sum', alias: 'total' },
234+
{ field: 'amount', method: 'avg', alias: 'avg_amount' },
235+
{ field: 'amount', method: 'min', alias: 'min_amount' },
236+
{ field: 'amount', method: 'max', alias: 'max_amount' },
237+
{ field: 'buyer', method: 'count_distinct', alias: 'buyers' },
238+
]);
239+
});
240+
241+
it('an enum-invalid drift type is NOT refused as the caller\'s mistake', async () => {
242+
// `median` is not authorable (`AggregationMetricType` is closed), so an
243+
// arrival is OUR drift — the undeclared-500 tier, never the caller-shaped
244+
// 400 (#5716). This is the case that reds a "refuse every method that is
245+
// not one of the six aggregates" implementation: extensionally identical
246+
// to the partition check on every enum-valid cube, it re-blames the
247+
// caller exactly here.
248+
const r = await run({ cube: 'orders_drift', measures: ['weird'], dimensions: ['status'] }, 'objectql');
249+
expect(r.error?.code).not.toBe('INVALID_FIELD');
250+
});
251+
});
252+
253+
// ── 3. The other strategy on the SAME fixture: still serves the expression ───
254+
255+
describe('native-SQL path: the same custom-SQL measures stay served', () => {
256+
it('emits the number expression verbatim, no refusal', async () => {
257+
const r = await run({ cube: 'orders', measures: ['margin'], dimensions: ['status'] }, 'native');
258+
expect(r.error).toBeUndefined();
259+
expect(r.sqls).toHaveLength(1);
260+
expect(r.sqls[0]).toContain('SUM(revenue) / NULLIF(SUM(cost), 0) AS "margin"');
261+
expect(r.calls).toEqual([]);
262+
});
263+
264+
it('emits string and boolean expressions verbatim, no refusal', async () => {
265+
const r = await run({ cube: 'orders', measures: ['top_status', 'any_paid'] }, 'native');
266+
expect(r.error).toBeUndefined();
267+
expect(r.sqls[0]).toContain(`MAX(CASE WHEN paid THEN 'paid' ELSE 'open' END) AS "top_status"`);
268+
expect(r.sqls[0]).toContain('MAX(paid) AS "any_paid"');
269+
});
270+
});
271+
272+
// ── 4. The twin keeps its exact message ──────────────────────────────────────
273+
274+
describe('the cross-object non-recombinable refusal is untouched beside the new arm', () => {
275+
it('still refuses avg + cross-object dimension with its exact shipped message', async () => {
276+
const r = await run(
277+
{ cube: 'orders', dimensions: ['account.region'], measures: ['avg_amount'] },
278+
'objectql',
279+
);
280+
expect(r.error?.code).toBe('INVALID_FIELD');
281+
expect(r.error?.status).toBe(400);
282+
expect(r.error?.member).toBe('avg_amount');
283+
expect(r.error?.message).toBe(
284+
'[Analytics] ObjectQLStrategy cannot group by a cross-object dimension ' +
285+
'with a "avg" measure ("avg_amount") — its value cannot be recombined ' +
286+
'across the intermediate FK grouping. Use sum/count/min/max, or run on ' +
287+
'a native-SQL driver.',
288+
);
289+
expect(r.calls).toEqual([]);
290+
});
291+
292+
it('a custom-SQL measure beside a cross-object dimension is refused as custom-SQL', async () => {
293+
// Deliberate precedence: the custom-SQL verdict names the real defect (the
294+
// measure can never run on this engine, cross-object dimension or not),
295+
// and both doors reach it through the one resolver — so the attribution
296+
// cannot fork between /analytics/query and /analytics/sql.
297+
const r = await run(
298+
{ cube: 'orders', dimensions: ['account.region'], measures: ['margin'] },
299+
'objectql',
300+
);
301+
expectCustomSqlRefusal(r, 'margin', 'number');
302+
});
303+
});

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

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -103,6 +103,11 @@ export const CONDITIONAL_AGGREGATE_SQL_KEYS = Object.keys(CONDITIONAL_AGGREGATE_
103103
* expression and emit a bare column. `metric-type-coverage.test.ts` asserts these
104104
* two sets partition `AggregationMetricType`, so a new member fails a test
105105
* instead of picking a default.
106+
*
107+
* [#12209] `ObjectQLStrategy.resolveMeasureAggregation` keys its refusal arm on
108+
* this same set — the engine aggregate AST cannot carry a raw SQL expression,
109+
* so the ObjectQL path REFUSES exactly what this strategy emits verbatim. One
110+
* set, two strategies, so the partition cannot fork per path.
106111
*/
107112
export const EXPRESSION_METRIC_TYPES = new Set(['number', 'string', 'boolean']);
108113

packages/services/service-analytics/src/strategies/objectql-strategy.ts

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,10 @@ import {
2727
type MeasureRecombine,
2828
type RecombinableMethod,
2929
} from './cross-object-rebucket.js';
30+
// [#12209] The custom-SQL half of the `AggregationMetricType` partition, ONE
31+
// source shared with `NativeSQLStrategy` and pinned against the spec enum by
32+
// `metric-type-coverage.test.ts` — a second literal set here would drift.
33+
import { EXPRESSION_METRIC_TYPES } from './native-sql-strategy.js';
3034

3135
/**
3236
* [#10861 / #11461] Where a member in the cross-object envelope's inventory
@@ -1260,6 +1264,46 @@ export class ObjectQLStrategy implements AnalyticsStrategy {
12601264
| { sql: string; type: string }
12611265
| undefined;
12621266
if (direct) {
1267+
// [#12209] A custom-SQL measure (`AggregationMetricType`
1268+
// `number`/`string`/`boolean`) is REFUSED here rather than forwarded. Its
1269+
// `sql` IS the whole computation (a ratio, a `CASE`, a window function),
1270+
// and the engine aggregate AST has no place to carry a raw SQL
1271+
// expression: forwarding put the whole expression in `field` and the
1272+
// metric TYPE in `method`, so `driver-sql` threw `INVALID_QUERY`/400
1273+
// blaming a `function` key the author never wrote, and the in-memory
1274+
// evaluator answered `null` for every bucket through its `switch`
1275+
// default — a silent wrong answer under the author's own metric name,
1276+
// the #4157 class in its null variant. #4157's fix landed on
1277+
// `NativeSQLStrategy` only (where the expression is legal and emitted
1278+
// verbatim, `EXPRESSION_METRIC_TYPES`); this arm is the matching
1279+
// partition on the strategy that cannot serve it.
1280+
//
1281+
// Same posture and same envelope as `planCrossObject`'s refusals below
1282+
// (`INVALID_FIELD` / 400, #5716; the non-recombinable-measure arm is the
1283+
// wording twin): the engine physically cannot evaluate this member, and
1284+
// a loud, correctly-attributed refusal beats a silent wrong number.
1285+
// Sitting HERE — the one resolver both doors call — keeps
1286+
// `/analytics/query` and `/analytics/sql` accepting/rejecting the same
1287+
// set by construction (#10759's invariant).
1288+
//
1289+
// Keyed on the DECLARED metric-type partition, deliberately NOT on
1290+
// "method is not one of the six aggregates": the two read identically on
1291+
// every enum-valid cube, but an enum-INVALID type (host drift, e.g. a
1292+
// cube registered without meeting `CubeSchema`) is OUR bug — the
1293+
// undeclared-500 tier `dataset-refusal.ts`'s header assigns it — and a
1294+
// method allowlist would re-blame the caller for it with a 400.
1295+
if (EXPRESSION_METRIC_TYPES.has(direct.type)) {
1296+
throw invalidMemberError(
1297+
`[Analytics] ObjectQLStrategy cannot evaluate the custom-SQL measure ` +
1298+
`("${measureName}") — its type "${direct.type}" declares a raw SQL ` +
1299+
`expression, which the engine aggregate AST cannot carry; served ` +
1300+
`anyway it would answer null for every bucket under the measure's ` +
1301+
`own name. Use an aggregate measure ` +
1302+
`(count/sum/avg/min/max/count_distinct), or run on a native-SQL ` +
1303+
`driver.`,
1304+
{ member: measureName, param: 'measures', cube: cube.name },
1305+
);
1306+
}
12631307
return {
12641308
field: direct.sql.replace(/^\$/, ''),
12651309
method: direct.type === 'count_distinct' ? 'count_distinct' : direct.type,

0 commit comments

Comments
 (0)