Improve Exception handling and CallError responses (mobilityhouse#335)

* Added missing NotSupported to the exception list.
* Changed usage of NotImplemented to NotSupported when a handler doesn't exist.
*Use the default description and moved additional information to details
* Reduce the usage of ValidationError as it is not a valid OCPP error type, doesn't generate a CallError, and crashes the code when handling messages. I've tried to replace them with relevant OCPP error types.

Author: proelke

ocpp/charge_point.py

@@ -9,7 +9,7 @@
 
 from ocpp.routing import create_route_map
 from ocpp.messages import Call, validate_payload, MessageType
-from ocpp.exceptions import OCPPError, NotImplementedError
+from ocpp.exceptions import OCPPError, NotSupportedError
 from ocpp.messages import unpack
 
 LOGGER = logging.getLogger('ocpp')
@@ -171,8 +171,8 @@ async def _handle_call(self, msg):
         try:
             handlers = self.route_map[msg.action]
         except KeyError:
-            raise NotImplementedError(f"No handler for '{msg.action}' "
-                                      "registered.")
+            raise NotSupportedError(
+                details={"cause": f"No handler for {msg.action} registered."})
 
         if not handlers.get('_skip_schema_validation', False):
             validate_payload(msg, self._ocpp_version)
@@ -187,8 +187,8 @@ async def _handle_call(self, msg):
         try:
             handler = handlers['_on_action']
         except KeyError:
-            raise NotImplementedError(f"No handler for '{msg.action}' "
-                                      "registered.")
+            raise NotSupportedError(
+                details={"cause": f"No handler for {msg.action} registered."})
 
         try:
             response = handler(**snake_case_payload)

ocpp/exceptions.py

@@ -31,14 +31,19 @@ def __str__(self):
 
 class NotImplementedError(OCPPError):
     code = "NotImplemented"
-    default_description = "Request Action is recognized but not supported by \
-                          the receiver"
+    default_description = "Requested Action is not known by receiver"
+
+
+class NotSupportedError(OCPPError):
+    code = "NotSupported"
+    default_description = ("Request Action is recognized but not supported by "
+                           "the receiver")
 
 
 class InternalError(OCPPError):
     code = "InternalError"
-    default_description = "An internal error occurred and the receiver was \
-                          able to process the requested Action successfully"
+    default_description = ("An internal error occurred and the receiver was "
+                           "able to process the requested Action successfully")
 
 
 class ProtocolError(OCPPError):
@@ -48,35 +53,35 @@ class ProtocolError(OCPPError):
 
 class SecurityError(OCPPError):
     code = "SecurityError"
-    default_description = "During the processing of Action a security issue \
-                          occurred preventing receiver from completing the \
-                          Action successfully"
+    default_description = ("During the processing of Action a security issue "
+                           "occurred preventing receiver from completing the "
+                           "Action successfully")
 
 
 class FormatViolationError(OCPPError):
     code = "FormatViolation"
-    default_description = "Payload for Action is syntactically incorrect or \
-                          structure for Action"
+    default_description = ("Payload for Action is syntactically incorrect or "
+                           "structure for Action")
 
 
 class PropertyConstraintViolationError(OCPPError):
     code = "PropertyConstraintViolation"
-    default_description = "Payload is syntactically correct but at least \
-                          one field contains an invalid value"
+    default_description = ("Payload is syntactically correct but at least "
+                           "one field contains an invalid value")
 
 
 class OccurenceConstraintViolationError(OCPPError):
     code = "OccurenceConstraintViolation"
-    default_description = "Payload for Action is syntactically correct but \
-                          at least one of the fields violates occurence \
-                          constraints"
+    default_description = ("Payload for Action is syntactically correct but "
+                           "at least one of the fields violates occurence "
+                           "constraints")
 
 
 class TypeConstraintViolationError(OCPPError):
     code = "TypeConstraintViolation"
-    default_description = "Payload for Action is syntactically correct but at \
-                          least one of the fields violates data type \
-                          constraints (e.g. “somestring”: 12)"
+    default_description = ("Payload for Action is syntactically correct but "
+                           "at least one of the fields violates data type "
+                           "constraints (e.g. “somestring”: 12)")
 
 
 class GenericError(OCPPError):

ocpp/messages.py

@@ -12,7 +12,8 @@
 from ocpp.exceptions import (OCPPError, FormatViolationError,
                              PropertyConstraintViolationError,
                              ProtocolError, TypeConstraintViolationError,
-                             ValidationError, UnknownCallErrorCodeError)
+                             NotImplementedError, ValidationError,
+                             UnknownCallErrorCodeError)
 
 _validators: Dict[str, Draft4Validator] = {}
 
