From cb4a60b931a83b4574f0343434f9fdfe5f2625f5 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 5 Aug 2026 23:29:19 +0000 Subject: [PATCH] fix(service-analytics,rest): gate analytics DIMENSIONS on the object's fields, and stop the dataset 500 echoing SQL (#5520) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #4437 gave a measure over a non-existent field a `400 INVALID_FIELD` naming the field — a driver error class must never be the caller's `error.code` for a caller-shaped mistake (ADR-0112). It covered the measure half only, so the identical typo one request key over still reached the driver as a `GROUP BY` column and came back as `500 SQLITE_ERROR`, while the measure control group on the same route answered a clean 400. `ensureCube` now runs `assertDimensionFields` alongside `assertMeasureFields` on all three of its paths, so a dimension whose source column the backing object does not have is refused before any SQL exists, with the same envelope (`INVALID_FIELD`/400 + `field`/`object`/`param`) and a message naming the field, the valid dimensions and the object's known fields. `query`, `generateSql` and `queryDataset` are all covered; a rejected query leaves the cube registry as it found it. `timeDimensions` are covered too — same `cube.dimensions` bag, same `lookupMember`, same 500 — with `param` naming the key that carried it. Grouping by a REAL field the cube never declared keeps working: the question is "does the object have this field", never "did the cube declare this dimension". Expression cubes, dotted relation dimensions and probe-less hosts stand down exactly as the measure gate stands down. `POST /analytics/dataset/query` composed its own 5xx body and echoed the message verbatim, so a knex ` - ` error handed the caller the generated statement with its physical table and column names. The sibling face never did: `/analytics/query` exits through `dispatcher-plugin.errorResponseBase`, which has applied the shared `looksLikeInternalErrorLeak` predicate to every >= 500 message since #3867. That predicate now guards this route's 500 body too — status, code, the ADR-0112 envelope branch and the transitional message list all unchanged, and the full text still reaches `logError`. Fixes objectstack-ai/objectstack#5520 Co-Authored-By: Claude --- .../analytics-dimension-source-field-gate.md | 58 ++ .../analytics-dataset-dimension-gate.test.ts | 310 ++++++++++ packages/rest/src/rest-server.ts | 24 +- .../dimension-source-field-gate.test.ts | 566 ++++++++++++++++++ .../src/analytics-service.ts | 163 +++++ .../services/service-analytics/src/plugin.ts | 3 +- 6 files changed, 1122 insertions(+), 2 deletions(-) create mode 100644 .changeset/analytics-dimension-source-field-gate.md create mode 100644 packages/rest/src/analytics-dataset-dimension-gate.test.ts create mode 100644 packages/services/service-analytics/src/__tests__/dimension-source-field-gate.test.ts diff --git a/.changeset/analytics-dimension-source-field-gate.md b/.changeset/analytics-dimension-source-field-gate.md new file mode 100644 index 0000000000..d9b35b6068 --- /dev/null +++ b/.changeset/analytics-dimension-source-field-gate.md @@ -0,0 +1,58 @@ +--- +"@objectstack/service-analytics": patch +"@objectstack/rest": patch +--- + +fix(service-analytics,rest): an analytics dimension over a missing field answers 400 INVALID_FIELD, not a driver 500 (#5520) + +#4437 gave a **measure** over a non-existent field a `400 INVALID_FIELD` naming +the field, because a driver error class must never be the caller's `error.code` +for a caller-shaped mistake (ADR-0112). It covered the measure half only, so the +identical typo one request key over still reached the driver as a `GROUP BY` +column: + +``` +POST /analytics/query {"cube":"account_metrics","measures":["account_count"],"dimensions":["bogus_dim"]} +→ 500 {"code":"SQLITE_ERROR","message":"Internal server error"} + +# the control group on the same route, already fixed by #4437 +POST /analytics/query {"cube":"account_metrics","measures":["bogus_measure"]} +→ 400 {"code":"INVALID_FIELD","message":"Measure 'bogus_measure' … Valid measures: …"} +``` + +**The gate.** `ensureCube` now runs `assertDimensionFields` alongside +`assertMeasureFields` on every path, so a dimension whose source column the +backing object does not have is refused **before** any SQL is built, with the +same envelope the measure gate uses: `INVALID_FIELD` / 400 plus +`field` / `object` / `param`, a message naming the field, the valid dimensions, +and the object's known field list. `query`, `generateSql` and `queryDataset` are +all covered, and a rejected query leaves nothing behind in the cube registry. +`timeDimensions` are covered too — they resolve through the same +`cube.dimensions` bag and produced the same 500 — with `param` reporting which +request key carried the bad name. + +**What deliberately did not change:** grouping by a REAL field the cube never +declared as a dimension (`dimensions: ["phone"]`) still works. The gate asks +"does the *object* have this field", never "did the cube declare this +dimension". A cube whose `sql` is an expression, a dotted relation dimension, +and a host that wires no field-name probe are all stood down on, exactly as the +measure gate stands down. + +**The SQL echo, same request.** `POST /analytics/dataset/query` composed its own +5xx body and echoed the error message verbatim. Knex prefixes the offending +statement to its message, so the caller received the generated SQL — physical +table and column names included: + +``` +500 {"code":"ANALYTICS_QUERY_FAILED", + "error":"SELECT bogus_dim AS \"bogus_dim\", COUNT(*) AS \"account_count\" + FROM \"crm_account\" GROUP BY bogus_dim - no such column: bogus_dim"} +``` + +The sibling face never leaked it: `/analytics/query` exits through the +dispatcher, which has applied the shared `looksLikeInternalErrorLeak` predicate +to every >= 500 message since #3867. That same predicate now guards this route's +500 body. Classification is untouched — the status stays 500, the code stays +`ANALYTICS_QUERY_FAILED`, the ADR-0112 envelope branch and the transitional +message list are unchanged — and the full text still reaches server logs. A 500 +whose message does not look like driver output keeps its prose. diff --git a/packages/rest/src/analytics-dataset-dimension-gate.test.ts b/packages/rest/src/analytics-dataset-dimension-gate.test.ts new file mode 100644 index 0000000000..2f17c84ccd --- /dev/null +++ b/packages/rest/src/analytics-dataset-dimension-gate.test.ts @@ -0,0 +1,310 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#5520] `POST /analytics/dataset/query` — the caller's own view of the + * dimension source-field gate, and of the SQL echo that rode along with it. + * + * Two faults, one caller typo, both visible only from this seam: + * + * 1. A selection naming a field the object does not have reached the driver and + * came back as a driver error with no envelope, so this route answered + * `500 ANALYTICS_QUERY_FAILED`. The service-side gate + * (`assertDimensionFields`, pinned in service-analytics' + * `dimension-source-field-gate.test.ts`) now rejects it with + * `INVALID_FIELD`/400, and #5352's envelope branch carries that verdict + * through untouched — which is what the first block asserts END TO END, + * because "the service throws the right shape" and "the caller receives it" + * are different facts. + * + * 2. The 500 body echoed the message verbatim. Knex prefixes the offending + * statement to its own message (` - `), so the response carried + * the generated SQL — physical table and column names included: + * + * ``` + * {"code":"ANALYTICS_QUERY_FAILED","error":"SELECT bogus_dim AS \"bogus_dim\", + * COUNT(*) AS \"account_count\" FROM \"crm_account\" GROUP BY bogus_dim + * - no such column: bogus_dim"} + * ``` + * + * The SIBLING analytics face never leaked it: `/analytics/query` exits + * through `dispatcher-plugin.errorResponseBase`, which has applied + * `looksLikeInternalErrorLeak` to every >=500 message since #3867 — which is + * precisely why the issue's repro ① read `"Internal server error"` while + * repro ③ dumped the statement. The second block pins the missing + * application of that same shared predicate here. Classification is NOT + * touched: the status stays 500, the code stays `ANALYTICS_QUERY_FAILED`, + * #5352's envelope branch and the transitional message list are untouched + * (#5367 owns that list), and the full text still reaches the operator's log. + * + * ## Reverse verification, direction predicted BEFORE running + * + * Two independent halves, and they fail in different places: + * - Remove `assertDimensionFields` from `ensureCube` → the first block's three + * rejection cases go RED (they answer 500 again) and its positive control + * stays GREEN. Measured: only TWO went red at first, because the "no + * generated SQL" case was satisfied by the OTHER fix — the sanitiser + * withheld the driver message, so the body was leak-free without the gate. + * That case now also asserts the 400, so each fix is falsifiable on its own; + * the mis-prediction is recorded here because it is the whole reason the + * second block injects its driver error directly rather than provoking one + * through a bogus dimension. + * - Restore `error: msg.slice(0, 500)` → the second block's withheld cases go + * RED and the first block stays GREEN (a 400 never reaches that branch). + * Both confirmed by running them (see the PR). + * + * Note this file exercises the BUILT `@objectstack/service-analytics`, not its + * sources: mutating the service without rebuilding it proves nothing here. + */ + +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import type { Logger } from '@objectstack/spec/contracts'; +import { AnalyticsService } from '@objectstack/service-analytics'; +import { INTERNAL_ERROR_MESSAGE } from '@objectstack/types'; +import { RestServer } from './rest-server'; + +// ── harness (the shape `analytics-filter-refusal-envelope.test.ts` uses) ────── + +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 did: the statement prefixed to the cause. That is what + * made the leak reachable, 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 = /\bbogus_dim\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), + }); +} + +/** A service double whose `queryDataset` throws exactly the given error. */ +function throwingAnalytics(error: unknown) { + return { queryDataset: vi.fn().mockRejectedValue(error) }; +} + +async function post(route: any, body: unknown) { + const res = mockRes(); + await route.handler({ method: 'POST', params: {}, headers: {}, body } as any, res); + return res; +} + +/** The generated statement the pre-fix response carried, verbatim from repro ③. */ +const LEAKED_SQL = + 'SELECT bogus_dim AS "bogus_dim", COUNT(*) AS "account_count" FROM "crm_account" GROUP BY bogus_dim'; + +let logged: string[] = []; +let consoleError: ReturnType; +beforeEach(() => { + logged = []; + consoleError = vi.spyOn(console, 'error').mockImplementation((...args: unknown[]) => { + logged.push(args.map((a) => (a instanceof Error ? a.message : String(a))).join(' ')); + }); +}); +afterEach(() => consoleError.mockRestore()); + +// ───────────────────────────────────────────────────────────────────────────── + +describe('[#5520] a bogus selection dimension answers 400 INVALID_FIELD, end to end', () => { + it('names the field and the object — and is not a 500 (repro ③)', async () => { + const route = buildRoute(async () => realAnalytics()); + const res = await post(route, { + dataset, + selection: { measures: ['account_count'], dimensions: ['bogus_dim'] }, + }); + + 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(/groups by field 'bogus_dim'/); + expect(String(res.body.message)).toMatch(/object 'crm_account' does not have/); + }); + + it('carries no generated SQL — because the statement was never built', async () => { + // Written first as "the body contains no SELECT/GROUP BY" and REJECTED at + // that: with the gate removed it stayed green, because the message that + // replaced it was withheld by the sanitiser in the second block. A leak-free + // body is not evidence of this fix unless the body is also the 400 the gate + // produces, so the assertion pins BOTH halves of the one claim: the answer + // is the field-naming 400, and that answer carries no statement. + const route = buildRoute(async () => realAnalytics()); + const res = await post(route, { + dataset, + selection: { measures: ['account_count'], dimensions: ['bogus_dim'] }, + }); + + 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(/GROUP BY/i); + expect(body).not.toMatch(/no such column/); + }); + + it('a POSITIVE control: the same wiring with a real field → 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 that this + // dataset never declared as a dimension, and it still groups. + const route = buildRoute(async () => realAnalytics()); + + const declared = await post(route, { + dataset, + selection: { measures: ['account_count'], dimensions: ['industry'] }, + }); + 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'], dimensions: ['phone'] }, + }); + expect(undeclaredButReal.statusCode).toBe(200); + }); + + it('a bogus TIME dimension answers the same way, naming that request key', async () => { + const route = buildRoute(async () => realAnalytics()); + const res = await post(route, { + dataset, + selection: { + measures: ['account_count'], + dimensions: [], + timeDimensions: [{ dimension: 'bogus_dim', granularity: 'month' }], + }, + }); + + expect(res.statusCode).toBe(400); + expect(res.body.code).toBe('INVALID_FIELD'); + }); +}); + +describe('[#5520] the 500 body no longer ships driver internals', () => { + it('withholds a message carrying the generated statement, and logs it instead', async () => { + // The class the gate cannot close: any other driver fault whose message + // arrives with the statement attached. + const driverError = new Error(`${LEAKED_SQL} - no such column: bogus_dim`); + const route = buildRoute(async () => throwingAnalytics(driverError)); + const res = await post(route, { dataset, selection: { measures: ['account_count'], dimensions: ['industry'] } }); + + // Classification is unchanged — only the prose is withheld. + expect(res.statusCode).toBe(500); + expect(res.body.code).toBe('ANALYTICS_QUERY_FAILED'); + expect(res.body.error).toBe(INTERNAL_ERROR_MESSAGE); + expect(JSON.stringify(res.body)).not.toContain('crm_account'); + expect(JSON.stringify(res.body)).not.toContain('GROUP BY'); + + // The operator keeps the whole diagnostic: after this change the log line is + // the ONLY copy, which is the reason it is asserted here and not assumed. + expect(logged.join('\n')).toContain(LEAKED_SQL); + }); + + it('withholds a dialect error code as well (`SQLITE_ERROR`, `SQLSTATE`)', async () => { + for (const message of [ + 'SQLITE_ERROR: no such column: bogus_dim', + 'error: column "bogus_dim" does not exist (SQLSTATE 42703)', + ]) { + const route = buildRoute(async () => throwingAnalytics(new Error(message))); + const res = await post(route, { dataset, selection: { measures: ['account_count'], dimensions: ['industry'] } }); + + expect(res.statusCode, message).toBe(500); + expect(res.body.code, message).toBe('ANALYTICS_QUERY_FAILED'); + expect(res.body.error, message).toBe(INTERNAL_ERROR_MESSAGE); + } + }); + + it('keeps an ordinary internal fault readable — the narrowing is targeted, not a blanket withhold', async () => { + // `looksLikeInternalErrorLeak` is a heuristic over the MESSAGE, deliberately + // not a driver taxonomy, and applying it here must not turn every 500 into + // an opaque one: a self-authored fault still says what happened. (The 5xx + // family that withholds unconditionally is #5437's `sendError` path; this + // route composes its own body and keeps the #3867 tiering.) + const route = buildRoute(async () => + throwingAnalytics(new Error('[Analytics] no strategy can handle query for cube "account_metrics"')), + ); + const res = await post(route, { dataset, selection: { measures: ['account_count'], dimensions: ['industry'] } }); + + expect(res.statusCode).toBe(500); + expect(res.body.code).toBe('ANALYTICS_QUERY_FAILED'); + expect(String(res.body.error)).toMatch(/no strategy can handle query/); + }); + + it('does not disturb the 4xx branches #5352 and #5367 own', async () => { + // ① a producer-declared 4xx envelope still passes through with its own code… + const enveloped = Object.assign(new Error('Unsupported filter operator "$sortOf" on "stage".'), { + code: 'INVALID_FILTER', + status: 400, + }); + const a = await post( + buildRoute(async () => throwingAnalytics(enveloped)), + { dataset, selection: { measures: ['account_count'], dimensions: ['industry'] } }, + ); + expect(a.statusCode).toBe(400); + expect(a.body.code).toBe('INVALID_FILTER'); + expect(String(a.body.message)).toMatch(/\$sortOf/); + + // …② and the transitional message list still answers 400 DATASET_INVALID + // with its message intact. #5367 owns that list; this change is 5xx-only. + const b = await post( + buildRoute(async () => + throwingAnalytics(new Error('[dataset-compiler] dimension "region" is not declared in the dataset.')), + ), + { dataset, selection: { measures: ['account_count'], dimensions: ['industry'] } }, + ); + expect(b.statusCode).toBe(400); + expect(b.body.code).toBe('DATASET_INVALID'); + expect(String(b.body.message)).toMatch(/not declared in the dataset/); + }); +}); diff --git a/packages/rest/src/rest-server.ts b/packages/rest/src/rest-server.ts index d37cf0578d..4bc77589b4 100644 --- a/packages/rest/src/rest-server.ts +++ b/packages/rest/src/rest-server.ts @@ -6811,8 +6811,30 @@ export class RestServer { if (/not declared in the dataset|not backed by a declared relationship|not supported by the v1 dataset runtime|read-scope-sql|not a selected dimension or measure|is not a subset of the selected dimensions/.test(msg)) { return res.status(400).json({ code: 'DATASET_INVALID', message: msg.slice(0, 1000) }); } + // ── ③ The 500 — and it does not ship driver internals ──── + // [#5520] This route built its 5xx body by hand and echoed + // the message verbatim, so a driver error arrived here with + // the generated statement prefixed to it (knex's format is + // ` - `) and the caller received the physical + // table and column names of the query: + // + // {"code":"ANALYTICS_QUERY_FAILED","error":"SELECT bogus_dim AS + // \"bogus_dim\", COUNT(*) … FROM \"crm_account\" GROUP BY + // bogus_dim - no such column: bogus_dim"} + // + // The SIBLING analytics face never did: `/analytics/query` + // exits through `dispatcher-plugin.errorResponseBase`, which + // applies `looksLikeInternalErrorLeak` to any >=500 message + // (#3867) — which is why the same mistake read "Internal + // server error" there and dumped SQL here. One boundary + // property, one shared predicate; this is the application + // that was missing, not a new rule. Classification is + // untouched (still `500 ANALYTICS_QUERY_FAILED`), and the + // full text still reaches the operator through `logError` + // immediately below — the log line is now the only copy. logError('[REST] Analytics dataset query error:', error); - res.status(500).json({ code: 'ANALYTICS_QUERY_FAILED', error: msg.slice(0, 500) }); + const outward = looksLikeInternalErrorLeak(msg) ? INTERNAL_ERROR_MESSAGE : msg.slice(0, 500); + res.status(500).json({ code: 'ANALYTICS_QUERY_FAILED', error: outward }); } }, metadata: { summary: 'Run a semantic-layer dataset (preview/query)', tags: ['analytics'] }, diff --git a/packages/services/service-analytics/src/__tests__/dimension-source-field-gate.test.ts b/packages/services/service-analytics/src/__tests__/dimension-source-field-gate.test.ts new file mode 100644 index 0000000000..8e08c91d78 --- /dev/null +++ b/packages/services/service-analytics/src/__tests__/dimension-source-field-gate.test.ts @@ -0,0 +1,566 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #5520 — the dimension SOURCE-FIELD gate, the symmetric half of #4437. + * + * #4437 gave MEASURES a `400 INVALID_FIELD` naming the field, and stopped there. + * The identical mistake one request key over kept reaching the driver, on both + * analytics faces. Live repro on hotcrm + 17.0.0-rc.2 (SQLite file driver) + * before the fix: + * + * ``` + * POST /analytics/query {"cube":"account_metrics","measures":["account_count"],"dimensions":["bogus_dim"]} + * → 500 {"code":"SQLITE_ERROR","message":"Internal server error"} + * + * POST /analytics/dataset/query {"datasetName":"account_metrics", + * "selection":{"measures":["account_count"],"dimensions":["bogus_dim"]}} + * → 500 {"code":"ANALYTICS_QUERY_FAILED", + * "error":"SELECT bogus_dim AS \"bogus_dim\", COUNT(*) AS \"account_count\" + * FROM \"crm_account\" GROUP BY bogus_dim - no such column: bogus_dim"} + * ``` + * + * — while the measure control group on the very same route answered a clean + * 400. Two ADR-0112 faults for one caller typo: a driver error class as the + * caller's `error.code`, and (on the dataset face) the generated statement, + * physical table and column names included, echoed back to the caller. + * + * The cases below pin three things: the rejection itself on BOTH paths, that a + * rejected query never reaches the driver (so no statement exists to echo), and + * — the guard that keeps the gate from over-reaching — that grouping by a REAL + * field the cube never declared still works, because that is the established + * contract and the dimension twin of measure auto-inference. + * + * ## Reverse verification, direction predicted BEFORE running + * + * Removing the three `assertDimensionFields` calls from `ensureCube` turns RED + * every case in the first two blocks that asserts on the rejection — they assert + * on an error only the gate produces — and leaves the third block ("what the + * gate must NOT do") GREEN, because those cases assert the pre-#5520 behaviour + * this change preserves. Ordinary direction, no inversion: the rejection is new, + * and no `??`-chain order was touched. Predicted 10 red / 12 green; measured + * exactly that. + * + * The one case that survives the mutation inside the second block is deliberate: + * "the pre-fix driver error carried the statement" asserts the OLD behaviour on + * a cube the gate stands down for, so it is green before and after — it is the + * control that proves the harness can still produce the leak this 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 every object an aggregate actually ran against and `sqls` + * every statement the native path built, so a test can assert the rejected query + * 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 exactly how the SQL got into the + * caller's response body. + */ +function makeService( + opts: { cubes?: Cube[]; wireProbe?: boolean; fields?: string[]; native?: boolean } = {}, +) { + const aggregated: string[] = []; + 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) => { + aggregated.push(objectName); + return [{ account_count: 1 }]; + }, + executeRawSql: async (objectName: string, sql: string) => { + aggregated.push(objectName); + sqls.push(sql); + const bogus = /\b(bogus_dim|bogus_at|dropped_column)\b/.exec(sql)?.[1]; + if (bogus) throw new Error(`${sql} - no such column: ${bogus}`); + return [{ account_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, sqls }; +} + +/** + * The error a call rejected with, typed — and a loud failure if it RESOLVED. + * + * `.catch((e) => e as Error)` (the sibling measure-gate file's spelling) widens + * the awaited value to `Error | AnalyticsResult`, on which `.message` type-checks + * as neither: four frozen TS2339s in `tsc --noEmit`. That debt is #4311's, not + * worth reworking someone else's file for — but a new file must not add to it. + */ +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 gate already produces for the same mistake (#4437). */ +const INVALID_FIELD = { + code: 'INVALID_FIELD', + status: 400, + object: 'crm_account', + param: 'dimensions', +}; + +/** The dataset behind repro ③ — one declared dimension, one declared 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('#5520 — the gate: a dimension over a missing field is a 400, not a driver 500', () => { + it('refuses the bare-cube path (repro ①) and names the field', async () => { + const { service, aggregated } = makeService(); + + await expect( + service.query({ + cube: 'crm_account', + measures: ['count'], + dimensions: ['bogus_dim'], + } as any), + ).rejects.toMatchObject({ ...INVALID_FIELD, field: 'bogus_dim', dimension: 'bogus_dim' }); + + // The whole point: the typo never became a `GROUP BY` 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'], dimensions: ['bogus_dim'] } as any), + ); + + expect(err.message).toMatch(/groups by field 'bogus_dim'/); + 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 guard cases below pin, so a + // caller is not left thinking they must declare a dimension first. + expect(err.message).toMatch(/may also be used as a dimension without the cube declaring it/); + }); + + it('does not offer the caller their own typo back as a valid dimension', async () => { + // On the auto-inference path the bogus dimension is already in + // `cube.dimensions` (it was minted from this very query), so echoing the + // cube's dimension list verbatim would suggest `bogus_dim` — the one + // alternative guaranteed not to work. + const { service } = makeService(); + + const err = await rejection( + service.query({ + cube: 'crm_account', + measures: ['count'], + dimensions: ['industry', 'bogus_dim'], + } as any), + ); + + expect(err.message).toMatch(/Valid dimensions: industry\./); + expect(err.message).not.toMatch(/Valid dimensions:[^.]*bogus_dim/); + }); + + 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'], dimensions: ['bogus_dim'] } as any), + ); + + expect(err.message).toMatch(/Valid dimensions: \(none\)\./); + }); + + it('refuses a bogus TIME dimension too, naming that request key', async () => { + // Same bag (`cube.dimensions`), same `lookupMember`, same 500 before the + // fix — `date_trunc('month', bogus_at)` on a column that does not exist. + // Only `param` differs, because that is the key the caller must fix. + const { service, aggregated } = makeService(); + + await expect( + service.query({ + cube: 'crm_account', + measures: ['count'], + timeDimensions: [{ dimension: 'bogus_at', granularity: 'month' }], + } as any), + ).rejects.toMatchObject({ + code: 'INVALID_FIELD', + status: 400, + object: 'crm_account', + param: 'timeDimensions', + field: 'bogus_at', + dimension: 'bogus_at', + }); + expect(aggregated).toEqual([]); + }); + + it('does not poison the registry with the rejected cube', async () => { + // Same rule the #3867 inference gate and the #4437 measure gate keep: a + // rejected query must leave no trace, or the retry finds a "registered" + // cube carrying the bogus dimension and sails straight into SQL. + const { service, aggregated } = makeService(); + + await expect( + service.query({ cube: 'crm_account', measures: ['count'], dimensions: ['bogus_dim'] } as any), + ).rejects.toThrow(); + expect(service.cubeRegistry.get('crm_account')).toBeUndefined(); + + await expect( + service.query({ cube: 'crm_account', measures: ['count'], dimensions: ['bogus_dim'] } 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 grouping by a column that does not exist. + const { service } = makeService(); + + await expect( + service.generateSql({ + cube: 'crm_account', + measures: ['count'], + dimensions: ['bogus_dim'], + } 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 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] }); + + await expect( + service.query({ cube: 'account_cube', measures: ['count'], dimensions: ['legacy'] } as any), + ).rejects.toMatchObject({ + ...INVALID_FIELD, + field: 'dropped_column', + dimension: 'legacy', + }); + expect(aggregated).toEqual([]); + + // Its healthy sibling still groups, and is what the rejection suggests. + await service.query({ cube: 'account_cube', measures: ['count'], dimensions: ['industry'] } as any); + expect(aggregated).toEqual(['crm_account']); + }); +}); + +describe('#5520 — the dataset face: refused before SQL exists, so nothing can echo it', () => { + it('refuses a bogus selection dimension (repro ③) with the same envelope', async () => { + const { service, aggregated, sqls } = makeService({ native: true }); + + await expect( + service.queryDataset(ACCOUNT_METRICS, { + measures: ['account_count'], + dimensions: ['bogus_dim'], + } as any), + ).rejects.toMatchObject({ ...INVALID_FIELD, field: 'bogus_dim', dimension: 'bogus_dim' }); + + // Repro ③'s leak, at its source: 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-dimension-gate.test.ts` — + // but the driver error that carried the statement no longer happens.) + expect(sqls).toEqual([]); + expect(aggregated).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'], + dimensions: ['bogus_dim'], + } as any), + ); + + // It names the object and the field — a caller-shaped fact — and neither + // a SELECT list, a quoted table name, nor a GROUP BY clause. + expect(err.message).toContain("object 'crm_account' does not have"); + expect(err.message).not.toMatch(/SELECT/i); + expect(err.message).not.toMatch(/GROUP BY/i); + 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 dimension the gate cannot judge, the SAME harness + // still produces the knex-shaped ` - ` message that used to + // reach callers verbatim. This is what the gate removes for a caller typo + // and what `looksLikeInternalErrorLeak` withholds for everything else. + const derived: Cube = { + name: 'derived_cube', + title: 'Derived', + sql: 'SELECT * FROM crm_account', + measures: { count: { name: 'count', label: 'Count', type: 'count', sql: '*' } }, + dimensions: { + bogus_dim: { name: 'bogus_dim', label: 'x', type: 'string', sql: 'bogus_dim' }, + }, + public: false, + }; + const { service } = makeService({ cubes: [derived], native: true }); + + const err = await rejection( + service.query({ cube: 'derived_cube', measures: ['count'], dimensions: ['bogus_dim'] } as any), + ); + + expect(err.code).toBeUndefined(); + expect(err.message).toMatch(/^SELECT /); + expect(err.message).toMatch(/no such column: bogus_dim/); + }); +}); + +describe('#5520 — what the gate must NOT do', () => { + it('lets an UNDECLARED but REAL field group, on the bare-cube path', async () => { + // `dimensions: ['phone']` on a cube that declares no `phone` is an + // established contract — the dimension twin of measure auto-inference — + // and grouping by it returned 200 before this change. The gate asks + // "does the OBJECT have this field", never "did the cube declare it". + const { service, aggregated } = makeService(); + + const result = await service.query({ + cube: 'crm_account', + measures: ['count'], + dimensions: ['phone'], + } as any); + + expect(result.rows).toHaveLength(1); + expect(result.fields.map((f) => f.name)).toContain('phone'); + expect(aggregated).toEqual(['crm_account']); + }); + + it('lets an UNDECLARED but REAL field group, on the dataset path', async () => { + // Same contract through `queryDataset`: `phone` is not one of + // `account_metrics`' declared dimensions, and still groups. + const { service, sqls } = makeService({ native: true }); + + const result = await service.queryDataset(ACCOUNT_METRICS, { + measures: ['account_count'], + dimensions: ['phone'], + } as any); + + expect(result.rows).toEqual([{ account_count: 1 }]); + expect(sqls).toEqual([ + 'SELECT phone AS "phone", COUNT(*) AS "account_count" FROM "crm_account" GROUP BY phone', + ]); + }); + + 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'], + dimensions: ['id', 'created_at', 'updated_at'], + } 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 } = makeService({ cubes: [authored] }); + + await service.query({ cube: 'renamed_cube', measures: ['count'], dimensions: ['assessed'] } as any); + + expect(aggregated).toEqual(['crm_account']); + }); + + it('accepts the canonical `.` qualifier', async () => { + const { service, aggregated } = makeService(); + + await service.query({ + cube: 'crm_account', + measures: ['count'], + dimensions: ['crm_account.industry'], + } as any); + + expect(aggregated).toEqual(['crm_account']); + }); + + it('leaves a cube whose `sql` is an expression alone — no field list to check', async () => { + // `sql` is a subquery, not an object name: there is no schema to consult, + // and guessing would reject perfectly good authored analytics. + 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: { + anything: { name: 'anything', label: 'x', type: 'string', sql: 'anything' }, + }, + public: false, + }; + const { service } = makeService({ cubes: [derived] }); + + await expect( + service.query({ cube: 'derived_cube', measures: ['count'], dimensions: ['anything'] } as any), + ).resolves.toBeTruthy(); + }); + + it('leaves a dotted relation dimension to the layers that own it', async () => { + // `account.industry` resolves through a JOIN this gate cannot see — + // `industry` 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. + 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'], dimensions: ['owner.region'] } as any), + ); + + // Either it ran, or it was declined by the join layer — but never here. + expect(settled.code).not.toBe('INVALID_FIELD'); + }); + + 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'], dimensions: ['bucket'] } as any), + ).resolves.toBeTruthy(); + }); + + it('stands down when no field probe is configured — nothing to consult', async () => { + // Same tiering as the #3867 registry gate, the #4437 measure gate and the + // data path's `resolveQueryFields`: 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'], dimensions: ['bogus_dim'] } as any); + + expect(aggregated).toEqual(['crm_account']); + }); + + it('stands down for an object the probe cannot describe', async () => { + // An external datasource whose columns are not mirrored locally answers + // `undefined` — "cannot answer", not "has no fields". + const external: Cube = { + name: 'external_cube', + title: 'External', + sql: 'remote_table', + measures: { count: { name: 'count', label: 'Count', type: 'count', sql: '*' } }, + dimensions: { + ghost: { name: 'ghost', label: 'x', type: 'string', sql: 'ghost' }, + }, + public: false, + }; + const { service } = makeService({ cubes: [external] }); + + await expect( + service.query({ cube: 'external_cube', measures: ['count'], dimensions: ['ghost'] } as any), + ).resolves.toBeTruthy(); + }); + + it('still answers about the MEASURE first when a query gets both wrong', async () => { + // One rejection at a time, and #4437's is the one that was already + // load-bearing for callers — so its envelope does not move. + const { service } = makeService(); + + await expect( + service.query({ + cube: 'crm_account', + measures: ['ghost_sum'], + dimensions: ['bogus_dim'], + } as any), + ).rejects.toMatchObject({ + code: 'INVALID_FIELD', + status: 400, + param: 'measures', + field: 'ghost', + }); + }); +}); diff --git a/packages/services/service-analytics/src/analytics-service.ts b/packages/services/service-analytics/src/analytics-service.ts index 8a3b83bc3b..1ff1a4d5fc 100644 --- a/packages/services/service-analytics/src/analytics-service.ts +++ b/packages/services/service-analytics/src/analytics-service.ts @@ -286,6 +286,12 @@ export interface AnalyticsServiceConfig { * the same mistake with a `400 INVALID_FIELD` naming the field (#4315/#4254); * this hook is what lets the ANALYTICS route give the same answer. * + * [#5520] The same probe now answers for DIMENSIONS too + * ({@link AnalyticsService.assertDimensionFields}). #4437 gated only the + * measure half, so the identical typo one key over — `dimensions: + * ['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. + * * 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 @@ -978,6 +984,13 @@ export class AnalyticsService implements IAnalyticsService { * `cube.measures` (e.g. `amount_sum`, `amount_avg` emitted by dashboard * 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 + * 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`. */ private ensureCube(query: AnalyticsQuery): void { const name = query.cube!; @@ -997,6 +1010,11 @@ export class AnalyticsService implements IAnalyticsService { // (same rule the #3867 gate above keeps), or a retry would find a // "registered" cube carrying the bogus measure and sail straight to SQL. this.assertMeasureFields(query, cube, Object.keys(cube.measures)); + // [#5520] …and the dimensions', for exactly the same reason. On this path + // `cube.dimensions` was minted from the query moments ago, so the bogus + // 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)); this.cubeRegistry.register(cube); // A scalar query — only measures, no grouping (no `dimensions`/ // `timeDimensions`) — is the first-class "metric over an object" path @@ -1037,6 +1055,10 @@ export class AnalyticsService implements IAnalyticsService { // cube so the rejection can suggest what the caller could have meant — // and so a rejected query leaves the registry as it found it. this.assertMeasureFields(query, augmented, Object.keys(cube.measures)); + // [#5520] Dimensions are never augmented (nothing infers one), so the + // authored/compiled list IS the vocabulary a caller may name — and the one + // the rejection suggests. + this.assertDimensionFields(query, augmented, Object.keys(cube.dimensions)); this.cubeRegistry.register(augmented); this.logger.debug( `[Analytics] Augmented cube "${name}" with inferred measures: ${Object.keys(extraMeasures).join(',')}`, @@ -1045,6 +1067,7 @@ export class AnalyticsService implements IAnalyticsService { // No inference happened — every measure is declared. Still validate: an // 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)); } } @@ -1135,6 +1158,146 @@ export class AnalyticsService implements IAnalyticsService { } } + /** + * [#5520] Reject a DIMENSION whose source field the backing object does not + * have, BEFORE the strategy compiles it into `GROUP BY`. + * + * The symmetric half of {@link assertMeasureFields}. #4437 closed the measure + * side and stopped there, so the identical mistake one request key over still + * reached the driver: + * + * ``` + * POST /analytics/query {"cube":"crm_account","measures":["account_count"],"dimensions":["bogus_dim"]} + * → 500 {"code":"SQLITE_ERROR","message":"Internal server error"} + * + * POST /analytics/dataset/query {"selection":{"dimensions":["bogus_dim"],…}} + * → 500 {"code":"ANALYTICS_QUERY_FAILED", + * "error":"SELECT bogus_dim AS \"bogus_dim\", … GROUP BY bogus_dim - no such column: bogus_dim"} + * ``` + * + * A driver error class as the caller's `error.code` is the ADR-0112 violation + * #4437 named, and the dataset face additionally echoed the generated + * statement — physical table and column names — back to the caller. The + * envelope here is deliberately the SAME as the measure gate's + * (`INVALID_FIELD`/400 + `field`/`object`/`param`), because "the query names a + * field the object does not have" is ONE mistake and must have one wire shape + * whichever member kind carried it. + * + * What it checks, and what it deliberately does not: + * + * - **Both dimension keys.** `query.dimensions` and `query.timeDimensions` + * land in the same `cube.dimensions` bag, are resolved by the same + * `lookupMember`, and produced the same 500 (a bogus time dimension became + * `date_trunc('month', bogus_at)`); `param` reports which key carried it. + * - **An UNDECLARED but real field stays legal.** `dimensions: ['phone']` on a + * cube that never declared `phone` groups by `phone` today — the dimension + * twin of measure auto-inference, and an established contract. So the + * question asked is "does the OBJECT have this field", never "did the cube + * declare this dimension". An undeclared member is checked against the + * object under the name the strategies would use as the column (their own + * `resolveDimensionSql`/`resolveFieldName` fallback: the member itself). + * - Only when the cube's `sql` is a bare OBJECT NAME, only when + * {@link AnalyticsServiceConfig.getObjectFieldNames} answers, and only for + * sources that are BARE COLUMNS — same three stand-downs as the measure + * gate, for the same reasons (no field list to check against; nothing + * authoritative to consult; a dotted reference resolves through a join whose + * target this gate cannot see, so it belongs to the join allowlist). + * - `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. + */ + private assertDimensionFields(query: AnalyticsQuery, cube: Cube, declaredDimensions: string[]): void { + const probe = this.getObjectFieldNames; + if (!probe) return; + /** Every dimension this query names, tagged with the request key it came from. */ + const members: Array<{ member: string; param: 'dimensions' | 'timeDimensions' }> = [ + ...(query.dimensions ?? []).map((member) => ({ member, param: 'dimensions' as const })), + ...(query.timeDimensions ?? []).map((td) => ({ member: td.dimension, param: 'timeDimensions' as const })), + ]; + if (members.length === 0) 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']); + + /** + * 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". + */ + 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 }; + }; + + // 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 + // verbatim would offer the caller their own typo back as a valid alternative. + const invalid = new Set(); + for (const { member } of members) { + const { key, source } = resolve(member); + if (source && !known.has(source)) invalid.add(key); + } + if (invalid.size === 0) return; + const usable = declaredDimensions.filter((d) => !invalid.has(d)); + + for (const { member, param } of members) { + const { source } = resolve(member); + if (!source || known.has(source)) continue; + + const kind = param === 'timeDimensions' ? 'Time dimension' : 'Dimension'; + const verb = param === 'timeDimensions' ? 'buckets' : 'groups by'; + const err = new Error( + `${kind} '${member}' on cube '${cube.name}' ${verb} field '${source}', which object ` + + `'${object}' does not have. ` + + `Valid dimensions: ${usable.join(', ') || '(none)'}. ` + + `Any of the object's OWN fields may also be used as a dimension 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; dimension?: string }; + err.code = 'INVALID_FIELD'; + err.status = 400; + err.field = source; + err.object = object; + err.param = param; + err.dimension = 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. diff --git a/packages/services/service-analytics/src/plugin.ts b/packages/services/service-analytics/src/plugin.ts index dca9434d9d..077b98fe8b 100644 --- a/packages/services/service-analytics/src/plugin.ts +++ b/packages/services/service-analytics/src/plugin.ts @@ -585,7 +585,8 @@ export class AnalyticsServicePlugin implements Plugin { if (!engine) return true; return engine.getObject?.(name) != null; }, - // [#4437] Field names for the measure source-field gate. Read from the + // [#4437, #5520] Field names for the two source-field gates — measures + // (#4437) and dimensions/timeDimensions (#5520). Read from the // SAME schema registry `isRegisteredObject` above consults (and the data // path's #4315 gate reads), so "which fields exist" has one answer across // /data and /analytics. `undefined` — no engine, unknown object, or an