Copilot commented on code in PR #2889:
URL:
https://github.com/apache/incubator-hugegraph/pull/2889#discussion_r2471612720
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/cypher/CypherAPI.java:
##########
@@ -71,31 +71,41 @@ private CypherManager cypherManager() {
@Timed
@CompressInterceptor.Compress(buffer = (1024 * 40))
@Produces(APPLICATION_JSON_WITH_CHARSET)
- public CypherModel query(@PathParam("graph") String graph, @Context
HttpHeaders headers,
+ public CypherModel query(@Context HttpHeaders headers,
+ @PathParam("graphspace") String graphspace,
+ @PathParam("graph") String graph,
@QueryParam("cypher") String cypher) {
- LOG.debug("Graph [{}] query by cypher: {}", graph, cypher);
- return this.queryByCypher(graph, headers, cypher);
+
+ return this.queryByCypher(headers, graphspace, graph, cypher);
}
+
@POST
@Timed
@CompressInterceptor.Compress
@Consumes(APPLICATION_JSON)
@Produces(APPLICATION_JSON_WITH_CHARSET)
- public CypherModel post(@PathParam("graph") String graph,
- @Context HttpHeaders headers, String cypher) {
- LOG.debug("Graph [{}] query by cypher: {}", graph, cypher);
- return this.queryByCypher(graph, headers, cypher);
+ public CypherModel post(@Context HttpHeaders headers,
+ @PathParam("graphspace") String graphspace,
+ @PathParam("graph") String graph,
+ String cypher) {
+
+ return this.queryByCypher(headers, graphspace, graph, cypher);
}
- private CypherModel queryByCypher(String graph, HttpHeaders headers,
String cypher) {
+ private CypherModel queryByCypher(HttpHeaders headers, String graphspace,
+ String graph, String cypher) {
+ E.checkArgument(graphspace != null && !graphspace.isEmpty(),
+ "The graphspace parameter can't be null or empty");
E.checkArgument(graph != null && !graph.isEmpty(),
"The graph parameter can't be null or empty");
E.checkArgument(cypher != null && !cypher.isEmpty(),
"The cypher parameter can't be null or empty");
- Map<String, String> aliases = new HashMap<>(1, 1);
- aliases.put("g", "__g_" + graph);
+ String graphInfo = graphspace + "-" + graph;
+ Map<String, String> aliases = new HashMap<>(2, 1);
Review Comment:
The HashMap is initialized with a load factor of 1, which will cause
immediate resizing when a second element is added. Use the default load factor
of 0.75 by calling `new HashMap<>(2)` instead, or increase the initial capacity
to 3 to accommodate 2 elements without resizing.
```suggestion
Map<String, String> aliases = new HashMap<>(3);
```
##########
hugegraph-pd/hg-pd-service/src/main/java/org/apache/hugegraph/pd/watch/KvWatchSubject.java:
##########
@@ -207,19 +210,21 @@ public void keepClientAlive() {
value.onNext(testAlive);
}
Map<String, String> clientKeys =
kvService.scanWithPrefix(clientKey);
- for (Map.Entry<String, String> keyEntry :
clientKeys.entrySet()) {
+ Set<Map.Entry<String, String>> set = clientKeys.entrySet();
+ for (Map.Entry<String, String> keyEntry : set) {
Review Comment:
The intermediate variable `set` is unnecessary. Iterate directly over
`clientKeys.entrySet()` to simplify the code: `for (Map.Entry<String, String>
keyEntry : clientKeys.entrySet())`.
```suggestion
for (Map.Entry<String, String> keyEntry :
clientKeys.entrySet()) {
```
##########
hugegraph-pd/hg-pd-service/src/main/java/org/apache/hugegraph/pd/service/interceptor/Authentication.java:
##########
@@ -0,0 +1,95 @@
+/*
+ * 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.pd.service.interceptor;
+
+import java.nio.charset.StandardCharsets;
+import java.util.Base64;
+import java.util.Set;
+import java.util.function.Function;
+import java.util.function.Supplier;
+
+import org.apache.commons.lang3.StringUtils;
+import org.springframework.security.access.AccessDeniedException;
+import org.springframework.security.authentication.BadCredentialsException;
+import org.springframework.stereotype.Component;
+
+/**
+ * Simple internal authentication component for PD service.
+ * <p>
+ * <b>WARNING:</b> This class currently implements only basic internal
authentication
+ * validation for internal modules (hg, store, hubble, vermeer). The
authentication mechanism
+ * is designed for internal service-to-service communication only.
+ * </p>
+ *
+ * <p><b>Important SEC Considerations:</b></p>
+ * <ul>
+ * <li><b>DO NOT expose RPC interfaces to external networks</b> - This
authentication is NOT
+ * designed for public-facing services and should only be used in
trusted internal networks.</li>
+ * <li><b>Production Environment Best Practices:</b> It is STRONGLY
RECOMMENDED to configure
+ * IP whitelisting and network-level access control policies (e.g.,
firewall rules,
+ * security groups) to restrict access to trusted sources only.</li>
+ * <li><b>Future Improvements:</b> This authentication mechanism will be
enhanced in future
+ * versions with more robust security features. Do not rely on this as
the sole security
+ * measure for production deployments.</li>
+ * </ul>
+ *
+ * <p>
+ * For production deployments, ensure proper network isolation and implement
defense-in-depth
+ * strategies including but not limited to:
+ * - VPC isolation
+ * - IP whitelisting
+ * - TLS/mTLS encryption,
+ * and regular security audits.
+ * </p>
+ */
+@Component
+public class Authentication {
+ private static final Set<String> innerModules = Set.of("hg", "store",
"hubble", "vermeer");
+
+ protected <T> T authenticate(String authority, String token,
Function<String, T> tokenCall,
+ Supplier<T> call) {
+ try {
+ String invalidBasicInfo = "invalid basic authentication info";
+ if (StringUtils.isEmpty(authority)) {
+ throw new BadCredentialsException(invalidBasicInfo);
+ }
+ byte[] bytes = authority.getBytes(StandardCharsets.UTF_8);
+ byte[] decode = Base64.getDecoder().decode(bytes);
+ String info = new String(decode);
+ int delim = info.indexOf(':');
+ if (delim == -1) {
+ throw new BadCredentialsException(invalidBasicInfo);
+ }
+
+ String name = info.substring(0, delim);
+ //String pwd = info.substring(delim + 1);
Review Comment:
The commented-out password extraction code suggests incomplete
implementation. Either remove the commented code if password validation is
intentionally not implemented, or implement proper password verification if
it's a work-in-progress feature.
```suggestion
```
##########
hugegraph-pd/hg-pd-service/src/main/java/org/apache/hugegraph/pd/rest/PartitionAPI.java:
##########
@@ -132,13 +142,12 @@ public RestApiResponse getHighLevelPartitions() {
partition2DataSize.getOrDefault(resultPartition.partitionId, 0L);
for (ShardStats shard : resultPartition.shards) {
// Assign values to the address and partition information
of the replica
- shard.address = storesMap.get(shard.storeId).getAddress();
- shard.partitionId = partition.getId();
- }
- if ((partitionStats != null) && (partitionStats.getLeader() !=
null)) {
- long storeId = partitionStats.getLeader().getStoreId();
- resultPartition.leaderAddress =
- storesMap.get(storeId).getAddress();
+ Metapb.Store s = storesMap.get(shard.storeId);
+ shard.address = (s != null) ? s.getAddress() : "";
+ if (s == null) {
+ log.error("store not found for shard storeId={},
partitionId={}",
+ shard.storeId, partition.getId());
Review Comment:
The null check for store `s` is performed twice - once in the ternary
operator and again in the if statement. Consider restructuring to eliminate
redundancy: check for null once, log the error, and set the address in a single
conditional block.
```suggestion
if (s == null) {
log.error("store not found for shard storeId={},
partitionId={}",
shard.storeId, partition.getId());
shard.address = "";
} else {
shard.address = s.getAddress();
```
##########
hugegraph-pd/hg-pd-service/src/main/java/org/apache/hugegraph/pd/util/DateUtil.java:
##########
@@ -26,21 +26,21 @@
public class DateUtil {
- private static final String DATE = "yyyy-MM-dd";
- private static final String DATETIME = "yyyy-MM-dd HH:mm:ss";
- private static final String DATETIME_MM = "yyyy-MM-dd HH:mm";
- private static final String DATETIME_SSS = "yyyy-MM-dd HH:mm:ss.SSS";
- private static final String TIME = "HH:mm";
- private static final String TIME_SS = "HH:mm:ss";
- private static final String SYS_DATE = "yyyy/MM/dd";
- private static final String SYS_DATETIME = "yyyy/MM/dd HH:mm:ss";
- private static final String SYS_DATETIME_MM = "yyyy/MM/dd HH:mm";
- private static final String SYS_DATETIME_SSS = "yyyy/MM/dd HH:mm:ss.SSS";
- private static final String NONE_DATE = "yyyyMMdd";
- private static final String NONE_DATETIME = "yyyyMMddHHmmss";
- private static final String NONE_DATETIME_MM = "yyyyMMddHHmm";
- private static final String NONE_DATETIME_SSS = "yyyyMMddHHmmssSSS";
- private static final String[] PATTERNS = new String[]{
+ private static String DATE = "yyyy-MM-dd";
+ private static String DATETIME = "yyyy-MM-dd HH:mm:ss";
+ private static String DATETIME_MM = "yyyy-MM-dd HH:mm";
+ private static String DATETIME_SSS = "yyyy-MM-dd HH:mm:ss.SSS";
+ private static String TIME = "HH:mm";
+ private static String TIME_SS = "HH:mm:ss";
+ private static String SYS_DATE = "yyyy/MM/dd";
+ private static String SYS_DATETIME = "yyyy/MM/dd HH:mm:ss";
+ private static String SYS_DATETIME_MM = "yyyy/MM/dd HH:mm";
+ private static String SYS_DATETIME_SSS = "yyyy/MM/dd HH:mm:ss.SSS";
+ private static String NONE_DATE = "yyyyMMdd";
+ private static String NONE_DATETIME = "yyyyMMddHHmmss";
+ private static String NONE_DATETIME_MM = "yyyyMMddHHmm";
+ private static String NONE_DATETIME_SSS = "yyyyMMddHHmmssSSS";
+ private static String[] PATTERNS = new String[]{
Review Comment:
These pattern strings should be declared as `static final` constants rather
than mutable `static` fields. Change `private static String` to `private static
final String` for all pattern fields to prevent accidental modification and
clearly indicate they are constants.
```suggestion
private static final String DATE = "yyyy-MM-dd";
private static final String DATETIME = "yyyy-MM-dd HH:mm:ss";
private static final String DATETIME_MM = "yyyy-MM-dd HH:mm";
private static final String DATETIME_SSS = "yyyy-MM-dd HH:mm:ss.SSS";
private static final String TIME = "HH:mm";
private static final String TIME_SS = "HH:mm:ss";
private static final String SYS_DATE = "yyyy/MM/dd";
private static final String SYS_DATETIME = "yyyy/MM/dd HH:mm:ss";
private static final String SYS_DATETIME_MM = "yyyy/MM/dd HH:mm";
private static final String SYS_DATETIME_SSS = "yyyy/MM/dd HH:mm:ss.SSS";
private static final String NONE_DATE = "yyyyMMdd";
private static final String NONE_DATETIME = "yyyyMMddHHmmss";
private static final String NONE_DATETIME_MM = "yyyyMMddHHmm";
private static final String NONE_DATETIME_SSS = "yyyyMMddHHmmssSSS";
private static final String[] PATTERNS = new String[]{
```
##########
hugegraph-pd/hg-pd-service/src/main/java/org/apache/hugegraph/pd/service/PDService.java:
##########
@@ -1760,36 +1779,229 @@ public void getPartitions(GetGraphRequest request,
observer.onCompleted();
}
- private List<KVPair<String, PeerId>> parseConfig(String conf) {
- List<KVPair<String, PeerId>> result = new LinkedList<>();
-
- if (conf != null && conf.length() > 0) {
- for (var s : conf.split(",")) {
- if (s.endsWith("/leader")) {
- result.add(new KVPair<>("leader",
-
JRaftUtils.getPeerId(s.substring(0, s.length() - 7))));
- } else if (s.endsWith("/learner")) {
- result.add(new KVPair<>("learner",
-
JRaftUtils.getPeerId(s.substring(0, s.length() - 8))));
- } else if (s.endsWith("/follower")) {
- result.add(new KVPair<>("follower",
-
JRaftUtils.getPeerId(s.substring(0, s.length() - 9))));
- } else {
- result.add(new KVPair<>("follower",
JRaftUtils.getPeerId(s)));
+ @Override
+ public void getGraphStats(GetGraphRequest request,
+ io.grpc.stub.StreamObserver<GraphStatsResponse>
observer) {
+ if (!isLeader()) {
+ redirectToLeader(PDGrpc.getGetGraphStatsMethod(), request,
observer);
+ return;
+ }
+ String graphName = request.getGraphName();
+ GraphStatsResponse.Builder builder = GraphStatsResponse.newBuilder();
+ try {
+ List<Metapb.Store> stores = storeNodeService.getStores(graphName);
+ long dataSize = 0;
+ long keySize = 0;
+ for (Metapb.Store store : stores) {
+ List<GraphStats> gss = store.getStats().getGraphStatsList();
+ if (gss.size() > 0) {
+ String gssGraph = gss.get(0).getGraphName();
+ String suffix = "/g";
+ if (gssGraph.split("/").length > 2 &&
!graphName.endsWith(suffix)) {
+ graphName += suffix;
+ }
Review Comment:
The suffix variable is defined locally but represents a constant value.
Consider defining it as a class-level constant (e.g., `private static final
String GRAPH_NAME_SUFFIX = \"/g\";`) to improve maintainability and reusability.
--
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]
