DATACMNS-206 - Improved working with getters and setters for PersistentProperty.

PersistentProperty now exposes getGetter() and getSetter() methods which restrict the methods being returned to return or accept type compatible values to the actual properties type. Thus when you implement a custom getter or setter which - by the definition of the JavaBean spec - would qualify as read or write method for a particular property, we now ensure that they are only considered if they match the type of the field exposed.

spring-data-commons-core/src/main/java/org/springframework/data/mapping/PersistentProperty.java

@@ -2,6 +2,7 @@ package org.springframework.data.mapping;
 
 import java.beans.PropertyDescriptor;
 import java.lang.reflect.Field;
+import java.lang.reflect.Method;
 import java.util.Collection;
 import java.util.Map;
 
@@ -44,10 +45,28 @@ public interface PersistentProperty<P extends PersistentProperty<P>> {
 	/**
 	 * Returns the {@link PropertyDescriptor} backing the {@link PersistentProperty}.
 	 * 
+	 * @deprecated use {@link #getGetter()} or {@link #getSetter()} directly, will be removed for 1.4.0.GA.
 	 * @return
 	 */
+	@Deprecated
 	PropertyDescriptor getPropertyDescriptor();
 
+	/**
+	 * Returns the getter method to access the property value if available. Might return {@literal null} in case there is
+	 * no getter method with a return type assignable to the actual property's type.
+	 * 
+	 * @return the getter method to access the property value if available, otherwise {@literal null}.
+	 */
+	Method getGetter();
+
+	/**
+	 * Returns the setter method to set a property value. Might return {@literal null} in case there is no setter
+	 * available.
+	 * 
+	 * @returnthe setter method to set a property value if available, otherwise {@literal null}.
+	 */
+	Method getSetter();
+
 	Field getField();
 
 	String getSpelExpression();

spring-data-commons-core/src/main/java/org/springframework/data/mapping/model/AbstractPersistentProperty.java

@@ -18,6 +18,7 @@ package org.springframework.data.mapping.model;
 import java.beans.PropertyDescriptor;
 import java.lang.annotation.Annotation;
 import java.lang.reflect.Field;
+import java.lang.reflect.Method;
 import java.util.ArrayList;
 import java.util.List;
 import java.util.Map;
@@ -65,22 +66,42 @@ public abstract class AbstractPersistentProperty<P extends PersistentProperty<P>
 
 	protected abstract Association<P> createAssociation();
 
+	/*
+	 * (non-Javadoc)
+	 * @see org.springframework.data.mapping.PersistentProperty#getOwner()
+	 */
 	public PersistentEntity<?, P> getOwner() {
 		return owner;
 	}
 
+	/*
+	 * (non-Javadoc)
+	 * @see org.springframework.data.mapping.PersistentProperty#getName()
+	 */
 	public String getName() {
 		return name;
 	}
 
+	/*
+	 * (non-Javadoc)
+	 * @see org.springframework.data.mapping.PersistentProperty#getType()
+	 */
 	public Class<?> getType() {
 		return information.getType();
 	}
 
+	/*
+	 * (non-Javadoc)
+	 * @see org.springframework.data.mapping.PersistentProperty#getRawType()
+	 */
 	public Class<?> getRawType() {
 		return this.rawType;
 	}
 
+	/*
+	 * (non-Javadoc)
+	 * @see org.springframework.data.mapping.PersistentProperty#getTypeInformation()
+	 */
 	public TypeInformation<?> getTypeInformation() {
 		return information;
 	}
@@ -113,14 +134,56 @@ public abstract class AbstractPersistentProperty<P extends PersistentProperty<P>
 		return information == null || simpleTypeHolder.isSimpleType(information.getType()) ? null : information;
 	}
 
+	/*
+	 * (non-Javadoc)
+	 * @see org.springframework.data.mapping.PersistentProperty#getPropertyDescriptor()
+	 */
 	public PropertyDescriptor getPropertyDescriptor() {
 		return propertyDescriptor;
 	}
 
+	/* 
+	 * (non-Javadoc)
+	 * @see org.springframework.data.mapping.PersistentProperty#getGetter()
+	 */
+	public Method getGetter() {
+
+		Method getter = propertyDescriptor.getReadMethod();
+
+		if (getter == null) {
+			return null;
+		}
+
+		return rawType.isAssignableFrom(getter.getReturnType()) ? getter : null;
+	}
+
+	/* 
+	 * (non-Javadoc)
+	 * @see org.springframework.data.mapping.PersistentProperty#getSetter()
+	 */
+	public Method getSetter() {
+
+		Method setter = propertyDescriptor.getWriteMethod();
+
+		if (setter == null) {
+			return null;
+		}
+
+		return setter.getParameterTypes()[0].isAssignableFrom(rawType) ? setter : null;
+	}
+
+	/*
+	 * (non-Javadoc)
+	 * @see org.springframework.data.mapping.PersistentProperty#getField()
+	 */
 	public Field getField() {
 		return field;
 	}
 
+	/*
+	 * (non-Javadoc)
+	 * @see org.springframework.data.mapping.PersistentProperty#getSpelExpression()
+	 */
 	public String getSpelExpression() {
 		return null;
 	}
