Merge pull request #1173 from yzxcj797/fix/neo4j-edge-id-space-1136

fix(graph_store): resolve application ids to internal ids when creating relationships

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,25 @@ 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. 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(
-            start_node_id, end_node_id, rel_type, properties, **options
+            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,
         )
 
     def get_relationships(
@@ -794,6 +819,24 @@ 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). 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 isinstance(app_id, str) 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 +896,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

tests/graph_store/test_app_id_resolution_1136.py

@@ -0,0 +1,176 @@
+"""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_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()
+
+        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()