aio-pika icon indicating copy to clipboard operation
aio-pika copied to clipboard

adding a Python3 fix to JsonRPC serializer

Open cloud-rocket opened this issue 2 years ago • 13 comments

JsonRPC is throwing and error encoding error "string argument without an encoding" without this fix.

It's probably a leftover from Python 2

cloud-rocket avatar Oct 04 '21 16:10 cloud-rocket

Very strange fix. When your right that's not working right now, but tests says isn't this.

mosquito avatar Oct 04 '21 17:10 mosquito

There were no tests for JsonRPC I think

cloud-rocket avatar Oct 04 '21 19:10 cloud-rocket

It’s wrong

mosquito avatar Oct 04 '21 20:10 mosquito

@cloud-rocket please give an example code which reproduces error.

decaz avatar Oct 04 '21 20:10 decaz

I am working on tests and the following code reproduces the error:

def rpc_func(*, foo, bar):
    assert not foo
    assert not bar

    return {"foo": "bar"}


    async def test_jsonrpc_simple(self, channel: aio_pika.Channel):
        rpc = await JsonRPC.create(channel, auto_delete=True)

        await rpc.register("test.rpc", rpc_func, auto_delete=True)

        result = await rpc.proxy.test.rpc(foo=None, bar=None)
        assert result == {"foo": "bar"}

        await rpc.unregister(rpc_func)
        await rpc.close()

        # Close already closed
        await rpc.close()

This code works with my fix.

But my fix is not dealing with the case when an exception is thrown. So the fix is only partly solving the problem. Trying to find what's wrong with the exception serialization....

cloud-rocket avatar Oct 07 '21 18:10 cloud-rocket

Everything fixed (including exception handling) and tests are added.

cloud-rocket avatar Oct 08 '21 15:10 cloud-rocket

@mosquito, what do you think is wrong? The tests I added reproduce the problem in the original code and it works with the fix.

@decaz, what do you think?

cloud-rocket avatar Oct 14 '21 17:10 cloud-rocket

Why this PR is ignored?

cloud-rocket avatar Oct 25 '21 23:10 cloud-rocket

@cloud-rocket why have the fallen tests have been ignored?

mosquito avatar Oct 26 '21 08:10 mosquito

@mosquito - fixed, sorry. Please run the workflow. Also added proper documentation here - https://github.com/mosquito/aio-pika/issues/425

cloud-rocket avatar Oct 26 '21 17:10 cloud-rocket

@mosquito -

Are you open to consider this version of deserialize?

    def deserialize(self, data: Any) -> bytes:
        res = RPC.deserialize(self, data)
        if isinstance(res, dict) and "error" in res:
            cls = locate(res['error']['type'])
            if cls:
                res = cls(res['error']['message'], res['error']['args'])
            else:
                res = JsonRPCError(res['error']['type'],
                                   res['error']['message'],
                                   res['error']['args'])
        return res

I found myself anyway using it, because known exceptions are easier to handle and there is no vulnerability here as far as I understand.

cloud-rocket avatar Nov 24 '21 22:11 cloud-rocket

@mosquito, if this PR is OK, would you merge it, because right now we have to make nasty tricks to make this functionality work.

skrech avatar Mar 11 '22 15:03 skrech

@screech since bump aio-pika>7.0.0, the JsonRPC-related classes was refactored and now you may change theirs behaviour with inheritance.

mosquito avatar Mar 11 '22 15:03 mosquito