Copilot commented on code in PR #857:
URL:
https://github.com/apache/rocketmq-dashboard/pull/857#discussion_r3703955558
##########
server/src/main/java/org/apache/rocketmq/studio/ops/alert/SystemAlertController.java:
##########
@@ -43,7 +44,9 @@ public Result<List<SystemAlertVO>> listAlerts(
}
@PostMapping("/acknowledge")
- public Result<SystemAlertVO> acknowledgeAlert(@Valid @RequestBody
AcknowledgeSystemAlertDTO request) {
+ public Result<SystemAlertVO> acknowledgeAlert(
+ @Valid @RequestBody(required = false) AcknowledgeSystemAlertDTO
request) {
+ requireAcknowledgeRequest(request);
Review Comment:
Since `required = false` changes how missing/empty request bodies are
handled, add a controller test for an empty body (and/or no content) to lock in
the intended behavior and prevent regressions distinct from the JSON literal
`null` case.
##########
server/src/main/java/org/apache/rocketmq/studio/ops/alert/SystemAlertController.java:
##########
@@ -43,7 +44,9 @@ public Result<List<SystemAlertVO>> listAlerts(
}
@PostMapping("/acknowledge")
- public Result<SystemAlertVO> acknowledgeAlert(@Valid @RequestBody
AcknowledgeSystemAlertDTO request) {
+ public Result<SystemAlertVO> acknowledgeAlert(
+ @Valid @RequestBody(required = false) AcknowledgeSystemAlertDTO
request) {
+ requireAcknowledgeRequest(request);
return Result.ok(alertService.acknowledgeAlert(request.getId()));
}
Review Comment:
Changing `@RequestBody` to `required = false` alters behavior for requests
with a missing/empty body: they will now be accepted by argument resolution and
then fail via `BusinessException`, instead of failing in Spring’s request-body
handling. If the intent is to reject only the JSON literal `null` while
preserving the previous handling for missing bodies, consider keeping `required
= true` and adding a targeted check via request-body advice/custom
deserialization/exception translation for the `null` literal case.
##########
server/src/main/java/org/apache/rocketmq/studio/ops/alert/SystemAlertController.java:
##########
@@ -52,4 +55,10 @@ public Result<Map<String, Integer>> clearAcknowledged() {
int cleared = alertService.clearAcknowledged();
return Result.ok(Map.of("cleared", cleared));
}
+
+ private void requireAcknowledgeRequest(AcknowledgeSystemAlertDTO request) {
+ if (request == null) {
+ throw new BusinessException(400, "System alert acknowledge request
is required");
+ }
+ }
Review Comment:
Avoid the magic number `400` here. Prefer a shared constant (e.g.,
`HttpStatus.BAD_REQUEST.value()`) or the project’s existing error-code
enum/constant so the status/code mapping stays consistent and easier to
refactor.
--
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]
