From d4df01f414ddf73c5761a50a5769175194f68006 Mon Sep 17 00:00:00 2001 From: Decebal Suiu Date: Mon, 24 Jun 2024 14:40:14 +0300 Subject: [PATCH] Fix #576 (#579) --- .../java/org/pf4j/AbstractPluginManager.java | 25 +++++++++- .../org/pf4j/AbstractPluginManagerTest.java | 47 +++++++++++++++++-- .../org/pf4j/DefaultPluginManagerTest.java | 36 ++++++++++++++ 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 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()); + } + } -- 2.39.5