From 3391ee075ee38ed00f4379ae75eb73d53e16479e Mon Sep 17 00:00:00 2001 From: Lucas Welscher Date: Thu, 13 Jul 2023 12:16:01 +0200 Subject: [PATCH] Development: Enforce consistent usage of nullness annotations (#6872) --- .../artemis/repository/RepositoryImpl.java | 2 +- .../localvc/LocalVCFetchFilter.java | 2 +- .../connectors/localvc/LocalVCPushFilter.java | 2 +- .../GeneralInstantNotificationService.java | 3 +- .../tum/in/www1/artemis/ArchitectureTest.java | 52 +++++++++++++++++-- .../metis/AbstractConversationTest.java | 6 +-- .../artemis/metis/PostIntegrationTest.java | 2 +- 7 files changed, 55 insertions(+), 14 deletions(-) diff --git a/src/main/java/de/tum/in/www1/artemis/repository/RepositoryImpl.java b/src/main/java/de/tum/in/www1/artemis/repository/RepositoryImpl.java index 61ceb4fab43c..ca53cd33bd8f 100644 --- a/src/main/java/de/tum/in/www1/artemis/repository/RepositoryImpl.java +++ b/src/main/java/de/tum/in/www1/artemis/repository/RepositoryImpl.java @@ -3,6 +3,7 @@ import java.io.Serializable; import java.util.Collections; +import javax.annotation.Nullable; import javax.persistence.EntityManager; import javax.persistence.TypedQuery; import javax.persistence.criteria.*; @@ -11,7 +12,6 @@ import org.springframework.data.jpa.repository.support.JpaEntityInformation; import org.springframework.data.jpa.repository.support.JpaEntityInformationSupport; import org.springframework.data.jpa.repository.support.SimpleJpaRepository; -import org.springframework.lang.Nullable; import org.springframework.util.Assert; public class RepositoryImpl extends SimpleJpaRepository { diff --git a/src/main/java/de/tum/in/www1/artemis/service/connectors/localvc/LocalVCFetchFilter.java b/src/main/java/de/tum/in/www1/artemis/service/connectors/localvc/LocalVCFetchFilter.java index c02e3d9cfe1f..b6306a4f7e5a 100644 --- a/src/main/java/de/tum/in/www1/artemis/service/connectors/localvc/LocalVCFetchFilter.java +++ b/src/main/java/de/tum/in/www1/artemis/service/connectors/localvc/LocalVCFetchFilter.java @@ -6,8 +6,8 @@ import javax.servlet.ServletException; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; +import javax.validation.constraints.NotNull; -import org.jetbrains.annotations.NotNull; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.springframework.web.filter.OncePerRequestFilter; diff --git a/src/main/java/de/tum/in/www1/artemis/service/connectors/localvc/LocalVCPushFilter.java b/src/main/java/de/tum/in/www1/artemis/service/connectors/localvc/LocalVCPushFilter.java index 9ebafdcc189e..fcf279313573 100644 --- a/src/main/java/de/tum/in/www1/artemis/service/connectors/localvc/LocalVCPushFilter.java +++ b/src/main/java/de/tum/in/www1/artemis/service/connectors/localvc/LocalVCPushFilter.java @@ -6,8 +6,8 @@ import javax.servlet.ServletException; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; +import javax.validation.constraints.NotNull; -import org.jetbrains.annotations.NotNull; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.springframework.web.filter.OncePerRequestFilter; diff --git a/src/main/java/de/tum/in/www1/artemis/service/notifications/GeneralInstantNotificationService.java b/src/main/java/de/tum/in/www1/artemis/service/notifications/GeneralInstantNotificationService.java index 895fe93d0318..37e03dfaf4bf 100644 --- a/src/main/java/de/tum/in/www1/artemis/service/notifications/GeneralInstantNotificationService.java +++ b/src/main/java/de/tum/in/www1/artemis/service/notifications/GeneralInstantNotificationService.java @@ -5,7 +5,8 @@ import java.util.List; -import org.jetbrains.annotations.NotNull; +import javax.validation.constraints.NotNull; + import org.springframework.scheduling.annotation.Async; import org.springframework.stereotype.Service; diff --git a/src/test/java/de/tum/in/www1/artemis/ArchitectureTest.java b/src/test/java/de/tum/in/www1/artemis/ArchitectureTest.java index 612340123138..56d8452c0622 100644 --- a/src/test/java/de/tum/in/www1/artemis/ArchitectureTest.java +++ b/src/test/java/de/tum/in/www1/artemis/ArchitectureTest.java @@ -1,17 +1,21 @@ package de.tum.in.www1.artemis; -import static com.tngtech.archunit.base.DescribedPredicate.not; +import static com.tngtech.archunit.base.DescribedPredicate.*; import static com.tngtech.archunit.core.domain.JavaClass.Predicates.*; -import static com.tngtech.archunit.lang.conditions.ArchPredicates.are; -import static com.tngtech.archunit.lang.conditions.ArchPredicates.have; +import static com.tngtech.archunit.lang.SimpleConditionEvent.violated; +import static com.tngtech.archunit.lang.conditions.ArchPredicates.*; import static com.tngtech.archunit.lang.syntax.ArchRuleDefinition.*; +import java.util.*; import java.util.stream.Collectors; import org.junit.jupiter.api.*; import org.junit.jupiter.params.ParameterizedTest; -import com.tngtech.archunit.lang.ArchRule; +import com.tngtech.archunit.base.DescribedPredicate; +import com.tngtech.archunit.core.domain.JavaAnnotation; +import com.tngtech.archunit.core.domain.JavaMethod; +import com.tngtech.archunit.lang.*; class ArchitectureTest extends AbstractArchitectureTest { @@ -52,4 +56,44 @@ void testNoCollectorsToList() { .because("You should use .toList() or .collect(Collectors.toCollection(ArrayList::new)) instead"); toListUsage.check(allClasses); } + + @Test + void testNullnessAnnotations() { + var notNullPredicate = and(not(resideInPackageAnnotation("javax.validation.constraints")), simpleNameAnnotation("NotNull")); + var nonNullPredicate = simpleNameAnnotation("NonNull"); + var nullablePredicate = and(not(resideInPackageAnnotation("javax.annotation")), simpleNameAnnotation("Nullable")); + + Set>> allPredicates = Set.of(notNullPredicate, nonNullPredicate, nullablePredicate); + + for (var predicate : allPredicates) { + ArchRule units = noCodeUnits().should().beAnnotatedWith(predicate); + ArchRule parameters = methods().should(notHaveAnyParameterAnnotatedWith(predicate)); + + units.check(allClasses); + parameters.check(allClasses); + } + } + + // Custom Predicates for JavaAnnotations since ArchUnit only defines them for classes + + private DescribedPredicate> simpleNameAnnotation(String name) { + return equalTo(name).as("Annotation with simple name " + name).onResultOf(annotation -> annotation.getRawType().getSimpleName()); + } + + private DescribedPredicate> resideInPackageAnnotation(String packageName) { + return equalTo(packageName).as("Annotation in package " + packageName).onResultOf(annotation -> annotation.getRawType().getPackageName()); + } + + private ArchCondition notHaveAnyParameterAnnotatedWith(DescribedPredicate> annotationPredicate) { + return new ArchCondition<>("have parameters annotated with ") { + + @Override + public void check(JavaMethod item, ConditionEvents events) { + boolean satisfied = item.getParameterAnnotations().stream().flatMap(Collection::stream).noneMatch(annotationPredicate); + if (!satisfied) { + events.add(violated(item, String.format("Method %s has parameter violating %s", item.getFullName(), annotationPredicate.getDescription()))); + } + } + }; + } } diff --git a/src/test/java/de/tum/in/www1/artemis/metis/AbstractConversationTest.java b/src/test/java/de/tum/in/www1/artemis/metis/AbstractConversationTest.java index 65ed87dc4650..e36dc6eed8aa 100644 --- a/src/test/java/de/tum/in/www1/artemis/metis/AbstractConversationTest.java +++ b/src/test/java/de/tum/in/www1/artemis/metis/AbstractConversationTest.java @@ -7,7 +7,6 @@ import java.util.Arrays; import java.util.Set; -import org.jetbrains.annotations.NotNull; import org.junit.jupiter.api.BeforeEach; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.data.domain.Pageable; @@ -16,7 +15,6 @@ import de.tum.in.www1.artemis.AbstractSpringIntegrationBambooBitbucketJiraTest; import de.tum.in.www1.artemis.course.CourseUtilService; -import de.tum.in.www1.artemis.domain.Course; import de.tum.in.www1.artemis.domain.User; import de.tum.in.www1.artemis.domain.enumeration.CourseInformationSharingConfiguration; import de.tum.in.www1.artemis.domain.enumeration.DisplayPriority; @@ -169,13 +167,11 @@ void removeUsersFromConversation(Long conversationId, String... userLoginsWithou } } - @NotNull - Course setCourseInformationSharingConfiguration(CourseInformationSharingConfiguration courseInformationSharingConfiguration) { + void setCourseInformationSharingConfiguration(CourseInformationSharingConfiguration courseInformationSharingConfiguration) { var persistedCourse = courseRepository.findByIdElseThrow(exampleCourseId); persistedCourse.setCourseInformationSharingConfiguration(courseInformationSharingConfiguration); persistedCourse = courseRepository.saveAndFlush(persistedCourse); assertThat(persistedCourse.getCourseInformationSharingConfiguration()).isEqualTo(courseInformationSharingConfiguration); - return persistedCourse; } ChannelDTO createChannel(boolean isPublicChannel, String name) throws Exception { diff --git a/src/test/java/de/tum/in/www1/artemis/metis/PostIntegrationTest.java b/src/test/java/de/tum/in/www1/artemis/metis/PostIntegrationTest.java index f4348527a9ac..66a235fecb65 100644 --- a/src/test/java/de/tum/in/www1/artemis/metis/PostIntegrationTest.java +++ b/src/test/java/de/tum/in/www1/artemis/metis/PostIntegrationTest.java @@ -11,8 +11,8 @@ import javax.validation.Validation; import javax.validation.Validator; import javax.validation.ValidatorFactory; +import javax.validation.constraints.NotNull; -import org.jetbrains.annotations.NotNull; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test;