You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@rocketmq.apache.org by "messere1 (via GitHub)" <gi...@apache.org> on 2026/07/08 09:08:04 UTC
[PR] [ISSUE #390] Fix unable to add ProxyAddr by aligning frontend/backend parameter binding [rocketmq-dashboard]
messere1 opened a new pull request, #422:
URL: https://github.com/apache/rocketmq-dashboard/pull/422
## What is the purpose of the change
Fix [#390](https://github.com/apache/rocketmq-dashboard/issues/390) — users are unable to add a ProxyAddr in the dashboard UI.
**Root cause:** The frontend `addProxyAddr` API sends a JSON request body (`Content-Type: application/json`), but the backend `ProxyController.addProxyAddr` uses `@RequestParam`, which only binds query/form parameters — not JSON body. The two sides never agree on the parameter binding format.
Additionally, the `_fetch` utility in `remoteApi.js` has a header-merge order bug: `{...options.headers, 'Content-Type': 'application/json'}` always overwrites any caller-supplied `Content-Type`, so even if a caller sets `application/x-www-form-urlencoded`, it gets clobbered.
## Brief changelog
1. **`ProxyController.java`** — Changed `addProxyAddr` and `updateProxyAddr` from `@RequestParam String` to `@RequestBody Map<String, String>`, so the controller accepts JSON request bodies sent by the frontend.
2. **`remoteApi.js` (`_fetch`)** — Fixed header spread order from `{...options.headers, 'Content-Type': 'application/json'}` to `{'Content-Type': 'application/json', ...options.headers}`, so caller-provided headers take precedence.
3. **`remoteApi.js` (`addProxyAddr`)** — Changed from `URLSearchParams` form body to `JSON.stringify({newProxyAddr})` to match the new `@RequestBody` backend contract.
4. **`ProxyControllerTest.java`** (new) — Added 3 unit tests: `testHomePage`, `testAddProxyAddr` (POST with JSON body), `testUpdateProxyAddr` (POST with JSON body).
## Verifying this change
Run unit tests:
```bash
mvn test -Dtest=ProxyControllerTest
```
All 3 tests pass:
```
Tests run: 3, Failures: 0, Errors: 0, Skipped: 0
```
Manual verification:
1. Start dashboard, navigate to Proxy page.
2. Click "Add ProxyAddr", enter a valid address (e.g. `127.0.0.1:8081`).
3. The address should be added successfully without errors.
## Checklist
- [x] Make sure there is a [Github issue](https://github.com/apache/rocketmq/issues) filed for the change.
- [x] Format the pull request title like `[ISSUE #390] Fix ...`.
- [x] Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
- [x] Write necessary unit-test to verify your logic correction.
- [x] Run `mvn clean install -DskipITs` to make sure unit-test pass.
- [ ] If this contribution is large, please file an [Apache Individual Contributor License Agreement](http://www.apache.org/licenses/#clas).
--
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] [ISSUE #390] Fix unable to add ProxyAddr by aligning frontend/backend parameter binding [rocketmq-dashboard]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #422:
URL: https://github.com/apache/rocketmq-dashboard/pull/422#issuecomment-5008770221
This pull request has been automatically closed because there has been no activity for 7 days. If you would like to continue working on this, please reopen the PR with updated 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: commits-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ISSUE #390] Fix unable to add ProxyAddr by aligning frontend/backend parameter binding [rocketmq-dashboard]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #422: [ISSUE #390] Fix unable to add ProxyAddr by aligning frontend/backend parameter binding
URL: https://github.com/apache/rocketmq-dashboard/pull/422
--
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] [ISSUE #390] Fix unable to add ProxyAddr by aligning frontend/backend parameter binding [rocketmq-dashboard]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org