From 1ccb45d680a665bca9ac7fd47d0fad5fb848d40c Mon Sep 17 00:00:00 2001 From: Andrew Davison Date: Mon, 14 Sep 2026 18:08:13 +0200 Subject: [PATCH] Improve handling of filter values containing "+" and "%" In the master branch, get_filter_value() truncates string filter values at the first "+" (or raised ValueError if it appeared in the first three characters), to work around a KG bug affecting filter values sent as request parameters for stored queries. This change removes the workaround since fairgraph no longer uses stored queries, it writes fresh queries each time that include the filter values, and in this case filters containing "+" are handled correctly. This means that we can now match values such as "C++", email addresses with a "+" suffix, or timestamps with a UTC offset. This also removes the exemptions for date/datetime properties and for Regex values, which existed only to bypass the workaround, and replaces the Regex-specific test (and the timezone-aware datetime test) with a single test that filter values containing "+" are passed through unchanged. The underlying bug still affects filter values sent as request parameters: the KG decodes parameter values twice, once by Spring and again in DataQueryBuilder.createAqlForFilter() in marmotgraph-core, so a "+" is received as a space and a "%" causes an error or is decoded together with the following characters. The bug is absent from the v4 branch of marmotgraph-core. fairgraph no longer passes filter values as parameters, but KGClient.query() still accepts a `filter` dict, so it now raises ValueError if a filter value contains "+" or "%", rather than silently returning the wrong results. A live test checks the current KG behaviour, so that we will know when v4 has been deployed and this check can be removed. --- fairgraph/client.py | 20 +++++++++++++++++ fairgraph/queries.py | 13 +---------- test/test_client.py | 51 ++++++++++++++++++++++++++++++++++++++++++++ test/test_queries.py | 49 ++++++++++++++++++++++++++---------------- 4 files changed, 103 insertions(+), 30 deletions(-) diff --git a/fairgraph/client.py b/fairgraph/client.py index 1a5dbc26..5aa67029 100644 --- a/fairgraph/client.py +++ b/fairgraph/client.py @@ -249,8 +249,28 @@ def query( Returns: A ResultPage object containing a list of JSON-LD instances that satisfy the query, along with metadata about the query results such as total number of instances, and pagination information. + + Raises: + ValueError: if a value in `filter` contains "+" or "%" (see below). """ release_status = handle_scope_keyword(scope, release_status) + if filter: + # `filter` values are sent to the KG as request parameters, which the KG decodes twice: + # once by Spring, and again in DataQueryBuilder.createAqlForFilter() in marmotgraph-core. + # As a result, a "+" is received as a space, so the query silently returns the wrong results, + # and a "%" either causes a "400 Bad Request" error or is decoded together with the following + # characters. Filter values given within the query definition itself are not affected. + # The second decoding is absent from the v4 branch of marmotgraph-core, which replaces DataQueryBuilder. + # If test_kg_misreads_plus_and_percent_in_query_parameters in test/test_client.py starts failing, + # the KG has been fixed and this check can be removed. + for name, value in filter.items(): + values = value if isinstance(value, (list, tuple)) else [value] + if any(isinstance(item, str) and ("+" in item or "%" in item) for item in values): + raise ValueError( + f"Cannot filter on {name}={value!r} using a query parameter, since the KG does not handle " + "'+' or '%' in parameter values correctly. Include the filter value in the query definition " + "instead." + ) query_id = query.get("@id", None) if use_stored_query: diff --git a/fairgraph/queries.py b/fairgraph/queries.py index 3f4909f7..1b6879e8 100644 --- a/fairgraph/queries.py +++ b/fairgraph/queries.py @@ -520,8 +520,6 @@ def get_filter_value(property, value: Any) -> Union[str, List[str]]: """ from .kgproxy import KGProxy - has_temporal_type = any(temporal_type in property.types for temporal_type in (datetime, date)) - def is_valid(val): if isinstance(val, str): try: @@ -559,10 +557,7 @@ def is_valid(val): filter_items = [] for item in as_list(value): - if isinstance(item, Regex): - # a pattern must be passed through untouched, in particular past the "+" workaround below - filter_item = item - elif isinstance(item, IRI): + if isinstance(item, IRI): filter_item = item.value elif isinstance(item, (date, datetime)): filter_item = item.isoformat() @@ -572,12 +567,6 @@ def is_valid(val): # todo: consider using client.uri_from_uuid() # would require passing client as arg filter_item = f"https://kg.ebrains.eu/api/instances/{item}" - elif isinstance(item, str) and "+" in item and not has_temporal_type: # workaround for KG bug - invalid_char_index = item.index("+") - if invalid_char_index < 3: - raise ValueError(f"Cannot use {item} as filter, contains invalid characters") - filter_item = item[:invalid_char_index] - warn(f"Truncating filter value {item} --> {filter_item}") else: filter_item = item filter_items.append(filter_item) diff --git a/test/test_client.py b/test/test_client.py index b8c8e2fd..794eb8dd 100644 --- a/test/test_client.py +++ b/test/test_client.py @@ -1,6 +1,7 @@ import os import pytest +from kg_core.request import Stage, Pagination from kg_core.response import Error as KGError from fairgraph.kgobject import KGObject from fairgraph.queries import Query, QueryProperty, Filter @@ -146,6 +147,56 @@ def test_query_filter_by_space(kg_client): assert "model" == list(spaces)[0] +@pytest.mark.parametrize("use_stored_query", [False, True]) +@pytest.mark.parametrize("value", ["application/ld+json", "100%"]) +def test_query_rejects_plus_and_percent_in_filter_parameters(offline_kg_client, mocker, use_stored_query, value): + # the KG misreads "+" and "%" in request parameters (see test_kg_misreads_plus_and_percent_in_query_parameters), + # so the query must not be sent + for method in ("test_query", "execute_query_by_id"): + mocker.patch.object(offline_kg_client._kg_client.queries, method, side_effect=AssertionError("query was sent")) + query = {"@id": "https://kg.ebrains.eu/api/instances/00000000-0000-0000-0000-000000000000"} + with pytest.raises(ValueError, match="Cannot filter on name="): + offline_kg_client.query(query, filter={"name": value}, use_stored_query=use_stored_query) + + +@skip_if_no_connection +def test_kg_misreads_plus_and_percent_in_query_parameters(kg_client): + """ + The KG decodes request parameter values twice (once by Spring, and again in + DataQueryBuilder.createAqlForFilter() in marmotgraph-core), so a "+" in a filter parameter + is received as a space, and a "%" that isn't part of a valid escape sequence causes an error. + KGClient.query() therefore refuses filter parameters containing "+" or "%". + + The second decoding is absent from the v4 branch of marmotgraph-core. If this test starts failing, + the KG has been fixed, and that check can be removed. + """ + query = Query( + node_type="https://openminds.om-i.org/types/ContentType", + properties=[ + QueryProperty("@type"), + QueryProperty( + "https://openminds.om-i.org/props/name", + name="name", + filter=Filter("CONTAINS", parameter="name"), + required=True, + ), + ], + ).serialize() + + def run_query(value): + # calls kg-core directly, since KGClient.query() rejects these filters + return kg_client._kg_client.queries.test_query( + query, additional_request_params={"name": value}, stage=Stage.RELEASED, pagination=Pagination(size=20) + ) + + def names_found(value): + return [item["name"] for item in run_query(value).data] + + assert "application/ld+json" in names_found("application/ld") + assert "application/ld+json" not in names_found("application/ld+json") + assert run_query("100%").error is not None + + @skip_if_no_connection def test_get_admin_client(kg_client): admin_client = kg_client._kg_admin_client diff --git a/test/test_queries.py b/test/test_queries.py index 4c478cd1..e9c02d83 100644 --- a/test/test_queries.py +++ b/test/test_queries.py @@ -1,12 +1,10 @@ import os import json -from datetime import datetime - import pytest from kg_core.request import Stage, Pagination -from openminds.properties import Property -from fairgraph.queries import Query, QueryProperty, Filter, PathElement, Regex, get_filter_value +from fairgraph.queries import Query, QueryProperty, Filter, PathElement, Regex import fairgraph.openminds.core as omcore +import fairgraph.openminds.controlled_terms as omterms from .utils import kg_client, mock_client, skip_if_no_connection @@ -634,13 +632,6 @@ def test_path_element_conflicts_with_top_level_reverse(): ) -def test_get_filter_value_preserves_timezone_aware_datetime(): - prop = Property("timestamp", datetime, "https://openminds.om-i.org/props/timestamp") - timestamp = "2026-09-13T12:00:00+00:00" - - assert get_filter_value(prop, timestamp) == timestamp - - @skip_if_no_connection def test_execute_query_with_multi_element_path_with_path_elements(kg_client): # This query should return only Files belonging to the specified dataset. @@ -733,13 +724,35 @@ def test_generate_query_with_plain_string_filter_still_uses_contains(mock_client assert filters == [{"op": "CONTAINS", "value": "Müller"}] -def test_regex_filter_is_not_truncated_at_a_plus_sign(mock_client): - # plain string filter values containing "+" are truncated to work around a KG bug; - # a regular expression must survive intact - pattern = Regex("^CLARITY[-+/]TDE$") - query = omcore.Person.generate_query(client=mock_client, space=None, filters={"family_name": pattern}) - filters = [prop["filter"] for prop in query["structure"] if prop.get("propertyName", None) == "Qfamily_name"] - assert filters == [{"op": "REGEX", "value": "^CLARITY[-+/]TDE$"}] +def test_filter_values_containing_a_plus_sign_are_not_modified(mock_client): + # filter values containing "+" used to be truncated, to work around a KG bug that no longer occurs + for cls, property_name, value, expected in ( + (omterms.ProgrammingLanguage, "name", "C++", {"op": "CONTAINS", "value": "C++"}), + ( + omcore.ContactInformation, + "email", + "jane.doe+kg@example.org", + {"op": "CONTAINS", "value": "jane.doe+kg@example.org"}, + ), + (omterms.Technique, "name", Regex("^CLARITY[-+/]TDE$"), {"op": "REGEX", "value": "^CLARITY[-+/]TDE$"}), + ( + omcore.Comment, + "timestamp", + "2025-01-17T16:22:53.824903+00:00", + {"op": "EQUALS", "value": "2025-01-17T16:22:53.824903+00:00"}, + ), + ( + omcore.Comment, + "timestamp", + "2026-09-13T14:00:00+02:00", + {"op": "EQUALS", "value": "2026-09-13T14:00:00+02:00"}, + ), + ): + query = cls.generate_query(client=mock_client, space=None, filters={property_name: value}) + filters = [ + prop["filter"] for prop in query["structure"] if prop.get("propertyName", None) == f"Q{property_name}" + ] + assert filters == [expected] def test_regex_rejects_a_malformed_pattern():