From 5bcf68e25a0c5a4e33772cbbd5c8885b7dcfabaf Mon Sep 17 00:00:00 2001 From: Chris Beams Date: Sun, 9 Sep 2012 16:04:40 +0200 Subject: [PATCH] Use ExtendedBeanInfo on an as-needed basis only Prior to this change, CachedIntrospectionResults delegated to ExtendedBeanInfo by default in order to inspect JavaBean PropertyDescriptor information for bean classes. Originally introduced with SPR-8079, ExtendedBeanInfo was designed to go beyond the capabilities of the default JavaBeans Introspector in order to support non-void returning setter methods, principally to support use of builder-style APIs within Spring XML. This is a complex affair, and the non-trivial logic in ExtendedBeanInfo has led to various bugs including regressions for bean classes that do not declare non-void returning setters. This commit takes advantage of the new BeanInfoFactory mechanism introduced in SPR-9677 to take ExtendedBeanInfo out of the default code path for CachedIntrospectionResults. Now, the new ExtendedBeanInfoFactory class will be detected and instantiated (per its entry in the META-INF/spring.beanInfoFactories properties file shipped with the spring-beans jar). ExtendedBeanInfoFactory#supports is invoked for all bean classes in order to determine whether they are candidates for ExtendedBeanInfo introspection, i.e. whether they declare non-void returning setter methods. If a class does not declare any such non-standard setter methods (the 99% case), then CachedIntrospectionResults will fall back to the default JavaBeans Introspector. While efforts have been made to fix any bugs with ExtendedBeanInfo, this change means that EBI will not pose any future risk for bean classes that do not declare non-standard setter methods, and also means greater efficiency in general. Issue: SPR-9723, SPR-9677, SPR-8079 --- .../beans/CachedIntrospectionResults.java | 4 +- .../beans/ExtendedBeanInfoFactory.java | 73 +++++++++++++++ .../META-INF/spring.beanInfoFactories | 1 + .../CachedIntrospectionResultsTests.java | 33 ++++++- .../beans/DummyBeanInfoFactory.java | 41 --------- .../beans/ExtendedBeanInfoFactoryTests.java | 89 +++++++++++++++++++ .../META-INF/spring.beanInfoFactories | 3 - 7 files changed, 194 insertions(+), 50 deletions(-) create mode 100644 spring-beans/src/main/java/org/springframework/beans/ExtendedBeanInfoFactory.java create mode 100644 spring-beans/src/main/resources/META-INF/spring.beanInfoFactories delete mode 100644 spring-beans/src/test/java/org/springframework/beans/DummyBeanInfoFactory.java create mode 100644 spring-beans/src/test/java/org/springframework/beans/ExtendedBeanInfoFactoryTests.java delete mode 100644 spring-beans/src/test/resources/META-INF/spring.beanInfoFactories diff --git a/spring-beans/src/main/java/org/springframework/beans/CachedIntrospectionResults.java b/spring-beans/src/main/java/org/springframework/beans/CachedIntrospectionResults.java index a06edf17c1..c0da75ffcc 100644 --- a/spring-beans/src/main/java/org/springframework/beans/CachedIntrospectionResults.java +++ b/spring-beans/src/main/java/org/springframework/beans/CachedIntrospectionResults.java @@ -248,8 +248,8 @@ public class CachedIntrospectionResults { } } if (beanInfo == null) { - // If none of the factories supported the class, use the default - beanInfo = new ExtendedBeanInfo(Introspector.getBeanInfo(beanClass)); + // If none of the factories supported the class, fall back to the default + beanInfo = Introspector.getBeanInfo(beanClass); } this.beanInfo = beanInfo; diff --git a/spring-beans/src/main/java/org/springframework/beans/ExtendedBeanInfoFactory.java b/spring-beans/src/main/java/org/springframework/beans/ExtendedBeanInfoFactory.java new file mode 100644 index 0000000000..c06d53d0ea --- /dev/null +++ b/spring-beans/src/main/java/org/springframework/beans/ExtendedBeanInfoFactory.java @@ -0,0 +1,73 @@ +/* + * Copyright 2002-2012 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. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.beans; + +import java.beans.BeanInfo; +import java.beans.IntrospectionException; +import java.beans.Introspector; + +import java.lang.reflect.Method; +import java.lang.reflect.Modifier; + +import org.springframework.core.Ordered; + +/** + * {@link BeanInfoFactory} implementation that evaluates whether bean classes have + * "non-standard" JavaBeans setter methods and are thus candidates for introspection by + * Spring's {@link ExtendedBeanInfo}. + * + *

