Skip to content

Commit 4fdf40e

Browse files
sbrannensbrannen
committed
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
1 parent aac61b8 commit 4fdf40e

File tree

6 files changed

+45
-39
lines changed

6 files changed

+45
-39
lines changed

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

Lines changed: 20 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -31,8 +31,10 @@
3131
import java.util.List;
3232
import java.util.Set;
3333

34-
import javax.annotation.Nonnull;
35-
import javax.annotation.ParametersAreNonnullByDefault;
34+
import javax.annotation.RegEx;
35+
import javax.annotation.Syntax;
36+
import javax.annotation.concurrent.ThreadSafe;
37+
import javax.annotation.meta.TypeQualifierNickname;
3638
import javax.annotation.meta.When;
3739

3840
import jakarta.annotation.Resource;
@@ -46,6 +48,7 @@
4648
import org.springframework.core.annotation.AnnotationUtilsTests.WebMapping;
4749
import org.springframework.core.testfixture.stereotype.Component;
4850
import org.springframework.core.testfixture.stereotype.Indexed;
51+
import org.springframework.lang.Contract;
4952
import org.springframework.util.MultiValueMap;
5053

5154
import static java.util.Arrays.asList;
@@ -237,9 +240,8 @@ void isAnnotatedOnClassWithMetaDepth() {
237240
@SuppressWarnings("deprecation")
238241
void isAnnotatedForPlainTypes() {
239242
assertThat(isAnnotated(Order.class, Documented.class)).isTrue();
240-
assertThat(isAnnotated(org.springframework.lang.NonNullApi.class, Documented.class)).isTrue();
241-
assertThat(isAnnotated(org.springframework.lang.NonNullApi.class, Nonnull.class)).isTrue();
242-
assertThat(isAnnotated(ParametersAreNonnullByDefault.class, Nonnull.class)).isTrue();
243+
assertThat(isAnnotated(Inherited.class, Documented.class)).isTrue();
244+
assertThat(isAnnotated(Contract.class, Documented.class)).isTrue();
243245
}
244246

245247
@Test
@@ -278,9 +280,8 @@ void hasAnnotationOnClassWithMetaDepth() {
278280
@SuppressWarnings("deprecation")
279281
void hasAnnotationForPlainTypes() {
280282
assertThat(hasAnnotation(Order.class, Documented.class)).isTrue();
281-
assertThat(hasAnnotation(org.springframework.lang.NonNullApi.class, Documented.class)).isTrue();
282-
assertThat(hasAnnotation(org.springframework.lang.NonNullApi.class, Nonnull.class)).isTrue();
283-
assertThat(hasAnnotation(ParametersAreNonnullByDefault.class, Nonnull.class)).isTrue();
283+
assertThat(hasAnnotation(Inherited.class, Documented.class)).isTrue();
284+
assertThat(hasAnnotation(Contract.class, Documented.class)).isTrue();
284285
}
285286

286287
@Test
@@ -346,17 +347,16 @@ void getAllAnnotationAttributesOnClassWithMultipleComposedAnnotations() {
346347
@SuppressWarnings("deprecation")
347348
void getAllAnnotationAttributesOnLangType() {
348349
MultiValueMap<String, Object> attributes = getAllAnnotationAttributes(
349-
org.springframework.lang.NonNullApi.class, Nonnull.class.getName());
350-
assertThat(attributes).as("Annotation attributes map for @Nonnull on NonNullApi").isNotNull();
351-
assertThat(attributes.get("when")).as("value for NonNullApi").isEqualTo(List.of(When.ALWAYS));
350+
org.springframework.lang.NonNullApi.class, javax.annotation.Nonnull.class.getName());
351+
assertThat(attributes).as("Annotation attributes map for @Nonnull on @NonNullApi").isNotNull();
352+
assertThat(attributes.get("when")).as("value for @NonNullApi").isEqualTo(List.of(When.ALWAYS));
352353
}
353354

354355
@Test
355356
void getAllAnnotationAttributesOnJavaxType() {
356-
MultiValueMap<String, Object> attributes = getAllAnnotationAttributes(
357-
ParametersAreNonnullByDefault.class, Nonnull.class.getName());
358-
assertThat(attributes).as("Annotation attributes map for @Nonnull on NonNullApi").isNotNull();
359-
assertThat(attributes.get("when")).as("value for NonNullApi").isEqualTo(List.of(When.ALWAYS));
357+
MultiValueMap<String, Object> attributes = getAllAnnotationAttributes(RegEx.class, Syntax.class.getName());
358+
assertThat(attributes).as("Annotation attributes map for @Syntax on @RegEx").isNotNull();
359+
assertThat(attributes.get("when")).as("value for @RegEx").isEqualTo(List.of(When.ALWAYS));
360360
}
361361

362362
@Test
@@ -855,8 +855,10 @@ void javaxAnnotationTypeViaFindMergedAnnotation() {
855855

856856
@Test
857857
void javaxMetaAnnotationTypeViaFindMergedAnnotation() {
858-
assertThat(findMergedAnnotation(ParametersAreNonnullByDefault.class, Nonnull.class)).isEqualTo(ParametersAreNonnullByDefault.class.getAnnotation(Nonnull.class));
859-
assertThat(findMergedAnnotation(ResourceHolder.class, Nonnull.class)).isEqualTo(ParametersAreNonnullByDefault.class.getAnnotation(Nonnull.class));
858+
assertThat(findMergedAnnotation(ThreadSafe.class, Documented.class))
859+
.isEqualTo(ThreadSafe.class.getAnnotation(Documented.class));
860+
assertThat(findMergedAnnotation(ResourceHolder.class, TypeQualifierNickname.class))
861+
.isEqualTo(RegEx.class.getAnnotation(TypeQualifierNickname.class));
860862
}
861863

862864
@Test
@@ -1536,7 +1538,7 @@ static class SpringAppConfigClass {
15361538
}
15371539

15381540
@Resource(name = "x")
1539-
@ParametersAreNonnullByDefault
1541+
@RegEx
15401542
static class ResourceHolder {
15411543
}
15421544

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

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -19,9 +19,8 @@
1919
import java.lang.annotation.Retention;
2020
import java.lang.annotation.RetentionPolicy;
2121

22-
import javax.annotation.Nonnull;
22+
import javax.annotation.concurrent.ThreadSafe;
2323

24-
import org.jspecify.annotations.Nullable;
2524
import org.junit.jupiter.api.Test;
2625

2726
import org.springframework.lang.Contract;
@@ -86,12 +85,13 @@ void javaWhenJavaLangAnnotationReturnsTrue() {
8685

8786
@Test
8887
void javaWhenJavaxAnnotationReturnsTrue() {
89-
assertThat(AnnotationFilter.JAVA.matches(Nonnull.class)).isTrue();
88+
assertThat(AnnotationFilter.JAVA.matches(ThreadSafe.class)).isTrue();
9089
}
9190

9291
@Test
92+
@SuppressWarnings("deprecation")
9393
void javaWhenSpringLangAnnotationReturnsFalse() {
94-
assertThat(AnnotationFilter.JAVA.matches(Nullable.class)).isFalse();
94+
assertThat(AnnotationFilter.JAVA.matches(org.springframework.lang.Nullable.class)).isFalse();
9595
}
9696

9797
@Test
@@ -103,7 +103,7 @@ void javaWhenOtherAnnotationReturnsFalse() {
103103
@SuppressWarnings("deprecation")
104104
void noneReturnsFalse() {
105105
assertThat(AnnotationFilter.NONE.matches(Retention.class)).isFalse();
106-
assertThat(AnnotationFilter.NONE.matches(Nullable.class)).isFalse();
106+
assertThat(AnnotationFilter.NONE.matches(org.springframework.lang.Nullable.class)).isFalse();
107107
assertThat(AnnotationFilter.NONE.matches(TestAnnotation.class)).isFalse();
108108
}
109109

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

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -29,8 +29,6 @@
2929
import java.util.Map;
3030
import java.util.stream.IntStream;
3131

32-
import javax.annotation.Nullable;
33-
3432
import org.junit.jupiter.api.Test;
3533

3634
import org.springframework.core.annotation.AnnotationTypeMapping.MirrorSets;
@@ -442,7 +440,7 @@ private Method[] resolveMirrorSets(AnnotationTypeMapping mapping, Class<?> eleme
442440
return result;
443441
}
444442

445-
private @Nullable Method getAliasMapping(AnnotationTypeMapping mapping, int attributeIndex) {
443+
private Method getAliasMapping(AnnotationTypeMapping mapping, int attributeIndex) {
446444
int mapped = mapping.getAliasMapping(attributeIndex);
447445
return mapped != -1 ? mapping.getRoot().getAttributes().get(mapped) : null;
448446
}

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

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -30,8 +30,9 @@
3030
import java.util.NoSuchElementException;
3131
import java.util.Set;
3232

33-
import javax.annotation.Nonnull;
34-
import javax.annotation.ParametersAreNonnullByDefault;
33+
import javax.annotation.RegEx;
34+
import javax.annotation.Syntax;
35+
import javax.annotation.concurrent.ThreadSafe;
3536

3637
import org.junit.jupiter.api.BeforeEach;
3738
import org.junit.jupiter.api.Test;
@@ -428,8 +429,8 @@ void isAnnotationInheritedForAllScenarios() {
428429
@Test
429430
void isAnnotationMetaPresentForPlainType() {
430431
assertThat(isAnnotationMetaPresent(Order.class, Documented.class)).isTrue();
431-
assertThat(isAnnotationMetaPresent(ParametersAreNonnullByDefault.class, Documented.class)).isTrue();
432-
assertThat(isAnnotationMetaPresent(ParametersAreNonnullByDefault.class, Nonnull.class)).isTrue();
432+
assertThat(isAnnotationMetaPresent(ThreadSafe.class, Documented.class)).isTrue();
433+
assertThat(isAnnotationMetaPresent(RegEx.class, Syntax.class)).isTrue();
433434
}
434435

435436
@Test

‎src/checkstyle/checkstyle-suppressions.xml‎

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -21,9 +21,6 @@
2121
<!-- Framework docs -->
2222
<suppress files="org[\\/]springframework[\\/]docs[\\/].+" checks="IllegalImport" id="bannedJUnitJupiterImports" />
2323

24-
<!-- spring-aop -->
25-
<suppress files="[\\/]src[\\/]main[\\/]java[\\/]org[\\/]aopalliance[\\/]" checks="IllegalImport" id="bannedImports" message="javax"/>
26-
2724
<!-- spring-beans -->
2825
<suppress files="TypeMismatchException" checks="MutableException"/>
2926
<suppress files="BeanCreationException" checks="MutableException"/>
@@ -40,9 +37,7 @@
4037
<suppress files="[\\/]src[\\/]main[\\/]java[\\/]org[\\/]springframework[\\/](asm|cglib|objenesis|javapoet)[\\/]" checks=".*"/>
4138
<suppress files="[\\/]src[\\/]main[\\/]java[\\/]org[\\/]springframework[\\/]aot[\\/]nativex[\\/]feature[\\/]" checks="RegexpSinglelineJava" id="systemOutErrPrint"/>
4239
<suppress files="[\\/]src[\\/]main[\\/]java[\\/]org[\\/]springframework[\\/](core|util)[\\/](SpringProperties|SystemPropertyUtils)" checks="RegexpSinglelineJava" id="systemOutErrPrint"/>
43-
<suppress files="[\\/]src[\\/]main[\\/]java[\\/]org[\\/]springframework[\\/]lang[\\/]" checks="IllegalImport" id="bannedImports" message="javax"/>
44-
<suppress files="[\\/]src[\\/]main[\\/]java[\\/]org[\\/]springframework[\\/]core[\\/]annotation[\\/]" checks="IllegalImport" id="bannedImports" message="javax"/>
45-
<suppress files="[\\/]src[\\/]test[\\/]java[\\/]org[\\/]springframework[\\/]core[\\/]annotation[\\/]" checks="IllegalImport" id="bannedImports" message="javax"/>
40+
<suppress files="[\\/]src[\\/]main[\\/]java[\\/]org[\\/]springframework[\\/]lang[\\/]NonNull.+" checks="IllegalImport" id="bannedNullabilityImports"/>
4641
<suppress files="ByteArrayEncoder" checks="SpringLambda"/>
4742
<suppress files="SocketUtils" checks="HideUtilityClassConstructor"/>
4843
<suppress files="ResolvableType" checks="FinalClass"/>

‎src/checkstyle/checkstyle.xml‎

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -104,10 +104,20 @@
104104
<property name="sortStaticImportsAlphabetically" value="true"/>
105105
</module>
106106
<module name="com.puppycrawl.tools.checkstyle.checks.imports.IllegalImportCheck">
107-
<property name="id" value="bannedImports"/>
107+
<property name="id" value="bannedReactorImports"/>
108108
<property name="regexp" value="true"/>
109-
<property name="illegalClasses"
110-
value="^reactor\.core\.support\.Assert,^org\.slf4j\.LoggerFactory,^(?!org\.jspecify|\.annotations).*(NonNull|Nullable)$"/>
109+
<property name="illegalClasses" value="^reactor\.core\.support\.Assert"/>
110+
</module>
111+
<module name="com.puppycrawl.tools.checkstyle.checks.imports.IllegalImportCheck">
112+
<property name="id" value="bannedSlf4jImports"/>
113+
<property name="regexp" value="true"/>
114+
<property name="illegalClasses" value="^org\.slf4j\.LoggerFactory"/>
115+
</module>
116+
<module name="com.puppycrawl.tools.checkstyle.checks.imports.IllegalImportCheck">
117+
<property name="id" value="bannedNullabilityImports"/>
118+
<property name="regexp" value="true"/>
119+
<!-- Rejects all NonNull, Nonnull, and Nullable types that are NOT in the org.jspecify.annotations package. -->
120+
<property name="illegalClasses" value="^(?!org\.jspecify\.annotations).*(Non[Nn]ull|Nullable)$"/>
111121
</module>
112122
<module name="com.puppycrawl.tools.checkstyle.checks.imports.IllegalImportCheck">
113123
<property name="id" value="bannedJUnit3Imports"/>

0 commit comments

Comments
 (0)