From 11ccde2f28995c19b266c7d5419fda7a1775c64b Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 6 Aug 2026 01:44:40 +0000 Subject: [PATCH] fix(analytics,rest): envelope five dataset refusals as DATASET_INVALID/400 and shrink the route's message list to one entry (#5367) `POST /analytics/dataset/query` classified six error families by matching hardcoded substrings of their message text, because all six producers were bare `throw new Error(...)`. That made their HTTP status a property of their wording: a rephrasing with no logic change moved a refusal from 400 to 500 with nothing going red. #5352/#5366 delivered declared/loud/tested for that accommodation; Prime Directive #12 also requires removable on a schedule. Five producers now declare the verdict themselves through a new `dataset-refusal.ts` (`datasetInvalidError`, `DATASET_INVALID`/400, the same shape as `invalidFilterError`): - dataset-compiler: unsupported aggregate; undeclared relationship path - dataset-executor: unselected order key; totals grouping outside the selection - native-sql-strategy: join outside the declared allowlist Their five entries are deleted from the route's regex. `read-scope-sql` keeps its entry on purpose: its ten fail-closed refusals lower an admin-authored RLS policy, not caller input, so the right code/status is a separate judgement. `DATASET_INVALID` is registered under `@objectstack/service-analytics` in ERROR_CODE_LEDGER for provenance (the union is unchanged), and the constructor types it as `RegisteredErrorCode` so an unregistered code fails `tsc`. --- .../analytics-dataset-refusal-envelope.md | 73 ++++ .../analytics-dataset-dimension-gate.test.ts | 28 +- ...analytics-dataset-refusal-envelope.test.ts | 256 +++++++++++++ .../analytics-filter-refusal-envelope.test.ts | 71 +++- packages/rest/src/analytics-routes.test.ts | 12 +- packages/rest/src/rest-server.ts | 46 ++- .../dataset-refusal-envelope.test.ts | 337 ++++++++++++++++++ .../service-analytics/src/dataset-compiler.ts | 10 +- .../service-analytics/src/dataset-executor.ts | 9 +- .../service-analytics/src/dataset-refusal.ts | 95 +++++ .../src/strategies/native-sql-strategy.ts | 23 +- .../spec/src/api/error-code-ledger.zod.ts | 1 + 12 files changed, 919 insertions(+), 42 deletions(-) create mode 100644 .changeset/analytics-dataset-refusal-envelope.md create mode 100644 packages/rest/src/analytics-dataset-refusal-envelope.test.ts create mode 100644 packages/services/service-analytics/src/__tests__/dataset-refusal-envelope.test.ts create mode 100644 packages/services/service-analytics/src/dataset-refusal.ts diff --git a/.changeset/analytics-dataset-refusal-envelope.md b/.changeset/analytics-dataset-refusal-envelope.md new file mode 100644 index 0000000000..94b059ecc4 --- /dev/null +++ b/.changeset/analytics-dataset-refusal-envelope.md @@ -0,0 +1,73 @@ +--- +"@objectstack/service-analytics": patch +"@objectstack/rest": patch +"@objectstack/spec": patch +--- + +fix(analytics,rest): five dataset refusals declare `DATASET_INVALID` / 400 themselves, and the route's message-sniffing list shrinks to one entry (#5367) + +`POST /analytics/dataset/query` answered `400 DATASET_INVALID` for six error +families because the route recognised their **prose**, not because the errors +said anything about themselves. #5352 gave the catch an ADR-0112 envelope branch +(`error.code` + a 4xx `error.status`, read first) and had to leave a hardcoded +list of message substrings behind it, since all six producers were still bare +`throw new Error(…)`: + +``` +/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/ +``` + +That made the HTTP status of six families a property of their wording. +Rephrasing `dataset-compiler`'s "is not declared in the dataset's `include`" — +no logic change — moved that refusal from 400 to 500, i.e. re-opened #5352 for a +different family, and no test and no gate would have gone red. Prime Directive +#12 permits an accommodation like that only while it is declared, loud, tested +**and removable on a schedule**; #5366 delivered the first three and nothing +carried the fourth. + +**Five producers now declare their own verdict.** A new +`dataset-refusal.ts` in `@objectstack/service-analytics` exports +`datasetInvalidError` — the same shape as that package's existing +`invalidFilterError` (`INVALID_FILTER` / 400) and `assertDimensionFields` +(`INVALID_FIELD` / 400) — and five sites throw through it: + +- `dataset-compiler.ts` — a measure whose aggregate the v1 runtime cannot lower; + a dimension/measure traversing a relationship path the dataset never declared + in `include`; +- `dataset-executor.ts` — an `order` key that is not a selected dimension or + measure; a `totals` grouping that is not a subset of the selected dimensions; +- `native-sql-strategy.ts` — a join outside the dataset's declared allowlist. + +Their five entries are gone from the route's list, which is now a single +`read-scope-sql` test. + +**`read-scope-sql` deliberately stays.** Its ten fail-closed refusals are RLS +read-scope lowering failures whose inputs are an admin-authored policy and a +compiler-generated join alias — not caller input — so `DATASET_INVALID` ("your +request is invalid") may well be the wrong verdict and choosing the right one is +a separate judgement, still tracked by #5367. Deleting the entry before that +judgement lands would regress those ten from `400 DATASET_INVALID` to 500. + +**No outward behaviour change for the five.** They answered +`400 DATASET_INVALID` before and answer `400 DATASET_INVALID` now, with the same +message; what changed is the mechanism, from message-matching to the producer's +own declaration. The one visible difference is for a bare `Error` that merely +*resembles* one of those messages: it is no longer promoted to a 400. That is the +point — a phrase is no longer a classification. + +`DATASET_INVALID` is registered in `ERROR_CODE_LEDGER` under +`@objectstack/service-analytics` as well as `@objectstack/rest` (provenance, per +ADR-0112 D3; the code itself is unchanged and the union does not grow), and the +constructor types it as `RegisteredErrorCode` so an unregistered code is a +compile error rather than a body some route rejects at runtime. + +Coverage: `dataset-refusal-envelope.test.ts` (service-analytics) pins each of the +five refusals against its real producer — the refusal SET first, green before and +after, then the envelope; `analytics-dataset-refusal-envelope.test.ts` (rest) +drives all five end-to-end through a real `AnalyticsService` with positive +controls on both the aggregate and raw-SQL paths; and +`analytics-filter-refusal-envelope.test.ts` pins the deletion in both directions +— the five messages answer 400 when enveloped and 500 when bare, so re-adding a +regex entry turns it red. diff --git a/packages/rest/src/analytics-dataset-dimension-gate.test.ts b/packages/rest/src/analytics-dataset-dimension-gate.test.ts index 2f17c84ccd..17ca677a8a 100644 --- a/packages/rest/src/analytics-dataset-dimension-gate.test.ts +++ b/packages/rest/src/analytics-dataset-dimension-gate.test.ts @@ -295,16 +295,38 @@ describe('[#5520] the 500 body no longer ships driver internals', () => { 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. + // …② and so does a dataset refusal, with its message intact. + // + // [#5367] This half used to send a BARE `Error` reading "… is not declared + // in the dataset." and rely on the route's transitional message list to + // classify it. #5367 enveloped that producer (`dataset-compiler` now throws + // `datasetInvalidError`) and deleted the list entry, so the same refusal is + // now carried by branch ① — same outward answer, chosen by reading the error + // instead of by matching its prose. This change is still 5xx-only. const b = await post( buildRoute(async () => - throwingAnalytics(new Error('[dataset-compiler] dimension "region" is not declared in the dataset.')), + throwingAnalytics( + Object.assign( + new Error('[dataset-compiler] dimension "region" references relationship path "account" via "account.region", but "account" is not declared in the dataset\'s `include`.'), + { code: 'DATASET_INVALID', status: 400 }, + ), + ), ), { 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/); + + // …③ and the ONE message-list entry #5367 deliberately left in place still + // answers 400 for `read-scope-sql`'s bare fail-closed refusals. + const c = await post( + buildRoute(async () => + throwingAnalytics(new Error('[read-scope-sql] unsupported operator "$regex" on "owner" (fail-closed).')), + ), + { dataset, selection: { measures: ['account_count'], dimensions: ['industry'] } }, + ); + expect(c.statusCode).toBe(400); + expect(c.body.code).toBe('DATASET_INVALID'); }); }); diff --git a/packages/rest/src/analytics-dataset-refusal-envelope.test.ts b/packages/rest/src/analytics-dataset-refusal-envelope.test.ts new file mode 100644 index 0000000000..e1a3e28fb6 --- /dev/null +++ b/packages/rest/src/analytics-dataset-refusal-envelope.test.ts @@ -0,0 +1,256 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#5367] `POST /analytics/dataset/query` — five dataset refusals reach the + * caller as `400 DATASET_INVALID` because their PRODUCER says so, not because + * this route recognised their prose. + * + * ## The seam, and why this file boots the REAL analytics service + * + * #5352 / PR #5366 gave the route an envelope branch (`error.code` + a 4xx + * `error.status`, read first) and left a transitional list of message substrings + * behind it, because six refusal families were still bare `throw new Error(…)`: + * + * ``` + * /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/ + * ``` + * + * With that list in place the HTTP status of six families was a property of their + * **wording**: rephrasing `dataset-compiler`'s "is not declared in the dataset's + * `include`" — no logic change — dropped the refusal from 400 to 500, and no test + * and no gate would have gone red. Prime Directive #12 allows such an + * accommodation only while it is declared, loud, tested **and removable on a + * schedule**; #5366 delivered the first three. #5367 is the schedule: five + * producers now throw `datasetInvalidError` (`DATASET_INVALID` / 400) and their + * five entries are gone from the list. + * + * The claim has two halves and either alone reads as fixed: + * + * - **B** — the producer throws the envelope. Pinned per site, against the real + * producer, in `service-analytics`'s `dataset-refusal-envelope.test.ts`. + * - **A** — the route classifies on that envelope rather than on the message. + * Pinned in `analytics-filter-refusal-envelope.test.ts`, including the + * five deleted entries asserted in both directions. + * + * A unit test on either side can be green while an author still sees a 500. So + * this file asserts the SEAM: the analytics provider is a real `AnalyticsService` + * compiling a real dataset, the error crossing into the catch is the one the real + * `dataset-compiler` / `dataset-executor` / `native-sql-strategy` throws, and + * nothing here asserts a shape it also constructs. + * + * ## Reverse verification, direction predicted BEFORE running + * + * Restore ANY of the five `throw new Error(…)` calls in `service-analytics` (and + * rebuild it — this file exercises the BUILT package) and that family's case here + * goes RED with `500 ANALYTICS_QUERY_FAILED`, because the route no longer carries + * a message entry that would rescue it. The direction is plain red, not the + * inverted/extra-diagnostic shapes: the canonical envelope branch is FIRST in the + * catch and the fallback that used to answer for these families is gone, so + * "producer stops declaring" has exactly one outward consequence. Confirmed by + * running it (see the PR). + * + * Positive controls sit next to the refusals so a case cannot pass merely because + * the wiring never reached the producer. + */ + +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-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; +} + +/** Build a RestServer over an analytics provider (positional arg #15). */ +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() {} }; + +/** + * A real `AnalyticsService` on the ObjectQL aggregate path — one fixed bucket, so + * a selection that gets far enough to touch data answers 200. That is what makes + * the refusals meaningful: they fail on the dataset/selection, on a route that + * demonstrably works otherwise. + */ +function aggregateAnalytics(): AnalyticsService { + return new AnalyticsService({ + logger: silent, + queryCapabilities: () => ({ nativeSql: false, objectqlAggregate: true, inMemory: false }), + executeAggregate: async () => [{ stage: 'won', revenue: 100 }], + isRegisteredObject: () => true, + }); +} + +/** A real `AnalyticsService` on the raw-SQL path — the only one that enforces the join allowlist. */ +function nativeAnalytics(): AnalyticsService { + return new AnalyticsService({ + logger: silent, + queryCapabilities: () => ({ nativeSql: true, objectqlAggregate: false, inMemory: false }), + executeRawSql: async () => [{ stage: 'won', revenue: 100 }], + isRegisteredObject: () => true, + }); +} + +/** A valid single-object dataset — no `include`, so nothing here needs a join. */ +const dataset = { + name: 'pipeline', + label: 'Pipeline', + object: 'crm_opportunity', + dimensions: [{ name: 'stage', field: 'stage', type: 'string' }], + measures: [{ name: 'revenue', aggregate: 'sum', field: 'amount' }], +}; +const selection = { dimensions: ['stage'], measures: ['revenue'] }; + +// ───────────────────────────────────────────────────────────────────────────── + +describe('[#5367] a dataset refusal answers 400 DATASET_INVALID from its own envelope', () => { + /** + * One row per entry #5367 removed from the route's message list, driven through + * the real producer. `listEntry` records the substring that used to classify it + * — the audit trail that the deletion and this coverage are the same set. + */ + const CASES: Array<{ + name: string; + listEntry: string; + body: unknown; + analytics: () => AnalyticsService; + message: RegExp; + }> = [ + { + name: 'dataset-compiler: an aggregate the v1 runtime cannot lower', + listEntry: 'not supported by the v1 dataset runtime', + analytics: aggregateAnalytics, + body: { + dataset: { + ...dataset, + measures: [{ name: 'names', aggregate: 'string_agg', field: 'name' }], + }, + selection: { dimensions: ['stage'], measures: ['names'] }, + }, + message: /measure "names" uses aggregate "string_agg" which is not supported by the v1 dataset runtime/, + }, + { + name: 'dataset-compiler: a dimension traversing an undeclared relationship path', + listEntry: 'not declared in the dataset', + analytics: aggregateAnalytics, + body: { + dataset: { + ...dataset, + include: [], + dimensions: [{ name: 'region', field: 'account.region', type: 'string' }], + }, + selection: { dimensions: ['region'], measures: ['revenue'] }, + }, + message: /"account" is not declared in the dataset's `include`/, + }, + { + name: 'dataset-executor: an order key that is not selected', + listEntry: 'not a selected dimension or measure', + analytics: aggregateAnalytics, + body: { dataset, selection: { ...selection, order: { profit: 'desc' } } }, + message: /order key\(s\) "profit" — not a selected dimension or measure/, + }, + { + name: 'dataset-executor: a totals grouping outside the selection', + listEntry: 'is not a subset of the selected dimensions', + analytics: aggregateAnalytics, + body: { dataset, selection: { ...selection, totals: { groupings: [['region']] } } }, + message: /totals grouping \[region\] is not a subset of the selected dimensions/, + }, + { + // The caller-shaped trigger: the DATASET declares no `include`, and the + // SELECTION names a dotted dimension, which `lookupMember`'s synthetic + // relation fallback turns into a join at alias `account`. + name: 'native-sql-strategy: a selection naming a join outside the allowlist', + listEntry: 'not backed by a declared relationship', + analytics: nativeAnalytics, + body: { dataset, selection: { dimensions: ['account.region'], measures: ['revenue'] } }, + message: /join "account" is not backed by a declared relationship on cube "pipeline"/, + }, + ]; + + for (const c of CASES) { + it(`${c.name} → 400 DATASET_INVALID`, async () => { + const route = buildRoute(async () => c.analytics()); + const res = await post(route, c.body); + + expect(res.statusCode).toBe(400); + expect(res.body.code).toBe('DATASET_INVALID'); + // 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('covers exactly the five entries #5367 deleted from the message list', () => { + expect(CASES.map((c) => c.listEntry).sort()).toEqual([ + 'is not a subset of the selected dimensions', + 'not a selected dimension or measure', + 'not backed by a declared relationship', + 'not declared in the dataset', + 'not supported by the v1 dataset runtime', + ]); + }); + + 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): a DECLARED relationship still joins → 200 with rows', async () => { + // The allowlist case's twin: `include: ['account']` makes the same dotted + // selection legal, so case ⑤ above is a verdict about the allowlist rather + // than about dotted members being rejected outright. + const route = buildRoute(async () => nativeAnalytics()); + const res = await post(route, { + dataset: { ...dataset, include: ['account'] }, + selection: { dimensions: ['account.region'], measures: ['revenue'] }, + }); + expect(res.statusCode).toBe(200); + expect(res.body.rows).toEqual([{ stage: 'won', revenue: 100 }]); + }); +}); diff --git a/packages/rest/src/analytics-filter-refusal-envelope.test.ts b/packages/rest/src/analytics-filter-refusal-envelope.test.ts index c273e3827b..c35cdac19a 100644 --- a/packages/rest/src/analytics-filter-refusal-envelope.test.ts +++ b/packages/rest/src/analytics-filter-refusal-envelope.test.ts @@ -32,10 +32,11 @@ * Reading the envelope makes this route classify on what the error SAYS about * itself. Three regressions would each be worse than the bug: * - * 1. The message list still classifies the families that remain bare `Error`s - * (the dataset compiler, `read-scope-sql`, the executor) — all six of its - * entries were re-verified unenveloped at the time of #5352, so deleting - * it would regress them from `400 DATASET_INVALID` to 500. + * 1. The message list still classifies the ONE family that remains a bare + * `Error` — `read-scope-sql` — so deleting its entry would regress it from + * `400 DATASET_INVALID` to 500. (#5352 left six entries here. #5367 + * enveloped five of the six producers and deleted their entries; the block + * near the bottom of this file now pins the deletion in both directions.) * 2. A genuine internal fault must still be a 500 with its `logError` line — * "read the envelope" must not become "call everything a 400". * 3. A 5xx-status error is NOT passed through, so an internal fault can never @@ -268,16 +269,43 @@ describe('[#5322] empty combinators are boolean identities at the REST face — } }); -describe('[#5352] the message-sniffing fallback still classifies the families that carry no envelope', () => { - // Every entry of the route's regex list, produced as its owner produces it: - // a bare `Error`. Re-verified unenveloped while #5352 was implemented — - // `dataset-compiler.ts`, `native-sql-strategy.ts`, `dataset-executor.ts` and - // `read-scope-sql.ts` all `throw new Error(…)` with no `code`/`status` — so - // the list is the only thing standing between them and a 500. - const FALLBACK: Array<{ name: string; message: string }> = [ +describe('[#5352 → #5367] the message-sniffing fallback is down to its last entry', () => { + // ── The surviving entry ──────────────────────────────────────────────────── + // `read-scope-sql.ts`'s ten refusals are still bare `Error`s, and #5367 + // deliberately left them that way: their inputs are an admin-authored RLS + // policy and a compiler-generated join alias, not caller input, so + // `DATASET_INVALID` may well be the wrong verdict for them and choosing the + // right one is a separate judgement. Until it lands, this entry is all that + // stands between them and a 500. + it('read-scope-sql: a fail-closed read scope → still 400 DATASET_INVALID by the message list', async () => { + const message = '[read-scope-sql] unsupported operator "$regex" on "owner" (fail-closed).'; + const route = buildRoute(async () => ({ queryDataset: vi.fn().mockRejectedValue(new Error(message)) })); + const res = await post(route, { dataset, selection }); + expect(res.statusCode).toBe(400); + expect(res.body.code).toBe('DATASET_INVALID'); + expect(String(res.body.message)).toMatch(/read-scope-sql/); + }); + + // ── The five entries #5367 deleted, pinned in BOTH directions ────────────── + // These rows used to assert "a bare `Error` with this message → 400", which is + // precisely the fragility #5367 removed: the status was a property of the + // wording. Re-asserting it would now be asserting the defect. So each family + // is pinned twice instead: + // + // - ENVELOPED (what its producer now throws) → 400 `DATASET_INVALID` by ①, + // i.e. the same outward answer, reached by reading the error rather than + // by matching its prose; + // - BARE with the identical message → 500, which is what proves the regex + // entry is really gone. Re-adding one turns this half red. + // + // The producer end — that these are the messages and envelopes the real + // `dataset-compiler` / `dataset-executor` / `native-sql-strategy` throw — is + // pinned in `service-analytics`'s `dataset-refusal-envelope.test.ts`, and the + // whole path is driven end-to-end in `analytics-dataset-refusal-envelope.test.ts`. + const RETIRED: Array<{ name: string; message: string }> = [ { name: 'dataset-compiler: undeclared relationship path', - message: 'dimension "region" references relationship path "account" via "account.region", but "account" is not declared in the dataset\'s `include`.', + message: '[dataset-compiler] dimension "region" references relationship path "account" via "account.region", but "account" is not declared in the dataset\'s `include`.', }, { name: 'native-sql-strategy: join outside the allowlist', @@ -287,10 +315,6 @@ describe('[#5352] the message-sniffing fallback still classifies the families th name: 'dataset-compiler: aggregate outside the v1 runtime', message: '[dataset-compiler] measure "x" uses aggregate "median" which is not supported by the v1 dataset runtime (supported: sum, avg).', }, - { - name: 'read-scope-sql: fail-closed read scope', - message: '[read-scope-sql] unsupported operator "$regex" on "owner" (fail-closed).', - }, { name: 'dataset-executor: order key that is not selected', message: '[dataset-executor] order key(s) "profit" — not a selected dimension or measure. Selectable here: stage, revenue.', @@ -301,12 +325,21 @@ describe('[#5352] the message-sniffing fallback still classifies the families th }, ]; - for (const c of FALLBACK) { - it(`${c.name} → still 400 DATASET_INVALID`, async () => { - const route = buildRoute(async () => ({ queryDataset: vi.fn().mockRejectedValue(new Error(c.message)) })); + for (const c of RETIRED) { + it(`${c.name} → 400 DATASET_INVALID by its ENVELOPE`, async () => { + const err = Object.assign(new Error(c.message), { code: 'DATASET_INVALID', status: 400 }); + const route = buildRoute(async () => ({ queryDataset: vi.fn().mockRejectedValue(err) })); const res = await post(route, { dataset, selection }); expect(res.statusCode).toBe(400); expect(res.body.code).toBe('DATASET_INVALID'); + expect(String(res.body.message)).toBe(c.message); + }); + + it(`${c.name} → the same message WITHOUT an envelope is no longer sniffed (500)`, async () => { + const route = buildRoute(async () => ({ queryDataset: vi.fn().mockRejectedValue(new Error(c.message)) })); + const res = await post(route, { dataset, selection }); + expect(res.statusCode).toBe(500); + expect(res.body.code).toBe('ANALYTICS_QUERY_FAILED'); }); } }); diff --git a/packages/rest/src/analytics-routes.test.ts b/packages/rest/src/analytics-routes.test.ts index 9645a4bda7..867970becf 100644 --- a/packages/rest/src/analytics-routes.test.ts +++ b/packages/rest/src/analytics-routes.test.ts @@ -149,7 +149,17 @@ describe('POST /analytics/dataset/query', () => { }); it('maps a dataset D-C compile error to 400 (undeclared relationship)', async () => { - const queryDataset = vi.fn().mockRejectedValue(new Error("dimension \"region\" references relationship \"account\" via \"account.region\", but \"account\" is not declared in the dataset's `include`.")); + // [#5367] The refusal now arrives in the ADR-0112 envelope + // (`dataset-compiler`'s `datasetInvalidError`) instead of as a bare `Error` + // classified by the route's message list — that list's five dataset entries + // were deleted once their producers carried the envelope. The outward answer + // is unchanged; what changed is which branch produces it. + const queryDataset = vi.fn().mockRejectedValue( + Object.assign( + new Error("dimension \"region\" references relationship \"account\" via \"account.region\", but \"account\" is not declared in the dataset's `include`."), + { code: 'DATASET_INVALID', status: 400 }, + ), + ); const { route } = buildServer(async () => ({ queryDataset })); const res = mockRes(); await route!.handler({ method: 'POST', params: {}, headers: {}, body: { dataset: inlineDataset, selection } } as any, res); diff --git a/packages/rest/src/rest-server.ts b/packages/rest/src/rest-server.ts index 4bc77589b4..5727944477 100644 --- a/packages/rest/src/rest-server.ts +++ b/packages/rest/src/rest-server.ts @@ -6794,21 +6794,39 @@ export class RestServer { return res.status(envelopeStatus).json({ code: envelopeCode, message: msg.slice(0, 1000) }); } // ── ② TRANSITIONAL fallback: message sniffing ──────────── - // Dataset-compiler D-C / unsupported-aggregate / read-scope - // errors are client-side mistakes — surface as 400. + // ⚠️ ONE entry left, and it is on a retirement schedule. // - // ⚠️ This list survives only because those producers are - // still bare `Error`s: nothing in the dataset compiler or - // `read-scope-sql` carries a `code`/`status` yet, so with the - // list gone they would regress from `400 DATASET_INVALID` to - // 500. It is a placeholder for their enveloping, NOT a - // second classification mechanism — a phrasing change in any - // of these messages silently reclassifies the error, which - // is exactly the fragility #5352 removed for the filter - // family. Enveloping them retires this branch; until then, - // do not add to it — give the new refusal a `code`/`status` - // and it is served by ① for free. - 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)) { + // #5352 left this list at six entries because all six + // producers were bare `Error`s. That made the HTTP status of + // six error families a property of their WORDING: rephrasing + // a message — no logic change — moved the error from 400 to + // 500 with no test and no gate going red, which is #5352's + // own fragility surviving in the thing that patched it. + // #5367 is the schedule the accommodation was missing + // (Prime Directive #12: declared, loud, tested AND removable + // on a schedule). + // + // FIVE entries are gone because their producers now carry the + // ADR-0112 envelope and are served by ① — `dataset-compiler` + // (undeclared relationship path, unsupported aggregate), + // `dataset-executor` (order key, totals grouping) and + // `native-sql-strategy` (join outside the allowlist) all throw + // `datasetInvalidError` (`DATASET_INVALID` / 400) from + // `service-analytics`'s `dataset-refusal.ts`. + // + // `read-scope-sql` is the LAST entry, and deliberately so: + // its ten refusals are fail-closed RLS-lowering failures + // whose inputs are an admin-authored policy and a + // compiler-generated join alias — NOT caller input — so + // `DATASET_INVALID` ("your request is invalid") is very + // possibly the wrong verdict for them and the right one is a + // separate judgement. Until that judgement lands, deleting + // this entry would regress them from `400 DATASET_INVALID` to + // 500, i.e. #5352 again. Tracked as the final item of #5367. + // + // Do not add to this list. Give the new refusal a + // `code`/`status` and ① serves it for free. + if (/read-scope-sql/.test(msg)) { return res.status(400).json({ code: 'DATASET_INVALID', message: msg.slice(0, 1000) }); } // ── ③ The 500 — and it does not ship driver internals ──── diff --git a/packages/services/service-analytics/src/__tests__/dataset-refusal-envelope.test.ts b/packages/services/service-analytics/src/__tests__/dataset-refusal-envelope.test.ts new file mode 100644 index 0000000000..7e71657b32 --- /dev/null +++ b/packages/services/service-analytics/src/__tests__/dataset-refusal-envelope.test.ts @@ -0,0 +1,337 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#5367] The five dataset refusals carry the ADR-0112 envelope + * (`DATASET_INVALID` / 400) — and the refusal SET did not move. + * + * ## What was wrong + * + * #5352 / PR #5366 made `/analytics/dataset/query` classify a thrown error by + * reading its `code` + 4xx `status`, and enveloped `filter-normalizer.ts`'s nine + * refusals so the route could. Six OTHER refusal families in this package stayed + * bare `throw new Error(…)`, and kept answering `400 DATASET_INVALID` only + * because the route carried a transitional list of message SUBSTRINGS: + * + * ``` + * /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/ + * ``` + * + * That made the HTTP status of six families a property of their **wording**. + * Rephrasing "is not declared in the dataset's `include`" — no logic change — + * moved the refusal from 400 to 500, which is exactly the defect #5352 fixed, + * replayed on a different family, and no test or gate would have gone red. + * Prime Directive #12 permits an accommodation like that while it is declared, + * loud, tested **and removable on a schedule**; #5366 shipped the first three + * and nothing carried the fourth. #5367 is the schedule. + * + * ## The two halves of this file, and why the second one exists + * + * `describe('the refusal SET is unchanged')` pins WHICH inputs are refused and + * what each refusal says. Every assertion in it passes both BEFORE and AFTER + * #5367 — that is its whole job. This change alters the SHAPE of an error and + * nothing about the judgement that produced it, and "we only touched the + * envelope" is a claim worth being able to re-run rather than assert. + * + * `describe('every enveloped refusal carries …')` is the change. Run it against + * pre-#5367 code and all five cases fail with `code` / `status` `undefined`, + * while the block above stays green. + * + * `describe('what deliberately stays a bare Error')` is the retirement schedule's + * remaining item, pinned so it cannot be forgotten in either direction: the + * `read-scope-sql` family is still unenveloped, so the route's list still needs + * its one surviving entry. When those ten refusals gain an envelope, this block + * goes red — and the fix is to delete that last entry in the same PR. + */ + +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, resolveOrdering } from '../dataset-executor.js'; +import { NativeSQLStrategy } from '../strategies/native-sql-strategy.js'; +import { compileScopedFilterToSql } from '../read-scope-sql.js'; + +/** The ADR-0112 fields the REST boundary classifies on. */ +interface Refusal extends Error { + code?: unknown; + status?: unknown; +} + +/** Run `thunk` and return the error it threw, if any. */ +async function refusalFrom(thunk: () => unknown | Promise): Promise { + try { + await thunk(); + return undefined; + } catch (e) { + return e as Refusal; + } +} + +// ── fixtures ───────────────────────────────────────────────────────────────── + +/** A valid single-object dataset — nothing here needs a join. */ +const validDataset = DatasetSchema.parse({ + name: 'pipeline', + label: 'Pipeline', + object: 'crm_opportunity', + dimensions: [{ name: 'stage', field: 'stage', type: 'string' }], + measures: [{ name: 'revenue', aggregate: 'sum', field: 'amount' }], +}); + +function fakeService(rows: Record[] = [{ stage: 'won', revenue: 100 }]): IAnalyticsService { + return { + query: vi.fn(async (_q: AnalyticsQuery): Promise => ({ + rows, + fields: [{ name: 'revenue', type: 'number' }], + })), + getMeta: async () => [], + }; +} + +/** + * A cube whose ONLY dotted reference comes from the QUERY, not the cube — the + * caller-shaped trigger for the join allowlist. + * + * `lookupMember` answers a dotted member no dimension declares with a synthetic + * `{ sql: member }`, so `selection.dimensions: ['account.region']` registers a + * join at alias `account`. A dataset's own dimensions cannot reach here: + * `compileDataset`'s `assertDeclared` refuses an undeclared relationship path at + * compile time (case ② below). + */ +const bareCube: Cube = { + name: 'pipeline', + title: 'Pipeline', + sql: 'crm_opportunity', + measures: { revenue: { name: 'revenue', label: 'Revenue', type: 'sum', sql: 'amount' } }, + dimensions: { stage: { name: 'stage', label: 'Stage', type: 'string', sql: 'stage' } }, + public: false, +}; + +function nativeCtx(allowed: Set): StrategyContext { + return { + getCube: (name: string) => (name === 'pipeline' ? bareCube : undefined), + queryCapabilities: () => ({ nativeSql: true, objectqlAggregate: false, inMemory: false }), + executeRawSql: async () => [], + getAllowedRelationships: () => allowed, + } as StrategyContext; +} + +/** + * The five refusing sites the route's message list used to classify, one input + * each, driven through the REAL producer. + * + * `listEntry` is the substring the route matched before #5367 — it is what makes + * the deletion auditable: every entry removed from that regex has a row here. + */ +const REFUSALS: Array<{ + name: string; + listEntry: string; + message: RegExp; + run: () => unknown | Promise; +}> = [ + { + name: '① dataset-compiler: aggregate outside the v1 runtime', + listEntry: 'not supported by the v1 dataset runtime', + message: /measure "names" uses aggregate "string_agg" which is not supported by the v1 dataset runtime/, + run: () => + compileDataset( + DatasetSchema.parse({ + name: 'pipeline', + label: 'Pipeline', + object: 'crm_opportunity', + dimensions: [{ name: 'stage', field: 'stage', type: 'string' }], + measures: [{ name: 'names', aggregate: 'string_agg', field: 'name' }], + }), + ), + }, + { + name: '② dataset-compiler: field traversing an undeclared relationship path', + listEntry: 'not declared in the dataset', + message: /dimension "region" references relationship path "account" via "account\.region", but "account" is not declared in the dataset's `include`/, + run: () => + compileDataset( + DatasetSchema.parse({ + name: 'pipeline', + label: 'Pipeline', + object: 'crm_opportunity', + include: [], + dimensions: [{ name: 'region', field: 'account.region', type: 'string' }], + measures: [{ name: 'revenue', aggregate: 'sum', field: 'amount' }], + }), + ), + }, + { + name: '③ dataset-executor: an order key that is not selected', + listEntry: 'not a selected dimension or measure', + message: /order key\(s\) "profit" — not a selected dimension or measure\. Selectable here: stage, revenue/, + run: () => + resolveOrdering( + { dimensions: ['stage'], measures: ['revenue'], order: { profit: 'desc' } } as DatasetSelection, + ['stage'], + ), + }, + { + name: '④ dataset-executor: a totals grouping outside the selection', + listEntry: 'is not a subset of the selected dimensions', + message: /totals grouping \[region\] is not a subset of the selected dimensions — unknown: region/, + run: () => + new DatasetExecutor(fakeService()).execute(compileDataset(validDataset), { + dimensions: ['stage'], + measures: ['revenue'], + totals: { groupings: [['region']] }, + }), + }, + { + name: '⑤ native-sql-strategy: a join outside the declared allowlist', + listEntry: 'not backed by a declared relationship', + message: /join "account" is not backed by a declared relationship on cube "pipeline"/, + run: () => + new NativeSQLStrategy().generateSql( + { cube: 'pipeline', measures: ['revenue'], dimensions: ['account.region'], timezone: 'UTC' }, + nativeCtx(new Set()), + ), + }, +]; + +// ───────────────────────────────────────────────────────────────────────────── + +describe('[#5367] the refusal SET is unchanged — only the error shape moved', () => { + for (const c of REFUSALS) { + it(`still REFUSES: ${c.name}`, async () => { + const err = await refusalFrom(c.run); + expect(err, `${c.name} was accepted — the refusal set moved`).toBeInstanceOf(Error); + expect(String(err?.message)).toMatch(c.message); + }); + } + + it('the ACCEPTING neighbours still compile — the refusal did not widen', async () => { + // ①/② compile clean; ③ returns the requested order; ④ runs the grouping; + // ⑤ emits the join. One assertion per site, on the input one character away + // from the refused one. + expect(compileDataset(validDataset).cube.name).toBe('pipeline'); + expect( + compileDataset( + DatasetSchema.parse({ + name: 'joined', + label: 'Joined', + object: 'crm_opportunity', + include: ['account'], + dimensions: [{ name: 'region', field: 'account.region', type: 'string' }], + measures: [{ name: 'revenue', aggregate: 'sum', field: 'amount' }], + }), + ).allowedRelationships.has('account'), + ).toBe(true); + expect( + resolveOrdering( + { dimensions: ['stage'], measures: ['revenue'], order: { revenue: 'desc' } } as DatasetSelection, + ['stage'], + ), + ).toEqual({ revenue: 'desc' }); + const totalled = await new DatasetExecutor(fakeService()).execute(compileDataset(validDataset), { + dimensions: ['stage'], + measures: ['revenue'], + totals: { groupings: [['stage']] }, + }); + expect(totalled.totals?.[0].dimensions).toEqual(['stage']); + const { sql } = await new NativeSQLStrategy().generateSql( + { cube: 'pipeline', measures: ['revenue'], dimensions: ['account.region'], timezone: 'UTC' }, + nativeCtx(new Set(['account'])), + ); + expect(sql).toContain('LEFT JOIN "account"'); + }); +}); + +describe('[#5367] every enveloped refusal carries the ADR-0112 envelope (DATASET_INVALID / 400)', () => { + for (const c of REFUSALS) { + it(`${c.name} → code DATASET_INVALID, status 400`, async () => { + const err = await refusalFrom(c.run); + expect(err).toBeInstanceOf(Error); + // Read exactly as `rest-server.ts`'s catch reads them. + expect(err?.code, 'a refusal with no `code` lands as 500 ANALYTICS_QUERY_FAILED').toBe('DATASET_INVALID'); + expect(err?.status, 'a refusal with no `status` lands as 500 ANALYTICS_QUERY_FAILED').toBe(400); + }); + } + + it('covers every entry #5367 removed from the route message list', () => { + // The audit trail for the deletion: five entries left that regex, and each + // one has a row above driven through its real producer. `read-scope-sql` is + // the sixth and is deliberately absent — see the block below. + expect(REFUSALS.map((c) => c.listEntry)).toEqual([ + 'not supported by the v1 dataset runtime', + 'not declared in the dataset', + 'not a selected dimension or measure', + 'is not a subset of the selected dimensions', + 'not backed by a declared relationship', + ]); + }); +}); + +describe('[#5367] what deliberately stays a bare Error — the schedule’s remaining item', () => { + /** + * Two of `read-scope-sql.ts`'s ten fail-closed refusals, as samples of the + * family: an unsupported operator (the deepest site) and an unsafe identifier + * (the shallowest). + * + * They stay bare because their INPUTS are not the caller's: the filter is the + * RLS `FilterCondition` the security service compiles from an + * admin-authored policy, and the alias is a join alias the dataset compiler + * generated. `DATASET_INVALID` says "your request is invalid", which for a + * broken sharing rule blames the wrong author — so the right code/status is a + * separate judgement (#5367's open question), and until it lands the route's + * list keeps its one `read-scope-sql` entry. + */ + const READ_SCOPE: Array<{ name: string; run: () => unknown }> = [ + { + name: 'unsupported operator in a read scope', + run: () => compileScopedFilterToSql({ owner: { $regex: 'admin' } } as never, 'crm_opportunity'), + }, + { + name: 'unsafe alias identifier', + run: () => compileScopedFilterToSql({ owner_id: 'u1' } as never, 'not a valid alias'), + }, + ]; + + for (const c of READ_SCOPE) { + it(`read-scope-sql: ${c.name} → still refuses, still WITHOUT an envelope`, async () => { + const err = await refusalFrom(c.run); + expect(err).toBeInstanceOf(Error); + expect(String(err?.message)).toContain('[read-scope-sql]'); + expect(String(err?.message)).toContain('(fail-closed)'); + // ⛔ When this flips, `rest-server.ts`'s analytics catch must lose its last + // message-list entry in the SAME PR — that deletion is the final item of + // #5367's retirement schedule, and the `[read-scope-sql]` prefix above is + // what that entry matches on. + expect(err?.code, 'read-scope-sql gained an envelope — retire the route list entry').toBeUndefined(); + expect(err?.status).toBeUndefined(); + }); + } + + it('the dataset-compiler internal invariant stays a 500, not a 400', async () => { + // "non-derived measure has no aggregate" is guarded defensively behind a + // spec refinement that already guarantees it. An arrival is OUR bug, so + // enveloping it as the caller's 400 would be the mirror-image defect — + // ops alerting stops seeing it and the author is told to fix something they + // did not write. Reached by bypassing the schema, which is the only way in. + const err = await refusalFrom(() => + compileDataset({ + name: 'pipeline', + label: 'Pipeline', + object: 'crm_opportunity', + dimensions: [{ name: 'stage', field: 'stage', type: 'string' }], + measures: [{ name: 'broken', field: 'amount' }], + } as never), + ); + expect(String(err?.message)).toMatch(/non-derived measure "broken" has no aggregate/); + expect(err?.code).toBeUndefined(); + expect(err?.status).toBeUndefined(); + }); +}); diff --git a/packages/services/service-analytics/src/dataset-compiler.ts b/packages/services/service-analytics/src/dataset-compiler.ts index 6cb9a6d3fa..627dcbdc0b 100644 --- a/packages/services/service-analytics/src/dataset-compiler.ts +++ b/packages/services/service-analytics/src/dataset-compiler.ts @@ -4,6 +4,7 @@ import type { Cube, Metric, Dimension as CubeDimension, CubeJoin } from '@object import { AggregationFunction } from '@objectstack/spec/data'; import type { Dataset, DatasetMeasure, DatasetDimension } from '@objectstack/spec/ui'; import type { FilterCondition } from '@objectstack/spec/data'; +import { datasetInvalidError } from './dataset-refusal.js'; /** * Dataset → Cube compiler (ADR-0021 D-A=(c), WS2). @@ -134,7 +135,9 @@ function aggregateToMetricType(m: DatasetMeasure): Metric['type'] { throw new Error(`[dataset-compiler] non-derived measure "${m.name}" has no aggregate`); } if (UNSUPPORTED_AGGREGATES.has(m.aggregate)) { - throw new Error( + // [#5367] `DATASET_INVALID` / 400 — the aggregate is the dataset author's + // choice, and the message already names the ones that would work. + throw datasetInvalidError( `[dataset-compiler] measure "${m.name}" uses aggregate "${m.aggregate}" which is ` + `not supported by the v1 dataset runtime (supported: ${SUPPORTED_AGGREGATES.join(', ')}).`, ); @@ -302,7 +305,10 @@ export function compileDataset( const assertDeclared = (field: string, ownerKind: string, ownerName: string) => { const relPath = fieldRelationshipPath(field); if (relPath && !joins[joinAlias(relPath)]) { - throw new Error( + // [#5367] `DATASET_INVALID` / 400 — a dimension/measure traversing a + // relationship the same document never declared in `include` is the + // dataset author's mistake, and the fix is in the document they hold. + throw datasetInvalidError( `[dataset-compiler] ${ownerKind} "${ownerName}" references relationship path "${relPath}" ` + `via "${field}", but "${relPath}" is not declared in the dataset's \`include\`. ` + `Only fields along a declared relationship path are joinable.`, diff --git a/packages/services/service-analytics/src/dataset-executor.ts b/packages/services/service-analytics/src/dataset-executor.ts index fcfbf87b3d..a63703ca8e 100644 --- a/packages/services/service-analytics/src/dataset-executor.ts +++ b/packages/services/service-analytics/src/dataset-executor.ts @@ -11,6 +11,7 @@ import { emptyGroupValueFor, type FilterCondition } from '@objectstack/spec/data import type { ExecutionContext } from '@objectstack/spec/kernel'; import { filterTokenContextFrom, resolveFilterTokens } from '@objectstack/core'; import type { CompiledDataset, DerivedMeasureSpec } from './dataset-compiler.js'; +import { datasetInvalidError } from './dataset-refusal.js'; import type { OrderLabelResolver } from './dimension-labels.js'; // Re-export the shared protocol shapes so existing importers keep working. @@ -435,7 +436,9 @@ export function resolveOrdering( ]); const unknown = Object.keys(order).filter((k) => !selectable.has(k)); if (unknown.length) { - throw new Error( + // [#5367] `DATASET_INVALID` / 400 — `selection.order` is request input and + // the message already lists what was selectable, so the caller can fix it. + throw datasetInvalidError( `[dataset-executor] order key(s) ${unknown.map((k) => `"${k}"`).join(', ')} — ` + `not a selected dimension or measure. Selectable here: ` + `${[...selectable].join(', ') || '(none)'}.`, @@ -595,7 +598,9 @@ export class DatasetExecutor { for (const grouping of groupings) { const unknown = grouping.filter((d) => !selected.has(d)); if (unknown.length) { - throw new Error( + // [#5367] `DATASET_INVALID` / 400 — `selection.totals.groupings` is + // request input, judged against the caller's own `selection.dimensions`. + throw datasetInvalidError( `[dataset-executor] totals grouping [${grouping.join(', ')}] is not a subset of the selected dimensions — unknown: ${unknown.join(', ')}.`, ); } diff --git a/packages/services/service-analytics/src/dataset-refusal.ts b/packages/services/service-analytics/src/dataset-refusal.ts new file mode 100644 index 0000000000..2f77f20d8d --- /dev/null +++ b/packages/services/service-analytics/src/dataset-refusal.ts @@ -0,0 +1,95 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#5367] The dataset refusals this service raises, in the ADR-0112 envelope. + * + * ## Why this file exists + * + * `/analytics/dataset/query` classifies a thrown error by reading its `code` + + * 4xx `status` (#5352 / PR #5366). Five refusals in this package were still bare + * `throw new Error(…)`, so the route could not read them at all — and they only + * kept answering `400 DATASET_INVALID` because the catch carried a hardcoded + * list of message SUBSTRINGS as a transitional fallback: + * + * ``` + * /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/ + * ``` + * + * Prime Directive #12 allows an accommodation like that only while it is + * declared, loud, tested **and removable on a schedule**. #5366 delivered the + * first three and nothing carried the fourth, which made the HTTP status of five + * error families a property of their WORDING: rephrasing + * "is not declared in the dataset's `include`" — no logic change, no test red, no + * gate red — silently moved that refusal from 400 to 500, i.e. re-opened #5352 + * for a different family. #5367 is that schedule; this constructor is how the + * five families leave the list. + * + * ## The envelope, and why it is `DATASET_INVALID` / 400 + * + * Same shape as `filter-normalizer.ts`'s `invalidFilterError` + * (`INVALID_FILTER` / 400) and `analytics-service.ts`'s dimension/measure gates + * (`INVALID_FIELD` / 400): the code names the caller-shaped mistake, the status + * says whose fault it is, and the message stays whatever the refusing site says. + * `DATASET_INVALID` is not a new code — it is what the route's fallback list has + * answered for these five families since #5352, registered in + * `ERROR_CODE_LEDGER` (ADR-0112 D3). Producing it HERE, rather than deriving it + * there from message text, is the whole change: one condition, one wire shape, + * chosen by the producer that knows. + * + * The `RegisteredErrorCode` annotation is load-bearing rather than decorative — + * 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. + * + * ## What deliberately does NOT go through here + * + * Not every `throw` in this package is the caller's mistake, and enveloping one + * that isn't would be the mirror-image defect — an internal fault re-labelled + * `400`, which hides it from ops alerting and tells the author to fix something + * they did not write. Three families stay bare on purpose: + * + * - **`read-scope-sql.ts`'s ten fail-closed refusals.** Its inputs are an RLS + * `FilterCondition` from the security service and a join alias from the + * dataset compiler — neither is caller input, so "the caller sent something + * invalid" is the wrong verdict for all ten. Their correct code/status is a + * separate judgement, tracked in #5367; the route's list therefore keeps its + * `read-scope-sql` entry, and that entry is now the only one left. + * - **Internal invariants** — e.g. `dataset-compiler.ts`'s "non-derived measure + * has no aggregate", which the spec refinement already guarantees. An + * arrival there is our bug; `500` is the honest answer. + * - **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 + * between two of our own tables, not a caller-shaped mistake", #5333). + * + * So this module is deliberately NOT "the only way this package refuses" — the + * claim `invalidFilterError` can make about `filter-normalizer.ts`. It is the way + * this package refuses **the caller**. + */ + +import type { RegisteredErrorCode } from '@objectstack/spec/api'; + +/** + * `DATASET_INVALID`, pinned against the ledger. + * + * Typed as `RegisteredErrorCode` so removing the ledger row (or misspelling the + * code here) fails `tsc` rather than shipping a code `ApiErrorSchema` rejects. + */ +const DATASET_INVALID: RegisteredErrorCode = 'DATASET_INVALID'; + +/** + * A dataset refusal in the ADR-0112 envelope — `DATASET_INVALID` / 400. + * + * Use it for a refusal the CALLER can fix by changing the request or the dataset + * definition they authored: a selection that names something the dataset does not + * declare, a dataset whose fields traverse an undeclared relationship, an + * aggregate the v1 runtime does not implement. See this module's header for the + * families that deliberately stay bare `Error`s. + */ +export function datasetInvalidError(message: string): Error { + const err = new Error(message) as Error & { code?: string; status?: number }; + err.code = DATASET_INVALID; + err.status = 400; + 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 f5d131d7aa..bf3f4d5dbc 100644 --- a/packages/services/service-analytics/src/strategies/native-sql-strategy.ts +++ b/packages/services/service-analytics/src/strategies/native-sql-strategy.ts @@ -11,6 +11,7 @@ import { type NormalizedFilterNode, } from './filter-normalizer.js'; import { compileScopedFilterToSql } from '../read-scope-sql.js'; +import { datasetInvalidError } from '../dataset-refusal.js'; import { likePattern, LIKE_ESCAPE_CHAR, type LikeShape } from '../like-pattern.js'; import { nextUtcCalendarDay } from '@objectstack/core'; @@ -220,7 +221,27 @@ export class NativeSQLStrategy implements AnalyticsStrategy { if (allowed) { for (const alias of joins.keys()) { if (!allowed.has(alias)) { - throw new Error( + // [#5367] `DATASET_INVALID` / 400 — verified caller-shaped before + // enveloping. Every join in `joins` was registered by + // `qualifyAndRegisterJoin`, and on the dataset route the only inputs + // that can register one OUTSIDE the allowlist are the REQUEST's own: + // `lookupMember`'s synthetic relation fallback mints a dotted + // dimension nobody declared, so `selection.dimensions`, + // `selection.timeDimensions` and a `runtimeFilter` member spelled + // `account.name` each land here. The dataset's OWN dimensions and + // measures cannot: `compileDataset`'s `assertDeclared` refuses an + // undeclared relationship path at compile time (also 400 + // `DATASET_INVALID`, so the two agree rather than diverge), and + // `resolveMeasureSql` has no synthetic fallback at all. + // + // The one non-caller trigger is the legacy + // `config.getAllowedRelationships` hook for hand-authored cubes, + // where a mismatch is the host's configuration rather than the + // caller's query. It is unreachable from + // `/analytics/dataset/query`: `queryDataset` registers the compiled + // dataset first, so `getAllowedRelationships` answers from + // `datasetRegistry` and never falls through to the hook. + throw datasetInvalidError( `[NativeSQLStrategy] join "${alias}" is not backed by a declared relationship on ` + `cube "${query.cube}". v1 only joins along relationships listed in the dataset's \`include\`.`, ); diff --git a/packages/spec/src/api/error-code-ledger.zod.ts b/packages/spec/src/api/error-code-ledger.zod.ts index 6ba5fd45d7..261e41c8c9 100644 --- a/packages/spec/src/api/error-code-ledger.zod.ts +++ b/packages/spec/src/api/error-code-ledger.zod.ts @@ -293,6 +293,7 @@ export const ERROR_CODE_LEDGER = { ], '@objectstack/service-analytics': [ 'CUBE_NOT_FOUND', + 'DATASET_INVALID', // [#5367] dataset/selection refusal raised by `dataset-refusal.ts` 'RAW_SQL_UNSUPPORTED', ], '@objectstack/service-datasource': [