Merge pull request #757 from python-discord/feat/backend/b131/error-handling

Handle more types of CommandError

2 files changed, 149 insertions, 102 deletions

diff --git a/bot/cogs/error_handler.py b/bot/cogs/error_handler.py
index 0abb7e521..864450139 100644
--- a/bot/cogs/error_handler.py
+++ b/bot/cogs/error_handler.py
@@ -1,20 +1,8 @@
 import contextlib
 import logging
+import typing as t
 
-from discord.ext.commands import (
-    BadArgument,
-    BotMissingPermissions,
-    CheckFailure,
-    CommandError,
-    CommandInvokeError,
-    CommandNotFound,
-    CommandOnCooldown,
-    DisabledCommand,
-    MissingPermissions,
-    NoPrivateMessage,
-    UserInputError,
-)
-from discord.ext.commands import Cog, Context
+from discord.ext.commands import Cog, Command, Context, errors
 from sentry_sdk import push_scope
 
 from bot.api import ResponseCodeError
@@ -32,118 +20,177 @@ class ErrorHandler(Cog):
         self.bot = bot
 
     @Cog.listener()
-    async def on_command_error(self, ctx: Context, e: CommandError) -> None:
+    async def on_command_error(self, ctx: Context, e: errors.CommandError) -> None:
         """
         Provide generic command error handling.
 
-        Error handling is deferred to any local error handler, if present.
-
-        Error handling emits a single error response, prioritized as follows:
-            1. If the name fails to match a command but matches a tag, the tag is invoked
-            2. Send a BadArgument error message to the invoking context & invoke the command's help
-            3. Send a UserInputError error message to the invoking context & invoke the command's help
-            4. Send a NoPrivateMessage error message to the invoking context
-            5. Send a BotMissingPermissions error message to the invoking context
-            6. Log a MissingPermissions error, no message is sent
-            7. Send a InChannelCheckFailure error message to the invoking context
-            8. Log CheckFailure, CommandOnCooldown, and DisabledCommand errors, no message is sent
-            9. For CommandInvokeErrors, response is based on the type of error:
-                * 404: Error message is sent to the invoking context
-                * 400: Log the resopnse JSON, no message is sent
-                * 500 <= status <= 600: Error message is sent to the invoking context
-            10. Otherwise, handling is deferred to `handle_unexpected_error`
+        Error handling is deferred to any local error handler, if present. This is done by
+        checking for the presence of a `handled` attribute on the error.
+
+        Error handling emits a single error message in the invoking context `ctx` and a log message,
+        prioritised as follows:
+
+        1. If the name fails to match a command but matches a tag, the tag is invoked
+            * If CommandNotFound is raised when invoking the tag (determined by the presence of the
+              `invoked_from_error_handler` attribute), this error is treated as being unexpected
+              and therefore sends an error message
+            * Commands in the verification channel are ignored
+        2. UserInputError: see `handle_user_input_error`
+        3. CheckFailure: see `handle_check_failure`
+        4. CommandOnCooldown: send an error message in the invoking context
+        5. ResponseCodeError: see `handle_api_error`
+        6. Otherwise, if not a DisabledCommand, handling is deferred to `handle_unexpected_error`
         """
         command = ctx.command
-        parent = None
 
+        if hasattr(e, "handled"):
+            log.trace(f"Command {command} had its error already handled locally; ignoring.")
+            return
+
+        # Try to look for a tag with the command's name if the command isn't found.
+        if isinstance(e, errors.CommandNotFound) and not hasattr(ctx, "invoked_from_error_handler"):
+            if ctx.channel.id != Channels.verification:
+                await self.try_get_tag(ctx)
+                return  # Exit early to avoid logging.
+        elif isinstance(e, errors.UserInputError):
+            await self.handle_user_input_error(ctx, e)
+        elif isinstance(e, errors.CheckFailure):
+            await self.handle_check_failure(ctx, e)
+        elif isinstance(e, errors.CommandOnCooldown):
+            await ctx.send(e)
+        elif isinstance(e, errors.CommandInvokeError):
+            if isinstance(e.original, ResponseCodeError):
+                await self.handle_api_error(ctx, e.original)
+            else:
+                await self.handle_unexpected_error(ctx, e.original)
+            return  # Exit early to avoid logging.
+        elif not isinstance(e, errors.DisabledCommand):
+            # ConversionError, MaxConcurrencyReached, ExtensionError
+            await self.handle_unexpected_error(ctx, e)
+            return  # Exit early to avoid logging.
+
+        log.debug(
+            f"Command {command} invoked by {ctx.message.author} with error "
+            f"{e.__class__.__name__}: {e}"
+        )
+
+    async def get_help_command(self, command: t.Optional[Command]) -> t.Tuple:
+        """Return the help command invocation args to display help for `command`."""
+        parent = None
         if command is not None:
             parent = command.parent
 
         # Retrieve the help command for the invoked command.
         if parent and command:
