diff --git a/doc/release_notes.rst b/doc/release_notes.rst index 087950a0..1be7b58f 100644 --- a/doc/release_notes.rst +++ b/doc/release_notes.rst @@ -65,6 +65,13 @@ Bug fixes Such a link now gives the same existence query as the object it points to (`#145 `_). +- Resolving or counting a reverse property, e.g. ``license.is_applied_to.resolve(client)``, no longer raises + :exc:`ValueError` ("Invalid filter") when the linking property has different names in different classes + (here ``license`` in :class:`DatasetVersion` but ``licenses`` in :class:`ModelVersion`), + and now finds the nodes linking through any of the forward properties a reverse property stands for, + where previously only the first was used + (`#92 `_). + Connection robustness ---------------------- diff --git a/fairgraph/kgobject.py b/fairgraph/kgobject.py index 748571d0..6428bae0 100644 --- a/fairgraph/kgobject.py +++ b/fairgraph/kgobject.py @@ -89,12 +89,6 @@ def __init__( self.release_status = release_status self.allow_update = True super().__init__(data=data, **properties) - for prop in self.reverse_properties: - if not hasattr(self, prop.name): - query = KGQuery( - prop.types, {prop.reverse: self.id}, callback=lambda value: setattr(self, prop.name, value) - ) - setattr(self, prop.name, query) self._raw_remote_data = None self.remote_data = {} diff --git a/fairgraph/kgquery.py b/fairgraph/kgquery.py index 9a935ade..77e9eb35 100644 --- a/fairgraph/kgquery.py +++ b/fairgraph/kgquery.py @@ -22,6 +22,7 @@ import logging from typing import Dict, List, Optional, Union, Any, TYPE_CHECKING +from openminds.properties import Property from openminds.registry import lookup from .utility import as_list, expand_filter @@ -75,6 +76,38 @@ def __init__( self.classes.append(cls) self.filter = filter self.preferred_release_status = preferred_release_status + # set by from_reverse_property(), where the filter differs between classes + self._reverse_paths: Optional[List[str]] = None + self._node_id: Optional[str] = None + + @classmethod + def from_reverse_property( + cls, prop: Property, node_id: str, preferred_release_status: str = "released" + ) -> "KGQuery": + """ + Create a query for the nodes that link to the node with the given id, + as represented by the reverse property `prop` of that node. + + The forward properties that make these links are identified by their path, + since the same path does not always have the same Python name in all classes + (e.g. "license" in DatasetVersion, "licenses" in ModelVersion), + and a class may link through more than one forward property. + """ + query = cls(prop.types, {as_list(prop.reverse)[0]: node_id}, preferred_release_status) + query._reverse_paths = as_list(prop.path) + query._node_id = node_id + return query + + def _class_filters(self): + """Return (class, filter) pairs, one per query to send.""" + if self._reverse_paths is None: + return [(cls, self.filter) for cls in self.classes] + return [ + (cls, {forward_prop.name: self._node_id}) + for cls in self.classes + for forward_prop in cls.properties + if forward_prop.path in self._reverse_paths + ] def __repr__(self): return f"""{self.__class__.__name__}([{", ".join(cls.__name__ for cls in self.classes)}], filter="{self.filter!r}")""".replace( @@ -111,12 +144,12 @@ def resolve( """ release_status = release_status or self.preferred_release_status objects: List[KGObject] = [] - for cls in self.classes: + for cls, filter in self._class_filters(): if hasattr(cls, "generate_query"): # if cls is EmbeddedMetadata we cannot query it query = cls.generate_query( client=client, - filters=self.filter, + filters=filter, space=space, follow_links=follow_links, with_reverse_properties=with_reverse_properties, @@ -128,6 +161,9 @@ def resolve( release_status=release_status, ).data objects.extend(cls.from_jsonld(instance_data, client) for instance_data in instances) + if self._reverse_paths is not None: + # a node that links through more than one forward property is found more than once + objects = list({obj.id: obj for obj in objects}.values()) for obj in objects: object_cache[obj.id] = obj @@ -143,9 +179,12 @@ def resolve( def count(self, client: KGClient, space: Optional[str] = None, release_status: Optional[str] = None): """ Return the number of objects that would be returned by resolving this query. + + For a query created with `from_reverse_property()`, a node that links through + more than one forward property is counted once for each. """ release_status = release_status or self.preferred_release_status n = 0 - for cls in self.classes: - n += cls.count(client, api="query", release_status=release_status, space=space, **self.filter) + for cls, filter in self._class_filters(): + n += cls.count(client, api="query", release_status=release_status, space=space, **filter) return n diff --git a/fairgraph/node.py b/fairgraph/node.py index 5cfaf228..866c67b0 100644 --- a/fairgraph/node.py +++ b/fairgraph/node.py @@ -441,12 +441,7 @@ def _normalize_type(data_item): if data_item is None: if prop.reverse and "@id" in data: - if isinstance(prop.reverse, list): - # todo: handle all possible reverses - # for now, we just take the first - deserialized_data[prop.name] = KGQuery(prop.types, {prop.reverse[0]: data["@id"]}) - else: - deserialized_data[prop.name] = KGQuery(prop.types, {prop.reverse: data["@id"]}) + deserialized_data[prop.name] = KGQuery.from_reverse_property(prop, data["@id"]) else: deserialized_data[prop.name] = None else: diff --git a/test/test_openminds_core.py b/test/test_openminds_core.py index c7c49d56..f27fc4a4 100644 --- a/test/test_openminds_core.py +++ b/test/test_openminds_core.py @@ -654,6 +654,77 @@ def test_KGQuery_count(kg_client): assert n_models_q > 1 +REVERSE_TEST_ID = "https://kg.ebrains.eu/api/instances/00000000-0000-0000-0000-000000000001" + + +def _all_reverse_properties(): + import fairgraph.openminds.v4 as v4 + import fairgraph.openminds.v5 as v5 + + for version in (v4, v5): + for module_name in dir(version): + module = getattr(version, module_name) + if hasattr(module, "list_kg_classes"): + for cls in module.list_kg_classes(): + for prop in cls.reverse_properties: + yield cls, prop + + +def test_reverse_property_queries_are_valid_for_every_target_class(mock_client): + # the forward property does not have the same Python name in all target classes (#92) + reverse_properties = list(_all_reverse_properties()) + assert len(reverse_properties) > 1000 + for cls, prop in reverse_properties: + class_filters = KGQuery.from_reverse_property(prop, REVERSE_TEST_ID)._class_filters() + assert {target for target, _ in class_filters} == set(prop.types), (cls, prop.name) + for target, filter in class_filters: + if not hasattr(target, "generate_query") or target.__name__ == "MRIScannerUsage": + continue # embedded classes cannot be queried; MRIScannerUsage query generation fails separately + target.generate_query(client=mock_client, space=None, filters=filter) + + +def test_reverse_property_query_uses_the_forward_property_name_of_each_class(): + query = KGQuery.from_reverse_property(omcore.License.get_property("is_applied_to"), REVERSE_TEST_ID) + class_filters = dict((target.__name__, filter) for target, filter in query._class_filters()) + assert class_filters["DatasetVersion"] == {"license": REVERSE_TEST_ID} + assert class_filters["ModelVersion"] == {"licenses": REVERSE_TEST_ID} + + +def test_reverse_property_query_covers_every_forward_property_with_the_path(): + import fairgraph.openminds.v5.core as v5core + + query = KGQuery.from_reverse_property(v5core.BehavioralProtocol.get_property("used_in"), REVERSE_TEST_ID) + assert [filter for target, filter in query._class_filters() if target is v5core.ProtocolExecution] == [ + {"behavioral_protocols": REVERSE_TEST_ID}, + {"protocols": REVERSE_TEST_ID}, + ] + + +def test_resolving_reverse_property_query_returns_each_node_once(): + import fairgraph.openminds.v5.core as v5core + + execution_id = "https://kg.ebrains.eu/api/instances/00000000-0000-0000-0000-000000000002" + + class ClientLinkingThroughBothProperties: + # the protocol execution links to the protocol through both "behavioralProtocol" and "protocol" + def query(self, query, size, from_index, release_status): + if query["meta"]["type"] == v5core.ProtocolExecution.type_: + return MockKGResponse([{"@id": execution_id, "@type": [v5core.ProtocolExecution.type_]}]) + return MockKGResponse([]) + + query = KGQuery.from_reverse_property(v5core.BehavioralProtocol.get_property("used_in"), REVERSE_TEST_ID) + result = query.resolve(ClientLinkingThroughBothProperties()) + assert isinstance(result, v5core.ProtocolExecution) + assert result.id == execution_id + + +@skip_if_no_connection +def test_count_reverse_link_with_different_forward_property_names(kg_client): + # DatasetVersion has "license", ModelVersion and SoftwareVersion have "licenses" (#92) + license = omcore.License.by_name("CC-BY-4.0", kg_client) + assert license.is_applied_to.count(kg_client) > 0 + + def test_to_jsonld(): person1 = omcore.Person( id="0000",