Merge pull request #266 from clairemcginty/allow-list-param

Add target* params for conflict targeting

CHANGELOG.md

@@ -1,3 +1,7 @@
+### 0.2.6
+- Added `targetSourcePackages` and `targetDestinationPackages` configuration options
+- Renamed `ignoreSubpackages` configuration option to `filterSubpackages`
+
 ### 0.2.5
 
 - Added `java.lang.invoke.VarHandle` to the list of classes with

README.md

@@ -205,9 +205,44 @@ packages on the destination side can be ignored with
 
 By default, all subpackages of the specified packages are also ignored, but
 this can be disabled on an individual basis by adding
-`<ignoreSubpackages>false</ignoreSubpackages>` to the `<ignoreSourcePackage>`
+`<filterSubpackages>false</filterSubpackages>` to the `<ignoreSourcePackage>`
+or `<ignoreDestinationPackage>` element. **Note**: In previous releases (<=0.2.5), this setting was named
+`ignoreSubpackages`. Setting `ignoreSubpackages` in your pom.xml is still supported; the plugin will
+translate it to the new key value.
+
+### Target only conflicts from certain packages
+
+Conversely, the plugin can be configured to _only_ report on conflicts in specific packages, based on the name of the class that has the
+conflict. There are separate configuration options for targeting conflicts on the "source" side of the conflict
+and the "destination" side of the conflict.
+
+Packages on the source side can be targeted with `<targetSourcePackages>` and
+packages on the destination side can be targeted with `<targetDestinationPackages>`:
+
+```xml
+<configuration>
+  <!-- Only target conflicts coming from groovy.lang source package -->
+  <targetSourcePackages>
+    <targetSourcePackage>
+      <package>groovy.lang</package>
+    </targetSourcePackage>
+  </targetSourcePackages>
+  <!-- Only target conflicts coming from com.foo package on the callee side -->
+  <targetDestinationPackages>
+    <targetDestinationPackage>
+      <package>com.foo</package>
+    </targetDestinationPackage>
+  </targetDestinationPackages>
+</configuration>
+```
+
+By default, all subpackages of the specified packages are also ignored, but
+this can be disabled on an individual basis by adding
+`<filterSubpackages>false</filterSubpackages>` to the `<ignoreSourcePackage>`
 or `<ignoreDestinationPackage>` element.
 
+**Note** that `target*` options CANNOT be used in conjunction with `ignore*` options. You can only specify one or the other.
+
 # Caveats and Limitations
 
 Because this plugin analyzes the bytecode of the `.class` files of your code

maven-plugin/src/it/00-test-harness/a-v1/src/main/java/com/spotify/missinglink/d/WillGoAway.java

@@ -0,0 +1,8 @@
+package com.spotify.missinglink.d;
+
+/**
+ * TODO: document!
+ */
+public class WillGoAway {
+
+}

maven-plugin/src/it/00-test-harness/b/src/main/java/com/spotify/missinglink/e/E.java

@@ -0,0 +1,12 @@
+package com.spotify.missinglink.e;
+
+import com.spotify.missinglink.d.WillGoAway;
+
+/**
+ * TODO: document!
+ */
+public class E {
+  public static void classShouldBeMissing() {
+    System.out.println(new WillGoAway());
+  }
+}

maven-plugin/src/it/class-missing-target-destination/pom.xml

