Repository: ResearchObject/ro-crate-py

Issue:
Setting/Accessing nested properties on entities without @id
RO-Crates aim to write the metadata file as flattend and compacted JSON-LD. I have noticed some inconsistencies with enforcing this behavior in this library. 

Currently, it is possible to write nested properties to an entity, even if this property is a dictionary that does not contain an `@id`. Here is an example:

```python
from rocrate.rocrate import ROCrate

crate = ROCrate()

# setting a nested property throws no error
crate.root_dataset["license"] = {
    "@type": "CreativeWork",
    "name": "CC-O",
    "url": "https://spdx.org/licenses/CC0-1.0"
}

crate.write("mycrate")
```

While the crate is written to disk without errors, attempting to access such a nested property once set from an ROCrate object raises an exception:
```python
crate.root_dataset["license"]
```
raises:
```bash
---------------------------------------------------------------------------
KeyError                                  Traceback (most recent call last)
File ~/projects/playground/venv/lib/python3.12/site-packages/rocrate/model/entity.py:91, in Entity.__getitem__(self, key)
     90 try:
---> 91     id_ = entry["@id"]
     92 except KeyError:

KeyError: '@id'

During handling of the above exception, another exception occurred:

ValueError                                Traceback (most recent call last)
Cell In[32], line 1
----> 1 crate.root_dataset["license"]

File ~/projects/playground/venv/lib/python3.12/site-packages/rocrate/model/entity.py:93, in Entity.__getitem__(self, key)
     91     id_ = entry["@id"]
     92 except KeyError:
---> 93     raise ValueError(f"no @id in {entry}")
     94 else:
     95     deref_values.append(self.crate.get(id_, id_))

ValueError: no @id in {'@type': 'CreativeWork', 'name': 'CC-O', 'url': 'https://spdx.org/licenses/CC0-1.0'}
```

To enforce consistency and adhere to the intended behaviour, I suggest to implement a check when assigning a property that makes sure that the `@id` key is present when the property value is a dictionary. It could even be considered to check if the id corresponds to an entity present in the crate.
When accessing values from an RO Crate, it might make sense to be more forgiving and return the property if it has no id or the id does not correspond to an entity.

These changes would prevent the library from creating RO-Crates that cannot be accessed with the very same library afterwards.

Discussion:
@simleo: > To enforce consistency and adhere to the intended behaviour, I suggest to implement a check when assigning a property that makes sure that the `@id` key is present when the property value is a dictionary

Done in #193.

> It could even be considered to check if the id corresponds to an entity present in the crate

This cannot be done. `"a": {"@id": "b"}` properties are allowed even if there's no `"b"` entity in the crate: for instance, `"conformsTo": {"@id": "https://w3id.org/ro/crate/1.1"}` in the `"ro-crate-metadata.json"` entity. Moreover, after #192, `__setitem__` is called in `Entity.__init__`, and the `"b"` entity could be added to the crate after the `"a"` one has been initialized.

> When accessing values from an RO Crate, it might make sense to be more forgiving and return the property if it has no id or the id does not correspond to an entity

In theory we could generate a random id and add the entity to the crate, but then this would need to work recursively, we'd have to detect cycles etc. The code would become much more complex to essentially support something that's not meant to be supported. An input RO-Crate must be flattened, and if it's not the behavior is undefined.

> These changes would prevent the library from creating RO-Crates that cannot be accessed with the very same library afterwards

Well, even after #193, one could do tricks like `e._jsonld["p"] = {"k": "v"}` (in fact, I used this trick in the unit test in #193). But if you modify the `_jsonld` attribute you should know what you're doing.

Solution patch:
diff --git a/rocrate/model/entity.py b/rocrate/model/entity.py
index 22e69a55..fc23eada 100644
--- a/rocrate/model/entity.py
+++ b/rocrate/model/entity.py
@@ -102,6 +102,9 @@ def __setitem__(self, key: str, value):
         if key.startswith("@"):
             raise KeyError(f"cannot set '{key}'")
         values = value if isinstance(value, list) else [value]
+        for v in values:
+            if isinstance(v, dict) and "@id" not in v:
+                raise ValueError(f"no @id in {v}")
         ref_values = [{"@id": _.id} if isinstance(_, Entity) else _ for _ in values]
         self._jsonld[key] = ref_values if isinstance(value, list) else ref_values[0]
 
