From 37994449d76c6d9fcafa479bc84c673b91009e3d Mon Sep 17 00:00:00 2001 From: Andrew Steinborn Date: Sun, 16 Feb 2020 03:19:13 -0500 Subject: [PATCH] Fix regressions with plugin dependency loading from #272 This is a quick and dirty fix because it's late. I'll need to commit a better fix later. --- .../proxy/plugin/VelocityPluginManager.java | 26 ++++++------ .../proxy/plugin/loader/PluginLoader.java | 4 +- .../plugin/loader/java/JavaPluginLoader.java | 41 +++++++++++++++---- ...avaVelocityPluginDescriptionCandidate.java | 27 ++++++++++++ .../plugin/util/PluginDependencyUtils.java | 10 +++-- .../util/PluginDependencyUtilsTest.java | 4 +- 6 files changed, 86 insertions(+), 26 deletions(-) create mode 100644 proxy/src/main/java/com/velocitypowered/proxy/plugin/loader/java/JavaVelocityPluginDescriptionCandidate.java diff --git a/proxy/src/main/java/com/velocitypowered/proxy/plugin/VelocityPluginManager.java b/proxy/src/main/java/com/velocitypowered/proxy/plugin/VelocityPluginManager.java index f6e6712c4..b1a0c3aaa 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/plugin/VelocityPluginManager.java +++ b/proxy/src/main/java/com/velocitypowered/proxy/plugin/VelocityPluginManager.java @@ -16,6 +16,7 @@ import com.velocitypowered.api.plugin.meta.PluginDependency; import com.velocitypowered.api.proxy.ProxyServer; import com.velocitypowered.proxy.VelocityServer; import com.velocitypowered.proxy.plugin.loader.VelocityPluginContainer; +import com.velocitypowered.proxy.plugin.loader.VelocityPluginDescription; import com.velocitypowered.proxy.plugin.loader.java.JavaPluginLoader; import com.velocitypowered.proxy.plugin.util.PluginDependencyUtils; import java.io.IOException; @@ -25,12 +26,13 @@ import java.nio.file.Path; import java.util.ArrayList; import java.util.Collection; import java.util.Collections; -import java.util.Comparator; import java.util.HashMap; +import java.util.HashSet; import java.util.IdentityHashMap; import java.util.List; import java.util.Map; import java.util.Optional; +import java.util.Set; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; @@ -68,7 +70,7 @@ public class VelocityPluginManager implements PluginManager { .newDirectoryStream(directory, p -> p.toFile().isFile() && p.toString().endsWith(".jar"))) { for (Path path : stream) { try { - found.add(loader.loadPlugin(path)); + found.add(loader.loadPluginDescription(path)); } catch (Exception e) { logger.error("Unable to load plugin {}", path, e); } @@ -80,31 +82,29 @@ public class VelocityPluginManager implements PluginManager { return; } - // Sort the loaded plugins twice. First, sort the already-loaded plugins by their IDs, so as - // to make the topographic sort deterministic (since the order will differ depending on the - // first node chosen in the graph, which is the first plugin we found). Afterwards, we execute - // a depth-first search over the loaded plugins. - found.sort(Comparator.comparing(PluginDescription::getId)); List sortedPlugins = PluginDependencyUtils.sortCandidates(found); + Set loadedPluginsById = new HashSet<>(); Map pluginContainers = new HashMap<>(); // Now load the plugins pluginLoad: - for (PluginDescription plugin : sortedPlugins) { + for (PluginDescription candidate : sortedPlugins) { // Verify dependencies - for (PluginDependency dependency : plugin.getDependencies()) { - if (!dependency.isOptional() && !isLoaded(dependency.getId())) { - logger.error("Can't load plugin {} due to missing dependency {}", plugin.getId(), + for (PluginDependency dependency : candidate.getDependencies()) { + if (!dependency.isOptional() && !loadedPluginsById.contains(dependency.getId())) { + logger.error("Can't load plugin {} due to missing dependency {}", candidate.getId(), dependency.getId()); continue pluginLoad; } } try { - VelocityPluginContainer container = new VelocityPluginContainer(plugin); + PluginDescription realPlugin = loader.loadPlugin(candidate); + VelocityPluginContainer container = new VelocityPluginContainer(realPlugin); pluginContainers.put(container, loader.createModule(container)); + loadedPluginsById.add(realPlugin.getId()); } catch (Exception e) { - logger.error("Can't create module for plugin {}", plugin.getId(), e); + logger.error("Can't create module for plugin {}", candidate.getId(), e); } } diff --git a/proxy/src/main/java/com/velocitypowered/proxy/plugin/loader/PluginLoader.java b/proxy/src/main/java/com/velocitypowered/proxy/plugin/loader/PluginLoader.java index e261ee586..3ffdeb4b4 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/plugin/loader/PluginLoader.java +++ b/proxy/src/main/java/com/velocitypowered/proxy/plugin/loader/PluginLoader.java @@ -11,7 +11,9 @@ import java.nio.file.Path; */ public interface PluginLoader { - PluginDescription loadPlugin(Path source) throws Exception; + PluginDescription loadPluginDescription(Path source) throws Exception; + + PluginDescription loadPlugin(PluginDescription source) throws Exception; /** * Creates a {@link Module} for the provided {@link PluginContainer} diff --git a/proxy/src/main/java/com/velocitypowered/proxy/plugin/loader/java/JavaPluginLoader.java b/proxy/src/main/java/com/velocitypowered/proxy/plugin/loader/java/JavaPluginLoader.java index a87846006..17a075c23 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/plugin/loader/java/JavaPluginLoader.java +++ b/proxy/src/main/java/com/velocitypowered/proxy/plugin/loader/java/JavaPluginLoader.java @@ -38,7 +38,7 @@ public class JavaPluginLoader implements PluginLoader { } @Override - public PluginDescription loadPlugin(Path source) throws Exception { + public PluginDescription loadPluginDescription(Path source) throws Exception { Optional serialized = getSerializedPluginInfo(source); if (!serialized.isPresent()) { @@ -50,13 +50,23 @@ public class JavaPluginLoader implements PluginLoader { throw new InvalidPluginException("Plugin ID '" + pd.getId() + "' is invalid."); } + return createCandidateDescription(pd, source); + } + + @Override + public PluginDescription loadPlugin(PluginDescription source) throws Exception { + if (!(source instanceof JavaVelocityPluginDescriptionCandidate)) { + throw new IllegalArgumentException("Description provided isn't of the Java plugin loader"); + } PluginClassLoader loader = new PluginClassLoader( - new URL[]{source.toUri().toURL()} + new URL[]{source.getSource().get().toUri().toURL()} ); loader.addToClassloaders(); - Class mainClass = loader.loadClass(pd.getMain()); - return createDescription(pd, source, mainClass); + JavaVelocityPluginDescriptionCandidate candidate = + (JavaVelocityPluginDescriptionCandidate) source; + Class mainClass = loader.loadClass(candidate.getMainClass()); + return createDescription(candidate, mainClass); } @Override @@ -127,15 +137,16 @@ public class JavaPluginLoader implements PluginLoader { } } - private VelocityPluginDescription createDescription(SerializedPluginDescription description, - Path source, Class mainClass) { + private VelocityPluginDescription createCandidateDescription( + SerializedPluginDescription description, + Path source) { Set dependencies = new HashSet<>(); for (SerializedPluginDescription.Dependency dependency : description.getDependencies()) { dependencies.add(toDependencyMeta(dependency)); } - return new JavaVelocityPluginDescription( + return new JavaVelocityPluginDescriptionCandidate( description.getId(), description.getName(), description.getVersion(), @@ -144,6 +155,22 @@ public class JavaPluginLoader implements PluginLoader { description.getAuthors(), dependencies, source, + description.getMain() + ); + } + + private VelocityPluginDescription createDescription( + JavaVelocityPluginDescriptionCandidate description, + Class mainClass) { + return new JavaVelocityPluginDescription( + description.getId(), + description.getName().orElse(null), + description.getVersion().orElse(null), + description.getDescription().orElse(null), + description.getUrl().orElse(null), + description.getAuthors(), + description.getDependencies(), + description.getSource().orElse(null), mainClass ); } diff --git a/proxy/src/main/java/com/velocitypowered/proxy/plugin/loader/java/JavaVelocityPluginDescriptionCandidate.java b/proxy/src/main/java/com/velocitypowered/proxy/plugin/loader/java/JavaVelocityPluginDescriptionCandidate.java new file mode 100644 index 000000000..a5d6d0a1a --- /dev/null +++ b/proxy/src/main/java/com/velocitypowered/proxy/plugin/loader/java/JavaVelocityPluginDescriptionCandidate.java @@ -0,0 +1,27 @@ +package com.velocitypowered.proxy.plugin.loader.java; + +import static com.google.common.base.Preconditions.checkNotNull; + +import com.velocitypowered.api.plugin.meta.PluginDependency; +import com.velocitypowered.proxy.plugin.loader.VelocityPluginDescription; +import java.nio.file.Path; +import java.util.Collection; +import java.util.List; +import org.checkerframework.checker.nullness.qual.Nullable; + +class JavaVelocityPluginDescriptionCandidate extends VelocityPluginDescription { + + private final String mainClass; + + JavaVelocityPluginDescriptionCandidate(String id, @Nullable String name, @Nullable String version, + @Nullable String description, @Nullable String url, + @Nullable List authors, Collection dependencies, Path source, + String mainClass) { + super(id, name, version, description, url, authors, dependencies, source); + this.mainClass = checkNotNull(mainClass); + } + + String getMainClass() { + return mainClass; + } +} diff --git a/proxy/src/main/java/com/velocitypowered/proxy/plugin/util/PluginDependencyUtils.java b/proxy/src/main/java/com/velocitypowered/proxy/plugin/util/PluginDependencyUtils.java index 8bf4d1ff8..e09cc5bec 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/plugin/util/PluginDependencyUtils.java +++ b/proxy/src/main/java/com/velocitypowered/proxy/plugin/util/PluginDependencyUtils.java @@ -8,6 +8,7 @@ import com.velocitypowered.api.plugin.PluginDescription; import com.velocitypowered.api.plugin.meta.PluginDependency; import java.util.ArrayDeque; import java.util.ArrayList; +import java.util.Comparator; import java.util.Deque; import java.util.HashMap; import java.util.List; @@ -28,17 +29,20 @@ public class PluginDependencyUtils { * @throws IllegalStateException if there is a circular loop in the dependency graph */ public static List sortCandidates(List candidates) { + List sortedCandidates = new ArrayList<>(candidates); + sortedCandidates.sort(Comparator.comparing(PluginDescription::getId)); + // Create a graph and populate it with plugin dependencies. Specifically, each graph has plugin // nodes, and edges that represent the dependencies that plugin relies on. Non-existent plugins // are ignored. MutableGraph graph = GraphBuilder.directed() .allowsSelfLoops(false) - .expectedNodeCount(candidates.size()) + .expectedNodeCount(sortedCandidates.size()) .build(); - Map candidateMap = Maps.uniqueIndex(candidates, + Map candidateMap = Maps.uniqueIndex(sortedCandidates, PluginDescription::getId); - for (PluginDescription description : candidates) { + for (PluginDescription description : sortedCandidates) { graph.addNode(description); for (PluginDependency dependency : description.getDependencies()) { diff --git a/proxy/src/test/java/com/velocitypowered/proxy/plugin/util/PluginDependencyUtilsTest.java b/proxy/src/test/java/com/velocitypowered/proxy/plugin/util/PluginDependencyUtilsTest.java index 5709b2864..29fc31e7e 100644 --- a/proxy/src/test/java/com/velocitypowered/proxy/plugin/util/PluginDependencyUtilsTest.java +++ b/proxy/src/test/java/com/velocitypowered/proxy/plugin/util/PluginDependencyUtilsTest.java @@ -69,8 +69,8 @@ class PluginDependencyUtilsTest { void sortCandidatesMultiplePluginsDependentOnOne() throws Exception { List plugins = ImmutableList.of(HAS_DEPENDENCY_3, HAS_DEPENDENCY_1, NO_DEPENDENCY); - List expected = ImmutableList.of(NO_DEPENDENCY, HAS_DEPENDENCY_3, - HAS_DEPENDENCY_1); + List expected = ImmutableList.of(NO_DEPENDENCY, HAS_DEPENDENCY_1, + HAS_DEPENDENCY_3); assertEquals(expected, PluginDependencyUtils.sortCandidates(plugins)); }