From eb0aae066c83ae0b7be280bd5c9e0679ed394a92 Mon Sep 17 00:00:00 2001 From: Arjen Poutsma Date: Thu, 4 Jun 2020 17:32:06 +0200 Subject: [PATCH 1/4] Respect MediaType charset in Jackson converters Before this commit, AbstractJackson2HttpMessageConverter and subclasses did not check media type encoding in the canRead and canWrite methods. As a result, the converter reported that it can write (for instance) "application/json;charset=ISO-8859-1", but in practice wrote the default charset (UTF-8). This commit fixes that bug. See: gh-25076 --- .../AbstractJackson2HttpMessageConverter.java | 42 +++++++++++++++++-- ...pingJackson2HttpMessageConverterTests.java | 4 ++ ...ackson2SmileHttpMessageConverterTests.java | 5 +++ ...gJackson2XmlHttpMessageConverterTests.java | 4 ++ 4 files changed, 51 insertions(+), 4 deletions(-) diff --git a/spring-web/src/main/java/org/springframework/http/converter/json/AbstractJackson2HttpMessageConverter.java b/spring-web/src/main/java/org/springframework/http/converter/json/AbstractJackson2HttpMessageConverter.java index 3c49fd268e..9ad1a32010 100644 --- a/spring-web/src/main/java/org/springframework/http/converter/json/AbstractJackson2HttpMessageConverter.java +++ b/spring-web/src/main/java/org/springframework/http/converter/json/AbstractJackson2HttpMessageConverter.java @@ -21,7 +21,11 @@ import java.lang.reflect.Type; import java.nio.charset.Charset; import java.util.Arrays; import java.util.Collections; +import java.util.EnumSet; +import java.util.Map; import java.util.concurrent.atomic.AtomicReference; +import java.util.function.Function; +import java.util.stream.Collectors; import com.fasterxml.jackson.core.JsonEncoding; import com.fasterxml.jackson.core.JsonGenerator; @@ -68,6 +72,8 @@ import org.springframework.util.TypeUtils; */ public abstract class AbstractJackson2HttpMessageConverter extends AbstractGenericHttpMessageConverter { + private static final Map ENCODINGS = jsonEncodings(); + /** * The default charset used by the converter. */ @@ -167,6 +173,14 @@ public abstract class AbstractJackson2HttpMessageConverter extends AbstractGener return false; } + @Override + protected boolean canRead(@Nullable MediaType mediaType) { + if (!super.canRead(mediaType)) { + return false; + } + return checkEncoding(mediaType); + } + @Override public boolean canWrite(Class clazz, @Nullable MediaType mediaType) { if (!canWrite(mediaType)) { @@ -180,6 +194,14 @@ public abstract class AbstractJackson2HttpMessageConverter extends AbstractGener return false; } + @Override + protected boolean canWrite(@Nullable MediaType mediaType) { + if (!super.canWrite(mediaType)) { + return false; + } + return checkEncoding(mediaType); + } + /** * Determine whether to log the given exception coming from a * {@link ObjectMapper#canDeserialize} / {@link ObjectMapper#canSerialize} check. @@ -211,6 +233,14 @@ public abstract class AbstractJackson2HttpMessageConverter extends AbstractGener } } + private boolean checkEncoding(@Nullable MediaType mediaType) { + if (mediaType != null && mediaType.getCharset() != null) { + Charset charset = mediaType.getCharset(); + return ENCODINGS.containsKey(charset.name()); + } + return true; + } + @Override protected Object readInternal(Class clazz, HttpInputMessage inputMessage) throws IOException, HttpMessageNotReadableException { @@ -333,10 +363,9 @@ public abstract class AbstractJackson2HttpMessageConverter extends AbstractGener protected JsonEncoding getJsonEncoding(@Nullable MediaType contentType) { if (contentType != null && contentType.getCharset() != null) { Charset charset = contentType.getCharset(); - for (JsonEncoding encoding : JsonEncoding.values()) { - if (charset.name().equals(encoding.getJavaName())) { - return encoding; - } + JsonEncoding encoding = ENCODINGS.get(charset.name()); + if (encoding != null) { + return encoding; } } return JsonEncoding.UTF8; @@ -359,4 +388,9 @@ public abstract class AbstractJackson2HttpMessageConverter extends AbstractGener return super.getContentLength(object, contentType); } + private static Map jsonEncodings() { + return EnumSet.allOf(JsonEncoding.class).stream() + .collect(Collectors.toMap(JsonEncoding::getJavaName, Function.identity())); + } + } diff --git a/spring-web/src/test/java/org/springframework/http/converter/json/MappingJackson2HttpMessageConverterTests.java b/spring-web/src/test/java/org/springframework/http/converter/json/MappingJackson2HttpMessageConverterTests.java index a630f6cd78..c3f25bff6d 100644 --- a/spring-web/src/test/java/org/springframework/http/converter/json/MappingJackson2HttpMessageConverterTests.java +++ b/spring-web/src/test/java/org/springframework/http/converter/json/MappingJackson2HttpMessageConverterTests.java @@ -64,12 +64,16 @@ public class MappingJackson2HttpMessageConverterTests { public void canRead() { assertThat(converter.canRead(MyBean.class, new MediaType("application", "json"))).isTrue(); assertThat(converter.canRead(Map.class, new MediaType("application", "json"))).isTrue(); + assertThat(converter.canRead(MyBean.class, new MediaType("application", "json", StandardCharsets.UTF_8))).isTrue(); + assertThat(converter.canRead(MyBean.class, new MediaType("application", "json", StandardCharsets.ISO_8859_1))).isFalse(); } @Test public void canWrite() { assertThat(converter.canWrite(MyBean.class, new MediaType("application", "json"))).isTrue(); assertThat(converter.canWrite(Map.class, new MediaType("application", "json"))).isTrue(); + assertThat(converter.canWrite(MyBean.class, new MediaType("application", "json", StandardCharsets.UTF_8))).isTrue(); + assertThat(converter.canWrite(MyBean.class, new MediaType("application", "json", StandardCharsets.ISO_8859_1))).isFalse(); } @Test // SPR-7905 diff --git a/spring-web/src/test/java/org/springframework/http/converter/smile/MappingJackson2SmileHttpMessageConverterTests.java b/spring-web/src/test/java/org/springframework/http/converter/smile/MappingJackson2SmileHttpMessageConverterTests.java index 2cbb22bdef..3d6c8912f5 100644 --- a/spring-web/src/test/java/org/springframework/http/converter/smile/MappingJackson2SmileHttpMessageConverterTests.java +++ b/spring-web/src/test/java/org/springframework/http/converter/smile/MappingJackson2SmileHttpMessageConverterTests.java @@ -17,6 +17,7 @@ package org.springframework.http.converter.smile; import java.io.IOException; +import java.nio.charset.StandardCharsets; import com.fasterxml.jackson.databind.ObjectMapper; import com.fasterxml.jackson.dataformat.smile.SmileFactory; @@ -45,6 +46,8 @@ public class MappingJackson2SmileHttpMessageConverterTests { assertThat(converter.canRead(MyBean.class, new MediaType("application", "x-jackson-smile"))).isTrue(); assertThat(converter.canRead(MyBean.class, new MediaType("application", "json"))).isFalse(); assertThat(converter.canRead(MyBean.class, new MediaType("application", "xml"))).isFalse(); + assertThat(converter.canRead(MyBean.class, new MediaType("application", "x-jackson-smile", StandardCharsets.UTF_8))).isTrue(); + assertThat(converter.canRead(MyBean.class, new MediaType("application", "x-jackson-smile", StandardCharsets.ISO_8859_1))).isFalse(); } @Test @@ -52,6 +55,8 @@ public class MappingJackson2SmileHttpMessageConverterTests { assertThat(converter.canWrite(MyBean.class, new MediaType("application", "x-jackson-smile"))).isTrue(); assertThat(converter.canWrite(MyBean.class, new MediaType("application", "json"))).isFalse(); assertThat(converter.canWrite(MyBean.class, new MediaType("application", "xml"))).isFalse(); + assertThat(converter.canWrite(MyBean.class, new MediaType("application", "x-jackson-smile", StandardCharsets.UTF_8))).isTrue(); + assertThat(converter.canWrite(MyBean.class, new MediaType("application", "x-jackson-smile", StandardCharsets.ISO_8859_1))).isFalse(); } @Test diff --git a/spring-web/src/test/java/org/springframework/http/converter/xml/MappingJackson2XmlHttpMessageConverterTests.java b/spring-web/src/test/java/org/springframework/http/converter/xml/MappingJackson2XmlHttpMessageConverterTests.java index 1541295157..f39747b26f 100644 --- a/spring-web/src/test/java/org/springframework/http/converter/xml/MappingJackson2XmlHttpMessageConverterTests.java +++ b/spring-web/src/test/java/org/springframework/http/converter/xml/MappingJackson2XmlHttpMessageConverterTests.java @@ -50,6 +50,8 @@ public class MappingJackson2XmlHttpMessageConverterTests { assertThat(converter.canRead(MyBean.class, new MediaType("application", "xml"))).isTrue(); assertThat(converter.canRead(MyBean.class, new MediaType("text", "xml"))).isTrue(); assertThat(converter.canRead(MyBean.class, new MediaType("application", "soap+xml"))).isTrue(); + assertThat(converter.canRead(MyBean.class, new MediaType("text", "xml", StandardCharsets.UTF_8))).isTrue(); + assertThat(converter.canRead(MyBean.class, new MediaType("text", "xml", StandardCharsets.ISO_8859_1))).isFalse(); } @Test @@ -57,6 +59,8 @@ public class MappingJackson2XmlHttpMessageConverterTests { assertThat(converter.canWrite(MyBean.class, new MediaType("application", "xml"))).isTrue(); assertThat(converter.canWrite(MyBean.class, new MediaType("text", "xml"))).isTrue(); assertThat(converter.canWrite(MyBean.class, new MediaType("application", "soap+xml"))).isTrue(); + assertThat(converter.canWrite(MyBean.class, new MediaType("text", "xml", StandardCharsets.UTF_8))).isTrue(); + assertThat(converter.canWrite(MyBean.class, new MediaType("text", "xml", StandardCharsets.ISO_8859_1))).isFalse(); } @Test From 847629412fe7022dfefb62d837383f2e7dc80521 Mon Sep 17 00:00:00 2001 From: Arjen Poutsma Date: Fri, 5 Jun 2020 10:52:09 +0200 Subject: [PATCH 2/4] Respect MimeType charset in Jackson codecs Before this commit, Jackson2CodecSupport and subclasses did not check media type encoding in the supportsMimeType method (called from canEncode/canDecode). As a result, the encoder reported that it can write (for instance) "application/json;charset=ISO-8859-1", but in practice wrote the default charset (UTF-8). This commit fixes that bug. Closes: gh-25076 --- .../http/codec/json/Jackson2CodecSupport.java | 26 ++++++++++++++++++- .../codec/cbor/Jackson2CborDecoderTests.java | 16 ++++++++---- .../codec/cbor/Jackson2CborEncoderTests.java | 8 ++++++ .../codec/json/Jackson2JsonDecoderTests.java | 4 +++ .../codec/json/Jackson2JsonEncoderTests.java | 7 +++++ .../codec/json/Jackson2SmileDecoderTests.java | 19 +++++++++----- .../codec/json/Jackson2SmileEncoderTests.java | 7 +++++ 7 files changed, 75 insertions(+), 12 deletions(-) diff --git a/spring-web/src/main/java/org/springframework/http/codec/json/Jackson2CodecSupport.java b/spring-web/src/main/java/org/springframework/http/codec/json/Jackson2CodecSupport.java index 6162be280c..63eabcd750 100644 --- a/spring-web/src/main/java/org/springframework/http/codec/json/Jackson2CodecSupport.java +++ b/spring-web/src/main/java/org/springframework/http/codec/json/Jackson2CodecSupport.java @@ -18,13 +18,18 @@ package org.springframework.http.codec.json; import java.lang.annotation.Annotation; import java.lang.reflect.Type; +import java.nio.charset.Charset; import java.util.Arrays; import java.util.Collections; +import java.util.EnumSet; import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.function.Function; +import java.util.stream.Collectors; import com.fasterxml.jackson.annotation.JsonView; +import com.fasterxml.jackson.core.JsonEncoding; import com.fasterxml.jackson.databind.JavaType; import com.fasterxml.jackson.databind.ObjectMapper; import com.fasterxml.jackson.databind.type.TypeFactory; @@ -75,6 +80,9 @@ public abstract class Jackson2CodecSupport { new MimeType("application", "json"), new MimeType("application", "*+json"))); + private static final Map ENCODINGS = jsonEncodings(); + + protected final Log logger = HttpLogging.forLogName(getClass()); @@ -107,7 +115,17 @@ public abstract class Jackson2CodecSupport { protected boolean supportsMimeType(@Nullable MimeType mimeType) { - return (mimeType == null || this.mimeTypes.stream().anyMatch(m -> m.isCompatibleWith(mimeType))); + if (mimeType == null) { + return true; + } + else if (this.mimeTypes.stream().noneMatch(m -> m.isCompatibleWith(mimeType))) { + return false; + } + else if (mimeType.getCharset() != null) { + Charset charset = mimeType.getCharset(); + return ENCODINGS.containsKey(charset.name()); + } + return true; } protected JavaType getJavaType(Type type, @Nullable Class contextClass) { @@ -145,4 +163,10 @@ public abstract class Jackson2CodecSupport { @Nullable protected abstract A getAnnotation(MethodParameter parameter, Class annotType); + private static Map jsonEncodings() { + return EnumSet.allOf(JsonEncoding.class).stream() + .collect(Collectors.toMap(JsonEncoding::getJavaName, Function.identity())); + } + + } diff --git a/spring-web/src/test/java/org/springframework/http/codec/cbor/Jackson2CborDecoderTests.java b/spring-web/src/test/java/org/springframework/http/codec/cbor/Jackson2CborDecoderTests.java index 63eaf2488c..041ad618b5 100644 --- a/spring-web/src/test/java/org/springframework/http/codec/cbor/Jackson2CborDecoderTests.java +++ b/spring-web/src/test/java/org/springframework/http/codec/cbor/Jackson2CborDecoderTests.java @@ -16,6 +16,7 @@ package org.springframework.http.codec.cbor; +import java.nio.charset.StandardCharsets; import java.util.Arrays; import java.util.List; @@ -27,13 +28,13 @@ import reactor.core.publisher.Flux; import org.springframework.core.ResolvableType; import org.springframework.core.io.buffer.DataBuffer; import org.springframework.core.testfixture.codec.AbstractDecoderTests; +import org.springframework.http.MediaType; import org.springframework.http.converter.json.Jackson2ObjectMapperBuilder; import org.springframework.util.MimeType; import org.springframework.web.testfixture.xml.Pojo; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatExceptionOfType; -import static org.springframework.core.ResolvableType.forClass; import static org.springframework.http.MediaType.APPLICATION_JSON; /** @@ -58,11 +59,16 @@ public class Jackson2CborDecoderTests extends AbstractDecoderTests Date: Fri, 5 Jun 2020 14:29:06 +0200 Subject: [PATCH 3/4] Polishing --- .../framework/adapter/MethodBeforeAdviceInterceptor.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/spring-aop/src/main/java/org/springframework/aop/framework/adapter/MethodBeforeAdviceInterceptor.java b/spring-aop/src/main/java/org/springframework/aop/framework/adapter/MethodBeforeAdviceInterceptor.java index 420c6203fb..342de103da 100644 --- a/spring-aop/src/main/java/org/springframework/aop/framework/adapter/MethodBeforeAdviceInterceptor.java +++ b/spring-aop/src/main/java/org/springframework/aop/framework/adapter/MethodBeforeAdviceInterceptor.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2018 the original author or authors. + * Copyright 2002-2020 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. @@ -26,9 +26,9 @@ import org.springframework.aop.MethodBeforeAdvice; import org.springframework.util.Assert; /** - * Interceptor to wrap am {@link org.springframework.aop.MethodBeforeAdvice}. - * Used internally by the AOP framework; application developers should not need - * to use this class directly. + * Interceptor to wrap a {@link MethodBeforeAdvice}. + *

Used internally by the AOP framework; application developers should not + * need to use this class directly. * * @author Rod Johnson * @see AfterReturningAdviceInterceptor From fbeecf3bf8f7facd14d13a0ddae9ca00d5c192ad Mon Sep 17 00:00:00 2001 From: Sam Brannen Date: Fri, 5 Jun 2020 15:22:27 +0200 Subject: [PATCH 4/4] Test status quo for invocation order of all advice types Prior to this commit we did not have tests in place to verify the status quo for the invocation order of all advice types when declared within a single aspect, either via the XML namespace or AspectJ auto-proxy support. This commit introduces such tests that demonstrate where such ordering is broken or suboptimal. The only test for which the advice invocation order is correct or at least as expected is the afterAdviceTypes() test method in ReflectiveAspectJAdvisorFactoryTests, where an AOP proxy is hand crafted using ReflectiveAspectJAdvisorFactory without the use of Spring's AspectJPrecedenceComparator. See gh-25186 --- ...aceHandlerAdviceOrderIntegrationTests.java | 32 ++++++--- ...JAutoProxyAdviceOrderIntegrationTests.java | 69 ++++++++++++++++--- ...AdviceOrderIntegrationTests-afterFirst.xml | 10 +-- ...rAdviceOrderIntegrationTests-afterLast.xml | 10 +-- .../AbstractAspectJAdvisorFactoryTests.java | 36 +++++++--- .../AspectJPointcutAdvisorTests.java | 5 +- .../annotation/AspectMetadataTests.java | 6 +- 7 files changed, 124 insertions(+), 44 deletions(-) diff --git a/integration-tests/src/test/java/org/springframework/aop/config/AopNamespaceHandlerAdviceOrderIntegrationTests.java b/integration-tests/src/test/java/org/springframework/aop/config/AopNamespaceHandlerAdviceOrderIntegrationTests.java index d4a659f824..da32ff1201 100644 --- a/integration-tests/src/test/java/org/springframework/aop/config/AopNamespaceHandlerAdviceOrderIntegrationTests.java +++ b/integration-tests/src/test/java/org/springframework/aop/config/AopNamespaceHandlerAdviceOrderIntegrationTests.java @@ -19,6 +19,7 @@ package org.springframework.aop.config; import java.util.ArrayList; import java.util.List; +import org.aspectj.lang.ProceedingJoinPoint; import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; @@ -45,14 +46,14 @@ class AopNamespaceHandlerAdviceOrderIntegrationTests { class AfterAdviceFirstTests { @Test - void afterAdviceIsInvokedFirst(@Autowired Echo echo, @Autowired EchoAspect aspect) throws Exception { + void afterAdviceIsInvokedFirst(@Autowired Echo echo, @Autowired InvocationTrackingAspect aspect) throws Exception { assertThat(aspect.invocations).isEmpty(); assertThat(echo.echo(42)).isEqualTo(42); - assertThat(aspect.invocations).containsExactly("after", "after returning"); + assertThat(aspect.invocations).containsExactly("around - start", "before", "around - end", "after", "after returning"); aspect.invocations.clear(); assertThatExceptionOfType(Exception.class).isThrownBy(() -> echo.echo(new Exception())); - assertThat(aspect.invocations).containsExactly("after", "after throwing"); + assertThat(aspect.invocations).containsExactly("around - start", "before", "around - end", "after", "after throwing"); } } @@ -62,14 +63,14 @@ class AopNamespaceHandlerAdviceOrderIntegrationTests { class AfterAdviceLastTests { @Test - void afterAdviceIsInvokedLast(@Autowired Echo echo, @Autowired EchoAspect aspect) throws Exception { + void afterAdviceIsInvokedLast(@Autowired Echo echo, @Autowired InvocationTrackingAspect aspect) throws Exception { assertThat(aspect.invocations).isEmpty(); assertThat(echo.echo(42)).isEqualTo(42); - assertThat(aspect.invocations).containsExactly("after returning", "after"); + assertThat(aspect.invocations).containsExactly("around - start", "before", "around - end", "after returning", "after"); aspect.invocations.clear(); assertThatExceptionOfType(Exception.class).isThrownBy(() -> echo.echo(new Exception())); - assertThat(aspect.invocations).containsExactly("after throwing", "after"); + assertThat(aspect.invocations).containsExactly("around - start", "before", "around - end", "after throwing", "after"); } } @@ -84,18 +85,29 @@ class AopNamespaceHandlerAdviceOrderIntegrationTests { } } - static class EchoAspect { + static class InvocationTrackingAspect { List invocations = new ArrayList<>(); - void echo() { + Object around(ProceedingJoinPoint joinPoint) throws Throwable { + invocations.add("around - start"); + try { + return joinPoint.proceed(); + } + finally { + invocations.add("around - end"); + } } - void succeeded() { + void before() { + invocations.add("before"); + } + + void afterReturning() { invocations.add("after returning"); } - void failed() { + void afterThrowing() { invocations.add("after throwing"); } diff --git a/integration-tests/src/test/java/org/springframework/aop/framework/autoproxy/AspectJAutoProxyAdviceOrderIntegrationTests.java b/integration-tests/src/test/java/org/springframework/aop/framework/autoproxy/AspectJAutoProxyAdviceOrderIntegrationTests.java index f088cc1024..6e67a881fa 100644 --- a/integration-tests/src/test/java/org/springframework/aop/framework/autoproxy/AspectJAutoProxyAdviceOrderIntegrationTests.java +++ b/integration-tests/src/test/java/org/springframework/aop/framework/autoproxy/AspectJAutoProxyAdviceOrderIntegrationTests.java @@ -19,10 +19,13 @@ package org.springframework.aop.framework.autoproxy; import java.util.ArrayList; import java.util.List; +import org.aspectj.lang.ProceedingJoinPoint; 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.Aspect; +import org.aspectj.lang.annotation.Before; import org.aspectj.lang.annotation.Pointcut; import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; @@ -47,25 +50,39 @@ import static org.assertj.core.api.Assertions.assertThatExceptionOfType; */ class AspectJAutoProxyAdviceOrderIntegrationTests { + /** + * {@link After @After} advice declared as first after method in source code. + */ @Nested @SpringJUnitConfig(AfterAdviceFirstConfig.class) @DirtiesContext class AfterAdviceFirstTests { @Test - void afterAdviceIsInvokedFirst(@Autowired Echo echo, @Autowired AfterAdviceFirstAspect aspect) throws Exception { + void afterAdviceIsNotInvokedLast(@Autowired Echo echo, @Autowired AfterAdviceFirstAspect aspect) throws Exception { assertThat(aspect.invocations).isEmpty(); assertThat(echo.echo(42)).isEqualTo(42); - assertThat(aspect.invocations).containsExactly("after", "after returning"); + assertThat(aspect.invocations).containsExactly("around - start", "before", "around - end", "after", "after returning"); aspect.invocations.clear(); assertThatExceptionOfType(Exception.class).isThrownBy( () -> echo.echo(new Exception())); - assertThat(aspect.invocations).containsExactly("after", "after throwing"); + assertThat(aspect.invocations).containsExactly("around - start", "before", "around - end", "after", "after throwing"); } } + /** + * This test class uses {@link AfterAdviceLastAspect} which declares its + * {@link After @After} advice as the last after advice type method + * in its source code. + * + *

On Java versions prior to JDK 7, we would have expected the {@code @After} + * advice method to be invoked after {@code @AfterThrowing} and + * {@code @AfterReturning} advice methods due to the AspectJ precedence + * rules implemented in + * {@link org.springframework.aop.aspectj.autoproxy.AspectJPrecedenceComparator}. + */ @Nested @SpringJUnitConfig(AfterAdviceLastConfig.class) @DirtiesContext @@ -75,14 +92,12 @@ class AspectJAutoProxyAdviceOrderIntegrationTests { void afterAdviceIsNotInvokedLast(@Autowired Echo echo, @Autowired AfterAdviceLastAspect aspect) throws Exception { assertThat(aspect.invocations).isEmpty(); assertThat(echo.echo(42)).isEqualTo(42); - // On Java versions prior to JDK 7, we would expect the @After advice to be invoked last. - assertThat(aspect.invocations).containsExactly("after", "after returning"); + assertThat(aspect.invocations).containsExactly("around - start", "before", "around - end", "after", "after returning"); aspect.invocations.clear(); assertThatExceptionOfType(Exception.class).isThrownBy( () -> echo.echo(new Exception())); - // On Java versions prior to JDK 7, we would expect the @After advice to be invoked last. - assertThat(aspect.invocations).containsExactly("after", "after throwing"); + assertThat(aspect.invocations).containsExactly("around - start", "before", "around - end", "after", "after throwing"); } } @@ -145,14 +160,30 @@ class AspectJAutoProxyAdviceOrderIntegrationTests { } @AfterReturning("echo()") - void succeeded() { + void afterReturning() { invocations.add("after returning"); } @AfterThrowing("echo()") - void failed() { + void afterThrowing() { invocations.add("after throwing"); } + + @Before("echo()") + void before() { + invocations.add("before"); + } + + @Around("echo()") + Object around(ProceedingJoinPoint joinPoint) throws Throwable { + invocations.add("around - start"); + try { + return joinPoint.proceed(); + } + finally { + invocations.add("around - end"); + } + } } /** @@ -167,13 +198,29 @@ class AspectJAutoProxyAdviceOrderIntegrationTests { void echo() { } + @Around("echo()") + Object around(ProceedingJoinPoint joinPoint) throws Throwable { + invocations.add("around - start"); + try { + return joinPoint.proceed(); + } + finally { + invocations.add("around - end"); + } + } + + @Before("echo()") + void before() { + invocations.add("before"); + } + @AfterReturning("echo()") - void succeeded() { + void afterReturning() { invocations.add("after returning"); } @AfterThrowing("echo()") - void failed() { + void afterThrowing() { invocations.add("after throwing"); } diff --git a/integration-tests/src/test/resources/org/springframework/aop/config/AopNamespaceHandlerAdviceOrderIntegrationTests-afterFirst.xml b/integration-tests/src/test/resources/org/springframework/aop/config/AopNamespaceHandlerAdviceOrderIntegrationTests-afterFirst.xml index 6293b21982..fd98a0c1b4 100644 --- a/integration-tests/src/test/resources/org/springframework/aop/config/AopNamespaceHandlerAdviceOrderIntegrationTests-afterFirst.xml +++ b/integration-tests/src/test/resources/org/springframework/aop/config/AopNamespaceHandlerAdviceOrderIntegrationTests-afterFirst.xml @@ -7,14 +7,16 @@ - + - + + + - - + + diff --git a/integration-tests/src/test/resources/org/springframework/aop/config/AopNamespaceHandlerAdviceOrderIntegrationTests-afterLast.xml b/integration-tests/src/test/resources/org/springframework/aop/config/AopNamespaceHandlerAdviceOrderIntegrationTests-afterLast.xml index 376896533a..bcab0df678 100644 --- a/integration-tests/src/test/resources/org/springframework/aop/config/AopNamespaceHandlerAdviceOrderIntegrationTests-afterLast.xml +++ b/integration-tests/src/test/resources/org/springframework/aop/config/AopNamespaceHandlerAdviceOrderIntegrationTests-afterLast.xml @@ -7,13 +7,15 @@ - + - + - - + + + + 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 67a7f0aa78..c7bad57501 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 @@ -455,7 +455,7 @@ abstract class AbstractAspectJAdvisorFactoryTests { TestBean target = new TestBean(); UnsupportedOperationException expectedException = new UnsupportedOperationException(); List advisors = getFixture().getAdvisors( - new SingletonMetadataAwareAspectInstanceFactory(new ExceptionAspect(expectedException), "someBean")); + new SingletonMetadataAwareAspectInstanceFactory(new ExceptionThrowingAspect(expectedException), "someBean")); assertThat(advisors.size()).as("One advice method was found").isEqualTo(1); ITestBean itb = (ITestBean) createProxy(target, advisors, ITestBean.class); assertThatExceptionOfType(UnsupportedOperationException.class).isThrownBy( @@ -469,7 +469,7 @@ abstract class AbstractAspectJAdvisorFactoryTests { TestBean target = new TestBean(); RemoteException expectedException = new RemoteException(); List advisors = getFixture().getAdvisors( - new SingletonMetadataAwareAspectInstanceFactory(new ExceptionAspect(expectedException), "someBean")); + new SingletonMetadataAwareAspectInstanceFactory(new ExceptionThrowingAspect(expectedException), "someBean")); assertThat(advisors.size()).as("One advice method was found").isEqualTo(1); ITestBean itb = (ITestBean) createProxy(target, advisors, ITestBean.class); assertThatExceptionOfType(UndeclaredThrowableException.class).isThrownBy( @@ -510,18 +510,18 @@ abstract class AbstractAspectJAdvisorFactoryTests { @Test void afterAdviceTypes() throws Exception { - ExceptionHandling aspect = new ExceptionHandling(); + InvocationTrackingAspect aspect = new InvocationTrackingAspect(); List advisors = getFixture().getAdvisors( new SingletonMetadataAwareAspectInstanceFactory(aspect, "exceptionHandlingAspect")); Echo echo = (Echo) createProxy(new Echo(), advisors, Echo.class); assertThat(aspect.invocations).isEmpty(); assertThat(echo.echo(42)).isEqualTo(42); - assertThat(aspect.invocations).containsExactly("after returning", "after"); + assertThat(aspect.invocations).containsExactly("around - start", "before", "after returning", "after", "around - end"); aspect.invocations.clear(); assertThatExceptionOfType(FileNotFoundException.class).isThrownBy(() -> echo.echo(new FileNotFoundException())); - assertThat(aspect.invocations).containsExactly("after throwing", "after"); + assertThat(aspect.invocations).containsExactly("around - start", "before", "after throwing", "after", "around - end"); } @Test @@ -742,11 +742,11 @@ abstract class AbstractAspectJAdvisorFactoryTests { @Aspect - static class ExceptionAspect { + static class ExceptionThrowingAspect { private final Exception ex; - ExceptionAspect(Exception ex) { + ExceptionThrowingAspect(Exception ex) { this.ex = ex; } @@ -769,7 +769,7 @@ abstract class AbstractAspectJAdvisorFactoryTests { @Aspect - static class ExceptionHandling { + private static class InvocationTrackingAspect { List invocations = new ArrayList<>(); @@ -778,13 +778,29 @@ abstract class AbstractAspectJAdvisorFactoryTests { void echo() { } + @Around("echo()") + Object around(ProceedingJoinPoint joinPoint) throws Throwable { + invocations.add("around - start"); + try { + return joinPoint.proceed(); + } + finally { + invocations.add("around - end"); + } + } + + @Before("echo()") + void before() { + invocations.add("before"); + } + @AfterReturning("echo()") - void succeeded() { + void afterReturning() { invocations.add("after returning"); } @AfterThrowing("echo()") - void failed() { + void afterThrowing() { invocations.add("after throwing"); } diff --git a/spring-aop/src/test/java/org/springframework/aop/aspectj/annotation/AspectJPointcutAdvisorTests.java b/spring-aop/src/test/java/org/springframework/aop/aspectj/annotation/AspectJPointcutAdvisorTests.java index b944da483e..806f30587b 100644 --- a/spring-aop/src/test/java/org/springframework/aop/aspectj/annotation/AspectJPointcutAdvisorTests.java +++ b/spring-aop/src/test/java/org/springframework/aop/aspectj/annotation/AspectJPointcutAdvisorTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2019 the original author or authors. + * Copyright 2002-2020 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. @@ -22,6 +22,7 @@ import test.aop.PerTargetAspect; import org.springframework.aop.Pointcut; import org.springframework.aop.aspectj.AspectJExpressionPointcut; import org.springframework.aop.aspectj.AspectJExpressionPointcutTests; +import org.springframework.aop.aspectj.annotation.AbstractAspectJAdvisorFactoryTests.ExceptionThrowingAspect; import org.springframework.aop.framework.AopConfigException; import org.springframework.beans.testfixture.beans.TestBean; @@ -44,7 +45,7 @@ public class AspectJPointcutAdvisorTests { InstantiationModelAwarePointcutAdvisorImpl ajpa = new InstantiationModelAwarePointcutAdvisorImpl( ajexp, TestBean.class.getMethod("getAge"), af, - new SingletonMetadataAwareAspectInstanceFactory(new AbstractAspectJAdvisorFactoryTests.ExceptionAspect(null), "someBean"), + new SingletonMetadataAwareAspectInstanceFactory(new ExceptionThrowingAspect(null), "someBean"), 1, "someBean"); assertThat(ajpa.getAspectMetadata().getPerClausePointcut()).isSameAs(Pointcut.TRUE); diff --git a/spring-aop/src/test/java/org/springframework/aop/aspectj/annotation/AspectMetadataTests.java b/spring-aop/src/test/java/org/springframework/aop/aspectj/annotation/AspectMetadataTests.java index 55fa481ece..637baa2450 100644 --- a/spring-aop/src/test/java/org/springframework/aop/aspectj/annotation/AspectMetadataTests.java +++ b/spring-aop/src/test/java/org/springframework/aop/aspectj/annotation/AspectMetadataTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2019 the original author or authors. + * Copyright 2002-2020 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. @@ -22,7 +22,7 @@ import test.aop.PerTargetAspect; import org.springframework.aop.Pointcut; import org.springframework.aop.aspectj.AspectJExpressionPointcut; -import org.springframework.aop.aspectj.annotation.AbstractAspectJAdvisorFactoryTests.ExceptionAspect; +import org.springframework.aop.aspectj.annotation.AbstractAspectJAdvisorFactoryTests.ExceptionThrowingAspect; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException; @@ -42,7 +42,7 @@ class AspectMetadataTests { @Test void singletonAspect() { - AspectMetadata am = new AspectMetadata(ExceptionAspect.class, "someBean"); + AspectMetadata am = new AspectMetadata(ExceptionThrowingAspect.class, "someBean"); assertThat(am.isPerThisOrPerTarget()).isFalse(); assertThat(am.getPerClausePointcut()).isSameAs(Pointcut.TRUE); assertThat(am.getAjType().getPerClause().getKind()).isEqualTo(PerClauseKind.SINGLETON);