launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/apiclient-multipart into lp:maas.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~jtv/maas/apiclient-multipart/+merge/107046

This continues work done by Julian: a helper to write multipart forms, as needed for submitting POST/PUT requests to the Piston APIs.

The code originated in Juju's MAAS provider, but now we need it in MAAS itself as well so that the provisioning workers can give feedback to the API.  The obvious next step is to extract it into a stand-alone library.

A very noticeable difference with the Juju version is that in MAAS coding standards, string constants are unicode, not str (or bytes, in more modern parlance).  We generate MIME as raw bytes, and that becomes a very explicit thing here.

The old code was not tested in great detail.  For example the boundary strings (what those are will become clear as you read the diff) were one byte longer than requested for some reason.  Fixing that in turn broke the integration test, which hard-coded the length of an extensive stretch of text.  Luckily we don't have any need for non-ASCII text in the API yet; at some point we'll have to become a lot more careful.

A later branch builds an HTTP API client using this MIME helper.  For now, it just sits there uncalled.


Jeroen
-- 
https://code.launchpad.net/~jtv/maas/apiclient-multipart/+merge/107046
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/maas/apiclient-multipart into lp:maas.

=== added directory 'src/apiclient'
=== added file 'src/apiclient/__init__.py'
=== added file 'src/apiclient/multipart.py'
--- src/apiclient/multipart.py	1970-01-01 00:00:00 +0000
+++ src/apiclient/multipart.py	2012-05-23 15:31:50 +0000
@@ -0,0 +1,82 @@
+# Copyright 2012 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Encoding of MIME multipart data."""
+
+from __future__ import (
+    absolute_import,
+    print_function,
+    unicode_literals,
+    )
+
+__metaclass__ = type
+__all__ = [
+    'encode_multipart_data',
+    ]
+
+import mimetypes
+import random
+import string
+
+
+def make_random_boundary(length=30):
+    """Create a random string for use in MIME boundary lines."""
+    return b''.join(random.choice(string.letters) for ii in range(length))
+
+
+def get_content_type(filename):
+    """Return the MIME content type for the file with the given name."""
+    return mimetypes.guess_type(filename)[0] or b'application/octet-stream'
+
+
+def encode_field(field_name, data, boundary):
+    """MIME-encode a form field."""
+    field_name = field_name.encode('ascii')
+    return (
+        b'--' + boundary,
+        b'Content-Disposition: form-data; name="%s"' % field_name,
+        b'',
+        bytes(data),
+        )
+
+
+def encode_file(name, fileObj, boundary):
+    """MIME-encode a file upload."""
+    content_type = get_content_type(name)
+    name = name.encode('ascii')
+    return (
+        b'--' + boundary,
+        b'Content-Disposition: form-data; name="%s"; filename="%s"' %
+            (name, name),
+        b'Content-Type: %s' % content_type,
+        b'',
+        fileObj.read(),
+        )
+
+
+def encode_multipart_data(data, files):
+    """Create a MIME multipart payload from L{data} and L{files}.
+
+    @param data: A mapping of names (ASCII strings) to data (byte string).
+    @param files: A mapping of names (ASCII strings) to file objects ready to
+        be read.
+    @return: A 2-tuple of C{(body, headers)}, where C{body} is a a byte string
+        and C{headers} is a dict of headers to add to the enclosing request in
+        which this payload will travel.
+    """
+    boundary = make_random_boundary()
+
+    lines = []
+    for name, content in data.items():
+        lines.extend(encode_field(name, content, boundary))
+    for name, file_obj in files.items():
+        lines.extend(encode_file(name, file_obj, boundary))
+    lines.extend((b'--%s--' % boundary, b''))
+    body = b'\r\n'.join(lines)
+
+    headers = {
+        b'content-type': b'multipart/form-data; boundary=' + boundary,
+        b'content-length': b'%s' % (len(body)),
+        }
+
+    return body, headers

