Code review comment for lp:~jameinel/maas/allow-json

Revision history for this message
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

« Back to merge proposal