imbajin commented on code in PR #325:
URL: https://github.com/apache/hugegraph-ai/pull/325#discussion_r3207309575
##########
hugegraph-python-client/src/pyhugegraph/api/auth.py:
##########
@@ -22,19 +22,25 @@
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")
+ """Manage HugeGraph authentication and authorization.
+
+ This manager implements a dual-path strategy for auth endpoints:
+ - Graphspace-scoped endpoints (users, accesses, belongs, targets) use
+ the pattern: graphspaces/{graphspace}/auth/{endpoint}
+ - Server-level endpoints (groups) use the pattern: /auth/{endpoint}
+
+ This strategy mirrors the Java client's AuthAPI implementation and ensures
+ compatibility with HugeGraph 1.7.0+ where auth APIs are graphspace-scoped.
Review Comment:
⚠️ The PR description frames this as "paths currently work via a temporary
`PathFilter` compatibility layer that will be removed." Reading the actual
server code tells a different story:
- `[email protected]` whitelists `"auth"` as the first path segment — it
**does not rewrite** `/auth/*`, it lets it through unchanged.
- Server-side `UserAPI`/`AccessAPI`/`BelongAPI`/`TargetAPI` are only mounted
under `@Path("graphspaces/{graphspace}/auth/...")`. Nothing mounts them at
`/auth/...` on OSS 1.7.0.
- So `/auth/users` on Apache OSS HugeGraph 1.7.0 is already a 404 today — it
doesn't "work via PathFilter" and isn't "about to break when PathFilter is
removed."
The fix direction (match the server's `@Path` annotations) is correct, but
the motivation in both the PR description and this docstring is off. A more
accurate framing: *"The previous absolute `/auth/...` paths don't match the
server's JAX-RS `@Path` annotations on HugeGraph 1.7.0+ and return 404. This
change aligns the client with the server's graphspace-scoped paths
(users/accesses/belongs/targets) and keeps `/auth/groups` at server level,
matching `GroupAPI`'s `@Path("/auth/groups")`."*
Please update the docstring and PR description accordingly so future readers
don't chase a non-existent `PathFilter` deprecation.
##########
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}", "")
+ formatted_path = fallback_path.format(**all_kwargs)
Review Comment:
⚠️ This server-level fallback branch looks unreachable in practice on Apache
OSS HugeGraph, and it diverges from the Java client. Walking through
`huge_config.py`:
- Server version ≥ 1.5.0 → config forces `gs_supported = True`, `graphspace
= "DEFAULT"` → takes the graphspace branch, never this fallback.
- Server version < 1.5.0 → `__post_init__` raises `RuntimeError` → client
never constructs, fallback is never reached.
- `/versions` unreachable during init → `gs_supported = False`, `graphspace
= None` → this fallback fires, but the auth request itself would then hit a
server that is by definition unreachable (or not OSS HugeGraph).
Compare with `AuthAPI.java` in apache/hugegraph-toolchain:
`UserAPI`/`AccessAPI`/`BelongAPI`/`TargetAPI` only expose the `(client,
graphSpace)` constructor — there is **no server-level fallback** for these
endpoints in the Java client, on purpose, because the server has no matching
`@Path`.
Recommendation: either (a) delete this fallback branch and fail fast with a
clear error (`raise ValueError("graphspace required for auth endpoint ...")`),
matching the Java client's contract, or (b) keep it but document which
deployment it targets (an internal/enterprise HugeGraph variant that still
mounts `UserAPI` at `/auth/users`?) and add a test that actually exercises it
end-to-end. As written, the branch is a silent degradation path that no
reachable configuration will take and no test currently covers.
--
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]
