From e20bff9c64a8738be9e2ab8a012a15f7bf1ddf17 Mon Sep 17 00:00:00 2001 From: Juergen Hoeller Date: Tue, 1 Sep 2020 19:10:32 +0200 Subject: [PATCH] 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 --- .../org/springframework/beans/BeanUtils.java | 29 +++++++++++++ .../jdbc/core/DataClassRowMapper.java | 18 +------- .../ModelAttributeMethodProcessor.java | 38 +++++++++-------- .../ModelAttributeMethodArgumentResolver.java | 16 +------- ...nnotationControllerHandlerMethodTests.java | 41 +++++++++++++++++++ 5 files changed, 93 insertions(+), 49 deletions(-) diff --git a/spring-beans/src/main/java/org/springframework/beans/BeanUtils.java b/spring-beans/src/main/java/org/springframework/beans/BeanUtils.java index 33c70920d1e..3980a24dd71 100644 --- a/spring-beans/src/main/java/org/springframework/beans/BeanUtils.java +++ b/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 Constructor getResolvableConstructor(Class clazz) { + Constructor ctor = findPrimaryConstructor(clazz); + if (ctor == null) { + Constructor[] ctors = clazz.getConstructors(); + if (ctors.length == 1) { + ctor = (Constructor) 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 diff --git a/spring-jdbc/src/main/java/org/springframework/jdbc/core/DataClassRowMapper.java b/spring-jdbc/src/main/java/org/springframework/jdbc/core/DataClassRowMapper.java index 599f151c72f..1d419f9885f 100644 --- a/spring-jdbc/src/main/java/org/springframework/jdbc/core/DataClassRowMapper.java +++ b/spring-jdbc/src/main/java/org/springframework/jdbc/core/DataClassRowMapper.java @@ -79,23 +79,7 @@ public class DataClassRowMapper extends BeanPropertyRowMapper { protected void initialize(Class mappedClass) { super.initialize(mappedClass); - this.mappedConstructor = BeanUtils.findPrimaryConstructor(mappedClass); - - if (this.mappedConstructor == null) { - Constructor[] ctors = mappedClass.getConstructors(); - if (ctors.length == 1) { - this.mappedConstructor = (Constructor) 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(); diff --git a/spring-web/src/main/java/org/springframework/web/method/annotation/ModelAttributeMethodProcessor.java b/spring-web/src/main/java/org/springframework/web/method/annotation/ModelAttributeMethodProcessor.java index 72baeca5ecf..c09d9ec7534 100644 --- a/spring-web/src/main/java/org/springframework/web/method/annotation/ModelAttributeMethodProcessor.java +++ b/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); } diff --git a/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/annotation/ModelAttributeMethodArgumentResolver.java b/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/annotation/ModelAttributeMethodArgumentResolver.java index 1afa1a131c9..3375e264b91 100644 --- a/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/annotation/ModelAttributeMethodArgumentResolver.java +++ b/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); } diff --git a/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/ServletAnnotationControllerHandlerMethodTests.java b/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/ServletAnnotationControllerHandlerMethodTests.java index f325d931005..cadaa20ddfd 100644 --- a/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/ServletAnnotationControllerHandlerMethodTests.java +++ b/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 {