Merge lp:~allenap/maas/cli-upload-files--bug-1187826 into lp:~maas-committers/maas/trunk

[Gavin Panella]

Allow uploading of files from the command-line using a new parameter@=filename syntax.

For example, this enables creating commissioning scripts from the command-line API client.

The new syntax, parameter@=filename, takes inspiration from a number of command-line tools I've used over the years, where prefixing an argument with @ denotes that the following word is a filename. I put it to the left of the equals sign to avoid mix-ups with arguments that begin with @. The @ symbol is also not a special character to (most|all) shells, so it's safe to leave unquoted for convenience; the use of < for example would have always required quoting.

[Gavin Panella (allenap)]

To try it out:

  bin/maas foo files add filename=foobar file@=setup.py
  bin/maas foo files get filename=foobar

[Gavin Panella (allenap)]

Or:

  bin/maas foo commissioning-scripts create name=foobar content@=setup.py
  bin/maas foo commissioning-script read foobar

[Jeroen T. Vermeulen (jtv)]

Thanks. This is something we've been hoping somebody else would do for so long!

.

The comment in test_encode_multipart_data_multiple_params made me blink a few times. Maybe I just lack context. What do you mean by "callables are called then used as context managers"? Are the return values used as context managers? The passive voice tends to obscure things: who calls the callables?

Also, try reading that first sentence in the comment out loud — whether the original version or your new version. If you got the emphasis right, I think that's because you already knew what the comment meant before you started. The way it's phrased seems to say: if you have sequences of parameters or files, you can pass those to encode_multipart_data() as opposed to somewhere else.

What I think the text actually means (but without much confidence based on the text, which is my point) is: encode_multipart_data() accepts files and regular parameters in the form of sequences, so that you can pass multiple values for the same parameter. But the sentence uses the passive voice *twice*, so as a reader I'm left with two who-does-what unknowns in the equation. Adding unknowns makes equations harder to solve.

.

I am similarly befuddled by maybe_file(). It might help if the name started with a verb, although the docstring suggests that that verb would be “check” — when clearly that's not what it does.

Having said that, of course, I have probably obligated myself to come up with something better. Here's a shot, with a name that I hope fits in well with ‘prepare_payload’:

    @staticmethod
    def prepare_parameter(name, value):
        """Return parameter in a form usable by `build_multipart_message`.

        Returns a (`name`, `value`) tuple. For file uploads, the returned
        `value` is a callable that returns a `file` object for the upload.
        Otherwise, the returned `value` is simply the value that was passed
        in.

        File uploads are distinguished by an ampersand (`@`) suffix to the
        parameter `name`::

            parameter@=filename

        In that case, the `value` parameter is the file's
        path. The returned `name` will not have the ampersand.
        """

This version also hints at _why_ a file is returned as a callable: because that's how build_multipart_message likes it.

.

In test_files_are_included, I would suggest using factory.make_name('param') instead of factory.getRandomString() for generating the ‘parameter’ value.

.

In that same test, I'm not very comfortable with expected_body_template. Is the ordering of those lines actually defined? It'd be better if we could either:

(a) parse the MIME message back and see that we get the expected result; or
(b) excise the relevant parts and use Contains matchers.

Jeroen

review: Approve

[Raphaël Badin (rvb)]

> Thanks. This is something we've been hoping somebody else would do for so long!

Oh yes! This is probably worth backporting to 1.5 since the bug prevents using the CLI for something as important as uploading custom commissioning scripts.

[Gavin Panella (allenap)]

> Thanks.  This is something we've been hoping somebody else would do for so
> long!
>
> .
>
> The comment in test_encode_multipart_data_multiple_params made me blink a few
> times.  Maybe I just lack context.  What do you mean by "callables are called
> then used as context managers"?  Are the return values used as context
> managers?  The passive voice tends to obscure things: who calls the callables?
>
> Also, try reading that first sentence in the comment out loud — whether the
> original version or your new version.  If you got the emphasis right, I think
> that's because you already knew what the comment meant before you started.
> The way it's phrased seems to say: if you have sequences of parameters or
> files, you can pass those to encode_multipart_data() as opposed to somewhere
> else.
>
> What I think the text actually means (but without much confidence based on the
> text, which is my point) is: encode_multipart_data() accepts files and regular
> parameters in the form of sequences, so that you can pass multiple values for
> the same parameter.  But the sentence uses the passive voice *twice*, so as a
> reader I'm left with two who-does-what unknowns in the equation.  Adding
> unknowns makes equations harder to solve.

