Don't force resolve dependencies when unload a transitive plugin#576

2 files changed, 63 insertions, 8 deletions

diff --git a/pf4j/src/main/java/org/pf4j/AbstractPluginManager.java b/pf4j/src/main/java/org/pf4j/AbstractPluginManager.java
index d602abf..699a7fd 100644
--- a/pf4j/src/main/java/org/pf4j/AbstractPluginManager.java
+++ b/pf4j/src/main/java/org/pf4j/AbstractPluginManager.java
@@ -282,11 +282,23 @@ public abstract class AbstractPluginManager implements PluginManager {
      * @return true if the plugin was unloaded, otherwise false
      */
     protected boolean unloadPlugin(String pluginId, boolean unloadDependents) {
+        return unloadPlugin(pluginId, unloadDependents, true);
+    }
+
+    /**
+     * Unload the specified plugin and it's dependents.
+     *
+     * @param pluginId the pluginId of the plugin to unload
+     * @param unloadDependents if true, unload dependents
+     * @param resolveDependencies if true, resolve dependencies
+     * @return true if the plugin was unloaded, otherwise false
+     */
+    protected boolean unloadPlugin(String pluginId, boolean unloadDependents, boolean resolveDependencies) {
         if (unloadDependents) {
             List<String> dependents = dependencyResolver.getDependents(pluginId);
             while (!dependents.isEmpty()) {
                 String dependent = dependents.remove(0);
-                unloadPlugin(dependent, false);
+                unloadPlugin(dependent, false, false);
                 dependents.addAll(0, dependencyResolver.getDependents(dependent));
             }
         }
@@ -332,7 +344,9 @@ public abstract class AbstractPluginManager implements PluginManager {
         }
 
         // resolve the plugins again (update plugins graph)
-        resolveDependencies();
+        if (resolveDependencies) {
+            resolveDependencies();
+        }
 
         return true;
     }
@@ -920,7 +934,7 @@ public abstract class AbstractPluginManager implements PluginManager {
         pluginId = pluginDescriptor.getPluginId();
 
         // add plugin to the list with plugins
-        plugins.put(pluginId, pluginWrapper);
+        addPlugin(pluginWrapper);
         getUnresolvedPlugins().add(pluginWrapper);
 
         // add plugin class loader to the list with class loaders
@@ -1117,6 +1131,10 @@ public abstract class AbstractPluginManager implements PluginManager {
         this.resolveRecoveryStrategy = resolveRecoveryStrategy;
     }
 
+    void addPlugin(PluginWrapper pluginWrapper) {
+        plugins.put(pluginWrapper.getPluginId(), pluginWrapper);
+    }
+
     /**
      * Strategy for handling the recovery of a plugin that could not be resolved
      * (loaded) due to a dependency problem.
diff --git a/pf4j/src/test/java/org/pf4j/AbstractPluginManagerTest.java b/pf4j/src/test/java/org/pf4j/AbstractPluginManagerTest.java
index 07a083b..88be865 100644
--- a/pf4j/src/test/java/org/pf4j/AbstractPluginManagerTest.java
+++ b/pf4j/src/test/java/org/pf4j/AbstractPluginManagerTest.java
@@ -36,6 +36,8 @@ import static org.mockito.Mockito.doNothing;
 import static org.mockito.Mockito.doReturn;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.reset;
+import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 import static org.mockito.Mockito.withSettings;
@@ -49,7 +51,7 @@ public class AbstractPluginManagerTest {
 
     @BeforeEach
     void setUp() {
-        pluginManager = mock(AbstractPluginManager.class, withSettings()
+        pluginManager = mock(AbstractPluginManagerWithDefaultVersionManager.class, withSettings()
             .useConstructor()
             .defaultAnswer(CALLS_REAL_METHODS));
     }
@@ -119,11 +121,33 @@ public class AbstractPluginManagerTest {
         assertThrows(PluginNotFoundException.class, () -> pluginManager.checkPluginId("plugin1"));
     }
 
-    private PluginWrapper createPluginWrapper(String pluginId) {
-        DefaultPluginDescriptor pluginDescriptor = new DefaultPluginDescriptor();
-        pluginDescriptor.setPluginId(pluginId);
-        pluginDescriptor.setPluginVersion("1.2.3");
+    @Test
+    void unloadPluginCallsResolveDependenciesOnce() {
+        PluginWrapper pluginWrapper1 = createPluginWrapper("plugin1", "plugin2");
+        PluginWrapper pluginWrapper2 = createPluginWrapper("plugin2");
+
+        pluginManager.addPlugin(pluginWrapper1);
+        pluginManager.addPlugin(pluginWrapper2);
+        pluginManager.resolveDependencies();
+
+        // reset the mock to not count the explicit call of resolveDependencies
+        reset(pluginManager);
+
+        pluginManager.unloadPlugin("plugin2", true);
+
+        verify(pluginManager, times(1)).resolveDependencies();
+    }
+
+    private PluginWrapper createPluginWrapper(String pluginId, String... dependencies) {
+        PluginDescriptor pluginDescriptor = new DefaultPluginDescriptor()
+            .setPluginId(pluginId)
+            .setPluginVersion("1.2.3")
+            .setDependencies(String.join(", ", dependencies));
 
+        return createPluginWrapper(pluginDescriptor);
+    }
+
+    private PluginWrapper createPluginWrapper(PluginDescriptor pluginDescriptor) {
         PluginWrapper pluginWrapper = new PluginWrapper(pluginManager, pluginDescriptor, Paths.get("plugin1"), getClass().getClassLoader());
         Plugin plugin = mock(Plugin.class);
         pluginWrapper.setPluginFactory(wrapper -> plugin);
@@ -131,4 +155,17 @@ public class AbstractPluginManagerTest {
         return pluginWrapper;
     }
 
+    static abstract class AbstractPluginManagerWithDefaultVersionManager extends AbstractPluginManager {
+
+        public AbstractPluginManagerWithDefaultVersionManager() {
+            super();
+        }
+
+        @Override
+        protected VersionManager createVersionManager() {
+            return new DefaultVersionManager();
+        }
+
+    }
+
 }