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

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;
 

spring-beans/src/test/java/org/springframework/beans/DummyBeanInfoFactory.java → spring-beans/src/main/java/org/springframework/beans/ExtendedBeanInfoFactory.java

@@ -17,25 +17,57 @@
 package org.springframework.beans;
 
 import java.beans.BeanInfo;
-import java.beans.PropertyDescriptor;
-import java.beans.SimpleBeanInfo;
+import java.beans.IntrospectionException;
+import java.beans.Introspector;
 
-public class DummyBeanInfoFactory implements BeanInfoFactory {
+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}.
+ *
+ * <p>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) {
-		return CachedIntrospectionResultsTests.class.equals(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;
 	}
 
-	public BeanInfo getBeanInfo(Class<?> beanClass) {
-		return new DummyBeanInfo();
+	/**
+	 * Return a new {@link ExtendedBeanInfo} for the given bean class.
+	 */
+	public BeanInfo getBeanInfo(Class<?> beanClass) throws IntrospectionException {
+		return new ExtendedBeanInfo(Introspector.getBeanInfo(beanClass));
 	}
 
-	public static class DummyBeanInfo extends SimpleBeanInfo {
-
-		@Override
-		public PropertyDescriptor[] getPropertyDescriptors() {
-			return new PropertyDescriptor[0];
-		}
+	public int getOrder() {
+		return Ordered.LOWEST_PRECEDENCE;
 	}
 
 }

spring-beans/src/main/resources/META-INF/spring.beanInfoFactories

+org.springframework.beans.ExtendedBeanInfoFactory

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; }
+		}
+
+		// 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;
+			}
+		}
 
-		assertTrue("Invalid BeanInfo instance", beanInfo instanceof DummyBeanInfoFactory.DummyBeanInfo);
+		// 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)));
 	}
 
 }

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

+/*
+ * 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));
+	}
+
+}

spring-beans/src/test/resources/META-INF/spring.beanInfoFactories

-# Dummy bean info factories file, used by CachedIntrospectionResultsTests
-
-org.springframework.beans.DummyBeanInfoFactory
\ No newline at end of file