Skip to content

Commit e83d579

Browse files
os-zhuangclaude
andcommitted
fix(analytics): render the read scope in ObjectQLStrategy.generateSql (#3654 residual 2)
generateSql (the representative statement echoed to /analytics/sql and dataset responses) rendered the query's own filters but NOT the tenant/RLS read scope that execute() injects via withReadScope — so the previewed WHERE understated what actually runs. It now renders the base object's scope too (parameterized, $n placeholders), ANDed with the query filters. For consistency generateSql also applies the #3654 cross-object guard, so /analytics/sql no longer previews a dotted-column statement for a query execute() refuses to run — same clear error instead of misleading SQL. Display-only (no data leak — the string is documentation, values stay placeholders); no change when no read-scope provider is configured. Tests: generateSql now renders "opportunity"."organization_id" = $n and ANDs it with the query filter; a cross-object preview rejects; the no-provider case is unchanged. Reverting the change turns the three load-bearing assertions red. Co-Authored-By: Claude <noreply@anthropic.com>
1 parent adabaa8 commit e83d579

3 files changed

Lines changed: 116 additions & 1 deletion

File tree

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
---
2+
"@objectstack/service-analytics": patch
3+
---
4+
5+
fix(analytics): show the read scope in ObjectQLStrategy's previewed SQL (#3654 residual 2)
6+
7+
`ObjectQLStrategy.generateSql` (the representative statement echoed to
8+
`/analytics/sql` and dataset responses) rendered the query's own filters but NOT
9+
the tenant/RLS read scope that `execute()` injects. The preview therefore
10+
understated the real WHERE — an author debugging a widget saw fewer constraints
11+
than the query actually ran with. It now renders the base object's read scope
12+
too, parameterized, alongside the query filters.
13+
14+
For consistency, `generateSql` also applies the #3654 cross-object guard, so
15+
`/analytics/sql` no longer previews a dotted-column statement for a cross-object
16+
query that `execute()` refuses to run (the engine cannot join in an aggregate) —
17+
it returns the same clear error instead.
18+
19+
Display-only: the previewed string is documentation, never the executed
20+
statement, and carries no data (filter values stay `$n` placeholders). No change
21+
when no read-scope provider is configured.

packages/services/service-analytics/src/__tests__/objectql-read-scope.test.ts

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -267,3 +267,63 @@ describe('ObjectQLStrategy — cross-object references are fail-closed (#3654, s
267267
expect(wasExecuted()).toBe(false);
268268
});
269269
});
270+
271+
describe('ObjectQLStrategy.generateSql — read scope in the previewed WHERE (#3654 residual 2)', () => {
272+
it('renders the tenant/RLS predicate the query actually runs with', async () => {
273+
const { sql, params } = await makeService([]).generateSql(
274+
{ cube: 'sales', dimensions: ['region'], measures: ['revenue'] },
275+
ctxA,
276+
);
277+
// The previewed SQL must carry the scope, not just the SELECT/GROUP BY —
278+
// before #3654 residual 2 the WHERE omitted it and understated what runs.
279+
expect(sql).toMatch(/WHERE .*"opportunity"\."organization_id" = \$\d+/);
280+
expect(params).toContain('org_A');
281+
});
282+
283+
it('ANDs the query filter and the scope together in the preview', async () => {
284+
const { sql, params } = await makeService([]).generateSql(
285+
{
286+
cube: 'sales',
287+
dimensions: ['region'],
288+
measures: ['revenue'],
289+
where: { region: 'West' },
290+
},
291+
ctxA,
292+
);
293+
expect(sql).toContain('WHERE');
294+
expect(sql).toMatch(/region .*=.* \$\d+/);
295+
expect(sql).toMatch(/"opportunity"\."organization_id" = \$\d+/);
296+
expect(params).toEqual(expect.arrayContaining(['West', 'org_A']));
297+
});
298+
299+
it('omits the scope predicate when no provider is configured (unchanged)', async () => {
300+
const { sql } = await makeService([], { getReadScope: undefined }).generateSql(
301+
{ cube: 'sales', dimensions: ['region'], measures: ['revenue'] },
302+
ctxA,
303+
);
304+
expect(sql).not.toContain('organization_id');
305+
});
306+
307+
it('rejects a cross-object preview (no dotted-column SQL for a query execute() refuses)', async () => {
308+
const compiled = compileDataset(
309+
DatasetSchema.parse({
310+
name: 'sql_by_account',
311+
label: 'x',
312+
object: 'opportunity',
313+
include: ['account'],
314+
dimensions: [{ name: 'region', field: 'account.region', type: 'string' }],
315+
measures: [{ name: 'revenue', aggregate: 'sum', field: 'amount' }],
316+
}),
317+
);
318+
const svc = new AnalyticsService({
319+
cubes: [compiled.cube],
320+
queryCapabilities: objectqlOnly,
321+
executeAggregate: async () => [],
322+
getReadScope: readScope,
323+
getAllowedRelationships: () => compiled.allowedRelationships,
324+
});
325+
await expect(
326+
svc.generateSql({ cube: 'sql_by_account', dimensions: ['region'], measures: ['revenue'] }, ctxA),
327+
).rejects.toThrow(/cannot group or aggregate across a relationship/);
328+
});
329+
});

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

