Copilot commented on code in PR #712:
URL:
https://github.com/apache/incubator-hugegraph-toolchain/pull/712#discussion_r2803996699
##########
hugegraph-client/src/main/java/org/apache/hugegraph/api/space/GraphSpaceAPI.java:
##########
@@ -71,15 +70,16 @@ public List<String> list() {
}
public List<Map<String, Object>> listProfile(String prefix) {
- String profilePath = joinPath(this.path(), "profile");
- Map<String, Object> params = new LinkedHashMap<>();
- params.put("prefix", prefix);
- RestResult result = this.client.get(profilePath, params);
- List<Map> results = result.readList(Map.class);
+ List<String> names = this.list();
List<Map<String, Object>> profiles = new ArrayList<>();
- for (Object entry : results) {
- profiles.add(JsonUtil.fromJson(JsonUtil.toJson(entry), Map.class));
+ for (String name : names) {
+ if (name.startsWith(prefix)) {
Review Comment:
The prefix parameter can be null (as seen in GraphSpaceManager.listProfile()
which calls this method with null), but this code calls name.startsWith(prefix)
without checking if prefix is null first. This will cause a
NullPointerException. Consider using StringUtils.isEmpty(prefix) to handle
null/empty prefix cases - when prefix is null or empty, all names should be
included in the result.
```suggestion
if (StringUtils.isEmpty(prefix) || name.startsWith(prefix)) {
```
##########
hugegraph-client/src/main/java/org/apache/hugegraph/api/space/GraphSpaceAPI.java:
##########
@@ -71,15 +70,16 @@ public List<String> list() {
}
public List<Map<String, Object>> listProfile(String prefix) {
- String profilePath = joinPath(this.path(), "profile");
- Map<String, Object> params = new LinkedHashMap<>();
- params.put("prefix", prefix);
- RestResult result = this.client.get(profilePath, params);
- List<Map> results = result.readList(Map.class);
+ List<String> names = this.list();
List<Map<String, Object>> profiles = new ArrayList<>();
- for (Object entry : results) {
- profiles.add(JsonUtil.fromJson(JsonUtil.toJson(entry), Map.class));
+ for (String name : names) {
+ if (name.startsWith(prefix)) {
+ GraphSpace space = this.get(name);
+ Map<String, Object> profileMap =
JsonUtil.fromJson(JsonUtil.toJson(space), Map.class);
+ profiles.add(profileMap);
+ }
}
+
return profiles;
Review Comment:
The new implementation performs N+1 API calls (one list() call plus one
get() call for each matching graph space), which could impact performance when
there are many graph spaces. While this is necessary due to the removal of the
/profile endpoint in server 1.7.0, consider documenting this performance
characteristic in a code comment to inform future developers, especially if
this method is called frequently or with many graph spaces.
--
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]
