mobilityhouse / ocpp

Python implementation of the Open Charge Point Protocol (OCPP).
MIT License
788 stars 310 forks source link

A routing method decorated with another decorator on top of @on or @after, will not be included in the routables #156

Open tropxy opened 3 years ago

tropxy commented 3 years ago

OCPP Version: 0.8.1 Python Version: 3.X

If we have an handler for a BootNotification like this:

@on(Action.BootNotification)
def on_boot_notitication(self, charge_point_vendor: str, charge_point_model: str, **kwargs):
    return call_result.BootNotificationPayload(
        current_time=datetime.utcnow().isoformat(),
        interval=10,
        status=RegistrationStatus.accepted
    )

Things work as expected. But if we have a simple decorator like this one:

def say_hello(f):
    def inner(*args, **kwargs):
        print("hello")
        return f(*args, **kwargs)
    return inner

and we apply it to the BootNotification handler:

@on(Action.BootNotification)
@say_hello
def on_boot_notitication(self, charge_point_vendor: str, charge_point_model: str, **kwargs):
    return call_result.BootNotificationPayload(
        current_time=datetime.utcnow().isoformat(),
        interval=10,
        status=RegistrationStatus.accepted
    )

The handler is not registered in the routables and we get the error:

INFO:ocpp:thesimulatedone: receive message [2, "39a937eb-aa77-414e-9d50-870bca5d08d3", "BootNotification", {"chargePointModel": "Generic", "chargePointVendor": "Virtual Charge Point"}]
ERROR:websockets.server:Error in connection handler
Traceback (most recent call last):
  File "/Users/andre/.virtualenvs/tmh-ocpp/lib/python3.8/site-packages/ocpp/charge_point.py", line 162, in _handle_call
    handlers = self.route_map[msg.action]
KeyError: 'BootNotification'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/Users/andre/.virtualenvs/tmh-ocpp/lib/python3.8/site-packages/websockets/server.py", line 191, in handler
    await self.ws_handler(self, path)
  File "central_system.py", line 78, in on_connect
    await cp.start()
  File "/Users/andre/.virtualenvs/tmh-ocpp/lib/python3.8/site-packages/ocpp/charge_point.py", line 126, in start
    await self.route_message(message)
  File "/Users/andre/.virtualenvs/tmh-ocpp/lib/python3.8/site-packages/ocpp/charge_point.py", line 144, in route_message
    await self._handle_call(msg)
  File "/Users/andre/.virtualenvs/tmh-ocpp/lib/python3.8/site-packages/ocpp/charge_point.py", line 164, in _handle_call
    raise NotImplementedError(f"No handler for '{msg.action}' "
ocpp.exceptions.NotImplementedError: NotImplementedError: No handler for 'BootNotification' registered., {}

This happens because the code in here: https://github.com/mobilityhouse/ocpp/blob/48092b453a5829ff96bba11eb18a132babe0b390/ocpp/routing.py#L37

uses the function name to establish a route, but the say_hello decorator changes the function name to 'inner', in this case. A quick fix on the client side is to use the functools and decorate the inner method with it, like:

import functools

def say_hello(f):
    @functools.wraps(f)
    def inner(*args, **kwargs):
        print("hello")
        return f(*args, **kwargs)
    return inner

But ideally, the lib should be shielded against other decorators actions

OrangeTux commented 3 years ago

I'm unsure how to fix this on this library. Or even issue a warning to the user.

Consider the following code:

def other(f):
    def inner(*args, **kwargs):
        return f(args, kwargs)

    return inner

class ChargePoint:
    @on(Action.Heartbeat)
    @other
    def on_heartbeat(self):
        pass

Note that decorators are executed from bottom to top. Also note that decorators are syntactic sugar. The above could be rewritten as: on(other)(on_heartbeat)

As you can see on() receives other(). And in it's current implementation on() will register other in a global list of routables. Ans it sets the attributes _on_action and _skip_schema_validation on this method instead of setting them on on_heartbeat.

create_route_map() iterates over that global lists of routables. It verifies if an instance of ChargePoint has an attribute that matches the routables. So in our case something like getattr(ChargePoint(), 'other') is done. That look up will fail.