launchpad-reviewers team mailing list archive

[John A Meinel <john@xxxxxxxxxxxxxxxxx>]

John A Meinel has proposed merging lp:~jameinel/maas/allow-json into lp:maas.

Commit message:
Add MAASClient.post(as_json=True) to enable users of the client to post their data in a more efficient manner if the API on the other side supports it.

Testing shows that if the data being uploaded is large, you can see a big improvement in performance.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~jameinel/maas/allow-json/+merge/128512

This updates MAASClient so that it can take an 'as_json' parameter.

This changes how the data gets POSTed, sending it as JSON encoding, rather than MimeMultipart.

In my testing, this is dramatically faster for the tag processing. (It drops the time to update 10,000 nodes from 45s down to 35s.)

I will be posting a follow up patch that does that code.

The main reason it is still a flag vs the default, is because the API side needs some updates to support this. It seems that most of our API code uses 'request.POST' but this comes in as 'request.data'. Also, multipart comes in as a multidict (mapping each parameter to a list) while if you pass a dict as json you get just a dict on the other side.

-- 
https://code.launchpad.net/~jameinel/maas/allow-json/+merge/128512
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jameinel/maas/allow-json into lp:maas.

=== added file 'src/apiclient/encode_json.py'
--- src/apiclient/encode_json.py	1970-01-01 00:00:00 +0000
+++ src/apiclient/encode_json.py	2012-10-08 14:57:19 +0000
@@ -0,0 +1,32 @@
+# Copyright 2012 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Encoding requests as JSON data."""
+
+from __future__ import (
+    absolute_import,
+    print_function,
+    unicode_literals,
+    )
+
+__metaclass__ = type
+__all__ = [
+    'encode_json_data',
+    ]
+
+import json
+
+
+def encode_json_data(params):
+    """Encode params as JSON and set up headers for an HTTP POST.
+
+    :param params: Can be a dict or a list, but should generally be a dict, to
+    match the key-value data expected by most receiving APIs.
+    :return: (body, headers)
+    """
+    body = json.dumps(params)
+    headers = {
+        'Content-Length': len(body),
+        'Content-Type': 'application/json',
+        }
+    return body, headers

=== modified file 'src/apiclient/maas_client.py'
--- src/apiclient/maas_client.py	2012-10-05 07:38:54 +0000
+++ src/apiclient/maas_client.py	2012-10-08 14:57:19 +0000
@@ -19,6 +19,7 @@
 from urllib import urlencode
 import urllib2
 
+from apiclient.encode_json import encode_json_data
 from apiclient.multipart import encode_multipart_data
 import oauth.oauth as oauth
 
@@ -138,7 +139,7 @@
         self.auth.sign_request(url, headers)
         return url, headers
 
-    def _formulate_change(self, path, params):
+    def _formulate_change(self, path, params, as_json=False):
         """Return URL, headers, and body for a non-GET request.
 
         This is similar to _formulate_get, except parameters are encoded as
@@ -146,6 +147,9 @@
 
         :param path: Path to the object to issue a GET on.
         :param params: A dict of parameter values.
+        :param as_json: Encode params as application/json instead of
+            application/x-www-form-urlencoded. Only use this if you know the
+            API already supports JSON requests.
         :return: A tuple: URL, headers, and body for the request.
         """
         url = self._make_url(path)
@@ -153,7 +157,10 @@
             params = dict(params)
             op = params.pop('op')
             url += '?' + urlencode({'op': op})
-        body, headers = encode_multipart_data(params, {})
+        if as_json:
+            body, headers = encode_json_data(params)
+        else:
+            body, headers = encode_multipart_data(params, {})
         self.auth.sign_request(url, headers)
         return url, headers, body
 
@@ -170,13 +177,16 @@
         return self.dispatcher.dispatch_query(
             url, method="GET", headers=headers)
 
