diff --git a/.changeset/analytics-face-refuses-uncompilable-filters.md b/.changeset/analytics-face-refuses-uncompilable-filters.md new file mode 100644 index 0000000000..5be104c1e4 --- /dev/null +++ b/.changeset/analytics-face-refuses-uncompilable-filters.md @@ -0,0 +1,87 @@ +--- +"@objectstack/driver-memory": minor +--- + +fix(driver-memory): the analytics (cube) face REFUSES a filter it cannot compile instead of silently dropping it (#5345) + +**This is an observable behaviour change on a shipped surface, and it will turn +some working-looking dashboards red.** That is the point: the widgets it breaks +were returning inflated aggregates, and some of them were returning rows the +caller had no permission to read. + +## What was happening + +`MemoryAnalyticsService` lowers `AnalyticsQuery.where` into a flat, cube-style +`{member, operator, values}` list. Anything that did not fit was answered with +`continue` — in two places, and with a comment presenting it as a feature +("ignore so a partial query still runs rather than failing entirely"): + +| dropped | why it did not fit | +|---|---| +| `$or` (whole branch) | no expression in a flat AND-list | +| `$not` (whole branch) | same | +| `$between` | no row in the mongo→cube operator table | +| `$startsWith` / `$endsWith` | same | +| `$null` | same | +| `$regex` | same — and `plugin-auth`'s ObjectQL adapter emits it | + +Dropping a predicate does not narrow a query, it **widens** it: fewer +constraints means more rows. A widget filtered to two stages with +`{$or: [{stage: 'won'}, {stage: 'lost'}]}` aggregated the **entire table** and +rendered as a perfectly normal chart. Measured on the shared +`FILTER_LOGIC_CASES` fixture, **15 of its 17 cases** returned a wider row set +than the standard specifies — usually every row. Of the two that did agree, one +(`a $or nested under a top-level $and`) agreed by *coincidence*: its dropped +`$or` happened to be redundant against a surviving sibling key, which is the +best illustration available of why "the number looked right" was never evidence. + +`$not` makes it more than a wrong number. `cel-to-filter.ts` compiles a CEL +`!expr` RLS read scope into `{$not: {…}}`, so the dropped branch was the read +scope itself — the aggregate included records the caller is not allowed to see. + +## What changes for you + +A `where` carrying any of the shapes above now raises **`INVALID_FILTER` / 400** +(the ADR-0112 envelope every sibling filter refusal in this driver already +speaks, reaching REST callers as a 400 since #5366) naming the offending +operator or combinator and its position, e.g.: + +> Filter operator `"$between"` on field `"amount"` at `where.amount` is declared +> by the Filter Protocol but cannot be compiled by driver-memory's analytics +> (cube) face. Supported operators on this surface: `$eq, $ne, $gt, $gte, $lt, +> $lte, $in, $nin, $contains, $notContains, $exists`. + +Both entry points refuse identically — `query()` and `generateSql()`. + +**The fix, per shape:** + +- `$between` on a range → the two bounds, which this face has always compiled: + `{ closed_at: { $gte: '2026-01-01', $lte: '2026-01-31' } }`, or a + `timeDimensions[].dateRange`, which is unaffected. +- `$startsWith` / `$endsWith` / `$regex` → `$contains`, or move the query to + `find()`. +- `$null` → `{ field: { $exists: false } }` for the absent case. +- `$or` / `$not` → restate as the implicit AND of field keys where the intent + allows it; where it does not, the cube pipeline genuinely cannot express it, + and the query belongs on `find()`. + +Nothing that was **compiled** changes. All eleven supported operators, `$and`, +implicit equality, nested-relation flattening, time dimensions and the empty +filter produce byte-identical pipelines. + +## Why refuse rather than teach the cube pipeline `$or` + +This is the call ADR-0078 / #4286 made for `objectql`'s `having` — an ignored +operator there "silently returns UNFILTERED aggregates", so it throws — and the +posture #3948 established for every filter backend: a filter that cannot be +compiled is refused loudly, never skipped. It is also where the two neighbouring +faces landed (#5366, #5368). + +Mechanically, the refusal is not a new check bolted onto this face. It reuses +the package's single filter gate, `assertFilterConditionShape`, which now takes +the calling face's declared capabilities; and the analytics face derives those +capabilities from its own mongo→cube operator table, so widening what it accepts +and teaching it to compile the operator are now the same edit. The shared +`FILTER_LOGIC_CASES` conformance table covers this third face for the first time +(it watched only two of the driver's three), holding it to: agree with +`find()`, or refuse — never a third, quieter answer. diff --git a/packages/plugins/driver-memory/src/filter-refusal.ts b/packages/plugins/driver-memory/src/filter-refusal.ts index bc52ad0c17..59d6214c6d 100644 --- a/packages/plugins/driver-memory/src/filter-refusal.ts +++ b/packages/plugins/driver-memory/src/filter-refusal.ts @@ -4,23 +4,33 @@ * The filter refusals this driver raises, in ONE place — and, since #5324/#5328, * the ONE walk that decides which shapes are refused at all. * - * Both of this package's filter surfaces refuse the same shapes with the same - * wire envelope: the live query path (`memory-driver.ts` → mingo) and the + * All THREE of this package's filter surfaces refuse the same shapes with the + * same wire envelope: the live query path (`memory-driver.ts` → mingo), the * reference matcher (`memory-matcher.ts`, the record-at-a-time evaluator the - * conformance suites hold against `driver-sql` and `@objectstack/formula`). They - * were two independent code paths with two independent notions of what a filter - * may be, which is exactly how #5240's divergence survived unnoticed in-package. - * - * #5240 gave the two faces one refusal by writing the same check twice. That was - * still two implementations of one rule, and the shapes #5324/#5328 measured - * proved how far apart two such implementations drift: given a malformed - * `$between` the live path answered "no rows" while the matcher answered "EVERY - * row" — opposite answers, inside one package, to one filter. So the rule now - * lives in exactly one function, {@link assertFilterConditionShape}, and both - * faces call it before they evaluate anything. + * conformance suites hold against `driver-sql` and `@objectstack/formula`), and + * since #5345 the analytics/cube face (`memory-analytics.ts`). They were + * independent code paths with independent notions of what a filter may be, which + * is exactly how #5240's divergence survived unnoticed in-package. + * + * #5240 gave the first two faces one refusal by writing the same check twice. + * That was still two implementations of one rule, and the shapes #5324/#5328 + * measured proved how far apart two such implementations drift: given a + * malformed `$between` the live path answered "no rows" while the matcher + * answered "EVERY row" — opposite answers, inside one package, to one filter. So + * the rule now lives in exactly one function, {@link assertFilterConditionShape}, + * and every face calls it before it evaluates anything. + * + * [#5345] The faces are not equally capable, and pretending they were is what + * kept the third one out. `memory-analytics` lowers a `where` into a cube-style + * `{member, operator, values}` list, and that pipeline expresses neither `$or` + * nor `$not` nor five of the declared field operators. Its answer used to be a + * `continue`. So the walk now takes the calling face's {@link + * FilterFaceCapabilities} — what that face can COMPILE — and refuses what it + * cannot, in the same envelope, from the same place. A face declares its + * vocabulary; it does not get to drop what falls outside it. */ -import { FILTER_OPERATORS } from '@objectstack/spec/data'; +import { FILTER_OPERATORS, LOGICAL_OPERATORS } from '@objectstack/spec/data'; import { StandardErrorCode } from '@objectstack/spec/api'; /** @@ -165,6 +175,108 @@ export const SUPPORTED_FIELD_OPERATORS: ReadonlySet = new Set([ /** The vocabulary as it appears in a refusal message, in declaration order. */ const SUPPORTED_FIELD_OPERATOR_LIST = [...SUPPORTED_FIELD_OPERATORS].join(', '); +/** + * [#5345] What ONE evaluation face can COMPILE — the narrower vocabulary a + * particular surface enforces on top of the package-wide one above. + * + * The distinction this type draws is the whole of #5345. Two different things + * can be wrong with `{ amount: { $sounds_like: 3 } }` and + * `{ amount: { $between: [1, 3] } }` on the analytics face: + * + * - the first names an operator the **Filter Protocol** does not declare — it is + * wrong everywhere, and {@link unknownFieldOperatorError} says so; + * - the second is a declared operator this **face** cannot lower into its cube + * pipeline. It is a perfectly good filter that `find()` runs today. + * + * Before #5345 the second class was answered with `continue`, silently, on the + * analytics face only. A face that declares its vocabulary here gets the second + * class refused for it, by the same walk, in the same envelope — and, crucially, + * cannot answer it any other way, because the walk runs before the face's + * lowering code is reached. + * + * Derive the sets from the face's own lowering table rather than hand-listing + * them (see `MONGO_TO_CUBE_OPERATOR` in `memory-analytics.ts`): a hand-written + * copy agrees with the compiler on the day it is typed and never again, which is + * the note already sitting over {@link SUPPORTED_FIELD_OPERATORS}. + */ +export interface FilterFaceCapabilities { + /** How the face names itself in a refusal, e.g. `"the analytics (cube) face"`. */ + readonly face: string; + /** The field operators this face lowers. A subset of {@link SUPPORTED_FIELD_OPERATORS}. */ + readonly fieldOperators: ReadonlySet; + /** The logical combinators this face lowers. A subset of `LOGICAL_OPERATORS`. */ + readonly combinators: ReadonlySet; +} + +/** + * [#5345] The default: the whole vocabulary this driver's query path and + * reference matcher evaluate. Passing no capabilities means "this face compiles + * everything the driver does", which is true of both of them and keeps every + * pre-#5345 call site behaving byte-for-byte as before. + */ +export const DRIVER_FILTER_CAPABILITIES: FilterFaceCapabilities = Object.freeze({ + face: 'this driver', + fieldOperators: SUPPORTED_FIELD_OPERATORS, + combinators: new Set(LOGICAL_OPERATORS), +}); + +/** + * [#5345] A DECLARED field operator that this face cannot lower. + * + * Distinct from {@link unknownFieldOperatorError} on purpose: that one means + * "the Filter Protocol has no such operator", this one means "the protocol has + * it, `find()` runs it, and this surface cannot". Collapsing them would tell a + * dashboard author their `$between` is a typo. + * + * The tail is the #3948 rule stated in the direction that matters here. A + * dropped predicate does not narrow a query, it WIDENS it: the aggregate is + * computed over rows the author excluded, and a chart drawn over them looks + * exactly like a working chart. This is ADR-0078 / #4286's call on `objectql`'s + * `having`, which was refused rather than skipped for the identical reason. + */ +export function uncompilableFieldOperatorError( + op: string, + field: string, + path: string, + capabilities: FilterFaceCapabilities, +): Error { + const supported = [...capabilities.fieldOperators].join(', ') || '(none)'; + return unsupportedFilterError( + `Filter operator "${op}" on field "${field}" at ${path} is declared by the Filter Protocol ` + + `but cannot be compiled by ${capabilities.face}. Supported operators on this surface: ` + + `${supported}. It is refused rather than dropped: a predicate that compiles to nothing does ` + + `not narrow the query, it WIDENS it — the aggregate is then computed over rows the filter ` + + `excluded, and a chart drawn over them looks like a working chart (#3948, #4286/ADR-0078, ` + + `#5345). Rewrite the predicate with a supported operator, or run it through find().`, + ); +} + +/** + * [#5345] A DECLARED logical combinator that this face cannot lower. + * + * Named separately from {@link unknownLogicalOperatorError} for the same reason + * as the field-operator pair above, and it is the sharper half of #5345: a + * dropped `$or` discards a whole branch of the filter, and `$not` is precisely + * what `cel-to-filter.ts` compiles a CEL `!expr` RLS read scope into. Dropping + * that one does not make a number inaccurate — it puts rows the caller has no + * permission to read into the aggregate. + */ +export function uncompilableCombinatorError( + key: string, + path: string, + capabilities: FilterFaceCapabilities, +): Error { + const supported = [...capabilities.combinators].join(', ') || '(none)'; + return unsupportedFilterError( + `Filter combinator "${key}" at ${path} is declared by the Filter Protocol but cannot be ` + + `compiled by ${capabilities.face}. Supported combinators on this surface: ${supported}. ` + + `It is refused rather than ignored: dropping a combinator discards a whole branch of the ` + + `filter and WIDENS the result set, and "$not" is what compileCelToFilter emits for a CEL ` + + `"!expr" RLS read scope — a dropped one is an over-permissive read, not an inaccurate ` + + `number (#3948, #5345).`, + ); +} + /** A short type name for an operand a filter refusal has to describe. */ function describeFilterOperand(value: unknown): string { if (value === null) return 'null'; @@ -383,27 +495,46 @@ export function filterNodeExpectedError(value: unknown, path: string): Error { * - the MEMBER types of a `$between` array — `driver-sql` checks its arity and * nothing else (#5041 measured the member case and deliberately left it); * - a stringified comparand for the `LIKE` family — same, and fail-closed. + * + * ## What `capabilities` adds (#5345) + * + * Shape is universal; CAPABILITY is per-face. `capabilities` narrows what this + * particular caller can lower — see {@link FilterFaceCapabilities} — and the + * walk refuses the difference. It defaults to + * {@link DRIVER_FILTER_CAPABILITIES}, i.e. everything, so the query path and the + * matcher are unaffected. + * + * The capability check is made BEFORE the shape checks at the same key, and + * deliberately: on a face that cannot compile `$or` at all, reporting that its + * operand should have been an array would send the author to fix the wrong + * thing, then refuse the corrected filter anyway. */ -export function assertFilterConditionShape(node: unknown, path: string): void { +export function assertFilterConditionShape( + node: unknown, + path: string, + capabilities: FilterFaceCapabilities = DRIVER_FILTER_CAPABILITIES, +): void { if (!isFilterNode(node)) return; for (const [key, value] of Object.entries(node)) { const here = `${path}.${key}`; if (key === '$and' || key === '$or') { + if (!capabilities.combinators.has(key)) throw uncompilableCombinatorError(key, here, capabilities); if (!Array.isArray(value)) throw filterNodeListExpectedError(key, value, here); value.forEach((child, index) => { const childPath = `${here}[${index}]`; if (!isFilterNode(child)) throw filterNodeExpectedError(child, childPath); - assertFilterConditionShape(child, childPath); + assertFilterConditionShape(child, childPath, capabilities); }); continue; } if (key === '$not') { + if (!capabilities.combinators.has(key)) throw uncompilableCombinatorError(key, here, capabilities); if (!isFilterNode(value)) throw filterNodeExpectedError(value, here); - assertFilterConditionShape(value, here); + assertFilterConditionShape(value, here, capabilities); continue; } if (key.startsWith('$')) throw unknownLogicalOperatorError(key, here); - assertFieldConstraintShape(key, value, here); + assertFieldConstraintShape(key, value, here, capabilities); } } @@ -418,7 +549,12 @@ export function assertFilterConditionShape(node: unknown, path: string): void { * is: the two faces silently disagreed about it (the matcher ignored the * non-`$` key, mingo did not). */ -function assertFieldConstraintShape(field: string, spec: unknown, path: string): void { +function assertFieldConstraintShape( + field: string, + spec: unknown, + path: string, + capabilities: FilterFaceCapabilities, +): void { if (!isFilterNode(spec)) return; // [#5240] The zero-operator constraint keeps its own predicate rather than an // inlined `keys.length === 0`, so the reasoning for what does and does not @@ -429,6 +565,13 @@ function assertFieldConstraintShape(field: string, spec: unknown, path: string): if (!keys.some((key) => key.startsWith('$'))) return; for (const op of keys) { if (!SUPPORTED_FIELD_OPERATORS.has(op)) throw unknownFieldOperatorError(op, field, path); + // [#5345] Declared, but not by THIS face. Checked before the comparand-shape + // rules below so a `$between` a face cannot compile is reported as + // unsupported-here rather than as a malformed range the face would refuse + // even once corrected. + if (!capabilities.fieldOperators.has(op)) { + throw uncompilableFieldOperatorError(op, field, path, capabilities); + } if (op === '$between' && !isBetweenComparand(spec[op])) { throw malformedBetweenError(field, spec[op], `${path}.$between`); } diff --git a/packages/plugins/driver-memory/src/memory-analytics-filter-refusal.test.ts b/packages/plugins/driver-memory/src/memory-analytics-filter-refusal.test.ts new file mode 100644 index 0000000000..52c1f80e25 --- /dev/null +++ b/packages/plugins/driver-memory/src/memory-analytics-filter-refusal.test.ts @@ -0,0 +1,216 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#5345] The analytics (cube) face refuses the filters it cannot compile. + * + * `memory-analytics.ts` lowers `AnalyticsQuery.where` into a flat cube-style + * `{member, operator, values}` list. Two `continue`s in that lowering used to + * discard whatever did not fit: + * + * 1. `if (key === '$or' || key === '$not') continue;` — a whole branch of the + * filter, gone; + * 2. `if (!cubeOp) continue;` — the five declared operators with no row in the + * mongo→cube table (`$between`, `$startsWith`, `$endsWith`, `$null`, + * `$regex`), gone one predicate at a time. + * + * The direction is what makes it a defect and not a limitation: a dropped + * predicate is FEWER constraints, therefore MORE rows. A widget filtered to two + * stages aggregated the whole table and rendered as a working widget — the + * amplifying failure #3948 outlawed, and the same call ADR-0078 / #4286 made for + * `objectql`'s `having`. `$not` is worse still: `cel-to-filter.ts` compiles a CEL + * `!expr` RLS read scope into `{$not: {...}}`, so dropping it is an + * over-permissive read, not a wrong number. + * + * Both public entry points are covered here — `query()` and `generateSql()` — + * because both call `normalizeFilters` and a refusal on one only would leave the + * other silently answering the old way. + */ + +import { describe, it, expect, beforeEach } from 'vitest'; +import { InMemoryDriver } from './memory-driver.js'; +import { MemoryAnalyticsService } from './memory-analytics.js'; +import { AnalyticsQuerySchema } from '@objectstack/spec/data'; +import type { AnalyticsQuery, AnalyticsQueryInput, Cube, FilterCondition } from '@objectstack/spec/data'; + +const asQuery = (input: AnalyticsQueryInput): AnalyticsQuery => AnalyticsQuerySchema.parse(input); + +/** Five rows over two stages, so a dropped predicate shows up as a bigger count. */ +const DEALS = [ + { id: 1, stage: 'won', amount: 100, owner: 'u1', name: 'alpha', closed_at: null }, + { id: 2, stage: 'won', amount: 200, owner: 'u1', name: 'beta', closed_at: '2026-01-02' }, + { id: 3, stage: 'lost', amount: 300, owner: 'u2', name: 'gamma', closed_at: '2026-02-03' }, + { id: 4, stage: 'open', amount: 400, owner: 'u2', name: 'delta', closed_at: null }, + { id: 5, stage: 'open', amount: 500, owner: 'u3', name: 'epsilon', closed_at: '2026-03-04' }, +]; + +const CUBE: Cube = { + name: 'deals', + title: 'Deals', + sql: 'deals', + measures: { + count: { name: 'count', label: 'Deal Count', type: 'count', sql: 'id' }, + totalAmount: { name: 'total_amount', label: 'Total', type: 'sum', sql: 'amount' }, + }, + dimensions: { + stage: { name: 'stage', label: 'Stage', type: 'string', sql: 'stage' }, + owner: { name: 'owner', label: 'Owner', type: 'string', sql: 'owner' }, + name: { name: 'name', label: 'Name', type: 'string', sql: 'name' }, + amount: { name: 'amount', label: 'Amount', type: 'number', sql: 'amount' }, + closedAt: { name: 'closed_at', label: 'Closed At', type: 'time', sql: 'closed_at' }, + }, + public: true, +}; + +/** + * The refusal envelope, asserted as a whole. ADR-0112: `INVALID_FILTER` / 400, + * the same envelope every sibling refusal in this package speaks — a coded + * refusal on one face and a bare `{error}` on another is exactly the divergence + * #5240 closed. + */ +const expectRefusal = async (run: () => Promise, ...mustMention: string[]): Promise => { + let caught: unknown; + try { + await run(); + } catch (error) { + caught = error; + } + expect(caught, 'the filter was accepted instead of refused').toBeInstanceOf(Error); + const err = caught as Error & { code?: string; status?: number }; + expect(err.code).toBe('INVALID_FILTER'); + expect(err.status).toBe(400); + // A dashboard author has to be able to tell WHICH predicate was rejected. + for (const needle of mustMention) expect(err.message).toContain(needle); + return err; +}; + +describe('[#5345] MemoryAnalyticsService — filters it cannot compile are refused, not dropped', () => { + let driver: InMemoryDriver; + let service: MemoryAnalyticsService; + + beforeEach(async () => { + driver = new InMemoryDriver({ initialData: { deals: [...DEALS] } }); + await driver.connect(); + service = new MemoryAnalyticsService({ driver, cubes: [CUBE] }); + }); + + const count = async (where?: FilterCondition): Promise => { + const result = await service.query(asQuery({ cube: 'deals', measures: ['deals.count'], where })); + return Number((result.rows[0] as Record | undefined)?.['deals.count'] ?? 0); + }; + + it('the fixture is five rows, so an inflated aggregate is visible', async () => { + expect(await count()).toBe(5); + }); + + // ── The combinators (`continue` #1) ──────────────────────────────────────── + + it('refuses $or instead of aggregating the whole table', async () => { + const where: FilterCondition = { $or: [{ stage: 'won' }, { stage: 'lost' }] }; + const err = await expectRefusal(() => count(where), '$or', 'where.$or'); + // The regression in one line: three rows were asked for, five were returned. + expect(err.message).toContain('WIDENS'); + }); + + it('refuses a $or nested inside a $and it CAN compile', async () => { + const where: FilterCondition = { $and: [{ owner: 'u1' }, { $or: [{ stage: 'won' }, { stage: 'open' }] }] }; + await expectRefusal(() => count(where), '$or', 'where.$and[1].$or'); + }); + + it('refuses a $or that sits beside a compilable sibling key', async () => { + // The sharpest shape: the sibling `owner` lowered fine, so the query ran and + // returned a plausible-looking number computed without the $or at all. + const where: FilterCondition = { owner: 'u1', $or: [{ stage: 'won' }, { stage: 'lost' }] }; + await expectRefusal(() => count(where), '$or'); + }); + + it('refuses $not — the shape an RLS read scope compiles to', async () => { + const where: FilterCondition = { $not: { stage: 'lost' } }; + const err = await expectRefusal(() => count(where), '$not', 'where.$not'); + expect(err.message).toContain('over-permissive read'); + }); + + it('names the combinators it CAN compile, so the refusal is actionable', async () => { + const err = await expectRefusal(() => count({ $or: [{ stage: 'won' }] })); + expect(err.message).toContain('Supported combinators on this surface: $and'); + }); + + // ── The unmapped operators (`continue` #2) ───────────────────────────────── + + const UNCOMPILABLE: Array<{ op: string; where: FilterCondition }> = [ + { op: '$between', where: { amount: { $between: [100, 200] } } }, + { op: '$startsWith', where: { name: { $startsWith: 'al' } } }, + { op: '$endsWith', where: { name: { $endsWith: 'ta' } } }, + { op: '$null', where: { closed_at: { $null: true } } }, + { op: '$regex', where: { name: { $regex: '^al' } } }, + ]; + + for (const { op, where } of UNCOMPILABLE) { + it(`refuses ${op} — declared by the Filter Protocol, not compilable by this face`, async () => { + const err = await expectRefusal(() => count(where), op); + // Not "you made a typo": the operator is real, this surface cannot run it. + expect(err.message).toContain('declared by the Filter Protocol'); + expect(err.message).toContain('Supported operators on this surface'); + }); + } + + it('refuses an unmapped operator reached through the nested-relation branch', async () => { + // `{profile: {verified: …}}` is re-entered as a synthesised `{'profile.verified': …}` + // node the up-front gate never walked — the one path where the lowering's own + // refusal is load-bearing rather than defence in depth. + await expectRefusal(() => count({ profile: { verified: { $between: [1, 2] } } } as FilterCondition), '$between'); + }); + + it('still refuses an operator the protocol does not declare at all, with the OTHER message', async () => { + const err = await expectRefusal( + () => count({ name: { $sounds_like: 'alpha' } } as unknown as FilterCondition), + '$sounds_like', + ); + expect(err.message).toContain('Unsupported filter operator'); + expect(err.message).not.toContain('declared by the Filter Protocol'); + }); + + // ── What must NOT have changed ───────────────────────────────────────────── + + it('every compilable operator still aggregates exactly as before', async () => { + expect(await count({ stage: 'won' })).toBe(2); + expect(await count({ stage: { $eq: 'won' } })).toBe(2); + expect(await count({ stage: { $ne: 'won' } })).toBe(3); + expect(await count({ stage: { $in: ['won', 'lost'] } })).toBe(3); + expect(await count({ stage: { $nin: ['won'] } })).toBe(3); + expect(await count({ amount: { $gt: 200 } })).toBe(3); + expect(await count({ amount: { $gte: 200 } })).toBe(4); + expect(await count({ amount: { $lt: 300 } })).toBe(2); + expect(await count({ name: { $contains: 'et' } })).toBe(1); + expect(await count({ name: { $contains: 'a' } })).toBe(4); + expect(await count({ stage: 'won', owner: 'u1' })).toBe(2); + expect(await count({ $and: [{ stage: 'open' }, { owner: 'u2' }] })).toBe(1); + expect(await count({})).toBe(5); + }); + + // ── The second public entry point ────────────────────────────────────────── + + it('generateSql() refuses the same filters — a WHERE that lost a branch is the same bug', async () => { + await expectRefusal( + () => service.generateSql(asQuery({ + cube: 'deals', + measures: ['deals.count'], + where: { $or: [{ stage: 'won' }, { stage: 'lost' }] }, + })), + '$or', + ); + await expectRefusal( + () => service.generateSql(asQuery({ + cube: 'deals', + measures: ['deals.count'], + where: { amount: { $between: [1, 2] } }, + })), + '$between', + ); + const ok = await service.generateSql(asQuery({ + cube: 'deals', + measures: ['deals.count'], + where: { stage: 'won' }, + })); + expect(ok.sql).toContain("stage = 'won'"); + }); +}); diff --git a/packages/plugins/driver-memory/src/memory-analytics.ts b/packages/plugins/driver-memory/src/memory-analytics.ts index b76ef47bad..a9ec1f704d 100644 --- a/packages/plugins/driver-memory/src/memory-analytics.ts +++ b/packages/plugins/driver-memory/src/memory-analytics.ts @@ -4,6 +4,63 @@ import type { IAnalyticsService, AnalyticsResult, CubeMeta } from '@objectstack/ import type { Cube, AnalyticsQuery } from '@objectstack/spec/data'; import type { InMemoryDriver } from './memory-driver.js'; import { Logger, createLogger, nextUtcCalendarDay } from '@objectstack/core'; +import { + assertFilterConditionShape, + uncompilableCombinatorError, + uncompilableFieldOperatorError, + type FilterFaceCapabilities, +} from './filter-refusal.js'; + +/** + * [#5345] The Filter Protocol operators this face can LOWER into a cube-style + * `{member, operator, values}` entry — and, because + * {@link ANALYTICS_FILTER_CAPABILITIES} is derived from its keys, the complete + * statement of what the face accepts. + * + * That derivation is the point. This table used to be a `switch` with + * `default: return null`, and the caller answered `null` with `continue` — so + * the vocabulary was declared nowhere and enforced nowhere, and the five + * declared operators missing from it (`$between`, `$startsWith`, `$endsWith`, + * `$null`, `$regex`) vanished out of any `where` that carried them. Keeping the + * gate's vocabulary and the compiler's table as one object makes adding a row + * here the only way to widen what this face accepts, and makes forgetting to + * add one a loud refusal rather than a wrong number. + * + * A row here means the face ATTEMPTS the operator, not that the predicate it + * builds is correct — `$notContains` lowers to a bare mingo `{$not: 'x'}` that + * constrains nothing (#5374), and the comparand round-trip through `string[]` + * loses booleans and `null` (#5373). Both are out of #5345's scope (which ruled + * on operators with NO mapping) and are filed rather than fixed here; do not + * read this list as eleven operators known to work. + */ +const MONGO_TO_CUBE_OPERATOR: Readonly> = Object.freeze({ + $eq: 'equals', + $ne: 'notEquals', + $gt: 'gt', + $gte: 'gte', + $lt: 'lt', + $lte: 'lte', + $in: 'in', + $nin: 'notIn', + $contains: 'contains', + $notContains: 'notContains', + $exists: 'set', +}); + +/** + * [#5345] What the analytics (cube) face compiles, for the shared filter walk. + * + * `$and` is the one combinator: {@link MemoryAnalyticsService.flattenFilterCondition} + * folds its branches into the same implicit-AND list the top level already is. + * `$or` and `$not` have no expression in a flat `{member, operator, values}` + * pipeline at all — which is why they were being skipped, and why refusing is + * the answer here rather than a lowering nobody can write. + */ +export const ANALYTICS_FILTER_CAPABILITIES: FilterFaceCapabilities = Object.freeze({ + face: "driver-memory's analytics (cube) face", + fieldOperators: new Set(Object.keys(MONGO_TO_CUBE_OPERATOR)), + combinators: new Set(['$and']), +}); /** * Configuration for MemoryAnalyticsService @@ -68,12 +125,11 @@ export class MemoryAnalyticsService implements IAnalyticsService { const pipeline: Record[] = []; // Stage 1: $match for filters - // Filters can arrive in two shapes (per spec/data/analytics.zod.ts): - // - Array of { member, operator, values } (cube-style, legacy) - // - FilterCondition (MongoDB-style — canonical spec shape, used by - // dashboard widget metadata directly). - // Normalize both into the cube-style array before processing so the - // existing pipeline logic stays untouched. + // `AnalyticsQuery.where` is a FilterCondition (MongoDB-style — the canonical + // spec shape, used by dashboard widget metadata directly). It is lowered + // into the cube-style `{member, operator, values}` list this pipeline + // consumes, and anything this face cannot lower is refused there rather than + // dropped (#5345). const normalizedFilters = this.normalizeFilters(query); if (normalizedFilters.length > 0) { const matchStage: Record = {}; @@ -383,21 +439,31 @@ export class MemoryAnalyticsService implements IAnalyticsService { // =================================== /** - * Normalize filters into a cube-style array regardless of input shape. + * Normalize a query's `where` into the cube-style array the pipeline consumes. * - * Accepts: - * - undefined / null → [] - * - cube-style array `[{member, operator, values}]` → returned as-is - * - MongoDB FilterCondition object (per spec/data/filter.zod.ts): - * * implicit equality: `{is_active: true}` - * * operator wrapper: `{stage: {$nin: [...]}}` - * * mixed: `{stage: 'won', amount: {$gte: 100}}` - * → flattened into one cube-style entry per (field, operator) pair + * Accepts a MongoDB-style `FilterCondition` (per spec/data/filter.zod.ts) — + * the canonical `AnalyticsQuery.where` shape, and the only one the schema + * declares: + * - implicit equality: `{is_active: true}` + * - operator wrapper: `{stage: {$nin: [...]}}` + * - mixed: `{stage: 'won', amount: {$gte: 100}}` + * - `$and`: folded into the same implicit-AND list + * → flattened into one cube-style entry per (field, operator) pair. * - * Logical combinators (`$and`, `$or`, `$not`) are not yet expanded into - * the cube pipeline; for current dashboard widget metadata the implicit - * top-level AND of fields is sufficient. `$and` clauses are flattened - * into the same AND list. + * [#5345] Everything outside {@link ANALYTICS_FILTER_CAPABILITIES} is REFUSED + * with `INVALID_FILTER` / 400, by the same walk the query path and the + * reference matcher use. It used to be dropped, and the direction of that drop + * is what made it a defect rather than a limitation: fewer predicates means + * MORE rows, so a widget filtered on `{$or: [...]}` aggregated the whole table + * and looked like a working widget. `$not` made it a permission bug on top — + * `cel-to-filter.ts` compiles a CEL `!expr` RLS read scope into exactly that + * shape, so dropping it put unreadable rows into the numbers. + * + * The gate runs HERE, before a single key is lowered, for the reason + * `assertFilterConditionShape` documents at length: a refusal raised partway + * through a lowering fires or does not fire depending on key order and on + * which sibling branch was walked first. Both public entry points (`query()` + * and `generateSql()`) go through this method, so both refuse identically. */ private normalizeFilters(query: unknown): Array<{ member: string; operator: string; values: string[] }> { if (!query || typeof query !== 'object') return []; @@ -406,7 +472,8 @@ export class MemoryAnalyticsService implements IAnalyticsService { const where = (query as { where?: unknown }).where; if (where && typeof where === 'object' && !Array.isArray(where)) { - this.flattenFilterCondition(where as Record, out); + assertFilterConditionShape(where, 'where', ANALYTICS_FILTER_CAPABILITIES); + this.flattenFilterCondition(where as Record, out, 'where'); } return out; @@ -415,22 +482,28 @@ export class MemoryAnalyticsService implements IAnalyticsService { private flattenFilterCondition( cond: Record, out: Array<{ member: string; operator: string; values: string[] }>, + path: string, ): void { for (const [key, raw] of Object.entries(cond)) { + const here = `${path}.${key}`; if (raw == null) continue; - // Logical combinators - if (key === '$and' && Array.isArray(raw)) { - for (const sub of raw) { - if (sub && typeof sub === 'object') { - this.flattenFilterCondition(sub as Record, out); - } + // Logical combinators. `$and` folds into the same implicit-AND list; the + // gate above has already proven it is an array of filter nodes. + if (key === '$and') { + for (const sub of raw as unknown[]) { + this.flattenFilterCondition(sub as Record, out, here); } continue; } - // $or / $not are not yet supported in the cube pipeline; ignore so - // a partial query still runs rather than failing entirely. - if (key === '$or' || key === '$not') continue; + // [#5345] Unreachable via normalizeFilters — the gate refuses these for + // this face before the lowering starts. Kept as a throw rather than left + // implicit so that the `continue` which caused #5345 cannot come back, and + // so a future caller that lowers a condition without gating it first fails + // loudly instead of silently widening the result set. + if (key === '$or' || key === '$not') { + throw uncompilableCombinatorError(key, here, ANALYTICS_FILTER_CAPABILITIES); + } // Operator wrapper: { field: { $op: value, ... } } if (typeof raw === 'object' && !Array.isArray(raw) && !(raw instanceof Date)) { @@ -438,8 +511,7 @@ export class MemoryAnalyticsService implements IAnalyticsService { const opEntries = Object.keys(wrapper).filter(k => k.startsWith('$')); if (opEntries.length > 0) { for (const opKey of opEntries) { - const cubeOp = this.mongoOperatorToCubeOperator(opKey); - if (!cubeOp) continue; + const cubeOp = this.mongoOperatorToCubeOperator(opKey, key, `${here}.${opKey}`); const v = wrapper[opKey]; const values = Array.isArray(v) ? v.map(x => this.stringifyForCube(x)) @@ -451,7 +523,7 @@ export class MemoryAnalyticsService implements IAnalyticsService { // Otherwise treat as nested relation (e.g. {profile: {verified: true}}). // Flatten with dot-prefixed keys. for (const [nestedKey, nestedVal] of Object.entries(wrapper)) { - this.flattenFilterCondition({ [`${key}.${nestedKey}`]: nestedVal }, out); + this.flattenFilterCondition({ [`${key}.${nestedKey}`]: nestedVal }, out, here); } continue; } @@ -469,24 +541,20 @@ export class MemoryAnalyticsService implements IAnalyticsService { } /** - * Map MongoDB-style `$op` keys (from FilterCondition) to the cube-style - * operator names accepted by `convertOperatorToMongo` / `operatorToSql`. + * Lower a Filter Protocol `$op` key to the cube-style operator name + * `convertOperatorToMongo` / `operatorToSql` accept. + * + * [#5345] An operator with no row in {@link MONGO_TO_CUBE_OPERATOR} is + * REFUSED, not skipped. The gate in `normalizeFilters` refuses the same set + * one step earlier, so for a top-level or `$and`-nested constraint this throw + * is unreachable — but the nested-relation branch above re-enters this + * function with a synthesised `{'a.b': spec}` node the gate never saw, and + * that is a real path to an unmapped operator. It used to `continue`. */ - private mongoOperatorToCubeOperator(op: string): string | null { - switch (op) { - case '$eq': return 'equals'; - case '$ne': return 'notEquals'; - case '$gt': return 'gt'; - case '$gte': return 'gte'; - case '$lt': return 'lt'; - case '$lte': return 'lte'; - case '$in': return 'in'; - case '$nin': return 'notIn'; - case '$contains': return 'contains'; - case '$notContains': return 'notContains'; - case '$exists': return 'set'; - default: return null; - } + private mongoOperatorToCubeOperator(op: string, field: string, path: string): string { + const cubeOp = MONGO_TO_CUBE_OPERATOR[op]; + if (!cubeOp) throw uncompilableFieldOperatorError(op, field, path, ANALYTICS_FILTER_CAPABILITIES); + return cubeOp; } /** diff --git a/packages/plugins/driver-memory/src/memory-driver-filter-logic-conformance.test.ts b/packages/plugins/driver-memory/src/memory-driver-filter-logic-conformance.test.ts index 9690a3a745..064f6d7de4 100644 --- a/packages/plugins/driver-memory/src/memory-driver-filter-logic-conformance.test.ts +++ b/packages/plugins/driver-memory/src/memory-driver-filter-logic-conformance.test.ts @@ -2,7 +2,8 @@ /** * [#5324/#5328] Filter logical-combinator conformance for the LIVE QUERY PATH — - * `InMemoryDriver.find`, through `normalizeFilterCondition` and mingo. + * `InMemoryDriver.find`, through `normalizeFilterCondition` and mingo — and + * [#5345] for the ANALYTICS (cube) face beside it. * * # Why this file exists at all * @@ -32,17 +33,55 @@ * stays: the matcher is still the reference evaluator, and holding BOTH faces to * the same table is what makes "this package has two filter surfaces" a * statement someone can check. + * + * # The third face (#5345) + * + * It was never two. `memory-analytics.ts` compiles `AnalyticsQuery.where` with a + * third, unrelated lowering — into cube-style `{member, operator, values}` — and + * that one was outside the table too, for exactly as long and with exactly the + * consequence this file's opening paragraph predicts: it answered `$or` and + * `$not` by DROPPING them, so `{$or: [{a:'x'}, {b:'y'}]}` aggregated all four + * rows instead of three. Nothing failed, because nothing asked. + * + * A cube pipeline genuinely cannot express `$or` or `$not`, so this face cannot + * pass the table row-for-row and never will. That is not a reason to leave it + * unmeasured — it is the reason to measure the thing that actually matters: + * + * > For every case, the analytics face must either return the SAME ids as the + * > live query path, or REFUSE with `INVALID_FILTER`. It may never quietly + * > return a different set. + * + * That predicate is what the two silent `continue`s violated, it is what #3948 + * and ADR-0078 / #4286 each ruled on, and it stays true if the cube pipeline + * later learns `$or` — the case simply moves from the refused column to the + * agreeing one without this file changing. */ import { describe, it, expect, beforeAll } from 'vitest'; import { FILTER_LOGIC_CASES, FILTER_LOGIC_ROWS } from '@objectstack/spec/data'; -import type { FilterCondition } from '@objectstack/spec/data'; +import type { Cube, FilterCondition } from '@objectstack/spec/data'; import { InMemoryDriver } from './memory-driver.js'; +import { MemoryAnalyticsService } from './memory-analytics.js'; import { match } from './memory-matcher.js'; const TABLE = 'conformance'; +/** The conformance fixture as a cube: one row per id, so a query returns ids. */ +const CONFORMANCE_CUBE: Cube = { + name: TABLE, + title: 'Filter logic conformance', + sql: TABLE, + measures: { count: { name: 'count', label: 'Rows', type: 'count', sql: 'id' } }, + dimensions: Object.fromEntries( + (['id', 'a', 'b', 'c', 'owner', 'status', 'parent_object', 'parent_id'] as const).map((f) => [ + f, + { name: f, label: f, type: 'string' as const, sql: f }, + ]), + ), + public: true, +}; + describe('[#5324] InMemoryDriver.find — filter logic conformance (the LIVE query path)', () => { let driver: InMemoryDriver; @@ -105,3 +144,102 @@ describe('[#5324] InMemoryDriver.find — filter logic conformance (the LIVE que } }); }); + +describe('[#5345] MemoryAnalyticsService — the same table, through the THIRD filter face', () => { + let driver: InMemoryDriver; + let service: MemoryAnalyticsService; + + beforeAll(async () => { + driver = new InMemoryDriver({ persistence: false }); + await driver.connect(); + await driver.syncSchema(TABLE, { + fields: Object.fromEntries( + (['id', 'a', 'b', 'c', 'owner', 'status', 'parent_object', 'parent_id'] as const).map((f) => [ + f, + { type: 'text', name: f }, + ]), + ), + }); + for (const row of FILTER_LOGIC_ROWS) await driver.create(TABLE, { ...row }); + service = new MemoryAnalyticsService({ driver, cubes: [CONFORMANCE_CUBE] }); + }); + + /** + * The ids a case matches on the analytics face, or `REFUSED`. + * + * Grouping by `id` makes an aggregation answer the same question `find` + * answers — which is the only way to compare the two faces at all, since one + * returns records and the other returns counts. A widget's real symptom is the + * count; the ids behind it are how you see WHICH rows it counted. + */ + const REFUSED = Symbol('refused'); + const analyticsIds = async (where: FilterCondition): Promise => { + let result; + try { + result = await service.query({ + cube: TABLE, + measures: [`${TABLE}.count`], + dimensions: [`${TABLE}.id`], + where, + }); + } catch (error) { + const err = error as Error & { code?: string; status?: number }; + // Only a CATALOGUED refusal counts as a legitimate non-answer. An + // uncoded throw would be this face failing, not refusing, and must not + // be able to pass for conformance. + expect(err.code, `${JSON.stringify(where)} threw without the ADR-0112 envelope: ${err.message}`) + .toBe('INVALID_FILTER'); + expect(err.status).toBe(400); + return REFUSED; + } + return (result.rows as Array>) + .map((r) => String(r[`${TABLE}.id`])) + .sort((x, y) => x.localeCompare(y)); + }; + + it('the fixture really is all four rows', async () => { + expect(await analyticsIds({})).toEqual(['1', '2', '3', '4']); + }); + + /** + * The invariant, case by case: agree with the live path, or refuse. Never a + * third, quieter answer. + * + * Before #5345 the `$or` cases landed in neither column — they returned all + * four rows, which is the widening direction #3948 outlawed, and the read-scope + * cases at the bottom of the table are the ones where that widening is an + * unauthorized read rather than a wrong number. + */ + for (const c of FILTER_LOGIC_CASES) { + it(c.name, async () => { + const analytics = await analyticsIds(c.filter); + if (analytics === REFUSED) return; + expect(analytics, `${c.note ?? ''} — the analytics face answered without refusing, so it must AGREE`) + .toEqual([...c.expected]); + }); + } + + /** + * The table's cases are mostly combinator shapes, so most of them refuse here. + * That is the honest state of the cube pipeline — but a suite where EVERY case + * refuses would also pass if `query()` had simply stopped working, so pin both + * ends: at least one case must be genuinely answered, and the shapes the cube + * pipeline cannot express must be the ones refused. + */ + it('at least one case is answered, and every combinator case is refused rather than dropped', async () => { + const answered: string[] = []; + const refused: string[] = []; + for (const c of FILTER_LOGIC_CASES) { + ((await analyticsIds(c.filter)) === REFUSED ? refused : answered).push(c.name); + } + expect(answered.length, 'no case was answered at all — the face is broken, not merely narrow').toBeGreaterThan(0); + expect(refused.length, 'nothing was refused — the silent drop is back').toBeGreaterThan(0); + // Every case whose filter mentions a combinator this face cannot lower must + // be in the refused column, by name — not merely "some things were refused". + const uncompilable = FILTER_LOGIC_CASES.filter((c) => /"\$(or|not)"/.test(JSON.stringify(c.filter))); + expect(uncompilable.length).toBeGreaterThan(0); + for (const c of uncompilable) { + expect(refused, `${c.name}: a $or/$not case must refuse, never answer`).toContain(c.name); + } + }); +});