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

@@ -587,8 +587,16 @@ def exists(self, client: KGClient, ignore_duplicates: bool = False, in_spaces: O
                     self.id = save_cache[self.__class__][query_cache_key]
                     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?
+                        if self._raw_remote_data is None:
+                            self._raw_remote_data = cached_obj._raw_remote_data
+                        data = cached_obj.remote_data
+                        if "@type" not in data:  # should not happen, but just in case
+                            data = {"@type": self.type_, **data}
+                        # 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(data)
                     return True
 
                 query = self.__class__.generate_minimal_query(

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
 
@@ -208,6 +210,20 @@ class MockKGObject(KGObject, LinkedMetadata):
 ID_NAMESPACE = "https://kg.ebrains.eu/api/instances/"
 
 
+class RecordingMockClient:
+    """A minimal client that serves a fixed set of instances and records what is written."""
+
+    def __init__(self, instances=None):
+        self.instances = instances or {}
+        self.updates = []  # (instance_id, payload) for each update_instance() call
+
+    def instance_from_full_uri(self, uri, use_cache=True, release_status="in progress", require_full_data=True):
+        return deepcopy(self.instances.get(uri, None))
+
+    def update_instance(self, instance_id, data):
+        self.updates.append((instance_id, deepcopy(data)))
+
+
 class TestKGObject(object):
     object_counter = 0
 
@@ -607,6 +623,110 @@ 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, but in
+        # separate dicts: a later write by one must not rewrite the other's record
+        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, 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)
+        client = RecordingMockClient({orig_object.id: self._kg_record(orig_object)})
+
+        new_obj = self._construct_object_not_yet_in_kg()
+        log = ActivityLog()
+        new_obj.save(client, space="mock", recursive=False, activity_log=log)
+
+        assert 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(client, space="mock", recursive=False, activity_log=log)
+
+        assert [entry.type for entry in log.entries] == ["update"]
+        assert len(client.updates) == 1
+        instance_id, payload = 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):
@@ -507,6 +513,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)

test/utils.py

@@ -6,8 +6,10 @@
 from requests.exceptions import RequestException, SSLError
 
 from fairgraph.base import OPENMINDS_VERSION
+from fairgraph.caching import object_cache, save_cache
 from fairgraph.client import KGClient
 from fairgraph.errors import AuthenticationError, AuthorizationError
+from fairgraph.utility import as_list
 
 import pytest
 
@@ -74,6 +76,8 @@ def __init__(self, openminds_version: str = OPENMINDS_VERSION):
         self.openminds_version = openminds_version
         self.instances = {}
         self.cache = {}
+        self.updates = []  # (instance_id, payload) for each update_instance() call
+        self.replacements = []  # (instance_id, payload) for each replace_instance() call
 
     def retrieve_query(self, query_label):
         return {"@id": f"mock-query-{query_label}"}
@@ -86,7 +90,9 @@ def instance_from_full_uri(
         require_full_data: bool = True,
     ):
         mock_id = "http://example.org/00000000-0000-0000-0000-000000000000"
-        if uri == mock_id:
+        if uri in self.instances:
+            return deepcopy(self.instances[uri])
+        elif uri == mock_id:
             return {"@id": mock_id, "@type": ["https://openminds.om-i.org/types/Model"]}
         else:
             raise NotImplementedError
@@ -167,8 +173,40 @@ def query(
                 filter_value = prop["filter"]["value"]
                 if filter_value == "Thorin":
                     return MockKGResponse([])
+        matches = self._match_instances(query)
+        if matches is not None:
+            return MockKGResponse(matches)
         raise NotImplementedError("case not yet handled by mock client")
 
+    def _match_instances(self, query):
+        """
+        Match any instances that have been added to the mock KG (either seeded by a
+        test or created through `create_new_instance`) against a query definition.
+
+        Returns None if the query is not of a shape this mock understands, so that
+        the caller can fall back to raising NotImplementedError.
+        """
+        if not self.instances:
+            return None
+        node_type = query.get("meta", {}).get("type", None)
+        if node_type is None:
+            return None
+        filters = {}
+        for prop in query.get("structure", []):
+            path = prop.get("path", None)
+            value = prop.get("filter", {}).get("value", None)
+            if value is not None:
+                if not isinstance(path, str) or not path.startswith("http"):
+                    return None  # e.g. filtering on "@id", which we don't support here
+                filters[path] = value
+        matches = []
+        for instance in self.instances.values():
+            if node_type not in as_list(instance.get("@type", [])):
+                continue
+            if all(value in as_list(instance.get(path, [])) for path, value in filters.items()):
+                matches.append(deepcopy(instance))
+        return matches
+
     def create_new_instance(self, data, space, instance_id=None):
         assert space is not None
         assert data is not None
@@ -181,10 +219,12 @@ def create_new_instance(self, data, space, instance_id=None):
     def update_instance(self, instance_id, data):
         assert instance_id is not None
         assert data is not None
+        self.updates.append((instance_id, deepcopy(data)))
 
     def replace_instance(self, instance_id, data):
         assert instance_id is not None
         assert data is not None
+        self.replacements.append((instance_id, deepcopy(data)))
 
     def uri_from_uuid(self, uuid):
         return f"https://kg.ebrains.eu/api/instances/{uuid}"
@@ -193,3 +233,18 @@ def uri_from_uuid(self, uuid):
 @pytest.fixture
 def mock_client():
     return MockKGClient()
+
+
+@pytest.fixture
+def clear_caches():
+    """
+    Ensure a test starts and finishes with empty global caches.
+
+    `save_cache` and `object_cache` are module-level globals, so tests that
+    exercise them would otherwise leak into one another.
+    """
+    save_cache.clear()
+    object_cache.clear()
+    yield
+    save_cache.clear()
+    object_cache.clear()