From 92ad7bc2df9424e2e7f71ad9af568a56656babb5 Mon Sep 17 00:00:00 2001 From: yzxcj797 Date: Sat, 22 Aug 2026 02:07:55 +0800 Subject: [PATCH] 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()