From 59f82944c53119e0a569abf8b5b0a981e5268227 Mon Sep 17 00:00:00 2001 From: Vineeth Voruganti <13438633+VVoruganti@users.noreply.github.com> Date: Tue, 28 Jul 2026 17:50:25 -0400 Subject: [PATCH] fix(filter): make NOT and ne include rows where the field is unset MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit NOT (col = v) and col <> v are NULL when col is NULL, so negation dropped rows whose column is unset — a conclusion with no session is not "some other session", but was excluded anyway. IS NOT TRUE and IS DISTINCT FROM behave identically when no NULL is involved. Adds a row-count test, since this class of bug builds and executes cleanly, and documents {"ne": null} for excluding unset fields. --- .../features/advanced/using-filters.mdx | 121 ++++++++++++++++++ src/utils/filter.py | 17 ++- tests/routes/test_conclusions.py | 78 +++++++++++ tests/utils/test_filter.py | 56 +++++++- 4 files changed, 268 insertions(+), 4 deletions(-) diff --git a/docs/v3/documentation/features/advanced/using-filters.mdx b/docs/v3/documentation/features/advanced/using-filters.mdx index 2a015c03..6ac93b9c 100644 --- a/docs/v3/documentation/features/advanced/using-filters.mdx +++ b/docs/v3/documentation/features/advanced/using-filters.mdx @@ -192,6 +192,83 @@ 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. + +`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: + + +```python Python +# Every conclusion except the ones in this session — including +# workspace-level conclusions, which belong to no session at all +conclusions = peer.conclusions.list(filters={ + "NOT": [ + {"session_id": "session-123"} + ] +}) + +# Equivalent +conclusions = peer.conclusions.list(filters={ + "session_id": {"ne": "session-123"} +}) +``` + +```typescript TypeScript +(async () => { + // Every conclusion except the ones in this session — including + // workspace-level conclusions, which belong to no session at all + const conclusions = await peer.conclusions.list({ + filters: { NOT: [{ session_id: "session-123" }] } + }); + + // Equivalent + const same = await peer.conclusions.list({ + filters: { session_id: { ne: "session-123" } } + }); +})(); +``` + + +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: + + +```python Python +# In some session, just not this one — excludes conclusions with no session +conclusions = peer.conclusions.list(filters={ + "AND": [ + {"session_id": {"ne": "session-123"}}, + {"session_id": {"ne": None}} + ] +}) +``` + +```typescript TypeScript +(async () => { + // In some session, just not this one — excludes conclusions with no session + const conclusions = await peer.conclusions.list({ + filters: { + AND: [ + { session_id: { ne: "session-123" } }, + { session_id: { ne: null } } + ] + } + }); +})(); +``` + + +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: @@ -673,6 +750,50 @@ bob_explicit = peer.conclusions_of("bob").list(filters={"level": "explicit"}) ``` +## Value Types + +A filter value has to be usable against the field it targets. Honcho validates +this before running the query and returns a `422` with an explanation when it +doesn't hold, rather than failing mid-query or quietly returning nothing. + +| Field | Accepts | +| --- | --- | +| Text — `peer_id`, `session_id`, `id`, `content` | Strings | +| 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 | +| Fields with fixed values — `level` | One of the documented values | + +Two 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". + +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: + + +```python Python +# Returns no results — an empty allowlist excludes everything +messages = session.messages(filters={"peer_id": {"in": []}}) +``` + +```typescript TypeScript +(async () => { + // Returns no results — an empty allowlist excludes everything + const messages = await session.messages({ filters: { peer_id: { in: [] } } }); +})(); +``` + + ## Error Handling Handle filter errors gracefully: diff --git a/src/utils/filter.py b/src/utils/filter.py index a89a613d..9267b6dd 100644 --- a/src/utils/filter.py +++ b/src/utils/filter.py @@ -5,7 +5,7 @@ from logging import getLogger from typing import Any, TypeVar, get_args from typing import cast as typing_cast -from sqlalchemy import ColumnElement, Select, and_, case, cast, literal, not_, or_ +from sqlalchemy import ColumnElement, Select, and_, case, cast, literal, or_ from sqlalchemy.dialects.postgresql import JSONB from sqlalchemy.types import Numeric @@ -425,8 +425,15 @@ def _build_filter_conditions( _depth=_depth + 1, ) if sub_condition is not None: + # `IS NOT TRUE` rather than `NOT`: under SQL's three-valued + # logic a comparison against a NULL column is NULL, and `NOT + # NULL` is NULL, so plain negation drops rows whose column is + # unset — even though an unset column does not match what is + # being excluded. NOT [{"session_id": "abc"}] must include + # documents that have no session, since those are not "abc". + # Composes over compound sub-conditions: (a AND b) IS NOT TRUE. not_conditions.append( - not_(sub_condition) + sub_condition.is_not(True) ) # Apply NOT to each condition individually if not_conditions: conditions.append(and_(*not_conditions)) # Then AND them together @@ -767,7 +774,11 @@ def _build_comparison_conditions( elif operator == "lt": condition = column < casted_value elif operator == "ne": - condition = column != casted_value + # IS DISTINCT FROM, not <>: `NULL <> 'abc'` is NULL, so plain + # inequality drops rows whose column is unset. Identical to <> + # whenever no NULL is involved, and keeps `ne` agreeing with the + # NOT operator instead of quietly returning a different row set. + condition = column.is_distinct_from(casted_value) elif operator == "in": if hasattr(op_value, "__iter__") and not isinstance(op_value, str | bytes): # Handle wildcard in iterable - if present, matches everything, so no condition needed diff --git a/tests/routes/test_conclusions.py b/tests/routes/test_conclusions.py index 77cb23fb..ccd3b337 100644 --- a/tests/routes/test_conclusions.py +++ b/tests/routes/test_conclusions.py @@ -1381,3 +1381,81 @@ class TestConclusionRoutes: # Verify the conclusion has null session_id conclusion = next(c for c in data["items"] if c["id"] == created_id) assert conclusion["session_id"] is None + + @pytest.mark.asyncio + async def test_negation_includes_conclusions_with_no_session( + self, + client: TestClient, + db_session: AsyncSession, + sample_data: tuple[Workspace, Peer], + ): + """Negation must not silently drop workspace-level conclusions. + + A conclusion with no session is not "some other session", so excluding + that session has to leave it in the result. Under SQL's three-valued + logic a comparison against NULL is NULL, which would drop the row. + + This is only visible by counting returned rows — the filter builds and + executes cleanly either way. + """ + test_workspace, test_peer = sample_data + + test_peer2 = models.Peer( + name=str(generate_nanoid()), workspace_name=test_workspace.name + ) + db_session.add(test_peer2) + await db_session.flush() + + test_session = models.Session( + name=str(generate_nanoid()), workspace_name=test_workspace.name + ) + other_session = models.Session( + name=str(generate_nanoid()), workspace_name=test_workspace.name + ) + db_session.add_all([test_session, other_session]) + await db_session.commit() + + await self._create_collection( + db_session, test_workspace.name, test_peer.name, test_peer2.name + ) + + scoped = models.Document( + workspace_name=test_workspace.name, + observer=test_peer.name, + observed=test_peer2.name, + content="Scoped to a session", + session_name=test_session.name, + ) + workspace_level = models.Document( + workspace_name=test_workspace.name, + observer=test_peer.name, + observed=test_peer2.name, + content="Not scoped to any session", + session_name=None, + ) + db_session.add_all([scoped, workspace_level]) + await db_session.commit() + + def contents(filters: dict[str, object]) -> set[str]: + response = client.post( + f"/v3/workspaces/{test_workspace.name}/conclusions/list", + json={"filters": filters}, + ) + assert response.status_code == 200, response.text + return {item["content"] for item in response.json()["items"]} + + both = {"Scoped to a session", "Not scoped to any session"} + + # NOT and ne agree, and both keep the session-less conclusion. + assert contents({"NOT": [{"session_id": other_session.name}]}) == both + assert contents({"session_id": {"ne": other_session.name}}) == both + + # Requiring the field to be set is how you narrow to sessioned rows. + assert contents( + { + "AND": [ + {"session_id": {"ne": other_session.name}}, + {"session_id": {"ne": None}}, + ] + } + ) == {"Scoped to a session"} diff --git a/tests/utils/test_filter.py b/tests/utils/test_filter.py index f1464827..ce774740 100644 --- a/tests/utils/test_filter.py +++ b/tests/utils/test_filter.py @@ -129,7 +129,7 @@ def test_ne_string_on_text_column_compares_as_string(): """Regression: numeric operators float()-cast on every column type, so a string inequality on a text column was rejected as a bad number.""" stmt = apply_filter(select(Document), Document, {"session_id": {"ne": "abc"}}) - assert "session_name !=" in str(stmt) + assert "session_name IS DISTINCT FROM" in str(stmt) def test_numeric_operator_still_validates_on_numeric_column(): @@ -208,6 +208,60 @@ def test_incompatible_operand_types_are_rejected(model: Any, filters: dict[str, apply_filter(select(model), model, filters) +def _where(model: Any, filters: dict[str, Any]) -> str: + stmt = apply_filter(select(model), model, filters) + return str(stmt.compile(dialect=psycopg_dialect.dialect())).split("WHERE")[-1] + + +def test_not_is_null_safe(): + """`NOT (col = v)` is NULL when col is NULL, so plain negation drops rows + whose column is unset — even though an unset column is not `v`.""" + where = _where(Document, {"NOT": [{"session_id": "abc"}]}) + assert "IS NOT true" in where + + +def test_ne_is_null_safe(): + where = _where(Document, {"session_id": {"ne": "abc"}}) + assert "IS DISTINCT FROM" in where + + +def test_not_is_null_safe_over_a_compound_condition(): + """Negation has to survive nesting, not just single comparisons.""" + where = _where( + Document, {"NOT": [{"AND": [{"session_id": "a"}, {"level": "explicit"}]}]} + ) + assert "IS NOT true" in where + assert "AND" in where + + +def test_not_is_null_safe_over_contains(): + where = _where(Document, {"NOT": [{"session_id": {"contains": "x"}}]}) + assert "ILIKE" in where + assert "IS NOT true" in where + + +def test_ne_null_still_renders_is_not_null(): + """The null operand is intercepted before the operator dispatch, so this + path is unchanged by null-safe `ne`.""" + assert "IS NOT NULL" in _where(Document, {"session_id": {"ne": None}}) + + +@pytest.mark.parametrize( + ("filters", "expected"), + [ + ({"session_id": "abc"}, "session_name ="), + ({"session_id": {"contains": "x"}}, "ILIKE"), + ], +) +def test_positive_predicates_are_unchanged(filters: dict[str, Any], expected: str): + """A NULL column does not equal or contain anything, so positive predicates + correctly exclude those rows and must keep their plain operators.""" + where = _where(Document, filters) + assert expected in where + assert "IS NOT true" not in where + assert "IS DISTINCT FROM" not in where + + # --- Invariants over the whole DSL ------------------------------------------- # # The filter body is arbitrary client JSON. Enumerating bad shapes one at a time