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

[Jan Cholasta]

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.

-- 
Jan Cholasta