imbajin commented on code in PR #325:
URL: https://github.com/apache/hugegraph-ai/pull/325#discussion_r3207144062
##########
hugegraph-python-client/src/tests/api/test_auth.py:
##########
@@ -146,7 +150,10 @@ def test_target_operations(self):
[{"type": "VERTEX", "label": "person", "properties": {"city":
"Shanghai"}}],
)
# Verify the target was modified
- self.assertEqual(target["target_resources"][0]["properties"]["city"],
"Shanghai")
+ target_resources = target["target_resources"]
+ if isinstance(target_resources, dict):
+ target_resources = next(iter(target_resources.values()), [])
+ self.assertEqual(target_resources[0]["properties"]["city"], "Shanghai")
Review Comment:
⚠️ Accepting both `dict` and `list` shapes silently hides a real API
contract divergence. If the server ever returns an unexpected shape due to a
bug, the test still passes.
Better: pick the expected shape deterministically (e.g., branch on
`cls.client.cfg.version` or `gs_supported`), or assert the shape explicitly so
a drift surfaces as a test failure, not invisibly. At minimum, add a comment
documenting *which* HugeGraph versions return which shape and link the
server-side change — otherwise future maintainers can't tell whether the dict
branch is compensating for a server bug or a legitimate format.
##########
hugegraph-python-client/src/pyhugegraph/utils/huge_router.py:
##########
@@ -126,7 +126,27 @@ def wrapper(self: "HGraphContext", *args: Any, **kwargs:
Any) -> Any:
all_kwargs = dict(bound_args.arguments)
# Remove 'self' from the arguments used to format the pathinfo
all_kwargs.pop("self")
- formatted_path = path.format(**all_kwargs)
+
+ # Support graphspace-scoped paths: prefer graphspace-prefixed
path
+ # but gracefully fall back to server-level /auth/... if
graphspace
+ # is not configured or the server does not support graphspaces.
+ if "{graphspace}" in path:
+ # Prefer explicit graphspace argument passed by caller
+ graphspace_arg = all_kwargs.get("graphspace")
+ graphspace_cfg = getattr(self.session.cfg, "graphspace",
None)
+ gs_supported = getattr(self.session.cfg, "gs_supported",
False)
+
+ # Use graphspace if available and server supports it
+ if graphspace_arg or (graphspace_cfg and gs_supported):
+ all_kwargs.setdefault("graphspace", graphspace_arg or
graphspace_cfg)
Review Comment:
‼️ `setdefault` is the wrong primitive here: if a future method declares
`graphspace: str | None = None` (mirroring `services.py`),
`bound_args.apply_defaults()` populates `all_kwargs['graphspace'] = None`. The
key now exists, so `setdefault` is a no-op and `path.format` produces
`/graphspaces/None/auth/...`.
Use direct assignment instead:
```suggestion
all_kwargs["graphspace"] = graphspace_arg or
graphspace_cfg
```
Current auth methods don't take `graphspace` as a parameter, so this doesn't
trigger today — but the whole point of this decorator is reusable routing, so
the bug will be silent until someone adds such a method.
##########
.github/workflows/ruff.yml:
##########
@@ -38,13 +38,9 @@ jobs:
restore-keys: |
${{ runner.os }}-uv-${{ matrix.python-version }}-
- - name: Install dependencies
+ - name: Install dev dependencies
run: |
- uv sync --extra all --extra dev
-
- - name: Check DGL version
- run: |
- uv run python -c "import dgl; print(dgl.__version__)"
+ uv sync --extra dev
Review Comment:
🧹 This CI change (drop `--extra all` + remove DGL version check) is out of
scope for a Python-client auth-routing fix and isn't called out in the PR
description. It's probably fine — ruff lint shouldn't need DGL/torch installed
— but please either split it into its own PR or add a one-liner to the PR
description explaining why it's bundled, so reviewers don't have to guess at
the intent.
##########
hugegraph-python-client/src/pyhugegraph/utils/huge_router.py:
##########
@@ -126,7 +126,27 @@ def wrapper(self: "HGraphContext", *args: Any, **kwargs:
Any) -> Any:
all_kwargs = dict(bound_args.arguments)
# Remove 'self' from the arguments used to format the pathinfo
all_kwargs.pop("self")
- formatted_path = path.format(**all_kwargs)
+
+ # Support graphspace-scoped paths: prefer graphspace-prefixed
path
+ # but gracefully fall back to server-level /auth/... if
graphspace
+ # is not configured or the server does not support graphspaces.
+ if "{graphspace}" in path:
+ # Prefer explicit graphspace argument passed by caller
+ graphspace_arg = all_kwargs.get("graphspace")
+ graphspace_cfg = getattr(self.session.cfg, "graphspace",
None)
+ gs_supported = getattr(self.session.cfg, "gs_supported",
False)
+
+ # Use graphspace if available and server supports it
+ if graphspace_arg or (graphspace_cfg and gs_supported):
+ all_kwargs.setdefault("graphspace", graphspace_arg or
graphspace_cfg)
+ formatted_path = path.format(**all_kwargs)
+ else:
+ # Fallback to server-level absolute auth path by
removing
+ # the leading '/graphspaces/{graphspace}' segment.
+ fallback_path =
path.replace("/graphspaces/{graphspace}", "")
Review Comment:
⚠️ `str.replace` replaces *all* occurrences, which works only because the
token currently appears exactly once as a prefix. If any future path ever
embeds `/graphspaces/{graphspace}` deeper in the URL (nested resources, proxy
paths, etc.), the fallback will quietly mangle it. More explicit and harder to
misuse:
```suggestion
prefix = "/graphspaces/{graphspace}"
if not path.startswith(prefix + "/"):
raise ValueError(f"Expected graphspace-prefixed
path, got: {path}")
fallback_path = path.removeprefix(prefix)
formatted_path = fallback_path.format(**all_kwargs)
```
##########
hugegraph-python-client/src/tests/api/test_auth_routing.py:
##########
@@ -0,0 +1,103 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from urllib.parse import urljoin
+
+import pytest
+from pyhugegraph.api.auth import AuthManager
+
+
+class DummyCfg:
+ def __init__(self, url, graphspace, gs_supported, graph_name):
+ self.url = url
+ self.graphspace = graphspace
+ self.gs_supported = gs_supported
+ self.graph_name = graph_name
+
+
+class DummySession:
+ """Minimal session mimic implementing resolve and request used by
router."""
+
+ def __init__(self, cfg: DummyCfg):
+ self.cfg = cfg
+ self.last = None
+
+ def resolve(self, path: str) -> str:
+ base = f"{self.cfg.url.rstrip('/')}/"
+ if self.cfg.gs_supported:
+ base = urljoin(base,
f"graphspaces/{self.cfg.graphspace}/graphs/{self.cfg.graph_name}/")
+ else:
+ base = urljoin(base, f"graphs/{self.cfg.graph_name}/")
+ return urljoin(base, path).strip("/")
+
+ def request(self, path: str, method: str = "GET", validator=None,
**kwargs):
+ # mirror behavior of real session.request used by router: resolve path
+ self.last = self.resolve(path)
+ return {"url": self.last, "method": method}
+
+
[email protected](
+ "endpoint, method_call, args, expected_subpath",
+ [
+ ("users", "list_users", (), "graphspaces/GS/auth/users"),
+ ("accesses", "list_accesses", (), "graphspaces/GS/auth/accesses"),
+ ("targets", "list_targets", (), "graphspaces/GS/auth/targets"),
+ ("belongs", "list_belongs", (), "graphspaces/GS/auth/belongs"),
+ ],
+)
+def test_graphspace_scoped_endpoints_use_graphspace(endpoint, method_call,
args, expected_subpath):
+ cfg = DummyCfg(url="http://127.0.0.1:8080";, graphspace="GS",
gs_supported=True, graph_name="g")
+ sess = DummySession(cfg)
+ auth = AuthManager(sess)
+
+ getattr(auth, method_call)(*args)
+ assert expected_subpath in sess.last
+
+
[email protected](
+ "endpoint, method_call, args, expected_subpath",
+ [
+ ("users", "list_users", (), "auth/users"),
+ ("accesses", "list_accesses", (), "auth/accesses"),
+ ("targets", "list_targets", (), "auth/targets"),
+ ("belongs", "list_belongs", (), "auth/belongs"),
+ ],
+)
+def test_graphspace_scoped_endpoints_fallback_to_server_level(endpoint,
method_call, args, expected_subpath):
+ # No graphspace support configured -> should fall back to server-level
/auth/...
+ cfg = DummyCfg(url="http://127.0.0.1:8080";, graphspace=None,
gs_supported=False, graph_name="g")
+ sess = DummySession(cfg)
+ auth = AuthManager(sess)
+
+ getattr(auth, method_call)(*args)
+ assert expected_subpath in sess.last
+
+
+def test_groups_are_server_level():
+ # With graphspace support
+ cfg = DummyCfg(url="http://127.0.0.1:8080";, graphspace="GS",
gs_supported=True, graph_name="g")
+ sess = DummySession(cfg)
+ auth = AuthManager(sess)
+ auth.list_groups()
+ assert "/auth/groups" in sess.last or "auth/groups" in sess.last
Review Comment:
🧹 `"/auth/groups" in x or "auth/groups" in x` is redundant — the first
substring contains the second, so if the first matches the second always does,
and if the second fails the first always fails. Just assert `"auth/groups" in
sess.last` once (or use `endswith("/auth/groups")` if you want to pin the
position).
##########
hugegraph-python-client/src/tests/api/test_auth_routing.py:
##########
@@ -0,0 +1,103 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from urllib.parse import urljoin
+
+import pytest
+from pyhugegraph.api.auth import AuthManager
+
+
+class DummyCfg:
+ def __init__(self, url, graphspace, gs_supported, graph_name):
+ self.url = url
+ self.graphspace = graphspace
+ self.gs_supported = gs_supported
+ self.graph_name = graph_name
+
+
+class DummySession:
+ """Minimal session mimic implementing resolve and request used by
router."""
+
+ def __init__(self, cfg: DummyCfg):
+ self.cfg = cfg
+ self.last = None
+
+ def resolve(self, path: str) -> str:
+ base = f"{self.cfg.url.rstrip('/')}/"
+ if self.cfg.gs_supported:
+ base = urljoin(base,
f"graphspaces/{self.cfg.graphspace}/graphs/{self.cfg.graph_name}/")
+ else:
+ base = urljoin(base, f"graphs/{self.cfg.graph_name}/")
+ return urljoin(base, path).strip("/")
+
+ def request(self, path: str, method: str = "GET", validator=None,
**kwargs):
+ # mirror behavior of real session.request used by router: resolve path
+ self.last = self.resolve(path)
+ return {"url": self.last, "method": method}
+
+
[email protected](
+ "endpoint, method_call, args, expected_subpath",
+ [
+ ("users", "list_users", (), "graphspaces/GS/auth/users"),
+ ("accesses", "list_accesses", (), "graphspaces/GS/auth/accesses"),
+ ("targets", "list_targets", (), "graphspaces/GS/auth/targets"),
Review Comment:
⚠️ Coverage gap: all parametrized cases call `list_*` methods that have no
URL placeholders. The routes with IDs
(`/graphspaces/{graphspace}/auth/users/{user_id}`, `.../accesses/{access_id}`,
etc.) exercise a different code path — they format two placeholders at once,
and the `setdefault` interaction I flagged in `huge_router.py` is only
observable there.
Add at least one case like `("get_user", ("u1",),
"graphspaces/GS/auth/users/u1")` to both parametrize blocks.
--
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]
