mobilityhouse / ocpp

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

Error in v1.6 dataclasses breaks Authorize messaging #279

Closed chan-vince closed 2 years ago

chan-vince commented 2 years ago

When making an Authorize request from the Charge Point to the Central System, the request is an instance of call.AuthorizePayload:

@dataclass
class AuthorizePayload:
    id_tag: str

Note that the id_tag is of type str, and the OCPP-J v1.6 spec says it should be an IdToken type. Since this is basically the same as a string but constrained to a 20 character case-insensitive string, this is fine.

The response must be an instance of call_result.AuthorizePayload:

@dataclass
class AuthorizePayload:
    id_tag_info: Dict

Where there is a dataclass for id_tag_info (though not yet added to the above dataclass) defined in datatypes.py:

@dataclass
class IdToken:
    """
    Contains the identifier to use for authorization. It is a case
    insensitive string. In future releases this may become a complex type to
    support multiple forms of identifiers.
    """

    id_token: str

@dataclass
class IdTagInfo:
    """
    Contains status information about an identifier. It is returned in
    Authorize, Start Transaction and Stop Transaction responses.

    If expiryDate is not given, the status has no end date.
    """

    status: AuthorizationStatus
    parent_id_tag: Optional[IdToken] = None
    expiry_date: Optional[str] = None

The spec says that parent_id_tag is also an IdToken. However in this IdTagInfo dataclass it's an IdToken dataclass, with an id_token str attribute inside. Therefore, in a call_result.AuthorizePayload it gets serialised to json like this:

{
  "idTagInfo": {
    "status": "Accepted",
    "parentIdTag": {
      "idToken": "123"
    },
    "expiryDate": "2022-01-07T20:22:02.735181"
}

When this is received back at the Charge Point, it then fails the json schema validation because it's expecting the idToken to be a str as defined in the json schema. If schema validation is skpped, only then do you get a helpful exception:

Traceback (most recent call last):
  File "/Users/vince/projects/cloud-services/services/ocpp/ocpp-beam/tests/chargepoint/./charge_point.py", line 109, in <module>
    asyncio.run(main())
  File "/opt/homebrew/Cellar/python@3.9/3.9.9/Frameworks/Python.framework/Versions/3.9/lib/python3.9/asyncio/runners.py", line 44, in run
    return loop.run_until_complete(main)
  File "/opt/homebrew/Cellar/python@3.9/3.9.9/Frameworks/Python.framework/Versions/3.9/lib/python3.9/asyncio/base_events.py", line 642, in run_until_complete
    return future.result()
  File "/Users/vince/projects/cloud-services/services/ocpp/ocpp-beam/tests/chargepoint/./charge_point.py", line 102, in main
    await asyncio.gather(cp.start(), cp.send_authorize())
  File "/Users/vince/projects/cloud-services/services/ocpp/ocpp-beam/tests/chargepoint/./charge_point.py", line 45, in send_authorize
    response2 = await self.call(request2)
  File "/Users/vince/.cache/pypoetry/virtualenvs/ocpp-beam-ChE6HI8P-py3.9/lib/python3.9/site-packages/ocpp/charge_point.py", line 285, in call
    validate_payload(response, self._ocpp_version)
  File "/Users/vince/.cache/pypoetry/virtualenvs/ocpp-beam-ChE6HI8P-py3.9/lib/python3.9/site-packages/ocpp/messages.py", line 194, in validate_payload
    raise ValidationError(f"Payload '{message.payload} for action "
ocpp.exceptions.ValidationError: Payload '{'idTagInfo': {'status': 'Accepted', 'parentIdTag': {'idToken': '123'}, 'expiryDate': '2022-01-07T21:19:11.794097'}} for action 'Authorize' is not valid: {'idToken': '123'} is not of type 'string'

Failed validating 'type' in schema['properties']['idTagInfo']['properties']['parentIdTag']:
    {'maxLength': 20, 'type': 'string'}

On instance['idTagInfo']['parentIdTag']:
    {'idToken': '123'}

I'm not sure why this only gets raised when skip_schema_validation is enabled on the on decorator - unfotunately it seems to not be surfaced as otherwise the websocket disconnects instantly:

Traceback (most recent call last):
  File "/Users/vince/projects/cloud-services/services/ocpp/ocpp-beam/tests/chargepoint/./charge_point.py", line 109, in <module>
    asyncio.run(main())
  File "/opt/homebrew/Cellar/python@3.9/3.9.9/Frameworks/Python.framework/Versions/3.9/lib/python3.9/asyncio/runners.py", line 44, in run
    return loop.run_until_complete(main)
  File "/opt/homebrew/Cellar/python@3.9/3.9.9/Frameworks/Python.framework/Versions/3.9/lib/python3.9/asyncio/base_events.py", line 642, in run_until_complete
    return future.result()
  File "/Users/vince/projects/cloud-services/services/ocpp/ocpp-beam/tests/chargepoint/./charge_point.py", line 102, in main
    await asyncio.gather(cp.start(), cp.send_authorize())
  File "/Users/vince/.cache/pypoetry/virtualenvs/ocpp-beam-ChE6HI8P-py3.9/lib/python3.9/site-packages/ocpp/charge_point.py", line 125, in start
    message = await self._connection.recv()
  File "/Users/vince/.cache/pypoetry/virtualenvs/ocpp-beam-ChE6HI8P-py3.9/lib/python3.9/site-packages/websockets/legacy/protocol.py", line 552, in recv
    await self.ensure_open()
  File "/Users/vince/.cache/pypoetry/virtualenvs/ocpp-beam-ChE6HI8P-py3.9/lib/python3.9/site-packages/websockets/legacy/protocol.py", line 929, in ensure_open
    raise self.connection_closed_exc()
websockets.exceptions.ConnectionClosedOK: received 1000 (OK); then sent 1000 (OK)

I think the fix here is to remove the dataclass for IdToken and make the IdTagInfo be type annotated like so:

  parent_id_tag: Optional[str] = None

I tested this by adding this id_tag_info to a call_result.AuthorizePayload which works, despite it obviously going against the dataclass definition/type hinting.

@on(Action.Authorize)
def on_authorize(self, id_tag: str, **kwargs) -> call_result.AuthorizePayload:
    # Do some checking on the requested id
    id_tag_info = IdTagInfo(
        status=AuthorizationStatus.accepted,
        expiry_date=(datetime.now() + timedelta(days=30)).isoformat(),
        parent_id_tag=IdToken("123")
    )
    return call_result.AuthorizePayload(id_tag_info.__dict__) 
tropxy commented 2 years ago

Hi,

What this seems to request is a way to constrain the str data type, something that pydantic does: https://pydantic-docs.helpmanual.io/usage/types/#constrained-types

But, we have the schema that does the validation later on, so pydantic can be skipped.

Anyway, according to the spec and the schema IdToken is a str Type, so that is what we should use. Thanks for your analysis ;)

OrangeTux commented 2 years ago

Fi