BeanFactory does not unwrap java.util.Optional for top-level bean

jhoeller · jhoeller · commit 5c1d3fca15d9 · 2016-04-07T14:18:30.000+02:00
Issue: SPR-14121
diff --git a/spring-beans/src/main/java/org/springframework/beans/AbstractNestablePropertyAccessor.java b/spring-beans/src/main/java/org/springframework/beans/AbstractNestablePropertyAccessor.java
@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2015 the original author or authors.
+ * Copyright 2002-2016 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.
@@ -87,10 +87,10 @@ public abstract class AbstractNestablePropertyAccessor extends AbstractPropertyA
 		}
 	}
 
+
 	private int autoGrowCollectionLimit = Integer.MAX_VALUE;
 
-	/** The wrapped object */
-	private Object object;
+	Object wrappedObject;
 
 	private String nestedPath = "";
 
@@ -204,23 +204,23 @@ public void setWrappedInstance(Object object) {
 	public void setWrappedInstance(Object object, String nestedPath, Object rootObject) {
 		Assert.notNull(object, "Target object must not be null");
 		if (object.getClass() == javaUtilOptionalClass) {
-			this.object = OptionalUnwrapper.unwrap(object);
+			this.wrappedObject = OptionalUnwrapper.unwrap(object);
 		}
 		else {
-			this.object = object;
+			this.wrappedObject = object;
 		}
 		this.nestedPath = (nestedPath != null ? nestedPath : "");
-		this.rootObject = (!"".equals(this.nestedPath) ? rootObject : this.object);
+		this.rootObject = (!"".equals(this.nestedPath) ? rootObject : this.wrappedObject);
 		this.nestedPropertyAccessors = null;
-		this.typeConverterDelegate = new TypeConverterDelegate(this, this.object);
+		this.typeConverterDelegate = new TypeConverterDelegate(this, this.wrappedObject);
 	}
 
 	public final Object getWrappedInstance() {
-		return this.object;
+		return this.wrappedObject;
 	}
 
 	public final Class<?> getWrappedClass() {
-		return (this.object != null ? this.object.getClass() : null);
+		return (this.wrappedObject != null ? this.wrappedObject.getClass() : null);
 	}
 
 	/**
@@ -303,7 +303,7 @@ protected void setPropertyValue(PropertyTokenHolder tokens, PropertyValue pv) th
 			catch (NotReadablePropertyException ex) {
 				throw new NotWritablePropertyException(getRootClass(), this.nestedPath + propertyName,
 						"Cannot access indexed value in property referenced " +
-								"in indexed property path '" + propertyName + "'", ex);
+						"in indexed property path '" + propertyName + "'", ex);
 			}
 			// Set value for last key.
 			String key = tokens.keys[tokens.keys.length - 1];
@@ -318,7 +318,7 @@ protected void setPropertyValue(PropertyTokenHolder tokens, PropertyValue pv) th
 				else {
 					throw new NullValueInNestedPathException(getRootClass(), this.nestedPath + propertyName,
 							"Cannot access indexed value in property referenced " +
-									"in indexed property path '" + propertyName + "': returned null");
+							"in indexed property path '" + propertyName + "': returned null");
 				}
 			}
 			if (propValue.getClass().isArray()) {
@@ -367,8 +367,8 @@ else if (propValue instanceof List) {
 						catch (NullPointerException ex) {
 							throw new InvalidPropertyException(getRootClass(), this.nestedPath + propertyName,
 									"Cannot set element with index " + index + " in List of size " +
-											size + ", accessed using property path '" + propertyName +
-											"': List does not support filling up gaps with null elements");
+									size + ", accessed using property path '" + propertyName +
+									"': List does not support filling up gaps with null elements");
 						}
 					}
 					list.add(convertedValue);
@@ -405,7 +405,7 @@ else if (propValue instanceof Map) {
 			else {
 				throw new InvalidPropertyException(getRootClass(), this.nestedPath + propertyName,
 						"Property referenced in indexed property path '" + propertyName +
-								"' is neither an array nor a List nor a Map; returned value was [" + propValue + "]");
+						"' is neither an array nor a List nor a Map; returned value was [" + propValue + "]");
 			}
 		}
 
@@ -451,7 +451,7 @@ else if (propValue instanceof Map) {
 					}
 					pv.getOriginalPropertyValue().conversionNecessary = (valueToApply != originalValue);
 				}
-				ph.setValue(object, valueToApply);
+				ph.setValue(this.wrappedObject, valueToApply);
 			}
 			catch (TypeMismatchException ex) {
 				throw ex;
@@ -953,10 +953,9 @@ private PropertyTokenHolder getPropertyNameTokens(String propertyName) {
 		tokens.actualName = (actualName != null ? actualName : propertyName);
 		tokens.canonicalName = tokens.actualName;
 		if (!keys.isEmpty()) {
-			tokens.canonicalName +=
-					PROPERTY_KEY_PREFIX +
-							StringUtils.collectionToDelimitedString(keys, PROPERTY_KEY_SUFFIX + PROPERTY_KEY_PREFIX) +
-							PROPERTY_KEY_SUFFIX;
+			tokens.canonicalName += PROPERTY_KEY_PREFIX +
+					StringUtils.collectionToDelimitedString(keys, PROPERTY_KEY_SUFFIX + PROPERTY_KEY_PREFIX) +
+					PROPERTY_KEY_SUFFIX;
 			tokens.keys = StringUtils.toStringArray(keys);
 		}
 		return tokens;
@@ -965,8 +964,8 @@ private PropertyTokenHolder getPropertyNameTokens(String propertyName) {
 	@Override
 	public String toString() {
 		StringBuilder sb = new StringBuilder(getClass().getName());
-		if (this.object != null) {
-			sb.append(": wrapping object [").append(ObjectUtils.identityToString(this.object)).append("]");
+		if (this.wrappedObject != null) {
+			sb.append(": wrapping object [").append(ObjectUtils.identityToString(this.wrappedObject)).append("]");
 		}
 		else {
 			sb.append(": no wrapped object set");
diff --git a/spring-beans/src/main/java/org/springframework/beans/BeanWrapperImpl.java b/spring-beans/src/main/java/org/springframework/beans/BeanWrapperImpl.java
@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2015 the original author or authors.
+ * Copyright 2002-2016 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.
@@ -133,26 +133,22 @@ private BeanWrapperImpl(Object object, String nestedPath, BeanWrapperImpl parent
 	}
 
 
-	@Override
-	public void setWrappedInstance(Object object, String nestedPath, Object rootObject) {
-		super.setWrappedInstance(object, nestedPath, rootObject);
-		setIntrospectionClass(getWrappedInstance().getClass());
-	}
-
 	/**
-	 * Set the security context used during the invocation of the wrapped instance methods.
-	 * Can be null.
+	 * Set a bean instance to hold, without any unwrapping of {@link java.util.Optional}.
+	 * @param object the actual target object
+	 * @since 4.3
+	 * @see #setWrappedInstance(Object)
 	 */
-	public void setSecurityContext(AccessControlContext acc) {
-		this.acc = acc;
+	public void setBeanInstance(Object object) {
+		this.wrappedObject = object;
+		this.typeConverterDelegate = new TypeConverterDelegate(this, this.wrappedObject);
+		setIntrospectionClass(object.getClass());
 	}
 
-	/**
-	 * Return the security context used during the invocation of the wrapped instance methods.
-	 * Can be null.
-	 */
-	public AccessControlContext getSecurityContext() {
-		return this.acc;
+	@Override
+	public void setWrappedInstance(Object object, String nestedPath, Object rootObject) {
+		super.setWrappedInstance(object, nestedPath, rootObject);
+		setIntrospectionClass(getWrappedClass());
 	}
 
 	/**
@@ -161,8 +157,7 @@ public AccessControlContext getSecurityContext() {
 	 * @param clazz the class to introspect
 	 */
 	protected void setIntrospectionClass(Class<?> clazz) {
-		if (this.cachedIntrospectionResults != null &&
-				!clazz.equals(this.cachedIntrospectionResults.getBeanClass())) {
+		if (this.cachedIntrospectionResults != null && this.cachedIntrospectionResults.getBeanClass() != clazz) {
 			this.cachedIntrospectionResults = null;
 		}
 	}
@@ -179,6 +174,22 @@ private CachedIntrospectionResults getCachedIntrospectionResults() {
 		return this.cachedIntrospectionResults;
 	}
 
+	/**
+	 * Set the security context used during the invocation of the wrapped instance methods.
+	 * Can be null.
+	 */
+	public void setSecurityContext(AccessControlContext acc) {
+		this.acc = acc;
+	}
+
+	/**
+	 * Return the security context used during the invocation of the wrapped instance methods.
+	 * Can be null.
+	 */
+	public AccessControlContext getSecurityContext() {
+		return this.acc;
+	}
+
 
 	/**
 	 * Convert the given value for the specified property to the latter's type.
diff --git a/spring-beans/src/main/java/org/springframework/beans/factory/support/ConstructorResolver.java b/spring-beans/src/main/java/org/springframework/beans/factory/support/ConstructorResolver.java
@@ -272,7 +272,7 @@ public Object run() {
 						mbd, beanName, this.beanFactory, constructorToUse, argsToUse);
 			}
 
-			bw.setWrappedInstance(beanInstance);
+			bw.setBeanInstance(beanInstance);
 			return bw;
 		}
 		catch (Throwable ex) {
@@ -592,7 +592,7 @@ public Object run() {
 			if (beanInstance == null) {
 				return null;
 			}
-			bw.setWrappedInstance(beanInstance);
+			bw.setBeanInstance(beanInstance);
 			return bw;
 		}
 		catch (Throwable ex) {
diff --git a/spring-beans/src/test/java/org/springframework/beans/factory/DefaultListableBeanFactoryTests.java b/spring-beans/src/test/java/org/springframework/beans/factory/DefaultListableBeanFactoryTests.java
@@ -31,6 +31,7 @@
 import java.util.List;
 import java.util.Locale;
 import java.util.Map;
+import java.util.Optional;
 import java.util.Properties;
 import java.util.Set;
 import java.util.concurrent.Callable;
@@ -2711,7 +2712,7 @@ public void testContainsBeanReturnsTrueEvenForAbstractBeanDefinition() {
 	}
 
 	@Test
-	public void resolveEmbeddedValue() throws Exception {
+	public void resolveEmbeddedValue() {
 		DefaultListableBeanFactory bf = new DefaultListableBeanFactory();
 		StringValueResolver r1 = mock(StringValueResolver.class);
 		StringValueResolver r2 = mock(StringValueResolver.class);
@@ -2730,6 +2731,25 @@ public void resolveEmbeddedValue() throws Exception {
 		verify(r3, never()).resolveStringValue(isNull(String.class));
 	}
 
+	@Test
+	public void populatedJavaUtilOptionalBean() {
+		DefaultListableBeanFactory bf = new DefaultListableBeanFactory();
+		RootBeanDefinition bd = new RootBeanDefinition(Optional.class);
+		bd.setFactoryMethodName("of");
+		bd.getConstructorArgumentValues().addGenericArgumentValue("CONTENT");
+		bf.registerBeanDefinition("optionalBean", bd);
+		assertEquals(Optional.of("CONTENT"), bf.getBean(Optional.class));
+	}
+
+	@Test
+	public void emptyJavaUtilOptionalBean() {
+		DefaultListableBeanFactory bf = new DefaultListableBeanFactory();
+		RootBeanDefinition bd = new RootBeanDefinition(Optional.class);
+		bd.setFactoryMethodName("empty");
+		bf.registerBeanDefinition("optionalBean", bd);
+		assertSame(Optional.empty(), bf.getBean(Optional.class));
+	}
+
 	/**
 	 * Test that by-type bean lookup caching is working effectively by searching for a
 	 * bean of type B 10K times within a container having 1K additional beans of type A.

Original file line number	Diff line number	Diff line change
`@@ -272,7 +272,7 @@ public Object run() {`
`272`	`272`	`mbd, beanName, this.beanFactory, constructorToUse, argsToUse);`
`273`	`273`	`}`
`274`	`274`
`275`		`- bw.setWrappedInstance(beanInstance);`
	`275`	`+ bw.setBeanInstance(beanInstance);`
`276`	`276`	`return bw;`
`277`	`277`	`}`
`278`	`278`	`catch (Throwable ex) {`
`@@ -592,7 +592,7 @@ public Object run() {`
`592`	`592`	`if (beanInstance == null) {`
`593`	`593`	`return null;`
`594`	`594`	`}`
`595`		`- bw.setWrappedInstance(beanInstance);`
	`595`	`+ bw.setBeanInstance(beanInstance);`
`596`	`596`	`return bw;`
`597`	`597`	`}`
`598`	`598`	`catch (Throwable ex) {`