Skip to content

Commit 896f0d9

Browse files
jhoellerjhoeller
committed
AspectJExpressionPointcut defensively catches exceptions thrown from ShadowMatch.matchesJoinPoint
Issue: SPR-13102 (cherry picked from commit 84ae28b)
1 parent 50eef1b commit 896f0d9

File tree

3 files changed

+131
-58
lines changed

3 files changed

+131
-58
lines changed

‎spring-aop/src/main/java/org/springframework/aop/aspectj/AspectJExpressionPointcut.java‎

Lines changed: 33 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919
import java.io.IOException;
2020
import java.io.ObjectInputStream;
2121
import java.lang.reflect.Method;
22+
import java.util.Arrays;
2223
import java.util.HashSet;
2324
import java.util.Map;
2425
import java.util.Set;
@@ -318,29 +319,41 @@ public boolean matches(Method method, Class<?> targetClass, Object[] args) {
318319
catch (IllegalStateException ex) {
319320
// No current invocation...
320321
// TODO: Should we really proceed here?
321-
logger.debug("Couldn't access current invocation - matching with limited context: " + ex);
322-
}
323-
324-
JoinPointMatch joinPointMatch = shadowMatch.matchesJoinPoint(thisObject, targetObject, args);
325-
326-
/*
327-
* Do a final check to see if any this(TYPE) kind of residue match. For
328-
* this purpose, we use the original method's (proxy method's) shadow to
329-
* ensure that 'this' is correctly checked against. Without this check,
330-
* we get incorrect match on this(TYPE) where TYPE matches the target
331-
* type but not 'this' (as would be the case of JDK dynamic proxies).
332-
* <p>See SPR-2979 for the original bug.
333-
*/
334-
if (pmi != null) { // there is a current invocation
335-
RuntimeTestWalker originalMethodResidueTest = getRuntimeTestWalker(originalShadowMatch);
336-
if (!originalMethodResidueTest.testThisInstanceOfResidue(thisObject.getClass())) {
337-
return false;
322+
if (logger.isDebugEnabled()) {
323+
logger.debug("Could not access current invocation - matching with limited context: " + ex);
338324
}
339-
if (joinPointMatch.matches()) {
340-
bindParameters(pmi, joinPointMatch);
325+
}
326+
327+
try {
328+
JoinPointMatch joinPointMatch = shadowMatch.matchesJoinPoint(thisObject, targetObject, args);
329+
330+
/*
331+
* Do a final check to see if any this(TYPE) kind of residue match. For
332+
* this purpose, we use the original method's (proxy method's) shadow to
333+
* ensure that 'this' is correctly checked against. Without this check,
334+
* we get incorrect match on this(TYPE) where TYPE matches the target
335+
* type but not 'this' (as would be the case of JDK dynamic proxies).
336+
* <p>See SPR-2979 for the original bug.
337+
*/
338+
if (pmi != null) { // there is a current invocation
339+
RuntimeTestWalker originalMethodResidueTest = getRuntimeTestWalker(originalShadowMatch);
340+
if (!originalMethodResidueTest.testThisInstanceOfResidue(thisObject.getClass())) {
341+
return false;
342+
}
343+
if (joinPointMatch.matches()) {
344+
bindParameters(pmi, joinPointMatch);
345+
}
341346
}
347+
348+
return joinPointMatch.matches();
349+
}
350+
catch (Throwable ex) {
351+
if (logger.isDebugEnabled()) {
352+
logger.debug("Failed to evaluate join point for arguments " + Arrays.asList(args) +
353+
" - falling back to non-match", ex);
354+
}
355+
return false;
342356
}
343-
return joinPointMatch.matches();
344357
}
345358

346359
protected String getCurrentProxiedBeanName() {

‎spring-context/src/test/java/org/springframework/aop/aspectj/BeanNamePointcutAtAspectTests.java‎

Lines changed: 12 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright 2002-2013 the original author or authors.
2+
* Copyright 2002-2015 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,16 +16,17 @@
1616

1717
package org.springframework.aop.aspectj;
1818

19-
import static org.junit.Assert.*;
20-
2119
import org.aspectj.lang.annotation.Aspect;
2220
import org.aspectj.lang.annotation.Before;
2321
import org.junit.Test;
22+
2423
import org.springframework.aop.aspectj.annotation.AspectJProxyFactory;
2524
import org.springframework.aop.framework.Advised;
25+
import org.springframework.context.support.ClassPathXmlApplicationContext;
2626
import org.springframework.tests.sample.beans.ITestBean;
2727
import org.springframework.tests.sample.beans.TestBean;
28-
import org.springframework.context.support.ClassPathXmlApplicationContext;
28+
29+
import static org.junit.Assert.*;
2930

3031
/**
3132
* Test for correct application of the bean() PCD for &#64;AspectJ-based aspects.
@@ -34,39 +35,40 @@
3435
* @author Juergen Hoeller
3536
* @author Chris Beams
3637
*/
37-
public final class BeanNamePointcutAtAspectTests {
38+
public class BeanNamePointcutAtAspectTests {
3839

3940
private ITestBean testBean1;
4041

41-
private ITestBean testBean2;
42-
4342
private ITestBean testBean3;
4443

4544
private CounterAspect counterAspect;
4645

4746

4847
@org.junit.Before
48+
@SuppressWarnings("resource")
4949
public void setUp() {
5050
ClassPathXmlApplicationContext ctx =
51-
new ClassPathXmlApplicationContext(getClass().getSimpleName() + ".xml", getClass());
51+
new ClassPathXmlApplicationContext(getClass().getSimpleName() + ".xml", getClass());
52+
5253
counterAspect = (CounterAspect) ctx.getBean("counterAspect");
5354
testBean1 = (ITestBean) ctx.getBean("testBean1");
54-
testBean2 = (ITestBean) ctx.getBean("testBean2");
5555
testBean3 = (ITestBean) ctx.getBean("testBean3");
5656
}
5757

5858
@Test
5959
public void testMatchingBeanName() {
6060
assertTrue("Expected a proxy", testBean1 instanceof Advised);
61+
6162
// Call two methods to test for SPR-3953-like condition
6263
testBean1.setAge(20);
6364
testBean1.setName("");
64-
assertEquals(2 /*TODO: make this 3 when upgrading to AspectJ 1.6.0 and advice in CounterAspect are uncommented*/, counterAspect.count);
65+
assertEquals(2, counterAspect.count);
6566
}
6667

6768
@Test
6869
public void testNonMatchingBeanName() {
6970
assertFalse("Didn't expect a proxy", testBean3 instanceof Advised);
71+
7072
testBean3.setAge(20);
7173
assertEquals(0, counterAspect.count);
7274
}

‎spring-context/src/test/java/org/springframework/context/annotation/EnableAspectJAutoProxyTests.java‎

Lines changed: 86 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright 2002-2011 the original author or authors.
2+
* Copyright 2002-2015 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,56 +16,45 @@
1616

1717
package org.springframework.context.annotation;
1818

19-
import static org.hamcrest.CoreMatchers.is;
20-
import static org.junit.Assert.assertEquals;
21-
import static org.junit.Assert.assertThat;
22-
import static org.junit.Assert.assertTrue;
19+
import java.lang.annotation.Retention;
20+
import java.lang.annotation.RetentionPolicy;
2321

22+
import example.scannable.FooService;
23+
import example.scannable.ServiceInvocationCounter;
24+
import org.aspectj.lang.annotation.Aspect;
25+
import org.aspectj.lang.annotation.Before;
2426
import org.junit.Test;
27+
2528
import org.springframework.aop.support.AopUtils;
2629
import org.springframework.context.ApplicationContext;
30+
import org.springframework.context.ConfigurableApplicationContext;
2731

28-
import example.scannable.FooService;
29-
import example.scannable.ServiceInvocationCounter;
32+
import static org.hamcrest.CoreMatchers.*;
33+
import static org.junit.Assert.*;
3034

3135
/**
3236
* @author Juergen Hoeller
3337
* @author Chris Beams
3438
*/
3539
public class EnableAspectJAutoProxyTests {
3640

37-
@Configuration
38-
@ComponentScan("example.scannable")
39-
@EnableAspectJAutoProxy
40-
static class Config_WithJDKProxy {
41-
}
42-
43-
@Configuration
44-
@ComponentScan("example.scannable")
45-
@EnableAspectJAutoProxy(proxyTargetClass=true)
46-
static class Config_WithCGLIBProxy {
47-
}
48-
4941
@Test
50-
public void withJDKProxy() throws Exception {
51-
ApplicationContext ctx =
52-
new AnnotationConfigApplicationContext(Config_WithJDKProxy.class);
42+
public void withJdkProxy() {
43+
ApplicationContext ctx = new AnnotationConfigApplicationContext(ConfigWithJdkProxy.class);
5344

5445
aspectIsApplied(ctx);
5546
assertThat(AopUtils.isJdkDynamicProxy(ctx.getBean(FooService.class)), is(true));
5647
}
5748

5849
@Test
59-
public void withCGLIBProxy() throws Exception {
60-
ApplicationContext ctx =
61-
new AnnotationConfigApplicationContext(Config_WithCGLIBProxy.class);
50+
public void withCglibProxy() {
51+
ApplicationContext ctx = new AnnotationConfigApplicationContext(ConfigWithCglibProxy.class);
6252

6353
aspectIsApplied(ctx);
6454
assertThat(AopUtils.isCglibProxy(ctx.getBean(FooService.class)), is(true));
6555
}
6656

67-
68-
private void aspectIsApplied(ApplicationContext ctx) throws Exception {
57+
private void aspectIsApplied(ApplicationContext ctx) {
6958
FooService fooService = ctx.getBean(FooService.class);
7059
ServiceInvocationCounter counter = ctx.getBean(ServiceInvocationCounter.class);
7160

@@ -81,4 +70,73 @@ private void aspectIsApplied(ApplicationContext ctx) throws Exception {
8170
fooService.foo(1);
8271
assertEquals(3, counter.getCount());
8372
}
84-
}
73+
74+
@Test
75+
public void withAnnotationOnArgumentAndJdkProxy() {
76+
ConfigurableApplicationContext ctx = new AnnotationConfigApplicationContext(
77+
ConfigWithJdkProxy.class, SampleService.class, LoggingAspect.class);
78+
79+
SampleService sampleService = ctx.getBean(SampleService.class);
80+
sampleService.execute(new SampleDto());
81+
sampleService.execute(new SampleInputBean());
82+
sampleService.execute((SampleDto) null);
83+
sampleService.execute((SampleInputBean) null);
84+
}
85+
86+
@Test
87+
public void withAnnotationOnArgumentAndCglibProxy() {
88+
ConfigurableApplicationContext ctx = new AnnotationConfigApplicationContext(
89+
ConfigWithCglibProxy.class, SampleService.class, LoggingAspect.class);
90+
91+
SampleService sampleService = ctx.getBean(SampleService.class);
92+
sampleService.execute(new SampleDto());
93+
sampleService.execute(new SampleInputBean());
94+
sampleService.execute((SampleDto) null);
95+
sampleService.execute((SampleInputBean) null);
96+
}
97+
98+
99+
@Configuration
100+
@ComponentScan("example.scannable")
101+
@EnableAspectJAutoProxy
102+
static class ConfigWithJdkProxy {
103+
}
104+
105+
@Configuration
106+
@ComponentScan("example.scannable")
107+
@EnableAspectJAutoProxy(proxyTargetClass = true)
108+
static class ConfigWithCglibProxy {
109+
}
110+
111+
112+
@Retention(RetentionPolicy.RUNTIME)
113+
public @interface Loggable {
114+
}
115+
116+
@Loggable
117+
public static class SampleDto {
118+
}
119+
120+
public static class SampleInputBean {
121+
}
122+
123+
public static class SampleService {
124+
125+
// Not matched method on {@link LoggingAspect}.
126+
public void execute(SampleInputBean inputBean) {
127+
}
128+
129+
// Matched method on {@link LoggingAspect}
130+
public void execute(SampleDto dto) {
131+
}
132+
}
133+
134+
@Aspect
135+
public static class LoggingAspect {
136+
137+
@Before("@args(org.springframework.context.annotation.EnableAspectJAutoProxyTests.Loggable))")
138+
public void loggingBeginByAtArgs() {
139+
}
140+
}
141+
142+
}

0 commit comments

Comments
 (0)