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

Proposed by John A Meinel on 2012-10-08
Status: Merged
Merged at revision: 1230
Proposed branch: lp:~jameinel/maas/allow-json
Merge into: lp:maas/trunk
Diff against target: 245 lines (+138/-5)
5 files modified
src/apiclient/encode_json.py (+32/-0)
src/apiclient/maas_client.py (+14/-4)
src/apiclient/testing/django.py (+26/-0)
src/apiclient/tests/test_encode_json.py (+36/-0)
src/apiclient/tests/test_maas_client.py (+30/-1)
To merge this branch: bzr merge lp:~jameinel/maas/allow-json
Reviewer Review Type Date Requested Status
John A Meinel Approve on 2012-10-09
Gavin Panella (community) 2012-10-08 Approve on 2012-10-08
Review via email: mp+128512@code.launchpad.net

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.

Description of the change

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.

To post a comment you must log in.
John A Meinel (jameinel) wrote :

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) wrote :

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) wrote :

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) wrote :

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) wrote :

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.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== added file 'src/apiclient/encode_json.py'
2--- src/apiclient/encode_json.py 1970-01-01 00:00:00 +0000
3+++ src/apiclient/encode_json.py 2012-10-08 15:04:22 +0000
4@@ -0,0 +1,32 @@
5+# Copyright 2012 Canonical Ltd. This software is licensed under the
6+# GNU Affero General Public License version 3 (see the file LICENSE).
7+
8+"""Encoding requests as JSON data."""
9+
10+from __future__ import (
11+ absolute_import,
12+ print_function,
13+ unicode_literals,
14+ )
15+
16+__metaclass__ = type
17+__all__ = [
18+ 'encode_json_data',
19+ ]
20+
21+import json
22+
23+
24+def encode_json_data(params):
25+ """Encode params as JSON and set up headers for an HTTP POST.
26+
27+ :param params: Can be a dict or a list, but should generally be a dict, to
28+ match the key-value data expected by most receiving APIs.
29+ :return: (body, headers)
30+ """
31+ body = json.dumps(params)
32+ headers = {
33+ 'Content-Length': len(body),
34+ 'Content-Type': 'application/json',
35+ }
36+ return body, headers
37
38=== modified file 'src/apiclient/maas_client.py'
39--- src/apiclient/maas_client.py 2012-10-05 07:38:54 +0000
40+++ src/apiclient/maas_client.py 2012-10-08 15:04:22 +0000
41@@ -19,6 +19,7 @@
42 from urllib import urlencode
43 import urllib2
44
45+from apiclient.encode_json import encode_json_data
46 from apiclient.multipart import encode_multipart_data
47 import oauth.oauth as oauth
48
49@@ -138,7 +139,7 @@
50 self.auth.sign_request(url, headers)
51 return url, headers
52
53- def _formulate_change(self, path, params):
54+ def _formulate_change(self, path, params, as_json=False):
55 """Return URL, headers, and body for a non-GET request.
56
57 This is similar to _formulate_get, except parameters are encoded as
58@@ -146,6 +147,9 @@
59
60 :param path: Path to the object to issue a GET on.
61 :param params: A dict of parameter values.
62+ :param as_json: Encode params as application/json instead of
63+ application/x-www-form-urlencoded. Only use this if you know the
64+ API already supports JSON requests.
65 :return: A tuple: URL, headers, and body for the request.
66 """
67 url = self._make_url(path)
68@@ -153,7 +157,10 @@
69 params = dict(params)
70 op = params.pop('op')
71 url += '?' + urlencode({'op': op})
72- body, headers = encode_multipart_data(params, {})
73+ if as_json:
74+ body, headers = encode_json_data(params)
75+ else:
76+ body, headers = encode_multipart_data(params, {})
77 self.auth.sign_request(url, headers)
78 return url, headers, body
79
80@@ -170,13 +177,16 @@
81 return self.dispatcher.dispatch_query(
82 url, method="GET", headers=headers)
83
84- def post(self, path, op, **kwargs):
85+ def post(self, path, op, as_json=False, **kwargs):
86 """Dispatch POST method `op` on `path`, with the given parameters.
87
88+ :param as_json: Instead of POSTing the content as multipart
89+ x-www-form-urlencoded post it as application/json
90 :return: The result of the dispatch_query call on the dispatcher.
91 """
92 kwargs['op'] = op
93- url, headers, body = self._formulate_change(path, kwargs)
94+ url, headers, body = self._formulate_change(
95+ path, kwargs, as_json=as_json)
96 return self.dispatcher.dispatch_query(
97 url, method="POST", headers=headers, data=body)
98
99
100=== modified file 'src/apiclient/testing/django.py'
101--- src/apiclient/testing/django.py 2012-09-22 19:35:59 +0000
102+++ src/apiclient/testing/django.py 2012-10-08 15:04:22 +0000
103@@ -14,8 +14,17 @@
104
105 from io import BytesIO
106
107+from django.core.handlers.wsgi import WSGIRequest
108 from django.core.files.uploadhandler import MemoryFileUploadHandler
109 from django.http.multipartparser import MultiPartParser
110+from piston import emitters
111+from piston.utils import translate_mime
112+from maasserver.utils import ignore_unused
113+
114+
115+# Importing emitters has a side effect of registering mime type handlers with
116+# utils.translate_mime. So we must import it, even though we don't use it.
117+ignore_unused(emitters)
118
119
120 def parse_headers_and_body_with_django(headers, body):
121@@ -46,3 +55,20 @@
122 META=meta, input_data=BytesIO(body),
123 upload_handlers=[handler])
124 return parser.parse()
125+
126+
127+def parse_headers_and_body_with_mimer(headers, body):
128+ """Use piston's Mimer functionality to handle the content.
129+
130+ :return: The value of 'request.data' after using Piston's translate_mime on
131+ the input.
132+ """
133+ environ = {'wsgi.input': BytesIO(body)}
134+ for name, value in headers.items():
135+ environ[name.upper().replace('-', '_')] = value
136+ environ['REQUEST_METHOD'] = 'POST'
137+ environ['SCRIPT_NAME'] = ''
138+ environ['PATH_INFO'] = ''
139+ request = WSGIRequest(environ)
140+ translate_mime(request)
141+ return request.data
142
143=== added file 'src/apiclient/tests/test_encode_json.py'
144--- src/apiclient/tests/test_encode_json.py 1970-01-01 00:00:00 +0000
145+++ src/apiclient/tests/test_encode_json.py 2012-10-08 15:04:22 +0000
146@@ -0,0 +1,36 @@
147+# Copyright 2012 Canonical Ltd. This software is licensed under the
148+# GNU Affero General Public License version 3 (see the file LICENSE).
149+
150+"""Test encoding requests as JSON."""
151+
152+from __future__ import (
153+ absolute_import,
154+ print_function,
155+ unicode_literals,
156+ )
157+
158+__metaclass__ = type
159+__all__ = []
160+
161+
162+from apiclient.encode_json import encode_json_data
163+from maastesting.testcase import TestCase
164+
165+
166+class TestEncodeJSONData(TestCase):
167+
168+ def assertEncodeJSONData(self, expected_body, expected_headers, params):
169+ self.assertEqual(
170+ (expected_body, expected_headers),
171+ encode_json_data(params))
172+
173+ def test_encode_empty_dict(self):
174+ self.assertEncodeJSONData(
175+ '{}', {'Content-Length': 2, 'Content-Type': 'application/json'},
176+ {})
177+
178+ def test_encode_dict(self):
179+ self.assertEncodeJSONData(
180+ '{"alt": [1, 2, 3, 4], "param": "value"}',
181+ {'Content-Length': 39, 'Content-Type': 'application/json'},
182+ {'param': 'value', 'alt': [1, 2, 3, 4]})
183
184=== modified file 'src/apiclient/tests/test_maas_client.py'
185--- src/apiclient/tests/test_maas_client.py 2012-10-05 07:38:54 +0000
186+++ src/apiclient/tests/test_maas_client.py 2012-10-08 15:04:22 +0000
187@@ -12,6 +12,7 @@
188 __metaclass__ = type
189 __all__ = []
190
191+import json
192 from random import randint
193 from urlparse import (
194 parse_qs,
195@@ -24,7 +25,10 @@
196 MAASDispatcher,
197 MAASOAuth,
198 )
199-from apiclient.testing.django import parse_headers_and_body_with_django
200+from apiclient.testing.django import (
201+ parse_headers_and_body_with_django,
202+ parse_headers_and_body_with_mimer,
203+ )
204 from maastesting.factory import factory
205 from maastesting.testcase import TestCase
206
207@@ -146,6 +150,16 @@
208 self.assertEqual(
209 {name: [value] for name, value in params.items()}, post)
210
211+ def test_formulate_change_as_json(self):
212+ params = {factory.getRandomString(): factory.getRandomString()}
213+ url, headers, body = make_client()._formulate_change(
214+ make_path(), params, as_json=True)
215+ self.assertEqual('application/json', headers.get('Content-Type'))
216+ self.assertEqual(len(body), headers.get('Content-Length'))
217+ self.assertEqual(params, json.loads(body))
218+ data = parse_headers_and_body_with_mimer(headers, body)
219+ self.assertEqual(params, data)
220+
221 def test_get_dispatches_to_resource(self):
222 path = make_path()
223 client = make_client()
224@@ -205,6 +219,21 @@
225 self.assertTrue(request["request_url"].endswith('?op=%s' % (method,)))
226 self.assertEqual({"parameter": [param]}, post)
227
228+ def test_post_as_json(self):
229+ param = factory.getRandomString()
230+ method = factory.getRandomString()
231+ list_param = [factory.getRandomString() for i in range(10)]
232+ client = make_client()
233+ client.post(make_path(), method, as_json=True,
234+ param=param, list_param=list_param)
235+ request = client.dispatcher.last_call
236+ self.assertEqual('application/json',
237+ request['headers'].get('Content-Type'))
238+ content = parse_headers_and_body_with_mimer(
239+ request['headers'], request['data'])
240+ self.assertTrue(request["request_url"].endswith('?op=%s' % (method,)))
241+ self.assertEqual({'param': param, 'list_param': list_param}, content)
242+
243 def test_put_dispatches_to_resource(self):
244 path = make_path()
245 client = make_client()