launchpad-reviewers team mailing list archive

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

Raphaël Badin has proposed merging lp:~rvb/maas/bug-1123986-key-api into lp:maas with lp:~rvb/maas/bug-1123986-api-fix as a prerequisite.

Commit message:
Add API endpoint to fetch the content of a file using its 'key'. Add 'anon_url' field when serializing file storage objects.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~rvb/maas/bug-1123986-key-api/+merge/149084
-- 
https://code.launchpad.net/~rvb/maas/bug-1123986-key-api/+merge/149084
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~rvb/maas/bug-1123986-key-api into lp:maas.

=== modified file 'src/maasserver/api.py'
--- src/maasserver/api.py	2013-02-18 16:12:22 +0000
+++ src/maasserver/api.py	2013-02-18 16:12:22 +0000
@@ -81,7 +81,10 @@
     "store_node_power_parameters",
     ]
 
-from base64 import b64decode
+from base64 import (
+    b64decode,
+    b64encode,
+    )
 from cStringIO import StringIO
 from datetime import (
     datetime,
@@ -792,6 +795,18 @@
     return HttpResponse(db_file.content, status=httplib.OK)
 
 
+def get_file_by_key(handler, request):
+    """Get a file from the file storage using its key.
+
+    :param key: The exact key of the file you want to get.
+    :type key: string
+    :return: The file is returned in the response content.
+    """
+    key = get_mandatory_param(request.GET, 'key')
+    db_file = get_object_or_404(FileStorage, key=key)
+    return HttpResponse(db_file.content, status=httplib.OK)
+
+
 class AnonFilesHandler(AnonymousOperationsHandler):
     """Anonymous file operations.
 
@@ -806,7 +821,10 @@
     """
     create = read = update = delete = None
 
-    get = operation(idempotent=True, exported_as='get')(get_file_by_name)
+    get_by_name = operation(
+        idempotent=True, exported_as='get')(get_file_by_name)
+    get_by_key = operation(
+        idempotent=True, exported_as='get_by_key')(get_file_by_key)
 
     @classmethod
     def resource_uri(cls, *args, **kwargs):
@@ -815,10 +833,25 @@
 
 # DISPLAYED_FILES_FIELDS_OBJECT is the list of fields used when dumping
 # lists of FileStorage objects.
-DISPLAYED_FILES_FIELDS = ('filename', )
-# DISPLAYED_FILES_FIELDS_OBJECT is the list of fields used when dumping
-# individual FileStorage objects.
-DISPLAYED_FILES_FIELDS_OBJECT = DISPLAYED_FILES_FIELDS + ('content', )
+DISPLAYED_FILES_FIELDS = ('filename', 'anon_url')
+
+
+def json_file_storage(stored_file, request):
+    # Convert stored_file into a json object: use the same fields used
+    # when serialising lists of object, plus the base64-encoded content.
+    dict_representation = {}
+    for fieldname in DISPLAYED_FILES_FIELDS:
+        dict_representation[fieldname] = getattr(stored_file, fieldname)
+    # Encode the content as base64.
+    dict_representation['content'] = b64encode(
+        getattr(stored_file, 'content'))
+    # Emit the json for this object manually because, no matter what the
+    # piston documentation says, once an type is associated with a list
+    # of fields by piston's typemapper mechanism, there is no way to
+    # override that in a specific handler with 'fields' or 'exclude'.
+    emitter = JSONEmitter(dict_representation, typemapper, None)
+    stream = emitter.render(request)
+    return stream
 
 
 class FileHandler(OperationsHandler):
@@ -834,10 +867,8 @@
         """GET a FileStorage object as a json object.
 
         The 'content' of the file is base64-encoded."""
-        # 'content' is a BinaryField, its 'value' is a base64-encoded version
-        # of its value.
         stored_files = FileStorage.objects.filter(
-            filename=filename, owner=request.user).values()
+            filename=filename, owner=request.user)
         # filename and owner are 'unique together', we can have either 0 or 1
         # objects in 'stored_files'.
         if len(stored_files) == 0:
