Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fetch channel when not found upon converting message #9213

Open
shtlrs opened this issue Jan 29, 2023 · 3 comments
Open

Fetch channel when not found upon converting message #9213

shtlrs opened this issue Jan 29, 2023 · 3 comments
Labels
feature request This is a feature request.

Comments

@shtlrs
Copy link

shtlrs commented Jan 29, 2023

Summary

Fetch channel upon message converion

What is the feature request for?

discord.ext.commands

The Problem

Currently, here's how the MessageConverter works

If the message trying to be converted is found, return directly
Otherwise, try to resolve the channel from the bot's cache.
If the channel isn't found, throw a ChannelNotFound exception

 async def convert(self, ctx: Context[BotT], argument: str) -> discord.Message:
        guild_id, message_id, channel_id = PartialMessageConverter._get_id_matches(ctx, argument)
        message = ctx.bot._connection._get_message(message_id)
        if message:
            return message
        channel = PartialMessageConverter._resolve_channel(ctx, guild_id, channel_id)
        if not channel or not isinstance(channel, discord.abc.Messageable):
            raise ChannelNotFound(channel_id)
        try:
            return await channel.fetch_message(message_id)
        except discord.NotFound:
            raise MessageNotFound(argument)
        except discord.Forbidden:
            raise ChannelNotReadable(channel)  # type: ignore # type-checker thinks channel could be a DMChannel at this point

The problem here is that it tries to fetch the message if not found, but not the channel, which results in never being able to convert the message.

The Ideal Solution

Ideal solution would be to try to fetch the channel if it's not in cache, then (maybe) cache it again for future usages.
Which would result in something like

 async def convert(self, ctx: Context[BotT], argument: str) -> discord.Message:
        guild_id, message_id, channel_id = PartialMessageConverter._get_id_matches(ctx, argument)
        message = ctx.bot._connection._get_message(message_id)
        if message:
            return message
        channel = PartialMessageConverter._resolve_channel(ctx, guild_id, channel_id) or bot.fetch_channel(channel_id)
        if not channel or not isinstance(channel, discord.abc.Messageable):
            raise ChannelNotFound(channel_id)
        try:
            return await channel.fetch_message(message_id)
        except discord.NotFound:
            raise MessageNotFound(argument)
        except discord.Forbidden:
            raise ChannelNotReadable(channel)  # type: ignore # type-checker thinks channel could be a DMChannel at this point

The Current Solution

No response

Additional Context

No response

@shtlrs shtlrs added the feature request This is a feature request. label Jan 29, 2023
@Rapptz
Copy link
Owner

Rapptz commented Jan 29, 2023

It's not this way on purpose so you don't get bombarded with (another) bogus 400 requests when someone crafts either a garbage URL with an obviously wrong ID or a message ID you can't read to begin with. This change would cause the invalid requests on the worst case to go from one 400 to two.

There is a PartialMessageable type now that could be used but it'd need some work in order to make ChannelNotFound viable.

@shtlrs
Copy link
Author

shtlrs commented Jan 29, 2023

It's not this way on purpose so you don't get bombarded with (another) bogus 400 requests when someone crafts either a garbage URL with an obviously wrong ID or a message ID you can't read to begin with. This change would cause the invalid requests on the worst case to go from one 400 to two.

There is a PartialMessageable type now that could be used but it'd need some work in order to make ChannelNotFound viable.

Hum, I see.

But leveraging a PartialMessage wouldn't really solve the issue in the MessageConverter, or am I missing something ?

I guess this would need to be done client-side once, then that can be cached along with the rest to avoid recurrent requests.

@Rapptz
Copy link
Owner

Rapptz commented Jan 29, 2023

Not PartialMessage, PartialMessageable. The latter is a partial channel type where messages can be sent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request This is a feature request.
Projects
None yet
Development

No branches or pull requests

2 participants