Improve handling of filter values containing "+" and "%"

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:

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)

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

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