From 6c9ab11ebc5606607db20368025d031a055e0727 Mon Sep 17 00:00:00 2001 From: Ulli Hafner Date: Fri, 14 Apr 2023 14:39:30 +0200 Subject: [PATCH 1/2] Fix architecture test for `readResolve` for final classes. --- .../edu/hm/hafner/util/ArchitectureRules.java | 41 ++++++++++-- .../hm/hafner/util/ArchitectureRulesTest.java | 65 ++++++++++++++++++- 2 files changed, 97 insertions(+), 9 deletions(-) diff --git a/src/test/java/edu/hm/hafner/util/ArchitectureRules.java b/src/test/java/edu/hm/hafner/util/ArchitectureRules.java index e2e6b03b..b11f404a 100644 --- a/src/test/java/edu/hm/hafner/util/ArchitectureRules.java +++ b/src/test/java/edu/hm/hafner/util/ArchitectureRules.java @@ -11,10 +11,14 @@ import com.tngtech.archunit.core.domain.JavaCall; import com.tngtech.archunit.core.domain.JavaClass; import com.tngtech.archunit.core.domain.JavaConstructorCall; +import com.tngtech.archunit.core.domain.JavaMethod; import com.tngtech.archunit.core.domain.JavaModifier; import com.tngtech.archunit.core.domain.properties.CanBeAnnotated; import com.tngtech.archunit.junit.ArchTest; +import com.tngtech.archunit.lang.ArchCondition; import com.tngtech.archunit.lang.ArchRule; +import com.tngtech.archunit.lang.ConditionEvents; +import com.tngtech.archunit.lang.SimpleConditionEvent; import static com.tngtech.archunit.core.domain.JavaAccess.Predicates.*; import static com.tngtech.archunit.lang.conditions.ArchConditions.*; @@ -107,7 +111,7 @@ public final class ArchitectureRules { public static final ArchRule READ_RESOLVE_SHOULD_BE_PROTECTED = methods().that().haveName("readResolve").and().haveRawReturnType(Object.class) .should().beDeclaredInClassesThat().implement(Serializable.class) - .andShould().beProtected().allowEmptyShould(true); + .andShould(beProtected()).allowEmptyShould(true); private static ExceptionHasNoContext exceptionHasNoContextAsParameter() { return new ExceptionHasNoContext(); @@ -121,6 +125,13 @@ private static DescribedPredicate> accessIsRestrictedForTest return new AccessRestrictedToTests(); } + /** + * Predicate to match exception constructor calls without contexts. + */ + private static ArchCondition beProtected() { + return new ShouldBeProtectedCondition(); + } + /** * Matches if a call from outside the defining class uses a method or constructor annotated with * {@link VisibleForTesting}. There are two exceptions: @@ -130,6 +141,7 @@ private static DescribedPredicate> accessIsRestrictedForTest * */ private static class AccessRestrictedToTests extends DescribedPredicate> { + AccessRestrictedToTests() { super("access is restricted to tests"); } @@ -140,16 +152,13 @@ public boolean test(final JavaCall input) { && !input.getOriginOwner().equals(input.getTargetOwner()) && !isVisibleForTesting(input.getOrigin()); } - private boolean isVisibleForTesting(final CanBeAnnotated target) { return target.isAnnotatedWith(VisibleForTesting.class); } - } - /** - * Predicate to match exception constructor calls without contexts. - */ + } public static class ExceptionHasNoContext extends DescribedPredicate { + private final List> allowedExceptions; /** @@ -178,5 +187,25 @@ public boolean test(final JavaConstructorCall javaConstructorCall) { private boolean isPermittedException(final JavaClass owner) { return allowedExceptions.stream().anyMatch(owner::isAssignableTo); } + + } + + private static class ShouldBeProtectedCondition extends ArchCondition { + ShouldBeProtectedCondition() { + super("should be protected"); + } + + @Override + public void check(final JavaMethod method, final ConditionEvents events) { + if (method.getModifiers().contains(JavaModifier.PROTECTED)) { + return; + } + if (method.getOwner().getModifiers().contains(JavaModifier.FINAL)) { + return; + } + events.add(SimpleConditionEvent.violated(method, + String.format("%s is not protected but the class might be extended in %s", + method.getDescription(), method.getSourceCodeLocation()))); + } } } diff --git a/src/test/java/edu/hm/hafner/util/ArchitectureRulesTest.java b/src/test/java/edu/hm/hafner/util/ArchitectureRulesTest.java index 2543dee7..b9d92ff2 100644 --- a/src/test/java/edu/hm/hafner/util/ArchitectureRulesTest.java +++ b/src/test/java/edu/hm/hafner/util/ArchitectureRulesTest.java @@ -1,5 +1,7 @@ package edu.hm.hafner.util; +import java.io.Serializable; + import org.junit.jupiter.api.Test; import com.tngtech.archunit.core.domain.JavaClasses; @@ -15,6 +17,19 @@ class ArchitectureRulesTest { private static final String BROKEN_CLASS_NAME = ArchitectureRulesViolatedTest.class.getTypeName(); + @Test + void shouldUseProtectedForReadResolve() { + assertThatExceptionOfType(AssertionError.class).isThrownBy( + () -> ArchitectureRules.READ_RESOLVE_SHOULD_BE_PROTECTED.check(importBrokenClass())) + .withMessageContainingAll(BROKEN_CLASS_NAME, "was violated (3 times)", + "Method is not protected but the class might be extended in (ArchitectureRulesTest.java:", + "Method is not declared in classes that implement java.io.Serializable in (ArchitectureRulesTest.java:", + "Method is not protected but the class might be extended in (ArchitectureRulesTest.java:"); + + assertThatNoException().isThrownBy( + () -> ArchitectureRules.READ_RESOLVE_SHOULD_BE_PROTECTED.check(importPassingClass())); + } + @Test void shouldNotUseJsr305Annotations() { assertThatExceptionOfType(AssertionError.class).isThrownBy( @@ -81,11 +96,13 @@ void shouldVerifyNoPublicTestMethodsRule() { } private JavaClasses importPassingClass() { - return new ClassFileImporter().importClasses(ArchitectureRulesPassedTest.class); + return new ClassFileImporter().importClasses(ArchitectureRulesPassedTest.class, + ArchitectureRulesAlsoPassedTest.class); } private JavaClasses importBrokenClass() { - return importClasses(ArchitectureRulesViolatedTest.class); + return importClasses(ArchitectureRulesViolatedTest.class, + ArchitectureRulesAlsoViolatedTest.class); } private JavaClasses importClasses(final Class... classes) { @@ -110,13 +127,55 @@ public void shouldFail() { protected String method(@javax.annotation.Nonnull String param) { return null; } + + /** + * Called after de-serialization to retain backward compatibility. + * + * @return this + */ + private Object readResolve() { + return this; + } + } + + @SuppressWarnings("all") @Generated // This class is just there to be used in architecture tests + public static class ArchitectureRulesAlsoViolatedTest implements Serializable { + /** + * Called after de-serialization to retain backward compatibility. + * + * @return this + */ + private Object readResolve() { + return this; + } } @SuppressWarnings("all") // This class is just there to be used in architecture tests - static class ArchitectureRulesPassedTest { + static final class ArchitectureRulesPassedTest implements Serializable { @Test void shouldPass() { throw new IllegalArgumentException("context"); } + + /** + * Called after de-serialization to retain backward compatibility. + * + * @return this + */ + private Object readResolve() { + return this; + } + } + + @SuppressWarnings("all") // This class is just there to be used in architecture tests + static class ArchitectureRulesAlsoPassedTest implements Serializable { + /** + * Called after de-serialization to retain backward compatibility. + * + * @return this + */ + protected Object readResolve() { + return this; + } } } From 6f96f86b5f3471c82bf3250aebc088cd624e9ac0 Mon Sep 17 00:00:00 2001 From: Ulli Hafner Date: Sat, 15 Apr 2023 11:44:45 +0200 Subject: [PATCH 2/2] Fix styling. --- .../edu/hm/hafner/util/ArchitectureRules.java | 46 +++++++++++++------ 1 file changed, 31 insertions(+), 15 deletions(-) diff --git a/src/test/java/edu/hm/hafner/util/ArchitectureRules.java b/src/test/java/edu/hm/hafner/util/ArchitectureRules.java index b11f404a..71011f20 100644 --- a/src/test/java/edu/hm/hafner/util/ArchitectureRules.java +++ b/src/test/java/edu/hm/hafner/util/ArchitectureRules.java @@ -76,7 +76,9 @@ public final class ArchitectureRules { public static final ArchRule NO_TEST_API_CALLED = noClasses().that().haveSimpleNameNotEndingWith("Test") .and().haveSimpleNameNotContaining("Benchmark") - .should().callCodeUnitWhere(accessIsRestrictedForTests()); + .should().callCodeUnitWhere(accessIsRestrictedForTests()) + .because("Production code should never access methods that are marked with @VisibleForTesting") + .allowEmptyShould(true); /** Prevents that classes use visible but forbidden API. */ public static final ArchRule NO_FORBIDDEN_PACKAGE_ACCESSED = @@ -93,18 +95,29 @@ public final class ArchitectureRules { /** Prevents that classes use visible but forbidden annotations. */ public static final ArchRule NO_FORBIDDEN_ANNOTATION_USED = - noClasses().should().dependOnClassesThat().haveNameMatching("javax.annotation.Check.*") - .orShould().dependOnClassesThat().haveNameMatching("javax.annotation.Nonnull") - .orShould().dependOnClassesThat().haveNameMatching("javax.annotation.Nullable") - .orShould().dependOnClassesThat().haveNameMatching("javax.annotation.Parameters.*") - .orShould().dependOnClassesThat().haveNameMatching("edu.umd.cs.findbugs.annotations.Nullable") // only CheckForNull and NonNull is allowed + noClasses().should() + .dependOnClassesThat() + .haveNameMatching("javax.annotation.Check.*") + .orShould() + .dependOnClassesThat() + .haveNameMatching("javax.annotation.Nonnull") + .orShould() + .dependOnClassesThat() + .haveNameMatching("javax.annotation.Nullable") + .orShould() + .dependOnClassesThat() + .haveNameMatching("javax.annotation.Parameters.*") + .orShould() + .dependOnClassesThat() + .haveNameMatching( + "edu.umd.cs.findbugs.annotations.Nullable") // only CheckForNull and NonNull is allowed .because("JSR 305 annotations are now part of edu.umd.cs.findbugs.annotations package"); /** Prevents that classes use visible but forbidden API. */ public static final ArchRule NO_FORBIDDEN_CLASSES_CALLED = noClasses().should().callCodeUnitWhere(targetOwner(has( - fullyQualifiedName("org.junit.jupiter.api.Assertions") - .or(fullyQualifiedName("org.junit.Assert"))))) + fullyQualifiedName("org.junit.jupiter.api.Assertions") + .or(fullyQualifiedName("org.junit.Assert"))))) .because("only AssertJ should be used for assertions"); /** Ensures that the {@code readResolve} methods are protected so subclasses can call the parent method. */ @@ -117,14 +130,14 @@ private static ExceptionHasNoContext exceptionHasNoContextAsParameter() { return new ExceptionHasNoContext(); } - private ArchitectureRules() { - // prevents instantiation - } - private static DescribedPredicate> accessIsRestrictedForTests() { return new AccessRestrictedToTests(); } + private ArchitectureRules() { + // prevents instantiation + } + /** * Predicate to match exception constructor calls without contexts. */ @@ -141,7 +154,6 @@ private static ArchCondition beProtected() { * */ private static class AccessRestrictedToTests extends DescribedPredicate> { - AccessRestrictedToTests() { super("access is restricted to tests"); } @@ -152,13 +164,17 @@ public boolean test(final JavaCall input) { && !input.getOriginOwner().equals(input.getTargetOwner()) && !isVisibleForTesting(input.getOrigin()); } + private boolean isVisibleForTesting(final CanBeAnnotated target) { return target.isAnnotatedWith(VisibleForTesting.class); } } - public static class ExceptionHasNoContext extends DescribedPredicate { + /** + * Matches if an exception has no context, i.e., the constructor is invoked without a message. + */ + private static class ExceptionHasNoContext extends DescribedPredicate { private final List> allowedExceptions; /** @@ -168,7 +184,7 @@ public static class ExceptionHasNoContext extends DescribedPredicate... allowedExceptions) { + ExceptionHasNoContext(final Class... allowedExceptions) { super("exception context is missing"); this.allowedExceptions = List.of(allowedExceptions);