aiogram / aiogram

aiogram is a modern and fully asynchronous framework for Telegram Bot API written in Python using asyncio
https://aiogram.dev
MIT License
4.76k stars 834 forks source link

KeyboardBuilder: if self.max_width = 0, endless while loop #1595

Open skinkie opened 1 month ago

skinkie commented 1 month ago

Checklist

Operating system

Linux

Python version

3.11, 3.12

aiogram version

3.13.1

Expected behavior

Keyboard builder is created.

Current behavior

        # Separate buttons to exclusive rows with max possible row width
        while buttons:
            row, buttons = buttons[: self.max_width], buttons[self.max_width :]
            markup.append(list(row))

This code results in an endless loop if self.max_width = 0, since the number of buttons never decreases.

Steps to reproduce

        builder = KeyboardBuilder(button_type=KeyboardButton)
        for device_id in devices:
            button = KeyboardButton(text=device_id)
            builder.add(button)

When max_width is explicitly set, code runs without an issue.

        builder = KeyboardBuilder(button_type=KeyboardButton)
        builder.max_width = 5
        for device_id in devices:
            button = KeyboardButton(text=device_id)
            builder.add(button)
JrooTJunior commented 1 month ago

This is actually not an bug, this attribute should never be changed externally. This attribute is at the class level and is only needed to set constraints (based on API constraints) when creating the markup. Configured in child classes.

If you want to set a size other than "maximum per row", you should use the .adjust method, which does not allow values ​​outside the range of max_width and min_width.

Another point: you should not use KeyboardBuilder directly, you should use InlineKeyboardBuilder and ReplyKeyboardBuilder, which are preconfigured to be used with a certain type of markup.


In summary, you shouldn't expect code to work correctly if you use it in a completely wrong way, ignoring the documentation.

I will think about how to protect these attributes from over-inquisitive developers who poke their noses where they shouldn't be.

skinkie commented 1 month ago

@JrooTJunior I was not using max_width. The code broke without it. max_width is initialised with 0. My guess is this issue was introduced when upgrading aiogram. Thanks for the suggestion to use InlineKeyboardBuilder and ReplyKeyboardBuilder.

In my opinion the while buttons should assert that the self.max_width > 0, not doing so leads to a process that eats all the users memory.

And if you review the current documentation, KeyboardBuilder is used directly.

image

...over-inquisitive developers knows how to use git blame:

https://github.com/aiogram/aiogram/commit/844d6f58f5af3ce103d1b4b420e994c1635a3dd5

From MAX_WIDTH = 8 we went into max_width: int = 0.

JrooTJunior commented 1 month ago

Oh, now that makes sense. This method is shown in the documentation from a parent class that doesn't know about more specific child classes, so it can't be referenced with the correct class name. So we should figure out how to adjust this example so that it doesn't confuse developers.


...over-inquisitive developers knows how to use git blame:

844d6f5

From MAX_WIDTH = 8 we went into max_width: int = 0.

Yes, it has been moved from module-level constants to inside the class definition, since these values ​​are specific to each markup type.


Thank you very much. I'll think about how to change the documentation and make the limit configuration safer to use.

skinkie commented 1 month ago

@JrooTJunior thank you for your prompt response. I am a very happy user and we use this project for good use.

JrooTJunior commented 1 month ago

Thank you very much. I'll think about how to change the documentation and make the limit configuration safer to use.

... or you can help me just creating pull request that fix this issue 😏.

We can also discuss changes based on an already created pull request to adjust the vision of the solution.

I think it can speed up the process of change, because now there are difficulties with free time.

Feel free to contribute to the open-source project.