Merge pull request #3387 from Multiverse/fix/selectors-error-spam

Fix selectors spamming stacktrace and improve invalid format message

Author: benwoo1110

src/main/java/org/mvplugins/multiverse/core/command/MVCommandContexts.java

@@ -11,6 +11,7 @@
 import co.aikar.commands.PaperCommandContexts;
 import co.aikar.commands.contexts.ContextResolver;
 import com.google.common.base.Strings;
+import io.vavr.control.Option;
 import io.vavr.control.Try;
 import jakarta.inject.Inject;
 import org.bukkit.Bukkit;
@@ -272,10 +273,17 @@ private IssuerAwareContextBuilder<Player> playerContextBuilder() {
     private IssuerAwareContextBuilder<Player[]> playerArrayContextBuilder() {
         return new IssuerAwareContextBuilder<Player[]>()
                 .fromPlayer((context, player) -> new Player[]{player})
-                .fromInput((context, input) -> {
-                    Player[] players = PlayerFinder.getMulti(input, context.getSender()).toArray(new Player[0]);
-                    return (players.length == 0) ? null : players;
-                })
+                .fromInput((context, input) -> PlayerFinder
+                        .tryGetMulti(input, context.getSender())
+                        .map(list -> list.toArray(new Player[0]))
+                        .map(arr -> (arr.length == 0) ? null : arr)
+                        .getOrElseThrow(failure -> {
+                            if (failure instanceof MVInvalidCommandArgument mvFailure) {
+                                throw mvFailure;
+                            }
+                            throw new InvalidCommandArgument(failure.getLocalizedMessage() + " "
+                                    + Option.of(failure.getCause()).map(Throwable::getLocalizedMessage).getOrElse(""));
+                        }))
                 .issuerOnlyFailMessage((context) -> Message.of("This command can only be used by a player."))
                 .issuerAwareInputFailMessage((context, input) -> Message.of("Invalid player: " + input + ". Either specify an online player or use this command as a player."))
                 .inputOnlyFailMessage((context, input) -> {

src/main/java/org/mvplugins/multiverse/core/utils/PlayerFinder.java

@@ -1,12 +1,14 @@
 package org.mvplugins.multiverse.core.utils;
 
 import java.util.ArrayList;
+import java.util.Collections;
 import java.util.List;
 import java.util.UUID;
 import java.util.stream.Collectors;
 
 import com.dumptruckman.minecraft.util.Logging;
 import com.google.common.base.Strings;
+import io.vavr.control.Try;
 import org.bukkit.Bukkit;
 import org.bukkit.command.CommandSender;
 import org.bukkit.entity.Player;
@@ -76,22 +78,33 @@ public final class PlayerFinder {
     public static @NotNull List<Player> getMulti(@Nullable String playerIdentifiers,
                                                  @NotNull CommandSender sender
     ) {
-        List<Player> playerResults = new ArrayList<>();
+        return tryGetMulti(playerIdentifiers, sender)
+                .getOrElse(Collections.emptyList());
+    }
 
+    /**
+     * Get multiple {@link Player} based on many identifiers of name UUID or selector.
+     *
+     * @param playerIdentifiers An identifier of multiple names, UUIDs or selectors, separated by comma.
+     * @param sender            Target sender for selector.
+     * @return A list of all the {@link Player} found.
+     *
+     * @since 5.4
+     */
+    @ApiStatus.AvailableSince("5.4")
+    public static @NotNull Try<@NotNull List<Player>> tryGetMulti(@Nullable String playerIdentifiers,
+                                                                   @NotNull CommandSender sender
+    ) {
         if (playerIdentifiers == null || Strings.isNullOrEmpty(playerIdentifiers)) {
-            return playerResults;
+            return Try.success(Collections.emptyList());
         }
 
         // TODO: Currently just assume entire string is a selector. Add support for comma seperated mixture of names, uuids and selectors
         if (isSelector(playerIdentifiers)) {
-            Logging.finer("Using selector: %s", playerIdentifiers);
-            List<Player> targetPlayers = getMultiBySelector(playerIdentifiers, sender);
-            if (targetPlayers != null) {
-                playerResults.addAll(targetPlayers);
-            }
-            return playerResults;
+            return tryGetMultiBySelector(playerIdentifiers, sender);
         }
 
+        List<Player> playerResults = new ArrayList<>();
         String[] playerIdentifierArray = REPatterns.COMMA.split(playerIdentifiers);
         for (String playerIdentifier : playerIdentifierArray) {
             Player targetPlayer = getByName(playerIdentifier);
@@ -104,12 +117,13 @@ public final class PlayerFinder {
                 playerResults.add(targetPlayer);
                 continue;
             }
-            List<Player> targetPlayers = getMultiBySelector(playerIdentifier, sender);
-            if (targetPlayers != null) {
-                playerResults.addAll(targetPlayers);
+            Try<@NotNull List<Player>> selectorParseResult = tryGetMultiBySelector(playerIdentifier, sender);
+            if  (selectorParseResult.isFailure()) {
+                return Try.failure(selectorParseResult.getCause());
             }
+            playerResults.addAll(selectorParseResult.getOrElse(Collections.emptyList()));
         }
-        return playerResults;
+        return Try.success(playerResults);
     }
 
     /**
@@ -176,7 +190,7 @@ public static Player getByName(@NotNull String playerName) {
                                                  @NotNull CommandSender sender
     ) {
         List<Player> matchedPlayers = getMultiBySelector(playerSelector, sender);
-        if (matchedPlayers == null || matchedPlayers.isEmpty()) {
+        if (matchedPlayers.isEmpty()) {
             Logging.fine("No player found with selector '%s' for %s.", playerSelector, sender.getName());
             return null;
         }
@@ -195,22 +209,36 @@ public static Player getByName(@NotNull String playerName) {
      * @param sender            Target sender for selector.
      * @return A list of all the {@link Player} found.
      */
-    public static @Nullable List<Player> getMultiBySelector(@NotNull String playerSelector,
+    public static @NotNull List<Player> getMultiBySelector(@NotNull String playerSelector,
                                                             @NotNull CommandSender sender
+    ) {
+        return tryGetMultiBySelector(playerSelector, sender)
+                .onFailure(throwable -> Logging.warning(
+                        "Error selecting entities with selector '%s' for %s: %s",
+                        playerSelector, sender.getName(), throwable.getMessage()
+                ))
+                .getOrElse(Collections::emptyList);
+    }
+
+    /**
+     * Get multiple {@link Player} based on <a href="https://minecraft.gamepedia.com/Commands#Target_selectors">vanilla selectors</a>.
+     *
+     * @param playerSelector    A target selector, usually starts with an '@'.
+     * @param sender            Target sender for selector.
+     * @return A list of all the {@link Player} found.
+     *
+     * @since 5.4
+     */
+    @ApiStatus.AvailableSince("5.4")
+    public static @NotNull Try<@NotNull List<Player>> tryGetMultiBySelector(@NotNull String playerSelector,
+                                                                            @NotNull CommandSender sender
     ) {
         if (playerSelector.charAt(0) != '@') {
-            return null;
-        }
-        try {
-            return Bukkit.selectEntities(sender, playerSelector).stream()
-                    .filter(e -> e instanceof Player)
-                    .map(e -> ((Player) e))
-                    .collect(Collectors.toList());
-        } catch (IllegalArgumentException e) {
-            Logging.warning("An error occurred while parsing selector '%s' for %s. Is it is the correct format?",
-                    playerSelector, sender.getName());
-            e.printStackTrace();
-            return null;
+            return Try.success(Collections.emptyList());
         }
+        return Try.of(() -> Bukkit.selectEntities(sender, playerSelector).stream()
+                .filter(e -> e instanceof Player)
+                .map(e -> ((Player) e))
+                .collect(Collectors.toList()));
     }
 }