@@ -845,17 +876,11 @@
             # of a file owner was not yet implemented, return non-owned files
             # with this filename at a last resort option.
             stored_files = FileStorage.objects.filter(
-                filename=filename, owner=None).values()
+                filename=filename, owner=None)
             if len(stored_files) == 0:
                 raise Http404
         stored_file = stored_files[0]
-        # Emit the json for this object manually because, no matter what the
-        # piston documentation says, once an type is associated with a list
-        # of fields by piston's typemapper mechanism, there is no way to
-        # override that in a specific handler with 'fields' or 'exclude'.
-        emitter = JSONEmitter(
-            stored_file, typemapper, None, DISPLAYED_FILES_FIELDS_OBJECT)
-        stream = emitter.render(request)
+        stream = json_file_storage(stored_file, request)
         return HttpResponse(
             stream, mimetype='application/json; charset=utf-8',
             status=httplib.OK)
@@ -881,7 +906,10 @@
     create = read = update = delete = None
     anonymous = AnonFilesHandler
 
-    get = operation(idempotent=True, exported_as='get')(get_file_by_name)
+    get_by_name = operation(
+        idempotent=True, exported_as='get')(get_file_by_name)
+    get_by_key = operation(
+        idempotent=True, exported_as='get_by_key')(get_file_by_key)
 
     @operation(idempotent=False)
     def add(self, request):

=== modified file 'src/maasserver/models/filestorage.py'
--- src/maasserver/models/filestorage.py	2013-02-18 16:12:22 +0000
+++ src/maasserver/models/filestorage.py	2013-02-18 16:12:22 +0000
@@ -15,9 +15,11 @@
     ]
 
 
+from urllib import urlencode
 from uuid import uuid1
 
 from django.contrib.auth.models import User
+from django.core.urlresolvers import reverse
 from django.db.models import (
     CharField,
     ForeignKey,
@@ -94,3 +96,10 @@
 
     def __unicode__(self):
         return self.filename
+
+    @property
+    def anon_url(self):
+        """URI where the content of the file can be retrieved anonymously."""
+        params = {'op': 'get_by_key', 'key': self.key}
+        url = '%s?%s' % (reverse('files_handler'), urlencode(params))
+        return url

=== modified file 'src/maasserver/tests/test_api.py'
--- src/maasserver/tests/test_api.py	2013-02-18 16:12:22 +0000
+++ src/maasserver/tests/test_api.py	2013-02-18 16:12:22 +0000
@@ -2627,8 +2627,8 @@
         storage = factory.make_file_storage()
         response = self.make_API_GET_request("get", storage.filename)
 
+        self.assertEqual(storage.content, response.content)
         self.assertEqual(httplib.OK, response.status_code)
-        self.assertEqual(storage.content, response.content)
 
     def test_get_fetches_the_most_recent_file(self):
         filename = factory.make_name('file')
@@ -2640,6 +2640,20 @@
         self.assertEqual(httplib.OK, response.status_code)
         self.assertEqual(storage.content, response.content)
 
+    def test_get_by_key_works_anonymously(self):
+        storage = factory.make_file_storage()
+        response = self.client.get(
+            self.get_uri('files/'), {'key': storage.key, 'op': 'get_by_key'})
+
+        self.assertEqual(httplib.OK, response.status_code)
+        self.assertEqual(storage.content, response.content)
+
+    def test_anon_url_allows_anonymous_access(self):
+        storage = factory.make_file_storage()
+        response = self.client.get(storage.anon_url)
+        self.assertEqual(httplib.OK, response.status_code)
+        self.assertEqual(storage.content, response.content)
+
     def test_anon_cannot_list_files(self):
         factory.make_file_storage()
         response = self.make_API_GET_request("list")
@@ -2808,15 +2822,16 @@
         filename = factory.make_name("file")
         factory.make_file_storage(filename=filename, owner=None)
         content = sample_binary_data
-        factory.make_file_storage(
+        storage = factory.make_file_storage(
             filename=filename, content=content, owner=self.logged_in_user)
         response = self.client.get(
             reverse('file_handler', args=[filename]))
         parsed_result = json.loads(response.content)
         self.assertEqual(
-            (filename, content),
+            (filename, storage.anon_url, content),
             (
                 parsed_result['filename'],
+                parsed_result['anon_url'],
                 b64decode(parsed_result['content'])
             ))