From d639e47fbf30fd7a260139f4466322010626b3bf Mon Sep 17 00:00:00 2001 From: Andrew Steinborn Date: Thu, 27 Sep 2018 00:51:33 -0400 Subject: [PATCH] Fix a number of issues with dependency resolution and add unit tests --- .../plugin/util/PluginDependencyUtils.java | 35 +++++++- .../util/PluginDependencyUtilsTest.java | 84 +++++++++++++++++++ 2 files changed, 117 insertions(+), 2 deletions(-) create mode 100644 proxy/src/test/java/com/velocitypowered/proxy/plugin/util/PluginDependencyUtilsTest.java 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 2dcfc79f9..5b35a4200 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 @@ -1,6 +1,8 @@ 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; @@ -37,7 +39,7 @@ public class PluginDependencyUtils { PluginDescription candidate = noEdges.poll(); sorted.add(candidate); - for (PluginDescription node : graph.successors(candidate)) { + for (PluginDescription node : ImmutableSet.copyOf(graph.adjacentNodes(candidate))) { graph.removeEdge(node, candidate); if (graph.adjacentNodes(node).isEmpty()) { @@ -49,7 +51,7 @@ public class PluginDependencyUtils { } if (!graph.edges().isEmpty()) { - throw new IllegalStateException("Plugin circular dependency found: " + graph.toString()); + throw new IllegalStateException("Plugin circular dependency found: " + createLoopInformation(graph)); } return sorted; @@ -66,4 +68,33 @@ public class PluginDependencyUtils { return found; } + + public static String createLoopInformation(Graph graph) { + StringBuilder repr = new StringBuilder("{"); + for (EndpointPair edge : graph.edges()) { + repr.append(edge.target().getId()).append(": ["); + repr.append(dependencyLoopInfo(graph, edge.target(), new HashSet<>())).append("], "); + } + repr.setLength(repr.length() - 2); + repr.append("}"); + return repr.toString(); + } + + private static String dependencyLoopInfo(Graph graph, PluginDescription dependency, Set 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 ""; + } + } } 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 new file mode 100644 index 000000000..9a3533b16 --- /dev/null +++ b/proxy/src/test/java/com/velocitypowered/proxy/plugin/util/PluginDependencyUtilsTest.java @@ -0,0 +1,84 @@ +package com.velocitypowered.proxy.plugin.util; + +import com.google.common.collect.ImmutableList; +import com.velocitypowered.api.plugin.PluginDescription; +import com.velocitypowered.api.plugin.meta.PluginDependency; +import com.velocitypowered.proxy.plugin.loader.VelocityPluginDescription; +import org.junit.jupiter.api.Test; + +import java.util.ArrayList; +import java.util.List; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; + +class PluginDependencyUtilsTest { + private static final PluginDescription NO_DEPENDENCY_1_EXAMPLE = new VelocityPluginDescription( + "example", "tuxed", "0.1", null, null, ImmutableList.of("example2"), + ImmutableList.of(), null + ); + private static final PluginDescription NO_DEPENDENCY_2_EXAMPLE = new VelocityPluginDescription( + "example2", "tuxed", "0.1", null, null, ImmutableList.of(), ImmutableList.of(), null + ); + private static final PluginDescription NEVER_DEPENDED = new VelocityPluginDescription( + "and-again", "tuxed", "0.1", null, null, ImmutableList.of(), ImmutableList.of(), null + ); + private static final PluginDescription SOFT_DEPENDENCY_EXISTS = new VelocityPluginDescription( + "soft", "tuxed", "0.1", null, null, ImmutableList.of(), + ImmutableList.of(new PluginDependency("example", "", true)), null + ); + private static final PluginDescription SOFT_DEPENDENCY_DOES_NOT_EXIST = new VelocityPluginDescription( + "fluffy", "tuxed", "0.1", null, null, ImmutableList.of(), + ImmutableList.of(new PluginDependency("i-dont-exist", "", false)), null + ); + private static final PluginDescription MULTI_DEPENDENCY = new VelocityPluginDescription( + "multi-depend", "tuxed", "0.1", null, null, ImmutableList.of(), + ImmutableList.of( + new PluginDependency("example", "", false), + new PluginDependency("example2", "", false) + ), null + ); + + private static final PluginDescription CIRCULAR_DEPENDENCY_1 = new VelocityPluginDescription( + "circle", "tuxed", "0.1", null, null, ImmutableList.of(), + ImmutableList.of( + new PluginDependency("oval", "", false) + ), null + ); + private static final PluginDescription CIRCULAR_DEPENDENCY_2 = new VelocityPluginDescription( + "oval", "tuxed", "0.1", null, null, ImmutableList.of(), + ImmutableList.of( + new PluginDependency("circle", "", false) + ), null + ); + + // Note: Kahn's algorithm is non-unique in its return result, although the topological sort will have the correct + // order. + private static final List EXPECTED = ImmutableList.of( + NO_DEPENDENCY_1_EXAMPLE, + NO_DEPENDENCY_2_EXAMPLE, + NEVER_DEPENDED, + SOFT_DEPENDENCY_DOES_NOT_EXIST, + SOFT_DEPENDENCY_EXISTS, + MULTI_DEPENDENCY + ); + + @Test + void sortCandidates() throws Exception { + List descriptionList = new ArrayList<>(); + descriptionList.add(NO_DEPENDENCY_1_EXAMPLE); + descriptionList.add(NO_DEPENDENCY_2_EXAMPLE); + descriptionList.add(NEVER_DEPENDED); + descriptionList.add(SOFT_DEPENDENCY_DOES_NOT_EXIST); + descriptionList.add(SOFT_DEPENDENCY_EXISTS); + descriptionList.add(MULTI_DEPENDENCY); + + assertEquals(EXPECTED, PluginDependencyUtils.sortCandidates(descriptionList)); + } + + @Test + void sortCandidatesCircularDependency() throws Exception { + List descs = ImmutableList.of(CIRCULAR_DEPENDENCY_1, CIRCULAR_DEPENDENCY_2); + assertThrows(IllegalStateException.class, () -> PluginDependencyUtils.sortCandidates(descs)); + } +}