Merge lp:~rvb/maas/file-delete into lp:~maas-committers/maas/trunk

[Raphaël Badin]

Add support for listing files (FileStorage objects).

[Gavin Panella (allenap)]

Looks good, with only one question.

[1]

> +    url(r'files/(?P<filename>[^/]+)/$', file_handler, name='file_handler'),

It seems fishy to refer to a filename with a URL that looks like a
directory.

review: Approve

[Raphaël Badin (rvb)]

> Looks good, with only one question.
>
>
> [1]
>
> > +    url(r'files/(?P<filename>[^/]+)/$', file_handler, name='file_handler'),
>
> It seems fishy to refer to a filename with a URL that looks like a
> directory.

I don't understand the question… I suppose you're referring to the fact that the filename is used as an element of the path… but what do you suggest instead?

[Gavin Panella (allenap)]

> I don't understand the question… I suppose you're referring to the fact that the filename is used as an element of the path… but what do you suggest instead?

It's that there's a forward slash after the filename.

[Raphaël Badin (rvb)]

> > I don't understand the question… I suppose you're referring to the fact that
> the filename is used as an element of the path… but what do you suggest
> instead?
>
> It's that there's a forward slash after the filename.

Oh. That.

The fact that we have a final slash is an option in Django we choose to turn on to make urls unique (instead of having http://server.com/aa and http://server.com/aa/, we only have http://server.com/aa/ and http://server.com/aa redirects to http://server.com/aa/).

[Julian Edwards (julian-edwards)]

Question: what about the multiple users bug? ie. every user can see all the
files. This is bad :)

=== modified file 'src/maasserver/api.py'
--- src/maasserver/api.py	2013-01-30 15:32:09 +0000
+++ src/maasserver/api.py	2013-02-14 08:15:27 +0000
@@ -63,6 +63,7 @@
     "CommissioningScriptHandler",
     "CommissioningScriptsHandler",
     "CommissioningResultsHandler",
+    "FileHandler",
     "FilesHandler",
     "get_oauth_token",
     "MaasHandler",
@@ -108,6 +109,7 @@
     render_to_response,
     )
 from django.template import RequestContext
+from django.utils.http import urlquote_plus
 from docutils import core
 from formencode import validators
 from maasserver.api_support import (
@@ -808,6 +810,26 @@
         return ('files_handler', [])
 
 
+DISPLAYED_FILES_FIELDS = ('filename', )
+
+
+class FileHandler(OperationsHandler):
+    """Manage a FileStorage object.
+
+    The file is identified by its filename.
+    """
+    model = FileStorage
+    fields = DISPLAYED_FILES_FIELDS
+    create = read = update = delete = None
+
+    @classmethod
+    def resource_uri(cls, stored_file=None):
+        filename = "filename"
+        if stored_file is not None:
+            filename = urlquote_plus(stored_file.filename)
+        return ('file_handler', (filename, ))
+
+
 class FilesHandler(OperationsHandler):
     """File management operations."""
     create = read = update = delete = None
@@ -840,6 +862,23 @@
         FileStorage.objects.save_file(filename, uploaded_file)
         return HttpResponse('', status=httplib.CREATED)
 
+    @operation(idempotent=True)
+    def list(self, request):
+        """List the files from the file storage.
+
+        The returned files are ordered by file name and the content is
+        excluded.
+
+        :param prefix: Optional prefix used to filter out the returned files.
+        :type prefix: string
+        """
+        prefix = request.GET.get("prefix", None)
+        files = FileStorage.objects.all()
+        if prefix is not None:
+            files = files.filter(filename__startswith=prefix)
+        files = files.order_by('filename')
+        return files
+
     @classmethod
     def resource_uri(cls, *args, **kwargs):
         return ('files_handler', [])

=== modified file 'src/maasserver/tests/test_api.py'
--- src/maasserver/tests/test_api.py	2013-01-30 10:24:06 +0000
+++ src/maasserver/tests/test_api.py	2013-02-14 08:15:27 +0000
@@ -45,6 +45,7 @@
     )
 from django.http import QueryDict
 from django.test.client import RequestFactory
