Browse Source

Fix Checkstyle configuration for nullability annotations

While assessing #35195, I noticed the following issues with our
Checkstyle configuration regarding nullability annotations.

- "^(?!org\.jspecify|\.annotations).*(NonNull|Nullable)$" contains a "|".

- "^(?!org\.jspecify|\.annotations).*(NonNull|Nullable)$" matches against
  NonNull but not against Nonnull, and therefore incorrectly permits
  usage of javax.annotation.Nonnull.

- Some of the Checkstyle suppressions no longer apply.

This commit addresses all of the above issues and updates several tests
to use example annotations other than javax.annotation.Nonnull where
feasible.

See gh-35195
Closes gh-35205
pull/35212/head
Sam Brannen 5 months ago
parent
commit
4fdf40e39e
  1. 38
      spring-core/src/test/java/org/springframework/core/annotation/AnnotatedElementUtilsTests.java
  2. 10
      spring-core/src/test/java/org/springframework/core/annotation/AnnotationFilterTests.java
  3. 4
      spring-core/src/test/java/org/springframework/core/annotation/AnnotationTypeMappingsTests.java
  4. 9
      spring-core/src/test/java/org/springframework/core/annotation/AnnotationUtilsTests.java
  5. 7
      src/checkstyle/checkstyle-suppressions.xml
  6. 16
      src/checkstyle/checkstyle.xml

38
spring-core/src/test/java/org/springframework/core/annotation/AnnotatedElementUtilsTests.java

