Merge lp:~ltrager/maas/delete_on_files into lp:maas/trunk

Proposed by Lee Trager on 2016-04-21
Status: Merged
Approved by: Lee Trager on 2016-04-25
Approved revision: 4944
Merged at revision: 4951
Proposed branch: lp:~ltrager/maas/delete_on_files
Merge into: lp:maas/trunk
Diff against target: 181 lines (+58/-21)
6 files modified
src/maascli/api.py (+1/-1)
src/maascli/tests/test_api.py (+6/-6)
src/maasserver/api/files.py (+17/-2)
src/maasserver/api/support.py (+8/-6)
src/maasserver/api/tests/test_filestorage.py (+12/-0)
src/maastesting/djangoclient.py (+14/-6)
To merge this branch: bzr merge lp:~ltrager/maas/delete_on_files
Reviewer Review Type Date Requested Status
Gavin Panella (community) 2016-04-21 Approve on 2016-04-21
Review via email: mp+292455@code.launchpad.net

Commit message

Allow deleting a file by specifying the filename as a parameter

Description of the change

MAAS currently has the ability to get files on the files handle by calling the get operation and specifying the filename as a parameter. This allows users to get files with a leading /. The main concern in lp:1566108 is how to delete them. This adds the ability to call the delete operation on files specifying the filename as a parameter.

I had to make this call a POST operation as piston only processes form data on GET and POST. See the link below for a summary on why

http://stackoverflow.com/questions/500434/using-django-rest-interface-with-http-put

To post a comment you must log in.
Gavin Panella (allenap) wrote :

> I had to make this call a POST operation as piston only processes form
> data on GET and POST.

Django is deceiving us.

First, Django does parse the query string for all HTTP methods, but it
makes it sound like it doesn't because it puts the result into
request.GET.

Second, the Django test client assumes that you only ever put form data
into the request body OR the query string, not both. For GET it assumes
that form data goes in the query string, and for PUT, POST, and DELETE
it assumes that all form data goes into the request body. When using the
test client we don't see that this is happening.

Now, the URL http://example.com/?foo=123 refers to a resource in the
same way that http://example.com/bar/789 refers to a resource — that is,
the _whole URL_ is a reference to a thing, not just the bit before the
query string.

But Django leads us to imagine that `GET foo?bar=123` is akin to a
function call to the resource foo with the argument bar=123, but what
we've actually got is a reference — foo?bar=123 — to a resource.

The *implementation* may route to a function according to the path
before the query string, and then pass parameters from the query into
the function, but at the HTTP level it's just a reference, an address to
a resource.

Anyway, merge lp:~allenap/maas/delete_on_files and you'll find that I've
added an extra fix-up to the Django test client and adjusted the code so
that `DELETE http://.../files/?filename=foo/bar` works.

