From 583a60baedb135c0f2db8aae8117b2817320a97e Mon Sep 17 00:00:00 2001 From: Oliver Gierke Date: Tue, 24 Jul 2012 15:06:20 +0200 Subject: [PATCH] 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. --- .../data/mapping/PersistentProperty.java | 19 ++++ .../model/AbstractPersistentProperty.java | 85 +++++++++++++- .../data/mapping/model/BeanWrapper.java | 27 ++--- .../AbstractPersistentPropertyUnitTests.java | 104 ++++++++++++++++++ 4 files changed, 214 insertions(+), 21 deletions(-) diff --git a/spring-data-commons-core/src/main/java/org/springframework/data/mapping/PersistentProperty.java b/spring-data-commons-core/src/main/java/org/springframework/data/mapping/PersistentProperty.java index 7ffe9a6c5..9d2eda9d1 100644 --- a/spring-data-commons-core/src/main/java/org/springframework/data/mapping/PersistentProperty.java +++ b/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

> { /** * 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(); diff --git a/spring-data-commons-core/src/main/java/org/springframework/data/mapping/model/AbstractPersistentProperty.java b/spring-data-commons-core/src/main/java/org/springframework/data/mapping/model/AbstractPersistentProperty.java index dd10a26dc..18ad2687d 100644 --- a/spring-data-commons-core/src/main/java/org/springframework/data/mapping/model/AbstractPersistentProperty.java +++ b/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

protected abstract Association

createAssociation(); + /* + * (non-Javadoc) + * @see org.springframework.data.mapping.PersistentProperty#getOwner() + */ public PersistentEntity 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

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

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

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

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; } diff --git a/spring-data-commons-core/src/main/java/org/springframework/data/mapping/model/BeanWrapper.java b/spring-data-commons-core/src/main/java/org/springframework/data/mapping/model/BeanWrapper.java index cee678d49..ad013e2b3 100644 --- a/spring-data-commons-core/src/main/java/org/springframework/data/mapping/model/BeanWrapper.java +++ b/spring-data-commons-core/src/main/java/org/springframework/data/mapping/model/BeanWrapper.java @@ -61,13 +61,10 @@ public class BeanWrapper, 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, 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, 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, T> { * @param * @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, T> { * Returns the value of the given {@link PersistentProperty} potentially converted to the given type. * * @param - * @param property + * @param property must not be {@literal null}. * @param type * @param fieldAccessOnly * @return - * @throws IllegalAccessException - * @throws InvocationTargetException */ public S getProperty(PersistentProperty property, Class 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, 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); diff --git a/spring-data-commons-core/src/test/java/org/springframework/data/mapping/model/AbstractPersistentPropertyUnitTests.java b/spring-data-commons-core/src/test/java/org/springframework/data/mapping/model/AbstractPersistentPropertyUnitTests.java index 2aef42918..c21c5e04e 100644 --- a/spring-data-commons-core/src/test/java/org/springframework/data/mapping/model/AbstractPersistentPropertyUnitTests.java +++ b/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 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 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 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 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 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 { public SamplePersistentProperty(Field field, PropertyDescriptor propertyDescriptor,