Refactor validation of PluginDescriptors (#130)

6 files changed, 83 insertions, 87 deletions

diff --git a/pf4j/src/main/java/ro/fortsoft/pf4j/AbstractPluginManager.java b/pf4j/src/main/java/ro/fortsoft/pf4j/AbstractPluginManager.java
index 6087ec4..bd7c921 100644
--- a/pf4j/src/main/java/ro/fortsoft/pf4j/AbstractPluginManager.java
+++ b/pf4j/src/main/java/ro/fortsoft/pf4j/AbstractPluginManager.java
@@ -19,19 +19,14 @@ import com.github.zafarkhaja.semver.Version;
 import com.github.zafarkhaja.semver.expr.Expression;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
+import ro.fortsoft.pf4j.util.StringUtils;
 
 import java.io.Closeable;
 import java.io.IOException;
 import java.nio.file.Files;
 import java.nio.file.Path;
 import java.nio.file.Paths;
-import java.util.ArrayList;
-import java.util.Collections;
-import java.util.HashMap;
-import java.util.Iterator;
-import java.util.List;
-import java.util.Map;
-import java.util.Set;
+import java.util.*;
 
 /**
  * This class implements the boilerplate plugin code that any {@link PluginManager}
@@ -751,6 +746,7 @@ public abstract class AbstractPluginManager implements PluginManager {
         // retrieves the plugin descriptor
         log.debug("Find plugin descriptor '{}'", pluginPath);
         PluginDescriptor pluginDescriptor = getPluginDescriptorFinder().find(pluginPath);
+        validatePluginDescriptor(pluginDescriptor);
         log.debug("Descriptor {}", pluginDescriptor);
         String pluginClassName = pluginDescriptor.getPluginClass();
         log.debug("Class '{}' for plugin '{}'",  pluginClassName, pluginPath);
@@ -792,6 +788,23 @@ public abstract class AbstractPluginManager implements PluginManager {
         return pluginWrapper;
     }
 
+    /**
+     * Override this to change the validation criteria
+     * @param descriptor the plugin descriptor to validate
+     * @throws PluginException if validation fails
+     */
+    protected void validatePluginDescriptor(PluginDescriptor descriptor) throws PluginException {
+        if (StringUtils.isEmpty(descriptor.getPluginId())) {
+            throw new PluginException("id cannot be empty");
+        }
+        if (StringUtils.isEmpty(descriptor.getPluginClass())) {
+            throw new PluginException("class cannot be empty");
+        }
+        if (descriptor.getVersion() == null) {
+            throw new PluginException("version cannot be empty");
+        }
+    }
+
     // TODO add this method in PluginManager as default method for Java 8.
     protected boolean isDevelopment() {
         return RuntimeMode.DEVELOPMENT.equals(getRuntimeMode());
diff --git a/pf4j/src/main/java/ro/fortsoft/pf4j/ManifestPluginDescriptorFinder.java b/pf4j/src/main/java/ro/fortsoft/pf4j/ManifestPluginDescriptorFinder.java
index f9ef927..de0f84e 100644
--- a/pf4j/src/main/java/ro/fortsoft/pf4j/ManifestPluginDescriptorFinder.java
+++ b/pf4j/src/main/java/ro/fortsoft/pf4j/ManifestPluginDescriptorFinder.java
@@ -33,10 +33,7 @@ public abstract class ManifestPluginDescriptorFinder implements PluginDescriptor
 	public PluginDescriptor find(Path pluginPath) throws PluginException {
         Manifest manifest = readManifest(pluginPath);
 
-        PluginDescriptor pluginDescriptor = createPluginDescriptor(manifest);
-        validatePluginDescriptor(pluginDescriptor);
-
-		return pluginDescriptor;
+        return createPluginDescriptor(manifest);
 	}
 
     public abstract Manifest readManifest(Path pluginPath) throws PluginException;
@@ -82,17 +79,4 @@ public abstract class ManifestPluginDescriptorFinder implements PluginDescriptor
     protected PluginDescriptor createPluginDescriptorInstance() {
         return new PluginDescriptor();
     }
-
-    protected void validatePluginDescriptor(PluginDescriptor pluginDescriptor) throws PluginException {
-        if (StringUtils.isEmpty(pluginDescriptor.getPluginId())) {
-            throw new PluginException("Plugin-Id cannot be empty");
-        }
-        if (StringUtils.isEmpty(pluginDescriptor.getPluginClass())) {
-            throw new PluginException("Plugin-Class cannot be empty");
-        }
-        if (pluginDescriptor.getVersion() == null) {
-            throw new PluginException("Plugin-Version cannot be empty");
-        }
-    }
-
 }
diff --git a/pf4j/src/main/java/ro/fortsoft/pf4j/PropertiesPluginDescriptorFinder.java b/pf4j/src/main/java/ro/fortsoft/pf4j/PropertiesPluginDescriptorFinder.java
index ee47562..8af4269 100644
--- a/pf4j/src/main/java/ro/fortsoft/pf4j/PropertiesPluginDescriptorFinder.java
+++ b/pf4j/src/main/java/ro/fortsoft/pf4j/PropertiesPluginDescriptorFinder.java
@@ -45,17 +45,14 @@ public class PropertiesPluginDescriptorFinder implements PluginDescriptorFinder
 	}
 
 	public PropertiesPluginDescriptorFinder(String propertiesFileName) {
-		this.propertiesFileName = propertiesFileName;
+        this.propertiesFileName = propertiesFileName;
 	}
 
 	@Override
 	public PluginDescriptor find(Path pluginPath) throws PluginException {
         Properties properties = readProperties(pluginPath);
 
-        PluginDescriptor pluginDescriptor = createPluginDescriptor(properties);
-        validatePluginDescriptor(pluginDescriptor);
-
-        return pluginDescriptor;
+        return createPluginDescriptor(properties);
 	}
 
     protected Properties readProperties(Path pluginPath) throws PluginException {
@@ -116,17 +113,4 @@ public class PropertiesPluginDescriptorFinder implements PluginDescriptorFinder
     protected PluginDescriptor createPluginDescriptorInstance() {
         return new PluginDescriptor();
     }
-
-    protected void validatePluginDescriptor(PluginDescriptor pluginDescriptor) throws PluginException {
-        if (StringUtils.isEmpty(pluginDescriptor.getPluginId())) {
-            throw new PluginException("plugin.id cannot be empty");
-        }
-        if (StringUtils.isEmpty(pluginDescriptor.getPluginClass())) {
-            throw new PluginException("plugin.class cannot be empty");
-        }
-        if (pluginDescriptor.getVersion() == null) {
-            throw new PluginException("plugin.version cannot be empty");
-        }
-    }
-
 }
