Copilot commented on code in PR #859:
URL:
https://github.com/apache/rocketmq-dashboard/pull/859#discussion_r3704338307
##########
server/src/main/java/org/apache/rocketmq/studio/instance/topic/MetadataService.java:
##########
@@ -121,4 +125,16 @@ public List<NamespaceVO> listNamespaces() {
private String normalizeFilter(String value) {
return value == null || value.isBlank() ? null : value.trim();
}
+
+ private void requireTopic(TopicVO topic) {
+ if (topic == null) {
+ throw new BusinessException(400, "Topic request is required");
+ }
+ }
+
+ private void requireSendMessageRequest(SendMessageDTO request) {
+ if (request == null) {
+ throw new BusinessException(400, "Topic send message request is
required");
+ }
+ }
Review Comment:
As in `TopicController`, this introduces duplicated hard-coded
status/message literals. Centralizing these constants (and using a named
status/error-code constant instead of `400`) would make it easier to keep
controller/service behavior consistent over time.
##########
server/src/main/java/org/apache/rocketmq/studio/instance/topic/TopicController.java:
##########
@@ -71,7 +75,26 @@ public Result<List<TopicConsumerVO>>
getTopicConsumers(@PathVariable String name
}
@PostMapping("/send")
- public Result<SendMessageVO> sendMessage(@RequestBody SendMessageDTO
request) {
+ public Result<SendMessageVO> sendMessage(@RequestBody(required = false)
SendMessageDTO request) {
+ requireSendMessageRequest(request);
return Result.ok(metadataService.sendMessage(request));
}
+
+ private void requireTopicRequest(TopicVO topic) {
+ if (topic == null) {
+ throw new BusinessException(400, "Topic request is required");
+ }
+ }
+
+ private void requireDeleteTopicRequest(DeleteTopicDTO request) {
+ if (request == null) {
+ throw new BusinessException(400, "Topic delete request is
required");
+ }
+ }
+
+ private void requireSendMessageRequest(SendMessageDTO request) {
+ if (request == null) {
+ throw new BusinessException(400, "Topic send message request is
required");
+ }
+ }
Review Comment:
The status code and error messages are hard-coded literals here. To reduce
duplication and prevent drift across controller/service/tests, consider using a
shared constant/source for these messages and using a named status constant
(e.g., `HttpStatus.BAD_REQUEST.value()` or a project-wide error code constant)
instead of the magic number `400`.
##########
server/src/test/java/org/apache/rocketmq/studio/instance/topic/TopicControllerTest.java:
##########
@@ -108,6 +108,39 @@ void createTopicShouldReturnCreatedTopic() throws
Exception {
.andExpect(jsonPath("$.data.writeQueues").value(16));
}
+ @Test
+ void topicWriteEndpointsShouldRejectNullRequestBody() throws Exception {
+ mockMvc.perform(post("/api/topics/create")
+ .contentType(MediaType.APPLICATION_JSON)
+ .content("null"))
+ .andExpect(status().isBadRequest())
+ .andExpect(jsonPath("$.code").value(400))
+ .andExpect(jsonPath("$.message").value("Topic request is
required"));
+
+ mockMvc.perform(post("/api/topics/update")
+ .contentType(MediaType.APPLICATION_JSON)
+ .content("null"))
+ .andExpect(status().isBadRequest())
+ .andExpect(jsonPath("$.code").value(400))
+ .andExpect(jsonPath("$.message").value("Topic request is
required"));
+
+ mockMvc.perform(post("/api/topics/delete")
+ .contentType(MediaType.APPLICATION_JSON)
+ .content("null"))
+ .andExpect(status().isBadRequest())
+ .andExpect(jsonPath("$.code").value(400))
+ .andExpect(jsonPath("$.message").value("Topic delete request
is required"));
+
+ mockMvc.perform(post("/api/topics/send")
+ .contentType(MediaType.APPLICATION_JSON)
+ .content("null"))
+ .andExpect(status().isBadRequest())
+ .andExpect(jsonPath("$.code").value(400))
+ .andExpect(jsonPath("$.message").value("Topic send message
request is required"));
Review Comment:
These tests cover the JSON literal `null`, but not other common 'missing
body' forms (e.g., empty body with `Content-Type: application/json`, or
omitting the body entirely). Since the controller now uses
`@RequestBody(required = false)`, adding coverage for empty/missing request
bodies would better validate the intended controller-boundary behavior.
--
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]
