Add configuration option to perform a post-traversal sort according to the inheritance (weight) rules

bukkit/src/main/resources/config.yml

@@ -367,6 +367,19 @@ meta-formatting:
 #   => depth-first-post-order   See: https://en.wikipedia.org/wiki/Depth-first_search
 inheritance-traversal-algorithm: depth-first-pre-order
 
+# If a final sort according to "inheritance rules" should be performed after the traversal algorithm
+# has resolved the inheritance tree.
+#
+# "Inheritance rules" refers to things such as group weightings, primary group status, and the
+# natural contextual ordering of the group nodes.
+#
+# Setting this to 'true' will allow for the inheritance rules to take priority over the structure of
+# the inheritance tree.
+#
+# Effectively when this setting is 'true': the tree is flattened, and rules applied afterwards,
+# and when this setting is 'false':, the rules are just applied during each step of the traversal.
+post-traversal-inheritance-sort: false
+
 # +----------------------------------------------------------------------------------------------+ #
 # | Permission resolution settings                                                               | #
 # +----------------------------------------------------------------------------------------------+ #

bungee/src/main/resources/config.yml

@@ -375,6 +375,19 @@ meta-formatting:
 #   => depth-first-post-order   See: https://en.wikipedia.org/wiki/Depth-first_search
 inheritance-traversal-algorithm: depth-first-pre-order
 
+# If a final sort according to "inheritance rules" should be performed after the traversal algorithm
+# has resolved the inheritance tree.
+#
+# "Inheritance rules" refers to things such as group weightings, primary group status, and the
+# natural contextual ordering of the group nodes.
+#
+# Setting this to 'true' will allow for the inheritance rules to take priority over the structure of
+# the inheritance tree.
+#
+# Effectively when this setting is 'true': the tree is flattened, and rules applied afterwards,
+# and when this setting is 'false':, the rules are just applied during each step of the traversal.
+post-traversal-inheritance-sort: false
+
 # +----------------------------------------------------------------------------------------------+ #
 # | Permission resolution settings                                                               | #
 # +----------------------------------------------------------------------------------------------+ #

common/src/main/java/me/lucko/luckperms/common/config/ConfigKeys.java

@@ -263,6 +263,11 @@ public final class ConfigKeys {
         }
     });
 
+    /**
+     *
+     */
+    public static final ConfigKey<Boolean> POST_TRAVERSAL_INHERITANCE_SORT = booleanKey("post-traversal-inheritance-sort", false);
+
     /**
      * The configured group weightings
      */

common/src/main/java/me/lucko/luckperms/common/inheritance/InheritanceComparator.java

@@ -25,10 +25,10 @@
 
 package me.lucko.luckperms.common.inheritance;
 
+import me.lucko.luckperms.common.model.Group;
 import me.lucko.luckperms.common.model.HolderType;
 import me.lucko.luckperms.common.model.PermissionHolder;
 import me.lucko.luckperms.common.model.User;
-import me.lucko.luckperms.common.node.comparator.NodeWithContextComparator;
 import me.lucko.luckperms.common.node.factory.NodeFactory;
 
 import java.util.Comparator;
@@ -36,10 +36,10 @@ import java.util.Comparator;
 /**
  * Determines the order of group inheritance in {@link PermissionHolder}.
  */
