Refactor file upload methods to accept Path-like objects (#116)

* refactor file upload services to use Path-like objects

* fix

* lint

* remove reference to PathBase

* fix code coverage, change KiB to KB

Author: nathanthorpe

.github/workflows/lint.yml

@@ -43,7 +43,6 @@ jobs:
           poetry run flake8 cirro
 
       - name: Run Tests
-        working-directory: tests
         run: |
           poetry run pytest --cov --cov-report=xml --cov-branch -o junit_family=xunit2 --junitxml=junit/test-results.xml
 

cirro/cli/controller.py

@@ -98,7 +98,7 @@ def run_ingest(input_params: UploadArguments, interactive=False):
     logger.info("Uploading files")
     cirro.datasets.upload_files(project_id=project_id,
                                 dataset_id=create_resp.id,
-                                local_directory=directory,
+                                directory=directory,
                                 files=files)
 
     if cirro.configuration.enable_additional_checksum:

cirro/cli/interactive/upload_args.py

@@ -71,7 +71,7 @@ def confirm_data_files(data_directory: str, files: List[str]):
 
     if not ask(
         "confirm",
-        f'Please confirm that you wish to upload {stats["numberOfFiles"]} files ({stats["sizeFriendly"]})'
+        f'Please confirm that you wish to upload {stats.number_of_files} files ({stats.size_friendly})'
     ):
         sys.exit(1)
 

cirro/file_utils.py

@@ -8,7 +8,7 @@
 from botocore.exceptions import ConnectionError
 
 from cirro.clients import S3Client
-from cirro.models.file import DirectoryStatistics, File
+from cirro.models.file import DirectoryStatistics, File, PathLike
 
 if os.name == 'nt':
     import win32api
@@ -82,26 +82,58 @@ def get_files_in_directory(
     return paths
 
 
-def get_files_stats(files: List[Path]) -> DirectoryStatistics:
+def _bytes_to_human_readable(num_bytes: int) -> str:
+    for unit in ['B', 'KB', 'MB', 'GB', 'TB', 'PB']:
+        if num_bytes < 1000.0 or unit == 'PB':
+            break
+        num_bytes /= 1000.0
+    return f"{num_bytes:,.2f} {unit}"
+
+
+def get_files_stats(files: List[PathLike]) -> DirectoryStatistics:
     """
-    @private
+    Returns information about the list of files provided, such as the total size and number of files.
     """
     sizes = [f.stat().st_size for f in files]
-    total_size = sum(sizes) / float(1 << 30)
-    return {
-        'sizeFriendly': f'{total_size:,.3f} GB',
-        'size': total_size,
-        'numberOfFiles': len(sizes)
-    }
-
-
-def upload_directory(directory: str, files: List[str], s3_client: S3Client, bucket: str, prefix: str, max_retries=10):
+    total_size = sum(sizes)
+    return DirectoryStatistics(
+        size_friendly=_bytes_to_human_readable(total_size),
+        size=total_size,
+        number_of_files=len(sizes)
+    )
+
+
+def upload_directory(directory: PathLike,
+                     files: List[PathLike],
+                     s3_client: S3Client,
+                     bucket: str,
+                     prefix: str,
+                     max_retries=10):
     """
     @private
+
+    Uploads a list of files from the specified directory
+    Args:
+        directory (str|Path): Path to directory
+        files (typing.List[str|Path]): List of paths to files within the directory
+            must be the same type as directory.
+        s3_client (cirro.clients.S3Client): S3 client
+        bucket (str): S3 bucket
+        prefix (str): S3 prefix
+        max_retries (int): Number of retries
     """
+    # Ensure all files are of the same type as the directory
+    if not all(isinstance(file, type(directory)) for file in files):
+        raise ValueError("All files must be of the same type as the directory (str or Path)")
+
     for file in files:
-        key = f'{prefix}/{file}'
-        local_path = Path(directory, file)
+        if isinstance(file, str):
+            file_path = Path(directory, file)
+        else:
+            file_path = file
+
+        file_relative = file_path.relative_to(directory).as_posix()
+        key = f'{prefix}/{file_relative}'
         success = False
 
         # Retry up to max_retries times
@@ -110,7 +142,7 @@ def upload_directory(directory: str, files: List[str], s3_client: S3Client, buck
             # Try the upload
             try:
                 s3_client.upload_file(
-                    file_path=local_path,
+                    file_path=file_path,
                     bucket=bucket,
                     key=key
                 )

cirro/models/file.py

@@ -1,16 +1,21 @@
 from dataclasses import dataclass
-from pathlib import PurePath
-from typing import TypedDict, Dict, Optional
+from pathlib import PurePath, Path
+from typing import Dict, Optional, TypeVar, NamedTuple
 
 from cirro_api_client.v1.models import FileAccessRequest, AccessType, FileEntry
 
 from cirro.models.s3_path import S3Path
 
+PathLike = TypeVar('PathLike', str, Path)
 
-class DirectoryStatistics(TypedDict):
+
+class DirectoryStatistics(NamedTuple):
     size: float
-    sizeFriendly: str
-    numberOfFiles: int
+    " Size in bytes"
+    size_friendly: str
+    " Size in user friendly format (e.g. 1.2 KB)"
+    number_of_files: int
+    " Number of files"
 
 
 class FileAccessContext:

cirro/sdk/project.py

@@ -195,7 +195,7 @@ def upload_dataset(
         self._client.datasets.upload_files(
             project_id=self.id,
             dataset_id=create_response.id,
-            local_directory=upload_folder,
+            directory=upload_folder,
             files=files
         )
 

cirro/services/dataset.py

@@ -5,7 +5,7 @@
 from cirro_api_client.v1.models import ImportDataRequest, UploadDatasetRequest, UpdateDatasetRequest, Dataset, \
     DatasetDetail, CreateResponse, UploadDatasetCreateResponse
 
-from cirro.models.file import FileAccessContext, File
+from cirro.models.file import FileAccessContext, File, PathLike
 from cirro.services.base import get_all_records
 from cirro.services.file import FileEnabledService
 
@@ -166,16 +166,20 @@ def get_file_listing(self, project_id: str, dataset_id: str, file_limit: int = 1
         ]
         return files
 
-    def upload_files(self, project_id: str, dataset_id: str, local_directory: str, files: List[str]) -> None:
+    def upload_files(self,
+                     project_id: str,
+                     dataset_id: str,
+                     directory: PathLike,
+                     files: List[PathLike]) -> None:
         """
-        Uploads files to a given dataset from the specified local directory
+        Uploads files to a given dataset from the specified directory.
 
         Args:
             project_id (str): ID of the Project
             dataset_id (str): ID of the Dataset
-            local_directory (str): Path to local directory
-            files (typing.List[str]): List of relative paths to files within the local directory
-
+            directory (str|Path): Path to directory
+            files (typing.List[str|Path]): List of paths to files within the directory,
+                must be the same type as directory.
         """
         dataset = self.get(project_id, dataset_id)
 
@@ -186,9 +190,9 @@ def upload_files(self, project_id: str, dataset_id: str, local_directory: str, f
         )
 
         self._file_service.upload_files(
-            access_context,
-            local_directory,
-            files
+            access_context=access_context,
+            directory=directory,
+            files=files
         )
 
     def download_files(

cirro/services/file.py

@@ -9,7 +9,7 @@
 
 from cirro.clients.s3 import S3Client
 from cirro.file_utils import upload_directory, download_directory
-from cirro.models.file import FileAccessContext, File
+from cirro.models.file import FileAccessContext, File, PathLike
 from cirro.services.base import BaseService
 
 
@@ -126,14 +126,18 @@ def create_file(self, access_context: FileAccessContext, key: str,
             bucket=access_context.bucket
         )
 
-    def upload_files(self, access_context: FileAccessContext, directory: str, files: List[str]) -> None:
+    def upload_files(self,
+                     access_context: FileAccessContext,
+                     directory: PathLike,
+                     files: List[PathLike]) -> None:
         """
         Uploads a list of files from the specified directory
 
         Args:
             access_context (cirro.models.file.FileAccessContext): File access context, use class methods to generate
-            directory (str): base path to upload from
-            files (List[str]): relative path of files to upload
+            directory (str|Path): Path to directory
+            files (typing.List[str|Path]): List of paths to files within the directory
+                must be the same type as directory.
         """
 
         s3_client = S3Client(

sonar-project.properties

@@ -1,6 +1,6 @@
 sonar.projectKey=CirroBio_Cirro-client
 sonar.organization=cirrobio
 sonar.python.version=3.9
-sonar.python.xunit.reportPath=tests/junit/test-results.xml
-sonar.python.coverage.reportPaths=tests/coverage.xml
+sonar.python.xunit.reportPath=junit/test-results.xml
+sonar.python.coverage.reportPaths=coverage.xml
 sonar.exclusions=**/tests/*.py

tests/test_file_utils.py

@@ -0,0 +1,75 @@
+import unittest
+from pathlib import Path
+from unittest.mock import Mock, call
+
+from cirro.file_utils import upload_directory, get_files_in_directory, get_files_stats
+
+
+class TestFileUtils(unittest.TestCase):
+    def setUp(self):
+        self.mock_s3_client = Mock()
+        self.mock_s3_client.upload_file = Mock()
+        self.test_bucket = 'project-1a1a'
+        self.test_prefix = 'datasets/1a1a/data'
+
+    def test_get_file_stats(self):
+        directory = Path(__file__).parent / 'data'
+        files = get_files_in_directory(directory)
+        files = [Path(directory, f) for f in files]
+        stats = get_files_stats(files=files)
+        self.assertGreater(stats.size, 0)
+        self.assertGreater(stats.number_of_files, 0)
+        self.assertIn('KB', stats.size_friendly)
+
+    def test_upload_directory_pathlike(self):
+        test_path = Path('/Users/test/Documents/dataset1')
+        test_files = [
+            Path('/Users/test/Documents/dataset1/test_file.fastq'),
+            Path('/Users/test/Documents/dataset1/folder1/test_file.fastq'),
+        ]
+        upload_directory(directory=test_path,
+                         files=test_files,
+                         s3_client=self.mock_s3_client,
+                         bucket=self.test_bucket,
+                         prefix=self.test_prefix)
+
+        # The function should upload files relative to the directory path.
+        self.mock_s3_client.upload_file.assert_has_calls([
+            call(file_path=test_files[0], bucket=self.test_bucket, key=f'{self.test_prefix}/test_file.fastq'),
+            call(file_path=test_files[1], bucket=self.test_bucket, key=f'{self.test_prefix}/folder1/test_file.fastq')
+        ], any_order=True)
+
+    def test_upload_directory_string(self):
+        test_path = 'data'
+        test_files = [
+            'file1.txt',
+            'folder1/file2.txt'
+        ]
+        upload_directory(directory=test_path,
+                         files=test_files,
+                         s3_client=self.mock_s3_client,
+                         bucket=self.test_bucket,
+                         prefix=self.test_prefix)
+
+        # The function should upload files relative to the directory path,
+        # but also format file_path into the Path object.
+        self.mock_s3_client.upload_file.assert_has_calls([
+            call(file_path=Path(test_path, test_files[0]),
+                 bucket=self.test_bucket,
+                 key=f'{self.test_prefix}/file1.txt'),
+            call(file_path=Path(test_path, test_files[1]),
+                 bucket=self.test_bucket,
+                 key=f'{self.test_prefix}/folder1/file2.txt')
+        ], any_order=True)
+
+    def test_upload_directory_different_types(self):
+        test_path = Path('s3://bucket/dataset1')
+        test_files = [
+            's3://bucket/dataset1/file2.txt'
+        ]
+        with self.assertRaises(ValueError):
+            upload_directory(directory=test_path,
+                             files=test_files,
+                             s3_client=self.mock_s3_client,
+                             bucket=self.test_bucket,
+                             prefix=self.test_prefix)