From aafa2dd6d07ef2948289f58b4ce6eecc6429b278 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20S=C3=B6derberg?= Date: Sat, 13 Jan 2024 14:24:21 +0100 Subject: [PATCH] feat(core): make `CommandComponent` immutable The "owning command" has been moved into the command node, as the node is always present when we access the command anyway. This allows us to only have final fields in the command component. This also means that we no longer need to copy the components before storing them in the command tree, and we can instead re-use the components. --- .../feature/BuilderDecoratorTest.java | 4 +- .../feature/DescriptionMapperTest.java | 2 +- .../java/cloud/commandframework/Command.java | 15 +++--- .../commandframework/CommandComponent.java | 44 ---------------- .../cloud/commandframework/CommandTree.java | 52 +++++++++---------- .../TypedCommandComponent.java | 14 ----- .../help/StandardHelpHandler.java | 16 +++--- .../internal/CommandNode.java | 23 ++++++++ .../commandframework/CommandBeanTest.java | 2 +- .../node/LiteralBrigadierNodeFactory.java | 2 +- .../bukkit/BukkitCommand.java | 4 +- 11 files changed, 68 insertions(+), 110 deletions(-) diff --git a/cloud-annotations/src/test/java/cloud/commandframework/annotations/feature/BuilderDecoratorTest.java b/cloud-annotations/src/test/java/cloud/commandframework/annotations/feature/BuilderDecoratorTest.java index 12cee485b..aed869698 100644 --- a/cloud-annotations/src/test/java/cloud/commandframework/annotations/feature/BuilderDecoratorTest.java +++ b/cloud-annotations/src/test/java/cloud/commandframework/annotations/feature/BuilderDecoratorTest.java @@ -60,7 +60,7 @@ void testDefaultDescription() { this.annotationParser.parse(new TestClass()); // Assert - assertThat(this.commandManager.commandTree().getNamedNode("command").component().owningCommand().commandDescription()) + assertThat(this.commandManager.commandTree().getNamedNode("command").command().commandDescription()) .isEqualTo(CommandDescription.commandDescription("default description")); } @@ -74,7 +74,7 @@ void testDefaultPermission() { this.annotationParser.parse(new TestClass()); // Assert - assertThat(this.commandManager.commandTree().getNamedNode("command").component().owningCommand().commandPermission()) + assertThat(this.commandManager.commandTree().getNamedNode("command").command().commandPermission()) .isEqualTo(Permission.of("default.permission")); } diff --git a/cloud-annotations/src/test/java/cloud/commandframework/annotations/feature/DescriptionMapperTest.java b/cloud-annotations/src/test/java/cloud/commandframework/annotations/feature/DescriptionMapperTest.java index 7e28dd848..9cd7655de 100644 --- a/cloud-annotations/src/test/java/cloud/commandframework/annotations/feature/DescriptionMapperTest.java +++ b/cloud-annotations/src/test/java/cloud/commandframework/annotations/feature/DescriptionMapperTest.java @@ -63,7 +63,7 @@ void test() { // Assert final CommandNode node = this.commandManager.commandTree().getNamedNode("command"); final CommandNode argumentNode = node.children().get(0); - assertThat(argumentNode.component().owningCommand().commandDescription().description()).isEqualTo(testDescription); + assertThat(argumentNode.command().commandDescription().description()).isEqualTo(testDescription); assertThat(node.children().get(0).component().description()).isEqualTo(testDescription); } diff --git a/cloud-core/src/main/java/cloud/commandframework/Command.java b/cloud-core/src/main/java/cloud/commandframework/Command.java index a75d38094..da6430ca0 100644 --- a/cloud-core/src/main/java/cloud/commandframework/Command.java +++ b/cloud-core/src/main/java/cloud/commandframework/Command.java @@ -1442,8 +1442,6 @@ private Builder( /** * Adds the given {@code argument} to the command. - *

- * The component will be copied using {@link CommandComponent#copy()} before being inserted into the command tree. * * @param argument argument to add * @return new builder instance with the command argument inserted into the argument list @@ -1454,7 +1452,7 @@ private Builder( final @NonNull CommandComponent argument ) { final List> commandComponents = new ArrayList<>(this.commandComponents); - commandComponents.add(argument.copy()); + commandComponents.add(argument); return new Builder<>( this.commandManager, this.commandMeta, @@ -1469,8 +1467,6 @@ private Builder( /** * Adds the given {@code argument} to the command. - *

- * The component will be copied using {@link CommandComponent#copy()} before being inserted into the command tree. * * @param builder builder that builds the component to add * @return new builder instance with the command argument inserted into the argument list @@ -2237,7 +2233,7 @@ private Builder( /** * Makes the current command be a proxy of the supplied command. T *

- * his means that all the proxied command's variable command arguments will be inserted into this + * This means that all the proxied command's variable command arguments will be inserted into this * builder instance, in the order they are declared in the proxied command. Furthermore, * the proxied command's command handler will be shown by the command that is currently * being built. If the current command builder does not have a permission node set, this @@ -2252,12 +2248,15 @@ private Builder( if (component.type() == CommandComponent.ComponentType.LITERAL) { continue; } - final CommandComponent componentCopy = component.copy(); - builder = builder.argument(componentCopy); + builder = builder.argument(component); } if (this.permission.permissionString().isEmpty()) { builder = builder.permission(command.commandPermission()); } + final Class senderType = command.senderType().orElse(null); + if (senderType != null) { + builder = builder.senderType(senderType); + } return builder.handler(command.commandExecutionHandler); } diff --git a/cloud-core/src/main/java/cloud/commandframework/CommandComponent.java b/cloud-core/src/main/java/cloud/commandframework/CommandComponent.java index b11656ce5..746f0004e 100644 --- a/cloud-core/src/main/java/cloud/commandframework/CommandComponent.java +++ b/cloud-core/src/main/java/cloud/commandframework/CommandComponent.java @@ -42,7 +42,6 @@ import java.util.Collections; import java.util.Objects; import org.apiguardian.api.API; -import org.checkerframework.checker.nullness.qual.MonotonicNonNull; import org.checkerframework.checker.nullness.qual.NonNull; import org.checkerframework.checker.nullness.qual.Nullable; import org.checkerframework.common.returnsreceiver.qual.This; @@ -68,8 +67,6 @@ public class CommandComponent implements Comparable>, Pre private final SuggestionProvider suggestionProvider; private final Collection<@NonNull ComponentPreprocessor> componentPreprocessors; - private Command owningCommand; - /** * Creates a new mutable builder. * @@ -247,29 +244,6 @@ public final boolean hasDefaultValue() { return this.suggestionProvider; } - /** - * Returns the command that owns this component. - * - * @return the owning command - */ - public final @MonotonicNonNull Command owningCommand() { - return this.owningCommand; - } - - /** - * Sets the command that owns this component. - * - * @param owningCommand the command - * @throws IllegalStateException if {@link #owningCommand()} has already been set - */ - @API(status = API.Status.INTERNAL, consumers = "cloud.commandframework.CommandTree") - final void owningCommand(final @NonNull Command owningCommand) { - if (this.owningCommand != null) { - throw new IllegalStateException("Cannot replace owning command"); - } - this.owningCommand = owningCommand; - } - /** * Registers a new preprocessor. * @@ -347,24 +321,6 @@ public final boolean equals(final Object o) { ); } - /** - * Returns a copy of the component. - * - * @return copy of the component - */ - public @NonNull CommandComponent copy() { - return new CommandComponent<>( - this.name(), - this.parser(), - this.valueType(), - this.description(), - this.type(), - this.defaultValue(), - this.suggestionProvider(), - this.preprocessors() - ); - } - @Override public final int compareTo(final @NonNull CommandComponent other) { if (this.type() == ComponentType.LITERAL) { diff --git a/cloud-core/src/main/java/cloud/commandframework/CommandTree.java b/cloud-core/src/main/java/cloud/commandframework/CommandTree.java index 3322f6217..a674aee27 100644 --- a/cloud-core/src/main/java/cloud/commandframework/CommandTree.java +++ b/cloud-core/src/main/java/cloud/commandframework/CommandTree.java @@ -245,7 +245,7 @@ private CommandTree(final @NonNull CommandManager commandManager) { // There are 0 or more static arguments as children. No variable child arguments are present if (root.children().isEmpty()) { final CommandComponent rootComponent = root.component(); - if (rootComponent == null || rootComponent.owningCommand() == null || !commandInput.isEmpty()) { + if (rootComponent == null || root.command() == null || !commandInput.isEmpty()) { // Too many arguments. We have a unique path, so we can send the entire context return CompletableFutures.failedFuture( new InvalidSyntaxException( @@ -255,7 +255,7 @@ private CommandTree(final @NonNull CommandManager commandManager) { ) ); } - return CompletableFuture.completedFuture(rootComponent.owningCommand()); + return CompletableFuture.completedFuture(root.command()); } CompletableFuture> childCompletable = CompletableFuture.completedFuture(null); @@ -318,8 +318,8 @@ private CommandTree(final @NonNull CommandManager commandManager) { // If we couldn't match a child, check if there's a command attached and execute it final CommandComponent rootComponent = root.component(); - if (rootComponent != null && rootComponent.owningCommand() != null && commandInput.isEmpty()) { - final Command command = rootComponent.owningCommand(); + if (rootComponent != null && root.command() != null && commandInput.isEmpty()) { + final Command command = root.command(); if (!this.commandManager().hasPermission( commandContext.sender(), command.commandPermission() @@ -332,7 +332,7 @@ private CommandTree(final @NonNull CommandManager commandManager) { ) ); } - return CompletableFuture.completedFuture(rootComponent.owningCommand()); + return CompletableFuture.completedFuture(root.command()); } // We know that there's no command, and we also cannot match any of the children @@ -410,25 +410,23 @@ private CommandTree(final @NonNull CommandManager commandManager) { argumentValue = defaultValue.evaluateDefault(commandContext); } } else if (!child.component().required()) { - if (childComponent.owningCommand() == null) { + if (child.command() == null) { // If there are multiple children with different owning commands then it's ambiguous and // not allowed, therefore we're able to pick any child command, as long as we can find it CommandNode node = child; while (!node.isLeaf()) { node = node.children().get(0); final CommandComponent nodeComponent = node.component(); - if (nodeComponent != null && nodeComponent.owningCommand() != null) { - childComponent.owningCommand(nodeComponent.owningCommand()); + if (nodeComponent != null && node.command() != null) { + child.command(node.command()); } } } - return CompletableFuture.completedFuture(childComponent.owningCommand()); + return CompletableFuture.completedFuture(child.command()); } else if (child.isLeaf()) { final CommandComponent rootComponent = root.component(); - if (rootComponent == null || rootComponent.owningCommand() == null) { - final List> components = Objects.requireNonNull( - childComponent.owningCommand() - ).components(); + if (rootComponent == null || root.command() == null) { + final List> components = Objects.requireNonNull(child.command()).components(); return CompletableFutures.failedFuture( new InvalidSyntaxException( this.commandManager.commandSyntaxFormatter() @@ -439,7 +437,7 @@ private CommandTree(final @NonNull CommandManager commandManager) { ); } - final Command command = rootComponent.owningCommand(); + final Command command = root.command(); if (this.commandManager().hasPermission(commandContext.sender(), command.commandPermission())) { return CompletableFuture.completedFuture(command); } @@ -453,7 +451,7 @@ private CommandTree(final @NonNull CommandManager commandManager) { } else { // The child is not a leaf, but may have an intermediary executor, attempt to use it final CommandComponent rootComponent = root.component(); - if (rootComponent == null || rootComponent.owningCommand() == null) { + if (rootComponent == null || root.command() == null) { // Child does not have a command, and so we cannot proceed return CompletableFutures.failedFuture( new InvalidSyntaxException( @@ -466,7 +464,7 @@ private CommandTree(final @NonNull CommandManager commandManager) { } // If the sender has permission to use the command, then we're completely done - final Command command = Objects.requireNonNull(rootComponent.owningCommand()); + final Command command = Objects.requireNonNull(root.command()); if (this.commandManager().hasPermission(commandContext.sender(), command.commandPermission())) { return CompletableFuture.completedFuture(command); } @@ -501,7 +499,7 @@ private CommandTree(final @NonNull CommandManager commandManager) { commandContext.store(component.name(), value); if (child.isLeaf()) { if (commandInput.isEmpty()) { - return CompletableFuture.completedFuture(component.owningCommand()); + return CompletableFuture.completedFuture(child.command()); } return CompletableFutures.failedFuture( new InvalidSyntaxException( @@ -932,14 +930,14 @@ public void insertCommand(final @NonNull Command command) { final CommandComponent nodeComponent = node.component(); if (nodeComponent != null) { - if (nodeComponent.owningCommand() != null) { + if (node.command() != null) { throw new IllegalStateException(String.format( "Duplicate command chains detected. Node '%s' already has an owning command (%s)", - node, nodeComponent.owningCommand() + node, node.command() )); } - nodeComponent.owningCommand(command); + node.command(command); } this.verifyAndRegister(); @@ -1022,10 +1020,10 @@ public void verifyAndRegister() { // Verify that all leaf nodes have command registered this.getLeaves(this.internalTree).forEach(leaf -> { - if (leaf.component().owningCommand() == null) { + if (leaf.command() == null) { throw new NoCommandInLeafException(leaf.component()); } else { - final Command owningCommand = leaf.component().owningCommand(); + final Command owningCommand = leaf.command(); this.commandManager.commandRegistrationHandler().registerCommand(owningCommand); } }); @@ -1041,8 +1039,8 @@ public void verifyAndRegister() { */ @SuppressWarnings("unchecked") private void propagateRequirements(final @NonNull CommandNode leafNode) { - final Permission commandPermission = leafNode.component().owningCommand().commandPermission(); - Class senderType = leafNode.component().owningCommand().senderType().orElse(null); + final Permission commandPermission = leafNode.command().commandPermission(); + Class senderType = leafNode.command().senderType().orElse(null); if (senderType == null) { senderType = Object.class; } @@ -1065,8 +1063,8 @@ private void propagateRequirements(final @NonNull CommandNode leafNode) { } /* Now also check if there's a command handler attached to an upper level node */ - if (commandArgumentNode.component() != null && commandArgumentNode.component().owningCommand() != null) { - final Command command = commandArgumentNode.component().owningCommand(); + if (commandArgumentNode.component() != null && commandArgumentNode.command() != null) { + final Command command = commandArgumentNode.command(); if (this.commandManager().settings().get(ManagerSetting.ENFORCE_INTERMEDIARY_PERMISSIONS)) { permission = command.commandPermission(); } else { @@ -1230,7 +1228,7 @@ void deleteRecursively( } final @Nullable CommandComponent component = node.component(); - final @Nullable Command owner = component == null ? null : component.owningCommand(); + final @Nullable Command owner = component == null ? null : node.command(); if (owner != null) { commandConsumer.accept(owner); } diff --git a/cloud-core/src/main/java/cloud/commandframework/TypedCommandComponent.java b/cloud-core/src/main/java/cloud/commandframework/TypedCommandComponent.java index 59b1a989f..bbc0ee0e9 100644 --- a/cloud-core/src/main/java/cloud/commandframework/TypedCommandComponent.java +++ b/cloud-core/src/main/java/cloud/commandframework/TypedCommandComponent.java @@ -72,18 +72,4 @@ public final class TypedCommandComponent extends CommandComponent imple public @NonNull CloudKey key() { return CloudKey.of(this.name(), this.valueType()); } - - @Override - public @NonNull TypedCommandComponent copy() { - return new TypedCommandComponent<>( - this.name(), - this.parser(), - this.valueType(), - this.description(), - this.type(), - this.defaultValue(), - this.suggestionProvider(), - this.preprocessors() - ); - } } diff --git a/cloud-core/src/main/java/cloud/commandframework/help/StandardHelpHandler.java b/cloud-core/src/main/java/cloud/commandframework/help/StandardHelpHandler.java index ba900d0ab..12896422d 100644 --- a/cloud-core/src/main/java/cloud/commandframework/help/StandardHelpHandler.java +++ b/cloud-core/src/main/java/cloud/commandframework/help/StandardHelpHandler.java @@ -140,17 +140,15 @@ public StandardHelpHandler( ++index; traversedNodes.add(head.component()); - if (head.component() != null && head.component().owningCommand() != null) { + if (head.component() != null && head.command() != null) { if (head.isLeaf() || index == queryFragments.size()) { - if (this.commandManager.hasPermission(query.sender(), head.component() - .owningCommand() - .commandPermission())) { + if (this.commandManager.hasPermission(query.sender(), head.command().commandPermission())) { return VerboseCommandResult.of( query, CommandEntry.of( - head.component().owningCommand(), + head.command(), this.commandManager.commandSyntaxFormatter() - .apply(query.sender(), head.component().owningCommand().components(), null) + .apply(query.sender(), head.command().components(), null) ) ); } @@ -193,9 +191,9 @@ public StandardHelpHandler( } final List> traversedNodesSub = new LinkedList<>(traversedNodes); - if (child.component() == null || child.component().owningCommand() == null + if (child.component() == null || child.command() == null || this.commandManager.hasPermission(query.sender(), - child.component().owningCommand().commandPermission() + child.command().commandPermission() )) { traversedNodesSub.add(child.component()); childSuggestions.add(this.commandManager.commandSyntaxFormatter() @@ -241,7 +239,7 @@ protected boolean isNodeVisible(final @NonNull CommandNode node) { /* Check node is itself a command that is visible */ final CommandComponent component = node.component(); if (component != null) { - final Command owningCommand = component.owningCommand(); + final Command owningCommand = node.command(); if (owningCommand != null && this.commandFilter.test(owningCommand)) { return true; } diff --git a/cloud-core/src/main/java/cloud/commandframework/internal/CommandNode.java b/cloud-core/src/main/java/cloud/commandframework/internal/CommandNode.java index bbe8428f5..56d9ad503 100644 --- a/cloud-core/src/main/java/cloud/commandframework/internal/CommandNode.java +++ b/cloud-core/src/main/java/cloud/commandframework/internal/CommandNode.java @@ -23,6 +23,7 @@ // package cloud.commandframework.internal; +import cloud.commandframework.Command; import cloud.commandframework.CommandComponent; import java.util.Collections; import java.util.Comparator; @@ -52,6 +53,7 @@ public final class CommandNode { private final List> children = new LinkedList<>(); private final CommandComponent component; private CommandNode parent; + private Command command; /** * Creates a new command node @@ -135,6 +137,27 @@ public boolean isLeaf() { return this.component; } + /** + * Returns the command that the {@link #component()} belongs to, if the {@link #component()} is executable. + * + * @return the command + */ + public @MonotonicNonNull Command command() { + return this.command; + } + + /** + * Sets the executable command of this node. + * + * @param command command + */ + public void command(final @NonNull Command command) { + if (this.command != null) { + throw new IllegalStateException("Cannot replace owning command"); + } + this.command = command; + } + /** * Returns the parent node * diff --git a/cloud-core/src/test/java/cloud/commandframework/CommandBeanTest.java b/cloud-core/src/test/java/cloud/commandframework/CommandBeanTest.java index 4db5d55bb..2d9aadab2 100644 --- a/cloud-core/src/test/java/cloud/commandframework/CommandBeanTest.java +++ b/cloud-core/src/test/java/cloud/commandframework/CommandBeanTest.java @@ -60,7 +60,7 @@ void testCommandBeanRegistration() { final CommandComponent component = node.children().get(0).component(); assertThat(component).isNotNull(); - final Command command = component.owningCommand(); + final Command command = node.children().get(0).command(); assertThat(command).isNotNull(); assertThat(command.nonFlagArguments().get(0).aliases()).containsExactly("t", "test"); assertThat(command.commandMeta().getOrDefault(META_KEY, "otherValue")).isEqualTo("value"); diff --git a/cloud-minecraft/cloud-brigadier/src/main/java/cloud/commandframework/brigadier/node/LiteralBrigadierNodeFactory.java b/cloud-minecraft/cloud-brigadier/src/main/java/cloud/commandframework/brigadier/node/LiteralBrigadierNodeFactory.java index f9aa6d174..85a86a45d 100644 --- a/cloud-minecraft/cloud-brigadier/src/main/java/cloud/commandframework/brigadier/node/LiteralBrigadierNodeFactory.java +++ b/cloud-minecraft/cloud-brigadier/src/main/java/cloud/commandframework/brigadier/node/LiteralBrigadierNodeFactory.java @@ -319,7 +319,7 @@ private void updateExecutes( if (this.cloudBrigadierManager.settings().get(BrigadierSetting.FORCE_EXECUTABLE) || node.isLeaf() || node.component().optional() - || node.component().owningCommand() != null + || node.command() != null || node.children().stream().map(CommandNode::component) .filter(Objects::nonNull).anyMatch(CommandComponent::optional)) { builder.executes(executor); diff --git a/cloud-minecraft/cloud-bukkit/src/main/java/cloud/commandframework/bukkit/BukkitCommand.java b/cloud-minecraft/cloud-bukkit/src/main/java/cloud/commandframework/bukkit/BukkitCommand.java index 7652cb188..a2e02f909 100644 --- a/cloud-minecraft/cloud-bukkit/src/main/java/cloud/commandframework/bukkit/BukkitCommand.java +++ b/cloud-minecraft/cloud-bukkit/src/main/java/cloud/commandframework/bukkit/BukkitCommand.java @@ -81,9 +81,7 @@ final class BukkitCommand extends org.bukkit.command.Command implements Plugi this.command = command; this.manager = manager; this.cloudCommand = cloudCommand; - if (this.command.owningCommand() != null) { - this.setPermission(this.command.owningCommand().commandPermission().toString()); - } + this.setPermission(cloudCommand.commandPermission().toString()); this.disabled = false; }