Repository: Duke-GCB/DukeDSClient

Issue:
Empty files fail to upload to DukeDS S3 backend
This results in an error like `Failed to send file to external store. Error:400 https...`.
This error occurs when uploading the file to the S3 backend.
The text response from the exception:
```
<Error><Code>InvalidPartSizeZero</Code><Message>The upload part size cannot be zero.</Message><Resource...
```

Discussion:

Patch:
diff --git a/ddsc/core/ddsapi.py b/ddsc/core/ddsapi.py
index 0ec7f94..d0dd066 100644
--- a/ddsc/core/ddsapi.py
+++ b/ddsc/core/ddsapi.py
@@ -470,7 +470,7 @@ class DataServiceApi(object):
         return self._get_collection(url_prefix, data)
 
     def create_upload(self, project_id, filename, content_type, size,
-                      hash_value, hash_alg, storage_provider_id=None):
+                      hash_value, hash_alg, storage_provider_id=None, chunked=True):
         """
         Post to /projects/{project_id}/uploads to create a uuid for uploading chunks.
         NOTE: The optional hash_value and hash_alg parameters are being removed from the DukeDS API.
@@ -481,6 +481,8 @@ class DataServiceApi(object):
         :param hash_value: str hash value of the entire file
         :param hash_alg: str algorithm used to create hash_value
         :param storage_provider_id: str optional storage provider id
+        :param chunked: is the uploaded file made up of multiple chunks. When False a single upload url is returned.
+            For more see https://github.com/Duke-Translational-Bioinformatics/duke-data-service/blob/develop/api_design/DDS-1182-nonchucked_upload_api_design.md
         :return: requests.Response containing the successful result
         """
         data = {
@@ -491,6 +493,7 @@ class DataServiceApi(object):
                 "value": hash_value,
                 "algorithm": hash_alg
             },
+            "chunked": chunked,
         }
         if storage_provider_id:
             data['storage_provider'] = {'id': storage_provider_id}
diff --git a/ddsc/core/fileuploader.py b/ddsc/core/fileuploader.py
index 0a64039..fb7e3e7 100644
--- a/ddsc/core/fileuploader.py
+++ b/ddsc/core/fileuploader.py
@@ -97,14 +97,16 @@ class FileUploadOperations(object):
         self.data_service = data_service
         self.waiting_monitor = waiting_monitor
 
-    def create_upload(self, project_id, path_data, hash_data, remote_filename=None, storage_provider_id=None):
+    def _create_upload(self, project_id, path_data, hash_data, remote_filename=None, storage_provider_id=None,
+                       chunked=True):
         """
-        Create upload so we can send call further methods.
+        Create upload for uploading multiple chunks or the non-chunked variety (includes upload url).
         :param project_id: str: uuid of the project
         :param path_data: PathData: holds file system data about the file we are uploading
         :param hash_data: HashData: contains hash alg and value for the file we are uploading
         :param remote_filename: str: name to use for our remote file (defaults to path_data basename otherwise)
         :param storage_provider_id: str: optional storage provider id
+        :param chunked: bool: should we create a chunked upload
         :return: str: uuid for the upload
         """
         if not remote_filename:
@@ -115,10 +117,42 @@ class FileUploadOperations(object):
         def func():
             return self.data_service.create_upload(project_id, remote_filename, mime_type, size,
                                                    hash_data.value, hash_data.alg,
-                                                   storage_provider_id=storage_provider_id)
+                                                   storage_provider_id=storage_provider_id,
+                                                   chunked=chunked)
 
         resp = retry_until_resource_is_consistent(func, self.waiting_monitor)
