launchpad-reviewers team mailing list archive

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

Raphaël Badin has proposed merging lp:~rvb/maas/cleanup-shadow into lp:maas.

Commit message:
Remove the "shadow behavior" in the file storage API.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~rvb/maas/cleanup-shadow/+merge/151207

Now that we're planning to add a migration that will make sure each file has an owner, this compatibility trick is not required anymore.

Note: this should be landed once the migration has landed.
-- 
https://code.launchpad.net/~rvb/maas/cleanup-shadow/+merge/151207
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~rvb/maas/cleanup-shadow 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-01 11:48:49 +0000
@@ -855,28 +855,6 @@
     return stream
 
 
-def get_owned_file_or_404(filename, user):
-    """Return the file named 'filename' owned by 'user'.
-
-    If there is no such file, try getting the corresponding unowned file
-    to be compatible with old versions of MAAS where all the files where
-    unowned.
-    """
-    stored_files = FileStorage.objects.filter(
-        filename=filename, owner=user)
-    # filename and owner are 'unique together', we can have either 0 or 1
-    # objects in 'stored_files'.
-    if len(stored_files) == 0:
-        # In order to support upgrading from installations where the notion
-        # 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)
-        if len(stored_files) == 0:
-            raise Http404
-    return stored_files[0]
-
-
 class FileHandler(OperationsHandler):
     """Manage a FileStorage object.
 
@@ -891,8 +869,8 @@
 
         The 'content' of the file is base64-encoded."""
         try:
-            stored_file = get_owned_file_or_404(
-                filename=filename, user=request.user)
+            stored_file = get_object_or_404(FileStorage,
+                filename=filename, owner=request.user)
         except Http404:
             # In order to fix bug 1123986 we need to distinguish between
             # a 404 returned when the file is not present and a 404 returned
@@ -909,8 +887,8 @@
     @operation(idempotent=False)
     def delete(self, request, filename):
         """Delete a FileStorage object."""
-        stored_file = get_owned_file_or_404(
-            filename=filename, user=request.user)
+        stored_file = get_object_or_404(FileStorage,
+            filename=filename, owner=request.user)
         stored_file.delete()
         return rc.DELETED
 

=== modified file 'src/maasserver/tests/test_api.py'
--- src/maasserver/tests/test_api.py	2013-02-25 15:00:20 +0000
+++ src/maasserver/tests/test_api.py	2013-03-01 11:48:49 +0000
@@ -46,10 +46,7 @@
     reverse,
     get_script_prefix,
     )
-from django.http import (
-    Http404,
-    QueryDict,
-    )
+from django.http import QueryDict
 from django.test.client import RequestFactory
 from django.utils.http import urlquote_plus
 from fixtures import (
@@ -65,7 +62,6 @@
     DISPLAYED_NODEGROUP_FIELDS,
     extract_constraints,
     find_nodegroup_for_pxeconfig_request,
-    get_owned_file_or_404,
     store_node_power_parameters,
     )
 from maasserver.enum import (
@@ -2625,36 +2621,6 @@
         return self.client.get(self.get_uri('files/'), params)
 
 
-class CompatibilityUtilitiesTest(TestCase):
-
-    def test_get_owned_file_or_404_returns_owned_file(self):
-        user = factory.make_user()
-        filename = factory.make_name('filename')
-        factory.make_file_storage(filename=filename, owner=None)
-        filestorage = factory.make_file_storage(filename=filename, owner=user)
-
-        self.assertEqual(filestorage, get_owned_file_or_404(filename, user))
-
-    def test_get_owned_file_or_404_fallsback_to_nonowned_file(self):
-        user = factory.make_user()
-        filename = factory.make_name('filename')
-        factory.make_file_storage(
-            filename=factory.make_name('filename'), owner=user)
-        filestorage = factory.make_file_storage(filename=filename, owner=None)
-
-        self.assertEqual(filestorage, get_owned_file_or_404(filename, user))
-
-    def test_get_owned_file_or_404_raises_404_if_no_file(self):
-        user = factory.make_user()
-        filename = factory.make_name('filename')
-        factory.make_file_storage(
-            filename=factory.make_name('filename'), owner=user)
-        factory.make_file_storage(
-            filename=filename, owner=factory.make_user())
-
-        self.assertRaises(Http404, get_owned_file_or_404, filename, user)
-
-
 class AnonymousFileStorageAPITest(FileStorageAPITestMixin, AnonAPITestCase):
 
     def test_get_works_anonymously(self):
@@ -2869,24 +2835,6 @@
                 b64decode(parsed_result['content'])
             ))
 
-    def test_get_file_returns_file_object_with_anon_owner(self):
-        # To be compatible with the MAAS provider present in Juju as of
-        # revision 616 (lp:juju), reading a file returns the non-owned files
-        # if no owned file can be found.
-        filename = factory.make_name("file")
-        content = sample_binary_data
-        factory.make_file_storage(
-            filename=filename, content=content, owner=None)
-        response = self.client.get(
-            reverse('file_handler', args=[filename]))
-        parsed_result = json.loads(response.content)
-        self.assertEqual(
-            (filename, content),
-            (
-                parsed_result['filename'],
-                b64decode(parsed_result['content'])
-            ))
-
     def test_get_file_returning_404_file_includes_header(self):
         # In order to fix bug 1123986 we need to distinguish between
         # a 404 returned when the file is not present and a 404 returned
@@ -2925,19 +2873,6 @@
         files = FileStorage.objects.filter(filename=filename)
         self.assertEqual([], list(files))
 
-    def test_delete_file_deletes_file_with_anon_owner(self):
-        # To be compatible with the MAAS provider present in Juju as of
-        # revision 616 (lp:juju), deleting a file deletes the non-owned file
-        # if no owned file can be found.
-        filename = factory.make_name('file')
-        factory.make_file_storage(
-            filename=filename, content=b"test content", owner=None)
-        response = self.client.delete(
-            reverse('file_handler', args=[filename]))
-        self.assertEqual(httplib.NO_CONTENT, response.status_code)
-        files = FileStorage.objects.filter(filename=filename)
-        self.assertEqual([], list(files))
-
 
 class TestTagAPI(APITestCase):
     """Tests for /api/1.0/tags/<tagname>/."""