From 5a461132e063fc8a5abab0583b324880cf93ad52 Mon Sep 17 00:00:00 2001 From: Jack Zhuang <277994282+os-zhuang@users.noreply.github.com> Date: Fri, 31 Jul 2026 09:20:56 +0800 Subject: [PATCH] fix(data-objectstack,core): an object filter no longer depends on whether the query expands a lookup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #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 --- .changeset/object-filters-take-one-route.md | 38 +++++++++++++ .../utils/__tests__/filter-converter.test.ts | 44 +++++++++++---- .../__tests__/filter-source-merge.test.ts | 38 ++++++++++++- packages/core/src/utils/filter-converter.ts | 41 +++++++++++--- .../src/filter-entry-translation.test.ts | 54 +++++++++++++++++++ packages/data-objectstack/src/index.ts | 9 +++- 6 files changed, 203 insertions(+), 21 deletions(-) create mode 100644 .changeset/object-filters-take-one-route.md diff --git a/.changeset/object-filters-take-one-route.md b/.changeset/object-filters-take-one-route.md new file mode 100644 index 0000000000..4d72368606 --- /dev/null +++ b/.changeset/object-filters-take-one-route.md @@ -0,0 +1,38 @@ +--- +"@object-ui/core": patch +"@object-ui/data-objectstack": patch +--- + +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 — `{$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`.** `$regex: 'a.c'` matches + "abc"; `contains 'a.c'` matches only those three literal characters. That is 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 now refused, the same treatment the neighbouring + unknown operator already got. Nothing in the repo depended on the conversion. + +Both refusals now throw `FilterOperatorError`, carrying `code: 'INVALID_FILTER'` +/ `httpStatus: 400`. The pre-existing unknown-operator throw was a bare `Error`, +which `classifyLoadError` classifies as a network fault — so a malformed filter +told the user to check their connection (#3066), the one thing it definitely +was not. diff --git a/packages/core/src/utils/__tests__/filter-converter.test.ts b/packages/core/src/utils/__tests__/filter-converter.test.ts index ddd4582bee..f66cedef9d 100644 --- a/packages/core/src/utils/__tests__/filter-converter.test.ts +++ b/packages/core/src/utils/__tests__/filter-converter.test.ts @@ -74,21 +74,43 @@ describe('Filter Converter Utilities', () => { expect(result).toEqual(['status', 'nin', ['archived']]); }); - it('should warn on $regex operator and convert to contains', () => { + /** + * This used to assert the opposite — `$regex` converted to `contains` + * behind a `console.warn`. The example it used is the argument against it: + * `$regex: '^John'` means "starts with John", while `contains '^John'` + * looks for a literal caret, so "John Smith" does NOT match. The rewrite + * answered a different question and returned a result that looks fine. + * + * A `console.warn` is not an error channel in a deployed app, the spec has + * no `$regex` to translate into (`FILTER_OPERATORS`, data/filter.zod.ts), + * and the neighbouring unknown-operator case already throws for exactly + * this reason. So it is refused. + */ + it('should refuse $regex rather than answer with a substring match', () => { const consoleSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}); - - const result = convertFiltersToAST({ - name: { $regex: '^John' } - }); - - expect(result).toEqual(['name', 'contains', '^John']); - expect(consoleSpy).toHaveBeenCalledWith( - expect.stringContaining('[ObjectUI] Warning: $regex operator is not fully supported') - ); - + + expect(() => convertFiltersToAST({ name: { $regex: '^John' } })) + .toThrow(/\$regex/); + // Refused loudly, not warned about quietly. + expect(consoleSpy).not.toHaveBeenCalled(); + consoleSpy.mockRestore(); }); + it('should tag both refusals as a rejected request, not a network fault', () => { + // A bare `Error` reads as "check your connection" in the list error panel + // (#3066) — the one thing a malformed filter definitely is not. + for (const bad of [{ n: { $regex: 'x' } }, { n: { $unknown: 1 } }]) { + expect(() => convertFiltersToAST(bad as any)).toThrow(); + try { + convertFiltersToAST(bad as any); + } catch (e: any) { + expect(e.code).toBe('INVALID_FILTER'); + expect(e.httpStatus).toBe(400); + } + } + }); + it('should throw error on unknown operator', () => { expect(() => { convertFiltersToAST({ age: { $unknown: 18 } }); diff --git a/packages/core/src/utils/__tests__/filter-source-merge.test.ts b/packages/core/src/utils/__tests__/filter-source-merge.test.ts index 2bb856971a..c20403d951 100644 --- a/packages/core/src/utils/__tests__/filter-source-merge.test.ts +++ b/packages/core/src/utils/__tests__/filter-source-merge.test.ts @@ -29,7 +29,7 @@ import { describe, it, expect } from 'vitest'; import { isFilterAST, parseFilterAST } from '@objectstack/spec/data'; -import { toFilterNode, mergeFilterNodes } from '../filter-converter'; +import { toFilterNode, mergeFilterNodes, convertFiltersToAST, FilterOperatorError } from '../filter-converter'; const RULES = [{ field: 'stage', operator: 'eq', value: 'won' }]; const TUPLE = ['owner', '=', 'me']; @@ -102,3 +102,39 @@ describe('what reaches the server', () => { }); }); }); + +describe('operators this layer will not translate', () => { + it('refuses $regex instead of silently answering with `contains`', () => { + // `$regex: 'a.c'` matches "abc"; `contains 'a.c'` matches only the literal + // three characters. Neither result looks wrong on screen, which is exactly + // why the old `console.warn` + rewrite was the wrong shape of handling — + // a warning is not an error channel in a deployed app. + expect(() => convertFiltersToAST({ name: { $regex: 'a.c' } })) + .toThrow(/regex/i); + }); + + it('refuses an unknown operator', () => { + expect(() => convertFiltersToAST({ name: { $bogus: 1 } })).toThrow(/\$bogus/); + }); + + it('carries the code that classifies it as a rejected request, not a network fault', () => { + // A bare Error reads as "check your connection" in the list error panel + // (#3066). Both refusals above are the server-understood-and-refused kind. + for (const bad of [{ n: { $regex: 'x' } }, { n: { $bogus: 1 } }]) { + try { + convertFiltersToAST(bad as any); + throw new Error('expected a refusal'); + } catch (e: any) { + expect(e).toBeInstanceOf(FilterOperatorError); + expect(e.code).toBe('INVALID_FILTER'); + expect(e.httpStatus).toBe(400); + } + } + }); + + it('still translates every operator it does support', () => { + expect(convertFiltersToAST({ a: { $gte: 1 } })).toEqual(['a', '>=', 1]); + expect(convertFiltersToAST({ a: { $contains: 'x' } })).toEqual(['a', 'contains', 'x']); + expect(convertFiltersToAST({ a: { $null: true } })).toEqual(['a', 'is_null', true]); + }); +}); diff --git a/packages/core/src/utils/filter-converter.ts b/packages/core/src/utils/filter-converter.ts index 09f6a28468..8f1352a427 100644 --- a/packages/core/src/utils/filter-converter.ts +++ b/packages/core/src/utils/filter-converter.ts @@ -34,6 +34,23 @@ export type FilterNode = * @param operator - MongoDB-style operator (e.g., '$gte', '$in') * @returns ObjectStack operator or null if not recognized */ +/** + * A filter operator this layer will not translate. + * + * Carries the data API's own code and status for the same refusal so a failed + * list renders "the filter is malformed" rather than "check your connection" — + * `classifyLoadError` reads these, and a bare `Error` classifies as a network + * fault, which is the one thing this is definitely not. + */ +export class FilterOperatorError extends Error { + readonly code = 'INVALID_FILTER'; + readonly httpStatus = 400; + constructor(message: string) { + super(message); + this.name = 'FilterOperatorError'; + } +} + export function convertOperatorToAST(operator: string): string | null { // Spec reference: framework/packages/spec/src/data/filter.zod.ts // Canonical MongoDB-style keys are camelCase ($startsWith, $endsWith, $notContains). @@ -95,16 +112,24 @@ export function convertFiltersToAST(filter: Record): FilterNode | R if (typeof value === 'object' && !Array.isArray(value)) { // Handle operator-based filters for (const [operator, operatorValue] of Object.entries(value)) { - // Special handling for $regex - warn users about limited support + // `$regex` is refused, not downgraded. It used to become `contains` + // behind a `console.warn` — but substring matching is a DIFFERENT + // QUESTION, not a weaker version of the same one: `$regex: 'a.c'` + // matches "abc", `contains 'a.c'` does not, and neither result looks + // wrong on screen. A warning is not an error channel; nobody reads the + // console of a deployed app. + // + // The spec has no `$regex` (`FILTER_OPERATORS`, data/filter.zod.ts), so + // there is nothing to translate it INTO. Same treatment the unknown + // operator below already gets, and for the same stated reason. if (operator === '$regex') { - console.warn( - `[ObjectUI] Warning: $regex operator is not fully supported. ` + - `Converting to 'contains' which only supports substring matching, not regex patterns. ` + + throw new FilterOperatorError( + `[ObjectUI] The '$regex' filter operator is not supported. It used to be ` + + `converted to 'contains', which matches a literal substring rather than a ` + + `pattern — a different result, not a degraded one. ` + `Field: '${field}', Value: ${JSON.stringify(operatorValue)}. ` + - `Consider using $contains or $startsWith instead.` + `Use $contains, $startsWith or $endsWith.` ); - conditions.push([field, 'contains', operatorValue]); - continue; } // $null / $exists translate based on their boolean value (per spec semantics). @@ -125,7 +150,7 @@ export function convertFiltersToAST(filter: Record): FilterNode | R conditions.push([field, astOperator, operatorValue]); } else { // Unknown operator - throw error to avoid silent failure - throw new Error( + throw new FilterOperatorError( `[ObjectUI] Unknown filter operator '${operator}' for field '${field}'. ` + `Supported operators: $eq, $ne, $gt, $gte, $lt, $lte, $in, $nin, $between, ` + `$contains, $notContains, $startsWith, $endsWith, $null, $exists. ` + diff --git a/packages/data-objectstack/src/filter-entry-translation.test.ts b/packages/data-objectstack/src/filter-entry-translation.test.ts index d0a3eabea1..cd43be4a80 100644 --- a/packages/data-objectstack/src/filter-entry-translation.test.ts +++ b/packages/data-objectstack/src/filter-entry-translation.test.ts @@ -283,6 +283,60 @@ describe('a bare rule object directly under a logical node', () => { ); }); +describe('an OBJECT filter reaches the same predicate on both routes', () => { + beforeEach(() => clearSharedDiscoveryCache()); + + /** + * The array branch was single-sourced in #3072; the object branch was not. + * `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 expanded a lookup. + * + * Measured across 21 operator shapes, four diverged. `$exists` vs `$null` was + * a cosmetic difference the server treats identically, but the other three + * were not: see below. + */ + bothRoutes( + 'converts a plain equality object', + { status: 'active' }, + (wire) => expect(wire).toEqual(['status', '=', 'active']), + ); + + bothRoutes( + 'converts an operator object', + { age: { $gte: 18 } }, + (wire) => expect(wire).toEqual(['age', '>=', 18]), + ); + + bothRoutes( + 'leaves a top-level Mongo logical node alone', + // `mergeFilters` (dashboard scope filters) produces this. It survives as a + // `['$and', '=', [...]]` comparison that `parseFilterAST` reads back as a + // real `$and` — verified, and the reason this is NOT rewritten here. + { $and: [{ a: 1 }, { b: 2 }] }, + (wire) => expect(wire).toEqual(['$and', '=', [{ a: 1 }, { b: 2 }]]), + ); + + for (const route of ['plain', 'expand'] as const) { + it(`refuses an unknown operator on the ${route} route`, async () => { + // The guard exists "to avoid silent failure" — it used to run on one + // route only, so a typo shipped silently whenever a lookup was expanded. + const err = await findRejects({ s: { $bogus: 1 } }, route); + expect(err).toBeInstanceOf(Error); + expect(String((err as Error).message)).toContain('$bogus'); + expect(isMalformedFilterError(err)).toBe(true); + }); + + it(`refuses $regex rather than answering a different question (${route})`, async () => { + // Was silently rewritten to `contains`: `$regex: 'a.c'` matches "abc", + // `contains 'a.c'` does not. The spec has no `$regex` to translate into. + const err = await findRejects({ s: { $regex: 'a.c' } }, route); + expect(isMalformedFilterError(err)).toBe(true); + expect(String((err as Error).message)).toMatch(/regex/i); + }); + } +}); + describe('the two routes agree on shapes that are neither form', () => { beforeEach(() => clearSharedDiscoveryCache()); diff --git a/packages/data-objectstack/src/index.ts b/packages/data-objectstack/src/index.ts index 72e26455a5..d267b11852 100644 --- a/packages/data-objectstack/src/index.ts +++ b/packages/data-objectstack/src/index.ts @@ -287,7 +287,14 @@ function translateFilterToAST(filter: unknown): unknown | undefined { if (typeof filter === 'object') { if (Object.keys(filter as Record).length === 0) return undefined; - return filter; + // Same conversion `convertQueryParams` applies. This branch used to return + // the object VERBATIM, so the two `find()` routes disagreed about the same + // filter — decided, as ever, by whether the query happened to expand a + // lookup. Measured across 21 operator shapes, four diverged; the one that + // mattered is that `convertFiltersToAST`'s unknown-operator guard — added + // expressly "to avoid silent failure" — never ran on this route, so a typo'd + // operator threw on a plain read and shipped silently on an expanded one. + return convertFiltersToAST(filter as Record); } return undefined;