Skip to content

Commit 5a46113

Browse files
os-zhuangclaude
andcommitted
fix(data-objectstack,core): an object filter no longer depends on whether the query expands a lookup
#3072 single-sourced the ARRAY branch of the adapter's two `find()` routes. The object branch was left as it was: `convertQueryParams` converted a MongoDB-style filter to AST while `translateFilterToAST` returned it verbatim — so the same `$filter` went out in two formats, decided by whether the query happened to expand a lookup. Measured across 21 operator shapes, four diverged. Most of the gap turned out to be harmless, which is worth recording because it was not obvious: `{$and: […]}` survives the plain route as a `['$and','=',[…]]` comparison that `parseFilterAST` reads back as a real `$and`, and `$exists` vs `$null` is a difference the server treats identically. Two were not harmless: - THE UNKNOWN-OPERATOR GUARD ONLY RAN ON ONE ROUTE. `convertFiltersToAST` throws on an unrecognised operator, with a comment saying it does so "to avoid silent failure" — but the expanded route never called it, so a typo'd operator threw on a plain read and shipped silently whenever a lookup was expanded. - `$regex` WAS SILENTLY REWRITTEN TO `contains`. The existing test's own example makes the case: `$regex: '^John'` means "starts with John", while `contains '^John'` looks for a literal caret — so "John Smith" does not match. A different question, not a weaker version of the same one, and neither result looks wrong on screen. The rewrite sat behind a `console.warn`, which is not an error channel in a deployed app, and the function's own unknown-operator message never listed `$regex` among the supported set. The spec has no `$regex` (`FILTER_OPERATORS`, data/filter.zod.ts), so there is nothing to translate it into: it is refused now, the same treatment the neighbouring unknown operator already got. Nothing in the repo depended on the conversion. Both refusals throw `FilterOperatorError` carrying `code: 'INVALID_FILTER'` / `httpStatus: 400`. The pre-existing unknown-operator throw was a bare `Error`, which `classifyLoadError` reads as a network fault — so a malformed filter told the user to check their connection (#3066), the one thing it was not. `filter-converter.test.ts`'s `$regex` case asserted the old behaviour and is rewritten to assert the refusal, keeping the `'^John'` example because it demonstrates the harm better than any prose. Verification: 9 new/changed tests; reverting the two source files fails 9 of them. Full suite 762 files / 8875 tests green; tsc clean; eslint 0 errors. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 parent d132bb5 commit 5a46113

6 files changed

Lines changed: 203 additions & 21 deletions

