Rewrite dependency plugin resolving.

Velocity now resolves plugin dependencies using a depth-first search
(DFS), instead of using Kahn's algorithm like before. This properly handles duplicate indirect dependencies.

Thanks to @creeper123123321 for noticing this issue!

proxy/src/main/java/com/velocitypowered/proxy/plugin/util/PluginDependencyUtils.java

@@ -1,20 +1,15 @@
 package com.velocitypowered.proxy.plugin.util;
 
-import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Maps;
-import com.google.common.graph.EndpointPair;
 import com.google.common.graph.Graph;
 import com.google.common.graph.GraphBuilder;
 import com.google.common.graph.MutableGraph;
 import com.velocitypowered.api.plugin.PluginDescription;
 import com.velocitypowered.api.plugin.meta.PluginDependency;
-import java.util.ArrayDeque;
 import java.util.ArrayList;
-import java.util.HashSet;
+import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
-import java.util.Queue;
-import java.util.Set;
 
 public class PluginDependencyUtils {
 
@@ -24,7 +19,7 @@ public class PluginDependencyUtils {
 
   /**
    * Attempts to topographically sort all plugins for the proxy to load by dependencies using
-   * Kahn's algorithm.
+   * a depth-first search.
    *
    * @param candidates the plugins to sort
    * @return the sorted list of plugins
@@ -49,72 +44,37 @@ public class PluginDependencyUtils {
       }
     }
 
-    // Find nodes that have no edges
-    Queue<PluginDescription> noEdges = getNoDependencyCandidates(graph);
-
-    // Actually run Kahn's algorithm
     List<PluginDescription> sorted = new ArrayList<>();
-    while (!noEdges.isEmpty()) {
+    Map<PluginDescription, Mark> marks = new HashMap<>();
-      PluginDescription candidate = noEdges.remove();
-      sorted.add(candidate);
 
-      for (PluginDescription node : ImmutableSet.copyOf(graph.adjacentNodes(candidate))) {
+    for (PluginDescription node : graph.nodes()) {
-        graph.removeEdge(node, candidate);
+      visitNode(graph, node, marks, sorted);
-
-        if (graph.adjacentNodes(node).isEmpty()) {
-          noEdges.add(node);
-        }
-      }
-    }
-
-    if (!graph.edges().isEmpty()) {
-      throw new IllegalStateException(
-          "Plugin circular dependency found: " + createLoopInformation(graph));
     }
 
     return sorted;
   }
 
-  private static Queue<PluginDescription> getNoDependencyCandidates(Graph<PluginDescription> graph) {
+  private static void visitNode(Graph<PluginDescription> dependencyGraph, PluginDescription node,
-    Queue<PluginDescription> found = new ArrayDeque<>();
+      Map<PluginDescription, Mark> marks, List<PluginDescription> sorted) {
-
+    Mark mark = marks.getOrDefault(node, Mark.NOT_VISITED);
-    for (PluginDescription node : graph.nodes()) {
+    if (mark == Mark.PERMANENT) {
-      if (graph.outDegree(node) == 0) {
+      return;
-        found.add(node);
+    } else if (mark == Mark.TEMPORARY) {
-      }
+      throw new IllegalStateException("Improper plugin dependency graph");
     }
 
-    return found;
+    marks.put(node, Mark.TEMPORARY);
+    for (PluginDescription edge : dependencyGraph.successors(node)) {
+      visitNode(dependencyGraph, edge, marks, sorted);
+    }
+
+    marks.put(node, Mark.PERMANENT);
+    sorted.add(node);
   }
 
-  private static String createLoopInformation(Graph<PluginDescription> graph) {
+  private enum Mark {
-    StringBuilder repr = new StringBuilder("{");
+    NOT_VISITED,
-    for (EndpointPair<PluginDescription> edge : graph.edges()) {
+    TEMPORARY,
-      repr.append(edge.target().getId()).append(": [");
+    PERMANENT
-      repr.append(dependencyLoopInfo(graph, edge.target(), new HashSet<>())).append("], ");
-    }
-    repr.setLength(repr.length() - 2);
-    repr.append("}");
-    return repr.toString();
-  }
-
-  private static String dependencyLoopInfo(Graph<PluginDescription> graph,
-      PluginDescription dependency, Set<PluginDescription> seen) {
-    StringBuilder repr = new StringBuilder();
-    for (PluginDescription pd : graph.adjacentNodes(dependency)) {
-      if (seen.add(pd)) {
-        repr.append(pd.getId()).append(": [").append(dependencyLoopInfo(graph, dependency, seen))
-            .append("], ");
-      } else {
-        repr.append(pd.getId()).append(", ");
-      }
-    }
-
-    if (repr.length() != 0) {
-      repr.setLength(repr.length() - 2);
-      return repr.toString();
-    } else {
-      return "<no depends>";
-    }
   }
 }

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

@@ -8,6 +8,7 @@ import com.velocitypowered.api.plugin.PluginDescription;
 import com.velocitypowered.api.plugin.meta.PluginDependency;
 import com.velocitypowered.proxy.plugin.loader.VelocityPluginDescription;
 import java.util.ArrayList;
+import java.util.Comparator;
 import java.util.List;
 import org.junit.jupiter.api.Test;
 
@@ -38,6 +39,13 @@ class PluginDependencyUtilsTest {
           new PluginDependency("example2", "", false)
       ), null
   );
+  private static final PluginDescription TEST_WITH_DUPLICATE_DEPEND = new VelocityPluginDescription(
+      "dup-depend", "tuxed", "0.1", null, null, ImmutableList.of(),
+      ImmutableList.of(
+          new PluginDependency("multi-depend", "", false),
+          new PluginDependency("example2", "", false)
+      ), null
+  );
 
   private static final PluginDescription CIRCULAR_DEPENDENCY_1 = new VelocityPluginDescription(
       "circle", "tuxed", "0.1", null, null, ImmutableList.of(),
@@ -55,12 +63,13 @@ class PluginDependencyUtilsTest {
   // Note: Kahn's algorithm is non-unique in its return result, although the topological sort will have the correct
   // order.
   private static final List<PluginDescription> EXPECTED = ImmutableList.of(
+      NEVER_DEPENDED,
       NO_DEPENDENCY_1_EXAMPLE,
       NO_DEPENDENCY_2_EXAMPLE,
-      NEVER_DEPENDED,
+      MULTI_DEPENDENCY,
+      TEST_WITH_DUPLICATE_DEPEND,
       SOFT_DEPENDENCY_DOES_NOT_EXIST,
-      SOFT_DEPENDENCY_EXISTS,
+      SOFT_DEPENDENCY_EXISTS
-      MULTI_DEPENDENCY
   );
 
   @Test
@@ -72,6 +81,8 @@ class PluginDependencyUtilsTest {
     descriptionList.add(SOFT_DEPENDENCY_DOES_NOT_EXIST);
     descriptionList.add(SOFT_DEPENDENCY_EXISTS);
     descriptionList.add(MULTI_DEPENDENCY);
+    descriptionList.add(TEST_WITH_DUPLICATE_DEPEND);
+    descriptionList.sort(Comparator.comparing(PluginDescription::getId));
 
     assertEquals(EXPECTED, PluginDependencyUtils.sortCandidates(descriptionList));
   }