From 348c5d20a0eabb013cd3e3e9be058197309cdcf8 Mon Sep 17 00:00:00 2001 From: Jack Zhuang <277994282+os-zhuang@users.noreply.github.com> Date: Tue, 28 Jul 2026 10:59:07 +0800 Subject: [PATCH 1/2] fix(analytics): ObjectQLStrategy applies timeDimensions[].dateRange (#3650) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `execute()` built its engine filter purely from `normalizeAnalyticsFilters`, which reads only `query.where`. `dateRange` is a SIBLING of `where`, never folded into it, so the window was dropped on the floor — no error, and the chart plotted all of history. Not a corner case: `NativeSQLStrategy.canHandle` declines any query carrying a `granularity`, so a date-bucketed trend lands on this path on EVERY driver, and a bucketed trend is exactly the shape that also carries a range. It also made `compareTo` structurally dead — `runCompare` shifts `dateRange` and nothing else, so both passes issued a byte-identical aggregate and every `__compare` equalled its primary. The window now lowers to an inclusive `{$gte,$lte}` on the resolved field — the shape NativeSQL binds as BETWEEN and the memory driver builds as a `$match`. No storage coercion here on purpose: this path goes through `engine.aggregate()`, where the driver's own CRUD filter coercion applies. Same-field composition is fixed alongside it, because the window makes the collision routine: operands naming DIFFERENT operators still share one entry, colliding ones become their own `$and` conjunct so the engine intersects them instead of the last writer winning. `generateSql()` renders the window as a parameterised BETWEEN to match. A cross-object time dimension is still rejected, now reported as the bucketing error it is. Co-Authored-By: Claude --- .changeset/objectql-strategy-daterange.md | 62 +++ .../src/sql-driver-nested-and-filter.test.ts | 99 ++++ .../src/__tests__/objectql-daterange.test.ts | 471 ++++++++++++++++++ .../src/strategies/objectql-strategy.ts | 170 +++++-- 4 files changed, 772 insertions(+), 30 deletions(-) create mode 100644 .changeset/objectql-strategy-daterange.md create mode 100644 packages/plugins/driver-sql/src/sql-driver-nested-and-filter.test.ts create mode 100644 packages/services/service-analytics/src/__tests__/objectql-daterange.test.ts diff --git a/.changeset/objectql-strategy-daterange.md b/.changeset/objectql-strategy-daterange.md new file mode 100644 index 0000000000..d95043630d --- /dev/null +++ b/.changeset/objectql-strategy-daterange.md @@ -0,0 +1,62 @@ +--- +"@objectstack/service-analytics": patch +--- + +fix(analytics): ObjectQLStrategy applies `timeDimensions[].dateRange` — the predicate every date-bucketed chart was missing (#3650) + +`ObjectQLStrategy.execute()` built its engine filter purely from +`normalizeAnalyticsFilters(query)`, which reads only `query.where`. But +`dateRange` is a **sibling** of `where`, never folded into it — so the window +was dropped on the floor. No error, no warning: the chart rendered, and the +numbers were for all of history. + +This was not a "some drivers only" corner. `NativeSQLStrategy.canHandle` +declines any query carrying a `granularity`, so a **date-bucketed trend lands on +the ObjectQL path on every driver**, Postgres and SQLite included — and a +bucketed trend is precisely the shape that also carries a range ("last 12 +months", "this quarter"). The other two paths always applied it +(`NativeSQLStrategy` as `BETWEEN`, `preview-evaluator` row-wise); only this one +did not. + +**Two visible symptoms:** + +- A trend chart with a time filter plotted **every row ever recorded** instead + of the selected window. +- `compareTo` (period-over-period) was **structurally dead**. `runCompare` + builds the comparison pass by shifting `dateRange` and changing nothing else, + so with the window ignored both passes issued a byte-identical aggregate: + every `__compare` column equalled its primary and the delta was a + flat 0%. And since `compareTo` requires a time dimension, it always took this + path. + +The window now lowers to an inclusive `{$gte, $lte}` on the resolved field — the +same shape `NativeSQLStrategy` binds as `BETWEEN` and the memory driver builds +as a `$match` — so one dashboard reads the same on every driver. No storage +coercion is applied here on purpose: unlike the raw-SQL path (which had to learn +about SQLite's INTEGER epoch in #2034), this path goes through +`engine.aggregate()`, where the driver's own CRUD filter coercion already +handles a `where` bound on that same column. + +**Same-field composition was fixed alongside it**, because the window makes it +routine. Operands merged into one field entry by spreading, which silently kept +whichever came last: a `where` bound and a window bound on `close_date` would +have had one erase the other, and a `where` that names one field twice through +`$and` (`{$and: [{stage: 'won'}, {stage: {$ne: 'lost'}}]}`) already lost its +first operand today. Operands that name **different** operators still share one +entry; colliding ones become their own `$and` conjunct, so the engine +intersects them instead of the strategy picking a winner. + +`generateSql()` renders the window as a parameterised `BETWEEN` to match — its +comment previously explained why a `BETWEEN` was deliberately absent, which was +correct only while `execute()` dropped the window. Bounds bind as `$n` +placeholders, never inlined: the echoed statement travels to the browser. + +A window on a **cross-object** time dimension is still rejected, and is now +reported as the bucketing error it is rather than as the "cross-object filter" +its lowered predicate would otherwise resemble. `execute()` and +`/analytics/sql` continue to accept and reject the same set. + +Relative-phrase ranges ("Last 7 days") are still not resolved on this path, and +a bare-string `dateRange` degenerates to a single point — both matching +`NativeSQLStrategy` exactly, rather than inventing a second interpretation for +the driver-independent path. diff --git a/packages/plugins/driver-sql/src/sql-driver-nested-and-filter.test.ts b/packages/plugins/driver-sql/src/sql-driver-nested-and-filter.test.ts new file mode 100644 index 0000000000..eb1f27d606 --- /dev/null +++ b/packages/plugins/driver-sql/src/sql-driver-nested-and-filter.test.ts @@ -0,0 +1,99 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * A FilterCondition may carry a `$and` array ALONGSIDE plain field keys, and + * the `$and` elements may nest further. Both are legal per + * `FilterConditionSchema` (`$and` recurses; the schema intersects a record with + * the logical-operator object), and `applyFilterCondition` handles them by + * iterating every key — but nothing exercised the combination end-to-end, so + * "the compiler happens to iterate all keys" was an implementation detail + * rather than a contract. + * + * It became a contract with #3650: ObjectQLStrategy now AND-composes predicates + * that cannot share one field entry (a `where` bound colliding with a + * `timeDimensions[].dateRange` window) into `filter.$and`, and `withReadScope` + * then wraps THAT in another `$and` with the tenant predicate. The exact + * two-level shape below is what the analytics path hands the driver. Silently + * dropping either level would widen the query — the analytics path's read scope + * lives in the outer one. + */ + +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { SqlDriver } from '../src/index.js'; + +/** One row survives all three predicates; each other row fails exactly one. */ +const FIXTURE = [ + { id: 'a', close_date: '2026-01-05', organization_id: 'org_A', amount: 10 }, // fails inner-sibling $gte + { id: 'b', close_date: '2026-01-20', organization_id: 'org_A', amount: 20 }, // survives + { id: 'c', close_date: '2026-02-10', organization_id: 'org_A', amount: 40 }, // fails nested $lte + { id: 'd', close_date: '2026-01-20', organization_id: 'org_B', amount: 80 }, // fails outer tenant +]; + +/** + * `{ $and: [ {field…, $and: [ … ]}, {tenant} ] }` — a field key and a nested + * `$and` inside one branch of an outer `$and`. + */ +const NESTED_AND = { + $and: [ + { + close_date: { $gte: '2026-01-15' }, + $and: [{ close_date: { $gte: '2026-01-01', $lte: '2026-01-31' } }], + }, + { organization_id: 'org_A' }, + ], +}; + +describe('SqlDriver — field key alongside a nested $and (#3650)', () => { + let driver: SqlDriver; + let knex: any; + + beforeEach(async () => { + driver = new SqlDriver({ + client: 'better-sqlite3', + connection: { filename: ':memory:' }, + useNullAsDefault: true, + }); + knex = (driver as any).knex; + await knex.schema.createTable('opportunity', (t: any) => { + t.string('id').primary(); + t.string('close_date'); + t.string('organization_id'); + t.float('amount'); + }); + await knex('opportunity').insert(FIXTURE); + }); + + afterEach(async () => { + await knex.destroy(); + }); + + it('intersects every level on find()', async () => { + const rows = await driver.find('opportunity', { where: NESTED_AND } as any); + expect(rows.map((r: any) => r.id)).toEqual(['b']); + }); + + it('intersects every level on aggregate() — the analytics path', async () => { + const rows = await driver.aggregate('opportunity', { + groupBy: ['organization_id'], + aggregations: [{ function: 'sum', field: 'amount', alias: 'total' }], + where: NESTED_AND, + } as any); + + // 20 only. Dropping the nested `$and` would add row c (60); dropping the + // sibling field key would add row a (30); dropping the outer branch would + // add row d and split the grouping. + expect(rows).toEqual([{ organization_id: 'org_A', total: 20 }]); + }); + + it('keeps a bucketed aggregate scoped to the same intersection', async () => { + // The #3650 shape in its native habitat: a date-bucketed trend whose window + // and read scope both have to survive. + const rows = await driver.aggregate('opportunity', { + groupBy: [{ field: 'close_date', dateGranularity: 'month' }], + aggregations: [{ function: 'sum', field: 'amount', alias: 'total' }], + where: NESTED_AND, + } as any); + + expect(rows).toEqual([{ close_date: '2026-01', total: 20 }]); + }); +}); diff --git a/packages/services/service-analytics/src/__tests__/objectql-daterange.test.ts b/packages/services/service-analytics/src/__tests__/objectql-daterange.test.ts new file mode 100644 index 0000000000..56145612b5 --- /dev/null +++ b/packages/services/service-analytics/src/__tests__/objectql-daterange.test.ts @@ -0,0 +1,471 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #3650 — `timeDimensions[].dateRange` reaches the ObjectQL aggregate path. + * + * The strategy built its engine filter purely from `normalizeAnalyticsFilters`, + * which reads only `where`. `dateRange` is a SIBLING of `where`, so the window + * was dropped on the floor: no error, just every row ever recorded. + * + * That is not a corner case. `NativeSQLStrategy.canHandle` declines any query + * carrying a `granularity`, so a date-bucketed trend lands here on EVERY driver + * — and a bucketed trend is exactly the shape that also carries a range ("last + * 12 months", "this quarter"). The chart rendered, the numbers were wrong. + * + * The bridge below is HONEST — it actually applies the filter it is handed — so + * a missing predicate produces real extra rows rather than an artifact of a + * permissive stub. + */ + +import { describe, it, expect } from 'vitest'; +import { DatasetSchema } from '@objectstack/spec/ui'; +import type { ExecutionContext } from '@objectstack/spec/kernel'; +import { AnalyticsService } from '../analytics-service.js'; +import { compileDataset } from '../dataset-compiler.js'; +import { DatasetExecutor } from '../dataset-executor.js'; + +const dataset = DatasetSchema.parse({ + name: 'sales', + label: 'Sales', + object: 'opportunity', + dimensions: [ + { name: 'close_date', field: 'close_date', type: 'date' }, + { name: 'stage', field: 'stage', type: 'string' }, + ], + measures: [{ name: 'revenue', aggregate: 'sum', field: 'amount' }], +}); + +/** Rows straddling the window, so a dropped bound shows up as extra revenue. */ +const TABLE = [ + { id: 1, close_date: '2025-11-15', stage: 'won', amount: 900 }, // before + { id: 2, close_date: '2026-01-10', stage: 'won', amount: 100 }, // inside + { id: 3, close_date: '2026-01-20', stage: 'lost', amount: 200 }, // inside + { id: 4, close_date: '2026-02-14', stage: 'won', amount: 30 }, // inside (Feb) + { id: 5, close_date: '2026-06-05', stage: 'won', amount: 700 }, // after +]; + +const ctx = { tenantId: 'org_A', userId: 'u_a' } as ExecutionContext; +const objectqlOnly = () => ({ nativeSql: false, objectqlAggregate: true, inMemory: false }); + +type Row = Record; +type GroupByItem = string | { field: string; dateGranularity: string }; +type AggOpts = { + groupBy?: GroupByItem[]; + aggregations?: Array<{ field: string; method: string; alias: string }>; + filter?: Record; +}; + +/** Evaluate the FilterCondition subset these tests can produce. */ +function matches(row: Row, filter: Record): boolean { + return Object.entries(filter).every(([key, cond]) => { + if (key === '$and') return (cond as Record[]).every((sub) => matches(row, sub)); + const v = row[key]; + if (cond && typeof cond === 'object' && !Array.isArray(cond)) { + return Object.entries(cond as Record).every(([op, operand]) => { + switch (op) { + case '$gte': return String(v) >= String(operand); + case '$lte': return String(v) <= String(operand); + case '$gt': return String(v) > String(operand); + case '$lt': return String(v) < String(operand); + case '$ne': return v !== operand; + default: throw new Error(`test bridge: unhandled operator ${op}`); + } + }); + } + return v === cond; + }); +} + +/** `engine.aggregate()` stand-in: filters, buckets, sums — for real. */ +function makeAggregate(seen: AggOpts[], table: Row[] = TABLE) { + return async (_object: string, opts: AggOpts): Promise => { + seen.push(opts); + const rows = table.filter((r) => matches(r, opts.filter ?? {})); + const buckets = new Map(); + for (const r of rows) { + const key: Row = {}; + for (const g of opts.groupBy ?? []) { + if (typeof g === 'string') key[g] = r[g]; + // Only `month` bucketing is exercised here. + else key[g.field] = String(r[g.field]).slice(0, 7); + } + const id = JSON.stringify(key); + const b = buckets.get(id) ?? { ...key }; + for (const a of opts.aggregations ?? []) { + if (a.method === 'sum') b[a.alias] = Number(b[a.alias] ?? 0) + Number(r[a.field] ?? 0); + if (a.method === 'count') b[a.alias] = Number(b[a.alias] ?? 0) + 1; + } + buckets.set(id, b); + } + return [...buckets.values()]; + }; +} + +function makeService(seen: AggOpts[], overrides: Record = {}) { + const compiled = compileDataset(dataset); + return new AnalyticsService({ + cubes: [compiled.cube], + queryCapabilities: objectqlOnly, + executeAggregate: makeAggregate(seen), + ...overrides, + }); +} + +describe('ObjectQLStrategy — timeDimensions[].dateRange (#3650)', () => { + it('confines a date-bucketed trend to the requested window', async () => { + const seen: AggOpts[] = []; + const result = await makeService(seen).query( + { + cube: 'sales', + dimensions: ['close_date'], + measures: ['revenue'], + timeDimensions: [ + { dimension: 'close_date', granularity: 'month', dateRange: ['2026-01-01', '2026-02-28'] }, + ], + }, + ctx, + ); + + // The window reaches the engine as an inclusive range — the same shape + // NativeSQLStrategy binds as BETWEEN. + expect(seen[0].filter).toEqual({ close_date: { $gte: '2026-01-01', $lte: '2026-02-28' } }); + // Nov (900) and Jun (700) are outside; before the fix all of history landed + // in the chart — 1930 across four buckets instead of 330 across two. + expect(result.rows).toEqual([ + { close_date: '2026-01', revenue: 300 }, + { close_date: '2026-02', revenue: 30 }, + ]); + }); + + it('still buckets by month while the window applies', async () => { + const seen: AggOpts[] = []; + await makeService(seen).query( + { + cube: 'sales', + dimensions: ['close_date'], + measures: ['revenue'], + timeDimensions: [ + { dimension: 'close_date', granularity: 'month', dateRange: ['2026-01-01', '2026-02-28'] }, + ], + }, + ctx, + ); + // The window must not displace the granularity lowering — both travel. + expect(seen[0].groupBy).toEqual([{ field: 'close_date', dateGranularity: 'month' }]); + }); + + it('applies a window on a time dimension that is not also a selected dimension', async () => { + const seen: AggOpts[] = []; + const result = await makeService(seen).query( + { + cube: 'sales', + dimensions: ['stage'], + measures: ['revenue'], + timeDimensions: [{ dimension: 'close_date', dateRange: ['2026-01-01', '2026-01-31'] }], + }, + ctx, + ); + + expect(seen[0].filter).toEqual({ close_date: { $gte: '2026-01-01', $lte: '2026-01-31' } }); + expect(result.rows).toEqual([ + { stage: 'won', revenue: 100 }, + { stage: 'lost', revenue: 200 }, + ]); + }); + + it('degenerates a bare-string dateRange to a single point, like NativeSQLStrategy', async () => { + const seen: AggOpts[] = []; + const result = await makeService(seen).query( + { + cube: 'sales', + dimensions: ['stage'], + measures: ['revenue'], + timeDimensions: [{ dimension: 'close_date', dateRange: '2026-01-20' }], + }, + ctx, + ); + + expect(seen[0].filter).toEqual({ close_date: { $gte: '2026-01-20', $lte: '2026-01-20' } }); + expect(result.rows).toEqual([{ stage: 'lost', revenue: 200 }]); + }); + + it('narrows rather than vanishes on a one-entry dateRange array', async () => { + const seen: AggOpts[] = []; + await makeService(seen).query( + { + cube: 'sales', + dimensions: ['stage'], + measures: ['revenue'], + // The schema types `dateRange` as a plain `string[]`, so this parses. + // `NativeSQLStrategy` drops such a window — but "drop the window" means + // "plot all of history", the very failure #3650 is about. + timeDimensions: [{ dimension: 'close_date', dateRange: ['2026-01-20'] }], + }, + ctx, + ); + expect(seen[0].filter).toEqual({ close_date: { $gte: '2026-01-20', $lte: '2026-01-20' } }); + }); + + it('ANDs the read scope around the window rather than replacing it', async () => { + const seen: AggOpts[] = []; + const svc = makeService(seen, { + getReadScope: (_o: string, c?: ExecutionContext) => + c?.tenantId ? { organization_id: c.tenantId } : undefined, + }); + await svc.query( + { + cube: 'sales', + dimensions: ['stage'], + measures: ['revenue'], + timeDimensions: [{ dimension: 'close_date', dateRange: ['2026-01-01', '2026-01-31'] }], + }, + ctx, + ); + + expect(seen[0].filter).toEqual({ + $and: [ + { close_date: { $gte: '2026-01-01', $lte: '2026-01-31' } }, + { organization_id: 'org_A' }, + ], + }); + }); + + it('leaves a query without a dateRange unfiltered', async () => { + const seen: AggOpts[] = []; + await makeService(seen).query( + { + cube: 'sales', + dimensions: ['close_date'], + measures: ['revenue'], + timeDimensions: [{ dimension: 'close_date', granularity: 'month' }], + }, + ctx, + ); + // An empty filter stays absent — the window is the only thing that would + // have populated it, so a bucketed trend with no range still scans all. + expect(seen[0].filter).toBeUndefined(); + }); +}); + +describe('ObjectQLStrategy — window ∧ where on one field (#3650)', () => { + it('intersects a window with a disjoint-operator where bound', async () => { + const seen: AggOpts[] = []; + const result = await makeService(seen).query( + { + cube: 'sales', + dimensions: ['stage'], + measures: ['revenue'], + // `$gt` and the window's `$gte`/`$lte` name different operators, so they + // share one entry. + where: { close_date: { $gt: '2026-01-15' } }, + timeDimensions: [{ dimension: 'close_date', dateRange: ['2026-01-01', '2026-01-31'] }], + }, + ctx, + ); + + expect(seen[0].filter).toEqual({ + close_date: { $gt: '2026-01-15', $gte: '2026-01-01', $lte: '2026-01-31' }, + }); + expect(result.rows).toEqual([{ stage: 'lost', revenue: 200 }]); + }); + + it('intersects a window with a COLLIDING where bound instead of widening', async () => { + const seen: AggOpts[] = []; + const result = await makeService(seen).query( + { + cube: 'sales', + dimensions: ['stage'], + measures: ['revenue'], + // A narrower `$gte` on the same field. Spreading the window over it + // would keep the window's looser `2026-01-01` and silently widen the + // query; the collision becomes its own conjunct instead. + where: { close_date: { $gte: '2026-01-15' } }, + timeDimensions: [{ dimension: 'close_date', dateRange: ['2026-01-01', '2026-01-31'] }], + }, + ctx, + ); + + expect(seen[0].filter).toEqual({ + close_date: { $gte: '2026-01-15' }, + $and: [{ close_date: { $gte: '2026-01-01', $lte: '2026-01-31' } }], + }); + // Jan 10 (100) is excluded by the caller's own narrower bound. + expect(result.rows).toEqual([{ stage: 'lost', revenue: 200 }]); + }); + + it('keeps both operands when a bare equality meets an operator object', async () => { + const seen: AggOpts[] = []; + // `$and` in `where` flattens to two entries on one field — the shape that + // used to have the second silently replace the first. + await makeService(seen).query( + { + cube: 'sales', + dimensions: ['close_date'], + measures: ['revenue'], + where: { $and: [{ stage: 'won' }, { stage: { $ne: 'lost' } }] }, + }, + ctx, + ); + + expect(seen[0].filter).toEqual({ + stage: 'won', + $and: [{ stage: { $ne: 'lost' } }], + }); + }); +}); + +describe('DatasetExecutor compareTo over the ObjectQL path (#3650)', () => { + it('shifts the comparison window instead of re-running the identical query', async () => { + const seen: AggOpts[] = []; + const compiled = compileDataset(dataset); + const svc = makeService(seen); + + const res = await new DatasetExecutor(svc).execute(compiled, { + dimensions: ['stage'], + measures: ['revenue'], + timeDimensions: [{ dimension: 'close_date', dateRange: ['2026-01-01', '2026-01-31'] }], + compareTo: { kind: 'previousPeriod', dimension: 'close_date' }, + }); + + // `runCompare` builds the comparison pass by shifting `dateRange` and + // nothing else. With the window dropped, the two passes were byte-identical + // — every `__compare` column equalled its primary and the delta was always + // a flat 0%. + expect(seen).toHaveLength(2); + expect(seen[0].filter).not.toEqual(seen[1].filter); + const windows = seen.map((s) => (s.filter?.close_date as Record)?.$gte); + expect(windows).toEqual(['2026-01-01', '2025-12-01']); + + // Dec 2025 holds no rows, so the comparison is genuinely empty — not a copy + // of the primary. + const won = res.rows.find((r) => r.stage === 'won')!; + expect(won.revenue).toBe(100); + expect(won.revenue__compare ?? 0).toBe(0); + }); +}); + +describe('ObjectQLStrategy.generateSql — window rendering (#3650)', () => { + it('renders the window as a parameterised BETWEEN', async () => { + const seen: AggOpts[] = []; + const svc = makeService(seen); + + const { sql, params } = await svc.generateSql!({ + cube: 'sales', + dimensions: ['close_date'], + measures: ['revenue'], + timeDimensions: [ + { dimension: 'close_date', granularity: 'month', dateRange: ['2026-01-01', '2026-02-28'] }, + ], + }); + + // The preview used to omit the window deliberately, because `execute()` + // dropped it. Now that the window applies, omitting it would be the lie in + // the other direction. + expect(sql).toContain('close_date BETWEEN $1 AND $2'); + expect(sql).toContain("date_trunc('month', close_date)"); + // Bounds bind as parameters — the echoed string travels to the browser. + expect(params).toEqual(['2026-01-01', '2026-02-28']); + expect(sql).not.toContain('2026-01-01'); + }); + + it('numbers window placeholders after the caller\'s own filters', async () => { + const seen: AggOpts[] = []; + const svc = makeService(seen); + + const { sql, params } = await svc.generateSql!({ + cube: 'sales', + dimensions: ['stage'], + measures: ['revenue'], + where: { stage: 'won' }, + timeDimensions: [{ dimension: 'close_date', dateRange: ['2026-01-01', '2026-01-31'] }], + }); + + expect(sql).toContain('close_date BETWEEN $2 AND $3'); + expect(params).toEqual(['won', '2026-01-01', '2026-01-31']); + }); +}); + +describe('ObjectQLStrategy — cross-object FK-expand carries the window (#3650 × #3654)', () => { + const crossDataset = DatasetSchema.parse({ + name: 'sales_by_account', + label: 'Sales by account', + object: 'opportunity', + include: ['account'], + dimensions: [ + { name: 'region', field: 'account.region', type: 'string' }, + { name: 'close_date', field: 'close_date', type: 'date' }, + ], + measures: [{ name: 'revenue', aggregate: 'sum', field: 'amount' }], + }); + + it('scopes the BASE aggregate of an FK-expand to the window', async () => { + const calls: Array<{ object: string; filter?: unknown }> = []; + const compiled = compileDataset(crossDataset); + const svc = new AnalyticsService({ + cubes: [compiled.cube], + queryCapabilities: objectqlOnly, + getAllowedRelationships: () => compiled.allowedRelationships, + executeAggregate: async (object, opts) => { + calls.push({ object, filter: opts.filter }); + return object === 'opportunity' + ? [{ account: 'acc_w', revenue: 100 }] + : [{ id: 'acc_w', region: 'West' }]; + }, + }); + + const result = await svc.query( + { + cube: 'sales_by_account', + dimensions: ['region'], + measures: ['revenue'], + timeDimensions: [{ dimension: 'close_date', dateRange: ['2026-01-01', '2026-01-31'] }], + }, + ctx, + ); + + const base = calls.find((c) => c.object === 'opportunity')!; + expect(base.filter).toEqual({ close_date: { $gte: '2026-01-01', $lte: '2026-01-31' } }); + // The FK→attribute resolution is keyed by id and must NOT inherit the + // window — a related record has no `close_date`. + const ref = calls.find((c) => c.object === 'account')!; + expect(ref.filter).toEqual({ id: { $in: ['acc_w'] } }); + expect(result.rows).toEqual([{ region: 'West', revenue: 100 }]); + }); + + it('rejects a window on a CROSS-OBJECT time dimension as a bucketing error', async () => { + const crossTime = DatasetSchema.parse({ + name: 'sales_by_acct_date', + label: 'Sales by account date', + object: 'opportunity', + include: ['account'], + dimensions: [ + { name: 'region', field: 'account.region', type: 'string' }, + { name: 'acct_created', field: 'account.created_at', type: 'date' }, + ], + measures: [{ name: 'revenue', aggregate: 'sum', field: 'amount' }], + }); + const compiled = compileDataset(crossTime); + const svc = new AnalyticsService({ + cubes: [compiled.cube], + queryCapabilities: objectqlOnly, + getAllowedRelationships: () => compiled.allowedRelationships, + executeAggregate: async () => [], + }); + + const query = { + cube: 'sales_by_acct_date', + dimensions: ['region'], + measures: ['revenue'], + timeDimensions: [{ dimension: 'acct_created', dateRange: ['2026-01-01', '2026-01-31'] }], + }; + // Reported as a time-dimension bucketing error, not as the "cross-object + // filter" its lowered predicate would otherwise look like. + await expect(svc.query(query, ctx)).rejects.toThrow( + /cannot bucket a cross-object time dimension/, + ); + // `/analytics/sql` rejects the same query, the same way. + await expect(svc.generateSql!(query)).rejects.toThrow( + /cannot bucket a cross-object time dimension/, + ); + }); +}); diff --git a/packages/services/service-analytics/src/strategies/objectql-strategy.ts b/packages/services/service-analytics/src/strategies/objectql-strategy.ts index 6336aea345..b683b63a7b 100644 --- a/packages/services/service-analytics/src/strategies/objectql-strategy.ts +++ b/packages/services/service-analytics/src/strategies/objectql-strategy.ts @@ -89,23 +89,29 @@ export class ObjectQLStrategy implements AnalyticsStrategy { } } - // Build filter from query filters. A single field may carry MULTIPLE - // operators (e.g. a range `{$gte, $lte}` from `close_date` between two - // bounds). Merge same-field operator objects instead of overwriting, or a - // range would silently lose a bound (only the last operator would survive). + // Build the engine filter. Every predicate — the caller's `where` and the + // time-dimension windows alike — is contributed through + // `mergeFilterOperand`, because one field routinely carries MULTIPLE + // operators (a range `{$gte, $lte}` on `close_date`) and a plain assignment + // would keep only the last. const filter: Record = {}; - const normalizedFilters = normalizeAnalyticsFilters(query); - if (normalizedFilters.length > 0) { - for (const f of normalizedFilters) { - const fieldName = this.resolveFieldName(cube, f.member, 'any'); - const converted = this.convertFilter(f.operator, f.values); - const existing = filter[fieldName]; - const mergeable = (v: unknown): v is Record => - !!v && typeof v === 'object' && !Array.isArray(v); - filter[fieldName] = mergeable(existing) && mergeable(converted) - ? { ...existing, ...converted } - : converted; - } + // Operands that cannot merge into their field's entry without one silently + // replacing the other; ANDed in below so the engine intersects them. + const conjuncts: Record[] = []; + for (const f of normalizeAnalyticsFilters(query)) { + const fieldName = this.resolveFieldName(cube, f.member, 'any'); + const extra = this.mergeFilterOperand(filter, fieldName, this.convertFilter(f.operator, f.values)); + if (extra) conjuncts.push(extra); + } + // #3650 — and the time-dimension WINDOWS, through the SAME merge, so a + // `dateRange` and a caller `where` bound on one field compose instead of + // clobbering each other. + for (const { field, bounds } of this.dateRangeBounds(cube, query)) { + const extra = this.mergeFilterOperand(filter, field, bounds); + if (extra) conjuncts.push(extra); + } + if (conjuncts.length > 0) { + filter.$and = [...(Array.isArray(filter.$and) ? filter.$and : []), ...conjuncts]; } // #3654 — classify cross-object references. A cross-object DIMENSION within @@ -270,11 +276,12 @@ export class ObjectQLStrategy implements AnalyticsStrategy { // The cross-object guard runs here for the same reason: this must not // render SQL for a query `execute()` would reject outright (#3654). // - // Faithfulness cuts both ways: `execute()` does NOT apply - // `timeDimensions[].dateRange` on this path (only `where` reaches the - // engine), so neither does this. Rendering a BETWEEN here would invent a - // predicate the ObjectQL path never applies. That gap is real but separate - // — filed as #3650, not papered over here. + // Faithfulness cuts both ways: the time-dimension WINDOWS render too, from + // the same `dateRangeBounds` lowering `execute()` sends to the engine + // (#3650). This comment used to explain why a BETWEEN was deliberately + // absent — because `execute()` dropped the window and rendering one would + // have invented a predicate. Now that it applies the window, omitting it + // here would be the lie in the other direction. // (The cross-object envelope was already enforced by `planCrossObject` above, // so `/analytics/sql` rejects the same out-of-envelope set `execute()` does.) @@ -288,6 +295,12 @@ export class ObjectQLStrategy implements AnalyticsStrategy { ); if (clause) whereParts.push(clause); } + // Bounds bind as `$n` placeholders like every other comparand: this string + // travels to the browser, and a window can carry tenant-derived dates. + for (const { field, bounds } of this.dateRangeBounds(cube, query)) { + params.push(bounds.$gte, bounds.$lte); + whereParts.push(`${field} BETWEEN $${params.length - 1} AND $${params.length}`); + } // Read scope last, so it reads as the outermost constraint. Compiled by the // same fail-closed compiler `NativeSQLStrategy` uses — it throws rather than // drop a predicate, which is the correct posture even for a display string: @@ -388,6 +401,19 @@ export class ObjectQLStrategy implements AnalyticsStrategy { ): CrossObjectPlan | null { const baseObject = this.extractObjectName(cube); + // A date bucket over a related object's field is not supported. Checked + // FIRST: since #3650 a `dateRange` also lands in `filter`, so a cross-object + // time dimension would otherwise be reported as a "cross-object filter" — + // true of the lowered predicate, but not what the author wrote. + for (const td of query.timeDimensions ?? []) { + const field = this.resolveFieldName(cube, td.dimension, 'dimension'); + if (this.isCrossObjectField(cube, field, baseObject)) { + throw new Error( + `[Analytics] ObjectQLStrategy cannot bucket a cross-object time dimension ("${field}").`, + ); + } + } + // A cross-object MEASURE or FILTER can only be evaluated with a real join. const nonDim = [ ...(query.measures ?? []).map((m) => ({ where: 'measure', field: this.resolveMeasureAggregation(cube, m).field })), @@ -416,15 +442,6 @@ export class ObjectQLStrategy implements AnalyticsStrategy { } crossDims.push({ outputName: dim, fkField: alias, attr, refObject: cube.joins?.[alias]?.name ?? alias }); } - // A date bucket over a related object's field is not supported. - for (const td of query.timeDimensions ?? []) { - const field = this.resolveFieldName(cube, td.dimension, 'dimension'); - if (this.isCrossObjectField(cube, field, baseObject)) { - throw new Error( - `[Analytics] ObjectQLStrategy cannot bucket a cross-object time dimension ("${field}").`, - ); - } - } if (crossDims.length === 0) return null; @@ -684,6 +701,99 @@ export class ObjectQLStrategy implements AnalyticsStrategy { return { field: '*', method: 'count' }; } + /** + * AND one more operand onto `filter[field]`, merging operator objects rather + * than overwriting them. Returns a standalone conjunct when the two cannot + * share one entry, or `null` when the merge absorbed the operand. + * + * Every predicate this strategy contributes goes through here — the caller's + * `where` and the time-dimension `dateRange` alike. Two operands on one field + * are the normal case (`{$gte}` from a `where` plus `{$gte,$lte}` from a + * window on `close_date`), and a plain assignment would keep only the last: + * that is how a range used to lose a bound. + * + * Spreading is sound only while the operands name DIFFERENT operators. Where + * they collide — two `$gte` bounds on one field, which a window makes routine + * and which a `where` can already produce on its own through `$and` — the + * spread keeps whichever came last and WIDENS the query. Same for a bare + * equality meeting an operator object: neither can absorb the other. Those + * are handed back for the caller to AND in separately, so the engine + * intersects them instead of the strategy picking a winner. + */ + private mergeFilterOperand( + filter: Record, + field: string, + operand: unknown, + ): Record | null { + const existing = filter[field]; + if (existing === undefined) { + filter[field] = operand; + return null; + } + const mergeable = (v: unknown): v is Record => + !!v && typeof v === 'object' && !Array.isArray(v); + if (!mergeable(existing) || !mergeable(operand)) return { [field]: operand }; + if (Object.keys(operand).some((op) => op in existing)) return { [field]: operand }; + filter[field] = { ...existing, ...operand }; + return null; + } + + /** + * Lower `timeDimensions[].dateRange` into resolved-field bounds (#3650). + * + * `dateRange` states a WINDOW on a time dimension; it is a SIBLING of `where`, + * never folded into it. `normalizeAnalyticsFilters` reads only `where`, so + * this path used to drop the window on the floor — no error, just every row + * ever recorded. Nor is that a corner case: `NativeSQLStrategy.canHandle` + * declines any query carrying a `granularity`, so a date-bucketed trend lands + * HERE on every driver — and "bucketed trend" is precisely the shape that also + * carries a range ("last 12 months", "this quarter"). + * + * Bounds are inclusive on both ends — the same `$gte`/`$lte` pair + * `NativeSQLStrategy` binds as `BETWEEN` and the memory driver builds as a + * `$match`, so one dashboard reads the same on every driver. + * + * Comparands are coerced by the SAME helper the `where` path uses, so an + * epoch-ms bound recovers as a number and an ISO string stays a string. No + * STORAGE coercion happens here, deliberately: `NativeSQLStrategy` needs + * `coerceTemporal` because it binds into raw SQL and had to learn that a + * SQLite `Field.datetime` is an INTEGER epoch (#2034); this path goes through + * `engine.aggregate()`, where the driver's own CRUD filter coercion applies — + * the very coercion that already makes a `where` bound on that same column + * work today. + * + * A bare-string `dateRange` degenerates to the single point `[s, s]`, matching + * `NativeSQLStrategy`. Relative phrases ("Last 7 days") are NOT resolved here; + * neither SQL path resolves them, and inventing a second interpretation on the + * driver-independent path is how the two would drift apart again. + * + * An oddly-sized array (the schema types `dateRange` as a plain `string[]`) + * takes its first two entries, a one-entry array degenerating to a point. + * `NativeSQLStrategy` drops such a window entirely — but "drop the window" + * means "plot all of history", which is the very failure this fixes, so the + * fallback here errs toward the narrower query instead. + */ + private dateRangeBounds( + cube: Cube, + query: AnalyticsQuery, + ): Array<{ field: string; bounds: Record }> { + const out: Array<{ field: string; bounds: Record }> = []; + for (const td of query.timeDimensions ?? []) { + if (!td.dateRange) continue; + const range = Array.isArray(td.dateRange) ? td.dateRange : [td.dateRange, td.dateRange]; + const [start, end = start] = range; + if (start == null) continue; + out.push({ + field: this.resolveFieldName(cube, td.dimension, 'dimension'), + bounds: { + $gte: coerceFilterValueForObjectQL(String(start)), + $lte: coerceFilterValueForObjectQL(String(end)), + }, + }); + } + return out; + } + private convertFilter(operator: string, values?: string[]): unknown { if (operator === 'set') return { $ne: null }; if (operator === 'notSet') return null; From f584845c5c8f7bde54d5997546aac5174b6e1987 Mon Sep 17 00:00:00 2001 From: Jack Zhuang <277994282+os-zhuang@users.noreply.github.com> Date: Tue, 28 Jul 2026 11:07:45 +0800 Subject: [PATCH 2/2] test(driver-sql): pin the storage-coercion assumption #3650 rests on MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `ObjectQLStrategy` hands `engine.aggregate()` uncoerced ISO bounds and relies on the driver's own CRUD filter coercion — the justification for NOT doing what `NativeSQLStrategy` does with `coerceTemporal` (#2034). That is an assumption about the driver, not the strategy, so it is pinned against real SQLite: a `Field.datetime` column stores INTEGER epoch ms, where an uncoerced ISO TEXT comparand would match nothing and quietly make the #3650 fix a no-op on the column type dashboards use most. The assumption holds — the window applies on both `datetime` and `date` storage. Writing it surfaced an unrelated pre-existing gap on the SAME query shape: bucketing an epoch-stored `datetime` collapses every row into one NULL bucket (`strftime` reads a bare INTEGER as a Julian day; SQLite advertises the granularity so the engine never falls back to in-memory bucketing). Not fixed here — pinned as a KNOWN GAP test so it is not later rediscovered as "the dateRange fix did nothing", with the assertion it should become once fixed. Co-Authored-By: Claude --- ...l-driver-aggregate-datetime-window.test.ts | 146 ++++++++++++++++++ 1 file changed, 146 insertions(+) create mode 100644 packages/plugins/driver-sql/src/sql-driver-aggregate-datetime-window.test.ts diff --git a/packages/plugins/driver-sql/src/sql-driver-aggregate-datetime-window.test.ts b/packages/plugins/driver-sql/src/sql-driver-aggregate-datetime-window.test.ts new file mode 100644 index 0000000000..ab1abfdc9e --- /dev/null +++ b/packages/plugins/driver-sql/src/sql-driver-aggregate-datetime-window.test.ts @@ -0,0 +1,146 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * The load-bearing assumption behind #3650. + * + * `ObjectQLStrategy` lowers `timeDimensions[].dateRange` to `{$gte, $lte}` ISO + * strings and hands them to `engine.aggregate()` WITHOUT the storage coercion + * `NativeSQLStrategy` performs. The justification is that this path goes through + * the driver's own CRUD filter coercion, whereas the raw-SQL path binds straight + * into a statement and therefore had to learn about SQLite's INTEGER epoch + * itself (#2034). + * + * That is an assumption about the driver, not about the strategy — so it is + * pinned here against a real SQLite database. A `Field.datetime` column is + * stored as INTEGER epoch ms; an uncoerced ISO TEXT comparand matches NOTHING + * (TEXT sorts after every INTEGER), which would have made the #3650 fix a no-op + * on exactly the column type dashboards use most. + * + * `Field.date` (ISO TEXT storage) is covered alongside it, since a dataset may + * bucket on either. + */ + +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { SqlDriver } from '../src/index.js'; + +const TABLE = 'opportunity'; + +describe('SqlDriver.aggregate — ISO window over epoch-stored datetime (#3650)', () => { + let driver: SqlDriver; + + beforeEach(async () => { + driver = new SqlDriver({ + client: 'better-sqlite3', + connection: { filename: ':memory:' }, + useNullAsDefault: true, + }); + + await driver.initObjects([ + { + name: TABLE, + fields: { + closed_at: { type: 'datetime' }, // INTEGER epoch ms under better-sqlite3 + closed_on: { type: 'date' }, // YYYY-MM-DD TEXT + amount: { type: 'number' }, + }, + }, + ]); + + // Inserted as real Date objects, the path the seed loader takes. + const rows = [ + ['o1', '2025-11-15T09:00:00Z', '2025-11-15', 900], // before window + ['o2', '2026-01-10T09:00:00Z', '2026-01-10', 100], // inside + ['o3', '2026-01-20T09:00:00Z', '2026-01-20', 200], // inside + ['o4', '2026-02-14T09:00:00Z', '2026-02-14', 30], // inside (Feb) + ['o5', '2026-06-05T09:00:00Z', '2026-06-05', 700], // after window + ] as const; + for (const [id, at, on, amount] of rows) { + await driver.create( + TABLE, + { id, closed_at: new Date(at), closed_on: on, amount }, + { bypassTenantAudit: true }, + ); + } + }); + + afterEach(async () => { + await driver.disconnect(); + }); + + /** Exactly what the strategy emits: an inclusive ISO range, uncoerced. */ + const window = (col: string) => ({ + [col]: { $gte: '2026-01-01', $lte: '2026-02-28' }, + }); + + it('confines a datetime aggregate to the window (the epoch-affinity trap)', async () => { + const rows = await driver.aggregate(TABLE, { + aggregations: [ + { function: 'sum', field: 'amount', alias: 'total' }, + { function: 'count', alias: 'n' }, + ], + where: window('closed_at'), + } as any); + + // 330 over 3 rows. An uncoerced TEXT-vs-INTEGER compare yields 0 rows; + // a dropped window yields 1930 over 5. + expect(Number(rows[0].total)).toBe(330); + expect(Number(rows[0].n)).toBe(3); + }); + + it('buckets a TEXT-stored date inside the window — the shape #3650 is about', async () => { + // A `granularity` is what makes NativeSQLStrategy decline, so window + + // bucketing together is the combination that reaches the ObjectQL path. + const rows = await driver.aggregate(TABLE, { + groupBy: [{ field: 'closed_on', dateGranularity: 'month' }], + aggregations: [{ function: 'sum', field: 'amount', alias: 'total' }], + where: window('closed_on'), + } as any); + + const byMonth = Object.fromEntries(rows.map((r: any) => [r.closed_on, Number(r.total)])); + expect(byMonth).toEqual({ '2026-01': 300, '2026-02': 30 }); + }); + + it('KNOWN GAP — bucketing an EPOCH-stored datetime collapses into one null bucket', async () => { + // Pre-existing, unrelated to #3650, and deliberately NOT fixed here — but + // it lands on the exact same query shape, so it is pinned rather than left + // to be rediscovered as "the dateRange fix did nothing". + // + // SQLite advertises `queryDateGranularity.month`, so `engine.aggregate` + // pushes the bucketing down to the driver — `engine.ts` only falls back to + // in-memory bucketing when a granularity is UNSUPPORTED or a non-UTC + // timezone is in play, neither of which applies here. The dialect + // expression is `strftime('%Y-%m', col)`, and SQLite reads a bare INTEGER + // as a Julian day number; an epoch-ms value is far outside the legal range, + // so every row buckets as NULL. + const rows = await driver.aggregate(TABLE, { + groupBy: [{ field: 'closed_at', dateGranularity: 'month' }], + aggregations: [{ function: 'sum', field: 'amount', alias: 'total' }], + where: window('closed_at'), + } as any); + + // The WINDOW works — the total is the in-window 330, not the full 1930 — + // which is what #3650 is responsible for. The BUCKETS are what is broken. + // When that is fixed, this becomes `{ '2026-01': 300, '2026-02': 30 }`. + const byMonth = Object.fromEntries(rows.map((r: any) => [String(r.closed_at), Number(r.total)])); + expect(byMonth).toEqual({ null: 330 }); + }); + + it('confines a date (TEXT-stored) aggregate to the same window', async () => { + const rows = await driver.aggregate(TABLE, { + aggregations: [{ function: 'sum', field: 'amount', alias: 'total' }], + where: window('closed_on'), + } as any); + + expect(Number(rows[0].total)).toBe(330); + }); + + it('excludes the whole table when the window selects nothing', async () => { + const rows = await driver.aggregate(TABLE, { + aggregations: [{ function: 'count', alias: 'n' }], + where: { closed_at: { $gte: '2027-01-01', $lte: '2027-12-31' } }, + } as any); + + // An empty window must read as zero, not as "filter ignored → 5". + expect(Number(rows[0]?.n ?? 0)).toBe(0); + }); +});