Skip to content

fix(objectql): gate having's $icontains comparand shape (#7158)#7236

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
os-help merged 1 commit into from
Aug 10, 2026
Merged

fix(objectql): gate having's $icontains comparand shape (#7158) #7236

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
14 changes: 14 additions & 0 deletions .changeset/silly-jars-shake.md
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -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.
217 changes: 127 additions & 90 deletions packages/objectql/src/having-filter-text-conformance.test.ts
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -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)
*/
Expand All@@ -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<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));

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