-            help_command = (self.bot.get_command("help"), parent.name, command.name)
+            return self.bot.get_command("help"), parent.name, command.name
         elif command:
-            help_command = (self.bot.get_command("help"), command.name)
+            return self.bot.get_command("help"), command.name
         else:
-            help_command = (self.bot.get_command("help"),)
+            return self.bot.get_command("help")
 
-        if hasattr(e, "handled"):
-            log.trace(f"Command {command} had its error already handled locally; ignoring.")
+    async def try_get_tag(self, ctx: Context) -> None:
+        """
+        Attempt to display a tag by interpreting the command name as a tag name.
+
+        The invocation of tags get respects its checks. Any CommandErrors raised will be handled
+        by `on_command_error`, but the `invoked_from_error_handler` attribute will be added to
+        the context to prevent infinite recursion in the case of a CommandNotFound exception.
+        """
+        tags_get_command = self.bot.get_command("tags get")
+        ctx.invoked_from_error_handler = True
+
+        log_msg = "Cancelling attempt to fall back to a tag due to failed checks."
+        try:
+            if not await tags_get_command.can_run(ctx):
+                log.debug(log_msg)
+                return
+        except errors.CommandError as tag_error:
+            log.debug(log_msg)
+            await self.on_command_error(ctx, tag_error)
             return
 
-        # Try to look for a tag with the command's name if the command isn't found.
-        if isinstance(e, CommandNotFound) and not hasattr(ctx, "invoked_from_error_handler"):
-            if not ctx.channel.id == Channels.verification:
-                tags_get_command = self.bot.get_command("tags get")
-                ctx.invoked_from_error_handler = True
-
-                log_msg = "Cancelling attempt to fall back to a tag due to failed checks."
-                try:
-                    if not await tags_get_command.can_run(ctx):
-                        log.debug(log_msg)
-                        return
-                except CommandError as tag_error:
-                    log.debug(log_msg)
-                    await self.on_command_error(ctx, tag_error)
-                    return
-
-                # Return to not raise the exception
-                with contextlib.suppress(ResponseCodeError):
-                    await ctx.invoke(tags_get_command, tag_name=ctx.invoked_with)
-                    return
-        elif isinstance(e, BadArgument):
+        # Return to not raise the exception
+        with contextlib.suppress(ResponseCodeError):
+            await ctx.invoke(tags_get_command, tag_name=ctx.invoked_with)
+            return
+
+    async def handle_user_input_error(self, ctx: Context, e: errors.UserInputError) -> None:
+        """
+        Send an error message in `ctx` for UserInputError, sometimes invoking the help command too.
+
+        * MissingRequiredArgument: send an error message with arg name and the help command
+        * TooManyArguments: send an error message and the help command
+        * BadArgument: send an error message and the help command
+        * BadUnionArgument: send an error message including the error produced by the last converter
+        * ArgumentParsingError: send an error message
+        * Other: send an error message and the help command
+        """
+        # TODO: use ctx.send_help() once PR #519 is merged.
+        help_command = await self.get_help_command(ctx.command)
+
+        if isinstance(e, errors.MissingRequiredArgument):
+            await ctx.send(f"Missing required argument `{e.param.name}`.")
+            await ctx.invoke(*help_command)
+        elif isinstance(e, errors.TooManyArguments):
+            await ctx.send(f"Too many arguments provided.")
+            await ctx.invoke(*help_command)
+        elif isinstance(e, errors.BadArgument):
             await ctx.send(f"Bad argument: {e}\n")
             await ctx.invoke(*help_command)
-        elif isinstance(e, UserInputError):
+        elif isinstance(e, errors.BadUnionArgument):
+            await ctx.send(f"Bad argument: {e}\n```{e.errors[-1]}```")
+        elif isinstance(e, errors.ArgumentParsingError):
+            await ctx.send(f"Argument parsing error: {e}")
+        else:
             await ctx.send("Something about your input seems off. Check the arguments:")
             await ctx.invoke(*help_command)
