Skip to content

Commit

Permalink
Merge pull request #743 from uhafner/read-resolve
Browse files Browse the repository at this point in the history 
Fix architecture test for `readResolve` for final classes
  • Loading branch information
@uhafner
uhafner committed Apr 15, 2023
2 parents be9cf4e + 6f96f86 commit 36dfbb1
Show file tree
Hide file tree
Showing 2 changed files with 121 additions and 17 deletions.
73 changes: 59 additions & 14 deletions src/test/java/edu/hm/hafner/util/ArchitectureRules.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.*;
Expand Down Expand Up @@ -72,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 =
Expand All @@ -89,36 +95,54 @@ 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. */
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();
}

private static DescribedPredicate<? super JavaCall<?>> accessIsRestrictedForTests() {
return new AccessRestrictedToTests();
}

private ArchitectureRules() {
// prevents instantiation
}

private static DescribedPredicate<? super JavaCall<?>> accessIsRestrictedForTests() {
return new AccessRestrictedToTests();
/**
* Predicate to match exception constructor calls without contexts.
*/
private static ArchCondition<JavaMethod> beProtected() {
return new ShouldBeProtectedCondition();
}

/**
Expand All @@ -144,12 +168,13 @@ public boolean test(final JavaCall<?> input) {
private boolean isVisibleForTesting(final CanBeAnnotated target) {
return target.isAnnotatedWith(VisibleForTesting.class);
}

}

/**
* Predicate to match exception constructor calls without contexts.
* Matches if an exception has no context, i.e., the constructor is invoked without a message.
*/
public static class ExceptionHasNoContext extends DescribedPredicate<JavaConstructorCall> {
private static class ExceptionHasNoContext extends DescribedPredicate<JavaConstructorCall> {
private final List<Class<? extends Throwable>> allowedExceptions;

/**
Expand All @@ -159,7 +184,7 @@ public static class ExceptionHasNoContext extends DescribedPredicate<JavaConstru
* exceptions that are allowed to be instantiated without arguments
*/
@SafeVarargs
public ExceptionHasNoContext(final Class<? extends Throwable>... allowedExceptions) {
ExceptionHasNoContext(final Class<? extends Throwable>... allowedExceptions) {
super("exception context is missing");

this.allowedExceptions = List.of(allowedExceptions);
Expand All @@ -178,5 +203,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<JavaMethod> {
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())));
}
}
}
65 changes: 62 additions & 3 deletions src/test/java/edu/hm/hafner/util/ArchitectureRulesTest.java
View file
Original file line number Diff line number Diff line change
@@ -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;
Expand All @@ -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 <edu.hm.hafner.util.ArchitectureRulesTest$ArchitectureRulesAlsoViolatedTest.readResolve()> is not protected but the class might be extended in (ArchitectureRulesTest.java:",
"Method <edu.hm.hafner.util.ArchitectureRulesTest$ArchitectureRulesViolatedTest.readResolve()> is not declared in classes that implement java.io.Serializable in (ArchitectureRulesTest.java:",
"Method <edu.hm.hafner.util.ArchitectureRulesTest$ArchitectureRulesViolatedTest.readResolve()> 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(
Expand Down Expand Up @@ -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) {
Expand All @@ -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;
}
}
}

0 comments on commit 36dfbb1

Please sign in to comment.