Identify REST exception origin

[ChrnyaevEK]

We are currently dealing with issue - when calling scene_service.start(), this call might take too long (longer then defined 20s. rest timeout) and then will consequently fail with RestException as timeout will happen. We would like to add an opportunity to easily identify the origin of the error for each rest.call call (scene service client in this case).

Proposed solution

When rest.call raise any exception it get's converted by arcor2.exceptions.helpers.handle to SceneServiceException. However, not all service clients has such conversion implemented. Moreover, this logic must be changed after new API convention is implemented as it hides WebApiError properties (convert WebApiError to string to raise required exception type).

It would be enough to implement a function that will accept service unique exception (for scene service it would be SceneServiceException) and will return wrapped rest.call function. Wrapper will catch only RestException exceptions and raise desired type from it (just like arcor2.exceptions.helpers.handle does).

Each client that will want to mark rest error origin will request wrapped rest.call and use it as usually.

[ZdenekM]

Well, originally the intention behind introducing the handle decorator and SceneServiceException was to be able to check where the exception comes from - to differentiate scene-related exceptions from the general Arcor2Exception or RestException. And also to provide more user-friendly messages as we (usually) can't directly pass messages from underlying services to users. I checked the code and it seems that we are actualy (mostly) not using the messages defined in handle. On the other hand, it would be nice to be able to catch scene/project service-specific exceptions. Maybe we might just derive SceneServiceException and ProjectServiceException from WebApiError and the handle decorator will just convert plain WebApiError to a more specific type so it can be properly handled?

[ChrnyaevEK]

In case of WebApiError, yes, it is actually a nice idea to define some subclasses and use regular try...except... . I guess it would be enough to update SceneServiceException like this:

from arcor2.data.common import WebAbiError
class SceneServiceWebApiError(WebApiError):
    pass

But there is another requirement from our PO (product owner) - rest.call should raise exceptions with some service identifier. This could be ether exception.origin/caller= "scene_service" | "aubo" | "capture" | ... property or some sort of service specific exception type... I'm not really sure how to do this properly, so to keep python exception handling system I suggest that each service will in fact define it's own RestException and then not just import rest.call, but request it's own rest.call that will reraise RestExceptions

--- in arcor2.clients.scene_service.py ---
from arcor2.data.common import WebAbiError
from arcor2.rest import RestException, get_call 
... or ...
from arcor2.rest import RestException, RestClient

class SceneServiceWebApiError(WebApiError):
    pass

class SceneServiceRestException(RestException):
    pass

call = get_call(SceneServiceRestException, SceneServiceWebApiError)
... or ...
client = RestClient(SceneServiceRestException, SceneServiceWebApiError)

--- in arcor2.rest ---
def get_call(exception_type, web_api_exception_type):
    def wrapped_rest_call(*args, **kwargs):
        try:
            return call(*args, **kwargs)
        except RestException as e:
            raise exception_type from e
        except WebApiError as e:
            raise cast(web_api_exception_type, e)  # or something like this
     return wrapped_rest_call

... or ...

class RestClient:
    def __init__(rest_exception_type, web_api_exception_type):
        self._exception_type = exception_type
        self._web_api_exception_type = web_api_exception_type

     def call(*args, **kwargs):
        try:
            return call(*args, **kwargs)
        except RestException as e:
            raise self._exception_type from e
        except WebApiError as e:
            raise cast(self._web_api_exception_type, e)  # or something like this

Still there will be problems importing those exceptions from our OT's, but I don't think that use properties is better idea:

--- in arcor2.clients.scene_service.py ---
from arcor2.data.common import WebAbiError
from arcor2.rest import RestException, get_call 

class SceneServiceWebApiError(WebApiError):
    pass

call = get_call('scene_service',SceneServiceWebApiError)

--- in arcor2.rest ---
class RestException:
    origin = None

def get_call(exception_type, web_api_exception_type):
    def wrapped_rest_call(*args, **kwargs):
        try:
            return call(*args, **kwargs)
        except RestException as e:
            e.origin = exception_type
            raise
        except WebApiError as e:
            raise cast(web_api_exception_type, e)  # or something like this
     return wrapped_rest_call

[ZdenekM]

Well, the Pythonic way would be to define service-specific exceptions, and handle them using try ... except, where there might be multiple exceptions, from specific to general...

try:
    aubo.something_that_calls_scene_service_inside()
except AuboSpecificException:
    pass
except AuboException:
    pass
except SceneServiceException:
    pass
except RestException:
    pass
except Arcor2Exception:
    pass
except Exception:
    pass

The identifier is the exception type itself. Everything else seems to me as unnecessarily complicated and against Python's nature (e.g. single except and then some conditions). I'm not sure what will be the advantage of having the origin property. But if it is really necessary for you, what about something simple like this?

from arcor2.data.common import WebAbiError

class AuboException(WebAbiError):
    origin = "aubo"

...or this:

class KinaliBaseException(WebApiError):
    
    @property
    def origin():
        return self.__class__.__name__[:-9]

class AuboException(KinaliBaseException):
    pass

...moreover, there is already the service property - isn't it the same as origin?

[ChrnyaevEK]

Thanks for your advices, I've reviewed the issue and our requirements and it seems like we can solve it on our side by using @handle decorator with proper exceptions, just like scene_service.

There still is an problem with such approach, which blocks solving current issue. rest.call may generally raise RestException(Arcor2Exception) and WebApiError(Arcor2Exception), @handle decorator on scene_service will catch any Arcor2Exception and raise SceneException, so WebApiError will be replaced with SceneException too. This new exception does no longer have service, type, content and other WebApiError properties, which we need to have when catching exception as for example type property will be used to select proper dialog. I guess it's enough to update @handle behaviour so it handle WebApiError differently from other exceptions. As well we may define some kind of SceneWebApiError to keep this pythonic.

This is related to WebApiError integration to REST clients, do you want to create new issue for that? Current issue could be left for now as it requires modification of @handle decorator as described above.

[ZdenekM]

I guess it's enough to update @handle behaviour so it handle WebApiError differently from other exceptions. As well we may define some kind of SceneWebApiError to keep this pythonic.

Yes, I agree. Changing the ancestor of SceneServiceException to WebApiError is not a problem at all - it won't break any of our code. There is no reason for having two scene service-related exceptions.

In the meantime, you may use something like this to get the "parent" WebApiError from SceneServiceException:

try:
    scene_client.whatever()
except SceneServiceException as e:
    if isinstance(e.__context__, WebApiError):
        pass

[ZdenekM]

This is related to WebApiError integration to REST clients, do you want to create new issue for that? Current issue could be left for now as it requires modification of @handle decorator as described above.

Do you mean project_service and scene_service, right, or something else? Maybe it is one issue? Well, for project_service, we can also change the base of ProjectServiceException to WebApiError -

[ZdenekM]

And by the way, wouldn't it be more logical to base WebApiError not on Arcor2Exception but on RestException? It might simplify catching errors a bit.

[ChrnyaevEK]

Well, using e.__context__ actually solves the whole problem with handle decorator and new WebApiError, thanks! For now we will go with this approach, it seems to be fine. Current issue may stay open until solution is implemented (I will close the issue as soon as we finish integration).

Yes, it make sense to derive WebApiError from RestException and could be changed in future versions.