fix(data-objectstack,core): an object filter no longer depends on whether the query expands a lookup

.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.

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 } });

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]);
+  });
+});

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<string, any>): 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<string, any>): 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. ` +

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());
 

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<string, unknown>).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<string, unknown>);
   }
 
   return undefined;