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..c251c08398 --- /dev/null +++ b/packages/drivers/driver-sql/src/sql-driver-json-column-operator-refusal.test.ts @@ -0,0 +1,503 @@ +// 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', field: '*', alias: 'n' }], + }), + ], + ['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;