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()