Merge BeanWrapperImpl and DirectFieldAccessor

`BeanWrapperImpl` and `DirectFieldAccessor` are two
`ConfigurablePropertyAccessor` implementations with different features
set.

This commit harmonizes the two implementations to use a common base class
that delegates the actual property handling to the sub-classes:

* `BeanWrapperImpl`:  `PropertyDescriptor` and introspection utilities
* `DirectFieldAccessor`: reflection on `java.lang.Field`

Issues: SPR-12206 - SPR-12805

spring-aop/src/test/java/org/springframework/aop/aspectj/MethodInvocationProceedingJoinPointTests.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2013 the original author or authors.
+ * Copyright 2002-2015 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.
@@ -211,7 +211,7 @@ public final class MethodInvocationProceedingJoinPointTests {
 		itb.setName("foo");
 		itb.getDoctor();
 		itb.getStringArray();
-		itb.getSpouses();
+		itb.getSpouse();
 		itb.setSpouse(new TestBean());
 		try {
 			itb.unreliableFileOperation();

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

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2014 the original author or authors.
+ * Copyright 2002-2015 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,25 +16,22 @@
 
 package org.springframework.beans;
 
-import java.beans.PropertyChangeEvent;
 import java.lang.reflect.Field;
-import java.util.ArrayList;
 import java.util.HashMap;
-import java.util.List;
 import java.util.Map;
-import java.util.StringTokenizer;
 
-import org.springframework.core.convert.ConversionException;
-import org.springframework.core.convert.ConverterNotFoundException;
+import org.springframework.core.ResolvableType;
 import org.springframework.core.convert.TypeDescriptor;
-import org.springframework.util.Assert;
 import org.springframework.util.ReflectionUtils;
 
 /**
- * {@link PropertyAccessor} implementation that directly accesses instance fields.
- * Allows for direct binding to fields instead of going through JavaBean setters.
+ * {@link ConfigurablePropertyAccessor} implementation that directly accesses
+ * instance fields. Allows for direct binding to fields instead of going through
+ * JavaBean setters.
  *
- * <p>As of Spring 4.1, this implementation supports nested field traversal.
+ * <p>As of Spring 4.2, the vast majority of the {@link BeanWrapper} features have
+ * been merged to {@link AbstractPropertyAccessor}, which means that property
+ * traversal as well as collections and map access is now supported here as well.
  *
  * <p>A DirectFieldAccessor's default for the "extractOldValueForEditor" setting
  * is "true", since a field can always be read without side effects.
@@ -49,264 +46,89 @@ import org.springframework.util.ReflectionUtils;
  */
 public class DirectFieldAccessor extends AbstractPropertyAccessor {
 
-	private final Object rootObject;
+	private final Map<String, FieldPropertyHandler> fieldMap = new HashMap<String, FieldPropertyHandler>();
 
-	private final Map<String, FieldAccessor> fieldMap = new HashMap<String, FieldAccessor>();
-
-
-	/**
-	 * Create a new DirectFieldAccessor for the given root object.
-	 * @param rootObject the root object to access
-	 */
-	public DirectFieldAccessor(final Object rootObject) {
-		Assert.notNull(rootObject, "Root object must not be null");
-		this.rootObject = rootObject;
-		this.typeConverterDelegate = new TypeConverterDelegate(this, rootObject);
-		registerDefaultEditors();
-		setExtractOldValueForEditor(true);
-	}
-
-	/**
-	 * Return the root object at the top of the path of this instance.
-	 */
-	public final Object getRootInstance() {
-		return this.rootObject;
-	}
-
-	/**
-	 * Return the class of the root object at the top of the path of this instance.
-	 */
-	public final Class<?> getRootClass() {
-		return (this.rootObject != null ? this.rootObject.getClass() : null);
-	}
-
-	@Override
-	public boolean isReadableProperty(String propertyName) throws BeansException {
-		return hasProperty(propertyName);
+	public DirectFieldAccessor(Object object) {
+		super(object);
 	}
 
-	@Override
-	public boolean isWritableProperty(String propertyName) throws BeansException {
-		return hasProperty(propertyName);
-	}
-
-	@Override
-	public Class<?> getPropertyType(String propertyPath) throws BeansException {
-		FieldAccessor fieldAccessor = getFieldAccessor(propertyPath);
-		if (fieldAccessor != null) {
-			return fieldAccessor.getField().getType();
-		}
-		return null;
+	protected DirectFieldAccessor(Object object, String nestedPath, DirectFieldAccessor superBw) {
+		super(object, nestedPath, superBw);
 	}
 
 	@Override
-	public TypeDescriptor getPropertyTypeDescriptor(String propertyName) throws BeansException {
-		FieldAccessor fieldAccessor = getFieldAccessor(propertyName);
-		if (fieldAccessor != null) {
-			return new TypeDescriptor(fieldAccessor.getField());
+	protected FieldPropertyHandler getLocalPropertyHandler(String propertyName) {
+		FieldPropertyHandler propertyHandler = this.fieldMap.get(propertyName);
+		if (propertyHandler == null) {
+			Field field = ReflectionUtils.findField(getWrappedClass(), propertyName);
+			if (field != null) {
+				propertyHandler = new FieldPropertyHandler(field);
+			}
+			this.fieldMap.put(propertyName, propertyHandler);
 		}
-		return null;
+		return propertyHandler;
 	}
 
 	@Override
-	public Object getPropertyValue(String propertyName) throws BeansException {
-		FieldAccessor fieldAccessor = getFieldAccessor(propertyName);
-		if (fieldAccessor == null) {
-			throw new NotReadablePropertyException(
-					getRootClass(), propertyName, "Field '" + propertyName + "' does not exist");
-		}
-		return fieldAccessor.getValue();
+	protected DirectFieldAccessor newNestedPropertyAccessor(Object object, String nestedPath) {
+		return new DirectFieldAccessor(object, nestedPath, this);
 	}
 
 	@Override
-	public void setPropertyValue(String propertyName, Object newValue) throws BeansException {
-		FieldAccessor fieldAccessor = getFieldAccessor(propertyName);
-		if (fieldAccessor == null) {
-			throw new NotWritablePropertyException(
-					getRootClass(), propertyName, "Field '" + propertyName + "' does not exist");
-		}
-		Field field = fieldAccessor.getField();
-		Object oldValue = null;
-		try {
-			oldValue = fieldAccessor.getValue();
-			Object convertedValue = this.typeConverterDelegate.convertIfNecessary(
-					field.getName(), oldValue, newValue, field.getType(), new TypeDescriptor(field));
-			fieldAccessor.setValue(convertedValue);
-		}
-		catch (ConverterNotFoundException ex) {
-			PropertyChangeEvent pce = new PropertyChangeEvent(getRootInstance(), propertyName, oldValue, newValue);
-			throw new ConversionNotSupportedException(pce, field.getType(), ex);
-		}
-		catch (ConversionException ex) {
-			PropertyChangeEvent pce = new PropertyChangeEvent(getRootInstance(), propertyName, oldValue, newValue);
-			throw new TypeMismatchException(pce, field.getType(), ex);
-		}
-		catch (IllegalStateException ex) {
-			PropertyChangeEvent pce = new PropertyChangeEvent(getRootInstance(), propertyName, oldValue, newValue);
-			throw new ConversionNotSupportedException(pce, field.getType(), ex);
-		}
-		catch (IllegalArgumentException ex) {
-			PropertyChangeEvent pce = new PropertyChangeEvent(getRootInstance(), propertyName, oldValue, newValue);
-			throw new TypeMismatchException(pce, field.getType(), ex);
-		}
+	protected NotWritablePropertyException createNotWritablePropertyException(String propertyName) {
+		throw new NotWritablePropertyException(
+				getRootClass(), getNestedPath() + propertyName, "Field does not exist",
+				new String[0]);
 	}
 
-	private boolean hasProperty(String propertyPath) {
-		Assert.notNull(propertyPath, "PropertyPath must not be null");
-		return getFieldAccessor(propertyPath) != null;
-	}
 
-	private FieldAccessor getFieldAccessor(String propertyPath) {
-		FieldAccessor fieldAccessor = this.fieldMap.get(propertyPath);
-		if (fieldAccessor == null) {
-			fieldAccessor = doGetFieldAccessor(propertyPath, getRootClass());
-			this.fieldMap.put(propertyPath, fieldAccessor);
-		}
-		return fieldAccessor;
-	}
-
-	private FieldAccessor doGetFieldAccessor(String propertyPath, Class<?> targetClass) {
-		StringTokenizer st = new StringTokenizer(propertyPath, ".");
-		FieldAccessor accessor = null;
-		Class<?> parentType = targetClass;
-		while (st.hasMoreTokens()) {
-			String localProperty = st.nextToken();
-			Field field = ReflectionUtils.findField(parentType, localProperty);
-			if (field == null) {
-				return null;
-			}
-			if (accessor == null) {
-				accessor = root(propertyPath, localProperty, field);
-			}
-			else {
-				accessor = accessor.child(localProperty, field);
-			}
-			parentType = field.getType();
-		}
-		return accessor;
-	}
-
-	/**
-	 * Create a root {@link FieldAccessor}.
-	 * @param canonicalName the full expression for the field to access
-	 * @param actualName the name of the local (root) property
-	 * @param field the field accessing the property
-	 */
-	private FieldAccessor root(String canonicalName, String actualName, Field field) {
-		return new FieldAccessor(null, canonicalName, actualName, field);
-	}
-
-
-	/**
-	 * Provide an easy access to a potentially hierarchical value.
-	 */
-	private class FieldAccessor {
-
-		private final List<FieldAccessor> parents;
-
-		private final String canonicalName;
-
-		private final String actualName;
+	private class FieldPropertyHandler extends PropertyHandler {
 
 		private final Field field;
 
-		/**
-		 * Create a new FieldAccessor instance.
-		 * @param parent the parent accessor, if any
-		 * @param canonicalName the full expression for the field to access
-		 * @param actualName the name of the partial expression for this property
-		 * @param field the field accessing the property
-		 */
-		public FieldAccessor(FieldAccessor parent, String canonicalName, String actualName, Field field) {
-			Assert.notNull(canonicalName, "Expression must no be null");
-			Assert.notNull(field, "Field must no be null");
-			this.parents = buildParents(parent);
-			this.canonicalName = canonicalName;
-			this.actualName = actualName;
+		public FieldPropertyHandler(Field field) {
+			super(field.getType(), true, true);
 			this.field = field;
 		}
 
-		/**
-		 * Create a child instance.
-		 * @param actualName the name of the child property
-		 * @param field the field accessing the child property
-		 */
-		public FieldAccessor child(String actualName, Field field) {
-			return new FieldAccessor(this, this.canonicalName, this.actualName + "." + actualName, field);
+		@Override
+		public TypeDescriptor toTypeDescriptor() {
+			return new TypeDescriptor(this.field);
 		}
 
-		public Field getField() {
-			return this.field;
+		@Override
+		public ResolvableType getResolvableType() {
+			return ResolvableType.forField(this.field);
 		}
 
-		public Object getValue() {
-			Object localTarget = getLocalTarget(getRootInstance());
-			return getParentValue(localTarget);
-
+		@Override
+		public TypeDescriptor nested(int level) {
+			return TypeDescriptor.nested(this.field, level);
 		}
 
-		public void setValue(Object value) {
-			Object localTarget = getLocalTarget(getRootInstance());
+		@Override
+		public Object getValue() throws Exception {
 			try {
-				this.field.set(localTarget, value);
+				ReflectionUtils.makeAccessible(this.field);
+				return this.field.get(getWrappedInstance());
 			}
+
 			catch (IllegalAccessException ex) {
-				throw new InvalidPropertyException(localTarget.getClass(), canonicalName, "Field is not accessible", ex);
+				throw new InvalidPropertyException(getWrappedClass(),
+						this.field.getName(), "Field is not accessible", ex);
 			}
 		}
 
-		private Object getParentValue(Object target) {
+		@Override
+		public void setValue(Object object, Object value) throws Exception {
 			try {
 				ReflectionUtils.makeAccessible(this.field);
-				return this.field.get(target);
+				this.field.set(object, value);
 			}
 			catch (IllegalAccessException ex) {
-				throw new InvalidPropertyException(target.getClass(),
-						this.canonicalName, "Field is not accessible", ex);
-			}
-		}
-
-		private Object getLocalTarget(Object rootTarget) {
-			Object localTarget = rootTarget;
-			for (FieldAccessor parent : parents) {
-				localTarget = autoGrowIfNecessary(parent, parent.getParentValue(localTarget));
-				if (localTarget == null) { // Could not traverse the graph any further
-					throw new NullValueInNestedPathException(getRootClass(), parent.actualName,
-							"Cannot access indexed value of property referenced in indexed property path '" +
-									getField().getName() + "': returned null");
-				}
-			}
-			return localTarget;
-		}
-
-		private Object newValue() {
-			Class<?> type = getField().getType();
-			try {
-				return type.newInstance();
-			}
-			catch (Exception ex) {
-				throw new NullValueInNestedPathException(getRootClass(), this.actualName,
-						"Could not instantiate property type [" + type.getName() +
-								"] to auto-grow nested property path: " + ex);
-			}
-		}
-
-		private Object autoGrowIfNecessary(FieldAccessor accessor, Object value) {
-			if (value == null && isAutoGrowNestedPaths()) {
-				Object defaultValue = accessor.newValue();
-				accessor.setValue(defaultValue);
-				return defaultValue;
-			}
-			return value;
-		}
-
-		private List<FieldAccessor> buildParents(FieldAccessor parent) {
-			List<FieldAccessor> parents = new ArrayList<FieldAccessor>();
-			if (parent != null) {
-				parents.addAll(parent.parents);
-				parents.add(parent);
+				throw new InvalidPropertyException(getWrappedClass(), this.field.getName(),
+						"Field is not accessible", ex);
 			}
-			return parents;
 		}
 	}
 

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

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2014 the original author or authors.
+ * Copyright 2002-2015 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,7 +16,6 @@
 
 package org.springframework.beans;
 
-import java.beans.PropertyDescriptor;
 import java.io.Serializable;
 
 import org.springframework.util.Assert;
@@ -60,9 +59,6 @@ public class PropertyValue extends BeanMetadataAttributeAccessor implements Seri
 	/** Package-visible field for caching the resolved property path tokens */
 	transient volatile Object resolvedTokens;
 
-	/** Package-visible field for caching the resolved PropertyDescriptor */
-	transient volatile PropertyDescriptor resolvedDescriptor;
-
 
 	/**
 	 * Create a new PropertyValue instance.
@@ -88,7 +84,6 @@ public class PropertyValue extends BeanMetadataAttributeAccessor implements Seri
 		this.convertedValue = original.convertedValue;
 		this.conversionNecessary = original.conversionNecessary;
 		this.resolvedTokens = original.resolvedTokens;
-		this.resolvedDescriptor = original.resolvedDescriptor;
 		copyAttributesFrom(original);
 	}
 
@@ -106,7 +101,6 @@ public class PropertyValue extends BeanMetadataAttributeAccessor implements Seri
 		this.optional = original.isOptional();
 		this.conversionNecessary = original.conversionNecessary;
 		this.resolvedTokens = original.resolvedTokens;
-		this.resolvedDescriptor = original.resolvedDescriptor;
 		copyAttributesFrom(original);
 	}
 

spring-beans/src/test/java/org/springframework/beans/DirectFieldAccessorTests.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2014 the original author or authors.
+ * Copyright 2002-2015 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.
@@ -23,27 +23,29 @@ import org.springframework.tests.sample.beans.TestBean;
 import static org.junit.Assert.*;
 
 /**
- * Unit tests for {@link DirectFieldAccessor}
+ * Specific {@link DirectFieldAccessor} tests.
  *
  * @author Jose Luis Martin
  * @author Chris Beams
+ * @@author Stephane Nicoll
  */
 public class DirectFieldAccessorTests extends AbstractConfigurablePropertyAccessorTests {
 
 	@Override
-	protected ConfigurablePropertyAccessor createAccessor(Object target) {
+	protected DirectFieldAccessor createAccessor(Object target) {
 		return new DirectFieldAccessor(target);
 	}
 
+
 	@Test
 	public void withShadowedField() throws Exception {
 		@SuppressWarnings("serial")
-		TestBean tb = new TestBean() {
+		TestBean target = new TestBean() {
 			@SuppressWarnings("unused")
 			StringBuilder name = new StringBuilder();
 		};
 
-		DirectFieldAccessor dfa = new DirectFieldAccessor(tb);
+		DirectFieldAccessor dfa = createAccessor(target);
 		assertEquals(StringBuilder.class, dfa.getPropertyType("name"));
 	}
 

spring-beans/src/test/java/org/springframework/tests/sample/beans/TestBean.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2012 the original author or authors.
+ * Copyright 2002-2015 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.
@@ -38,6 +38,7 @@ import org.springframework.util.ObjectUtils;
  *
  * @author Rod Johnson
  * @author Juergen Hoeller
+ * @author Stephane Nicoll
  * @since 15 April 2001
  */
 public class TestBean implements BeanNameAware, BeanFactoryAware, ITestBean, IOther, Comparable<Object> {
@@ -58,6 +59,8 @@ public class TestBean implements BeanNameAware, BeanFactoryAware, ITestBean, IOt
 
 	private boolean jedi;
 
+	private ITestBean spouse;
+
 	protected ITestBean[] spouses;
 
 	private String touchy;
@@ -113,7 +116,7 @@ public class TestBean implements BeanNameAware, BeanFactoryAware, ITestBean, IOt
 	}
 
 	public TestBean(ITestBean spouse) {
-		this.spouses = new ITestBean[] {spouse};
+		this.spouse = spouse;
 	}
 
 	public TestBean(String name, int age) {
@@ -122,7 +125,7 @@ public class TestBean implements BeanNameAware, BeanFactoryAware, ITestBean, IOt
 	}
 
 	public TestBean(ITestBean spouse, Properties someProperties) {
-		this.spouses = new ITestBean[] {spouse};
+		this.spouse = spouse;
 		this.someProperties = someProperties;
 	}
 
@@ -210,17 +213,17 @@ public class TestBean implements BeanNameAware, BeanFactoryAware, ITestBean, IOt
 
 	@Override
 	public ITestBean getSpouse() {
-		return (spouses != null ? spouses[0] : null);
+		return this.spouse;
 	}
 
 	@Override
 	public void setSpouse(ITestBean spouse) {
-		this.spouses = new ITestBean[] {spouse};
+		this.spouse = spouse;
 	}
 
 	@Override
 	public ITestBean[] getSpouses() {
-		return spouses;
+		return (spouse != null ? new ITestBean[]{spouse} : null);
 	}
 
 	public String getTouchy() {

spring-web/src/test/java/org/springframework/web/bind/support/WebRequestDataBinderTests.java

@@ -345,11 +345,11 @@ public class WebRequestDataBinderTests {
 
 	static class TestBeanWithConcreteSpouse extends TestBean {
 		public void setConcreteSpouse(TestBean spouse) {
-			this.spouses = new ITestBean[] {spouse};
+			setSpouse(spouse);
 		}
 
 		public TestBean getConcreteSpouse() {
-			return (spouses != null ? (TestBean) spouses[0] : null);
+			return (TestBean) getSpouse();
 		}
 	}