From fb67f94ff5c3cce2c9290043a4639e0cfc7710c3 Mon Sep 17 00:00:00 2001 From: Claude Fable 5 Date: Fri, 7 Aug 2026 13:06:11 +0000 Subject: [PATCH] fix(service-analytics): refuse a dotted `measures` entry loudly, naming the caller's spelling (#5918) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `inferCubeFromQuery`'s measure mint dropped the first segment of ANY dotted entry, so `owner.region_count_distinct` minted a Metric over the BASE table's own `region` column: `SELECT COUNT(DISTINCT region) AS "owner.region_count_distinct" FROM "crm_account"` — no JOIN, no error, and a response column labelled with a relation attribute whose number came from the base table. Where the object had no same-named column it degraded to #4437's `400 INVALID_FIELD` naming the STRIPPED tail, about a string the caller had never written. `measures` is the fourth mint site of the punctuation #5739 sorted out on the three dimension-shaped ones, and it is ruled the other way: `lookupMember`'s relation-traversal tier is dimension-only, so a dotted measure has no correct traversal answer to converge on. Maintainer ruling 2026-08-07 — refuse it, with the caller's own spelling in the envelope (`INVALID_FIELD` / 400 / `member` / `param: 'measures'`, the #4437 family's shape, via `invalidMemberError`). The rule lives in one place (`mintableMeasureKey`) and both measure mints call it: the ad-hoc one and `ensureCube`'s suffix-augmentation loop. Measured: the ad-hoc path REGISTERS what it infers, so from the second request onwards the same query reaches the augmentation loop instead — refusing at one site only would have left every warm service exactly as it was. A measure the cube DECLARES is authored, not minted, and is checked verbatim first so a cube's own dotted key still resolves and still compiles its JOIN. Unchanged: the `.` qualifier, bare measures, and the dimension traversal #5739 ruled the other way. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_015a5qkLzpGXhLL2F5gvJ7dD --- .../analytics-dotted-measure-refusal.md | 55 +++ .../__tests__/dotted-measure-refusal.test.ts | 421 ++++++++++++++++++ .../infer-cube-relation-traversal.test.ts | 37 +- .../measure-source-field-gate.test.ts | 48 +- .../src/analytics-service.ts | 144 +++++- 5 files changed, 673 insertions(+), 32 deletions(-) create mode 100644 .changeset/analytics-dotted-measure-refusal.md create mode 100644 packages/services/service-analytics/src/__tests__/dotted-measure-refusal.test.ts diff --git a/.changeset/analytics-dotted-measure-refusal.md b/.changeset/analytics-dotted-measure-refusal.md new file mode 100644 index 0000000000..92ceecb889 --- /dev/null +++ b/.changeset/analytics-dotted-measure-refusal.md @@ -0,0 +1,55 @@ +--- +"@objectstack/service-analytics": patch +--- + +fix(service-analytics): refuse a dotted `measures` entry loudly instead of aggregating the base column (#5918) + +**Observable behaviour change.** An analytics query whose `measures` entry +carries a dot that is not the cube-name qualifier — `owner.region_count_distinct`, +`total.sum` — now answers `400 INVALID_FIELD` naming the entry **as the request +spelled it**. Some of these queries used to succeed. + +That is the point: succeeding is what was wrong with them. The auto-inference +path minted a measure by dropping the first segment of any dotted entry, so on +an object that happened to carry a same-named column the query ran + +``` +SELECT COUNT(DISTINCT region) AS "owner.region_count_distinct" FROM "crm_account" +``` + +— no JOIN, no error, a response column labelled with a relation attribute and a +number that came from the base table. The caller could not tell from the result +that it was wrong. Where the object had no same-named column it degraded to the +#4437 gate's `400 INVALID_FIELD`, which was honest about what reached SQL +(`aggregates field 'score'`) but named a string nobody had written; the caller +had sent `owner.score_sum`. + +`measures` was the fourth and last mint site of the punctuation #5739 sorted +out on `dimensions` / `where` / `timeDimensions`. It is ruled the other way, and +deliberately so: `lookupMember`'s relation-traversal tier is dimension-only, so a +dotted measure has no correct traversal answer to converge on. A refusal is the +honest answer, and it costs nothing that was working. Maintainer ruling, +2026-08-07. + +Both a genuine traversal intent (`owner.amount_sum`) and a plain typo +(`total.sum`) get this refusal. They are lexically indistinguishable on this +path, and separating them would need field metadata the ad-hoc path does not +have. A real relation-traversal measure (`SUM("owner"."amount")` + LEFT JOIN) +would be a capability with its own justification, not a side effect of a strip. + +The refusal is applied at both places a Metric is minted from a request +spelling — the ad-hoc mint and the suffix-augmentation mint for a cube that is +already registered — because the ad-hoc path registers what it infers, so the +very same query reaches the second one from the second request onwards. + +Unchanged: the `.` qualifier (`crm_account.region_count_distinct`) is +still stripped and still runs; bare measures (`region_count_distinct`, `count`, +`created_at_max`) are untouched; a cube's own declared measure is authored, not +minted, so a Cube whose measure names a related column in its `sql` still +compiles the JOIN — which is the supported way to aggregate across a +relationship; and dotted **dimensions** still traverse, per #5739. + +**Migration.** Aggregate one of the object's own fields +(`_sum` / `_avg` / `_min` / `_max` / `_count_distinct`), or declare a Cube +whose measure names the related column. The refusal message says both, and names +the entry you sent. diff --git a/packages/services/service-analytics/src/__tests__/dotted-measure-refusal.test.ts b/packages/services/service-analytics/src/__tests__/dotted-measure-refusal.test.ts new file mode 100644 index 0000000000..0dce4b45f3 --- /dev/null +++ b/packages/services/service-analytics/src/__tests__/dotted-measure-refusal.test.ts @@ -0,0 +1,421 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #5918 — a DOTTED measure is refused loudly, naming what the caller wrote. + * + * The fourth mint site of the punctuation #5739 sorted out. That issue narrowed + * the three DIMENSION-shaped mints in `inferCubeFromQuery` from "strip the first + * segment of any dotted member" to "strip the `.` qualifier and nothing + * else", so `owner.region` became a traversal instead of the base table's own + * `region` column. `measures` deliberately kept the blanket strip, on + * measurement: `lookupMember`'s synthetic relation-traversal tier is + * dimension-only (`if (kind === 'dimension')`), so a dotted MEASURE had no + * correct traversal answer to converge on, and minting it verbatim would have + * turned a plain typo (`total.sum`) from a clean 400 into ObjectQL's uncoded + * "cannot evaluate a cross-object measure". + * + * What that left, measured on `origin/main` `01faeb13a` with `crm_account` + * carrying a base column also called `region`: + * + * ``` + * measures: ['owner.region_count_distinct'] + * NativeSQL → SELECT COUNT(DISTINCT region) AS "owner.region_count_distinct" FROM "crm_account" + * ObjectQL → aggregations: [{field:'region', method:'count_distinct', + * alias:'owner.region_count_distinct'}] + * getMeta → crm_account.region_count_distinct + * ``` + * + * No JOIN, no error, and a response column LABELLED with a relation attribute + * whose number came from the base table — the caller cannot see it is wrong. + * Where the object had no same-named column it degraded instead to #4437's + * `400 INVALID_FIELD` naming the STRIPPED tail (`… aggregates field 'score'` + * for a caller who wrote `owner.score_sum`): honest about what reached SQL, + * about a string nobody sent. + * + * **Maintainer ruling, 2026-08-07 (#5918, option 3): refuse the dotted measure + * loudly** — `INVALID_FIELD` / 400 naming the caller's original spelling. + * Explicitly NOT an extension of #5739's ruling: there, refusing would have + * rejected queries that already compiled correctly; here there is no correct + * answer to converge on. Both the typo (`total.sum`) and the genuine traversal + * intent (`owner.amount_sum`) eat this 400, because the two are lexically + * indistinguishable and telling them apart needs field metadata the ad-hoc path + * does not have. A real traversal measure would be a capability with its own + * justification. This TIGHTENS externally visible behaviour on purpose: queries + * that pass silently today start answering 400 — today they aggregate the wrong + * column. + * + * ## Both mint sites, because both were reachable + * + * `ensureCube` mints a Metric from a request spelling in two places, and the + * issue's repro reaches BOTH. The ad-hoc mint (`inferCubeFromQuery`) REGISTERS + * what it infers, so from the second request onwards the same cube arrives at + * the augmentation loop in the "cube exists" branch instead. Measured on + * `origin/main` `01faeb13a`, one service, two queries: + * + * ``` + * ① measures: ['count'] → SELECT COUNT(*) … (warms the registry) + * ② measures: ['owner.region_count_distinct'] → SELECT COUNT(DISTINCT region) AS "owner.region_count_distinct" + * ``` + * + * Refusing at only one site would have closed the cold request and left every + * warm one exactly as it was — i.e. left the reported harm in place on any + * live server. Block 2 is that half; `mintableMeasureKey` is the one rule both + * sites call. + * + * ## Reverse verification, direction predicted BEFORE running + * + * Restoring the blanket strip at either mint site turns blocks 1 and 2 RED, in + * the ordinary direction: every case there asserts a refusal that names the + * caller's spelling in `member`, and the same-named-column cases additionally + * assert that NOTHING was executed (`sqls` / `calls` empty). With the strip + * back, those queries succeed and produce exactly the statement quoted above, + * so they cannot pass by producing nothing. The no-same-named-column case + * (`owner.score_sum`) goes red on `member`/`field` rather than on execution: it + * was refused before the change too, but by #4437's gate, which names `score` in + * `field` and sets no `member` — the inversion trap this file avoids by + * asserting the DIAGNOSTIC, not merely that something threw. + * + * Block 3 fences what the ruling does NOT move — the `.` qualifier, bare + * measures, a cube's own declared vocabulary, and the DIMENSION traversal #5739 + * ruled the other way — and four of its five cases stay green in both + * directions. The fifth does not, and it is worth naming rather than rounding + * off: "a cube's OWN declared dotted measure … still runs" goes RED under the + * full reversal, because the blanket strip does not merely mis-cast REQUEST + * spellings — it also walks straight past a cube's declared dotted key + * (`owner.amount_sum`), mints `amount_sum` over a base column `amount` that + * `crm_account` does not have, and lets #4437 refuse the cube's own authored + * vocabulary. Checking the declared key VERBATIM first is therefore part of this + * change and not a tidy-up: without it the refusal would have taken a working + * authored measure with it. + * + * **Predicted before running: 10 red (blocks 1+2, plus that one block-3 case), + * 4 green. Measured exactly that set.** + */ + +import { describe, it, expect, vi } from 'vitest'; +import type { Cube } from '@objectstack/spec/data'; +import { AnalyticsService } from '../analytics-service.js'; + +const silentLogger = { + info: vi.fn(), + debug: vi.fn(), + warn: vi.fn(), + error: vi.fn(), + child: vi.fn().mockReturnThis(), +} as any; + +/** + * `crm_account`'s real columns. `region` is deliberately one of them — it is the + * base column `owner.region_count_distinct` used to be aggregated over, and the + * reason the defect was silent rather than a `no such column`. There is no + * `score`, which is the other measured tier (#4437's 400 on the stripped tail). + */ +const ACCOUNT_FIELDS = ['id', 'name', 'industry', 'region', 'owner', 'created_at']; + +type Refusal = Error & { + code?: string; + status?: number; + field?: string; + member?: string; + param?: string; + cube?: string; + measure?: string; +}; + +function makeService(opts: { native?: boolean; cubes?: Cube[] } = {}) { + const sqls: string[] = []; + const calls: Array<{ object: string; aggregations?: unknown }> = []; + const service = new AnalyticsService({ + logger: silentLogger, + ...(opts.cubes ? { cubes: opts.cubes } : {}), + queryCapabilities: () => ({ + nativeSql: !!opts.native, + objectqlAggregate: !opts.native, + inMemory: false, + }), + executeAggregate: async (object: string, options: any) => { + calls.push({ object, aggregations: options?.aggregations }); + return [{ count: 5 }]; + }, + executeRawSql: async (_object: string, sql: string) => { + sqls.push(sql); + return [{ count: 1 }]; + }, + isRegisteredObject: (n: string) => n === 'crm_account', + getObjectFieldNames: (n: string) => (n === 'crm_account' ? ACCOUNT_FIELDS : undefined), + } as any); + return { service, sqls, calls }; +} + +/** Run one query on a fresh service and report everything it produced. */ +async function run(query: unknown, opts: { native?: boolean; cubes?: Cube[] } = {}) { + const { service, sqls, calls } = makeService(opts); + let rows: unknown[] | undefined; + let error: Refusal | undefined; + try { + rows = (await service.query(query as never)).rows as unknown[]; + } catch (e) { + error = e as Refusal; + } + const [meta] = await service.getMeta((query as { cube: string }).cube); + const cubePrefix = `${(query as { cube: string }).cube}.`; + return { + rows, + error, + sqls, + calls, + service, + measures: (meta?.measures ?? []).map((m) => m.name.replace(cubePrefix, '')).sort(), + }; +} + +/** The one wire shape every refusal in blocks 1 and 2 must have. */ +function expectDottedMeasureRefusal(error: Refusal | undefined, member: string) { + expect(error).toBeInstanceOf(Error); + // The #4437 family's envelope — one class of mistake, one shape (ADR-0112). + expect(error?.code).toBe('INVALID_FIELD'); + expect(error?.status).toBe(400); + expect(error?.param).toBe('measures'); + expect(error?.cube).toBe('crm_account'); + // The ruling's substance: the caller's ORIGINAL spelling, verbatim. + expect(error?.member).toBe(member); + expect(error?.message).toContain(`Measure '${member}'`); + // …and never the string the old strip produced instead. + expect(error?.message).not.toMatch(/aggregates field/); + // `field` is the source-field gates' diagnostic (a missing base COLUMN). This + // refusal has no column to name — naming one would invent a fact (#5716). + expect(error?.field).toBeUndefined(); +} + +// ── 1. The two shapes the issue measured, on both strategies ───────────────── + +describe('[#5918] a dotted measure is refused, naming the caller\'s spelling', () => { + it('NativeSQL — the silent one: base table has a same-named column', async () => { + const { error, sqls, rows } = await run( + { cube: 'crm_account', measures: ['owner.region_count_distinct'] }, + { native: true }, + ); + + expectDottedMeasureRefusal(error, 'owner.region_count_distinct'); + // The whole point: it never became `COUNT(DISTINCT region)` on the base table. + expect(sqls).toEqual([]); + expect(rows).toBeUndefined(); + }); + + it('ObjectQL — the same silent one, refused identically', async () => { + // Refusing at the MINT is what makes the two strategies agree for free: the + // verdict is reached before a strategy is even resolved. + const { error, calls } = await run({ + cube: 'crm_account', + measures: ['owner.region_count_distinct'], + }); + + expectDottedMeasureRefusal(error, 'owner.region_count_distinct'); + expect(calls).toEqual([]); + }); + + it('the honest-but-misnamed one: base table has NO same-named column', async () => { + // Was #4437's `400 INVALID_FIELD` with `field: 'score'` — true of what + // reached SQL, about a string the caller never wrote. Now it names + // `owner.score_sum`, like its silent sibling above, so the two tiers of the + // SAME mistake stop giving two different answers. + const { error, sqls } = await run( + { cube: 'crm_account', measures: ['owner.score_sum'] }, + { native: true }, + ); + + expectDottedMeasureRefusal(error, 'owner.score_sum'); + expect(error?.message).not.toContain("'score'"); + expect(sqls).toEqual([]); + }); + + it('the TYPO spelling eats the same 400 — deliberately, not incidentally', async () => { + // `total.sum` is a `total_sum` slip, not a traversal, and the ruling covers + // it on purpose: the two are lexically indistinguishable here, and telling + // them apart would need field metadata this path does not have. + const { error, sqls } = await run( + { cube: 'crm_account', measures: ['total.sum'] }, + { native: true }, + ); + + expectDottedMeasureRefusal(error, 'total.sum'); + expect(sqls).toEqual([]); + }); + + it('refuses `/analytics/sql` too — a dry run must not hand back the wrong SQL', async () => { + const { service } = makeService({ native: true }); + + const error = await service + .generateSql({ cube: 'crm_account', measures: ['owner.region_count_distinct'] } as any) + .then(() => undefined, (e) => e as Refusal); + + expectDottedMeasureRefusal(error, 'owner.region_count_distinct'); + }); + + it('leaves the registry as it found it — a retry gets the same answer', async () => { + // Same rule the #3867 and #4437 gates keep: a rejected query must leave no + // trace, or the retry finds a "registered" cube carrying the bogus measure + // and sails straight into SQL. + const { service, sqls } = makeService({ native: true }); + const q = { cube: 'crm_account', measures: ['owner.region_count_distinct'] } as any; + + await expect(service.query(q)).rejects.toThrow(); + expect(service.cubeRegistry.get('crm_account')).toBeUndefined(); + + const second = await service.query(q).then( + () => { throw new Error('expected the retry to be refused too'); }, + (e) => e as Refusal, + ); + expectDottedMeasureRefusal(second, 'owner.region_count_distinct'); + expect(sqls).toEqual([]); + }); +}); + +// ── 2. The second mint site: a WARM registry ───────────────────────────────── + +describe('[#5918] the warm registry gets the same answer as the cold one', () => { + it('NativeSQL — a first query registers the inferred cube; the second is still refused', async () => { + // The ad-hoc path registers what it infers, so request #2 arrives at + // `ensureCube`'s augmentation loop instead of `inferCubeFromQuery`. Before + // the ruling that loop still blanket-stripped, so a live server silently + // aggregated the wrong column from the second request onwards. + const { service, sqls } = makeService({ native: true }); + + await service.query({ cube: 'crm_account', measures: ['count'] } as any); + expect(sqls).toEqual(['SELECT COUNT(*) AS "count" FROM "crm_account"']); + expect(service.cubeRegistry.get('crm_account')).toBeDefined(); + + const error = await service + .query({ cube: 'crm_account', measures: ['owner.region_count_distinct'] } as any) + .then(() => undefined, (e) => e as Refusal); + + expectDottedMeasureRefusal(error, 'owner.region_count_distinct'); + // Nothing new executed… + expect(sqls).toEqual(['SELECT COUNT(*) AS "count" FROM "crm_account"']); + // …and the registered cube was not augmented with the bogus measure either. + expect(Object.keys(service.cubeRegistry.get('crm_account')!.measures)).toEqual(['count']); + }); + + it('ObjectQL — same', async () => { + const { service, calls } = makeService(); + + await service.query({ cube: 'crm_account', measures: ['count'] } as any); + expect(calls).toHaveLength(1); + + const error = await service + .query({ cube: 'crm_account', measures: ['owner.region_count_distinct'] } as any) + .then(() => undefined, (e) => e as Refusal); + + expectDottedMeasureRefusal(error, 'owner.region_count_distinct'); + expect(calls).toHaveLength(1); + }); + + it('an AUTHORED cube mints measures through the same rule', async () => { + // The augmentation loop serves authored cubes too, and the dotted spelling + // was never a traversal there either: it minted `balance_sum` over the BASE + // table, so nothing that worked is being taken away. + const authored: Cube = { + name: 'crm_account', + title: 'Accounts', + sql: 'crm_account', + measures: { count: { name: 'count', label: 'Count', type: 'count', sql: '*' } }, + dimensions: {}, + public: false, + }; + const { error, sqls } = await run( + { cube: 'crm_account', measures: ['owner.region_count_distinct'] }, + { native: true, cubes: [authored] }, + ); + + expectDottedMeasureRefusal(error, 'owner.region_count_distinct'); + expect(sqls).toEqual([]); + }); +}); + +// ── 3. What the ruling does NOT move ───────────────────────────────────────── + +describe('[#5918] the surfaces the ruling leaves alone', () => { + it('the `.` qualifier is still the one dot a measure may carry', async () => { + // `getMeta` hands members out cube-prefixed and callers echo them back, so + // this prefix is noise — stripping it is right, and is all that is stripped. + const { error, sqls, measures } = await run( + { cube: 'crm_account', measures: ['crm_account.region_count_distinct'] }, + { native: true }, + ); + + expect(error).toBeUndefined(); + expect(measures).toContain('region_count_distinct'); + expect(sqls[0]).toBe( + 'SELECT COUNT(DISTINCT region) AS "crm_account.region_count_distinct" FROM "crm_account"', + ); + }); + + it('bare measures are untouched — inferred suffixes, `count`, engine columns', async () => { + const { service, sqls } = makeService({ native: true }); + + await service.query({ cube: 'crm_account', measures: ['count'] } as any); + await service.query({ + cube: 'crm_account', + measures: ['region_count_distinct', 'created_at_max'], + } as any); + + expect(sqls).toEqual([ + 'SELECT COUNT(*) AS "count" FROM "crm_account"', + 'SELECT COUNT(DISTINCT region) AS "region_count_distinct", MAX(created_at) AS "created_at_max" ' + + 'FROM "crm_account"', + ]); + }); + + it('a cube\'s OWN declared dotted measure is authored, not minted — and still runs', async () => { + // The refusal is about INVENTING a Metric from a request spelling. A cube + // that declares the dotted key authored it deliberately, `lookupMember` + // resolves it by direct hit, and the JOIN machinery serves it — which is + // also the escape hatch the refusal message points callers at. + const authored: Cube = { + name: 'crm_account', + title: 'Accounts', + sql: 'crm_account', + measures: { + count: { name: 'count', label: 'Count', type: 'count', sql: '*' }, + 'owner.amount_sum': { + name: 'owner.amount_sum', label: 'Owner amount', type: 'sum', sql: 'owner.amount', + }, + }, + dimensions: {}, + public: false, + }; + const { error, sqls } = await run( + { cube: 'crm_account', measures: ['owner.amount_sum'] }, + { native: true, cubes: [authored] }, + ); + + expect(error).toBeUndefined(); + expect(sqls[0]).toContain('SUM("owner"."amount")'); + expect(sqls[0]).toContain('LEFT JOIN "owner"'); + }); + + it('DIMENSIONS still traverse — #5739 ruled the other way, and stays ruled', async () => { + // The two rulings are different because the two faces are: a dotted + // dimension HAS a correct traversal answer to converge on, and converges. + const { error, sqls } = await run( + { cube: 'crm_account', measures: ['count'], dimensions: ['owner.region'] }, + { native: true }, + ); + + expect(error).toBeUndefined(); + expect(sqls[0]).toContain('LEFT JOIN "owner" ON "crm_account"."owner" = "owner"."id"'); + expect(sqls[0]).toContain('"owner"."region"'); + }); + + it('an unknown CUBE is still a 404 before any measure is judged', async () => { + // Precedence, not politeness: the cube must exist before its measures can be + // wrong about anything (#3867). + const { error } = await run( + { cube: 'not_an_object', measures: ['owner.region_count_distinct'] }, + { native: true }, + ); + + expect(error?.code).toBe('CUBE_NOT_FOUND'); + expect(error?.status).toBe(404); + }); +}); diff --git a/packages/services/service-analytics/src/__tests__/infer-cube-relation-traversal.test.ts b/packages/services/service-analytics/src/__tests__/infer-cube-relation-traversal.test.ts index 582e5c5a60..25421b14cb 100644 --- a/packages/services/service-analytics/src/__tests__/infer-cube-relation-traversal.test.ts +++ b/packages/services/service-analytics/src/__tests__/infer-cube-relation-traversal.test.ts @@ -52,6 +52,17 @@ * green; measured exactly that set** — plus, in the same run, the three flipped * cases in `infer-cube-where-spelling-parity.test.ts` and the one in * `where-source-field-gate.test.ts`, for 14 red across the package. + * + * ## [#5918] One case in block 5 changed hands + * + * The dotted MEASURE this file pinned as "#4437 refuses it, naming the strip" + * is now refused at the MINT, naming the caller's own spelling — the fourth + * mint site #5739 measured and deliberately left alone, ruled on 2026-08-07 in + * its own right (measures have no traversal tier to converge on, so a refusal, + * not a verbatim mint). Block 5's floor is untouched: the query is refused, + * with `INVALID_FIELD` / 400, and never becomes SQL. Both directions of the + * reverse verification above are unaffected — restoring the blanket + * DIMENSION strip leaves that case green, as this block promises. */ import { describe, it, expect, vi } from 'vitest'; @@ -429,12 +440,20 @@ describe('[#5739] the source-field gates keep every rejection they already made' expect(error?.message).toMatch(/constrains field 'bogus_col'/); }); - it('still refuses a dotted MEASURE through #4437, naming what it stripped to', async () => { - // Measures deliberately keep the blanket strip — `lookupMember`'s synthetic - // traversal tier is dimension-only, so a dotted measure has no traversal to - // converge with, and minting it verbatim would trade this `400 INVALID_FIELD` - // for ObjectQL's uncoded "cross-object measure" throw. See the mint loop, and - // #5918 for the residue that leaves on the `measures` key. + it('[#5918] still refuses a dotted MEASURE — now naming the caller\'s spelling, not the strip', async () => { + // Rewritten by #5918, which is the one case in this file whose ANSWERING + // LAYER moved. When #5739 landed, measures kept the blanket strip (their + // traversal tier is dimension-only, so there was no correct answer to + // converge on) and this case pinned #4437's verdict on the stripped tail: + // `field: 'score'`. The residue #5739 filed as #5918 was the sibling + // spelling whose tail DOES exist — `owner.region_count_distinct` silently + // aggregating the base `region`. The 2026-08-07 ruling refuses the dotted + // measure at the mint instead, so both spellings now answer identically and + // name what the caller wrote. + // + // The floor this block is about is unchanged and is what stays asserted: a + // dotted measure is REFUSED, with `INVALID_FIELD` / 400, and never becomes + // SQL. Only the diagnostic moved — from `field` (this gate's) to `member`. const { error, sqls } = await run( { cube: 'crm_account', measures: ['owner.score_sum'] }, { native: true }, @@ -442,7 +461,11 @@ describe('[#5739] the source-field gates keep every rejection they already made' expect(error?.code).toBe('INVALID_FIELD'); expect(error?.status).toBe(400); - expect(error?.field).toBe('score'); + expect((error as { member?: string } | undefined)?.member).toBe('owner.score_sum'); + expect(error?.message).toMatch(/Measure 'owner\.score_sum'/); + // Not the source-field gate's verdict any more — it names a base column in + // `field`, and `score` is not a column the caller asked about. + expect(error?.field).toBeUndefined(); expect(sqls).toEqual([]); }); diff --git a/packages/services/service-analytics/src/__tests__/measure-source-field-gate.test.ts b/packages/services/service-analytics/src/__tests__/measure-source-field-gate.test.ts index ac007a120a..6866d9cc4b 100644 --- a/packages/services/service-analytics/src/__tests__/measure-source-field-gate.test.ts +++ b/packages/services/service-analytics/src/__tests__/measure-source-field-gate.test.ts @@ -20,6 +20,16 @@ * these cases pin the analytics half of that answer, and pin the tiering that * keeps it from over-reaching (ADR-0112: a driver error class is never the * `error.code` for a caller-shaped mistake). + * + * [#5918] The DOTTED spelling has since left this gate. A dotted measure is + * refused at the MINT — `mintableMeasureKey`, maintainer ruling 2026-08-07 — + * because stripping the prefix answered the wrong question in both directions: + * it named `sum` to a caller who wrote `total.sum`, and where the stripped tail + * happened to be a real column it produced no verdict at all, silently + * aggregating the base table under a relation-shaped label. Same code, same + * status, same request key; different layer, and the message now names what the + * caller actually sent. The case below pins that hand-off from this side; the + * rule's own file is `dotted-measure-refusal.test.ts`. */ import { describe, it, expect, vi } from 'vitest'; @@ -105,14 +115,40 @@ describe('#4437 — measure source-field gate', () => { expect(err.message).not.toMatch(/Valid measures:[^.]*ghost_sum/); }); - it('refuses the dotted spelling the same way, naming what it stripped to', async () => { - // `total.sum` prefix-strips to `sum`, which infers `SUM(sum)` — a column - // named `sum` that does not exist. Same 500 pre-fix, same 400 now. + it('[#5918] refuses the dotted spelling by NAMING IT, not by naming what it stripped to', async () => { + // Rewritten from "…naming what it stripped to". Until #5918 `total.sum` + // prefix-stripped to `sum`, inferred `SUM(sum)`, and THIS gate refused it + // as a missing column: `{field: 'sum', measure: 'total.sum'}` — honest + // about what reached SQL, but naming a string the caller never wrote, and + // silently right for the sibling spelling whose tail DID exist + // (`owner.region_count_distinct` over a real `region` column — #5918's + // measured defect). The maintainer ruled on 2026-08-07 that a dotted + // measure is refused at the MINT instead, naming the caller's own + // spelling; `dotted-measure-refusal.test.ts` is that rule's own file. + // + // What this case still pins, and why it belongs in #4437's file: the + // dotted spelling is refused with the SAME wire shape (`INVALID_FIELD` / + // 400 / `param: 'measures'`), and it still never reaches the driver. What + // changed is WHICH layer answers — so the verdict no longer carries this + // gate's `field`, which is exactly how the two are told apart (the same + // distinction #5716 drew for the strategy's own refusals). const { service, aggregated } = makeService(); - await expect( - service.query({ cube: 'showcase_invoice', measures: ['total.sum'] } as any), - ).rejects.toMatchObject({ ...INVALID_FIELD, field: 'sum', measure: 'total.sum' }); + const err = await service.query({ cube: 'showcase_invoice', measures: ['total.sum'] } as any).then( + () => { throw new Error('expected the dotted measure to be refused'); }, + (e) => e as Error & { code?: string; status?: number; field?: string; member?: string; param?: string }, + ); + + expect(err.code).toBe('INVALID_FIELD'); + expect(err.status).toBe(400); + expect(err.param).toBe('measures'); + // The caller's own spelling, verbatim. + expect(err.member).toBe('total.sum'); + expect(err.message).toMatch(/Measure 'total\.sum'/); + // Not this gate's verdict: it names a missing base COLUMN in `field`, and + // `sum` is not a column anyone asked about. + expect(err.field).toBeUndefined(); + expect(err.message).not.toMatch(/aggregates field 'sum'/); expect(aggregated).toEqual([]); }); diff --git a/packages/services/service-analytics/src/analytics-service.ts b/packages/services/service-analytics/src/analytics-service.ts index 1a063a9fa4..74b8052c3c 100644 --- a/packages/services/service-analytics/src/analytics-service.ts +++ b/packages/services/service-analytics/src/analytics-service.ts @@ -34,6 +34,11 @@ import { type DimensionLabelDeps, } from './dimension-labels.js'; import { evaluateAnalyticsQueryOverRows } from './preview-evaluator.js'; +// [#5918] The measure mint refuses a dotted member through the SAME constructor +// the strategies' member-level refusals use — `INVALID_FIELD` / 400, naming the +// member as the request spelled it (see `dataset-refusal.ts`'s header for why +// that code and not `DATASET_INVALID`). +import { invalidMemberError } from './dataset-refusal.js'; /** * Analytics result augmented with drill-through metadata (ADR-0021 D2; see @@ -1244,10 +1249,29 @@ export class AnalyticsService implements IAnalyticsService { // Cube exists — check for unknown measures referenced by the query and // augment the cube with suffix-inferred Metric definitions so callers // that pass `_sum` / `_avg` etc. get the right aggregation. - const stripPrefix = (m: string) => (m.includes('.') ? m.split('.').slice(1).join('.') : m); + // + // [#5918] This is the SECOND measure mint, and it judges a dotted spelling + // exactly as the ad-hoc one does — `mintableMeasureKey` owns the rule. It + // has to: the ad-hoc path REGISTERS what it infers, so from the second + // request onwards a cube minted moments ago by `inferCubeFromQuery` is + // "registered" and arrives here. Measured on `origin/main` `01faeb13a`, + // one service, two queries: + // + // ① measures: ['count'] → SELECT COUNT(*) … (warms the registry) + // ② measures: ['owner.region_count_distinct'] → SELECT COUNT(DISTINCT region) AS "owner.region_count_distinct" + // + // i.e. the silent wrong column #5918 reports, reached through this loop + // instead of that one. Refusing in only one of the two would have closed the + // cold request and left every warm one exactly as it was. + // + // A measure the cube DECLARES is never minted, so it never reaches the + // rule — including a declared DOTTED key, which `lookupMember` resolves by + // direct hit and which this loop must therefore check for verbatim FIRST or + // it would refuse a cube's own authored vocabulary. const extraMeasures: Record = {}; for (const m of query.measures || []) { - const key = stripPrefix(m); + if (cube.measures[m] || extraMeasures[m]) continue; + const key = mintableMeasureKey(m, name); if (cube.measures[key] || extraMeasures[key]) continue; extraMeasures[key] = inferMeasure(key); } @@ -1311,6 +1335,13 @@ export class AnalyticsService implements IAnalyticsService { * the data path's `resolveQueryFields`: they are engine-assigned rather than * declared, and a gate stricter than the engine it guards would reject * queries that used to work. + * + * [#5918] Its `stripPrefix` below is deliberately NOT narrowed the way the two + * MINTS were. This is a RESOLVER — it mirrors `lookupMember`'s tiers to answer + * "which Metric will the strategy read", and that tier order did not change. + * What changed is what can reach it: a dotted measure is now either a + * `.` qualifier or a key the cube itself declares, because every other + * dotted spelling is refused at the mint before this gate runs. */ private assertMeasureFields(query: AnalyticsQuery, cube: Cube, declaredMeasures: string[]): void { const probe = this.getObjectFieldNames; @@ -1684,9 +1715,11 @@ export class AnalyticsService implements IAnalyticsService { // the array `where` spelling has compiled all along, and the two spellings // converge instead of disagreeing. Maintainer ruling, 2026-08-06 (#5739). // - // Scope, measured rather than assumed: this governs the DIMENSION-shaped - // mints (`dimensions`, the `where`'s field keys, `timeDimensions`) and NOT - // `measures`, which keeps the old blanket strip below. See that loop. + // Scope: this governs the DIMENSION-shaped mints (`dimensions`, the + // `where`'s field keys, `timeDimensions`), which SERVE a traversal. The + // measure mint applies the same qualifier rule but ends the other way — + // `mintableMeasureKey` refuses a non-qualifier dot outright, because the + // measure side has no traversal to serve (#5918, and the loop below). const stripCubeQualifier = (m: string): string => { const dot = m.indexOf('.'); if (dot < 0) return m; @@ -1697,20 +1730,18 @@ export class AnalyticsService implements IAnalyticsService { measures.count = { name: 'count', label: 'Count', type: 'count', sql: '*' }; for (const m of query.measures || []) { - // [#5739] MEASURES keep the blanket strip, deliberately and on measurement. - // `lookupMember`'s synthetic relation-traversal tier is DIMENSION-ONLY - // (`if (kind === 'dimension')`), so a dotted measure has no traversal - // answer to converge with — minting `measures['total.sum']` verbatim does - // not join anything, it only re-routes the member into ObjectQL's - // "cannot evaluate a cross-object measure" throw, which carries no - // `code`/`status`. Measured on this tree: `measures: ['total.sum']` (a - // `total_sum` typo, not a traversal) would go from #4437's - // `400 INVALID_FIELD` naming `sum` to that uncoded 5xx-class error. A - // worse envelope and a wrong diagnosis, for a spelling that is not what - // this issue is about — so the strip stays until a dotted MEASURE is ruled - // on in its own right. Its own residue (`owner.region_count_distinct` - // silently aggregating the BASE `region`, measured) is filed as #5918. - const key = m.includes('.') ? m.split('.').slice(1).join('.') : m; + // [#5918] MEASURES no longer take the blanket strip #5739 left them with. + // That strip cast `owner.region_count_distinct` onto the BASE `region` + // column — silently where the object had one, and as a #4437 400 naming + // the stripped tail where it did not. Neither is the caller's query. + // + // The ruling here is NOT #5739's (mint the traversal verbatim): measures + // have no traversal tier to converge on, so there is nothing correct to + // converge to. It is a loud refusal instead — see `mintableMeasureKey`, + // which owns the rule and the envelope, and which the augmentation mint in + // `ensureCube` shares so the same spelling gets the same answer on a warm + // registry. Maintainer ruling, 2026-08-07 (#5918). + const key = mintableMeasureKey(m, cubeName); if (measures[key]) continue; const inferred = inferMeasure(key); measures[key] = inferred; @@ -1814,6 +1845,81 @@ export class AnalyticsService implements IAnalyticsService { } } +/** + * [#5918] The `cube.measures` KEY a request's `measures` entry may be MINTED + * under — or a loud refusal when the entry is a dotted member. + * + * Two mint sites feed {@link inferMeasure}, and both go through here: the + * ad-hoc mint in {@link AnalyticsService.inferCubeFromQuery} (no cube + * registered) and the suffix-augmentation loop in + * {@link AnalyticsService.ensureCube} (a cube exists but does not declare the + * measure). They are the same act — inventing a Metric out of a request + * spelling — so they must judge the spelling the same way. + * + * ## What is refused, and why it is a refusal rather than a traversal + * + * A `.` QUALIFIER is stripped, exactly as `inferCubeFromQuery`'s + * `stripCubeQualifier` does for the dimension-shaped mints (#5739): `getMeta` + * hands members out cube-prefixed and + * callers echo them back, so that prefix is noise. **Every other dot is + * refused.** The predecessor dropped the first segment of ANY dotted member, + * which meant `owner.region_count_distinct` minted + * `measures.region_count_distinct = {type:'count_distinct', sql:'region'}` — the + * BASE table's own `region` column — and then: + * + * ``` + * NativeSQL → SELECT COUNT(DISTINCT region) AS "owner.region_count_distinct" FROM "crm_account" + * ObjectQL → aggregations: [{field:'region', method:'count_distinct', alias:'owner.region_count_distinct'}] + * ``` + * + * No JOIN, no error, and a response column LABELLED with a relation attribute + * whose number came from the base table — a wrong answer the caller cannot see + * (measured on `origin/main` `01faeb13a`). Where the base object had no + * same-named column it degraded instead to #4437's `400 INVALID_FIELD` naming + * the STRIPPED tail (`Measure 'owner.score_sum' … aggregates field 'score'`) — + * honest about what reached SQL, but naming a string the caller never wrote. + * + * #5739 fixed the same punctuation on the three DIMENSION-shaped mints by + * minting the traversal verbatim, and that ruling deliberately does NOT carry + * over here: `lookupMember`'s synthetic relation-traversal tier is + * dimension-only (`if (kind === 'dimension')`), so a dotted measure has no + * correct traversal answer to converge on. Minting it verbatim would only + * re-route it into ObjectQL's `cannot evaluate a cross-object measure` — which + * would be the wrong diagnosis for a plain typo like `total.sum`. Maintainer + * ruling, 2026-08-07 (#5918, option 3): refuse the dotted measure LOUDLY, with + * the caller's own spelling in the envelope. A genuine traversal measure + * (`SUM("owner"."amount")` + LEFT JOIN) would be a capability with its own + * justification, not a side effect of a strip. + * + * The refusal deliberately reaches BOTH the typo (`total.sum`) and the genuine + * traversal intent (`owner.amount_sum`): the two are lexically indistinguishable + * on this path, and telling them apart would need field metadata the ad-hoc + * path does not have (`getObjectFieldNames` answers names, not types or + * relation targets). One honest 400 for both beats a metadata capability nobody + * has asked for. + * + * A measure the cube DECLARES under a dotted key is not this function's + * business — it was authored, not minted, and `lookupMember` resolves it by + * direct hit. Both call sites check that first. + */ +function mintableMeasureKey(member: string, cubeName: string): string { + const dot = member.indexOf('.'); + if (dot < 0) return member; + if (member.slice(0, dot) === cubeName) return member.slice(dot + 1); + + throw invalidMemberError( + `[Analytics] Measure '${member}' on cube '${cubeName}' is a DOTTED member, and ` + + `measures do not traverse relationships — only dimensions do — so there is no ` + + `related column for this to aggregate. Until #5918 the prefix was silently ` + + `dropped, so the aggregate ran against '${cubeName}' itself while the result ` + + `column kept the label '${member}'. Aggregate one of the object's OWN fields ` + + `instead ('_sum' / '_avg' / '_min' / '_max' / '_count_distinct'), or ` + + `declare a Cube whose measure names the related column in its own 'sql'. The ` + + `only dot a measure may carry is the '${cubeName}.' qualifier.`, + { member, param: 'measures', cube: cubeName }, + ); +} + /** * Infer a Metric definition from a measure key name. *