From 8a2674c5392de62a97e4d0b33e4dd94dbaae407d Mon Sep 17 00:00:00 2001 From: Decebal Suiu Date: Sat, 20 Apr 2019 13:50:58 +0300 Subject: [PATCH] Improve #292 --- .../java/org/pf4j/AbstractPluginManager.java | 63 +++++++------------ .../java/org/pf4j/BasePluginRepository.java | 4 +- .../org/pf4j/CompoundPluginRepository.java | 2 +- .../org/pf4j/DefaultPluginRepository.java | 2 +- .../org/pf4j/DefaultPluginStatusProvider.java | 32 ++++------ .../src/main/java/org/pf4j/PluginManager.java | 12 ++-- .../main/java/org/pf4j/PluginRepository.java | 2 +- .../java/org/pf4j/PluginStatusProvider.java | 6 +- .../org/pf4j/DefaultPluginRepositoryTest.java | 2 +- .../pf4j/DefaultPluginStatusProviderTest.java | 21 ++++--- .../org/pf4j/JarPluginRepositoryTest.java | 3 +- 11 files changed, 60 insertions(+), 89 deletions(-) diff --git a/pf4j/src/main/java/org/pf4j/AbstractPluginManager.java b/pf4j/src/main/java/org/pf4j/AbstractPluginManager.java index 3833588..4fa1d38 100644 --- a/pf4j/src/main/java/org/pf4j/AbstractPluginManager.java +++ b/pf4j/src/main/java/org/pf4j/AbstractPluginManager.java @@ -232,11 +232,11 @@ public abstract class AbstractPluginManager implements PluginManager { * Unload the specified plugin and it's dependents. */ @Override - public boolean unloadPlugin(String pluginId) { + public boolean unloadPlugin(String pluginId) throws PluginException { return unloadPlugin(pluginId, true); } - private boolean unloadPlugin(String pluginId, boolean unloadDependents) { + private boolean unloadPlugin(String pluginId, boolean unloadDependents) throws PluginException { try { if (unloadDependents) { List dependents = dependencyResolver.getDependents(pluginId); @@ -269,7 +269,7 @@ public abstract class AbstractPluginManager implements PluginManager { try { ((Closeable) classLoader).close(); } catch (IOException e) { - log.error("Cannot close classloader", e); + throw new PluginException("Cannot close classloader", e); } } } @@ -283,7 +283,7 @@ public abstract class AbstractPluginManager implements PluginManager { } @Override - public boolean deletePlugin(String pluginId) { + public boolean deletePlugin(String pluginId) throws PluginException { checkPluginId(pluginId); PluginWrapper pluginWrapper = getPlugin(pluginId); @@ -304,12 +304,7 @@ public abstract class AbstractPluginManager implements PluginManager { } // notify the plugin as it's deleted - try { - plugin.delete(); - } catch (PluginException e) { - log.error(e.getMessage(), e); - return false; - } + plugin.delete(); Path pluginPath = pluginWrapper.getPluginPath(); @@ -342,7 +337,7 @@ public abstract class AbstractPluginManager implements PluginManager { * Start the specified plugin and its dependencies. */ @Override - public PluginState startPlugin(String pluginId) { + public PluginState startPlugin(String pluginId) throws PluginException { checkPluginId(pluginId); PluginWrapper pluginWrapper = getPlugin(pluginId); @@ -369,16 +364,12 @@ public abstract class AbstractPluginManager implements PluginManager { startPlugin(dependency.getPluginId()); } - try { - log.info("Start plugin '{}'", getPluginLabel(pluginDescriptor)); - pluginWrapper.getPlugin().start(); - pluginWrapper.setPluginState(PluginState.STARTED); - startedPlugins.add(pluginWrapper); + log.info("Start plugin '{}'", getPluginLabel(pluginDescriptor)); + pluginWrapper.getPlugin().start(); + pluginWrapper.setPluginState(PluginState.STARTED); + startedPlugins.add(pluginWrapper); - firePluginStateEvent(new PluginStateEvent(this, pluginWrapper, pluginState)); - } catch (PluginException e) { - log.error(e.getMessage(), e); - } + firePluginStateEvent(new PluginStateEvent(this, pluginWrapper, pluginState)); return pluginWrapper.getPluginState(); } @@ -413,11 +404,11 @@ public abstract class AbstractPluginManager implements PluginManager { * Stop the specified plugin and it's dependents. */ @Override - public PluginState stopPlugin(String pluginId) { + public PluginState stopPlugin(String pluginId) throws PluginException { return stopPlugin(pluginId, true); } - private PluginState stopPlugin(String pluginId, boolean stopDependents) { + private PluginState stopPlugin(String pluginId, boolean stopDependents) throws PluginException { checkPluginId(pluginId); PluginWrapper pluginWrapper = getPlugin(pluginId); @@ -443,16 +434,12 @@ public abstract class AbstractPluginManager implements PluginManager { } } - try { - log.info("Stop plugin '{}'", getPluginLabel(pluginDescriptor)); - pluginWrapper.getPlugin().stop(); - pluginWrapper.setPluginState(PluginState.STOPPED); - startedPlugins.remove(pluginWrapper); + log.info("Stop plugin '{}'", getPluginLabel(pluginDescriptor)); + pluginWrapper.getPlugin().stop(); + pluginWrapper.setPluginState(PluginState.STOPPED); + startedPlugins.remove(pluginWrapper); - firePluginStateEvent(new PluginStateEvent(this, pluginWrapper, pluginState)); - } catch (PluginException e) { - log.error(e.getMessage(), e); - } + firePluginStateEvent(new PluginStateEvent(this, pluginWrapper, pluginState)); return pluginWrapper.getPluginState(); } @@ -464,7 +451,7 @@ public abstract class AbstractPluginManager implements PluginManager { } @Override - public boolean disablePlugin(String pluginId) { + public boolean disablePlugin(String pluginId) throws PluginException { checkPluginId(pluginId); PluginWrapper pluginWrapper = getPlugin(pluginId); @@ -480,10 +467,7 @@ public abstract class AbstractPluginManager implements PluginManager { firePluginStateEvent(new PluginStateEvent(this, pluginWrapper, PluginState.STOPPED)); - if (!pluginStatusProvider.disablePlugin(pluginId)) { - return false; - } - + pluginStatusProvider.disablePlugin(pluginId); log.info("Disabled plugin '{}'", getPluginLabel(pluginDescriptor)); return true; @@ -493,7 +477,7 @@ public abstract class AbstractPluginManager implements PluginManager { } @Override - public boolean enablePlugin(String pluginId) { + public boolean enablePlugin(String pluginId) throws PluginException { checkPluginId(pluginId); PluginWrapper pluginWrapper = getPlugin(pluginId); @@ -509,9 +493,7 @@ public abstract class AbstractPluginManager implements PluginManager { return true; } - if (!pluginStatusProvider.enablePlugin(pluginId)) { - return false; - } + pluginStatusProvider.enablePlugin(pluginId); pluginWrapper.setPluginState(PluginState.CREATED); @@ -612,7 +594,6 @@ public abstract class AbstractPluginManager implements PluginManager { return extensionFactory; } - // TODO remove public PluginLoader getPluginLoader() { return pluginLoader; } diff --git a/pf4j/src/main/java/org/pf4j/BasePluginRepository.java b/pf4j/src/main/java/org/pf4j/BasePluginRepository.java index aa152be..b083994 100644 --- a/pf4j/src/main/java/org/pf4j/BasePluginRepository.java +++ b/pf4j/src/main/java/org/pf4j/BasePluginRepository.java @@ -86,7 +86,7 @@ public class BasePluginRepository implements PluginRepository { } @Override - public boolean deletePluginPath(Path pluginPath) { + public boolean deletePluginPath(Path pluginPath) throws PluginException { if (!filter.accept(pluginPath.toFile())) { return false; } @@ -97,7 +97,7 @@ public class BasePluginRepository implements PluginRepository { } catch (NoSuchFileException e) { return false; // Return false on not found to be compatible with previous API (#135) } catch (IOException e) { - throw new RuntimeException(e); + throw new PluginException(e); } } diff --git a/pf4j/src/main/java/org/pf4j/CompoundPluginRepository.java b/pf4j/src/main/java/org/pf4j/CompoundPluginRepository.java index 705b912..c738191 100644 --- a/pf4j/src/main/java/org/pf4j/CompoundPluginRepository.java +++ b/pf4j/src/main/java/org/pf4j/CompoundPluginRepository.java @@ -48,7 +48,7 @@ public class CompoundPluginRepository implements PluginRepository { } @Override - public boolean deletePluginPath(Path pluginPath) { + public boolean deletePluginPath(Path pluginPath) throws PluginException { for (PluginRepository repository : repositories) { if (repository.deletePluginPath(pluginPath)) { return true; diff --git a/pf4j/src/main/java/org/pf4j/DefaultPluginRepository.java b/pf4j/src/main/java/org/pf4j/DefaultPluginRepository.java index da456eb..1c031b6 100644 --- a/pf4j/src/main/java/org/pf4j/DefaultPluginRepository.java +++ b/pf4j/src/main/java/org/pf4j/DefaultPluginRepository.java @@ -54,7 +54,7 @@ public class DefaultPluginRepository extends BasePluginRepository { } @Override - public boolean deletePluginPath(Path pluginPath) { + public boolean deletePluginPath(Path pluginPath) throws PluginException { FileUtils.optimisticDelete(FileUtils.findWithEnding(pluginPath, ".zip", ".ZIP", ".Zip")); return super.deletePluginPath(pluginPath); } diff --git a/pf4j/src/main/java/org/pf4j/DefaultPluginStatusProvider.java b/pf4j/src/main/java/org/pf4j/DefaultPluginStatusProvider.java index ca85407..823a29c 100644 --- a/pf4j/src/main/java/org/pf4j/DefaultPluginStatusProvider.java +++ b/pf4j/src/main/java/org/pf4j/DefaultPluginStatusProvider.java @@ -66,31 +66,23 @@ public class DefaultPluginStatusProvider implements PluginStatusProvider { } @Override - public boolean disablePlugin(String pluginId) { - if (disabledPlugins.add(pluginId)) { - try { - FileUtils.writeLines(disabledPlugins, pluginsRoot.resolve("disabled.txt").toFile()); - } catch (IOException e) { - log.error("Failed to disable plugin {}", pluginId, e); - return false; - } + public void disablePlugin(String pluginId) throws PluginException { + disabledPlugins.add(pluginId); + try { + FileUtils.writeLines(disabledPlugins, pluginsRoot.resolve("disabled.txt").toFile()); + } catch (IOException e) { + throw new PluginException(e); } - - return true; } @Override - public boolean enablePlugin(String pluginId) { - if (disabledPlugins.remove(pluginId)) { - try { - FileUtils.writeLines(disabledPlugins, pluginsRoot.resolve("disabled.txt").toFile()); - } catch (IOException e) { - log.error("Failed to enable plugin {}", pluginId, e); - return false; - } + public void enablePlugin(String pluginId) throws PluginException { + disabledPlugins.remove(pluginId); + try { + FileUtils.writeLines(disabledPlugins, pluginsRoot.resolve("disabled.txt").toFile()); + } catch (IOException e) { + throw new PluginException(e); } - - return true; } } diff --git a/pf4j/src/main/java/org/pf4j/PluginManager.java b/pf4j/src/main/java/org/pf4j/PluginManager.java index 11d3eac..a283738 100644 --- a/pf4j/src/main/java/org/pf4j/PluginManager.java +++ b/pf4j/src/main/java/org/pf4j/PluginManager.java @@ -85,7 +85,7 @@ public interface PluginManager { * * @return the plugin state */ - PluginState startPlugin(String pluginId); + PluginState startPlugin(String pluginId) throws PluginException; /** * Stop all active plugins. @@ -97,7 +97,7 @@ public interface PluginManager { * * @return the plugin state */ - PluginState stopPlugin(String pluginId); + PluginState stopPlugin(String pluginId) throws PluginException; /** * Unload a plugin. @@ -105,7 +105,7 @@ public interface PluginManager { * @param pluginId the unique plugin identifier, specified in its metadata * @return true if the plugin was unloaded */ - boolean unloadPlugin(String pluginId); + boolean unloadPlugin(String pluginId) throws PluginException; /** * Disables a plugin from being loaded. @@ -113,7 +113,7 @@ public interface PluginManager { * @param pluginId the unique plugin identifier, specified in its metadata * @return true if plugin is disabled */ - boolean disablePlugin(String pluginId); + boolean disablePlugin(String pluginId) throws PluginException; /** * Enables a plugin that has previously been disabled. @@ -121,7 +121,7 @@ public interface PluginManager { * @param pluginId the unique plugin identifier, specified in its metadata * @return true if plugin is enabled */ - boolean enablePlugin(String pluginId); + boolean enablePlugin(String pluginId) throws PluginException; /** * Deletes a plugin. @@ -129,7 +129,7 @@ public interface PluginManager { * @param pluginId the unique plugin identifier, specified in its metadata * @return true if the plugin was deleted */ - boolean deletePlugin(String pluginId); + boolean deletePlugin(String pluginId) throws PluginException; ClassLoader getPluginClassLoader(String pluginId); diff --git a/pf4j/src/main/java/org/pf4j/PluginRepository.java b/pf4j/src/main/java/org/pf4j/PluginRepository.java index d0b2125..941d68f 100644 --- a/pf4j/src/main/java/org/pf4j/PluginRepository.java +++ b/pf4j/src/main/java/org/pf4j/PluginRepository.java @@ -39,6 +39,6 @@ public interface PluginRepository { * @param pluginPath the plugin path * @return true if deleted */ - boolean deletePluginPath(Path pluginPath); + boolean deletePluginPath(Path pluginPath) throws PluginException; } diff --git a/pf4j/src/main/java/org/pf4j/PluginStatusProvider.java b/pf4j/src/main/java/org/pf4j/PluginStatusProvider.java index dda5f82..228575f 100644 --- a/pf4j/src/main/java/org/pf4j/PluginStatusProvider.java +++ b/pf4j/src/main/java/org/pf4j/PluginStatusProvider.java @@ -33,16 +33,14 @@ public interface PluginStatusProvider { * Disables a plugin from being loaded. * * @param pluginId the unique plugin identifier, specified in its metadata - * @return true if plugin is disabled */ - boolean disablePlugin(String pluginId); + void disablePlugin(String pluginId) throws PluginException; /** * Enables a plugin that has previously been disabled. * * @param pluginId the unique plugin identifier, specified in its metadata - * @return true if plugin is enabled */ - boolean enablePlugin(String pluginId); + void enablePlugin(String pluginId) throws PluginException; } diff --git a/pf4j/src/test/java/org/pf4j/DefaultPluginRepositoryTest.java b/pf4j/src/test/java/org/pf4j/DefaultPluginRepositoryTest.java index 3aaba8f..3805c42 100644 --- a/pf4j/src/test/java/org/pf4j/DefaultPluginRepositoryTest.java +++ b/pf4j/src/test/java/org/pf4j/DefaultPluginRepositoryTest.java @@ -85,7 +85,7 @@ public class DefaultPluginRepositoryTest { * Test of {@link DefaultPluginRepository#deletePluginPath(Path)} method. */ @Test - public void testDeletePluginPath() { + public void testDeletePluginPath() throws PluginException { PluginRepository repository = new DefaultPluginRepository(pluginsPath, false); assertTrue(Files.exists(pluginsPath.resolve("plugin-1.zip"))); diff --git a/pf4j/src/test/java/org/pf4j/DefaultPluginStatusProviderTest.java b/pf4j/src/test/java/org/pf4j/DefaultPluginStatusProviderTest.java index 635e41c..e766735 100644 --- a/pf4j/src/test/java/org/pf4j/DefaultPluginStatusProviderTest.java +++ b/pf4j/src/test/java/org/pf4j/DefaultPluginStatusProviderTest.java @@ -60,58 +60,59 @@ public class DefaultPluginStatusProviderTest { } @Test - public void testDisablePlugin() throws IOException { + public void testDisablePlugin() throws Exception { createEnabledFile(); createDisabledFile(); PluginStatusProvider statusProvider = new DefaultPluginStatusProvider(pluginsPath); + statusProvider.disablePlugin("plugin-1"); - assertTrue(statusProvider.disablePlugin("plugin-1")); assertTrue(statusProvider.isPluginDisabled("plugin-1")); assertTrue(statusProvider.isPluginDisabled("plugin-2")); assertTrue(statusProvider.isPluginDisabled("plugin-3")); } @Test - public void testDisablePluginWithEnableEmpty() throws IOException { + public void testDisablePluginWithEnableEmpty() throws Exception { createDisabledFile(); PluginStatusProvider statusProvider = new DefaultPluginStatusProvider(pluginsPath); + statusProvider.disablePlugin("plugin-1"); - assertTrue(statusProvider.disablePlugin("plugin-1")); assertTrue(statusProvider.isPluginDisabled("plugin-1")); assertTrue(statusProvider.isPluginDisabled("plugin-2")); assertFalse(statusProvider.isPluginDisabled("plugin-3")); } @Test - public void testEnablePlugin() throws IOException { + public void testEnablePlugin() throws Exception { createEnabledFile(); PluginStatusProvider statusProvider = new DefaultPluginStatusProvider(pluginsPath); + statusProvider.enablePlugin("plugin-2"); - assertTrue(statusProvider.enablePlugin("plugin-2")); assertFalse(statusProvider.isPluginDisabled("plugin-1")); assertFalse(statusProvider.isPluginDisabled("plugin-2")); assertTrue(statusProvider.isPluginDisabled("plugin-3")); } @Test - public void testEnablePluginWithEnableEmpty() { + public void testEnablePluginWithEnableEmpty() throws Exception{ PluginStatusProvider statusProvider = new DefaultPluginStatusProvider(pluginsPath); + statusProvider.enablePlugin("plugin-2"); - assertTrue(statusProvider.enablePlugin("plugin-2")); assertFalse(statusProvider.isPluginDisabled("plugin-1")); assertFalse(statusProvider.isPluginDisabled("plugin-2")); assertFalse(statusProvider.isPluginDisabled("plugin-3")); } @Test - public void testDisablePluginWithoutDisabledFile() { + public void testDisablePluginWithoutDisabledFile() throws Exception { PluginStatusProvider statusProvider = new DefaultPluginStatusProvider(pluginsPath); assertFalse(statusProvider.isPluginDisabled("plugin-1")); - assertTrue(statusProvider.disablePlugin("plugin-1")); + + statusProvider.disablePlugin("plugin-1"); assertTrue(statusProvider.isPluginDisabled("plugin-1")); } diff --git a/pf4j/src/test/java/org/pf4j/JarPluginRepositoryTest.java b/pf4j/src/test/java/org/pf4j/JarPluginRepositoryTest.java index 2260680..02d2bcc 100644 --- a/pf4j/src/test/java/org/pf4j/JarPluginRepositoryTest.java +++ b/pf4j/src/test/java/org/pf4j/JarPluginRepositoryTest.java @@ -18,7 +18,6 @@ package org.pf4j; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.io.TempDir; -import java.io.IOException; import java.nio.file.Files; import java.nio.file.Path; import java.util.List; @@ -39,7 +38,7 @@ public class JarPluginRepositoryTest { * Test of {@link JarPluginRepository#deletePluginPath(Path)} method. */ @Test - public void testDeletePluginPath() throws IOException { + public void testDeletePluginPath() throws Exception { PluginRepository repository = new JarPluginRepository(pluginsPath); Path plugin1Path = Files.createDirectory(pluginsPath.resolve("plugin-1")); -- 2.39.5