From f36f411a8dc48ebf6914e46029a78b770418292b Mon Sep 17 00:00:00 2001 From: Luck Date: Thu, 22 Dec 2016 17:50:56 +0000 Subject: [PATCH] Fix broken inheritance caching system - closes #97 --- ...desHolder.java => GetAllNodesRequest.java} | 2 +- .../common/core/model/PermissionHolder.java | 118 ++++++------------ .../common/utils/WeightComparator.java | 42 +++++++ 3 files changed, 84 insertions(+), 78 deletions(-) rename common/src/main/java/me/lucko/luckperms/common/caching/holder/{GetAllNodesHolder.java => GetAllNodesRequest.java} (97%) create mode 100644 common/src/main/java/me/lucko/luckperms/common/utils/WeightComparator.java diff --git a/common/src/main/java/me/lucko/luckperms/common/caching/holder/GetAllNodesHolder.java b/common/src/main/java/me/lucko/luckperms/common/caching/holder/GetAllNodesRequest.java similarity index 97% rename from common/src/main/java/me/lucko/luckperms/common/caching/holder/GetAllNodesHolder.java rename to common/src/main/java/me/lucko/luckperms/common/caching/holder/GetAllNodesRequest.java index b9a8a07c9..7675f32c1 100644 --- a/common/src/main/java/me/lucko/luckperms/common/caching/holder/GetAllNodesHolder.java +++ b/common/src/main/java/me/lucko/luckperms/common/caching/holder/GetAllNodesRequest.java @@ -35,7 +35,7 @@ import me.lucko.luckperms.common.utils.ExtractedContexts; @ToString @EqualsAndHashCode @AllArgsConstructor(staticName = "of") -public class GetAllNodesHolder { +public class GetAllNodesRequest { private final ImmutableList excludedGroups; private final ExtractedContexts contexts; diff --git a/common/src/main/java/me/lucko/luckperms/common/core/model/PermissionHolder.java b/common/src/main/java/me/lucko/luckperms/common/core/model/PermissionHolder.java index 64153b1fb..4f67d03ab 100644 --- a/common/src/main/java/me/lucko/luckperms/common/core/model/PermissionHolder.java +++ b/common/src/main/java/me/lucko/luckperms/common/core/model/PermissionHolder.java @@ -52,7 +52,7 @@ import me.lucko.luckperms.common.caching.handlers.CachedStateManager; import me.lucko.luckperms.common.caching.handlers.GroupReference; import me.lucko.luckperms.common.caching.handlers.HolderReference; import me.lucko.luckperms.common.caching.holder.ExportNodesHolder; -import me.lucko.luckperms.common.caching.holder.GetAllNodesHolder; +import me.lucko.luckperms.common.caching.holder.GetAllNodesRequest; import me.lucko.luckperms.common.commands.utils.Util; import me.lucko.luckperms.common.core.InheritanceInfo; import me.lucko.luckperms.common.core.NodeBuilder; @@ -61,6 +61,7 @@ import me.lucko.luckperms.common.core.PriorityComparator; import me.lucko.luckperms.common.utils.Cache; import me.lucko.luckperms.common.utils.ExtractedContexts; import me.lucko.luckperms.common.utils.ImmutableLocalizedNode; +import me.lucko.luckperms.common.utils.WeightComparator; import me.lucko.luckperms.exceptions.ObjectAlreadyHasException; import me.lucko.luckperms.exceptions.ObjectLacksException; @@ -127,6 +128,7 @@ public abstract class PermissionHolder { /* Internal Caches - only depend on the state of this instance. */ + // Just holds immutable copies of the node sets. private Cache> enduringCache = new Cache<>(() -> { synchronized (nodes) { return ImmutableSet.copyOf(nodes); @@ -137,16 +139,19 @@ public abstract class PermissionHolder { return ImmutableSet.copyOf(transientNodes); } }); - private Cache> cache = new Cache<>(this::cacheApply); - private Cache> mergedCache = new Cache<>(this::mergedCacheApply); + + // Merges transient and persistent nodes together, and converts Node instances to a localized form. + private Cache> cache = new Cache<>(() -> cacheApply(false)); + // Same as the cache above, except this merges temporary values with any permanent values if any are matching. + private Cache> mergedCache = new Cache<>(() -> cacheApply(true)); /* External Caches - may depend on the state of other instances. */ - private LoadingCache> getAllNodesCache = CacheBuilder.newBuilder() + private LoadingCache> getAllNodesCache = CacheBuilder.newBuilder() .expireAfterAccess(10, TimeUnit.MINUTES) - .build(new CacheLoader>() { + .build(new CacheLoader>() { @Override - public SortedSet load(GetAllNodesHolder getAllNodesHolder) { + public SortedSet load(GetAllNodesRequest getAllNodesHolder) { return getAllNodesCacheApply(getAllNodesHolder); } }); @@ -220,7 +225,7 @@ public abstract class PermissionHolder { declareState(); } - private ImmutableSortedSet cacheApply() { + private ImmutableSortedSet cacheApply(boolean mergeTemp) { TreeSet combined = new TreeSet<>(PriorityComparator.reverse()); Set enduring = getNodes(); if (!enduring.isEmpty()) { @@ -244,7 +249,7 @@ public abstract class PermissionHolder { while (it.hasNext()) { LocalizedNode entry = it.next(); for (LocalizedNode h : higherPriority) { - if (entry.getNode().almostEquals(h.getNode())) { + if (mergeTemp ? entry.getNode().equalsIgnoringValueOrTemp(h.getNode()) : entry.getNode().almostEquals(h.getNode())) { it.remove(); continue iterate; } @@ -254,91 +259,48 @@ public abstract class PermissionHolder { return ImmutableSortedSet.copyOfSorted(combined); } - private ImmutableSortedSet mergedCacheApply() { - TreeSet combined = new TreeSet<>(PriorityComparator.reverse()); - Set enduring = getNodes(); - if (!enduring.isEmpty()) { - combined.addAll(enduring.stream() - .map(n -> makeLocal(n, getObjectName())) - .collect(Collectors.toList()) - ); - } - Set tran = getTransientNodes(); - if (!tran.isEmpty()) { - combined.addAll(transientNodes.stream() - .map(n -> makeLocal(n, getObjectName())) - .collect(Collectors.toList()) - ); - } - - Iterator it = combined.iterator(); - Set higherPriority = new HashSet<>(); - - iterate: - while (it.hasNext()) { - LocalizedNode entry = it.next(); - for (LocalizedNode h : higherPriority) { - if (entry.getNode().equalsIgnoringValueOrTemp(h.getNode())) { - it.remove(); - continue iterate; - } - } - higherPriority.add(entry); - } - return ImmutableSortedSet.copyOfSorted(combined); - } - - private SortedSet getAllNodesCacheApply(GetAllNodesHolder getAllNodesHolder) { + private SortedSet getAllNodesCacheApply(GetAllNodesRequest getAllNodesHolder) { + // Expand the holder. List excludedGroups = new ArrayList<>(getAllNodesHolder.getExcludedGroups()); ExtractedContexts contexts = getAllNodesHolder.getContexts(); + // Don't register users, as they cannot be inherited. + if (!(this instanceof User)) { + excludedGroups.add(getObjectName().toLowerCase()); + } + + // Get the objects base permissions. SortedSet all = new TreeSet<>((SortedSet) getPermissions(true)); - excludedGroups.add(getObjectName().toLowerCase()); - - Set parents = all.stream() - .map(LocalizedNode::getNode) - .filter(Node::getValue) - .filter(Node::isGroupNode) - .collect(Collectors.toSet()); - Contexts context = contexts.getContexts(); String server = contexts.getServer(); String world = contexts.getWorld(); - parents.removeIf(node -> - !node.shouldApplyOnServer(server, context.isApplyGlobalGroups(), plugin.getConfiguration().isApplyingRegex()) || - !node.shouldApplyOnWorld(world, context.isApplyGlobalWorldGroups(), plugin.getConfiguration().isApplyingRegex()) || - !node.shouldApplyWithContext(contexts.getContextSet(), false) - ); + Set parents = all.stream() + .map(LocalizedNode::getNode) + .filter(Node::getValue) + .filter(Node::isGroupNode) + .filter(n -> n.shouldApplyOnServer(server, context.isApplyGlobalGroups(), plugin.getConfiguration().isApplyingRegex())) + .filter(n -> n.shouldApplyOnWorld(world, context.isApplyGlobalWorldGroups(), plugin.getConfiguration().isApplyingRegex())) + .filter(n -> n.shouldApplyWithContext(contexts.getContextSet(), false)) + .collect(Collectors.toSet()); - TreeSet> sortedParents = new TreeSet<>(Util.META_COMPARATOR.reversed()); + // Resolve & sort parents into order before we apply. + TreeSet> sortedParents = new TreeSet<>(WeightComparator.INSTANCE.reversed()); for (Node node : parents) { Group group = plugin.getGroupManager().getIfLoaded(node.getGroupName()); if (group != null) { - OptionalInt weight = group.getWeight(); - if (weight.isPresent()) { - sortedParents.add(Maps.immutableEntry(weight.getAsInt(), node)); - continue; - } + sortedParents.add(Maps.immutableEntry(group.getWeight().orElse(0), group)); } - - sortedParents.add(Maps.immutableEntry(0, node)); } - for (Map.Entry e : sortedParents) { - Node parent = e.getValue(); - Group group = plugin.getGroupManager().getIfLoaded(parent.getGroupName()); - if (group == null) { - continue; - } - - if (excludedGroups.contains(group.getObjectName().toLowerCase())) { + for (Map.Entry e : sortedParents) { + if (excludedGroups.contains(e.getValue().getObjectName().toLowerCase())) { continue; } inherited: - for (LocalizedNode inherited : group.getAllNodes(excludedGroups, contexts)) { + for (LocalizedNode inherited : e.getValue().getAllNodes(excludedGroups, contexts)) { for (LocalizedNode existing : all) { if (existing.getNode().almostEquals(inherited.getNode())) { continue inherited; @@ -353,12 +315,11 @@ public abstract class PermissionHolder { } private Set getAllNodesFilteredApply(ExtractedContexts contexts) { - SortedSet allNodes; - Contexts context = contexts.getContexts(); String server = contexts.getServer(); String world = contexts.getWorld(); + SortedSet allNodes; if (context.isApplyGroups()) { allNodes = getAllNodes(null, contexts); } else { @@ -366,9 +327,11 @@ public abstract class PermissionHolder { } allNodes.removeIf(node -> - !node.shouldApplyOnServer(server, context.isIncludeGlobal(), plugin.getConfiguration().isApplyingRegex()) || + !node.isGroupNode() && ( + !node.shouldApplyOnServer(server, context.isIncludeGlobal(), plugin.getConfiguration().isApplyingRegex()) || !node.shouldApplyOnWorld(world, context.isIncludeGlobalWorld(), plugin.getConfiguration().isApplyingRegex()) || !node.shouldApplyWithContext(contexts.getContextSet(), false) + ) ); Set perms = ConcurrentHashMap.newKeySet(); @@ -462,6 +425,7 @@ public abstract class PermissionHolder { /** * Combines and returns this holders nodes in a priority order. * + * @param mergeTemp if the temporary nodes should be merged together with permanent nodes * @return the holders transient and permanent nodes */ public SortedSet getPermissions(boolean mergeTemp) { @@ -476,7 +440,7 @@ public abstract class PermissionHolder { * @return a set of nodes */ public SortedSet getAllNodes(List excludedGroups, ExtractedContexts contexts) { - return getAllNodesCache.getUnchecked(GetAllNodesHolder.of(excludedGroups == null ? ImmutableList.of() : ImmutableList.copyOf(excludedGroups), contexts)); + return getAllNodesCache.getUnchecked(GetAllNodesRequest.of(excludedGroups == null ? ImmutableList.of() : ImmutableList.copyOf(excludedGroups), contexts)); } /** diff --git a/common/src/main/java/me/lucko/luckperms/common/utils/WeightComparator.java b/common/src/main/java/me/lucko/luckperms/common/utils/WeightComparator.java new file mode 100644 index 000000000..8dd35dfb1 --- /dev/null +++ b/common/src/main/java/me/lucko/luckperms/common/utils/WeightComparator.java @@ -0,0 +1,42 @@ +/* + * Copyright (c) 2016 Lucko (Luck) + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in all + * copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE + * SOFTWARE. + */ + +package me.lucko.luckperms.common.utils; + +import lombok.AccessLevel; +import lombok.NoArgsConstructor; + +import me.lucko.luckperms.common.core.model.Group; + +import java.util.Comparator; +import java.util.Map; + +@NoArgsConstructor(access = AccessLevel.PRIVATE) +public class WeightComparator implements Comparator> { + public static final WeightComparator INSTANCE = new WeightComparator(); + + @Override + public int compare(Map.Entry o1, Map.Entry o2) { + int result = Integer.compare(o1.getKey(), o2.getKey()); + return result != 0 ? result : 1; + } +}