+from django.utils.http import urlquote_plus
 from fixtures import (
     EnvironmentVariableFixture,
     Fixture,
@@ -2626,6 +2627,13 @@
         self.assertEqual(httplib.OK, response.status_code)
         self.assertEqual(b"give me rope", response.content)
 
+    def test_anon_cannot_list_files(self):
+        factory.make_file_storage(
+             filename="filename", content=b"test content")
+        response = self.make_API_GET_request("list")
+        # The 'list' operation is not available to anon users.
+        self.assertEqual(httplib.BAD_REQUEST, response.status_code)
+
 
 class FileStorageAPITest(FileStorageAPITestMixin, APITestCase):
 
@@ -2702,6 +2710,48 @@
         self.assertIn('text/plain', response['Content-Type'])
         self.assertEqual("File not found", response.content)
 
+    def test_list_files_returns_ordered_list(self):
+        filenames = ["myfiles/a", "myfiles/z", "myfiles/b"]
+        for filename in filenames:
+            factory.make_file_storage(
+                filename=filename, content=b"test content")
+        response = self.make_API_GET_request("list")
+        self.assertEqual(httplib.OK, response.status_code)
+        parsed_results = json.loads(response.content)
+        filenames = [result['filename'] for result in parsed_results]
+        self.assertEqual(sorted(filenames), filenames)
+
+    def test_list_files_lists_files_with_prefix(self):
+        filenames_with_prefix = ["prefix-file1", "prefix-file2"]
+        filenames = filenames_with_prefix + ["otherfile", "otherfile2"]
+        for filename in filenames:
+            factory.make_file_storage(
+                filename=filename, content=b"test content")
+        response = self.client.get(
+            self.get_uri('files/'), {"op": "list", "prefix": "prefix-"})
+        self.assertEqual(httplib.OK, response.status_code)
+        parsed_results = json.loads(response.content)
+        filenames = [result['filename'] for result in parsed_results]
+        self.assertItemsEqual(filenames_with_prefix, filenames)
+
+    def test_list_files_does_not_include_file_content(self):
+        factory.make_file_storage(
+             filename="filename", content=b"test content")
+        response = self.make_API_GET_request("list")
+        parsed_results = json.loads(response.content)
+        self.assertNotIn('content', parsed_results[0].keys())
+
+    def test_files_resource_uri_are_url_escaped(self):
+        filename = "&*!c/%//filename/"
+        factory.make_file_storage(
+             filename=filename, content=b"test content")
+        response = self.make_API_GET_request("list")
+        parsed_results = json.loads(response.content)
+        resource_uri = parsed_results[0]['resource_uri']
+        resource_uri_elements = resource_uri.split('/')
+        # The url-escaped name of the file is part of the resource uri.
+        self.assertIn(urlquote_plus(filename), resource_uri_elements)
+
 
 class TestTagAPI(APITestCase):
     """Tests for /api/1.0/tags/<tagname>/."""

=== modified file 'src/maasserver/urls_api.py'
--- src/maasserver/urls_api.py	2012-12-20 14:42:17 +0000
+++ src/maasserver/urls_api.py	2013-02-14 08:15:27 +0000
@@ -24,6 +24,7 @@
     CommissioningScriptHandler,
     CommissioningScriptsHandler,
     describe,
+    FileHandler,
     FilesHandler,
     MaasHandler,
     NodeGroupHandler,
@@ -47,6 +48,7 @@
 
 account_handler = RestrictedResource(AccountHandler, authentication=api_auth)
 files_handler = RestrictedResource(FilesHandler, authentication=api_auth)
+file_handler = RestrictedResource(FileHandler, authentication=api_auth)
 node_handler = RestrictedResource(NodeHandler, authentication=api_auth)
 nodes_handler = RestrictedResource(NodesHandler, authentication=api_auth)
 node_mac_handler = RestrictedResource(NodeMacHandler, authentication=api_auth)
@@ -105,6 +107,7 @@
     url(r'nodegroups/(?P<uuid>[^/]+)/interfaces/(?P<interface>[^/]+)/$',
         nodegroupinterface_handler, name='nodegroupinterface_handler'),
     url(r'files/$', files_handler, name='files_handler'),
+    url(r'files/(?P<filename>[^/]+)/$', file_handler, name='file_handler'),
     url(r'account/$', account_handler, name='account_handler'),
     url(r'boot-images/$', boot_images_handler, name='boot_images_handler'),
     url(r'tags/(?P<name>[\w\-]+)/$', tag_handler, name='tag_handler'),