From c6d57349da915f05c21b807dad72324795735d0e Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 6 Aug 2026 04:13:02 +0000 Subject: [PATCH] fix(service-analytics): lower the `where` before seeding an ad-hoc cube's dimensions (#5353) `inferCubeFromQuery` guarded its `where` arm with `!Array.isArray(query.where)`, written when an array `where` was not a filter. #5334 made it one, so one filter minted two different cubes depending on its spelling: where: {stage: 'won'} -> dimensions: {stage} where: [['stage','=','won']] -> dimensions: {} The `where` is now lowered to its canonical FilterCondition first, so the spelling stops mattering. The lowering is #5334's own, extracted from `normalizeAnalyticsFilterTree` as `lowerAnalyticsWhere` so exactly one of it survives; keys are read via `conjunctFieldKeys`, which descends `$and` because the lowering introduces `$and` where the object spelling has none. `$or` / `$not` contribute no key on either spelling, as before. No compiled statement, bound value or gate verdict changes: both spellings already compiled a byte-identical predicate, and an inferred cube declares no `joins`, so `qualifyAndRegisterJoin` leaves the newly-declared members' columns bare. The rejection suggestion lists and `getMeta` now read alike for both. A DOTTED `where` key stays spelling-dependent, deliberately: unifying it means either propagating #5739's base-column mis-cast to the array spelling (measured: a working traversal becomes a different-rows base-column filter, or a 400) or splitting a verdict #5740 shares with the `dimensions` request key. Left to #5739 and pinned by tests. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01BWS4heBoAitLmzCLhcYdbK --- .changeset/infer-cube-array-where-parity.md | 51 +++ .../infer-cube-where-spelling-parity.test.ts | 409 ++++++++++++++++++ .../src/analytics-service.ts | 136 ++++-- .../src/strategies/filter-normalizer.ts | 128 ++++-- 4 files changed, 667 insertions(+), 57 deletions(-) create mode 100644 .changeset/infer-cube-array-where-parity.md create mode 100644 packages/services/service-analytics/src/__tests__/infer-cube-where-spelling-parity.test.ts diff --git a/.changeset/infer-cube-array-where-parity.md b/.changeset/infer-cube-array-where-parity.md new file mode 100644 index 0000000000..24f8bca7e0 --- /dev/null +++ b/.changeset/infer-cube-array-where-parity.md @@ -0,0 +1,51 @@ +--- +"@objectstack/service-analytics": patch +--- + +fix(service-analytics): an ad-hoc cube's dimensions no longer depend on how the `where` was spelled (#5353) + +`inferCubeFromQuery` mints a Cube for a free-form analytics query that names no +registered cube, seeding `dimensions` from the fields the query mentions — its +`measures`, `dimensions`, `timeDimensions`, and its `where`. The `where` arm was +guarded by `!Array.isArray(query.where)`, written when an array `where` was not a +filter. #5334 made it one, so from then on one filter minted two different cubes +depending on its spelling: + +``` +where: {stage: 'won'} → dimensions: {stage} ← seeded +where: [['stage','=','won']] → dimensions: {} ← skipped +``` + +The `where` is now LOWERED to its canonical `FilterCondition` before its keys are +read, so the spelling stops mattering. The lowering is the same one the +strategies already use (#5334's `parseFilterAST` call, extracted from +`normalizeAnalyticsFilterTree` as `lowerAnalyticsWhere` so there is still exactly +one of it), and the keys are read through `conjunctFieldKeys`, which descends +`$and` — necessarily, because the lowering itself introduces `$and` where the +object spelling has none: `[[a,…],[b,…]]` lowers to `{$and: [{a…},{b…}]}`. As a +result an explicit `{$and: […]}` object `where` now also seeds its conjuncts' +keys, which it never did. + +`$or` / `$not` are not descended, and contribute no key on either spelling, as +before. + +**No compiled statement, bound value or gate verdict changes.** Both spellings +already compiled a byte-identical predicate (which is why this shipped as an +observation rather than a defect): `resolveFieldSql` falls back to the bare +column name for an undeclared member, and `qualifyAndRegisterJoin` leaves bare +columns bare on a cube with no `joins` — which an inferred cube never has. So the +newly-declared dimensions move those members from the undeclared branch to the +declared one and both yield the same column. What does change is the suggestion +list in a rejection: `Valid filter members:` / `Valid dimensions:` now read the +same for both spellings of one filter, and `getMeta` reports the same dimension +vocabulary for both. + +**Still spelling-dependent: a DOTTED `where` key.** `{'owner.region': 'NA'}` +seeds the stripped tail `region` as a base-table dimension; the array spelling +`[['owner.region','=','NA']]` seeds nothing and compiles the relation traversal. +Unifying them is #5739's call, not this change's — propagating the mint to the +array spelling turns a working traversal into a base-column filter over different +rows (and a `400 INVALID_FIELD` where the base table has no such column), while +withdrawing it from the object spelling would split a verdict #5740 deliberately +shares with the `dimensions` request key. Dotted keys therefore keep today's +per-spelling answer, pinned by tests, until #5739 rules. 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 new file mode 100644 index 0000000000..ee1ab977f6 --- /dev/null +++ b/packages/services/service-analytics/src/__tests__/infer-cube-where-spelling-parity.test.ts @@ -0,0 +1,409 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #5353 — one filter, two spellings, ONE ad-hoc cube. + * + * `inferCubeFromQuery` mints a Cube for a free-form query naming no registered + * cube, seeding `dimensions` from the fields the query mentions — `measures`, + * `dimensions`, `timeDimensions`, and the `where`. Its `where` arm was guarded by + * `!Array.isArray(query.where)`, written when an array `where` was not a filter. + * #5334 made it one, so from then on: + * + * ``` + * where: {stage: 'won'} → dimensions: {stage} ← seeded + * where: [['stage','=','won']] → dimensions: {} ← skipped + * ``` + * + * Same filter, byte-identical compiled predicate since #5334, two different + * cubes. This file pins the parity, and — the half that keeps the fix from + * over-reaching — the three things it must NOT change. + * + * ## 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 + * 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 + * in the window before the ad-hoc path grows a join. + * + * ## Reverse verification, direction predicted BEFORE running + * + * Restoring the `!Array.isArray(query.where)` guard turns the parity table RED + * for every case whose filter lowers to a CONJUNCTION naming at least one field + * — 11 of the 13 — because the array spelling seeds nothing while the object + * spelling seeds its keys. Ordinary direction, no inversion: the assertion is on + * a bag that GAINS entries, and both the parity comparison and the exact + * expected set are asserted, so a case cannot pass by both sides being empty. + * + * The two exceptions are named here rather than left for the next reader: + * `$or`-rooted filters (`prefix OR group`, `nested group — OR of an AND`) stay + * GREEN in both directions. Neither spelling contributes a key through a + * disjunction, before or after — there was no asymmetry there to fix, and + * `conjunctFieldKeys` deliberately does not descend `$or`. Those two cases pin a + * deliberate NON-change; reading the table as "13 red" would be wrong. + * + * Block 3 is GREEN IN BOTH DIRECTIONS, and that is the point rather than a gap. + * It pins the DOTTED residue — the one shape #5353 left answering per spelling — + * so restoring the guard changes nothing there. Reading block 3 as part of the + * fix would be wrong; it is the fence around what the fix could not decide, and + * it is what makes a future `collectFilterLeaves` refactor (which would flatten + * `{owner: {region: 'NA'}}` to the leaf `owner.region` and mint `region`) fail + * loudly instead of quietly changing a verdict #5740 shares with the `dimensions` + * request key. + */ + +import { describe, it, expect, vi } from 'vitest'; +import type { FilterCondition } from '@objectstack/spec/data'; +import { AnalyticsService } from '../analytics-service.js'; + +const silentLogger = { + info: vi.fn(), + debug: vi.fn(), + warn: vi.fn(), + error: vi.fn(), + child: vi.fn().mockReturnThis(), +} as any; + +/** The columns `deal` really has — the source-field gates (#4437/#5520/#5669) read these. */ +const DEAL_FIELDS = ['id', 'stage', 'owner', 'amount', 'closed_at']; + +/** + * A service with NO registered cube for `deal`, so every query takes the + * auto-inference path. One service per query: `ensureCube` registers what it + * infers, so a second query would find the cube and never infer again. + */ +function makeService(opts: { native?: boolean; fields?: string[] } = {}) { + const sqls: string[] = []; + const filters: unknown[] = []; + const service = new AnalyticsService({ + logger: silentLogger, + queryCapabilities: () => ({ + nativeSql: !!opts.native, + objectqlAggregate: !opts.native, + inMemory: false, + }), + executeAggregate: async (_object: string, options: unknown) => { + filters.push((options as { filter?: unknown } | undefined)?.filter); + return [{ count: 1 }]; + }, + executeRawSql: async (_object: string, sql: string) => { + sqls.push(sql); + return [{ count: 1 }]; + }, + isRegisteredObject: (n: string) => n === 'deal', + getObjectFieldNames: (n: string) => (n === 'deal' ? (opts.fields ?? DEAL_FIELDS) : undefined), + }); + return { service, sqls, filters }; +} + +/** The ad-hoc cube's dimension keys, read through the public discovery API. */ +async function inferredDimensions(where: unknown, opts?: { native?: boolean; fields?: string[] }) { + const { service, sqls, filters } = makeService(opts); + await service.query({ cube: 'deal', measures: ['count'], where } as never); + const [meta] = await service.getMeta('deal'); + return { + // `getMeta` prefixes with the cube name; the KEY is what seeding produced. + dimensions: meta.dimensions.map((d) => d.name.replace(/^deal\./, '')).sort(), + sqls, + filters, + }; +} + +/** The error a call rejected with — or a loud failure if it RESOLVED. */ +async function rejection( + call: Promise, +): Promise { + try { + await call; + } catch (e) { + return e as T; + } + throw new Error('expected the query to be refused, but it resolved'); +} + +/** + * The equivalence table, taken from #5334's own `EQUIVALENT_SPELLINGS` so the + * two files cannot disagree about what "the same filter, two spellings" means. + * #5334 asserts the two select the same ROWS; this asserts they mint the same + * CUBE. + * + * `seeded` is the exact dimension vocabulary the filter must contribute — an + * explicit value, not just "both sides equal", because a table that only + * compared the spellings would pass just as happily with both empty, which is + * the defect itself. + */ +const EQUIVALENT_SPELLINGS: Array<{ + name: string; + object: FilterCondition; + array: unknown[]; + seeded: string[]; +}> = [ + { + name: "equality — the issue's own filter, in its lowerable spelling", + object: { stage: 'won' }, + array: [['stage', '=', 'won']], + seeded: ['stage'], + }, + { + name: 'a bare comparison node, not wrapped in a list', + object: { stage: 'won' }, + array: ['stage', '=', 'won'], + seeded: ['stage'], + }, + { + name: 'inequality', + object: { stage: { $ne: 'won' } }, + array: ['stage', '!=', 'won'], + seeded: ['stage'], + }, + { + name: 'ordered comparison', + object: { amount: { $gt: 15 } }, + array: ['amount', '>', 15], + seeded: ['amount'], + }, + { + // The lowering's own `$and`: `conjunctFieldKeys` descends it, so an explicit + // AND group seeds what its conjuncts name. Before #5353 BOTH spellings + // seeded nothing here — parity held at the wrong value, which is why the + // expected set is asserted and not merely the equality. + name: 'prefix AND group', + object: { $and: [{ stage: 'won' }, { owner: 'u1' }] }, + array: ['and', ['stage', '=', 'won'], ['owner', '=', 'u1']], + seeded: ['owner', 'stage'], + }, + { + // GREEN before and after — see the reverse-verification note. A disjunction + // contributes no dimension on either spelling. + name: 'prefix OR group — the disjunction a flat array could never carry', + object: { $or: [{ stage: 'won' }, { stage: 'lost' }] }, + array: ['or', ['stage', '=', 'won'], ['stage', '=', 'lost']], + seeded: [], + }, + { + // The shape that makes descending `$and` NECESSARY rather than tidy: the + // flat array is the array spelling of `{stage: …, owner: …}`, and + // `parseFilterAST` lowers it to `{$and: […]}`. Read only the lowered + // object's own top-level keys and the answer would be `$and` alone — i.e. + // nothing — and the two spellings would still mint two cubes. + name: 'legacy flat list — implicit AND', + object: { $and: [{ stage: 'won' }, { owner: 'u2' }] }, + array: [['stage', '=', 'won'], ['owner', '=', 'u2']], + seeded: ['owner', 'stage'], + }, + { + name: 'set membership', + object: { stage: { $in: ['won', 'lost'] } }, + array: ['stage', 'in', ['won', 'lost']], + seeded: ['stage'], + }, + { + name: 'null predicate — two-element node, direction from the operator name', + object: { closed_at: { $null: true } }, + array: ['closed_at', 'is_null'], + seeded: ['closed_at'], + }, + { + name: 'not-null predicate', + object: { closed_at: { $null: false } }, + array: ['closed_at', 'is_not_null'], + seeded: ['closed_at'], + }, + { + // The lowered predicate is the boolean constant FALSE and names no member in + // the TREE (`collectFilterLeaves` returns nothing for a `const` node) — but + // the filter still names the field `stage`, and the cube's vocabulary is + // about what the author wrote, not what the predicate binds. A second reason + // the two readers want different views of one filter. + name: 'empty set membership — the boolean constant FALSE, not "no filter"', + object: { stage: { $in: [] } }, + array: ['stage', 'in', []], + seeded: ['stage'], + }, + { + name: 'range — `between` lowers to its two bounds on both spellings', + object: { amount: { $between: [15, 35] } }, + array: ['amount', 'between', [15, 35]], + seeded: ['amount'], + }, + { + // GREEN before and after, for the `prefix OR group` reason. + name: 'nested group — OR of an AND', + object: { $or: [{ $and: [{ stage: 'won' }, { owner: 'u1' }] }, { stage: 'lost' }] }, + array: ['or', ['and', ['stage', '=', 'won'], ['owner', '=', 'u1']], ['stage', '=', 'lost']], + seeded: [], + }, +]; + +// ── 1. The parity the issue asked for ──────────────────────────────────────── + +describe('[#5353] inferCubeFromQuery — the `where` spelling does not change the cube', () => { + for (const c of EQUIVALENT_SPELLINGS) { + it(`mints one dimension vocabulary for both spellings: ${c.name}`, async () => { + const objectSpelling = await inferredDimensions(c.object); + const arraySpelling = await inferredDimensions(c.array); + + // `count` is the measure every inferred cube carries; dimensions are the + // filter's contribution alone (the query names no `dimensions`). + expect(objectSpelling.dimensions).toEqual(c.seeded); + expect(arraySpelling.dimensions).toEqual(c.seeded); + // Stated as its own assertion so a failure reads as the DEFECT ("the two + // spellings disagree") and not merely as a wrong expected value. + expect(arraySpelling.dimensions).toEqual(objectSpelling.dimensions); + }); + } + + it('seeds the `where` keys ALONGSIDE the ones `dimensions` and `measures` contribute', async () => { + const { service } = makeService(); + await service.query({ + cube: 'deal', + measures: ['amount_sum'], + dimensions: ['stage'], + where: [['owner', '=', 'u1']], + } as never); + const [meta] = await service.getMeta('deal'); + + expect(meta.dimensions.map((d) => d.name).sort()).toEqual(['deal.owner', 'deal.stage']); + // The measure arm is untouched by #5353 — `amount_sum` still infers a SUM + // over `amount` rather than becoming a dimension. + expect(meta.measures.map((m) => m.name).sort()).toEqual(['deal.amount_sum', 'deal.count']); + }); +}); + +// ── 2. What the fix must NOT change ────────────────────────────────────────── + +describe('[#5353] the seeded dimensions change no verdict and no statement', () => { + it('compiles the identical SQL for both spellings — measured, not assumed', async () => { + const objectSpelling = await inferredDimensions({ stage: 'won' }, { native: true }); + const arraySpelling = await inferredDimensions([['stage', '=', 'won']], { native: true }); + + 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. + expect(objectSpelling.sqls[0]).toContain('WHERE stage = '); + expect(objectSpelling.sqls[0]).not.toContain('"deal"."stage"'); + }); + + it('hands the engine the identical filter for both spellings', async () => { + const objectSpelling = await inferredDimensions({ stage: 'won' }); + const arraySpelling = await inferredDimensions([['stage', '=', 'won']]); + + expect(arraySpelling.filters).toEqual(objectSpelling.filters); + expect(objectSpelling.filters).toEqual([{ stage: 'won' }]); + }); + + it('still rejects a bogus filter field on BOTH spellings, with the same envelope', async () => { + const objectErr = await rejection( + makeService().service.query({ cube: 'deal', measures: ['count'], where: { bogus_col: 'x' } } as never), + ); + const arrayErr = await rejection( + makeService().service.query({ + cube: 'deal', + measures: ['count'], + where: [['bogus_col', '=', 'x']], + } as never), + ); + + for (const err of [objectErr, arrayErr]) { + expect((err as { code?: string }).code).toBe('INVALID_FIELD'); + expect((err as { status?: number }).status).toBe(400); + expect((err as { field?: string }).field).toBe('bogus_col'); + expect((err as { param?: string }).param).toBe('where'); + } + // #5669's gate reads filter LEAVES, not `cube.dimensions`, so seeding the + // array spelling's keys could not change its verdict — and did not. What it + // DID change is the suggestion list, in the direction that closes the split: + // one filter now gets one message whichever way it is spelled. + expect(arrayErr.message).toBe(objectErr.message); + }); + + it('stands down when the `where` array cannot be lowered — the refusal stays in the strategy', async () => { + // `[{stage:'won'}]` is #5334's own unlowerable repro: a list of CONDITION + // OBJECTS. `inferCubeFromQuery` must not raise from `ensureCube`, or the + // answer's geography moves and the draft-preview path (whose `matchesWhere` + // never consults the normalizer) would newly refuse. + const err = await rejection( + makeService({ native: true }).service.query({ + cube: 'deal', + measures: ['count'], + where: [{ stage: 'won' }], + } as never), + ); + + expect((err as { code?: string }).code).toBe('INVALID_FILTER'); + expect(err.message).toMatch(/is not a filter/); + }); + + it('treats `[]` as no filter, seeding nothing and refusing nothing', async () => { + const { dimensions, sqls } = await inferredDimensions([], { native: true }); + expect(dimensions).toEqual([]); + expect(sqls[0]).not.toContain('WHERE'); + }); +}); + +// ── 3. The #5739 residue: dotted keys, NOT unified, and why ────────────────── + +/** + * These pin what #5353 deliberately did NOT fix. A dotted `where` key still + * answers per spelling, because unifying it means choosing a direction that + * belongs to #5739 — and both directions are measured here so the choice is made + * on facts rather than on which spelling someone tried first. + */ +describe('[#5353] a dotted `where` key keeps its per-spelling answer (#5739 owns it)', () => { + /** No base `region` column — the shape a relation filter is normally written against. */ + const NO_REGION = { fields: ['id', 'stage', 'owner', 'amount', 'closed_at'], native: true }; + + it('the OBJECT spelling still mints the stripped tail as a base-table dimension', async () => { + // `origin/main`'s behaviour, reproduced verbatim by the residue loop. The + // minted `region` is then found by `declaredMemberEntry`'s dotted tail lookup, + // so #5669's gate resolves the member `owner.region` to a base column `deal` + // does not have and refuses — which #5740 pinned as the honest answer given + // what actually reaches the driver on this path. + const err = await rejection( + makeService(NO_REGION).service.query({ + cube: 'deal', + measures: ['count'], + where: { 'owner.region': 'NA' }, + } as never), + ); + expect((err as { code?: string }).code).toBe('INVALID_FIELD'); + expect(err.message).toMatch(/constrains field 'region'/); + }); + + it('the ARRAY spelling still mints nothing, and compiles the traversal', async () => { + // The other half of the residue. Propagating the mint to this spelling is the + // measured REGRESSION #5353 refused: this query runs today and would become + // either a base-column filter over different rows or — as the case above + // shows — a 400. + const { dimensions, sqls } = await inferredDimensions([['owner.region', '=', 'NA']], NO_REGION); + + expect(dimensions).toEqual([]); + expect(sqls[0]).toContain('LEFT JOIN "owner" ON "deal"."owner" = "owner"."id"'); + expect(sqls[0]).toContain('WHERE "owner"."region" = '); + }); + + it('a nested relation object seeds its RELATION key, not the tail', async () => { + // `{owner: {region: 'NA'}}`'s top-level key is the bare `owner`, so it seeds + // `owner` — unchanged. The LEAF member is `owner.region`, which is why a + // `collectFilterLeaves`-based seeder would have produced `region` here and + // walked into the mis-cast above from a third direction. + const { dimensions, sqls } = await inferredDimensions({ owner: { region: 'NA' } }, NO_REGION); + expect(dimensions).toEqual(['owner']); + expect(sqls[0]).toContain('WHERE "owner"."region" = '); + }); + + it('bare keys reach parity even when a dotted key rides along', async () => { + // The residue is scoped to the dotted key alone: `stage` is unified, and the + // whole query is still refused for `region` — one rejection at a time, naming + // a real mistake either way. + const { dimensions } = await inferredDimensions( + [['stage', '=', 'won'], ['owner.region', '=', 'NA']], + NO_REGION, + ); + expect(dimensions).toEqual(['stage']); + }); +}); diff --git a/packages/services/service-analytics/src/analytics-service.ts b/packages/services/service-analytics/src/analytics-service.ts index 90cee40a22..ae55e50592 100644 --- a/packages/services/service-analytics/src/analytics-service.ts +++ b/packages/services/service-analytics/src/analytics-service.ts @@ -19,7 +19,12 @@ import { ObjectQLStrategy } from './strategies/objectql-strategy.js'; // [#5669] The `where` source-field gate reads the filter tree through the SAME // pair the strategies compile it with, so "the field the gate saw" and "the // column that reached SQL" cannot be two different things. -import { normalizeAnalyticsFilterTree, collectFilterLeaves } from './strategies/filter-normalizer.js'; +import { + normalizeAnalyticsFilterTree, + collectFilterLeaves, + lowerAnalyticsWhere, + conjunctFieldKeys, +} from './strategies/filter-normalizer.js'; import { compileDataset, type CompiledDataset, type RelationshipResolver } from './dataset-compiler.js'; import { DatasetExecutor, resolveDimensionGranularity, type DateGranularityValue } from './dataset-executor.js'; import { @@ -1441,20 +1446,27 @@ export class AnalyticsService implements IAnalyticsService { * - Per member, {@link resolveMemberSource} stands down on an expression `sql` * and on a dotted relation traversal — for the dimension gate's reasons. * - * # Array `where` IS gated, and that is not #5353's territory + * # Array `where` IS gated, and #5353's fix did not change that + * + * Since #5334 an array `where` is lowered by `normalizeAnalyticsFilterTree` + * and compiles to the identical predicate — a measured fact, + * `where: [['bogus_col','=','x']]` and `where: {bogus_col: 'x'}` both produce + * `WHERE bogus_col = $1` and hand `executeAggregate` the same + * `{bogus_col: 'x'}`. Gating one spelling and not the other would answer one + * mistake two ways, which is the split this whole gate family exists to close. * - * `inferCubeFromQuery` still skips an array `where` when minting the ad-hoc - * cube's `dimensions` (the stale `!Array.isArray` guard #5353 records). That - * skip is about the cube's dimension VOCABULARY. It says nothing about which - * columns reach the driver: since #5334 an array `where` is lowered by - * `normalizeAnalyticsFilterTree` and compiles to the identical predicate — a - * measured fact, `where: [['bogus_col','=','x']]` and - * `where: {bogus_col: 'x'}` both produce `WHERE bogus_col = $1` and hand - * `executeAggregate` the same `{bogus_col: 'x'}`. Gating one spelling and not - * the other would answer one mistake two ways, which is the split this whole - * gate family exists to close. Nothing here changes `inferCubeFromQuery`, so - * #5353 is untouched — and because this gate reads leaves rather than - * `cube.dimensions`, #5353's fix cannot change its verdicts either. + * `inferCubeFromQuery` used to skip an array `where` when minting the ad-hoc + * cube's `dimensions` — a separate question (the cube's dimension VOCABULARY, + * not which columns reach the driver), fixed by #5353 by lowering before + * reading keys. Because this gate reads filter LEAVES rather than + * `cube.dimensions`, that fix could not change its verdicts, and measurement + * confirms it did not: the array where's keys now reach `cube.dimensions`, so + * {@link resolveMemberSource} takes the DECLARED-dimension branch for those + * members instead of the undeclared-bare-column one — and both branches yield + * the same `source` for the same member, since the minted dimension's `sql` IS + * the member name. What did change is the rejection's suggestion list, in the + * direction that closes the split: `Valid filter members:` now reads the same + * for both spellings of one filter. */ private assertWhereFields(query: AnalyticsQuery, cube: Cube, declaredDimensions: string[]): void { const probe = this.getObjectFieldNames; @@ -1479,9 +1491,10 @@ export class AnalyticsService implements IAnalyticsService { if (members.length === 0) return; // Two passes, for the reason the measure and dimension gates have two: on the - // auto-inference path `inferCubeFromQuery` mints the `where`'s own top-level - // keys into `cube.dimensions`, so echoing that bag verbatim would offer the - // caller their own typo back as a valid filter member. + // auto-inference path `inferCubeFromQuery` mints the `where`'s own field keys + // into `cube.dimensions` — since #5353 for the array spelling too — so + // echoing that bag verbatim would offer the caller their own typo back as a + // valid filter member. const invalid = new Set(); for (const member of members) { const { key, source } = resolveMemberSource(cube, member, 'any'); @@ -1574,18 +1587,83 @@ export class AnalyticsService implements IAnalyticsService { dimensions[key] = { name: key, label: key, type: 'string', sql: key }; } - if (query.where && typeof query.where === 'object' && !Array.isArray(query.where)) { - // Canonical FilterCondition: top-level keys (excluding logical - // combinators) are field names. We only need them to seed an - // ad-hoc cube definition for free-form queries. - // - // The `!Array.isArray` guard predates #5334 and is stale — an array `where` - // IS a filter now, and its fields do not get seeded here. That is #5353, - // deliberately left alone. It does NOT weaken #5669's `where` gate, which - // reads the lowered filter TREE rather than this bag, so both spellings are - // judged identically today and #5353's eventual fix cannot change that. - for (const key of Object.keys(query.where as Record)) { - if (key.startsWith('$')) continue; + // The `where`'s field keys seed dimensions too. LOWER FIRST, then read keys: + // the rule this bag has always followed is "the `where`'s own top-level keys + // are field names", and the only reason an ARRAY `where` was skipped here is + // that it was not a filter when the code was written. + // + // [#5353] The `!Array.isArray(query.where)` guard this replaces predates + // #5334. Since #5334 an array `where` IS a filter — lowered by + // `lowerAnalyticsWhere` and compiling to a byte-identical predicate — so + // skipping it meant ONE filter, spelled two ways, minted two different + // cubes: `{stage: 'won'}` seeded `dimensions.stage`, `[['stage','=','won']]` + // seeded nothing. Lowering first makes the spelling stop mattering, which is + // the same fix #5334 applied one layer down. + let lowered: Record | null = null; + try { + lowered = lowerAnalyticsWhere(query); + } catch { + // A `where` the lowering REFUSES (an unlowerable array) is not judged + // here — the same stand-down `assertWhereFields` makes, for the same + // reason: that refusal already happens in the strategy with an + // `INVALID_FILTER`/400 envelope (#5352/#5367), and raising it from + // `ensureCube` instead would move the answer's geography and would newly + // refuse the draft-preview path, whose `matchesWhere` never consults the + // normalizer at all. A cube minted without those keys is exactly what a + // query that is about to be refused needs. + } + if (lowered) { + // `conjunctFieldKeys` descends `$and` — which the LOWERING introduces for a + // flat array (`[[a,…],[b,…]]` → `{$and: [{a…},{b…}]}`) — and not `$or` / + // `$not`, which contribute no key on either spelling. Deliberately NOT + // `collectFilterLeaves`: the leaves answer "what does the compiled + // predicate BIND", this bag answers "what may a caller NAME", and the two + // genuinely differ — `{stage: {$in: []}}` lowers to the boolean constant + // FALSE, binding nothing while still naming `stage`. + for (const key of conjunctFieldKeys(lowered)) { + // BARE keys only — a dotted one is a relation traversal, and #5353 cannot + // unify those. See the residue loop below for why, and for the ONE case + // that still answers per-spelling. + if (key.includes('.')) continue; + if (dimensions[key] || measures[key]) continue; + dimensions[key] = { name: key, label: key, type: 'string', sql: key }; + } + } + + // ── #5739 residue: dotted keys, still answered per SPELLING ─────────────── + // + // A dotted key names a relation traversal, and an ad-hoc single-table cube + // has nothing to declare it as. `stripPrefix` mints the TAIL as a BASE-TABLE + // dimension, which is #5739's mis-cast, and today only the OBJECT spelling + // reaches that mint. #5353 can go neither way on its own: + // + // - PROPAGATE it to the array spelling (`stripPrefix` in the loop above) is + // a measured REGRESSION. On cube `deal`, filter `owner.region = 'NA'`: + // {'owner.region': 'NA'} → WHERE region = $1 (both, after) + // [['owner.region','=','NA']] → LEFT JOIN "owner" ON "deal"."owner" = + // "owner"."id" WHERE "owner"."region" = $1 + // (before) + // …i.e. a working traversal becomes a different-rows base-column filter — + // and where the base has no `region` column, a 400 INVALID_FIELD, because + // `declaredMemberEntry`'s dotted tail lookup resolves `owner.region` to + // the minted dimension and hands #5669's gate a column that is absent. + // - WITHDRAW it from the object spelling (skip dotted keys entirely) breaks + // an invariant #5740 pinned deliberately: one dotted member gets one + // answer across the `where` and `dimensions` request keys, sharing + // `resolveMemberSource`, and "if that reading is ever judged wrong it must + // change for BOTH keys at once". Withdrawing here alone would leave + // `where: {'owner.region': …}` standing down while + // `dimensions: ['owner.region']` still 400s on `region`. + // + // Both directions are #5739's to choose, so this loop reproduces `origin/main` + // verbatim — the OBJECT `where`'s own top-level dotted keys, `stripPrefix`ed — + // and the asymmetry #5353 is about survives for dotted keys alone. Delete the + // loop (or fold it into the one above) when #5739 rules; the parity test names + // the case that then flips. + const rawWhere = (query as { where?: unknown }).where; + if (rawWhere && typeof rawWhere === 'object' && !Array.isArray(rawWhere)) { + for (const key of Object.keys(rawWhere as Record)) { + if (key.startsWith('$') || !key.includes('.')) continue; const stripped = stripPrefix(key); if (dimensions[stripped] || measures[stripped]) continue; dimensions[stripped] = { name: stripped, label: stripped, type: 'string', sql: stripped }; diff --git a/packages/services/service-analytics/src/strategies/filter-normalizer.ts b/packages/services/service-analytics/src/strategies/filter-normalizer.ts index 0189d2d99b..8251a576d7 100644 --- a/packages/services/service-analytics/src/strategies/filter-normalizer.ts +++ b/packages/services/service-analytics/src/strategies/filter-normalizer.ts @@ -900,6 +900,101 @@ function filterArrayNotLowerableError(where: unknown[]): Error { ); } +/** + * Lower an analytics query's `where` to the CANONICAL `FilterCondition` object, + * before any node is built. `null` when the query carries no `where`. + * + * Extracted from {@link normalizeAnalyticsFilterTree} for #5353's second reader + * (`inferCubeFromQuery`, which needs the lowered condition's own KEYS rather + * than the compiled tree's leaves — see that function for why the two readers + * want different views of one filter). The three arrival answers documented on + * {@link normalizeAnalyticsFilterTree} are all decided HERE; that function is + * now this lowering plus {@link buildNode}. + * + * Keeping the lowering in ONE place is the point of the extraction. The + * alternative — a second `isFilterAST`/`parseFilterAST` call at the new reader — + * is how "the shape the cube was minted from" and "the shape that reached SQL" + * drift apart, and both refusal paths below would then have had to be + * re-derived to stay in step. + */ +export function lowerAnalyticsWhere( + query: { where?: unknown } | unknown, +): Record | null { + if (!query || typeof query !== 'object') return null; + const where = (query as { where?: unknown }).where; + if (!where || typeof where !== 'object') return null; + + if (Array.isArray(where)) { + // (1) `[]` is "no filter", not a failed filter. + if (where.length === 0) return null; + // (3) Not a shape `parseFilterAST` can express. + if (!isFilterAST(where)) throw filterArrayNotLowerableError(where); + // (2) The declared path. + const condition = parseFilterAST(where); + if (!condition || typeof condition !== 'object' || Array.isArray(condition)) { + // Unreachable by construction — `isFilterAST` accepted the shape, so + // `parseFilterAST` has a lowering for it. Loud rather than silent for the + // same reason the engine door is: the failure mode of the two spec + // functions disagreeing is a dropped predicate, i.e. every row. + throw invalidFilterError( + `[analytics] filter array ${JSON.stringify(where)} passed isFilterAST() but ` + + `parseFilterAST() lowered it to ${JSON.stringify(condition)}. Refusing rather than ` + + `charting the dataset unfiltered (#5158/#5334).`, + ); + } + return condition as Record; + } + + return where as Record; +} + +/** + * The FIELD KEYS a lowered `FilterCondition` names in its top-level + * CONJUNCTION — `$and` descended through, `$or` / `$not` deliberately not. + * + * # Why a conjunction walker and not {@link collectFilterLeaves} + * + * This answers a VOCABULARY question, not a predicate question: #5353's caller + * mints an ad-hoc cube's `dimensions` from the `where`, and the rule that bag + * has always followed is "the `where`'s own top-level keys are field names". + * `collectFilterLeaves` answers a different question (every member the compiled + * predicate binds, structure discarded) and substituting it here would have + * changed three behaviours #5353 does not ask about — see the caller's note. + * + * `$and` is descended because the LOWERING ITSELF introduces it: a flat filter + * array `[[a,…],[b,…]]` is the array spelling of the object `{a…, b…}`, and + * `parseFilterAST` lowers it to `{$and: [{a…}, {b…}]}`. Without descending, the + * two spellings of one filter would still mint two different cubes — the whole + * defect. Conjunction is associative and flat, so nested `$and`s are descended + * too; recursing at all is safe here precisely because every entry of one + * object ANDs with its siblings at every depth (`buildNode`'s rule). + * + * `$or` / `$not` are NOT descended, and both spellings agree on that today: + * `{$or: […]}` and `["or", …]` each contribute no key. Reading a disjunction's + * branches as cube dimensions is a separate question from #5353's asymmetry — + * and answering it would force a policy on DOTTED members that #5739 owns. + */ +export function conjunctFieldKeys(condition: Record): string[] { + const keys: string[] = []; + const walk = (cond: Record): void => { + for (const [key, value] of Object.entries(cond)) { + if (key === '$and' && Array.isArray(value)) { + for (const child of value) { + if (isFilterObject(child)) walk(child); + } + continue; + } + // Every other `$` key is a combinator this walk does not enter (`$or`, + // `$not`) or an operator that belongs to a field ENTRY, not to the + // condition — neither names a field here. + if (key.startsWith('$')) continue; + keys.push(key); + } + }; + walk(condition); + return keys; +} + /** * Normalize an analytics query's `where` into the tree the strategies compile. * `null` when the query carries no `where` — i.e. no constraint. @@ -908,8 +1003,8 @@ function filterArrayNotLowerableError(where: unknown[]): Error { * object form is the whole of the contract downstream. An ARRAY nevertheless * arrives — it is the `FilterArray` authoring sugar four published contracts * teach, and analytics is a door into the runtime like any other — so it is - * LOWERED here (#5334, on #5158's ruling C), giving the same three answers - * `ObjectQL`'s six entry points give since #5329: + * LOWERED by {@link lowerAnalyticsWhere} (#5334, on #5158's ruling C), giving + * the same three answers `ObjectQL`'s six entry points give since #5329: * * 1. `[]` — "no filter", not a failed filter: `null`, the same reading every * layer gives it (the engine door DELETES the key; `parseFilterAST([])` is @@ -931,32 +1026,9 @@ function filterArrayNotLowerableError(where: unknown[]): Error { export function normalizeAnalyticsFilterTree( query: { where?: unknown } | unknown, ): NormalizedFilterNode | null { - if (!query || typeof query !== 'object') return null; - const where = (query as { where?: unknown }).where; - if (!where || typeof where !== 'object') return null; - - if (Array.isArray(where)) { - // (1) `[]` is "no filter", not a failed filter. - if (where.length === 0) return null; - // (3) Not a shape `parseFilterAST` can express. - if (!isFilterAST(where)) throw filterArrayNotLowerableError(where); - // (2) The declared path. - const condition = parseFilterAST(where); - if (!condition || typeof condition !== 'object' || Array.isArray(condition)) { - // Unreachable by construction — `isFilterAST` accepted the shape, so - // `parseFilterAST` has a lowering for it. Loud rather than silent for the - // same reason the engine door is: the failure mode of the two spec - // functions disagreeing is a dropped predicate, i.e. every row. - throw invalidFilterError( - `[analytics] filter array ${JSON.stringify(where)} passed isFilterAST() but ` + - `parseFilterAST() lowered it to ${JSON.stringify(condition)}. Refusing rather than ` + - `charting the dataset unfiltered (#5158/#5334).`, - ); - } - return buildNode(condition as Record); - } - - return buildNode(where as Record); + const condition = lowerAnalyticsWhere(query); + if (!condition) return null; + return buildNode(condition); } /**