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/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<PluginDescription> sortedPlugins = PluginDependencyUtils.sortCandidates(found);
 
+    Set<String> loadedPluginsById = new HashSet<>();
     Map<PluginContainer, Module> 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);
       }
     }
 

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}

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<SerializedPluginDescription> 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<PluginDependency> 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
     );
   }

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<String> authors, Collection<PluginDependency> dependencies, Path source,
+      String mainClass) {
+    super(id, name, version, description, url, authors, dependencies, source);
+    this.mainClass = checkNotNull(mainClass);
+  }
+
+  String getMainClass() {
+    return mainClass;
+  }
+}

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<PluginDescription> sortCandidates(List<PluginDescription> candidates) {
+    List<PluginDescription> 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<PluginDescription> graph = GraphBuilder.directed()
         .allowsSelfLoops(false)
-        .expectedNodeCount(candidates.size())
+        .expectedNodeCount(sortedCandidates.size())
         .build();
-    Map<String, PluginDescription> candidateMap = Maps.uniqueIndex(candidates,
+    Map<String, PluginDescription> candidateMap = Maps.uniqueIndex(sortedCandidates,
         PluginDescription::getId);
 
-    for (PluginDescription description : candidates) {
+    for (PluginDescription description : sortedCandidates) {
       graph.addNode(description);
 
       for (PluginDependency dependency : description.getDependencies()) {

proxy/src/test/java/com/velocitypowered/proxy/plugin/util/PluginDependencyUtilsTest.java

@@ -69,8 +69,8 @@ class PluginDependencyUtilsTest {
   void sortCandidatesMultiplePluginsDependentOnOne() throws Exception {
     List<PluginDescription> plugins = ImmutableList.of(HAS_DEPENDENCY_3, HAS_DEPENDENCY_1,
         NO_DEPENDENCY);
-    List<PluginDescription> expected = ImmutableList.of(NO_DEPENDENCY, HAS_DEPENDENCY_3,
-        HAS_DEPENDENCY_1);
+    List<PluginDescription> expected = ImmutableList.of(NO_DEPENDENCY, HAS_DEPENDENCY_1,
+        HAS_DEPENDENCY_3);
     assertEquals(expected, PluginDependencyUtils.sortCandidates(plugins));
   }