From 3db8a3ec4054c785dff988c9edabddfabecb1824 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 10 Aug 2026 10:17:46 +0000 Subject: [PATCH 1/2] fix(driver-sql): refuse scalar-comparison operators on JSON/multi-value columns MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A `multiple: true` field — and every other `JSON_COLUMN_TYPES` field — is stored as a JSON TEXT column, and the equality family lowered straight to SQL against that text with no column-type consultation. The result was a wrong answer with a 200: {members:{$in:[U1]}} -> 0 rows (fail-closed) {members: U1} -> 0 rows (fail-closed) {members:{$nin:[U1]}} -> the excluded row (fail-OPEN) {members:{$lte:U1}} -> 1 row, lexicographic on the leading '[' `members not in ('U1')` is TRUE — the stored text genuinely is not equal to that id — so "exclude these" compiled to "return everything". An exclusion that silently stops excluding widens a result set, and a 200 with [] is byte-identical to a query that legitimately matched nothing, so nothing existed for a caller to key on. Gate the three lowering entries on the column type, ahead of every rewrite and both comparison emitters: the operator-object branch and the bare-value branch of applyFilterCondition, and the plain-map loop of applyFilters. Placing it before applyNormalizedComparison matters — a `multiple: true` datetime column on an external object is served by the normalised whereRaw arms rather than the plain whereIn arms, and showed the identical defect. The refusal names the operator, the field, why the column cannot answer it, states the filter was not applied, and prescribes $contains (or an $or of $contains for any-of). ADR-0112 class 1 — INVALID_FILTER / 400, the same envelope as the unknown-operator refusal, on every face that lowers a filter. $contains / $notContains / $startsWith / $endsWith / $icontains and the null predicates are untouched: the LIKE family matches the serialization as text and is the only working membership spelling, and column presence is a well-formed question whatever the column holds. Fixes #7398 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_011GCuQuqxKvWLYUGss7CXdc --- ...driver-sql-json-column-operator-refusal.md | 29 + ...river-json-column-operator-refusal.test.ts | 500 ++++++++++++++++++ packages/drivers/driver-sql/src/sql-driver.ts | 165 ++++++ 3 files changed, 694 insertions(+) create mode 100644 .changeset/driver-sql-json-column-operator-refusal.md create mode 100644 packages/drivers/driver-sql/src/sql-driver-json-column-operator-refusal.test.ts diff --git a/.changeset/driver-sql-json-column-operator-refusal.md b/.changeset/driver-sql-json-column-operator-refusal.md new file mode 100644 index 0000000000..6da29f80a1 --- /dev/null +++ b/.changeset/driver-sql-json-column-operator-refusal.md @@ -0,0 +1,29 @@ +--- +'@objectstack/driver-sql': patch +--- + +fix(driver-sql): refuse scalar-comparison operators on JSON/multi-value columns with 400 `INVALID_FILTER` instead of answering a silently wrong result + +A `multiple: true` field — and every other `JSON_COLUMN_TYPES` field — is stored by this driver as a **JSON TEXT** column. The equality family lowered straight to SQL against that text with no column-type consultation, so a filter naming such a column compiled, ran, and returned a wrong answer with a `200`. + +**Behaviour change (user-visible).** On a row whose `members` holds `["U1","U2"]`: + +| filter | before | after | +|---|---|---| +| `{members:{$in:[U1]}}` | `200`, **0 rows** | `400 INVALID_FILTER` | +| `{members:{$eq:U1}}` | `200`, **0 rows** | `400 INVALID_FILTER` | +| `{members: U1}` (bare equality) | `200`, **0 rows** | `400 INVALID_FILTER` | +| `{members:{$nin:[U1]}}` | `200`, **the row it was asked to EXCLUDE** ⚠️ | `400 INVALID_FILTER` | +| `{members:{$ne:U1}}` | `200`, **the row it was asked to exclude** ⚠️ | `400 INVALID_FILTER` | +| `{members:{$lte:U1}}` | `200`, **1 row** (lexicographic, on the leading `[`) | `400 INVALID_FILTER` | +| `{members:{$contains:U1}}` | `200`, 1 row | **unchanged** | + +**`$nin` is why this is a fix and not a documented footgun.** `members not in ('U1')` is TRUE — the stored text genuinely is not equal to that id — so "exclude these" compiled to "return everything". `$in` fails **closed** (fewer rows than exist, bad but narrowing); `$nin` and `$ne` fail **OPEN**, so any exclusion built on them silently stops filtering and the failure direction is *widening*. A downstream delete-guard written as `plans.find({ where: { assignees: { $in: memberIds } } })` therefore never fired once since it shipped, threw nothing, logged nothing, and type-checked — and a `200` with `[]` is byte-identical to a query that legitimately matched nothing, so no caller had anything to key on. + +**What is refused:** `$eq`, `$ne`, `$gt`, `$gte`, `$lt`, `$lte`, `$in`, `$nin`, `$between`, the bare `{ field: value }` spelling, and the infix spellings the normalised emitter also answers (`=`, `<>`, `in`, `nin`, `not_in`, `notin`, …) — on any column this driver stores as JSON, i.e. `field.multiple` arrays **and** the structured-JSON types (`address`, `location`, `composite`, the file-metadata and multi-option types). The structured-JSON half is included because the mechanism is the JSON-text storage rather than the array-ness: `{address:{$nin:['Beijing']}}` showed the identical fail-open inversion. + +The refusal names the operator, the field, why the column cannot answer it, states that the filter **was not applied**, and prescribes the working spelling. It carries the same ADR-0112 envelope as the unknown-operator refusal (`INVALID_FILTER` / 400), on every face that lowers a filter: `find`, `findOne`, `count`, `aggregate`, `distinct`, and the where-clauses of `updateMany` / `deleteMany`. + +**What does NOT change:** `$contains`, `$notContains`, `$startsWith`, `$endsWith`, `$icontains` — the `LIKE` family matches the serialization as text, and `$contains` (or an `$or` of `$contains` for any-of) is the working membership spelling this refusal points at. `$null` / `$exists` also keep working: the column's presence is a well-formed question whatever it holds. Filters on scalar columns are untouched, and a table this driver was never told about (no registered field types) is unaffected — the gate fires only where the column is KNOWN to be JSON. + +Giving array columns a real membership operator (`$overlaps` / `$containsAny`) is a separate question about the closed `FILTER_OPERATORS` set and is deliberately not answered here. diff --git a/packages/drivers/driver-sql/src/sql-driver-json-column-operator-refusal.test.ts b/packages/drivers/driver-sql/src/sql-driver-json-column-operator-refusal.test.ts new file mode 100644 index 0000000000..ff185edb98 --- /dev/null +++ b/packages/drivers/driver-sql/src/sql-driver-json-column-operator-refusal.test.ts @@ -0,0 +1,500 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#7398] A DECLARED operator against a column type it cannot apply to is + * refused, loudly, instead of compiling to a silently wrong answer. + * + * `driver-sql` stores a `multiple: true` field — and every other + * `JSON_COLUMN_TYPES` field — as a JSON TEXT column. The equality family lowered + * straight to SQL against that text with no column-type consultation, so on the + * issue's fixture (ONE row whose `members` holds `["U1","U2"]`): + * + * | filter | before | direction | + * |---------------------------|---------------|-------------| + * | `{members:{$in:[U1]}}` | 0 rows, `200` | fail-CLOSED | + * | `{members:{$eq:U1}}` | 0 rows, `200` | fail-CLOSED | + * | `{members: U1}` (bare) | 0 rows, `200` | fail-CLOSED | + * | `{members:{$nin:[U1]}}` | **1 row** | fail-OPEN ⚠️ | + * | `{members:{$ne:U1}}` | **1 row** | fail-OPEN ⚠️ | + * | `{members:{$lte:U1}}` | **1 row** | just WRONG | + * | `{members:{$contains:U1}}`| 1 row | ✅ correct | + * + * The `$nin` line is why this is a defect and not a footgun. `members not in + * ('U1')` is TRUE — the stored text genuinely is not equal to that id — so + * "exclude these" compiled to "return everything". An exclusion that silently + * stops excluding WIDENS a result set, which is the direction #3948 / #4209 / + * #5347 all ruled outranks a narrowing one; the issue's downstream delete-guard + * (`{ assignees: { $in: memberIds } }`) never fired once since it shipped, and + * a `200` with `[]` is byte-identical to a query that legitimately matched + * nothing, so nothing existed for a caller to key on. + * + * `$lte` is worth its own row: the answers were never merely empty. `["usr_…"` + * sorts below `usr_…` on the leading `[`, so the ordering comparisons return a + * lexicographic verdict over a *serialization*. + * + * ## What these tests pin + * + * 1. **The refusal**, per operator and per FACE. #6203's one-query-two-answers + * rule: a filter refused by `find` and compiled by `count` is two answers to + * one question, so every face that lowers a filter is swept — + * `find`/`findOne`/`count`/`aggregate`/`distinct` and the where-clauses of + * `updateMany`/`deleteMany`. + * 2. **`$contains` unchanged**, on the same fixture and the same faces. It is + * the only working membership spelling and downstream code depends on it, so + * it is pinned on both sides of the change. + * 3. **Both lowering families.** A managed `multiple: true` lookup is served by + * the plain-column arms (`whereIn` / `orWhereNotIn` / `where(f, op, v)`); + * a `multiple: true` DATETIME column on an EXTERNAL object (ADR-0015) is + * served by `applyNormalizedComparison`'s normalised `whereRaw` arms + * instead, because `registerExternalObject` never marks its datetime columns + * canonical and `needsLegacyDatetimeRepair` therefore stays true. Both were + * measured wrong the same way before this gate; a gate in only one of them + * leaves the other silently wrong, which is what makes the external cell a + * POSITIVE CONTROL on the site enumeration rather than a second example. + * 4. **A closed-world sweep over `FILTER_OPERATORS`**, so a newly declared + * operator or a newly added lowering site cannot join the silent set without + * turning this file red. + * + * Out of scope by the dispatch's ruling: the issue's ask 2 (`$overlaps` / + * `$containsAny`), which would open the closed `FILTER_OPERATORS` set. + */ + +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import { SqlDriver } from '../src/index.js'; +import { FILTER_OPERATORS } from '@objectstack/spec/data'; +import type { FilterCondition } from '@objectstack/spec/data'; + +const U1 = 'usr_1111'; +const U2 = 'usr_2222'; + +/** The shape `mapDataError` / `sendError` read off a thrown driver error. */ +interface WireBearingError extends Error { + code?: string; + status?: number; +} + +async function refusalOf(run: () => Promise): Promise { + try { + await run(); + } catch (e) { + return e as WireBearingError; + } + throw new Error('expected the driver to refuse this filter, but it resolved'); +} + +/** + * Every assertion this file makes about a refusal, in one place: the ADR-0112 + * wire identity AND the actionable content. Never a bare `toThrow` (#6144) — + * a refusal with no `code` is the #4436 defect, and one that does not name the + * working spelling sends its author back for another round-trip. + */ +function expectJsonColumnRefusal(err: WireBearingError, op: string, field: string): void { + expect(err.code).toBe('INVALID_FILTER'); + expect(err.status).toBe(400); + expect(err.message).not.toContain('[sql-driver]'); + expect(err.message).toContain(op); + expect(err.message).toContain(field); + // The three things the message owes a caller: that the filter did NOT run, + // why (the column is JSON text), and what to write instead. + expect(err.message).toContain('WAS NOT APPLIED'); + expect(err.message).toContain('JSON TEXT column'); + expect(err.message).toContain('$contains'); +} + +/** + * The refused operators, each with a comparand of the shape its own schema + * declares — so a failure here is always the column-type gate, never a + * comparand-shape refusal wearing its message. + */ +const REFUSED: ReadonlyArray = [ + ['$eq', U1], + ['$ne', U1], + ['$gt', U1], + ['$gte', U1], + ['$lt', U1], + ['$lte', U1], + ['$in', [U1]], + ['$nin', [U1]], + // Not in the card's stated minimum set, refused deliberately: `$between` IS + // `>= AND <=`, and this driver decomposes a calendar-day `$between` into + // `$gte`/`$lt` a few lines above the emitter. Refusing the halves while + // compiling the compound would leave the same wrong answer alive at one more + // spelling — the reasoning #5234 already applied in this file. + ['$between', [U1, U2]], +]; + +/** + * The operators that MUST keep working on a JSON column. The `LIKE` family + * matches the serialization as text (which is how `$contains` works at all), + * and the null predicates ask about the column's presence — a well-formed + * question whatever the column holds. + */ +const KEPT: ReadonlyArray = [ + ['$contains', U1], + ['$notContains', 'nobody'], + ['$startsWith', '['], + ['$endsWith', ']'], + ['$icontains', U1], + ['$null', false], + ['$exists', true], +]; + +describe('[#7398] SqlDriver refuses scalar-comparison operators on JSON/multi-value columns', () => { + let driver: SqlDriver; + + beforeAll(async () => { + driver = new SqlDriver({ + client: 'better-sqlite3', + connection: { filename: ':memory:' }, + useNullAsDefault: true, + }); + await driver.initObjects([ + { + name: 'team', + fields: { + id: { type: 'text', name: 'id' }, + name: { type: 'text', name: 'name' }, + // The issue's own fixture column: a multi-value lookup. + members: { type: 'lookup', name: 'members', multiple: true }, + // The single-value control from the issue's second table. + owner: { type: 'lookup', name: 'owner' }, + // A STRUCTURED-JSON column (not `multiple`), for the predicate + // decision pinned in its own block below. + address: { type: 'address', name: 'address' }, + }, + } as any, + ]); + await driver.create('team', { + id: 't1', + name: 'Alpha', + members: [U1, U2], + owner: U1, + address: { city: 'Beijing' }, + }); + }); + + afterAll(async () => { + await driver?.disconnect?.(); + }); + + const find = (where: unknown) => driver.find('team', { where: where as FilterCondition }); + + // ── The card's table, flipped ───────────────────────────────────────────── + + describe('the issue\'s measured table', () => { + it('the fixture really is stored as JSON text — the premise the rest rests on', async () => { + const raw = await (driver as any).knex('team').select('members').first(); + expect(raw.members).toBe(JSON.stringify([U1, U2])); + }); + + for (const [op, comparand] of REFUSED) { + it(`{ members: { "${op}": … } } → 400 INVALID_FILTER`, async () => { + const err = await refusalOf(() => find({ members: { [op]: comparand } })); + expectJsonColumnRefusal(err, op, 'members'); + }); + } + + it('bare equality { members: value } → 400 INVALID_FILTER', async () => { + const err = await refusalOf(() => find({ members: U1 })); + expectJsonColumnRefusal(err, '=', 'members'); + expect(err.message).toContain('bare equality'); + }); + + it('bare equality reached through the OPERATOR walk is refused identically', async () => { + // A sibling key carrying an operator routes the whole node to + // `applyFilterCondition`, whose bare-value branch is a SECOND compilation + // of `{ field: value }`. One condition, one verdict — whatever the + // siblings look like. + const err = await refusalOf(() => find({ members: U1, name: { $eq: 'Alpha' } })); + expectJsonColumnRefusal(err, '=', 'members'); + }); + + it('$nin — the fail-OPEN half — no longer returns the row it was asked to exclude', async () => { + const err = await refusalOf(() => find({ members: { $nin: [U1] } })); + expectJsonColumnRefusal(err, '$nin', 'members'); + // The regression this refusal replaces, stated so the intent survives a + // future reading of the file: this filter used to answer `['t1']` — the + // one row whose `members` CONTAINS U1. + const contained = await find({ members: { $contains: U1 } }); + expect(contained.map((r: any) => r.id)).toEqual(['t1']); + }); + }); + + // ── The spelling that must NOT change ───────────────────────────────────── + + describe('$contains and friends keep working, unchanged', () => { + it('$contains matches each member', async () => { + expect((await find({ members: { $contains: U1 } })).map((r: any) => r.id)).toEqual(['t1']); + expect((await find({ members: { $contains: U2 } })).map((r: any) => r.id)).toEqual(['t1']); + }); + + it('$contains for a member that is not there matches nothing', async () => { + expect(await find({ members: { $contains: 'usr_9999' } })).toEqual([]); + }); + + it('an $or of $contains is the any-of spelling, and it still compiles', async () => { + const rows = await find({ + $or: [{ members: { $contains: U1 } }, { members: { $contains: 'usr_9999' } }], + }); + expect(rows.map((r: any) => r.id)).toEqual(['t1']); + }); + + it('$notContains excludes on the serialization', async () => { + expect(await find({ members: { $notContains: U1 } })).toEqual([]); + expect((await find({ members: { $notContains: 'usr_9999' } })).map((r: any) => r.id)).toEqual(['t1']); + }); + + for (const [op, comparand] of KEPT) { + it(`"${op}" is not refused on a JSON column`, async () => { + await expect(find({ members: { [op]: comparand } })).resolves.toBeInstanceOf(Array); + }); + } + }); + + // ── The control: the same operators on ordinary columns ─────────────────── + + describe('scalar columns are untouched', () => { + it('$in on the single-value lookup still matches — the issue\'s own control row', async () => { + expect((await find({ owner: { $in: [U1] } })).map((r: any) => r.id)).toEqual(['t1']); + }); + + for (const [op, comparand] of REFUSED) { + it(`"${op}" still compiles against a text column`, async () => { + await expect(find({ name: { [op]: comparand } })).resolves.toBeInstanceOf(Array); + }); + } + + it('bare equality on a text column still compiles', async () => { + expect((await find({ name: 'Alpha' })).map((r: any) => r.id)).toEqual(['t1']); + }); + }); + + // ── The predicate decision ──────────────────────────────────────────────── + + describe('the gate keys on JSON STORAGE, not on `multiple` alone', () => { + it('a structured-JSON column shows the identical defect, so it is refused too', async () => { + // Measured on `main` before this change, on this very fixture: + // `{address:{$eq:'Beijing'}}` → 0 rows, and `{address:{$nin:['Beijing']}}` + // → the row — the same fail-open inversion as the array case, because the + // mechanism is the JSON-text storage and not the array-ness. + const eqErr = await refusalOf(() => find({ address: { $eq: 'Beijing' } })); + expectJsonColumnRefusal(eqErr, '$eq', 'address'); + const ninErr = await refusalOf(() => find({ address: { $nin: ['Beijing'] } })); + expectJsonColumnRefusal(ninErr, '$nin', 'address'); + }); + + it('$null / $exists still answer on a JSON column — presence is a fair question', async () => { + expect((await find({ address: { $exists: true } })).map((r: any) => r.id)).toEqual(['t1']); + expect(await find({ address: { $null: true } })).toEqual([]); + }); + }); + + // ── Combinator depth ────────────────────────────────────────────────────── + + describe('the refusal holds at every depth', () => { + it('inside $or', async () => { + const err = await refusalOf(() => find({ $or: [{ name: 'Alpha' }, { members: { $in: [U1] } }] })); + expectJsonColumnRefusal(err, '$in', 'members'); + }); + + it('inside $and', async () => { + const err = await refusalOf(() => find({ $and: [{ name: 'Alpha' }, { members: { $nin: [U1] } }] })); + expectJsonColumnRefusal(err, '$nin', 'members'); + }); + + it('inside $not', async () => { + const err = await refusalOf(() => find({ $not: { members: { $eq: U1 } } })); + expectJsonColumnRefusal(err, '$eq', 'members'); + }); + }); + + // ── #6203: every face that lowers a filter ──────────────────────────────── + + describe('every face that lowers a filter refuses alike', () => { + const faces: ReadonlyArray Promise]> = [ + ['find', (where) => driver.find('team', { where })], + ['findOne', (where) => driver.findOne('team', { where })], + ['count', (where) => driver.count('team', { where })], + [ + 'aggregate', + (where) => + driver.aggregate('team', { where, aggregations: [{ function: 'count', alias: 'n' }] } as any), + ], + ['distinct', (where) => driver.distinct('team', 'name', where)], + ['updateMany', (where) => driver.updateMany('team', { where }, { name: 'Beta' })], + ['deleteMany', (where) => driver.deleteMany('team', { where })], + ]; + + for (const [faceName, run] of faces) { + describe(faceName, () => { + for (const [op, comparand] of REFUSED) { + it(`refuses "${op}"`, async () => { + const err = await refusalOf(() => run({ members: { [op]: comparand } } as FilterCondition)); + expectJsonColumnRefusal(err, op, 'members'); + }); + } + + it('refuses bare equality', async () => { + const err = await refusalOf(() => run({ members: U1 } as FilterCondition)); + expectJsonColumnRefusal(err, '=', 'members'); + }); + + it('still serves $contains', async () => { + if (faceName !== 'updateMany' && faceName !== 'deleteMany') { + await expect(run({ members: { $contains: U1 } } as FilterCondition)).resolves.toBeDefined(); + return; + } + // The write faces are asserted on a scratch row rather than the + // fixture, and on the ROW COUNT rather than on "it resolved": a + // where-clause that reached no row would satisfy a bare resolve while + // proving nothing about whether `$contains` still compiles there. + await driver.create('team', { + id: 'scratch', + name: 'Scratch', + members: ['usr_scratch'], + owner: U1, + }); + const affected = await run({ members: { $contains: 'usr_scratch' } } as FilterCondition); + expect(affected).toBe(1); + await driver.deleteMany('team', { where: { id: 'scratch' } }); + }); + }); + } + + it('the write faces really did not write — a refusal is not a half-applied mutation', async () => { + const row = await driver.findOne('team', { where: { id: 't1' } }); + expect(row.name).toBe('Alpha'); + expect(await driver.count('team', {})).toBe(1); + }); + }); + + // ── The closed-world control on the site enumeration ────────────────────── + + describe('closed-world sweep over the declared vocabulary', () => { + /** A comparand of the shape each declared operator's own schema wants. */ + const comparandFor: Record = { + ...Object.fromEntries(REFUSED), + ...Object.fromEntries(KEPT), + }; + + it('every declared FILTER_OPERATOR is either refused here or on the keep-working list', async () => { + const refused: string[] = []; + const compiled: string[] = []; + for (const op of FILTER_OPERATORS) { + expect( + Object.prototype.hasOwnProperty.call(comparandFor, op), + `FILTER_OPERATORS gained "${op}" and this sweep has no comparand for it — decide ` + + `whether it is meaningful against a JSON TEXT column and add it to REFUSED or KEPT.`, + ).toBe(true); + try { + await find({ members: { [op]: comparandFor[op] } }); + compiled.push(op); + } catch (e) { + const err = e as WireBearingError; + expectJsonColumnRefusal(err, op, 'members'); + refused.push(op); + } + } + // The partition is asserted whole, so an operator quietly moving from one + // side to the other cannot pass as "still 16 operators". + expect(refused).toEqual(REFUSED.map(([op]) => op)); + expect(compiled).toEqual(KEPT.map(([op]) => op)); + }); + + it('an UNDECLARED operator still gets the unknown-operator refusal, not this one', async () => { + // The issue's `$overlaps` row: already 400 before this change, and this + // gate must not annex it — ask 2 (giving array columns a real membership + // operator) is a spec question, deliberately not answered here. + const err = await refusalOf(() => find({ members: { $overlaps: [U1] } })); + expect(err.code).toBe('INVALID_FILTER'); + expect(err.status).toBe(400); + expect(err.message).toContain('Unsupported filter operator'); + expect(err.message).toContain('$overlaps'); + expect(err.message).not.toContain('WAS NOT APPLIED'); + }); + }); +}); + +/** + * [#7398] The POSITIVE CONTROL on the site enumeration. + * + * The issue named `applyNormalizedComparison` as the lowering site; the plain + * `whereIn` / `where(field, op, v)` arms of `applyFilterCondition` are a second, + * separate family. Which one serves a JSON column depends on whether + * `filterColumnExpr` returns an expression — and for a MANAGED table it never + * does, so the fixture above exercises only the plain family. + * + * An EXTERNAL object (ADR-0015) with a `multiple: true` DATETIME field reaches + * the other one: `registerExternalObject` populates `datetimeFields` but never + * runs `backfillCanonicalDatetimes`, so `canonicalDatetimeFields` stays empty, + * `needsLegacyDatetimeRepair` is true, and every comparison on that column is + * compiled by `applyNormalizedComparison`'s `whereRaw` arms instead. + * + * Measured on `main` before this change, on the row below: `$in` → 0 rows, + * `$nin` → the excluded row, `$eq` and bare equality → 0 rows. Identical + * defect, different emitter. A gate installed in only one family leaves this + * cell green while the bug is still live, which is exactly what this block + * exists to detect. + */ +describe('[#7398] the second lowering family — normalised columns', () => { + let driver: SqlDriver; + const WHEN = '2026-01-01T00:00:00.000Z'; + + beforeAll(async () => { + driver = new SqlDriver({ + client: 'better-sqlite3', + connection: { filename: ':memory:' }, + useNullAsDefault: true, + }); + const knex = (driver as any).knex; + await knex.schema.createTable('ext_sprint', (t: any) => { + t.string('id').primary(); + t.string('milestones'); + }); + await knex('ext_sprint').insert([{ id: 's1', milestones: JSON.stringify([WHEN]) }]); + (driver as any).registerExternalObject({ + name: 'ext_sprint', + fields: { + id: { type: 'text', name: 'id' }, + milestones: { type: 'datetime', name: 'milestones', multiple: true }, + }, + }); + }); + + afterAll(async () => { + await driver?.disconnect?.(); + }); + + it('this column really is served by the normalised family — the control\'s own premise', () => { + // If this ever returns `null`, the cell has silently become a copy of the + // block above and stops controlling anything. + expect((driver as any).filterColumnExpr('ext_sprint', 'milestones', 'milestones')).not.toBeNull(); + expect((driver as any).isJsonColumn('ext_sprint', 'milestones')).toBe(true); + }); + + for (const [op, comparand] of [ + ['$in', [WHEN]], + ['$nin', [WHEN]], + ['$eq', WHEN], + ['$ne', WHEN], + ['$gt', WHEN], + ['$between', [WHEN, WHEN]], + ] as ReadonlyArray) { + it(`refuses "${op}" on the normalised JSON column`, async () => { + const err = await refusalOf(() => + driver.find('ext_sprint', { where: { milestones: { [op]: comparand } } as FilterCondition }), + ); + expectJsonColumnRefusal(err, op, 'milestones'); + }); + } + + it('refuses bare equality on the normalised JSON column', async () => { + const err = await refusalOf(() => driver.find('ext_sprint', { where: { milestones: WHEN } as FilterCondition })); + expectJsonColumnRefusal(err, '=', 'milestones'); + }); + + it('$contains still works there too', async () => { + const rows = await driver.find('ext_sprint', { where: { milestones: { $contains: WHEN } } as FilterCondition }); + expect(rows.map((r: any) => r.id)).toEqual(['s1']); + }); +}); diff --git a/packages/drivers/driver-sql/src/sql-driver.ts b/packages/drivers/driver-sql/src/sql-driver.ts index 543f3cc91a..a055740227 100644 --- a/packages/drivers/driver-sql/src/sql-driver.ts +++ b/packages/drivers/driver-sql/src/sql-driver.ts @@ -1189,6 +1189,95 @@ function unrenderableTextComparandError(field: string, op: string, value: unknow ); } +/** + * [#7398] Operators whose SQL lowering compares a column's STORED SCALAR to a + * value — every spelling either of this driver's two comparison emitters + * answers ({@link SqlDriver.applyFilterCondition}'s plain-column switch and + * {@link SqlDriver.applyNormalizedComparison}'s normalised arms). + * + * The bare infix forms are here for the same reason they are in + * {@link SCALAR_COMPARAND_OPERATORS}: `applyNormalizedComparison` really does + * answer `in` / `nin` / `not_in` / `notin` / `=` / `<>` / `>` …, so a filter + * spelled that way against a normalised column compiles, and a gate that only + * knew the `$`-forms would leave the failure alive at a different spelling — + * the lesson #5234 already paid for in this file. + * + * `$between` is included although the card's minimum set stopped at the four + * ordering comparisons: it IS `>= AND <=` (this driver even decomposes a + * calendar-day `$between` into `$gte`/`$lt` a few lines above the emitter), so + * refusing the halves and compiling the compound would be the same wrong answer + * at one more spelling. + * + * Deliberately ABSENT, and this is the load-bearing half of the set: the `LIKE` + * family (`$contains`, `$notContains`, `$startsWith`, `$endsWith`, + * `$icontains`) and the null predicates (`$null`, `$exists`). `$contains` is + * the ONLY working membership spelling on a JSON-array column and downstream + * code depends on it (#7398's own tables), while `IS NULL` asks about the + * column's presence, which is a well-formed question whatever the column holds. + */ +const JSON_COLUMN_INCOMPATIBLE_OPERATORS: ReadonlySet = new Set([ + '$eq', '=', '==', + '$ne', '!=', '<>', + '$gt', '>', '$gte', '>=', '$lt', '<', '$lte', '<=', + '$in', 'in', + '$nin', 'nin', 'not_in', 'notin', + '$between', 'between', +]); + +/** + * [#7398] A scalar-comparison operator met a column this driver stores as JSON + * TEXT, so the comparison can never mean what the caller wrote. + * + * The mechanism is one line of SQL. A `multiple: true` field is stored as the + * serialization `["U1","U2"]`, so `members in ('U1')` is FALSE — the text + * genuinely is not equal to that id — and `members not in ('U1')` is TRUE. The + * measured consequences on the issue's fixture, one row whose `members` + * contains `U1`: + * + * - `$in` / `$eq` / bare equality → **0 rows**, fail-CLOSED. Silent, and a + * `200` with an empty array is byte-identical to a query that legitimately + * matched nothing, so no caller has anything to key on. + * - `$nin` / `$ne` → **the row it was asked to exclude**, fail-OPEN. That is + * the dangerous half and the reason this is a refusal rather than a + * documented footgun: an exclusion that silently stops excluding WIDENS a + * result set, the direction #3948 / #4209 / #5347 all ruled outranks a + * narrowing one. The issue's downstream delete-guard + * (`{ assignees: { $in: memberIds } }`) never fired once since it shipped. + * - The ordering comparisons are not even uniformly empty: `$lte` matched, + * because `["usr_…"` sorts below `usr_…` on the leading `[`. A lexicographic + * compare over a serialization is a wrong answer, not a narrow one. + * + * ADR-0112 class 1 — a caller mistake, so `INVALID_FILTER` / 400, the same + * envelope {@link unsupportedFilterError} gives the unknown-operator refusal + * one arm over. The message states the filter WAS NOT APPLIED for the reason + * that wording exists on the comparand-shape refusals: "no rows" is a + * legitimate answer to a legitimate query, so a caller must be told that this + * one was never asked. + * + * The prescription is `$contains` (and an `$or` of `$contains` for any-of) + * because that is the only spelling that works today — it lowers to + * `LIKE '%v%'` over the serialization. That it works at all is incidental + * rather than designed, which is the issue's ask 2 (`$overlaps` / + * `$containsAny`); ask 2 is a closed-spec-set question and is deliberately not + * answered here. + */ +function jsonColumnOperatorError(field: string, op: string, bare: boolean): Error { + const spelling = bare + ? `The bare equality spelling { "${field}": value }` + : `Operator "${op}"`; + const on = bare ? '' : ` on field "${field}"`; + return unsupportedFilterError( + `${spelling}${on} WAS NOT APPLIED: "${field}" is a multi-value (or otherwise JSON-valued) ` + + `field, stored by this driver as a JSON TEXT column (e.g. ["a","b"]), and "${op}" compares ` + + `that whole serialized text against a single value — it can never equal one member. ` + + `Use "$contains" for membership ({ "${field}": { "$contains": "a" } }), or an $or of ` + + `"$contains" for any-of ({ "$or": [{ "${field}": { "$contains": "a" } }, ` + + `{ "${field}": { "$contains": "b" } }] }). Refused rather than compiled because the answer ` + + `was silently wrong in BOTH directions: $in/$eq matched nothing, while $nin/$ne returned ` + + `the very rows they were asked to exclude (#7398).`, + ); +} + /** A short, non-throwing rendering of an offending comparand for the message. */ function safeShapePreview(value: unknown): string { try { @@ -7963,6 +8052,68 @@ export class SqlDriver implements IDataDriver { return columnSql; } + /** + * [#7398] Is `localField` stored as a JSON TEXT column on `table`? + * + * Reads `jsonFields` — the per-table registry both `initObjects` and + * `registerExternalObject` fill from {@link SqlDriver.isJsonField}, i.e. + * `JSON_COLUMN_TYPES.has(type) || !!field.multiple`. Asking THAT registry + * rather than growing a second one is the whole point: `JSON_COLUMN_TYPES` + * already carries a header calling itself the single source for the DDL + * column-type switch and `isJsonField` "so the two can't drift", and a + * filter-side copy of "which columns are JSON" would be a third list to keep + * in step with the first two. + * + * It answers on the LOCAL field name, which is what the registry is keyed by + * (both writers enumerate `obj.fields`), not the possibly-remapped remote + * column an external `columnMap` produces. + * + * **A table with no entry answers `false`, deliberately.** A table this + * driver was never told about — one created straight through knex, as the + * conformance sweeps do — has no field types to consult, and inventing a + * refusal from an absent registry would refuse filters on ordinary columns. + * The gate fires only where the column type is KNOWN to be JSON. + */ + protected isJsonColumn(table: string | null | undefined, localField: string): boolean { + if (!table) return false; + return this.jsonFields[table]?.includes(localField) === true; + } + + /** + * [#7398] The column-type half of the filter gate: refuse a DECLARED operator + * that the column it was aimed at cannot give a meaningful answer for. + * + * The sibling of {@link assertCompilableComparand}, and called from the same + * three positions, one per lowering entry: the operator-object branch of + * {@link SqlDriver.applyFilterCondition}, that method's bare-value branch, + * and the plain `{ field: value }` loop in {@link SqlDriver.applyFilters}. + * The two gates ask different questions of the same comparison — that one + * asks whether the COMPARAND can be bound, this one whether the COLUMN can be + * compared — and #7398 is the square of that grid that had nothing on it. + * + * Placed BEFORE the calendar-day rewrites and before + * {@link SqlDriver.applyNormalizedComparison}, because a JSON column reaches + * BOTH emitters and only one of them is the site the issue named. A + * `multiple: true` datetime field on an external object (ADR-0015) has a + * `filterColumnExpr` — `registerExternalObject` never marks its datetime + * columns canonical, so `needsLegacyDatetimeRepair` stays true — and is + * therefore served by the normalised `whereRaw` arms, not by the plain + * `whereIn` / `where(field, op, v)` arms a managed `multiple: true` lookup + * goes through. Measured on both before this gate existed: `$in` → 0 rows, + * `$nin` → the excluded row, on each. + */ + protected assertOperatorAppliesToColumn( + table: string | null | undefined, + localField: string, + column: string, + op: string, + bare = false, + ): void { + if (!JSON_COLUMN_INCOMPATIBLE_OPERATORS.has(op)) return; + if (!this.isJsonColumn(table, localField)) return; + throw jsonColumnOperatorError(column, op, bare); + } + protected applyFilters(builder: Knex.QueryBuilder, filters: any) { if (!filters) return; @@ -8025,6 +8176,10 @@ export class SqlDriver implements IDataDriver { // #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); + // #7398 — and the column-type question the comparand gate cannot ask. + // This loop IS the bare `{ field: value }` spelling, one of the four + // the issue measured as silently answering zero rows on an array column. + this.assertOperatorAppliesToColumn(table, key, column, '=', true); const coerced = this.coerceFilterValue(table, key, value); const expr = this.filterColumnExpr(table, key, column); if (expr && this.applyNormalizedComparison(builder, 'and', expr, '=', coerced)) continue; @@ -8305,6 +8460,11 @@ export class SqlDriver implements IDataDriver { // BEFORE any rewrite or coercion touches it, so the message names the // shape the caller actually sent. assertCompilableComparand(field, rawOp, opValue); + // #7398 — the column-type gate, on the operator AS WRITTEN and ahead + // of every rewrite and both emitters, so a JSON column cannot reach + // the plain `whereIn` arms below NOR the normalised `whereRaw` arms + // an external multi-value datetime column is routed to. + this.assertOperatorAppliesToColumn(table, localField, field, rawOp); // Calendar-day upper bounds first (#3777): `$lte` on a bare // `YYYY-MM-DD` against a datetime column compiles half-open, and a // `$between` whose max is a bare day decomposes into the same pair — @@ -8453,6 +8613,11 @@ export class SqlDriver implements IDataDriver { const localField = this.mapSortField(key); const field = this.remoteColumn(table, key, localField); const method = logicalOp === 'or' ? 'orWhere' : 'where'; + // #7398 — the bare `{ field: value }` spelling again, reached when the + // condition carries an operator SOMEWHERE (so `applyFilters` routed the + // whole node here) but not on this key. Same compilation, same gate: + // one condition must not have two verdicts depending on its siblings. + this.assertOperatorAppliesToColumn(table, localField, field, '=', true); const coerced = this.coerceFilterValue(table, localField, value); const columnExpr = this.filterColumnExpr(table, localField, field); if (columnExpr && this.applyNormalizedComparison(builder, logicalOp, columnExpr, '=', coerced)) continue; From 4acc2cd9d7ac733e4269b0062ec90f5ae70b401c Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 10 Aug 2026 10:57:44 +0000 Subject: [PATCH 2/2] test(driver-sql): type the aggregate face's query bag instead of casting it `check:query-options-erasure` counts an `as any` at the options position of find/findOne/count/aggregate, and the new refusal sweep raised the test surface 249 -> 250. The cast was gratuitous: `aggregations` is on `DriverQuery`, so the call types as written once the entry carries its `field`. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_011GCuQuqxKvWLYUGss7CXdc --- .../src/sql-driver-json-column-operator-refusal.test.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/packages/drivers/driver-sql/src/sql-driver-json-column-operator-refusal.test.ts b/packages/drivers/driver-sql/src/sql-driver-json-column-operator-refusal.test.ts index ff185edb98..c251c08398 100644 --- a/packages/drivers/driver-sql/src/sql-driver-json-column-operator-refusal.test.ts +++ b/packages/drivers/driver-sql/src/sql-driver-json-column-operator-refusal.test.ts @@ -318,7 +318,10 @@ describe('[#7398] SqlDriver refuses scalar-comparison operators on JSON/multi-va [ 'aggregate', (where) => - driver.aggregate('team', { where, aggregations: [{ function: 'count', alias: 'n' }] } as any), + driver.aggregate('team', { + where, + aggregations: [{ function: 'count', field: '*', alias: 'n' }], + }), ], ['distinct', (where) => driver.distinct('team', 'name', where)], ['updateMany', (where) => driver.updateMany('team', { where }, { name: 'Beta' })],