diff --git a/.changeset/view-ast-operator-parity.md b/.changeset/view-ast-operator-parity.md new file mode 100644 index 0000000000..975dc6a003 --- /dev/null +++ b/.changeset/view-ast-operator-parity.md @@ -0,0 +1,41 @@ +--- +"@objectstack/spec": minor +"@objectstack/driver-sql": patch +"@objectstack/driver-memory": patch +--- + +fix(spec,drivers): the view filter vocabulary and the AST vocabulary now agree (#3948) + +`VIEW_FILTER_OPERATORS` (`ui/view.zod.ts`) is what an author may declare on a +`ViewFilterRule`. `VALID_AST_OPERATORS` (`data/filter.zod.ts`) gates +`isFilterAST()`, which decides whether a filter is parsed into a query at all. +They disagreed on **8 of 19** members: `equals`, `not_equals`, `greater_than`, +`less_than`, `greater_than_or_equal`, `less_than_or_equal`, `before`, `after`. + +An author could declare any of them, `ViewFilterRuleSchema` validated them, +`defineStack` accepted them — and then `isFilterAST()` refused the filter, the +protocol passed the array through unconverted, and the driver could not apply it. +Six of the eight were reachable only in theory because ObjectUI's adapter alias +table happened to translate them; the safety of the query path was resting on a +hand-written table in another repository being complete, and for `before`/`after` +it wasn't. + +**`AST_OPERATOR_MAP` is now the single source of truth.** `VALID_AST_OPERATORS` +is derived from its keys rather than restated, so an operator can no longer be +accepted by the gate without also having a lowering — the two were separate +hand-written lists that happened to agree, with nothing enforcing it. The map +gained the eight canonical view spellings plus the squashed/short forms stored +metadata carries (`notequals`, `greaterthanorequal`, `eq`, `gt`, …). + +**New export `canonicalAstOperator(op)`** folds every accepted spelling of one +comparison onto a single infix form. Both drivers now call it instead of growing +private alias lists, which is what let them accept different vocabularies. +`like`/`ilike` are deliberately not folded onto `contains`: driver-sql passes them +to SQL verbatim, so folding would silently wrap the value in `%…%`. + +Widening only — no spelling was removed, so no stored filter stops validating. +A filter that previously produced an error (after #4029) or was silently dropped +(before it) now compiles. `filter-view-operator-parity.test.ts` asserts every +`VIEW_FILTER_OPERATORS` member and every `VIEW_FILTER_OPERATOR_ALIASES` key has a +lowering that is a real `$`-operator rather than the `$${op}` fallback, so the +next operator the view layer gains fails a test instead of a query. diff --git a/packages/plugins/driver-memory/src/memory-driver.ts b/packages/plugins/driver-memory/src/memory-driver.ts index 969a6cc5ff..5c836fb6ec 100644 --- a/packages/plugins/driver-memory/src/memory-driver.ts +++ b/packages/plugins/driver-memory/src/memory-driver.ts @@ -1,6 +1,7 @@ // Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. import type { QueryAST, QueryInput, DriverOptions } from '@objectstack/spec/data'; +import { canonicalAstOperator } from '@objectstack/spec/data'; import type { IDataDriver } from '@objectstack/spec/contracts'; import { Logger, createLogger } from '@objectstack/core'; import { Query, Aggregator } from 'mingo'; @@ -760,7 +761,12 @@ export class InMemoryDriver implements IDataDriver { * Convert a single ObjectQL condition to MongoDB operator format. */ private convertConditionToMongo(field: string, operator: string, value: any): Record | null { - switch (operator) { + // Fold every accepted spelling of one comparison onto a single infix form, + // so this switch has one case per comparison rather than one per spelling — + // `VALID_AST_OPERATORS` accepts `>`, `gt`, `greater_than`, `greaterthan` and + // `after` for the same thing. A private alias list here is what let this + // driver and driver-sql accept different vocabularies. #3948. + switch (canonicalAstOperator(operator)) { case '=': case '==': return { [field]: value }; case '!=': case '<>': diff --git a/packages/plugins/driver-sql/src/sql-driver.ts b/packages/plugins/driver-sql/src/sql-driver.ts index 214cc15785..a9b0f79533 100644 --- a/packages/plugins/driver-sql/src/sql-driver.ts +++ b/packages/plugins/driver-sql/src/sql-driver.ts @@ -10,6 +10,7 @@ import type { QueryAST, DriverOptions, SchemaMode } from '@objectstack/spec/data'; import { parseAutonumberFormat, renderAutonumber, missingFieldValues, isTenancyDisabled, isGlobalUnique, isUniqueDeclared, type AutonumberToken } from '@objectstack/spec/data'; import { STRUCTURED_JSON_TYPES, FILE_REFERENCE_TYPES, MULTI_OPTION_TYPES, NUMERIC_VALUE_TYPES } from '@objectstack/spec/data'; +import { canonicalAstOperator } from '@objectstack/spec/data'; import type { IDataDriver } from '@objectstack/spec/contracts'; import { StorageNameMapping } from '@objectstack/spec/system'; import { ExternalSchemaModeViolationError } from '@objectstack/spec/shared'; @@ -4766,7 +4767,12 @@ export class SqlDriver implements IDataDriver { const where = join === 'or' ? 'orWhere' : 'where'; const whereNull = join === 'or' ? 'orWhereNull' : 'whereNull'; const whereNotNull = join === 'or' ? 'orWhereNotNull' : 'whereNotNull'; - const opLower = String(op).toLowerCase(); + // Fold every accepted spelling of one comparison onto a single infix form so + // the switch below has one case per comparison rather than one per spelling. + // `VALID_AST_OPERATORS` accepts `>`, `gt`, `greater_than`, `greaterthan` and + // `after` for the same thing; growing a private alias list here is how this + // driver and driver-memory drifted apart. #3948. + const opLower = canonicalAstOperator(String(op)); // Value comparisons on a mixed-storage column read it through the CASE; every // other operator (null predicates, the LIKE family, a malformed `between`) diff --git a/packages/spec/api-surface.json b/packages/spec/api-surface.json index 10b9a430cf..ddfdb80167 100644 --- a/packages/spec/api-surface.json +++ b/packages/spec/api-surface.json @@ -549,6 +549,7 @@ "WindowFunctionNodeSchema (const)", "WindowSpec (type)", "WindowSpecSchema (const)", + "canonicalAstOperator (function)", "canonicalizeSqlType (function)", "classifyFilterToken (function)", "countAuthorableFields (function)", diff --git a/packages/spec/src/data/filter-view-operator-parity.test.ts b/packages/spec/src/data/filter-view-operator-parity.test.ts new file mode 100644 index 0000000000..79d96df64b --- /dev/null +++ b/packages/spec/src/data/filter-view-operator-parity.test.ts @@ -0,0 +1,143 @@ +// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * The view vocabulary and the AST vocabulary must agree. (#3948) + * + * `VIEW_FILTER_OPERATORS` (`ui/view.zod.ts`) is what an author may declare on a + * `ViewFilterRule`, and what `ViewFilterRuleSchema` validates against. + * `VALID_AST_OPERATORS` (`data/filter.zod.ts`) gates `isFilterAST()`, which + * decides whether a filter is parsed into a query at all. + * + * They disagreed on **8 of 19** members — `equals`, `not_equals`, + * `greater_than`, `less_than`, `greater_than_or_equal`, `less_than_or_equal`, + * `before`, `after`. An author could declare any of them, the schema accepted + * them, `defineStack` accepted them, and then `isFilterAST()` refused the filter, + * the protocol passed the array through unconverted, and the driver dropped it: + * an unfiltered result set with no error anywhere. + * + * Six of the eight were reachable only in theory, because ObjectUI's adapter + * alias table happened to translate them. The safety of the query path was + * resting on a hand-written table in a different repository being complete, and + * it wasn't — `before`/`after` had no entry, which is how this surfaced. + * + * `data/` cannot import `ui/` (that direction is already taken, so it would be + * circular), which is why the AST map is not literally derived from the view + * vocabulary. This test is the enforcement instead: it fails the moment the view + * layer gains an operator the AST layer cannot lower. + */ + +import { describe, it, expect } from 'vitest'; +import { + VALID_AST_OPERATORS, + isFilterAST, + parseFilterAST, +} from './filter.zod'; +import { + VIEW_FILTER_OPERATORS, + VIEW_FILTER_OPERATOR_ALIASES, +} from '../ui/view.zod'; + +/** + * View operators that resolve to a value-shape before an operator is ever + * emitted, so they legitimately need no AST lowering. + * + * Empty today: `is_empty`/`is_not_empty` DO have lowerings (to `$null`), because + * clients send them as operators rather than resolving them client-side. Kept as + * an explicit, empty exemption list so that adding to it is a visible decision + * rather than a quiet edit to the assertion. + */ +const NO_AST_LOWERING_REQUIRED = new Set([]); + +describe('every view filter operator has an AST lowering', () => { + it('reads both vocabularies', () => { + // Guards the assertions below from passing vacuously. + expect(VIEW_FILTER_OPERATORS.length).toBeGreaterThan(0); + expect(VALID_AST_OPERATORS.size).toBeGreaterThan(0); + }); + + it('VALID_AST_OPERATORS covers every canonical view operator', () => { + const missing = VIEW_FILTER_OPERATORS + .filter((op) => !NO_AST_LOWERING_REQUIRED.has(op)) + .filter((op) => !VALID_AST_OPERATORS.has(op.toLowerCase())); + expect( + missing, + 'an author can declare these on a ViewFilterRule and the schema validates them, ' + + 'but isFilterAST() refuses the filter — it is passed through unconverted and ' + + 'the driver cannot apply it. Add a lowering to AST_OPERATOR_MAP.', + ).toEqual([]); + }); + + it('VALID_AST_OPERATORS covers every legacy alias spelling too', () => { + // `saveMeta` persists the authored body verbatim, so the schema's own + // `z.preprocess` normalization never reaches the stored row — every alias in + // this table is live in metadata, not merely historical. + const missing = Object.keys(VIEW_FILTER_OPERATOR_ALIASES) + .filter((alias) => !NO_AST_LOWERING_REQUIRED.has(VIEW_FILTER_OPERATOR_ALIASES[alias])) + .filter((alias) => !VALID_AST_OPERATORS.has(alias.toLowerCase())); + expect( + missing, + 'these spellings exist in stored view metadata and have no AST lowering', + ).toEqual([]); + }); + + it.each([...VIEW_FILTER_OPERATORS])('%s survives isFilterAST as a bare triple', (op) => { + // The bare triple is the shape that used to vanish: a single AND condition + // is emitted as `[field, op, value]`, and when isFilterAST() rejects it the + // whole filter is silently dropped. + if (NO_AST_LOWERING_REQUIRED.has(op)) return; + const value = op === 'in' || op === 'not_in' ? ['a'] : op === 'between' ? [1, 2] : 'x'; + expect(isFilterAST(['some_field', op, value]), `isFilterAST rejects "${op}"`).toBe(true); + }); + + it('lowers each view operator to a real $-operator, never the $${op} fallback', () => { + // `convertComparison` ends in `{ [field]: { [`$${op}`]: value } }` for an + // unmapped operator, which produces e.g. `$before` — a key no driver knows, + // so the failure moves from "silently unfiltered" to "driver throws". Both + // are wrong; this asserts we produce a real operator. + const KNOWN = new Set([ + '$eq', '$ne', '$gt', '$gte', '$lt', '$lte', '$in', '$nin', + '$between', '$contains', '$notContains', '$startsWith', '$endsWith', + '$null', '$exists', + ]); + const bad: string[] = []; + for (const op of VIEW_FILTER_OPERATORS) { + if (NO_AST_LOWERING_REQUIRED.has(op)) continue; + const value = op === 'in' || op === 'not_in' ? ['a'] : op === 'between' ? [1, 2] : 'x'; + const parsed = parseFilterAST(['some_field', op, value]) as Record; + const arm = parsed.some_field; + // The equality shorthand is `{ field: value }` — a bare value, not an + // operator object. That is a real lowering, not a fallback. + if (arm === null || typeof arm !== 'object') continue; + const keys = Object.keys(arm as Record); + if (!keys.every((k) => KNOWN.has(k))) bad.push(`${op} → ${keys.join(',')}`); + } + expect(bad, 'these fell through to the $${op} fallback').toEqual([]); + }); + + it('the date comparisons that regressed now lower correctly', () => { + expect(parseFilterAST(['close_date', 'before', '2024-01-01'])) + .toEqual({ close_date: { $lt: '2024-01-01' } }); + expect(parseFilterAST(['close_date', 'after', '2024-01-01'])) + .toEqual({ close_date: { $gt: '2024-01-01' } }); + }); + + it('spells equality one way regardless of which alias the author used', () => { + const shorthand = { status: 'active' }; + for (const op of ['=', '==', 'equals', 'eq']) { + expect(parseFilterAST(['status', op, 'active']), `via "${op}"`).toEqual(shorthand); + } + }); + + it('keeps null-direction keyed on the operator name, not the filler value', () => { + // Clients send a truthy placeholder for both directions. + expect(parseFilterAST(['note', 'is_empty', true])).toEqual({ note: { $null: true } }); + expect(parseFilterAST(['note', 'isempty', true])).toEqual({ note: { $null: true } }); + expect(parseFilterAST(['note', 'is_not_empty', true])).toEqual({ note: { $null: false } }); + expect(parseFilterAST(['note', 'isnotempty', true])).toEqual({ note: { $null: false } }); + }); + + it('still refuses an operator in neither vocabulary', () => { + // Widening must not turn the gate off. + expect(isFilterAST(['some_field', 'sounds_like', 'x'])).toBe(false); + }); +}); diff --git a/packages/spec/src/data/filter.zod.ts b/packages/spec/src/data/filter.zod.ts index 7d3468e389..4e1b8a8457 100644 --- a/packages/spec/src/data/filter.zod.ts +++ b/packages/spec/src/data/filter.zod.ts @@ -345,24 +345,135 @@ export type NormalizedFilter = z.infer; // AST Array Format Detection & Validation // ============================================================================ +/** + * Operator mapping from AST infix operators to FilterCondition `$`-prefixed + * operators. **This is the single source of truth for the AST vocabulary** — + * {@link VALID_AST_OPERATORS} is derived from its keys, so an operator cannot be + * accepted by `isFilterAST()` without also having a lowering, which is how the + * two lists silently disagreed before (#3948). + * + * Keys are matched case-insensitively (`convertComparison` lowercases), so only + * lowercase spellings belong here — but underscores are NOT stripped, so a + * spelling that exists in both `snake_case` and squashed form needs both. + * + * Must lower every member of `VIEW_FILTER_OPERATORS` and every key of + * `VIEW_FILTER_OPERATOR_ALIASES` (`ui/view.zod.ts`) — those are the spellings an + * author can declare on a `ViewFilterRule` and that stored view metadata + * carries. `ui/` imports `data/`, so this file cannot import that vocabulary to + * derive from it; `filter-view-operator-parity.test.ts` asserts the coverage + * instead. Do not hand-add a view operator without running it. + */ +const AST_OPERATOR_MAP: Record = { + '=': '$eq', + '==': '$eq', + 'equals': '$eq', + 'eq': '$eq', + '!=': '$ne', + '<>': '$ne', + 'ne': '$ne', + 'neq': '$ne', + 'not_equals': '$ne', + 'notequals': '$ne', + '>': '$gt', + 'gt': '$gt', + 'greater_than': '$gt', + 'greaterthan': '$gt', + '>=': '$gte', + 'gte': '$gte', + 'greater_than_or_equal': '$gte', + 'greaterthanorequal': '$gte', + 'greaterorequal': '$gte', + '<': '$lt', + 'lt': '$lt', + 'less_than': '$lt', + 'lessthan': '$lt', + '<=': '$lte', + 'lte': '$lte', + 'less_than_or_equal': '$lte', + 'lessthanorequal': '$lte', + 'lessorequal': '$lte', + // Date comparisons. Canonical `VIEW_FILTER_OPERATORS` members with no infix + // spelling of their own; a stored view legitimately carries them, and before + // #3948 they had no lowering, so `isFilterAST()` refused the filter and it was + // dropped rather than applied. + 'before': '$lt', + 'after': '$gt', + 'in': '$in', + 'nin': '$nin', + 'not_in': '$nin', + 'notin': '$nin', + 'contains': '$contains', + 'notcontains': '$notContains', + 'not_contains': '$notContains', + 'like': '$contains', + 'startswith': '$startsWith', + 'starts_with': '$startsWith', + 'endswith': '$endsWith', + 'ends_with': '$endsWith', + 'between': '$between', + 'is_null': '$null', + 'is_not_null': '$null', + 'isnull': '$null', + 'isnotnull': '$null', + 'is_empty': '$null', + 'is_not_empty': '$null', + 'isempty': '$null', + 'isnotempty': '$null', +}; + /** * Set of valid AST comparison operators (case-insensitive). * Used by `isFilterAST()` to validate AST structure beyond `Array.isArray`. + * + * Derived from {@link AST_OPERATOR_MAP} rather than restated. The two were + * separate hand-written lists that happened to agree; nothing enforced it, and + * an operator in one but not the other is invisible — a name in the Set with no + * lowering hits `convertComparison`'s `$${op}` fallback and reaches the driver + * as an unknown `$`-operator, while a name in the Map but not the Set makes + * `isFilterAST()` refuse the filter entirely. #3948. */ -export const VALID_AST_OPERATORS = new Set([ - '=', '==', '!=', '<>', '>', '>=', '<', '<=', - 'in', 'nin', 'not_in', - 'contains', 'notcontains', 'not_contains', 'like', - 'startswith', 'starts_with', - 'endswith', 'ends_with', - 'between', - // Null / empty predicates. `is_null` / `is_not_null` are canonical; `isnull`, - // `isnotnull`, `is_empty`, `is_not_empty` are the alias spellings the ObjectUI - // `data-objectstack` adapter emits and the driver-sql/#2704 fix accepts — kept - // in sync here so `parseFilterAST()` never treats them as an unknown operator. - 'is_null', 'is_not_null', - 'isnull', 'isnotnull', 'is_empty', 'is_not_empty', -]); +export const VALID_AST_OPERATORS = new Set(Object.keys(AST_OPERATOR_MAP)); + +/** + * Canonical infix spelling for every accepted AST operator. + * + * `VALID_AST_OPERATORS` accepts many spellings of one comparison (`>`, `gt`, + * `greater_than`, `greaterthan`, `after`). A driver's array-format handler wants + * to `switch` on ONE of them, and each driver growing its own alias list is how + * the vocabularies drifted apart in the first place. Fold here instead. + * + * Returns the input lowercased and unchanged when it is not a known operator, so + * a caller's own `default:` still reports it. + */ +const CANONICAL_INFIX: Record = { + '$eq': '=', '$ne': '!=', '$gt': '>', '$gte': '>=', '$lt': '<', '$lte': '<=', + '$in': 'in', '$nin': 'nin', '$contains': 'contains', + '$notContains': 'not_contains', '$startsWith': 'starts_with', + '$endsWith': 'ends_with', '$between': 'between', +}; + +export function canonicalAstOperator(op: string): string { + const lower = String(op).toLowerCase(); + // Null predicates carry a DIRECTION that the shared `$null` lowering erases, + // so they cannot round-trip through CANONICAL_INFIX — fold them by name. + if (lower === 'is_null' || lower === 'isnull' || lower === 'is_empty' || lower === 'isempty') { + return 'is_null'; + } + if ( + lower === 'is_not_null' || lower === 'isnotnull' + || lower === 'is_not_empty' || lower === 'isnotempty' + ) { + return 'is_not_null'; + } + // `like`/`ilike` share the `$contains` lowering but are NOT substring matches + // at the driver: driver-sql passes them to SQL verbatim, so the caller binds + // the wildcards. Folding them onto `contains` would silently wrap the value in + // `%…%` and change what the query means. + if (lower === 'like' || lower === 'ilike') return lower; + const dollar = AST_OPERATOR_MAP[lower]; + if (!dollar) return lower; + return CANONICAL_INFIX[dollar] ?? lower; +} /** * Detect whether a value is a valid Filter AST array structure. @@ -413,38 +524,6 @@ export function isFilterAST(filter: unknown): boolean { // AST Array → FilterCondition Conversion // ============================================================================ -/** - * Operator mapping from AST infix operators to FilterCondition `$`-prefixed operators. - */ -const AST_OPERATOR_MAP: Record = { - '=': '$eq', - '==': '$eq', - '!=': '$ne', - '<>': '$ne', - '>': '$gt', - '>=': '$gte', - '<': '$lt', - '<=': '$lte', - 'in': '$in', - 'nin': '$nin', - 'not_in': '$nin', - 'contains': '$contains', - 'notcontains': '$notContains', - 'not_contains': '$notContains', - 'like': '$contains', - 'startswith': '$startsWith', - 'starts_with': '$startsWith', - 'endswith': '$endsWith', - 'ends_with': '$endsWith', - 'between': '$between', - 'is_null': '$null', - 'is_not_null': '$null', - 'isnull': '$null', - 'isnotnull': '$null', - 'is_empty': '$null', - 'is_not_empty': '$null', -}; - /** * Convert a single AST comparison node `[field, operator, value]` to a FilterCondition object. */ @@ -452,18 +531,23 @@ function convertComparison(node: [string, string, unknown]): FilterCondition { const [field, operator, value] = node; const op = operator.toLowerCase(); - // Special case: equality shorthand - if (op === '=' || op === '==') { + // Special case: equality shorthand. `equals`/`eq` are the view vocabulary's + // spellings of the same thing and must produce the same output, or one filter + // would compile two different ways depending on how the author spelled it. + if (op === '=' || op === '==' || op === 'equals' || op === 'eq') { return { [field]: value } as FilterCondition; } // Null / empty predicates — direction comes from the operator NAME, not the // (filler) value: the ObjectUI client sends a truthy placeholder value for // both `isnull` and `isnotnull`, so keying off `value` would collapse them. - if (op === 'is_null' || op === 'isnull' || op === 'is_empty') { + if (op === 'is_null' || op === 'isnull' || op === 'is_empty' || op === 'isempty') { return { [field]: { $null: true } } as FilterCondition; } - if (op === 'is_not_null' || op === 'isnotnull' || op === 'is_not_empty') { + if ( + op === 'is_not_null' || op === 'isnotnull' + || op === 'is_not_empty' || op === 'isnotempty' + ) { return { [field]: { $null: false } } as FilterCondition; }