diff --git a/.changeset/having-adr0112-envelope.md b/.changeset/having-adr0112-envelope.md new file mode 100644 index 0000000000..79f771609c --- /dev/null +++ b/.changeset/having-adr0112-envelope.md @@ -0,0 +1,56 @@ +--- +"@objectstack/objectql": patch +--- + +fix(objectql): put `having`'s operator refusals inside the ADR-0112 envelope (#7047) + +`having-filter.ts`'s `unknownOperator()` returned a bare `new Error(...)` from +**both** of its branches — the RETIRED spellings (`$regex`, `$options`) and the +unknown ones (`$nand`, `$median`, a mistyped `$icontain`) — so the thrown error +carried `code: undefined` and `status: undefined`. `rest` served it through the +unclassified-fault branch, and a **400-class author mistake reached the client +500-shaped**. + +This is the last of the five filter-refusal faces to join the envelope, and the +only one that disagreed. Measured by EXECUTING each face rather than by grep +(#6993), before and after: + +| face | `code` before | `code` after | +|:--|:--|:--| +| driver-sql, driver-sqlite-wasm, driver-turso (local + remote) | `INVALID_FILTER` / 400 | unchanged | +| driver-memory (`filter-refusal.ts`), driver-mongodb | `INVALID_FILTER` / 400 | unchanged | +| **objectql `having`** | **`undefined` / `undefined`** | **`INVALID_FILTER` / 400** | + +The refusal itself, and its message, are unchanged — the retired branch already +printed `RETIRED_FILTER_OPERATORS[op].why` verbatim like the four driver faces. +Only the envelope was missing, which is the half of #5324 that a refusal does +not fix on its own and the half `FilterTextRejectionCase.code` exists to pin. +The code is `INVALID_FILTER` because this joins the contract the other four +already speak; a caller swapping HAVING for a driver-side `where` must not have +to catch two shapes for one mistake. + +**Client-visible change.** Code catching a `having` refusal by message +substring, or branching on the absence of `err.code`, sees `INVALID_FILTER` / +400 where it previously saw an uncoded `Error`. Over HTTP the status moves from +500 to 400, which is the point of the change. + +Both `unknownOperator()` returns are covered, deliberately: enveloping only the +retired path would have left `{ $nand: [...] }` and every operator typo +arriving 500-shaped — the same defect, one operator name away, and the more +likely of the two to be typed. + +The envelope constructor is now shared with the package's other filter-refusal +site (`filter-comparand-shape.ts`'s `invalidFilterError`, exported for this) +rather than copied, so objectql's two refusal sites cannot answer one mistake +with two envelopes. + +Test coverage moved with it. The rejection assertions in `having-filter.test.ts` +were `toThrow(/message/)` only, which is green whether or not the error carries +an envelope (#6142/#6050) — that is how the defect survived the PR that wrote +those messages. They now pin `code` + `status` + the verbatim prescription, on +both branches and through `applyHaving`, the entry point the engine calls. A new +`having-filter-text-conformance.test.ts` drives this face against +`FILTER_TEXT_CASES` — the standard the driver suites answer — so the faces +cannot drift apart silently again; `having` had no conformance-table coverage at +all, which is why both of the last two defects on it (#5905, this one) were +found by a hand-run census rather than by CI. diff --git a/packages/objectql/src/filter-comparand-shape.ts b/packages/objectql/src/filter-comparand-shape.ts index 29a828f1f6..36c63da6a0 100644 --- a/packages/objectql/src/filter-comparand-shape.ts +++ b/packages/objectql/src/filter-comparand-shape.ts @@ -143,8 +143,21 @@ function shapePreview(value: unknown): string { return text.length > 60 ? `${text.slice(0, 59)}…` : text; } -/** The wire envelope every filter refusal in the platform already uses. */ -function invalidFilterError(message: string): Error { +/** + * The wire envelope every filter refusal in the platform already uses. + * + * [#7047] EXPORTED, because this package has a second filter-refusal site and a + * private second copy of these four lines is how the platform's refusal faces + * drifted in the first place. `having-filter.ts` threw a bare `new Error` for a + * retired or unknown operator — `code` and `status` both `undefined` — so a + * 400-class author error reached the client 500-shaped, on the ONE refusal face + * of five that no conformance table drove. It calls this now, so the two + * objectql refusal sites cannot answer one mistake with two envelopes. + * + * The twin outside this package is `driver-memory`'s `unsupportedFilterError` + * (`filter-refusal.ts`), which carries the cross-driver rationale. + */ +export function invalidFilterError(message: string): Error { const err = new Error(message) as Error & { code?: string; status?: number }; err.code = StandardErrorCode.enum.INVALID_FILTER; err.status = 400; diff --git a/packages/objectql/src/having-filter-text-conformance.test.ts b/packages/objectql/src/having-filter-text-conformance.test.ts new file mode 100644 index 0000000000..69010ea633 --- /dev/null +++ b/packages/objectql/src/having-filter-text-conformance.test.ts @@ -0,0 +1,180 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#7047] `having` held to `FILTER_TEXT_CASES` — the standard the driver faces + * answer, driven against the SIXTH text-operator face. + * + * ## Why this file exists at all + * + * `having` is the one refusal face with no conformance-table coverage, and both + * of the last two defects on it say the same thing about that gap: + * + * - #5905: HAVING was the lone holdout on #5298's NULL-safety ruling, and "no + * conformance table would have caught it, because `FILTER_LOGIC_CASES` does + * not drive the HAVING path". + * - #7047 (this card): HAVING was the lone face whose refusals carried no + * ADR-0112 envelope, while `FilterTextRejectionCase.code` — the field that + * pins exactly that — sat in a table this package imported nowhere. + * + * `scripts/check-driver-conformance.mjs` scopes its ledger to + * `packages/drivers/*`, so no gate was ever going to notice. Both defects were + * 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 + * + * 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: + * + * | 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** | + * + * 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 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. + * + * @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/6993 (the five-face census) + * @see https://github.com/objectstack-ai/objectstack/issues/5324 (the envelope half) + */ + +import { describe, it, expect } from 'vitest'; +import { + FILTER_TEXT_CASES, + FILTER_TEXT_ROWS, + type FilterTextCase, + type FilterTextRejectionCase, +} 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)); + +/** + * The fixture rows are `{ id, name }`, which is exactly the shape of an + * aggregated row this face filters — a groupBy projection (`name`) beside an + * identifier. No adaptation needed, which is the point: HAVING's namespace is + * the aggregated row's own columns, so one text predicate must select the same + * rows here as it does in a driver's `where`. + */ +const AGGREGATED_ROWS = FILTER_TEXT_ROWS.map((row) => ({ ...row })); + +describe('[#7047] `having` answers FILTER_TEXT_CASES — the evaluated rows', () => { + // The precondition for the refusal half meaning anything. If this face + // selected different rows from the driver faces, agreeing on the shape of a + // refusal would be agreement about the wrong thing. + for (const testCase of ROWS_CASES) { + it(testCase.name, () => { + const ids = applyHaving(AGGREGATED_ROWS, testCase.filter).map((row) => row.id); + expect(ids, testCase.note).toEqual([...testCase.expected]); + }); + } +}); + +describe('[#7047] `having` answers FILTER_TEXT_CASES — the retired-operator refusals', () => { + for (const testCase of RETIRED_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); + }); + } + + 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([ + '$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', + ]); + }); +}); + +/** + * [#7047] The exclusion, stated as a measurement rather than as prose. + * + * 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. + */ +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', + ]); + }); + + 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 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); + }); +}); diff --git a/packages/objectql/src/having-filter.test.ts b/packages/objectql/src/having-filter.test.ts index d90f74f1ea..840278604b 100644 --- a/packages/objectql/src/having-filter.test.ts +++ b/packages/objectql/src/having-filter.test.ts @@ -13,6 +13,7 @@ */ import { describe, it, expect } from 'vitest'; +import { RETIRED_FILTER_OPERATORS } from '@objectstack/spec/data'; import { applyHaving, matchesHaving } from './having-filter.js'; const ROWS = [ @@ -65,15 +66,58 @@ describe('applyHaving', () => { }); }); +/** + * [#7047] The refusal's ENVELOPE, asserted separately from its message. + * + * Every assertion in this describe block used to be `toThrow(/message/)` alone, + * and that is precisely why the defect survived: a throw-only assertion is green + * whether or not the thrown error carries `code` and `status` (#6142/#6050 + * measured this shape of hole). Both `unknownOperator()` returns were bare + * `new Error`, so a 400-class author mistake reached the client 500-shaped + * through `rest`'s unclassified-fault branch — the only one of the five refusal + * faces that did (#6993's execution census; the four driver faces all answered + * `INVALID_FILTER` / 400). + * + * `code` and `status` are checked on BOTH branches — retired and unknown — and + * on BOTH positions — a field condition and a node-level logical key — because + * they are four independent `return`/`throw` sites and enveloping some of them + * leaves the same 500 one operator name away. + */ +function expectInvalidFilterEnvelope(fn: () => unknown): Error & { code?: string; status?: number } { + let err: (Error & { code?: string; status?: number }) | undefined; + try { + fn(); + } catch (e) { + err = e as Error & { code?: string; status?: number }; + } + expect(err, 'expected `having` to REFUSE this filter').toBeInstanceOf(Error); + // The ADR-0112 envelope the four driver faces already speak. Not a new code: + // a caller swapping HAVING for a driver-side `where` catches one shape. + expect(err!.code).toBe('INVALID_FILTER'); + expect(err!.status).toBe(400); + return err!; +} + describe('matchesHaving — the unknown-operator refusal', () => { it('THROWS on an unknown condition operator instead of ignoring it', () => { - expect(() => matchesHaving(ROWS[0], { order_count: { $median: 3 } })) - .toThrow(/Unsupported operator '\$median' in `having`.*\$gte.*unfiltered aggregates/s); + const err = expectInvalidFilterEnvelope(() => matchesHaving(ROWS[0], { order_count: { $median: 3 } })); + expect(err.message).toMatch(/Unsupported operator '\$median' in `having`.*\$gte.*unfiltered aggregates/s); }); it('THROWS on an unknown logical operator', () => { - expect(() => matchesHaving(ROWS[0], { $nand: [{ order_count: 2 }] })) - .toThrow(/Unsupported operator '\$nand'/); + const err = expectInvalidFilterEnvelope(() => matchesHaving(ROWS[0], { $nand: [{ order_count: 2 }] })); + expect(err.message).toMatch(/Unsupported operator '\$nand'/); + }); + + /** + * [#7047] The unknown branch is reachable from `applyHaving` too, which is the + * entry point `engine.aggregate()` actually calls — the envelope has to be on + * the error that leaves the module, not only on the one `matchesHaving` + * raises directly. + */ + it('carries the envelope out through applyHaving, the engine-facing entry point', () => { + expectInvalidFilterEnvelope(() => applyHaving(ROWS, { order_count: { $median: 3 } })); + expectInvalidFilterEnvelope(() => applyHaving(ROWS, { $nand: [{ order_count: 2 }] })); }); /** @@ -99,16 +143,18 @@ describe('matchesHaving — the unknown-operator refusal', () => { ["'$regex'", "'$options'", '$icontains'], ], ] as const) { - it(`REFUSES the retired ${label}, naming the replacement`, () => { - let err: Error | undefined; - try { - matchesHaving({ k: 'Alpha' }, condition); - } catch (e) { - err = e as Error; - } - expect(err, 'expected `having` to refuse a retired operator').toBeInstanceOf(Error); - expect(err!.message).toContain('RETIRED'); - for (const mention of mustMention) expect(err!.message).toContain(mention); + it(`REFUSES the retired ${label}, naming the replacement, in the ADR-0112 envelope`, () => { + // [#7047] `code` + `status` + message. The message half already passed + // before this card; the envelope half is what was missing. + const err = expectInvalidFilterEnvelope(() => matchesHaving({ k: 'Alpha' }, condition)); + expect(err.message).toContain('RETIRED'); + for (const mention of mustMention) expect(err.message).toContain(mention); + // [#7047] The prescription VERBATIM, not a paraphrase of it. The spec + // table exists so the five refusal sites stop each writing their own + // sentence about one retirement (#5701); a substring check on '$icontains' + // alone would stay green if this face started rewording it. + const first = Object.keys(condition.k)[0]!; + expect(err.message).toContain(RETIRED_FILTER_OPERATORS[first]!.why); }); } }); diff --git a/packages/objectql/src/having-filter.ts b/packages/objectql/src/having-filter.ts index 5f7f8b8e25..fb52785f5e 100644 --- a/packages/objectql/src/having-filter.ts +++ b/packages/objectql/src/having-filter.ts @@ -51,6 +51,10 @@ import { RETIRED_FILTER_OPERATORS } from '@objectstack/spec/data'; // of one vocabulary, and it reads the fold from the spec for the same reason it // reads the retirement prescriptions from there: one rule, one definition. import { asciiCaseInsensitiveContains } from '@objectstack/spec/data'; +// [#7047] The ADR-0112 envelope this face's refusals used to omit. Shared with +// `filter-comparand-shape.ts` rather than re-declared here — see the note on +// {@link invalidFilterError} and on {@link unknownOperator} below. +import { invalidFilterError } from './filter-comparand-shape.js'; const LOGICAL_OPERATORS = ['$and', '$or', '$not'] as const; @@ -72,6 +76,38 @@ const CONDITION_OPERATORS = [ '$contains', '$notContains', '$startsWith', '$endsWith', '$icontains', ] as const; +/** + * The refusal this face raises for an operator it will not evaluate — retired + * (`$regex`, `$options`) or simply unknown (`$nand`, `$median`). + * + * ## [#7047] BOTH returns carry the ADR-0112 envelope now + * + * They were bare `new Error(...)`. The refusal was happening and its message was + * already right — it printed `RETIRED_FILTER_OPERATORS[op].why` verbatim, like + * the four driver faces — but `code` and `status` were `undefined`, so `rest` + * served it through the unclassified-fault branch as a 500-shaped body for what + * is a 400-class AUTHOR mistake. Measured across all five refusal faces by + * EXECUTION rather than grep (#6993), this face was the only disagreement: + * + * | face | `code` | `status` | + * |:--|:--|:--| + * | driver-sql, driver-sqlite-wasm, driver-turso (both transports) | `INVALID_FILTER` | 400 | + * | driver-memory (`filter-refusal.ts`), driver-mongodb | `INVALID_FILTER` | 400 | + * | **objectql `having`** (here) | **undefined** | **undefined** | + * + * That is the half of #5324 the refusal itself does not fix, and the half + * `FilterTextRejectionCase.code` exists to pin: a suite that only asserts THREW + * stays green while the envelope is missing (#6142/#6050), which is exactly how + * this survived the retirement PR that wrote the messages. + * + * Both branches, deliberately. Enveloping only the retired path would leave + * `{ $nand: [...] }` and `{ k: { $median: 3 } }` arriving 500-shaped — the same + * defect, one operator name away, and the more likely of the two to be typed. + * + * The code is `INVALID_FILTER` because this joins the contract the other four + * faces already speak; it is not a new one. A caller swapping HAVING for a + * driver-side `where` must not have to catch two shapes for one mistake. + */ function unknownOperator( op: string, where: 'logical' | 'condition', @@ -90,7 +126,7 @@ function unknownOperator( + `${alsoRetired.map((key) => `'${key}'`).join(', ')} — one '${retired.to}' replaces the ` + `whole shape, so this is ONE mistake with ONE fix, not one per key.` : ''; - return new Error( + return invalidFilterError( `Filter operator '${op}' in \`having\` is RETIRED and is no longer evaluated.${replacement} ` + `${retired.why}${also}`, ); @@ -98,7 +134,7 @@ function unknownOperator( const supported = where === 'logical' ? `${LOGICAL_OPERATORS.join(', ')} (or a column condition)` : CONDITION_OPERATORS.join(', '); - return new Error( + return invalidFilterError( `Unsupported operator '${op}' in \`having\`. HAVING filters the aggregated rows ` + `(aggregation aliases + groupBy projections) and supports: ${supported}. ` + `An unknown operator is refused rather than ignored — ignoring it would silently `