Skip to content

Commit c44f4f2

Browse files
committed
fix(spec): enforce the list-comparand rule at the shared compile face (#9228)
The #5869 rule ("a list operator takes a list") shipped at the objectql lowering seam, so it covered every query reaching a driver THROUGH the engine and nothing else. A caller that lowers with parseFilterAST and calls a driver directly met no gate; mingo's coercion of a non-array $in/$nin operand hid it until mingo 7.2.3 removed the coercion, and from 7.2.4 on the same input escapes as a raw TypeError with no code/status. Move the one implementation into packages/spec (filter-comparand-shape.ts) and call it from parseFilterAST, shape before type — the order the engine seam already applied. objectql's assertListComparandShapes becomes a delegating wrapper supplying the engine's caller prefix; parseFilterAST takes that prefix as an optional `context`, so the array branch keeps the message it has pinned since PR #6209. No driver behaviour is patched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HDA9nN6nXQngoQUAAzRdMb
1 parent 5904b05 commit c44f4f2

11 files changed

Lines changed: 768 additions & 255 deletions

File tree

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,53 @@
1+
---
2+
"@objectstack/spec": patch
3+
"@objectstack/objectql": patch
4+
---
5+
6+
fix(spec): enforce the list-comparand rule at the shared compile face, so a scalar `in`/`nin` no longer reaches a driver (#9228)
7+
8+
<!-- adr-0087: not-required (no-migration-prescription) No authorable key is
9+
added, renamed, retired or tombstoned. The change moves an existing runtime rule
10+
from `@objectstack/objectql` to `@objectstack/spec` and calls it from
11+
`parseFilterAST`; the authorable metadata surface is byte-identical. -->
12+
13+
`FieldOperatorsSchema` has always declared `$in` / `$nin` as `z.array(z.any())`
14+
and `$between` as `z.tuple([min, max])`, and #5869 / PR #6209 built the gate that
15+
enforces it — but only at `@objectstack/objectql`'s lowering seam. That covers
16+
every query reaching a driver **through the engine** and nothing else. A caller
17+
that lowers a filter with `parseFilterAST` and calls a driver directly — an
18+
embedder, and this repo's own driver conformance suites — met no gate at all:
19+
`parseFilterAST([['name', 'notin', 'alpha']])` returned `{ name: { $nin:
20+
'alpha' } }`, a shape the contract forbids, and handed it over.
21+
22+
That path was carried by mingo's own coercion of a non-array `$in`/`$nin`
23+
operand. mingo 7.2.3 removed the coercion, so from 7.2.4 on the same input
24+
escapes as an unhandled third-party `TypeError: b.filter is not a function` —
25+
no `code`, no `status`, no field name — straight to the caller. It is the sole
26+
failure blocking the `mingo` 7.2.2 -> 7.2.4 bump.
27+
28+
**Fixed at the shared face, with exactly one implementation.** The rule now
29+
lives in `@objectstack/spec`'s `data/filter-comparand-shape.ts`, the same place
30+
the comparand-TYPE door (#7872 / PR #8234) was promoted to for the same reason
31+
("enforced once at the shared compile face for all five drivers"), and
32+
`parseFilterAST` runs it on everything it returns — shape first, then type, the
33+
order the engine's own seam already applied. `@objectstack/objectql`'s
34+
`assertListComparandShapes` is now a delegating wrapper whose only remaining job
35+
is the engine's `find('deal'): ` caller prefix; no driver was patched (both
36+
driver families are under the #5499 investment freeze).
37+
38+
**The accept/reject delta is narrow and one-directional.** Newly refused, with
39+
the ADR-0112 `INVALID_FILTER` / 400 envelope: a non-array `$in` / `$nin`
40+
comparand and a non-`[min, max]` `$between` comparand, reaching a driver via a
41+
direct `parseFilterAST` call. Nothing else changes — every filter the engine
42+
accepted still lowers byte-identically, `$in: []` / `$nin: []` remain legitimate
43+
declared predicates, list MEMBER types stay unjudged here, and a field spec with
44+
no `$` key is still not descended into. The same inputs were already refused
45+
with the same envelope on every engine verb and at the REST ingress, so no
46+
authored metadata in the repo or in `objectui` produces a shape that newly
47+
fails: a survey of `examples/**`, `content/docs/**`, fixtures, seeded platform
48+
objects and objectui's view definitions found every membership rule already
49+
carrying an array.
50+
51+
`parseFilterAST` gains an optional second argument, `context` — the caller
52+
prefix both doors in `@objectstack/spec` already take. It is additive and
53+
defaulted; existing calls are unaffected.

‎packages/drivers/driver-memory/src/memory-filter-ast-vocabulary.test.ts‎

Lines changed: 51 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -77,10 +77,36 @@ describe('InMemoryDriver filter vocabulary ↔ VALID_AST_OPERATORS', () => {
7777
expect(VALID_AST_OPERATORS.size).toBeGreaterThan(0);
7878
});
7979

80+
/**
81+
* [#9228] Which `$` operator an authoring spelling lowers to, derived by
82+
* LOWERING one rather than by hand.
83+
*
84+
* `valueFor` used to name the membership spellings in a literal list, and it
85+
* named three of the four: `notin` (and, had it been in the vocabulary, any
86+
* later spelling) fell through to the scalar default. That handed the driver
87+
* `{ name: { $nin: 'alpha' } }` — a shape the declared contract forbids —
88+
* which mingo silently coerced until 7.2.3 and answers with a raw
89+
* `TypeError: b.filter is not a function` from 7.2.4 on. The accident was
90+
* load-bearing: it was the one probe covering the shape hole #9228 closed.
91+
* Deriving the value from the spec's own lowering closes the CLASS instead of
92+
* the instance — a membership spelling added to `AST_OPERATOR_MAP` tomorrow
93+
* gets a list here without anyone remembering to edit this line.
94+
*
95+
* The probe uses a two-element array, which is legal for all three list
96+
* operators, so it never trips the shape door it is used to satisfy.
97+
*/
98+
const loweredOperatorOf = (op: string): string | undefined => {
99+
const lowered = parseFilterAST([['probe', op, ['a', 'b']]]) as Record<string, unknown> | undefined;
100+
const spec = lowered?.probe;
101+
if (spec === null || typeof spec !== 'object' || Array.isArray(spec)) return undefined;
102+
return Object.keys(spec).find((key) => key.startsWith('$'));
103+
};
104+
80105
/** A representative value per operator, so each one is actually exercised. */
81106
const valueFor = (op: string): unknown => {
82-
if (op === 'in' || op === 'nin' || op === 'not_in') return ['alpha'];
83-
if (op === 'between') return [0, 100];
107+
const lowered = loweredOperatorOf(op);
108+
if (lowered === '$in' || lowered === '$nin') return ['alpha'];
109+
if (lowered === '$between') return [0, 100];
84110
if (/null|empty/.test(op)) return true;
85111
if (/contains|like|startswith|starts_with|endswith|ends_with/.test(op)) return 'alp';
86112
return 'alpha';
@@ -141,6 +167,29 @@ describe('InMemoryDriver filter vocabulary ↔ VALID_AST_OPERATORS', () => {
141167
expect(err.message).toContain('[min, max]');
142168
});
143169

170+
it.each([['in'], ['nin'], ['not_in'], ['notin']])(
171+
'[#9228] refuses a scalar comparand on %s instead of letting it reach mingo',
172+
async (op) => {
173+
// The escape this card closed. `valueFor` above hands every membership
174+
// spelling a list now, so nothing in this suite produces the shape any
175+
// more — which is exactly why it is pinned HERE, deliberately, rather
176+
// than left to the accident that used to cover it.
177+
//
178+
// Before #9228 the shape reached `Query.compile` and answered two ways
179+
// depending on a transitive dependency's minor version: mingo <= 7.2.2
180+
// coerced it and returned rows; mingo >= 7.2.3 threw
181+
// `TypeError: b.filter is not a function` — no `code`, no `status`, no
182+
// field name — which is why BOTH envelope halves are asserted and not
183+
// just "it throws". A bare `toThrow()` here passed on 7.2.4 while the
184+
// defect was wide open.
185+
const err = await refusalOf(() => findAuthored([['name', op, 'alpha']]));
186+
expect(err.code).toBe('INVALID_FILTER');
187+
expect(err.status).toBe(400);
188+
expect(err.message).toContain('requires an ARRAY');
189+
expect(err.message).toContain('name');
190+
},
191+
);
192+
144193
it('still honours a well-formed logical node', async () => {
145194
const rows = await findAuthored(['or', ['name', '=', 'alpha'], ['name', '=', 'beta']]);
146195
expect(rows.map((r: any) => r.id).sort()).toEqual(['1', '2']);

‎packages/objectql/src/engine.ts‎

Lines changed: 15 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -722,7 +722,13 @@ function lowerWhereFilterArray<T extends object | undefined>(
722722
}
723723

724724
// (2) The declared path.
725-
const condition = parseFilterAST(where);
725+
// [#9228] The context prefix is passed because `parseFilterAST` now runs the
726+
// #5869 shape gate itself, one step before this branch could: a scalar `$nin`
727+
// is refused DURING lowering. Handing it `${operation}('${object}')` is what
728+
// keeps this branch's pinned `find('deal'): ` prefix on that refusal — and on
729+
// the #7872 type door's, which `parseFilterAST` has always run and which used
730+
// to answer this branch unprefixed.
731+
const condition = parseFilterAST(where, `${operation}('${object}')`);
726732
if (condition === undefined) {
727733
// Unreachable by construction — `isFilterAST` accepted the shape, so
728734
// `parseFilterAST` has a lowering for it. Loud rather than silent because
@@ -734,11 +740,14 @@ function lowerWhereFilterArray<T extends object | undefined>(
734740
`unfiltered (#5158).`,
735741
);
736742
}
737-
// [#5869] Door 2's half of the same check. `isFilterAST` vouched for the
738-
// OPERATOR and `parseFilterAST` lowered it, but neither looks at the
739-
// comparand — `['status', 'not_in', 'done']` lowers to `{status: {$nin:
740-
// 'done'}}` and a scalar `$nin` is what reached the driver as a 500.
741-
assertListComparandShapes(object, operation, condition);
743+
// [#5869] Door 2's half of the same check USED to be a second
744+
// `assertListComparandShapes(object, operation, condition)` call here.
745+
// [#9228] moved the rule into `parseFilterAST` itself, which this branch
746+
// calls three lines up with the same context — so the call became provably
747+
// unreachable (nothing between the two can introduce a list operator) and is
748+
// deleted rather than kept as a gate that reads live and never fires. The
749+
// OBJECT branch above keeps its call: that `where` never passes
750+
// `parseFilterAST` at all, which is the whole reason both branches exist.
742751
// [#8296] Same door as the object branch above, on the LOWERED condition —
743752
// the array sugar (`[['is_open','=',true]]`) names fields too, and a gate on
744753
// one branch would answer one mistake two ways depending on the spelling.

0 commit comments

Comments
 (0)