diff --git a/.changeset/undefined-comparand-refusal.md b/.changeset/undefined-comparand-refusal.md new file mode 100644 index 0000000000..15938e5715 --- /dev/null +++ b/.changeset/undefined-comparand-refusal.md @@ -0,0 +1,67 @@ +--- +"@objectstack/driver-sql": patch +"@objectstack/driver-turso": patch +--- + +fix(drivers): refuse an `undefined` filter comparand instead of crashing (SQL) or silently answering `IS NULL` (Turso remote) (#6050) + +**⚠️ 行为变更(升级说明在最后一节)。** 比较数位置上的 `undefined` 从「静默/崩溃」变为 `INVALID_FILTER` / 400 拒收。作者侧的修法是显式判空,或改用 `null` / `$null`。 + +## 实测到的毛病 + +同一个 `TursoDriver`,同一条过滤器,答案取决于它是用哪个 `url` 构造的 —— 四行 fixture(`d` 在 1-2 有值、3-4 为 NULL),`origin/main` @ `cba7454df`: + +| filter | LOCAL(继承 `SqlDriver`) | REMOTE(`RemoteTransport`) | +|---|---|---| +| `{ d: undefined }` | 抛裸 knex `Undefined binding(s)` | `['3','4']` | +| `{ d: { $eq: undefined } }` | 抛裸 knex `Undefined binding(s)` | `['3','4']` | +| `{ $not: { d: undefined } }` | 抛裸 knex `Undefined binding(s)` | `['1','2']` | +| `{ d: { $ne: undefined } }` | `['1','2']` | `['1','2']` | +| `{ $not: { d: { $ne: undefined } } }` | `[]` | `['3','4']` | +| `{ d: { $in: [undefined] } }` | 抛裸 knex `Undefined binding(s)` | `[]` | +| `{ d: { $gt: undefined } }` | 抛裸 knex `Undefined binding(s)` | `[]` | + +两个可分开的毛病: + +**A —— 抛出的那几格没有 ADR-0112 信封。** knex 的 `Undefined binding(s) detected when compiling SELECT` 既没有 `code` 也没有 `status`,`mapDataError` 落默认分支,于是一条「调用方把 filter 写坏了」的错误以不透明 500 的形态到达客户端。#1116 / #4436 为这条通路清点过同类形态,唯独漏了这一格。 + +**B —— 守卫与它自己的发射器分裂。** `$ne` 发射器读 `coerced == null`(宽松,所以 `undefined` 编译成 `IS NOT NULL` —— 一条 TOTAL 谓词),而必须钉住这个发射器的两张极性表 `operatorIsNullTotal` / `nullValueSatisfiesOperator` 读 `=== null`(严格,于是判它「不 total」且「NULL 行满足它」)。`nullGuardForFieldSpec` 因此把一条已经 total 的谓词包成 `d IS NULL OR d IS NOT NULL` —— 恒真 —— 取反后恒假,答 `[]`。这正是 #5298 立的不变量(每张极性表钉的是它自己发射器的拼写)在它自己的定义处被破坏。 + +## 修法 + +一道闸,落在比较数进入**任何**发射器或守卫之前,两个毛病同闸消灭:knex 再也见不到 undefined 绑定,守卫与发射器对 undefined 的分歧变成**不可达**而不是「被修好」。 + +- `driver-sql`:闸落在 `reduceFilterKey` 的校验走查上(与 `$null` / `$exists` 的拒收并排),外加 `applyFilters` 的平铺映射分支 —— `{ d: undefined }` 进不了走查(`typeof undefined` 不是 `'object'`,构不成 `hasMongoOperators`),而它恰恰是这个 bug 最常见的拼写。两处共用一个函数。 +- `driver-turso`:`buildWhereSQL` 入口做一次整棵子树的前置走查。必须前置,否则 `{ $not: { d: undefined } }` 会先把操作数交给 `nullSafeNegationOperand`(一个守卫)。 +- 顺带把两侧的 `== null` / `|| === undefined` 拼写统一收严成 `=== null`(#5347 收紧 `$null` 臂时给的理由:宽松拼写在闸被挪走后会悄悄恢复回答一个没人裁决过的取值)。 + +拒收的位置逐个清点:直接比较数、单值算子的比较数(`$eq`/`$ne`/`$gt`/`$gte`/`$lt`/`$lte` 与 LIKE 族)、列表算子数组的**成员**(`$in`/`$nin`/`$between`)、以及嵌在 `$and`/`$or`/`$not` 里的以上各位。`$null` / `$exists` 的 `undefined` 保持它们**自己**的拒收措辞(比较数是声明的布尔量,那条消息更贴切 —— #5240「一个条件一种措辞」两个方向都适用)。两个驱动的拒收句子逐字一致。 + +## ⛔ `null` 一字未动 + +`{ f: null }`、`{ $eq: null }` → `IS NULL`;`{ $ne: null }` → `IS NOT NULL`;`$null: true/false` 不变;`null` 仍是合法的 `$in` 成员。`null` 是声明过的比较数,拒的只是 JS 里与「没有这个键」不可区分的那个值。 + +## 升级说明 + +如果你的进程内代码这样拼过 filter: + +```ts +// 之前:id 缺失时 —— 本地崩、远端静默匹配全环境行 +await ql.find('deal', { where: { owner_id: ctx.user?.id } }); +``` + +现在会收到 `INVALID_FILTER` / 400,消息里带修法。两种正确写法: + +```ts +// 1) 显式判空 —— 键不存在就是「不约束」 +const where: Record = {}; +if (ctx.user?.id !== undefined) where.owner_id = ctx.user.id; + +// 2) 真的想要空值谓词 —— 写出来 +await ql.find('deal', { where: { owner_id: null } }); // 或 +await ql.find('deal', { where: { owner_id: { $null: true } } } ); +``` + +`where` 整体缺席仍然是「没有过滤器」(`query?.where` 为 `undefined` 是它唯一合法的位置),不受影响。 + +⚠️ 本次只覆盖 `driver-sql` 与 `driver-turso`(含 remote)。`driver-memory` / `driver-mongodb` 是 #5499 的投入冻结面,按裁决只测不改;`@objectstack/formula` 与 `service-analytics` 的 `read-scope-sql.ts` 对同一形状各有一种不同读法,实测记录在 #6125,留待单独裁决。 diff --git a/packages/drivers/driver-sql/src/sql-driver-undefined-comparand-refusal.test.ts b/packages/drivers/driver-sql/src/sql-driver-undefined-comparand-refusal.test.ts new file mode 100644 index 0000000000..3a95c63fec --- /dev/null +++ b/packages/drivers/driver-sql/src/sql-driver-undefined-comparand-refusal.test.ts @@ -0,0 +1,295 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#6050] An `undefined` COMPARAND is refused, in every position — and `null` + * keeps every one of its meanings. + * + * # What was measured + * + * On `origin/main` at `cba7454df`, one `TursoDriver` over the shared conformance + * fixture (`d` valued on rows 1-2, NULL on 3-4) answered the same filter two + * ways, chosen by the `url` it was constructed with: + * + * | filter | LOCAL (this compiler) | REMOTE (`RemoteTransport`) | + * |---|---|---| + * | `{ d: undefined }` | knex `Undefined binding(s)` | `['3','4']` | + * | `{ d: { $eq: undefined } }` | knex `Undefined binding(s)` | `['3','4']` | + * | `{ $not: { d: undefined } }` | knex `Undefined binding(s)` | `['1','2']` | + * | `{ d: { $ne: undefined } }` | `['1','2']` | `['1','2']` | + * | `{ $not: { d: { $ne: undefined } } }` | `[]` | `['3','4']` | + * | `{ d: { $in: [undefined] } }` | knex `Undefined binding(s)` | `[]` | + * | `{ d: { $gt: undefined } }` | knex `Undefined binding(s)` | `[]` | + * + * Two defects, both closed by ONE gate placed before any emitter or guard runs: + * + * **A — no ADR-0112 envelope.** knex's `Undefined binding(s) detected when + * compiling SELECT` carries neither `code` nor `status`, so `mapDataError` fell + * to its default branch and served an opaque 500 for what is a caller mistake in + * a filter. #1116 / #4436 catalogued this exact shape for other inputs; this one + * was missing from the list. + * + * **B — the guard disagreed with its own emitter.** The `$ne` emitter read + * `coerced == null` (LOOSE — `undefined` compiled `IS NOT NULL`, a TOTAL + * predicate) while `operatorIsNullTotal` / `nullValueSatisfiesOperator` read + * `=== null` (STRICT — they judged the same leaf non-total AND satisfied by a + * NULL row). `nullGuardForFieldSpec` therefore wrapped a total predicate in + * `d IS NULL OR d IS NOT NULL` — a tautology whose negation is FALSE, which is + * the `[]` above. #5298's invariant is that a polarity table pins the spelling + * of ITS OWN emitter; this was that invariant broken at its own definition. + * + * # The ruling + * + * REFUSED (`INVALID_FILTER` / 400), adjudicated 2026-08-07 on #6050 — the + * disposition #5347-A gave a non-boolean `$null`. `FieldOperatorsSchema` + * declares no `undefined` comparand; `{ f: undefined }` and `{}` are the same + * object to every JavaScript reader while meaning opposite things (a predicate + * versus no constraint at all); and `undefined` cannot survive a JSON round + * trip, so it is always the fingerprint of an in-process authoring bug — the + * `{ owner_id: ctx.user?.id }` that silently matched every env-wide row. + * + * # Reverse verification — direction predicted before it was run, then measured + * + * Prediction: NOT one uniform direction. The refusal cases must go red, the + * control cases must stay green, and the refusal cases must go red by TWO + * different mechanisms — because the un-fixed driver answered this family two + * different ways (see the matrix above). + * + * Measured, with both `assertDefinedComparands` call sites deleted and nothing + * else changed: **22 failed / 6 passed** of 28. The 6 that stayed green are + * exactly the control cases — the `null` block, the `$null`/`$exists` + * boolean-domain case, the legal-vocabulary case and the `Date` case — which is + * the assertion that this change moved nothing it was not ruled to move. + * + * The two mechanisms, per the `origin/main` measurement above: most positions + * went red by THROWING knex's bare `Undefined binding(s)` — an Error with no + * `code` and no `status`, so `refusalOf` returns normally and only the envelope + * assertions fail. **A test that asserted merely "it throws" would have stayed + * GREEN on the driver this issue was filed against**, which is why every case + * here asserts `code` and `status`. The rest — `$ne`, the LIKE family, and + * `{ $not: { … $ne: undefined } }` — went red by RESOLVING: they never threw at + * all, and `{ $not: { stage: { $ne: undefined } } }` resolved to `[]`, the + * defect-B tautology. + */ + +import { describe, it, expect, beforeEach } from 'vitest'; +import { SqlDriver } from './index.js'; +import type { FilterCondition } from '@objectstack/spec/data'; + +interface WireBearingError extends Error { + code?: string; + status?: number; +} + +describe('[#6050] SqlDriver refuses an undefined comparand', () => { + let driver: SqlDriver; + + beforeEach(async () => { + driver = new SqlDriver({ + client: 'better-sqlite3', + connection: { filename: ':memory:' }, + useNullAsDefault: true, + }); + await driver.initObjects([ + { + name: 'deal', + fields: { + id: { type: 'text', name: 'id' }, + stage: { type: 'text', name: 'stage' }, + score: { type: 'number', name: 'score' }, + }, + } as any, + ]); + await driver.create('deal', { id: '1', stage: 'won', score: 10 }); + await driver.create('deal', { id: '2', stage: null, score: 20 }); + }); + + const ids = async (where: unknown): Promise => { + const rows = await driver.find('deal', { + object: 'deal', + fields: ['id'], + where: where as FilterCondition, + }); + return (rows as any[]).map((r) => String(r.id)).sort(); + }; + + const refusalOf = async (where: unknown): Promise => { + try { + await ids(where); + } catch (e) { + return e as WireBearingError; + } + throw new Error('expected the driver to refuse this filter, but it resolved'); + }; + + /** + * Every position a comparand can occupy, with the path the refusal must name. + * + * "Comparand" is a POSITION, not a type, so the list is enumerated rather than + * swept: the direct comparand, each single-value operator's comparand, and + * each MEMBER of a list operator's array — the same split #5041 made when it + * refused a `$field` reference inside an `$in` list. + */ + const UNDEFINED_POSITIONS: Array<[label: string, where: unknown, path: string]> = [ + // The shape the issue opened on. It is also the one that does NOT reach the + // reduction walk: `typeof undefined` is not `'object'`, so it cannot make + // `applyFilters`' `hasMongoOperators` test true and the plain-map loop + // compiles it instead. Both call sites share one function for this reason. + ['a direct comparand', { stage: undefined }, 'filter.stage'], + ['$eq', { stage: { $eq: undefined } }, 'filter.stage.$eq'], + ['$ne', { stage: { $ne: undefined } }, 'filter.stage.$ne'], + ['$gt', { score: { $gt: undefined } }, 'filter.score.$gt'], + ['$gte', { score: { $gte: undefined } }, 'filter.score.$gte'], + ['$lt', { score: { $lt: undefined } }, 'filter.score.$lt'], + ['$lte', { score: { $lte: undefined } }, 'filter.score.$lte'], + ['$contains', { stage: { $contains: undefined } }, 'filter.stage.$contains'], + ['$notContains', { stage: { $notContains: undefined } }, 'filter.stage.$notContains'], + ['$startsWith', { stage: { $startsWith: undefined } }, 'filter.stage.$startsWith'], + ['$endsWith', { stage: { $endsWith: undefined } }, 'filter.stage.$endsWith'], + // The array IS `$in`'s comparand; each ELEMENT is a comparand in its own + // right, and one bad element is enough. + ['an $in member', { stage: { $in: [undefined] } }, 'filter.stage.$in[0]'], + ['an $in member beside a good one', { stage: { $in: ['won', undefined] } }, 'filter.stage.$in[1]'], + ['a $nin member', { stage: { $nin: [undefined] } }, 'filter.stage.$nin[0]'], + ['a $between bound', { score: { $between: [5, undefined] } }, 'filter.score.$between[1]'], + // Nested positions — the refusal must name where it happened, not just that + // it happened. + ['inside $and', { $and: [{ stage: undefined }] }, 'filter.$and[0].stage'], + ['inside $or', { $or: [{ stage: { $eq: undefined } }] }, 'filter.$or[0].stage.$eq'], + ['inside $not', { $not: { stage: undefined } }, 'filter.$not.stage'], + ['inside $not, one operator down', { $not: { stage: { $ne: undefined } } }, 'filter.$not.stage.$ne'], + ]; + + for (const [label, where, path] of UNDEFINED_POSITIONS) { + it(`refuses ${label} with INVALID_FILTER / 400`, async () => { + const err = await refusalOf(where); + // Defect A: the envelope. Without these two lines the test passes on the + // UNFIXED driver, which threw knex's bare `Undefined binding(s)`. + expect(err.code).toBe('INVALID_FILTER'); + expect(err.status).toBe(400); + expect(err.message).toContain('is undefined'); + expect(err.message).toContain(path); + // The repair the author needs, not just the complaint. + expect(err.message).toContain('Write null if you meant the null predicate'); + expect(err.message).toContain('FieldOperatorsSchema'); + // #3867 — no driver-internal prefix on the wire. + expect(err.message).not.toContain('[sql-driver]'); + // It must never BE knex's message. Checked on the opening rather than by + // absence of the phrase, because this refusal deliberately QUOTES knex's + // wording in its explanatory tail — a `not.toContain` here would be a + // phantom assertion that fails on the fixed driver and passes on nothing. + expect(err.message.startsWith('Comparand at ')).toBe(true); + }); + } + + /** + * The placement proof, in the same shape #5348 used. `{ stage: 'won' }` is a + * satisfiable disjunct and `{}` is the TRUE identity — an emitter-side gate + * would resolve the enclosing node from those siblings and never look at the + * offending one, making the refusal conditional on evaluation order. + */ + it('refuses an undefined comparand beside a satisfiable disjunct', async () => { + const err = await refusalOf({ $or: [{ stage: 'won' }, { stage: undefined }] }); + expect(err.code).toBe('INVALID_FILTER'); + expect(err.message).toContain('filter.$or[1].stage'); + }); + + it('refuses an undefined comparand beside the TRUE identity `{}`', async () => { + const err = await refusalOf({ $or: [{}, { stage: { $eq: undefined } }] }); + expect(err.code).toBe('INVALID_FILTER'); + expect(err.message).toContain('filter.$or[1].stage.$eq'); + }); + + /** + * Defect B, pinned as a REFUSAL rather than as an answer. + * + * This is the row that answered `[]` locally and `['3','4']` remotely, and the + * one whose cause was internal to this file: guard `=== null`, emitter + * `== null`, one leaf, two readings. There is no "correct" row set to assert + * for it any more — the ruling removed the question — so what is pinned is + * that it is refused, and that the refusal names the `$ne` comparand rather + * than some downstream consequence of the tautology. + */ + it('refuses the guard/emitter split case instead of answering it', async () => { + const err = await refusalOf({ $not: { stage: { $ne: undefined } } }); + expect(err.code).toBe('INVALID_FILTER'); + expect(err.status).toBe(400); + expect(err.message).toContain('filter.$not.stage.$ne'); + }); + + /** + * ⛔ The control block: `null` did not move, in any position. + * + * A gate written by pattern-matching on "empty-ish comparand" would take + * `null` with it and silently delete the null predicate from the protocol. + * Every line here answered exactly this before the change. + */ + describe('null comparands are unchanged, line by line', () => { + it('the null predicate spellings still compile', async () => { + expect(await ids({ stage: null })).toEqual(['2']); + expect(await ids({ stage: { $eq: null } })).toEqual(['2']); + expect(await ids({ stage: { $ne: null } })).toEqual(['1']); + expect(await ids({ stage: { $null: true } })).toEqual(['2']); + expect(await ids({ stage: { $null: false } })).toEqual(['1']); + expect(await ids({ stage: { $exists: true } })).toEqual(['1']); + expect(await ids({ stage: { $exists: false } })).toEqual(['2']); + }); + + it('null keeps its meaning under $not and inside combinators', async () => { + expect(await ids({ $not: { stage: null } })).toEqual(['1']); + expect(await ids({ $not: { stage: { $ne: null } } })).toEqual(['2']); + expect(await ids({ $and: [{ stage: null }] })).toEqual(['2']); + expect(await ids({ $or: [{ stage: null }, { stage: 'won' }] })).toEqual(['1', '2']); + }); + + it('null is still a legitimate $in / $nin member', async () => { + // `IN (NULL)` is UNKNOWN for every row in SQL — that is SQL's answer, not + // this gate's, and it is untouched here. + expect(await ids({ stage: { $in: ['won', null] } })).toEqual(['1']); + }); + }); + + /** + * `$null` / `$exists` keep their OWN refusal for `undefined`. + * + * Their comparand is a declared BOOLEAN — a flag, not a value to compare + * against — and `nonBooleanNullComparandError` / `nonBooleanExistsComparandError` + * already name that declared domain, which is the more useful message for + * that mistake. #5240's rule is one condition, one wording; re-answering these + * two here would have given the same mistake two. + */ + it('$null / $exists undefined keeps the boolean-domain wording', async () => { + for (const where of [{ stage: { $null: undefined } }, { stage: { $exists: undefined } }]) { + const err = await refusalOf(where); + expect(err.code).toBe('INVALID_FILTER'); + expect(err.message).toContain('requires a boolean comparand (true or false)'); + expect(err.message).not.toContain('Write null if you meant the null predicate'); + } + }); + + /** + * The ordinary vocabulary is untouched. A gate on the reduction walk runs for + * every filter in the process, so "did it change an answer it should not have" + * is a question this file has to ask out loud. + */ + it('legal filters compile exactly as before', async () => { + expect(await ids({ stage: 'won' })).toEqual(['1']); + expect(await ids({ stage: { $in: ['won'] } })).toEqual(['1']); + expect(await ids({ stage: { $ne: 'won' } })).toEqual(['2']); + expect(await ids({ score: { $between: [5, 15] } })).toEqual(['1']); + expect(await ids({ score: { $gte: 10 } })).toEqual(['1', '2']); + expect(await ids({ $not: { stage: 'won' } })).toEqual(['2']); + expect(await ids({})).toEqual(['1', '2']); + expect(await ids({ $and: [] })).toEqual(['1', '2']); + expect(await ids({ $or: [] })).toEqual([]); + }); + + /** + * A Date comparand is an OBJECT that is a VALUE. The walk must not read its + * (empty) entry list as an operator map and must not refuse it — the #1066 + * routing seam, asserted from the gate's side. + */ + it('a Date comparand is not mistaken for an operator map', async () => { + // No row matches; the point is that it COMPILES rather than being refused. + expect(await ids({ stage: { $ne: new Date('2020-01-01T00:00:00.000Z') } })).toEqual(['1', '2']); + }); +}); diff --git a/packages/drivers/driver-sql/src/sql-driver.ts b/packages/drivers/driver-sql/src/sql-driver.ts index 986757eaeb..9b493fd8f9 100644 --- a/packages/drivers/driver-sql/src/sql-driver.ts +++ b/packages/drivers/driver-sql/src/sql-driver.ts @@ -887,6 +887,117 @@ function nonBooleanNullComparandError(field: string, value: unknown, path: strin ); } +/** + * [#6050] `undefined` in a COMPARAND position. + * + * Ruled REFUSED on 2026-08-07 (ruling B on #6050), the same disposition #5347-A + * gave a non-boolean `$null`, and for a sharper version of the same reason: + * there is no reading of `undefined` here that is not a guess, and the two + * candidate readings are each other's opposite. + * + * `undefined` cannot survive a JSON round trip, so it only ever arrives from + * IN-PROCESS code — which is exactly what makes it dangerous rather than + * academic. `{ owner_id: ctx.user?.id }` with a missing id is the shape that + * produced this issue: measured on `origin/main` (`cba7454df`, four rows, `d` + * valued on 1-2 and NULL on 3-4), the SAME `TursoDriver` answered it two ways, + * chosen by the `url` it was constructed with: + * + * | filter | LOCAL (this compiler) | REMOTE (`RemoteTransport`) | + * |---|---|---| + * | `{ d: undefined }` | bare knex `Undefined binding(s)` | `['3','4']` | + * | `{ d: { $eq: undefined } }` | bare knex `Undefined binding(s)` | `['3','4']` | + * | `{ $not: { d: undefined } }` | bare knex `Undefined binding(s)` | `['1','2']` | + * | `{ d: { $ne: undefined } }` | `['1','2']` | `['1','2']` | + * | `{ $not: { d: { $ne: undefined } } }` | `[]` | `['3','4']` | + * | `{ d: { $in: [undefined] } }` | bare knex `Undefined binding(s)` | `[]` | + * | `{ d: { $gt: undefined } }` | bare knex `Undefined binding(s)` | `[]` | + * + * Two defects in one gate: + * + * 1. The thrown rows carried NO ADR-0112 envelope — knex's `Undefined + * binding(s) detected when compiling SELECT` has neither `code` nor + * `status`, so `mapDataError` served an opaque 500 for what is a caller + * mistake in a filter (#1116 / #4436 catalogued this exact shape for other + * inputs; this one was missing from the list). + * 2. The `$not: { $ne: undefined }` row is this driver contradicting ITSELF: + * the `$ne` emitter reads `coerced == null` (LOOSE — so `undefined` compiled + * `IS NOT NULL`, a TOTAL predicate), while {@link operatorIsNullTotal} and + * {@link nullValueSatisfiesOperator} read `value === null` (STRICT — so they + * judged the same leaf non-total AND satisfied by a NULL row). The guard + * then wrapped a total predicate in `d IS NULL OR d IS NOT NULL`, a + * tautology, whose negation is FALSE — the `[]` above. That is the #5298 + * invariant broken at its own definition: a polarity table pins the spelling + * of ITS OWN emitter. + * + * Refusing the comparand kills both at once and does it BEFORE either can act: + * knex never sees an undefined binding, and guard-vs-emitter disagreement about + * `undefined` becomes unreachable rather than merely repaired. + * + * ⛔ What deliberately does NOT move: `null`. `{ f: null }`, `{ $eq: null }`, + * `{ $ne: null }` and `$null` keep their exact behaviour — `null` IS a declared + * comparand and IS the null predicate. The refusal is about the JS value that + * the language cannot distinguish from an ABSENT key: `{ f: undefined }` and + * `{}` are the same object to every reader, and they mean opposite things (a + * predicate vs no constraint at all). + */ +function undefinedComparandError(field: string, path: string): Error { + return unsupportedFilterError( + `Comparand at ${path} is undefined. @objectstack/spec FieldOperatorsSchema declares no ` + + `undefined comparand, and in JavaScript { "${field}": undefined } cannot be told apart from ` + + `omitting the key — yet the two mean OPPOSITE things (a predicate versus no constraint at ` + + `all), so there is no reading of it that is not a guess. Write null if you meant the null ` + + `predicate ({ "${field}": null } or { "${field}": { "$null": true } }), or omit the key when ` + + `the value is genuinely absent (e.g. \`if (id !== undefined) where.${field} = id\`). It is ` + + `refused rather than compiled because the backends disagreed: driver-sql handed it to knex ` + + `and got a bare "Undefined binding(s)" Error carrying no code, while Turso's remote transport ` + + `compiled it to IS NULL — so \`{ owner_id: ctx.user?.id }\` with a missing id silently ` + + `matched every env-wide row instead of failing (#6050).`, + ); +} + +/** + * [#6050] Refuse every `undefined` sitting in a comparand position of ONE field + * constraint. + * + * The positions are enumerated rather than swept, because "comparand" is a + * position and not a type: + * + * - the DIRECT comparand — `{ d: undefined }`, the implicit `=`; + * - an OPERATOR's comparand — `{ d: { $eq: undefined } }`, `$ne`, `$gt`, the + * LIKE family, and every other single-value operator; + * - a MEMBER of a list operator's array — `{ d: { $in: [undefined] } }`, + * `$nin`, `$between`. The array itself IS `$in`'s legitimate comparand; each + * element is a comparand in its own right, which is the same split + * {@link assertCompilableComparand} makes for `$field`. + * + * Two positions are deliberately NOT swept: + * + * - `$null` / `$exists`. Their comparand is a declared BOOLEAN — a flag, not a + * value to compare against — and `undefined` there is already refused by + * {@link nonBooleanNullComparandError} / {@link nonBooleanExistsComparandError} + * with a message that names the declared domain. Re-answering it here would + * swap a better message for a worse one and give one condition two wordings, + * which is what #5240 ruled against. + * - a bare ARRAY in DIRECT comparand position (`{ d: [1, undefined] }`). An + * array is not a comparand outside a list operator, and + * {@link assertCompilableComparand} already refuses it as a whole for that + * reason; inspecting its members here would relabel a shape that is refused + * either way. + */ +function assertDefinedComparands(field: string, spec: unknown, path: string): void { + if (spec === undefined) throw undefinedComparandError(field, path); + if (!isFilterNode(spec)) return; + for (const [op, opValue] of Object.entries(spec)) { + if (op === '$null' || op === '$exists') continue; + const opPath = `${path}.${op}`; + if (opValue === undefined) throw undefinedComparandError(field, opPath); + if (!Array.isArray(opValue)) continue; + opValue.forEach((member, index) => { + if (member === undefined) throw undefinedComparandError(field, `${opPath}[${index}]`); + }); + } +} + /** [#5134] `$and`/`$or` take a list; anything else is refused, never coerced. */ function assertFilterNodeList(value: unknown, key: string, path: string): asserts value is unknown[] { if (Array.isArray(value)) return; @@ -990,6 +1101,22 @@ function reduceFilterKey(key: string, value: unknown, path: string): FilterVerdi // before compiles byte-identically now. if (isEmptyFieldConstraint(value)) throw emptyFieldConstraintError(key, here); + // [#6050] `undefined` in a comparand position, refused on THIS walk for the + // two reasons the walk exists — it is exhaustive and it runs FIRST. + // + // "First" is the whole design here, not a preference. Both defects #6050 + // measured live downstream of this line: the emitter hands an undefined bind + // to knex (a bare `Undefined binding(s)` Error, outside ADR-0112), and the + // `$not` branch's {@link nullSafeNegationOperand} rewrite consults + // {@link operatorIsNullTotal} / {@link nullValueSatisfiesOperator}, whose + // `=== null` spelling disagrees with the `$ne` emitter's `== null` about + // exactly this value. `applyFilterCondition` runs the whole reduction before + // it reaches either, so a refusal raised here makes BOTH unreachable rather + // than repaired — and the polarity tables never have to answer a question + // nobody ruled on. See {@link undefinedComparandError} for the measured + // local/remote matrix and why `null` is untouched. + assertDefinedComparands(key, value, here); + // [#5347] `$null`'s comparand is a boolean by declaration. Checked on this // walk rather than in the emitter's `$null` arm for the same // evaluation-order reason, and checked on the RAW value so the message names @@ -1054,6 +1181,15 @@ type NullGuard = 'none' | 'requireValue' | 'allowNull'; function nullValueSatisfiesOperator(op: string, value: unknown): boolean { switch (op) { // `$eq: null` IS the null predicate; any other comparand is a value test. + // + // [#6050] These two arms are STRICT (`=== null`) while the `$ne` emitter + // used to be LOOSE (`== null`) — the #5298 invariant broken at its own + // definition, since a polarity table pins the spelling of its own emitter. + // The repair is the gate, not a third spelling: `reduceFilterKey` refuses + // an `undefined` comparand before this table is consulted, so `null` and + // real values are the only comparands left and the emitter now reads + // `=== null` too. Both spellings are exhaustive over the surviving domain, + // and they are the SAME spelling — which is what the invariant asks for. case '$eq': return value === null; case '$ne': return value !== null; // [#5347] `$null` is now TOTAL over its declared domain: `reduceFilterKey` @@ -1110,6 +1246,10 @@ function operatorIsNullTotal(op: string, value: unknown): boolean { return true; // A null comparand makes these null PREDICATES too (see the `$eq`/`$ne` // arms of the emitter below), not comparisons. + // + // [#6050] Same note as {@link nullValueSatisfiesOperator}'s `$eq`/`$ne` + // arms: this `=== null` and the emitter's test now read the same value set, + // because `undefined` is refused before either runs. case '$eq': case '$ne': return value === null; @@ -6326,6 +6466,16 @@ export class SqlDriver implements IDataDriver { // with #5041's generic "cannot be bound as a SQL parameter", which // describes a comparand and not a constraint with no operator at all. if (isEmptyFieldConstraint(value)) throw emptyFieldConstraintError(key, `filter.${key}`); + // #6050 — the same position as the `undefined` gate on the reduction + // walk, reached the same way `{ field: {} }` above reaches its own: this + // loop runs when NO key of the filter carries an operator, so the walk + // never sees the node. `{ d: undefined }` IS that shape — `typeof + // undefined` is not `'object'`, so it cannot make `hasMongoOperators` + // true — and it is the single most likely spelling of the defect + // (`{ owner_id: ctx.user?.id }`). ONE function answers both call sites + // so the two positions cannot drift into two verdicts, which is the + // #5240 lesson this driver already paid for once. + assertDefinedComparands(key, value, `filter.${key}`); // #5041 — the plain `{ field: value }` map compiles to an implicit `=`, // so it is a comparison emitter too and gets the same gate. assertCompilableComparand(column, '=', value); @@ -6623,7 +6773,22 @@ export class SqlDriver implements IDataDriver { // UNCHANGED by #5298: `IS NOT NULL` is already total, and both // sides of the ruling agree a row with no value does NOT have // "any value". Only the value COMPARISON below becomes NULL-safe. - if (coerced == null) (builder as any)[logicalOp === 'or' ? 'orWhereNotNull' : 'whereNotNull'](field); + // + // [#6050] The test was `coerced == null` — LOOSE, so it also + // caught `undefined`, while the two polarity tables that must + // pin THIS emitter's spelling (`operatorIsNullTotal`, + // `nullValueSatisfiesOperator`) read `=== null`. That split was + // defect B: `{ $not: { d: { $ne: undefined } } }` compiled a + // guarded tautology and answered `[]` where remote answered + // `['3','4']`. Now that `undefined` is refused upstream the two + // spellings cover the same domain, and the strict one is written + // for the reason #5347 gave when it tightened the `$null` arm: + // a lenient test keeps compiling if the gate is ever moved, and + // silently resumes answering for a value nobody ruled on. Note + // `coerceFilterValue` is total (every arm returns its input on a + // shape it cannot canonicalise), so it cannot manufacture an + // `undefined` the gate never saw. + if (coerced === null) (builder as any)[logicalOp === 'or' ? 'orWhereNotNull' : 'whereNotNull'](field); else this.applyNullSafeNegative(builder, method, field, (qb) => qb.orWhere(field, '<>', coerced)); break; case '$gt': diff --git a/packages/drivers/driver-turso/src/remote-transport-undefined-comparand-refusal.test.ts b/packages/drivers/driver-turso/src/remote-transport-undefined-comparand-refusal.test.ts new file mode 100644 index 0000000000..f5fa901d5a --- /dev/null +++ b/packages/drivers/driver-turso/src/remote-transport-undefined-comparand-refusal.test.ts @@ -0,0 +1,268 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#6050] `undefined` in a comparand position is REFUSED — and this transport's + * half of the defect was the silent one. + * + * # What was measured + * + * On `origin/main` at `cba7454df`, one `TursoDriver` over the shared conformance + * fixture (`d` valued on rows 1-2, NULL on 3-4). The `url` it was constructed + * with chose the answer: + * + * | filter | LOCAL (`SqlDriver`) | REMOTE (here) | + * |---|---|---| + * | `{ d: undefined }` | knex `Undefined binding(s)` | `['3','4']` | + * | `{ d: { $eq: undefined } }` | knex `Undefined binding(s)` | `['3','4']` | + * | `{ $not: { d: undefined } }` | knex `Undefined binding(s)` | `['1','2']` | + * | `{ $not: { d: { $ne: undefined } } }` | `[]` | `['3','4']` | + * | `{ d: { $in: [undefined] } }` | knex `Undefined binding(s)` | `[]` | + * | `{ d: { $gt: undefined } }` | knex `Undefined binding(s)` | `[]` | + * + * A crash is loud and gets fixed. `['3','4']` is not: this transport compiled + * `{ d: undefined }` to `"d" IS NULL` and answered. `undefined` cannot survive a + * JSON round trip, so it only ever arrives from in-process code — which is + * exactly where `{ owner_id: ctx.user?.id }` lives. With the id missing, a + * per-owner read scope became `owner_id IS NULL` and returned every env-wide + * row: not a degraded filter, an over-read. + * + * The `$gt` / `$in` rows are the same cause wearing the other polarity, through + * `serializeComparand`'s `undefined → null` bind: `col > NULL` is UNKNOWN for + * every row, so the filter answered an empty page and reported nothing. + * + * # The ruling + * + * REFUSED (`INVALID_FILTER` / 400), adjudicated 2026-08-07 on #6050 — the + * disposition #5347-A gave a non-boolean `$null`, applied to the one value + * JavaScript cannot tell apart from an absent key. `{ f: undefined }` and `{}` + * are the same object to every reader while meaning opposite things, so any + * compilation of it is a guess about intent. + * + * # Reverse verification — direction predicted before it was run, then measured + * + * Prediction: unlike `driver-sql`'s twin of this file — where the un-fixed + * driver threw on most positions and answered on the rest — every refusal case + * here should go red by RESOLVING, because this transport never threw on any of + * them. It compiled all of them into valid SQL and returned rows. + * + * Measured, with the `assertDefinedComparands` call deleted from + * `buildWhereSQL` and nothing else changed: **20 failed / 9 passed** of 29, and + * every one of the 20 failed inside `refusalOf`'s "expected … to be refused, but + * it compiled to …" branch — i.e. by answering, exactly as predicted. What each + * one answers is the matrix above. + * + * The 9 that stayed green are the controls, and they are the other direction: + * the `null` block, the node-position refusals, the absent-`where` case, the + * `$null`/`$exists` boolean-domain case and the ordinary vocabulary. They pin + * that this change moved nothing it was not ruled to move. + */ + +import { describe, it, expect, vi } from 'vitest'; +import { RemoteTransport } from './remote-transport.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 }; +} + +async function refusalOf(where: unknown): Promise { + const { t, calls } = transportWithCapturingClient(); + try { + await t.find('deal', { where } as unknown as QueryAST); + } catch (e) { + // A refused filter must not have reached the database on its way to + // throwing — a statement that ran is a statement whose rows were read. + expect(calls).toEqual([]); + return e as WireBearingError; + } + throw new Error( + `expected ${JSON.stringify(where)} to be refused, but it compiled to ${JSON.stringify(calls[0])}`, + ); +} + +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]; +} + +/** + * `driver-sql`'s leading sentence, pinned as a LITERAL rather than imported. + * + * #5240's rule is that one condition speaks one wording wherever the author + * reached it, and a caller who hits this on a Turso remote url must read the + * same sentence they would read on Postgres. A literal is what makes the two + * copies fail apart when one is edited alone — an import would make them agree + * by construction and prove nothing. + */ +const FRAMEWORK_LEADING_SENTENCE = + '@objectstack/spec FieldOperatorsSchema declares no undefined comparand'; + +const BARE_SCAN = 'SELECT * FROM "deal"'; + +describe('[#6050] RemoteTransport refuses an undefined comparand', () => { + /** Every position a comparand can occupy, with the path the refusal names. */ + const UNDEFINED_POSITIONS: Array<[label: string, where: unknown, path: string]> = [ + ['a direct comparand', { stage: undefined }, 'where.stage'], + ['$eq', { stage: { $eq: undefined } }, 'where.stage.$eq'], + ['$ne', { stage: { $ne: undefined } }, 'where.stage.$ne'], + ['$gt', { score: { $gt: undefined } }, 'where.score.$gt'], + ['$gte', { score: { $gte: undefined } }, 'where.score.$gte'], + ['$lt', { score: { $lt: undefined } }, 'where.score.$lt'], + ['$lte', { score: { $lte: undefined } }, 'where.score.$lte'], + ['$contains', { stage: { $contains: undefined } }, 'where.stage.$contains'], + ['$notContains', { stage: { $notContains: undefined } }, 'where.stage.$notContains'], + ['$startsWith', { stage: { $startsWith: undefined } }, 'where.stage.$startsWith'], + ['$endsWith', { stage: { $endsWith: undefined } }, 'where.stage.$endsWith'], + ['an $in member', { stage: { $in: [undefined] } }, 'where.stage.$in[0]'], + ['an $in member beside a good one', { stage: { $in: ['won', undefined] } }, 'where.stage.$in[1]'], + ['a $nin member', { stage: { $nin: [undefined] } }, 'where.stage.$nin[0]'], + ['inside $and', { $and: [{ stage: undefined }] }, 'where.$and[0].stage'], + ['inside $or', { $or: [{ stage: { $eq: undefined } }] }, 'where.$or[0].stage.$eq'], + ['inside $not', { $not: { stage: undefined } }, 'where.$not.stage'], + ['inside $not, one operator down', { $not: { stage: { $ne: undefined } } }, 'where.$not.stage.$ne'], + ]; + + for (const [label, where, path] of UNDEFINED_POSITIONS) { + it(`refuses ${label} with INVALID_FILTER / 400`, async () => { + const err = await refusalOf(where); + expect(err.code).toBe('INVALID_FILTER'); + expect(err.status).toBe(400); + expect(err.message).toContain('is undefined'); + expect(err.message).toContain(path); + expect(err.message).toContain(FRAMEWORK_LEADING_SENTENCE); + expect(err.message).toContain('Write null if you meant the null predicate'); + // This transport's messages keep their prefix (#1116 left the prefix pass + // to #1077); the SENTENCE is what must match framework's. + expect(err.message).toContain('[RemoteTransport]'); + expect(err.message).toContain(`on 'deal'`); + }); + } + + /** + * The pre-walk placement, proved from the outside. + * + * `{ $not: { d: undefined } }` is the case that decides where the gate goes: + * compiling key by key would hand the operand to `nullSafeNegationOperand` — + * a GUARD reading the polarity tables — with the `undefined` still in it, and + * #6050 is precisely about guard and emitter disagreeing over this value. The + * walk runs over the whole subtree first, so the guard never sees it. + */ + it('refuses inside $not before the NULL-safe rewrite reads the operand', async () => { + const err = await refusalOf({ $not: { stage: { $ne: undefined } } }); + expect(err.code).toBe('INVALID_FILTER'); + expect(err.message).toContain('where.$not.stage.$ne'); + }); + + it('refuses beside a satisfiable disjunct, and beside the TRUE identity', async () => { + // An emitter-side gate would resolve the `$or` from the good branch and + // never look at the offending one — a refusal conditional on siblings. + const withSibling = await refusalOf({ $or: [{ stage: 'won' }, { stage: undefined }] }); + expect(withSibling.message).toContain('where.$or[1].stage'); + const withIdentity = await refusalOf({ $or: [{}, { stage: { $eq: undefined } }] }); + expect(withIdentity.message).toContain('where.$or[1].stage.$eq'); + }); + + /** + * ⛔ `null` is untouched, clause for clause. + * + * These are SQL-text assertions rather than row counts on purpose: the risk a + * gate like this carries is deleting the null predicate along with the + * undefined one, and the compiled clause is where that would show. + */ + describe('null comparands compile byte-identically to before', () => { + it('`{ field: null }` is still IS NULL', async () => { + const call = await compile({ stage: null }); + expect(call.sql).toBe(`${BARE_SCAN} WHERE "stage" IS NULL`); + expect(call.args).toEqual([]); + }); + + it('`$eq: null` is still IS NULL and `$ne: null` still IS NOT NULL', async () => { + expect((await compile({ stage: { $eq: null } })).sql).toBe(`${BARE_SCAN} WHERE "stage" IS NULL`); + expect((await compile({ stage: { $ne: null } })).sql).toBe(`${BARE_SCAN} WHERE "stage" IS NOT NULL`); + }); + + it('`$null` keeps both booleans', async () => { + expect((await compile({ stage: { $null: true } })).sql).toBe(`${BARE_SCAN} WHERE "stage" IS NULL`); + expect((await compile({ stage: { $null: false } })).sql).toBe(`${BARE_SCAN} WHERE "stage" IS NOT NULL`); + }); + + it('a null $in member is still bound, not refused', async () => { + const call = await compile({ stage: { $in: ['won', null] } }); + expect(call.sql).toContain('IN (?, ?)'); + expect(call.args).toEqual(['won', null]); + }); + + it('`$not` of a null predicate is unchanged', async () => { + const call = await compile({ $not: { stage: null } }); + expect(call.sql).toBe(`${BARE_SCAN} WHERE NOT ("stage" IS NULL)`); + }); + }); + + /** + * `$null` / `$exists` keep their OWN refusal for `undefined` — their comparand + * is a declared BOOLEAN, and the boolean-domain message is the better one for + * that mistake. One condition, one wording (#5240) cuts both ways. + */ + it('$null / $exists undefined keeps the boolean-domain wording', async () => { + for (const where of [{ stage: { $null: undefined } }, { stage: { $exists: undefined } }]) { + const err = await refusalOf(where); + expect(err.code).toBe('INVALID_FILTER'); + expect(err.message).toContain('requires a boolean comparand (true or false)'); + expect(err.message).not.toContain('Write null if you meant the null predicate'); + } + }); + + /** + * A node-position `undefined` is NOT a comparand and must keep its own + * refusal, which names the SHAPE the caller sent rather than a comparand + * position that does not exist there. + */ + it('leaves the node-position refusals alone', async () => { + const notOperand = await refusalOf({ $not: undefined }); + expect(notOperand.code).toBe('INVALID_FILTER'); + expect(notOperand.message).toContain('$not'); + expect(notOperand.message).toContain('undefined'); + expect(notOperand.message).not.toContain('Write null if you meant the null predicate'); + + const andElement = await refusalOf({ $and: [undefined] }); + expect(andElement.code).toBe('INVALID_FILTER'); + expect(andElement.message).not.toContain('Write null if you meant the null predicate'); + }); + + it('an absent `where` is still "no filter", not a refusal', async () => { + // The ONE `undefined` that is legal, and it is not in a comparand position: + // all five call sites hand `buildWhereSQL` a `query?.where`, so "no filter" + // arrives as `undefined` by design. + const { t, calls } = transportWithCapturingClient(); + await t.find('deal', {} as unknown as QueryAST); + expect(calls[0].sql).toBe(BARE_SCAN); + await t.find('deal', { where: undefined } as unknown as QueryAST); + expect(calls[1].sql).toBe(BARE_SCAN); + await t.find('deal', { where: null } as unknown as QueryAST); + expect(calls[2].sql).toBe(BARE_SCAN); + }); + + it('the ordinary vocabulary compiles exactly as before', async () => { + expect((await compile({ stage: 'won' })).args).toEqual(['won']); + expect((await compile({ stage: { $in: ['won', 'lost'] } })).args).toEqual(['won', 'lost']); + expect((await compile({ score: { $gt: 5 } })).args).toEqual([5]); + expect((await compile({ stage: { $contains: 'w' } })).args).toEqual(['%w%']); + expect((await compile({})).sql).toBe(BARE_SCAN); + }); +}); diff --git a/packages/drivers/driver-turso/src/remote-transport.ts b/packages/drivers/driver-turso/src/remote-transport.ts index 12da1bba74..e7cb8c3218 100644 --- a/packages/drivers/driver-turso/src/remote-transport.ts +++ b/packages/drivers/driver-turso/src/remote-transport.ts @@ -250,11 +250,21 @@ 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; + // + // [#6050] The `|| value === undefined` half is GONE from both arms. It was + // correct while this transport COMPILED an undefined comparand to the null + // predicate — the table pinned its own emitter, which is the #5298 + // invariant — and it is wrong now that the comparand is refused before + // either runs: the emitter arms below dropped their `undefined` half in the + // same edit, so guard and emitter still read the identical value set. This + // is deliberately not "harmless extra tolerance": a spelling that keeps + // answering for a value nobody ruled on is exactly what #5347 tightened out + // of `driver-sql`'s `$null` arm, and it is what let this family diverge. + case '$eq': return value === null; // 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); + case '$ne': return value !== null; // 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 @@ -283,11 +293,13 @@ function operatorIsNullTotal(op: string, value: unknown): boolean { 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. + // A null comparand makes these null PREDICATES too, not comparisons — see + // the `$eq` / `$ne` arms of the emitter. [#6050] `undefined` dropped here + // for the reason given on {@link nullValueSatisfiesOperator}'s twin arms: + // it is refused upstream, so the two tables and the emitter read one set. case '$eq': case '$ne': - return value === null || value === undefined; + return value === null; default: return false; } @@ -299,11 +311,16 @@ function operatorIsNullTotal(op: string, value: unknown): boolean { * 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'; + // `{ field: null }` compiles to `IS NULL` — already total. + // + // [#6050] `undefined` no longer shares this arm. The old comment's reasoning + // ("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") was right about the invariant and has simply run out of subject: + // the field arm no longer reads it as anything, because + // {@link RemoteTransport.assertDefinedComparands} refuses it before this + // classification runs. + if (spec === null) 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 @@ -1343,6 +1360,22 @@ export class RemoteTransport { return { whereClauses: '', args: [] }; } + // [#6050] Refuse every `undefined` comparand in the WHOLE subtree, before a + // single clause is emitted and before the `$not` branch below rewrites its + // operand through the polarity tables. + // + // The pre-walk is what makes the gate hold for `{ $not: { d: undefined } }`. + // Compiling key-by-key would reach `nullSafeNegationOperand` — a GUARD — + // with the undefined still in it, and #6050's whole point is that guard and + // emitter must never get to disagree about this value. Walking first makes + // the disagreement unreachable rather than merely repaired, and it is the + // same discipline `driver-sql` applies with `reduceFilterNode`. + // + // Idempotent by construction: `$and`/`$or`/`$not` re-enter this method + // through `buildSubFilterSQL`, so a nested node is walked more than once and + // answers the same both times. Cheap, and cheaper than a second gate. + this.assertDefinedComparands(object, filters, path); + const clauses: string[] = []; const args: any[] = []; @@ -1520,7 +1553,12 @@ export class RemoteTransport { for (const [op, opValue] of Object.entries(value as Record)) { switch (op) { case '$eq': - if (opValue === null || opValue === undefined) { + // [#6050] `=== null` only. `undefined` used to share this arm and + // compile to `IS NULL`, which is the silent half of the + // local/remote fork this issue closes; it is refused by + // {@link RemoteTransport.assertDefinedComparands} before the loop + // starts, so the arm now spells exactly the comparand it serves. + if (opValue === null) { clauses.push(`${column} IS NULL`); } else { const bind = this.serializeComparand(object, key, op, opValue); @@ -1529,7 +1567,11 @@ export class RemoteTransport { } break; case '$ne': - if (opValue === null || opValue === undefined) { + // [#6050] `=== null` only, the mirror of the `$eq` arm above and + // the exact spelling `driver-sql`'s `$ne` emitter now carries — + // the two compilers of one driver agree on the value set, which + // is what #5298's invariant asks of a deliberate copy. + if (opValue === null) { // [#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 @@ -1695,7 +1737,13 @@ export class RemoteTransport { if (clauses.length === clausesBefore) { throw this.emptyFieldFilter(object, key, value); } - } else if (value === null || value === undefined) { + } else if (value === null) { + // [#6050] `=== null` only — `{ field: undefined }` is refused by + // {@link RemoteTransport.assertDefinedComparands} before this loop, and + // it was THE shape the issue opened on: `{ owner_id: ctx.user?.id }` + // with a missing id landed here and compiled `owner_id IS NULL`, + // matching every env-wide row. `null` keeps this arm untouched. + // // Null equality MUST use `IS NULL` — `col = NULL` is always UNKNOWN // in SQL, so it matches zero rows. Env-wide metadata (and drafts) are // stored with `organization_id IS NULL`; emitting `= ?` here is what @@ -2036,6 +2084,115 @@ export class RemoteTransport { * name the disjunct it came from (`where.$or[1].$where`) rather than only the * key. Nothing else reads it — see {@link buildWhereSQL}. */ + /** + * [#6050] Refuse every `undefined` sitting in a COMPARAND position anywhere in + * one filter subtree. + * + * Ruled REFUSED on 2026-08-07 (ruling B on #6050) — the disposition #5347-A + * gave a non-boolean `$null`, applied to the one value JavaScript cannot tell + * apart from an absent key. Measured on `origin/main` (`cba7454df`) against + * the shared conformance fixture, the SAME `TursoDriver` answered these two + * ways depending only on the `url` it was constructed with: + * + * | filter | LOCAL (`SqlDriver`) | REMOTE (here) | + * |---|---|---| + * | `{ d: undefined }` | bare knex `Undefined binding(s)` | `['3','4']` | + * | `{ d: { $eq: undefined } }` | bare knex `Undefined binding(s)` | `['3','4']` | + * | `{ $not: { d: undefined } }` | bare knex `Undefined binding(s)` | `['1','2']` | + * | `{ $not: { d: { $ne: undefined } } }` | `[]` | `['3','4']` | + * | `{ d: { $in: [undefined] } }` | bare knex `Undefined binding(s)` | `[]` | + * + * This transport's half was the SILENT half, and it is the dangerous one: + * `undefined` cannot survive a JSON round trip, so it only ever arrives from + * in-process code — `{ owner_id: ctx.user?.id }` with a missing id compiled to + * `owner_id IS NULL` and returned every env-wide row, which is a read the + * caller's own filter was written to prevent. + * + * ⛔ `null` is untouched, in every position: `{ f: null }` / `{ $eq: null }` + * stay `IS NULL`, `{ $ne: null }` stays `IS NOT NULL`, `$null` is unchanged. + * `null` IS a declared comparand. `undefined` is not, and `{ f: undefined }` + * versus `{}` — a predicate versus no constraint at all — is a distinction the + * language does not preserve, so any compilation of it is a guess. + * + * The positions, enumerated rather than swept (comparand is a POSITION, not a + * type): the direct comparand, an operator's comparand, and each MEMBER of a + * list operator's array. `$null` / `$exists` are skipped — their comparand is + * a declared BOOLEAN and {@link RemoteTransport.nonBooleanNullComparand} / + * {@link RemoteTransport.nonBooleanExistsComparand} already refuse `undefined` + * there by naming the declared domain, which is the better message for that + * mistake and the one #5240 says must stay the only one. + * + * Node positions are recursed, never judged: a `$not` operand or an `$and` + * element that is not a filter node keeps reaching its own refusal + * ({@link RemoteTransport.uncompilableSubFilter}) with the shape the caller + * actually sent. + */ + private assertDefinedComparands(object: string, node: Record, path: string): void { + for (const [key, value] of Object.entries(node)) { + if (key === '$and' || key === '$or') { + if (!Array.isArray(value)) continue; + value.forEach((element, index) => { + if (isFilterNode(element)) { + this.assertDefinedComparands(object, element, `${path}.${key}[${index}]`); + } + }); + continue; + } + if (key === '$not') { + if (isFilterNode(value)) this.assertDefinedComparands(object, value, `${path}.${key}`); + continue; + } + // Any other `$`-key is an undeclared combinator, refused by name in the + // compile loop (#5769). Leaving it alone here keeps that message. + if (key.startsWith('$')) continue; + + const here = `${path}.${key}`; + if (value === undefined) throw this.undefinedComparand(object, key, here); + // `isOperatorMap` and NOT `isFilterNode`, because this must walk exactly + // what the compile loop below ROUTES to its operator arms — a `Date` is a + // value comparand on both tests, but a class instance is an operator map + // to the router and not a filter node, and its entries do reach the + // operator loop (#1066's routing seam). + if (!isOperatorMap(value)) continue; + for (const [op, opValue] of Object.entries(value as Record)) { + if (op === '$null' || op === '$exists') continue; + const opPath = `${here}.${op}`; + if (opValue === undefined) throw this.undefinedComparand(object, key, opPath); + if (!Array.isArray(opValue)) continue; + opValue.forEach((member, index) => { + if (member === undefined) throw this.undefinedComparand(object, key, `${opPath}[${index}]`); + }); + } + } + } + + /** + * The error for an `undefined` comparand (#6050). + * + * The requirement sentence is `driver-sql`'s + * ({@link undefinedComparandError}), word for word from "Comparand at" to + * "matched every env-wide row instead of failing" — one condition, one + * wording, whichever transport the caller happened to reach (#5240). Only the + * `[RemoteTransport]` prefix and the `''` qualifier are added, exactly + * as {@link RemoteTransport.undeclaredCombinator} adds them to its own shared + * sentence. + */ + private undefinedComparand(object: string, field: string, path: string): Error { + return invalidFilterError( + `[RemoteTransport] Comparand at ${path} on '${object}' is undefined. @objectstack/spec ` + + `FieldOperatorsSchema declares no undefined comparand, and in JavaScript ` + + `{ "${field}": undefined } cannot be told apart from omitting the key — yet the two mean ` + + `OPPOSITE things (a predicate versus no constraint at all), so there is no reading of it ` + + `that is not a guess. Write null if you meant the null predicate ({ "${field}": null } or ` + + `{ "${field}": { "$null": true } }), or omit the key when the value is genuinely absent ` + + `(e.g. \`if (id !== undefined) where.${field} = id\`). It is refused rather than compiled ` + + `because the backends disagreed: driver-sql handed it to knex and got a bare "Undefined ` + + `binding(s)" Error carrying no code, while Turso's remote transport compiled it to IS NULL ` + + `— so \`{ owner_id: ctx.user?.id }\` with a missing id silently matched every env-wide row ` + + `instead of failing (#6050).`, + ); + } + private buildSubFilterSQL( object: string, branch: '$and' | '$or' | '$not', @@ -2191,7 +2348,12 @@ export class RemoteTransport { * and an OPERATOR MAP there (#1066). */ private serializeComparand(object: string, field: string, op: string, value: unknown): any { - if (value === null || value === undefined) return null; + // [#6050] `undefined` no longer maps silently to a `null` bind. That + // mapping is how `{ $gt: undefined }` became `col > NULL` (UNKNOWN for + // every row — a filter that answers the empty page and reports nothing); + // the comparand is refused upstream, and if one ever reached here it now + // falls to the allow-list below and is named rather than laundered. + if (value === null) return null; if (isBindableObjectComparand(value)) return value.toISOString(); if ( typeof value === 'string' || 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 index 3175a56ff3..3c31bc930e 100644 --- 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 @@ -200,6 +200,47 @@ const PARITY_CASES: Array<{ name: string; filter: unknown; expected: string[]; w /** Every non-boolean `$exists` comparand both transports must REFUSE. */ const NON_BOOLEAN_EXISTS: unknown[] = ['yes', 1, 0, null, undefined, {}, 'false', []]; +/** + * [#6050] Every COMPARAND position an `undefined` can occupy, which both + * transports must refuse with the same code and the same sentence. + * + * #6047 deliberately left this column of the matrix uncovered — it was writing + * the NULL-safe polarity table and `undefined` turned out to be a DIFFERENT + * defect underneath it, filed as #6050 rather than settled as a rider. This is + * that column, measured on `origin/main` at `cba7454df` before the fix: + * + * | filter | LOCAL | REMOTE | + * |---|---|---| + * | `{ d: undefined }` | knex `Undefined binding(s)` | `['3','4']` | + * | `{ d: { $eq: undefined } }` | knex `Undefined binding(s)` | `['3','4']` | + * | `{ $not: { d: undefined } }` | knex `Undefined binding(s)` | `['1','2']` | + * | `{ d: { $ne: undefined } }` | `['1','2']` | `['1','2']` | + * | `{ $not: { d: { $ne: undefined } } }` | `[]` | `['3','4']` | + * | `{ d: { $in: [undefined] } }` | knex `Undefined binding(s)` | `[]` | + * | `{ d: { $gt: undefined } }` | knex `Undefined binding(s)` | `[]` | + * + * Note the two rows that were never a CRASH-vs-answer split: `$ne: undefined` + * agreed by coincidence, and `$not: { $ne: undefined }` disagreed while BOTH + * sides answered — this driver's own guard contradicting its own emitter + * (`=== null` versus `== null`), which is the #5298 invariant broken at its + * definition. Ruled REFUSED on 2026-08-07, per #5347-A. + */ +const UNDEFINED_COMPARANDS: Array<[label: string, where: unknown]> = [ + ['a direct comparand', { d: undefined }], + ['$eq', { d: { $eq: undefined } }], + ['$ne', { d: { $ne: undefined } }], + ['$gt', { d: { $gt: undefined } }], + ['$contains', { d: { $contains: undefined } }], + ['$notContains', { d: { $notContains: undefined } }], + ['an $in member', { d: { $in: [undefined] } }], + ['an $in member beside a good one', { d: { $in: ['v1', undefined] } }], + ['a $nin member', { d: { $nin: [undefined] } }], + ['inside $and', { $and: [{ d: undefined }] }], + ['inside $or, beside a satisfiable disjunct', { $or: [{ a: 'x' }, { d: undefined }] }], + ['inside $not', { $not: { d: undefined } }], + ['inside $not, one operator down', { $not: { d: { $ne: undefined } } }], +]; + describe('[#5903] TursoDriver LOCAL and REMOTE answer the NULL family identically', () => { let local: TursoDriver; let remote: TursoDriver; @@ -296,4 +337,61 @@ describe('[#5903] TursoDriver LOCAL and REMOTE answer the NULL family identicall expect(ids(await driver.find('conformance', { object: 'conformance', where: { d: { $exists: false } } } as QueryAST))).toEqual(['3', '4']); } }); + + // ── [#6050] The `undefined` column of the matrix ─────────────────────────── + + describe('[#6050] both transports refuse an undefined comparand, identically', () => { + for (const [label, where] of UNDEFINED_COMPARANDS) { + it(`refuses ${label} on both faces, with one code and one sentence`, async () => { + const errors: Record = {}; + for (const [faceLabel, driver] of [['local', local], ['remote', remote]] as const) { + const err = (await driver + .find('conformance', { object: 'conformance', where } as unknown as QueryAST) + .catch((e) => e)) as Error & { code?: string; status?: number }; + expect(err, faceLabel).toBeInstanceOf(Error); + // The ENVELOPE is half the fix (#1116 / #4436): LOCAL used to throw + // knex's bare `Undefined binding(s)` with neither code nor status, so + // an assertion that only checked "it threw" would have passed on the + // driver this issue was filed against. + expect(err.code, faceLabel).toBe('INVALID_FILTER'); + expect(err.status, faceLabel).toBe(400); + errors[faceLabel] = err; + } + // #5240 — one condition, one wording. The two compilers are independent + // implementations, so the sentence is what holds them together; only + // the transport prefix and the `on ''` qualifier may differ. + const requirement = + '@objectstack/spec FieldOperatorsSchema declares no undefined comparand'; + expect(errors.local.message).toContain(requirement); + expect(errors.remote.message).toContain(requirement); + expect(errors.local.message).toContain('Write null if you meant the null predicate'); + expect(errors.remote.message).toContain('Write null if you meant the null predicate'); + }); + } + + /** + * ⛔ The control this whole block needs: refusing `undefined` must not have + * cost `null` a single answer, on either face. Every row below is one of + * `PARITY_CASES`' own null spellings, re-asserted here so a regression + * caused by the #6050 gate is attributed to the gate. + */ + it('null comparands still answer identically, and correctly, on both faces', async () => { + const NULL_CASES: Array<[unknown, string[]]> = [ + [{ d: null }, ['3', '4']], + [{ d: { $eq: null } }, ['3', '4']], + [{ d: { $ne: null } }, ['1', '2']], + [{ d: { $null: true } }, ['3', '4']], + [{ d: { $null: false } }, ['1', '2']], + [{ $not: { d: null } }, ['1', '2']], + [{ $not: { d: { $ne: null } } }, ['3', '4']], + [{ d: { $in: ['v1', null] } }, ['1']], + ]; + for (const [where, expected] of NULL_CASES) { + const localIds = ids(await local.find('conformance', { object: 'conformance', where } as QueryAST)); + const remoteIds = ids(await remote.find('conformance', { object: 'conformance', where } as QueryAST)); + expect(remoteIds, `divergence on ${JSON.stringify(where)}`).toEqual(localIds); + expect(localIds, `wrong answer on ${JSON.stringify(where)}`).toEqual(expected); + } + }); + }); });