I've done what I can to improve it, largely reducing the use of the
passive voice.

>
> .
>
> I am similarly befuddled by maybe_file().  It might help if the name started
> with a verb, although the docstring suggests that that verb would be “check” —
> when clearly that's not what it does.
>
> Having said that, of course, I have probably obligated myself to come up with
> something better.  Here's a shot, with a name that I hope fits in well with
> ‘prepare_payload’:
>
>     @staticmethod
>     def prepare_parameter(name, value):
>         """Return parameter in a form usable by `build_multipart_message`.
>
>         Returns a (`name`, `value`) tuple.  For file uploads, the returned
>         `value` is a callable that returns a `file` object for the upload.
>         Otherwise, the returned `value` is simply the value that was passed
>         in.
>
>         File uploads are distinguished by an ampersand (`@`) suffix to the
>         parameter `name`::
>
>             parameter@=filename
>
>         In that case, the `value` parameter is the file's
>         path.  The returned `name` will not have the ampersand.
>         """
>
> This version also hints at _why_ a file is returned as a callable: because
> that's how build_multipart_message likes it.

I have changed how this works. It's broadly similar to what you've
written, but happens at a different point. maybe_file() is gone.

>
> .
>
> In test_files_are_included, I would suggest using factory.make_name('param')
> instead of factory.getRandomString() for generating the ‘parameter’ value.

Done.

>
> .
>
> In that same test, I'm not very comfortable with expected_body_template.  Is
> the ordering of those lines actually defined?  It'd be better if we could
> either:
>
> (a) parse the MIME message back and see that we get the expected result; or
> (b) excise the relevant parts and use Contains matchers.

Messages headers are stored in order, so this'll be fine...

=== modified file 'src/apiclient/multipart.py'
--- src/apiclient/multipart.py	2013-10-07 09:12:40 +0000
+++ src/apiclient/multipart.py	2014-04-03 19:58:37 +0000
@@ -65,11 +65,31 @@
 
 
 def make_payloads(name, content):
+    """Constructs payload(s) for the given `name` and `content`.
+
+    If `content` is a byte string, this calls `make_bytes_payload` to
+    construct the payload, which this then yields.
+
+    If `content` is a unicode string, this calls `make_string_payload`.
+
+    If `content` is file-like -- it inherits from `IOBase` or `file` --
+    this calls `make_file_payload`.
+
+    If `content` is iterable, this calls `make_payloads` for each item,
+    with the same name, and then re-yields each payload generated.
+
+    If `content` is callable, this calls it with no arguments, and then
+    uses the result as a context manager. This can be useful if the
+    callable returns an open file, for example, because the context
+    protocol means it will be closed after use.
+
+    This raises `AssertionError` if it encounters anything else.
+    """
     if isinstance(content, bytes):
         yield make_bytes_payload(name, content)
     elif isinstance(content, unicode):
         yield make_string_payload(name, content)
-    elif isinstance(content, IOBase):
+    elif isinstance(content, (IOBase, file)):
         yield make_file_payload(name, content)
     elif callable(content):
         with content() as content:
 
=== modified file 'src/apiclient/tests/test_multipart.py'
--- src/apiclient/tests/test_multipart.py	2013-10-18 16:57:37 +0000
+++ src/apiclient/tests/test_multipart.py	2014-04-03 19:58:37 +0000
@@ -85,18 +85,20 @@
             ahem_django_ahem)
 
     def test_encode_multipart_data_multiple_params(self):
