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.

Sign up for GitHub

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

Feat/allow passing answer from @on to @after hook #683

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Feat/allow passing answer from @on to @after hook #683

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
12ed77c
fix typo CostUpdated enum for 201
OSkrk Mar 18, 2024
6c7e4b2
Merge branch 'mobilityhouse:master' into master
OSkrk Nov 26, 2024
1447f88
feat/allow_passing_answer_from_@on_to_@after_hook
OSkrk Nov 26, 2024
7bbace2
Update tests/test_charge_point.py
OSkrk Nov 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 17 additions & 2 deletions ocpp/charge_point.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -356,10 +356,25 @@ async def _handle_call(self, msg):
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
inject_response = getattr(handler, "_inject_response", False)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why check attribute on the handler, instead of checking signature parameters? This opens up for some corner cases, like if inject_response is True but after handler does not accept "on_response" kwarg. Also it is different from how call_unique_id is handled.

Copy link
Collaborator Author

@OSkrk OSkrk Dec 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@astrand , thanks for your comment. Why would you set the flag to True (which defaults to False) to pass the reponse and do not accept it in the @after ?

if call_unique_id_required:
response = handler(**snake_case_payload, call_unique_id=msg.unique_id)
if inject_response:
response = handler(
**snake_case_payload,
call_unique_id=msg.unique_id,
on_response=response_payload,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be better the term "call_response". The "on" prefix is given to functions/callbacks as an action will be performed via that callback function. Here we want to pass an argument..

)
else:
response = handler(
**snake_case_payload, call_unique_id=msg.unique_id
)
else:
response = handler(**snake_case_payload)
if inject_response:
response = handler(
**snake_case_payload, on_response=response_payload
)
else:
response = handler(**snake_case_payload)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not so "elegant" to have 4 calls to handler(). My suggestion is to use keyword arguments instead, like I did in #264 (comment) .

Copy link
Collaborator Author

@OSkrk OSkrk Dec 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This approach was suggested by @tropxy in this comment and clarified by @OrangeTux in a later comment (both authors of this repo). There is no 4 calls to the handler, the handler is only called once depending on the parameters.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@OSkrk what @astrand is suggesting is something like this (right @astrand ?):

if "call_unique_id" in handler_signature.parameters:
    snake_case_payload["call_unique_id"] = msg.unique_id
if  inject_response:
    snake_case_payload["on_response"] = response_payload

response = handler(**snake_case_payload)

May you try that and see if it works?

Note: I have used your current nomenclature, but if you change the code then "on_response" will be "call_response"

# Create task to avoid blocking when making a call inside the
# after handler
if inspect.isawaitable(response):
Expand Down
3 changes: 2 additions & 1 deletion ocpp/routing.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ def inner(*args, **kwargs):
return decorator


def after(action):
def after(action, inject_response=False):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you include notes in the doc strings in respect to the new argument and its purpose?

"""Function decorator to mark function as hook to post-request hook.

This hook's arguments are the data that is in the payload for the specific
Expand All @@ -76,6 +76,7 @@ def inner(*args, **kwargs):
return func(*args, **kwargs)

inner._after_action = action
inner._inject_response = inject_response
if func.__name__ not in routables:
routables.append(func.__name__)
return inner
Expand Down
29 changes: 29 additions & 0 deletions tests/test_charge_point.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -472,3 +472,32 @@ def after_boot_notification(self, *args, **kwargs):
assert ChargerA.after_boot_notification_call_count == 1
assert ChargerB.on_boot_notification_call_count == 1
assert ChargerB.after_boot_notification_call_count == 1


@pytest.mark.asyncio
async def test_response_injected_to_after_handler(connection):
"""
This test ensures that the response is injected to the `after` handler
when `inject_response` is set to True.
"""

class TestChargePoint(cp_16):
@on(Action.BootNotification)
def on_boot_notification(self, **kwargs):
return BootNotificationResult(
current_time="2024-11-01T00:00:00Z",
interval=300,
status=RegistrationStatus.accepted,
)

@after(Action.BootNotification, inject_response=True)
def after_boot_notification(self, on_response, **kwargs):

assert on_response["current_time"] == "2024-11-01T00:00:00Z"
assert on_response["interval"] == 300
assert on_response["status"] == RegistrationStatus.accepted

charge_point = TestChargePoint("test_cp", connection)
payload = {"chargePointVendor": "vendor", "chargePointModel": "model"}
msg = Call(unique_id="1234", action=Action.BootNotification.value, payload=payload)
await charge_point._handle_call(msg)