From f1620f7f8aac25eda45d4a4084cefb015bd8c47a Mon Sep 17 00:00:00 2001 From: Decebal Suiu Date: Sun, 23 Jun 2024 13:17:41 +0300 Subject: [PATCH] Don't force resolve dependencies when unload a transitive plugin --- .../java/org/pf4j/AbstractPluginManager.java | 24 ++++++++-- .../org/pf4j/AbstractPluginManagerTest.java | 47 +++++++++++++++++-- 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 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(); + } + + } + } -- 2.39.5