Repository: ResearchObject/ro-crate-py

Issue:
Support for non-slash root data entity
The [1.1 spec](https://www.researchobject.org/ro-crate/1.1/root-data-entity.html#direct-properties-of-the-root-data-entity) states that the root data entity SHOULD be the string `./`, but in principle it could be an arbitrary URI. This in contrast with RO-Crate 1.0, where the root data entity was always `./`. We now have a [find_root_entity_id](https://github.com/ResearchObject/ro-crate-py/blob/4ab8975b9478b7ba2f13fb0d13bd1b3b5287ac54/rocrate/rocrate.py#L105) method that looks for the root data entity without assuming it's `./`, but other parts of the code still assume that. For instance, see the [RootDataset](https://github.com/ResearchObject/ro-crate-py/blob/4ab8975b9478b7ba2f13fb0d13bd1b3b5287ac54/rocrate/model/root_dataset.py) class. We should add tests for non-slash root entities and update the code as needed.

Discussion:

Patch:
diff --git a/rocrate/model/file_or_dir.py b/rocrate/model/file_or_dir.py
index d82c2c1..afe3226 100644
--- a/rocrate/model/file_or_dir.py
+++ b/rocrate/model/file_or_dir.py
@@ -45,5 +45,5 @@ class FileOrDir(DataEntity):
             if is_url(str(source)):
                 identifier = os.path.basename(source) if fetch_remote else source
             else:
-                identifier = os.path.basename(source)
+                identifier = "./" if source == "./" else os.path.basename(source)
         super().__init__(crate, identifier, properties)
diff --git a/rocrate/model/metadata.py b/rocrate/model/metadata.py
index b06d187..4c96163 100644
--- a/rocrate/model/metadata.py
+++ b/rocrate/model/metadata.py
@@ -35,11 +35,13 @@ class Metadata(File):
     BASENAME = "ro-crate-metadata.json"
     PROFILE = "https://w3id.org/ro/crate/1.1"
 
-    def __init__(self, crate, properties=None):
+    def __init__(self, crate, source=None, dest_path=None, properties=None):
+        if source is None and dest_path is None:
+            dest_path = self.BASENAME
         super().__init__(
             crate,
-            source=None,
-            dest_path=self.BASENAME,
+            source=source,
+            dest_path=dest_path,
             fetch_remote=False,
             validate_url=False,
             properties=properties
@@ -49,7 +51,7 @@ class Metadata(File):
 
     def _empty(self):
         # default properties of the metadata entry
-        val = {"@id": self.BASENAME,
+        val = {"@id": self.id,
                "@type": "CreativeWork",
                "conformsTo": {"@id": self.PROFILE},
                "about": {"@id": "./"}}
@@ -102,9 +104,10 @@ TESTING_EXTRA_TERMS = {
 
 
 def metadata_class(descriptor_id):
-    if descriptor_id == Metadata.BASENAME:
+    basename = descriptor_id.rsplit("/", 1)[-1]
+    if basename == Metadata.BASENAME:
         return Metadata
-    elif descriptor_id == LegacyMetadata.BASENAME:
+    elif basename == LegacyMetadata.BASENAME:
         return LegacyMetadata
     else:
-        return ValueError("Invalid metadata descriptor ID: {descriptor_id!r}")
+        raise ValueError(f"Invalid metadata descriptor ID: {descriptor_id!r}")
diff --git a/rocrate/model/root_dataset.py b/rocrate/model/root_dataset.py
index 25c2060..3403529 100644
--- a/rocrate/model/root_dataset.py
+++ b/rocrate/model/root_dataset.py
@@ -24,11 +24,17 @@ from ..utils import iso_now
 
 class RootDataset(Dataset):
 
-    def __init__(self, crate, properties=None):
-        super().__init__(crate, dest_path='./', properties=properties)
-
-    def format_id(self, identifier):
-        return './'
+    def __init__(self, crate, source=None, dest_path=None, properties=None):
+        if source is None and dest_path is None:
+            dest_path = "./"
+        super().__init__(
+            crate,
+            source=source,
+            dest_path=dest_path,
+            fetch_remote=False,
+            validate_url=False,
+            properties=properties
+        )
 
     def _empty(self):
         val = {
diff --git a/rocrate/rocrate.py b/rocrate/rocrate.py
index 65927a6..8ed15de 100644
--- a/rocrate/rocrate.py
+++ b/rocrate/rocrate.py
@@ -25,6 +25,7 @@ import zipfile
 import atexit
 import shutil
 import tempfile
+import warnings
 
 from collections import OrderedDict
 from pathlib import Path
@@ -143,55 +144,79 @@ class ROCrate():
         self.__read_contextual_entities(entities)
         return source
 
-    def find_root_entity_id(self, entities):
-        """Find Metadata file and Root Data Entity in RO-Crate.
+    def __check_metadata(self, metadata, entities):
+        if metadata["@type"] != "CreativeWork":
+            raise ValueError('metadata descriptor must be of type "CreativeWork"')
+        try:
+            root = entities[metadata["about"]["@id"]]
+        except (KeyError, TypeError):
+            raise ValueError("metadata descriptor does not reference the root entity")
+        if root["@type"] != "Dataset":
+            raise ValueError('root entity must be of type "Dataset"')
+        return metadata["@id"], root["@id"]
 
-        Returns a tuple of the @id identifiers (metadata, root)
+    def find_root_entity_id(self, entities):
+        """\
+        Find metadata file descriptor and root data entity.
+
+        Return a tuple of the corresponding identifiers (metadata, root).
+        If the entities are not found, raise KeyError. If they are found,
+        but they don't satisfy the required constraints, raise ValueError.
+
+        In the general case, the metadata file descriptor id can be an
+        absolute URI whose last path segment is "ro-crate-metadata.json[ld]".
+        Since there can be more than one such id in the crate, we need to
+        choose among the corresponding (metadata, root) entity pairs. First, we
+        exclude those that don't satisfy other constraints, such as the
+        metadata entity being of type CreativeWork, etc.; if this doesn't
+        leave us with a single pair, we try to pick one with a
+        heuristic. Suppose we are left with the (m1, r1) and (m2, r2) pairs:
+        if r1 is the actual root of this crate, then m2 and r2 are regular
+        files in it, and as such they must appear in r1's hasPart; r2,
+        however, is not required to have a hasPart property listing other
+        files. Thus, we look for a pair whose root entity "contains" all
+        metadata entities from other pairs. If there is no such pair, or there
+        is more than one, we just return an arbitrary pair.
         """
-        # Note that for all cases below we will deliberately
-        # throw KeyError if "about" exists but it has no "@id"
-
-        # First let's try conformsTo algorithm in
-        # <https://www.researchobject.org/ro-crate/1.1/root-data-entity.html#finding-the-root-data-entity>
-        for entity in entities.values():
-            about = get_norm_value(entity, "about")
-            if about:
-                for id_ in get_norm_value(entity, "conformsTo"):
-                    if id_.startswith("https://w3id.org/ro/crate/"):
-                        return(entity["@id"], about[0])
-        # ..fall back to a generous look up by filename,
-        for candidate in (
-                Metadata.BASENAME, LegacyMetadata.BASENAME,
-                f"./{Metadata.BASENAME}", f"./{LegacyMetadata.BASENAME}"
-        ):
-            metadata_file = entities.get(candidate)
-            if metadata_file and "about" in metadata_file:
-                return (metadata_file["@id"], metadata_file["about"]["@id"])
-        # No luck! Is there perhaps a root dataset directly in here?
-        root = entities.get("./", {})
-        # FIXME: below will work both for
-        # "@type": "Dataset"
-        # "@type": ["Dataset"]
-        # ..but also the unlikely
-        # "@type": "DatasetSomething"
-        if root and "Dataset" in root.get("@type", []):
-            return (None, "./")
-        # Uh oh..
-        raise KeyError(
-            "Can't find Root Data Entity in RO-Crate, "
-            "see https://www.researchobject.org/ro-crate/1.1/root-data-entity.html"
-        )
+        metadata = entities.get(Metadata.BASENAME, entities.get(LegacyMetadata.BASENAME))
+        if metadata:
+            return self.__check_metadata(metadata, entities)
+        candidates = []
+        for id_, e in entities.items():
+            basename = id_.rsplit("/", 1)[-1]
+            if basename == Metadata.BASENAME or basename == LegacyMetadata.BASENAME:
+                try:
+                    candidates.append(self.__check_metadata(e, entities))
+                except ValueError:
+                    pass
+        if not candidates:
+            raise KeyError("Metadata file descriptor not found")
+        elif len(candidates) == 1:
+            return candidates[0]
+        else:
+            warnings.warn("Multiple metadata file descriptors, will pick one with a heuristic")
+            metadata_ids = set(_[0] for _ in candidates)
+            for m_id, r_id in candidates:
+                try:
+                    root = entities[r_id]
+                    part_ids = set(_["@id"] for _ in root["hasPart"])
+                except KeyError:
+                    continue
+                if part_ids >= metadata_ids - {m_id}:
+                    # if True for more than one candidate, this pick is arbitrary
+                    return m_id, r_id
+            return candidates[0]  # fall back to arbitrary pick
 
     def __read_data_entities(self, entities, source, gen_preview):
         metadata_id, root_id = self.find_root_entity_id(entities)
         MetadataClass = metadata_class(metadata_id)
         metadata_properties = entities.pop(metadata_id)
-        self.add(MetadataClass(self, properties=metadata_properties))
+        self.add(MetadataClass(self, metadata_id, properties=metadata_properties))
 
         root_entity = entities.pop(root_id)
         assert root_id == root_entity.pop('@id')
         parts = root_entity.pop('hasPart', [])
-        self.add(RootDataset(self, properties=root_entity))
+        self.add(RootDataset(self, root_id, properties=root_entity))
         preview_entity = entities.pop(Preview.BASENAME, None)
         if preview_entity and not gen_preview:
             self.add(Preview(self, source / Preview.BASENAME, properties=preview_entity))


Test patch:
diff --git a/test/test_read.py b/test/test_read.py
index 85aeb03..e3f2820 100644
--- a/test/test_read.py
+++ b/test/test_read.py
@@ -21,6 +21,7 @@ import pytest
 import shutil
 import uuid
 import zipfile
+from copy import deepcopy
 from pathlib import Path
 
 from rocrate.rocrate import ROCrate
@@ -452,3 +453,149 @@ def test_multi_type_context_entity(tmpdir):
     entity = crate.dereference(id_)
     assert entity in crate.contextual_entities
     assert set(entity.type) == set(type_)
+
+
+@pytest.mark.parametrize("root,basename", [
+    ("", "ro-crate-metadata.json"),
+    ("", "ro-crate-metadata.jsonld"),
+    ("https://example.org/crate/", "ro-crate-metadata.json"),
+    ("https://example.org/crate/", "ro-crate-metadata.jsonld"),
+    ("", "bad-name.json"),
+])
+def test_find_root(tmpdir, root, basename):
+    metadata_id = root + basename
+    root_id = root or "./"
+    metadata = {
+        "@context": "https://w3id.org/ro/crate/1.1/context",
+        "@graph": [
+            {
+                "@id": metadata_id,
+                "@type": "CreativeWork",
+                "about": {"@id": root_id},
+                "conformsTo": [
+                    {"@id": "https://w3id.org/ro/crate/1.1"},
+                    {"@id": "https://example.org/fancy-ro-crate/1.0"},
+                ]
+            },
+            {
+                "@id": root_id,
+                "@type": "Dataset",
+            },
+        ]
+    }
+    crate_dir = tmpdir / "test_find_root"
+    crate_dir.mkdir()
+    # fixed filename, we only want the metadata entry to change
+    with open(crate_dir / "ro-crate-metadata.json", "wt") as f:
+        json.dump(metadata, f, indent=4)
+    if basename not in {"ro-crate-metadata.json", "ro-crate-metadata.jsonld"}:
+        with pytest.raises(KeyError):
+            ROCrate(crate_dir)
+    else:
+        crate = ROCrate(crate_dir)
+        assert crate.metadata.id == metadata_id
+        assert crate.root_dataset.id == root_id
+
+
+def test_find_root_bad_entities():
+    orig_entities = {
+        "ro-crate-metadata.json": {
+            "@id": "ro-crate-metadata.json",
+            "@type": "CreativeWork",
+            "about": {"@id": "./"},
+            "conformsTo": {"@id": "https://w3id.org/ro/crate/1.1"},
+        },
+        "./": {
+            "@id": "./",
+            "@type": "Dataset",
+        },
+    }
+    crate = ROCrate()
+    # missing "about"
+    entities = deepcopy(orig_entities)
+    del entities["ro-crate-metadata.json"]["about"]
+    with pytest.raises(ValueError, match="does not reference"):
+        crate.find_root_entity_id(entities)
+    # "about" does not reference the root entity
+    entities = deepcopy(orig_entities)
+    for about in "http://example.org", {"@id": "http://example.org"}:
+        entities["ro-crate-metadata.json"]["about"] = about
+        with pytest.raises(ValueError, match="does not reference"):
+            crate.find_root_entity_id(entities)
+    # metadata type is not CreativeWork
+    entities = deepcopy(orig_entities)
+    entities["ro-crate-metadata.json"]["@type"] = "Thing"
+    with pytest.raises(ValueError, match="must be of type"):
+        crate.find_root_entity_id(entities)
+    # root type is not Dataset
+    entities = deepcopy(orig_entities)
+    entities["./"]["@type"] = "Thing"
+    with pytest.raises(ValueError, match="must be of type"):
+        crate.find_root_entity_id(entities)
+
+
+@pytest.mark.filterwarnings("ignore")
+def test_find_root_multiple_entries():
+    orig_entities = {
+        "http://example.org/ro-crate-metadata.json": {
+            "@id": "http://example.org/ro-crate-metadata.json",
+            "@type": "CreativeWork",
+            "about": {"@id": "http://example.org/"},
+            "conformsTo": {"@id": "https://w3id.org/ro/crate/1.1"},
+        },
+        "http://example.org/": {
+            "@id": "http://example.org/",
+            "@type": "Dataset",
+            "hasPart": [
+                {"@id": "http://example.com/"},
+                {"@id": "http://example.com/ro-crate-metadata.json"}
+            ]
+        },
+        "http://example.com/ro-crate-metadata.json": {
+            "@id": "http://example.com/ro-crate-metadata.json",
+            "@type": "CreativeWork",
+            "about": {"@id": "http://example.com/"},
+            "conformsTo": {"@id": "https://w3id.com/ro/crate/1.1"},
+        },
+        "http://example.com/": {
+            "@id": "http://example.com/",
+            "@type": "Dataset",
+        },
+    }
+    crate = ROCrate()
+
+    def check_finds_org(entities):
+        m_id, r_id = crate.find_root_entity_id(entities)
+        assert m_id == "http://example.org/ro-crate-metadata.json"
+        assert r_id == "http://example.org/"
+
+    def check_picks_one(entities):
+        m_id, r_id = crate.find_root_entity_id(entities)
+        assert m_id in [f"http://example.{_}/ro-crate-metadata.json" for _ in ("org", "com")]
+        assert r_id in [f"http://example.{_}/" for _ in ("org", "com")]
+
+    check_finds_org(orig_entities)
+    # no root candidate contains the other one
+    mod_entities = deepcopy(orig_entities)
+    del mod_entities["http://example.org/"]["hasPart"]
+    check_picks_one(mod_entities)
+    # each root candidate contains the other one
+    mod_entities = deepcopy(orig_entities)
+    mod_entities["http://example.com/"]["hasPart"] = [
+        {"@id": "http://example.org/"},
+        {"@id": "http://example.org/ro-crate-metadata.json"}
+    ]
+    check_picks_one(mod_entities)
+    # "about" does not reference the root entity
+    mod_entities = deepcopy(orig_entities)
+    for about in "http://google.com", {"@id": "http://google.com"}:
+        mod_entities["http://example.com/ro-crate-metadata.json"]["about"] = about
+        check_finds_org(mod_entities)
+    # metadata type is not CreativeWork
+    mod_entities = deepcopy(orig_entities)
+    mod_entities["http://example.com/ro-crate-metadata.json"]["@type"] = "Thing"
+    check_finds_org(mod_entities)
+    # root type is not Dataset
+    mod_entities = deepcopy(orig_entities)
+    mod_entities["http://example.com/"]["@type"] = "Thing"
+    check_finds_org(mod_entities)