Copilot commented on code in PR #422: URL: https://github.com/apache/rocketmq-dashboard/pull/422#discussion_r3542690302
########## src/test/java/org/apache/rocketmq/dashboard/controller/ProxyControllerTest.java: ########## @@ -0,0 +1,101 @@ +/* + * 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.dashboard.controller; + +import com.google.common.collect.Lists; +import org.apache.rocketmq.dashboard.config.RMQConfigure; +import org.apache.rocketmq.dashboard.service.impl.ProxyServiceImpl; +import org.junit.Before; +import org.junit.Test; +import org.mockito.InjectMocks; +import org.mockito.Mock; +import org.mockito.Spy; +import org.springframework.http.MediaType; +import org.springframework.test.web.servlet.request.MockMvcRequestBuilders; + +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.Mockito.doNothing; +import static org.mockito.Mockito.when; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.jsonPath; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; + +public class ProxyControllerTest extends BaseControllerTest { + + @InjectMocks + private ProxyController proxyController; + + @Spy + private ProxyServiceImpl proxyService; + + @Mock + private RMQConfigure configure; + Review Comment: This `configure` field hides the `protected RMQConfigure configure` mock already declared in `BaseControllerTest`, which makes auto-injection into spies ambiguous and can cause stubbing to target a different instance than the one actually used. Remove the duplicate field and use the inherited `configure` mock (and then clean up now-unused imports/annotations if needed). ########## src/main/java/org/apache/rocketmq/dashboard/controller/ProxyController.java: ########## @@ -41,14 +42,16 @@ public Object homePage() { @RequestMapping(value = "/addProxyAddr.do", method = RequestMethod.POST) @ResponseBody - public Object addProxyAddr(@RequestParam String newProxyAddr) { + public Object addProxyAddr(@RequestBody Map<String, String> params) { + String newProxyAddr = params.get("newProxyAddr"); proxyService.addProxyAddrList(newProxyAddr); return true; } Review Comment: `@RequestBody Map` will successfully bind `{}` (or a body missing `newProxyAddr`) and `params.get("newProxyAddr")` becomes null. That propagates into `addProxyAddrList`, which can end up adding `null` into the configured address list and still return success. This also changes behavior compared to the previous `@RequestParam` binding (which rejected missing params). Add an explicit required/non-blank check and fail fast with a clear message. ########## src/main/java/org/apache/rocketmq/dashboard/controller/ProxyController.java: ########## @@ -41,14 +42,16 @@ public Object homePage() { @RequestMapping(value = "/addProxyAddr.do", method = RequestMethod.POST) @ResponseBody - public Object addProxyAddr(@RequestParam String newProxyAddr) { + public Object addProxyAddr(@RequestBody Map<String, String> params) { + String newProxyAddr = params.get("newProxyAddr"); proxyService.addProxyAddrList(newProxyAddr); return true; } @RequestMapping(value = "/updateProxyAddr.do", method = RequestMethod.POST) @ResponseBody - public Object updateProxyAddr(@RequestParam String proxyAddr) { + public Object updateProxyAddr(@RequestBody Map<String, String> params) { + String proxyAddr = params.get("proxyAddr"); proxyService.updateProxyAddrList(proxyAddr); return true; } Review Comment: `@RequestBody Map` does not enforce presence of `proxyAddr` within the JSON object; requests like `{}` will set `proxyAddr` to null and still return success, allowing the configured proxy address to be cleared unintentionally. Add a required/non-blank check similar to the previous `@RequestParam` behavior. ########## src/test/java/org/apache/rocketmq/dashboard/controller/ProxyControllerTest.java: ########## @@ -0,0 +1,101 @@ +/* + * 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.dashboard.controller; + +import com.google.common.collect.Lists; +import org.apache.rocketmq.dashboard.config.RMQConfigure; +import org.apache.rocketmq.dashboard.service.impl.ProxyServiceImpl; +import org.junit.Before; +import org.junit.Test; +import org.mockito.InjectMocks; +import org.mockito.Mock; +import org.mockito.Spy; +import org.springframework.http.MediaType; +import org.springframework.test.web.servlet.request.MockMvcRequestBuilders; + +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.Mockito.doNothing; +import static org.mockito.Mockito.when; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.jsonPath; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; + +public class ProxyControllerTest extends BaseControllerTest { + + @InjectMocks + private ProxyController proxyController; + + @Spy + private ProxyServiceImpl proxyService; + + @Mock + private RMQConfigure configure; + + @Override + protected Object getTestController() { + return proxyController; + } + + @Before + public void init() { + super.mockRmqConfigure(); + } + + @Test + public void testHomePage() throws Exception { + final String url = "/proxy/homePage.query"; + when(configure.getProxyAddr()).thenReturn("127.0.0.1:8080"); + when(configure.getProxyAddrs()).thenReturn(Lists.newArrayList("127.0.0.1:8080", "127.0.0.2:8080")); + + requestBuilder = MockMvcRequestBuilders.get(url); + perform = mockMvc.perform(requestBuilder); + perform.andExpect(status().isOk()) + .andExpect(jsonPath("$.data").isMap()) + .andExpect(jsonPath("$.data.currentProxyAddr").value("127.0.0.1:8080")) + .andExpect(jsonPath("$.data.proxyAddrList").isArray()); + } + + @Test + public void testAddProxyAddr() throws Exception { + final String url = "/proxy/addProxyAddr.do"; + { + when(configure.getProxyAddrs()).thenReturn(Lists.newArrayList("127.0.0.1:8080")); + doNothing().when(configure).setProxyAddrs(any()); + } + requestBuilder = MockMvcRequestBuilders.post(url) + .contentType(MediaType.APPLICATION_JSON) + .content("{\"newProxyAddr\":\"127.0.0.2:8080\"}"); + perform = mockMvc.perform(requestBuilder); + perform.andExpect(status().isOk()) + .andExpect(jsonPath("$.data").value(true)); + } Review Comment: This test only asserts `$.data == true`, so it would still pass if the JSON body was ignored (or if `newProxyAddr` was missing/parsed as null) as long as the controller returns success. Add a verification that the expected address is actually persisted via `configure.setProxyAddrs(...)`, which makes the test exercise the binding and the side effect that matters. ########## src/test/java/org/apache/rocketmq/dashboard/controller/ProxyControllerTest.java: ########## @@ -0,0 +1,101 @@ +/* + * 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.dashboard.controller; + +import com.google.common.collect.Lists; +import org.apache.rocketmq.dashboard.config.RMQConfigure; +import org.apache.rocketmq.dashboard.service.impl.ProxyServiceImpl; +import org.junit.Before; +import org.junit.Test; +import org.mockito.InjectMocks; +import org.mockito.Mock; +import org.mockito.Spy; +import org.springframework.http.MediaType; +import org.springframework.test.web.servlet.request.MockMvcRequestBuilders; + +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.Mockito.doNothing; +import static org.mockito.Mockito.when; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.jsonPath; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; + +public class ProxyControllerTest extends BaseControllerTest { + + @InjectMocks + private ProxyController proxyController; + + @Spy + private ProxyServiceImpl proxyService; + + @Mock + private RMQConfigure configure; + + @Override + protected Object getTestController() { + return proxyController; + } + + @Before + public void init() { + super.mockRmqConfigure(); + } + + @Test + public void testHomePage() throws Exception { + final String url = "/proxy/homePage.query"; + when(configure.getProxyAddr()).thenReturn("127.0.0.1:8080"); + when(configure.getProxyAddrs()).thenReturn(Lists.newArrayList("127.0.0.1:8080", "127.0.0.2:8080")); + + requestBuilder = MockMvcRequestBuilders.get(url); + perform = mockMvc.perform(requestBuilder); + perform.andExpect(status().isOk()) + .andExpect(jsonPath("$.data").isMap()) + .andExpect(jsonPath("$.data.currentProxyAddr").value("127.0.0.1:8080")) + .andExpect(jsonPath("$.data.proxyAddrList").isArray()); + } + + @Test + public void testAddProxyAddr() throws Exception { + final String url = "/proxy/addProxyAddr.do"; + { + when(configure.getProxyAddrs()).thenReturn(Lists.newArrayList("127.0.0.1:8080")); + doNothing().when(configure).setProxyAddrs(any()); + } + requestBuilder = MockMvcRequestBuilders.post(url) + .contentType(MediaType.APPLICATION_JSON) + .content("{\"newProxyAddr\":\"127.0.0.2:8080\"}"); + perform = mockMvc.perform(requestBuilder); + perform.andExpect(status().isOk()) + .andExpect(jsonPath("$.data").value(true)); + } + + @Test + public void testUpdateProxyAddr() throws Exception { + final String url = "/proxy/updateProxyAddr.do"; + { + doNothing().when(configure).setProxyAddr(anyString()); + } + requestBuilder = MockMvcRequestBuilders.post(url) + .contentType(MediaType.APPLICATION_JSON) + .content("{\"proxyAddr\":\"127.0.0.2:8080\"}"); + perform = mockMvc.perform(requestBuilder); + perform.andExpect(status().isOk()) + .andExpect(jsonPath("$.data").value(true)); + } Review Comment: Similar to `testAddProxyAddr`, this test only checks the controller returns success. Add a verification that `configure.setProxyAddr(...)` is called with the expected value so the test actually validates JSON binding and the intended side effect. -- 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]
