fix(textclean): an unknown field type must not lose its protection silently - #474
Merged
Merged
Conversation
…lently
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%.
Contributor
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
FreshData benchmark report —
|
| fixture | n_rows | n_cols | p50 s | p95 s | peak MB | repair % | false-repair % | preserve % | trust | monotonic | export % |
|---|
Authored-code reduction (Metric 6)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
textclean.config_for_fieldmatched the declaredsemantic_typeexactly — 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)and the valueSekr3t-P@ss!!!:semantic_typeidentifier,ticker,emailSekr3t-P@ss!!!preservedfree_textsekr3tpss— correct, it is free textpassword,api_key,secretsekr3tpssTicker,TICKER,"ticker "sekr3tpssidentifer(typo),e-mailsekr3tpssSo 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 fix
Ticker,TICKERand"ticker "resolve toticker.fieldcheckalready extends for an unknownsemantic_type. The message names the type, the specific options that will run, and the known types.Deliberately limited
case,remove_punctuation,strip_html,strip_urls,max_char_repeat,max_length) is opt-in, so a defaultfd.cleannever reached this path and is unchanged.semantic_type=Noneis not a misspelling and stays silent.Worth noting textclean's 28 known types are a superset of both
fieldcheck._KNOWN_SEMANTIC_TYPES(23) andSEMANTIC_TYPES(17), so this was lookup strictness rather than a third taxonomy.Verification
tests/test_field_type_lookup.py: 21 cases covering the case/whitespace variants, the must-still-be-lossyfree_textcontrol, the warning content, and the silent default path.ruff check .clean.