diff --git a/.changeset/21249-native-sql-base-column-qualify.md b/.changeset/21249-native-sql-base-column-qualify.md new file mode 100644 index 00000000000..874234df533 --- /dev/null +++ b/.changeset/21249-native-sql-base-column-qualify.md @@ -0,0 +1,19 @@ +--- +"@objectstack/service-analytics": patch +--- + +fix(service-analytics): on the native-SQL strategy, a base-table column is qualified with its table whenever the statement joins a related object, not only when the cube declares a join + +Clause-②: no + +A cube that declares no join still joins a lookup's declared `reference` when a query names a relationship path through it (`owner.email`). The native-SQL strategy qualified base-table columns only for a cube that declares a join, so it wrote them bare beside the joined object. When that object declares a column of the same name, the database refused the statement as ambiguous, and `POST /api/v1/analytics/query` answered `500 DATABASE_ERROR` on SQLite and on PostgreSQL. The ObjectQL strategy answered `200` for the same query. + +**Before and after**, measured on a configured cube over a `deal` object that declares no join, whose lookup `owner` points at a person object that also declares `note`, `amount`, `closed_on` and `id`: + +- Dimensions `note` and `owner.email`, with or without a `where` on `note` and an `order` by it: `500` → `200`, one group per (deal note, owner email). +- A `sum` over `amount`, a `timeDimensions` window on `closed_on`, or a `where` on `id`, each grouped by `owner.email`: `500` → `200`. +- An ad-hoc query over the object, whose inferred cube never declares a join: the same. + +The strategy now reads what the statement actually joins, from the one relationship-path resolver, and qualifies every base column in the select list, the grouping, the filters, the measures and the time windows. A statement that joins nothing keeps bare columns. That is now also true on a cube that declares a join when the query uses none of it: the statement it shows on `POST /api/v1/analytics/sql` reads `note` where it read `"deal"."note"`, and the answer is the same. + +**Unchanged.** The ObjectQL strategy; every query on a cube that declares the join it uses; every statement that joins nothing on a cube that declares no join; the refusals. diff --git a/packages/services/service-analytics/src/__tests__/analytics-service.test.ts b/packages/services/service-analytics/src/__tests__/analytics-service.test.ts index ad4aed32088..189a92d2692 100644 --- a/packages/services/service-analytics/src/__tests__/analytics-service.test.ts +++ b/packages/services/service-analytics/src/__tests__/analytics-service.test.ts @@ -247,7 +247,9 @@ describe('NativeSQLStrategy', () => { expect(sql).toContain('LEFT JOIN "account" ON "opportunity"."account" = "account"."id"'); expect(sql).toContain('"account"."industry" AS "account.industry"'); - expect(sql).toContain('SUM(amount)'); + // [#21249] The cube declares no join, but the statement joins `account`, so + // the base column the measure sums is qualified against the base table. + expect(sql).toContain('SUM("opportunity"."amount")'); }); it('should execute query and return structured result', async () => { diff --git a/packages/services/service-analytics/src/__tests__/infer-cube-relation-traversal.test.ts b/packages/services/service-analytics/src/__tests__/infer-cube-relation-traversal.test.ts index 968a6d28563..90e21e6c67d 100644 --- a/packages/services/service-analytics/src/__tests__/infer-cube-relation-traversal.test.ts +++ b/packages/services/service-analytics/src/__tests__/infer-cube-relation-traversal.test.ts @@ -352,10 +352,11 @@ describe('[#5739] the object and array `where` spellings converge on one travers ); expect(dimensions).toEqual(['industry', 'owner.region']); - // The bare column stays BARE: `qualifyAndRegisterJoin` qualifies plain - // identifiers only when the cube declares `joins`, and an inferred cube never - // does — minting a dotted dimension does not change that (#5353's block 2). - expect(sqls[0]).toContain('WHERE (industry = $1 AND "owner"."region" = $2)'); + // [#21249] The base column is qualified: the statement joins `owner`, so + // `industry` beside it is written against the base table, whether or not + // the cube declares `joins` — an inferred cube never does. Left bare, a + // joined target that also declares `industry` makes it ambiguous. + expect(sqls[0]).toContain('WHERE ("crm_account"."industry" = $1 AND "owner"."region" = $2)'); }); }); diff --git a/packages/services/service-analytics/src/__tests__/infer-cube-where-spelling-parity.test.ts b/packages/services/service-analytics/src/__tests__/infer-cube-where-spelling-parity.test.ts index cc09d4c4482..55b3e548831 100644 --- a/packages/services/service-analytics/src/__tests__/infer-cube-where-spelling-parity.test.ts +++ b/packages/services/service-analytics/src/__tests__/infer-cube-where-spelling-parity.test.ts @@ -20,9 +20,10 @@ * * ## Why this had no user-visible symptom (the issue's own observation class) * - * `NativeSQLStrategy.resolveFieldSql` falls back to the bare column name for a - * member the cube does not declare, and `qualifyAndRegisterJoin` leaves bare - * columns bare on a cube with no `joins` — which an ad-hoc cube never has. So + * `NativeSQLStrategy.resolveFieldSql` falls back to the column the member names + * for a member the cube does not declare, and `qualifyAndRegisterJoin` leaves + * that column bare in a statement that joins nothing ([#21249]: what the + * statement joins, not the cube's `joins`, which an ad-hoc cube never has). So * both spellings compiled the same SQL before the fix and still do; block 2 * measures that rather than asserting it. The divergence was confined to the * dimension VOCABULARY, which is why this was filed as an observation and fixed @@ -317,9 +318,9 @@ describe('[#5353] the seeded dimensions change no verdict and no statement', () expect(arraySpelling.sqls).toEqual(objectSpelling.sqls); // A bare column stays bare: `qualifyAndRegisterJoin` only qualifies when the - // cube declares `joins`, and an inferred cube never does. This is the whole - // reason #5353 was an observation rather than a defect — and the assertion - // that keeps a newly-DECLARED dimension from starting to qualify. + // statement joins something ([#21249]), and this one joins nothing. This is + // the whole reason #5353 was an observation rather than a defect — and the + // assertion that keeps a newly-DECLARED dimension from starting to qualify. expect(objectSpelling.sqls[0]).toContain('WHERE stage = '); expect(objectSpelling.sqls[0]).not.toContain('"deal"."stage"'); }); @@ -459,10 +460,11 @@ describe('[#5353/#5739] a dotted `where` key is unified too — as a traversal', it('bare and dotted keys reach parity together when both ride along', async () => { // Before the ruling only `stage` was unified and the whole query was refused // for `region`; now both keys are minted, on both spellings, and the query - // runs. The bare column stays BARE in the statement — `qualifyAndRegisterJoin` - // qualifies plain identifiers only for a cube declaring `joins`, and minting a - // dotted dimension does not give an inferred cube one (block 2's rule, still - // holding with a traversal in the same filter). + // runs. [#21249] The traversal makes the statement join `owner`, so the base + // column is qualified against the base table on both spellings alike — + // `qualifyAndRegisterJoin` reads what the statement joins, not whether the + // cube declares `joins` (block 2's bare column is the statement that joins + // nothing). const both = [['stage', '=', 'won'], ['owner.region', '=', 'NA']]; const array = await inferredDimensions(both, NO_REGION); const object = await inferredDimensions( @@ -473,6 +475,6 @@ describe('[#5353/#5739] a dotted `where` key is unified too — as a traversal', expect(array.dimensions).toEqual(['owner.region', 'stage']); expect(object.dimensions).toEqual(array.dimensions); expect(object.sqls).toEqual(array.sqls); - expect(array.sqls[0]).toContain('WHERE (stage = $1 AND "owner"."region" = $2)'); + expect(array.sqls[0]).toContain('WHERE ("deal"."stage" = $1 AND "owner"."region" = $2)'); }); }); diff --git a/packages/services/service-analytics/src/__tests__/native-sql-base-column-qualify.test.ts b/packages/services/service-analytics/src/__tests__/native-sql-base-column-qualify.test.ts new file mode 100644 index 00000000000..3c53783d7b1 --- /dev/null +++ b/packages/services/service-analytics/src/__tests__/native-sql-base-column-qualify.test.ts @@ -0,0 +1,323 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#21249] On the native-SQL face, a base-table column is qualified with the + * base table exactly when the STATEMENT joins something — read off the joins + * the one hop resolver (`hop-object.ts`) registered for the query, never off + * whether the cube declares `joins`. + * + * ## The shape this closes + * + * Measured through `POST /api/v1/analytics/query` on the real dispatcher route + * at `4727fcb2`, SQLite and PostgreSQL 16.14. A configured cube over `deal` + * declares NO join; `owner` is a lookup whose `reference` is the person + * object, which declares `note`, `amount` and `closed_on` too: + * + * | query | native face | ObjectQL face | + * |:--|:--|:--| + * | dimensions `note` + `owner.email` | 500 (SQLite "ambiguous column name: note"; PostgreSQL 42702) | 200 | + * | the same with `where: { note }` and `order: { note }` | 500 | 200 | + * | `sum(amount)` by `owner.email` | 500 (42702 on `amount`) | 200 | + * | a `closed_on` time-dimension window by `owner.email` | 500 (42702 on `closed_on`) | 200 | + * | `where: { id }` (a member the cube does not declare) by `owner.email` | 500 (42702 on `id`) | 200 | + * | control: the same pair on a cube that declares the join | 200 | 200 | + * + * The statement joined `owner` through the lookup's declared `reference` + * (the resolver's tier 2) while `qualifyAndRegisterJoin` qualified base + * columns only for a cube declaring `joins`: `SELECT note AS "note", … LEFT + * JOIN "" "owner" … GROUP BY note, …`. + * + * ## Why the statement is compiled twice + * + * Joins are registered lazily — by whichever member walks a relationship path + * first — and an absorbed `$or` takes back the joins its branches registered, + * so "does this statement join anything" is known only after every member + * has been resolved. In the card's own pair `note` resolves before + * `owner.email` registers the join. `generateSql` compiles once with bare base + * columns and, when that compile registered a join, once more with every base + * column qualified; the "join registered by a later member" pin below is the + * one a call-time read of the join set fails. + * + * ## The dialect axis of THIS file + * + * The SQLite cell always runs. The PostgreSQL cell runs where + * `OS_TEST_POSTGRES_URL` is set and is a named skip otherwise; no CI step + * provisions that variable for this package, so the live cell is red-capable + * and un-run in CI, and the PR that landed this file carries its local + * PostgreSQL 16 run. The live cell owns its tables, dropped before and after. + */ + +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import { ObjectQL } from '@objectstack/objectql'; +import { SqlDriver } from '@objectstack/driver-sql'; +import type { Cube } from '@objectstack/spec/data'; +import type { AnalyticsService } from '../analytics-service.js'; +import { AnalyticsServicePlugin } from '../plugin.js'; + +const PERSON = 'os21249_person'; +const DEAL = 'os21249_deal'; + +/** The lookup's target declares a column of every name the base columns below use. */ +const PERSON_OBJECT = { + name: PERSON, + label: 'Base-column qualification person', + fields: { + email: { name: 'email', type: 'text' as const }, + note: { name: 'note', type: 'text' as const }, + amount: { name: 'amount', type: 'number' as const }, + closed_on: { name: 'closed_on', type: 'date' as const }, + }, +}; + +const DEAL_OBJECT = { + name: DEAL, + label: 'Base-column qualification deal', + fields: { + note: { name: 'note', type: 'text' as const }, + amount: { name: 'amount', type: 'number' as const }, + closed_on: { name: 'closed_on', type: 'date' as const }, + // Named after nothing the cube joins: only the declared `reference` reaches the person. + owner: { name: 'owner', type: 'lookup' as const, reference: PERSON }, + }, +}; + +// The person rows carry values that would change every answer below if the +// statement read the PERSON's `note` / `amount` / `closed_on` instead. +const PEOPLE = [ + { id: 'p1', email: 'a@x', note: 'pn1', amount: 100, closed_on: '2026-01-01' }, + { id: 'p2', email: 'b@x', note: 'pn2', amount: 200, closed_on: '2026-01-01' }, +] as const; +const DEALS = [ + { id: 'd1', note: 'x', amount: 10, closed_on: '2026-03-01', owner: 'p1' }, + { id: 'd2', note: 'x', amount: 5, closed_on: '2026-03-02', owner: 'p1' }, + { id: 'd3', note: 'y', amount: 7, closed_on: '2026-03-03', owner: 'p2' }, + { id: 'd4', note: 'x', amount: 1, closed_on: '2026-05-01', owner: 'p2' }, +] as const; + +const MEMBERS = { + measures: { + count: { type: 'count', sql: '*', label: 'Rows' }, + amount_sum: { type: 'sum', sql: 'amount', label: 'Amount' }, + }, + dimensions: { + note: { type: 'string', sql: 'note', label: 'Note' }, + closed_on: { type: 'time', sql: 'closed_on', label: 'Closed on' }, + }, +}; + +/** The card's cube: it declares NO join. */ +const NO_JOIN = 'os21249_no_join'; +/** The control: the same members, with the join declared. */ +const DECLARED = 'os21249_declared'; + +const CUBES = [ + { name: NO_JOIN, title: 'No declared join', sql: DEAL, public: true, ...MEMBERS }, + { name: DECLARED, title: 'Declared join', sql: DEAL, public: true, ...MEMBERS, joins: { owner: { name: PERSON } } }, +] as unknown as Cube[]; + +/** The card's pair, grouped by the DEAL's `note`: (note, owner email, rows). */ +const PAIR_GROUPS = [['x', 'a@x', 2], ['x', 'b@x', 1], ['y', 'b@x', 1]]; + +interface Cell { + id: 'sqlite' | 'pg'; + label: string; + env: string | null; + config: () => Record | null; +} + +const CELLS: readonly Cell[] = [ + { id: 'sqlite', label: 'sqlite', env: null, config: () => ({ client: 'better-sqlite3', connection: { filename: ':memory:' }, useNullAsDefault: true }) }, + { + id: 'pg', + label: 'live postgres', + env: 'OS_TEST_POSTGRES_URL', + config: () => (process.env.OS_TEST_POSTGRES_URL ? { client: 'pg', connection: process.env.OS_TEST_POSTGRES_URL } : null), + }, +]; + +const FACES = ['native', 'objectql'] as const; +type Face = (typeof FACES)[number]; + +const quiet = { debug() {}, info() {}, warn() {}, error() {}, child() { return quiet; } }; + +type Row = Record; + +/** Tuples in one order, so two faces' row orders compare equal. */ +const sorted = (tuples: unknown[][]) => [...tuples].sort((a, b) => JSON.stringify(a).localeCompare(JSON.stringify(b))); + +/** Rows as sorted tuples of the named columns; a measure reads as a number on every dialect. */ +const tuples = (rows: unknown, columns: readonly string[]) => + sorted((rows as Row[]).map((row) => columns.map((c) => (typeof row[c] === 'number' || /^-?\d+(\.\d+)?$/.test(String(row[c])) ? Number(row[c]) : row[c])))); + +for (const cell of CELLS) { + const config = cell.config(); + describe.skipIf(!config)( + `[#21249] a base column beside a relationship path the cube declares no join for (${cell.label})${config ? '' : ` (skipped: set ${cell.env} to run this cell)`}`, + () => { + let driver: any; + let engine: ObjectQL; + /** Raw-SQL statements and engine aggregates, on any object. */ + const reads = { rawSql: 0, aggregate: 0 }; + /** `native`: the plugin's own capabilities. `objectql`: narrowed to the engine-aggregate path. */ + const services: Partial> = {}; + + const dropTables = async () => { + if (cell.id !== 'pg') return; + for (const table of [DEAL, PERSON]) await driver?.execute(`drop table if exists ${table}`).catch(() => {}); + }; + + /** One query on one face: its rows, or its error, and the reads it caused. */ + const read = async (face: Face, query: Record) => { + const before = { ...reads }; + const outcome = await services[face]!.query(query as any).then( + (res) => ({ rows: res.rows as unknown, err: undefined as Error | undefined }), + (err) => ({ rows: undefined as unknown, err: err as Error }), + ); + return { ...outcome, rawSql: reads.rawSql - before.rawSql, aggregate: reads.aggregate - before.aggregate }; + }; + + /** The statement the native face runs for a query — the dry-run door. */ + const statement = async (query: Record) => (await services.native!.generateSql(query as any)).sql; + + /** Both faces serve `query` with the same `expected` groups; the native face in one raw statement. */ + const servedOnBothFaces = async (query: Record, columns: readonly string[], expected: unknown[][]) => { + for (const face of FACES) { + const { rows, err, rawSql, aggregate } = await read(face, query); + expect(err, `${face}: ${err?.message}`).toBeUndefined(); + expect(tuples(rows, columns), face).toEqual(sorted(expected)); + if (face === 'native') { + expect(rawSql, 'the native face answered: one raw statement').toBe(1); + expect(aggregate, 'the native face asked no engine aggregate').toBe(0); + } else { + expect(rawSql, 'the ObjectQL face ran no raw statement').toBe(0); + } + } + }; + + beforeAll(async () => { + driver = new SqlDriver(config as any); + await dropTables(); + engine = new ObjectQL({ logger: quiet } as any); + engine.registerDriver(driver, true); + await engine.init(); + for (const object of [PERSON_OBJECT, DEAL_OBJECT]) engine.registry.registerObject(object as any); + await engine.syncSchemas(); + for (const row of PEOPLE) await engine.insert(PERSON, { ...row } as any); + for (const row of DEALS) await engine.insert(DEAL, { ...row } as any); + + const realExecute = (engine as any).execute.bind(engine); + (engine as any).execute = (sql: unknown, opts?: unknown) => { + reads.rawSql += 1; + return realExecute(sql, opts); + }; + const realAggregate = engine.aggregate.bind(engine); + (engine as any).aggregate = (...args: unknown[]) => { + reads.aggregate += 1; + return (realAggregate as any)(...args); + }; + + // The plugin's own composition over the real engine — the relationship + // resolver wired from the engine's registry, so `owner` reaches the person. + for (const [face, caps] of [ + ['native', undefined], + ['objectql', () => ({ nativeSql: false, objectqlAggregate: true, inMemory: false })], + ] as const) { + const registered: Record = {}; + await new AnalyticsServicePlugin({ cubes: CUBES, ...(caps ? { queryCapabilities: caps } : {}) } as any).init({ + getService: (name: string) => (name === 'data' ? engine : registered[name]), + registerService: (name: string, svc: unknown) => { registered[name] = svc; }, + replaceService: (name: string, svc: unknown) => { registered[name] = svc; }, + hook: () => {}, + logger: quiet, + } as never); + services[face] = registered.analytics as AnalyticsService; + } + }); + + afterAll(async () => { + await dropTables(); + try { await engine?.destroy(); } catch { /* noop */ } + }); + + it('the card pair is served on the native face, grouped by the BASE column, as the ObjectQL face serves it', async () => { + const query = { cube: NO_JOIN, measures: ['count'], dimensions: ['note', 'owner.email'] }; + await servedOnBothFaces(query, ['note', 'owner.email', 'count'], PAIR_GROUPS); + const sql = await statement(query); + expect(sql).toContain(`SELECT "${DEAL}"."note" AS "note", "owner"."email" AS "owner.email"`); + expect(sql).toContain(`LEFT JOIN "${PERSON}" "owner" ON "${DEAL}"."owner" = "owner"."id"`); + expect(sql).toContain(`GROUP BY "${DEAL}"."note", "owner"."email"`); + }); + + it('the pair with a `where` on the base column and an `order` by it', async () => { + const query = { cube: NO_JOIN, measures: ['count'], dimensions: ['note', 'owner.email'], where: { note: 'x' }, order: { note: 'asc' } }; + await servedOnBothFaces(query, ['note', 'owner.email', 'count'], [['x', 'a@x', 2], ['x', 'b@x', 1]]); + const sql = await statement(query); + expect(sql).toContain(`WHERE "${DEAL}"."note" = $1`); + expect(sql).toContain(`GROUP BY "${DEAL}"."note", "owner"."email"`); + // ORDER BY names the output column the SELECT list aliases, never a table column. + expect(sql).toContain('ORDER BY "note" ASC'); + }); + + it('every other base-column emitter beside the join: a measure, a time-dimension window, a member the cube does not declare', async () => { + await servedOnBothFaces( + { cube: NO_JOIN, measures: ['amount_sum'], dimensions: ['owner.email'] }, + ['owner.email', 'amount_sum'], + [['a@x', 15], ['b@x', 8]], + ); + expect(await statement({ cube: NO_JOIN, measures: ['amount_sum'], dimensions: ['owner.email'] })).toContain(`"${DEAL}"."amount"`); + + const window = { cube: NO_JOIN, measures: ['count'], dimensions: ['owner.email'], timeDimensions: [{ dimension: 'closed_on', dateRange: ['2026-03-01', '2026-03-31'] }] }; + await servedOnBothFaces(window, ['owner.email', 'count'], [['a@x', 2], ['b@x', 1]]); + expect(await statement(window)).toContain(`"${DEAL}"."closed_on" >= $1`); + + const undeclared = { cube: NO_JOIN, measures: ['count'], dimensions: ['owner.email'], where: { id: 'd1' } }; + await servedOnBothFaces(undeclared, ['owner.email', 'count'], [['a@x', 1]]); + expect(await statement(undeclared)).toContain(`WHERE "${DEAL}"."id" = $1`); + }); + + it('the join is known only once every member is resolved: a base column compiled BEFORE a later member registers the join is qualified too', async () => { + // The SELECT list is compiled before the `where`, so `note` is resolved + // while nothing is joined yet; the filter then joins `owner`. + // Native face only: the ObjectQL face refuses a cross-object filter of its own accord. + const byFilter = { cube: NO_JOIN, measures: ['count'], dimensions: ['note'], where: { 'owner.email': 'a@x' } }; + const { rows, err } = await read('native', byFilter); + expect(err, err?.message).toBeUndefined(); + expect(tuples(rows, ['note', 'count'])).toEqual([['x', 2]]); + const sql = await statement(byFilter); + expect(sql).toContain(`SELECT "${DEAL}"."note" AS "note"`); + expect(sql).toContain(`GROUP BY "${DEAL}"."note"`); + + // Either order of the card's pair compiles the same qualified columns. + const reversed = await statement({ cube: NO_JOIN, measures: ['count'], dimensions: ['owner.email', 'note'] }); + expect(reversed).toContain(`GROUP BY "owner"."email", "${DEAL}"."note"`); + }); + + it('CONTROL the declared-join cube compiles the pair to the statement it always did, and serves the same groups', async () => { + const query = { cube: DECLARED, measures: ['count'], dimensions: ['note', 'owner.email'] }; + await servedOnBothFaces(query, ['note', 'owner.email', 'count'], PAIR_GROUPS); + expect(await statement(query)).toBe( + `SELECT "${DEAL}"."note" AS "note", "owner"."email" AS "owner.email", COUNT(*) AS "count" FROM "${DEAL}" ` + + `LEFT JOIN "${PERSON}" "owner" ON "${DEAL}"."owner" = "owner"."id" GROUP BY "${DEAL}"."note", "owner"."email"`, + ); + // The cube without the declaration now compiles the very same statement. + expect(await statement({ ...query, cube: NO_JOIN })).toBe(await statement(query)); + }); + + it('CONTROL a statement that joins nothing keeps its bare columns — on either cube, and when an absorbed `$or` takes its join back', async () => { + const bare = `SELECT note AS "note", COUNT(*) AS "count" FROM "${DEAL}" GROUP BY note`; + for (const cube of [NO_JOIN, DECLARED]) { + const query = { cube, measures: ['count'], dimensions: ['note'] }; + await servedOnBothFaces(query, ['note', 'count'], [['x', 3], ['y', 1]]); + // On the declared-join cube this statement used to read `""."note"`: + // both name one column of the one table the statement reads. + expect(await statement(query), cube).toBe(bare); + } + // `{}` is TRUE and absorbs the disjunction, taking back the join its + // other branch registered: the statement reads one table again. + const absorbed = { cube: NO_JOIN, measures: ['count'], dimensions: ['note'], where: { $or: [{ 'owner.email': 'a@x' }, {}] } }; + await servedOnBothFaces(absorbed, ['note', 'count'], [['x', 3], ['y', 1]]); + expect(await statement(absorbed)).toBe(bare); + }); + }, + ); +} 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 d7c20d35c3b..d2c652842c9 100644 --- a/packages/services/service-analytics/src/strategies/native-sql-strategy.ts +++ b/packages/services/service-analytics/src/strategies/native-sql-strategy.ts @@ -170,13 +170,34 @@ function measureColumnOf( * `relationshipReference`) so every path the statement walks — a dimension, a * measure, a filter member, a time dimension — is resolved with the answer the * door admitted the query with. + * + * [#21249] …and whether the statement qualifies its base-table columns: + * `true` exactly when this statement joins something, which + * {@link NativeSQLStrategy.generateSql} reads off the joins a first compile of + * the same query registered. See {@link NativeSQLStrategy.qualifyAndRegisterJoin}. */ class StatementJoins extends Map { - constructor(readonly referenceOf: HopReference | undefined) { + constructor( + readonly referenceOf: HopReference | undefined, + readonly qualifyBaseColumns: boolean, + ) { super(); } } +/** + * [#21249] The clauses one compile of a query produces, before the join + * allowlist and the read scopes are applied and the statement is assembled. + */ +interface StatementClauses { + readonly tableName: string; + readonly params: unknown[]; + readonly selectClauses: string[]; + readonly groupByClauses: string[]; + readonly whereClauses: string[]; + readonly joins: StatementJoins; +} + /** * NativeSQLStrategy — Priority 1 * @@ -773,6 +794,38 @@ export class NativeSQLStrategy implements AnalyticsStrategy { // See {@link assertNoCrossFieldComparison} for why it is asserted anyway. this.assertNoCrossFieldComparison(query, ctx); + // [#21249] A base-table column is qualified exactly when THIS statement + // joins something, and what it joins is known only once every member has + // been resolved: joins are registered lazily, by whichever member walks a + // relationship path first, and an absorbed `$or` takes back the joins its + // branches registered. So the query is compiled once with bare base + // columns, and, when that compile registered any join, once more with + // every base column qualified. The predicate is the hop resolver's own + // answer for this query, never `cube.joins`: a cube that declares no join + // still joins a lookup's declared `reference` (`hop-object.ts`, tier 2), + // and reading the declaration left `note` bare beside a joined target that + // also declares `note` — "ambiguous column" on SQLite and PostgreSQL. Both + // compiles walk the same members through the same resolver, so the second + // registers the same joins; a statement that joins nothing is compiled + // once and keeps its bare columns. + let clauses = this.compileClauses(query, ctx, cube, false); + if (clauses.joins.size > 0) clauses = this.compileClauses(query, ctx, cube, true); + return this.assembleStatement(query, ctx, cube, clauses); + } + + /** + * [#21249] Compile a query's SELECT, GROUP BY and WHERE clauses — the + * dimensions, the measures with their scoped filters, the `where`, the + * dataset's own scope and the time-dimension windows — registering the joins + * its relationship paths walk. `qualifyBaseColumns` is whether its base-table + * columns are written `"".""`; see {@link generateSql}. + */ + private compileClauses( + query: AnalyticsQuery, + ctx: StrategyContext, + cube: Cube, + qualifyBaseColumns: boolean, + ): StatementClauses { const params: unknown[] = []; const selectClauses: string[] = []; const groupByClauses: string[] = []; @@ -781,7 +834,7 @@ export class NativeSQLStrategy implements AnalyticsStrategy { // lazily as dotted dimensions/measures/filters are resolved. [#20986] Each // hop's object comes from the one resolver, with the host's answer for a // relationship field's declared target — the door's own. - const joins = new StatementJoins(relationshipReferenceOf(ctx)); + const joins = new StatementJoins(relationshipReferenceOf(ctx), qualifyBaseColumns); // Build SELECT for dimensions if (query.dimensions && query.dimensions.length > 0) { @@ -935,6 +988,21 @@ export class NativeSQLStrategy implements AnalyticsStrategy { } } + return { tableName, params, selectClauses, groupByClauses, whereClauses, joins }; + } + + /** + * [#21249] Enforce the join allowlist, inject the read scopes and assemble + * the statement from the clauses {@link compileClauses} produced. + */ + private assembleStatement( + query: AnalyticsQuery, + ctx: StrategyContext, + cube: Cube, + clauses: StatementClauses, + ): { sql: string; params: unknown[] } { + const { tableName, params, selectClauses, groupByClauses, whereClauses, joins } = clauses; + // ── ADR-0021 D-C — enforce the join allowlist + inject per-object RLS ── // 1. Reject any join not backed by a relationship the dataset declared. const allowed = ctx.getAllowedRelationships?.(query.cube!); @@ -1087,7 +1155,8 @@ export class NativeSQLStrategy implements AnalyticsStrategy { * target under the field's alias, `LEFT JOIN "crm_person" "owner" ON …`. * * Returns the qualified SQL reference (e.g. `"account"."industry"`). - * Pure column references (no dot) are returned as-is. + * A base-table column (no dot) is returned qualified with the base table + * when the statement joins something, and as-is otherwise. */ private qualifyAndRegisterJoin( rawSql: string, @@ -1096,14 +1165,20 @@ export class NativeSQLStrategy implements AnalyticsStrategy { cube?: Cube, ): string { if (!rawSql.includes('.')) { - // Base-table column. When the cube can join other tables, a bare column - // that also exists on a joined table (e.g. base `status` vs joined + // Base-table column. When the statement joins another table, a bare + // column that also exists on a joined table (e.g. base `status` vs joined // `account.status`) makes the SQL engine raise "ambiguous column name". // Qualify plain identifiers with the base table; leave SQL expressions - // and `*` untouched. Single-object cubes (no joins) keep bare columns so - // their generated SQL is byte-for-byte unchanged. - const canJoin = !!cube?.joins && Object.keys(cube.joins).length > 0; - if (canJoin && /^[A-Za-z_][A-Za-z0-9_]*$/.test(rawSql)) { + // and `*` untouched. A statement that joins nothing keeps bare columns, + // so its generated SQL is byte-for-byte unchanged. + // + // [#21249] "Joins something" is THIS statement's joins, as the hop + // resolver registered them — `joins.qualifyBaseColumns`, set by + // `generateSql` — never whether the cube declares a join. The declaration + // answers for the cube, not the statement: a cube declaring no join still + // joins a lookup's declared `reference`, and its base columns were left + // bare beside it. + if (joins.qualifyBaseColumns && /^[A-Za-z_][A-Za-z0-9_]*$/.test(rawSql)) { return `"${parentTable}"."${rawSql}"`; } return rawSql; @@ -1327,7 +1402,10 @@ export class NativeSQLStrategy implements AnalyticsStrategy { const measure = this.lookupMember(cube, member, 'measure'); if (measure) return this.qualifyAndRegisterJoin(measure.sql, parentTable, joins, cube); const fieldName = member.includes('.') ? member.split('.')[1] : member; - return fieldName; + // [#21249] A member the cube does not declare is a base-table column too + // (`where: { id }`), so it takes the same qualification as a declared one: + // returned bare it sat beside a joined target's own `id`, ambiguous. + return this.qualifyAndRegisterJoin(fieldName, parentTable, joins, cube); } /**