Avoid 'type mismatch' errors in ExtendedBeanInfo
Certain edge cases around return type covariance can trigger an IntrospectionException when trying to create a new PropertyDescriptor; particularly around the addition of write methods with parameter types that do not match read method return types. These type mismatch exceptions are raised during normal Introspector operations as well (i.e. without ExtendedBeanInfo in the mix), but the Introspector intentionally supresses them. In covariance cases, there is often already a suitable write method present, e.g. discovered in a supertype or superinterface, that, with the benefit of bridge methods works just fine in practice at runtime. That is to say, in these suppression cases, the rejection of the write method is 'OK' in that there is already a write method present that can handle a call. ExtendedBeanInfo now mirrors this suppression behavior, but does issue a WARN-level log message to let the user know. An important effect of this change is that ExtendedBeanInfo now modifies the delegate BeanInfo object, whereas previously it did not. In practice this probably matters very little, but it is a design change worth noting. The reason for this change was to avoid the need to create new PropertyDescriptors wherever possible. It was discovered that by updating existing PDs, one can avoid these IntrospectionExceptions a greater percentage of the time. Issue: SPR-8806
This commit is contained in:
Chris Beams
parent
947b5fefff
commit
b2ae3dbb47
|
|
@ -16,6 +16,8 @@
|
|||
|
||||
package org.springframework.beans;
|
||||
|
||||
import static java.lang.String.format;
|
||||
|
||||
import java.awt.Image;
|
||||
import java.beans.BeanDescriptor;
|
||||
import java.beans.BeanInfo;
|
||||
|
|
@ -30,6 +32,9 @@ import java.util.Comparator;
|
|||
import java.util.SortedSet;
|
||||
import java.util.TreeSet;
|
||||
|
||||
import org.apache.commons.logging.Log;
|
||||
import org.apache.commons.logging.LogFactory;
|
||||
import org.springframework.util.Assert;
|
||||
import org.springframework.util.ReflectionUtils;
|
||||
import org.springframework.util.StringUtils;
|
||||
|
||||
|
|
@ -48,15 +53,20 @@ import org.springframework.util.StringUtils;
|
|||
* @see CachedIntrospectionResults
|
||||
*/
|
||||
class ExtendedBeanInfo implements BeanInfo {
|
||||
|
||||
private final Log logger = LogFactory.getLog(getClass());
|
||||
|
||||
private final BeanInfo delegate;
|
||||
|
||||
private final SortedSet<PropertyDescriptor> propertyDescriptors =
|
||||
new TreeSet<PropertyDescriptor>(new PropertyDescriptorComparator());
|
||||
|
||||
|
||||
/**
|
||||
* Wrap the given delegate {@link BeanInfo} instance and find any non-void returning
|
||||
* setter methods, creating and adding a {@link PropertyDescriptor} for each.
|
||||
*
|
||||
* <p>The wrapped {@code BeanInfo} is not modified in any way by this process.
|
||||
* <p>Note that the wrapped {@code BeanInfo} is modified by this process.
|
||||
*
|
||||
* @see #getPropertyDescriptors()
|
||||
* @throws IntrospectionException if any problems occur creating and adding new {@code PropertyDescriptors}
|
||||
|
|
@ -92,20 +102,20 @@ class ExtendedBeanInfo implements BeanInfo {
|
|||
if (writeMethod != null
|
||||
&& writeMethod.getName().equals(method.getName())) {
|
||||
// yes -> copy it, including corresponding getter method (if any -- may be null)
|
||||
this.addOrUpdatePropertyDescriptor(propertyName, readMethod, writeMethod);
|
||||
this.addOrUpdatePropertyDescriptor(pd, propertyName, readMethod, writeMethod);
|
||||
continue ALL_METHODS;
|
||||
}
|
||||
// has a getter corresponding to this setter already been found by the wrapped BeanInfo?
|
||||
if (readMethod != null
|
||||
&& readMethod.getName().equals(getterMethodNameFor(propertyName))
|
||||
&& readMethod.getReturnType().equals(method.getParameterTypes()[0])) {
|
||||
this.addOrUpdatePropertyDescriptor(propertyName, readMethod, method);
|
||||
this.addOrUpdatePropertyDescriptor(pd, propertyName, readMethod, method);
|
||||
continue ALL_METHODS;
|
||||
}
|
||||
}
|
||||
// the setter method was not found by the wrapped BeanInfo -> add a new PropertyDescriptor for it
|
||||
// no corresponding getter was detected, so the 'read method' parameter is null.
|
||||
this.addOrUpdatePropertyDescriptor(propertyName, null, method);
|
||||
this.addOrUpdatePropertyDescriptor(null, propertyName, null, method);
|
||||
continue ALL_METHODS;
|
||||
}
|
||||
|
||||
|
|
@ -129,20 +139,20 @@ class ExtendedBeanInfo implements BeanInfo {
|
|||
if (indexedWriteMethod != null
|
||||
&& indexedWriteMethod.getName().equals(method.getName())) {
|
||||
// yes -> copy it, including corresponding getter method (if any -- may be null)
|
||||
this.addOrUpdatePropertyDescriptor(propertyName, readMethod, writeMethod, indexedReadMethod, indexedWriteMethod);
|
||||
this.addOrUpdatePropertyDescriptor(pd, propertyName, readMethod, writeMethod, indexedReadMethod, indexedWriteMethod);
|
||||
continue ALL_METHODS;
|
||||
}
|
||||
// has a getter corresponding to this setter already been found by the wrapped BeanInfo?
|
||||
if (indexedReadMethod != null
|
||||
&& indexedReadMethod.getName().equals(getterMethodNameFor(propertyName))
|
||||
&& indexedReadMethod.getReturnType().equals(method.getParameterTypes()[1])) {
|
||||
this.addOrUpdatePropertyDescriptor(propertyName, readMethod, writeMethod, indexedReadMethod, method);
|
||||
this.addOrUpdatePropertyDescriptor(pd, propertyName, readMethod, writeMethod, indexedReadMethod, method);
|
||||
continue ALL_METHODS;
|
||||
}
|
||||
}
|
||||
// the INDEXED setter method was not found by the wrapped BeanInfo -> add a new PropertyDescriptor
|
||||
// for it. no corresponding INDEXED getter was detected, so the 'indexed read method' parameter is null.
|
||||
this.addOrUpdatePropertyDescriptor(propertyName, null, null, null, method);
|
||||
this.addOrUpdatePropertyDescriptor(null, propertyName, null, null, null, method);
|
||||
continue ALL_METHODS;
|
||||
}
|
||||
|
||||
|
|
@ -154,7 +164,7 @@ class ExtendedBeanInfo implements BeanInfo {
|
|||
&& existingPD.getName().equals(pd.getName())) {
|
||||
if (existingPD.getReadMethod() == null) {
|
||||
// no -> add it now
|
||||
this.addOrUpdatePropertyDescriptor(pd.getName(), method, pd.getWriteMethod());
|
||||
this.addOrUpdatePropertyDescriptor(pd, pd.getName(), method, pd.getWriteMethod());
|
||||
}
|
||||
// yes -> do not add a duplicate
|
||||
continue ALL_METHODS;
|
||||
|
|
@ -164,9 +174,9 @@ class ExtendedBeanInfo implements BeanInfo {
|
|||
|| (pd instanceof IndexedPropertyDescriptor && method == ((IndexedPropertyDescriptor) pd).getIndexedReadMethod())) {
|
||||
// yes -> copy it, including corresponding setter method (if any -- may be null)
|
||||
if (pd instanceof IndexedPropertyDescriptor) {
|
||||
this.addOrUpdatePropertyDescriptor(pd.getName(), pd.getReadMethod(), pd.getWriteMethod(), ((IndexedPropertyDescriptor)pd).getIndexedReadMethod(), ((IndexedPropertyDescriptor)pd).getIndexedWriteMethod());
|
||||
this.addOrUpdatePropertyDescriptor(pd, pd.getName(), pd.getReadMethod(), pd.getWriteMethod(), ((IndexedPropertyDescriptor)pd).getIndexedReadMethod(), ((IndexedPropertyDescriptor)pd).getIndexedWriteMethod());
|
||||
} else {
|
||||
this.addOrUpdatePropertyDescriptor(pd.getName(), pd.getReadMethod(), pd.getWriteMethod());
|
||||
this.addOrUpdatePropertyDescriptor(pd, pd.getName(), pd.getReadMethod(), pd.getWriteMethod());
|
||||
}
|
||||
continue ALL_METHODS;
|
||||
}
|
||||
|
|
@ -174,11 +184,13 @@ class ExtendedBeanInfo implements BeanInfo {
|
|||
}
|
||||
}
|
||||
|
||||
private void addOrUpdatePropertyDescriptor(String propertyName, Method readMethod, Method writeMethod) throws IntrospectionException {
|
||||
addOrUpdatePropertyDescriptor(propertyName, readMethod, writeMethod, null, null);
|
||||
private void addOrUpdatePropertyDescriptor(PropertyDescriptor pd, String propertyName, Method readMethod, Method writeMethod) throws IntrospectionException {
|
||||
addOrUpdatePropertyDescriptor(pd, propertyName, readMethod, writeMethod, null, null);
|
||||
}
|
||||
|
||||
private void addOrUpdatePropertyDescriptor(String propertyName, Method readMethod, Method writeMethod, Method indexedReadMethod, Method indexedWriteMethod) throws IntrospectionException {
|
||||
private void addOrUpdatePropertyDescriptor(PropertyDescriptor pd, String propertyName, Method readMethod, Method writeMethod, Method indexedReadMethod, Method indexedWriteMethod) throws IntrospectionException {
|
||||
Assert.notNull(propertyName, "propertyName may not be null");
|
||||
propertyName = pd == null ? propertyName : pd.getName();
|
||||
for (PropertyDescriptor existingPD : this.propertyDescriptors) {
|
||||
if (existingPD.getName().equals(propertyName)) {
|
||||
// is there already a descriptor that captures this read method or its corresponding write method?
|
||||
|
|
@ -256,10 +268,32 @@ class ExtendedBeanInfo implements BeanInfo {
|
|||
}
|
||||
|
||||
// we haven't yet seen read or write methods for this property -> add a new descriptor
|
||||
if (indexedReadMethod == null && indexedWriteMethod == null) {
|
||||
this.propertyDescriptors.add(new PropertyDescriptor(propertyName, readMethod, writeMethod));
|
||||
} else {
|
||||
this.propertyDescriptors.add(new IndexedPropertyDescriptor(propertyName, readMethod, writeMethod, indexedReadMethod, indexedWriteMethod));
|
||||
if (pd == null) {
|
||||
try {
|
||||
if (indexedReadMethod == null && indexedWriteMethod == null) {
|
||||
pd = new PropertyDescriptor(propertyName, readMethod, writeMethod);
|
||||
}
|
||||
else {
|
||||
pd = new IndexedPropertyDescriptor(propertyName, readMethod, writeMethod, indexedReadMethod, indexedWriteMethod);
|
||||
}
|
||||
this.propertyDescriptors.add(pd);
|
||||
} catch (IntrospectionException ex) {
|
||||
logger.warn(format("Could not create new PropertyDescriptor for readMethod [%s] writeMethod [%s] " +
|
||||
"indexedReadMethod [%s] indexedWriteMethod [%s] for property [%s]. Reason: %s",
|
||||
readMethod, writeMethod, indexedReadMethod, indexedWriteMethod, propertyName, ex.getMessage()));
|
||||
// suppress exception and attempt to continue
|
||||
}
|
||||
}
|
||||
else {
|
||||
pd.setReadMethod(readMethod);
|
||||
try {
|
||||
pd.setWriteMethod(writeMethod);
|
||||
} catch (IntrospectionException ex) {
|
||||
logger.warn(format("Could not add write method [%s] for property [%s]. Reason: %s",
|
||||
writeMethod, propertyName, ex.getMessage()));
|
||||
// fall through -> add property descriptor as best we can
|
||||
}
|
||||
this.propertyDescriptors.add(pd);
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -16,6 +16,7 @@
|
|||
|
||||
package org.springframework.beans;
|
||||
|
||||
import static java.lang.String.format;
|
||||
import static org.hamcrest.CoreMatchers.equalTo;
|
||||
import static org.hamcrest.CoreMatchers.instanceOf;
|
||||
import static org.hamcrest.CoreMatchers.is;
|
||||
|
|
@ -29,7 +30,9 @@ import java.beans.IndexedPropertyDescriptor;
|
|||
import java.beans.IntrospectionException;
|
||||
import java.beans.Introspector;
|
||||
import java.beans.PropertyDescriptor;
|
||||
import java.lang.reflect.Method;
|
||||
|
||||
import org.junit.Ignore;
|
||||
import org.junit.Test;
|
||||
import org.springframework.beans.ExtendedBeanInfo.PropertyDescriptorComparator;
|
||||
|
||||
|
|
@ -116,11 +119,15 @@ public class ExtendedBeanInfoTests {
|
|||
}
|
||||
|
||||
BeanInfo bi = Introspector.getBeanInfo(C.class);
|
||||
ExtendedBeanInfo ebi = new ExtendedBeanInfo(bi);
|
||||
|
||||
assertThat(hasReadMethodForProperty(bi, "foo"), is(true));
|
||||
assertThat(hasWriteMethodForProperty(bi, "foo"), is(false));
|
||||
|
||||
ExtendedBeanInfo ebi = new ExtendedBeanInfo(bi);
|
||||
|
||||
assertThat(hasReadMethodForProperty(bi, "foo"), is(true));
|
||||
assertThat(hasWriteMethodForProperty(bi, "foo"), is(true));
|
||||
|
||||
assertThat(hasReadMethodForProperty(ebi, "foo"), is(true));
|
||||
assertThat(hasWriteMethodForProperty(ebi, "foo"), is(true));
|
||||
}
|
||||
|
|
@ -134,11 +141,15 @@ public class ExtendedBeanInfoTests {
|
|||
}
|
||||
|
||||
BeanInfo bi = Introspector.getBeanInfo(C.class);
|
||||
ExtendedBeanInfo ebi = new ExtendedBeanInfo(bi);
|
||||
|
||||
assertThat(hasReadMethodForProperty(bi, "foo"), is(true));
|
||||
assertThat(hasWriteMethodForProperty(bi, "foo"), is(false));
|
||||
|
||||
ExtendedBeanInfo ebi = new ExtendedBeanInfo(bi);
|
||||
|
||||
assertThat(hasReadMethodForProperty(bi, "foo"), is(true));
|
||||
assertThat(hasWriteMethodForProperty(bi, "foo"), is(true));
|
||||
|
||||
assertThat(hasReadMethodForProperty(ebi, "foo"), is(true));
|
||||
assertThat(hasWriteMethodForProperty(ebi, "foo"), is(true));
|
||||
|
||||
|
|
@ -161,11 +172,15 @@ public class ExtendedBeanInfoTests {
|
|||
}
|
||||
|
||||
BeanInfo bi = Introspector.getBeanInfo(C.class);
|
||||
ExtendedBeanInfo ebi = new ExtendedBeanInfo(bi);
|
||||
|
||||
assertThat(hasReadMethodForProperty(bi, "foo"), is(true));
|
||||
assertThat(hasWriteMethodForProperty(bi, "foo"), is(false));
|
||||
|
||||
ExtendedBeanInfo ebi = new ExtendedBeanInfo(bi);
|
||||
|
||||
assertThat(hasReadMethodForProperty(bi, "foo"), is(true));
|
||||
assertThat(hasWriteMethodForProperty(bi, "foo"), is(true));
|
||||
|
||||
assertThat(hasReadMethodForProperty(ebi, "foo"), is(true));
|
||||
assertThat(hasWriteMethodForProperty(ebi, "foo"), is(true));
|
||||
}
|
||||
|
|
@ -200,7 +215,6 @@ public class ExtendedBeanInfoTests {
|
|||
assertThat(c.getBar(), is(42));
|
||||
|
||||
BeanInfo bi = Introspector.getBeanInfo(C.class);
|
||||
ExtendedBeanInfo ebi = new ExtendedBeanInfo(bi);
|
||||
|
||||
assertThat(hasReadMethodForProperty(bi, "foo"), is(true));
|
||||
assertThat(hasWriteMethodForProperty(bi, "foo"), is(false));
|
||||
|
|
@ -208,6 +222,14 @@ public class ExtendedBeanInfoTests {
|
|||
assertThat(hasReadMethodForProperty(bi, "bar"), is(true));
|
||||
assertThat(hasWriteMethodForProperty(bi, "bar"), is(false));
|
||||
|
||||
ExtendedBeanInfo ebi = new ExtendedBeanInfo(bi);
|
||||
|
||||
assertThat(hasReadMethodForProperty(bi, "foo"), is(true));
|
||||
assertThat(hasWriteMethodForProperty(bi, "foo"), is(true));
|
||||
|
||||
assertThat(hasReadMethodForProperty(bi, "bar"), is(true));
|
||||
assertThat(hasWriteMethodForProperty(bi, "bar"), is(true));
|
||||
|
||||
assertThat(hasReadMethodForProperty(ebi, "foo"), is(true));
|
||||
assertThat(hasWriteMethodForProperty(ebi, "foo"), is(true));
|
||||
|
||||
|
|
@ -430,13 +452,19 @@ public class ExtendedBeanInfoTests {
|
|||
}
|
||||
|
||||
BeanInfo bi = Introspector.getBeanInfo(C.class);
|
||||
BeanInfo ebi = new ExtendedBeanInfo(Introspector.getBeanInfo(C.class));
|
||||
|
||||
assertThat(hasIndexedReadMethodForProperty(bi, "foos"), is(true));
|
||||
assertThat(hasWriteMethodForProperty(bi, "foos"), is(false));
|
||||
// again as above, standard Inspector picks up non-void return types on indexed write methods by default
|
||||
assertThat(hasIndexedWriteMethodForProperty(bi, "foos"), is(true));
|
||||
|
||||
BeanInfo ebi = new ExtendedBeanInfo(Introspector.getBeanInfo(C.class));
|
||||
|
||||
assertThat(hasIndexedReadMethodForProperty(bi, "foos"), is(true));
|
||||
assertThat(hasWriteMethodForProperty(bi, "foos"), is(true));
|
||||
// again as above, standard Inspector picks up non-void return types on indexed write methods by default
|
||||
assertThat(hasIndexedWriteMethodForProperty(bi, "foos"), is(true));
|
||||
|
||||
assertThat(hasIndexedReadMethodForProperty(ebi, "foos"), is(true));
|
||||
assertThat(hasWriteMethodForProperty(ebi, "foos"), is(true));
|
||||
assertThat(hasIndexedWriteMethodForProperty(ebi, "foos"), is(true));
|
||||
|
|
@ -454,11 +482,15 @@ public class ExtendedBeanInfoTests {
|
|||
}
|
||||
|
||||
BeanInfo bi = Introspector.getBeanInfo(C.class);
|
||||
ExtendedBeanInfo ebi = new ExtendedBeanInfo(bi);
|
||||
|
||||
assertThat(hasReadMethodForProperty(bi, "foo"), is(true));
|
||||
assertThat(hasWriteMethodForProperty(bi, "foo"), is(false));
|
||||
|
||||
ExtendedBeanInfo ebi = new ExtendedBeanInfo(bi);
|
||||
|
||||
assertThat(hasReadMethodForProperty(bi, "foo"), is(true));
|
||||
assertThat(hasWriteMethodForProperty(bi, "foo"), is(true));
|
||||
|
||||
assertThat(hasReadMethodForProperty(ebi, "foo"), is(true));
|
||||
assertThat(hasWriteMethodForProperty(ebi, "foo"), is(true));
|
||||
|
||||
|
|
@ -514,13 +546,19 @@ public class ExtendedBeanInfoTests {
|
|||
public Object setDateFormat(int dateStyle, int timeStyle) { return new Object(); }
|
||||
}
|
||||
BeanInfo bi = Introspector.getBeanInfo(C.class);
|
||||
ExtendedBeanInfo ebi = new ExtendedBeanInfo(bi);
|
||||
|
||||
assertThat(hasReadMethodForProperty(bi, "dateFormat"), is(false));
|
||||
assertThat(hasWriteMethodForProperty(bi, "dateFormat"), is(false));
|
||||
assertThat(hasIndexedReadMethodForProperty(bi, "dateFormat"), is(false));
|
||||
assertThat(hasIndexedWriteMethodForProperty(bi, "dateFormat"), is(true));
|
||||
|
||||
ExtendedBeanInfo ebi = new ExtendedBeanInfo(bi);
|
||||
|
||||
assertThat(hasReadMethodForProperty(bi, "dateFormat"), is(false));
|
||||
assertThat(hasWriteMethodForProperty(bi, "dateFormat"), is(true));
|
||||
assertThat(hasIndexedReadMethodForProperty(bi, "dateFormat"), is(false));
|
||||
assertThat(hasIndexedWriteMethodForProperty(bi, "dateFormat"), is(true));
|
||||
|
||||
assertThat(hasReadMethodForProperty(ebi, "dateFormat"), is(false));
|
||||
assertThat(hasWriteMethodForProperty(ebi, "dateFormat"), is(true));
|
||||
assertThat(hasIndexedReadMethodForProperty(ebi, "dateFormat"), is(false));
|
||||
|
|
@ -625,4 +663,75 @@ public class ExtendedBeanInfoTests {
|
|||
return false;
|
||||
}
|
||||
|
||||
@Test
|
||||
public void reproSpr8806_y() throws IntrospectionException, SecurityException, NoSuchMethodException {
|
||||
Introspector.getBeanInfo(LawLibrary.class);
|
||||
}
|
||||
|
||||
@Ignore @Test
|
||||
public void reproSpr8806_x() throws IntrospectionException, SecurityException, NoSuchMethodException {
|
||||
BeanInfo info = Introspector.getBeanInfo(LawLibrary.class);
|
||||
for (PropertyDescriptor d : info.getPropertyDescriptors()) {
|
||||
if (d.getName().equals("book")) {
|
||||
Method readMethod = d.getReadMethod();
|
||||
Method writeMethod = d.getWriteMethod();
|
||||
System.out.println(format("READ : %s.%s (bridge:%s)",
|
||||
readMethod.getDeclaringClass().getSimpleName(), readMethod.getName(), readMethod.isBridge()));
|
||||
System.out.println(format("WRITE: %s.%s (bridge:%s)",
|
||||
writeMethod.getDeclaringClass().getSimpleName(), writeMethod.getName(), writeMethod.isBridge()));
|
||||
new PropertyDescriptor("book", readMethod, writeMethod);
|
||||
}
|
||||
}
|
||||
|
||||
Method readMethod = LawLibrary.class.getMethod("getBook");
|
||||
Method writeMethod = LawLibrary.class.getMethod("setBook", Book.class);
|
||||
|
||||
System.out.println(format("read : %s.%s (bridge:%s)",
|
||||
readMethod.getDeclaringClass().getSimpleName(), readMethod.getName(), readMethod.isBridge()));
|
||||
System.out.println(format("write: %s.%s (bridge:%s)",
|
||||
writeMethod.getDeclaringClass().getSimpleName(), writeMethod.getName(), writeMethod.isBridge()));
|
||||
|
||||
|
||||
System.out.println("--------");
|
||||
for (Method m : LawLibrary.class.getMethods()) {
|
||||
if (m.getDeclaringClass() == Object.class) continue;
|
||||
System.out.println(format("%s %s.%s(%s) [bridge:%s]",
|
||||
m.getReturnType().getSimpleName(), m.getDeclaringClass().getSimpleName(),
|
||||
m.getName(),
|
||||
m.getParameterTypes().length == 1 ? m.getParameterTypes()[0].getSimpleName() : "",
|
||||
m.isBridge()));
|
||||
}
|
||||
|
||||
//new ExtendedBeanInfo(info);
|
||||
}
|
||||
|
||||
@Test
|
||||
public void reproSpr8806() throws IntrospectionException {
|
||||
BeanInfo bi = Introspector.getBeanInfo(LawLibrary.class);
|
||||
new ExtendedBeanInfo(bi); // throws
|
||||
}
|
||||
|
||||
interface Book { }
|
||||
|
||||
interface TextBook extends Book { }
|
||||
|
||||
interface LawBook extends TextBook { }
|
||||
|
||||
interface BookOperations {
|
||||
Book getBook();
|
||||
void setBook(Book book);
|
||||
}
|
||||
|
||||
interface TextBookOperations extends BookOperations {
|
||||
TextBook getBook();
|
||||
}
|
||||
|
||||
abstract class Library {
|
||||
public Book getBook() { return null; }
|
||||
public void setBook(Book book) { }
|
||||
}
|
||||
|
||||
class LawLibrary extends Library implements TextBookOperations {
|
||||
public LawBook getBook() { return null; }
|
||||
}
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in New Issue