=== added directory 'src/apiclient/tests'
=== added file 'src/apiclient/tests/test_multipart.py'
--- src/apiclient/tests/test_multipart.py	1970-01-01 00:00:00 +0000
+++ src/apiclient/tests/test_multipart.py	2012-05-23 15:31:50 +0000
@@ -0,0 +1,137 @@
+# Copyright 2012 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Test multipart MIME helpers."""
+
+from __future__ import (
+    absolute_import,
+    print_function,
+    unicode_literals,
+    )
+
+__metaclass__ = type
+__all__ = []
+
+from io import BytesIO
+from random import randint
+import re
+from textwrap import dedent
+
+from apiclient.multipart import (
+    encode_field,
+    encode_file,
+    encode_multipart_data,
+    get_content_type,
+    make_random_boundary,
+    )
+from maastesting.factory import factory
+from maastesting.testcase import TestCase
+from testtools.matchers import EndsWith
+
+
+class TestMultiPart(TestCase):
+
+    def test_make_random_boundary_produces_bytes(self):
+        self.assertIsInstance(make_random_boundary(), bytes)
+
+    def test_make_random_boundary_produces_different_strings(self):
+        self.assertNotEqual(
+            make_random_boundary(),
+            make_random_boundary())
+
+    def test_make_random_boundary_obeys_length(self):
+        length = randint(5, 100)
+        self.assertEqual(length, len(make_random_boundary(length)))
+
+    def test_make_random_boundary_uses_no_weird_characters(self):
+        boundary = make_random_boundary(1000)
+        self.assertTrue(boundary.isalnum())
+        self.assertEqual([boundary], boundary.split())
+        self.assertNotIn('-', boundary)
+
+    def test_get_content_type_guesses_type(self):
+        guess = get_content_type('text.txt')
+        self.assertEqual('text/plain', guess)
+        self.assertIsInstance(guess, bytes)
+
+    def test_get_content_type_defaults_to_raw_bytes(self):
+        guess = get_content_type('mysterious-data')
+        self.assertEqual('application/octet-stream', guess)
+        self.assertIsInstance(guess, bytes)
+
+    def test_encode_field_encodes_form_field_as_sequence_of_byteses(self):
+        name = factory.getRandomString()
+        data = factory.getRandomString().encode('ascii')
+        boundary = make_random_boundary(5)
+        encoded_field = encode_field(name, data, boundary)
+        self.assertIn(b'--' + boundary, encoded_field)
+        self.assertIn(data, encoded_field)
+        text = b'\n'.join(encoded_field)
+        self.assertIn(b'name="%s"' % name, text)
+        self.assertIsInstance(text, bytes)
+
+    def test_encode_file_encodes_file_as_sequence_of_byteses(self):
+        name = factory.getRandomString()
+        data = factory.getRandomString().encode('ascii')
+        boundary = make_random_boundary(5)
+        encoded_file = encode_file(name, BytesIO(data), boundary)
+        self.assertIn(b'--' + boundary, encoded_file)
+        self.assertIn(data, encoded_file)
+        text = b'\n'.join(encoded_file)
+        self.assertIn(b'name="%s"' % name, text)
+        self.assertIsInstance(text, bytes)
+
+    def test_encode_multipart_data_produces_bytes(self):
+        data = {
+            factory.getRandomString():
+                factory.getRandomString().encode('ascii'),
+        }
+        files = {
+            factory.getRandomString():
+                BytesIO(factory.getRandomString().encode('ascii')),
+            }
+        body, headers = encode_multipart_data(data, files)
+        self.assertIsInstance(body, bytes)
+
+    def test_encode_multipart_data_closes_with_closing_boundary_line(self):
+        data = {b'foo': factory.getRandomString().encode('ascii')}
+        files = {b'bar': BytesIO(factory.getRandomString().encode('ascii'))}
+        body, headers = encode_multipart_data(data, files)
+        self.assertThat(body, EndsWith(b'--\r\n'))
+
+    def test_encode_multipart_data(self):
+        # The encode_multipart_data() function should take a list of
+        # parameters and files and encode them into a MIME
+        # multipart/form-data suitable for posting to the MAAS server.
+        params = {"op": "add", "filename": "foo"}
+        fileObj = BytesIO(b"random data")
+        files = {"file": fileObj}
+        body, headers = encode_multipart_data(params, files)
+
+        expected_body_regex = b"""\
+            --(?P<boundary>.+)
+            Content-Disposition: form-data; name="filename"
+
+            foo
+            --(?P=boundary)
+            Content-Disposition: form-data; name="op"
+
+            add
+            --(?P=boundary)
+            Content-Disposition: form-data; name="file"; filename="file"
+            Content-Type: application/octet-stream
+
+            random data
+            --(?P=boundary)--
+            """
+        expected_body_regex = dedent(expected_body_regex)
+        expected_body_regex = b"\r\n".join(expected_body_regex.splitlines())
+        expected_body = re.compile(expected_body_regex, re.MULTILINE)
+        self.assertRegexpMatches(body, expected_body)
+
+        boundary = expected_body.match(body).group("boundary")
+        expected_headers = {
+            b"content-length": str(len(body)),
+            b"content-type": b"multipart/form-data; boundary=%s" % boundary,
+            }
+        self.assertEqual(expected_headers, headers)