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

[Lee Trager]

Allow deleting a file by specifying the filename as a parameter

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

[Gavin Panella (allenap)]

> 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

[Gavin Panella (allenap)]

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)]

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)]

> 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)]

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)]

> 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!

=== modified file 'src/maascli/api.py'
--- src/maascli/api.py	2015-12-15 22:44:22 +0000
+++ src/maascli/api.py	2016-04-21 19:52:54 +0000
@@ -226,7 +226,7 @@
             with opener() as fd:
                 return fd.read()
 
-        if method == "GET":
+        if method in ["GET", "DELETE"]:
             query.extend(
                 (name, slurp(value) if callable(value) else value)
                 for name, value in data)

=== modified file 'src/maascli/tests/test_api.py'
--- src/maascli/tests/test_api.py	2016-03-28 13:54:47 +0000
+++ src/maascli/tests/test_api.py	2016-04-21 19:52:54 +0000
@@ -527,9 +527,9 @@
           "expected_headers": sentinel.headers}),
         ("delete-with-data",
          {"method": "DELETE", "data": [("foo", "bar"), ("foo", "baz")],
-          "expected_uri": uri_base,
-          "expected_body": sentinel.body,
-          "expected_headers": sentinel.headers}),
+          "expected_uri": uri_base + "?foo=bar&foo=baz",
+          "expected_body": None,
+          "expected_headers": []}),
         )
 
     # Scenarios for non-ReSTful operations; i.e. with an "op" parameter.
@@ -579,9 +579,9 @@
           "expected_headers": sentinel.headers}),
         ("delete-with-data",
          {"method": "DELETE", "data": [("foo", "bar"), ("foo", "baz")],
-          "expected_uri": uri_base + "?op=something",
-          "expected_body": sentinel.body,
-          "expected_headers": sentinel.headers}),
+          "expected_uri": uri_base + "?op=something&foo=bar&foo=baz",
+          "expected_body": None,
+          "expected_headers": []}),
         )
 
     scenarios_without_op = tuple(

=== modified file 'src/maasserver/api/files.py'
--- src/maasserver/api/files.py	2016-04-14 15:25:36 +0000
+++ src/maasserver/api/files.py	2016-04-21 19:52:54 +0000
@@ -139,7 +139,6 @@
             stream, content_type='application/json; charset=utf-8',
             status=int(http.client.OK))
 
-    @operation(idempotent=False)
     def delete(self, request, filename):
         """Delete a FileStorage object."""
         stored_file = get_object_or_404(
@@ -158,7 +157,7 @@
 class FilesHandler(OperationsHandler):
     """Manage the collection of all the files in this MAAS."""
     api_doc_section_name = "Files"
-    update = delete = None
+    update = None
     anonymous = AnonFilesHandler
 
     get_by_name = operation(
@@ -211,6 +210,22 @@
         files = files.order_by('filename')
         return files
 
+    def delete(self, request, **kwargs):
+        """Delete a FileStorage object.
+
+        :param filename: The filename of the object to be deleted.
+        :type filename: unicode
+        """
+        # It is valid for a path in a POST, PUT, or DELETE (or any other HTTP
+        # method) to contain a query string. However, Django only makes
+        # parameters from the query string available in the badly named
+        # request.GET object.
+        filename = get_mandatory_param(request.GET, 'filename')
+        stored_file = get_object_or_404(
+            FileStorage, filename=filename, owner=request.user)
+        stored_file.delete()
+        return rc.DELETED
+
     @classmethod
     def resource_uri(cls, *args, **kwargs):
         return ('files_handler', [])

=== modified file 'src/maasserver/api/support.py'
--- src/maasserver/api/support.py	2016-04-14 15:25:36 +0000
+++ src/maasserver/api/support.py	2016-04-21 19:52:54 +0000
@@ -180,13 +180,15 @@
         exports.update(operations)
 
         # Check that no CRUD methods have been marked as operations (i.e.
-        # those that are used via op=name). This causes weird behaviour within
-        # Piston3 and/or Django so avoid it.
-        for signature in OperationsResource.crudmap.items():
-            if signature in exports:
+        # those that are used via op=name). This causes (unconfirmed) weird
+        # behaviour within Piston3 and/or Django, and is plain confusing
+        # anyway, so forbid it.
+        methods_exported = {method for http_method, method in exports}
+        for http_method, method in OperationsResource.crudmap.items():
+            if method in methods_exported:
                 raise AssertionError(
-                    "A CRUD operation (%s) has been registered as an "
-                    "operation on %s." % ("%s/%s" % signature, name))
+                    "A CRUD operation (%s/%s) has been registered as an "
+                    "operation on %s." % (http_method, method, name))
 
         # Export CRUD methods.
         exports.update(crud)

=== modified file 'src/maasserver/api/tests/test_filestorage.py'
--- src/maasserver/api/tests/test_filestorage.py	2016-04-14 15:25:36 +0000
+++ src/maasserver/api/tests/test_filestorage.py	2016-04-21 19:52:54 +0000
@@ -385,3 +385,15 @@
         self.assertEqual(http.client.NO_CONTENT, response.status_code)
         files = FileStorage.objects.filter(filename=filename)
         self.assertEqual([], list(files))
+
+    def test_delete_on_files(self):
+        filename = factory.make_name('file')
+        factory.make_FileStorage(
+            filename=filename, content=b"test content",
+            owner=self.logged_in_user)
+        response = self.client.delete(
+            reverse('files_handler'), query={"filename": filename})
+
+        self.assertEqual(http.client.NO_CONTENT, response.status_code)
+        files = FileStorage.objects.filter(filename=filename)
+        self.assertEqual([], list(files))

=== modified file 'src/maastesting/djangoclient.py'
--- src/maastesting/djangoclient.py	2015-12-01 18:12:59 +0000
+++ src/maastesting/djangoclient.py	2016-04-21 19:52:54 +0000
@@ -10,23 +10,27 @@
 from functools import wraps
 
 from django.test import client
+from django.utils.http import urlencode
 
 
 def transparent_encode_multipart(func):
     """Wrap an HTTP client method, transparently encoding multipart data.
 
-    This wraps Django's `Client` HTTP verb methods -- put, get, etc. -- in a
-    way that's both convenient and compatible across Django versions. It
-    augments those methods to accept a dict of data to be sent as part of the
+    This wraps some of Django's `Client` HTTP verb methods -- delete, options,
+    patch, put -- so they accept a dict of data to be sent as part of the
     request body, in MIME multipart encoding.
 
+    This also accepts an optional dict of query parameters (as `query`) to be
+    encoded as a query string and appended to the given path.
+
     Since Django 1.5, these HTTP verb methods require data in the form of a
-    byte string. The application (that's us) need to take care of MIME
+    byte string. The application (that's us) needs to take care of MIME
     encoding.
     """
     @wraps(func)
     def maybe_encode_multipart(
-            self, path, data=b"", content_type=None, **extra):
+            self, path, data=b"", content_type=None, secure=False, query=None,
+            **extra):
 
         if isinstance(data, bytes):
             if content_type is None:
@@ -39,7 +43,11 @@
                 "Cannot combine data (%r) with content-type (%r)."
                 % (data, content_type))
 
-        return func(self, path, data, content_type, **extra)
+        if query is not None:
+            query = urlencode(query, doseq=True)
+            path = path + ("&" if "?" in path else "?") + query
+
+        return func(self, path, data, content_type, secure, **extra)
 
     return maybe_encode_multipart