Repository: ResearchObject/ro-crate-py

Issue:
Bug: Won't load a crate with hasPart reference to a RepositoryCollection.
Loading the below test data gives an error:

```
File "/Users/pt/working/ro-crate-py/test.py", line 3, in <module>
    crate = ROCrate("haspart") 
  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 227, in build_crate
    self.add(instance)
UnboundLocalError: local variable 'instance' referenced before assignment

```

I would expect to be able to load anything with a flat JSON-LD structure even if it's not schema.org compliant.


Here's the offending ro-crate-metadata.json

```json
{
  "@context": [
    "https://w3id.org/ro/crate/1.1/context",
    {
      "@vocab": "http://schema.org/"
    },
    {
      "@base": null
    }
  ],
  "@graph": [
    {
      "@id": "#collection",
      "@type": "RepositoryCollection ",
      "name": "Test collection"
    },
    {
      "@id": "./",
      "@type": "Dataset",
      "hasFile": [],
      "hasPart": [{"@id": "#collection"}],
      "name": "testing hasPart"
    },
    {
      "@id": "ro-crate-metadata.json",
      "@type": "CreativeWork",
      "about": {
        "@id": "./"
      },
      "identifier": "ro-crate-metadata.json"
    }
  ]
}
```

Discussion:

Patch:
diff --git a/rocrate/model/data_entity.py b/rocrate/model/data_entity.py
index d04d364..e9acbe5 100644
--- a/rocrate/model/data_entity.py
+++ b/rocrate/model/data_entity.py
@@ -24,13 +24,14 @@ from .entity import Entity
 
 class DataEntity(Entity):
 
-    def __init__(self, crate, identifier, properties=None):
-        if not identifier or str(identifier).startswith("#"):
-            raise ValueError("Identifier for data entity must be a relative path or absolute URI: %s" % identifier)
-        super(DataEntity, self).__init__(crate, identifier, properties)
-
     def filepath(self, base_path=None):
         if base_path:
             return os.path.join(base_path, self.id)
         else:
             return self.id
+
+    def write(self, base_path):
+        pass
+
+    def write_zip(self, zip_out):
+        pass
diff --git a/rocrate/rocrate.py b/rocrate/rocrate.py
index 029b8e3..b213172 100644
--- a/rocrate/rocrate.py
+++ b/rocrate/rocrate.py
@@ -32,6 +32,7 @@ from urllib.parse import urljoin
 from .model import contextentity
 from .model.entity import Entity
 from .model.root_dataset import RootDataset
+from .model.data_entity import DataEntity
 from .model.file import File
 from .model.dataset import Dataset
 from .model.metadata import Metadata, LegacyMetadata, TESTING_EXTRA_TERMS
@@ -186,42 +187,39 @@ class ROCrate():
         added_entities = []
         # iterate over data entities
         for data_entity_ref in root_entity_parts:
-            data_entity_id = data_entity_ref['@id']
-            # print(data_entity_id)
-            entity = entities[data_entity_id]
-            # basic checks should be moved to a separate function
-            if '@type' not in entity.keys():
-                raise Exception("Entity with @id:" + data_entity_id +
-                                " has no type defined")
-
-            # Data entities can have an array as @type. So far we just parse
-            # them as File class if File is in the list. For further
-            # extensions (e.g if a Workflow class is created) we can add extra
-            # cases or create a mapping table for specific combinations. See
+            id_ = data_entity_ref['@id']
+            entity = entities[id_]
+            try:
+                t = entity["@type"]
+            except KeyError:
+                raise ValueError(f'entity "{id_}" has no @type')
+            types = {_.strip() for _ in set(t if isinstance(t, list) else [t])}
+            # Deciding what to instantiate is not trivial, see
             # https://github.com/ResearchObject/ro-crate/issues/83
-            entity_types = (entity['@type']
-                            if isinstance(entity['@type'], list)
-                            else [entity['@type']])
-            if 'File' in entity_types:
+            if {'File', 'Dataset'} <= types:
+                raise ValueError("entity can't have both File and Dataset types")
+            if 'File' in types:
                 # temporary workaround, should be handled in the general case
-                cls = TestDefinition if "TestDefinition" in entity_types else File
-                identifier = entity['@id']
+                cls = TestDefinition if "TestDefinition" in types else File
                 props = {k: v for k, v in entity.items() if k != '@id'}
-                if is_url(identifier):
-                    instance = cls(self, source=identifier, properties=props)
+                if is_url(id_):
+                    instance = cls(self, source=id_, properties=props)
                 else:
                     instance = cls(
                         self,
-                        source=os.path.join(source, identifier),
-                        dest_path=identifier,
+                        source=os.path.join(source, id_),
+                        dest_path=id_,
                         properties=props
                     )
