From 623ccd1a4ff1cf37a9835cb226d3bfe653cde3c6 Mon Sep 17 00:00:00 2001 From: Sam Brannen <104798+sbrannen@users.noreply.github.com> Date: Thu, 16 Apr 2026 14:20:25 +0200 Subject: [PATCH] Revise "Skip binding entirely when field is not allowed" This commit reverts the changes made to WebDataBinder's doBind() implementation in e4d03f6625 and instead implements the skipping logic directly in checkFieldDefaults(), checkFieldMarkers(), and adaptEmptyArrayIndices() by preemptively checking if the corresponding field is allowed. This commit also improves the Javadoc and adds missing tests. See gh-36625 Fixes gh-36627 (cherry picked from commit 68c575ab14057e04dba86beae64167bbfe3ac9fe) --- .../web/bind/WebDataBinder.java | 57 ++++---- .../support/WebRequestDataBinderTests.java | 134 ++++++++++++++---- 2 files changed, 137 insertions(+), 54 deletions(-) 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 6b77c06fb94..0c7cec036f0 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 @@ -54,6 +54,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 @@ -222,28 +223,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(); @@ -252,7 +253,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); @@ -262,14 +263,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) { @@ -279,7 +281,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)); } @@ -290,19 +292,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/WebRequestDataBinderTests.java b/spring-web/src/test/java/org/springframework/web/bind/support/WebRequestDataBinderTests.java index 2a7c4b826cb..bd1d341d6fc 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 @@ -23,7 +23,12 @@ 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; @@ -37,9 +42,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 +90,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 +110,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 +161,7 @@ class WebRequestDataBinderTests { assertThat(target.isPostProcessed()).isFalse(); } - // SPR-13502 - @Test + @Test // SPR-13502 void collectionFieldsDefault() { TestBean target = new TestBean(); target.setSomeSet(null); @@ -332,7 +352,7 @@ class WebRequestDataBinderTests { 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 is string").isInstanceOf(String.class); assertThat(val).as("val matches expected").isEqualTo(pv.getValue()); m.remove(pv.getName()); } @@ -359,21 +379,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 +483,4 @@ class WebRequestDataBinderTests { } } - }