diff --git a/CHANGELOG.md b/CHANGELOG.md index 14ebdf9..4d36f13 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added +- **`resolve_sort_value(record, field)`** (`jvspatial/db/database.py`) — the + dotted-path resolution `finalize_find_results` uses, exported so adapters and + cursor logic resolve a sort field the same way. Added to the module's + `__all__`. + - **Deferred-task exception hierarchy** (`jvspatial/exceptions.py`) — `DeferredTaskError` → `TaskDispatchError` → `TaskSchedulerNotConfiguredError`, replacing the bare `RuntimeError`s raised by strict dispatch. A strict caller @@ -19,6 +24,66 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed + +- **`SQLiteDB.find` treated `sort=[]` as an untranslatable sort** + (`jvspatial/db/sqlite.py`). An empty list failed the `sort is None` guard, so + it took the fallback branch: the whole collection was loaded and `limit` + applied in memory instead of being pushed into SQL. Results were correct, the + work was not. A falsy `sort` is now normalized to `None`. +- **`ObjectPager` re-sorted each page with a key that disagreed with the + database slice** (`jvspatial/core/pager.py`; `paginate_by_field` inherited + it). The in-Python safety-net sort used + `item.get("context", {}).get(order_by, 0)`, so a record missing `order_by` + was ordered as `0` — among the real values — while the DB-side + `sort` + `limit` that produced the slice had placed it in the trailing + missing-value run. Records could therefore appear on two pages or on none. + A blanket `contextlib.suppress(KeyError, TypeError)` also left a page + silently unsorted on mixed-type keys. The re-sort now routes through + `finalize_find_results`, making it a genuine no-op whenever the backend + honored the sort. +- **`GraphContext.find_page` broke on dotted sort fields and could not reach + records missing the sort value** (`jvspatial/core/context.py`). Two defects: + the cursor payload was minted with a flat `last.get(primary_field)`, so a + `sort=[("context.started_at", -1)]` page always encoded `sort: None` and the + next page's keyset filter compared against `None` — raising + `TypeError: '>' not supported between instances of 'int' and 'NoneType'` from + `QueryEngine` on JsonDB. And with records missing the sort field now sorting + last, the keyset filter `{field: {"$lt": value}}` could never match them, so + iteration silently stopped at the last record that had a value. The cursor now + uses `resolve_sort_value`, the filter carries a `{field: None}` branch to reach + the trailing run, and a cursor minted inside that run walks it by `id`. +- **Postgres applied `LIMIT` in SQL even when the sort could not be pushed + down** (`jvspatial/db/postgres.py`, both `PostgresDB.find` and + `PostgresTransaction.find`). `translate_sort` returns `None` for a field path + it cannot safely interpolate (e.g. `context.my-field`), leaving the ordering + to `finalize_find_results` — but the `LIMIT` was still pushed, so the database + returned an arbitrary N rows and the in-memory sort ordered that arbitrary + subset. `find(sort=..., limit=10)` returned "the top 10 of an arbitrary 10" + instead of the true top 10. The `LIMIT` is now withheld whenever the sort + falls back to memory, matching `SQLiteDB.find` and `DynamoDB.find`. Vector + (`$near`) queries additionally no longer have their distance ordering + overwritten by an in-memory re-sort on the user's `sort`. + +- **In-memory `find` sort ignored dotted field paths** (`jvspatial/db/database.py`). + `_find_sort_key` resolved `sort` fields with a flat `record.get(field)`, so a + spec like `sort=[("context.started_at", -1)]` produced `None` for every row and + left the result in arbitrary order. The SQLite and Postgres pushdowns + (`translate_sort`) and Mongo's native sort already resolved dotted paths, so + the same query ordered correctly on those backends and silently did not on + JsonDB/DynamoDB — and on SQLite/Postgres whenever the query fell back to the + in-memory path. Dotted paths now resolve in memory too; a non-dict segment + along the path yields `None` rather than raising. +- **Descending in-memory sorts placed records missing the sort field first** + (`jvspatial/db/database.py`). `finalize_find_results` sorts with + `reverse=True`, which flipped `_find_sort_key`'s `None` flag along with the + values. Both SQL translators emit `NULLS LAST` for descending and Mongo sorts + missing values last, so a "newest N" `sort` + `limit` fetch returned real rows + on SQLite/Postgres/Mongo and a window of records missing the field on the + in-memory path. Missing values now sort last in both directions everywhere. + The comment in `_sqlite_translate.translate_sort` asserting the in-memory path + already matched has been corrected. + + - **Strict deferred scheduling only caught the no-op-scheduler case** (`jvspatial/serverless/`). `dispatch_deferred_task(..., strict=True)` raised when serverless mode resolved a logging no-op, but every *provider* failure @@ -47,6 +112,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed + - **`TaskScheduler.schedule` takes a `strict` argument** (`jvspatial/serverless/tasks/base.py`). `TaskScheduler` is a public/stable extension point and `config.task_scheduler` is duck-typed, so @@ -56,6 +122,27 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 raises `TaskSchedulerNotConfiguredError` (it cannot honor the guarantee) rather than `TypeError`. +### Documentation + + +- **`find` sort contract moved to SPEC §4.1** (beside the `Database` method + table it governs, rather than under §4.2 capability flags) and extended: the + `limit`-must-not-outlive-the-sort-pushdown rule, plus a **Known divergences** + table covering MongoDB's ascending sorts (native `cursor.sort()` places + missing values first — documented, not normalized), array-index path segments, + and heterogeneous value types. +- **Corrected stale NULL-ordering docstrings** in + `jvspatial/db/_sqlite_translate.py` (module docstring and `translate_sort`) + and `jvspatial/db/_postgres_translate.py` (`translate_sort`). All three still + claimed "NULLs sort last for ascending, first for descending, mirroring + `finalize_find_results`" — the opposite of what the code emits and of the + contract. +- **`Database.find`** now documents the ordering contract adapter authors must + satisfy; **`Database.find_iter`** no longer claims a composite + `(sort_value, id)` cursor — the default implementation tracks `id` only, so a + non-`id` sort drops records that sort late but carry a lower `id`. + + ## [0.0.11] - 2026-07-02 ### Fixed diff --git a/SPEC.md b/SPEC.md index 73d5c28..5a74fa5 100644 --- a/SPEC.md +++ b/SPEC.md @@ -207,7 +207,7 @@ When `DeferredSaveMixin` is mixed into an entity *and* `deferred_saves_globally_ | `save(collection, data)` | Yes | Insert-or-replace by ID; returns saved record | | `get(collection, id)` | Yes | Fetch by ID or `None` | | `delete(collection, id)` | Yes | Idempotent delete by ID | -| `find(collection, query, *, limit, sort)` | Yes | Mongo-style query; returns list | +| `find(collection, query, *, limit, sort)` | Yes | Mongo-style query; returns list. Ordering contract below | | `count(collection, query=None)` | Default impl | Default counts the result of `find`; adapters should override for efficiency | | `find_one(collection, query)` | Default impl | First match or `None` | | `find_many(collection, ids)` | Default impl | Bulk-fetch by ID; default is N sequential `get`s — adapters override for round-trip efficiency | @@ -216,6 +216,36 @@ When `DeferredSaveMixin` is mixed into an entity *and* `deferred_saves_globally_ | `bulk_save` | Default impl | Multi-record save; partial-success semantics vary by adapter | | `begin_transaction` | Optional | Returns a transaction context manager if `supports_transactions=True` | +#### `find` sort contract + +`sort` is a list of `(field, direction)` with `1` ascending and `-1` descending. +An adapter may push the sort into the backend or fall back to +`finalize_find_results` (`jvspatial/db/database.py`); either way it must produce +the same ordering: + +- **Dotted paths** (`context.started_at`) resolve into nested documents. A + non-dict segment along the path resolves to "missing", not an error. Use + `resolve_sort_value` rather than a flat `record.get(field)` anywhere a sort + field is read — including cursor payloads. +- **Missing values sort last in both directions** — matching the `NULLS LAST` + emitted by `_sqlite_translate.translate_sort` and + `_postgres_translate.translate_sort`. A `sort` + `limit` "newest N" fetch + therefore never fills its window with records lacking the sort field. +- Sorting is **stable**; compound sorts apply from the last key to the first. +- **`limit` must not be pushed down when the sort is not.** An adapter that + cannot express the ordering in the backend has to fetch the full match set + and apply `sort` and `limit` together in memory — otherwise it orders an + arbitrary N rows instead of returning the true top N. + +**Known divergences** — the contract holds for homogeneous scalar leaves; these +cases are documented rather than normalized: + +| Case | Behavior | +|---|---| +| MongoDB, ascending | `MongoDB.find` uses native `cursor.sort()`. BSON orders null/missing lowest, so **missing values come first** on ascending sorts. Descending matches the contract. Normalizing would require an aggregation pipeline on every `find`. | +| Array-index segments (`items.0`) | Resolve to "missing" in memory and on SQLite; rejected by the Postgres pushdown (leading digit fails `_safe_field_path`, so it falls back and agrees); resolved natively by MongoDB. | +| Heterogeneous values on one key | The in-memory path raises `TypeError` (Python cannot order `str` against `int`); SQL pushdowns order by storage class instead. Object/array leaves likewise raise in memory and sort as JSON text on SQLite. | + ### 4.2 Capability flags Adapters declare capabilities as class attributes: diff --git a/jvspatial/core/context.py b/jvspatial/core/context.py index 9268f80..b201043 100644 --- a/jvspatial/core/context.py +++ b/jvspatial/core/context.py @@ -23,7 +23,7 @@ cast, ) -from jvspatial.db.database import Database +from jvspatial.db.database import Database, resolve_sort_value from jvspatial.db.factory import create_database, get_current_database from jvspatial.db.manager import get_database_manager @@ -1435,17 +1435,43 @@ async def find_page( if cursor_payload and "id" in cursor_payload and "sort" in cursor_payload: sort_op = "$lt" if primary_dir < 0 else "$gt" id_op = "$lt" if id_dir < 0 else "$gt" - keyset_filter = { - "$or": [ - {primary_field: {sort_op: cursor_payload["sort"]}}, + cursor_sort = cursor_payload["sort"] + keyset_branches: List[Dict[str, Any]] + if cursor_sort is None: + # The cursor sits in the trailing run of records that have + # no value for the sort field. Records missing the sort + # field sort last in both directions (see + # ``finalize_find_results``), so everything still ahead of + # us is also missing it — walk that run by id alone. + keyset_branches = [ { "$and": [ - {primary_field: cursor_payload["sort"]}, + {primary_field: None}, + {"id": {id_op: cursor_payload["id"]}}, + ] + } + ] + else: + keyset_branches = [ + {primary_field: {sort_op: cursor_sort}}, + # ``{field: None}`` matches both an explicit null and a + # missing key. Without this branch the nulls-last tail + # is unreachable: ``{field: {"$lt": v}}`` never matches + # a record that has no value at all, so iteration would + # stop at the last record that does. + {primary_field: None}, + { + "$and": [ + {primary_field: cursor_sort}, {"id": {id_op: cursor_payload["id"]}}, ] }, ] - } + keyset_filter: Dict[str, Any] = ( + keyset_branches[0] + if len(keyset_branches) == 1 + else {"$or": keyset_branches} + ) final_query = ( {"$and": [final_query, keyset_filter]} if final_query else keyset_filter ) @@ -1459,7 +1485,13 @@ async def find_page( next_cursor: Optional[str] = None if has_more and page_rows: last = page_rows[-1] - payload = {"id": last.get("id"), "sort": last.get(primary_field)} + # Dotted sort fields (``context.started_at``) need the same + # path walk the adapters use; a flat ``.get`` would mint a + # ``None`` sort value for every cursor and stall pagination. + payload = { + "id": last.get("id"), + "sort": resolve_sort_value(last, primary_field), + } next_cursor = base64.urlsafe_b64encode( json.dumps(payload, separators=(",", ":")).encode() ).decode() diff --git a/jvspatial/core/pager.py b/jvspatial/core/pager.py index a411a53..b8fb74e 100644 --- a/jvspatial/core/pager.py +++ b/jvspatial/core/pager.py @@ -5,10 +5,11 @@ Designed to integrate seamlessly with UI frameworks requiring paginated data. """ -import contextlib from math import ceil from typing import TYPE_CHECKING, Any, Dict, List, Optional, Type, TypeVar +from jvspatial.db.database import finalize_find_results + if TYPE_CHECKING: from .entities import Object @@ -195,14 +196,15 @@ async def get_page( ) page_items_raw = all_items_raw[offset : offset + self.page_size] - # Apply in-Python ordering when a non-id order_by is set. + # Safety net for a backend that ignored the requested sort. Route it + # through the same helper the adapters use so it is a no-op when the + # sort was honored: an ad-hoc key here would disagree with the DB-side + # ordering that produced the slice above. In particular, defaulting a + # missing value to ``0`` ordered it among the real values while the + # slice had already placed it in the trailing missing-value run, which + # duplicates or drops rows across page boundaries. if self.order_by and page_sort != [("id", 1)]: - reverse = self.order_direction.lower() == "desc" - with contextlib.suppress(KeyError, TypeError): - page_items_raw.sort( - key=lambda item: item.get("context", {}).get(self.order_by, 0), - reverse=reverse, - ) + page_items_raw = finalize_find_results(page_items_raw, sort=page_sort) page_objects: List[T] = [] for item_data in page_items_raw: diff --git a/jvspatial/db/_postgres_translate.py b/jvspatial/db/_postgres_translate.py index 1681f1b..447d081 100644 --- a/jvspatial/db/_postgres_translate.py +++ b/jvspatial/db/_postgres_translate.py @@ -589,10 +589,10 @@ def translate_sort( or invalid direction). The fragment does NOT include the leading ``ORDER BY`` keyword. - NULLs sort last for ascending, first for descending — mirrors - :func:`jvspatial.db.database.finalize_find_results` semantics. (Postgres' - default puts NULLs first for ASC, which is the opposite of what - callers expect, so we set NULLS LAST / NULLS FIRST explicitly.) + NULLs sort last in both directions — mirrors + :func:`jvspatial.db.database.finalize_find_results` semantics (SPEC §4.1, + find sort contract). Postgres' own default puts NULLs first for ASC and + last for DESC, so ``NULLS LAST`` is set explicitly on every key. """ if not sort: return None diff --git a/jvspatial/db/_sqlite_translate.py b/jvspatial/db/_sqlite_translate.py index 402315a..dda3a6e 100644 --- a/jvspatial/db/_sqlite_translate.py +++ b/jvspatial/db/_sqlite_translate.py @@ -28,9 +28,9 @@ ORDER BY pushdown ----------------- :func:`translate_sort` handles single-/multi-key sorts on simple -identifiers (no operators in the key). NULLs sort last for ascending and -first for descending, mirroring the in-memory ``finalize_find_results`` -behavior. +identifiers (no operators in the key). NULLs sort last in *both* +directions, mirroring the in-memory ``finalize_find_results`` behavior +(SPEC §4.1, find sort contract). Security -------- @@ -271,8 +271,8 @@ def translate_sort(sort: Optional[List[Tuple[str, int]]]) -> Optional[str]: invalid direction). The fragment does NOT include the leading ``ORDER BY`` keyword. - NULLs sort last for ascending, first for descending -- this matches - ``finalize_find_results`` semantics. + NULLs sort last in both directions -- this matches + ``finalize_find_results`` semantics (SPEC §4.1, find sort contract). """ if not sort: return None @@ -287,10 +287,10 @@ def translate_sort(sort: Optional[List[Tuple[str, int]]]) -> Optional[str]: # ascending: NULLs last parts.append(f"({column} IS NULL), {column} ASC") else: - # descending: NULLs last too (matches in-memory behavior: - # the in-memory sort uses (value is None, value), reverse=True, - # which puts None last because it sorts (True, ...) after - # (False, ...).) + # descending: NULLs last too. The leading ``IS NULL`` term is + # itself sorted ASC, so non-NULL rows (0) precede NULL rows (1) + # regardless of direction. ``_find_sort_key`` inverts its None + # flag for descending sorts to reach the same ordering. parts.append(f"({column} IS NULL), {column} DESC") return ", ".join(parts) diff --git a/jvspatial/db/database.py b/jvspatial/db/database.py index 5a16c21..525ee49 100644 --- a/jvspatial/db/database.py +++ b/jvspatial/db/database.py @@ -70,10 +70,52 @@ def all_saved(self) -> bool: logger = logging.getLogger(__name__) -def _find_sort_key(record: Dict[str, Any], field: str) -> Tuple[bool, Any]: - """Sort key: non-``None`` values first, then by value (with ``None`` last).""" - value = record.get(field) - return (value is None, value) +def resolve_sort_value(record: Dict[str, Any], field: str) -> Any: + """Read ``field`` out of ``record``, following dotted paths. + + ``field`` may be a plain key (``id``) or a dotted path into nested + documents (``context.started_at``), matching what the SQLite/Postgres + sort pushdowns and Mongo's native sort accept. A missing key — or a + non-dict encountered part-way along the path — resolves to ``None`` + rather than raising, so a malformed record sorts as "missing" instead + of breaking the whole page. + + Anything that mints or interprets a sort value should go through this + rather than a flat ``record.get(field)``: cursor payloads in + :meth:`GraphContext.find_page` and adapter-side sorting must agree on + what a sort field resolves to. + """ + if "." not in field: + return record.get(field) + value: Any = record + for part in field.split("."): + if not isinstance(value, dict): + return None + value = value.get(part) + return value + + +def _find_sort_key( + record: Dict[str, Any], field: str, *, descending: bool = False +) -> Tuple[bool, Any]: + """Sort key placing missing values last, ascending or descending. + + Supports dotted paths (``context.started_at``) so callers can sort attribute + fields the same way they query them. The SQLite/Postgres sort pushdowns + (``_sqlite_translate.translate_sort`` / ``_postgres_translate.translate_sort``) + and Mongo's native sort already resolve dotted paths; without this the same + sort spec silently degraded to "every key is ``None``" whenever a backend + fell back to the in-memory path. + + ``finalize_find_results`` sorts descending via ``reverse=True``, which flips + the whole key — including the ``None`` flag. Inverting the flag for + descending sorts keeps missing values last in both directions, matching the + ``NULLS LAST`` both SQL translators emit. All missing values share a flag, + so ``None`` is never compared against a real value. + """ + value = resolve_sort_value(record, field) + missing = value is None + return (not missing if descending else missing, value) def _normalize_id_query(query: Dict[str, Any]) -> Dict[str, Any]: @@ -104,15 +146,18 @@ def finalize_find_results( ``sort`` is a list of ``(field, direction)`` with ``direction`` ``1`` for ascending and ``-1`` for descending. Sorting is stable; compound sorts are - applied from the last key to the first. + applied from the last key to the first. Records missing the sort field sort + last in both directions, matching the ``NULLS LAST`` the SQLite and Postgres + pushdowns emit. """ out = records if sort: out = list(records) for sort_field, direction in reversed(sort): + descending = direction == -1 out.sort( - key=partial(_find_sort_key, field=sort_field), - reverse=(direction == -1), + key=partial(_find_sort_key, field=sort_field, descending=descending), + reverse=descending, ) if limit is not None: out = out[:limit] @@ -212,6 +257,25 @@ async def find( Returns: List of matching records + + Ordering contract (SPEC §4.1) — implementations must satisfy all of + it whether they push the sort into the backend or fall back to + :func:`finalize_find_results`: + + * ``field`` may be a dotted path into nested documents + (``context.started_at``); resolve it with + :func:`resolve_sort_value`, never a flat ``record.get(field)``. + * Records with no value for the sort field come **last in both + directions**, matching the ``NULLS LAST`` the SQL translators emit. + * The sort is stable; compound sorts apply from the last key first. + * If the ordering cannot be expressed in the backend, ``limit`` must + not be pushed down either — fetch the full match set and apply + ``sort`` and ``limit`` together in memory, or the result is the top + N of an arbitrary N. + + Known divergences are listed in SPEC §4.1 (MongoDB places missing + values first on ascending sorts; heterogeneous value types raise in + memory but order by storage class in SQL). """ pass @@ -257,10 +321,16 @@ async def find_iter( Args: collection: Collection name. query: Mongo-style query (same operator surface as ``find``). - sort: Optional ``[(field, direction)]``. When provided, the - cursor is a composite ``(sort_value, id)`` for stable - pagination across concurrent writes. When omitted, - sort defaults to ``id`` ascending. + sort: Optional ``[(field, direction)]``, defaulting to ``id`` + ascending. **The default implementation's cursor tracks + ``id`` only**, so a non-``id`` sort is not safely pageable + here: each batch asks for ``id > last_id``, which drops + records that sort after the batch but carry a lower ``id``. + Use ``sort=None`` (or an ``id`` sort) with this default, or + an adapter override with a native cursor. A composite + ``(sort_value, id)`` cursor is not yet implemented — for a + custom sort key with correct paging use + :meth:`jvspatial.core.context.GraphContext.find_page`. batch_size: Records per round trip. Default 100. cursor: Opaque bytes from a prior call's last record. Pass back to resume; pass ``None`` (default) to start fresh. @@ -580,6 +650,7 @@ async def drop_deprecated_indexes(self, deprecated: Dict[str, List[str]]) -> Non "encode_cursor", "decode_cursor", "finalize_find_results", + "resolve_sort_value", ] diff --git a/jvspatial/db/postgres.py b/jvspatial/db/postgres.py index 98c8e3d..690615b 100644 --- a/jvspatial/db/postgres.py +++ b/jvspatial/db/postgres.py @@ -810,11 +810,19 @@ async def find( else: order_clause = "" + # The sort pushdown may have failed (``translate_sort`` returns None + # for an unsafe field path). The ordering then has to happen in + # memory — which means the LIMIT must NOT be pushed down: the + # database would return an arbitrary N rows and we would order that + # arbitrary subset instead of the true top N. SQLite + # (``sqlite.py``) and DynamoDB withhold it for the same reason. + sort_in_memory = bool(sort) and sort_sql is None and vec_field is None + effective_limit = limit if vec_limit is not None and (limit is None or vec_limit < limit): effective_limit = vec_limit limit_clause = "" - if effective_limit is not None: + if effective_limit is not None and not sort_in_memory: params = list(params) + [int(effective_limit)] limit_clause = f" LIMIT ${len(params)}" @@ -825,10 +833,11 @@ async def find( rows = await conn.fetch(sql, *params) records = [self._record_from_row(r) for r in rows] - # Sort pushdown may have failed (translate_sort returned None); - # honor it in-memory in that case so the contract still holds. - if sort and sort_sql is None: - records = finalize_find_results(records, sort=sort, limit=None) + # Vector queries keep their distance ordering: re-sorting by the + # user's ``sort`` here would discard the ORDER BY the comment above + # says wins. + if sort_in_memory: + records = finalize_find_results(records, sort=sort, limit=limit) return records async def count( @@ -1795,8 +1804,12 @@ async def find( clauses = [where_sql] if where_sql else [] where_clause = f" WHERE {' AND '.join(clauses)}" if clauses else "" order_clause = f" ORDER BY {sort_sql}" if sort_sql else "" + # Withhold the LIMIT when the sort has to happen in memory, so we + # order the full match set rather than an arbitrary N rows. Mirrors + # ``PostgresDB.find``. + sort_in_memory = bool(sort) and sort_sql is None limit_clause = "" - if limit is not None: + if limit is not None and not sort_in_memory: params = list(params) + [int(limit)] limit_clause = f" LIMIT ${len(params)}" rows = await self._connection.fetch( @@ -1804,8 +1817,8 @@ async def find( *params, ) records = [self._db._record_from_row(r) for r in rows] - if sort and sort_sql is None: - records = finalize_find_results(records, sort=sort, limit=None) + if sort_in_memory: + records = finalize_find_results(records, sort=sort, limit=limit) return records async def commit(self) -> None: diff --git a/jvspatial/db/sqlite.py b/jvspatial/db/sqlite.py index 69f8c45..153edc3 100644 --- a/jvspatial/db/sqlite.py +++ b/jvspatial/db/sqlite.py @@ -470,6 +470,12 @@ async def find( serialized. """ connection = await self._get_connection() + # An empty sort spec is "no ordering requested", same as None. Without + # this the ``sort is None`` guard below fails and an empty list takes + # the untranslatable-sort branch: full-collection load, LIMIT applied + # only in memory. + if not sort: + sort = None translated = translate_query(query) if query else ("", []) if translated is not None: diff --git a/tests/core/test_graph_context.py b/tests/core/test_graph_context.py index e123ed5..c157278 100644 --- a/tests/core/test_graph_context.py +++ b/tests/core/test_graph_context.py @@ -904,3 +904,68 @@ async def test_find_page_accepts_dict_cursor_payload(self, temp_context): limit=2, ) assert len(page2) >= 1 + + # ---- dotted sort fields + the nulls-last tail -------------------------- + + @staticmethod + async def _walk_all(context, sort, limit=2): + """Page through everything, returning ids in visit order.""" + seen: list = [] + cursor = None + for _ in range(20): # guard against a non-terminating cursor loop + rows, cursor = await context.find_page( + "node", {}, sort=sort, after=cursor, limit=limit + ) + seen.extend(row["id"] for row in rows) + if not cursor: + return seen + raise AssertionError(f"pagination did not terminate; saw {seen}") + + @pytest.mark.asyncio + async def test_find_page_pages_through_dotted_sort_field(self, temp_context): + """A dotted sort field used to mint a None cursor on every page.""" + for n, ts in (("a", 5), ("b", 4), ("c", 3), ("d", 2), ("e", 1)): + await temp_context.database.save( + "node", {"id": n, "context": {"started_at": ts}} + ) + + seen = await self._walk_all(temp_context, [("context.started_at", -1)]) + assert seen == ["a", "b", "c", "d", "e"] + + @pytest.mark.asyncio + async def test_find_page_walks_the_missing_value_tail(self, temp_context): + """Records with no sort value sort last and must still be reachable.""" + for n, ts in (("a", 3), ("b", 2), ("c", 1)): + await temp_context.database.save( + "node", {"id": n, "context": {"started_at": ts}} + ) + for n in ("x", "y"): + await temp_context.database.save("node", {"id": n, "context": {}}) + + seen = await self._walk_all(temp_context, [("context.started_at", -1)]) + assert seen[:3] == ["a", "b", "c"] + assert sorted(seen[3:]) == ["x", "y"] + assert len(seen) == len(set(seen)) + + @pytest.mark.asyncio + async def test_find_page_ascending_also_walks_the_tail(self, temp_context): + for n, ts in (("a", 1), ("b", 2), ("c", 3)): + await temp_context.database.save( + "node", {"id": n, "context": {"started_at": ts}} + ) + for n in ("x", "y"): + await temp_context.database.save("node", {"id": n, "context": {}}) + + seen = await self._walk_all(temp_context, [("context.started_at", 1)]) + assert seen[:3] == ["a", "b", "c"] + assert sorted(seen[3:]) == ["x", "y"] + assert len(seen) == len(set(seen)) + + @pytest.mark.asyncio + async def test_find_page_flat_field_still_paginates(self, temp_context): + """Regression guard for the pre-existing flat-field path.""" + for n, ts in (("a", 3), ("b", 2), ("c", 1)): + await temp_context.database.save("node", {"id": n, "ts": ts}) + + seen = await self._walk_all(temp_context, [("ts", -1)]) + assert seen == ["a", "b", "c"] diff --git a/tests/core/test_pagination.py b/tests/core/test_pagination.py index 319c3b0..43ccdfb 100644 --- a/tests/core/test_pagination.py +++ b/tests/core/test_pagination.py @@ -222,12 +222,24 @@ async def mock_deserialize(cls, data): @pytest.mark.asyncio async def test_get_page_with_ordering(self, mock_context, sample_data): - """Test page retrieval with ordering.""" + """Test page retrieval with ordering. + + Uses ``mock_find_respecting_limit`` so the fake backend actually + honors ``sort``/``limit``. With a mock that ignores them, this test + passes on the pager's in-Python re-sort alone and would not notice + the DB-side ordering being wrong (or absent). + """ with patch( "jvspatial.core.context.get_default_context", return_value=mock_context ): mock_context.database.count.return_value = len(sample_data) - mock_context.database.find.return_value = sample_data + + async def find(collection, query, *, limit=None, sort=None): + return await mock_find_respecting_limit( + sample_data, collection, query, limit=limit, sort=sort + ) + + mock_context.database.find.side_effect = find async def mock_deserialize(cls, data): return PaginationTestObject(id=data["id"], **data["context"]) @@ -252,6 +264,55 @@ async def mock_deserialize(cls, data): values = [obj.value for obj in results] assert values == sorted(values, reverse=True) # Should be sorted descending + @pytest.mark.asyncio + async def test_ordering_with_missing_values_agrees_across_page_boundary( + self, mock_context + ): + """The in-page re-sort must not disagree with the DB-side slice. + + Records missing ``order_by`` sort last (SPEC §4.1). The pager used to + re-sort each page with ``.get(order_by, 0)``, placing them among the + real values — so a record could appear on two pages, or on none. + """ + records = [ + {"id": "1", "context": {"value": 30}}, + {"id": "2", "context": {}}, # missing -> sorts last + {"id": "3", "context": {"value": 10}}, + {"id": "4", "context": {"value": 20}}, + ] + + with patch( + "jvspatial.core.context.get_default_context", return_value=mock_context + ): + mock_context.database.count.return_value = len(records) + + async def find(collection, query, *, limit=None, sort=None): + return await mock_find_respecting_limit( + records, collection, query, limit=limit, sort=sort + ) + + mock_context.database.find.side_effect = find + + async def mock_deserialize(cls, data): + return PaginationTestObject( + id=data["id"], value=data["context"].get("value", -1) + ) + + mock_context._deserialize_entity.side_effect = mock_deserialize + + seen = [] + for page in (1, 2): + pager = ObjectPager( + PaginationTestObject, + page_size=2, + order_by="value", + order_direction="asc", + ) + seen.extend(obj.id for obj in await pager.get_page(page)) + + assert seen == ["3", "4", "1", "2"] + assert len(seen) == len(set(seen)) + class TestObjectPagerNavigation: """Test ObjectPager navigation methods.""" diff --git a/tests/db/test_find_sort_dotted.py b/tests/db/test_find_sort_dotted.py new file mode 100644 index 0000000..ea7dd3e --- /dev/null +++ b/tests/db/test_find_sort_dotted.py @@ -0,0 +1,65 @@ +"""Dotted-path sort keys for ``finalize_find_results``. + +The SQLite/Postgres sort pushdowns and Mongo's native sort already resolve +dotted field paths. These cases pin the in-memory fallback to the same +behavior so a ``sort=[("context.started_at", -1)]`` spec does not silently +degrade to "every key is ``None``" on backends that sort in Python. +""" + +from __future__ import annotations + +import tempfile + +import pytest + +from jvspatial.db.database import finalize_find_results +from jvspatial.db.jsondb import JsonDB + + +def _rows(): + return [ + {"id": "a", "context": {"started_at": "2026-01-03T00:00:00+00:00"}}, + {"id": "b", "context": {"started_at": "2026-01-01T00:00:00+00:00"}}, + {"id": "c", "context": {"started_at": "2026-01-02T00:00:00+00:00"}}, + ] + + +def test_finalize_find_sorts_dotted_context_field_desc(): + out = finalize_find_results(_rows(), sort=[("context.started_at", -1)], limit=2) + assert [r["id"] for r in out] == ["a", "c"] + + +def test_finalize_find_sorts_dotted_context_field_asc(): + out = finalize_find_results(_rows(), sort=[("context.started_at", 1)]) + assert [r["id"] for r in out] == ["b", "c", "a"] + + +def test_finalize_find_flat_field_unchanged(): + rows = [{"id": "2"}, {"id": "1"}, {"id": "3"}] + out = finalize_find_results(rows, sort=[("id", 1)], limit=2) + assert [r["id"] for r in out] == ["1", "2"] + + +def test_compound_sort_mixes_flat_and_dotted_keys(): + rows = [ + {"id": "a", "kind": "x", "context": {"n": 2}}, + {"id": "b", "kind": "x", "context": {"n": 1}}, + {"id": "c", "kind": "w", "context": {"n": 9}}, + ] + out = finalize_find_results(rows, sort=[("kind", 1), ("context.n", -1)]) + assert [r["id"] for r in out] == ["c", "a", "b"] + + +@pytest.mark.asyncio +async def test_jsondb_find_honors_dotted_sort_with_limit(): + """End-to-end through the JSON backend, which sorts in Python.""" + with tempfile.TemporaryDirectory() as tmpdir: + db = JsonDB(base_path=tmpdir) + for row in _rows(): + await db.save("interaction", row) + + out = await db.find( + "interaction", {}, sort=[("context.started_at", -1)], limit=2 + ) + + assert [r["id"] for r in out] == ["a", "c"] diff --git a/tests/db/test_find_sort_nulls_last.py b/tests/db/test_find_sort_nulls_last.py new file mode 100644 index 0000000..e0731d1 --- /dev/null +++ b/tests/db/test_find_sort_nulls_last.py @@ -0,0 +1,88 @@ +"""Missing sort values land last in both directions, on every backend path. + +``finalize_find_results`` sorts descending with ``reverse=True``, which used to +flip the ``None`` flag along with the values and float rows missing the sort +field to the *front*. Both SQL translators emit ``NULLS LAST`` for descending, +so a ``sort`` + ``limit`` "newest N" fetch returned real rows on +SQLite/Postgres/Mongo and a window of holes on the in-memory path. +""" + +from __future__ import annotations + +import tempfile + +import pytest + +from jvspatial.db._postgres_translate import translate_sort as pg_translate_sort +from jvspatial.db._sqlite_translate import translate_sort as sqlite_translate_sort +from jvspatial.db.database import finalize_find_results +from jvspatial.db.jsondb import JsonDB + + +def _rows_with_holes(): + return [ + {"id": "a", "context": {"started_at": "2026-01-03T00:00:00+00:00"}}, + {"id": "missing"}, + {"id": "b", "context": {"started_at": "2026-01-01T00:00:00+00:00"}}, + {"id": "empty", "context": {}}, + {"id": "c", "context": {"started_at": "2026-01-02T00:00:00+00:00"}}, + ] + + +@pytest.mark.parametrize("direction", [1, -1]) +def test_missing_flat_field_sorts_last_in_both_directions(direction): + rows = [{"id": "a", "v": 1}, {"id": "none"}, {"id": "b", "v": 2}] + out = finalize_find_results(rows, sort=[("v", direction)]) + assert out[-1]["id"] == "none" + + +@pytest.mark.parametrize("direction", [1, -1]) +def test_missing_dotted_path_sorts_last_in_both_directions(direction): + out = finalize_find_results( + _rows_with_holes(), sort=[("context.started_at", direction)] + ) + assert {r["id"] for r in out[-2:]} == {"missing", "empty"} + + +def test_newest_n_limit_window_holds_real_rows(): + """The motivating case: descending + limit must not return holes.""" + out = finalize_find_results( + _rows_with_holes(), sort=[("context.started_at", -1)], limit=2 + ) + assert [r["id"] for r in out] == ["a", "c"] + + +def test_non_dict_segment_sorts_last_instead_of_raising(): + rows = [ + {"id": "scalar", "context": "not-a-dict"}, + {"id": "ok", "context": {"started_at": "2026-01-01T00:00:00+00:00"}}, + ] + out = finalize_find_results(rows, sort=[("context.started_at", -1)]) + assert [r["id"] for r in out] == ["ok", "scalar"] + + +@pytest.mark.parametrize("direction", [1, -1]) +def test_sql_translators_agree_on_nulls_last(direction): + """Pin the contract the in-memory path is now matching.""" + sqlite_frag = sqlite_translate_sort([("v", direction)]) + pg_frag = pg_translate_sort([("v", direction)]) + + # SQLite: leading ``IS NULL`` term sorts ASC, so 0 (non-NULL) precedes 1. + assert sqlite_frag is not None and sqlite_frag.startswith( + "(json_extract(data, '$.v') IS NULL)," + ) + assert pg_frag is not None and pg_frag.endswith("NULLS LAST") + + +@pytest.mark.asyncio +async def test_jsondb_descending_limit_skips_rows_missing_the_field(): + with tempfile.TemporaryDirectory() as tmpdir: + db = JsonDB(base_path=tmpdir) + for row in _rows_with_holes(): + await db.save("interaction", row) + + out = await db.find( + "interaction", {}, sort=[("context.started_at", -1)], limit=2 + ) + + assert [r["id"] for r in out] == ["a", "c"] diff --git a/tests/db/test_postgres_integration.py b/tests/db/test_postgres_integration.py index 871ed12..1ea2dd9 100644 --- a/tests/db/test_postgres_integration.py +++ b/tests/db/test_postgres_integration.py @@ -243,6 +243,41 @@ async def test_sort_pushdown(self, pg_db: "PostgresDB") -> None: out = await pg_db.find("node", {}, sort=[("context.score", 1)]) assert [r["id"] for r in out] == ["n.1", "n.2", "n.5", "n.7", "n.8"] + async def test_untranslatable_sort_with_limit_returns_true_top_n( + self, pg_db: "PostgresDB" + ) -> None: + """LIMIT must not be pushed when the sort falls back to memory. + + ``my-score`` has a hyphen, so ``translate_sort`` refuses it and the + ordering happens in Python. Pushing the LIMIT would order an + arbitrary N rows instead of the true top N. + """ + for score in (5, 2, 8, 1, 7): + await pg_db.save( + "node", + { + "id": f"n.{score}", + "entity": "n", + "context": {"my-score": score}, + }, + ) + out = await pg_db.find("node", {}, sort=[("context.my-score", -1)], limit=2) + assert [r["id"] for r in out] == ["n.8", "n.7"] + + async def test_sort_places_missing_values_last(self, pg_db: "PostgresDB") -> None: + """NULLS LAST in both directions — SPEC §4.1 find sort contract.""" + for score in (5, 2): + await pg_db.save( + "node", + {"id": f"n.{score}", "entity": "n", "context": {"score": score}}, + ) + await pg_db.save("node", {"id": "n.hole", "entity": "n", "context": {}}) + + asc = await pg_db.find("node", {}, sort=[("context.score", 1)]) + desc = await pg_db.find("node", {}, sort=[("context.score", -1)]) + assert asc[-1]["id"] == "n.hole" + assert desc[-1]["id"] == "n.hole" + # ---- Atomic find_one_and_update -------------------------------------------- diff --git a/tests/db/test_postgres_sort_limit.py b/tests/db/test_postgres_sort_limit.py new file mode 100644 index 0000000..60f76c7 --- /dev/null +++ b/tests/db/test_postgres_sort_limit.py @@ -0,0 +1,153 @@ +"""Postgres must not push LIMIT when the sort pushdown failed. + +``translate_sort`` returns ``None`` for a field path it cannot safely +interpolate (anything outside ``[A-Za-z_][A-Za-z0-9_]*`` per segment). The +ordering then happens in memory — so pushing the LIMIT would hand the +in-memory sort an arbitrary N rows and return "the top N of an arbitrary +subset" instead of the true top N. + +``SQLiteDB.find`` and ``DynamoDB.find`` already withhold the LIMIT in this +situation; these cases pin Postgres to the same behavior. Stubbed pool — no +live database required. The DSN-gated end-to-end case lives in +``test_postgres_integration.py``. +""" + +from __future__ import annotations + +import contextlib +from typing import Any, Dict, List, Optional + +import pytest + +# PostgresDB imports asyncpg at module load. +pytest.importorskip("asyncpg") + +from jvspatial.db.postgres import PostgresDB # noqa: E402 + + +class _FakeConn: + """Records every SQL string it is handed and replays canned rows.""" + + def __init__(self, rows: List[Dict[str, Any]]) -> None: + self._rows = rows + self.queries: List[str] = [] + self.params: List[Any] = [] + + async def fetch(self, sql: str, *params: Any) -> List[Dict[str, Any]]: + self.queries.append(sql) + self.params.append(params) + # Mimic a LIMIT the database would have applied itself. + rows = self._rows + if " LIMIT " in sql and params: + rows = rows[: int(params[-1])] + return [{"data": r} for r in rows] + + async def execute(self, *_args: Any, **_kwargs: Any) -> None: + return None + + +def _stub_db(rows: List[Dict[str, Any]]) -> tuple[PostgresDB, _FakeConn]: + db = PostgresDB(dsn="postgresql://stub/stub") + conn = _FakeConn(rows) + + class _FakePool: + @contextlib.asynccontextmanager + async def acquire(self): # type: ignore[no-untyped-def] + yield conn + + async def _ensure_pool() -> Any: + return _FakePool() + + db._ensure_pool = _ensure_pool # type: ignore[assignment] + # Skip DDL; the fake pool has no real table behind it. + db._collections_bootstrapped.add("interaction") + return db, conn + + +def _rows() -> List[Dict[str, Any]]: + """Deliberately stored in an order that is not the sorted order.""" + return [ + {"id": "old", "context": {"my-field": 1}}, + {"id": "newest", "context": {"my-field": 9}}, + {"id": "mid", "context": {"my-field": 5}}, + ] + + +@pytest.mark.asyncio +async def test_untranslatable_sort_withholds_limit_pushdown(): + db, conn = _stub_db(_rows()) + + # "my-field" contains a hyphen -> _safe_field_path rejects it -> + # translate_sort returns None. + out = await db.find("interaction", {}, sort=[("context.my-field", -1)], limit=2) + + assert "LIMIT" not in conn.queries[-1] + assert "ORDER BY" not in conn.queries[-1] + # The true top 2, not the first 2 rows the table happened to yield. + assert [r["id"] for r in out] == ["newest", "mid"] + + +@pytest.mark.asyncio +async def test_translatable_sort_still_pushes_order_by_and_limit(): + db, conn = _stub_db(_rows()) + + await db.find("interaction", {}, sort=[("context.started_at", -1)], limit=2) + + assert "ORDER BY" in conn.queries[-1] + assert "LIMIT" in conn.queries[-1] + + +@pytest.mark.asyncio +async def test_no_sort_still_pushes_limit(): + """Without a sort there is no ordering to preserve — keep the pushdown.""" + db, conn = _stub_db(_rows()) + + out = await db.find("interaction", {}, limit=2) + + assert "LIMIT" in conn.queries[-1] + assert len(out) == 2 + + +@pytest.mark.asyncio +async def test_untranslatable_sort_without_limit_is_unaffected(): + db, conn = _stub_db(_rows()) + + out = await db.find("interaction", {}, sort=[("context.my-field", 1)]) + + assert "LIMIT" not in conn.queries[-1] + assert [r["id"] for r in out] == ["old", "mid", "newest"] + + +@pytest.mark.asyncio +async def test_untranslatable_sort_places_missing_values_last_under_limit(): + """The nulls-last contract survives the in-memory limit path.""" + rows = [ + {"id": "hole"}, + {"id": "newest", "context": {"my-field": 9}}, + {"id": "mid", "context": {"my-field": 5}}, + ] + db, _conn = _stub_db(rows) + + out = await db.find("interaction", {}, sort=[("context.my-field", -1)], limit=2) + + assert [r["id"] for r in out] == ["newest", "mid"] + + +class _FakeTxConn(_FakeConn): + async def fetchrow(self, *_args: Any, **_kwargs: Any) -> Optional[Any]: + return None + + +@pytest.mark.asyncio +async def test_transaction_find_withholds_limit_pushdown_too(): + """``PostgresTransaction.find`` duplicates the query builder verbatim.""" + from jvspatial.db.postgres import PostgresTransaction + + db, _ = _stub_db(_rows()) + conn = _FakeTxConn(_rows()) + tx = PostgresTransaction(db, conn, transaction=None) + + out = await tx.find("interaction", {}, sort=[("context.my-field", -1)], limit=2) + + assert "LIMIT" not in conn.queries[-1] + assert [r["id"] for r in out] == ["newest", "mid"] diff --git a/tests/db/test_sqlite_sort.py b/tests/db/test_sqlite_sort.py new file mode 100644 index 0000000..3c4b915 --- /dev/null +++ b/tests/db/test_sqlite_sort.py @@ -0,0 +1,116 @@ +"""End-to-end SQLite sort behavior — both the pushdown and the fallback. + +``tests/db/test_find_sort_nulls_last.py`` only string-matches the SQL +fragments the translators emit. These cases run a real ``SQLiteDB`` so the +ordering contract in SPEC §4.1 is asserted against actual query results, +through both branches of ``SQLiteDB.find``: + +* ``translate_sort`` succeeds → ORDER BY + LIMIT pushed into SQL +* ``translate_sort`` returns None (unsafe field path) → full match set + loaded and ordered by ``finalize_find_results`` + +Both must produce the same ordering. +""" + +import pytest + +import jvspatial.db.sqlite as sqlite_module +from jvspatial.db.sqlite import SQLiteDB + +# A hyphen is outside ``_SAFE_SEGMENT_RE``, so translate_sort refuses the +# path and find() takes the in-memory branch. +PUSHED = "context.started_at" +FALLBACK = "context.started-at" + + +@pytest.fixture +async def db(): + database = SQLiteDB(db_path=":memory:") + yield database + await database.close() + + +async def _seed(database, field): + """Three valued records plus two with no value, saved out of order.""" + await database.save("node", {"id": "mid", "context": {field: 2}}) + await database.save("node", {"id": "hole", "context": {}}) + await database.save("node", {"id": "newest", "context": {field: 3}}) + await database.save("node", {"id": "oldest", "context": {field: 1}}) + await database.save("node", {"id": "nulled", "context": {field: None}}) + + +@pytest.mark.parametrize("path", [PUSHED, FALLBACK]) +async def test_descending_orders_values_then_missing(db, path): + await _seed(db, path.split(".", 1)[1]) + + out = await db.find("node", {}, sort=[(path, -1)]) + + assert [r["id"] for r in out[:3]] == ["newest", "mid", "oldest"] + assert sorted(r["id"] for r in out[3:]) == ["hole", "nulled"] + + +@pytest.mark.parametrize("path", [PUSHED, FALLBACK]) +async def test_ascending_orders_values_then_missing(db, path): + await _seed(db, path.split(".", 1)[1]) + + out = await db.find("node", {}, sort=[(path, 1)]) + + assert [r["id"] for r in out[:3]] == ["oldest", "mid", "newest"] + assert sorted(r["id"] for r in out[3:]) == ["hole", "nulled"] + + +@pytest.mark.parametrize("path", [PUSHED, FALLBACK]) +async def test_limit_returns_the_true_top_n(db, path): + """The motivating case — a "newest 2" fetch must not return holes.""" + await _seed(db, path.split(".", 1)[1]) + + out = await db.find("node", {}, sort=[(path, -1)], limit=2) + + assert [r["id"] for r in out] == ["newest", "mid"] + + +async def test_pushdown_and_fallback_agree(db): + """Same data under both branches produces the same order.""" + await db.save( + "node", + {"id": "a", "context": {"started_at": 1, "started-at": 1}}, + ) + await db.save( + "node", + {"id": "b", "context": {"started_at": 3, "started-at": 3}}, + ) + await db.save("node", {"id": "c", "context": {}}) + + pushed = await db.find("node", {}, sort=[(PUSHED, -1)]) + fallback = await db.find("node", {}, sort=[(FALLBACK, -1)]) + + assert [r["id"] for r in pushed] == [r["id"] for r in fallback] + assert [r["id"] for r in pushed] == ["b", "a", "c"] + + +async def test_empty_sort_spec_pushes_limit_instead_of_loading_everything( + db, monkeypatch +): + """``sort=[]`` must not divert into the untranslatable-sort branch. + + It used to fail the ``sort is None`` guard, so an empty list loaded the + whole collection and applied the LIMIT in memory. The row count is + identical either way, so assert the branch: the pushdown path returns + without consulting ``finalize_find_results`` at all. + """ + for i in range(5): + await db.save("node", {"id": str(i), "context": {}}) + + calls = [] + real = sqlite_module.finalize_find_results + + def spy(records, **kwargs): + calls.append(len(records)) + return real(records, **kwargs) + + monkeypatch.setattr(sqlite_module, "finalize_find_results", spy) + + out = await db.find("node", {}, sort=[], limit=2) + + assert len(out) == 2 + assert calls == [] # never fell back to the in-memory path