@ -31,8 +31,10 @@ import java.util.Date; @@ -31,8 +31,10 @@ import java.util.Date;
import java.util.List;
import java.util.Set;
import javax.annotation.Nonnull;
import javax.annotation.ParametersAreNonnullByDefault;
import javax.annotation.RegEx;
import javax.annotation.Syntax;
import javax.annotation.concurrent.ThreadSafe;
import javax.annotation.meta.TypeQualifierNickname;
import javax.annotation.meta.When;
import jakarta.annotation.Resource;
@ -46,6 +48,7 @@ import org.springframework.core.annotation.AnnotationUtilsTests.WebController; @@ -46,6 +48,7 @@ import org.springframework.core.annotation.AnnotationUtilsTests.WebController;
import org.springframework.core.annotation.AnnotationUtilsTests.WebMapping;
import org.springframework.core.testfixture.stereotype.Component;
import org.springframework.core.testfixture.stereotype.Indexed;
import org.springframework.lang.Contract;
import org.springframework.util.MultiValueMap;
import static java.util.Arrays.asList;
@ -237,9 +240,8 @@ class AnnotatedElementUtilsTests { @@ -237,9 +240,8 @@ class AnnotatedElementUtilsTests {
@SuppressWarnings("deprecation")
void isAnnotatedForPlainTypes() {
assertThat(isAnnotated(Order.class, Documented.class)).isTrue();
assertThat(isAnnotated(org.springframework.lang.NonNullApi.class, Documented.class)).isTrue();
assertThat(isAnnotated(org.springframework.lang.NonNullApi.class, Nonnull.class)).isTrue();
assertThat(isAnnotated(ParametersAreNonnullByDefault.class, Nonnull.class)).isTrue();
assertThat(isAnnotated(Inherited.class, Documented.class)).isTrue();
assertThat(isAnnotated(Contract.class, Documented.class)).isTrue();
}
@Test
@ -278,9 +280,8 @@ class AnnotatedElementUtilsTests { @@ -278,9 +280,8 @@ class AnnotatedElementUtilsTests {
@SuppressWarnings("deprecation")
void hasAnnotationForPlainTypes() {
assertThat(hasAnnotation(Order.class, Documented.class)).isTrue();
assertThat(hasAnnotation(org.springframework.lang.NonNullApi.class, Documented.class)).isTrue();
assertThat(hasAnnotation(org.springframework.lang.NonNullApi.class, Nonnull.class)).isTrue();
assertThat(hasAnnotation(ParametersAreNonnullByDefault.class, Nonnull.class)).isTrue();
assertThat(hasAnnotation(Inherited.class, Documented.class)).isTrue();
assertThat(hasAnnotation(Contract.class, Documented.class)).isTrue();
}
@Test
@ -346,17 +347,16 @@ class AnnotatedElementUtilsTests { @@ -346,17 +347,16 @@ class AnnotatedElementUtilsTests {
@SuppressWarnings("deprecation")
void getAllAnnotationAttributesOnLangType() {
MultiValueMap<String, Object> attributes = getAllAnnotationAttributes(
org.springframework.lang.NonNullApi.class, Nonnull.class.getName());
assertThat(attributes).as("Annotation attributes map for @Nonnull on NonNullApi").isNotNull();
assertThat(attributes.get("when")).as("value for NonNullApi").isEqualTo(List.of(When.ALWAYS));
org.springframework.lang.NonNullApi.class, javax.annotation.Nonnull.class.getName());
assertThat(attributes).as("Annotation attributes map for @Nonnull on @NonNullApi").isNotNull();
assertThat(attributes.get("when")).as("value for @NonNullApi").isEqualTo(List.of(When.ALWAYS));
}
@Test
void getAllAnnotationAttributesOnJavaxType() {
MultiValueMap<String, Object> attributes = getAllAnnotationAttributes(
ParametersAreNonnullByDefault.class, Nonnull.class.getName());
assertThat(attributes).as("Annotation attributes map for @Nonnull on NonNullApi").isNotNull();
assertThat(attributes.get("when")).as("value for NonNullApi").isEqualTo(List.of(When.ALWAYS));
MultiValueMap<String, Object> attributes = getAllAnnotationAttributes(RegEx.class, Syntax.class.getName());
assertThat(attributes).as("Annotation attributes map for @Syntax on @RegEx").isNotNull();
assertThat(attributes.get("when")).as("value for @RegEx").isEqualTo(List.of(When.ALWAYS));
}
@Test
@ -855,8 +855,10 @@ class AnnotatedElementUtilsTests { @@ -855,8 +855,10 @@ class AnnotatedElementUtilsTests {
@Test
void javaxMetaAnnotationTypeViaFindMergedAnnotation() {
assertThat(findMergedAnnotation(ParametersAreNonnullByDefault.class, Nonnull.class)).isEqualTo(ParametersAreNonnullByDefault.class.getAnnotation(Nonnull.class));
assertThat(findMergedAnnotation(ResourceHolder.class, Nonnull.class)).isEqualTo(ParametersAreNonnullByDefault.class.getAnnotation(Nonnull.class));
assertThat(findMergedAnnotation(ThreadSafe.class, Documented.class))
.isEqualTo(ThreadSafe.class.getAnnotation(Documented.class));
assertThat(findMergedAnnotation(ResourceHolder.class, TypeQualifierNickname.class))
.isEqualTo(RegEx.class.getAnnotation(TypeQualifierNickname.class));
}
@Test
@ -1536,7 +1538,7 @@ class AnnotatedElementUtilsTests { @@ -1536,7 +1538,7 @@ class AnnotatedElementUtilsTests {
}
@Resource(name = "x")
@ParametersAreNonnullByDefault
@RegEx
static class ResourceHolder {
}

10
spring-core/src/test/java/org/springframework/core/annotation/AnnotationFilterTests.java

@ -19,9 +19,8 @@ package org.springframework.core.annotation; @@ -19,9 +19,8 @@ package org.springframework.core.annotation;
import java.lang.annotation.Retention;
import java.lang.annotation.RetentionPolicy;
import javax.annotation.Nonnull;
import javax.annotation.concurrent.ThreadSafe;
import org.jspecify.annotations.Nullable;
import org.junit.jupiter.api.Test;
import org.springframework.lang.Contract;
@ -86,12 +85,13 @@ class AnnotationFilterTests { @@ -86,12 +85,13 @@ class AnnotationFilterTests {
@Test
void javaWhenJavaxAnnotationReturnsTrue() {
assertThat(AnnotationFilter.JAVA.matches(Nonnull.class)).isTrue();
assertThat(AnnotationFilter.JAVA.matches(ThreadSafe.class)).isTrue();
}
@Test
@SuppressWarnings("deprecation")
void javaWhenSpringLangAnnotationReturnsFalse() {
assertThat(AnnotationFilter.JAVA.matches(Nullable.class)).isFalse();
assertThat(AnnotationFilter.JAVA.matches(org.springframework.lang.Nullable.class)).isFalse();
}
@Test
@ -103,7 +103,7 @@ class AnnotationFilterTests { @@ -103,7 +103,7 @@ class AnnotationFilterTests {
@SuppressWarnings("deprecation")
void noneReturnsFalse() {
assertThat(AnnotationFilter.NONE.matches(Retention.class)).isFalse();
assertThat(AnnotationFilter.NONE.matches(Nullable.class)).isFalse();
assertThat(AnnotationFilter.NONE.matches(org.springframework.lang.Nullable.class)).isFalse();
assertThat(AnnotationFilter.NONE.matches(TestAnnotation.class)).isFalse();
}

4
spring-core/src/test/java/org/springframework/core/annotation/AnnotationTypeMappingsTests.java

@ -29,8 +29,6 @@ import java.util.List; @@ -29,8 +29,6 @@ import java.util.List;
import java.util.Map;
import java.util.stream.IntStream;
import javax.annotation.Nullable;
import org.junit.jupiter.api.Test;
import org.springframework.core.annotation.AnnotationTypeMapping.MirrorSets;
@ -442,7 +440,7 @@ class AnnotationTypeMappingsTests { @@ -442,7 +440,7 @@ class AnnotationTypeMappingsTests {
return result;
}
private @Nullable Method getAliasMapping(AnnotationTypeMapping mapping, int attributeIndex) {
private Method getAliasMapping(AnnotationTypeMapping mapping, int attributeIndex) {
int mapped = mapping.getAliasMapping(attributeIndex);
return mapped != -1 ? mapping.getRoot().getAttributes().get(mapped) : null;
}

9
spring-core/src/test/java/org/springframework/core/annotation/AnnotationUtilsTests.java

@ -30,8 +30,9 @@ import java.util.Map; @@ -30,8 +30,9 @@ import java.util.Map;
import java.util.NoSuchElementException;
import java.util.Set;
import javax.annotation.Nonnull;
import javax.annotation.ParametersAreNonnullByDefault;
import javax.annotation.RegEx;
import javax.annotation.Syntax;
import javax.annotation.concurrent.ThreadSafe;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
@ -428,8 +429,8 @@ class AnnotationUtilsTests { @@ -428,8 +429,8 @@ class AnnotationUtilsTests {
@Test
void isAnnotationMetaPresentForPlainType() {
assertThat(isAnnotationMetaPresent(Order.class, Documented.class)).isTrue();
assertThat(isAnnotationMetaPresent(ParametersAreNonnullByDefault.class, Documented.class)).isTrue();
assertThat(isAnnotationMetaPresent(ParametersAreNonnullByDefault.class, Nonnull.class)).isTrue();
assertThat(isAnnotationMetaPresent(ThreadSafe.class, Documented.class)).isTrue();
assertThat(isAnnotationMetaPresent(RegEx.class, Syntax.class)).isTrue();
}
@Test

7
src/checkstyle/checkstyle-suppressions.xml

@ -21,9 +21,6 @@ @@ -21,9 +21,6 @@
<!-- Framework docs -->
<suppress files="org[\\/]springframework[\\/]docs[\\/].+" checks="IllegalImport" id="bannedJUnitJupiterImports" />
<!-- spring-aop -->
<suppress files="[\\/]src[\\/]main[\\/]java[\\/]org[\\/]aopalliance[\\/]" checks="IllegalImport" id="bannedImports" message="javax"/>
<!-- spring-beans -->
<suppress files="TypeMismatchException" checks="MutableException"/>
<suppress files="BeanCreationException" checks="MutableException"/>
@ -40,9 +37,7 @@ @@ -40,9 +37,7 @@
<suppress files="[\\/]src[\\/]main[\\/]java[\\/]org[\\/]springframework[\\/](asm|cglib|objenesis|javapoet)[\\/]" checks=".*"/>
<suppress files="[\\/]src[\\/]main[\\/]java[\\/]org[\\/]springframework[\\/]aot[\\/]nativex[\\/]feature[\\/]" checks="RegexpSinglelineJava" id="systemOutErrPrint"/>
<suppress files="[\\/]src[\\/]main[\\/]java[\\/]org[\\/]springframework[\\/](core|util)[\\/](SpringProperties|SystemPropertyUtils)" checks="RegexpSinglelineJava" id="systemOutErrPrint"/>
<suppress files="[\\/]src[\\/]main[\\/]java[\\/]org[\\/]springframework[\\/]lang[\\/]" checks="IllegalImport" id="bannedImports" message="javax"/>
<suppress files="[\\/]src[\\/]main[\\/]java[\\/]org[\\/]springframework[\\/]core[\\/]annotation[\\/]" checks="IllegalImport" id="bannedImports" message="javax"/>
<suppress files="[\\/]src[\\/]test[\\/]java[\\/]org[\\/]springframework[\\/]core[\\/]annotation[\\/]" checks="IllegalImport" id="bannedImports" message="javax"/>
<suppress files="[\\/]src[\\/]main[\\/]java[\\/]org[\\/]springframework[\\/]lang[\\/]NonNull.+" checks="IllegalImport" id="bannedNullabilityImports"/>
<suppress files="ByteArrayEncoder" checks="SpringLambda"/>
<suppress files="SocketUtils" checks="HideUtilityClassConstructor"/>
<suppress files="ResolvableType" checks="FinalClass"/>

16
src/checkstyle/checkstyle.xml

@ -104,10 +104,20 @@ @@ -104,10 +104,20 @@
<property name="sortStaticImportsAlphabetically" value="true"/>
</module>
<module name="com.puppycrawl.tools.checkstyle.checks.imports.IllegalImportCheck">
<property name="id" value="bannedImports"/>
<property name="id" value="bannedReactorImports"/>
<property name="regexp" value="true"/>
<property name="illegalClasses"
value="^reactor\.core\.support\.Assert,^org\.slf4j\.LoggerFactory,^(?!org\.jspecify|\.annotations).*(NonNull|Nullable)$"/>
<property name="illegalClasses" value="^reactor\.core\.support\.Assert"/>
</module>
<module name="com.puppycrawl.tools.checkstyle.checks.imports.IllegalImportCheck">
<property name="id" value="bannedSlf4jImports"/>
<property name="regexp" value="true"/>
<property name="illegalClasses" value="^org\.slf4j\.LoggerFactory"/>
</module>
<module name="com.puppycrawl.tools.checkstyle.checks.imports.IllegalImportCheck">
<property name="id" value="bannedNullabilityImports"/>
<property name="regexp" value="true"/>
<!-- Rejects all NonNull, Nonnull, and Nullable types that are NOT in the org.jspecify.annotations package. -->
<property name="illegalClasses" value="^(?!org\.jspecify\.annotations).*(Non[Nn]ull|Nullable)$"/>
</module>
<module name="com.puppycrawl.tools.checkstyle.checks.imports.IllegalImportCheck">
<property name="id" value="bannedJUnit3Imports"/>

Loading…
Cancel
Save