diff --git a/CHANGELOG.md b/CHANGELOG.md index 6f7d1c1..aefb261 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [0.0.11] - 2026-07-02 + +### Fixed + +- **Single-hop `find_connected_nodes` fast path dropped valid neighbors** (`jvspatial/core/entities/node.py`). Two regressions introduced with the 0.0.10 fast path, both invisible to the existing suite because `JsonDB` exposes no `find_connected_nodes` (so tests fell back to the slow edge scan): + - *Limit-before-filter:* `limit` was pushed into the DB scan **before** the Python node-type filter, so `node(node=T)` / `nodes(node=T, limit=n)` could return nothing when a non-matching neighbor sorted first, even though matching neighbors existed. `limit` is now applied to the DB scan only when there is no node filter; otherwise it is applied after filtering. + - *Subtype resolution under an entity-name collision:* rows were deserialized with the base `Node` class, so when two `Node` subclasses persisted in one database shared an entity name (e.g. an app `User` and an embedded-agent `User`), `find_subclass_by_name` returned the first global match and `isinstance`-based filtering silently dropped the valid neighbor. The fast path now hydrates using the caller's requested concrete type as the resolution hint. +- Regression coverage: `tests/core/test_connected_nodes_fast_path.py` installs a `find_connected_nodes` shim so the fast path is exercised (the stock `JsonDB` never did). + ## [0.0.10] - 2026-07-02 ### Added diff --git a/jvspatial/core/entities/node.py b/jvspatial/core/entities/node.py index 5e16fb4..8576ae8 100644 --- a/jvspatial/core/entities/node.py +++ b/jvspatial/core/entities/node.py @@ -677,18 +677,46 @@ async def _node_query( edge_coll = context._get_collection_name( context._get_entity_type_code(edge_cls) ) + # Only push ``limit`` into the DB scan when there is no node + # filter. With a node filter the type match happens in Python + # below, so a DB-side limit would truncate the candidate set + # BEFORE filtering (e.g. limit=1 fetches one neighbor that may + # not match the type, yielding an empty result even though + # matching neighbors exist). Fetch all, filter, then slice. + db_limit = limit if node_filter is None else None records = await find_connected( node_coll, edge_coll, self.id, direction=direction, edge_entity=edge_entity, - limit=limit, + limit=db_limit, ) + # Deserialize with the caller's requested concrete type as the + # hint (not the base ``Node``) when the node filter names one. + # ``_deserialize_entity`` resolves the stored ``entity`` name + # against this class's subtree first, so when two Node + # subclasses across embedded graphs share an entity name + # (e.g. an app ``User`` and an embedded-agent ``User``), the + # row hydrates as the class the caller asked for instead of the + # first global name match — otherwise ``_matches_node_filter``'s + # ``isinstance`` check silently drops a valid neighbor. + deser_class: type = NodeClass + if isinstance(node_filter, type): + deser_class = node_filter + elif ( + isinstance(node_filter, list) + and len(node_filter) == 1 + and isinstance(node_filter[0], type) + ): + deser_class = node_filter[0] + connected_nodes: List["Node"] = [] for data in records: try: - node_obj = await context._deserialize_entity(NodeClass, data) + node_obj: Optional["Node"] = await context._deserialize_entity( + deser_class, data + ) if node_obj: connected_nodes.append(node_obj) await context._add_to_cache(node_obj.id, node_obj) diff --git a/jvspatial/version.py b/jvspatial/version.py index 7ffc32a..f1c5450 100644 --- a/jvspatial/version.py +++ b/jvspatial/version.py @@ -9,4 +9,4 @@ # - MAJOR: Breaking changes # - MINOR: New features, backward compatible # - PATCH: Bug fixes, backward compatible -__version__ = "0.0.10" +__version__ = "0.0.11" diff --git a/tests/core/test_connected_nodes_fast_path.py b/tests/core/test_connected_nodes_fast_path.py new file mode 100644 index 0000000..234989d --- /dev/null +++ b/tests/core/test_connected_nodes_fast_path.py @@ -0,0 +1,145 @@ +"""Regression tests for the single-hop ``find_connected_nodes`` fast path. + +Both bugs below shipped in 0.0.10 because the stock ``JsonDB`` used by the +rest of the suite exposes no ``find_connected_nodes`` method, so ``_node_query`` +silently fell back to the slow edge-scan path and the fast path went untested. +These tests install a minimal ``find_connected_nodes`` shim to exercise it. + +Bug 1 — limit-before-filter: ``limit`` was pushed into the DB scan *before* the +Python node-type filter, so ``nodes(node=T, limit=1)`` could return nothing when +the first raw neighbor was a different type, even though matching neighbors +existed. + +Bug 2 — subtype resolution under an entity-name collision: the fast path +deserialized every row with the base ``Node`` class, so when two ``Node`` +subclasses shared an entity name (e.g. an app ``User`` and an embedded-agent +``User`` persisted in one database) ``find_subclass_by_name`` returned the first +global match and ``_matches_node_filter``'s ``isinstance`` check dropped the +valid neighbor. +""" + +from __future__ import annotations + +from typing import Any, Dict, List, Optional + +import pytest + +from jvspatial.core.context import GraphContext, set_default_context +from jvspatial.core.entities.node import Node +from jvspatial.db.jsondb import JsonDB + + +class _FastJsonDB(JsonDB): + """JsonDB plus a ``find_connected_nodes`` so the fast path is exercised. + + Mirrors the Postgres backend contract: strict source/target endpoints per + ``direction`` and DB-side ``limit`` applied to the raw (unfiltered) rows. + """ + + async def find_connected_nodes( + self, + node_collection: str, + edge_collection: str, + node_id: str, + *, + direction: str = "out", + edge_entity: Optional[str] = None, + limit: Optional[int] = None, + ) -> List[Dict[str, Any]]: + edges = await self.find(edge_collection, {}) + rows: List[Dict[str, Any]] = [] + for e in edges: + src, tgt = e.get("source"), e.get("target") + if direction == "out" and src == node_id: + other = tgt + elif direction == "in" and tgt == node_id: + other = src + else: + continue + if edge_entity is not None and e.get("entity") != edge_entity: + continue + n = await self.get(node_collection, other) + if n is not None: + rows.append(n) + # Deterministic order (Postgres has no implicit ORDER BY, so the bug + # surfaces whenever a non-matching neighbor happens to sort first). + rows.sort(key=lambda r: r["id"]) + if limit is not None: + rows = rows[:limit] + return rows + + +class Widget(Node): + """A distinct child type used to prove type-filtered fetches.""" + + label: str = "" + + +class Gadget(Node): + """A different child type sharing the parent but not the filter.""" + + label: str = "" + + +# Two subclasses that intentionally share one entity name — the collision that +# broke subtype resolution on the fast path. +class AlphaAccount(Node): + __entity_name__ = "CollidingAccount" + label: str = "" + + +class BetaAccount(Node): + __entity_name__ = "CollidingAccount" + label: str = "" + + +@pytest.fixture +async def ctx(tmp_path): + context = GraphContext(database=_FastJsonDB(base_path=str(tmp_path))) + set_default_context(context) + yield context + + +@pytest.mark.asyncio +async def test_type_filter_with_limit_is_not_truncated_before_filtering(ctx): + """``nodes(node=Widget, limit=1)`` returns a Widget even when a non-Widget + neighbor sorts first — limit must apply after the type filter.""" + parent = Node() + # ids chosen so the non-matching Gadget sorts first — a DB-side limit of 1 + # would grab only the Gadget and then filter it out (the bug). + gadget = Gadget(id="n.Gadget.0000", label="g") + w1 = Widget(id="n.Widget.1111", label="w1") + w2 = Widget(id="n.Widget.2222", label="w2") + for n in (parent, gadget, w1, w2): + await ctx.save(n) + await parent.connect(gadget) + await parent.connect(w1) + await parent.connect(w2) + + one = await parent.nodes(node=Widget, direction="out", limit=1) + assert len(one) == 1 + assert isinstance(one[0], Widget) + + got = await parent.node(node=Widget, direction="out") + assert got is not None and isinstance(got, Widget) + + +@pytest.mark.asyncio +async def test_subtype_resolved_under_entity_name_collision(ctx): + """When two Node subclasses share an entity name, the fast path hydrates the + concrete class the caller filtered on (not the first global name match).""" + parent = Node() + beta = BetaAccount(label="beta") + for n in (parent, beta): + await ctx.save(n) + await parent.connect(beta) + + # AlphaAccount is defined first, so an unhinted global name lookup for + # "CollidingAccount" resolves to Alpha — the old bug dropped the Beta row. + got = await parent.node(node=BetaAccount, direction="out") + assert got is not None, "subtype filter dropped a valid neighbor (bug 2)" + assert isinstance(got, BetaAccount) + assert got.id == beta.id + + many = await parent.nodes(node=BetaAccount, direction="out") + assert [n.id for n in many] == [beta.id]