From 68c575ab14057e04dba86beae64167bbfe3ac9fe 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 1/2] 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. Fixes gh-36625 --- .../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 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/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 {
}
}
-
}
From c9b88b4ebd47c7616816602204b106dd2c1275ef Mon Sep 17 00:00:00 2001
From: Sam Brannen <104798+sbrannen@users.noreply.github.com>
Date: Thu, 16 Apr 2026 14:38:17 +0200
Subject: [PATCH 2/2] Extract ServletRequestParameterPropertyValuesTests
---
...etRequestParameterPropertyValuesTests.java | 87 +++++++++++++++++++
.../support/WebRequestDataBinderTests.java | 54 ------------
2 files changed, 87 insertions(+), 54 deletions(-)
create mode 100644 spring-web/src/test/java/org/springframework/web/bind/support/ServletRequestParameterPropertyValuesTests.java
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