Copilot commented on code in PR #3008:
URL: https://github.com/apache/hugegraph/pull/3008#discussion_r3189310411
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/auth/StandardAuthManager.java:
##########
@@ -813,6 +814,207 @@ 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);
+ }
+ Id userId = IdGenerator.of(user);
+ HugeBelong belong = new HugeBelong(userId, groupId);
+ belong.creator(user);
+ Id belongId = IdGenerator.of(userId.asString() + "->ug->" +
groupId.asString());
+ if (!this.belong.exists(belongId)) {
+ this.belong.add(belong);
+ } else {
+ belong.id(belongId);
+ this.belong.update(belong);
+ }
Review Comment:
`belongId` is being fabricated as `"{userId}->ug->{groupId}"` and then used
with `RelationshipManager.exists()/delete()/update()`. However
`RelationshipManager.save()` computes edge IDs from the actual edge it
constructs (and `HugeBelong.Schema` doesn’t use a custom string ID), so this
string-based ID is unlikely to match the real edge ID. This breaks idempotency
(second call may throw “already exists”) and can prevent `unsetDefaultGraph()`
from deleting the binding. Consider locating the existing belong edge by
listing belongs from `userId` and matching `target()==groupId` (or
catching/ignoring the duplicate-add exception), and delete/update using the
real edge id.
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/space/SchemaTemplateAPI.java:
##########
@@ -0,0 +1,221 @@
+/*
+ * 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.
+ */
+
+package org.apache.hugegraph.api.space;
+
+import java.util.Date;
+import java.util.Set;
+
+import org.apache.commons.lang3.StringUtils;
+import org.apache.hugegraph.HugeException;
+import org.apache.hugegraph.api.API;
+import org.apache.hugegraph.api.filter.StatusFilter;
+import org.apache.hugegraph.auth.HugeGraphAuthProxy;
+import org.apache.hugegraph.core.GraphManager;
+import org.apache.hugegraph.define.Checkable;
+import org.apache.hugegraph.server.RestServer;
+import org.apache.hugegraph.space.SchemaTemplate;
+import org.apache.hugegraph.util.E;
+import org.apache.hugegraph.util.Log;
+import org.slf4j.Logger;
+
+import com.codahale.metrics.annotation.Timed;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.google.common.collect.ImmutableMap;
+
+import io.swagger.v3.oas.annotations.tags.Tag;
+import jakarta.annotation.security.RolesAllowed;
+import jakarta.inject.Singleton;
+import jakarta.ws.rs.Consumes;
+import jakarta.ws.rs.DELETE;
+import jakarta.ws.rs.ForbiddenException;
+import jakarta.ws.rs.GET;
+import jakarta.ws.rs.POST;
+import jakarta.ws.rs.PUT;
+import jakarta.ws.rs.Path;
+import jakarta.ws.rs.PathParam;
+import jakarta.ws.rs.Produces;
+import jakarta.ws.rs.core.Context;
+import jakarta.ws.rs.core.SecurityContext;
+
+@Path("graphspaces/{graphspace}/schematemplates")
+@Singleton
+@Tag(name = "SchemaTemplateAPI")
+public class SchemaTemplateAPI extends API {
+
+ private static final Logger LOG = Log.logger(RestServer.class);
+
Review Comment:
Logger is initialized with `RestServer.class`, which will categorize
SchemaTemplateAPI logs under the wrong logger name. Use
`SchemaTemplateAPI.class` (or the current class) so log filtering/configuration
works as expected.
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/auth/StandardAuthManager.java:
##########
@@ -813,6 +814,207 @@ 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);
+ }
+ Id userId = IdGenerator.of(user);
+ HugeBelong belong = new HugeBelong(userId, groupId);
+ belong.creator(user);
+ Id belongId = IdGenerator.of(userId.asString() + "->ug->" +
groupId.asString());
+ if (!this.belong.exists(belongId)) {
+ this.belong.add(belong);
+ } else {
+ belong.id(belongId);
+ this.belong.update(belong);
+ }
+ }
+
+ @Override
+ public void unsetDefaultGraph(String graphSpace, String graph, String
user) {
+ String markerName = defaultGraphGroupName(graphSpace, graph);
+ Id groupId = IdGenerator.of(markerName);
+ Id userId = IdGenerator.of(user);
+ Id belongId = IdGenerator.of(userId.asString() + "->ug->" +
groupId.asString());
+ if (this.belong.exists(belongId)) {
+ this.belong.delete(belongId);
+ }
+ }
+
+ @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 Id createBelongBinding(String owner, Id groupId) {
+ Id userId = IdGenerator.of(owner);
+ HugeBelong belong = new HugeBelong(userId, groupId);
+ belong.creator(owner);
+ Id belongId = IdGenerator.of(
+ userId.asString() + "->ug->" + groupId.asString());
+ if (!this.belong.exists(belongId)) {
+ this.belong.add(belong);
+ } else {
Review Comment:
The helper methods for default-role persistence
(`createBelongBinding`/`removeBelongBinding`/`existsBelongBinding`) rely on the
same fabricated `"{userId}->ug->{groupId}"` edge id. If that ID doesn’t match
the actual `HugeBelong` edge id, role existence checks and deletions will
silently fail. Please use the actual edge id returned by
`RelationshipManager.add()` / discovered via relationship queries, rather than
assuming a string id format.
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/auth/HugeUser.java:
##########
@@ -287,7 +300,7 @@ public void initSchemaIfNeeded() {
.properties(properties)
.usePrimaryKeyId()
.primaryKeys(P.NAME)
- .nullableKeys(P.PHONE, P.EMAIL, P.AVATAR,
P.DESCRIPTION)
+ .nullableKeys(P.NICKNAME, P.PHONE,
P.EMAIL, P.AVATAR, P.DESCRIPTION)
.enableLabelIndex(true)
Review Comment:
Adding the `user_nickname` property key here only affects *new* auth graphs.
Since `initSchemaIfNeeded()` returns immediately when the `user` vertex label
already exists, existing deployments won’t get the new property key / nullable
key added, and attempts to write `~user_nickname` may fail at runtime. Consider
adding schema-upgrade logic when the label exists (create the property key if
missing and update/append it to the vertex label), or document a required
migration step.
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/profile/GraphsAPI.java:
##########
@@ -136,6 +222,61 @@ public Object get(@Context GraphManager manager,
return ImmutableMap.of("name", g.name(), "backend", g.backend());
}
+ @POST
+ @Timed
+ @Path("{name}/default")
+ @Produces(APPLICATION_JSON_WITH_CHARSET)
+ @RolesAllowed({"space_member", "$owner=$name"})
+ public Map<String, Object> setDefault(@Context GraphManager manager,
+ @Parameter(description = "The graph
space name")
+ @PathParam("graphspace") String
graphSpace,
+ @Parameter(description = "The graph
name")
+ @PathParam("name") String name) {
+ LOG.debug("Set default graph '{}' in graph space '{}'", name,
graphSpace);
+ E.checkArgument(manager.graph(graphSpace, name) != null,
+ "Graph '%s/%s' does not exist", graphSpace, name);
+ String user = HugeGraphAuthProxy.username();
+ AuthManager authManager = manager.authManager();
+ authManager.setDefaultGraph(graphSpace, name, user);
+ Map<String, Date> defaults = authManager.getDefaultGraph(graphSpace,
user);
+ return ImmutableMap.of("default_graph", defaults.keySet());
+ }
+
+ @DELETE
+ @Timed
+ @Path("{name}/default")
+ @Produces(APPLICATION_JSON_WITH_CHARSET)
+ @RolesAllowed({"space_member", "$owner=$name"})
+ public Map<String, Object> unsetDefault(@Context GraphManager manager,
Review Comment:
PR description documents default-graph endpoints as `GET .../default` and
`GET .../undefault`, but the implementation uses `POST {name}/default` and
`DELETE {name}/default`. Please align the PR description (or the API) so
frontend/API consumers have a single, consistent contract.
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/profile/GraphsAPI.java:
##########
@@ -207,11 +404,23 @@ public Object create(@Context GraphManager manager,
if (StringUtils.isEmpty(clone)) {
// Only check required parameters when creating new graph, not
when cloning
E.checkArgument(configs != null, "Config parameters cannot be
null");
- String[] requiredKeys = {"backend", "serializer", "store"};
- for (String key : requiredKeys) {
- Object value = configs.get(key);
- E.checkArgument(value instanceof String &&
!StringUtils.isEmpty((String) value),
- "Required parameter '%s' is missing or empty",
key);
+ // Always required by TinkerPop's GraphFactory.open()
+ configs.putIfAbsent("gremlin.graph",
+ "org.apache.hugegraph.HugeFactory");
+ if (manager.isPDEnabled()) {
+ // Auto-fill HStore/PD mode defaults only when in distributed
mode
+ configs.putIfAbsent("backend", "hstore");
+ } else {
+ // Auto-fill standalone (RocksDB) mode defaults
+ configs.putIfAbsent("backend", "rocksdb");
+ }
Review Comment:
This change makes `backend`/`serializer`/`store` effectively optional by
auto-filling defaults. That will break existing REST tests that assert a 4xx
when required params are missing (e.g.
`GraphsApiTest#testcreateGraphInRocksDBWithMissingRequiredParams`). If the new
behavior is intended, please update/add tests accordingly; if not intended,
keep validation strict (or validate backend-specific required options so
invalid configs fail with a clear 4xx).
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/space/SchemaTemplateAPI.java:
##########
@@ -0,0 +1,221 @@
+/*
+ * 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.
+ */
+
+package org.apache.hugegraph.api.space;
+
+import java.util.Date;
+import java.util.Set;
+
+import org.apache.commons.lang3.StringUtils;
+import org.apache.hugegraph.HugeException;
+import org.apache.hugegraph.api.API;
+import org.apache.hugegraph.api.filter.StatusFilter;
+import org.apache.hugegraph.auth.HugeGraphAuthProxy;
+import org.apache.hugegraph.core.GraphManager;
+import org.apache.hugegraph.define.Checkable;
+import org.apache.hugegraph.server.RestServer;
+import org.apache.hugegraph.space.SchemaTemplate;
+import org.apache.hugegraph.util.E;
+import org.apache.hugegraph.util.Log;
+import org.slf4j.Logger;
+
+import com.codahale.metrics.annotation.Timed;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.google.common.collect.ImmutableMap;
+
+import io.swagger.v3.oas.annotations.tags.Tag;
+import jakarta.annotation.security.RolesAllowed;
+import jakarta.inject.Singleton;
+import jakarta.ws.rs.Consumes;
+import jakarta.ws.rs.DELETE;
+import jakarta.ws.rs.ForbiddenException;
+import jakarta.ws.rs.GET;
+import jakarta.ws.rs.POST;
+import jakarta.ws.rs.PUT;
+import jakarta.ws.rs.Path;
+import jakarta.ws.rs.PathParam;
+import jakarta.ws.rs.Produces;
+import jakarta.ws.rs.core.Context;
+import jakarta.ws.rs.core.SecurityContext;
+
+@Path("graphspaces/{graphspace}/schematemplates")
+@Singleton
+@Tag(name = "SchemaTemplateAPI")
+public class SchemaTemplateAPI extends API {
+
+ private static final Logger LOG = Log.logger(RestServer.class);
+
+ @GET
+ @Timed
+ @Produces(APPLICATION_JSON_WITH_CHARSET)
+ @RolesAllowed({"space_member", "$dynamic"})
+ public Object list(@Context GraphManager manager,
+ @PathParam("graphspace") String graphSpace) {
+ LOG.debug("List all schema templates for graph space {}", graphSpace);
+ ensurePdModeEnabled(manager);
+
+ Set<String> templates = manager.schemaTemplates(graphSpace);
+ return ImmutableMap.of("schema_templates", templates);
+ }
+
+ @GET
+ @Timed
+ @Path("{name}")
+ @Produces(APPLICATION_JSON_WITH_CHARSET)
+ @RolesAllowed({"space_member", "$dynamic"})
+ public Object get(@Context GraphManager manager,
+ @PathParam("graphspace") String graphSpace,
+ @PathParam("name") String name) {
+ LOG.debug("Get schema template by name '{}' for graph space {}",
+ name, graphSpace);
+ ensurePdModeEnabled(manager);
+
+ return manager.serializer().writeSchemaTemplate(
+ schemaTemplate(manager, graphSpace, name));
+ }
+
+ @POST
+ @Timed
+ @StatusFilter.Status(StatusFilter.Status.CREATED)
+ @Consumes(APPLICATION_JSON)
+ @Produces(APPLICATION_JSON_WITH_CHARSET)
+ @RolesAllowed({"space_member", "$dynamic"})
+ public String create(@Context GraphManager manager,
+ @PathParam("graphspace") String graphSpace,
+ JsonSchemaTemplate jsonSchemaTemplate) {
+ LOG.debug("Create schema template {} for graph space: '{}'",
+ jsonSchemaTemplate, graphSpace);
+ ensurePdModeEnabled(manager);
+ E.checkArgumentNotNull(jsonSchemaTemplate,
+ "Request body cannot be null or empty");
+ jsonSchemaTemplate.checkCreate(false);
+
+ E.checkArgument(manager.graphSpace(graphSpace) != null,
+ "The graph space '%s' is not exist", graphSpace);
+
+ SchemaTemplate template = jsonSchemaTemplate.toSchemaTemplate();
+ template.create(new Date());
+ template.creator(HugeGraphAuthProxy.username());
+ manager.createSchemaTemplate(graphSpace, template);
+ return manager.serializer().writeSchemaTemplate(template);
+ }
Review Comment:
This is a new public CRUD API surface (schema template
list/get/create/update/delete) but there are currently no REST tests covering
`schematemplates` endpoints in `hugegraph-test`. Please add basic API tests for
happy-path CRUD + permission checks (creator vs space manager) to prevent
regressions.
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/profile/GraphsAPI.java:
##########
@@ -120,6 +126,86 @@ public Object list(@Context GraphManager manager,
return ImmutableMap.of("graphs", filterGraphs);
}
+ @GET
+ @Timed
+ @Path("profile")
+ @Produces(APPLICATION_JSON_WITH_CHARSET)
+ @RolesAllowed({"space_member", "$dynamic"})
+ public Object listProfile(@Context GraphManager manager,
+ @Parameter(description = "The graph space name")
+ @PathParam("graphspace") String graphSpace,
+ @Parameter(description = "Filter graphs by name
or nickname prefix")
+ @QueryParam("prefix") String prefix,
+ @Context SecurityContext sc) {
Review Comment:
New endpoints/behaviors added here (graph profile listing with config
serialization + prefix filtering, and default graph set/unset/query) don’t
appear to be covered by existing REST tests (e.g. `hugegraph-test` has
`GraphsApiTest` but no coverage for `/graphs/profile` or
`/graphs/{name}/default`). Please add/extend tests to cover these paths and
expected response shapes/ordering.
--
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]
