From 31fa1569c54649c7f3cbf7ec330e2a7232c7cd3a Mon Sep 17 00:00:00 2001 From: Sam Brannen Date: Mon, 20 Apr 2020 15:30:48 +0200 Subject: [PATCH] Avoid synthesizing annotations unnecessarily Prior to Spring Framework 5.2, some of our annotation utilities would not synthesize an annotation if it was already synthesized or not synthesizable (i.e., did not declare local aliases via @AliasFor and did not declare attributes that could override attributes in the meta-annotation hierarchy above the given annotation); however, we lost most of this functionality with the introduction of the new MergedAnnotations API. This commit revises the implementation of createSynthesized() in TypeMappedAnnotation so that, for invocations of MergedAnnotation.synthesize() and indirectly for invocations of AnnotatedElementUtils.findMergedAnnotation(), etc.: 1. An annotation that was previously synthesized will not be synthesized again. 2. An annotation that is not "synthesizable" will not be synthesized. For both of the above use cases, the original annotation is now returned from createSynthesized(). Closes gh-24861 --- .../core/annotation/TypeMappedAnnotation.java | 11 ++++ .../annotation/MergedAnnotationsTests.java | 57 +++++++++++++++++++ 2 files changed, 68 insertions(+) diff --git a/spring-core/src/main/java/org/springframework/core/annotation/TypeMappedAnnotation.java b/spring-core/src/main/java/org/springframework/core/annotation/TypeMappedAnnotation.java index 68a67ea7bc..15c674f9e3 100644 --- a/spring-core/src/main/java/org/springframework/core/annotation/TypeMappedAnnotation.java +++ b/spring-core/src/main/java/org/springframework/core/annotation/TypeMappedAnnotation.java @@ -329,7 +329,18 @@ final class TypeMappedAnnotation extends AbstractMergedAnn } @Override + @SuppressWarnings("unchecked") protected A createSynthesized() { + if (this.rootAttributes instanceof Annotation) { + // Nothing to synthesize? + if (this.resolvedRootMirrors.length == 0 && this.resolvedMirrors.length == 0) { + return (A) this.rootAttributes; + } + // Already synthesized? + if (this.rootAttributes instanceof SynthesizedAnnotation) { + return (A) this.rootAttributes; + } + } return SynthesizedMergedAnnotationInvocationHandler.createProxy(this, getType()); } diff --git a/spring-core/src/test/java/org/springframework/core/annotation/MergedAnnotationsTests.java b/spring-core/src/test/java/org/springframework/core/annotation/MergedAnnotationsTests.java index 8a9a90e6da..307f4d020c 100644 --- a/spring-core/src/test/java/org/springframework/core/annotation/MergedAnnotationsTests.java +++ b/spring-core/src/test/java/org/springframework/core/annotation/MergedAnnotationsTests.java @@ -50,6 +50,7 @@ import org.springframework.util.ClassUtils; import org.springframework.util.MultiValueMap; import org.springframework.util.ReflectionUtils; +import static java.lang.annotation.RetentionPolicy.RUNTIME; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatExceptionOfType; import static org.assertj.core.api.Assertions.assertThatIllegalStateException; @@ -1376,6 +1377,50 @@ class MergedAnnotationsTests { assertThat(synthesizedWebMapping.value()).containsExactly("/test"); } + @Test + void synthesizeShouldNotSynthesizeNonsynthesizableAnnotations() throws Exception { + Method method = getClass().getDeclaredMethod("getId"); + Id id = method.getAnnotation(Id.class); + assertThat(id).isNotNull(); + + Id synthesizedId = MergedAnnotation.from(id).synthesize(); + assertThat(id).isEqualTo(synthesizedId); + // It doesn't make sense to synthesize {@code @Id} since it declares zero attributes. + assertThat(synthesizedId).isNotInstanceOf(SynthesizedAnnotation.class); + assertThat(id).isSameAs(synthesizedId); + } + + /** + * If an attempt is made to synthesize an annotation from an annotation instance + * that has already been synthesized, the original synthesized annotation should + * ideally be returned as-is without creating a new proxy instance with the same + * values. + */ + @Test + void synthesizeShouldNotResynthesizeAlreadySynthesizedAnnotations() throws Exception { + Method method = WebController.class.getMethod("handleMappedWithValueAttribute"); + RequestMapping webMapping = method.getAnnotation(RequestMapping.class); + assertThat(webMapping).isNotNull(); + + MergedAnnotation mergedAnnotation1 = MergedAnnotation.from(webMapping); + RequestMapping synthesizedWebMapping1 = mergedAnnotation1.synthesize(); + RequestMapping synthesizedWebMapping2 = MergedAnnotation.from(webMapping).synthesize(); + + assertThat(synthesizedWebMapping1).isInstanceOf(SynthesizedAnnotation.class); + assertThat(synthesizedWebMapping2).isInstanceOf(SynthesizedAnnotation.class); + assertThat(synthesizedWebMapping1).isEqualTo(synthesizedWebMapping2); + + // Synthesizing an annotation from a different MergedAnnotation results in a different synthesized annotation instance. + assertThat(synthesizedWebMapping1).isNotSameAs(synthesizedWebMapping2); + // Synthesizing an annotation from the same MergedAnnotation results in the same synthesized annotation instance. + assertThat(synthesizedWebMapping1).isSameAs(mergedAnnotation1.synthesize()); + + RequestMapping synthesizedAgainWebMapping = MergedAnnotation.from(synthesizedWebMapping1).synthesize(); + assertThat(synthesizedWebMapping1).isEqualTo(synthesizedAgainWebMapping); + // Synthesizing an already synthesized annotation results in the original synthesized annotation instance. + assertThat(synthesizedWebMapping1).isSameAs(synthesizedAgainWebMapping); + } + @Test void synthesizeWhenAliasForIsMissingAttributeDeclaration() throws Exception { AliasForWithMissingAttributeDeclaration annotation = @@ -2951,6 +2996,18 @@ class MergedAnnotationsTests { } } + /** + * Mimics javax.persistence.Id + */ + @Retention(RUNTIME) + @interface Id { + } + + @Id + private Long getId() { + return 42L; + } + @Retention(RetentionPolicy.RUNTIME) @interface TestConfiguration {