Skip to content

Commit 10c4ea9

Browse files
baozhoutaoclaude
andauthored
fix(objectql): 集合算子的标量比较值答 400 INVALID_FILTER 并点名期望形状,不再 500 (#5869) (#6209)
* fix(objectql): collection operators answer 400 INVALID_FILTER for a scalar comparand (#5869) `FieldOperatorsSchema` declares `$in`/`$nin` as arrays and `$between` as a [min, max] tuple, but nothing enforced that on the way in: `isFilterAST` checks only the OPERATOR and `parseFilterAST` lowers whatever comparand it is handed, so `['status', 'not_in', 'done']` became `{ status: { $nin: 'done' } }` and reached the driver, where `driver-sql` handed a scalar to `whereIn()` and answered 500 DATABASE_ERROR -- a server-fault code for a filter the caller can fix, naming neither the operator nor the field. The gate goes at the engine's single filter collection point rather than in each driver: three backends answered three different ways for one declared contract, and both driver families are under an investment freeze (#5499). It runs on the LOWERED condition, which is what makes it cover the door the defect was actually measured through: the protocol face runs its own isFilterAST -> parseFilterAST and hands the engine a FilterCondition OBJECT, so a guard on the array branch alone would have left the reported 500 exactly where it was. `$between` arity is hoisted to the same seam -- driver-sql and driver-memory each already refuse it (wording kept verbatim), while driver-mongodb's arm falls through without emitting a range predicate. Not judged here: empty lists (declared predicates), list MEMBER types (#5234, another face), and non-collection operators. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019Q7oc7ASjh8yxyS3Yz78We * test(objectql): type the new #5869 call sites instead of erasing them to `any` `check:query-options-erasure` went red: "test surface grew 267 -> 289". The 22 new engine call sites in engine-filter-array-lowering.test.ts all carried a bare `as any`, which the #4918 ratchet counts. Both remedies the gate names are used, split by what `tsc` actually says about each input rather than applied uniformly: - `as unknown as EngineQueryOptions` (via the `asFilterArrayQuery` helper, and the `EngineCountOptions` / `EngineAggregateOptions` twins on count/aggregate) for the FilterArray inputs. Those are off-contract BY DECLARATION -- `where` is a FilterCondition / Record<string, unknown> that an array is not assignable to, because FilterArray is INPUT-ONLY sugar the spec excludes (#5285). - The assertion simply DROPPED on the malformed-comparand cases (`{ stage: { $nin: 'won' } }`). Those type-check fine, because `where` is declared loosely on purpose -- which is precisely why the runtime gate this file pins has to exist. Erasing them would have hidden that they are type-legal. The 23 pre-existing sites in this file are untouched, as are the baseline JSON and eslint.config.mjs -- the ceiling is met by fixing the new sites, not by raising the number. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019Q7oc7ASjh8yxyS3Yz78We --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 70f132c commit 10c4ea9

4 files changed

Lines changed: 531 additions & 1 deletion

File tree

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
---
2+
"@objectstack/objectql": patch
3+
---
4+
5+
fix(objectql): 集合算子的标量比较值答 400 INVALID_FILTER 并点名期望形状,不再 500 DATABASE_ERROR
6+
7+
`FieldOperatorsSchema` 声明 `$in` / `$nin` 的比较值是数组、`$between``[min, max]` 二元组,但入口处没有任何一层强制这条声明:`isFilterAST` 只看算子,`parseFilterAST` 照单下降,于是 `['status', 'not_in', 'done']` 变成 `{ status: { $nin: 'done' } }` 一路走到驱动。
8+
9+
**行为变化(用户可见)**:此前 `driver-sql` 把标量交给 `whereIn(field, scalar)`,答 **500 `DATABASE_ERROR`** —— 用服务端故障码报告一个调用方能自己改好的过滤器,且不说明是哪个算子、哪个字段、该写成什么。现在引擎在唯一收口点拒收,答 **400 `INVALID_FILTER`**,信息点名算子(同时给出 `not_in` / `nin` / `notin` 这类作者实际书写的拼法)、字段、收到的值与位置、以及可直接粘贴的正确形状,并声明该过滤器**未被应用**
10+
11+
覆盖两道门:直接调用引擎(`FilterArray` 下降路径)与 HTTP 面(协议层已自行下降成 `FilterCondition` 对象后再交给引擎)—— 后者正是本问题实测到的那道门。`find` / `findOne` / `count` / `aggregate` / `update` / `delete` 六个入口一致。
12+
13+
`$between` 的非二元组比较值一并收在同一处:`driver-sql``driver-memory` 各自已经拒收(措辞保持逐字一致),`driver-mongodb` 的分支则直接落空、不发射区间谓词 —— 收在收口点后三家答案一致。
14+
15+
**不变的**:`$in: []` / `$nin: []` 仍是合法谓词(分别表示「不匹配任何行」与「匹配所有行」);列表**成员**的类型不在此处复判(那是 #5234,另一个面);非集合算子的标量比较值不受影响,包括 `$gt` 的 ISO 日期字符串这类 `FieldOperatorsSchema` 声明更严、而各后端一致接受的形状。

packages/objectql/src/engine-filter-array-lowering.test.ts

Lines changed: 221 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,8 +26,32 @@
2626
*/
2727

2828
import { describe, it, expect, beforeEach } from 'vitest';
29+
import type {
30+
EngineAggregateOptions,
31+
EngineCountOptions,
32+
EngineQueryOptions,
33+
} from '@objectstack/spec/data';
2934
import { ObjectQL } from './engine.js';
3035

36+
/**
37+
* [#4918] `FilterArray` on `where` is off-contract BY DECLARATION, and these
38+
* tests exist to drive it: `EngineQueryOptions.where` is a `FilterCondition` /
39+
* `Record< string, unknown >`, which an array is not assignable to, because
40+
* `FilterArray` is INPUT-ONLY authoring sugar the spec deliberately excludes
41+
* (#5285). So a test that hands the engine one has to say so, and
42+
* `as unknown as EngineQueryOptions` is how: it names the contract being
43+
* bypassed, keeps the rest of the call type-checked, and greps as an
44+
* intentional act — none of which a bare `as any` does.
45+
*
46+
* Deliberately NOT used for the malformed-COMPARAND cases below
47+
* (`{ stage: { $nin: 'won' } }`). Those are ordinary objects that `tsc`
48+
* accepts, because `where` is declared loosely on purpose — which is the whole
49+
* reason the runtime gate this file pins has to exist. Erasing them would hide
50+
* that they are type-legal, which is the point.
51+
*/
52+
const asFilterArrayQuery = (where: unknown): EngineQueryOptions =>
53+
({ where }) as unknown as EngineQueryOptions;
54+
3155
const deal = {
3256
name: 'deal',
3357
label: 'Deal',
@@ -304,6 +328,203 @@ describe('Door 2 lowers FilterArray to FilterCondition before the driver (#5158)
304328
expect(reads).toHaveLength(0);
305329
});
306330

331+
// ── #5869: the list-shaped operators' comparands ──────────────────────
332+
//
333+
// `isFilterAST` vouches for the OPERATOR and nothing else, and
334+
// `parseFilterAST` lowers whatever comparand it is handed. So the shapes
335+
// below passed both, reached the driver, and — on driver-sql, via
336+
// `whereIn(field, scalar)` — came back as `500 DATABASE_ERROR`: a
337+
// server-fault code for a filter the caller can fix, naming neither the
338+
// operator nor the field. Same collection point, same envelope as the
339+
// refusals above.
340+
341+
it.each([
342+
['not_in', [['stage', 'not_in', 'won']]],
343+
['nin', [['stage', 'nin', 'won']]],
344+
['notin', [['stage', 'notin', 'won']]],
345+
['in', [['stage', 'in', 'won']]],
346+
])('refuses a scalar comparand on the collection operator %s', async (_op, where) => {
347+
await expect(engine.find('deal', asFilterArrayQuery(where)))
348+
.rejects.toMatchObject({ status: 400, code: 'INVALID_FILTER' });
349+
// Nothing ran: a refused filter must not reach the driver at all, or the
350+
// 400 would be describing a query that already returned rows.
351+
expect(reads).toHaveLength(0);
352+
});
353+
354+
it('the refusal NAMES the operator, the field and the expected shape (#5346/#5348 wording)', async () => {
355+
const err = await engine.find('deal', asFilterArrayQuery([['stage', 'not_in', 'won']]))
356+
.then(() => null, (e: any) => e);
357+
358+
expect(err).not.toBeNull();
359+
// The entry point that refused, matching the sibling refusals above.
360+
expect(err.message).toMatch(/^find\('deal'\): /);
361+
// The operator, in the lowered spelling…
362+
expect(err.message).toMatch(/Operator "\$nin"/);
363+
// …and in the spellings an author actually types on a ViewFilterRule —
364+
// nobody writes `$nin` into metadata, so a refusal naming only the lowered
365+
// form sends them looking for a key their file does not contain.
366+
expect(err.message).toMatch(/not_in/);
367+
// The member.
368+
expect(err.message).toMatch(/field "stage"/);
369+
// What was received, and where.
370+
expect(err.message).toMatch(/Received string \("won"\)/);
371+
expect(err.message).toMatch(/where\.stage\.\$nin/);
372+
// The expected shape, as a value the caller can paste.
373+
expect(err.message).toMatch(/\["won"\]/);
374+
// The alternative, for the caller who meant a scalar comparison.
375+
expect(err.message).toMatch(/"!=" \(\$ne\)/);
376+
// And the part a status code cannot carry.
377+
expect(err.message).toMatch(/NOT applied/);
378+
expect(err.message).toMatch(/UNFILTERED result set/);
379+
});
380+
381+
it('the whole refusal survives the REST boundary — it fits under CLIENT_MESSAGE_MAX', async () => {
382+
// `rest-server.ts` truncates a declared-4xx message at 500 chars before it
383+
// reaches the client (#5423 made it a truncation rather than a swap). The
384+
// "NOT applied" sentence is the part a caller cannot infer from a status
385+
// code, and it sits at the END — so a message that overflows loses exactly
386+
// the sentence the refusal exists to deliver. Pinned here rather than
387+
// trusted, because the bound lives in another package.
388+
const CLIENT_MESSAGE_MAX = 500;
389+
for (const where of [
390+
[['stage', 'not_in', 'won']],
391+
[['stage', 'in', 'won']],
392+
[['amount', 'between', 5]],
393+
]) {
394+
const err = await engine.find('deal', asFilterArrayQuery(where))
395+
.then(() => null, (e: any) => e);
396+
expect(err.message.length, JSON.stringify(where)).toBeLessThan(CLIENT_MESSAGE_MAX);
397+
expect(err.message, JSON.stringify(where)).toMatch(/UNFILTERED result set/);
398+
}
399+
});
400+
401+
it('refuses through the OBJECT door too — the door #5869 was measured through', async () => {
402+
// The protocol/HTTP face runs its own `isFilterAST` → `parseFilterAST` and
403+
// hands the engine an already-lowered FilterCondition, so the array branch
404+
// above never sees a wire query. This is that shape, arriving as an object.
405+
// NOT erased: `where` is declared `Record< string, unknown >`, so `tsc`
406+
// accepts a malformed comparand. That it type-checks and still has to be
407+
// refused at runtime is exactly why this gate exists.
408+
await expect(engine.find('deal', { where: { stage: { $nin: 'won' } } }))
409+
.rejects.toMatchObject({ status: 400, code: 'INVALID_FILTER' });
410+
await expect(engine.find('deal', { where: { stage: { $in: 'won' } } }))
411+
.rejects.toMatchObject({ status: 400, code: 'INVALID_FILTER' });
412+
});
413+
414+
it.each([
415+
['null', { stage: { $in: null } }],
416+
['a number', { amount: { $in: 10 } }],
417+
['an object', { stage: { $in: { a: 1 } } }],
418+
])('refuses a comparand that is %s — every non-list, not just strings', async (_l, where) => {
419+
await expect(engine.find('deal', { where }))
420+
.rejects.toMatchObject({ status: 400, code: 'INVALID_FILTER' });
421+
});
422+
423+
it('walks into $and / $or / $not — a nested scalar is refused with its own path', async () => {
424+
const err = await engine.find(
425+
'deal',
426+
asFilterArrayQuery(['and', ['amount', '>', 5], ['stage', 'not_in', 'won']]),
427+
).then(() => null, (e: any) => e);
428+
expect(err?.status).toBe(400);
429+
expect(err.message).toMatch(/where\.\$and\[1\]\.stage\.\$nin/);
430+
431+
await expect(engine.find('deal', { where: { $not: { stage: { $in: 'won' } } } }))
432+
.rejects.toMatchObject({ status: 400, code: 'INVALID_FILTER' });
433+
});
434+
435+
it('every engine entry point refuses it, not just find()', async () => {
436+
const where = [['stage', 'not_in', 'won']];
437+
await expect(engine.findOne('deal', asFilterArrayQuery(where)))
438+
.rejects.toMatchObject({ status: 400 });
439+
await expect(engine.count('deal', { where } as unknown as EngineCountOptions))
440+
.rejects.toMatchObject({ status: 400 });
441+
await expect(engine.aggregate('deal', {
442+
where, groupBy: ['stage'], aggregations: [{ function: 'count', field: 'id', alias: 'n' }],
443+
} as unknown as EngineAggregateOptions)).rejects.toMatchObject({ status: 400 });
444+
await expect(engine.update('deal', { amount: 1 }, { where, multi: true } as any))
445+
.rejects.toMatchObject({ status: 400 });
446+
await expect(engine.delete('deal', { where, multi: true } as any))
447+
.rejects.toMatchObject({ status: 400 });
448+
// Refused before any of them touched the store.
449+
expect(reads).toHaveLength(0);
450+
expect(writes).toHaveLength(0);
451+
expect(await engine.count('deal')).toBe(3);
452+
});
453+
454+
// `$between`'s arity, hoisted to the same seam. driver-sql and driver-memory
455+
// each already refuse this (#5328); driver-mongodb's arm falls through
456+
// without emitting a range predicate. Checking here is what makes the three
457+
// agree — the same reason the collection point exists.
458+
it.each([
459+
['a scalar', [['amount', 'between', 5]]],
460+
['a 1-tuple', [['amount', 'between', [1]]]],
461+
['a 3-tuple', [['amount', 'between', [1, 2, 3]]]],
462+
])('refuses a $between comparand that is %s', async (_l, where) => {
463+
await expect(engine.find('deal', asFilterArrayQuery(where)))
464+
.rejects.toMatchObject({ status: 400, code: 'INVALID_FILTER' });
465+
});
466+
467+
it('the $between refusal keeps the platform-wide wording and names the field', async () => {
468+
const err = await engine.find('deal', asFilterArrayQuery([['amount', 'between', 5]]))
469+
.then(() => null, (e: any) => e);
470+
// Verbatim leading sentence from driver-sql / driver-memory: one condition,
471+
// one wording, wherever the caller meets it.
472+
expect(err.message).toMatch(
473+
/Operator "\$between" on field "amount" requires a \[min, max\] value array\./,
474+
);
475+
expect(err.message).toMatch(/where\.amount\.\$between/);
476+
});
477+
478+
// ── what must KEEP working: the declared list shapes ───────────────────
479+
480+
it('a proper list comparand still reaches the driver untouched', async () => {
481+
await engine.find('deal', asFilterArrayQuery([['stage', 'in', ['won', 'lost']]]));
482+
expect(lastWhere()).toEqual({ stage: { $in: ['won', 'lost'] } });
483+
484+
await engine.find('deal', asFilterArrayQuery([['stage', 'not_in', ['lost']]]));
485+
expect(lastWhere()).toEqual({ stage: { $nin: ['lost'] } });
486+
487+
await engine.find('deal', asFilterArrayQuery([['amount', 'between', [5, 25]]]));
488+
expect(lastWhere()).toEqual({ amount: { $between: [5, 25] } });
489+
});
490+
491+
it('an EMPTY list is a declared predicate, not a malformed one', async () => {
492+
// `$in: []` matches nothing and `$nin: []` matches everything — both
493+
// drivers say so in as many words. Arity is not this gate's business.
494+
await engine.find('deal', { where: { stage: { $in: [] } } });
495+
expect(lastWhere()).toEqual({ stage: { $in: [] } });
496+
await engine.find('deal', { where: { stage: { $nin: [] } } });
497+
expect(lastWhere()).toEqual({ stage: { $nin: [] } });
498+
});
499+
500+
it('the gate does not re-judge list MEMBERS — that is #5234, on another face', async () => {
501+
// A `$field` reference and a plain object are both legitimate members here;
502+
// this gate asks only whether the comparand is a list at all.
503+
const where = { stage: { $in: [{ $field: 'other' }, 'won'] } };
504+
await engine.find('deal', { where });
505+
expect(lastWhere()).toEqual(where);
506+
});
507+
508+
it('does not descend into a deep-equality comparand that merely LOOKS like an operator map', async () => {
509+
// `{ $eq: {...} }` holds DATA. A gate that walked into it would refuse a
510+
// stored document whose own key happens to be `$in` — a stricter contract
511+
// than any backend applies.
512+
const where = { stage: { $eq: { $in: 'not-an-operator-here' } } };
513+
await engine.find('deal', { where });
514+
expect(lastWhere()).toEqual(where);
515+
});
516+
517+
it('a scalar on a NON-collection operator is untouched', async () => {
518+
await engine.find('deal', asFilterArrayQuery([['stage', '!=', 'won']]));
519+
expect(lastWhere()).toEqual({ stage: { $ne: 'won' } });
520+
// String bounds on a range comparison stay legal — `FieldOperatorsSchema`
521+
// declares `$gt` as number|Date|FieldReference, but ISO strings are what the
522+
// showcase apps send and every backend accepts. This gate enforces the
523+
// three list declarations, not the whole schema.
524+
await engine.find('deal', asFilterArrayQuery([['stage', '>', '2026-01-01']]));
525+
expect(lastWhere()).toEqual({ stage: { $gt: '2026-01-01' } });
526+
});
527+
307528
// ── the object form is untouched ──────────────────────────────────────
308529

309530
it('a FilterCondition object passes through byte-for-byte', async () => {

packages/objectql/src/engine.ts

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ import { parseAutonumberFormat, renderAutonumber, missingFieldValues, isTenancyD
2121
// [#5158] Door 2's lowering sink — the SAME pair the protocol face (Door 1)
2222
// runs, so `FilterArray` has exactly one lowering in the product.
2323
import { isFilterAST, parseFilterAST, VALID_AST_OPERATORS } from '@objectstack/spec/data';
24+
import { assertListComparandShapes } from './filter-comparand-shape.js';
2425
import {
2526
DATA_MIGRATION_FLAG_OBJECT,
2627
FILE_REFERENCES_MIGRATION_ID,
@@ -451,7 +452,16 @@ function lowerWhereFilterArray<T extends object | undefined>(
451452
): T {
452453
if (!bag) return bag;
453454
const where = (bag as Record<string, unknown>).where;
454-
if (!Array.isArray(where)) return bag;
455+
if (!Array.isArray(where)) {
456+
// [#5869] Door 1 lands HERE, not below: the protocol face runs its own
457+
// `isFilterAST` → `parseFilterAST` and hands the engine an already-lowered
458+
// `FilterCondition` object, so a gate on the array branch alone would miss
459+
// every query that arrived over the wire. The comparand check is the same
460+
// one either way — it reads the lowered condition, which is what both doors
461+
// produce.
462+
assertListComparandShapes(object, operation, where);
463+
return bag;
464+
}
455465

456466
const lowered: Record<string, unknown> = { ...bag };
457467

@@ -488,6 +498,11 @@ function lowerWhereFilterArray<T extends object | undefined>(
488498
`unfiltered (#5158).`,
489499
);
490500
}
501+
// [#5869] Door 2's half of the same check. `isFilterAST` vouched for the
502+
// OPERATOR and `parseFilterAST` lowered it, but neither looks at the
503+
// comparand — `['status', 'not_in', 'done']` lowers to `{status: {$nin:
504+
// 'done'}}` and a scalar `$nin` is what reached the driver as a 500.
505+
assertListComparandShapes(object, operation, condition);
491506
lowered.where = condition;
492507
return lowered as T;
493508
}

0 commit comments

Comments
 (0)