diff --git a/fairgraph/node.py b/fairgraph/node.py index 1969084b..6f699d31 100644 --- a/fairgraph/node.py +++ b/fairgraph/node.py @@ -26,7 +26,14 @@ from .base import Resolvable, ErrorHandling, Releasable from .kgproxy import KGProxy from .kgquery import KGQuery -from .queries import Equals, QueryProperty, get_query_properties, get_query_filter_property, get_filter_value +from .queries import ( + Equals, + QueryProperty, + get_query_properties, + get_query_filter_property, + get_filter_value, + to_single_value_filters, +) from .errors import ResolutionFailure, CannotBuildExistenceQuery from .utility import ( as_list, # temporary for backwards compatibility (a lot of code imports it from here) @@ -303,7 +310,16 @@ def generate_query_filter_properties( properties = [] for prop in cls.all_properties: if prop.name in filters: - properties.append(get_query_filter_property(prop, cls.context, filters[prop.name])) + # the KG accepts one value per filter, so a filter containing a list of values + # becomes one copy of the property per value, each of which must match + single_value_filters = to_single_value_filters(filters[prop.name]) + if len(single_value_filters) == 1: + properties.append(get_query_filter_property(prop, cls.context, single_value_filters[0])) + else: + for i, single_value_filter in enumerate(single_value_filters, start=1): + properties.append( + get_query_filter_property(prop, cls.context, single_value_filter, name=f"Q{prop.name}{i}") + ) return properties @classmethod diff --git a/fairgraph/queries.py b/fairgraph/queries.py index 549bac1e..89d89938 100644 --- a/fairgraph/queries.py +++ b/fairgraph/queries.py @@ -75,7 +75,8 @@ class Equals(str): The KG ignores case when matching, but not whitespace, so `Equals("FOO")` does match "foo", but not "FOO ". - Only a single value is supported: a list of `Equals` values is filtered with "CONTAINS". + As with any filter value, a list of `Equals` values matches only if every item is matched, + each with "EQUALS". Args: value (str): the value the property must be equal to. @@ -483,12 +484,50 @@ def get_query_properties( return properties -def get_query_filter_property(property, context, filter: Any) -> QueryProperty: +def to_single_value_filters(filter: Any) -> List[Any]: + """ + Return one or more filters equivalent to `filter`, each with a single value at every leaf. + + A filter that contains no lists is returned unchanged, as the only element of the result. + The KG query API accepts only a single value per filter, so a filter containing a list of values + is expressed by repeating the property once per item, each copy being required: in that case + one filter is returned per item. The list may be nested within a dict of filters for a linked + or embedded node, in which case each copy of the dict contains one of the items. + + Example: + >>> to_single_value_filters({"family_name": ["Baggins", "Oakenshield"], "given_name": "A"}) + [{"family_name": "Baggins", "given_name": "A"}, {"family_name": "Oakenshield", "given_name": "A"}] + """ + if isinstance(filter, (list, tuple)): + if len(filter) == 0: + raise ValueError("Cannot filter on an empty list of values") + return list(filter) + if isinstance(filter, dict): + split = {key: to_single_value_filters(value) for key, value in filter.items()} + keys_with_lists = [key for key, values in split.items() if len(values) > 1] + if len(keys_with_lists) > 1: + raise ValueError( + f"Only one list of filter values is supported within a single property, not {len(keys_with_lists)}" + ) + base = {key: values[0] for key, values in split.items()} + if keys_with_lists: + key = keys_with_lists[0] + return [{**base, key: item} for item in split[key]] + return [base] + return [filter] + + +def get_query_filter_property(property, context, filter: Any, name: Optional[str] = None) -> QueryProperty: """ Generate a QueryProperty instance containing a filter, for use in constructing a KG query definition. + + The filter must contain a single value: use `to_single_value_filters()` for filters containing lists. + The name of the QueryProperty defaults to "Q" followed by the property name. """ assert filter is not None + if name is None: + name = f"Q{property.name}" if isinstance(filter, dict): # we pass the filter through to the next level filter_obj = None @@ -508,7 +547,7 @@ def get_query_filter_property(property, context, filter: Any) -> QueryProperty: if any(issubclass(_type, Node) for _type in property.types): assert all(issubclass(_type, Node) for _type in property.types) - prop = QueryProperty(expanded_path, name=f"Q{property.name}", required=True, reverse=property.reverse) + prop = QueryProperty(expanded_path, name=name, required=True, reverse=property.reverse) if filter_obj: if filter_obj.value.startswith("https://kg.ebrains.eu/api/instances"): filter_path = "@id" @@ -525,7 +564,7 @@ def get_query_filter_property(property, context, filter: Any) -> QueryProperty: prop.properties.extend(child_properties) break else: - prop = QueryProperty(expanded_path, name=f"Q{property.name}", filter=filter_obj, required=True) + prop = QueryProperty(expanded_path, name=name, filter=filter_obj, required=True) return prop @@ -564,7 +603,9 @@ def is_valid(val): isinstance(val, KGProxy) and not set(val.classes).isdisjoint(property.types) ) - if isinstance(value, list) and len(value) > 0: + if isinstance(value, (list, tuple)) and len(value) == 0: + raise ValueError(f"Cannot filter {property.name} on an empty list of values") + if isinstance(value, list): valid_type = all(is_valid(item) for item in value) have_multiple = True else: @@ -573,7 +614,7 @@ def is_valid(val): if not valid_type: if property.name == "hash": # bit of a hack filter_value = value - elif isinstance(value, str) and value.startswith("http"): # for @id + elif all(isinstance(item, str) and item.startswith("http") for item in as_list(value)): # for @id filter_value = value else: raise TypeError("{} must be of type {}, not {}".format(property.name, property.types, type(value))) diff --git a/fairgraph/utility/jsonld.py b/fairgraph/utility/jsonld.py index 0a55e813..a7b3ce72 100644 --- a/fairgraph/utility/jsonld.py +++ b/fairgraph/utility/jsonld.py @@ -219,7 +219,6 @@ def expand_filter(filter_dict: Dict[str, Any]): local_path = expanded parts = key.split("__") for part in parts[:-1]: - local_path[part] = {} - local_path = local_path[part] + local_path = local_path.setdefault(part, {}) local_path[parts[-1]] = value return expanded diff --git a/test/test_queries.py b/test/test_queries.py index 9d49c809..af4639e3 100644 --- a/test/test_queries.py +++ b/test/test_queries.py @@ -847,6 +847,101 @@ def test_existence_query_rejects_an_invalid_match(): omcore.Dataset(short_name="FOO")._build_existence_query("fuzzy") +def _filter_properties(query, path): + return [prop for prop in query["structure"] if prop.get("path") == path and prop.get("required")] + + +def test_list_filter_repeats_the_property_once_per_value(mock_client): + # each value must be matched by some developer, as in a hand-written KG query + query = omcore.SoftwareVersion.generate_query( + client=mock_client, + space=None, + filters={"developers__family_name": [Equals("Baggins"), Equals("Oakenshield")]}, + ) + props = _filter_properties(query, "https://openminds.om-i.org/props/developer") + assert [prop["propertyName"] for prop in props] == ["Qdevelopers1", "Qdevelopers2"] + assert [prop["structure"] for prop in props] == [ + [ + { + "path": "https://openminds.om-i.org/props/familyName", + "propertyName": "Qfamily_name", + "filter": {"op": "EQUALS", "value": family_name}, + "required": True, + } + ] + for family_name in ("Baggins", "Oakenshield") + ] + + +def test_list_filter_keeps_sibling_filters_in_each_copy(mock_client): + query = omcore.SoftwareVersion.generate_query( + client=mock_client, + space=None, + filters={"developers__given_name": "Bilbo", "developers__family_name": ["Baggins", "Oakenshield"]}, + ) + props = _filter_properties(query, "https://openminds.om-i.org/props/developer") + assert len(props) == 2 + for prop, family_name in zip(props, ("Baggins", "Oakenshield")): + filters = {sub_prop["propertyName"]: sub_prop["filter"]["value"] for sub_prop in prop["structure"]} + assert filters == {"Qgiven_name": "Bilbo", "Qfamily_name": family_name} + + +@pytest.mark.parametrize("as_objects", [True, False]) +def test_list_filter_on_links(mock_client, as_objects): + import fairgraph.openminds.publications as ompub + + ids = [f"https://kg.ebrains.eu/api/instances/00000000-0000-0000-0000-00000000000{i}" for i in (1, 2)] + if as_objects: + values = [omcore.Person(id=id, given_name="A", family_name="B") for id in ids] + else: + values = ids + query = ompub.Chapter.generate_query(client=mock_client, space=None, filters={"authors": values}) + props = _filter_properties(query, "https://openminds.om-i.org/props/author") + assert [prop["propertyName"] for prop in props] == ["Qauthors1", "Qauthors2"] + assert [prop["structure"][0]["path"] for prop in props] == ["@id", "@id"] + assert [prop["structure"][0]["filter"]["value"] for prop in props] == ids + + +def test_list_filter_with_one_value_is_the_same_as_a_single_value(mock_client): + single = omcore.SoftwareVersion.generate_query( + client=mock_client, space=None, filters={"developers__family_name": "Baggins"} + ) + as_list = omcore.SoftwareVersion.generate_query( + client=mock_client, space=None, filters={"developers__family_name": ["Baggins"]} + ) + assert as_list == single + + +def test_list_filter_rejects_an_empty_list(mock_client): + with pytest.raises(ValueError, match="empty list"): + omcore.SoftwareVersion.generate_query(client=mock_client, space=None, filters={"developers__family_name": []}) + + +def test_list_filter_rejects_two_lists_within_one_property(mock_client): + with pytest.raises(ValueError, match="Only one list"): + omcore.SoftwareVersion.generate_query( + client=mock_client, + space=None, + filters={"developers__family_name": ["Baggins", "Oakenshield"], "developers__given_name": ["A", "B"]}, + ) + + +@skip_if_no_connection +def test_execute_query_with_list_filter(kg_client): + # software versions developed by both people should be those found by filtering on each separately + family_names = ["Davison", "Appukuttan"] + + def software_version_ids(family_name): + software_versions = omcore.SoftwareVersion.list( + kg_client, release_status="released", size=1000, developers__family_name=family_name + ) + return {software_version.id for software_version in software_versions} + + expected_ids = set.intersection(*(software_version_ids(Equals(family_name)) for family_name in family_names)) + assert len(expected_ids) > 0 + assert software_version_ids([Equals(family_name) for family_name in family_names]) == expected_ids + + def _all_kg_classes(): import fairgraph.openminds.v4 as v4 import fairgraph.openminds.v5 as v5 diff --git a/test/test_utility.py b/test/test_utility.py index 36f5208f..a31c600f 100644 --- a/test/test_utility.py +++ b/test/test_utility.py @@ -27,6 +27,14 @@ def test_expand_filter(): assert result == expected +def test_expand_filter_keeps_siblings(): + filter = { + "developers__given_name": "Bilbo", + "developers__family_name": "Baggins", + } + assert expand_filter(filter) == {"developers": {"given_name": "Bilbo", "family_name": "Baggins"}} + + def test_compact_uri(): context = {"foaf": "http://xmlns.com/foaf/0.1/"} uri_list = "http://xmlns.com/foaf/0.1/Person"