Consistent data class constructor resolution with clear error message

MVC data class processor constructs target instance even in case of binding failure, as long as the corresponding method parameter is not marked as optional.

Closes gh-24372

spring-beans/src/main/java/org/springframework/beans/BeanUtils.java

@@ -226,6 +226,35 @@ public abstract class BeanUtils {
 		}
 	}
 
+	/**
+	 * Return a resolvable constructor for the provided class, either a primary constructor
+	 * or single public constructor or simply a default constructor. Callers have to be
+	 * prepared to resolve arguments for the returned constructor's parameters, if any.
+	 * @param clazz the class to check
+	 * @since 5.3
+	 * @see #findPrimaryConstructor
+	 */
+	@SuppressWarnings("unchecked")
+	public static <T> Constructor<T> getResolvableConstructor(Class<T> clazz) {
+		Constructor<T> ctor = findPrimaryConstructor(clazz);
+		if (ctor == null) {
+			Constructor<?>[] ctors = clazz.getConstructors();
+			if (ctors.length == 1) {
+				ctor = (Constructor<T>) ctors[0];
+			}
+			else {
+				try {
+					ctor = clazz.getDeclaredConstructor();
+				}
+				catch (NoSuchMethodException ex) {
+					throw new IllegalStateException("No primary or single public constructor found for " +
+							clazz + " - and no default constructor found either");
+				}
+			}
+		}
+		return ctor;
+	}
+
 	/**
 	 * Return the primary constructor of the provided class. For Kotlin classes, this
 	 * returns the Java constructor corresponding to the Kotlin primary constructor

spring-jdbc/src/main/java/org/springframework/jdbc/core/DataClassRowMapper.java

@@ -79,23 +79,7 @@ public class DataClassRowMapper<T> extends BeanPropertyRowMapper<T> {
 	protected void initialize(Class<T> mappedClass) {
 		super.initialize(mappedClass);
 
-		this.mappedConstructor = BeanUtils.findPrimaryConstructor(mappedClass);
-
-		if (this.mappedConstructor == null) {
-			Constructor<?>[] ctors = mappedClass.getConstructors();
-			if (ctors.length == 1) {
-				this.mappedConstructor = (Constructor<T>) ctors[0];
-			}
-			else {
-				try {
-					this.mappedConstructor = mappedClass.getDeclaredConstructor();
-				}
-				catch (NoSuchMethodException ex) {
-					throw new IllegalStateException("No primary or default constructor found for " + mappedClass, ex);
-				}
-			}
-		}
-
+		this.mappedConstructor = BeanUtils.getResolvableConstructor(mappedClass);
 		if (this.mappedConstructor.getParameterCount() > 0) {
 			this.constructorParameterNames = BeanUtils.getParameterNames(this.mappedConstructor);
 			this.constructorParameterTypes = this.mappedConstructor.getParameterTypes();

spring-web/src/main/java/org/springframework/web/method/annotation/ModelAttributeMethodProcessor.java

@@ -33,6 +33,7 @@ import javax.servlet.http.Part;
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
 
+import org.springframework.beans.BeanInstantiationException;
 import org.springframework.beans.BeanUtils;
 import org.springframework.beans.TypeMismatchException;
 import org.springframework.core.MethodParameter;
@@ -149,6 +150,9 @@ public class ModelAttributeMethodProcessor implements HandlerMethodArgumentResol
 				if (parameter.getParameterType() == Optional.class) {
 					attribute = Optional.empty();
 				}
+				else {
+					attribute = ex.getTarget();
+				}
 				bindingResult = ex.getBindingResult();
 			}
 		}
@@ -207,22 +211,7 @@ public class ModelAttributeMethodProcessor implements HandlerMethodArgumentResol
 		MethodParameter nestedParameter = parameter.nestedIfOptional();
 		Class<?> clazz = nestedParameter.getNestedParameterType();
 
-		Constructor<?> ctor = BeanUtils.findPrimaryConstructor(clazz);
-		if (ctor == null) {
-			Constructor<?>[] ctors = clazz.getConstructors();
-			if (ctors.length == 1) {
-				ctor = ctors[0];
-			}
-			else {
-				try {
-					ctor = clazz.getDeclaredConstructor();
-				}
-				catch (NoSuchMethodException ex) {
-					throw new IllegalStateException("No primary or default constructor found for " + clazz, ex);
-				}
-			}
-		}
-
+		Constructor<?> ctor = BeanUtils.getResolvableConstructor(clazz);
 		Object attribute = constructAttribute(ctor, attributeName, parameter, binderFactory, webRequest);
 		if (parameter != nestedParameter) {
 			attribute = Optional.of(attribute);
@@ -244,6 +233,7 @@ public class ModelAttributeMethodProcessor implements HandlerMethodArgumentResol
 	 * @throws Exception in case of constructor invocation failure
 	 * @since 5.1
 	 */
