Fix #576 (#579)

3 files changed, 101 insertions, 7 deletions

diff --git a/pf4j/src/main/java/org/pf4j/AbstractPluginManager.java b/pf4j/src/main/java/org/pf4j/AbstractPluginManager.java
index 286b624..b9ba5c5 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));
             }
         }
@@ -331,6 +343,11 @@ public abstract class AbstractPluginManager implements PluginManager {
             }
         }
 
+        // resolve the plugins again (update plugins graph)
+        if (resolveDependencies) {
+            resolveDependencies();
+        }
+
         return true;
     }
 
@@ -917,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
@@ -1114,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();
+        }
+
+    }
+
 }
diff --git a/pf4j/src/test/java/org/pf4j/DefaultPluginManagerTest.java b/pf4j/src/test/java/org/pf4j/DefaultPluginManagerTest.java
index d10689f..33f9db0 100644
--- a/pf4j/src/test/java/org/pf4j/DefaultPluginManagerTest.java
+++ b/pf4j/src/test/java/org/pf4j/DefaultPluginManagerTest.java
@@ -396,4 +396,40 @@ public class DefaultPluginManagerTest {
         assertThrows(PluginNotFoundException.class, () -> pluginManager.stopPlugin(pluginZip2.pluginId()));
     }
 
+    @Test
+    void deletePluginWithDependency() throws IOException {
+        PluginZip pluginZip1 = new PluginZip.Builder(pluginsPath.resolve("my-first-plugin-1.1.1.zip"), "myPlugin1")
+            .pluginVersion("1.1.1")
+            .build();
+
+        PluginZip pluginZip2 = new PluginZip.Builder(pluginsPath.resolve("my-second-plugin-2.2.2.zip"), "myPlugin2")
+            .pluginVersion("2.2.2")
+            .pluginDependencies("myPlugin1")
+            .build();
+
+        PluginZip pluginZip3 = new PluginZip.Builder(pluginsPath.resolve("my-third-plugin-3.3.3.zip"), "myPlugin3")
+            .pluginVersion("3.3.3")
+            .pluginDependencies("myPlugin2")
+            .build();
+
+        pluginManager.loadPlugins();
+        pluginManager.startPlugins();
+
+        assertEquals(PluginState.STARTED, pluginManager.getPlugin(pluginZip1.pluginId()).getPluginState());
+        assertEquals(PluginState.STARTED, pluginManager.getPlugin(pluginZip2.pluginId()).getPluginState());
+        assertEquals(PluginState.STARTED, pluginManager.getPlugin(pluginZip3.pluginId()).getPluginState());
+
+        System.out.println("Stopping " + pluginZip2.pluginId());
+        pluginManager.stopPlugin(pluginZip2.pluginId());
+        assertEquals(PluginState.STOPPED, pluginManager.getPlugin(pluginZip2.pluginId()).getPluginState());
+
+        boolean deleted = pluginManager.deletePlugin(pluginZip2.pluginId());
+        assertTrue(deleted);
+
+        assertEquals(1, pluginManager.getPlugins().size()); // myPlugin1 should still be there, myPlugin2 and myPlugin3 should be gone
+
+        pluginManager.stopPlugin(pluginZip1.pluginId());
+        assertEquals(PluginState.STOPPED, pluginManager.getPlugin(pluginZip1.pluginId()).getPluginState());
+    }
+
 }