From 2f8343cfda32e282a9a75f7e83cd6b05b8aea93a Mon Sep 17 00:00:00 2001 From: Lee David Painter Date: Thu, 20 Feb 2020 17:06:22 +0000 Subject: [PATCH] PluginClassLoader does not resolve classpath resources from plugin dependencies (#365) --- .../java/org/pf4j/AbstractPluginManager.java | 8 +- .../main/java/org/pf4j/PluginClassLoader.java | 83 ++++++++++-- .../java/org/pf4j/PluginClassLoaderTest.java | 127 +++++++++++++++++- 3 files changed, 199 insertions(+), 19 deletions(-) diff --git a/pf4j/src/main/java/org/pf4j/AbstractPluginManager.java b/pf4j/src/main/java/org/pf4j/AbstractPluginManager.java index d94b0c2..13ae937 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.pf4j.util.StringUtils; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; - import java.io.Closeable; import java.io.IOException; import java.nio.file.Files; @@ -32,6 +28,10 @@ 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} * implementation would have to support. diff --git a/pf4j/src/main/java/org/pf4j/PluginClassLoader.java b/pf4j/src/main/java/org/pf4j/PluginClassLoader.java index 755921a..8dc8415 100644 --- a/pf4j/src/main/java/org/pf4j/PluginClassLoader.java +++ b/pf4j/src/main/java/org/pf4j/PluginClassLoader.java @@ -15,17 +15,19 @@ */ package org.pf4j; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; - import java.io.File; import java.io.IOException; import java.net.URL; import java.net.URLClassLoader; import java.util.ArrayList; +import java.util.Collection; import java.util.Collections; import java.util.Enumeration; import java.util.List; +import java.util.Objects; + +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; /** * One instance of this class should be created by plugin manager for every available plug-in. @@ -175,8 +177,14 @@ public class PluginClassLoader extends URLClassLoader { log.trace("Found resource '{}' in plugin classpath", name); return url; } + + url = findResourceFromDependencies(name); + if (url != null) { + log.trace("Found resource '{}' in plugin dependencies", name); + return url; + } - log.trace("Couldn't find resource '{}' in plugin classpath. Delegating to parent", name); + log.trace("Couldn't find resource '{}' in plugin or dependencies classpath. Delegating to parent", name); return super.getResource(name); } else { @@ -188,25 +196,43 @@ public class PluginClassLoader extends URLClassLoader { log.trace("Couldn't find resource '{}' in parent", name); + url = findResourceFromDependencies(name); + + if (url != null) { + log.trace("Found resource '{}' in dependencies", name); + return url; + } + return findResource(name); } } @Override - public Enumeration getResources(String name) throws IOException { - if (!parentFirst) { - List resources = new ArrayList<>(); + public Enumeration getResources(String name) throws IOException { + List resources = new ArrayList<>(); + if (!parentFirst) { + resources.addAll(Collections.list(findResources(name))); + resources.addAll(findResourcesFromDependencies(name)); + if (getParent() != null) { resources.addAll(Collections.list(getParent().getResources(name))); } - return Collections.enumeration(resources); } else { - return super.getResources(name); + + if (getParent() != null) { + resources.addAll(Collections.list(getParent().getResources(name))); + } + + resources.addAll(findResourcesFromDependencies(name)); + + resources.addAll(Collections.list(super.findResources(name))); } + + return Collections.enumeration(resources); } private Class loadClassFromDependencies(String className) { @@ -216,7 +242,7 @@ public class PluginClassLoader extends URLClassLoader { ClassLoader classLoader = pluginManager.getPluginClassLoader(dependency.getPluginId()); // If the dependency is marked as optional, its class loader might not be available. - if (classLoader == null && dependency.isOptional()) { + if (classLoader == null || dependency.isOptional()) { continue; } @@ -229,5 +255,42 @@ public class PluginClassLoader extends URLClassLoader { return null; } + + private URL findResourceFromDependencies(String name) { + log.trace("Search in dependencies for resource '{}'", name); + List dependencies = pluginDescriptor.getDependencies(); + for (PluginDependency dependency : dependencies) { + PluginClassLoader classLoader = (PluginClassLoader) pluginManager.getPluginClassLoader(dependency.getPluginId()); + + // If the dependency is marked as optional, its class loader might not be available. + if (classLoader == null || dependency.isOptional()) { + continue; + } + + URL url = classLoader.findResource(name); + if (Objects.nonNull(url)) { + return url; + } + } + + return null; + } + + private Collection findResourcesFromDependencies(String name) throws IOException { + log.trace("Search in dependencies for resources '{}'", name); + List results = new ArrayList<>(); + List dependencies = pluginDescriptor.getDependencies(); + for (PluginDependency dependency : dependencies) { + PluginClassLoader classLoader = (PluginClassLoader) pluginManager.getPluginClassLoader(dependency.getPluginId()); + + // If the dependency is marked as optional, its class loader might not be available. + if (classLoader == null || dependency.isOptional()) { + continue; + } + results.addAll(Collections.list(classLoader.findResources(name))); + } + + return results; + } } diff --git a/pf4j/src/test/java/org/pf4j/PluginClassLoaderTest.java b/pf4j/src/test/java/org/pf4j/PluginClassLoaderTest.java index 64adbc1..b0316c7 100644 --- a/pf4j/src/test/java/org/pf4j/PluginClassLoaderTest.java +++ b/pf4j/src/test/java/org/pf4j/PluginClassLoaderTest.java @@ -42,11 +42,16 @@ import static org.junit.jupiter.api.Assertions.*; */ public class PluginClassLoaderTest { - private DefaultPluginManager pluginManager; + private TestPluginManager pluginManager; + private TestPluginManager pluginManagerParentFirst; + private DefaultPluginDescriptor pluginDependencyDescriptor; private DefaultPluginDescriptor pluginDescriptor; private PluginClassLoader parentLastPluginClassLoader; private PluginClassLoader parentFirstPluginClassLoader; + + private PluginClassLoader parentLastPluginDependencyClassLoader; + private PluginClassLoader parentFirstPluginDependencyClassLoader; @TempDir Path pluginsPath; @@ -62,6 +67,8 @@ public class PluginClassLoaderTest { } createFile(parentClassPathBase.resolve("META-INF").resolve("file-only-in-parent")); + createFile(parentClassPathBase.resolve("META-INF").resolve("file-in-both-parent-and-dependency-and-plugin")); + createFile(parentClassPathBase.resolve("META-INF").resolve("file-in-both-parent-and-dependency")); createFile(parentClassPathBase.resolve("META-INF").resolve("file-in-both-parent-and-plugin")); } @@ -69,7 +76,7 @@ public class PluginClassLoaderTest { File file = pathToFile.toFile(); file.deleteOnExit(); - assertTrue(file.createNewFile(), "failed to create '" + pathToFile + "'"); + assertTrue(file.exists() || file.createNewFile(), "failed to create '" + pathToFile + "'"); try (PrintWriter printWriter = new PrintWriter(file)) { printWriter.write("parent"); } @@ -77,13 +84,57 @@ public class PluginClassLoaderTest { @BeforeEach void setUp() throws IOException { - pluginManager = new DefaultPluginManager(pluginsPath); + pluginManager = new TestPluginManager(pluginsPath); + pluginManagerParentFirst = new TestPluginManager(pluginsPath); + + pluginDependencyDescriptor = new DefaultPluginDescriptor(); + pluginDependencyDescriptor.setPluginId("myDependency"); + pluginDependencyDescriptor.setPluginVersion("1.2.3"); + pluginDependencyDescriptor.setPluginDescription("My plugin"); + pluginDependencyDescriptor.setDependencies(""); + pluginDependencyDescriptor.setProvider("Me"); + pluginDependencyDescriptor.setRequires("5.0.0"); + + + Path pluginDependencyPath = pluginsPath.resolve(pluginDependencyDescriptor.getPluginId() + "-" + pluginDependencyDescriptor.getVersion() + ".zip"); + PluginZip pluginDependencyZip = new PluginZip.Builder(pluginDependencyPath, pluginDependencyDescriptor.getPluginId()) + .pluginVersion(pluginDependencyDescriptor.getVersion()) + .addFile(Paths.get("classes/META-INF/dependency-file"), "dependency") + .addFile(Paths.get("classes/META-INF/file-in-both-parent-and-dependency-and-plugin"), "dependency") + .addFile(Paths.get("classes/META-INF/file-in-both-parent-and-dependency"), "dependency") + .build(); + + FileUtils.expandIfZip(pluginDependencyZip.path()); + + PluginClasspath pluginDependencyClasspath = new DefaultPluginClasspath(); + + parentLastPluginDependencyClassLoader = new PluginClassLoader(pluginManager, pluginDependencyDescriptor, PluginClassLoaderTest.class.getClassLoader()); + parentFirstPluginDependencyClassLoader = new PluginClassLoader(pluginManagerParentFirst, pluginDependencyDescriptor, PluginClassLoaderTest.class.getClassLoader(), true); + + pluginManager.addClassLoader(pluginDependencyDescriptor.getPluginId(), parentLastPluginDependencyClassLoader); + pluginManagerParentFirst.addClassLoader(pluginDependencyDescriptor.getPluginId(), parentFirstPluginDependencyClassLoader); + + + for (String classesDirectory : pluginDependencyClasspath.getClassesDirectories()) { + File classesDirectoryFile = pluginDependencyZip.unzippedPath().resolve(classesDirectory).toFile(); + parentLastPluginDependencyClassLoader.addFile(classesDirectoryFile); + parentFirstPluginDependencyClassLoader.addFile(classesDirectoryFile); + } + for (String jarsDirectory : pluginDependencyClasspath.getJarsDirectories()) { + Path jarsDirectoryPath = pluginDependencyZip.unzippedPath().resolve(jarsDirectory); + List jars = FileUtils.getJars(jarsDirectoryPath); + for (File jar : jars) { + parentLastPluginDependencyClassLoader.addFile(jar); + parentFirstPluginDependencyClassLoader.addFile(jar); + } + } + pluginDescriptor = new DefaultPluginDescriptor(); pluginDescriptor.setPluginId("myPlugin"); pluginDescriptor.setPluginVersion("1.2.3"); pluginDescriptor.setPluginDescription("My plugin"); - pluginDescriptor.setDependencies("bar, baz"); + pluginDescriptor.setDependencies("myDependency"); pluginDescriptor.setProvider("Me"); pluginDescriptor.setRequires("5.0.0"); @@ -91,6 +142,7 @@ public class PluginClassLoaderTest { PluginZip pluginZip = new PluginZip.Builder(pluginPath, pluginDescriptor.getPluginId()) .pluginVersion(pluginDescriptor.getVersion()) .addFile(Paths.get("classes/META-INF/plugin-file"), "plugin") + .addFile(Paths.get("classes/META-INF/file-in-both-parent-and-dependency-and-plugin"), "plugin") .addFile(Paths.get("classes/META-INF/file-in-both-parent-and-plugin"), "plugin") .build(); @@ -101,6 +153,9 @@ public class PluginClassLoaderTest { parentLastPluginClassLoader = new PluginClassLoader(pluginManager, pluginDescriptor, PluginClassLoaderTest.class.getClassLoader()); parentFirstPluginClassLoader = new PluginClassLoader(pluginManager, pluginDescriptor, PluginClassLoaderTest.class.getClassLoader(), true); + pluginManager.addClassLoader(pluginDescriptor.getPluginId(), parentLastPluginClassLoader); + pluginManagerParentFirst.addClassLoader(pluginDescriptor.getPluginId(), parentFirstPluginClassLoader); + for (String classesDirectory : pluginClasspath.getClassesDirectories()) { File classesDirectoryFile = pluginZip.unzippedPath().resolve(classesDirectory).toFile(); parentLastPluginClassLoader.addFile(classesDirectoryFile); @@ -120,7 +175,10 @@ public class PluginClassLoaderTest { @AfterEach void tearDown() { pluginManager = null; + pluginDependencyDescriptor = null; pluginDescriptor = null; + parentLastPluginClassLoader = null; + parentFirstPluginClassLoader = null; } @Test @@ -138,7 +196,7 @@ public class PluginClassLoaderTest { URL resource = parentLastPluginClassLoader.getResource("META-INF/file-only-in-parent"); assertFirstLine("parent", resource); } - + @Test void parentFirstGetResourceExistsInParent() throws IOException, URISyntaxException { URL resource = parentFirstPluginClassLoader.getResource("META-INF/file-only-in-parent"); @@ -156,6 +214,18 @@ public class PluginClassLoaderTest { URL resource = parentFirstPluginClassLoader.getResource("META-INF/plugin-file"); assertFirstLine("plugin", resource); } + + @Test + void parentLastGetResourceExistsOnlyInDependnecy() throws IOException, URISyntaxException { + URL resource = parentLastPluginClassLoader.getResource("META-INF/dependency-file"); + assertFirstLine("dependency", resource); + } + + @Test + void parentFirstGetResourceExistsOnlyInDependency() throws IOException, URISyntaxException { + URL resource = parentFirstPluginClassLoader.getResource("META-INF/dependency-file"); + assertFirstLine("dependency", resource); + } @Test void parentLastGetResourceExistsInBothParentAndPlugin() throws URISyntaxException, IOException { @@ -168,6 +238,18 @@ public class PluginClassLoaderTest { URL resource = parentFirstPluginClassLoader.getResource("META-INF/file-in-both-parent-and-plugin"); assertFirstLine("parent", resource); } + + @Test + void parentLastGetResourceExistsInParentAndDependencyAndPlugin() throws URISyntaxException, IOException { + URL resource = parentLastPluginClassLoader.getResource("META-INF/file-in-both-parent-and-dependency-and-plugin"); + assertFirstLine("plugin", resource); + } + + @Test + void parentFirstGetResourceExistsInParentAndDependencyAndPlugin() throws URISyntaxException, IOException { + URL resource = parentFirstPluginClassLoader.getResource("META-INF/file-in-both-parent-and-dependency-and-plugin"); + assertFirstLine("parent", resource); + } @Test void parentLastGetResourcesNonExisting() throws IOException { @@ -191,6 +273,18 @@ public class PluginClassLoaderTest { assertNumberOfResourcesAndFirstLineOfFirstElement(1, "parent", resources); } + @Test + void parentLastGetResourcesExistsOnlyInDependency() throws IOException, URISyntaxException { + Enumeration resources = parentLastPluginClassLoader.getResources("META-INF/dependency-file"); + assertNumberOfResourcesAndFirstLineOfFirstElement(1, "dependency", resources); + } + + @Test + void parentFirstGetResourcesExistsOnlyInDependency() throws IOException, URISyntaxException { + Enumeration resources = parentFirstPluginClassLoader.getResources("META-INF/dependency-file"); + assertNumberOfResourcesAndFirstLineOfFirstElement(1, "dependency", resources); + } + @Test void parentLastGetResourcesExistsOnlyInPlugin() throws IOException, URISyntaxException { Enumeration resources = parentLastPluginClassLoader.getResources("META-INF/plugin-file"); @@ -214,6 +308,18 @@ public class PluginClassLoaderTest { Enumeration resources = parentFirstPluginClassLoader.getResources("META-INF/file-in-both-parent-and-plugin"); assertNumberOfResourcesAndFirstLineOfFirstElement(2, "parent", resources); } + + @Test + void parentLastGetResourcesExistsInParentAndDependencyAndPlugin() throws URISyntaxException, IOException { + Enumeration resources = parentLastPluginClassLoader.getResources("META-INF/file-in-both-parent-and-dependency-and-plugin"); + assertNumberOfResourcesAndFirstLineOfFirstElement(3, "plugin", resources); + } + + @Test + void parentFirstGetResourcesExistsInParentAndDependencyAndPlugin() throws URISyntaxException, IOException { + Enumeration resources = parentFirstPluginClassLoader.getResources("META-INF/file-in-both-parent-and-dependency-and-plugin"); + assertNumberOfResourcesAndFirstLineOfFirstElement(3, "parent", resources); + } private static void assertFirstLine(String expected, URL resource) throws URISyntaxException, IOException { assertNotNull(resource); @@ -227,4 +333,15 @@ public class PluginClassLoaderTest { URL firstResource = list.get(0); assertEquals(expectedFirstLine, Files.readAllLines(Paths.get(firstResource.toURI())).get(0)); } + + class TestPluginManager extends DefaultPluginManager { + + public TestPluginManager(Path pluginsPath) { + super(pluginsPath); + } + + void addClassLoader(String pluginId, PluginClassLoader classLoader) { + getPluginClassLoaders().put(pluginId, classLoader); + } + } } -- 2.39.5