Skip to content

Commit 51368f7

Browse files
committed
fix(service-analytics): qualify native-SQL base columns when the statement joins anything, read from the hop resolver's joins
The native strategy qualified a bare base-table column only when the cube declared a join. A cube that declares none still joins a lookup's declared reference through the one hop resolver, so a base column sharing its name with a column of the joined target was emitted bare: "ambiguous column" on SQLite and PostgreSQL (500). The statement is now compiled once with bare base columns and, when that compile registered any join, once more with every base column qualified. Claude-Session: https://claude.ai/code/session_01DiCSbmJrkzNhuEAier4VoJ Co-authored-by: Claude <noreply@anthropic.com>
1 parent 4727fcb commit 51368f7

1 file changed

Lines changed: 84 additions & 9 deletions

File tree

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

Lines changed: 84 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -170,13 +170,34 @@ function measureColumnOf(
170170
* `relationshipReference`) so every path the statement walks — a dimension, a
171171
* measure, a filter member, a time dimension — is resolved with the answer the
172172
* door admitted the query with.
173+
*
174+
* [#21249] …and whether the statement qualifies its base-table columns:
175+
* `true` exactly when this statement joins something, which
176+
* {@link NativeSQLStrategy.generateSql} reads off the joins a first compile of
177+
* the same query registered. See {@link NativeSQLStrategy.qualifyAndRegisterJoin}.
173178
*/
174179
class StatementJoins extends Map<string, { readonly sql: string; readonly object: string }> {
175-
constructor(readonly referenceOf: HopReference | undefined) {
180+
constructor(
181+
readonly referenceOf: HopReference | undefined,
182+
readonly qualifyBaseColumns: boolean,
183+
) {
176184
super();
177185
}
178186
}
179187

188+
/**
189+
* [#21249] The clauses one compile of a query produces, before the join
190+
* allowlist and the read scopes are applied and the statement is assembled.
191+
*/
192+
interface StatementClauses {
193+
readonly tableName: string;
194+
readonly params: unknown[];
195+
readonly selectClauses: string[];
196+
readonly groupByClauses: string[];
197+
readonly whereClauses: string[];
198+
readonly joins: StatementJoins;
199+
}
200+
180201
/**
181202
* NativeSQLStrategy — Priority 1
182203
*
@@ -773,6 +794,38 @@ export class NativeSQLStrategy implements AnalyticsStrategy {
773794
// See {@link assertNoCrossFieldComparison} for why it is asserted anyway.
774795
this.assertNoCrossFieldComparison(query, ctx);
775796

797+
// [#21249] A base-table column is qualified exactly when THIS statement
798+
// joins something, and what it joins is known only once every member has
799+
// been resolved: joins are registered lazily, by whichever member walks a
800+
// relationship path first, and an absorbed `$or` takes back the joins its
801+
// branches registered. So the query is compiled once with bare base
802+
// columns, and, when that compile registered any join, once more with
803+
// every base column qualified. The predicate is the hop resolver's own
804+
// answer for this query, never `cube.joins`: a cube that declares no join
805+
// still joins a lookup's declared `reference` (`hop-object.ts`, tier 2),
806+
// and reading the declaration left `note` bare beside a joined target that
807+
// also declares `note` — "ambiguous column" on SQLite and PostgreSQL. Both
808+
// compiles walk the same members through the same resolver, so the second
809+
// registers the same joins; a statement that joins nothing is compiled
810+
// once and keeps its bare columns.
811+
let clauses = this.compileClauses(query, ctx, cube, false);
812+
if (clauses.joins.size > 0) clauses = this.compileClauses(query, ctx, cube, true);
813+
return this.assembleStatement(query, ctx, cube, clauses);
814+
}
815+
816+
/**
817+
* [#21249] Compile a query's SELECT, GROUP BY and WHERE clauses — the
818+
* dimensions, the measures with their scoped filters, the `where`, the
819+
* dataset's own scope and the time-dimension windows — registering the joins
820+
* its relationship paths walk. `qualifyBaseColumns` is whether its base-table
821+
* columns are written `"<table>"."<column>"`; see {@link generateSql}.
822+
*/
823+
private compileClauses(
824+
query: AnalyticsQuery,
825+
ctx: StrategyContext,
826+
cube: Cube,
827+
qualifyBaseColumns: boolean,
828+
): StatementClauses {
776829
const params: unknown[] = [];
777830
const selectClauses: string[] = [];
778831
const groupByClauses: string[] = [];
@@ -781,7 +834,7 @@ export class NativeSQLStrategy implements AnalyticsStrategy {
781834
// lazily as dotted dimensions/measures/filters are resolved. [#20986] Each
782835
// hop's object comes from the one resolver, with the host's answer for a
783836
// relationship field's declared target — the door's own.
784-
const joins = new StatementJoins(relationshipReferenceOf(ctx));
837+
const joins = new StatementJoins(relationshipReferenceOf(ctx), qualifyBaseColumns);
785838

786839
// Build SELECT for dimensions
787840
if (query.dimensions && query.dimensions.length > 0) {
@@ -935,6 +988,21 @@ export class NativeSQLStrategy implements AnalyticsStrategy {
935988
}
936989
}
937990

991+
return { tableName, params, selectClauses, groupByClauses, whereClauses, joins };
992+
}
993+
994+
/**
995+
* [#21249] Enforce the join allowlist, inject the read scopes and assemble
996+
* the statement from the clauses {@link compileClauses} produced.
997+
*/
998+
private assembleStatement(
999+
query: AnalyticsQuery,
1000+
ctx: StrategyContext,
1001+
cube: Cube,
1002+
clauses: StatementClauses,
1003+
): { sql: string; params: unknown[] } {
1004+
const { tableName, params, selectClauses, groupByClauses, whereClauses, joins } = clauses;
1005+
9381006
// ── ADR-0021 D-C — enforce the join allowlist + inject per-object RLS ──
9391007
// 1. Reject any join not backed by a relationship the dataset declared.
9401008
const allowed = ctx.getAllowedRelationships?.(query.cube!);
@@ -1087,7 +1155,8 @@ export class NativeSQLStrategy implements AnalyticsStrategy {
10871155
* target under the field's alias, `LEFT JOIN "crm_person" "owner" ON …`.
10881156
*
10891157
* Returns the qualified SQL reference (e.g. `"account"."industry"`).
1090-
* Pure column references (no dot) are returned as-is.
1158+
* A base-table column (no dot) is returned qualified with the base table
1159+
* when the statement joins something, and as-is otherwise.
10911160
*/
10921161
private qualifyAndRegisterJoin(
10931162
rawSql: string,
@@ -1096,14 +1165,20 @@ export class NativeSQLStrategy implements AnalyticsStrategy {
10961165
cube?: Cube,
10971166
): string {
10981167
if (!rawSql.includes('.')) {
1099-
// Base-table column. When the cube can join other tables, a bare column
1100-
// that also exists on a joined table (e.g. base `status` vs joined
1168+
// Base-table column. When the statement joins another table, a bare
1169+
// column that also exists on a joined table (e.g. base `status` vs joined
11011170
// `account.status`) makes the SQL engine raise "ambiguous column name".
11021171
// Qualify plain identifiers with the base table; leave SQL expressions
1103-
// and `*` untouched. Single-object cubes (no joins) keep bare columns so
1104-
// their generated SQL is byte-for-byte unchanged.
1105-
const canJoin = !!cube?.joins && Object.keys(cube.joins).length > 0;
1106-
if (canJoin && /^[A-Za-z_][A-Za-z0-9_]*$/.test(rawSql)) {
1172+
// and `*` untouched. A statement that joins nothing keeps bare columns,
1173+
// so its generated SQL is byte-for-byte unchanged.
1174+
//
1175+
// [#21249] "Joins something" is THIS statement's joins, as the hop
1176+
// resolver registered them — `joins.qualifyBaseColumns`, set by
1177+
// `generateSql` — never whether the cube declares a join. The declaration
1178+
// answers for the cube, not the statement: a cube declaring no join still
1179+
// joins a lookup's declared `reference`, and its base columns were left
1180+
// bare beside it.
1181+
if (joins.qualifyBaseColumns && /^[A-Za-z_][A-Za-z0-9_]*$/.test(rawSql)) {
11071182
return `"${parentTable}"."${rawSql}"`;
11081183
}
11091184
return rawSql;

0 commit comments

Comments
 (0)