I've also tightened up the rules on method naming so that `POST
...?op=delete` will not longer be permitted; I noticed that both
FileHandler.delete and FilesHandler.delete worked via both `POST
path?op=delete` AND `DELETE path`.

review: Needs Fixing
lp:~ltrager/maas/delete_on_files updated on 2016-04-21
4944. By Lee Trager on 2016-04-21

Merge allenap's branch

Gavin Panella (allenap) wrote :

It's well past EOD here and I don't want to block you overnight, so +1 on the basis that you think the changes I made in lp:~allenap/maas/delete_on_files make sense. If they're no good or you're not happy with them then it's up to you if you want to land this and talk tomorrow, or if you want to wait before landing.

review: Approve
Lee Trager (ltrager) wrote :

Thanks for your help with this branch. During testing I noticed that maascli was sending the request data in the body, not the query string. As you mentioned piston only processes form data in the query string for request.GET so this doesn't work for us. I modified maascli to send all form data in the query string for DELETE requests. As this is the only DELETE operation in MAAS which accepts parameters there should be no effect on any other calls.

Gavin Panella (allenap) wrote :

> I modified maascli to send all form data in the query string for
> DELETE requests. As this is the only DELETE operation in MAAS which
> accepts parameters there should be no effect on any other calls.

I think this is a reasonable and good thing to do. I can't think of a
situation where it makes much sense to send a request body for DELETE.

More work though: I suspect this ought to be backported so that maascli
on Trusty can work with MAAS on Xenial, though Andres is arbiter of
that.

Lee Trager (ltrager) wrote :

Spoke with Andres in the standup today, we don't need a backport for 1.9.2 but might do it for 1.9.3. Was there anything else this MP needs?

Gavin Panella (allenap) wrote :

> Spoke with Andres in the standup today, we don't need a backport for
> 1.9.2 but might do it for 1.9.3. Was there anything else this MP
> needs?

Nope, think it's all good. Land it!

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/maascli/api.py'
2--- src/maascli/api.py 2015-12-15 22:44:22 +0000
3+++ src/maascli/api.py 2016-04-21 19:52:54 +0000
4@@ -226,7 +226,7 @@
5 with opener() as fd:
6 return fd.read()
7
8- if method == "GET":
9+ if method in ["GET", "DELETE"]:
10 query.extend(
11 (name, slurp(value) if callable(value) else value)
12 for name, value in data)
13
14=== modified file 'src/maascli/tests/test_api.py'
15--- src/maascli/tests/test_api.py 2016-03-28 13:54:47 +0000
16+++ src/maascli/tests/test_api.py 2016-04-21 19:52:54 +0000
17@@ -527,9 +527,9 @@
18 "expected_headers": sentinel.headers}),
19 ("delete-with-data",
20 {"method": "DELETE", "data": [("foo", "bar"), ("foo", "baz")],
21- "expected_uri": uri_base,
22- "expected_body": sentinel.body,
23- "expected_headers": sentinel.headers}),
24+ "expected_uri": uri_base + "?foo=bar&foo=baz",
25+ "expected_body": None,
26+ "expected_headers": []}),
27 )
28
29 # Scenarios for non-ReSTful operations; i.e. with an "op" parameter.
30@@ -579,9 +579,9 @@
31 "expected_headers": sentinel.headers}),
32 ("delete-with-data",
33 {"method": "DELETE", "data": [("foo", "bar"), ("foo", "baz")],
34- "expected_uri": uri_base + "?op=something",
35- "expected_body": sentinel.body,
36- "expected_headers": sentinel.headers}),
37+ "expected_uri": uri_base + "?op=something&foo=bar&foo=baz",
38+ "expected_body": None,
39+ "expected_headers": []}),
40 )
41
42 scenarios_without_op = tuple(
43
44=== modified file 'src/maasserver/api/files.py'
45--- src/maasserver/api/files.py 2016-04-14 15:25:36 +0000
46+++ src/maasserver/api/files.py 2016-04-21 19:52:54 +0000
47@@ -139,7 +139,6 @@
48 stream, content_type='application/json; charset=utf-8',
49 status=int(http.client.OK))
50
51- @operation(idempotent=False)
52 def delete(self, request, filename):
53 """Delete a FileStorage object."""
54 stored_file = get_object_or_404(
55@@ -158,7 +157,7 @@
56 class FilesHandler(OperationsHandler):
57 """Manage the collection of all the files in this MAAS."""
58 api_doc_section_name = "Files"
59- update = delete = None
60+ update = None
61 anonymous = AnonFilesHandler
62
63 get_by_name = operation(
64@@ -211,6 +210,22 @@
65 files = files.order_by('filename')
66 return files
67
68+ def delete(self, request, **kwargs):
69+ """Delete a FileStorage object.
70+
71+ :param filename: The filename of the object to be deleted.
72+ :type filename: unicode
73+ """
74+ # It is valid for a path in a POST, PUT, or DELETE (or any other HTTP
75+ # method) to contain a query string. However, Django only makes
76+ # parameters from the query string available in the badly named
77+ # request.GET object.
78+ filename = get_mandatory_param(request.GET, 'filename')
79+ stored_file = get_object_or_404(
80+ FileStorage, filename=filename, owner=request.user)
81+ stored_file.delete()
82+ return rc.DELETED
83+
84 @classmethod
85 def resource_uri(cls, *args, **kwargs):
86 return ('files_handler', [])
87
88=== modified file 'src/maasserver/api/support.py'
89--- src/maasserver/api/support.py 2016-04-14 15:25:36 +0000
90+++ src/maasserver/api/support.py 2016-04-21 19:52:54 +0000
91@@ -180,13 +180,15 @@
92 exports.update(operations)
93
94 # Check that no CRUD methods have been marked as operations (i.e.
95- # those that are used via op=name). This causes weird behaviour within
96- # Piston3 and/or Django so avoid it.
97- for signature in OperationsResource.crudmap.items():
98- if signature in exports:
99+ # those that are used via op=name). This causes (unconfirmed) weird
100+ # behaviour within Piston3 and/or Django, and is plain confusing
101+ # anyway, so forbid it.
102+ methods_exported = {method for http_method, method in exports}
103+ for http_method, method in OperationsResource.crudmap.items():
104+ if method in methods_exported:
105 raise AssertionError(
106- "A CRUD operation (%s) has been registered as an "
107- "operation on %s." % ("%s/%s" % signature, name))
108+ "A CRUD operation (%s/%s) has been registered as an "
109+ "operation on %s." % (http_method, method, name))
110
111 # Export CRUD methods.
112 exports.update(crud)
113
114=== modified file 'src/maasserver/api/tests/test_filestorage.py'
115--- src/maasserver/api/tests/test_filestorage.py 2016-04-14 15:25:36 +0000
116+++ src/maasserver/api/tests/test_filestorage.py 2016-04-21 19:52:54 +0000
117@@ -385,3 +385,15 @@
118 self.assertEqual(http.client.NO_CONTENT, response.status_code)
119 files = FileStorage.objects.filter(filename=filename)
120 self.assertEqual([], list(files))
121+
122+ def test_delete_on_files(self):
123+ filename = factory.make_name('file')
124+ factory.make_FileStorage(
125+ filename=filename, content=b"test content",
126+ owner=self.logged_in_user)
127+ response = self.client.delete(
128+ reverse('files_handler'), query={"filename": filename})
129+
130+ self.assertEqual(http.client.NO_CONTENT, response.status_code)
131+ files = FileStorage.objects.filter(filename=filename)
132+ self.assertEqual([], list(files))
133
134=== modified file 'src/maastesting/djangoclient.py'
135--- src/maastesting/djangoclient.py 2015-12-01 18:12:59 +0000
136+++ src/maastesting/djangoclient.py 2016-04-21 19:52:54 +0000
137@@ -10,23 +10,27 @@
138 from functools import wraps
139
140 from django.test import client
141+from django.utils.http import urlencode
142
143
144 def transparent_encode_multipart(func):
145 """Wrap an HTTP client method, transparently encoding multipart data.
146
147- This wraps Django's `Client` HTTP verb methods -- put, get, etc. -- in a
148- way that's both convenient and compatible across Django versions. It
149- augments those methods to accept a dict of data to be sent as part of the
150+ This wraps some of Django's `Client` HTTP verb methods -- delete, options,
151+ patch, put -- so they accept a dict of data to be sent as part of the
152 request body, in MIME multipart encoding.
153
154+ This also accepts an optional dict of query parameters (as `query`) to be
155+ encoded as a query string and appended to the given path.
156+
157 Since Django 1.5, these HTTP verb methods require data in the form of a
158- byte string. The application (that's us) need to take care of MIME
159+ byte string. The application (that's us) needs to take care of MIME
160 encoding.
161 """
162 @wraps(func)
163 def maybe_encode_multipart(
164- self, path, data=b"", content_type=None, **extra):
165+ self, path, data=b"", content_type=None, secure=False, query=None,
166+ **extra):
167
168 if isinstance(data, bytes):
169 if content_type is None:
170@@ -39,7 +43,11 @@
171 "Cannot combine data (%r) with content-type (%r)."
172 % (data, content_type))
173
174- return func(self, path, data, content_type, **extra)
175+ if query is not None:
176+ query = urlencode(query, doseq=True)
177+ path = path + ("&" if "?" in path else "?") + query
178+
179+ return func(self, path, data, content_type, secure, **extra)
180
181 return maybe_encode_multipart
182