imbajin commented on code in PR #685:
URL:
https://github.com/apache/incubator-hugegraph-toolchain/pull/685#discussion_r2458831204
##########
hugegraph-client/src/main/java/org/apache/hugegraph/api/graphs/GraphsAPI.java:
##########
@@ -203,6 +203,28 @@ public void mode(String graph, GraphMode mode) {
this.client.put(joinPath(this.path(), graph, MODE), null, mode);
}
+ public void mode(String graph, String graphSpace, GraphMode mode) {
Review Comment:
‼️ **Critical - 方法重载存在歧义**
这两个重载方法的参数顺序不一致，可能导致调用时的混淆：
- `mode(String graph, String graphSpace, GraphMode mode)` - graph在前
- `mode(String graph, GraphMode mode)` - 旧方法
建议统一参数顺序，将graphSpace作为第一个参数（因为它是更高层级的概念），或者考虑使用Builder模式来避免重载歧义：
```java
// 推荐的参数顺序
public void mode(String graphSpace, String graph, GraphMode mode) {
this.client.put(joinPath(this.path(), graphSpace, graph, MODE), null,
mode);
}
```
这样可以清晰地体现层级关系：graphspace -> graph -> mode
##########
hugegraph-client/src/main/java/org/apache/hugegraph/api/graphs/GraphsAPI.java:
##########
@@ -203,6 +203,28 @@ public void mode(String graph, GraphMode mode) {
this.client.put(joinPath(this.path(), graph, MODE), null, mode);
}
+ public void mode(String graph, String graphSpace, GraphMode mode) {
+ // NOTE: Must provide id for PUT. If you use "graph/mode", "/" will
+ // be encoded to "%2F". So use "mode" here, although inaccurate.
+ this.client.put(joinPath(this.path(), graphSpace, graph, MODE), null,
mode);
+ }
+
+ public GraphMode mode(String graph, String graphSpace) {
+ RestResult result = this.client.get(joinPath(this.path(), graphSpace,
graph), MODE);
+ @SuppressWarnings("unchecked")
+ Map<String, String> mode = result.readObject(Map.class);
+ String value = mode.get(MODE);
+ if (value == null) {
+ throw new InvalidResponseException("Invalid response, expect
'mode' in response");
+ }
+ try {
+ return GraphMode.valueOf(value);
+ } catch (IllegalArgumentException e) {
+ throw new InvalidResponseException("Invalid GraphMode value '%s'",
value);
+ }
+ }
+
Review Comment:
⚠️ **代码风格 - 多余的空行**
这里有两个连续的空行，应该删除一个以保持代码风格一致。
##########
hugegraph-client/src/main/java/org/apache/hugegraph/api/graphs/GraphsAPI.java:
##########
@@ -203,6 +203,28 @@ public void mode(String graph, GraphMode mode) {
this.client.put(joinPath(this.path(), graph, MODE), null, mode);
}
+ public void mode(String graph, String graphSpace, GraphMode mode) {
Review Comment:
‼️ **Critical - 缺少单元测试**
这个PR添加了多个新的API方法（6个重载方法），但没有看到相应的测试。建议：
1. 为所有新增的graphSpace相关方法添加单元测试
2. 测试应该覆盖：
- 正常的mode/readMode获取和设置
- 无效的GraphMode/GraphReadMode值处理
- 空graphSpace参数的边界情况
- API版本兼容性检查
示例测试场景：
```java
@Test
public void testModeWithGraphSpace() {
GraphsAPI api = new GraphsAPI(client);
api.mode("testGraph", "testSpace", GraphMode.RESTORING);
GraphMode mode = api.mode("testGraph", "testSpace");
assertEquals(GraphMode.RESTORING, mode);
}
```
##########
hugegraph-client/src/main/java/org/apache/hugegraph/api/graphs/GraphsAPI.java:
##########
@@ -203,6 +203,28 @@ public void mode(String graph, GraphMode mode) {
this.client.put(joinPath(this.path(), graph, MODE), null, mode);
}
+ public void mode(String graph, String graphSpace, GraphMode mode) {
+ // NOTE: Must provide id for PUT. If you use "graph/mode", "/" will
+ // be encoded to "%2F". So use "mode" here, although inaccurate.
+ this.client.put(joinPath(this.path(), graphSpace, graph, MODE), null,
mode);
+ }
+
+ public GraphMode mode(String graph, String graphSpace) {
Review Comment:
⚠️ **代码重复 - 考虑重构**
`mode`和`readMode`方法中存在大量重复的逻辑模式。建议提取通用方法来减少代码重复：
```java
private <T extends Enum<T>> T getEnumMode(String graph, String graphSpace,
String modeKey, Class<T>
enumClass) {
String path = graphSpace != null ?
joinPath(this.path(), graphSpace, graph) :
joinPath(this.path(), graph);
RestResult result = this.client.get(path, modeKey);
Map<String, String> modeMap = result.readObject(Map.class);
String value = modeMap.get(modeKey);
if (value == null) {
throw new InvalidResponseException(
"Invalid response, expect '%s' in response", modeKey);
}
try {
return Enum.valueOf(enumClass, value);
} catch (IllegalArgumentException e) {
throw new InvalidResponseException(
"Invalid %s value '%s'", enumClass.getSimpleName(), value);
}
}
```
这样可以显著减少重复代码并提高可维护性。
##########
hugegraph-client/src/main/java/org/apache/hugegraph/client/RestClient.java:
##########
@@ -49,7 +49,7 @@ public class RestClient extends AbstractRestClient {
private Version apiVersion = null;
@Setter
@Getter
- private boolean supportGs = false;
+ private boolean supportGs = true;
Review Comment:
‼️ **Critical - 默认值改变可能导致破坏性变更**
将`supportGs`的默认值从`false`改为`true`是一个**破坏性变更**，可能影响现有的1.6.x或更早版本的客户端。
建议：
1. 保持默认值为`false`，通过版本检测动态设置（这已经在`checkServerApiVersion`中实现）
2. 或者在PR描述中明确说明这是breaking change，并更新文档
3. 考虑添加兼容性测试确保不会影响旧版本服务器
当前的实现逻辑：
- 构造函数默认设为`true`
- `checkServerApiVersion`会根据服务器版本重新设置
这会导致在`checkServerApiVersion`调用前的短暂时间窗口内，客户端认为支持graphspace但服务器可能不支持。
##########
hugegraph-client/src/main/java/org/apache/hugegraph/driver/HugeClient.java:
##########
@@ -173,7 +173,7 @@ private void checkServerApiVersion() {
// 0.81 equals to the {latest_api_version} +10
VersionUtil.check(apiVersion, "0.38", "0.81", "hugegraph-api in
server");
this.client.apiVersion(apiVersion);
- boolean supportGs = VersionUtil.gte(this.version.getCoreVersion(),
"2.0");
+ boolean supportGs = VersionUtil.gte(this.version.getCoreVersion(),
"1.7");
Review Comment:
⚠️ **版本比较逻辑一致性**
版本检查从`2.0`改为`1.7`，但建议：
1. 使用精确版本号`1.7.0`而不是`1.7`，以避免歧义
2. 在注释中说明为什么1.7版本开始支持graphspace
3. 考虑添加单元测试验证版本比较逻辑：
- 1.6.x版本应该返回false
- 1.7.0及以上返回true
```java
// 建议改为
boolean supportGs = VersionUtil.gte(this.version.getCoreVersion(), "1.7.0");
// Since version 1.7.0, server supports graphspace feature
```
##########
hugegraph-client/src/main/java/org/apache/hugegraph/api/graphs/GraphsAPI.java:
##########
@@ -203,6 +203,28 @@ public void mode(String graph, GraphMode mode) {
this.client.put(joinPath(this.path(), graph, MODE), null, mode);
}
+ public void mode(String graph, String graphSpace, GraphMode mode) {
Review Comment:
⚠️ **API一致性 - 缺少参数校验**
新增的graphSpace参数方法缺少参数校验。建议添加：
```java
public void mode(String graph, String graphSpace, GraphMode mode) {
E.checkArgumentNotNull(graph, "Graph name cannot be null");
E.checkArgumentNotNull(graphSpace, "GraphSpace name cannot be null");
E.checkArgumentNotNull(mode, "GraphMode cannot be null");
this.client.put(joinPath(this.path(), graphSpace, graph, MODE), null,
mode);
}
```
这样可以提供更清晰的错误信息，而不是让空指针在URL拼接时才失败。
##########
hugegraph-client/src/main/java/org/apache/hugegraph/api/graphs/GraphsAPI.java:
##########
@@ -203,6 +203,28 @@ public void mode(String graph, GraphMode mode) {
this.client.put(joinPath(this.path(), graph, MODE), null, mode);
}
+ public void mode(String graph, String graphSpace, GraphMode mode) {
Review Comment:
🧹 **文档完善**
虽然PR标记为`Doc - No Need`，但建议添加：
1. **JavaDoc注释**：新增方法应该有完整的JavaDoc，说明：
- `@param graphSpace`参数的含义和用途
- 与无graphSpace版本的方法区别
- `@since 1.7.0`标记新API的引入版本
2. **迁移指南**：在CHANGELOG或升级文档中说明：
- 如何从旧API迁移到新API
- graphspace参数何时需要传入
示例：
```java
/**
* Set the mode for specified graph in graphspace.
*
* @param graph the graph name
* @param graphSpace the graphspace name (required for server >= 1.7.0)
* @param mode the graph mode to set
* @since 1.7.0
*/
public void mode(String graph, String graphSpace, GraphMode mode) {
// ...
}
```
--
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]