@@ -141,6 +204,10 @@ public abstract class AbstractPersistentProperty<P extends PersistentProperty<P>
 		return !isTransient();
 	}
 
+	/*
+	 * (non-Javadoc)
+	 * @see org.springframework.data.mapping.PersistentProperty#isAssociation()
+	 */
 	public boolean isAssociation() {
 		if (field.isAnnotationPresent(Reference.class)) {
 			return true;
@@ -166,13 +233,18 @@ public abstract class AbstractPersistentProperty<P extends PersistentProperty<P>
 		return information.isCollectionLike();
 	}
 
+	/*
+	 * (non-Javadoc)
+	 * @see org.springframework.data.mapping.PersistentProperty#isMap()
+	 */
 	public boolean isMap() {
 		return Map.class.isAssignableFrom(getType());
 	}
 
-	/* (non-Javadoc)
-		 * @see org.springframework.data.mapping.model.PersistentProperty#isArray()
-		 */
+	/*
+	 * (non-Javadoc)
+	 * @see org.springframework.data.mapping.PersistentProperty#isArray()
+	 */
 	public boolean isArray() {
 		return getType().isArray();
 	}
@@ -202,9 +274,10 @@ public abstract class AbstractPersistentProperty<P extends PersistentProperty<P>
 		return componentType == null ? null : componentType.getType();
 	}
 
-	/* (non-Javadoc)
-		 * @see org.springframework.data.mapping.model.PersistentProperty#getMapValueType()
-		 */
+	/*
+	 * (non-Javadoc)
+	 * @see org.springframework.data.mapping.PersistentProperty#getMapValueType()
+	 */
 	public Class<?> getMapValueType() {
 		return isMap() ? information.getMapValueType().getType() : null;
 	}

spring-data-commons-core/src/main/java/org/springframework/data/mapping/model/BeanWrapper.java

