[Freeipa-devel] [PATCH] 0119 Switch client to JSON-RPC

[Petr Viktorin]

On 11/26/2013 03:06 PM, Jan Cholasta wrote:
> On 18.10.2013 12:26, Petr Viktorin wrote:
>> On 10/17/2013 06:08 PM, Jan Cholasta wrote:
>>> Hi,
>>>
>>> On 7.10.2013 18:16, Petr Viktorin wrote:
>>>> On 08/12/2013 10:17 AM, Petr Viktorin wrote:
>>>>> On 08/02/2013 11:13 AM, Petr Viktorin wrote:
>>>>>> On 05/10/2013 04:54 PM, Petr Viktorin wrote:
>>>>>>> On 04/01/2013 11:37 PM, Rob Crittenden wrote:
>>>>>>>> Petr Viktorin wrote:
>>>>>>>>> On 01/15/2013 12:36 PM, Petr Viktorin wrote:
>>>>>>>>>> I meant to hold this patch a while longer to let it mature, but
>>>>>>>>>> from
>>>>>>>>>> what Brian Smith asked on the user list it seems it could help
>>>>>>>>>> him.
>>>>>>>>>>
>>>>>>>>>> Design: http://freeipa.org/page/V3/JSON-RPC
>>>>>>>>>> Ticket: https://fedorahosted.org/freeipa/ticket/3299
>>>>>>>>>>
>>>>>>>>>> See the design page for what the patch does.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> As much as I've tried to avoid them, the code includes some
>>>>>>>>>> workarounds:
>>>>>>>>>> It extends xmlrpclib to also support JSON. This is rather
>>>>>>>>>> intrusive,
>>>>>>>>>> but
>>>>>>>>>> to not do that I'd need to write a parallel stack for JSON,
>>>>>>>>>> without
>>>>>>>>>> the
>>>>>>>>>> help of a standard library.
>>>
>>> It would be nice to write the JSON stack in the future anyway, this
>>> indeed seems intrusive to me.
>>
>> Yes, it would.
>>
>>>>>>>>>> The registration of either jsonclient or xmlclient as
>>>>>>>>>> "rpcclient" in
>>>>>>>>>> the
>>>>>>>>>> API also needs a bit of magic, since the framework requires the
>>>>>>>>>> class
>>>>>>>>>> name to match the attribute.
>>>
>>> You could also put the name of the plugin in a configuration option and
>>> address the plugin like "api.Backend[api.env.rpcclient_plugin]", but I
>>> guess your solution is no worse.
>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> To prevent backwards compatibility problems, we need to ensure
>>>>>>>>>> that
>>>>>>>>>> all
>>>>>>>>>> official JSON clients send the API version, so this patch
>>>>>>>>>> should be
>>>>>>>>>> applied after my patches 0104-0106.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Updating to current master.
>>>>>>>>
>>>>>>>> Please reverse this change in ipalib/rpc.py:
>>>>>>>>
>>>>>>>> @@ -665,8 +788,6 @@ class xmlclient(Connectible):
>>>>>>>>               except Exception, e:
>>>>>>>>                   if not fallback:
>>>>>>>>                       raise
>>>>>>>> -                else:
>>>>>>>> -                    self.log.info('Connection to %s failed with
>>>>>>>> %s',
>>>>>>>> url, e)
>>>>>>>>                   serverproxy = None
>>>>>>>>
>>>>>>>> This logs connection errors when the client fails over to another
>>>>>>>> server.
>>>>>>>
>>>>>>> Thanks. Done, and rebased to master.
>>>
>>> Thanks for the patch, it works for me.
>>>
>>> I have just a few nitpicks:
>>>
>>>       def forward(self, *args, **kw):
>>>           """
>>> -        Forward call over XML-RPC to this same command on server.
>>> +        Forward call over JSON-RPC to this same command on server.
>>>
>>> The new docstring is not entirely true.
>>
>> Fixed, thanks
>>
>>> +    def send_content(self, connection, request_body):
>>> +        if self.protocol == 'json':
>>> +            connection.putheader("Content-Type", "application/json")
>>> +        else:
>>> +            connection.putheader("Content-Type", "text/xml")
>>> +
>>> +        connection.putheader("Content-Length", str(len(request_body)))
>>> +        connection.endheaders(request_body)
>>>
>>> The original implementation of send_content in the Transport class sets
>>> up gzip compression. I think it may be useful to do it here as well.
>>
>> We don't opt in for gzip compression, so that's unnecessary.
>> I've added a comment.
>>
>>> +    def rpc_uri_from_env(self, env):
>>> +        raise NotImplementedError('RPCClient.rpc_uri_from_env')
>>> ...
>>> -            xmlrpc_uri = self.env.xmlrpc_uri
>>> +            rpc_uri = self.rpc_uri_from_env(self.env)
>>>
>>> I don't think this is necessary, since Env is also a mapping, you can do
>>> this instead:
>>>
>>> +    env_rpc_uri_var = None
>>> ...
>>> -            xmlrpc_uri = self.env.xmlrpc_uri
>>> +            rpc_uri = self.env[env_rpc_uri_var]
>>
>> You're right, changed
>>
>>> +class xmlclient(RPCClient):
>>> +    session_path = '/ipa/session/xml'
>>> +    server_proxy_class = ServerProxy
>>> +    protocol = 'xml'
>>> +    wrap_functions = xml_wrap, xml_unwrap
>>> ...
>>> +class jsonclient(RPCClient):
>>> +    session_path = '/ipa/session/json'
>>> +    server_proxy_class = JSONServerProxy
>>> +    protocol = 'json'
>>> +    wrap_functions = identity, identity
>>>
>>> It seems to me that json_encode_binary and json_decode_binary are
>>> equivallent to xml_wrap and xml_unwrap, is there a reason you used the
>>> identity function in jsonclient.wrap_functions?
>>
>> Yes, it's for unwrapping error results. For JSON, we want the decoding
>> done before the error is raised, but for XML no decoding is done (error
>> results can't contain binary data).
>> I've moved this into a single method, hopefully it's clearer this way.
>>
>
> Thanks. ACK once you remove the unused identity function.

Thank you!
Removed the function and pushed to master: 
73b8047b2298d347475a5c8d9f1853052ddced57




-- 
Petr³