Improved error handling during schema validation

[joaomariord]

Added error differentiation when schema is invalid. supporting:
- :
- Type mismatch: TypeConstraintViolationError
- Lack of properties: ProtocolError
- Non-requested properties: FormatViolationError

Now catches exceptions thrown by schema validation errors instead of
dropping the WS connection.

Previous behavior was to drop the socket connection whenever a request payload was not compliant with the schema, this PR introduces support for schema error handling according to the OCPP specification (from 1.6 to 2.0.1).

[OrangeTux]

Hello @joaomariord, thanks for the PR! Although it's a backwards incompatible change, I think it's a good improvement.
Can you make sure to fix the lint errors?

[joaomariord]

Hello @OrangeTux, thank you for promptly answering the PR, I've corrected the lint errors and adapted tests for the new behavior.

I think we could keep the NotImplemented exception for core ("required") OCPP routes, and only answer with CALLERROR-NotImplemented for the optional routes. I can do a PR on this if you agree.

Another thing that would be nice is to allow the programmer to give is own exception and error handling routines. I think the decorator could be used set the error handling callback (e.g. @on(Action.Heartbeat, my_error_handler)). An alternative, more complete, way would be to allow the routing of the errors to a given error handle (e.g. @error(Action.Hearbeat, TypeConstraintError)).
These are just suggestions where I would like some insight on, I can put time to this if its worthy and makes sense.

[OrangeTux]

Another thing that would be nice is to allow the programmer to give is own exception and error handling routines. I think the decorator could be used set the error handling callback (e.g. @on(Action.Heartbeat, my_error_handler)). An alternative, more complete, way would be to allow the routing of the errors to a given error handle (e.g. @error(Action.Hearbeat, TypeConstraintError)).

Thanks for the feedback. The current routing system works fine for most cases, but is limited if your the use case is more complex. You give a good example of one of the routing system's limitation: one is unable to control the response of a call if any exception is raised in this library. #295 highlights another limitation.

With your suggestion you gave me some food for though.

[jainmohit2001]

An alternative, more complete, way would be to allow the routing of the errors to a given error handle (e.g. @error(Action.Hearbeat, TypeConstraintError)). These are just suggestions where I would like some insight on, I can put time to this if its worthy and makes sense.

This would actually help a lot. I am trying to build a fully-fledged CSMS and this is much needed to handle all possible real-world scenarios.

[OrangeTux]

Let's explore the idea of the @error() handler.

1. What arguments should receive?
2. What return value should it return (if any)?

You suggest an action specific route using @error(Action.Heartbeat). I guess people probably want to have a 'catch all' error handler. Would that change the answer on the questions above?

[jainmohit2001]

After reading both the documentation for OCPP-1.6 and OCPP-2.0.1, I found that the errors are based on whether the payload received by the CSMS follows the RPC framework or whether the CP sends a CALL which is not a BootNotification request or a message triggered by TriggerMessage request

@joaomariord handled the first case elegantly.
And for the second one, I have to apply a check inside all the @on decorated functions for the CSMS as well as on the CP side as mentioned in #295 .

If it's possible to create a 'catch all' error handler that knows from which @on decorated function this error is generated, e.g., Action.Heartbeat, and what is the error type, e.g. SecurityError, then we can leave the implementation up to the user. This can help the user customize the actions it has to take on some specific set of errors generated and by which function it is being raised.

[joaomariord]

@jainmohit2001 I completely agree with your idea of a 'catch all' error handler.
If we can have an umbrella error handler, it seems there is no need for handling each specific error with a unique handler, as we can also do this in the umbrella handler.
There is only one quirk about that, maybe it is more pythonic to have something like:

@on(Action.Hearbeat)
def heartbeat_handler():
    ....

@error(Action.Heartbeat, ProtocolError)
def protocol_error_handler(error: ProtocolError):
   .....
   return True # On Error handled
   return False # On not handled

instead of:

def ProtocolErrorHandler(error: ProtocolError): -> bool 
   
   return True # On Error handled
   return False # On not handled

def heartbeat_error_handler(error: BaseException | Any):
   sub_handler = {
           ProtocolError.__class__: ProtocolErrorHandler,
           ... #Other Handlers
   }.get(error.__class__, None)   
   if sub_handler in not None:
       return sub_handler()
   else:
       return False

@on(Action.Hearbeat, heartbeat_error_handler)
def heartbeat_handler():
    ....

The "switch-case" is just a quick and dirty way to do it, but ignore it for the sake of the example, this could be solved in many and more elegant ways.

[jainmohit2001]

I actually thought of something else though.
Can we have a default error handler in the ChargePoint class that does nothing and is called whenever a @on method raises an error? This method can be overridden by the user itself if the user wishes to handle the errors.

[joaomariord]

That should be easy enough, it's an elegant solution.

[OrangeTux]

You mean something like this, @jainmohit2001 ?

# ocpp.charge_point.py

class ChargePoint:
    ...
  
    async def on_exception(self, call: Call, exc: Exception) -> Union[CallResult, CallError]:
         """ Default implementation just reraise the error to keep backwards compatibility. """
         raise exc

from ocpp.v16 import ChargePoint

