Skip to content

Commit c4d7b20

Browse files
os-zhuangclaude
andauthored
fix(view,list,core): a view's filter no longer disappears, or arrives as a predicate on columns that don't exist (#3081)
Sweeping the other `$filter` producers after #3078 turned up two live defects in `ObjectView`, which fetches its own data for calendar / kanban / gallery / timeline (grid delegates to `ObjectGrid`). 1. AN OBJECT FILTER WAS DROPPED, AND ONLY FOR NON-GRID VIEWS. `table.defaultFilters` is declared `Record<string, any>`, and the merge tested `baseFilter.length > 0` — `undefined > 0` for an object. The filter vanished and the view returned EVERY RECORD. `ObjectGrid` assigns the same value straight to `params.$filter`, so one view definition filtered correctly as a grid and returned everything as a calendar. 2. RULE OBJECTS WERE SPREAD INTO THE `and`, NOT WRAPPED. `['and', ...baseFilter, ...userFilter]` is only correct when the source is an array of AST nodes. `activeView.filter` is a spec `ViewFilterRule[]`, so spreading put bare rule objects where the AST expects nodes: isFilterAST(['and', {field:'stage',operator:'eq',value:'won'}, ['owner','=','me']]) // false → 400 since objectstack#4121 parseFilterAST(same) // {$and:[{field:'stage',operator:'eq',value:'won'}, {owner:'me'}]} The second line is a predicate over three columns named `field`, `operator` and `value` — which do not exist. Reachable whenever a view with a filter meets a user filter value. New in core: `toFilterNode` normalizes one source (rule array / AST / MongoDB object) and `mergeFilterNodes` combines sources as siblings under one `and`. `ObjectView` and `ListView.buildEffectiveFilter` both use them, so the three filter shapes are reconciled in one place instead of by hand at each renderer. The adapter also now translates a bare rule object sitting directly under a logical node — the chokepoint defence for any producer still emitting the spread shape. Only rule-SHAPED objects are touched; a child with no `field` is a genuine MongoDB condition and passes through untouched. CORRECTING A COMMENT SHIPPED IN #3078. `buildEffectiveFilter` documented the dropped-object case as unreachable, "nothing in this repo produces one for a list view". That was wrong: `ObjectView` passes `mergedFilters` straight into that schema's `filter`, and its last fallback is `table.defaultFilters`. The case is now handled rather than explained away. Also checked and clean: the dashboard widgets (metric/chart/pivot/data-table) pass a single resolved filter with no cross-vocabulary merge; gallery, gantt, calendar and map forward `schema.filter` unchanged; `filter-converter`'s own `['and', ...conditions]` spreads tuples it built itself. `ObjectView` was the only other producer merging two vocabularies. Verification: 19 tests across the four packages; reverting each source file fails the ones covering it. Emitted filters are asserted against the spec's own `isFilterAST` / `parseFilterAST`, including an executable pin on what the old spread shape produced. Full suite 761 files / 8856 tests green; tsc clean; eslint 0 errors. Co-authored-by: Jack Zhuang <277994282+os-zhuang@users.noreply.github.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
1 parent 0fcd4a9 commit c4d7b20

9 files changed

Lines changed: 429 additions & 31 deletions

File tree

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,57 @@
1+
---
2+
"@object-ui/core": patch
3+
"@object-ui/data-objectstack": patch
4+
"@object-ui/plugin-list": patch
5+
"@object-ui/plugin-view": patch
6+
---
7+
8+
fix(view,list,core): a view's filter no longer disappears, or arrives as a predicate on columns that don't exist
9+
10+
Sweeping the other `$filter` producers after #3078 turned up two live defects in
11+
`ObjectView`, which fetches its own data for calendar / kanban / gallery /
12+
timeline (grid delegates to `ObjectGrid`).
13+
14+
**1. An object filter was dropped, and only for non-grid views.**
15+
`table.defaultFilters` is declared `Record<string, any>`, and the merge tested
16+
`baseFilter.length > 0``undefined > 0` for an object. So the filter vanished
17+
and the view returned **every record**. `ObjectGrid` assigns the same value
18+
straight to `params.$filter`, so one view definition filtered correctly as a
19+
grid and returned everything as a calendar.
20+
21+
**2. Rule objects were spread into the `and`, not wrapped.**
22+
`['and', ...baseFilter, ...userFilter]` is only correct when the source is an
23+
array of AST nodes. `activeView.filter` is a spec `ViewFilterRule[]`, so
24+
spreading put bare rule objects where the AST expects nodes:
25+
26+
```js
27+
isFilterAST(['and', {field:'stage',operator:'eq',value:'won'}, ['owner','=','me']])
28+
// false → 400 since objectstack#4121
29+
parseFilterAST(same)
30+
// {$and:[{field:'stage',operator:'eq',value:'won'}, {owner:'me'}]}
31+
```
32+
33+
That second line is a predicate over three columns named `field`, `operator`
34+
and `value` — which don't exist. Reachable whenever a view with a filter meets a
35+
user filter value.
36+
37+
New in `@object-ui/core`: `toFilterNode` normalizes one source (rule array / AST
38+
/ MongoDB object) and `mergeFilterNodes` combines sources as siblings under one
39+
`and`. `ObjectView` and `ListView.buildEffectiveFilter` both use them, so the
40+
three filter shapes are reconciled in one place instead of by hand at each
41+
renderer.
42+
43+
`ObjectStackAdapter` also now translates a bare rule object sitting directly
44+
under a logical node — the chokepoint defence for any producer still emitting
45+
the spread shape. Only rule-*shaped* objects are touched; a child with no
46+
`field` is a genuine MongoDB condition and passes through untouched.
47+
48+
**Correcting a comment shipped in #3078.** `buildEffectiveFilter` documented the
49+
dropped-object case as unreachable, "nothing in this repo produces one for a
50+
list view". That was wrong: `ObjectView` passes `mergedFilters` straight into
51+
that schema's `filter`, and its last fallback is `table.defaultFilters`. The
52+
case is now handled rather than explained away.
53+
54+
Verified with 19 tests across the four packages; reverting each source file
55+
fails the ones that cover it. Emitted filters are asserted against the spec's
56+
own `isFilterAST` / `parseFilterAST`, including an executable pin on what the
57+
old spread shape produced.
Lines changed: 104 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,104 @@
1+
/**
2+
* ObjectUI
3+
* Copyright (c) 2024-present ObjectStack Inc.
4+
*
5+
* This source code is licensed under the MIT license found in the
6+
* LICENSE file in the root directory of this source tree.
7+
*/
8+
9+
/**
10+
* Merging the filter sources a view can carry.
11+
*
12+
* Three shapes are in circulation and all are legitimate — a spec
13+
* `ViewFilterRule[]`, an AST node, and a MongoDB-style object. Renderers merged
14+
* them by hand and got two things wrong, both silent:
15+
*
16+
* 1. They tested `source.length > 0` before using it. That is `undefined > 0`
17+
* for an object, so a `table.defaultFilters` (declared `Record<string,
18+
* any>`) was DROPPED and the view returned every record.
19+
* 2. They SPREAD a source into the `and` (`['and', ...rules]`). That is only
20+
* correct when the source is an array of nodes; for a `ViewFilterRule[]`
21+
* it puts bare rule objects where the AST expects nodes. `isFilterAST`
22+
* rejects that (a 400 since objectstack#4121) and `parseFilterAST` reads
23+
* the rule as a Mongo condition — filtering on columns literally named
24+
* `field`, `operator` and `value`.
25+
*
26+
* The emitted shape is asserted against the server's own `isFilterAST` rather
27+
* than a restated literal wherever the result is an AST.
28+
*/
29+
30+
import { describe, it, expect } from 'vitest';
31+
import { isFilterAST, parseFilterAST } from '@objectstack/spec/data';
32+
import { toFilterNode, mergeFilterNodes } from '../filter-converter';
33+
34+
const RULES = [{ field: 'stage', operator: 'eq', value: 'won' }];
35+
const TUPLE = ['owner', '=', 'me'];
36+
37+
describe('toFilterNode', () => {
38+
it('passes a non-empty array source through unchanged', () => {
39+
expect(toFilterNode(RULES)).toEqual(RULES);
40+
expect(toFilterNode([TUPLE])).toEqual([TUPLE]);
41+
});
42+
43+
it('converts a MongoDB-style object into an AST node', () => {
44+
expect(toFilterNode({ status: 'active' })).toEqual(['status', '=', 'active']);
45+
});
46+
47+
it('treats absent and empty sources as nothing', () => {
48+
for (const empty of [undefined, null, [], {}, '', 0]) {
49+
expect(toFilterNode(empty)).toBeUndefined();
50+
}
51+
});
52+
});
53+
54+
describe('mergeFilterNodes', () => {
55+
it('returns undefined when every source is empty', () => {
56+
expect(mergeFilterNodes(undefined, [], {})).toBeUndefined();
57+
});
58+
59+
it('returns a lone source as-is rather than wrapping it in a pointless and', () => {
60+
expect(mergeFilterNodes([TUPLE], undefined)).toEqual([TUPLE]);
61+
});
62+
63+
it('wraps each source as its own child — never spreads it', () => {
64+
// The regression. Spreading would give ['and', {field…}, 'owner', '=', 'me'].
65+
expect(mergeFilterNodes(RULES, TUPLE)).toEqual(['and', RULES, TUPLE]);
66+
});
67+
68+
it('keeps an object source instead of dropping it', () => {
69+
// `table.defaultFilters` is declared `Record<string, any>`; the old
70+
// `.length > 0` guard read false and the whole filter disappeared.
71+
expect(mergeFilterNodes({ status: 'active' }, TUPLE))
72+
.toEqual(['and', ['status', '=', 'active'], TUPLE]);
73+
});
74+
});
75+
76+
describe('what reaches the server', () => {
77+
/**
78+
* `ViewFilterRule[]` is not itself AST — the adapter translates it on the way
79+
* out — so `isFilterAST` is only the right oracle for the all-AST cases.
80+
*/
81+
it('produces a node the server accepts when every source is AST', () => {
82+
expect(isFilterAST(mergeFilterNodes([TUPLE], ['amount', '>', 1]))).toBe(true);
83+
expect(isFilterAST(mergeFilterNodes({ status: 'active' }, ['amount', '>', 1]))).toBe(true);
84+
expect(isFilterAST(mergeFilterNodes({ status: 'active' }))).toBe(true);
85+
});
86+
87+
it('an object source survives the round trip to a real predicate', () => {
88+
const merged = mergeFilterNodes({ status: 'active' }, ['amount', '>', 1]);
89+
expect(parseFilterAST(merged)).toEqual({
90+
$and: [{ status: 'active' }, { amount: { $gt: 1 } }],
91+
});
92+
});
93+
94+
it('the spread shape it replaces was NOT acceptable — pinning why', () => {
95+
// What `['and', ...rules, ...tuples]` produced. Kept as executable evidence
96+
// that the wrapping above is not a stylistic preference.
97+
const spread = ['and', ...RULES, TUPLE];
98+
expect(isFilterAST(spread)).toBe(false);
99+
// Worse than rejected: read as a predicate over three columns that do not exist.
100+
expect(parseFilterAST(spread)).toEqual({
101+
$and: [{ field: 'stage', operator: 'eq', value: 'won' }, { owner: 'me' }],
102+
});
103+
});
104+
});

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

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -152,3 +152,54 @@ export function convertFiltersToAST(filter: Record<string, any>): FilterNode | R
152152
// Multiple conditions: combine with 'and'
153153
return ['and', ...conditions];
154154
}
155+
156+
/**
157+
* Normalize ONE filter source into a single filter node.
158+
*
159+
* A "source" is whatever a view hands a renderer, and there are three shapes in
160+
* circulation, all legitimate:
161+
*
162+
* - `[{ field, operator, value }, ...]` a spec `ViewFilterRule[]`
163+
* - `[['stage', '=', 'won'], ...]` an AST node / legacy flat array
164+
* - `{ status: 'active' }` a MongoDB-style object
165+
*
166+
* The third is the one that kept getting lost. Renderers tested `source.length
167+
* > 0` before using it, which is `undefined > 0` for an object — so a
168+
* `table.defaultFilters` (declared `Record<string, any>`) was DROPPED and the
169+
* view returned every record. Silently: no error, just a wider answer.
170+
*
171+
* Returns `undefined` for an absent or empty source, so callers can skip
172+
* `$filter` rather than sending an empty array.
173+
*/
174+
export function toFilterNode(source: unknown): FilterNode | Record<string, any> | undefined {
175+
if (source === null || source === undefined) return undefined;
176+
if (Array.isArray(source)) return source.length > 0 ? (source as FilterNode) : undefined;
177+
if (typeof source !== 'object') return undefined;
178+
const obj = source as Record<string, any>;
179+
if (Object.keys(obj).length === 0) return undefined;
180+
// MongoDB-style → AST, so it can sit beside the other shapes under one `and`.
181+
return convertFiltersToAST(obj);
182+
}
183+
184+
/**
185+
* Combine filter sources under a single `and`, each as its OWN child.
186+
*
187+
* Wrapping rather than spreading, on purpose. `['and', ...rules]` looks
188+
* equivalent and is not: spreading a `ViewFilterRule[]` puts bare rule OBJECTS
189+
* where the AST expects nodes, and the server neither understands nor rejects
190+
* that cleanly — `isFilterAST` says no (a 400 since objectstack#4121), while
191+
* `parseFilterAST` reads the rule as a Mongo condition and filters on columns
192+
* literally named `field` / `operator` / `value`. Spreading is only correct
193+
* when the source happens to be an array of nodes, which is why it survived.
194+
*
195+
* Sources that normalize to nothing are skipped; one surviving source is
196+
* returned as-is rather than wrapped in a pointless `and`.
197+
*/
198+
export function mergeFilterNodes(
199+
...sources: unknown[]
200+
): FilterNode | Record<string, any> | undefined {
201+
const nodes = sources.map(toFilterNode).filter((n) => n !== undefined);
202+
if (nodes.length === 0) return undefined;
203+
if (nodes.length === 1) return nodes[0];
204+
return ['and', ...nodes] as FilterNode;
205+
}

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

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -257,6 +257,32 @@ describe('what this adapter emits passes the server’s own gate', () => {
257257
}
258258
});
259259

