diff --git a/.changeset/silly-jars-shake.md b/.changeset/silly-jars-shake.md new file mode 100644 index 0000000000..4e96e17d49 --- /dev/null +++ b/.changeset/silly-jars-shake.md @@ -0,0 +1,14 @@ +--- +'@objectstack/objectql': patch +--- + +`having` now REFUSES an `$icontains` comparand that is not a non-empty string, instead of evaluating it + +**Client-visible effect — two `having` filters that used to return rows now return a 400.** Both were answering the wrong thing silently: + +- `having: { name: { $icontains: '' } }` matched **every** row. Every string contains the empty substring, so the predicate constrained nothing: the author wrote a constraint and got the UNFILTERED aggregate back with no error. A predicate that constrains nothing does not narrow a result set, it widens it — on a row-level-security read scope that is a permission bypass rather than a degraded filter. +- `having: { name: { $icontains: 42 } }` matched **no** rows. `StringOperatorSchema` declares `$icontains: z.string()`, so a non-string comparand was answered `false` rather than refused — the silent-wrong-answer shape `$regex` was retired over. + +Both are now refused with `INVALID_FILTER` / HTTP 400 in the ADR-0112 envelope, naming the field and its position inside the clause (`having.$and[0].name.$icontains`). This is the gate the five sibling filter faces already had (`driver-memory` and `driver-sql`'s `icontainsComparandError`, and their twins); `having` was the sixth evaluation face of the same vocabulary and the only one without it. + +Nothing else about `having` changes: every filter it evaluated correctly before, including every `$icontains` with a real comparand, is evaluated identically. Callers writing either of the two degenerate comparands should write the substring they meant to search for, or drop the predicate. diff --git a/packages/objectql/src/having-filter-text-conformance.test.ts b/packages/objectql/src/having-filter-text-conformance.test.ts index 69010ea633..6a8756caf2 100644 --- a/packages/objectql/src/having-filter-text-conformance.test.ts +++ b/packages/objectql/src/having-filter-text-conformance.test.ts @@ -21,41 +21,39 @@ * found by a human-run census (#6993) rather than by CI, twice. This file is * the coverage, so the five faces cannot drift apart again silently. * - * ## Two rejection rows are DELIBERATELY not enrolled + * ## [#7158] ALL FIVE rejection rows are enrolled now * - * The table's five rejection rows split three/two against what this face does - * today, measured by execution on `origin/main` @ `3e8e669` before this card's - * change: + * When #7047 wrote this file the table's five rejection rows split three/two + * against what this face did, measured by execution on `origin/main` @ + * `3e8e669`: * - * | row | `having` today | - * |:--|:--| - * | `$regex` refused | refused (uncoded before this card) | - * | `$regex` + `$options` refused as one mistake | refused (uncoded before this card) | - * | dangling `$options` refused | refused (uncoded before this card) | - * | **empty `$icontains` comparand refused** | **NOT refused — matched ALL NINE rows** | - * | **non-string `$icontains` comparand refused** | **NOT refused — matched none** | + * | row | `having` under #7047 | `having` now (#7158) | + * |:--|:--|:--| + * | `$regex` refused | refused, enveloped by #7047 | unchanged | + * | `$regex` + `$options` refused as one mistake | refused, enveloped by #7047 | unchanged | + * | dangling `$options` refused | refused, enveloped by #7047 | unchanged | + * | **empty `$icontains` comparand refused** | **NOT refused — matched ALL NINE rows** | **refused** | + * | **non-string `$icontains` comparand refused** | **NOT refused — matched none** | **refused** | * - * The first three are this card: the refusal was already correct and only the - * envelope was missing, so they are enrolled and are what - * {@link RETIRED_REJECTION_CASES} drives. + * The first three were #7047's: the refusal was already correct and only the + * envelope was missing. The last two were a DIFFERENT defect — a comparand-shape + * gate this face had never had, which the driver faces get from `driver-memory`'s + * `icontainsComparandError` and `driver-sql`'s twin — and closing it meant + * REFUSING filters this face used to evaluate, a behaviour change that took its + * own card and its own changeset (the same fence #6993 kept). That card is + * #7158, and `having-filter.ts`'s `icontainsComparandError` is its landing. * - * The last two are a DIFFERENT defect — a comparand-shape gate this face has - * never had, which the driver faces get from `driver-memory`'s - * `icontainsComparandError` and `driver-sql`'s twin. Closing it means REFUSING - * filters this face evaluates today, which is a behaviour change beyond an - * envelope card and belongs in its own PR with its own changeset (the same - * fence #6993 kept, and the reason this card exists separately from it). Note - * the empty-comparand row in particular: matching all nine rows is a predicate - * that constrains NOTHING, i.e. the WIDENING that is a permission bypass rather - * than a degraded filter on an RLS read scope (#3948). - * - * They are named here rather than filtered out silently, and pinned by - * {@link UNENROLLED_REJECTION_CASES} below, so the exclusion is a measured - * statement that goes RED when it stops being true — not a gap that reads as - * coverage. Tracked as #7158. + * #7047 did not filter those two rows out silently: it PINNED them as + * measurements — asserting that the face did not refuse them, and that the empty + * comparand matched all nine rows — precisely so that adding the gate would go + * RED and force them into the enrolment rather than leaving a gap that reads as + * coverage. It did. {@link REJECTION_CASES} is now every rejection row the table + * declares, and the flipped measurements are kept below rather than deleted, in + * their new direction: the two expressions that used to answer now refuse. * * @see FILTER_TEXT_CASES — the standard - * @see https://github.com/objectstack-ai/objectstack/issues/7047 (this card) + * @see https://github.com/objectstack-ai/objectstack/issues/7047 (the envelope card, which wrote this file) + * @see https://github.com/objectstack-ai/objectstack/issues/7158 (the comparand gate) * @see https://github.com/objectstack-ai/objectstack/issues/6993 (the five-face census) * @see https://github.com/objectstack-ai/objectstack/issues/5324 (the envelope half) */ @@ -69,26 +67,51 @@ import { } from '@objectstack/spec/data'; import { applyHaving, matchesHaving } from './having-filter.js'; -/** The rejection rows whose refusal this face already makes. */ -const RETIRED_REJECTION_SPELLINGS = ['$regex', '$options'] as const; - function isRejection(c: FilterTextCase): c is FilterTextRejectionCase { return c.expectRejection === true; } -/** A rejection row is this card's iff its filter names a RETIRED operator. */ -function namesRetiredOperator(c: FilterTextRejectionCase): boolean { - const constraints = Object.values(c.filter as Record); - return constraints.some((spec) => - !!spec - && typeof spec === 'object' - && Object.keys(spec).some((op) => (RETIRED_REJECTION_SPELLINGS as readonly string[]).includes(op))); -} - const ROWS_CASES = FILTER_TEXT_CASES.filter((c): c is Exclude => !isRejection(c)); -const RETIRED_REJECTION_CASES = FILTER_TEXT_CASES.filter(isRejection).filter(namesRetiredOperator); -const UNENROLLED_REJECTION_CASES = FILTER_TEXT_CASES.filter(isRejection).filter((c) => !namesRetiredOperator(c)); + +/** + * [#7158] EVERY rejection row the table declares — no `namesRetiredOperator` + * partition any more, because the two families it separated are both refused by + * this face now. The partition existed to name an EXCLUSION; with nothing + * excluded, keeping it would mean re-deriving a distinction the enrolment no + * longer makes. + */ +const REJECTION_CASES = FILTER_TEXT_CASES.filter(isRejection); + +/** + * A rejection row's refusal, asserted on the ADR-0112 envelope rather than on + * "it threw". + * + * `toThrow()` alone carries one bit where the defect has two (#6142/#6050) — it + * is exactly what stayed green through #7047's missing `code`/`status`, and what + * would stay green again if this face grew a second error shape for the same + * mistake. + */ +function expectRefusal( + run: () => unknown, + testCase: FilterTextRejectionCase, +): void { + let err: (Error & { code?: string; status?: number }) | undefined; + try { + run(); + } catch (e) { + err = e as Error & { code?: string; status?: number }; + } + + // Not `expected: []`. The whole reason `FilterTextRejectionCase` is a separate + // discriminant is that "returned no rows" and "refused to run" must be told + // apart — answering zero rows is the silent wrong answer #4706 retired the + // operator over, and (for the comparand rows) the very shape #7158 closed. + expect(err, testCase.note ?? 'expected a refusal').toBeInstanceOf(Error); + expect(err!.code).toBe(testCase.code); + expect(err!.status).toBe(400); + for (const mention of testCase.mustMention) expect(err!.message).toContain(mention); +} /** * The fixture rows are `{ id, name }`, which is exactly the shape of an @@ -111,70 +134,84 @@ describe('[#7047] `having` answers FILTER_TEXT_CASES — the evaluated rows', () } }); -describe('[#7047] `having` answers FILTER_TEXT_CASES — the retired-operator refusals', () => { - for (const testCase of RETIRED_REJECTION_CASES) { +describe('[#7047/#7158] `having` answers FILTER_TEXT_CASES — every rejection row', () => { + for (const testCase of REJECTION_CASES) { it(`${testCase.name} — refused in the ADR-0112 envelope`, () => { - let err: (Error & { code?: string; status?: number }) | undefined; - try { - applyHaving(AGGREGATED_ROWS, testCase.filter); - } catch (e) { - err = e as Error & { code?: string; status?: number }; - } - - // Not `expected: []`. The whole reason `FilterTextRejectionCase` is a - // separate discriminant is that "returned no rows" and "refused to run" - // must be told apart — answering zero rows is the silent wrong answer - // #4706 retired the operator over. - expect(err, testCase.note ?? 'expected a refusal').toBeInstanceOf(Error); - - // The `code` half — the field this card adds and the reason a - // throw-only assertion stayed green through the defect. - expect(err!.code).toBe(testCase.code); - expect(err!.status).toBe(400); - - for (const mention of testCase.mustMention) expect(err!.message).toContain(mention); + expectRefusal(() => applyHaving(AGGREGATED_ROWS, testCase.filter), testCase); }); } - it('drives every retired-operator rejection row the table declares', () => { - // A guard on the filters above: if a retired spelling joins - // `RETIRED_FILTER_OPERATORS` and the table, this count moves and the - // enrolment is re-read rather than silently skipping the new row. - expect(RETIRED_REJECTION_CASES.map((c) => c.name)).toEqual([ + it('drives every rejection row the table declares', () => { + // A guard on the filter above: if a rejection row joins the table — a new + // retirement, or a comparand shape some face stops evaluating — this list + // moves and the enrolment is re-read rather than silently skipping it. + // + // [#7158] The last two rows are the ones #7047 could not enrol. They are IN + // this list now, which is the single assertion that says the exclusion is + // over: five declared, five driven. + expect(REJECTION_CASES.map((c) => c.name)).toEqual([ '$regex is REFUSED, and the refusal names $icontains', '$regex with $options is REFUSED as one mistake, not two', 'a dangling $options with no $regex is REFUSED', + 'an empty $icontains comparand is REFUSED', + 'a non-string $icontains comparand is REFUSED', ]); }); }); /** - * [#7047] The exclusion, stated as a measurement rather than as prose. + * [#7158] The two measurements #7047 pinned, kept and FLIPPED. + * + * These are the same four expressions that block asserted, in their new + * direction. They are not folded into the enrolment above because they say + * something the table-driven rows cannot: the enrolment asserts that a REJECTION + * ROW is refused, while these assert that the two specific behaviours the defect + * consisted of — matching every row, and answering "no rows" — are gone at the + * `matchesHaving` entry point as well as at `applyHaving`. * - * These two rows are NOT enrolled above (see the module note). This block pins - * WHY — the face does not refuse them at all — so the day someone adds the - * comparand gate, this test goes red and forces the rows into the enrolment - * instead of leaving them permanently excluded by a comment nobody re-reads. - * Tracked as #7158. + * The empty-comparand half is the sharp one: it is not a filter that returned + * the wrong rows, it is a filter that returned the UNFILTERED aggregate (#3948), + * which on an RLS read scope is a permission bypass rather than a degraded + * filter. */ -describe('[#7047] the two comparand-shape rejection rows this face does NOT yet refuse (#7158)', () => { - it('names exactly the two rows left out of the enrolment', () => { - expect(UNENROLLED_REJECTION_CASES.map((c) => c.name)).toEqual([ - 'an empty $icontains comparand is REFUSED', - 'a non-string $icontains comparand is REFUSED', - ]); +describe('[#7158] the two comparand-shape measurements, in their post-gate direction', () => { + const EMPTY_COMPARAND_CASE = REJECTION_CASES.find( + (c) => c.name === 'an empty $icontains comparand is REFUSED')!; + const NON_STRING_COMPARAND_CASE = REJECTION_CASES.find( + (c) => c.name === 'a non-string $icontains comparand is REFUSED')!; + + it('an empty $icontains comparand is REFUSED — it no longer matches every row', () => { + expectRefusal( + () => matchesHaving({ name: 'ACME Corp' }, { name: { $icontains: '' } }), + EMPTY_COMPARAND_CASE); + expectRefusal( + () => applyHaving(AGGREGATED_ROWS, { name: { $icontains: '' } }), + EMPTY_COMPARAND_CASE); }); - it('an empty $icontains comparand is EVALUATED, and matches every row — the widening', () => { - // The sharp one: a predicate that constrains nothing does not narrow a - // query, it WIDENS it (#3948). Measured, not predicted. - expect(matchesHaving({ name: 'ACME Corp' }, { name: { $icontains: '' } })).toBe(true); - expect(applyHaving(AGGREGATED_ROWS, { name: { $icontains: '' } })).toHaveLength( - AGGREGATED_ROWS.length); + it('a non-string $icontains comparand is REFUSED — it no longer answers "no rows"', () => { + expectRefusal( + () => matchesHaving({ name: 'ACME Corp' }, { name: { $icontains: 42 } as never }), + NON_STRING_COMPARAND_CASE); + expectRefusal( + () => applyHaving(AGGREGATED_ROWS, { name: { $icontains: 42 } as never }), + NON_STRING_COMPARAND_CASE); }); - it('a non-string $icontains comparand is EVALUATED as "no rows" rather than refused', () => { - expect(matchesHaving({ name: 'ACME Corp' }, { name: { $icontains: 42 } as never })).toBe(false); - expect(applyHaving(AGGREGATED_ROWS, { name: { $icontains: 42 } as never })).toHaveLength(0); + /** + * The refusal NAMES the position, which is what makes it actionable on a face + * whose clause nests. `having.$and[0].name.$icontains` tells its reader which + * branch of which clause to fix; `having` alone would not. + */ + it('names the field and its position inside the `having` clause', () => { + const err = (() => { + try { + applyHaving(AGGREGATED_ROWS, { $and: [{ name: { $icontains: '' } }] }); + return null; + } catch (e) { return e as Error; } + })(); + expect(err).toBeInstanceOf(Error); + expect(err!.message).toContain('on field "name"'); + expect(err!.message).toContain('at having.$and[0].name.$icontains'); }); }); diff --git a/packages/objectql/src/having-filter.ts b/packages/objectql/src/having-filter.ts index fb52785f5e..144f6b8f2f 100644 --- a/packages/objectql/src/having-filter.ts +++ b/packages/objectql/src/having-filter.ts @@ -40,6 +40,10 @@ // driver-memory / driver-mongodb still answer the old way only because // #5499 freezes them; the divergence is against a frozen face, not against // the ruling. +// +// [#7158] A THIRD divergence has been REMOVED rather than added: this face had +// no comparand-shape gate, which is what the five sibling faces refuse an +// unevaluable `$icontains` comparand with. See {@link icontainsComparandError}. import type { FilterCondition } from '@objectstack/spec/data'; // [#5702] The retired operators and the prescription a refusal prints. HAVING is @@ -142,6 +146,51 @@ function unknownOperator( ); } +/** + * [#7158] `$icontains` received a comparand that is not a non-empty string. + * + * ## Two rejections, one constructor + * + * Word for word `driver-memory`'s `icontainsComparandError` (`filter-refusal.ts`) + * and `driver-sql`'s twin of it — deliberately, because they are ONE mistake at + * ONE position and #5240's rule (one condition keeps one wording) applies across + * packages, not only within one: + * + * - **non-string** — `StringOperatorSchema` declares `$icontains: z.string()`, + * so coercing `42` to `"42"` would answer a query nobody wrote. Evaluated + * here, it answered `false` — "no rows" — which is the silent-wrong-answer + * shape #4706 retired `$regex` over. + * - **empty string** — every string contains the empty substring, so the + * predicate constrains nothing. A predicate that constrains nothing does not + * narrow a query, it WIDENS it (#3948), and on an RLS read scope that is a + * permission bypass rather than a degraded filter. Evaluated here, it matched + * ALL NINE `FILTER_TEXT_ROWS` — the author wrote a constraint and got the + * unfiltered aggregate back, with no error. + * + * ## Why this face is the last to get the gate + * + * HAVING is the SIXTH JS evaluation face of one filter vocabulary (#6520 gave it + * the shared ASCII fold) and was the only one with no comparand gate at all, + * because it is the one face no conformance table drove until #7047 wrote + * `having-filter-text-conformance.test.ts`. That file pinned the two exclusions + * as measurements so the gap was red-on-change; this is the change. + * + * The `at ${path}` position is spelled from the `having` root (`having.total`, + * `having.$and[0].name`) where the driver faces spell it from `where` — the + * clause is the difference, and a caller reading a 400 needs to know which of + * the two they mis-wrote. Everything after the position is verbatim. + */ +function icontainsComparandError(field: string, value: unknown, path: string): Error { + const shown = typeof value === 'string' ? `""` : JSON.stringify(value) ?? String(value); + return invalidFilterError( + `Operator "$icontains" on field "${field}" at ${path} requires a NON-EMPTY string comparand, ` + + `received ${shown}. "$icontains" is a case-insensitive LITERAL substring search, so its ` + + `comparand is the text to look for — an empty one matches every row (a predicate that ` + + `constrains nothing), and a non-string one would have to be coerced into text this query ` + + `never asked for.`, + ); +} + /** * [#5905] Operators whose answer for a column with NO VALUE is decided by the * operator's own arm below, not by the early exit in {@link checkCondition}. @@ -173,34 +222,42 @@ export function applyHaving(rows: any[], having: FilterCondition | null | undefi return rows.filter((row) => matchesHaving(row, having)); } -/** Evaluate one aggregated row against a HAVING FilterCondition. */ -export function matchesHaving(row: Record, cond: any): boolean { +/** + * Evaluate one aggregated row against a HAVING FilterCondition. + * + * [#7158] `path` is the position of `cond` inside the clause the caller wrote — + * `having`, `having.$and[0]`, `having.$not` — carried down so a comparand + * refusal can NAME where the offending key sits. It defaults, so this stays the + * two-argument function every existing caller (and `applyHaving` below) uses. + */ +export function matchesHaving(row: Record, cond: any, path = 'having'): boolean { if (!cond || typeof cond !== 'object') return true; for (const [key, value] of Object.entries(cond)) { + const here = `${path}.${key}`; if (key === '$and') { const branches = Array.isArray(value) ? value : [value]; - if (!branches.every((c) => matchesHaving(row, c))) return false; + if (!branches.every((c, i) => matchesHaving(row, c, `${here}[${i}]`))) return false; continue; } if (key === '$or') { const branches = Array.isArray(value) ? value : [value]; - if (!branches.some((c) => matchesHaving(row, c))) return false; + if (!branches.some((c, i) => matchesHaving(row, c, `${here}[${i}]`))) return false; continue; } if (key === '$not') { - if (matchesHaving(row, value)) return false; + if (matchesHaving(row, value, here)) return false; continue; } if (key.startsWith('$')) throw unknownOperator(key, 'logical'); // Aggregated rows are flat (aliases + group projections) — direct access, // no dotted-path resolution. - if (!checkCondition(row?.[key], value)) return false; + if (!checkCondition(row?.[key], value, key, here)) return false; } return true; } /** One column's condition — implicit equality or an operator object. */ -function checkCondition(value: any, condition: any): boolean { +function checkCondition(value: any, condition: any, field: string, path: string): boolean { // Implicit equality (primitives, null, Date, array exact-match) — loose `==` // to mirror the Filter Protocol's memory evaluation. if ( @@ -228,6 +285,15 @@ function checkCondition(value: any, condition: any): boolean { // file refuses unknown operators to avoid. It now falls to `default:` and is // refused with the spec's prescription. const target = (condition as Record)[op]; + // [#7158] The comparand-shape gate, ABOVE the no-value exit on purpose. The + // shape of a comparand is a property of the FILTER, not of the row being + // judged: below the exit, `{ missing_column: { $icontains: '' } }` would be + // answered `false` for every row that lacks the column and refused only for + // the rows that carry it — one filter, two verdicts, decided by the data. + // See {@link icontainsComparandError}. + if (op === '$icontains' && (typeof target !== 'string' || target === '')) { + throw icontainsComparandError(field, target, `${path}.${op}`); + } if (value === undefined && !NO_VALUE_ANSWERED_BY_OPERATOR.has(op)) return false; switch (op) { // eslint-disable-next-line eqeqeq @@ -270,8 +336,18 @@ function checkCondition(value: any, condition: any): boolean { // contain a substring — and the same fold every other JS face uses, from // the spec, so an aggregate filtered HERE and the same predicate run as a // `where` on any driver select the same rows. + // + // [#7158] The `typeof target !== 'string'` limb this arm used to carry is + // GONE — not relaxed, HOISTED. It was the whole of this face's opinion + // about a bad comparand, and its opinion was `return false`: "no rows", + // silently, for a comparand the declared type does not permit. The gate + // above now refuses that input before the arm is reached, so the limb was + // dead code that read as a check. What survives here is the guard on the + // COLUMN VALUE, which is a different judgement and stays: a value that is + // not text cannot contain a substring, so it does not match — the same + // answer `$contains` gives, about the row rather than about the filter. case '$icontains': - if (typeof value !== 'string' || typeof target !== 'string' + if (typeof value !== 'string' || !asciiCaseInsensitiveContains(value, target)) return false; break; // [#5702] The `$regex` arm is GONE. It built `new RegExp(target, $options)`