From db81136b0a0d4482b023f3ab1256364caec09cd8 Mon Sep 17 00:00:00 2001 From: yzxcj797 Date: Sat, 22 Aug 2026 01:23:46 +0800 Subject: [PATCH 1/2] fix(graph_store): resolve application ids to internal ids when creating relationships MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GraphStore.add_edges reads application-level string ids from source_id/target_id and passed them straight to the backend, while Neo4jStore.create_relationship matches on internal integer ids (id(n)). Nothing resolved one to the other, so persisting a graph created every node and zero relationships — each edge failed with 'nodes not found' as a logger.warning and the call appeared to succeed (#1136). add_nodes already receives the application-id/internal-id pair from create_nodes (the app id is preserved in properties['id']) and discarded it one statement before add_edges needed it. Keep the map on the store, populate it from both add_nodes and create_node, and resolve known application ids in create_relationship. Unknown ids pass through unchanged, so direct internal-id callers and backends whose ids are the application ids keep their existing behavior. --- semantica/graph_store/graph_store.py | 50 +++++- .../test_app_id_resolution_1136.py | 157 ++++++++++++++++++ 2 files changed, 204 insertions(+), 3 deletions(-) create mode 100644 tests/graph_store/test_app_id_resolution_1136.py diff --git a/semantica/graph_store/graph_store.py b/semantica/graph_store/graph_store.py index a8bd73f9..b0e15e15 100644 --- a/semantica/graph_store/graph_store.py +++ b/semantica/graph_store/graph_store.py @@ -556,6 +556,13 @@ class GraphStore: ) self.config = config + # Application-id -> backend-internal-id map for nodes added through + # the compatibility layer (#1136). add_nodes()/create_node() record + # the internal ids the backend returns; create_relationship() + # resolves known application ids through it so string ids stop + # mismatching backends that match on internal ids (Neo4j id(n)). + self._app_node_id_map: Dict[Any, Any] = {} + # Initialize store backend self._store_backend = None self._manager = None @@ -630,7 +637,9 @@ class GraphStore: **options, ) -> Dict[str, Any]: """Create a node.""" - return self._manager.nodes.create(labels, properties, **options) + created = self._manager.nodes.create(labels, properties, **options) + self._record_app_node_id(created) + return created def create_nodes( self, @@ -688,9 +697,20 @@ class GraphStore: properties: Optional[Dict[str, Any]] = None, **options, ) -> Dict[str, Any]: - """Create a relationship.""" + """Create a relationship. + + Node ids added through the compatibility layer are application-level + strings, while backends such as Neo4j match on internal integer ids + (#1136). Known application ids are resolved to the internal ids the + backend returned at creation time; unknown ids pass through + unchanged, so direct internal-id callers keep working. + """ return self._manager.relationships.create( - start_node_id, end_node_id, rel_type, properties, **options + self._app_node_id_map.get(start_node_id, start_node_id), + self._app_node_id_map.get(end_node_id, end_node_id), + rel_type, + properties, + **options, ) def get_relationships( @@ -794,6 +814,21 @@ class GraphStore: """Create an index.""" return self._manager.create_index(label, property_name, index_type, **options) + def _record_app_node_id(self, created: Optional[Dict[str, Any]]) -> None: + """Record the application-id -> internal-id pair of a created node. + + Backends return their own internal id alongside the stored properties; + when the caller supplied an application id it is preserved in + ``properties["id"]`` by the compatibility layer, which makes the pair + recoverable (#1136). + """ + if not isinstance(created, dict): + return + app_id = (created.get("properties") or {}).get("id") + internal_id = created.get("id") + if app_id is not None and internal_id is not None: + self._app_node_id_map[app_id] = internal_id + # Compatibility with AgentMemory / ContextGraph interface def add_nodes(self, nodes: List[Dict[str, Any]], **options) -> int: """ @@ -853,12 +888,21 @@ class GraphStore: # and properties. result = self.create_nodes(graph_nodes, **options) + # Keep the application-id -> internal-id pairs instead of discarding + # them, so add_edges()/create_relationship() can resolve the string + # ids callers actually use (#1136). + for created in result: + self._record_app_node_id(created) return len(result) def add_edges(self, edges: List[Dict[str, Any]], **options) -> int: """ Add edges (Compatibility method). + ``source_id``/``target_id`` are application-level string ids; they are + resolved to the backend's internal ids via the map ``add_nodes`` + populated when the nodes were created (#1136). + Args: edges: List of edge dictionaries **options: Additional options diff --git a/tests/graph_store/test_app_id_resolution_1136.py b/tests/graph_store/test_app_id_resolution_1136.py new file mode 100644 index 00000000..605802d7 --- /dev/null +++ b/tests/graph_store/test_app_id_resolution_1136.py @@ -0,0 +1,157 @@ +"""Regression tests for the application-id / internal-id mismatch (#1136). + +`GraphStore.add_edges` reads application-level string ids from +``source_id``/``target_id`` while backends such as Neo4j match relationships +on their internal integer ids (``id(n)``). Every edge therefore failed with +"nodes not found" and a graph persisted with all nodes and zero +relationships. + +The fix keeps the application-id -> internal-id map that ``add_nodes`` +already receives from the backend (instead of discarding it) and resolves +known string ids in ``create_relationship``. These tests pin that contract +with a fake manager — no live database needed. +""" + +import unittest +from typing import Any, Dict, List, Optional, Tuple + +from semantica.graph_store.graph_store import GraphStore + + +class FakeNodeManager: + """Mimics NodeManager against a backend that mints internal integer ids.""" + + def __init__(self) -> None: + self._next_internal_id = 100 + self.created: List[Dict[str, Any]] = [] + + def create_batch( + self, nodes: List[Dict[str, Any]], **options: Any + ) -> List[Dict[str, Any]]: + created: List[Dict[str, Any]] = [] + for node in nodes: + internal_id = self._next_internal_id + self._next_internal_id += 1 + record = { + "id": internal_id, + "labels": node.get("labels", []), + "properties": dict(node.get("properties", {})), + } + created.append(record) + self.created.append(record) + return created + + def create( + self, labels: List[str], properties: Dict[str, Any], **options: Any + ) -> Dict[str, Any]: + return self.create_batch( + [{"labels": labels, "properties": properties}], **options + )[0] + + +class FakeRelationshipManager: + """Records the resolved ids create_relationship hands to the backend.""" + + def __init__(self) -> None: + self.calls: List[Tuple[Any, Any, str]] = [] + + def create( + self, + start_node_id: Any, + end_node_id: Any, + rel_type: str, + properties: Optional[Dict[str, Any]] = None, + **options: Any, + ) -> Dict[str, Any]: + self.calls.append((start_node_id, end_node_id, rel_type)) + return {"start": start_node_id, "end": end_node_id, "type": rel_type} + + +class FakeManager: + def __init__(self) -> None: + self.nodes = FakeNodeManager() + self.relationships = FakeRelationshipManager() + + +def make_store() -> Tuple[GraphStore, FakeManager]: + """Build a GraphStore around fakes, skipping backend initialization.""" + store = GraphStore.__new__(GraphStore) + store.logger = None # type: ignore[assignment] + store.progress_tracker = None # type: ignore[assignment] + store.backend = "fake" + store.config = {} + store._app_node_id_map = {} + manager = FakeManager() + store._store_backend = None + store._manager = manager # type: ignore[assignment] + return store, manager + + +class AppIdResolutionTests(unittest.TestCase): + def test_add_edges_resolves_application_ids_to_internal_ids(self) -> None: + store, manager = make_store() + + node_count = store.add_nodes( + [ + {"id": "e1", "type": "Person", "properties": {}}, + {"id": "e2", "type": "Organization", "properties": {}}, + ] + ) + self.assertEqual(node_count, 2) + + edge_count = store.add_edges( + [{"source_id": "e1", "target_id": "e2", "type": "knows"}] + ) + + self.assertEqual(edge_count, 1) + self.assertEqual(len(manager.relationships.calls), 1) + start, end, rel_type = manager.relationships.calls[0] + self.assertEqual(start, 100, "source app id must resolve to the internal id") + self.assertEqual(end, 101, "target app id must resolve to the internal id") + self.assertEqual(rel_type, "knows") + + def test_build_from_entities_and_relationships_creates_the_edge(self) -> None: + """The exact shape of the #1136 reproduction, against fakes.""" + store, manager = make_store() + + stats = store.build_from_entities_and_relationships( + [ + {"id": "e1", "type": "Person", "text": "Alice"}, + {"id": "e2", "type": "Organization", "text": "Acme"}, + ], + [{"source_id": "e1", "target_id": "e2", "type": "knows"}], + ) + + self.assertEqual(stats["statistics"]["node_count"], 2) + self.assertEqual( + stats["statistics"]["edge_count"], 1, "the edge must be created, not dropped" + ) + self.assertEqual(manager.relationships.calls, [(100, 101, "knows")]) + + def test_unknown_ids_pass_through_unchanged(self) -> None: + """Ids the map does not know keep the pre-fix pass-through behavior.""" + store, manager = make_store() + + store.create_relationship(7, 8, "RELATES_TO") + + self.assertEqual(manager.relationships.calls, [(7, 8, "RELATES_TO")]) + + def test_create_node_also_populates_the_map(self) -> None: + store, manager = make_store() + + store.create_node("Person", {"id": "app-1", "name": "Alice"}) + store.add_edges([{"source_id": "app-1", "target_id": 42, "type": "knows"}]) + + self.assertEqual(manager.relationships.calls, [(100, 42, "knows")]) + + def test_nodes_without_application_id_do_not_pollute_the_map(self) -> None: + store, manager = make_store() + + store.add_nodes([{"labels": ["Person"], "properties": {"name": "Anon"}}]) + store.create_relationship("not-mapped", 5, "RELATES_TO") + + self.assertEqual(manager.relationships.calls, [("not-mapped", 5, "RELATES_TO")]) + + +if __name__ == "__main__": + unittest.main() From 92ad7bc2df9424e2e7f71ad9af568a56656babb5 Mon Sep 17 00:00:00 2001 From: yzxcj797 Date: Sat, 22 Aug 2026 02:07:55 +0800 Subject: [PATCH 2/2] Address review: string-only application id resolution MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per the Qodo review: only string application ids are recorded in (and resolved through) _app_node_id_map. Internal ids are commonly integers, so an integer application id could collide with — and silently remap — a caller-supplied internal id of the same value. Also pass a labels list to create_node in the regression test, matching the API signature. --- semantica/graph_store/graph_store.py | 18 +++++++++++----- .../test_app_id_resolution_1136.py | 21 ++++++++++++++++++- 2 files changed, 33 insertions(+), 6 deletions(-) diff --git a/semantica/graph_store/graph_store.py b/semantica/graph_store/graph_store.py index b0e15e15..2ff9aaa6 100644 --- a/semantica/graph_store/graph_store.py +++ b/semantica/graph_store/graph_store.py @@ -703,11 +703,16 @@ class GraphStore: strings, while backends such as Neo4j match on internal integer ids (#1136). Known application ids are resolved to the internal ids the backend returned at creation time; unknown ids pass through - unchanged, so direct internal-id callers keep working. + unchanged, so direct internal-id callers keep working. Only string + application ids participate: internal ids are commonly integers, so + recording or resolving an integer key could remap a caller-supplied + internal id to a different node. """ return self._manager.relationships.create( - self._app_node_id_map.get(start_node_id, start_node_id), - self._app_node_id_map.get(end_node_id, end_node_id), + self._app_node_id_map.get(start_node_id, start_node_id) + if isinstance(start_node_id, str) else start_node_id, + self._app_node_id_map.get(end_node_id, end_node_id) + if isinstance(end_node_id, str) else end_node_id, rel_type, properties, **options, @@ -820,13 +825,16 @@ class GraphStore: Backends return their own internal id alongside the stored properties; when the caller supplied an application id it is preserved in ``properties["id"]`` by the compatibility layer, which makes the pair - recoverable (#1136). + recoverable (#1136). Only STRING application ids are recorded: + internal ids are commonly integers, and an integer application id + would collide with (and silently remap) a caller-supplied internal id + of the same value in ``create_relationship``. """ if not isinstance(created, dict): return app_id = (created.get("properties") or {}).get("id") internal_id = created.get("id") - if app_id is not None and internal_id is not None: + if isinstance(app_id, str) and internal_id is not None: self._app_node_id_map[app_id] = internal_id # Compatibility with AgentMemory / ContextGraph interface diff --git a/tests/graph_store/test_app_id_resolution_1136.py b/tests/graph_store/test_app_id_resolution_1136.py index 605802d7..93d8b1f1 100644 --- a/tests/graph_store/test_app_id_resolution_1136.py +++ b/tests/graph_store/test_app_id_resolution_1136.py @@ -139,11 +139,30 @@ class AppIdResolutionTests(unittest.TestCase): def test_create_node_also_populates_the_map(self) -> None: store, manager = make_store() - store.create_node("Person", {"id": "app-1", "name": "Alice"}) + store.create_node(["Person"], {"id": "app-1", "name": "Alice"}) store.add_edges([{"source_id": "app-1", "target_id": 42, "type": "knows"}]) self.assertEqual(manager.relationships.calls, [(100, 42, "knows")]) + def test_integer_application_ids_never_collide_with_internal_ids(self) -> None: + # Qodo review: an int application id must not be recorded/resolved, + # or a caller passing that same int as an internal id would be + # silently remapped to a different node. + store, manager = make_store() + + store.add_nodes( + [ + {"id": 100, "type": "Person", "properties": {}}, + {"id": "str-app", "type": "Person", "properties": {}}, + ] + ) + # Internal id 100 legitimately targets the FIRST node; the integer + # app id of that same value must not redirect it to the second. + store.create_relationship(100, 101, "RELATES_TO") + + self.assertEqual(manager.relationships.calls, [(100, 101, "RELATES_TO")]) + self.assertNotIn(100, store._app_node_id_map) + def test_nodes_without_application_id_do_not_pollute_the_map(self) -> None: store, manager = make_store()