-    def post(self, path, op, **kwargs):
+    def post(self, path, op, as_json=False, **kwargs):
         """Dispatch POST method `op` on `path`, with the given parameters.
 
+        :param as_json: Instead of POSTing the content as multipart
+            x-www-form-urlencoded post it as application/json
         :return: The result of the dispatch_query call on the dispatcher.
         """
         kwargs['op'] = op
-        url, headers, body = self._formulate_change(path, kwargs)
+        url, headers, body = self._formulate_change(
+            path, kwargs, as_json=as_json)
         return self.dispatcher.dispatch_query(
             url, method="POST", headers=headers, data=body)
 

=== modified file 'src/apiclient/testing/django.py'
--- src/apiclient/testing/django.py	2012-09-22 19:35:59 +0000
+++ src/apiclient/testing/django.py	2012-10-08 14:57:19 +0000
@@ -14,8 +14,17 @@
 
 from io import BytesIO
 
+from django.core.handlers.wsgi import WSGIRequest
 from django.core.files.uploadhandler import MemoryFileUploadHandler
 from django.http.multipartparser import MultiPartParser
+from piston import emitters
+from piston.utils import translate_mime
+from maasserver.utils import ignore_unused
+
+
+# Importing emitters has a side effect of registering mime type handlers with
+# utils.translate_mime. So we must import it, even though we don't use it.
+ignore_unused(emitters)
 
 
 def parse_headers_and_body_with_django(headers, body):
@@ -46,3 +55,20 @@
         META=meta, input_data=BytesIO(body),
         upload_handlers=[handler])
     return parser.parse()
+
+
+def parse_headers_and_body_with_mimer(headers, body):
+    """Use piston's Mimer functionality to handle the content.
+
+    :return: The value of 'request.data' after using Piston's translate_mime on
+        the input.
+    """
+    environ = {'wsgi.input': BytesIO(body)}
+    for name, value in headers.items():
+        environ[name.upper().replace('-', '_')] = value
+    environ['REQUEST_METHOD'] = 'POST'
+    environ['SCRIPT_NAME'] = ''
+    environ['PATH_INFO'] = ''
+    request = WSGIRequest(environ)
+    translate_mime(request)
+    return request.data

=== added file 'src/apiclient/tests/test_encode_json.py'
--- src/apiclient/tests/test_encode_json.py	1970-01-01 00:00:00 +0000
+++ src/apiclient/tests/test_encode_json.py	2012-10-08 14:57:19 +0000
@@ -0,0 +1,36 @@
+# Copyright 2012 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Test encoding requests as JSON."""
+
+from __future__ import (
+    absolute_import,
+    print_function,
+    unicode_literals,
+    )
+
+__metaclass__ = type
+__all__ = []
+
+
+from apiclient.encode_json import encode_json_data
+from maastesting.testcase import TestCase
+
+
+class TestEncodeJSONData(TestCase):
+
+    def assertEncodeJSONData(self, expected_body, expected_headers, params):
+        self.assertEqual(
+            (expected_body, expected_headers),
+            encode_json_data(params))
+
+    def test_encode_empty_dict(self):
+        self.assertEncodeJSONData(
+            '{}', {'Content-Length': 2, 'Content-Type': 'application/json'},
+            {})
+
+    def test_encode_dict(self):
+        self.assertEncodeJSONData(
+            '{"alt": [1, 2, 3, 4], "param": "value"}',
+            {'Content-Length': 39, 'Content-Type': 'application/json'},
+            {'param': 'value', 'alt': [1, 2, 3, 4]})

=== modified file 'src/apiclient/tests/test_maas_client.py'
--- src/apiclient/tests/test_maas_client.py	2012-10-05 07:38:54 +0000
+++ src/apiclient/tests/test_maas_client.py	2012-10-08 14:57:19 +0000
@@ -12,6 +12,7 @@
 __metaclass__ = type
 __all__ = []
 