class MyChargePoint(ChargePoint):
    ....

    async def on_exception(self, call: Call, exc: Exception) -> Union[CallResult, CallError]:
         """ Override that allows for custom error handling. """
         if isinstance(call, call.Heartbeat):
             return call_result.HeartbeatPayload(timestamp=datetime.now(tz=timezone.utc).isoformat())
        
        raise exc

A couple of notes about this implementation:

1. One needs to either raise an exception when overriding on_exception() or return a CallError or CallResult. What should be done if one doesn't do that and returns a different type?

2. This implementation only handlers exceptions that happened while receiving and handling calls. Users of this library might falsely assume the handler is also executed for exceptions raised while sending calls. We might want to change the name of on_exception() to something more specific. on_exceptions_while_handling_calls() would be very specific, but also a bit verbose. Not sure if you've better suggestion.

[jainmohit2001]

Yeah something like this, but where can we integrate this in the code?

I think the following should preserve the backward compatibility

# ocpp.charge_point.py
try:
    response = handler(**snake_case_payload)
    if inspect.isawaitable(response):
        response = await response
except Exception as e:
    LOGGER.exception("Error while handling request '%s'", msg)
    response = msg.create_call_error(e).to_json()
    await self._send(response)

    # await self.on_exception() # Pass the relevant args and kwargs

    return

I think a simple return statement should suffice.

async def on_exception(self, call: Call, exc : Exception):
    """ Do nothing """
    return

I never meant to change the way the code currently handles the exceptions. That is required and should not be changed. I just wanted to do something more after the error is raised and the corresponding CALLERROR is sent. Mostly calling another ChargePoint method to fetch data from CP.

[jainmohit2001]

We might want to change the name of on_exception() to something more specific. on_exceptions_while_handling_calls() would be very specific, but also a bit verbose. Not sure if you've better suggestion.

Yeah, I totally agree with you here. The name of the function should reflect that it is called after the CALLERROR is sent and the user wants to do something more.

[proelke]

Personally I like the proposed idea of an error decorator rather than a single function. A single function will end up with a rather long set of isinstance() statements depending on how many messages have error handling implemented for.

[jainmohit2001]

Personally I like the proposed idea of an error decorator rather than a single function. A single function will end up with a rather long set of isinstance() statements depending on how many messages have error handling implemented for.

Thanks for your opinion @proelke. I agree with you, the idea of an @error decorator is something that can be achieved in a long term maybe. What I proposed was a quick solution to the problem that I am facing right now.

[proelke]

I think it could be added quite quickly and I can't imagine off hand how it could break backwards compatibility. It could be designed based on the after decorator.

[jainmohit2001]

I think it could be added quite quickly and I can't imagine off hand how it could break backwards compatibility. It could be designed based on the after decorator.

If it's possible, please include this funtionality. @joaomariord @OrangeTux @proelke .

[joaomariord]

@OrangeTux can you review this PR? It would be great to have this merged by the end of the week.

I think that what we discussed needs to be further developed and tested, and in that sense this PR could advance right away.

[OrangeTux]

I've a last suggestion that combines several of the discussed variants in one.

What if we introduce a decorater @after_exception() with the following signature:

class Charger(CP):
    @after_exception()
    async def on_exception(self, call: Call, exc: Exception) -> None:
         """ Override that allows for custom error handling. """
         if isinstance(call, call.Heartbeat):
             // do something

The handler has similar semantics as the @after() decorator. It receives the Call that caused the exception and the exception itself. Similar to @after() any return value is ignored and any exception raised in the handler is ignored too.
Similar to @after(), @after_exception() is executed after the CallError has been send to the other peer. In other words, this handler can't influence that response.

I choose the decorator approach over the approach of overriding a method. The former is consistent with @after() and allows for adding multiple handlers in the future.

In a future release we could implement filtering by passing arguments to @after_exception() so that we can call it like @after_exception(action = 'Heartbeat' | 'MeterValue') or @after_exception(exc_type=ProtocolError).
But before we do that, I'm curious to see how this API holds up. Even without these arguments to filter for specific calls or exceptions, one can implement such filtering themself.

The change is backwards compatible.

If an exception is raised in the @on() handler, this @after_exception() is called. The regular @after() should not be called.

@joaomariord I have no plans to work on this topic this week, but I'm happy to receive a PR.

[joaomariord]

@joaomariord I have no plans to work on this topic this week, but I'm happy to receive a PR.

I was talking about the current PR. If I have the time I can try to work our conversation into an actual PR.

[joaomariord]

Hi @OrangeTux, @jainmohit2001, and @proelke, I've not been available for the past few weeks, but have been passing my eyes through the code and something came up.
Could this be done with the current state of the code?

Would something like:

• SkipSchemaValidation on @on,
• A call to validate_payload(snake_to_camel_case(payload)) inside the on_action handler method
• Wrap that call inside try except

, work?

This could be easily made with a custom decorator, like this one:

def ocpp_error_handler(f, y=None):
    async def wrapper(payload):
        result = None
        try:
            validate_payload(snake_to_camel_case(payload), ocpp_version)
            result = await f(payload)
        except OCPPError as e:
            log(e)
            if y is not None:
                result = await y()

        return result

    setattr(wrapper, "__name__", f.__name__)
    return wrapper

There could be a problem with the return, but I leave it for you to find that and other problems :D