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

adding a Python3 fix to JsonRPC serializer #418

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

cloud-rocket
Copy link
Contributor

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

It's probably a leftover from Python 2

@mosquito
Copy link
Owner

mosquito commented Oct 4, 2021

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

@cloud-rocket
Copy link
Contributor Author

There were no tests for JsonRPC I think

@mosquito
Copy link
Owner

mosquito commented Oct 4, 2021

It’s wrong

@decaz
Copy link
Collaborator

decaz commented Oct 4, 2021

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

@cloud-rocket
Copy link
Contributor Author

cloud-rocket commented Oct 7, 2021

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
Copy link
Contributor Author

cloud-rocket commented Oct 8, 2021

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

@cloud-rocket cloud-rocket force-pushed the fix-json-rpc-serialization branch 3 times, most recently from 4060132 to f1f7623 Compare October 8, 2021 16:32
@cloud-rocket
Copy link
Contributor Author

cloud-rocket commented Oct 14, 2021

@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
Copy link
Contributor Author

Why this PR is ignored?

@mosquito
Copy link
Owner

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

@cloud-rocket cloud-rocket force-pushed the fix-json-rpc-serialization branch 2 times, most recently from 5c92d10 to f8337e9 Compare October 26, 2021 17:16
@cloud-rocket
Copy link
Contributor Author

cloud-rocket commented Oct 26, 2021

@mosquito - fixed, sorry. Please run the workflow. Also added proper documentation here - #425

Copy link
Owner

@mosquito mosquito left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a bad job, thanks for writing the tests, and code style in general LGTM, but I have to demand changes because it's completely unsafe.

aio_pika/patterns/rpc.py Outdated Show resolved Hide resolved
aio_pika/patterns/rpc.py Show resolved Hide resolved
Copy link
Owner

@mosquito mosquito left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cloud-rocket, I like what is happening, I have left a few wishes, and I hope they will be taken into consideration.

aio_pika/patterns/rpc.py Show resolved Hide resolved
aio_pika/patterns/rpc.py Outdated Show resolved Hide resolved
@cloud-rocket
Copy link
Contributor Author

@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.

@skrech
Copy link

skrech commented Mar 11, 2022

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

@mosquito
Copy link
Owner

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants