Repository: ResearchObject/ro-crate-py (python)

Issue:
Fix behavior wrt "missing" data entities
After the merge of #75, we allow data entities whose `@id` does not map to an existing file or directory, even if local. While this adds flexibility for use cases like #73, such crates are not supported by other libraries. Specifically, I got reports of crates generated with ro-crate-py that lead to errors when submission to WorkflowHub is attempted. The main issue is that it's too easy to add a "missing" data entity:

```python
crate.add_file("/this/does/not/exist")  # Adds a File entity with @id = "exist"
```

This means it can be done involuntarily, by simply mistyping the path. It should be harder to do so (advanced usage), and we should issue a warning when it's done.

Pull request:
Fix missing file
Fixes #135.

Now when `write` is called on a crate containing at least a file or dataset whose source is missing, an `OSError` will be raised. Note that the use case mentioned in #73 is still covered: a crate with a missing data entity can still be loaded, but will not be immediately writable back to disk. To "fix" the crate, one can set the source of the offending data entity to:

1. an existing file or directory

1. `None`: in this case, when the crate is written:
  1.1 If the data entity is a `File`, the actual file will still be missing in the serialized crate, and a warning will be issued
  2.1 If the data entity is a `Dataset`, an empty directory will be created for it

When _creating_ a new RO-Crate, it's now possible to pass `None` as the source argument of `File` and `Dataset`: the former allows to create a crate with a missing file, which will be treated as explained above; the latter allows to add an empty directory to a new crate.

Example:

```python
import uuid
from pathlib import Path
from rocrate.rocrate import ROCrate

crate = ROCrate()
file_path = Path("/tmp") / uuid.uuid4().hex
assert not file_path.exists()
crate.add_file(file_path)
crate.write("/tmp/crate")  # Fails with FileNotFoundError
file_entity = crate.dereference(file_path.name)
file_entity.source = None
crate.write("/tmp/crate")  # UserWarning: No source for 13af7607702f487faa712425968cf61b

# Directly add a file entity with missing source
file_path_2 = Path("/tmp") / uuid.uuid4().hex
assert not file_path_2.exists()
crate.add_file(None, file_path_2.name)
crate.write("/tmp/crate_2")  # UserWarning: No source for e645714d126446e4bf7303d7615406a5

# Read a crate containing an entity with missing source
read_crate = ROCrate("/tmp/crate")  # No error when loading the crate
read_crate.write("/tmp/crate_copy")  # Fails with FileNotFoundError
crate_file_path = Path("/tmp/crate") / file_path.name
crate_file_path.touch()
read_crate.write("/tmp/crate_copy")  # OK, source now exists
```

Se the unit tests diff for detailed checks on all use cases.

Finally, note that another option to create a data entity with missing source is to use a `file:` URI as the id.

Patch:
diff --git a/rocrate/model/dataset.py b/rocrate/model/dataset.py
index f883a02..b378ea3 100644
--- a/rocrate/model/dataset.py
+++ b/rocrate/model/dataset.py
@@ -18,6 +18,8 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
+import errno
+import os
 import shutil
 from pathlib import Path
 from urllib.request import urlopen
@@ -48,9 +50,16 @@ class Dataset(FileOrDir):
             if self.fetch_remote:
                 self.__get_parts(out_path)
         else:
-            out_path.mkdir(parents=True, exist_ok=True)
-            if not self.crate.source and self.source and Path(self.source).exists():
-                self.crate._copy_unlisted(self.source, out_path)
+            if self.source is None:
+                out_path.mkdir(parents=True, exist_ok=True)
+            else:
+                if not Path(self.source).exists():
+                    raise FileNotFoundError(
+                        errno.ENOENT, os.strerror(errno.ENOENT), str(self.source)
+                    )
+                out_path.mkdir(parents=True, exist_ok=True)
+                if not self.crate.source:
+                    self.crate._copy_unlisted(self.source, out_path)
 
     def __get_parts(self, out_path):
         out_path.mkdir(parents=True, exist_ok=True)
diff --git a/rocrate/model/file.py b/rocrate/model/file.py
index 263677d..6ea48db 100644
--- a/rocrate/model/file.py
+++ b/rocrate/model/file.py
@@ -18,10 +18,10 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-import os
 from pathlib import Path
 import shutil
 import urllib.request
+import warnings
 from http.client import HTTPResponse
 from io import BytesIO, StringIO
 
@@ -59,7 +59,10 @@ class File(FileOrDir):
                     if self.fetch_remote:
                         out_file_path.parent.mkdir(parents=True, exist_ok=True)
                         urllib.request.urlretrieve(response.url, out_file_path)
-        elif os.path.isfile(self.source):
+        elif self.source is None:
+            # Allows to record a File entity whose @id does not exist, see #73
+            warnings.warn(f"No source for {self.id}")
+        else:
             out_file_path.parent.mkdir(parents=True, exist_ok=True)
             if not out_file_path.exists() or not out_file_path.samefile(self.source):
                 shutil.copy(self.source, out_file_path)


Test patch:
diff --git a/test/test_read.py b/test/test_read.py
index edb1d9f..df50801 100644
--- a/test/test_read.py
+++ b/test/test_read.py
@@ -320,10 +320,27 @@ def test_missing_dir(test_data_dir, tmpdir):
     assert examples_dataset.id == f'{name}/'
 
     out_path = tmpdir / 'crate_read_out'
