diff --git a/.changeset/user-filters-tab-preset-operators-single-exit.md b/.changeset/user-filters-tab-preset-operators-single-exit.md new file mode 100644 index 000000000..748e426b9 --- /dev/null +++ b/.changeset/user-filters-tab-preset-operators-single-exit.md @@ -0,0 +1,9 @@ +--- +'@object-ui/plugin-list': patch +--- + +`UserFilters` no longer carries its own operator table when it lowers a `ViewTab.filter` preset into an ObjectQL AST node. The private `specOperatorToAst` was the second hand-kept operator map in this package and it had drifted: it lowered `not_in` — the spec's OWN canonical spelling — and the legacy `nin` to the spaced `'not in'`, a spelling that appears in no spec vocabulary. `isFilterAST` refuses it, so clicking such a tab produced an empty list plus `400 INVALID_FILTER`. Measured against a real backend (published `@objectstack/*@17.0.0-rc.2` + app-showcase, on `showcase_task`): `$filter=[["status","not in",["done"]]]` returned `400 INVALID_FILTER`, while `[["status","not_in",["done"]]]` returned `200` with the same 8 records as the `["status","!=","done"]` baseline. + +Lowering is now purely structural — all 19 `VIEW_FILTER_OPERATORS` are already members of the wire's `VALID_AST_OPERATORS`, so nothing needs translating — with the spec's own `normalizeFilterOperator` as the single exit for the legacy spellings stored metadata still carries (`gt`, `eq`, `nin`, `notEquals`, …). That is the same exit the write side (`viewFilterFold`) and the saved-view fold in `@object-ui/core` already use, so the directions cannot drift into two dialects. An operator the spec does not know is passed through verbatim, so a misspelling still earns a loud `400` rather than being coerced into a valid filter. + +`before` and `after` are now passed through rather than rewritten to `<` and `>`. That was the one judgement call, and it was settled by measurement rather than assumption: on the same live backend the word and the symbol return identical status and identical record ids, on a `date` field and on a `datetime` field, in both directions — so the rewrite was a no-op and dropping it is a pure fix. The remaining 18 canonical operators were measured the same way and are likewise unchanged in what the server answers; `not_in` is the only one whose answer changes, from `400` to the correct rows. Tab presets given in the legacy already-lowered `filters: triplet[]` shape are untouched, as before. diff --git a/packages/plugin-list/src/UserFilters.tsx b/packages/plugin-list/src/UserFilters.tsx index 809492680..19e220c04 100644 --- a/packages/plugin-list/src/UserFilters.tsx +++ b/packages/plugin-list/src/UserFilters.tsx @@ -10,6 +10,7 @@ import * as React from 'react'; import { cn, Button, Popover, PopoverContent, PopoverTrigger, LookupValuePicker } from '@object-ui/components'; import { ChevronDown, X, Plus } from 'lucide-react'; import type { ListViewSchema } from '@object-ui/types'; +import { normalizeFilterOperator } from '@objectstack/spec/ui'; import { useSafeFieldLabel, useObjectTranslation } from '@object-ui/i18n'; function useMoreLabel(): string { @@ -96,31 +97,63 @@ export interface UserFiltersProps { onSelectionsChange?: (selections: Record>) => void; } -/** - * Map @objectstack/spec ViewFilterRule operators to ObjectQL AST operators. - * Accepts the canonical vocabulary (`greater_than`, `not_equals`, `before`, …) - * that the Studio filter builder now writes, plus legacy shorthand spellings - * (`gt`, `eq`, `nin`) still present in already-stored view metadata. - */ -function specOperatorToAst(op: string | undefined): string { - switch (op) { - case undefined: case 'equals': case 'eq': return '='; - case 'not_equals': case 'ne': case 'neq': return '!='; - case 'greater_than_or_equal': case 'gte': return '>='; - case 'less_than_or_equal': case 'lte': return '<='; - case 'greater_than': case 'gt': case 'after': return '>'; - case 'less_than': case 'lt': case 'before': return '<'; - case 'not_contains': return 'notcontains'; - case 'starts_with': return 'startswith'; - case 'not_in': case 'nin': return 'not in'; - default: return op; - } -} - /** * Normalize tab presets to the client shape. Accepts both: * - @objectstack/spec ViewTab: `{ name, label, filter: ViewFilterRule[], isDefault }` * - legacy client shape: `{ id, label, filters: triplet[], default }` + * + * **The spec rule → AST lowering is purely structural**, and deliberately owns + * NO operator table of its own (#3470). All 19 `VIEW_FILTER_OPERATORS` are + * already members of the wire's `VALID_AST_OPERATORS`, so nothing needs + * translating; only the legacy spellings stored view metadata still carries + * (`gt`, `eq`, `nin`, `notEquals`, …) need folding onto the canonical word, and + * that is exactly what the spec's OWN {@link normalizeFilterOperator} does — + * the same single exit the WRITE side uses (`app-shell/views/viewFilterFold.ts`) + * and the same one `@object-ui/core`'s `viewFilterRuleToNode` uses for a saved + * view's `filter` (#3431). One exit, so the directions cannot drift apart. + * + * The private table this replaced was the second hand-kept operator map in this + * package, and it had drifted: it lowered `not_in`/`nin` to the SPACED + * `'not in'`, which is in no spec vocabulary — `isFilterAST()` refuses it and + * the wire answers `400 INVALID_FILTER`. Measured against a real backend + * (published `@objectstack/*@17.0.0-rc.2` + app-showcase, `showcase_task`): + * `[["status","not in",["done"]]]` → **400 INVALID_FILTER**; + * `[["status","not_in",["done"]]]` → **200, 8 rows** (same rows as the + * `["status","!=","done"]` baseline). Every other operator the old table + * rewrote was measured to be a no-op on the answer — see below. + * + * **`before`/`after` are passed through, not mapped** (the one judgement call + * this change had to make: the old table rewrote them to `<`/`>`, and both + * words are themselves `VALID_AST_OPERATORS` members). Measured on the same + * backend, on a `date` field and a `datetime` field, both directions — + * identical status AND identical record ids: + * + * `["due_date","before","2026-08-01"]` → 200, 2 rows + * `["due_date","<","2026-08-01"]` → 200, the SAME 2 rows + * `["due_date","after","2026-08-01"]` → 200, 8 rows + * `["due_date",">","2026-08-01"]` → 200, the SAME 8 rows + * `["created_at","before","2026-08-01T00:00:00.000Z"]` → 200, 6 rows (`<` idem) + * `["created_at","after","2026-08-01T00:00:00.000Z"]` → 200, 3 rows (`>` idem) + * + * So dropping that rewrite is a pure fix, not a behaviour change. (The spec + * agrees independently: `canonicalAstOperator('before')` is `'<'`.) + * + * An operator the spec does not know is passed through **verbatim**, so + * `isFilterAST()` still refuses it and the server still answers a loud `400`. + * A misspelling must never be coerced into a valid operator (AGENTS.md #0.1): + * silently reading `bfore` as `before` would return a plausible-looking wrong + * record set instead of an error the author can see. + * + * That applies to a rule which OMITS the operator too, and it is the one place + * this is a deliberate behaviour change rather than a pure fix. The deleted + * table opened with `case undefined: … return '='`, inventing an equality + * predicate for a rule that has no operator at all. `ViewFilterRuleSchema` + * REQUIRES `operator` (it is a bare `z.enum`, no default — an operator-less rule + * fails `safeParse` with `invalid_value`), so such a rule is off-spec metadata + * that publish validation refuses; silently answering it with `field = value` + * was a lenient consumer standing in for the contract. It now lowers to + * `[field, undefined, value]`, which `isFilterAST()` refuses — the same loud + * `400` every other off-spec spelling gets. */ function normalizeTabPresets(tabs: any[]): Array<{ id: string; label: string; filters: any[]; default?: boolean }> { return (tabs || []) @@ -133,7 +166,7 @@ function normalizeTabPresets(tabs: any[]): Array<{ id: string; label: string; fi : (Array.isArray(t.filter) ? t.filter .filter((r: any) => r && typeof r.field === 'string') - .map((r: any) => [r.field, specOperatorToAst(r.operator), r.value]) + .map((r: any) => [r.field, normalizeFilterOperator(r.operator), r.value]) : []), default: t.default ?? t.isDefault, })); diff --git a/packages/plugin-list/src/__tests__/UserFilters.tabPresetOperators.test.tsx b/packages/plugin-list/src/__tests__/UserFilters.tabPresetOperators.test.tsx new file mode 100644 index 000000000..ad9bd1ac2 --- /dev/null +++ b/packages/plugin-list/src/__tests__/UserFilters.tabPresetOperators.test.tsx @@ -0,0 +1,198 @@ +/** + * 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. + */ + +/** + * Tab-preset operator lowering: `ViewTab.filter` → ObjectQL AST (#3470). + * + * `UserFilters` used to carry its OWN operator table (`specOperatorToAst`) — + * the second hand-kept operator map in this package — and it had drifted: it + * lowered `not_in`/`nin` to the SPACED `'not in'`, a spelling that appears in + * no spec vocabulary. `isFilterAST()` refuses it and the wire answers `400 + * INVALID_FILTER`, so a tab preset written with the spec's own canonical + * spelling produced an empty list plus an error the moment it was clicked. + * + * The table is gone. Lowering is now purely structural — all 19 + * `VIEW_FILTER_OPERATORS` are already `VALID_AST_OPERATORS` members — with the + * spec's OWN `normalizeFilterOperator` as the single exit, exactly as the write + * side (`app-shell/views/viewFilterFold.ts`) and `@object-ui/core`'s saved-view + * fold (#3431) do. + * + * These assertions pin the acceptance fact OFFLINE, using the spec's own + * `isFilterAST` — the very predicate that gates the filter server-side — so the + * fix cannot regress without a live backend in the loop. The corresponding LIVE + * measurement (published `@objectstack/*@17.0.0-rc.2` + app-showcase) is + * recorded on `normalizeTabPresets` and in the PR: `'not in'` → 400, + * `not_in` → 200 with the 8 rows the `!=` baseline returns, and `before`/`after` + * → byte-identical answers to `<`/`>` on both a `date` and a `datetime` field. + */ + +import * as React from 'react'; +import { describe, it, expect, vi } from 'vitest'; +import { render } from '@testing-library/react'; +import { isFilterAST, VALID_AST_OPERATORS } from '@objectstack/spec/data'; +import { + VIEW_FILTER_OPERATORS, + VIEW_FILTER_OPERATOR_ALIASES, + ViewFilterRuleSchema, + normalizeFilterOperator, +} from '@objectstack/spec/ui'; +import { UserFilters } from '../UserFilters'; + +/** + * Render a tabs-mode `UserFilters` whose single preset is the given spec + * `ViewFilterRule`, and return the AST node it emits. + * + * The preset is the DEFAULT tab, so `TabFilters`' mount effect emits it without + * a click — the same path a user takes when a view opens on its default tab, + * and the path that shipped the 400. + */ +function emitFor(rule: Record): unknown[] { + const onFilterChange = vi.fn(); + render( + , + ); + expect(onFilterChange).toHaveBeenCalledTimes(1); + return onFilterChange.mock.calls[0][0]; +} + +describe('UserFilters tab presets — the reported defect (#3470)', () => { + it('lowers a canonical `not_in` preset to a node isFilterAST ACCEPTS', () => { + // BEFORE this fix: emitted `['status', 'not in', ['done']]` — isFilterAST + // false, 400 INVALID_FILTER measured against a real backend. This is the + // assertion that flips red→green. + const node = emitFor({ field: 'status', operator: 'not_in', value: ['done'] }); + expect(node).toEqual([['status', 'not_in', ['done']]]); + expect( + isFilterAST(node), + `a not_in tab preset produced ${JSON.stringify(node)}, which isFilterAST() rejects — ` + + 'the server answers 400 INVALID_FILTER and the tab shows an empty list', + ).toBe(true); + }); + + it('folds the legacy `nin` spelling onto the canonical `not_in`', () => { + // Stored view metadata carries the shorthand: saveMeta persists the authored + // body verbatim, so the spec's own z.preprocess never reaches the row. + const node = emitFor({ field: 'status', operator: 'nin', value: ['done'] }); + expect(node).toEqual([['status', 'not_in', ['done']]]); + expect(isFilterAST(node)).toBe(true); + }); + + it('never emits the spaced `not in`, which no spec vocabulary defines', () => { + for (const spelling of ['not_in', 'nin', 'notin', 'notIn']) { + const [[, operator]] = emitFor({ field: 'status', operator: spelling, value: ['done'] }) as [ + [string, string, unknown], + ]; + expect(operator, `authored '${spelling}' lowered to '${operator}'`).not.toBe('not in'); + expect(operator).toBe('not_in'); + } + }); +}); + +describe('UserFilters tab presets — lowering is structural, not translated', () => { + it('reads both vocabularies from the spec', () => { + // Guards every it.each below against silently passing on an empty list. + expect(VIEW_FILTER_OPERATORS.length).toBe(19); + expect(VALID_AST_OPERATORS.size).toBeGreaterThan(0); + }); + + it('needs no operator table at all: every view operator is already AST-valid', () => { + // This is WHY the private table could be deleted rather than repaired. + expect(VIEW_FILTER_OPERATORS.filter((op) => !VALID_AST_OPERATORS.has(op))).toEqual([]); + }); + + it.each(VIEW_FILTER_OPERATORS)('a `%s` preset survives the isFilterAST gate', (op) => { + const value = op === 'in' || op === 'not_in' ? ['a', 'b'] : op === 'between' ? [1, 2] : 'x'; + const node = emitFor({ field: 'some_field', operator: op, value }); + expect( + isFilterAST(node), + `a '${op}' tab preset produced ${JSON.stringify(node)}, which isFilterAST() rejects`, + ).toBe(true); + }); + + it.each(Object.keys(VIEW_FILTER_OPERATOR_ALIASES))( + 'the legacy spelling `%s` lowers through the spec, not a local table', + (alias) => { + const [[, operator]] = emitFor({ field: 'f', operator: alias, value: ['a', 'b'] }) as [ + [string, string, unknown], + ]; + // The single-exit pin: whatever the spec says, verbatim. A second table + // reintroduced here fails this the moment it disagrees by one spelling. + expect(operator).toBe(normalizeFilterOperator(alias)); + expect(VALID_AST_OPERATORS.has(operator)).toBe(true); + }, + ); + + it('passes `before`/`after` through instead of rewriting them to `<`/`>`', () => { + // The one judgement call in #3470, settled by MEASUREMENT rather than + // assumption: on the live rc.2 backend the word and the symbol return + // identical status and identical record ids, on a `date` field and on a + // `datetime` field, in both directions. So the rewrite was a no-op and + // dropping it is a pure fix. (`canonicalAstOperator('before') === '<'` + // in the spec says the same thing independently.) + expect(emitFor({ field: 'due_date', operator: 'before', value: '2026-08-01' })) + .toEqual([['due_date', 'before', '2026-08-01']]); + expect(emitFor({ field: 'due_date', operator: 'after', value: '2026-08-01' })) + .toEqual([['due_date', 'after', '2026-08-01']]); + }); + + it('passes an unknown spelling through VERBATIM so the server rejects it loudly', () => { + // Direction note (honest reverse-verification): this one was GREEN BEFORE + // the change too — the deleted table's `default:` branch also passed + // unknowns through. It is here to pin the CONTRACT, not to flip: coercing + // `bfore` to `before` would answer a plausible-looking wrong record set + // instead of the 400 that tells the author their metadata is misspelled. + const node = emitFor({ field: 'due_date', operator: 'bfore', value: '2026-08-01' }); + expect(node).toEqual([['due_date', 'bfore', '2026-08-01']]); + expect(isFilterAST(node)).toBe(false); + }); + + it('refuses to invent `=` for a rule that omits the operator', () => { + // The ONE deliberate behaviour change in #3470, and the only assertion here + // whose "before" was a passing 200 rather than a 400. The deleted table + // opened with `case undefined: … return '='`, so `{field, value}` with no + // operator lowered to `['status','=','x']` — accepted by isFilterAST, and + // answered by the server as a real equality predicate. + // + // `ViewFilterRuleSchema` REQUIRES `operator` (bare z.enum, no default), so + // that rule is off-spec metadata publish validation refuses. Inventing an + // answer for it was a lenient consumer standing in for the contract + // (AGENTS.md #0.1); it now earns the same loud 400 as any other off-spec + // spelling. + expect(ViewFilterRuleSchema.safeParse({ field: 'status', value: 'x' }).success).toBe(false); + + const node = emitFor({ field: 'status', value: 'x' }); + expect(isFilterAST(node)).toBe(false); + // What the old table would have produced, for contrast — accepted, and so + // silently answered as `status = 'x'`. + expect(isFilterAST([['status', '=', 'x']])).toBe(true); + }); + + it('leaves an already-lowered legacy `filters` triplet list untouched', () => { + // The other accepted tab shape (`{ id, label, filters: triplet[] }`) never + // went through the operator table and must not start now. + const onFilterChange = vi.fn(); + render( + , + ); + expect(onFilterChange).toHaveBeenCalledWith([['status', '=', 'active']]); + }); +}); diff --git a/packages/plugin-list/src/__tests__/UserFilters.test.tsx b/packages/plugin-list/src/__tests__/UserFilters.test.tsx index 6da68f60d..c1d8a70bc 100644 --- a/packages/plugin-list/src/__tests__/UserFilters.test.tsx +++ b/packages/plugin-list/src/__tests__/UserFilters.test.tsx @@ -114,8 +114,12 @@ describe('UserFilters — selection persistence (ADR-0047)', () => { />, ); - // Restored tab wins over the isDefault tab and emits its preset filter - expect(onFilterChange).toHaveBeenCalledWith([['priority', '=', 'urgent']]); + // Restored tab wins over the isDefault tab and emits its preset filter. + // The operator stays the spec's canonical `equals` — lowering a rule to an + // AST node is structural and translates nothing (#3470); `equals` is itself + // a `VALID_AST_OPERATORS` member, measured 200 on a live backend. Operator + // coverage proper lives in `UserFilters.tabPresetOperators.test.tsx`. + expect(onFilterChange).toHaveBeenCalledWith([['priority', 'equals', 'urgent']]); }); it('reports tab switches through onSelectionsChange', () => {