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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
