Passing the response from @on to the @after method

[HugoJP1]

Hello,

I have the following scenario:

    @on(Action.RequestStartTransaction)
    async def on_request_start_transaction(
        self,
        *args,
        **kwargs
    ) -> call_result.RequestStartTransactionPayload:

        response: call_result.RequestStartTransactionPayload = (
            await TransactionHandler.handle_on_request_start_transaction(**kwargs)
        )

        # at this point, the response can be accepted
        # or it can be rejected
        return response


    @after(Action.RequestStartTransaction)
    async def after_request_start_transaction(
        self,
        *args,
        **kwargs
    ) -> call_result.TransactionEventPayload:

        # at this point, I would like to run this method conditionally,
        # only if the response sent back to the CSMS was accepted
        # if it was rejected, I do not want to run this method
        return await TransactionHandler.handle_after_request_start_transaction(**kwargs)

Looking at charge_point.py, it looks like it is the payload from the initial call which is passed into the @after function. What would you think of also passing through the response of the call so that it can be used for further processing?

I have a workaround by using a temp variable, but thought it could be a nice feature of the library so posting here.

[tropxy]

I think this is a good suggestion and the enhancement should not be too hard as we only need to pass the response to the handler here:

ocpp/ocpp/charge_point.py

Line 221 in efc8f08

response = handler(**snake_case_payload)

To not break backwards compatibility, a flag could be added on the after decorator to enable/disable this feature, same way it was done with the validation for the on decorator where the flag is by default False

[OrangeTux]

The routing system of this library is not very sophisticated and could use some improvement.

In the current implementation, the @on() handler is executed and it's return value is send as response to the other peer. Next, the @after() handler is executed. The return value of the @after() handler is ignored.

I'm afraid that above process might not be clear to all users of this library. In fact, the code sample that @HugoJP1 seem to indicate that this is not clear for them, as the @after() contains a return statement. Although this return value doesn't do anything. Don't get me wrong, that is a fault of the API, not of the users.

Assume for now, return value of the @on() handler is passed down to the @after() handler. Modifying the response and trying return it won't send the modified response to the other peer. Here a code sample that shows invalid use of the proposed API:

class ChargePoint(cp)
    @on(Action.BootNotification)
    def on_boot_notification(self, charge_point_vendor: str, charge_point_model: str, **kwargs):
        return call_result.BootNotificationPayload(
            current_time=datetime.utcnow().isoformat(),
            interval=10,
            status=RegistrationStatus.accepted
        )
     
    @after(Action.BootNotification)
    def after_boot_notification(self, charge_point_vendor: str, charge_point_model: str, _response: call_result.BootNotificationPayload, **kwargs):
        # Invalid use of the API. This return value won't be send to the charge point.
        _response.interval = 20
       return _response

In retrospect, the current api of @on() and @after() is too limiting. And we could use a better one. Maybe this ticket can serve as a discussion.

One quick proposal, that I didn't thought through yet is to allow preprocessing of requests,and post-processing of response. E.g.

class ChargePoint
    @on(Action.BootNotification, pre=pre_boot_notification, post=post_boot_notification)
    def on_boot_notification(self, charge_point_vendor: str, charge_point_model: str, **kwargs):
        return call_result.BootNotificationPayload(
            current_time=datetime.utcnow().isoformat(),
            interval=10,
            status=RegistrationStatus.accepted
        )
        
     def pre_boot_notification(self, request: call.BootNotificationPayload) -> call.BootNotificationPayload:
         # Modify request here before it's passed to `@on()` handler.
         return request
    
    def post_boot_notification(self, request: call.BootNotificationPayload, response: call_result.BootNotificationPayload) -> call_result.BootNotificationPayload:
        # Modify response before it's send to peer.
        return response

It might even be made backwards compatible.

[tropxy]

Assume for now, return value of the @on() handler is passed down to the @after() handler. Modifying the response and trying return it won't send the modified response to the other peer. Here a code sample that shows invalid use of the proposed API

