Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 56 additions & 0 deletions .changeset/having-adr0112-envelope.md
Original file line number Diff line number Diff line change
@@ -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.
17 changes: 15 additions & 2 deletions packages/objectql/src/filter-comparand-shape.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
180 changes: 180 additions & 0 deletions packages/objectql/src/having-filter-text-conformance.test.ts
Original file line number Diff line number Diff line change
@@ -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<string, unknown>);
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<FilterTextCase, FilterTextRejectionCase> =>
!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);
});
});
74 changes: 60 additions & 14 deletions packages/objectql/src/having-filter.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 = [
Expand Down Expand Up @@ -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 }] }));
});

/**
Expand All @@ -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);
});
}
});
Expand Down
Loading
Loading