From 2c48fa6f990dd3930f35ce6297e9778d9e6b0a83 Mon Sep 17 00:00:00 2001 From: Eldon Marks Date: Fri, 31 Jul 2026 11:27:19 -0400 Subject: [PATCH 1/8] fix(db): resolve dotted field paths in in-memory find sort MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `_find_sort_key` looked up sort fields with a flat `record.get(field)`, so a spec like `sort=[("context.started_at", -1)]` produced `None` for every row and left results in arbitrary order. The SQLite and Postgres sort pushdowns (`translate_sort`) and Mongo's native sort already resolve dotted paths, so the same query ordered correctly on those backends and silently did not on JsonDB/DynamoDB — or on SQLite and Postgres whenever the query fell back to the in-memory path. Resolve dotted paths in `_find_sort_key`; a non-dict segment along the path yields `None` rather than raising. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 12 ++++++ jvspatial/db/database.py | 21 +++++++++- tests/db/test_find_sort_dotted.py | 65 +++++++++++++++++++++++++++++++ 3 files changed, 96 insertions(+), 2 deletions(-) create mode 100644 tests/db/test_find_sort_dotted.py diff --git a/CHANGELOG.md b/CHANGELOG.md index aefb261..392c6be 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,18 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed + +- **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. + ## [0.0.11] - 2026-07-02 ### Fixed diff --git a/jvspatial/db/database.py b/jvspatial/db/database.py index 5a16c21..a575013 100644 --- a/jvspatial/db/database.py +++ b/jvspatial/db/database.py @@ -71,8 +71,25 @@ def all_saved(self) -> bool: 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) + """Sort key: non-``None`` values first, then by value (with ``None`` last). + + 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. + """ + value: Any + if "." not in field: + value = record.get(field) + else: + value = record + for part in field.split("."): + if not isinstance(value, dict): + value = None + break + value = value.get(part) return (value is None, value) 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"] From bb024eeb928a36551f67e957fa0bf70353135fb4 Mon Sep 17 00:00:00 2001 From: Eldon Marks Date: Fri, 31 Jul 2026 11:29:56 -0400 Subject: [PATCH 2/8] fix(db): place records missing the sort field last when sorting descending MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `finalize_find_results` sorts descending via `reverse=True`, which flipped `_find_sort_key`'s `(value is None, value)` flag along with the values and floated records missing the sort field to the front. Both SQL translators emit NULLS LAST for descending — SQLite via a leading `(col IS NULL)` term that is itself sorted ASC, Postgres via an explicit `DESC NULLS LAST` — and Mongo sorts missing values last. A "newest N" `sort` + `limit` fetch therefore returned real rows on SQLite/Postgres/Mongo and a window of records missing the field on the in-memory path. Invert the None flag for descending sorts so missing values land last in both directions. All missing values share a flag, so None is never compared against a real value. Also correct the comment in `_sqlite_translate.translate_sort`, which asserted the in-memory sort already put NULLs last. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 9 +++ jvspatial/db/_sqlite_translate.py | 8 +-- jvspatial/db/database.py | 24 ++++++-- tests/db/test_find_sort_nulls_last.py | 88 +++++++++++++++++++++++++++ 4 files changed, 119 insertions(+), 10 deletions(-) create mode 100644 tests/db/test_find_sort_nulls_last.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 392c6be..b7b2196 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 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. ## [0.0.11] - 2026-07-02 diff --git a/jvspatial/db/_sqlite_translate.py b/jvspatial/db/_sqlite_translate.py index 402315a..8d45672 100644 --- a/jvspatial/db/_sqlite_translate.py +++ b/jvspatial/db/_sqlite_translate.py @@ -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 a575013..e23101e 100644 --- a/jvspatial/db/database.py +++ b/jvspatial/db/database.py @@ -70,8 +70,10 @@ 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). +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 @@ -79,6 +81,12 @@ def _find_sort_key(record: Dict[str, Any], field: str) -> Tuple[bool, Any]: 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: Any if "." not in field: @@ -90,7 +98,8 @@ def _find_sort_key(record: Dict[str, Any], field: str) -> Tuple[bool, Any]: value = None break value = value.get(part) - return (value is None, value) + missing = value is None + return (not missing if descending else missing, value) def _normalize_id_query(query: Dict[str, Any]) -> Dict[str, Any]: @@ -121,15 +130,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] 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"] From 96c1c8a7a04b413ede9e62dcfb4eca31e65ff8f2 Mon Sep 17 00:00:00 2001 From: Eldon Marks Date: Fri, 31 Jul 2026 11:30:45 -0400 Subject: [PATCH 3/8] docs(spec): document the find() sort contract adapters must match Records the two properties every adapter has to produce identically whether it pushes the sort into the backend or falls back to `finalize_find_results`: dotted paths resolve into nested documents, and missing values sort last in both directions. Co-Authored-By: Claude Opus 5 --- SPEC.md | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/SPEC.md b/SPEC.md index a20ffcf..8709015 100644 --- a/SPEC.md +++ b/SPEC.md @@ -224,6 +224,21 @@ Adapters declare capabilities as class attributes: Callers branching on capabilities should test the flag, not the adapter class. +#### `find` sort contract + +`sort` is a list of `(field, direction)` with `1` ascending and `-1` descending. +Every adapter must produce the same ordering regardless of whether it pushes the +sort into the backend or falls back to +`finalize_find_results` (`jvspatial/db/database.py:109`): + +- **Dotted paths** (`context.started_at`) resolve into nested documents. A + non-dict segment along the path resolves to "missing", not an error. +- **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. + ### 4.3 Built-in adapters | Adapter | File | Transactions | Notes | From 2166da8584fecc498da7fabdc89a949d8b07da0e Mon Sep 17 00:00:00 2001 From: Eldon Marks Date: Fri, 31 Jul 2026 12:11:53 -0400 Subject: [PATCH 4/8] fix(db): postgres withholds LIMIT when the sort falls back to memory `translate_sort` returns None for a field path it cannot safely interpolate (e.g. `context.my-field`), so the ordering has to happen in `finalize_find_results`. The LIMIT was still pushed into the SQL, 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 rather than the true top 10. Withhold the LIMIT whenever the sort falls back to memory, then apply it after sorting. `SQLiteDB.find` and `DynamoDB.find` already did this. Also stop re-sorting vector (`$near`) results by the user's `sort`. The comment says the vector ORDER BY wins when both are present, but forcing `sort_sql = None` for vector queries meant the in-memory branch fired and discarded the distance ordering. Applies to both `PostgresDB.find` and the verbatim copy in `PostgresTransaction.find`. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 12 ++ jvspatial/db/postgres.py | 29 +++-- tests/db/test_postgres_integration.py | 35 ++++++ tests/db/test_postgres_sort_limit.py | 153 ++++++++++++++++++++++++++ 4 files changed, 221 insertions(+), 8 deletions(-) create mode 100644 tests/db/test_postgres_sort_limit.py diff --git a/CHANGELOG.md b/CHANGELOG.md index b7b2196..a55af8c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,18 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- **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 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/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"] From c6ba22bf81bc7b86bc435d46662305a9c6cd286b Mon Sep 17 00:00:00 2001 From: Eldon Marks Date: Fri, 31 Jul 2026 12:14:44 -0400 Subject: [PATCH 5/8] fix(core): find_page resolves dotted cursors and reaches the missing-value tail MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two defects in `GraphContext.find_page` keyset pagination. The cursor payload was minted with a flat `last.get(primary_field)`. For a dotted sort field the value is nested, so every cursor encoded `sort: None` and the next page compared against it — raising `TypeError: '>' not supported between instances of 'int' and 'NoneType'` out of `QueryEngine` on JsonDB rather than paging. Mint the cursor with `resolve_sort_value`, the same path walk the adapters and `finalize_find_results` use. Records missing the sort field sort last in both directions, but the keyset filter `{field: {"$lt": value}}` can never match a record that has no value, so iteration stopped at the last record that had one and the trailing run was unreachable. Add a `{field: None}` branch — which matches both an explicit null and a missing key — and, when the cursor itself was minted inside that run, walk it by `id` alone. Extract the path walk from `_find_sort_key` into `resolve_sort_value` and export it so cursor minting and adapter sorting cannot drift apart. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 18 +++++++++ jvspatial/core/context.py | 46 ++++++++++++++++++---- jvspatial/db/database.py | 37 +++++++++++++----- tests/core/test_graph_context.py | 65 ++++++++++++++++++++++++++++++++ 4 files changed, 149 insertions(+), 17 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a55af8c..81f6f08 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,8 +7,26 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### 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__`. + ### Fixed +- **`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 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/db/database.py b/jvspatial/db/database.py index e23101e..9d5ea55 100644 --- a/jvspatial/db/database.py +++ b/jvspatial/db/database.py @@ -70,6 +70,31 @@ def all_saved(self) -> bool: logger = logging.getLogger(__name__) +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]: @@ -88,16 +113,7 @@ def _find_sort_key( ``NULLS LAST`` both SQL translators emit. All missing values share a flag, so ``None`` is never compared against a real value. """ - value: Any - if "." not in field: - value = record.get(field) - else: - value = record - for part in field.split("."): - if not isinstance(value, dict): - value = None - break - value = value.get(part) + value = resolve_sort_value(record, field) missing = value is None return (not missing if descending else missing, value) @@ -609,6 +625,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/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"] From 1ddf35b06b6201d1a4c0e2623f32efb201a6ea51 Mon Sep 17 00:00:00 2001 From: Eldon Marks Date: Fri, 31 Jul 2026 12:16:10 -0400 Subject: [PATCH 6/8] fix(core): pager in-page re-sort agrees with the database slice MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `ObjectPager.get_page` fetched a slice with a DB-side `sort` + `limit`, then re-sorted that page in Python with `item.get("context", {}).get(order_by, 0)`. A record missing `order_by` became `0` and sorted among the real values, while the slice that produced the page had already placed it in the trailing missing-value run. The two orderings disagree, so a record could appear on two pages or on none. The blanket `contextlib.suppress(KeyError, TypeError)` around it also left a page silently in DB order whenever the key raised. Route the safety net through `finalize_find_results` instead. It is now a genuine no-op when the backend honored the sort, and matches the nulls-last contract when it did not. `test_get_page_with_ordering` was passing on the in-Python re-sort alone — its mock ignored `sort` and `limit` entirely, so it could not have caught this. Switch it to `mock_find_respecting_limit`, which the keyset tests in the same file already use. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 11 ++++++ jvspatial/core/pager.py | 18 +++++----- tests/core/test_pagination.py | 65 +++++++++++++++++++++++++++++++++-- 3 files changed, 84 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 81f6f08..73659e0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- **`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 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/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.""" From 96f13fd13f59c7f8aa632edfce9cc7832d1164ba Mon Sep 17 00:00:00 2001 From: Eldon Marks Date: Fri, 31 Jul 2026 12:17:45 -0400 Subject: [PATCH 7/8] docs: correct stale NULL-ordering claims and scope the find sort contract MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three docstrings still said "NULLs sort last for ascending, first for descending, mirroring finalize_find_results" — the opposite of what the code emits and of the contract: the module docstring and `translate_sort` docstring in `_sqlite_translate.py`, and `translate_sort` in `_postgres_translate.py`. Only the inline comment was corrected when the in-memory ordering was fixed. Move the `find` sort contract from SPEC §4.2 (capability flags) to §4.1, beside the `Database` method table it governs, and extend it with the rule that `limit` must not be pushed down when the sort is not, plus a Known divergences table: MongoDB's native `cursor.sort()` places missing values first on ascending sorts (documented, not normalized — compensating needs an aggregation pipeline on every find), array-index path segments, and heterogeneous value types. Document the contract on `Database.find` for adapter authors, and correct `Database.find_iter`, which claimed a composite `(sort_value, id)` cursor it does not implement — it tracks `id` only, so a non-`id` sort drops records that sort late but carry a lower `id`. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 19 +++++++++++++ SPEC.md | 41 ++++++++++++++++++++--------- jvspatial/db/_postgres_translate.py | 8 +++--- jvspatial/db/_sqlite_translate.py | 10 +++---- jvspatial/db/database.py | 33 ++++++++++++++++++++--- 5 files changed, 85 insertions(+), 26 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 73659e0..592b249 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,25 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 cursor logic resolve a sort field the same way. Added to the module's `__all__`. +### 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`. + ### Fixed - **`ObjectPager` re-sorted each page with a key that disagreed with the diff --git a/SPEC.md b/SPEC.md index 8709015..ad281cc 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,28 +216,43 @@ 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` | -### 4.2 Capability flags - -Adapters declare capabilities as class attributes: - -- `supports_transactions: bool` — `True` for MongoDB (replica set); `False` for SQLite (best-effort), JSON, DynamoDB. - -Callers branching on capabilities should test the flag, not the adapter class. - #### `find` sort contract `sort` is a list of `(field, direction)` with `1` ascending and `-1` descending. -Every adapter must produce the same ordering regardless of whether it pushes the -sort into the backend or falls back to -`finalize_find_results` (`jvspatial/db/database.py:109`): +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. + 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: + +- `supports_transactions: bool` — `True` for MongoDB (replica set); `False` for SQLite (best-effort), JSON, DynamoDB. + +Callers branching on capabilities should test the flag, not the adapter class. ### 4.3 Built-in adapters 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 8d45672..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 diff --git a/jvspatial/db/database.py b/jvspatial/db/database.py index 9d5ea55..525ee49 100644 --- a/jvspatial/db/database.py +++ b/jvspatial/db/database.py @@ -257,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 @@ -302,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. From 92116aa12f8f53f37c40aadd0851d807078df39f Mon Sep 17 00:00:00 2001 From: Eldon Marks Date: Fri, 31 Jul 2026 12:19:26 -0400 Subject: [PATCH 8/8] test(db): sqlite sort e2e for both branches; normalize empty sort spec MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `test_find_sort_nulls_last.py` only string-matches the SQL fragments the translators emit, so the two backends the contract binds hardest had no runtime assertion. Add end-to-end `SQLiteDB` coverage that runs the same data through both branches of `find` — ORDER BY pushed into SQL, and the in-memory `finalize_find_results` fallback reached via an unsafe field path — and asserts they agree on values-then-missing ordering in both directions and on the true top N under `limit`. Also normalize a falsy `sort` to `None` in `SQLiteDB.find`. `sort=[]` failed the `sort is None` guard and took the untranslatable-sort branch, loading the whole collection and applying `limit` in memory. Same rows, needless work. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 5 ++ jvspatial/db/sqlite.py | 6 ++ tests/db/test_sqlite_sort.py | 116 +++++++++++++++++++++++++++++++++++ 3 files changed, 127 insertions(+) create mode 100644 tests/db/test_sqlite_sort.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 592b249..98a2470 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -35,6 +35,11 @@ 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 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/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