Merge lp:~jameinel/maas/allow-json into lp:~maas-committers/maas/trunk

[John A Meinel]

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.

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.

[John A Meinel (jameinel)]

It looks like I accidentally included the API changes necessary to support POSTing as json to the tag-related APIs. I'm updating the branch now to remove them, but that change will be present in the next branch I propose.

[Gavin Panella (allenap)]

Looks good. I think [4] needs fixing, but it's minor, so +1.

[1]

+        :param as_json: Encode params as application/json instead of
+            application/x-www-form-urlencoded. Only use this if you know the
..
+        :param as_json: Instead of POSTing the content as multipart
+            x-www-form-urlencoded post it as application/json

I think this should be multipart/form-data for all non-GET calls.

[2]

+    def assertEncodeJSONData(self, expected_body, expected_headers, params):
+        self.assertEqual(
+            (expected_body, expected_headers),
+            encode_json_data(params))

Something I used for the first time earlier today might make failures
here prettier:

        expected = Equals((expected_body, expected_headers))
        self.assertThat(
            params, AfterPreprocessing(
                encode_json_data, Equals(expected)))

Up to you though; it's a small win.

[3]

+        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)

Elsewhere in MAAS it's been useful to use testtools' compound
matchers, so that failures contain more information about their
nature. Consider something like:

        observed = (
            headers.get('Content-Type'),
            headers.get('Content-Length'),
            body,
            )
        expected = (
            'application/json',
            "%d" % len(body),
            AfterPreprocessing(json.loads, Equals(params)),
            )
        self.assertThat(observed, MatchesListwise(expected))

[4]

+        self.assertEqual(len(body), headers.get('Content-Length'))

The header should be a string value, so check that it's being
populated correctly.

review: Approve

[MAAS Lander (maas-lander)]

The Jenkins job https://jenkins.qa.ubuntu.com/job/maas-merger-quantal/170/console reported an error when processing this lp:~jameinel/maas/allow-json branch.
Not merging it.

[John A Meinel (jameinel)]

Submitting again because I think I have a fix for failing tests that should have failed in the first submission.

review: Approve

[MAAS Lander (maas-lander)]

The Jenkins job https://jenkins.qa.ubuntu.com/job/maas-merger-quantal/172/console reported an error when processing this lp:~jameinel/maas/allow-json branch.
Not merging it.

=== 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 15:04:22 +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 15:04:22 +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 15:04:22 +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 15:04:22 +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 15:04:22 +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()