Copilot commented on code in PR #3008: URL: https://github.com/apache/hugegraph/pull/3008#discussion_r3279586693
########## hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/api/SchemaTemplateApiTest.java: ########## @@ -0,0 +1,135 @@ +/* + * 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; + +import org.junit.Assert; +import org.junit.Before; +import org.junit.Test; + +import com.google.common.collect.ImmutableMap; + Review Comment: Unused import 'ImmutableMap' will cause compilation to fail; remove it or use it in the test. ########## hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/space/SchemaTemplateAPI.java: ########## @@ -0,0 +1,220 @@ +/* + * 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.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(SchemaTemplateAPI.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); + } + + @DELETE + @Timed + @Path("{name}") + @Produces(APPLICATION_JSON_WITH_CHARSET) + @RolesAllowed({"space_member", "$dynamic"}) + public void delete(@Context GraphManager manager, + @PathParam("graphspace") String graphSpace, + @PathParam("name") String name, + @Context SecurityContext sc) { + LOG.debug("Remove schema template by name '{}' for graph space '{}'", + name, graphSpace); + ensurePdModeEnabled(manager); + E.checkArgument(manager.graphSpace(graphSpace) != null, + "The graph space '%s' is not exist", graphSpace); + + SchemaTemplate st = schemaTemplate(manager, graphSpace, name); + E.checkArgument(st != null, + "Schema template '%s' does not exist", name); + + String username = HugeGraphAuthProxy.username(); + boolean isSpace = manager.authManager() + .isSpaceManager(graphSpace, username); + if (st.creator().equals(username) || isSpace) { + manager.dropSchemaTemplate(graphSpace, name); + } else { + throw new ForbiddenException("No permission to delete schema template"); + } Review Comment: `st.creator()` can be null (SchemaTemplate allows null creator), so `st.creator().equals(username)` can throw NPE and block deletions. Use a null-safe comparison (e.g., username.equals(st.creator()) / Objects.equals) and decide how to authorize templates with missing creator. ########## hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/api/DefaultRoleApiStandaloneTest.java: ########## @@ -0,0 +1,185 @@ +/* + * 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; + +import java.util.Map; + +import org.apache.hugegraph.util.JsonUtil; +import org.junit.Assert; +import org.junit.Before; +import org.junit.Test; + +import com.google.common.collect.ImmutableMap; + +import jakarta.ws.rs.core.Response; Review Comment: Unused imports ('java.util.Map', 'org.apache.hugegraph.util.JsonUtil') will cause compilation to fail; remove them or use them. ########## hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/auth/HugeUser.java: ########## @@ -287,16 +302,27 @@ 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) .build(); this.graph.schemaTransaction().addVertexLabel(label); } + private void upgradeSchemaIfNeeded() { + // Add user_nickname property key if missing (new in this version) + if (!this.graph.graph().existsPropertyKey(P.NICKNAME)) { + createPropertyKey(P.NICKNAME); + VertexLabel vl = this.graph.graph().vertexLabel(this.label); + vl.nullableKey(this.graph.graph().propertyKey(P.NICKNAME).id()); Review Comment: In the incremental schema upgrade, you add `~user_nickname` as a new PropertyKey, but only call `vl.nullableKey(...)`. `nullableKey()` just marks a key nullable and does NOT add it to the vertex label's properties, so the label may still reject writes of `user_nickname`. Update the vertex label to include the new property as well (and then mark it nullable if desired). ########## hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/space/SchemaTemplateAPI.java: ########## @@ -0,0 +1,220 @@ +/* + * 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.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(SchemaTemplateAPI.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); + } + + @DELETE + @Timed + @Path("{name}") + @Produces(APPLICATION_JSON_WITH_CHARSET) + @RolesAllowed({"space_member", "$dynamic"}) + public void delete(@Context GraphManager manager, + @PathParam("graphspace") String graphSpace, + @PathParam("name") String name, + @Context SecurityContext sc) { + LOG.debug("Remove schema template by name '{}' for graph space '{}'", + name, graphSpace); + ensurePdModeEnabled(manager); + E.checkArgument(manager.graphSpace(graphSpace) != null, + "The graph space '%s' is not exist", graphSpace); + + SchemaTemplate st = schemaTemplate(manager, graphSpace, name); + E.checkArgument(st != null, + "Schema template '%s' does not exist", name); + + String username = HugeGraphAuthProxy.username(); + boolean isSpace = manager.authManager() + .isSpaceManager(graphSpace, username); + if (st.creator().equals(username) || isSpace) { + manager.dropSchemaTemplate(graphSpace, name); + } else { + throw new ForbiddenException("No permission to delete schema template"); + } + } + + @PUT + @Timed + @Path("{name}") + @Consumes(APPLICATION_JSON) + @Produces(APPLICATION_JSON_WITH_CHARSET) + @RolesAllowed({"space_member", "$dynamic"}) + public String update(@Context GraphManager manager, + @PathParam("graphspace") String graphSpace, + @PathParam("name") String name, + @Context SecurityContext sc, + JsonSchemaTemplate jsonSchemaTemplate) { + ensurePdModeEnabled(manager); + E.checkArgumentNotNull(jsonSchemaTemplate, + "Request body cannot be null or empty"); + jsonSchemaTemplate.checkUpdate(); + + SchemaTemplate old = schemaTemplate(manager, graphSpace, name); + if (null == old) { + throw new HugeException("Schema template '%s' does not exist", name); + } + + String username = HugeGraphAuthProxy.username(); + boolean isSpace = manager.authManager() + .isSpaceManager(graphSpace, username); + if (old.creator().equals(username) || isSpace) { + SchemaTemplate template = jsonSchemaTemplate.build(old); + template.creator(old.creator()); + template.create(old.create()); + template.refreshUpdateTime(); + + manager.updateSchemaTemplate(graphSpace, template); + return manager.serializer().writeSchemaTemplate(template); + } + throw new ForbiddenException("No permission to update schema template"); Review Comment: `old.creator()` can be null (SchemaTemplate allows null creator), so `old.creator().equals(username)` can throw NPE and block updates. Use a null-safe comparison (e.g., username.equals(old.creator()) / Objects.equals) and decide how to authorize templates with missing creator. ########## hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/auth/ManagerAPI.java: ########## @@ -259,6 +262,46 @@ public String getRolesInGs(@Context GraphManager manager, result)); } + @GET + @Timed + @Path("default") + @Consumes(APPLICATION_JSON) + public String checkDefaultRole(@Context GraphManager manager, + @PathParam("graphspace") String graphSpace, + @QueryParam("role") String role, + @QueryParam("graph") String graph) { + LOG.debug("check if current user is default role: {} {} {}", + role, graphSpace, graph); + ensurePdModeEnabled(manager); + AuthManager authManager = manager.authManager(); + String user = HugeGraphAuthProxy.username(); + + E.checkArgument(StringUtils.isNotEmpty(role) && + StringUtils.isNotEmpty(graphSpace), + "Must pass graphspace and role params"); + + HugeDefaultRole defaultRole; + try { + defaultRole = HugeDefaultRole.valueOf(role.toUpperCase()); + } catch (IllegalArgumentException e) { + E.checkArgument(false, "Invalid role value '%s'", role); + defaultRole = null; // unreachable, satisfies compiler + } + boolean hasGraph = defaultRole.equals(HugeDefaultRole.OBSERVER); + E.checkArgument(!hasGraph || StringUtils.isNotEmpty(graph), + "Must set a graph for observer"); + + boolean result; + if (hasGraph) { + result = authManager.isDefaultRole(graphSpace, graph, user, + defaultRole); + } else { + result = authManager.isDefaultRole(graphSpace, user, + defaultRole); + } + return manager.serializer().writeMap(ImmutableMap.of("check", result)); + } Review Comment: PR description mentions a global `GET /auth/manager/default`, but this implementation is scoped under the graphspace path (`/graphspaces/{graphspace}/auth/managers/default`). Please reconcile the PR description (or endpoint path) so frontend/backend agree. ########## hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/profile/GraphsAPI.java: ########## @@ -136,6 +227,76 @@ 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; + try { + authManager = manager.authManager(); + } catch (IllegalStateException e) { + throw new HugeException(STANDALONE_ERROR); + } + 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, + @Parameter(description = "The graph space name") + @PathParam("graphspace") String graphSpace, + @Parameter(description = "The graph name") + @PathParam("name") String name) { + LOG.debug("Unset 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; + try { + authManager = manager.authManager(); + } catch (IllegalStateException e) { + throw new HugeException(STANDALONE_ERROR); + } + authManager.unsetDefaultGraph(graphSpace, name, user); + Map<String, Date> defaults = authManager.getDefaultGraph(graphSpace, user); + return ImmutableMap.of("default_graph", defaults.keySet()); + } Review Comment: PR description says setting/unsetting default graph uses GET endpoints (and a separate `/undefault`), but the implementation exposes POST/DELETE on `/{name}/default`. Either update the PR description/API contract or adjust the endpoints to match what Hubble expects. -- 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]
