[JENKINS-33843] Permit disabling optional dependencies (#4001)

* Implied dependencies on detached plugins were not being marked optional despite being listed in optionalDependencies, confusing the calculation of optionalDependents in resolveDependentPlugins.

* Javadoc improvements based on code exploration.

* [JENKINS-33843] Do not consider optional dependencies for purposes of disabling plugins.

* PluginManagerInstalledGUITest updated to reflect new behavior.

core/src/main/java/hudson/LocalPluginManager.java

@@ -66,12 +66,6 @@ public class LocalPluginManager extends PluginManager {
         this(null, rootDir);
     }
 
-    /**
-     * If the war file has any "/WEB-INF/plugins/*.jpi", extract them into the plugin directory.
-     *
-     * @return
-     *      File names of the bundled plugins. Like {"ssh-slaves.jpi","subversion.jpi"}
-     */
     @Override
     protected Collection<String> loadBundledPlugins() {
         // this is used in tests, when we want to override the default bundled plugins with .jpl (or .hpl) versions

core/src/main/java/hudson/PluginManager.java

@@ -1019,15 +1019,18 @@ public abstract class PluginManager extends AbstractModelObject implements OnMas
      * If the war file has any "/WEB-INF/plugins/[*.jpi | *.hpi]", extract them into the plugin directory.
      *
      * @return
-     *      File names of the bundled plugins. Like {"ssh-slaves.hpi","subversion.jpi"}
+     *      File names of the bundled plugins. Normally empty (not to be confused with {@link #loadDetachedPlugins}) but OEM WARs may have some.
      * @throws Exception
      *      Any exception will be reported and halt the startup.
      */
     protected abstract Collection<String> loadBundledPlugins() throws Exception;
 
     /**
-     * Copies the bundled plugin from the given URL to the destination of the given file name (like 'abc.jpi'),
-     * with a reasonable up-to-date check. A convenience method to be used by the {@link #loadBundledPlugins()}.
+     * Copies the plugin from the given URL to the given destination.
+     * Despite the name, this is used also from {@link #loadDetachedPlugins}.
+     * Includes a reasonable up-to-date check.
+     * A convenience method to be used by {@link #loadBundledPlugins()}.
+     * @param fileName like {@code abc.jpi}
      */
     protected void copyBundledPlugin(URL src, String fileName) throws IOException {
         LOGGER.log(FINE, "Copying {0}", src);

core/src/main/java/hudson/PluginWrapper.java

@@ -255,6 +255,7 @@ public class PluginWrapper implements Comparable<PluginWrapper>, ModelObject {
 
     /**
      * Get the list of components that depend on this plugin.
+     * Note that the list will include elements of {@link #getOptionalDependents}.
      * @return The list of components that depend on this plugin.
      */
     public @Nonnull Set<String> getDependents() {
@@ -273,6 +274,16 @@ public class PluginWrapper implements Comparable<PluginWrapper>, ModelObject {
         return getDependents();
     }
 
+    /**
+     * Like {@link #getDependents} but excluding optional dependencies.
+     * @since TODO
+     */
+    public @Nonnull Set<String> getMandatoryDependents() {
+        Set<String> s = new HashSet<>(dependents);
+        s.removeAll(optionalDependents);
+        return s;
+    }
+
     /**
      * @return The list of components that depend optionally on this plugin.
      */
@@ -290,6 +301,7 @@ public class PluginWrapper implements Comparable<PluginWrapper>, ModelObject {
 
     /**
      * Does this plugin have anything that depends on it.
+     * Note that optional dependents are included.
      * @return {@code true} if something (Jenkins core, or another plugin) depends on this
      * plugin, otherwise {@code false}.
      */
@@ -297,6 +309,17 @@ public class PluginWrapper implements Comparable<PluginWrapper>, ModelObject {
         return (isBundled || !dependents.isEmpty());
     }
 
+    /**
+     * Like {@link #hasDependents} but excluding optional dependencies.
+     * @since TODO
+     */
+    public boolean hasMandatoryDependents() {
+        if (isBundled) {
+            return true;
+        }
+        return dependents.stream().anyMatch(d -> !optionalDependents.contains(d));
+    }
+
     /**
      * @deprecated Please use {@link hasDependents}.
      */
@@ -324,10 +347,19 @@ public class PluginWrapper implements Comparable<PluginWrapper>, ModelObject {
 
     /**
      * Does this plugin depend on any other plugins.
+     * Note that this include optional dependencies.
      * @return {@code true} if this plugin depends on other plugins, otherwise {@code false}.
      */
     public boolean hasDependencies() {
-        return (dependencies != null && !dependencies.isEmpty());
+        return !dependencies.isEmpty();
+    }
+
+    /**
+     * Like {@link #hasDependencies} but omitting optional dependencies.
+     * @since TODO
+     */
+    public boolean hasMandatoryDependencies() {
+        return dependencies.stream().anyMatch(d -> !d.optional);
     }
 
     @ExportedBean
@@ -394,6 +426,9 @@ public class PluginWrapper implements Comparable<PluginWrapper>, ModelObject {
 		this.active = !disableFile.exists();
 		this.dependencies = dependencies;
 		this.optionalDependencies = optionalDependencies;
+        for (Dependency d : optionalDependencies) {
+            assert d.optional : d + " included among optionalDependencies of " + shortName + " but was not marked optional";
+        }
         this.archive = archive;
     }
 
@@ -438,11 +473,24 @@ public class PluginWrapper implements Comparable<PluginWrapper>, ModelObject {
         return getBaseName(fileName);
     }
 
+    /**
+     * Gets all dependencies of this plugin on other plugins.
+     * Note that the list will <em>usually</em> include the members of {@link #getOptionalDependencies}
+     * (missing optional dependencies will however be omitted).
+     */
     @Exported
     public List<Dependency> getDependencies() {
         return dependencies;
     }
 
+    /**
+     * Like {@link #getDependencies} but omits optional dependencies.
+     * @since TODO
+     */
+    public List<Dependency> getMandatoryDependencies() {
+        return dependencies.stream().filter(d -> !d.optional).collect(Collectors.toList());
+    }
+
     public List<Dependency> getOptionalDependencies() {
         return optionalDependencies;
     }
@@ -753,7 +801,12 @@ public class PluginWrapper implements Comparable<PluginWrapper>, ModelObject {
     public void setHasCycleDependency(boolean hasCycle){
         hasCycleDependency = hasCycle;
     }
-    
+
+    /**
+     * Is this plugin bundled in the WAR?
+     * Normally false as noted in {@link PluginManager#loadBundledPlugins}:
+     * this does <em>not</em> apply to “detached” plugins.
+     */
     @Exported
     public boolean isBundled() {
         return isBundled;

core/src/main/java/jenkins/plugins/DetachedPluginsUtil.java

@@ -93,7 +93,7 @@ public class DetachedPluginsUtil {
             }
             // some earlier versions of maven-hpi-plugin apparently puts "null" as a literal in Hudson-Version. watch out for them.
             if (jenkinsVersion == null || jenkinsVersion.equals("null") || new VersionNumber(jenkinsVersion).compareTo(detached.splitWhen) <= 0) {
-                out.add(new PluginWrapper.Dependency(detached.shortName + ':' + detached.requiredVersion));
+                out.add(new PluginWrapper.Dependency(detached.shortName + ':' + detached.requiredVersion + ";resolution:=optional"));
                 LOGGER.log(Level.FINE, "adding implicit dependency {0} → {1} because of {2}",
                            new Object[]{pluginName, detached.shortName, jenkinsVersion});
             }

core/src/main/resources/hudson/PluginManager/installed.jelly

@@ -67,7 +67,7 @@ THE SOFTWARE.
                 <th width="1">${%Uninstall}</th>
               </tr>
               <j:forEach var="p" items="${app.pluginManager.plugins}">
-                <tr class="plugin ${p.hasDependents()?'has-dependents':''} ${(p.hasDependents() &amp;&amp; !p.enabled)?'has-dependents-but-disabled':''} ${p.isDeleted()?'deleted':''}" data-plugin-id="${p.shortName}" data-plugin-name="${p.displayName}">
+                <tr class="plugin ${p.hasMandatoryDependents()?'has-dependents':''} ${(p.hasMandatoryDependents() &amp;&amp; !p.enabled)?'has-dependents-but-disabled':''} ${p.isDeleted()?'deleted':''}" data-plugin-id="${p.shortName}" data-plugin-name="${p.displayName}">
                   <j:set var="state" value="${p.enabled?'true':null}"/>
                   <td class="center pane enable" data="${state}">
                     <input type="checkbox" checked="${state}" onclick="flip(event)"
@@ -118,16 +118,16 @@ THE SOFTWARE.
                         <p>${%Uninstallation pending}</p>
                       </j:when>
                       <j:otherwise>
-                          <j:if test="${p.hasDependents()}">
+                          <j:if test="${p.hasMandatoryDependents()}">
                             <div class="dependent-list">
-                              <j:forEach var="dependent" items="${p.dependents}">
+                              <j:forEach var="dependent" items="${p.mandatoryDependents}">
                                 <span data-plugin-id="${dependent}"></span>
                               </j:forEach>
                             </div>
                           </j:if>
-                          <j:if test="${p.hasDependencies()}">
+                          <j:if test="${p.hasMandatoryDependencies()}">
                             <div class="dependency-list">
-                              <j:forEach var="dependency" items="${p.dependencies}">
+                              <j:forEach var="dependency" items="${p.mandatoryDependencies}">
                                 <span data-plugin-id="${dependency.shortName}"></span>  
                               </j:forEach>
                             </div>

test/src/test/java/hudson/PluginManagerInstalledGUITest.java

@@ -32,6 +32,7 @@ import com.gargoylesoftware.htmlunit.html.HtmlTableRow;
 import org.junit.Assert;
 import org.junit.Rule;
 import org.junit.Test;
+import org.jvnet.hudson.test.Issue;
 import org.jvnet.hudson.test.JenkinsRule;
 import org.jvnet.hudson.test.TestPluginManager;
 import org.xml.sax.SAXException;
@@ -60,8 +61,10 @@ public class PluginManagerInstalledGUITest {
                         try {
                             return super.loadBundledPlugins();
                         } finally {
-                            copyBundledPlugin(PluginManagerInstalledGUITest.class.getResource("/WEB-INF/detached-plugins/cvs.hpi"), "cvs.jpi"); // cannot use installDetachedPlugin at this point
-                            copyBundledPlugin(PluginManagerInstalledGUITest.class.getResource("/plugins/tasks.jpi"), "tasks.jpi");
+                            copyBundledPlugin(PluginManagerInstalledGUITest.class.getResource("/WEB-INF/detached-plugins/matrix-auth.hpi"), "matrix-auth.jpi"); // cannot use installDetachedPlugin at this point
+                            copyBundledPlugin(PluginManagerInstalledGUITest.class.getResource("/plugins/dependee-0.0.2.hpi"), "dependee.jpi");
+                            copyBundledPlugin(PluginManagerInstalledGUITest.class.getResource("/plugins/depender-0.0.2.hpi"), "depender.jpi");
+                            copyBundledPlugin(PluginManagerInstalledGUITest.class.getResource("/plugins/mandatory-depender-0.0.2.hpi"), "mandatory-depender.jpi");
                         }
                     }
                 };
@@ -71,52 +74,72 @@ public class PluginManagerInstalledGUITest {
             }
         }
     };
-    
+
+    @Issue("JENKINS-33843")
     @Test
     public void test_enable_disable_uninstall() throws IOException, SAXException {
         InstalledPlugins installedPlugins = new InstalledPlugins();
         
-        InstalledPlugin tasksPlugin = installedPlugins.get("tasks");
-        InstalledPlugin cvsPlugin = installedPlugins.get("cvs");
+        InstalledPlugin matrixAuthPlugin = installedPlugins.get("matrix-auth");
+        InstalledPlugin dependeePlugin = installedPlugins.get("dependee");
+        InstalledPlugin dependerPlugin = installedPlugins.get("depender");
+        InstalledPlugin mandatoryDependerPlugin = installedPlugins.get("mandatory-depender");
 
-        tasksPlugin.assertHasNoDependents();
-        cvsPlugin.assertHasDependents();
+        // As a detached plugin, it is an optional dependency of others built against a newer baseline.
+        matrixAuthPlugin.assertHasNoDependents();
+        // Has a mandatory dependency:
+        dependeePlugin.assertHasDependents();
+        // Leaf plugins:
+        dependerPlugin.assertHasNoDependents();
+        mandatoryDependerPlugin.assertHasNoDependents();
         
-        // Tasks plugin should be enabled and it should be possible to disable it
+        // This plugin should be enabled and it should be possible to disable it
         // because no other plugins depend on it.
-        tasksPlugin.assertEnabled();
-        tasksPlugin.assertEnabledStateChangeable();
-        tasksPlugin.assertUninstallable();
+        mandatoryDependerPlugin.assertEnabled();
+        mandatoryDependerPlugin.assertEnabledStateChangeable();
+        mandatoryDependerPlugin.assertUninstallable();
         
-        // CVS plugin should be enabled, but it should not be possible to disable or uninstall it
-        // because the tasks plugin depends on it.
-        cvsPlugin.assertEnabled();
-        cvsPlugin.assertEnabledStateNotChangeable();
-        cvsPlugin.assertNotUninstallable();
+        // This plugin should be enabled, but it should not be possible to disable or uninstall it
+        // because another plugin depends on it.
+        dependeePlugin.assertEnabled();
+        dependeePlugin.assertEnabledStateNotChangeable();
+        dependeePlugin.assertNotUninstallable();
         
-        // Disable the tasks plugin
-        tasksPlugin.clickEnabledWidget();
+        // Disable one plugin
+        mandatoryDependerPlugin.clickEnabledWidget();
                 
-        // Now the tasks plugin should be disabled, but it should be possible to re-enable it 
+        // Now that plugin should be disabled, but it should be possible to re-enable it
         // and it should still be uninstallable.
-        tasksPlugin.assertNotEnabled(); // this is different to earlier
-        tasksPlugin.assertEnabledStateChangeable();
-        tasksPlugin.assertUninstallable();
+        mandatoryDependerPlugin.assertNotEnabled(); // this is different to earlier
+        mandatoryDependerPlugin.assertEnabledStateChangeable();
+        mandatoryDependerPlugin.assertUninstallable();
                 
-        // The CVS plugin should still be enabled, but it should now be possible to disable it because
-        // the tasks plugin is no longer enabled. Should still not be possible to uninstall it.
-        cvsPlugin.assertEnabled();
-        cvsPlugin.assertEnabledStateChangeable(); // this is different to earlier
-        cvsPlugin.assertNotUninstallable();
+        // The dependee plugin should still be enabled, but it should now be possible to disable it because
+        // the mandatory depender plugin is no longer enabled. Should still not be possible to uninstall it.
+        // Note that the depender plugin does not block its disablement.
+        dependeePlugin.assertEnabled();
+        dependeePlugin.assertEnabledStateChangeable(); // this is different to earlier
+        dependeePlugin.assertNotUninstallable();
+        dependerPlugin.assertEnabled();
         
-        // Disable the cvs plugin
-        cvsPlugin.clickEnabledWidget();
+        // Disable the dependee plugin
+        dependeePlugin.clickEnabledWidget();
         
-        // Now it should NOT be possible to change the enable state of the tasks plugin because one
-        // of the plugins it depends on (the CVS plugin) is not enabled.
-        tasksPlugin.assertNotEnabled();
-        tasksPlugin.assertEnabledStateNotChangeable();  // this is different to earlier
-        tasksPlugin.assertUninstallable();
+        // Now it should NOT be possible to change the enable state of the depender plugin because one
+        // of the plugins it depends on is not enabled.
+        mandatoryDependerPlugin.assertNotEnabled();
+        mandatoryDependerPlugin.assertEnabledStateNotChangeable();  // this is different to earlier
+        mandatoryDependerPlugin.assertUninstallable();
+        dependerPlugin.assertEnabled();
+
+        // You can disable a detached plugin if there is no explicit dependency on it.
+        matrixAuthPlugin.assertEnabled();
+        matrixAuthPlugin.assertEnabledStateChangeable();
+        matrixAuthPlugin.assertUninstallable();
+        matrixAuthPlugin.clickEnabledWidget();
+        matrixAuthPlugin.assertNotEnabled();
+        matrixAuthPlugin.assertEnabledStateChangeable();
+        matrixAuthPlugin.assertUninstallable();
     }
     
     private class InstalledPlugins {
@@ -146,7 +169,7 @@ public class PluginManagerInstalledGUITest {
                     return plugin;
                 }
             }
-            Assert.fail("Now pluginManager/installed row for plugin " + pluginId);
+            Assert.fail("No pluginManager/installed row for plugin " + pluginId);
             return null;
         }
 
@@ -219,11 +242,11 @@ public class PluginManagerInstalledGUITest {
         }
 
         public void assertHasDependents() {
-            Assert.assertTrue(hasDependents());
+            Assert.assertTrue("Plugin '" + getId() + "' is expected to have dependents.", hasDependents());
         }
 
         public void assertHasNoDependents() {
-            Assert.assertFalse(hasDependents());
+            Assert.assertFalse("Plugin '" + getId() + "' is expected to have no dependents.", hasDependents());
         }
 
         private boolean hasClassName(String className) {