Version 3.1.0.RC3, simplify package acceptance rules for package scan…

…ning.
nickebbutt · Jan 3, 2019 · ec5b534 · ec5b534
1 parent 5ce6bf5
commit ec5b534
Show file tree

Hide file tree

Showing 11 changed files with 40 additions and 77 deletions.
diff --git a/build.gradle b/build.gradle
@@ -23,7 +23,7 @@ allprojects  {
     group = "org.chorusbdd"
     sourceCompatibility = '1.8'
     targetCompatibility = '1.8'
-    version = '3.1.0.RC2'
+    version = '3.1.0.RC3'
 
     test {
         testLogging {

diff --git a/...r/chorus-pathscanner/src/main/java/org/chorusbdd/chorus/pathscanner/ClasspathScanner.java b/...r/chorus-pathscanner/src/main/java/org/chorusbdd/chorus/pathscanner/ClasspathScanner.java
@@ -93,8 +93,7 @@ public static Set<Class> doScan(ClassFilter classFilter) {
      * in that the slashes will be changed to dots
      * and the .class file extension will be removed.
      */
-    static String[] getClasspathClassNames()
-            throws ZipException, IOException {
+    static String[] getClasspathClassNames() throws ZipException, IOException {
         final String[] classes = getClasspathFileNamesWithExtension(".class");
         for (int i = 0; i < classes.length; i++) {
             classes[i] = classes[i].substring(0, classes[i].length() - 6).replace("/", ".");
@@ -110,8 +109,7 @@ public boolean accept(String filename) {
         });
     }
 
-    static String[] getClasspathFileNames(FilenameFilter filter)
-            throws ZipException, IOException {
+    static String[] getClasspathFileNames(FilenameFilter filter) throws ZipException, IOException {
         final List<String> filenames = new ArrayList<>();
         for (String filename : getClasspathFileNames()) {
             if (filter.accept(filename)) {

diff --git a/...rus-pathscanner/src/main/java/org/chorusbdd/chorus/pathscanner/HandlerClassDiscovery.java b/...rus-pathscanner/src/main/java/org/chorusbdd/chorus/pathscanner/HandlerClassDiscovery.java
@@ -59,7 +59,7 @@ public HashMap<String, Class> discoverHandlerClasses(List<String> basePackages)
         HashMap<String, Class> handlerNameToHandlerClass = new HashMap<>();
 
         HandlerAnnotationFilter handlerAnnotationFilter = new HandlerAnnotationFilter();
-        ClassFilter filter = new ClassFilterDecorator().decorateWithPackageFilters(handlerAnnotationFilter, basePackages);
+        ClassFilter filter = new ClassFilterDecorator().decorateWithPackageNameFilters(handlerAnnotationFilter, basePackages);
 
         Set<Class> classes = ClasspathScanner.doScan(filter);
         for (Class handlerClass : classes) {

diff --git a/...thscanner/filter/PackagePrefixFilter.java → ...er/filter/AcceptNamedPackagePrefixes.java b/...thscanner/filter/PackagePrefixFilter.java → ...er/filter/AcceptNamedPackagePrefixes.java
@@ -23,9 +23,6 @@
  */
 package org.chorusbdd.chorus.pathscanner.filter;
 
-import org.chorusbdd.chorus.util.ChorusConstants;
-
-import java.util.Arrays;
 import java.util.List;
 
 /**
@@ -37,23 +34,17 @@
  * Accept any packages which match user specified prefixes
  * Deny any packages which do not
  */
-public class PackagePrefixFilter extends ChainableFilterRule {
+public class AcceptNamedPackagePrefixes extends ChainablePackageFilter {
 
     private List<String> packageNames;
-    private boolean userPackagesWereSpecified;
 
-    public PackagePrefixFilter(ClassFilter filterDelegate, List<String> packageNames) {
+    public AcceptNamedPackagePrefixes(ClassFilter filterDelegate, List<String> packageNames) {
         super(filterDelegate);
         this.packageNames = packageNames;
-        userPackagesWereSpecified = ! packageNames.equals(Arrays.asList(ChorusConstants.ANY_PACKAGE));
     }
 
     public boolean shouldAccept(String className) {
-        return userPackagesWereSpecified && checkMatch(className);
-    }
-
-    public boolean shouldDeny(String className) {
-        return userPackagesWereSpecified && ! checkMatch(className);
+        return packageNames.size() > 0 && checkMatch(className);
     }
 
     private boolean checkMatch(String className) {

diff --git a/.../src/main/java/org/chorusbdd/chorus/pathscanner/filter/AlwaysAllowBuiltInPackageRule.java b/.../src/main/java/org/chorusbdd/chorus/pathscanner/filter/AlwaysAllowBuiltInPackageRule.java
@@ -33,7 +33,7 @@
  *
  * Always accept built in handler packages
  */
-public class AlwaysAllowBuiltInPackageRule extends ChainableFilterRule {
+public class AlwaysAllowBuiltInPackageRule extends ChainablePackageFilter {
 
     public AlwaysAllowBuiltInPackageRule(ClassFilter filterDelegate) {
         super(filterDelegate);
@@ -43,10 +43,6 @@ protected boolean shouldAccept(String className) {
         return isBuiltInHandler(className);
     }
 
-    protected boolean shouldDeny(String className) {
-        return false;
-    }
-
     private boolean isBuiltInHandler(String className) {
         boolean result = false;
         for ( String pkg : ChorusConstants.BUILT_IN_PACKAGE_PREFIXES) {

diff --git a/...thscanner/filter/ChainableFilterRule.java → ...canner/filter/ChainablePackageFilter.java b/...thscanner/filter/ChainableFilterRule.java → ...canner/filter/ChainablePackageFilter.java
@@ -34,13 +34,13 @@
  *
  * Delegate to the chained delegate filter only if this filter rule is passed
  */
-public class ChainableFilterRule implements ClassFilter {
+public class ChainablePackageFilter implements ClassFilter {
 
-    ChorusLog log = ChorusLogFactory.getLog(ChainableFilterRule.class);
+    ChorusLog log = ChorusLogFactory.getLog(ChainablePackageFilter.class);
 
     private ClassFilter filterDelegate;
 
-    public ChainableFilterRule(ClassFilter filterDelegate) {
+    public ChainablePackageFilter(ClassFilter filterDelegate) {
         this.filterDelegate = filterDelegate;
     }
 
@@ -65,10 +65,7 @@ protected boolean shouldAccept(String className) {
     }
 
     public final boolean acceptByClass(Class clazz) {
-        return doAcceptByClass(clazz) && filterDelegate.acceptByClass(clazz);
+        return filterDelegate.acceptByClass(clazz);
     }
 
-    protected boolean doAcceptByClass(Class clazz) {
-        return true;
-    }
 }
diff --git a/...thscanner/src/main/java/org/chorusbdd/chorus/pathscanner/filter/ClassFilterDecorator.java b/...thscanner/src/main/java/org/chorusbdd/chorus/pathscanner/filter/ClassFilterDecorator.java
@@ -23,9 +23,6 @@
  */
 package org.chorusbdd.chorus.pathscanner.filter;
 
-import org.chorusbdd.chorus.util.ChorusConstants;
-
-import java.util.Arrays;
 import java.util.List;
 
 /**
@@ -34,17 +31,18 @@
  * Date: 19/06/12
  * Time: 08:18
  *
- * Construct filters for class search, by decorating a supplied ClassFilter with other filter rules
+ * Loading classes is expensive and may generate class loading errors, so we only permit scanning/classloading 
+ * for preconfigured packages, and those configured by the user (and their subpackages)
+ * 
+ * Construct filters to accept or deny by package name, by decorating a supplied ClassFilter with other filter rules
  *
  * Process for each class name found on the classpath until that class is 'accepted' or 'denied' by a rule
+ * If the package is accepted, delegate to the wrapped filter to perform further filtering by Class type
  *
- * 1. Accept all handlers from Chorus handlers package
- * 2. If the user specified package prefixes, accept only those deny any others
- * 3. Deny any other chorus packages so that we don't trigger class load of optional dependencies
- * 4. Deny handler scanning from standard jdk or library package prefixes
- * 5. Delegate to wrapped classFilter (e.g. to check annotation exists on class)
- *
- * 2 before 3 allows us to specify an extra chorus package as a handler package for chorus self testing
+ * Rules will be processed in this order:
+ * 1. Accept Chorus built in handler packages
+ * 2. Accept packages named by the user (and their subpackages)
+ * 3. Deny all packages not otherwise allowed
  */
 public class ClassFilterDecorator {
 
@@ -54,18 +52,14 @@ public class ClassFilterDecorator {
      * @param filterToDecorate this filter will be decorated with extra rules to check package names
      * @param userSpecifiedPrefixes any handler package prefixes specified by user
      */
-    public ClassFilter decorateWithPackageFilters(ClassFilter filterToDecorate, List<String> userSpecifiedPrefixes) {
-
-        //deny other chorus packages from the non-standard handlers package
-        DenyOtherChorusPackagesRule denyOtherChorusPackagesRule = new DenyOtherChorusPackagesRule(filterToDecorate);
+    public ClassFilter decorateWithPackageNameFilters(ClassFilter filterToDecorate, List<String> userSpecifiedPrefixes) {
+        DenyAllPackageNames denyAllPackageNames = new DenyAllPackageNames(filterToDecorate);
 
         //if user has specified package prefixes, restrict to those
-        List<String> userPackageNames = userSpecifiedPrefixes.isEmpty() ? Arrays.asList(ChorusConstants.ANY_PACKAGE) : userSpecifiedPrefixes;
-        ClassFilter packagePrefixFilter = new PackagePrefixFilter(denyOtherChorusPackagesRule, userPackageNames);
+        ClassFilter packagePrefixFilter = new AcceptNamedPackagePrefixes(denyAllPackageNames, userSpecifiedPrefixes);
 
          //always permit built in handlers, deny other chorus packages
-        ClassFilter builtInHandlerClassFilter = new AlwaysAllowBuiltInPackageRule(packagePrefixFilter);
-        return builtInHandlerClassFilter;
+        return new AlwaysAllowBuiltInPackageRule(packagePrefixFilter);
     }
 
 }
diff --git a/...r/filter/DenyOtherChorusPackagesRule.java → ...thscanner/filter/DenyAllPackageNames.java b/...r/filter/DenyOtherChorusPackagesRule.java → ...thscanner/filter/DenyAllPackageNames.java
@@ -23,33 +23,21 @@
  */
 package org.chorusbdd.chorus.pathscanner.filter;
 
-import org.chorusbdd.chorus.util.ChorusConstants;
-
 /**
  * Created by IntelliJ IDEA.
  * User: Nick Ebbutt
  * Date: 18/06/12
  * Time: 18:49
- *
- * Always accept built in handler packages
- * Always deny all other org.chorusbdd.* packages, avoid scanning classes which might load optional dependencies
- *
- *  We only look for handlers in the dedicated interpreter handler package
- * loading other classes in the interpreter may trigger class locating of
- * classes from optional dependencies, which we do not want to do since
- * this would make those optional dependencies mandatory
+ * 
+ * Deny all classes irrespective of package prefix
  */
-public class DenyOtherChorusPackagesRule extends ChainableFilterRule {
-
-    public DenyOtherChorusPackagesRule(ClassFilter filterDelegate) {
-        super(filterDelegate);
-    }
+public class DenyAllPackageNames extends ChainablePackageFilter {
 
-    protected boolean shouldAccept(String className) {
-        return false;
+    public DenyAllPackageNames(ClassFilter classFilter) {
+        super(classFilter);
     }
 
     protected boolean shouldDeny(String className) {
-        return className.startsWith(ChorusConstants.CHORUS_ROOT_PACKAGE);
+        return true;
     }
 }
diff --git a/...rus-pathscanner/src/main/java/org/chorusbdd/chorus/pathscanner/filter/FilenameFilter.java b/...rus-pathscanner/src/main/java/org/chorusbdd/chorus/pathscanner/filter/FilenameFilter.java
@@ -36,5 +36,5 @@ public interface FilenameFilter {
      * using forward slashes and no
      * files will begin with a slash
      */
-    public boolean accept(String filename);
+    boolean accept(String filename);
 }
diff --git a/...anner/src/test/java/org/chorusbdd/chorus/pathscanner/filter/TestClassFilterDecorator.java b/...anner/src/test/java/org/chorusbdd/chorus/pathscanner/filter/TestClassFilterDecorator.java
@@ -42,34 +42,33 @@ public class TestClassFilterDecorator extends Assert {
 
     @Test
     public void testChorusBuildInHandlersPermitted() {
-        ClassFilter classFilter = filterFactory.decorateWithPackageFilters(new HandlerAnnotationFilter(), Collections.<String>emptyList());
+        ClassFilter classFilter = filterFactory.decorateWithPackageNameFilters(new HandlerAnnotationFilter(), Collections.<String>emptyList());
         assertTrue("Allows built in handler", classFilter.acceptByName("org.chorusbdd.chorus.handlers"));
         assertTrue("Allows built in tests handlers", classFilter.acceptByName("org.chorusbdd.chorus.selftest.wibble"));
     }
 
     @Test
     public void testChorusInterpreterPackagesDenied() {
-        ClassFilter classFilter = filterFactory.decorateWithPackageFilters(new HandlerAnnotationFilter(), Collections.<String>emptyList());
+        ClassFilter classFilter = filterFactory.decorateWithPackageNameFilters(new HandlerAnnotationFilter(), Collections.<String>emptyList());
         assertFalse("Denies other interpreter packages", classFilter.acceptByName(ChorusConstants.CHORUS_ROOT_PACKAGE));
     }
 
-
     @Test
-    public void testAllowsAllOtherIfNoUserPrefixesSpecified() {
-        ClassFilter classFilter = filterFactory.decorateWithPackageFilters(new HandlerAnnotationFilter(), Collections.<String>emptyList());
-        assertTrue("Allows all non-chorus if user did not restrict", classFilter.acceptByName("com.mynew.google"));
+    public void testDoesNotAllowNonChorusIfNoUserPackagesSpecified() {
+        ClassFilter classFilter = filterFactory.decorateWithPackageNameFilters(new HandlerAnnotationFilter(), Collections.<String>emptyList());
+        assertFalse("Does not allow non-chorus if no user packages specified", classFilter.acceptByName("com.mynew.google"));
     }
 
     @Test
     public void testUserPrefixesSpecified() {
-        ClassFilter classFilter = filterFactory.decorateWithPackageFilters(new HandlerAnnotationFilter(), Arrays.asList("com.test"));
+        ClassFilter classFilter = filterFactory.decorateWithPackageNameFilters(new HandlerAnnotationFilter(), Arrays.asList("com.test"));
         assertFalse("Denies non-specified if user restricted", classFilter.acceptByName("com.mynew.google"));
         assertTrue("Allows specified if user restricted", classFilter.acceptByName("com.test.mypackage"));
     }
 
     @Test
     public void testCoreHandlersIfUserPackagesSpecified() {
-       ClassFilter classFilter = filterFactory.decorateWithPackageFilters(new HandlerAnnotationFilter(), Arrays.asList("com.test"));
+       ClassFilter classFilter = filterFactory.decorateWithPackageNameFilters(new HandlerAnnotationFilter(), Arrays.asList("com.test"));
        assertTrue("Allow standard handlers even if users sets handler package prefixes", classFilter.acceptByName("org.chorusbdd.chorus.handlers.MyHandler"));
     }
 

diff --git a/...ter/chorus-subsystem/src/main/java/org/chorusbdd/chorus/subsystem/SubsystemDiscovery.java b/...ter/chorus-subsystem/src/main/java/org/chorusbdd/chorus/subsystem/SubsystemDiscovery.java
@@ -63,7 +63,7 @@ HashMap<String, Class> discoverSubsystems(List<String> basePackages, ChorusLog l
         HashMap<String, Class> result = new HashMap<>();
 
         SubsystemConfigAnnotationFilter s = new SubsystemConfigAnnotationFilter();
-        ClassFilter filter = new ClassFilterDecorator().decorateWithPackageFilters(s, basePackages);
+        ClassFilter filter = new ClassFilterDecorator().decorateWithPackageNameFilters(s, basePackages);
 
         Set<Class> classes = ClasspathScanner.doScan(filter);
         for (Class subsystemInterface : classes) {