Repository: ResearchObject/ro-crate-py
Pull request #40 (2021-01-06 17:44:01)

Fix entity dereference
Fixes some problems with entity dereferencing, most notably an overly eager character stripping rule that was removing leading and trailing dots from entity names.


Patch:
diff --git a/README.md b/README.md
index ee18656..8e742d4 100644
--- a/README.md
+++ b/README.md
@@ -111,12 +111,12 @@ dataset_entity = crate.add_directory(sample_dir, 'relative/rocrate/path')
 
 ```python
 # Add authors info
-crate.add_person('#joe', {'name': 'Joe Bloggs'})
+crate.add(Person(crate, '#joe', {'name': 'Joe Bloggs'}))
 
 # wf_crate example
-publisher = wf_crate.add_person('001', {'name': 'Bert Verlinden'})
-
-creator = wf_crate.add_person('002', {'name': 'Lee Ritenour'})
+publisher = Person(crate, '001', {'name': 'Bert Verlinden'})
+creator = Person(crate, '002', {'name': 'Lee Ritenour'})
+wf_crate.add(publisher, creator)
 
 # These contextual entities can be assigned to other metadata properties:
 
diff --git a/rocrate/__init__.py b/rocrate/__init__.py
index e5787b6..f5edf6f 100644
--- a/rocrate/__init__.py
+++ b/rocrate/__init__.py
@@ -43,6 +43,9 @@ Copyright 2020 Center for Advanced Studies, Research and Development in Sardinia
 __license__ = ("Apache License, version 2.0 "
                "<https://www.apache.org/licenses/LICENSE-2.0>")
 
+# for arcp scheme registration with urllib.parse
+import arcp  # noqa
+
 # Convenience export of public functions/types
 from .model.metadata import Metadata  # noqa
 from ._version import __version__  # noqa
diff --git a/rocrate/model/contextentity.py b/rocrate/model/contextentity.py
index 7e91a22..e467b29 100644
--- a/rocrate/model/contextentity.py
+++ b/rocrate/model/contextentity.py
@@ -17,13 +17,11 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-from urllib.parse import urlparse
-
 from .. import vocabs
-from ..utils import as_list
+from ..utils import as_list, is_url
 
 from .entity import Entity
-from arcp import is_arcp_uri
+
 
 """
 A property class that can be used during class declaration
@@ -71,17 +69,12 @@ class ContextEntity(Entity):
         super(ContextEntity, self).__init__(crate, identifier, properties)
 
     def format_id(self, identifier):
-        if is_arcp_uri(identifier):
+        if is_url(identifier):
+            return identifier
+        elif identifier.startswith('#'):
             return identifier
         else:
-            # check if it's an absolute URL
-            url = urlparse(identifier)
-            if all([url.scheme, url.netloc, url.path]):
-                return identifier
-            elif identifier.startswith('#'):
-                return identifier
-            else:
-                return '#' + identifier
+            return '#' + identifier
 
     def getmany(self, instance):
         for json in as_list(instance.get(self.property)):
diff --git a/rocrate/model/data_entity.py b/rocrate/model/data_entity.py
index f761a41..7484d68 100644
--- a/rocrate/model/data_entity.py
+++ b/rocrate/model/data_entity.py
@@ -21,7 +21,9 @@ import os
 
 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)
diff --git a/rocrate/model/dataset.py b/rocrate/model/dataset.py
index 6cbd624..cc22fb0 100644
--- a/rocrate/model/dataset.py
+++ b/rocrate/model/dataset.py
@@ -17,7 +17,6 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-import datetime
 import os
 import pathlib
 import shutil
diff --git a/rocrate/model/entity.py b/rocrate/model/entity.py
index 7b75e77..bad0f6c 100644
--- a/rocrate/model/entity.py
+++ b/rocrate/model/entity.py
@@ -39,13 +39,9 @@ class Entity(object):
             self._jsonld = self._empty()
 
     # Format the given ID with rules appropriate for this type.
-    # For example:
-    #  * contextual entities MUST be absolute URIs, or begin with: #
-    #  * files MUST NOT begin with ./
-    #  * directories MUST NOT begin with ./ (except for the crate itself),
-    #    and MUST end with /
+    # For example, Dataset (directory) data entities SHOULD end with /
     def format_id(self, identifier):
-        return str(identifier).strip('./')
+        return str(identifier)
 
     def __repr__(self):
         return "<%s %s>" % (self.id, self.type)
@@ -70,7 +66,7 @@ class Entity(object):
         return self.crate.resolve_id(self.id)
 
     def hash(self):
-        hash(self.canonical_id)
+        hash(self.canonical_id())
 
     def _empty(self):
         val = {
diff --git a/rocrate/model/preview.py b/rocrate/model/preview.py
index fca6ea4..e9750b8 100644
--- a/rocrate/model/preview.py
+++ b/rocrate/model/preview.py
@@ -27,7 +27,7 @@ from .file import File
 class Preview(File):
     """
     RO-Crate preview file
-    
+
     This object holds a preview of an RO Crate in HTML format_
     """
     BASENAME = "ro-crate-preview.html"
@@ -46,13 +46,13 @@ class Preview(File):
 
     def generate_html(self):
         base_path = os.path.abspath(os.path.dirname(__file__))
-        template = open(os.path.join(base_path,'..' ,'templates', 'preview_template.html.j2'))
+        template = open(os.path.join(base_path, '..', 'templates', 'preview_template.html.j2'))
         src = Template(template.read())
 
         def template_function(func):
             src.globals[func.__name__] = func
             return func
-        
+
         @template_function
         def stringify(a):
             if type(a) is list:
@@ -61,7 +61,7 @@ class Preview(File):
                 return a
             else:
                 if a._jsonld and a._jsonld['name']:
-                    return  a._jsonld['name']
+                    return a._jsonld['name']
                 else:
                     return a
 
diff --git a/rocrate/rocrate.py b/rocrate/rocrate.py
index 2eccc45..7332bb9 100644
--- a/rocrate/rocrate.py
+++ b/rocrate/rocrate.py
@@ -28,11 +28,11 @@ import shutil
 import tempfile
 
 from pathlib import Path
+from urllib.parse import urljoin
 
 from .model import contextentity
 from .model.root_dataset import RootDataset
 from .model.file import File
-from .model.person import Person
 from .model.dataset import Dataset
 from .model.metadata import Metadata, LegacyMetadata
 from .model.preview import Preview
@@ -44,32 +44,28 @@ from .model.testservice import TestService  # noqa
 from .model.softwareapplication import SoftwareApplication  # noqa
 from .model.testsuite import TestSuite  # noqa
 
-
-from arcp import generate
+from .utils import is_url
 
 
 class ROCrate():
 
     def __init__(self, source_path=None, load_preview=False):
+        self.__entity_map = {}
         self.default_entities = []
         self.data_entities = []
         self.contextual_entities = []
         # TODO: add this as @base in the context? At least when loading
         # from zip
         self.uuid = uuid.uuid4()
+        self.arcp_base_uri = f"arcp://uuid,{self.uuid}/"
 
         # TODO: default_properties must include name, description,
         # datePublished, license
         if not source_path or not load_preview:
-            # create preview entity and add it to default_entities
-            self.preview = Preview(self)
-            self.default_entities.append(self.preview)
+            self.add(Preview(self))
         if not source_path:
             # create a new ro-crate
-            self.root_dataset = RootDataset(self)
-            self.default_entities.append(self.root_dataset)
-            self.metadata = Metadata(self)
-            self.default_entities.append(self.metadata)
+            self.add(RootDataset(self), Metadata(self))
         else:
             # load an existing ro-crate
             if zipfile.is_zipfile(source_path):
@@ -86,8 +82,7 @@ class ROCrate():
             if not os.path.isfile(metadata_path):
                 raise ValueError('The directory is not a valid RO-crate, '
                                  f'missing {Metadata.BASENAME}')
-            self.metadata = MetadataClass(self)
-            self.default_entities.append(self.metadata)
+            self.add(MetadataClass(self))
             entities = self.entities_from_metadata(metadata_path)
             self.build_crate(entities, source_path, load_preview)
             # TODO: load root dataset properties
@@ -141,7 +136,10 @@ class ROCrate():
         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")
+        raise KeyError(
+            "Can't find Root Data Entity in RO-Crate, "
+            "see https://www.researchobject.org/ro-crate/1.1/root-data-entity.html"
+        )
 
     def build_crate(self, entities, source, load_preview):
         # add data and contextual entities to the crate
@@ -153,14 +151,12 @@ class ROCrate():
         # properties to the build
         root_entity.pop('@id', None)
         root_entity.pop('hasPart', None)
-        self.root_dataset = RootDataset(self, root_entity)
-        self.default_entities.append(self.root_dataset)
+        self.add(RootDataset(self, root_entity))
 
         # check if a preview is present
         if Preview.BASENAME in entities.keys() and load_preview:
             preview_source = os.path.join(source, Preview.BASENAME)
-            self.preview = Preview(self, preview_source)
-            self.default_entities.append(self.preview)
+            self.add(Preview(self, preview_source))
 
         added_entities = []
         # iterate over data entities
@@ -203,7 +199,7 @@ class ROCrate():
                     instance = Dataset(self, dir_path, entity['@id'], props)
                 else:
                     raise Exception('Directory not found')
-            self._add_data_entity(instance)
+            self.add(instance)
             added_entities.append(data_entity_id)
 
         # the rest of the entities must be contextual entities
@@ -230,7 +226,7 @@ class ROCrate():
                     instance = contextentity.ContextEntity(
                         self, identifier, entity
                     )
-                self._add_context_entity(instance)
+                self.add(instance)
 
     # TODO: add contextual entities
     # def add_contact_point(id, properties = {})
@@ -335,12 +331,13 @@ class ROCrate():
             return rval
         return None
 
-    def resolve_id(self, relative_id):
-        return generate.arcp_random(relative_id.strip('./'), uuid=self.uuid)
+    def resolve_id(self, id_):
+        if not is_url(id_):
+            id_ = urljoin(self.arcp_base_uri, id_)  # also does path normalization
+        return id_.rstrip("/")
 
     def get_entities(self):
-        return (self.default_entities + self.data_entities +
-                self.contextual_entities)
+        return self.__entity_map.values()
 
     def set_main_entity(self, main_entity):
         self.root_dataset['mainEntity'] = main_entity
@@ -350,10 +347,7 @@ class ROCrate():
 
     def dereference(self, entity_id):
         canonical_id = self.resolve_id(entity_id)
-        for entity in self.get_entities():
-            if canonical_id == entity.canonical_id():
-                return entity
-        return None
+        return self.__entity_map.get(canonical_id, None)
 
     # source: file object or path (str)
     def add_file(self, source, crate_path=None, fetch_remote=False,
@@ -361,47 +355,35 @@ class ROCrate():
         props = dict(properties)
         props.update(kwargs)
         file_entity = File(self, source=source, dest_path=crate_path, fetch_remote=fetch_remote, properties=props)
-        self._add_data_entity(file_entity)
+        self.add(file_entity)
         return file_entity
 
-    def remove_file(self, file_id):
-        # if file in data_entities:
-        self._remove_data_entity(file_id)
-
     def add_directory(self, source, crate_path=None, properties={}, **kwargs):
         props = dict(properties)
         props.update(kwargs)
         dataset_entity = Dataset(self, source, crate_path, properties)
-        self._add_data_entity(dataset_entity)
+        self.add(dataset_entity)
         return dataset_entity
 
-    def remove_directory(self, dir_id):
-        # if file in data_entities:
-        self._remove_data_entity(dir_id)
-
-    def _add_data_entity(self, data_entity):
-        self._remove_data_entity(data_entity)
-        self.data_entities.append(data_entity)
-
-    def _remove_data_entity(self, data_entity):
-        if data_entity in self.data_entities:
-            self.data_entities.remove(data_entity)
-
-    ################################
-    #     Contextual entities      #
-    ################################
-
-    def _add_context_entity(self, entity):
-        if entity in self.contextual_entities:
-            self.contextual_entities.remove(entity)
-        self.contextual_entities.append(entity)
-
-    def add_person(self, identifier=None, properties={}, **kwargs):
-        props = dict(properties)
-        props.update(kwargs)
-        new_person = Person(self, identifier, props)
-        self._add_context_entity(new_person)
-        return new_person
+    def add(self, *entities):
+        for e in entities:
+            key = e.canonical_id()
+            # crate MUST NOT list multiple entities with the same @id
+            if key in self.__entity_map:
+                raise ValueError(f'duplicate entity id: "{key}"')
+            self.__entity_map[key] = e
+            if isinstance(e, RootDataset):
+                self.root_dataset = e
+            if isinstance(e, (Metadata, LegacyMetadata)):
+                self.metadata = e
+            if isinstance(e, Preview):
+                self.preview = e
+            if isinstance(e, (RootDataset, Metadata, LegacyMetadata, Preview)):
+                self.default_entities.append(e)
+            elif hasattr(e, "write"):
+                self.data_entities.append(e)
+            else:
+                self.contextual_entities.append(e)
 
     # TODO
     # def fetch_all(self):
diff --git a/rocrate/rocrate_api.py b/rocrate/rocrate_api.py
index c4f0654..abf51c6 100644
--- a/rocrate/rocrate_api.py
+++ b/rocrate/rocrate_api.py
@@ -69,7 +69,7 @@ def make_workflow_rocrate(workflow_path, wf_type, include_files=[],
     wf_path = Path(workflow_path)
     # should this be added in a special path within the crate?
     wf_file = Workflow(wf_crate, str(wf_path), wf_path.name)
-    wf_crate._add_data_entity(wf_file)
+    wf_crate.add(wf_file)
     wf_crate.set_main_entity(wf_file)
     if wf_type == 'CWL':
         programming_language_entity = entity.Entity(
@@ -91,7 +91,7 @@ def make_workflow_rocrate(workflow_path, wf_type, include_files=[],
             atexit.register(os.unlink, f.name)
             abstract_wf_id = wf_path.with_suffix(".cwl").name
             abstract_wf_file = Workflow(wf_crate, f.name, abstract_wf_id)
-            wf_crate._add_data_entity(abstract_wf_file)
+            wf_crate.add(abstract_wf_file)
             wf_file["subjectOf"] = abstract_wf_file
         programming_language_entity = entity.Entity(
             wf_crate, 'https://galaxyproject.org/'
diff --git a/rocrate/utils.py b/rocrate/utils.py
index d1f71cf..e3e6660 100644
--- a/rocrate/utils.py
+++ b/rocrate/utils.py
@@ -18,7 +18,7 @@
 # limitations under the License.
 
 import collections
-from datetime import date, datetime
+from urllib.parse import urlsplit
 
 
 def first(iterable):
@@ -40,3 +40,8 @@ def as_list(list_or_other):
         and not isinstance(list_or_other, str)):  # FIXME: bytes?
         return list_or_other
     return [list_or_other]
+
+
+def is_url(string):
+    parts = urlsplit(string)
+    return all((parts.scheme, parts.netloc, parts.path))
diff --git a/setup.cfg b/setup.cfg
index b29f537..61a5b16 100644
--- a/setup.cfg
+++ b/setup.cfg
@@ -1,2 +1,3 @@
 [flake8]
 ignore = E226,E741,E402,E129,W503,W504
+max-line-length = 127
diff --git a/tools/add_boilerplate.py b/tools/add_boilerplate.py
index b870aac..6dff655 100644
--- a/tools/add_boilerplate.py
+++ b/tools/add_boilerplate.py
@@ -105,7 +105,7 @@ def add_boilerplate(fn):
 def main():
     join = os.path.join
     for root, dirs, files in os.walk(TOP_DIR):
-        dirs[:] = [_ for _ in dirs if not _.startswith(".") and not _ in EXCLUDE_DIRS]
+        dirs[:] = [_ for _ in dirs if not _.startswith(".") and _ not in EXCLUDE_DIRS]
         for name in files:
             if not name.endswith(".py"):
                 continue


Test patch:
diff --git a/test/test_model.py b/test/test_model.py
index 84bea97..010bd90 100644
--- a/test/test_model.py
+++ b/test/test_model.py
@@ -18,11 +18,15 @@
 import datetime
 import uuid
 
+import pytest
 from rocrate.rocrate import ROCrate
 from rocrate.model.file import File
 from rocrate.model.person import Person
 
 
+RAW_REPO_URL = "https://raw.githubusercontent.com/ResearchObject/ro-crate-py"
+
+
 def test_dereferencing(test_data_dir, helpers):
     crate = ROCrate()
 
@@ -40,25 +44,34 @@ def test_dereferencing(test_data_dir, helpers):
     assert isinstance(file_returned, File)
     dereference_file = crate.dereference("sample_file.txt")
     assert file_returned is dereference_file
-
-
-def test_dereferencing_equivalent_id(helpers):
-    crate = ROCrate()
-
-    root_dataset = crate.dereference('./')
-    assert crate.root_dataset is root_dataset
-    root_dataset = crate.dereference('')
-    assert crate.root_dataset is root_dataset
-
-    metadata_entity = crate.dereference(helpers.METADATA_FILE_NAME)
-    assert crate.metadata is metadata_entity
-    metadata_entity = crate.dereference(f'./{helpers.METADATA_FILE_NAME}')
-    assert crate.metadata is metadata_entity
+    readme_url = f'{RAW_REPO_URL}/master/README.md'
+    readme_entity = crate.add_file(readme_url)
+    assert crate.dereference(readme_url) is readme_entity
+
+
+@pytest.mark.parametrize("name", [".foo", "foo.", ".foo/", "foo./"])
+def test_dereferencing_equivalent_id(test_data_dir, name):
+    test_ids = [name, f'./{name}', f'.//{name}', f'./a/../{name}']
+    path = test_data_dir / name
+    if name.endswith("/"):
+        path.mkdir()
+        test_ids.extend([f"{name}/", f"{name}//"])
+    else:
+        path.touch()
+    for id_ in test_ids:
+        crate = ROCrate()
+        if name.endswith("/"):
+            entity = crate.add_directory(path, name)
+        else:
+            entity = crate.add_file(path, name)
+        deref_entity = crate.dereference(id_)
+        assert deref_entity is entity
 
 
 def test_contextual_entities():
     crate = ROCrate()
-    new_person = crate.add_person('#joe', {'name': 'Joe Pesci'})
+    new_person = Person(crate, '#joe', {'name': 'Joe Pesci'})
+    crate.add(new_person)
     person_dereference = crate.dereference('#joe')
     assert person_dereference is new_person
     assert person_dereference.type == 'Person'
@@ -66,6 +79,11 @@ def test_contextual_entities():
     assert person_prop['@type'] == 'Person'
     assert person_prop['name'] == 'Joe Pesci'
     assert not new_person.datePublished
+    id_ = "https://orcid.org/0000-0002-1825-0097"
+    new_person = Person(crate, id_, {'name': 'Josiah Carberry'})
+    crate.add(new_person)
+    person_dereference = crate.dereference(id_)
+    assert person_dereference is new_person
 
 
 def test_properties():
@@ -86,14 +104,16 @@ def test_properties():
     crate.datePublished = crate_datePublished
     assert crate.datePublished == crate_datePublished
 
-    new_person = crate.add_person('#001', {'name': 'Lee Ritenour'})
+    new_person = Person(crate, '#001', {'name': 'Lee Ritenour'})
+    crate.add(new_person)
     crate.creator = new_person
     assert crate.creator is new_person
     assert isinstance(crate.creator, Person)
     assert crate.creator['name'] == 'Lee Ritenour'
     assert crate.creator.type == 'Person'
 
-    new_person2 = crate.add_person('#002', {'name': 'Lee Ritenour'})
+    new_person2 = Person(crate, '#002', {'name': 'Lee Ritenour'})
+    crate.add(new_person2)
     crate.creator = [new_person, new_person2]
     assert isinstance(crate.creator, list)
     assert crate.creator[0] is new_person
@@ -102,7 +122,8 @@ def test_properties():
 
 def test_uuid():
     crate = ROCrate()
-    new_person = crate.add_person(name="No Identifier")
+    new_person = Person(crate, properties={"name": "No Identifier"})
+    crate.add(new_person)
     jsonld = new_person.as_jsonld()
     assert "Person" == jsonld["@type"]
     assert jsonld["@id"].startswith("#")
diff --git a/test/test_test_metadata.py b/test/test_test_metadata.py
index 8072620..6237999 100644
--- a/test/test_test_metadata.py
+++ b/test/test_test_metadata.py
@@ -94,14 +94,14 @@ def test_create():
     crate = ROCrate()
 
     test_service = TestService(crate, identifier=JENKINS)
-    crate._add_context_entity(test_service)
+    crate.add(test_service)
     test_service.name = "Jenkins"
     test_service.url = {"@id": "https://www.jenkins.io"}
     assert test_service.name == "Jenkins"
     assert test_service.url == "https://www.jenkins.io"
 
     test_instance = TestInstance(crate, identifier="#foo_instance_1")
-    crate._add_context_entity(test_instance)
+    crate.add(test_instance)
     test_instance.name = "Foo Instance 1"
     test_instance.url = {"@id": "http://example.org/foo"}
     test_instance.resource = "job/foobar"
@@ -115,14 +115,14 @@ def test_create():
     assert test_instance.service is test_service
 
     test_engine = SoftwareApplication(crate, identifier=PLANEMO)
-    crate._add_context_entity(test_engine)
+    crate.add(test_engine)
     test_engine.name = "Planemo"
     test_engine.url = {"@id": "https://github.com/galaxyproject/planemo"}
     assert test_engine.name == "Planemo"
     assert test_engine.url == "https://github.com/galaxyproject/planemo"
 
     test_definition = TestDefinition(crate, dest_path="test/foo/bar.yml")
-    crate._add_context_entity(test_definition)
+    crate.add(test_definition)
     test_definition.conformsTo = test_service
     assert test_definition.conformsTo is test_service
     test_definition.conformsTo = None
@@ -132,7 +132,7 @@ def test_create():
     assert test_definition.engineVersion == ">=0.70"
 
     test_suite = TestSuite(crate, "#foosuite")
-    crate._add_data_entity(test_suite)
+    crate.add(test_suite)
     assert test_suite.id == "#foosuite"
     assert test_suite.type == "TestSuite"
     test_suite.name = "Foo Suite"
diff --git a/test/test_write.py b/test/test_write.py
index 1570f36..ec4e672 100644
--- a/test/test_write.py
+++ b/test/test_write.py
@@ -20,6 +20,7 @@ import pytest
 import zipfile
 
 from rocrate.model.dataset import Dataset
+from rocrate.model.person import Person
 from rocrate.rocrate import ROCrate
 
 
@@ -30,7 +31,8 @@ def test_file_writing(test_data_dir, tmpdir, helpers, to_zip):
     crate.name = crate_name
     creator_id = '001'
     creator_name = 'Lee Ritenour'
-    new_person = crate.add_person(creator_id, {'name': creator_name})
+    new_person = Person(crate, creator_id, {'name': creator_name})
+    crate.add(new_person)
     crate.creator = new_person
 
     sample_file_id = 'sample_file.txt'