Skip to content

Commit 11ccde2

Browse files
committed
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`.
1 parent 8dbd2a8 commit 11ccde2

12 files changed

Lines changed: 919 additions & 42 deletions
Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,73 @@
1+
---
2+
"@objectstack/service-analytics": patch
3+
"@objectstack/rest": patch
4+
"@objectstack/spec": patch
5+
---
6+
7+
fix(analytics,rest): five dataset refusals declare `DATASET_INVALID` / 400 themselves, and the route's message-sniffing list shrinks to one entry (#5367)
8+
9+
`POST /analytics/dataset/query` answered `400 DATASET_INVALID` for six error
10+
families because the route recognised their **prose**, not because the errors
11+
said anything about themselves. #5352 gave the catch an ADR-0112 envelope branch
12+
(`error.code` + a 4xx `error.status`, read first) and had to leave a hardcoded
13+
list of message substrings behind it, since all six producers were still bare
14+
`throw new Error(…)`:
15+
16+
```
17+
/not declared in the dataset|not backed by a declared relationship|
18+
not supported by the v1 dataset runtime|read-scope-sql|
19+
not a selected dimension or measure|is not a subset of the selected dimensions/
20+
```
21+
22+
That made the HTTP status of six families a property of their wording.
23+
Rephrasing `dataset-compiler`'s "is not declared in the dataset's `include`" —
24+
no logic change — moved that refusal from 400 to 500, i.e. re-opened #5352 for a
25+
different family, and no test and no gate would have gone red. Prime Directive
26+
#12 permits an accommodation like that only while it is declared, loud, tested
27+
**and removable on a schedule**; #5366 delivered the first three and nothing
28+
carried the fourth.
29+
30+
**Five producers now declare their own verdict.** A new
31+
`dataset-refusal.ts` in `@objectstack/service-analytics` exports
32+
`datasetInvalidError` — the same shape as that package's existing
33+
`invalidFilterError` (`INVALID_FILTER` / 400) and `assertDimensionFields`
34+
(`INVALID_FIELD` / 400) — and five sites throw through it:
35+
36+
- `dataset-compiler.ts` — a measure whose aggregate the v1 runtime cannot lower;
37+
a dimension/measure traversing a relationship path the dataset never declared
38+
in `include`;
39+
- `dataset-executor.ts` — an `order` key that is not a selected dimension or
40+
measure; a `totals` grouping that is not a subset of the selected dimensions;
41+
- `native-sql-strategy.ts` — a join outside the dataset's declared allowlist.
42+
43+
Their five entries are gone from the route's list, which is now a single
44+
`read-scope-sql` test.
45+
46+
**`read-scope-sql` deliberately stays.** Its ten fail-closed refusals are RLS
47+
read-scope lowering failures whose inputs are an admin-authored policy and a
48+
compiler-generated join alias — not caller input — so `DATASET_INVALID` ("your
49+
request is invalid") may well be the wrong verdict and choosing the right one is
50+
a separate judgement, still tracked by #5367. Deleting the entry before that
51+
judgement lands would regress those ten from `400 DATASET_INVALID` to 500.
52+
53+
**No outward behaviour change for the five.** They answered
54+
`400 DATASET_INVALID` before and answer `400 DATASET_INVALID` now, with the same
55+
message; what changed is the mechanism, from message-matching to the producer's
56+
own declaration. The one visible difference is for a bare `Error` that merely
57+
*resembles* one of those messages: it is no longer promoted to a 400. That is the
58+
point — a phrase is no longer a classification.
59+
60+
`DATASET_INVALID` is registered in `ERROR_CODE_LEDGER` under
61+
`@objectstack/service-analytics` as well as `@objectstack/rest` (provenance, per
62+
ADR-0112 D3; the code itself is unchanged and the union does not grow), and the
63+
constructor types it as `RegisteredErrorCode` so an unregistered code is a
64+
compile error rather than a body some route rejects at runtime.
65+
66+
Coverage: `dataset-refusal-envelope.test.ts` (service-analytics) pins each of the
67+
five refusals against its real producer — the refusal SET first, green before and
68+
after, then the envelope; `analytics-dataset-refusal-envelope.test.ts` (rest)
69+
drives all five end-to-end through a real `AnalyticsService` with positive
70+
controls on both the aggregate and raw-SQL paths; and
71+
`analytics-filter-refusal-envelope.test.ts` pins the deletion in both directions
72+
— the five messages answer 400 when enveloped and 500 when bare, so re-adding a
73+
regex entry turns it red.

packages/rest/src/analytics-dataset-dimension-gate.test.ts

Lines changed: 25 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -295,16 +295,38 @@ describe('[#5520] the 500 body no longer ships driver internals', () => {
295295
expect(a.body.code).toBe('INVALID_FILTER');
296296
expect(String(a.body.message)).toMatch(/\$sortOf/);
297297

298-
// …② and the transitional message list still answers 400 DATASET_INVALID
299-
// with its message intact. #5367 owns that list; this change is 5xx-only.
298+
// …② and so does a dataset refusal, with its message intact.
299+
//
300+
// [#5367] This half used to send a BARE `Error` reading "… is not declared
301+
// in the dataset." and rely on the route's transitional message list to
302+
// classify it. #5367 enveloped that producer (`dataset-compiler` now throws
303+
// `datasetInvalidError`) and deleted the list entry, so the same refusal is
304+
// now carried by branch ① — same outward answer, chosen by reading the error
305+
// instead of by matching its prose. This change is still 5xx-only.
300306
const b = await post(
301307
buildRoute(async () =>
302-
throwingAnalytics(new Error('[dataset-compiler] dimension "region" is not declared in the dataset.')),
308+
throwingAnalytics(
309+
Object.assign(
310+
new Error('[dataset-compiler] dimension "region" references relationship path "account" via "account.region", but "account" is not declared in the dataset\'s `include`.'),
311+
{ code: 'DATASET_INVALID', status: 400 },
312+
),
313+
),
303314
),
304315
{ dataset, selection: { measures: ['account_count'], dimensions: ['industry'] } },
305316
);
306317
expect(b.statusCode).toBe(400);
307318
expect(b.body.code).toBe('DATASET_INVALID');
308319
expect(String(b.body.message)).toMatch(/not declared in the dataset/);
320+
321+
// …③ and the ONE message-list entry #5367 deliberately left in place still
322+
// answers 400 for `read-scope-sql`'s bare fail-closed refusals.
323+
const c = await post(
324+
buildRoute(async () =>
325+
throwingAnalytics(new Error('[read-scope-sql] unsupported operator "$regex" on "owner" (fail-closed).')),
326+
),
327+
{ dataset, selection: { measures: ['account_count'], dimensions: ['industry'] } },
328+
);
329+
expect(c.statusCode).toBe(400);
330+
expect(c.body.code).toBe('DATASET_INVALID');
309331
});
310332
});
Lines changed: 256 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,256 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* [#5367] `POST /analytics/dataset/query` — five dataset refusals reach the
5+
* caller as `400 DATASET_INVALID` because their PRODUCER says so, not because
6+
* this route recognised their prose.
7+
*
8+
* ## The seam, and why this file boots the REAL analytics service
9+
*
10+
* #5352 / PR #5366 gave the route an envelope branch (`error.code` + a 4xx
11+
* `error.status`, read first) and left a transitional list of message substrings
12+
* behind it, because six refusal families were still bare `throw new Error(…)`:
13+
*
14+
* ```
15+
* /not declared in the dataset|not backed by a declared relationship|
16+
* not supported by the v1 dataset runtime|read-scope-sql|
17+
* not a selected dimension or measure|is not a subset of the selected dimensions/
18+
* ```
19+
*
20+
* With that list in place the HTTP status of six families was a property of their
21+
* **wording**: rephrasing `dataset-compiler`'s "is not declared in the dataset's
22+
* `include`" — no logic change — dropped the refusal from 400 to 500, and no test
23+
* and no gate would have gone red. Prime Directive #12 allows such an
24+
* accommodation only while it is declared, loud, tested **and removable on a
25+
* schedule**; #5366 delivered the first three. #5367 is the schedule: five
26+
* producers now throw `datasetInvalidError` (`DATASET_INVALID` / 400) and their
27+
* five entries are gone from the list.
28+
*
29+
* The claim has two halves and either alone reads as fixed:
30+
*
31+
* - **B** — the producer throws the envelope. Pinned per site, against the real
32+
* producer, in `service-analytics`'s `dataset-refusal-envelope.test.ts`.
33+
* - **A** — the route classifies on that envelope rather than on the message.
34+
* Pinned in `analytics-filter-refusal-envelope.test.ts`, including the
35+
* five deleted entries asserted in both directions.
36+
*
37+
* A unit test on either side can be green while an author still sees a 500. So
38+
* this file asserts the SEAM: the analytics provider is a real `AnalyticsService`
39+
* compiling a real dataset, the error crossing into the catch is the one the real
40+
* `dataset-compiler` / `dataset-executor` / `native-sql-strategy` throws, and
41+
* nothing here asserts a shape it also constructs.
42+
*
43+
* ## Reverse verification, direction predicted BEFORE running
44+
*
45+
* Restore ANY of the five `throw new Error(…)` calls in `service-analytics` (and
46+
* rebuild it — this file exercises the BUILT package) and that family's case here
47+
* goes RED with `500 ANALYTICS_QUERY_FAILED`, because the route no longer carries
48+
* a message entry that would rescue it. The direction is plain red, not the
49+
* inverted/extra-diagnostic shapes: the canonical envelope branch is FIRST in the
50+
* catch and the fallback that used to answer for these families is gone, so
51+
* "producer stops declaring" has exactly one outward consequence. Confirmed by
52+
* running it (see the PR).
53+
*
54+
* Positive controls sit next to the refusals so a case cannot pass merely because
55+
* the wiring never reached the producer.
56+
*/
57+
58+
import { describe, it, expect, vi } from 'vitest';
59+
import type { Logger } from '@objectstack/spec/contracts';
60+
import { AnalyticsService } from '@objectstack/service-analytics';
61+
import { RestServer } from './rest-server';
62+
63+
// ── harness (the shape `analytics-filter-refusal-envelope.test.ts` uses) ──────
64+
65+
function mockServer() {
66+
return {
67+
get: vi.fn(), post: vi.fn(), put: vi.fn(), delete: vi.fn(), patch: vi.fn(),
68+
use: vi.fn(), listen: vi.fn().mockResolvedValue(undefined), close: vi.fn().mockResolvedValue(undefined),
69+
};
70+
}
71+
function mockProtocol() {
72+
return {
73+
getDiscovery: vi.fn().mockResolvedValue({ version: 'v0', endpoints: {} }),
74+
getMetaTypes: vi.fn().mockResolvedValue([]),
75+
getMetaItems: vi.fn().mockResolvedValue([]),
76+
};
77+
}
78+
function mockRes() {
79+
const res: any = { statusCode: 200, body: undefined };
80+
res.status = vi.fn((c: number) => { res.statusCode = c; return res; });
81+
res.json = vi.fn((b: any) => { res.body = b; return res; });
82+
res.end = vi.fn(() => res);
83+
return res;
84+
}
85+
86+
/** Build a RestServer over an analytics provider (positional arg #15). */
87+
function buildRoute(analyticsProvider?: any) {
88+
const rest = new RestServer(
89+
mockServer() as any, mockProtocol() as any, { api: { requireAuth: false } } as any,
90+
undefined, undefined, undefined, undefined, undefined, undefined, undefined,
91+
undefined, undefined, undefined, undefined,
92+
analyticsProvider,
93+
);
94+
(rest as any).resolveExecCtx = async () => ({ userId: 'test-user' });
95+
rest.registerRoutes();
96+
return rest.getRoutes().find((r) => r.method === 'POST' && r.path.endsWith('/analytics/dataset/query'))!;
97+
}
98+
99+
async function post(route: any, body: unknown) {
100+
const res = mockRes();
101+
await route.handler({ method: 'POST', params: {}, headers: {}, body } as any, res);
102+
return res;
103+
}
104+
105+
const silent: Logger = { debug() {}, info() {}, warn() {}, error() {} };
106+
107+
/**
108+
* A real `AnalyticsService` on the ObjectQL aggregate path — one fixed bucket, so
109+
* a selection that gets far enough to touch data answers 200. That is what makes
110+
* the refusals meaningful: they fail on the dataset/selection, on a route that
111+
* demonstrably works otherwise.
112+
*/
113+
function aggregateAnalytics(): AnalyticsService {
114+
return new AnalyticsService({
115+
logger: silent,
116+
queryCapabilities: () => ({ nativeSql: false, objectqlAggregate: true, inMemory: false }),
117+
executeAggregate: async () => [{ stage: 'won', revenue: 100 }],
118+
isRegisteredObject: () => true,
119+
});
120+
}
121+
122+
/** A real `AnalyticsService` on the raw-SQL path — the only one that enforces the join allowlist. */
123+
function nativeAnalytics(): AnalyticsService {
124+
return new AnalyticsService({
125+
logger: silent,
126+
queryCapabilities: () => ({ nativeSql: true, objectqlAggregate: false, inMemory: false }),
127+
executeRawSql: async () => [{ stage: 'won', revenue: 100 }],
128+
isRegisteredObject: () => true,
129+
});
130+
}
131+
132+
/** A valid single-object dataset — no `include`, so nothing here needs a join. */
133+
const dataset = {
134+
name: 'pipeline',
135+
label: 'Pipeline',
136+
object: 'crm_opportunity',
137+
dimensions: [{ name: 'stage', field: 'stage', type: 'string' }],
138+
measures: [{ name: 'revenue', aggregate: 'sum', field: 'amount' }],
139+
};
140+
const selection = { dimensions: ['stage'], measures: ['revenue'] };
141+
142+
// ─────────────────────────────────────────────────────────────────────────────
143+
144+
describe('[#5367] a dataset refusal answers 400 DATASET_INVALID from its own envelope', () => {
145+
/**
146+
* One row per entry #5367 removed from the route's message list, driven through
147+
* the real producer. `listEntry` records the substring that used to classify it
148+
* — the audit trail that the deletion and this coverage are the same set.
149+
*/
150+
const CASES: Array<{
151+
name: string;
152+
listEntry: string;
153+
body: unknown;
154+
analytics: () => AnalyticsService;
155+
message: RegExp;
156+
}> = [
157+
{
158+
name: 'dataset-compiler: an aggregate the v1 runtime cannot lower',
159+
listEntry: 'not supported by the v1 dataset runtime',
160+
analytics: aggregateAnalytics,
161+
body: {
162+
dataset: {
163+
...dataset,
164+
measures: [{ name: 'names', aggregate: 'string_agg', field: 'name' }],
165+
},
166+
selection: { dimensions: ['stage'], measures: ['names'] },
167+
},
168+
message: /measure "names" uses aggregate "string_agg" which is not supported by the v1 dataset runtime/,
169+
},
170+
{
171+
name: 'dataset-compiler: a dimension traversing an undeclared relationship path',
172+
listEntry: 'not declared in the dataset',
173+
analytics: aggregateAnalytics,
174+
body: {
175+
dataset: {
176+
...dataset,
177+
include: [],
178+
dimensions: [{ name: 'region', field: 'account.region', type: 'string' }],
179+
},
180+
selection: { dimensions: ['region'], measures: ['revenue'] },
181+
},
182+
message: /"account" is not declared in the dataset's `include`/,
183+
},
184+
{
185+
name: 'dataset-executor: an order key that is not selected',
186+
listEntry: 'not a selected dimension or measure',
187+
analytics: aggregateAnalytics,
188+
body: { dataset, selection: { ...selection, order: { profit: 'desc' } } },
189+
message: /order key\(s\) "profit" not a selected dimension or measure/,
190+
},
191+
{
192+
name: 'dataset-executor: a totals grouping outside the selection',
193+
listEntry: 'is not a subset of the selected dimensions',
194+
analytics: aggregateAnalytics,
195+
body: { dataset, selection: { ...selection, totals: { groupings: [['region']] } } },
196+
message: /totals grouping \[region\] is not a subset of the selected dimensions/,
197+
},
198+
{
199+
// The caller-shaped trigger: the DATASET declares no `include`, and the
200+
// SELECTION names a dotted dimension, which `lookupMember`'s synthetic
201+
// relation fallback turns into a join at alias `account`.
202+
name: 'native-sql-strategy: a selection naming a join outside the allowlist',
203+
listEntry: 'not backed by a declared relationship',
204+
analytics: nativeAnalytics,
205+
body: { dataset, selection: { dimensions: ['account.region'], measures: ['revenue'] } },
206+
message: /join "account" is not backed by a declared relationship on cube "pipeline"/,
207+
},
208+
];
209+
210+
for (const c of CASES) {
211+
it(`${c.name} → 400 DATASET_INVALID`, async () => {
212+
const route = buildRoute(async () => c.analytics());
213+
const res = await post(route, c.body);
214+
215+
expect(res.statusCode).toBe(400);
216+
expect(res.body.code).toBe('DATASET_INVALID');
217+
// The message survives intact so the author can act on it, and the body is
218+
// the 4xx shape (`message`), not the 5xx one (`error`).
219+
expect(String(res.body.message)).toMatch(c.message);
220+
expect(res.body.error).toBeUndefined();
221+
// The defect, asserted as the defect rather than as the fix.
222+
expect(res.statusCode).not.toBe(500);
223+
expect(res.body.code).not.toBe('ANALYTICS_QUERY_FAILED');
224+
});
225+
}
226+
227+
it('covers exactly the five entries #5367 deleted from the message list', () => {
228+
expect(CASES.map((c) => c.listEntry).sort()).toEqual([
229+
'is not a subset of the selected dimensions',
230+
'not a selected dimension or measure',
231+
'not backed by a declared relationship',
232+
'not declared in the dataset',
233+
'not supported by the v1 dataset runtime',
234+
]);
235+
});
236+
237+
it('POSITIVE control (aggregate path): the same wiring, a valid selection → 200 with rows', async () => {
238+
const route = buildRoute(async () => aggregateAnalytics());
239+
const res = await post(route, { dataset, selection });
240+
expect(res.statusCode).toBe(200);
241+
expect(res.body.rows).toEqual([{ stage: 'won', revenue: 100 }]);
242+
});
243+
244+
it('POSITIVE control (raw-SQL path): a DECLARED relationship still joins → 200 with rows', async () => {
245+
// The allowlist case's twin: `include: ['account']` makes the same dotted
246+
// selection legal, so case ⑤ above is a verdict about the allowlist rather
247+
// than about dotted members being rejected outright.
248+
const route = buildRoute(async () => nativeAnalytics());
249+
const res = await post(route, {
250+
dataset: { ...dataset, include: ['account'] },
251+
selection: { dimensions: ['account.region'], measures: ['revenue'] },
252+
});
253+
expect(res.statusCode).toBe(200);
254+
expect(res.body.rows).toEqual([{ stage: 'won', revenue: 100 }]);
255+
});
256+
});

0 commit comments

Comments
 (0)