-            if 'Dataset' in entity_types:
-                dir_path = os.path.join(source, entity['@id'])
+            elif 'Dataset' in types:
+                dir_path = os.path.join(source, id_)
                 props = {k: v for k, v in entity.items() if k != '@id'}
-                instance = Dataset(self, dir_path, entity['@id'], props)
+                instance = Dataset(self, dir_path, id_, props)
+            else:
+                props = {k: v for k, v in entity.items() if k != '@id'}
+                instance = DataEntity(self, identifier=id_, properties=props)
             self.add(instance)
-            added_entities.append(data_entity_id)
+            added_entities.append(id_)
 
         # the rest of the entities must be contextual entities
         prebuilt_entities = [


Test patch:
diff --git a/test/test_model.py b/test/test_model.py
index aaf5169..e9d211c 100644
--- a/test/test_model.py
+++ b/test/test_model.py
@@ -20,7 +20,9 @@ import uuid
 
 import pytest
 from rocrate.rocrate import ROCrate
+from rocrate.model.data_entity import DataEntity
 from rocrate.model.file import File
+from rocrate.model.dataset import Dataset
 from rocrate.model.computationalworkflow import ComputationalWorkflow
 from rocrate.model.person import Person
 from rocrate.model.preview import Preview
@@ -70,6 +72,16 @@ def test_dereferencing_equivalent_id(test_data_dir, name):
         assert deref_entity is entity
 
 
+def test_data_entities(test_data_dir):
+    crate = ROCrate()
+    file_ = crate.add(File(crate, test_data_dir / 'sample_file.txt'))
+    dataset = crate.add(Dataset(crate, test_data_dir / 'test_add_dir'))
+    data_entity = crate.add(DataEntity(crate, '#mysterious'))
+    assert set(crate.data_entities) == {file_, dataset, data_entity}
+    part_ids = set(_["@id"] for _ in crate.root_dataset._jsonld["hasPart"])
+    assert set(_.id for _ in (file_, dataset, data_entity)) <= part_ids
+
+
 def test_contextual_entities():
     crate = ROCrate()
     new_person = crate.add(Person(crate, '#joe', {'name': 'Joe Pesci'}))
diff --git a/test/test_read.py b/test/test_read.py
index e170040..67f19b1 100644
--- a/test/test_read.py
+++ b/test/test_read.py
@@ -15,6 +15,7 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
+import json
 import pytest
 import shutil
 import uuid
@@ -22,6 +23,7 @@ import zipfile
 from pathlib import Path
 
 from rocrate.rocrate import ROCrate
+from rocrate.model.data_entity import DataEntity
 from rocrate.model.file import File
 from rocrate.model.dataset import Dataset
 
@@ -298,3 +300,58 @@ def test_missing_file(test_data_dir, tmpdir):
     out_path = tmpdir / 'crate_read_out'
     crate.write_crate(out_path)
     assert not (out_path / name).exists()
+
+
+def test_generic_data_entity(tmpdir):
+    rc_id = "#collection"
+    metadata = {
+        "@context": [
+            "https://w3id.org/ro/crate/1.1/context",
+            {"@vocab": "http://schema.org/"},
+            {"@base": None}
+        ],
+        "@graph": [
+            {
+                "@id": "ro-crate-metadata.json",
+                "@type": "CreativeWork",
+                "about": {
+                    "@id": "./"
+                },
+                "identifier": "ro-crate-metadata.json"
+            },
+            {
+                "@id": "./",
+                "@type": "Dataset",
+                "hasPart": [{"@id": rc_id}],
+                "name": "Test RepositoryCollection"
+            },
+            {
+                "@id": rc_id,
+                "@type": "RepositoryCollection",
+                "name": "Test collection"
+            }
+        ]
+    }
+    crate_dir = tmpdir / "test_repository_collection"
+    crate_dir.mkdir()
+    with open(crate_dir / "ro-crate-metadata.json", "wt") as f:
+        json.dump(metadata, f, indent=4)
+    crate = ROCrate(crate_dir)
+
+    def check_rc():
+        rc = crate.dereference(rc_id)
+        assert rc is not None
+        assert isinstance(rc, DataEntity)
+        assert rc.id == rc_id
+        assert rc.type == "RepositoryCollection"
+        assert rc._jsonld["name"] == "Test collection"
+        assert crate.data_entities == [rc]
+        assert not crate.contextual_entities
+
+    check_rc()
+
+    out_crate_dir = tmpdir / "output_crate"
+    crate.write_crate(out_crate_dir)
+    crate = ROCrate(out_crate_dir)
+
+    check_rc()