Copilot commented on code in PR #849:
URL:
https://github.com/apache/rocketmq-dashboard/pull/849#discussion_r3703860068
##########
server/src/main/java/org/apache/rocketmq/studio/instance/InstanceController.java:
##########
@@ -59,4 +62,10 @@ public Result<Void> deleteInstance(@Valid @RequestBody
InstanceDeleteRequestDTO
instanceService.deleteInstance(request.getId());
return Result.ok();
}
+
+ private void requireInstance(InstanceVO instance) {
+ if (instance == null) {
+ throw new BusinessException(400, "Instance request is required");
+ }
+ }
Review Comment:
The error code `400` and message `"Instance request is required"` are
hard-coded here (and duplicated in `InstanceService` and tests). To avoid drift
and brittle tests, centralize these as shared constants (e.g.,
`INSTANCE_REQUEST_REQUIRED_MSG` and `BAD_REQUEST_CODE`) or a small shared
helper/factory used by both controller and service.
##########
server/src/main/java/org/apache/rocketmq/studio/instance/InstanceController.java:
##########
@@ -45,12 +46,14 @@ public Result<List<InstanceVO>> listInstances(
}
@PostMapping("/create")
- public Result<InstanceVO> createInstance(@RequestBody InstanceVO instance)
{
+ public Result<InstanceVO> createInstance(@RequestBody(required = false)
InstanceVO instance) {
+ requireInstance(instance);
return Result.ok(instanceService.createInstance(instance));
}
@PostMapping("/update")
- public Result<InstanceVO> updateInstance(@RequestBody InstanceVO instance)
{
+ public Result<InstanceVO> updateInstance(@RequestBody(required = false)
InstanceVO instance) {
+ requireInstance(instance);
return Result.ok(instanceService.updateInstance(instance));
}
Review Comment:
The error code `400` and message `"Instance request is required"` are
hard-coded here (and duplicated in `InstanceService` and tests). To avoid drift
and brittle tests, centralize these as shared constants (e.g.,
`INSTANCE_REQUEST_REQUIRED_MSG` and `BAD_REQUEST_CODE`) or a small shared
helper/factory used by both controller and service.
##########
server/src/main/java/org/apache/rocketmq/studio/instance/InstanceService.java:
##########
@@ -65,6 +66,7 @@ public InstanceVO createInstance(InstanceVO instance) {
}
public InstanceVO updateInstance(InstanceVO instance) {
+ requireInstance(instance);
Review Comment:
`requireInstance(...)` throws the same hard-coded `BusinessException(400,
"Instance request is required")` as the controller. Consider reusing the same
shared constants/helper as the controller so the service/controller/tests stay
consistent if the error code/message changes.
##########
server/src/main/java/org/apache/rocketmq/studio/instance/InstanceService.java:
##########
@@ -111,6 +113,12 @@ public void deleteInstance(String id) {
instanceRepository.deleteById(id);
}
+ private void requireInstance(InstanceVO instance) {
+ if (instance == null) {
+ throw new BusinessException(400, "Instance request is required");
+ }
+ }
Review Comment:
`requireInstance(...)` throws the same hard-coded `BusinessException(400,
"Instance request is required")` as the controller. Consider reusing the same
shared constants/helper as the controller so the service/controller/tests stay
consistent if the error code/message changes.
##########
server/src/main/java/org/apache/rocketmq/studio/instance/InstanceController.java:
##########
@@ -45,12 +46,14 @@ public Result<List<InstanceVO>> listInstances(
}
@PostMapping("/create")
- public Result<InstanceVO> createInstance(@RequestBody InstanceVO instance)
{
+ public Result<InstanceVO> createInstance(@RequestBody(required = false)
InstanceVO instance) {
+ requireInstance(instance);
return Result.ok(instanceService.createInstance(instance));
}
@PostMapping("/update")
- public Result<InstanceVO> updateInstance(@RequestBody InstanceVO instance)
{
+ public Result<InstanceVO> updateInstance(@RequestBody(required = false)
InstanceVO instance) {
Review Comment:
If `InstanceVO` has bean validation annotations, they won’t run unless the
parameter is annotated with `@Valid`. Consider changing the signatures to
`createInstance(@Valid @RequestBody(required = false) InstanceVO instance)` /
`updateInstance(@Valid @RequestBody(required = false) InstanceVO instance)` so
non-null requests still get validated at the controller boundary.
##########
server/src/main/java/org/apache/rocketmq/studio/instance/InstanceController.java:
##########
@@ -45,12 +46,14 @@ public Result<List<InstanceVO>> listInstances(
}
@PostMapping("/create")
- public Result<InstanceVO> createInstance(@RequestBody InstanceVO instance)
{
+ public Result<InstanceVO> createInstance(@RequestBody(required = false)
InstanceVO instance) {
Review Comment:
If `InstanceVO` has bean validation annotations, they won’t run unless the
parameter is annotated with `@Valid`. Consider changing the signatures to
`createInstance(@Valid @RequestBody(required = false) InstanceVO instance)` /
`updateInstance(@Valid @RequestBody(required = false) InstanceVO instance)` so
non-null requests still get validated at the controller boundary.
##########
server/src/main/java/org/apache/rocketmq/studio/instance/InstanceService.java:
##########
@@ -49,6 +49,7 @@ public List<InstanceVO> listInstances(InstanceType type,
String search) {
}
public InstanceVO createInstance(InstanceVO instance) {
+ requireInstance(instance);
Review Comment:
`requireInstance(...)` throws the same hard-coded `BusinessException(400,
"Instance request is required")` as the controller. Consider reusing the same
shared constants/helper as the controller so the service/controller/tests stay
consistent if the error code/message changes.
--
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]
