From 75edb3979eadde8df02bf4da28e2218511c991bb Mon Sep 17 00:00:00 2001 From: Juergen Hoeller Date: Thu, 11 Jun 2015 14:44:19 +0200 Subject: [PATCH] AspectJExpressionPointcut defensively catches exceptions thrown from ShadowMatch.matchesJoinPoint Issue: SPR-13102 --- .../aspectj/AspectJExpressionPointcut.java | 53 +++++---- .../BeanNamePointcutAtAspectTests.java | 13 ++- .../EnableAspectJAutoProxyTests.java | 104 ++++++++++++++---- 3 files changed, 124 insertions(+), 46 deletions(-) diff --git a/spring-aop/src/main/java/org/springframework/aop/aspectj/AspectJExpressionPointcut.java b/spring-aop/src/main/java/org/springframework/aop/aspectj/AspectJExpressionPointcut.java index 5f44293030..60427610a6 100644 --- a/spring-aop/src/main/java/org/springframework/aop/aspectj/AspectJExpressionPointcut.java +++ b/spring-aop/src/main/java/org/springframework/aop/aspectj/AspectJExpressionPointcut.java @@ -19,6 +19,7 @@ package org.springframework.aop.aspectj; import java.io.IOException; import java.io.ObjectInputStream; import java.lang.reflect.Method; +import java.util.Arrays; import java.util.HashSet; import java.util.Map; import java.util.Set; @@ -326,29 +327,41 @@ public class AspectJExpressionPointcut extends AbstractExpressionPointcut catch (IllegalStateException ex) { // No current invocation... // TODO: Should we really proceed here? - logger.debug("Couldn't access current invocation - matching with limited context: " + ex); - } - - JoinPointMatch joinPointMatch = shadowMatch.matchesJoinPoint(thisObject, targetObject, args); - - /* - * Do a final check to see if any this(TYPE) kind of residue match. For - * this purpose, we use the original method's (proxy method's) shadow to - * ensure that 'this' is correctly checked against. Without this check, - * we get incorrect match on this(TYPE) where TYPE matches the target - * type but not 'this' (as would be the case of JDK dynamic proxies). - *

See SPR-2979 for the original bug. - */ - if (pmi != null) { // there is a current invocation - RuntimeTestWalker originalMethodResidueTest = getRuntimeTestWalker(originalShadowMatch); - if (!originalMethodResidueTest.testThisInstanceOfResidue(thisObject.getClass())) { - return false; + if (logger.isDebugEnabled()) { + logger.debug("Could not access current invocation - matching with limited context: " + ex); } - if (joinPointMatch.matches()) { - bindParameters(pmi, joinPointMatch); + } + + try { + JoinPointMatch joinPointMatch = shadowMatch.matchesJoinPoint(thisObject, targetObject, args); + + /* + * Do a final check to see if any this(TYPE) kind of residue match. For + * this purpose, we use the original method's (proxy method's) shadow to + * ensure that 'this' is correctly checked against. Without this check, + * we get incorrect match on this(TYPE) where TYPE matches the target + * type but not 'this' (as would be the case of JDK dynamic proxies). + *

See SPR-2979 for the original bug. + */ + if (pmi != null) { // there is a current invocation + RuntimeTestWalker originalMethodResidueTest = getRuntimeTestWalker(originalShadowMatch); + if (!originalMethodResidueTest.testThisInstanceOfResidue(thisObject.getClass())) { + return false; + } + if (joinPointMatch.matches()) { + bindParameters(pmi, joinPointMatch); + } } + + return joinPointMatch.matches(); + } + catch (Throwable ex) { + if (logger.isDebugEnabled()) { + logger.debug("Failed to evaluate join point for arguments " + Arrays.asList(args) + + " - falling back to non-match", ex); + } + return false; } - return joinPointMatch.matches(); } protected String getCurrentProxiedBeanName() { diff --git a/spring-context/src/test/java/org/springframework/aop/aspectj/BeanNamePointcutAtAspectTests.java b/spring-context/src/test/java/org/springframework/aop/aspectj/BeanNamePointcutAtAspectTests.java index dc99b68355..1c8573390a 100644 --- a/spring-context/src/test/java/org/springframework/aop/aspectj/BeanNamePointcutAtAspectTests.java +++ b/spring-context/src/test/java/org/springframework/aop/aspectj/BeanNamePointcutAtAspectTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2013 the original author or authors. + * Copyright 2002-2015 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. @@ -35,10 +35,12 @@ import static org.junit.Assert.*; * @author Juergen Hoeller * @author Chris Beams */ -public final class BeanNamePointcutAtAspectTests { +public class BeanNamePointcutAtAspectTests { private ITestBean testBean1; + private ITestBean testBean3; + private CounterAspect counterAspect; @@ -46,7 +48,8 @@ public final class BeanNamePointcutAtAspectTests { @SuppressWarnings("resource") public void setUp() { ClassPathXmlApplicationContext ctx = - new ClassPathXmlApplicationContext(getClass().getSimpleName() + ".xml", getClass()); + new ClassPathXmlApplicationContext(getClass().getSimpleName() + ".xml", getClass()); + counterAspect = (CounterAspect) ctx.getBean("counterAspect"); testBean1 = (ITestBean) ctx.getBean("testBean1"); testBean3 = (ITestBean) ctx.getBean("testBean3"); @@ -55,15 +58,17 @@ public final class BeanNamePointcutAtAspectTests { @Test public void testMatchingBeanName() { assertTrue("Expected a proxy", testBean1 instanceof Advised); + // Call two methods to test for SPR-3953-like condition testBean1.setAge(20); testBean1.setName(""); - assertEquals(2 /*TODO: make this 3 when upgrading to AspectJ 1.6.0 and advice in CounterAspect are uncommented*/, counterAspect.count); + assertEquals(2, counterAspect.count); } @Test public void testNonMatchingBeanName() { assertFalse("Didn't expect a proxy", testBean3 instanceof Advised); + testBean3.setAge(20); assertEquals(0, counterAspect.count); } diff --git a/spring-context/src/test/java/org/springframework/context/annotation/EnableAspectJAutoProxyTests.java b/spring-context/src/test/java/org/springframework/context/annotation/EnableAspectJAutoProxyTests.java index b4b466d565..9995fc5a02 100644 --- a/spring-context/src/test/java/org/springframework/context/annotation/EnableAspectJAutoProxyTests.java +++ b/spring-context/src/test/java/org/springframework/context/annotation/EnableAspectJAutoProxyTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2011 the original author or authors. + * Copyright 2002-2015 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,12 +16,18 @@ package org.springframework.context.annotation; +import java.lang.annotation.Retention; +import java.lang.annotation.RetentionPolicy; + import example.scannable.FooService; import example.scannable.ServiceInvocationCounter; +import org.aspectj.lang.annotation.Aspect; +import org.aspectj.lang.annotation.Before; import org.junit.Test; import org.springframework.aop.support.AopUtils; import org.springframework.context.ApplicationContext; +import org.springframework.context.ConfigurableApplicationContext; import static org.hamcrest.CoreMatchers.*; import static org.junit.Assert.*; @@ -32,38 +38,23 @@ import static org.junit.Assert.*; */ public class EnableAspectJAutoProxyTests { - @Configuration - @ComponentScan("example.scannable") - @EnableAspectJAutoProxy - static class Config_WithJDKProxy { - } - - @Configuration - @ComponentScan("example.scannable") - @EnableAspectJAutoProxy(proxyTargetClass=true) - static class Config_WithCGLIBProxy { - } - @Test - public void withJDKProxy() throws Exception { - ApplicationContext ctx = - new AnnotationConfigApplicationContext(Config_WithJDKProxy.class); + public void withJdkProxy() { + ApplicationContext ctx = new AnnotationConfigApplicationContext(ConfigWithJdkProxy.class); aspectIsApplied(ctx); assertThat(AopUtils.isJdkDynamicProxy(ctx.getBean(FooService.class)), is(true)); } @Test - public void withCGLIBProxy() throws Exception { - ApplicationContext ctx = - new AnnotationConfigApplicationContext(Config_WithCGLIBProxy.class); + public void withCglibProxy() { + ApplicationContext ctx = new AnnotationConfigApplicationContext(ConfigWithCglibProxy.class); aspectIsApplied(ctx); assertThat(AopUtils.isCglibProxy(ctx.getBean(FooService.class)), is(true)); } - - private void aspectIsApplied(ApplicationContext ctx) throws Exception { + private void aspectIsApplied(ApplicationContext ctx) { FooService fooService = ctx.getBean(FooService.class); ServiceInvocationCounter counter = ctx.getBean(ServiceInvocationCounter.class); @@ -79,4 +70,73 @@ public class EnableAspectJAutoProxyTests { fooService.foo(1); assertEquals(3, counter.getCount()); } -} \ No newline at end of file + + @Test + public void withAnnotationOnArgumentAndJdkProxy() { + ConfigurableApplicationContext ctx = new AnnotationConfigApplicationContext( + ConfigWithJdkProxy.class, SampleService.class, LoggingAspect.class); + + SampleService sampleService = ctx.getBean(SampleService.class); + sampleService.execute(new SampleDto()); + sampleService.execute(new SampleInputBean()); + sampleService.execute((SampleDto) null); + sampleService.execute((SampleInputBean) null); + } + + @Test + public void withAnnotationOnArgumentAndCglibProxy() { + ConfigurableApplicationContext ctx = new AnnotationConfigApplicationContext( + ConfigWithCglibProxy.class, SampleService.class, LoggingAspect.class); + + SampleService sampleService = ctx.getBean(SampleService.class); + sampleService.execute(new SampleDto()); + sampleService.execute(new SampleInputBean()); + sampleService.execute((SampleDto) null); + sampleService.execute((SampleInputBean) null); + } + + + @Configuration + @ComponentScan("example.scannable") + @EnableAspectJAutoProxy + static class ConfigWithJdkProxy { + } + + @Configuration + @ComponentScan("example.scannable") + @EnableAspectJAutoProxy(proxyTargetClass = true) + static class ConfigWithCglibProxy { + } + + + @Retention(RetentionPolicy.RUNTIME) + public @interface Loggable { + } + + @Loggable + public static class SampleDto { + } + + public static class SampleInputBean { + } + + public static class SampleService { + + // Not matched method on {@link LoggingAspect}. + public void execute(SampleInputBean inputBean) { + } + + // Matched method on {@link LoggingAspect} + public void execute(SampleDto dto) { + } + } + + @Aspect + public static class LoggingAspect { + + @Before("@args(org.springframework.context.annotation.EnableAspectJAutoProxyTests.Loggable))") + public void loggingBeginByAtArgs() { + } + } + +}