Ordered at {@link Ordered#LOWEST_PRECEDENCE} to allow other user-defined + * {@link BeanInfoFactory} types to take precedence. + * + * @author Chris Beams + * @since 3.2 + * @see BeanInfoFactory + */ +class ExtendedBeanInfoFactory implements Ordered, BeanInfoFactory { + + /** + * Return whether the given bean class declares or inherits any non-void returning + * JavaBeans setter methods. + */ + public boolean supports(Class beanClass) { + for (Method method : beanClass.getMethods()) { + String methodName = method.getName(); + Class[] parameterTypes = method.getParameterTypes(); + if (Modifier.isPublic(method.getModifiers()) + && methodName.length() > 3 + && methodName.startsWith("set") + && (parameterTypes.length == 1 + || (parameterTypes.length == 2 && parameterTypes[0].equals(int.class))) + && !void.class.isAssignableFrom(method.getReturnType())) { + return true; + } + } + return false; + } + + /** + * Return a new {@link ExtendedBeanInfo} for the given bean class. + */ + public BeanInfo getBeanInfo(Class beanClass) throws IntrospectionException { + return new ExtendedBeanInfo(Introspector.getBeanInfo(beanClass)); + } + + public int getOrder() { + return Ordered.LOWEST_PRECEDENCE; + } + +} diff --git a/spring-beans/src/main/resources/META-INF/spring.beanInfoFactories b/spring-beans/src/main/resources/META-INF/spring.beanInfoFactories new file mode 100644 index 0000000000..7a104edec2 --- /dev/null +++ b/spring-beans/src/main/resources/META-INF/spring.beanInfoFactories @@ -0,0 +1 @@ +org.springframework.beans.ExtendedBeanInfoFactory diff --git a/spring-beans/src/test/java/org/springframework/beans/CachedIntrospectionResultsTests.java b/spring-beans/src/test/java/org/springframework/beans/CachedIntrospectionResultsTests.java index e5a6a0f82c..4675d22dbf 100644 --- a/spring-beans/src/test/java/org/springframework/beans/CachedIntrospectionResultsTests.java +++ b/spring-beans/src/test/java/org/springframework/beans/CachedIntrospectionResultsTests.java @@ -17,6 +17,7 @@ package org.springframework.beans; import java.beans.BeanInfo; +import java.beans.PropertyDescriptor; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; @@ -25,6 +26,9 @@ import test.beans.TestBean; import org.springframework.core.OverridingClassLoader; +import static org.hamcrest.CoreMatchers.*; +import static org.junit.Assert.*; + /** * @author Juergen Hoeller * @author Chris Beams @@ -54,11 +58,32 @@ public final class CachedIntrospectionResultsTests { } @Test - public void customBeanInfoFactory() throws Exception { - CachedIntrospectionResults results = CachedIntrospectionResults.forClass(CachedIntrospectionResultsTests.class); - BeanInfo beanInfo = results.getBeanInfo(); + public void shouldUseExtendedBeanInfoWhenApplicable() throws NoSuchMethodException, SecurityException { + // given a class with a non-void returning setter method + @SuppressWarnings("unused") + class C { + public Object setFoo(String s) { return this; } + public String getFoo() { return null; } + } - assertTrue("Invalid BeanInfo instance", beanInfo instanceof DummyBeanInfoFactory.DummyBeanInfo); + // CachedIntrospectionResults should delegate to ExtendedBeanInfo + CachedIntrospectionResults results = CachedIntrospectionResults.forClass(C.class); + BeanInfo info = results.getBeanInfo(); + PropertyDescriptor pd = null; + for (PropertyDescriptor candidate : info.getPropertyDescriptors()) { + if (candidate.getName().equals("foo")) { + pd = candidate; + } + } + + // resulting in a property descriptor including the non-standard setFoo method + assertThat(pd, notNullValue()); + assertThat(pd.getReadMethod(), equalTo(C.class.getMethod("getFoo"))); + assertThat( + "No write method found for non-void returning 'setFoo' method. " + + "Check to see if CachedIntrospectionResults is delegating to " + + "ExtendedBeanInfo as expected", + pd.getWriteMethod(), equalTo(C.class.getMethod("setFoo", String.class))); } } diff --git a/spring-beans/src/test/java/org/springframework/beans/DummyBeanInfoFactory.java b/spring-beans/src/test/java/org/springframework/beans/DummyBeanInfoFactory.java deleted file mode 100644 index ef52db2771..0000000000 --- a/spring-beans/src/test/java/org/springframework/beans/DummyBeanInfoFactory.java +++ /dev/null @@ -1,41 +0,0 @@ -/* - * Copyright 2002-2012 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. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package org.springframework.beans; - -import java.beans.BeanInfo; -import java.beans.PropertyDescriptor; -import java.beans.SimpleBeanInfo; - -public class DummyBeanInfoFactory implements BeanInfoFactory { - - public boolean supports(Class beanClass) { - return CachedIntrospectionResultsTests.class.equals(beanClass); - } - - public BeanInfo getBeanInfo(Class beanClass) { - return new DummyBeanInfo(); - } - - public static class DummyBeanInfo extends SimpleBeanInfo { - - @Override - public PropertyDescriptor[] getPropertyDescriptors() { - return new PropertyDescriptor[0]; - } - } - -} diff --git a/spring-beans/src/test/java/org/springframework/beans/ExtendedBeanInfoFactoryTests.java b/spring-beans/src/test/java/org/springframework/beans/ExtendedBeanInfoFactoryTests.java new file mode 100644 index 0000000000..d56b672e5b --- /dev/null +++ b/spring-beans/src/test/java/org/springframework/beans/ExtendedBeanInfoFactoryTests.java @@ -0,0 +1,89 @@ +/* + * Copyright 2002-2012 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. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.beans; + +import java.beans.IntrospectionException; + +import org.junit.Test; + +import static org.hamcrest.CoreMatchers.*; +import static org.junit.Assert.*; + +/** + * Unit tests for {@link ExtendedBeanInfoTests}. + * + * @author Chris Beams + */ +public class ExtendedBeanInfoFactoryTests { + + private ExtendedBeanInfoFactory factory = new ExtendedBeanInfoFactory(); + + @Test + public void shouldNotSupportClassHavingOnlyVoidReturningSetter() throws IntrospectionException { + @SuppressWarnings("unused") + class C { + public void setFoo(String s) { } + } + assertThat(factory.supports(C.class), is(false)); + } + + @Test + public void shouldSupportClassHavingNonVoidReturningSetter() throws IntrospectionException { + @SuppressWarnings("unused") + class C { + public C setFoo(String s) { return this; } + } + assertThat(factory.supports(C.class), is(true)); + } + + @Test + public void shouldSupportClassHavingNonVoidReturningIndexedSetter() throws IntrospectionException { + @SuppressWarnings("unused") + class C { + public C setFoo(int i, String s) { return this; } + } + assertThat(factory.supports(C.class), is(true)); + } + + @Test + public void shouldNotSupportClassHavingNonPublicNonVoidReturningIndexedSetter() throws IntrospectionException { + @SuppressWarnings("unused") + class C { + void setBar(String s) { } + } + assertThat(factory.supports(C.class), is(false)); + } + + @Test + public void shouldNotSupportClassHavingNonVoidReturningParameterlessSetter() throws IntrospectionException { + @SuppressWarnings("unused") + class C { + C setBar() { return this; } + } + assertThat(factory.supports(C.class), is(false)); + } + + @Test + public void shouldNotSupportClassHavingNonVoidReturningMethodNamedSet() throws IntrospectionException { + @SuppressWarnings("unused") + class C { + C set(String s) { return this; } + } + assertThat(factory.supports(C.class), is(false)); + } + +} diff --git a/spring-beans/src/test/resources/META-INF/spring.beanInfoFactories b/spring-beans/src/test/resources/META-INF/spring.beanInfoFactories deleted file mode 100644 index c2a8c64a4e..0000000000 --- a/spring-beans/src/test/resources/META-INF/spring.beanInfoFactories +++ /dev/null @@ -1,3 +0,0 @@ -# Dummy bean info factories file, used by CachedIntrospectionResultsTests - -org.springframework.beans.DummyBeanInfoFactory \ No newline at end of file