Skip to content

Commit 90f4492

Browse files
jpenillajpenilla
committed
Cache parent/child permission results when attempting to convert no permission to syntax error
1 parent 4cf8d33 commit 90f4492

File tree

3 files changed

+112
-63
lines changed

3 files changed

+112
-63
lines changed

‎cloud-core/src/main/java/org/incendo/cloud/CommandManager.java‎

Lines changed: 5 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -26,17 +26,13 @@
2626
import io.leangen.geantyref.TypeToken;
2727
import java.util.Collection;
2828
import java.util.Collections;
29-
import java.util.HashMap;
3029
import java.util.HashSet;
3130
import java.util.LinkedList;
3231
import java.util.List;
33-
import java.util.Map;
3432
import java.util.Objects;
3533
import java.util.Set;
36-
import java.util.concurrent.atomic.AtomicInteger;
3734
import java.util.concurrent.atomic.AtomicReference;
3835
import java.util.function.Consumer;
39-
import java.util.function.Function;
4036
import java.util.stream.Collectors;
4137
import org.apiguardian.api.API;
4238
import org.checkerframework.checker.nullness.qual.NonNull;
@@ -66,6 +62,7 @@
6662
import org.incendo.cloud.injection.ParameterInjectorRegistry;
6763
import org.incendo.cloud.internal.CommandNode;
6864
import org.incendo.cloud.internal.CommandRegistrationHandler;
65+
import org.incendo.cloud.internal.ThreadLocalPermissionCache;
6966
import org.incendo.cloud.meta.CommandMeta;
7067
import org.incendo.cloud.parser.ArgumentParser;
7168
import org.incendo.cloud.parser.ParserParameter;
@@ -112,6 +109,7 @@ public abstract class CommandManager<C> implements Stateful<RegistrationState>,
112109
private final Set<CloudCapability> capabilities = new HashSet<>();
113110
private final ExceptionController<C> exceptionController = new ExceptionController<>();
114111
private final CommandExecutor<C> commandExecutor;
112+
final ThreadLocalPermissionCache<C> threadLocalPermissionCache = new ThreadLocalPermissionCache<>(this.settings);
115113

116114
private CaptionFormatter<C, String> captionVariableReplacementHandler = CaptionFormatter.placeholderReplacing();
117115
private CommandSyntaxFormatter<C> commandSyntaxFormatter = new StandardCommandSyntaxFormatter<>(this);
@@ -349,22 +347,6 @@ public boolean hasCapability(final @NonNull CloudCapability capability) {
349347
return Collections.unmodifiableSet(new HashSet<>(this.capabilities));
350348
}
351349

