Merge pull request #1 from alvarolobato/master

[JENKINS-37317] Fix problems with requiredClasses
jenkinsci · Oct 5, 2016 · b974548 · b974548
2 parents 268df48 + e9d3dd9
commit b974548
Show file tree

Hide file tree

Showing 6 changed files with 110 additions and 18 deletions.
diff --git a/pom.xml b/pom.xml
@@ -41,4 +41,26 @@
       <url>http://repo.jenkins-ci.org/public/</url>
     </pluginRepository>
   </pluginRepositories>
+  <dependencies>
+    <dependency>
+      <groupId>org.jenkins-ci.plugins</groupId>
+      <artifactId>beer</artifactId>
+      <version>1.2</version>
+      <optional>true</optional>
+      <scope>test</scope>
+    </dependency>
+  </dependencies>
+ <build>
+    <plugins>
+        <plugin>
+          <groupId>org.apache.maven.plugins</groupId>
+          <artifactId>maven-surefire-plugin</artifactId>
+          <configuration>
+            <classpathDependencyExcludes>
+              <classpathDependencyExcludes>org.jenkins-ci.plugins:beer</classpathDependencyExcludes>
+            </classpathDependencyExcludes>
+          </configuration>
+        </plugin>
+      </plugins>
+    </build>
 </project>
diff --git a/src/main/java/org/jenkinsci/plugins/variant/OptionalExtensionProcessor.java b/src/main/java/org/jenkinsci/plugins/variant/OptionalExtensionProcessor.java
@@ -1,13 +1,13 @@
 package org.jenkinsci.plugins.variant;
 
+import java.lang.reflect.AnnotatedElement;
+import java.lang.reflect.Member;
+
 import hudson.Extension;
 import hudson.ExtensionFinder.GuiceExtensionAnnotation;
 import hudson.PluginWrapper;
 import jenkins.model.Jenkins;
 
