From 3505c4bcad9fab4a6754d6a4f331669722ae2fae Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simon=20Basl=C3=A9?= Date: Thu, 26 Dec 2024 15:37:00 +0100 Subject: [PATCH 1/2] Ensure DataBinder can bind constructor with a Map with simple values This change ensures that DataBinder can bind constructor with a Map parameter that has no nested properties, but just simple values like a String: `someMap[0]=exampleString`. Integration tests have been added to cover similar cases that use the ServletRequestDataBinder. Closes gh-34043 --- .../validation/DataBinder.java | 4 +- ...vletRequestDataBinderIntegrationTests.java | 138 ++++++++++++++++++ 2 files changed, 140 insertions(+), 2 deletions(-) create mode 100644 spring-test/src/test/java/org/springframework/test/web/servlet/samples/spr/ServletRequestDataBinderIntegrationTests.java diff --git a/spring-context/src/main/java/org/springframework/validation/DataBinder.java b/spring-context/src/main/java/org/springframework/validation/DataBinder.java index 41b8ec3a58..3e9c9893ed 100644 --- a/spring-context/src/main/java/org/springframework/validation/DataBinder.java +++ b/spring-context/src/main/java/org/springframework/validation/DataBinder.java @@ -1080,7 +1080,7 @@ public class DataBinder implements PropertyEditorRegistry, TypeConverter { } int startIdx = paramPath.length() + 1; int endIdx = name.indexOf(']', startIdx); - String nestedPath = name.substring(0, endIdx + 2); + String nestedPath = ((name.length() > endIdx + 1) ? name.substring(0, endIdx + 2) : ""); boolean quoted = (endIdx - startIdx > 2 && name.charAt(startIdx) == '\'' && name.charAt(endIdx - 1) == '\''); String key = (quoted ? name.substring(startIdx + 1, endIdx - 1) : name.substring(startIdx, endIdx)); if (map == null) { @@ -1114,7 +1114,7 @@ public class DataBinder implements PropertyEditorRegistry, TypeConverter { SortedSet indexes = null; for (String name : valueResolver.getNames()) { if (name.startsWith(paramPath + "[")) { - int endIndex = name.indexOf(']', paramPath.length() + 2); + int endIndex = name.indexOf(']', paramPath.length() + 1); String rawIndex = name.substring(paramPath.length() + 1, endIndex); int index = Integer.parseInt(rawIndex); indexes = (indexes != null ? indexes : new TreeSet<>()); diff --git a/spring-test/src/test/java/org/springframework/test/web/servlet/samples/spr/ServletRequestDataBinderIntegrationTests.java b/spring-test/src/test/java/org/springframework/test/web/servlet/samples/spr/ServletRequestDataBinderIntegrationTests.java new file mode 100644 index 0000000000..1750d4a895 --- /dev/null +++ b/spring-test/src/test/java/org/springframework/test/web/servlet/samples/spr/ServletRequestDataBinderIntegrationTests.java @@ -0,0 +1,138 @@ +/* + * Copyright 2002-2024 the original author or authors. + * + * Licensed 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 + * + * https://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.springframework.test.web.servlet.samples.spr; + +import java.util.List; +import java.util.Map; + +import org.junit.jupiter.api.Disabled; +import org.junit.jupiter.api.Test; + +import org.springframework.test.context.junit.jupiter.web.SpringJUnitWebConfig; +import org.springframework.test.web.servlet.MockMvc; +import org.springframework.web.bind.annotation.ModelAttribute; +import org.springframework.web.bind.annotation.PostMapping; +import org.springframework.web.bind.annotation.RestController; +import org.springframework.web.context.WebApplicationContext; + +import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.post; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.content; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; +import static org.springframework.test.web.servlet.setup.MockMvcBuilders.webAppContextSetup; + +@SpringJUnitWebConfig(ServletRequestDataBinderIntegrationTests.SpringWebKeyValueController.class) +class ServletRequestDataBinderIntegrationTests { + + @Test // gh-34043 + void postMap(WebApplicationContext wac) throws Exception { + MockMvc mockMvc = webAppContextSetup(wac).build(); + mockMvc.perform(post("/map") + .param("someMap[a]", "valueA") + .param("someMap[b]", "valueB")) + .andExpect(status().isOk()) + .andExpect(content().string("valueB")); + } + + @Test + void postArray(WebApplicationContext wac) throws Exception { + MockMvc mockMvc = webAppContextSetup(wac).build(); + mockMvc.perform(post("/array") + .param("someArray[0]", "valueA") + .param("someArray[1]", "valueB")) + .andExpect(status().isOk()) + .andExpect(content().string("valueB")); + } + + @Disabled("see gh-34121") + @Test // gh-34121 + void postArrayWithEmptyIndex(WebApplicationContext wac) throws Exception { + MockMvc mockMvc = webAppContextSetup(wac).build(); + mockMvc.perform(post("/array") + .param("someArray[]", "valueA") + .param("someArray[]", "valueB")) + .andExpect(status().isOk()) + .andExpect(content().string("valueB")); + } + + @Test + void postArrayWithoutIndex(WebApplicationContext wac) throws Exception { + MockMvc mockMvc = webAppContextSetup(wac).build(); + mockMvc.perform(post("/array") + .param("someArray", "valueA") + .param("someArray", "valueB")) + .andExpect(status().isOk()) + .andExpect(content().string("valueB")); + } + + @Test + void postList(WebApplicationContext wac) throws Exception { + MockMvc mockMvc = webAppContextSetup(wac).build(); + mockMvc.perform(post("/list") + .param("someList[0]", "valueA") + .param("someList[1]", "valueB")) + .andExpect(status().isOk()) + .andExpect(content().string("valueB")); + } + + @Disabled("see gh-34121") + @Test // gh-34121 + void postListWithEmptyIndex(WebApplicationContext wac) throws Exception { + MockMvc mockMvc = webAppContextSetup(wac).build(); + mockMvc.perform(post("/list") + .param("someList[]", "valueA") + .param("someList[]", "valueB")) + .andExpect(status().isOk()) + .andExpect(content().string("valueB")); + } + + @Test + void postListWithoutIndex(WebApplicationContext wac) throws Exception { + MockMvc mockMvc = webAppContextSetup(wac).build(); + mockMvc.perform(post("/list") + .param("someList", "valueA") + .param("someList", "valueB")) + .andExpect(status().isOk()) + .andExpect(content().string("valueB")); + } + + record PayloadWithMap(Map someMap) {} + + record PayloadWithArray(String[] someArray) {} + + record PayloadWithList(List someList) {} + + @RestController + @SuppressWarnings("unused") + static class SpringWebKeyValueController { + + @PostMapping("/map") + String postMap(@ModelAttribute("payload") PayloadWithMap payload) { + return payload.someMap.get("b"); + } + + @PostMapping("/array") + String postArray(@ModelAttribute("payload") PayloadWithArray payload) { + return payload.someArray[1]; + } + + @PostMapping("/list") + String postList(@ModelAttribute("payload") PayloadWithList payload) { + return payload.someList.get(1); + } + } + +} From 0f38c28e9155e0190cf3ffb5478aca3e9282b6e7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simon=20Basl=C3=A9?= Date: Fri, 27 Dec 2024 10:30:35 +0100 Subject: [PATCH 2/2] Fix ServletRequestDataBinder ctor binding with `[]`-indexed query params This change ensures that a request containing query parameters in the array format `someArray[]=value` can be bound into a simple array in constructors, even for cases where the array values don't have nested properties. The value resolver is directly called in the constructor case, before any mutable properties are considered or even cleared (see `WebDataBinder#adaptEmptyArrayIndices` method). As a result, we need to accommodate the possibility that the request stores array elements under the `name[]` key rather than `name`. This change attempts a secondary lookup with the `[]` suffix if the type is a list or array, and the key doesn't include an index. Closes gh-34121 --- ...vletRequestDataBinderIntegrationTests.java | 3 --- .../web/bind/ServletRequestDataBinder.java | 4 +++ .../bind/ServletRequestDataBinderTests.java | 26 +++++++++++++++++++ 3 files changed, 30 insertions(+), 3 deletions(-) diff --git a/spring-test/src/test/java/org/springframework/test/web/servlet/samples/spr/ServletRequestDataBinderIntegrationTests.java b/spring-test/src/test/java/org/springframework/test/web/servlet/samples/spr/ServletRequestDataBinderIntegrationTests.java index 1750d4a895..64d7d5f434 100644 --- a/spring-test/src/test/java/org/springframework/test/web/servlet/samples/spr/ServletRequestDataBinderIntegrationTests.java +++ b/spring-test/src/test/java/org/springframework/test/web/servlet/samples/spr/ServletRequestDataBinderIntegrationTests.java @@ -19,7 +19,6 @@ package org.springframework.test.web.servlet.samples.spr; import java.util.List; import java.util.Map; -import org.junit.jupiter.api.Disabled; import org.junit.jupiter.api.Test; import org.springframework.test.context.junit.jupiter.web.SpringJUnitWebConfig; @@ -57,7 +56,6 @@ class ServletRequestDataBinderIntegrationTests { .andExpect(content().string("valueB")); } - @Disabled("see gh-34121") @Test // gh-34121 void postArrayWithEmptyIndex(WebApplicationContext wac) throws Exception { MockMvc mockMvc = webAppContextSetup(wac).build(); @@ -88,7 +86,6 @@ class ServletRequestDataBinderIntegrationTests { .andExpect(content().string("valueB")); } - @Disabled("see gh-34121") @Test // gh-34121 void postListWithEmptyIndex(WebApplicationContext wac) throws Exception { MockMvc mockMvc = webAppContextSetup(wac).build(); diff --git a/spring-web/src/main/java/org/springframework/web/bind/ServletRequestDataBinder.java b/spring-web/src/main/java/org/springframework/web/bind/ServletRequestDataBinder.java index 07ae355566..6add6a2dbb 100644 --- a/spring-web/src/main/java/org/springframework/web/bind/ServletRequestDataBinder.java +++ b/spring-web/src/main/java/org/springframework/web/bind/ServletRequestDataBinder.java @@ -244,6 +244,10 @@ public class ServletRequestDataBinder extends WebDataBinder { @Nullable protected Object getRequestParameter(String name, Class type) { Object value = this.request.getParameterValues(name); + if (value == null && !name.endsWith ("[]") && + (List.class.isAssignableFrom(type) || type.isArray())) { + value = this.request.getParameterValues(name + "[]"); + } return (ObjectUtils.isArray(value) && Array.getLength(value) == 1 ? Array.get(value, 0) : value); } diff --git a/spring-web/src/test/java/org/springframework/web/bind/ServletRequestDataBinderTests.java b/spring-web/src/test/java/org/springframework/web/bind/ServletRequestDataBinderTests.java index adbae1ddc6..fa10f23a04 100644 --- a/spring-web/src/test/java/org/springframework/web/bind/ServletRequestDataBinderTests.java +++ b/spring-web/src/test/java/org/springframework/web/bind/ServletRequestDataBinderTests.java @@ -93,6 +93,32 @@ class ServletRequestDataBinderTests { assertThat(target.isPostProcessed()).isFalse(); } + @Test + public void testFieldWithArrayIndex() { + TestBean target = new TestBean(); + ServletRequestDataBinder binder = new ServletRequestDataBinder(target); + binder.setIgnoreUnknownFields(false); + + MockHttpServletRequest request = new MockHttpServletRequest(); + request.addParameter("stringArray[0]", "ONE"); + request.addParameter("stringArray[1]", "TWO"); + binder.bind(request); + assertThat(target.getStringArray()).containsExactly("ONE", "TWO"); + } + + @Test + public void testFieldWithEmptyArrayIndex() { + TestBean target = new TestBean(); + ServletRequestDataBinder binder = new ServletRequestDataBinder(target); + binder.setIgnoreUnknownFields(false); + + MockHttpServletRequest request = new MockHttpServletRequest(); + request.addParameter("stringArray[]", "ONE"); + request.addParameter("stringArray[]", "TWO"); + binder.bind(request); + assertThat(target.getStringArray()).containsExactly("ONE", "TWO"); + } + @Test void testFieldDefault() { TestBean target = new TestBean();