django / channels

Developer-friendly asynchrony for Django
https://channels.readthedocs.io
BSD 3-Clause "New" or "Revised" License
6.01k stars 793 forks source link

Handler declaration by decorator/annotation (in order to prevent potential method leaks) #2075

Closed timwoocker closed 4 months ago

timwoocker commented 4 months ago

Current functionality

Right now, handlers for specific messages are determined by processing the type in get_handler_name and passing the result to getattr on the Consumer-class.

Problems with this design

While it is very unlikely that this might cause any security vulnerabilities by "leaking" different methods than those intended to be a handler, I think it is still very possible for an unexperienced developer to do a mistake like passing the type from a client-sent message to channels. While it is the developer's responsibility to prevent such mistakes, I believe this is something that should be changed in a future version similar to how base django does everything in its power to prevent "beginner" mistakes (sql injection, form validation/escaping etc).

Another reason to get rid of the current style is that it is inconvenient for a developer to be "forced" to call their methods based on the event type they are handling. This introduces 2 problems: 1) Worse method names for handlers 2) "helper" methods should always be named in a way that the developer can be sure the same method name won't be used as an event handler at a later point during development (example: I have a helper method in my Consumer called process_data and at some point in the future introduce an event called "process.data". Now I'd either need to choose an unfitting type or refactor all my code to rename the original helper method).

I even have another reason why this design is bad: You can only have a single handler per type. Especially when working with a modular architecture, it might be required to perform different tasks for example in a base class and a child class. While you can currently theoretically implement this by overriding the handler and calling super() in the child, this is "ugly" code as the two handlers might not be related to each other.

Solution

In order to fix this, I propose a new way of declaring a handler. Most modern modules use decorators instead in a style like this:

@on('event.type')
async def handle_whatever(self, ...):
    ...

If this is introduced, the old way should be deprecated and disabled by default however there should be an option you can set in your settings.py to re-enable the old method for compatibility reasons until it is completely removed from channels.

Additional thoughts

I'm creating an issue instead of working on this feature and creating a PR because my thoughts might be invalid. If you believe that I have made valid points, I'd be happy to write the code needed for this feature and create a PR but first I want some feedback on the idea.

In my current projects, I have implemented a similar feature which automatically "generates" message types so I don't need to throw around strings and method names. It would be another alternative way of "declaring" events so I will just drop some example code here on how this concept is currently used in my production projects:

Declaration of an event type including serializable attributes:

class ExampleEvent(ChannelsEvent):
    id: int = None
    price: Decimal = None
    qty: Decimal = None
    is_buy: bool = None

Declaring a handler for an event:

    @ExampleEvent.handler
    async def yeah_cool_example(self, event: RequestSnapshot):
        ...

Triggering the event:

# this will send the event to all members of the group "a_group"
await OrderBookUpdatedEvent(
                        id=0,
                        price=Decimal(10),
                        qty=Decimal(10),
                        is_buy=False
                    ).trigger(self.channel_layer.group_send, "a_group")

Please also leave your thoughts on that approach which differs from my proposed approach in the way that the event type for the developer no longer is a string but a class. This makes it easier to refactor your code if you wish to rename an event. My code generates a hash based on the event class (name + attributes) which is used in the type to make sure different servers can communicate with the same event classes -> same class, same "type".

As a short explanation here is how I currently achieve this functionality with the handler-decorator. This replaced the original method with a new one which iterates all handlers registered for the specified event, gets a generated unique name for the event and replaces the original method's name with the generated name for the handler function:

    class handler:
        def __init__(self, cls: "ChannelsEvent", func):
            self.cls = cls
            self.func = func

        def __set_name__(self, owner: Type[AsyncConsumer], name):
            if owner not in self.cls._handlers:
                self.cls._handlers[owner] = []
            self.cls._handlers[owner].append(self.func)

            if len(self.cls._handlers[owner]) == 1:
                async def call_handlers(consumer, msg: Dict):
                    event = self.cls.from_dict(msg)
                    for handler in self.cls._handlers[owner]:
                        await handler(consumer, event)
                setattr(owner, self.cls._channels_event_type.replace(".", "_"), call_handlers)
                setattr(owner, name, self.func)
carltongibson commented 4 months ago

This would be an alternative implementation of Channel's generic consumers. The place to develop that would be in a separate package to demonstrate viability. A shift would require a major version change in Channels, so it's not something we'd do lightly.

I'm going to close for now, but do follow up when you have a proof of concept ready!