-public class InheritanceComparator implements Comparator<ResolvedGroup> {
+public class InheritanceComparator implements Comparator<PermissionHolder> {
-    private static final Comparator<ResolvedGroup> NULL_ORIGIN = new InheritanceComparator(null).reversed();
+    private static final Comparator<PermissionHolder> NULL_ORIGIN = new InheritanceComparator(null).reversed();
 
-    public static Comparator<ResolvedGroup> getFor(PermissionHolder origin) {
+    public static Comparator<? super PermissionHolder> getFor(PermissionHolder origin) {
         if (origin.getType() == HolderType.USER) {
             return new InheritanceComparator(((User) origin)).reversed();
         }
@@ -53,25 +53,38 @@ public class InheritanceComparator implements Comparator<ResolvedGroup> {
     }
 
     @Override
-    public int compare(ResolvedGroup o1, ResolvedGroup o2) {
+    public int compare(PermissionHolder o1, PermissionHolder o2) {
-        int result = Integer.compare(o1.group().getWeight().orElse(0), o2.group().getWeight().orElse(0));
+        // if both users, return 0
+        // if one or the other is a user, give a higher priority to the user
+        boolean o1IsUser = o1.getType() == HolderType.USER;
+        boolean o2IsUser = o2.getType() == HolderType.USER;
+        if (o1IsUser && o2IsUser) {
+            return 0;
+        } else if (o1IsUser) {
+            return 1;
+        } else if (o2IsUser) {
+            return -1;
+        }
+
+        Group o1Group = (Group) o1;
+        Group o2Group = (Group) o2;
+
+        int result = Integer.compare(o1.getWeight().orElse(0), o2.getWeight().orElse(0));
         if (result != 0) {
             return result;
         }
 
         // failing differing group weights, check if one of the groups is a primary group
         if (this.origin != null) {
-            result = Boolean.compare(
+            return Boolean.compare(
-                    o1.group().getName().equalsIgnoreCase(this.origin.getPrimaryGroup().getStoredValue().orElse(NodeFactory.DEFAULT_GROUP_NAME)),
+                    o1Group.getName().equalsIgnoreCase(this.origin.getPrimaryGroup().getStoredValue().orElse(NodeFactory.DEFAULT_GROUP_NAME)),
-                    o2.group().getName().equalsIgnoreCase(this.origin.getPrimaryGroup().getStoredValue().orElse(NodeFactory.DEFAULT_GROUP_NAME))
+                    o2Group.getName().equalsIgnoreCase(this.origin.getPrimaryGroup().getStoredValue().orElse(NodeFactory.DEFAULT_GROUP_NAME))
             );
-
-            if (result != 0) {
-                return result;
-            }
         }
 
-        // failing weight checks, fallback to which group applies in more specific context
+        // failing weight/primary group checks, fallback to the node ordering
-        return NodeWithContextComparator.normal().compare(o1.node(), o2.node());
+        // this comparator is only ever used by Collections.sort - which is stable. the existing
+        // ordering of the nodes will therefore apply.
+        return 0;
     }
 }

common/src/main/java/me/lucko/luckperms/common/inheritance/InheritanceGraph.java

@@ -25,12 +25,60 @@
 
 package me.lucko.luckperms.common.inheritance;
 
+import me.lucko.luckperms.common.config.ConfigKeys;
+import me.lucko.luckperms.common.config.LuckPermsConfiguration;
 import me.lucko.luckperms.common.graph.Graph;
+import me.lucko.luckperms.common.graph.TraversalAlgorithm;
 import me.lucko.luckperms.common.model.PermissionHolder;
 
+import java.util.ArrayList;
+import java.util.List;
+
 /**
  * A {@link Graph} which represents an "inheritance tree".
  */
 public interface InheritanceGraph extends Graph<PermissionHolder> {
 
+    /**
+     * Returns an iterable which will traverse this inheritance graph using the specified
+     * algorithm starting at the given permission holder start node.
+     *
+     * @param algorithm the algorithm to use when traversing
+     * @param postTraversalSort if a final sort according to inheritance (weight, primary group) rules
+     *                          should be performed after the traversal algorithm has completed
+     * @param startNode the start node in the inheritance graph
+     * @return an iterable
+     */
+    default Iterable<PermissionHolder> traverse(TraversalAlgorithm algorithm, boolean postTraversalSort, PermissionHolder startNode) {
+        Iterable<PermissionHolder> traversal = traverse(algorithm, startNode);
+
+        // perform post traversal sort if needed
+        if (postTraversalSort) {
+            List<PermissionHolder> resolvedTraversal = new ArrayList<>();
+            for (PermissionHolder node : traversal) {
+                resolvedTraversal.add(node);
+            }
+
+            resolvedTraversal.sort(startNode.getInheritanceComparator());
+            traversal = resolvedTraversal;
+        }
+
+        return traversal;
+    }
+
+    /**
+     * Perform a traversal according to the rules defined in the configuration.
+     *
+     * @param configuration the configuration object
+     * @param startNode the start node in the inheritance graph
+     * @return an iterable
+     */
+    default Iterable<PermissionHolder> traverse(LuckPermsConfiguration configuration, PermissionHolder startNode) {
+        return traverse(
+                configuration.get(ConfigKeys.INHERITANCE_TRAVERSAL_ALGORITHM),
+                configuration.get(ConfigKeys.POST_TRAVERSAL_INHERITANCE_SORT),
+                startNode
+        );
+    }
+
 }

common/src/main/java/me/lucko/luckperms/common/inheritance/InheritanceHandler.java

@@ -33,9 +33,9 @@ import me.lucko.luckperms.common.model.PermissionHolder;
 import me.lucko.luckperms.common.plugin.LuckPermsPlugin;
 
 import java.util.ArrayList;
+import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Set;
-import java.util.TreeSet;
 
 /**
  * Provides {@link InheritanceGraph}s.
@@ -48,14 +48,12 @@ public class InheritanceHandler {
      */
     private final InheritanceGraph nonContextualGraph;
 
-    // some cached contextual graphs for common Contexts
+    // cached contextual graphs for common Contexts
-    private final InheritanceGraph allowAllContextualGraph;
     private final InheritanceGraph globalContextualGraph;
 
     public InheritanceHandler(LuckPermsPlugin plugin) {
         this.plugin = plugin;
         this.nonContextualGraph = new NonContextualGraph(plugin);
-        this.allowAllContextualGraph = new ContextualGraph(plugin, Contexts.allowAll());
         this.globalContextualGraph = new ContextualGraph(plugin, Contexts.global());
     }
 
@@ -65,13 +63,14 @@ public class InheritanceHandler {
 
     public InheritanceGraph getGraph(Contexts contexts) {
         if (contexts == Contexts.allowAll()) {
-            return this.allowAllContextualGraph;
+            throw new IllegalArgumentException("Contexts#allowAll passed to contextual #getGraph method");
-        }
-        if (contexts == Contexts.global()) {
-            return this.globalContextualGraph;
         }
 
-        return new ContextualGraph(this.plugin, contexts);
+        if (contexts == Contexts.global()) {
+            return this.globalContextualGraph;
+        } else {
+            return new ContextualGraph(this.plugin, contexts);
+        }
     }
 
     private static final class NonContextualGraph implements InheritanceGraph {
@@ -83,15 +82,17 @@ public class InheritanceHandler {
 
         @Override
         public Iterable<? extends PermissionHolder> successors(PermissionHolder holder) {
-            Set<ResolvedGroup> successors = new TreeSet<>(holder.getInheritanceComparator());
+            Set<Group> successors = new LinkedHashSet<>();
-            List<? extends Node> nodes = holder.getOwnGroupNodes();
+            for (Node n : holder.getOwnGroupNodes()) {
-            for (Node n : nodes) {
                 Group g = this.plugin.getGroupManager().getIfLoaded(n.getGroupName());
                 if (g != null) {
-                    successors.add(new ResolvedGroup(n, g));
+                    successors.add(g);
                 }
             }
-            return composeSuccessors(successors);
+
+            List<Group> successorsSorted = new ArrayList<>(successors);
+            successorsSorted.sort(holder.getInheritanceComparator());
+            return successorsSorted;
         }
     }
 
@@ -110,9 +111,8 @@ public class InheritanceHandler {
 
         @Override
         public Iterable<? extends PermissionHolder> successors(PermissionHolder holder) {
-            Set<ResolvedGroup> successors = new TreeSet<>(holder.getInheritanceComparator());
+            Set<Group> successors = new LinkedHashSet<>();
-            List<? extends Node> nodes = holder.getOwnGroupNodes(this.context.getContexts());
+            for (Node n : holder.getOwnGroupNodes(this.context.getContexts())) {
-            for (Node n : nodes) {
                 // effectively: if not (we're applying global groups or it's specific anyways)
                 if (!((this.context.hasSetting(LookupSetting.APPLY_PARENTS_SET_WITHOUT_SERVER) || n.isServerSpecific()) && (this.context.hasSetting(LookupSetting.APPLY_PARENTS_SET_WITHOUT_WORLD) || n.isWorldSpecific()))) {
                     continue;
@@ -120,19 +120,14 @@ public class InheritanceHandler {
 
                 Group g = this.plugin.getGroupManager().getIfLoaded(n.getGroupName());
                 if (g != null) {
-                    successors.add(new ResolvedGroup(n, g));
+                    successors.add(g);
                 }
             }
-            return composeSuccessors(successors);
-        }
-    }
 
-    private static Iterable<PermissionHolder> composeSuccessors(Set<ResolvedGroup> successors) {
+            List<Group> successorsSorted = new ArrayList<>(successors);
-        List<PermissionHolder> holders = new ArrayList<>(successors.size());
+            successorsSorted.sort(holder.getInheritanceComparator());
-        for (ResolvedGroup resolvedGroup : successors) {
+            return successorsSorted;
-            holders.add(resolvedGroup.group());
         }
-        return holders;
     }
 
 }

common/src/main/java/me/lucko/luckperms/common/model/PermissionHolder.java

@@ -43,10 +43,8 @@ import me.lucko.luckperms.api.context.ContextSet;
 import me.lucko.luckperms.api.context.ImmutableContextSet;
 import me.lucko.luckperms.common.cacheddata.HolderCachedData;
 import me.lucko.luckperms.common.cacheddata.type.MetaAccumulator;
-import me.lucko.luckperms.common.config.ConfigKeys;
 import me.lucko.luckperms.common.inheritance.InheritanceComparator;
 import me.lucko.luckperms.common.inheritance.InheritanceGraph;
-import me.lucko.luckperms.common.inheritance.ResolvedGroup;
 import me.lucko.luckperms.common.node.comparator.NodeWithContextComparator;
 import me.lucko.luckperms.common.node.utils.InheritanceInfo;
 import me.lucko.luckperms.common.node.utils.MetaType;
@@ -130,7 +128,7 @@ public abstract class PermissionHolder {
     /**
      * Comparator used to ordering groups when calculating inheritance
      */
-    private final Comparator<ResolvedGroup> inheritanceComparator = InheritanceComparator.getFor(this);
+    private final Comparator<? super PermissionHolder> inheritanceComparator = InheritanceComparator.getFor(this);
 
     /**
      * Creates a new instance
@@ -151,7 +149,7 @@ public abstract class PermissionHolder {
         return this.ioLock;
     }
 
-    public Comparator<ResolvedGroup> getInheritanceComparator() {
+    public Comparator<? super PermissionHolder> getInheritanceComparator() {
         return this.inheritanceComparator;
     }
 
@@ -321,7 +319,7 @@ public abstract class PermissionHolder {
 
     public void accumulateInheritancesTo(List<? super LocalizedNode> accumulator, Contexts context) {
         InheritanceGraph graph = this.plugin.getInheritanceHandler().getGraph(context);
-        Iterable<PermissionHolder> traversal = graph.traverse(this.plugin.getConfiguration().get(ConfigKeys.INHERITANCE_TRAVERSAL_ALGORITHM), this);
+        Iterable<PermissionHolder> traversal = graph.traverse(this.plugin.getConfiguration(), this);
         for (PermissionHolder holder : traversal) {
             List<? extends LocalizedNode> nodes = holder.getOwnNodes(context.getContexts());
             accumulator.addAll(nodes);
@@ -336,7 +334,7 @@ public abstract class PermissionHolder {
 
     public void accumulateInheritancesTo(List<? super LocalizedNode> accumulator) {
         InheritanceGraph graph = this.plugin.getInheritanceHandler().getGraph();
-        Iterable<PermissionHolder> traversal = graph.traverse(this.plugin.getConfiguration().get(ConfigKeys.INHERITANCE_TRAVERSAL_ALGORITHM), this);
+        Iterable<PermissionHolder> traversal = graph.traverse(this.plugin.getConfiguration(), this);
         for (PermissionHolder holder : traversal) {
             List<? extends LocalizedNode> nodes = holder.getOwnNodes();
             accumulator.addAll(nodes);
@@ -409,7 +407,7 @@ public abstract class PermissionHolder {
         }
 
         InheritanceGraph graph = this.plugin.getInheritanceHandler().getGraph(context);
-        Iterable<PermissionHolder> traversal = graph.traverse(this.plugin.getConfiguration().get(ConfigKeys.INHERITANCE_TRAVERSAL_ALGORITHM), this);
+        Iterable<PermissionHolder> traversal = graph.traverse(this.plugin.getConfiguration(), this);
         for (PermissionHolder holder : traversal) {
             List<? extends LocalizedNode> nodes = holder.getOwnNodes(context.getContexts());
             for (LocalizedNode node : nodes) {
@@ -438,7 +436,7 @@ public abstract class PermissionHolder {
         }
 
         InheritanceGraph graph = this.plugin.getInheritanceHandler().getGraph();
-        Iterable<PermissionHolder> traversal = graph.traverse(this.plugin.getConfiguration().get(ConfigKeys.INHERITANCE_TRAVERSAL_ALGORITHM), this);
+        Iterable<PermissionHolder> traversal = graph.traverse(this.plugin.getConfiguration(), this);
         for (PermissionHolder holder : traversal) {
             List<? extends LocalizedNode> nodes = holder.getOwnNodes();
             for (LocalizedNode node : nodes) {

common/src/main/java/me/lucko/luckperms/common/primarygroup/AllParentsByWeightHolder.java

@@ -45,25 +45,15 @@ public class AllParentsByWeightHolder extends ContextualHolder {
     protected @NonNull Optional<String> calculateValue(Contexts contexts) {
         InheritanceGraph graph = this.user.getPlugin().getInheritanceHandler().getGraph(contexts);
 
-        // fully traverse the graph, obtain a list of permission holders the user inherits from
+        // fully traverse the graph, obtain a list of permission holders the user inherits from in weight order.
-        Iterable<PermissionHolder> traversal = graph.traverse(this.user.getPlugin().getConfiguration().get(ConfigKeys.INHERITANCE_TRAVERSAL_ALGORITHM), this.user);
+        Iterable<PermissionHolder> traversal = graph.traverse(this.user.getPlugin().getConfiguration().get(ConfigKeys.INHERITANCE_TRAVERSAL_ALGORITHM), true, this.user);
 
-        Group bestGroup = null;
+        // return the name of the first found group
-
-        int best = 0;
         for (PermissionHolder holder : traversal) {
-            if (!(holder instanceof Group)) {
+            if (holder instanceof Group) {
-                continue;
+                return Optional.of(((Group) holder).getName());
-            }
-            Group g = ((Group) holder);
-
-            int weight = g.getWeight().orElse(0);
-            if (bestGroup == null || g.getWeight().orElse(0) > best) {
-                bestGroup = g;
-                best = weight;
             }
         }
-
+        return Optional.empty();
-        return bestGroup == null ? Optional.empty() : Optional.of(bestGroup.getName());
     }
 }

nukkit/src/main/resources/config.yml

@@ -362,6 +362,19 @@ meta-formatting:
 #   => depth-first-post-order   See: https://en.wikipedia.org/wiki/Depth-first_search
 inheritance-traversal-algorithm: depth-first-pre-order
 
+# If a final sort according to "inheritance rules" should be performed after the traversal algorithm
+# has resolved the inheritance tree.
+#
+# "Inheritance rules" refers to things such as group weightings, primary group status, and the
+# natural contextual ordering of the group nodes.
+#
+# Setting this to 'true' will allow for the inheritance rules to take priority over the structure of
+# the inheritance tree.
+#
+# Effectively when this setting is 'true': the tree is flattened, and rules applied afterwards,
+# and when this setting is 'false':, the rules are just applied during each step of the traversal.
+post-traversal-inheritance-sort: false
+
 # +----------------------------------------------------------------------------------------------+ #
 # | Permission resolution settings                                                               | #
 # +----------------------------------------------------------------------------------------------+ #

sponge/src/main/resources/luckperms.conf

@@ -376,6 +376,19 @@ meta-formatting {
 #   => depth-first-post-order   See: https://en.wikipedia.org/wiki/Depth-first_search
 inheritance-traversal-algorithm = "depth-first-pre-order"
 
+# If a final sort according to "inheritance rules" should be performed after the traversal algorithm
+# has resolved the inheritance tree.
+#
+# "Inheritance rules" refers to things such as group weightings, primary group status, and the
+# natural contextual ordering of the group nodes.
+#
+# Setting this to 'true' will allow for the inheritance rules to take priority over the structure of
+# the inheritance tree.
+#
+# Effectively when this setting is 'true': the tree is flattened, and rules applied afterwards,
+# and when this setting is 'false':, the rules are just applied during each step of the traversal.
+post-traversal-inheritance-sort = false
+
 # +----------------------------------------------------------------------------------------------+ #
 # | Permission resolution settings                                                               | #
 # +----------------------------------------------------------------------------------------------+ #

velocity/src/main/resources/config.yml

@@ -366,6 +366,19 @@ meta-formatting:
 #   => depth-first-post-order   See: https://en.wikipedia.org/wiki/Depth-first_search
 inheritance-traversal-algorithm: depth-first-pre-order
 
+# If a final sort according to "inheritance rules" should be performed after the traversal algorithm
+# has resolved the inheritance tree.
+#
+# "Inheritance rules" refers to things such as group weightings, primary group status, and the
+# natural contextual ordering of the group nodes.
+#
+# Setting this to 'true' will allow for the inheritance rules to take priority over the structure of
+# the inheritance tree.
+#
+# Effectively when this setting is 'true': the tree is flattened, and rules applied afterwards,
+# and when this setting is 'false':, the rules are just applied during each step of the traversal.
+post-traversal-inheritance-sort: false
+
 # +----------------------------------------------------------------------------------------------+ #
 # | Permission resolution settings                                                               | #
 # +----------------------------------------------------------------------------------------------+ #