mirror of
https://github.com/data-privacy-stack/presidio.git
synced 2026-09-21 05:27:53 -05:00
fix(analyzer): remove a non-language code and a duplicate entry from default_recognizers.yaml (#2236)
* fix(analyzer): remove a non-language code and a duplicate entry from default_recognizers.yaml The five Korean entries listed `kr` next to `ko`. `ko` is the ISO 639-1 language code; `kr` is the ISO 3166-1 country code, which the entries already carry in `country_code`. The loader builds one recognizer per listed language, so `kr` either logged a "language is not supported" warning on every load, or - for a registry configured with it - registered a second copy of all five Korean recognizers that no NLP engine can serve. UkPostcodeRecognizer was listed twice: #1858 added it, and #1857 added it again and merged later. The registry built two identical instances. Generated-by: Claude Opus 5 * test(analyzer): guard default_recognizers.yaml against duplicates and non-language codes Both tests fail on main and pass with the config fix. The language guard keeps an explicit set of ISO 639-1 codes so that adding a language is a deliberate edit rather than a side effect of a new entry. Generated-by: Claude Opus 5 * test(analyzer): stop asserting kr as a servable language for KrPassport #2170 parametrised this over ["ko", "kr"] to match the four sibling Kr* entries. `kr` is the ISO 3166-1 country code the entry already carries in `country_code`, and no NLP engine can serve it, so the `kr` case asserted the defect rather than the contract. Generated-by: Claude Opus 5 * test(analyzer): read YAML entries the way the loader does Both guards indexed every entry as a dict. `RecognizerListLoader` also accepts bare-string entries such as `- SpacyRecognizer`, so one of those in the shipped file would have crashed the guards with a `TypeError` instead of failing on the assertion they exist to make. Names now go through `_entry_name`, which prefers the instance `name` and falls back to `RecognizerListLoader.get_recognizer_name` for a bare string or a class-only entry. The duplicate check stays keyed on the instance name rather than the class: two entries may share a class as long as `class_name` carries it and `name` tells the instances apart. Languages go through `_declared_languages`, which covers all three shapes `RecognizerListLoader._get_recognizer_languages` reads -- absent or `None`, a list of codes, and a list of `{language, context}` mappings -- rather than only the one the shipped file happens to use today. Generated-by: Claude Opus 5 * fix(analyzer): keep the kr alias on KrRrnRecognizer and KrPassportRecognizer Per review, `kr` goes only from the three entries that can have no legacy users. `KrDriverLicenseRecognizer`, `KrBrnRecognizer` and `KrFrnRecognizer` have only ever defaulted to `supported_language="ko"`, so no configuration can depend on `kr` for them. `KrRrnRecognizer` and `KrPassportRecognizer` are a deprecation question instead: both defaulted to `kr` until it was moved to `ko` -- #1742 (2025-10-08) and #2170 (2026-08-05) respectively -- so a registry configured against the old default would go quiet if the alias were dropped now. They keep `kr`, and the removal is deferred to a later release. The language guard gains a carve-out naming those two entries, why the alias is there, and the release it goes away in. That also restores the `kr` case of `test_loads_from_default_recognizers_yaml`, which now asserts a documented alias rather than the defect it was reverted for. Generated-by: Claude Opus 5 * test(analyzer): assert the deprecated kr alias still loads its recognizers The carve-out in the language guard records why `kr` survives on two entries but does not prove the reason is real. This builds a registry through `RecognizerRegistryProvider` with `supported_languages: ["kr"]` and the Korean entries enabled, and asserts that exactly `KrRrnRecognizer` and `KrPassportRecognizer` load, both under `kr`. That is the configuration the alias exists for. The loader builds one instance per declared language and `RecognizerListLoader._is_language_supported_globally` then drops every instance whose language the registry does not list, so the entries that declare only `ko` are built and discarded. Remove the alias and this registry loads nothing, which is the breakage the deprecation is staged to avoid. Generated-by: Claude Opus 5 * docs(changelog): note that the kr language code is deprecated Records the user-visible half of the change: `kr` is deprecated in favour of the ISO 639-1 code `ko`, the alias survives on `KrRrnRecognizer` and `KrPassportRecognizer` for registries configured against their original class defaults, and it is gone from the other three entries as of this release. The repository has no migration or upgrade guide -- `docs/project_transition.md` covers the move to Data Privacy Stack and `docs/build_release.md` the release machinery -- so this goes under a new Analyzer/Deprecated heading, which is where the changelog already records this kind of change. Generated-by: Claude Opus 5 * docs(changelog): move the kr deprecation note under Analyzer/Changed Per review, the changelog should not grow a new `Deprecated` heading for this. The note moves verbatim into the Analyzer component's `Changed` section, which did not exist under `[unreleased]` yet and is created here in the Added/Changed/Fixed order the 2.2.363 release already uses. Text is unchanged, `<release TBD>` placeholder included. Generated-by: Claude Opus 5 --------- Co-authored-by: Omri Mendels <omri374@users.noreply.github.com>
This commit is contained in:
채희원
@@ -13,6 +13,9 @@ All notable changes to this project will be documented in this file.
|
||||
- Added per-recognizer and per-entity score threshold configuration in the recognizer registry YAML, with the analyzer's global `default_score_threshold` as the fallback (#2116) (Thanks @rodboev)
|
||||
- Added `PhUmidRecognizer` for Philippine Unified Multi-Purpose ID (UMID/CRN) numbers in dashed and plain 12-digit formats; disabled by default (#2045) (Thanks @Surya-5555)
|
||||
|
||||
#### Changed
|
||||
- The `kr` language code is deprecated in favor of the ISO 639-1 code `ko`. `KrRrnRecognizer` and `KrPassportRecognizer` still accept `kr` in `supported_languages` as a backward-compatibility alias for registries configured against their original class defaults; the alias will be removed in <release TBD>. `KrBrnRecognizer`, `KrDriverLicenseRecognizer` and `KrFrnRecognizer` have only ever defaulted to `ko`, so `kr` was removed from their `default_recognizers.yaml` entries in this release. (#2236)
|
||||
|
||||
#### Fixed
|
||||
- `PhoneRecognizer.DEFAULT_SUPPORTED_REGIONS` used `"UK"`, which is not a valid `phonenumbers` (libphonenumber) region code — region codes are ISO 3166-1 alpha-2, where the United Kingdom is `"GB"`. The `"UK"` entry was a no-op, so UK numbers in national/local format (e.g. `020 7946 0958`) were never detected by default; only international-format `+44 …` numbers matched, because they carry the country code and match under any region. Replaced `"UK"` with `"GB"`.
|
||||
- Language model recognizers (`BasicLangExtractRecognizer`, `AzureOpenAILangExtractRecognizer`) configured in a recognizer registry YAML now honour `config_path` (and other recognizer-specific kwargs). Previously these entries were validated by the strict `PredefinedRecognizerConfig` schema, which has no `config_path` field and does not allow extra keys, so `config_path` was silently dropped and the recognizer fell back to its bundled default model configuration. Added a `LangExtractRecognizerConfig` model (`extra="allow"`) and registered both recognizer class names in `CONFIG_MODEL_MAP`.
|
||||
|
||||
@@ -160,13 +160,6 @@ recognizers:
|
||||
enabled: false
|
||||
country_code: uk
|
||||
|
||||
- name: UkPostcodeRecognizer
|
||||
supported_languages:
|
||||
- en
|
||||
type: predefined
|
||||
enabled: false
|
||||
country_code: uk
|
||||
|
||||
- name: UkPostcodeRecognizer
|
||||
supported_languages:
|
||||
- en
|
||||
@@ -345,7 +338,6 @@ recognizers:
|
||||
- name: KrBrnRecognizer
|
||||
supported_languages:
|
||||
- ko
|
||||
- kr
|
||||
type: predefined
|
||||
enabled: false
|
||||
country_code: kr
|
||||
@@ -361,7 +353,6 @@ recognizers:
|
||||
- name: KrDriverLicenseRecognizer
|
||||
supported_languages:
|
||||
- ko
|
||||
- kr
|
||||
type: predefined
|
||||
enabled: false
|
||||
country_code: kr
|
||||
@@ -369,7 +360,6 @@ recognizers:
|
||||
- name: KrFrnRecognizer
|
||||
supported_languages:
|
||||
- ko
|
||||
- kr
|
||||
type: predefined
|
||||
enabled: false
|
||||
country_code: kr
|
||||
|
||||
@@ -140,10 +140,11 @@ def test_accepts_name_kwarg():
|
||||
def test_loads_from_default_recognizers_yaml(language):
|
||||
"""Recognizer is registered in the default YAML and loads once enabled.
|
||||
|
||||
The constructor default is ``ko`` (see above), but the shipped entry
|
||||
advertises ``kr`` as well, matching the four sibling ``Kr*`` entries already
|
||||
in the file. Both codes are asserted here because an entry that lists a
|
||||
language it cannot serve is the same class of defect this PR fixes.
|
||||
The constructor default is ``ko`` (see above), but the shipped entry also
|
||||
advertises ``kr`` as a deprecated alias, kept because the class defaulted to
|
||||
``kr`` until #2170 (2026-08-05) and a registry configured against that
|
||||
default would otherwise go quiet. Both codes are asserted here so the alias
|
||||
cannot be dropped without this test noticing.
|
||||
"""
|
||||
conf = Path(presidio_analyzer.__file__).parent / "conf" / "default_recognizers.yaml"
|
||||
recognizers = yaml.safe_load(conf.read_text(encoding="utf-8"))["recognizers"]
|
||||
|
||||
@@ -13,6 +13,9 @@ from presidio_analyzer import (
|
||||
RecognizerRegistry,
|
||||
)
|
||||
from presidio_analyzer.recognizer_registry import RecognizerRegistryProvider
|
||||
from presidio_analyzer.recognizer_registry.recognizers_loader_utils import (
|
||||
RecognizerListLoader,
|
||||
)
|
||||
from presidio_analyzer.predefined_recognizers import SpacyRecognizer, UsSsnRecognizer
|
||||
|
||||
|
||||
@@ -733,3 +736,159 @@ def test_when_newly_registered_yaml_recognizers_enabled_then_they_load(tmp_path)
|
||||
).create_recognizer_registry()
|
||||
|
||||
assert targets <= _recognizer_class_names(registry)
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# ``default_recognizers.yaml`` hygiene
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
def _default_recognizers_conf():
|
||||
conf_path = (
|
||||
Path(__file__).parent.parent
|
||||
/ "presidio_analyzer"
|
||||
/ "conf"
|
||||
/ "default_recognizers.yaml"
|
||||
)
|
||||
return yaml.safe_load(conf_path.read_text())
|
||||
|
||||
|
||||
def _entry_name(entry):
|
||||
"""Return the instance name an entry declares.
|
||||
|
||||
``RecognizerListLoader.get_recognizer_name`` answers a different question:
|
||||
it resolves the *class* to instantiate and so prefers ``class_name``. That
|
||||
makes it the wrong key for the duplicate check below, where two entries may
|
||||
legitimately share a class as long as ``name`` tells the instances apart.
|
||||
It is still the right fallback for a bare-string entry such as
|
||||
``- SpacyRecognizer`` and for a dict that names only a class, because there
|
||||
the class name is also the instance name.
|
||||
"""
|
||||
if isinstance(entry, dict) and entry.get("name"):
|
||||
return entry["name"]
|
||||
return RecognizerListLoader.get_recognizer_name(entry)
|
||||
|
||||
|
||||
def _declared_languages(entry):
|
||||
"""Return every language code an entry declares, in all the loader's shapes.
|
||||
|
||||
``RecognizerListLoader._get_recognizer_languages`` reads
|
||||
``supported_languages`` three ways: absent or ``None``, in which case one
|
||||
recognizer is built per registry language and the entry declares no code of
|
||||
its own; a list of plain codes; or a list of
|
||||
``{"language": ..., "context": ...}`` mappings. A bare-string entry carries
|
||||
no configuration at all and falls into the first case. Only the second and
|
||||
third declare anything for this test to check.
|
||||
"""
|
||||
if isinstance(entry, str):
|
||||
return []
|
||||
languages = entry.get("supported_languages")
|
||||
if not languages:
|
||||
return []
|
||||
return [
|
||||
language if isinstance(language, str) else language["language"]
|
||||
for language in languages
|
||||
]
|
||||
|
||||
|
||||
def test_default_recognizers_yaml_has_no_duplicate_entries():
|
||||
"""A name listed twice is instantiated twice.
|
||||
|
||||
``UkPostcodeRecognizer`` was added by #1858 and again by #1857, which
|
||||
merged later, so the file carried two identical blocks and the registry
|
||||
built two identical recognizers. Results happen to dedupe downstream, so
|
||||
nothing looked wrong from the outside while the regexes ran twice.
|
||||
|
||||
Worse, construction is per entry but removal is per class:
|
||||
``RecognizerRegistryProvider.__remove_disabled_nlp_recognizers`` resolves
|
||||
the not-enabled entries to classes and drops every instance of those
|
||||
classes, so enabling one of a same-class pair and leaving the other
|
||||
disabled builds the recognizer and then silently removes it again.
|
||||
|
||||
Keyed on the instance name rather than the class, deliberately: two entries
|
||||
of the same class are legitimate as long as ``class_name`` carries the class
|
||||
and ``name`` distinguishes the instances. What the shipped file must not
|
||||
carry is the same name twice.
|
||||
"""
|
||||
names = [_entry_name(entry) for entry in _default_recognizers_conf()["recognizers"]]
|
||||
duplicates = sorted({name for name in names if names.count(name) > 1})
|
||||
|
||||
assert duplicates == [], f"duplicate recognizer entries: {duplicates}"
|
||||
|
||||
|
||||
def test_default_recognizers_yaml_declares_languages_not_countries():
|
||||
"""``supported_languages`` holds language codes; ``country_code`` holds the country.
|
||||
|
||||
The Korean entries listed ``kr`` — the ISO 3166-1 country code — next to
|
||||
``ko``, the ISO 639-1 language code. The loader builds one recognizer per
|
||||
listed language, so on a registry that does not name ``kr`` every such
|
||||
instance is built and then dropped by
|
||||
``RecognizerListLoader._is_language_supported_globally``, one warning
|
||||
apiece, on every registry build.
|
||||
|
||||
``ConfigurationValidator.validate_language_codes`` does not catch this:
|
||||
``kr`` is a well-formed two-letter code, just not the right one.
|
||||
"""
|
||||
# ISO 639-1 codes the shipped recognizers are written against. Adding one
|
||||
# here should be a deliberate decision, not a side effect of a new entry.
|
||||
known_languages = {"de", "en", "es", "fi", "fr", "it", "ko", "pl", "sv", "th", "tr"}
|
||||
|
||||
# ``kr`` survives on these two entries as a backward-compatibility alias,
|
||||
# not as a language. Both classes defaulted to ``supported_language="kr"``
|
||||
# until it was moved to ``ko`` — #1742 (2025-10-08) for the RRN, #2170
|
||||
# (2026-08-05) for the passport — so a registry still configured with the
|
||||
# old code would go quiet if the alias were dropped now. The three sibling
|
||||
# ``Kr*`` recognizers never had a ``kr`` default and carry no such
|
||||
# allowance.
|
||||
#
|
||||
# Deprecated in favour of ``ko`` and scheduled for removal in
|
||||
# <release TBD>. Delete these two entries in the same change.
|
||||
deprecated_language_aliases = {
|
||||
"KrRrnRecognizer": {"kr"},
|
||||
"KrPassportRecognizer": {"kr"},
|
||||
}
|
||||
|
||||
offenders = {}
|
||||
for entry in _default_recognizers_conf()["recognizers"]:
|
||||
name = _entry_name(entry)
|
||||
allowed = known_languages | deprecated_language_aliases.get(name, set())
|
||||
for code in _declared_languages(entry):
|
||||
if code not in allowed:
|
||||
offenders.setdefault(name, []).append(code)
|
||||
|
||||
assert offenders == {}, f"non-language codes in supported_languages: {offenders}"
|
||||
|
||||
|
||||
def test_deprecated_kr_alias_still_loads_its_recognizers():
|
||||
"""The retained ``kr`` alias has to actually serve a registry that names it.
|
||||
|
||||
This is the configuration the carve-out above exists for. With
|
||||
``supported_languages: ["kr"]`` the loader builds one instance per declared
|
||||
language and ``_is_language_supported_globally`` then drops every instance
|
||||
whose language the registry does not list, so only the entries that still
|
||||
declare ``kr`` survive. Drop the alias and this registry loads nothing —
|
||||
which is the breakage the deprecation is being staged to avoid.
|
||||
"""
|
||||
korean_entries = [
|
||||
entry
|
||||
for entry in _default_recognizers_conf()["recognizers"]
|
||||
if _entry_name(entry).startswith("Kr")
|
||||
]
|
||||
assert {"KrRrnRecognizer", "KrPassportRecognizer"} <= {
|
||||
_entry_name(entry) for entry in korean_entries
|
||||
}, "the two alias entries are missing from the shipped file"
|
||||
|
||||
registry = RecognizerRegistryProvider(
|
||||
registry_configuration={
|
||||
"supported_languages": ["kr"],
|
||||
"recognizers": [dict(entry, enabled=True) for entry in korean_entries],
|
||||
}
|
||||
).create_recognizer_registry()
|
||||
|
||||
assert _recognizer_class_names(registry) == {
|
||||
"KrRrnRecognizer",
|
||||
"KrPassportRecognizer",
|
||||
}
|
||||
# The three entries that only declare ``ko`` are built and then dropped,
|
||||
# so nothing survives under a language the registry never asked for.
|
||||
assert {rec.supported_language for rec in registry.recognizers} == {"kr"}
|
||||
|
||||
Reference in New Issue
Block a user