diff --git a/.changeset/8555-filter-converter-date-comparand.md b/.changeset/8555-filter-converter-date-comparand.md new file mode 100644 index 0000000000..e654a95fc1 --- /dev/null +++ b/.changeset/8555-filter-converter-date-comparand.md @@ -0,0 +1,39 @@ +--- +'@object-ui/core': patch +--- + +`convertFiltersToAST` now lowers a `Date` comparand instead of silently dropping +the field it sits on (objectui#8555). + +The operator-object arm opened on `typeof value === 'object' && +!Array.isArray(value)`, and a `Date` passes both tests. `Object.entries` of a +Date is `[]`, so the operator loop body never ran and NO condition was pushed: +`{ status: 'a', created: someDate }` lowered to `['status', '=', 'a']`. Nothing +threw and nothing warned — the result set simply got WIDER than the author asked +for, which is the one failure direction this file exists to avoid. The defect +also depended on the field's siblings: with the Date alone, `conditions` ended +empty and the original object came back untouched, so it only became visible +once a second field was present. + +It is LOWERED, not refused, and `@objectstack/spec` is what decides that — +the opposite answer to objectui#8514, which was a refusal precisely because the +spec declined to rule on that shape. Here it rules twice over (measured against +spec 17.3.0): `ACCEPTED_FILTER_COMPARAND_TYPES` is +`['string','number','bigint','boolean','null','Date']`, and +`$gt` / `$gte` / `$lt` / `$lte` / `$between` declare `z.ZodDate` in comparand +position. + +The AST leaf carries the `Date` INSTANCE. The wire form is deliberately not this +adapter's question to answer: `parseFilterAST(['created', '=', d])` hands back +`{ created: d }` with the Date intact, and the operator arm has always emitted +`{ created: { $gte: d } }` as `['created', '>=', d]` — so converting to an ISO +string or an epoch here would make the shorthand and the operator form emit two +different comparand types for one author intent. The gate is the spec's own +`isAcceptedFilterComparand` rather than a local `instanceof Date`, the same +reason this file already routes operators through the spec's +`normalizeFilterOperator` instead of a second map. + +Operator objects are untouched: `{ age: { $gt: 26 } }`, `$in` / `$nin` / +`$between` members, `$null` / `$exists`, the `$regex` / `$not` / bare-array +refusals, and an empty `{}` operator object (still the TRUE identity, still +constraining nothing) all behave exactly as before. diff --git a/.changeset/8557-view-filter-rule-arity.md b/.changeset/8557-view-filter-rule-arity.md new file mode 100644 index 0000000000..e2f4bc73fd --- /dev/null +++ b/.changeset/8557-view-filter-rule-arity.md @@ -0,0 +1,45 @@ +--- +'@object-ui/core': patch +--- + +`viewFilterRuleToNode` now refuses an ARRAY on a single-value operator instead +of passing it through verbatim (objectui#8557). + +A stored view rule never had its `value` inspected, so +`{ field: 'tags', operator: 'equals', value: ['a'] }` lowered to +`['tags', 'equals', ['a']]` — and the spec's own doors accept that node unjudged +(`isFilterAST` is `true`, `parseFilterAST` hands back `{ tags: ['a'] }`, measured +against spec 17.3.0). The refusal therefore arrived two layers away, as +`@objectstack/driver-sql`'s `400 INVALID_FILTER` or as an empty list from an +in-memory matcher, with nothing to attribute it to. It is the same +array-in-a-scalar-slot shape objectui#8530 refused in `convertFiltersToAST`'s +object arm, which deliberately did not reach this door — so a hand-authored +`{ tags: ['a'] }` failed fast with a message naming `$in` while the same mistake +saved into a view stayed silent. That asymmetry is closed. + +The guard keys on the operator's ARITY, never on `Array.isArray(value)`: `in`, +`not_in` and `between` legitimately carry arrays through this same function and +are untouched. The arity comes from the spec's own +`VIEW_FILTER_LIST_VALUE_OPERATORS` and `VIEW_FILTER_PAIR_VALUE_OPERATORS` rather +than a local list — the docblock on the first of them names a hard-coded +`["in", "notIn"]` as the mistake it exists to prevent — and the check runs after +`normalizeFilterOperator`, so an alias is judged by what it means (`nin` is +`not_in`, and keeps its array). Two classes are deliberately left alone: an +operator the spec does not know is still passed through verbatim, because the +misspelling is already the loud failure and refusing here would report the wrong +problem; and the valueless operators (`is_null`, `is_empty`, …) are not refused, +because the spec discards their value anyway +(`parseFilterAST(['tags', 'is_null', ['a']])` is `{ tags: { $null: true } }`), so +a stray array there cannot select the wrong rows. A pin holds the four arity +classes to an exact partition of `VIEW_FILTER_OPERATORS`, so a new spec operator +reddens rather than silently inheriting a verdict. + +The refusal is a `FilterOperatorError` (`INVALID_FILTER` / 400), which means a +saved view with one bad rule now fails at render rather than returning a +narrower result. That is not a new blast radius: both sinks already catch this +error class from this same file — `plugin-list`'s `buildEffectiveFilter` and +`plugin-view`'s `ObjectView` each call it inside their load `try` — and +`classifyLoadError` reads the code and status, so what a user sees is the +"filter is malformed" panel rather than a network fault or a crashed page. The +alternatives are both silent: dropping the rule widens the result set, and +rewriting `equals` into `in` changes what the saved view means. diff --git a/packages/core/src/utils/__tests__/filter-date-comparand-8555.test.ts b/packages/core/src/utils/__tests__/filter-date-comparand-8555.test.ts new file mode 100644 index 0000000000..31cfd4bf6d --- /dev/null +++ b/packages/core/src/utils/__tests__/filter-date-comparand-8555.test.ts @@ -0,0 +1,265 @@ +/** + * 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. + */ + +/** + * A `Date` comparand through `convertFiltersToAST` — objectui#8555. + * + * ## The defect + * + * The operator-object arm opened on `typeof value === 'object' && + * !Array.isArray(value)`. A `Date` passes both tests, so it entered the + * operator loop — and `Object.entries(someDate)` is `[]`, so the loop body + * never ran and NO condition was pushed for that field. + * + * Not refused, not lowered wrongly: ABSENT. `{ status: 'a', created: someDate }` + * lowered to `['status', '=', 'a']`, which is a WIDER result set than the author + * asked for, with nothing thrown and nothing logged — the one failure direction + * this file exists to avoid. + * + * It also made the field's fate depend on its SIBLINGS. With the Date alone, + * `conditions` ended empty and `convertFiltersToAST` returned the ORIGINAL + * OBJECT untouched, so the shape survived by accident and the defect only + * appeared once a second field was present. That asymmetry is part of the bug; + * section 2 is what pins it closed. + * + * ## The ruling — LOWER it, and the spec is what decides that + * + * This is the OPPOSITE answer to objectui#8514, which was resolved as a refusal + * precisely because the spec DECLINED to rule on that shape. Here it rules, + * measured against `@objectstack/spec` 17.3.0 and pinned in section 3: + * + * - `ACCEPTED_FILTER_COMPARAND_TYPES` is + * `['string','number','bigint','boolean','null','Date']` — `Date` is a + * first-class literal comparand, and `isAcceptedFilterComparand(new Date())` + * is `true`. + * - `parseFilterAST(['created', '=', d])` hands back `{ created: d }` with the + * Date INSTANCE intact. + * + * So the wire-form question the card raised ("ISO string? epoch?") is answered + * by NOT answering it here: the AST leaf carries the `Date`, exactly as the + * operator arm has always emitted it for `{ created: { $gte: d } }`. + * Stringifying in this arm would make the shorthand and the operator form emit + * two different comparand types for one author intent. + * + * ## What carries the weight — three legs, RUN rather than predicted + * + * Each was applied to the committed implementation, proved on disk in both + * directions, and restored by state. The counts and MODES below are measured, + * not expected — one of them corrected the prediction written here first. + * + * 1. **Ablation** — the arm removed. 7 of 16 red. MODE: a MISSING condition. + * `{ status, created }` produces `['status', '=', 'a']` and `node[2]` is + * `undefined`. Sections 1 and 2 both carry it. + * 2. **`value.toISOString()`** — the plausible wrong fix, since "what wire + * form?" is the question the card asks. 6 of 16 red. MODE: the right node + * with the WRONG COMPARAND TYPE — `'2026-01-01T00:00:00.000Z'` where a Date + * was expected. + * ⚠️ This paragraph first claimed only the `instanceof Date` and + * operator-parity pins would discriminate. Measured, six do: every + * `toEqual([..., D])` in sections 1 and 2 reddens too, because a string is + * not deep-equal to a Date. The correction that matters runs the other way — + * §2's "lone and sibling forms lower the SAME field" pin does NOT redden + * here, because the caricature is symmetric too. Symmetry alone never + * discriminates a wrong comparand type; the `instanceof` / `toBe(D)` pins + * are what do, and they are in their own `it()` blocks so no earlier + * assertion can stop them running (objectui#8506, objectui#8514). + * 3. **`Object.keys(value).length === 0` as the gate** — handles the Date and + * reads EVERY entry-less object as an equality comparand. 2 of 16 red, both + * in section 4, and NOTHING else: for a Date input this caricature is + * byte-identical to the shipped fix, which is exactly why section 4 has to + * exist. MODE: over-promotion — `{}` and `/x/` appear in comparand position + * (`['created', '=', {}]`), and a RegExp comparand is one + * `normalizeFilterComparandTypes` refuses outright, so the caricature moves + * the failure downstream instead of removing it. + */ + +import { describe, it, expect } from 'vitest'; +import { + isFilterAST, + parseFilterAST, + isAcceptedFilterComparand, + ACCEPTED_FILTER_COMPARAND_TYPES, +} from '@objectstack/spec/data'; +import { + convertFiltersToAST, + toFilterNode, + mergeFilterNodes, +} from '../filter-converter'; + +const D = new Date('2026-01-01T00:00:00.000Z'); + +// --------------------------------------------------------------------------- +// 1. The Date is lowered — as a Date, not as a string this layer invented +// --------------------------------------------------------------------------- + +describe('objectui#8555 — a Date comparand lowers to an equality node', () => { + it('lowers { status, created } to BOTH conditions — the sibling no longer eats the Date', () => { + // The whole node, so the pre-fix failure MODE is legible in the diff: the + // old emission was `['status', '=', 'a']` — one condition, not two, and no + // group at all. + const node = convertFiltersToAST({ status: 'a', created: D }); + expect(node).toEqual(['and', ['status', '=', 'a'], ['created', '=', D]]); + }); + + it('emits the Date INSTANCE, not an ISO string', () => { + // Its own block on purpose: this is the single assertion that separates the + // shipped fix from `value.toISOString()`, and an earlier failing assertion + // in a shared block would stop it ever running (objectui#8506 / #8514). + const node = convertFiltersToAST({ created: D }) as [string, string, unknown]; + expect(node[2]).toBeInstanceOf(Date); + expect(node[2]).toBe(D); + expect(typeof node[2]).not.toBe('string'); + }); + + it('emits the SAME comparand the operator arm has always emitted', () => { + // Parity with `{ created: { $gte: d } }` is the reason the shorthand is not + // stringified: one author intent, one comparand type. A stringifying fix + // breaks this and nothing else. + const shorthand = convertFiltersToAST({ created: D }) as [string, string, unknown]; + const operatorForm = convertFiltersToAST({ created: { $gte: D } }) as [string, string, unknown]; + expect(shorthand[2]).toBe(operatorForm[2]); + }); + + it('produces a node the spec accepts, with the Date intact through its doors', () => { + const node = convertFiltersToAST({ created: D }); + expect(isFilterAST(node)).toBe(true); + const parsed = parseFilterAST(node) as { created: unknown }; + expect(parsed).toEqual({ created: D }); + expect(parsed.created).toBeInstanceOf(Date); + }); + + it('lowers a Date inside $and / $or and through both public sinks', () => { + expect(convertFiltersToAST({ $or: [{ created: D }, { status: 'open' }] })) + .toEqual(['or', ['created', '=', D], ['status', '=', 'open']]); + expect(toFilterNode({ status: 'a', created: D })) + .toEqual(['and', ['status', '=', 'a'], ['created', '=', D]]); + // One AST-node source beside the object source; `mergeFilterNodes` keeps + // each source as its own child of the `and`. + expect(mergeFilterNodes({ created: D }, ['stage', '=', 'won'])) + .toEqual(['and', ['created', '=', D], ['stage', '=', 'won']]); + }); +}); + +// --------------------------------------------------------------------------- +// 2. The ASYMMETRY — a Date ALONE behaves the same as a Date with siblings +// --------------------------------------------------------------------------- + +describe('objectui#8555 — a lone Date is lowered, not handed back unchanged', () => { + it('no longer returns the original filter object when the Date is the only field', () => { + // Pre-fix: `conditions` was empty, so the `if (conditions.length === 0)` + // fallback returned the INPUT OBJECT — `isFilterAST` false, and the wire + // answers 400. The defect hid here, because the shape came back "unharmed". + const node = convertFiltersToAST({ created: D }); + expect(node).toEqual(['created', '=', D]); + expect(node).not.toBe(undefined); + expect(Array.isArray(node)).toBe(true); + expect(isFilterAST(node)).toBe(true); + }); + + it('lone and sibling forms lower the SAME field to the SAME condition', () => { + const alone = convertFiltersToAST({ created: D }); + const withSibling = convertFiltersToAST({ status: 'a', created: D }) as unknown[]; + // The `created` child of the group is byte-for-byte the lone emission. + expect(withSibling[2]).toEqual(alone); + }); +}); + +// --------------------------------------------------------------------------- +// 3. Why LOWER and not REFUSE — the spec's own ruling, pinned +// --------------------------------------------------------------------------- + +describe('objectui#8555 — the spec rules Date IN, which is why this is not a refusal', () => { + it('declares Date one of its six accepted literal comparand types', () => { + expect(ACCEPTED_FILTER_COMPARAND_TYPES).toContain('Date'); + expect(isAcceptedFilterComparand(D)).toBe(true); + }); + + it('accepts a Date in the AST leaf and keeps it a Date', () => { + // The wire-form question, answered by the spec rather than by this adapter. + expect(isFilterAST(['created', '=', D])).toBe(true); + expect((parseFilterAST(['created', '=', D]) as { created: unknown }).created) + .toBeInstanceOf(Date); + }); + + it('has Date as the ONLY object-typed member, so this arm is a Date arm', () => { + // The gate is `isAcceptedFilterComparand`, not `instanceof Date`. This pin + // is what makes that safe to read: every OTHER accepted comparand type is a + // primitive and can never reach the `typeof value === 'object'` branch. If + // the spec ever adds a second object-shaped literal, this reddens and the + // reader learns the arm's reach just widened. + const objectTyped = ACCEPTED_FILTER_COMPARAND_TYPES.filter( + (t) => t !== 'string' && t !== 'number' && t !== 'bigint' && t !== 'boolean' && t !== 'null', + ); + expect(objectTyped).toEqual(['Date']); + }); +}); + +// --------------------------------------------------------------------------- +// 4. NON-regression — the operator-object path `Object.entries` is there for. +// This is the axis that separates the fix from "anything entry-less is a +// comparand". Every case asserts the PRODUCED node. +// --------------------------------------------------------------------------- + +describe('objectui#8555 — operator objects lower exactly as before', () => { + it('$gt / $gte / $lt / $lte still reach the operator loop', () => { + expect(convertFiltersToAST({ age: { $gt: 26 } })).toEqual(['age', '>', 26]); + expect(convertFiltersToAST({ age: { $gte: 18, $lte: 65 } })) + .toEqual(['and', ['age', '>=', 18], ['age', '<=', 65]]); + }); + + it('an operator object carrying a Date value still lowers through the loop', () => { + // The Date arm must not swallow `{ created: { $gte: d } }`: the VALUE of the + // field is a plain object there, and only its member is a Date. + expect(convertFiltersToAST({ created: { $gte: D } })).toEqual(['created', '>=', D]); + expect(convertFiltersToAST({ created: { $between: [D, D] } })) + .toEqual(['created', 'between', [D, D]]); + }); + + it('$in / $nin / $null / $exists and the refusals are untouched', () => { + expect(convertFiltersToAST({ status: { $in: ['a', 'b'] } })).toEqual(['status', 'in', ['a', 'b']]); + expect(convertFiltersToAST({ deleted: { $null: true } })).toEqual(['deleted', 'is_null', true]); + expect(convertFiltersToAST({ deleted: { $exists: false } })).toEqual(['deleted', 'is_null', true]); + expect(() => convertFiltersToAST({ name: { $regex: 'a.c' } })).toThrow(/\$regex/); + expect(() => convertFiltersToAST({ tags: ['a'] })).toThrow(/bare ARRAY/); + }); + + it('an EMPTY operator object still constrains nothing — it is not a comparand', () => { + // `{}` has no entries either, so a fix gated on `Object.keys(value).length + // === 0` would lower it to `['created', '=', {}]`. It must stay the TRUE + // identity it has always been: no operators means no constraint, the same + // reading `{ $and: [] }` gets in `lowerLogicalGroup`. + expect(convertFiltersToAST({ status: 'a', created: {} })).toEqual(['status', '=', 'a']); + }); + + it('a non-Date exotic object is NOT promoted into comparand position', () => { + // A `RegExp` is entry-less too, so the same `Object.keys` caricature would + // emit `['created', '=', /x/]` — a comparand `normalizeFilterComparandTypes` + // refuses outright (INVALID_FILTER / 400), i.e. the failure moves downstream + // rather than going away. + // + // ⚠️ This pins the NEGATIVE only. What this arm does with a RegExp today is + // the same silent drop objectui#8555 describes, left in place deliberately: + // the spec rules Date IN and rules RegExp OUT, so the two shapes need + // different answers and only one of them was in scope. Filed separately. + expect(isAcceptedFilterComparand(/x/)).toBe(false); + expect(convertFiltersToAST({ status: 'a', created: /x/ })) + .not.toEqual(['and', ['status', '=', 'a'], ['created', '=', /x/]]); + }); + + it('plain scalars, null and undefined are exactly what they were', () => { + expect(convertFiltersToAST({ status: 'active' })).toEqual(['status', '=', 'active']); + expect(convertFiltersToAST({ count: 0, ok: false })).toEqual([ + 'and', + ['count', '=', 0], + ['ok', '=', false], + ]); + // A null/undefined value is skipped before any of this, and an all-skipped + // filter still returns the original object. + expect(convertFiltersToAST({ a: null, b: undefined })).toEqual({ a: null, b: undefined }); + }); +}); diff --git a/packages/core/src/utils/__tests__/filter-view-rule-arity-8557.test.ts b/packages/core/src/utils/__tests__/filter-view-rule-arity-8557.test.ts new file mode 100644 index 0000000000..176d6d7cfe --- /dev/null +++ b/packages/core/src/utils/__tests__/filter-view-rule-arity-8557.test.ts @@ -0,0 +1,317 @@ +/** + * 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. + */ + +/** + * An ARRAY on a single-value operator in the STORED-VIEW arm — objectui#8557. + * + * ## The defect + * + * `viewFilterRuleToNode` never inspected `rule.value`, so a saved view rule + * carrying an array on a scalar operator travelled through verbatim: + * + * `toFilterNode([{ field: 'tags', operator: 'equals', value: ['a'] }])` + * → `[['tags', 'equals', ['a']]]` + * → `parseFilterAST` → `{ tags: ['a'] }` + * + * Section 1 pins that the spec's doors accept that node unjudged (measured + * against `@objectstack/spec` 17.3.0), which is why the refusal used to arrive + * two layers away as a `400 INVALID_FILTER` from `driver-sql`, or as an empty + * list from an in-memory matcher. + * + * It is the SAME array-in-a-scalar-slot shape objectui#8530 / PR #8551 refused + * in `convertFiltersToAST`'s object arm. That fix deliberately did not reach + * here — the two shapes enter the lowering by different doors — so a + * hand-authored `{ tags: ['a'] }` failed fast with a message naming `$in` while + * the same mistake SAVED INTO A VIEW stayed silent. This closes that asymmetry. + * + * ## The two things the card said must be got right + * + * 1. **Keyed on the operator's ARITY, never on `Array.isArray(value)`.** `in`, + * `not_in` and `between` legitimately carry arrays through this very + * function, and that path had no pin protecting it. Section 3 is that pin, + * and it is the axis that separates the fix from "refuse anything + * array-shaped". The arity comes from the spec's own exported sets, so this + * file also pins (section 4) that the four arity classes PARTITION + * `VIEW_FILTER_OPERATORS` exactly — an operator added to the spec falls into + * no class, that pin reddens, and someone classifies it. + * 2. **Where the refusal belongs.** It throws from the lowering. Section 5 + * records the measurement that made that safe: both sinks already catch a + * `FilterOperatorError` from this same file, so a malformed saved view lands + * in the list's "filter is malformed" panel — the blast radius the object arm + * has had since objectui#8530, not a new one. + * + * ## Legs, RUN rather than predicted + * + * Applied to the committed implementation, proved on disk both ways, restored + * by state. Counts and MODES below are measured, not expected. + * + * - **Ablation** (the arm removed): 7 of 16 red — all of section 2 AND + * section 5's envelope-parity pin, which this paragraph did not predict. + * MODE: no throw at all — `captureRefusal` fails on its own first line, + * printing the node that travelled through (`[['tags','equals',['a']]]`), + * so nothing downstream of it runs on a stale assumption. + * - **Caricature `Array.isArray(value)` alone** (the card's named one, no + * arity key): 6 of 16 red, ALL in section 3 — and section 2 stays entirely + * green. That is the shape of the warning: the caricature passes every + * "the array is now refused" assertion and eats `in` / `not_in` / + * `between` with it. MODE: a refusal where a lowering was expected. + * - **Caricature "refuse on every known operator"** (the valueless class not + * carved out): exactly 1 of 16 red — the `is_null` pin in section 3, the + * single assertion written for it, and nothing else. + * + * Refusal pins assert the envelope on a CAPTURED error AND the message's first + * sentence. Envelope-only is not sufficient in this file: objectui#8530 saw + * `code` + `httpStatus` go green for the wrong reason, on a refusal that was + * really "Unknown filter operator 0". + */ + +import { describe, it, expect } from 'vitest'; +import { isFilterAST, parseFilterAST } from '@objectstack/spec/data'; +import { + VIEW_FILTER_OPERATORS, + VIEW_FILTER_LIST_VALUE_OPERATORS, + VIEW_FILTER_PAIR_VALUE_OPERATORS, +} from '@objectstack/spec/ui'; +import { toFilterNode, mergeFilterNodes, FilterOperatorError } from '../filter-converter'; + +/** + * Run the lowering and hand back the refusal it raised. Fails on the FIRST + * line, printing what was produced, when it did not refuse — so no later + * assertion is silently skipped by a non-throwing lowering. + */ +function captureRefusal(run: () => unknown): FilterOperatorError { + let caught: unknown; + let produced: unknown; + try { + produced = run(); + } catch (e) { + caught = e; + } + expect( + caught, + `expected the lowering to refuse, it produced ${JSON.stringify(produced)}`, + ).toBeInstanceOf(FilterOperatorError); + return caught as FilterOperatorError; +} + +const rule = (operator: string, value?: unknown) => + value === undefined ? { field: 'tags', operator } : { field: 'tags', operator, value }; + +// --------------------------------------------------------------------------- +// 1. Why the producer must refuse: the spec doors pass the old node unjudged +// --------------------------------------------------------------------------- + +describe('objectui#8557 — the pre-fix node reaches the wire unjudged', () => { + it('is accepted by isFilterAST and lowered to an array comparand', () => { + // If the spec ever starts refusing this, the pin reddens and the reader + // learns the refusal gained a sibling — not that this arm can go. + const preFixNode = ['tags', 'equals', ['a']]; + expect(isFilterAST(preFixNode)).toBe(true); + expect(parseFilterAST(preFixNode)).toEqual({ tags: ['a'] }); + }); +}); + +// --------------------------------------------------------------------------- +// 2. The refusal — envelope AND first sentence, on a captured error +// --------------------------------------------------------------------------- + +describe('objectui#8557 — an array on a single-value operator is refused', () => { + it('refuses the card`s measured case with the INVALID_FILTER / 400 envelope', () => { + const err = captureRefusal(() => toFilterNode([rule('equals', ['a'])])); + expect(err.code).toBe('INVALID_FILTER'); + expect(err.httpStatus).toBe(400); + expect(err.name).toBe('FilterOperatorError'); + }); + + it('names the field, the operator and the comparand in its first sentence', () => { + // Its own block: objectui#8530 measured an envelope pin going green for the + // wrong reason ("Unknown filter operator 0"), and only the first-sentence + // assertion discriminated. Separate `it()` so nothing reddens ahead of it. + const err = captureRefusal(() => toFilterNode([rule('equals', ['a', 'b'])])); + expect(err.message).toMatch( + /^\[ObjectUI\] The stored view rule on field 'tags' carries an ARRAY as the comparand of 'equals', which takes a single value: \["a","b"\]\./, + ); + }); + + it('prescribes the spellings that work and says it did not rewrite to `in`', () => { + const err = captureRefusal(() => toFilterNode([rule('equals', ['a'])])); + expect(err.message).toContain("{ field: 'tags', operator: 'in', value: [...] }"); + expect(err.message).toContain("'not_in'"); + expect(err.message).toContain("{ operator: 'between', value: [min, max] }"); + expect(err.message).toMatch(/NOT rewritten to 'in'/); + expect(err.message).toContain('objectui#8557'); + // Not the sibling diagnostics — this is neither an unknown operator, nor a + // combinator problem, nor the object arm's bare-array refusal. + expect(err.message).not.toMatch(/Unknown filter operator/); + expect(err.message).not.toMatch(/combinator/); + expect(err.message).not.toMatch(/bare ARRAY/); + }); + + it('judges the operator by what it MEANS — an alias is normalized first', () => { + // `eq` is an alias of `equals`; it must be refused the same way, and the + // message must name the canonical spelling rather than what was stored. + const err = captureRefusal(() => toFilterNode([rule('eq', ['a'])])); + expect(err.message).toContain("comparand of 'equals'"); + }); + + it('refuses on every single-value operator the spec declares, not just equals', () => { + for (const op of ['not_equals', 'contains', 'starts_with', 'greater_than', 'before', 'icontains']) { + const err = captureRefusal(() => toFilterNode([rule(op, ['a'])])); + expect(err.code, `operator ${op}`).toBe('INVALID_FILTER'); + expect(err.message, `operator ${op}`).toContain(`comparand of '${op}'`); + } + }); + + it('refuses the empty array too, and wherever the rule sits', () => { + // `[]` is exactly as unanswerable as a populated one, and reading it as + // "no constraint" would widen the result set. + expect(captureRefusal(() => toFilterNode([rule('equals', [])])).message) + .toContain("which takes a single value: []."); + // Mixed source: rules concatenated with URL triples, as ObjectView builds them. + expect(captureRefusal( + () => toFilterNode([['stage', '=', 'won'], rule('equals', ['a'])]), + ).code).toBe('INVALID_FILTER'); + // Through the sink every renderer actually calls. + expect(captureRefusal( + () => mergeFilterNodes({ status: 'x' }, [rule('equals', ['a'])]), + ).message).toContain("field 'tags'"); + }); +}); + +// --------------------------------------------------------------------------- +// 3. NON-regression — the arity axis. This is what separates the fix from +// "refuse anything array-shaped", and the path the card said had no pin. +// --------------------------------------------------------------------------- + +describe('objectui#8557 — array-valued operators keep their arrays', () => { + it('`in` lowers with its member list intact, through the spec doors', () => { + const node = toFilterNode([rule('in', ['a', 'b'])]); + expect(node).toEqual([['tags', 'in', ['a', 'b']]]); + expect(isFilterAST(node)).toBe(true); + expect(parseFilterAST(['tags', 'in', ['a', 'b']])).toEqual({ tags: { $in: ['a', 'b'] } }); + }); + + it('`not_in` lowers — and so does its `nin` alias, which normalizes into it', () => { + expect(toFilterNode([rule('not_in', ['a'])])).toEqual([['tags', 'not_in', ['a']]]); + // The alias is the case a hard-coded `["in", "notIn"]` list gets wrong; the + // gate reads the NORMALIZED operator, so it cannot. + expect(toFilterNode([rule('nin', ['a'])])).toEqual([['tags', 'not_in', ['a']]]); + }); + + it('`between` lowers with its [min, max] pair', () => { + const node = toFilterNode([{ field: 'amount', operator: 'between', value: [1, 10] }]); + expect(node).toEqual([['amount', 'between', [1, 10]]]); + expect(isFilterAST(node)).toBe(true); + expect(parseFilterAST(['amount', 'between', [1, 10]])).toEqual({ amount: { $between: [1, 10] } }); + }); + + it('an operator the spec does NOT know is still passed through verbatim', () => { + // A misspelling is already the loud failure: `normalizeFilterOperator` leaves + // it alone so `isFilterAST` refuses it and the server names it. Refusing + // HERE would report "use $in" for what is actually a typo. + const node = toFilterNode([rule('bogus_op', ['a'])]); + expect(node).toEqual([['tags', 'bogus_op', ['a']]]); + expect(isFilterAST(['tags', 'bogus_op', ['a']])).toBe(false); + }); + + it('a valueless operator is not refused — the spec discards its value anyway', () => { + // Measured: `parseFilterAST(['tags', 'is_null', ['a']])` is + // `{ tags: { $null: true } }`. The stray array produces no wrong node, so + // refusing would turn a harmless input into a render-time throw — and would + // prescribe `in` for an operator that takes no value at all. + expect(toFilterNode([rule('is_null', ['a'])])).toEqual([['tags', 'is_null', ['a']]]); + expect(parseFilterAST(['tags', 'is_null', ['a']])).toEqual({ tags: { $null: true } }); + }); + + it('an AST GROUP node is not a rule, so the arity gate never sees its array', () => { + // objectui#8456 (`617707a48`) made `$and` / `$or` lower to real AST group + // nodes, and a group node carries an ARRAY of children legitimately. It + // cannot collide with this gate: `isViewFilterRule` requires a plain OBJECT + // with a non-empty string `field`, and every AST node is an ARRAY — so a + // group is passed through by `toFilterNode` without ever entering + // `viewFilterRuleToNode`. Pinned because the two features are one function + // apart and both are about arrays. + const group = ['or', ['status', '=', 'open'], ['status', '=', 'blocked']]; + expect(toFilterNode([group])).toEqual([group]); + // And mixed with a rule that IS lowered, which is how ObjectView builds them. + expect(toFilterNode([group, { field: 'amount', operator: 'in', value: [1, 2] }])) + .toEqual([group, ['amount', 'in', [1, 2]]]); + // The object arm still produces the group, unaffected by this card. + expect(mergeFilterNodes({ $or: [{ status: 'open' }, { status: 'blocked' }] })) + .toEqual(group); + }); + + it('scalar values on scalar operators, and valueless rules, are untouched', () => { + expect(toFilterNode([{ field: 'status', operator: 'equals', value: 'in_progress' }])) + .toEqual([['status', 'equals', 'in_progress']]); + // The shipped `showcase_task.in_progress` view — the case objectui#3431 was + // verified against a real backend. + expect(toFilterNode([rule('is_empty')])).toEqual([['tags', 'is_empty']]); + // A blank `field` is still not a rule, so it is still left unlowered. + expect(toFilterNode([{ field: '', operator: 'equals', value: ['a'] }])) + .toEqual([{ field: '', operator: 'equals', value: ['a'] }]); + }); +}); + +// --------------------------------------------------------------------------- +// 4. The arity classes PARTITION the spec's vocabulary — the ratchet +// --------------------------------------------------------------------------- + +describe('objectui#8557 — the arity classification cannot drift from the spec', () => { + it('assigns every VIEW_FILTER_OPERATORS member to exactly one arity class', () => { + // The list and pair classes are the spec's own exports. The valueless class + // is the one written out in the source, because the spec exports no set for + // it — so this pin is what keeps it honest. A new spec operator lands in the + // scalar remainder by default, which is the REFUSING class, so this pin is + // also the review trigger for that choice. + const valueless = ['is_empty', 'is_not_empty', 'is_null', 'is_not_null']; + const arrayValued: string[] = [ + ...VIEW_FILTER_LIST_VALUE_OPERATORS, + ...VIEW_FILTER_PAIR_VALUE_OPERATORS, + ]; + const all = [...VIEW_FILTER_OPERATORS] as string[]; + + for (const op of [...valueless, ...arrayValued]) expect(all).toContain(op); + expect(arrayValued).toEqual(['in', 'not_in', 'between']); + + const scalar = all.filter((op) => !valueless.includes(op) && !arrayValued.includes(op)); + // The refusing class, spelled out. If the spec adds an operator this list + // changes and the reader has to say which class it belongs to. + expect(scalar).toEqual([ + 'equals', 'not_equals', 'contains', 'not_contains', 'icontains', + 'starts_with', 'ends_with', 'greater_than', 'less_than', + 'greater_than_or_equal', 'less_than_or_equal', 'before', 'after', + ]); + // Exact partition: no operator in two classes, none in none. + expect(scalar.length + valueless.length + arrayValued.length).toBe(all.length); + }); +}); + +// --------------------------------------------------------------------------- +// 5. Where the refusal lands — the blast radius, as an envelope contract +// --------------------------------------------------------------------------- + +describe('objectui#8557 — the throw is the blast radius the object arm already had', () => { + it('raises the SAME error class and envelope as the object arm`s refusals', () => { + // `plugin-list`'s `buildEffectiveFilter` and `plugin-view`'s `ObjectView` + // both call this sink inside their load `try`, and have had to survive a + // throw from it since objectui#8530. `classifyLoadError` reads this code and + // status, which is what makes a malformed saved view render as "the filter + // is malformed" rather than as a network fault (objectui#3066). A bare + // `Error` here would classify as the latter. + const fromViewArm = captureRefusal(() => toFilterNode([rule('equals', ['a'])])); + const fromObjectArm = captureRefusal(() => toFilterNode({ tags: ['a'] })); + expect(fromViewArm.constructor).toBe(fromObjectArm.constructor); + expect(fromViewArm.code).toBe(fromObjectArm.code); + expect(fromViewArm.httpStatus).toBe(fromObjectArm.httpStatus); + // Same envelope, DIFFERENT diagnosis — the reader must be able to tell which + // door refused, because the fix differs (a rule to edit vs. a literal). + expect(fromViewArm.message).toContain('stored view rule'); + expect(fromObjectArm.message).not.toContain('stored view rule'); + }); +}); diff --git a/packages/core/src/utils/filter-converter.ts b/packages/core/src/utils/filter-converter.ts index 92cd70dc37..47d5aa378b 100644 --- a/packages/core/src/utils/filter-converter.ts +++ b/packages/core/src/utils/filter-converter.ts @@ -13,7 +13,13 @@ * to ObjectStack FilterNode AST format. */ -import { normalizeFilterOperator } from '@objectstack/spec/ui'; +import { + normalizeFilterOperator, + VIEW_FILTER_OPERATORS, + VIEW_FILTER_LIST_VALUE_OPERATORS, + VIEW_FILTER_PAIR_VALUE_OPERATORS, +} from '@objectstack/spec/ui'; +import { isAcceptedFilterComparand } from '@objectstack/spec/data'; /** * FilterNode AST type definition @@ -216,6 +222,13 @@ function falseIdentityLeaf(field: string, value: unknown[]): FilterNode { * convertFiltersToAST({ $or: [{ status: 'open' }, { status: 'blocked' }] }) * // => ['or', ['status', '=', 'open'], ['status', '=', 'blocked']] * + * @example + * // A Date is a comparand, not an operator map (objectui#8555). It lowers as + * // the Date INSTANCE — the spec accepts one, and the operator form already + * // emits one. + * convertFiltersToAST({ created: new Date('2026-01-01') }) + * // => ['created', '=', Date(2026-01-01)] + * * @throws {FilterOperatorError} If an unknown operator is encountered, if * `$not` is used — see the `$not` arm for why the AST cannot carry it — or if a * field's value is a bare ARRAY (`{ tags: ['a', 'b'] }`) — see the array arm for @@ -297,6 +310,50 @@ export function convertFiltersToAST(filter: Record): FilterNode | R // Check if value is a complex operator object if (typeof value === 'object' && !Array.isArray(value)) { + // A `Date` is a COMPARAND, not an operator map — objectui#8555. + // + // `typeof new Date()` is `'object'` and a Date is not an array, so it used + // to enter the loop below; `Object.entries(someDate)` is `[]`, the body + // never ran, and NO condition was pushed for the field. Not refused, not + // lowered wrongly — ABSENT, so `{ status: 'a', created: someDate }` + // lowered to `['status', '=', 'a']` and the result set got WIDER than the + // author asked for, silently. That is the one failure direction this file + // exists to avoid. It also made the field's behaviour depend on its + // SIBLINGS: a Date alone left `conditions` empty, so the original object + // came back untouched and the defect was invisible until a second field + // appeared. + // + // Lowered rather than refused, and the spec is what decides it — the + // opposite answer to objectui#8514, which was a refusal precisely because + // the spec DECLINED to rule on that shape. Here it rules, twice over + // (measured against @objectstack/spec 17.3.0): + // `ACCEPTED_FILTER_COMPARAND_TYPES` is + // `['string','number','bigint','boolean','null','Date']`, and + // `$gt`/`$gte`/`$lt`/`$lte`/`$between` declare `z.ZodDate` in comparand + // position. So a Date is a first-class filter comparand, not a shape this + // layer has to invent an answer for. + // + // ⛔ NOT converted to an ISO string or an epoch here. The wire form is not + // this adapter's question to answer: `parseFilterAST(['created', '=', d])` + // hands back `{ created: d }` with the Date INSTANCE intact (measured), and + // `normalizeFilterComparandTypes` accepts it as-is. The operator arm below + // already passes a Date through untouched (`{ created: { $gte: d } }` → + // `['created', '>=', d]`), so stringifying here would make the shorthand + // and the operator form emit two different comparand types for the same + // author intent — a second dialect, in the file whose whole job is to have + // one. + // + // The gate is the spec's own predicate rather than a local `instanceof + // Date`, the same reason `normalizeFilterOperator` is used below instead of + // a second operator map. Today `Date` is its only object-typed member + // (pinned in filter-date-comparand-8555.test.ts), so this arm is a Date arm + // in practice; if the spec ever accepts another object-shaped literal, this + // reads it as a comparand instead of silently dropping it. + if (isAcceptedFilterComparand(value)) { + conditions.push([field, '=', value]); + continue; + } + // Handle operator-based filters for (const [operator, operatorValue] of Object.entries(value)) { // `$regex` is refused, not downgraded. It used to become `contains` @@ -409,9 +466,135 @@ function isViewFilterRule(value: unknown): value is ViewFilterRuleLike { * 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)`). + * + * The value's SHAPE is checked too, against the operator's arity — an array on a + * single-value operator is refused rather than passed through. See the arm + * itself for the reasoning and for why the refusal is a throw (objectui#8557). + * + * @throws {FilterOperatorError} If the rule carries an ARRAY on an operator the + * spec declares single-valued. */ +/** + * The view-filter operators whose `value` is legitimately an ARRAY. + * + * Not a local list — both halves are the spec's own, and it exports them for + * exactly this question: `VIEW_FILTER_LIST_VALUE_OPERATORS` (`['in', 'not_in']`) + * says which operators take a membership list, `VIEW_FILTER_PAIR_VALUE_OPERATORS` + * (`['between']`) which take a `[min, max]` pair, and the docblock on the first + * of them names a hard-coded `["in", "notIn"]` elsewhere in this repo as the + * mistake it exists to prevent (`notIn` is an alias, not the canonical member). + * Same reason the operator itself goes through `normalizeFilterOperator` rather + * than a second map. + */ +const ARRAY_VALUED_VIEW_OPERATORS: ReadonlySet = new Set([ + ...VIEW_FILTER_LIST_VALUE_OPERATORS, + ...VIEW_FILTER_PAIR_VALUE_OPERATORS, +]); + +/** + * The view-filter operators that carry NO comparand — their direction comes from + * the operator NAME. + * + * The spec exports the two array-valued sets but no set for these, so this is + * the one classification written out here. It is pinned against + * `VIEW_FILTER_OPERATORS` (filter-view-rule-arity-8557.test.ts) so the four sets + * partition the vocabulary exactly: an operator added to the spec lands in no + * class, the pin reddens, and someone classifies it rather than it silently + * inheriting a verdict. + * + * They are excluded from the refusal below deliberately, and on a measurement: + * the spec DISCARDS a value on these operators — `parseFilterAST(['tags', + * 'is_null', ['a']])` is `{ tags: { $null: true } }` — so a stray array here + * cannot produce a node that selects the wrong rows. Refusing it would turn a + * harmless input into a render-time throw, and the message would prescribe `in` + * for an operator that takes no value at all. + */ +const VALUELESS_VIEW_OPERATORS: ReadonlySet = new Set([ + 'is_empty', + 'is_not_empty', + 'is_null', + 'is_not_null', +]); + +/** Every operator spelling the spec knows, canonical forms only. */ +const KNOWN_VIEW_OPERATORS: ReadonlySet = new Set(VIEW_FILTER_OPERATORS); + function viewFilterRuleToNode(rule: ViewFilterRuleLike): FilterNode { const operator = normalizeFilterOperator(rule.operator as string); + + // An ARRAY on an operator that takes ONE value is refused here — objectui#8557. + // + // `rule.value` used to travel through unread, so a stored view rule + // `{ field: 'tags', operator: 'equals', value: ['a'] }` became + // `['tags', 'equals', ['a']]`. Measured against @objectstack/spec 17.3.0, the + // spec's doors pass that node through unjudged — `isFilterAST` is `true` and + // `parseFilterAST` hands back `{ tags: ['a'] }` — which is the SAME + // array-in-a-scalar-slot shape the object arm above refuses (objectui#8530): + // the ObjectQL AST has no array-equality node, `@objectstack/driver-sql` + // answers `400 INVALID_FILTER`, and every in-memory matcher excludes every + // row. So a hand-authored `{ tags: ['a'] }` already failed fast with a message + // naming `$in`, while the same mistake SAVED INTO A VIEW still travelled + // silently to a 400 the author could not attribute to their filter. This + // closes that asymmetry. + // + // Keyed on the operator's ARITY, never on `Array.isArray(value)` alone: + // `in` / `not_in` / `between` rules legitimately carry arrays through this + // very function, and that path had no pin protecting it until this card. + // The check runs AFTER normalization so an alias is judged by what it means — + // `nin` is `not_in`, and is array-valued. + // + // An operator the spec does NOT know is left alone. `normalizeFilterOperator` + // passes a misspelling through verbatim precisely so `isFilterAST` refuses it + // and the server names it; refusing it HERE would report the wrong problem + // ("use $in") for what is actually a typo. + // + // ## Why the refusal is a THROW, and why it belongs here + // + // It throws from the lowering, which for a saved view means the view fails at + // render rather than returning a narrower answer. That is a real blast radius + // and it was weighed: + // + // - It is not a NEW blast radius. Both sinks already catch a + // `FilterOperatorError` from this same file — `plugin-list`'s + // `buildEffectiveFilter` runs inside `ListView`'s load `try` and lands in + // `setLoadError`, `plugin-view`'s `ObjectView` calls `mergeFilterNodes` + // inside its own load `try` — because the object arm has thrown for + // `$regex`, `$not` and bare arrays since objectui#8530. `classifyLoadError` + // reads this error's `INVALID_FILTER` / `400`, so what a user sees is the + // "filter is malformed" panel, not a crashed page and not a network error. + // - The alternatives are both silent. DROPPING the rule widens the result + // set, the one direction this file exists to avoid — and a stored view's + // whole purpose can be to hide rows. Rewriting `equals` into `in` changes + // the author's meaning, the lenient second contract objectui#8514 was + // resolved against on this same data shape. + // - The rule was authored long ago by someone who is not present, which + // argues for the LOUDER answer, not the quieter one: a silent 400 two + // layers away is attributable to nothing, whereas this names the view's + // field and operator at the moment the filter is built. + if ( + Array.isArray(rule.value) + && typeof operator === 'string' + && KNOWN_VIEW_OPERATORS.has(operator) + && !ARRAY_VALUED_VIEW_OPERATORS.has(operator) + && !VALUELESS_VIEW_OPERATORS.has(operator) + ) { + throw new FilterOperatorError( + `[ObjectUI] The stored view rule on field '${rule.field}' carries an ARRAY ` + + `as the comparand of '${operator}', which takes a single value: ` + + `${JSON.stringify(rule.value)}. It cannot be lowered: the ObjectQL filter ` + + `AST has no array-equality node, so the lowered node ` + + `[${rule.field}, '${operator}', [...]] is refused by @objectstack/driver-sql ` + + `(400 INVALID_FILTER) and matches no row in the in-memory matchers — it can ` + + `never select anything. It is deliberately NOT rewritten to 'in': that ` + + `would change what the saved view means, and 'in' is already spellable in ` + + `the rule itself. Spell membership as ` + + `{ field: '${rule.field}', operator: 'in', value: [...] }, its negation as ` + + `'not_in', or a range as { operator: 'between', value: [min, max] } — the ` + + `three operators the spec declares array-valued, which are untouched ` + + `(objectui#8557; the same ruling objectui#8530 applied to the object arm).` + ); + } + return ( rule.value === undefined ? [rule.field, operator]