Copilot commented on code in PR #329:
URL: https://github.com/apache/hugegraph-ai/pull/329#discussion_r3224388932
##########
hugegraph-python-client/src/pyhugegraph/utils/huge_router.py:
##########
@@ -17,6 +17,7 @@
import functools
import inspect
+import json
Review Comment:
`import json` is currently unused in this module (the vertex_id quoting
logic concatenates strings manually). Either remove this import or switch the
quoting/escaping to use `json.dumps()` so the import is justified.
##########
hugegraph-python-client/src/pyhugegraph/utils/huge_router.py:
##########
@@ -148,6 +149,14 @@ def wrapper(self: "HGraphContext", *args: Any, **kwargs:
Any) -> Any:
all_kwargs["graphspace"] = graphspace_arg or graphspace_cfg
formatted_path = path.format(**all_kwargs)
+ elif "{vertex_id}" in path:
+ # fix vertex_id format process
+ # only quote string type vertex_id
+ vertex_id = all_kwargs.pop("vertex_id")
+ if isinstance(vertex_id, str):
+ vertex_id = "\"" + vertex_id + "\""
+ all_kwargs['vertex_id'] = vertex_id
Review Comment:
The special-casing for "{vertex_id}" applies to any route containing that
placeholder (e.g. TraverserManager routes like `vertex="{vertex_id}"`), which
will now double-quote string ids and produce invalid URLs (e.g.
`vertex=""person:marko""`). Restrict this logic to vertex-id path segments only
(e.g. when the template contains `/{vertex_id}`) and prefer
`json.dumps(vertex_id)` for correct escaping instead of manual `"`
concatenation.
##########
hugegraph-python-client/src/tests/api/test_graph.py:
##########
@@ -89,6 +106,16 @@ def test_remove_vertex_by_id(self):
except NotFoundError as e:
self.assertTrue("Alice\\' does not exist" in str(e))
+ def test_remove_vertex_by_number_id(self):
+ vertex = self.graph.addVertex("department", {"name": "DepartmentA",
"headcount": 10, "floor": 1})
+ self.graph.removeVertexById(vertex.id)
+ try:
+ self.graph.getVertexById(vertex.id)
+ except NotFoundError as e:
+ msg = "\\'{}\\' does not exist".format(vertex.id)
+ logger.info(f'test_msg: {msg}')
Review Comment:
`logger` is used but never imported/defined in this test file, which will
raise `NameError`. Import the project logger (or remove the log line) so the
test runs reliably.
##########
hugegraph-python-client/src/tests/api/test_graph.py:
##########
@@ -89,6 +106,16 @@ def test_remove_vertex_by_id(self):
except NotFoundError as e:
self.assertTrue("Alice\\' does not exist" in str(e))
+ def test_remove_vertex_by_number_id(self):
+ vertex = self.graph.addVertex("department", {"name": "DepartmentA",
"headcount": 10, "floor": 1})
+ self.graph.removeVertexById(vertex.id)
+ try:
+ self.graph.getVertexById(vertex.id)
+ except NotFoundError as e:
+ msg = "\\'{}\\' does not exist".format(vertex.id)
+ logger.info(f'test_msg: {msg}')
+ self.assertTrue(msg in str(e))
+
Review Comment:
This test only asserts inside the `except NotFoundError` block; if
`getVertexById()` unexpectedly succeeds (or raises a different exception), the
test will pass silently. Use `with self.assertRaises(NotFoundError)` (or add an
`else: self.fail(...)`) to ensure the expected failure is enforced.
##########
hugegraph-python-client/src/tests/client_utils.py:
##########
@@ -68,6 +70,9 @@ def init_vertex_label(self):
schema.vertexLabel("book").useCustomizeStringId().properties("name",
"price").nullableKeys(
"price"
).ifNotExist().create()
+ schema.vertexLabel("department").properties("name", "headcount",
"floor").nullableKeys(
Review Comment:
The new `department` vertex label doesn’t set an id strategy or primary
keys. Given other labels explicitly call `.primaryKeys(...)` or
`.useCustomizeStringId()`, this risks schema creation failing (PRIMARY_KEY
strategy requires primary keys) and it doesn’t guarantee numeric vertex ids for
the new tests. Consider explicitly setting `.useAutomaticId()` (or
`.useCustomizeNumberId()` and passing `id=` when adding vertices) to ensure
numeric ids.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