Lines changed: 35 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,10 @@
11
// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license.
22

33
import type { AnalyticsQuery, AnalyticsResult } from '@objectstack/spec/contracts';
4-
import type { Cube } from '@objectstack/spec/data';
4+
import type { Cube, FilterCondition } from '@objectstack/spec/data';
55
import type { AnalyticsStrategy, StrategyContext } from './types.js';
66
import { normalizeAnalyticsFilters, coerceFilterValueForObjectQL } from './filter-normalizer.js';
7+
import { compileScopedFilterToSql } from '../read-scope-sql.js';
78

89
/**
910
* ObjectQLStrategy — Priority 2
@@ -178,6 +179,21 @@ export class ObjectQLStrategy implements AnalyticsStrategy {
178179
return gran ? `date_trunc('${gran}', ${col})` : col;
179180
};
180181

182+
// #3654 — reject a cross-object query HERE too, so `/analytics/sql` never
183+
// previews a dotted-column statement that `execute()` refuses to run (the
184+
// engine cannot join in an aggregate). Reconstruct the resolved field names
185+
// the shared guard inspects.
186+
const guardGroupBy = [
187+
...(query.dimensions ?? []).map((d) => this.resolveFieldName(cube, d, 'dimension')),
188+
...[...granByDim.keys()]
189+
.filter((d) => !query.dimensions?.includes(d))
190+
.map((d) => this.resolveFieldName(cube, d, 'dimension')),
191+
];
192+
const guardFilter = Object.fromEntries(
193+
normalizeAnalyticsFilters(query).map((f) => [this.resolveFieldName(cube, f.member, 'any'), true]),
194+
);
195+
this.assertNoCrossObjectReferences(cube, query, guardGroupBy, guardFilter);
196+
181197
if (query.dimensions) {
182198
for (const dim of query.dimensions) {
183199
const expr = dimExpr(dim);
@@ -230,6 +246,24 @@ export class ObjectQLStrategy implements AnalyticsStrategy {
230246
whereParts.push(`${col} ${op} $${params.length}`);
231247
}
232248
}
249+
250+
// #3654 — render the READ SCOPE (tenant + RLS) that `execute()` injects via
251+
// `withReadScope`, so the previewed WHERE is an honest account of what runs.
252+
// Without this the query's own filters showed but the security predicate
253+
// silently didn't — the preview understated the real constraint set. Base
254+
// object only: cross-object queries are already rejected above.
255+
if (typeof ctx.getReadScope === 'function') {
256+
const scope = ctx.getReadScope(tableName);
257+
if (scope != null) {
258+
const { sql: scopeSql, params: scopeParams } = compileScopedFilterToSql(scope as FilterCondition, tableName);
259+
if (scopeSql) {
260+
let i = 0;
261+
const rendered = scopeSql.replace(/\?/g, () => { params.push(scopeParams[i++]); return `$${params.length}`; });
262+
whereParts.push(`(${rendered})`);
263+
}
264+
}
265+
}
266+
233267
if (whereParts.length > 0) sql += ` WHERE ${whereParts.join(' AND ')}`;
234268

235269
if (groupByParts.length > 0) {

0 commit comments

Comments
 (0)