The point is not to modify the response and send it to the peer in the after decorator... The method decorated by the after decorator, afik, was always treated as a post processing method, where you can perform a few actions that only make sense after you have processed the message associated with the on decorator one and the intention was not to change the response that needs to be sent to the peer. I remind that the motivation for the after decorator, was because we saw the need to do post-processing and we didnt want to delay the sending of the response to the peer.

This said, I think we can keep unchanged the goal of the @after decorator and just improve it a bit, by allowing to process also the response sent during process of the @on one, avoiding the user to save some context in a temp variable.

[OrangeTux]

The point is not to modify the response and send it to the peer in the after decorator...

Correct. My only concern is: is that clear for users? IMHO we should try to design an API that hard to use wrong.

But assuming it's clear for them. How should we modify the current API?

Something like:

@after("BootNotification", inject_response=True):
async def after_boot_notification_handler(self, response, *args, **kwargs):
   pass

A couple of notes and questions:

• inject_response is a keyword only argument which defaults to False to be backwards compatible.
• In this example, the response is passed as argument response. Is that a good name or should we pick another?
• What is the type of response? A dict or a call_result.BootNotificationPayload?

[tropxy]

I think with a proper doc/example, we can make it clear as I agree that currently may not be that clear...

Something like:

@after("BootNotification", inject_response=True):
async def after_boot_notification_handler(self, response, *args, **kwargs):
pass
A couple of notes and questions:

inject_response is a keyword only argument which defaults to False to be backwards compatible.
In this example, the response is passed as argument response. Is that a good name or should we pick another?
What is the type of response? A dict or a call_result.BootNotificationPayload?

Yup, that was how I was imagining. I think your name suggestions are fine. As for the type of the data I think call_result.BootNotificationPayloadmakes sense

[betovaca]

@tropxy Did you manage to do it? If so, can you share an example with us? Thanks

[astrand]

I agree that the existing API is very limited. It is common that you do not want to execute the "after" method, for example, if the request handler returned/responded with "Rejected". I don't see any easy way of doing that, currently?

I'm not sure we talke about the same "response", though, but as I see it, whatever the "on" handler returned, should be available in the "after" method.

[astrand]

Untested idea:

--- a/ocpp/charge_point.py
+++ b/ocpp/charge_point.py
@@ -256,23 +256,23 @@ class ChargePoint:
         # * firmware_version becomes firmwareVersion
         camel_case_payload = snake_to_camel_case(response_payload)
 
-        response = msg.create_call_result(camel_case_payload)
+        call_response = msg.create_call_result(camel_case_payload)
 
         if not handlers.get("_skip_schema_validation", False):
-            validate_payload(response, self._ocpp_version)
+            validate_payload(call_response, self._ocpp_version)
 
-        await self._send(response.to_json())
+        await self._send(call_response.to_json())
 
         try:
             handler = handlers["_after_action"]
             handler_signature = inspect.signature(handler)
-            call_unique_id_required = "call_unique_id" in handler_signature.parameters
-            # call_unique_id should be passed as kwarg only if is defined explicitly
-            # in the handler signature
-            if call_unique_id_required:
-                response = handler(**snake_case_payload, call_unique_id=msg.unique_id)
-            else:
-                response = handler(**snake_case_payload)
+            # call_unique_id and handler_response should be passed as kwarg only if is
+            # defined explicitly in the handler signature
+            if "call_unique_id" in handler_signature.parameters:
+                snake_case_payload["call_unique_id"] = msg.unique_id
+            if "handler_response" in handler_signature.parameters:
+                snake_case_payload["handler_response"] = response
+            response = handler(**snake_case_payload)
             # Create task to avoid blocking when making a call inside the
             # after handler
             if inspect.isawaitable(response):

[OSkrk]

I've created a PR which implements what has been discussed here.

[github-actions]

This issue is stale because it has been open for 30 days with no activity.