cleanup CommandRegistry

core/src/main/java/org/geysermc/geyser/command/CommandRegistry.java

@@ -26,7 +26,6 @@
 package org.geysermc.geyser.command;
 
 import it.unimi.dsi.fastutil.objects.Object2ObjectOpenHashMap;
-import lombok.AllArgsConstructor;
 import org.checkerframework.checker.nullness.qual.NonNull;
 import org.geysermc.geyser.Constants;
 import org.geysermc.geyser.GeyserImpl;
@@ -37,6 +36,7 @@ import org.geysermc.geyser.api.event.lifecycle.GeyserRegisterPermissionsEvent;
 import org.geysermc.geyser.api.extension.Extension;
 import org.geysermc.geyser.api.util.PlatformType;
 import org.geysermc.geyser.api.util.TriState;
+import org.geysermc.geyser.command.GeyserPermission.Result;
 import org.geysermc.geyser.command.defaults.AdvancedTooltipsCommand;
 import org.geysermc.geyser.command.defaults.AdvancementsCommand;
 import org.geysermc.geyser.command.defaults.ConnectionTestCommand;
@@ -63,14 +63,11 @@ import org.incendo.cloud.exception.InvalidCommandSenderException;
 import org.incendo.cloud.exception.InvalidSyntaxException;
 import org.incendo.cloud.exception.NoPermissionException;
 import org.incendo.cloud.exception.NoSuchCommandException;
-import org.incendo.cloud.exception.handling.ExceptionContext;
 import org.incendo.cloud.exception.handling.ExceptionController;
-import org.incendo.cloud.exception.handling.ExceptionHandler;
 import org.incendo.cloud.execution.ExecutionCoordinator;
 
 import java.util.Collections;
 import java.util.HashMap;
-import java.util.List;
 import java.util.Map;
 import java.util.function.BiConsumer;
 
