diff --git a/.changeset/saved-view-filter-rules-to-ast.md b/.changeset/saved-view-filter-rules-to-ast.md new file mode 100644 index 0000000000..c16f8ec680 --- /dev/null +++ b/.changeset/saved-view-filter-rules-to-ast.md @@ -0,0 +1,7 @@ +--- +'@object-ui/core': patch +--- + +`toFilterNode` now lowers a spec `ViewFilterRule[]` into ObjectQL AST nodes instead of returning the array verbatim, so a saved view's stored filter reaches `$filter` as something the server accepts. It previously did not: `ListViewSchema.filter` / `ViewTab.filter` are declared `z.array(ViewFilterRuleSchema)`, and the whole read path — `ObjectView` → `ListViewSchema.filter` → `buildEffectiveFilter` → `mergeFilterNodes` → `toFilterNode` — carried those rule objects untouched into the query. `isFilterAST` is `false` for an array of objects, so the data API answered `400 INVALID_FILTER` and the list rendered no rows at all. Measured against a real backend on the showcase's shipped `showcase_task.in_progress` view: `$filter=[{"field":"status","operator":"equals","value":"in_progress"}]` returned `400`, while the lowered `[["status","equals","in_progress"]]` returned its 2 rows. Every saved view carrying a filter was affected, on both producers that share this sink — `plugin-list`'s `buildEffectiveFilter` (the grid and its export) and `plugin-view`'s `ObjectView` (calendar / kanban / gallery / timeline). + +Operators are canonicalised through the spec's own `normalizeFilterOperator`, the same exit the write side (`viewFilterFold`) uses, so the two directions cannot drift into two dialects; no second operator table is introduced. An operator the spec does not know is passed through verbatim so the server still refuses it loudly rather than having a misspelling coerced into a valid filter. AST nodes and MongoDB-style object filters are unaffected, mixed arrays (a view's rules concatenated with `?filter[]=` URL triples) fold element-wise with the triples untouched, and a rule with a blank `field` is deliberately left unlowered — `["", op, value]` passes `isFilterAST` and returns an empty list, whereas the unlowered rule keeps the loud `400`. diff --git a/e2e/live/saved-view-filter.spec.ts b/e2e/live/saved-view-filter.spec.ts new file mode 100644 index 0000000000..d8a8bf2cae --- /dev/null +++ b/e2e/live/saved-view-filter.spec.ts @@ -0,0 +1,59 @@ +import { test, expect } from '@playwright/test'; + +/** + * objectui#3431 — a saved view's `ViewFilterRule[]` must reach `$filter` as an + * ObjectQL AST, not as bare rule objects. + * + * This is the empirical pin for the defect, run against the REAL stack because + * only the server can answer the question the issue asked: does it accept a + * rule array in `$filter`? It does not. Measured on + * `@objectstack/*@17.0.0-rc.2` with the showcase app: + * + * GET /api/v1/data/showcase_task + * ?$filter=[{"field":"status","operator":"equals","value":"in_progress"}] + * -> HTTP 400 {"error":"Request failed","code":"INVALID_FILTER",...} + * + * GET /api/v1/data/showcase_task?$filter=[["status","equals","in_progress"]] + * -> HTTP 200 {"total":2,...} + * + * `showcase_task.in_progress` is a SHIPPED saved view whose stored filter is + * exactly that rule array (`src/ui/views/task.view.ts`), so no fixture setup is + * needed — opening it is the reproduction. Before the fix the list renders no + * rows at all (the request 400s, so `record-count-bar` never mounts); after it, + * the 2 in-progress tasks out of 10. + * + * Prereqs: the usual live-e2e pair (see playwright.live.config.ts). Run: + * pnpm test:e2e:live e2e/live/saved-view-filter.spec.ts + * + * NOT yet in the `test:e2e:live:ci` allowlist, on purpose: live-e2e.yml's own + * policy is that specs join it only after they have proven flake-free on the + * lane, and this one has no record yet. Promote it in a follow-up once the + * nightly has run it. + */ +const APP = process.env.SHOWCASE_APP_NAME || 'showcase_app'; + +test('a saved view filter reaches $filter as AST, and the server answers 200', async ({ page }) => { + // Record every data response the page provokes, so a failure says WHICH + // request was refused and with what payload — not just "no rows". + const refused: string[] = []; + page.on('response', async (res) => { + const url = res.url(); + if (!url.includes('/api/v1/data/showcase_task')) return; + if (res.status() < 400) return; + let code = ''; + try { + code = ((await res.json()) as { code?: string }).code ?? ''; + } catch { + /* non-JSON error body — the status alone is the signal */ + } + refused.push(`${res.status()} ${code} ${decodeURIComponent(url)}`); + }); + + await page.goto(`/apps/${APP}/showcase_task/view/showcase_task.in_progress`); + + // The filter applied: 2 of the 10 seeded tasks are in_progress. + await expect(page.getByTestId('record-count-bar')).toContainText(/^2 /, { timeout: 20000 }); + + // And it applied by being ACCEPTED, not by some later rescue. + expect(refused, `data requests the backend refused:\n${refused.join('\n')}`).toEqual([]); +}); 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 c20403d951..74cb546ca9 100644 --- a/packages/core/src/utils/__tests__/filter-source-merge.test.ts +++ b/packages/core/src/utils/__tests__/filter-source-merge.test.ts @@ -25,6 +25,25 @@ * * The emitted shape is asserted against the server's own `isFilterAST` rather * than a restated literal wherever the result is an AST. + * + * objectui#3431 — the third mistake, and the one this file used to certify. + * `toFilterNode` returned EVERY array verbatim, so a saved view's + * `ViewFilterRule[]` reached `$filter` as bare rule objects. The old case here + * ("passes a non-empty array source through unchanged") asserted exactly that, + * and the note below it explained it away with "the adapter translates it on + * the way out". No adapter does. Measured against a real backend + * (`@objectstack/*@17.0.0-rc.2`, the showcase app, the SHIPPED + * `showcase_task.in_progress` view): + * + * $filter=[{"field":"status","operator":"equals","value":"in_progress"}] + * -> HTTP 400 {"error":"Request failed","code":"INVALID_FILTER"} + * $filter=[["status","equals","in_progress"]] + * -> HTTP 200, total=2, every row status=in_progress + * + * so every saved view carrying a filter was a failed list. The rule cases below + * now pin the lowering; reverting `toFilterNode`'s fold turns them RED (they + * assert a produced value and `isFilterAST(...) === true`, not the absence of + * something). */ import { describe, it, expect } from 'vitest'; @@ -32,12 +51,20 @@ import { isFilterAST, parseFilterAST } from '@objectstack/spec/data'; import { toFilterNode, mergeFilterNodes, convertFiltersToAST, FilterOperatorError } from '../filter-converter'; const RULES = [{ field: 'stage', operator: 'eq', value: 'won' }]; +/** `RULES` after the fold — `eq` canonicalised by the spec's own normalizer. */ +const RULES_AS_AST = [['stage', 'equals', 'won']]; const TUPLE = ['owner', '=', 'me']; describe('toFilterNode', () => { - it('passes a non-empty array source through unchanged', () => { - expect(toFilterNode(RULES)).toEqual(RULES); + it('lowers a spec ViewFilterRule[] to AST nodes', () => { + expect(toFilterNode(RULES)).toEqual(RULES_AS_AST); + }); + + it('passes an array of AST nodes through unchanged', () => { expect(toFilterNode([TUPLE])).toEqual([TUPLE]); + // Same reference: nothing is rebuilt when there is no rule object to fold. + const nodes = [TUPLE, ['amount', '>', 1]]; + expect(toFilterNode(nodes)).toBe(nodes); }); it('converts a MongoDB-style object into an AST node', () => { @@ -51,6 +78,64 @@ describe('toFilterNode', () => { }); }); +describe('lowering a ViewFilterRule — the operator vocabulary (objectui#3431)', () => { + /** + * The write side (`app-shell/views/viewFilterFold.ts`) canonicalises through + * the spec's `normalizeFilterOperator`; so does the read side now. One exit, + * so the two directions cannot become two dialects — the failure the Studio + * inspector's private table already demonstrated (four operators behind). + */ + it('canonicalises legacy shorthand and camelCase spellings already in storage', () => { + expect(toFilterNode([{ field: 'a', operator: 'eq', value: 1 }])).toEqual([['a', 'equals', 1]]); + expect(toFilterNode([{ field: 'a', operator: 'nin', value: [1] }])).toEqual([['a', 'not_in', [1]]]); + expect(toFilterNode([{ field: 'a', operator: 'greaterThan', value: 1 }])) + .toEqual([['a', 'greater_than', 1]]); + expect(toFilterNode([{ field: 'a', operator: 'startsWith', value: 'x' }])) + .toEqual([['a', 'starts_with', 'x']]); + }); + + it('emits no value slot for a rule that carries none', () => { + // `is_empty` takes its direction from the operator NAME. A third element + // would be an invented `null` once the array is serialised — a real + // `{a: null}` predicate the author never wrote. + const node = toFilterNode([{ field: 'a', operator: 'is_empty' }]); + expect(node).toEqual([['a', 'is_empty']]); + expect(isFilterAST(node)).toBe(true); + expect(parseFilterAST(node)).toEqual({ a: { $null: true } }); + }); + + it('passes an operator the spec does not know through VERBATIM', () => { + // Not coerced to `equals`. The server's own gate then refuses it, which is + // the whole point: a misspelling must fail loudly, not silently widen the + // result set. + const node = toFilterNode([{ field: 'a', operator: 'sounds_like', value: 'x' }]); + expect(node).toEqual([['a', 'sounds_like', 'x']]); + expect(isFilterAST(node)).toBe(false); + }); + + it('folds the ObjectView concat element-wise, leaving URL triples byte-identical', () => { + // `ObjectView` builds `[...viewDef.filter, ...urlFilters]` — stored rules + // and `?filter[]=` triples in ONE array. Only the rules fold. + const urlTriple = ['account_id', '=', 'acct_1']; + const mixed = toFilterNode([{ field: 'stage', operator: 'equals', value: 'won' }, urlTriple]); + expect(mixed).toEqual([['stage', 'equals', 'won'], urlTriple]); + expect((mixed as unknown[])[1]).toBe(urlTriple); + expect(isFilterAST(mixed)).toBe(true); + expect(parseFilterAST(mixed)).toEqual({ $and: [{ stage: 'won' }, { account_id: 'acct_1' }] }); + }); + + it('does NOT lower a blank-field rule — a loud 400 beats a silently-empty list', () => { + // Measured on a live backend: `[["","equals","x"]]` passes `isFilterAST` + // and answers `200` with `total: 0`. Left as an object it is refused with + // `400 INVALID_FILTER`, which names the element. Same blank-row predicate + // the write side applies. + const blank = [{ field: '', operator: 'equals', value: 'x' }]; + expect(toFilterNode(blank)).toEqual(blank); + expect(isFilterAST(toFilterNode(blank))).toBe(false); + expect(isFilterAST([['', 'equals', 'x']])).toBe(true); // the shape NOT produced + }); +}); + describe('mergeFilterNodes', () => { it('returns undefined when every source is empty', () => { expect(mergeFilterNodes(undefined, [], {})).toBeUndefined(); @@ -62,7 +147,7 @@ describe('mergeFilterNodes', () => { it('wraps each source as its own child — never spreads it', () => { // The regression. Spreading would give ['and', {field…}, 'owner', '=', 'me']. - expect(mergeFilterNodes(RULES, TUPLE)).toEqual(['and', RULES, TUPLE]); + expect(mergeFilterNodes(RULES, TUPLE)).toEqual(['and', RULES_AS_AST, TUPLE]); }); it('keeps an object source instead of dropping it', () => { @@ -75,13 +160,40 @@ describe('mergeFilterNodes', () => { describe('what reaches the server', () => { /** - * `ViewFilterRule[]` is not itself AST — the adapter translates it on the way - * out — so `isFilterAST` is only the right oracle for the all-AST cases. + * `isFilterAST` / `parseFilterAST` are imported from `@objectstack/spec/data` + * — the SAME code the backend runs — so these are not a restatement of what + * we hope the server does. Every source shape must clear them, rule arrays + * included: there is no later adapter. */ - it('produces a node the server accepts when every source is AST', () => { + it('produces a node the server accepts, whichever shape the source had', () => { expect(isFilterAST(mergeFilterNodes([TUPLE], ['amount', '>', 1]))).toBe(true); expect(isFilterAST(mergeFilterNodes({ status: 'active' }, ['amount', '>', 1]))).toBe(true); expect(isFilterAST(mergeFilterNodes({ status: 'active' }))).toBe(true); + expect(isFilterAST(mergeFilterNodes(RULES))).toBe(true); + expect(isFilterAST(mergeFilterNodes(RULES, TUPLE))).toBe(true); + }); + + /** + * The issue's own repro, byte-for-byte: the showcase ships + * `showcase_task.in_progress` with this exact `filter`, and it is what the + * `/meta/view` response hands ObjectView today. + */ + it('lowers the shipped saved-view filter into the payload the backend answered 200 for', () => { + const shipped = [{ field: 'status', operator: 'equals', value: 'in_progress' }]; + // Before the fold this was the wire payload, and the server refused it. + expect(isFilterAST(shipped)).toBe(false); + expect(parseFilterAST(shipped)).toBeUndefined(); + // After: the payload a live backend answered with total=2. + const sent = mergeFilterNodes(shipped); + expect(sent).toEqual([['status', 'equals', 'in_progress']]); + expect(isFilterAST(sent)).toBe(true); + expect(parseFilterAST(sent)).toEqual({ status: 'in_progress' }); + }); + + it('a rule source survives the round trip to a real predicate', () => { + expect(parseFilterAST(mergeFilterNodes(RULES))).toEqual({ stage: 'won' }); + expect(parseFilterAST(mergeFilterNodes(RULES, TUPLE))) + .toEqual({ $and: [{ stage: 'won' }, { owner: 'me' }] }); }); it('an object source survives the round trip to a real predicate', () => { diff --git a/packages/core/src/utils/filter-converter.ts b/packages/core/src/utils/filter-converter.ts index 8f1352a427..2f69f27c13 100644 --- a/packages/core/src/utils/filter-converter.ts +++ b/packages/core/src/utils/filter-converter.ts @@ -8,11 +8,13 @@ /** * Filter Converter Utilities - * + * * Shared utilities for converting MongoDB-like filter operators * to ObjectStack FilterNode AST format. */ +import { normalizeFilterOperator } from '@objectstack/spec/ui'; + /** * FilterNode AST type definition * Represents a filter condition or a logical combination of conditions @@ -178,6 +180,60 @@ export function convertFiltersToAST(filter: Record): FilterNode | R return ['and', ...conditions]; } +/** + * A spec `ViewFilterRule` as it arrives from stored view metadata. + * + * Structurally recognisable and NOT guessed at: every AST node is an ARRAY, a + * rule is a plain OBJECT. The two can never be confused, so this predicate is + * exact rather than heuristic. + * + * A blank `field` is deliberately NOT a rule — same predicate the write side + * uses to drop the row `Add filter` inserts before a column is picked. Lowering + * it would produce `['', op, value]`, which `isFilterAST` ACCEPTS and the + * server answers `200` with zero rows (measured) — a silently-empty list. Left + * unlowered it stays an object in AST position, which the server refuses with + * `400 INVALID_FILTER` naming the element. Loud beats silently-empty. + */ +interface ViewFilterRuleLike { + field: string; + operator?: unknown; + value?: unknown; +} + +function isViewFilterRule(value: unknown): value is ViewFilterRuleLike { + if (typeof value !== 'object' || value === null || Array.isArray(value)) return false; + const field = (value as { field?: unknown }).field; + return typeof field === 'string' && field !== ''; +} + +/** + * Lower ONE spec `ViewFilterRule` to an ObjectQL AST comparison node. + * + * The operator goes through the spec's OWN {@link normalizeFilterOperator} — + * the exact exit the WRITE side uses (`app-shell/views/viewFilterFold.ts`), so + * the two directions cannot drift into two dialects. No second canonical map is + * introduced, and none is needed: all 19 `VIEW_FILTER_OPERATORS` are already + * members of the wire's `VALID_AST_OPERATORS`, so the lowering is purely + * structural. An operator the spec does not know is passed through VERBATIM, so + * `isFilterAST` still refuses it and the server still answers `400 + * INVALID_FILTER` — a misspelling must not be coerced into a valid one. + * + * `value` is emitted only when the rule carries one. A rule that omits it + * (`is_empty` / `is_null`, whose direction comes from the operator NAME) would + * otherwise gain an invented `null` the author never wrote — `JSON.stringify` + * turns a hole in an array into `null`, and `['x', 'equals', null]` is a real + * `{x: null}` predicate, i.e. a silently-wrong filter. Same rule the write side + * applies (`if (c.value !== undefined)`). + */ +function viewFilterRuleToNode(rule: ViewFilterRuleLike): FilterNode { + const operator = normalizeFilterOperator(rule.operator as string); + return ( + rule.value === undefined + ? [rule.field, operator] + : [rule.field, operator, rule.value] + ) as FilterNode; +} + /** * Normalize ONE filter source into a single filter node. * @@ -193,12 +249,52 @@ export function convertFiltersToAST(filter: Record): FilterNode | R * `table.defaultFilters` (declared `Record`) was DROPPED and the * view returned every record. Silently: no error, just a wider answer. * + * The FIRST never worked at all (objectui#3431). The array branch returned + * every array VERBATIM, so a saved view's `ViewFilterRule[]` travelled to + * `$filter` as bare rule objects — which the server refuses: `isFilterAST` is + * false for an array of objects, and the wire face answers `400 + * INVALID_FILTER`. Verified against a real backend on the showcase's SHIPPED + * `showcase_task.in_progress` view (`filter: [{field:'status', + * operator:'equals', value:'in_progress'}]`): 400 as sent, 200 with its 2 rows + * once lowered to `[['status','equals','in_progress']]`. Every saved view + * carrying a filter was a failed list. `mergeFilterNodes` below has warned + * about exactly this hazard since it was written — the warning was accurate, + * and the sink it warns for never implemented the lowering it describes. + * + * **Why the fold lives HERE and not at the producer.** `ListViewSchema.filter` + * / `ViewTab.filter` are spec-declared `z.array(ViewFilterRuleSchema)`, and a + * view hands a renderer a `ListViewSchema` — so folding one hop earlier (in + * `app-shell`'s ObjectView, say) would write ObjectQL AST triples INTO a + * spec-declared rule-array slot: an off-spec value in a spec field, AGENTS.md + * #0.1 inverted. This function is the last hop before the wire, where the value + * legitimately leaves the spec's view vocabulary and becomes an ObjectQL AST. + * It is also the single sink both producers already share — `plugin-list`'s + * `buildEffectiveFilter` (which feeds the grid AND the export) and + * `plugin-view`'s ObjectView (calendar / kanban / gallery / timeline). One + * lowering, one place; the same reason the MongoDB-style shape is lowered here + * rather than at each of its callers. + * + * Mixed arrays fold ELEMENT-WISE, because that is what reaches this function in + * practice: `ObjectView` concatenates a saved view's rules with the + * `?filter[]=` URL triples into one array. Triples pass through + * untouched. + * * Returns `undefined` for an absent or empty source, so callers can skip * `$filter` rather than sending an empty array. */ export function toFilterNode(source: unknown): FilterNode | Record | undefined { if (source === null || source === undefined) return undefined; - if (Array.isArray(source)) return source.length > 0 ? (source as FilterNode) : undefined; + if (Array.isArray(source)) { + if (source.length === 0) return undefined; + // Spec `ViewFilterRule[]` (possibly mixed with AST nodes) → AST nodes. Left + // untouched when the array holds no rule objects, which is the common case: + // user-filter conditions and URL triples are already nodes. + return ( + source.some(isViewFilterRule) + ? source.map((el) => (isViewFilterRule(el) ? viewFilterRuleToNode(el) : el)) + : source + ) as FilterNode; + } if (typeof source !== 'object') return undefined; const obj = source as Record; if (Object.keys(obj).length === 0) return undefined; @@ -211,11 +307,15 @@ export function toFilterNode(source: unknown): FilterNode | Record * * Wrapping rather than spreading, on purpose. `['and', ...rules]` looks * equivalent and is not: spreading a `ViewFilterRule[]` puts bare rule OBJECTS - * where the AST expects nodes, and the server neither understands nor rejects - * that cleanly — `isFilterAST` says no (a 400 since objectstack#4121), while - * `parseFilterAST` reads the rule as a Mongo condition and filters on columns - * literally named `field` / `operator` / `value`. Spreading is only correct - * when the source happens to be an array of nodes, which is why it survived. + * where the AST expects nodes, and the server does not accept that — + * `isFilterAST` says no and the wire face answers `400 INVALID_FILTER` + * (objectstack#4121). Spreading is only correct when the source happens to be + * an array of nodes, which is why it survived. + * + * Since objectui#3431 the rule objects never reach that position anyway: + * `toFilterNode` lowers a `ViewFilterRule[]` to AST nodes on the way in. The + * paragraph above is kept because it is why each source stays its own child — + * and because it correctly diagnosed the bug the sink itself was carrying. * * Sources that normalize to nothing are skipped; one surviving source is * returned as-is rather than wrapped in a pointless `and`. diff --git a/packages/plugin-view/src/__tests__/ObjectView.filterSources.test.tsx b/packages/plugin-view/src/__tests__/ObjectView.filterSources.test.tsx index 0b1853de65..9a4c868fea 100644 --- a/packages/plugin-view/src/__tests__/ObjectView.filterSources.test.tsx +++ b/packages/plugin-view/src/__tests__/ObjectView.filterSources.test.tsx @@ -22,6 +22,12 @@ * puts bare rule objects where the AST expects nodes. Covered at the merge * level in core's `filter-source-merge.test.ts`, which pins what the server * does with the old shape. + * 3. Even UNSPREAD, a `ViewFilterRule[]` was never AST: it reached `$filter` + * as rule objects and the server answered `400 INVALID_FILTER` + * (objectui#3431). This file is one of the two producers that feed the + * shared `toFilterNode` sink — the other is `plugin-list`'s + * `buildEffectiveFilter` — which is why the lowering lives there and not + * in either caller. */ import { describe, it, expect, vi, beforeEach } from 'vitest'; @@ -82,10 +88,17 @@ describe('ObjectView carries every filter source into the query', () => { expect(await queriedFilter(find)).toEqual(['status', '=', 'active']); }); - it('keeps a ViewFilterRule[] table.defaultFilters', async () => { + it('LOWERS a ViewFilterRule[] table.defaultFilters into AST nodes', async () => { + // objectui#3431. This case used to assert `toEqual(rules)` — that the rule + // objects reached `$filter` VERBATIM — and it was green because that is + // what happened, not because it was right: the server refuses an array of + // rule objects with `400 INVALID_FILTER`, so this view queried nothing at + // all. `toFilterNode` now lowers the rules; `eq` canonicalises to `equals` + // through the spec's own `normalizeFilterOperator`, the same exit the + // write side uses. const rules = [{ field: 'stage', operator: 'eq', value: 'won' }]; const find = renderCalendar({ table: { defaultFilters: rules } as any }); - expect(await queriedFilter(find)).toEqual(rules); + expect(await queriedFilter(find)).toEqual([['stage', 'equals', 'won']]); }); it('keeps an AST-shaped source', async () => {