diff --git a/spring-web/src/main/java/org/springframework/web/bind/WebDataBinder.java b/spring-web/src/main/java/org/springframework/web/bind/WebDataBinder.java index 52abea7e40d..092b451281c 100644 --- a/spring-web/src/main/java/org/springframework/web/bind/WebDataBinder.java +++ b/spring-web/src/main/java/org/springframework/web/bind/WebDataBinder.java @@ -55,6 +55,7 @@ import org.springframework.web.multipart.MultipartFile; * @author Juergen Hoeller * @author Scott Andrews * @author Brian Clozel + * @author Sam Brannen * @since 1.2 * @see #registerCustomEditor * @see #setAllowedFields @@ -220,28 +221,28 @@ public class WebDataBinder extends DataBinder { } /** - * This implementation performs a field default and marker check - * before delegating to the superclass binding process. - * @see #checkFieldDefaults - * @see #checkFieldMarkers + * This implementation checks default fields and marker fields and then adapts + * empty array indices before delegating to the superclass binding process. + * @see #checkFieldDefaults(MutablePropertyValues) + * @see #checkFieldMarkers(MutablePropertyValues) + * @see #adaptEmptyArrayIndices(MutablePropertyValues) */ @Override protected void doBind(MutablePropertyValues mpvs) { - checkAllowedFields(mpvs); checkFieldDefaults(mpvs); checkFieldMarkers(mpvs); adaptEmptyArrayIndices(mpvs); - checkRequiredFields(mpvs); - applyPropertyValues(mpvs); + super.doBind(mpvs); } /** - * Check the given property values for field defaults, - * i.e. for fields that start with the field default prefix. - *

The existence of a field defaults indicates that the specified - * value should be used if the field is otherwise not present. + * Check the given property values for fields that start with the field default + * prefix. + *

The existence of a field default indicates that the specified value should + * be used if the field is allowed and is otherwise not present. * @param mpvs the property values to be bound (can be modified) - * @see #getFieldDefaultPrefix + * @see #getFieldDefaultPrefix() + * @see #isAllowed(String) */ protected void checkFieldDefaults(MutablePropertyValues mpvs) { String fieldDefaultPrefix = getFieldDefaultPrefix(); @@ -250,7 +251,7 @@ public class WebDataBinder extends DataBinder { for (PropertyValue pv : pvArray) { if (pv.getName().startsWith(fieldDefaultPrefix)) { String field = pv.getName().substring(fieldDefaultPrefix.length()); - if (getPropertyAccessor().isWritableProperty(field) && !mpvs.contains(field)) { + if (isAllowed(field) && getPropertyAccessor().isWritableProperty(field) && !mpvs.contains(field)) { mpvs.add(field, pv.getValue()); } mpvs.removePropertyValue(pv); @@ -260,14 +261,15 @@ public class WebDataBinder extends DataBinder { } /** - * Check the given property values for field markers, - * i.e. for fields that start with the field marker prefix. - *

The existence of a field marker indicates that the specified - * field existed in the form. If the property values do not contain - * a corresponding field value, the field will be considered as empty - * and will be reset appropriately. + * Check the given property values for fields that start with the field marker + * prefix. + *

The existence of a field marker indicates that the specified field existed + * in the form. + *

If the field is allowed and the property values do not contain a corresponding + * field value, the field will be considered as empty and will be reset appropriately. * @param mpvs the property values to be bound (can be modified) - * @see #getFieldMarkerPrefix + * @see #getFieldMarkerPrefix() + * @see #isAllowed(String) * @see #getEmptyValue(String, Class) */ protected void checkFieldMarkers(MutablePropertyValues mpvs) { @@ -277,7 +279,7 @@ public class WebDataBinder extends DataBinder { for (PropertyValue pv : pvArray) { if (pv.getName().startsWith(fieldMarkerPrefix)) { String field = pv.getName().substring(fieldMarkerPrefix.length()); - if (getPropertyAccessor().isWritableProperty(field) && !mpvs.contains(field)) { + if (isAllowed(field) && getPropertyAccessor().isWritableProperty(field) && !mpvs.contains(field)) { Class fieldType = getPropertyAccessor().getPropertyType(field); mpvs.add(field, getEmptyValue(field, fieldType)); } @@ -288,19 +290,22 @@ public class WebDataBinder extends DataBinder { } /** - * Check for property values with names that end on {@code "[]"}. This is - * used by some clients for array syntax without an explicit index value. - * If such values are found, drop the brackets to adapt to the expected way - * of expressing the same for data binding purposes. + * Check the given property values for fields that end with empty brackets + * ({@code []}). + *

This is used by some clients for array syntax without an explicit index + * value. + *

