Repository: SciCatProject/scitacean

Issue:
Allow uploading a dataset without uploading files
There are cases where it is not possible to upload files with Scitacean because of complicated permissions on the file server. In these cases, it would be useful to only upload the metadata with Scitacean.

This can be done by adding files with `Dataset.add_files` and skipping all files with `local_path=None` in the upload.

To do:

- Better constructor for `File` that doesn't require setting protected members or using a model.
- Proper way of filtering files to upload. Probably needs to look at creation times and checksums, not just the path.

Discussion:

Patch:
diff --git a/docs/_templates/scitacean-class-template.rst b/docs/_templates/scitacean-class-template.rst
index c60a129..45f2086 100644
--- a/docs/_templates/scitacean-class-template.rst
+++ b/docs/_templates/scitacean-class-template.rst
@@ -2,7 +2,7 @@
 
 {% set constructors = {"Client": ["from_credentials", "from_token", "without_login"],
                        "Dataset": ["__init__", "from_download_models"],
-                       "File": ["from_local", "from_download_model"],
+                       "File": ["from_local", "from_remote", "from_download_model"],
                        "OrigDatablockProxy": ["__init__", "from_download_model"],
                        "PID": ["__init__", "parse"],
                        "ScicatClient": ["from_credentials", "from_token", "without_login"],
diff --git a/docs/release-notes.rst b/docs/release-notes.rst
index c4ea34f..3e30ddb 100644
--- a/docs/release-notes.rst
+++ b/docs/release-notes.rst
@@ -52,6 +52,8 @@ Breaking changes
 Bugfixes
 ~~~~~~~~
 
+* :class:`scitacean.Client` no longer requires a file transfer during upload if the dataset has no files that need to be uploaded.
+
 Documentation
 ~~~~~~~~~~~~~
 
diff --git a/src/scitacean/client.py b/src/scitacean/client.py
index 9c461e1..0ef2598 100644
--- a/src/scitacean/client.py
+++ b/src/scitacean/client.py
@@ -10,7 +10,7 @@ import re
 import warnings
 from contextlib import contextmanager
 from pathlib import Path
-from typing import Any, Callable, Dict, Iterator, List, Optional, Tuple, Union
+from typing import Any, Callable, Dict, Iterable, Iterator, List, Optional, Tuple, Union
 from urllib.parse import quote_plus
 
 import requests
@@ -20,6 +20,7 @@ from ._base_model import convert_download_to_user_model
 from .dataset import Dataset
 from .error import ScicatCommError, ScicatLoginError
 from .file import File
+from .filesystem import RemotePath
 from .logging import get_logger
 from .pid import PID
 from .typing import DownloadConnection, FileTransfer, UploadConnection
@@ -246,15 +247,13 @@ class Client:
             and some files or a partial dataset are left on the servers.
             Note the error message if that happens.
         """
-        dataset = dataset.replace(
-            source_folder=self._expect_file_transfer().source_folder_for(dataset)
-        )
+        dataset = dataset.replace(source_folder=self._source_folder_for(dataset))
+        files_to_upload = _files_to_upload(dataset.files)
         self.scicat.validate_dataset_model(dataset.make_upload_model())
-        # TODO skip if there are no files
-        with self._connect_for_file_upload(dataset) as con:
+        with self._connect_for_file_upload(dataset, files_to_upload) as con:
             # TODO check if any remote file is out of date.
             #  if so, raise an error. We never overwrite remote files!
-            uploaded_files = con.upload_files(*dataset.files)
+            uploaded_files = con.upload_files(*files_to_upload)
             dataset = dataset.replace_files(*uploaded_files)
             try:
                 finalized_model = self.scicat.create_dataset_model(
@@ -316,9 +315,24 @@ class Client:
             ) from exc
 
     @contextmanager
-    def _connect_for_file_upload(self, dataset: Dataset) -> Iterator[UploadConnection]:
-        with self._expect_file_transfer().connect_for_upload(dataset) as con:
-            yield con
+    def _connect_for_file_upload(
+        self, dataset: Dataset, files_to_upload: Iterable[File]
+    ) -> Iterator[UploadConnection]:
+        if not files_to_upload:
+            yield _NullUploadConnection()
+        else:
+            with self._expect_file_transfer().connect_for_upload(dataset) as con:
+                yield con
+
+    def _source_folder_for(self, dataset: Dataset) -> RemotePath:
+        if self._file_transfer is not None:
+            return self._file_transfer.source_folder_for(dataset)
+        if dataset.source_folder is None:
+            raise ValueError(
+                "Cannot determine source_folder for dataset because "
+                "the dataset's source_folder is None and there is no file transfer."
+            )
+        return dataset.source_folder
 
     def _expect_file_transfer(self) -> FileTransfer:
         if self.file_transfer is None:
@@ -1062,3 +1076,34 @@ def _remove_up_to_date_local_files(
             file.local_path.exists() and is_up_to_date(file)  # type: ignore[union-attr]
         )
     ]
+
+
+def _files_to_upload(files: Iterable[File]) -> List[File]:
+    for file in files:
+        if file.is_on_local and file.is_on_remote:
+            raise ValueError(
+                f"Refusing to upload file at remote_path={file.remote_path} "
+                "because it is both on local and remote and it is unclear what "
+                "to do. If you want to perform the upload, set the local path to None."
+            )
+    return [file for file in files if file.local_path is not None]
+
+
+class _NullUploadConnection:
+    """File-upload connection that does not upload anything.
+
+    Raises if called with files because uit should only be used by Client
+    when there are no files to upload.
+    """
+
+    def upload_files(self, *files: File) -> List[File]:
+        """Raise if given files."""
+        if files:
+            raise RuntimeError("Internal error: Bad upload connection")
+        return []
+
+    def revert_upload(self, *files: File) -> None:
+        """Raise if given files."""
+        if files:
+            raise RuntimeError("Internal error: Bad upload connection")
+        return []
diff --git a/src/scitacean/file.py b/src/scitacean/file.py
index 8211572..d6b8e03 100644
--- a/src/scitacean/file.py
+++ b/src/scitacean/file.py
@@ -10,6 +10,8 @@ from datetime import datetime, timezone
 from pathlib import Path
 from typing import NoReturn, Optional, Union, cast
 
+import dateutil.parser
+
 from .error import IntegrityError
 from .filesystem import RemotePath, checksum_of_file, file_modification_time, file_size
 from .logging import get_logger
@@ -92,7 +94,7 @@ class File:
         Parameters
         ----------
         path:
-            Full path the local file.
+            Full path of the local file.
         base_path:
             Only use ``path.relative_to(base_path)`` to determine the remote location.
         remote_path:
@@ -103,6 +105,11 @@ class File:
             Group ID on the remote. Will be determined automatically on upload.
         remote_perm:
             File permissions on the remote. Will be determined automatically on upload.
+
+        Returns
+        -------
+        :
+            A new file object.
         """
         path = Path(path)
         if not remote_path:
@@ -116,6 +123,73 @@ class File:
             _checksum_cache=_Checksum(),
         )
 
+    @classmethod
+    def from_remote(
+        cls,
+        remote_path: Union[str, RemotePath],
+        size: int,
+        creation_time: Union[datetime, str],
+        checksum: Optional[str] = None,
+        checksum_algorithm: Optional[str] = None,
+        remote_uid: Optional[str] = None,
+        remote_gid: Optional[str] = None,
+        remote_perm: Optional[str] = None,
+    ):
+        """Construct a new file object for a remote file.
+
+        The local path of the returned ``File`` is ``None``.
+
+        Parameters
+        ----------
+        remote_path:
+            Path the remote file relative to the dataset's source folder.
+        size:
+            Size in bytes on the remote filesystem.
+        creation_time:
+            Date and time the file was created on the remote filesystem.
+            If a ``str``, it is parsed using ``dateutil.parser.parse``.
+        checksum:
+            Checksum of the file.
+        checksum_algorithm:
+            Algorithm used to compute the given checksum.
+            Must be passed when ``checksum is not None``.
+        remote_uid:
+            User ID on the remote.
+        remote_gid:
+            Group ID on the remote.
+        remote_perm:
+            File permissions on the remote.
+
+        Returns
+        -------
+        :
+            A new file object.
+
+
+        .. versionadded:: RELEASE_PLACEHOLDER
+        """
+        if checksum is not None and checksum_algorithm is None:
+            raise TypeError(
+                "Must specify checksum_algorithm when providing a checksum."
+            )
+
+        creation_time = (
+            creation_time
+            if isinstance(creation_time, datetime)
+            else dateutil.parser.parse(creation_time)
+        )
+        return File(
+            local_path=None,
+            remote_path=RemotePath(remote_path),
+            remote_uid=remote_uid,
+            remote_gid=remote_gid,
+            remote_perm=remote_perm,
+            _remote_size=size,
+            _remote_creation_time=creation_time,
+            _remote_checksum=checksum,
+            checksum_algorithm=checksum_algorithm,
+        )
+
     @classmethod
     def from_download_model(
         cls,


Test patch:
diff --git a/tests/file_test.py b/tests/file_test.py
index 82187ab..0802612 100644
--- a/tests/file_test.py
+++ b/tests/file_test.py
@@ -113,6 +113,57 @@ def test_file_from_local_remote_path_uses_forward_slash(fs):
     assert file.make_model().path == "file.dat"
 
 
+def test_file_from_remote_default_args():
+    file = File.from_remote(
+        remote_path="/remote/source/file.nxs",
+        size=6123,
+        creation_time="2023-09-27T16:00:15Z",
+    )
+    assert file.local_path is None
+    assert isinstance(file.remote_path, RemotePath)
+    assert file.remote_path == "/remote/source/file.nxs"
+    assert file.size == 6123
+    assert isinstance(file.creation_time, datetime)
+    assert file.creation_time == parse_date("2023-09-27T16:00:15Z")
+    assert file.checksum() is None
+    assert file.checksum_algorithm is None
+    assert file.remote_uid is None
+    assert file.remote_gid is None
+    assert file.remote_perm is None
+
+
+def test_file_from_remote_all_args():
+    file = File.from_remote(
+        remote_path="/remote/image.png",
+        size=9,
+        creation_time=parse_date("2023-10-09T08:12:59Z"),
+        checksum="abcde",
+        checksum_algorithm="md5",
+        remote_uid="user-usy",
+        remote_gid="groupy-group",
+        remote_perm="wrx",
+    )
+    assert file.local_path is None
+    assert file.remote_path == "/remote/image.png"
+    assert file.size == 9
+    assert file.creation_time == parse_date("2023-10-09T08:12:59Z")
+    assert file.checksum() == "abcde"
+    assert file.checksum_algorithm == "md5"
+    assert file.remote_uid == "user-usy"
+    assert file.remote_gid == "groupy-group"
+    assert file.remote_perm == "wrx"
+
+
+def test_file_from_remote_checksum_requires_algorithm():
+    with pytest.raises(TypeError, match="checksum"):
+        File.from_remote(
+            remote_path="/remote/image.png",
+            size=9,
+            creation_time="2023-10-09T08:12:59Z",
+            checksum="abcde",
+        )
+
+
 def test_file_from_download_model():
     model = DownloadDataFile(
         path="dir/image.jpg", size=12345, time=parse_date("2022-06-22T15:42:53.123Z")
diff --git a/tests/upload_test.py b/tests/upload_test.py
index ab7a0e4..8e593b3 100644
--- a/tests/upload_test.py
+++ b/tests/upload_test.py
@@ -11,6 +11,7 @@ from scitacean import (
     Client,
     Dataset,
     DatasetType,
+    File,
     ScicatCommError,
     Thumbnail,
 )
@@ -122,6 +123,86 @@ def test_upload_without_files_creates_dataset(client, dataset):
         client.scicat.get_orig_datablocks(finalized.pid)
 
 
+def test_upload_without_files_does_not_need_file_transfer(dataset):
+    client = FakeClient()
+    finalized = client.upload_new_dataset_now(dataset)
+    expected = client.get_dataset(finalized.pid, attachments=True).replace(
+        # The backend may update the dataset after upload
+        _read_only={
+            "updated_at": finalized.updated_at,
+            "updated_by": finalized.updated_by,
+        }
+    )
+    assert finalized == expected
+    with pytest.raises(ScicatCommError):
+        client.scicat.get_orig_datablocks(finalized.pid)
+
+
+def test_upload_without_files_does_not_need_revert_files(dataset):
+    client = FakeClient(
+        disable={"create_dataset_model": ScicatCommError("Ingestion failed")}
+    )
+    with pytest.raises(ScicatCommError):
+        # Does not raise from attempting to access the file transfer.
+        client.upload_new_dataset_now(dataset)
+
+
+def test_upload_with_only_remote_files_does_not_need_file_transfer(dataset):
+    dataset.add_files(
+        File.from_remote(
+            remote_path="source/file1.h5", size=512, creation_time=dataset.creation_time
+        )
+    )
+
+    client = FakeClient()
+    finalized = client.upload_new_dataset_now(dataset)
+    expected = client.get_dataset(finalized.pid, attachments=True).replace(
+        # The backend may update the dataset after upload
+        _read_only={
+            "updated_at": finalized.updated_at,
+            "updated_by": finalized.updated_by,
+        }
+    )
+    assert finalized == expected
+
+
+def test_upload_with_both_remote_and_local_files(client, dataset_with_files):
+    original_file_names = set(
+        dataset_with_files.source_folder / file.remote_path
+        for file in dataset_with_files.files
+    )
+    dataset_with_files.add_files(
+        File.from_remote(
+            remote_path="file1.h5", size=6123, creation_time="2019-09-09T19:29:39Z"
+        )
+    )
+
+    finalized = client.upload_new_dataset_now(dataset_with_files)
+    expected = client.get_dataset(finalized.pid, attachments=True).replace(
+        # The backend may update the dataset after upload
+        _read_only={
+            "updated_at": finalized.updated_at,
+            "updated_by": finalized.updated_by,
+        }
+    )
+    assert finalized == expected
+    # Does not include the remote file "file1.h5"
+    assert get_file_transfer(client).files.keys() == original_file_names
+
+
+def test_upload_with_file_with_both_remote_and_local_path(client, dataset_with_files):
+    file = File.from_remote(
+        remote_path="file1.h5", size=6123, creation_time="2019-09-09T19:29:39Z"
+    )
+    file = file.downloaded(local_path="/local/file1.h5")
+    dataset_with_files.add_files(file)
+
+    # Client w/o file transfer to ensure that the client never attempts to upload.
+    client = FakeClient()
+    with pytest.raises(ValueError, match="path"):
+        client.upload_new_dataset_now(dataset_with_files)
+
+
 def test_upload_creates_dataset_and_datablock(client, dataset_with_files):
     finalized = client.upload_new_dataset_now(dataset_with_files)
     assert client.datasets[finalized.pid].createdAt == finalized.created_at