Skip to content

Commit 9ce0460

Browse files
committed
fix(driver-memory): the analytics face refuses a filter it cannot compile instead of dropping it (#5345)
`MemoryAnalyticsService` lowers `AnalyticsQuery.where` into a flat cube-style `{member, operator, values}` list, and answered everything that did not fit with `continue` — `$or` and `$not` wholesale, plus the five declared operators with no row in the mongo→cube table (`$between`, `$startsWith`, `$endsWith`, `$null`, `$regex`). A comment presented this as a feature: "ignore so a partial query still runs rather than failing entirely" — the identical reasoning ADR-0078 / #4286 judged wrong on `objectql`'s `having`. Deleted along with the behaviour it justified. The direction is what makes it a defect: a dropped predicate is FEWER constraints, therefore MORE rows. Measured on `FILTER_LOGIC_CASES`, 15 of its 17 cases returned a wider row set than the standard specifies, usually every row. `$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. Route 1 of the issue's two: refuse, do not backfill the cube pipeline — where #5366 and #5368 put the two neighbouring faces. Implementation reuses #5349's primitives rather than adding a second validator. `assertFilterConditionShape` now takes the calling face's `FilterFaceCapabilities` — what that face can COMPILE, distinct from what the protocol declares — and refuses the difference through `unsupportedFilterError` (INVALID_FILTER / 400). The query path and the matcher pass nothing and are byte-for-byte unaffected. The analytics face derives its capabilities from its own `MONGO_TO_CUBE_OPERATOR` table, so widening what it accepts and teaching it to compile the operator are now one edit. Two refusals distinct on purpose: an operator the protocol never declared is still `unknownFieldOperatorError` ("you have a typo"), one it declares and this face cannot lower is the new `uncompilableFieldOperatorError`. The gate runs in `normalizeFilters`, before any lowering, for the reason that module already documents — a refusal raised mid-lowering fires or not depending on key order. Both public entry points (`query()`, `generateSql()`) go through it. The two former `continue` sites now throw; the operator one is reachable via the nested-relation branch, which re-enters with a synthesised node the gate never walked. Tests: `FILTER_LOGIC_CASES` now covers this third face — the package had three filter surfaces and the shared table watched two. It cannot pass row-for-row (a cube pipeline has no `$or`), so it is held to the predicate that actually matters: agree with `find()`, or refuse with INVALID_FILTER, never a third quieter answer. Reverting only `memory-analytics.ts` fails 16 of the new assertions. Plus a dedicated suite asserting the envelope (code, status) and that each refusal names the offending operator or combinator. Out of scope, filed not fixed: #5373 (the cube comparand round-trip loses booleans and `null`), #5374 (`$notContains` lowers to a bare mingo `{$not: 'x'}` that constrains nothing). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Pbu27iNUfQCHeuS551Rqo7
1 parent 9c5abf4 commit 9ce0460

5 files changed

Lines changed: 722 additions & 70 deletions

File tree

Lines changed: 87 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,87 @@
1+
---
2+
"@objectstack/driver-memory": minor
3+
---
4+
5+
fix(driver-memory): the analytics (cube) face REFUSES a filter it cannot compile instead of silently dropping it (#5345)
6+
7+
**This is an observable behaviour change on a shipped surface, and it will turn
8+
some working-looking dashboards red.** That is the point: the widgets it breaks
9+
were returning inflated aggregates, and some of them were returning rows the
10+
caller had no permission to read.
11+
12+
## What was happening
13+
14+
`MemoryAnalyticsService` lowers `AnalyticsQuery.where` into a flat, cube-style
15+
`{member, operator, values}` list. Anything that did not fit was answered with
16+
`continue` — in two places, and with a comment presenting it as a feature
17+
("ignore so a partial query still runs rather than failing entirely"):
18+
19+
| dropped | why it did not fit |
20+
|---|---|
21+
| `$or` (whole branch) | no expression in a flat AND-list |
22+
| `$not` (whole branch) | same |
23+
| `$between` | no row in the mongo→cube operator table |
24+
| `$startsWith` / `$endsWith` | same |
25+
| `$null` | same |
26+
| `$regex` | same — and `plugin-auth`'s ObjectQL adapter emits it |
27+
28+
Dropping a predicate does not narrow a query, it **widens** it: fewer
29+
constraints means more rows. A widget filtered to two stages with
30+
`{$or: [{stage: 'won'}, {stage: 'lost'}]}` aggregated the **entire table** and
31+
rendered as a perfectly normal chart. Measured on the shared
32+
`FILTER_LOGIC_CASES` fixture, **15 of its 17 cases** returned a wider row set
33+
than the standard specifies — usually every row. Of the two that did agree, one
34+
(`a $or nested under a top-level $and`) agreed by *coincidence*: its dropped
35+
`$or` happened to be redundant against a surviving sibling key, which is the
36+
best illustration available of why "the number looked right" was never evidence.
37+
38+
`$not` makes it more than a wrong number. `cel-to-filter.ts` compiles a CEL
39+
`!expr` RLS read scope into `{$not: {…}}`, so the dropped branch was the read
40+
scope itself — the aggregate included records the caller is not allowed to see.
41+
42+
## What changes for you
43+
44+
A `where` carrying any of the shapes above now raises **`INVALID_FILTER` / 400**
45+
(the ADR-0112 envelope every sibling filter refusal in this driver already
46+
speaks, reaching REST callers as a 400 since #5366) naming the offending
47+
operator or combinator and its position, e.g.:
48+
49+
> Filter operator `"$between"` on field `"amount"` at `where.amount` is declared
50+
> by the Filter Protocol but cannot be compiled by driver-memory's analytics
51+
> (cube) face. Supported operators on this surface: `$eq, $ne, $gt, $gte, $lt,
52+
> $lte, $in, $nin, $contains, $notContains, $exists`.
53+
54+
Both entry points refuse identically — `query()` and `generateSql()`.
55+
56+
**The fix, per shape:**
57+
58+
- `$between` on a range → the two bounds, which this face has always compiled:
59+
`{ closed_at: { $gte: '2026-01-01', $lte: '2026-01-31' } }`, or a
60+
`timeDimensions[].dateRange`, which is unaffected.
61+
- `$startsWith` / `$endsWith` / `$regex``$contains`, or move the query to
62+
`find()`.
63+
- `$null``{ field: { $exists: false } }` for the absent case.
64+
- `$or` / `$not` → restate as the implicit AND of field keys where the intent
65+
allows it; where it does not, the cube pipeline genuinely cannot express it,
66+
and the query belongs on `find()`.
67+
68+
Nothing that was **compiled** changes. All eleven supported operators, `$and`,
69+
implicit equality, nested-relation flattening, time dimensions and the empty
70+
filter produce byte-identical pipelines.
71+
72+
## Why refuse rather than teach the cube pipeline `$or`
73+
74+
This is the call ADR-0078 / #4286 made for `objectql`'s `having` — an ignored
75+
operator there "silently returns UNFILTERED aggregates", so it throws — and the
76+
posture #3948 established for every filter backend: a filter that cannot be
77+
compiled is refused loudly, never skipped. It is also where the two neighbouring
78+
faces landed (#5366, #5368).
79+
80+
Mechanically, the refusal is not a new check bolted onto this face. It reuses
81+
the package's single filter gate, `assertFilterConditionShape`, which now takes
82+
the calling face's declared capabilities; and the analytics face derives those
83+
capabilities from its own mongo→cube operator table, so widening what it accepts
84+
and teaching it to compile the operator are now the same edit. The shared
85+
`FILTER_LOGIC_CASES` conformance table covers this third face for the first time
86+
(it watched only two of the driver's three), holding it to: agree with
87+
`find()`, or refuse — never a third, quieter answer.

packages/plugins/driver-memory/src/filter-refusal.ts

Lines changed: 162 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -4,23 +4,33 @@
44
* The filter refusals this driver raises, in ONE place — and, since #5324/#5328,
55
* the ONE walk that decides which shapes are refused at all.
66
*
7-
* Both of this package's filter surfaces refuse the same shapes with the same
8-
* wire envelope: the live query path (`memory-driver.ts` → mingo) and the
7+
* All THREE of this package's filter surfaces refuse the same shapes with the
8+
* same wire envelope: the live query path (`memory-driver.ts` → mingo), the
99
* reference matcher (`memory-matcher.ts`, the record-at-a-time evaluator the
10-
* conformance suites hold against `driver-sql` and `@objectstack/formula`). They
11-
* were two independent code paths with two independent notions of what a filter
12-
* may be, which is exactly how #5240's divergence survived unnoticed in-package.
13-
*
14-
* #5240 gave the two faces one refusal by writing the same check twice. That was
15-
* still two implementations of one rule, and the shapes #5324/#5328 measured
16-
* proved how far apart two such implementations drift: given a malformed
17-
* `$between` the live path answered "no rows" while the matcher answered "EVERY
18-
* row" — opposite answers, inside one package, to one filter. So the rule now
19-
* lives in exactly one function, {@link assertFilterConditionShape}, and both
20-
* faces call it before they evaluate anything.
10+
* conformance suites hold against `driver-sql` and `@objectstack/formula`), and
11+
* since #5345 the analytics/cube face (`memory-analytics.ts`). They were
12+
* independent code paths with independent notions of what a filter may be, which
13+
* is exactly how #5240's divergence survived unnoticed in-package.
14+
*
15+
* #5240 gave the first two faces one refusal by writing the same check twice.
16+
* That was still two implementations of one rule, and the shapes #5324/#5328
17+
* measured proved how far apart two such implementations drift: given a
18+
* malformed `$between` the live path answered "no rows" while the matcher
19+
* answered "EVERY row" — opposite answers, inside one package, to one filter. So
20+
* the rule now lives in exactly one function, {@link assertFilterConditionShape},
21+
* and every face calls it before it evaluates anything.
22+
*
23+
* [#5345] The faces are not equally capable, and pretending they were is what
24+
* kept the third one out. `memory-analytics` lowers a `where` into a cube-style
25+
* `{member, operator, values}` list, and that pipeline expresses neither `$or`
26+
* nor `$not` nor five of the declared field operators. Its answer used to be a
27+
* `continue`. So the walk now takes the calling face's {@link
28+
* FilterFaceCapabilities} — what that face can COMPILE — and refuses what it
29+
* cannot, in the same envelope, from the same place. A face declares its
30+
* vocabulary; it does not get to drop what falls outside it.
2131
*/
2232

23-
import { FILTER_OPERATORS } from '@objectstack/spec/data';
33+
import { FILTER_OPERATORS, LOGICAL_OPERATORS } from '@objectstack/spec/data';
2434
import { StandardErrorCode } from '@objectstack/spec/api';
2535

2636
/**
@@ -165,6 +175,108 @@ export const SUPPORTED_FIELD_OPERATORS: ReadonlySet<string> = new Set<string>([
165175
/** The vocabulary as it appears in a refusal message, in declaration order. */
166176
const SUPPORTED_FIELD_OPERATOR_LIST = [...SUPPORTED_FIELD_OPERATORS].join(', ');
167177

178+
/**
179+
* [#5345] What ONE evaluation face can COMPILE — the narrower vocabulary a
180+
* particular surface enforces on top of the package-wide one above.
181+
*
182+
* The distinction this type draws is the whole of #5345. Two different things
183+
* can be wrong with `{ amount: { $sounds_like: 3 } }` and
184+
* `{ amount: { $between: [1, 3] } }` on the analytics face:
185+
*
186+
* - the first names an operator the **Filter Protocol** does not declare — it is
187+
* wrong everywhere, and {@link unknownFieldOperatorError} says so;
188+
* - the second is a declared operator this **face** cannot lower into its cube
189+
* pipeline. It is a perfectly good filter that `find()` runs today.
190+
*
191+
* Before #5345 the second class was answered with `continue`, silently, on the
192+
* analytics face only. A face that declares its vocabulary here gets the second
193+
* class refused for it, by the same walk, in the same envelope — and, crucially,
194+
* cannot answer it any other way, because the walk runs before the face's
195+
* lowering code is reached.
196+
*
197+
* Derive the sets from the face's own lowering table rather than hand-listing
198+
* them (see `MONGO_TO_CUBE_OPERATOR` in `memory-analytics.ts`): a hand-written
199+
* copy agrees with the compiler on the day it is typed and never again, which is
200+
* the note already sitting over {@link SUPPORTED_FIELD_OPERATORS}.
201+
*/
202+
export interface FilterFaceCapabilities {
203+
/** How the face names itself in a refusal, e.g. `"the analytics (cube) face"`. */
204+
readonly face: string;
205+
/** The field operators this face lowers. A subset of {@link SUPPORTED_FIELD_OPERATORS}. */
206+
readonly fieldOperators: ReadonlySet<string>;
207+
/** The logical combinators this face lowers. A subset of `LOGICAL_OPERATORS`. */
208+
readonly combinators: ReadonlySet<string>;
209+
}
210+
211+
/**
212+
* [#5345] The default: the whole vocabulary this driver's query path and
213+
* reference matcher evaluate. Passing no capabilities means "this face compiles
214+
* everything the driver does", which is true of both of them and keeps every
215+
* pre-#5345 call site behaving byte-for-byte as before.
216+
*/
217+
export const DRIVER_FILTER_CAPABILITIES: FilterFaceCapabilities = Object.freeze({
218+
face: 'this driver',
219+
fieldOperators: SUPPORTED_FIELD_OPERATORS,
220+
combinators: new Set<string>(LOGICAL_OPERATORS),
221+
});
222+
223+
/**
224+
* [#5345] A DECLARED field operator that this face cannot lower.
225+
*
226+
* Distinct from {@link unknownFieldOperatorError} on purpose: that one means
227+
* "the Filter Protocol has no such operator", this one means "the protocol has
228+
* it, `find()` runs it, and this surface cannot". Collapsing them would tell a
229+
* dashboard author their `$between` is a typo.
230+
*
231+
* The tail is the #3948 rule stated in the direction that matters here. A
232+
* dropped predicate does not narrow a query, it WIDENS it: the aggregate is
233+
* computed over rows the author excluded, and a chart drawn over them looks
234+
* exactly like a working chart. This is ADR-0078 / #4286's call on `objectql`'s
235+
* `having`, which was refused rather than skipped for the identical reason.
236+
*/
237+
export function uncompilableFieldOperatorError(
238+
op: string,
239+
field: string,
240+
path: string,
241+
capabilities: FilterFaceCapabilities,
242+
): Error {
243+
const supported = [...capabilities.fieldOperators].join(', ') || '(none)';
244+
return unsupportedFilterError(
245+
`Filter operator "${op}" on field "${field}" at ${path} is declared by the Filter Protocol ` +
246+
`but cannot be compiled by ${capabilities.face}. Supported operators on this surface: ` +
247+
`${supported}. It is refused rather than dropped: a predicate that compiles to nothing does ` +
248+
`not narrow the query, it WIDENS it — the aggregate is then computed over rows the filter ` +
249+
`excluded, and a chart drawn over them looks like a working chart (#3948, #4286/ADR-0078, ` +
250+
`#5345). Rewrite the predicate with a supported operator, or run it through find().`,
251+
);
252+
}
253+
254+
/**
255+
* [#5345] A DECLARED logical combinator that this face cannot lower.
256+
*
257+
* Named separately from {@link unknownLogicalOperatorError} for the same reason
258+
* as the field-operator pair above, and it is the sharper half of #5345: a
259+
* dropped `$or` discards a whole branch of the filter, and `$not` is precisely
260+
* what `cel-to-filter.ts` compiles a CEL `!expr` RLS read scope into. Dropping
261+
* that one does not make a number inaccurate — it puts rows the caller has no
262+
* permission to read into the aggregate.
263+
*/
264+
export function uncompilableCombinatorError(
265+
key: string,
266+
path: string,
267+
capabilities: FilterFaceCapabilities,
268+
): Error {
269+
const supported = [...capabilities.combinators].join(', ') || '(none)';
270+
return unsupportedFilterError(
271+
`Filter combinator "${key}" at ${path} is declared by the Filter Protocol but cannot be ` +
272+
`compiled by ${capabilities.face}. Supported combinators on this surface: ${supported}. ` +
273+
`It is refused rather than ignored: dropping a combinator discards a whole branch of the ` +
274+
`filter and WIDENS the result set, and "$not" is what compileCelToFilter emits for a CEL ` +
275+
`"!expr" RLS read scope — a dropped one is an over-permissive read, not an inaccurate ` +
276+
`number (#3948, #5345).`,
277+
);
278+
}
279+
168280
/** A short type name for an operand a filter refusal has to describe. */
169281
function describeFilterOperand(value: unknown): string {
170282
if (value === null) return 'null';
@@ -383,27 +495,46 @@ export function filterNodeExpectedError(value: unknown, path: string): Error {
383495
* - the MEMBER types of a `$between` array — `driver-sql` checks its arity and
384496
* nothing else (#5041 measured the member case and deliberately left it);
385497
* - a stringified comparand for the `LIKE` family — same, and fail-closed.
498+
*
499+
* ## What `capabilities` adds (#5345)
500+
*
501+
* Shape is universal; CAPABILITY is per-face. `capabilities` narrows what this
502+
* particular caller can lower — see {@link FilterFaceCapabilities} — and the
503+
* walk refuses the difference. It defaults to
504+
* {@link DRIVER_FILTER_CAPABILITIES}, i.e. everything, so the query path and the
505+
* matcher are unaffected.
506+
*
507+
* The capability check is made BEFORE the shape checks at the same key, and
508+
* deliberately: on a face that cannot compile `$or` at all, reporting that its
509+
* operand should have been an array would send the author to fix the wrong
510+
* thing, then refuse the corrected filter anyway.
386511
*/
387-
export function assertFilterConditionShape(node: unknown, path: string): void {
512+
export function assertFilterConditionShape(
513+
node: unknown,
514+
path: string,
515+
capabilities: FilterFaceCapabilities = DRIVER_FILTER_CAPABILITIES,
516+
): void {
388517
if (!isFilterNode(node)) return;
389518
for (const [key, value] of Object.entries(node)) {
390519
const here = `${path}.${key}`;
391520
if (key === '$and' || key === '$or') {
521+
if (!capabilities.combinators.has(key)) throw uncompilableCombinatorError(key, here, capabilities);
392522
if (!Array.isArray(value)) throw filterNodeListExpectedError(key, value, here);
393523
value.forEach((child, index) => {
394524
const childPath = `${here}[${index}]`;
395525
if (!isFilterNode(child)) throw filterNodeExpectedError(child, childPath);
396-
assertFilterConditionShape(child, childPath);
526+
assertFilterConditionShape(child, childPath, capabilities);
397527
});
398528
continue;
399529
}
400530
if (key === '$not') {
531+
if (!capabilities.combinators.has(key)) throw uncompilableCombinatorError(key, here, capabilities);
401532
if (!isFilterNode(value)) throw filterNodeExpectedError(value, here);
402-
assertFilterConditionShape(value, here);
533+
assertFilterConditionShape(value, here, capabilities);
403534
continue;
404535
}
405536
if (key.startsWith('$')) throw unknownLogicalOperatorError(key, here);
406-
assertFieldConstraintShape(key, value, here);
537+
assertFieldConstraintShape(key, value, here, capabilities);
407538
}
408539
}
409540

@@ -418,7 +549,12 @@ export function assertFilterConditionShape(node: unknown, path: string): void {
418549
* is: the two faces silently disagreed about it (the matcher ignored the
419550
* non-`$` key, mingo did not).
420551
*/
421-
function assertFieldConstraintShape(field: string, spec: unknown, path: string): void {
552+
function assertFieldConstraintShape(
553+
field: string,
554+
spec: unknown,
555+
path: string,
556+
capabilities: FilterFaceCapabilities,
557+
): void {
422558
if (!isFilterNode(spec)) return;
423559
// [#5240] The zero-operator constraint keeps its own predicate rather than an
424560
// 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):
429565
if (!keys.some((key) => key.startsWith('$'))) return;
430566
for (const op of keys) {
431567
if (!SUPPORTED_FIELD_OPERATORS.has(op)) throw unknownFieldOperatorError(op, field, path);
568+
// [#5345] Declared, but not by THIS face. Checked before the comparand-shape
569+
// rules below so a `$between` a face cannot compile is reported as
570+
// unsupported-here rather than as a malformed range the face would refuse
571+
// even once corrected.
572+
if (!capabilities.fieldOperators.has(op)) {
573+
throw uncompilableFieldOperatorError(op, field, path, capabilities);
574+
}
432575
if (op === '$between' && !isBetweenComparand(spec[op])) {
433576
throw malformedBetweenError(field, spec[op], `${path}.$between`);
434577
}

0 commit comments

Comments
 (0)