If such a field is allowed, the brackets will be removed in order to adapt + * to the expected format for expressing the same for data binding purposes. * @param mpvs the property values to be bound (can be modified) * @since 5.3 + * @see #isAllowed(String) */ protected void adaptEmptyArrayIndices(MutablePropertyValues mpvs) { for (PropertyValue pv : mpvs.getPropertyValues()) { String name = pv.getName(); if (name.endsWith("[]")) { String field = name.substring(0, name.length() - 2); - if (getPropertyAccessor().isWritableProperty(field) && !mpvs.contains(field)) { + if (isAllowed(field) && getPropertyAccessor().isWritableProperty(field) && !mpvs.contains(field)) { mpvs.add(field, pv.getValue()); } mpvs.removePropertyValue(pv); diff --git a/spring-web/src/test/java/org/springframework/web/bind/support/ServletRequestParameterPropertyValuesTests.java b/spring-web/src/test/java/org/springframework/web/bind/support/ServletRequestParameterPropertyValuesTests.java new file mode 100644 index 00000000000..4822469760b --- /dev/null +++ b/spring-web/src/test/java/org/springframework/web/bind/support/ServletRequestParameterPropertyValuesTests.java @@ -0,0 +1,87 @@ +/* + * Copyright 2002-present 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.web.bind.support; + +import java.util.HashMap; +import java.util.Map; + +import org.junit.jupiter.api.Test; + +import org.springframework.beans.PropertyValue; +import org.springframework.beans.PropertyValues; +import org.springframework.web.bind.ServletRequestParameterPropertyValues; +import org.springframework.web.testfixture.servlet.MockHttpServletRequest; + +import static org.assertj.core.api.Assertions.assertThat; + +/** + * Tests for {@link ServletRequestParameterPropertyValues}. + */ +class ServletRequestParameterPropertyValuesTests { + + @Test + void noPrefix() { + MockHttpServletRequest request = new MockHttpServletRequest(); + request.addParameter("forname", "Tony"); + request.addParameter("surname", "Blair"); + request.addParameter("age", "" + 50); + + ServletRequestParameterPropertyValues pvs = new ServletRequestParameterPropertyValues(request); + testTony(pvs); + } + + @Test + void prefix() { + MockHttpServletRequest request = new MockHttpServletRequest(); + request.addParameter("test_forname", "Tony"); + request.addParameter("test_surname", "Blair"); + request.addParameter("test_age", "" + 50); + + ServletRequestParameterPropertyValues pvs = new ServletRequestParameterPropertyValues(request); + assertThat(pvs.contains("forname")).as("Didn't find normal when given prefix").isFalse(); + assertThat(pvs.contains("test_forname")).as("Did treat prefix as normal when not given prefix").isTrue(); + + pvs = new ServletRequestParameterPropertyValues(request, "test"); + testTony(pvs); + } + + /** + * Must contain: forname=Tony surname=Blair age=50 + */ + private static void testTony(PropertyValues pvs) { + assertThat(pvs.getPropertyValues().length).as("Contains 3").isEqualTo(3); + assertThat(pvs.contains("forname")).as("Contains forname").isTrue(); + assertThat(pvs.contains("surname")).as("Contains surname").isTrue(); + assertThat(pvs.contains("age")).as("Contains age").isTrue(); + assertThat(pvs.contains("tory")).as("Doesn't contain tory").isFalse(); + + PropertyValue[] pvArray = pvs.getPropertyValues(); + Map m = new HashMap<>(); + m.put("forname", "Tony"); + m.put("surname", "Blair"); + m.put("age", "50"); + for (PropertyValue pv : pvArray) { + Object val = m.get(pv.getName()); + assertThat(val).as("Can't have unexpected value").isNotNull(); + assertThat(val).as("Val is string").isInstanceOf(String.class); + assertThat(val).as("val matches expected").isEqualTo(pv.getValue()); + m.remove(pv.getName()); + } + assertThat(m.size()).as("Map size is 0").isEqualTo(0); + } + +} diff --git a/spring-web/src/test/java/org/springframework/web/bind/support/WebRequestDataBinderTests.java b/spring-web/src/test/java/org/springframework/web/bind/support/WebRequestDataBinderTests.java index 2a7c4b826cb..d126e0a6713 100644 --- a/spring-web/src/test/java/org/springframework/web/bind/support/WebRequestDataBinderTests.java +++ b/spring-web/src/test/java/org/springframework/web/bind/support/WebRequestDataBinderTests.java @@ -18,15 +18,17 @@ package org.springframework.web.bind.support; import java.beans.PropertyEditorSupport; import java.util.Arrays; -import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.Set; +import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.Parameter; +import org.junit.jupiter.params.ParameterizedClass; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; -import org.springframework.beans.PropertyValue; -import org.springframework.beans.PropertyValues; import org.springframework.beans.testfixture.beans.ITestBean; import org.springframework.beans.testfixture.beans.TestBean; import org.springframework.web.bind.ServletRequestParameterPropertyValues; @@ -37,9 +39,15 @@ import org.springframework.web.testfixture.servlet.MockMultipartFile; import org.springframework.web.testfixture.servlet.MockMultipartHttpServletRequest; import static org.assertj.core.api.Assertions.assertThat; +import static org.springframework.web.bind.WebDataBinder.DEFAULT_FIELD_DEFAULT_PREFIX; +import static org.springframework.web.bind.WebDataBinder.DEFAULT_FIELD_MARKER_PREFIX; /** + * Tests for {@link WebRequestDataBinder}. + * * @author Juergen Hoeller + * @author Brian Clozel + * @author Sam Brannen */ class WebRequestDataBinderTests { @@ -79,10 +87,13 @@ class WebRequestDataBinderTests { assertThat(tb.getSpouse().getName()).isEqualTo("test"); } - @Test - void fieldPrefixCausesFieldReset() { + @ParameterizedTest + @ValueSource(booleans = { true, false }) + void markerPrefixCausesFieldReset(boolean ignoreUnknownFields) { TestBean target = new TestBean(); + WebRequestDataBinder binder = new WebRequestDataBinder(target); + binder.setIgnoreUnknownFields(ignoreUnknownFields); MockHttpServletRequest request = new MockHttpServletRequest(); request.addParameter("_postProcessed", "visible"); @@ -96,23 +107,30 @@ class WebRequestDataBinderTests { } @Test - void fieldPrefixCausesFieldResetWithIgnoreUnknownFields() { + void fieldWithArrayIndices() { TestBean target = new TestBean(); WebRequestDataBinder binder = new WebRequestDataBinder(target); - binder.setIgnoreUnknownFields(false); MockHttpServletRequest request = new MockHttpServletRequest(); - request.addParameter("_postProcessed", "visible"); - request.addParameter("postProcessed", "on"); + request.addParameter("stringArray[0]", "ONE"); + request.addParameter("stringArray[1]", "TWO"); binder.bind(new ServletWebRequest(request)); - assertThat(target.isPostProcessed()).isTrue(); - - request.removeParameter("postProcessed"); - binder.bind(new ServletWebRequest(request)); - assertThat(target.isPostProcessed()).isFalse(); + assertThat(target.getStringArray()).containsExactly("ONE", "TWO"); } - @Test // gh-25836 + @Test + void fieldWithMissingArrayIndex() { + TestBean target = new TestBean(); + WebRequestDataBinder binder = new WebRequestDataBinder(target); + + MockHttpServletRequest request = new MockHttpServletRequest(); + request.addParameter("stringArray", "ONE"); + request.addParameter("stringArray", "TWO"); + binder.bind(new ServletWebRequest(request)); + assertThat(target.getStringArray()).containsExactly("ONE", "TWO"); + } + + @Test // gh-25836 void fieldWithEmptyArrayIndex() { TestBean target = new TestBean(); WebRequestDataBinder binder = new WebRequestDataBinder(target); @@ -140,8 +158,7 @@ class WebRequestDataBinderTests { assertThat(target.isPostProcessed()).isFalse(); } - // SPR-13502 - @Test + @Test // SPR-13502 void collectionFieldsDefault() { TestBean target = new TestBean(); target.setSomeSet(null); @@ -288,57 +305,6 @@ class WebRequestDataBinderTests { assertThat(target.getStringArray()[1]).isEqualTo("Eva"); } - @Test - void noPrefix() { - MockHttpServletRequest request = new MockHttpServletRequest(); - request.addParameter("forname", "Tony"); - request.addParameter("surname", "Blair"); - request.addParameter("age", "" + 50); - - ServletRequestParameterPropertyValues pvs = new ServletRequestParameterPropertyValues(request); - doTestTony(pvs); - } - - @Test - void prefix() { - MockHttpServletRequest request = new MockHttpServletRequest(); - request.addParameter("test_forname", "Tony"); - request.addParameter("test_surname", "Blair"); - request.addParameter("test_age", "" + 50); - - ServletRequestParameterPropertyValues pvs = new ServletRequestParameterPropertyValues(request); - assertThat(pvs.contains("forname")).as("Didn't find normal when given prefix").isFalse(); - assertThat(pvs.contains("test_forname")).as("Did treat prefix as normal when not given prefix").isTrue(); - - pvs = new ServletRequestParameterPropertyValues(request, "test"); - doTestTony(pvs); - } - - /** - * Must contain: forname=Tony surname=Blair age=50 - */ - void doTestTony(PropertyValues pvs) { - assertThat(pvs.getPropertyValues().length).as("Contains 3").isEqualTo(3); - assertThat(pvs.contains("forname")).as("Contains forname").isTrue(); - assertThat(pvs.contains("surname")).as("Contains surname").isTrue(); - assertThat(pvs.contains("age")).as("Contains age").isTrue(); - assertThat(pvs.contains("tory")).as("Doesn't contain tory").isFalse(); - - PropertyValue[] pvArray = pvs.getPropertyValues(); - Map m = new HashMap<>(); - m.put("forname", "Tony"); - m.put("surname", "Blair"); - m.put("age", "50"); - for (PropertyValue pv : pvArray) { - Object val = m.get(pv.getName()); - assertThat(val).as("Can't have unexpected value").isNotNull(); - assertThat(val).as("Val i string").isInstanceOf(String.class); - assertThat(val).as("val matches expected").isEqualTo(pv.getValue()); - m.remove(pv.getName()); - } - assertThat(m.size()).as("Map size is 0").isEqualTo(0); - } - @Test void noParameters() { MockHttpServletRequest request = new MockHttpServletRequest(); @@ -359,21 +325,80 @@ class WebRequestDataBinderTests { assertThat(Arrays.asList(original)).as("Correct values").isEqualTo(Arrays.asList(values)); } - @Test - void defaultArgumentShouldNotTriggerAutoGrowWhenDisallowed() { - TestBean tb = new TestBean(); - tb.setSomeMap(null); - WebRequestDataBinder binder = new WebRequestDataBinder(tb, "person"); - binder.setAllowedFields("name"); + @ParameterizedClass // gh-36625 + @ValueSource(strings = { DEFAULT_FIELD_DEFAULT_PREFIX, DEFAULT_FIELD_MARKER_PREFIX }) + @Nested + class DefaultAndMarkerPrefixTests { - MockHttpServletRequest request = new MockHttpServletRequest(); - request.addParameter("name", "spring"); - request.addParameter("!someMap[key1]", "test"); - binder.bind(new ServletWebRequest(request)); + @Parameter + String prefix; - assertThat(tb.getName()).isEqualTo("spring"); - assertThat(tb.getSomeMap()).isNull(); + @Test + void shouldNotTriggerBindingWhenFieldIsNotAllowed() { + TestBean tb = new TestBean(); + + WebRequestDataBinder binder = new WebRequestDataBinder(tb, "person"); + binder.setAllowedFields("name"); + + MockHttpServletRequest request = new MockHttpServletRequest(); + request.addParameter("name", "spring"); + request.addParameter(prefix + "country", "test"); + binder.bind(new ServletWebRequest(request)); + + assertThat(tb.getName()).isEqualTo("spring"); + assertThat(tb.getCountry()).isNull(); + } + + @Test + void shouldNotTriggerBindingWhenFieldIsDisallowed() { + TestBean tb = new TestBean(); + + WebRequestDataBinder binder = new WebRequestDataBinder(tb, "person"); + binder.setDisallowedFields("country"); + + MockHttpServletRequest request = new MockHttpServletRequest(); + request.addParameter("name", "spring"); + request.addParameter(prefix + "country", "test"); + binder.bind(new ServletWebRequest(request)); + + assertThat(tb.getName()).isEqualTo("spring"); + assertThat(tb.getCountry()).isNull(); + } + + @Test + void shouldNotTriggerAutoGrowWhenFieldIsNotAllowed() { + TestBean tb = new TestBean(); + tb.setSomeMap(null); + + WebRequestDataBinder binder = new WebRequestDataBinder(tb, "person"); + binder.setAllowedFields("name"); + + MockHttpServletRequest request = new MockHttpServletRequest(); + request.addParameter("name", "spring"); + request.addParameter(prefix + "someMap[key1]", "test"); + binder.bind(new ServletWebRequest(request)); + + assertThat(tb.getName()).isEqualTo("spring"); + assertThat(tb.getSomeMap()).isNull(); + } + + @Test + void shouldNotTriggerAutoGrowWhenFieldIsDisallowed() { + TestBean tb = new TestBean(); + tb.setSomeMap(null); + + WebRequestDataBinder binder = new WebRequestDataBinder(tb, "person"); + binder.setDisallowedFields("someMap[key1]"); + + MockHttpServletRequest request = new MockHttpServletRequest(); + request.addParameter("name", "spring"); + request.addParameter(prefix + "someMap[key1]", "test"); + binder.bind(new ServletWebRequest(request)); + + assertThat(tb.getName()).isEqualTo("spring"); + assertThat(tb.getSomeMap()).isNull(); + } } @@ -404,5 +429,4 @@ class WebRequestDataBinderTests { } } - }