From 95957634669c9bfd3b0803eb7db1e416877df6ee Mon Sep 17 00:00:00 2001 From: Andrew Davison Date: Fri, 16 Oct 2026 14:40:30 +0200 Subject: [PATCH] Support list values in existence queries Previously, `_build_existence_query()` raised `CannotBuildExistenceQuery("not implemented yet")` if any of a class's existence-query properties held a list, so `exists()` and `save()` treated such objects as new, creating duplicates on repeated saves, and the save cache was never filled for them. This affects any class with a multi-valued existence-query property, e.g. `Chapter.authors` or `LearningResource.about`. A list value now becomes a list in the existence query, which is expressed in the KG query using the support for lists of filter values: every local item must be present in the KG, but the order of the items is ignored, as are any additional items in the KG. Linked nodes are given by ID, and embedded or unsaved nodes by a sub-query, kept as a separate dict for each item so that all its filters must match the same node in the KG. An empty list is treated as a missing value. To support this, `expand_filter()` now expands dicts within lists, and `normalize_filter()` normalizes each item of such a list separately. A linked node whose class has no existence query now raises `CannotBuildExistenceQuery` rather than an `AttributeError`. This also fixes `generate_cache_key()` for lists: it used the whole list in place of each item and omitted the property name, so different queries could share a key. Item order no longer affects the key. --- fairgraph/caching.py | 5 +- fairgraph/node.py | 145 ++++++++++++++++++++++++++---------- fairgraph/utility/jsonld.py | 3 + test/test_base.py | 84 +++++++++++++++++++++ test/test_queries.py | 61 +++++++++++++++ test/test_utility.py | 7 ++ 6 files changed, 263 insertions(+), 42 deletions(-) diff --git a/fairgraph/caching.py b/fairgraph/caching.py index 79189d9a..ae5e4a7b 100644 --- a/fairgraph/caching.py +++ b/fairgraph/caching.py @@ -34,8 +34,9 @@ def generate_cache_key(qd: Dict[str, str]) -> Tuple: if isinstance(sub_value, dict): sub_key.append(generate_cache_key(sub_value)) else: - sub_key.append(str(value)) - cache_key.append(tuple(sub_key)) + sub_key.append(str(sub_value)) + # the order of the items does not matter in a query + cache_key.append((key, tuple(sorted(sub_key, key=str)))) elif isinstance(value, dict): cache_key.append((key, str(value))) elif value.__class__.__name__ == "IRI": # a bit hacky diff --git a/fairgraph/node.py b/fairgraph/node.py index 6f699d31..5cfaf228 100644 --- a/fairgraph/node.py +++ b/fairgraph/node.py @@ -183,23 +183,43 @@ def normalize_filter(cls, filter_dict: Dict[str, Any], check_validity: bool = Tr Normalize a dict containing filter key:value pairs so that it can be used in a call to the KG query API. - Example: + The dict must already be nested (see `expand_filter()`): a filter on a property of a linked + or embedded node is given as a dict under the name of the linking property. + Values are converted into the form used in queries, e.g. a node is replaced by its ID. + A list of values is kept as a list, each item being normalized in the same way. + + Examples: >>> import fairgraph.openminds.core as omcore - >>> person = omcore.Person.from_uuid("045f846f-f010-4db8-97b9-b95b20970bf2", kg_client) - >>> filter_dict = {"custodians": person, "name": "Virtual"} - >>> omcore.Dataset.normalize_filter(filter_dict) - {'name': 'Virtual', - 'custodians': 'https://kg.ebrains.eu/api/instances/045f846f-f010-4db8-97b9-b95b20970bf2'} + >>> person = omcore.Person(id="https://kg.ebrains.eu/api/instances/00000000-0000-0000-0000-000000000001") + >>> omcore.Dataset.normalize_filter({"custodians": person, "name": "Virtual"}) + {'custodians': 'https://kg.ebrains.eu/api/instances/00000000-0000-0000-0000-000000000001', + 'full_name': 'Virtual'} + + Here the linked node is replaced by its ID, and the alias "name" by the property it stands for. + + >>> from datetime import date + >>> from fairgraph.utility import expand_filter + >>> filters = expand_filter({"developers__family_name": ["Baggins", "Oakenshield"], + ... "release_date": date(2024, 1, 1)}) + >>> omcore.SoftwareVersion.normalize_filter(filters) + {'developers': {'family_name': ['Baggins', 'Oakenshield']}, 'release_date': '2024-01-01'} + + Here the filter on the developers' family names is normalized by the Person class + (one of the possible types of `developers`), and the date is converted to a string. """ normalized = {} filter_dict_copy = filter_dict.copy() - # handle aliases + # replace aliases (alternative names for properties, e.g. "name" for "full_name") + # by the names of the properties they stand for for name_, alias_ in cls.aliases.items(): if name_ in filter_dict_copy: filter_dict_copy[alias_] = filter_dict_copy.pop(name_) def _check_validity(filters, property_names): + """ + Raise a ValueError if any of the keys of `filters` is not in `property_names`. + """ invalid_filters = set(filters).difference(property_names) if invalid_filters: if len(invalid_filters) == 1: @@ -208,28 +228,46 @@ def _check_validity(filters, property_names): msg = f"Invalid filters: " raise ValueError(msg + ", ".join(sorted(invalid_filters))) + # the check is skipped when this method is called by `_normalize_dict()` for one of several + # possible child classes, since a filter key need only be valid for one of them if check_validity: _check_validity(filter_dict_copy, cls.all_property_names + list(cls.aliases.keys())) + def _normalize_dict(prop, value): + """ + Normalize `value`, a dict of filters on the properties of the node(s) linked or embedded via `prop`. + + A property may have several possible types (e.g. Person or Organization), + so each filter key must be a property of at least one of them. + Each type normalizes the filters on its own properties, and the results are merged. + """ + _check_validity( + value, + set( + chain(*(child_cls.all_property_names + list(child_cls.aliases.keys()) for child_cls in prop.types)) + ), + ) + normalized_value = {} + for child_cls in prop.types: + normalized_value.update(child_cls.normalize_filter(value, check_validity=False)) + return normalized_value + for prop in cls.all_properties: if prop.name in filter_dict_copy: value = filter_dict_copy[prop.name] if isinstance(value, dict): - _check_validity( - value, - set( - chain( - *( - child_cls.all_property_names + list(child_cls.aliases.keys()) - for child_cls in prop.types - ) - ) - ), - ) - normalized[prop.name] = {} - for child_cls in prop.types: - normalized[prop.name].update(child_cls.normalize_filter(value, check_validity=False)) + # filters on the properties of a linked or embedded node + normalized[prop.name] = _normalize_dict(prop, value) + elif isinstance(value, list) and any(isinstance(item, dict) for item in value): + # a list in which some items are filters on the properties of a linked or embedded node + # (as in existence queries for nodes with no ID), and others may be plain values, + # so each item is normalized separately + normalized[prop.name] = [ + _normalize_dict(prop, item) if isinstance(item, dict) else get_filter_value(prop, item) + for item in value + ] else: + # a plain value, or a list of plain values normalized[prop.name] = get_filter_value(prop, value) return normalized @@ -516,6 +554,9 @@ def _build_existence_query(self, existence_match: str = "equals") -> Union[None, With `existence_match="equals"`, string values must match exactly (ignoring case); with `"contains"`, it is enough for the value in the KG to contain the local value. + + For a property holding a list, every local item must be present in the KG, + but the order of the items is ignored, as are any additional items in the KG. """ check_existence_match(existence_match) if self.existence_query_properties is None: @@ -533,28 +574,52 @@ def _build_existence_query(self, existence_match: str = "equals") -> Union[None, for property in query_properties: query_property_name = property.name value = getattr(self, property.name) - if isinstance(value, KGProxy): - query[query_property_name] = value.id - elif isinstance(value, KGNode): - if hasattr(value, "id") and value.id: - query[query_property_name] = value.id - else: - sub_query = value._build_existence_query(existence_match) - query.update({f"{query_property_name}__{key}": val for key, val in sub_query.items()}) - elif isinstance(value, (list, tuple)): - raise CannotBuildExistenceQuery("not implemented yet") - elif value is None: - raise CannotBuildExistenceQuery(f"Required value for '{query_property_name}' is missing") - else: - query_val = value_to_jsonld(value, include_empty_properties=False, embed_linked_nodes=LinkedNodeEmbedding.NEVER) - if query_val is None: + if isinstance(value, (list, tuple)): + if len(value) == 0: raise CannotBuildExistenceQuery(f"Required value for '{query_property_name}' is missing") - if isinstance(query_val, str) and existence_match == "equals": - # match exactly, so that we don't identify this object with one whose value merely contains ours - query_val = Equals(query_val) - query[query_property_name] = query_val + # sub-queries for nodes in the list are kept as separate dicts, so that each is matched as a whole + query[query_property_name] = [ + self._existence_query_value(query_property_name, item, existence_match) for item in value + ] + else: + query_val = self._existence_query_value(query_property_name, value, existence_match) + if isinstance(query_val, dict): + query.update({f"{query_property_name}__{key}": val for key, val in query_val.items()}) + else: + query[query_property_name] = query_val return query + @staticmethod + def _existence_query_value(property_name: str, value: Any, existence_match: str) -> Any: + """ + Return the value to use in an existence query for a single property value (not a list): + an ID for a linked node that has one, a sub-query dict for any other node, or else a JSON-LD value. + """ + if isinstance(value, KGProxy): + return value.id + elif isinstance(value, KGNode): + if hasattr(value, "id") and value.id: + return value.id + sub_query = value._build_existence_query(existence_match) + if sub_query is None: + raise CannotBuildExistenceQuery( + f"Cannot build an existence query for '{property_name}', " + f"since {value.__class__.__name__} has no existence query" + ) + return sub_query + elif value is None: + raise CannotBuildExistenceQuery(f"Required value for '{property_name}' is missing") + else: + query_val = value_to_jsonld( + value, include_empty_properties=False, embed_linked_nodes=LinkedNodeEmbedding.NEVER + ) + if query_val is None: + raise CannotBuildExistenceQuery(f"Required value for '{property_name}' is missing") + if isinstance(query_val, str) and existence_match == "equals": + # match exactly, so that we don't identify this object with one whose value merely contains ours + query_val = Equals(query_val) + return query_val + # Deprecated alias ContainsMetadata = KGNode diff --git a/fairgraph/utility/jsonld.py b/fairgraph/utility/jsonld.py index a7b3ce72..f7e7d208 100644 --- a/fairgraph/utility/jsonld.py +++ b/fairgraph/utility/jsonld.py @@ -216,6 +216,9 @@ def expand_filter(filter_dict: Dict[str, Any]): for key, value in filter_dict.items(): if hasattr(value, "items"): raise TypeError("Filter specifications should be a single-level dict, without nesting") + if isinstance(value, list): + # each dict in a list is a separate filter specification, e.g. for one of several embedded nodes + value = [expand_filter(item) if isinstance(item, dict) else item for item in value] local_path = expanded parts = key.split("__") for part in parts[:-1]: diff --git a/test/test_base.py b/test/test_base.py index 8f76f68b..1c1c92ba 100644 --- a/test/test_base.py +++ b/test/test_base.py @@ -16,6 +16,7 @@ from fairgraph.errors import CannotBuildExistenceQuery from fairgraph.base import ErrorHandling from fairgraph.utility import ActivityLog +from fairgraph.queries import Equals from .utils import MockKGResponse, clear_caches, mock_client import pytest @@ -458,6 +459,81 @@ def test_save__with_proxy_in_existence_query(self, mock_client, clear_caches, mo cache_key = generate_cache_key(resolved_obj._build_existence_query()) assert save_cache[MockKGObject] == {cache_key: proxy_obj.id} + @pytest.fixture() + def list_existence_query(self, monkeypatch): + monkeypatch.setattr( + MockKGObject, + "existence_query_properties", + ( + "a_required_list_of_strings", + "a_required_list_of_datetimes", + "a_required_list_of_linked_objects", + "a_required_list_of_embedded_objects", + ), + ) + + def test_build_existence_query__with_lists(self, list_existence_query): + obj = self._construct_object_all_properties() + linked_id = "https://kg.ebrains.eu/api/instances/00000000-0000-0000-0000-000000000002" + query = obj._build_existence_query() + assert query == { + "a_required_list_of_strings": ["banana", "pear"], + "a_required_list_of_datetimes": ["1900-01-01T00:00:00", "2000-01-01T00:00:00"], + "a_required_list_of_linked_objects": [linked_id, linked_id], + # each embedded object is kept as a separate dict, so that it is matched as a whole + "a_required_list_of_embedded_objects": [{"a_number": 100.0}, {"a_number": 200.0}], + } + assert all(isinstance(item, Equals) for item in query["a_required_list_of_strings"]) + + def test_build_existence_query__with_lists__contains(self, list_existence_query): + obj = self._construct_object_all_properties() + query = obj._build_existence_query("contains") + assert query["a_required_list_of_strings"] == ["banana", "pear"] + assert not any(isinstance(item, Equals) for item in query["a_required_list_of_strings"]) + + def test_build_existence_query__with_empty_list(self, list_existence_query): + obj = self._construct_object_all_properties() + orig_error_handling = obj.__class__.error_handling + obj.__class__.error_handling = ErrorHandling.none + obj.a_required_list_of_strings = [] + try: + with pytest.raises(CannotBuildExistenceQuery) as exc_info: + obj._build_existence_query() + finally: + obj.__class__.error_handling = orig_error_handling + assert exc_info.value.args[0] == "Required value for 'a_required_list_of_strings' is missing" + + def test_build_existence_query__with_linked_object_without_existence_query( + self, list_existence_query, monkeypatch + ): + monkeypatch.setattr(MockKGObject2, "existence_query_properties", None) + obj = self._construct_object_all_properties() + obj.a_required_list_of_linked_objects = [MockKGObject2(a=1)] + with pytest.raises(CannotBuildExistenceQuery, match="MockKGObject2 has no existence query"): + obj._build_existence_query() + + def test_exists_and_save__with_lists_in_existence_query( + self, list_existence_query, mock_client, clear_caches, mocker + ): + obj = self._construct_object_all_properties() + obj.id = None + query = mocker.patch.object(mock_client, "query", return_value=MockKGResponse([])) + + assert not obj.exists(mock_client) + + # previously, no query was made, since the existence query could not be built + query.assert_called_once() + expected_query = MockKGObject.generate_minimal_query(client=mock_client, filters=obj._build_existence_query()) + assert query.call_args.kwargs["query"] == expected_query + + obj.save(mock_client, space="mock", recursive=False) # also checks existence + n_queries = query.call_count + new_obj = self._construct_object_all_properties() + new_obj.id = None + assert new_obj.exists(mock_client) # found in the save cache, without a further query + assert new_obj.id == obj.id + assert query.call_count == n_queries + def test_build_data_all_properties(self): obj = self._construct_object_all_properties() expected = { @@ -818,6 +894,14 @@ def test_generate_cache_key(): generate_cache_key(None) +def test_generate_cache_key__with_lists(): + # the key depends on which property holds a list, but not on the order of its items + assert generate_cache_key({"a": ["x", "y"], "b": "z"}) != generate_cache_key({"a": "z", "b": ["x", "y"]}) + assert generate_cache_key({"a": ["x", "y"]}) == generate_cache_key({"a": ["y", "x"]}) + assert generate_cache_key({"a": [{"b": "x"}, {"b": "y"}]}) == generate_cache_key({"a": [{"b": "y"}, {"b": "x"}]}) + assert generate_cache_key({"a": [{"b": "x"}]}) != generate_cache_key({"a": [{"b": "y"}]}) + + class TestKGProxy: def test_initialization_with_class(self): uri = "https://kg.ebrains.eu/api/instances/00000000-0000-0000-0000-000000001234" diff --git a/test/test_queries.py b/test/test_queries.py index af4639e3..d761dfac 100644 --- a/test/test_queries.py +++ b/test/test_queries.py @@ -1,5 +1,7 @@ import os import json +from datetime import date +from openminds import IRI import pytest from kg_core.request import Stage, Pagination from fairgraph.queries import Query, QueryProperty, Filter, PathElement, Regex, Equals @@ -847,6 +849,65 @@ def test_existence_query_rejects_an_invalid_match(): omcore.Dataset(short_name="FOO")._build_existence_query("fuzzy") +def _new_chapter(authors): + import fairgraph.openminds.publications as ompub + + return ompub.Chapter( + name="There and Back Again", + is_part_of=ompub.Book(id="https://kg.ebrains.eu/api/instances/00000000-0000-0000-0000-000000000001"), + publication_date=date(1937, 9, 21), + authors=authors, + ) + + +def _existence_filter_properties(cls, obj, mock_client, property_name): + query = cls.generate_minimal_query(client=mock_client, filters=obj._build_existence_query()) + return [prop for prop in query["structure"] if prop.get("propertyName", "").startswith(f"Q{property_name}")] + + +def test_existence_query_with_list_of_links(mock_client): + ids = [f"https://kg.ebrains.eu/api/instances/00000000-0000-0000-0000-00000000000{i}" for i in (2, 3)] + chapter = _new_chapter([omcore.Person(id=id, given_name="A", family_name="B") for id in ids]) + props = _existence_filter_properties(type(chapter), chapter, mock_client, "authors") + assert [prop["propertyName"] for prop in props] == ["Qauthors1", "Qauthors2"] + assert [prop["structure"] for prop in props] == [ + [{"path": "@id", "filter": {"op": "CONTAINS", "value": id}}] for id in ids + ] + + +def test_existence_query_with_list_of_embedded_nodes(mock_client): + file = omcore.File( + iri=IRI("https://example.org/data.nwb"), + hashes=[omcore.Hash(algorithm="MD5", digest="abc"), omcore.Hash(algorithm="SHA1", digest="def")], + ) + props = _existence_filter_properties(omcore.File, file, mock_client, "hashes") + assert [prop["propertyName"] for prop in props] == ["Qhashes1", "Qhashes2"] + # the filters for each hash are kept together, so that they must match the same hash in the KG + assert [ + {sub_prop["propertyName"]: sub_prop["filter"] for sub_prop in prop["structure"]} for prop in props + ] == [ + {"Qalgorithm": {"op": "EQUALS", "value": algorithm}, "Qdigest": {"op": "EQUALS", "value": digest}} + for algorithm, digest in (("MD5", "abc"), ("SHA1", "def")) + ] + + +def test_existence_query_with_list_of_saved_and_unsaved_links(mock_client): + saved_id = "https://kg.ebrains.eu/api/instances/00000000-0000-0000-0000-000000000002" + chapter = _new_chapter( + [ + omcore.Person(id=saved_id, given_name="A", family_name="B"), + omcore.Person(given_name="Bilbo", family_name="Baggins"), + ] + ) + props = _existence_filter_properties(type(chapter), chapter, mock_client, "authors") + assert [prop["propertyName"] for prop in props] == ["Qauthors1", "Qauthors2"] + assert props[0]["structure"] == [{"path": "@id", "filter": {"op": "CONTAINS", "value": saved_id}}] + assert {sub_prop["propertyName"]: sub_prop["filter"] for sub_prop in props[1]["structure"]} == { + "Qgiven_name": {"op": "EQUALS", "value": "Bilbo"}, + "Qfamily_name": {"op": "EQUALS", "value": "Baggins"}, + } + + def _filter_properties(query, path): return [prop for prop in query["structure"] if prop.get("path") == path and prop.get("required")] diff --git a/test/test_utility.py b/test/test_utility.py index a31c600f..1a5d5740 100644 --- a/test/test_utility.py +++ b/test/test_utility.py @@ -35,6 +35,13 @@ def test_expand_filter_keeps_siblings(): assert expand_filter(filter) == {"developers": {"given_name": "Bilbo", "family_name": "Baggins"}} +def test_expand_filter_expands_dicts_in_lists(): + filter = {"coordinates": [{"value": 1.0, "unit__name": "mm"}, {"value": 2.0, "unit__name": "mm"}]} + assert expand_filter(filter) == { + "coordinates": [{"value": 1.0, "unit": {"name": "mm"}}, {"value": 2.0, "unit": {"name": "mm"}}] + } + + def test_compact_uri(): context = {"foaf": "http://xmlns.com/foaf/0.1/"} uri_list = "http://xmlns.com/foaf/0.1/Person"