Skip to content

Commit 77c9321

Browse files
philwebbcbeams
authored andcommitted
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
1 parent 719a9e1 commit 77c9321

File tree

6 files changed

+457
-35
lines changed

6 files changed

+457
-35
lines changed

spring-aop/src/main/java/org/springframework/aop/aspectj/annotation/ReflectiveAspectJAdvisorFactory.java

Lines changed: 56 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright 2002-2007 the original author or authors.
2+
* Copyright 2002-2012 the original author or authors.
33
*
44
* Licensed under the Apache License, Version 2.0 (the "License");
55
* you may not use this file except in compliance with the License.
@@ -16,14 +16,20 @@
1616

1717
package org.springframework.aop.aspectj.annotation;
1818

19+
import java.lang.annotation.Annotation;
1920
import java.lang.reflect.Field;
2021
import java.lang.reflect.Method;
22+
import java.util.Collections;
23+
import java.util.Comparator;
2124
import java.util.LinkedList;
2225
import java.util.List;
2326

2427
import org.aopalliance.aop.Advice;
28+
import org.aspectj.lang.annotation.After;
2529
import org.aspectj.lang.annotation.AfterReturning;
2630
import org.aspectj.lang.annotation.AfterThrowing;
31+
import org.aspectj.lang.annotation.Around;
32+
import org.aspectj.lang.annotation.Before;
2733
import org.aspectj.lang.annotation.DeclareParents;
2834
import org.aspectj.lang.annotation.Pointcut;
2935

@@ -40,8 +46,12 @@
4046
import org.springframework.aop.framework.AopConfigException;
4147
import org.springframework.aop.support.DefaultPointcutAdvisor;
4248
import org.springframework.core.annotation.AnnotationUtils;
49+
import org.springframework.core.convert.converter.Converter;
50+
import org.springframework.core.convert.converter.ConvertingComparator;
4351
import org.springframework.util.ReflectionUtils;
4452
import org.springframework.util.StringUtils;
53+
import org.springframework.util.comparator.CompoundComparator;
54+
import org.springframework.util.comparator.InstanceComparator;
4555

