From e25e690ad403649dbc38b54d190667ef4452f8b8 Mon Sep 17 00:00:00 2001 From: Sam Brannen Date: Sat, 8 Aug 2020 12:05:59 +0200 Subject: [PATCH] Assert preconditions for MergedAnnotations.from() factory methods Prior to this commit, if null values were supplied for the RepeatableContainers or AnnotationFilter arguments to `from()` factory methods in MergedAnnotations, certain operations would eventually result in a NullPointerException. This is to be expected; however, the NullPointerException is often swallowed and only logged at INFO level with an exception message similar to the following. > Failed to introspect annotations on org.example.MyClass: NullPointerException In such cases, the INFO log message is not helpful in diagnosing the problem. Furthermore, since the exception is swallowed, the desired operation (e.g., MergedAnnotations.stream(...)) simply returns no results. This commit improves the user experience by eagerly asserting non-null preconditions for required arguments in MergedAnnotations.from() factory methods. Closes gh-25568 --- .../core/annotation/MergedAnnotations.java | 5 +++++ .../annotation/MergedAnnotationsTests.java | 21 +++++++++++++++++++ 2 files changed, 26 insertions(+) diff --git a/spring-core/src/main/java/org/springframework/core/annotation/MergedAnnotations.java b/spring-core/src/main/java/org/springframework/core/annotation/MergedAnnotations.java index dc3452e70f..55dff9086f 100644 --- a/spring-core/src/main/java/org/springframework/core/annotation/MergedAnnotations.java +++ b/spring-core/src/main/java/org/springframework/core/annotation/MergedAnnotations.java @@ -25,6 +25,7 @@ import java.util.function.Predicate; import java.util.stream.Stream; import org.springframework.lang.Nullable; +import org.springframework.util.Assert; /** * Provides access to a collection of merged annotations, usually obtained @@ -345,6 +346,8 @@ public interface MergedAnnotations extends Iterable static MergedAnnotations from(AnnotatedElement element, SearchStrategy searchStrategy, RepeatableContainers repeatableContainers, AnnotationFilter annotationFilter) { + Assert.notNull(repeatableContainers, "RepeatableContainers must not be null"); + Assert.notNull(annotationFilter, "AnnotationFilter must not be null"); return TypeMappedAnnotations.from(element, searchStrategy, repeatableContainers, annotationFilter); } @@ -405,6 +408,8 @@ public interface MergedAnnotations extends Iterable static MergedAnnotations from(Object source, Annotation[] annotations, RepeatableContainers repeatableContainers, AnnotationFilter annotationFilter) { + Assert.notNull(repeatableContainers, "RepeatableContainers must not be null"); + Assert.notNull(annotationFilter, "AnnotationFilter must not be null"); return TypeMappedAnnotations.from(source, annotations, repeatableContainers, annotationFilter); } 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 959f7f22ea..e9e3586bb6 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 @@ -53,6 +53,7 @@ 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.assertThatIllegalArgumentException; import static org.assertj.core.api.Assertions.assertThatIllegalStateException; import static org.assertj.core.api.Assertions.entry; @@ -73,6 +74,26 @@ import static org.assertj.core.api.Assertions.entry; */ class MergedAnnotationsTests { + @Test + void fromPreconditions() { + SearchStrategy strategy = SearchStrategy.DIRECT; + RepeatableContainers containers = RepeatableContainers.standardRepeatables(); + + assertThatIllegalArgumentException() + .isThrownBy(() -> MergedAnnotations.from(getClass(), strategy, null, AnnotationFilter.PLAIN)) + .withMessage("RepeatableContainers must not be null"); + assertThatIllegalArgumentException() + .isThrownBy(() -> MergedAnnotations.from(getClass(), strategy, containers, null)) + .withMessage("AnnotationFilter must not be null"); + + assertThatIllegalArgumentException() + .isThrownBy(() -> MergedAnnotations.from(getClass(), new Annotation[0], null, AnnotationFilter.PLAIN)) + .withMessage("RepeatableContainers must not be null"); + assertThatIllegalArgumentException() + .isThrownBy(() -> MergedAnnotations.from(getClass(), new Annotation[0], containers, null)) + .withMessage("AnnotationFilter must not be null"); + } + @Test void streamWhenFromNonAnnotatedClass() { assertThat(MergedAnnotations.from(NonAnnotatedClass.class).