Skip to content

Commit

Permalink
Assert preconditions for MergedAnnotations.from() factory methods
Browse files Browse the repository at this point in the history
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
  • Loading branch information
sbrannen committed Aug 8, 2020
1 parent a614abe commit e25e690
Show file tree
Hide file tree
Showing 2 changed files with 26 additions and 0 deletions.
Expand Up @@ -25,6 +25,7 @@
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
Expand Down Expand Up @@ -345,6 +346,8 @@ static MergedAnnotations from(AnnotatedElement element, SearchStrategy searchStr
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);
}

Expand Down Expand Up @@ -405,6 +408,8 @@ static MergedAnnotations from(Object source, Annotation[] annotations, Repeatabl
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);
}

Expand Down
Expand Up @@ -53,6 +53,7 @@
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;

Expand All @@ -73,6 +74,26 @@
*/
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).
Expand Down

0 comments on commit e25e690

Please sign in to comment.