-import java.lang.reflect.AnnotatedElement;
-import java.lang.reflect.Member;
-
 /**
  * Processes extensions marked with {@link OptionalExtension} and decides when they are activated.
  *
@@ -25,13 +25,17 @@ protected double getOrdinal(OptionalExtension annotation) {
     }
 
     /**
-     * If the trigger condition is not met, we filter it by not making the extension active,
-     * which means extensions are always non-optional as far as {@link GuiceExtensionAnnotation}
-     * is concerned.
+     * If the trigger condition is not met, we filter it by not making the extension active, which means extensions
+     * could always be non-optional as far as {@link GuiceExtensionAnnotation} is concerned. But in some situations, the
+     * class cannot even be loaded which would make {@link GuiceExtensionAnnotation} to fail, so we need to also make it
+     * optional.
+     * <p>
+     * See <a href="https://issues.jenkins-ci.org/browse/JENKINS-37317">JENKINS-37317</a>
+     * 
      */
     @Override
     protected boolean isOptional(OptionalExtension annotation) {
-        return false;
+        return true;
     }
 
     /**
@@ -41,12 +45,30 @@ protected boolean isOptional(OptionalExtension annotation) {
      */
     @Override
     protected boolean isActive(AnnotatedElement e) {
-        for (; e!=null; e=getParentOf(e)) {
-            OptionalExtension a = e.getAnnotation(OptionalExtension.class);
-            if (a!=null && !isActive(a))    return false;
-
-            OptionalPackage b = e.getAnnotation(OptionalPackage.class);
-            if (b!=null && !isActive(b))    return false;
+        for (; e != null; e = getParentOf(e)) {
+            try {
+                OptionalExtension a = e.getAnnotation(OptionalExtension.class);
+                if (a != null && !isActive(a)) {
+                    return false;
+                }
+            } catch (ArrayStoreException e1) {
+                // In this case the annotation is referencing a non existent class, make the extension inactive it is
+                // due to the use of requiredClasses
+                // see http://bugs.java.com/view_bug.do?bug_id=7183985
+                return false;
+            }
+
+            try {
+                OptionalPackage b = e.getAnnotation(OptionalPackage.class);
+                if (b != null && !isActive(b)) {
+                    return false;
+                }
+            } catch (ArrayStoreException e1) {
+                // In this case the annotation is referencing a non existent class, make the extension inactive it is
+                // due to the use of requiredClasses
+                // see http://bugs.java.com/view_bug.do?bug_id=7183985
+                return false;
+            }
         }
 
         return true;
@@ -57,10 +79,11 @@ private boolean isActive(OptionalExtension a) {
         try {
             a.requireClasses();
         } catch (ArrayStoreException x) {
+            // In this case the annotation is referencing a non existent class, make the extension inactive
             // see http://bugs.java.com/view_bug.do?bug_id=7183985
-            return true;
+            return false;
         } catch (TypeNotPresentException x) {
-            return true;
+            return false;
         }
 
         for (String name : a.requirePlugins()) {
@@ -79,10 +102,11 @@ private boolean isActive(OptionalPackage a) {
         try {
             a.requireClasses();
         } catch (ArrayStoreException x) {
+            // In this case the annotation is referencing a non existent class, make the extension inactive
             // see http://bugs.java.com/view_bug.do?bug_id=7183985
-            return true;
+            return false;
         } catch (TypeNotPresentException x) {
-            return true;
+            return false;
         }
 
         for (String name : a.requirePlugins()) {

diff --git a/src/test/java/org/jenkinsci/plugins/variant/Negative3.java b/src/test/java/org/jenkinsci/plugins/variant/Negative3.java
@@ -0,0 +1,9 @@
+package org.jenkinsci.plugins.variant;
+
+import org.jenkinsci.plugins.Beer;
+import org.jvnet.hudson.test.Issue;
+
+@OptionalExtension(requireClasses = Beer.class)
+@Issue("JENKINS-37317")
+public class Negative3 extends Base {
+}
diff --git a/src/test/java/org/jenkinsci/plugins/variant/Negative4.java b/src/test/java/org/jenkinsci/plugins/variant/Negative4.java
@@ -0,0 +1,10 @@
+package org.jenkinsci.plugins.variant;
+
+import org.jenkinsci.plugins.Beer;
+import org.jvnet.hudson.test.Issue;
+
+@OptionalExtension(requireClasses = Beer.class)
+@Issue("JENKINS-37317")
+public class Negative4 extends Beer {
+
+}
diff --git a/src/test/java/org/jenkinsci/plugins/variant/Positive4.java b/src/test/java/org/jenkinsci/plugins/variant/Positive4.java
@@ -0,0 +1,8 @@
+package org.jenkinsci.plugins.variant;
+
+import org.jvnet.hudson.test.Issue;
+
+@OptionalExtension(requireClasses = VariantSet.class)
+@Issue("JENKINS-37317")
+public class Positive4 extends Base {
+}
diff --git a/src/test/java/org/jenkinsci/plugins/variant/VariantTest.java b/src/test/java/org/jenkinsci/plugins/variant/VariantTest.java
@@ -9,6 +9,7 @@
 import org.junit.BeforeClass;
 import org.junit.Rule;
 import org.junit.Test;
+import org.jvnet.hudson.test.Issue;
 import org.jvnet.hudson.test.JenkinsRule;
 
 import java.util.ArrayList;
@@ -21,7 +22,6 @@ public class VariantTest extends Assert {
     @Rule
     public JenkinsRule j = new JenkinsRule();
 
-
     @BeforeClass
     public static void setUp() {
         VariantSet.INSTANCE = new VariantSet("test");
@@ -47,4 +47,23 @@ public void test() {
         assertTrue(!classes.contains(Negative5.class));
         assertTrue(!classes.contains(Negative6.class));
     }
+
+    @Test
+    @Issue("JENKINS-37317")
+    public void testRequiredClass() {
+        List<Class> classes = new ArrayList<Class>();
+        for (Action a : j.jenkins.getActions()) {
+            classes.add(a.getClass());
+        }
+        assertTrue("Negative 3 should not exist", !classes.contains(Negative3.class));
+
+        for (Class klass : classes) {
+            System.out.println(klass.getCanonicalName());
+            assertTrue("Negative 4 should not exist",
+                    !klass.getCanonicalName().equals("org.jenkinsci.plugins.variant.Negative4"));
+        }
+
+        assertTrue(classes.contains(Positive4.class));
+    }
+
 }