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

_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to