imbajin commented on code in PR #3008:
URL: https://github.com/apache/hugegraph/pull/3008#discussion_r3279886059
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/auth/StandardAuthManager.java:
##########
@@ -813,6 +814,197 @@ public HugeGroup findGroup(String name) {
return null;
}
+ /**
+ * <h3>Design Note: Default graph / default role persistence
(workaround)</h3>
+ *
+ * <p>We reuse the existing {@code HugeGroup} / {@code HugeBelong} auth
entities as a storage
+ * mechanism to avoid introducing new schema or storage APIs.
+ *
+ * <p><b>How it works:</b>
+ * <ul>
+ * <li>A "marker group" with a special prefixed name (e.g. {@code
"~default_graph:gs:g"})
+ * is created in the system graph to represent a (graphSpace, graph)
binding.</li>
+ * <li>A {@code HugeBelong} edge from the user vertex to this marker group
records which
+ * user has set which graph as their default.</li>
+ * <li>The Belong ID is deterministic: {@code "{userId}->ug->{groupId}"},
which lets us
+ * check existence without a full scan.</li>
+ * </ul>
+ *
+ * <p><b>Known limitations:</b>
+ * <ol>
+ * <li>These marker groups appear in {@code listGroups()} results and may
confuse callers.</li>
+ * <li>This mechanism only works in <b>PD mode</b> (system graph backed by
HStore).
+ * In non-PD (standalone RocksDB) mode, the API layer degrades gracefully
+ * by returning empty results (see {@code GraphsAPI.setDefault /
getDefault}).</li>
+ * <li>The belong ID format depends on the internal convention {@code
"{u}->ug->{g}"}.</li>
+ * </ol>
+ *
+ * @todo Consider introducing a dedicated lightweight KV store or a
separate
+ * vertex label for user-preference data to avoid polluting the auth graph.
+ */
+ private static final String DEFAULT_GRAPH_MARKER = "~default_graph";
+
+ private String defaultGraphGroupName(String graphSpace, String graph) {
+ return DEFAULT_GRAPH_MARKER + ":" + graphSpace + ":" + graph;
+ }
+
+ @Override
+ public void setDefaultGraph(String graphSpace, String graph, String user) {
+ // Use a special-named HugeGroup as a marker for the default graph,
+ // then create a HugeBelong (user -> marker-group) to persist the
binding.
+ String markerName = defaultGraphGroupName(graphSpace, graph);
+ Id groupId = IdGenerator.of(markerName);
+ if (!this.groups.exists(groupId)) {
+ HugeGroup markerGroup = new HugeGroup(markerName);
+ markerGroup.creator(user);
+ this.groups.add(markerGroup);
+ }
+ createBelongBinding(user, groupId);
+ }
+
+ @Override
+ public void unsetDefaultGraph(String graphSpace, String graph, String
user) {
+ String markerName = defaultGraphGroupName(graphSpace, graph);
+ Id groupId = IdGenerator.of(markerName);
+ removeBelongBinding(user, groupId);
+ }
+
+ @Override
+ public Map<String, Date> getDefaultGraph(String graphSpace, String user) {
+ Id userId = IdGenerator.of(user);
+ List<HugeBelong> belongs = this.belong.list(userId, Directions.OUT,
+ HugeBelong.P.BELONG, -1);
+ String prefix = DEFAULT_GRAPH_MARKER + ":" + graphSpace + ":";
+ Map<String, Date> result = new LinkedHashMap<>();
+ for (HugeBelong b : belongs) {
+ String targetName = b.target().asString();
+ if (targetName.startsWith(prefix)) {
+ String graphName = targetName.substring(prefix.length());
+ result.put(graphName, b.update());
+ }
+ }
+ return result;
+ }
+
+ private static final String DEFAULT_ROLE_MARKER = "~default_role";
+
+ /**
+ * Build marker group name for a space-level role.
+ * Format: ~default_role:<graphSpace>:<role>
+ */
+ private String defaultRoleGroupName(String graphSpace, String role) {
+ return DEFAULT_ROLE_MARKER + ":" + graphSpace + ":" + role;
+ }
+
+ /**
+ * Build marker group name for a graph-level role (e.g. OBSERVER).
+ * Format: ~default_role:<graphSpace>:<role>:<graph>
+ */
+ private String defaultRoleGroupName(String graphSpace, String role,
+ String graph) {
+ return DEFAULT_ROLE_MARKER + ":" + graphSpace + ":" + role +
+ ":" + graph;
+ }
+
+ private Id ensureMarkerGroup(String markerName, String creator) {
+ Id groupId = IdGenerator.of(markerName);
+ if (!this.groups.exists(groupId)) {
+ HugeGroup markerGroup = new HugeGroup(markerName);
+ markerGroup.creator(creator);
+ this.groups.add(markerGroup);
+ }
+ return groupId;
+ }
+
+ private HugeBelong findBelongBinding(String owner, Id groupId) {
+ Id userId = IdGenerator.of(owner);
+ List<HugeBelong> belongs = this.belong.list(userId, Directions.OUT,
+ HugeBelong.P.BELONG, -1);
+ for (HugeBelong b : belongs) {
+ if (groupId.equals(b.target())) {
+ return b;
+ }
+ }
+ return null;
+ }
+
+ private Id createBelongBinding(String owner, Id groupId) {
+ HugeBelong existing = findBelongBinding(owner, groupId);
+ if (existing != null) {
+ return existing.id();
+ }
+ Id userId = IdGenerator.of(owner);
+ HugeBelong belong = new HugeBelong(userId, groupId);
+ belong.creator(owner);
+ return this.belong.add(belong);
Review Comment:
‼️ **Invalidate the user cache after V1 default graph/role mutations**
`createBelongBinding()` and `removeBelongBinding()` mutate the auth
relationship store, but unlike the other mutating methods in this class they
never call `invalidateUserCache()`. This can leave permission/default-role
checks using stale cached auth data after `setDefaultGraph()`,
`unsetDefaultGraph()`, `createDefaultRole()`, or `deleteDefaultRole()` until
the cache naturally expires.
Please invalidate the cache after adding or deleting the belong binding. For
example:
```suggestion
Id id = this.belong.add(belong);
this.invalidateUserCache();
return id;
```
And mirror the same invalidation after `this.belong.delete(existing.id())`
in `removeBelongBinding()`.
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/profile/GraphsAPI.java:
##########
@@ -220,15 +490,17 @@ public Object create(@Context GraphManager manager,
if (StringUtils.isNotEmpty(clone)) {
// Clone from existing graph
LOG.debug("Clone graph '{}' to '{}' in graph space '{}'", clone,
name, graphSpace);
- graph = manager.cloneGraph(graphSpace, clone, name,
convConfig(configs));
+ Map<String, Object> cloneConfigs = configs != null ? configs : new
HashMap<>();
+ graph = manager.cloneGraph(graphSpace, clone, name,
convConfig(cloneConfigs));
} else {
// Create new graph
graph = manager.createGraph(graphSpace, name, creator,
convConfig(configs), true);
Review Comment:
⚠️ **Validate config values before stringifying them**
`entry.getValue().toString()` turns malformed client input into either a 500
(`null` value) or a lossy config string (`schema: { ... }` becomes a Java map
string). Since this is request input for graph creation/cloning, please reject
null or non-scalar values with a clear 4xx before converting to backend config
strings.
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/core/GraphManager.java:
##########
@@ -2174,6 +2199,45 @@ public Map<String, Object> graphConfig(String graphSpace,
return this.metaManager.getGraphConfig(graphSpace, graphName);
}
+ /**
+ * Update the nickname of a graph.
+ * In non-PD mode (standalone RocksDB), only the in-memory instance is
updated
+ * since local config files cannot be hot-reloaded.
+ * In PD mode, the change is also persisted to the meta storage so it
+ * survives restarts.
+ */
+ public void updateGraphNickname(String graphSpace, String graphName,
+ String nickname) {
+ // Always update the in-memory graph instance first
+ HugeGraph g = this.graph(graphSpace, graphName);
+ // Capture the old nickname so we can restore it on failure
+ String oldNickname = g != null ? g.nickname() : null;
+ if (g != null) {
+ g.nickname(nickname);
+ }
+ if (!isPDEnabled()) {
+ // Non-PD mode: in-memory only, acceptable for standalone RocksDB
+ return;
+ }
+ try {
+ Map<String, Object> configs =
+ this.metaManager.getGraphConfig(graphSpace, graphName);
+ if (configs != null) {
+ configs.put("nickname", nickname);
+ this.metaManager.updateGraphConfig(graphSpace, graphName,
configs);
Review Comment:
⚠️ **Avoid rolling back local state after the PD config write has already
succeeded**
If `updateGraphConfig()` succeeds but `notifyGraphUpdate()` throws, the
catch block restores the local in-memory nickname to the old value while PD
meta already contains the new nickname. That leaves this node inconsistent with
the persisted state and with any node that observes the meta update.
Please distinguish persistence failure from notification failure, or only
roll back the in-memory nickname when the config write itself did not succeed.
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/auth/StandardAuthManagerV2.java:
##########
@@ -1510,6 +1516,211 @@ public HugeGroup findGroup(String name) {
return result;
}
+ @Override
+ public void setDefaultGraph(String graphSpace, String graph, String user) {
+ try {
+ String role = graph + DEFAULT_SETTER_ROLE_KEY;
+ String belongId = this.metaManager.belongId(user, role);
+ Id id = IdGenerator.of(belongId);
+ // Idempotent: if binding already exists, treat as success
+ if (this.metaManager.existBelong(graphSpace, id)) {
+ return;
+ }
+ HugeBelong belong = new HugeBelong(graphSpace,
+ IdGenerator.of(user),
+ IdGenerator.of(graph +
+
DEFAULT_SETTER_ROLE_KEY));
+ this.tryInitDefaultGraph(graphSpace, graph);
+ this.updateCreator(belong);
+ belong.create(belong.update());
+ this.metaManager.createBelong(graphSpace, belong);
+ this.invalidateUserCache();
+ } catch (Exception e) {
+ throw new HugeException("Exception occurs when " +
+ "set default graph", e);
+ }
+ }
+
+ @Override
+ public void unsetDefaultGraph(String graphSpace, String graph, String
user) {
+ String role = graph + DEFAULT_SETTER_ROLE_KEY;
+ String belongId = this.metaManager.belongId(user, role);
+ Id id = IdGenerator.of(belongId);
+ // Check if belong exists before attempting to delete to make this
operation idempotent
+ if (!this.metaManager.existBelong(graphSpace, id)) {
+ // Already unset, treat as success (idempotent behavior)
+ return;
+ }
+ try {
+ this.metaManager.deleteBelong(graphSpace, id);
+ this.invalidateUserCache();
+ } catch (Exception e) {
+ throw new HugeException("Exception occurs when unset default " +
+ "graph", e);
+ }
+ }
+
+ @Override
+ public Map<String, Date> getDefaultGraph(String graphSpace, String user) {
+ List<HugeBelong> belongs = this.listBelongBySource(graphSpace,
+
IdGenerator.of(user),
+ HugeBelong.UR, -1);
+ Map<String, Date> map = new HashMap<>();
+ for (HugeBelong belong : belongs) {
+ String role = belong.target().asString();
+ if (role.endsWith(DEFAULT_SETTER_ROLE_KEY) &&
+ role.length() != DEFAULT_SETTER_ROLE_KEY.length()) {
+ map.put(role.substring(0, role.lastIndexOf(
+ DEFAULT_SETTER_ROLE_KEY)), belong.update());
+ }
+ }
+ return map;
+ }
+
+ @Override
+ public Id createDefaultRole(String graphSpace, String owner,
+ HugeDefaultRole role, String graph) {
+ String roleName = (role.isGraphRole()) ?
+ getGraphDefaultRole(graph, role.toString()) : role.toString();
+ try {
+ HugeBelong belong;
+ String link;
+ if (HugeGroup.isGroup(owner)) {
+ belong = new HugeBelong(graphSpace, null,
+ IdGenerator.of(owner),
+ IdGenerator.of(roleName),
+ HugeBelong.GR);
+ link = HugeBelong.GR;
+ } else {
+ belong = new HugeBelong(graphSpace, IdGenerator.of(owner),
+ null, IdGenerator.of(roleName),
+ HugeBelong.UR);
+ link = HugeBelong.UR;
+ }
+
+ // Idempotent: if binding already exists, treat as success
+ String belongId = this.metaManager.belongId(owner, roleName, link);
+ if (this.metaManager.existBelong(graphSpace,
IdGenerator.of(belongId))) {
+ return IdGenerator.of(belongId);
+ }
+
+ this.tryInitDefaultRole(graphSpace, roleName, graph);
+ this.updateCreator(belong);
+ belong.create(belong.update());
+ Id result = this.metaManager.createBelong(graphSpace, belong);
+ this.invalidateUserCache();
+ return result;
+ } catch (Exception e) {
+ throw new HugeException("Exception occurs when " +
+ "create " + role + ".", e);
+ }
+ }
+
+ @Override
+ public Id createSpaceDefaultRole(String graphSpace, String owner,
+ HugeDefaultRole role) {
+ return createDefaultRole(graphSpace, owner, role, ALL_GRAPHS);
+ }
+
+ @Override
+ public boolean isDefaultRole(String graphSpace, String owner,
+ HugeDefaultRole role) {
+ return isDefaultRole(graphSpace, owner, role.toString());
+ }
+
+ @Override
+ public boolean isDefaultRole(String graphSpace, String graph,
+ String owner, HugeDefaultRole role) {
+ String roleName = getGraphDefaultRole(graph, role.toString());
+ return isDefaultRole(graphSpace, owner, roleName);
+ }
+
+ @Override
+ public void deleteDefaultRole(String graphSpace, String owner,
+ HugeDefaultRole role) {
+ deleteDefaultRoleByName(graphSpace, owner, role.toString());
+ }
+
+ @Override
+ public void deleteDefaultRole(String graphSpace, String owner,
+ HugeDefaultRole role, String graph) {
+ String roleName = getGraphDefaultRole(graph, role.toString());
+ deleteDefaultRoleByName(graphSpace, owner, roleName);
+ }
+
+ private boolean isDefaultRole(String graphSpace, String owner,
+ String role) {
+ try {
+ String belongId;
+ if (HugeGroup.isGroup(owner)) {
+ belongId = this.metaManager.belongId(owner, role,
+ HugeBelong.GR);
+ return this.metaManager.existBelong(graphSpace,
+ IdGenerator.of(belongId));
+ }
+
+ List<HugeGroup> groups = this.listGroupsByUser(owner, -1);
+ for (HugeGroup group : groups) {
+ String belongIdG = this.metaManager.belongId(group.name(),
+ role,
+ HugeBelong.GR);
+ if (this.metaManager.existBelong(graphSpace,
+ IdGenerator.of(belongIdG))) {
+ return true;
+ }
+ }
+
+ belongId = this.metaManager.belongId(owner, role);
+ return this.metaManager.existBelong(graphSpace,
+ IdGenerator.of(belongId));
+ } catch (Exception e) {
+ throw new HugeException("Exception occurs when check if is " +
+ role + ".", e);
+ }
+ }
+
+ private void deleteDefaultRoleByName(String graphSpace, String owner,
+ String role) {
+ try {
+ String belongId;
+ if (HugeGroup.isGroup(owner)) {
+ belongId = this.metaManager.belongId(owner, role,
+ HugeBelong.GR);
+ } else {
+ belongId = this.metaManager.belongId(owner, role,
+ HugeBelong.UR);
+ }
+ this.metaManager.deleteBelong(graphSpace,
Review Comment:
⚠️ **Make default-role delete idempotent like unset default graph**
`deleteBelong()` validates existence, so deleting an already-removed default
role can still be wrapped as a server error. `unsetDefaultGraph()` now checks
`existBelong()` and treats missing bindings as success; default-role deletion
should follow the same behavior so retries or duplicate UI actions do not fail
unnecessarily.
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/space/GraphSpaceAPI.java:
##########
@@ -103,6 +107,161 @@ public Object get(@Context GraphManager manager,
return gsInfo;
}
+ @POST
+ @Timed
+ @Status(Status.CREATED)
+ @Consumes(APPLICATION_JSON)
+ @Produces(APPLICATION_JSON_WITH_CHARSET)
+ @Path("{graphspace}/role")
+ @RolesAllowed({"analyst"})
+ public String setDefaultRole(@Context GraphManager manager,
+ @PathParam("graphspace") String name,
+ JsonDefaultRole jsonRole) {
+ E.checkArgumentNotNull(jsonRole, "Request body cannot be null");
+ E.checkArgument(StringUtils.isNotEmpty(jsonRole.user),
+ "The 'user' field cannot be null or empty");
+ E.checkArgument(StringUtils.isNotEmpty(jsonRole.role),
+ "The 'role' field cannot be null or empty");
+ String user = jsonRole.user;
+ String graph = jsonRole.graph;
+ HugeDefaultRole role;
+ try {
+ role = HugeDefaultRole.valueOf(jsonRole.role.toUpperCase());
+ } catch (IllegalArgumentException e) {
+ E.checkArgument(false, "Invalid role value '%s'", jsonRole.role);
+ role = null; // unreachable, satisfies compiler
+ }
+ LOG.debug("Create default role: {} {} {}", user, role,
+ name);
+ AuthManager authManager;
+ try {
+ authManager = manager.authManager();
+ } catch (IllegalStateException e) {
+ throw new HugeException(STANDALONE_ERROR);
+ }
+ E.checkArgument(authManager.findUser(user) != null ||
+ authManager.findGroup(user) != null,
+ "The user or group is not exist");
+ // only admin can set space admin
+ if (!authManager.isAdminManager(HugeGraphAuthProxy.username()) &&
+ role.equals(HugeDefaultRole.SPACE)) {
+ throw new ForbiddenException("Forbidden to set role " +
role.toString());
+ }
+
+ boolean hasGraph = role.equals(HugeDefaultRole.OBSERVER);
+
+ E.checkArgument(!hasGraph || StringUtils.isNotEmpty(graph),
+ "Must set a graph for observer");
+
+ Map <String, String> result = new HashMap<>();
+ result.put("user", user);
+ result.put("role", jsonRole.role);
+ result.put("graphSpace", name);
+
+ if (hasGraph) {
+ authManager.createDefaultRole(name, user, role, graph);
+ result.put("graph", graph);
+ } else {
+ authManager.createSpaceDefaultRole(name, user, role);
+ }
+
+ return manager.serializer().writeMap(result);
+ }
+
+ @GET
+ @Timed
+ @Path("{graphspace}/role")
+ @Consumes(APPLICATION_JSON)
Review Comment:
🧹 **Drop `@Consumes` from entity-less role endpoints**
`GET` and `DELETE` here do not consume a request body, so declaring
`@Consumes(APPLICATION_JSON)` is misleading and can make generated clients
think they need to send a JSON entity/content type. Please remove it from the
entity-less role endpoints here, and from the similar
`ManagerAPI.checkDefaultRole()` endpoint.
--
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]