diff --git a/rocrate/rocrate.py b/rocrate/rocrate.py
index 5597d54f..63f2eb34 100644
--- a/rocrate/rocrate.py
+++ b/rocrate/rocrate.py
@@ -556,6 +556,22 @@ def add_test_definition(
         self.metadata.extra_terms.update(TESTING_EXTRA_TERMS)
         return definition
 
+    def add_action(self, instrument, identifier=None, object=None, result=None, properties=None):
+        if properties is None:
+            properties = {}
+        if "@type" not in properties:
+            properties["@type"] = "CreateAction"
+        action = self.add(ContextEntity(self, identifier, properties=properties))
+        action["instrument"] = instrument
+        if "name" not in properties:
+            action.name = action.id.lstrip("#")
+        if object:
+            action["object"] = object
+        if result:
+            action["result"] = result
+        self.root_dataset.append_to("mentions", action)
+        return action
+
     def add_jsonld(self, jsonld):
         """Add a JSON-LD dictionary as a contextual entity to the RO-Crate.
 


Test patch:
diff --git a/test/test_model.py b/test/test_model.py
index 4465ecee..c452e211 100644
--- a/test/test_model.py
+++ b/test/test_model.py
@@ -363,6 +363,11 @@ def test_entity_as_mapping(tmpdir, helpers):
     }
     crate_dir = tmpdir / "in_crate"
     crate_dir.mkdir()
+    with open(crate_dir / helpers.METADATA_FILE_NAME, "wt") as f:
+        json.dump(metadata, f, indent=4)
+    with pytest.raises(ValueError):  # due to "badProp", which has no "@id"
+        crate = ROCrate(crate_dir)
+    del metadata["@graph"][2]["badProp"]
     with open(crate_dir / helpers.METADATA_FILE_NAME, "wt") as f:
         json.dump(metadata, f, indent=4)
     crate = ROCrate(crate_dir)
@@ -411,6 +416,9 @@ def test_entity_as_mapping(tmpdir, helpers):
         "application/json",
         "https://www.json.org",
     }
+    with pytest.raises(ValueError):
+        correction["badProp"] = {"k": "v"}  # value has no "@id"
+    correction._jsonld["badProp"] = {"k": "v"}  # force set using _jsonld
     with pytest.raises(ValueError):
         correction["badProp"]
 
diff --git a/test/test_wrroc.py b/test/test_wrroc.py
new file mode 100644
index 00000000..3b9d7df1
--- /dev/null
+++ b/test/test_wrroc.py
@@ -0,0 +1,79 @@
+# Copyright 2019-2024 The University of Manchester, UK
+# Copyright 2020-2024 Vlaams Instituut voor Biotechnologie (VIB), BE
+# Copyright 2020-2024 Barcelona Supercomputing Center (BSC), ES
+# Copyright 2020-2024 Center for Advanced Studies, Research and Development in Sardinia (CRS4), IT
+# Copyright 2022-2024 École Polytechnique Fédérale de Lausanne, CH
+# Copyright 2024 Data Centre, SciLifeLab, SE
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+from rocrate.rocrate import ROCrate
+from rocrate.model import ContextEntity, SoftwareApplication
+
+
+def test_add_action(tmpdir):
+    crate = ROCrate()
+    instrument = SoftwareApplication(crate)
+    crate.add(instrument)
+    f_in_name = "f_in"
+    f_in_path = tmpdir / f_in_name
+    with open(f_in_path, "wt") as f:
+        f.write("IN\n")
+    f_in = crate.add_file(f_in_path)
+    f_out_name = "f_out"
+    f_out_path = tmpdir / f_out_name
+    with open(f_out_path, "wt") as f:
+        f.write("OUT\n")
+    f_out = crate.add_file(f_out_path)
+    param = crate.add(ContextEntity(crate, "#param", properties={
+        "@type": "PropertyValue",
+        "name": "param_name",
+        "value": "param_value",
+    }))
+    create_action = crate.add_action(
+        instrument,
+        object=[f_in, param],
+        result=[f_out],
+        properties={
+            "name": f"Run 1 of {instrument.id}",
+            "startTime": "2018-10-25T15:46:35.211153",
+            "endTime": "2018-10-25T15:46:43.020168",
+        }
+    )
+    assert create_action.type == "CreateAction"
+    create_actions = crate.get_by_type("CreateAction")
+    assert crate.root_dataset.get("mentions") == create_actions
+    assert create_actions == [create_action]
+    assert create_action.get("instrument") is instrument
+    assert create_action.get("object") == [f_in, param]
+    assert create_action.get("result") == [f_out]
+    assert create_action.get("name") == f"Run 1 of {instrument.id}"
+    assert create_action.get("startTime") == "2018-10-25T15:46:35.211153"
+    assert create_action.get("endTime") == "2018-10-25T15:46:43.020168"
+
+    activate_action = crate.add_action(
+        instrument,
+        object=[f_out],
+        properties={
+            "@type": "ActivateAction",
+            "name": f"Run 2 of {instrument.id}",
+            "endTime": "2018-10-25T16:48:41.021563",
+        }
+    )
+    assert activate_action.type == "ActivateAction"
+    assert crate.root_dataset.get("mentions") == [create_action, activate_action]
+    assert activate_action.get("instrument") is instrument
+    assert activate_action.get("object") == [f_out]
+    assert "result" not in activate_action
+    assert activate_action.get("name") == f"Run 2 of {instrument.id}"
+    assert activate_action.get("endTime") == "2018-10-25T16:48:41.021563"