fix(core): 已保存视图的 ViewFilterRule[] 在进 $filter 前折成 AST (#3431)

.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[<field>]=<value>` 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`.

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

packages/core/src/utils/__tests__/filter-source-merge.test.ts

@@ -25,19 +25,46 @@
  *
  * 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';
 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[<field>]=<value>` 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', () => {