From 2059215a4edc5767a9a3da6a73d2d188ae16ac64 Mon Sep 17 00:00:00 2001 From: Vineeth Voruganti <13438633+VVoruganti@users.noreply.github.com> Date: Wed, 12 Aug 2026 23:37:19 -0400 Subject: [PATCH] fix(filter): treat a bare null operand as a null check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Routing every operand through _coerce_operand made bare `None` a type error rather than a null check, so {"session_id": null} raised FilterError where it previously built IS NULL: _build_field_condition used to end in `column == value`, which SQLAlchemy renders as IS NULL. Confirmed 422 on all five column families (text, numeric, boolean, datetime, JSONB). Nothing caught it. The docs added in this branch promise `{"session_id": None}` matches unset rows, the comment in _build_comparison_conditions claimed the equality path already covered it, and the DSL-wide invariant test accepts "compiles OR raises FilterError", so a 422 passed. _coerce_operand's docstring already stated the contract its caller wasn't honoring — "Callers handle None (a null check) and `*` (a wildcard) before calling" — so the guard restores that rather than adding a new rule. The three null forms now agree: {"col": null} is IS NULL, {"col": {"ne": null}} is IS NOT NULL, NOT [{"col": null}] is (IS NULL) IS NOT true. Also from review of #947: - Log filter keys, not the body. That log line is new in this branch and operands carry peer/session ids and free-text `contains` values; the traceback plus the entry shape is what locates a builder bug. - Assert whereclause in the _where test helper, so a dropped condition fails instead of returning the whole statement to substring-match. - Cover the raw-key JSONB path via source_ids, a JSONB column outside JSONB_COLUMNS reachable through Document's raw-key fallback. - Drop the orphaned comment left above ENUM_COLUMN_VALUES when _coerce_operand replaced SCALAR_OPERAND_TYPES. - Document that a JSONB column takes an object bare or under `contains` and nothing else. Bare {"metadata": X} is containment, so the `ne` this branch removed was never its inverse: a row with {"status":"done","x":1} satisfied both it and {"metadata": {"status":"done"}}. Per-key operators and NOT cover the real intents. - Rewrite "Negation and Unset Fields" to lead with the operator rule and a truth table, forward-linking to Filtering Conclusions instead of using conclusions ~525 lines before they are introduced. A conclusion's session_id is the only nullable documented filterable field, verified across Message/Document/Session/Peer/Workspace. Co-Authored-By: Claude Opus 5 (1M context) --- .../features/advanced/using-filters.mdx | 58 ++++++++++++------- src/utils/filter.py | 22 +++++-- tests/utils/test_filter.py | 40 ++++++++++++- 3 files changed, 91 insertions(+), 29 deletions(-) diff --git a/docs/v3/documentation/features/advanced/using-filters.mdx b/docs/v3/documentation/features/advanced/using-filters.mdx index 38bdabcd..4a1eefec 100644 --- a/docs/v3/documentation/features/advanced/using-filters.mdx +++ b/docs/v3/documentation/features/advanced/using-filters.mdx @@ -194,11 +194,26 @@ sessions = honcho.sessions(filters={ ### Negation and Unset Fields -Some fields can be unset. A conclusion drawn across a whole workspace has no -session, so its `session_id` is null. +A field can be unset, and negation has to account for it. `NOT` and the `ne` +comparison operator both **include** rows where the field has no value at all: a +field with no value is not the value you are excluding, so excluding that value +keeps the row. -`NOT` and `ne` include those rows. A conclusion with no session is not part of -some particular session, so excluding that session keeps it in the result: +Positive conditions work the other way around. An unset field matches nothing, +so equality and `contains` never return those rows. To select them, filter on +`null` directly: + +| Filter | Rows where the field is unset | +| --- | --- | +| `{"field": "x"}`, `{"field": {"contains": "x"}}` | Excluded | +| `{"NOT": [{"field": "x"}]}`, `{"field": {"ne": "x"}}` | Included | +| `{"field": null}` | Only these | +| `{"field": {"ne": null}}` | Excluded — the field must have some value | + +Of the filterable fields, only a conclusion's `session_id` can be unset: a +conclusion drawn across a whole workspace belongs to no single session. Every +other field is always populated, so none of this affects filters on them. See +[Filtering Conclusions](#filtering-conclusions) for what conclusions are. ```python Python @@ -232,12 +247,12 @@ conclusions = peer.conclusions.list(filters={ ``` -To exclude a value **and** require the field to be set, add a null check. -`{"ne": null}` matches rows where the field has any value: +To exclude a value **and** require the field to be set, combine the two with +`AND`: ```python Python -# In some session, just not this one — excludes conclusions with no session +# Excludes workspace-level conclusions and `session-123` conclusions = peer.conclusions.list(filters={ "AND": [ {"session_id": {"ne": "session-123"}}, @@ -248,7 +263,7 @@ conclusions = peer.conclusions.list(filters={ ```typescript TypeScript (async () => { - // In some session, just not this one — excludes conclusions with no session + // Excludes workspace-level conclusions and `session-123` const conclusions = await peer.conclusions.list({ filters: { AND: [ @@ -261,14 +276,6 @@ conclusions = peer.conclusions.list(filters={ ``` -Positive conditions work the other way around: an unset field matches nothing, -so `{"session_id": "session-123"}` and `{"session_id": {"contains": "abc"}}` -never return rows whose `session_id` is null. Use `{"session_id": None}` to -match those rows specifically. - -Fields that are always populated — `metadata`, which defaults to `{}`, and -`created_at` — are unaffected by any of this. - ### Combining Logical Operators Create sophisticated queries by combining different logical operators: @@ -789,21 +796,28 @@ doesn't hold, rather than failing mid-query or quietly returning nothing. | Numeric — `token_count` | Numbers, or numeric strings like `"5"`. Exact for integers of any size | | Timestamps — `created_at` | ISO 8601 strings such as `"2026-01-01"` or `"2026-01-01T12:00:00Z"` | | Boolean — `is_active` | `true` / `false` | -| `metadata` | An object, matched by containment | +| `metadata` | An object, matched by containment — bare or under `contains` | | Fields with fixed values — `level` | One of the documented values | +| Any field | `null`, which matches rows where the field is unset | -Two consequences worth knowing: +Three consequences worth knowing: - **Booleans must be real booleans.** `{"is_active": True}` filters; the string `{"is_active": "true"}` is rejected. - **Fixed-value fields are checked.** `{"level": "explicit"}` filters; `{"level": "typo"}` is rejected instead of returning an empty list, so a misspelling doesn't look like "no results". +- **`metadata` takes only the two shapes above** — bare, or under `contains`. + Comparison operators don't apply to the object as a whole, so + `{"metadata": {"ne": {...}}}` is rejected. To compare *within* metadata, put + the operator on the key — `{"metadata": {"status": {"ne": "done"}}}`. To negate + a match, wrap the whole condition in `NOT`. See + [Metadata Filtering](#metadata-filtering). -The same rules apply however the value is wrapped — bare, under an operator, or -inside an `in` list — so `{"level": "explicit"}`, -`{"level": {"ne": "explicit"}}` and `{"level": {"in": ["explicit"]}}` all -validate identically. +For every field other than `metadata`, the same rules apply however the value is +wrapped — bare, under an operator, or inside an `in` list — so +`{"level": "explicit"}`, `{"level": {"ne": "explicit"}}` and +`{"level": {"in": ["explicit"]}}` all validate identically. An empty `in` list matches nothing: diff --git a/src/utils/filter.py b/src/utils/filter.py index 3e46615f..1155d408 100644 --- a/src/utils/filter.py +++ b/src/utils/filter.py @@ -68,9 +68,6 @@ ALLOWED_EXTERNAL_TO_INTERNAL_COLUMN_MAPPING_DOCUMENTS = { MAX_SESSION_ALLOWLIST_ENTRIES = 1000 -# Values that can be bound to a non-JSONB column. Anything else (dict, list, -# bytes, arbitrary objects) compiles into a valid statement and then fails in -# psycopg at execute time as an unhandled 500, so it is rejected up front. # Columns whose values come from a closed set. Derived from the Literal types # themselves, so adding a level (e.g. "abduction") or a sync state updates # filter validation with no change here — an unlisted value is a 422 rather @@ -367,8 +364,13 @@ def apply_filter( except FilterError: raise except Exception: + # Keys only, not the body: filter operands are client-supplied and carry + # peer/session ids and free-text `contains` values. The traceback plus + # the entry shape is what actually locates a builder bug. logger.exception( - "Unexpected error building filter for %s: %r", model_class.__name__, filters + "Unexpected error building filter for %s; filter keys: %s", + model_class.__name__, + sorted(filters), ) raise FilterError("Invalid filter configuration") from None @@ -521,6 +523,14 @@ def _build_field_condition( if value == "*": return None + # A null operand is a null check, not a value to compare. Every branch below + # binds the operand against the column's type, and no type accepts None, so + # this has to short-circuit or `{"col": null}` raises instead of matching the + # rows it names. Keeps bare null agreeing with `{"ne": null}` (IS NOT NULL) + # and with `NOT [{"col": null}]`. + if value is None: + return column.is_(None) + # Bare-list sugar on regular columns: {"session_id": ["a", "b"]} is # shorthand for {"session_id": {"in": ["a", "b"]}}. JSONB columns are # excluded — a bare list there keeps JSONB containment semantics. @@ -764,8 +774,8 @@ def _build_comparison_conditions( continue # A null operand is a null check, not a value comparison, on every - # column type. Only `ne` is meaningful: {"col": None} already covers - # IS NULL via the equality path in _build_field_condition. + # column type. Only `ne` is meaningful: {"col": None} covers IS NULL + # via the null guard in _build_field_condition. if op_value is None: if operator != "ne": raise FilterError( diff --git a/tests/utils/test_filter.py b/tests/utils/test_filter.py index ce774740..c90c8442 100644 --- a/tests/utils/test_filter.py +++ b/tests/utils/test_filter.py @@ -210,6 +210,9 @@ def test_incompatible_operand_types_are_rejected(model: Any, filters: dict[str, def _where(model: Any, filters: dict[str, Any]) -> str: stmt = apply_filter(select(model), model, filters) + # Without this, a dropped condition makes split("WHERE") return the whole + # statement, so a filter that silently widened could still pass the asserts. + assert stmt.whereclause is not None return str(stmt.compile(dialect=psycopg_dialect.dialect())).split("WHERE")[-1] @@ -246,6 +249,42 @@ def test_ne_null_still_renders_is_not_null(): assert "IS NOT NULL" in _where(Document, {"session_id": {"ne": None}}) +@pytest.mark.parametrize( + ("model", "filters"), + [ + (Document, {"session_id": None}), # text + (Message, {"token_count": None}), # numeric + (Session, {"is_active": None}), # boolean + (Message, {"created_at": None}), # datetime + (Document, {"metadata": None}), # JSONB + (Document, {"source_ids": None}), # JSONB via the raw-key fallback + ], +) +def test_bare_null_is_a_null_check(model: Any, filters: dict[str, Any]): + """Regression: every operand routes through _coerce_operand, which accepts + no None on any column type, so bare null raised FilterError instead of + matching the unset rows it names.""" + assert "IS NULL" in _where(model, filters) + + +def test_bare_null_agrees_with_negation(): + """`NOT [{col: null}]` and `{col: {ne: null}}` must select the same rows. + `x IS NULL` never evaluates to NULL, so `(x IS NULL) IS NOT true` is exactly + `x IS NOT NULL` — the three forms have to stay in agreement.""" + assert "IS NOT NULL" in _where(Document, {"session_id": {"ne": None}}) + negated = _where(Document, {"NOT": [{"session_id": None}]}) + assert "IS NULL" in negated + assert "IS NOT true" in negated + + +def test_dict_on_raw_key_jsonb_column_is_equality(): + """Document falls back to raw column names, so a JSONB column outside + JSONB_COLUMNS reaches _build_field_condition's dict branch. It compares as + equality rather than containment — unlike `metadata`.""" + where = _where(Document, {"source_ids": {"kind": "note"}}) + assert "source_ids =" in where + + @pytest.mark.parametrize( ("filters", "expected"), [ @@ -316,7 +355,6 @@ _MALFORMED: list[dict[str, Any]] = [ {"NOT": None}, {"NOT": [None]}, {"unknown_column": 1}, - {"metadata": None}, ]