@@ -65,22 +66,29 @@ def unpack(msg):
     """
     try:
         msg = json.loads(msg)
-    except json.JSONDecodeError as e:
-        raise FormatViolationError(f'Message is not valid JSON: {e}')
+    except json.JSONDecodeError:
+        raise FormatViolationError(
+            details={"cause": "Message is not valid JSON"})
 
     if not isinstance(msg, list):
-        raise ProtocolError("OCPP message hasn't the correct format. It "
-                            f"should be a list, but got {type(msg)} instead")
+        raise ProtocolError(
+            details={"cause": ("OCPP message hasn't the correct format. It "
+                               f"should be a list, but got '{type(msg)}' "
+                               "instead")})
 
     for cls in [Call, CallResult, CallError]:
         try:
             if msg[0] == cls.message_type_id:
                 return cls(*msg[1:])
         except IndexError:
-            raise ProtocolError("Message doesn\'t contain MessageTypeID")
+            raise ProtocolError(
+                details={"cause": "Message does not contain MessageTypeId"})
+        except TypeError:
+            raise ProtocolError(
+                details={"cause": "Message is missing elements."})
 
-    raise PropertyConstraintViolationError(f"MessageTypeId '{msg[0]}' isn't "
-                                           "valid")
+    raise PropertyConstraintViolationError(
+        details={f"MessageTypeId '{msg[0]}' isn't valid"})
 
 
 def pack(msg):
@@ -184,9 +192,9 @@ def validate_payload(message, ocpp_version):
             validator = get_validator(
                 message.message_type_id, message.action, ocpp_version
             )
-    except (OSError, json.JSONDecodeError) as e:
-        raise ValidationError("Failed to load validation schema for action "
-                              f"'{message.action}': {e}")
+    except (OSError, json.JSONDecodeError):
+        raise NotImplementedError(
+            details={"cause": f"Failed to validate action: {message.action}"})
 
     try:
         validator.validate(message.payload)
@@ -201,8 +209,9 @@ def validate_payload(message, ocpp_version):
             raise TypeConstraintViolationError(
                 details={"cause": e.message}) from e
         else:
-            raise ValidationError(f"Payload '{message.payload} for action "
-                                  f"'{message.action}' is not valid: {e}")
+            raise FormatViolationError(
+                details={"cause": f"Payload '{message.payload} for action "
+                         f"'{message.action}' is not valid: {e}"})
 
 
 class Call:

tests/test_messages.py

@@ -8,6 +8,7 @@
                              FormatViolationError,
                              PropertyConstraintViolationError,
                              TypeConstraintViolationError,
+                             NotImplementedError,
                              UnknownCallErrorCodeError)
 from ocpp.messages import (validate_payload, get_validator, _validators,
                            unpack, Call, CallError, CallResult, MessageType,
@@ -239,7 +240,7 @@ def test_validate_payload_with_non_existing_schema():
         payload={'invalid_key': True},
     )
 
-    with pytest.raises(ValidationError):
+    with pytest.raises(NotImplementedError):
         validate_payload(message, ocpp_version="1.6")
 
 

tests/v16/test_v16_charge_point.py

@@ -3,7 +3,7 @@
 import asyncio
 from unittest import mock
 
-from ocpp.exceptions import ValidationError, GenericError
+from ocpp.exceptions import FormatViolationError, GenericError
 from ocpp.messages import CallError
 from ocpp.routing import on, after, create_route_map
 from ocpp.v16.enums import Action
@@ -115,9 +115,9 @@ async def test_route_message_with_no_route(base_central_system,
         json.dumps([
             4,
             1,
-            "NotImplemented",
-            "No handler for \'Heartbeat\' registered.",
-            {}
+            "NotSupported",
+            "Request Action is recognized but not supported by the receiver",
+            {"cause": "No handler for Heartbeat registered."}
         ],
             separators=(',', ':')
         )
@@ -147,7 +147,7 @@ async def test_send_call_with_timeout(connection):
 async def test_send_invalid_call(base_central_system):
     payload = call.ResetPayload(type="Medium")
 
-    with pytest.raises(ValidationError):
+    with pytest.raises(FormatViolationError):
         await base_central_system.call(payload)
 
 

tests/v20/test_v20_charge_point.py

@@ -73,9 +73,9 @@ async def test_route_message_with_no_route(base_central_system,
         json.dumps([
             4,
             1,
-            "NotImplemented",
-            "No handler for \'Heartbeat\' registered.",
-            {}
+            "NotSupported",
+            "Request Action is recognized but not supported by the receiver",
+            {"cause": "No handler for Heartbeat registered."}
         ],
             separators=(',', ':')
         )

tests/v201/test_v201_charge_point.py

@@ -73,9 +73,9 @@ async def test_route_message_with_no_route(base_central_system,
         json.dumps([
             4,
             1,
-            "NotImplemented",
-            "No handler for \'Heartbeat\' registered.",
-            {}
+            "NotSupported",
+            "Request Action is recognized but not supported by the receiver",
+            {"cause": "No handler for Heartbeat registered."}
         ],
             separators=(',', ':')
         )