Code review comment for lp:~rvb/maas/bug-1123986-api-fix

Revision history for this message
Gavin Panella (allenap) wrote :

Looks good, but I have a couple of questions.

[1]

-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')

Shouldn't this restrict itself to files without an owner?

[2]

     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()

Should this fall back to deleting files without owner when an
owned file is not found?

review: Needs Information

« Back to merge proposal