Skip to content

Commit fce4c73

Browse files
os-zhuangclaude
andauthored
fix(service-analytics): gate where source fields — a filter over a missing field is 400 INVALID_FIELD, not a driver 500 (#5669) (#5740)
`ensureCube` carried two source-field gates — `assertMeasureFields` (#4437) and `assertDimensionFields` (#5520) — and none for the filter face. A `where` naming a field the object does not have compiled straight into the statement and came back as a driver error with no envelope: POST /analytics/query {"cube":"crm_account","measures":["count"], "where":{"bogus_col":"x"}} -> SELECT COUNT(*) AS "count" FROM "crm_account" WHERE bogus_col = $1 -> 500 {"code":"SQLITE_ERROR","message":"Internal server error"} `assertWhereFields` now runs after the other two on every `ensureCube` path, refusing before any SQL is built with the siblings' envelope: INVALID_FIELD/400 plus field/object/param='where'. `query`, `generateSql` and `queryDataset` (runtimeFilter and a dataset's own declared filter) are covered, and a rejected query leaves nothing in the cube registry. `/analytics/dataset/query` needed no change — #5352's envelope branch already carries a coded 4xx through — and the new rest-face test pins that end to end. Members are collected through `normalizeAnalyticsFilterTree` + `collectFilterLeaves`, the same pair both strategies compile the predicate with, so combinator nesting, `$`-operator keys, `$between` lowering, the nested relation dot flattening and the #5334 array spelling are read exactly as they will be compiled — not by a second walker that could drift from it. #5520's member->column resolution is extracted to the module-level `resolveMemberSource`, shared by both gates. Its new `kind` parameter is load-bearing rather than cosmetic: a filter member resolves through dimensions AND measures (what `resolveFieldSql` / `resolveFieldName(.., 'any')` do), so a dimensions-only lookup would have rejected `where: {revenue: {$gt: 100}}` on a cube declaring `measures.revenue = {sql: 'annual_revenue'}` — a query that works on both strategies today. Deliberately unchanged: filtering on a real field the cube never declared; a declared member followed to its real column; id/created_at/updated_at admitted; expression `sql`, dotted relation traversals and a probe-less host all stood down on; and the INVALID_FILTER family untouched — a `where` the normalizer refuses outright is not judged here, so those refusals stay where they already happen (#5352/#5367) and the draft-preview path, whose matcher never consults the normalizer, is not newly refused. Array `where` IS gated, and that is not #5353's territory: `inferCubeFromQuery` still skips it when minting the ad-hoc cube's dimension vocabulary, but since #5334 the array spelling lowers to the identical predicate (measured: both produce `WHERE bogus_col = $1`), so gating one spelling only would answer one mistake two ways. Fixes #5669 Claude-Session: https://claude.ai/code/session_01BWS4heBoAitLmzCLhcYdbK Co-authored-by: Claude <noreply@anthropic.com>
1 parent dadf542 commit fce4c73

4 files changed

Lines changed: 1282 additions & 41 deletions

File tree

Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,69 @@
1+
---
2+
"@objectstack/service-analytics": patch
3+
---
4+
5+
fix(service-analytics): an analytics `where` over a missing field answers 400 INVALID_FIELD, not a driver 500 (#5669)
6+
7+
`ensureCube` carried two source-field gates — `assertMeasureFields` (#4437,
8+
`param: 'measures'`) and `assertDimensionFields` (#5520,
9+
`param: 'dimensions' | 'timeDimensions'`) — and none for the filter face, the
10+
request key most likely to carry a hand-typed field name. A `where` naming a
11+
field the object does not have compiled straight into the statement and came
12+
back as a driver error with no envelope:
13+
14+
```
15+
POST /analytics/query {"cube":"crm_account","measures":["count"],"where":{"bogus_col":"x"}}
16+
→ SELECT COUNT(*) AS "count" FROM "crm_account" WHERE bogus_col = $1
17+
→ 500 {"code":"SQLITE_ERROR","message":"Internal server error"}
18+
19+
# the control group on the same route, already fixed by #4437 / #5520
20+
POST /analytics/query {"cube":"crm_account","measures":["count"],"dimensions":["bogus_dim"]}
21+
→ 400 {"code":"INVALID_FIELD","message":"Dimension 'bogus_dim' … "}
22+
```
23+
24+
A driver error class as the caller's `error.code` for a caller-shaped mistake is
25+
the ADR-0112 fault #4437 was filed about; the `/data` route has answered the same
26+
typo with a field-naming 400 since #4315/#4254.
27+
28+
**The gate.** `ensureCube` now runs `assertWhereFields` after the other two on
29+
every path, so a filter whose source column the backing object does not have is
30+
refused **before** any SQL is built, with the same envelope its two siblings
31+
use: `INVALID_FIELD` / 400 plus `field` / `object` / `param: 'where'`, and a
32+
message naming the field, the valid filter members and the object's known field
33+
list. `query`, `generateSql` and `queryDataset` (both `runtimeFilter` and a
34+
dataset's own declared `filter`) are covered, and a rejected query leaves
35+
nothing behind in the cube registry. `/analytics/dataset/query` needed no
36+
change: #5352's envelope branch already carries a coded 4xx through, which the
37+
new REST-face test pins end to end.
38+
39+
**Field names come from the SQL producer's own reader.** The members are
40+
collected through `normalizeAnalyticsFilterTree` + `collectFilterLeaves` — the
41+
same pair both strategies call to build the predicate — rather than by walking
42+
the raw `where` object. So `$and`/`$or`/`$not` nesting, `$`-prefixed operator
43+
keys, `$between` lowering, the `{owner: {region: 'NA'}}``owner.region`
44+
flattening and the #5334 array spelling are all read exactly as they will be
45+
compiled, in one place, instead of in a second walker that could drift from it.
46+
47+
**What deliberately did not change:**
48+
49+
- Filtering on a REAL field the cube never declared (`where: {phone: '555'}`)
50+
still works — the gate asks "does the *object* have this field", never "did the
51+
cube declare it".
52+
- A filter member resolves through `cube.dimensions` **and** `cube.measures`,
53+
which is what the strategies do: a cube declaring
54+
`measures.revenue = {sql: 'annual_revenue'}` still answers
55+
`where: {revenue: {$gt: 100}}` as `annual_revenue > ?`.
56+
- A declared member is followed to its real column, so a dimension `assessed`
57+
over column `assessed_at` is not judged by its own name.
58+
- `id` / `created_at` / `updated_at` stay admitted unconditionally, matching the
59+
data path's `resolveQueryFields`.
60+
- An expression `sql` (on the cube or on a member), a dotted relation traversal,
61+
and a host that wires no field-name probe are all stood down on, exactly as the
62+
measure and dimension gates stand down.
63+
- The `INVALID_FILTER` family is untouched. A `where` the normalizer refuses
64+
outright — an unknown operator, a zero-operator field constraint, an
65+
unlowerable filter array — is *not* judged here: the gate stands down and the
66+
refusal stays where it already happens (#5352 / #5367's geography). A field
67+
gate that cannot read the tree has nothing to say about it, and pulling those
68+
refusals forward would also have newly refused them on the draft-preview path,
69+
whose matcher never consults the normalizer.
Lines changed: 223 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,223 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* [#5669] `POST /analytics/dataset/query` — the caller's own view of the `where`
5+
* source-field gate.
6+
*
7+
* The third and last param of the defect #4437 (measures) and #5520 (dimensions)
8+
* closed one key at a time. A filter naming a field the object does not have
9+
* reached the driver, came back as a driver error with no envelope, and this
10+
* route answered `500 ANALYTICS_QUERY_FAILED` for what is a plain typo — the
11+
* same classification fault, on the request key most likely to carry a
12+
* hand-typed field name. Measured on this harness against `origin/main`:
13+
*
14+
* ```
15+
* {"dataset":…,"selection":{"measures":["account_count"],"runtimeFilter":{"bogus_col":"x"}}}
16+
* → SELECT COUNT(*) AS "account_count" FROM "crm_account" WHERE bogus_col = $1
17+
* → 500 ANALYTICS_QUERY_FAILED
18+
* ```
19+
*
20+
* Why this file exists next to the service-side pin
21+
* (`service-analytics`'s `where-source-field-gate.test.ts`): "the service throws
22+
* the right shape" and "the caller receives it" are different facts, separated by
23+
* this route's catch. The gate needs no rest-layer change at all — #5352's
24+
* envelope branch ① reads `code` + 4xx `status` and carries the verdict through —
25+
* and that is precisely the claim worth pinning end to end, because it is a
26+
* claim about a seam neither side's unit tests cross. So the provider here is a
27+
* REAL `AnalyticsService` whose driver double fails the way SQLite/knex does
28+
* (statement prefixed to the cause), not a mock that would assume half the seam.
29+
*
30+
* `runtimeFilter` is the load-bearing input for the same reason
31+
* `analytics-filter-refusal-envelope.test.ts` uses it: it is the
32+
* presentation-scope filter a dashboard widget carries, i.e. exactly the field
33+
* an author typos.
34+
*
35+
* ## Reverse verification, direction predicted BEFORE running
36+
*
37+
* Remove the three `assertWhereFields` calls from `ensureCube` and rebuild
38+
* `@objectstack/service-analytics` (this file exercises the BUILT package —
39+
* mutating sources without rebuilding proves nothing here): the three rejection
40+
* cases go RED, answering `500 ANALYTICS_QUERY_FAILED` again, while the positive
41+
* control and the `INVALID_FILTER` case — neither of which this gate produces —
42+
* stay GREEN. Ordinary direction. Predicted 3 red / 2 green; measured exactly
43+
* that, each red reading `expected 500 to be 400`.
44+
*
45+
* Note the "carries no generated SQL" case asserts the 400 as well as the
46+
* absence of a statement — #5520's file records why: with the gate gone, a
47+
* leak-free body proves nothing on its own, because the sibling fix (#5520's
48+
* sanitiser on the 500 branch) withholds the driver message anyway. Each fix
49+
* must be falsifiable on its own.
50+
*/
51+
52+
import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest';
53+
import type { Logger } from '@objectstack/spec/contracts';
54+
import { AnalyticsService } from '@objectstack/service-analytics';
55+
import { RestServer } from './rest-server';
56+
57+
// ── harness (the shape the two sibling analytics rest tests use) ─────────────
58+
59+
function mockServer() {
60+
return {
61+
get: vi.fn(), post: vi.fn(), put: vi.fn(), delete: vi.fn(), patch: vi.fn(),
62+
use: vi.fn(), listen: vi.fn().mockResolvedValue(undefined), close: vi.fn().mockResolvedValue(undefined),
63+
};
64+
}
65+
function mockProtocol() {
66+
return {
67+
getDiscovery: vi.fn().mockResolvedValue({ version: 'v0', endpoints: {} }),
68+
getMetaTypes: vi.fn().mockResolvedValue([]),
69+
getMetaItems: vi.fn().mockResolvedValue([]),
70+
};
71+
}
72+
function mockRes() {
73+
const res: any = { statusCode: 200, body: undefined };
74+
res.status = vi.fn((c: number) => { res.statusCode = c; return res; });
75+
res.json = vi.fn((b: any) => { res.body = b; return res; });
76+
res.end = vi.fn(() => res);
77+
return res;
78+
}
79+
80+
/** The dataset from the issue's repro — one declared dimension, one measure. */
81+
const dataset = {
82+
name: 'account_metrics',
83+
label: 'Account metrics',
84+
object: 'crm_account',
85+
dimensions: [{ name: 'industry', field: 'industry', type: 'string' }],
86+
measures: [{ name: 'account_count', aggregate: 'count' }],
87+
};
88+
89+
const ACCOUNT_FIELDS = ['id', 'name', 'phone', 'industry', 'annual_revenue'];
90+
91+
function buildRoute(analyticsProvider?: any) {
92+
const rest = new RestServer(
93+
mockServer() as any, mockProtocol() as any, { api: { requireAuth: false } } as any,
94+
undefined, undefined, undefined, undefined, undefined, undefined, undefined,
95+
undefined, undefined, undefined, undefined,
96+
analyticsProvider,
97+
);
98+
(rest as any).resolveExecCtx = async () => ({ userId: 'test-user' });
99+
rest.registerRoutes();
100+
return rest.getRoutes().find((r) => r.method === 'POST' && r.path.endsWith('/analytics/dataset/query'))!;
101+
}
102+
103+
/**
104+
* A REAL `AnalyticsService` on the native-SQL path whose driver double fails the
105+
* way the SQLite/knex one does: the statement prefixed to the cause. That is
106+
* what made the pre-fix 500 body carry the generated SQL, so the harness
107+
* reproduces it rather than asserting about a hypothetical message.
108+
*/
109+
function realAnalytics(): AnalyticsService {
110+
const silent: Logger = { debug() {}, info() {}, warn() {}, error() {} };
111+
return new AnalyticsService({
112+
logger: silent,
113+
queryCapabilities: () => ({ nativeSql: true, objectqlAggregate: false, inMemory: false }),
114+
executeRawSql: async (_object: string, sql: string) => {
115+
const bogus = /\b(bogus_col|dropped_column)\b/.exec(sql)?.[0];
116+
if (bogus) throw new Error(`${sql} - no such column: ${bogus}`);
117+
return [{ industry: 'tech', account_count: 3 }];
118+
},
119+
isRegisteredObject: (n: string) => n === 'crm_account',
120+
getObjectFieldNames: (n: string) => (n === 'crm_account' ? ACCOUNT_FIELDS : undefined),
121+
});
122+
}
123+
124+
async function post(route: any, body: unknown) {
125+
const res = mockRes();
126+
await route.handler({ method: 'POST', params: {}, headers: {}, body } as any, res);
127+
return res;
128+
}
129+
130+
let consoleError: ReturnType<typeof vi.spyOn>;
131+
beforeEach(() => {
132+
consoleError = vi.spyOn(console, 'error').mockImplementation(() => {});
133+
});
134+
afterEach(() => consoleError.mockRestore());
135+
136+
// ─────────────────────────────────────────────────────────────────────────────
137+
138+
describe('[#5669] a bogus `where` field answers 400 INVALID_FIELD, end to end', () => {
139+
it('names the field and the object — and is not a 500', async () => {
140+
const route = buildRoute(async () => realAnalytics());
141+
const res = await post(route, {
142+
dataset,
143+
selection: { measures: ['account_count'], runtimeFilter: { bogus_col: 'x' } },
144+
});
145+
146+
expect(res.statusCode).toBe(400);
147+
expect(res.body.code).toBe('INVALID_FIELD');
148+
// The defect, asserted as the defect rather than as the fix.
149+
expect(res.statusCode).not.toBe(500);
150+
expect(res.body.code).not.toBe('ANALYTICS_QUERY_FAILED');
151+
expect(String(res.body.message)).toMatch(/Filter member 'bogus_col' in 'where'/);
152+
expect(String(res.body.message)).toMatch(/object 'crm_account' does not have/);
153+
});
154+
155+
it('carries no generated SQL — because the statement was never built', async () => {
156+
const route = buildRoute(async () => realAnalytics());
157+
const res = await post(route, {
158+
dataset,
159+
selection: { measures: ['account_count'], runtimeFilter: { bogus_col: 'x' } },
160+
});
161+
162+
// BOTH halves of the one claim: the answer is the field-naming 400, and that
163+
// answer carries no statement. See the header for why the second alone is
164+
// not evidence.
165+
expect(res.statusCode).toBe(400);
166+
expect(res.body.code).toBe('INVALID_FIELD');
167+
const body = JSON.stringify(res.body);
168+
expect(body).not.toMatch(/SELECT/i);
169+
expect(body).not.toMatch(/FROM \\"/);
170+
expect(body).not.toMatch(/no such column/);
171+
});
172+
173+
it('answers the same way for a DATASET-declared filter over a dropped column', async () => {
174+
// The authored half of the same mistake: a dataset whose object dropped a
175+
// column its own declared `filter` still names.
176+
const route = buildRoute(async () => realAnalytics());
177+
const res = await post(route, {
178+
dataset: { ...dataset, filter: { dropped_column: 'x' } },
179+
selection: { measures: ['account_count'] },
180+
});
181+
182+
expect(res.statusCode).toBe(400);
183+
expect(res.body.code).toBe('INVALID_FIELD');
184+
expect(String(res.body.message)).toMatch(/constrains field 'dropped_column'/);
185+
});
186+
187+
it('a POSITIVE control: the same wiring with real fields → 200 with rows', async () => {
188+
// Without this the cases above could pass for any reason that makes the route
189+
// 400, including a pipeline that never reaches the gate. It also pins the
190+
// contract the gate must not kill: `phone` is a REAL column this dataset
191+
// never declared as a dimension, and filtering on it still works.
192+
const route = buildRoute(async () => realAnalytics());
193+
194+
const declared = await post(route, {
195+
dataset,
196+
selection: { measures: ['account_count'], dimensions: ['industry'], runtimeFilter: { industry: 'tech' } },
197+
});
198+
expect(declared.statusCode).toBe(200);
199+
expect(declared.body.rows).toEqual([{ industry: 'tech', account_count: 3 }]);
200+
201+
const undeclaredButReal = await post(route, {
202+
dataset,
203+
selection: { measures: ['account_count'], runtimeFilter: { phone: '555' } },
204+
});
205+
expect(undeclaredButReal.statusCode).toBe(200);
206+
});
207+
208+
it('does not disturb the INVALID_FILTER family #5352 / #5367 own', async () => {
209+
// A structurally-invalid filter is a DIFFERENT verdict from a
210+
// field-that-does-not-exist, and both must keep their own code: the gate
211+
// stands down on a `where` the normalizer refuses, so `INVALID_FILTER` still
212+
// comes from where it always did.
213+
const route = buildRoute(async () => realAnalytics());
214+
const res = await post(route, {
215+
dataset,
216+
selection: { measures: ['account_count'], runtimeFilter: { industry: { $sortOf: 'tech' } } },
217+
});
218+
219+
expect(res.statusCode).toBe(400);
220+
expect(res.body.code).toBe('INVALID_FILTER');
221+
expect(String(res.body.message)).toMatch(/\$sortOf/);
222+
});
223+
});

0 commit comments

Comments
 (0)