Centralize error handling
Closed this issue · 8 comments
Problem
Currently, all of the error handling is usually done through an ErrorHandler Cog.
This cog will only handle errors raised from Text Commands and will not catch errors raised from App Commands (Slash Commands and Context Menu Commands)
Solution
The way we can centralize handling for App Commands would ideally be by subclassing the CommandTree class and override the on_error method, then upon instantiating the bot, will pass this class as an argument to the tree_cls param.
We obviously want to factor out the commun error handling code to avoid duplication, but not at the cost of code simplicity/readability. What this means is that a little bit of duplication is ok instead of having alot of python magic thrown at it.
I wanted to discuss the approach I had in mind before I start working on this.
Idea
- Basically, this would be just like middlewares in most web frameworks, implemented as a
chain of error handlers - Each concrete error handler would implement a specific interface that would allow it to know
- a. Whether it should handle the error in the first place
- b. How to handle the error
- These handlers would then be registered inside a container class that would loop over all the handlers.
Once the previous is done, the ErrorHandler cog and the CommandTree subclass would take an instance of this container, and let it do all the work.
This approach would allow a more flexible design of each error handler, and a clearer separation of responsibilities instead of all the if/elses we currently have chained together.
Error handler interface
The interface of an error handler look something like this
# pydis_core.utils.error_handling.abc
from abc import ABC, abstractmethod
from discord import Interaction, errors
from discord.ext.commands import Context
class AbstractCommandErrorHandler(ABC):
"""An abstract command error handler"""
@abstractmethod
def should_handle_error(self, error: errors.DiscordException) -> bool:
"""A predicate that determines whether the error should be handled or not."""
@abstractmethod
async def handle_app_command_error(self, interaction: Interaction, error: errors.DiscordException):
"""Handle error raised in the context of app commands."""
@abstractmethod
async def handle_text_command_error(self, context: Context, error: errors.DiscordException):
"""Handle error raised in the context of text commands."""Concrete error handler
This is just a reimplementation of the current check failure error handler we have in our bot repo.
# pydis_core.utils.error_handling.handlers.check_failure
from pydis_core.utils.error_handling.abc import AbstractCommandErrorHandler
from discord.ext.commands import errors
class CheckFailureErrorHandler(AbstractCommandErrorHandler):
def __init__(self, bot):
self.bot = bot
def should_handle_error(self, error: errors.DiscordException) -> bool:
if isinstance(error, errors.CheckFailure):
return True
return False
async def handle_app_command_error(self, interaction: Interaction, error: errors.DiscordException):
await self._handle_error(error=error, context=None, interaction=interaction)
async def handle_text_command_error(self, context: Context, error: errors.DiscordException):
await self._handle_error(error=error, context=context, interaction=None)
async def _handle_error(
self,
error: errors.DiscordException,
context: Context = None,
interaction: Interaction = None
):
bot_missing_errors = (
errors.BotMissingPermissions,
errors.BotMissingRole,
errors.BotMissingAnyRole
)
if isinstance(error, bot_missing_errors):
self.bot.stats.incr("errors.bot_permission_error")
if context:
await context.send("Sorry, it looks like I don't have the permissions or roles I need to do that.")
if interaction:
await interaction.response.send_message(
"Sorry, it looks like I don't have the permissions or roles I need to do that."
)
elif isinstance(error, errors.ContextCheckFailure | errors.NoPrivateMessage):
self.bot.stats.incr("errors.wrong_channel_or_dm_error")
if context:
await context.send(str(error))
if interaction:
await interaction.response.send_message(str(error))Error handlers' container
This class would register all error handlers, and allow to loop overthem to look for the handler that would fit.
We could register handlers under specific names if we ever want to have some "overriding" of error handlers in client apps.
# pydis_core.utils.error_handling.container
class ErrorHandlerContainer:
def __init__(self, handlers: list[AbstractCommandErrorHandler] = None):
self.handlers = handlers if handlers else []
async def handle_error(
self,
error: errors.DiscordException,
context_or_interaction: Context | Interaction
) -> None:
for handler in self.handlers:
if not handler.should_handle_error(error):
continue
if isinstance(context_or_interaction, Context):
await handler.handle_text_command_error(context_or_interaction, error)
if isinstance(context_or_interaction, Interaction):
await handler.handle_app_command_error(context_or_interaction, error)Usage inside a cog
Once all the handlers are implemented, the error handler cog would be simplified to the following.
class ErrorHandler(Cog):
def __init__(self, bot, error_handlers_container):
self.bot = bot
self.error_handlers_container = error_handlers_container
@Cog.listener()
async def on_command_error(self, ctx: Context, e: errors.CommandError) -> None:
await self.error_handlers_container.handle_error(e, ctx)Things that can be added on top of the previous
- It goes without saying that a default handler needs to be implemented.
- The order of handlers is important, so we can either rely on some sort of priority system, or just the order of registering them"
Fyi this is somewhat related to #85, which also mentions we should port over the error handling.
Hello @ChrisLovering, let's get this rolling. I don't have any feedback at the moment, but I hope to provide some in the near future.
This seems like quite a nice approach to dealing with this. One that may even be suitable for adding to D.py itself!
If you're interested in wokring on this yourself @shtlrs please do go ahead!
@ChrisLovering I started working on this, and one thing I'm wondering is how'd we want to register handlers ?
Do we want to do it a la discord.py ? Where we walk some modules/packages look for some method to register them ?
Or do we just register them by hand, one by one ?
If we go with option 1, I think the loading can be done synchronously since they're simple classes to setup with no equivalent of cog_load, and it's not like we'll be having millions of handlers.
I think we want it to be something that needs to be called manually, rather than walking them.
That way cogs can register error handlers, or in the setup_hook of bots we can register generic handlers.
That way cogs can register error handlers,
Do you mean like we have a cog that's responsible for registering these handlers ?
or in the setup_hook of bots we can register generic handlers.
The way I have it written now, is that the container class needs to have a default handler passed to it upon instantiation, to ensure we'll always fallback to it.
It's also there because the container exposes a register_handler method to register handlers, and I didn't want to do any non-simple manoeuvers to determine the order of iteration of the registered handlers.
the container's handler error basically does this
class Container:
def handle_error(...):
for handler in self.handlers]:
if handler.should_handle():
handler.handle()
break
else:
self.default.handle()That way we can solely rely on the should_handle method to determine the right conditions to handle an error without worrying about registration order.
I'm not sure I fully understand the proposal in this issue, but i'm not sure what the benefit of being able to have lots of error handler classes is.
Would it not be simpler to have only a base ErrorHandler cog in bot-core, and then inherit from that in individual projects to add project-specific behaviour?
I'm not sure I fully understand the proposal in this issue, but i'm not sure what the benefit of being able to have lots of error handler classes is.
The benefit is to be able to
- Deflate the error cog and extract each specific logic to its own class, making it easier to change/extend
- Being able to use the same instance of the error handling for both text command & app commands, as that's not currently possible.