bitflicker64 commented on code in PR #760:
URL:
https://github.com/apache/hugegraph-toolchain/pull/760#discussion_r3903173979
##########
hugegraph-client/src/main/java/org/apache/hugegraph/driver/AuthManager.java:
##########
@@ -66,7 +66,7 @@ public AuthManager(RestClient client, String graphSpace,
String graph) {
this.targetAPI = new TargetAPI(client, graphSpace);
this.groupAPI = new GroupAPI(client);
this.graphSpaceGroupAPI = new GroupAPI(client, graphSpace);
- this.userAPI = new UserAPI(client, graphSpace);
+ this.userAPI = new UserAPI(client, graphSpace, graph);
Review Comment:
🧹 Follow-up note rather than a request to widen this PR: #759 scopes the fix
to `UserAPI`, and the other auth entities were already broken against 1.5.
Worth capturing before it is lost, since "Fixes #759" closes the tracking issue
on merge.
`targetAPI`, `graphSpaceGroupAPI`, `accessAPI`, `projectAPI`, `belongAPI`
and `managerAPI` still take the two-argument constructor, so against a 1.5
server they build `graphspaces/DEFAULT/auth/<type>`, which
`RestClient.removeDefaultGsPrefix` rewrites to `auth/<type>`. Server 1.5.0 has
no such routes: `TargetAPI` is `@Path("graphs/{graph}/auth/targets")`, and
`GroupAPI`, `AccessAPI`, `BelongAPI` and `ProjectAPI` are graph-scoped the same
way. Hubble's `RoleService`, `BelongService`, `AccessService` and
`TargetService` all depend on them.
Could you open a follow-up issue for the remaining auth surface and link it
here?
##########
hugegraph-client/src/main/java/org/apache/hugegraph/api/auth/UserAPI.java:
##########
@@ -70,8 +79,19 @@ public List<User> list(int limit) {
}
public User getByName(String name) {
- Map<String, Object> params = ImmutableMap.of("name", name);
+ Map<String, Object> params = this.legacyGraphScoped ?
+ ImmutableMap.of("limit", -1) :
+ ImmutableMap.of("name", name);
RestResult result = this.client.get(this.path(), params);
+ if (this.legacyGraphScoped) {
+ List<User> users = result.readList(this.type(), User.class);
+ for (User user : users) {
+ if (name.equals(user.name())) {
+ return user;
+ }
+ }
+ return null;
Review Comment:
⚠️ The legacy branch returns `null` on a miss, a value the GraphSpace branch
never produces: there `readObject(User.class)` either yields a user or throws,
so `null` is a new third outcome for the same method.
Hubble's callers are split on whether they handle it.
`UserService.getpersonal` guards it (line 172), while `updatePersonal` (line
406) and `updatepwd` (line 433) dereference the result straight away, and
`getUser` (line 100) turns a miss into a null entity rather than an error.
Please throw on a miss so both branches signal it the same way. Two cheap
`AuthApiPathTest` additions would pin this down: a name absent from the
response, and `graph == null` with `supportGs` false, which is the fall-through
`ManagerAPITest` already exercises via `new AuthManager(client, "DEFAULT",
null)`.
##########
hugegraph-client/src/main/java/org/apache/hugegraph/api/auth/UserAPI.java:
##########
@@ -30,8 +30,17 @@
public class UserAPI extends AuthAPI {
+ private final boolean legacyGraphScoped;
+
public UserAPI(RestClient client, String graphSpace) {
super(client, graphSpace);
+ this.legacyGraphScoped = false;
+ }
+
+ public UserAPI(RestClient client, String graphSpace, String graph) {
+ super(client, graphSpace, graph);
+ this.legacyGraphScoped = !client.isSupportGs() &&
Review Comment:
🧹 This repeats the predicate `AuthAPI` already evaluated to pick between
`LEGACY_PATH` and `PATH`, with nothing tying the two copies together. If a
later edit moves one, `getByName` sends the wrong query for the path it is
actually on: `name=<n>` against `graphs/{graph}/auth/users` is ignored by
server 1.5.0, which returns the first 100 users in a `users` wrapper, and
`readObject(User.class)` then fails with `SerializeException: Failed to
deserialize` (reproduced against the built client classes). That is the exact
symptom #759 reports.
Suggest storing the decision once in `AuthAPI` (a `protected final boolean`
plus an accessor) and reading it here. The legacy branch of `getByName` is
`list(-1)` inlined, so it can call `this.list(-1)` and scan the result, which
also restores the `checkLimit` guard the hand-built params map skips.
--
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]
