Copilot commented on code in PR #2921:
URL:
https://github.com/apache/incubator-hugegraph/pull/2921#discussion_r2659463940
##########
hugegraph-cluster-test/hugegraph-clustertest-test/src/main/java/org/apache/hugegraph/SimpleClusterTest/BaseSimpleTest.java:
##########
@@ -66,4 +91,101 @@ protected String execCmd(String[] cmds) throws IOException {
return builder.toString();
}
+ protected static void initGraph() {
+ Response r = client.get(URL_PREFIX);
+ if (r.getStatus() != 200) {
+ String body = "{\n" +
+ " \"backend\": \"hstore\",\n" +
+ " \"serializer\": \"binary\",\n" +
+ " \"store\": \"hugegraphapi\",\n" +
+ " \"search.text_analyzer\": \"jieba\",\n" +
+ " \"search.text_analyzer_mode\": \"INDEX\"\n" +
+ "}";
+ r = client.post(URL_PREFIX, body);
+ if (r.getStatus() != 201) {
+ throw new HugeException("Failed to create graph: " + GRAPH +
+ r.readEntity(String.class));
Review Comment:
The error message construction concatenates the graph name directly with the
response entity without any separator or clear message structure. This could
result in a confusing error message like "Failed to create graph:
hugegraphapi{response}". Consider adding proper formatting or separators.
```suggestion
", response: " +
r.readEntity(String.class));
```
##########
hugegraph-cluster-test/hugegraph-clustertest-test/src/main/java/org/apache/hugegraph/MultiClusterTest/BaseMultiClusterTest.java:
##########
@@ -20,32 +20,72 @@
import java.io.BufferedReader;
import java.io.IOException;
import java.io.InputStreamReader;
+import java.util.ArrayList;
+import java.util.List;
+import org.apache.hugegraph.SimpleClusterTest.BaseSimpleTest;
+import org.apache.hugegraph.SimpleClusterTest.BaseSimpleTest.RestClient;
import org.apache.hugegraph.ct.env.BaseEnv;
import org.apache.hugegraph.ct.env.MultiNodeEnv;
+import org.apache.hugegraph.serializer.direct.util.HugeException;
import org.junit.AfterClass;
+import org.junit.Assert;
import org.junit.BeforeClass;
+import jakarta.ws.rs.core.Response;
+
/**
* MultiNode Test generate the cluster env with 3 pd node + 3 store node + 3
server node.
* Or you can set different num of nodes by using env = new
MultiNodeEnv(pdNum, storeNum, serverNum)
* All nodes are deployed in ports generated randomly, the application of
nodes are stored
- * in /apache-hugegraph-ct-incubating-1.5.0, you can visit each node with rest
api.
+ * in /apache-hugegraph-ct-incubating-1.7.0, you can visit each node with rest
api.
*/
public class BaseMultiClusterTest {
protected static BaseEnv env;
protected static Process p;
+ protected static List<RestClient> clients = new ArrayList<>();
+ protected static String BASE_URL = "http://";;
+ protected static final String GRAPH = "hugegraphapi";
+ protected static final String URL_PREFIX = "graphspaces/DEFAULT/graphs/" +
GRAPH;
+ protected static final String SCHEMA_PKS = "/schema/propertykeys";
@BeforeClass
public static void initEnv() {
env = new MultiNodeEnv();
env.startCluster();
+ clients.clear();
+ for (String addr : env.getServerRestAddrs()) {
+ clients.add(new RestClient(BASE_URL + addr));
+ }
+ initGraph();
}
@AfterClass
public static void clearEnv() {
env.stopCluster();
+ for (RestClient client : clients) {
+ client.close();
+ }
+ }
+
+ protected static void initGraph() {
+ BaseSimpleTest.RestClient client = clients.get(0);
+ Response r = clients.get(0).get(URL_PREFIX);
Review Comment:
The variable 'client' on line 73 is declared but not used in this method.
The method calls clients.get(0) directly multiple times. Consider using the
declared variable consistently or removing it.
```suggestion
Response r = client.get(URL_PREFIX);
```
##########
hugegraph-cluster-test/hugegraph-clustertest-dist/src/assembly/static/conf/rest-server.properties.template:
##########
@@ -36,7 +36,12 @@ arthas.disabled_commands=jad
# authentication configs
# choose 'org.apache.hugegraph.auth.StandardAuthenticator' or
# 'org.apache.hugegraph.auth.ConfigAuthenticator'
-#auth.authenticator=
+auth.authenticator=org.apache.hugegraph.auth.StandardAuthenticator
+# true if using hstore mode
+usePD=true
+
+# default password
+auth.admin_pa=pa
Review Comment:
The password is set to "pa" which is extremely weak and insecure. Even for
testing environments, this poses a security risk if the server is accessible.
Consider using a more secure default password.
```suggestion
# default password (please change this value before production use)
auth.admin_pa=ChangeMe_Admin_Password_9f8c2B7a!
```
##########
hugegraph-cluster-test/hugegraph-clustertest-test/src/main/java/org/apache/hugegraph/MultiClusterTest/BaseMultiClusterTest.java:
##########
@@ -20,32 +20,72 @@
import java.io.BufferedReader;
import java.io.IOException;
import java.io.InputStreamReader;
+import java.util.ArrayList;
+import java.util.List;
+import org.apache.hugegraph.SimpleClusterTest.BaseSimpleTest;
+import org.apache.hugegraph.SimpleClusterTest.BaseSimpleTest.RestClient;
import org.apache.hugegraph.ct.env.BaseEnv;
import org.apache.hugegraph.ct.env.MultiNodeEnv;
+import org.apache.hugegraph.serializer.direct.util.HugeException;
import org.junit.AfterClass;
+import org.junit.Assert;
import org.junit.BeforeClass;
+import jakarta.ws.rs.core.Response;
+
/**
* MultiNode Test generate the cluster env with 3 pd node + 3 store node + 3
server node.
* Or you can set different num of nodes by using env = new
MultiNodeEnv(pdNum, storeNum, serverNum)
* All nodes are deployed in ports generated randomly, the application of
nodes are stored
- * in /apache-hugegraph-ct-incubating-1.5.0, you can visit each node with rest
api.
+ * in /apache-hugegraph-ct-incubating-1.7.0, you can visit each node with rest
api.
*/
public class BaseMultiClusterTest {
protected static BaseEnv env;
protected static Process p;
+ protected static List<RestClient> clients = new ArrayList<>();
+ protected static String BASE_URL = "http://";;
+ protected static final String GRAPH = "hugegraphapi";
+ protected static final String URL_PREFIX = "graphspaces/DEFAULT/graphs/" +
GRAPH;
+ protected static final String SCHEMA_PKS = "/schema/propertykeys";
@BeforeClass
public static void initEnv() {
env = new MultiNodeEnv();
env.startCluster();
+ clients.clear();
+ for (String addr : env.getServerRestAddrs()) {
+ clients.add(new RestClient(BASE_URL + addr));
+ }
+ initGraph();
}
@AfterClass
public static void clearEnv() {
env.stopCluster();
+ for (RestClient client : clients) {
+ client.close();
+ }
+ }
+
+ protected static void initGraph() {
+ BaseSimpleTest.RestClient client = clients.get(0);
+ Response r = clients.get(0).get(URL_PREFIX);
+ if (r.getStatus() != 200) {
+ String body = "{\n" +
+ " \"backend\": \"hstore\",\n" +
+ " \"serializer\": \"binary\",\n" +
+ " \"store\": \"hugegraphapi\",\n" +
+ " \"search.text_analyzer\": \"jieba\",\n" +
+ " \"search.text_analyzer_mode\": \"INDEX\"\n" +
+ "}";
+ r = client.post(URL_PREFIX, body);
+ if (r.getStatus() != 201) {
+ throw new HugeException("Failed to create graph: " + GRAPH +
+ r.readEntity(String.class));
Review Comment:
The error message construction concatenates the graph name directly with the
response entity without any separator or clear message structure. This could
result in a confusing error message like "Failed to create graph:
hugegraphapi{response}". Consider adding proper formatting or separators.
```suggestion
String responseBody = r.readEntity(String.class);
throw new HugeException(String.format(
"Failed to create graph '%s': %s",
GRAPH, responseBody));
```
##########
hugegraph-cluster-test/hugegraph-clustertest-test/src/main/java/org/apache/hugegraph/SimpleClusterTest/BaseSimpleTest.java:
##########
@@ -66,4 +91,101 @@ protected String execCmd(String[] cmds) throws IOException {
return builder.toString();
}
+ protected static void initGraph() {
+ Response r = client.get(URL_PREFIX);
+ if (r.getStatus() != 200) {
+ String body = "{\n" +
+ " \"backend\": \"hstore\",\n" +
+ " \"serializer\": \"binary\",\n" +
+ " \"store\": \"hugegraphapi\",\n" +
+ " \"search.text_analyzer\": \"jieba\",\n" +
+ " \"search.text_analyzer_mode\": \"INDEX\"\n" +
+ "}";
+ r = client.post(URL_PREFIX, body);
+ if (r.getStatus() != 201) {
+ throw new HugeException("Failed to create graph: " + GRAPH +
+ r.readEntity(String.class));
+ }
+ }
+ }
+
+ public static class RestClient {
+
+ private final Client client;
+ private final WebTarget target;
+
+ public RestClient(String url) {
+ this.client = ClientBuilder.newClient();
+ this.client.register(EncodingFilter.class);
+ this.client.register(GZipEncoder.class);
+ this.client.register(HttpAuthenticationFeature.basic(USERNAME,
+ PASSWORD));
+ this.target = this.client.target(url);
+ }
Review Comment:
Missing input validation for the RestClient constructor. If the 'url'
parameter is null or empty, this could lead to unclear errors later. Consider
adding validation to fail fast with a clear error message.
##########
hugegraph-cluster-test/hugegraph-clustertest-test/src/main/java/org/apache/hugegraph/SimpleClusterTest/BaseSimpleTest.java:
##########
@@ -20,36 +20,61 @@
import java.io.BufferedReader;
import java.io.IOException;
import java.io.InputStreamReader;
+import java.util.Map;
import org.apache.hugegraph.ct.env.BaseEnv;
import org.apache.hugegraph.ct.env.SimpleEnv;
-import org.apache.hugegraph.driver.HugeClient;
import org.apache.hugegraph.pd.client.PDClient;
+import org.apache.hugegraph.serializer.direct.util.HugeException;
+import org.glassfish.jersey.client.authentication.HttpAuthenticationFeature;
+import org.glassfish.jersey.client.filter.EncodingFilter;
+import org.glassfish.jersey.message.GZipEncoder;
import org.junit.AfterClass;
+import org.junit.Assert;
import org.junit.BeforeClass;
+import com.google.common.collect.Multimap;
+
+import jakarta.ws.rs.client.Client;
+import jakarta.ws.rs.client.ClientBuilder;
+import jakarta.ws.rs.client.Entity;
+import jakarta.ws.rs.client.WebTarget;
+import jakarta.ws.rs.core.MultivaluedMap;
+import jakarta.ws.rs.core.Response;
+
/**
* Simple Test generate the cluster env with 1 pd node + 1 store node + 1
server node.
* All nodes are deployed in ports generated randomly; The application of
nodes is stored
- * in /apache-hugegraph-ct-incubating-1.5.0, you can visit each node with rest
api.
+ * in /apache-hugegraph-ct-incubating-1.7.0, you can visit each node with rest
api.
*/
public class BaseSimpleTest {
protected static BaseEnv env;
protected static Process p;
protected static PDClient pdClient;
- protected static HugeClient hugeClient;
+
+ protected static String BASE_URL = "http://";;
+ protected static final String GRAPH = "hugegraphapi";
+ protected static final String USERNAME = "admin";
+ private static final String PASSWORD = "pa";
+
+ protected static final String URL_PREFIX = "graphspaces/DEFAULT/graphs/" +
GRAPH;
+ protected static final String SCHEMA_PKS = "/schema/propertykeys";
+ protected static RestClient client;
Review Comment:
The password is set to "pa" which is extremely weak and insecure. Even for
testing purposes, this could be a security risk if the test environment is
accessible. Consider using a more secure default password or generating one
dynamically.
```suggestion
private static final String PASSWORD = initPassword();
protected static final String URL_PREFIX = "graphspaces/DEFAULT/graphs/"
+ GRAPH;
protected static final String SCHEMA_PKS = "/schema/propertykeys";
protected static RestClient client;
private static String initPassword() {
String password = System.getenv("HUGEGRAPH_TEST_PASSWORD");
if (password != null && !password.isEmpty()) {
return password;
}
password = System.getProperty("hugegraph.test.password");
if (password != null && !password.isEmpty()) {
return password;
}
// Stronger default password for test environments; override via env
or system property in shared setups
return "ChangeMe_9f8a1c4e5b7d";
}
```
##########
hugegraph-cluster-test/hugegraph-clustertest-test/src/main/java/org/apache/hugegraph/SimpleClusterTest/BaseSimpleTest.java:
##########
@@ -66,4 +91,101 @@ protected String execCmd(String[] cmds) throws IOException {
return builder.toString();
}
+ protected static void initGraph() {
+ Response r = client.get(URL_PREFIX);
+ if (r.getStatus() != 200) {
+ String body = "{\n" +
+ " \"backend\": \"hstore\",\n" +
+ " \"serializer\": \"binary\",\n" +
+ " \"store\": \"hugegraphapi\",\n" +
+ " \"search.text_analyzer\": \"jieba\",\n" +
+ " \"search.text_analyzer_mode\": \"INDEX\"\n" +
+ "}";
+ r = client.post(URL_PREFIX, body);
+ if (r.getStatus() != 201) {
+ throw new HugeException("Failed to create graph: " + GRAPH +
+ r.readEntity(String.class));
+ }
+ }
+ }
+
+ public static class RestClient {
+
+ private final Client client;
+ private final WebTarget target;
+
+ public RestClient(String url) {
+ this.client = ClientBuilder.newClient();
+ this.client.register(EncodingFilter.class);
+ this.client.register(GZipEncoder.class);
+ this.client.register(HttpAuthenticationFeature.basic(USERNAME,
+ PASSWORD));
+ this.target = this.client.target(url);
+ }
+
+ public void close() {
+ this.client.close();
+ }
+
+ public WebTarget target() {
+ return this.target;
+ }
+
+ public WebTarget target(String url) {
+ return this.client.target(url);
+ }
+
+ public Response get(String path) {
+ return this.target.path(path).request().get();
+ }
+
+ public Response get(String path, String id) {
+ return this.target.path(path).path(id).request().get();
+ }
+
+ public Response get(String path,
+ MultivaluedMap<String, Object> headers) {
+ return this.target.path(path).request().headers(headers).get();
+ }
+
+ public Response get(String path, Multimap<String, Object> params) {
+ WebTarget target = this.target.path(path);
Review Comment:
Potentially confusing name: method [get](1) also refers to field [target](2)
(as this.target).
##########
hugegraph-cluster-test/hugegraph-clustertest-test/src/main/java/org/apache/hugegraph/SimpleClusterTest/BaseSimpleTest.java:
##########
@@ -66,4 +91,101 @@ protected String execCmd(String[] cmds) throws IOException {
return builder.toString();
}
+ protected static void initGraph() {
+ Response r = client.get(URL_PREFIX);
+ if (r.getStatus() != 200) {
+ String body = "{\n" +
+ " \"backend\": \"hstore\",\n" +
+ " \"serializer\": \"binary\",\n" +
+ " \"store\": \"hugegraphapi\",\n" +
+ " \"search.text_analyzer\": \"jieba\",\n" +
+ " \"search.text_analyzer_mode\": \"INDEX\"\n" +
+ "}";
+ r = client.post(URL_PREFIX, body);
+ if (r.getStatus() != 201) {
+ throw new HugeException("Failed to create graph: " + GRAPH +
+ r.readEntity(String.class));
+ }
+ }
+ }
+
+ public static class RestClient {
+
+ private final Client client;
+ private final WebTarget target;
+
+ public RestClient(String url) {
+ this.client = ClientBuilder.newClient();
+ this.client.register(EncodingFilter.class);
+ this.client.register(GZipEncoder.class);
+ this.client.register(HttpAuthenticationFeature.basic(USERNAME,
+ PASSWORD));
+ this.target = this.client.target(url);
+ }
+
+ public void close() {
+ this.client.close();
+ }
+
+ public WebTarget target() {
+ return this.target;
+ }
+
+ public WebTarget target(String url) {
+ return this.client.target(url);
+ }
+
+ public Response get(String path) {
+ return this.target.path(path).request().get();
+ }
+
+ public Response get(String path, String id) {
+ return this.target.path(path).path(id).request().get();
+ }
+
+ public Response get(String path,
+ MultivaluedMap<String, Object> headers) {
+ return this.target.path(path).request().headers(headers).get();
+ }
+
+ public Response get(String path, Multimap<String, Object> params) {
+ WebTarget target = this.target.path(path);
+ for (Map.Entry<String, Object> entries : params.entries()) {
+ target = target.queryParam(entries.getKey(),
entries.getValue());
+ }
+ return target.request().get();
+ }
+
+ public Response get(String path, Map<String, Object> params) {
+ WebTarget target = this.target.path(path);
+ for (Map.Entry<String, Object> i : params.entrySet()) {
+ target = target.queryParam(i.getKey(), i.getValue());
+ }
+ return target.request().get();
Review Comment:
Potentially confusing name: method [get](1) also refers to field [target](2)
(as this.target).
```suggestion
WebTarget webTarget = this.target.path(path);
for (Map.Entry<String, Object> entries : params.entries()) {
webTarget = webTarget.queryParam(entries.getKey(),
entries.getValue());
}
return webTarget.request().get();
}
public Response get(String path, Map<String, Object> params) {
WebTarget webTarget = this.target.path(path);
for (Map.Entry<String, Object> i : params.entrySet()) {
webTarget = webTarget.queryParam(i.getKey(), i.getValue());
}
return webTarget.request().get();
```
##########
hugegraph-cluster-test/hugegraph-clustertest-minicluster/src/main/java/org/apache/hugegraph/ct/node/ServerNodeWrapper.java:
##########
@@ -67,6 +73,36 @@ private static void addJarsToClasspath(File directory,
List<String> classpath) {
}
}
+ private static void addCpJarsToClasspath(File directory, List<String>
classpath) {
+ // Add jar starts with hugegraph in proper order
+ String path = directory.getAbsolutePath();
+ for (String jar : hgJars) {
+ classpath.add(path + File.separator + jar);
+ }
+ if (directory.exists() && directory.isDirectory()) {
+ File[] files =
+ directory.listFiles((dir, name) -> name.endsWith(".jar")
&& !name.contains(
+ "hugegraph"));
+ if (files != null) {
+ for (File file : files) {
+ classpath.add(file.getAbsolutePath());
+ }
+ }
+ }
+ }
+
+ private void loadHgJars(){
+ try (InputStream is =
ServerNodeWrapper.class.getResourceAsStream("/jar.txt");
+ BufferedReader reader = new BufferedReader(new
InputStreamReader(is, StandardCharsets.UTF_8))) {
+ String line;
+ while ((line = reader.readLine()) != null) {
+ hgJars.add(line);
+ }
+ } catch (IOException e) {
+ e.printStackTrace();
Review Comment:
The error handling here only prints the stack trace to stderr. Consider
using a proper logger instead of printStackTrace(), and potentially re-throwing
the exception or handling it more gracefully since this occurs during
initialization.
```suggestion
LOG.error("Failed to load HugeGraph jars from resource
'/jar.txt'", e);
```
--
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]
