diff --git a/.changeset/tidy-cubes-count-their-columns.md b/.changeset/tidy-cubes-count-their-columns.md new file mode 100644 index 0000000000..f419deb259 --- /dev/null +++ b/.changeset/tidy-cubes-count-their-columns.md @@ -0,0 +1,14 @@ +--- +"@objectstack/service-analytics": patch +--- + +Analytics measures are now compiled from everything they declare — `aggregate`, `field` **and** `filter` — on both the dashboard path and `POST /api/v1/analytics/query`. + +**Reported figures change, and the new ones are the declared ones.** Two corrections, both of which move numbers a dashboard or an API consumer is already reading: + +- A measure written `{ aggregate: 'count', field: 'some_column' }` used to compile to `COUNT(*)` and count **rows**. It now compiles to `COUNT("some_column")` and counts **non-null values**. Any such measure will report the same number as before or a **smaller** one, and a rate built on top of it (a numerator over a total) will drop accordingly — a "100%" tile whose column was mostly empty was reading its own denominator. +- `POST /api/v1/analytics/query` used to drop every per-measure `filter`, and the dataset's definition-level `filter` with it, returning unfiltered aggregates under the author's measure names. It now applies both, so the endpoint answers what the dashboard already answered for the same cube. Figures pulled through the API — agent tools, exports, downstream reports — will move to the filtered values; a measure declaring `filter: { stage: 'closed_won' }` stops counting every row. + +Measures that declare no `field` still compile to `COUNT(*)`, and a cube that is not a compiled dataset (an inferred or manifest cube) emits byte-for-byte the statement it did before. Measure filters lower to portable `CASE WHEN` conditional aggregates rather than `FILTER (WHERE …)`, which MySQL does not have. + +If a saved figure or a screenshot disagrees with what the platform now reports, the new number is the one the metadata declares. diff --git a/packages/services/service-analytics/src/__tests__/measure-field-and-filter-compilation.test.ts b/packages/services/service-analytics/src/__tests__/measure-field-and-filter-compilation.test.ts new file mode 100644 index 0000000000..e001d5190c --- /dev/null +++ b/packages/services/service-analytics/src/__tests__/measure-field-and-filter-compilation.test.ts @@ -0,0 +1,385 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * A measure is compiled from EVERYTHING it declares — `aggregate`, `field` and + * `filter` — on both doors (#10298). + * + * Two defects, one gap. A dataset measure carries three declarations; the + * compiled SQL used only the first: + * + * 1. `{ aggregate: 'count', field: 'resolved_by_article' }` emitted `COUNT(*)`. + * The wrapper table took the column and threw it away, so a measure that + * asks "how many cases carry an article" counted every case it was handed. + * A deflection rate built as `kb_resolved_count / closed_count` therefore + * read 100% where the truth was 12.5% — with 8 and 8 printed beside it, and + * the pivot underneath grouping seven of those eight under a null article. + * + * 2. `/api/v1/analytics/query` dropped every per-measure `filter`, and the + * dataset's definition-level `filter` with it. That door addresses the + * REGISTERED CUBE directly; the filters live beside the cube, in the + * dataset registry, and only `DatasetExecutor` — the dashboard's door — + * ever read them. So one cube and one set of measure names answered two + * different numbers depending on which door the caller came in, and the API + * door was the one that silently answered wrong. + * + * # Why the pins are on the SQL, not only on the number + * + * The response carries the compiled statement, and that is what this card is + * about. A number-only assertion passes on any arithmetic that happens to agree + * on one fixture — `COUNT(*)` and `COUNT(col)` agree on any fixture with no + * nulls, which is most of them. So the compiled shape is pinned directly: + * `COUNT()` for a `count` that names a field, and one conditional + * aggregate per filtered measure. + * + * `CASE WHEN` rather than SQL-standard `FILTER (WHERE …)`: `FILTER` is + * Postgres and SQLite ≥ 3.30 only, and this strategy compiles one statement for + * whichever SQL driver owns the object — MySQL among them. + * + * # And the numbers, on a real database + * + * The last block runs both doors against a real SQLite (`sql.js`, the pure-WASM + * engine `driver-sql` itself falls back to) over the issue's own ground truth: + * 24 opportunities, 8 won, 5 lost, won revenue 1,290,000. Before the fix the + * API door answered 24 / 24 / 24 / 5,632,500 — the shape assertions above + * cannot tell that apart from a fix that merely emits plausible SQL, so the two + * doors are made to agree on rows that really exist. + */ + +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import type { AnalyticsQuery } from '@objectstack/spec/contracts'; +import { DatasetSchema, type Dataset } from '@objectstack/spec/ui'; +import { AnalyticsService } from '../analytics-service.js'; + +// ── datasets ──────────────────────────────────────────────────────────────── + +/** The card's `case_metrics`: a `count` that names a field, both measures scoped. */ +const CASE_METRICS: Dataset = DatasetSchema.parse({ + name: 'case_metrics', + label: 'Case Metrics', + object: 'crm_case', + dimensions: [], + measures: [ + { name: 'case_count', label: 'Cases', aggregate: 'count' }, + { name: 'closed_count', label: 'Closed Cases', aggregate: 'count', filter: { is_closed: true } }, + { + name: 'kb_resolved_count', label: 'Resolved by KB', aggregate: 'count', + field: 'resolved_by_article', filter: { is_closed: true }, + }, + // The same `count(field)` WITHOUT a filter — the plain `COUNT(col)` form. + { name: 'article_count', label: 'With an article', aggregate: 'count', field: 'resolved_by_article' }, + ], +}) as Dataset; + +/** The card's `opportunity_metrics`, measure for measure. */ +const OPPORTUNITY_METRICS: Dataset = DatasetSchema.parse({ + name: 'opportunity_metrics', + label: 'Opportunity Metrics', + object: 'crm_opportunity', + dimensions: [ + { name: 'stage', label: 'Stage', field: 'stage', type: 'string' }, + // Grouped agreement is asserted on OWNER, never on `stage`: grouping by the + // very column a measure filters on makes the filtered and unfiltered + // aggregates COINCIDE inside the matching group, so an assertion there + // passes with the filter dropped. Measured — that is exactly what the first + // draft of the grouped test did, and the ablation caught it, not the fix. + { name: 'owner', label: 'Owner', field: 'owner', type: 'string' }, + ], + measures: [ + { name: 'opp_count', label: 'Opportunities', aggregate: 'count' }, + { name: 'won_count', label: 'Won Deals', aggregate: 'count', filter: { stage: 'closed_won' } }, + { name: 'lost_count', label: 'Lost Deals', aggregate: 'count', filter: { stage: 'closed_lost' } }, + { + name: 'won_amount', label: 'Won Revenue', aggregate: 'sum', field: 'amount', + filter: { stage: 'closed_won' }, + }, + ], +}) as Dataset; + +/** A dataset carrying a definition-level `filter` — its intrinsic scope. */ +const SCOPED_METRICS: Dataset = DatasetSchema.parse({ + name: 'scoped_metrics', + label: 'Scoped Metrics', + object: 'crm_opportunity', + filter: { is_deleted: false }, + dimensions: [], + measures: [ + { name: 'opp_count', label: 'Opportunities', aggregate: 'count' }, + { name: 'won_count', label: 'Won Deals', aggregate: 'count', filter: { stage: 'closed_won' } }, + ], +}) as Dataset; + +/** + * A dataset that JOINS, so base columns come out table-qualified — the exact + * shape the card reported from a real deployment (`SUM("crm_opportunity"."amount")`). + * A single-object cube keeps bare columns by design, so both spellings are + * pinned rather than one of them being mistaken for the rule. + */ +const JOINED_METRICS: Dataset = DatasetSchema.parse({ + name: 'joined_case_metrics', + label: 'Joined Case Metrics', + object: 'crm_case', + include: ['account'], + dimensions: [{ name: 'industry', label: 'Industry', field: 'account.industry', type: 'string' }], + measures: [ + { + name: 'kb_resolved_count', label: 'Resolved by KB', aggregate: 'count', + field: 'resolved_by_article', filter: { is_closed: true }, + }, + ], +}) as Dataset; + +/** A service with the native-SQL capability and no executor — SQL only. */ +const sqlOnlyService = (...datasets: Dataset[]): AnalyticsService => { + const svc = new AnalyticsService({ + debugSql: true, + queryCapabilities: () => ({ nativeSql: true, objectqlAggregate: false, inMemory: false }), + executeRawSql: async () => [], + }); + for (const d of datasets) svc.registerDataset(d); + return svc; +}; + +const sqlFor = async (svc: AnalyticsService, query: AnalyticsQuery) => + svc.generateSql(query); + +// ── 1. `count` compiles its `field` ───────────────────────────────────────── + +describe('[#10298] `aggregate: \'count\'` compiles the `field` it declares', () => { + it('emits COUNT() for a count that names a field', async () => { + const svc = sqlOnlyService(CASE_METRICS); + const { sql } = await sqlFor(svc, { cube: 'case_metrics', measures: ['article_count'] }); + expect(sql).toContain('COUNT(resolved_by_article) AS "article_count"'); + // The defect's signature, gone: the star counted rows, not values. + expect(sql).not.toContain('COUNT(*) AS "article_count"'); + }); + + it('table-qualifies the counted column when the cube can join', async () => { + const svc = sqlOnlyService(JOINED_METRICS); + const { sql } = await sqlFor(svc, { + cube: 'joined_case_metrics', measures: ['kb_resolved_count'], dimensions: ['industry'], + }); + expect(sql).toContain('THEN "crm_case"."resolved_by_article" END)'); + }); + + it('keeps COUNT(*) for a count that names NO field', async () => { + const svc = sqlOnlyService(CASE_METRICS); + const { sql } = await sqlFor(svc, { cube: 'case_metrics', measures: ['case_count'] }); + // `sql: '*'` IS the compiler's "no field declared" spelling; a star must + // keep counting rows, or every unqualified `count` measure changes meaning. + expect(sql).toContain('COUNT(*) AS "case_count"'); + }); +}); + +// ── 2. per-measure filters reach the strict wrapper ───────────────────────── + +describe('[#10298] `/api/v1/analytics/query` compiles every per-measure `filter`', () => { + it('emits one conditional aggregate per filtered measure, and binds its comparand', async () => { + const svc = sqlOnlyService(OPPORTUNITY_METRICS); + const { sql, params } = await sqlFor(svc, { + cube: 'opportunity_metrics', + measures: ['opp_count', 'won_count', 'lost_count', 'won_amount'], + }); + + // The unfiltered measure is untouched… + expect(sql).toContain('COUNT(*) AS "opp_count"'); + // …and each filtered one carries its own predicate. + expect(sql).toContain('COUNT(CASE WHEN stage = $1 THEN 1 END) AS "won_count"'); + expect(sql).toContain('COUNT(CASE WHEN stage = $2 THEN 1 END) AS "lost_count"'); + expect(sql).toContain('SUM(CASE WHEN stage = $3 THEN amount END) AS "won_amount"'); + + // Comparands are BOUND, in the order their placeholders appear. The SELECT + // list precedes the WHERE clause and `$n` is positional, so a filter + // compiled anywhere but inside the SELECT loop would misalign the binds. + expect(params).toEqual(['closed_won', 'closed_lost', 'closed_won']); + + // The defect's exact signature from the card: three identical row counts. + expect(sql).not.toContain('COUNT(*) AS "won_count"'); + expect(sql).not.toContain('COUNT(*) AS "lost_count"'); + expect(sql).not.toContain('SUM(amount) AS "won_amount"'); + }); + + it('composes a measure `filter` with a measure `field` — the card\'s `kb_resolved_count`', async () => { + const svc = sqlOnlyService(CASE_METRICS); + const { sql, params } = await sqlFor(svc, { + cube: 'case_metrics', measures: ['closed_count', 'kb_resolved_count'], + }); + // Both scoped to closed cases; only the second counts a COLUMN's values. + expect(sql).toContain('COUNT(CASE WHEN is_closed = $1 THEN 1 END) AS "closed_count"'); + expect(sql).toContain('COUNT(CASE WHEN is_closed = $2 THEN resolved_by_article END) AS "kb_resolved_count"'); + expect(params).toEqual([1, 1]); + }); + + it('applies the dataset\'s definition-level filter on the strict door too', async () => { + const svc = sqlOnlyService(SCOPED_METRICS); + const { sql, params } = await sqlFor(svc, { + cube: 'scoped_metrics', measures: ['opp_count', 'won_count'], + }); + // The dataset's intrinsic scope narrows the whole statement… + expect(sql).toContain('WHERE is_deleted = $2'); + // …while the measure filter stays scoped to its own measure. + expect(sql).toContain('COUNT(CASE WHEN stage = $1 THEN 1 END) AS "won_count"'); + expect(sql).toContain('COUNT(*) AS "opp_count"'); + expect(params).toEqual(['closed_won', 0]); + }); + + it('leaves a cube that is not a compiled dataset exactly as it was', async () => { + const svc = new AnalyticsService({ + debugSql: true, + queryCapabilities: () => ({ nativeSql: true, objectqlAggregate: false, inMemory: false }), + executeRawSql: async () => [], + // A MANIFEST cube — no dataset registry entry, so there is nothing to + // scope by and the emitted statement must be what it always was. + cubes: [{ + name: 'crm_case', title: 'Cases', sql: 'crm_case', public: false, + measures: { + count: { name: 'count', label: 'Count', type: 'count', sql: '*' }, + amount_sum: { name: 'amount_sum', label: 'Amount', type: 'sum', sql: 'amount' }, + }, + dimensions: {}, + }], + }); + const { sql, params } = await sqlFor(svc, { cube: 'crm_case', measures: ['count', 'amount_sum'] }); + expect(sql).toBe('SELECT COUNT(*) AS "count", SUM(amount) AS "amount_sum" FROM "crm_case"'); + expect(params).toEqual([]); + }); +}); + +// ── 3. both doors, one cube, real rows ────────────────────────────────────── + +/** Point sql.js at the `.wasm` shipped inside its own package (Node-safe). */ +async function locateWasm(): Promise<((file: string) => string) | undefined> { + try { + const { createRequire } = await import('node:module'); + const require = createRequire(import.meta.url); + const pkgJsonPath = require.resolve('sql.js/package.json'); + const { dirname, join } = await import('node:path'); + const dir = dirname(pkgJsonPath); + return (file: string) => join(dir, 'dist', file); + } catch { + return undefined; + } +} + +/** + * The card's ground truth, row for row: 24 opportunities, 8 won summing to + * 1,290,000, 5 lost, and a grand total of 5,632,500 — the number the broken + * door answered for `won_amount`. + */ +interface Opp { amount: number; owner: string } +const WON: Opp[] = [ + { amount: 300_000, owner: 'u1' }, { amount: 250_000, owner: 'u2' }, + { amount: 200_000, owner: 'u1' }, { amount: 150_000, owner: 'u2' }, + { amount: 120_000, owner: 'u1' }, { amount: 110_000, owner: 'u2' }, + { amount: 100_000, owner: 'u1' }, { amount: 60_000, owner: 'u2' }, +]; +const LOST: Opp[] = [ + { amount: 90_000, owner: 'u1' }, { amount: 90_000, owner: 'u2' }, + { amount: 90_000, owner: 'u1' }, { amount: 90_000, owner: 'u2' }, + { amount: 90_000, owner: 'u1' }, +]; +const OPEN: Opp[] = [ + { amount: 350_000, owner: 'u2' }, { amount: 350_000, owner: 'u1' }, + { amount: 350_000, owner: 'u2' }, { amount: 350_000, owner: 'u1' }, + { amount: 350_000, owner: 'u2' }, { amount: 350_000, owner: 'u1' }, + { amount: 350_000, owner: 'u2' }, { amount: 350_000, owner: 'u1' }, + { amount: 350_000, owner: 'u2' }, { amount: 350_000, owner: 'u1' }, + { amount: 392_500, owner: 'u2' }, +]; +const ALL: Opp[] = [...WON, ...LOST, ...OPEN]; +const sum = (rows: Opp[]) => rows.reduce((a, r) => a + r.amount, 0); + +describe('[#10298] the dashboard door and the API door answer the same numbers', () => { + let db: any; + let svc: AnalyticsService; + + beforeAll(async () => { + const mod: any = await import('sql.js'); + const initSqlJs = mod.default ?? mod; + const locateFile = await locateWasm(); + const SQL = await initSqlJs(locateFile ? { locateFile } : undefined); + + db = new SQL.Database(); + db.run(`CREATE TABLE "crm_opportunity" ( + "id" TEXT PRIMARY KEY, "stage" TEXT, "owner" TEXT, "amount" INTEGER + );`); + const insert = db.prepare( + `INSERT INTO "crm_opportunity" ("id","stage","owner","amount") VALUES (?,?,?,?)`, + ); + let n = 0; + for (const r of WON) insert.run([`o${++n}`, 'closed_won', r.owner, r.amount]); + for (const r of LOST) insert.run([`o${++n}`, 'closed_lost', r.owner, r.amount]); + for (const r of OPEN) insert.run([`o${++n}`, 'prospecting', r.owner, r.amount]); + insert.free(); + + svc = new AnalyticsService({ + debugSql: true, + queryCapabilities: () => ({ nativeSql: true, objectqlAggregate: false, inMemory: false }), + executeRawSql: async (_object: string, sql: string, params: unknown[]) => { + const stmt = db.prepare(sql.replace(/\$\d+/g, '?')); + stmt.bind(params as any[]); + const out: Record[] = []; + while (stmt.step()) out.push(stmt.getAsObject()); + stmt.free(); + return out; + }, + }); + svc.registerDataset(OPPORTUNITY_METRICS); + }); + + afterAll(() => db?.close()); + + const MEASURES = ['opp_count', 'won_count', 'lost_count', 'won_amount']; + + it('the fixture really is the card\'s org: 24 opportunities, 8 won, 5 lost', () => { + expect(ALL.length).toBe(24); + expect(WON.length).toBe(8); + expect(LOST.length).toBe(5); + expect(sum(WON)).toBe(1_290_000); + // The grand total is the card's WRONG answer for `won_amount` — the fixture + // can still produce it, which is what makes the assertion below falsifiable. + expect(sum(ALL)).toBe(5_632_500); + }); + + it('the API door answers the DECLARED numbers, not the unfiltered ones', async () => { + const result = await svc.query({ cube: 'opportunity_metrics', measures: MEASURES }); + expect(result.rows[0]).toMatchObject({ + opp_count: 24, won_count: 8, lost_count: 5, won_amount: 1_290_000, + }); + // The card's measured wrong answer, which the fixture can still produce. + expect(result.rows[0]).not.toMatchObject({ won_count: 24 }); + expect(result.rows[0]).not.toMatchObject({ won_amount: 5_632_500 }); + }); + + it('the dashboard door answers the same, for the same cube', async () => { + const viaApi = await svc.query({ cube: 'opportunity_metrics', measures: MEASURES }); + const viaDashboard = await svc.queryDataset(OPPORTUNITY_METRICS, { measures: MEASURES }); + for (const m of MEASURES) { + expect(viaDashboard.rows[0]?.[m]).toBe(viaApi.rows[0]?.[m]); + } + }); + + it('and they agree grouped by a dimension the filters do NOT name', async () => { + const selection = { measures: MEASURES, dimensions: ['owner'] }; + const viaApi = await svc.query({ cube: 'opportunity_metrics', ...selection }); + const viaDashboard = await svc.queryDataset(OPPORTUNITY_METRICS, selection); + const byOwner = (rows: Record[]) => + Object.fromEntries(rows.map((r) => [String(r.owner), r])); + + const api = byOwner(viaApi.rows); + // Each owner holds 12 of the 24 deals, four of them won. + expect(api.u1).toMatchObject({ opp_count: 12, won_count: 4, lost_count: 3 }); + expect(api.u2).toMatchObject({ opp_count: 12, won_count: 4, lost_count: 2 }); + expect(api.u1.won_amount).toBe(sum(WON.filter((r) => r.owner === 'u1'))); + expect(api.u2.won_amount).toBe(sum(WON.filter((r) => r.owner === 'u2'))); + // …and NOT each owner's whole book, which is what a dropped filter answers. + expect(api.u1.won_amount).not.toBe(sum(ALL.filter((r) => r.owner === 'u1'))); + + const dash = byOwner(viaDashboard.rows); + for (const owner of ['u1', 'u2']) { + for (const m of MEASURES) { + expect(dash[owner]?.[m], `${owner}.${m} disagrees between the two doors`) + .toBe(api[owner]?.[m]); + } + } + }); +}); diff --git a/packages/services/service-analytics/src/aggregation-lockstep.test.ts b/packages/services/service-analytics/src/aggregation-lockstep.test.ts index a7f8b55299..2434118d15 100644 --- a/packages/services/service-analytics/src/aggregation-lockstep.test.ts +++ b/packages/services/service-analytics/src/aggregation-lockstep.test.ts @@ -21,13 +21,24 @@ 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'; +import { SUPPORTED_AGGREGATE_SQL_KEYS, CONDITIONAL_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 lowered aggregate also has a measure-FILTERED form (#10298)', () => { + // Two tables in the strategy: the plain wrapper and the conditional one a + // measure's own `filter` selects. An aggregate present in the first only + // does not fail — it silently DROPS the author's filter and answers the + // unfiltered number under the filtered measure's name, which is the defect + // #10298 closed on the whole vocabulary at once. Pinned as set equality so + // the next aggregate added to one table and not the other fails here. + expect([...CONDITIONAL_AGGREGATE_SQL_KEYS].sort()) + .toEqual([...SUPPORTED_AGGREGATE_SQL_KEYS].sort()); + }); + it('every spec aggregate is either lowered or explicitly rejected', () => { const lowered = new Set(SUPPORTED_AGGREGATE_SQL_KEYS); const unhandled = AggregationFunction.options.filter( diff --git a/packages/services/service-analytics/src/analytics-service.ts b/packages/services/service-analytics/src/analytics-service.ts index e7e1761613..45e0aa40d0 100644 --- a/packages/services/service-analytics/src/analytics-service.ts +++ b/packages/services/service-analytics/src/analytics-service.ts @@ -22,7 +22,7 @@ import { createLogger, getEnv, bucketKeyToCalendarRange, zonedDateStartToUtcMs } // docblock for why the edge is acyclic and why it was worth adding. import { matchMissingColumnOfRelation } from '@objectstack/types'; import { CubeRegistry } from './cube-registry.js'; -import type { AnalyticsStrategy, AnalyticsDriverCapabilities, StrategyContext } from './strategies/types.js'; +import type { AnalyticsStrategy, AnalyticsDriverCapabilities, StrategyContext, DatasetScopedStrategyContext } from './strategies/types.js'; import { NativeSQLStrategy } from './strategies/native-sql-strategy.js'; import { ObjectQLStrategy } from './strategies/objectql-strategy.js'; // [#5669] The `where` source-field gate reads the filter tree through the SAME @@ -644,7 +644,7 @@ const DEFAULT_CAPABILITIES: AnalyticsDriverCapabilities = { export class AnalyticsService implements IAnalyticsService { private readonly strategies: AnalyticsStrategy[]; /** Context-independent part of the StrategyContext (no per-request scope). */ - private readonly baseCtx: StrategyContext; + private readonly baseCtx: DatasetScopedStrategyContext; /** Context-aware read-scope provider (bound to the request's context per call). */ private readonly readScopeProvider?: AnalyticsServiceConfig['getReadScope']; /** Compiled datasets by name — feeds the join allowlist (D-C) and queryDataset. */ @@ -724,6 +724,16 @@ export class AnalyticsService implements IAnalyticsService { getAllowedRelationships: (cubeName: string) => this.datasetRegistry.get(cubeName)?.allowedRelationships ?? config.getAllowedRelationships?.(cubeName), + // [#10298] The compiled dataset's definition-level filter and its + // per-measure filters — the half of the declaration the Cube model has + // no room for. Same shape and same registry as `getAllowedRelationships` + // directly above: answered for a cube that IS a compiled dataset, + // `undefined` for every other cube. + getDatasetScope: (cubeName: string) => { + const compiled = this.datasetRegistry.get(cubeName); + if (!compiled) return undefined; + return { filter: compiled.filter, measureFilters: compiled.measureFilters }; + }, coerceTemporalFilterValue: config.coerceTemporalFilterValue, coerceTemporalFilterColumn: config.coerceTemporalFilterColumn, isExternalObject: config.isExternalObject, @@ -760,7 +770,7 @@ export class AnalyticsService implements IAnalyticsService { private async callCtx( query: AnalyticsQuery, context?: ExecutionContext, - ): Promise { + ): Promise { // #3602 — `context` rides along unconditionally. It is the ENGINE-side belt // (forwarded to `engine.aggregate`, where the middleware chain applies its // own RLS), so it must not be gated on the analytics-side belt being wired: 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 dd98857dc0..b2ac0c2321 100644 --- a/packages/services/service-analytics/src/strategies/native-sql-strategy.ts +++ b/packages/services/service-analytics/src/strategies/native-sql-strategy.ts @@ -2,7 +2,7 @@ import type { AnalyticsQuery, AnalyticsResult } from '@objectstack/spec/contracts'; import type { Cube } from '@objectstack/spec/data'; -import type { AnalyticsStrategy, StrategyContext } from './types.js'; +import type { AnalyticsStrategy, StrategyContext, DatasetScopedStrategyContext } from './types.js'; import { lowerAnalyticsWhere, normalizeAnalyticsFilterTree, @@ -34,7 +34,15 @@ import { nextUtcCalendarDay } from '@objectstack/core'; * author's expression rather than wrapping it. */ const AGGREGATE_SQL: Record string> = { - 'count': () => 'COUNT(*)', + // [#10298] `count` takes its COLUMN when the measure declares one. The + // wrapper used to discard `col` and always emit `COUNT(*)`, so a measure + // written `{ aggregate: 'count', field: 'resolved_by_article' }` counted + // ROWS instead of non-null values — and a deflection rate built as + // `kb_resolved_count / closed_count` read 100% where the truth was 12.5%, + // with the numerator and denominator printed beside it as 8 and 8. `*` is + // still `COUNT(*)`: the compiler writes `sql: m.field ?? '*'`, so the star + // IS the "no field declared" spelling and must keep counting rows. + 'count': (col) => (col === '*' ? 'COUNT(*)' : `COUNT(${col})`), 'sum': (col) => `SUM(${col})`, 'avg': (col) => `AVG(${col})`, 'min': (col) => `MIN(${col})`, @@ -42,9 +50,44 @@ const AGGREGATE_SQL: Record string> = { 'count_distinct': (col) => `COUNT(DISTINCT ${col})`, }; +/** + * The same six aggregates, restricted to the rows a measure's own `filter` + * admits (#10298). + * + * Spelled `CASE WHEN` rather than SQL-standard `FILTER (WHERE …)` on purpose: + * `FILTER` is Postgres and SQLite ≥ 3.30 only — MySQL has never had it — and + * this strategy hand-compiles ONE statement for whichever SQL driver owns the + * object. A portable conditional aggregate is the only form that cannot answer + * a syntax error on one supported driver and a number on another. + * + * `count` over `*` counts a constant, because `COUNT(CASE WHEN p THEN * END)` + * is not a thing; over a real column it counts that column's non-null values + * among the admitted rows, which composes the two corrections this card makes. + * + * Keyed identically to {@link AGGREGATE_SQL} — `aggregation-lockstep.test.ts` + * pins the two key sets equal, so an aggregate added to one and not the other + * fails a test instead of silently losing its measure filter. + */ +const CONDITIONAL_AGGREGATE_SQL: Record string> = { + 'count': (col, pred) => `COUNT(CASE WHEN ${pred} THEN ${col === '*' ? '1' : col} END)`, + 'sum': (col, pred) => `SUM(CASE WHEN ${pred} THEN ${col} END)`, + 'avg': (col, pred) => `AVG(CASE WHEN ${pred} THEN ${col} END)`, + 'min': (col, pred) => `MIN(CASE WHEN ${pred} THEN ${col} END)`, + 'max': (col, pred) => `MAX(CASE WHEN ${pred} THEN ${col} END)`, + 'count_distinct': (col, pred) => `COUNT(DISTINCT CASE WHEN ${pred} THEN ${col} END)`, +}; + /** Exported for the lockstep guard — the aggregates this strategy can lower. */ export const SUPPORTED_AGGREGATE_SQL_KEYS = Object.keys(AGGREGATE_SQL); +/** + * Exported for the same guard — the aggregates this strategy can lower WITH a + * measure-scoped filter (#10298). Equal to {@link SUPPORTED_AGGREGATE_SQL_KEYS} + * by construction and pinned equal by the lockstep suite: an aggregate present + * in one table only would silently drop the author's `filter` rather than fail. + */ +export const CONDITIONAL_AGGREGATE_SQL_KEYS = Object.keys(CONDITIONAL_AGGREGATE_SQL); + /** * Metric types that are a custom SQL *expression*, not an aggregate to wrap. * @@ -375,10 +418,37 @@ export class NativeSQLStrategy implements AnalyticsStrategy { } } + // ── #10298 — the half of a compiled dataset the Cube cannot carry ────── + // A dataset's definition-level `filter` and each measure's own scoped + // `filter` live beside the Cube, in the dataset registry. `DatasetExecutor` + // read them; this strategy did not — so `/api/v1/analytics/query`, which + // addresses the registered Cube directly, answered UNFILTERED aggregates + // under the author's measure names while the dashboard answered filtered + // ones, for the same cube. `undefined` for any cube that is not a compiled + // dataset, which is why an inferred or manifest cube compiles unchanged. + const datasetScope = (ctx as DatasetScopedStrategyContext).getDatasetScope?.(query.cube!); + // Build SELECT for measures if (query.measures && query.measures.length > 0) { for (const measure of query.measures) { - const aggExpr = this.resolveMeasureSql(cube, measure, tableName, joins); + // The measure's own filter becomes a CONDITIONAL aggregate rather than + // a `WHERE` conjunct: the statement carries several measures at once and + // a `WHERE` would narrow ALL of them. Compiled here, inside the SELECT + // loop, so its bound values are pushed onto `params` in the order the + // placeholders appear in the statement — the SELECT list precedes the + // WHERE clause, and `$n` is positional. + const measureFilter = datasetScope?.measureFilters?.[measure]; + const predicate = measureFilter + ? this.compileFilterNode( + normalizeAnalyticsFilterTree({ where: measureFilter }), + cube, + tableName, + joins, + params, + ctx, + ) + : null; + const aggExpr = this.resolveMeasureSql(cube, measure, tableName, joins, predicate); selectClauses.push(`${aggExpr} AS "${measure}"`); } } @@ -397,6 +467,24 @@ export class NativeSQLStrategy implements AnalyticsStrategy { ); if (filterSql) whereClauses.push(filterSql); + // [#10298] The dataset's OWN scope, for the door that never went through + // `DatasetExecutor`. Applied as a plain conjunct because it narrows the + // whole statement — every measure in it — which is exactly what the + // definition-level filter means. Redundant on the dataset door (the + // executor already merged it into `where`) and idempotent there: ANDing a + // predicate with itself selects the same rows. + if (datasetScope?.filter) { + const scopeSql = this.compileFilterNode( + normalizeAnalyticsFilterTree({ where: datasetScope.filter }), + cube, + tableName, + joins, + params, + ctx, + ); + if (scopeSql) whereClauses.push(scopeSql); + } + // Build time dimension filters if (query.timeDimensions && query.timeDimensions.length > 0) { for (const td of query.timeDimensions) { @@ -669,11 +757,17 @@ export class NativeSQLStrategy implements AnalyticsStrategy { return this.qualifyAndRegisterJoin(raw, parentTable, joins, cube); } + /** + * @param predicate - The measure's own scoped filter, already compiled to a + * SQL boolean (`null` = the measure declares none, or declares one that + * constrains nothing — `compileFilterNode`'s TRUE). #10298. + */ private resolveMeasureSql( cube: Cube, member: string, parentTable: string, joins: Map, + predicate: string | null = null, ): string { const measure = this.lookupMember(cube, member, 'measure') as | { sql: string; type: string } @@ -702,6 +796,25 @@ export class NativeSQLStrategy implements AnalyticsStrategy { ? '*' : this.qualifyAndRegisterJoin(measure.sql, parentTable, joins, cube); + if (predicate !== null) { + const wrapConditional = CONDITIONAL_AGGREGATE_SQL[measure.type]; + if (wrapConditional) return wrapConditional(col, predicate); + // [#10298] Deliberately BARE — an undeclared 500, same tier and same + // reasoning as the "unrecognised type" throw below. A measure filter only + // ever arrives here from a COMPILED DATASET, and `DatasetMeasure.aggregate` + // is `AggregationFunction`, whose every member is a key of the table + // above — so an expression metric type (`number`/`string`/`boolean`, + // where `sql` IS the whole computation and there is no aggregate to make + // conditional) cannot carry one. What would reach here is our own drift. + // Emitting the unfiltered aggregate instead is precisely the defect this + // card closes: a 200 carrying different arithmetic than the author declared. + throw new Error( + `[native-sql-strategy] measure "${member}" on cube "${cube.name}" carries a ` + + `scoped filter, but its type "${measure.type}" has no conditional form ` + + `(conditional: ${CONDITIONAL_AGGREGATE_SQL_KEYS.join(', ')}).`, + ); + } + const wrap = AGGREGATE_SQL[measure.type]; if (wrap) return wrap(col); // A custom SQL expression: the measure's `sql` IS the computation, so emit diff --git a/packages/services/service-analytics/src/strategies/types.ts b/packages/services/service-analytics/src/strategies/types.ts index 6b00d93986..b2ef2b9b81 100644 --- a/packages/services/service-analytics/src/strategies/types.ts +++ b/packages/services/service-analytics/src/strategies/types.ts @@ -14,3 +14,42 @@ export type { StrategyContext, AnalyticsDriverCapabilities, } from '@objectstack/spec/contracts'; + +import type { FilterCondition } from '@objectstack/spec/data'; +import type { StrategyContext } from '@objectstack/spec/contracts'; + +/** + * The semantic scope a compiled DATASET carries beside its Cube (#10298). + * + * `compileDataset` splits a dataset into two halves: the parts the Cube model + * can express (measures, dimensions, joins) and the parts it cannot — the + * dataset's definition-level `filter` and each measure's own scoped `filter`. + * Until #10298 the second half was read by `DatasetExecutor` alone, so the + * dashboard door applied it and the strict `/api/v1/analytics/query` door — + * which addresses the registered Cube directly and never touches the executor + * — silently answered UNFILTERED aggregates under the same measure names, for + * the same cube. Two doors, two numbers. + * + * This is the channel that carries the missing half to the strategy, so both + * doors compile the same declaration. It is deliberately declared HERE rather + * than on the spec's {@link StrategyContext}: the analytics package builds the + * context object it hands its own strategies, and nothing about this channel + * is an authorable surface — no metadata key, no wire shape, no error code — + * so widening the published contract would buy nothing and cost a spec edit. + * A strategy that does not know the hook keeps the behaviour it had. + * + * Every member is optional and tiered "cannot answer, do not block": a cube + * that is not a compiled dataset (an inferred cube, a manifest cube) answers + * `undefined` and compiles exactly as it did before. + */ +export interface DatasetScope { + /** The dataset's definition-level filter — its intrinsic scope. */ + filter?: FilterCondition; + /** Per-measure scoped filters, keyed by measure name. */ + measureFilters?: Record; +} + +/** A {@link StrategyContext} that can answer for a compiled dataset (#10298). */ +export interface DatasetScopedStrategyContext extends StrategyContext { + getDatasetScope?(cubeName: string): DatasetScope | undefined; +}