imbajin commented on code in PR #2945:
URL: https://github.com/apache/hugegraph/pull/2945#discussion_r2909532571
##########
hugegraph-store/hg-store-node/src/main/java/org/apache/hugegraph/store/node/controller/PartitionAPI.java:
##########
@@ -215,14 +231,28 @@ public Map<String, Object> compact(@RequestParam(value =
"id") int id) {
map.put("msg",
"compaction task fail to submit, and there could be
another task in progress");
}
- return map;
+ return ok("body", map);
Review Comment:
‼️ **Critical: This wraps `/compat` in a new `body` envelope**
Previously the response was the compaction result itself: `{ "code": ...,
"msg": ... }`. After this change it becomes `{ "status": 200, "body": { "code":
..., "msg": ... } }`, which silently breaks any caller reading `code`/`msg`
from the top level.
If we only need an HTTP 200 transport status here, we can keep the original
JSON contract:
```suggestion
return ResponseEntity.ok(map);
```
##########
hugegraph-store/hg-store-node/src/main/java/org/apache/hugegraph/store/node/controller/PartitionAPI.java:
##########
@@ -138,16 +142,15 @@ public Raft getPartition(@PathVariable(value = "id") int
id) {
raft.getPartitions().add(partition);
}
- return raft;
- //return okMap("partition", rafts);
+ return ok("raft", raft);
Review Comment:
‼️ **Critical: `/v1/partition/{id}` response schema changed unexpectedly**
Before this PR, this endpoint serialized the `Raft` object directly, so
callers could read fields like `groupId`, `leader`, and `partitions` from the
top level. Wrapping it as `{ "status": ..., "raft": ... }` is a wire-format
change unrelated to the Arthas hardening, and it will break existing
scripts/tools that already consume the old shape.
If the goal here is only to return a proper HTTP status for `/arthasstart`,
I'd keep the existing payload for `/partition/{id}` and avoid broad
response-shape changes in the same patch.
##########
hugegraph-store/hg-store-node/src/main/java/org/apache/hugegraph/store/node/controller/PartitionAPI.java:
##########
@@ -215,14 +231,28 @@ public Map<String, Object> compact(@RequestParam(value =
"id") int id) {
map.put("msg",
"compaction task fail to submit, and there could be
another task in progress");
}
- return map;
+ return ok("body", map);
}
- public Map<String, Object> okMap(String k, Object v) {
- Map<String, Object> map = new HashMap<>();
- map.put("status", 0);
- map.put(k, v);
- return map;
+ public ResponseEntity<Map<String, Object>> ok(String k, Object v) {
+ return ResponseEntity.ok(Map.of("status", 200, k, v));
Review Comment:
⚠️ **Important: Store APIs currently use `status = 0` for success, not
`200`**
`IndexAPI.okMap()` and the pre-change `PartitionAPI.okMap()` both expose
success as `status: 0`. Switching the JSON payload to `status: 200` changes the
existing API contract for every endpoint routed through `ok(...)`
(`/v1/partitions`, `/v1/partition/*`, `/v1/arthasstart`, etc.), even though the
HTTP status is already conveyed by `ResponseEntity`.
To keep the transport semantics improvement without breaking body-level
consumers, the helper should preserve the old success code:
```suggestion
return ResponseEntity.ok(Map.of("status", 0, k, v));
```
--
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]