-            log.debug(
-                f"Command {command} invoked by {ctx.message.author} with error "
-                f"{e.__class__.__name__}: {e}"
-            )
-        elif isinstance(e, NoPrivateMessage):
-            await ctx.send("Sorry, this command can't be used in a private message!")
-        elif isinstance(e, BotMissingPermissions):
-            await ctx.send(f"Sorry, it looks like I don't have the permissions I need to do that.")
-            log.warning(
-                f"The bot is missing permissions to execute command {command}: {e.missing_perms}"
-            )
-        elif isinstance(e, MissingPermissions):
-            log.debug(
-                f"{ctx.message.author} is missing permissions to invoke command {command}: "
-                f"{e.missing_perms}"
+
+    @staticmethod
+    async def handle_check_failure(ctx: Context, e: errors.CheckFailure) -> None:
+        """
+        Send an error message in `ctx` for certain types of CheckFailure.
+
+        The following types are handled:
+
+        * BotMissingPermissions
+        * BotMissingRole
+        * BotMissingAnyRole
+        * NoPrivateMessage
+        * InChannelCheckFailure
+        """
+        bot_missing_errors = (
+            errors.BotMissingPermissions,
+            errors.BotMissingRole,
+            errors.BotMissingAnyRole
+        )
+
+        if isinstance(e, bot_missing_errors):
+            await ctx.send(
+                f"Sorry, it looks like I don't have the permissions or roles I need to do that."
             )
-        elif isinstance(e, InChannelCheckFailure):
+        elif isinstance(e, (InChannelCheckFailure, errors.NoPrivateMessage)):
             await ctx.send(e)
-        elif isinstance(e, (CheckFailure, CommandOnCooldown, DisabledCommand)):
-            log.debug(
-                f"Command {command} invoked by {ctx.message.author} with error "
-                f"{e.__class__.__name__}: {e}"
-            )
-        elif isinstance(e, CommandInvokeError):
-            if isinstance(e.original, ResponseCodeError):
-                status = e.original.response.status
-
-                if status == 404:
-                    await ctx.send("There does not seem to be anything matching your query.")
-                elif status == 400:
-                    content = await e.original.response.json()
-                    log.debug(f"API responded with 400 for command {command}: %r.", content)
-                    await ctx.send("According to the API, your request is malformed.")
-                elif 500 <= status < 600:
-                    await ctx.send("Sorry, there seems to be an internal issue with the API.")
-                    log.warning(f"API responded with {status} for command {command}")
-                else:
-                    await ctx.send(f"Got an unexpected status code from the API (`{status}`).")
-                    log.warning(f"Unexpected API response for command {command}: {status}")
-            else:
-                await self.handle_unexpected_error(ctx, e.original)
+
+    @staticmethod
+    async def handle_api_error(ctx: Context, e: ResponseCodeError) -> None:
+        """Send an error message in `ctx` for ResponseCodeError and log it."""
+        if e.status == 404:
+            await ctx.send("There does not seem to be anything matching your query.")
+            log.debug(f"API responded with 404 for command {ctx.command}")
+        elif e.status == 400:
+            content = await e.response.json()
+            log.debug(f"API responded with 400 for command {ctx.command}: %r.", content)
+            await ctx.send("According to the API, your request is malformed.")
+        elif 500 <= e.status < 600:
+            await ctx.send("Sorry, there seems to be an internal issue with the API.")
+            log.warning(f"API responded with {e.status} for command {ctx.command}")
         else:
-            await self.handle_unexpected_error(ctx, e)
+            await ctx.send(f"Got an unexpected status code from the API (`{e.status}`).")
+            log.warning(f"Unexpected API response for command {ctx.command}: {e.status}")
 
     @staticmethod
-    async def handle_unexpected_error(ctx: Context, e: CommandError) -> None:
-        """Generic handler for errors without an explicit handler."""
+    async def handle_unexpected_error(ctx: Context, e: errors.CommandError) -> None:
+        """Send a generic error message in `ctx` and log the exception as an error with exc_info."""
         await ctx.send(
             f"Sorry, an unexpected error occurred. Please let us know!\n\n"
             f"```{e.__class__.__name__}: {e}```"
diff --git a/bot/cogs/moderation/infractions.py b/bot/cogs/moderation/infractions.py
index f4e296df9..9ea17b2b3 100644
--- a/bot/cogs/moderation/infractions.py
+++ b/bot/cogs/moderation/infractions.py
@@ -313,6 +313,6 @@ class Infractions(InfractionScheduler, commands.Cog):
     async def cog_command_error(self, ctx: Context, error: Exception) -> None:
         """Send a notification to the invoking context on a Union failure."""
         if isinstance(error, commands.BadUnionArgument):
-            if discord.User in error.converters:
+            if discord.User in error.converters or discord.Member in error.converters:
                 await ctx.send(str(error.errors[0]))
                 error.handled = True