Merge pull request #563 from uhafner/arch-documentation

Improve detection of JSR 305 annotations

src/main/java/edu/hm/hafner/util/Ensure.java

@@ -15,24 +15,26 @@
 /**
  * Provides several helper methods to validate method arguments and class invariants thus supporting the design by
  * contract concept (DBC).
+ *
  * <p>
- *     Note: the static methods provided by this class use a fluent interface, i.e., in order to
- * verify an assertion a method sequence needs to be called.
+ * Note: the static methods provided by this class use a fluent interface, i.e., in order to verify an assertion a
+ * method sequence needs to be called.
  * </p>
- *
- * Available checks:
- *     <ul>
- *         <li>Boolean assertions, e.g., {@code Ensure.that(condition).isTrue(); } </li>
- *         <li>String assertions, e.g., {@code Ensure.that(string).isNotEmpty(); } </li>
- *         <li>Object assertions, e.g., {@code Ensure.that(element).isNotNull(); } </li>
- *         <li>Array assertions, e.g., {@code Ensure.that(array).isNotEmpty(); } </li>
- *         <li>Iterable assertions, e.g., {@code Ensure.that(collection).isNotNull(); } </li>
- *         </ul>
+ * <p>
+ *  Available checks:
+ * </p>
+ * <ul>
+ *      <li>Boolean assertions, e.g., {@code Ensure.that(condition).isTrue(); } </li>
+ *      <li>String assertions, e.g., {@code Ensure.that(string).isNotEmpty(); } </li>
+ *      <li>Object assertions, e.g., {@code Ensure.that(element).isNotNull(); } </li>
+ *      <li>Array assertions, e.g., {@code Ensure.that(array).isNotEmpty(); } </li>
+ *      <li>Iterable assertions, e.g., {@code Ensure.that(collection).isNotNull(); } </li>
+ * </ul>
  *
  * @author Ullrich Hafner
  * @see <a href="http://se.ethz.ch/~meyer/publications/computer/contract.pdf"> Design by Contract (Meyer, Bertrand)</a>
  */
