From 26ef8cd5cb5ab9d143a4cf2f956adc365fbc094e Mon Sep 17 00:00:00 2001 From: Andrew Davison Date: Mon, 31 Aug 2026 16:57:30 +0200 Subject: [PATCH] Fill in empty properties when an object is recognized through the save cache exists() has three ways of finding an object that already exists in the KG, but only two of them filled in the properties that were left empty locally. The save cache branch took the cached object's remote_data without doing so, which made every property that is set in the KG but absent from the object look like a deliberate deletion to modified_data(), so the next call to save() set it to null. Metadata-harvesting scripts, which typically build a fresh object for each role a person holds, were losing people's contact information, affiliations and ORCIDs the second time a person was saved in a single run. Assigning remote_data directly also left the two objects sharing a single dict, as the comment there suspected; the back-fill gives each its own. The mock client used in the tests can now serve seeded instances, answer existence queries against them, and record what gets written, so a save can be followed end to end. --- doc/release_notes.rst | 12 ++++ fairgraph/kgobject.py | 12 +++- test/test_base.py | 124 +++++++++++++++++++++++++++++++++++- test/test_openminds_core.py | 51 ++++++++++++++- test/utils.py | 57 ++++++++++++++++- 5 files changed, 250 insertions(+), 6 deletions(-) diff --git a/doc/release_notes.rst b/doc/release_notes.rst index df12b8cf..111eaa7d 100644 --- a/doc/release_notes.rst +++ b/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 `_). + Version 0.14.0 ============== diff --git a/fairgraph/kgobject.py b/fairgraph/kgobject.py index 334fe454..ae95c8ad 100644 --- a/fairgraph/kgobject.py +++ b/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( diff --git a/test/test_base.py b/test/test_base.py index 704cc25c..1c9e8f87 100644 --- a/test/test_base.py +++ b/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: diff --git a/test/test_openminds_core.py b/test/test_openminds_core.py index 83711ffd..187e000d 100644 --- a/test/test_openminds_core.py +++ b/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) diff --git a/test/utils.py b/test/utils.py index 8158bcb9..4b59ee6f 100644 --- a/test/utils.py +++ b/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()