Strip leading and trailing whitespace from text properties when saving

doc/creatingupdating.rst

@@ -23,6 +23,9 @@ To update a node, edit the attributes of the corresponding Python object, then :
 
 (Note that for updating existing objects you don't need to specify the space.)
 
+.. note:: Before saving, **fairgraph** removes leading and trailing whitespace
+          from properties that contain text (including those of embedded nodes).
+
 How does fairgraph distinguish between creating a new node and modifying an existing one?
 =========================================================================================
 

doc/release_notes.rst

@@ -6,6 +6,13 @@ Release notes
 Version 0.16.0
 ==============
 
+Changes in behaviour
+--------------------
+
+- Leading and trailing whitespace is now removed from text properties before they are saved
+  Text loaded from the Knowledge Graph is left as it is until saved, so saving a fetched object corrects
+  any untrimmed text that is stored.
+
 Bug fixes
 ---------
 

fairgraph/kgobject.py

@@ -749,6 +749,10 @@ def save(
             - An `AuthorizationError` if the current user is not authorized to perform the requested operation.
 
         """
+        # Done first, so that the existence query, the data that is sent and the comparison with
+        # the remote data all use the same values. The object is changed in place on purpose:
+        # afterwards it is identical to what is stored in the KG.
+        self._normalize_text()
         if recursive:
             for prop in self.properties:
                 # We do not save reverse properties, those objects must be saved separately.

fairgraph/node.py

@@ -33,6 +33,7 @@
     expand_uri,
     normalize_data,
 )
+from .utility.whitespace import normalize_property_value
 
 if TYPE_CHECKING:
     from .client import KGClient
@@ -462,6 +463,25 @@ def resolve(
                                 setattr(self, prop.name, resolved_values)
         return self
 
+    def _normalize_text(self) -> None:
+        """
+        Normalize, in place, the whitespace in the text values of this node's properties.
+
+        This includes the values of embedded nodes, which are saved along with this node,
+        but not those of linked objects, which are normalised when they are themselves saved.
+        """
+        from .embedded import KGEmbedded  # local import avoids a circular import at load time
+
+        for prop in self.properties:
+            value = getattr(self, prop.name)
+            normalized = normalize_property_value(prop, value)
+            if normalized is not value and normalized != value:
+                setattr(self, prop.name, normalized)
+                value = normalized
+            for item in as_list(value):
+                if isinstance(item, KGEmbedded):
+                    item._normalize_text()
+
     def _build_existence_query(self) -> Union[None, Dict[str, Any]]:
         """
         Generate a KG query definition (as a JSON-LD document) that can be used to

fairgraph/utility/whitespace.py

@@ -0,0 +1,55 @@
+"""
+Normalization of the whitespace in the text values that fairgraph writes to, and queries for in, the Knowledge Graph.
+"""
+
+# Copyright 2018-2026 CNRS and fairgraph authors and/or their employers
+
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+
+#     http://www.apache.org/licenses/LICENSE-2.0
+
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+from __future__ import annotations
+from typing import Any, TYPE_CHECKING
+
+if TYPE_CHECKING:
+    from openminds.properties import Property
+
+
+def normalize_whitespace(value: str, prop: Property) -> str:
+    """
+    Normalize the whitespace in a text value, for storage in the Knowledge Graph.
+
+    Args:
+        value: the text to normalize.
+        prop: the openMINDS property that the text is a value of.
+    """
+    # Currently this only removes leading and trailing whitespace. It takes the property (rather than
+    # just the string) so that internal whitespace can later be handled differently for single-line
+    # and multi-line properties (using ``prop.multiline`` and ``prop.formatting``) by changing only this function.
+    return value.strip()
+
+
+def normalize_property_value(prop: Property, value: Any) -> Any:
+    """
+    Normalize the whitespace in the value(s) of a property, if it holds text.
+
+    Values of properties that do not accept text are returned unchanged, as are ``None``
+    and any items of a list that are not strings (IRIs, dates, nodes, proxies...).
+    An all-whitespace string becomes an empty string.
+    """
+    if str not in prop.types:
+        return value
+    if isinstance(value, str):
+        return normalize_whitespace(value, prop)
+    elif isinstance(value, (list, tuple)):
+        items = [normalize_whitespace(item, prop) if isinstance(item, str) else item for item in value]
+        return tuple(items) if isinstance(value, tuple) else items
+    return value

test/test_whitespace.py

@@ -0,0 +1,210 @@
+"""
+Tests for `fairgraph.utility.whitespace`, and for its use when saving objects.
+
+Only property values are normalized, when saving. Queries (existence queries and filters) are used as given.
+"""
+
+import pytest
+from openminds import IRI
+
+from fairgraph.kgproxy import KGProxy
+import fairgraph.openminds.core as omcore
+import fairgraph.openminds.v4.core as omcore4
+import fairgraph.openminds.v5.core as omcore5
+from fairgraph.utility import ActivityLog
+from fairgraph.utility.whitespace import normalize_property_value, normalize_whitespace
+
+from .utils import MockKGResponse, clear_caches, mock_client
+
+BILBO_ID = "https://kg.ebrains.eu/api/instances/12345678-90ab-cdef-0123-4567890abcde"
+
+
+def _seed_bilbo(mock_client, given_name="Bilbo", family_name="Baggins"):
+    mock_client.instances[BILBO_ID] = {
+        "@id": BILBO_ID,
+        "@type": ["https://openminds.om-i.org/types/Person"],
+        "https://core.kg.ebrains.eu/vocab/meta/space": "common",
+        "https://openminds.om-i.org/props/givenName": given_name,
+        "https://openminds.om-i.org/props/familyName": family_name,
+    }
+
+
+class TestNormalizeWhitespace:
+    prop = omcore.Person.get_property("given_name")
+
+    def test_strips_both_ends(self):
+        assert normalize_whitespace("  Bilbo\t\n", self.prop) == "Bilbo"
+
+    def test_leaves_internal_whitespace_alone(self):
+        assert normalize_whitespace(" Bilbo  Baggins\nof Bag End ", self.prop) == "Bilbo  Baggins\nof Bag End"
+
+    def test_all_whitespace_becomes_empty(self):
+        assert normalize_whitespace(" \t ", self.prop) == ""
+
+
+class TestNormalizePropertyValue:
+    single = omcore.Person.get_property("given_name")
+    multiple = omcore.Person.get_property("alternate_names")
+    not_text = omcore.Person.get_property("contact_information")
+
+    def test_single_value(self):
+        assert normalize_property_value(self.single, " Bilbo ") == "Bilbo"
+
+    def test_list(self):
+        assert normalize_property_value(self.multiple, [" Barrel-rider ", "Ringbearer"]) == [
+            "Barrel-rider",
+            "Ringbearer",
+        ]
+
+    def test_tuple_stays_a_tuple(self):
+        assert normalize_property_value(self.multiple, (" a ", "b ")) == ("a", "b")
+
+    def test_none(self):
+        assert normalize_property_value(self.single, None) is None
+
+    def test_non_str_items_pass_through(self):
+        proxy = KGProxy(omcore.ContactInformation, "https://kg.ebrains.eu/api/instances/abc")
+        assert normalize_property_value(self.multiple, [" a ", proxy]) == ["a", proxy]
+
+    def test_property_that_does_not_take_text_is_untouched(self):
+        proxy = KGProxy(omcore.ContactInformation, "https://kg.ebrains.eu/api/instances/abc")
+        assert normalize_property_value(self.not_text, proxy) is proxy
+
+    def test_iri_is_untouched(self):
+        prop = omcore.File.get_property("iri")
+        iri = IRI("http://example.org/file ")
+        assert normalize_property_value(prop, iri) is iri
+
+    def test_all_whitespace_becomes_empty(self):
+        assert normalize_property_value(self.single, "  ") == ""
+
+
+@pytest.mark.parametrize("module", [omcore4, omcore5], ids=["v4", "v5"])
+def test_text_properties_have_the_formatting_information_that_future_normalization_relies_on(module):
+    """
+    A later version of `normalize_whitespace` is intended to treat single-line and multi-line
+    text differently, using `Property.formatting` and `Property.multiline`.
+    """
+    n_checked = 0
+    for name in dir(module):
+        cls = getattr(module, name)
+        if not (isinstance(cls, type) and hasattr(cls, "properties")):
+            continue
+        for prop in cls.properties:
+            if str in prop.types:
+                n_checked += 1
+                assert prop.formatting in ("text/plain", "text/markdown"), f"{name}.{prop.name}"
+                assert isinstance(prop.multiline, bool), f"{name}.{prop.name}"
+    assert n_checked > 0
+
+
+class TestNormalizeText:
+    def test_strips_text_properties_in_place(self):
+        person = omcore.Person(given_name=" Bilbo ", family_name="Baggins\n", alternate_names=[" Barrel-rider "])
+        person._normalize_text()
+        assert person.given_name == "Bilbo"
+        assert person.family_name == "Baggins"
+        assert person.alternate_names == ["Barrel-rider"]
+
+    def test_embedded_nodes_are_normalized(self):
+        file = omcore.File(
+            name="foo.txt",
+            iri=IRI("http://example.org/foo.txt"),
+            hash=omcore.Hash(algorithm=" SHA-1 ", digest=" abc "),
+        )
+        file._normalize_text()
+        assert file.hash.algorithm == "SHA-1"
+        assert file.hash.digest == "abc"
+
+    def test_linked_objects_are_not_normalized(self):
+        child = omcore.Person(given_name=" Frodo ", family_name="Baggins")
+        dataset_version = omcore.DatasetVersion(custodians=[child], version_identifier=" v1 ")
+        dataset_version._normalize_text()
+        assert dataset_version.version_identifier == "v1"
+        assert child.given_name == " Frodo "
+
+
+class TestSave:
+    def test_new_object_is_saved_stripped(self, mock_client, clear_caches):
+        person = omcore.Person(given_name=" Bilbo ", family_name="Baggins ")
+        person.save(mock_client, space="common")
+        (data,) = mock_client.instances.values()
+        assert data["https://openminds.om-i.org/props/givenName"] == "Bilbo"
+        assert data["https://openminds.om-i.org/props/familyName"] == "Baggins"
+        # the local object now matches what is in the KG
+        assert person.given_name == "Bilbo"
+        assert person.family_name == "Baggins"
+
+    def test_synonym_lists_are_saved_stripped(self, mock_client, clear_caches):
+        person = omcore.Person(given_name="Bilbo", family_name="Baggins", alternate_names=[" Barrel-rider "])
+        person.save(mock_client, space="common")
+        (data,) = mock_client.instances.values()
+        assert data["https://openminds.om-i.org/props/alternateName"] == ["Barrel-rider"]
+
+    def test_untrimmed_new_object_finds_stored_object_and_creates_no_duplicate(self, mock_client, clear_caches):
+        _seed_bilbo(mock_client)
+        person = omcore.Person(given_name="Bilbo ", family_name=" Baggins")
+        log = ActivityLog()
+        person.save(mock_client, space="common", activity_log=log)
+        assert person.id == BILBO_ID
+        assert len(mock_client.instances) == 1
+        assert [entry.type for entry in log.entries] == ["no-op"]
+        assert mock_client.updates == []
+
+    def test_loaded_object_with_untrimmed_stored_value_is_repaired(self, mock_client, clear_caches):
+        _seed_bilbo(mock_client, given_name="Bilbo ")
+        person = omcore.Person.from_id(BILBO_ID, mock_client)
+        assert person.given_name == "Bilbo "  # loading does not strip
+        assert person.remote_data["https://openminds.om-i.org/props/givenName"] == "Bilbo "
+        log = ActivityLog()
+        person.save(mock_client, space="common", activity_log=log)
+        assert [entry.type for entry in log.entries] == ["update"]
+        ((instance_id, payload),) = mock_client.updates
+        assert instance_id == BILBO_ID.split("/")[-1]
+        assert payload["https://openminds.om-i.org/props/givenName"] == "Bilbo"
+
+    def test_linked_child_is_left_alone_when_not_recursive(self, mock_client, clear_caches):
+        child = omcore.Person(id=BILBO_ID, given_name=" Frodo ", family_name="Baggins")
+        parent = omcore.DatasetVersion(custodians=[child], version_identifier=" v1 ")
+        parent.save(mock_client, space="dataset", recursive=False)
+        assert parent.version_identifier == "v1"
+        assert child.given_name == " Frodo "
+
+
+class TestExists:
+    def test_existence_query_is_not_stripped(self):
+        person = omcore.Person(given_name="Bilbo ", family_name=" Baggins")
+        query = person._build_existence_query()
+        assert query == {"given_name": "Bilbo ", "family_name": " Baggins"}
+
+    def test_exists_does_not_strip_and_does_not_change_the_object(self, mock_client, clear_caches):
+        _seed_bilbo(mock_client)
+        person = omcore.Person(given_name="Bilbo ", family_name=" Baggins")
+        assert not person.exists(mock_client)
+        assert person.given_name == "Bilbo "
+        assert person.family_name == " Baggins"
+
+
+def test_loading_does_not_strip_remote_data():
+    data = {
+        "@id": BILBO_ID,
+        "@type": ["https://openminds.om-i.org/types/Person"],
+        "https://openminds.om-i.org/props/givenName": "Bilbo ",
+        "https://openminds.om-i.org/props/familyName": " Baggins",
+    }
+    person = omcore.Person.from_jsonld(data)
+    assert person.given_name == "Bilbo "
+    assert person.family_name == " Baggins"
+
+
+def test_filter_values_are_not_stripped(mock_client):
+    queries = []
+
+    def record_query(query, **kwargs):
+        queries.append(query)
+        return MockKGResponse([])
+
+    mock_client.query = record_query
+    omcore.Person.list(mock_client, family_name="Baggins ")
+    filter_values = [prop["filter"]["value"] for prop in queries[0]["structure"] if "value" in prop.get("filter", {})]
+    assert filter_values == ["Baggins "]