-        # Sequences of parameters and files can be passed to
-        # encode_multipart_data() so that multiple parameters/files with the
-        # same name can be provided.
+        # Sequences of parameters and files passed to
+        # encode_multipart_data() permit use of the same name for
+        # multiple parameters and/or files. See `make_payloads` to
+        # understand how it processes different types of parameter
+        # values.
         params_in = [
             ("one", "ABC"),
             ("one", "XYZ"),
-            ("two", "DEF"),
-            ("two", "UVW"),
+            ("two", ["DEF", "UVW"]),
             ]
         files_in = [
-            ("f-one", BytesIO(urandom(32))),
-            ("f-two", BytesIO(urandom(32))),
+            ("f-one", BytesIO(b"f1")),
+            ("f-two", open(self.make_file(contents=b"f2"), "rb")),
+            ("f-three", lambda: open(self.make_file(contents=b"f3"), "rb")),
             ]
         body, headers = encode_multipart_data(params_in, files_in)
         self.assertEqual("%s" % len(body), headers["Content-Length"])
@@ -107,13 +109,14 @@
         params_out, files_out = (
             parse_headers_and_body_with_django(headers, body))
         params_out_expected = MultiValueDict()
-        for name, value in params_in:
-            params_out_expected.appendlist(name, value)
+        params_out_expected.appendlist("one", "ABC")
+        params_out_expected.appendlist("one", "XYZ")
+        params_out_expected.appendlist("two", "DEF")
+        params_out_expected.appendlist("two", "UVW")
         self.assertEqual(
             params_out_expected, params_out,
             ahem_django_ahem)
