Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG] InternalDaprError in actor method call can cause serialisation error when returning exception details #761

Closed
Druid-of-Luhn opened this issue Dec 10, 2024 · 2 comments · Fixed by #765
Labels
kind/bug Something isn't working

Comments

@Druid-of-Luhn
Copy link
Contributor

Expected Behavior

When calling an actor method results in an InternalDaprError being raised, the exception details should be returned without any issue, whatever their content.

Actual Behavior

If the exception details, which are returned as a dict (with ex.as_dict()), contain bytes or any other non-JSON-serialisable value, then a JSON serialisation error is raised rather than returning the details of the initial exception.

Steps to Reproduce the Problem

  1. Have an actor method call fail due to an InternalDaprError.
  2. Have the exception contain a non-JSON-serialisable value (like bytes).
  3. The exception that is thrown is of a JSON serialisation error, rather than a 500 response containing the original exception details.

The issue is caused when line 99 in ext/dapr-ext-fastapi/dapr/ext/fastapi/actor.py is hit, resulting in line 47 trying to serialise a non-serialisable value to JSON (evidently a dict that at some level contains a bytes object).

Stack trace extract:

  File "venv\Lib\site-packages\fastapi\routing.py", line 212, in run_endpoint_function
    return await dependant.call(**values)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "venv\Lib\site-packages\dapr\ext\fastapi\actor.py", line 99, in actor_method
    return _wrap_response(status.HTTP_500_INTERNAL_SERVER_ERROR, ex.as_dict())
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "venv\Lib\site-packages\dapr\ext\fastapi\actor.py", line 47, in _wrap_response
    resp = JSONResponse(content=msg, status_code=status_code)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "venv\Lib\site-packages\starlette\responses.py", line 180, in __init__
    super().__init__(content, status_code, headers, media_type, background)
  File "venv\Lib\site-packages\starlette\responses.py", line 43, in __init__
    self.body = self.render(content)
                ^^^^^^^^^^^^^^^^^^^^
  File "venv\Lib\site-packages\starlette\responses.py", line 183, in render
    return json.dumps(
           ^^^^^^^^^^^
  File "AppData\Local\miniconda3\Lib\json\__init__.py", line 238, in dumps
    **kw).encode(obj)
          ^^^^^^^^^^^
  File "AppData\Local\miniconda3\Lib\json\encoder.py", line 200, in encode
    chunks = self.iterencode(o, _one_shot=True)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "AppData\Local\miniconda3\Lib\json\encoder.py", line 258, in iterencode
    return _iterencode(o, 0)
           ^^^^^^^^^^^^^^^^^
  File "AppData\Local\miniconda3\Lib\json\encoder.py", line 180, in default
    raise TypeError(f'Object of type {o.__class__.__name__} '
TypeError: Object of type bytes is not JSON serializable

Release Note

RELEASE NOTE: FIX Internal Dapr error during actor method call no longer raises a JSON serialisation error.

@Druid-of-Luhn
Copy link
Contributor Author

My temporary fix is to wrap the code that results in the data being serialised to JSON in a try/catch, and using repr if it fails. I'm happy to make a PR with this. It doesn't feel like an ideal solution however.

def _wrap_response(
    status_code: int,
    msg: Any,
    error_code: Optional[str] = None,
    content_type: Optional[str] = DEFAULT_CONTENT_TYPE,
):
    resp = None
    if isinstance(msg, str):
        response_obj = {
            'message': msg,
        }
        if not (status_code >= 200 and status_code < 300) and error_code:
            response_obj['errorCode'] = error_code
        resp = JSONResponse(content=response_obj, status_code=status_code)
    elif isinstance(msg, bytes):
        resp = Response(content=msg, media_type=content_type)
    else:
        # BEGIN NEW CODE
        try:
            resp = JSONResponse(content=msg, status_code=status_code)
        except Exception:
            resp = _wrap_response(status_code, repr(msg), error_code, content_type)
        # END NEW CODE
    return resp

However the actual error comes from the fact that a DaprInternalError contains bytes, so this issue could be mitigated by:

  • Not returning the field in as_dict,
  • Encoding the field in as_dict, or
  • Removing the field after calling as_dict in _wrap_response,
  • Encoding the field after calling as_dict in _wrap_response, or
  • Something else entirely that I haven't thought of.
class DaprInternalError(Exception):
    """DaprInternalError encapsulates all Dapr exceptions"""

    def __init__(
        self,
        message: Optional[str],
        error_code: Optional[str] = ERROR_CODE_UNKNOWN,
        raw_response_bytes: Optional[bytes] = None,
    ):
        self._message = message
        self._error_code = error_code
        self._raw_response_bytes = raw_response_bytes

    def as_dict(self):
        return {
            'message': self._message,
            'errorCode': self._error_code,
            'raw_response_bytes': self._raw_response_bytes,
        }

@elena-kolevska
Copy link
Contributor

Thanks for reporting this @Druid-of-Luhn .

I was able to reproduce the problem and I think it would be best if we added something like this in the DaprInternalError class:

    def json_safe_dict(self):
        error_dict = {'message': self._message, 'errorCode': self._error_code, }

        if self._raw_response_bytes is not None:
            # Encode bytes to base64 for JSON compatibility
            error_dict['raw_response_bytes'] = base64.b64encode(self._raw_response_bytes).decode(
                'utf-8')

        return error_dict

And then in actor.py:

            except DaprInternalError as ex:
                return _wrap_response(status.HTTP_500_INTERNAL_SERVER_ERROR, ex.json_safe_dict())

Would you like to send a PR with these changes and maybe an extra test for them?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants