diff --git a/.changeset/analytics-unlisted-refusal-envelope.md b/.changeset/analytics-unlisted-refusal-envelope.md new file mode 100644 index 0000000000..592d881b1c --- /dev/null +++ b/.changeset/analytics-unlisted-refusal-envelope.md @@ -0,0 +1,90 @@ +--- +"@objectstack/service-analytics": patch +--- + +fix(service-analytics): thirteen caller-shaped analytics refusals answer 4xx from their own envelope instead of `500` (#5716) + +**Observable behaviour change — read this if you alert, retry, or assert on status.** +Thirteen refusal conditions in `service-analytics` (twelve `throw` sites — the +cross-object measure and filter share one) used to reach the caller as +`500 {"code":"ANALYTICS_QUERY_FAILED"}` on `POST /analytics/dataset/query`, and as +`500 {"code":"INTERNAL_ERROR"}` on `POST /analytics/query`. They now answer **400** — +`DATASET_INVALID` for the seven that are a verdict about the dataset or the whole +selection, `INVALID_FIELD` for the six that name one member of the request: + +| refusal | now | +|---|---| +| dataset JOIN crosses datasources (#5115) | `DATASET_INVALID` / 400 | +| `include` names a relationship the object does not have | `DATASET_INVALID` / 400 | +| `include` path past the 3-hop limit | `DATASET_INVALID` / 400 | +| a `dateRange` bound that is not a date | `DATASET_INVALID` / 400 | +| `compareTo` names a timeDimension with no `dateRange` | `DATASET_INVALID` / 400 | +| `compareTo` with no dated window to shift | `DATASET_INVALID` / 400 | +| `compareTo` ambiguous between two dated windows | `DATASET_INVALID` / 400 | +| cube declares no such measure (#4157) | `INVALID_FIELD` / 400 | +| ObjectQL: cross-object time-dimension bucket | `INVALID_FIELD` / 400 | +| ObjectQL: cross-object measure | `INVALID_FIELD` / 400 | +| ObjectQL: cross-object filter | `INVALID_FIELD` / 400 | +| ObjectQL: multi-hop cross-object dimension | `INVALID_FIELD` / 400 | +| ObjectQL: non-recombinable measure over a cross-object dimension | `INVALID_FIELD` / 400 | + +Monitoring that counted these as server errors will see a 5xx disappear and a 4xx +appear, and a client retrying on 5xx will stop retrying a request that cannot +succeed until the request or the dataset changes. **No refusal condition moved and +no message was reworded** — the same inputs are refused, in the same words; only +the envelope is new. (The messages are load-bearing beyond readability: #5923's +tests assert the `planCrossObject` wording, and #5717 tracks one compiler message +for colliding with a downstream sniffer.) + +## What was wrong + +#5352 gave the dataset route a list of message SUBSTRINGS so six refusal families +could answer 400, and #5367 retired five of those entries by giving their +producers an ADR-0112 envelope. Both rounds worked from that list — and the list +was only ever the refusals someone had already hit. Reading every `throw` in the +package afterwards found thirteen more of exactly the same kind, which had never +been on it: a typo in `compareTo`, a `dateRange` the dashboard sent, a dataset +whose `include` names a relationship that does not exist. Each answered "the +platform is broken" for a mistake the caller or the author could fix, on both +analytics faces. + +**Both faces move, measured.** `/analytics/dataset/query` reads the envelope in +its catch (#5352); `/analytics/query` exits through +`dispatcher-plugin.errorResponseBase`, which already adopts a thrown `status` and +carries the `code` (#3867/#3842) — so the cross-object refusals go from +`500 INTERNAL_ERROR` to `400 INVALID_FIELD` there as well, without touching that +route. The open question #5811 tracks on that face is about *withholding the +message of a declared 5xx*, which none of these are. + +## Why two codes + +`dataset-refusal.ts` gains a second constructor, `invalidMemberError` +(`INVALID_FIELD` / 400 + `member`/`param`/`cube`), beside `datasetInvalidError`. +The split is by what the refusal is a verdict ABOUT: the dataset/selection as a +whole, or one member the request named. The member family is `INVALID_FIELD` +because the three shipped analytics gates already answer exactly that for the +NEIGHBOURING member-level mistakes on the same request keys — `measures` (#4437), +`dimensions`/`timeDimensions` (#5520), `where` (#5669) — so one class of mistake +keeps one wire shape; and because these six fire on `/analytics/query` too, where +there is no dataset for `DATASET_INVALID` to be about. No new code is registered: +both are already in the ADR-0112 vocabulary. + +## What deliberately did NOT change + +`native-sql-strategy`'s "measure … has unrecognised type" stays a bare `Error` +(an undeclared 500) although #5716 listed it as author-shaped. Measured: +`Metric.type` is the closed `AggregationMetricType` enum, `metric-type-coverage.test.ts` +pins that the strategy handles every member of it, the dataset compiler writes +only `SUPPORTED_AGGREGATES` into a cube, and `inferMeasure` mints six known types +— so no spec-valid cube can reach it. An arrival is our own drift or a host +registering an unparsed cube, and blaming the caller would hide a platform fault +from 5xx alerting. The two "Cube not found" guards and the two operator-drift +throws stay bare for the same reason. + +Coverage: `unlisted-refusal-envelope.test.ts` (service-analytics) drives all +thirteen refusals through the real producers — one block pinning that the refusal +SET and its wording are unchanged, one pinning the envelope, one pinning the +verdicts that stay 500; `analytics-dataset-unlisted-refusal-envelope.test.ts` +(rest) drives eleven of them end-to-end through the route with a real +`AnalyticsService`, plus three positive controls and the two sites that route +cannot reach (with the measurement that explains why). diff --git a/packages/rest/src/analytics-dataset-unlisted-refusal-envelope.test.ts b/packages/rest/src/analytics-dataset-unlisted-refusal-envelope.test.ts new file mode 100644 index 0000000000..22034419cb --- /dev/null +++ b/packages/rest/src/analytics-dataset-unlisted-refusal-envelope.test.ts @@ -0,0 +1,389 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#5716] `POST /analytics/dataset/query` — the caller-shaped refusals that were + * never on the message list, and therefore never left `500`. + * + * ## The seam this file asserts + * + * #5352 gave this route a list of message substrings so six refusal families + * could answer 400; #5367 retired five of those entries by giving their + * producers an ADR-0112 envelope. Both rounds worked from that list — and the + * list was only ever the refusals someone had already hit. Twelve more throw + * sites in `service-analytics` (thirteen refusal conditions) are the same kind of + * mistake — the caller's request, or the dataset they authored — and were + * answering `500 {"code":"ANALYTICS_QUERY_FAILED"}`: the platform reporting + * itself broken for a typo in a `compareTo`. + * + * The producer half is pinned per site in `service-analytics`'s + * `unlisted-refusal-envelope.test.ts`. This file is the SEAM: a real + * `AnalyticsService` compiling a real dataset, the error crossing into the + * route's catch is the one the real compiler/executor/strategy throws, and + * nothing here constructs a shape it also asserts. + * + * ## Reverse verification, direction predicted BEFORE running + * + * Revert any producer to `throw new Error(…)` (and rebuild `service-analytics` — + * this file exercises the BUILT package) and that case goes RED with + * `500 ANALYTICS_QUERY_FAILED`. Plain direction, no inversion: since #5808 the + * route carries NO message list at all, so "producer stops declaring" has + * exactly one outward consequence. Positive controls sit beside the refusals so + * a case cannot pass because the wiring never reached the producer. + * + * MEASURED with the four producer files reverted and `service-analytics` rebuilt: + * **11 red / 5 green** — every refusal case red on `expected 200/500 to be 400`, + * the three positive controls and the two "this route cannot show it" cases + * green, which is what makes them controls rather than filler. + */ + +import { describe, it, expect, vi } from 'vitest'; +import type { Logger } from '@objectstack/spec/contracts'; +import { AnalyticsService } from '@objectstack/service-analytics'; +import { RestServer } from './rest-server'; + +// ── harness (the shape `analytics-dataset-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', routes: { data: '', metadata: '' } }), + 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; +} + +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'))!; +} + +async function post(route: any, body: unknown) { + const res = mockRes(); + await route.handler({ method: 'POST', params: {}, headers: {}, body } as any, res); + return res; +} + +const silent: Logger = { debug() {}, info() {}, warn() {}, error() {} }; + +/** The ObjectQL aggregate path — one fixed bucket, so a valid selection is 200. */ +function aggregateAnalytics(extra: Record = {}): AnalyticsService { + return new AnalyticsService({ + logger: silent, + queryCapabilities: () => ({ nativeSql: false, objectqlAggregate: true, inMemory: false }), + executeAggregate: async () => [{ stage: 'won', revenue: 100 }], + isRegisteredObject: () => true, + ...extra, + } as never); +} + +/** The raw-SQL path. */ +function nativeAnalytics(extra: Record = {}): AnalyticsService { + return new AnalyticsService({ + logger: silent, + queryCapabilities: () => ({ nativeSql: true, objectqlAggregate: false, inMemory: false }), + executeRawSql: async () => [{ stage: 'won', revenue: 100 }], + isRegisteredObject: () => true, + ...extra, + } as never); +} + +/** A valid single-object dataset — nothing here needs a join. */ +const dataset = { + name: 'pipeline', + label: 'Pipeline', + object: 'crm_opportunity', + dimensions: [ + { name: 'stage', field: 'stage', type: 'string' }, + { name: 'close_date', field: 'close_date', type: 'date' }, + { name: 'created_at', field: 'created_at', type: 'date' }, + ], + measures: [{ name: 'revenue', aggregate: 'sum', field: 'amount' }], +}; +const selection = { dimensions: ['stage'], measures: ['revenue'] }; +const WINDOW = ['2026-01-01', '2026-01-31']; + +/** The same dataset, joined to `account` — the shape every cross-object case needs. */ +const joinedDataset = { + ...dataset, + name: 'pipeline_by_account', + include: ['account'], + dimensions: [ + { name: 'stage', field: 'stage', type: 'string' }, + { name: 'region', field: 'account.region', type: 'string' }, + { name: 'account_opened', field: 'account.created_at', type: 'date' }, + ], + measures: [ + { name: 'revenue', aggregate: 'sum', field: 'amount' }, + { name: 'avg_deal', aggregate: 'avg', field: 'amount' }, + { name: 'remote_sum', aggregate: 'sum', field: 'account.balance' }, + ], +}; + +// ───────────────────────────────────────────────────────────────────────────── + +describe('[#5716] a refusal nobody listed still answers 4xx, from its own envelope', () => { + const CASES: Array<{ + name: string; + body: unknown; + analytics: () => AnalyticsService; + code: 'DATASET_INVALID' | 'INVALID_FIELD'; + message: RegExp; + }> = [ + { + name: 'dataset-compiler: an `include` JOIN across datasources (#5115)', + analytics: () => + aggregateAnalytics({ + getObjectDatasource: (o: string) => (o === 'crm_opportunity' ? 'primary' : 'warehouse'), + }), + body: { + dataset: { ...dataset, include: ['account'], dimensions: [{ name: 'region', field: 'account.region', type: 'string' }] }, + selection: { dimensions: ['region'], measures: ['revenue'] }, + }, + code: 'DATASET_INVALID', + message: /declares a JOIN that crosses datasources/, + }, + { + name: 'dataset-compiler: an `include` relationship the object does not have', + analytics: () => aggregateAnalytics({ relationshipResolver: () => undefined }), + body: { + dataset: { ...dataset, include: ['bogus'] }, + selection, + }, + code: 'DATASET_INVALID', + message: /includes relationship "bogus" which does not exist on object "crm_opportunity"/, + }, + { + name: 'dataset-executor: a `dateRange` bound that is not a date', + analytics: aggregateAnalytics, + body: { + dataset, + selection: { + ...selection, + timeDimensions: [{ dimension: 'close_date', dateRange: ['2026-01-01', 'the-first-of-never'] }], + compareTo: { kind: 'previousPeriod' }, + }, + }, + code: 'DATASET_INVALID', + message: /invalid date in dateRange: "the-first-of-never"/, + }, + { + name: 'dataset-executor: compareTo names a dimension with no dateRange', + analytics: aggregateAnalytics, + body: { + dataset, + selection: { + ...selection, + timeDimensions: [{ dimension: 'close_date', dateRange: WINDOW }], + compareTo: { kind: 'previousPeriod', dimension: 'created_at' }, + }, + }, + code: 'DATASET_INVALID', + message: /compareTo requires a timeDimension "created_at" with a dateRange/, + }, + { + name: 'dataset-executor: compareTo with no dated window at all', + analytics: aggregateAnalytics, + body: { + dataset, + selection: { + ...selection, + timeDimensions: [{ dimension: 'close_date', granularity: 'month' }], + compareTo: { kind: 'previousPeriod' }, + }, + }, + code: 'DATASET_INVALID', + message: /compareTo needs a dated window to shift/, + }, + { + name: 'dataset-executor: compareTo ambiguous between two dated windows', + analytics: aggregateAnalytics, + body: { + dataset, + selection: { + ...selection, + timeDimensions: [ + { dimension: 'close_date', dateRange: WINDOW }, + { dimension: 'created_at', dateRange: WINDOW }, + ], + compareTo: { kind: 'previousPeriod' }, + }, + }, + code: 'DATASET_INVALID', + message: /compareTo\.dimension is ambiguous: 2 time dimensions carry a dateRange/, + }, + { + name: 'objectql-strategy: bucketing a cross-object time dimension', + analytics: aggregateAnalytics, + body: { + dataset: joinedDataset, + selection: { + dimensions: ['stage'], + measures: ['revenue'], + timeDimensions: [{ dimension: 'account_opened', granularity: 'month' }], + }, + }, + code: 'INVALID_FIELD', + message: /cannot bucket a cross-object time dimension \("account\.created_at"\)/, + }, + { + name: 'objectql-strategy: a cross-object MEASURE', + analytics: aggregateAnalytics, + body: { + dataset: joinedDataset, + selection: { dimensions: ['stage'], measures: ['remote_sum'] }, + }, + code: 'INVALID_FIELD', + message: /cannot evaluate a cross-object measure \("account\.balance"\)/, + }, + { + // A dashboard's own filter on a joined column — `runtimeFilter` is where a + // widget's date/segment picker lands. + name: 'objectql-strategy: a cross-object FILTER', + analytics: aggregateAnalytics, + body: { + dataset: joinedDataset, + selection: { dimensions: ['stage'], measures: ['revenue'], runtimeFilter: { region: 'NA' } }, + }, + code: 'INVALID_FIELD', + message: /cannot evaluate a cross-object filter \("account\.region"\)/, + }, + { + name: 'objectql-strategy: a non-recombinable measure over a cross-object dimension', + analytics: aggregateAnalytics, + body: { + dataset: joinedDataset, + selection: { dimensions: ['region'], measures: ['avg_deal'] }, + }, + code: 'INVALID_FIELD', + message: /cannot group by a cross-object dimension with a "avg" measure \("avg_deal"\)/, + }, + { + // Measured, against the guess this file first carried: a two-hop dataset + // dimension DOES reach the multi-hop arm. `joinAlias` flattens the JOIN's + // alias to `account__owner`, but the dimension's own `sql` stays the raw + // `account.owner.region`, which is what `planCrossObject` classifies. + name: 'objectql-strategy: a MULTI-HOP cross-object dimension', + analytics: aggregateAnalytics, + body: { + dataset: { + ...joinedDataset, + include: ['account.owner'], + dimensions: [{ name: 'owner_region', field: 'account.owner.region', type: 'string' }], + measures: [{ name: 'revenue', aggregate: 'sum', field: 'amount' }], + }, + selection: { dimensions: ['owner_region'], measures: ['revenue'] }, + }, + code: 'INVALID_FIELD', + message: /supports only single-hop cross-object dimensions; "account\.owner\.region"/, + }, + ]; + + for (const c of CASES) { + it(`${c.name} → 400 ${c.code} (was 500 ANALYTICS_QUERY_FAILED)`, async () => { + const route = buildRoute(async () => c.analytics()); + const res = await post(route, c.body); + + expect(res.statusCode).toBe(400); + expect(res.body.code).toBe(c.code); + // The message survives intact so the author can act on it, and the body is + // the 4xx shape (`message`), not the 5xx one (`error`). + expect(String(res.body.message)).toMatch(c.message); + expect(res.body.error).toBeUndefined(); + // 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'); + }); + } + + it('POSITIVE control (aggregate path): the same wiring, a valid selection → 200 with rows', async () => { + const route = buildRoute(async () => aggregateAnalytics()); + const res = await post(route, { dataset, selection }); + expect(res.statusCode).toBe(200); + expect(res.body.rows).toEqual([{ stage: 'won', revenue: 100 }]); + }); + + it('POSITIVE control (raw-SQL path): the declared measure still runs → 200 with rows', async () => { + // The twin of the undeclared-measure case: one character away, same wiring. + const route = buildRoute(async () => nativeAnalytics()); + const res = await post(route, { dataset, selection }); + expect(res.statusCode).toBe(200); + expect(res.body.rows).toEqual([{ stage: 'won', revenue: 100 }]); + }); + + it('POSITIVE control (cross-object, in envelope): an FK-expandable dimension → 200', async () => { + // What keeps the four `planCrossObject` cases verdicts about the ENVELOPE + // rather than about cross-object queries being rejected outright: a + // single-hop dimension with a recombinable measure is SERVED on this path. + const route = buildRoute(async () => aggregateAnalytics()); + const res = await post(route, { + dataset: joinedDataset, + selection: { dimensions: ['region'], measures: ['revenue'] }, + }); + expect(res.statusCode).toBe(200); + }); +}); + +describe('[#5716] the two sites this route cannot show, and why', () => { + it('an `include` past the hop limit is answered by the SCHEMA first — 400 VALIDATION_FAILED', async () => { + // The compiler's `MAX_JOIN_HOPS` refusal is enveloped too (its producer test + // drives it directly, bypassing the schema), but it is not reachable HERE: + // the route parses the document with `DatasetSchema`, which refines the same + // limit, so the caller gets the schema's 400 and the compiler never runs. + // Recorded rather than asserted as `DATASET_INVALID`, because manufacturing + // the expected code would mean asserting a path that does not exist — the + // observable fact is a 4xx either way, from the earliest layer that can tell. + const route = buildRoute(async () => aggregateAnalytics()); + const res = await post(route, { + dataset: { ...dataset, include: ['a.b.c.d'] }, + selection, + }); + + expect(res.statusCode).toBe(400); + expect(res.body.code).toBe('VALIDATION_FAILED'); + expect(String(res.body.detail)).toMatch(/3-hop limit/); + }); + + it('an undeclared MEASURE never reaches the strategy — `ensureCube` mints one, and the #4437 gate judges it', async () => { + // The twelfth site (`native-sql-strategy`'s "cube declares no measure") is + // enveloped, and its producer test drives it — but it cannot be shown from + // this route, and the measurement is worth writing down rather than faking: + // `AnalyticsService.ensureCube` AUGMENTS a registered cube with a + // suffix-inferred `Metric` for every measure name the query carries, so the + // strategy is handed a cube that declares `profit` and compiles + // `SUM(profit)`. Reaching the refusal means calling the strategy directly + // (`measure-expression-sql.test.ts` does). + // + // What answers on THIS face is the #4437 source-field gate, as soon as a + // field probe can say the object has no such column — and it answers + // `400 INVALID_FIELD`, the same code this PR gives the strategy's own + // refusal. That agreement is the point of the choice, so it is asserted + // rather than asserted about. + const route = buildRoute(async () => + nativeAnalytics({ getObjectFieldNames: () => ['stage', 'amount', 'close_date', 'created_at'] }), + ); + const res = await post(route, { dataset, selection: { dimensions: ['stage'], measures: ['profit_sum'] } }); + + expect(res.statusCode).toBe(400); + expect(res.body.code).toBe('INVALID_FIELD'); + expect(String(res.body.message)).toMatch(/aggregates field 'profit', which object 'crm_opportunity' does not have/); + }); +}); diff --git a/packages/services/service-analytics/src/__tests__/measure-source-field-gate.test.ts b/packages/services/service-analytics/src/__tests__/measure-source-field-gate.test.ts index d1312c4ee8..ac007a120a 100644 --- a/packages/services/service-analytics/src/__tests__/measure-source-field-gate.test.ts +++ b/packages/services/service-analytics/src/__tests__/measure-source-field-gate.test.ts @@ -212,7 +212,14 @@ describe('#4437 — measure source-field gate', () => { // reported as a missing one. Whether the query can run at all is the // strategy's call (the ObjectQL aggregate path declines cross-object // measures outright) and the join allowlist's (ADR-0021 D-C); either - // way the answer must not be this gate's INVALID_FIELD. + // way the answer must not be THIS GATE's verdict. + // + // [#5716] `code !== 'INVALID_FIELD'` was a PROXY for "not this gate", + // usable only while the strategy's refusal carried no code. It now + // carries `INVALID_FIELD` / 400 as well — the same class of mistake, one + // wire shape — so the proxy is replaced by what it stood for: the + // strategy's own message and metadata, and the ABSENCE of the `field` + // this gate sets on every verdict it makes. const joined: Cube = { name: 'joined_cube', title: 'Joined', @@ -227,13 +234,19 @@ describe('#4437 — measure source-field gate', () => { const err = await service .query({ cube: 'joined_cube', measures: ['remote_sum'] } as any) - .catch((e) => e as Error & { code?: string }); + .catch((e) => e as Error & { code?: string; field?: string; member?: string; param?: string }); expect(err).toBeInstanceOf(Error); - expect(err.code).not.toBe('INVALID_FIELD'); // It got as far as the strategy — i.e. past this gate — and was declined // there for the strategy's own declared reason. expect(err.message).toMatch(/cannot evaluate a cross-object measure/); + // The lie this case prevents: `balance` reported as a missing column of + // `showcase_invoice`. This gate always names one in `field`; the strategy + // never does. + expect(err.message).not.toMatch(/does not have/); + expect(err.field).toBeUndefined(); + expect(err.member).toBe('remote_sum'); + expect(err.param).toBe('measures'); }); it('stands down when no field probe is configured — nothing to consult', async () => { diff --git a/packages/services/service-analytics/src/__tests__/unlisted-refusal-envelope.test.ts b/packages/services/service-analytics/src/__tests__/unlisted-refusal-envelope.test.ts new file mode 100644 index 0000000000..4f8afc1d2f --- /dev/null +++ b/packages/services/service-analytics/src/__tests__/unlisted-refusal-envelope.test.ts @@ -0,0 +1,572 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#5716] The refusals that were never on anybody's list. + * + * ## What was wrong + * + * #5352 gave `/analytics/dataset/query` a message-substring list so six refusal + * families could answer 400, and #5367 retired five of those entries by giving + * the producers an ADR-0112 envelope. Both rounds worked from the same list — + * and the list was assembled from the refusals someone had already hit. + * + * Reading every `throw` in this package afterwards turned up TWELVE more throw + * sites — thirteen refusal conditions, since the cross-object measure and filter + * share one — of exactly the same kind: caller- or author-shaped refusals that + * never entered the regex at all, so no round ever touched them and they were + * answering `500 ANALYTICS_QUERY_FAILED` — "the platform is broken" — for a + * mistake in the request or in the dataset the caller wrote: + * + * ``` + * dataset-compiler.ts JOIN crosses datasources (#5115) · `include` + * relationship absent · `include` path over the hop limit + * dataset-executor.ts unparseable `dateRange` bound · compareTo names an + * undated dimension · compareTo with no dated window · + * compareTo ambiguous between two dated windows + * native-sql-strategy.ts cube declares no such measure (#4157) + * objectql-strategy.ts cross-object time bucket · cross-object measure · + * cross-object filter · multi-hop cross-object + * dimension · non-recombinable measure (planCrossObject) + * ``` + * + * ## The three blocks + * + * `the refusal SET is unchanged` pins WHICH inputs are refused and what each one + * says. Every assertion in it passes before AND after this change — that is its + * whole job, because "we only touched the envelope" is a claim worth being able + * to re-run. It matters more here than it did for #5367: five of these messages + * are asserted by #5923's tests, so a rewording would break another PR's pins. + * + * `every newly enveloped refusal carries…` is the change. Run it against + * pre-#5716 code and all thirteen fail with `code`/`status` `undefined`, while + * the block above stays green. + * + * `the verdicts that deliberately stay 500` is the fork, pinned rather than + * argued: `native-sql-strategy`'s "unrecognised type" was on #5716's list of + * nine as author-shaped, and it is not — see the case for the measurement. + * + * ## Reverse verification, direction predicted BEFORE running + * + * Ordinary direction, no inversion: revert any one producer to `throw new + * Error(…)` and that row of block 2 goes RED on `code`/`status` `undefined`, + * while its row in block 1 stays green (the message did not move) — which is + * exactly the asymmetry that made these thirteen invisible for two rounds. There + * is no "more diagnostics" or "inverted" shape available here: nothing counts + * verdicts, and no rule was narrowed. + * + * MEASURED with the four producer files reverted to `origin/main` (tests all + * kept): **22 red / 1053 green** across `@objectstack/service-analytics`, and the + * set is exactly the predicted one plus one correction worth recording: + * + * - 13 red — block 2's envelope rows; + * - 6 red — block 2's `member`/`param` rows (the member-level family only); + * - 3 red — the pins this PR RE-JUDGED in `where-source-field-gate.test.ts` + * (×2) and `measure-source-field-gate.test.ts` (×1), which now assert the + * strategy's envelope instead of `code !== 'INVALID_FIELD'`; + * - GREEN — all 13 rows of block 1, both cases of block 3, and — correcting + * this header's first draft — the `covers every site` ledger case, which + * reads the CASES table rather than running anything, so reverting a + * producer cannot move it. It fails on a site being ADDED or REMOVED from + * this file, which is the different thing it is for. + */ + +import { describe, it, expect, vi } from 'vitest'; +import { DatasetSchema } from '@objectstack/spec/ui'; +import type { Cube } from '@objectstack/spec/data'; +import type { + AnalyticsQuery, + AnalyticsResult, + DatasetSelection, + IAnalyticsService, + StrategyContext, +} from '@objectstack/spec/contracts'; +import { compileDataset } from '../dataset-compiler.js'; +import { DatasetExecutor } from '../dataset-executor.js'; +import { NativeSQLStrategy } from '../strategies/native-sql-strategy.js'; +import { ObjectQLStrategy } from '../strategies/objectql-strategy.js'; + +/** The ADR-0112 fields the REST boundary classifies on, plus the diagnostics. */ +interface Refusal extends Error { + code?: unknown; + status?: unknown; + member?: unknown; + param?: unknown; + cube?: unknown; + field?: unknown; +} + +async function refusalFrom(thunk: () => unknown | Promise): Promise { + try { + await thunk(); + return undefined; + } catch (e) { + return e as Refusal; + } +} + +// ── fixtures ───────────────────────────────────────────────────────────────── + +/** The base dataset every compiler case bends one property of. */ +const baseDataset = { + name: 'pipeline', + label: 'Pipeline', + object: 'crm_opportunity', + dimensions: [{ name: 'stage', field: 'stage', type: 'string' }], + measures: [{ name: 'revenue', aggregate: 'sum', field: 'amount' }], +}; + +/** A dataset with the three date dimensions the compareTo cases select from. */ +const datedDataset = DatasetSchema.parse({ + name: 'sales', + label: 'Sales', + object: 'opportunity', + dimensions: [ + { name: 'region', field: 'region', type: 'string' }, + { name: 'close_date', field: 'close_date', type: 'date' }, + { name: 'created_at', field: 'created_at', type: 'date' }, + ], + measures: [{ name: 'revenue', aggregate: 'sum', field: 'amount' }], +}); + +const WINDOW = ['2026-01-01', '2026-01-31'] as [string, string]; + +function fakeService(): IAnalyticsService { + return { + query: vi.fn(async (_q: AnalyticsQuery): Promise => ({ + rows: [{ region: 'NA', revenue: 100 }], + fields: [], + })), + getMeta: async () => [], + }; +} + +/** Run a selection over the dated dataset through the real executor. */ +function runSelection(selection: Record) { + return new DatasetExecutor(fakeService()).execute( + compileDataset(datedDataset), + selection as unknown as DatasetSelection, + ); +} + +/** + * A cube that JOINS — the shape every `planCrossObject` refusal needs, and the + * one an inferred single-table cube can never produce. `region` / `opened` / + * `remote_sum` resolve THROUGH the join; `stage` / `revenue` / `avg_deal` do + * not, which is what lets the same cube carry the accepting neighbours. + */ +const joinedCube: Cube = { + name: 'sales_by_account', + title: 'Sales by account', + sql: 'opportunity', + measures: { + revenue: { name: 'revenue', label: 'Revenue', type: 'sum', sql: 'amount' }, + avg_deal: { name: 'avg_deal', label: 'Avg deal', type: 'avg', sql: 'amount' }, + remote_sum: { name: 'remote_sum', label: 'Remote', type: 'sum', sql: 'account.balance' }, + }, + dimensions: { + stage: { name: 'stage', label: 'Stage', type: 'string', sql: 'stage' }, + region: { name: 'region', label: 'Region', type: 'string', sql: 'account.region' }, + opened: { name: 'opened', label: 'Opened', type: 'time', sql: 'account.created_at' }, + }, + joins: { + account: { name: 'account', relationship: 'many_to_one', sql: 'opportunity.account = account.id' }, + }, + public: false, +}; + +/** A cube declaring exactly ONE measure — so `revenue` is undeclared on it. */ +const countOnlyCube: Cube = { + name: 'pipeline', + title: 'Pipeline', + sql: 'crm_opportunity', + measures: { count: { name: 'count', label: 'Count', type: 'count', sql: '*' } }, + dimensions: { stage: { name: 'stage', label: 'Stage', type: 'string', sql: 'stage' } }, + public: false, +}; + +function ctxFor(cube: Cube): StrategyContext { + return { + getCube: (name: string) => (name === cube.name ? cube : undefined), + queryCapabilities: () => ({ nativeSql: true, objectqlAggregate: true, inMemory: false }), + executeRawSql: async () => [], + executeAggregate: async () => [], + getAllowedRelationships: () => new Set(['account']), + } as unknown as StrategyContext; +} + +const objectqlSql = (query: Record) => + new ObjectQLStrategy().generateSql(query as unknown as AnalyticsQuery, ctxFor(joinedCube)); + +// ── the twelve sites, one input each, through the real producer ────────────── + +interface Case { + name: string; + /** What the caller changed to trigger it — the audit trail for the set. */ + site: string; + message: RegExp; + code: 'DATASET_INVALID' | 'INVALID_FIELD'; + /** Only for the member-level family. */ + member?: string; + param?: string; + run: () => unknown | Promise; +} + +const CASES: Case[] = [ + { + name: '① dataset-compiler: an `include` JOIN that crosses datasources', + site: 'dataset-compiler.ts assertSameDatasource (#5115)', + message: /declares a JOIN that crosses datasources/, + code: 'DATASET_INVALID', + run: () => + compileDataset( + DatasetSchema.parse({ + ...baseDataset, + include: ['account'], + dimensions: [{ name: 'region', field: 'account.region', type: 'string' }], + }), + undefined, + { getObjectDatasource: (o: string) => (o === 'crm_opportunity' ? 'primary' : 'warehouse') }, + ), + }, + { + name: '② dataset-compiler: an `include` relationship the object does not have', + site: 'dataset-compiler.ts resolveHop', + message: /includes relationship "bogus" which does not exist on object "crm_opportunity"/, + code: 'DATASET_INVALID', + run: () => + compileDataset( + DatasetSchema.parse({ ...baseDataset, include: ['bogus'] }), + () => undefined, + ), + }, + { + name: '③ dataset-compiler: an `include` path past the hop limit', + site: 'dataset-compiler.ts MAX_JOIN_HOPS (ADR-0071)', + message: /include path "a\.b\.c\.d" exceeds the 3-hop limit \(4 hops\)/, + code: 'DATASET_INVALID', + // ⚠️ Measured while writing this file, and worth knowing before reading the + // verdict: `DatasetSchema` ALSO refines the hop limit, so on + // `/analytics/dataset/query` — which parses the document first — this shape + // never reaches the compiler; it answers `400 VALIDATION_FAILED` with + // "include path exceeds the 3-hop limit (ADR-0071)". What reaches the + // compiler is an UNPARSED dataset: a host calling `queryDataset` directly. + // Enveloping it is still right, and is the reason the schema bypass here is + // deliberate rather than lazy: the SAME authoring mistake must not answer 400 + // on the parsed path and 500 on the unparsed one. It is also not our own + // drift — nothing in the runtime synthesizes `include` — which is what keeps + // it out of the "internal invariant" tier its neighbour at + // `aggregateToMetricType` sits in. + run: () => compileDataset({ ...baseDataset, include: ['a.b.c.d'] } as never), + }, + { + name: '④ dataset-executor: a dateRange bound that is not a date', + site: 'dataset-executor.ts parseUTC', + message: /invalid date in dateRange: "the-first-of-never"/, + code: 'DATASET_INVALID', + run: () => + runSelection({ + dimensions: ['region'], + measures: ['revenue'], + timeDimensions: [{ dimension: 'close_date', dateRange: ['2026-01-01', 'the-first-of-never'] }], + compareTo: { kind: 'previousPeriod' }, + }), + }, + { + name: '⑤ dataset-executor: compareTo names a dimension with no dateRange', + site: 'dataset-executor.ts resolveCompareDimension (named)', + message: /compareTo requires a timeDimension "created_at" with a dateRange/, + code: 'DATASET_INVALID', + run: () => + runSelection({ + dimensions: ['region'], + measures: ['revenue'], + timeDimensions: [{ dimension: 'close_date', dateRange: WINDOW }], + compareTo: { kind: 'previousPeriod', dimension: 'created_at' }, + }), + }, + { + name: '⑥ dataset-executor: compareTo with no dated window at all', + site: 'dataset-executor.ts resolveCompareDimension (none)', + message: /compareTo needs a dated window to shift/, + code: 'DATASET_INVALID', + run: () => + runSelection({ + dimensions: ['region'], + measures: ['revenue'], + timeDimensions: [{ dimension: 'close_date', granularity: 'month' }], + compareTo: { kind: 'previousPeriod' }, + }), + }, + { + name: '⑦ dataset-executor: compareTo ambiguous between two dated windows', + site: 'dataset-executor.ts resolveCompareDimension (ambiguous)', + message: /compareTo\.dimension is ambiguous: 2 time dimensions carry a dateRange/, + code: 'DATASET_INVALID', + run: () => + runSelection({ + dimensions: ['region'], + measures: ['revenue'], + timeDimensions: [ + { dimension: 'close_date', dateRange: WINDOW }, + { dimension: 'created_at', dateRange: WINDOW }, + ], + compareTo: { kind: 'previousPeriod' }, + }), + }, + { + name: '⑧ native-sql-strategy: a measure the cube does not declare', + site: 'native-sql-strategy.ts resolveMeasureSql (#4157)', + message: /cube "pipeline" declares no measure "revenue" \(declared: count\)/, + code: 'INVALID_FIELD', + member: 'revenue', + param: 'measures', + // Driven at the STRATEGY, like `measure-expression-sql.test.ts`, and + // deliberately: measured, `AnalyticsService.ensureCube` augments a + // registered cube with a suffix-inferred `Metric` for every measure the + // query names, so no request reaching the service arrives here with an + // undeclared one — what answers on that path is the #4437 gate, with the + // SAME `INVALID_FIELD` / 400 (asserted end-to-end in `packages/rest`'s + // `analytics-dataset-unlisted-refusal-envelope.test.ts`). That agreement is + // why this site is `invalidMemberError` and not `datasetInvalidError`. + run: () => + new NativeSQLStrategy().generateSql( + { cube: 'pipeline', measures: ['revenue'], timezone: 'UTC' }, + ctxFor(countOnlyCube), + ), + }, + { + name: '⑨ objectql-strategy: bucketing a cross-object time dimension', + site: 'objectql-strategy.ts planCrossObject (time bucket)', + message: /cannot bucket a cross-object time dimension \("account\.created_at"\)/, + code: 'INVALID_FIELD', + member: 'opened', + param: 'timeDimensions', + run: () => + objectqlSql({ + cube: 'sales_by_account', + measures: ['revenue'], + timeDimensions: [{ dimension: 'opened', granularity: 'month' }], + }), + }, + { + name: '⑩ objectql-strategy: a cross-object MEASURE', + site: 'objectql-strategy.ts planCrossObject (measure)', + message: /cannot evaluate a cross-object measure \("account\.balance"\)/, + code: 'INVALID_FIELD', + member: 'remote_sum', + param: 'measures', + run: () => objectqlSql({ cube: 'sales_by_account', measures: ['remote_sum'] }), + }, + { + name: '⑪ objectql-strategy: a cross-object FILTER', + site: 'objectql-strategy.ts planCrossObject (filter)', + message: /cannot evaluate a cross-object filter \("account\.region"\)/, + code: 'INVALID_FIELD', + member: 'account.region', + param: 'where', + run: () => + objectqlSql({ + cube: 'sales_by_account', + measures: ['revenue'], + where: { 'account.region': 'NA' }, + }), + }, + { + name: '⑫ objectql-strategy: a MULTI-HOP cross-object dimension', + site: 'objectql-strategy.ts planCrossObject (multi-hop)', + message: /supports only single-hop cross-object dimensions; "account\.owner\.region"/, + code: 'INVALID_FIELD', + member: 'account.owner.region', + param: 'dimensions', + run: () => + objectqlSql({ + cube: 'sales_by_account', + measures: ['revenue'], + dimensions: ['account.owner.region'], + }), + }, + { + name: '⑬ objectql-strategy: a non-recombinable measure over a cross-object dimension', + site: 'objectql-strategy.ts planCrossObject (recombination)', + message: /cannot group by a cross-object dimension with a "avg" measure \("avg_deal"\)/, + code: 'INVALID_FIELD', + member: 'avg_deal', + param: 'measures', + run: () => + objectqlSql({ + cube: 'sales_by_account', + measures: ['avg_deal'], + dimensions: ['region'], + }), + }, +]; + +// ───────────────────────────────────────────────────────────────────────────── + +describe('[#5716] the refusal SET is unchanged — only the error shape moved', () => { + for (const c of CASES) { + it(`still REFUSES, with the same words: ${c.name}`, async () => { + const err = await refusalFrom(c.run); + expect(err, `${c.name} was accepted — the refusal set moved`).toBeInstanceOf(Error); + // The wording is load-bearing beyond readability: #5923's tests assert the + // planCrossObject messages, and #5717 tracks one of them (case ②) for + // colliding with `isMissingSourceError`'s sniffer. Enveloping must not + // reword anything. + expect(String(err?.message)).toMatch(c.message); + }); + } + + it('the ACCEPTING neighbours still compile — no refusal widened', async () => { + // One input a character away from each refused one, per producer. + // ①/② — the same joins, placed on one datasource and resolvable. + expect( + compileDataset( + DatasetSchema.parse({ + ...baseDataset, + include: ['account'], + dimensions: [{ name: 'region', field: 'account.region', type: 'string' }], + }), + undefined, + { getObjectDatasource: () => 'primary' }, + ).allowedRelationships.has('account'), + ).toBe(true); + // ③ — three hops is the limit, not one past it (and the schema agrees: this + // one parses, the four-hop one above does not). + expect( + compileDataset(DatasetSchema.parse({ ...baseDataset, include: ['a.b.c'] })).cube.name, + ).toBe('pipeline'); + // ④–⑦ — one dated window, parseable, unambiguous: the compare pass runs. + const compared = await runSelection({ + dimensions: ['region'], + measures: ['revenue'], + timeDimensions: [{ dimension: 'close_date', dateRange: WINDOW }], + compareTo: { kind: 'previousPeriod' }, + }); + expect(compared.rows[0]).toMatchObject({ region: 'NA', revenue: 100 }); + // ⑧ — the measure the cube DOES declare. + const counted = await new NativeSQLStrategy().generateSql( + { cube: 'pipeline', measures: ['count'], timezone: 'UTC' }, + ctxFor(countOnlyCube), + ); + expect(counted.sql).toContain('COUNT(*) AS "count"'); + // ⑨–⑬ — the in-envelope cross-object shape: a single-hop dimension with a + // recombinable measure, plus a purely base-object query. + const expanded = await objectqlSql({ + cube: 'sales_by_account', + measures: ['revenue'], + dimensions: ['region'], + }); + expect(expanded.sql).toContain('account'); + const base = await objectqlSql({ + cube: 'sales_by_account', + measures: ['avg_deal'], + dimensions: ['stage'], + }); + expect(base.sql).toContain('AVG'); + }); +}); + +describe('[#5716] every newly enveloped refusal carries the ADR-0112 envelope', () => { + for (const c of CASES) { + it(`${c.name} → ${c.code} / 400`, async () => { + const err = await refusalFrom(c.run); + expect(err).toBeInstanceOf(Error); + // Read exactly as `rest-server.ts`'s catch reads them: a 4xx status AND a + // code, or the route falls through to 500 ANALYTICS_QUERY_FAILED. + expect(err?.code, 'no `code` ⇒ 500 ANALYTICS_QUERY_FAILED').toBe(c.code); + expect(err?.status, 'no `status` ⇒ 500 ANALYTICS_QUERY_FAILED').toBe(400); + }); + } + + for (const c of CASES.filter((x) => x.member)) { + it(`${c.name} → names the member the REQUEST spelled`, async () => { + const err = await refusalFrom(c.run); + expect(err?.member).toBe(c.member); + expect(err?.param).toBe(c.param); + expect(err?.cube).toBeTruthy(); + // Never `field`: these sites either have no such column (an undeclared + // measure) or the column is real and works on another driver (a + // cross-object member). The three shipped gates set `field` because they + // DO resolve one; inventing it here would be a fact we do not have. + expect(err?.field).toBeUndefined(); + }); + } + + it('covers every site #5716 enumerated, and splits them by verdict', () => { + // The audit trail: nine sites from the issue's own table, minus the one + // re-judged below, plus the five-refusal `planCrossObject` family the PM + // merged in on 2026-08-06 (four throws — measure and filter share one). + expect(CASES.filter((c) => c.code === 'DATASET_INVALID').map((c) => c.site)).toEqual([ + 'dataset-compiler.ts assertSameDatasource (#5115)', + 'dataset-compiler.ts resolveHop', + 'dataset-compiler.ts MAX_JOIN_HOPS (ADR-0071)', + 'dataset-executor.ts parseUTC', + 'dataset-executor.ts resolveCompareDimension (named)', + 'dataset-executor.ts resolveCompareDimension (none)', + 'dataset-executor.ts resolveCompareDimension (ambiguous)', + ]); + expect(CASES.filter((c) => c.code === 'INVALID_FIELD').map((c) => c.site)).toEqual([ + 'native-sql-strategy.ts resolveMeasureSql (#4157)', + 'objectql-strategy.ts planCrossObject (time bucket)', + 'objectql-strategy.ts planCrossObject (measure)', + 'objectql-strategy.ts planCrossObject (filter)', + 'objectql-strategy.ts planCrossObject (multi-hop)', + 'objectql-strategy.ts planCrossObject (recombination)', + ]); + }); +}); + +describe('[#5716] the verdicts that deliberately stay an undeclared 500', () => { + it('native-sql-strategy: an unrecognised metric type — a FORK against the issue list', async () => { + // #5716's table has this down as "cube / dataset author". It is not, and the + // measurement is three-sided: + // + // - `Metric.type` is the CLOSED `AggregationMetricType` enum, and + // `metric-type-coverage.test.ts` pins that the strategy's aggregate and + // expression sets PARTITION it — its second case is literally "leaves no + // metric type to the unrecognised-type throw"; + // - `dataset-compiler` writes only a `SUPPORTED_AGGREGATES` member into a + // cube (and refuses the other two aggregates with `DATASET_INVALID` + // first), so no DATASET can produce one; + // - `inferMeasure` mints six known types, so no ad-hoc cube can either. + // + // So reaching it needs a cube that never met `CubeSchema` — our own drift, or + // a host registering an off-spec object programmatically. Blaming the caller + // with a 400 would hide a platform bug from ops alerting and tell a dashboard + // user to fix metadata they cannot see. It stays UNDECLARED (no `code`) so + // #5667's tiering keeps it readable in the response, like the compiler + // invariant it sits beside. + const offSpec = { + ...countOnlyCube, + measures: { median_deal: { name: 'median_deal', label: 'Median', type: 'median', sql: 'amount' } }, + } as unknown as Cube; + + const err = await refusalFrom(() => + new NativeSQLStrategy().generateSql( + { cube: 'pipeline', measures: ['median_deal'], timezone: 'UTC' }, + ctxFor(offSpec), + ), + ); + + expect(String(err?.message)).toMatch(/has unrecognised type "median"/); + expect(err?.code).toBeUndefined(); + expect(err?.status).toBeUndefined(); + }); + + it('the two "Cube not found" guards stay bare too — the strategies cannot be reached without one', async () => { + // Named here so the fork above is not read as "everything else moved": + // `ctx.getCube` returning nothing means the registration the service does + // before dispatching a strategy did not happen. That is ours, not the + // caller's — a caller naming an unknown cube is stopped much earlier by + // #3867's `CUBE_NOT_FOUND` / 404 gate. + for (const run of [ + () => new NativeSQLStrategy().generateSql({ cube: 'ghost', measures: ['count'], timezone: 'UTC' }, ctxFor(countOnlyCube)), + () => new ObjectQLStrategy().generateSql({ cube: 'ghost', measures: ['count'] } as AnalyticsQuery, ctxFor(joinedCube)), + ]) { + const err = await refusalFrom(run); + expect(String(err?.message)).toMatch(/Cube not found: ghost/); + expect(err?.code).toBeUndefined(); + expect(err?.status).toBeUndefined(); + } + }); +}); 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 index 6936aa78fb..d3aac4e0f5 100644 --- 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 @@ -137,7 +137,9 @@ async function rejection(call: Prom } /** How a call settled: the error it rejected with, or `{}` when it resolved. */ -async function settle(call: Promise): Promise<{ code?: string; message?: string }> { +async function settle( + call: Promise, +): Promise<{ code?: string; message?: string; field?: string; member?: string; param?: string }> { try { await call; return {}; @@ -583,10 +585,19 @@ describe('#5669 — what the gate must NOT do', () => { // 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 + // answer must not be THIS GATE's verdict. Measured on both // strategies: NativeSQL joins it // (`LEFT JOIN "owner" … WHERE "owner"."region" = $1`), ObjectQL declines it // with its own cross-object message. + // + // [#5716] The assertion used to be `code !== 'INVALID_FIELD'`, which was a + // PROXY for "not this gate" — available only while the strategy's own + // refusal carried no code at all. It now carries `INVALID_FIELD` / 400 too + // (both name a member the request cannot be served with; one wire shape + // for one class of mistake), so the proxy is dead and is replaced by the + // facts it stood for: the message is the strategy's, and the gate's own + // `field` — the base column it reports as missing, set on every one of its + // three verdicts and on none of the strategies' — is absent. const joined: Cube = { name: 'joined_cube', title: 'Joined', @@ -601,7 +612,14 @@ describe('#5669 — what the gate must NOT do', () => { 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"\)/); + // The lie this case exists to prevent, asserted as itself. + expect(settled.message).not.toMatch(/constrains field 'region'/); + expect(settled.field).toBeUndefined(); + // …and positively, the strategy's envelope: the member as the request + // spelled it, under the request key it was written on. + expect(settled.member).toBe('owner.region'); + expect(settled.param).toBe('where'); }); it('reads a NESTED relation filter as the same dotted member the strategies do', async () => { @@ -626,8 +644,14 @@ describe('#5669 — what the gate must NOT do', () => { service.query({ cube: 'joined_cube', measures: ['count'], where: { owner: { region: 'NA' } } } as any), ); - expect(settled.code).not.toBe('INVALID_FIELD'); + // [#5716] Same substitution as the case above: `code !== 'INVALID_FIELD'` + // no longer separates the two producers, `field` does. expect(settled.message).toMatch(/cross-object filter \("owner\.region"\)/); + expect(settled.message).not.toMatch(/constrains field 'owner'/); + expect(settled.field).toBeUndefined(); + // Both spellings reach the SAME refusal, envelope included — which is the + // invariant this case is really about. + expect(settled.member).toBe('owner.region'); }); it('stands down for a dotted member on the INFERENCE path, exactly as the shipped dimension gate does', async () => { diff --git a/packages/services/service-analytics/src/dataset-compiler.ts b/packages/services/service-analytics/src/dataset-compiler.ts index 627dcbdc0b..e476ae7a3b 100644 --- a/packages/services/service-analytics/src/dataset-compiler.ts +++ b/packages/services/service-analytics/src/dataset-compiler.ts @@ -234,7 +234,13 @@ export function compileDataset( const targetDatasource = declaredDatasource(targetObject); if (!targetDatasource) return; // cannot answer for this target if (sameDatasource(targetDatasource, baseDatasource)) return; - throw new Error( + // [#5716] `DATASET_INVALID` / 400 — the AUTHOR's verdict, decided entirely + // from metadata before any query runs: the dataset's `include` path and the + // two objects' `datasource` bindings. Both are things the author (or an + // admin) can change, and the message already names both fixes. Nothing here + // is a runtime fault, so a 500 told the author "the platform is broken" + // about a document they wrote. + throw datasetInvalidError( `[dataset-compiler] dataset "${dataset.name}" declares a JOIN that crosses datasources: ` + `its base object "${dataset.object}" is on datasource "${baseDatasource}", but the joined ` + `object "${targetObject}" — reached via the \`include\` path "${path}" — is on datasource ` + @@ -257,7 +263,22 @@ export function compileDataset( if (!resolver) return { object: rel, table: rel }; const resolved = resolver(fromObject, rel); if (!resolved) { - throw new Error( + // [#5716] `DATASET_INVALID` / 400 — the dataset's own `include` names a + // relationship the object graph does not have: a typo or a stale dataset, + // fixable only by the author. Sibling of the already-enveloped "…is not + // declared in the dataset's `include`" (#5367), one step earlier in the + // same resolution. + // + // ⚠️ The WORDING is left exactly as it was, and that is not an oversight: + // it contains both "relation"(ship) and "does not exist", so + // `analytics-service.ts`'s `isMissingSourceError` matches it — the mine + // #5717 filed. Enveloping it does not disarm that sniffer (it reads the + // message, not the envelope) and does not arm it either (this throw is + // still OUTSIDE `queryDataset`'s try, which is the only reason the mine + // has never gone off). What it DOES do is make #5717's option B — "never + // degrade an error that declares a 4xx envelope to an empty result" — + // able to cover this site, which before today it could not. + throw datasetInvalidError( `[dataset-compiler] dataset "${dataset.name}" includes relationship "${rel}" ` + `which does not exist on object "${fromObject}".`, ); @@ -268,7 +289,12 @@ export function compileDataset( for (const path of include) { const segments = path.split('.'); if (segments.length > MAX_JOIN_HOPS) { - throw new Error( + // [#5716] `DATASET_INVALID` / 400 — a limit of the v1 runtime, reported + // against a path the author wrote. Same family as the aggregate refusal a + // few lines up ("not supported by the v1 dataset runtime", #5367): what + // the runtime cannot do is stated as a property of the dataset, because + // that is what the author has to change. + throw datasetInvalidError( `[dataset-compiler] dataset "${dataset.name}" include path "${path}" exceeds the ` + `${MAX_JOIN_HOPS}-hop limit (${segments.length} hops). Deeper traversal is not supported.`, ); diff --git a/packages/services/service-analytics/src/dataset-executor.ts b/packages/services/service-analytics/src/dataset-executor.ts index a63703ca8e..45f30231df 100644 --- a/packages/services/service-analytics/src/dataset-executor.ts +++ b/packages/services/service-analytics/src/dataset-executor.ts @@ -463,7 +463,14 @@ export function resolveOrdering( function parseUTC(date: string): number { // Accepts 'YYYY-MM-DD' (and ISO datetimes); interpreted as UTC. const ms = Date.parse(date.length === 10 ? `${date}T00:00:00Z` : date); - if (Number.isNaN(ms)) throw new Error(`[dataset-executor] invalid date in dateRange: "${date}"`); + // [#5716] `DATASET_INVALID` / 400 — the string comes from the REQUEST + // (`selection.timeDimensions[].dateRange`, usually a dashboard's date filter), + // reaches here only through `shiftRange`'s `compareTo` math, and no schema + // refines it into a date. A caller who sends an unparseable bound gets told + // which bound it was; nothing about it is a server fault. + if (Number.isNaN(ms)) { + throw datasetInvalidError(`[dataset-executor] invalid date in dateRange: "${date}"`); + } return ms; } @@ -508,7 +515,15 @@ function resolveCompareDimension(selection: DatasetSelection): string { if (cmp.dimension != null) { if (!names.includes(cmp.dimension)) { - throw new Error( + // [#5716] `DATASET_INVALID` / 400 for all three refusals in this function. + // Every one of them is a verdict about the SELECTION as a whole — which + // `timeDimensions` carry a `dateRange`, and whether `compareTo` can pick + // one — so it is `datasetInvalidError`, not the member-level + // `invalidMemberError`: the fix is to add a window or drop `compareTo`, + // not to correct a misspelled member. Same request, same document, same + // family as the order-key and totals refusals already enveloped here + // (#5367). + throw datasetInvalidError( `[dataset-executor] compareTo requires a timeDimension "${cmp.dimension}" with a dateRange. ` + (names.length > 0 ? `This selection dates ${names.map((n) => `"${n}"`).join(', ')} — name one of those, or omit compareTo.dimension to let the executor choose when there is only one.` @@ -521,7 +536,7 @@ function resolveCompareDimension(selection: DatasetSelection): string { if (names.length === 1) return names[0]; if (names.length === 0) { - throw new Error( + throw datasetInvalidError( '[dataset-executor] compareTo needs a dated window to shift, but this selection declares no ' + 'timeDimension with a dateRange. Give the time dimension a dateRange (a dashboard date-range ' + 'filter is the usual source), or drop compareTo — a period-over-period comparison is only ' @@ -529,7 +544,7 @@ function resolveCompareDimension(selection: DatasetSelection): string { ); } - throw new Error( + throw datasetInvalidError( `[dataset-executor] compareTo.dimension is ambiguous: ${names.length} time dimensions carry a ` + `dateRange (${names.map((n) => `"${n}"`).join(', ')}). Name the one to shift — ` + `compareTo: { kind: '${cmp.kind}', dimension: '${names[0]}' }.`, diff --git a/packages/services/service-analytics/src/dataset-refusal.ts b/packages/services/service-analytics/src/dataset-refusal.ts index 91c34baeed..803e82a6d1 100644 --- a/packages/services/service-analytics/src/dataset-refusal.ts +++ b/packages/services/service-analytics/src/dataset-refusal.ts @@ -42,6 +42,36 @@ * it is what makes an unregistered code a compile error instead of a string that * only fails when some route happens to parse its own response body. * + * ## [#5716] The second constructor, and how to choose between them + * + * #5352 named six refusal families and #5367 enveloped five of them. Reading + * every `throw` in this package afterwards turned up NINE more sites of exactly + * the same kind — caller- or author-shaped refusals that never entered the + * route's message list at all, so they were answering `500` with nobody's + * regex to rescue them — plus the `objectql-strategy.ts` `planCrossObject` + * family (PM ruling on #5716, 2026-08-06). What decides the CODE is not which + * file throws but what the refusal is a verdict ABOUT: + * + * - {@link datasetInvalidError} — a verdict about the DATASET or the whole + * SELECTION: an `include` path that cannot be joined, an aggregate v1 cannot + * lower, a `compareTo` with no window to shift, a `dateRange` that is not a + * date. The caller fixes the dataset definition or the selection. + * - {@link invalidMemberError} — a verdict about ONE MEMBER the request named: + * a measure the cube does not declare, a member this engine cannot join to. + * The caller fixes (or drops) that member. + * + * The member family is `INVALID_FIELD` / 400 rather than a second + * `DATASET_INVALID` for two measured reasons. First, the three shipped analytics + * gates already answer `INVALID_FIELD` / 400 to the NEIGHBOURING member-level + * mistakes on the very same request keys — `measures` (#4437), `dimensions` / + * `timeDimensions` (#5520), `where` (#5669) — so a caller who mistypes a member + * and a caller who names one the engine cannot serve would otherwise get two + * wire shapes for one class of mistake, which is the defect ADR-0112 exists to + * remove. Second, these sites are NOT dataset-only: `planCrossObject` and the + * undeclared-measure refusal fire on `/analytics/query` too, where there is no + * dataset at all — `DATASET_INVALID` would name a document the caller never + * sent, while `INVALID_FIELD` reads correctly on both faces. + * * ## What deliberately does NOT go through here * * Not every `throw` in this package is the caller's mistake, and enveloping one @@ -62,7 +92,16 @@ * has no aggregate", which the spec refinement already guarantees. An * arrival there is our bug; an undeclared `500` is the honest answer, and * staying bare keeps it readable in the response (#5667's tiering) instead of - * withheld like a declared server fault. + * withheld like a declared server fault. [#5716] `native-sql-strategy.ts`'s + * "measure … has unrecognised type" joins this bullet after measurement, and + * against #5716's own list, which had it down as author-shaped: `Metric.type` + * is the CLOSED `AggregationMetricType` enum, `metric-type-coverage.test.ts` + * pins that every member of it is handled (its second case is literally "leaves + * no metric type to the unrecognised-type throw"), the dataset compiler maps + * only `SUPPORTED_AGGREGATES` into a cube, and `inferMeasure` mints six known + * types. So no spec-valid cube can reach it — an arrival is our own drift or a + * host registering an unparsed cube object, which is the same 500 tier as the + * line above, not the author's 400. * - **Producer/consumer drift between two of OUR tables** — the posture * `objectql-strategy.ts`'s display-SQL renderer already states explicitly * ("Deliberately NOT `invalidFilterError`'s 400 envelope: this is drift @@ -74,7 +113,7 @@ * refuses **the caller**. */ -import type { RegisteredErrorCode } from '@objectstack/spec/api'; +import type { RegisteredErrorCode, StandardErrorCode } from '@objectstack/spec/api'; /** * `DATASET_INVALID`, pinned against the ledger. @@ -84,6 +123,21 @@ import type { RegisteredErrorCode } from '@objectstack/spec/api'; */ const DATASET_INVALID: RegisteredErrorCode = 'DATASET_INVALID'; +/** + * [#5716] `INVALID_FIELD`, pinned against the STANDARD catalog. + * + * Same load-bearing annotation as `DATASET_INVALID` above, one tier over: this + * code is platform-wide (`StandardErrorCode`), not registered per package, which + * is precisely why the member-level refusals use it — see the module header. + */ +const INVALID_FIELD: StandardErrorCode = 'INVALID_FIELD'; + +/** + * [#5716] Which request key named the member — the analytics vocabulary, spelled + * exactly as the shipped source-field gates spell it in `err.param`. + */ +export type AnalyticsRequestKey = 'measures' | 'dimensions' | 'timeDimensions' | 'where'; + /** * A dataset refusal in the ADR-0112 envelope — `DATASET_INVALID` / 400. * @@ -99,3 +153,44 @@ export function datasetInvalidError(message: string): Error { err.status = 400; return err; } + +/** + * [#5716] A refusal about ONE MEMBER the request named — `INVALID_FIELD` / 400. + * + * Use it when the verdict is about a single `measures` / `dimensions` / + * `timeDimensions` / `where` entry rather than about the dataset or the whole + * selection: a measure the cube does not declare (#4157), a member this engine + * cannot evaluate because it traverses a relationship the driver cannot join + * (`planCrossObject`). The message stays whatever the refusing site says — every + * one of these already names the member and how to fix it, and #5923's tests + * assert that wording. + * + * `member` is the entry AS THE REQUEST SPELLED IT — `revenue`, not the + * `account.balance` it resolved to — because that is the string the caller can + * find in the body they sent; the resolved form stays in the message, which is + * where the explanation lives. `member` / `param` / `cube` mirror the diagnostic + * fields the three shipped gates attach + * (`err.field`/`err.param`/`err.measure`…). `field` is deliberately + * NOT among them: those gates resolve a member to a base COLUMN and name the + * column that is missing, while here either there is no such column (an + * undeclared measure) or the column exists and is perfectly fine on another + * driver (a cross-object member). Naming one would be inventing a fact. + */ +export function invalidMemberError( + message: string, + meta: { member: string; param?: AnalyticsRequestKey; cube?: string }, +): Error { + const err = new Error(message) as Error & { + code?: string; + status?: number; + member?: string; + param?: string; + cube?: string; + }; + err.code = INVALID_FIELD; + err.status = 400; + err.member = meta.member; + if (meta.param) err.param = meta.param; + if (meta.cube) err.cube = meta.cube; + return err; +} diff --git a/packages/services/service-analytics/src/strategies/native-sql-strategy.ts b/packages/services/service-analytics/src/strategies/native-sql-strategy.ts index bf3f4d5dbc..8356e10bac 100644 --- a/packages/services/service-analytics/src/strategies/native-sql-strategy.ts +++ b/packages/services/service-analytics/src/strategies/native-sql-strategy.ts @@ -11,7 +11,7 @@ import { type NormalizedFilterNode, } from './filter-normalizer.js'; import { compileScopedFilterToSql } from '../read-scope-sql.js'; -import { datasetInvalidError } from '../dataset-refusal.js'; +import { datasetInvalidError, invalidMemberError } from '../dataset-refusal.js'; import { likePattern, LIKE_ESCAPE_CHAR, type LikeShape } from '../like-pattern.js'; import { nextUtcCalendarDay } from '@objectstack/core'; @@ -465,9 +465,17 @@ export class NativeSQLStrategy implements AnalyticsStrategy { // for revenue and got a row count, aliased AS "revenue". #4157. if (!measure) { const declared = Object.keys(cube.measures ?? {}); - throw new Error( + // [#5716] `INVALID_FIELD` / 400, naming the member — the request's + // `measures` entry is the only input, and #4437's gate already answers + // exactly this code for the measure one character away (a measure whose + // SOURCE FIELD the object lacks). Two spellings of "your `measures` entry + // is wrong" must not get two wire shapes. `DATASET_INVALID` would be wrong + // on the other face this fires on: `/analytics/query` names a cube, not a + // dataset. + throw invalidMemberError( `[native-sql-strategy] cube "${cube.name}" declares no measure "${member}"` + (declared.length ? ` (declared: ${declared.join(', ')})` : ' (it declares none)'), + { member, param: 'measures', cube: cube.name }, ); } @@ -484,6 +492,18 @@ export class NativeSQLStrategy implements AnalyticsStrategy { // keep; silently substituting `COUNT(*)` did not keep it for them. if (EXPRESSION_METRIC_TYPES.has(measure.type)) return col; + // [#5716] Deliberately BARE — an undeclared 500, and the one site on that + // issue's list of nine that is NOT the author's mistake. `Metric.type` is the + // CLOSED `AggregationMetricType` enum; `metric-type-coverage.test.ts` pins + // that {@link AGGREGATE_SQL} ∪ {@link EXPRESSION_METRIC_TYPES} partitions it + // exactly, `dataset-compiler` only ever writes a `SUPPORTED_AGGREGATES` + // member into a cube, and `inferMeasure` mints six known types. So no + // spec-valid cube can arrive here: what does is our own drift or a host + // registering a cube object that never met `CubeSchema`. Answering the + // CALLER 400 for that would hide a platform bug from ops alerting and tell a + // dashboard user to fix metadata they cannot see. Same tier as + // `dataset-compiler`'s "non-derived measure has no aggregate"; the reasoning + // is written once in `dataset-refusal.ts`'s header. throw new Error( `[native-sql-strategy] measure "${member}" on cube "${cube.name}" has ` + `unrecognised type "${measure.type}" — expected an aggregate ` + diff --git a/packages/services/service-analytics/src/strategies/objectql-strategy.ts b/packages/services/service-analytics/src/strategies/objectql-strategy.ts index 22af5b80e3..e7e53d95da 100644 --- a/packages/services/service-analytics/src/strategies/objectql-strategy.ts +++ b/packages/services/service-analytics/src/strategies/objectql-strategy.ts @@ -11,6 +11,7 @@ import { type NormalizedFilterNode, } from './filter-normalizer.js'; import { compileScopedFilterToSql } from '../read-scope-sql.js'; +import { invalidMemberError } from '../dataset-refusal.js'; import { likePattern, LIKE_ESCAPE_CHAR, type LikeShape } from '../like-pattern.js'; import { nextUtcCalendarDay } from '@objectstack/core'; import { @@ -433,6 +434,17 @@ export class ObjectQLStrategy implements AnalyticsStrategy { * cannot be merged). A loud error beats the silent mis-bucket #3654 kills. * `generateSql()` calls this too, so the preview accepts/rejects the same set. * + * [#5716] All four refusals below are `invalidMemberError` — `INVALID_FIELD` / + * 400, naming the member — and the MESSAGES are unchanged (they are good + * diagnostics, and #5923's tests read them). Each is decided by two caller-side + * facts and nothing else: a member the query named, and whether that member + * resolves across a join. Neither is an internal invariant — a cube where the + * member exists and a driver that could serve it are both perfectly ordinary, + * which is exactly what the "run this on a native-SQL driver" half of each + * message says. They are member-level rather than dataset-level (hence not + * `datasetInvalidError`) because the fix is always to change or drop ONE named + * member, and because they fire on `/analytics/query` where no dataset exists. + * * Detection is on RESOLVED field names, so a dotted dimension the cube * flattens to a real column is treated as base, not cross-object. */ @@ -450,22 +462,37 @@ export class ObjectQLStrategy implements AnalyticsStrategy { for (const td of query.timeDimensions ?? []) { const field = this.resolveFieldName(cube, td.dimension, 'dimension'); if (this.isCrossObjectField(cube, field, baseObject)) { - throw new Error( + throw invalidMemberError( `[Analytics] ObjectQLStrategy cannot bucket a cross-object time dimension ("${field}").`, + { member: td.dimension, param: 'timeDimensions', cube: cube.name }, ); } } // A cross-object MEASURE or FILTER can only be evaluated with a real join. + // [#5716] `member` is the entry AS THE REQUEST SPELLED IT (`revenue`), which + // is what a caller can act on; `field` is what it RESOLVED to + // (`account.balance`), which is what the message explains the refusal with. + // The measure's request spelling used to be dropped here — the map kept only + // the resolved field — so the envelope had nothing to name. const nonDim = [ - ...(query.measures ?? []).map((m) => ({ where: 'measure', field: this.resolveMeasureAggregation(cube, m).field })), - ...Object.keys(filter).map((f) => ({ where: 'filter', field: f })), + ...(query.measures ?? []).map((m) => ({ + where: 'measure', member: m, field: this.resolveMeasureAggregation(cube, m).field, + })), + ...Object.keys(filter).map((f) => ({ where: 'filter', member: f, field: f })), ].filter((r) => this.isCrossObjectField(cube, r.field, baseObject)); if (nonDim.length > 0) { - throw new Error( + throw invalidMemberError( `[Analytics] ObjectQLStrategy cannot evaluate a cross-object ${nonDim[0].where} ` + `("${nonDim[0].field}") — the engine cannot join in an aggregate. Run this ` + `query on a native-SQL driver, or remove the cross-object ${nonDim[0].where}.`, + { + member: nonDim[0].member, + // The two kinds share one throw, so the request key follows the kind + // rather than being guessed by the reader of the message. + param: nonDim[0].where === 'measure' ? 'measures' : 'where', + cube: cube.name, + }, ); } @@ -477,9 +504,10 @@ export class ObjectQLStrategy implements AnalyticsStrategy { const [alias, ...rest] = field.split('.'); const attr = rest.join('.'); if (attr.includes('.')) { - throw new Error( + throw invalidMemberError( `[Analytics] ObjectQLStrategy supports only single-hop cross-object ` + `dimensions; "${field}" traverses more than one relationship.`, + { member: dim, param: 'dimensions', cube: cube.name }, ); } crossDims.push({ outputName: dim, fkField: alias, attr, refObject: cube.joins?.[alias]?.name ?? alias }); @@ -491,11 +519,12 @@ export class ObjectQLStrategy implements AnalyticsStrategy { for (const m of query.measures ?? []) { const { method } = this.resolveMeasureAggregation(cube, m); if (!RECOMBINABLE_METHODS.has(method)) { - throw new Error( + throw invalidMemberError( `[Analytics] ObjectQLStrategy cannot group by a cross-object dimension ` + `with a "${method}" measure ("${m}") — its value cannot be recombined ` + `across the intermediate FK grouping. Use sum/count/min/max, or run on ` + `a native-SQL driver.`, + { member: m, param: 'measures', cube: cube.name }, ); } }