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

Issue:
Bug? Fails to load a crate file without its data entities  (directories)
I think this is a bug.

On loading a directory with ONLY the sample file from here: https://raw.githubusercontent.com/ResearchObject/example-ro-sample-image-crate/main/sample-crate/ro-crate-metadata.jsonld

```
from rocrate.rocrate import ROCrate

crate = ROCrate("sample") 
```

It dies like this:
```
Traceback (most recent call last):
  File "/Users/pt/working/ro-crate-py/test.py", line 3, in <module>
    crate = ROCrate("sample") 
  File "/Users/pt/rocrate/lib/python3.9/site-packages/rocrate/rocrate.py", line 92, in __init__
    self.build_crate(entities, source_path, gen_preview)
  File "/Users/pt/rocrate/lib/python3.9/site-packages/rocrate/rocrate.py", line 226, in build_crate
    raise Exception('Directory not found')
Exception: Directory not found

```

Expected behaviour:

I would expect to be able to load an incomplete crate if I am:
- Writing an HTML renderer
- Writing a validator and I want to report all the errors (rather than dying on the first one



 

Pull request:
Allow metadata to reference missing files or directories
Closes #73.

With this PR, the library does not complain if files and directories referenced by the RO-Crate metadata are missing. Corresponding entities are created as usual and the crate can be written out, but missing files will of course also be missing from the output crate.

Patch:
diff --git a/rocrate/cli.py b/rocrate/cli.py
index 7512383..0f02132 100644
--- a/rocrate/cli.py
+++ b/rocrate/cli.py
@@ -39,25 +39,23 @@ class State:
 @click.pass_context
 def cli(ctx, crate_dir):
     ctx.obj = state = State()
-    state.crate_dir = crate_dir
+    state.crate_dir = os.getcwd() if not crate_dir else os.path.abspath(crate_dir)
 
 
 @cli.command()
 @click.option('--gen-preview', is_flag=True)
 @click.pass_obj
 def init(state, gen_preview):
-    crate_dir = state.crate_dir or os.getcwd()
-    crate = ROCrate(crate_dir, init=True, gen_preview=gen_preview)
-    crate.metadata.write(crate_dir)
+    crate = ROCrate(state.crate_dir, init=True, gen_preview=gen_preview)
+    crate.metadata.write(state.crate_dir)
     if crate.preview:
-        crate.preview.write(crate_dir)
+        crate.preview.write(state.crate_dir)
 
 
 @cli.group()
 @click.pass_obj
 def add(state):
-    crate_dir = state.crate_dir or os.getcwd()
-    state.crate = ROCrate(crate_dir, init=False, gen_preview=False)
+    state.crate = ROCrate(state.crate_dir, init=False, gen_preview=False)
 
 
 @add.command()
@@ -65,16 +63,15 @@ def add(state):
 @click.option('-l', '--language', type=click.Choice(LANG_CHOICES), default="cwl")
 @click.pass_obj
 def workflow(state, path, language):
-    crate_dir = state.crate_dir or os.getcwd()
     source = Path(path).resolve(strict=True)
     try:
-        dest_path = source.relative_to(crate_dir)
+        dest_path = source.relative_to(state.crate_dir)
     except ValueError:
         # For now, only support marking an existing file as a workflow
-        raise ValueError(f"{source} is not in the crate dir")
+        raise ValueError(f"{source} is not in the crate dir {state.crate_dir}")
     # TODO: add command options for main and gen_cwl
     state.crate.add_workflow(source, dest_path, main=True, lang=language, gen_cwl=False)
-    state.crate.metadata.write(crate_dir)
+    state.crate.metadata.write(state.crate_dir)
 
 
 @add.command(name="test-suite")
@@ -83,9 +80,8 @@ def workflow(state, path, language):
 @click.option('-m', '--main-entity')
 @click.pass_obj
 def suite(state, identifier, name, main_entity):
-    crate_dir = state.crate_dir or os.getcwd()
     suite_ = state.crate.add_test_suite(identifier=identifier, name=name, main_entity=main_entity)
-    state.crate.metadata.write(crate_dir)
+    state.crate.metadata.write(state.crate_dir)
     print(suite_.id)
 
 
@@ -98,9 +94,8 @@ def suite(state, identifier, name, main_entity):
 @click.option('-n', '--name')
 @click.pass_obj
 def instance(state, suite, url, resource, service, identifier, name):
-    crate_dir = state.crate_dir or os.getcwd()
     instance_ = state.crate.add_test_instance(suite, url, resource=resource, service=service, identifier=identifier, name=name)
-    state.crate.metadata.write(crate_dir)
+    state.crate.metadata.write(state.crate_dir)
     print(instance_.id)
 
 
@@ -111,23 +106,21 @@ def instance(state, suite, url, resource, service, identifier, name):
 @click.option('-v', '--engine-version')
 @click.pass_obj
 def definition(state, suite, path, engine, engine_version):
-    crate_dir = state.crate_dir or os.getcwd()
     source = Path(path).resolve(strict=True)
     try:
-        dest_path = source.relative_to(crate_dir)
+        dest_path = source.relative_to(state.crate_dir)
     except ValueError:
         # For now, only support marking an existing file as a test definition
-        raise ValueError(f"{source} is not in the crate dir")
+        raise ValueError(f"{source} is not in the crate dir {state.crate_dir}")
     state.crate.add_test_definition(suite, source=source, dest_path=dest_path, engine=engine, engine_version=engine_version)
-    state.crate.metadata.write(crate_dir)
+    state.crate.metadata.write(state.crate_dir)
 
 
 @cli.command()
 @click.argument('dst', type=click.Path(writable=True))
 @click.pass_obj
 def write_zip(state, dst):
-    crate_dir = state.crate_dir or os.getcwd()
-    crate = ROCrate(crate_dir, init=True, gen_preview=False)
+    crate = ROCrate(state.crate_dir, init=True, gen_preview=False)
     crate.write_zip(dst)
 
 
diff --git a/rocrate/model/file.py b/rocrate/model/file.py
index 4b5cc50..8005fcf 100644
--- a/rocrate/model/file.py
+++ b/rocrate/model/file.py
@@ -20,10 +20,9 @@
 import os
 from pathlib import Path
 import shutil
-import urllib
+import urllib.request
 
 from io import IOBase
-from shutil import copy
 from urllib.error import HTTPError
 
 from .data_entity import DataEntity
@@ -38,20 +37,14 @@ class File(DataEntity):
             properties = {}
         self.fetch_remote = fetch_remote
         self.source = source
-        is_local = is_remote = False
-        if isinstance(source, (str, Path)):
-            is_local = os.path.isfile(str(source))
-            is_remote = is_url(str(source))
-            if not is_local and not is_remote:
-                raise ValueError(f"'{source}' is not a path to a local file or a valid remote URI")
-        elif dest_path is None:
+        if not isinstance(source, (str, Path)) and dest_path is None:
             raise ValueError("dest_path must be provided if source is not a path or URI")
         if dest_path:
             # the entity is refrencing a path relative to the ro-crate root
             identifier = Path(dest_path).as_posix()  # relative path?
         else:
             # if there is no dest_path there must be a URI/local path as source
-            if is_local:
+            if not is_url(str(source)):
                 # local source -> becomes local reference = reference relative
                 # to ro-crate root
                 identifier = os.path.basename(source)
@@ -105,23 +98,16 @@ class File(DataEntity):
             out_file_path.parent.mkdir(parents=True, exist_ok=True)
             with open(out_file_path, 'w') as out_file:
                 out_file.write(self.source.getvalue())
-        else:
-            if os.path.isfile(self.source):
-                out_file_path.parent.mkdir(parents=True, exist_ok=True)
-                copy(self.source, out_file_path)
-            else:
-                if self.fetch_remote:
-                    # Legacy version
-                    # urllib.request.urlretrieve(self.source, out_file_path)
-                    # can check that the encodingFormat and contentSize matches
-                    # the request data? i.e.:
-                    # response.getheader('Content-Length') == \
-                    #   self._jsonld['contentSize']
-                    # this would help check if the dataset to be retrieved is
-                    # in fact what was registered in the first place.
-                    out_file_path.parent.mkdir(parents=True, exist_ok=True)
-                    with urllib.request.urlopen(self.source) as response, open(out_file_path, 'wb') as out_file:
-                        shutil.copyfileobj(response, out_file)
+        elif is_url(str(self.source)) and self.fetch_remote:
+            # Should we check that the resource hasn't changed? E.g., compare
+            # response.getheader('Content-Length') to self._jsonld['contentSize'] or
+            # response.getheader('Content-Type') to self._jsonld['encodingFormat']
+            out_file_path.parent.mkdir(parents=True, exist_ok=True)
+            with urllib.request.urlopen(self.source) as response, open(out_file_path, 'wb') as out_file:
+                shutil.copyfileobj(response, out_file)
+        elif os.path.isfile(self.source):
+            out_file_path.parent.mkdir(parents=True, exist_ok=True)
+            shutil.copy(self.source, out_file_path)
 
     def write_zip(self, zip_out):
         if self.id not in zip_out.namelist():
diff --git a/rocrate/rocrate.py b/rocrate/rocrate.py
index a968f10..029b8e3 100644
--- a/rocrate/rocrate.py
+++ b/rocrate/rocrate.py
@@ -21,7 +21,6 @@ import importlib
 import json
 import os
 import uuid
-import requests
 import zipfile
 import atexit
 import shutil
@@ -206,25 +205,21 @@ class ROCrate():
             if 'File' in entity_types:
                 # temporary workaround, should be handled in the general case
                 cls = TestDefinition if "TestDefinition" in entity_types else File
-                file_path = os.path.join(source, entity['@id'])
-                identifier = entity.pop('@id', None)
-                if os.path.exists(file_path):
-                    # referencing a file path relative to crate-root
-                    instance = cls(self, file_path, identifier, properties=entity)
+                identifier = entity['@id']
+                props = {k: v for k, v in entity.items() if k != '@id'}
+                if is_url(identifier):
+                    instance = cls(self, source=identifier, properties=props)
                 else:
-                    # check if it is a valid absolute URI
-                    try:
-                        requests.get(identifier)
-                        instance = cls(self, identifier, properties=entity)
-                    except requests.ConnectionError:
-                        print("Source is not a valid URI")
+                    instance = cls(
+                        self,
+                        source=os.path.join(source, identifier),
+                        dest_path=identifier,
+                        properties=props
+                    )
             if 'Dataset' in entity_types:
                 dir_path = os.path.join(source, entity['@id'])
-                if os.path.exists(dir_path):
-                    props = {k: v for k, v in entity.items() if k != '@id'}
-                    instance = Dataset(self, dir_path, entity['@id'], props)
-                else:
-                    raise Exception('Directory not found')
+                props = {k: v for k, v in entity.items() if k != '@id'}
+                instance = Dataset(self, dir_path, entity['@id'], props)
             self.add(instance)
             added_entities.append(data_entity_id)
 


Test patch:
diff --git a/test/test_read.py b/test/test_read.py
index a3e334b..e170040 100644
--- a/test/test_read.py
+++ b/test/test_read.py
@@ -269,3 +269,32 @@ def test_extra_data(test_data_dir, tmpdir, to_zip):
         assert (out_path / rel).is_file()
         with open(crate_dir / rel) as f1, open(out_path / rel) as f2:
             assert f1.read() == f2.read()
+
+
+def test_missing_dir(test_data_dir, tmpdir):
+    crate_dir = test_data_dir / 'read_crate'
+    name = 'examples'
+    shutil.rmtree(crate_dir / name)
+    crate = ROCrate(crate_dir)
+
+    examples_dataset = crate.dereference(name)
+    assert examples_dataset.id == f'{name}/'
+
+    out_path = tmpdir / 'crate_read_out'
+    crate.write_crate(out_path)
+    assert not (out_path / 'README.txt').exists()
+
+
+def test_missing_file(test_data_dir, tmpdir):
+    crate_dir = test_data_dir / 'read_crate'
+    name = 'test_file_galaxy.txt'
+    test_path = crate_dir / name
+    test_path.unlink()
+    crate = ROCrate(crate_dir)
+
+    test_file = crate.dereference(name)
+    assert test_file.id == name
+
+    out_path = tmpdir / 'crate_read_out'
+    crate.write_crate(out_path)
+    assert not (out_path / name).exists()
diff --git a/test/test_write.py b/test/test_write.py
index c83797c..102d793 100644
--- a/test/test_write.py
+++ b/test/test_write.py
@@ -165,14 +165,22 @@ def test_remote_uri_exceptions(tmpdir):
 
 
 @pytest.mark.parametrize("fetch_remote,validate_url", [(False, False), (False, True), (True, False), (True, True)])
-def test_missing_source(test_data_dir, fetch_remote, validate_url):
-    crate = ROCrate()
+def test_missing_source(test_data_dir, tmpdir, fetch_remote, validate_url):
     path = test_data_dir / uuid.uuid4().hex
-    with pytest.raises(ValueError):
-        crate.add_file(str(path), fetch_remote=fetch_remote, validate_url=validate_url)
+    args = {"fetch_remote": fetch_remote, "validate_url": validate_url}
 
-    with pytest.raises(ValueError):
-        crate.add_file(str(path), path.name, fetch_remote=fetch_remote, validate_url=validate_url)
+    crate = ROCrate()
+    file_ = crate.add_file(str(path), **args)
+    assert file_ is crate.dereference(path.name)
+    out_path = tmpdir / 'ro_crate_out_1'
+    crate.write_crate(out_path)
+    assert not (out_path / path.name).exists()
+
+    crate = ROCrate()
+    file_ = crate.add_file(str(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()
 
 
 @pytest.mark.parametrize("fetch_remote,validate_url", [(False, False), (False, True), (True, False), (True, True)])