From 7a7049d48d1e0445a0e2f5e3a2d4ccb8642ea955 Mon Sep 17 00:00:00 2001 From: Andrew Davison Date: Sat, 17 Oct 2026 22:53:32 +0200 Subject: [PATCH] Resolve reverse links using the forward property's path, not its Python name (#92) Resolving or counting a reverse property sent the same filter, keyed by a single Python name, to every target class. The forward property that makes the link does not always have the same name in each class (e.g. "license" in DatasetVersion, "licenses" in ModelVersion), so query generation raised "Invalid filter" and nothing was returned. Where a reverse property stands for several forward properties, only the first was queried. KGQuery.from_reverse_property() now builds one filter for each forward property whose path matches, in each target class, and resolve() returns each node once even if it links through more than one of them. Also remove the reverse-KGQuery block in `KGObject.__init__`, which never ran: `KGNode.__init__` has already set every reverse property, and it passed a `callback` argument that `KGQuery` does not accept. --- doc/release_notes.rst | 7 ++++ fairgraph/kgobject.py | 6 ---- fairgraph/kgquery.py | 47 +++++++++++++++++++++--- fairgraph/node.py | 7 +--- test/test_openminds_core.py | 71 +++++++++++++++++++++++++++++++++++++ 5 files changed, 122 insertions(+), 16 deletions(-) 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",