@@ -61,13 +61,10 @@ public class BeanWrapper<E extends PersistentEntity<T, ?>, T> {
 	 * {@link ConversionService} is configured. Will use the accessor method of the given {@link PersistentProperty} if it
 	 * has one or field access otherwise.
 	 * 
-	 * @param property
+	 * @param property must not be {@literal null}.
 	 * @param value
-	 * @throws IllegalAccessException
-	 * @throws InvocationTargetException
 	 */
-	public void setProperty(PersistentProperty<?> property, Object value) throws IllegalAccessException,
-			InvocationTargetException {
+	public void setProperty(PersistentProperty<?> property, Object value) {
 		setProperty(property, value, false);
 	}
 
@@ -75,16 +72,18 @@ public class BeanWrapper<E extends PersistentEntity<T, ?>, T> {
 	 * Sets the given {@link PersistentProperty} to the given value. Will do type conversion if a
 	 * {@link ConversionService} is configured.
 	 * 
-	 * @param property
+	 * @param property must not be {@literal null}.
 	 * @param value
+	 * @param fieldAccessOnly whether to only try accessing the field ({@literal true}) or try using the getter first.
 	 * @throws IllegalAccessException
 	 * @throws InvocationTargetException
 	 */
 	public void setProperty(PersistentProperty<?> property, Object value, boolean fieldAccessOnly) {
 
-		Method setter = property.getPropertyDescriptor() != null ? property.getPropertyDescriptor().getWriteMethod() : null;
+		Method setter = property.getSetter();
 
 		try {
+
 			if (fieldAccessOnly || null == setter) {
 				Object valueToSet = getPotentiallyConvertedValue(value, property.getType());
 				ReflectionUtils.makeAccessible(property.getField());
@@ -96,6 +95,7 @@ public class BeanWrapper<E extends PersistentEntity<T, ?>, T> {
 			Object valueToSet = getPotentiallyConvertedValue(value, paramTypes[0]);
 			ReflectionUtils.makeAccessible(setter);
 			ReflectionUtils.invokeMethod(setter, bean, valueToSet);
+
 		} catch (IllegalStateException e) {
 			throw new MappingException("Could not set object property!", e);
 		}
@@ -107,10 +107,8 @@ public class BeanWrapper<E extends PersistentEntity<T, ?>, T> {
 	 * @param <S>
 	 * @param property
 	 * @return
-	 * @throws IllegalAccessException
-	 * @throws InvocationTargetException
 	 */
-	public Object getProperty(PersistentProperty<?> property) throws IllegalAccessException, InvocationTargetException {
+	public Object getProperty(PersistentProperty<?> property) {
 		return getProperty(property, property.getType(), false);
 	}
 
@@ -118,19 +116,17 @@ public class BeanWrapper<E extends PersistentEntity<T, ?>, T> {
 	 * Returns the value of the given {@link PersistentProperty} potentially converted to the given type.
 	 * 
 	 * @param <S>
-	 * @param property
+	 * @param property must not be {@literal null}.
 	 * @param type
 	 * @param fieldAccessOnly
 	 * @return
-	 * @throws IllegalAccessException
-	 * @throws InvocationTargetException
 	 */
 	public <S> S getProperty(PersistentProperty<?> property, Class<? extends S> type, boolean fieldAccessOnly) {
+
 		try {
 			Object obj;
 			Field field = property.getField();
-			Method getter = (null != property.getPropertyDescriptor() ? property.getPropertyDescriptor().getReadMethod()
-					: null);
+			Method getter = property.getGetter();
 
 			if (fieldAccessOnly || null == getter) {
 				ReflectionUtils.makeAccessible(field);
@@ -141,6 +137,7 @@ public class BeanWrapper<E extends PersistentEntity<T, ?>, T> {
 			}
 
 			return getPotentiallyConvertedValue(obj, type);
+
 		} catch (IllegalStateException e) {
 			throw new MappingException(String.format("Could not read property %s of %s!", property.toString(),
 					bean.toString()), e);

spring-data-commons-core/src/test/java/org/springframework/data/mapping/model/AbstractPersistentPropertyUnitTests.java

@@ -18,6 +18,9 @@ package org.springframework.data.mapping.model;
 import static org.hamcrest.Matchers.*;
 import static org.junit.Assert.*;
 
+import java.beans.BeanInfo;
+import java.beans.IntrospectionException;
+import java.beans.Introspector;
 import java.beans.PropertyDescriptor;
 import java.lang.reflect.Field;
 import java.util.Collection;
@@ -122,6 +125,69 @@ public class AbstractPersistentPropertyUnitTests {
 		assertThat(property.isTransient(), is(false));
 	}
 
+	@Test
+	public void findsSimpleGettersAndASetters() {
+
+		Field field = ReflectionUtils.findField(AccessorTestClass.class, "id");
+		PersistentProperty<SamplePersistentProperty> property = new SamplePersistentProperty(field, getPropertyDescriptor(
+				AccessorTestClass.class, "id"), entity, typeHolder);
+
+		assertThat(property.getGetter(), is(notNullValue()));
+		assertThat(property.getSetter(), is(notNullValue()));
+	}
+
+	@Test
+	public void doesNotUseInvalidGettersAndASetters() {
+
+		Field field = ReflectionUtils.findField(AccessorTestClass.class, "anotherId");
+		PersistentProperty<SamplePersistentProperty> property = new SamplePersistentProperty(field, getPropertyDescriptor(
+				AccessorTestClass.class, "anotherId"), entity, typeHolder);
+
+		assertThat(property.getGetter(), is(nullValue()));
+		assertThat(property.getSetter(), is(nullValue()));
+	}
+
+	@Test
+	public void usesCustomGetter() {
+
+		Field field = ReflectionUtils.findField(AccessorTestClass.class, "yetAnotherId");
+		PersistentProperty<SamplePersistentProperty> property = new SamplePersistentProperty(field, getPropertyDescriptor(
+				AccessorTestClass.class, "yetAnotherId"), entity, typeHolder);
+
+		assertThat(property.getGetter(), is(notNullValue()));
+		assertThat(property.getSetter(), is(nullValue()));
+	}
+
+	@Test
+	public void usesCustomSetter() {
+
+		Field field = ReflectionUtils.findField(AccessorTestClass.class, "yetYetAnotherId");
+		PersistentProperty<SamplePersistentProperty> property = new SamplePersistentProperty(field, getPropertyDescriptor(
+				AccessorTestClass.class, "yetYetAnotherId"), entity, typeHolder);
+
+		assertThat(property.getGetter(), is(nullValue()));
+		assertThat(property.getSetter(), is(notNullValue()));
+	}
+
+	private static PropertyDescriptor getPropertyDescriptor(Class<?> type, String propertyName) {
+
+		try {
+
+			BeanInfo info = Introspector.getBeanInfo(type);
+
+			for (PropertyDescriptor descriptor : info.getPropertyDescriptors()) {
+				if (descriptor.getName().equals(propertyName)) {
+					return descriptor;
+				}
+			}
+
+			return null;
+
+		} catch (IntrospectionException e) {
+			return null;
+		}
+	}
+
 	class Generic<T> {
 		T genericField;
 
@@ -149,6 +215,44 @@ public class AbstractPersistentPropertyUnitTests {
 		transient Object transientField;
 	}
 
+	class AccessorTestClass {
+
+		// Valid getters and setters
+		Long id;
+		// Invalid getters and setters
+		Long anotherId;
+
+		// Customized getter
+		Number yetAnotherId;
+
+		// Customized setter
+		Number yetYetAnotherId;
+
+		public Long getId() {
+			return id;
+		}
+
+		public void setId(Long id) {
+			this.id = id;
+		}
+
+		public String getAnotherId() {
+			return anotherId.toString();
+		}
+
+		public void setAnotherId(String anotherId) {
+			this.anotherId = Long.parseLong(anotherId);
+		}
+
+		public Long getYetAnotherId() {
+			return null;
+		}
+
+		public void setYetYetAnotherId(Object yetYetAnotherId) {
+			this.yetYetAnotherId = null;
+		}
+	}
+
 	class SamplePersistentProperty extends AbstractPersistentProperty<SamplePersistentProperty> {
 
 		public SamplePersistentProperty(Field field, PropertyDescriptor propertyDescriptor,