Skip to content

Commit 4a7a532

Browse files
os-zhuangclaude
andcommitted
fix(analytics): make organization timezone drive date-dimension bucketing (#1982)
Date-bucketed analytics silently ignored the reference timezone end-to-end. Three independent seams were broken; all three are needed for an org's localization timezone to actually change a date-bucketed chart: 1. service-analytics — NativeSQLStrategy (priority 10) won every cube/dataset query on a SQL driver, but it groups by the raw column (no date_trunc) and ignores `timezone`, so date dimensions never bucketed (one row per raw timestamp) and a non-UTC zone was dropped. It now declines queries carrying a timeDimensions[].granularity, handing them to ObjectQLStrategy → engine.aggregate (native bucketing UTC-safe, uniform in-memory when non-UTC). 2. objectql — the in-memory `count` aggregation treated the `*` count-all sentinel (Cube `count` measure / fieldless dataset `count`, both compiled to sql:'*') as a column name → counted 0 for every bucket. The driver COUNT(*) masked it; the in-memory path (non-UTC date buckets, driver-rest/-memory) returned zeros. `*` is now counted as all rows. 3. rest — resolveExecCtx never resolved the localization timezone/locale, so /analytics/dataset/query always ran with timezone:'UTC'. It now resolves them through the settings service (4-tier cascade incl. the OS_LOCALIZATION_TIMEZONE env override), mirroring the dispatcher path. Verified end-to-end against example-crm: an org tz of America/Los_Angeles now buckets a 03:5x UTC lead into 2026-06-17 (cnt 5) vs UTC's 2026-06-18 (cnt 5). Regression tests added for seams 1 and 2; full suites green (objectql 660, service-analytics 127, rest 121). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent 4f5c9c3 commit 4a7a532

7 files changed

Lines changed: 184 additions & 2 deletions

File tree

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
---
2+
"@objectstack/service-analytics": patch
3+
"@objectstack/objectql": patch
4+
"@objectstack/rest": patch
5+
---
6+
7+
fix(analytics): make organization timezone actually drive date-dimension bucketing (ADR-0053 Phase 2, #1982)
8+
9+
Date-bucketed analytics silently ignored the reference timezone end-to-end. Three independent seams were broken:
10+
11+
- **service-analytics**`NativeSQLStrategy` (priority 10) won every cube/dataset query on a SQL driver, but it groups by the raw column (no `date_trunc`) and ignores `timezone`, so a date dimension never bucketed (one row per raw timestamp) and a non-UTC zone was dropped. It now declines queries that carry a `timeDimensions[].granularity`, handing them to `ObjectQLStrategy``engine.aggregate` (native bucketing when UTC-safe, uniform in-memory bucketing when non-UTC).
12+
- **objectql** — the in-memory `count` aggregation treated the `*` count-all sentinel (the Cube `count` measure / a fieldless dataset `count`, both compiled to `sql: '*'`) as a column name, counting non-null of a non-existent property → `0` for every bucket. The driver's `COUNT(*)` masked it; the in-memory path (non-UTC date buckets, `driver-rest`/`driver-memory`) returned zeros. `*` is now counted as all rows.
13+
- **rest**`resolveExecCtx` never resolved the localization timezone/locale, so `/analytics/dataset/query` always ran with `timezone: 'UTC'`. It now resolves them through the `settings` service (honouring the 4-tier cascade incl. the `OS_LOCALIZATION_TIMEZONE` env override), mirroring the dispatcher path.

packages/objectql/src/in-memory-aggregation.test.ts

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -158,3 +158,37 @@ describe('bucketDateValue', () => {
158158
});
159159
});
160160
});
161+
162+
describe('count-all `*` sentinel (regression #1982)', () => {
163+
// The Cube `count` measure and a dataset `count` with no field compile to
164+
// `sql: '*'`. The in-memory count branch treated `'*'` as a column name and
165+
// counted non-null of a non-existent property → 0 for every bucket. The
166+
// driver's `COUNT(*)` masked it; the tz≠UTC date-bucket path (which always
167+
// runs in-memory) surfaced it as a 0-count bucket with a correct label.
168+
const evts = [
169+
{ closed_at: '2024-03-01T03:00:00.000Z' }, // NY: 02-29
170+
{ closed_at: '2024-03-01T07:00:00.000Z' }, // NY: 03-01
171+
{ closed_at: '2024-03-01T09:00:00.000Z' }, // NY: 03-01
172+
];
173+
174+
it("counts all rows for a fieldless / `'*'` count, ungrouped", () => {
175+
const out = applyInMemoryAggregation(evts, {
176+
aggregations: [{ function: 'count', field: '*', alias: 'n' }],
177+
});
178+
expect(out).toEqual([{ n: 3 }]);
179+
});
180+
181+
it("counts rows per tz bucket for a `'*'` count (was 0 before the fix)", () => {
182+
const ast = {
183+
groupBy: [{ field: 'closed_at', dateGranularity: 'day' as const }],
184+
aggregations: [{ function: 'count', field: '*', alias: 'n' }],
185+
};
186+
const ny = applyInMemoryAggregation(evts, ast, 'America/New_York').sort(
187+
(a, b) => String(a.closed_at).localeCompare(String(b.closed_at)),
188+
);
189+
expect(ny).toEqual([
190+
{ closed_at: '2024-02-29', n: 1 },
191+
{ closed_at: '2024-03-01', n: 2 },
192+
]);
193+
});
194+
});

