mobilityhouse / ocpp

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

Revert changes made in pr #537 to change DataTransfer.data typehint. #570

Closed alexmclarty closed 7 months ago

alexmclarty commented 7 months ago

According to this issue, a PR was made to change the type of DataTransfer.data to Any rather than str.

This PR makes the type str instead of Any.

OrangeTux commented 7 months ago

@mdwcrft Does this PR have any impact on our internal codebase?

mdwcrft commented 7 months ago

As discussed offline I think this should still be of type Any - but we will clarify with the OCA what their definition of anyType means

proelke commented 7 months ago

The schema specifies a string here.

Jared-Newell-Mobility commented 7 months ago

I would suggest also that we add a comment to explain this complication in the standard in detail - I might prevent it reverted again into the future?

alexmclarty commented 7 months ago

@Jared-Newell-Mobility ready for review (once tests pass)

Jared-Newell-Mobility commented 7 months ago

I've check over things again, I don't see a type specified in the schema. There is clearly a discrepancy between the descriptions 1. Optional. Data without specified length or format, in response to request and 2. text, data without specified length or format.

Schema normally trumps; I'm going to raise it with the OCA at minimum this might be cleared up also for others in the next Errata


  "$schema": "http://json-schema.org/draft-06/schema#",
  "$id": "urn:OCPP:Cp:2:2020:3:DataTransferRequest",
  "comment": "OCPP 2.0.1 FINAL",
  "definitions": {
    "CustomDataType": {
      "description": "This class does not get 'AdditionalProperties = false' in the schema generation, so it can be extended with arbitrary JSON properties to allow adding custom data.",
      "javaType": "CustomData",
      "type": "object",
      "properties": {
        "vendorId": {
          "type": "string",
          "maxLength": 255
        }
      },
      "required": [
        "vendorId"
      ]
    }
  },
  "type": "object",
  "additionalProperties": false,
  "properties": {
    "customData": {
      "$ref": "#/definitions/CustomDataType"
    },
    "messageId": {
      "description": "May be used to indicate a specific message or implementation.\r\n",
      "type": "string",
      "maxLength": 50
    },
-----------------
    "data": {
      "description": "Data without specified length or format. This needs to be decided by both parties (Open to implementation).\r\n"
    },
-----------------
    "vendorId": {
      "description": "This identifies the Vendor specific implementation\r\n\r\n",
      "type": "string",
      "maxLength": 255
    }
  },
  "required": [
    "vendorId"
  ]
}```

```{
  "$schema": "http://json-schema.org/draft-06/schema#",
  "$id": "urn:OCPP:Cp:2:2020:3:DataTransferResponse",
  "comment": "OCPP 2.0.1 FINAL",
  "definitions": {
    "CustomDataType": {
      "description": "This class does not get 'AdditionalProperties = false' in the schema generation, so it can be extended with arbitrary JSON properties to allow adding custom data.",
      "javaType": "CustomData",
      "type": "object",
      "properties": {
        "vendorId": {
          "type": "string",
          "maxLength": 255
        }
      },
      "required": [
        "vendorId"
      ]
    },
    "DataTransferStatusEnumType": {
      "description": "This indicates the success or failure of the data transfer.\r\n",
      "javaType": "DataTransferStatusEnum",
      "type": "string",
      "additionalProperties": false,
      "enum": [
        "Accepted",
        "Rejected",
        "UnknownMessageId",
        "UnknownVendorId"
      ]
    },
    "StatusInfoType": {
      "description": "Element providing more information about the status.\r\n",
      "javaType": "StatusInfo",
      "type": "object",
      "additionalProperties": false,
      "properties": {
        "customData": {
          "$ref": "#/definitions/CustomDataType"
        },
        "reasonCode": {
          "description": "A predefined code for the reason why the status is returned in this response. The string is case-insensitive.\r\n",
          "type": "string",
          "maxLength": 20
        },
        "additionalInfo": {
          "description": "Additional text to provide detailed information.\r\n",
          "type": "string",
          "maxLength": 512
        }
      },
      "required": [
        "reasonCode"
      ]
    }
  },
  "type": "object",
  "additionalProperties": false,
  "properties": {
    "customData": {
      "$ref": "#/definitions/CustomDataType"
    },
    "status": {
      "$ref": "#/definitions/DataTransferStatusEnumType"
    },
    "statusInfo": {
      "$ref": "#/definitions/StatusInfoType"
    },
-------------------------
    "data": {
      "description": "Data without specified length or format, in response to request.\r\n"
    }
-------------------------
  },
  "required": [
    "status"
  ]
}```
Jared-Newell-Mobility commented 7 months ago

The OCA has confirmed that AnyType is any type as expected and is reflected in the schema. However, in OCPP 2.0.1 Part 2, Section 2.1.4. Primitive Datatypes the description for AnyType could be improved on, that is to remove the word "Text". This has been raised. So given this, I will close this PR as the current type used is correct.

OrangeTux commented 7 months ago

Thanks for your effort @Jared-Newell-Mobility