Fix #520

pf4j/src/main/java/org/pf4j/AbstractPluginManager.java

@@ -271,6 +271,8 @@ public abstract class AbstractPluginManager implements PluginManager {
     }
 
     protected boolean unloadPlugin(String pluginId, boolean unloadDependents) {
+        checkPluginId(pluginId);
+
         try {
             if (unloadDependents) {
                 List<String> dependents = dependencyResolver.getDependents(pluginId);
@@ -451,18 +453,11 @@ public abstract class AbstractPluginManager implements PluginManager {
     protected PluginState stopPlugin(String pluginId, boolean stopDependents) {
         checkPluginId(pluginId);
 
-        PluginWrapper pluginWrapper = getPlugin(pluginId);
-        PluginDescriptor pluginDescriptor = pluginWrapper.getDescriptor();
-        PluginState pluginState = pluginWrapper.getPluginState();
-        if (PluginState.STOPPED == pluginState) {
-            log.debug("Already stopped plugin '{}'", getPluginLabel(pluginDescriptor));
-            return PluginState.STOPPED;
-        }
-
-        // test for disabled plugin
-        if (PluginState.DISABLED == pluginState) {
+        // test for started plugin
+        if (!checkPluginState(pluginId, PluginState.STARTED)) {
             // do nothing
-            return pluginState;
+            log.debug("Plugin '{}' is not started", getPluginLabel(pluginId));
+            return getPlugin(pluginId).getPluginState();
         }
 
         if (stopDependents) {
@@ -474,22 +469,33 @@ public abstract class AbstractPluginManager implements PluginManager {
             }
         }
 
-        log.info("Stop plugin '{}'", getPluginLabel(pluginDescriptor));
+        log.info("Stop plugin '{}'", getPluginLabel(pluginId));
+        PluginWrapper pluginWrapper = getPlugin(pluginId);
         pluginWrapper.getPlugin().stop();
         pluginWrapper.setPluginState(PluginState.STOPPED);
-        startedPlugins.remove(pluginWrapper);
+        getStartedPlugins().remove(pluginWrapper);
 
-        firePluginStateEvent(new PluginStateEvent(this, pluginWrapper, pluginState));
+        firePluginStateEvent(new PluginStateEvent(this, pluginWrapper, PluginState.STOPPED));
 
-        return pluginWrapper.getPluginState();
+        return PluginState.STOPPED;
     }
 
+    /**
+     * This method throws an {@link IllegalArgumentException} if there is no plugin with this id.
+     */
     protected void checkPluginId(String pluginId) {
         if (!plugins.containsKey(pluginId)) {
             throw new IllegalArgumentException(String.format("Unknown pluginId %s", pluginId));
         }
     }
 
+    /**
+     * This method returns {@code true} if the plugin state is equals with the value passed for parameter {@code pluginState}.
+     */
+    protected boolean checkPluginState(String pluginId, PluginState pluginState) {
+        return getPlugin(pluginId).getPluginState() == pluginState;
+    }
+
     @Override
     public boolean disablePlugin(String pluginId) {
         checkPluginId(pluginId);
@@ -879,15 +885,14 @@ public abstract class AbstractPluginManager implements PluginManager {
     }
 
     /**
-     * creates the plugin wrapper. override this if you want to prevent plugins having full access to the plugin manager
-     *
-     * @return
+     * Creates the plugin wrapper. override this if you want to prevent plugins having full access to the plugin manager.
      */
     protected PluginWrapper createPluginWrapper(PluginDescriptor pluginDescriptor, Path pluginPath, ClassLoader pluginClassLoader) {
         // create the plugin wrapper
         log.debug("Creating wrapper for plugin '{}'", pluginPath);
         PluginWrapper pluginWrapper = new PluginWrapper(this, pluginDescriptor, pluginPath, pluginClassLoader);
         pluginWrapper.setPluginFactory(getPluginFactory());
+
         return pluginWrapper;
     }
 
@@ -946,8 +951,12 @@ public abstract class AbstractPluginManager implements PluginManager {
         return versionManager;
     }
 
+    protected String getPluginLabel(String pluginId) {
+        return getPluginLabel(getPlugin(pluginId).getDescriptor());
+    }
+
     /**
-     * The plugin label is used in logging and it's a string in format {@code pluginId@pluginVersion}.
+     * The plugin label is used in logging, and it's a string in format {@code pluginId@pluginVersion}.
      */
     protected String getPluginLabel(PluginDescriptor pluginDescriptor) {
         return pluginDescriptor.getPluginId() + "@" + pluginDescriptor.getVersion();

pf4j/src/test/java/org/pf4j/AbstractPluginManagerTest.java

@@ -15,28 +15,50 @@
  */
 package org.pf4j;
 
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 import org.pf4j.test.TestExtension;
 import org.pf4j.test.TestExtensionPoint;
 
+import java.io.IOException;
+import java.nio.file.Paths;
 import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.List;
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertNotEquals;
+import static org.junit.jupiter.api.Assertions.assertSame;
+import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.mockito.Mockito.CALLS_REAL_METHODS;
+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.verify;
 import static org.mockito.Mockito.when;
+import static org.mockito.Mockito.withSettings;
 
 /**
  * @author Decebal Suiu
  */
-public class AbstractPluginManagerTest {
+class AbstractPluginManagerTest {
 
-    @Test
-    public void getExtensionsByType() {
-        AbstractPluginManager pluginManager = mock(AbstractPluginManager.class, CALLS_REAL_METHODS);
+    private AbstractPluginManager pluginManager;
+
+    @BeforeEach
+    void setUp() {
+        pluginManager = mock(AbstractPluginManager.class, withSettings().useConstructor().defaultAnswer(CALLS_REAL_METHODS));
+    }
 
+    @AfterEach
+    void tearDown() {
+        pluginManager = null;
+    }
+
+    @Test
+    void getExtensionsByType() {
         ExtensionFinder extensionFinder = mock(ExtensionFinder.class);
         List<ExtensionWrapper<TestExtensionPoint>> extensionList = new ArrayList<>(1);
         extensionList.add(new ExtensionWrapper<>(new ExtensionDescriptor(0, TestExtension.class), new DefaultExtensionFactory()));
@@ -46,11 +68,65 @@ public class AbstractPluginManagerTest {
         List<TestExtensionPoint> extensions = pluginManager.getExtensions(TestExtensionPoint.class);
         assertEquals(1, extensions.size());
     }
-    
+
     @Test
-    public void getVersion() {
-        AbstractPluginManager pluginManager = mock(AbstractPluginManager.class, CALLS_REAL_METHODS);
+    void getVersion() {
         assertNotEquals("0.0.0", pluginManager.getVersion());
     }
 
+    @Test
+    void stopStartedPlugin() throws IOException {
+        String pluginId = "plugin1";
+        PluginWrapper pluginWrapper = createPluginWrapper(pluginId);
+        pluginWrapper.setPluginState(PluginState.STARTED);
+
+        doReturn(pluginWrapper).when(pluginManager).getPlugin(pluginId);
+        doNothing().when(pluginManager).checkPluginId(pluginId);
+        doReturn(new ArrayList<>(Arrays.asList(pluginWrapper))).when(pluginManager).getStartedPlugins();
+
+        PluginState pluginState = pluginManager.stopPlugin(pluginId, false);
+        verify(pluginWrapper.getPlugin()).stop();
+        assertSame(PluginState.STOPPED, pluginState);
+    }
+
+    @Test
+    void stopCreatedPlugin() {
+        String pluginId = "plugin1";
+        PluginWrapper pluginWrapper = createPluginWrapper(pluginId);
+
+        doReturn(pluginWrapper).when(pluginManager).getPlugin(pluginId);
+        doNothing().when(pluginManager).checkPluginId(pluginId);
+        doReturn(new ArrayList<>(Arrays.asList(pluginWrapper))).when(pluginManager).getStartedPlugins();
+
+        PluginState pluginState = pluginManager.stopPlugin(pluginId, false);
+        verify(pluginWrapper.getPlugin(), never()).stop();
+        assertSame(PluginState.CREATED, pluginState);
+    }
+
+    @Test
+    void checkExistedPluginId() {
+        String pluginId = "plugin1";
+        PluginWrapper pluginWrapper = createPluginWrapper(pluginId);
+
+        pluginManager.plugins.put("plugin1", pluginWrapper);
+        pluginManager.checkPluginId("plugin1");
+    }
+
+    @Test
+    void checkNotExistedPluginId() {
+        assertThrows(IllegalArgumentException.class, () -> pluginManager.checkPluginId("plugin1"));
+    }
+
+    private PluginWrapper createPluginWrapper(String pluginId) {
+        DefaultPluginDescriptor pluginDescriptor = new DefaultPluginDescriptor();
+        pluginDescriptor.setPluginId(pluginId);
+        pluginDescriptor.setPluginVersion("1.2.3");
+
+        PluginWrapper pluginWrapper = new PluginWrapper(pluginManager, pluginDescriptor, Paths.get("plugin1"), getClass().getClassLoader());
+        Plugin plugin= mock(Plugin.class);
+        pluginWrapper.setPluginFactory(wrapper -> plugin);
+
+        return pluginWrapper;
+    }
+
 }