diff --git a/.changeset/analytics-filter-refusal-envelope.md b/.changeset/analytics-filter-refusal-envelope.md new file mode 100644 index 0000000000..db1c2d0cc0 --- /dev/null +++ b/.changeset/analytics-filter-refusal-envelope.md @@ -0,0 +1,61 @@ +--- +"@objectstack/service-analytics": patch +"@objectstack/rest": patch +--- + +fix(analytics,rest): an analytics filter refusal reaches the caller as `400 INVALID_FILTER`, not `500 ANALYTICS_QUERY_FAILED` (#5352) + +Misspell an operator in a dashboard widget's filter and analytics refuses it — +correctly, and loudly, which is the posture #3948 / #5240 / #5325 / #5334 each +argued for one refusal at a time: dropping a predicate the compiler cannot +express does not narrow the query, it **widens** it to rows the author excluded, +and a chart drawn over the whole dataset looks like a working chart. + +The refusal never reached the author. It landed as `500 ANALYTICS_QUERY_FAILED` +— read as "the platform is broken" rather than "your filter has a typo", and +counted by ops alerting as a 5xx. The identical mistake on `find()` has answered +`400 INVALID_FILTER` since #3948, so one authoring error had two wire shapes, +chosen by which face happened to catch it. + +**One defect, two halves — either alone leaves it unfixed.** + +- **Producer** (`filter-normalizer.ts`): seven of its nine refusals were bare + `throw new Error(…)` carrying no `code`/`status`. All nine now go through the + `invalidFilterError` helper #5334 introduced (`INVALID_FILTER` / 400), which + becomes the module's only way to refuse. +- **Consumer** (`rest-server.ts`, `POST /analytics/dataset/query`): the catch + discarded `error.code` / `error.status` and re-derived the classification from + a hardcoded list of message substrings — so a producer that took ADR-0112 + seriously was punished for it. It now reads the envelope **first**; the + substring list is demoted to a fallback for the families that still carry no + envelope. + +**Observable behaviour change — read this if you alert or retry on status.** +The same request that returned `500 ANALYTICS_QUERY_FAILED` now returns +`400 INVALID_FILTER` (and, for two neighbouring conditions whose producers +already declared an envelope this route was discarding, `400 INVALID_FIELD` for +a measure over a field the object does not have, `404 CUBE_NOT_FOUND` for an +unregistered cube). Monitoring that counted these as server faults will see the +5xx rate drop and a 4xx rate appear; a client that retries on 5xx will stop +retrying a request that could only ever fail the same way. Both are the intended +correction — the condition was always the caller's mistake — but they are +visible, so they are stated rather than buried. + +**Which inputs are refused did not change.** This changes the SHAPE of the +error and nothing about the judgement that produced it: no refusal condition +was touched, no input that used to compile now refuses, and no input that used +to refuse now compiles. That claim is pinned input-by-input (refusals *and* +accepted inputs with their compiled trees) in +`filter-refusal-envelope.test.ts`, which is green both before and after the +change — only the envelope assertions move. + +The message-substring list survives on purpose. All six of its entries were +re-verified as bare `Error`s (`dataset-compiler.ts`, `native-sql-strategy.ts`, +`dataset-executor.ts`, `read-scope-sql.ts`), so deleting it would regress those +families from `400 DATASET_INVALID` to 500. It is a placeholder for their +enveloping, not a second classification mechanism, and it is now documented as +such: a new refusal should carry a `code`/`status` and be served by the +envelope branch for free. The passthrough is deliberately **4xx-only** and +requires **both** `code` and `status`, so an internal fault can never be +re-labelled as the caller's fault, and this route never invents a code a +producer failed to supply. diff --git a/packages/rest/package.json b/packages/rest/package.json index 5ee6493b58..c753ac5847 100644 --- a/packages/rest/package.json +++ b/packages/rest/package.json @@ -31,6 +31,7 @@ "devDependencies": { "@objectstack/metadata-protocol": "workspace:*", "@objectstack/objectql": "workspace:*", + "@objectstack/service-analytics": "workspace:*", "@types/node": "^26.1.2", "typescript": "^6.0.3", "vitest": "^4.1.10" diff --git a/packages/rest/src/analytics-filter-refusal-envelope.test.ts b/packages/rest/src/analytics-filter-refusal-envelope.test.ts new file mode 100644 index 0000000000..343f46c4bf --- /dev/null +++ b/packages/rest/src/analytics-filter-refusal-envelope.test.ts @@ -0,0 +1,313 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#5352] `/analytics/dataset/query` answers a filter refusal as the caller's + * mistake (`400 INVALID_FILTER`), not as a platform fault + * (`500 ANALYTICS_QUERY_FAILED`). + * + * ## The seam, and why this file boots the REAL analytics service + * + * The defect had two halves and either one alone reads as fixed: + * + * - **B** — `filter-normalizer.ts` refused a malformed filter with a bare + * `throw new Error(…)`, carrying no `code`/`status`. + * - **A** — this route's catch discarded `error.code` / `error.status` and + * re-derived the classification from a hardcoded list of message + * substrings, which no filter refusal matched. + * + * So a unit test on either side can be green while an author still sees a 500: + * mock the service and half B is assumed; assert on the thrown error and half A + * is assumed. `analytics-routes.test.ts` next door mocks `queryDataset` because + * its subjects (dataset resolution, decoration stripping, schema validation) + * live entirely on this side of the seam. This file's subject IS the seam, so + * the provider is a real `AnalyticsService` and the error crossing into the + * catch is the real one `normalizeAnalyticsFilterTree` throws — nothing here + * asserts a shape it also constructs. + * + * `runtimeFilter` is the load-bearing input: it is the presentation-scope + * filter a dashboard widget carries, i.e. exactly the field an author typos. + * + * ## What must NOT change + * + * 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. + * 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 + * be re-labelled with a code of its own choosing. + */ + +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 ────────────────────────────────────────────────────────────────── + +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; +} + +/** A 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'] }; + +/** 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'))!; +} + +/** + * A REAL `AnalyticsService` on the ObjectQL aggregate path. + * + * `executeAggregate` returns a fixed bucket, so a query that gets far enough to + * touch data succeeds — which is what makes the refusal cases meaningful: they + * fail on the FILTER, on a route that demonstrably answers 200 otherwise. + */ +function realAnalytics(): AnalyticsService { + const silent: Logger = { debug() {}, info() {}, warn() {}, error() {} }; + return new AnalyticsService({ + logger: silent, + queryCapabilities: () => ({ nativeSql: false, objectqlAggregate: true, inMemory: false }), + executeAggregate: async () => [{ stage: 'won', revenue: 100 }], + isRegisteredObject: () => true, + }); +} + +/** POST a body at the route and return the recorded response. */ +async function post(route: any, body: unknown) { + const res = mockRes(); + await route.handler({ method: 'POST', params: {}, headers: {}, body } as any, res); + return res; +} + +// ───────────────────────────────────────────────────────────────────────────── + +describe('[#5352] POST /analytics/dataset/query — a filter refusal reaches the caller as 400', () => { + it('a misspelled operator in a widget filter → 400 INVALID_FILTER (was 500 ANALYTICS_QUERY_FAILED)', async () => { + const route = buildRoute(async () => realAnalytics()); + const res = await post(route, { + dataset, + selection: { ...selection, runtimeFilter: { stage: { $sortOf: 'won' } } }, + }); + + expect(res.statusCode).toBe(400); + expect(res.body.code).toBe('INVALID_FILTER'); + // The two halves of 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'); + // The message still names the operator, so the author can act on it. + expect(String(res.body.message)).toMatch(/Unsupported filter operator "\$sortOf" on "stage"/); + }); + + it('a POSITIVE control: the same wiring, a valid filter → 200 with rows', async () => { + // Without this, the case above could pass for any reason that makes the + // route 400 — including the pipeline never reaching the filter normalizer. + const route = buildRoute(async () => realAnalytics()); + const res = await post(route, { + dataset, + selection: { ...selection, runtimeFilter: { stage: { $eq: 'won' } } }, + }); + + expect(res.statusCode).toBe(200); + expect(res.body.rows).toEqual([{ stage: 'won', revenue: 100 }]); + }); + + // The other refusal spellings an author reaches through the same field. Each + // is a real refusal from the real normalizer, crossing the real seam. + const REFUSALS: Array<{ name: string; runtimeFilter: unknown; message: RegExp }> = [ + { + name: 'a field constraint with zero operators (#5240)', + runtimeFilter: { stage: {} }, + message: /carries a field constraint with zero operators/, + }, + { + name: 'a $between with one bound', + runtimeFilter: { amount: { $between: [10] } }, + message: /needs a two-element \[min, max\] array/, + }, + { + name: 'an empty $or', + runtimeFilter: { $or: [] }, + message: /"\$or" requires a non-empty array/, + }, + { + name: 'an $or branch that is not a filter object', + runtimeFilter: { $or: [{ stage: 'won' }, 'nope'] }, + message: /branches must be filter objects/, + }, + { + name: 'a $not of a non-object', + runtimeFilter: { $not: 5 }, + message: /"\$not" requires a filter object/, + }, + { + name: 'an unsupported top-level operator', + runtimeFilter: { $nor: [{ stage: 'won' }] }, + message: /Unsupported top-level filter operator "\$nor"/, + }, + ]; + + for (const c of REFUSALS) { + it(`${c.name} → 400 INVALID_FILTER`, async () => { + const route = buildRoute(async () => realAnalytics()); + const res = await post(route, { dataset, selection: { ...selection, runtimeFilter: c.runtimeFilter } }); + expect(res.statusCode).toBe(400); + expect(res.body.code).toBe('INVALID_FILTER'); + expect(String(res.body.message)).toMatch(c.message); + }); + } +}); + +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 }> = [ + { + 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`.', + }, + { + name: 'native-sql-strategy: join outside the allowlist', + message: '[NativeSQLStrategy] join "account" is not backed by a declared relationship on cube "pipeline".', + }, + { + 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.', + }, + { + name: 'dataset-executor: totals grouping outside the selection', + message: '[dataset-executor] totals grouping [region] is not a subset of the selected dimensions — unknown: region.', + }, + ]; + + 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)) })); + const res = await post(route, { dataset, selection }); + expect(res.statusCode).toBe(400); + expect(res.body.code).toBe('DATASET_INVALID'); + }); + } +}); + +describe('[#5352] reading the envelope did not turn every failure into a 400', () => { + it('a genuine internal fault is still 500 ANALYTICS_QUERY_FAILED', async () => { + // Nothing filter-shaped, no envelope, no message the list matches — the + // class the 500 exists for. + const route = buildRoute(async () => ({ + queryDataset: vi.fn().mockRejectedValue(new Error('ECONNRESET: socket hang up while reading from the analytics datasource')), + })); + const res = await post(route, { dataset, selection }); + + expect(res.statusCode).toBe(500); + expect(res.body.code).toBe('ANALYTICS_QUERY_FAILED'); + }); + + it('a 5xx-status error is NOT passed through — an internal fault keeps the 500 envelope', async () => { + // Deliberate asymmetry: the passthrough is 4xx-only, so a producer cannot + // re-label a server fault with a code of its own and slip past the + // `logError` line that makes it visible to operators. + const err = Object.assign(new Error('upstream analytics warehouse is unavailable'), { + code: 'WAREHOUSE_UNAVAILABLE', + status: 503, + }); + const route = buildRoute(async () => ({ queryDataset: vi.fn().mockRejectedValue(err) })); + const res = await post(route, { dataset, selection }); + + expect(res.statusCode).toBe(500); + expect(res.body.code).toBe('ANALYTICS_QUERY_FAILED'); + }); + + it('a HALF envelope (4xx status, no code) is not honoured — this route invents no code', async () => { + // ADR-0112's point is that the PRODUCER names the condition. A status with + // no code is a producer bug; answering it with a code chosen here would be + // the consumer-side leniency the ADR exists to remove, and would hide the + // bug behind a plausible wire shape. + const err = Object.assign(new Error('something was rejected, unspecified'), { status: 400 }); + const route = buildRoute(async () => ({ queryDataset: vi.fn().mockRejectedValue(err) })); + const res = await post(route, { dataset, selection }); + + expect(res.statusCode).toBe(500); + expect(res.body.code).toBe('ANALYTICS_QUERY_FAILED'); + }); +}); + +describe('[#5352] the envelope is read generically — not by an allowlist of codes', () => { + // A code-specific branch (`if (code === 'INVALID_FILTER')`) would be the + // message-sniffing anti-pattern in new clothes. These two producers already + // DECLARE their answer in their own doc comments — `INVALID_FIELD`/400 so the + // analytics face can answer a typo'd measure the way `/data` does (#4437), + // `CUBE_NOT_FOUND`/404 so "no such cube" does not reach the driver as a table + // (#3867) — and this route was discarding both. + it('a measure over a field the object does not have → 400 INVALID_FIELD (#4437)', async () => { + const err = Object.assign(new Error("Measure 'ghost_sum' on cube 'pipeline' aggregates field 'ghost', which object 'crm_opportunity' does not have."), { + code: 'INVALID_FIELD', + 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('INVALID_FIELD'); + }); + + it('an unregistered cube → 404 CUBE_NOT_FOUND (#3867)', async () => { + const err = Object.assign(new Error("Cube 'nope' not found: no cube is registered under that name."), { + code: 'CUBE_NOT_FOUND', + status: 404, + }); + const route = buildRoute(async () => ({ queryDataset: vi.fn().mockRejectedValue(err) })); + const res = await post(route, { dataset, selection }); + + expect(res.statusCode).toBe(404); + expect(res.body.code).toBe('CUBE_NOT_FOUND'); + }); +}); diff --git a/packages/rest/src/rest-server.ts b/packages/rest/src/rest-server.ts index b64d0899ae..d0c40264b2 100644 --- a/packages/rest/src/rest-server.ts +++ b/packages/rest/src/rest-server.ts @@ -6078,8 +6078,52 @@ export class RestServer { res.json(result); } catch (error: any) { const msg = String(error?.message ?? error ?? ''); + // ── [#5352] ① The ADR-0112 envelope, read FIRST ────────── + // A thrown error that already carries `code` + a 4xx + // `status` has ANSWERED the classification question. This + // route used to discard both and re-derive the answer from + // the message text below, so every producer that took + // ADR-0112 seriously was punished for it: analytics' + // filter refusals (`INVALID_FILTER`/400 — a misspelled + // operator in a dashboard widget, #3948/#5240/#5325/#5334), + // the measure source-field gate (`INVALID_FIELD`/400, + // #4437) and the cube-existence gate (`CUBE_NOT_FOUND`/404, + // #3867) all landed as `500 ANALYTICS_QUERY_FAILED` — read + // by the author as "the platform is broken" and by ops + // alerting as a 5xx. The same mistakes answer 400 on + // `/data`; one condition must not get two wire shapes + // because a different face caught it. + // + // BOTH halves are required, deliberately. A 4xx status with + // no code would force this route to invent one, which is + // the consumer-side leniency ADR-0112 exists to remove — a + // producer that ships half an envelope has a bug of its own + // and should be found, not papered over here. + // + // 4xx ONLY: a 5xx-status error keeps going through the + // `ANALYTICS_QUERY_FAILED` envelope below, so an internal + // fault can never be re-labelled as the caller's fault (and + // keeps its `logError` line). + const envelopeStatus = typeof error?.status === 'number' ? error.status : undefined; + const envelopeCode = typeof error?.code === 'string' && error.code.length > 0 ? error.code : undefined; + if (envelopeStatus !== undefined && envelopeStatus >= 400 && envelopeStatus < 500 && envelopeCode) { + 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. + // + // ⚠️ 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)) { return res.status(400).json({ code: 'DATASET_INVALID', message: msg.slice(0, 1000) }); } diff --git a/packages/services/service-analytics/src/__tests__/filter-refusal-envelope.test.ts b/packages/services/service-analytics/src/__tests__/filter-refusal-envelope.test.ts new file mode 100644 index 0000000000..92d8897ba5 --- /dev/null +++ b/packages/services/service-analytics/src/__tests__/filter-refusal-envelope.test.ts @@ -0,0 +1,242 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#5352] Every analytics filter refusal carries the ADR-0112 envelope — and + * the refusal SET did not move. + * + * ## What was wrong + * + * `filter-normalizer.ts` refuses nine distinct malformed filters, and #3948 / + * #5240 / #5325 / #5334 each argued the same case for one of them: a filter the + * compiler cannot express must be REFUSED, because dropping the predicate does + * not narrow the query, it WIDENS it to rows the author excluded — a chart + * drawn over the whole dataset that looks like a working chart. + * + * All correct, and all invisible to the caller. Seven of the nine refusals were + * bare `throw new Error(…)`. `rest-server.ts`'s `/analytics/dataset/query` + * catch had no `code`/`status` to read, so it fell through to + * `500 ANALYTICS_QUERY_FAILED` — which an author reads as "the platform is + * broken" (not "your filter has a typo") and ops alerting counts as a 5xx. + * The SAME misspelled operator on `find()` has answered `400 INVALID_FILTER` + * since #3948, so one authoring mistake had two wire shapes chosen by which + * face happened to catch it. + * + * ## 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 the accepted ones compile to. Every assertion in it passes both BEFORE + * and AFTER #5352 — that is its entire job. #5352 changes 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 refusal carries the ADR-0112 envelope')` is the change. Run + * it against pre-#5352 code and every case fails with `code` / `status` + * `undefined`, while the block above stays green. + * + * The nine sites are enumerated deliberately: #5352's body listed four bullets + * (five sites), but a half-enveloped module is indistinguishable from an + * unenveloped one at the REST boundary — an author who writes `{$not: 5}` + * deserves the same 400 as one who writes `{$nott: {…}}`. + */ + +import { describe, it, expect } from 'vitest'; +import { normalizeAnalyticsFilterTree } from '../strategies/filter-normalizer.js'; + +/** The ADR-0112 fields a refusal must carry. */ +interface FilterRefusal extends Error { + code?: unknown; + status?: unknown; +} + +/** Run the normalizer over a `where` and return the error it threw, if any. */ +function refusalFor(where: unknown): FilterRefusal | undefined { + try { + normalizeAnalyticsFilterTree({ where }); + return undefined; + } catch (e) { + return e as FilterRefusal; + } +} + +/** + * Every refusing site in `filter-normalizer.ts`, one input each. + * + * `issueBullet` records whether #5352's body named the site. The two `false` + * rows are the sites the bullets missed — same file, same class, same one-line + * change; leaving them bare would have kept the defect alive for two spellings + * of the same authoring mistake. + */ +const REFUSALS: Array<{ name: string; where: unknown; message: RegExp; issueBullet: boolean }> = [ + { + name: 'operator outside the vocabulary (#3948)', + where: { stage: { $sortOf: 'won' } }, + message: /Unsupported filter operator "\$sortOf" on "stage"/, + issueBullet: true, + }, + { + name: 'field constraint with zero operators (#5240)', + where: { stage: {} }, + message: /carries a field constraint with zero operators/, + issueBullet: true, + }, + { + name: '$between without exactly two bounds', + where: { amount: { $between: [10] } }, + message: /needs a two-element \[min, max\] array/, + issueBullet: true, + }, + { + name: '$and with an empty array', + where: { $and: [] }, + message: /"\$and" requires a non-empty array/, + issueBullet: true, + }, + { + name: '$or with an empty array', + where: { $or: [] }, + message: /"\$or" requires a non-empty array/, + issueBullet: true, + }, + { + name: '$or branch that is not a filter object', + where: { $or: [{ stage: 'won' }, 'nope'] }, + message: /branches must be filter objects/, + issueBullet: true, + }, + { + name: '$not of a non-object', + where: { $not: 5 }, + message: /"\$not" requires a filter object/, + issueBullet: false, + }, + { + name: 'unsupported TOP-LEVEL operator', + where: { $nor: [{ stage: 'won' }] }, + message: /Unsupported top-level filter operator "\$nor"/, + issueBullet: false, + }, + { + name: 'a `where` array that cannot be lowered (#5334)', + where: [{ stage: 'won' }], + message: /received a 'where' array that is not a filter/, + issueBullet: true, + }, +]; + +/** + * Inputs that must keep being ACCEPTED, with what they compile to. + * + * The other half of "only the shape changed": a refusal-shape edit that + * accidentally widened the refusal would show up here as a throw, and one that + * changed the compiled predicate would show up as a tree mismatch. Both are + * green before and after #5352. + */ +const ACCEPTED: Array<{ name: string; where: unknown; tree: unknown }> = [ + { + name: 'implicit equality', + where: { stage: 'won' }, + tree: { kind: 'leaf', member: 'stage', operator: 'equals', values: ['won'] }, + }, + { + name: 'an explicit operator', + where: { amount: { $gte: 10 } }, + tree: { kind: 'leaf', member: 'amount', operator: 'gte', values: ['10'] }, + }, + { + name: '$between lowered to its two bounds', + where: { amount: { $between: [10, 20] } }, + tree: { + kind: 'and', + children: [ + { kind: 'leaf', member: 'amount', operator: 'gte', values: ['10'] }, + { kind: 'leaf', member: 'amount', operator: 'lte', values: ['20'] }, + ], + }, + }, + { + name: 'a disjunction', + where: { $or: [{ stage: 'won' }, { stage: 'lost' }] }, + tree: { + kind: 'or', + children: [ + { kind: 'leaf', member: 'stage', operator: 'equals', values: ['won'] }, + { kind: 'leaf', member: 'stage', operator: 'equals', values: ['lost'] }, + ], + }, + }, + { + // #5325: an empty `$in` is the FALSE constant, not an absent predicate. + name: 'an empty $in as the FALSE constant (#5325)', + where: { stage: { $in: [] } }, + tree: { kind: 'const', value: false }, + }, + { + // #5325: `{}` constrains nothing, which is the constant TRUE (`null`). + name: 'an empty filter object as TRUE (#5325)', + where: {}, + tree: null, + }, + { + // #5334: `[]` is "no filter", not a failed filter. + name: 'an empty `where` array as "no filter" (#5334)', + where: [], + tree: null, + }, + { + // #5334: the lowerable array spelling compiles to the object spelling's tree. + name: 'a lowerable FilterArray (#5334)', + where: [['stage', '=', 'won']], + tree: { kind: 'leaf', member: 'stage', operator: 'equals', values: ['won'] }, + }, +]; + +// ───────────────────────────────────────────────────────────────────────────── + +describe('[#5352] the refusal SET is unchanged — only the error shape moved', () => { + for (const c of REFUSALS) { + it(`still REFUSES: ${c.name}`, () => { + const err = refusalFor(c.where); + expect(err, `${c.name} was accepted — the refusal set moved`).toBeInstanceOf(Error); + // The message is pinned too: the REST fallback list still classifies by + // message text for the non-filter families, so a wording drift here is + // the kind of thing that must be a deliberate edit. + expect(String(err?.message)).toMatch(c.message); + }); + } + + for (const c of ACCEPTED) { + it(`still ACCEPTS (and compiles identically): ${c.name}`, () => { + expect(refusalFor(c.where), `${c.name} was refused — the refusal set moved`).toBeUndefined(); + expect(normalizeAnalyticsFilterTree({ where: c.where })).toEqual(c.tree); + }); + } + + it('carries no `where` at all → no constraint, no error', () => { + expect(normalizeAnalyticsFilterTree({})).toBeNull(); + }); +}); + +describe('[#5352] every refusal carries the ADR-0112 envelope (INVALID_FILTER / 400)', () => { + for (const c of REFUSALS) { + it(`${c.name} → code INVALID_FILTER, status 400`, () => { + const err = refusalFor(c.where); + expect(err).toBeInstanceOf(Error); + // Read as the REST boundary reads them — `error.code` / `error.status`, + // the two fields `rest-server.ts` now classifies on. + expect(err?.code, 'a refusal with no `code` lands as 500 ANALYTICS_QUERY_FAILED').toBe('INVALID_FILTER'); + expect(err?.status, 'a refusal with no `status` lands as 500 ANALYTICS_QUERY_FAILED').toBe(400); + }); + } + + it('covers every refusing site in the module, including the two the issue did not list', () => { + // A structural guard rather than a count for its own sake: #5352's body + // enumerated four bullets, and enveloping only those would have left + // `{$not: 5}` and `{$nor: […]}` answering 500 while their neighbours + // answered 400 — the same one-condition-two-shapes split the issue is about. + expect(REFUSALS.filter((c) => !c.issueBullet).map((c) => c.name)).toEqual([ + '$not of a non-object', + 'unsupported TOP-LEVEL operator', + ]); + expect(REFUSALS).toHaveLength(9); + }); +}); diff --git a/packages/services/service-analytics/src/strategies/filter-normalizer.ts b/packages/services/service-analytics/src/strategies/filter-normalizer.ts index 0a10e473c4..86898a54fc 100644 --- a/packages/services/service-analytics/src/strategies/filter-normalizer.ts +++ b/packages/services/service-analytics/src/strategies/filter-normalizer.ts @@ -105,6 +105,25 @@ * array spelling. {@link normalizeAnalyticsFilterTree} now gives the same three * answers the engine door gives, so one query means one thing on every path. * + * # Every refusal here is a 400, and SAYS so (#5352) + * + * All of the above only helps the author if the refusal REACHES them. Each + * refusal in this module is a caller-shaped mistake — a misspelled operator, a + * `$between` with one bound, a `{}` where an operator belongs — and ADR-0112's + * rule is that such an error carries its own machine-readable semantics + * (`code` + `status`) rather than leaving each consumer to guess from the + * message text. Until #5352 only the #5334 array refusals did; the other seven + * were bare `throw new Error(…)`, so `/analytics/dataset/query` had nothing to + * read and answered `500 ANALYTICS_QUERY_FAILED` — "the platform is broken" for + * what is a typo in a widget's filter, counted as a 5xx by ops alerting. The + * same mistake on `find()` has answered `400 INVALID_FILTER` since #3948. + * + * So {@link invalidFilterError} is now the ONLY way this module refuses, and + * `rest-server.ts`'s analytics catch reads that envelope before anything else. + * #5352 changed the SHAPE of these errors and nothing about WHICH inputs are + * refused — the refusal set is pinned input-by-input in + * `filter-refusal-envelope.test.ts` precisely so that stays true. + * * Row-result cover: `filter-operator-coverage.test.ts` for the operator * vocabulary, `native-sql-filter-logic-conformance.test.ts`, which runs the * SHARED combinator table (`FILTER_LOGIC_CASES`, #3774) that the SQL compiler, @@ -123,6 +142,35 @@ export interface NormalizedAnalyticsFilter { values: string[]; } +// ── [#5334 / #5352] The refusal envelope ───────────────────────────────────── + +/** + * [#5334, generalised by #5352] A filter refusal in the ADR-0112 envelope every + * sibling filter refusal in the repo speaks — `INVALID_FILTER` / 400. + * + * The twin of `driver-sql`'s and `driver-memory`'s `unsupportedFilterError`. + * A caller that writes a filter this module cannot compile has made a + * 400-class mistake, and a coded refusal is what lets the `/analytics` face + * answer it as one instead of as an opaque 500. + * + * ⛔ **The only way this module refuses.** #5334 introduced it for the two + * array-door refusals while the other seven sites stayed bare `Error`s, and a + * half-enveloped module is indistinguishable from an unenveloped one at the + * REST boundary: `error.code` was `undefined` for the operator typo that is by + * far the commonest of the nine, so the whole family landed as + * `500 ANALYTICS_QUERY_FAILED` (#5352). A new refusal added to this file must + * be thrown through here; a bare `throw new Error` is the defect returning. + * + * It carries no `#5352`-specific wording on purpose — the envelope is the + * contract, the message stays whatever the refusing site says. + */ +function invalidFilterError(message: string): Error { + const err = new Error(message) as Error & { code?: string; status?: number }; + err.code = StandardErrorCode.enum.INVALID_FILTER; + err.status = 400; + return err; +} + /** * The value-INDEPENDENT operators: the pipeline name depends only on the key. * @@ -264,7 +312,7 @@ function fieldLeaves(key: string, raw: unknown): NormalizedFilterNode[] { // accident — a filter builder that recorded a field and never its operator), // and a loud refusal is the answer the rest of the repo already gives. if (Object.keys(wrapper).length === 0) { - throw new Error( + throw invalidFilterError( `[analytics] "${key}" carries a field constraint with zero operators ({}). ` + `Refusing rather than reading it as "every row" or "no row" — #5240 ruled this ` + `shape refused on every backend.`, @@ -295,7 +343,7 @@ function fieldLeaves(key: string, raw: unknown): NormalizedFilterNode[] { // branch exists to prevent, and it is indistinguishable from a // legitimately wide query. Same stance driver-memory took for the // same shape (#3948). - throw new Error( + throw invalidFilterError( `[analytics] "$between" on "${key}" needs a two-element [min, max] array, got ` + `${JSON.stringify(v)}. Dropping the predicate would silently widen the query to every row.`, ); @@ -340,7 +388,7 @@ function fieldLeaves(key: string, raw: unknown): NormalizedFilterNode[] { // the emitted SQL. That failure mode is #3650's, and skipping // unmapped operators is how `$between` reproduced it (#4128). // driver-memory made the same call for the same reason in #3948. - throw new Error( + throw invalidFilterError( `[analytics] Unsupported filter operator "${opKey}" on "${key}". ` + `Supported: ${Object.keys(MONGO_TO_CUBE_OP).join(', ')}, $between, $null, $exists, ` + `and the $and/$or/$not combinators. ` + @@ -387,7 +435,7 @@ function buildNode(cond: Record): NormalizedFilterNode | null { if (key === '$and' || key === '$or') { if (!Array.isArray(raw) || raw.length === 0) { - throw new Error( + throw invalidFilterError( `[analytics] "${key}" requires a non-empty array. An empty combinator has no ` + `defensible reading — dropping it widens the query, and treating it as "match ` + `nothing" silently empties a chart.`, @@ -400,7 +448,7 @@ function buildNode(cond: Record): NormalizedFilterNode | null { // query to every row. Neither is a defensible reading of garbage input — // `read-scope-sql.ts` refuses the same shape. if (!isFilterObject(sub)) { - throw new Error( + throw invalidFilterError( `[analytics] "${key}" branches must be filter objects, got ${JSON.stringify(sub)}. ` + `Skipping it would silently change which rows the filter admits.`, ); @@ -426,7 +474,7 @@ function buildNode(cond: Record): NormalizedFilterNode | null { if (!isFilterObject(raw)) { // Same call as the branch elements above: a `$not` of garbage used to // vanish, which turns "exclude these rows" into "exclude nothing". - throw new Error( + throw invalidFilterError( `[analytics] "$not" requires a filter object, got ${JSON.stringify(raw)}. ` + `Dropping it would silently widen the query to rows the filter excludes.`, ); @@ -444,7 +492,7 @@ function buildNode(cond: Record): NormalizedFilterNode | null { } if (key.startsWith('$')) { - throw new Error( + throw invalidFilterError( `[analytics] Unsupported top-level filter operator "${key}". ` + `Dropping it would silently widen the query to rows the filter excludes.`, ); @@ -662,22 +710,6 @@ function nullSafeNegationOperand(node: Record): Record