352-
private final ThreadLocal<Pair<Map<Pair<C, Permission>, PermissionResult>, AtomicInteger>> threadLocalPermissionCache =
353-
ThreadLocal.withInitial(() -> Pair.of(new HashMap<>(), new AtomicInteger(0)));
354-
355-
@SuppressWarnings("rawtypes")
356-
private @NonNull <T> PermissionResult testPermissionCaching(
357-
final @NonNull C sender,
358-
final @NonNull T permission,
359-
final @NonNull Function<Pair<C, T>, @NonNull PermissionResult> tester
360-
) {
361-
if (!this.settings.get(ManagerSetting.REDUCE_REDUNDANT_PERMISSION_CHECKS)) {
362-
return tester.apply(Pair.of(sender, permission));
363-
}
364-
return this.threadLocalPermissionCache.get().first()
365-
.computeIfAbsent((Pair) Pair.of(sender, permission), (Function) tester);
366-
}
367-
368350
/**
369351
* Checks if the command sender has the required permission and returns the result.
370352
*
@@ -377,30 +359,13 @@ public boolean hasCapability(final @NonNull CloudCapability capability) {
377359
final @NonNull C sender,
378360
final @NonNull Permission permission
379361
) {
380-
final boolean cache = this.settings.get(ManagerSetting.REDUCE_REDUNDANT_PERMISSION_CHECKS);
381-
try {
382-
if (cache) {
383-
final int prev = this.threadLocalPermissionCache.get().second().getAndIncrement();
384-
if (prev == 0) {
385-
// Cleanup from case where cache was enabled mid-permission check
386-
this.threadLocalPermissionCache.get().first().clear();
387-
}
388-
}
389-
return this.testPermission_(sender, permission);
390-
} finally {
391-
if (cache) {
392-
final Pair<Map<Pair<C, Permission>, PermissionResult>, AtomicInteger> pair = this.threadLocalPermissionCache.get();
393-
if (pair.second().getAndDecrement() == 1) {
394-
pair.first().clear();
395-
}
396-
}
397-
}
362+
return this.threadLocalPermissionCache.withPermissionCache(() -> this.testPermission_(sender, permission));
398363
}
399364

400365
@SuppressWarnings("unchecked")
401366
private @NonNull PermissionResult testPermission_(final @NonNull C sender, final @NonNull Permission permission) {
402367
if (permission instanceof PredicatePermission) {
403-
return this.testPermissionCaching(sender, (PredicatePermission<C>) permission, pair -> {
368+
return this.threadLocalPermissionCache.testPermissionCaching(sender, (PredicatePermission<C>) permission, pair -> {
404369
return pair.second().testPermission(pair.first());
405370
});
406371
} else if (permission instanceof OrPermission) {
@@ -420,7 +385,7 @@ public boolean hasCapability(final @NonNull CloudCapability capability) {
420385
}
421386
return PermissionResult.allowed(permission);
422387
}
423-
return this.testPermissionCaching(sender, permission, pair -> {
388+
return this.threadLocalPermissionCache.testPermissionCaching(sender, permission, pair -> {
424389
return PermissionResult.of(
425390
pair.second().isEmpty() || this.hasPermission(pair.first(), pair.second().permissionString()), pair.second());
426391
});

‎cloud-core/src/main/java/org/incendo/cloud/CommandTree.java‎

Lines changed: 25 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -356,31 +356,33 @@ private Exception noPermissionOrSyntax(
356356
);
357357
}
358358

359-
if (this.childPermitted(root, sender)) {
360-
return new InvalidSyntaxException(
361-
this.commandManager.commandSyntaxFormatter().apply(sender, (List) this.getComponentChain(root), root),
362-
sender, this.getComponentChain(root)
363-
);
364-
}
359+
return this.commandManager.threadLocalPermissionCache.withPermissionCache(() -> {
360+
if (this.childPermitted(root, sender)) {
361+
return new InvalidSyntaxException(
362+
this.commandManager.commandSyntaxFormatter().apply(sender, (List) this.getComponentChain(root), root),
363+
sender, this.getComponentChain(root)
364+
);
365+
}
365366

366-
final @Nullable List<CommandNode<C>> parentChain = this.permittedParentChain(root, sender);
367-
if (parentChain != null) {
368-
return new InvalidSyntaxException(
369-
this.commandManager.commandSyntaxFormatter().apply(
370-
sender,
371-
parentChain.stream().map(CommandNode::component)
372-
.filter(Objects::nonNull).collect(Collectors.toList()),
373-
root
374-
),
375-
sender, this.getComponentChain(root)
376-
);
377-
}
367+
final @Nullable List<CommandNode<C>> parentChain = this.permittedParentChain(root, sender);
368+
if (parentChain != null) {
369+
return new InvalidSyntaxException(
370+
this.commandManager.commandSyntaxFormatter().apply(
371+
sender,
372+
parentChain.stream().map(CommandNode::component)
373+
.filter(Objects::nonNull).collect(Collectors.toList()),
374+
root
375+
),
376+
sender, this.getComponentChain(root)
377+
);
378+
}
378379

379-
return new NoPermissionException(
380-
permissionResult,
381-
sender,
382-
this.getComponentChain(root)
383-
);
380+
return new NoPermissionException(
381+
permissionResult,
382+
sender,
383+
this.getComponentChain(root)
384+
);
385+
});
384386
}
385387

386388
private boolean childPermitted(CommandNode<C> node, C sender) {

‎cloud-core/src/main/java/org/incendo/cloud/internal/ThreadLocalPermissionCache.java‎

Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,82 @@
1+
//
2+
// MIT License
3+
//
4+
// Copyright (c) 2024 Incendo
5+
//
6+
// Permission is hereby granted, free of charge, to any person obtaining a copy
7+
// of this software and associated documentation files (the "Software"), to deal
8+
// in the Software without restriction, including without limitation the rights
9+
// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
10+
// copies of the Software, and to permit persons to whom the Software is
11+
// furnished to do so, subject to the following conditions:
12+
//
13+
// The above copyright notice and this permission notice shall be included in all
14+
// copies or substantial portions of the Software.
15+
//
16+
// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
17+
// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
18+
// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
19+
// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
20+
// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
21+
// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
22+
// SOFTWARE.
23+
//
24+
package org.incendo.cloud.internal;
25+
26+
import java.util.HashMap;
27+
import java.util.Map;
28+
import java.util.concurrent.atomic.AtomicInteger;
29+
import java.util.function.Function;
30+
import java.util.function.Supplier;
31+
import org.apiguardian.api.API;
32+
import org.checkerframework.checker.nullness.qual.NonNull;
33+
import org.incendo.cloud.permission.Permission;
34+
import org.incendo.cloud.permission.PermissionResult;
35+
import org.incendo.cloud.setting.Configurable;
36+
import org.incendo.cloud.setting.ManagerSetting;
37+
import org.incendo.cloud.type.tuple.Pair;
38+
39+
@API(status = API.Status.INTERNAL)
40+
public final class ThreadLocalPermissionCache<C> {
41+
private final ThreadLocal<Pair<Map<Pair<C, Permission>, PermissionResult>, AtomicInteger>> threadLocalPermissionCache =
42+
ThreadLocal.withInitial(() -> Pair.of(new HashMap<>(), new AtomicInteger(0)));
43+
private final Configurable<ManagerSetting> settings;
44+
45+
public ThreadLocalPermissionCache(final Configurable<ManagerSetting> settings) {
46+
this.settings = settings;
47+
}
48+
49+
public <T> T withPermissionCache(final Supplier<T> action) {
50+
final boolean cache = this.settings.get(ManagerSetting.REDUCE_REDUNDANT_PERMISSION_CHECKS);
51+
try {
52+
if (cache) {
53+
final int prev = this.threadLocalPermissionCache.get().second().getAndIncrement();
54+
if (prev == 0) {
55+
// Cleanup from case where cache was enabled mid-permission check
56+
this.threadLocalPermissionCache.get().first().clear();
57+
}
58+
}
59+
return action.get();
60+
} finally {
61+
if (cache) {
62+
final Pair<Map<Pair<C, Permission>, PermissionResult>, AtomicInteger> pair = this.threadLocalPermissionCache.get();
63+
if (pair.second().getAndDecrement() == 1) {
64+
pair.first().clear();
65+
}
66+
}
67+
}
68+
}
69+
70+
@SuppressWarnings({"rawtypes", "unchecked"})
71+
public @NonNull <T> PermissionResult testPermissionCaching(
72+
final @NonNull C sender,
73+
final @NonNull T permission,
74+
final @NonNull Function<Pair<C, T>, @NonNull PermissionResult> tester
75+
) {
76+
if (!this.settings.get(ManagerSetting.REDUCE_REDUNDANT_PERMISSION_CHECKS)) {
77+
return tester.apply(Pair.of(sender, permission));
78+
}
79+
return this.threadLocalPermissionCache.get().first()
80+
.computeIfAbsent((Pair) Pair.of(sender, permission), (Function) tester);
81+
}
82+
}

0 commit comments

Comments
 (0)