From 965dd66f8cfc162de94d517c4e5515d6002ac874 Mon Sep 17 00:00:00 2001 From: Sam Brannen Date: Tue, 11 Oct 2022 16:41:19 +0200 Subject: [PATCH 1/2] Improve Javadoc for RepeatableContainers --- .../core/annotation/RepeatableContainers.java | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/spring-core/src/main/java/org/springframework/core/annotation/RepeatableContainers.java b/spring-core/src/main/java/org/springframework/core/annotation/RepeatableContainers.java index a235a581b0..9564e1cbe6 100644 --- a/spring-core/src/main/java/org/springframework/core/annotation/RepeatableContainers.java +++ b/spring-core/src/main/java/org/springframework/core/annotation/RepeatableContainers.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2019 the original author or authors. + * Copyright 2002-2022 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. @@ -39,6 +39,7 @@ import org.springframework.util.ReflectionUtils; *

To completely disable repeatable support use {@link #none()}. * * @author Phillip Webb + * @author Sam Brannen * @since 5.2 */ public abstract class RepeatableContainers { @@ -101,15 +102,19 @@ public abstract class RepeatableContainers { } /** - * Create a {@link RepeatableContainers} instance that uses a defined - * container and repeatable type. - * @param repeatable the contained repeatable annotation - * @param container the container annotation or {@code null}. If specified, + * Create a {@link RepeatableContainers} instance that uses predefined + * repeatable and container types. + * @param repeatable the contained repeatable annotation type + * @param container the container annotation type or {@code null}. If specified, * this annotation must declare a {@code value} attribute returning an array * of repeatable annotations. If not specified, the container will be * deduced by inspecting the {@code @Repeatable} annotation on * {@code repeatable}. * @return a {@link RepeatableContainers} instance + * @throws IllegalArgumentException if the supplied container type is + * {@code null} and the annotation type is not a repeatable annotation + * @throws AnnotationConfigurationException if the supplied container type + * is not a properly configured container for a repeatable annotation */ public static RepeatableContainers of( Class repeatable, @Nullable Class container) { From 828f74f71a068f30b9c158f2a182f7fb9dc50b5e Mon Sep 17 00:00:00 2001 From: Sam Brannen Date: Tue, 11 Oct 2022 17:19:02 +0200 Subject: [PATCH 2/2] Support nesting in AnnotatedElementUtils.findMergedRepeatableAnnotations() Prior to this commit, the findMergedRepeatableAnnotations() methods in AnnotatedElementUtils failed to find repeatable annotations declared on other repeatable annotations (i.e., when one repeatable annotation type was used as a meta-annotation on a different repeatable annotation type). The reason is that findMergedRepeatableAnnotations(element, annotationType, containerType) always used RepeatableContainers.of(annotationType, containerType) to create a RepeatableContainers instance, even if the supplied containerType was null. Doing so restricts the search to supporting only repeatable annotations whose container is the supplied containerType and prevents the search from finding repeatable annotations declared as meta-annotations on other types of repeatable annotations. Note, however, that direct use of the MergedAnnotations API already supported finding nested repeatable annotations when using RepeatableContainers.standardRepeatables() or RepeatableContainers.of(...).and(...).and(...). The latter composes support for multiple repeatable annotation types and their containers. This commit addresses the issue for findMergedRepeatableAnnotations() when the containerType is null or not provided. However, findMergedRepeatableAnnotations(element, annotationType, containerType) still suffers from the aforementioned limitation, and the Javadoc has been updated to make that clear. Closes gh-20279 --- .../annotation/AnnotatedElementUtils.java | 30 ++- .../NestedRepeatableAnnotationsTests.java | 177 ++++++++++++++++++ 2 files changed, 205 insertions(+), 2 deletions(-) create mode 100644 spring-core/src/test/java/org/springframework/core/annotation/NestedRepeatableAnnotationsTests.java diff --git a/spring-core/src/main/java/org/springframework/core/annotation/AnnotatedElementUtils.java b/spring-core/src/main/java/org/springframework/core/annotation/AnnotatedElementUtils.java index 850cee07d2..e5a107c574 100644 --- a/spring-core/src/main/java/org/springframework/core/annotation/AnnotatedElementUtils.java +++ b/spring-core/src/main/java/org/springframework/core/annotation/AnnotatedElementUtils.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2019 the original author or authors. + * Copyright 2002-2022 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. @@ -725,6 +725,16 @@ public abstract class AnnotatedElementUtils { * single annotation and within annotation hierarchies. *

This method follows find semantics as described in the * {@linkplain AnnotatedElementUtils class-level javadoc}. + *

WARNING: if the supplied {@code containerType} is not + * {@code null}, the search will be restricted to supporting only repeatable + * annotations whose container is the supplied {@code containerType}. This + * prevents the search from finding repeatable annotations declared as + * meta-annotations on other types of repeatable annotations. If you need to + * support such a use case, favor {@link #findMergedRepeatableAnnotations(AnnotatedElement, Class)} + * over this method or alternatively use the {@link MergedAnnotations} API + * directly in conjunction with {@link RepeatableContainers} that are + * {@linkplain RepeatableContainers#and(Class, Class) composed} to support + * multiple repeatable annotation types. * @param element the annotated element (never {@code null}) * @param annotationType the annotation type to find (never {@code null}) * @param containerType the type of the container that holds the annotations; @@ -767,7 +777,23 @@ public abstract class AnnotatedElementUtils { private static MergedAnnotations findRepeatableAnnotations(AnnotatedElement element, @Nullable Class containerType, Class annotationType) { - RepeatableContainers repeatableContainers = RepeatableContainers.of(annotationType, containerType); + RepeatableContainers repeatableContainers; + if (containerType == null) { + // Invoke RepeatableContainers.of() in order to adhere to the contract of + // findMergedRepeatableAnnotations() which states that an IllegalArgumentException + // will be thrown if the the container cannot be resolved. + // + // In any case, we use standardRepeatables() in order to support repeatable + // annotations on other types of repeatable annotations (i.e., nested repeatable + // annotation types). + // + // See https://github.com/spring-projects/spring-framework/issues/20279 + RepeatableContainers.of(annotationType, null); + repeatableContainers = RepeatableContainers.standardRepeatables(); + } + else { + repeatableContainers = RepeatableContainers.of(annotationType, containerType); + } return MergedAnnotations.from(element, SearchStrategy.TYPE_HIERARCHY, repeatableContainers); } diff --git a/spring-core/src/test/java/org/springframework/core/annotation/NestedRepeatableAnnotationsTests.java b/spring-core/src/test/java/org/springframework/core/annotation/NestedRepeatableAnnotationsTests.java new file mode 100644 index 0000000000..97df12dee4 --- /dev/null +++ b/spring-core/src/test/java/org/springframework/core/annotation/NestedRepeatableAnnotationsTests.java @@ -0,0 +1,177 @@ +/* + * Copyright 2002-2022 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. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.core.annotation; + +import java.lang.annotation.ElementType; +import java.lang.annotation.Repeatable; +import java.lang.annotation.Retention; +import java.lang.annotation.RetentionPolicy; +import java.lang.annotation.Target; +import java.lang.reflect.Method; +import java.util.Set; + +import org.junit.jupiter.api.Nested; +import org.junit.jupiter.api.Test; + +import org.springframework.core.annotation.MergedAnnotations.SearchStrategy; +import org.springframework.util.ReflectionUtils; + +import static org.assertj.core.api.Assertions.assertThat; + +/** + * Tests for various ways to search for repeatable annotations that are + * nested (i.e., repeatable annotations used as meta-annotations on other + * repeatable annotations). + * + * @author Sam Brannen + * @since 5.3.24 + * @see https://github.com/spring-projects/spring-framework/issues/20279 + */ +@SuppressWarnings("unused") +class NestedRepeatableAnnotationsTests { + + @Nested + class SingleRepeatableAnnotationTests { + + private final Method method = ReflectionUtils.findMethod(getClass(), "annotatedMethod"); + + @Test + void streamRepeatableAnnotations_MergedAnnotationsApi() { + Set annotations = MergedAnnotations.from(method, SearchStrategy.TYPE_HIERARCHY) + .stream(A.class).collect(MergedAnnotationCollectors.toAnnotationSet()); + // Merged, so we expect to find @A once with its value coming from @B(5). + assertThat(annotations).extracting(A::value).containsExactly(5); + } + + @Test + void findMergedRepeatableAnnotations_AnnotatedElementUtils() { + Set annotations = AnnotatedElementUtils.findMergedRepeatableAnnotations(method, A.class); + // Merged, so we expect to find @A once with its value coming from @B(5). + assertThat(annotations).extracting(A::value).containsExactly(5); + } + + @Test + @SuppressWarnings("deprecation") + void getRepeatableAnnotations_AnnotationUtils() { + Set annotations = AnnotationUtils.getRepeatableAnnotations(method, A.class); + // Not merged, so we expect to find @A once with the default value of 0. + // @A will actually be found twice, but we have Set semantics here. + assertThat(annotations).extracting(A::value).containsExactly(0); + } + + @B(5) + void annotatedMethod() { + } + + } + + @Nested + class MultipleRepeatableAnnotationsTests { + + private final Method method = ReflectionUtils.findMethod(getClass(), "annotatedMethod"); + + @Test + void streamRepeatableAnnotationsWithStandardRepeatables_MergedAnnotationsApi() { + RepeatableContainers repeatableContainers = RepeatableContainers.standardRepeatables(); + Set annotations = MergedAnnotations.from(method, SearchStrategy.TYPE_HIERARCHY, repeatableContainers) + .stream(A.class).collect(MergedAnnotationCollectors.toAnnotationSet()); + // Merged, so we expect to find @A twice with values coming from @B(5) and @B(10). + assertThat(annotations).extracting(A::value).containsExactly(5, 10); + } + + @Test + void streamRepeatableAnnotationsWithExplicitRepeatables_MergedAnnotationsApi() { + RepeatableContainers repeatableContainers = + RepeatableContainers.of(A.class, A.Container.class).and(B.Container.class, B.class); + Set annotations = MergedAnnotations.from(method, SearchStrategy.TYPE_HIERARCHY, repeatableContainers) + .stream(A.class).collect(MergedAnnotationCollectors.toAnnotationSet()); + // Merged, so we expect to find @A twice with values coming from @B(5) and @B(10). + assertThat(annotations).extracting(A::value).containsExactly(5, 10); + } + + @Test + void findMergedRepeatableAnnotationsWithStandardRepeatables_AnnotatedElementUtils() { + Set annotations = AnnotatedElementUtils.findMergedRepeatableAnnotations(method, A.class); + // Merged, so we expect to find @A twice with values coming from @B(5) and @B(10). + // However, findMergedRepeatableAnnotations() currently finds ZERO annotations. + assertThat(annotations).extracting(A::value).containsExactly(5, 10); + } + + @Test + void findMergedRepeatableAnnotationsWithExplicitContainer_AnnotatedElementUtils() { + Set annotations = AnnotatedElementUtils.findMergedRepeatableAnnotations(method, A.class, A.Container.class); + // When findMergedRepeatableAnnotations(...) is invoked with an explicit container + // type, it uses RepeatableContainers.of(...) which limits the repeatable annotation + // support to a single container type. + // + // In this test case, we are therefore limiting the support to @A.Container, which + // means that @B.Container is unsupported and effectively ignored as a repeatable + // container type. + // + // Long story, short: the search doesn't find anything. + assertThat(annotations).isEmpty(); + } + + @Test + @SuppressWarnings("deprecation") + void getRepeatableAnnotations_AnnotationUtils() { + Set annotations = AnnotationUtils.getRepeatableAnnotations(method, A.class); + // Not merged, so we expect to find a single @A with default value of 0. + // @A will actually be found twice, but we have Set semantics here. + assertThat(annotations).extracting(A::value).containsExactly(0); + } + + @B(5) + @B(10) + void annotatedMethod() { + } + + } + + + @Retention(RetentionPolicy.RUNTIME) + @Target({ ElementType.METHOD, ElementType.ANNOTATION_TYPE }) + @Repeatable(A.Container.class) + public @interface A { + + int value() default 0; + + @Retention(RetentionPolicy.RUNTIME) + @Target({ ElementType.METHOD, ElementType.ANNOTATION_TYPE }) + @interface Container { + A[] value(); + } + } + + @Retention(RetentionPolicy.RUNTIME) + @Target({ ElementType.METHOD, ElementType.ANNOTATION_TYPE }) + @Repeatable(B.Container.class) + @A + @A + public @interface B { + + @AliasFor(annotation = A.class) + int value(); + + @Retention(RetentionPolicy.RUNTIME) + @Target({ ElementType.METHOD, ElementType.ANNOTATION_TYPE }) + @interface Container { + B[] value(); + } + } + +}