From cfc821d1799ca7c64b1bbc53811b712fdaa4776c Mon Sep 17 00:00:00 2001 From: Juergen Hoeller Date: Thu, 25 Sep 2014 17:00:36 +0200 Subject: [PATCH] DataBinder unwraps Optional objects and allows for proper handling of Optional.empty() Issue: SPR-12241 --- .../beans/BeanWrapperImpl.java | 16 ++-- .../validation/DataBinder.java | 41 +++++++- .../ModelAttributeMethodProcessor.java | 38 ++++---- ...etModelAttributeMethodProcessorTests.java} | 93 +++++++++++++++++-- 4 files changed, 151 insertions(+), 37 deletions(-) rename spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/{SerlvetModelAttributeMethodProcessorTests.java => ServletModelAttributeMethodProcessorTests.java} (61%) diff --git a/spring-beans/src/main/java/org/springframework/beans/BeanWrapperImpl.java b/spring-beans/src/main/java/org/springframework/beans/BeanWrapperImpl.java index 56af0f8c61..98e4ba0397 100644 --- a/spring-beans/src/main/java/org/springframework/beans/BeanWrapperImpl.java +++ b/spring-beans/src/main/java/org/springframework/beans/BeanWrapperImpl.java @@ -568,11 +568,10 @@ public class BeanWrapperImpl extends AbstractPropertyAccessor implements BeanWra // Get value of bean property. PropertyTokenHolder tokens = getPropertyNameTokens(nestedProperty); String canonicalName = tokens.canonicalName; - Object propertyValue = getPropertyValue(tokens); - if (propertyValue == null || - (propertyValue.getClass().equals(javaUtilOptionalClass) && OptionalUnwrapper.isEmpty(propertyValue))) { + Object value = getPropertyValue(tokens); + if (value == null || (value.getClass().equals(javaUtilOptionalClass) && OptionalUnwrapper.isEmpty(value))) { if (isAutoGrowNestedPaths()) { - propertyValue = setDefaultValue(tokens); + value = setDefaultValue(tokens); } else { throw new NullValueInNestedPathException(getRootClass(), this.nestedPath + canonicalName); @@ -581,11 +580,12 @@ public class BeanWrapperImpl extends AbstractPropertyAccessor implements BeanWra // Lookup cached sub-BeanWrapper, create new one if not found. BeanWrapperImpl nestedBw = this.nestedBeanWrappers.get(canonicalName); - if (nestedBw == null || nestedBw.getWrappedInstance() != propertyValue) { + if (nestedBw == null || nestedBw.getWrappedInstance() != + (value.getClass().equals(javaUtilOptionalClass) ? OptionalUnwrapper.unwrap(value) : value)) { if (logger.isTraceEnabled()) { logger.trace("Creating new nested BeanWrapper for property '" + canonicalName + "'"); } - nestedBw = newNestedBeanWrapper(propertyValue, this.nestedPath + canonicalName + NESTED_PROPERTY_SEPARATOR); + nestedBw = newNestedBeanWrapper(value, this.nestedPath + canonicalName + NESTED_PROPERTY_SEPARATOR); // Inherit all type-specific PropertyEditors. copyDefaultEditorsTo(nestedBw); copyCustomEditorsTo(nestedBw, canonicalName); @@ -1212,7 +1212,9 @@ public class BeanWrapperImpl extends AbstractPropertyAccessor implements BeanWra public static Object unwrap(Object optionalObject) { Optional optional = (Optional) optionalObject; Assert.isTrue(optional.isPresent(), "Optional value must be present"); - return optional.get(); + Object result = optional.get(); + Assert.isTrue(!(result instanceof Optional), "Multi-level Optional usage not supported"); + return result; } public static boolean isEmpty(Object optionalObject) { 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 1b2db14377..6c19ce712f 100644 --- a/spring-context/src/main/java/org/springframework/validation/DataBinder.java +++ b/spring-context/src/main/java/org/springframework/validation/DataBinder.java @@ -24,6 +24,7 @@ import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.Optional; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; @@ -41,7 +42,9 @@ import org.springframework.beans.TypeConverter; import org.springframework.beans.TypeMismatchException; import org.springframework.core.MethodParameter; import org.springframework.core.convert.ConversionService; +import org.springframework.lang.UsesJava8; import org.springframework.util.Assert; +import org.springframework.util.ClassUtils; import org.springframework.util.ObjectUtils; import org.springframework.util.PatternMatchUtils; import org.springframework.util.StringUtils; @@ -119,6 +122,19 @@ public class DataBinder implements PropertyEditorRegistry, TypeConverter { */ protected static final Log logger = LogFactory.getLog(DataBinder.class); + private static Class javaUtilOptionalClass = null; + + static { + try { + javaUtilOptionalClass = + ClassUtils.forName("java.util.Optional", DataBinder.class.getClassLoader()); + } + catch (ClassNotFoundException ex) { + // Java 8 not available - Optional references simply not supported then. + } + } + + private final Object target; private final String objectName; @@ -165,7 +181,12 @@ public class DataBinder implements PropertyEditorRegistry, TypeConverter { * @param objectName the name of the target object */ public DataBinder(Object target, String objectName) { - this.target = target; + if (target != null && target.getClass().equals(javaUtilOptionalClass)) { + this.target = OptionalUnwrapper.unwrap(target); + } + else { + this.target = target; + } this.objectName = objectName; } @@ -779,4 +800,22 @@ public class DataBinder implements PropertyEditorRegistry, TypeConverter { return getBindingResult().getModel(); } + + /** + * Inner class to avoid a hard dependency on Java 8. + */ + @UsesJava8 + private static class OptionalUnwrapper { + + public static Object unwrap(Object optionalObject) { + Optional optional = (Optional) optionalObject; + if (!optional.isPresent()) { + return null; + } + Object result = optional.get(); + Assert.isTrue(!(result instanceof Optional), "Multi-level Optional usage not supported"); + return result; + } + } + } 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 aee03bef14..2ecd2c8173 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 @@ -1,5 +1,5 @@ /* - * Copyright 2002-2012 the original author or authors. + * Copyright 2002-2014 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. @@ -21,6 +21,7 @@ import java.util.Map; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; + import org.springframework.beans.BeanUtils; import org.springframework.core.MethodParameter; import org.springframework.core.annotation.AnnotationUtils; @@ -58,6 +59,7 @@ public class ModelAttributeMethodProcessor implements HandlerMethodArgumentResol private final boolean annotationNotRequired; + /** * @param annotationNotRequired if "true", non-simple method arguments and * return values are considered model attributes with or without a @@ -67,6 +69,7 @@ public class ModelAttributeMethodProcessor implements HandlerMethodArgumentResol this.annotationNotRequired = annotationNotRequired; } + /** * @return true if the parameter is annotated with {@link ModelAttribute} * or in default resolution mode also if it is not a simple type. @@ -94,18 +97,16 @@ public class ModelAttributeMethodProcessor implements HandlerMethodArgumentResol * @throws Exception if WebDataBinder initialization fails. */ @Override - public final Object resolveArgument( - MethodParameter parameter, ModelAndViewContainer mavContainer, - NativeWebRequest request, WebDataBinderFactory binderFactory) - throws Exception { + public final Object resolveArgument(MethodParameter parameter, ModelAndViewContainer mavContainer, + NativeWebRequest webRequest, WebDataBinderFactory binderFactory) throws Exception { String name = ModelFactory.getNameForParameter(parameter); - Object attribute = (mavContainer.containsAttribute(name)) ? - mavContainer.getModel().get(name) : createAttribute(name, parameter, binderFactory, request); + Object attribute = (mavContainer.containsAttribute(name) ? + mavContainer.getModel().get(name) : createAttribute(name, parameter, binderFactory, webRequest)); - WebDataBinder binder = binderFactory.createBinder(request, attribute, name); + WebDataBinder binder = binderFactory.createBinder(webRequest, attribute, name); if (binder.getTarget() != null) { - bindRequestParameters(binder, request); + bindRequestParameters(binder, webRequest); validateIfApplicable(binder, parameter); if (binder.getBindingResult().hasErrors()) { if (isBindExceptionRequired(binder, parameter)) { @@ -120,17 +121,17 @@ public class ModelAttributeMethodProcessor implements HandlerMethodArgumentResol mavContainer.removeAttributes(bindingResultModel); mavContainer.addAllAttributes(bindingResultModel); - return binder.getTarget(); + return binder.convertIfNecessary(binder.getTarget(), parameter.getParameterType(), parameter); } /** * Extension point to create the model attribute if not found in the model. * The default implementation uses the default constructor. - * @param attributeName the name of the attribute, never {@code null} + * @param attributeName the name of the attribute (never {@code null}) * @param parameter the method parameter * @param binderFactory for creating WebDataBinder instance * @param request the current request - * @return the created model attribute, never {@code null} + * @return the created model attribute (never {@code null}) */ protected Object createAttribute(String attributeName, MethodParameter parameter, WebDataBinderFactory binderFactory, NativeWebRequest request) throws Exception { @@ -155,9 +156,9 @@ public class ModelAttributeMethodProcessor implements HandlerMethodArgumentResol */ protected void validateIfApplicable(WebDataBinder binder, MethodParameter parameter) { Annotation[] annotations = parameter.getParameterAnnotations(); - for (Annotation annot : annotations) { - if (annot.annotationType().getSimpleName().startsWith("Valid")) { - Object hints = AnnotationUtils.getValue(annot); + for (Annotation ann : annotations) { + if (ann.annotationType().getSimpleName().startsWith("Valid")) { + Object hints = AnnotationUtils.getValue(ann); binder.validate(hints instanceof Object[] ? (Object[]) hints : new Object[] {hints}); break; } @@ -199,14 +200,13 @@ public class ModelAttributeMethodProcessor implements HandlerMethodArgumentResol * Add non-null return values to the {@link ModelAndViewContainer}. */ @Override - public void handleReturnValue( - Object returnValue, MethodParameter returnType, - ModelAndViewContainer mavContainer, NativeWebRequest webRequest) - throws Exception { + public void handleReturnValue(Object returnValue, MethodParameter returnType, + ModelAndViewContainer mavContainer, NativeWebRequest webRequest) throws Exception { if (returnValue != null) { String name = ModelFactory.getNameForReturnValue(returnValue, returnType); mavContainer.addAttribute(name, returnValue); } } + } diff --git a/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/SerlvetModelAttributeMethodProcessorTests.java b/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/ServletModelAttributeMethodProcessorTests.java similarity index 61% rename from spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/SerlvetModelAttributeMethodProcessorTests.java rename to spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/ServletModelAttributeMethodProcessorTests.java index 26e95d0c33..d5f794f97b 100644 --- a/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/SerlvetModelAttributeMethodProcessorTests.java +++ b/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/ServletModelAttributeMethodProcessorTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2013 the original author or authors. + * Copyright 2002-2014 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. @@ -16,19 +16,18 @@ package org.springframework.web.servlet.mvc.method.annotation; -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertNotNull; - import java.lang.reflect.Method; import java.util.HashMap; import java.util.Map; +import java.util.Optional; import org.junit.Before; import org.junit.Test; -import org.springframework.tests.sample.beans.TestBean; + import org.springframework.core.MethodParameter; import org.springframework.core.convert.support.DefaultConversionService; import org.springframework.mock.web.test.MockHttpServletRequest; +import org.springframework.tests.sample.beans.TestBean; import org.springframework.web.bind.annotation.ModelAttribute; import org.springframework.web.bind.support.ConfigurableWebBindingInitializer; import org.springframework.web.bind.support.WebDataBinderFactory; @@ -37,13 +36,15 @@ import org.springframework.web.context.request.ServletWebRequest; import org.springframework.web.method.support.ModelAndViewContainer; import org.springframework.web.servlet.HandlerMapping; +import static org.junit.Assert.*; + /** * Test fixture for {@link ServletModelAttributeMethodProcessor} specific tests. * Also see org.springframework.web.method.annotation.support.ModelAttributeMethodProcessorTests * * @author Rossen Stoyanchev */ -public class SerlvetModelAttributeMethodProcessorTests { +public class ServletModelAttributeMethodProcessorTests { private ServletModelAttributeMethodProcessor processor; @@ -51,6 +52,8 @@ public class SerlvetModelAttributeMethodProcessorTests { private MethodParameter testBeanWithoutStringConstructorModelAttr; + private MethodParameter testBeanWithOptionalModelAttr; + private ModelAndViewContainer mavContainer; private NativeWebRequest webRequest; @@ -59,26 +62,29 @@ public class SerlvetModelAttributeMethodProcessorTests { private WebDataBinderFactory binderFactory; + @Before public void setUp() throws Exception { this.processor = new ServletModelAttributeMethodProcessor(false); Method method = getClass().getDeclaredMethod("modelAttribute", - TestBean.class, TestBeanWithoutStringConstructor.class); + TestBean.class, TestBeanWithoutStringConstructor.class, Optional.class); this.testBeanModelAttr = new MethodParameter(method, 0); this.testBeanWithoutStringConstructorModelAttr = new MethodParameter(method, 1); + this.testBeanWithOptionalModelAttr = new MethodParameter(method, 2); ConfigurableWebBindingInitializer initializer = new ConfigurableWebBindingInitializer(); initializer.setConversionService(new DefaultConversionService()); - this.binderFactory = new ServletRequestDataBinderFactory(null, initializer ); + this.binderFactory = new ServletRequestDataBinderFactory(null, initializer); this.mavContainer = new ModelAndViewContainer(); this.request = new MockHttpServletRequest(); this.webRequest = new ServletWebRequest(request); } + @Test public void createAttributeUriTemplateVar() throws Exception { Map uriTemplateVars = new HashMap(); @@ -107,6 +113,21 @@ public class SerlvetModelAttributeMethodProcessorTests { assertNotNull(testBean); } + @Test + public void createAttributeUriTemplateVarWithOptional() throws Exception { + Map uriTemplateVars = new HashMap(); + uriTemplateVars.put("testBean3", "Patty"); + this.request.setAttribute(HandlerMapping.URI_TEMPLATE_VARIABLES_ATTRIBUTE, uriTemplateVars); + + // Type conversion from "Patty" to TestBean via TestBean(String) constructor + + Optional testBean = + (Optional) this.processor.resolveArgument( + this.testBeanWithOptionalModelAttr, this.mavContainer, this.webRequest, this.binderFactory); + + assertEquals("Patty", testBean.get().getName()); + } + @Test public void createAttributeRequestParameter() throws Exception { this.request.addParameter("testBean1", "Patty"); @@ -122,7 +143,7 @@ public class SerlvetModelAttributeMethodProcessorTests { @Test public void createAttributeRequestParameterCannotConvert() throws Exception { - this.request.addParameter("testBean1", "Patty"); + this.request.addParameter("testBean2", "Patty"); TestBeanWithoutStringConstructor testBean = (TestBeanWithoutStringConstructor) this.processor.resolveArgument( @@ -131,10 +152,62 @@ public class SerlvetModelAttributeMethodProcessorTests { assertNotNull(testBean); } + @Test + public void createAttributeRequestParameterWithOptional() throws Exception { + this.request.addParameter("testBean3", "Patty"); + + Optional testBean = + (Optional) this.processor.resolveArgument( + this.testBeanWithOptionalModelAttr, this.mavContainer, this.webRequest, this.binderFactory); + + assertEquals("Patty", testBean.get().getName()); + } + + @Test + public void attributesAsNullValues() throws Exception { + this.request.addParameter("name", "Patty"); + + this.mavContainer.getModel().put("testBean1", null); + this.mavContainer.getModel().put("testBean2", null); + this.mavContainer.getModel().put("testBean3", null); + + assertNull(this.processor.resolveArgument( + this.testBeanModelAttr, this.mavContainer, this.webRequest, this.binderFactory)); + + assertNull(this.processor.resolveArgument( + this.testBeanWithoutStringConstructorModelAttr, this.mavContainer, this.webRequest, this.binderFactory)); + + Optional testBean = + (Optional) this.processor.resolveArgument( + this.testBeanWithOptionalModelAttr, this.mavContainer, this.webRequest, this.binderFactory); + assertFalse(testBean.isPresent()); + } + + @Test + public void attributesAsOptionalEmpty() throws Exception { + this.request.addParameter("name", "Patty"); + + this.mavContainer.getModel().put("testBean1", Optional.empty()); + this.mavContainer.getModel().put("testBean2", Optional.empty()); + this.mavContainer.getModel().put("testBean3", Optional.empty()); + + assertNull(this.processor.resolveArgument( + this.testBeanModelAttr, this.mavContainer, this.webRequest, this.binderFactory)); + + assertNull(this.processor.resolveArgument( + this.testBeanWithoutStringConstructorModelAttr, this.mavContainer, this.webRequest, this.binderFactory)); + + Optional testBean = + (Optional) this.processor.resolveArgument( + this.testBeanWithOptionalModelAttr, this.mavContainer, this.webRequest, this.binderFactory); + assertFalse(testBean.isPresent()); + } + @SuppressWarnings("unused") private void modelAttribute(@ModelAttribute("testBean1") TestBean testBean1, - @ModelAttribute("testBean2") TestBeanWithoutStringConstructor testBean2) { + @ModelAttribute("testBean2") TestBeanWithoutStringConstructor testBean2, + @ModelAttribute("testBean3") Optional testBean3) { }