From 224725ca10274e018dede8ab6f00e869c3c3a7c6 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 2 Sep 2026 14:57:09 +0000 Subject: [PATCH] fix(core): teach ValueDataSource's matcher the filter vocabulary the wire has MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `matchesASTFilter` recognised exactly two node shapes — a logical `and` / `or` head and a three-element comparison — and returned `true` for everything else. A filtered in-memory list therefore came back UNFILTERED, with no error and no console line. Three distinct ways in, all reachable from a shipped view: - a legacy flat implicit-AND array `[[…], […]]` matched no shape, at top level and as a nested child of `and` / `or` alike. That is what `mergeFilterNodes` returns for a lone surviving source, i.e. the common case, and what `ListView`'s `finalFilter` puts on `$filter`; - the null-ness operators had no arm in the operator switch, so `is_null` / `is_not_null` selected every row in both dialects; - 16 of the spec's 20 canonical `VIEW_FILTER_OPERATORS` had no arm either. `viewFilterRuleToNode` lowers a stored view's rules through the spec's `normalizeFilterOperator`, so what arrives is the canonical VIEW spelling (`equals`, `greater_than`, `starts_with`) — none of which the spelling-keyed switch knew. The matcher now reads the four shapes `FilterArraySchema` declares and canonicalises operators through the spec's own `canonicalAstOperator`, so the accepted vocabulary is the published one rather than a second hand-written list that drifts from it. An operator or shape it cannot execute excludes the row and logs once per `find()` — the measured sibling answer (`@object-ui/permissions` returns `false` from its `default` arm) rather than the silent widening. No producer changes: not one request byte moves. Refs #7221, #7349 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01EMrWaQw3XS5DxTHxp4yRyC --- .../value-datasource-ast-filter-vocabulary.md | 20 ++ packages/core/src/adapters/ValueDataSource.ts | 221 ++++++++++--- ...alueDataSource.astFilterVocabulary.test.ts | 312 ++++++++++++++++++ .../filter-dialect-equivalence-7221.test.ts | 154 +++++---- 4 files changed, 598 insertions(+), 109 deletions(-) create mode 100644 .changeset/value-datasource-ast-filter-vocabulary.md create mode 100644 packages/core/src/adapters/__tests__/ValueDataSource.astFilterVocabulary.test.ts diff --git a/.changeset/value-datasource-ast-filter-vocabulary.md b/.changeset/value-datasource-ast-filter-vocabulary.md new file mode 100644 index 0000000000..42954f8092 --- /dev/null +++ b/.changeset/value-datasource-ast-filter-vocabulary.md @@ -0,0 +1,20 @@ +--- +'@object-ui/core': patch +--- + +fix(core): `ValueDataSource` applies the filters it is given instead of returning every row + +`matchesASTFilter` recognised only two node shapes — a logical `and` / `or` head +and a three-element comparison — and answered `true` for everything else. Three +consequences, all silent: a legacy flat implicit-AND array (`[[…], […]]`) applied +no filter at all, at top level and as a nested child of `and` / `or` alike; the +null-ness operators had no arm, so `is_null` / `is_not_null` selected every row; +and 16 of the spec's 20 canonical view operators — `equals`, `greater_than`, +`starts_with` among them, the spellings `toFilterNode` lowers a stored view's +rules into — fell through the same way. + +The matcher now canonicalises operators through the spec's own +`canonicalAstOperator` and reads all four shapes `FilterArraySchema` declares, so +an in-memory `provider: 'value'` list applies the same filter the wire would. An +operator or shape it cannot execute now excludes the row and logs once per +`find()`, rather than passing every row with no signal anywhere. diff --git a/packages/core/src/adapters/ValueDataSource.ts b/packages/core/src/adapters/ValueDataSource.ts index 0832e566f1..4ebeef5bc4 100644 --- a/packages/core/src/adapters/ValueDataSource.ts +++ b/packages/core/src/adapters/ValueDataSource.ts @@ -18,6 +18,7 @@ import type { AggregateParams, AggregateResult, } from '@object-ui/types'; +import { canonicalAstOperator } from '@objectstack/spec/data'; import { emulateBatchTransaction } from './batchTransaction.js'; // --------------------------------------------------------------------------- @@ -41,69 +42,179 @@ function getRecordId(record: any, idField?: string): string | number | undefined return record.id ?? record._id; } +/** + * A filter node this matcher cannot execute. + * + * Recorded and EXCLUDING, never silently ignored. Answering `true` for a node + * the matcher does not understand is how a filtered list came back UNFILTERED — + * every row, no error, not one console line (objectui#7349). The measured + * sibling behaviour is a refusal, not a pass: `evaluateCondition` + * (`@object-ui/permissions`) returns `false` from its `default` arm, and + * `ReportViewer`'s formatting switch leaves `match` false. The wire-side + * sibling `@object-ui/data-objectstack` goes further and THROWS + * (`MalformedFilterError`), on the stated ground that dropping one entry of an + * `and` WIDENS the result set — but it is deciding whether to send a query at + * all, while this matcher is deciding about a single row. So the row is + * excluded and the reason is logged once per distinct refusal per `find()`, + * which keeps a 10k-row scan to one line. + */ +function refuseFilterNode(refusals: Set, reason: string): false { + refusals.add(`[ObjectUI] ValueDataSource: ${reason}. Rows are excluded rather than passed through.`); + return false; +} + +/** + * Evaluate ONE comparison node — `[field, operator]` or `[field, operator, value]`. + * + * The operator is canonicalized through the spec's own + * {@link canonicalAstOperator}, so this switch has ONE arm per operator rather + * than one per spelling, and the accepted vocabulary is the published one + * rather than a second hand-written list that drifts from it. That matters here + * more than it reads: `viewFilterRuleToNode` (`../utils/filter-converter.ts`) + * lowers a stored view's rules through the spec's `normalizeFilterOperator`, + * so what actually arrives is the CANONICAL VIEW spelling — `equals`, + * `greater_than`, `starts_with` — and 16 of the 20 `VIEW_FILTER_OPERATORS` + * had no arm in the old spelling-keyed switch. Every one of them reached its + * `default: return true` and selected every row. + */ +function matchesComparisonNode( + record: any, + node: any[], + refusals: Set, +): boolean { + const field = node[0] as string; + const rawOperator = node[1] as string; + // `'not in'` (with a space) is NOT a member of the spec's + // `VALID_AST_OPERATORS` — the wire would refuse it — but this matcher has + // always implemented it and a test pins it. Canonicalizing it here keeps the + // refusal arm below from deleting support that exists today; whether to + // retire the spelling is a separate question from this card's. + const operator = + rawOperator === 'not in' ? 'nin' : canonicalAstOperator(rawOperator); + const value = record[field]; + const target = node[2]; + + switch (operator) { + // -- Null-ness. Direction comes from the operator NAME; the value slot is + // never read, so the 2-tuple `['x', 'is_not_null']` and the 3-tuple + // `['x', 'isnotnull', null]` are the same predicate. `canonicalAstOperator` + // folds all eight spellings (`is_null` / `isnull` / `is_empty` / `isempty` + // and their four negatives) onto these two arms — including `is_empty`, + // which the spec lowers to `$null` rather than to an emptiness test. + case 'is_null': + return value === null || value === undefined; + case 'is_not_null': + return value !== null && value !== undefined; + + case '=': + return value === target; + case '!=': + return value !== target; + case '>': + return value > target; + case '>=': + return value >= target; + case '<': + return value < target; + case '<=': + return value <= target; + case 'in': + return Array.isArray(target) && target.includes(value); + case 'nin': + return Array.isArray(target) && !target.includes(value); + case 'contains': + case 'icontains': { + const lv = typeof value === 'string' ? value.toLowerCase() : ''; + return typeof value === 'string' && lv.includes(String(target).toLowerCase()); + } + case 'not_contains': { + const lv = typeof value === 'string' ? value.toLowerCase() : ''; + return typeof value === 'string' && !lv.includes(String(target).toLowerCase()); + } + case 'starts_with': { + const lv = typeof value === 'string' ? value.toLowerCase() : ''; + return typeof value === 'string' && lv.startsWith(String(target).toLowerCase()); + } + case 'ends_with': { + const lv = typeof value === 'string' ? value.toLowerCase() : ''; + return typeof value === 'string' && lv.endsWith(String(target).toLowerCase()); + } + case 'between': + return Array.isArray(target) && target.length === 2 && value >= target[0] && value <= target[1]; + + default: + // Includes the spec-valid `like` / `ilike`: they carry pattern semantics + // this in-memory matcher does not implement, and no producer in this repo + // emits them into a filter. Refusing is the loud answer; matching every + // row was the silent one. + return refuseFilterNode( + refusals, + `filter operator '${String(rawOperator)}' is not implemented by the in-memory matcher`, + ); + } +} + /** * Evaluate an AST-format filter node against a record. - * Supports conditions like ['field', 'op', value] and logical - * combinations like ['and', ...conditions] or ['or', ...conditions]. + * + * Reads the four shapes the spec's `FilterArraySchema` declares and + * `isFilterAST` accepts, so this consumer applies the same filter the wire + * would (objectui#7349): + * + * - `['and' | 'or', ...children]` a logical group + * - `[field, operator, value]` a comparison + * - `[field, operator]` a comparison whose operator needs no value + * - `[[…], […]]` a legacy flat list of conditions, implicit AND + * + * The last one is the shape this matcher used to ignore ENTIRELY, at top level + * and as a child of `and` / `or` alike — and it is what `mergeFilterNodes` + * returns for a lone surviving filter source, i.e. the common case. A flat + * array is unambiguous: the spec's `FilterArrayFieldSchema` forbids `and` / `or` + * as field names, so a node whose head is itself an array can only be a list. + * + * Anything else is refused rather than passed. An empty array stays "no + * filter" — the spec says the same, and `find()` never calls with one. */ -function matchesASTFilter(record: any, filterNode: any[]): boolean { - if (!filterNode || filterNode.length === 0) return true; +function matchesASTFilter(record: any, filterNode: any, refusals: Set): boolean { + if (!Array.isArray(filterNode)) { + return refuseFilterNode( + refusals, + `filter node ${JSON.stringify(filterNode) ?? String(filterNode)} is not an array`, + ); + } + if (filterNode.length === 0) return true; const head = filterNode[0]; - // Logical operators: ['and', ...conditions] or ['or', ...conditions] - if (head === 'and') { - return filterNode.slice(1).every((sub: any) => matchesASTFilter(record, sub)); + // Logical group. `length >= 2` mirrors `isFilterAST`: the keyword opens a + // group and a group needs at least one condition. + if (typeof head === 'string') { + const keyword = head.toLowerCase(); + if (keyword === 'and' || keyword === 'or') { + if (filterNode.length < 2) { + return refuseFilterNode(refusals, `'${keyword}' group carries no conditions`); + } + const children = filterNode.slice(1); + return keyword === 'and' + ? children.every((sub: any) => matchesASTFilter(record, sub, refusals)) + : children.some((sub: any) => matchesASTFilter(record, sub, refusals)); + } } - if (head === 'or') { - return filterNode.slice(1).some((sub: any) => matchesASTFilter(record, sub)); + + // Legacy flat list of conditions — implicit AND, the way the server reads it. + if (Array.isArray(head)) { + return filterNode.every((sub: any) => matchesASTFilter(record, sub, refusals)); } - // Condition node: [field, operator, value] - if (filterNode.length === 3 && typeof head === 'string') { - const [field, operator, target] = filterNode; - const value = record[field]; - - switch (operator) { - case '=': - return value === target; - case '!=': - return value !== target; - case '>': - return value > target; - case '>=': - return value >= target; - case '<': - return value < target; - case '<=': - return value <= target; - case 'in': - return Array.isArray(target) && target.includes(value); - case 'not in': - case 'not_in': - case 'nin': // canonical (per spec) - case 'notin': // legacy alias - return Array.isArray(target) && !target.includes(value); - case 'contains': { - const lv = typeof value === 'string' ? value.toLowerCase() : ''; - return typeof value === 'string' && lv.includes(String(target).toLowerCase()); - } - case 'notcontains': { - const lv = typeof value === 'string' ? value.toLowerCase() : ''; - return typeof value === 'string' && !lv.includes(String(target).toLowerCase()); - } - case 'startswith': { - const lv = typeof value === 'string' ? value.toLowerCase() : ''; - return typeof value === 'string' && lv.startsWith(String(target).toLowerCase()); - } - case 'between': - return Array.isArray(target) && target.length === 2 && value >= target[0] && value <= target[1]; - default: - return true; - } + // Comparison node, 2- or 3-element. + if (typeof head === 'string' && typeof filterNode[1] === 'string' && filterNode.length <= 3) { + return matchesComparisonNode(record, filterNode, refusals); } - return true; + return refuseFilterNode( + refusals, + `filter node ${JSON.stringify(filterNode)} is not a shape the matcher reads`, + ); } /** @@ -258,7 +369,11 @@ export class ValueDataSource implements DataSource { // Filter — support both MongoDB-style objects and AST-format arrays if (params?.$filter) { if (Array.isArray(params.$filter) && params.$filter.length > 0) { - result = result.filter((r) => matchesASTFilter(r, params.$filter as any[])); + // One collector per `find()`, drained after the pass: a node the matcher + // refuses would otherwise log once PER ROW. + const refusals = new Set(); + result = result.filter((r) => matchesASTFilter(r, params.$filter as any[], refusals)); + for (const message of refusals) console.warn(message); } else if (!Array.isArray(params.$filter) && Object.keys(params.$filter).length > 0) { result = result.filter((r) => matchesFilter(r, params.$filter!)); } diff --git a/packages/core/src/adapters/__tests__/ValueDataSource.astFilterVocabulary.test.ts b/packages/core/src/adapters/__tests__/ValueDataSource.astFilterVocabulary.test.ts new file mode 100644 index 0000000000..bcb254979a --- /dev/null +++ b/packages/core/src/adapters/__tests__/ValueDataSource.astFilterVocabulary.test.ts @@ -0,0 +1,312 @@ +/** + * 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. + */ + +/** + * objectui#7349 — `ValueDataSource`'s in-memory matcher reads the filter + * vocabulary the wire already accepts, and refuses what it cannot execute. + * + * ## Why every case here needs a control + * + * The defect being fixed is a fall-through to `true`: before this card, + * `matchesASTFilter` answered "matches" for every shape and operator it did not + * recognise. So "the filter selected my row" passes on the BROKEN code as + * loudly as on the fixed code, and an assertion written that way measures + * nothing. Every case below is therefore written as a row-SET equality where + * the broken answer is the full set — `expect(ids).toEqual(['a','c'])` is red + * when the matcher returns `['a','b','c']`. + * + * The live control is {@link CONTROL_ROWS} + `['role', '=', 'admin']`: an + * operator and shape the matcher implemented BEFORE this card, so it selects + * correctly on both trees and proves the harness itself discriminates. + * + * ## What the fix taught the matcher + * + * 1. The legacy flat array `[[…], […]]` is an implicit AND — at top level and + * as a child of `and` / `or`. It is what `mergeFilterNodes` returns for a + * lone surviving source, so it is the COMMON shape, not an exotic one. + * 2. The null-ness operators take their direction from the operator NAME and + * never read the value slot, in every spelling the spec folds onto them. + * 3. An operator or shape the matcher cannot execute excludes the row and says + * so, instead of passing every row silently. + */ + +import { describe, it, expect, vi, afterEach } from 'vitest'; +import { VIEW_FILTER_OPERATORS } from '@objectstack/spec/ui'; +import { ValueDataSource } from '../ValueDataSource'; +import { mergeFilterNodes, toFilterNode } from '../../utils/filter-converter'; + +/** Three rows, two of them sharing a `role`, so a wrong answer is never the right size. */ +const CONTROL_ROWS = [ + { id: 'a', role: 'admin', age: 30 }, + { id: 'b', role: 'user', age: 25 }, + { id: 'c', role: 'admin', age: 20 }, +]; + +const ALL_CONTROL_IDS = ['a', 'b', 'c']; + +async function selectedIds( + filter: unknown, + rows: Array> = CONTROL_ROWS, +): Promise { + const ds = new ValueDataSource({ items: rows }); + const result = await ds.find('rows', { $filter: filter as any }); + return result.data.map((r) => r.id as string); +} + +afterEach(() => { + vi.restoreAllMocks(); +}); + +// --------------------------------------------------------------------------- +// 0. The live control +// --------------------------------------------------------------------------- + +describe('objectui#7349 — live control', () => { + it('an operator the matcher implemented BEFORE this card still selects', async () => { + // Green on the broken tree AND on the fixed one. Its job is to prove the + // rows, the adapter and the id projection work, so that a red anywhere + // below is about the vocabulary rather than the harness. + expect(await selectedIds(['role', '=', 'admin'])).toEqual(['a', 'c']); + expect(await selectedIds(['and', ['role', '=', 'admin'], ['age', '>', 24]])).toEqual(['a']); + }); + + it('the broken answer is the full set, so every case below discriminates', async () => { + expect(await selectedIds([])).toEqual(ALL_CONTROL_IDS); + }); +}); + +// --------------------------------------------------------------------------- +// 1. The flat implicit-AND array +// --------------------------------------------------------------------------- + +describe('objectui#7349 — a flat array of rules is an implicit AND', () => { + it('at top level, multi-rule', async () => { + expect(await selectedIds([['role', '=', 'admin'], ['age', '>', 24]])).toEqual(['a']); + }); + + it('at top level, single-rule — it is the SHAPE, not the count', async () => { + expect(await selectedIds([['role', '=', 'admin']])).toEqual(['a', 'c']); + }); + + it('as a child of `and` — the gate’s two-source output keeps its authored rules', async () => { + // `['and', , ]` is what + // `mergeFilterNodes` emits when two sources survive. The nested child used + // to be swallowed whole. + expect( + await selectedIds(['and', [['role', '=', 'admin'], ['age', '>', 24]], ['id', '!=', 'zzz']]), + ).toEqual(['a']); + }); + + it('as a child of `or`', async () => { + expect( + await selectedIds(['or', [['role', '=', 'admin'], ['age', '>', 24]], ['id', '=', 'b']]), + ).toEqual(['a', 'b']); + }); + + it('nesting is equivalent to flattening, which is what objectui#7221 asked', async () => { + const nested = ['and', [['role', '=', 'admin'], ['age', '>', 24]], ['id', '!=', 'zzz']]; + const flattened = ['and', ['role', '=', 'admin'], ['age', '>', 24], ['id', '!=', 'zzz']]; + expect(await selectedIds(nested)).toEqual(await selectedIds(flattened)); + }); +}); + +// --------------------------------------------------------------------------- +// 2. The null-ness operators +// --------------------------------------------------------------------------- + +const NULL_ROWS = [ + { id: 'has', ts: '2026-01-01' }, + { id: 'null', ts: null }, + { id: 'undef', ts: undefined }, + { id: 'missing' }, +]; + +/** Every spelling the spec's `canonicalAstOperator` folds onto `is_null`. */ +const IS_NULL_SPELLINGS = ['is_null', 'isnull', 'is_empty', 'isempty']; +/** …and onto `is_not_null`. `is_empty` folds to `$null` in the spec too. */ +const IS_NOT_NULL_SPELLINGS = ['is_not_null', 'isnotnull', 'is_not_empty', 'isnotempty']; + +describe('objectui#7349 — null-ness takes direction from the operator NAME', () => { + it.each(IS_NULL_SPELLINGS)('`%s` selects null, undefined and the absent key', async (op) => { + expect(await selectedIds([['ts', op]], NULL_ROWS)).toEqual(['null', 'undef', 'missing']); + }); + + it.each(IS_NOT_NULL_SPELLINGS)('`%s` selects only the row that has a value', async (op) => { + expect(await selectedIds([['ts', op]], NULL_ROWS)).toEqual(['has']); + }); + + it('never reads the value slot — filler, null, or no slot at all', async () => { + // The 2-tuple and the 3-tuple-with-`null` are the same predicate, and a + // filler comparand changes nothing. This is the question objectui#7221 + // asked and could not answer, because the operator was unimplemented. + expect(await selectedIds(['ts', 'is_not_null'], NULL_ROWS)).toEqual(['has']); + expect(await selectedIds(['ts', 'isnotnull', null], NULL_ROWS)).toEqual(['has']); + expect(await selectedIds(['ts', 'isnotnull', 'FILLER'], NULL_ROWS)).toEqual(['has']); + expect(await selectedIds(['and', ['ts', 'isnotnull', null]], NULL_ROWS)).toEqual(['has']); + expect(await selectedIds([['ts', 'isnotnull', null]], NULL_ROWS)).toEqual(['has']); + }); + + it('the card’s own two-rule filter, in both dialects', async () => { + const rows = [ + { id: 'both', visible_from: '2026-01-01', due_date: '2026-02-01' }, + { id: 'one', visible_from: '2026-01-01', due_date: null }, + { id: 'neither' }, + ]; + const flat = [['visible_from', 'is_not_null'], ['due_date', 'is_not_null']]; + const wrapped = ['and', ['visible_from', 'isnotnull', null], ['due_date', 'isnotnull', null]]; + expect(await selectedIds(flat, rows)).toEqual(['both']); + expect(await selectedIds(wrapped, rows)).toEqual(['both']); + }); +}); + +// --------------------------------------------------------------------------- +// 3. Refusal — the arm that used to be `return true` +// --------------------------------------------------------------------------- + +describe('objectui#7349 — what the matcher cannot execute, it refuses', () => { + it('an unknown operator excludes every row and logs once', async () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + expect(await selectedIds(['role', 'no_such_operator', 'admin'])).toEqual([]); + // One line for three rows: the refusal is collected per `find()`, not per row. + expect(warn).toHaveBeenCalledTimes(1); + expect(String(warn.mock.calls[0]?.[0])).toContain('no_such_operator'); + }); + + it('a spec-valid operator this matcher does not implement refuses too', async () => { + // `like` is a member of the spec's `VALID_AST_OPERATORS`, so the wire would + // accept it; the in-memory matcher has no pattern engine. Refusing is loud, + // matching every row was silent. + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + expect(await selectedIds(['role', 'like', '%admin%'])).toEqual([]); + expect(warn).toHaveBeenCalledTimes(1); + }); + + it('a shape the matcher cannot read excludes every row and logs', async () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + expect(await selectedIds(['role'])).toEqual([]); + expect(warn).toHaveBeenCalledTimes(1); + }); + + it('a non-array child of `and` is refused rather than passed', async () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + expect(await selectedIds(['and', ['role', '=', 'admin'], 'garbage'])).toEqual([]); + expect(warn).toHaveBeenCalledTimes(1); + }); + + it('an empty filter still means "no filter"', async () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + expect(await selectedIds([])).toEqual(ALL_CONTROL_IDS); + expect(warn).not.toHaveBeenCalled(); + }); +}); + +// --------------------------------------------------------------------------- +// 4. The canonical VIEW vocabulary — what the producer in this package emits +// --------------------------------------------------------------------------- + +/** + * A comparand per canonical view operator, chosen so the expected row set is + * never the full set. `viewFilterRuleToNode` lowers a stored view's rules + * through the spec's `normalizeFilterOperator`, which canonicalizes to + * `VIEW_FILTER_OPERATORS` — so THESE are the spellings that actually arrive at + * the matcher from a saved view, and 16 of the 20 had no arm before this card. + */ +const VIEW_OPERATOR_CASES: Record = { + equals: { node: ['role', 'equals', 'admin'], expected: ['a', 'c'] }, + not_equals: { node: ['role', 'not_equals', 'admin'], expected: ['b'] }, + contains: { node: ['role', 'contains', 'dmi'], expected: ['a', 'c'] }, + not_contains: { node: ['role', 'not_contains', 'dmi'], expected: ['b'] }, + icontains: { node: ['role', 'icontains', 'ADMI'], expected: ['a', 'c'] }, + starts_with: { node: ['role', 'starts_with', 'adm'], expected: ['a', 'c'] }, + ends_with: { node: ['role', 'ends_with', 'min'], expected: ['a', 'c'] }, + greater_than: { node: ['age', 'greater_than', 24], expected: ['a', 'b'] }, + less_than: { node: ['age', 'less_than', 25], expected: ['c'] }, + greater_than_or_equal: { node: ['age', 'greater_than_or_equal', 25], expected: ['a', 'b'] }, + less_than_or_equal: { node: ['age', 'less_than_or_equal', 25], expected: ['b', 'c'] }, + in: { node: ['role', 'in', ['admin']], expected: ['a', 'c'] }, + not_in: { node: ['role', 'not_in', ['admin']], expected: ['b'] }, + between: { node: ['age', 'between', [24, 31]], expected: ['a', 'b'] }, + // `before` / `after` are canonical view operators with no infix spelling of + // their own; the spec lowers them to `$lt` / `$gt`. + before: { node: ['age', 'before', 25], expected: ['c'] }, + after: { node: ['age', 'after', 24], expected: ['a', 'b'] }, + is_empty: { node: ['nickname', 'is_empty'], expected: ['b', 'c'] }, + is_not_empty: { node: ['nickname', 'is_not_empty'], expected: ['a'] }, + is_null: { node: ['nickname', 'is_null'], expected: ['b', 'c'] }, + is_not_null: { node: ['nickname', 'is_not_null'], expected: ['a'] }, +}; + +/** `CONTROL_ROWS` plus a nullable column, so the null-ness cases discriminate. */ +const VIEW_ROWS = [ + { id: 'a', role: 'admin', age: 30, nickname: 'ace' }, + { id: 'b', role: 'user', age: 25, nickname: null }, + { id: 'c', role: 'admin', age: 20 }, +]; + +describe('objectui#7349 — every canonical VIEW operator is executed, not waved through', () => { + it('the case table covers VIEW_FILTER_OPERATORS exactly — no operator drops out unnoticed', () => { + // A parity guard, not a restatement: when a spec release adds a view + // operator, this goes red instead of the new operator silently selecting + // every row through the refusal arm. + expect([...VIEW_FILTER_OPERATORS].sort()).toEqual(Object.keys(VIEW_OPERATOR_CASES).sort()); + }); + + it.each(Object.entries(VIEW_OPERATOR_CASES))( + '`%s` filters rather than matching everything', + async (_op, { node, expected }) => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + expect(await selectedIds(node, VIEW_ROWS)).toEqual(expected); + // Executed, not refused: a refusal would also fail the row-set assertion, + // but this names WHICH failure it is. + expect(warn).not.toHaveBeenCalled(); + }, + ); +}); + +// --------------------------------------------------------------------------- +// 5. The reachability chain, executed +// --------------------------------------------------------------------------- + +describe('objectui#7349 — the production chain, end to end', () => { + const AUTHORED_RULES = [ + { field: 'visible_from', operator: 'is_not_null' }, + { field: 'due_date', operator: 'is_not_null' }, + ]; + const ROWS = [ + { id: 'both', visible_from: '2026-01-01', due_date: '2026-02-01' }, + { id: 'one', visible_from: '2026-01-01', due_date: null }, + { id: 'neither' }, + ]; + + it('a lone surviving source reaches the adapter FLAT and is applied', async () => { + // `buildEffectiveFilter` → `mergeFilterNodes` → `$filter` → `ValueDataSource`. + // The lone-source path returns the flat array unwrapped, which is exactly + // the shape the matcher used to ignore. + const filter = mergeFilterNodes(AUTHORED_RULES); + expect(filter).toEqual([['visible_from', 'is_not_null'], ['due_date', 'is_not_null']]); + expect(await selectedIds(filter, ROWS)).toEqual(['both']); + }); + + it('two surviving sources nest the authored array and it still applies', async () => { + const filter = mergeFilterNodes(AUTHORED_RULES, ['id', '!=', 'zzz']); + expect(filter).toEqual([ + 'and', + [['visible_from', 'is_not_null'], ['due_date', 'is_not_null']], + ['id', '!=', 'zzz'], + ]); + expect(await selectedIds(filter, ROWS)).toEqual(['both']); + }); + + it('a saved view’s `equals` rule — the shipped spelling — filters', async () => { + // The spelling `toFilterNode` produces from stored view metadata. It had no + // arm in the old switch, so a `provider: 'value'` list showed every row. + const filter = toFilterNode([{ field: 'role', operator: 'equals', value: 'admin' }]); + expect(filter).toEqual([['role', 'equals', 'admin']]); + expect(await selectedIds(filter)).toEqual(['a', 'c']); + }); +}); diff --git a/packages/core/src/utils/__tests__/filter-dialect-equivalence-7221.test.ts b/packages/core/src/utils/__tests__/filter-dialect-equivalence-7221.test.ts index 56487c133d..de052792a0 100644 --- a/packages/core/src/utils/__tests__/filter-dialect-equivalence-7221.test.ts +++ b/packages/core/src/utils/__tests__/filter-dialect-equivalence-7221.test.ts @@ -24,41 +24,47 @@ * half of that measurement; `data-objectstack/src/filter-dialect-wire-7221.test.ts` * is the wire half. * - * ## The answer, in one line + * ## The answer, in one line — and what became of it * - * On the card's own filter the two dialects agree — and they agree because - * `matchesASTFilter` evaluates NEITHER of them. Change the operator to one the - * in-memory matcher implements and they part: the flat dialect applies no filter - * at all while the `and`-wrapped one filters correctly. So the equivalence the - * card hoped to establish does NOT hold in general, and the case it was observed - * on is the accidentally-benign one. + * MEASURED (objectui#7221, this file as filed): on the card's own filter the two + * dialects agreed — and they agreed because `matchesASTFilter` evaluated NEITHER + * of them. Changing the operator to one the in-memory matcher implemented parted + * them: the flat dialect applied no filter at all while the `and`-wrapped one + * filtered correctly. So the equivalence the card hoped to establish did NOT hold + * in general, and the case it was observed on was the accidentally-benign one. * - * ## Why the flat dialect is inert here + * REPAIRED (objectui#7349): the matcher now reads the flat implicit-AND array — + * at top level and as a child of `and` / `or` — and implements the null-ness + * operators, so the two dialects select the same rows for the right reason. The + * two `it.fails` cases below became plain `it` in that PR, and the row sets in + * sections 2 and 3 were RE-MEASURED against the repaired matcher. Each one keeps + * its pre-repair number in a comment, because those numbers are the evidence + * objectui#7221's severity was graded on. * - * `matchesASTFilter` (`../../adapters/ValueDataSource.ts`) recognises exactly two + * ## What the flat dialect used to be inert against + * + * `matchesASTFilter` (`../../adapters/ValueDataSource.ts`) recognised exactly two * node shapes: a logical `['and'|'or', ...children]` head, and a THREE-element * comparison `[field, operator, value]`. A legacy flat array of condition nodes — * `[[…], […]]`, the implicit-AND shape `toFilterNode` returns for a lone source — - * matches neither, so it reaches the closing `return true` and every row passes. - * The same fall-through swallows it as a CHILD of an `and`, which is the shape + * matched neither, so it reached the closing `return true` and every row passed. + * The same fall-through swallowed it as a CHILD of an `and`, which is the shape * `ElementDataSourceGate` produces from two surviving sources. * - * Two separate reasons the card's own filter is inert, and both are recorded - * below: the flat shape is unread AS A SHAPE, and `is_not_null` / `isnotnull` are - * unimplemented AS OPERATORS (the `switch` has no null-ness arm, so its `default` - * returns true for the wrapped dialect too). + * Two separate reasons the card's own filter was inert, and both are recorded + * below: the flat shape was unread AS A SHAPE, and `is_not_null` / `isnotnull` + * were unimplemented AS OPERATORS (the `switch` had no null-ness arm, so its + * `default` returned true for the wrapped dialect too). * * ## Reading a red in this file * - * - An `it.fails` case going RED means the divergence was repaired — the two - * dialects now agree. That is the good day; delete the `.fails` and keep the - * assertion. - * - A plain case going red means the measured behaviour moved. Re-measure before - * changing the expectation: these numbers are the evidence objectui#7221's - * severity was graded on. + * A case going red means the measured behaviour moved — re-measure before + * changing the expectation. Note the change of status the repair brought: the two + * equivalence assertions are now plain `it`, so they are a CONTRACT rather than + * an observation, and a red there is a regression of objectui#7349. * - * ⛔ No product code is changed by this card. Which dialect wins is a ruling, not - * a test's decision. + * The vocabulary the repair taught the matcher is pinned separately, in + * `../../adapters/__tests__/ValueDataSource.astFilterVocabulary.test.ts`. */ import { describe, it, expect } from 'vitest'; @@ -163,6 +169,23 @@ const EDGE_ROWS: Array> = [ const ALL_EDGE_IDS = EDGE_ROWS.map((r) => r.id as string); +/** + * What both dialects keep once the null-ness operators are IMPLEMENTED + * (objectui#7349): every edge value that is neither `null` nor `undefined` + * survives `is_not_null` — `''`, `0` and `false` included, since the operator + * asks about PRESENCE and not about truthiness — while the `null` row, the + * `undefined` row and the row carrying no such key are excluded. + * + * `NaN` is excluded, and NOT because of the operator. `ValueDataSource`'s + * constructor deep-clones its items with `JSON.parse(JSON.stringify(items))`, + * and `JSON.stringify` writes `NaN` as `null` — so by the time any filter runs, + * that row genuinely holds `null`. Measured while repairing objectui#7349; + * before the repair nothing was ever excluded, so the round-trip was invisible + * here. (`undefined` is dropped by the same round-trip, which is why the + * `undefined` row and the `missing-key` row are indistinguishable below.) + */ +const PRESENT_EDGE_IDS = ['empty-string', 'zero', 'false', 'array', 'object', 'real']; + async function selectedIds( filter: unknown, rows: Array> = EDGE_ROWS, @@ -173,37 +196,52 @@ async function selectedIds( } describe('objectui#7221 — row sets through ValueDataSource, the card’s filter', () => { - it('dialect A selects EVERY row, including the ones the filter exists to exclude', async () => { - // The measured behaviour of path A, on its own. A flat array is not a shape - // `matchesASTFilter` reads, so nothing is excluded — not the null row, not + it('dialect A excludes exactly the rows the filter exists to exclude', async () => { + // BEFORE objectui#7349 this was ALL_EDGE_IDS: a flat array was not a shape + // `matchesASTFilter` read, so nothing was excluded — not the null row, not // the row that has no such key. - expect(await selectedIds(DIALECT_A)).toEqual(ALL_EDGE_IDS); + expect(await selectedIds(DIALECT_A)).toEqual(PRESENT_EDGE_IDS); }); - it('dialect B selects EVERY row too — for a different reason', async () => { - // Path B's shape IS read: `and` recurses, each child is a 3-element - // comparison node. It is the OPERATOR that is unimplemented — the `switch` - // has no `isnotnull` arm and its `default` returns true. - expect(await selectedIds(DIALECT_B)).toEqual(ALL_EDGE_IDS); + it('dialect B selects the same rows — and now for the same reason', async () => { + // BEFORE objectui#7349 this was ALL_EDGE_IDS too, but for a DIFFERENT reason + // than dialect A: path B's shape WAS read (`and` recurses, each child is a + // 3-element comparison node) and it was the OPERATOR that was unimplemented — + // the `switch` had no `isnotnull` arm and its `default` returned true. + expect(await selectedIds(DIALECT_B)).toEqual(PRESENT_EDGE_IDS); }); - it('so the two dialects agree here — at "no filter applied", on both sides', async () => { + it('so the two dialects agree here — now at the SAME APPLIED filter', async () => { expect(await selectedIds(DIALECT_A)).toEqual(await selectedIds(DIALECT_B)); }); + it('and they no longer agree at "no filter applied", which is what changed', async () => { + // `ALL_EDGE_IDS` was the measured answer for BOTH dialects before + // objectui#7349, so naming it here keeps the regression visible: a return + // of the fall-through to `true` takes exactly this shape. The repaired + // matcher only ever EXCLUDES rows, so the kept set stays a subset. + expect(await selectedIds(DIALECT_A)).not.toEqual(ALL_EDGE_IDS); + expect(await selectedIds(DIALECT_B)).not.toEqual(ALL_EDGE_IDS); + expect(PRESENT_EDGE_IDS.every((id) => ALL_EDGE_IDS.includes(id))).toBe(true); + }); + it('the null-ness operators never read their value slot, in any spelling', async () => { // The card asked specifically whether the matcher reads the value slot for - // the null-ness operators. It does not — but only because it does not reach - // the value at all. Filler, `null`, or an absent slot: same rows. + // the null-ness operators. It does not — and since objectui#7349 that is a + // real answer rather than an artefact of never reaching the value at all. + // Filler, `null`, or an absent slot: same rows. + // + // BEFORE objectui#7349 every line below was `['null-row', 'real-row']` — + // the whole set, i.e. no filter applied in any spelling. const rows = [ { id: 'null-row', visible_from: null }, { id: 'real-row', visible_from: '2026-01-01' }, ]; - const both = ['null-row', 'real-row']; - expect(await selectedIds(['visible_from', 'isnotnull', null], rows)).toEqual(both); - expect(await selectedIds(['visible_from', 'isnotnull', 'FILLER'], rows)).toEqual(both); - expect(await selectedIds(['visible_from', 'is_not_null'], rows)).toEqual(both); - expect(await selectedIds(['and', ['visible_from', 'isnotnull', null]], rows)).toEqual(both); + const onlyReal = ['real-row']; + expect(await selectedIds(['visible_from', 'isnotnull', null], rows)).toEqual(onlyReal); + expect(await selectedIds(['visible_from', 'isnotnull', 'FILLER'], rows)).toEqual(onlyReal); + expect(await selectedIds(['visible_from', 'is_not_null'], rows)).toEqual(onlyReal); + expect(await selectedIds(['and', ['visible_from', 'isnotnull', null]], rows)).toEqual(onlyReal); }); }); @@ -234,40 +272,44 @@ const CONTROL_GATE_TWO_SOURCE = ['and', CONTROL_A, ['id', '!=', 'zzz']]; const CONTROL_GATE_FLATTENED = ['and', ['role', '=', 'admin'], ['age', '>', 24], ['id', '!=', 'zzz']]; describe('objectui#7221 — the dialects on an operator the matcher implements', () => { - it('dialect A’s shape applies NO filter — every row comes back', async () => { - expect(await selectedIds(CONTROL_A, CONTROL_ROWS)).toEqual(['a', 'b', 'c']); + it('dialect A’s shape applies the filter — one row of three', async () => { + // BEFORE objectui#7349: ['a', 'b', 'c'] — no filter applied at all. + expect(await selectedIds(CONTROL_A, CONTROL_ROWS)).toEqual(['a']); }); - it('a single-rule flat array is inert too — it is the SHAPE, not the count', async () => { - expect(await selectedIds([['role', '=', 'admin']], CONTROL_ROWS)).toEqual(['a', 'b', 'c']); + it('a single-rule flat array filters too — it was the SHAPE, not the count', async () => { + // BEFORE objectui#7349: ['a', 'b', 'c']. + expect(await selectedIds([['role', '=', 'admin']], CONTROL_ROWS)).toEqual(['a', 'c']); }); it('dialect B’s shape filters correctly — one row of three', async () => { expect(await selectedIds(CONTROL_B, CONTROL_ROWS)).toEqual(['a']); }); - it('the gate’s two-source shape loses its nested authored rules', async () => { - // Only the sibling tuple child is evaluated; the nested flat child is - // swallowed by the same fall-through. - expect(await selectedIds(CONTROL_GATE_TWO_SOURCE, CONTROL_ROWS)).toEqual(['a', 'b', 'c']); + it('the gate’s two-source shape keeps its nested authored rules', async () => { + // BEFORE objectui#7349 only the sibling tuple child was evaluated and the + // nested flat child was swallowed by the fall-through, so the first line + // was ['a', 'b', 'c'] while the second was already ['a']. + expect(await selectedIds(CONTROL_GATE_TWO_SOURCE, CONTROL_ROWS)).toEqual(['a']); expect(await selectedIds(CONTROL_GATE_FLATTENED, CONTROL_ROWS)).toEqual(['a']); }); - it.fails( - 'the two dialects select the same rows — diverges — objectui#7221', + it( + 'the two dialects select the same rows — repaired — objectui#7349', async () => { - // GREEN today BECAUSE IT FAILS: dialect A returns a,b,c and dialect B - // returns a. Remove the `.fails` the day one lowered form is agreed on and - // `matchesASTFilter` reads it. + // Was `it.fails`: dialect A returned a,b,c while dialect B returned a. + // objectui#7349 taught `matchesASTFilter` to read the flat form, so both + // return a and this is now an ordinary passing contract. expect(await selectedIds(CONTROL_A, CONTROL_ROWS)) .toEqual(await selectedIds(CONTROL_B, CONTROL_ROWS)); }, ); - it.fails( - 'nesting an authored array under `and` preserves its rules — diverges — objectui#7221', + it( + 'nesting an authored array under `and` preserves its rules — repaired — objectui#7349', async () => { - // The gate's own output versus the same conditions unnested. + // The gate's own output versus the same conditions unnested. Was + // `it.fails`; the nested child is evaluated since objectui#7349. expect(await selectedIds(CONTROL_GATE_TWO_SOURCE, CONTROL_ROWS)) .toEqual(await selectedIds(CONTROL_GATE_FLATTENED, CONTROL_ROWS)); },