diff --git a/.changeset/turso-remote-node-operator-refusal.md b/.changeset/turso-remote-node-operator-refusal.md new file mode 100644 index 0000000000..da706e55a8 --- /dev/null +++ b/.changeset/turso-remote-node-operator-refusal.md @@ -0,0 +1,47 @@ +--- +"@objectstack/driver-turso": patch +--- + +fix(driver-turso): remote 模式拒收条件层的 `$`-算子键 —— 不再编译成静默空集/全表写 (#5769) + +`RemoteTransport.buildWhereSQL` 只认 `$and` / `$or` / `$not` 三个组合算子;条件 +层其余任何 `$` 开头的键都掉进**字段路径**,被双引号引成一个**列名**。在 +`origin/main`(`5c94f833c`)上用捕获客户端 + 三行 fixture 实测: + +``` +{ $eq: 'won' } → SELECT * FROM "deal" WHERE "$eq" = ? → [] +{ $gt: 5 } → SELECT * FROM "deal" WHERE "$gt" = ? → [] +{ $where: 'return true' } → SELECT * FROM "deal" WHERE "$where" = ? → [] +{ $and: 'x' } → SELECT * FROM "deal" WHERE "$and" = ? → [] +{ $or: [{}, { $where: 'x' }] } → SELECT * FROM "deal"(整句没有 WHERE) → 全部三行 +``` + +前四行是**静默空结果集**:SQLite 的向后兼容规则把「解析不到列的双引号标识符」 +降级成字符串字面量,于是语句编得出、跑得通、一行不匹配 —— 和「确实没有匹配的 +行」在调用侧完全无法区分(在关掉该规则的构建上,`find()` 自己的 `no such column` +兜底也会把它吞成 `[]`,两条路一个答案)。 + +第五行不依赖任何方言怪癖,也是代价最大的一种:`{}` 是 `$or` 的 TRUE 单位元, +整组被吸收,连同它那个畸形兄弟已经编出来的子句一起被丢掉,语句**整个丢掉了 +WHERE**。读路径上这是把过滤器本要排除的行原样交还;`deleteMany` / `updateMany` +上这是**全表写** —— 实测三行全部被一个一行都没点名的过滤器改写。 + +现在:条件层任何非 `$and`/`$or`/`$not` 的 `$` 键,在 find / findOne / count / +aggregate / deleteMany / updateMany 六个建 WHERE 的入口上一律以 +`INVALID_FILTER` / 400 响亮拒收,且**不发出任何语句**。消息分两种 —— 是字段算子 +写高了一层(`$eq`/`$gt`/…)就指路 `{ <字段名>: { <算子>: <值> } }`;协议根本没 +声明的键(`$where`/`$nor`/`$expr`/`$elemMatch`)就点名拒收。声明正确但值不是数组 +的 `$and` / `$or`(`{ $and: 'x' }`)同样落在这个闸里,按「需要条件数组」拒收 —— +它此前从两个 `Array.isArray` 判断底下漏进同一条字段路径,结局一模一样。 + +这条规则本来就是 objectstack#5348 的裁定,PR #5368 已在 `SqlDriver` 的校验遍历 +(`reduceFilterKey`)落地,`driver-sqlite-wasm` 与 Turso **local** 继承。 +`RemoteTransport` 是独立的过滤器编译器,什么都继承不到,所以同一个 +`TursoDriver`、同一个过滤器,只因 `url` 不同就给两个答案,而且方向是反的:local +严、remote 松。本次补的正是这最后一面,新增的 local/remote 一致性用例把这条叉 +钉死。 + +合法过滤器一个字节都没变:三个组合算子的嵌套、`$and: []` / `$or: []` / `$not: {}` +的布尔单位元、字段层算子、隐式相等、`IS NULL`,以及既有的六种拒收(未知字段算子、 +不可绑定比较值、空算子映射、非节点子过滤器、非节点顶层 `where`、非布尔 `$null`) +各自的措辞,全部照旧。 diff --git a/packages/drivers/driver-turso/src/remote-transport-node-operator-refusal.test.ts b/packages/drivers/driver-turso/src/remote-transport-node-operator-refusal.test.ts new file mode 100644 index 0000000000..af0ee39b86 --- /dev/null +++ b/packages/drivers/driver-turso/src/remote-transport-node-operator-refusal.test.ts @@ -0,0 +1,568 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#5769] A `$`-key in a NODE position that is not a declared combinator is + * refused, on every path that builds a WHERE. + * + * `FilterConditionSchema` declares exactly three combinators (`$and`, `$or`, + * `$not`); every other key of a filter node is a FIELD NAME. `buildWhereSQL` + * recognised the three and let everything else fall into the field arms, where + * it was quoted into SQL as a COLUMN of that name. Measured on `origin/main` + * (`5c94f833c`) with a capturing client, three rows seeded: + * + * ``` + * { $eq: 'won' } → SELECT * FROM "deal" WHERE "$eq" = ? → [] + * { $gt: 5 } → SELECT * FROM "deal" WHERE "$gt" = ? → [] + * { $where: 'return true' } → SELECT * FROM "deal" WHERE "$where" = ? → [] + * { $and: 'x' } → SELECT * FROM "deal" WHERE "$and" = ? → [] + * { $or: [{}, { $where: 'x' }] } → SELECT * FROM "deal" → ALL THREE ROWS + * ``` + * + * ## The two endings, and why the second is the expensive one + * + * The first four are the **silent empty set**. SQLite's backwards-compatibility + * rule degrades a double-quoted name that resolves to no column into a string + * literal, so `"$eq" = 'won'` is `'$eq' = 'won'` — false for every row. The + * statement compiles, runs, and answers "nothing matched", which is exactly + * what a genuinely-empty result looks like. (A build with `SQLITE_DQS=0` — the + * one this suite's `better-sqlite3`-backed stub is — raises `no such column` + * instead, and `RemoteTransport.find`'s own unknown-column backstop swallows + * that into `[]`. Two roads, one answer.) + * + * The fifth needs no dialect quirk at all and is the one that costs rows. A + * `{}` disjunct is `$or`'s TRUE identity and absorbs the whole group; the + * clauses compiled from its malformed sibling are discarded with it, and the + * statement loses its WHERE **entirely**. On a read that hands back every row + * the filter was written to exclude; on `deleteMany` / `updateMany` it is a + * whole-table write — measured, all three rows rewritten by a filter that named + * none of them. That is the direction #1075 / #1073 spent two fixes closing, + * reopened one key at a time by an undeclared combinator. + * + * ## Why remote was the last face + * + * `objectstack#5348` ruled the shape REFUSED and PR #5368 landed the gate on + * `SqlDriver`'s validation walk (`reduceFilterKey`), which `driver-sqlite-wasm` + * and Turso's **local** transport inherit. `RemoteTransport` inherits nothing — + * it is an independent filter compiler — so one `TursoDriver` gave two answers + * to one filter depending only on the `url` it was constructed with, and the + * fork ran the wrong way: strict locally, silent remotely. The parity block at + * the bottom of this file is that fork, pinned closed. + * + * ## Where the gate sits, and why HERE rather than on a separate walk + * + * `SqlDriver` puts its gate on a validation walk because its emitter is skipped + * wholesale by a boolean identity, so an emitter-side gate there would be + * conditional on a node's SIBLINGS. This compiler has no such gap: its `$and` + * and `$or` branches compile EVERY element before applying their identity rule + * — the fifth measurement above is that fact, since `{ $where: 'x' }` was + * compiled and only then thrown away — so the entry loop already visits every + * node of the tree. The two cases under "(c) placement" are the ones that would + * fail if that were not so. + * + * @see sql-driver-out-of-contract-filter-input.test.ts — the twin, one driver up + */ + +import { describe, it, expect, vi, beforeAll, afterAll } from 'vitest'; +import { RemoteTransport } from './remote-transport.js'; +import { TursoDriver } from './turso-driver.js'; +import { makeLibsqlSqliteStub, type LibsqlSqliteStub } from './libsql-sqlite-stub.testkit.js'; +import type { QueryAST } from '@objectstack/spec/data'; + +interface WireBearingError extends Error { + code?: string; + status?: number; +} + +function transportWithCapturingClient() { + const calls: Array<{ sql: string; args: any[] }> = []; + const client = { + execute: vi.fn(async (stmt: any) => { + calls.push({ sql: stmt.sql ?? String(stmt), args: stmt.args ?? [] }); + return { rows: [], columns: [] }; + }), + close: vi.fn(), + }; + const t = new RemoteTransport(); + t.setClient(client as any); + return { t, calls }; +} + +/** The SQL a `find` compiled to, plus its bind list. */ +async function compile(where: unknown): Promise<{ sql: string; args: any[] }> { + const { t, calls } = transportWithCapturingClient(); + await t.find('deal', { where } as unknown as QueryAST); + return calls[0]; +} + +/** The error a `find` refused with — and proof that nothing reached the database. */ +async function refusalOf(where: unknown): Promise { + const { t, calls } = transportWithCapturingClient(); + try { + await t.find('deal', { where } as unknown as QueryAST); + } catch (e) { + expect(calls, 'a refused filter must not execute a statement').toEqual([]); + return e as WireBearingError; + } + throw new Error(`expected the transport to refuse ${JSON.stringify(where)}, but it compiled`); +} + +const BARE_SCAN = 'SELECT * FROM "deal"'; + +/** + * Undeclared `$`-keys, at the top level and at each depth a node can occur. + * + * `$where` / `$expr` are named rather than invented: `driver-mongodb`'s refusal + * calls them P0 because a backend that EVALUATES them bypasses query intent. + * `$nor` and `$elemMatch` are what a Mongo-fluent author reaches for. The last + * three prove the gate is reached through every combinator, including the two + * whose identity rules can resolve a node before its siblings are read. + */ +const UNDECLARED: Array<[label: string, where: unknown, key: string, path: string]> = [ + ['$where at the top level', { $where: 'return true' }, '$where', 'where.$where'], + ['$nor at the top level', { $nor: [{ stage: 'won' }] }, '$nor', 'where.$nor'], + ['$expr at the top level', { $expr: { $eq: ['$stage', 'won'] } }, '$expr', 'where.$expr'], + ['$elemMatch at the top level', { $elemMatch: { stage: 'won' } }, '$elemMatch', 'where.$elemMatch'], + ['$where inside $or', { $or: [{ $where: 'x' }] }, '$where', 'where.$or[0].$where'], + ['$nor inside $and', { $and: [{ $nor: [{ stage: 'won' }] }] }, '$nor', 'where.$and[0].$nor'], + ['$expr inside $not', { $not: { $expr: 1 } }, '$expr', 'where.$not.$expr'], + [ + '$where two combinators deep', + { $and: [{ $or: [{ stage: 'won' }, { $where: 'x' }] }] }, + '$where', + 'where.$and[0].$or[1].$where', + ], +]; + +/** + * Field operators in the node position — one level too high. + * + * This is the likelier arrival route of the two: `{ $eq: 'won' }` is what a + * hand-written or AI-authored filter produces when the field name is dropped, + * and every one of them compiled to a predicate on a column named after the + * operator. `$between` is included even though this transport never compiles it + * (TursoDriver lowers it first) — misplaced is misplaced — and `$regex` because + * better-auth's adapter really emits it. + */ +const MISPLACED: Array<[label: string, where: unknown, key: string]> = [ + ['$eq', { $eq: 'won' }, '$eq'], + ['$ne', { $ne: 'won' }, '$ne'], + ['$gt', { $gt: 5 }, '$gt'], + ['$gte', { $gte: 5 }, '$gte'], + ['$lt', { $lt: 5 }, '$lt'], + ['$lte', { $lte: 5 }, '$lte'], + ['$in', { $in: ['won'] }, '$in'], + ['$nin', { $nin: ['won'] }, '$nin'], + ['$between', { $between: [1, 2] }, '$between'], + ['$contains', { $contains: 'wo' }, '$contains'], + ['$notContains', { $notContains: 'wo' }, '$notContains'], + ['$startsWith', { $startsWith: 'w' }, '$startsWith'], + ['$endsWith', { $endsWith: 'n' }, '$endsWith'], + ['$regex', { $regex: 'wo' }, '$regex'], + ['$null', { $null: true }, '$null'], + ['$exists', { $exists: true }, '$exists'], +]; + +describe('[#5769] RemoteTransport refuses a $-key in a node position', () => { + describe('(a) undeclared combinators', () => { + for (const [label, where, key, path] of UNDECLARED) { + it(`refuses ${label} with INVALID_FILTER / 400`, async () => { + const err = await refusalOf(where); + expect(err.code).toBe('INVALID_FILTER'); + expect(err.status).toBe(400); + // The caller must be able to see WHICH key, and WHERE. + expect(err.message).toContain(`"${key}"`); + expect(err.message).toContain(path); + expect(err.message).toContain('$and, $or and $not'); + // Named as a combinator, never as a field of the object. + expect(err.message).not.toContain(`'deal.${key}'`); + }); + } + + it('says the protocol has no such key at any level, and names the write cost', async () => { + const err = await refusalOf({ $where: 'return true' }); + expect(err.message).toMatch(/declares no "\$where" at any level/); + expect(err.message).toMatch(/deleteMany\/updateMany/); + }); + }); + + describe('(b) field operators one level too high', () => { + for (const [label, where, key] of MISPLACED) { + it(`refuses a node-position ${label} and points at the field spelling`, async () => { + const err = await refusalOf(where); + expect(err.code).toBe('INVALID_FILTER'); + expect(err.status).toBe(400); + expect(err.message).toContain(`"${key}"`); + expect(err.message).toContain(`where.${key}`); + // The repair, not merely the diagnosis: an author who wrote `{ $eq: … }` + // needs to be told to name the field, not that `$eq` is "not a + // combinator" — which is true and useless. + expect(err.message).toContain('IS a field operator, one level down'); + expect(err.message).toContain(`{ : { "${key}": } }`); + }); + } + + it('the same key one level DOWN still compiles, byte for byte', async () => { + // The distinction the two messages rest on has to be real: the identical + // operator in the position it is declared for is untouched. + expect((await compile({ stage: { $eq: 'won' } })).sql).toBe(`${BARE_SCAN} WHERE "stage" = ?`); + expect((await compile({ amount: { $gt: 5 } })).sql).toBe(`${BARE_SCAN} WHERE "amount" > ?`); + expect((await compile({ stage: { $null: true } })).sql).toBe(`${BARE_SCAN} WHERE "stage" IS NULL`); + }); + }); + + describe('(c) placement — the gate is reached whatever the siblings say', () => { + /** + * `{ stage: 'won' }` is a satisfiable disjunct, so a gate that ran only on + * emitted clauses could compile the `$or` from that branch and never look + * at the malformed sibling. Pre-fix this compiled BOTH and answered `[]`. + */ + it('refuses a malformed disjunct beside a satisfiable one', async () => { + const err = await refusalOf({ $or: [{ stage: 'won' }, { $where: 'x' }] }); + expect(err.code).toBe('INVALID_FILTER'); + expect(err.message).toContain('where.$or[1].$where'); + }); + + /** + * The half that used to cost the whole table: `{}` is `$or`'s TRUE identity, + * so the group was absorbed and the compiled clauses of its malformed + * sibling were discarded with it — leaving `SELECT * FROM "deal"` with no + * WHERE at all. + */ + it('refuses a malformed disjunct beside the TRUE identity `{}`', async () => { + const err = await refusalOf({ $or: [{}, { $nor: [{ stage: 'won' }] }] }); + expect(err.code).toBe('INVALID_FILTER'); + expect(err.message).toContain('where.$or[1].$nor'); + }); + + /** `$and: []` / `$and: [{}]` resolve to TRUE the same way. */ + it('refuses a malformed conjunct beside a vacuous one', async () => { + const err = await refusalOf({ $and: [{}, { $expr: 1 }] }); + expect(err.code).toBe('INVALID_FILTER'); + expect(err.message).toContain('where.$and[1].$expr'); + }); + }); + + describe('(d) the declared combinators still have to carry a list', () => { + // Not a typo in the KEY but a wrong VALUE, and told apart from (a) on + // purpose: `$and` is spelled correctly. Pre-fix both fell through the + // `Array.isArray` tests into the very same field arm. + const NON_LIST: Array<[label: string, where: unknown, key: string, shown: string]> = [ + ['$and: a string', { $and: 'x' }, '$and', 'a string'], + ['$and: an object', { $and: { stage: 'won' } }, '$and', 'an object'], + ['$or: an object', { $or: { stage: 'won' } }, '$or', 'an object'], + ['$or: null', { $or: null }, '$or', 'null'], + ['$and: a number', { $and: 42 }, '$and', 'a number'], + ]; + + for (const [label, where, key, shown] of NON_LIST) { + it(`refuses ${label} by name, not as a column`, async () => { + const err = await refusalOf(where); + expect(err.code).toBe('INVALID_FILTER'); + expect(err.status).toBe(400); + expect(err.message).toContain(`"${key}" at where.${key}`); + expect(err.message).toContain('requires an array of filter conditions'); + expect(err.message).toContain(shown); + expect(err.message).toContain('FilterCondition[]'); + }); + } + + it('`$not` keeps its own single-operand refusal (#1076), not this one', async () => { + // `$not` takes ONE condition rather than a list, so it is shaped one level + // down in `buildSubFilterSQL`. Two spellings for one condition is what + // this file exists to prevent, so the boundary is pinned. + const err = await refusalOf({ $not: 'won' }); + expect(err.code).toBe('INVALID_FILTER'); + expect(err.message).toMatch(/\$not on 'deal' is a string/); + expect(err.message).not.toContain('requires an array of filter conditions'); + }); + }); + + describe('(e) every WHERE-building entry point refuses, and executes nothing', () => { + const PROBE = { $where: 'return true' }; + + it('find / findOne / count / aggregate', async () => { + const { t, calls } = transportWithCapturingClient(); + await expect(t.find('deal', { where: PROBE } as unknown as QueryAST)).rejects.toThrow( + /Unsupported filter combinator "\$where"/, + ); + await expect(t.findOne('deal', { where: PROBE } as unknown as QueryAST)).rejects.toThrow( + /Unsupported filter combinator "\$where"/, + ); + await expect(t.count('deal', { where: PROBE } as unknown as QueryAST)).rejects.toThrow( + /Unsupported filter combinator "\$where"/, + ); + await expect( + t.aggregate('deal', { where: PROBE, aggregations: [{ function: 'count' }] } as unknown as QueryAST), + ).rejects.toThrow(/Unsupported filter combinator "\$where"/); + expect(calls).toEqual([]); + }); + + it('deleteMany / updateMany — the paths where the old answer WROTE', async () => { + const { t, calls } = transportWithCapturingClient(); + await expect(t.deleteMany('deal', { where: PROBE } as any)).rejects.toThrow( + /Unsupported filter combinator "\$where"/, + ); + await expect(t.updateMany('deal', { where: PROBE } as any, { stage: 'lost' })).rejects.toThrow( + /Unsupported filter combinator "\$where"/, + ); + expect(calls).toEqual([]); + }); + + it('and on the identity-absorbed shape, whose old compile had NO where clause', async () => { + const { t, calls } = transportWithCapturingClient(); + const where = { $or: [{}, { $where: 'x' }] }; + await expect(t.deleteMany('deal', { where } as any)).rejects.toThrow(/INVALID_FILTER|combinator/); + await expect(t.updateMany('deal', { where } as any, { stage: 'lost' })).rejects.toThrow( + /INVALID_FILTER|combinator/, + ); + expect(calls).toEqual([]); + }); + }); + + describe('(f) nothing that compiled before changes', () => { + it('keeps the three declared combinators byte-identical', async () => { + expect((await compile({ stage: 'won' })).sql).toBe(`${BARE_SCAN} WHERE "stage" = ?`); + expect((await compile({ $or: [{ a: 1 }, { b: 2 }] })).sql).toBe( + `${BARE_SCAN} WHERE (("a" = ?) OR ("b" = ?))`, + ); + expect((await compile({ $and: [{ a: 1 }, { b: 2 }] })).sql).toBe( + `${BARE_SCAN} WHERE (("a" = ?) AND ("b" = ?))`, + ); + expect((await compile({ $not: { stage: 'won' } })).sql).toBe( + `${BARE_SCAN} WHERE NOT ("stage" = ?)`, + ); + expect( + (await compile({ $and: [{ $or: [{ stage: 'won' }] }, { $not: { stage: 'lost' } }] })).sql, + ).toBe(`${BARE_SCAN} WHERE (((("stage" = ?))) AND (NOT ("stage" = ?)))`); + }); + + it('keeps the boolean identities of #1073 / #1076 exactly where they were', async () => { + expect((await compile({})).sql).toBe(BARE_SCAN); + expect((await compile({ $and: [] })).sql).toBe(BARE_SCAN); + expect((await compile({ $and: [{}] })).sql).toBe(BARE_SCAN); + expect((await compile({ $or: [] })).sql).toBe(`${BARE_SCAN} WHERE 1 = 0`); + expect((await compile({ $or: [{}, { a: 1 }] })).sql).toBe(BARE_SCAN); + expect((await compile({ $not: {} })).sql).toBe(`${BARE_SCAN} WHERE 1 = 0`); + expect((await compile(undefined)).sql).toBe(BARE_SCAN); + expect((await compile(null)).sql).toBe(BARE_SCAN); + }); + + it('leaves an ordinary field named without a $ alone', async () => { + // The gate keys on the `$` prefix only, so every real column is untouched + // — including one whose name merely CONTAINS a dollar-free operator word. + expect((await compile({ eq: 'won', not_stage: 'lost' })).sql).toBe( + `${BARE_SCAN} WHERE "eq" = ? AND "not_stage" = ?`, + ); + }); + }); + + describe('(g) the neighbouring refusals keep their own wording', () => { + // Each of these was already refused, by a gate that names a different + // condition. The new gate must not swallow them into one message — that is + // the #1051 diagnostic detour re-created, just from the other side. + it('an unknown FIELD operator is still #1004`s message', async () => { + const err = await refusalOf({ stage: { $sounds_like: 'won' } }); + expect(err.message).toMatch(/Unsupported filter operator "\$sounds_like" on 'deal\.stage'/); + expect(err.message).not.toMatch(/combinator/); + }); + + it('an unbindable comparand is still #1058`s message', async () => { + const err = await refusalOf({ amount: { $gt: { $field: 'budget' } } }); + expect(err.message).toMatch(/Cross-field comparison is not supported/); + }); + + it('a non-node sub-filter is still #1073 / #1076`s message', async () => { + expect((await refusalOf({ $or: [null] })).message).toMatch(/\$or\[0\] on 'deal' is null/); + expect((await refusalOf({ $not: 'won' })).message).toMatch(/\$not on 'deal' is a string/); + }); + + it('a non-node top-level where is still #1075`s message', async () => { + expect((await refusalOf([['stage', '=', 'won']])).message).toMatch(/not a filter condition/); + }); + + it('an empty operator map is still #1071`s message', async () => { + expect((await refusalOf({ stage: {} })).message).toMatch(/compiles to NO predicate/); + }); + + it('a non-boolean $null comparand is still #1116`s message', async () => { + expect((await refusalOf({ stage: { $null: 'yes' } })).message).toMatch( + /requires a boolean comparand/, + ); + }); + }); +}); + +/** + * Rows, not SQL strings. + * + * A predicate compiled against a column that cannot exist leaves the statement + * valid — it just answers a different question — and the shape that loses its + * WHERE entirely leaves a statement that is valid and MUCH wider. Neither is + * visible to a string assertion. These run against a real SQLite database + * wearing the libsql interface: the reads must not answer, and the refused + * mutations must leave every row exactly as it was. + */ +describe('[#5769] a refused node-position $-key touches no rows', () => { + let driver: TursoDriver; + let stub: LibsqlSqliteStub; + + beforeAll(async () => { + stub = makeLibsqlSqliteStub(); + driver = new TursoDriver({ url: 'libsql://node-operator.turso.io', client: stub as never }); + await driver.connect(); + expect(driver.transportMode).toBe('remote'); + await driver.syncSchema('deal', { + name: 'deal', + fields: { stage: { type: 'string' }, amount: { type: 'number' } }, + }); + await driver.create('deal', { id: 'd_won', stage: 'won', amount: 10 }); + await driver.create('deal', { id: 'd_lost', stage: 'lost', amount: 20 }); + await driver.create('deal', { id: 'd_open', stage: 'open', amount: 30 }); + }); + + afterAll(async () => { + await driver.disconnect(); + stub.close(); + }); + + const allRows = () => + stub.raw.prepare('SELECT id, stage FROM "deal" ORDER BY id').all() as Array<{ + id: string; + stage: string; + }>; + + const ids = async (where: unknown) => + ((await driver.find('deal', { where } as unknown as QueryAST)) as any[]).map((r) => r.id).sort(); + + it('a read that used to come back empty now says why', async () => { + // Pre-fix: `RESOLVED []` — a filter that never compiled, reported as + // "no rows matched". + for (const where of [{ $eq: 'won' }, { $gt: 5 }, { $where: 'return true' }, { $and: 'x' }]) { + const err = (await driver + .find('deal', { where } as unknown as QueryAST) + .catch((e) => e)) as WireBearingError; + expect(err, JSON.stringify(where)).toBeInstanceOf(Error); + expect(err.code, JSON.stringify(where)).toBe('INVALID_FILTER'); + expect(err.status, JSON.stringify(where)).toBe(400); + } + }); + + it('a refused `deleteMany` deletes NOTHING', async () => { + await expect(driver.deleteMany('deal', { where: { $eq: 'won' } } as any)).rejects.toThrow( + /combinator/, + ); + expect(allRows().map((r) => r.id)).toEqual(['d_lost', 'd_open', 'd_won']); + }); + + it('a refused `updateMany` writes NOTHING', async () => { + await expect( + driver.updateMany('deal', { where: { $where: 'x' } } as any, { stage: 'archived' }), + ).rejects.toThrow(/combinator/); + expect(allRows().map((r) => r.stage).sort()).toEqual(['lost', 'open', 'won']); + }); + + /** + * The measurement that made this a P0 rather than a diagnostics complaint. + * Pre-fix on this exact fixture: `UPDATE "deal" SET "stage" = ?` with no + * WHERE, and all three rows came back `ARCHIVED`. + */ + it('the identity-absorbed `$or` no longer rewrites the whole table', async () => { + await expect( + driver.updateMany('deal', { where: { $or: [{}, { $where: 'x' }] } } as any, { stage: 'ARCHIVED' }), + ).rejects.toThrow(/combinator/); + expect(allRows().map((r) => r.stage).sort()).toEqual(['lost', 'open', 'won']); + await expect( + driver.deleteMany('deal', { where: { $or: [{}, { $where: 'x' }] } } as any), + ).rejects.toThrow(/combinator/); + expect(allRows()).toHaveLength(3); + }); + + it('well-formed filters still answer exactly as before', async () => { + expect(await ids({ stage: 'won' })).toEqual(['d_won']); + expect(await ids({ stage: { $eq: 'won' } })).toEqual(['d_won']); + expect(await ids({ $or: [{ stage: 'won' }, { stage: 'lost' }] })).toEqual(['d_lost', 'd_won']); + expect(await ids({ $and: [{ stage: 'won' }, { amount: { $gte: 10 } }] })).toEqual(['d_won']); + expect(await ids({ $not: { stage: 'won' } })).toEqual(['d_lost', 'd_open']); + expect(await ids({})).toEqual(['d_lost', 'd_open', 'd_won']); + expect(await ids({ $and: [] })).toEqual(['d_lost', 'd_open', 'd_won']); + expect(await ids({ $or: [] })).toEqual([]); + expect(await driver.count('deal', { object: 'deal', where: { stage: 'won' } })).toBe(1); + }); +}); + +/** + * Local and remote answer this filter the SAME way. + * + * The whole of #5769 is that they did not. `TursoDriver` picks its transport + * from `url`: a local/replica url inherits `SqlDriver`'s compiler and its + * #5348 gate, a `libsql://` url gets `RemoteTransport`, which had none. So one + * driver, one filter, two answers — and the strict one was the local one, which + * is the direction that lets a bug reach production untested. + * + * The wording legitimately differs (this transport keeps its `[RemoteTransport]` + * prefix, noted as deliberate in `invalidFilterError`); what must not differ is + * the VERDICT and the envelope a caller branches on. + */ +describe('[#5769] local and remote give the same verdict', () => { + let local: TursoDriver; + let remote: TursoDriver; + let stub: LibsqlSqliteStub; + + const OBJECT = { + name: 'deal', + fields: { stage: { type: 'string' }, amount: { type: 'number' } }, + }; + + beforeAll(async () => { + local = new TursoDriver({ url: ':memory:' }); + expect(local.transportMode).toBe('local'); + await local.initObjects([OBJECT as any]); + await local.create('deal', { id: 'd_won', stage: 'won', amount: 10 }, { bypassTenantAudit: true }); + + stub = makeLibsqlSqliteStub(); + remote = new TursoDriver({ url: 'libsql://parity.turso.io', client: stub as never }); + await remote.connect(); + expect(remote.transportMode).toBe('remote'); + await remote.syncSchema('deal', OBJECT); + await remote.create('deal', { id: 'd_won', stage: 'won', amount: 10 }); + }); + + afterAll(async () => { + await local.disconnect(); + await remote.disconnect(); + stub.close(); + }); + + const PARITY: Array<[label: string, where: unknown]> = [ + ['$where at the top level', { $where: 'return true' }], + ['$nor at the top level', { $nor: [{ stage: 'won' }] }], + ['$expr inside $not', { $not: { $expr: 1 } }], + ['$where beside the TRUE identity', { $or: [{}, { $where: 'x' }] }], + ['a misplaced $eq', { $eq: 'won' }], + ['a non-list $and', { $and: 'x' }], + ]; + + for (const [label, where] of PARITY) { + it(`${label}: both transports refuse with INVALID_FILTER / 400`, async () => { + const l = (await local + .find('deal', { object: 'deal', where } as unknown as QueryAST) + .catch((e) => e)) as WireBearingError; + const r = (await remote + .find('deal', { object: 'deal', where } as unknown as QueryAST) + .catch((e) => e)) as WireBearingError; + expect(l, `local resolved ${label}`).toBeInstanceOf(Error); + expect(r, `remote resolved ${label}`).toBeInstanceOf(Error); + expect(l.code).toBe('INVALID_FILTER'); + expect(r.code).toBe('INVALID_FILTER'); + expect(l.status).toBe(400); + expect(r.status).toBe(400); + }); + } + + it('and both still answer a well-formed filter identically', async () => { + const l = (await local.find('deal', { object: 'deal', where: { stage: 'won' } })) as any[]; + const r = (await remote.find('deal', { object: 'deal', where: { stage: 'won' } })) as any[]; + expect(l.map((x) => x.id)).toEqual(['d_won']); + expect(r.map((x) => x.id)).toEqual(['d_won']); + }); +}); diff --git a/packages/drivers/driver-turso/src/remote-transport-not-operator.test.ts b/packages/drivers/driver-turso/src/remote-transport-not-operator.test.ts index 86ef90db99..5c361874ff 100644 --- a/packages/drivers/driver-turso/src/remote-transport-not-operator.test.ts +++ b/packages/drivers/driver-turso/src/remote-transport-not-operator.test.ts @@ -127,17 +127,35 @@ describe('RemoteTransport $not (#1076)', () => { expect(call.args).toEqual(['won', 10]); }); - it('compiles the issue\'s `$not: { $eq: "won" }` as a negation of what the caller wrote', async () => { - // Pre-fix: `WHERE "$not" = ?`. `$not` is now the operator it is declared - // to be; the residual `"$eq"` column is the CALLER's malformed inner - // condition (a field operator written one level too high) and is compiled - // byte-identically by the local `SqlDriver`, which also hands a - // condition-level `$eq` to Knex as a column name. Tightening that is - // #1077, deliberately not this fix — doing it here would diverge remote - // from local at a second key while closing the first. - const call = await compile({ $not: { $eq: 'won' } }); - expect(call.sql).toBe(`${BARE_SCAN} WHERE NOT ("$eq" = ?)`); - expect(call.args).toEqual(['won']); + it('refuses the issue\'s `$not: { $eq: "won" }` at the INNER key, not as a field named $not', async () => { + // Two fixes, read in order. + // + // #1076 (this file): `$not` stopped being a column. Pre-#1076 this whole + // filter compiled to `WHERE "$not" = ?`. + // + // #5769 (the successor this case was written to anticipate): the residual + // `"$eq"` column — the CALLER's malformed inner condition, a field + // operator written one level too high — is refused rather than compiled. + // This assertion USED to pin `WHERE NOT ("$eq" = ?)`, on the reasoning + // that the local `SqlDriver` handed a condition-level `$eq` to Knex as a + // column name too, so tightening it here alone would fork remote from + // local. That reasoning expired: objectstack#5348 / PR #5368 put the gate + // on `SqlDriver`'s validation walk, local went strict, and remote became + // the last face — which is what objectstack#5769 closed. + // + // What the case still proves is what it always proved: the key named in + // the refusal is the INNER `$eq`, at `where.$not.$eq`. `$not` is the + // operator it is declared to be, all the way down. + const err = (await compile({ $not: { $eq: 'won' } }).catch((e) => e)) as Error & { + code?: string; + status?: number; + }; + expect(err).toBeInstanceOf(Error); + expect(err.code).toBe('INVALID_FILTER'); + expect(err.status).toBe(400); + expect(err.message).toContain('"$eq"'); + expect(err.message).toContain('where.$not.$eq'); + expect(err.message).not.toContain(`'deal.$not'`); }); }); diff --git a/packages/drivers/driver-turso/src/remote-transport.ts b/packages/drivers/driver-turso/src/remote-transport.ts index 28f1f7f9ae..dbc04f0589 100644 --- a/packages/drivers/driver-turso/src/remote-transport.ts +++ b/packages/drivers/driver-turso/src/remote-transport.ts @@ -14,6 +14,7 @@ import type { Client, InStatement, ResultSet } from '@libsql/client'; import { StandardErrorCode } from '@objectstack/spec/api'; +import { FILTER_OPERATORS, LOGICAL_OPERATORS } from '@objectstack/spec/data'; import { nanoid } from 'nanoid'; /** @@ -60,6 +61,39 @@ const SUPPORTED_FILTER_OPERATORS = [ '$exists', ] as const; +/** + * The `$`-keys a filter NODE may carry (#5769). + * + * `FilterConditionSchema` declares exactly three combinators; every OTHER key + * of a node is a FIELD NAME. Taken from `@objectstack/spec`'s own + * `LOGICAL_OPERATORS` rather than restated here — a private copy of a shared + * vocabulary agrees with the spec on the day it is typed and never again, which + * is the note `driver-memory`'s `SUPPORTED_FIELD_OPERATORS` already carries. + */ +const NODE_COMBINATORS: ReadonlySet = new Set(LOGICAL_OPERATORS); + +/** + * The `$`-keys that are FIELD operators, i.e. legal one level DOWN (#5769). + * + * Used only to pick which sentence a refusal prints — never whether to refuse. + * That distinction matters because `FILTER_OPERATORS` is a runtime allowlist + * other packages derive enforcement from (#5701): a name added there must not + * be able to change this transport's VERDICT on a node-position key, and here + * it cannot — both branches of {@link RemoteTransport.undeclaredCombinator} + * throw `INVALID_FILTER`, they differ only in the repair they suggest. + * + * `$regex` is added for the same reason {@link SUPPORTED_FILTER_OPERATORS} + * carries it: it is not spec-declared but is what better-auth's adapter emits, + * so a caller really can misplace it. `$between` comes in with the spec list + * even though this transport never compiles it (TursoDriver lowers it first) — + * a misplaced `$between` is still a misplaced FIELD operator, and telling its + * author "unknown combinator" would send them looking for the wrong mistake. + */ +const MISPLACED_FIELD_OPERATORS: ReadonlySet = new Set([ + ...FILTER_OPERATORS, + '$regex', +]); + /** * Where the wildcard goes in a LIKE pattern: `contains` → `%v%`, * `starts` → `v%`, `ends` → `%v`. @@ -1033,8 +1067,24 @@ export class RemoteTransport { * test before the loop runs. The only remaining sources of `''` are * genuinely-empty inputs (`{}`, `$and: []`, an `$or` with a TRUE disjunct) — * all of which ARE TRUE. + * + * Since #5769 that list is also *reachable-from* correct: an `$or` whose TRUE + * disjunct absorbed the group used to be the one way a MALFORMED sibling + * could still produce `''`, because its compiled clauses were discarded along + * with the branch. The node gate at the top of the loop below refuses the + * sibling on the way in, so `''` now means TRUE **and** every key of every + * node was one this transport compiles. + * + * `path` names this node's position for a refusal message — `where` at the + * top, `where.$or[0]` inside a disjunct. It is read by the #5769 gate ONLY; + * every refusal that predates it keeps its own wording and its own way of + * naming a location, so threading this parameter changes no existing message. */ - private buildWhereSQL(object: string, filters: any): { whereClauses: string; args: any[] } { + private buildWhereSQL( + object: string, + filters: any, + path = 'where', + ): { whereClauses: string; args: any[] } { // "No filter" is spelled by ABSENCE, and that is the only spelling. All // five call sites hand this method `query?.where` (`query.where` in // `buildSelectSQL`), so a caller that supplied no filter arrives as @@ -1084,11 +1134,67 @@ export class RemoteTransport { const args: any[] = []; for (const [key, value] of Object.entries(filters)) { + // [#5769] Declared = enforced, in the NODE position. + // + // Everything `$`-prefixed here that is not one of the three declared + // combinators fell through to the FIELD arms below and was compiled as a + // COLUMN of that name — the #1051/#1076 diagnostic detour, one level up + // from where `$not` used to land. Measured on `origin/main` against three + // rows, with a capturing client: + // + // { $eq: 'won' } → SELECT … WHERE "$eq" = ? → [] + // { $where: 'return true' } → SELECT … WHERE "$where" = ? → [] + // { $or: [{}, { $where: 'x' }] } → SELECT … (no WHERE at all) → every row + // + // The first two are the silent empty set: SQLite's backwards-compatible + // rule degrades a double-quoted name that resolves to no column into a + // STRING LITERAL, so the statement compiles, runs and matches nothing — + // and where a build disables that rule (`SQLITE_DQS=0`) `find()`'s own + // `no such column` backstop swallows the error into `[]` anyway. Two + // roads, one answer, and neither is distinguishable from "no rows + // matched". + // + // The third is the expensive direction, and it needs no dialect quirk at + // all: a `{}` disjunct absorbs its `$or` to TRUE, the compiled clauses are + // discarded with it, and the statement loses its WHERE entirely. On + // `deleteMany`/`updateMany` that is the whole table — measured, all three + // rows updated by a filter naming none of them. + // + // `SqlDriver` refuses the same shape on its validation walk + // (`reduceFilterKey`, objectstack#5348 / PR #5368), which Turso's LOCAL + // transport inherits. This transport was the one remaining face, and the + // fork ran the wrong way: strict locally, silent remotely, for one filter + // whose only difference was the `url` it was sent to. + // + // Placed inline in this loop rather than on a separate validation walk — + // the opposite of `SqlDriver`'s placement, for a reason particular to + // this compiler. `SqlDriver`'s emitter is skipped wholesale by a boolean + // identity, so an emitter-side gate there would be conditional on a + // node's SIBLINGS. This one is not: the `$and` and `$or` branches below + // compile EVERY element before applying their identity rule (the third + // measurement above is that fact — `{ $where: 'x' }` was compiled, then + // its clause thrown away), so this loop already visits every node of the + // tree, `$or`'s TRUE-absorbing branch included. The pinning cases for + // exactly that are in `remote-transport-node-operator-refusal.test.ts`. + if (key.startsWith('$')) { + if (!NODE_COMBINATORS.has(key)) { + throw this.undeclaredCombinator(object, key, `${path}.${key}`); + } + // The three declared names still have to carry the shape they are + // declared with. `{ $and: 'x' }` fell through the `Array.isArray` tests + // below into the very same field arm — `WHERE "$and" = ?` — so leaving + // it out would put a hole in this gate at precisely the three keys the + // gate is defined by. `$not` takes a single operand rather than a list + // and is refused by shape one level down, in `buildSubFilterSQL`. + if ((key === '$and' || key === '$or') && !Array.isArray(value)) { + throw this.nonListCombinator(object, key, value, `${path}.${key}`); + } + } if (key === '$and' && Array.isArray(value)) { const subClauses: string[] = []; const subArgs: any[] = []; for (const [index, sub] of value.entries()) { - const { whereClauses: sc, args: sa } = this.buildSubFilterSQL(object, key, index, sub); + const { whereClauses: sc, args: sa } = this.buildSubFilterSQL(object, key, index, sub, path); // A TRUE conjunct is AND's identity element: `x AND TRUE ≡ x`, so // dropping it loses nothing. This is the ONE direction the old // "skip whatever compiled to nothing" rule happened to get right. @@ -1112,7 +1218,7 @@ export class RemoteTransport { // truth value — which is the asymmetry #1073 is about. let hasTrueDisjunct = false; for (const [index, sub] of value.entries()) { - const { whereClauses: sc, args: sa } = this.buildSubFilterSQL(object, key, index, sub); + const { whereClauses: sc, args: sa } = this.buildSubFilterSQL(object, key, index, sub, path); if (!sc) { hasTrueDisjunct = true; continue; @@ -1163,7 +1269,7 @@ export class RemoteTransport { // return it (JS `undefined !== 'won'`). The divergence is the SQL // family's, not this transport's, and remote mode is pinned to the // family it belongs to — filed as objectstack#5146. - const { whereClauses: sc, args: sa } = this.buildSubFilterSQL(object, key, null, value); + const { whereClauses: sc, args: sa } = this.buildSubFilterSQL(object, key, null, value, path); if (sc) { clauses.push(`NOT (${sc})`); args.push(...sa); @@ -1479,6 +1585,85 @@ export class RemoteTransport { ); } + /** + * The error for a `$`-key in a NODE position that is not a declared + * combinator (#5769). + * + * The leading sentence is `driver-sql`'s and `driver-memory`'s, verbatim — + * they are word-for-word identical to each other because #5240 ruled that one + * condition speaks one wording, and this is the same condition on a third + * backend. Only the tail differs, and it says what THIS transport used to do + * with the key, because what it used to do is not what the others used to do: + * they handed it to knex or to mingo, this one quoted it into SQL as a column + * name. + * + * ## Two tails, one verdict + * + * A node-position `$eq` and a node-position `$where` are not the same + * mistake. The first is a real operator ONE LEVEL too high — the author wrote + * `{ $eq: 'won' }` where `{ stage: { $eq: 'won' } }` was meant, which is the + * single most likely way to arrive here from a hand-written or AI-authored + * filter — and the repair is to name the field. The second names nothing this + * protocol declares at any level, and its repair is to stop using it. Telling + * a misplaced `$eq`'s author "$eq is not a combinator" is true and useless. + * + * Both are refused, with the same `INVALID_FILTER` / 400 and the same first + * sentence; only the closing direction differs. Nothing branches on the + * distinction — see {@link MISPLACED_FIELD_OPERATORS} for why it must stay + * that way. + */ + private undeclaredCombinator(object: string, key: string, path: string): Error { + const shared = + `[RemoteTransport] Unsupported filter combinator "${key}" at ${path} on '${object}'. A filter ` + + `node's $-prefixed keys are the declared logical operators $and, $or and $not ` + + `(@objectstack/spec LOGICAL_OPERATORS); every other key is a field name. It is refused rather ` + + `than compiled as a COLUMN of that name, which is what this transport used to do — SQLite ` + + `degrades a double-quoted name that matches no column into a string literal, so the statement ` + + `compiled, ran and matched nothing, and a caller could not tell "no rows matched" from "the ` + + `filter never compiled"`; + if (MISPLACED_FIELD_OPERATORS.has(key)) { + return invalidFilterError( + `${shared}. "${key}" IS a field operator, one level down: write ` + + `{ : { "${key}": } } — e.g. { stage: { "${key}": … } } — rather than putting ` + + `it at the condition level (objectstack#5769, objectstack#5348).`, + ); + } + return invalidFilterError( + `${shared}. The Filter Protocol declares no "${key}" at any level; on a read it cost an empty ` + + `page, and on deleteMany/updateMany the predicate is constantly false or constantly true ` + + `depending on which side of the comparison the literal lands (objectstack#5769, ` + + `objectstack#5348).`, + ); + } + + /** + * The error for `$and` / `$or` carrying something other than a list (#5769). + * + * Split from {@link undeclaredCombinator} because the key is not the mistake: + * `$and` is declared, it is spelled correctly, and its VALUE is wrong. Told + * "unsupported combinator", its author would go looking for a typo that is + * not there. + * + * It belongs to #5769 all the same, because the OLD ending was identical: + * both branches below tested `Array.isArray` before claiming the key, so a + * non-list `$and` fell straight through them into the field arms and compiled + * to `WHERE "$and" = ?` — measured on `origin/main`, the same silent empty set + * as `{ $eq: 'won' }`. `SqlDriver` refuses it on its walk + * (`assertFilterNodeList`), so local and remote now agree here too. + * + * The requirement sentence is `driver-sql`'s, so the two read alike. + */ + private nonListCombinator(object: string, key: string, value: unknown, path: string): Error { + const shown = value === null ? 'null' : value === undefined ? 'undefined' : describeValue(value); + return invalidFilterError( + `[RemoteTransport] Filter combinator "${key}" at ${path} on '${object}' requires an array of ` + + `filter conditions, but received ${shown} (${preview(value)}). @objectstack/spec ` + + `FilterConditionSchema declares "${key}" as FilterCondition[]. Refusing rather than falling ` + + `through to the field path, which read '${key}' as a COLUMN NAME and compiled a predicate ` + + `against a column that cannot exist (objectstack#5769).`, + ); + } + /** * Compile ONE sub-filter of a logical operator — an element of `$and` / `$or` * (#1073), or the whole operand of `$not` (#1076). @@ -1497,15 +1682,21 @@ export class RemoteTransport { * * `index` is the element's position for the array operators, and `null` for * `$not`, which takes a single operand rather than a list. + * + * `path` is the ENCLOSING node's position; this method extends it with the + * branch it is descending into, so a #5769 refusal raised further down can + * name the disjunct it came from (`where.$or[1].$where`) rather than only the + * key. Nothing else reads it — see {@link buildWhereSQL}. */ private buildSubFilterSQL( object: string, branch: '$and' | '$or' | '$not', index: number | null, sub: unknown, + path = 'where', ): { whereClauses: string; args: any[] } { if (!isFilterNode(sub)) throw this.uncompilableSubFilter(object, branch, index, sub); - return this.buildWhereSQL(object, sub); + return this.buildWhereSQL(object, sub, index === null ? `${path}.${branch}` : `${path}.${branch}[${index}]`); } /**