Fill in empty properties when an object is recognized through the save cache

doc/release_notes.rst

@@ -58,6 +58,18 @@ and v4 namespaces is no longer needed. The following have been removed:
 Code that called these directly will need updating; code that simply used the client is
 unaffected.
 
+Bug fixes in this release:
+
+- Saving the same object twice in one session no longer erases the properties it does not
+  carry. When :meth:`~fairgraph.kgobject.KGObject.exists` recognized an object from the save
+  cache, it took the cached object's view of what the Knowledge Graph holds without filling in
+  the properties left empty locally. Any property that was set in the KG but absent from the
+  object then looked like a deliberate deletion, and was set to null by the following
+  :meth:`~fairgraph.kgobject.KGObject.save`. Metadata-harvesting scripts, which typically build
+  a fresh object for each role a person holds, were losing people's contact information,
+  affiliations and ORCIDs this way
+  (`#134 <https://github.com/HumanBrainProject/fairgraph/issues/134>`_).
+
 
 Version 0.14.0
 ==============

fairgraph/kgobject.py

@@ -98,7 +98,7 @@ def __init__(
                 try:
                     self.remote_data = normalize_data(
                         self.to_jsonld(include_empty_properties=False, embed_linked_nodes=LinkedNodeEmbedding.NEVER),
-                        data.get("@context", self.context)
+                        data.get("@context", self.context),
                     )
                 except ValueError as err:
                     # ideally, we should handle errors at the level of individual properties
@@ -126,10 +126,7 @@ def space(self) -> Union[str, None]:
 
     @classmethod
     def from_jsonld(
-        cls,
-        data: JSONdict,
-        ignore_unexpected_keys: Optional[bool] = False,
-        release_status: Optional[str] = None
+        cls, data: JSONdict, ignore_unexpected_keys: Optional[bool] = False, release_status: Optional[str] = None
     ) -> KGObject:
         """Create an instance of the class from a JSON-LD document."""
         # todo: handle ignore_unexpected_keys
@@ -588,7 +585,11 @@ def exists(self, client: KGClient, ignore_duplicates: bool = False, in_spaces: O
                     cached_obj = object_cache.get(self.id)
                     if cached_obj and cached_obj.remote_data:
                         self._raw_remote_data = cached_obj._raw_remote_data
-                        self.remote_data = cached_obj.remote_data  # copy or update needed?
+                        # this also updates `self.remote_data`. It must not be replaced by a
+                        # direct assignment to `self.remote_data`: a property that is empty
+                        # locally but present remotely would then look like a deliberate
+                        # deletion, and be set to null by the next call to save().
+                        self._update_empty_properties(cached_obj.remote_data)
                     return True
 
                 query = self.__class__.generate_minimal_query(
@@ -597,7 +598,9 @@ def exists(self, client: KGClient, ignore_duplicates: bool = False, in_spaces: O
                 )
 
                 try:
-                    instances = client.query(query=query, size=2, release_status="any", restrict_to_spaces=in_spaces).data
+                    instances = client.query(
+                        query=query, size=2, release_status="any", restrict_to_spaces=in_spaces
+                    ).data
                 except ConnectionError as err:
                     if "RemoteDisconnected" in str(err):
                         warn(
@@ -649,11 +652,8 @@ def values_are_equal(local, remote):
                 return local == remote
 
         current_data = normalize_data(
-            self.to_jsonld(
-                include_empty_properties=True,
-                embed_linked_nodes=LinkedNodeEmbedding.IF_NECESSARY
-            ),
-            self.context
+            self.to_jsonld(include_empty_properties=True, embed_linked_nodes=LinkedNodeEmbedding.IF_NECESSARY),
+            self.context,
         )
         modified_data = {}
         for key, current_value in current_data.items():
@@ -748,7 +748,7 @@ def save(
                 # update
                 local_data = normalize_data(
                     self.to_jsonld(include_empty_properties=False, embed_linked_nodes=LinkedNodeEmbedding.NEVER),
-                    self.context
+                    self.context,
                 )
                 if replace:
                     logger.info(f"  - replacing - {self.__class__.__name__}(id={self.id})")
@@ -817,7 +817,7 @@ def save(
             # create new
             local_data = normalize_data(
                 self.to_jsonld(include_empty_properties=False, embed_linked_nodes=LinkedNodeEmbedding.NEVER),
-                self.context
+                self.context,
             )
             logger.info("  - creating instance with data {}".format(local_data))
             if self.id and self.id.startswith("http"):
@@ -909,7 +909,15 @@ def by_name(
         release_status = handle_scope_keyword(scope, release_status)
         # todo: move this to openminds generation, and include only in those subclasses
         # that have a name-like property
-        namelike_properties = ("name", "lookup_label", "family_name", "full_name", "short_name", "abbreviation", "synonyms")
+        namelike_properties = (
+            "name",
+            "lookup_label",
+            "family_name",
+            "full_name",
+            "short_name",
+            "abbreviation",
+            "synonyms",
+        )
         objects = []
         if client:
             kwargs = dict(space=space, release_status=release_status, api="query", follow_links=follow_links)
@@ -920,11 +928,9 @@ def by_name(
             objects = cls.list(client, **kwargs)
             if match == "equals":
                 objects = [
-                    obj for obj in objects
-                    if any(
-                        getattr(obj, prop_name, None) == name
-                        for prop_name in namelike_properties
-                    )
+                    obj
+                    for obj in objects
+                    if any(getattr(obj, prop_name, None) == name for prop_name in namelike_properties)
                 ]
         elif hasattr(cls, "instances"):  # controlled terms, etc.
             if cls._instance_lookup is None:
@@ -1075,9 +1081,7 @@ def generate_minimal_query(
         query.properties.extend(cls.generate_query_filter_properties(normalized_filters))
         return query.serialize()
 
-    def children(
-        self, client: KGClient, follow_links: Optional[Dict[str, Any]] = None
-    ) -> List[Releasable]:
+    def children(self, client: KGClient, follow_links: Optional[Dict[str, Any]] = None) -> List[Releasable]:
         """Return a list of child objects."""
         if follow_links:
             self.resolve(client, follow_links=follow_links)

test/test_base.py

@@ -3,6 +3,7 @@
 Tests of fairgraph.base module.
 """
 
+from copy import deepcopy
 from datetime import date, datetime
 from numbers import Real
 from openminds.base import LinkedMetadata, EmbeddedMetadata as OMEmbeddedMetadata, LinkedNodeEmbedding
@@ -11,10 +12,11 @@
 from fairgraph.embedded import KGEmbedded
 from fairgraph.kgobject import KGObject
 from fairgraph.kgproxy import KGProxy
-from fairgraph.caching import generate_cache_key
+from fairgraph.caching import generate_cache_key, object_cache, save_cache
 from fairgraph.errors import CannotBuildExistenceQuery
 from fairgraph.base import ErrorHandling
-from .utils import mock_client
+from fairgraph.utility import ActivityLog
+from .utils import clear_caches, mock_client
 
 import pytest
 
@@ -504,7 +506,7 @@ def test_modified_data(self):
                     "https://openminds.ebrains.eu/vocab/aString": None,
                 },
             ],
-            "https://openminds.ebrains.eu/vocab/anOptionalListOfLinkedObjects": None
+            "https://openminds.ebrains.eu/vocab/anOptionalListOfLinkedObjects": None,
         }
         assert obj.modified_data() == expected
 
@@ -529,7 +531,9 @@ def test_exists__it_does_exist(self):
 
         class MockClient:
             def instance_from_full_uri(self, id, use_cache=True, release_status="in progress", require_full_data=True):
-                data = orig_object.to_jsonld(include_empty_properties=False, embed_linked_nodes=LinkedNodeEmbedding.NEVER)
+                data = orig_object.to_jsonld(
+                    include_empty_properties=False, embed_linked_nodes=LinkedNodeEmbedding.NEVER
+                )
                 data["https://core.kg.ebrains.eu/vocab/meta/space"] = "collab-foobar"
                 data["@id"] = orig_object.id
                 data["@type"] = orig_object.type_
@@ -607,6 +611,112 @@ def instance_from_full_uri(self, id, use_cache=True, release_status="in progress
         }
         assert new_obj.modified_data() == expected
 
+    def _kg_record(self, obj):
+        """
+        The JSON-LD document the KG would return for `obj`, including a property
+        that is set in the KG but that user code never provides.
+        """
+        record = deepcopy(obj.remote_data)
+        record["@id"] = obj.id
+        record["@type"] = [MockKGObject.type_]  # the KG returns a list of types
+        record["https://openminds.ebrains.eu/vocab/anOptionalString"] = "lime"
+        return record
+
+    def _construct_object_as_found_in_kg(self):
+        """An object in the state it would be in after exists() found it in the KG."""
+        obj = self._construct_object_required_properties()
+        obj._update_empty_properties(self._kg_record(obj))
+        assert obj.an_optional_string == "lime"
+        return obj
+
+    def _construct_object_not_yet_in_kg(self):
+        """
+        A freshly built object, as user code would construct it, knowing nothing
+        about what the KG already holds.
+        """
+        obj = self._construct_object_required_properties()
+        obj.id = None
+        obj._raw_remote_data = None
+        obj.remote_data = {}
+        return obj
+
+    def _register_in_save_cache(self, obj):
+        """Mimic the caching that exists() and save() perform for an object in the KG."""
+        save_cache[MockKGObject][generate_cache_key(obj._build_existence_query())] = obj.id
+        object_cache[obj.id] = obj
+
+    def test_exists__found_via_save_cache(self, clear_caches):
+        """
+        An object found through the save cache - i.e. an equivalent object was
+        already looked up or saved earlier in the same run - must have its empty
+        properties filled in from the cached object, just as when it is found by
+        querying the KG. Otherwise a property that exists in the KG but was not
+        provided locally looks like a deliberate deletion to modified_data().
+        """
+        orig_object = self._construct_object_as_found_in_kg()
+        self._register_in_save_cache(orig_object)
+
+        new_obj = self._construct_object_not_yet_in_kg()
+        assert new_obj.an_optional_string is None
+
+        assert new_obj.exists(client=None)
+        assert new_obj.id == orig_object.id
+        assert new_obj.an_optional_string == "lime"  # filled in from the cached object
+        assert new_obj.modified_data() == {}  # so nothing would be nulled by a save
+        # both objects hold the same record of what the KG contains, in separate
+        # top-level dicts, so neither can rewrite the other's record. Nested values
+        # are shared by reference, which is safe because remote_data is only ever
+        # written a key at a time or replaced wholesale, never mutated in place.
+        assert new_obj.remote_data == orig_object.remote_data
+        assert new_obj.remote_data is not orig_object.remote_data
+
+    def test_exists__found_via_save_cache_keeps_local_values(self, clear_caches):
+        """
+        Being recognized through the save cache tells an object which KG instance
+        it is, not what its properties should be. Values provided locally are the
+        changes the caller wants to make, so they must survive, and must still be
+        seen as modified relative to what the KG holds.
+        """
+        orig_object = self._construct_object_as_found_in_kg()
+        self._register_in_save_cache(orig_object)
+
+        new_obj = self._construct_object_not_yet_in_kg()
+        new_obj.an_optional_string = "kiwi"  # differs from the value in the KG
+
+        assert new_obj.exists(client=None)
+        assert new_obj.id == orig_object.id
+        assert new_obj.an_optional_string == "kiwi"  # not overwritten with "lime"
+        assert new_obj.modified_data() == {"https://openminds.ebrains.eu/vocab/anOptionalString": "kiwi"}
+        assert orig_object.an_optional_string == "lime"  # and the cached object is untouched
+
+    def test_save__found_via_save_cache_does_not_null_properties(self, mock_client, clear_caches):
+        """
+        Saving a freshly-built object that is found through the save cache must
+        not set the properties it doesn't know about to null in the KG.
+        """
+        orig_object = self._construct_object_as_found_in_kg()
+        self._register_in_save_cache(orig_object)
+        mock_client.instances[orig_object.id] = self._kg_record(orig_object)
+
+        new_obj = self._construct_object_not_yet_in_kg()
+        log = ActivityLog()
+        new_obj.save(mock_client, space="mock", recursive=False, activity_log=log)
+
+        assert mock_client.updates == []
+        assert [entry.type for entry in log.entries] == ["no-op"]
+        assert new_obj.an_optional_string == "lime"
+
+        # ...but a genuine local change must still be sent
+        new_obj.an_optional_string = "kiwi"
+        log = ActivityLog()
+        new_obj.save(mock_client, space="mock", recursive=False, activity_log=log)
+
+        assert [entry.type for entry in log.entries] == ["update"]
+        assert len(mock_client.updates) == 1
+        instance_id, payload = mock_client.updates[0]
+        assert instance_id == new_obj.uuid
+        assert payload == {"https://openminds.ebrains.eu/vocab/anOptionalString": "kiwi"}
+
     def test_exists_insufficient_query_properties(self):
         """If an object is missing required metadata, exists should return False"""
         for prop_name in MockKGObject.existence_query_properties:

test/test_openminds_core.py

@@ -21,7 +21,13 @@
 import fairgraph.openminds.controlled_terms as omterms
 from fairgraph.utility import ActivityLog, sha1sum, normalize_data
 
-from test.utils import mock_client, kg_client, skip_if_no_connection, skip_if_using_production_server
+from test.utils import (
+    clear_caches,
+    mock_client,
+    kg_client,
+    skip_if_no_connection,
+    skip_if_using_production_server,
+)
 
 
 def test_query_generation(mock_client):
@@ -76,7 +82,9 @@ def test_retrieve_released_datasets_filter_species_by_openminds_obj(kg_client):
     assert rat.name == rat_om.name == "Rattus norvegicus"
     follow_links = {"study_targets": {}}
     datasets = omcore.DatasetVersion.list(kg_client, space="dataset", study_targets=rat, follow_links=follow_links)
-    datasets_om = omcore.DatasetVersion.list(kg_client, space="dataset", study_targets=rat_om, follow_links=follow_links)
+    datasets_om = omcore.DatasetVersion.list(
+        kg_client, space="dataset", study_targets=rat_om, follow_links=follow_links
+    )
     assert len(datasets) > 0
     assert len(datasets) == len(datasets_om)
     assert [ds.id for ds in datasets] == [ds.id for ds in datasets_om]
@@ -458,14 +466,9 @@ def test_modified_data_method_with_local_changes():
         }
     }
     dsv.repository = omcore.FileRepository(
-        id="https://kg.ebrains.eu/api/instances/23456789-0abc-def0-1234-567890abcdef",
-        iri="http://example.org"
+        id="https://kg.ebrains.eu/api/instances/23456789-0abc-def0-1234-567890abcdef", iri="http://example.org"
     )
-    assert dsv.modified_data() == {
-        "https://openminds.om-i.org/props/repository": {
-            "@id": dsv.repository.id
-        }
-    }
+    assert dsv.modified_data() == {"https://openminds.om-i.org/props/repository": {"@id": dsv.repository.id}}
 
 
 def test__update():
@@ -507,6 +510,49 @@ def test__update():
     assert len(updated_data) == 0
 
 
+def test_save_same_person_twice_preserves_remote_only_properties(mock_client, clear_caches):
+    """
+    Metadata-harvesting scripts typically build a new Person object for each
+    role a person has (developer, custodian, ...) and for each project, so the
+    same person may be saved several times in a single run, from objects that
+    contain only the name. The second and subsequent saves must not remove the
+    contact information, ORCID, etc. already held in the KG.
+    """
+    person_id = "https://kg.ebrains.eu/api/instances/12345678-90ab-cdef-0123-4567890abcde"
+    contact_id = "https://kg.ebrains.eu/api/instances/23456789-0abc-def0-1234-567890abcdef"
+    orcid_id = "https://kg.ebrains.eu/api/instances/34567890-abcd-ef01-2345-67890abcdef0"
+    mock_client.instances[person_id] = {
+        "@id": person_id,
+        "@type": ["https://openminds.om-i.org/types/Person"],
+        "https://core.kg.ebrains.eu/vocab/meta/space": "common",
+        "https://openminds.om-i.org/props/givenName": "Bilbo",
+        "https://openminds.om-i.org/props/familyName": "Baggins",
+        "https://openminds.om-i.org/props/alternateName": ["Barrel-rider"],
+        "https://openminds.om-i.org/props/contactInformation": {"@id": contact_id},
+        "https://openminds.om-i.org/props/digitalIdentifier": [{"@id": orcid_id}],
+    }
+
+    # first encounter, e.g. as a developer: found by querying the KG
+    developer = omcore.Person(given_name="Bilbo", family_name="Baggins")
+    log = ActivityLog()
+    developer.save(mock_client, space="common", activity_log=log)
+    assert developer.id == person_id
+    assert developer.contact_information == KGProxy(omcore.ContactInformation, contact_id)
+    assert [entry.type for entry in log.entries] == ["no-op"]
+    assert mock_client.updates == []
+
+    # second encounter, e.g. as a custodian: found in the save cache
+    custodian = omcore.Person(given_name="Bilbo", family_name="Baggins")
+    log = ActivityLog()
+    custodian.save(mock_client, space="common", activity_log=log)
+    assert custodian.id == person_id
+    assert custodian.contact_information == KGProxy(omcore.ContactInformation, contact_id)
+    assert custodian.digital_identifiers == [KGProxy(omcore.ORCID, orcid_id)]
+    assert custodian.alternate_names == ["Barrel-rider"]
+    assert [entry.type for entry in log.entries] == ["no-op"]
+    assert mock_client.updates == []
+
+
 @skip_if_no_connection
 def test_KGQuery_resolve(kg_client):
     ca1 = omterms.UBERONParcellation.by_name("CA1 field of hippocampus", kg_client)