@@ -0,0 +1,60 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
+  <modelVersion>4.0.0</modelVersion>
+
+  <groupId>com.spotify.missinglink.tests</groupId>
+  <artifactId>class-missing-target-destination</artifactId>
+  <version>1-SNAPSHOT</version>
+
+  <dependencies>
+    <dependency>
+      <groupId>com.spotify.missinglink.tests</groupId>
+      <artifactId>a</artifactId>
+      <version>2-SNAPSHOT</version>
+    </dependency>
+    <dependency>
+      <groupId>com.spotify.missinglink.tests</groupId>
+      <artifactId>b</artifactId>
+      <version>1-SNAPSHOT</version>
+    </dependency>
+    <dependency>
+      <groupId>junit</groupId>
+      <artifactId>junit</artifactId>
+      <version>4.13.1</version>
+      <scope>test</scope>
+    </dependency>
+  </dependencies>
+
+  <build>
+    <plugins>
+      <plugin>
+        <groupId>@project.groupId@</groupId>
+        <artifactId>@project.artifactId@</artifactId>
+        <version>@project.version@</version>
+        <configuration>
+          <targetDestinationPackages>
+            <targetDestinationPackage>
+              <package>com.spotify.missinglink.d</package>
+            </targetDestinationPackage>
+          </targetDestinationPackages>
+        </configuration>
+        <executions>
+          <execution>
+            <goals><goal>check</goal></goals>
+            <phase>process-classes</phase>
+          </execution>
+        </executions>
+      </plugin>
+      <plugin>
+        <groupId>org.apache.maven.plugins</groupId>
+        <artifactId>maven-compiler-plugin</artifactId>
+        <version>3.5</version>
+        <configuration>
+          <source>8</source>
+          <target>8</target>
+          <useIncrementalCompilation>false</useIncrementalCompilation>
+        </configuration>
+      </plugin>
+    </plugins>
+  </build>
+</project>

maven-plugin/src/it/class-missing-target-destination/src/main/java/com/spotify/missinglink/ClassMissingAllowDestination.java

@@ -0,0 +1,14 @@
+package com.spotify.missinglink;
+
+import com.spotify.missinglink.e.E;
+
+/**
+ * This is expected to generate a runtime error because WillGoAway(), as invoked by E.classShouldBeMissing(),
+ * doesn't exist in a conflicting package.
+ */
+public class ClassMissingAllowDestination {
+
+  public static void main(String[] args) {
+    E.classShouldBeMissing();
+  }
+}

maven-plugin/src/it/class-missing-target-destination/src/test/java/com/spotify/missinglink/ClassMissingAllowDestinationTest.java

@@ -0,0 +1,13 @@
+package com.spotify.missinglink;
+
+import org.junit.Assert;
+import org.junit.Test;
+
+public class ClassMissingAllowDestinationTest {
+
+  @Test
+  public void shouldThrowError() throws Exception {
+    Throwable ex = Assert.assertThrows(NoClassDefFoundError.class, () -> ClassMissingAllowDestination.main(new String[0]));
+    Assert.assertTrue(ex.getMessage().contains("WillGoAway"));
+  }
+}

maven-plugin/src/main/java/com/spotify/missinglink/maven/CheckMojo.java

