Skip to content

Commit 75edb39

Browse files
jhoellerjhoeller
committed
AspectJExpressionPointcut defensively catches exceptions thrown from ShadowMatch.matchesJoinPoint
Issue: SPR-13102
1 parent ad55687 commit 75edb39

File tree

3 files changed

+124
-46
lines changed

3 files changed

+124
-46
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;
@@ -326,29 +327,41 @@ public boolean matches(Method method, Class<?> targetClass, Object[] args) {
326327
catch (IllegalStateException ex) {
327328
// No current invocation...
328329
// TODO: Should we really proceed here?
329-
logger.debug("Couldn't access current invocation - matching with limited context: " + ex);
330-
}
331-
332-
JoinPointMatch joinPointMatch = shadowMatch.matchesJoinPoint(thisObject, targetObject, args);
333-
334-
/*
335-
* Do a final check to see if any this(TYPE) kind of residue match. For
336-
* this purpose, we use the original method's (proxy method's) shadow to
337-
* ensure that 'this' is correctly checked against. Without this check,
338-
* we get incorrect match on this(TYPE) where TYPE matches the target
339-
* type but not 'this' (as would be the case of JDK dynamic proxies).
340-
* <p>See SPR-2979 for the original bug.
341-
*/
342-
if (pmi != null) { // there is a current invocation
343-
RuntimeTestWalker originalMethodResidueTest = getRuntimeTestWalker(originalShadowMatch);
344-
if (!originalMethodResidueTest.testThisInstanceOfResidue(thisObject.getClass())) {
345-
return false;
330+
if (logger.isDebugEnabled()) {
331+
logger.debug("Could not access current invocation - matching with limited context: " + ex);
346332
}
347-
if (joinPointMatch.matches()) {
348-
bindParameters(pmi, joinPointMatch);
333+
}
334+
335+
try {
336+
JoinPointMatch joinPointMatch = shadowMatch.matchesJoinPoint(thisObject, targetObject, args);
337+
338+
/*
339+
* Do a final check to see if any this(TYPE) kind of residue match. For
340+
* this purpose, we use the original method's (proxy method's) shadow to
341+
* ensure that 'this' is correctly checked against. Without this check,
342+
* we get incorrect match on this(TYPE) where TYPE matches the target
343+
* type but not 'this' (as would be the case of JDK dynamic proxies).
344+
* <p>See SPR-2979 for the original bug.
345+
*/
346+
if (pmi != null) { // there is a current invocation
347+
RuntimeTestWalker originalMethodResidueTest = getRuntimeTestWalker(originalShadowMatch);
348+
if (!originalMethodResidueTest.testThisInstanceOfResidue(thisObject.getClass())) {
349+
return false;
350+
}
351+
if (joinPointMatch.matches()) {
352+
bindParameters(pmi, joinPointMatch);
353+
}
349354
}
355+
356+
return joinPointMatch.matches();
357+
}
358+
catch (Throwable ex) {
359+
if (logger.isDebugEnabled()) {
360+
logger.debug("Failed to evaluate join point for arguments " + Arrays.asList(args) +
361+
" - falling back to non-match", ex);
362+
}
363+
return false;
350364
}
351-
return joinPointMatch.matches();
352365
}
353366

354367
protected String getCurrentProxiedBeanName() {

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

Lines changed: 9 additions & 4 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.
@@ -35,18 +35,21 @@
3535
* @author Juergen Hoeller
3636
* @author Chris Beams
3737
*/
38-
public final class BeanNamePointcutAtAspectTests {
38+
public class BeanNamePointcutAtAspectTests {
3939

4040
private ITestBean testBean1;
41+
4142
private ITestBean testBean3;
43+
4244
private CounterAspect counterAspect;
4345

4446

4547
@org.junit.Before
4648
@SuppressWarnings("resource")
4749
public void setUp() {
4850
ClassPathXmlApplicationContext ctx =
49-
new ClassPathXmlApplicationContext(getClass().getSimpleName() + ".xml", getClass());
51+
new ClassPathXmlApplicationContext(getClass().getSimpleName() + ".xml", getClass());
52+
5053
counterAspect = (CounterAspect) ctx.getBean("counterAspect");
5154
testBean1 = (ITestBean) ctx.getBean("testBean1");
5255
testBean3 = (ITestBean) ctx.getBean("testBean3");
@@ -55,15 +58,17 @@ public void setUp() {
5558
@Test
5659
public void testMatchingBeanName() {
5760
assertTrue("Expected a proxy", testBean1 instanceof Advised);
61+
5862
// Call two methods to test for SPR-3953-like condition
5963
testBean1.setAge(20);
6064
testBean1.setName("");
61-
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);
6266
}
6367

6468
@Test
6569
public void testNonMatchingBeanName() {
6670
assertFalse("Didn't expect a proxy", testBean3 instanceof Advised);
71+
6772
testBean3.setAge(20);
6873
assertEquals(0, counterAspect.count);
6974
}

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

Lines changed: 82 additions & 22 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,12 +16,18 @@
1616

1717
package org.springframework.context.annotation;
1818

19+
import java.lang.annotation.Retention;
20+
import java.lang.annotation.RetentionPolicy;
21+
1922
import example.scannable.FooService;
2023
import example.scannable.ServiceInvocationCounter;
24+
import org.aspectj.lang.annotation.Aspect;
25+
import org.aspectj.lang.annotation.Before;
2126
import org.junit.Test;
2227

2328
import org.springframework.aop.support.AopUtils;
2429
import org.springframework.context.ApplicationContext;
30+
import org.springframework.context.ConfigurableApplicationContext;
2531

2632
import static org.hamcrest.CoreMatchers.*;
2733
import static org.junit.Assert.*;
@@ -32,38 +38,23 @@
3238
*/
3339
public class EnableAspectJAutoProxyTests {
3440

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

5245
aspectIsApplied(ctx);
5346
assertThat(AopUtils.isJdkDynamicProxy(ctx.getBean(FooService.class)), is(true));
5447
}
5548

5649
@Test
57-
public void withCGLIBProxy() throws Exception {
58-
ApplicationContext ctx =
59-
new AnnotationConfigApplicationContext(Config_WithCGLIBProxy.class);
50+
public void withCglibProxy() {
51+
ApplicationContext ctx = new AnnotationConfigApplicationContext(ConfigWithCglibProxy.class);
6052

6153
aspectIsApplied(ctx);
6254
assertThat(AopUtils.isCglibProxy(ctx.getBean(FooService.class)), is(true));
6355
}
6456

65-
66-
private void aspectIsApplied(ApplicationContext ctx) throws Exception {
57+
private void aspectIsApplied(ApplicationContext ctx) {
6758
FooService fooService = ctx.getBean(FooService.class);
6859
ServiceInvocationCounter counter = ctx.getBean(ServiceInvocationCounter.class);
6960

@@ -79,4 +70,73 @@ private void aspectIsApplied(ApplicationContext ctx) throws Exception {
7970
fooService.foo(1);
8071
assertEquals(3, counter.getCount());
8172
}
82-
}
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)