You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@rocketmq.apache.org by "Aias00 (via GitHub)" <gi...@apache.org> on 2026/08/03 14:37:45 UTC
[PR] fix(server): validate consumer group create requests [rocketmq-dashboard]
Aias00 opened a new pull request, #868:
URL: https://github.com/apache/rocketmq-dashboard/pull/868
### Summary
- add a create-specific ConsumerGroup DTO with request validation
- reject blank group names and negative retry/delay values at the controller boundary
- keep the existing ConsumerGroupVO response model optional for list/detail responses
- add MockMvc regression coverage for valid and invalid create requests
### Related Issue
Fixes #867
### Tests
- `JAVA_HOME=/Users/aias/Library/Java/JavaVirtualMachines/openjdk-21.0.2/Contents/Home mvn -Dtest=ConsumerGroupControllerTest test`
--
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: commits-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(server): validate consumer group create requests [rocketmq-dashboard]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #868:
URL: https://github.com/apache/rocketmq-dashboard/pull/868#discussion_r3705038943
##########
server/src/main/java/org/apache/rocketmq/studio/instance/group/CreateConsumerGroupDTO.java:
##########
@@ -0,0 +1,57 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.rocketmq.studio.instance.group;
+
+import jakarta.validation.constraints.NotBlank;
+import jakarta.validation.constraints.PositiveOrZero;
+import lombok.Data;
+import org.apache.rocketmq.studio.common.domain.enums.ConsumeType;
+import org.apache.rocketmq.studio.common.domain.enums.SubscriptionMode;
+
+@Data
+public class CreateConsumerGroupDTO {
+ @NotBlank(message = "name is required")
+ private String name;
+ private String namespace;
+ private String clusterId;
+ private SubscriptionMode subscriptionMode;
+ private ConsumeType consumeType;
+ private String subscriptionDataType;
+ private String deliveryOrderType;
+ @PositiveOrZero(message = "retryMaxTimes must be zero or positive")
+ private Integer retryMaxTimes;
+ @PositiveOrZero(message = "delaySeconds must be zero or positive")
+ private Integer delaySeconds;
Review Comment:
This PR adds validation for `delaySeconds` (must be zero or positive), but the new MockMvc coverage only exercises invalid `retryMaxTimes`. Add a regression test for a negative `delaySeconds` payload asserting 400 + the expected message, and verify the service is not invoked.
##########
server/src/main/java/org/apache/rocketmq/studio/instance/group/CreateConsumerGroupDTO.java:
##########
@@ -0,0 +1,57 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.rocketmq.studio.instance.group;
+
+import jakarta.validation.constraints.NotBlank;
+import jakarta.validation.constraints.PositiveOrZero;
+import lombok.Data;
+import org.apache.rocketmq.studio.common.domain.enums.ConsumeType;
+import org.apache.rocketmq.studio.common.domain.enums.SubscriptionMode;
+
+@Data
+public class CreateConsumerGroupDTO {
+ @NotBlank(message = "name is required")
+ private String name;
+ private String namespace;
+ private String clusterId;
+ private SubscriptionMode subscriptionMode;
+ private ConsumeType consumeType;
+ private String subscriptionDataType;
+ private String deliveryOrderType;
+ @PositiveOrZero(message = "retryMaxTimes must be zero or positive")
+ private Integer retryMaxTimes;
+ @PositiveOrZero(message = "delaySeconds must be zero or positive")
+ private Integer delaySeconds;
+
+ public ConsumerGroupVO toConsumerGroupVO() {
+ ConsumerGroupVO group = new ConsumerGroupVO();
+ group.setName(name);
+ group.setNamespace(namespace);
+ group.setClusterId(clusterId);
+ group.setSubscriptionMode(subscriptionMode);
+ group.setConsumeType(consumeType);
+ group.setSubscriptionDataType(subscriptionDataType);
+ group.setDeliveryOrderType(deliveryOrderType);
+ if (retryMaxTimes != null) {
+ group.setRetryMaxTimes(retryMaxTimes);
+ }
+ if (delaySeconds != null) {
+ group.setDelaySeconds(delaySeconds);
+ }
Review Comment:
The DTO→VO mapping now conditionally forwards `delaySeconds`. The current 'happy path' test sends `delaySeconds: 0` but doesn’t assert that `delaySeconds` is actually propagated to `metadataService.createConsumerGroup(...)`. Consider asserting `captor.getValue().getDelaySeconds()` (and/or the response JSON) to prevent regressions in this new mapping.
##########
server/src/test/java/org/apache/rocketmq/studio/instance/group/ConsumerGroupControllerTest.java:
##########
@@ -56,6 +59,70 @@ class ConsumerGroupControllerTest {
@MockBean
private ConsumerDiagnosticsService consumerDiagnosticsService;
+ @Test
+ void createConsumerGroupShouldPassValidatedRequest() throws Exception {
+ Map<String, Object> body = Map.of(
+ "name", "cg-orders",
+ "clusterId", "cluster-a",
+ "retryMaxTimes", 8,
+ "delaySeconds", 0
+ );
+ ConsumerGroupVO created = new ConsumerGroupVO();
+ created.setName("cg-orders");
+ created.setClusterId("cluster-a");
+ created.setRetryMaxTimes(8);
+
+ when(metadataService.createConsumerGroup(any(ConsumerGroupVO.class))).thenReturn(created);
+
+ mockMvc.perform(post("/api/groups/create")
+ .contentType(MediaType.APPLICATION_JSON)
+ .content(objectMapper.writeValueAsString(body)))
+ .andExpect(status().isOk())
+ .andExpect(jsonPath("$.code").value(200))
+ .andExpect(jsonPath("$.data.name").value("cg-orders"))
+ .andExpect(jsonPath("$.data.retryMaxTimes").value(8));
+
+ ArgumentCaptor<ConsumerGroupVO> captor = ArgumentCaptor.forClass(ConsumerGroupVO.class);
+ verify(metadataService).createConsumerGroup(captor.capture());
+ assertThat(captor.getValue().getName()).isEqualTo("cg-orders");
+ assertThat(captor.getValue().getClusterId()).isEqualTo("cluster-a");
+ assertThat(captor.getValue().getRetryMaxTimes()).isEqualTo(8);
+ }
+
+ @Test
+ void createConsumerGroupShouldRejectMissingName() throws Exception {
+ Map<String, Object> body = Map.of(
+ "clusterId", "cluster-a",
+ "retryMaxTimes", 8
+ );
+
+ mockMvc.perform(post("/api/groups/create")
+ .contentType(MediaType.APPLICATION_JSON)
+ .content(objectMapper.writeValueAsString(body)))
+ .andExpect(status().isBadRequest())
+ .andExpect(jsonPath("$.code").value(400))
+ .andExpect(jsonPath("$.message").value("name is required"));
+
+ verifyNoInteractions(metadataService);
Review Comment:
`verifyNoInteractions(metadataService)` is used in the new tests. In the shown import block, there is no static import for `verifyNoInteractions`; if it’s not imported elsewhere in the file, this won’t compile. Add `import static org.mockito.Mockito.verifyNoInteractions;` (or qualify the call) to ensure compilation.
--
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: commits-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(server): validate consumer group create requests [rocketmq-dashboard]
Posted by "Aias00 (via GitHub)" <gi...@apache.org>.
Aias00 commented on PR #868:
URL: https://github.com/apache/rocketmq-dashboard/pull/868#issuecomment-5174205769
Superseded by consolidated PR #930, which contains this change and has passing checks. Closing this narrower PR to keep review focused.
--
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: commits-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(server): validate consumer group create requests [rocketmq-dashboard]
Posted by "Aias00 (via GitHub)" <gi...@apache.org>.
Aias00 closed pull request #868: fix(server): validate consumer group create requests
URL: https://github.com/apache/rocketmq-dashboard/pull/868
--
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: commits-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org