fix(filter): make NOT and ne include rows where the field is unset
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.
This commit is contained in:
Vineeth Voruganti
parent
81e7090d70
commit
59f82944c5
|
|
@ -192,6 +192,83 @@ sessions = honcho.sessions(filters={
|
|||
```
|
||||
</CodeGroup>
|
||||
|
||||
### 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:
|
||||
|
||||
<CodeGroup>
|
||||
```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" } }
|
||||
});
|
||||
})();
|
||||
```
|
||||
</CodeGroup>
|
||||
|
||||
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:
|
||||
|
||||
<CodeGroup>
|
||||
```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 } }
|
||||
]
|
||||
}
|
||||
});
|
||||
})();
|
||||
```
|
||||
</CodeGroup>
|
||||
|
||||
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"})
|
|||
```
|
||||
</CodeGroup>
|
||||
|
||||
## 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:
|
||||
|
||||
<CodeGroup>
|
||||
```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: [] } } });
|
||||
})();
|
||||
```
|
||||
</CodeGroup>
|
||||
|
||||
## Error Handling
|
||||
|
||||
Handle filter errors gracefully:
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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"}
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
Loading…
Reference in New Issue