+    with pytest.raises(OSError):
+        crate.write(out_path)
+
+    # Two options to get a writable crate
+
+    # 1. Set the source to None (will create an empty dir)
+    examples_dataset.source = None
+    crate.write(out_path)
+    assert (out_path / name).is_dir()
+
+    shutil.rmtree(out_path)
+
+    # 2. Provide an existing source
+    source = tmpdir / "source"
+    source.mkdir()
+    examples_dataset.source = source
     crate.write(out_path)
-    assert not (out_path / 'README.txt').exists()
+    assert (out_path / name).is_dir()
 
 
+@pytest.mark.filterwarnings("ignore")
 def test_missing_file(test_data_dir, tmpdir):
     crate_dir = test_data_dir / 'read_crate'
     name = 'test_file_galaxy.txt'
@@ -335,9 +352,26 @@ def test_missing_file(test_data_dir, tmpdir):
     assert test_file.id == name
 
     out_path = tmpdir / 'crate_read_out'
+    with pytest.raises(OSError):
+        crate.write(out_path)
+
+    # Two options to get a writable crate
+
+    # 1. Set the source to None (file will still be missing in the copy)
+    test_file.source = None
     crate.write(out_path)
     assert not (out_path / name).exists()
 
+    shutil.rmtree(out_path)
+
+    # 2. Provide an existing source
+    source = tmpdir / "source.txt"
+    text = "foo\nbar\n"
+    source.write_text(text)
+    test_file.source = source
+    crate.write(out_path)
+    assert (out_path / name).read_text() == text
+
 
 def test_generic_data_entity(tmpdir):
     rc_id = "#collection"
diff --git a/test/test_write.py b/test/test_write.py
index b806ae5..15a5244 100644
--- a/test/test_write.py
+++ b/test/test_write.py
@@ -301,23 +301,36 @@ def test_remote_uri_exceptions(tmpdir):
     # no error on Windows, or on Linux as root, so we don't use pytest.raises
 
 
-@pytest.mark.parametrize("fetch_remote,validate_url", [(False, False), (False, True), (True, False), (True, True)])
-def test_missing_source(test_data_dir, tmpdir, fetch_remote, validate_url):
+@pytest.mark.filterwarnings("ignore")
+@pytest.mark.parametrize("what", ["file", "dataset"])
+def test_missing_source(test_data_dir, tmpdir, what):
     path = test_data_dir / uuid.uuid4().hex
-    args = {"fetch_remote": fetch_remote, "validate_url": validate_url}
 
     crate = ROCrate()
-    file_ = crate.add_file(path, **args)
-    assert file_ is crate.dereference(path.name)
-    out_path = tmpdir / 'ro_crate_out_1'
-    crate.write(out_path)
-    assert not (out_path / path.name).exists()
+    entity = getattr(crate, f"add_{what}")(path)
+    assert entity is crate.dereference(path.name)
+    crate_dir = tmpdir / 'ro_crate_out_1'
+    with pytest.raises(OSError):
+        crate.write(crate_dir)
 
     crate = ROCrate()
-    file_ = crate.add_file(path, path.name, **args)
-    assert file_ is crate.dereference(path.name)
-    out_path = tmpdir / 'ro_crate_out_2'
-    assert not (out_path / path.name).exists()
+    entity = getattr(crate, f"add_{what}")(path, path.name)
+    assert entity is crate.dereference(path.name)
+    crate_dir = tmpdir / 'ro_crate_out_2'
+    with pytest.raises(OSError):
+        crate.write(crate_dir)
+
+    crate = ROCrate()
+    entity = getattr(crate, f"add_{what}")(None, path.name)
+    assert entity is crate.dereference(path.name)
+    crate_dir = tmpdir / 'ro_crate_out_3'
+    crate.write(crate_dir)
+    out_path = crate_dir / path.name
+    if what == "file":
+        assert not out_path.exists()
+    else:
+        assert out_path.is_dir()
+        assert not any(out_path.iterdir())
 
 
 @pytest.mark.parametrize("fetch_remote,validate_url", [(False, False), (False, True), (True, False), (True, True)])
@@ -336,12 +349,15 @@ def test_no_source_no_dest(test_data_dir, fetch_remote, validate_url):
 
 def test_dataset(test_data_dir, tmpdir):
     crate = ROCrate()
-    path = test_data_dir / "a" / "b"
-    d1 = crate.add_dataset(path)
+    path_a_b = test_data_dir / "a" / "b"
+    path_c = test_data_dir / "c"
+    for p in path_a_b, path_c:
+        p.mkdir(parents=True)
+    d1 = crate.add_dataset(path_a_b)
     assert crate.dereference("b") is d1
-    d2 = crate.add_dataset(path, "a/b")
+    d2 = crate.add_dataset(path_a_b, "a/b")
     assert crate.dereference("a/b") is d2
-    d_from_str = crate.add_dataset(str(test_data_dir / "c"))
+    d_from_str = crate.add_dataset(str(path_c))
     assert crate.dereference("c") is d_from_str
 
     out_path = tmpdir / 'ro_crate_out'