launchpad-reviewers team mailing list archive

[Raphaël Badin <raphael.badin@xxxxxxxxxxxxx>]

Raphaël Badin has proposed merging lp:~rvb/maas/support-slashes-filename into lp:maas.

Commit message:
Add support for slashes in filenames without resorting to escaping slashes.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~rvb/maas/support-slashes-filename/+merge/152369

This branch changes how we support slashes in filenames.  Instead of url-encoding them, we simply change the regexp in urls.py to allow for slashes in filenames.  This reduces the burden on the client side (no need to deal with escaping).  The only drawback is that this will make it hard (if not impossible) to add aother REST-ish endpoint on top of the existing file endpoint (something like /api/1.0/files/<filename>/subobject/<subobjectname>/).  But we don't expect we will have to do that anytime soon.
-- 
https://code.launchpad.net/~rvb/maas/support-slashes-filename/+merge/152369
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~rvb/maas/support-slashes-filename into lp:maas.

=== modified file 'src/maasserver/api.py'
--- src/maasserver/api.py	2013-02-25 15:00:20 +0000
+++ src/maasserver/api.py	2013-03-08 10:30:33 +0000
@@ -115,7 +115,6 @@
     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 (
@@ -918,7 +917,7 @@
     def resource_uri(cls, stored_file=None):
         filename = "filename"
         if stored_file is not None:
-            filename = urlquote_plus(stored_file.filename)
+            filename = stored_file
         return ('file_handler', (filename, ))
 
 

=== modified file 'src/maasserver/tests/test_api.py'
--- src/maasserver/tests/test_api.py	2013-03-01 16:06:54 +0000
+++ src/maasserver/tests/test_api.py	2013-03-08 10:30:33 +0000
@@ -51,7 +51,6 @@
     QueryDict,
     )
 from django.test.client import RequestFactory
-from django.utils.http import urlquote_plus
 from fixtures import (
     EnvironmentVariableFixture,
     Fixture,
@@ -2737,6 +2736,16 @@
             "add", factory.make_name('upload'), factory.make_file_upload())
         self.assertEqual(httplib.CREATED, response.status_code)
 
+    def test_add_file_with_slashes_in_name_succeeds(self):
+        filename = "filename/with/slashes/in/it"
+        response = self.make_API_POST_request(
+            "add", filename, factory.make_file_upload())
+        self.assertEqual(httplib.CREATED, response.status_code)
+        self.assertItemsEqual(
+            [filename],
+            FileStorage.objects.filter(
+                filename=filename).values_list('filename', flat=True))
+
     def test_add_file_fails_with_no_filename(self):
         response = self.make_API_POST_request(
             "add", fileObj=factory.make_file_upload())
@@ -2846,17 +2855,16 @@
         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/"
+    def test_files_resource_uri_supports_slashes_in_filenames(self):
+        filename = "a/filename/with/slashes/in/it/"
         factory.make_file_storage(
              filename=filename, content=b"test content",
              owner=self.logged_in_user)
         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)
+        expected_uri = reverse('file_handler', args=[filename])
+        self.assertEqual(expected_uri, resource_uri)
 
     def test_get_file_returns_file_object_with_content_base64_encoded(self):
         filename = factory.make_name("file")

=== modified file 'src/maasserver/urls_api.py'
--- src/maasserver/urls_api.py	2013-02-28 21:26:27 +0000
+++ src/maasserver/urls_api.py	2013-03-08 10:30:33 +0000
@@ -110,7 +110,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'^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'),