diff --git a/.changeset/matcher-null-value-and-comparand.md b/.changeset/matcher-null-value-and-comparand.md new file mode 100644 index 0000000000..c78572d992 --- /dev/null +++ b/.changeset/matcher-null-value-and-comparand.md @@ -0,0 +1,11 @@ +--- +'@objectstack/driver-memory': patch +--- + +Stop `driver-memory`'s reference matcher from answering a null comparand or a null value differently from the query path beside it. + +`{$eq: null}` did not match a row whose key was ABSENT, although it matched one whose value was a stored `null`. The pre-switch guard in `checkCondition` short-circuited a missing key to "no match" before the `$eq` arm ran, so one operator answered the two readings of "no value" two ways — while the live mingo path, `formula`, the SQL family and the analytics normalizer all read `$eq: null` as the null predicate. `$eq` now reaches its arm, whose loose comparison had the right answer for both readings all along, exactly as its complement `$ne` already did. + +`{$between: [null, null]}` matched every VALUED row, and a well-formed bounded `$between` matched a null-VALUED row. Both come from one line: the range arm was written as an exclusion test, and a relational comparison against a null is false in both directions, so neither disjunct fired and a bounded range stopped bounding — the widening direction, which on a row-level-security read scope is a permission bypass rather than a degraded filter. The arm decides comparability before it compares now: a no-value row is not inside a range with a real bound, a valued row is not inside a range whose bound is absent, and the degenerate range whose both ends are absent selects the no-value rows. A range with one absent end selects nothing rather than everything. + +Every answer above is the one this package's live query path already gave, cell for cell; the two faces no longer answer one filter two ways. Ranges over valued rows, and `$eq` with a real comparand, are unchanged. diff --git a/packages/drivers/driver-memory/src/memory-matcher-null-value-and-comparand.test.ts b/packages/drivers/driver-memory/src/memory-matcher-null-value-and-comparand.test.ts new file mode 100644 index 0000000000..98e417c0fa --- /dev/null +++ b/packages/drivers/driver-memory/src/memory-matcher-null-value-and-comparand.test.ts @@ -0,0 +1,258 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#13494/#13495/#13549] The reference matcher against a null COMPARAND and a + * null VALUE — held to the live mingo path of its own package. + * + * Three cards, two roots, one file. Measured on `df18120502` before the repair: + * + * | filter | reading | matcher | live path | | + * |---|---|---|---|---| + * | `{$eq: null}` | key ABSENT | `[]` | `['3']` | #13494 | + * | `{$between: [null, null]}` | value `null` | `['1','3']` | `['3']` | #13495 | + * | `{$between: ['2026-07-01','2026-07-15']}` | value `null` | `['1','2','4']` | `['1','2']` | #13549 | + * + * ## The two roots — measured, not assumed + * + * #13495 and #13549 ARE one root: the same `$between` arm, the same line, the + * same coercion. It was written as an EXCLUSION test (`value < min || + * value > max`), and a relational comparison against a null is false in BOTH + * directions, so neither disjunct fired and the range stopped bounding. + * + * #13494 is a DIFFERENT root in a different place: the pre-switch guard in + * `checkCondition` short-circuited a MISSING key to "no match" before the + * `$eq` arm ever ran. Nothing in it is a failed comparison — the arm's own + * loose `!=` had the right answer for both readings all along, and the proof + * is `$ne`, which was already on the guard's allowlist and answered both + * readings correctly throughout. + * + * ## Why every expectation here is stated on BOTH faces + * + * This file's recurring defect is not "a wrong answer", it is "two answers": + * #5240, #5324, #5328 and #5374 each closed a cell where this reference face + * and the live query path answered one filter two ways. So no cell below + * asserts a row set alone — each asserts that both faces produce it. A repair + * that moved only one face would pass a one-face suite and re-open the class. + * + * ⚠️ #13357's `$in: [null]` / `$nin: [null]` arms are deliberately ABSENT from + * this file. They are `needs-user-decision` and sitting with the maintainer, + * and pinning their current answers here — in either direction — would + * prejudge that ruling. They were measured byte-identical across this repair + * (the guard exemption is written over the OPERATOR `$eq`, never over "the + * comparand is null", which is what keeps them out of its blast radius); the + * proof is in the PR, not in an assertion here. + */ + +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; + +import { InMemoryDriver } from './memory-driver.js'; +import { match } from './memory-matcher.js'; + +const sorted = (ids: string[]): string[] => [...ids].sort(); + +/** + * The cards' own fixture, in both readings of "no value" — a stored `null` and + * an ABSENT key. The two reach different code (`null` reaches the operator + * arm, `undefined` meets the pre-switch guard first), which is exactly how the + * matcher came to disagree with ITSELF across them. + */ +const NULLED_ROWS: Array> = [ + { id: '1', name: 'a' }, + { id: '3', name: null }, +]; +const MISSING_ROWS: Array> = [ + { id: '1', name: 'a' }, + { id: '3' }, +]; +/** #13549's five-row fixture: three valued, one null, one absent. */ +const SWEEP_ROWS: Array> = [ + { id: '1', v: '2026-07-01' }, + { id: '2', v: '2026-07-15' }, + { id: '3', v: '2026-07-28' }, + { id: '4', v: null }, + { id: '5' }, +]; +/** + * The NUMERIC fixture, and the reason it exists: `null` coerces to `0` under a + * relational comparison, so a null-valued row sits inside `[-1, 1]` while + * every string fixture in the three cards shows the arm repaired. A fix + * validated on strings alone passes those and leaves this one broken. + */ +const NUMERIC_ROWS: Array> = [ + { id: '1', n: 5 }, + { id: '2', n: 0 }, + { id: '3', n: null }, + { id: '4' }, +]; + +let nulled: InMemoryDriver; +let missing: InMemoryDriver; +let sweep: InMemoryDriver; +let numeric: InMemoryDriver; + +async function driverFor(rows: Array>): Promise { + const driver = new InMemoryDriver({ persistence: false }); + await driver.connect(); + for (const row of rows) await driver.create('t', { ...row }); + return driver; +} + +beforeAll(async () => { + nulled = await driverFor(NULLED_ROWS); + missing = await driverFor(MISSING_ROWS); + sweep = await driverFor(SWEEP_ROWS); + numeric = await driverFor(NUMERIC_ROWS); +}); + +afterAll(async () => { + await nulled.disconnect(); + await missing.disconnect(); + await sweep.disconnect(); + await numeric.disconnect(); +}); + +/** The LIVE query path: `find()` → `normalizeFilterCondition` → mingo. */ +async function liveIds(driver: InMemoryDriver, where: unknown): Promise { + const out = await driver.find('t', { where } as never); + return sorted((out as Array>).map((r) => String(r.id))); +} + +/** The REFERENCE face: the record-at-a-time matcher. */ +const matcherIds = (rows: Array>, where: unknown): string[] => + sorted(rows.filter((row) => match(row, where)).map((row) => String(row.id))); + +/** + * Every cell asserts the row set on BOTH faces, in one call, so a repair that + * moves one of them cannot pass. The expected set is written out literally — + * comparing the two faces to each other alone would be satisfied by both being + * wrong together. + */ +async function bothFaces( + driver: InMemoryDriver, + rows: Array>, + where: unknown, + expected: string[], +): Promise { + expect({ face: 'live', ids: await liveIds(driver, where) }).toEqual({ face: 'live', ids: expected }); + expect({ face: 'matcher', ids: matcherIds(rows, where) }).toEqual({ face: 'matcher', ids: expected }); +} + +describe('[#13494] `$eq: null` is the null predicate on BOTH readings of "no value"', () => { + it('a MISSING key matches `$eq: null` — the cell that disagreed', async () => { + // Was `[]` on the matcher against `['3']` live: `$eq` was not on the + // pre-switch guard's allowlist, so an absent key short-circuited to "no + // match" before the arm ran. #5332 ruled `$eq: null` IS the null predicate. + await bothFaces(missing, MISSING_ROWS, { name: { $eq: null } }, ['3']); + }); + + it('a stored null matches it too — the reading that always worked', async () => { + await bothFaces(nulled, NULLED_ROWS, { name: { $eq: null } }, ['3']); + }); + + it('`$eq: null` now answers exactly what `$null: true` answers, on both readings', async () => { + // The anchor #5332 aligned every other surface to. Before the repair these + // two spellings of one predicate differed on the MISSING reading alone. + for (const [driver, rows] of [[missing, MISSING_ROWS], [nulled, NULLED_ROWS]] as const) { + const viaNull = matcherIds(rows, { name: { $null: true } }); + const viaEq = matcherIds(rows, { name: { $eq: null } }); + expect(viaEq).toEqual(viaNull); + expect(viaEq).toEqual(['3']); + } + }); + + it('`$ne: null` is unmoved — it was already on the allowlist, and was already right', async () => { + await bothFaces(missing, MISSING_ROWS, { name: { $ne: null } }, ['1']); + await bothFaces(nulled, NULLED_ROWS, { name: { $ne: null } }, ['1']); + }); + + it('a REAL comparand keeps the answer it had on a missing key', async () => { + // The guard exemption moved the no-value cells and only those: the arm + // reaches the same verdict the guard did (`undefined != 'a'` is true). + await bothFaces(missing, MISSING_ROWS, { name: { $eq: 'a' } }, ['1']); + await bothFaces(missing, MISSING_ROWS, { name: { $eq: '' } }, []); + await bothFaces(missing, MISSING_ROWS, { name: { $eq: false } }, []); + await bothFaces(nulled, NULLED_ROWS, { name: { $eq: 'a' } }, ['1']); + }); +}); + +describe('[#13495] a `$between` bound that is null no longer stops bounding', () => { + it('`[null, null]` does not match the VALUED row', async () => { + // Was `['1','3']` on the matcher against `['3']` live: `'a' < null` and + // `'a' > null` are BOTH false, so the exclusion test excluded nothing. + await bothFaces(nulled, NULLED_ROWS, { name: { $between: [null, null] } }, ['3']); + }); + + it('`[null, null]` on the MISSING reading selects nothing, on both faces', async () => { + await bothFaces(missing, MISSING_ROWS, { name: { $between: [null, null] } }, []); + }); + + it('a HALF-null bound is the same defect and the same repair', async () => { + // Neither card named these: #13495 measured `[null, null]` only. A range + // with one real end and one absent end is not a meaningful range, and both + // faces now select nothing rather than everything. + await bothFaces(nulled, NULLED_ROWS, { name: { $between: [null, 'z'] } }, []); + await bothFaces(nulled, NULLED_ROWS, { name: { $between: ['a', null] } }, []); + await bothFaces(missing, MISSING_ROWS, { name: { $between: [null, 'z'] } }, []); + await bothFaces(missing, MISSING_ROWS, { name: { $between: ['a', null] } }, []); + }); + + it('a null bound over a NUMERIC column does not match the zero row', async () => { + // `0 >= null` is `true` — null coerces to 0 — so the numeric column is + // where a comparison-only repair silently keeps the defect. + await bothFaces(numeric, NUMERIC_ROWS, { n: { $between: [null, null] } }, ['3']); + }); +}); + +describe('[#13549] a null VALUE is not inside a well-formed bounded range', () => { + it("the card's cell: a bounded `$between` excludes the null-valued row", async () => { + // Was `['1','2','4']` on the matcher against `['1','2']` live. + await bothFaces(sweep, SWEEP_ROWS, { v: { $between: ['2026-07-01', '2026-07-15'] } }, ['1', '2']); + }); + + it('the two readings of "no value" now agree with EACH OTHER', async () => { + // The matcher used to disagree with itself here: the null-valued row + // matched the range while the same absence spelled as a missing key did + // not, because only the second met the pre-switch guard. + const bounded = { v: { $between: ['2026-07-01', '2026-07-28'] } }; + const withNullValue = matcherIds([{ id: 'x', v: null }], bounded); + const withMissingKey = matcherIds([{ id: 'x' }], bounded); + expect(withNullValue).toEqual(withMissingKey); + expect(withNullValue).toEqual([]); + }); + + it('THE NUMERIC CELL — the one a comparison-only repair leaves broken', async () => { + // Rewriting the arm as `!(value >= min && value <= max)` repairs every + // string cell in all three cards and NOT this one: `null` coerces to `0`, + // so `null >= -1 && null <= 1` is true and the null-valued row stays + // inside the range. Comparability is decided before the comparison, and + // this cell is what holds that to the code. + await bothFaces(numeric, NUMERIC_ROWS, { n: { $between: [-1, 1] } }, ['2']); + await bothFaces(numeric, NUMERIC_ROWS, { n: { $between: [0, 10] } }, ['1', '2']); + }); +}); + +describe('[#13494/#13495/#13549] the ordinary vocabulary is untouched', () => { + it('a well-formed range over valued rows still selects the range', async () => { + await bothFaces(sweep, SWEEP_ROWS, { v: { $between: ['2026-07-01', '2026-07-28'] } }, ['1', '2', '3']); + await bothFaces(numeric, NUMERIC_ROWS, { n: { $between: [1, 9] } }, ['1']); + }); + + it('a range that excludes every valued row still selects nothing', async () => { + await bothFaces(sweep, SWEEP_ROWS, { v: { $between: ['2026-09-01', '2026-09-30'] } }, []); + }); + + it('the range boundaries stay CLOSED on both ends', async () => { + // `$between` is `$gte min` AND `$lte max` — what the live path compiles it + // to. An off-by-one in the repair would show up here first. + await bothFaces(sweep, SWEEP_ROWS, { v: { $between: ['2026-07-01', '2026-07-01'] } }, ['1']); + await bothFaces(numeric, NUMERIC_ROWS, { n: { $between: [5, 5] } }, ['1']); + await bothFaces(numeric, NUMERIC_ROWS, { n: { $between: [0, 0] } }, ['2']); + }); + + it('`$null` and `$exists` are unmoved on both readings', async () => { + await bothFaces(missing, MISSING_ROWS, { name: { $null: true } }, ['3']); + await bothFaces(nulled, NULLED_ROWS, { name: { $null: true } }, ['3']); + await bothFaces(missing, MISSING_ROWS, { name: { $exists: true } }, ['1']); + await bothFaces(nulled, NULLED_ROWS, { name: { $exists: true } }, ['1']); + }); +}); diff --git a/packages/drivers/driver-memory/src/memory-matcher.ts b/packages/drivers/driver-memory/src/memory-matcher.ts index aa5e508712..ade9aa050d 100644 --- a/packages/drivers/driver-memory/src/memory-matcher.ts +++ b/packages/drivers/driver-memory/src/memory-matcher.ts @@ -162,6 +162,65 @@ function noValueSatisfiesNegation(op: string): boolean { return op === '$ne' || op === '$nin' || op === '$notContains'; } +/** + * [#13495/#13549] Is `value` inside the closed range `[min, max]`? + * + * `$between` is the conjunction of `$gte min` and `$lte max`. That is not an + * interpretation of the operator, it is what this package's LIVE path compiles + * it to — `memory-driver.ts`'s `$between` arm writes `$gte`/`$lte` and hands + * them to mingo — so this face has to compute the same predicate. + * + * The arm used to be written the other way round, as an EXCLUSION test: + * `if (value < min || value > max) return false`. The two are equivalent only + * while both comparisons are MEANINGFUL, and against a null they are not: JS + * answers a relational comparison between a null and a string `false` in both + * directions (`null` coerces to `0`, the string to `NaN`). Neither disjunct + * fired, nothing returned false, and a bounded range silently stopped bounding + * — the WIDENING direction, which on an RLS read scope is a permission bypass + * rather than a degraded filter (#3948, and the identical notes this file + * already carries for the malformed `$between` shape and for `$null`). Two + * cards measured the two ways in: + * + * - #13495, the COMPARAND axis: `{$between: [null, null]}` matched every + * VALUED row, because `'a' < null` and `'a' > null` are both false. + * - #13549, the VALUE axis: a null-valued row sat inside a well-formed bounded + * range, because `null < '2026-07-01'` and `null > '2026-07-15'` are both + * false. + * + * ⚠️ Flipping the test to `!(value >= min && value <= max)` repairs the + * string cells and NOT the numeric ones — measured, not reasoned: `null` + * coerces to `0`, so `null >= -1 && null <= 1` is `true` and a null-valued row + * stays inside a numeric range while looking repaired on every string fixture + * the cards used. Comparability has to be decided BEFORE the comparison, which + * is what this function does: + * + * - a no-value row is not inside a range with a real bound, and a valued row is + * not inside a range whose bound is no value — that comparison is not + * meaningful, and JS answers it anyway; + * - the degenerate range whose BOTH ends are no value selects the no-value + * rows, and only those. + * + * Every one of those answers is the one this package's live mingo path already + * gives, cell for cell — the tie-break this file has used since #5240, #5324, + * #5328 and #5374, each of which closed a "this face and the live one answer + * one filter two ways" divergence. No new reading of "no value" is asserted + * here: what a stored null MEANS is #13357's question and it is the + * maintainer's. + */ +function valueWithinRange(value: any, min: any, max: any): boolean { + const valueIsNoValue = value === null || value === undefined; + const minIsNoValue = min === null || min === undefined; + const maxIsNoValue = max === null || max === undefined; + + // Mixed: one side is a value and the other is an absence. Not comparable. + if (valueIsNoValue !== minIsNoValue || valueIsNoValue !== maxIsNoValue) return false; + + // Both ends and the value are absences: the degenerate null-to-null range. + if (valueIsNoValue) return true; + + return value >= min && value <= max; +} + /** * Evaluate a specific condition against a value */ @@ -207,7 +266,30 @@ function checkCondition(value: any, condition: any): boolean { // match" for `$nin` and `$notContains` before their arms ever ran — one // of the two independent causes of the divergence #13166 measured, and // the only one this guard can reach. - if (value === undefined && op !== '$exists' && op !== '$null' && !noValueSatisfiesNegation(op)) { + // + // [#13494] `$eq` joins them, and for the SAME reason its complement + // `$ne` was already here: its arm decides the no-value case itself, + // in one place, with the loose `!=` that reads `undefined` and `null` + // as one absence. This guard was deciding it FIRST and differently, so + // one operator answered the two readings of "no value" two ways — a + // NULLED row reached the arm and matched `$eq: null`, a MISSING row + // short-circuited to "no match" before the arm ever ran. #5332 ruled + // that `$eq: null` IS the null predicate, and every other surface that + // can express the MISSING reading already answers it so — including + // this package's own live mingo path, measured: `['3']` where this + // face said `[]`. + // + // A non-null comparand keeps the answer it had, because the arm + // reaches the same verdict the guard did: `undefined != 'a'` is true, + // so the row is excluded one line further down. + // + // ⚠️ `$eq` ONLY, deliberately. The exemption is written over the + // OPERATOR and not over "the comparand is null", because the latter + // spelling would have moved `$in: [null]` / `$nin: [null]` with it — + // and those are #13357's cells, `needs-user-decision`, held for the + // maintainer. They are measured byte-identical across this change. + if (value === undefined && op !== '$exists' && op !== '$null' && op !== '$eq' + && !noValueSatisfiesNegation(op)) { return false; } @@ -239,7 +321,14 @@ function checkCondition(value: any, condition: any): boolean { // no longer this face's ANSWER to a malformed range: it used to // skip the comparison entirely, which meant "matches EVERY row" // — the opposite of what the live query path silently answered. - if (Array.isArray(target) && (value < target[0] || value > target[1])) return false; + // + // [#13495/#13549] The comparison is {@link valueWithinRange} + // now, and no longer an EXCLUSION test spelled with `<` and + // `>`. Against a null comparand or a null value that test was + // false in BOTH directions, so nothing returned false and the + // range stopped bounding — the same widening this arm's note + // above records for the malformed SHAPE. + if (Array.isArray(target) && !valueWithinRange(value, target[0], target[1])) return false; break; // Sets diff --git a/packages/drivers/driver-memory/src/memory-operator-key-clobber.test.ts b/packages/drivers/driver-memory/src/memory-operator-key-clobber.test.ts index 806e9c01c1..4272d522b3 100644 --- a/packages/drivers/driver-memory/src/memory-operator-key-clobber.test.ts +++ b/packages/drivers/driver-memory/src/memory-operator-key-clobber.test.ts @@ -32,14 +32,21 @@ * express this defect, and it is the face #5962 aligned. Every cell below is * scored against it. * - * ⚠️ With ONE measured exception, kept deliberately and NOT repaired here: - * `$between` ALONE already disagrees with the reference matcher on a row whose - * value is `null` (live `['1','2']`, matcher `['1','2','4']` on the enumeration - * fixture). That is the reference matcher's own `$between` defect — a - * separately queued card — so the sweep below scores the live path against - * ITSELF (the composition law) rather than against the matcher, and the - * matcher is the oracle for the named cells, where the two agree operator by - * operator. + * ⚠️ That exception is now CLOSED, and this note records it rather than + * repeating it. `$between` ALONE used to disagree with the reference matcher on + * a row whose value is `null` (live `['1','2']`, matcher `['1','2','4']` on the + * enumeration fixture) — the matcher's own `$between` defect, which #13549 + * repaired together with #13494 and #13495. The two faces answer that filter + * identically now, and `memory-matcher-null-value-and-comparand.test.ts` holds + * them there. + * + * The sweep below still scores the live path against ITSELF (the composition + * law) rather than against the matcher, and that is deliberate: it is a + * statement about what a CLOBBER test measures — two constraints on one field + * select exactly the rows both select alone — and it keeps this file's verdict + * independent of the matcher's own cells. It was never a workaround for the + * divergence, so closing the divergence does not change it. The matcher remains + * the oracle for the named cells, where the two agree operator by operator. * * ## Why the sweep ranges over the vocabulary and not over three operators *