Check no plugin with same pluginId is loaded (#287)

4 files changed, 130 insertions, 22 deletions

diff --git a/pf4j/pom.xml b/pf4j/pom.xml
index a1be7a4..061412d 100644
--- a/pf4j/pom.xml
+++ b/pf4j/pom.xml
@@ -60,6 +60,26 @@
             <version>${asm.version}</version>
             <optional>true</optional>
         </dependency>
+
+        <dependency>
+            <groupId>org.hamcrest</groupId>
+            <artifactId>hamcrest</artifactId>
+            <version>${hamcrest.version}</version>
+            <scope>test</scope>
+        </dependency>
+
+        <dependency>
+            <!-- An empty artifact, required while JUnit 4 is on the classpath to override its
+                 dependency on hamcrest.
+
+                 See http://hamcrest.org/JavaHamcrest/distributables#upgrading-from-hamcrest-1x
+                 -->
+            <groupId>org.hamcrest</groupId>
+            <artifactId>hamcrest-core</artifactId>
+            <version>${hamcrest.version}</version>
+            <scope>test</scope>
+        </dependency>
+
         <dependency>
             <groupId>junit</groupId>
             <artifactId>junit</artifactId>
diff --git a/pf4j/src/main/java/org/pf4j/AbstractPluginManager.java b/pf4j/src/main/java/org/pf4j/AbstractPluginManager.java
index 22cf7e3..c17fdab 100644
--- a/pf4j/src/main/java/org/pf4j/AbstractPluginManager.java
+++ b/pf4j/src/main/java/org/pf4j/AbstractPluginManager.java
@@ -15,10 +15,6 @@
  */
 package org.pf4j;
 
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
-import org.pf4j.util.StringUtils;
-
 import java.io.Closeable;
 import java.io.IOException;
 import java.nio.file.Files;
@@ -31,6 +27,9 @@ import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
+import org.pf4j.util.StringUtils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 /**
  * This class implements the boilerplate plugin code that any {@link PluginManager}
@@ -51,17 +50,17 @@ public abstract class AbstractPluginManager implements PluginManager {
 
     private PluginDescriptorFinder pluginDescriptorFinder;
 
-    /*
+    /**
      * A map of plugins this manager is responsible for (the key is the 'pluginId').
      */
     protected Map<String, PluginWrapper> plugins;
 
-    /*
+    /**
      * A map of plugin class loaders (the key is the 'pluginId').
      */
     private Map<String, ClassLoader> pluginClassLoaders;
 
-    /*
+    /**
      * A list with unresolved plugins (unresolved dependency).
      */
     private List<PluginWrapper> unresolvedPlugins;
@@ -71,23 +70,23 @@ public abstract class AbstractPluginManager implements PluginManager {
      */
     private List<PluginWrapper> resolvedPlugins;
 
-    /*
+    /**
      * A list with started plugins.
      */
     private List<PluginWrapper> startedPlugins;
 
-    /*
+    /**
      * The registered {@link PluginStateListener}s.
      */
     private List<PluginStateListener> pluginStateListeners;
 
-    /*
+    /**
      * Cache value for the runtime mode.
      * No need to re-read it because it wont change at runtime.
      */
     private RuntimeMode runtimeMode;
 
-    /*
+    /**
      * The system version used for comparisons to the plugin requires attribute.
      */
     private String systemVersion = "0.0.0";
@@ -822,18 +821,31 @@ public abstract class AbstractPluginManager implements PluginManager {
     }
 
     protected PluginWrapper loadPluginFromPath(Path pluginPath) throws PluginException {
-        // test for plugin duplication
+        // Test for plugin path duplication
         String pluginId = idForPath(pluginPath);
         if (pluginId != null) {
             throw new PluginAlreadyLoadedException(pluginId, pluginPath);
         }
 
-        // retrieves the plugin descriptor
+        // Retrieve and validate the plugin descriptor
         PluginDescriptorFinder pluginDescriptorFinder = getPluginDescriptorFinder();
         log.debug("Use '{}' to find plugins descriptors", pluginDescriptorFinder);
         log.debug("Finding plugin descriptor for plugin '{}'", pluginPath);
         PluginDescriptor pluginDescriptor = pluginDescriptorFinder.find(pluginPath);
         validatePluginDescriptor(pluginDescriptor);
+
+        // Check there are no loaded plugins with the retrieved id
+        pluginId = pluginDescriptor.getPluginId();
+        if (plugins.containsKey(pluginId)) {
+            PluginWrapper loadedPlugin = getPlugin(pluginId);
+            throw new PluginException("There is an already loaded plugin ({}) "
+                    + "with the same id ({}) as the plugin at path '{}'. Simultaneous loading "
+                    + "of plugins with the same PluginId is not currently supported.\n"
+                    + "As a workaround you may include PluginVersion and PluginProvider "
+                    + "in PluginId.",
+                loadedPlugin, pluginId, pluginPath);
+        }
+
         log.debug("Found descriptor {}", pluginDescriptor);
         String pluginClassName = pluginDescriptor.getPluginClass();
         log.debug("Class '{}' for plugin '{}'",  pluginClassName, pluginPath);
diff --git a/pf4j/src/test/java/org/pf4j/LoadPluginsTest.java b/pf4j/src/test/java/org/pf4j/LoadPluginsTest.java
index 8b6af11..c2917e8 100644
--- a/pf4j/src/test/java/org/pf4j/LoadPluginsTest.java
+++ b/pf4j/src/test/java/org/pf4j/LoadPluginsTest.java
@@ -15,20 +15,26 @@
  */
 package org.pf4j;
 
-import org.junit.Before;
-import org.junit.Rule;
-import org.junit.Test;
-import org.junit.rules.TemporaryFolder;
-import org.pf4j.plugin.PluginZip;
-
-import java.nio.file.Files;
-import java.nio.file.Paths;
-
+import static org.hamcrest.CoreMatchers.containsString;
+import static org.hamcrest.CoreMatchers.equalTo;
+import static org.hamcrest.CoreMatchers.startsWith;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertThat;
 import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
+
+import java.io.File;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.TemporaryFolder;
+import org.pf4j.plugin.PluginZip;
 
 public class LoadPluginsTest {
 
@@ -74,6 +80,75 @@ public class LoadPluginsTest {
     }
 
     @Test
+    public void loadPluginWithSameIdDifferentPathFails() throws Exception {
+        String pluginId = "myPlugin";
+        String pluginVersion = "1.2.3";
+        File plugin1Path = pluginsFolder.newFile("my-plugin-1.2.3.zip");
+        PluginZip plugin1 = new PluginZip.Builder(plugin1Path, pluginId)
+            .pluginVersion(pluginVersion)
+            .build();
+
+        File plugin2Path = pluginsFolder.newFile("my-plugin-1.2.3-renamed.zip");
+        PluginZip plugin2 = new PluginZip.Builder(plugin2Path, pluginId)
+            .pluginVersion(pluginVersion)
+            .build();
+
+        // Verify the first plugin with the given id is loaded
+        assertNotNull(pluginManager.loadPluginFromPath(plugin1.path()));
+        Path loadedPlugin1Path = pluginManager.getPlugin(pluginId).getPluginPath();
+
+        try {
+            // Verify the second plugin is not loaded as it has the same metadata
+            pluginManager.loadPluginFromPath(plugin2.path());
+            fail("Expected loadPluginFromPath to fail");
+        } catch (PluginException e) {
+            // Check the path of the loaded plugin remains the same
+            PluginWrapper loadedPlugin = pluginManager.getPlugin(pluginId);
+            assertThat(loadedPlugin.getPluginPath(), equalTo(loadedPlugin1Path));
+            // Check the message includes relevant information
+            String message = e.getMessage();
+            assertThat(message, startsWith("There is an already loaded plugin"));
+            assertThat(message, containsString(pluginId));
+            assertThat(message, containsString("my-plugin-1.2.3-renamed"));
+        }
+    }
+
+    /**
+     * This test verifies the behaviour as of PF4J 2.x, where plugins of different
+     * versions but with the pluginId cannot be loaded correctly because the API
+     * uses pluginId as the unique identifier of the loaded plugin.
+     */
+    @Test
+    public void loadPluginWithSameIdDifferentVersionsFails() throws Exception {
+        String pluginId = "myPlugin";
+        String plugin1Version = "1.2.3";
+        File plugin1Path = pluginsFolder.newFile("my-plugin-1.2.3.zip");
+        PluginZip plugin1 = new PluginZip.Builder(plugin1Path, pluginId)
+            .pluginVersion(plugin1Version)
+            .build();
+
+        String plugin2Version = "2.0.0";
+        File plugin2Path = pluginsFolder.newFile("my-plugin-2.0.0.zip");
+        PluginZip plugin2 = new PluginZip.Builder(plugin2Path, pluginId)
+            .pluginVersion(plugin2Version)
+            .build();
+
+        // Verify the first plugin with the given id is loaded
+        assertNotNull(pluginManager.loadPluginFromPath(plugin1.path()));
+        Path loadedPlugin1Path = pluginManager.getPlugin(pluginId).getPluginPath();
+        try {
+            // Verify the second plugin is not loaded as it has the same pluginId
+            pluginManager.loadPluginFromPath(plugin2.path());
+            fail("Expected loadPluginFromPath to fail");
+        } catch (PluginException e) {
+            // Check the path and version of the loaded plugin remain the same
+            PluginWrapper loadedPlugin = pluginManager.getPlugin(pluginId);
+            assertThat(loadedPlugin.getPluginPath(), equalTo(loadedPlugin1Path));
+            assertThat(loadedPlugin.getDescriptor().getVersion(), equalTo(plugin1Version));
+        }
+    }
+
+    @Test
     public void loadUnloadLoad() throws Exception {
         PluginZip pluginZip = new PluginZip.Builder(pluginsFolder.newFile("my-plugin-1.2.3.zip"), "myPlugin")
             .pluginVersion("1.2.3")
diff --git a/pom.xml b/pom.xml
index 6de3eef..dd40519 100644
--- a/pom.xml
+++ b/pom.xml
@@ -48,6 +48,7 @@
         <asm.version>7.0</asm.version>
 
         <junit.version>4.12</junit.version>
+        <hamcrest.version>2.1</hamcrest.version>
         <mockito.version>2.24.0</mockito.version>
         <cobertura.version>2.7</cobertura.version>
         <coveralls.version>3.1.0</coveralls.version>