+	@SuppressWarnings("serial")
 	protected Object constructAttribute(Constructor<?> ctor, String attributeName, MethodParameter parameter,
 			WebDataBinderFactory binderFactory, NativeWebRequest webRequest) throws Exception {
 
@@ -290,7 +280,7 @@ public class ModelAttributeMethodProcessor implements HandlerMethodArgumentResol
 			}
 			catch (TypeMismatchException ex) {
 				ex.initPropertyName(paramName);
-				args[i] = value;
+				args[i] = null;
 				failedParams.add(paramName);
 				binder.getBindingResult().recordFieldValue(paramName, paramType, value);
 				binder.getBindingErrorProcessor().processPropertyAccessException(ex, binder.getBindingResult());
@@ -308,6 +298,20 @@ public class ModelAttributeMethodProcessor implements HandlerMethodArgumentResol
 					validateValueIfApplicable(binder, parameter, ctor.getDeclaringClass(), paramName, value);
 				}
 			}
+			if (!parameter.isOptional()) {
+				try {
+					Object target = BeanUtils.instantiateClass(ctor, args);
+					throw new BindException(result) {
+						@Override
+						public Object getTarget() {
+							return target;
+						}
+					};
+				}
+				catch (BeanInstantiationException ex) {
+					// swallow and proceed without target instance
+				}
+			}
 			throw new BindException(result);
 		}
 

spring-webflux/src/main/java/org/springframework/web/reactive/result/method/annotation/ModelAttributeMethodArgumentResolver.java

@@ -201,21 +201,7 @@ public class ModelAttributeMethodArgumentResolver extends HandlerMethodArgumentR
 	private Mono<?> createAttribute(
 			String attributeName, Class<?> clazz, BindingContext context, ServerWebExchange exchange) {
 
-		Constructor<?> ctor = BeanUtils.findPrimaryConstructor(clazz);
-		if (ctor == null) {
-			Constructor<?>[] ctors = clazz.getConstructors();
-			if (ctors.length == 1) {
-				ctor = ctors[0];
-			}
-			else {
-				try {
-					ctor = clazz.getDeclaredConstructor();
-				}
-				catch (NoSuchMethodException ex) {
-					throw new IllegalStateException("No primary or default constructor found for " + clazz, ex);
-				}
-			}
-		}
+		Constructor<?> ctor = BeanUtils.getResolvableConstructor(clazz);
 		return constructAttribute(ctor, attributeName, context, exchange);
 	}
 

spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/ServletAnnotationControllerHandlerMethodTests.java

@@ -2060,6 +2060,31 @@ public class ServletAnnotationControllerHandlerMethodTests extends AbstractServl
 		assertThat(response.getContentAsString()).isEqualTo("2:null-x-null");
 	}
 
+	@PathPatternsParameterizedTest
+	void dataClassBindingWithNullable(boolean usePathPatterns) throws Exception {
+		initDispatcherServlet(NullableDataClassController.class, usePathPatterns);
+
+		MockHttpServletRequest request = new MockHttpServletRequest("GET", "/bind");
+		request.addParameter("param1", "value1");
+		request.addParameter("param2", "true");
+		request.addParameter("param3", "3");
+		MockHttpServletResponse response = new MockHttpServletResponse();
+		getServlet().service(request, response);
+		assertThat(response.getContentAsString()).isEqualTo("value1-true-3");
+	}
+
+	@PathPatternsParameterizedTest
+	void dataClassBindingWithNullableAndConversionError(boolean usePathPatterns) throws Exception {
+		initDispatcherServlet(NullableDataClassController.class, usePathPatterns);
+
+		MockHttpServletRequest request = new MockHttpServletRequest("GET", "/bind");
+		request.addParameter("param1", "value1");
+		request.addParameter("param2", "x");
+		MockHttpServletResponse response = new MockHttpServletResponse();
+		getServlet().service(request, response);
+		assertThat(response.getContentAsString()).isEqualTo("value1-x-null");
+	}
+
 	@PathPatternsParameterizedTest
 	void dataClassBindingWithOptional(boolean usePathPatterns) throws Exception {
 		initDispatcherServlet(OptionalDataClassController.class, usePathPatterns);
@@ -3895,6 +3920,7 @@ public class ServletAnnotationControllerHandlerMethodTests extends AbstractServl
 
 		@RequestMapping("/bind")
 		public BindStatusView handle(@Valid DataClass data, BindingResult result) {
+			assertThat(data).isNotNull();
 			if (result.hasErrors()) {
 				return new BindStatusView(result.getErrorCount() + ":" + result.getFieldValue("param1") + "-" +
 						result.getFieldValue("param2") + "-" + result.getFieldValue("param3"));
@@ -3987,6 +4013,21 @@ public class ServletAnnotationControllerHandlerMethodTests extends AbstractServl
 		}
 	}
 
+	@RestController
+	public static class NullableDataClassController {
+
+		@RequestMapping("/bind")
+		public String handle(@Nullable DataClass data, BindingResult result) {
+			if (result.hasErrors()) {
+				assertThat(data).isNull();
+				return result.getFieldValue("param1") + "-" + result.getFieldValue("param2") + "-" +
+						result.getFieldValue("param3");
+			}
+			assertThat(data).isNotNull();
+			return data.param1 + "-" + data.param2 + "-" + data.param3;
+		}
+	}
+
 	@RestController
 	public static class OptionalDataClassController {