-        return resp.json()['id']
+        return resp.json()
+
+    def create_upload(self, project_id, path_data, hash_data, remote_filename=None, storage_provider_id=None):
+        """
+        Create a chunked upload id to pass to create_file_chunk_url to create upload urls.
+        :param project_id: str: uuid of the project
+        :param path_data: PathData: holds file system data about the file we are uploading
+        :param hash_data: HashData: contains hash alg and value for the file we are uploading
+        :param remote_filename: str: name to use for our remote file (defaults to path_data basename otherwise)
+        :param storage_provider_id: str: optional storage provider id
+        :return: str: uuid for the upload
+        """
+        upload_response = self._create_upload(project_id, path_data, hash_data, remote_filename=remote_filename,
+                                              storage_provider_id=storage_provider_id, chunked=True)
+        return upload_response['id']
+
+    def create_upload_and_chunk_url(self, project_id, path_data, hash_data, remote_filename=None,
+                                    storage_provider_id=None):
+        """
+        Create an non-chunked upload that returns upload id and upload url. This type of upload doesn't allow
+        additional upload urls. For single chunk files this method is more efficient than
+        create_upload/create_file_chunk_url.
+        :param project_id: str: uuid of the project
+        :param path_data: PathData: holds file system data about the file we are uploading
+        :param hash_data: HashData: contains hash alg and value for the file we are uploading
+        :param remote_filename: str: name to use for our remote file (defaults to path_data basename otherwise)
+        :param storage_provider_id:str: optional storage provider id
+        :return: str, dict: uuid for the upload, upload chunk url dict
+        """
+        upload_response = self._create_upload(project_id, path_data, hash_data, remote_filename=remote_filename,
+                                              storage_provider_id=storage_provider_id, chunked=False)
+        return upload_response['id'], upload_response['signed_url']
 
     def create_file_chunk_url(self, upload_id, chunk_num, chunk):
         """
diff --git a/ddsc/core/projectuploader.py b/ddsc/core/projectuploader.py
index 0e577a1..8873288 100644
--- a/ddsc/core/projectuploader.py
+++ b/ddsc/core/projectuploader.py
@@ -402,15 +402,13 @@ def create_small_file(upload_context):
     parent_data, path_data, remote_file_id = upload_context.params
 
     # The small file will fit into one chunk so read into memory and hash it.
-    chunk_num = 0
     chunk = path_data.read_whole_file()
     hash_data = path_data.get_hash()
 
     # Talk to data service uploading chunk and creating the file.
     upload_operations = FileUploadOperations(data_service, upload_context)
-    upload_id = upload_operations.create_upload(upload_context.project_id, path_data, hash_data,
-                                                storage_provider_id=upload_context.config.storage_provider_id)
-    url_info = upload_operations.create_file_chunk_url(upload_id, chunk_num, chunk)
+    upload_id, url_info = upload_operations.create_upload_and_chunk_url(
+        upload_context.project_id, path_data, hash_data, storage_provider_id=upload_context.config.storage_provider_id)
     upload_operations.send_file_external(url_info, chunk)
     return upload_operations.finish_upload(upload_id, hash_data, parent_data, remote_file_id)
 


Test patch:
diff --git a/ddsc/core/tests/test_ddsapi.py b/ddsc/core/tests/test_ddsapi.py
index eb51af9..1f093d5 100644
--- a/ddsc/core/tests/test_ddsapi.py
+++ b/ddsc/core/tests/test_ddsapi.py
@@ -720,6 +720,31 @@ class TestDataServiceApi(TestCase):
                 "value": "somehash",
                 "algorithm": "md5"
             },
+            "chunked": True,
+            "storage_provider": {"id": "abc456"}
+        })
+        mock_requests.post.assert_called_with('something.com/v1/projects/123/uploads',
+                                              expected_data,
+                                              headers=ANY)
+
+    def test_create_non_chunked_upload(self):
+        mock_requests = MagicMock()
+        api = DataServiceApi(auth=self.create_mock_auth(config_page_size=100),
+                             url="something.com/v1",
+                             http=mock_requests)
+        mock_requests.post.return_value = fake_response(status_code=201, json_return_value={})
+        api.create_upload(project_id='123', filename='data.txt', content_type='sometype', size=10,
+                          hash_value='somehash', hash_alg='md5', storage_provider_id='abc456',
+                          chunked=False)
+        expected_data = json.dumps({
+            "name": "data.txt",
+            "content_type": "sometype",
+            "size": 10,
+            "hash": {
+                "value": "somehash",
+                "algorithm": "md5"
+            },
+            "chunked": False,
             "storage_provider": {"id": "abc456"}
         })
         mock_requests.post.assert_called_with('something.com/v1/projects/123/uploads',
diff --git a/ddsc/core/tests/test_fileuploader.py b/ddsc/core/tests/test_fileuploader.py
index 02f33a9..51043ba 100644
--- a/ddsc/core/tests/test_fileuploader.py
+++ b/ddsc/core/tests/test_fileuploader.py
@@ -228,11 +228,13 @@ class TestFileUploadOperations(TestCase):
         fop = FileUploadOperations(data_service, MagicMock())
         path_data = MagicMock()
         path_data.name.return_value = 'data.dat'
-        upload_id = fop.create_upload(project_id='12', path_data=path_data, hash_data=MagicMock(), remote_filename='other.dat')
+        upload_id = fop.create_upload(project_id='12', path_data=path_data, hash_data=MagicMock(),
+                                      remote_filename='other.dat')
         self.assertEqual(upload_id, '123')
         args, kwargs = data_service.create_upload.call_args
         self.assertEqual(args[0], '12')
         self.assertEqual(args[1], 'other.dat')
+        self.assertEqual(kwargs['chunked'], True)
 
     @patch('ddsc.core.fileuploader.HashData')
     def test_create_file_chunk_url_uses_one_based_indexing(self, hash_data):
@@ -256,3 +258,28 @@ class TestFileUploadOperations(TestCase):
                                                           4,       # chunk_len
                                                           'h@sh',  # hash_value
                                                           'md5')   # hash_alg
+
+    def test_create_upload_and_chunk_url(self):
+        data_service = MagicMock()
+        response = Mock()
+        response.json.return_value = {'id': '123', 'signed_url': {
+            "http_verb": "PUT",
+            "host": "duke_data_service_prod.s3.amazonaws.com",
+        }}
+        data_service.create_upload.side_effect = [
+            response
+        ]
+        fop = FileUploadOperations(data_service, MagicMock())
+        path_data = MagicMock()
+        path_data.name.return_value = 'data.dat'
+        upload_id, upload_chunk_url = fop.create_upload_and_chunk_url(
+            project_id='12', path_data=path_data, hash_data=MagicMock(), remote_filename='other.dat')
+        self.assertEqual(upload_id, '123')
+        args, kwargs = data_service.create_upload.call_args
+        self.assertEqual(args[0], '12')
+        self.assertEqual(args[1], 'other.dat')
+        self.assertEqual(kwargs['chunked'], False)
+        self.assertEqual(upload_chunk_url, {
+            "http_verb": "PUT",
+            "host": "duke_data_service_prod.s3.amazonaws.com",
+        })
diff --git a/ddsc/core/tests/test_projectuploader.py b/ddsc/core/tests/test_projectuploader.py
index 0b9294b..a543847 100644
--- a/ddsc/core/tests/test_projectuploader.py
+++ b/ddsc/core/tests/test_projectuploader.py
@@ -147,18 +147,16 @@ class TestCreateProjectCommand(TestCase):
 
 
 class TestCreateSmallFile(TestCase):
-    @patch('ddsc.core.projectuploader.FileUploadOperations')
+    @patch('ddsc.core.projectuploader.FileUploadOperations', autospec=True)
     def test_create_small_file_passes_zero_index(self, mock_file_operations):
         mock_path_data = Mock()
         mock_path_data.read_whole_file.return_value = 'data'
-        mock_file_operations.return_value.create_upload.return_value = 'someId'
+        mock_file_operations.return_value.create_upload_and_chunk_url.return_value = (
+            'someId', {'host': 'somehost', 'url': 'someurl'}
+        )
 
         upload_context = Mock(params=(Mock(), mock_path_data, Mock()))
         resp = create_small_file(upload_context)
 
         self.assertEqual(resp, mock_file_operations.return_value.finish_upload.return_value)
-        mock_file_operations.return_value.create_file_chunk_url.assert_called_with(
-            'someId',
-            0,  # zero based index passed to create_file_chunk_url
-            'data'
-        )
+        mock_file_operations.return_value.create_file_chunk_url.assert_not_called()