From 2f91c09d3a00c9af0e002c8d1d69700f0b658fb7 Mon Sep 17 00:00:00 2001 From: Jack Zhuang <277994282+os-zhuang@users.noreply.github.com> Date: Thu, 30 Jul 2026 10:45:40 +0800 Subject: [PATCH] fix(list,data): bridge every spec view operator onto the filter AST (#2901) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A stored view filter using `before` or `after` came back **unfiltered**. Not an error — every row, silently. `before`/`after` are canonical members of the spec's `VIEW_FILTER_OPERATORS` (`ui/view.zod.ts`), so a view legitimately carries them. They are absent from `VALID_AST_OPERATORS` (`data/filter.zod.ts`), which gates `isFilterAST()`. Neither translation table had an entry, so they reached the wire verbatim, the server's gate rejected the shape, the protocol passed the array through unconverted, and driver-sql then skipped it entirely — no WHERE clause emitted. See objectstack#3948 for the server-side hardening; this is the client half. The trigger is the single-condition case: `toSpecFilter` emits a bare triple for one AND condition, which is the shape that vanishes. Two or more conditions produce a nested array, which reaches the driver's wider switch and throws. Also fixes `mapOperator` emitting `'not in'` with a space — in no spec vocabulary. Arrays never reached the wire (`normalizeFilterCondition` expands them), but a non-array value escaped as an unfiltered query. The new parity guard caught eight more on its first run — `notequals`, `greaterthan`, `lessthan`, `greaterorequal`, `greaterThanOrEqual`, `lessorequal`, `lessThanOrEqual`, `notin`. All are spellings the spec's `VIEW_FILTER_OPERATOR_ALIASES` still folds, and all are live in stored metadata because `saveMeta` persists the authored body verbatim, so the spec's own `z.preprocess` normalization never reaches the row. Rather than enumerate them, `mapOperator` now matches case- and underscore-insensitively, which collapses the class instead of the instances. Guards assert both tables land inside `VALID_AST_OPERATORS` and that every canonical `VIEW_FILTER_OPERATORS` member survives `isFilterAST()` — so the next operator the spec adds fails a test instead of returning unfiltered rows. Requires `@objectstack/spec` as a devDependency in both packages; safe now that all importers resolve to one spec version. Refs #2901, objectstack#3948 Co-Authored-By: Claude --- packages/data-objectstack/package.json | 1 + .../src/filter-operator-ast-parity.test.ts | 71 ++++++++++++++ packages/data-objectstack/src/index.ts | 18 +++- packages/plugin-list/package.json | 1 + packages/plugin-list/src/ListView.tsx | 57 ++++++++--- .../filter-operator-ast-parity.test.ts | 98 +++++++++++++++++++ pnpm-lock.yaml | 6 ++ 7 files changed, 239 insertions(+), 13 deletions(-) create mode 100644 packages/data-objectstack/src/filter-operator-ast-parity.test.ts create mode 100644 packages/plugin-list/src/__tests__/filter-operator-ast-parity.test.ts diff --git a/packages/data-objectstack/package.json b/packages/data-objectstack/package.json index 4503a6533b..c232b8cfe1 100644 --- a/packages/data-objectstack/package.json +++ b/packages/data-objectstack/package.json @@ -35,6 +35,7 @@ "@objectstack/client": "^17.0.0-rc.0" }, "devDependencies": { + "@objectstack/spec": "^17.0.0-rc.0", "tsup": "^8.5.1", "typescript": "^6.0.3", "vitest": "^4.1.10" diff --git a/packages/data-objectstack/src/filter-operator-ast-parity.test.ts b/packages/data-objectstack/src/filter-operator-ast-parity.test.ts new file mode 100644 index 0000000000..e0c8441853 --- /dev/null +++ b/packages/data-objectstack/src/filter-operator-ast-parity.test.ts @@ -0,0 +1,71 @@ +/** + * ObjectUI + * Copyright (c) 2024-present ObjectStack Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +/** + * Adapter operator table → filter-AST parity (#2901, objectstack#3948). + * + * `FILTER_OPERATOR_ALIASES` is the last translation a filter passes through + * before it goes on the wire, and `normalizeFilterOperator` ends in `?? op` — + * an unmapped operator is emitted verbatim. The server then rejects the shape + * at `isFilterAST()`, passes the array through unconverted, and driver-sql + * skips it entirely: **no WHERE clause, no error, every row returned.** + * + * So a missing row in this table is not a validation failure, it is an + * unfiltered query. `before`/`after` — canonical members of the spec's + * `VIEW_FILTER_OPERATORS` — were missing, which is exactly how a stored + * "close_date before X" view came back unfiltered. + * + * These tests pin the table against the spec vocabularies in both directions. + */ +import { describe, it, expect } from 'vitest'; +import { VALID_AST_OPERATORS } from '@objectstack/spec/data'; +import { VIEW_FILTER_OPERATORS } from '@objectstack/spec/ui'; +import { FILTER_OPERATOR_ALIASES } from './index'; + +/** + * View operators this adapter is not the bridge for — the value-shape ones the + * view layer resolves to a null comparison before an operator is ever emitted. + */ +const NOT_THIS_ADAPTERS_JOB = new Set(['is_empty', 'is_not_empty']); + +describe('FILTER_OPERATOR_ALIASES lands inside the spec AST vocabulary', () => { + it('reads both vocabularies from the spec', () => { + expect(VIEW_FILTER_OPERATORS.length).toBeGreaterThan(0); + expect(VALID_AST_OPERATORS.size).toBeGreaterThan(0); + }); + + it('every alias target is an operator the AST gate accepts', () => { + const bad = Object.entries(FILTER_OPERATOR_ALIASES) + .filter(([, target]) => !VALID_AST_OPERATORS.has(String(target).toLowerCase())) + .map(([alias, target]) => `${alias} → ${target}`); + expect( + bad, + 'these aliases translate to operators VALID_AST_OPERATORS rejects, so the ' + + 'server drops the filter silently instead of erroring', + ).toEqual([]); + }); + + it('covers every canonical view operator the spec defines', () => { + const uncovered = VIEW_FILTER_OPERATORS + .filter((op) => !NOT_THIS_ADAPTERS_JOB.has(op)) + .filter((op) => { + const target = FILTER_OPERATOR_ALIASES[op] ?? op; + return !VALID_AST_OPERATORS.has(String(target).toLowerCase()); + }); + expect( + uncovered, + 'an author can declare these on a ViewFilterRule and the spec validates them, ' + + 'but they reach the wire unmapped and the filter is silently dropped', + ).toEqual([]); + }); + + it('maps the date comparisons that regressed', () => { + expect(FILTER_OPERATOR_ALIASES.before).toBe('<'); + expect(FILTER_OPERATOR_ALIASES.after).toBe('>'); + }); +}); diff --git a/packages/data-objectstack/src/index.ts b/packages/data-objectstack/src/index.ts index fcf4211fdc..6829e48528 100644 --- a/packages/data-objectstack/src/index.ts +++ b/packages/data-objectstack/src/index.ts @@ -47,8 +47,15 @@ import { * (e.g. `lead.view.ts`) to the canonical operator symbols expected by the * ObjectStack server's filter AST. Unknown operators fall through unchanged * so existing AST-style entries keep working. + * + * Every VALUE here must be a member of the spec's `VALID_AST_OPERATORS` + * (`@objectstack/spec/data`) — that set gates `isFilterAST()`, and a filter it + * rejects is not converted, not validated, and then silently DROPPED by + * driver-sql (objectstack#3948). Pinned by `filter-operator-ast-parity.test.ts`. + * + * Exported for that test. @internal */ -const FILTER_OPERATOR_ALIASES: Record = { +export const FILTER_OPERATOR_ALIASES: Record = { equals: '=', eq: '=', '==': '=', @@ -85,6 +92,15 @@ const FILTER_OPERATOR_ALIASES: Record = { isnull: 'isnull', is_not_null: 'isnotnull', isnotnull: 'isnotnull', + // Date comparisons. `before`/`after` are CANONICAL members of the spec's + // `VIEW_FILTER_OPERATORS` (ui/view.zod.ts), so a stored view legitimately + // carries them — but they are absent from `VALID_AST_OPERATORS` + // (data/filter.zod.ts), which gates `isFilterAST()`. Without these two + // entries they reached the wire unchanged, the server's AST gate rejected + // the shape, and driver-sql skipped the filter ENTIRELY — an unfiltered + // result set with no error anywhere. objectstack#3948. + before: '<', + after: '>', }; function normalizeFilterOperator(op: unknown): string | null { diff --git a/packages/plugin-list/package.json b/packages/plugin-list/package.json index f26074aac9..6feaf0e24b 100644 --- a/packages/plugin-list/package.json +++ b/packages/plugin-list/package.json @@ -53,6 +53,7 @@ "@object-ui/mobile": "workspace:*", "@object-ui/react": "workspace:*", "@object-ui/types": "workspace:*", + "@objectstack/spec": "^17.0.0-rc.0", "@types/react": "19.2.17", "@types/react-dom": "19.2.3", "@vitejs/plugin-react": "^6.0.4", diff --git a/packages/plugin-list/src/ListView.tsx b/packages/plugin-list/src/ListView.tsx index 28ae038256..9047f21dc4 100644 --- a/packages/plugin-list/src/ListView.tsx +++ b/packages/plugin-list/src/ListView.tsx @@ -59,27 +59,57 @@ export interface ListViewProps { // Helper to convert FilterBuilder group to ObjectStack AST. // Accepts both the FilterBuilder vocabulary (camelCase) and the // @objectstack/spec ViewFilterRule vocabulary (snake_case). -function mapOperator(op: string) { - switch (op) { +/** + * Filter-builder / view operator → filter-AST operator. + * + * Every value returned must be a member of the spec's `VALID_AST_OPERATORS` + * (`@objectstack/spec/data`). That set gates `isFilterAST()`, and a filter it + * rejects is passed through unconverted and then silently DROPPED by driver-sql + * — an unfiltered result set with no error (objectstack#3948). Pinned by + * `filter-operator-ast-parity.test.ts`. + * + * Exported for that test. @internal + */ +export function mapOperator(op: string) { + // The spec's alias table carries the same operator in up to four spellings + // (`not_equals`, `notEquals`, `notequals`, `ne`), and stored view metadata + // holds all of them — `saveMeta` persists the authored body verbatim, so the + // spec's own normalization never reaches the row. Matching them case- and + // underscore-insensitively collapses that whole class instead of enumerating + // it: a switch listing spellings by hand had already missed eight. + switch (op.toLowerCase().replace(/[_\s]/g, '')) { case 'equals': case 'eq': return '='; - case 'notEquals': case 'not_equals': case 'ne': case 'neq': return '!='; + case 'notequals': case 'ne': case 'neq': return '!='; case 'contains': return 'contains'; - case 'notContains': case 'not_contains': case 'notcontains': return 'notcontains'; - case 'startsWith': case 'starts_with': return 'startswith'; - case 'greaterThan': case 'greater_than': case 'gt': return '>'; - case 'greaterOrEqual': case 'greater_than_or_equal': case 'gte': return '>='; - case 'lessThan': case 'less_than': case 'lt': return '<'; - case 'lessOrEqual': case 'less_than_or_equal': case 'lte': return '<='; + case 'notcontains': return 'notcontains'; + case 'startswith': return 'startswith'; + case 'endswith': return 'endswith'; + case 'greaterthan': case 'gt': return '>'; + case 'greaterorequal': case 'greaterthanorequal': case 'gte': return '>='; + case 'lessthan': case 'lt': return '<'; + case 'lessorequal': case 'lessthanorequal': case 'lte': return '<='; case 'in': return 'in'; - case 'notIn': case 'not_in': case 'nin': return 'not in'; + // `nin`, not `'not in'`: the spaced spelling is in no spec vocabulary, so + // `isFilterAST()` rejected it and driver-sql skipped the filter entirely. + // The array case never reached the wire (normalizeFilterCondition expands + // it below), but a non-array value escaped as an unfiltered query. + case 'notin': case 'nin': return 'nin'; + // Canonical `VIEW_FILTER_OPERATORS` members with no AST counterpart; the + // gap here is what returned unfiltered rows for a stored date filter. case 'before': return '<'; case 'after': return '>'; + case 'between': return 'between'; + case 'isnull': return 'isnull'; + case 'isnotnull': return 'isnotnull'; default: return op; } } +/** Every not-in spelling this normalizer expands. See the note at the call site. */ +const NOT_IN_SPELLINGS = new Set(['nin', 'not_in', 'notIn', 'notin', 'not in']); + /** - * Normalize a single filter condition: convert `in`/`not in` operators + * Normalize a single filter condition: convert `in`/not-in operators * into backend-compatible `or`/`and` of equality conditions. * E.g., ['status', 'in', ['a','b']] → ['or', ['status','=','a'], ['status','=','b']] */ @@ -101,7 +131,10 @@ export function normalizeFilterCondition(condition: any[]): any[] { return ['or', ...value.map((v: any) => [field, '=', v])]; } - if (op === 'not in' && Array.isArray(value)) { + // `nin` is what mapOperator now emits; the rest are spellings an external + // caller may still pass, since this function is part of plugin-list's public + // surface. Accepting all of them keeps the expansion working either way. + if (NOT_IN_SPELLINGS.has(op) && Array.isArray(value)) { if (value.length === 0) return []; if (value.length === 1) return [field, '!=', value[0]]; return ['and', ...value.map((v: any) => [field, '!=', v])]; diff --git a/packages/plugin-list/src/__tests__/filter-operator-ast-parity.test.ts b/packages/plugin-list/src/__tests__/filter-operator-ast-parity.test.ts new file mode 100644 index 0000000000..366f30510b --- /dev/null +++ b/packages/plugin-list/src/__tests__/filter-operator-ast-parity.test.ts @@ -0,0 +1,98 @@ +/** + * ObjectUI + * Copyright (c) 2024-present ObjectStack Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +/** + * View operator → filter-AST operator parity (#2901, objectstack#3948). + * + * The spec ships two operator vocabularies that must agree at this boundary: + * + * - `VIEW_FILTER_OPERATORS` (`ui/view.zod.ts`) — what an author may declare on + * a `ViewFilterRule`, and what `ViewFilterRuleSchema` validates against. + * - `VALID_AST_OPERATORS` (`data/filter.zod.ts`) — what gates `isFilterAST()` + * on the server, deciding whether a filter is parsed into a query at all. + * + * They do NOT overlap: 8 of the 19 canonical view operators are absent from the + * AST set. `mapOperator` is what bridges them, and a gap in it is invisible — + * `isFilterAST()` returns false, the protocol passes the array through + * unconverted, and driver-sql then skips it entirely. **No WHERE clause, no + * error, every row returned.** That is how `before`/`after` shipped broken: they + * are canonical view operators with no entry in the bridge. + * + * These tests assert the bridge is total, so the next operator the spec adds to + * the view vocabulary fails here instead of silently returning unfiltered rows. + */ +import { describe, it, expect } from 'vitest'; +import { VALID_AST_OPERATORS, isFilterAST } from '@objectstack/spec/data'; +import { VIEW_FILTER_OPERATORS, VIEW_FILTER_OPERATOR_ALIASES } from '@objectstack/spec/ui'; +import { mapOperator, normalizeFilterCondition } from '../ListView'; + +/** Operators this bridge deliberately resolves without reaching the AST gate. */ +const HANDLED_BEFORE_MAPPING = new Set([ + // convertFilterGroupToAST rewrites these to `[field, '=' | '!=', null]` + // before mapOperator is consulted, so they never need an AST spelling. + 'is_empty', 'is_not_empty', +]); + +describe('mapOperator bridges the spec view vocabulary onto the AST vocabulary', () => { + it('reads both vocabularies from the spec', () => { + // Guards every assertion below against silently passing on an empty list. + expect(VIEW_FILTER_OPERATORS.length).toBeGreaterThan(0); + expect(VALID_AST_OPERATORS.size).toBeGreaterThan(0); + }); + + const bridged = VIEW_FILTER_OPERATORS.filter((op) => !HANDLED_BEFORE_MAPPING.has(op)); + + it.each(bridged)('%s maps to an AST-valid operator', (viewOp) => { + const mapped = mapOperator(viewOp); + expect( + VALID_AST_OPERATORS.has(String(mapped).toLowerCase()), + `mapOperator('${viewOp}') → '${mapped}', which VALID_AST_OPERATORS rejects. ` + + 'isFilterAST() will return false and the filter will be silently dropped ' + + 'server-side — an unfiltered result set, not an error.', + ).toBe(true); + }); + + it.each(bridged)('a single %s condition survives the isFilterAST gate', (viewOp) => { + // The reachable shape: one condition, AND logic, emitted as a bare triple. + // This is exactly what silently full-scanned before the fix. + const value = viewOp === 'in' || viewOp === 'not_in' ? ['a', 'b'] : 'x'; + const triple = normalizeFilterCondition(['some_field', mapOperator(viewOp), value]); + expect( + isFilterAST(triple), + `a '${viewOp}' filter produced ${JSON.stringify(triple)}, which isFilterAST() rejects`, + ).toBe(true); + }); + + it('also bridges every legacy alias the spec still folds', () => { + // Stored view metadata carries these: saveMeta persists the authored body + // verbatim, so the spec's z.preprocess normalization never reaches the row. + const unbridged = Object.keys(VIEW_FILTER_OPERATOR_ALIASES) + .filter((alias) => !HANDLED_BEFORE_MAPPING.has(VIEW_FILTER_OPERATOR_ALIASES[alias])) + .filter((alias) => !VALID_AST_OPERATORS.has(String(mapOperator(alias)).toLowerCase())); + expect( + unbridged, + 'these legacy spellings exist in stored view metadata and map to no AST operator', + ).toEqual([]); + }); + + it('emits `nin`, never the spaced `not in`, which no spec vocabulary defines', () => { + for (const spelling of ['notIn', 'not_in', 'nin']) { + expect(mapOperator(spelling)).toBe('nin'); + } + }); + + it('still expands a not-in array into an AND of inequalities', () => { + // Regression: the expansion keyed on the old spaced spelling. + expect(normalizeFilterCondition(['stage', 'nin', ['won', 'lost']])) + .toEqual(['and', ['stage', '!=', 'won'], ['stage', '!=', 'lost']]); + // …and keeps accepting the spellings an external caller may pass, since + // normalizeFilterCondition is part of plugin-list's public surface. + expect(normalizeFilterCondition(['stage', 'not in', ['won', 'lost']])) + .toEqual(['and', ['stage', '!=', 'won'], ['stage', '!=', 'lost']]); + }); +}); diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 77915732ed..4886e8cb50 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -1137,6 +1137,9 @@ importers: specifier: ^17.0.0-rc.0 version: 17.0.0-rc.0(ai@7.0.37(zod@4.4.3)) devDependencies: + '@objectstack/spec': + specifier: ^17.0.0-rc.0 + version: 17.0.0-rc.0(ai@7.0.37(zod@4.4.3)) tsup: specifier: ^8.5.1 version: 8.5.1(@microsoft/api-extractor@7.58.2(@types/node@26.1.1))(@swc/core@1.15.33)(jiti@2.7.0)(postcss@8.5.23)(tsx@4.23.1)(typescript@6.0.3)(yaml@2.9.0) @@ -2097,6 +2100,9 @@ importers: '@object-ui/types': specifier: workspace:* version: link:../types + '@objectstack/spec': + specifier: ^17.0.0-rc.0 + version: 17.0.0-rc.0(ai@7.0.37(zod@4.4.3)) '@types/react': specifier: 19.2.17 version: 19.2.17