Copilot commented on code in PR #2950:
URL:
https://github.com/apache/incubator-hugegraph/pull/2950#discussion_r2805155224
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/space/GraphSpaceAPI.java:
##########
@@ -93,6 +98,55 @@ public Object get(@Context GraphManager manager,
return gsInfo;
}
+ @GET
+ @Timed
+ @Path("profile")
+ @Produces(APPLICATION_JSON_WITH_CHARSET)
+ @RolesAllowed({"admin"})
+ public Object listProfile(@Context GraphManager manager,
+ @QueryParam("prefix") String prefix,
+ @Context SecurityContext sc) {
+ Set<String> spaces = manager.graphSpaces();
+ List<Map<String, Object>> spaceList = new ArrayList<>();
+ List<Map<String, Object>> result = new ArrayList<>();
+ String user = HugeGraphAuthProxy.username();
+ AuthManager authManager = manager.authManager();
+ // FIXME: defaultSpace related interface is not implemented
+ // String defaultSpace = authManager.getDefaultSpace(user);
+ SimpleDateFormat format = new SimpleDateFormat("yyyy-MM-dd hh:mm:ss");
+ for (String sp : spaces) {
+ manager.getSpaceStorage(sp);
+ GraphSpace gs = space(manager, sp);
+ Map<String, Object> gsProfile = gs.info();
+ boolean isManager = verifyPermission(user, authManager, sp);
+
+ // 设置当前用户的是否允许访问该空间
+ if (gs.auth() && !isManager) {
+ gsProfile.put("authed", false);
+ } else {
+ gsProfile.put("authed", true);
+ }
+
+ gsProfile.put("create_time", format.format(gs.createTime()));
+ gsProfile.put("update_time", format.format(gs.updateTime()));
+ if (!isPrefix(gsProfile, prefix)) {
+ continue;
+ }
+
+ gsProfile.put("default", false);
+ result.add(gsProfile);
+ //boolean defaulted = StringUtils.equals(sp, defaultSpace);
+ //gsProfile.put("default", defaulted);
+ //if (defaulted) {
+ // result.add(gsProfile);
+ //} else {
+ // spaceList.add(gsProfile);
+ //}
+ }
+ result.addAll(spaceList);
+ return result;
Review Comment:
`listProfile()` builds `spaceList` but never adds anything to it (the only
code path that would add is commented out), and then does
`result.addAll(spaceList)` which is a no-op. This looks like
leftover/unfinished logic; consider removing `spaceList` entirely (and the
final `addAll`) or re-enabling the intended default-space ordering logic.
##########
hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/api/GraphSpaceApiTest.java:
##########
@@ -275,4 +277,106 @@ public void testInvalidSpaceCreation() {
r = this.client().post(PATH, negativeLimitsBody);
assertResponseStatus(400, r);
}
+
+ @Test
+ public void testListProfile() {
+ // Get profile list without prefix
+ Response r = this.client().get(PATH + "/profile");
+ String result = assertResponseStatus(200, r);
+
+ List<Map<String, Object>> profiles = JsonUtil.fromJson(result,
List.class);
+
+ // Should contain at least the DEFAULT space
+ assert profiles.size() >= 1;
+
+ // Verify profile structure
+ for (Map<String, Object> profile : profiles) {
+ assert profile.containsKey("name");
+ assert profile.containsKey("authed");
+ assert profile.containsKey("create_time");
+ assert profile.containsKey("update_time");
+ assert profile.containsKey("default");
+ }
Review Comment:
These tests rely on Java `assert` statements for verification. Maven
Surefire isn’t configured with `-ea` in this module, so these assertions may be
skipped at runtime and the tests would still pass even if the endpoint is
broken. Prefer JUnit assertions (e.g., `Assert.assertTrue/False`,
`assertEquals`, etc.) for test validation.
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/space/GraphSpaceAPI.java:
##########
@@ -93,6 +98,55 @@ public Object get(@Context GraphManager manager,
return gsInfo;
}
+ @GET
+ @Timed
+ @Path("profile")
+ @Produces(APPLICATION_JSON_WITH_CHARSET)
+ @RolesAllowed({"admin"})
+ public Object listProfile(@Context GraphManager manager,
+ @QueryParam("prefix") String prefix,
+ @Context SecurityContext sc) {
+ Set<String> spaces = manager.graphSpaces();
+ List<Map<String, Object>> spaceList = new ArrayList<>();
+ List<Map<String, Object>> result = new ArrayList<>();
+ String user = HugeGraphAuthProxy.username();
+ AuthManager authManager = manager.authManager();
+ // FIXME: defaultSpace related interface is not implemented
+ // String defaultSpace = authManager.getDefaultSpace(user);
+ SimpleDateFormat format = new SimpleDateFormat("yyyy-MM-dd hh:mm:ss");
+ for (String sp : spaces) {
+ manager.getSpaceStorage(sp);
+ GraphSpace gs = space(manager, sp);
+ Map<String, Object> gsProfile = gs.info();
+ boolean isManager = verifyPermission(user, authManager, sp);
+
+ // 设置当前用户的是否允许访问该空间
+ if (gs.auth() && !isManager) {
+ gsProfile.put("authed", false);
+ } else {
+ gsProfile.put("authed", true);
+ }
+
+ gsProfile.put("create_time", format.format(gs.createTime()));
+ gsProfile.put("update_time", format.format(gs.updateTime()));
Review Comment:
The date format pattern uses `hh` (12-hour clock) without an AM/PM marker,
which can produce ambiguous timestamps. Also, elsewhere in the codebase (e.g.,
HugeGraphSONModule) dates are formatted with `yyyy-MM-dd HH:mm:ss.SSS`;
consider using a consistent 24-hour format (and ideally the same formatter) for
`create_time`/`update_time`, or avoid overriding the `Date` values from
`gs.info()` and let the configured JSON Date serializer handle formatting.
##########
hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/api/GraphSpaceApiTest.java:
##########
@@ -275,4 +277,106 @@ public void testInvalidSpaceCreation() {
r = this.client().post(PATH, negativeLimitsBody);
assertResponseStatus(400, r);
}
+
+ @Test
+ public void testListProfile() {
+ // Get profile list without prefix
+ Response r = this.client().get(PATH + "/profile");
+ String result = assertResponseStatus(200, r);
+
+ List<Map<String, Object>> profiles = JsonUtil.fromJson(result,
List.class);
+
+ // Should contain at least the DEFAULT space
+ assert profiles.size() >= 1;
+
+ // Verify profile structure
+ for (Map<String, Object> profile : profiles) {
+ assert profile.containsKey("name");
+ assert profile.containsKey("authed");
+ assert profile.containsKey("create_time");
+ assert profile.containsKey("update_time");
+ assert profile.containsKey("default");
+ }
+ }
+
+ @Test
+ public void testListProfileWithPrefix() {
+ // Create test spaces with different names
+ String space1 = "{"
+ + "\"name\":\"test_profile_space1\","
+ + "\"nickname\":\"TestProfileSpace\","
+ + "\"description\":\"Testprofilelisting\","
+ + "\"cpu_limit\":1000,"
+ + "\"memory_limit\":1024,"
+ + "\"storage_limit\":1000,"
+ + "\"compute_cpu_limit\":0,"
+ + "\"compute_memory_limit\":0,"
+ + "\"oltp_namespace\":null,"
+ + "\"olap_namespace\":null,"
+ + "\"storage_namespace\":null,"
+ + "\"operator_image_path\":\"test\","
+ + "\"internal_algorithm_image_url\":\"test\","
+ + "\"max_graph_number\":100,"
+ + "\"max_role_number\":100,"
+ + "\"auth\":false,"
+ + "\"configs\":{}"
+ + "}";
+
+ // Create spaces
+ Response r = this.client().post(PATH, space1);
+ assertResponseStatus(201, r);
+
+ // Test with prefix filter
+ r = this.client().get(PATH + "/profile",
+ ImmutableMap.of("prefix", "test"));
+ String result = assertResponseStatus(200, r);
+
+ List<Map<String, Object>> profiles = JsonUtil.fromJson(result,
List.class);
+ assert !profiles.isEmpty();
Review Comment:
`testListProfileWithPrefix()` only asserts the returned list is non-empty,
which doesn’t verify that the `prefix` filter is actually applied. To make the
test meaningful, assert that every returned profile has `name` (and/or
`nickname`) starting with the requested prefix, and ideally that profiles
without the prefix are excluded.
```suggestion
String space2 = "{"
+ "\"name\":\"other_profile_space\","
+ "\"nickname\":\"OtherProfileSpace\","
+ "\"description\":\"Other profile listing\","
+ "\"cpu_limit\":1000,"
+ "\"memory_limit\":1024,"
+ "\"storage_limit\":1000,"
+ "\"compute_cpu_limit\":0,"
+ "\"compute_memory_limit\":0,"
+ "\"oltp_namespace\":null,"
+ "\"olap_namespace\":null,"
+ "\"storage_namespace\":null,"
+ "\"operator_image_path\":\"test\","
+ "\"internal_algorithm_image_url\":\"test\","
+ "\"max_graph_number\":100,"
+ "\"max_role_number\":100,"
+ "\"auth\":false,"
+ "\"configs\":{}"
+ "}";
// Create spaces
Response r = this.client().post(PATH, space1);
assertResponseStatus(201, r);
r = this.client().post(PATH, space2);
assertResponseStatus(201, r);
// Test with prefix filter
r = this.client().get(PATH + "/profile",
ImmutableMap.of("prefix", "test"));
String result = assertResponseStatus(200, r);
@SuppressWarnings("unchecked")
List<Map<String, Object>> profiles = JsonUtil.fromJson(result,
List.class);
assert !profiles.isEmpty() : "Expected non-empty profile list with
prefix filter";
for (Map<String, Object> profile : profiles) {
String name = Objects.toString(profile.get("name"), "");
String nickname = Objects.toString(profile.get("nickname"), "");
boolean matchesPrefix = name.startsWith("test") ||
nickname.startsWith("test");
assert matchesPrefix
: "Profile does not match prefix filter 'test': " +
profile;
// Ensure the space without the 'test' prefix is excluded
assert !("other_profile_space".equals(name)
|| "OtherProfileSpace".equals(nickname))
: "Non-matching profile should be excluded by prefix
filter: " + profile;
}
```
##########
hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/api/GraphSpaceApiTest.java:
##########
@@ -275,4 +277,106 @@ public void testInvalidSpaceCreation() {
r = this.client().post(PATH, negativeLimitsBody);
assertResponseStatus(400, r);
}
+
+ @Test
+ public void testListProfile() {
+ // Get profile list without prefix
+ Response r = this.client().get(PATH + "/profile");
+ String result = assertResponseStatus(200, r);
+
+ List<Map<String, Object>> profiles = JsonUtil.fromJson(result,
List.class);
+
+ // Should contain at least the DEFAULT space
+ assert profiles.size() >= 1;
+
+ // Verify profile structure
+ for (Map<String, Object> profile : profiles) {
+ assert profile.containsKey("name");
+ assert profile.containsKey("authed");
+ assert profile.containsKey("create_time");
+ assert profile.containsKey("update_time");
+ assert profile.containsKey("default");
+ }
+ }
+
+ @Test
+ public void testListProfileWithPrefix() {
+ // Create test spaces with different names
+ String space1 = "{"
+ + "\"name\":\"test_profile_space1\","
+ + "\"nickname\":\"TestProfileSpace\","
+ + "\"description\":\"Testprofilelisting\","
+ + "\"cpu_limit\":1000,"
+ + "\"memory_limit\":1024,"
+ + "\"storage_limit\":1000,"
+ + "\"compute_cpu_limit\":0,"
+ + "\"compute_memory_limit\":0,"
+ + "\"oltp_namespace\":null,"
+ + "\"olap_namespace\":null,"
+ + "\"storage_namespace\":null,"
+ + "\"operator_image_path\":\"test\","
+ + "\"internal_algorithm_image_url\":\"test\","
+ + "\"max_graph_number\":100,"
+ + "\"max_role_number\":100,"
+ + "\"auth\":false,"
+ + "\"configs\":{}"
+ + "}";
+
+ // Create spaces
+ Response r = this.client().post(PATH, space1);
+ assertResponseStatus(201, r);
+
+ // Test with prefix filter
+ r = this.client().get(PATH + "/profile",
+ ImmutableMap.of("prefix", "test"));
+ String result = assertResponseStatus(200, r);
+
+ List<Map<String, Object>> profiles = JsonUtil.fromJson(result,
List.class);
+ assert !profiles.isEmpty();
+ }
+
+ @Test
+ public void testListProfileWithAuth() {
+ // Create a space with auth enabled
+ String authSpace = "{"
+ + "\"name\":\"auth_test_space\","
+ + "\"nickname\":\"AuthTestSpace\","
+ + "\"description\":\"Test auth in profile\","
+ + "\"cpu_limit\":1000,"
+ + "\"memory_limit\":1024,"
+ + "\"storage_limit\":1000,"
+ + "\"compute_cpu_limit\":0,"
+ + "\"compute_memory_limit\":0,"
+ + "\"oltp_namespace\":null,"
+ + "\"olap_namespace\":null,"
+ + "\"storage_namespace\":null,"
+ + "\"operator_image_path\":\"test\","
+ + "\"internal_algorithm_image_url\":\"test\","
+ + "\"max_graph_number\":100,"
+ + "\"max_role_number\":100,"
+ + "\"auth\":true,"
+ + "\"configs\":{}"
+ + "}";
+
+ Response r = this.client().post(PATH, authSpace);
+ assertResponseStatus(201, r);
+
+ // Get profile list
+ r = this.client().get(PATH + "/profile");
+ String result = assertResponseStatus(200, r);
+
+ List<Map<String, Object>> profiles = JsonUtil.fromJson(result,
List.class);
+
+ // Find the auth_test_space and verify authed field
+ boolean found = false;
+ for (Map<String, Object> profile : profiles) {
+ if ("auth_test_space".equals(profile.get("name"))) {
+ found = true;
+ // Admin user should be authed
+ assert profile.containsKey("authed");
Review Comment:
`testListProfileWithAuth()` only checks that the `authed` key exists for
`auth_test_space`, but it doesn’t validate the boolean value. Since the purpose
of this test is auth-related behavior, assert that `authed` is `true` for the
admin user (and consider adding a negative case if non-admin access is expected
to show `false`/be forbidden).
```suggestion
assert profile.containsKey("authed");
assert Objects.equals(profile.get("authed"), true);
```
--
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]
