From bc8b5d606fb87fb4ab45b95bbcb64ed55455f6b1 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 10 Sep 2026 02:53:47 +0000 Subject: [PATCH] fix(driver-memory): read a stored ARRAY as its elements in the equality arm, so both filter faces answer one filter one way MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `memory-matcher.ts`'s equality arm ended in `value == condition`. Loose `==` converts a stored ARRAY to a primitive — `['a','b']` becomes the string `"a,b"` — so the reference matcher and the live query path (`InMemoryDriver .find`, through mingo) disagreed about the same filter in BOTH directions: | filter | stored | matcher, before | live path | |-------------------|-------------|-----------------|-----------| | `{ tags: 'a' }` | `['a','b']` | no row | the row | | `{ tags: 'a,b' }` | `['a','b']` | the row | no row | | `{ tags: 'a' }` | `['a']` | the row | the row | The second row is the sharper one: a false positive, a filter written to narrow returning a row it should not, which on a read scope is a permission concern rather than a degraded filter. The first is fail-open the other way and just as silent. A stored array is now read as its ELEMENTS, and each is asked the question the arm asks of a scalar — so an array answers the OR of the answers its elements would give. That is mingo's composition, which is this file's standing tie-break: the reference face converges on the path users actually run instead of inventing a third reading. One level only, measured: mingo does not descend into a nested array, so neither does this face. Refusing the shape was not available — a refusal is raised from the FILTER before any row is seen, and this cell is a property of the stored ROW. `comparandEquals` becomes the entry every arm calls; the previous body is `singleValueEquals`, unchanged, deciding one value against one comparand. The live query path is untouched. Tests: `memory-matcher-scalar-comparand-array-value.test.ts` drives BOTH faces in one process over one fixture — the card's three rows, its firing control and its negative twin, `$eq`/`$ne`, a null comparand against a null member, and the OR-over-elements property over the whole matrix. #16810's pin block, which recorded this behaviour as unchanged so its own refusal could not move it by accident, is rewritten rather than deleted: the three answers move with the value side's ruling, and the invariant the block exists for — the comparand refusal must not reach the value side — is now asserted directly. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01XTBcV7zZHmokdyQgXjbyEU --- ...ry-matcher-scalar-comparand-array-value.md | 19 ++ ...y-matcher-array-and-date-comparand.test.ts | 62 ++++- ...tcher-scalar-comparand-array-value.test.ts | 249 ++++++++++++++++++ .../driver-memory/src/memory-matcher.ts | 70 ++++- 4 files changed, 387 insertions(+), 13 deletions(-) create mode 100644 .changeset/memory-matcher-scalar-comparand-array-value.md create mode 100644 packages/drivers/driver-memory/src/memory-matcher-scalar-comparand-array-value.test.ts diff --git a/.changeset/memory-matcher-scalar-comparand-array-value.md b/.changeset/memory-matcher-scalar-comparand-array-value.md new file mode 100644 index 0000000000..dd0926535c --- /dev/null +++ b/.changeset/memory-matcher-scalar-comparand-array-value.md @@ -0,0 +1,19 @@ +--- +"@objectstack/driver-memory": minor +--- + +fix(driver-memory): a scalar comparand against a stored ARRAY is read as membership on both filter faces, so a filter written to narrow stops returning rows it never selected (#16838) + +`memory-matcher.ts`'s equality arm ended in `value == condition`. Loose `==` converts a stored ARRAY to a primitive — `['a','b']` becomes the string `"a,b"` — so this package's reference matcher and its live query path (`InMemoryDriver.find`, through mingo) answered the same filter two different ways, in both directions at once: + +| filter | stored value | reference matcher, before | live query path | +|---|---|---|---| +| `{ tags: 'a' }` | `['a','b']` | no row | the row | +| `{ tags: 'a,b' }` | `['a','b']` | the row | no row | +| `{ tags: 'a' }` | `['a']` | the row | the row | + +The second row is the sharper one: a **false positive**, a filter written to narrow returning a row it should not, which on a read scope is a permission concern rather than a degraded filter. The first is fail-open in the other direction and just as silent — `if (!rows.length)` cannot tell "genuinely none" from "the predicate asked the wrong question". + +**What changes.** A stored array is now read as its elements, and each is asked the question the arm asks of a scalar: the answer for a row storing an array is the OR of the answers for the rows storing its elements. That is MongoDB's array semantics and therefore mingo's, so the reference face converges on the path this package's users actually run rather than on a third reading nobody wrote. One level only — a nested array is not descended into, matching mingo. `$eq` and `$ne` take the same equality as the implicit spelling, so `$ne` stays the exact complement. + +**What does not change.** An array in the **comparand** position is still refused (`INVALID_FILTER` / 400) by the shape gate every face of this package runs; this is the VALUE side, which that door does not judge. The live query path is untouched — it already answered membership — so a caller who only ever used `find()` sees no difference. Callers who compared results against the reference matcher, or who ran it directly as a driver double, will see a stored array select on membership instead of on its joined string. diff --git a/packages/drivers/driver-memory/src/memory-matcher-array-and-date-comparand.test.ts b/packages/drivers/driver-memory/src/memory-matcher-array-and-date-comparand.test.ts index 8d48022631..5e6a8b35a9 100644 --- a/packages/drivers/driver-memory/src/memory-matcher-array-and-date-comparand.test.ts +++ b/packages/drivers/driver-memory/src/memory-matcher-array-and-date-comparand.test.ts @@ -28,11 +28,24 @@ * analytics face answer it identically. * * ⚠️ The third behaviour, pinned here so a later edit cannot take it away by - * accident: a SCALAR comparand against a stored ARRAY is untouched. `==` - * stringifies the stored array (`['a','b']` becomes `"a,b"`), which is a third + * accident: a SCALAR comparand against a stored ARRAY was untouched. `==` + * stringified the stored array (`['a','b']` becomes `"a,b"`), which is a third * bad direction of the same operator — but it is on the VALUE side, and the - * comparand door judges comparands. It is recorded, not repaired, and the - * numbers below are the record. + * comparand door judges comparands. It was recorded, not repaired, and the + * numbers below were the record. + * + * [#16838] **That third behaviour has since been repaired, and this file's last + * block moves with it — deliberately, not by accident.** The pin did its job: + * it stated in one place what the VALUE side answered, so the change that moved + * it had to come and say so here rather than sliding through as a side effect + * of the refusal above. The cell was measured on its own card, on both faces — + * the live query path read `['a','b']` as MEMBERSHIP where this face read the + * joined string `"a,b"` — and the reference face converged on the live one, so + * the numbers below are now the AGREEMENT rather than the record of a + * divergence. ⛔ The block is rewritten, never deleted: what it exists to catch + * — this refusal reaching the value side by accident — is still live, and the + * assertion that the two sides stay distinct is the same assertion whichever + * answer the value side gives. */ import { describe, it, expect } from 'vitest'; @@ -146,13 +159,38 @@ describe('[#16810] an ARRAY comparand is refused, in the ADR-0112 envelope', () }); }); -describe('[#16810] the value side is NOT the comparand side — recorded, not repaired', () => { - it('a scalar comparand against a stored array keeps the answers it had', () => { - // `==` stringifies the stored array. All three lines are the behaviour - // BEFORE this change as well; they are pinned so the refusal above cannot - // silently take the third direction of `==` with it. - expect(match({ tags: ['a', 'b'] }, { tags: 'a' })).toBe(false); - expect(match({ tags: ['a', 'b'] }, { tags: 'a,b' })).toBe(true); // ⚠️ the coercion, still here - expect(match({ tags: ['a'] }, { tags: 'a' })).toBe(true); // ⚠️ and its single-element form +describe('[#16810/#16838] the value side is NOT the comparand side — still two cells, both now answered', () => { + it('a scalar comparand against a stored array is MEMBERSHIP, and is not refused', () => { + // [#16838] The three lines this block pinned as UNCHANGED under #16810, + // with the two that #16838 moved and the one it did not: + // + // before → after + // `{tags:'a'}` vs `['a','b']` false → true the missing membership reading + // `{tags:'a,b'}` vs `['a','b']` true → false the false positive, the sharper half + // `{tags:'a'}` vs `['a']` true → true the firing control, unmoved + // + // They are still asserted here, and still for #16810's reason: this file's + // refusal is about the COMPARAND, and an edit that let it reach the VALUE + // side would turn the first two lines into a throw. Their VALUES track the + // value side's own ruling; the shape of the assertion — an answer, not an + // exception — is what #16810 pinned and it is unchanged. + expect(match({ tags: ['a', 'b'] }, { tags: 'a' })).toBe(true); + expect(match({ tags: ['a', 'b'] }, { tags: 'a,b' })).toBe(false); + expect(match({ tags: ['a'] }, { tags: 'a' })).toBe(true); + }); + + it('the ARRAY-comparand refusal did not follow the value side — a stored array is still evaluated', () => { + // The invariant this block was created to hold, stated directly rather than + // left to be inferred from the three answers above: the door refuses an + // array in the COMPARAND position and says nothing about a stored one, so a + // scalar comparand against any stored array must ANSWER. + for (const stored of [['a', 'b'], ['a'], [] as unknown[], [null, 'b'], [['a']]]) { + expect(() => match({ tags: stored }, { tags: 'a' }), `stored ${JSON.stringify(stored)} was refused`) + .not.toThrow(); + } + // …while the comparand position still refuses, on the same row. + expect(() => match({ tags: ['a', 'b'] }, { tags: ['a', 'b'] })).toThrow( + /requires a single comparable value/, + ); }); }); diff --git a/packages/drivers/driver-memory/src/memory-matcher-scalar-comparand-array-value.test.ts b/packages/drivers/driver-memory/src/memory-matcher-scalar-comparand-array-value.test.ts new file mode 100644 index 0000000000..033f079606 --- /dev/null +++ b/packages/drivers/driver-memory/src/memory-matcher-scalar-comparand-array-value.test.ts @@ -0,0 +1,249 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#16838] A SCALAR comparand against a stored ARRAY value — the VALUE side of + * the equality arm, and the third bad direction of `==` that #16810 recorded + * and deliberately did not repair. + * + * # What was measured, and why it is one defect and not two + * + * `checkCondition`'s equality arm ended in `value == condition`. Loose `==` + * converts the stored ARRAY to a primitive, so `['a','b']` becomes the string + * `"a,b"` — and that single conversion produced a disagreement between this + * package's two filter faces in BOTH directions at once: + * + * | filter | stored | reference matcher, BEFORE | live `InMemoryDriver.find` (mingo) | + * |---|---|---|---| + * | `{ tags: 'a' }` | `['a','b']` | `false` — no row | the row | + * | `{ tags: 'a,b' }` | `['a','b']` | `true` — the row | no row | + * | `{ tags: 'a' }` | `['a']` | `true` — the row | the row | + * + * The second row is the sharper one: a FALSE POSITIVE, a filter written to + * narrow returning a row it should not, which on an RLS read scope is a + * permission concern rather than a degraded filter (#3948, and the identical + * notes `memory-matcher.ts` already carries for `$null`, for the malformed + * `$between` shape and for an unknown operator). The third row is the firing + * control: it answers the same on both faces before and after, so a suite that + * went green by never running would not look like a pass. + * + * # Which face was chosen, and why it was not a free choice + * + * The live path's membership reading is MongoDB's array semantics; the + * matcher's string-join reading is an accident of the operator it happens to be + * written with. This file's tie-break is the one `memory-matcher.ts` has used + * since #5240, #5324, #5328 and #5374 — the live mingo path is what this + * package's users actually run, so the reference face converges on it, cell for + * cell. Refusing the shape was the third answer available and is not open here: + * a refusal is raised from the FILTER (`assertFilterConditionShape` walks the + * filter, once, before any row is seen) and this cell is a property of the + * stored ROW, so a refusal would have to fire or not fire depending on the data + * — the very record-dependence #5240 moved the shape walk out of the field loop + * to avoid. + * + * # The rule, stated so it can be checked rather than described + * + * A stored array is read as its elements, and the arm asks each of them the + * question it asks a scalar. That is asserted directly, as a property over the + * whole matrix below: for every case, the answer for a row storing an array + * equals the OR of the answers for the rows storing its elements. It is the + * same composition mingo performs, which is why the two faces agree here by + * construction rather than by coincidence. + * + * ⚠️ One level only, measured rather than reasoned: mingo does not descend into + * a NESTED array, so neither does this face — `[['a']]` does not match `'a'` on + * either face, and that row is in the fixture to hold it. + */ + +import { describe, it, expect, beforeAll } from 'vitest'; + +import { InMemoryDriver } from './memory-driver.js'; +import { match } from './memory-matcher.js'; + +const TABLE = 'array_value_equality'; + +/** + * One fixture, both faces, one process. The two scalar rows are the card's + * firing control — a comparand that legitimately matches and its negative twin + * — and they are asserted in every case below, so "the filter never ran" and + * "the filter correctly excluded everything" cannot read alike. + */ +const ROWS: ReadonlyArray> = [ + { id: 'scalar-hit', tags: 'a' }, + { id: 'scalar-miss', tags: 'z' }, + { id: 'array-multi', tags: ['a', 'b'] }, + { id: 'array-single', tags: ['a'] }, + { id: 'array-other', tags: ['b'] }, + { id: 'array-nested', tags: [['a']] }, + { id: 'array-with-null', tags: [null, 'b'] }, + { id: 'array-empty', tags: [] }, +]; + +/** + * Every case names the row set BOTH faces must answer. The expectations are the + * live path's measured answers — see the header for why that is the tie-break. + */ +const CASES: ReadonlyArray<{ + name: string; + where: Record; + expected: string[]; + /** + * Whether the case asks the equality question or its NEGATION. The OR-over- + * elements property below is a statement about the equality predicate; `$ne` + * is that predicate's complement, so on an array it means "NO element equals" + * — the AND, not the OR. Marking the polarity states which of the two is + * being asserted instead of leaving a reader to infer it from an operator. + */ + polarity: 'equality' | 'negated'; +}> = [ + { + name: "{ tags: 'a' } — a scalar comparand is MEMBERSHIP against a stored array", + where: { tags: 'a' }, + expected: ['array-multi', 'array-single', 'scalar-hit'], + polarity: 'equality', + }, + { + name: "{ tags: 'a,b' } — the JOINED string matches nothing; the false positive is gone", + where: { tags: 'a,b' }, + expected: [], + polarity: 'equality', + }, + { + name: "{ tags: 'z' } — the firing control's negative twin", + where: { tags: 'z' }, + expected: ['scalar-miss'], + polarity: 'equality', + }, + { + name: "{ tags: { $eq: 'a' } } — the operator spelling answers as the implicit one", + where: { tags: { $eq: 'a' } }, + expected: ['array-multi', 'array-single', 'scalar-hit'], + polarity: 'equality', + }, + { + name: "{ tags: { $ne: 'a' } } — and its complement is the exact complement", + where: { tags: { $ne: 'a' } }, + expected: ['array-empty', 'array-nested', 'array-other', 'array-with-null', 'scalar-miss'], + polarity: 'negated', + }, + { + name: '{ tags: null } — a null comparand finds a null MEMBER, and only that', + where: { tags: null }, + expected: ['array-with-null'], + polarity: 'equality', + }, + { + name: "{ tags: 'b' } — the member that is not first, so position cannot be what matches", + where: { tags: 'b' }, + expected: ['array-multi', 'array-other', 'array-with-null'], + polarity: 'equality', + }, +]; + +const sorted = (ids: readonly string[]): string[] => [...ids].sort((x, y) => x.localeCompare(y)); + +/** The reference face: `memory-matcher.ts`, one record at a time. */ +const referenceIds = (where: Record): string[] => + sorted(ROWS.filter((r) => match(r, where)).map((r) => String(r.id))); + +describe('[#16838] a scalar comparand against a stored array — both faces, one process', () => { + let driver: InMemoryDriver; + /** The live face: `InMemoryDriver.find`, through `normalizeFilterCondition` and mingo. */ + let liveIds: (where: Record) => Promise; + + beforeAll(async () => { + driver = new InMemoryDriver({ persistence: false }); + await driver.connect(); + await driver.syncSchema(TABLE, { + fields: { + id: { type: 'text', name: 'id' }, + tags: { type: 'text', name: 'tags' }, + }, + } as never); + for (const row of ROWS) await driver.create(TABLE, { ...row }); + + liveIds = async (where) => { + const rows = (await driver.find(TABLE, { fields: ['id'], where } as never)) as Array>; + return sorted(rows.map((r) => String(r.id))); + }; + }); + + it('the fixture really is all eight rows, arrays included', async () => { + // A case that returns nothing because the seed failed must not read as a + // case that correctly excluded everything. + expect(await liveIds({})).toEqual(sorted(ROWS.map((r) => String(r.id)))); + const stored = (await driver.find(TABLE, {} as never)) as Array>; + expect(stored.find((r) => r.id === 'array-multi')?.tags).toEqual(['a', 'b']); + }); + + for (const c of CASES) { + it(`${c.name} — the LIVE query path`, async () => { + expect(await liveIds(c.where)).toEqual(sorted(c.expected)); + }); + + it(`${c.name} — the REFERENCE matcher`, () => { + expect(referenceIds(c.where)).toEqual(sorted(c.expected)); + }); + } + + it('both faces answer the whole matrix identically', async () => { + for (const c of CASES) { + expect(await liveIds(c.where), `${c.name}: the live query path and the reference matcher disagree`) + .toEqual(referenceIds(c.where)); + } + }); + + /** + * The card's three rows, spelled exactly as it measured them — `match()` + * directly, one row, one filter — so the numbers in the card and the numbers + * here can be compared without reading the fixture above. + */ + it("the card's own three rows, on the reference matcher", () => { + expect(match({ tags: ['a', 'b'] }, { tags: 'a' })).toBe(true); // was false — the missing membership + expect(match({ tags: ['a', 'b'] }, { tags: 'a,b' })).toBe(false); // was true — the false positive + expect(match({ tags: ['a'] }, { tags: 'a' })).toBe(true); // the firing control, unmoved + }); + + /** + * The rule the arm implements, asserted as a property rather than described: + * an array answers what the OR of its elements answers. A future edit that + * reintroduces any whole-array conversion breaks this for every case at once, + * not only for the two the card happened to measure. + */ + it('a stored array answers the OR of the answers its ELEMENTS would give', () => { + for (const c of CASES) { + if (c.polarity !== 'equality') continue; + for (const row of ROWS) { + const stored = row.tags; + if (!Array.isArray(stored)) continue; + // One level only: an element that is itself an array is not descended + // into, on either face. + const elementwise = stored.some((element) => !Array.isArray(element) && match({ tags: element }, c.where)); + expect(match(row, c.where), `${c.name} / ${String(row.id)}: not the OR over its elements`) + .toBe(elementwise); + } + } + }); + + /** + * `$ne` is the equality predicate's exact complement, per row — which on an + * array is "NO element equals", the AND rather than the OR. Stated because + * the two spellings share {@link comparandEquals} and a future edit that + * fixed one direction only would leave a stored array both matching and not + * matching the same comparand. + */ + it('$ne is the per-row complement of $eq, arrays included', () => { + for (const comparand of ['a', 'b', 'z', 'a,b', null]) { + for (const row of ROWS) { + expect( + match(row, { tags: { $ne: comparand } }), + `${String(row.id)} / ${JSON.stringify(comparand)}: $ne is not the complement of $eq`, + ).toBe(!match(row, { tags: { $eq: comparand } })); + } + } + }); + + it('a NESTED array is not descended into — one level, on both faces', async () => { + expect(match({ tags: [['a']] }, { tags: 'a' })).toBe(false); + expect(await liveIds({ tags: 'a' })).not.toContain('array-nested'); + }); +}); diff --git a/packages/drivers/driver-memory/src/memory-matcher.ts b/packages/drivers/driver-memory/src/memory-matcher.ts index 364870cae7..a09ace0602 100644 --- a/packages/drivers/driver-memory/src/memory-matcher.ts +++ b/packages/drivers/driver-memory/src/memory-matcher.ts @@ -271,8 +271,13 @@ function valueWithinRange(value: any, min: any, max: any): boolean { * An Invalid Date has no time value (`NaN`), so it equals nothing, itself * included — JS `Date` convention, `formula`'s answer, and ADR-0053 D-F1's * reading that an Invalid Date has no canonical text. + * + * ⚠️ This function decides ONE stored value against one comparand. A stored + * ARRAY is not one value, and {@link comparandEquals} — the entry every arm + * calls — decides that case before reaching here. ⛔ Do not call this directly + * from an arm: `==` against an array is exactly the conversion #16838 removed. */ -function comparandEquals(value: any, condition: any): boolean { +function singleValueEquals(value: any, condition: any): boolean { if (value instanceof Date && condition instanceof Date) { return value.getTime() === condition.getTime(); } @@ -287,6 +292,69 @@ function comparandEquals(value: any, condition: any): boolean { return value == condition; } +/** + * [#16838] Equality as the arms ask it: one comparand against a stored value + * that may be an ARRAY. + * + * ## What `==` did to a stored array, and in which direction + * + * The arm used to end in `value == condition` for every stored value. `==` + * against an array converts it to a PRIMITIVE — `['a','b']` becomes the string + * `"a,b"` — and that one conversion made this face disagree with the live query + * path in BOTH directions on the same row: + * + * | filter | stored | this face, BEFORE | the live path (`InMemoryDriver.find` → mingo) | + * |---|---|---|---| + * | `{ tags: 'a' }` | `['a','b']` | no row | the row | + * | `{ tags: 'a,b' }` | `['a','b']` | the row | no row | + * + * The second is the sharper one — a FALSE POSITIVE, a filter written to narrow + * returning a row it should not, which on an RLS read scope is a permission + * concern rather than a degraded filter (#3948, and the identical notes this + * file carries for `$null`, for the malformed `$between` shape and for an + * unknown operator). The first is fail-open in the other direction and just as + * silent: `if (!rows.length)` cannot tell "genuinely none" from "the predicate + * asked the wrong question". + * + * ## The rule, and why it is not a free choice + * + * A stored array is read as its ELEMENTS, and each of them is asked the + * question {@link singleValueEquals} asks of a scalar. So the answer for a row + * storing an array is the OR of the answers for the rows storing its elements — + * a property `memory-matcher-scalar-comparand-array-value.test.ts` asserts over + * its whole matrix rather than case by case. + * + * That is MongoDB's array semantics and therefore mingo's, which is this file's + * standing tie-break (#5240, #5324, #5328, #5374): the live path is what users + * of this package actually run, so the reference face converges on it cell for + * cell instead of inventing a third reading. The string-join reading was never + * a reading — no author writes `"a,b"` meaning `['a','b']`. + * + * ⛔ REFUSING the shape, the way #16810 refused an array COMPARAND, is not + * available here and the difference is structural, not a preference: a refusal + * is raised from the FILTER by `assertFilterConditionShape`, once, before any + * row is seen. This cell is a property of the stored ROW, so refusing it would + * fire or not fire depending on the data — the exact record-dependence #5240 + * moved the shape walk out of the field loop to avoid. + * + * ⚠️ ONE level, measured and not reasoned: mingo does not descend into a nested + * array, so an element that is itself an array matches no scalar comparand + * here either (`[['a']]` against `'a'` is no row on both faces — it used to be + * a match on this one, by the same join). + * + * ⚠️ An array COMPARAND does not reach this composition. It is refused at the + * shape gate (#16810) and floored again at the top of {@link checkCondition}; + * if a direct caller gets one here anyway it keeps the answer it had, so this + * change cannot be read as this package growing array-equality semantics on the + * comparand side — the cell #16810 declined to invent. + */ +function comparandEquals(value: any, condition: any): boolean { + if (Array.isArray(value) && !Array.isArray(condition)) { + return value.some((element) => !Array.isArray(element) && singleValueEquals(element, condition)); + } + return singleValueEquals(value, condition); +} + /** * Evaluate a specific condition against a value */