From 77c93219670a789c7b380c85e5b2514c47fb920a Mon Sep 17 00:00:00 2001 From: Phillip Webb Date: Mon, 27 Aug 2012 13:24:58 -0700 Subject: [PATCH] Sort candidate @AspectJ methods deterministically Update the ReflectiveAspectJAdvisorFactory class to sort candidate AOP methods based on their annotation first and method name second. Prior to this the order of aspects created from annotated methods could differ depending on the underling JVM, as first noticed under JDK7 in SPR-9729. - ConvertingComparator and InstanceComparator have been introduced in support of this change, per SPR-9730. - A shared static INSTANCE field has been added to ComparableComparator to avoid unnecessary instantiation costs within ConvertingComparator as well as to prevent generics warnings during certain caller scenarios. Issue: SPR-9729, SPR-9730 --- .../ReflectiveAspectJAdvisorFactory.java | 68 ++++++-- .../AbstractAspectJAdvisorFactoryTests.java | 43 +++-- .../converter/ConvertingComparator.java | 145 ++++++++++++++++ .../util/comparator/ComparableComparator.java | 5 +- .../util/comparator/InstanceComparator.java | 72 ++++++++ .../converter/ConvertingComparatorTests.java | 159 ++++++++++++++++++ 6 files changed, 457 insertions(+), 35 deletions(-) create mode 100644 spring-core/src/main/java/org/springframework/core/convert/converter/ConvertingComparator.java create mode 100644 spring-core/src/main/java/org/springframework/util/comparator/InstanceComparator.java create mode 100644 spring-core/src/test/java/org/springframework/core/convert/converter/ConvertingComparatorTests.java diff --git a/spring-aop/src/main/java/org/springframework/aop/aspectj/annotation/ReflectiveAspectJAdvisorFactory.java b/spring-aop/src/main/java/org/springframework/aop/aspectj/annotation/ReflectiveAspectJAdvisorFactory.java index 2adede99794..c2e356678a7 100644 --- a/spring-aop/src/main/java/org/springframework/aop/aspectj/annotation/ReflectiveAspectJAdvisorFactory.java +++ b/spring-aop/src/main/java/org/springframework/aop/aspectj/annotation/ReflectiveAspectJAdvisorFactory.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2007 the original author or authors. + * 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. @@ -16,14 +16,20 @@ package org.springframework.aop.aspectj.annotation; +import java.lang.annotation.Annotation; import java.lang.reflect.Field; import java.lang.reflect.Method; +import java.util.Collections; +import java.util.Comparator; import java.util.LinkedList; import java.util.List; import org.aopalliance.aop.Advice; +import org.aspectj.lang.annotation.After; import org.aspectj.lang.annotation.AfterReturning; import org.aspectj.lang.annotation.AfterThrowing; +import org.aspectj.lang.annotation.Around; +import org.aspectj.lang.annotation.Before; import org.aspectj.lang.annotation.DeclareParents; import org.aspectj.lang.annotation.Pointcut; @@ -40,8 +46,12 @@ import org.springframework.aop.aspectj.DeclareParentsAdvisor; import org.springframework.aop.framework.AopConfigException; import org.springframework.aop.support.DefaultPointcutAdvisor; import org.springframework.core.annotation.AnnotationUtils; +import org.springframework.core.convert.converter.Converter; +import org.springframework.core.convert.converter.ConvertingComparator; import org.springframework.util.ReflectionUtils; import org.springframework.util.StringUtils; +import org.springframework.util.comparator.CompoundComparator; +import org.springframework.util.comparator.InstanceComparator; /** * Factory that can create Spring AOP Advisors given AspectJ classes from @@ -52,10 +62,34 @@ import org.springframework.util.StringUtils; * @author Adrian Colyer * @author Juergen Hoeller * @author Ramnivas Laddad + * @author Phillip Webb * @since 2.0 */ public class ReflectiveAspectJAdvisorFactory extends AbstractAspectJAdvisorFactory { + private static final Comparator METHOD_COMPARATOR; + + static { + CompoundComparator comparator = new CompoundComparator(); + comparator.addComparator(new ConvertingComparator( + new InstanceComparator( + Around.class, Before.class, After.class, AfterReturning.class, AfterThrowing.class), + new Converter() { + public Annotation convert(Method method) { + AspectJAnnotation annotation = AbstractAspectJAdvisorFactory.findAspectJAnnotationOnMethod(method); + return annotation == null ? null : annotation.getAnnotation(); + } + })); + comparator.addComparator(new ConvertingComparator( + new Converter() { + public String convert(Method method) { + return method.getName(); + } + })); + METHOD_COMPARATOR = comparator; + } + + public List getAdvisors(MetadataAwareAspectInstanceFactory maaif) { final Class aspectClass = maaif.getAspectMetadata().getAspectClass(); final String aspectName = maaif.getAspectMetadata().getAspectName(); @@ -67,17 +101,12 @@ public class ReflectiveAspectJAdvisorFactory extends AbstractAspectJAdvisorFacto new LazySingletonAspectInstanceFactoryDecorator(maaif); final List advisors = new LinkedList(); - ReflectionUtils.doWithMethods(aspectClass, new ReflectionUtils.MethodCallback() { - public void doWith(Method method) throws IllegalArgumentException { - // Exclude pointcuts - if (AnnotationUtils.getAnnotation(method, Pointcut.class) == null) { - Advisor advisor = getAdvisor(method, lazySingletonAspectInstanceFactory, advisors.size(), aspectName); - if (advisor != null) { - advisors.add(advisor); - } - } + for (Method method : getAdvisorMethods(aspectClass)) { + Advisor advisor = getAdvisor(method, lazySingletonAspectInstanceFactory, advisors.size(), aspectName); + if (advisor != null) { + advisors.add(advisor); } - }); + } // If it's a per target aspect, emit the dummy instantiating aspect. if (!advisors.isEmpty() && lazySingletonAspectInstanceFactory.getAspectMetadata().isLazilyInstantiated()) { @@ -96,6 +125,20 @@ public class ReflectiveAspectJAdvisorFactory extends AbstractAspectJAdvisorFacto return advisors; } + private List getAdvisorMethods(Class aspectClass) { + final List methods = new LinkedList(); + ReflectionUtils.doWithMethods(aspectClass, new ReflectionUtils.MethodCallback() { + public void doWith(Method method) throws IllegalArgumentException { + // Exclude pointcuts + if (AnnotationUtils.getAnnotation(method, Pointcut.class) == null) { + methods.add(method); + } + } + }); + Collections.sort(methods, METHOD_COMPARATOR); + return methods; + } + /** * Build a {@link org.springframework.aop.aspectj.DeclareParentsAdvisor} * for the given introduction field. @@ -104,7 +147,7 @@ public class ReflectiveAspectJAdvisorFactory extends AbstractAspectJAdvisorFacto * @return null if not an Advisor */ private Advisor getDeclareParentsAdvisor(Field introductionField) { - DeclareParents declareParents = (DeclareParents) introductionField.getAnnotation(DeclareParents.class); + DeclareParents declareParents = introductionField.getAnnotation(DeclareParents.class); if (declareParents == null) { // Not an introduction field return null; @@ -224,6 +267,7 @@ public class ReflectiveAspectJAdvisorFactory extends AbstractAspectJAdvisorFacto * Triggered by per-clause pointcut on non-singleton aspect. * The advice has no effect. */ + @SuppressWarnings("serial") protected static class SyntheticInstantiationAdvisor extends DefaultPointcutAdvisor { public SyntheticInstantiationAdvisor(final MetadataAwareAspectInstanceFactory aif) { diff --git a/spring-aop/src/test/java/org/springframework/aop/aspectj/annotation/AbstractAspectJAdvisorFactoryTests.java b/spring-aop/src/test/java/org/springframework/aop/aspectj/annotation/AbstractAspectJAdvisorFactoryTests.java index 285a6c2e594..9ad8b2dc0f2 100644 --- a/spring-aop/src/test/java/org/springframework/aop/aspectj/annotation/AbstractAspectJAdvisorFactoryTests.java +++ b/spring-aop/src/test/java/org/springframework/aop/aspectj/annotation/AbstractAspectJAdvisorFactoryTests.java @@ -39,7 +39,11 @@ import org.aspectj.lang.annotation.DeclareParents; import org.aspectj.lang.annotation.DeclarePrecedence; import org.aspectj.lang.annotation.Pointcut; import org.aspectj.lang.reflect.MethodSignature; + +import org.junit.Rule; import org.junit.Test; +import org.junit.rules.ExpectedException; + import org.springframework.aop.Advisor; import org.springframework.aop.aspectj.annotation.ReflectiveAspectJAdvisorFactory.SyntheticInstantiationAdvisor; import org.springframework.aop.framework.Advised; @@ -65,9 +69,13 @@ import test.beans.TestBean; * * @author Rod Johnson * @author Chris Beams + * @author Phillip Webb */ public abstract class AbstractAspectJAdvisorFactoryTests { - + + @Rule + public ExpectedException thrown = ExpectedException.none(); + /** * To be overridden by concrete test subclasses. * @return the fixture @@ -571,31 +579,22 @@ public abstract class AbstractAspectJAdvisorFactoryTests { @Test public void testFailureWithoutExplicitDeclarePrecedence() { TestBean target = new TestBean(); - ITestBean itb = (ITestBean) createProxy(target, - getFixture().getAdvisors(new SingletonMetadataAwareAspectInstanceFactory(new NoDeclarePrecedenceShouldFail(), "someBean")), - ITestBean.class); - try { - itb.getAge(); - fail(); - } - catch (IllegalStateException ex) { - // expected - } + MetadataAwareAspectInstanceFactory aspectInstanceFactory = new SingletonMetadataAwareAspectInstanceFactory( + new NoDeclarePrecedenceShouldFail(), "someBean"); + ITestBean itb = (ITestBean) createProxy(target, + getFixture().getAdvisors(aspectInstanceFactory), ITestBean.class); + itb.getAge(); } - + @Test public void testDeclarePrecedenceNotSupported() { TestBean target = new TestBean(); - try { - createProxy(target, - getFixture().getAdvisors(new SingletonMetadataAwareAspectInstanceFactory( - new DeclarePrecedenceShouldSucceed(),"someBean")), - ITestBean.class); - fail(); - } - catch (IllegalArgumentException ex) { - // Not supported in 2.0 - } + thrown.expect(IllegalArgumentException.class); + thrown.expectMessage("DeclarePrecendence not presently supported in Spring AOP"); + MetadataAwareAspectInstanceFactory aspectInstanceFactory = new SingletonMetadataAwareAspectInstanceFactory( + new DeclarePrecedenceShouldSucceed(), "someBean"); + createProxy(target, getFixture().getAdvisors(aspectInstanceFactory), + ITestBean.class); } /** Not supported in 2.0! diff --git a/spring-core/src/main/java/org/springframework/core/convert/converter/ConvertingComparator.java b/spring-core/src/main/java/org/springframework/core/convert/converter/ConvertingComparator.java new file mode 100644 index 00000000000..8807a7750b9 --- /dev/null +++ b/spring-core/src/main/java/org/springframework/core/convert/converter/ConvertingComparator.java @@ -0,0 +1,145 @@ +/* + * 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.core.convert.converter; + +import java.util.Comparator; +import java.util.Map; +import java.util.Map.Entry; + +import org.springframework.core.convert.ConversionService; +import org.springframework.util.Assert; +import org.springframework.util.comparator.ComparableComparator; + +/** + * A {@link Comparator} that converts values before they are compared. The specified + * {@link Converter} will be used to convert each value before it passed to the underlying + * {@code Comparator}. + * + * @author Phillip Webb + * @param the source type + * @param the target type + * @since 3.2 + */ +public class ConvertingComparator implements Comparator { + + private Comparator comparator; + + private Converter converter; + + + /** + * Create a new {@link ConvertingComparator} instance. + * + * @param comparator the underlying comparator used to compare the converted values + * @param converter the converter + */ + @SuppressWarnings("unchecked") + public ConvertingComparator(Converter converter) { + this(ComparableComparator.INSTANCE, converter); + } + + /** + * Create a new {@link ConvertingComparator} instance. + * + * @param comparator the underlying comparator used to compare the converted values + * @param converter the converter + */ + public ConvertingComparator(Comparator comparator, Converter converter) { + Assert.notNull(comparator, "Comparator must not be null"); + Assert.notNull(converter, "Converter must not be null"); + this.comparator = comparator; + this.converter = converter; + } + + /** + * Create a new {@link ComparableComparator} instance. + * + * @param comparator the underlying comparator + * @param conversionService the conversion service + * @param targetType the target type + */ + public ConvertingComparator(Comparator comparator, + ConversionService conversionService, Class targetType) { + this(comparator, new ConversionServiceConverter( + conversionService, targetType)); + } + + + public int compare(S o1, S o2) { + T c1 = this.converter.convert(o1); + T c2 = this.converter.convert(o2); + return this.comparator.compare(c1, c2); + } + + /** + * Create a new {@link ConvertingComparator} that compares {@link Map.Entry map + * entries} based on their {@link Map.Entry#getKey() keys}. + * + * @param comparator the underlying comparator used to compare keys + * @return a new {@link ConvertingComparator} instance + */ + public static ConvertingComparator, K> mapEntryKeys( + Comparator comparator) { + return new ConvertingComparator, K>(comparator, new Converter, K>() { + + public K convert(Entry source) { + return source.getKey(); + } + }); + } + + /** + * Create a new {@link ConvertingComparator} that compares {@link Map.Entry map + * entries} based on their {@link Map.Entry#getValue() values}. + * + * @param comparator the underlying comparator used to compare values + * @return a new {@link ConvertingComparator} instance + */ + public static ConvertingComparator, V> mapEntryValues( + Comparator comparator) { + return new ConvertingComparator, V>(comparator, new Converter, V>() { + + public V convert(Entry source) { + return source.getValue(); + } + }); + } + + + /** + * Adapts a {@link ConversionService} and targetType to a {@link Converter}. + */ + private static class ConversionServiceConverter implements Converter { + + private final ConversionService conversionService; + + private final Class targetType; + + public ConversionServiceConverter(ConversionService conversionService, + Class targetType) { + Assert.notNull(conversionService, "ConversionService must not be null"); + Assert.notNull(targetType, "TargetType must not be null"); + this.conversionService = conversionService; + this.targetType = targetType; + } + + public T convert(S source) { + return this.conversionService.convert(source, this.targetType); + } + } + +} diff --git a/spring-core/src/main/java/org/springframework/util/comparator/ComparableComparator.java b/spring-core/src/main/java/org/springframework/util/comparator/ComparableComparator.java index 5bc3c89aa88..269fddb4b9b 100644 --- a/spring-core/src/main/java/org/springframework/util/comparator/ComparableComparator.java +++ b/spring-core/src/main/java/org/springframework/util/comparator/ComparableComparator.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2008 the original author or authors. + * 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. @@ -29,6 +29,9 @@ import java.util.Comparator; */ public class ComparableComparator> implements Comparator { + @SuppressWarnings("rawtypes") + public static final ComparableComparator INSTANCE = new ComparableComparator(); + public int compare(T o1, T o2) { return o1.compareTo(o2); } diff --git a/spring-core/src/main/java/org/springframework/util/comparator/InstanceComparator.java b/spring-core/src/main/java/org/springframework/util/comparator/InstanceComparator.java new file mode 100644 index 00000000000..80b8b9b369f --- /dev/null +++ b/spring-core/src/main/java/org/springframework/util/comparator/InstanceComparator.java @@ -0,0 +1,72 @@ +/* + * 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.util.comparator; + +import java.util.Comparator; + +import org.springframework.util.Assert; + +/** + * Compares objects based on an arbitrary class order. Allows objects to be sorted based + * on the types of class that they inherit, for example: this comparator can be used to + * sort a list {@code Number}s such that {@code Long}s occur before {@code Integer}s. + * + *

Only the specified {@code instanceOrder} classes are considered during comparison. + * If two objects are both instances of the ordered type this comparator will return a + * {@code 0}. Consider combining with a {@link CompoundComparator} if additional sorting + * is required. + * + * @author Phillip Webb + * @param The type of objects being compared + * @see CompoundComparator + * @since 3.2 + */ +public class InstanceComparator implements Comparator { + + private Class[] instanceOrder; + + + /** + * Create a new {@link InstanceComparator} instance. + * + * @param instanceOrder the ordered list of classes that should be used when comparing + * objects. Classes earlier in the list will be be given a higher priority. + */ + public InstanceComparator(Class... instanceOrder) { + Assert.notNull(instanceOrder, "InstanceOrder must not be null"); + this.instanceOrder = instanceOrder; + } + + + public int compare(T o1, T o2) { + int i1 = getOrder(o1); + int i2 = getOrder(o2); + return (i1 < i2 ? -1 : (i1 == i2 ? 0 : 1)); + } + + private int getOrder(T object) { + if(object != null) { + for (int i = 0; i < instanceOrder.length; i++) { + if (instanceOrder[i].isInstance(object)) { + return i; + } + } + } + return instanceOrder.length; + } + +} \ No newline at end of file diff --git a/spring-core/src/test/java/org/springframework/core/convert/converter/ConvertingComparatorTests.java b/spring-core/src/test/java/org/springframework/core/convert/converter/ConvertingComparatorTests.java new file mode 100644 index 00000000000..853abca0ad5 --- /dev/null +++ b/spring-core/src/test/java/org/springframework/core/convert/converter/ConvertingComparatorTests.java @@ -0,0 +1,159 @@ +/* + * 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.core.convert.converter; + +import static org.hamcrest.Matchers.is; +import static org.junit.Assert.*; +import java.util.ArrayList; +import java.util.Collections; +import java.util.Comparator; +import java.util.LinkedHashMap; +import java.util.Map; +import java.util.Map.Entry; + +import org.junit.Rule; +import org.junit.Test; +import org.junit.rules.ExpectedException; +import org.springframework.core.convert.ConversionService; +import org.springframework.core.convert.converter.Converter; +import org.springframework.core.convert.converter.ConvertingComparator; +import org.springframework.core.convert.support.DefaultConversionService; +import org.springframework.util.comparator.ComparableComparator; + +/** + * Tests for {@link ConvertingComparator}. + * + * @author Phillip Webb + */ +public class ConvertingComparatorTests { + + @Rule + public ExpectedException thown = ExpectedException.none(); + + private final StringToInteger converter = new StringToInteger(); + + private final ConversionService conversionService = new DefaultConversionService(); + + private final TestComparator comparator = new TestComparator(); + + @Test + public void shouldThrowOnNullComparator() throws Exception { + thown.expect(IllegalArgumentException.class); + thown.expectMessage("Comparator must not be null"); + new ConvertingComparator(null, this.converter); + } + + @Test + public void shouldThrowOnNullConverter() throws Exception { + thown.expect(IllegalArgumentException.class); + thown.expectMessage("Converter must not be null"); + new ConvertingComparator(this.comparator, null); + } + + @Test + public void shouldThrowOnNullConversionService() throws Exception { + thown.expect(IllegalArgumentException.class); + thown.expectMessage("ConversionService must not be null"); + new ConvertingComparator(this.comparator, null, Integer.class); + } + + @Test + public void shouldThrowOnNullType() throws Exception { + thown.expect(IllegalArgumentException.class); + thown.expectMessage("TargetType must not be null"); + new ConvertingComparator(this.comparator, + this.conversionService, null); + } + + @Test + public void shouldUseConverterOnCompare() throws Exception { + ConvertingComparator convertingComparator = new ConvertingComparator( + this.comparator, this.converter); + testConversion(convertingComparator); + } + + @Test + public void shouldUseConversionServiceOnCompare() throws Exception { + ConvertingComparator convertingComparator = new ConvertingComparator( + comparator, conversionService, Integer.class); + testConversion(convertingComparator); + } + + @Test + public void shouldGetForConverter() throws Exception { + testConversion(new ConvertingComparator(comparator, converter)); + } + + private void testConversion(ConvertingComparator convertingComparator) { + assertThat(convertingComparator.compare("0", "0"), is(0)); + assertThat(convertingComparator.compare("0", "1"), is(-1)); + assertThat(convertingComparator.compare("1", "0"), is(1)); + comparator.assertCalled(); + } + + @Test + public void shouldGetMapEntryKeys() throws Exception { + ArrayList> list = createReverseOrderMapEntryList(); + Comparator> comparator = ConvertingComparator.mapEntryKeys(new ComparableComparator()); + Collections.sort(list, comparator); + assertThat(list.get(0).getKey(), is("a")); + } + + @Test + public void shouldGetMapEntryValues() throws Exception { + ArrayList> list = createReverseOrderMapEntryList(); + Comparator> comparator = ConvertingComparator.mapEntryValues(new ComparableComparator()); + Collections.sort(list, comparator); + assertThat(list.get(0).getValue(), is(1)); + } + + private ArrayList> createReverseOrderMapEntryList() { + Map map = new LinkedHashMap(); + map.put("b", 2); + map.put("a", 1); + ArrayList> list = new ArrayList>( + map.entrySet()); + assertThat(list.get(0).getKey(), is("b")); + return list; + } + + private static class StringToInteger implements Converter { + + public Integer convert(String source) { + return new Integer(source); + } + + } + + + private static class TestComparator extends ComparableComparator { + + private boolean called; + + public int compare(Integer o1, Integer o2) { + assertThat(o1, is(Integer.class)); + assertThat(o2, is(Integer.class)); + this.called = true; + return super.compare(o1, o2); + }; + + public void assertCalled() { + assertThat(this.called, is(true)); + } + } + +}