Provide the way to customize error responses in JSONRPCProtocol

[dangusev]

Hey! First of all I'd like to say thank you for such a cool lib. There's not such much of them and this one is the most flexible.

Problem: I need to customize the content of error responses in JSONRPCProtocol somehow. As I see, all logic regarding error responses is located in _get_code_message_and_data(). I decided to move it to JSONRPCRequest class because it's the only place where it is using. Thus it's possible to override it now. To override the JSONRPCRequest itself I added attribute JSONRPCProtocol.request_class which should return such class.

Now it's possible to catch special validation-related errors, for example.

As a bonus I simplified the method RPCDispatcher._dispatch a bit.

[lnoor]

Hi Dan,

I like your code simplification of _dispatch(). I gladly include it.

As for the custom error handling I have some reservations.

Have you seen the documentation about Adding Custom Exceptions? This might be just what you are looking for.

If not your requirement may well invalidate the protocol from being JSONRPC. And in that case it would be cleaner to create a new protocol, tinyrpc makes that quite easy.

If neither is an option for you then do lets talk on how to implement your requirements.

regards, Leo

[dangusev]

Hi Inoor and thank you for response!

So, to use Custom exceptions feature I should catch all application's exceptions (which are RPC-agnostic) and re-raise them in RPC methods. It presumes too much of repeatable code.

In my case I just want to customize some error messages, the protocol itself remains the same. For example, I don't want to return descriptions for arbitrary exceptions, there may be some confidential information. And to do that now I should inherit JSONRPCProtocol class, fully copy-paste methods _parse_subrequest and create_request just to replace the JSONRPCRequest class by my own one. I want to avoid the copy-pasting and just to put my custom request class using inheritance only.

The approach I proposed doesn't seem to violate the JSON-RPC protocol) It just adds some flexibility in the code.

[lnoor]

Clearly catching just to reissue is not the way to go. Copying instead or reusing is not the way either. You have your own tree of Exception classes; I get all that.

The nice thing about tinyrpc's exception mixins is that you can combine them with any kind of Exception derived class. So you can have your own Exception class tree and some selected nodes have a FixedErrorMessageMixin added to it. Please have a look if that works for you.

Mind you, I'm not against your solution per se but only as a last resort. It is the request_class property in particular that I dislike. Especially putting it in the RPCRequest class which forces all derived classes to consider it. (There are at least two other protocols, that I know off, implemented on top of tinyrpc) What about a jsonrpc.RPCRequestFactory a la Python3s logging.LogRecordFactory?

[dangusev]

Hm, actually I like your idea of RPCRequestFactory ) We could put it right in the JSONRPCProtocol class, without adding it to the basic RPCProtocol interface to avoid breaking of another derived protocols.

For example:

class JSONRPCProtocol(RPCBatchProtocol):
    def get_request_factory(self):
        return JSONRPCRequest

    def create_request(self, *args, **kwargs):
        ...
        request = self.get_request_factory()()
        ...

    def _parse_subrequest(self, *args, **kwargs):
        ...
        request = self.get_request_factory()()
        ...

What do you think?

[lnoor]

Works for me. I'd put it at the module level instead of the class but either works. Only: factories return instances so your return JSONRPCRequest should become return JSONRPCRequest().

That also improves the use of that function: instead of self.get_request_factory()() it would become self.get_request_factory().

If you change it like this and remove the changes from RPCProtocol I'd be happy to include your changes. I'm in the planning stages for a new release so that will still take some time. But I expect that by the end of the sommer there will be a new tinyrpc release including your changes.

[dangusev]

Ok) At the module level it's not possible to override it if you really need to do that. I'll provide new changes in few hours.

[dangusev]

@lnoor, code has been updated

I also would like to discuss the support of asyncio. Would you mind to do that in a separate PR?

[lnoor]

@dangusev, your commits have been merged.

I'm not really familiar with asyncio. I believe it serves a similar purpose as gevent. The problem with asyncio is that it is Python3 only; isn't it? I don't want to maintain separate versions of tinyrpc for Python2 and Python3. But by all means lets discuss if you know of a solution.

[lnoor]

@dangusev, I've been looking at asyncio. Appears that it would surmount to 'yet another transport'. In that case it is not so much of a problem that it is Python3 only. Looking forward to your suggestions.

[dangusev]

It's not only about transport. It would also affect dispatcher, server and client. In general, you should replace all dispatch() _dispatch() send_reply() with await dispatch() await _dispatch() and so on. And the only way to do that is to inherit current classes and fully copy-paste their methods with such changes. It could seem ugly, but I don't see another way. I will open a new PR with my current adaptation as an example.