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}, ]