diff --git a/.changeset/filter-no-silent-drop.md b/.changeset/filter-no-silent-drop.md new file mode 100644 index 0000000000..910055cef8 --- /dev/null +++ b/.changeset/filter-no-silent-drop.md @@ -0,0 +1,42 @@ +--- +"@objectstack/driver-memory": minor +"@objectstack/driver-sql": patch +--- + +fix(driver-sql,driver-memory): an uncompilable filter now throws instead of matching everything (#3948) + +A filter the driver could not compile was **skipped**, not rejected. No predicate +was emitted and the query returned every row — the caller asked to filter and +silently received the unfiltered set. + +The reachable shape is a bare comparison triple. `['close_date','before','2024-01-01']` +arrives at a driver only when `isFilterAST()` refused it — its operator is outside +`VALID_AST_OPERATORS`, so `parseFilterAST()` never converted it and the raw array +was assigned to `where`. `driver-sql`'s loop then saw three *strings*, matched +neither `and` nor `or`, and `continue`d past all three. `driver-memory` was worse: +it cast every string to a logic keyword, opening three empty groups and returning +`{}` — a filter matching every record. + +This is reachable from ordinary authoring, not just malformed input: `before` and +`after` are canonical `VIEW_FILTER_OPERATORS` members that `VALID_AST_OPERATORS` +does not accept. Eight of the nineteen canonical view operators are in that +position, including `equals`; the others were masked only because ObjectUI's +adapter alias table happened to cover them. + +**Behaviour change.** Both drivers now throw on a filter element that is neither a +logical keyword (`and`/`or`) nor a condition array, and `driver-memory` throws on +an operator it cannot express rather than dropping the condition. The nested and +`$`-object paths already threw on the same input, so this makes the three paths +agree. A caller that was relying on the old silence was receiving wrong results; +the error names the operator and the offending filter. + +**`driver-memory` also gains seven operators it silently ignored:** `not_in`, +`is_null`, `is_not_null`, `isnull`, `isnotnull`, `is_empty`, `is_not_empty` — all +members of `VALID_AST_OPERATORS`, all previously falling through to +`default: return null`. `is_null` narrowed nothing instead of matching null rows. +Alias sets and semantics mirror `driver-sql`'s `whereNull`/`whereNotNull` arms so +the two backends accept one vocabulary. + +Migration: none for well-formed filters. If a query now throws, the filter was +never being applied — fix the operator (the message names it), or lower it to an +AST spelling. `before` → `<`, `after` → `>`, `'not in'` → `nin`. diff --git a/packages/plugins/driver-memory/src/memory-driver.ts b/packages/plugins/driver-memory/src/memory-driver.ts index 91653451e7..969a6cc5ff 100644 --- a/packages/plugins/driver-memory/src/memory-driver.ts +++ b/packages/plugins/driver-memory/src/memory-driver.ts @@ -703,15 +703,40 @@ export class InMemoryDriver implements IDataDriver { for (const item of filters) { if (typeof item === 'string') { - const newLogic = item.toLowerCase() as 'and' | 'or'; - if (newLogic !== currentLogic) { - currentLogic = newLogic; + const lower = item.toLowerCase(); + // Previously this cast ANY string to 'and' | 'or'. A bare comparison + // triple — which reaches a driver only when `isFilterAST()` refused its + // operator, leaving the array unparsed — therefore opened three empty + // logic groups, produced no conditions, and returned `{}`: a filter that + // matches EVERY record. An unapplied filter must not look like a + // satisfied one. #3948. + if (lower !== 'and' && lower !== 'or') { + throw new Error( + `[driver-memory] Unrecognized filter operator "${item}" in a comparison triple. ` + + `A filter array is either a logical node (["and"|"or", …]) or nested ` + + `conditions ([[field, op, value], …]); a bare [field, op, value] only ` + + `reaches the driver when its operator is outside @objectstack/spec ` + + `VALID_AST_OPERATORS, which leaves the filter unparsed. ` + + `Filter was: ${JSON.stringify(filters)}`, + ); + } + if (lower !== currentLogic) { + currentLogic = lower; logicGroups.push({ logic: currentLogic, conditions: [] }); } } else if (Array.isArray(item)) { const [field, operator, value] = item; + // `convertConditionToMongo` now throws rather than returning null for an + // operator it cannot express, so a dropped condition can no longer + // silently widen the result set. const cond = this.convertConditionToMongo(field, operator, value); if (cond) logicGroups[logicGroups.length - 1].conditions.push(cond); + } else { + throw new Error( + `[driver-memory] Unrecognized filter element of type ` + + `"${item === null ? 'null' : typeof item}" — expected a logical keyword ` + + `("and"/"or") or a condition array. Filter was: ${JSON.stringify(filters)}`, + ); } } @@ -750,9 +775,9 @@ export class InMemoryDriver implements IDataDriver { return { [field]: { $lte: value } }; case 'in': return { [field]: { $in: value } }; - case 'nin': case 'not in': + case 'nin': case 'not_in': case 'notin': case 'not in': return { [field]: { $nin: value } }; - case 'contains': case 'like': + case 'contains': case 'like': case 'ilike': return { [field]: { $regex: new RegExp(this.escapeRegex(value), 'i') } }; case 'notcontains': case 'not_contains': return { [field]: { $not: { $regex: new RegExp(this.escapeRegex(value), 'i') } } }; @@ -760,13 +785,36 @@ export class InMemoryDriver implements IDataDriver { return { [field]: { $regex: new RegExp(`^${this.escapeRegex(value)}`, 'i') } }; case 'endswith': case 'ends_with': return { [field]: { $regex: new RegExp(`${this.escapeRegex(value)}$`, 'i') } }; + // Null / empty predicates. These are in `VALID_AST_OPERATORS` and were + // absent here, so every one of them fell to `default: return null` and was + // dropped — `is_null` narrowed nothing instead of matching null rows. + // Alias sets and semantics mirror driver-sql's `whereNull`/`whereNotNull` + // arms so both backends accept the same vocabulary. In a document store + // `{field: null}` matches null AND missing, and `$ne: null` excludes both, + // which is the right analogue of SQL IS [NOT] NULL. #3948. + case 'is_null': case 'isnull': case 'is_empty': case 'isempty': case 'empty': + return { [field]: null }; + case 'is_not_null': case 'isnotnull': + case 'is_not_empty': case 'isnotempty': case 'not_empty': case 'notempty': + case 'is_set': case 'set': + return { [field]: { $ne: null } }; case 'between': if (Array.isArray(value) && value.length === 2) { return { [field]: { $gte: value[0], $lte: value[1] } }; } - return null; + throw new Error( + `[driver-memory] "between" on field "${field}" needs a two-element array, got ` + + `${JSON.stringify(value)}. Returning no predicate would silently match every record.`, + ); default: - return null; + // Was `return null`, which the caller dropped — so an operator this + // driver cannot express narrowed nothing instead of erroring. driver-sql + // already threw on the same input; the two backends disagreed. #3948. + throw new Error( + `[driver-memory] Unsupported filter operator "${operator}" on field "${field}". ` + + `Supported operators: =, !=, <, <=, >, >=, in, nin, between, contains, ` + + `not_contains, starts_with, ends_with (see @objectstack/spec VALID_AST_OPERATORS).`, + ); } } diff --git a/packages/plugins/driver-memory/src/memory-filter-ast-vocabulary.test.ts b/packages/plugins/driver-memory/src/memory-filter-ast-vocabulary.test.ts new file mode 100644 index 0000000000..2c588ad624 --- /dev/null +++ b/packages/plugins/driver-memory/src/memory-filter-ast-vocabulary.test.ts @@ -0,0 +1,102 @@ +/** + * Filter-AST vocabulary parity, and no-silent-drop. (#3948) + * + * Two invariants, both of which this driver used to break in the same direction: + * + * 1. Every operator in `VALID_AST_OPERATORS` must be expressible. That set gates + * `isFilterAST()`, so anything in it is an operator the protocol will happily + * parse and hand to a driver. Seven of them — `not_in`, `is_null`, + * `is_not_null`, `isnull`, `isnotnull`, `is_empty`, `is_not_empty` — fell to + * `default: return null` and the caller dropped the condition, so e.g. + * `is_null` narrowed nothing instead of matching null rows. + * + * 2. An operator this driver cannot express must THROW, never be skipped. A + * dropped condition widens the result set: the caller asked to filter and + * silently received more rows than it asked for. driver-sql already threw on + * the same input, so the two backends disagreed about the same query. + * + * The nastiest case is the bare comparison triple. `['x','before',1]` reaches a + * driver only when `isFilterAST()` refused it, leaving the array unparsed — and + * the old loop cast each string element to a logic keyword, opening empty logic + * groups and returning `{}`: a filter matching EVERY record. + */ +import { describe, it, expect, beforeEach } from 'vitest'; +import { VALID_AST_OPERATORS } from '@objectstack/spec/data'; +import { InMemoryDriver } from './memory-driver.js'; + +const TABLE = 'vocab_probe'; + +describe('InMemoryDriver filter vocabulary ↔ VALID_AST_OPERATORS', () => { + let driver: InMemoryDriver; + + beforeEach(async () => { + driver = new InMemoryDriver({ persistence: false }); + await driver.connect(); + await driver.create(TABLE, { id: '1', name: 'alpha', score: 10, note: null }); + await driver.create(TABLE, { id: '2', name: 'beta', score: 20, note: 'set' }); + }); + + /** Operators are exercised through `find`, the path a real query takes. */ + const find = (where: unknown) => + driver.find(TABLE, { object: TABLE, fields: ['id'], where } as any); + + it('reads a non-empty operator set from the spec', () => { + // Guards every assertion below from passing vacuously. + expect(VALID_AST_OPERATORS.size).toBeGreaterThan(0); + }); + + /** A representative value per operator, so each one is actually exercised. */ + const valueFor = (op: string): unknown => { + if (op === 'in' || op === 'nin' || op === 'not_in') return ['alpha']; + if (op === 'between') return [0, 100]; + if (/null|empty/.test(op)) return true; + if (/contains|like|startswith|starts_with|endswith|ends_with/.test(op)) return 'alp'; + return 'alpha'; + }; + + it.each([...VALID_AST_OPERATORS])('expresses %s without dropping it', async (op) => { + const field = /^[<>=!]/.test(op) || op === 'between' ? 'score' : 'name'; + const value = field === 'score' && !Array.isArray(valueFor(op)) ? 10 : valueFor(op); + // The assertion is that this does not throw and does not silently degrade to + // "no predicate". An operator the driver cannot express now throws, so any + // rejection here means the spec accepts a name this driver cannot honour. + await expect( + find([[field, op, value]]), + `VALID_AST_OPERATORS accepts "${op}" but InMemoryDriver cannot express it`, + ).resolves.toBeDefined(); + }); + + it('matches null rows for is_null instead of dropping the predicate', async () => { + // The regression this pins: `is_null` used to return null from the converter, + // the condition was dropped, and the query returned BOTH rows. + const rows = await find([['note', 'is_null', true]]); + expect(rows.map((r: any) => r.id)).toEqual(['1']); + }); + + it('matches non-null rows for is_not_null', async () => { + const rows = await find([['note', 'is_not_null', true]]); + expect(rows.map((r: any) => r.id)).toEqual(['2']); + }); + + it('throws on an operator it cannot express, rather than matching everything', async () => { + await expect(find([['name', 'sounds_like', 'alpha']])) + .rejects.toThrow(/Unsupported filter operator "sounds_like"/); + }); + + it('throws on a bare comparison triple instead of returning every record', async () => { + // `before` is a canonical VIEW_FILTER_OPERATORS member that VALID_AST_OPERATORS + // does not accept, so this is the exact shape that reached drivers unparsed. + await expect(find(['created_at', 'before', '2024-01-01'])) + .rejects.toThrow(/Unrecognized filter operator "created_at"/); + }); + + it('throws on a malformed between rather than emitting no predicate', async () => { + await expect(find([['score', 'between', 5]])) + .rejects.toThrow(/needs a two-element array/); + }); + + it('still honours a well-formed logical node', async () => { + const rows = await find(['or', ['name', '=', 'alpha'], ['name', '=', 'beta']]); + expect(rows.map((r: any) => r.id).sort()).toEqual(['1', '2']); + }); +}); diff --git a/packages/plugins/driver-sql/src/sql-driver-filter-no-silent-drop.test.ts b/packages/plugins/driver-sql/src/sql-driver-filter-no-silent-drop.test.ts new file mode 100644 index 0000000000..aae994e8a8 --- /dev/null +++ b/packages/plugins/driver-sql/src/sql-driver-filter-no-silent-drop.test.ts @@ -0,0 +1,99 @@ +// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * A filter that cannot be compiled must THROW, never be skipped. (#3948) + * + * `applyFilters` walked the filter array and `continue`d past anything that was + * not a join keyword or a condition array. For a BARE comparison triple — + * `['close_date', 'before', '2024-01-01']` — the three elements are all strings, + * so every one was skipped and **no WHERE clause was emitted at all**: the caller + * asked to filter and silently received every row. + * + * A bare triple reaches a driver only when `isFilterAST()` refused it, i.e. its + * operator is outside `VALID_AST_OPERATORS`, so `parseFilterAST()` never + * converted it and the raw array arrived as `where`. That is reachable from + * ordinary authoring: `before`/`after` are canonical `VIEW_FILTER_OPERATORS` + * members which `VALID_AST_OPERATORS` does not accept. + * + * The nested and `$`-object paths already threw on the same class of input, so + * the three code paths disagreed about one query. These tests pin the loud + * behaviour, and pin that well-formed filters still compile. + */ + +import { describe, it, expect, beforeEach } from 'vitest'; +import { SqlDriver } from '../src/index.js'; + +describe('SqlDriver rejects an uncompilable filter instead of dropping it', () => { + let driver: SqlDriver; + + beforeEach(async () => { + driver = new SqlDriver({ + client: 'better-sqlite3', + connection: { filename: ':memory:' }, + useNullAsDefault: true, + }); + + await driver.initObjects([ + { + name: 'deal', + fields: { + id: { type: 'text', name: 'id' }, + stage: { type: 'text', name: 'stage' }, + amount: { type: 'number', name: 'amount' }, + }, + } as any, + ]); + + await driver.create('deal', { id: '1', stage: 'won', amount: 10 }); + await driver.create('deal', { id: '2', stage: 'lost', amount: 20 }); + }); + + const find = (where: unknown) => + driver.find('deal', { object: 'deal', fields: ['id'], where } as any); + + it('throws on a bare triple whose operator the AST gate refused', async () => { + // The exact shape a stored single-condition `before` view produced. + await expect(find(['close_date', 'before', '2024-01-01'])) + .rejects.toThrow(/Unrecognized filter operator "close_date"/); + }); + + it('does not silently return every row for that filter', async () => { + // The regression itself: before the fix this resolved with BOTH rows. + await expect(find(['stage', 'sounds_like', 'won'])).rejects.toThrow(); + }); + + it('throws on a filter element that is neither a keyword nor a condition', async () => { + await expect(find([42 as any])).rejects.toThrow(/Unrecognized filter element of type "number"/); + await expect(find([null as any])).rejects.toThrow(/Unrecognized filter element of type "null"/); + }); + + it('still throws on an unsupported operator inside a well-formed condition', async () => { + // Pre-existing behaviour, pinned so the two paths cannot diverge again. + await expect(find([['stage', 'sounds_like', 'won']])) + .rejects.toThrow(/Unsupported filter operator "sounds_like"/); + }); + + it('compiles a nested condition array', async () => { + const rows = await find([['stage', '=', 'won']]); + expect(rows.map((r: any) => r.id)).toEqual(['1']); + }); + + it('compiles an infix logical join', async () => { + // This path's legacy array form is INFIX — `[condA, 'or', condB]`. The + // prefix spec-AST form `['or', condA, condB]` reaches the driver already + // converted to `{$or: […]}` by `parseFilterAST()`, so it takes the + // object branch instead and never lands here. + const rows = await find([['stage', '=', 'won'], 'or', ['stage', '=', 'lost']]); + expect(rows.map((r: any) => r.id).sort()).toEqual(['1', '2']); + }); + + it('compiles an object-form filter', async () => { + const rows = await find({ stage: 'lost' }); + expect(rows.map((r: any) => r.id)).toEqual(['2']); + }); + + it('leaves an empty filter alone (no filter is not a failed filter)', async () => { + const rows = await find([]); + expect(rows).toHaveLength(2); + }); +}); diff --git a/packages/plugins/driver-sql/src/sql-driver.ts b/packages/plugins/driver-sql/src/sql-driver.ts index 5f97c28559..1cab07c674 100644 --- a/packages/plugins/driver-sql/src/sql-driver.ts +++ b/packages/plugins/driver-sql/src/sql-driver.ts @@ -4641,9 +4641,23 @@ export class SqlDriver implements IDataDriver { for (const item of filters) { if (typeof item === 'string') { - if (item.toLowerCase() === 'or') nextJoin = 'or'; - else if (item.toLowerCase() === 'and') nextJoin = 'and'; - continue; + const lower = item.toLowerCase(); + if (lower === 'or') { nextJoin = 'or'; continue; } + if (lower === 'and') { nextJoin = 'and'; continue; } + // Anything else is not a join keyword, and the only way a bare string + // reaches here is a comparison triple that `isFilterAST()` refused — + // its operator is outside `VALID_AST_OPERATORS`, so `parseFilterAST()` + // never converted it and the raw array arrived as `where`. Skipping it + // (the old behaviour) emitted NO predicate at all: the caller asked to + // filter and silently got every row. Fail loudly instead. #3948. + throw new Error( + `[sql-driver] Unrecognized filter operator "${item}" in a comparison triple. ` + + `A filter array is either a logical node (["and"|"or", …]) or nested ` + + `conditions ([[field, op, value], …]); a bare [field, op, value] only ` + + `reaches the driver when its operator is outside @objectstack/spec ` + + `VALID_AST_OPERATORS, which leaves the filter unparsed. ` + + `Filter was: ${JSON.stringify(filters)}`, + ); } if (Array.isArray(item)) { @@ -4666,7 +4680,18 @@ export class SqlDriver implements IDataDriver { } nextJoin = 'and'; + continue; } + + // Neither a join keyword nor a condition. Previously fell out of both + // branches and was dropped, so a malformed element silently narrowed + // nothing. Same reasoning as above: an unapplied filter must not look + // like a satisfied one. #3948. + throw new Error( + `[sql-driver] Unrecognized filter element of type "${item === null ? 'null' : typeof item}" — ` + + `expected a logical keyword ("and"/"or") or a condition array. ` + + `Filter was: ${JSON.stringify(filters)}`, + ); } }