260+
describe('a bare rule object directly under a logical node', () => {
261+
beforeEach(() => clearSharedDiscoveryCache());
262+
263+
/**
264+
* Produced by any caller that SPREADS a `ViewFilterRule[]` into an `and`
265+
* rather than wrapping it — `['and', ...rules, ...tuples]`. The rules land as
266+
* bare objects where the AST expects nodes, `isFilterAST` rejects the whole
267+
* node (a 400 since objectstack#4121), and `parseFilterAST` reads the rule as
268+
* a Mongo condition on columns literally named `field`/`operator`/`value`.
269+
* objectui's own producer was fixed to wrap; this is the chokepoint defence.
270+
*/
271+
bothRoutes(
272+
'is translated in place',
273+
['and', { field: 'stage', operator: 'eq', value: 'won' }, ['owner', '=', 'me']],
274+
(wire) => expect(wire).toEqual(['and', ['stage', '=', 'won'], ['owner', '=', 'me']]),
275+
);
276+
277+
bothRoutes(
278+
'leaves a genuine MongoDB condition child alone',
279+
// No `field` key, so it is a condition on a column named `status` — not a
280+
// rule. Translating it would invent a filter the caller never wrote.
281+
['and', { status: 'active' }, ['owner', '=', 'me']],
282+
(wire) => expect(wire).toEqual(['and', { status: 'active' }, ['owner', '=', 'me']]),
283+
);
284+
});
285+
260286
describe('the two routes agree on shapes that are neither form', () => {
261287
beforeEach(() => clearSharedDiscoveryCache());
262288

packages/data-objectstack/src/index.ts

Lines changed: 22 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -238,9 +238,29 @@ function translateFilterArray(filter: unknown[]): unknown[] {
238238
return filter;
239239
}
240240

241-
/** A child of a logical node: another array node, or a value we leave alone. */
241+
/**
242+
* A child of a logical node: another array node, a bare rule object, or a value
243+
* we leave alone.
244+
*
245+
* The bare-rule case comes from producers that SPREAD a `ViewFilterRule[]` into
246+
* an `and` (`['and', ...rules, ...tuples]`) instead of wrapping it. That puts
247+
* rule objects where the AST expects nodes, and the server has no good answer:
248+
* `isFilterAST` rejects it (a 400 since objectstack#4121), while
249+
* `parseFilterAST` reads the rule as a MongoDB condition and filters on columns
250+
* literally named `field` / `operator` / `value` — three columns that do not
251+
* exist, so the honest-looking result is empty.
252+
*
253+
* Only rule-SHAPED objects are translated: a child with no `field` is a genuine
254+
* MongoDB condition (`{ status: 'active' }`) and must pass through untouched.
255+
* Same discriminator `isObjectFilterEntryForm` uses at the top level.
256+
*/
242257
function translateFilterChild(child: unknown): unknown {
243-
return Array.isArray(child) && child.length > 0 ? translateFilterArray(child) : child;
258+
if (Array.isArray(child)) return child.length > 0 ? translateFilterArray(child) : child;
259+
if (child && typeof child === 'object' && (child as any).field !== undefined) {
260+
const tuple = objectFilterEntryToAST(child);
261+
if (tuple) return tuple;
262+
}
263+
return child;
244264
}
245265

246266
/**

packages/plugin-list/src/ListView.tsx

Lines changed: 15 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@ import { useDensityMode } from '@object-ui/react';
1919
import type { ListViewSchema } from '@object-ui/types';
2020
import { detectStatusField } from '@object-ui/types';
2121
import { usePullToRefresh } from '@object-ui/mobile';
22-
import { resolveConditionalFormatting, buildExpandFields, buildExportFileName, resolveCrudAffordances, normalizeListViewSchema, rowHeightToDensityMode } from '@object-ui/core';
22+
import { resolveConditionalFormatting, buildExpandFields, buildExportFileName, resolveCrudAffordances, normalizeListViewSchema, rowHeightToDensityMode, mergeFilterNodes } from '@object-ui/core';
2323
import { useObjectTranslation, useObjectLabel, useSafeFieldLabel, createSafeTranslation } from '@object-ui/i18n';
2424
import { usePermissions } from '@object-ui/permissions';
2525

@@ -232,26 +232,27 @@ export function normalizeFilters(filters: any[]): any[] {
232232
* Returns `undefined` when nothing is active, so callers can skip `$filter`
233233
* entirely rather than sending an empty array.
234234
*
235-
* Known gap, preserved deliberately: a MongoDB-style object `schema.filter` has
236-
* no `.length` and is dropped here, as it always has been. Nothing in this repo
237-
* produces one for a list view (they come from dashboard widgets, which query
238-
* directly), so this stays a pure extraction rather than a behaviour change.
235+
* A MongoDB-style object `schema.filter` used to be dropped here (`.length` is
236+
* `undefined` on an object, so the guard read false) and the list returned every
237+
* record. An earlier version of this comment called that unreachable, on the
238+
* grounds that nothing in the repo hands a list view an object filter. That was
239+
* wrong: `ObjectView` passes `mergedFilters` straight into this schema's
240+
* `filter`, and its last fallback is `table.defaultFilters`, declared
241+
* `Record<string, any>`. `toFilterNode` now converts it instead.
239242
*/
240243
export function buildEffectiveFilter(
241244
baseFilter: unknown,
242245
currentFilters: FilterGroup,
243246
userFilterConditions: any[],
244-
): any[] | undefined {
245-
const base = Array.isArray(baseFilter) ? baseFilter : [];
247+
): any[] | Record<string, any> | undefined {
246248
const userFilter = convertFilterGroupToAST(currentFilters);
247-
const allFilters = [
248-
...(base.length > 0 ? [base] : []),
249-
...(userFilter.length > 0 ? [userFilter] : []),
250-
// Normalize the per-field conditions (expands `in` into an `or` of `=`).
249+
return mergeFilterNodes(
250+
baseFilter,
251+
userFilter.length > 0 ? userFilter : undefined,
252+
// Normalize the per-field conditions (expands `in` into an `or` of `=`),
253+
// each of which is already its own node.
251254
...normalizeFilters(userFilterConditions),
252-
].filter((f: any) => Array.isArray(f) && f.length > 0);
253-
if (allFilters.length === 0) return undefined;
254-
return allFilters.length === 1 ? allFilters[0] : ['and', ...allFilters];
255+
);
255256
}
256257

257258
export function convertFilterGroupToAST(group: FilterGroup): any[] {

packages/plugin-list/src/__tests__/ListView.exportMatchesView.test.tsx

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -100,6 +100,34 @@ describe('ListView export mirrors the active view', () => {
100100
});
101101
});
102102

103+
describe('a MongoDB-style view filter is not dropped', () => {
104+
beforeEach(() => {
105+
vi.spyOn(URL, 'createObjectURL').mockReturnValue('blob:x');
106+
vi.spyOn(URL, 'revokeObjectURL').mockImplementation(() => {});
107+
});
108+
afterEach(() => vi.restoreAllMocks());
109+
110+
/**
111+
* `ObjectView` passes `mergedFilters` straight into this schema's `filter`,
112+
* and its last fallback is `table.defaultFilters` — declared
113+
* `Record<string, any>`. The old `baseFilter.length > 0` guard read false for
114+
* an object, so the list queried unfiltered and showed every record. An
115+
* earlier comment here called that unreachable; it was not.
116+
*/
117+
it('reaches the query as an AST instead of vanishing', async () => {
118+
const { find } = harness({ ...BASE, filter: { status: 'active' } as any });
119+
await vi.waitFor(() => expect(find).toHaveBeenCalled());
120+
expect(find.mock.calls[0][1]?.$filter).toEqual(['status', '=', 'active']);
121+
});
122+
123+
it('reaches the export too, by the same route', async () => {
124+
const { exportDownload } = harness({ ...BASE, filter: { status: 'active' } as any });
125+
await clickExportCsv();
126+
await vi.waitFor(() => expect(exportDownload).toHaveBeenCalledTimes(1));
127+
expect(exportDownload.mock.calls[0][1]?.filter).toEqual(['status', '=', 'active']);
128+
});
129+
});
130+
103131
describe('the fetch and the export build the SAME filter', () => {
104132
beforeEach(() => {
105133
vi.spyOn(URL, 'createObjectURL').mockReturnValue('blob:x');

0 commit comments

Comments
 (0)