4656
/**
4757
* Factory that can create Spring AOP Advisors given AspectJ classes from
@@ -52,10 +62,34 @@
5262
* @author Adrian Colyer
5363
* @author Juergen Hoeller
5464
* @author Ramnivas Laddad
65+
* @author Phillip Webb
5566
* @since 2.0
5667
*/
5768
public class ReflectiveAspectJAdvisorFactory extends AbstractAspectJAdvisorFactory {
5869

70+
private static final Comparator<Method> METHOD_COMPARATOR;
71+
72+
static {
73+
CompoundComparator<Method> comparator = new CompoundComparator<Method>();
74+
comparator.addComparator(new ConvertingComparator<Method, Annotation>(
75+
new InstanceComparator<Annotation>(
76+
Around.class, Before.class, After.class, AfterReturning.class, AfterThrowing.class),
77+
new Converter<Method, Annotation>() {
78+
public Annotation convert(Method method) {
79+
AspectJAnnotation<?> annotation = AbstractAspectJAdvisorFactory.findAspectJAnnotationOnMethod(method);
80+
return annotation == null ? null : annotation.getAnnotation();
81+
}
82+
}));
83+
comparator.addComparator(new ConvertingComparator<Method, String>(
84+
new Converter<Method, String>() {
85+
public String convert(Method method) {
86+
return method.getName();
87+
}
88+
}));
89+
METHOD_COMPARATOR = comparator;
90+
}
91+
92+
5993
public List<Advisor> getAdvisors(MetadataAwareAspectInstanceFactory maaif) {
6094
final Class<?> aspectClass = maaif.getAspectMetadata().getAspectClass();
6195
final String aspectName = maaif.getAspectMetadata().getAspectName();
@@ -67,17 +101,12 @@ public List<Advisor> getAdvisors(MetadataAwareAspectInstanceFactory maaif) {
67101
new LazySingletonAspectInstanceFactoryDecorator(maaif);
68102

69103
final List<Advisor> advisors = new LinkedList<Advisor>();
70-
ReflectionUtils.doWithMethods(aspectClass, new ReflectionUtils.MethodCallback() {
71-
public void doWith(Method method) throws IllegalArgumentException {
72-
// Exclude pointcuts
73-
if (AnnotationUtils.getAnnotation(method, Pointcut.class) == null) {
74-
Advisor advisor = getAdvisor(method, lazySingletonAspectInstanceFactory, advisors.size(), aspectName);
75-
if (advisor != null) {
76-
advisors.add(advisor);
77-
}
78-
}
104+
for (Method method : getAdvisorMethods(aspectClass)) {
105+
Advisor advisor = getAdvisor(method, lazySingletonAspectInstanceFactory, advisors.size(), aspectName);
106+
if (advisor != null) {
107+
advisors.add(advisor);
79108
}
80-
});
109+
}
81110

82111
// If it's a per target aspect, emit the dummy instantiating aspect.
83112
if (!advisors.isEmpty() && lazySingletonAspectInstanceFactory.getAspectMetadata().isLazilyInstantiated()) {
@@ -96,6 +125,20 @@ public void doWith(Method method) throws IllegalArgumentException {
96125
return advisors;
97126
}
98127

128+
private List<Method> getAdvisorMethods(Class<?> aspectClass) {
129+
final List<Method> methods = new LinkedList<Method>();
130+
ReflectionUtils.doWithMethods(aspectClass, new ReflectionUtils.MethodCallback() {
131+
public void doWith(Method method) throws IllegalArgumentException {
132+
// Exclude pointcuts
133+
if (AnnotationUtils.getAnnotation(method, Pointcut.class) == null) {
134+
methods.add(method);
135+
}
136+
}
137+
});
138+
Collections.sort(methods, METHOD_COMPARATOR);
139+
return methods;
140+
}
141+
99142
/**
100143
* Build a {@link org.springframework.aop.aspectj.DeclareParentsAdvisor}
101144
* for the given introduction field.
@@ -104,7 +147,7 @@ public void doWith(Method method) throws IllegalArgumentException {
104147
* @return <code>null</code> if not an Advisor
105148
*/
106149
private Advisor getDeclareParentsAdvisor(Field introductionField) {
107-
DeclareParents declareParents = (DeclareParents) introductionField.getAnnotation(DeclareParents.class);
150+
DeclareParents declareParents = introductionField.getAnnotation(DeclareParents.class);
108151
if (declareParents == null) {
109152
// Not an introduction field
110153
return null;
@@ -224,6 +267,7 @@ public Advice getAdvice(Method candidateAdviceMethod, AspectJExpressionPointcut
224267
* Triggered by per-clause pointcut on non-singleton aspect.
225268
* The advice has no effect.
226269
*/
270+
@SuppressWarnings("serial")
227271
protected static class SyntheticInstantiationAdvisor extends DefaultPointcutAdvisor {
228272

229273
public SyntheticInstantiationAdvisor(final MetadataAwareAspectInstanceFactory aif) {

spring-aop/src/test/java/org/springframework/aop/aspectj/annotation/AbstractAspectJAdvisorFactoryTests.java

Lines changed: 21 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,11 @@
3939
import org.aspectj.lang.annotation.DeclarePrecedence;
4040
import org.aspectj.lang.annotation.Pointcut;
4141
import org.aspectj.lang.reflect.MethodSignature;
42+
43+
import org.junit.Rule;
4244
import org.junit.Test;
45+
import org.junit.rules.ExpectedException;
46+
4347
import org.springframework.aop.Advisor;
4448
import org.springframework.aop.aspectj.annotation.ReflectiveAspectJAdvisorFactory.SyntheticInstantiationAdvisor;
4549
import org.springframework.aop.framework.Advised;
@@ -65,9 +69,13 @@
6569
*
6670
* @author Rod Johnson
6771
* @author Chris Beams
72+
* @author Phillip Webb
6873
*/
6974
public abstract class AbstractAspectJAdvisorFactoryTests {
70-
75+
76+
@Rule
77+
public ExpectedException thrown = ExpectedException.none();
78+
7179
/**
7280
* To be overridden by concrete test subclasses.
7381
* @return the fixture
@@ -571,31 +579,22 @@ public void testAfterAdviceTypes() throws Exception {
571579
@Test
572580
public void testFailureWithoutExplicitDeclarePrecedence() {
573581
TestBean target = new TestBean();
574-
ITestBean itb = (ITestBean) createProxy(target,
575-
getFixture().getAdvisors(new SingletonMetadataAwareAspectInstanceFactory(new NoDeclarePrecedenceShouldFail(), "someBean")),
576-
ITestBean.class);
577-
try {
578-
itb.getAge();
579-
fail();
580-
}
581-
catch (IllegalStateException ex) {
582-
// expected
583-
}
582+
MetadataAwareAspectInstanceFactory aspectInstanceFactory = new SingletonMetadataAwareAspectInstanceFactory(
583+
new NoDeclarePrecedenceShouldFail(), "someBean");
584+
ITestBean itb = (ITestBean) createProxy(target,
585+
getFixture().getAdvisors(aspectInstanceFactory), ITestBean.class);
586+
itb.getAge();
584587
}
585-
588+
586589
@Test
587590
public void testDeclarePrecedenceNotSupported() {
588591
TestBean target = new TestBean();
589-
try {
590-
createProxy(target,
591-
getFixture().getAdvisors(new SingletonMetadataAwareAspectInstanceFactory(
592-
new DeclarePrecedenceShouldSucceed(),"someBean")),
593-
ITestBean.class);
594-
fail();
595-
}
596-
catch (IllegalArgumentException ex) {
597-
// Not supported in 2.0
598-
}
592+
thrown.expect(IllegalArgumentException.class);
593+
thrown.expectMessage("DeclarePrecendence not presently supported in Spring AOP");
594+
MetadataAwareAspectInstanceFactory aspectInstanceFactory = new SingletonMetadataAwareAspectInstanceFactory(
595+
new DeclarePrecedenceShouldSucceed(), "someBean");
596+
createProxy(target, getFixture().getAdvisors(aspectInstanceFactory),
597+
ITestBean.class);
599598
}
600599

601600
/** Not supported in 2.0!
Lines changed: 145 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,145 @@
1+
/*
2+
* Copyright 2002-2012 the original author or authors.
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* http://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
17+
package org.springframework.core.convert.converter;
18+
19+
import java.util.Comparator;
20+
import java.util.Map;
21+
import java.util.Map.Entry;
22+
23+
import org.springframework.core.convert.ConversionService;
24+
import org.springframework.util.Assert;
25+
import org.springframework.util.comparator.ComparableComparator;
26+
27+
/**
28+
* A {@link Comparator} that converts values before they are compared. The specified
29+
* {@link Converter} will be used to convert each value before it passed to the underlying
30+
* {@code Comparator}.
31+
*
32+
* @author Phillip Webb
33+
* @param <S> the source type
34+
* @param <T> the target type
35+
* @since 3.2
36+
*/
37+
public class ConvertingComparator<S, T> implements Comparator<S> {
38+
39+
private Comparator<T> comparator;
40+
41+
private Converter<S, T> converter;
42+
43+
44+
/**
45+
* Create a new {@link ConvertingComparator} instance.
46+
*
47+
* @param comparator the underlying comparator used to compare the converted values
48+
* @param converter the converter
49+
*/
50+
@SuppressWarnings("unchecked")
51+
public ConvertingComparator(Converter<S, T> converter) {
52+
this(ComparableComparator.INSTANCE, converter);
53+
}
54+
55+
/**
56+
* Create a new {@link ConvertingComparator} instance.
57+
*
58+
* @param comparator the underlying comparator used to compare the converted values
59+
* @param converter the converter
60+
*/
61+
public ConvertingComparator(Comparator<T> comparator, Converter<S, T> converter) {
62+
Assert.notNull(comparator, "Comparator must not be null");
63+
Assert.notNull(converter, "Converter must not be null");
64+
this.comparator = comparator;
65+
this.converter = converter;
66+
}
67+
68+
/**
69+
* Create a new {@link ComparableComparator} instance.
70+
*
71+
* @param comparator the underlying comparator
72+
* @param conversionService the conversion service
73+
* @param targetType the target type
74+
*/
75+
public ConvertingComparator(Comparator<T> comparator,
76+
ConversionService conversionService, Class<? extends T> targetType) {
77+
this(comparator, new ConversionServiceConverter<S, T>(
78+
conversionService, targetType));
79+
}
80+
81+
82+
public int compare(S o1, S o2) {
83+
T c1 = this.converter.convert(o1);
84+
T c2 = this.converter.convert(o2);
85+
return this.comparator.compare(c1, c2);
86+
}
87+
88+
/**
89+
* Create a new {@link ConvertingComparator} that compares {@link Map.Entry map
90+
* entries} based on their {@link Map.Entry#getKey() keys}.
91+
*
92+
* @param comparator the underlying comparator used to compare keys
93+
* @return a new {@link ConvertingComparator} instance
94+
*/
95+
public static <K, V> ConvertingComparator<Map.Entry<K, V>, K> mapEntryKeys(
96+
Comparator<K> comparator) {
97+
return new ConvertingComparator<Map.Entry<K,V>, K>(comparator, new Converter<Map.Entry<K, V>, K>() {
98+
99+
public K convert(Entry<K, V> source) {
100+
return source.getKey();
101+
}
102+
});
103+
}
104+
105+
/**
106+
* Create a new {@link ConvertingComparator} that compares {@link Map.Entry map
107+
* entries} based on their {@link Map.Entry#getValue() values}.
108+
*
109+
* @param comparator the underlying comparator used to compare values
110+
* @return a new {@link ConvertingComparator} instance
111+
*/
112+
public static <K, V> ConvertingComparator<Map.Entry<K, V>, V> mapEntryValues(
113+
Comparator<V> comparator) {
114+
return new ConvertingComparator<Map.Entry<K,V>, V>(comparator, new Converter<Map.Entry<K, V>, V>() {
115+
116+
public V convert(Entry<K, V> source) {
117+
return source.getValue();
118+
}
119+
});
120+
}
121+
122+
123+
/**
124+
* Adapts a {@link ConversionService} and <tt>targetType</tt> to a {@link Converter}.
125+
*/
126+
private static class ConversionServiceConverter<S, T> implements Converter<S, T> {
127+
128+
private final ConversionService conversionService;
129+
130+
private final Class<? extends T> targetType;
131+
132+
public ConversionServiceConverter(ConversionService conversionService,
133+
Class<? extends T> targetType) {
134+
Assert.notNull(conversionService, "ConversionService must not be null");
135+
Assert.notNull(targetType, "TargetType must not be null");
136+
this.conversionService = conversionService;
137+
this.targetType = targetType;
138+
}
139+
140+
public T convert(S source) {
141+
return this.conversionService.convert(source, this.targetType);
142+
}
143+
}
144+
145+
}

spring-core/src/main/java/org/springframework/util/comparator/ComparableComparator.java

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright 2002-2008 the original author or authors.
2+
* Copyright 2002-2012 the original author or authors.
33
*
44
* Licensed under the Apache License, Version 2.0 (the "License");
55
* you may not use this file except in compliance with the License.
@@ -29,6 +29,9 @@
2929
*/
3030
public class ComparableComparator<T extends Comparable<T>> implements Comparator<T> {
3131

32+
@SuppressWarnings("rawtypes")
33+
public static final ComparableComparator INSTANCE = new ComparableComparator();
34+
3235
public int compare(T o1, T o2) {
3336
return o1.compareTo(o2);
3437
}

0 commit comments

Comments
 (0)