From 4709aa6dc964ed2d137582cdb8441179743324d7 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 6 Aug 2026 03:15:27 +0000 Subject: [PATCH] =?UTF-8?q?fix(service-analytics):=20gate=20`where`=20sour?= =?UTF-8?q?ce=20fields=20=E2=80=94=20a=20filter=20over=20a=20missing=20fie?= =?UTF-8?q?ld=20is=20400=20INVALID=5FFIELD,=20not=20a=20driver=20500=20(#5?= =?UTF-8?q?669)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `ensureCube` carried two source-field gates — `assertMeasureFields` (#4437) and `assertDimensionFields` (#5520) — and none for the filter face. A `where` naming a field the object does not have compiled straight into the statement and came back as a driver error with no envelope: POST /analytics/query {"cube":"crm_account","measures":["count"], "where":{"bogus_col":"x"}} -> SELECT COUNT(*) AS "count" FROM "crm_account" WHERE bogus_col = $1 -> 500 {"code":"SQLITE_ERROR","message":"Internal server error"} `assertWhereFields` now runs after the other two on every `ensureCube` path, refusing before any SQL is built with the siblings' envelope: INVALID_FIELD/400 plus field/object/param='where'. `query`, `generateSql` and `queryDataset` (runtimeFilter and a dataset's own declared filter) are covered, and a rejected query leaves nothing in the cube registry. `/analytics/dataset/query` needed no change — #5352's envelope branch already carries a coded 4xx through — and the new rest-face test pins that end to end. Members are collected through `normalizeAnalyticsFilterTree` + `collectFilterLeaves`, the same pair both strategies compile the predicate with, so combinator nesting, `$`-operator keys, `$between` lowering, the nested relation dot flattening and the #5334 array spelling are read exactly as they will be compiled — not by a second walker that could drift from it. #5520's member->column resolution is extracted to the module-level `resolveMemberSource`, shared by both gates. Its new `kind` parameter is load-bearing rather than cosmetic: a filter member resolves through dimensions AND measures (what `resolveFieldSql` / `resolveFieldName(.., 'any')` do), so a dimensions-only lookup would have rejected `where: {revenue: {$gt: 100}}` on a cube declaring `measures.revenue = {sql: 'annual_revenue'}` — a query that works on both strategies today. Deliberately unchanged: filtering on a real field the cube never declared; a declared member followed to its real column; id/created_at/updated_at admitted; expression `sql`, dotted relation traversals and a probe-less host all stood down on; and the INVALID_FILTER family untouched — a `where` the normalizer refuses outright is not judged here, so those refusals stay where they already happen (#5352/#5367) and the draft-preview path, whose matcher never consults the normalizer, is not newly refused. Array `where` IS gated, and that is not #5353's territory: `inferCubeFromQuery` still skips it when minting the ad-hoc cube's dimension vocabulary, but since #5334 the array spelling lowers to the identical predicate (measured: both produce `WHERE bogus_col = $1`), so gating one spelling only would answer one mistake two ways. Fixes #5669 Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01BWS4heBoAitLmzCLhcYdbK --- .../analytics-where-source-field-gate.md | 69 ++ .../src/analytics-dataset-where-gate.test.ts | 223 ++++++ .../__tests__/where-source-field-gate.test.ts | 739 ++++++++++++++++++ .../src/analytics-service.ts | 292 ++++++- 4 files changed, 1282 insertions(+), 41 deletions(-) create mode 100644 .changeset/analytics-where-source-field-gate.md create mode 100644 packages/rest/src/analytics-dataset-where-gate.test.ts create mode 100644 packages/services/service-analytics/src/__tests__/where-source-field-gate.test.ts diff --git a/.changeset/analytics-where-source-field-gate.md b/.changeset/analytics-where-source-field-gate.md new file mode 100644 index 0000000000..21413977b3 --- /dev/null +++ b/.changeset/analytics-where-source-field-gate.md @@ -0,0 +1,69 @@ +--- +"@objectstack/service-analytics": patch +--- + +fix(service-analytics): an analytics `where` over a missing field answers 400 INVALID_FIELD, not a driver 500 (#5669) + +`ensureCube` carried two source-field gates — `assertMeasureFields` (#4437, +`param: 'measures'`) and `assertDimensionFields` (#5520, +`param: 'dimensions' | 'timeDimensions'`) — and none for the filter face, the +request key most likely to carry a hand-typed field name. A `where` naming a +field the object does not have compiled straight into the statement and came +back as a driver error with no envelope: + +``` +POST /analytics/query {"cube":"crm_account","measures":["count"],"where":{"bogus_col":"x"}} +→ SELECT COUNT(*) AS "count" FROM "crm_account" WHERE bogus_col = $1 +→ 500 {"code":"SQLITE_ERROR","message":"Internal server error"} + +# the control group on the same route, already fixed by #4437 / #5520 +POST /analytics/query {"cube":"crm_account","measures":["count"],"dimensions":["bogus_dim"]} +→ 400 {"code":"INVALID_FIELD","message":"Dimension 'bogus_dim' … "} +``` + +A driver error class as the caller's `error.code` for a caller-shaped mistake is +the ADR-0112 fault #4437 was filed about; the `/data` route has answered the same +typo with a field-naming 400 since #4315/#4254. + +**The gate.** `ensureCube` now runs `assertWhereFields` after the other two on +every path, so a filter whose source column the backing object does not have is +refused **before** any SQL is built, with the same envelope its two siblings +use: `INVALID_FIELD` / 400 plus `field` / `object` / `param: 'where'`, and a +message naming the field, the valid filter members and the object's known field +list. `query`, `generateSql` and `queryDataset` (both `runtimeFilter` and a +dataset's own declared `filter`) are covered, and a rejected query leaves +nothing behind in the cube registry. `/analytics/dataset/query` needed no +change: #5352's envelope branch already carries a coded 4xx through, which the +new REST-face test pins end to end. + +**Field names come from the SQL producer's own reader.** The members are +collected through `normalizeAnalyticsFilterTree` + `collectFilterLeaves` — the +same pair both strategies call to build the predicate — rather than by walking +the raw `where` object. So `$and`/`$or`/`$not` nesting, `$`-prefixed operator +keys, `$between` lowering, the `{owner: {region: 'NA'}}` → `owner.region` +flattening and the #5334 array spelling are all read exactly as they will be +compiled, in one place, instead of in a second walker that could drift from it. + +**What deliberately did not change:** + +- Filtering on a REAL field the cube never declared (`where: {phone: '555'}`) + still works — the gate asks "does the *object* have this field", never "did the + cube declare it". +- A filter member resolves through `cube.dimensions` **and** `cube.measures`, + which is what the strategies do: a cube declaring + `measures.revenue = {sql: 'annual_revenue'}` still answers + `where: {revenue: {$gt: 100}}` as `annual_revenue > ?`. +- A declared member is followed to its real column, so a dimension `assessed` + over column `assessed_at` is not judged by its own name. +- `id` / `created_at` / `updated_at` stay admitted unconditionally, matching the + data path's `resolveQueryFields`. +- An expression `sql` (on the cube or on a member), a dotted relation traversal, + and a host that wires no field-name probe are all stood down on, exactly as the + measure and dimension gates stand down. +- The `INVALID_FILTER` family is untouched. A `where` the normalizer refuses + outright — an unknown operator, a zero-operator field constraint, an + unlowerable filter array — is *not* judged here: the gate stands down and the + refusal stays where it already happens (#5352 / #5367's geography). A field + gate that cannot read the tree has nothing to say about it, and pulling those + refusals forward would also have newly refused them on the draft-preview path, + whose matcher never consults the normalizer. diff --git a/packages/rest/src/analytics-dataset-where-gate.test.ts b/packages/rest/src/analytics-dataset-where-gate.test.ts new file mode 100644 index 0000000000..1baf066f32 --- /dev/null +++ b/packages/rest/src/analytics-dataset-where-gate.test.ts @@ -0,0 +1,223 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#5669] `POST /analytics/dataset/query` — the caller's own view of the `where` + * source-field gate. + * + * The third and last param of the defect #4437 (measures) and #5520 (dimensions) + * closed one key at a time. A filter naming a field the object does not have + * reached the driver, came back as a driver error with no envelope, and this + * route answered `500 ANALYTICS_QUERY_FAILED` for what is a plain typo — the + * same classification fault, on the request key most likely to carry a + * hand-typed field name. Measured on this harness against `origin/main`: + * + * ``` + * {"dataset":…,"selection":{"measures":["account_count"],"runtimeFilter":{"bogus_col":"x"}}} + * → SELECT COUNT(*) AS "account_count" FROM "crm_account" WHERE bogus_col = $1 + * → 500 ANALYTICS_QUERY_FAILED + * ``` + * + * Why this file exists next to the service-side pin + * (`service-analytics`'s `where-source-field-gate.test.ts`): "the service throws + * the right shape" and "the caller receives it" are different facts, separated by + * this route's catch. The gate needs no rest-layer change at all — #5352's + * envelope branch ① reads `code` + 4xx `status` and carries the verdict through — + * and that is precisely the claim worth pinning end to end, because it is a + * claim about a seam neither side's unit tests cross. So the provider here is a + * REAL `AnalyticsService` whose driver double fails the way SQLite/knex does + * (statement prefixed to the cause), not a mock that would assume half the seam. + * + * `runtimeFilter` is the load-bearing input for the same reason + * `analytics-filter-refusal-envelope.test.ts` uses it: it is the + * presentation-scope filter a dashboard widget carries, i.e. exactly the field + * an author typos. + * + * ## Reverse verification, direction predicted BEFORE running + * + * Remove the three `assertWhereFields` calls from `ensureCube` and rebuild + * `@objectstack/service-analytics` (this file exercises the BUILT package — + * mutating sources without rebuilding proves nothing here): the three rejection + * cases go RED, answering `500 ANALYTICS_QUERY_FAILED` again, while the positive + * control and the `INVALID_FILTER` case — neither of which this gate produces — + * stay GREEN. Ordinary direction. Predicted 3 red / 2 green; measured exactly + * that, each red reading `expected 500 to be 400`. + * + * Note the "carries no generated SQL" case asserts the 400 as well as the + * absence of a statement — #5520's file records why: with the gate gone, a + * leak-free body proves nothing on its own, because the sibling fix (#5520's + * sanitiser on the 500 branch) withholds the driver message anyway. Each fix + * must be falsifiable on its own. + */ + +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import type { Logger } from '@objectstack/spec/contracts'; +import { AnalyticsService } from '@objectstack/service-analytics'; +import { RestServer } from './rest-server'; + +// ── harness (the shape the two sibling analytics rest tests use) ───────────── + +function mockServer() { + return { + get: vi.fn(), post: vi.fn(), put: vi.fn(), delete: vi.fn(), patch: vi.fn(), + use: vi.fn(), listen: vi.fn().mockResolvedValue(undefined), close: vi.fn().mockResolvedValue(undefined), + }; +} +function mockProtocol() { + return { + getDiscovery: vi.fn().mockResolvedValue({ version: 'v0', endpoints: {} }), + getMetaTypes: vi.fn().mockResolvedValue([]), + getMetaItems: vi.fn().mockResolvedValue([]), + }; +} +function mockRes() { + const res: any = { statusCode: 200, body: undefined }; + res.status = vi.fn((c: number) => { res.statusCode = c; return res; }); + res.json = vi.fn((b: any) => { res.body = b; return res; }); + res.end = vi.fn(() => res); + return res; +} + +/** The dataset from the issue's repro — one declared dimension, one measure. */ +const dataset = { + name: 'account_metrics', + label: 'Account metrics', + object: 'crm_account', + dimensions: [{ name: 'industry', field: 'industry', type: 'string' }], + measures: [{ name: 'account_count', aggregate: 'count' }], +}; + +const ACCOUNT_FIELDS = ['id', 'name', 'phone', 'industry', 'annual_revenue']; + +function buildRoute(analyticsProvider?: any) { + const rest = new RestServer( + mockServer() as any, mockProtocol() as any, { api: { requireAuth: false } } as any, + undefined, undefined, undefined, undefined, undefined, undefined, undefined, + undefined, undefined, undefined, undefined, + analyticsProvider, + ); + (rest as any).resolveExecCtx = async () => ({ userId: 'test-user' }); + rest.registerRoutes(); + return rest.getRoutes().find((r) => r.method === 'POST' && r.path.endsWith('/analytics/dataset/query'))!; +} + +/** + * A REAL `AnalyticsService` on the native-SQL path whose driver double fails the + * way the SQLite/knex one does: the statement prefixed to the cause. That is + * what made the pre-fix 500 body carry the generated SQL, so the harness + * reproduces it rather than asserting about a hypothetical message. + */ +function realAnalytics(): AnalyticsService { + const silent: Logger = { debug() {}, info() {}, warn() {}, error() {} }; + return new AnalyticsService({ + logger: silent, + queryCapabilities: () => ({ nativeSql: true, objectqlAggregate: false, inMemory: false }), + executeRawSql: async (_object: string, sql: string) => { + const bogus = /\b(bogus_col|dropped_column)\b/.exec(sql)?.[0]; + if (bogus) throw new Error(`${sql} - no such column: ${bogus}`); + return [{ industry: 'tech', account_count: 3 }]; + }, + isRegisteredObject: (n: string) => n === 'crm_account', + getObjectFieldNames: (n: string) => (n === 'crm_account' ? ACCOUNT_FIELDS : undefined), + }); +} + +async function post(route: any, body: unknown) { + const res = mockRes(); + await route.handler({ method: 'POST', params: {}, headers: {}, body } as any, res); + return res; +} + +let consoleError: ReturnType; +beforeEach(() => { + consoleError = vi.spyOn(console, 'error').mockImplementation(() => {}); +}); +afterEach(() => consoleError.mockRestore()); + +// ───────────────────────────────────────────────────────────────────────────── + +describe('[#5669] a bogus `where` field answers 400 INVALID_FIELD, end to end', () => { + it('names the field and the object — and is not a 500', async () => { + const route = buildRoute(async () => realAnalytics()); + const res = await post(route, { + dataset, + selection: { measures: ['account_count'], runtimeFilter: { bogus_col: 'x' } }, + }); + + expect(res.statusCode).toBe(400); + expect(res.body.code).toBe('INVALID_FIELD'); + // The defect, asserted as the defect rather than as the fix. + expect(res.statusCode).not.toBe(500); + expect(res.body.code).not.toBe('ANALYTICS_QUERY_FAILED'); + expect(String(res.body.message)).toMatch(/Filter member 'bogus_col' in 'where'/); + expect(String(res.body.message)).toMatch(/object 'crm_account' does not have/); + }); + + it('carries no generated SQL — because the statement was never built', async () => { + const route = buildRoute(async () => realAnalytics()); + const res = await post(route, { + dataset, + selection: { measures: ['account_count'], runtimeFilter: { bogus_col: 'x' } }, + }); + + // BOTH halves of the one claim: the answer is the field-naming 400, and that + // answer carries no statement. See the header for why the second alone is + // not evidence. + expect(res.statusCode).toBe(400); + expect(res.body.code).toBe('INVALID_FIELD'); + const body = JSON.stringify(res.body); + expect(body).not.toMatch(/SELECT/i); + expect(body).not.toMatch(/FROM \\"/); + expect(body).not.toMatch(/no such column/); + }); + + it('answers the same way for a DATASET-declared filter over a dropped column', async () => { + // The authored half of the same mistake: a dataset whose object dropped a + // column its own declared `filter` still names. + const route = buildRoute(async () => realAnalytics()); + const res = await post(route, { + dataset: { ...dataset, filter: { dropped_column: 'x' } }, + selection: { measures: ['account_count'] }, + }); + + expect(res.statusCode).toBe(400); + expect(res.body.code).toBe('INVALID_FIELD'); + expect(String(res.body.message)).toMatch(/constrains field 'dropped_column'/); + }); + + it('a POSITIVE control: the same wiring with real fields → 200 with rows', async () => { + // Without this the cases above could pass for any reason that makes the route + // 400, including a pipeline that never reaches the gate. It also pins the + // contract the gate must not kill: `phone` is a REAL column this dataset + // never declared as a dimension, and filtering on it still works. + const route = buildRoute(async () => realAnalytics()); + + const declared = await post(route, { + dataset, + selection: { measures: ['account_count'], dimensions: ['industry'], runtimeFilter: { industry: 'tech' } }, + }); + expect(declared.statusCode).toBe(200); + expect(declared.body.rows).toEqual([{ industry: 'tech', account_count: 3 }]); + + const undeclaredButReal = await post(route, { + dataset, + selection: { measures: ['account_count'], runtimeFilter: { phone: '555' } }, + }); + expect(undeclaredButReal.statusCode).toBe(200); + }); + + it('does not disturb the INVALID_FILTER family #5352 / #5367 own', async () => { + // A structurally-invalid filter is a DIFFERENT verdict from a + // field-that-does-not-exist, and both must keep their own code: the gate + // stands down on a `where` the normalizer refuses, so `INVALID_FILTER` still + // comes from where it always did. + const route = buildRoute(async () => realAnalytics()); + const res = await post(route, { + dataset, + selection: { measures: ['account_count'], runtimeFilter: { industry: { $sortOf: 'tech' } } }, + }); + + expect(res.statusCode).toBe(400); + expect(res.body.code).toBe('INVALID_FILTER'); + expect(String(res.body.message)).toMatch(/\$sortOf/); + }); +}); diff --git a/packages/services/service-analytics/src/__tests__/where-source-field-gate.test.ts b/packages/services/service-analytics/src/__tests__/where-source-field-gate.test.ts new file mode 100644 index 0000000000..291ee34926 --- /dev/null +++ b/packages/services/service-analytics/src/__tests__/where-source-field-gate.test.ts @@ -0,0 +1,739 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #5669 — the `where` SOURCE-FIELD gate, the third and last param of one defect. + * + * #4437 gave MEASURES a `400 INVALID_FIELD` naming the field; #5520 / PR #5667 + * gave `dimensions`/`timeDimensions` the same answer. The FILTER face — the + * request key that most often carries a hand-typed field name — had no gate at + * all. Measured on this harness against `origin/main` before the fix: + * + * ``` + * POST /analytics/query {"cube":"crm_account","measures":["count"],"where":{"bogus_col":"x"}} + * generateSql → SELECT COUNT(*) AS "count" FROM "crm_account" WHERE bogus_col = $1 + * executeAggregate ← {filter: {"bogus_col":"x"}} ← no refusal anywhere + * + * where: [["bogus_col","=","x"]] (the #5334 array spelling) + * → the IDENTICAL statement and the IDENTICAL engine filter + * + * where: {$or: [{industry:"tech"}, {$and: [{bogus_nested:{$gt:1}}]}]} + * → WHERE (industry = $1 OR bogus_nested > $2) + * + * dataset face, runtimeFilter: {bogus_col: "x"} + * → SELECT COUNT(*) AS "account_count" FROM "crm_account" WHERE bogus_col = $1 + * ``` + * + * On a real SQLite driver each of those is `no such column: bogus_col` with an + * empty `code`/`status`, so the REST face falls to its 5xx backstop — a driver + * error class on the wire for a caller-shaped typo (ADR-0112), exactly the shape + * #4437 and #5520 were filed about. + * + * The blocks below pin the rejection, the dataset face, and — the half that + * keeps the gate from over-reaching — everything it must NOT do. + * + * ## Reverse verification, direction predicted BEFORE running + * + * Removing the three `assertWhereFields` calls from `ensureCube` turns RED every + * case that asserts on an error only this gate produces (or on the driver never + * being reached). Ordinary direction, no inversion: the rejection is new, and no + * `??`-chain order was touched — the one thing that could have inverted here is + * `resolveMemberSource`'s bag order, and `kind` is a NEW parameter, not a + * reordered one. **Predicted 15 red / 16 green, naming the 15; measured exactly + * that set.** + * + * Two departures from "block 1+2 red, block 3 green" are deliberate, and are + * named here rather than left for the next reader to trip over: + * + * - In block 1, "is answered about its MEASURE first" stays GREEN — the #4437 + * gate produces that rejection, and the case exists to pin the ORDER, not the + * `where` verdict. + * - In block 3, "answers a dotted member on the INFERENCE path exactly as the + * shipped dimension gate does" goes RED. It sits in the must-NOT-do block + * because it bounds the gate's reach, but what it pins is a VERDICT (and its + * agreement with #5520's), not a stand-down. Reading it as green-before/red- + * after is correct; reading the block heading as a promise of greenness is not. + * - In block 2, "the pre-fix driver error carried the statement" stays GREEN by + * design: it asserts the OLD behaviour on a cube the gate stands down for, the + * control proving this harness can still produce the leak the gate removes. + */ + +import { describe, it, expect, vi } from 'vitest'; +import type { Cube } from '@objectstack/spec/data'; +import type { Dataset } from '@objectstack/spec/ui'; +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; + +const ACCOUNT_FIELDS = ['id', 'name', 'phone', 'industry', 'annual_revenue', 'assessed_at']; + +/** + * A service over one object (`crm_account`) whose columns are known. + * + * `aggregated` records the object + options of every aggregate that actually ran + * and `sqls` every statement the native path built, so a test can assert the + * rejected filter never reached the driver — which is also what proves there was + * no generated SQL for the dataset face to echo. + * + * `native: true` selects the NativeSQLStrategy path and makes the driver double + * fail the way the real one did: knex prefixes the offending statement to its + * message (` - `), which is how the SQL got into the caller's body. + */ +function makeService( + opts: { cubes?: Cube[]; wireProbe?: boolean; fields?: string[]; native?: boolean } = {}, +) { + const aggregated: string[] = []; + const filters: unknown[] = []; + const sqls: string[] = []; + const service = new AnalyticsService({ + logger: silentLogger, + ...(opts.cubes ? { cubes: opts.cubes } : {}), + queryCapabilities: () => ({ + nativeSql: !!opts.native, + objectqlAggregate: !opts.native, + inMemory: false, + }), + executeAggregate: async (objectName: string, options: unknown) => { + aggregated.push(objectName); + filters.push((options as { filter?: unknown } | undefined)?.filter); + return [{ account_count: 1, count: 1 }]; + }, + executeRawSql: async (objectName: string, sql: string) => { + aggregated.push(objectName); + sqls.push(sql); + const bogus = /\b(bogus_col|bogus_nested|dropped_column)\b/.exec(sql)?.[1]; + if (bogus) throw new Error(`${sql} - no such column: ${bogus}`); + return [{ account_count: 1, count: 1 }]; + }, + isRegisteredObject: (n: string) => n === 'crm_account', + ...(opts.wireProbe === false + ? {} + : { + getObjectFieldNames: (n: string) => + n === 'crm_account' ? (opts.fields ?? ACCOUNT_FIELDS) : undefined, + }), + }); + return { service, aggregated, filters, sqls }; +} + +/** The error a call rejected with, typed — and 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 call to reject, but it resolved'); +} + +/** How a call settled: the error it rejected with, or `{}` when it resolved. */ +async function settle(call: Promise): Promise<{ code?: string; message?: string }> { + try { + await call; + return {}; + } catch (e) { + return e as { code?: string; message?: string }; + } +} + +/** The envelope the measure (#4437) and dimension (#5520) gates already produce. */ +const INVALID_FIELD = { + code: 'INVALID_FIELD', + status: 400, + object: 'crm_account', + param: 'where', +}; + +/** The dataset behind the dataset-face repro — one declared dimension, one measure. */ +const ACCOUNT_METRICS: Dataset = { + name: 'account_metrics', + label: 'Account metrics', + object: 'crm_account', + dimensions: [{ name: 'industry', field: 'industry', type: 'string' }], + measures: [{ name: 'account_count', aggregate: 'count' }], +} as Dataset; + +describe('#5669 — the gate: a `where` over a missing field is a 400, not a driver 500', () => { + it('refuses the bare-cube path and names the field', async () => { + const { service, aggregated } = makeService(); + + await expect( + service.query({ + cube: 'crm_account', + measures: ['count'], + where: { bogus_col: 'x' }, + } as any), + ).rejects.toMatchObject({ ...INVALID_FIELD, field: 'bogus_col', member: 'bogus_col' }); + + // The whole point: the typo never became a WHERE column. + expect(aggregated).toEqual([]); + }); + + it('says what the caller can act on — the field, and that undeclared real fields are fine', async () => { + const { service } = makeService(); + + const err = await rejection( + service.query({ cube: 'crm_account', measures: ['count'], where: { bogus_col: 'x' } } as any), + ); + + expect(err.message).toMatch(/Filter member 'bogus_col' in 'where'/); + expect(err.message).toMatch(/constrains field 'bogus_col'/); + expect(err.message).toMatch(/object 'crm_account' does not have/); + // The known-fields list is what turns a typo into a one-look fix. + expect(err.message).toMatch(/known fields: annual_revenue, assessed_at, id, industry, name, phone\./); + // And the message states the contract the third block pins, so a caller is + // not left thinking a field must be declared as a dimension to be filtered. + expect(err.message).toMatch(/may also be filtered on without the cube declaring it/); + }); + + it('does not offer the caller their own typo back as a valid filter member', async () => { + // `inferCubeFromQuery` mints the `where`'s top-level keys into + // `cube.dimensions`, so echoing that bag verbatim would suggest + // `bogus_col` — the one alternative guaranteed not to work. + const { service } = makeService(); + + const err = await rejection( + service.query({ + cube: 'crm_account', + measures: ['count'], + dimensions: ['industry'], + where: { bogus_col: 'x' }, + } as any), + ); + + expect(err.message).toMatch(/Valid filter members: industry\./); + expect(err.message).not.toMatch(/Valid filter members:[^.]*bogus_col/); + }); + + it('reports `(none)` rather than an empty list when nothing survives', async () => { + const { service } = makeService(); + + const err = await rejection( + service.query({ cube: 'crm_account', measures: ['count'], where: { bogus_col: 'x' } } as any), + ); + + expect(err.message).toMatch(/Valid filter members: \(none\)\./); + }); + + it('finds a member nested under $or / $and, because a predicate under a disjunction still names a column', async () => { + // Measured before the fix: `WHERE (industry = $1 OR bogus_nested > $2)`. + // `collectFilterLeaves` discards structure on purpose — whether a + // predicate sits under an `$or` changes nothing about column existence. + const { service, aggregated } = makeService(); + + await expect( + service.query({ + cube: 'crm_account', + measures: ['count'], + where: { $or: [{ industry: 'tech' }, { $and: [{ bogus_nested: { $gt: 1 } }] }] }, + } as any), + ).rejects.toMatchObject({ ...INVALID_FIELD, field: 'bogus_nested', member: 'bogus_nested' }); + + expect(aggregated).toEqual([]); + }); + + it('finds a member under $not, whose operand the normalizer REWRITES before compiling', async () => { + // #5146's null-safe rewrite rebuilds the `$not` operand; the guard it adds + // rides the same member, so reading the tree after the rewrite still sees + // the caller's own field name. + const { service, aggregated } = makeService(); + + await expect( + service.query({ + cube: 'crm_account', + measures: ['count'], + where: { $not: { bogus_col: 'x' } }, + } as any), + ).rejects.toMatchObject({ ...INVALID_FIELD, field: 'bogus_col' }); + + expect(aggregated).toEqual([]); + }); + + it('finds a member behind $between, which LOWERS to two bounds', async () => { + const { service, aggregated } = makeService(); + + await expect( + service.query({ + cube: 'crm_account', + measures: ['count'], + where: { bogus_col: { $between: [1, 5] } }, + } as any), + ).rejects.toMatchObject({ ...INVALID_FIELD, field: 'bogus_col' }); + + expect(aggregated).toEqual([]); + }); + + it('gates the #5334 ARRAY spelling too — it compiles to the identical predicate', async () => { + // Measured on `origin/main`: `where: [['bogus_col','=','x']]` and + // `where: {bogus_col:'x'}` both produced + // `WHERE bogus_col = $1` and handed `executeAggregate` the same + // `{bogus_col: 'x'}`. `inferCubeFromQuery` skipping array `where` (#5353) + // is about the ad-hoc cube's dimension VOCABULARY, not about which + // columns reach the driver — so gating one spelling and not the other + // would answer ONE mistake two ways. + const { service, aggregated } = makeService(); + + await expect( + service.query({ + cube: 'crm_account', + measures: ['count'], + where: [['bogus_col', '=', 'x']], + } as any), + ).rejects.toMatchObject({ ...INVALID_FIELD, field: 'bogus_col' }); + + expect(aggregated).toEqual([]); + }); + + it('does not poison the registry with the rejected cube', async () => { + // Same rule the #3867 inference gate and the #4437 / #5520 gates keep: a + // rejected query must leave no trace, or the retry finds a "registered" + // cube and sails straight into SQL. + const { service, aggregated } = makeService(); + + await expect( + service.query({ cube: 'crm_account', measures: ['count'], where: { bogus_col: 'x' } } as any), + ).rejects.toThrow(); + expect(service.cubeRegistry.get('crm_account')).toBeUndefined(); + + await expect( + service.query({ cube: 'crm_account', measures: ['count'], where: { bogus_col: 'x' } } as any), + ).rejects.toMatchObject(INVALID_FIELD); + expect(aggregated).toEqual([]); + }); + + it('gates generateSql too, not just query', async () => { + // `/analytics/sql` runs the same `ensureCube`; leaving it ungated would + // hand back SQL filtering on a column that does not exist — which is + // literally the string the premise probe captured. + const { service } = makeService(); + + await expect( + service.generateSql({ + cube: 'crm_account', + measures: ['count'], + where: { bogus_col: 'x' }, + } as any), + ).rejects.toMatchObject(INVALID_FIELD); + }); + + it('validates an AUTHORED cube whose declared dimension lost its column', async () => { + // An authored cube is not second-guessed about WHICH table it reads + // (#3867), but filtering on a dimension it declares over a dropped column + // is the same caller-visible 500 — and here the suggestion list is real. + const authored: Cube = { + name: 'account_cube', + title: 'Accounts', + sql: 'crm_account', + measures: { count: { name: 'count', label: 'Count', type: 'count', sql: '*' } }, + dimensions: { + industry: { name: 'industry', label: 'Industry', type: 'string', sql: 'industry' }, + legacy: { name: 'legacy', label: 'Legacy', type: 'string', sql: 'dropped_column' }, + }, + public: false, + }; + const { service, aggregated } = makeService({ cubes: [authored] }); + + // The member is what the caller wrote; the field is the column it resolves + // to — the two differ here, which is why both are reported. + await expect( + service.query({ cube: 'account_cube', measures: ['count'], where: { legacy: 'x' } } as any), + ).rejects.toMatchObject({ + ...INVALID_FIELD, + field: 'dropped_column', + member: 'legacy', + }); + expect(aggregated).toEqual([]); + + // Its healthy sibling still filters, and is what the rejection suggests. + await service.query({ cube: 'account_cube', measures: ['count'], where: { industry: 'tech' } } as any); + expect(aggregated).toEqual(['crm_account']); + }); + + it('is answered about its MEASURE first when a query gets both wrong', async () => { + // Request-key order (measures → dimensions → where): one rejection at a + // time, naming a real mistake either way. + const { service } = makeService(); + + await expect( + service.query({ + cube: 'crm_account', + measures: ['ghost_sum'], + where: { bogus_col: 'x' }, + } as any), + ).rejects.toMatchObject({ code: 'INVALID_FIELD', param: 'measures', field: 'ghost' }); + }); +}); + +describe('#5669 — the dataset face: refused before SQL exists, so nothing can echo it', () => { + it('refuses a bogus `runtimeFilter` field with the same envelope', async () => { + const { service, aggregated, sqls } = makeService({ native: true }); + + await expect( + service.queryDataset(ACCOUNT_METRICS, { + measures: ['account_count'], + runtimeFilter: { bogus_col: 'x' }, + } as any), + ).rejects.toMatchObject({ ...INVALID_FIELD, field: 'bogus_col', member: 'bogus_col' }); + + // No statement was ever built, so there is no SQL for the REST envelope to + // echo. (`rest-server`'s 500 branch is sanitised as well — see + // `analytics-dataset-where-gate.test.ts` — but the driver error that + // carried the statement no longer happens.) + expect(sqls).toEqual([]); + expect(aggregated).toEqual([]); + }); + + it('refuses a DATASET-declared filter over a dropped column', async () => { + // Measured before the fix: `WHERE dropped_column = $1`. A dataset whose + // object dropped a column its declared filter still names is the authored + // half of the same mistake. + const stale: Dataset = { ...ACCOUNT_METRICS, filter: { dropped_column: 'x' } } as Dataset; + const { service, sqls } = makeService({ native: true }); + + await expect( + service.queryDataset(stale, { measures: ['account_count'] } as any), + ).rejects.toMatchObject({ ...INVALID_FIELD, field: 'dropped_column' }); + + expect(sqls).toEqual([]); + }); + + it('the rejection message itself contains no generated SQL', async () => { + const { service } = makeService({ native: true }); + + const err = await rejection( + service.queryDataset(ACCOUNT_METRICS, { + measures: ['account_count'], + runtimeFilter: { bogus_col: 'x' }, + } as any), + ); + + // It names the object and the field — a caller-shaped fact — and neither a + // SELECT list, a quoted table name, a bound parameter, nor the driver's + // own words. Note it DOES contain the word `where`: that is the REQUEST + // KEY the caller must fix, which is caller vocabulary, not a SQL clause — + // so the assertion is on the clause shape, not on the word. + expect(err.message).toContain("object 'crm_account' does not have"); + expect(err.message).not.toMatch(/SELECT/i); + expect(err.message).not.toMatch(/FROM "/); + expect(err.message).not.toMatch(/WHERE\s+\S+\s*[=<>]/i); + expect(err.message).not.toMatch(/\$\d/); + expect(err.message).not.toMatch(/no such column/); + }); + + it('is the whole reason the leak was reachable: the pre-fix driver error carried the statement', async () => { + // The control: with a cube the gate stands down for, the SAME harness + // still produces the knex-shaped ` - ` message that used to + // reach callers verbatim. Green before and after the change. + const derived: Cube = { + name: 'derived_cube', + title: 'Derived', + sql: 'SELECT * FROM crm_account', + measures: { count: { name: 'count', label: 'Count', type: 'count', sql: '*' } }, + dimensions: {}, + public: false, + }; + const { service } = makeService({ cubes: [derived], native: true }); + + const err = await rejection( + service.query({ cube: 'derived_cube', measures: ['count'], where: { bogus_col: 'x' } } as any), + ); + + expect(err.code).toBeUndefined(); + expect(err.message).toMatch(/^SELECT /); + expect(err.message).toMatch(/no such column: bogus_col/); + }); +}); + +describe('#5669 — what the gate must NOT do', () => { + it('lets an UNDECLARED but REAL field be filtered on', async () => { + // `where: {phone: 'x'}` on a cube that declares no `phone` dimension + // returned 200 before this change — the filter twin of the contract #5520 + // preserved for dimensions. The gate asks "does the OBJECT have this + // field", never "did the cube declare it". + const { service, aggregated, filters } = makeService(); + + const result = await service.query({ + cube: 'crm_account', + measures: ['count'], + where: { phone: '555' }, + } as any); + + expect(result.rows).toHaveLength(1); + expect(aggregated).toEqual(['crm_account']); + expect(filters).toEqual([{ phone: '555' }]); + }); + + it('lets a real field be filtered on through the dataset face', async () => { + const { service, sqls } = makeService({ native: true }); + + await service.queryDataset(ACCOUNT_METRICS, { + measures: ['account_count'], + runtimeFilter: { industry: 'tech' }, + } as any); + + expect(sqls).toEqual([ + 'SELECT COUNT(*) AS "account_count" FROM "crm_account" WHERE industry = $1', + ]); + }); + + it('admits the engine-assigned columns the data path admits', async () => { + // `id`/`created_at`/`updated_at` are engine-assigned rather than declared + // (`resolveQueryFields` on the data path admits them unconditionally); a + // gate stricter than the engine it guards would reject working queries. + const { service, aggregated } = makeService({ fields: ['name'] }); + + await service.query({ + cube: 'crm_account', + measures: ['count'], + where: { $and: [{ id: 'a1' }, { created_at: { $gte: '2026-01-01' } }, { updated_at: { $null: false } }] }, + } as any); + + expect(aggregated).toEqual(['crm_account']); + }); + + it('follows a declared dimension to its real column, not to its own name', async () => { + // Dimension `assessed` → column `assessed_at`. Checking the member name + // would reject a perfectly good authored cube. + const authored: Cube = { + name: 'renamed_cube', + title: 'Renamed', + sql: 'crm_account', + measures: { count: { name: 'count', label: 'Count', type: 'count', sql: '*' } }, + dimensions: { + assessed: { name: 'assessed', label: 'Assessed', type: 'time', sql: 'assessed_at' }, + }, + public: false, + }; + const { service, aggregated, filters } = makeService({ cubes: [authored] }); + + await service.query({ cube: 'renamed_cube', measures: ['count'], where: { assessed: '2026-01-01' } } as any); + + expect(aggregated).toEqual(['crm_account']); + // Measured on `origin/main`, and unchanged: the engine gets the COLUMN. + expect(filters).toEqual([{ assessed_at: '2026-01-01' }]); + }); + + it('follows a declared MEASURE to its column too, because a filter member resolves through both bags', async () => { + // The false positive a dimensions-only lookup would have created. Measured + // on `origin/main`: a cube declaring `measures.revenue = {sql: + // 'annual_revenue'}` answers `where: {revenue: {$gt: 100}}` as + // `annual_revenue > $1` on BOTH strategies — `resolveFieldSql` and + // `resolveFieldName(…, 'any')` both fall through to `cube.measures`. A + // gate that read dimensions only would have called `revenue` a missing + // column and 400'd a working query. + const authored: Cube = { + name: 'measure_cube', + title: 'Measures', + sql: 'crm_account', + measures: { + count: { name: 'count', label: 'Count', type: 'count', sql: '*' }, + revenue: { name: 'revenue', label: 'Revenue', type: 'sum', sql: 'annual_revenue' }, + }, + dimensions: {}, + public: false, + }; + const { service, aggregated, filters } = makeService({ cubes: [authored] }); + + await service.query({ cube: 'measure_cube', measures: ['count'], where: { revenue: { $gt: 100 } } } as any); + + expect(aggregated).toEqual(['crm_account']); + expect(filters).toEqual([{ annual_revenue: { $gt: 100 } }]); + }); + + it('accepts the canonical `.` qualifier', async () => { + const { service, aggregated } = makeService(); + + await service.query({ + cube: 'crm_account', + measures: ['count'], + where: { 'crm_account.industry': 'tech' }, + } as any); + + expect(aggregated).toEqual(['crm_account']); + }); + + it('leaves a cube whose `sql` is an expression alone — no field list to check', async () => { + const derived: Cube = { + name: 'derived_cube', + title: 'Derived', + sql: 'SELECT * FROM crm_account WHERE active = 1', + measures: { count: { name: 'count', label: 'Count', type: 'count', sql: '*' } }, + dimensions: {}, + public: false, + }; + const { service } = makeService({ cubes: [derived] }); + + await expect( + service.query({ cube: 'derived_cube', measures: ['count'], where: { anything: 1 } } as any), + ).resolves.toBeTruthy(); + }); + + it('leaves a dotted relation filter on an AUTHORED cube to the layers that own it', async () => { + // `owner.region` resolves through a JOIN this gate cannot see — `region` + // may well be a column of the RELATED object, and reporting it as missing + // from `crm_account` would be a lie. Whether the query can run is the + // strategy's call and the join allowlist's (ADR-0021 D-C); either way the + // answer must not be this gate's INVALID_FIELD. Measured on both + // strategies: NativeSQL joins it + // (`LEFT JOIN "owner" … WHERE "owner"."region" = $1`), ObjectQL declines it + // with its own cross-object message. + const joined: Cube = { + name: 'joined_cube', + title: 'Joined', + sql: 'crm_account', + measures: { count: { name: 'count', label: 'Count', type: 'count', sql: '*' } }, + dimensions: {}, + public: false, + }; + const { service } = makeService({ cubes: [joined] }); + + const settled = await settle( + service.query({ cube: 'joined_cube', measures: ['count'], where: { 'owner.region': 'NA' } } as any), + ); + + expect(settled.code).not.toBe('INVALID_FIELD'); + }); + + it('reads a NESTED relation filter as the same dotted member the strategies do', async () => { + // `{owner: {region: 'NA'}}` is the object spelling of the same traversal — + // `fieldLeaves` flattens it to the member `owner.region`, so reading the + // TREE (rather than the raw top-level keys) is what keeps the two + // spellings judged alike. A gate over raw keys would have judged `owner`, + // a field the object does not have, and 400'd a legal relation filter; + // measured here, both spellings reach ObjectQL's identical cross-object + // decline instead. + const joined: Cube = { + name: 'joined_cube', + title: 'Joined', + sql: 'crm_account', + measures: { count: { name: 'count', label: 'Count', type: 'count', sql: '*' } }, + dimensions: {}, + public: false, + }; + const { service } = makeService({ cubes: [joined] }); + + const settled = await settle( + service.query({ cube: 'joined_cube', measures: ['count'], where: { owner: { region: 'NA' } } } as any), + ); + + expect(settled.code).not.toBe('INVALID_FIELD'); + expect(settled.message).toMatch(/cross-object filter \("owner\.region"\)/); + }); + + it('answers a dotted member on the INFERENCE path exactly as the shipped dimension gate does', async () => { + // Not a stand-down, and reported rather than papered over. On the + // auto-inference path `inferCubeFromQuery` mints `stripPrefix(member)` — + // `region` — as a dimension, and `lookupMember`'s legacy second-segment + // lookup then finds it, so the member resolves to BASE column `region`. + // That is what actually reaches the engine there (measured on + // `origin/main`: `executeAggregate` received `{region: 'NA'}`, because the + // ad-hoc cube is single-table and `planCrossObject` never sees a dotted + // name to classify), so naming `region` as missing is the honest answer. + // + // The point of pinning it: `dimensions: ['owner.region']` on the same path + // ALREADY answers the identical `INVALID_FIELD`/`region` on `main` (#5520), + // so this is one behaviour shared through `resolveMemberSource`, not a new + // over-reach invented by the `where` gate. If that reading is ever judged + // wrong it must change for both keys at once — which is exactly why the + // resolution lives in one function. + const { service } = makeService(); + + const viaWhere = await settle( + service.query({ cube: 'crm_account', measures: ['count'], where: { 'owner.region': 'NA' } } as any), + ); + const viaDimension = await settle( + service.query({ cube: 'crm_account', measures: ['count'], dimensions: ['owner.region'] } as any), + ); + + expect(viaWhere.code).toBe('INVALID_FIELD'); + expect(viaWhere.message).toMatch(/constrains field 'region'/); + // The #5520 half, unchanged by this PR — the two keys agree. + expect(viaDimension.code).toBe('INVALID_FIELD'); + expect(viaDimension.message).toMatch(/groups by field 'region'/); + }); + + it('leaves a declared dimension whose `sql` is an expression alone', async () => { + const computed: Cube = { + name: 'computed_cube', + title: 'Computed', + sql: 'crm_account', + measures: { count: { name: 'count', label: 'Count', type: 'count', sql: '*' } }, + dimensions: { + bucket: { + name: 'bucket', + label: 'Bucket', + type: 'string', + sql: "CASE WHEN annual_revenue > 0 THEN 'yes' ELSE 'no' END", + }, + }, + public: false, + }; + const { service } = makeService({ cubes: [computed] }); + + await expect( + service.query({ cube: 'computed_cube', measures: ['count'], where: { bucket: 'yes' } } as any), + ).resolves.toBeTruthy(); + }); + + it('stands down when no field probe is configured — nothing to consult', async () => { + // Same tiering as the #3867 registry gate and the #4437 / #5520 gates: with + // no source of truth the question cannot be answered, and failing closed + // would break every embedding that runs analytics without a data engine. + const { service, aggregated } = makeService({ wireProbe: false }); + + await service.query({ cube: 'crm_account', measures: ['count'], where: { bogus_col: 'x' } } as any); + + expect(aggregated).toEqual(['crm_account']); + }); + + it('does not touch a query with no `where` at all', async () => { + const { service, aggregated } = makeService(); + + await service.query({ cube: 'crm_account', measures: ['count'] } as any); + + expect(aggregated).toEqual(['crm_account']); + }); + + it('leaves the boolean-identity `where` shapes exactly as #5322 / #5325 ruled them', async () => { + // `{}`, `[]`, `{$and: []}`, `{$or: []}` name no column at all, so a field + // gate has nothing to say about them and must not invent an answer. All + // four resolved before this change; the `$or: []` one is FALSE (zero rows) + // and still runs. + const { service } = makeService(); + + for (const where of [{}, [], { $and: [] }, { $or: [] }]) { + const settled = await settle( + service.query({ cube: 'crm_account', measures: ['count'], where } as any), + ); + expect(settled.code).toBeUndefined(); + } + }); + + it('does not move the INVALID_FILTER refusals this gate cannot read', async () => { + // #5352/#5367's geography, not this gate's: an unknown operator, a + // zero-operator field constraint, an unlowerable infix array and an + // unknown top-level combinator each still answer `INVALID_FILTER`/400 from + // where they already did. The gate stands down on a tree it cannot read + // rather than pulling those refusals forward into `ensureCube` — which + // would also newly refuse them on the draft-preview path, whose + // `matchesWhere` never consults the normalizer. + const { service } = makeService(); + + for (const where of [ + { industry: { $sounds_like: 'x' } }, + { industry: {} }, + [{ industry: 'tech' }, 'or', { industry: 'saas' }], + { $weird: 1 }, + ]) { + const settled = await settle( + service.query({ cube: 'crm_account', measures: ['count'], where } as any), + ); + expect(settled.code).toBe('INVALID_FILTER'); + } + }); +}); diff --git a/packages/services/service-analytics/src/analytics-service.ts b/packages/services/service-analytics/src/analytics-service.ts index 6a10f15533..90cee40a22 100644 --- a/packages/services/service-analytics/src/analytics-service.ts +++ b/packages/services/service-analytics/src/analytics-service.ts @@ -16,6 +16,10 @@ import { CubeRegistry } from './cube-registry.js'; import type { AnalyticsStrategy, AnalyticsDriverCapabilities, StrategyContext } from './strategies/types.js'; import { NativeSQLStrategy } from './strategies/native-sql-strategy.js'; 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 { compileDataset, type CompiledDataset, type RelationshipResolver } from './dataset-compiler.js'; import { DatasetExecutor, resolveDimensionGranularity, type DateGranularityValue } from './dataset-executor.js'; import { @@ -138,6 +142,87 @@ function missingSourceRelation(err: unknown): string | undefined { */ const BARE_IDENTIFIER = /^[a-z_][a-z0-9_]*$/i; +/** + * The `cube.dimensions` / `cube.measures` KEY a member resolves to, mirroring + * the strategies' own `lookupMember` — including its deliberate LAST case: a + * dotted member that matches no declared key is a synthetic relation traversal + * handed to the JOIN machinery, which the source-field gates must not judge + * (hence `undefined`, read as "nothing to check" rather than "undeclared bare + * column"). + * + * `kind` is which bags to consult, and it is NOT cosmetic — it is the difference + * between the two callers' real resolution rules: + * + * - `'dimension'` — {@link AnalyticsService.assertDimensionFields}, matching + * `NativeSQLStrategy.resolveDimensionSql` / `ObjectQLStrategy.resolveFieldName + * (…, 'dimension')`, which look in `cube.dimensions` only. + * - `'any'` — {@link AnalyticsService.assertWhereFields}, matching the FILTER + * member resolution in `NativeSQLStrategy.resolveFieldSql` and + * `ObjectQLStrategy.resolveFieldName(…, 'any')`, which fall through to + * `cube.measures`. Consulting dimensions only would have made the where gate + * reject a query that works on both strategies today: a cube declaring + * `measures.revenue = {sql: 'annual_revenue'}` answers + * `where: {revenue: {$gt: 100}}` as `annual_revenue > ?`, and a + * dimensions-only lookup would have called `revenue` a missing column. + * + * Extracted from #5520's gate so #5669's second caller reads the tree the same + * way — two open-coded copies of `lookupMember` in one file is exactly how + * "what the gate sees" and "what reaches SQL" drift apart. + */ +function declaredMemberEntry( + cube: Cube, + member: string, + kind: 'dimension' | 'any', +): { key: string; sql?: unknown } | undefined { + const bags: Array> = + kind === 'dimension' + ? [cube.dimensions as Record] + : [ + cube.dimensions as Record, + cube.measures as Record, + ]; + // `key` is spread LAST in every arm: it is the bag key this member RESOLVED + // to, and a `key` property on the cube entry itself must not shadow it. + for (const bag of bags) { + if (bag[member]) return { ...bag[member], key: member }; + if (member.includes('.')) { + const [first, ...rest] = member.split('.'); + const tail = rest.join('.'); + if (first === cube.name && bag[tail]) return { ...bag[tail], key: tail }; + if (bag[tail]) return { ...bag[tail], key: tail }; + const flat = member.replace(/\./g, '_'); + if (bag[flat]) return { ...bag[flat], key: flat }; + } + } + return undefined; +} + +/** + * The cube KEY a member resolves to (for the rejection's suggestion list) and + * the bare COLUMN it compiles to — `source: null` meaning "nothing this gate can + * check", which covers every deliberate stand-down at member level: an + * expression `sql` (`CASE WHEN …`, `*`), and a dotted relation traversal whose + * join target the gate cannot see. + * + * Shared by the dimension gate (#5520) and the `where` gate (#5669) — see + * {@link declaredMemberEntry} for why `kind` differs between them. + */ +function resolveMemberSource( + cube: Cube, + member: string, + kind: 'dimension' | 'any', +): { key: string; source: string | null } { + const entry = declaredMemberEntry(cube, member, kind); + if (entry) { + const source = typeof entry.sql === 'string' ? entry.sql.trim() : ''; + return { key: entry.key, source: source && BARE_IDENTIFIER.test(source) ? source : null }; + } + // Undeclared. A dotted spelling is the relation traversal above; a bare one IS + // the column the strategies will emit. + if (member.includes('.')) return { key: member, source: null }; + return { key: member, source: BARE_IDENTIFIER.test(member) ? member : null }; +} + /** * Configuration for AnalyticsService. */ @@ -292,6 +377,12 @@ export interface AnalyticsServiceConfig { * ['bogus_dim']` — still reached the driver as a `GROUP BY` column and came * back as the same 500. One probe, one answer, both member kinds. * + * [#5669] …and for the `where` members ({@link AnalyticsService.assertWhereFields}), + * the third and last request key that carries a field name. `where: + * {bogus_col: 'x'}` compiled straight into `WHERE bogus_col = $1` for exactly + * as long as #4437 and #5520 had each closed only their own key. One probe now + * answers for all three. + * * Same tiering as {@link isRegisteredObject}: absence means "skip the check" * (registry-less hosts, engine doubles, external datasources whose columns * are not mirrored locally). The production bridge in `plugin.ts` wires it @@ -995,12 +1086,18 @@ export class AnalyticsService implements IAnalyticsService { * widget translators), inject suffix-inferred Metric entries so the * strategies pick the right aggregation function and field. * - * It is also where the two SOURCE-FIELD gates run, on every path out of this + * It is also where the three SOURCE-FIELD gates run, on every path out of this * method and always BEFORE the (possibly augmented) cube is registered: - * {@link assertMeasureFields} (#4437) and {@link assertDimensionFields} - * (#5520). Both answer the same question — does the object actually have the - * column this member resolves to — and both must answer it here, because from - * the strategy onwards the answer is the driver's `no such column`. + * {@link assertMeasureFields} (#4437), {@link assertDimensionFields} (#5520) + * and {@link assertWhereFields} (#5669) — one per request key that can carry a + * field name. All three answer the same question — does the object actually + * have the column this member resolves to — and all three must answer it here, + * because from the strategy onwards the answer is the driver's `no such + * column`. + * + * They run in request-key order (measures → dimensions/timeDimensions → + * where), so a query that gets several wrong is answered about one at a time, + * naming a real mistake either way. */ private ensureCube(query: AnalyticsQuery): void { const name = query.cube!; @@ -1025,6 +1122,11 @@ export class AnalyticsService implements IAnalyticsService { // spelling is in there — which is why the suggestion list is computed by // subtraction inside the gate rather than echoed verbatim. this.assertDimensionFields(query, cube, Object.keys(cube.dimensions)); + // [#5669] …and the `where`'s, third and last of the three request keys that + // carry a field name. Its members are read from the filter TREE, not from + // `cube.dimensions` — which on this path was minted from this very query, + // bogus spelling included. + this.assertWhereFields(query, cube, Object.keys(cube.dimensions)); this.cubeRegistry.register(cube); // A scalar query — only measures, no grouping (no `dimensions`/ // `timeDimensions`) — is the first-class "metric over an object" path @@ -1069,6 +1171,11 @@ export class AnalyticsService implements IAnalyticsService { // authored/compiled list IS the vocabulary a caller may name — and the one // the rejection suggests. this.assertDimensionFields(query, augmented, Object.keys(cube.dimensions)); + // [#5669] The `where` gate resolves a filter member through dimensions AND + // measures (that is what the strategies do for a filter member), so it is + // handed the AUGMENTED cube — a caller filtering on a suffix-inferred + // measure must be judged against the same bag the strategy will read. + this.assertWhereFields(query, augmented, Object.keys(cube.dimensions)); this.cubeRegistry.register(augmented); this.logger.debug( `[Analytics] Augmented cube "${name}" with inferred measures: ${Object.keys(extraMeasures).join(',')}`, @@ -1078,6 +1185,7 @@ export class AnalyticsService implements IAnalyticsService { // authored cube can declare a measure over a field the object dropped. this.assertMeasureFields(query, cube, Object.keys(cube.measures)); this.assertDimensionFields(query, cube, Object.keys(cube.dimensions)); + this.assertWhereFields(query, cube, Object.keys(cube.dimensions)); } } @@ -1215,9 +1323,9 @@ export class AnalyticsService implements IAnalyticsService { * - `id` / `created_at` / `updated_at` are admitted unconditionally, matching * the data path's `resolveQueryFields`. * - * Runs after the measure gate on each `ensureCube` path, so a query that gets - * both wrong is answered about its measure first — one rejection at a time, - * naming a real mistake either way. + * Runs after the measure gate and before the `where` gate on each `ensureCube` + * path, so a query that gets several wrong is answered about its measure + * first — one rejection at a time, naming a real mistake either way. */ private assertDimensionFields(query: AnalyticsQuery, cube: Cube, declaredDimensions: string[]): void { const probe = this.getObjectFieldNames; @@ -1235,43 +1343,17 @@ export class AnalyticsService implements IAnalyticsService { if (!fieldNames || fieldNames.length === 0) return; const known = new Set([...fieldNames, 'id', 'created_at', 'updated_at']); - /** - * The `cube.dimensions` KEY a member resolves to, mirroring the strategies' - * `lookupMember` — including its deliberate LAST case: a dotted member that - * matches no declared key is a synthetic relation traversal handed to the - * JOIN machinery, which this gate must not judge (hence `undefined`, read as - * "nothing to check" rather than "undeclared bare column"). - */ - const declaredKeyOf = (member: string): string | undefined => { - const bag = cube.dimensions as Record; - if (bag[member]) return member; - if (member.includes('.')) { - const [first, ...rest] = member.split('.'); - const tail = rest.join('.'); - if (first === cube.name && bag[tail]) return tail; - if (bag[tail]) return tail; - const flat = member.replace(/\./g, '_'); - if (bag[flat]) return flat; - } - return undefined; - }; - /** * The `cube.dimensions` key a member resolves to (for the suggestion list) * and the column it groups by — `source: null` meaning "nothing to check". + * + * [#5669] The body moved to the module-level {@link resolveMemberSource}, + * shared with the `where` gate. `'dimension'` keeps this call site's + * resolution exactly as #5520 wrote it: `cube.dimensions` only, matching + * `resolveDimensionSql` / `resolveFieldName(…, 'dimension')`. */ - const resolve = (member: string): { key: string; source: string | null } => { - const key = declaredKeyOf(member); - if (key !== undefined) { - const dim = (cube.dimensions as Record)[key]; - const source = typeof dim.sql === 'string' ? dim.sql.trim() : ''; - return { key, source: source && BARE_IDENTIFIER.test(source) ? source : null }; - } - // Undeclared. A dotted spelling is the relation traversal above; a bare one - // IS the column the strategies will emit. - if (member.includes('.')) return { key: member, source: null }; - return { key: member, source: BARE_IDENTIFIER.test(member) ? member : null }; - }; + const resolve = (member: string): { key: string; source: string | null } => + resolveMemberSource(cube, member, 'dimension'); // Two passes, for the reason the measure gate has two: on the auto-inference // path `cube.dimensions` was minted from this very query, so echoing its keys @@ -1308,6 +1390,128 @@ export class AnalyticsService implements IAnalyticsService { } } + /** + * [#5669] Reject a `where` member whose source field the backing object does + * not have, BEFORE the strategy compiles it into `WHERE`. + * + * The third and last param of one defect. #4437 gated `measures`, #5520 gated + * `dimensions`/`timeDimensions`, and the filter face — the request key that + * most often carries a hand-typed field name — had no gate at all: + * + * ``` + * POST /analytics/query {"cube":"crm_account","measures":["count"],"where":{"bogus_col":"x"}} + * → SELECT COUNT(*) AS "count" FROM "crm_account" WHERE bogus_col = $1 + * → 500 {"code":"SQLITE_ERROR","message":"Internal server error"} + * ``` + * + * Same envelope as its two siblings (`INVALID_FIELD`/400 + `field`/`object`/ + * `param`), because "the query names a field the object does not have" is ONE + * mistake whichever request key carried it, and the DATA route has answered it + * that way since #4315/#4254 (`resolveQueryFields`). + * + * # Where the field names come from: the SQL producer's own reader + * + * The members are collected through `normalizeAnalyticsFilterTree` + + * `collectFilterLeaves` — the SAME pair both strategies call to build the + * predicate. This is deliberate and is the whole reason this gate is not a + * second filter-tree walker: a hand-rolled walk would have to re-derive + * `$and`/`$or`/`$not` recursion, `$`-prefixed operator keys, `$between` + * lowering, the nested-relation dot flattening (`{owner: {region: 'NA'}}` → + * member `owner.region`) and the #5334 array lowering, and every divergence + * would show up as "the field the gate saw" not being "the column that reached + * SQL" — in either direction (a phantom rejection, or a hole). + * `collectFilterLeaves` discards structure, which is exactly right here: + * whether a predicate sits under an `$or` changes nothing about whether its + * column exists. (Its doc's warning — never rebuild a predicate from this list + * — does not apply; this gate builds nothing.) + * + * # Three stand-downs at query level, plus the per-member ones + * + * - No {@link AnalyticsServiceConfig.getObjectFieldNames}, cube `sql` that is + * not a bare object name, or a probe that cannot answer for the object — the + * same three tiers as the measure and dimension gates, for the same reasons. + * - A `where` the normalizer REFUSES (an unknown operator, a non-array + * `$and`, an unlowerable filter array) is not judged here: this gate stands + * down and lets the refusal happen where it already does. Those inputs + * already answer `INVALID_FILTER`/400 from the strategy (#5352/#5367's + * geography, not this gate's), and pulling them forward into `ensureCube` + * would newly refuse them on the draft-preview path too, whose + * `matchesWhere` never consults the normalizer at all. A field gate that + * cannot read the tree has nothing to say about it. + * - 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 + * + * `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. + */ + private assertWhereFields(query: AnalyticsQuery, cube: Cube, declaredDimensions: string[]): void { + const probe = this.getObjectFieldNames; + if (!probe) return; + const where = (query as { where?: unknown }).where; + if (!where || typeof where !== 'object') return; + + const object = typeof cube.sql === 'string' ? cube.sql.trim() : ''; + if (!object || !BARE_IDENTIFIER.test(object)) return; + const fieldNames = probe(object); + if (!fieldNames || fieldNames.length === 0) return; + const known = new Set([...fieldNames, 'id', 'created_at', 'updated_at']); + + /** Every member the compiled predicate will bind against, structure discarded. */ + let members: string[]; + try { + members = collectFilterLeaves(normalizeAnalyticsFilterTree(query)).map((leaf) => leaf.member); + } catch { + // A `where` this layer refuses outright — see the stand-down note above. + return; + } + 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. + const invalid = new Set(); + for (const member of members) { + const { key, source } = resolveMemberSource(cube, member, 'any'); + if (source && !known.has(source)) invalid.add(key); + } + if (invalid.size === 0) return; + const usable = declaredDimensions.filter((d) => !invalid.has(d)); + + for (const member of members) { + const { source } = resolveMemberSource(cube, member, 'any'); + if (!source || known.has(source)) continue; + + const err = new Error( + `Filter member '${member}' in 'where' on cube '${cube.name}' constrains field ` + + `'${source}', which object '${object}' does not have. ` + + `Valid filter members: ${usable.join(', ') || '(none)'}. ` + + `Any of the object's OWN fields may also be filtered on without the cube ` + + `declaring it, so check the spelling of ` + + `'${source}' — known fields: ${[...fieldNames].sort().join(', ')}.`, + ) as Error & { code?: string; status?: number; field?: string; object?: string; param?: string; member?: string }; + err.code = 'INVALID_FIELD'; + err.status = 400; + err.field = source; + err.object = object; + err.param = 'where'; + err.member = member; + throw err; + } + } + /** * [#3867] Gate on the cube auto-inference path: a name with no registered * Cube may only be inferred into one if it is a registered object. @@ -1374,6 +1578,12 @@ export class AnalyticsService implements IAnalyticsService { // 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; const stripped = stripPrefix(key);