@@ -140,7 +140,15 @@ public class CheckMojo extends AbstractMojo {
    * #excludeDependencies} but operates at a package name level instead of a groupId/artifactId
    * level.
    */
-  @Parameter protected List<IgnoredPackage> ignoreSourcePackages = new ArrayList<>();
+  @Parameter protected List<PackageFilter> ignoreSourcePackages = new ArrayList<>();
+
+  /**
+   * Optional list of source packages to target in conflict searches. Any conflicts originating in
+   * packages outside of this list will be ignored.
+   *
+   * <p>This parameter CANNOT be used in conjunction with ignoreSourcePackages. Only one can be set.
+   */
+  @Parameter protected List<PackageFilter> targetSourcePackages = new ArrayList<>();
 
   /**
    * Optional list of packages to ignore conflicts in where the destination/called-side of the
@@ -152,8 +160,20 @@ public class CheckMojo extends AbstractMojo {
    *
    * <p>For example, if the package "javax.bar" is in ignoreDestinationPackages, then any conflict
    * found having to do with calling a method in a class in javax.bar is ignored.
+   *
+   * <p>This parameter CANNOT be used in conjunction with targetDestinationPackages. Only one can be
+   * set.
    */
-  @Parameter protected List<IgnoredPackage> ignoreDestinationPackages = new ArrayList<>();
+  @Parameter protected List<PackageFilter> ignoreDestinationPackages = new ArrayList<>();
+
+  /**
+   * Optional list of destination packages to target in conflict searches. Any conflicts originating
+   * in packages outside of this list will be ignored.
+   *
+   * <p>This parameter CANNOT be used in conjunction with ignoreDestinationPackages. Only one can be
+   * set.
+   */
+  @Parameter protected List<PackageFilter> targetDestinationPackages = new ArrayList<>();
 
   /**
    * Optional: can be set to explicitly define the path to use for the bootclasspath containing the
@@ -197,6 +217,17 @@ public void execute() throws MojoExecutionException, MojoFailureException {
               + Joiner.on(", ").join(ConflictCategory.values()));
     }
 
+    if (!ignoreDestinationPackages.isEmpty() && !targetDestinationPackages.isEmpty()) {
+      throw new MojoExecutionException(
+          "Either ignoreDestinationPackages or targetDestinationPackages can be set, "
+              + "but not both.");
+    }
+
+    if (!ignoreSourcePackages.isEmpty() && !targetSourcePackages.isEmpty()) {
+      throw new MojoExecutionException(
+          "Either ignoreSourcePackages or targetSourcePackages can be set, " + "but not both.");
+    }
+
     Collection<Conflict> conflicts = loadArtifactsAndCheckConflicts();
     final int initialCount = conflicts.size();
 
@@ -260,7 +291,7 @@ private Collection<Conflict> filterConflicts(
       getLog().debug("Ignoring source packages: " + Joiner.on(", ").join(ignoreSourcePackages));
 
       final Predicate<Conflict> predicate =
-          conflict -> !packageIsIgnored(ignoreSourcePackages, conflict.dependency().fromClass());
+          conflict -> !packageIsFiltered(ignoreSourcePackages, conflict.dependency().fromClass());
 
       conflicts =
           filterConflictsBy(
@@ -271,6 +302,24 @@ private Collection<Conflict> filterConflicts(
                       + " conflicts found in ignored source packages. "
                       + "Run plugin again without the 'ignoreSourcePackages' parameter to see "
                       + "all conflicts that were found.");
+    } else if (!targetSourcePackages.isEmpty()) {
+      getLog()
+          .debug(
+              "Checking for conflicts in source packages: "
+                  + Joiner.on(", ").join(targetSourcePackages));
+
+      final Predicate<Conflict> predicate =
+          conflict -> packageIsFiltered(targetSourcePackages, conflict.dependency().fromClass());
+
+      conflicts =
+          filterConflictsBy(
+              conflicts,
+              predicate,
+              num ->
+                  num
+                      + " conflicts found in targeted source packages. "
+                      + "Run plugin again without the 'targetSourcePackages' parameter to see "
+                      + "all conflicts that were found.");
     }
 
     if (!ignoreDestinationPackages.isEmpty()) {
@@ -280,7 +329,7 @@ private Collection<Conflict> filterConflicts(
 
       final Predicate<Conflict> predicate =
           conflict ->
-              !packageIsIgnored(ignoreDestinationPackages, conflict.dependency().targetClass());
+              !packageIsFiltered(ignoreDestinationPackages, conflict.dependency().targetClass());
 
       conflicts =
           filterConflictsBy(
@@ -291,6 +340,25 @@ private Collection<Conflict> filterConflicts(
                       + " conflicts found in ignored destination packages. "
                       + "Run plugin again without the 'ignoreDestinationPackages' parameter to see "
                       + "all conflicts that were found.");
+    } else if (!targetDestinationPackages.isEmpty()) {
+      getLog()
+          .debug(
+              "Checking for conflicts in destination packages: "
+                  + Joiner.on(", ").join(targetDestinationPackages));
+
+      final Predicate<Conflict> predicate =
+          conflict ->
+              packageIsFiltered(targetDestinationPackages, conflict.dependency().targetClass());
+
+      conflicts =
+          filterConflictsBy(
+              conflicts,
+              predicate,
+              num ->
+                  num
+                      + " conflicts found in allowed targeted packages. "
+                      + "Run plugin again without the 'targetDestinationPackages' parameter to see "
+                      + "all conflicts that were found.");
     }
 
     return conflicts;
@@ -327,19 +395,18 @@ private Collection<Conflict> filterConflictsBy(
    * destination-side) is ignored based on the collection of IgnoredPackages. Reusable logic between
    * ignoring source/destination packages.
    */
-  private boolean packageIsIgnored(
-      Collection<IgnoredPackage> ignoredPackages, ClassTypeDescriptor classTypeDescriptor) {
-
+  private boolean packageIsFiltered(
+      Collection<PackageFilter> packageFilters, ClassTypeDescriptor classTypeDescriptor) {
     final String className = classTypeDescriptor.getClassName().replace('/', '.');
     // this might be missing some corner-cases on naming rules:
     final String conflictPackageName = className.substring(0, className.lastIndexOf('.'));
 
-    return ignoredPackages.stream()
+    return packageFilters.stream()
         .anyMatch(
             p -> {
               final String ignoredPackageName = p.getPackage();
               return conflictPackageName.equals(ignoredPackageName)
-                  || (p.isIgnoreSubpackages()
+                  || (p.isFilterSubpackages()
                       && conflictPackageName.startsWith(ignoredPackageName + "."));
             });
   }

maven-plugin/src/main/java/com/spotify/missinglink/maven/IgnoredPackage.java → maven-plugin/src/main/java/com/spotify/missinglink/maven/PackageFilter.java

@@ -42,22 +42,22 @@
 /**
  * Package names to be ignored when reporting conflicts. A package name is a String like
  * "javax.servlet" (not a regular expression). By default, subpackages of the specified package are
- * also ignored - to disable this behavior, set ignoreSubpackages to false.
+ * also ignored - to disable this behavior, set filterSubpackages to false.
  */
-public class IgnoredPackage {
+public class PackageFilter {
 
   // Plexus seems to require classes to be referenced in the Maven project model to have no-arg
   // constructors and getters/setters.
 
   private String name;
-  private boolean ignoreSubpackages = true;
+  private boolean filterSubpackages = true;
 
-  public IgnoredPackage() {}
+  public PackageFilter() {}
 
-  public IgnoredPackage(String name, boolean ignoreSubpackages) {
+  public PackageFilter(String name, boolean filterSubpackages) {
     this.name = checkNotNull(name);
     Preconditions.checkArgument(!name.isEmpty(), "name cannot be empty");
-    this.ignoreSubpackages = ignoreSubpackages;
+    this.filterSubpackages = filterSubpackages;
   }
 
   public String getPackage() {
@@ -69,12 +69,16 @@ public void setPackage(String name) {
     this.name = name;
   }
 
-  public boolean isIgnoreSubpackages() {
-    return ignoreSubpackages;
+  public boolean isFilterSubpackages() {
+    return filterSubpackages;
+  }
+
+  public void setFilterSubpackages(boolean filterSubpackages) {
+    this.filterSubpackages = filterSubpackages;
   }
 
   public void setIgnoreSubpackages(boolean ignoreSubpackages) {
-    this.ignoreSubpackages = ignoreSubpackages;
+    this.filterSubpackages = ignoreSubpackages;
   }
 
   @Override
@@ -86,9 +90,9 @@ public boolean equals(Object o) {
       return false;
     }
 
-    IgnoredPackage that = (IgnoredPackage) o;
+    PackageFilter that = (PackageFilter) o;
 
-    if (ignoreSubpackages != that.ignoreSubpackages) {
+    if (filterSubpackages != that.filterSubpackages) {
       return false;
     }
     return name.equals(that.name);
@@ -97,7 +101,7 @@ public boolean equals(Object o) {
   @Override
   public int hashCode() {
     int result = name.hashCode();
-    result = 31 * result + (ignoreSubpackages ? 1 : 0);
+    result = 31 * result + (filterSubpackages ? 1 : 0);
     return result;
   }
 
@@ -107,8 +111,8 @@ public String toString() {
         + "name='"
         + name
         + '\''
-        + ", ignoreSubpackages="
-        + ignoreSubpackages
+        + ", filterSubpackages="
+        + filterSubpackages
         + '}';
   }
 }