+import json
 from random import randint
 from urlparse import (
     parse_qs,
@@ -24,7 +25,10 @@
     MAASDispatcher,
     MAASOAuth,
     )
-from apiclient.testing.django import parse_headers_and_body_with_django
+from apiclient.testing.django import (
+    parse_headers_and_body_with_django,
+    parse_headers_and_body_with_mimer,
+    )
 from maastesting.factory import factory
 from maastesting.testcase import TestCase
 
@@ -146,6 +150,16 @@
         self.assertEqual(
             {name: [value] for name, value in params.items()}, post)
 
+    def test_formulate_change_as_json(self):
+        params = {factory.getRandomString(): factory.getRandomString()}
+        url, headers, body = make_client()._formulate_change(
+            make_path(), params, as_json=True)
+        self.assertEqual('application/json', headers.get('Content-Type'))
+        self.assertEqual(len(body), headers.get('Content-Length'))
+        self.assertEqual(params, json.loads(body))
+        data = parse_headers_and_body_with_mimer(headers, body)
+        self.assertEqual(params, data)
+
     def test_get_dispatches_to_resource(self):
         path = make_path()
         client = make_client()
@@ -205,6 +219,21 @@
         self.assertTrue(request["request_url"].endswith('?op=%s' % (method,)))
         self.assertEqual({"parameter": [param]}, post)
 
+    def test_post_as_json(self):
+        param = factory.getRandomString()
+        method = factory.getRandomString()
+        list_param = [factory.getRandomString() for i in range(10)]
+        client = make_client()
+        client.post(make_path(), method, as_json=True,
+                    param=param, list_param=list_param)
+        request = client.dispatcher.last_call
+        self.assertEqual('application/json',
+                         request['headers'].get('Content-Type'))
+        content = parse_headers_and_body_with_mimer(
+            request['headers'], request['data'])
+        self.assertTrue(request["request_url"].endswith('?op=%s' % (method,)))
+        self.assertEqual({'param': param, 'list_param': list_param}, content)
+
     def test_put_dispatches_to_resource(self):
         path = make_path()
         client = make_client()

=== modified file 'src/maasserver/api.py'
--- src/maasserver/api.py	2012-10-08 11:46:58 +0000
+++ src/maasserver/api.py	2012-10-08 14:57:19 +0000
@@ -1197,7 +1197,12 @@
         to be stored in the cluster controllers (nodegroup) instead of the
         master controller.
         """
-        system_ids = request.POST.getlist('system_ids')
+        # If sent as x-www-form-urlencoded data, it comes in as a MultiDict, if
+        # sent as json it comes in as a simple dict(list).
+        if getattr(request.data, 'getlist', None) is not None:
+            system_ids = request.data.getlist('system_ids')
+        else:
+            system_ids = request.data['system_ids']
         nodegroup = get_object_or_404(NodeGroup, uuid=uuid)
         check_nodegroup_access(request, nodegroup)
         nodes = Node.objects.filter(
@@ -1409,7 +1414,12 @@
         return Tag.objects.get_nodes(name, user=request.user)
 
     def _get_nodes_for(self, request, param, nodegroup):
-        system_ids = get_optional_list(request.POST, param)
+        # The application/x-www-form-urlencoded handler uses a MultiDict, but
+        # the application/json handler uses just a dict(key=[list])
+        if getattr(request.data, 'getlist', None) is not None:
+            system_ids = request.data.getlist(param)
+        else:
+            system_ids = request.data[param]
         if system_ids:
             nodes = Node.objects.filter(system_id__in=system_ids)
             if nodegroup is not None:
@@ -1441,7 +1451,7 @@
         tag = Tag.objects.get_tag_or_404(name=name, user=request.user)
         nodegroup = None
         if not request.user.is_superuser:
-            uuid = request.POST.get('nodegroup', None)
+            uuid = request.data.get('nodegroup', None)
             if uuid is None:
                 raise PermissionDenied(
                     'Must be a superuser or supply a nodegroup')