fix(textclean): an unknown field type must not lose its protection silently (#474)

config_for_field matched the declared semantic_type exactly -- case-sensitive
and untrimmed -- and returned the caller's base config on no match. The failure
mode ran backwards for a safety-oriented library: a recognised structural type
was protected from lossy transformation, while an unrecognised one received it
in full. Declaring more about a column bought less protection.

With TextCleanConfig(case="lower", remove_punctuation=True):

    identifier / ticker / email   'Sekr3t-P@ss!!!'  preserved
    password / api_key / secret   'sekr3tpss'
    Ticker / TICKER / 'ticker '   'sekr3tpss'
    identifer (typo) / e-mail     'sekr3tpss'

So a near-miss in the type name quietly downgraded a protected column, and a
credential-bearing column named by a type outside the vocabulary was mangled.

The lookup is now normalised (casefold + strip), so Ticker, TICKER and
"ticker " resolve to ticker. A type that is still unrecognised warns when a
lossy option is active, which is the courtesy fieldcheck already extends for an
unknown semantic_type; the message names the column's type, the specific
options that will run, and the known types.

Deliberately limited:
- Every lossy option is opt-in, so a default fd.clean never reached this path
  and is unchanged.
- The default config has no lossy option, so no warning fires on the common
  path -- verified by a test, since the suite treats freshdata warnings as
  errors.
- semantic_type=None is not a misspelling and stays silent.
- Behaviour for an unrecognised type is otherwise unchanged. Whether it should
  instead default to the structural (lossless) config is a real behaviour
  change and is left for a decision rather than taken here.

Worth noting textclean's 28 known types are a superset of both
fieldcheck._KNOWN_SEMANTIC_TYPES (23) and SEMANTIC_TYPES (17), so this was
lookup strictness rather than a third taxonomy.

21 new tests; full suite 6643 passed / 0 failed, coverage 93.90%.

Co-authored-by: Kevin Costner <kiran.gangalakunta@gmail.com>

Author: kevincostner17

CHANGELOG.md

@@ -7,6 +7,18 @@ adheres to [Semantic Versioning](https://semver.org/).
 ## [Unreleased]
 
 ### Fixed
+- An unrecognised `semantic_type` no longer receives *more* text cleaning than
+  a recognised one. `textclean.config_for_field` matched the declared type
+  exactly — case-sensitively and untrimmed — and fell through to the caller's
+  config on no match, so `"Ticker"` was cleaned more aggressively than
+  `"ticker"`, and a column declared `"password"` or `"api_key"` was case-folded
+  and stripped of punctuation while `"identifier"` was protected. The lookup is
+  now normalised (casefolded and trimmed), and an unrecognised type warns when
+  a lossy option is active, as `fieldcheck` already does for an unknown
+  `semantic_type`. Only opt-in options (`case`, `remove_punctuation`,
+  `strip_html`, `strip_urls`, `max_char_repeat`, `max_length`) were ever
+  affected, so a default `fd.clean` is unchanged and the default path stays
+  warning-free.
 - Currency parsing no longer assumes a US locale for every currency. Every
   comma was deleted and the dot was taken as the decimal point regardless of
   the currency present, so `"EUR 1.200,50"` read as **1.2005** — a thousand-fold

src/freshdata/textclean.py

@@ -21,6 +21,7 @@
 import html as _html
 import re
 import unicodedata
+import warnings
 from collections.abc import Callable, Mapping
 from dataclasses import dataclass, field, replace
 from html.parser import HTMLParser
@@ -45,12 +46,23 @@
 _IRREGULAR_WS_RE = re.compile("[\u00a0\u202f\u2000-\u200a\u2007\u3000]")
 _WS_RE = re.compile(r"\s+")
 # Unicode punctuation → ASCII equivalents (smart quotes, dashes, ellipsis).
-_PUNCT_MAP = str.maketrans({
-    "‘": "'", "’": "'", "‚": "'", "′": "'",
-    "“": '"', "”": '"', "„": '"', "″": '"',
-    "–": "-", "—": "-", "―": "-", "−": "-",
-    "…": "...",
-})
+_PUNCT_MAP = str.maketrans(
+    {
+        "‘": "'",
+        "’": "'",
+        "‚": "'",
+        "′": "'",
+        "“": '"',
+        "”": '"',
+        "„": '"',
+        "″": '"',
+        "–": "-",
+        "—": "-",
+        "―": "-",
+        "−": "-",
+        "…": "...",
+    }
+)
 
 
 class _TextExtractor(HTMLParser):
@@ -121,21 +133,64 @@ def __post_init__(self) -> None:
 
 #: Field types whose values are structural — punctuation, casing and length
 #: are meaningful, so lossy operations are withheld even if configured.
-_STRUCTURAL_TYPES = frozenset({
-    "numeric", "integer", "float", "currency_amount", "rate", "percentage",
-    "identifier", "account_number", "national_id", "postal_code",
-    "email", "url", "phone", "date_like", "date", "datetime",
-    "stock_ticker", "ticker", "category_code", "boolean_like",
-})
-_ENTITY_TYPES = frozenset({
-    "person_name", "company_name", "entity_name", "city", "country", "address",
-})
+_STRUCTURAL_TYPES = frozenset(
+    {
+        "numeric",
+        "integer",
+        "float",
+        "currency_amount",
+        "rate",
+        "percentage",
+        "identifier",
+        "account_number",
+        "national_id",
+        "postal_code",
+        "email",
+        "url",
+        "phone",
+        "date_like",
+        "date",
+        "datetime",
+        "stock_ticker",
+        "ticker",
+        "category_code",
+        "boolean_like",
+    }
+)
+_ENTITY_TYPES = frozenset(
+    {
+        "person_name",
+        "company_name",
+        "entity_name",
+        "city",
+        "country",
+        "address",
+    }
+)
 #: Content-bearing types where typography *is* content: an em-dash, a curly
 #: quote or a prime mark (12″) in a product name or a comment carries meaning,
 #: so the punctuation→ASCII mapping is withheld for them.
 _CONTENT_TYPES = frozenset({"free_text", "text"}) | _ENTITY_TYPES
 
 
+def _lossy_options(cfg: TextCleanConfig) -> list[str]:
+    """Names of the opt-in, information-destroying options enabled on *cfg*."""
+    enabled = []
+    if cfg.case is not None:
+        enabled.append(f"case={cfg.case!r}")
+    if cfg.remove_punctuation:
+        enabled.append("remove_punctuation")
+    if cfg.strip_html:
+        enabled.append("strip_html")
+    if cfg.strip_urls:
+        enabled.append("strip_urls")
+    if cfg.max_char_repeat is not None:
+        enabled.append("max_char_repeat")
+    if cfg.max_length is not None:
+        enabled.append("max_length")
+    return enabled
+
+
 def config_for_field(
     semantic_type: str | None,
     base: TextCleanConfig | None = None,
@@ -149,17 +204,38 @@ def config_for_field(
     mapping only runs on untyped or structural fields.
     """
     cfg = base or TextCleanConfig()
-    if semantic_type in _STRUCTURAL_TYPES:
+    # Match on a normalized name: "Ticker", "TICKER" and "ticker " all name the
+    # same field type, and an exact-only lookup silently downgraded them to the
+    # unrestricted config -- i.e. a near-miss removed protection rather than
+    # adding it.
+    key = semantic_type.strip().casefold() if isinstance(semantic_type, str) else None
+    if key in _STRUCTURAL_TYPES:
         return replace(
-            cfg, strip_html=False, strip_urls=False, case=None,
-            remove_punctuation=False, max_char_repeat=None, max_length=cfg.max_length,
+            cfg,
+            strip_html=False,
+            strip_urls=False,
+            case=None,
+            remove_punctuation=False,
+            max_char_repeat=None,
+            max_length=cfg.max_length,
         )
-    if semantic_type in _ENTITY_TYPES:
+    if key in _ENTITY_TYPES:
         case = cfg.case if cfg.case == "title" else None
-        return replace(cfg, remove_punctuation=False, case=case,
-                       normalize_punctuation=False)
-    if semantic_type in _CONTENT_TYPES:
+        return replace(cfg, remove_punctuation=False, case=case, normalize_punctuation=False)
+    if key in _CONTENT_TYPES:
         return replace(cfg, normalize_punctuation=False)
+    if key is not None and _lossy_options(cfg):
+        # An unrecognized type keeps the caller's config, which means a lossy
+        # option applies in full. Say so, the way fieldcheck already warns for
+        # an unknown semantic_type, so a misspelling is not silent.
+        warnings.warn(
+            f"unknown semantic_type {semantic_type!r} for text cleaning: no "
+            f"field-specific protection applies, so lossy options "
+            f"({', '.join(_lossy_options(cfg))}) run on this column in full. "
+            f"Known types: {sorted(_STRUCTURAL_TYPES | _ENTITY_TYPES | _CONTENT_TYPES)}",
+            UserWarning,
+            stacklevel=2,
+        )
     return cfg
 
 
@@ -211,9 +287,9 @@ def step(name: str, new: str) -> None:
         step("strip_urls", _URL_RE.sub(" ", out))
     if cfg.strip_control_chars:
         cleaned = "".join(
-            c for c in out
-            if not (unicodedata.category(c) == "Cc" and c not in "\t\n\r")
-            and c not in _BIDI_MARKS
+            c
+            for c in out
+            if not (unicodedata.category(c) == "Cc" and c not in "\t\n\r") and c not in _BIDI_MARKS
         )
         step("strip_control_chars", cleaned)
     if cfg.strip_zero_width:
@@ -225,8 +301,10 @@ def step(name: str, new: str) -> None:
         pattern = r"(.)\1{" + str(n) + ",}"
         step("collapse_repeats", re.sub(pattern, lambda m: m.group(1) * n, out))
     if cfg.remove_punctuation:
-        step("remove_punctuation", "".join(
-            c for c in out if not unicodedata.category(c).startswith("P")))
+        step(
+            "remove_punctuation",
+            "".join(c for c in out if not unicodedata.category(c).startswith("P")),
+        )
     if cfg.case:
         step(f"case_{cfg.case}", getattr(out, cfg.case)())
     for name, fn in cfg.custom:
@@ -327,8 +405,9 @@ def clean_text(
     types = dict(field_types or {})
 
     for col in cols:
-        cfg = config_for_field(types[col], config) if col in types else (
-            config or TextCleanConfig())
+        cfg = (
+            config_for_field(types[col], config) if col in types else (config or TextCleanConfig())
+        )
         series = df[col]
         report.values_seen += int(series.notna().sum())
         # ponytail: per-cell python loop; vectorize per-op if profiling demands
@@ -344,11 +423,15 @@ def clean_text(
             if result.changed:
                 positions.append(pos)
                 cleaned_values.append(result.cleaned)
-                report.changes.append({
-                    "row": idx, "column": str(col),
-                    "original": val, "cleaned": result.cleaned,
-                    "transforms": list(result.transforms),
-                })
+                report.changes.append(
+                    {
+                        "row": idx,
+                        "column": str(col),
+                        "original": val,
+                        "cleaned": result.cleaned,
+                        "transforms": list(result.transforms),
+                    }
+                )
         if positions:
             new_col = series.copy()
             new_col.iloc[positions] = cleaned_values

tests/test_field_type_lookup.py

@@ -0,0 +1,88 @@
+"""An unrecognised field type must not silently lose protection (FD2-004).
+
+``config_for_field`` matched the declared ``semantic_type`` exactly -- case
+sensitive and untrimmed -- and fell through to the caller's base config on no
+match. The failure mode ran backwards for a safety-oriented library: a
+recognised structural type was protected from lossy transformation, while an
+unrecognised one received it in full. So ``"Ticker"`` was cleaned more
+aggressively than ``"ticker"``, and a column declared ``"password"`` was
+case-folded and stripped of punctuation.
+
+Every lossy option is opt-in, so a default ``fd.clean`` was never affected.
+"""
+
+from __future__ import annotations
+
+import warnings
+
+import pytest
+
+from freshdata.textclean import TextCleanConfig, clean_text_value, config_for_field
+
+LOSSY = TextCleanConfig(case="lower", remove_punctuation=True)
+SECRET = "Sekr3t-P@ss!!!"
+
+
+@pytest.mark.parametrize("declared", ["ticker", "Ticker", "TICKER", " ticker ", "\tticker\n"])
+def test_case_and_whitespace_variants_resolve_to_the_same_protection(declared):
+    """'Ticker' names the same field type as 'ticker'."""
+    assert config_for_field(declared, LOSSY).case is None
+    assert clean_text_value(SECRET, config_for_field(declared, LOSSY)).cleaned == SECRET
+
+
+@pytest.mark.parametrize("declared", ["email", "EMAIL", "Person_Name", "IDENTIFIER", "Free_Text"])
+def test_other_types_normalise_too(declared):
+    """The normalisation is not special-cased to one type."""
+    with warnings.catch_warnings():
+        warnings.simplefilter("error")
+        config_for_field(declared, LOSSY)  # must not warn: all are known
+
+
+def test_a_structural_type_still_refuses_lossy_options():
+    assert clean_text_value(SECRET, config_for_field("identifier", LOSSY)).cleaned == SECRET
+
+
+def test_free_text_still_accepts_them():
+    """The protection must not become blanket: free text really is free text."""
+    assert clean_text_value(SECRET, config_for_field("free_text", LOSSY)).cleaned == "sekr3tpss"
+
+
+@pytest.mark.parametrize("declared", ["password", "api_key", "secret", "identifer", "e-mail"])
+def test_an_unknown_type_warns_when_a_lossy_option_is_active(declared):
+    """Silence let a misspelling quietly downgrade a column.
+
+    fieldcheck already warns for an unknown semantic_type; this is the same
+    courtesy on the cleaning side.
+    """
+    with pytest.warns(UserWarning, match="unknown semantic_type"):
+        config_for_field(declared, LOSSY)
+
+
+def test_the_warning_names_the_offending_options_and_the_known_types():
+    with pytest.warns(UserWarning) as caught:
+        config_for_field("password", LOSSY)
+    message = str(caught[0].message)
+    assert "password" in message
+    assert "case='lower'" in message and "remove_punctuation" in message
+    assert "ticker" in message  # the known-type list is included
+
+
+def test_no_warning_on_the_default_path():
+    """A default config has no lossy option, so an unknown type is harmless."""
+    with warnings.catch_warnings():
+        warnings.simplefilter("error")
+        config_for_field("password", TextCleanConfig())
+        config_for_field(None, TextCleanConfig())
+
+
+def test_none_is_not_reported_as_an_unknown_type():
+    """No declaration is not a misspelling; it must stay quiet."""
+    with warnings.catch_warnings():
+        warnings.simplefilter("error")
+        assert config_for_field(None, LOSSY).case == "lower"
+
+
+def test_a_non_string_type_does_not_crash():
+    with warnings.catch_warnings():
+        warnings.simplefilter("ignore")
+        assert config_for_field(42, LOSSY).case == "lower"  # type: ignore[arg-type]