launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #15189
[Merge] lp:~rvb/maas/bug-1123986-api-fix into lp:maas
Raphaël Badin has proposed merging lp:~rvb/maas/bug-1123986-api-fix into lp:maas with lp:~rvb/maas/bug-1123986-db-filestorage-key as a prerequisite.
Commit message:
Adapt the file storage API to the 'owner' field on FileStorage objects.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~rvb/maas/bug-1123986-api-fix/+merge/149059
--
https://code.launchpad.net/~rvb/maas/bug-1123986-api-fix/+merge/149059
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~rvb/maas/bug-1123986-api-fix into lp:maas.
=== modified file 'src/maasserver/api.py'
--- src/maasserver/api.py 2013-02-14 14:25:39 +0000
+++ src/maasserver/api.py 2013-02-18 14:30:33 +0000
@@ -777,18 +777,16 @@
return ('node_mac_handler', [node_system_id, mac_address])
-def get_file(handler, request):
+def get_file_by_name(handler, request):
"""Get a named file from the file storage.
:param filename: The exact name of the file you want to get.
:type filename: string
:return: The file is returned in the response content.
"""
- filename = request.GET.get("filename", None)
- if not filename:
- raise MAASAPIBadRequest("Filename not supplied")
+ filename = get_mandatory_param(request.GET, 'filename')
try:
- db_file = FileStorage.objects.get(filename=filename)
+ db_file = FileStorage.objects.filter(filename=filename).latest('id')
except FileStorage.DoesNotExist:
raise MAASAPINotFound("File not found")
return HttpResponse(db_file.content, status=httplib.OK)
@@ -808,7 +806,7 @@
"""
create = read = update = delete = None
- get = operation(idempotent=True, exported_as='get')(get_file)
+ get = operation(idempotent=True, exported_as='get')(get_file_by_name)
@classmethod
def resource_uri(cls, *args, **kwargs):
@@ -826,7 +824,7 @@
class FileHandler(OperationsHandler):
"""Manage a FileStorage object.
- The file is identified by its filename.
+ The file is identified by its filename and owner.
"""
model = FileStorage
fields = DISPLAYED_FILES_FIELDS
@@ -838,11 +836,18 @@
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).values()
+ stored_files = FileStorage.objects.filter(
+ filename=filename, owner=request.user).values()
# filename is a 'unique' field, we can have either 0 or 1 objects in
# 'stored_files'.
if len(stored_files) == 0:
- raise Http404
+ # 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).values()
+ 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
@@ -858,7 +863,8 @@
@operation(idempotent=False)
def delete(self, request, filename):
"""Delete a FileStorage object."""
- stored_file = get_object_or_404(FileStorage, filename=filename)
+ stored_file = get_object_or_404(
+ FileStorage, filename=filename, owner=request.user)
stored_file.delete()
return rc.DELETED
@@ -875,7 +881,7 @@
create = read = update = delete = None
anonymous = AnonFilesHandler
- get = operation(idempotent=True, exported_as='get')(get_file)
+ get = operation(idempotent=True, exported_as='get')(get_file_by_name)
@operation(idempotent=False)
def add(self, request):
@@ -899,7 +905,7 @@
# As per the comment in FileStorage, this ought to deal in
# chunks instead of reading the file into memory, but large
# files are not expected.
- FileStorage.objects.save_file(filename, uploaded_file)
+ FileStorage.objects.save_file(filename, uploaded_file, request.user)
return HttpResponse('', status=httplib.CREATED)
@operation(idempotent=True)
@@ -913,7 +919,7 @@
:type prefix: string
"""
prefix = request.GET.get("prefix", None)
- files = FileStorage.objects.all()
+ files = FileStorage.objects.filter(owner=request.user)
if prefix is not None:
files = files.filter(filename__startswith=prefix)
files = files.order_by('filename')
=== modified file 'src/maasserver/models/filestorage.py'
--- src/maasserver/models/filestorage.py 2013-02-18 14:30:33 +0000
+++ src/maasserver/models/filestorage.py 2013-02-18 14:30:33 +0000
@@ -47,17 +47,17 @@
old file will continue without iterruption.
"""
- def save_file(self, filename, file_object):
+ def save_file(self, filename, file_object, owner):
"""Save the file to the database.
- If a file of that name already existed, it will be replaced by the
- new contents.
+ If a file of that name/owner already existed, it will be replaced by
+ the new contents.
"""
# This probably ought to read in chunks but large files are
# not expected.
content = Bin(file_object.read())
storage, created = self.get_or_create(
- filename=filename, defaults={'content': content})
+ filename=filename, owner=owner, defaults={'content': content})
if not created:
storage.content = content
storage.save()
=== modified file 'src/maasserver/testing/factory.py'
--- src/maasserver/testing/factory.py 2012-12-19 12:31:11 +0000
+++ src/maasserver/testing/factory.py 2013-02-18 14:30:33 +0000
@@ -330,9 +330,9 @@
admin.save()
return admin
- def make_file_storage(self, filename=None, content=None):
+ def make_file_storage(self, filename=None, content=None, owner=None):
fake_file = self.make_file_upload(filename, content)
- return FileStorage.objects.save_file(fake_file.name, fake_file)
+ return FileStorage.objects.save_file(fake_file.name, fake_file, owner)
def make_oauth_header(self, **kwargs):
"""Fake an OAuth authorization header.
=== modified file 'src/maasserver/tests/test_api.py'
--- src/maasserver/tests/test_api.py 2013-02-14 12:42:30 +0000
+++ src/maasserver/tests/test_api.py 2013-02-18 14:30:33 +0000
@@ -2624,34 +2624,38 @@
class AnonymousFileStorageAPITest(FileStorageAPITestMixin, AnonAPITestCase):
def test_get_works_anonymously(self):
- factory.make_file_storage(
- filename="foofilers", content=b"give me rope")
- response = self.make_API_GET_request("get", "foofilers")
-
- self.assertEqual(httplib.OK, response.status_code)
- self.assertEqual(b"give me rope", response.content)
+ storage = factory.make_file_storage()
+ response = self.make_API_GET_request("get", storage.filename)
+
+ 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')
+ factory.make_file_storage(filename=filename, owner=factory.make_user())
+ storage = factory.make_file_storage(
+ filename=filename, owner=factory.make_user())
+ response = self.make_API_GET_request("get", filename)
+
+ self.assertEqual(httplib.OK, response.status_code)
+ self.assertEqual(storage.content, response.content)
def test_anon_cannot_list_files(self):
- factory.make_file_storage(
- filename="filename", content=b"test content")
+ factory.make_file_storage()
response = self.make_API_GET_request("list")
# The 'list' operation is not available to anon users.
self.assertEqual(httplib.BAD_REQUEST, response.status_code)
def test_anon_cannot_get_file(self):
- filename = factory.make_name("file")
- factory.make_file_storage(
- filename=filename, content=b"test file content")
+ storage = factory.make_file_storage()
response = self.client.get(
- reverse('file_handler', args=[filename]))
+ reverse('file_handler', args=[storage.filename]))
self.assertEqual(httplib.UNAUTHORIZED, response.status_code)
def test_anon_cannot_delete_file(self):
- filename = factory.make_name('file')
- factory.make_file_storage(
- filename=filename, content=b"test content")
+ storage = factory.make_file_storage()
response = self.client.delete(
- reverse('file_handler', args=[filename]))
+ reverse('file_handler', args=[storage.filename]))
self.assertEqual(httplib.UNAUTHORIZED, response.status_code)
@@ -2721,7 +2725,7 @@
self.assertEqual(httplib.BAD_REQUEST, response.status_code)
self.assertIn('text/plain', response['Content-Type'])
- self.assertEqual("Filename not supplied", response.content)
+ self.assertEqual("No provided filename!", response.content)
def test_get_file_fails_with_missing_file(self):
response = self.make_API_GET_request("get", filename="missingfilename")
@@ -2734,19 +2738,28 @@
filenames = ["myfiles/a", "myfiles/z", "myfiles/b"]
for filename in filenames:
factory.make_file_storage(
- filename=filename, content=b"test content")
+ filename=filename, content=b"test content",
+ owner=self.logged_in_user)
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_filters_by_owner(self):
+ factory.make_file_storage(owner=factory.make_user())
+ response = self.make_API_GET_request("list")
+ self.assertEqual(httplib.OK, response.status_code)
+ parsed_results = json.loads(response.content)
+ self.assertEqual([], parsed_results)
+
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")
+ filename=filename, content=b"test content",
+ owner=self.logged_in_user)
response = self.client.get(
self.get_uri('files/'), {"op": "list", "prefix": "prefix-"})
self.assertEqual(httplib.OK, response.status_code)
@@ -2756,7 +2769,8 @@
def test_list_files_does_not_include_file_content(self):
factory.make_file_storage(
- filename="filename", content=b"test content")
+ filename="filename", content=b"test content",
+ owner=self.logged_in_user)
response = self.make_API_GET_request("list")
parsed_results = json.loads(response.content)
self.assertNotIn('content', parsed_results[0].keys())
@@ -2764,7 +2778,8 @@
def test_files_resource_uri_are_url_escaped(self):
filename = "&*!c/%//filename/"
factory.make_file_storage(
- filename=filename, content=b"test content")
+ 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']
@@ -2775,21 +2790,67 @@
def test_get_file_returns_file_object_with_content_base64_encoded(self):
filename = factory.make_name("file")
content = sample_binary_data
- factory.make_file_storage(filename=filename, content=content)
- 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'])
- ))
+ 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),
+ (
+ parsed_result['filename'],
+ b64decode(parsed_result['content'])
+ ))
+
+ def test_get_file_returns_owned_file(self):
+ # If both an owned file and a non-owned file are present (with the
+ # same name), the owned file is returned.
+ filename = factory.make_name("file")
+ factory.make_file_storage(filename=filename, owner=None)
+ 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_returns_file_object_with_anon_owner(self):
+ # To get 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_delete_filters_by_owner(self):
+ storage = factory.make_file_storage(owner=factory.make_user())
+ response = self.client.delete(
+ reverse('file_handler', args=[storage.filename]))
+ self.assertEqual(httplib.NOT_FOUND, response.status_code)
+ files = FileStorage.objects.filter(filename=storage.filename)
+ self.assertEqual([storage], list(files))
def test_delete_file_deletes_file(self):
filename = factory.make_name('file')
factory.make_file_storage(
- filename=filename, content=b"test content")
+ filename=filename, content=b"test content",
+ owner=self.logged_in_user)
response = self.client.delete(
reverse('file_handler', args=[filename]))
self.assertEqual(httplib.NO_CONTENT, response.status_code)
=== modified file 'src/maasserver/tests/test_filestorage.py'
--- src/maasserver/tests/test_filestorage.py 2013-02-18 14:30:33 +0000
+++ src/maasserver/tests/test_filestorage.py 2013-02-18 14:30:33 +0000
@@ -40,10 +40,12 @@
def test_save_file_creates_storage(self):
filename = factory.getRandomString()
content = self.make_data()
- storage = FileStorage.objects.save_file(filename, BytesIO(content))
+ user = factory.make_user()
+ storage = FileStorage.objects.save_file(
+ filename, BytesIO(content), user)
self.assertEqual(
- (filename, content),
- (storage.filename, storage.content))
+ (filename, content, user),
+ (storage.filename, storage.content, storage.owner))
def test_storage_can_be_retrieved(self):
filename = factory.getRandomString()
Follow ups
