From 7b999c676f855fd208fcd56943b3ca48233d7f3a Mon Sep 17 00:00:00 2001 From: Chris Beams Date: Fri, 6 May 2011 19:07:25 +0000 Subject: [PATCH] Introduce ReflectionUtils#getUniqueDeclaredMethods This change is in support of certain polymorphism cases in @Configuration class inheritance hierarchies. Consider the following scenario: @Configuration public abstract class AbstractConfig { public abstract Object bean(); } @Configuration public class ConcreteConfig { @Override @Bean public BeanPostProcessor bean() { ... } } ConcreteConfig overrides AbstractConfig's #bean() method with a covariant return type, in this case returning an object of type BeanPostProcessor. It is critically important that the container is able to detect the return type of ConcreteConfig#bean() in order to instantiate the BPP at the right point in the lifecycle. Prior to this change, the container could not do this. AbstractAutowireCapableBeanFactory#getTypeForFactoryMethod called ReflectionUtils#getAllDeclaredMethods, which returned Method objects for both the Object and BeanPostProcessor signatures of the #bean() method. This confused the implementation sufficiently as not to choose a type for the factory method at all. This means that the BPP never gets detected as a BPP. The new method being introduced here, #getUniqueDeclaredMethods, takes covariant return types into account, and filters out duplicates, favoring the most specific / narrow return type. Additionally, it filters out any CGLIB 'rewritten' methods, which is important in the case of @Configuration classes, which are enhanced by CGLIB. See the implementation for further details. --- .../springframework/aop/support/AopUtils.java | 2 + .../AbstractAutowireCapableBeanFactory.java | 2 +- .../ReflectionUtilsIntegrationTests.java | 74 ++++++++++++++++ .../springframework/util/ReflectionUtils.java | 50 +++++++++++ .../util/ReflectionUtilsTests.java | 86 ++++++++++++++++++- 5 files changed, 212 insertions(+), 2 deletions(-) create mode 100644 org.springframework.context/src/test/java/org/springframework/context/annotation/ReflectionUtilsIntegrationTests.java diff --git a/org.springframework.aop/src/main/java/org/springframework/aop/support/AopUtils.java b/org.springframework.aop/src/main/java/org/springframework/aop/support/AopUtils.java index ce15f9282c..efb771f74d 100644 --- a/org.springframework.aop/src/main/java/org/springframework/aop/support/AopUtils.java +++ b/org.springframework.aop/src/main/java/org/springframework/aop/support/AopUtils.java @@ -23,6 +23,8 @@ import java.util.HashSet; import java.util.LinkedList; import java.util.List; import java.util.Set; +import java.util.regex.Matcher; +import java.util.regex.Pattern; import org.springframework.aop.Advisor; import org.springframework.aop.AopInvocationException; diff --git a/org.springframework.beans/src/main/java/org/springframework/beans/factory/support/AbstractAutowireCapableBeanFactory.java b/org.springframework.beans/src/main/java/org/springframework/beans/factory/support/AbstractAutowireCapableBeanFactory.java index 75bdd689d5..1ea58c18c9 100644 --- a/org.springframework.beans/src/main/java/org/springframework/beans/factory/support/AbstractAutowireCapableBeanFactory.java +++ b/org.springframework.beans/src/main/java/org/springframework/beans/factory/support/AbstractAutowireCapableBeanFactory.java @@ -631,7 +631,7 @@ public abstract class AbstractAutowireCapableBeanFactory extends AbstractBeanFac // If all factory methods have the same return type, return that type. // Can't clearly figure out exact method due to type converting / autowiring! int minNrOfArgs = mbd.getConstructorArgumentValues().getArgumentCount(); - Method[] candidates = ReflectionUtils.getAllDeclaredMethods(factoryClass); + Method[] candidates = ReflectionUtils.getUniqueDeclaredMethods(factoryClass); Set returnTypes = new HashSet(1); for (Method factoryMethod : candidates) { if (Modifier.isStatic(factoryMethod.getModifiers()) == isStatic && diff --git a/org.springframework.context/src/test/java/org/springframework/context/annotation/ReflectionUtilsIntegrationTests.java b/org.springframework.context/src/test/java/org/springframework/context/annotation/ReflectionUtilsIntegrationTests.java new file mode 100644 index 0000000000..8f39b5f887 --- /dev/null +++ b/org.springframework.context/src/test/java/org/springframework/context/annotation/ReflectionUtilsIntegrationTests.java @@ -0,0 +1,74 @@ +/* + * Copyright 2002-2011 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.context.annotation; + +import static org.hamcrest.CoreMatchers.is; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertThat; + +import java.lang.reflect.Method; + +import org.junit.Test; +import org.springframework.beans.factory.support.DefaultListableBeanFactory; +import org.springframework.util.ReflectionUtils; + +/** + * Tests ReflectionUtils methods as used against CGLIB-generated classes created + * by ConfigurationClassEnhancer. + * + * @author Chris Beams + * @since 3.1 + * @see org.springframework.util.ReflectionUtilsTests + */ +public class ReflectionUtilsIntegrationTests { + + @Test + public void getUniqueDeclaredMethods_withCovariantReturnType_andCglibRewrittenMethodNames() throws Exception { + DefaultListableBeanFactory bf = new DefaultListableBeanFactory(); + Class cglibLeaf = new ConfigurationClassEnhancer(bf).enhance(Leaf.class); + int m1MethodCount = 0; + Method[] methods = ReflectionUtils.getUniqueDeclaredMethods(cglibLeaf); + for (Method method : methods) { + if (method.getName().equals("m1")) { + m1MethodCount++; + } + } + assertThat(m1MethodCount, is(1)); + for (Method method : methods) { + if (method.getName().contains("m1")) { + assertEquals(method.getReturnType(), Integer.class); + } + } + } + + + @Configuration + static abstract class Parent { + public abstract Number m1(); + } + + + @Configuration + static class Leaf extends Parent { + @Override + @Bean + public Integer m1() { + return new Integer(42); + } + } + +} diff --git a/org.springframework.core/src/main/java/org/springframework/util/ReflectionUtils.java b/org.springframework.core/src/main/java/org/springframework/util/ReflectionUtils.java index 8ce5083c7d..742126950a 100644 --- a/org.springframework.core/src/main/java/org/springframework/util/ReflectionUtils.java +++ b/org.springframework.core/src/main/java/org/springframework/util/ReflectionUtils.java @@ -26,6 +26,7 @@ import java.sql.SQLException; import java.util.ArrayList; import java.util.Arrays; import java.util.List; +import java.util.regex.Pattern; /** * Simple utility class for working with the reflection API and handling @@ -38,10 +39,13 @@ import java.util.List; * @author Rod Johnson * @author Costin Leau * @author Sam Brannen + * @author Chris Beams * @since 1.2.2 */ public abstract class ReflectionUtils { + private static final Pattern CGLIB_RENAMED_METHOD_PATTERN = Pattern.compile("CGLIB\\$(.+)\\$\\d+"); + /** * Attempt to find a {@link Field field} on the supplied {@link Class} with the * supplied name. Searches all superclasses up to {@link Object}. @@ -379,6 +383,16 @@ public abstract class ReflectionUtils { } } + /** + * Determine whether the given method is a CGLIB 'renamed' method, following + * the pattern "CGLIB$methodName$0". + * @param renamedMethod the method to check + * @see net.sf.cglib.proxy.Enhancer#rename + */ + public static boolean isCglibRenamedMethod(Method renamedMethod) { + return CGLIB_RENAMED_METHOD_PATTERN.matcher(renamedMethod.getName()).matches(); + } + /** * Make the given field accessible, explicitly setting it accessible if * necessary. The setAccessible(true) method is only called @@ -487,6 +501,42 @@ public abstract class ReflectionUtils { return methods.toArray(new Method[methods.size()]); } + /** + * Get the unique set of declared methods on the leaf class and all superclasses. Leaf + * class methods are included first and while traversing the superclass hierarchy any methods found + * with signatures matching a method already included are filtered out. + */ + public static Method[] getUniqueDeclaredMethods(Class leafClass) throws IllegalArgumentException { + final List methods = new ArrayList(32); + doWithMethods(leafClass, new MethodCallback() { + public void doWith(Method method) { + boolean knownSignature = false; + Method methodBeingOverriddenWithCovariantReturnType = null; + + for (Method existingMethod : methods) { + if (method.getName().equals(existingMethod.getName()) && + Arrays.equals(method.getParameterTypes(), existingMethod.getParameterTypes())) { + // is this a covariant return type situation? + if (existingMethod.getReturnType() != method.getReturnType() && + existingMethod.getReturnType().isAssignableFrom(method.getReturnType())) { + methodBeingOverriddenWithCovariantReturnType = existingMethod; + } else { + knownSignature = true; + } + break; + } + } + if (methodBeingOverriddenWithCovariantReturnType != null) { + methods.remove(methodBeingOverriddenWithCovariantReturnType); + } + if (!knownSignature && !isCglibRenamedMethod(method)) { + methods.add(method); + } + } + }); + return methods.toArray(new Method[methods.size()]); + } + /** * Invoke the given callback on all fields in the target class, going up the * class hierarchy to get all declared fields. diff --git a/org.springframework.core/src/test/java/org/springframework/util/ReflectionUtilsTests.java b/org.springframework.core/src/test/java/org/springframework/util/ReflectionUtilsTests.java index c883b2cd0d..f08e334f89 100644 --- a/org.springframework.core/src/test/java/org/springframework/util/ReflectionUtilsTests.java +++ b/org.springframework.core/src/test/java/org/springframework/util/ReflectionUtilsTests.java @@ -16,6 +16,9 @@ package org.springframework.util; +import static org.hamcrest.CoreMatchers.is; +import static org.junit.Assert.*; + import java.lang.reflect.Field; import java.lang.reflect.Method; import java.lang.reflect.Modifier; @@ -24,7 +27,6 @@ import java.rmi.RemoteException; import java.util.LinkedList; import java.util.List; -import static org.junit.Assert.*; import org.junit.Test; import org.springframework.beans.TestBean; @@ -238,6 +240,88 @@ public class ReflectionUtilsTests { assertNotNull(ReflectionUtils.findMethod(B.class, "getClass")); } + @Test + public void isCglibRenamedMethod() throws SecurityException, NoSuchMethodException { + @SuppressWarnings("unused") + class C { + public void CGLIB$m1$123() { } + public void CGLIB$m1$0() { } + public void CGLIB$$0() { } + public void CGLIB$m1$() { } + public void CGLIB$m1() { } + public void m1() { } + public void m1$() { } + public void m1$1() { } + } + assertTrue(ReflectionUtils.isCglibRenamedMethod(C.class.getMethod("CGLIB$m1$123"))); + assertTrue(ReflectionUtils.isCglibRenamedMethod(C.class.getMethod("CGLIB$m1$0"))); + assertFalse(ReflectionUtils.isCglibRenamedMethod(C.class.getMethod("CGLIB$$0"))); + assertFalse(ReflectionUtils.isCglibRenamedMethod(C.class.getMethod("CGLIB$m1$"))); + assertFalse(ReflectionUtils.isCglibRenamedMethod(C.class.getMethod("CGLIB$m1"))); + assertFalse(ReflectionUtils.isCglibRenamedMethod(C.class.getMethod("m1"))); + assertFalse(ReflectionUtils.isCglibRenamedMethod(C.class.getMethod("m1$"))); + assertFalse(ReflectionUtils.isCglibRenamedMethod(C.class.getMethod("m1$1"))); + } + + @Test + public void getAllDeclaredMethods() throws Exception { + class Foo { + @Override + public String toString() { + return super.toString(); + } + } + int toStringMethodCount = 0; + for (Method method : ReflectionUtils.getAllDeclaredMethods(Foo.class)) { + if (method.getName().equals("toString")) { + toStringMethodCount++; + } + } + assertThat(toStringMethodCount, is(2)); + } + + @Test + public void getUniqueDeclaredMethods() throws Exception { + class Foo { + @Override + public String toString() { + return super.toString(); + } + } + int toStringMethodCount = 0; + for (Method method : ReflectionUtils.getUniqueDeclaredMethods(Foo.class)) { + if (method.getName().equals("toString")) { + toStringMethodCount++; + } + } + assertThat(toStringMethodCount, is(1)); + } + + @Test + public void getUniqueDeclaredMethods_withCovariantReturnType() throws Exception { + class Parent { + public Number m1() { + return new Integer(42); + } + } + class Leaf extends Parent { + @Override + public Integer m1() { + return new Integer(42); + } + } + int m1MethodCount = 0; + Method[] methods = ReflectionUtils.getUniqueDeclaredMethods(Leaf.class); + for (Method method : methods) { + if (method.getName().equals("m1")) { + m1MethodCount++; + } + } + assertThat(m1MethodCount, is(1)); + assertTrue(ObjectUtils.containsElement(methods, Leaf.class.getMethod("m1"))); + assertFalse(ObjectUtils.containsElement(methods, Parent.class.getMethod("m1"))); + } + private static class ListSavingMethodCallback implements ReflectionUtils.MethodCallback {