Copilot commented on code in PR #320:
URL: https://github.com/apache/hugegraph-ai/pull/320#discussion_r3159119703
##########
hugegraph-python-client/src/tests/api/test_gremlin.py:
##########
@@ -26,21 +26,40 @@
class TestGremlin(unittest.TestCase):
client = None
gremlin = None
+ skip_gremlin_tests = False
@classmethod
def setUpClass(cls):
- cls.client = ClientUtils()
- cls.client.clear_graph_all_data()
- cls.gremlin = cls.client.gremlin
- cls.client.init_property_key()
- cls.client.init_vertex_label()
- cls.client.init_edge_label()
+ try:
+ cls.client = ClientUtils()
+ cls.gremlin = cls.client.gremlin
+ cls.client.clear_graph_all_data()
+ cls.client.init_property_key()
+ cls.client.init_vertex_label()
+ cls.client.init_edge_label()
+ # Test if gremlin endpoint is available by executing a simple query
+ cls.gremlin.exec("1 + 1")
+ except (NotFoundError, Exception) as e:
+ # Skip gremlin tests if the server is unavailable
+ # (connection timeout, 404, or other gremlin-specific errors)
+ error_str = str(e)
+ if any(
+ marker in error_str
+ for marker in ["404", "Not Found", "timed out", "Connection
refused", "Gremlin can't get results"]
+ ):
Review Comment:
The `except (NotFoundError, Exception)` clause is equivalent to `except
Exception` (so `NotFoundError` is redundant). Also, this block now
conditionally skips the entire test suite based on substring matching of the
exception message, which can accidentally skip on unrelated failures that
happen to contain one of these markers. Consider narrowing the exception
type(s) and using a more deterministic signal (HTTP status / explicit error
code) before skipping.
##########
hugegraph-python-client/src/tests/api/test_metric.py:
##########
@@ -83,13 +63,16 @@ def test_metrics_operations(self):
timers_metrics = self.metrics.get_timers_metrics()
self.assertIsInstance(timers_metrics, dict)
- server_version = tuple(self.client.client.cfg.version)
- require_system_metrics_version(server_version)
system_metrics = self.metrics.get_system_metrics()
self.assertIsInstance(system_metrics, dict)
statistics = self.metrics.get_statistics_metrics()
self.assertIsInstance(statistics, dict)
backend_metrics = self.metrics.get_backend_metrics()
- self.assertGreater(len(backend_metrics["hugegraph"]), 1)
+ # In HugeGraph 1.7.0+, the backend_metrics structure changed
+ # It's still a dict, but the "hugegraph" key may not exist in the same
format
+ self.assertIsInstance(backend_metrics, dict)
+ # Only assert on the "hugegraph" key if it exists (for backward
compatibility)
+ if "hugegraph" in backend_metrics:
+ self.assertGreater(len(backend_metrics["hugegraph"]), 1)
Review Comment:
This assertion became very permissive: `backend_metrics` can now be an empty
dict (or a dict missing all expected backend entries) and the test will still
pass. If the backend metrics payload changed in 1.7, consider asserting on a
more stable invariant (e.g., non-empty response, or presence of at least one
known top-level key) so the test still detects regressions.
##########
hugegraph-python-client/src/pyhugegraph/utils/huge_config.py:
##########
@@ -53,15 +53,31 @@ def __post_init__(self):
)
match = re.search(r"(\d+)\.(\d+)(?:\.(\d+))?(?:\.\d+)?", core)
Review Comment:
`re.search()` can return `None` if the server returns an unexpected `core`
version string, but the code immediately calls `match.group(...)`. That failure
is then treated as a generic parsing/network issue and falls back to "default
v1" behavior, which also bypasses the new <1.5.0 version guard. Consider
explicitly handling `match is None` (raise a clear error or at least log and
avoid proceeding as a supported version).
##########
hugegraph-python-client/src/pyhugegraph/api/gremlin.py:
##########
@@ -28,12 +28,16 @@ class GremlinManager(HugeParamsBase):
@router.http("POST", "/gremlin")
def exec(self, gremlin):
gremlin_data = GremlinData(gremlin)
+
+ # Version-specific gremlin request handling
if self._sess.cfg.gs_supported:
+ # For graphspace-supported versions (3.0+), use graphspace-scoped
aliases
gremlin_data.aliases = {
"graph":
f"{self._sess.cfg.graphspace}-{self._sess.cfg.graph_name}",
"g":
f"__g_{self._sess.cfg.graphspace}-{self._sess.cfg.graph_name}",
}
else:
+ # For HugeGraph 1.x (including 1.7.0), always include aliases so
`g` is bound
Review Comment:
The comment says graphspace support is for "3.0+", but the code path is
gated only by `cfg.gs_supported`. With the updated version detection enabling
`gs_supported` for 1.7 as well, this comment (and possibly the intended
branching) is now inconsistent. Please align the comment/logic so it’s clear
which server versions should use graphspace-scoped aliases vs graph-scoped
aliases.
##########
hugegraph-python-client/src/pyhugegraph/api/auth.py:
##########
@@ -22,13 +22,19 @@
from pyhugegraph.utils import huge_router as router
+# NOTE: Auth endpoints currently use absolute paths (/auth/...) which rely on a
+# temporary PathFilter compatibility layer in HugeGraph 1.7.0. This layer will
be
+# removed in future versions. When it is removed, these paths should be
converted
+# to relative paths (auth/...) with proper graphspace-scoped routing for
non-group
+# endpoints, similar to the Java Client's dual-path strategy.
+# See: apache/hugegraph-ai#322 (HugeGraph 1.7.0 auth API migration)
class AuthManager(HugeParamsBase):
- @router.http("GET", "auth/users")
+ @router.http("GET", "/auth/users")
def list_users(self, limit=None):
params = {"limit": limit} if limit is not None else {}
return self._invoke_request(params=params)
- @router.http("POST", "auth/users")
+ @router.http("POST", "/auth/users")
def create_user(self, user_name, user_password, user_phone=None,
user_email=None) -> dict | None:
Review Comment:
These auth routes were changed from relative paths (resolved under
`/graphs/{graph}/...` or `/graphspaces/{graphspace}/graphs/{graph}/...`) to
absolute paths (resolved at the server root). This is a breaking behavior
change for deployments/versions that only expose graph-scoped auth endpoints.
If the intention is to support both layouts, consider adding version-based
routing or a fallback strategy (try graph-scoped first, then root-scoped)
rather than permanently switching everything to absolute paths.
--
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]
