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():