Copilot commented on code in PR #853:
URL:
https://github.com/apache/rocketmq-dashboard/pull/853#discussion_r3703914539
##########
server/src/main/java/org/apache/rocketmq/studio/cluster/k8s/K8sCertController.java:
##########
@@ -40,23 +41,33 @@ public Result<List<K8sCertVO>> listCerts() {
}
@PostMapping("/create")
- public Result<K8sCertVO> createCert(@Valid @RequestBody CreateCertDTO
command) {
+ public Result<K8sCertVO> createCert(@Valid @RequestBody(required = false)
CreateCertDTO command) {
+ requireCommand(command);
return Result.ok(k8sCertService.createCert(command));
}
@PostMapping("/update")
- public Result<K8sCertVO> updateCert(@Valid @RequestBody UpdateCertDTO
command) {
+ public Result<K8sCertVO> updateCert(@Valid @RequestBody(required = false)
UpdateCertDTO command) {
+ requireCommand(command);
return Result.ok(k8sCertService.updateCert(command));
}
@PostMapping("/renew")
- public Result<K8sCertVO> renewCert(@Valid @RequestBody RenewCertDTO
command) {
+ public Result<K8sCertVO> renewCert(@Valid @RequestBody(required = false)
RenewCertDTO command) {
Review Comment:
Setting `@RequestBody(required = false)` makes the request body optional for
these endpoints (observable API contract change), not just “reject JSON null”.
If the intent is only to reject a JSON `null` payload, consider keeping the
default `required = true` and relying on the explicit `requireCommand(command)`
null-check (Jackson will still deserialize a literal `null` body to `null`).
Keep `required=false` only if you explicitly want to treat missing/empty bodies
as your custom BusinessException, and document/test that behavior.
##########
server/src/main/java/org/apache/rocketmq/studio/cluster/k8s/K8sCertController.java:
##########
@@ -40,23 +41,33 @@ public Result<List<K8sCertVO>> listCerts() {
}
@PostMapping("/create")
- public Result<K8sCertVO> createCert(@Valid @RequestBody CreateCertDTO
command) {
+ public Result<K8sCertVO> createCert(@Valid @RequestBody(required = false)
CreateCertDTO command) {
+ requireCommand(command);
return Result.ok(k8sCertService.createCert(command));
}
@PostMapping("/update")
- public Result<K8sCertVO> updateCert(@Valid @RequestBody UpdateCertDTO
command) {
+ public Result<K8sCertVO> updateCert(@Valid @RequestBody(required = false)
UpdateCertDTO command) {
Review Comment:
Setting `@RequestBody(required = false)` makes the request body optional for
these endpoints (observable API contract change), not just “reject JSON null”.
If the intent is only to reject a JSON `null` payload, consider keeping the
default `required = true` and relying on the explicit `requireCommand(command)`
null-check (Jackson will still deserialize a literal `null` body to `null`).
Keep `required=false` only if you explicitly want to treat missing/empty bodies
as your custom BusinessException, and document/test that behavior.
##########
server/src/main/java/org/apache/rocketmq/studio/cluster/k8s/K8sCertController.java:
##########
@@ -40,23 +41,33 @@ public Result<List<K8sCertVO>> listCerts() {
}
@PostMapping("/create")
- public Result<K8sCertVO> createCert(@Valid @RequestBody CreateCertDTO
command) {
+ public Result<K8sCertVO> createCert(@Valid @RequestBody(required = false)
CreateCertDTO command) {
Review Comment:
Setting `@RequestBody(required = false)` makes the request body optional for
these endpoints (observable API contract change), not just “reject JSON null”.
If the intent is only to reject a JSON `null` payload, consider keeping the
default `required = true` and relying on the explicit `requireCommand(command)`
null-check (Jackson will still deserialize a literal `null` body to `null`).
Keep `required=false` only if you explicitly want to treat missing/empty bodies
as your custom BusinessException, and document/test that behavior.
##########
server/src/main/java/org/apache/rocketmq/studio/cluster/k8s/K8sCertController.java:
##########
@@ -40,23 +41,33 @@ public Result<List<K8sCertVO>> listCerts() {
}
@PostMapping("/create")
- public Result<K8sCertVO> createCert(@Valid @RequestBody CreateCertDTO
command) {
+ public Result<K8sCertVO> createCert(@Valid @RequestBody(required = false)
CreateCertDTO command) {
+ requireCommand(command);
return Result.ok(k8sCertService.createCert(command));
}
@PostMapping("/update")
- public Result<K8sCertVO> updateCert(@Valid @RequestBody UpdateCertDTO
command) {
+ public Result<K8sCertVO> updateCert(@Valid @RequestBody(required = false)
UpdateCertDTO command) {
+ requireCommand(command);
return Result.ok(k8sCertService.updateCert(command));
}
@PostMapping("/renew")
- public Result<K8sCertVO> renewCert(@Valid @RequestBody RenewCertDTO
command) {
+ public Result<K8sCertVO> renewCert(@Valid @RequestBody(required = false)
RenewCertDTO command) {
+ requireCommand(command);
return Result.ok(k8sCertService.renewCert(command));
}
@PostMapping("/delete")
- public Result<Void> deleteCert(@Valid @RequestBody DeleteCertDTO command) {
+ public Result<Void> deleteCert(@Valid @RequestBody(required = false)
DeleteCertDTO command) {
Review Comment:
Setting `@RequestBody(required = false)` makes the request body optional for
these endpoints (observable API contract change), not just “reject JSON null”.
If the intent is only to reject a JSON `null` payload, consider keeping the
default `required = true` and relying on the explicit `requireCommand(command)`
null-check (Jackson will still deserialize a literal `null` body to `null`).
Keep `required=false` only if you explicitly want to treat missing/empty bodies
as your custom BusinessException, and document/test that behavior.
##########
server/src/main/java/org/apache/rocketmq/studio/cluster/k8s/K8sCertController.java:
##########
@@ -40,23 +41,33 @@ public Result<List<K8sCertVO>> listCerts() {
}
@PostMapping("/create")
- public Result<K8sCertVO> createCert(@Valid @RequestBody CreateCertDTO
command) {
+ public Result<K8sCertVO> createCert(@Valid @RequestBody(required = false)
CreateCertDTO command) {
+ requireCommand(command);
return Result.ok(k8sCertService.createCert(command));
}
@PostMapping("/update")
- public Result<K8sCertVO> updateCert(@Valid @RequestBody UpdateCertDTO
command) {
+ public Result<K8sCertVO> updateCert(@Valid @RequestBody(required = false)
UpdateCertDTO command) {
+ requireCommand(command);
return Result.ok(k8sCertService.updateCert(command));
}
@PostMapping("/renew")
- public Result<K8sCertVO> renewCert(@Valid @RequestBody RenewCertDTO
command) {
+ public Result<K8sCertVO> renewCert(@Valid @RequestBody(required = false)
RenewCertDTO command) {
+ requireCommand(command);
return Result.ok(k8sCertService.renewCert(command));
}
@PostMapping("/delete")
- public Result<Void> deleteCert(@Valid @RequestBody DeleteCertDTO command) {
+ public Result<Void> deleteCert(@Valid @RequestBody(required = false)
DeleteCertDTO command) {
+ requireCommand(command);
k8sCertService.deleteCert(command);
return Result.ok();
}
+
+ private void requireCommand(Object command) {
+ if (command == null) {
+ throw new BusinessException(400, "K8s certificate request is
required");
+ }
+ }
Review Comment:
This duplicates the same null-guard logic and message that now also exists
in `K8sCertService`. To avoid divergence (e.g., message/code changes in one
place but not the other), prefer extracting a shared constant for the message
(and possibly the code), or centralizing the guard in one reusable
utility/helper so both controller and service use the same source of truth.
##########
server/src/test/java/org/apache/rocketmq/studio/cluster/k8s/K8sCertControllerTest.java:
##########
@@ -139,6 +139,27 @@ void createCertShouldAcceptMinimalCommand() throws
Exception {
.andExpect(jsonPath("$.data.id").value("cert-min"));
}
+ @Test
+ void certWriteEndpointsShouldRejectNullRequestBody() throws Exception {
+ String[] paths = {
+ "/api/k8s-certs/create",
+ "/api/k8s-certs/update",
+ "/api/k8s-certs/renew",
+ "/api/k8s-certs/delete"
+ };
+
+ for (String path : paths) {
+ mockMvc.perform(post(path)
+ .contentType(MediaType.APPLICATION_JSON)
+ .content("null"))
Review Comment:
With `@RequestBody(required = false)` in the controller, an empty/missing
body can also be bound as `null` and will now take the same path as a literal
JSON `null`. Add a test case that sends an empty body (e.g., no `.content(...)`
or an empty string) and asserts the intended response/message, so the behavior
for “missing body vs JSON null” remains explicit and stable.
--
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]