@@ -106,18 +103,13 @@ public class CommandRegistry {
         // Yeet the default exception handlers that the typical cloud implementations provide so that we can perform localization.
         // This is kind of meaningless for our Geyser-Standalone implementation since these handlers are the default exception handlers in that case.
         cloud.exceptionController().clearHandlers();
-        List<GeyserExceptionHandler<?>> exceptionHandlers = List.of(
-                new GeyserExceptionHandler<>(InvalidSyntaxException.class, (src, e) -> src.sendLocaleString("geyser.command.invalid_syntax", e.correctSyntax())),
-                new GeyserExceptionHandler<>(InvalidCommandSenderException.class, (src, e) -> src.sendLocaleString("geyser.command.invalid_sender", e.commandSender().getClass().getSimpleName(), e.requiredSender())),
-                new GeyserExceptionHandler<>(NoPermissionException.class, this::handleNoPermission),
-                new GeyserExceptionHandler<>(NoSuchCommandException.class, (src, e) -> src.sendLocaleString("geyser.command.not_found")),
-                new GeyserExceptionHandler<>(ArgumentParseException.class, (src, e) -> src.sendLocaleString("geyser.command.invalid_argument", e.getCause().getMessage())),
-                new GeyserExceptionHandler<>(CommandExecutionException.class, (src, e) -> handleUnexpectedThrowable(src, e.getCause())),
-                new GeyserExceptionHandler<>(Throwable.class, (src, e) -> handleUnexpectedThrowable(src, e.getCause()))
-        );
-        for (GeyserExceptionHandler<?> handler : exceptionHandlers) {
-            handler.register(cloud);
-        }
+        registerExceptionHandler(InvalidSyntaxException.class, (src, e) -> src.sendLocaleString("geyser.command.invalid_syntax", e.correctSyntax()));
+        registerExceptionHandler(InvalidCommandSenderException.class, (src, e) -> src.sendLocaleString("geyser.command.invalid_sender", e.commandSender().getClass().getSimpleName(), e.requiredSender()));
+        registerExceptionHandler(NoPermissionException.class, CommandRegistry::handleNoPermission);
+        registerExceptionHandler(NoSuchCommandException.class, (src, e) -> src.sendLocaleString("geyser.command.not_found"));
+        registerExceptionHandler(ArgumentParseException.class, (src, e) -> src.sendLocaleString("geyser.command.invalid_argument", e.getCause().getMessage()));
+        registerExceptionHandler(CommandExecutionException.class, (src, e) -> handleUnexpectedThrowable(src, e.getCause()));
+        registerExceptionHandler(Throwable.class, (src, e) -> handleUnexpectedThrowable(src, e.getCause()));
 
         // begin command registration
         registerBuiltInCommand(new HelpCommand(geyser, "help", "geyser.commands.help.desc", "geyser.command.help", "geyser", "geyser.command", this.commands));
@@ -185,6 +177,13 @@ public class CommandRegistry {
         return Collections.unmodifiableMap(this.commands);
     }
 
+    private <E extends Throwable> void registerExceptionHandler(Class<E> type, BiConsumer<GeyserCommandSource, E> handler) {
+        cloud.exceptionController().registerHandler(type, context -> {
+            Throwable unwrapped = ExceptionController.unwrapCompletionException(context.exception());
+            handler.accept(context.context().sender(), type.cast(unwrapped));
+        });
+    }
+
     /**
      * For internal Geyser commands
      */
@@ -196,10 +195,6 @@ public class CommandRegistry {
         register(command, this.extensionCommands.computeIfAbsent(extension, e -> new HashMap<>()));
     }
 
-    public boolean hasPermission(GeyserCommandSource source, String permission) {
-        return cloud.hasPermission(source, permission);
-    }
-
     private void register(GeyserCommand command, Map<String, Command> commands) {
         command.register(cloud);
 
@@ -230,6 +225,11 @@ public class CommandRegistry {
         event.register(Constants.SETTINGS_GAMERULES_PERMISSION, TriState.NOT_SET);
     }
 
+    public boolean hasPermission(GeyserCommandSource source, String permission) {
+        return cloud.hasPermission(source, permission);
+    }
+
+
     /**
      * Returns the description of the given command
      *
@@ -258,22 +258,21 @@ public class CommandRegistry {
         cloud.commandExecutor().executeCommand(source, command);
     }
 
-    private void handleNoPermission(GeyserCommandSource source, NoPermissionException exception) {
-        // we basically recheck bedrock-only and player-only to see if they were the cause of this
-        if (exception.missingPermission() instanceof GeyserPermission permission) {
-            GeyserPermission.Result result = permission.check(source);
-            if (result == GeyserPermission.Result.NOT_BEDROCK) {
+    private static void handleNoPermission(GeyserCommandSource source, NoPermissionException exception) {
+        // custom handling if the source can't use the command because of additional requirements
+        if (exception.permissionResult() instanceof Result result) {
+            if (result.meta() == Result.Meta.NOT_BEDROCK) {
                 source.sendMessage(ChatColor.RED + GeyserLocale.getPlayerLocaleString("geyser.command.bedrock_only", source.locale()));
                 return;
             }
-            if (result == GeyserPermission.Result.NOT_PLAYER) {
+            if (result.meta() == Result.Meta.NOT_PLAYER) {
                 source.sendMessage(ChatColor.RED + GeyserLocale.getPlayerLocaleString("geyser.command.player_only", source.locale()));
                 return;
             }
         } else {
             GeyserLogger logger = GeyserImpl.getInstance().getLogger();
             if (logger.isDebug()) {
-                logger.debug("Expected a GeyserPermission for %s but instead got %s".formatted(exception.currentChain(), exception.missingPermission()));
+                logger.debug("Expected a GeyserPermission.Result for %s but instead got %s from %s".formatted(exception.currentChain(), exception.permissionResult(), exception.missingPermission()));
             }
         }
 
@@ -281,25 +280,8 @@ public class CommandRegistry {
         source.sendLocaleString("geyser.command.permission_fail");
     }
 
-    private void handleUnexpectedThrowable(GeyserCommandSource source, Throwable throwable) {
+    private static void handleUnexpectedThrowable(GeyserCommandSource source, Throwable throwable) {
         source.sendMessage(MinecraftLocale.getLocaleString("command.failed", source.locale())); // java edition translation key
         GeyserImpl.getInstance().getLogger().error("Exception while executing command handler", throwable);
     }
-
-    @AllArgsConstructor
-    private static class GeyserExceptionHandler<E extends Throwable> implements ExceptionHandler<GeyserCommandSource, E> {
-
-        final Class<E> type;
-        final BiConsumer<GeyserCommandSource, E> handler;
-
-        void register(CommandManager<GeyserCommandSource> manager) {
-            manager.exceptionController().registerHandler(type, this);
-        }
-
-        @Override
-        public void handle(@NonNull ExceptionContext context) throws Throwable {
-            Throwable unwrapped = ExceptionController.unwrapCompletionException(context.exception());
-            handler.accept((GeyserCommandSource) context.context().sender(), type.cast(unwrapped));
-        }
-    }
 }

core/src/main/java/org/geysermc/geyser/command/GeyserPermission.java

@@ -33,33 +33,48 @@ import org.incendo.cloud.permission.Permission;
 import org.incendo.cloud.permission.PermissionResult;
 import org.incendo.cloud.permission.PredicatePermission;
 
+import static org.geysermc.geyser.command.GeyserPermission.Result.Meta;
+
 @AllArgsConstructor
 public class GeyserPermission implements PredicatePermission<GeyserCommandSource> {
 
+    /**
+     * True if this permission requires the command source to be a bedrock player
+     */
     private final boolean bedrockOnly;
+
+    /**
+     * True if this permission requires the command source to be any player
+     */
     private final boolean playerOnly;
+
+    /**
+     * The permission node that the command source must have
+     */
     private final String permission;
+
+    /**
+     * The command manager to delegate permission checks to
+     */
     private final CommandManager<GeyserCommandSource> manager;
 
-    public Result check(GeyserCommandSource source) {
-        if (permission.isBlank()) {
-            return Result.ALLOWED;
-        }
+    @Override
+    public @NonNull PermissionResult testPermission(@NonNull GeyserCommandSource source) {
         if (bedrockOnly) {
             if (source.connection() == null) {
-                return Result.NOT_BEDROCK;
+                return new Result(Meta.NOT_BEDROCK);
             }
             // connection is present -> it is a player -> playerOnly is irrelevant
         } else if (playerOnly) {
             if (source.isConsole()) {
-                return Result.NOT_PLAYER; // must be a player but is console
+                return new Result(Meta.NOT_PLAYER); // must be a player but is console
             }
         }
 
-        if (manager.hasPermission(source, permission)) {
-            return Result.ALLOWED;
+        if (permission.isBlank() || manager.hasPermission(source, permission)) {
+            return new Result(Meta.ALLOWED);
         }
-        return Result.NO_PERMISSION;
+        return new Result(Meta.NO_PERMISSION);
     }
 
     @Override
@@ -67,35 +82,55 @@ public class GeyserPermission implements PredicatePermission<GeyserCommandSource
         return CloudKey.cloudKey(permission);
     }
 
-    @Override
-    public @NonNull PermissionResult testPermission(@NonNull GeyserCommandSource sender) {
-        return check(sender).toPermission(permission);
-    }
+    /**
+     * Basic implementation of cloud's {@link PermissionResult} that delegates to the more informative {@link Meta}.
+     */
+    public final class Result implements PermissionResult {
 
-    public enum Result {
+        private final Meta meta;
+
+        private Result(Meta meta) {
+            this.meta = meta;
+        }
+
+        public Meta meta() {
+            return meta;
+        }
+
+        @Override
+        public boolean allowed() {
+            return meta == Meta.ALLOWED;
+        }
+
+        @Override
+        public @NonNull Permission permission() {
+            return GeyserPermission.this;
+        }
 
         /**
-         * The source must be a bedrock player, but is not.
+         * More detailed explanation of whether the permission check passed.
          */
-        NOT_BEDROCK,
+        public enum Meta {
 
-        /**
-         * The source must be a player, but is not.
-         */
-        NOT_PLAYER,
+            /**
+             * The source must be a bedrock player, but is not.
+             */
+            NOT_BEDROCK,
 
-        /**
-         * The source does not have a required permission node.
-         */
-        NO_PERMISSION,
+            /**
+             * The source must be a player, but is not.
+             */
+            NOT_PLAYER,
 
-        /**
-         * The source meets all requirements.
-         */
-        ALLOWED;
+            /**
+             * The source does not have a required permission node.
+             */
+            NO_PERMISSION,
 
-        public PermissionResult toPermission(String  permission) {
-            return PermissionResult.of(this == ALLOWED, Permission.of(permission));
+            /**
+             * The source meets all requirements.
+             */
+            ALLOWED;
         }
     }
 }