Merge pull request #996 from scitran/empty-upload

400 on empty file uid|label|reaper upload

api/placer.py

@@ -18,6 +18,7 @@ from .jobs import rules
 from .jobs.jobs import Job
 from .types import Origin
 from .web import encoder
+from .web.errors import FileFormException
 
 class Placer(object):
     """
@@ -74,7 +75,7 @@ class Placer(object):
         Helper function that throws unless metadata was provided.
         """
         if self.metadata == None:
-            raise Exception('Metadata required')
+            raise FileFormException('Metadata required')
 
     def save_file(self, field=None, file_attrs=None):
         """
@@ -140,7 +141,7 @@ class UIDPlacer(Placer):
         super(UIDPlacer, self).__init__(container_type, container, id_, metadata, timestamp, origin, context)
         self.metadata_for_file = {}
         self.session_id = None
-
+        self.count = 0
 
     def check(self):
         self.requireMetadata()
@@ -149,22 +150,25 @@ class UIDPlacer(Placer):
         metadata_validator = validators.from_schema_path(payload_schema_uri)
         metadata_validator(self.metadata, 'POST')
 
-        # If not a superuser request, pass uid of user making the upload request
-        targets = self.create_hierarchy(self.metadata, type_=self.match_type, user=self.context.get('uid'))
-
-        self.metadata_for_file = {}
-
-        for target in targets:
-            if target[0].level is 'session':
-                self.session_id = target[0].id_
-            for name in target[1]:
-                self.metadata_for_file[name] = {
-                    'container': target[0],
-                    'metadata': target[1][name]
-                }
 
 
     def process_file_field(self, field, file_attrs):
+        # Only create the hierarchy once
+        if self.count == 0:
+            # If not a superuser request, pass uid of user making the upload request
+            targets = self.create_hierarchy(self.metadata, type_=self.match_type, user=self.context.get('uid'))
+
+            self.metadata_for_file = {}
+
+            for target in targets:
+                if target[0].level is 'session':
+                    self.session_id = target[0].id_
+                for name in target[1]:
+                    self.metadata_for_file[name] = {
+                        'container': target[0],
+                        'metadata': target[1][name]
+                    }
+        self.count += 1
 
         # For the file, given self.targets, choose a target
         name        = field.filename
@@ -194,6 +198,9 @@ class UIDPlacer(Placer):
         self.saved.append(file_attrs)
 
     def finalize(self):
+        # Check that there is at least one file being uploaded
+        if self.count < 1:
+            raise FileFormException("No files selected for upload")
         if self.session_id:
             self.container_type = 'session'
             self.id_ = self.session_id

api/upload.py

@@ -5,7 +5,7 @@ import os.path
 import shutil
 
 from .web import base
-from .web.errors import FileStoreException
+from .web.errors import FileStoreException, FileFormException
 from . import config
 from . import files
 from . import placer as pl
@@ -95,7 +95,8 @@ def process_upload(request, strategy, container_type=None, id_=None, origin=None
     # TODO: Change schemas to enabled targeted uploads of more than one file.
     # Ref docs from placer.TargetedPlacer for details.
     if strategy == Strategy.targeted and len(file_fields) > 1:
-        raise Exception("Targeted uploads can only send one file")
+        raise FileFormException("Targeted uploads can only send one file")
+
 
 
     for field in file_fields:

api/web/base.py

@@ -358,6 +358,8 @@ class RequestHandler(webapp2.RequestHandler):
             code = 400
         elif isinstance(exception, errors.FileFormException):
             code = 400
+        elif isinstance(exception, errors.FileFormException):
+            code = 400
         elif isinstance(exception, ElasticsearchException):
             code = 503
             message = "Search is currently down. Try again later."

tests/integration_tests/python/test_uploads.py

@@ -55,6 +55,20 @@ def test_reaper_upload(data_builder, randstr, upload_file_form, as_admin):
     ))
     assert r.ok
 
+    # reaper-upload files to group_1/project_label_1 using session_uid without any files
+    file_form = upload_file_form(
+        group={'_id': group_1},
+        project={'label': project_label_1, "files":[]},
+        session={'uid': session_uid+"1", "files":[], 'subject': {
+                    'code': prefix + '-subject-code',
+                    'files': []
+                }}
+    )
+    print file_form
+    r = as_admin.post('/upload/reaper', files={"metadata": file_form.get("metadata")})
+    print r.json()
+    assert r.status_code == 400
+
     # get session created by the upload
     project_1 = as_admin.get('/groups/' + group_1 + '/projects').json()[0]['_id']
     session = as_admin.get('/projects/' + project_1 + '/sessions').json()[0]['_id']
@@ -173,6 +187,25 @@ def test_reaper_upload_unknown_group_project(data_builder, file_form, as_root, a
     )
     assert r.ok
 
+    # Try uploading 0 files
+    r = as_root.post('/upload/label', files={"metadata":file_form(
+        'acquisition.csv',
+        meta={
+            'group': {'_id': 'not_a_real_group'},
+            'project': {
+                'label': 'new_project',
+            },
+            'session': {
+                'label': 'test_session_label',
+            },
+            'acquisition': {
+                'label': 'test_acquisition_label',
+                'files': [{'name': 'acquisition.csv'}]
+            }
+        }).get("metadata")}
+    )
+    assert r.status_code == 400
+
 
     # get session created by the upload
     r = as_root.get('/groups/unknown/projects')
@@ -362,7 +395,7 @@ def test_uid_upload(data_builder, file_form, as_admin, as_user, as_public):
 
     # try to uid-upload w/o metadata
     r = as_admin.post('/upload/uid', files=file_form('test.csv'))
-    assert r.status_code == 500
+    assert r.status_code == 400
 
     # NOTE unused.csv is testing code that discards files not referenced from meta
     uid_files = ('project.csv', 'subject.csv', 'session.csv', 'acquisition.csv', 'unused.csv')
@@ -390,6 +423,10 @@ def test_uid_upload(data_builder, file_form, as_admin, as_user, as_public):
     r = as_user.post('/upload/uid', files=file_form(*uid_files, meta=uid_meta))
     assert r.status_code == 403
 
+    # try to uid-upload no files
+    r = as_admin.post('/upload/uid', files={"metadata": file_form(*uid_files, meta=uid_meta).get("metadata")})
+    assert r.status_code == 400
+
     # uid-upload files
     r = as_admin.post('/upload/uid', files=file_form(*uid_files, meta=uid_meta))
     assert r.ok