imbajin commented on code in PR #2900:
URL:
https://github.com/apache/incubator-hugegraph/pull/2900#discussion_r2489650036
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/space/GraphSpaceAPI.java:
##########
@@ -103,7 +104,7 @@ public String create(@Context GraphManager manager,
jsonGraphSpace.checkCreate(false);
- String creator = "admin";
+ String creator = HugeGraphAuthProxy.getContext().user().getName();
Review Comment:
‼️ **Critical: 获取用户名的方法不一致**
在 GraphsAPI.java 中使用了 `username()`，而在 GraphSpaceAPI.java 中使用了
`getName()`。这两个方法可能返回不同的值，导致 creator 字段的数据不一致。
建议统一使用同一个方法，例如都使用 `username()` 或都使用 `getName()`。请检查这两个方法的语义是否相同。
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/profile/GraphsAPI.java:
##########
@@ -198,8 +199,7 @@ public Object create(@Context GraphManager manager,
}
}
- // todo: auth get actual user info
- String creator = "admin";
+ String creator = HugeGraphAuthProxy.getContext().user().username();
Review Comment:
‼️ **Critical: 缺少空指针检查**
`HugeGraphAuthProxy.getContext().user()` 可能返回 null，如果在未认证的场景下调用会导致
NullPointerException。
建议添加空值检查并提供默认值或抛出更友好的异常：
```suggestion
UserElement user = HugeGraphAuthProxy.getContext().user();
String creator = user != null ? user.username() : "admin";
```
##########
hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/api/GraphsApiTest.java:
##########
@@ -18,42 +18,320 @@
package org.apache.hugegraph.api;
import java.util.HashMap;
+import java.util.List;
import java.util.Map;
+import org.apache.hugegraph.util.JsonUtil;
import org.junit.AfterClass;
-import org.junit.BeforeClass;
+import org.junit.Assert;
import org.junit.Test;
+import com.google.common.collect.ImmutableMap;
+
+import jakarta.ws.rs.client.Entity;
import jakarta.ws.rs.core.Response;
public class GraphsApiTest extends BaseApiTest {
- private static final String TEMP_SPACE = "graph_test";
- private static final String TEMP_AUTH_SPACE = "graph_auth_test";
- private static final String PATH = "graphspaces/graph_test/graphs";
- private static final String PATH_AUTH = "graphspaces/graph_auth_test" +
- "/graphs";
+ private static final String TEMP_SPACE = "DEFAULT";
+ private static final String PATH = "graphspaces/DEFAULT/graphs";
- @BeforeClass
- public static void prepareSpace() {
- createSpace(TEMP_SPACE, false);
- createSpace(TEMP_AUTH_SPACE, true);
- }
@AfterClass
public static void tearDown() {
clearSpaces();
}
+ @Test
+ public void testListGraphs() {
+ // Create multiple graphs
+ Response r1 = createGraphInRocksDB(TEMP_SPACE, "listtest1");
+ assertResponseStatus(201, r1);
+
+ Response r2 = createGraphInRocksDB(TEMP_SPACE, "listtest2");
+ assertResponseStatus(201, r2);
+
+ // List all graphs
Review Comment:
⚠️ **Important: 资源清理不完整**
测试方法创建了图但没有保证在异常情况下也能清理资源。建议使用 try-finally 或 @After 方法确保资源被正确清理：
```suggestion
@Test
public void testListGraphs() {
try {
Response r1 = createGraphInRocksDB(TEMP_SPACE, "listtest1");
assertResponseStatus(201, r1);
Response r2 = createGraphInRocksDB(TEMP_SPACE, "listtest2");
assertResponseStatus(201, r2);
// ... test logic ...
} finally {
Map<String, Object> params = ImmutableMap.of(
"confirm_message", "I'm sure to drop the graph");
client().delete(PATH + "/listtest1", params);
client().delete(PATH + "/listtest2", params);
}
}
```
##########
hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/api/BaseApiTest.java:
##########
@@ -661,6 +661,28 @@ public static Response createGraph(String graphSpace,
String name) {
return createGraph(graphSpace, name, name);
}
+ public static Response createGraphInRocksDB(String graphSpace, String
name) {
Review Comment:
🧹 **Minor: 重复的配置代码**
`createGraphInRocksDB` 方法中有大量重复的配置字符串。建议提取为常量或使用配置对象来构建，提高代码可维护性：
```suggestion
private static final String ROCKSDB_CONFIG_TEMPLATE =
"{ \"gremlin.graph\": \"org.apache.hugegraph.HugeFactory\"," +
"\"backend\": \"rocksdb\", \"serializer\": \"binary\"," +
"\"store\": \"%s\", \"nickname\": \"%s\"," +
"\"rocksdb.data_path\": \"rocksdbtest-data-%s\"," +
"\"rocksdb.wal_path\": \"rocksdbtest-data-%s\"," +
"\"search.text_analyzer\": \"jieba\"," +
"\"search.text_analyzer_mode\": \"INDEX\" }";
```
##########
hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/api/GraphsApiTest.java:
##########
@@ -18,42 +18,320 @@
package org.apache.hugegraph.api;
import java.util.HashMap;
+import java.util.List;
import java.util.Map;
+import org.apache.hugegraph.util.JsonUtil;
import org.junit.AfterClass;
-import org.junit.BeforeClass;
+import org.junit.Assert;
import org.junit.Test;
+import com.google.common.collect.ImmutableMap;
+
+import jakarta.ws.rs.client.Entity;
import jakarta.ws.rs.core.Response;
public class GraphsApiTest extends BaseApiTest {
- private static final String TEMP_SPACE = "graph_test";
- private static final String TEMP_AUTH_SPACE = "graph_auth_test";
- private static final String PATH = "graphspaces/graph_test/graphs";
- private static final String PATH_AUTH = "graphspaces/graph_auth_test" +
- "/graphs";
+ private static final String TEMP_SPACE = "DEFAULT";
+ private static final String PATH = "graphspaces/DEFAULT/graphs";
- @BeforeClass
- public static void prepareSpace() {
- createSpace(TEMP_SPACE, false);
- createSpace(TEMP_AUTH_SPACE, true);
- }
@AfterClass
public static void tearDown() {
clearSpaces();
}
+ @Test
+ public void testListGraphs() {
+ // Create multiple graphs
+ Response r1 = createGraphInRocksDB(TEMP_SPACE, "listtest1");
+ assertResponseStatus(201, r1);
+
+ Response r2 = createGraphInRocksDB(TEMP_SPACE, "listtest2");
+ assertResponseStatus(201, r2);
+
+ // List all graphs
+ Response r = client().get(PATH);
+ String content = assertResponseStatus(200, r);
+
+ Map<String, Object> result = JsonUtil.fromJson(content, Map.class);
+ Assert.assertTrue(result.containsKey("graphs"));
+
+ @SuppressWarnings("unchecked")
+ List<String> graphs = (List<String>) result.get("graphs");
+ Assert.assertTrue(graphs.contains("listtest1"));
+ Assert.assertTrue(graphs.contains("listtest2"));
+
+ // Clean up
+ Map<String, Object> params = ImmutableMap.of(
+ "confirm_message", "I'm sure to drop the graph");
+ client().delete(PATH + "/listtest1", params);
+ client().delete(PATH + "/listtest2", params);
+ }
+
+ @Test
+ public void testGetGraph() {
+ // Create a graph
+ Response r = createGraphInRocksDB(TEMP_SPACE, "get_test",
"GetTestGraph");
+ assertResponseStatus(201, r);
+
+ // Get the graph
+ Response getResponse = client().get(PATH + "/get_test");
+ String content = assertResponseStatus(200, getResponse);
+
+ Map<String, Object> result = JsonUtil.fromJson(content, Map.class);
+ Assert.assertTrue(result.containsKey("name"));
+ Assert.assertTrue(result.containsKey("backend"));
+ Assert.assertEquals("get_test", result.get("name"));
+
+ // Clean up
+ Map<String, Object> params = ImmutableMap.of(
+ "confirm_message", "I'm sure to drop the graph");
+ client().delete(PATH + "/get_test", params);
+ }
+
+ @Test
+ public void testcreateGraphInRocksDB() {
+ String config = "{\n" +
+ " \"gremlin.graph\":
\"org.apache.hugegraph.HugeFactory\",\n" +
+ " \"backend\": \"rocksdb\",\n" +
+ " \"serializer\": \"binary\",\n" +
+ " \"store\": \"create_test\",\n" +
+ " \"nickname\": \"CreateTestGraph\",\n" +
+ " \"description\": \"Test graph creation\",\n" +
+ " \"rocksdb.data_path\":
\"rocksdbtest-data-create_test\",\n" +
+ " \"rocksdb.wal_path\":
\"rocksdbtest-data-create_test\"\n" +
+ "}";
+
+ Response r = client().post(PATH + "/create_test",
+ Entity.json(config));
+ String content = assertResponseStatus(201, r);
+
+ Map<String, Object> result = JsonUtil.fromJson(content, Map.class);
+ Assert.assertEquals("create_test", result.get("name"));
+ Assert.assertEquals("CreateTestGraph", result.get("nickname"));
+ Assert.assertEquals("rocksdb", result.get("backend"));
+ Assert.assertEquals("Test graph creation", result.get("description"));
+
+ // Clean up
+ Map<String, Object> params = ImmutableMap.of(
+ "confirm_message", "I'm sure to drop the graph");
+ client().delete(PATH + "/create_test", params);
+ }
+
+ @Test
+ public void testcreateGraphInRocksDBWithMissingRequiredParams() {
+ // Missing 'backend' parameter
+ String config = "{\n" +
+ " \"serializer\": \"binary\",\n" +
+ " \"store\": \"invalid_test\"\n" +
+ "}";
+
+ Response r = client().post(PATH + "/invalid_test",
+ Entity.json(config));
+ Assert.assertTrue(r.getStatus() >= 400);
+ }
+
+ @Test
+ public void testCloneGraph() {
+ // Create source graph
+ Response r1 = createGraphInRocksDB(TEMP_SPACE, "clone_source",
"SourceGraph");
+ assertResponseStatus(201, r1);
+
+ // Clone the graph
+ String config = "{\n" +
+ " \"gremlin.graph\":
\"org.apache.hugegraph.HugeFactory\",\n" +
+ " \"backend\": \"rocksdb\",\n" +
+ " \"serializer\": \"binary\",\n" +
+ " \"store\": \"clone_target\",\n" +
+ " \"nickname\": \"ClonedGraph\",\n" +
+ " \"rocksdb.data_path\":
\"rocksdbtest-data-clone_target\",\n" +
+ " \"rocksdb.wal_path\":
\"rocksdbtest-data-clone_target\"\n" +
+ "}";
+
+ Map<String, Object> params = ImmutableMap.of(
+ "clone_graph_name", "clone_source");
+
+ String path = PATH + "/clone_target";
+ Response r = client().target(baseUrl())
+ .path(path)
+ .queryParam("clone_graph_name", "clone_source")
+ .request()
+ .post(Entity.json(config));
+
+ String content = assertResponseStatus(201, r);
+ Map<String, Object> result = JsonUtil.fromJson(content, Map.class);
+ Assert.assertEquals("clone_target", result.get("name"));
+
+ // Clean up
+ Map<String, Object> deleteParams = ImmutableMap.of(
+ "confirm_message", "I'm sure to drop the graph");
+ client().delete(PATH + "/clone_source", deleteParams);
+ client().delete(PATH + "/clone_target", deleteParams);
+ }
+
@Test
public void testDeleteGraph() {
- Response r = createGraph(TEMP_SPACE, "delete");
+ Response r = createGraphInRocksDB(TEMP_SPACE, "delete_test");
assertResponseStatus(201, r);
Map<String, Object> params = new HashMap<>();
params.put("confirm_message", "I'm sure to drop the graph");
- r = client().delete(PATH + "/delete", params);
+ r = client().delete(PATH + "/delete_test", params);
assertResponseStatus(204, r);
+
+ // Verify graph is deleted
+ Response getResponse = client().get(PATH + "/delete_test");
+ Assert.assertTrue(getResponse.getStatus() >= 400);
+ }
+
+ @Test
+ public void testDeleteGraphWithoutConfirmMessage() {
+ Response r = createGraphInRocksDB(TEMP_SPACE, "delete_no_confirm");
+ assertResponseStatus(201, r);
+
+ // Try to delete without confirmation
+ Response deleteResponse = client().delete(PATH + "/delete_no_confirm",
+ new HashMap<>());
+ Assert.assertTrue(deleteResponse.getStatus() >= 400);
+
+ // Clean up properly
+ Map<String, Object> params = ImmutableMap.of(
+ "confirm_message", "I'm sure to drop the graph");
+ client().delete(PATH + "/delete_no_confirm", params);
+ }
+
+ @Test
+ public void testClearGraph() {
+ Response r = createGraphInRocksDB(TEMP_SPACE, "clear_test");
+ assertResponseStatus(201, r);
+
+ Map<String, Object> params = ImmutableMap.of(
+ "confirm_message", "I'm sure to delete all data");
+
+ Response clearResponse = client().delete(PATH + "/clear_test/clear",
+ params);
+ assertResponseStatus(204, clearResponse);
+
+ // Clean up
+ Map<String, Object> deleteParams = ImmutableMap.of(
+ "confirm_message", "I'm sure to drop the graph");
+ client().delete(PATH + "/clear_test", deleteParams);
+ }
+
+ @Test
+ public void testClearGraphWithoutConfirmMessage() {
+ Response r = createGraphInRocksDB(TEMP_SPACE, "clear_no_confirm");
+ assertResponseStatus(201, r);
+
+ // Try to clear without confirmation
+ Response clearResponse = client().delete(PATH +
"/clear_no_confirm/clear",
+ new HashMap<>());
+ Assert.assertTrue(clearResponse.getStatus() >= 400);
+
+ // Clean up
+ Map<String, Object> params = ImmutableMap.of(
+ "confirm_message", "I'm sure to drop the graph");
+ client().delete(PATH + "/clear_no_confirm", params);
+ }
+
+ @Test
+ public void testSetGraphMode() {
+ Response r = createGraphInRocksDB(TEMP_SPACE, "mode_test");
+ assertResponseStatus(201, r);
+
+ // Set mode to RESTORING
+ String modeJson = "\"RESTORING\"";
+ Response modeResponse = client().target(baseUrl())
+ .path(PATH + "/mode_test/mode")
+ .request()
+ .put(Entity.json(modeJson));
+
+ String content = assertResponseStatus(200, modeResponse);
+ Map<String, Object> result = JsonUtil.fromJson(content, Map.class);
+ Assert.assertEquals("RESTORING", result.get("mode"));
+
+ // Clean up
+ Map<String, Object> params = ImmutableMap.of(
+ "confirm_message", "I'm sure to drop the graph");
+ client().delete(PATH + "/mode_test", params);
+ }
+
+ @Test
+ public void testGetGraphMode() {
+ Response r = createGraphInRocksDB(TEMP_SPACE, "get_mode_test");
+ assertResponseStatus(201, r);
+
+ Response modeResponse = client().get(PATH + "/get_mode_test/mode");
+ String content = assertResponseStatus(200, modeResponse);
+
+ Map<String, Object> result = JsonUtil.fromJson(content, Map.class);
+ Assert.assertTrue(result.containsKey("mode"));
+
+ // Clean up
+ Map<String, Object> params = ImmutableMap.of(
+ "confirm_message", "I'm sure to drop the graph");
+ client().delete(PATH + "/get_mode_test", params);
+ }
+
+ @Test
+ public void testSetGraphReadMode() {
+ Response r = createGraphInRocksDB(TEMP_SPACE, "read_mode_test");
+ assertResponseStatus(201, r);
+
+ // Set read mode to OLTP_ONLY
+ String readModeJson = "\"OLTP_ONLY\"";
+ Response readModeResponse = client().target(baseUrl())
+ .path(PATH +
"/read_mode_test/graph_read_mode")
+ .request()
+ .put(Entity.json(readModeJson));
+
+ String content = assertResponseStatus(200, readModeResponse);
+ Map<String, Object> result = JsonUtil.fromJson(content, Map.class);
+ Assert.assertEquals("OLTP_ONLY", result.get("graph_read_mode"));
+
+ // Clean up
+ Map<String, Object> params = ImmutableMap.of(
+ "confirm_message", "I'm sure to drop the graph");
+ client().delete(PATH + "/read_mode_test", params);
+ }
+
+ @Test
+ public void testGetGraphReadMode() {
+ Response r = createGraphInRocksDB(TEMP_SPACE, "get_read_mode_test");
+ assertResponseStatus(201, r);
+
+ Response readModeResponse = client().get(PATH +
"/get_read_mode_test/graph_read_mode");
+ String content = assertResponseStatus(200, readModeResponse);
+
+ Map<String, Object> result = JsonUtil.fromJson(content, Map.class);
+ Assert.assertTrue(result.containsKey("graph_read_mode"));
+
+ // Clean up
+ Map<String, Object> params = ImmutableMap.of(
+ "confirm_message", "I'm sure to drop the graph");
+ client().delete(PATH + "/get_read_mode_test", params);
+ }
+
+ @Test
+ public void testReloadGraphsWithInvalidAction() {
+ String actionJson = "{\n" +
+ " \"action\": \"invalid_action\"\n" +
+ "}";
+
+ Response r = client().target(baseUrl())
+ .path(PATH + "/manage")
+ .request()
+ .put(Entity.json(actionJson));
+
+ Assert.assertTrue(r.getStatus() >= 400);
+ }
+
+ @Test
+ public void testGraphNotExist() {
+ Response r = client().get(PATH + "/non_existent_graph");
+ Assert.assertTrue(r.getStatus() >= 400);
}
}
Review Comment:
🧹 **Minor: 测试用例缺少边界条件验证**
建议添加更多边界条件测试：
- 创建同名图的冲突测试
- 使用无效字符的 nickname 测试
- 非常长的 nickname 测试
- 并发创建/删除图的测试
这些测试能更好地验证 GraphsAPI 的健壮性。
--
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]