-        self.assertSetEqual({"f-one", "f-two"}, set(files_out))
-        files_expected = {name: buf.getvalue() for name, buf in files_in}
+        files_expected = {"f-one": b"f1", "f-two": b"f2", "f-three": b"f3"}
         files_observed = {name: buf.read() for name, buf in files_out.items()}
         self.assertEqual(
             files_expected, files_observed,
 
=== modified file 'src/maascli/api.py'
--- src/maascli/api.py	2014-02-27 13:22:53 +0000
+++ src/maascli/api.py	2014-04-03 19:58:37 +0000
@@ -19,10 +19,12 @@
 import argparse
 from collections import defaultdict
 from email.message import Message
+from functools import partial
 import httplib
 from itertools import chain
 import json
 from operator import itemgetter
+import re
 import sys
 from textwrap import (
     dedent,
@@ -34,7 +36,10 @@
     )
 
 from apiclient.maas_client import MAASOAuth
-from apiclient.multipart import encode_multipart_data
+from apiclient.multipart import (
+    build_multipart_message,
+    encode_multipart_message,
+    )
 from apiclient.utils import (
     ascii_url,
     urlencode,
@@ -166,6 +171,10 @@
         uri, body, headers = self.prepare_payload(
             self.op, self.method, uri, options.data)
 
+        # Headers are returned as a list, but they must be a dict for
+        # the signing machinery.
+        headers = dict(headers)
+
         # Sign request if credentials have been provided.
         if self.credentials is not None:
             self.sign(uri, headers, self.credentials)
@@ -190,15 +199,32 @@
 
     @staticmethod
     def name_value_pair(string):
-        parts = string.split("=", 1)
-        if len(parts) == 2:
-            return tuple(parts)
+        """Ensure that `string` is a valid ``name:value`` pair.
+
+        When `string` is of the form ``name=value``, this returns a
+        2-tuple of ``name, value``.
+
+        However, when `string` is of the form ``name@=value``, this
+        returns a ``name, opener`` tuple, where ``opener`` is a function
+        that will return an open file handle when called. The file will
+        be opened in binary mode for reading only.
+        """
+        parts = re.split(r'(=|@=)', string, 1)
+        if len(parts) == 3:
+            name, what, value = parts
+            if what == "=":
+                return name, value
+            elif what == "@=":
+                return name, partial(open, value, "rb")
+            else:
+                raise AssertionError(
+                    "Unrecognised separator %r" % what)
         else:
             raise CommandError(
-                "%r is not a name=value pair" % string)
+                "%r is not a name=value or name@=filename pair" % string)
 
-    @staticmethod
-    def prepare_payload(op, method, uri, data):
+    @classmethod
+    def prepare_payload(cls, op, method, uri, data):
         """Return the URI (modified perhaps) and body and headers.
 
         - For GET requests, encode parameters in the query string.
@@ -209,18 +235,27 @@
 
         :param method: The HTTP method.
         :param uri: The URI of the action.
-        :param data: A dict or iterable of name=value pairs to pack into the
-            body or headers, depending on the type of request.
+        :param data: An iterable of ``name, value`` or ``name, opener``
+            tuples (see `name_value_pair`) to pack into the body or
+            query, depending on the type of request.
         """
+        query = [] if op is None else [("op", op)]
+
+        def slurp(opener):
+            with opener() as fd:
+                return fd.read()
+
         if method == "GET":
-            query = data if op is None else chain([("op", op)], data)
-            body, headers = None, {}
+            query.extend(
+                (name, slurp(value) if callable(value) else value)
+                for name, value in data)
+            body, headers = None, []
         else:
-            query = [] if op is None else [("op", op)]
-            if data:
-                body, headers = encode_multipart_data(data)
+            if data is None or len(data) == 0:
+                body, headers = None, []
             else:
-                body, headers = None, {}
+                message = build_multipart_message(data)
+                headers, body = encode_multipart_message(message)
 
         uri = urlparse(uri)._replace(query=urlencode(query)).geturl()
         return uri, body, headers
 
=== modified file 'src/maascli/tests/test_api.py'
--- src/maascli/tests/test_api.py	2014-02-27 13:22:53 +0000
+++ src/maascli/tests/test_api.py	2014-04-03 19:58:37 +0000
@@ -15,6 +15,7 @@
 __all__ = []
 
 import collections
+from functools import partial
 import httplib
 import io
 import json
@@ -515,25 +516,26 @@
          {"method": "POST", "data": [],
           "expected_uri": uri_base,
           "expected_body": None,
-          "expected_headers": {}}),
+          "expected_headers": []}),
         ("read",
          {"method": "GET", "data": [],
           "expected_uri": uri_base,
           "expected_body": None,
-          "expected_headers": {}}),
+          "expected_headers": []}),
         ("update",
          {"method": "PUT", "data": [],
           "expected_uri": uri_base,
           "expected_body": None,
-          "expected_headers": {}}),
+          "expected_headers": []}),
         ("delete",
          {"method": "DELETE", "data": [],
           "expected_uri": uri_base,
           "expected_body": None,
-          "expected_headers": {}}),
-        # With data, PUT, POST, and DELETE requests have their body and extra
-        # headers prepared by encode_multipart_data. For GET requests, the
-        # data is encoded into the query string, and both the request body and
+          "expected_headers": []}),
+        # With data, PUT, POST, and DELETE requests have their body and
+        # extra headers prepared by build_multipart_message and
+        # encode_multipart_message. For GET requests, the data is
+        # encoded into the query string, and both the request body and
         # extra headers are empty.
         ("create-with-data",
          {"method": "POST", "data": [("foo", "bar"), ("foo", "baz")],
@@ -544,7 +546,7 @@
          {"method": "GET", "data": [("foo", "bar"), ("foo", "baz")],
           "expected_uri": uri_base + "?foo=bar&foo=baz",
           "expected_body": None,
-          "expected_headers": {}}),
+          "expected_headers": []}),
         ("update-with-data",
          {"method": "PUT", "data": [("foo", "bar"), ("foo", "baz")],
           "expected_uri": uri_base,
@@ -565,27 +567,28 @@
          {"method": "POST", "data": [],
           "expected_uri": uri_base + "?op=something",
           "expected_body": None,
-          "expected_headers": {}}),
+          "expected_headers": []}),
         ("read",
          {"method": "GET", "data": [],
           "expected_uri": uri_base + "?op=something",
           "expected_body": None,
-          "expected_headers": {}}),
+          "expected_headers": []}),
         ("update",
          {"method": "PUT", "data": [],
           "expected_uri": uri_base + "?op=something",
           "expected_body": None,
-          "expected_headers": {}}),
+          "expected_headers": []}),
         ("delete",
          {"method": "DELETE", "data": [],
           "expected_uri": uri_base + "?op=something",
           "expected_body": None,
-          "expected_headers": {}}),
-        # With data, PUT, POST, and DELETE requests have their body and extra
-        # headers prepared by encode_multipart_data. For GET requests, the
-        # data is encoded into the query string, and both the request body and
-        # extra headers are empty. The operation is encoded into the query
-        # string.
+          "expected_headers": []}),
+        # With data, PUT, POST, and DELETE requests have their body and
+        # extra headers prepared by build_multipart_message and
+        # encode_multipart_message. For GET requests, the data is
+        # encoded into the query string, and both the request body and
+        # extra headers are empty. The operation is encoded into the
+        # query string.
         ("create-with-data",
          {"method": "POST", "data": [("foo", "bar"), ("foo", "baz")],
           "expected_uri": uri_base + "?op=something",
@@ -595,7 +598,7 @@
          {"method": "GET", "data": [("foo", "bar"), ("foo", "baz")],
           "expected_uri": uri_base + "?op=something&foo=bar&foo=baz",
           "expected_body": None,
-          "expected_headers": {}}),
+          "expected_headers": []}),
         ("update-with-data",
          {"method": "PUT", "data": [("foo", "bar"), ("foo", "baz")],
           "expected_uri": uri_base + "?op=something",
@@ -619,9 +622,12 @@
     scenarios = scenarios_without_op + scenarios_with_op
 
     def test_prepare_payload(self):
-        # Patch encode_multipart_data to match the scenarios.
-        encode_multipart_data = self.patch(api, "encode_multipart_data")
-        encode_multipart_data.return_value = sentinel.body, sentinel.headers
+        # Patch build_multipart_message and encode_multipart_message to
+        # match the scenarios.
+        build_multipart_message = self.patch(api, "build_multipart_message")
+        build_multipart_message.return_value = sentinel.message
+        encode_multipart_message = self.patch(api, "encode_multipart_message")
+        encode_multipart_message.return_value = sentinel.headers, sentinel.body
         # The payload returned is a 3-tuple of (uri, body, headers).
         payload = api.Action.prepare_payload(
             op=self.op, method=self.method,
@@ -632,8 +638,43 @@
             Equals(self.expected_headers),
             )
         self.assertThat(payload, MatchesListwise(expected))
-        # encode_multipart_data, when called, is passed the data
+        # encode_multipart_message, when called, is passed the data
         # unadulterated.
         if self.expected_body is sentinel.body:
             self.assertThat(
-                api.encode_multipart_data, MockCalledOnceWith(self.data))
+                api.build_multipart_message,
+                MockCalledOnceWith(self.data))
+            self.assertThat(
+                api.encode_multipart_message,
+                MockCalledOnceWith(sentinel.message))
+
+
+class TestPayloadPreparationWithFiles(MAASTestCase):
+    """Tests for `maascli.api.Action.prepare_payload` involving files."""
+
+    def test_files_are_included(self):
+        parameter = factory.make_name("param")
+        contents = factory.getRandomBytes()
+        filename = self.make_file(contents=contents)
+        # Writing the parameter as "parameter@=filename" on the
+        # command-line causes name_value_pair() to return a `name,
+        # opener` tuple, where `opener` is a callable that returns an
+        # open file handle.
+        data = [(parameter, partial(open, filename, "rb"))]
+        uri, body, headers = api.Action.prepare_payload(
+            op=None, method="POST", uri="http://localhost", data=data)
+
+        expected_body_template = """\
+            --...
+            Content-Transfer-Encoding: base64
+            Content-Disposition: form-data; name="%s"; filename="%s"
+            MIME-Version: 1.0
+            Content-Type: application/octet-stream
+
+            %s
+            --...--
+            """
+        expected_body = expected_body_template % (
+            parameter, parameter, contents.encode("base64"))
+
+        self.assertDocTestMatches(expected_body, body)