Merge lp:~allenap/maas/cli-upload-files--bug-1187826 into lp:~maas-committers/maas/trunk
- cli-upload-files--bug-1187826
- Merge into trunk
Status: | Merged | ||||
---|---|---|---|---|---|
Approved by: | Gavin Panella | ||||
Approved revision: | no longer in the source branch. | ||||
Merged at revision: | 2226 | ||||
Proposed branch: | lp:~allenap/maas/cli-upload-files--bug-1187826 | ||||
Merge into: | lp:~maas-committers/maas/trunk | ||||
Diff against target: |
366 lines (+149/-50) 4 files modified
src/apiclient/multipart.py (+21/-1) src/apiclient/tests/test_multipart.py (+14/-11) src/maascli/api.py (+50/-15) src/maascli/tests/test_api.py (+64/-23) |
||||
To merge this branch: | bzr merge lp:~allenap/maas/cli-upload-files--bug-1187826 | ||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Jeroen T. Vermeulen (community) | Approve | ||
Review via email: mp+213927@code.launchpad.net |
Commit message
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.
Description of the change
The new syntax, parameter@
Gavin Panella (allenap) wrote : | # |
Gavin Panella (allenap) wrote : | # |
Or:
bin/maas foo commissioning-
bin/maas foo commissioning-
Jeroen T. Vermeulen (jtv) wrote : | # |
Thanks. This is something we've been hoping somebody else would do for so long!
.
The comment in test_encode_
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_
What I think the text actually means (but without much confidence based on the text, which is my point) is: encode_
.
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_
"""Return parameter in a form usable by `build_
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`::
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
.
In test_files_
.
In that same test, I'm not very comfortable with expected_
(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
Raphaël Badin (rvb) wrote : | # |
> 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) wrote : | # |
> Thanks. This is something we've been hoping somebody else would do for so
> long!
>
> .
>
> The comment in test_encode_
> 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_
> else.
>
> What I think the text actually means (but without much confidence based on the
> text, which is my point) is: encode_
> 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_
> """Return parameter in a form usable by `build_
>
> 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
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_
> instead of factory.
Done.
>
> .
>
> In that same test, I'm not very comfortable with expected_
> 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...
Preview Diff
1 | === modified file 'src/apiclient/multipart.py' |
2 | --- src/apiclient/multipart.py 2013-10-07 09:12:40 +0000 |
3 | +++ src/apiclient/multipart.py 2014-04-03 19:58:37 +0000 |
4 | @@ -65,11 +65,31 @@ |
5 | |
6 | |
7 | def make_payloads(name, content): |
8 | + """Constructs payload(s) for the given `name` and `content`. |
9 | + |
10 | + If `content` is a byte string, this calls `make_bytes_payload` to |
11 | + construct the payload, which this then yields. |
12 | + |
13 | + If `content` is a unicode string, this calls `make_string_payload`. |
14 | + |
15 | + If `content` is file-like -- it inherits from `IOBase` or `file` -- |
16 | + this calls `make_file_payload`. |
17 | + |
18 | + If `content` is iterable, this calls `make_payloads` for each item, |
19 | + with the same name, and then re-yields each payload generated. |
20 | + |
21 | + If `content` is callable, this calls it with no arguments, and then |
22 | + uses the result as a context manager. This can be useful if the |
23 | + callable returns an open file, for example, because the context |
24 | + protocol means it will be closed after use. |
25 | + |
26 | + This raises `AssertionError` if it encounters anything else. |
27 | + """ |
28 | if isinstance(content, bytes): |
29 | yield make_bytes_payload(name, content) |
30 | elif isinstance(content, unicode): |
31 | yield make_string_payload(name, content) |
32 | - elif isinstance(content, IOBase): |
33 | + elif isinstance(content, (IOBase, file)): |
34 | yield make_file_payload(name, content) |
35 | elif callable(content): |
36 | with content() as content: |
37 | |
38 | === modified file 'src/apiclient/tests/test_multipart.py' |
39 | --- src/apiclient/tests/test_multipart.py 2013-10-18 16:57:37 +0000 |
40 | +++ src/apiclient/tests/test_multipart.py 2014-04-03 19:58:37 +0000 |
41 | @@ -85,18 +85,20 @@ |
42 | ahem_django_ahem) |
43 | |
44 | def test_encode_multipart_data_multiple_params(self): |
45 | - # Sequences of parameters and files can be passed to |
46 | - # encode_multipart_data() so that multiple parameters/files with the |
47 | - # same name can be provided. |
48 | + # Sequences of parameters and files passed to |
49 | + # encode_multipart_data() permit use of the same name for |
50 | + # multiple parameters and/or files. See `make_payloads` to |
51 | + # understand how it processes different types of parameter |
52 | + # values. |
53 | params_in = [ |
54 | ("one", "ABC"), |
55 | ("one", "XYZ"), |
56 | - ("two", "DEF"), |
57 | - ("two", "UVW"), |
58 | + ("two", ["DEF", "UVW"]), |
59 | ] |
60 | files_in = [ |
61 | - ("f-one", BytesIO(urandom(32))), |
62 | - ("f-two", BytesIO(urandom(32))), |
63 | + ("f-one", BytesIO(b"f1")), |
64 | + ("f-two", open(self.make_file(contents=b"f2"), "rb")), |
65 | + ("f-three", lambda: open(self.make_file(contents=b"f3"), "rb")), |
66 | ] |
67 | body, headers = encode_multipart_data(params_in, files_in) |
68 | self.assertEqual("%s" % len(body), headers["Content-Length"]) |
69 | @@ -107,13 +109,14 @@ |
70 | params_out, files_out = ( |
71 | parse_headers_and_body_with_django(headers, body)) |
72 | params_out_expected = MultiValueDict() |
73 | - for name, value in params_in: |
74 | - params_out_expected.appendlist(name, value) |
75 | + params_out_expected.appendlist("one", "ABC") |
76 | + params_out_expected.appendlist("one", "XYZ") |
77 | + params_out_expected.appendlist("two", "DEF") |
78 | + params_out_expected.appendlist("two", "UVW") |
79 | self.assertEqual( |
80 | params_out_expected, params_out, |
81 | ahem_django_ahem) |
82 | - self.assertSetEqual({"f-one", "f-two"}, set(files_out)) |
83 | - files_expected = {name: buf.getvalue() for name, buf in files_in} |
84 | + files_expected = {"f-one": b"f1", "f-two": b"f2", "f-three": b"f3"} |
85 | files_observed = {name: buf.read() for name, buf in files_out.items()} |
86 | self.assertEqual( |
87 | files_expected, files_observed, |
88 | |
89 | === modified file 'src/maascli/api.py' |
90 | --- src/maascli/api.py 2014-02-27 13:22:53 +0000 |
91 | +++ src/maascli/api.py 2014-04-03 19:58:37 +0000 |
92 | @@ -19,10 +19,12 @@ |
93 | import argparse |
94 | from collections import defaultdict |
95 | from email.message import Message |
96 | +from functools import partial |
97 | import httplib |
98 | from itertools import chain |
99 | import json |
100 | from operator import itemgetter |
101 | +import re |
102 | import sys |
103 | from textwrap import ( |
104 | dedent, |
105 | @@ -34,7 +36,10 @@ |
106 | ) |
107 | |
108 | from apiclient.maas_client import MAASOAuth |
109 | -from apiclient.multipart import encode_multipart_data |
110 | +from apiclient.multipart import ( |
111 | + build_multipart_message, |
112 | + encode_multipart_message, |
113 | + ) |
114 | from apiclient.utils import ( |
115 | ascii_url, |
116 | urlencode, |
117 | @@ -166,6 +171,10 @@ |
118 | uri, body, headers = self.prepare_payload( |
119 | self.op, self.method, uri, options.data) |
120 | |
121 | + # Headers are returned as a list, but they must be a dict for |
122 | + # the signing machinery. |
123 | + headers = dict(headers) |
124 | + |
125 | # Sign request if credentials have been provided. |
126 | if self.credentials is not None: |
127 | self.sign(uri, headers, self.credentials) |
128 | @@ -190,15 +199,32 @@ |
129 | |
130 | @staticmethod |
131 | def name_value_pair(string): |
132 | - parts = string.split("=", 1) |
133 | - if len(parts) == 2: |
134 | - return tuple(parts) |
135 | + """Ensure that `string` is a valid ``name:value`` pair. |
136 | + |
137 | + When `string` is of the form ``name=value``, this returns a |
138 | + 2-tuple of ``name, value``. |
139 | + |
140 | + However, when `string` is of the form ``name@=value``, this |
141 | + returns a ``name, opener`` tuple, where ``opener`` is a function |
142 | + that will return an open file handle when called. The file will |
143 | + be opened in binary mode for reading only. |
144 | + """ |
145 | + parts = re.split(r'(=|@=)', string, 1) |
146 | + if len(parts) == 3: |
147 | + name, what, value = parts |
148 | + if what == "=": |
149 | + return name, value |
150 | + elif what == "@=": |
151 | + return name, partial(open, value, "rb") |
152 | + else: |
153 | + raise AssertionError( |
154 | + "Unrecognised separator %r" % what) |
155 | else: |
156 | raise CommandError( |
157 | - "%r is not a name=value pair" % string) |
158 | + "%r is not a name=value or name@=filename pair" % string) |
159 | |
160 | - @staticmethod |
161 | - def prepare_payload(op, method, uri, data): |
162 | + @classmethod |
163 | + def prepare_payload(cls, op, method, uri, data): |
164 | """Return the URI (modified perhaps) and body and headers. |
165 | |
166 | - For GET requests, encode parameters in the query string. |
167 | @@ -209,18 +235,27 @@ |
168 | |
169 | :param method: The HTTP method. |
170 | :param uri: The URI of the action. |
171 | - :param data: A dict or iterable of name=value pairs to pack into the |
172 | - body or headers, depending on the type of request. |
173 | + :param data: An iterable of ``name, value`` or ``name, opener`` |
174 | + tuples (see `name_value_pair`) to pack into the body or |
175 | + query, depending on the type of request. |
176 | """ |
177 | + query = [] if op is None else [("op", op)] |
178 | + |
179 | + def slurp(opener): |
180 | + with opener() as fd: |
181 | + return fd.read() |
182 | + |
183 | if method == "GET": |
184 | - query = data if op is None else chain([("op", op)], data) |
185 | - body, headers = None, {} |
186 | + query.extend( |
187 | + (name, slurp(value) if callable(value) else value) |
188 | + for name, value in data) |
189 | + body, headers = None, [] |
190 | else: |
191 | - query = [] if op is None else [("op", op)] |
192 | - if data: |
193 | - body, headers = encode_multipart_data(data) |
194 | + if data is None or len(data) == 0: |
195 | + body, headers = None, [] |
196 | else: |
197 | - body, headers = None, {} |
198 | + message = build_multipart_message(data) |
199 | + headers, body = encode_multipart_message(message) |
200 | |
201 | uri = urlparse(uri)._replace(query=urlencode(query)).geturl() |
202 | return uri, body, headers |
203 | |
204 | === modified file 'src/maascli/tests/test_api.py' |
205 | --- src/maascli/tests/test_api.py 2014-02-27 13:22:53 +0000 |
206 | +++ src/maascli/tests/test_api.py 2014-04-03 19:58:37 +0000 |
207 | @@ -15,6 +15,7 @@ |
208 | __all__ = [] |
209 | |
210 | import collections |
211 | +from functools import partial |
212 | import httplib |
213 | import io |
214 | import json |
215 | @@ -515,25 +516,26 @@ |
216 | {"method": "POST", "data": [], |
217 | "expected_uri": uri_base, |
218 | "expected_body": None, |
219 | - "expected_headers": {}}), |
220 | + "expected_headers": []}), |
221 | ("read", |
222 | {"method": "GET", "data": [], |
223 | "expected_uri": uri_base, |
224 | "expected_body": None, |
225 | - "expected_headers": {}}), |
226 | + "expected_headers": []}), |
227 | ("update", |
228 | {"method": "PUT", "data": [], |
229 | "expected_uri": uri_base, |
230 | "expected_body": None, |
231 | - "expected_headers": {}}), |
232 | + "expected_headers": []}), |
233 | ("delete", |
234 | {"method": "DELETE", "data": [], |
235 | "expected_uri": uri_base, |
236 | "expected_body": None, |
237 | - "expected_headers": {}}), |
238 | - # With data, PUT, POST, and DELETE requests have their body and extra |
239 | - # headers prepared by encode_multipart_data. For GET requests, the |
240 | - # data is encoded into the query string, and both the request body and |
241 | + "expected_headers": []}), |
242 | + # With data, PUT, POST, and DELETE requests have their body and |
243 | + # extra headers prepared by build_multipart_message and |
244 | + # encode_multipart_message. For GET requests, the data is |
245 | + # encoded into the query string, and both the request body and |
246 | # extra headers are empty. |
247 | ("create-with-data", |
248 | {"method": "POST", "data": [("foo", "bar"), ("foo", "baz")], |
249 | @@ -544,7 +546,7 @@ |
250 | {"method": "GET", "data": [("foo", "bar"), ("foo", "baz")], |
251 | "expected_uri": uri_base + "?foo=bar&foo=baz", |
252 | "expected_body": None, |
253 | - "expected_headers": {}}), |
254 | + "expected_headers": []}), |
255 | ("update-with-data", |
256 | {"method": "PUT", "data": [("foo", "bar"), ("foo", "baz")], |
257 | "expected_uri": uri_base, |
258 | @@ -565,27 +567,28 @@ |
259 | {"method": "POST", "data": [], |
260 | "expected_uri": uri_base + "?op=something", |
261 | "expected_body": None, |
262 | - "expected_headers": {}}), |
263 | + "expected_headers": []}), |
264 | ("read", |
265 | {"method": "GET", "data": [], |
266 | "expected_uri": uri_base + "?op=something", |
267 | "expected_body": None, |
268 | - "expected_headers": {}}), |
269 | + "expected_headers": []}), |
270 | ("update", |
271 | {"method": "PUT", "data": [], |
272 | "expected_uri": uri_base + "?op=something", |
273 | "expected_body": None, |
274 | - "expected_headers": {}}), |
275 | + "expected_headers": []}), |
276 | ("delete", |
277 | {"method": "DELETE", "data": [], |
278 | "expected_uri": uri_base + "?op=something", |
279 | "expected_body": None, |
280 | - "expected_headers": {}}), |
281 | - # With data, PUT, POST, and DELETE requests have their body and extra |
282 | - # headers prepared by encode_multipart_data. For GET requests, the |
283 | - # data is encoded into the query string, and both the request body and |
284 | - # extra headers are empty. The operation is encoded into the query |
285 | - # string. |
286 | + "expected_headers": []}), |
287 | + # With data, PUT, POST, and DELETE requests have their body and |
288 | + # extra headers prepared by build_multipart_message and |
289 | + # encode_multipart_message. For GET requests, the data is |
290 | + # encoded into the query string, and both the request body and |
291 | + # extra headers are empty. The operation is encoded into the |
292 | + # query string. |
293 | ("create-with-data", |
294 | {"method": "POST", "data": [("foo", "bar"), ("foo", "baz")], |
295 | "expected_uri": uri_base + "?op=something", |
296 | @@ -595,7 +598,7 @@ |
297 | {"method": "GET", "data": [("foo", "bar"), ("foo", "baz")], |
298 | "expected_uri": uri_base + "?op=something&foo=bar&foo=baz", |
299 | "expected_body": None, |
300 | - "expected_headers": {}}), |
301 | + "expected_headers": []}), |
302 | ("update-with-data", |
303 | {"method": "PUT", "data": [("foo", "bar"), ("foo", "baz")], |
304 | "expected_uri": uri_base + "?op=something", |
305 | @@ -619,9 +622,12 @@ |
306 | scenarios = scenarios_without_op + scenarios_with_op |
307 | |
308 | def test_prepare_payload(self): |
309 | - # Patch encode_multipart_data to match the scenarios. |
310 | - encode_multipart_data = self.patch(api, "encode_multipart_data") |
311 | - encode_multipart_data.return_value = sentinel.body, sentinel.headers |
312 | + # Patch build_multipart_message and encode_multipart_message to |
313 | + # match the scenarios. |
314 | + build_multipart_message = self.patch(api, "build_multipart_message") |
315 | + build_multipart_message.return_value = sentinel.message |
316 | + encode_multipart_message = self.patch(api, "encode_multipart_message") |
317 | + encode_multipart_message.return_value = sentinel.headers, sentinel.body |
318 | # The payload returned is a 3-tuple of (uri, body, headers). |
319 | payload = api.Action.prepare_payload( |
320 | op=self.op, method=self.method, |
321 | @@ -632,8 +638,43 @@ |
322 | Equals(self.expected_headers), |
323 | ) |
324 | self.assertThat(payload, MatchesListwise(expected)) |
325 | - # encode_multipart_data, when called, is passed the data |
326 | + # encode_multipart_message, when called, is passed the data |
327 | # unadulterated. |
328 | if self.expected_body is sentinel.body: |
329 | self.assertThat( |
330 | - api.encode_multipart_data, MockCalledOnceWith(self.data)) |
331 | + api.build_multipart_message, |
332 | + MockCalledOnceWith(self.data)) |
333 | + self.assertThat( |
334 | + api.encode_multipart_message, |
335 | + MockCalledOnceWith(sentinel.message)) |
336 | + |
337 | + |
338 | +class TestPayloadPreparationWithFiles(MAASTestCase): |
339 | + """Tests for `maascli.api.Action.prepare_payload` involving files.""" |
340 | + |
341 | + def test_files_are_included(self): |
342 | + parameter = factory.make_name("param") |
343 | + contents = factory.getRandomBytes() |
344 | + filename = self.make_file(contents=contents) |
345 | + # Writing the parameter as "parameter@=filename" on the |
346 | + # command-line causes name_value_pair() to return a `name, |
347 | + # opener` tuple, where `opener` is a callable that returns an |
348 | + # open file handle. |
349 | + data = [(parameter, partial(open, filename, "rb"))] |
350 | + uri, body, headers = api.Action.prepare_payload( |
351 | + op=None, method="POST", uri="http://localhost", data=data) |
352 | + |
353 | + expected_body_template = """\ |
354 | + --... |
355 | + Content-Transfer-Encoding: base64 |
356 | + Content-Disposition: form-data; name="%s"; filename="%s" |
357 | + MIME-Version: 1.0 |
358 | + Content-Type: application/octet-stream |
359 | + |
360 | + %s |
361 | + --...-- |
362 | + """ |
363 | + expected_body = expected_body_template % ( |
364 | + parameter, parameter, contents.encode("base64")) |
365 | + |
366 | + self.assertDocTestMatches(expected_body, body) |
To try it out:
bin/maas foo files add filename=foobar file@=setup.py
bin/maas foo files get filename=foobar