-@SuppressWarnings({"NonBooleanMethodNameMayNotStartWithQuestion", "ConstantConditions", "CyclicClassDependency", "NullAway"})
+@SuppressWarnings({"NonBooleanMethodNameMayNotStartWithQuestion", "CyclicClassDependency", "NullAway"})
 public final class Ensure {
     /**
      * Returns a boolean condition.
@@ -55,9 +57,10 @@ public static BooleanCondition that(final boolean value) {
      *         the value to check
      * @param additionalValues
      *         the additional values to check
+     * @param <T>
+     *         type to check
      *
      * @return an object condition
-     * @param <T> type to check
      */
     @CheckReturnValue
     public static <T> ObjectCondition<T> that(@CheckForNull final T value,
@@ -494,7 +497,8 @@ private boolean isBlank() {
     /**
      * Assertions for objects.
      *
-     * @param <T> type to check
+     * @param <T>
+     *         type to check
      */
     public static class ObjectCondition<T> {
         @CheckForNull

src/main/java/edu/hm/hafner/util/ResourceExtractor.java

@@ -29,7 +29,7 @@
 public class ResourceExtractor {
     private final boolean readingFromJarFile;
     private final Extractor extractor;
-    private String resourcePath;
+    private final String resourcePath;
 
     /**
      * Creates a new {@link ResourceExtractor} that extracts resources from the classloader of the specified class.
@@ -160,14 +160,7 @@ public void extractFiles(final Path targetDirectory, final String... sources) {
                     JarEntry entry = entries.nextElement();
                     String name = entry.getName();
                     if (remaining.contains(name)) {
-                        Path targetFile = targetDirectory.resolve(name);
-                        if (!targetFile.normalize().startsWith(targetDirectory)) {
-                            throw new IllegalArgumentException("Corrupt jar structure, contains invalid path: " + name);
-                        }
-                        Files.createDirectories(targetFile.getParent());
-                        try (InputStream inputStream = jar.getInputStream(entry); OutputStream outputStream = Files.newOutputStream(targetFile)) {
-                            IOUtils.copy(inputStream, outputStream);
-                        }
+                        copy(targetDirectory, jar, entry, name);
                         remaining.remove(name);
                     }
                 }
@@ -179,5 +172,20 @@ public void extractFiles(final Path targetDirectory, final String... sources) {
                 throw new NoSuchElementException("The following files have not been found: " + remaining);
             }
         }
+
+        private void copy(final Path targetDirectory, final JarFile jar, final JarEntry entry, final String name)
+                throws IOException {
+            Path targetFile = targetDirectory.resolve(name);
+            if (!targetFile.normalize().startsWith(targetDirectory)) {
+                throw new IllegalArgumentException("Corrupt jar structure, contains invalid path: " + name);
+            }
+            Path parent = targetFile.getParent();
+            if (parent != null) {
+                Files.createDirectories(parent);
+            }
+            try (InputStream inputStream = jar.getInputStream(entry); OutputStream outputStream = Files.newOutputStream(targetFile)) {
+                IOUtils.copy(inputStream, outputStream);
+            }
+        }
     }
 }

src/main/java/edu/hm/hafner/util/TreeString.java

@@ -10,8 +10,10 @@
 /**
  * {@link TreeString} is an alternative string representation that saves the memory when you have a large number of
  * strings that share common prefixes (such as various file names.)
+ *
  * <p>
  * {@link TreeString} can be built with {@link TreeStringBuilder}.
+ * </p>
  *
  * @author Kohsuke Kawaguchi
  */
@@ -55,8 +57,10 @@ String getLabel() {
 
     /**
      * Inserts a new node between this node and its parent, and returns the newly inserted node.
+     *
      * <p>
      * This operation doesn't change the string representation of this node.
+     * </p>
      *
      * @param prefix
      *         the prefix to remove

src/main/java/edu/hm/hafner/util/TreeStringBuilder.java

@@ -8,11 +8,13 @@
  * Builds {@link TreeString}s that share common prefixes. Call {@link #intern(String)} and you get the {@link
  * TreeString} that represents the same string, but as you interns more strings that share the same prefixes, those
  * {@link TreeString}s that you get back start to share data.
+ *
  * <p>
  * Because the internal state of {@link TreeString}s get mutated as new strings are interned (to exploit new-found
  * common prefixes), {@link TreeString}s returned from {@link #intern(String)} aren't thread-safe until {@link
  * TreeStringBuilder} is disposed. That is, you have to make sure other threads don't see those {@link TreeString}s
  * until you are done interning strings.
+ * </p>
  *
  * @author Kohsuke Kawaguchi
  */

src/test/java/edu/hm/hafner/util/ArchitectureRules.java

@@ -4,7 +4,6 @@
 import java.util.Arrays;
 import java.util.List;
 
-import org.apache.commons.lang3.StringUtils;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.params.ParameterizedTest;
 
@@ -18,7 +17,9 @@
 import com.tngtech.archunit.junit.ArchTest;
 import com.tngtech.archunit.lang.ArchRule;
 
-import static com.tngtech.archunit.core.domain.JavaClass.Predicates.*;
+import static com.tngtech.archunit.core.domain.JavaAccess.Predicates.*;
+import static com.tngtech.archunit.lang.conditions.ArchConditions.*;
+import static com.tngtech.archunit.lang.conditions.ArchPredicates.*;
 import static com.tngtech.archunit.lang.syntax.ArchRuleDefinition.*;
 
 /**
@@ -30,71 +31,87 @@ public final class ArchitectureRules {
     /** Never create exception without any context. */
     public static final ArchRule NO_EXCEPTIONS_WITH_NO_ARG_CONSTRUCTOR =
             noClasses().that().haveSimpleNameNotContaining("Benchmark")
-                    .should().callConstructorWhere(new ExceptionHasNoContext());
+                    .should().callConstructorWhere(exceptionHasNoContextAsParameter())
+                    .because("exceptions should include failure-capture information in detail messages (Effective Java Item 75)");
 
     /** Junit 5 test classes should not be public. */
     public static final ArchRule NO_PUBLIC_TEST_CLASSES =
             noClasses().that().haveSimpleNameEndingWith("Test")
                     .and().haveSimpleNameNotContaining("_jmh")
                     .and().doNotHaveModifier(JavaModifier.ABSTRACT)
-                    .should().bePublic();
+                    .should().bePublic()
+                    .because("test classes are not part of the API and should be hidden in a package");
 
     /** Junit 5 test methods should not be public. */
     public static final ArchRule ONLY_PACKAGE_PRIVATE_TEST_METHODS =
             methods().that().areAnnotatedWith(Test.class)
                     .or().areAnnotatedWith(ParameterizedTest.class)
                     .and().areDeclaredInClassesThat()
                     .haveSimpleNameEndingWith("Test")
-                    .should().bePackagePrivate();
+                    .should().bePackagePrivate()
+                    .because("test methods are not part of the API and should be hidden in a package");
 
     /** ArchUnit tests should not be public. */
-    public static final ArchRule NO_PUBLIC_ARCHITECTURE_TESTS =
+    public static final ArchRule ONLY_PACKAGE_PRIVATE_ARCHITECTURE_TESTS =
             fields().that().areAnnotatedWith(ArchTest.class)
-                    .should().notBePublic();
+                    .should().bePackagePrivate();
 
     /**
      * Methods or constructors that are annotated with {@link VisibleForTesting} must not be called by other classes.
      * These methods are meant to be {@code private}. Only test classes are allowed to call these methods.
      */
     public static final ArchRule NO_TEST_API_CALLED =
-            noClasses().that()
-                    .haveSimpleNameNotEndingWith("Test").and().haveSimpleNameNotContaining("Benchmark")
-                    .should().callCodeUnitWhere(new AccessRestrictedToTests());
+            noClasses().that().haveSimpleNameNotEndingWith("Test")
+                    .and().haveSimpleNameNotContaining("Benchmark")
+                    .should().callCodeUnitWhere(accessIsRestrictedForTests());
 
     /** Prevents that classes use visible but forbidden API. */
     public static final ArchRule NO_FORBIDDEN_PACKAGE_ACCESSED =
-            noClasses().should().dependOnClassesThat(resideInAnyPackage(
+            noClasses().should().dependOnClassesThat().resideInAnyPackage(
                     "org.apache.commons.lang..",
                     "org.joda.time..",
                     "javax.xml.bind..",
                     "net.jcip.annotations..",
-                    "javax.annotation..",
                     "junit..",
                     "org.hamcrest..",
                     "com.google.common..",
                     "org.junit"
-            ));
+            );
 
-    /** Prevents that classes use visible but forbidden API. */
+    /** Prevents that classes use visible but forbidden annotations. */
     public static final ArchRule NO_FORBIDDEN_ANNOTATION_USED =
-            noClasses().should().dependOnClassesThat().haveSimpleNameEndingWith("Nullable");
+            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(new TargetIsForbiddenClass(
-                    "org.junit.jupiter.api.Assertions", "org.junit.Assert"));
+    public static final ArchRule NO_FORBIDDEN_CLASSES_CALLED =
+            noClasses().should().callCodeUnitWhere(targetOwner(has(
+                    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);
 
+    private static ExceptionHasNoContext exceptionHasNoContextAsParameter() {
+        return new ExceptionHasNoContext();
+    }
+
     private ArchitectureRules() {
         // prevents instantiation
     }
 
+    private static DescribedPredicate<? super JavaCall<?>> accessIsRestrictedForTests() {
+        return new AccessRestrictedToTests();
+    }
+
     /**
      * Matches if a call from outside the defining class uses a method or constructor annotated with
      * {@link VisibleForTesting}. There are two exceptions:
@@ -120,25 +137,6 @@ private boolean isVisibleForTesting(final CanBeAnnotated target) {
         }
     }
 
-    /**
-     * Matches if a code unit of one of the registered classes has been called.
-     */
-    private static class TargetIsForbiddenClass extends DescribedPredicate<JavaCall<?>> {
-        private final String[] classes;
-
-        TargetIsForbiddenClass(final String... classes) {
-            super("forbidden class");
-
-            this.classes = Arrays.copyOf(classes, classes.length);
-        }
-
-        @Override
-        public boolean apply(final JavaCall<?> input) {
-            return StringUtils.containsAny(input.getTargetOwner().getFullName(), classes)
-                    && !"assertTimeoutPreemptively".equals(input.getName());
-        }
-    }
-
     /**
      * Predicate to match exception constructor calls without contexts.
      */