diff --git a/.changeset/turso-remote-null-safe-negation.md b/.changeset/turso-remote-null-safe-negation.md new file mode 100644 index 0000000000..84e48e0f5d --- /dev/null +++ b/.changeset/turso-remote-null-safe-negation.md @@ -0,0 +1,46 @@ +--- +'@objectstack/driver-turso': patch +--- + +driver-turso: remote mode answers the NULL / no-value family the way local mode does + +`TursoDriver` compiles filters two different ways: local (and replica) mode +inherits `SqlDriver.applyFilterCondition`, remote mode uses +`RemoteTransport.buildWhereSQL`, an independent emitter. The NULL rulings landed +only on the first, so ONE driver gave one filter two answers depending on the +`url` it was constructed with. Measured against a fixture with two valued rows +and two no-value rows: + +| filter | local | remote (before) | +|---|---|---| +| `{ d: { $ne: 'v1' } }` | rows 2,3,4 | row 2 | +| `{ d: { $nin: ['v1'] } }` | rows 2,3,4 | row 2 | +| `{ d: { $notContains: 'v1' } }` | rows 2,3,4 | row 2 | +| `{ $not: { d: 'v1' } }` | rows 2,3,4 | row 2 | +| `{ d: { $exists: 'yes' } }` | `INVALID_FILTER` | rows 1,2 | + +Remote mode now matches local on all five: + +- **`$not` is NULL-safe** (#5146). Each leaf of the negated condition is made + total before the negation, so `NOT (…)` is TRUE or FALSE for every row instead + of vanishing into SQL's UNKNOWN. A row whose column has no value does not + satisfy the negated condition, so it IS returned. +- **`$ne`, `$nin` and `$notContains` are NULL-safe** (#5298), emitted as + `(col IS NULL OR )`. `$ne: null` is unchanged and still compiles to + `IS NOT NULL` — polarity follows the comparand, not the operator's name — and + no positive comparison changes shape. +- **A non-boolean `$exists` comparand is refused** with `INVALID_FILTER` / 400 + (#5369), as `$null` already was. `@objectstack/spec`'s `FieldOperatorsSchema` + declares `$exists` as a boolean, and the emitter's `=== false` test sent every + other value — including the truthy string `"false"` — to the `IS NOT NULL` + side. `$exists: true` / `$exists: false` are unchanged. + +Why it matters beyond a row count: a CEL `!expr` in a permission rule lowers to +`{ $not: {…} }`, so this was one RLS read scope admitting different row sets per +connection mode. The `$ne` and `$not` cases are now enrolled in the shared +`FILTER_LOGIC_CASES` conformance table, which all eleven filter backends run. + +**Upgrade note:** a query that relied on remote mode silently dropping no-value +rows from a negative filter will now see them. Spell that intent explicitly — +`{ $and: [{ d: { $ne: 'v1' } }, { d: { $null: false } }] }` — which is what it +already had to be on every other backend. 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 index af0ee39b86..779541fca7 100644 --- 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 @@ -328,12 +328,19 @@ describe('[#5769] RemoteTransport refuses a $-key in a node position', () => { expect((await compile({ $and: [{ a: 1 }, { b: 2 }] })).sql).toBe( `${BARE_SCAN} WHERE (("a" = ?) AND ("b" = ?))`, ); + // [#5903] The `$not` operand is NULL-safe since #5146 landed on this + // face, so the negated predicate now carries the `IS NOT NULL` guard that + // makes it TOTAL. That is a change THIS suite must not read as a #5769 + // regression: what #5769 pins is that the node gate compiles the three + // declared combinators rather than refusing them, and it still does. expect((await compile({ $not: { stage: 'won' } })).sql).toBe( - `${BARE_SCAN} WHERE NOT ("stage" = ?)`, + `${BARE_SCAN} WHERE NOT ((("stage" IS NOT NULL) AND ("stage" = ?)))`, ); expect( (await compile({ $and: [{ $or: [{ stage: 'won' }] }, { $not: { stage: 'lost' } }] })).sql, - ).toBe(`${BARE_SCAN} WHERE (((("stage" = ?))) AND (NOT ("stage" = ?)))`); + ).toBe( + `${BARE_SCAN} WHERE (((("stage" = ?))) AND (NOT ((("stage" IS NOT NULL) AND ("stage" = ?)))))`, + ); }); it('keeps the boolean identities of #1073 / #1076 exactly where they were', async () => { 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 5c361874ff..69a34f4d85 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 @@ -42,19 +42,35 @@ import type { QueryAST } from '@objectstack/spec/data'; * one RLS scope answered correctly on a local SqlDriver and broke on Turso * remote — a local/remote divergence on a declared spec construct. * - * Two semantics are deliberate and pinned below, because the three in-tree - * implementations do not agree on them (filed as objectstack#5146): + * Two semantics are deliberate and pinned below. One of them has since been + * RULED and reversed; both are recorded here because the reversal is the point. * - * - **NULL rows follow the SQL family.** `NOT ("stage" = ?)` is UNKNOWN when - * `stage` is NULL, so that row is not returned — exactly what Knex's - * `whereNot` emits for local mode (`select … where (not (\`stage\` = ?))`, - * measured). `driver-memory`/`matchesFilterCondition` return it. Remote mode - * is pinned to the family it belongs to, so local and remote SQL agree. - * - **`$not: {}` is FALSE.** The inner filter compiles to no SQL, which under - * the #1073 invariant can only mean "vacuously TRUE", and `NOT TRUE` is - * FALSE. `driver-memory` and `matchesFilterCondition` agree; driver-sql - * returns every row there, but only because Knex drops an empty group — the - * widening direction of the very bug family #2704 closed. + * - **NULL rows are RETURNED (#5146, landed on this face by #5903).** This + * bullet used to say the opposite — "NULL rows follow the SQL family: + * `NOT ("stage" = ?)` is UNKNOWN when `stage` is NULL, so that row is not + * returned, exactly what Knex's `whereNot` emits for local mode; + * `driver-memory`/`matchesFilterCondition` return it; remote mode is pinned to + * the family it belongs to, so local and remote SQL agree." Every clause of + * that was true when it was written and the CONCLUSION expired eleven months + * later: objectstack#5146 ruled the two-valued answer canonical and PR #5296 + * landed it on `SqlDriver.applyFilterCondition`, so local mode — which + * inherits it — started returning the NULL row while this independent + * compiler did not. The sentence that justified the pin ("local and remote SQL + * agree") became the reason to flip it. Measured on `origin/main` on + * 2026-08-06 against the shared conformance fixture: LOCAL `['2','3','4']`, + * REMOTE `['2']`. + * + * The predicate is made TOTAL leaf by leaf before the negation, so + * `NOT (…)` is TRUE or FALSE for every row and never UNKNOWN. That is why the + * compiled SQL below carries an `IS NOT NULL` conjunct that was not there + * before, and why the polarity is per operator rather than a blanket + * `OR col IS NULL`: `{ $not: { a: { $ne: 5 } } }` means "a is 5", which a + * no-value row must NOT satisfy (pinned in section (g) below). + * - **`$not: {}` is FALSE.** UNCHANGED. The inner filter compiles to no SQL, + * which under the #1073 invariant can only mean "vacuously TRUE", and + * `NOT TRUE` is FALSE. `driver-memory` and `matchesFilterCondition` agree; + * driver-sql returns every row there, but only because Knex drops an empty + * group — the widening direction of the very bug family #2704 closed. */ function transportWithCapturingClient() { const calls: Array<{ sql: string; args: any[] }> = []; @@ -98,7 +114,11 @@ describe('RemoteTransport $not (#1076)', () => { // Pre-fix: THREW `Filter on 'deal.$not' has an object comparand whose key // "stage" is not an operator`. const call = await compile({ $not: { stage: 'won' } }); - expect(call.sql).toBe(`${BARE_SCAN} WHERE NOT ("stage" = ?)`); + // [#5903] The `IS NOT NULL` conjunct is #5146's leaf-totalising guard: a + // row with no `stage` does not satisfy `stage = 'won'`, so it must satisfy + // the negation. Without it `NOT (NULL = ?)` is UNKNOWN and the row + // vanishes. + expect(call.sql).toBe(`${BARE_SCAN} WHERE NOT ((("stage" IS NOT NULL) AND ("stage" = ?)))`); expect(call.args).toEqual(['won']); expectBindsBalanced(call); }); @@ -123,7 +143,11 @@ describe('RemoteTransport $not (#1076)', () => { // `NOT (a AND b)`, never `NOT (a) AND b` — the inner object is one // condition and De Morgan is not the caller's intent. const call = await compile({ $not: { stage: 'won', amount: 10 } }); - expect(call.sql).toBe(`${BARE_SCAN} WHERE NOT ("stage" = ? AND "amount" = ?)`); + // Each leaf is guarded independently and the four conjuncts stay inside + // the ONE negated group — `NOT (a AND b)`, never `NOT (a) AND b`. + expect(call.sql).toBe( + `${BARE_SCAN} WHERE NOT ((("stage" IS NOT NULL) AND ("stage" = ?) AND ("amount" IS NOT NULL) AND ("amount" = ?)))`, + ); expect(call.args).toEqual(['won', 10]); }); @@ -202,27 +226,44 @@ describe('RemoteTransport $not (#1076)', () => { it('compiles `$not: { $not: {…} }` as two nested negations', async () => { // Pre-fix: THREW `Unsupported filter operator "$not" on 'deal.$not'`. const call = await compile({ $not: { $not: { stage: 'won' } } }); - expect(call.sql).toBe(`${BARE_SCAN} WHERE NOT (NOT ("stage" = ?))`); + // The INNER `$not` is left un-rewritten by the outer one on purpose: its + // own branch totalises its operand, and `NOT ` is itself total, so + // recursing would stack a redundant guard on the same column. The guard + // therefore appears exactly once, innermost. + expect(call.sql).toBe(`${BARE_SCAN} WHERE NOT (NOT ((("stage" IS NOT NULL) AND ("stage" = ?))))`); expect(call.args).toEqual(['won']); expectBindsBalanced(call); }); it('compiles `$not` over a nested `$or`', async () => { const call = await compile({ $not: { $or: [{ stage: 'won' }, { stage: 'lost' }] } }); - expect(call.sql).toBe(`${BARE_SCAN} WHERE NOT ((("stage" = ?) OR ("stage" = ?)))`); + // De Morgan is sound over two-valued leaves, which is the whole reason the + // guard rides each LEAF rather than the `NOT`: hoisting it above this + // `$or` would let a NULL `stage` satisfy the negation even when the other + // disjunct is satisfied. + expect(call.sql).toBe( + `${BARE_SCAN} WHERE NOT ((((("stage" IS NOT NULL) AND ("stage" = ?))) OR ((("stage" IS NOT NULL) AND ("stage" = ?)))))`, + ); expect(call.args).toEqual(['won', 'lost']); expectBindsBalanced(call); }); it('compiles `$not` over a nested `$and`', async () => { const call = await compile({ $not: { $and: [{ stage: 'won' }, { amount: 10 }] } }); - expect(call.sql).toBe(`${BARE_SCAN} WHERE NOT ((("stage" = ?) AND ("amount" = ?)))`); + expect(call.sql).toBe( + `${BARE_SCAN} WHERE NOT ((((("stage" IS NOT NULL) AND ("stage" = ?))) AND ((("amount" IS NOT NULL) AND ("amount" = ?)))))`, + ); expect(call.args).toEqual(['won', 10]); }); it('carries operator maps through the negation with their binds in order', async () => { const call = await compile({ $not: { amount: { $gte: 10, $lt: 100 } } }); - expect(call.sql).toBe(`${BARE_SCAN} WHERE NOT ("amount" >= ? AND "amount" < ?)`); + // ONE guard for the whole field constraint, not one per operator: the + // constraint is the AND of its operators, so a NULL column satisfies it + // only if it satisfies all of them — and it satisfies neither bound. + expect(call.sql).toBe( + `${BARE_SCAN} WHERE NOT ((("amount" IS NOT NULL) AND ("amount" >= ? AND "amount" < ?)))`, + ); expect(call.args).toEqual([10, 100]); expectBindsBalanced(call); }); @@ -243,20 +284,29 @@ describe('RemoteTransport $not (#1076)', () => { // 'lost')`. Pre-fix the whole read THREW, so the scope was unusable on // Turso remote while working locally. const call = await compile({ $and: [{ owner_id: 'u1' }, { $not: { stage: 'lost' } }] }); - expect(call.sql).toBe(`${BARE_SCAN} WHERE (("owner_id" = ?) AND (NOT ("stage" = ?)))`); + expect(call.sql).toBe( + `${BARE_SCAN} WHERE (("owner_id" = ?) AND (NOT ((("stage" IS NOT NULL) AND ("stage" = ?)))))`, + ); expect(call.args).toEqual(['u1', 'lost']); expectBindsBalanced(call); }); it('compiles a `$not` inside an `$or` branch', async () => { const call = await compile({ $or: [{ $not: { stage: 'lost' } }, { owner_id: 'u1' }] }); - expect(call.sql).toBe(`${BARE_SCAN} WHERE ((NOT ("stage" = ?)) OR ("owner_id" = ?))`); + expect(call.sql).toBe( + `${BARE_SCAN} WHERE ((NOT ((("stage" IS NOT NULL) AND ("stage" = ?)))) OR ("owner_id" = ?))`, + ); expect(call.args).toEqual(['lost', 'u1']); }); it('keeps a `$not` sibling of plain field keys at the same level', async () => { const call = await compile({ owner_id: 'u1', $not: { stage: 'lost' } }); - expect(call.sql).toBe(`${BARE_SCAN} WHERE "owner_id" = ? AND NOT ("stage" = ?)`); + // The guard stays INSIDE the negated group. This is the assertion that + // fails if it were ever spliced into the node's bare ` AND ` join, where + // a stray `OR` would bind looser than the AND and widen the whole filter. + expect(call.sql).toBe( + `${BARE_SCAN} WHERE "owner_id" = ? AND NOT ((("stage" IS NOT NULL) AND ("stage" = ?)))`, + ); expect(call.args).toEqual(['u1', 'lost']); }); @@ -351,9 +401,16 @@ describe('RemoteTransport $not (#1076)', () => { await t.count('deal', { where } as unknown as QueryAST); await t.deleteMany('deal', { where } as any); await t.updateMany('deal', { where } as any, { stage: 'lost' }); - expect(calls[0].sql).toMatch(/COUNT\(\*\).+WHERE NOT \("stage" = \?\)/i); - expect(calls[1].sql).toBe('DELETE FROM "deal" WHERE NOT ("stage" = ?)'); - expect(calls[2].sql).toMatch(/^UPDATE "deal" SET .* WHERE NOT \("stage" = \?\)$/); + // The one negated predicate all three statements must carry, verbatim — + // a WHERE that is right for `find` and wrong for `deleteMany` is how a + // filter fix becomes a data-loss bug. + const NEGATION = 'WHERE NOT ((("stage" IS NOT NULL) AND ("stage" = ?)))'; + expect(calls[0].sql).toMatch(/COUNT\(\*\)/i); + expect(calls[0].sql).toContain(NEGATION); + expect(calls[1].sql).toBe(`DELETE FROM "deal" ${NEGATION}`); + expect(calls[2].sql).toMatch( + /^UPDATE "deal" SET .* WHERE NOT \(\(\("stage" IS NOT NULL\) AND \("stage" = \?\)\)\)$/, + ); for (const call of calls) expectBindsBalanced(call); }); @@ -425,21 +482,34 @@ describe('TursoDriver remote — $not on real rows (#1076)', () => { it('`$not: { stage: "won" }` returns the other rows, not a `no such column` error', async () => { // Pre-fix: threw before reaching SQLite; had it compiled, SQLite would have - // answered `no such column: $not`. - expect(await ids({ $not: { stage: 'won' } })).toEqual(['d_lost', 'd_open']); + // answered `no such column: $not`. `d_null` joins the answer at #5903 — see + // the next case. + expect(await ids({ $not: { stage: 'won' } })).toEqual(['d_lost', 'd_null', 'd_open']); }); - it('drops the NULL-`stage` row — SQL three-valued logic, as `whereNot` does locally', async () => { + it('RETURNS the NULL-`stage` row — the #5146 ruling, on this face since #5903', async () => { + // The direction of this case is REVERSED, deliberately. It read: "drops the + // NULL-`stage` row — SQL three-valued logic, as `whereNot` does locally. // `d_null` is absent above and here. This is the SQL family's answer: // `NOT (NULL = 'won')` is UNKNOWN, not TRUE. `driver-memory` and // `matchesFilterCondition` would return it. Remote mode is pinned to local // SqlDriver so the two SQL transports cannot disagree; the cross-family - // divergence is objectstack#5146. + // divergence is objectstack#5146." + // + // #5146 ruled that cross-family divergence — the JS answer is canonical, a + // column with no value does not satisfy the negated condition — and PR + // #5296 landed it on `SqlDriver`. The last clause of the old comment is + // what makes this a flip rather than a regression: "the two SQL transports + // cannot disagree" stopped being true the moment local inherited the fix, + // and it is true again only with `d_null` on this side of the answer. const rows = await ids({ $not: { stage: 'won' } }); - expect(rows).not.toContain('d_null'); - // …and the row IS reachable — it is excluded by the negation's semantics, - // not missing from the table. + expect(rows).toContain('d_null'); + // …and the row is still identifiable as the no-value row, so this is not + // passing because the seed lost its NULL. expect(await ids({ stage: null })).toEqual(['d_null']); + // The complement still excludes it: negation is not a licence to return + // everything. + expect(await ids({ $not: { stage: { $ne: 'won' } } })).toEqual(['d_won']); }); it('`$not: {}` returns ZERO rows', async () => { @@ -455,7 +525,12 @@ describe('TursoDriver remote — $not on real rows (#1076)', () => { }); it('negates a nested `$or` (De Morgan on real rows)', async () => { - expect(await ids({ $not: { $or: [{ stage: 'won' }, { stage: 'lost' }] } })).toEqual(['d_open']); + // `d_null` is in the answer for the same reason it is above: it satisfies + // neither disjunct, so it satisfies the negation. + expect(await ids({ $not: { $or: [{ stage: 'won' }, { stage: 'lost' }] } })).toEqual([ + 'd_null', + 'd_open', + ]); }); it('answers the RLS shape — owner AND NOT lost', async () => { @@ -488,7 +563,19 @@ describe('TursoDriver remote — $not on real rows (#1076)', () => { }); // `d_lost` closed WITHIN 2025-01-15, so the negation excludes it; `d_won` // closed later, so the negation keeps it. Un-lowered, both come back. - expect(await ids({ $not: { closed_at: { $lte: '2025-01-15' } } })).toEqual(['d_won']); + // + // [#5903] `d_open` and `d_null` never got a `closed_at`, and a row with no + // value does not satisfy `closed_at <= …`, so the negation now returns + // them. That is the SAME ruling as the case above applied to a range + // operator — the guard is `requireValue` for every positive comparison — + // and it is what LOCAL mode has answered since PR #5296. The lowering + // assertion is unaffected: `d_lost` is still excluded, which is the only + // way to tell a lowered upper bound from an un-lowered one. + expect(await ids({ $not: { closed_at: { $lte: '2025-01-15' } } })).toEqual([ + 'd_null', + 'd_open', + 'd_won', + ]); }); it('lowers `$between` inside `$not` instead of refusing it', async () => { @@ -501,7 +588,11 @@ describe('TursoDriver remote — $not on real rows (#1076)', () => { }); it('`count` agrees with `find`', async () => { - expect(await driver.count('deal', { object: 'deal', where: { $not: { stage: 'won' } } })).toBe(2); + // Three since #5903 — `d_null` is inside the negation now, and a count that + // disagreed with the list under it is exactly the local/remote split this + // issue closed, wearing a total instead of a row set. + expect(await driver.count('deal', { object: 'deal', where: { $not: { stage: 'won' } } })).toBe(3); + expect((await ids({ $not: { stage: 'won' } })).length).toBe(3); expect(await driver.count('deal', { object: 'deal', where: { $not: {} } })).toBe(0); }); diff --git a/packages/drivers/driver-turso/src/remote-transport-null-comparand-refusal.test.ts b/packages/drivers/driver-turso/src/remote-transport-null-comparand-refusal.test.ts index 2338f4ed6b..1a29fba290 100644 --- a/packages/drivers/driver-turso/src/remote-transport-null-comparand-refusal.test.ts +++ b/packages/drivers/driver-turso/src/remote-transport-null-comparand-refusal.test.ts @@ -251,18 +251,52 @@ describe('RemoteTransport $null comparand refusal (#1116)', () => { }); }); - describe('(d) the scope fence #1116 draws, pinned so it is not crossed by accident', () => { - it('leaves `$exists` reading a non-boolean exactly as it did', async () => { - // `$exists` carries the identical `=== false` bisection one arm below, - // and is deliberately NOT tightened: its divergence is on another axis - // (whether "exists" means key-present or has-value — objectstack#5299, - // reopened as #5369), so tightening it HERE alone would manufacture a - // local/remote fork rather than close one. framework left its twin alone - // for the same reason. When #5299 is ruled on, this test is the one to - // change — deliberately, not incidentally. - expect((await compile({ stage: { $exists: 'yes' } })).sql).toBe( - `${BARE_SCAN} WHERE "stage" IS NOT NULL`, - ); + describe('(d) the fence #1116 drew around `$exists`, now TAKEN DOWN (#5369 / #5903)', () => { + // The block this replaces asserted the opposite, and named the condition + // for its own reversal: "`$exists` carries the identical `=== false` + // bisection one arm below, and is deliberately NOT tightened: its + // divergence is on another axis (whether 'exists' means key-present or + // has-value — objectstack#5299, reopened as #5369), so tightening it HERE + // alone would manufacture a local/remote fork rather than close one. When + // #5299 is ruled on, this test is the one to change — deliberately, not + // incidentally." + // + // Both conditions have now been met, so this is that deliberate change: + // + // - #5298's four-point ruling (2026-08-06) settled `$exists` on **has a + // value**, the reading this transport already compiled — so the "another + // axis" the fence was protecting is closed, and `$exists` / `$null` are + // strict mirrors. + // - PR #5962 landed the refusal on `driver-sql`, which Turso LOCAL + // inherits. Keeping the fence up stopped preventing a local/remote fork + // and started BEING one: measured on `origin/main` against the shared + // conformance fixture, `{ d: { $exists: 'yes' } }` threw `INVALID_FILTER` + // locally and returned the valued rows remotely. + it('refuses a non-boolean `$exists` comparand, as `$null` does', async () => { + for (const value of ['yes', 1, 0, null, undefined, {}, 'false', [], 'true']) { + const err = await refusalOf({ stage: { $exists: value } }); + expect(err.code, JSON.stringify(value) ?? 'undefined').toBe('INVALID_FILTER'); + expect(err.status).toBe(400); + expect(err.message).toContain( + 'Operator "$exists" on field "stage" requires a boolean comparand (true or false).', + ); + // The location convention every other refusal in this file uses. + expect(err.message).toContain(`'deal.stage'.$exists`); + } + }); + + it('names the STRING `"false"` as the trap it is', async () => { + // The same trap the `$null` twin calls out: `'false'` is truthy, so the + // spelling most likely to arrive from a JSON round-trip or a template + // concatenation compiled to `IS NOT NULL` — the exact opposite of what + // its author meant. + const err = await refusalOf({ stage: { $exists: 'false' } }); + expect(err.message).toContain('"false" the STRING is truthy'); + }); + + it('still compiles the two booleans, unchanged', async () => { + // The guard adds no third answer: `$exists` keeps the mirror of `$null` + // it has always had. expect((await compile({ stage: { $exists: true } })).sql).toBe( `${BARE_SCAN} WHERE "stage" IS NOT NULL`, ); @@ -271,6 +305,35 @@ describe('RemoteTransport $null comparand refusal (#1116)', () => { ); }); + it('refuses at every depth, and executes no statement', async () => { + // Same depths the `$null` gate is pinned at — the arm runs inside the + // operator loop, so a nested spelling must not slip past it. + const { t, calls } = transportWithCapturingClient(); + for (const where of [ + { $and: [{ stage: { $exists: 'yes' } }] }, + { $or: [{ stage: 'won' }, { stage: { $exists: 1 } }] }, + { $not: { stage: { $exists: 'yes' } } }, + { $or: [{}, { stage: { $exists: 'false' } }] }, + ]) { + await expect(t.find('deal', { where } as unknown as QueryAST)).rejects.toThrow( + /Operator "\$exists" .* requires a boolean comparand/, + ); + } + expect(calls).toEqual([]); + }); + + it('refuses on the writing entry points too — the direction that costs the most', async () => { + const { t, calls } = transportWithCapturingClient(); + const where = { stage: { $exists: 'false' } }; + await expect(t.deleteMany('deal', { where } as any)).rejects.toThrow( + /requires a boolean comparand/, + ); + await expect(t.updateMany('deal', { where } as any, { stage: 'archived' })).rejects.toThrow( + /requires a boolean comparand/, + ); + expect(calls).toEqual([]); + }); + it('leaves the regex family taking a non-string comparand', async () => { // Measured consistent and fail-closed across backends; `driver-sql`'s // #5041 comment excludes the family from its guard by name. @@ -295,6 +358,7 @@ describe('RemoteTransport $null comparand refusal (#1116)', () => { ['a non-node `$not` operand (#1076)', { $not: 'won' }], ['a non-node top-level `where` (#1075)', [['stage', '=', 'won']]], ['a non-boolean `$null` comparand (#1116)', { stage: { $null: 'yes' } }], + ['a non-boolean `$exists` comparand (#5369/#5903)', { stage: { $exists: 'yes' } }], ]; for (const [label, where] of FILTER_REFUSALS) { diff --git a/packages/drivers/driver-turso/src/remote-transport-top-level-where.test.ts b/packages/drivers/driver-turso/src/remote-transport-top-level-where.test.ts index 34a28e707b..58c0c4928b 100644 --- a/packages/drivers/driver-turso/src/remote-transport-top-level-where.test.ts +++ b/packages/drivers/driver-turso/src/remote-transport-top-level-where.test.ts @@ -249,8 +249,11 @@ describe('RemoteTransport top-level `where` refusal (#1075)', () => { ); expect((await compile({ $and: [] })).sql).toBe(BARE_SCAN); expect((await compile({ $or: [] })).sql).toBe(`${BARE_SCAN} WHERE 1 = 0`); + // [#5903] The `$not` operand carries the #5146 NULL guard on this face + // now. What #1075 pins is that a well-formed top-level `where` still + // COMPILES rather than being refused by the node gate — which it does. expect((await compile({ $not: { stage: 'won' } })).sql).toBe( - `${BARE_SCAN} WHERE NOT ("stage" = ?)`, + `${BARE_SCAN} WHERE NOT ((("stage" IS NOT NULL) AND ("stage" = ?)))`, ); expect((await compile({ closed_at: null })).sql).toBe(`${BARE_SCAN} WHERE "closed_at" IS NULL`); }); diff --git a/packages/drivers/driver-turso/src/remote-transport.ts b/packages/drivers/driver-turso/src/remote-transport.ts index dbc04f0589..12da1bba74 100644 --- a/packages/drivers/driver-turso/src/remote-transport.ts +++ b/packages/drivers/driver-turso/src/remote-transport.ts @@ -200,6 +200,219 @@ function isFilterNode(value: unknown): value is Record { return proto === Object.prototype || proto === null; } +// ── [#5146 / #5298] NULL-safe negation ─────────────────────────────────────── +// +// The THIRD implementation of one ruling, and deliberately so. `driver-sql` +// carries it as `nullSafeNegationOperand` (a module-private function over a +// knex builder's operand) and `service-analytics`'s `read-scope-sql.ts` carries +// it a second time for the RLS read lowering; that file's header records why a +// copy was preferable to an import, and the same two reasons hold here: +// +// 1. **This module imports no driver.** Its header states it outright — "No +// local SQLite or Knex dependency" — and `driver-sql`'s entry point pulls in +// knex. The one exportable piece is not the emitter, it is the RULING. +// 2. **Each polarity table is matched to its OWN emitter, not copied from +// another one.** `read-scope-sql` reads `$null`/`$exists` by truthiness +// because its emitter writes `val ? … : …`; `driver-sql` reads them by +// identity against `false` because its emitter does. This transport reads +// them by identity too — see the two arms below — because that is what +// `buildWhereSQL` emits. The invariant is the agreement of guard with +// emitter, not the sameness of the source text. +// +// What holds the three to one answer is not their text but the shared case +// table: `FILTER_LOGIC_CASES` runs the `$ne` and `$not` rows through all eleven +// harnesses, and `turso-local-remote-null-parity.test.ts` in this package runs +// the same filters through BOTH faces of this driver — which is the divergence +// #5903 actually was. + +/** + * What one field constraint needs so its compiled SQL is TOTAL — TRUE or FALSE + * for every row, never UNKNOWN. + * + * - `'none'` — already total (`IS NULL` / `IS NOT NULL`), or a shape + * this compiler refuses outright, which must keep refusing. + * - `'requireValue'` — a NULL column does NOT satisfy it: `col IS NOT NULL AND (…)`. + * - `'allowNull'` — a NULL column DOES satisfy it: `col IS NULL OR (…)`. + */ +type NullGuard = 'none' | 'requireValue' | 'allowNull'; + +/** + * Does a NULL column satisfy this one operator, under the semantics #5146 and + * #5298 ruled canonical — the two-valued answer the JS backends (`driver-memory` + * `match`, `formula` `matchesFilterCondition`) give a value that is not there? + * + * The default is the large positive-comparison family (`$gt`, `$in`, + * `$contains`, `$startsWith`, `$endsWith`, `$regex`, and any operator this + * transport refuses outright), every member of which answers `false` for a value + * that is not there. + */ +function nullValueSatisfiesOperator(op: string, value: unknown): boolean { + switch (op) { + // `$eq: null` IS the null predicate (the emitter writes `IS NULL`); any + // other comparand is a value test a NULL column fails. + case '$eq': return value === null || value === undefined; + // Mirror image: `$ne: null` compiles to `IS NOT NULL`, which a NULL fails. + // This is the polarity-by-COMPARAND rule #5298 is explicit about — `$ne` + // does not get one answer because of its name. + case '$ne': return !(value === null || value === undefined); + // Read by IDENTITY, matching this transport's emitter, and total over the + // declared domain because both comparands are now refused unless boolean + // (`$null` since #1116/#5347, `$exists` since #5903 — see + // {@link RemoteTransport.nonBooleanExistsComparand}). + case '$null': return value === true; + // `$null: true` and `$exists: false` are the same question, so these two + // arms are each other's MIRROR, not each other's copy. + case '$exists': return value === false; + // Negative-polarity set / substring tests hold vacuously for an absent + // value — the #5298 half of the ruling. + case '$nin': return true; + // `$notContains` is the one operator the two JS backends disagree on for a + // null-valued field (`driver-memory` answers false because `typeof null !== + // 'string'`; `formula` answers true). `formula` is followed because + // `driver-sql` follows it, so this transport casts no vote on a + // disagreement that is filed elsewhere. + case '$notContains': return true; + default: return false; + } +} + +/** Is this operator's compiled SQL already total for a NULL column? */ +function operatorIsNullTotal(op: string, value: unknown): boolean { + switch (op) { + // Compile to `IS NULL` / `IS NOT NULL` — two-valued by construction. + case '$null': + case '$exists': + return true; + // A null (or absent) comparand makes these null PREDICATES too, not + // comparisons — see the `$eq` / `$ne` arms of the emitter. + case '$eq': + case '$ne': + return value === null || value === undefined; + default: + return false; + } +} + +/** + * The guard one field constraint needs. A constraint is the AND of its + * operators, so it is total when every operator is, and a NULL column satisfies + * it only when it satisfies all of them. + */ +function nullGuardForFieldSpec(spec: unknown): NullGuard { + // `{ field: null }` / `{ field: undefined }` compile to `IS NULL` — already + // total. (`undefined` is NOT lumped in with the scalars below: this + // transport's field arm reads it as the null predicate, and a guard classified + // from another emitter's reading would contradict what this one emits.) + if (spec === null || spec === undefined) return 'none'; + // A scalar / Date is an implicit `=`; a NULL column fails it. A bare array is + // REFUSED by `serializeComparand`; classifying it here keeps that refusal + // reachable — the unrewritten `{field: […]}` conjunct still throws its own + // message. + if (typeof spec !== 'object' || isBindableObjectComparand(spec) || Array.isArray(spec)) return 'requireValue'; + const entries = Object.entries(spec as Record); + // `{ field: {} }` is refused by {@link RemoteTransport.emptyFieldFilter} and a + // non-`$` key by {@link RemoteTransport.unsupportedOperator}. Passing them + // through unrewritten is what preserves the exact message; wrapping them in a + // guard would only change which error the caller reads. + if (entries.length === 0) return 'none'; + let total = true; + let nullSatisfies = true; + for (const [op, value] of entries) { + if (!operatorIsNullTotal(op, value)) total = false; + if (!nullValueSatisfiesOperator(op, value)) nullSatisfies = false; + } + if (total) return 'none'; + return nullSatisfies ? 'allowNull' : 'requireValue'; +} + +/** + * [#5146] Rewrite the operand of a `$not` so every leaf compiles to a TOTAL + * predicate — which is what makes `NOT (…)` mean here what it means in + * `driver-memory`, `formula`, `driver-sql` (since #5296) and this driver's own + * LOCAL transport. + * + * # The defect + * + * SQL is three-valued: `NULL = 'won'` is UNKNOWN, `NOT UNKNOWN` is still + * UNKNOWN, and a `WHERE` keeps only TRUE — so `{ $not: { stage: 'won' } }` + * dropped every row whose `stage` is NULL. Measured on `origin/main` + * (2026-08-06) against the shared fixture: LOCAL answered `['2','3','4']` and + * REMOTE `['2']`, for one filter whose only difference was the `url` it was sent + * to. On a CEL `!expr` read scope lowered by `cel-to-filter.ts` that is not a + * count that differs — it is the SAME permission rule admitting a different set + * of rows per connection mode. + * + * # Why the guard rides the LEAF, not the `NOT` + * + * For a flat operand `NOT (a IS NOT NULL AND a = ?)` and `NOT (a = ?) OR a IS + * NULL` are the same predicate. They stop being the same as soon as the operand + * nests: hoisting the guard above a `$not` whose operand is a `$or` re-admits + * rows the JS backends exclude — a NULL `a` would satisfy the whole negation + * even when the `$or`'s OTHER branch is satisfied. Totalising each leaf makes + * the rewrite compositional instead: De Morgan is sound over two-valued leaves, + * so `$and`, `$or` and a nested `$not` all stay correct with no special cases. + * + * # Why polarity is per operator + * + * A blanket `OR col IS NULL` would WIDEN the negative-polarity operators: + * `{ $not: { a: { $ne: 5 } } }` means "a is 5", and both JS backends exclude a + * NULL row from it. Adding an unconditional null escape there would hand back + * exactly the rows the filter excludes. So each leaf is guarded in the direction + * its OWN operator answers, per {@link nullValueSatisfiesOperator}. + * + * # Why it is expressed as a filter TREE, not as SQL + * + * The guards are emitted as ordinary `$null` constraints inside `$and` / `$or` + * nodes and handed back to {@link RemoteTransport.buildWhereSQL}, so every + * parenthesis is the one that compiler already writes for a combinator. This + * transport joins a node's keys with a bare ` AND `; a hand-built `col IS NULL + * OR …` spliced into that list would bind LOOSER than the AND and silently widen + * the whole filter — the trap #5298's own driver-side twin records. Compiling + * the guard as structure makes that unrepresentable. + * + * A non-node `$and` / `$or` element is passed through untouched so + * {@link RemoteTransport.buildSubFilterSQL} still refuses it by name (#1073), + * and a nested `$not` is left alone because its own branch totalises its + * operand — `NOT ` is itself total, so recursing would stack a redundant + * guard on the same column. + */ +function nullSafeNegationOperand(node: Record): Record { + const out: Record = {}; + const guarded: unknown[] = []; + for (const [key, value] of Object.entries(node)) { + if ((key === '$and' || key === '$or') && Array.isArray(value)) { + out[key] = value.map((element) => + isFilterNode(element) ? nullSafeNegationOperand(element) : element, + ); + continue; + } + if (key.startsWith('$')) { + // `$not` (totalised by its own branch) and anything else `$`-prefixed keep + // whatever this transport does with them today — the rewrite rules on + // NULL, not on the operator vocabulary, and an undeclared combinator must + // still reach the #5769 gate that refuses it. + out[key] = value; + continue; + } + const guard = nullGuardForFieldSpec(value); + if (guard === 'none') { + out[key] = value; + } else if (guard === 'requireValue') { + // `col IS NOT NULL AND (…)` — both conjuncts of the enclosing node. + guarded.push({ [key]: { $null: false } }, { [key]: value }); + } else { + // `col IS NULL OR (…)` — ONE conjunct, so the OR binds tighter than the + // AND this node's keys form. + guarded.push({ $or: [{ [key]: { $null: true } }, { [key]: value }] }); + } + } + if (guarded.length > 0) { + const existing = Array.isArray(out.$and) ? out.$and : []; + out.$and = [...existing, ...guarded]; + } + return out; +} + /** How long a refused comparand may be echoed back in an error message. */ const COMPARAND_PREVIEW_LIMIT = 120; @@ -1263,13 +1476,26 @@ export class RemoteTransport { // path, i.e. straight back into the bug. Every `$not` is compiled // here, and a value that is not a filter node is refused BY NAME. // - // NULL semantics are SQL's, matching what `whereNot` emits locally: - // `NOT ("stage" = ?)` is UNKNOWN for a row whose `stage` is NULL, so - // that row is not returned. `driver-memory`/`matchesFilterCondition` - // 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, path); + // [#5146 → #5903] NULL semantics are NO LONGER "SQL's, whatever they + // are". This comment used to argue the opposite — `NOT ("stage" = ?)` + // is UNKNOWN for a NULL `stage`, so that row was dropped, and remote was + // "pinned to the family it belongs to" so local and remote SQL agreed. + // That premise expired the day PR #5296 landed #5146 on + // `SqlDriver.applyFilterCondition`: LOCAL mode inherits that fix + // (`TursoDriver extends SqlDriver`), this independent compiler inherited + // none of it, and the two faces of ONE driver started answering one + // filter by connection mode. #5146 ruled the JS backends' two-valued + // answer canonical — "the column has no value" does NOT satisfy the + // negated condition, so the row IS returned. + // + // The rewrite totalises every leaf of the operand BEFORE it is compiled, + // so `NOT (…)` negates a predicate that is TRUE or FALSE for every row + // and never UNKNOWN. Only a filter NODE is rewritten: a non-node operand + // must reach `buildSubFilterSQL` exactly as the caller wrote it, so the + // refusal keeps naming the shape that was sent (the branch above has no + // `isFilterNode` guard for precisely that reason). + const operand = isFilterNode(value) ? nullSafeNegationOperand(value) : value; + const { whereClauses: sc, args: sa } = this.buildSubFilterSQL(object, key, null, operand, path); if (sc) { clauses.push(`NOT (${sc})`); args.push(...sa); @@ -1304,10 +1530,15 @@ export class RemoteTransport { break; case '$ne': if (opValue === null || opValue === undefined) { + // [#5298] UNCHANGED, and deliberately: `IS NOT NULL` is already + // TOTAL, and both sides of the ruling agree a row with no value + // does NOT have "any value". Polarity follows the COMPARAND, not + // the operator's name — only the value COMPARISON below becomes + // NULL-safe. clauses.push(`${column} IS NOT NULL`); } else { const bind = this.serializeComparand(object, key, op, opValue); - clauses.push(`${field} <> ?`); + clauses.push(this.nullSafeNegative(column, `${field} <> ?`)); args.push(bind); } break; @@ -1340,7 +1571,11 @@ export class RemoteTransport { const binds = ninVals.map((v: any, i: number) => this.serializeComparand(object, key, `${op}[${i}]`, v), ); - clauses.push(`${field} NOT IN (${binds.map(() => '?').join(', ')})`); + // [#5298] NULL-safe: "not among this list" holds vacuously for a + // value that is not there, which is what every JS backend answers. + clauses.push( + this.nullSafeNegative(column, `${field} NOT IN (${binds.map(() => '?').join(', ')})`), + ); args.push(...binds); break; } @@ -1365,7 +1600,19 @@ export class RemoteTransport { this.pushLike(clauses, args, column, this.serializeComparand(object, key, op, opValue), 'contains'); break; case '$notContains': - this.pushLike(clauses, args, column, this.serializeComparand(object, key, op, opValue), 'contains', true); + // [#5298] NULL-safe: `NOT LIKE` is UNKNOWN for a NULL column, and + // "does not contain" is true of a value that is not there. The + // guard is applied INSIDE `pushLike` so the LIKE family keeps its + // single emission point. + this.pushLike( + clauses, + args, + column, + this.serializeComparand(object, key, op, opValue), + 'contains', + true, + true, + ); break; case '$startsWith': this.pushLike(clauses, args, column, this.serializeComparand(object, key, op, opValue), 'starts'); @@ -1397,13 +1644,41 @@ export class RemoteTransport { clauses.push(`${column} IS ${opValue ? 'NULL' : 'NOT NULL'}`); break; case '$exists': - // Deliberately NOT given the same guard (#1116's scope fence). - // `$exists` carries the identical `=== false` bisection, but its - // divergence is on another axis — whether "exists" means "key - // present" or "has a value" is objectstack#5299's open question, - // reopened as #5369 — and framework left it alone for exactly - // this reason. Tightening it here ALONE would manufacture a - // local/remote fork rather than close one. + // [#5369 → #5903] The fence this arm used to carry is DOWN. It + // read: "deliberately NOT given the same guard — whether `exists` + // means 'key present' or 'has a value' is #5299's open question, + // and tightening it here ALONE would manufacture a local/remote + // fork rather than close one." Both halves of that reasoning have + // since been answered, in the caller's favour: + // + // - #5298's four-point ruling (2026-08-06) settled the semantics + // on **has a value** — field existence is a property of the + // SCHEMA, not of a record — so `$exists` and `$null` are strict + // mirrors. This arm's `IS NULL` / `IS NOT NULL` was already + // that reading and does not move. + // - PR #5962 landed the non-boolean refusal on `driver-sql`, so + // LOCAL mode now refuses what this arm still answered. The fork + // the fence was protecting against EXISTS TODAY, pointing the + // other way: measured on `origin/main`, `{ $exists: 'yes' }` + // threw `INVALID_FILTER` locally and returned the valued rows + // remotely. Closing it is what this guard does. + // + // Refused for the reason `$null` is (#5347-A): `opValue === false` + // is a BISECTION, so every non-boolean — `'yes'`, `1`, `0`, `null`, + // `undefined`, `{}` and the string `'false'` — landed on the + // `NOT NULL` side and answered as if `true` had been written. + if (typeof opValue !== 'boolean') { + throw this.nonBooleanExistsComparand(object, key, opValue); + } + // Left as the `=== false` identity test rather than rewritten to a + // total two-way choice the way the `$null` arm above was. The two + // are not the same edit: `$null`'s emitter had `true` on its + // DEFAULT side, so a third value read as "the caller asked for + // null"; here `false` is the value being tested FOR, and with the + // guard holding there is no third value left for a default to + // catch. Its polarity twin in {@link nullValueSatisfiesOperator} + // is spelled `value === false` for exactly the same reason — + // `$null: true` and `$exists: false` are one question asked twice. clauses.push(`${column} IS ${opValue === false ? 'NULL' : 'NOT NULL'}`); break; default: @@ -1482,13 +1757,46 @@ export class RemoteTransport { value: unknown, shape: LikeShape, negate = false, + nullSafe = false, ): void { const escaped = String(value).replace(/[\\%_]/g, '\\$&'); const pattern = shape === 'starts' ? `${escaped}%` : shape === 'ends' ? `%${escaped}` : `%${escaped}%`; - clauses.push(`${column} ${negate ? 'NOT LIKE' : 'LIKE'} ? ESCAPE '\\'`); + const predicate = `${column} ${negate ? 'NOT LIKE' : 'LIKE'} ? ESCAPE '\\'`; + clauses.push(nullSafe ? this.nullSafeNegative(column, predicate) : predicate); args.push(pattern); } + /** + * [#5298] Wrap a negative-polarity value test so a row whose column has no + * value SATISFIES it: `(col IS NULL OR )`. + * + * The remote twin of `driver-sql`'s `applyNullSafeNegative` and + * `read-scope-sql`'s `nullSafeNegative`, and the only shape all three use: + * OR-expansion, never `IS DISTINCT FROM` / `IS NOT` / `<=>`. The three reasons + * are recorded on the driver-side twin and every one of them applies here with + * force — `NOT LIKE` has no such form at all, so `$notContains` would need the + * OR shape anyway and this compiler would carry two shapes for one ruling; the + * SQLite spelling depends on an engine version nothing pins (this transport + * talks to whatever libSQL the remote endpoint runs, which is further outside + * this repo's control than the local `sql.js`/`better-sqlite3` pair); and the + * measured query plans are identical either way, because `<>`, `NOT IN` and + * `NOT LIKE` were already full scans. + * + * **The parentheses are not optional.** {@link RemoteTransport.buildWhereSQL} + * joins a node's clauses with a bare ` AND `, so an unwrapped `col IS NULL OR + * …` would bind LOOSER than that AND: `a = ? AND b IS NULL OR b <> ?` parses as + * `(a = ? AND b IS NULL) OR b <> ?`, which returns rows the caller's `a` + * predicate excludes. A silently WIDENED filter is the bypass class this file + * keeps paying for (#1004, #1058, #1073), reached from a fourth direction. + * + * `column` is the PLAIN quoted column, never the {@link comparisonColumn} + * rewrite: whether a value is there is not something a storage form can + * change, which is the same reasoning the `$null` / `$exists` arms already use. + */ + private nullSafeNegative(column: string, test: string): string { + return `(${column} IS NULL OR ${test})`; + } + /** * The error for a `$null` whose comparand is not a boolean (#1116). * @@ -1552,6 +1860,46 @@ export class RemoteTransport { ); } + /** + * The error for an `$exists` whose comparand is not a boolean (#5369, landed + * on this face by #5903). + * + * The twin of {@link nonBooleanNullComparand} one method up, and written as a + * separate method rather than folded into a shared two-name helper on purpose: + * each operator's message names its OWN emitter's default direction, and one + * loop over `['$null', '$exists']` is where those two messages would start + * drifting into one imprecise sentence. + * + * The leading sentence is `driver-sql`'s `nonBooleanExistsComparandError`, + * verbatim, so a caller who hits this on Turso remote reads exactly what they + * would read on Postgres or on Turso LOCAL. Only the location is spelled in + * this transport's own convention — it names `'object.field'` rather than + * threading a `filter.…` path, matching every other refusal in this file. + * + * Measured on `origin/main` before this gate, against the shared conformance + * fixture (rows 1-2 valued, rows 3-4 NULL): `{ d: { $exists: v } }` for `v` in + * `'yes'`, `1`, `0`, `null`, `{}` each returned `['1','2']` — the answer for + * `$exists: true` — while the same five filters threw `INVALID_FILTER` on + * LOCAL mode. Five wrong answers and one right one, chosen by `url`. + */ + private nonBooleanExistsComparand(object: string, field: string, value: unknown): Error { + // `describeValue` calls `null` "an object" and `undefined` "a undefined" — + // both are among the comparands most likely to arrive here, so they are + // named outright, exactly as the `$null` twin does. + const shown = value === null ? 'null' : value === undefined ? 'undefined' : describeValue(value); + return invalidFilterError( + `[RemoteTransport] Operator "$exists" on field "${field}" requires a boolean comparand (true or ` + + `false). Received ${shown} (${preview(value)}) at '${object}.${field}'.$exists. ` + + `@objectstack/spec FieldOperatorsSchema declares $exists as a boolean. It is refused rather ` + + `than coerced for the same reason $null is (objectstack#5347): a non-boolean lands on ` + + `whichever side the backend's two-branch conditional happens to default to, and those ` + + `defaults point in OPPOSITE directions — this transport's \`=== false\` test compiled ` + + `IS NOT NULL for anything but false, a \`=== true\` test compiles IS NULL for anything but ` + + `true. Note "false" the STRING is truthy, so it landed on the side opposite the false it was ` + + `written to mean (objectstack#5369, objectstack#5903).`, + ); + } + /** * The error for an operator this transport does not compile. * diff --git a/packages/drivers/driver-turso/src/turso-local-remote-null-parity.test.ts b/packages/drivers/driver-turso/src/turso-local-remote-null-parity.test.ts new file mode 100644 index 0000000000..3175a56ff3 --- /dev/null +++ b/packages/drivers/driver-turso/src/turso-local-remote-null-parity.test.ts @@ -0,0 +1,299 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#5903] ONE `TursoDriver`, ONE answer — the two transports held against each + * other on the NULL / no-value family. + * + * # What this file is for + * + * #5903 was not "remote compiles a filter wrongly". It was **the same driver + * answering one filter two ways, chosen by the `url` it was constructed with**: + * `TursoDriver extends SqlDriver`, so LOCAL mode inherited #5146's NULL-safe + * `$not` (PR #5296) and #5298's NULL-safe `$ne`/`$nin`/`$notContains` (PR #5962) + * for free, while REMOTE mode compiles filters in + * `RemoteTransport.buildWhereSQL` — an independent emitter that inherited none + * of it. Measured on `origin/main` at `b5bdf48af`, against the fixture below: + * + * | filter | LOCAL | REMOTE | + * |---|---|---| + * | `{ d: { $ne: 'v1' } }` | `['2','3','4']` | `['2']` | + * | `{ d: { $nin: ['v1'] } }` | `['2','3','4']` | `['2']` | + * | `{ d: { $notContains: 'v1' } }`| `['2','3','4']` | `['2']` | + * | `{ $not: { d: 'v1' } }` | `['2','3','4']` | `['2']` | + * | `{ d: { $exists: 'yes' } }` | `INVALID_FILTER`| `['1','2']` | + * + * A filter-logic conformance suite exists for each transport already + * (`turso-filter-logic-conformance` / `turso-remote-filter-logic-conformance`), + * and since this PR both run the `$ne` and `$not` rows of `FILTER_LOGIC_CASES`. + * What they cannot do is fail on the DIFFERENCE: they are separate files with + * separate fixtures, so a divergence shows up as one suite red and the other + * green, in whichever order someone happens to read them. This file makes the + * difference itself the assertion. + * + * # Why it is the cross-pin for a second implementation + * + * `RemoteTransport` carries its own copy of the #5146/#5298 polarity table — + * `nullValueSatisfiesOperator` / `nullSafeNegationOperand` in + * `remote-transport.ts`, the third in-tree instance after `driver-sql`'s and + * `service-analytics`'s `read-scope-sql.ts`. The copy is deliberate (that file's + * header gives the two reasons), and a deliberate copy owes a pin that goes red + * when the copies drift. This is it, and it is stronger than a text comparison + * would be: it does not ask whether the two implementations LOOK alike, it asks + * whether they ANSWER alike, on rows, through the real driver. + * + * # Why every case asserts the canonical answer too + * + * Parity alone is satisfiable by breaking both transports the same way — the + * failure mode a "the two agree" suite invites. So each case carries the row ids + * the ruling requires, and both faces are checked against them. Agreement is + * necessary; agreement on the RIGHT answer is the assertion. + */ + +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import { FILTER_LOGIC_ROWS } from '@objectstack/spec/data'; +import { TursoDriver } from './turso-driver.js'; +import { makeLibsqlSqliteStub, type LibsqlSqliteStub } from './libsql-sqlite-stub.testkit.js'; +import type { QueryAST } from '@objectstack/spec/data'; + +/** + * The SHARED conformance fixture, not a private one. `d` is valued on rows 1-2 + * (`v1`, `v2`) and NULL on rows 3-4, which is the whole instrument: a filter + * that silently drops no-value rows loses exactly half the table. Reusing it + * means this suite and `FILTER_LOGIC_CASES` cannot drift apart about what a + * no-value row IS. + */ +const CONFORMANCE_OBJECT = { + name: 'conformance', + fields: { + a: { type: 'string' }, + b: { type: 'string' }, + c: { type: 'string' }, + d: { type: 'string' }, + owner: { type: 'string' }, + status: { type: 'string' }, + parent_object: { type: 'string' }, + parent_id: { type: 'string' }, + }, +}; + +const ids = (rows: Array>): string[] => + rows.map((r) => String(r.id)).sort((x, y) => x.localeCompare(y)); + +/** + * Every case both transports must answer identically, with the answer the + * #5146 / #5298 rulings require. + * + * Read the list as three groups: the four operators the rulings cover, the + * INVERSIONS that prove the polarity is per operator rather than a blanket null + * escape, and the shapes where a lost parenthesis would show up. + */ +const PARITY_CASES: Array<{ name: string; filter: unknown; expected: string[]; why: string }> = [ + // ── The four operators, positive direction: a no-value row IS returned ──── + { + name: '$ne excludes the value, keeps the rows that have none', + filter: { d: { $ne: 'v1' } }, + expected: ['2', '3', '4'], + why: '#5298. Bare `d <> ?` is UNKNOWN for a NULL d, so rows 3-4 vanished on remote.', + }, + { + name: '$nin holds vacuously for a value that is not there', + filter: { d: { $nin: ['v1'] } }, + expected: ['2', '3', '4'], + why: '#5298. `NOT IN` is UNKNOWN for a NULL column, exactly like `<>`.', + }, + { + name: '$nin over BOTH values still keeps the no-value rows', + filter: { d: { $nin: ['v1', 'v2'] } }, + expected: ['3', '4'], + why: 'The complement of the case above: nothing is left but the rows with no value, so a transport that drops them answers the empty set.', + }, + { + name: '$notContains is true of a value that is not there', + filter: { d: { $notContains: 'v1' } }, + expected: ['2', '3', '4'], + why: '#5298. `NOT LIKE` is UNKNOWN for a NULL column — and it is the operator with NO dialect one-operator form, which is why all three implementations use the OR expansion.', + }, + { + name: '$notContains matching every valued row leaves only the no-value rows', + filter: { d: { $notContains: 'v' } }, + expected: ['3', '4'], + why: 'Both v1 and v2 contain "v", so this is the LIKE half of the $nin complement above.', + }, + { + name: '$not of an equality returns the rows with no value', + filter: { $not: { d: 'v1' } }, + expected: ['2', '3', '4'], + why: '#5146, the original ruling. This is the shape a CEL `!expr` read scope lowers to.', + }, + + // ── The inversions: polarity is per OPERATOR, never a blanket null escape ─ + { + name: '$ne: null stays TOTAL — "has any value" is false for a row with none', + filter: { d: { $ne: null } }, + expected: ['1', '2'], + why: '#5298 is explicit that polarity follows the COMPARAND, not the operator name. `IS NOT NULL` is already total and must NOT be widened to include the rows it exists to exclude.', + }, + { + name: '$not of $ne means "is the value", and a no-value row is not it', + filter: { $not: { d: { $ne: 'v1' } } }, + expected: ['1'], + why: 'The widening trap #5146 names: a blanket `OR col IS NULL` under the negation would hand back rows 3-4 — exactly the rows the filter excludes.', + }, + { + name: '$not of $nin means "is among the list"', + filter: { $not: { d: { $nin: ['v1'] } } }, + expected: ['1'], + why: 'Same inversion through the set operator.', + }, + { + name: '$not of $notContains means "does contain"', + filter: { $not: { d: { $notContains: 'v1' } } }, + expected: ['1'], + why: 'Same inversion through the substring operator.', + }, + { + name: '$not of $null true is the valued rows', + filter: { $not: { d: { $null: true } } }, + expected: ['1', '2'], + why: '`IS NULL` is already total, so the rewrite must leave it alone — a guard stacked on it would compile a contradiction.', + }, + { + name: '$not of $exists false is the valued rows', + filter: { $not: { d: { $exists: false } } }, + expected: ['1', '2'], + why: '`$exists: false` and `$null: true` are one question asked twice (#5298 ③), so their negations must agree.', + }, + + // ── De Morgan, and the shapes a lost parenthesis leaks ──────────────────── + { + name: '$not over a nested $or', + filter: { $not: { $or: [{ d: 'v1' }, { d: 'v2' }] } }, + expected: ['3', '4'], + why: 'The guard rides each LEAF, not the NOT: hoisted above this $or, a NULL d would satisfy the negation even when the other disjunct is satisfied.', + }, + { + name: '$not over a nested $and', + filter: { $not: { $and: [{ d: 'v1' }, { a: 'x' }] } }, + expected: ['2', '3', '4'], + why: 'De Morgan is sound over two-valued leaves, so nesting needs no special case.', + }, + { + name: 'a NULL-safe $ne ANDs with a sibling key instead of leaking past it', + filter: { a: 'qq', d: { $ne: 'v1' } }, + expected: ['3', '4'], + why: 'THE parenthesis case. `buildWhereSQL` joins a node\'s clauses with a bare AND, so an unwrapped `d IS NULL OR d <> ?` parses as `(a = ? AND d IS NULL) OR d <> ?` and leaks row 2 — a row the caller\'s own `a` predicate excludes.', + }, + { + name: 'a NULL-safe $notContains ANDs with a sibling key too', + filter: { a: 'qq', d: { $notContains: 'v1' } }, + expected: ['3', '4'], + why: 'The same trap on the LIKE branch, where the guard is applied inside `pushLike` rather than at the call site.', + }, + { + name: 'a NULL-safe $ne inside one $or branch does not widen its sibling branch', + filter: { $or: [{ a: 'x', d: { $ne: 'v1' } }, { a: 'qq', b: 'y' }] }, + expected: ['2', '3'], + why: 'Row 1 (a=x, d=v1) must stay out and row 4 (a=qq, b=zz) must stay out; a leaked OR pulls both back in.', + }, +]; + +/** Every non-boolean `$exists` comparand both transports must REFUSE. */ +const NON_BOOLEAN_EXISTS: unknown[] = ['yes', 1, 0, null, undefined, {}, 'false', []]; + +describe('[#5903] TursoDriver LOCAL and REMOTE answer the NULL family identically', () => { + let local: TursoDriver; + let remote: TursoDriver; + let stub: LibsqlSqliteStub; + + beforeAll(async () => { + local = new TursoDriver({ url: ':memory:' }); + expect(local.transportMode).toBe('local'); + await local.initObjects([CONFORMANCE_OBJECT]); + for (const row of FILTER_LOGIC_ROWS) { + await local.create('conformance', { ...row }, { 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(CONFORMANCE_OBJECT.name, CONFORMANCE_OBJECT); + for (const row of FILTER_LOGIC_ROWS) { + await remote.create('conformance', { ...row }); + } + }); + + afterAll(async () => { + await local.disconnect(); + await remote.disconnect(); + stub.close(); + }); + + /** + * The fixture control, on BOTH faces. A parity suite whose two sides seeded + * different rows compares nothing, and a `d` stored as `''` instead of NULL + * would turn every case below green for the wrong reason. + */ + it('both transports hold the same four rows, with `d` really NULL on 3 and 4', async () => { + expect(ids(await local.find('conformance', { object: 'conformance' }))).toEqual(['1', '2', '3', '4']); + expect(ids(await remote.find('conformance', { object: 'conformance' }))).toEqual(['1', '2', '3', '4']); + for (const driver of [local, remote]) { + expect(ids(await driver.find('conformance', { object: 'conformance', where: { d: { $null: true } } } as QueryAST))).toEqual(['3', '4']); + expect(ids(await driver.find('conformance', { object: 'conformance', where: { d: { $null: false } } } as QueryAST))).toEqual(['1', '2']); + } + }); + + for (const c of PARITY_CASES) { + it(c.name, async () => { + const localIds = ids(await local.find('conformance', { object: 'conformance', where: c.filter } as QueryAST)); + const remoteIds = ids(await remote.find('conformance', { object: 'conformance', where: c.filter } as QueryAST)); + // Agreement first — this is the assertion #5903 is about. + expect(remoteIds, `local/remote divergence. ${c.why}`).toEqual(localIds); + // …and agreement on the RIGHT answer, so a shared regression cannot pass + // by making both faces wrong in the same direction. + expect(localIds, c.why).toEqual(c.expected); + }); + } + + it('count() agrees with find() on both transports, case for case', async () => { + // A `count` that disagrees with the list under it is the same divergence + // wearing a total instead of a row set — and remote builds its COUNT + // statement separately from its SELECT. + for (const c of PARITY_CASES) { + expect(await local.count('conformance', { object: 'conformance', where: c.filter } as QueryAST), `local ${c.name}`).toBe( + c.expected.length, + ); + expect(await remote.count('conformance', { object: 'conformance', where: c.filter } as QueryAST), `remote ${c.name}`).toBe( + c.expected.length, + ); + } + }); + + it('both transports REFUSE a non-boolean `$exists` comparand', async () => { + // The fork that pointed the OTHER way: PR #5962 landed this refusal on + // `driver-sql` (so LOCAL inherited it) while remote kept answering, which + // made a filter out of contract throw on one url and return the valued rows + // on another. Both faces speak `INVALID_FILTER` / 400 now. + for (const value of NON_BOOLEAN_EXISTS) { + const where = { d: { $exists: value } }; + for (const [label, driver] of [['local', local], ['remote', remote]] as const) { + const err = (await driver + .find('conformance', { where } as unknown as QueryAST) + .catch((e) => e)) as Error & { code?: string; status?: number }; + expect(err, `${label} ${JSON.stringify(value) ?? 'undefined'}`).toBeInstanceOf(Error); + expect(err.code, label).toBe('INVALID_FILTER'); + expect(err.status, label).toBe(400); + expect(err.message, label).toContain( + 'requires a boolean comparand (true or false)', + ); + } + } + }); + + it('both transports still COMPILE the two booleans `$exists` declares', async () => { + for (const driver of [local, remote]) { + expect(ids(await driver.find('conformance', { object: 'conformance', where: { d: { $exists: true } } } as QueryAST))).toEqual(['1', '2']); + expect(ids(await driver.find('conformance', { object: 'conformance', where: { d: { $exists: false } } } as QueryAST))).toEqual(['3', '4']); + } + }); +}); diff --git a/packages/spec/src/data/filter-logic-conformance.ts b/packages/spec/src/data/filter-logic-conformance.ts index 569750b40e..9adff89bbb 100644 --- a/packages/spec/src/data/filter-logic-conformance.ts +++ b/packages/spec/src/data/filter-logic-conformance.ts @@ -70,59 +70,44 @@ * #5146 made `$not` NULL-safe and #5298 did the same for the non-negated * `$ne` / `$nin` / `$notContains`, so "the column has no value" now has ONE * cross-backend answer and belongs to the standard like any other. The - * {@link FilterLogicRow.d} column carries it; see the `d`-column cases below, - * and family 2 of the next section for the rows still waiting on a backend. + * {@link FilterLogicRow.d} column carries it, and the four `d`-column cases + * below enforce it on every backend. * * ## Case families that are RULED but not yet enrolled * - * Both remaining families were ruled by the maintainer and are implemented in - * some backends. Neither is in the table yet — a red row here does not enforce - * a ruling, it just turns another lane's unfinished work into this table's - * failure, and each family still has a blocker standing, named per family - * below. Add the rows in the PR that closes the gap, not before. + * The one family left was ruled by the maintainer and is implemented in some + * backends. It is not in the table yet — a red row here does not enforce a + * ruling, it just turns another lane's unfinished work into this table's + * failure, and the family still has a blocker standing, named below. Add the + * rows in the PR that closes the gap, not before. * - * (Family 1 of this note — the boolean identities of the empty combinators — - * is GONE because it graduated: the #5322 ruling took the identity reduction, - * both analytics compilers aligned, and its four rows now sit in - * {@link FILTER_LOGIC_CASES} below, enrolled on every backend. The family - * numbering of the two that remain is kept as their historical ids.) + * Two families have GRADUATED out of this note, and how they did is the note's + * own instruction manual. Family 1 — the boolean identities of the empty + * combinators — went first: the #5322 ruling took the identity reduction, both + * analytics compilers aligned, and its four rows moved into + * {@link FILTER_LOGIC_CASES}. Family 2 — NULL-safe negation, `$not` (#5146) and + * `$ne`/`$nin`/`$notContains` (#5298), one family and not two because the second + * ruling extended the first — followed it in #5903, and is worth a sentence + * because of the shape of its blocker list rather than its content. It stood at + * two backends: `service-analytics`'s `filter-normalizer` (the Cube face) + * answered `['2']` for `$ne` while already answering `$not` correctly, and + * `driver-turso` REMOTE answered `['2']` for both. #5977 cleared the first by + * emitting the guard as tree STRUCTURE, so all three compilers of that tree + * agreed; #5903 cleared the second, and the two rows enrolled with it. * - * ### 2. NULL-safe negation — `$not` (#5146) and `$ne`/`$nin`/`$notContains` (#5298) - * - * A row whose column has no value does not satisfy the negated condition and IS - * returned. One family, not two: #5298 ruled the non-negated operators the same - * way #5146 ruled `$not`, so the rows land together or not at all. - * - * The FIXTURE half of this work is DONE (#5298): {@link FilterLogicRow.d} is - * nullable, all eleven harnesses declare and seed it, and the `$null` partition - * below proves they seeded it as NULL. What remains is ONE backend, measured - * against this fixture on 2026-08-06 rather than assumed: - * - * | backend | `{d: {$ne: 'v1'}}` | `{$not: {d: 'v1'}}` | blocker | - * |---|---|---|---| - * | `driver-turso` REMOTE | `['2']` | `['2']` | #5903 | - * - * Everything else already answers `['2','3','4']` on both: `driver-sql`, - * `driver-sqlite-wasm`, `driver-turso` LOCAL, `read-scope-sql`, `driver-memory` - * (both surfaces), `driver-mongodb` and `formula`. - * - * `service-analytics`'s `filter-normalizer` (the Cube face) was the second row - * of that table until #5977, answering `['2']` for `$ne` while already answering - * `$not` correctly — the #5146 rewrite lived in the normalizer, the three - * self-negating operators did not. It now emits the guard as tree STRUCTURE, so - * all three compilers of that tree (raw SQL, the ObjectQL engine condition and - * the display-SQL echo) answer `['2','3','4']`. That leaves `driver-turso` - * remote as the sole blocker for BOTH rows, so `$ne` and `$not` now enrol - * together, in #5903's PR. - * - * `driver-turso` remote is the interesting one and the reason the pre-#5298 + * `driver-turso` remote is the one to remember, and the reason the pre-#5298 * version of this note was WRONG where it said "every surface answers this * family the same way — no backend blocker remains". `TursoDriver` extends * `SqlDriver`, so local mode inherited #5146 for free; remote mode compiles * filters in `RemoteTransport.buildWhereSQL`, an independent emitter that * inherited none of it. One driver, two answers, chosen by connection mode — * exactly what this table exists to catch, and exactly what it could not see - * while the fixture had no nullable column. #5903 carries the fix. + * while the fixture had no nullable column. The lesson generalises past this + * family: enrolment is what makes a ruling enforceable, and a "no blocker + * remains" written from a reading of the backend LIST rather than from a + * measurement is the sentence that hides the next one. + * + * The family numbering of the one that remains is kept as its historical id. * * ### 3. `{ field: {} }` — a field constrained by zero operators (#5240) * @@ -316,23 +301,36 @@ export const FILTER_LOGIC_CASES: readonly FilterLogicCase[] = [ note: '#5322: emitting nothing for it runs the query UNSCOPED — on an RLS lowering that is a permission bypass (#5297).', }, - // ── NULL / no-value semantics (#5298) ───────────────────────────────────── + // ── NULL / no-value semantics (#5146, #5298) ────────────────────────────── // - // Rows 3 and 4 have no `d`, and these two cases are what makes that true of - // every harness: a fixture that quietly stored `''` instead of NULL, or a - // `NOT NULL` column that rejected the seed, fails HERE rather than by turning - // some later case green for the wrong reason. That is their first job — they - // are the control the `$ne` / `$not` rows will lean on when those land. + // Rows 3 and 4 have no `d`. The `$null` partition at the bottom of this + // section is what makes that true of every harness: a fixture that quietly + // stored `''` instead of NULL, or a `NOT NULL` column that rejected the seed, + // fails THERE rather than by turning one of the two cases here green for the + // wrong reason. Read the four together — a no-value row is either inside the + // negation or outside it, and the partition says which rows are which. // - // Only the `$null` partition is enrolled. The `$ne` and `$not` rows that - // belong beside it are written out in the module doc above, under "RULED but - // not yet enrolled" — two backends cannot answer them yet, and enrolling a - // row two lanes have to go fix is how this table stops meaning anything. + // The family completed here in #5903, the PR that made `driver-turso` REMOTE + // answer them: it was the last backend of eleven still returning `['2']`, and + // until it did, enrolling these two rows would have been a gate that reports a + // known red — which teaches every agent reading CI to discount the colour. + { + name: '$ne returns the rows with no value', + filter: { d: { $ne: 'v1' } }, + expected: ['2', '3', '4'], + note: '#5298: "not v1" is true of a row whose d is absent. A three-valued `d <> ?` drops rows 3-4 — half the table, silently.', + }, + { + name: '$not returns the rows with no value', + filter: { $not: { d: 'v1' } }, + expected: ['2', '3', '4'], + note: '#5146: the same ruling reached through the combinator. `NOT (NULL = ?)` is UNKNOWN, so an unguarded negation drops rows 3-4 — and on a CEL `!expr` read scope that is one permission rule admitting different row sets per backend.', + }, { name: '$null true selects exactly the no-value rows', filter: { d: { $null: true } }, expected: ['3', '4'], - note: 'The control for the two above: it pins WHICH rows have no value, so a case that returns 3-4 cannot be passing because the seed lost a value it should have kept.', + note: 'The control for the two above: it pins WHICH rows have no value, so a case that returns 2-4 cannot be passing because the seed lost a value it should have kept.', }, { name: '$null false selects exactly the valued rows',