Skip to content

Commit 388f45d

Browse files
jpenillajpenilla
committed
Cache parent/child permission results when attempting to convert no permission to syntax error
1 parent e430ea1 commit 388f45d

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;
@@ -115,6 +112,7 @@ public abstract class CommandManager<C> implements Stateful<RegistrationState>,
115112
private final Set<CloudCapability> capabilities = new HashSet<>();
116113
private final ExceptionController<C> exceptionController = new ExceptionController<>();
117114
private final CommandExecutor<C> commandExecutor;
115+
final ThreadLocalPermissionCache<C> threadLocalPermissionCache = new ThreadLocalPermissionCache<>(this.settings);
118116

119117
private CaptionFormatter<C, String> captionVariableReplacementHandler = CaptionFormatter.placeholderReplacing();
120118
private CommandSyntaxFormatter<C> commandSyntaxFormatter = new StandardCommandSyntaxFormatter<>(this);
@@ -382,22 +380,6 @@ public boolean hasCapability(final @NonNull CloudCapability capability) {
382380
return Collections.unmodifiableSet(new HashSet<>(this.capabilities));
383381
}
384382

385-
private final ThreadLocal<Pair<Map<Pair<C, Permission>, PermissionResult>, AtomicInteger>> threadLocalPermissionCache =
386-
ThreadLocal.withInitial(() -> Pair.of(new HashMap<>(), new AtomicInteger(0)));
387-
388-
@SuppressWarnings("rawtypes")
389-
private @NonNull <T> PermissionResult testPermissionCaching(
390-
final @NonNull C sender,
391-
final @NonNull T permission,
392-
final @NonNull Function<Pair<C, T>, @NonNull PermissionResult> tester
393-
) {
394-
if (!this.settings.get(ManagerSetting.REDUCE_REDUNDANT_PERMISSION_CHECKS)) {
395-
return tester.apply(Pair.of(sender, permission));
396-
}
397-
return this.threadLocalPermissionCache.get().first()
398-
.computeIfAbsent((Pair) Pair.of(sender, permission), (Function) tester);
399-
}
400-
401383
/**
402384
* Checks if the command sender has the required permission and returns the result.
403385
*
@@ -410,30 +392,13 @@ public boolean hasCapability(final @NonNull CloudCapability capability) {
410392
final @NonNull C sender,
411393
final @NonNull Permission permission
412394
) {
413-
final boolean cache = this.settings.get(ManagerSetting.REDUCE_REDUNDANT_PERMISSION_CHECKS);
414-
try {
415-
if (cache) {
416-
final int prev = this.threadLocalPermissionCache.get().second().getAndIncrement();
417-
if (prev == 0) {
418-
// Cleanup from case where cache was enabled mid-permission check
419-
this.threadLocalPermissionCache.get().first().clear();
420-
}
421-
}
422-
return this.testPermission_(sender, permission);
423-
} finally {
424-
if (cache) {
425-
final Pair<Map<Pair<C, Permission>, PermissionResult>, AtomicInteger> pair = this.threadLocalPermissionCache.get();
426-
if (pair.second().getAndDecrement() == 1) {
427-
pair.first().clear();
428-
}
429-
}
430-
}
395+
return this.threadLocalPermissionCache.withPermissionCache(() -> this.testPermission_(sender, permission));
431396
}
432397

433398
@SuppressWarnings("unchecked")
434399
private @NonNull PermissionResult testPermission_(final @NonNull C sender, final @NonNull Permission permission) {
435400
if (permission instanceof PredicatePermission) {
436-
return this.testPermissionCaching(sender, (PredicatePermission<C>) permission, pair -> {
401+
return this.threadLocalPermissionCache.testPermissionCaching(sender, (PredicatePermission<C>) permission, pair -> {
437402
return pair.second().testPermission(pair.first());
438403
});
439404
} else if (permission instanceof OrPermission) {
@@ -453,7 +418,7 @@ public boolean hasCapability(final @NonNull CloudCapability capability) {
453418
}
454419
return PermissionResult.allowed(permission);
455420
}
456-
return this.testPermissionCaching(sender, permission, pair -> {
421+
return this.threadLocalPermissionCache.testPermissionCaching(sender, permission, pair -> {
457422
return PermissionResult.of(
458423
pair.second().isEmpty() || this.hasPermission(pair.first(), pair.second().permissionString()), pair.second());
459424
});

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

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

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-
}
360+
return this.commandManager.threadLocalPermissionCache.withPermissionCache(() -> {
361+
if (this.childPermitted(root, sender)) {
362+
return new InvalidSyntaxException(
363+
this.commandManager.commandSyntaxFormatter().apply(sender, (List) this.getComponentChain(root), root),
364+
sender, this.getComponentChain(root)
365+
);
366+
}
366367

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

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

387389
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)