From d4ac90dae0fecbcd96e07c48c041a9c905c048d0 Mon Sep 17 00:00:00 2001 From: rstoyanchev Date: Mon, 26 Jun 2023 17:01:44 +0100 Subject: [PATCH] Support nested constructors in DataBinder Closes gh-20806 --- .../validation/DataBinder.java | 130 +++++---- ...nnotationControllerHandlerMethodTests.java | 249 +++++++++++++++++- 2 files changed, 328 insertions(+), 51 deletions(-) diff --git a/spring-context/src/main/java/org/springframework/validation/DataBinder.java b/spring-context/src/main/java/org/springframework/validation/DataBinder.java index 0c43937b5b1b..6e48f6a6c015 100644 --- a/spring-context/src/main/java/org/springframework/validation/DataBinder.java +++ b/spring-context/src/main/java/org/springframework/validation/DataBinder.java @@ -850,14 +850,32 @@ public final void construct(ValueResolver valueResolver) { Assert.state(this.target == null, "Target instance already available"); Assert.state(this.targetType != null, "Target type not set"); - Class clazz = this.targetType.resolve(); - clazz = (Optional.class.equals(clazz) ? this.targetType.resolveGeneric(0) : clazz); - Assert.state(clazz != null, "Unknown data binding target type"); + this.target = createObject(this.targetType, "", valueResolver); + if (!getBindingResult().hasErrors()) { + this.bindingResult = null; + if (this.typeConverter != null) { + this.typeConverter.registerCustomEditors(getPropertyAccessor()); + } + } + } + + @Nullable + private Object createObject(ResolvableType objectType, String nestedPath, ValueResolver valueResolver) { + Class clazz = objectType.resolve(); + boolean isOptional = (clazz == Optional.class); + clazz = (isOptional ? objectType.resolveGeneric(0) : clazz); + if (clazz == null) { + throw new IllegalStateException( + "Insufficient type information to create instance of " + objectType); + } + + Object result = null; Constructor ctor = BeanUtils.getResolvableConstructor(clazz); + if (ctor.getParameterCount() == 0) { // A single default constructor -> clearly a standard JavaBeans arrangement. - this.target = BeanUtils.instantiateClass(ctor); + result = BeanUtils.instantiateClass(ctor); } else { // A single data class constructor -> resolve constructor arguments from request parameters. @@ -865,91 +883,103 @@ public final void construct(ValueResolver valueResolver) { Class[] paramTypes = ctor.getParameterTypes(); Object[] args = new Object[paramTypes.length]; Set failedParamNames = new HashSet<>(4); - boolean bindFailure = false; + for (int i = 0; i < paramNames.length; i++) { - String paramName = paramNames[i]; + String paramPath = nestedPath + paramNames[i]; Class paramType = paramTypes[i]; - Object value = valueResolver.resolveValue(paramName, paramType); - try { - MethodParameter methodParam = MethodParameter.forFieldAwareConstructor(ctor, i, paramName); - if (value == null && methodParam.isOptional()) { - args[i] = (methodParam.getParameterType() == Optional.class ? Optional.empty() : null); + Object value = valueResolver.resolveValue(paramPath, paramType); + + MethodParameter param = MethodParameter.forFieldAwareConstructor(ctor, i, paramNames[i]); + if (value == null && !BeanUtils.isSimpleValueType(param.nestedIfOptional().getNestedParameterType())) { + ResolvableType type = ResolvableType.forMethodParameter(param); + args[i] = createObject(type, paramPath + ".", valueResolver); + } + else { + try { + if (value == null && (param.isOptional() || getBindingResult().hasErrors())) { + args[i] = (param.getParameterType() == Optional.class ? Optional.empty() : null); + } + else { + args[i] = convertIfNecessary(value, paramType, param); + } } - else { - args[i] = convertIfNecessary(value, paramType, methodParam); + catch (TypeMismatchException ex) { + ex.initPropertyName(paramPath); + args[i] = null; + failedParamNames.add(paramPath); + getBindingResult().recordFieldValue(paramPath, paramType, value); + getBindingErrorProcessor().processPropertyAccessException(ex, getBindingResult()); } } - catch (TypeMismatchException ex) { - ex.initPropertyName(paramName); - args[i] = null; - failedParamNames.add(paramName); - getBindingResult().recordFieldValue(paramName, paramType, value); - getBindingErrorProcessor().processPropertyAccessException(ex, getBindingResult()); - bindFailure = true; - } } - if (bindFailure) { + if (getBindingResult().hasErrors()) { for (int i = 0; i < paramNames.length; i++) { - String paramName = paramNames[i]; - if (!failedParamNames.contains(paramName)) { + String paramPath = nestedPath + paramNames[i]; + if (!failedParamNames.contains(paramPath)) { Object value = args[i]; - getBindingResult().recordFieldValue(paramName, paramTypes[i], value); - validateArgument(ctor.getDeclaringClass(), paramName, value); + getBindingResult().recordFieldValue(paramPath, paramTypes[i], value); + validateConstructorArgument(ctor.getDeclaringClass(), nestedPath, paramNames[i], value); } } - if (!(this.targetType.getSource() instanceof MethodParameter param && param.isOptional())) { + if (!(objectType.getSource() instanceof MethodParameter param && param.isOptional())) { try { - this.target = BeanUtils.instantiateClass(ctor, args); + result = BeanUtils.instantiateClass(ctor, args); } catch (BeanInstantiationException ex) { // swallow and proceed without target instance } } - return; - } - - try { - this.target = BeanUtils.instantiateClass(ctor, args); } - catch (BeanInstantiationException ex) { - if (KotlinDetector.isKotlinType(clazz) && ex.getCause() instanceof NullPointerException cause) { - ObjectError error = new ObjectError(ctor.getName(), cause.getMessage()); - getBindingResult().addError(error); - return; + else { + try { + result = BeanUtils.instantiateClass(ctor, args); + } + catch (BeanInstantiationException ex) { + if (KotlinDetector.isKotlinType(clazz) && ex.getCause() instanceof NullPointerException cause) { + ObjectError error = new ObjectError(ctor.getName(), cause.getMessage()); + getBindingResult().addError(error); + } + else { + throw ex; + } } - throw ex; } } - // Now that target is set, add PropertyEditor's to PropertyAccessor - if (this.typeConverter != null) { - this.typeConverter.registerCustomEditors(getPropertyAccessor()); - } + return (isOptional && !nestedPath.isEmpty() ? Optional.ofNullable(result) : result); } - private void validateArgument(Class constructorClass, String name, @Nullable Object value) { - Object[] validationHints = null; + private void validateConstructorArgument( + Class constructorClass, String nestedPath, String name, @Nullable Object value) { + + Object[] hints = null; if (this.targetType.getSource() instanceof MethodParameter parameter) { for (Annotation ann : parameter.getParameterAnnotations()) { - validationHints = ValidationAnnotationUtils.determineValidationHints(ann); - if (validationHints != null) { + hints = ValidationAnnotationUtils.determineValidationHints(ann); + if (hints != null) { break; } } } - if (validationHints == null) { + if (hints == null) { return; } for (Validator validator : getValidatorsToApply()) { if (validator instanceof SmartValidator smartValidator) { + boolean isNested = !nestedPath.isEmpty(); + if (isNested) { + getBindingResult().pushNestedPath(nestedPath.substring(0, nestedPath.length() - 1)); + } try { - smartValidator.validateValue( - constructorClass, name, value, getBindingResult(), validationHints); + smartValidator.validateValue(constructorClass, name, value, getBindingResult(), hints); } catch (IllegalArgumentException ex) { // No corresponding field on the target class... } + if (isNested) { + getBindingResult().popNestedPath(); + } } } } 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 fa53737fb80a..73eff3ad6028 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 @@ -2252,6 +2252,112 @@ void dataRecordBinding(boolean usePathPatterns) throws Exception { assertThat(response.getContentAsString()).isEqualTo("value1-true-3"); } + @PathPatternsParameterizedTest + void nestedDataClassBinding(boolean usePathPatterns) throws Exception { + initDispatcherServlet(NestedDataClassController.class, usePathPatterns); + + MockHttpServletRequest request = new MockHttpServletRequest("GET", "/bind"); + request.addParameter("param1", "value1"); + request.addParameter("nestedParam2.param1", "nestedValue1"); + request.addParameter("nestedParam2.param2", "true"); + MockHttpServletResponse response = new MockHttpServletResponse(); + getServlet().service(request, response); + assertThat(response.getContentAsString()).isEqualTo("value1-nestedValue1-true-0"); + } + + @PathPatternsParameterizedTest + void nestedDataClassBindingWithAdditionalSetter(boolean usePathPatterns) throws Exception { + initDispatcherServlet(NestedDataClassController.class, usePathPatterns); + + MockHttpServletRequest request = new MockHttpServletRequest("GET", "/bind"); + request.addParameter("param1", "value1"); + request.addParameter("nestedParam2.param1", "nestedValue1"); + request.addParameter("nestedParam2.param2", "true"); + request.addParameter("nestedParam2.param3", "3"); + MockHttpServletResponse response = new MockHttpServletResponse(); + getServlet().service(request, response); + assertThat(response.getContentAsString()).isEqualTo("value1-nestedValue1-true-3"); + } + + @PathPatternsParameterizedTest + void nestedDataClassBindingWithOptionalParameter(boolean usePathPatterns) throws Exception { + initDispatcherServlet(NestedValidatedDataClassController.class, usePathPatterns); + + MockHttpServletRequest request = new MockHttpServletRequest("GET", "/bind"); + request.addParameter("param1", "value1"); + request.addParameter("nestedParam2.param1", "nestedValue1"); + request.addParameter("nestedParam2.param2", "true"); + request.addParameter("nestedParam2.optionalParam", "8"); + MockHttpServletResponse response = new MockHttpServletResponse(); + getServlet().service(request, response); + assertThat(response.getContentAsString()).isEqualTo("value1-nestedValue1-true-8"); + } + + @PathPatternsParameterizedTest + void nestedDataClassBindingWithMissingParameter(boolean usePathPatterns) throws Exception { + initDispatcherServlet(NestedValidatedDataClassController.class, usePathPatterns); + + MockHttpServletRequest request = new MockHttpServletRequest("GET", "/bind"); + request.addParameter("param1", "value1"); + request.addParameter("nestedParam2.param1", "nestedValue1"); + MockHttpServletResponse response = new MockHttpServletResponse(); + getServlet().service(request, response); + assertThat(response.getContentAsString()).isEqualTo("1:value1-nestedValue1-null-null"); + } + + @PathPatternsParameterizedTest + void nestedDataClassBindingWithConversionError(boolean usePathPatterns) throws Exception { + initDispatcherServlet(NestedValidatedDataClassController.class, usePathPatterns); + + MockHttpServletRequest request = new MockHttpServletRequest("GET", "/bind"); + request.addParameter("param1", "value1"); + request.addParameter("nestedParam2.param1", "nestedValue1"); + request.addParameter("nestedParam2.param2", "x"); + MockHttpServletResponse response = new MockHttpServletResponse(); + getServlet().service(request, response); + assertThat(response.getContentAsString()).isEqualTo("1:value1-nestedValue1-x-null"); + } + + @PathPatternsParameterizedTest + void nestedDataClassBindingWithValidationError(boolean usePathPatterns) throws Exception { + initDispatcherServlet(NestedValidatedDataClassController.class, usePathPatterns); + + MockHttpServletRequest request = new MockHttpServletRequest("GET", "/bind"); + request.addParameter("param1", "value1"); + request.addParameter("nestedParam2.param2", "true"); + request.addParameter("nestedParam2.param3", "0"); + MockHttpServletResponse response = new MockHttpServletResponse(); + getServlet().service(request, response); + assertThat(response.getContentAsString()).isEqualTo("1:value1--true-0"); + } + + @PathPatternsParameterizedTest + void nestedDataClassBindingWithValidationErrorAndConversionError(boolean usePathPatterns) throws Exception { + initDispatcherServlet(NestedValidatedDataClassController.class, usePathPatterns); + + MockHttpServletRequest request = new MockHttpServletRequest("GET", "/bind"); + request.addParameter("param1", "value1"); + request.addParameter("nestedParam2.param2", "x"); + MockHttpServletResponse response = new MockHttpServletResponse(); + getServlet().service(request, response); + assertThat(response.getContentAsString()).isEqualTo("2:value1-null-x-null"); + } + + @PathPatternsParameterizedTest + void nestedDataClassBindingWithDataAndLocalDate(boolean usePathPatterns) throws Exception { + initDispatcherServlet(NestedDataAndDateClassController.class, usePathPatterns); + + MockHttpServletRequest request = new MockHttpServletRequest("GET", "/bind"); + request.addParameter("param1", "value1"); + request.addParameter("nestedParam2.param1", "nestedValue1"); + request.addParameter("nestedParam2.param2", "true"); + request.addParameter("nestedParam2.optionalParam", "8"); + request.addParameter("nestedParam3.date", "2010-01-01"); + MockHttpServletResponse response = new MockHttpServletResponse(); + getServlet().service(request, response); + assertThat(response.getContentAsString()).isEqualTo("2010-01-01"); + } + @Test void routerFunction() throws ServletException, IOException { GenericWebApplicationContext wac = new GenericWebApplicationContext(); @@ -4164,7 +4270,7 @@ public String handle(DateClass data, BindingResult result) { } } - static record DataRecord(String param1, boolean param2, int param3) { + record DataRecord(String param1, boolean param2, int param3) { } @RestController @@ -4176,4 +4282,145 @@ public String handle(DataRecord data) { } } + static class NestedDataClass { + + @NotNull + private final String param1; + + @Valid + private final DataClass nestedParam2; + + public NestedDataClass(@NotNull String param1, DataClass nestedParam2) { + this.param1 = param1; + this.nestedParam2 = nestedParam2; + } + + public String getParam1() { + return this.param1; + } + + public DataClass getNestedParam2() { + return this.nestedParam2; + } + } + + @RestController + static class NestedDataClassController { + + @RequestMapping("/bind") + public String handle(NestedDataClass data) { + DataClass nestedParam2 = data.nestedParam2; + return (data.param1 + "-" + nestedParam2.param1 + "-" + nestedParam2.param2 + "-" + nestedParam2.param3); + } + } + + @RestController + static class NestedValidatedDataClassController { + + @InitBinder + public void initBinder(WebDataBinder binder) { + binder.setConversionService(new DefaultFormattingConversionService()); + binder.registerCustomEditor(String.class, new StringTrimmerEditor(true)); + LocalValidatorFactoryBean vf = new LocalValidatorFactoryBean(); + vf.afterPropertiesSet(); + binder.setValidator(vf); + } + + @RequestMapping("/bind") + public NestedBindStatusView handle(@Valid NestedDataClass data, BindingResult result) { + assertThat(data).isNotNull(); + if (result.hasErrors()) { + String content = result.getErrorCount() + ":" + result.getFieldValue("param1"); + content += "-" + result.getFieldValue("nestedParam2.param1"); + content += "-" + result.getFieldValue("nestedParam2.param2"); + content += "-" + result.getFieldValue("nestedParam2.param3"); + return new NestedBindStatusView(content); + } + DataClass nested = data.nestedParam2; + return new NestedBindStatusView( + data.param1 + "-" + nested.param1 + "-" + nested.param2 + "-" + nested.param3); + } + } + + static class NestedBindStatusView extends AbstractView { + + private final String content; + + NestedBindStatusView(String content) { + this.content = content; + } + + @Override + protected void renderMergedOutputModel( + Map model, HttpServletRequest request, HttpServletResponse response) throws Exception { + RequestContext rc = new RequestContext(request, model); + rc.getBindStatus("nestedDataClass"); + rc.getBindStatus("nestedDataClass.param1"); + rc.getBindStatus("nestedDataClass.nestedParam2"); + rc.getBindStatus("nestedDataClass.nestedParam2.param1"); + rc.getBindStatus("nestedDataClass.nestedParam2.param2"); + rc.getBindStatus("nestedDataClass.nestedParam2.param3"); + response.getWriter().write(this.content); + } + } + + static class NestedDataAndDateClass { + + @NotNull + private final String param1; + + @Valid + private final DataClass nestedParam2; + + @Valid + private final DateClass nestedParam3; + + public NestedDataAndDateClass( + @NotNull String param1, DataClass nestedParam2, DateClass nestedParam3) { + + this.param1 = param1; + this.nestedParam2 = nestedParam2; + this.nestedParam3 = nestedParam3; + } + + public String getParam1() { + return this.param1; + } + + public DataClass getNestedParam2() { + return this.nestedParam2; + } + + public DateClass getNestedParam3() { + return this.nestedParam3; + } + } + + @RestController + static class NestedDataAndDateClassController { + + @InitBinder + public void initBinder(WebDataBinder binder) { + binder.initDirectFieldAccess(); + binder.setConversionService(new DefaultFormattingConversionService()); + } + + @RequestMapping("/bind") + public String handle(NestedDataAndDateClass data, BindingResult result) { + if (result.hasErrors()) { + return result.getFieldError().toString(); + } + assertThat(data).isNotNull(); + assertThat(data.getParam1()).isEqualTo("value1"); + assertThat(data.getNestedParam2().param1).isEqualTo("nestedValue1"); + assertThat(data.getNestedParam2().param2).isTrue(); + assertThat(data.getNestedParam2().param3).isEqualTo(8); + assertThat(data.getNestedParam3().date).isNotNull(); + assertThat(data.getNestedParam3().date.getYear()).isEqualTo(2010); + assertThat(data.getNestedParam3().date.getMonthValue()).isEqualTo(1); + assertThat(data.getNestedParam3().date.getDayOfMonth()).isEqualTo(1); + return result.getFieldValue("nestedParam3.date").toString(); + } + } + }