diff --git a/.changeset/view-operator-builder-parity.md b/.changeset/view-operator-builder-parity.md new file mode 100644 index 0000000000..2f467aadc1 --- /dev/null +++ b/.changeset/view-operator-builder-parity.md @@ -0,0 +1,43 @@ +--- +"@object-ui/components": minor +"@object-ui/plugin-view": patch +--- + +fix(view,components): the spec→FilterBuilder operator table covers the whole view vocabulary, and the dead write direction is gone + +`view-config-utils`' `SPEC_TO_BUILDER_OP` resolved **10 of the spec's 19 +canonical `VIEW_FILTER_OPERATORS`**. The nine it missed — +`not_equals`, `starts_with`, `ends_with`, `greater_than`, `less_than`, +`greater_than_or_equal`, `less_than_or_equal`, `is_null`, `is_not_null` — all +appear in stored view metadata (they are canonical; `ViewFilterRuleSchema` +validates against exactly this list), and each reached the FilterBuilder as a +raw spelling its operator dropdown cannot select. + +Same defect and same cause as #2974, one table over: spellings were enumerated +by hand. That table is now derived from the spec's own canonical list and +`VIEW_FILTER_OPERATOR_ALIASES`, matched case- and separator-insensitively, so +`not_in` / `notIn` / `'not in'` / `NOT_IN` are one entry rather than four +chances to miss one. + +Four canonical operators have no FilterBuilder equivalent — +`starts_with`/`ends_with` (absent from its vocabulary) and `is_null`/ +`is_not_null` (distinct from the `is_empty`/`is_not_empty` it does have). They +are recorded as explicit `null`s and asserted, and deliberately left unmapped: +folding them onto a near-equivalent would silently rewrite the author's +operator on the next save, whereas an unmapped operator surfaces as a condition +row the author must complete. + +Also retired `BUILDER_TO_SPEC_OP` and `toSpecFilter` — the write direction, +dead since the legacy `buildViewConfigSchema` engine was replaced by the +studio's spec-driven inspector (no caller anywhere in the repo, and not part of +`@object-ui/plugin-view`'s public exports). It was objectui's last emitter of +`'not in'` with a space, plus `before`/`after`, as *filter-AST* operators — +spellings that reached the server outside `VALID_AST_OPERATORS` and were dropped +without an error (objectstack-ai/objectstack#3948). + +`@object-ui/components` now exports `FILTER_BUILDER_OPERATORS` (and the +`FilterBuilderOperator` type), derived from the operators the FilterBuilder +actually renders, so tables mapping onto that vocabulary can assert against it +instead of restating it. + +Refs objectstack-ai/objectui#2945, #2901. diff --git a/packages/components/src/custom/filter-builder.tsx b/packages/components/src/custom/filter-builder.tsx index 9aa1dc4f80..3cb3026b6f 100644 --- a/packages/components/src/custom/filter-builder.tsx +++ b/packages/components/src/custom/filter-builder.tsx @@ -71,7 +71,23 @@ const defaultOperators = [ { value: "between", label: "Between" }, { value: "in", label: "In" }, { value: "notIn", label: "Not in" }, -] +] as const + +/** + * The FilterBuilder's own operator vocabulary — every id its dropdown can + * hold, derived from the operators it actually renders. + * + * Exported because several translation tables map an external vocabulary + * (the spec's `VIEW_FILTER_OPERATORS`, Mongo `$`-tokens) *onto* this one, and + * a value that is not in this list produces a condition row whose operator + * select has nothing to select. Those tables assert against this export + * rather than restating it, so adding an operator here is the only edit + * needed to widen them. + */ +export const FILTER_BUILDER_OPERATORS = defaultOperators.map(o => o.value) + +/** An operator id the FilterBuilder can render. */ +export type FilterBuilderOperator = (typeof defaultOperators)[number]['value'] const useSafeFilterTranslation = createSafeTranslation( { diff --git a/packages/plugin-view/src/config/__tests__/view-operator-builder-parity.test.ts b/packages/plugin-view/src/config/__tests__/view-operator-builder-parity.test.ts new file mode 100644 index 0000000000..813f472f55 --- /dev/null +++ b/packages/plugin-view/src/config/__tests__/view-operator-builder-parity.test.ts @@ -0,0 +1,117 @@ +/** + * 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 → FilterBuilder operator parity (#2901, #2945). + * + * The third of objectui's spec-operator translation tables. The other two — + * `ListView.mapOperator` and `data-objectstack`'s `normalizeFilterOperator` — + * were pinned to the spec in #2974, which found eight spellings they had missed + * by enumerating instead of deriving. This one maps the same vocabulary onto the + * FilterBuilder's operator ids, and it had missed nine: + * + * not_equals, greater_than, less_than, greater_than_or_equal, + * less_than_or_equal, starts_with, ends_with, is_null, is_not_null + * + * All nine are canonical `VIEW_FILTER_OPERATORS` members, so a stored view + * legitimately carries them, and all nine reached the builder as a raw spelling + * its dropdown cannot select. Five now map; four are recorded as deliberate + * gaps, asserted below so the list cannot grow silently. + */ +import { describe, it, expect } from 'vitest'; +import { VIEW_FILTER_OPERATORS, VIEW_FILTER_OPERATOR_ALIASES } from '@objectstack/spec/ui'; +import { FILTER_BUILDER_OPERATORS } from '@object-ui/components'; +import { specToBuilderOperator, __CANONICAL_TO_BUILDER } from '../view-config-utils'; + +/** + * Canonical view operators the FilterBuilder cannot express. + * + * `starts_with`/`ends_with`: no such operator in the builder's vocabulary. + * `is_null`/`is_not_null`: distinct from `is_empty`/`is_not_empty`, which the + * builder does have — folding them together would rewrite the author's operator + * on the next save. + * + * Shrink this list by adding the operator to the FilterBuilder, never by + * mapping onto a near-equivalent. + */ +const NO_BUILDER_EQUIVALENT = ['starts_with', 'ends_with', 'is_null', 'is_not_null']; + +describe('CANONICAL_TO_BUILDER', () => { + it('covers every canonical view operator, and nothing else', () => { + expect(Object.keys(__CANONICAL_TO_BUILDER).sort()).toEqual([...VIEW_FILTER_OPERATORS].sort()); + }); + + it('maps onto operator ids the FilterBuilder actually renders', () => { + const builderIds = new Set(FILTER_BUILDER_OPERATORS); + const notRenderable = Object.entries(__CANONICAL_TO_BUILDER) + .filter(([, id]) => id !== null && !builderIds.has(id as never)) + .map(([op, id]) => `${op} -> ${id}`); + expect(notRenderable).toEqual([]); + }); + + it('records exactly the documented gaps', () => { + const unmapped = Object.entries(__CANONICAL_TO_BUILDER) + .filter(([, id]) => id === null) + .map(([op]) => op); + expect(unmapped.sort()).toEqual([...NO_BUILDER_EQUIVALENT].sort()); + }); +}); + +describe('specToBuilderOperator', () => { + it('resolves every canonical view operator that has an equivalent', () => { + const builderIds = new Set(FILTER_BUILDER_OPERATORS); + const unresolved = VIEW_FILTER_OPERATORS + .filter(op => !NO_BUILDER_EQUIVALENT.includes(op)) + .filter(op => !builderIds.has(specToBuilderOperator(op) as never)); + expect(unresolved).toEqual([]); + }); + + it('resolves every legacy alias the spec still folds', () => { + const builderIds = new Set(FILTER_BUILDER_OPERATORS); + const unresolved = Object.entries(VIEW_FILTER_OPERATOR_ALIASES) + .filter(([, canonical]) => !NO_BUILDER_EQUIVALENT.includes(canonical)) + .filter(([alias]) => !builderIds.has(specToBuilderOperator(alias) as never)) + .map(([alias]) => alias); + expect(unresolved).toEqual([]); + }); + + it('resolves the infix spellings a stored filter array carries', () => { + expect(specToBuilderOperator('=')).toBe('equals'); + expect(specToBuilderOperator('==')).toBe('equals'); + expect(specToBuilderOperator('!=')).toBe('notEquals'); + expect(specToBuilderOperator('<>')).toBe('notEquals'); + expect(specToBuilderOperator('>')).toBe('greaterThan'); + expect(specToBuilderOperator('<')).toBe('lessThan'); + expect(specToBuilderOperator('>=')).toBe('greaterOrEqual'); + expect(specToBuilderOperator('<=')).toBe('lessOrEqual'); + expect(specToBuilderOperator('nin')).toBe('notIn'); + expect(specToBuilderOperator('like')).toBe('contains'); + }); + + it('folds case and separators, so one spelling class is one entry', () => { + for (const spelling of ['not_in', 'notIn', 'not in', 'NOT_IN', 'not-in', 'notin']) { + expect(specToBuilderOperator(spelling)).toBe('notIn'); + } + for (const spelling of ['greater_than_or_equal', 'greaterThanOrEqual', 'greaterorequal']) { + expect(specToBuilderOperator(spelling)).toBe('greaterOrEqual'); + } + }); + + it('returns an unmapped operator unchanged rather than coercing it', () => { + // Visible in the UI as an incomplete condition row beats a silent rewrite + // to `equals`, which would drop the author's predicate on the next save. + expect(specToBuilderOperator('is_null')).toBe('is_null'); + expect(specToBuilderOperator('starts_with')).toBe('starts_with'); + expect(specToBuilderOperator('totally_made_up')).toBe('totally_made_up'); + }); + + it('defaults an absent operator to equals', () => { + expect(specToBuilderOperator('')).toBe('equals'); + expect(specToBuilderOperator(undefined as unknown as string)).toBe('equals'); + }); +}); diff --git a/packages/plugin-view/src/config/view-config-utils.ts b/packages/plugin-view/src/config/view-config-utils.ts index fb7c863af2..4c5edb0f85 100644 --- a/packages/plugin-view/src/config/view-config-utils.ts +++ b/packages/plugin-view/src/config/view-config-utils.ts @@ -6,61 +6,109 @@ */ import type { FilterGroup, SortItem } from '@object-ui/components'; +import { VIEW_FILTER_OPERATORS, VIEW_FILTER_OPERATOR_ALIASES } from '@objectstack/spec/ui'; +import type { ViewFilterOperator } from '@objectstack/spec/ui'; // --------------------------------------------------------------------------- -// Operator mapping: @objectstack/spec ↔ FilterBuilder +// Operator mapping: @objectstack/spec → FilterBuilder // --------------------------------------------------------------------------- +// +// One direction only. A stored view filter is *read* into a FilterBuilder here; +// the write direction was retired with the legacy `buildViewConfigSchema` +// engine, and the studio's spec-driven inspector persists the authored body +// itself. The retired table was also objectui's last emitter of `'not in'` +// (with a space), `before` and `after` as filter-AST operators — spellings the +// server used to drop silently (#2901, objectstack#3948). -export const SPEC_TO_BUILDER_OP: Record = { - '=': 'equals', - '==': 'equals', - '!=': 'notEquals', - '<>': 'notEquals', - '>': 'greaterThan', - '<': 'lessThan', - '>=': 'greaterOrEqual', - '<=': 'lessOrEqual', +/** + * Every canonical `VIEW_FILTER_OPERATORS` member, mapped onto the operator id + * the FilterBuilder renders — `null` where the builder has no equivalent. + * + * Total by construction: the type is keyed by `ViewFilterOperator`, so an + * operator added to the spec's view vocabulary fails to compile here instead + * of reaching the builder as a raw spelling its dropdown cannot select. + * + * The four `null`s are honest gaps, not oversights. `starts_with`/`ends_with` + * have no FilterBuilder operator at all, and `is_null`/`is_not_null` carry a + * NULL/empty distinction that `isEmpty`/`isNotEmpty` erase — folding them onto + * the near-equivalent would silently rewrite the author's operator the next + * time the view was saved. An unmapped operator instead reaches the builder + * unchanged and shows up as a condition row the author must complete, which is + * the failure we want: visible, and lossless until they touch it. + */ +const CANONICAL_TO_BUILDER: Record = { + 'equals': 'equals', + 'not_equals': 'notEquals', 'contains': 'contains', 'not_contains': 'notContains', - 'is_empty': 'isEmpty', - 'is_not_empty': 'isNotEmpty', + 'starts_with': null, + 'ends_with': null, + 'greater_than': 'greaterThan', + 'less_than': 'lessThan', + 'greater_than_or_equal': 'greaterOrEqual', + 'less_than_or_equal': 'lessOrEqual', 'in': 'in', 'not_in': 'notIn', - 'not in': 'notIn', + 'is_empty': 'isEmpty', + 'is_not_empty': 'isNotEmpty', + 'is_null': null, + 'is_not_null': null, 'before': 'before', 'after': 'after', 'between': 'between', - // Pass-through for already-normalized IDs - 'equals': 'equals', - 'notEquals': 'notEquals', - 'greaterThan': 'greaterThan', - 'lessThan': 'lessThan', - 'greaterOrEqual': 'greaterOrEqual', - 'lessOrEqual': 'lessOrEqual', - 'notContains': 'notContains', - 'isEmpty': 'isEmpty', - 'isNotEmpty': 'isNotEmpty', - 'notIn': 'notIn', }; -export const BUILDER_TO_SPEC_OP: Record = { - 'equals': '=', - 'notEquals': '!=', - 'greaterThan': '>', - 'lessThan': '<', - 'greaterOrEqual': '>=', - 'lessOrEqual': '<=', - 'contains': 'contains', - 'notContains': 'not_contains', - 'isEmpty': 'is_empty', - 'isNotEmpty': 'is_not_empty', - 'in': 'in', - 'notIn': 'not in', - 'before': 'before', - 'after': 'after', - 'between': 'between', +/** + * Infix and short spellings a stored *filter array* carries instead of a view + * spelling — `['amount', '>', 100]`. These are `AST_OPERATOR_MAP` keys + * (`data/filter.zod.ts`) that case-folding alone cannot reach. + */ +const INFIX_TO_CANONICAL: Record = { + '=': 'equals', + '==': 'equals', + '!=': 'not_equals', + '<>': 'not_equals', + '>': 'greater_than', + '<': 'less_than', + '>=': 'greater_than_or_equal', + '<=': 'less_than_or_equal', + 'nin': 'not_in', + 'like': 'contains', }; +/** Case- and separator-insensitive key: `not_in`, `notIn`, `'not in'` → `notin`. */ +const fold = (op: string) => op.toLowerCase().replace(/[\s_-]+/g, ''); + +/** + * Every spelling the spec itself recognises, folded onto its canonical member. + * + * Derived from the spec's own canonical list and legacy-alias table rather than + * restated, so the ~30 aliases `VIEW_FILTER_OPERATOR_ALIASES` still folds — all + * of them live in stored metadata, because `saveMeta` persists the authored body + * verbatim — are covered without this file enumerating them. Hand-enumerating + * spellings is what left `before`/`after` unmapped in the first place. + */ +const FOLDED_TO_CANONICAL: Map = new Map([ + ...VIEW_FILTER_OPERATORS.map(op => [fold(op), op] as const), + ...Object.entries(VIEW_FILTER_OPERATOR_ALIASES).map(([alias, op]) => [fold(alias), op] as const), +]); + +/** + * Resolve any stored operator spelling to a FilterBuilder operator id. + * + * Returns the input unchanged when no mapping exists, so an unrecognised + * operator is visible in the UI rather than silently coerced to `equals`. + */ +export function specToBuilderOperator(op: string): string { + const raw = String(op ?? '').trim(); + if (!raw) return 'equals'; + const canonical = INFIX_TO_CANONICAL[raw.toLowerCase()] ?? FOLDED_TO_CANONICAL.get(fold(raw)); + return (canonical ? CANONICAL_TO_BUILDER[canonical] : null) ?? raw; +} + +/** Exported for the parity guard — the mapping's canonical half. */ +export const __CANONICAL_TO_BUILDER = CANONICAL_TO_BUILDER; + // --------------------------------------------------------------------------- // Field type normalization: ObjectUI → FilterBuilder // --------------------------------------------------------------------------- @@ -92,7 +140,7 @@ function parseTriplet(arr: any[]): { id: string; field: string; operator: string return { id: crypto.randomUUID(), field, - operator: SPEC_TO_BUILDER_OP[op] || op, + operator: specToBuilderOperator(op), value: value ?? '', }; } @@ -106,7 +154,7 @@ function parseSingleOrNested(item: any): Array<{ id: string; field: string; oper return [{ id: item.id || crypto.randomUUID(), field: item.field, - operator: SPEC_TO_BUILDER_OP[item.operator] || item.operator || 'equals', + operator: specToBuilderOperator(item.operator), value: item.value ?? '', }]; } @@ -146,20 +194,6 @@ export function parseSpecFilter(raw: any): { logic: 'and' | 'or'; conditions: Ar return { logic: 'and', conditions: cond ? [cond] : [] }; } -/** - * Convert FilterGroup conditions back to spec-style filter array. - */ -export function toSpecFilter(logic: 'and' | 'or', conditions: Array<{ field: string; operator: string; value: any }>): any[] { - const triplets = conditions - .filter(c => c.field) // skip empty - .map(c => [c.field, BUILDER_TO_SPEC_OP[c.operator] || c.operator, c.value]); - - if (triplets.length === 0) return []; - if (triplets.length === 1 && logic === 'and') return triplets[0]; - if (logic === 'or') return ['or', ...triplets]; - return triplets; -} - // --------------------------------------------------------------------------- // Parse helpers // ---------------------------------------------------------------------------