packages/objectql/src/in-memory-aggregation.ts

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -93,7 +93,13 @@ function aggregateBucket(rows: any[], aggregations: AggregationNode[]): Record<s
9393
const alias = agg.alias;
9494
const fn = agg.function;
9595
if (fn === 'count') {
96-
if (!agg.field) {
96+
// `*` is the count-all sentinel: the Cube `count` measure and a dataset
97+
// `count` with no field both compile to `sql: '*'` (→ SQL `COUNT(*)`).
98+
// A row never has a literal `'*'` property, so the non-null branch below
99+
// counted zero for every bucket — the driver's `COUNT(*)` masked it, but
100+
// the in-memory path (tz≠UTC date buckets per #1982, driver-rest/-memory)
101+
// returned 0. Treat `'*'` like a fieldless count over the bucket's rows.
102+
if (!agg.field || agg.field === '*') {
97103
out[alias] = rows.length;
98104
} else {
99105
out[alias] = rows.reduce(

packages/rest/src/rest-api-plugin.ts

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -163,6 +163,16 @@ export function createRestApiPlugin(config: RestApiPluginConfig = {}): Plugin {
163163
} catch { return undefined; }
164164
};
165165

166+
// Settings service resolver — used by resolveExecCtx to resolve the
167+
// reference timezone/locale (localization manifest) through the 4-tier
168+
// cascade incl. the `OS_LOCALIZATION_TIMEZONE` env override. Returns
169+
// undefined when no settings service is registered (UTC default).
170+
const settingsServiceProvider = async (_environmentId?: string): Promise<any | undefined> => {
171+
try {
172+
return ctx.getService<any>('settings');
173+
} catch { return undefined; }
174+
};
175+
166176
if (!server) {
167177
ctx.logger.warn(`RestApiPlugin: HTTP Server service '${serverService}' not found. REST routes skipped.`);
168178
return;
@@ -176,7 +186,7 @@ export function createRestApiPlugin(config: RestApiPluginConfig = {}): Plugin {
176186
ctx.logger.info('Hydrating REST API from Protocol...');
177187

178188
try {
179-
const restServer = new RestServer(server, protocol, config.api as any, kernelManager, envRegistry, defaultEnvironmentIdProvider, authServiceProvider, objectQLProvider, emailServiceProvider, sharingServiceProvider, reportsServiceProvider, approvalsServiceProvider, sharingRulesServiceProvider, i18nServiceProvider, analyticsServiceProvider);
189+
const restServer = new RestServer(server, protocol, config.api as any, kernelManager, envRegistry, defaultEnvironmentIdProvider, authServiceProvider, objectQLProvider, emailServiceProvider, sharingServiceProvider, reportsServiceProvider, approvalsServiceProvider, sharingRulesServiceProvider, i18nServiceProvider, analyticsServiceProvider, settingsServiceProvider);
180190
restServer.registerRoutes();
181191

182192
ctx.logger.info('REST API successfully registered');

packages/rest/src/rest-server.ts

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -541,6 +541,7 @@ export class RestServer {
541541
private sharingRulesServiceProvider?: (environmentId?: string) => Promise<any | undefined>;
542542
private i18nServiceProvider?: (environmentId?: string) => Promise<any | undefined>;
543543
private analyticsServiceProvider?: (environmentId?: string) => Promise<any | undefined>;
544+
private settingsServiceProvider?: (environmentId?: string) => Promise<any | undefined>;
544545

545546
constructor(
546547
server: IHttpServer,
@@ -558,6 +559,7 @@ export class RestServer {
558559
sharingRulesServiceProvider?: (environmentId?: string) => Promise<any | undefined>,
559560
i18nServiceProvider?: (environmentId?: string) => Promise<any | undefined>,
560561
analyticsServiceProvider?: (environmentId?: string) => Promise<any | undefined>,
562+
settingsServiceProvider?: (environmentId?: string) => Promise<any | undefined>,
561563
) {
562564
this.protocol = protocol;
563565
this.config = this.normalizeConfig(config);
@@ -574,6 +576,7 @@ export class RestServer {
574576
this.sharingRulesServiceProvider = sharingRulesServiceProvider;
575577
this.i18nServiceProvider = i18nServiceProvider;
576578
this.analyticsServiceProvider = analyticsServiceProvider;
579+
this.settingsServiceProvider = settingsServiceProvider;
577580
}
578581

579582
/**
@@ -1021,6 +1024,31 @@ export class RestServer {
10211024
}
10221025
} catch { /* fall back to self-only */ }
10231026
}
1027+
// Reference timezone + locale for date rendering and analytics date
1028+
// bucketing (ADR-0053 Phase 2 / localization manifest #2006). Resolved
1029+
// through the `settings` service so the 4-tier cascade — including the
1030+
// env override `OS_LOCALIZATION_TIMEZONE` — is honoured; `sys_setting`
1031+
// rows alone would miss the env/global tiers. Best-effort: any failure
1032+
// leaves the engine's UTC default. Mirrors the dispatcher path's
1033+
// `resolveLocalization` (runtime/security/resolve-execution-context.ts).
1034+
let timezone: string | undefined;
1035+
let locale: string | undefined;
1036+
try {
1037+
const settings = this.settingsServiceProvider
1038+
? await this.settingsServiceProvider(environmentId).catch(() => undefined)
1039+
: undefined;
1040+
if (settings && typeof settings.get === 'function') {
1041+
const sctx = { tenantId, userId };
1042+
const [tzRes, localeRes] = await Promise.all([
1043+
settings.get('localization', 'timezone', sctx).catch(() => undefined),
1044+
settings.get('localization', 'locale', sctx).catch(() => undefined),
1045+
]);
1046+
const tzVal = tzRes?.value;
1047+
const localeVal = localeRes?.value;
1048+
if (typeof tzVal === 'string' && tzVal.trim()) timezone = tzVal.trim();
1049+
if (typeof localeVal === 'string' && localeVal.trim()) locale = localeVal.trim();
1050+
}
1051+
} catch { /* best-effort — fall back to engine UTC default */ }
10241052
return {
10251053
userId,
10261054
tenantId,
@@ -1029,6 +1057,8 @@ export class RestServer {
10291057
systemPermissions,
10301058
isSystem: false,
10311059
org_user_ids,
1060+
...(timezone ? { timezone } : {}),
1061+
...(locale ? { locale } : {}),
10321062
} as any;
10331063
} catch {
10341064
return undefined;
Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,80 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
//
3+
// Regression (ADR-0053 Phase 2, #1982): on a SQL driver — where BOTH
4+
// `nativeSql` and `objectqlAggregate` capabilities are advertised (the real
5+
// SQLite/Postgres/MySQL deployment) — a date-bucketed query must route to the
6+
// ObjectQLStrategy so `engine.aggregate` buckets by granularity and honours a
7+
// non-UTC reference timezone (in-memory). NativeSQLStrategy groups by the raw
8+
// column (`GROUP BY <col>`, no `date_trunc`) and ignores `timezone`; because it
9+
// has higher precedence (priority 10 < 20) it silently won every cube/dataset
10+
// query, producing one bucket per row and tz-invariant results. The earlier
11+
// granularity test passed only because it forced `nativeSql: false`, masking
12+
// the real misrouting. This test pins the production capability shape.
13+
14+
import { describe, it, expect } from 'vitest';
15+
import { DatasetSchema } from '@objectstack/spec/ui';
16+
import { AnalyticsService } from '../analytics-service.js';
17+
18+
const dataset = DatasetSchema.parse({
19+
name: 'pipeline',
20+
label: 'Pipeline',
21+
object: 'opportunity',
22+
dimensions: [
23+
{ name: 'stage', field: 'stage', type: 'string' },
24+
{ name: 'close_date', field: 'close_date', type: 'date' },
25+
],
26+
measures: [{ name: 'opp_count', aggregate: 'count' }],
27+
});
28+
29+
// Mirror a real SQL deployment: native SQL AND objectql aggregate both available.
30+
const PROD_CAPS = { nativeSql: true, objectqlAggregate: true, inMemory: true };
31+
32+
function buildService() {
33+
const rawSqlCalls: Array<{ sql: string }> = [];
34+
const aggregateCalls: Array<{ groupBy?: unknown[]; timezone?: string }> = [];
35+
const svc = new AnalyticsService({
36+
queryCapabilities: () => PROD_CAPS,
37+
executeRawSql: async (_object, sql) => {
38+
rawSqlCalls.push({ sql });
39+
return [{ 'close_date': '2026-06-17T01:44:15.667Z', opp_count: 1 }];
40+
},
41+
executeAggregate: async (_object, options) => {
42+
aggregateCalls.push({ groupBy: options.groupBy as unknown[], timezone: options.timezone });
43+
return [{ close_date: '2026-06', opp_count: 6 }];
44+
},
45+
});
46+
return { svc, rawSqlCalls, aggregateCalls };
47+
}
48+
49+
describe('NativeSQLStrategy declines date-granularity queries (regression #1982)', () => {
50+
it('routes a granularity timeDimension to engine.aggregate, NOT raw SQL', async () => {
51+
const { svc, rawSqlCalls, aggregateCalls } = buildService();
52+
53+
await svc.queryDataset!(dataset, {
54+
dimensions: ['close_date'],
55+
measures: ['opp_count'],
56+
timeDimensions: [{ dimension: 'close_date', granularity: 'month' }],
57+
timezone: 'America/New_York',
58+
});
59+
60+
// ObjectQLStrategy handled it (native SQL would have lost the bucket + tz).
61+
expect(rawSqlCalls).toHaveLength(0);
62+
expect(aggregateCalls).toHaveLength(1);
63+
// Bucketed groupBy + reference tz reach the engine.
64+
expect(aggregateCalls[0].groupBy).toEqual([{ field: 'close_date', dateGranularity: 'month' }]);
65+
expect(aggregateCalls[0].timezone).toBe('America/New_York');
66+
});
67+
68+
it('still uses native SQL for a plain (non-bucketed) query', async () => {
69+
const { svc, rawSqlCalls, aggregateCalls } = buildService();
70+
71+
await svc.queryDataset!(dataset, {
72+
dimensions: ['stage'],
73+
measures: ['opp_count'],
74+
});
75+
76+
// No granularity → native SQL is correct and preferred; we did not over-decline.
77+
expect(rawSqlCalls).toHaveLength(1);
78+
expect(aggregateCalls).toHaveLength(0);
79+
});
80+
});

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

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,15 @@ export class NativeSQLStrategy implements AnalyticsStrategy {
1919

2020
canHandle(query: AnalyticsQuery, ctx: StrategyContext): boolean {
2121
if (!query.cube) return false;
22+
// This strategy groups by the raw column expression (`GROUP BY <col>`) and
23+
// emits no `date_trunc` — it cannot bucket a date dimension to a coarser
24+
// granularity, nor resolve buckets on a non-UTC calendar. When the query
25+
// asks for granularity bucketing we therefore DECLINE so the lower-priority
26+
// ObjectQLStrategy handles it via `engine.aggregate` (native date_trunc when
27+
// UTC-safe, else uniform in-memory bucketing). Without this, a date-bucketed
28+
// query silently grouped by the raw timestamp — one bucket per row — and a
29+
// non-UTC reference timezone was ignored entirely (ADR-0053 Phase 2, #1982).
30+
if (query.timeDimensions?.some((td) => !!td.granularity)) return false;
2231
const caps = ctx.queryCapabilities(query.cube);
2332
return caps.nativeSql && typeof ctx.executeRawSql === 'function';
2433
}

0 commit comments

Comments
 (0)