Repository: ResearchObject/ro-crate-py

Issue:
Auto-generate `File` metadata fields
If you `crate.add_file()` a local file, I think it makes sense to populate:
* [`contentSize`](https://schema.org/contentSize)
* [`sha256`](https://schema.org/sha256)

This seems to be done already for remote files, so it makes sense to do the same for local ones.

Some other fields like `height`, `width` and `duration` could be automatically determined for images and videos respectively, but this is a much harder and less essential.

Happy to help with this!

Hints from issue comments:
@rsirvent: On contentSize I agree, it's just a system call to get a number.

However, generating the sha256 of a file can take quite some time, especially if the file is large. Imagine if you have to do the same for 10000 files... This could lead to unwanted overheads when creating an RO-Crate.@multimeric: Maybe a boolean option to calculate the `sha256`, which defaults to false then?@simleo: Even a system call can introduce a consistent overhead, so we'd need boolean options for both properties. However, the `FileOrDir` and `File` code are already complicated enough, and a library user that needs to set those properties can easily compute them in client code and pass them through the `properties` argument:

```python
size = ...
checksum = ...
crate.add_file(source, dest, properties={
    "contentSize": size,
    "sha256": checksum
})
```

It's better for the library to stay lean and leave optional stuff like this to client code.

In the case of remote files, note that `contentSize` is only added if `validate_url` is `True` and it's copied from the `Content-Length` HTTP header that we already have from checking the URL.
@multimeric: So that's a no to adding these arguments? The annoying thing for end users is that we have to do the size and checksum calculation for every file, and there are likely to be many of these. So without a built-in argument, I suspect many of us will end up implementing our own wrapper functions to do this.@simleo: > So that's a no to adding these arguments? The annoying thing for end users is that we have to do the size and checksum calculation for every file, and there are likely to be many of these. So without a built-in argument, I suspect many of us will end up implementing our own wrapper functions to do this.

Well, those properties are not required by the RO-Crate spec at any level, so some end users might want to add them while others might not. I'd rather not add the burden of more code to maintain to the library for something that's optional. I'll leave this open for others to chime in.@simleo: Other things to consider:
* `sha256` is not in the [RO-Crate 1.1 context](https://www.researchobject.org/ro-crate/specification/1.1/context.jsonld) (but it's present in the forthcoming 1.2)
* One might want to use other checksum algorithms (e.g., `sha512`). These are also not in the RO-Crate 1.1 context (but several are present in the [workflow-run context](https://github.com/ResearchObject/ro-terms/blob/master/workflow-run/context.jsonld))
@kinow: I think having flags to turning these fields (and others, width/height/etc.) would be the way to go.

On having ro-crate-py doing it, or staying lean and asking clients to implement this, maybe there could be other options too. Like having plugins in ro-crate-py, like ro-crate-py-fileutils or so. When installed, then that brings code that tries to populate file information, mime type, whatnot.

This way ro-crate-py focuses only on RO-Crate and Python, and anything more specific but that helps users/implementation-devs would go to these plug-ins, and implementations can decide to use it or not. @simleo: Discussed with @stain last Thursday at the Workflow Run RO-Crate meeting. We decided to add `contentSize`: while it's not mentioned explicitly in the spec, the property shows up in many examples, so we can consider it kind of a recommendation. The implementation is in #201. Notes:

* The `record_size` flag is in `add_file` and is propagated to `FileOrDir.__init__`, but the `contentSize` property is actually added in `File.write` when the file is written to disk. If we added it when the file entity is created, it could get invalidated multiple times (e.g. if data is appended to the file before the crate is written).
* I ran a quick performance test that involved creating an RO-Crate with 10K files: the process is about 15% slower when `contentSize` is added to each `File` entity. For this reason I introduced the `record_size` flag and set its default to `False`.

Regarding the recording of a checksum such as sha256, we observed that:

* it's not mentioned in the RO-Crate spec at all
* there are too many types, and the terms are not in the RO-Crate context
* users might want to record more than one checksum, e.g., sha256 and sha512.

So this is better left to user-level code.
@kinow: A fair approach! Thanks for summarizing it here, @simleo ! @simleo: Implemented in #201

Solution patch:
diff --git a/rocrate/model/file.py b/rocrate/model/file.py
index 21295d5d..05e132a3 100644
--- a/rocrate/model/file.py
+++ b/rocrate/model/file.py
@@ -45,7 +45,10 @@ def write(self, base_path):
             out_file_path.parent.mkdir(parents=True, exist_ok=True)
             mode = 'w' + ('b' if isinstance(self.source, BytesIO) else 't')
             with open(out_file_path, mode) as out_file:
-                out_file.write(self.source.getvalue())
+                content = self.source.getvalue()
+                out_file.write(content)
+                if self.record_size:
+                    self._jsonld['contentSize'] = str(len(content))
         elif is_url(str(self.source)):
             if self.fetch_remote or self.validate_url:
                 if self.validate_url:
@@ -62,6 +65,8 @@ def write(self, base_path):
                     out_file_path.parent.mkdir(parents=True, exist_ok=True)
                     urllib.request.urlretrieve(self.source, out_file_path)
                     self._jsonld['contentUrl'] = str(self.source)
+                    if self.record_size:
+                        self._jsonld['contentSize'] = str(out_file_path.stat().st_size)
         elif self.source is None:
             # Allows to record a File entity whose @id does not exist, see #73
             warnings.warn(f"No source for {self.id}")
@@ -69,3 +74,5 @@ def write(self, base_path):
             out_file_path.parent.mkdir(parents=True, exist_ok=True)
             if not out_file_path.exists() or not out_file_path.samefile(self.source):
                 shutil.copy(self.source, out_file_path)
+            if self.record_size:
+                self._jsonld['contentSize'] = str(out_file_path.stat().st_size)
diff --git a/rocrate/model/file_or_dir.py b/rocrate/model/file_or_dir.py
index 743ac19c..5f192f00 100644
--- a/rocrate/model/file_or_dir.py
+++ b/rocrate/model/file_or_dir.py
@@ -29,11 +29,12 @@
 class FileOrDir(DataEntity):
 
     def __init__(self, crate, source=None, dest_path=None, fetch_remote=False,
-                 validate_url=False, properties=None):
+                 validate_url=False, properties=None, record_size=False):
         if properties is None:
             properties = {}
         self.fetch_remote = fetch_remote
         self.validate_url = validate_url
+        self.record_size = record_size
         self.source = source
         if dest_path:
             dest_path = Path(dest_path)
diff --git a/rocrate/rocrate.py b/rocrate/rocrate.py
index 2adefa2a..26d52960 100644
--- a/rocrate/rocrate.py
+++ b/rocrate/rocrate.py
@@ -342,7 +342,8 @@ def add_file(
             dest_path=None,
             fetch_remote=False,
             validate_url=False,
-            properties=None
+            properties=None,
+            record_size=False
     ):
         return self.add(File(
             self,
@@ -350,7 +351,8 @@ def add_file(
             dest_path=dest_path,
             fetch_remote=fetch_remote,
             validate_url=validate_url,
-            properties=properties
+            properties=properties,
+            record_size=record_size
         ))
 
     def add_dataset(
@@ -478,11 +480,12 @@ def write_zip(self, out_path):
 
     def add_workflow(
             self, source=None, dest_path=None, fetch_remote=False, validate_url=False, properties=None,
-            main=False, lang="cwl", lang_version=None, gen_cwl=False, cls=ComputationalWorkflow
+            main=False, lang="cwl", lang_version=None, gen_cwl=False, cls=ComputationalWorkflow,
+            record_size=False
     ):
         workflow = self.add(cls(
             self, source=source, dest_path=dest_path, fetch_remote=fetch_remote,
-            validate_url=validate_url, properties=properties
+            validate_url=validate_url, properties=properties, record_size=record_size
         ))
         if isinstance(lang, ComputerLanguage):
             assert lang.crate is self
@@ -503,7 +506,7 @@ def add_workflow(
             cwl_dest_path = Path(source).with_suffix(".cwl").name
             cwl_workflow = self.add_workflow(
                 source=cwl_source, dest_path=cwl_dest_path, fetch_remote=fetch_remote, properties=properties,
-                main=False, lang="cwl", gen_cwl=False, cls=WorkflowDescription
+                main=False, lang="cwl", gen_cwl=False, cls=WorkflowDescription, record_size=record_size
             )
             workflow.subjectOf = cwl_workflow
         return workflow
@@ -542,12 +545,12 @@ def add_test_instance(self, suite, url, resource="", service="jenkins", identifi
 
     def add_test_definition(
             self, suite, source=None, dest_path=None, fetch_remote=False, validate_url=False, properties=None,
-            engine="planemo", engine_version=None
+            engine="planemo", engine_version=None, record_size=False
     ):
         suite = self.__validate_suite(suite)
         definition = self.add(
             TestDefinition(self, source=source, dest_path=dest_path, fetch_remote=fetch_remote,
-                           validate_url=validate_url, properties=properties)
+                           validate_url=validate_url, properties=properties, record_size=record_size)
         )
         if isinstance(engine, SoftwareApplication):
             assert engine.crate is self


Test patch:
diff --git a/test/test_write.py b/test/test_write.py
index a03b1be1..cc98a0e6 100644
--- a/test/test_write.py
+++ b/test/test_write.py
@@ -49,7 +49,7 @@ def test_file_writing(test_data_dir, tmpdir, helpers, gen_preview, to_zip):
     file_subdir_id = 'sample_file_subdir.txt'
 
     sample_file = test_data_dir / sample_file_id
-    file_returned = crate.add_file(sample_file)
+    file_returned = crate.add_file(sample_file, record_size=True)
     assert file_returned.id == sample_file_id
     file_returned_subdir = crate.add_file(sample_file, sample_file2_id)
     assert file_returned_subdir.id == sample_file2_id
@@ -99,6 +99,8 @@ def test_file_writing(test_data_dir, tmpdir, helpers, gen_preview, to_zip):
     assert json_entities[formatted_creator_id]["name"] == creator_name
     if gen_preview:
         assert helpers.PREVIEW_FILE_NAME in json_entities
+    file_entity = json_entities[sample_file_id]
+    assert file_entity["contentSize"] == str(file1.stat().st_size)
 
 
 @pytest.mark.parametrize("stream_cls", [io.BytesIO, io.StringIO])
@@ -106,8 +108,8 @@ def test_in_mem_stream(stream_cls, tmpdir, helpers):
     crate = ROCrate()
 
     test_file_id = 'a/b/test_file.txt'
-    file_content = b'\x00\x01' if stream_cls is io.BytesIO else 'foo\n'
-    file_returned = crate.add_file(stream_cls(file_content), test_file_id)
+    file_content = b'\x00\x01\x02' if stream_cls is io.BytesIO else 'foo'
+    file_returned = crate.add_file(stream_cls(file_content), test_file_id, record_size=True)
     assert file_returned.id == test_file_id
 
     out_path = tmpdir / 'ro_crate_out'
@@ -121,6 +123,9 @@ def test_in_mem_stream(stream_cls, tmpdir, helpers):
     mode = 'r' + ('b' if stream_cls is io.BytesIO else 't')
     with open(file1, mode) as f:
         assert f.read() == file_content
+    json_entities = helpers.read_json_entities(out_path)
+    file_entity = json_entities[test_file_id]
+    assert file_entity['contentSize'] == '3'
 
 
 @pytest.mark.parametrize(
@@ -132,7 +137,11 @@ def test_remote_uri(tmpdir, helpers, fetch_remote, validate_url, to_zip):
     url = ('https://raw.githubusercontent.com/ResearchObject/ro-crate-py/'
            'master/test/test-data/sample_file.txt')
     relpath = "a/b/sample_file.txt"
-    kw = {"fetch_remote": fetch_remote, "validate_url": validate_url}
+    kw = {
+        "fetch_remote": fetch_remote,
+        "validate_url": validate_url,
+        "record_size": True,
+    }
     if fetch_remote:
         file_ = crate.add_file(url, relpath, **kw)
         assert file_.id == relpath
@@ -154,6 +163,7 @@ def test_remote_uri(tmpdir, helpers, fetch_remote, validate_url, to_zip):
         out_file = out_crate.dereference(file_.id)
         assert (out_path / relpath).is_file()
         assert out_file["contentUrl"] == url
+        assert out_file["contentSize"] == str((out_path / file_.id).stat().st_size)
     else:
         out_file = out_crate.dereference(url)
         assert not (out_path / relpath).exists()