Repository: ResearchObject/ro-crate-py

Issue:
Support percent-encoding of data entity paths
In [Describing entities in JSON-LD](https://www.researchobject.org/ro-crate/specification/1.1/appendix/jsonld.html#describing-entities-in-json-ld) the spec says:

> Care must be taken to express any relative paths using `/` separator and escape special characters like space (`%20`).

We don't have automatic escaping / unescaping:

```pycon
>>> from rocrate.rocrate import ROCrate
>>> crate = ROCrate()
>>> f = crate.add_file("example1/2017-06-11 12.56.14.jpg")
>>> f.id
'2017-06-11 12.56.14.jpg'
>>> crate.write("example1-crate")
```

In the above case, in the output crate the file's `@id` is `"2017-06-11 12.56.14.jpg"`. Also, this cannot be worked around by doing something like:

```pycon
>>> from rocrate.rocrate import ROCrate
>>> crate = ROCrate()
>>> f = crate.add_file("example1/2017-06-11 12.56.14.jpg", dest_path="2017-06-11%2012.56.14.jpg")
>>> f.id
'2017-06-11%2012.56.14.jpg'
>>> crate.write("example1-crate-2")
```

This time, in the output crate the file's `@id` is `"2017-06-11%2012.56.14.jpg"`, but *the name of the file stored inside the crate* is also `"2017-06-11%2012.56.14.jpg"`, instead of `"2017-06-11 12.56.14.jpg"`.


Discussion:

Patch:
diff --git a/README.md b/README.md
index 3800030..87feb62 100644
--- a/README.md
+++ b/README.md
@@ -166,6 +166,22 @@ for n in "Mickey_Mouse", "Scrooge_McDuck":
     donald.append_to("follows", p)
 ```
 
+#### Handling of special characters
+
+Since RO-Crate entity identifiers are URIs (relative or absolute), special characters in them must be percent-encoded. When adding data entities from the local file system, this is handled automatically by the library:
+
+```pycon
+>>> crate = ROCrate()
+>>> d = crate.add_dataset("read_crate/a b")
+>>> d.id
+'a%20b/'
+>>> f = crate.add_file("read_crate/a b/c d.txt", dest_path="data/a b/c d.txt")
+>>> f.id
+'data/a%20b/c%20d.txt'
+```
+
+When adding an entity whose identifier is an absolute URI (see next section), on the other hand, the identifier provided by the user is expected to be a valid URI, in particular with any special characters in path components already percent-encoded.
+
 #### Adding remote entities
 
 Data entities can also be remote:
diff --git a/rocrate/model/dataset.py b/rocrate/model/dataset.py
index 717d6f1..2832607 100644
--- a/rocrate/model/dataset.py
+++ b/rocrate/model/dataset.py
@@ -26,6 +26,7 @@ import os
 import warnings
 from pathlib import Path
 from urllib.request import urlopen
+from urllib.parse import unquote
 
 from .file_or_dir import FileOrDir
 from ..utils import is_url, iso_now
@@ -63,17 +64,18 @@ class Dataset(FileOrDir):
                 out_file.close()
 
     def _copy_folder(self, base_path):
-        abs_out_path = base_path / self.id
+        abs_out_path = base_path / unquote(self.id)
         if self.source is None:
             abs_out_path.mkdir(parents=True, exist_ok=True)
         else:
-            if not Path(self.source).exists():
+            path = unquote(str(self.source))
+            if not Path(path).exists():
                 raise FileNotFoundError(
-                    errno.ENOENT, os.strerror(errno.ENOENT), str(self.source)
+                    errno.ENOENT, os.strerror(errno.ENOENT), path
                 )
             abs_out_path.mkdir(parents=True, exist_ok=True)
             if not self.crate.source:
-                self.crate._copy_unlisted(self.source, abs_out_path)
+                self.crate._copy_unlisted(path, abs_out_path)
 
     def write(self, base_path):
         base_path = Path(base_path)
@@ -91,16 +93,17 @@ class Dataset(FileOrDir):
             yield from self._stream_folder_from_path(chunk_size)
 
     def _stream_folder_from_path(self, chunk_size=8192):
-        if not Path(str(self.source)).exists():
+        path = unquote(str(self.source))
+        if not Path(path).exists():
             raise FileNotFoundError(
-                errno.ENOENT, os.strerror(errno.ENOENT), str(self.source)
+                errno.ENOENT, os.strerror(errno.ENOENT), str(path)
             )
         if not self.crate.source:
-            for root, _, files in os.walk(self.source):
+            for root, _, files in os.walk(path):
                 root = Path(root)
                 for name in files:
                     source = root / name
-                    dest = source.relative_to(Path(self.source).parent)
+                    dest = source.relative_to(Path(path).parent)
                     with open(source, 'rb') as f:
                         while chunk := f.read(chunk_size):
                             yield str(dest), chunk
diff --git a/rocrate/model/file.py b/rocrate/model/file.py
index bfe4bc5..8ecfbc9 100644
--- a/rocrate/model/file.py
+++ b/rocrate/model/file.py
@@ -27,6 +27,7 @@ import shutil
 import urllib.request
 import warnings
 from io import BytesIO, StringIO
+from urllib.parse import unquote
 
 from .file_or_dir import FileOrDir
 from ..utils import is_url, iso_now
@@ -62,6 +63,7 @@ class File(FileOrDir):
                 out_file.write(chunk)
 
     def _copy_file(self, path, out_file_path):
+        path = unquote(str(path))
         out_file_path.parent.mkdir(parents=True, exist_ok=True)
         if not out_file_path.exists() or not out_file_path.samefile(path):
             shutil.copy(path, out_file_path)
@@ -69,7 +71,7 @@ class File(FileOrDir):
             self._jsonld['contentSize'] = str(out_file_path.stat().st_size)
 
     def write(self, base_path):
-        out_file_path = Path(base_path) / self.id
+        out_file_path = Path(base_path) / unquote(self.id)
         if isinstance(self.source, (BytesIO, StringIO)) or is_url(str(self.source)):
             self._write_from_stream(out_file_path)
         elif self.source is None:
@@ -117,6 +119,7 @@ class File(FileOrDir):
                     self._jsonld['contentSize'] = str(size)
 
     def _stream_from_file(self, path, chunk_size=8192):
+        path = unquote(str(path))
         size = 0
         with open(path, 'rb') as f:
             while chunk := f.read(chunk_size):
diff --git a/rocrate/model/file_or_dir.py b/rocrate/model/file_or_dir.py
index 31eee61..b7751d3 100644
--- a/rocrate/model/file_or_dir.py
+++ b/rocrate/model/file_or_dir.py
@@ -23,6 +23,7 @@
 
 import os
 from pathlib import Path
+from urllib.parse import quote
 
 from .data_entity import DataEntity
 from ..utils import is_url
@@ -43,6 +44,8 @@ class FileOrDir(DataEntity):
             if dest_path.is_absolute():
                 raise ValueError("if provided, dest_path must be relative")
             identifier = dest_path.as_posix()
+            if not crate.source:
+                identifier = quote(identifier)
         else:
             if not isinstance(source, (str, Path)):
                 raise ValueError("dest_path must be provided if source is not a path or URI")
@@ -50,4 +53,6 @@ class FileOrDir(DataEntity):
                 identifier = os.path.basename(source) if fetch_remote else source
             else:
                 identifier = os.path.basename(str(source).rstrip("/"))
+                if not crate.source:
+                    identifier = quote(identifier)
         super().__init__(crate, identifier, properties)
diff --git a/rocrate/rocrate.py b/rocrate/rocrate.py
index cf439a6..b602873 100644
--- a/rocrate/rocrate.py
+++ b/rocrate/rocrate.py
@@ -78,6 +78,7 @@ def pick_type(json_entity, type_map, fallback=None):
 class ROCrate():
 
     def __init__(self, source=None, gen_preview=False, init=False, exclude=None):
+        self.source = source
         self.exclude = exclude
         self.__entity_map = {}
         # TODO: add this as @base in the context? At least when loading


Test patch:
diff --git a/test/test-data/read_crate/a b/c d.txt b/test/test-data/read_crate/a b/c d.txt
new file mode 100644
index 0000000..d463417
--- /dev/null
+++ b/test/test-data/read_crate/a b/c d.txt
@@ -0,0 +1,1 @@
+C D
diff --git a/test/test-data/read_crate/ro-crate-metadata.json b/test/test-data/read_crate/ro-crate-metadata.json
index 8eb80ee..824293e 100644
--- a/test/test-data/read_crate/ro-crate-metadata.json
+++ b/test/test-data/read_crate/ro-crate-metadata.json
@@ -33,6 +33,12 @@
                 },
                 {
                     "@id": "test/"
+                },
+                {
+                    "@id": "with%20space.txt"
+                },
+                {
+                    "@id": "a%20b/"
                 }
             ],
             "mainEntity": {
@@ -88,6 +94,14 @@
         {
             "@id": "test/",
             "@type": "Dataset"
+        },
+        {
+            "@id": "with%20space.txt",
+            "@type": "File"
+        },
+        {
+            "@id": "a%20b/",
+            "@type": "Dataset"
         }
     ]
 }
diff --git a/test/test-data/read_crate/with space.txt b/test/test-data/read_crate/with space.txt
new file mode 100644
index 0000000..aafbb61
--- /dev/null
+++ b/test/test-data/read_crate/with space.txt
@@ -0,0 +1,1 @@
+Test handling of special character (un)escaping
diff --git a/test/test_model.py b/test/test_model.py
index 86c5f5b..fd0b864 100644
--- a/test/test_model.py
+++ b/test/test_model.py
@@ -276,7 +276,6 @@ def test_delete_refs(test_data_dir, tmpdir, helpers):
     crate.delete(definition)
     assert suite.definition is not definition  # so far, so good
     assert suite.definition == str(def_path)  # should probably be set to None
-    crate.write("/tmp/crate_out")
     # check json output
     out_path = tmpdir / "ro_crate_out"
     crate.write(out_path)
diff --git a/test/test_read.py b/test/test_read.py
index a205a48..98b812e 100644
--- a/test/test_read.py
+++ b/test/test_read.py
@@ -54,6 +54,8 @@ def test_crate_dir_loading(test_data_dir, tmpdir, helpers, gen_preview, from_zip
         "https://raw.githubusercontent.com/ResearchObject/ro-crate-py/master/test/test-data/sample_file.txt",
         "examples/",
         "test/",
+        "with%20space.txt",
+        "a%20b/",
     }
     assert set(_["@id"] for _ in crate.contextual_entities) == {"#joe"}
 
diff --git a/test/test_write.py b/test/test_write.py
index 0f31af1..205bc09 100644
--- a/test/test_write.py
+++ b/test/test_write.py
@@ -500,3 +500,26 @@ def test_stream(test_data_dir, tmpdir):
     assert (extract_path / "ro-crate-metadata.jsonld").is_file()
     assert (extract_path / "examples" / "README.txt").is_file()
     assert (extract_path / "test" / "test-metadata.json").is_file()
+
+
+def test_percent_escape(test_data_dir, tmpdir, helpers):
+    crate = ROCrate()
+    f_path = test_data_dir / "read_crate" / "with space.txt"
+    f1 = crate.add_file(f_path)
+    assert f1.id == "with%20space.txt"
+    f2 = crate.add_file(f_path, dest_path="subdir/with space.txt")
+    assert f2.id == "subdir/with%20space.txt"
+    d_path = test_data_dir / "read_crate" / "a b"
+    d1 = crate.add_dataset(d_path)
+    assert d1.id == "a%20b/"
+    d2 = crate.add_dataset(d_path, dest_path="subdir/a b")
+    assert d2.id == "subdir/a%20b/"
+    out_path = tmpdir / "ro_crate_out"
+    crate.write(out_path)
+    json_entities = helpers.read_json_entities(out_path)
+    assert "with%20space.txt" in json_entities
+    assert "subdir/with%20space.txt" in json_entities
+    assert "a%20b/" in json_entities
+    assert "subdir/a%20b/" in json_entities
+    assert (out_path / "a b" / "c d.txt").is_file()
+    assert (out_path / "subdir" / "a b" / "c d.txt").is_file()