diff --git a/src/main/java/org/cryptomator/integrations/common/ClassLoaderFactory.java b/src/main/java/org/cryptomator/integrations/common/ClassLoaderFactory.java index 06abfb3..11fc9a6 100644 --- a/src/main/java/org/cryptomator/integrations/common/ClassLoaderFactory.java +++ b/src/main/java/org/cryptomator/integrations/common/ClassLoaderFactory.java @@ -1,6 +1,7 @@ package org.cryptomator.integrations.common; import org.jetbrains.annotations.Contract; +import org.jetbrains.annotations.Nullable; import org.jetbrains.annotations.VisibleForTesting; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -13,15 +14,37 @@ import java.nio.file.Files; import java.nio.file.Path; import java.util.Arrays; +import java.util.Objects; import java.util.stream.Collectors; class ClassLoaderFactory { private static final Logger LOG = LoggerFactory.getLogger(ClassLoaderFactory.class); - private static final String USER_HOME = System.getProperty("user.home"); private static final String PLUGIN_DIR_KEY = "cryptomator.pluginDir"; private static final String JAR_SUFFIX = ".jar"; + static Cache CACHE = null; + + record Cache(@Nullable String pluginDir, URLClassLoader classLoader) { + } + + /** + * Returns the cached class loader instance. + *

+ * The returned instance does not recheck the pluginDir for updates. + * If no instance is cached or the system property changed, creates a new instance with {@link #forPluginDir()} and caches it. + * + * @return The cached URLClassLoader that is aware of all {@code .jar} files in the plugin dir at the creation time of the instance + */ + @Contract(value = "-> _", pure = false) + public synchronized static URLClassLoader forPluginDirFromCache() { + String currentPluginDir = System.getProperty(PLUGIN_DIR_KEY); + if (CACHE == null || !Objects.equals(CACHE.pluginDir, currentPluginDir)) { + CACHE = new Cache(currentPluginDir, forPluginDirInternal(currentPluginDir)); + } + return CACHE.classLoader; + } + /** * Attempts to find {@code .jar} files in the path specified in {@value #PLUGIN_DIR_KEY} system property. * A new class loader instance is returned that loads classes from the given classes. @@ -30,21 +53,33 @@ class ClassLoaderFactory { */ @Contract(value = "-> new", pure = true) public static URLClassLoader forPluginDir() { - String val = System.getProperty(PLUGIN_DIR_KEY, ""); - final Path p; - if (val.startsWith("~/")) { - p = Path.of(USER_HOME).resolve(val.substring(2)); - } else { - p = Path.of(val); + String val = System.getProperty(PLUGIN_DIR_KEY); + return forPluginDirInternal(val); + } + + @VisibleForTesting + @Contract(value = "_ -> new", pure = true) + static URLClassLoader forPluginDirInternal(@Nullable String val) { + if (val == null) { + return URLClassLoader.newInstance(new URL[0]); + } + + try { + if (val.isBlank()) { + throw new IllegalArgumentException("Plugin dir path is blank"); + } + return forPluginDirWithPath(Path.of(val)); //Path.of might throw InvalidPathException + } catch (IllegalArgumentException e) { + LOG.debug("{} contains illegal value. Skipping plugin directory.", PLUGIN_DIR_KEY, e); + return URLClassLoader.newInstance(new URL[0]); } - return forPluginDirWithPath(p); } @VisibleForTesting @Contract(value = "_ -> new", pure = true) static URLClassLoader forPluginDirWithPath(Path path) throws UncheckedIOException { var jars = findJars(path); - if (LOG.isDebugEnabled()) { + if (LOG.isDebugEnabled() && jars.length != 0) { String jarList = Arrays.stream(jars).map(URL::getPath).collect(Collectors.joining(", ")); LOG.debug("Found jars in cryptomator.pluginDir: {}", jarList); } @@ -56,7 +91,7 @@ static URL[] findJars(Path path) { try (var stream = Files.walk(path)) { return stream.filter(ClassLoaderFactory::isJarFile).map(ClassLoaderFactory::toUrl).toArray(URL[]::new); } catch (IOException | UncheckedIOException e) { - // unable to locate any jars // TODO: log a warning? + LOG.debug("Failed to read plugin dir {}", path, e); return new URL[0]; } } diff --git a/src/main/java/org/cryptomator/integrations/common/IntegrationsLoader.java b/src/main/java/org/cryptomator/integrations/common/IntegrationsLoader.java index c0f5116..b863cb9 100644 --- a/src/main/java/org/cryptomator/integrations/common/IntegrationsLoader.java +++ b/src/main/java/org/cryptomator/integrations/common/IntegrationsLoader.java @@ -44,7 +44,7 @@ public static Optional load(Class clazz) { * @param Type of the service */ public static Optional loadSpecific(Class clazz, String implementationClassName) { - return ServiceLoader.load(clazz, ClassLoaderFactory.forPluginDir()).stream() + return ServiceLoader.load(clazz, ClassLoaderFactory.forPluginDirFromCache()).stream() .filter(provider -> provider.type().getName().equals(implementationClassName)) .map(ServiceLoader.Provider::get) .findAny(); @@ -61,7 +61,7 @@ public static Optional loadSpecific(Class clazz, String implementation * @return An ordered stream of all suited service providers */ public static Stream loadAll(Class clazz) { - return loadAll(ServiceLoader.load(clazz, ClassLoaderFactory.forPluginDir()), clazz); + return loadAll(ServiceLoader.load(clazz, ClassLoaderFactory.forPluginDirFromCache()), clazz); } /** diff --git a/src/test/java/org/cryptomator/integrations/common/ClassLoaderFactoryTest.java b/src/test/java/org/cryptomator/integrations/common/ClassLoaderFactoryTest.java index 2463966..4d38819 100644 --- a/src/test/java/org/cryptomator/integrations/common/ClassLoaderFactoryTest.java +++ b/src/test/java/org/cryptomator/integrations/common/ClassLoaderFactoryTest.java @@ -1,11 +1,17 @@ package org.cryptomator.integrations.common; +import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.DisplayName; import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.io.TempDir; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.EmptySource; +import org.junit.jupiter.params.provider.NullSource; +import org.junit.jupiter.params.provider.ValueSource; +import org.mockito.MockedStatic; import org.mockito.Mockito; import java.io.ByteArrayInputStream; @@ -17,6 +23,9 @@ import java.util.Arrays; import java.util.Comparator; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.never; + public class ClassLoaderFactoryTest { @Nested @@ -82,6 +91,7 @@ public void testReadPluginDirFromSysProp() { var absPath = "/there/will/be/plugins"; try (var mockedClass = Mockito.mockStatic(ClassLoaderFactory.class)) { mockedClass.when(() -> ClassLoaderFactory.forPluginDir()).thenCallRealMethod(); + mockedClass.when(() -> ClassLoaderFactory.forPluginDirInternal(any())).thenCallRealMethod(); mockedClass.when(() -> ClassLoaderFactory.forPluginDirWithPath(Path.of(absPath))).thenReturn(ucl); System.setProperty("cryptomator.pluginDir", absPath); @@ -91,21 +101,108 @@ public void testReadPluginDirFromSysProp() { } } - @Test - @DisplayName("read path from cryptomator.pluginDir and replace ~/ with user.home") - public void testReadPluginDirFromSysPropAndReplaceHome() { - var ucl = Mockito.mock(URLClassLoader.class, "ucl"); - var relPath = "~/there/will/be/plugins"; - var absPath = Path.of(System.getProperty("user.home")).resolve("there/will/be/plugins"); - try (var mockedClass = Mockito.mockStatic(ClassLoaderFactory.class)) { - mockedClass.when(() -> ClassLoaderFactory.forPluginDir()).thenCallRealMethod(); - mockedClass.when(() -> ClassLoaderFactory.forPluginDirWithPath(absPath)).thenReturn(ucl); + @Nested + @DisplayName("Method pluginDirInternal") + public class PluginDirInternal { + MockedStatic mockedClass; - System.setProperty("cryptomator.pluginDir", relPath); - var result = ClassLoaderFactory.forPluginDir(); + @BeforeEach + public void beforeEach() { + mockedClass = Mockito.mockStatic(ClassLoaderFactory.class); + mockedClass.when(() -> ClassLoaderFactory.forPluginDirInternal(any())).thenCallRealMethod(); + } + @AfterEach + public void afterEach() { + mockedClass.close(); + } + + @Test + @DisplayName("Valid path string calls forPluginDirWithPath") + public void testValidPathString() { + var ucl = Mockito.mock(URLClassLoader.class, "ucl"); + mockedClass.when(() -> ClassLoaderFactory.forPluginDirWithPath(any())).thenReturn(ucl); + + var result = ClassLoaderFactory.forPluginDirInternal("some/string"); + mockedClass.verify(() -> ClassLoaderFactory.forPluginDirWithPath(any())); Assertions.assertSame(ucl, result); } + + @ParameterizedTest + @DisplayName("Invalid or null path strings do not call forPluginDirWithPath") + @EmptySource + @ValueSource(strings = {"\t\t", " "}) + @NullSource + public void testInvalidPathString(String propValue) { + var result = ClassLoaderFactory.forPluginDirInternal(propValue); + + mockedClass.verify(() -> ClassLoaderFactory.forPluginDirWithPath(any()), never()); + Assertions.assertNotNull(result); + Assertions.assertEquals(0, result.getURLs().length); + } + + } + + @Nested + @DisplayName("Method pluginDirFromCache") + public class PluginDirFromCache { + + MockedStatic mockedClass; + + @BeforeEach + public void beforeEach() { + ClassLoaderFactory.CACHE = null; + System.clearProperty("cryptomator.pluginDir"); + mockedClass = Mockito.mockStatic(ClassLoaderFactory.class); + mockedClass.when(() -> ClassLoaderFactory.forPluginDirFromCache()).thenCallRealMethod(); + } + + @AfterEach + public void afterEach() { + mockedClass.close(); + ClassLoaderFactory.CACHE = null; + System.clearProperty("cryptomator.pluginDir"); + } + + @Test + @DisplayName("returns cached classloader on subsequent calls with same property") + public void testReturnsCachedInstance() { + var ucl = Mockito.mock(URLClassLoader.class, "ucl"); + mockedClass.when(() -> ClassLoaderFactory.forPluginDirInternal(any())).thenReturn(ucl); + + System.setProperty("cryptomator.pluginDir", "/some/path"); + var first = ClassLoaderFactory.forPluginDirFromCache(); + var second = ClassLoaderFactory.forPluginDirFromCache(); + + Assertions.assertSame(first, second); + mockedClass.verify(() -> ClassLoaderFactory.forPluginDirInternal("/some/path"), Mockito.times(1)); + } + + @Test + @DisplayName("creates new classloader when property changes") + public void testInvalidatesCacheOnPropertyChange() { + var ucl1 = Mockito.mock(URLClassLoader.class, "ucl1"); + var ucl2 = Mockito.mock(URLClassLoader.class, "ucl2"); + var ucl3 = Mockito.mock(URLClassLoader.class, "ucl3"); + mockedClass.when(() -> ClassLoaderFactory.forPluginDirInternal(any())).thenReturn(ucl1, ucl2, ucl3); + + System.setProperty("cryptomator.pluginDir", "/path/one"); + var first = ClassLoaderFactory.forPluginDirFromCache(); + + System.setProperty("cryptomator.pluginDir", "/path/two"); + var second = ClassLoaderFactory.forPluginDirFromCache(); + + System.clearProperty("cryptomator.pluginDir"); + var third = ClassLoaderFactory.forPluginDirFromCache(); + + Assertions.assertSame(ucl1, first); + Assertions.assertSame(ucl2, second); + Assertions.assertSame(ucl3, third); + Assertions.assertNotSame(first, second); + Assertions.assertNotSame(second, third); + Assertions.assertNotSame(first, third); + mockedClass.verify(() -> ClassLoaderFactory.forPluginDirInternal(any()), Mockito.times(3)); + } } @Test