File tree

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,38 @@
1+
---
2+
"@object-ui/core": patch
3+
"@object-ui/data-objectstack": patch
4+
---
5+
6+
fix(data-objectstack,core): an object filter no longer depends on whether the query expands a lookup
7+
8+
#3072 single-sourced the ARRAY branch of the adapter's two `find()` routes. The
9+
object branch was left as it was: `convertQueryParams` converted a MongoDB-style
10+
filter to AST while `translateFilterToAST` returned it verbatim — so the same
11+
`$filter` went out in two formats, decided by whether the query happened to
12+
expand a lookup.
13+
14+
Measured across 21 operator shapes, **four diverged**. Most of the gap turned
15+
out to be harmless — `{$and: […]}` survives the plain route as a
16+
`['$and','=',[…]]` comparison that `parseFilterAST` reads back as a real `$and`,
17+
and `$exists` vs `$null` is a difference the server treats identically. Two were
18+
not harmless:
19+
20+
- **The unknown-operator guard only ran on one route.** `convertFiltersToAST`
21+
throws on an unrecognised operator, with a comment saying it does so "to avoid
22+
silent failure" — but the expanded route never called it, so a typo'd operator
23+
threw on a plain read and shipped silently whenever a lookup was expanded.
24+
- **`$regex` was silently rewritten to `contains`.** `$regex: 'a.c'` matches
25+
"abc"; `contains 'a.c'` matches only those three literal characters. That is a
26+
*different question*, not a weaker version of the same one, and neither result
27+
looks wrong on screen. The rewrite sat behind a `console.warn`, which is not
28+
an error channel in a deployed app — and the function's own unknown-operator
29+
message never listed `$regex` among the supported set. The spec has no
30+
`$regex` (`FILTER_OPERATORS`, `data/filter.zod.ts`), so there is nothing to
31+
translate it into: it is now refused, the same treatment the neighbouring
32+
unknown operator already got. Nothing in the repo depended on the conversion.
33+
34+
Both refusals now throw `FilterOperatorError`, carrying `code: 'INVALID_FILTER'`
35+
/ `httpStatus: 400`. The pre-existing unknown-operator throw was a bare `Error`,
36+
which `classifyLoadError` classifies as a network fault — so a malformed filter
37+
told the user to check their connection (#3066), the one thing it definitely
38+
was not.

packages/core/src/utils/__tests__/filter-converter.test.ts

Lines changed: 33 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -74,21 +74,43 @@ describe('Filter Converter Utilities', () => {
7474
expect(result).toEqual(['status', 'nin', ['archived']]);
7575
});
7676

77-
it('should warn on $regex operator and convert to contains', () => {
77+
/**
78+
* This used to assert the opposite — `$regex` converted to `contains`
79+
* behind a `console.warn`. The example it used is the argument against it:
80+
* `$regex: '^John'` means "starts with John", while `contains '^John'`
81+
* looks for a literal caret, so "John Smith" does NOT match. The rewrite
82+
* answered a different question and returned a result that looks fine.
83+
*
84+
* A `console.warn` is not an error channel in a deployed app, the spec has
85+
* no `$regex` to translate into (`FILTER_OPERATORS`, data/filter.zod.ts),
86+
* and the neighbouring unknown-operator case already throws for exactly
87+
* this reason. So it is refused.
88+
*/
89+
it('should refuse $regex rather than answer with a substring match', () => {
7890
const consoleSpy = vi.spyOn(console, 'warn').mockImplementation(() => {});
79-
80-
const result = convertFiltersToAST({
81-
name: { $regex: '^John' }
82-
});
83-
84-
expect(result).toEqual(['name', 'contains', '^John']);
85-
expect(consoleSpy).toHaveBeenCalledWith(
86-
expect.stringContaining('[ObjectUI] Warning: $regex operator is not fully supported')
87-
);
88-
91+
92+
expect(() => convertFiltersToAST({ name: { $regex: '^John' } }))
93+
.toThrow(/\$regex/);
94+
// Refused loudly, not warned about quietly.
95+
expect(consoleSpy).not.toHaveBeenCalled();
96+
8997
consoleSpy.mockRestore();
9098
});
9199

100+
it('should tag both refusals as a rejected request, not a network fault', () => {
101+
// A bare `Error` reads as "check your connection" in the list error panel
102+
// (#3066) — the one thing a malformed filter definitely is not.
103+
for (const bad of [{ n: { $regex: 'x' } }, { n: { $unknown: 1 } }]) {
104+
expect(() => convertFiltersToAST(bad as any)).toThrow();
105+
try {
106+
convertFiltersToAST(bad as any);
107+
} catch (e: any) {
108+
expect(e.code).toBe('INVALID_FILTER');
109+
expect(e.httpStatus).toBe(400);
110+
}
111+
}
112+
});
113+
92114
it('should throw error on unknown operator', () => {
93115
expect(() => {
94116
convertFiltersToAST({ age: { $unknown: 18 } });

packages/core/src/utils/__tests__/filter-source-merge.test.ts

Lines changed: 37 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@
2929

3030
import { describe, it, expect } from 'vitest';
3131
import { isFilterAST, parseFilterAST } from '@objectstack/spec/data';
32-
import { toFilterNode, mergeFilterNodes } from '../filter-converter';
32+
import { toFilterNode, mergeFilterNodes, convertFiltersToAST, FilterOperatorError } from '../filter-converter';
3333

3434
const RULES = [{ field: 'stage', operator: 'eq', value: 'won' }];
3535
const TUPLE = ['owner', '=', 'me'];
@@ -102,3 +102,39 @@ describe('what reaches the server', () => {
102102
});
103103
});
104104
});
105+
106+
describe('operators this layer will not translate', () => {
107+
it('refuses $regex instead of silently answering with `contains`', () => {
108+
// `$regex: 'a.c'` matches "abc"; `contains 'a.c'` matches only the literal
109+
// three characters. Neither result looks wrong on screen, which is exactly
110+
// why the old `console.warn` + rewrite was the wrong shape of handling —
111+
// a warning is not an error channel in a deployed app.
112+
expect(() => convertFiltersToAST({ name: { $regex: 'a.c' } }))
113+
.toThrow(/regex/i);
114+
});
115+
116+
it('refuses an unknown operator', () => {
117+
expect(() => convertFiltersToAST({ name: { $bogus: 1 } })).toThrow(/\$bogus/);
118+
});
119+
120+
it('carries the code that classifies it as a rejected request, not a network fault', () => {
121+
// A bare Error reads as "check your connection" in the list error panel
122+
// (#3066). Both refusals above are the server-understood-and-refused kind.
123+
for (const bad of [{ n: { $regex: 'x' } }, { n: { $bogus: 1 } }]) {
124+
try {
125+
convertFiltersToAST(bad as any);
126+
throw new Error('expected a refusal');
127+
} catch (e: any) {
128+
expect(e).toBeInstanceOf(FilterOperatorError);
129+
expect(e.code).toBe('INVALID_FILTER');
130+
expect(e.httpStatus).toBe(400);
131+
}
132+
}
133+
});
134+
135+
it('still translates every operator it does support', () => {
136+
expect(convertFiltersToAST({ a: { $gte: 1 } })).toEqual(['a', '>=', 1]);
137+
expect(convertFiltersToAST({ a: { $contains: 'x' } })).toEqual(['a', 'contains', 'x']);
138+
expect(convertFiltersToAST({ a: { $null: true } })).toEqual(['a', 'is_null', true]);
139+
});
140+
});

packages/core/src/utils/filter-converter.ts

Lines changed: 33 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,23 @@ export type FilterNode =
3434
* @param operator - MongoDB-style operator (e.g., '$gte', '$in')
3535
* @returns ObjectStack operator or null if not recognized
3636
*/
37+
/**
38+
* A filter operator this layer will not translate.
39+
*
40+
* Carries the data API's own code and status for the same refusal so a failed
41+
* list renders "the filter is malformed" rather than "check your connection" —
42+
* `classifyLoadError` reads these, and a bare `Error` classifies as a network
43+
* fault, which is the one thing this is definitely not.
44+
*/
45+
export class FilterOperatorError extends Error {
46+
readonly code = 'INVALID_FILTER';
47+
readonly httpStatus = 400;
48+
constructor(message: string) {
49+
super(message);
50+
this.name = 'FilterOperatorError';
51+
}
52+
}
53+
3754
export function convertOperatorToAST(operator: string): string | null {
3855
// Spec reference: framework/packages/spec/src/data/filter.zod.ts
3956
// Canonical MongoDB-style keys are camelCase ($startsWith, $endsWith, $notContains).
@@ -95,16 +112,24 @@ export function convertFiltersToAST(filter: Record<string, any>): FilterNode | R
95112
if (typeof value === 'object' && !Array.isArray(value)) {
96113
// Handle operator-based filters
97114
for (const [operator, operatorValue] of Object.entries(value)) {
98-
// Special handling for $regex - warn users about limited support
115+
// `$regex` is refused, not downgraded. It used to become `contains`
116+
// behind a `console.warn` — but substring matching is a DIFFERENT
117+
// QUESTION, not a weaker version of the same one: `$regex: 'a.c'`
118+
// matches "abc", `contains 'a.c'` does not, and neither result looks
119+
// wrong on screen. A warning is not an error channel; nobody reads the
120+
// console of a deployed app.
121+
//
122+
// The spec has no `$regex` (`FILTER_OPERATORS`, data/filter.zod.ts), so
123+
// there is nothing to translate it INTO. Same treatment the unknown
124+
// operator below already gets, and for the same stated reason.
99125
if (operator === '$regex') {
100-
console.warn(
101-
`[ObjectUI] Warning: $regex operator is not fully supported. ` +
102-
`Converting to 'contains' which only supports substring matching, not regex patterns. ` +
126+
throw new FilterOperatorError(
127+
`[ObjectUI] The '$regex' filter operator is not supported. It used to be ` +
128+
`converted to 'contains', which matches a literal substring rather than a ` +
129+
`pattern — a different result, not a degraded one. ` +
103130
`Field: '${field}', Value: ${JSON.stringify(operatorValue)}. ` +
104-
`Consider using $contains or $startsWith instead.`
131+
`Use $contains, $startsWith or $endsWith.`
105132
);
106-
conditions.push([field, 'contains', operatorValue]);
107-
continue;
108133
}
109134

110135
// $null / $exists translate based on their boolean value (per spec semantics).
@@ -125,7 +150,7 @@ export function convertFiltersToAST(filter: Record<string, any>): FilterNode | R
125150
conditions.push([field, astOperator, operatorValue]);
126151
} else {
127152
// Unknown operator - throw error to avoid silent failure
128-
throw new Error(
153+
throw new FilterOperatorError(
129154
`[ObjectUI] Unknown filter operator '${operator}' for field '${field}'. ` +
130155
`Supported operators: $eq, $ne, $gt, $gte, $lt, $lte, $in, $nin, $between, ` +
131156
`$contains, $notContains, $startsWith, $endsWith, $null, $exists. ` +

packages/data-objectstack/src/filter-entry-translation.test.ts

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -283,6 +283,60 @@ describe('a bare rule object directly under a logical node', () => {
283283
);
284284
});
285285

286+
describe('an OBJECT filter reaches the same predicate on both routes', () => {
287+
beforeEach(() => clearSharedDiscoveryCache());
288+
289+
/**
290+
* The array branch was single-sourced in #3072; the object branch was not.
291+
* `convertQueryParams` converted a MongoDB-style filter to AST while
292+
* `translateFilterToAST` returned it verbatim — so the same `$filter` went out
293+
* in two formats, decided by whether the query expanded a lookup.
294+
*
295+
* Measured across 21 operator shapes, four diverged. `$exists` vs `$null` was
296+
* a cosmetic difference the server treats identically, but the other three
297+
* were not: see below.
298+
*/
299+
bothRoutes(
300+
'converts a plain equality object',
301+
{ status: 'active' },
302+
(wire) => expect(wire).toEqual(['status', '=', 'active']),
303+
);
304+
305+
bothRoutes(
306+
'converts an operator object',
307+
{ age: { $gte: 18 } },
308+
(wire) => expect(wire).toEqual(['age', '>=', 18]),
309+
);
310+
311+
bothRoutes(
312+
'leaves a top-level Mongo logical node alone',
313+
// `mergeFilters` (dashboard scope filters) produces this. It survives as a
314+
// `['$and', '=', [...]]` comparison that `parseFilterAST` reads back as a
315+
// real `$and` — verified, and the reason this is NOT rewritten here.
316+
{ $and: [{ a: 1 }, { b: 2 }] },
317+
(wire) => expect(wire).toEqual(['$and', '=', [{ a: 1 }, { b: 2 }]]),
318+
);
319+
320+
for (const route of ['plain', 'expand'] as const) {
321+
it(`refuses an unknown operator on the ${route} route`, async () => {
322+
// The guard exists "to avoid silent failure" — it used to run on one
323+
// route only, so a typo shipped silently whenever a lookup was expanded.
324+
const err = await findRejects({ s: { $bogus: 1 } }, route);
325+
expect(err).toBeInstanceOf(Error);
326+
expect(String((err as Error).message)).toContain('$bogus');
327+
expect(isMalformedFilterError(err)).toBe(true);
328+
});
329+
330+
it(`refuses $regex rather than answering a different question (${route})`, async () => {
331+
// Was silently rewritten to `contains`: `$regex: 'a.c'` matches "abc",
332+
// `contains 'a.c'` does not. The spec has no `$regex` to translate into.
333+
const err = await findRejects({ s: { $regex: 'a.c' } }, route);
334+
expect(isMalformedFilterError(err)).toBe(true);
335+
expect(String((err as Error).message)).toMatch(/regex/i);
336+
});
337+
}
338+
});
339+
286340
describe('the two routes agree on shapes that are neither form', () => {
287341
beforeEach(() => clearSharedDiscoveryCache());
288342

packages/data-objectstack/src/index.ts

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -287,7 +287,14 @@ function translateFilterToAST(filter: unknown): unknown | undefined {
287287

288288
if (typeof filter === 'object') {
289289
if (Object.keys(filter as Record<string, unknown>).length === 0) return undefined;
290-
return filter;
290+
// Same conversion `convertQueryParams` applies. This branch used to return
291+
// the object VERBATIM, so the two `find()` routes disagreed about the same
292+
// filter — decided, as ever, by whether the query happened to expand a
293+
// lookup. Measured across 21 operator shapes, four diverged; the one that
294+
// mattered is that `convertFiltersToAST`'s unknown-operator guard — added
295+
// expressly "to avoid silent failure" — never ran on this route, so a typo'd
296+
// operator threw on a plain read and shipped silently on an expanded one.
297+
return convertFiltersToAST(filter as Record<string, unknown>);
291298
}
292299

293300
return undefined;

0 commit comments

Comments
 (0)