diff --git a/pf4j/src/test/java/ro/fortsoft/pf4j/DefaultPluginManagerTest.java b/pf4j/src/test/java/ro/fortsoft/pf4j/DefaultPluginManagerTest.java
new file mode 100644
index 0000000..cde124b
--- /dev/null
+++ b/pf4j/src/test/java/ro/fortsoft/pf4j/DefaultPluginManagerTest.java
@@ -0,0 +1,60 @@
+/*
+ * Copyright 2015 Decebal Suiu
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package ro.fortsoft.pf4j;
+
+import com.github.zafarkhaja.semver.Version;
+import org.junit.Before;
+import org.junit.Test;
+
+public class DefaultPluginManagerTest {
+    private PluginDescriptor pd1 = null;
+    private DefaultPluginManager pluginManager = new DefaultPluginManager();
+
+    @Before
+    public void init() {
+        pd1 = new PluginDescriptor();
+        pd1.setPluginId("myPlugin");
+        pd1.setPluginVersion(Version.valueOf("1.2.3"));
+        pd1.setPluginClass("foo");
+        pd1.setPluginDescription("My plugin");
+        pd1.setDependencies("bar, baz");
+        pd1.setProvider("Me");
+        pd1.setRequires("5.0.0");
+    }
+
+    @Test
+    public void validateOK() throws PluginException {
+        pluginManager.validatePluginDescriptor(pd1);
+    }
+
+    @Test(expected = PluginException.class)
+    public void validateFailsOnId() throws PluginException {
+        pd1.setPluginId("");
+        pluginManager.validatePluginDescriptor(pd1);
+    }
+
+    @Test(expected = PluginException.class)
+    public void validateFailsOnVersion() throws PluginException {
+        pd1.setPluginVersion(null);
+        pluginManager.validatePluginDescriptor(pd1);
+    }
+
+    @Test(expected = PluginException.class)
+    public void validateFailsOnClass() throws PluginException {
+        pd1.setPluginClass(null);
+        pluginManager.validatePluginDescriptor(pd1);
+    }
+}
diff --git a/pf4j/src/test/java/ro/fortsoft/pf4j/ManifestPluginDescriptorFinderTest.java b/pf4j/src/test/java/ro/fortsoft/pf4j/ManifestPluginDescriptorFinderTest.java
index 1ccaa22..b76b50e 100644
--- a/pf4j/src/test/java/ro/fortsoft/pf4j/ManifestPluginDescriptorFinderTest.java
+++ b/pf4j/src/test/java/ro/fortsoft/pf4j/ManifestPluginDescriptorFinderTest.java
@@ -111,33 +111,6 @@ public class ManifestPluginDescriptorFinderTest {
         instance.find(getPluginsRoot().resolve("test-plugin-3"));
     }
 
-    /**
-     * Test of {@link DefaultPluginDescriptorFinder#find(Path)} method.
-     */
-    @Test(expected = PluginException.class)
-    public void testFindMissingPluginClass() throws Exception {
-        PluginDescriptorFinder instance = new DefaultPluginDescriptorFinder(new DefaultPluginClasspath());
-        instance.find(getPluginsRoot().resolve("test-plugin-4"));
-    }
-
-    /**
-     * Test of {@link DefaultPluginDescriptorFinder#find(Path)} method.
-     */
-    @Test(expected = PluginException.class)
-    public void testFindMissingPluginVersion() throws Exception {
-        PluginDescriptorFinder instance = new DefaultPluginDescriptorFinder(new DefaultPluginClasspath());
-        instance.find(getPluginsRoot().resolve("test-plugin-5"));
-    }
-
-    /**
-     * Test of {@link DefaultPluginDescriptorFinder#find(Path)} method.
-     */
-    @Test(expected = PluginException.class)
-    public void testFindMissingPluginId() throws Exception {
-        PluginDescriptorFinder instance = new DefaultPluginDescriptorFinder(new DefaultPluginClasspath());
-        instance.find(getPluginsRoot().resolve("test-plugin-6"));
-    }
-
     private List<String> getPlugin1Manifest() {
         String[] lines = new String[] {
             "Manifest-Version: 1.0\n"
diff --git a/pf4j/src/test/java/ro/fortsoft/pf4j/PropertiesPluginDescriptorFinderTest.java b/pf4j/src/test/java/ro/fortsoft/pf4j/PropertiesPluginDescriptorFinderTest.java
index 9afb636..ca62770 100644
--- a/pf4j/src/test/java/ro/fortsoft/pf4j/PropertiesPluginDescriptorFinderTest.java
+++ b/pf4j/src/test/java/ro/fortsoft/pf4j/PropertiesPluginDescriptorFinderTest.java
@@ -96,24 +96,6 @@ public class PropertiesPluginDescriptorFinderTest {
         instance.find(getPluginsRoot().resolve("test-plugin-3"));
     }
 
-    @Test(expected = PluginException.class)
-    public void testFindMissingPluginClass() throws Exception {
-        PluginDescriptorFinder instance = new DefaultPluginDescriptorFinder(new DefaultPluginClasspath());
-        instance.find(getPluginsRoot().resolve("test-plugin-4"));
-    }
-
-    @Test(expected = PluginException.class)
-    public void testFindMissingPluginVersion() throws Exception {
-        PluginDescriptorFinder instance = new DefaultPluginDescriptorFinder(new DefaultPluginClasspath());
-        instance.find(getPluginsRoot().resolve("test-plugin-5"));
-    }
-
-    @Test(expected = PluginException.class)
-    public void testFindMissingPluginId() throws Exception {
-        PluginDescriptorFinder instance = new DefaultPluginDescriptorFinder(new DefaultPluginClasspath());
-        instance.find(getPluginsRoot().resolve("test-plugin-6"));
-    }
-
     private List<String> getPlugin1Properties() {
         String[] lines = new String[] {
             "plugin.id=test-plugin-1\n"